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 760873 - GstGLVideoMixerPad vertex_buffer leaks when pad dynamic removed
GstGLVideoMixerPad vertex_buffer leaks when pad dynamic removed
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
1.6.2
Other Linux
: Normal normal
: 1.13.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-01-20 04:17 UTC by comicfans44
Modified: 2018-01-29 10:07 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
misc patch to avoid pad's vertex buffer when dynamic release_pad (1.71 KB, patch)
2016-01-21 02:52 UTC, comicfans44
committed Details | Review
advance iterator in continue statement (1.82 KB, patch)
2016-02-24 02:53 UTC, comicfans44
committed Details | Review
fix the leak (1.59 KB, patch)
2016-10-03 12:12 UTC, Vincent Penquerc'h
committed Details | Review

Description comicfans44 2016-01-20 04:17:40 UTC
_reset_pad_gl (which release pad's vertex_buffer) only being called through

gst_gl_video_mixer_reset->_reset_gl

but when pads dynamic removed, it's not being called and leaks
Comment 1 Sebastian Dröge (slomo) 2016-01-20 07:48:22 UTC
Do you want to provide a patch for this?
Comment 2 comicfans44 2016-01-20 08:51:11 UTC
I've write a misc patch by override release_pad vmethod of GstGLVideoMixer,
but I'm not sure this is the correct way
Comment 3 Matthew Waters (ystreet00) 2016-01-21 02:07:08 UTC
That sounds like where it should occur. Attach the patch and we'll have a look.
Comment 4 comicfans44 2016-01-21 02:52:03 UTC
Created attachment 319478 [details] [review]
misc patch to avoid pad's vertex buffer when dynamic release_pad

I'm not sure if I should lock something in 
release_pad / _reset_pad_gl / gst_gl_video_mixer_callback 

before check and modify pad->vertex_buffer.


after applying this patch , some leaks disappear, but still one Buffer Object
leak reported by apitrace (recently it gains a basic leak trace function)

this leak should be gst_gl_video_mixer_callback:1146, gen pad's vertex buffer

I'm still tracing it so I didn't submit this early.
Comment 5 comicfans44 2016-01-21 04:50:41 UTC
after some trace, seems that video_mixer->process_textures(or maybe aggregator->aggregate) didn't consider pad remove event, I can confirm gst_gl_video_mixer_callback will use GstGLMixerFrameData after release_pad 
is called, so it may use invalid data (it has GstGLVideoMixerPad reference so  re-generated the vertex buffer after pad is released, trigger leaks).
Comment 6 Matthew Waters (ystreet00) 2016-02-16 23:33:45 UTC
These should remove the leak.

commit 5b1872e3873a2f27a61a2cfe01afc42996e27a0a
Author: Wang Xin-yu (王昕宇) <comicfans44@gmail.com>
Date:   Thu Jan 21 10:40:36 2016 +0800

    glvideomixer: don't leak pad's vertex buffer on release_pad
    
    https://bugzilla.gnome.org/show_bug.cgi?id=760873

commit ac690978f29c7addb90908fe2d597e8f8e0ba2b8
Author: Matthew Waters <matthew@centricular.com>
Date:   Wed Feb 17 01:08:18 2016 +1100

    glmixer: Remove usage of GstGLMixerFrameData
    
    Subclasses can just iterate over the list of pads themselves
    
    https://bugzilla.gnome.org/show_bug.cgi?id=760873
Comment 7 comicfans44 2016-02-24 02:53:11 UTC
Created attachment 322200 [details] [review]
advance iterator in continue statement

I've found commit ac69097 
glmixer: Remove usage of GstGLMixerFrameData
missed iterator advance in continue statement, leads a dead loop.
this misc patch address this.
Comment 8 Matthew Waters (ystreet00) 2016-02-24 07:31:39 UTC
Thanks!

commit 96ac4af7bfda488a44eb2fe99c48091361e5918f
Author: Wang Xin-yu (王昕宇) <comicfans44@gmail.com>
Date:   Wed Feb 24 10:45:17 2016 +0800

    glmixer: iterator didn't advance in continue statement
    
    Leading to a deadlock.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=760873
Comment 9 comicfans44 2016-03-17 07:13:16 UTC
I think this problem is not resolved. after insert some debug trace, I can confirm gst_gl_video_mixer_callback called after release_pad is called. maybe lock needed 

gstaggregator srcpad thread runs:
gst_aggregator_aggregate_func-> 
gst_videoaggregator_aggregate->
gst_gl_mixer_aggregate_frames->
gst_gl_video_mixer_process_textures-> 
call gst_gl_video_mixer_callback on gl thread and wait it finish


but gst_gl_video_mixer_release_pad can be called from gst_element_release_request_pad from any thread, these two thread should lock on something to ensure not access the pad which may be destroied 




0:00:00.842702829 11505 0x7fffc8001540    glvideomixer gstglvideomixer.c:1498:gst_gl_video_mixer_callback:<mixer:sink_0> gen vertex_buffer
0:00:00.845170634 11505 0x7fffd8002f00    glvideomixer gstglvideomixer.c:866:gst_gl_video_mixer_release_pad:<mixer:sink_0> release_pad
0:00:00.848196268 11505 0x7fffc8001540    glvideomixer gstglvideomixer.c:1498:gst_gl_video_mixer_callback:<mixer:sink_0> gen vertex_buffer
Comment 10 Matthew Waters (ystreet00) 2016-03-17 07:19:13 UTC
Can you share an example program ?
Comment 11 comicfans44 2016-03-23 03:38:10 UTC
(In reply to Matthew Waters (ystreet00) from comment #10)
> Can you share an example program ?

to confirm this situation, I add finalize to GstGLVideoMixerPadClass

static void
gst_gl_video_mixer_pad_finalize (GstGLVideoMixerPad * pad)
{ 
  if (pad->vertex_buffer) {
    g_assert (FALSE); --------------> should trap here
  }
  G_OBJECT_CLASS (g_type_class_peek_parent (G_OBJECT_GET_CLASS (pad)))
      ->finalize (pad);
}


example code as following:

if you change pipeline state and call gst_element_release_request_pad
on some other thread, this bug triggered. this is why I think some lock should be hold during test and set pad->vertex_buffer. 
gst_gl_video_mixer_callback holds GST_OBJECT_LOCK during painting,
but gst_gl_video_mixer_release_pad didn't have it (gst_element_release_request_pad have GST_OBJECT_LOCK, but that's after gst_gl_video_release_pad body run.)




#include <unistd.h>
#include <glib.h>
#include <gst/gst.h>
#include <gst/gstelement.h>
#include <gst/gstpad.h>
#include <gst/gstparse.h>


static GstBin *pipeline;
static GstElement *mixer;
static GstElement *prevsrc;

static void setSinkPad(GstElement* ele){

}

static
void changePipeline(){

  GstElement *next = gst_element_factory_make("videotestsrc", "to_change");

  GstClock *clock=GST_ELEMENT_CLOCK(pipeline);
  GstClockTime now=gst_clock_get_time(clock);

  gst_element_set_base_time(next,now);


  gst_element_set_state (prevsrc, GST_STATE_NULL);


  GstPad *old_request_pad=gst_pad_get_peer(
          gst_element_get_static_pad(prevsrc,"src"));

  
  gst_bin_remove (GST_BIN (pipeline), prevsrc);


  gst_element_release_request_pad(mixer,old_request_pad);

  gst_bin_add (GST_BIN (pipeline), next);

  GstPad* new_request_pad=gst_element_get_request_pad(mixer,"sink_%u");

  GstPad *srcPad=gst_element_get_static_pad(next,"src");

  gst_pad_link(srcPad,new_request_pad);

  g_object_set(G_OBJECT(new_request_pad),
            "xpos",400,"ypos",400,
            "width",400,"height",400,NULL);

  gst_element_set_state (next, GST_STATE_PLAYING);

  prevsrc=next;
}



static void thread_func(void *){

    while(true){

        sleep(1);

        changePipeline();

    }

}

static gboolean
start_thread(gpointer user_data)
{
    g_thread_new("test",(GThreadFunc)thread_func,NULL);

  return FALSE;
}


int
main (int argc, char **argv)
{

    gst_init(NULL,NULL);

    GMainLoop *loop=g_main_loop_new(NULL,FALSE);

    pipeline=(GstBin*)gst_parse_launch("videotestsrc name=no_change ! glvideomixer name=videomixer ! "
            "glimagesinkelement name=sink  videotestsrc name=to_change ! videomixer.",NULL);

    mixer=gst_bin_get_by_name(pipeline,"sink");

    prevsrc=gst_bin_get_by_name(pipeline,"to_change");

    GstPad *src=gst_element_get_static_pad(gst_bin_get_by_name(pipeline,"no_change"),"src");

    GstPad *sinkPad=gst_pad_get_peer(src);


    g_object_set(G_OBJECT(sinkPad),
            "xpos",0,"ypos",0,
            "width",400,"height",400,NULL);

    mixer=gst_bin_get_by_name(pipeline,"videomixer");

    GstPad* mixerPad1=gst_pad_get_peer(gst_element_get_static_pad(prevsrc,"src"));


    g_object_set(G_OBJECT(mixerPad1),
            "xpos",400,"ypos",400,
            "width",400,"height",400,NULL);

    g_timeout_add_seconds (5, start_thread, NULL); -->wait something already shown

    gst_element_set_state(GST_ELEMENT(pipeline),GST_STATE_PLAYING);

    g_main_loop_run(loop);


  return 0;
}
Comment 12 Sebastian Dröge (slomo) 2016-03-23 07:05:00 UTC
Please put code into attachments in the future
Comment 13 comicfans44 2016-03-23 07:22:44 UTC
(In reply to Sebastian Dröge (slomo) from comment #12)
> Please put code into attachments in the future

OK, I'd pay attention on this.
Comment 14 Tim-Philipp Müller 2016-05-22 17:39:04 UTC
Example code was provided.
Comment 15 Tim-Philipp Müller 2016-05-22 17:40:03 UTC
(Also, did you re-test this with 1.8 or master? Does it still happen with that?)
Comment 16 Vincent Penquerc'h 2016-09-30 15:30:05 UTC
It happens with current master still, I'm trying to work it out now.
Comment 17 Vincent Penquerc'h 2016-10-03 12:12:38 UTC
Created attachment 336802 [details] [review]
fix the leak
Comment 18 Vincent Penquerc'h 2016-10-03 12:14:06 UTC
Locking can't be done (except by adding an extra lock, but that adds complexity and deadlock risks) as the parent mixer pad finalize causes the lock to be taken to remove the pad from the aggregator, so deadlocks.
Comment 19 Tim-Philipp Müller 2018-01-29 10:06:52 UTC
Comment on attachment 336802 [details] [review]
fix the leak

commit 782fb43887ad5377c3dbc57f65ead1cd62cab18d (HEAD -> master)
Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk>
Date:   Mon Oct 3 13:11:07 2016 +0100

    glvideomixer: fix vertex_buffer leak
    
    We call the base class first as this will remove the pad from
    the aggregator, thus stopping misc callbacks from being called,
    one of which (process_textures) will recreate the vertex_buffer
    if it is destroyed
    
    https://bugzilla.gnome.org/show_bug.cgi?id=760873