Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Retry on failed frame captures, and update ext_image_copy_capture_session_v1 buffer parameters on every ::done #102

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mstoeckl
Copy link

@mstoeckl mstoeckl commented Jan 1, 2025

I recently tried to use wl-screenrec --experiemental-ext-image-copy-capture with wlroots' draft implementation of ext-image-copy-capture, but that produced an error described at #101, when I resized the output while a full-output recording was running. (By running Sway inside itself, and resizing the nested Sway window.) This is admittedly a weird thing to do, but it is an easy way to test that programs can reliably handle runtime output changes (like changing output scale or mode), that sometimes reveals race conditions.

After doing a quick fix for #101 (see last commit of https://github.com/mstoeckl/wl-screenrec/tree/fix-duplicate-frame-hack, which is probably not the best way to solve the problem), I made other patches to address minor issues that I noticed, which I am submitting in this PR.

  • Retry instead of quitting when copying a frame fails for an unknown reason, because when resizing the output compositors sometimes make the frame collection fail (e.g., because it was resized again after the ::frame request was made)
  • Update the buffer constraints (size and chosen format) every time ext_image_copy_capture_session_v1::done is received, instead of just the first time, so later frame requests use the new parameters.

Copy failures can occur when the compositor resizes a window or
output, but these are usually transient.
@russelltg
Copy link
Owner

To make sure I follow, this is a step towards a better ext-image-copy-capture implementation, and doesn't actually fix #101?

I'm not sure I'm interested in this partial implementation, as this doesn't actually resize the surfaces we're passing to the compositor.

I'll take a look at fixing those issues, altho it will probably be a somewhat large change, but a good one as it will pave the way to maybe capturing windows in the future.

@mstoeckl
Copy link
Author

mstoeckl commented Jan 5, 2025

To make sure I follow, this is a step towards a better ext-image-copy-capture implementation, and doesn't actually fix #101?

Yes: I don't see a way to fix #101 myself cleanly without doing some refactoring, and I expect you've got a far better sense for the current and future code requirements here than I do.

I'm not sure I'm interested in this partial implementation, as this doesn't actually resize the surfaces we're passing to the compositor.

Understandable; sometimes when doing a full implementation you need to rewrite enough that merging partial improvements like this would be wasted effort. Feel free to close this PR if you want. (And/or to take random bits from it without attribution; to make things simpler, I provide these changes under CC-0.)

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.

2 participants