vo_gpu_next: set color directly using Wayland protocol#17345
vo_gpu_next: set color directly using Wayland protocol#17345kasper93 wants to merge 9 commits intompv-player:masterfrom
Conversation
kasper93
force-pushed
the
wayland-color
branch
4 times, most recently
from
75522dd to
e9ec715
Compare
|
@mahkoh: This should work, and allow us better control over how things work. Needs https://code.videolan.org/videolan/libplacebo/-/merge_requests/796 and more fixes, because the fact the color update is not synchronized with rendered is bit jarring to look at. |
video/out/wayland_common.c
Outdated
| if (wl->color_manager && !wl->color_surface) | ||
| wl->color_surface = wp_color_manager_v1_get_surface(wl->color_manager, wl->callback_surface); |
There was a problem hiding this comment.
So does this set the color_surface before mesa gets a chance to? I'm a little confused on how this doesn't blow up.
There was a problem hiding this comment.
Yes, there is no descriptor by default, and with libplacebo change it won't be created, I need to add some code to go back from pass-through when target-colorspace-hint is disabled, will do later, need to destory our color surface
There was a problem hiding this comment.
We have this hardcoded to only dmabuf-wayland in vo_wayland_init at the moment. It can't just be created there?
There was a problem hiding this comment.
The point is to create it on demand. Same as existing hint works.
There was a problem hiding this comment.
If you want, we can remove target-colorspace-hint option, but people will not like it probably.
There was a problem hiding this comment.
If we're going to start creating color surfaces at all to manage color in this VO, then imo we should just completely bypass mesa and just do it all ourselves (granted, I dunno how much work this is on the libplacebo side). Having a weird mix-match where sometimes it's created, sometimes it's not, maybe gets destroyed, etc. sounds confusing and error prone.
There was a problem hiding this comment.
So does this set the color_surface before mesa gets a chance to? I'm a little confused on how this doesn't blow up.
It's not a race. Only one color surface can be created per wl_surface. Attempting to create more than one hangs the wayland descriptor, as we know. Handling that gracefully is probably doable.
The trick here relies on the the assumption that drivers won't create an idle color surface "just for the heck of it"™ sitting around (for passthrough) in the future. For now, that's the case for openg/vk+passthrough.
There was a problem hiding this comment.
The trick here relies on the the assumption
It's not an assumption or a hope. It's specified that there won't be a color surface created for PASSTHROUGH.
There was a problem hiding this comment.
I could only find an open issue on github on this. But it's more than enough for me. It's great we don't have to make this assumption!
There was a problem hiding this comment.
Read the discussion in this PR KhronosGroup/Vulkan-Docs#2410
video/out/wayland_common.c
Outdated
| if (!primaries || !transfer) { | ||
| wl->target_params.color = pl_color_space_srgb; | ||
| // but gamma2.2... | ||
| wl->target_params.color.transfer = PL_COLOR_TRC_GAMMA22; | ||
| } |
There was a problem hiding this comment.
Can't we save the colorspace from the hint somewhere in the wayland vo state and let set_color_management actually do the logic here?
There was a problem hiding this comment.
I need to know what parameters needs to be used before rendering. If params are supported all the rest is handled by existing code. The hint is already saved.
There are way more corner cases, for example what happens when some of the parameters are not supported, but frankly I don't think it's that important, as with Vulkan we didn't have this info anyway.
kasper93
force-pushed
the
wayland-color
branch
from
e9ec715 to
60c19db
Compare
|
There are a few separate issues: 1) For each image description, there are many other image descriptions that will be treated identically by the compositor. Given two image descriptions,
If for each the equation holds, then the compositor will render surfaces with image descriptions A and B identically. (Assuming all other parameters (primaries etc.) are identical.) 2) Therefore it seems reasonable that the client should also treat any two such image descriptions identically. 3) However, is not uniquely defined in the equivalence classes created by 1 above. An example is
Where contrast = 400 for A and 1000 for B. 4) Therefore, if mpv needs to calculate the contrast quantity, it needs to consistently choose one of the elements from the equivalence class before calculating the contrast. The code currently in master does this by transforming to a color space C with the following parameters:
The issue that was observed with this is that, if primary-min=target-min and primary-ref=target-max, then we get This is different from the previous mpv behavior which in practice always uses contrast=1000 in this case. 5) I don't believe choosing the element from the equivalence class that is sent by the compositor is a good solution for this. These elements are random both in theory (according to 1) and in practice:
Using the calculation from this PR, this leads to contrast=20_000 on KDE and contrast=400 on other compositors. Neither of these seem in any way accurate. I've looked at the top 3 recommended monitors on amazon and they advertise contrasts of 1000, 2000, and 4000. On KDE, this might lead to dark colors appearing too dark, but I have not tested this. On other compositors, mpv has a fallback where it increases contrast to at least 1000, so this would probably be ok, at least it would be the same as mpv without color management support. 6) In another issue it was said that contrast=1000 is good for LCDs and contrast=inf is good for OLEDs. But this information is currently not available via any wayland protocol. 7) Therefore I would instead suggest the following:
Users can always override the behavior on the command line. diff --git a/video/out/wayland_common.c b/video/out/wayland_common.c
index 7acc0b0ef..0367e1c8a 100644
--- a/video/out/wayland_common.c
+++ b/video/out/wayland_common.c
@@ -2164,6 +2164,7 @@ static void info_done(void *data, struct wp_image_description_info_v1 *image_des
// we need to round it.
if (fabsf(wd->csp.hdr.max_luma - PL_COLOR_SDR_WHITE) < 1e-2f) {
wd->csp.hdr.max_luma = PL_COLOR_SDR_WHITE;
+ wd->csp.hdr.min_luma = PL_COLOR_SDR_WHITE / PL_COLOR_SDR_CONTRAST;
if (wd->csp.hdr.max_cll != 0)
wd->csp.hdr.max_cll = MPMIN(wd->csp.hdr.max_cll, wd->csp.hdr.max_luma);
if (wd->csp.hdr.max_fall != 0) |
|
Thank you for the response, let me answer to the points. (EDIT: Also sorry if I'm being hard to discuss with, I genuinely don't mean bad, it's just how I approach things sometimes) 1) I think we already had this math in the other thread. The issue is still the same, it remaps display referred like transfer to another one disregarding the minimum luminance. It as any display referred space will map 0 to 0 relative black. Consider that one space has min-luminance of 100, the 2nd one has min-lumiance of 1. All other parameters are the same, we don't want to map the first 100 to 1 luminance. Because it will be significantly darker. I think all this stems from implementation details from current compositor implementations, and I think we already established this part, but let me recap some things, just so we are on the same page. cd/m² aka. nit is is the unit of luminance. It's an SI unit. Display referred transfers like naturally doesn't have luminance range, they are 0-1, just as the display can reproduce. Now we enter HDR/PQ where the encoding is absolute, in cd/m² and theoretical goal is to reproduce those values. The scale is unambiguous as defined in ST 2084. The trick part is where display referred transfer should land in PQ. That's where reference white value is useful. Now in Wayland all transfers are neither display referred, nor absolute scaled. (joking here :)) They both have luminances and references. And in each case cd/m² means a different thing. If that was an intention of protocol, why even use cd/m²? Why not just speak in relative normalized float? 2) In the end it likely will happen, I'm being stubborn here, because using cd/m² and saying
is bit confusing. Well, maybe the intention was to compositor scale brightness, but still cd/m² means what it means. It's not for client to worry if they set luminances. Either way, the main question which one of those cd/m² is proper scale? Don't think about libplacebo or mpv. Which one of those values is compatible with HDR metadata, which is encoded into movies as PQ and sent to display as metadata? 3, 4) You can think about this way. Let's consider simple example PREFERRED (P): 0.2, 80, 80, 0.2, 80 A: 0.5, 80, 80, 0.5, 80 0.5, correct? Or if you apply conversion, it would map to 0.2. This like I said is not correct, because you are ignoring luminance value of A colorspace. Now, of course, you have to ask what is P? Think about it, as if it is our virtual display. How exactly parameters are mapped to the real display is different story, because compositor may apply ICC profile, any other target adjustments, it's irrelevant from the point of view of client. But we can go there, if needed. P is an abstract space that we target, that's all. In perfect world it could describe display, but it's perfectly valid to any blend space really. It's what compositor requests as they would like to get. Complication comes from the fact that display referred spaces, doesn't have min luminance or they do, but it's display. So Generally you set 0, and that's all. With pipelines with BPC, it's different. But I digress... it's again compositor issue, not clients. Back to our example, if P reproduce 0.2-80 range and A reproduce 0.5-80 range, which is explicitly stated by the descriptor. A should remain at 0.5. Note that the reference is the same, so you can't say that reeeelative. Now, more interesting example, HDR metadata space, B: We established, that Wayland really wants to make reference white blended at the same level. While the values are still cd/m², they suddenly become different scale to A and P, which ok, we can work with. Where my current assumption that Wayland's compatible scale with HDR metadata and PQ signal is: I've split it into steps, so we can better follow, the intermediate normalized value. Which works out to: Now, according to 1) we know that compositors will map relative 0 to relative 0 of another space, so having targeting the above space and assuming black at The math is working here, if however we assume that compositors are broken and give us descriptors that are not usable, than we can ignore those descriptors. Though, it shouldn't affect the above math to calculate things. We can level higher sanitize metadata, and say, that it makes little sense. Also user can override that at will. 5) Compositors implementations doesn't have final word. That's the point I'm trying to make, those descriptors should be meaningful. Not random or irrelevant. It's like that because min/max luminance doesn't mean much for "SDR" transfers, so it kinda works with any values. But this shouldn't prevent clients from using those values for processing, else protocol shouldn't provide them. Now only KDE does send more relevant info, both in max and min luminance. When altering display brightness. I know it's not currently the case. But it could be a descriptor that really describes the output parameters. Initially I though preferred descriptor would be an amalgamation of user setting and hdr calibration options. Either way, that's not the case. I could use EDID info or any other, hardcode some values, but the point here is that we want to avoid any compositor adaptations, we do most of the things in-house. According to 1) if equivalence is not provided compositor will do adjustment. So, that's why changing to 1000:1 contrast as you suggest makes it not equivalent and would trigger compositor to do adaptations. Also, mpv is Wayland client, not compositor client. We shouldn't adapt to compositor implementations, only to make it work. This solidifies this behavior and will not fix anything ever in the future. 7)
Well I doubt that. Especially with HDR displays. For SDR none of this is relevant anyway, we could just disable color management and would result in perfectly valid output. And in HDR it has been for years complaints that users have to manually adjust those parameters. All this color management is for people who needs it. And justifying shortcuts, by the fact "99%" users don't need it, is no good. However if that's the case, we can disable color management by default and make it opt-in as it were before. |
I do not mind it. 1)
At least on the compositors I've tested, 0.0 in each buffer is treated as a It would be possible to do it differently, e.g. the compositor could uplift SDR
Maybe the intention was different, but I don't see a way to treat it as The scaling is in any case required because, in the protocol, SDR transfer 2)
I think ITU-R BT.2408-7 ("Guidance for operational practices in HDR television https://www.itu.int/dms_pub/itu-r/opb/rep/R-REP-BT.2408-7-2023-PDF-E.pdf It says that
CTA-861.3-A - "HDR Static Metadata Extensions" says that
SMPTE ST 2084 in turn says that the minimum luminance is 0. Therefore I would assume that the scale that should be used when using 3, 4)
All compositor that I know of treat 0.0 in the buffer as a fixed point.
Maybe it should, but it doesn't.
As per the recommendation from ITU-R BT.2408-7.
I believe so. Although the protocol says that min=0.005 for PQ. I'm not sure
I don't believe that is correct. It should be Since otherwise ref-a does not map to ref-b.
Maybe. You could use more complex tone mapping, I suppose.
I'm not sure what you mean by this. 5)
I'll have you know that I, too, send a changed reference level when the user
This information is usually not available. The EDID of my secondary monitor The values that KDE sends are not related at all to my monitors.
Is it not?
Adaptations here means tone mapping if the target-min-lum of the surface is less However, mpv already increases the contrast to at least 1000. Therefore,
mpv should not be used as a means to exert pressure. It should be used to 7)
You don't think that SDR displays with primary-ref=target-max are the 99% case?
Maybe disabling color management if primary-ref=target-max is the way to go. |
|
1)
Whether it's relative or not, is workable. I think we currently have a bit of mismatch in particular in black point and how the luminances are handled in compositor, what I think is bit incomplete. As you said:
Let say we have some luminances like so: min: 0.2 Exact values doesn't matter, whether it's relative or not. We can normalize this with regards to reference white min: 0.0025 We have around ~2x HDR headroom, but at the same time this should mean that <0.2 is pure black. Think about this like that we have HDR headroom, but also SDR range. If we ignore minimum, we should be consistent and don't set it as luminances.
Yes, that we established by now. Which in practice means that minimum value can be anything and it doesn't have significant influence on the image.
Yes. I'm confused at this too. 0.005 is common value in HDR movies metadata as close to black, but not sure why it's not just 0.
Ok, actually I fell into trap of your previous code. It should be just We don't want 0.2 to become 0.005, we only want to rescale it. Previously we also discussed that transfers are not "HDR" or "SDR", there is however a difference between traditional gamma transfer or PQ transfer. Of course you can reproduce any luminance range with either, but the significant difference is that one is display referred, while the other is not. Converting between those luminances as if they are all black scaled (i.e. display referred) is not fully correct. Particularly when converting to PQ transfer, we just shouldn't throw away the "nits" information of the traditional gamma transfer input.
This is for "target" parameters, which are fixed, more fancy tonemapping will map source to those values.
If compositor always treat black as display referred. If we set min luminance to higher value, we would map it correctly, either way, this is not that pragmatic, because compositors give dummy values for traditional transfers.
I don't mean to exert pressure, but who else can do reality check for compositor implementation except client application? Let's talk solutions. If we follow your advice to adjust code to compositors implementation rather than the specification. PQ transfer in Wayland world is offset by So, let say I want to put "black" at the display level, say 0.2 nits. We could do A) set_lumianances(0.2, 10000.2, 203) and output 0 PQ code point for black. B) set_lumianances(0, 10000, 203), set_mastering_luminance(0.2, ...) and output 0.2 PQ code point for black. This is also nominal case how any HDR video with metadata should be handled. So, while I have doubts the current compositor impl will map it correctly to different PQ space, it's out of mpv's hands. The main problem here, is that with Wayland we are not targeting the (real) display, but image descriptor that compositor request from us, which is kinda virtual display if you want to think about it this way. So, when compositor asks us to render 80 nits with 0.2 nits black level, we can respect that, scale it up to our reference and go from there. But apparently we have conflict of interest here, because those values are not "valid" for us to use, so we are advice to ignore them. There is also scaling issue, because 0.2 nits is actually perfectly valid black point target, but apparently in Wayland nits are not nits, so it's unknown what to do. We could just use this value as-is without scaling or ignore it completely. This unfortunately, limits ability of compositor to control our output. C) Output gamma2.2, also for HDR. This would require to use 16hf backbuffer. D) Output linear, this is similar to C), just without gamma part. E) Fix compositors to adhere to SMPTE ST 2084 as Wayland specification mandates for PQ transfer. B) seems most reasonable from mpv side, except E) of course. The only issue is that mapping PQ to PQ output will still do an incorrect mapping, but this is on compositor side and we really cannot workaround that here. For black clipping there are multiple patterns, I use Spears & Munsil, but I cannot share it here. This one is good too https://github.com/user-attachments/assets/8d105cd3-c9fe-40b7-b3f6-589079f7a090 the two bars on the right side should be visible.
I don't care about the %. Like I said we are optimizing for 1% (with that metric), the 99% case is already working fine and doesn't need any color management. |
I thought it wasn't but looking at it again it seems that SDR -> SDR playback is indeed unaffected by this issue. |
|
I will wait for #17395 and update this one after, we do some prerequisite changes there. |
kasper93
force-pushed
the
wayland-color
branch
from
60c19db to
2dbe827
Compare
kasper93
changed the title
kasper93
force-pushed
the
wayland-color
branch
from
2dbe827 to
95c2b43
Compare
|
Rebased on top of small color refactoring. Luminances situation is still pending fixes. Using Wayland protocol is a regression to current Vulkan based solution, color surface is not synchronized with the frames, so for example changes to preferred descriptor (ex. by changing brightness) produce visible mismatch from our rendering output and what's used by Wayland. I believe this is not directly related to this PR, but the wayland_common.c. |
kasper93
force-pushed
the
wayland-color
branch
from
95c2b43 to
2227603
Compare
|
We probably need equivalent of 468d34c for vo_gpu_next now |
kasper93
force-pushed
the
wayland-color
branch
2 times, most recently
from
2778678 to
304f0ae
Compare
|
I consider this ready for review. @llyyr, @Dudemanguy: I will need help with @mahkoh: Now we have full control of luminances and other parameters, we can bikesheed this further.
That's the point of display referred transfers, luminances doesn't really mean much for them, unless you convert to PQ (absolute one). I know Wayland treats everything relative, but now I set the descriptor for PQ, same as you would get Blu-ray movie metadata, which I belive Wayland should handle correctly. (if not, it's outside the scope of mpv) |
You will probably need to mimic vo_dmabuf_wayland assuming that waiting for the image description from the compositor also needs to happen in this case. When vo_gpu_next starts a frame, you should set vo_wait_on_vo if the wayland code sets |
|
Besides the one minor nit, this looks good to me. Thanks |
kasper93
force-pushed
the
wayland-color
branch
2 times, most recently
from
732ef8b to
88054d4
Compare
| #if HAVE_WAYLAND_PROTOCOLS_1_41 | ||
| if (!params) { | ||
| if (wl->color_surface) { | ||
| wp_color_management_surface_v1_destroy(wl->color_surface); |
There was a problem hiding this comment.
Why have this destroy behavior? I guess that would mean hacks like VK_hdr_layer won't work but I can't see that mattering?
There was a problem hiding this comment.
The idea is when we set --target-colorspace-hint=no we no longer hint anything. Which in practice means, Vulkan sets VK_COLOR_SPACE_SRGB_NONLINEAR_KHR, which means that Mesa take over color_surface duty. So we release our color surface, to let others use it. Conversely when we do --target-colorspace-hint=yes we switch to VK_COLOR_SPACE_PASS_THROUGH_EXT and use color surface ourselves.
I guess that would mean hacks like VK_hdr_layer won't work but I can't see that mattering?
Mesa supports Wayland nativelly now, so not only VK_hdr_layer is affected.
There was a problem hiding this comment.
Ah I forgot about the --target-colorspace-hint=no case. I guess it is fine as long as that is the only time this ever receives NULL params.
There was a problem hiding this comment.
Yes, in all other cases, hint is defined to "something". Also we might at some point get rid of this option completely, but people still complain that "color is wrong", so restoring to dumb player mode is good to debug or just workaround other bugs.
There was a problem hiding this comment.
Why have this destroy behavior? I guess that would mean hacks like
VK_hdr_layerwon't work but I can't see that mattering?
Well you will remove hdr support for us nvidia users, since amd have so many issues for me at the moment, I will be forced back to windows if this happens.
There was a problem hiding this comment.
Why have this destroy behavior? I guess that would mean hacks like
VK_hdr_layerwon't work but I can't see that mattering?Well you will remove hdr support for us nvidia users, since amd have so many issues for me at the moment, I will be forced back to windows if this happens.
this doesn't remove HDR support for nvidia. this PR makes it so that you dont need that layer for HDR support on nvidia
There was a problem hiding this comment.
Why have this destroy behavior? I guess that would mean hacks like
VK_hdr_layerwon't work but I can't see that mattering?Well you will remove hdr support for us nvidia users, since amd have so many issues for me at the moment, I will be forced back to windows if this happens.
this doesn't remove HDR support for nvidia. this PR makes it so that you dont need that layer for HDR support on nvidia
Ah okay thanks, kind of traumatised by a certain other open source project's disregard for nvidia.
|
when switching from |
|
I'm not sure if this is intended but there is now a discrepancy in the transfer function between
for sdr content, the stat (i) |
I decided for passthrough to prefer 16-bit backbuffers, to allow most flexibility. We could prefer a2bgr10, this should handle fine most things, except linear and things like extender range gamma.
Yeah, opengl doesn't use preferred colorspace, so by default it fallbacks to not adjusting gamma or srgb. Basically opengl works the same as master in regards to color output. This can be extended, but I didn't mix too much things here. |
Fixed this now. But still opengl will use 8-bit backbuffer, so not recommended for anything other than sdr really. |
kasper93
force-pushed
the
wayland-color
branch
3 times, most recently
from
8bd1e0b to
cdb4c6e
Compare
|
Will merge later today, if there won't be more comments. I want to stress test this and we can fix issues as we go. Luminances situation is still unknown, we may just hardcode in the end if needed. |
in my case, fbo-format is Edit: I guess it depends on what the bit depth the compositor uses for the screen. AFAIK opengl is not limited to 8bit. |
This is required, because other modules like Vulkan may need to use it, and only single color surface is allowed in Wayland.
Wayland color managment is not really flexible to be usable with Vulkan color spaces, so set it directly.
This fixes --target-trc=linear output and makes our output luminance ranges more explicit.
Co-authored-by: Kacper Michajłow <kasper93@gmail.com>
Those has Dolby Vision metadata stripped and have all parameters correctly guessed.
Wayland likes to kill the window or application on protocol violation.
| if (wl->color_queue) { | ||
| wl_event_queue_destroy(wl->color_queue); | ||
| wl->color_queue = NULL; | ||
| } |
There was a problem hiding this comment.
| if (wl->color_queue) { | |
| wl_event_queue_destroy(wl->color_queue); | |
| wl->color_queue = NULL; | |
| } |
I think it's fine to keep the queue alive.
video/out/wayland_common.c
Outdated
| if (!wl->color_queue) | ||
| wl->color_queue = wl_display_create_queue_with_name(wl->display, "image description creator queue"); |
There was a problem hiding this comment.
| if (!wl->color_queue) | |
| wl->color_queue = wl_display_create_queue_with_name(wl->display, "image description creator queue"); |
I'd move this to the place where the queue is used for the first time.
| #if HAVE_WAYLAND_PROTOCOLS_1_41 | ||
| if (!params) { | ||
| if (wl->color_surface) { | ||
| wp_color_management_surface_v1_destroy(wl->color_surface); |
There was a problem hiding this comment.
I don't believe this is sound as is.
Consider the following code:
static void image_description_failed(void *data, struct wp_image_description_v1 *image_description,
uint32_t cause, const char *msg)
{
struct vo_wayland_state *wl = data;
MP_VERBOSE(wl, "Image description failed: %d, %s\n", cause, msg);
wp_color_management_surface_v1_unset_image_description(wl->color_surface);
wp_image_description_v1_destroy(image_description);
}
// ...
struct wp_image_description_v1 *image_description = wp_image_description_creator_params_v1_create(image_creator_params);
wl_proxy_set_queue((struct wl_proxy *)image_description, wl->color_queue);
wp_image_description_v1_add_listener(image_description, &image_description_listener, wl);If the color_surface is destroyed while the image description is pending, the callback will access a NULL pointer.
Furthermore, it is probably a good idea to store the image_description inside wl and to destroy it at the appropriate times, since otherwise the image descriptions might complete out of order.
There was a problem hiding this comment.
I don't believe that is necessarily the case. See the discussion in #17495.
| wp_image_description_creator_params_v1_set_luminances(image_creator_params, | ||
| hdr.min_luma * WAYLAND_MIN_LUM_FACTOR, hdr.max_luma, PL_COLOR_SDR_WHITE); |
There was a problem hiding this comment.
I'm confused about this (and similar with the linear case above). Shouldn't max=ref in this case?
There was a problem hiding this comment.
If it's configured correctly, it's will be max=ref. However, it can be configured differently, setting higher max works fine.
| m_opt_choice_str(pl_csp_levels_names, params->repr.levels)); | ||
| wp_color_representation_surface_v1_set_coefficients_and_range(wl->color_representation_surface, coefficients, range); | ||
| } else { | ||
| MP_WARN(wl, "Color representation %s / %s not supported! Output can be incorrect.\n", |
There was a problem hiding this comment.
@mahkoh: Why Jay doesn't advertise any coefficients as supported? I would expect any implementation that supports this protocol at least advertise the baseline, which it in fact supports.
I'm not sure if that's intended, because it triggers this warning. I can exclude identity, because this in practice is always supported, but that seems weird.
There was a problem hiding this comment.
Why Jay doesn't advertise any coefficients as supported?
Jay does not support any YCbCr formats.
I would expect any implementation that supports this protocol at least advertise the baseline, which it in fact supports.
I've implemented the protocol only for the alpha modes.
There was a problem hiding this comment.
That's fair. I think I will keep this warning, it may be weird thing to log when the output is fine. But this is the information we get. RGB/identity is not advertised as supported.
Protocol states.
When this object is created, it shall immediately send this event once for each matrix coefficient and color range combination the compositor supports.
So, I assume that implementation should send RGB/identity as supported, if it is in fact supported.
EDIT: Note to self, in case I forget, change it to Output may be incorrect., sounds better.
There was a problem hiding this comment.
But it doesn't support them. If you call set_coefficients_and_range at all, you will get a protocol error.
There was a problem hiding this comment.
But it doesn't support them.
I kindly disagree that Jay doesn't support identity. It displays fine.
If you call set_coefficients_and_range at all, you will get a protocol error.
That's why I don't call it to avoid protocol error and display warning.
There was a problem hiding this comment.
FWIW, I don't think we need to scare people with a warning here. Verbose is probably fine.
There was a problem hiding this comment.
I think we should warn, it would save us dozens of "wrong colors" issues that we get for dmabuf wayland. Note that it will warn only when color repr proto is actually supported, so it's quite valid thing to say that output will be wrong, if certain params are not supported.
There was a problem hiding this comment.
Honestly my guess is that people would open an issue about the warning they see instead.
There was a problem hiding this comment.
As they should, not for mpv probably, but their compositor.
There was a problem hiding this comment.
FWIW, I don't think we need to scare people with a warning here. Verbose is probably fine.
The warning would only be visible with dmabuf-wayland in most cases, since full range rgb should be supported on all most compositors. In which case, I do think it being a warning is fair.
| wl->color_representation_surface = | ||
| wp_color_representation_manager_v1_get_surface(wl->color_representation_manager, wl->callback_surface); |
There was a problem hiding this comment.
I don't believe this is correct. There could be theoretical compositor that doesn't support the coefficient/range combination we want to set but does support the alpha/chromaloc. In such a case, we'll try to set them without a color repr surface
There was a problem hiding this comment.
Yes, I have it fixed locally, there was unrelated issue that caused problems. I will push later when I have time to work on this again. I got side-tracked again with luminances, because I'm still thinking if we should prefer PQ output and just output in gamma2.2 as compositors like to have it.
6 participants
(yep)