context_drm_gl: add support for hdr metadata #15106

Dudemanguy · 2024-10-15T22:21:49Z

Somebody please actually test since I have no actual hdr capable monitors. There's also a HDMI_EOTF_TRADITIONAL_GAMMA_HDR enum in there but I have no idea what on earth that was supposed to be so I ignored it.

Also not sure if any existing --drm-format options are capable enough here.

github-actions · 2024-10-15T22:37:33Z

Download the artifacts for this pull request:

Windows

macOS

video/out/drm_common.c

Dudemanguy · 2024-10-16T21:21:47Z

I added some sanity checking to this by using libdisplay-info. Only bothered with static metadata for now. In theory if the display doesn't support what the target_params specify it should not attempt to use it and fallback to sdr mode.

Of course it's totally possible I did the checks wrong so testing appreciated.

Edit: And of course they are wrong.

juw · 2024-10-25T09:03:53Z

I tested this with a HDR (DisplayHDR 600) capable monitor and the monitor confirmed it went into HDR mode. It seems to work fine. I used the following config:

vo=gpu-next
drm-connector=HDMI-A-1
drm-format=xrgb2101010
target-colorspace-hint
target-prim=dci-p3
target-trc=pq
target-peak=600

Dudemanguy · 2024-10-25T11:08:00Z

Off the top of my head, I still should add a check to make sure the monitor supports bt 2020 to this. I'm not sure if there's anything else that would be good to check.

quietvoid · 2024-10-25T11:20:28Z

For TVs to playback bt.2020 correctly, the Colorspace prop must be set if the connector supports it in the colorimetry data block (EDID).
Can probably be used to detect support, too.

Dudemanguy · 2024-10-25T17:25:53Z

For TVs to playback bt.2020 correctly, the Colorspace prop must be set if the connector supports it in the colorimetry data block (EDID).

Made an attempt at this. There's quite a few colorspaces but it seems that mutter and kwin both only care about bt2020_rgb so that is what I copied as well. Basically if we get an hdr one, set the colorspace property to bt2020_rgb. If not, set it back to the default one. Not sure if this is strictly correct of course, but there doesn't seem to be much prior art on this.

BergmannAtmet · 2024-10-31T15:57:00Z

The rendering issue I had is confirmed to be an AMD HDMI bug. So this PR is good to go IMHO.

Dudemanguy · 2024-10-31T16:13:26Z

I think the drm code is good. The only problem is what kasper mentioned earlier about gpu_next not passing the correct metadata to the backend. I haven't taken the time to look into that to see if there's a nice way to do that without libplacebo api changes.

Dudemanguy · 2024-11-03T03:01:30Z

I added a couple of commits that essentially passes the colorspace from the source directly to the target params and allows the backend to handle it (i.e. drm in this case). But I dunno if this is really correct.

I also added a commit to enable --target-colorspace-hint by default because it makes no sense to disable it.

video/out/opengl/context_drm_egl.c

video/out/vo_gpu_next.c

kasper93

Looks good. At least with gpu-next it will do the job.

na-na-hi · 2024-11-03T04:54:55Z

video/out/vo_gpu_next.c

    if (p->next_opts->target_hint && frame->current) {
-        struct pl_color_space hint = frame->current->params.color;
+        hint = frame->current->params.color;
+        if (p->ra_ctx->fns->pass_colorspace && p->ra_ctx->fns->pass_colorspace(p->ra_ctx))


pass_colorspace doesn't do what it says because it doesn't actually control colorspace passing: currently the pass is always attempted and this function only shows whether the attempt is successful.

I think this should be changed so that pass is only attempted if this function is called, when target-colorspace-hint is set, and don't attempt otherwise. This makes the behavior the same as pl_swapchain_colorspace_hint.

I think this should be changed so that pass is only attempted if this function is called, when target-colorspace-hint is set, and don't attempt otherwise. This makes the behavior the same as pl_swapchain_colorspace_hint.

I think it doesn't make sense to make it conditional if it is already based on target_params structure. Also it makes little sense to not signal colorspace. target-colorspace-hint should be removed, but that's outside of the scope of this PR. It is job for the player to render and send image in proper colorspace and metadata. IMHO sRGB fallback is ancient artifact, many monitors doesn't even have sRGB mode, so it is wrong.

The idea behind the naming was that vo_gpu_next passes the colorspace directly to the backend without going through libplacebo. Hence pass_colorspace. If you have a better name, I don't mind changing it.

Also it should already only be attempted if --target-colorspace-hint is set? Or am I misunderstanding what you mean here?

It is job for the player to render and send image in proper colorspace and metadata.

I am the one who know my display capability and quirks so I should be the one who control this setting. All desktop environments which support HDR have settings to let you enable/disable "HDR mode" which is the equivalent of this PR. mpv should also provide this control.

Also it should already only be attempted if --target-colorspace-hint is set?

vo_drm_set_hdr_metadata is always called no matter the --target-colorspace-hint setting. The intention is that the current name pass_colorspace is fine if vo_drm_set_hdr_metadata is not called without --target-colorspace-hint set.

vo_drm_set_hdr_metadata is always called

Sure but it should effectively be a no-op if you aren't using --target-colorspace-hint. The actual option is private to vo_gpu_next and changing that would be kind of annoying and not really needed.

Sure but it should effectively be a no-op if you aren't using --target-colorspace-hint.

Some parameters in struct hdr_output_metadata are always set even when video colorspace isn't HDR. This can change display behavior. context_drm_egl is also used by vo_gpu and it won't be aware of the underlying change.

The actual option is private to vo_gpu_next and changing that would be kind of annoying and not really needed.

You can just make pass_colorspace call set a flag in drm context which indicates that flips should set hdr metadata or not. Without pass_colorspace being called, the flag remains inactive.

This can change display behavior.

Does it? I would expect drivers to no-op the hdr metadata parameters if you aren't trying to use the HDR mode. It would be pretty broken if they actually tried to do anything with it. But OK it doesn't hurt to be paranoid I guess.

vo_gpu

This has many problems in this area that I'm not going to bother fixing.

Hmm but this proposed change is actually pretty annoying in reality to implement because it messes up the "restore back to sdr" logic. We do still need to do the modeset and set "metadata" (in reality signal that we're going back to sdr) in this case.

@na-na-hi: Yeah the additional code complexity is not worth it IMO unless you are aware of an actual concrete problem. Setting the metadata blob with SDR works exactly as expected for me. Setting the blob whenever the params change from the VO is the cleanest and best way to handle it. If you want to make vo_gpu aware of various --target options, that's fine but I'm not going to do it in this patch.

If you don't want to do it it's fine for now. I'm planning to lift target-colorspace-hint from gpu-next to VO options so that vo_gpu and vo_drm can use it.

na-na-hi · 2024-11-03T23:21:27Z

video/out/drm_common.c

+    while (*exts && !(cta = di_edid_ext_get_cta(*exts++)));
+
+    if (!cta) {
+        MP_WARN(drm, "Failed to get CTA-861 extension block.\n");


Does this have to be a warning? It's annoying if the display doesn't support this block.

Probably just a mistake on my part. Should be verbose like the rest.

Maybe also make it "not available" instead of "failed"? The later implies some error, while I think it is expected outcome.

Made it say Unable to find CTA-861 extension block. now

Dudemanguy · 2024-11-04T14:50:27Z

Will merge later today if there are no further comments.

It seems what we can do on this level is a bit limited but it's better than nothing. Closes mpv-player#8219.

Prevents vo_get_target_params from segfaulting if called somewhere else in the code since the underlying p->renderer was just freed.

libdrm unfortunately doesn't give us what is actually supported by the connector in question. To do that, we would have to parse the edid blob. Use libdisplay-info to do this and make it required for drm support. Using what we get back from the library, we can do a bit of sanity checking for hdr metadata to make sure that the display in question can handle it before we try setting the metadata. Strictly speaking, libdisplay-info has stuff for dynamic metadata that we could potentially use, but as a first pass and for simplicity, only static is considered for now.

Needed for drm to build now.

Only used if we have drm. Avoids a -Wunused-function error.

The libplacebo colorspace hint api makes sense for things like vulkan, but for other APIs like drm that are capable of directly handling colorspaces and hdr metadata it's a nuisance. Add a pass_colorspace fns that signals to vo_gpu_next that the backend doesn't need the color information to be manipulated and can handle the metadata itself.

It simply does not make any sense to not signal the correct colorspace and metadata by default.

Dudemanguy · 2024-11-04T19:35:21Z

Dropped one additional message from warn down to verbose.

hooke007 · 2024-11-06T06:16:41Z

23843b4#comments

Have no idea that a Linux-only related PR contains a global setting change?

hooke007 · 2024-11-06T06:22:02Z

Mac doesn't prepare well for this option neither.
#12666

hooke007 · 2024-11-06T06:25:12Z

Also in real world, the experience of countless devices's hdr passthrough is just a disaster. Retonemapping to SDR is always the better choice in recent years.

kasper93 · 2024-11-06T08:06:27Z

Also in real world, the experience of countless devices's hdr passthrough is just a disaster. Retonemapping to SDR is always the better choice in recent years.

This is 100% true, but we cannot be stuck behind assumptions like that. I think there is no correct way, there are multiple of factors that may make you use one or the other mode. If we want to stay with sRGB forever why even bother implementing all this protocols. Either way, it is a choice, many people are requesting passthrough, because they bought new fancy HDR displays. You can opt-out if needed.

Of course it is up to discussion. I think for best experience you have to tune the parameters either way. Even if you decide to tone-map to sRGB, you likely need to set target-peak. Almost never default values are optimal.

na-na-hi · 2024-11-06T12:30:53Z

This is 100% true, but we cannot be stuck behind assumptions like that. I think there is no correct way, there are multiple of factors that may make you use one or the other mode. If we want to stay with sRGB forever why even bother implementing all this protocols. Either way, it is a choice, many people are requesting passthrough, because they bought new fancy HDR displays. You can opt-out if needed.

Meanwhile hwdec is disabled by default for the same argument, and HDR passthrough has more problem with real-world implementations than hardware decoders nowadays. Since there are clearly problems with defaulting target-colorspace-hint to yes and mpv's current policy is to behave as consistent as possible across a large range of hardware, I think this should be reverted for now.

Dudemanguy force-pushed the drm-hdr branch 9 times, most recently from 7c71e5c to 91aab79 Compare October 16, 2024 12:31

kasper93 reviewed Oct 16, 2024

View reviewed changes

video/out/drm_common.c Outdated Show resolved Hide resolved

Dudemanguy force-pushed the drm-hdr branch 4 times, most recently from ea72864 to c045862 Compare October 16, 2024 21:10

Dudemanguy force-pushed the drm-hdr branch 2 times, most recently from 45a918e to d621383 Compare October 16, 2024 22:40

Dudemanguy marked this pull request as draft October 17, 2024 05:20

Dudemanguy force-pushed the drm-hdr branch from d621383 to dc86342 Compare October 17, 2024 06:08

Dudemanguy force-pushed the drm-hdr branch from dc86342 to 161ea43 Compare October 25, 2024 16:47

Dudemanguy marked this pull request as ready for review October 25, 2024 16:47

Dudemanguy marked this pull request as draft October 25, 2024 16:51

Dudemanguy force-pushed the drm-hdr branch from 161ea43 to 580c6c2 Compare October 25, 2024 17:22

Dudemanguy marked this pull request as ready for review October 25, 2024 17:25

Dudemanguy force-pushed the drm-hdr branch from 580c6c2 to 3086683 Compare October 25, 2024 17:33

Dudemanguy force-pushed the drm-hdr branch from b1c6a84 to dac4900 Compare October 29, 2024 16:00

kasper93 reviewed Nov 3, 2024

View reviewed changes

video/out/opengl/context_drm_egl.c Outdated Show resolved Hide resolved

video/out/vo_gpu_next.c Outdated Show resolved Hide resolved

video/out/vo_gpu_next.c Show resolved Hide resolved

Dudemanguy force-pushed the drm-hdr branch 3 times, most recently from ec6dc60 to f0e1efe Compare November 3, 2024 03:59

kasper93 approved these changes Nov 3, 2024

View reviewed changes

na-na-hi reviewed Nov 3, 2024

View reviewed changes

Dudemanguy force-pushed the drm-hdr branch 2 times, most recently from 92fa692 to 7e30112 Compare November 3, 2024 23:33

Dudemanguy added 7 commits November 4, 2024 13:19

context_drm_gl: add support for hdr metadata

d3eeb9f

It seems what we can do on this level is a bit limited but it's better than nothing. Closes mpv-player#8219.

vo_gpu: explicitly set target_params to NULL on uninit

e70d926

Prevents vo_get_target_params from segfaulting if called somewhere else in the code since the underlying p->renderer was just freed.

github/workflows: install libdisplay-info on freebsd

f251d27

Needed for drm to build now.

wayland: guard devices_are_equal completely

d8ab9ca

Only used if we have drm. Avoids a -Wunused-function error.

vo_gpu_next: set --target-colorspace-hint to yes by default

3a172b1

It simply does not make any sense to not signal the correct colorspace and metadata by default.

Dudemanguy force-pushed the drm-hdr branch from 7e30112 to 3a172b1 Compare November 4, 2024 19:20

Dudemanguy merged commit 23843b4 into mpv-player:master Nov 4, 2024
25 checks passed

Dudemanguy deleted the drm-hdr branch November 4, 2024 19:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

context_drm_gl: add support for hdr metadata #15106

context_drm_gl: add support for hdr metadata #15106

Dudemanguy commented Oct 15, 2024 •

edited

Loading

github-actions bot commented Oct 15, 2024 •

edited

Loading

Dudemanguy commented Oct 16, 2024 •

edited

Loading

juw commented Oct 25, 2024

Dudemanguy commented Oct 25, 2024

quietvoid commented Oct 25, 2024 •

edited

Loading

Dudemanguy commented Oct 25, 2024

BergmannAtmet commented Oct 31, 2024

Dudemanguy commented Oct 31, 2024

Dudemanguy commented Nov 3, 2024 •

edited

Loading

kasper93 left a comment

na-na-hi Nov 3, 2024 •

edited

Loading

kasper93 Nov 3, 2024 •

edited

Loading

Dudemanguy Nov 3, 2024 •

edited

Loading

na-na-hi Nov 3, 2024 •

edited

Loading

Dudemanguy Nov 3, 2024 •

edited

Loading

na-na-hi Nov 3, 2024

Dudemanguy Nov 3, 2024

Dudemanguy Nov 3, 2024 •

edited

Loading

Dudemanguy Nov 3, 2024

na-na-hi Nov 3, 2024

na-na-hi Nov 3, 2024

Dudemanguy Nov 3, 2024

kasper93 Nov 3, 2024

Dudemanguy Nov 3, 2024

Dudemanguy commented Nov 4, 2024

Dudemanguy commented Nov 4, 2024

hooke007 commented Nov 6, 2024 •

edited

Loading

hooke007 commented Nov 6, 2024

hooke007 commented Nov 6, 2024

kasper93 commented Nov 6, 2024 •

edited

Loading

na-na-hi commented Nov 6, 2024

context_drm_gl: add support for hdr metadata #15106

context_drm_gl: add support for hdr metadata #15106

Conversation

Dudemanguy commented Oct 15, 2024 • edited Loading

github-actions bot commented Oct 15, 2024 • edited Loading

Dudemanguy commented Oct 16, 2024 • edited Loading

juw commented Oct 25, 2024

Dudemanguy commented Oct 25, 2024

quietvoid commented Oct 25, 2024 • edited Loading

Dudemanguy commented Oct 25, 2024

BergmannAtmet commented Oct 31, 2024

Dudemanguy commented Oct 31, 2024

Dudemanguy commented Nov 3, 2024 • edited Loading

kasper93 left a comment

Choose a reason for hiding this comment

na-na-hi Nov 3, 2024 • edited Loading

Choose a reason for hiding this comment

kasper93 Nov 3, 2024 • edited Loading

Choose a reason for hiding this comment

Dudemanguy Nov 3, 2024 • edited Loading

Choose a reason for hiding this comment

na-na-hi Nov 3, 2024 • edited Loading

Choose a reason for hiding this comment

Dudemanguy Nov 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dudemanguy Nov 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dudemanguy commented Nov 4, 2024

Dudemanguy commented Nov 4, 2024

hooke007 commented Nov 6, 2024 • edited Loading

hooke007 commented Nov 6, 2024

hooke007 commented Nov 6, 2024

kasper93 commented Nov 6, 2024 • edited Loading

na-na-hi commented Nov 6, 2024

Dudemanguy commented Oct 15, 2024 •

edited

Loading

github-actions bot commented Oct 15, 2024 •

edited

Loading

Dudemanguy commented Oct 16, 2024 •

edited

Loading

quietvoid commented Oct 25, 2024 •

edited

Loading

Dudemanguy commented Nov 3, 2024 •

edited

Loading

na-na-hi Nov 3, 2024 •

edited

Loading

kasper93 Nov 3, 2024 •

edited

Loading

Dudemanguy Nov 3, 2024 •

edited

Loading

na-na-hi Nov 3, 2024 •

edited

Loading

Dudemanguy Nov 3, 2024 •

edited

Loading

Dudemanguy Nov 3, 2024 •

edited

Loading

hooke007 commented Nov 6, 2024 •

edited

Loading

kasper93 commented Nov 6, 2024 •

edited

Loading