mpv-player
wayland: switch to separate queue for wp_image_description_v1
Download the artifacts for this pull request:
Windows
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
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.
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.
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
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?
We already make the assumption in several places that a roundtrip will result in several wayland events being processed.
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.
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()
Oh my bad, got wp_image_description_info and wp_image_description mixed up.
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.
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?
The loop in wl_display_roundtrip_queue can end before the image description is ready.
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.
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.
Switched it back to the wl_display_dispatch_queue version since this is required here
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.
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.
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_queuestates 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.
If I am reading the document correctly,
wl_display_roundtrip_queuewill 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 thewl_display_roundtrip_queueone, thus returning fromwl_display_roundtrip_queuestill 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.
Instead of using a separate queue, you should add the ability for the vo to tell the frontend that it is not ready yet so that the frontend can keep processing events as usual but only continues playing the video once the vo becomes ready again.