-
Notifications
You must be signed in to change notification settings - Fork 3.2k
wayland_common: rescale reference white to mpv's white #17339
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
Conversation
This always scalled min_luma to 0, effectivelly disabling all black point compensation code in tone mapping, additionally, it tries to scale to the "libplacebo space" while the requested preferred space is what libplacebo will target. The Wayland's target_luminance povides the luminance range that the image description is targeting as the minimum and maximum absolute luminance L. This is exactly what libplacebo expects. For SDR relative mapping to reference white, it has to be rescaled, this will be done in next commit. This reverts commit ae211f7.
In SDR everything is relative to display, as opposed to PQ. Scale it to 203 which is our internal reference white, to land white at 1.0 while preserving the requested contrast in preferred colorspace. Note that we already cheat a bit, because we clamp contrast to 1000:1 for SDR transfers, as this has been working for years, and SDR monitors doesn't really provide this information. Some compositors defaults to 80/0.2 nits (400:1) which is taken verbatim from 1999 sRGB specification that defined 0.2 nits as veiling glare for CRT displays at the time. It's no longer applicable in present era, and while many current display may have contrast in the range of 800:1, I argue that little bit of clipping is better on those low contast targets, instead of rising the brightness, on already low contrast anyway. The user can override this with `--target-contrast` and `--target-peak`. And either way, those luminance values doesn't have to refer to display at all, they are just a dynamic range that we have to target.
51a8061 to
6ae5d85
Compare
| // In SDR output everything is relative, so rescale reference white | ||
| // to 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.
Wayland luminance values are always relative.
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.
wp_image_description_info_v1::target_luminance
Provides the luminance range that the image description is targeting as the minimum and maximum absolute luminance L.
Relative to what? I think there is misunderstanding, based on some implementation details that I am not preview too.
The current code is not working, because it always maps min_luma to 0, which effectively makes all targets infinite contrast, which is not correct.
If ref=max, "SDR" case, we output 0-1 which maps to min and max of the display, it's display referred. 1.0 is display white, 0.0 is display black. Or colorspace volume, if you prefer to not talk display.
If ref!=max, that means we have some HDR/EDR capabilities. In which case we always switch to PQ, which is absolute scaled, where 0-1 maps to 0-10000 nits.
https://en.wikipedia.org/wiki/Perceptual_quantizer
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.
Relative to what?
Relative to the scale sent in the luminances event. The HDR headroom is always target_max/ref (we can ignore the offsets here since they don't make a difference at the upper end.)
The current code is not working, because it always maps min_luma to 0, which effectively makes all targets infinite contrast, which is not correct.
There is no way for the compositor to determine accurate target_min_luma and target_max_luma values for the output if the output is not operating in HDR mode. EDIDs do not contain these values. Assuming anything other than these values being made up will lead to incorrect results.
By treating target_max_luma/target_min_luma as the contrast of the display, you're ascribing meaning to the messages sent by the compositors that is not intended by the compositors and will lead to results that are unexpected.
I already proposed another solution that might fix this issue in practice in afaedac#diff-7cd33ead8491d06868d9ff3b01c25933493014124ad7c496a59dabeb434d97f4R2164-R2165
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.
Relative to the scale sent in the
luminancesevent. The HDR headroom is alwaystarget_max/ref(we can ignore the offsets here since they don't make a difference at the upper end.)
I don't really need "HDR headroom". All I need to our tone mapper is the dynamic range of the virtual target.
Could you give citation, where it says that those values are "relative" to luminances event? And what does this mean?
Say we have
min_luminance = 0.2 nits
min_target_luminance = 0.2 nits
What in what why they are relative?
Doesn't it mean that 0.0 value is referring to 0.2 nits? Whether the 0.2 nits makes sense is another question, but let's ignore this.
There is no way for the compositor to determine accurate target_min_luma and target_max_luma values for the output if the output is not operating in HDR mode. EDIDs do not contain these values. Assuming anything other than these values being made up will lead to incorrect results.
Correct. But those are the values we get. Why should mpv doubt that? After all compositor may have a tool to calibrate those values, may use other means to determine them.
Like on KDE, those values are connected to display brightness. And yes, they are made up, but scale between 200-600 nits. Of course 600 nits doesn't mean display luminance output, when it's at 10% brightness, but it's the value that when you plug into the math of tonemapper will produce the expected HDR headroom.
By treating target_max_luma/target_min_luma as the contrast of the display, you're ascribing meaning to the messages sent by the compositors that is not intended by the compositors and will lead to results that are unexpected.
It's not "contrast of the display". It's contrast of the requested target encoding. I'm not sure what you are trying to say. It's some starting value. I think better than using some hardcoded values in binary, compositors may give use better guess.
I already proposed another solution that might fix this issue in practice in afaedac#diff-7cd33ead8491d06868d9ff3b01c25933493014124ad7c496a59dabeb434d97f4R2164-R2165
This is still not correct. You are trying to convert target_luminance to some undefined space. PL_COLOR_SDR_WHITE / PL_COLOR_SDR_CONTRAST is only used as a fallback.
The color space that mpv targets is THE preferred one, i.e. c = wd->csp.hdr.min_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.
Could you give citation, where it says that those values are "relative" to luminances event? And what does this mean?
I gave the formulas below. Citation or not, this is how KDE and other compositors convert between luminance spaces.
But those are the values we get. Why should mpv doubt that?
[...]
It's contrast of the requested target encoding.
The values are not intended to be interpreted without offsetting. Again, see below.
The color space that mpv targets is THE preferred one, i.e. c = wd->csp.hdr.min_luma.
It's not because mpv doesn't forward the values from the luminances event to libplacebo and libplacebo assumes a space in which the reference luminance is 203. Nor does the vulkan API allow mpv to use that luminance space in general.
Therefore mpv is not targeting the luminance space advertised in the preferred image description.
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.
If ref<max, we output PQ, which is absolute.
All wayland image descriptions are relative, including those using PQ. If you don't believe me, you can ping Xaver. Or you can look at the godot HDR implementation which treats target_max as a relative value: https://github.com/godotengine/godot/pull/102987/changes#diff-cb17fff099312adf05f6c79715f92ded45d468a8b4d57f8ab9ee79d99fe174a6R1038-R1041
float RenderingContextDriverVulkan::surface_get_hdr_output_max_value(SurfaceID p_surface) const {
Surface *surface = (Surface *)(p_surface);
return MAX(surface->hdr_max_luminance / MAX(surface->hdr_reference_luminance, 1.0f), 1.0f);
}(this calculates the maximum values that should be stored in the floating point buffer used by godot)
and you can see that Xaver has reviewed this PR.
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.
So how does dmabuf-wayland works if you send it a PQ video? Do you have whitepaper on this Wayland PQ? We could also update Wikipedia.
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.
It works just like every other transfer function. Reference white is scaled to whatever is being rendered to.
You could just perform the experiment and see that with this PR mpv appears much darker. I would take a screenshot but SDR screenshots don't properly represent this.
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.
So, you argue that current master mpv is correct? It's bizarre to me that we ignore min_luminance?
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 the upper bound, it is correct (+- a negligible amount).
At the lower bound it is less clear. It's probably fine for OLED displays but not for other displays. I feel like there is a bit of a mismatch between what the protocol exposes and what mpv works with.
It's bizarre to me that we ignore min_luminance?
I don't know what you mean by this.
|
I believe this is incorrect because it will cause image descriptions that are treated as equivalent by compositors to be treated differently by mpv. I already gave an example in #17329 (comment) |
I'm happy to be educated what is the correct way. Let's start from beginning, to avoid misconceptions. When I get preferred descriptor to have What is the value 1.0 referring to? Wouldn't it be that 1.0 is
And this example is perfectly valid, one compositor requests 400:1 contrast, the other 20_000:1, both valid values and we respect that. But if this is wrong, please tell how to derive contrast value? |
1.0 refers to primary_max_lum and 0.0 refers to primary_min_lum. The Let's assume that
and a linear transfer function for simplicity. Then the maximum values that makes sense in the buffer is and the minimum value that makes sense is since values outside this range cannot be displayed. If you want to convert these values to the following primary luminance space
Then you convert as follows: and This is the formula that KDE and other compositors use to convert buffer values in shaders. If you solve this for the new luminances, you get the formula that mpv currently uses. Except that primary_min_lum of libplacebo is unknown. (primary_max_lum cancels out after solving for the luminance so it doesn't matter.) For example, for target_min_lum: solves to |
Exactly! So you confirm 400 and 0.2 are the values that we should use. 10000 is encoding range, 400 is usable range. We should factor this offset that you show here.
There is no such thing. Do you mean when we get preferred gamma2.2 descriptor, when we fallback to PQ we should convert this to PQ descriptor? So primary_min_lum would be 0.005 as defined default for PQ, which Vulkan WSI will not change.
Yeah, but I don't want to "convert", between color spaces. We have tone mapper do do that. We need the usable range on input. |
After they have been scaled as shown in the rest of my message.
Using the minimum luminance of the transfer function is something that I had considered. But I don't immediately see how it follows that this is correct.
The usable range is the range after scaling as discussed above. |
I don't see how using any other value would be correct. I still don't see how ref value affects PQ code points. It is bizarre to me that you renormalize PQ into some other space. I understand for any other transfer, but PQ is kinda special in it's formula. EDIT: But if that helps, our reference is quite simple PQ: 0-1 == 0-10000 nits |
In the formula The numerator of the fraction is
So min is in a really different scale than I feel like afaedac#diff-7cd33ead8491d06868d9ff3b01c25933493014124ad7c496a59dabeb434d97f4R2164-R2165 is the best we can do at this point. |
For PQ output if you argue that it somehow clips the PQ curve. We should rescale to natural 0-10000 nits range. We need those values to be in the same range/space as HDR metadata of the input we provide. Of course nothing is really absolute, but you make it sound like we need to convert to "libplacebo" space, while there is no such thing. Of course internally we are normalized to some internal value, but it has zero significant on output as we encode the output according to transfer functions. We do not support encodings where ref!=max except for PQ which is absolute. If you argue that PQ is not absolute and PQ codepoints are scaled also, that we don't support Wayland at all. But then again, neither is dmabuf-wayland which is sending the PQ image to compositor, which we do not rescale. The values that we set have to be compatible with the values of HDR metadata, which is also explicitly stated that they are in Wayland spec.
This case is easy, we just multiply by the PL_COLOR_SDR_WHITE/80 factor. The exact value doesn't matter if you normalize, when ref==max, such that 1.0 is white, 0.0 is black.
This is random arbitrary value not connected with anything. |
If target_min=primary_min, which is the critical case, then the new target_min is |
Exactly. Why do you think this value is not correct? We won't make it more correct by doing some magic scaling. Also as discussed on IRC, there is no "correct" value. The above value is correct for early 90' CRT, while it may not be that correct today, it's the value we get from preferred descriptor. That requests 400:1 contrast ratio. |
|
I don't believe using that value would deliver the best experience for mpv users. |
We already clamp to 1000:1 anyway, so it would fallback to that. I hope it was you on IRC, because there was long discussion about those values and why they likely never be correct. Frankly for me the only way is to tune those values by eye for your viewing environment. Which can be done manually, with some other tool, maybe compositor can do it. |
|
So something like this: diff --git a/video/out/wayland_common.c b/video/out/wayland_common.c
index 7acc0b0ef..6d6783264 100644
--- a/video/out/wayland_common.c
+++ b/video/out/wayland_common.c
@@ -2144,6 +2144,23 @@ static void info_done(void *data, struct wp_image_description_info_v1 *image_des
if (!wd->icc_file) {
MP_VERBOSE(wl, "Preferred surface feedback received:\n");
log_color_space(wl->log, wd);
+ if (fabsf(wd->csp.hdr.max_luma / wd->ref_luma - 1.0f) > 1e-4f &&
+ !pl_color_transfer_is_hdr(wd->csp.transfer)) {
+ MP_VERBOSE(wl, "Setting preferred transfer to PQ for HDR output.\n");
+ wd->csp.transfer = PL_COLOR_TRC_PQ;
+ }
+ float default_ref = 80.0f;
+ float default_min = 0.2f;
+ if (wd->csp.transfer == PL_COLOR_TRC_PQ) {
+ default_ref = 203.0f;
+ default_min = 0.005f;
+ } else if (wd->csp.transfer == PL_COLOR_TRC_BT_1886) {
+ default_ref = 100.0f;
+ default_min = 0.01f;
+ } else if (wd->csp.transfer == PL_COLOR_TRC_HLG) {
+ default_ref = 1000.0f;
+ default_min = 0.005f;
+ }
// Wayland luminances are always in reference to the reference luminance. That is,
// if max_luma == 2*ref_luma, then there is 2x headroom above paper white. On the
// other hand, libplacebo hardcodes PL_COLOR_SDR_WHITE as the reference luminance.
@@ -2151,13 +2168,14 @@ static void info_done(void *data, struct wp_image_description_info_v1 *image_des
// otherwise libplacebo will assume that there is too little or too much headroom
// when ref_luma != PL_COLOR_SDR_WHITE.
float a = wd->min_luma;
- float b = (PL_COLOR_SDR_WHITE - PL_COLOR_HDR_BLACK) / (wd->ref_luma - a);
- wd->csp.hdr.min_luma = (wd->csp.hdr.min_luma - a) * b + PL_COLOR_HDR_BLACK;
- wd->csp.hdr.max_luma = (wd->csp.hdr.max_luma - a) * b + PL_COLOR_HDR_BLACK;
+ float c = PL_COLOR_SDR_WHITE / default_ref * default_min;
+ float b = (PL_COLOR_SDR_WHITE - c) / (wd->ref_luma - a);
+ wd->csp.hdr.min_luma = (wd->csp.hdr.min_luma - a) * b + c;
+ wd->csp.hdr.max_luma = (wd->csp.hdr.max_luma - a) * b + c;
if (wd->csp.hdr.max_cll != 0)
- wd->csp.hdr.max_cll = (wd->csp.hdr.max_cll - a) * b + PL_COLOR_HDR_BLACK;
+ wd->csp.hdr.max_cll = (wd->csp.hdr.max_cll - a) * b + c;
if (wd->csp.hdr.max_fall != 0)
- wd->csp.hdr.max_fall = (wd->csp.hdr.max_fall - a) * b + PL_COLOR_HDR_BLACK;
+ wd->csp.hdr.max_fall = (wd->csp.hdr.max_fall - a) * b + c;
// Ensure that min_luma doesn't become negative.
wd->csp.hdr.min_luma = MPMAX(wd->csp.hdr.min_luma, 0.0);
// Since we want to do some exact comparisons of max_luma with PL_COLOR_SDR_WHITE,
@@ -2170,10 +2188,6 @@ static void info_done(void *data, struct wp_image_description_info_v1 *image_des
wd->csp.hdr.max_fall = MPMIN(wd->csp.hdr.max_fall, wd->csp.hdr.max_luma);
}
wl->preferred_csp = wd->csp;
- if (wd->csp.hdr.max_luma != PL_COLOR_SDR_WHITE && !pl_color_transfer_is_hdr(wd->csp.transfer)) {
- MP_VERBOSE(wl, "Setting preferred transfer to PQ for HDR output.\n");
- wl->preferred_csp.transfer = PL_COLOR_TRC_PQ;
- }
} else {
if (wl->icc_size) {
munmap(wl->icc_file, wl->icc_size);
I don't really see how clamping is any less arbitrary. |
|
When Consider simple case of mapping sRGB to sRGB which should be identity. For simplicity assume that PL_COLOR_SDR_WHITE==80. Consider this example: min_target_luminance=0.455432 will produce 0.2 ( |
Yes, that's what I said above:
And then you said that that was what you wanted:
|
I was responding to this, and probably didn't parse what exactly you meant.
Because this is what this PR currently does.
However the value depends on the provided descriptor. For my example from above, we would get: In your solution we always will get 0.2 default (scaled), this doesn't sounds correct. |
The value in the case target_min=primary_min cannot depend on the descriptor because vulkan will always use the default values which are 80/0.2 in most cases. |
I really doubt that. I'm reading KWin code. |
You don't need to read the code. The protocol spells it out:
It does not make an exception for PQ. |
This doesn't say anything about PQ. Especially that it can be somehow "scaled" as you imply. |
|
Ok. I don't really see a need to argue about this. Just hack libplacebo to use passthrough and manually create a PQ image description but with ref=100 and you'll see that the compositor increases the brightness of the image by 2x. |
|
Or maybe even simpler hack mesa to set this value in the image description it creates. diff --git a/src/vulkan/wsi/wsi_common_wayland.c b/src/vulkan/wsi/wsi_common_wayland.c
index 79ddda92f48..d15373047f0 100644
--- a/src/vulkan/wsi/wsi_common_wayland.c
+++ b/src/vulkan/wsi/wsi_common_wayland.c
@@ -1315,9 +1315,12 @@ wsi_wl_swapchain_update_colorspace(struct wsi_wl_swapchain *chain)
wp_image_description_creator_params_v1_set_primaries_named(creator, primaries);
wp_image_description_creator_params_v1_set_tf_named(creator, tf);
+ uint32_t divider = 4;
+ wp_image_description_creator_params_v1_set_luminances(creator, 50, 10000, 203 / divider);
if (should_use_hdr_metadata) {
- wp_image_description_creator_params_v1_set_max_cll(creator, wayland_hdr_metadata.max_cll);
- wp_image_description_creator_params_v1_set_max_fall(creator, wayland_hdr_metadata.max_fall);
+ // also reduce the other values to prevent the compositor from tone mapping itself
+ wp_image_description_creator_params_v1_set_max_cll(creator, wayland_hdr_metadata.max_cll / divider);
+ wp_image_description_creator_params_v1_set_max_fall(creator, wayland_hdr_metadata.max_fall / divider);
if (display->color_features.mastering_display_primaries) {
uint32_t red_x = round(chain->color.hdr_metadata.displayPrimaryRed.x * 1000000);
uint32_t red_y = round(chain->color.hdr_metadata.displayPrimaryRed.y * 1000000);
@@ -1337,8 +1340,8 @@ wsi_wl_swapchain_update_colorspace(struct wsi_wl_swapchain *chain)
*/
if (wayland_hdr_metadata.max_luminance != 0) {
wp_image_description_creator_params_v1_set_mastering_luminance(creator,
- wayland_hdr_metadata.min_luminance,
- wayland_hdr_metadata.max_luminance);
+ wayland_hdr_metadata.min_luminance / divider,
+ wayland_hdr_metadata.max_luminance / divider);
}
}
} |
Ok, so PQ is absolute, but is later mapped into the same lightness based of reference value in the descriptor. Now we are clear on this.
This looks reasonable, and what will work mechanically to output the signal in "expected" range. However I don't think this is correct for PQ, where we would set different minimum luminance, through the WSI, so we shouldn't use default_min and instead use scaled source min. For SDR, it will get the same scaling as if we just rescale ref/ref, but this is working fine. I'm thinking we could simplify this to actually set the luminances value through protocol and avoid adjusting the the "default" values that mesa would set. Then we would just scale values exactly like this PR do, but set the reference on sRGB descriptor so the compositor remaps it for us. And still this is only a problem in those ref!=max sdr. Speaking of which direct scanout is probably allowed only if we set exact descriptor as the preferred one? Setting different ref would require compositor to convert it anyway. I could also add adjustable ref luminace in libplacebo, but frankly it's convenient to have everything in single point of reference there. Generally it's not even necessarly needed, as it would work just fine with gamma2.2 transfer and max_luma > 203, but we currently don't allow it and implicitly switch to PQ when this happens. Do you think it would be useful to support? |
|
Will fix this in a different way. |
No description provided.