wayland: switch to separate queue for wp_image_description_v1 #16844

llyyr · 2025-09-28T02:59:00Z

Based on #16836

This is theoretically more correct

github-actions · 2025-09-28T03:27:05Z

Download the artifacts for this pull request:

Windows

macOS

na-na-hi · 2025-09-28T03:29:06Z

clipboard-wayland: switch to separate queue

This should not be needed. clipboard has a separate wl_display and queue and does not share a wl_display or queue with VO.

llyyr · 2025-09-28T04:23:11Z

This should not be needed. clipboard has a separate wl_display and queue and does not share a wl_display or queue with VO.

You're right. Though I think it's still a good idea to create our own named queue for consistency, but I'll let the maintainers decide.

llyyr · 2025-09-29T01:32:53Z

Dropped everything here except what's strictly needed to fix the issue, though I'd still prefer to name our queues because that's useful when looking at WAYLAND_DEBUG output but I don't want to stall this too long for that

video/out/wayland_common.c

mahkoh · 2025-09-29T15:55:24Z

A round trip does not guarantee that the image description is ready.

llyyr · 2025-09-29T17:32:44Z

A round trip does not guarantee that the image description is ready.

Do I need to wl_display_dispatch_queue and wait for the ready event?

Dudemanguy · 2025-09-29T17:58:17Z

We already make the assumption in several places that a roundtrip will result in several wayland events being processed.

Dudemanguy · 2025-09-29T19:09:19Z

One problem now that I just realized. The wl_display_roundtrip(wl->display) in the uninit doesn't serve its intended purpose since it's on a different queue.

llyyr · 2025-09-29T19:14:30Z

One problem now that I just realized. The wl_display_roundtrip(wl->display) in the uninit doesn't serve its intended purpose since it's on a different queue.

Are you sure? wp_image_description_info_v1.done is still on the default queue

$ WAYLAND_DEBUG=1 mpv test.mp4 --start=50 --pause --vo=dmabuf-wayland 2>&1 | rg 'wp_image_description'
[1861075.072] {Default Queue}  -> wp_color_management_surface_feedback_v1#48.get_preferred_parametric(new id wp_image_description_v1#49)
[1861075.075] {Default Queue}  -> wp_image_description_v1#49.get_information(new id wp_image_description_info_v1#50)
[1861075.076] {Default Queue}  -> wp_image_description_v1#49.destroy()
[1861094.854] {Default Queue} wp_image_description_info_v1#50.primaries_named(1)
[1861094.856] {Default Queue} wp_image_description_info_v1#50.primaries(640000, 330000, 300000, 600000, 150000, 60000, 312700, 329000)
[1861094.858] {Default Queue} wp_image_description_info_v1#50.tf_named(9)
[1861094.859] {Default Queue} wp_image_description_info_v1#50.luminances(2000, 80, 80)
[1861094.861] {Default Queue} wp_image_description_info_v1#50.target_primaries(640000, 330000, 300000, 600000, 150000, 60000, 312700, 329000)
[1861094.862] {Default Queue} wp_image_description_info_v1#50.target_luminance(2000, 80)
[1861094.863] {Default Queue} wp_image_description_info_v1#50.done()
[1861116.850] {Default Queue}  -> wp_color_manager_v1#27.create_parametric_creator(new id wp_image_description_creator_params_v1#56)
[1861116.852] {Default Queue}  -> wp_image_description_creator_params_v1#56.set_primaries_named(6)
[1861116.853] {Default Queue}  -> wp_image_description_creator_params_v1#56.set_tf_named(11)
[1861116.855] {Default Queue}  -> wp_image_description_creator_params_v1#56.set_mastering_display_primaries(708000, 292000, 170000, 797000, 131000, 46000, 312700, 329000)
[1861116.857] {Default Queue}  -> wp_image_description_creator_params_v1#56.set_mastering_luminance(0, 10000)
[1861116.857] {Default Queue}  -> wp_image_description_creator_params_v1#56.set_max_cll(10000)
[1861116.858] {Default Queue}  -> wp_image_description_creator_params_v1#56.set_max_fall(1000)
[1861116.859] {Default Queue}  -> wp_image_description_creator_params_v1#56.create(new id wp_image_description_v1#57)
[1861116.919] wp_image_description_v1#57.ready(5572)
[1861116.921] {Default Queue}  -> wp_color_management_surface_v1#45.set_image_description(wp_image_description_v1#57, 0)
[1861116.924]  -> wp_image_description_v1#57.destroy()
[1861265.065] {Default Queue}  -> wp_color_management_surface_feedback_v1#48.get_preferred_parametric(new id wp_image_description_v1#53)
[1861265.067] {Default Queue}  -> wp_image_description_v1#53.get_information(new id wp_image_description_info_v1#54)
[1861265.068] {Default Queue}  -> wp_image_description_v1#53.destroy()
[1861265.778] {Default Queue} wp_image_description_info_v1#54.primaries_named(1)
[1861265.779] {Default Queue} wp_image_description_info_v1#54.primaries(640000, 330000, 300000, 600000, 150000, 60000, 312700, 329000)
[1861265.781] {Default Queue} wp_image_description_info_v1#54.tf_named(9)
[1861265.782] {Default Queue} wp_image_description_info_v1#54.luminances(2000, 80, 80)
[1861265.783] {Default Queue} wp_image_description_info_v1#54.target_primaries(640000, 330000, 300000, 600000, 150000, 60000, 312700, 329000)
[1861265.785] {Default Queue} wp_image_description_info_v1#54.target_luminance(2000, 80)
[1861265.787] {Default Queue} wp_image_description_info_v1#54.done()

Dudemanguy · 2025-09-29T19:18:11Z

Oh my bad, got wp_image_description_info and wp_image_description mixed up.

mahkoh · 2025-09-29T20:15:43Z

Do I need to wl_display_dispatch_queue and wait for the ready event?

Yes.

We already make the assumption in several places that a roundtrip will result in several wayland events being processed.

Some protocols guarantee that but this one does not.

Dudemanguy · 2025-09-29T20:34:23Z

I'm not sure I understand how wl_display_dispatch_queue and waiting is any different than doing the roundtrip on the queue. The roundtrip internally does wl_display_dispatch_queue and waits for a callback so it should be the same thing no?

mahkoh · 2025-09-29T20:38:24Z

wl_display_dispatch_queue needs to be called in a loop.

Dudemanguy · 2025-09-29T20:44:16Z

That's what wl_display_roundtrip_queue does though.

mahkoh · 2025-09-29T20:47:57Z

The loop in wl_display_roundtrip_queue can end before the image description is ready.

Dudemanguy · 2025-09-29T21:15:56Z

Isn't setting the sync_listener supposed to ensure that everything beforehand is processed since the done is supposed to be the very last reply.

mahkoh · 2025-09-29T21:24:59Z

It guarantees the the compositor has handled all messages sent before then. But the compositor is not required to respond to a wp_image_description_creator_params_v1_create request immediately.

llyyr · 2025-09-29T21:46:45Z

Switched it back to the wl_display_dispatch_queue version since this is required here

kasper93 · 2025-09-29T22:30:40Z

video/out/wayland_common.c

+    wl->image_desc_done = false;
    wp_image_description_v1_add_listener(image_description, &image_description_listener, wl);
+
+    /* Block here to ensure the compositor is ready to receive the


Can't we instead of blocking thread, react in the callback and do rest of the work when compositor is ready for it? Blocking and callbacks generally doesn't go together.

That's exactly what happens before this MR. If we don't block here then we need some other syncing primitive in vo.c's render_frame to wait after draw_frame before calling flip_page.

As I've already said before, this only happens on the very first frame mpv displays for each file.

Hmm too be honest, I don't really like this hard block here. A roundtrip is fine but since evidentially that is not sufficient, putting this while loop here doesn't feel very good. e.g. what if the compositor never gives us this event. Sure it would be bad behavior but that means we would spin here forever.

Don't have any concrete suggestions at this time though. Maybe something like how we wait for frame callback needs to be done here.

what if the compositor never gives us this event

It is guaranteed that the compositor will give us this event eventually by the protocol. Any compositor where this takes long enough for it to get noticeable for the end user is severely buggy and not our responsibility.

Of course compositor can be buggy and violate the protocol and actually never give us this event, then there will be no mpv window when opening a video with dmabuf-wayland and it will be very obvious what the cause is from WAYLAND_DEBUG logs.

Keep in mind that if the compositor never gives us this event on master, what will happen is that dmabuf-wayland will continue to display video but never set any metadata for it and the user would have no clue why.

I'm personally fine with a hard block here, since it only happens at the start of playback one time. But we can return to the roundtrip version if you prefer since in practice most compositors won't wait more than a roundtrip, I verified that a roundtrip will be sufficient for Kwin/wlroots compositors.

If you do a roundtrip and the event has not yet arrived, the process will be aborted when the event arrives after you've destroyed the queue.

We could keep the queue around then

Dudemanguy · 2025-09-29T23:11:06Z

It guarantees the the compositor has handled all messages sent before then. But the compositor is not required to respond to a wp_image_description_creator_params_v1_create request immediately.

Sorry, I know I'm probably coming off as argumentative here, but I'm just trying to understand this. The documentation for wl_display_roundtrip_queue states this:

This function blocks until the server has processed all currently issued requests by sending a request to the display server and waiting for a reply before returning.

To me, this sounds like exactly what we need. We add the image description listener in its own separate queue first, send the request and then wait for it to reply back. It doesn't need to be immediate or anything. The point is to block until the server replies to us. The only request in this queue is the image description one so surely the compositor has to handle our request. Is it possible for the compositor to "reply" in this case but somehow the reply isn't sending us either the failed/ready event (or protocol error)? If so, that seems quite unexpected and unintuitive with how everything else (AFAIK) works.

mahkoh · 2025-09-29T23:16:22Z

Is it possible for the compositor to "reply" in this case but somehow the reply isn't sending us either the failed/ready event (or protocol error)? If so, that seems quite unexpected and unintuitive with how everything else (AFAIK) works.

"processed" only means that the compositor has seen all previously sent requests not that it has taken any positive action beyond that. For example, if you send wl_surface.attach, wl_surface.frame, wl_surface.commit, roundtrip, then the roundtrip will complete immediately but the response to the frame request might only arrive much later when the next vblank happens.

Queues are a purely client-side construct. The compositor is not aware of them.

yejingchen · 2025-09-30T02:53:50Z

It guarantees the the compositor has handled all messages sent before then. But the compositor is not required to respond to a wp_image_description_creator_params_v1_create request immediately.

Sorry, I know I'm probably coming off as argumentative here, but I'm just trying to understand this. The documentation for wl_display_roundtrip_queue states this:

This function blocks until the server has processed all currently issued requests by sending a request to the display server and waiting for a reply before returning.

To me, this sounds like exactly what we need. We add the image description listener in its own separate queue first, send the request and then wait for it to reply back. It doesn't need to be immediate or anything. The point is to block until the server replies to us. The only request in this queue is the image description one so surely the compositor has to handle our request. Is it possible for the compositor to "reply" in this case but somehow the reply isn't sending us either the failed/ready event (or protocol error)? If so, that seems quite unexpected and unintuitive with how everything else (AFAIK) works.

If I am reading the document correctly, wl_display_roundtrip_queue will send another request of it's own and wait for that reply, so we are not only waiting for the image description reply event but also the wl_display_roundtrip_queue one, thus returning from wl_display_roundtrip_queue still do not guarantee the image description go be ready.

llyyr · 2025-09-30T03:55:32Z

If I am reading the document correctly, wl_display_roundtrip_queue will send another request of it's own and wait for that reply, so we are not only waiting for the image description reply event but also the wl_display_roundtrip_queue one, thus returning from wl_display_roundtrip_queue still do not guarantee the image description go be ready.

It's explained in the last comment, "processed" means the compositor has seen all the requests made by client. This specific protocol doesn't guarantee that it'll respond immediately. wl_display_roundtrip only guarantees that the compositor sees the request.

f0883cd fixed the issue that `set_color_management` would be called on every frame, however it didn't fix the problem that the first frame would have no image description set. Due to a lack of blocking here, events would end up happening in the following order: -> wl_surface.commit() -> wp_color_management_surface_v1.set_image_description(...) -> wl_surface.commit() -> wp_color_management_surface_v1.set_image_description(...) This would mean setting image description would always lag behind by 1 surface commit. This would effectively result in the first frame never having any image description set. Fix this by blocking until the compositor processes what's in the queue and responds with the ready event so we can set the image description.

llyyr force-pushed the wayland-use-separate-queues branch from 11e3f80 to 6b18e55 Compare September 28, 2025 04:23

llyyr mentioned this pull request Sep 28, 2025

wayland: call pl_color_space_infer before comparing target_params #16836

Merged

llyyr force-pushed the wayland-use-separate-queues branch 2 times, most recently from e0a67ae to 81be00c Compare September 29, 2025 01:32

llyyr changed the title ~~Wayland use separate queues~~ wayland: switch to separate queue for wp_image_description_v1 Sep 29, 2025

Dudemanguy reviewed Sep 29, 2025

View reviewed changes

video/out/wayland_common.c Outdated Show resolved Hide resolved

llyyr force-pushed the wayland-use-separate-queues branch from 81be00c to 2e61ffe Compare September 29, 2025 14:55

Dudemanguy reviewed Sep 29, 2025

View reviewed changes

video/out/wayland_common.c Outdated Show resolved Hide resolved

Dudemanguy added this to the Release v0.41.0 milestone Sep 29, 2025

llyyr force-pushed the wayland-use-separate-queues branch from 2e61ffe to 0f6c06b Compare September 29, 2025 18:48

llyyr force-pushed the wayland-use-separate-queues branch from 0f6c06b to 44b0b3b Compare September 29, 2025 21:46

kasper93 reviewed Sep 29, 2025

View reviewed changes

Dudemanguy removed this from the Release v0.41.0 milestone Sep 30, 2025

llyyr force-pushed the wayland-use-separate-queues branch from 44b0b3b to 9a91a28 Compare September 30, 2025 23:49

llyyr mentioned this pull request Oct 29, 2025

wayland: move image description info to separate queue #16982

Open

llyyr force-pushed the wayland-use-separate-queues branch from 9a91a28 to 554684d Compare October 29, 2025 17:14

wayland: switch to separate queue for wp_image_description_v1 #16844

Are you sure you want to change the base?

wayland: switch to separate queue for wp_image_description_v1 #16844

Conversation

llyyr commented Sep 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Sep 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

na-na-hi commented Sep 28, 2025

Uh oh!

llyyr commented Sep 28, 2025

Uh oh!

llyyr commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mahkoh commented Sep 29, 2025

Uh oh!

llyyr commented Sep 29, 2025

Uh oh!

Dudemanguy commented Sep 29, 2025

Uh oh!

Dudemanguy commented Sep 29, 2025

Uh oh!

llyyr commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Dudemanguy commented Sep 29, 2025

Uh oh!

mahkoh commented Sep 29, 2025

Uh oh!

Dudemanguy commented Sep 29, 2025

Uh oh!

mahkoh commented Sep 29, 2025

Uh oh!

Dudemanguy commented Sep 29, 2025

Uh oh!

mahkoh commented Sep 29, 2025

Uh oh!

Dudemanguy commented Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mahkoh commented Sep 29, 2025

Uh oh!

llyyr commented Sep 29, 2025

Uh oh!

kasper93 Sep 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

llyyr Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

Dudemanguy Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

llyyr Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

mahkoh Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

llyyr Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

Dudemanguy commented Sep 29, 2025

Uh oh!

mahkoh commented Sep 29, 2025

Uh oh!

yejingchen commented Sep 30, 2025

Uh oh!

llyyr commented Sep 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

llyyr commented Sep 28, 2025 •

edited

Loading

github-actions bot commented Sep 28, 2025 •

edited

Loading

llyyr commented Sep 29, 2025 •

edited

Loading

llyyr commented Sep 29, 2025 •

edited

Loading

Dudemanguy commented Sep 29, 2025 •

edited

Loading

kasper93 Sep 29, 2025 •

edited

Loading