feature: Renderer Framebuffer type

[Drakulix]

Superseeds #1616.

Original description:

Attempt to solve #1615.

This solution isn't perfect, most notably it doesn't synchronize across the access of Bind<GlesTexture>. To do that we would need to be able to store the lock, which means moving the GlesTarget into the GlesFrame and making Bind return a Frame. I think we talked about this, so we should fix this at some point, but I didn't want to do that refactor now.

Fixing Bind would automatically solve this for exporting and other things as well, as all of these rely to binding a framebuffer container object to our texture.

(Also binding a texture to write to it and sampling from it in a separate thread is probably a operation not as common.)

I lied, I did the refactor. The changes to Renderer and Bind are fairly small, however the fallout was huge, but since none of this compiles without all the other changes, I wasn't sure how to split this apart further. Please excuse the giant PR/commit everybody.

[ids1024]

Have you seen this fix pop-os/cosmic-comp#1078, or any similar issues, or are the synchronization issues this fixes currently theoretical and not reproducible?

[Drakulix]

Yes I could reproduce it and the first commit already fixed that, given that this synchronizes the imports.

The second commit fixes synchronization across the board for shared contexts, but we haven't run into this in cosmic-comp as we don't bind shared-textures, but just render them.

[ids1024]

Not having an Rc<EGLSurface> in the target field will be helpful if we want to make GlesRenderer implement Send (other than that, it has a pointer in gl_debug_span, GlesBuffer::image, and the _not_send member.) Not that I have a specific use for that, but it could be useful, if there are no issues with doing that.

[ids1024]

Needing a RendererSuper is annoying, but I guess not that bad as workarounds go.

[ids1024]

There's a CI failure, but it looks simple enough.

I like changing Bind::bind to take a reference and return a framebuffer.

I guess I might need to think about it for a while before I have any sense if the AliasableBox/transmute part is sound... (Not sure if this will also make abstracting gles/pixman more complicated too)

[Drakulix]

I guess I might need to think about it for a while before I have any sense if the AliasableBox/transmute part is sound... (Not sure if this will also make abstracting gles/pixman more complicated too)

https://morestina.net/blog/1868/self-referential-types-for-fun-and-profit was the inspiration for this once I realized that this is simply self-referential.

Though I'll leave seeing though those lifetime to you. I have stared long enough at the code to be confident, that it is sound, but I would really like somebody to come to the same conclusion. 😅

[nferhat]

I think RenderContext::draw should get a FnOnce(&mut T) instead of FnOnce(&T)

smithay/src/backend/renderer/element/texture.rs

Lines 594 to 597 in fe31867

	pub fn draw<F, E>(&mut self, f: F) -> Result<(), E>
	where
	F: FnOnce(&T) -> Result<Vec<Rectangle<i32, Buffer>>, E>,
	{

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 74.31193% with 28 lines in your changes missing coverage. Please review.

Project coverage is 19.31%. Comparing base (4689174) to head (5786ade).
Report is 17 commits behind head on master.

Files with missing lines	Patch %	Lines
src/backend/renderer/damage/mod.rs	90.62%	6 Missing ⚠️
src/backend/renderer/element/utils/elements.rs	0.00%	6 Missing ⚠️
src/backend/renderer/element/texture.rs	0.00%	5 Missing ⚠️
src/backend/renderer/test.rs	61.53%	5 Missing ⚠️
src/backend/egl/context.rs	0.00%	3 Missing ⚠️
src/backend/renderer/element/memory.rs	0.00%	2 Missing ⚠️
src/backend/renderer/mod.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1637      +/-   ##
==========================================
- Coverage   19.33%   19.31%   -0.02%     
==========================================
  Files         171      171              
  Lines       28375    28391      +16     
==========================================
  Hits         5485     5485              
- Misses      22890    22906      +16     

Flag	Coverage Δ
wlcs-buffer	16.82% <74.31%> (+<0.01%)	⬆️
wlcs-core	16.57% <74.31%> (+<0.01%)	⬆️
wlcs-output	6.82% <25.68%> (+<0.01%)	⬆️
wlcs-pointer-input	18.28% <74.31%> (-0.02%)	⬇️

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.

[ids1024]

Okay, after reading through https://morestina.net/blog/1868/self-referential-types-for-fun-and-profit, and looking through that part of the code here, I understand why this makes the struct self-referential, and it seems like a relatively straightforward case of that. As the article mentions, transmute isn't as scary as usual when it's just extending the lifetime.

So I think that part of the code looks correct, as does everything else here.

Alternatives:

• It should be possible to use ouroboros, but that's not especially more convenient or understandable, and would add more compile time in macros, only to generate something similar use aliasable. (As much as I'd like to just trust a widely used crate with the unsafe part.)
• I guess we could avoid this problem by making Renderer::render take a callback instead of returning a Frame<'_, '_>. But the existing API is somewhat more convenient.

So overall, this seems like a good approach. Hopefully Rust will have a better way to deal with self-referential types at some point.

[cmeissl]

We should allow to bind an Image with non 'static data lifetime. This would allow callers to bind byte[] slices in a safe way without having to rely on the ugly hack we do in the DrmCompositor for binding the dumb buffer slice.

[Drakulix]

I couldn't find a way to introduce the necessary bound of Image<'a, 'a> outlives PixmanTarget<'b>. I want to add a where 'a: 'b-bound to bind, but that isn't compatible with the trait definition.

Adding 'b to Bind<'b> doesn't help, as we still can't express the bound in the trait, Target: 'b doesn't help.

Nor does where Target: 'a even though that should imply, that bind should only be available if Image<'b, 'b> outlives PixmanTarget<'a>. The compiler doesn't seem smart enough.

Adding another lifetime to PixmanTarget makes it impossible to implement RendererSuper.

I couldn't find a way to specify the additional requirement with an higher-order bound (for<'b>), but that feels like what is necessary here.

[cmeissl]

I see, that's quite unfortunate. I also unsuccessfully tried to find a safe way to express this.
Would be really great if we could use this to basically bind a scoped slice :/

Theoretically is should just work as the 'bits inside of the Image are already guaranteed to outlive any reference to the Image. In PixmanTarget/PixmanTargetInternal we only care that the 'bits of the Image live at least as long as the reference of the image itself. So the only thing blocking this is that we can not prove it to the compiler. I wonder if it would be safe to just cast the lifetime in bind.

[cmeissl]

Quick test of the change: cmeissl@0e5eee5