Handle damage in image-copy-capture protocol correctly #1651
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
No we don't, but I guess it would be trivial to lock that to commits of any toplevel's surface? |
The important change here is that we now apply the additional damage first, instead of using `.extend()` to add it after other elements. This is important since `OutputDamageTracker` will ignore our damage elements if there are behind an element with an opaque region. This also makes things a bit simpler, especially `take_screencopy_frames()`, which no longer needs a mutable references to extend then truncate. The implementation of `OutputDamageTracker` isn't entirely clear, but as far as I can tell this is intended to work, and it seems to work in some testing.
The logic `age_for_buffer` used seems to be a misinterpretation of the protocol. The wording is a little unclear, but it seems tracking buffer age is the responsibility of the client, and the client is required to accumulate damage and pass it in `damage_buffer`. Our clients initially weren't doing that correctly. I updated xdg-desktop-portal-cosmic to use `damage_buffer` after testing on wlroots, and cosmic-workspaces was recently updated as well.
ids1024
force-pushed
the
screencopy-damage_noble
branch
from
197404f to
0cdb2b7
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Based on #1638.
It seems
age_for_buffershouldn't really be necessary; the client should accumulate damage, and send it withbuffer_damage. Though trying to change just that revealed thatadditional_damagewasn't being handled properly; it was placed incorrectly in the z-order of the render elements, so it didn't work properly where it was occluded by render elements with opaque regions.pop-os/cosmic-workspaces-epoch#217 updates cosmic-workspaces to accumulate damage, and send it in
damage_buffer.xdg-desktop-portal-cosmicalready does.This appears to work, but may be worth testing a bit more.
I guess we also don't have anything to delay toplevel and workspace capture of a new frame until there is new damage? We probably want that, though hopefully if we track damage properly it won't add too much overhead to capture extra frames that are identical.