wayland: call pl_color_space_infer before comparing target_params #16836
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Download the artifacts for this pull request: Windows |
Calling `pl_color_space_infer` on `wl->target_param` can cause the `pl_color_space_equal` check to fail on every frame if any values were changed by `pl_color_space_infer`. This will cause us to believe target_params get changed on every frame, and we'll end up in a state where wayland events happen in the following order indefinitely: -> wp_color_management_surface_v1#45.set_image_description(...) -> wp_color_management_surface_v1#45.unset_image_description() -> wl_surface#9.commit() -> wp_color_management_surface_v1#45.set_image_description(...) -> wp_color_management_surface_v1#45.unset_image_description() -> wl_surface#9.commit() Since image_description is double buffered, we always queue an unset to the pending state before each commit. As a result, no commit ever carries a valid image_description. Taking the sample file in the issue as an example, we end up in this state because the file has `min_luma == 0` but `pl_color_space_infer` normalizes this value to `min_luma == 0.000001`. This makes it so that we store a different `target_param` to `vo_wayland_state` on every frame than the one received from `vo_get_target_params`. So we end up setting image description on every frame in this case. The key problem here is that `set_color_management` isn't blocking the thread until `set_image_description is called`, so vo->driver->flip_page is called before this finishes. This commit fixes the problem by doing any operations that could change `wl->target_param` first before doing any equality checks to skip changing to image description pointlessly. This fixes the problem in the issue that image_description is never set on such files. The synchronization problem is fixed in a later commit, because otherwise setting image_description on every frame could have adverse frame time effects Fixes: mpv-player#16825
set_image_description replaces the old image description, so we don't need to call unset_image_description unconditionally. Only call it if we couldn't set an image description. This helps with the lack of synchronization between VO pageflips and wayland protocol events mentioned in the previous commit.
llyyr
force-pushed
the
wayland-color-hint2
branch
from
cc00f96 to
780f30e
Compare
kasper93
reviewed
Sep 27, 2025
video/out/wayland_common.c
Outdated
|
|
||
| /* Do a round trip to ensure the image description gets set before | ||
| * vo->driver->flip_page calls wl_surface_commit on us */ | ||
| wl_display_roundtrip(wl->display); |
Member
There was a problem hiding this comment.
Is it good thing to do? Does this synchronization can have potentially advert effects?
Contributor
Author
There was a problem hiding this comment.
If it was every frame I would say no, but since this should only happen on the first frame or when target_params change, it should be fine.
@Dudemanguy thoughts?
Contributor
Author
There was a problem hiding this comment.
Gstreamer and GDK do similar blocks, though they create a different queue just for this to block on. I could do that here if preferred
llyyr
force-pushed
the
wayland-color-hint2
branch
from
780f30e to
ba348e2
Compare
Contributor
Author
|
Dropped the third commit based on irc conversation, discuss it at #16844 instead |
kasper93
approved these changes
Sep 28, 2025
Member
kasper93
left a comment
kasper93
left a comment
There was a problem hiding this comment.
Should be fine. We can improve remaining issues later.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See individual commit descriptions.
First commit fixes us calling set_color_management on every frame.
Second commit removes unneeded unset_image_description calls.
Third commit adds a roundtrip to block the thread until the compositor responds with a ready to get image_description. This is fine because we aren't calling the function on every frame now as a result of the first commit, which is why I ordered them like this.