My fault, obviously gst_element_add_pad() has pad parameter that is marked as "transfer full", not "transfer floating".

Please see also bug 702969

"transfer none" would be wrong for these. It takes ownership of floating references, so semantically should be "transfer floating". Would that fix it for you?

I see that gtk_container_add() (which has the same semantics) has "transfer none" but that's semantically just wrong too.

(In reply to comment #3)
> "transfer none" would be wrong for these. It takes ownership of floating
> references, so semantically should be "transfer floating". Would that fix it
> for you?
> 
> I see that gtk_container_add() (which has the same semantics) has "transfer
> none" but that's semantically just wrong too.

Unfortunately GI has no concept of transfer floating that I know.
So, even if transfer none is not 100% correct, also other gtk functions behave like that other than container_add (see set_parent): they accept a transfer none object and ref_sink it.

There are a number of long standing bugs about GI and transfer floating: https://bugzilla.gnome.org/buglist.cgi?quicksearch=product%3Aglib+floating
We can workaround this on the Vala side quite easily if you aren't going to use transfer none.

The most useful bug is:

https://bugzilla.gnome.org/show_bug.cgi?id=657202

As far as (transfer floating)...the idea was simply that it's an alias for (transfer none).

Now, I think you guys are confusing the concept of "transfer for the mechanics of invoking this function" versus some conceptual "something in this callchain may keep a reference".

The (transfer) annotation refers to the first concept.  There is no annotation for the second.

Really your application shouldn't care if the callee takes a reference or not to do its job.

Not sure I understand what you mean, Colin. The function takes ownership of floating references, it does not take ownership of normal references. So the application really has to know that, otherwise there will be reference leaks or crashes.

(In reply to comment #5)
> Really your application shouldn't care if the callee takes a reference or not
> to do its job.

I've probably misunderstood this statement. Whether it's transfer full or transfer none is a huge difference for a parameter from the caller side.

Sorry, ignore comment #5 for now.

(In reply to comment #6)
> Not sure I understand what you mean, Colin. The function takes ownership of
> floating references, it does not take ownership of normal references. So the
> application really has to know that, otherwise there will be reference leaks or
> crashes.

There's two levels here:

1) The application
2) The binding machinery

The annotations are almost entirely for bindings to consume.  If I'm programming with gjs, I shouldn't have to care whether a function return value is (transfer full) or (transfer none).  It's just handled for me.

Now the annotations *are* nice for C programmers to read, and that's why I am glad they're in the docs.  They're a much more regular and reliable description than the previous English copy/paste text.

On this specific bug - it certainly is quite important for C programmers to know which functions will internally reference their arguments, and thus consume floating references.  But that concept is *not the same* as (transfer full).

Most input (transfer full) parameters are for convenience of C authors; like g_dbus_method_invocation_return_dbus_error (), where it frees the invocation for you.  But most bindings don't want application authors to be able to crash the process, so they generate an *extra reference* when invoking that function, and then clean up the object via normal means of garbage collection.

So it isn't right for this GStreamer API to use (transfer full) for "will sink floating refs", because that's just not what the annotation is for currently.

(In reply to comment #8)
> So it isn't right for this GStreamer API to use (transfer full) for "will sink
> floating refs", because that's just not what the annotation is for currently.

I think we all agree on that. So what's the right thing do do with current state of GObject Introspection ?  (transfer floating) and is exactly what it is, shall we use that ?

Same issue for

/**
 * gst_bin_add:
 * @bin: a #GstBin
 * @element: (transfer full): the #GstElement to add

btw. And it looks like we use '(transfer floating)' only for Returns: annotations (which isn't right either then really, is it? But bindings seem to currently ref_sink depending on whether the object is a GInitiallyUnowned and not on the transfer annotation)

g-i people, please agree on something and advise what to do, thanks.

Basically if you call g_object_ref_sink or g_object_ref and manage an object ref from within an APIs function, the annotation should be (transfer none). If the API is intended to steal the ref from the caller (probably rare), the API should be (transfer full).

I would limit "transfer floating" usage to return values from constructors which return GInitiallyUnowned based objects. For function arguments, you really should not care if the incoming object is actually floating or not, but rather you probably want to manage your own ref to the object rather than steal it from the caller. In which case you use g_object_ref_sink if objects are GInitiallyUnowned based or simply g_object_ref otherwise.

Besides floating refs being a confusing mess, I think the nomenclature is also part of the problem. IMO the terms the Python C API uses make more sense. For returns you have "new ref" and "borrowed ref" [1] which in GI can be mapped to "transfer full" and "transfer none" respectively. For arguments to a function, the callee can "steal" or add its own ref, which maps to "transfer full" and "transfer none" respectively.

Hope this helps!

[1] http://docs.python.org/2/c-api/tuple.html#PyTuple_GetItem

It doesn't really, ref_sink() "steals" a reference if the object was floating, and otherwise "adds its own ref". It's something in between both cases.

(In reply to comment #12)
> It doesn't really, ref_sink() "steals" a reference if the object was floating,
> and otherwise "adds its own ref". It's something in between both cases.

Agreed, but that can be relegated to an implementation detail only important to binding layers and the g_object_new function (which is always transfer full but may randomly return a floating object). Whether or not an argument to a function is floating, should be controlled by what the caller wants to do. If the caller wants to keep its own ref, it does its own ref_sink and the API function it is calling will add an additional ref, if it has a floating ref and wants to give it away, it passes it on. In either case, the callee should be guaranteed ownership of a reference without leaking when it uses g_object_ref_sink with the argument annotated as (transfer none). This is true at least in the Python and Gjs bindings, but I have no idea how vala deals with things from the perspective of the caller.

It's also important to C developers, and it would be nice to differentiate between these cases from a documentation annotation point of view at least.

Otherwise... a function that is (transfer none) and just calls g_object_ref() on the parameter will behave different than a function that has (transfer none) and calls g_object_ref_sink().


From a bindings point of view this distinction probably doesn't matter as bindings probably already do g_object_ref_sink() themselves right after they got the object if it was floating.

(In reply to comment #14)
> It's also important to C developers, and it would be nice to differentiate
> between these cases from a documentation annotation point of view at least.
> 
> Otherwise... a function that is (transfer none) and just calls g_object_ref()
> on the parameter will behave different than a function that has (transfer none)
> and calls g_object_ref_sink().

Ok, that makes sense and seems like it would be nice. The C API user should be provided with certainty as to what is going to occur with the reference. Without this, they have to make a best guess based on if the API uses GInitiallyUnowned, transfer full/none, usage of g_object_new, and possibly looking at the functions source code; which is unreasonable.

Although I'm not sure if using (transfer floating) to denote this is very clear either, at least makes some type of distinction. I would hope (transfer floating) could be reserved for functions which always return a floating ref. For arguments, you want specificity as to whether or not the function sinks it, not that the argument may or may not be floating. Although you could derive this meaning, it might be confusing when reading API docs and seeing it for an argument: "does this mean I need to call force_floating on the argument before passing it in?"

If what you are trying to achieve is API clarity, you don't necessarily need a GI annotation to do that. Looking back at the Python docs, each function could just say something like: "Note: This function will "sink" floating references for the element argument.", albeit that is pretty tedious.

> From a bindings point of view this distinction probably doesn't matter as
> bindings probably already do g_object_ref_sink() themselves right after they
> got the object if it was floating.

Yep, at least for functions with a return annotated as (transfer none) along with some special case hacks.

Interesting, I didn't know about enforce_floating(). How can this be used in any sensible way?


I think (transfer floating) for arguments is reasonably clear, there's no function anywhere that requires a floating reference to be passed AFAIK. I wouldn't know why such a thing would exist :)

Hey,

seems that this case got stuck.

Can I help in any way?

It still affects my projects.

Bug #657202 is blocking this currently.

commit 8fd03272c3dea767f757f73d2ff2c8dde70c0331
Author: Luca Bruno <lucabru@src.gnome.org>
Date:   Sun Mar 16 15:44:18 2014 +0100

    gstreamer-1.0: Make Element.add_pad parameter unowned
    
    Fixes bug 702960

We've fixed this in vala by simply making it transfer none, since vala always ref_sink InitiallyUnowned objects, just like gtk widgets and gtk_container_add.

Hey, thanks.

Isn't bug #702969 quite similar?

I mean, maybe this can be applied as a solution to leaks in Get.Bin#add and first param of Get.Bin#add_many?

(In reply to comment #20)
> Hey, thanks.
> 
> Isn't bug #702969 quite similar?
> 
> I mean, maybe this can be applied as a solution to leaks in Get.Bin#add and
> first param of Get.Bin#add_many?

I've overlooked add(), sorry:

commit 47868c96854fbdd146493b16ac8ab87f0f9cc8bd
Author: Luca Bruno <lucabru@src.gnome.org>
Date:   Sun Mar 16 16:28:04 2014 +0100

    gstreamer-1.0: Make Bin.add first parameter unowned

*** Bug 741390 has been marked as a duplicate of this bug. ***

Here is a workaround for PyGObject to get rid of the leaks introduced by this when using gst_bin_add() or gst_element_add_pad():

################################################

def monkeypatch_gst():

    def do_wrap(func):
        def wrap(self, obj):
            result = func(self, obj)
            obj.unref()
            return result
        return wrap

    parent = Gst.Bin()
    elm = Gst.Bin()
    parent.add(elm)
    if elm.__grefcount__ == 3:
        elm.unref()
        Gst.Bin.add = do_wrap(Gst.Bin.add)

    pad = Gst.Pad.new(None, Gst.PadDirection.SRC)
    parent.add_pad(pad)
    if pad.__grefcount__ == 3:
        pad.unref()
        Gst.Element.add_pad = do_wrap(Gst.Element.add_pad)

So what should we do about this?
Options:
- (transfer none), which would be semantically wrong and especially will give wrong information in the documentation for people using the C API
- (transfer floating), which would need to be added to GI for parameters first... and even then they seem to think that it's just an alias for (transfer none), which is simply not true unless you're only looking at bindings that behave like pygi (which never has floating references).

I just noticed that gi already understands transfer-floating as an alias for transfer-none in this case. So imho moving to transfer-floating should be a good first step, independent of what the final solution is going to be. It adds the needed documentation for C users and has the expected transfer type for current bindings.

Since you were worried (on IRC) about breaking bindings by moving to transfer-none. I'd assume all scripting bindings sink_ref atm, and vala seems to ref_sink as well, see this small example: https://bpaste.net/show/65bdcb07bfff

Affected arguments in gstreamer core I could find:

gst_object_set_parent.object
gst_element_add_pad.pad
gst_device_provider_device_add.device
gst_object_add_control_binding.binding
gst_bin_add.element
gst_registry_add_feature.feature

I'd also like to add that currently none of the above functions really are transfer-floating, as they don't sink the references in case of an error. Imo, if you really want proper semantics, this should be fixed as well. This would also need further changes as some functions depend on this error-case behavior.
 
* gst_ghost_pad_construct() unrefs a GstPad passed to gst_object_set_parent in its error path
* gstutils/ghost_up() unrefs a pad after a failed gst_element_add_pad
* gst_element_link_pads_filtered() unrefs a capsfilter after a failed gst_bin_add
* ...

(In reply to comment #25)
> I just noticed that gi already understands transfer-floating as an alias for
> transfer-none in this case. So imho moving to transfer-floating should be a
> good first step, independent of what the final solution is going to be. It adds
> the needed documentation for C users and has the expected transfer type for
> current bindings.

I thought (transfer floating) did only work for return values, not parameters. If it works for parameters we should definitely do that.

> Since you were worried (on IRC) about breaking bindings by moving to
> transfer-none. I'd assume all scripting bindings sink_ref atm, and vala seems
> to ref_sink as well, see this small example:
> https://bpaste.net/show/65bdcb07bfff

I assume all current bindings are doing this because otherwise they would run into problems because GI is not expressive enough, and instead of fixing this gtk_container_add() and friends became (transfer none).

> I'd also like to add that currently none of the above functions really are
> transfer-floating, as they don't sink the references in case of an error. Imo,
> if you really want proper semantics, this should be fixed as well. This would
> also need further changes as some functions depend on this error-case behavior.

Yes, those need to be fixed to have consistent behaviour on error cases too. There should be no difference in ownership transfer depending on error or not in any case.

Want to provide patches? :)

(In reply to comment #26)
> (In reply to comment #25)
> > I just noticed that gi already understands transfer-floating as an alias for
> > transfer-none in this case. So imho moving to transfer-floating should be a
> > good first step, independent of what the final solution is going to be. It adds
> > the needed documentation for C users and has the expected transfer type for
> > current bindings.
> 
> I thought (transfer floating) did only work for return values, not parameters.
> If it works for parameters we should definitely do that.

Seems this was unintentionally added in the patch for return values.
I've opened bug 742618 which adds a test to make sure it doesn't break.
Let's see how this works out..

> > Since you were worried (on IRC) about breaking bindings by moving to
> > transfer-none. I'd assume all scripting bindings sink_ref atm, and vala seems
> > to ref_sink as well, see this small example:
> > https://bpaste.net/show/65bdcb07bfff
> 
> I assume all current bindings are doing this because otherwise they would run
> into problems because GI is not expressive enough, and instead of fixing this
> gtk_container_add() and friends became (transfer none).
> 
> > I'd also like to add that currently none of the above functions really are
> > transfer-floating, as they don't sink the references in case of an error. Imo,
> > if you really want proper semantics, this should be fixed as well. This would
> > also need further changes as some functions depend on this error-case behavior.
> 
> Yes, those need to be fixed to have consistent behaviour on error cases too.
> There should be no difference in ownership transfer depending on error or not
> in any case.
> 
> Want to provide patches? :)

I can't promise anything.. maybe :)

The same problem affects gst_element_remove_pad(). Even though it effectively decreases the pad's refcount (due to gst_object_unparent), the pad argument is not passed according to (transfer full) semantics.

(In reply to Christoph Reiter (lazka) from comment #27)

> > > I'd also like to add that currently none of the above functions really are
> > > transfer-floating, as they don't sink the references in case of an error. Imo,
> > > if you really want proper semantics, this should be fixed as well. This would
> > > also need further changes as some functions depend on this error-case behavior.
> > 
> > Yes, those need to be fixed to have consistent behaviour on error cases too.
> > There should be no difference in ownership transfer depending on error or not
> > in any case.
> > 
> > Want to provide patches? :)
> 
> I can't promise anything.. maybe :)

This is handled in https://bugzilla.gnome.org/show_bug.cgi?id=747990 btw (a subset only so far, but should get them all covered really).

*** Bug 730138 has been marked as a duplicate of this bug. ***

Bug #730138 has a simple Python testcase for shoing this problem btw.

I'd suggest we just follow GTK here now (unlikely that GI changes anytime soon, and bindings already do what GTK does, and it is the right thing for bindings according to the GObject documentation).
That is, "transfer none" for gst_bin_add() / gst_element_add_pad() / etc., or rather "transfer floating" so that we have "transfer none" behaviour for bindings *and* signal in the documentation that it's not exactly "transfer none" for C developers.

Everything else causes memory leaks and worse.

Created attachment 351882 [details] [review]
gst: Correctly annotate functions taking floating reference parameters and returning floating references

Created attachment 351885 [details] [review]
allocators: Annotate constructors with (transfer floating)

GstAllocator is a GstObject and as such uses floating references.

Comment on attachment 351882 [details] [review]
gst: Correctly annotate functions taking floating reference parameters and returning floating references

Attachment 351882 [details] pushed as 30f871d - gst: Correctly annotate functions taking floating reference parameters and returning floating references

Attachment 351885 [details] pushed as 7185a7a - allocators: Annotate constructors with (transfer floating)