Skip to content

wayland: do a roundtrip to wait for image description creation#17495

Merged
kasper93 merged 2 commits intompv-player:masterfrom
llyyr:fix/wayland-cm-queue-take3
Mar 3, 2026
Merged

wayland: do a roundtrip to wait for image description creation#17495
kasper93 merged 2 commits intompv-player:masterfrom
llyyr:fix/wayland-cm-queue-take3

Conversation

@llyyr
Copy link
Contributor

@llyyr llyyr commented Mar 3, 2026

No description provided.

llyyr added 2 commits March 3, 2026 06:40
Also switch to a different event queue to ensure there are no other
events in the queue.
Not needed after previous commit
Copy link
Member

@kasper93 kasper93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works, thanks!

@kasper93 kasper93 requested a review from Dudemanguy March 3, 2026 01:42
Copy link
Member

@Dudemanguy Dudemanguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine.

@kasper93 kasper93 merged commit 35443a7 into mpv-player:master Mar 3, 2026
29 checks passed
@mahkoh
Copy link
Contributor

mahkoh commented Mar 3, 2026

I don't believe this is correct. wl_display_roundtrip_queue performs one roundtrip on the queue. But if the image_description callbacks are not called within that roundtrip, the queue will never be dispatched again and the callbacks will never be invoked.

@llyyr
Copy link
Contributor Author

llyyr commented Mar 3, 2026

I'm aware, but this was sufficient on every compositor I tested on. In fact the VK_hdr_layer layer also does a roundtrip like this.

Of course I'm perfectly fine with doing a dispatch until the callback is invoked, but there was some aversion among mpv maintainers with that approach. @Dudemanguy thoughts?

@mahkoh
Copy link
Contributor

mahkoh commented Mar 3, 2026

Am I trippin or didn't you add the "VO is ready" state to handle the image description not being ready while allowing the rest of mpv to run normally?

@llyyr
Copy link
Contributor Author

llyyr commented Mar 3, 2026

didn't you add the "VO is ready" state

There were several problems with it. If the callback didn't get invoked within the next two draw_frame calls, mpv would end up in a permanent deadlock. Fixing it would be far more complex than it is worth. I didn't like the idea of littering the rest of the codebase just because Wayland being special. It's better for Waylandisms to be contained in one place instead of in mpv core.

@mahkoh
Copy link
Contributor

mahkoh commented Mar 3, 2026

Mesa loops until the image description is ready. If that was good enough for waylandvk until now, then it should also be good enough for the rest of mpv.

@llyyr
Copy link
Contributor Author

llyyr commented Mar 3, 2026

Mesa loops until the image description is ready. If that was good enough for waylandvk until now, then it should also be good enough for the rest of mpv.

I completely agree, but I'm not mpv maintainer and I've already made that PR before and it was rejected. Something like llyyr@d330627 would be my preference

@kasper93
Copy link
Member

kasper93 commented Mar 3, 2026

Mesa loops until the image description is ready. If that was good enough for waylandvk until now, then it should also be good enough for the rest of mpv.

I completely agree, but I'm not mpv maintainer and I've already made that PR before and it was rejected. Something like llyyr@d330627 would be my preference

I don't want to add an api that spans three layers of abstraction, only to wait on some internal Wayland queue.

Why not just put while (wl->image_description_pending) if (wl_display_dispatch_queue(wl->display, wl->color_queue) < 0) break; instead of round trip, which is apparently not enough?

Alternatively, we can wait in other entry points, like flip_page or draw_frame.

@llyyr
Copy link
Contributor Author

llyyr commented Mar 3, 2026

Why not just put while (wl->image_description_pending) if (wl_display_dispatch_queue(wl->display, wl->color_queue) < 0) break; instead of round trip, which is apparently not enough?

This is fine too, in fact that was the first version of the PR I made. I can change it to that if that's fine, it's exactly what Mesa does

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants