GNOME Bugzilla – Bug 710342
docs: various constructors are annotated as "transfer full", but return floating references
Last modified: 2014-02-06 21:51:30 UTC
Looking at gst_task_new: /** * gst_task_new: * @func: The #GstTaskFunction to use * @user_data: User data to pass to @func * @notify: the function to call when @user_data is no longer needed. * * Create a new Task that will repeatedly call the provided @func * with @user_data as a parameter. Typically the task will run in * a new thread. * * The function cannot be changed after the task has been created. You * must create a new #GstTask to change the function. * * This function will not yet create and start a thread. Use gst_task_start() or * gst_task_pause() to create and start the GThread. * * Before the task can be used, a #GRecMutex must be configured using the * gst_task_set_lock() function. This lock will always be acquired while * @func is called. * * Returns: (transfer full): A new #GstTask. * * MT safe. */ GstTask * gst_task_new (GstTaskFunction func, gpointer user_data, GDestroyNotify notify) { GstTask *task; task = g_object_newv (GST_TYPE_TASK, 0, NULL); task->func = func; task->user_data = user_data; task->notify = notify; GST_DEBUG ("Created task %p", task); return task; } Since GstTask derives from GstObject (and thus, GInitiallyUnowned), shouldn't g_object_newv (GST_TYPE_TASK, ...) return a floating reference?
Created attachment 257468 [details] [review] Change documentation to say that gst_task_new returns a floating reference Patch in case the documentation is just wrong.
Same question with GstBus: /** * gst_bus_new: * * Creates a new #GstBus instance. * * Returns: (transfer full): a new #GstBus instance */ GstBus * gst_bus_new (void) { GstBus *result; result = g_object_newv (gst_bus_get_type (), 0, NULL); GST_DEBUG_OBJECT (result, "created new bus"); return result; }
Created attachment 257469 [details] [review] Change documentation for GstObject derived classes' new functions returning floating references I was curious about this, so here's a patch updating the documentation for every *_new function for a GstObject-derived class, to say that they return floating references.
Oh I see, I need to look at gst_*_init: static void gst_bus_init (GstBus * bus) { bus->priv = G_TYPE_INSTANCE_GET_PRIVATE (bus, GST_TYPE_BUS, GstBusPrivate); bus->priv->enable_async = DEFAULT_ENABLE_ASYNC; g_mutex_init (&bus->priv->queue_lock); bus->priv->queue = gst_atomic_queue_new (32); /* clear floating flag */ gst_object_ref_sink (bus); GST_DEBUG_OBJECT (bus, "created"); } I don't see anything like that in GstTask though: static void gst_task_init (GstTask * task) { GstTaskClass *klass; klass = GST_TASK_GET_CLASS (task); task->priv = GST_TASK_GET_PRIVATE (task); task->running = FALSE; task->thread = NULL; task->lock = NULL; g_cond_init (&task->cond); SET_TASK_STATE (task, GST_TASK_STOPPED); /* use the default klass pool for this task, users can * override this later */ g_mutex_lock (&pool_lock); task->priv->pool = gst_object_ref (klass->pool); g_mutex_unlock (&pool_lock); }
Looking through all of this code was too tedious, so I wrote a Python program to write a C program to just run the code and figure out the results. The program runs each constructor in my previous patch and then checks g_object_is_floating. The output is: gst_buffer_pool_new returns floating gst_bus_new returns transfer full gst_task_new returns floating gst_task_pool_new returns floating gst_collect_pads_new returns floating gst_test_clock_new returns transfer full gst_interpolation_control_source_new returns transfer full gst_lfo_control_source_new returns transfer full gst_trigger_control_source_new returns transfer full
Created attachment 257571 [details] Program which runs various constructors and checks if the result is_floating Here's the C program I used to test this. Compile like: libtool --mode=link gcc `gst-git pkg-config --cflags --libs gstreamer-1.0 gstreamer-check-1.0 gstreamer-base-1.0 gstreamer-controller-1.0` test.c -o test
Created attachment 257572 [details] [review] Fix documentation for constructors that return floating references And here's a patch that fixes the documentation for the constructors that that program says return floating references.
Created attachment 257581 [details] Program which runs various constructors and checks if the result is_floating gst_pad_new_from_*template functions are also documented wrong: gst_buffer_pool_new returns floating gst_bus_new returns transfer full gst_task_new returns floating gst_task_pool_new returns floating gst_collect_pads_new returns floating gst_test_clock_new returns transfer full gst_interpolation_control_source_new returns transfer full gst_lfo_control_source_new returns transfer full gst_trigger_control_source_new returns transfer full gst_pad_new returns floating gst_pad_new_from_template returns floating gst_pad_new_from_static_template returns floating
Created attachment 257584 [details] [review] Patch to fix documentation for constructors that return floating references Here's an update to the patch, including GstPad.
Created attachment 257589 [details] Program which runs various constructors and checks if the result is_floating I made this program dramatically simpler, so hopefully it's easier to verify.
Review of attachment 257584 [details] [review]: ::: gst/gsttask.c @@ +391,3 @@ * @func is called. * + * Returns: (transfer floating): A new #GstTask. This shouldn't be floating IMHO, so we should unset the flag. Floating only makes sense if the object becomes the child of some other object ::: gst/gsttaskpool.c @@ +159,3 @@ * GThreadPool for threads. * + * Returns: (transfer floating): a new #GstTaskPool. gst_object_unref() after usage. Same here ::: libs/gst/base/gstcollectpads.c @@ +291,3 @@ * MT safe. * + * Returns: (transfer floating): a new #GstCollectPads, or NULL in case of an error. and here
Should GstBufferPool be floating?
Created attachment 258618 [details] [review] Document floating reference for gst_pad_new_from_*_template I split this into two patches since one is just documentation and the other actually changes code. This one fixes the documentation for gst_pad_new_from_template and gst_pad_new_from_static_template.
Created attachment 258619 [details] [review] Clear floating reference in various contructors And here's the one that clears the floating references for the rest of these.
I think GstBufferPool could make sense as a floating reference, we should probably set the element that created it and is connected to it as parent of the GstBufferPool. GstCollectPads could have its element as parent too, so maybe makes sense to keep it there too. For the others it should be OK to remove the floating flag, but I'll let others comment too
What should we do about this?
Are you waiting on a response from me, or other GStreamer developers? I can update this patch to match your suggestions in comment #15 if you want.
(In reply to comment #15) > I think GstBufferPool could make sense as a floating reference, we should > probably set the element that created it and is connected to it as parent of > the GstBufferPool. +1 > > GstCollectPads could have its element as parent too, so maybe makes sense to > keep it there too. not sure if it makes sense here (this is purely internal, right?) > > For the others it should be OK to remove the floating flag, but I'll let others > comment too +1 It would be nice to turn the test-programm into a test that runs during make check and verifies that the behaviour is in sync with the annotation ...
Created attachment 266583 [details] [review] Clear floating references for GstTask, GstTaskPool and GstCollectPads
Created attachment 266584 [details] [review] Fix documentation, gst_pad_new_from_*_template and gst_buffer_pool_new constructors return floating references
I updated the patches again to apply cleanly, and leave GstBufferPool returning a floating reference (with the corrected documentation).
Is there anything more that needs done with this? The documentation is currently wrong (and can cause memory leaks), so I'd like to get this finished if possible. If you don't like the patches I can change them however works for you, I just want the documentation to be consistent with what's actually happening.
commit f85c1c46488abf2814c8ce51a892a4f5f9ab8b98 Author: Brendan Long <b.long@cablelabs.com> Date: Wed Oct 30 17:02:35 2013 -0500 gst: clear floating references for GstTask, GstTaskPool and GstCollectPads https://bugzilla.gnome.org/show_bug.cgi?id=710342 commit f6054722309c3702fbe40ccbf009b8f2cb37e4e2 Author: Brendan Long <b.long@cablelabs.com> Date: Wed Oct 30 17:02:02 2013 -0500 docs: gst_pad_new_from_*_template and gst_buffer_pool_new constructors retur https://bugzilla.gnome.org/show_bug.cgi?id=710342