-
Notifications
You must be signed in to change notification settings - Fork 342
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! Here are a few comments.
This needs to be rebased on top of #2593.
@@ -276,6 +275,9 @@ struct wlr_renderer *wlr_renderer_autocreate(struct wlr_egl *egl, | |||
config_attribs = all_config_attribs; | |||
} | |||
|
|||
struct wlr_egl *egl = calloc(1, sizeof(*egl)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This memory is leaked when the renderer is destroyed.
I think long-term we'll want to either:
- Allocate the
wlr_egl
as part of thewlr_gles2_renderer
. This meanswlr_gles2_renderer_create
would need to take all arguments necessary for the call towlr_egl_init
. - Make
wlr_gles2_renderer_create
take over thewlr_egl
(kind of like you're doing in this PR), and replacewlr_egl_init
/wlr_egl_finish
withwlr_egl_create
/wlr_egl_destroy
so that the GLES2 renderer can cleanly free thewlr_egl
(this would ensuring it's not part of a larger structure).
For this PR we can probably just free(renderer->egl)
when destroying the GLES2 renderer, and properly document that wlr_gles2_renderer_create
takes ownership of the wlr_egl
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, I was sure I added the free
in the wlr_renderer_destroy
function. Good catch.
I like the idea of wlr_gles2_renderer
becoming the owner of wlr_egl
, it makes more sense when we will add the pixman backend.
I'll slap a nice comment and a TODO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I slightly edited the PR to also add a note in the header defining wlr_gles2_renderer_create
.
Also, is it possible to get rid of the unnecessary includes in a separate commit? |
b3037ae
to
2ca7142
Compare
2ca7142
to
1fdd175
Compare
There you we go, I can reword the commit if you want, I put in something generic since it affects two both the backend and the render folder. I also rebased the patch to get the modifications from #2563 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Removes the
wlr_egl
member from various backend to keep it into thewlr_renderer
. This also removes various includes of therender/egl.h
header that were made useless by this patch.References: #2563
Breaking change:
wlr_renderer_autocreate
no longer takes astruct wlr_egl *
as parameter.