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 526814 - [API] add gst_pad_add_{data,event,buffer}_probe_full() with disconnect notify function
[API] add gst_pad_add_{data,event,buffer}_probe_full() with disconnect notify...
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal enhancement
: 0.10.20
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2008-04-07 20:49 UTC by José Alburquerque
Modified: 2008-05-07 15:55 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch to allow GstPad to notify when data, event or buffer probes have been disconnected (7.29 KB, patch)
2008-04-07 20:51 UTC, José Alburquerque
none Details | Review
Updatad patch (6.92 KB, patch)
2008-04-07 23:12 UTC, José Alburquerque
none Details | Review

Description José Alburquerque 2008-04-07 20:49:47 UTC
This patch adds the possibility of being informed when one of the GstPad probe handlers have been disconnected.  Would it be possible to include this so that C++ bindings can delete data that depends on the probe but should be deleted when the probe is disconnected?  Thanks.
Comment 1 José Alburquerque 2008-04-07 20:51:09 UTC
Created attachment 108811 [details] [review]
Patch to allow GstPad to notify when data, event or buffer probes have been disconnected
Comment 2 Tim-Philipp Müller 2008-04-07 22:52:21 UTC
This makes sense to have, I guess. IIRC the python bindings had the same issue, but just implemented those functions themselves (everything required is accessible via public functions, after all).

As for the patch:

Why do you use G_CONNECT_AFTER for g_signal_connect_data()?

And why special-case the if (disconnect_notify) case rather than using g_signal_connect_data for both cases?
Comment 3 José Alburquerque 2008-04-07 23:03:10 UTC
(In reply to comment #2)
> This makes sense to have, I guess. IIRC the python bindings had the same issue,
> but just implemented those functions themselves (everything required is
> accessible via public functions, after all).
> 
> As for the patch:
> 
> Why do you use G_CONNECT_AFTER for g_signal_connect_data()?
> 
> And why special-case the if (disconnect_notify) case rather than using
> g_signal_connect_data for both cases?
> 

You're right.  It is possible to use g_signal_connect_data for both cases.  The G_CONNECT_AFTER I wasn't sure about, it just seemed like g_signal_connect_data required something for connect_flags and G_CONNECT_SWAPPED did not seem right.  If the flags are not required, the patch can be modified (I'd do it).

As for the patch, if gst-python implemented these methods themselves, maybe we can also.  Would you want a fixed patch anyway?
Comment 4 José Alburquerque 2008-04-07 23:12:40 UTC
Created attachment 108826 [details] [review]
Updatad patch

I've attached updated patch in case desired
Comment 5 José Alburquerque 2008-04-07 23:27:32 UTC
(In reply to comment #3)
> As for the patch, if gst-python implemented these methods themselves, maybe we
> can also.  Would you want a fixed patch anyway?
> 
I just thought about this, and it doesn't seem to make sense to re-implement the gst_pad_add_{buffer,event,data}_probe method functionality outside of core; wouldn't that cause possible digression from core functionality (if core functions change)?  If you think it makes sense, would you please consider this patch?  Thanks.
Comment 6 Tim-Philipp Müller 2008-04-09 18:10:13 UTC
Last question: is the GClosureNotify significant or useful from a bindings point of view? I mean: do you need the closure argument passed to the notify callback in the bindings?

If not, it might be less confusing for other users of the API to expose a GDestroyNotify data_destroy instead of a GClosureNotify (passing extra arguments to C functions shouldn't cause any problems afaik).
Comment 7 José Alburquerque 2008-04-09 19:20:19 UTC
(In reply to comment #6)
> Last question: is the GClosureNotify significant or useful from a bindings
> point of view? I mean: do you need the closure argument passed to the notify
> callback in the bindings?
> 

For our purposes it does.  Let me explain.  In our bindings, when a probe is added to the pad, we create extra data (a pointer to a C++ callback called a slot) which is used to call the developer's C++ function with C++ type arguments.  We add the probe with a pointer to the extra data and in the C callback we cast the extra data to the C++ callback type (the slot) and then call the user's function with the arguments "wrapped" in C++ types that the user can use in C++ style.

The GClosureNotify is used so that when the probe is removed (the signal is disconnected), a callback is invoked in which case we can then remove the extra data.  Does that make sense?

> If not, it might be less confusing for other users of the API to expose a
> GDestroyNotify data_destroy instead of a GClosureNotify (passing extra
> arguments to C functions shouldn't cause any problems afaik).
> 

The problem, I think, is that a GDestroyNotify does not notify when signals are disconnected.  Am I mistaken?  If I could find a way to be informed of when the probe is removed, the extra data could then be removed in this way.  Do you think this is a bit of a burden?  Thanks.
Comment 8 Tim-Philipp Müller 2008-04-09 20:45:41 UTC
> The GClosureNotify is used so that when the probe is removed (the signal is
> disconnected), a callback is invoked in which case we can then remove the extra
> data.  Does that make sense?

Of course.

> The problem, I think, is that a GDestroyNotify does not notify when signals are
> disconnected.

It would in this case. It would be the exact same thing, the question is just what the notify callback is supposed to look like (GDestroyNotify only has one argument, namely the data argument, while GClosureNotify has two, namely the data argument and a GClosure argument). I guess this is a bit hacky and C-ish, I just think exposing a GClosureNotify might be confusing, that's all. I'll ask some other people for opinions.
Comment 9 José Alburquerque 2008-04-09 20:57:10 UTC
(In reply to comment #8)
> > The problem, I think, is that a GDestroyNotify does not notify when signals are
> > disconnected.
> 
> It would in this case. It would be the exact same thing, the question is just
> what the notify callback is supposed to look like (GDestroyNotify only has one
> argument, namely the data argument, while GClosureNotify has two, namely the
> data argument and a GClosure argument). I guess this is a bit hacky and C-ish,
> I just think exposing a GClosureNotify might be confusing, that's all. I'll ask
> some other people for opinions.
> 

If the GDestroyNotify notifies on signal disconnection (as GClosureNotify does), it would fine in this case.  I see what you mean by the callback needing two arguments for a GClosureNotify.  In this case, the GClosure argument is not used (all that the bindings need is the data pointer).  If it's ok, I'll make changes using GDestroyNotify and submit a new patch.
Comment 10 José Alburquerque 2008-04-09 21:12:00 UTC
(In reply to comment #9)
> (In reply to comment #8)
> > > The problem, I think, is that a GDestroyNotify does not notify when signals are
> > > disconnected.
> > 
> > It would in this case. It would be the exact same thing, the question is just
> > what the notify callback is supposed to look like (GDestroyNotify only has one
> > argument, namely the data argument, while GClosureNotify has two, namely the
> > data argument and a GClosure argument). I guess this is a bit hacky and C-ish,
> > I just think exposing a GClosureNotify might be confusing, that's all. I'll ask
> > some other people for opinions.
> > 
> 
> If the GDestroyNotify notifies on signal disconnection (as GClosureNotify
> does), it would fine in this case.  I see what you mean by the callback needing
> two arguments for a GClosureNotify.  In this case, the GClosure argument is not
> used (all that the bindings need is the data pointer).  If it's ok, I'll make
> changes using GDestroyNotify and submit a new patch.
> 

On second thought, if you're able to ask others for opinions, you may be able to find how to best do this.  I don't quite understand what needs to be done to use a GDestroyNotify so I'll leave it in your hands to advise as to how to best proceed.  Thanks.
Comment 11 Tim-Philipp Müller 2008-04-10 20:47:35 UTC
Thanks, committed with a few changes (most importantly: fix the argument ordering in the non-_full functions from NULL, data to data, NULL, so that the unit tests pass again instead of crashing):


 2008-04-10  Tim-Philipp Müller  <tim at centricular dot net>

        Based on patch by: José Alburquerque <jaalburqu at svn dot gnome dot org>

        * gst/gstutils.c: (gst_pad_add_data_probe),
          (gst_pad_add_data_probe_full), (gst_pad_add_event_probe),
          (gst_pad_add_event_probe_full), (gst_pad_add_buffer_probe),
          (gst_pad_add_buffer_probe_full):
        * gst/gstutils.h:
        * docs/gst/gstreamer-sections.txt:
        * win32/common/libgstreamer.def:
          Add gst_pad_add_*_probe_full() functions with a notify callback that
          lets the caller free the data it passes to the probe functions. This
          is useful for bindings such as gst-python or gstreamermm (#526814).
          API: gst_pad_add_data_probe_full
          API: gst_pad_add_buffer_probe_full
          API: gst_pad_add_event_probe_full

        * tests/check/gst/gstutils.c:
          Add minimal unit test to make sure freeing the data actually works
          as expected.

        * tests/benchmarks/.cvsignore:
          Random cvsignore addendum.

Comment 12 Murray Cumming 2008-05-07 15:52:19 UTC
Is this in any tarball version yet?
Comment 13 Tim-Philipp Müller 2008-05-07 15:55:40 UTC
No, it will be in the 0.10.20 release scheduled for mid-June.