wayland: remove check for ICC_V2_V4 feature #17036
Conversation
|
This does a lot more than what the subject of the commit message says. If we're actually using the icc part of the protocol wrong, then all of it should be removed completely. But it's true that ICC_V2_V4 doesn't need to be checked since we don't actually do ICC Creator requests. |
llyyr
force-pushed
the
fix/wayland-color-icc
branch
from
c22dfc1 to
913bb26
Compare
|
Changed the commit message and simplified the icc_file handling to what makes sense currently. In general, the only reason we'd support the icc_file event is if the compositor doesn't support parametric image descriptions. For such compositors, mpv currently won't do the right thing if We also don't need to log about compositors lack of parametric support since we decided to support those compositors with icc profiles instead. Similarly, we don't need to log about the compositor not giving us an icc_file because it doesn't have to with get_preferred_parametric. |
llyyr
force-pushed
the
fix/wayland-color-icc
branch
from
913bb26 to
e7b8b77
Compare
|
Download the artifacts for this pull request: Windows |
video/out/wayland_common.c
Outdated
| } | ||
| talloc_free(gl_opts); | ||
|
|
||
| get_compositor_preferred_description(wl, wl->supports_parametric); |
There was a problem hiding this comment.
If a compositor supports parametric image descriptions, we will only ever ask for a parametric image description and never get an icc file due to the way this currently works (old code would ask twice although it mistakenly guarded the icc check).
In general, the only reason we'd support the icc_file event is if the compositor doesn't support parametric image descriptions.
Are we absolutely certain this is true? For example, if a compositor sets ICC profiles is it guaranteed to communicate that in a parametric form that is exactly equivalent to the icc profile itself? Also doesn't the whole parametric business only work on vulkan?
There was a problem hiding this comment.
Are we absolutely certain this is true? For example, if a compositor sets ICC profiles is it guaranteed to communicate that in a parametric form that is exactly equivalent to the icc profile itself?
Yes
Also doesn't the whole parametric business only work on vulkan?
I don't know what you're referring to. This is about getting the preferred image description from the compositor, not mpv setting its image description. The former is API agnostic, in fact there's no way to get that info from Vulkan or OpenGL.
Vulkan will use the parametric creator for you to set image description for mpv if you use VK_EXT_hdr_metadata.
If we want to make it work on opengl we should stop creating the color manager only for dmabuf-wayland. But this is unrelated to this PR
There was a problem hiding this comment.
Actually after reading the protocol more carefully, my understanding is that get_preferred_parametric is guaranteed to give us the image description as parametric. While get_preferred may or may not parametrize the ICC profile.
So for compositors that support the feature parametric, we should prefer using get_preferred_parametric And for compositors that don't, we can use get_preferred as long as we're ready to accept both icc profiles or parametric description.
There was a problem hiding this comment.
The feature flags only indicate that is is legal to send certain requests. It is still possible for either request to fail at runtime if the compositor cannot represent the image description as an ICC file or parametric. E.g. consider a system with two monitors where one is configured with an ICC file and one is configured without.
Edit: Actually since get_preferred can deliver either an ICC file or a parametric description, get_preferred will almost surely always succeed if get_preferred_parametric succeeds but you get my point in the other direction.
There was a problem hiding this comment.
This is about getting the preferred image description from the compositor, not mpv setting its image description.
The whole point of getting that is to pass it to vo_gpu_next so it can use it. Getting the preferred image description is API agnostic, but I presume it only has any behavior difference for mpv if we're using vulkan since AFAIK hdr and so on doesn't work on opengl.
If we want to make it work on opengl we should stop creating the color manager only for dmabuf-wayland.
We only limit the color surface to dmabuf-wayland because mesa binds it in vulkan (opengl and wlshm of course don't work so no point in binding it there). Getting the image description info should work everywhere on any wayland VO.
Anyways based on the discussion so far, I don't see a reason to not just only call wp_color_management_surface_feedback_v1_get_preferred. We should work with whatever the compositor gives us no? There are scenarios where only an icc file is valid and scenarios where the parametric description is better. It's also worth noting that the icc file will work on opengl but I presume setting the target_csp in vo_gpu_next does nothing if the underlying backend is opengl (correct me if I'm wrong here). I know the --icc-profile-auto and --target-colorspace-* switches aren't exactly the same (e.g. you have to opt into the former), but it's what the user interface is.
There was a problem hiding this comment.
The whole point of getting that is to pass it to vo_gpu_next so it can use it. Getting the preferred image description is API agnostic, but I presume it only has any behavior difference for mpv if we're using vulkan since AFAIK hdr and so on doesn't work on opengl.
Why doesn't it work on opengl? It'll obviously work and I've verified it works if we create a color surface and let mpv use the parametric creator over opengl.
We only limit the color surface to dmabuf-wayland because mesa binds it in vulkan (opengl and wlshm of course don't work so no point in binding it there). Getting the image description info should work everywhere on any wayland VO.
opengl and wlshm not working is by choice, not because it can't. For those VOs, we have to set the image description ourselves similar to dmabuf-wayland. gpu-next can use VK_EXT_hdr_metadata and does use it, but I'm looking to move it to set the image description via the wayland protocol directly as well.
I don't see a reason to not just only call wp_color_management_surface_feedback_v1_get_preferred. We should work with whatever the compositor gives us no? There are scenarios where only an icc file is valid and scenarios where the parametric description is better
the ICC file can't perfectly represent all the information a parametric description can, while a parametric description can describe an ICC file. This is based on IRC discussion. Kwin for example, if the output has an ICC file and client requests parametric description, will return the primaries from the ICC profile and gamma22 transfer with black level matching the ICC profile.
If the compositor supports parametric, we should always use parametric. ICC can be fallback for compositors where this is not possible.
It's also worth noting that the icc file will work on opengl but I presume setting the target_csp in vo_gpu_next does nothing if the underlying backend is opengl (correct me if I'm wrong here).
This is again by our choice. If we create color surface for everything besides when the gpu-api in use is vulkan, it'll work on opengl as well. It'll work for Vulkan that way too if we make libplacebo pick PASSTHROUGH colorspace, but this requires new libplacebo API and some changes to ensure things work. I'll look into this eventually, it's a bit more involved than that and supporting anything other than --target-colorspace-hint-mode=source seems difficult without a lot of libplacebo changes
There was a problem hiding this comment.
Why doesn't it work on opengl? It'll obviously work and I've verified it works if we create a color surface and let mpv use the parametric creator over opengl.
The compositor will actually report HDR colorspaces and metadata when using opengl? Really?
opengl and wlshm not working is by choice, not because it can't.
wlshm is literally hardcoded to one format (well big endian/little endian version of the same format). It's not just a matter of copying what dmabuf-wayland does. It would take much more work. I didn't think the underlying drivers choose the correct colorspace for opengl among other things. Everyone's focused on vulkan in terms of HDR for wayland; I presume for a decent reason. Does this all actually work correctly, choose the right formats and everything?
Kwin for example, if the output has an ICC file and client requests parametric description, will return the primaries from the ICC profile and gamma22 transfer with black level matching the ICC profile.
That's Kwin, but I don't think we can just expect every compositor to work like that. I'm sure there are some that would not bother to transform the ICC profile values to parametric. Also in this case, I don't see why we should prefer the parametric value. If the user sets an icc profile for the output, they probably would expect this to work through icc options not the target colorspace stuff.
There was a problem hiding this comment.
The compositor will actually report HDR colorspaces and metadata when using opengl? Really?
The compositor reports the supported colorspaces through the wayland protocol, it doesn't care that we use opengl or vulkan.
I didn't think the underlying drivers choose the correct colorspace for opengl among other things. Everyone's focused on vulkan in terms of HDR for wayland; I presume for a decent reason. Does this all actually work correctly, choose the right formats and everything?
Of course not, i'm saying that mpv can encode PQ and output to a linear framebuffer in opengl as well. Then mpv just needs to use the parametric creator to let the compositor know what it's outputting.
I don't think we can just expect every compositor to work like that
If they claim to support parametric they have to work like that.
If the user sets an icc profile for the output, they probably would expect this to work through icc options not the target colorspace stuff.
I don't see why the user cares how things are working, this is literally just implementation details.
I want to avoid ICC profiles here because it's not well defined what happens when you do HDR with ICC profiles
There was a problem hiding this comment.
Also the current iteration of this PR doesn't disable using icc files either, it'll just only use icc files if the compositor can't give us a parametric description. Which is fair IMO.
Of course there's the added source of confusion that user needs to specify icc-profile-auto for mpv to use icc profiles when it really should be automatic for wayland at least, but I don't wanna stack too many controversial changes in one PR
The only part of the protocol that is behind this feature is the create_icc_creator which we don't use. We also seem to confuse the presence of this feature to mean that the compositor will always give us non-parametric image descriptions but these two things have no relation with each other. Remove the feature check and use `wl->supports_parametric` to decide if we want to use `get_preferred` or `get_preferred_parametric` instead. This also removes error messages for when the compositor doesn't support parametric descriptions or when the compositor doesn't support icc_v2_v4. The user needs to set `icc-profile-auto` for the icc_file path to work properly which isn't ideal but I'm not sure what to do about this and likely won't make any big changes right before a release.
llyyr
force-pushed
the
fix/wayland-color-icc
branch
from
df40da2 to
5c3e714
Compare
Why is that? ICC profile can be way more specific compared to parametric way of description. Why we prefer to limit available information. |
Well I'm not an expert on ICC profiles, but based on discussion if reference luminance isn't same as the max luminance, the icc profile can't describe that for instance. We rely on this information for 532bac0 which wouldn't be possible if we got an ICC profile instead. |
If something can't be represented in an ICC profile, the compositor shouldn't use it to describe things. |
If the compositor doesn't support the parametric feature, it is advertising to us the client that it is not capable of doing that. It's up to us to try the parametric feedback first before falling back to ICC. |
llyyr
force-pushed
the
fix/wayland-color-icc
branch
from
5c3e714 to
10a57e2
Compare
If we used get_preferred_parametric to get this image description, then it is guaranteed to be parametric. If we used get_preferred to get this image description, then it may or may not be parametric depending on the compositor. If we receive an icc_file event, then the image description is not parametric.
Compositor will probably give us a parametric description anyway, but mpv can handle icc files if it gives us one so allow compositors to give us an icc file.
llyyr
force-pushed
the
fix/wayland-color-icc
branch
from
10a57e2 to
0ee4701
Compare
|
I'm not seeing anything obviously wrong with this. |
|
It's fine for me. ICC profiles are little bit iffy in general with all our color management. |
|
Thanks guys. This can go in. |
4 participants
No description provided.