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]

There should probably be a comment mentioning that frame borrows framebuffer, and the order of the fields should be preserved to maintain the drop order.

[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.