element/damage: Better support effects on framebuffer contents

[Drakulix]

This is an attempt add changing Element/RenderElement alongside the damage algorithms to better support effects on the underlying framebuffer, more specifically blurring the background. But similar effects are imaginable and this tries to be generic towards whatever future versions of ext-background-effects-v1 might allow.

An implementation of a BlurElement, that makes use of this code, can be found here: https://github.com/pop-os/cosmic-comp/blob/frosted-glass_noble/src/backend/render/blur.rs#L206

To facilitate this, we need two things:

• We need to inform the element, when it's underlying framebuffer is dirty, so that it can re-capture the framebuffer and doesn't have to do that on every draw-call as these effects can be quite costly.
  • Most importantly we don't want to update the effect, if the surface on top of it updates or the a cursor passes over. (Assuming the element caches it's results, which makes sense for blurring but might not for others, which likely can be trivially implemented today.)
• We need to make sure to damage the whole element, whenever we want to allow the element to capture the framebuffer, so that everything below it is re-draw on that same cycle.

Facilitating the latter is easy. Element gets a new is_framebuffer_effect method. We use that for both "damaging the whole element" and figuring out, if we need the more expensive logic in the first place.

For the former, I introduced a damage_index to our OutputDamageTracker per element. Since we iterate through the elements top-to-bottom, we can record the index of all damage collected so far when we hit a new element to have a clean cut-off of damage above and below our element. We can then use that later to figure out, if any damage below the object occurred, which prompts capture to be called.

[YaLTeR]

Haven't looked at the code, but I've got a question from the description: does this help with the case where blur needs to track damage slightly further than directly below the blur element? Since blur samples pixels from slightly outside. Or is this solved by having the blur element be slightly bigger itself?

[Drakulix]

Haven't looked at the code, but I've got a question from the description: does this help with the case where blur needs to track damage slightly further than directly below the blur element? Since blur samples pixels from slightly outside. Or is this solved by having the blur element be slightly bigger itself?

Yes, you can just increase the size of the BlurElement to match the window-geometry + radius and this code will ensure that the area around the window is also repainted to give accurate results when blitting from the framebuffer. You then just need to make sure to only render the inner window-geometry from your blurred texture.

My 2 cents on the matter: The implementation linked above I doesn't do this. I noticed even MacOS, which has different blur effects all over the place, doesn't implement this correctly. E.g. moving a window right next (pixel perfect) to a colored line doesn't make the color show up in the blur until the window overlaps with said line at least partially.

This lead me to lazily implement kawase-blur with CLAMP_TO_EDGE set for the captured texture (to correctly sample "something" outside of the texture geometry). And that emitted exactly the same bug while looking just fine.

So while this can be solved with this implementation, I don't think it really is necessary. (And you need to use something like CLAMP_TO_EDGE anyway for the blur to not look weird, when you windows come up against the outputs edge.)

[YaLTeR]

Thanks.

I noticed even MacOS, which has different blur effects all over the place, doesn't implement this correctly. E.g. moving a window right next (pixel perfect) to a colored line doesn't make the color show up in the blur until the window overlaps with said line at least partially.

That's an interesting detail. Definitely makes it easier yeah.

[YaLTeR]

So since you only get a frame here and no renderer, that means you cannot create GlesTextures with their automatic cleanup, right? And have to manage the destruction manually somehow (relevant for pooling/reusing textures).

[Drakulix]

Yes, that is one of the things I am not so happy with here.

As you can see the cosmic-comp blur implementation currently uses a lot of raw GL code to side-step that issue, but yeah you don't get access to the destruction-channel this way.

Given this feature most likely needs a Blit anyway, I thought we have BlitFrame at least, but that also needs a Framebuffer and now you have to deal with storing a framebuffer and the texture in the element, making it self-referencial...

But think this is a bigger issue with the whole abstraction not necessarily something that needs to be fixed in this PR.

As for possible solutions I am not sure. We could implement more renderer-traits for the GlesFrame or possibly allow sub-Frames and then treat the whole thing more like a guard and abandon the strict separatation between Renderer and Frame? (related: #1750)

[YaLTeR]

One change that tripped me up a little is that since is_framebuffer_effect() has a default impl, nothing tells you that you need to update your custom wrapper elements to pass through this new function. Something for the release notes I guess

[YaLTeR]

In the cosmic-comp BlurElement impl, how does capture_framebuffer() handle the output transform? In winit when the transform is 180, the glBlitFramebuffer call blits from wrong coordinates for me.

[YaLTeR]

Perhaps current transform needs to be passed into capture_framebuffer() too?

[YaLTeR]

Found a potential issue with framebuffer effect, though not sure if it might be a mistake in my code. When it's covered by some other opaque window, and I move the covering window outside, it doesn't get redrawn:

fb-effect-damage-issue.mp4

[Drakulix]

Found a potential issue with framebuffer effect, though not sure if it might be a mistake in my code. When it's covered by some other opaque window, and I move the covering window outside, it doesn't get redrawn.

Pushed two commits that should fix the issue you are seeing here, please test.
I am not too happy with the code however, suggestions welcome.

[Drakulix]

In the cosmic-comp BlurElement impl, how does capture_framebuffer() handle the output transform? In winit when the transform is 180, the glBlitFramebuffer call blits from wrong coordinates for me.

Currently it doesn't. We should probably pass the output-transform. You could also receive the projection matrix of the GlesFrame, but that is likely not as helpful, given you'd want to use blit.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 27.90698% with 93 lines in your changes missing coverage. Please review.
✅ Project coverage is 18.39%. Comparing base (cb9ea26) to head (7f80cd8).
⚠️ Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
src/backend/renderer/element/mod.rs	25.45%	41 Missing ⚠️
src/backend/renderer/element/utils/elements.rs	0.00%	36 Missing ⚠️
src/backend/renderer/damage/mod.rs	56.75%	16 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1910      +/-   ##
==========================================
- Coverage   18.77%   18.39%   -0.39%     
==========================================
  Files         180      182       +2     
  Lines       28743    28974     +231     
==========================================
- Hits         5397     5329      -68     
- Misses      23346    23645     +299     

Flag	Coverage Δ
wlcs-buffer	15.98% <27.13%> (-0.37%)	⬇️
wlcs-core	15.53% <27.90%> (-0.47%)	⬇️
wlcs-output	6.80% <0.77%> (-0.06%)	⬇️
wlcs-pointer-input	17.29% <27.90%> (-0.48%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[YaLTeR]

Example way to repro on current latest commit:

fb-effect-damage-issue-2.mp4

[YaLTeR]

Could it be that the following causes the issue?

• I move opaque window on top towards outside
• Only the newly exposed part of the framebuffer effect element gets damaged
• It calls capture_framebuffer() with the new logic
• capture_framebuffer() operates on the entire area, not just the damaged region
• part of the area that is still covered by the opaque surface was never damaged or redrawn
• so the framebuffer contents of that area contain the opaque surface, even though we're drawing the framebuffer effect element "below" it
• they end up in the captured framebuffer

[Drakulix]

Could it be that the following causes the issue?

* I move opaque window on top towards outside

* Only the newly exposed part of the framebuffer effect element gets damaged

This overlaps with the framebuffer-effect element, so the whole intersection of the framebuffer-effect element and the output geometry gets added to the damage.

* It calls capture_framebuffer() with the new logic

* capture_framebuffer() operates on the entire area, not just the damaged region

* part of the area that is still covered by the opaque surface was never damaged or redrawn

All below it should be re-drawn in the same cycle. The new logic also ignores opaque regions on top of the framebuffer-effect element now, if any part of it (and thus the whole thing) got damaged.

* so the framebuffer contents of that area contain the opaque surface, even though we're drawing the framebuffer effect element "below" it

* they end up in the captured framebuffer

Yes, that is what happened before.

[YaLTeR]

I just thought of another fun edge case where a framebuffer effect may end up with wrong contents: drawing to a different damage tracker (e.g. offscreens or screencasts). The intended use is for capture_framebuffer() to write into some texture stored in the Element. So imagine drawing to the main damage tracker, then drawing to a different damage tracker once (e.g. one window screencast frame). This other damage tracker will call capture_framebuffer() which will update the texture (stored on the Element). Then, we draw back to the main damage tracker, with something changed on top of the element. There were no changes below, as far as the main damage tracker is concerned, so capture_framebuffer() is not called. But draw() is called, and it ends up drawing texture contents from the window screencast frame, rather than the intended ones from the main scene.

[Drakulix]

I just thought of another fun edge case where a framebuffer effect may end up with wrong contents: drawing to a different damage tracker (e.g. offscreens or screencasts). The intended use is for capture_framebuffer() to write into some texture stored in the Element. So imagine drawing to the main damage tracker, then drawing to a different damage tracker once (e.g. one window screencast frame). This other damage tracker will call capture_framebuffer() which will update the texture (stored on the Element). Then, we draw back to the main damage tracker, with something changed on top of the element. There were no changes below, as far as the main damage tracker is concerned, so capture_framebuffer() is not called. But draw() is called, and it ends up drawing texture contents from the window screencast frame, rather than the intended ones from the main scene.

Yeah caching the texture in the Element becomes problematic, if you use the same element across different framebuffers/damage-trackers. I guess this specifically becomes and issue with niri's block-on-screencast feature, right?

I am not sure how the code could address this without more information. The issue isn't really the different damage tracker, but that it potentially captured different contents, due to rendering for different intents, no?

Ideally it could remember the id's of the elements it captured somehow, so on the next render it would know, that there are different contents below instead of relying on the damage-tracker to store that information, but that seems not easily achievable.

Any ideas?

[Drakulix]

Since all issues in the damage-tracking algorithm seem to be addressed, I updated the PR again. I dropped the transform-argument, since this can be gathered from the frame and added a method to query the output_size as well.

[YaLTeR]

Yeah caching the texture in the Element becomes problematic, if you use the same element across different framebuffers/damage-trackers. I guess this specifically becomes and issue with niri's block-on-screencast feature, right?

Yeah. I think in niri I'll just make sure to use different copies of the element for different damage trackers, but it's still a bit error prone because you can't check that an issue is happening other than stumbling upon it.

I am not sure how the code could address this without more information. The issue isn't really the different damage tracker, but that it potentially captured different contents, due to rendering for different intents, no?

I think the issue is specifically different damage trackers, because the logic and state on whether a recapture needs to happen lies inside the damage tracker, and if you use two different ones, they don't share this info among each other.

Any ideas?

Not sure tbh, one workaround could be to have some kind of unique damage tracker id that the element could test (similarly to how elements can test the renderer context id), but idk how good an idea this is. And then again, the decision to recapture is done by the damage tracker and not by the element.

[Drakulix]

Not sure tbh, one workaround could be to have some kind of unique damage tracker id that the element could test (similarly to how elements can test the renderer context id), but idk how good an idea this is. And then again, the decision to recapture is done by the damage tracker and not by the element.

Not a bad idea. imo either the object holding the capture-state (the damage-tracker) needs to also cache the result (the texture) or the caching needs to be able to somehow reliably invalidate itself or split the cache into two. And we definitely want to encourage caching the effect result.

I guess the id could cause trouble, if the damage-tracker is destroyed later and the texture starts to leak potentially? Storing the texture in the damage-tracker also has it's problems, how is it passed to the draw call?

I wonder if we have the same issue somewhere without framebuffer-effect, but I guess just with damage this problem doesn't come up.

[cmeissl]

What about extending the whole rendering including the damage tracker with some external state context where the cached texture could be stored in. The context could be managed on a per use-case base, like output rendering, sceencapture and so on. Dropping the context would clean-up all cached textures.

[Drakulix]

What about extending the whole rendering including the damage tracker with some external state context where the cached texture could be stored in. The context could be managed on a per use-case base, like output rendering, sceencapture and so on. Dropping the context would clean-up all cached textures.

While I don't dislike the idea, I am failing to imagine the exact API for this. It seems very intrusive with everything from the DamageTracker's damage and render methods, RenderElement, AsRenderElements and probably more needing extensions for this new cache context.

Do you have other uses for this in mind? Should we also refactor the imported wl_buffer textures to use that context? I guess that would make it easier to do the import asynchronously from rendering, but given the textures are renderer-specific it is not like you can suddenly rely on textures being available, just because you use the same context. Would the context thus be specific to a ContextId?

I am struggling with with potentially adding a somehow overengineered feature, if there isn't a use-case for it outside of optimizing blur.

[YaLTeR]

Hmm, another point for consideration. Currently frame buffer effect elements won't properly work if several clones of an element are present in the render list, because all copies will overwrite the same framebuffer texture stored in them (assuming it's shared via Arc/Rc among all of them, e.g. a GlesTexture).

Where this comes up is e.g. niri's overview which draws background/bottom layer-shell multiple times (once per visible workspace).

[Drakulix]

Added a commit that makes it possible to use the update_surface_primary_scanout_output and surface_presentation_feedback_flags_from_states helpers with namespaced surface elements.

[Paraworker]

I noticed that Element already has the Kind enum. Would it make sense to represent framebuffer effects as another variant in Kind instead of introducing separate methods like is_framebuffer_effect?

[YaLTeR]

That Kind determines the scanout state (if the element should go on the Cursor plane, etc.) so it's orthogonal to is_framebuffer_effect.

[Paraworker]

That Kind determines the scanout state (if the element should go on the Cursor plane, etc.) so it's orthogonal to is_framebuffer_effect.

Ah, that makes sense. The name Kind misled me a bit at first.

[cmeissl]

Just a few nits from my side

[Drakulix]

@cmeissl Should all be addressed. Would appreciate a quick sanity check on the new commit before merging.

[cmeissl]

LGTM

[YaLTeR]

For the "Allow querying RenderElementStates with namespace" commit, is the idea for the compositor to keep track of and pick the "preferred" namespace for updating from render element states? I don't see a way to tell it "find element regardless of namespace", as far as I can tell passing None will only match unnamespaced elements.

So multiple unnamespaced clones will find "some" (undefined) single clone, but with a namespace you always have to pass the correct namespace (or correct absence of one), is that correct?