-
Notifications
You must be signed in to change notification settings - Fork 3.2k
wayland: luminance improvements #17070
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
This patch relies on the assumption that the reference luminance can be compared to the max_target_luminance, which is not entirely clear to me: https://gitlab.freedesktop.org/pq/color-and-hdr/-/issues/52. However, mpv already relies on this property. |
b7b3384 to
bac8a9a
Compare
| if (sw->fns->target_csp) { | ||
| float source_max_luma = 0.0f; | ||
| if (frame->current) { | ||
| source_max_luma = frame->current->params.color.hdr.max_luma; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This value is base HDR10 value. And won't be in fact used by libplacebo if HDR10+/DV metadata is also available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do I get the correct value here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
something like this, but I'm still reluctant of using those values to infer any target parameters. Depending on the source this may or may not be valid/useful value. It may be transfer nominal value, which we have to avoid for example.
pl_color_space_nominal_luma_ex(pl_nominal_luma_params(
.color = &frame->current->params.color,
.metadata = PL_HDR_METADATA_ANY,
.scaling = PL_HDR_NITS,
.out_max = &source_max_luma,
));There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At some point libplacebo has to choose some value to determine if and how it needs to do tone mapping. That is the value I'm interested in. It is also ok if this value is too large since in that case the worst that should happen is that we de-optimize by using PQ instead of the preferred SDR transfer function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the code below again, we don't actually need the max_luma of the source. We only need to know if libplacebo is going to do tone mapping when targeting an SDR display, because that's the case in which it will need accurate metadata from the target.
// Otherwise, if the max_luma of the source is brighter than paper white, then
// libplacebo will always perform tone mapping. To do this correctly, it will
// need to know the exact max_luma of the display.
if (source_max_luma > PL_COLOR_SDR_WHITE)
csp.transfer = PL_COLOR_TRC_PQ;There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the first commit it would have to be
if (wd->csp.hdr.max_luma != PL_COLOR_SDR_WHITE) {See below for why not just >.
Also note that if requested luminances is lower than PL_COLOR_SDR_WHITE, we can also switch to PQ, but we can just ignore this. This is what we actually do, because we don't want to map SDR lower than nominal ref.
If the reference luminance is higher than the max target luminance, then the first commit will cause wd->csp.hdr.max_luma < PL_COLOR_SDR_WHITE. In this case I don't understand why you don't want to tone-map to max_luma. This can obviously only happen in weird situations like when the user sets his brightness higher than 100% but it is easy to handle. Does libplacebo not handle tone mapping to below PL_COLOR_SDR_WHITE correctly?
Either way, if ref_lum==max_lum we should be outputting "SDR" regardless of the actual luminances.
The first commit ensures that in that case wd->csp.hdr.max_luma == PL_COLOR_SDR_WHITE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason that I don't want to use this coarse hammer is that it means that mpv will always ignore the preferred transfer function when the user is using an HDR display even if mpv is playing an SDR video.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, if that is a piece of contention, then I can drop the second commit in favor of what you've suggested. The user-facing improvement is in the first commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason that I don't want to use this coarse hammer is that it means that mpv will always ignore the preferred transfer function when the user is using an HDR display even if mpv is playing an SDR video.
This is actually expected behavior. I prefer PQ output on HDR targets, even for SDR video. You can change it by using target-mode=source.
This is how =target mode works on Windows. Where target_csp is on the wire format, hence whenever HDR mode is enabled, we output PQ. And the conversion to PQ is done inside mpv.
I know Wayland is much smarter than that, and uses preferred transfer, which most likely is not related to the actual display, but if preferred transfer function is not compatible with mpv definition of HDR, I would prefer to keep it simple and avoid doing "complex" heuristics and fallback to PQ.
Feel free to correct my thinking, I don't have strong opinion how this should work. I just feel like the target_csp should not depend on source. Well the hint can depend on the source, so maybe I'm wrong here.
My thinking is, if Wayland preferred function is "HDR-like" gamma2.2, then we should respect the HDR of it. I understand that in some cases, the definition may be compatible with mpv's SDR output and we can use gamma2.2 in this case, but this is slightly messy with all the heuristics. But I still don't see why it should depend on source itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're not wrong. The problem is pl_color_transfer_is_hdr and the way it is used. As others have already pointed out, there are no HDR transfer functions as far as wayland is concerned. Ideally, all of the uses of that function should be removed from both mpv and libplacebo and the transfer function of the target hint should only be taken into account as late as possible: Once libplacebo knows everything there is to know about the image it is going to render, it decides if the preferred TF is suitable for that and otherwise switches to a different one.
Getting there would be a much larger change and my thinking was that going the other way and passing the source into the windowing backend would get us roughly the same result with much less effort.
Signed-off-by: Julian Orth <[email protected]>
Signed-off-by: Julian Orth <[email protected]>
bac8a9a to
c5066b3
Compare
|
I've completely rewritten the code that decides when to switch to PQ. I've resolved all related threads since the referenced code was outdated. There was indeed at least one case where inverse tone mapping was not taken into account. |
|
Download the artifacts for this pull request: Windows |
| // when ref_luma != PL_COLOR_SDR_WHITE. | ||
| float a = wd->min_luma; | ||
| float b = (PL_COLOR_SDR_WHITE - a) / (wd->ref_luma - a); | ||
| wd->csp.hdr.min_luma = (wd->csp.hdr.min_luma - a) * b + a; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This value can realistically become negative and not all components might handle this. So this should probably be wrapped in MPMAX.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PS: a and b are chosen such that paper white in the compositor space maps to paper white in libplacebo and paper black (SDR black, all bits 0) maps to itself. This is the inverse of how I've implemented this mapping in my compositor and KDE seems to do something similar.
You can test this with the following file: test_pattern-PQ.jxl.gz
This image contains gradients that go up to 10,000 cd/m^2. If you set the paper white luminance in your compositor to 1% of its normal value, you create a lot of artificial head room. Without this patch, the entire image becomes dark. With this patch, the bright end of the gradient continues to fully light up the output.