The real problem is that we don't have a proper mechanism for making elements return buffers to a pool (and make deep copies where possible).

Why are we not doing a deep copies here as well, like in the other patch?

It's not clear to me that just throwing away queued buffers is the right thing to do here for a DRAIN query? shouldn't it rather block until all these buffers have been consumed / pulled by the app?

>> shouldn't it rather block until all these buffers have been consumed
>> / pulled by the app?

Looks correct indeed and the app can deal with the following to clear any local ref it keeps:

pad = gst_element_get_static_pad (appsink, "sink");
gst_pad_add_probe (pad, GST_PAD_PROBE_TYPE_QUERY_DOWNSTREAM, query_cb, user_data, NULL);

static GstPadProbeReturn 
query_cb (GstPad * pad, GstPadProbeInfo * info, gpointer user_data) {

  GstQuery *query = GST_PAD_PROBE_INFO_QUERY (info);
    switch (GST_QUERY_TYPE (query)) {
      case GST_QUERY_DRAIN:
      {
        // TODO clear all local/old gst buffers that came out from appsink.
        break;
      }
      default:
        break;
    }
    return GST_PAD_PROBE_OK;
  }

}

Thx!

Review of attachment 358308 [details] [review]:

This will lead do data lost. Only a flush can destroy queued buffers. You can could wait for the queue to be consumed, but with appsink, you'll rely on the application.

Created attachment 358707 [details] [review]
appsink: on drain wait for buffers to be consumed

Wait for buffers to be consumed instead of clearing the queue. Also added unit test that would fail without this patch.

Review of attachment 358707 [details] [review]:

Looks good, but I would like not to see yet another racy test using g_usleep(). There is so many tests to fix, let's not add more.

::: tests/check/elements/appsink.c
@@ +535,3 @@
+  GstSample *pulled_sample = NULL;
+
+  g_usleep (1 * G_USEC_PER_SEC);

Would be nice to avoid g_usleep(), it makes the tests slow and racy.

Created attachment 359997 [details] [review]
appsink: on drain wait for buffers to be consumed

Addressed remarks.

Review of attachment 359997 [details] [review]:

looks good now.

Comment on attachment 359997 [details] [review]
appsink: on drain wait for buffers to be consumed

commit fc86194595955886df248452b3aee6bbdd9851cc
Author: Julien Isorce <jisorce@oblong.com>
Date:   Thu Aug 24 10:02:31 2017 +0100

    appsink: on drain wait for buffers to be consumed
    
    So that an upstream element can claim all buffers to return to its buffer pool.
    
    Added unit test 'test_query_drain'
      make elements/appsink.check
    
    https://bugzilla.gnome.org/show_bug.cgi?id=786739