add destroy listener for textures #1962

Emantor · 2019-12-22T11:16:54Z

Add a destroy listener for textures, which destroys the gles2 textures
when the renderer is destroyed.

Fixes #1949

I am not entirely sure about the API changes, since the wlr_renderer is now passed directly to the texture creation functions. I am all ears for possible improvements.

emersion · 2019-12-25T10:28:16Z

The intent with the current API was to allow compositors to create wlr_textures without having to use wlr_renderer. However it doesn't seem like this is used by anyone (@ammen99?), since wlr_surface requires wlr_renderer anyway.

emersion · 2019-12-25T22:06:05Z

See #775

An alternative would be to add a destroy signal to wlr_egl.

Emantor · 2019-12-29T11:57:22Z

Pushed a new version which moves the destruction function to wlr_texture instead of being gles2 specific. I can also separate out the gles2 texture creation function into its own commit if required.

emersion · 2019-12-29T12:06:01Z

This is still GLES2 specific, as the renderer passed to the texture creation functions take a wlr_renderer which is asserted to be the GLES2 renderer.

Emantor · 2019-12-29T12:15:10Z

Yes, since there is no other renderer. But the listener is now registered for all wlr_textures in the init function. The texture creation functions obv. need to assert since they are gles2 specific. Only the texture destruction part on renderer destroy should now work for all wlr_texture parts, if we had more than gles2.

ammen99 · 2019-12-29T14:43:31Z

However it doesn't seem like this is used by anyone (@ammen99?), since wlr_surface requires wlr_renderer anyway.

I am not using any of the functions which are touched here, so I am not against this change. Renderers are supposed to be redesigned sometime in the future anyway IIRC.

emersion · 2020-01-05T00:07:43Z

Maybe we should just hide these functions from the public API for now.

Emantor · 2020-02-20T07:26:13Z

Maybe we should just hide these functions from the public API for now.

Should this be done in this PR? Is there anything else blocking this PR?

emersion · 2020-02-21T16:45:14Z

Yeah, since this PR contains breaking changes, can we make these functions private?

Emantor · 2020-02-22T14:03:07Z

Done, I also cleaned up some gles2.h imports.

emersion · 2020-02-22T17:10:19Z

Are wlr_texture users prepared for the texture to be implicitly destroyed?

include/render/gles2.h

Emantor · 2020-03-02T13:11:14Z

Are wlr_texture users prepared for the texture to be implicitly destroyed?

Not sure how to verify this short of verifying all callers, I have been running this PR on top of master with address sanitizer and tested output hotplug so far.

emersion · 2020-03-02T17:29:52Z

Grepping for wlr_texture:

backend/drm/renderer.c: since the backend has ownership over the renderer it should be fine
types/wlr_output.c: used for cursors, would be safer to add a listener
types/wlr_buffer.c: should add a listener

Emantor · 2020-03-04T20:02:48Z

Grepping for wlr_texture:

* `backend/drm/renderer.c`: since the backend has ownership over the renderer it should be fine

* `types/wlr_output.c`: used for cursors, would be safer to add a listener

Looks straightforward.

* `types/wlr_buffer.c`: should add a listener

Hm, how do I handle the reference counting? Just ignore, send a release for the buffer and subsequently destroy?

emersion · 2020-03-30T15:09:37Z

Hm, how do I handle the reference counting? Just ignore, send a release for the buffer and subsequently destroy?

We shouldn't release the buffer. We could set texture to NULL instead (this would be a breaking change I think? However most callers should already be prepared for it via wlr_surface_get_texture).

Add a destroy listener for textures, which destroys the textures of a renderer upon renderer destruction. Fixes swaywm#1949

Remove unnecessary imports and if necessary replace with the correct ones.

Remove the prefix, move the functions and remove the wrappers inside the renderer.

In case the renderer is destroyed, it will implicitly destroy the textures. Set buffer->texture to NULL to prevent use after free and guard against a NULL texture in wlr_client_buffer_apply_damage.

Listen for a renderer destory event and remove the reference to the to be destroyed texture from the cursor.

Emantor · 2020-04-18T11:17:24Z

This should be ready for review.

emersion · 2020-04-29T08:43:39Z

include/wlr/render/wlr_texture.h

@@ -18,6 +18,8 @@ struct wlr_texture_impl;

 struct wlr_texture {
 	const struct wlr_texture_impl *impl;
+
+	struct wl_listener renderer_destroy;


This listener may run before or after other renderer destroy listeners (listener call order is undefined). This can cause a use-after-free if the texture listener is called first, then the buffer/output listener is called.

We should probably add a destroy signal to wlr_texture to prevent this.

These functions are unused by compositors (see e.g. [1]) and prevent wlr_gles2_texture from accessing wlr_gles2_renderer state. This is an issue for proper teardown [2] and for accessing GLES2 extensions. [1]: swaywm#1962 (comment) [2]: swaywm#1962

These functions are unused by compositors (see e.g. [1]) and prevent wlr_gles2_texture from accessing wlr_gles2_renderer state. This is an issue for proper teardown [2] and for accessing GLES2 extensions. [1]: #1962 (comment) [2]: #1962

Emantor · 2020-12-04T16:23:12Z

Will be superseded by usage of wlr_buffer

emersion · 2020-12-04T16:25:58Z

Hm, I don't think so. wlr_texture will remain as the abstraction for pixels ready to be textured from by the renderer.

Emantor · 2020-12-04T16:39:08Z

Hm, I don't think so. wlr_texture will remain as the abstraction for pixels ready to be textured from by the renderer.

Yep, but I remember the last place where this was lacking to be the cursor code having no nice place to hook into. And with your reworks in flight currently (and this PR not having been rebased in a while), I'll see how things turn out in master. I intend to pick this up again, but it will be a while, so closing this for now.

emersion · 2020-12-04T16:53:12Z

I see! No problem, thanks for working on this :)

Emantor force-pushed the topic/texture_destroy_listener branch from 62023ab to f6e6029 Compare December 22, 2019 15:07

ddevault added the breaking Breaking change in public API label Dec 23, 2019

Emantor force-pushed the topic/texture_destroy_listener branch from f6e6029 to 197e5b5 Compare December 29, 2019 11:56

Emantor force-pushed the topic/texture_destroy_listener branch from 197e5b5 to d656a7c Compare February 22, 2020 13:33

emersion reviewed Feb 24, 2020

View reviewed changes

include/render/gles2.h Outdated Show resolved Hide resolved

Emantor force-pushed the topic/texture_destroy_listener branch from 7a155c7 to 5c82e0d Compare March 2, 2020 13:00

Emantor added 5 commits April 18, 2020 13:14

add destroy listener for textures

0fde1dd

Add a destroy listener for textures, which destroys the textures of a renderer upon renderer destruction. Fixes swaywm#1949

multiple: remove unnecessary gles2.h imports

54bd6d9

Remove unnecessary imports and if necessary replace with the correct ones.

gles2: make texture functions private

dbd702a

Remove the prefix, move the functions and remove the wrappers inside the renderer.

Handle renderer destroy for wlr_client_buffer

55bac6a

In case the renderer is destroyed, it will implicitly destroy the textures. Set buffer->texture to NULL to prevent use after free and guard against a NULL texture in wlr_client_buffer_apply_damage.

wlr_output: cursor: listen for renderer destroy

c3773c0

Listen for a renderer destory event and remove the reference to the to be destroyed texture from the cursor.

Emantor force-pushed the topic/texture_destroy_listener branch from 5c82e0d to c3773c0 Compare April 18, 2020 11:15

emersion reviewed Apr 29, 2020

View reviewed changes

Emantor closed this Dec 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add destroy listener for textures #1962

add destroy listener for textures #1962

Emantor commented Dec 22, 2019

emersion commented Dec 25, 2019

emersion commented Dec 25, 2019

Emantor commented Dec 29, 2019

emersion commented Dec 29, 2019

Emantor commented Dec 29, 2019 •

edited

ammen99 commented Dec 29, 2019

emersion commented Jan 5, 2020

Emantor commented Feb 20, 2020

emersion commented Feb 21, 2020

Emantor commented Feb 22, 2020

emersion commented Feb 22, 2020

Emantor commented Mar 2, 2020 •

edited

emersion commented Mar 2, 2020

Emantor commented Mar 4, 2020

emersion commented Mar 30, 2020

Emantor commented Apr 18, 2020

emersion Apr 29, 2020

Emantor commented Dec 4, 2020

emersion commented Dec 4, 2020

Emantor commented Dec 4, 2020

emersion commented Dec 4, 2020

add destroy listener for textures #1962

add destroy listener for textures #1962

Conversation

Emantor commented Dec 22, 2019

emersion commented Dec 25, 2019

emersion commented Dec 25, 2019

Emantor commented Dec 29, 2019

emersion commented Dec 29, 2019

Emantor commented Dec 29, 2019 • edited

ammen99 commented Dec 29, 2019

emersion commented Jan 5, 2020

Emantor commented Feb 20, 2020

emersion commented Feb 21, 2020

Emantor commented Feb 22, 2020

emersion commented Feb 22, 2020

Emantor commented Mar 2, 2020 • edited

emersion commented Mar 2, 2020

Emantor commented Mar 4, 2020

emersion commented Mar 30, 2020

Emantor commented Apr 18, 2020

emersion Apr 29, 2020

Choose a reason for hiding this comment

Emantor commented Dec 4, 2020

emersion commented Dec 4, 2020

Emantor commented Dec 4, 2020

emersion commented Dec 4, 2020

Emantor commented Dec 29, 2019 •

edited

Emantor commented Mar 2, 2020 •

edited