GNOME Bugzilla – Bug 678301
audioringbuffer: introspection fixes and API clean up for bindings
Last modified: 2016-12-22 15:37:46 UTC
I'm working on Vala bindings, and I'm generating a decent number of small patches. Most of these are just adding introspection annotations, but a few are C API/ABI changes.
Created attachment 216654 [details] [review] toc setter: change GstTocSetterIFace to GstTocSetterInterface Without this GObject Introspection does not recognize the connection to GstTocSetter.
Created attachment 216655 [details] [review] introspection: assorted introspection and documentation fixes These changes are to clean up syntax issues such as missing colons, missing spaces, etc., and minor issues such as argument names in headers not matching the implementation and/or documentation.
Created attachment 216656 [details] [review] mini object: register as boxed type
Created attachment 216657 [details] [review] atomic queue: register as boxed type
Created attachment 216658 [details] [review] introspection: assorted introspection and documentation fixes in base
Created attachment 216659 [details] [review] adapter: add missing element-type annotations
(In reply to comment #3) > Created an attachment (id=216656) [details] [review] > mini object: register as boxed type I don't think this makes sense. A miniobject type cannot exist, you always need to make a 'subclass'. However the subclass will not be also a GstMiniObject, it's just a boxed type. Glib doesn't know about the parent miniobject type. This mean that if you make a GstMiniObject GValue, you would not be able to assign it a GstBuffer. As a result, you will never be able to use the GstMiniObject GType because there exist no instances of the type.
commit 8f6b6b8ab54d9caab4f4ce121cdbaec0d279a0e6 Author: Evan Nemerson <evan@coeus-group.com> Date: Fri Jun 15 16:56:46 2012 -0700 introspection: assorted introspection and documentation fixes in base commit 4705eb61c34fb4e99a03739a33d8bfc073cdcaed Author: Evan Nemerson <evan@coeus-group.com> Date: Fri Jun 15 18:35:05 2012 -0700 adapter: add missing element-type annotations commit c29168ed5e999b85c20e6ed8d21b105ed6c22a88 Author: Evan Nemerson <evan@coeus-group.com> Date: Fri Jun 15 16:14:49 2012 -0700 atomic queue: register as boxed type commit 6c6bb0e21739208fc85fb068983f3b86dc4d177a Author: Evan Nemerson <evan@coeus-group.com> Date: Fri Jun 15 16:43:30 2012 -0700 introspection: assorted introspection and documentation fixes These changes are to clean up syntax issues such as missing colons, missing spaces, etc., and minor issues such as argument names in headers not matching the implementation and/or documentation. commit 49ba9ef056c4c279029aa028508b958cc092c83b Author: Evan Nemerson <evan@coeus-group.com> Date: Fri Jun 15 14:50:48 2012 -0700 toc setter: change GstTocSetterIFace to GstTocSetterInterface Without this GObject Introspection does not recognize the connection to GstTocSetter.
(In reply to comment #7) > (In reply to comment #3) > > Created an attachment (id=216656) [details] [review] [details] [review] > > mini object: register as boxed type > > I don't think this makes sense. > > A miniobject type cannot exist, you always need to make a 'subclass'. However > the subclass will not be also a GstMiniObject, it's just a boxed type. Glib > doesn't know about the parent miniobject type. > > This mean that if you make a GstMiniObject GValue, you would not be able to > assign it a GstBuffer. > > As a result, you will never be able to use the GstMiniObject GType because > there exist no instances of the type. The problem from g-i's point of view is that, unless it is registered as a boxed type, functions which return a GstMiniObject* will be skipped. So you don't have access to make_writable, copy, ref, or unref.
Ok, so what's the actual problem we're trying to solve here for mini objects? Aren't mini objects for bindings still completely broken for many bindings because writability is still linked to the refcount (see bug #510301), and the bindings don't know about GstMiniObject or weak refs etc. ? Just exposing dedicated _ref/unref/copy/make_writable functions for each mini object still wouldn't really help, would it?
There are a lot of patches in the introspection-fixes branches of these repositories: git://github.com/nemequ/gstreamer.git git://github.com/nemequ/gst-plugins-base.git They contain a fairly large number of relatively small patches--feel free to combine them if you want to avoid polluting the git log (or not if you're like me and prefer small commits).
I pushed a couple more patches for gstreamer core to my repo based on feedback from Jens Georg, so it will need another merge. I've also rebased gst-plugins-base so it only contains the two patches which didn't get merged. The rationale for those two patches is: "appsink, appsrc: skip set_callbacks APIs for introspection": GObject Introspection does not support multiple callbacks sharing the same user_data, and definitely not multiple struct member callbacks sharing the same user_data. Rather than "fix" this API (which would require changing it to use one user_data per callback), I think simply skipping it makes more sense since the same functionality is offered through signals. "audioringbuffer: add GDestroyNotify to set_callback method": I think this one is pretty self-explanatory, but the idea is that there needs to be a way to free the user_data when it is no longer needed.
commit 490d16b88ac78701be460fa3d7a9aac15c77014b Author: Evan Nemerson <evan@coeus-group.com> Date: Sun Jul 29 15:44:45 2012 -0700 pad: add GST_PAD_LINK_CHECK_DEFAULT to GstPadLinkCheck This allows introspection-based bindings to access Gst.PadLinkCheck.DEFAULT instead of Gst.PAD_LINK_CHECK_DEFAULT. https://bugzilla.gnome.org/show_bug.cgi?id=678301 commit f1690812908cfab17aeec1b78a433a35e2d42054 Author: Evan Nemerson <evan@coeus-group.com> Date: Sun Jul 29 14:57:41 2012 -0700 buffer: mark gst_buffer_wrapped* data as array https://bugzilla.gnome.org/show_bug.cgi?id=678301 commit 823d27e8374d117974db054fb7c85aa801f8a6cb Author: Evan Nemerson <evan@coeus-group.com> Date: Tue Jul 24 13:26:00 2012 -0700 introspection: fix some warnings generated by g-ir-scanner. https://bugzilla.gnome.org/show_bug.cgi?id=678301 commit 237f707c758170c2d0fbdbb5e3bd6a9bfc165333 Author: Evan Nemerson <evan@coeus-group.com> Date: Mon Jul 30 21:46:18 2012 -0700 buffer: convert gst_buffer_* macros to functions GObject Introspection does not support macros. This is needed for bindings. We can still add back macros or inline functions again later if we think it's worth it. https://bugzilla.gnome.org/show_bug.cgi?id=678301
> "audioringbuffer: add GDestroyNotify to set_callback method": I think this one > is pretty self-explanatory, but the idea is that there needs to be a way to > free the user_data when it is no longer needed. Not sure if gst_audio_ring_buffer_release() is the right place to free the user_data - Wim?
Created attachment 223669 [details] [review] audioringbuffer: add GDestroyNotify to set_callback method Since it's just the one patch remaining I'm attaching it here and getting rid of the git repositories.
> > "audioringbuffer: add GDestroyNotify to set_callback method": I think this one > > is pretty self-explanatory, but the idea is that there needs to be a way to > > free the user_data when it is no longer needed. > > Not sure if gst_audio_ring_buffer_release() is the right place to free the > user_data - Wim? Wim, any opinion?
Too late I guess. Still don't think the patch is entirely right. Do you still need a variant with GDestroyNotify, or was it mostly for completeness/correctness? (I doubt anyone is going to need to use this API from a binding any time soon).
(In reply to comment #17) > Too late I guess. Still don't think the patch is entirely right. Oh well. If it's not right I don't mind changing it, I just need to know where I should be freeing the data. Of course now I'll have to add a gst_audio_ring_buffer_set_callback_full or similar. > Do you still need a variant with GDestroyNotify, or was it mostly for > completeness/correctness? (I doubt anyone is going to need to use this API from > a binding any time soon). It's required if that functionality is required. I can't speak to the likelihood of the API being used from bindings. As it was the function would simply have been skipped, but since I created that patch Olivier Crête added (scope async) to the user_data argument, which means that the callback is expected to be called exactly once (less than once will leak the user data, more than once will double-free). My understanding is that that is false, but if not then my patch is no longer required.
Review of attachment 223669 [details] [review]: This needs to be a "_full" variant now ::: gst-libs/gst/audio/gstaudioringbuffer.c @@ +675,3 @@ + if (buf->cb_data_notify) + buf->cb_data_notify (buf->cb_data); I think this needs to be in GObject::finalize() instead. It's not expected by users of the ringbuffer to set a new callback and data whenever the ringbuffer is released.
Evan?
Created attachment 279353 [details] [review] audioringbuffer: add set_callback_full for G-I Untested, since I have no idea how to create a RingBuffer. Would be nice if someone who knows gstreamer could throw together a unit test.
Thanks: commit 98064ed9bfee75bf6b438db38b87f2387ec0d491 Author: Evan Nemerson <evan@nemerson.com> Date: Thu Jun 26 18:01:06 2014 -0700 audioringbuffer: add set_callback_full() for g-i https://bugzilla.gnome.org/show_bug.cgi?id=678301