Do you want to provide a patch for this?

I've write a misc patch by override release_pad vmethod of GstGLVideoMixer,
but I'm not sure this is the correct way

That sounds like where it should occur. Attach the patch and we'll have a look.

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.

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

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

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.

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

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

Can you share an example program ?

(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;
}

Please put code into attachments in the future

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

Example code was provided.

(Also, did you re-test this with 1.8 or master? Does it still happen with that?)

It happens with current master still, I'm trying to work it out now.

Created attachment 336802 [details] [review]
fix the leak

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