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 710342 - docs: various constructors are annotated as "transfer full", but return floating references
docs: various constructors are annotated as "transfer full", but return float...
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal minor
: 1.3.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-10-16 23:56 UTC by Brendan Long
Modified: 2014-02-06 21:51 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Change documentation to say that gst_task_new returns a floating reference (702 bytes, patch)
2013-10-17 00:00 UTC, Brendan Long
none Details | Review
Change documentation for GstObject derived classes' new functions returning floating references (5.38 KB, patch)
2013-10-17 00:27 UTC, Brendan Long
none Details | Review
Program which runs various constructors and checks if the result is_floating (2.27 KB, text/x-csrc)
2013-10-17 17:34 UTC, Brendan Long
  Details
Fix documentation for constructors that return floating references (2.26 KB, patch)
2013-10-17 17:37 UTC, Brendan Long
none Details | Review
Program which runs various constructors and checks if the result is_floating (3.11 KB, text/x-csrc)
2013-10-17 18:37 UTC, Brendan Long
  Details
Patch to fix documentation for constructors that return floating references (3.26 KB, patch)
2013-10-17 18:39 UTC, Brendan Long
needs-work Details | Review
Program which runs various constructors and checks if the result is_floating (1.30 KB, text/x-csrc)
2013-10-17 19:18 UTC, Brendan Long
  Details
Document floating reference for gst_pad_new_from_*_template (1.26 KB, patch)
2013-10-30 22:04 UTC, Brendan Long
none Details | Review
Clear floating reference in various contructors (1.95 KB, patch)
2013-10-30 22:05 UTC, Brendan Long
none Details | Review
Clear floating references for GstTask, GstTaskPool and GstCollectPads (1.54 KB, patch)
2014-01-17 22:26 UTC, Brendan Long
committed Details | Review
Fix documentation, gst_pad_new_from_*_template and gst_buffer_pool_new constructors return floating references (1.74 KB, patch)
2014-01-17 22:27 UTC, Brendan Long
committed Details | Review

Description Brendan Long 2013-10-16 23:56:36 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?
Comment 1 Brendan Long 2013-10-17 00:00:11 UTC
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.
Comment 2 Brendan Long 2013-10-17 00:02:01 UTC
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;
}
Comment 3 Brendan Long 2013-10-17 00:27:57 UTC
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.
Comment 4 Brendan Long 2013-10-17 17:04:40 UTC
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);
}
Comment 5 Brendan Long 2013-10-17 17:33:06 UTC
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
Comment 6 Brendan Long 2013-10-17 17:34:44 UTC
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
Comment 7 Brendan Long 2013-10-17 17:37:45 UTC
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.
Comment 8 Brendan Long 2013-10-17 18:37:15 UTC
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
Comment 9 Brendan Long 2013-10-17 18:39:22 UTC
Created attachment 257584 [details] [review]
Patch to fix documentation for constructors that return floating references

Here's an update to the patch, including GstPad.
Comment 10 Brendan Long 2013-10-17 19:18:16 UTC
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.
Comment 11 Sebastian Dröge (slomo) 2013-10-30 21:38:16 UTC
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
Comment 12 Brendan Long 2013-10-30 21:57:20 UTC
Should GstBufferPool be floating?
Comment 13 Brendan Long 2013-10-30 22:04:31 UTC
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.
Comment 14 Brendan Long 2013-10-30 22:05:47 UTC
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.
Comment 15 Sebastian Dröge (slomo) 2013-10-31 13:48:03 UTC
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
Comment 16 Sebastian Dröge (slomo) 2014-01-08 13:46:06 UTC
What should we do about this?
Comment 17 Brendan Long 2014-01-08 16:39:53 UTC
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.
Comment 18 Stefan Sauer (gstreamer, gtkdoc dev) 2014-01-08 21:57:58 UTC
(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 ...
Comment 19 Brendan Long 2014-01-17 22:26:55 UTC
Created attachment 266583 [details] [review]
Clear floating references for GstTask, GstTaskPool and GstCollectPads
Comment 20 Brendan Long 2014-01-17 22:27:39 UTC
Created attachment 266584 [details] [review]
Fix documentation, gst_pad_new_from_*_template and gst_buffer_pool_new constructors return floating references
Comment 21 Brendan Long 2014-01-17 22:28:44 UTC
I updated the patches again to apply cleanly, and leave GstBufferPool returning a floating reference (with the corrected documentation).
Comment 22 Brendan Long 2014-02-04 23:43:33 UTC
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.
Comment 23 Sebastian Dröge (slomo) 2014-02-06 21:51:21 UTC
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