After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 733545 - GL element leaks FBO when caps changed dynamically
GL element leaks FBO when caps changed dynamically
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
1.4.0
Other All
: Normal normal
: 1.4.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-07-22 10:13 UTC by comicfans44
Modified: 2014-07-24 01:52 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
misc patch to fix FBO leak (1.73 KB, patch)
2014-07-22 10:13 UTC, comicfans44
none Details | Review
improved patch to fix more gl object leaks (4.63 KB, patch)
2014-07-23 02:32 UTC, comicfans44
reviewed Details | Review
improved patch v2 based on review suggestion. (4.35 KB, patch)
2014-07-23 14:20 UTC, comicfans44
committed Details | Review

Description comicfans44 2014-07-22 10:13:35 UTC
Created attachment 281373 [details] [review]
misc patch to fix FBO leak

if caps dynamically changed, decide_allocation callback of GstGLFilter/GstGLMixer didn't release previously allocated FBO.


attachment is a misc patch to fix this.
Comment 1 comicfans44 2014-07-22 11:36:32 UTC
plus, I found GstGLFilter didn't release in/out_tex_id  in gst_gl_filter_decide_allocation , except these, there're other GL texture leaks ( with apitrace) and I have not found all of them.
Comment 2 comicfans44 2014-07-23 02:32:31 UTC
Created attachment 281446 [details] [review]
improved patch to fix more gl object leaks
Comment 3 comicfans44 2014-07-23 02:56:47 UTC
seems that GLFilter only calls display_init_cb  but not display_reset_cb in decide_allocation , which makes all GLFilter sub class lacks chance to release texture in display_reset_cb ,should we call display_reset_cb first as well?
Comment 4 Nicolas Dufresne (ndufresne) 2014-07-23 03:19:10 UTC
Review of attachment 281446 [details] [review]:

I'll leave it to Matthew on final review, I just have one small comment. Thanks for taking the time tracking this.

::: gst-libs/gst/gl/gstglfilter.c
@@ +1105,3 @@
+    gst_gl_context_del_texture (filter->context, &filter->out_tex_id);
+    filter->out_tex_id = 0;
+  }

Just a nithpick, style would e better if you first free the resources, and then start creating them, would also keep it clean in case one of the creation failed.
Comment 5 Matthew Waters (ystreet00) 2014-07-23 08:05:26 UTC
Review of attachment 281446 [details] [review]:

Looks good.  Just some formatting nitpicks on top of Nicolas' comment.

Thanks for looking at this.

::: gst-libs/gst/gl/gstglfilter.c
@@ +280,3 @@
+    }
+
+

Spurious new line.

@@ +1083,3 @@
   out_height = GST_VIDEO_INFO_HEIGHT (&filter->out_info);
 
+  if (filter->fbo != 0) {

Any particular reason why the sudden use of != 0 ? :)

@@ +1093,3 @@
     goto context_error;
 
+

Spurious new line
Comment 6 comicfans44 2014-07-23 14:20:14 UTC
Created attachment 281479 [details] [review]
improved patch v2 based on review suggestion.

(In reply to comment #5)
> Review of attachment 281446 [details] [review]:
> 
> Looks good.  Just some formatting nitpicks on top of Nicolas' comment.
Thanks for reviewing, a improved patch with Nicolas' suggestion, release
FBO/texture before creating new ones ,this should also avoid leaks when new FBO 
creation failed. 

also cleaned != 0 code. I just quickly copied code from reset function to test if
this worked, without re-check the style .sorry for that :)
Comment 7 Matthew Waters (ystreet00) 2014-07-24 01:52:26 UTC
5d1ccc93665dbfeea747f184015afc4a9ec93e63
Author: Wang Xin-yu (王昕宇) <comicfans44@gmail.com>
Date:   Wed Jul 23 10:25:31 2014 +0800

    gl: fix multi gl object leaks
    
    1. fix FBO leaks in decide_allocation
    2. fix texture leaks in decide_allocation and reset
    3. fix texture leaks in FBO incomplete error path