Is this the cause for bug #735529 then?

Yes this is a duplicate. 

Thibault has a fix for this bug:
http://paste.fedoraproject.org/136100/15701121/

*** This bug has been marked as a duplicate of bug 735529 ***

That patch is a workaround, the actual issue still exists in PyGobject.

For example that used to work before that commit:

from gi.repository import Gst

if __name__ == "__main__":
    Gst.init(None)

    caps = Gst.Caps.from_string("test")
    structure = caps.get_structure(0)
    structure.set_value("yes", 12)
    assert(caps.get_structure(0).get_value("yes") == 12)

And now fails.

This is rather unfortunate. The return value of gst_caps_get_structure() is annotated as transfer-none which means we should be making a copy of the result. Otherwise the memory is unsafe for Python to hold onto. We can back some of the changes out to get this working again but this API puts GI bindings in a tough position.

(In reply to comment #4)
> This is rather unfortunate. The return value of gst_caps_get_structure() is
> annotated as transfer-none which means we should be making a copy of the
> result. Otherwise the memory is unsafe for Python to hold onto. We can back
> some of the changes out to get this working again but this API puts GI bindings
> in a tough position.

Right, the documentation explicitly states that the GstStructure returned can be modified only in case you know that the caps are writable:

> WARNING: This function takes a const GstCaps *, but returns a non-const 
> GstStructure *. This is for programming convenience -- the caller should be 
> aware that structures inside a constant GstCaps should not be modified. However,
> if you know the caps are writable, either because you have just copied them or 
> made them writable with gst_caps_make_writable(), you may modify the structure 
> returned in the usual way, e.g. with functions like gst_structure_set().

I think that in the general case what PyGI does is correct, but we should figure out a solution, we can maybe do that in gst-python. Do you have ideas?

(In reply to comment #5)
> I think that in the general case what PyGI does is correct, but we should
> figure out a solution, we can maybe do that in gst-python. Do you have ideas?

A long term solution might be to add something like boost::python call policies to GI [1]. This allows more specific lifetime management of arguments and return values (essentially adding dependency relationships). But that's a serious feature.

Short term I guess there are a few solutions:

1) Backing out some of the PyGI changes.

2) Add a new method to GstCaps (or patch it in Gst Python) which allows setting values on specific structure indices:

    caps = Gst.Caps.from_string("test")
    caps.set_structure_value(0, "yes", 12)

3) I don't know the full extent of how this API is being used but you could manually build up the caps by first setting values on structures and then appending them as a workaround:

    structure = Gst.Structure.new_from_string('test')
    structure.set_value("yes", 12)
    structure.get_value("yes")  # 12
    caps = Gst.Caps.new_empty()
    caps.append_structure(structure)
    caps.get_structure(0).get_value('yes')  # 12

I'm fine with #1 since software is breaking (even though it depends on buggy PyGI behavior), but something like #2 or #3 would be best because it means PyGI maintains safety and I don't have to do any work :)

Also note that the example from comment #3 fails in Gjs as well, so relying on the memory mis-management that was in PyGI is probably also not in the best interest of GStreamer APIs.

[1] http://www.boost.org/doc/libs/1_56_0/libs/python/doc/tutorial/doc/html/python/functions.html#call_policies.call_policies

Created attachment 287503 [details] [review]
structure: Refcount GstStructure, and use it for the boxed type funcs

This enable bindings to return the same structure out of caps for
example and avoid copying structure around when used as a boxed type.

Moving it to GStreamer core see if that solution would be acceptable there.

Created attachment 287504 [details] [review]
structure: Refcount GstStructure, and use it for the boxed type funcs

This enable bindings to return the same structure out of caps for
example and avoid copying structure around when used as a boxed type.

I think making the boxed copy function return a ref just causes writability issues then, no?

Feels like we're re-inventing the "exclusive" locking from GstMiniObject (as used when putting GstMemories into a GstBuffer), but I can't see an easy way to retrofit that onto GstStructure

(In reply to comment #10)
> I think making the boxed copy function return a ref just causes writability
> issues then, no?

It can potentially, we should figure out how we want that to be handled. Can we / do we usually write into GstStructure coming from GValues ?

The other question is where is the Boxed type API used with GstStructure and if people rely (and should rely in the current context) on the fact that structures are hard copied when using it?

8517504 in pygobject also causes the refcount in Gst.Sample to be incremented making it impossible to modify the buffer contained within the Gst.Sample.  This is making stb-tester unable to modify GstBuffers contained within GstSamples[1] provided from appsink, even if I copy the buffers into a fresh GstSample[2].

[1]: https://github.com/drothlis/stb-tester/blob/master/stbt/__init__.py#L1551
[2]: https://github.com/drothlis/stb-tester/blob/master/stbt/__init__.py#L1495

Example demonstrating old-behaviour with pygobject@8517504^:

    In [1]: from gi.repository import Gst

    In [2]: Gst.init()
    Out[2]: []

    In [3]: s = Gst.Sample.new(Gst.Buffer.new_wrapped("Hello"))

    In [4]: s.get_buffer().mini_object.is_writable()
    Out[4]: True

    In [5]: buf = s.get_buffer()

    In [6]: buf.mini_object.is_writable()
    Out[6]: True

Example demonstrating new behaviour with pygobject@8517504:

    In [1]: from gi.repository import Gst

    In [2]: Gst.init()
    Out[2]: []

    In [3]: s = Gst.Sample.new(Gst.Buffer.new_wrapped("Hello"))

    In [4]: s.get_buffer().mini_object.is_writable()
    Out[4]: True

    In [5]: buf = s.get_buffer()

    In [6]: buf.mini_object.is_writable()
    Out[6]: False

Created attachment 289060 [details] [review]
Override gst_sample_get_buffer to avoid reffing the buffer

And make it possible to modify the buffer in place.

+ Add some simple tests

Created attachment 289062 [details] [review]
Override gst_caps_get_structure to avoid the structure to be copied

when it should not as we need to be able to modify it in place.

And I added tests in the corresponding branch (master on my repo):  http://cgit.collabora.com/git/user/tsaunier/gst-python/

I don't think this is a regression at the GStreamer C API level or a blocker?

I don't believe that this is a C API regression.

It does block us from using pygobject 3.13 or later for stb-tester.

I guess we should find a solution in GstPython. @Will, could you check my patch fixes it for you?

@Thibault I have tested your gst-python patches http://cgit.collabora.com/git/user/tsaunier/gst-python/ under Fedora 21 and it fixes the issue Will is describing in #13.

Can this patch get into the next version of gst-python?

With your patch:

    >>> from gi.repository import Gst
    >>> Gst.init()
    []
    >>> s = Gst.Sample.new(Gst.Buffer.new_wrapped("Hello"))
    >>> s.get_buffer().mini_object.is_writable()
    True
    >>> buf = s.get_buffer()
    >>> buf.mini_object.is_writable()
    True

(In reply to John Slade from comment #20)
> @Thibault I have tested your gst-python patches
> http://cgit.collabora.com/git/user/tsaunier/gst-python/ under Fedora 21 and
> it fixes the issue Will is describing in #13.
> 
> Can this patch get into the next version of gst-python?
> 
> With your patch:
> 
>     >>> from gi.repository import Gst
>     >>> Gst.init()
>     []
>     >>> s = Gst.Sample.new(Gst.Buffer.new_wrapped("Hello"))
>     >>> s.get_buffer().mini_object.is_writable()
>     True
>     >>> buf = s.get_buffer()
>     >>> buf.mini_object.is_writable()
>     True

I actually tested it a bit and it breaks things, it is too much of a hack and I do not think we can merge that.

I have added similare hacks like http://cgit.freedesktop.org/gstreamer/gst-python/commit/?id=3d19875eb7c20cedb4ab64e295c338ebd32f4b04 already, in that case it is good enough and will not break things, but this one does not look safe.

(In reply to Thibault Saunier from comment #21)
> I actually tested it a bit and it breaks things, it is too much of a hack
> and I do not think we can merge that.
> 
> I have added similare hacks like
> http://cgit.freedesktop.org/gstreamer/gst-python/commit/
> ?id=3d19875eb7c20cedb4ab64e295c338ebd32f4b04 already, in that case it is
> good enough and will not break things, but this one does not look safe.

I have tested 3d19875eb7c20cedb4ab64e295c338ebd32f4b04 and it doesn't fix the problem of the sample being un-writable.

What do your patches in http://cgit.collabora.com/git/user/tsaunier/gst-python/ break?  I have been using them for a while and haven't found any problems.

Also could you explain why you are calling these "hacks".  What is the problem being hacked around - something in PyGObject or in gstreamer?

Do you think it is possible these issues can be reliably fixed in gst-python?  Or will it require the long term solution suggested in https://bugzilla.gnome.org/show_bug.cgi?id=736896#c6

I think the fundamental problem is that GstMiniObject claims that:

     * Ref Func: gst_mini_object_ref
     * Unref Func: gst_mini_object_unref

But it's using a different definition of "ref" than Python uses.  Notably: calling this function can actually mutate the object being referenced (e.g. make it non-writable).

One option might be to add an override wrapping all GstMiniObjects with its own reference count that actually conforms to Python's reference counting expectations.  This could have a custom deleter, so then the Sample.get_buffer override could look something like:

    GstMiniObjectWrapper * gst_sample_override_gst_get_buffer(GstSample* sample)
    {
        return gst_mini_object_wrapper_new(
            gst_sample_get_buffer(gst_sample_ref(sample)),
            &gst_sample_unref,
            sample);
    }

Of course this naive approach would require overriding every place that a GstMiniObject is returned by GStreamer, but maybe pygi has a way around this?

My two cents. While this is being called a regression in the title, returning a copy of a boxed type with (transfer none) is a safe thing to do. Before that, it was really easy to crash the interpretor. Though, pygobject could rather put a "ref" to the original object to actually bind the lifetime (doing this at the Python reference level rather then touching the object references). Effectively creating a cirular reference though, which will hopefully be fixed by the GC.

> While this is being called a regression in the title

It is a regression because there were perfectly reasonable (and correct) programs that worked previously that no longer work.

I think everyone can agree that allowing the API to crash the interpreter is *bad*, but that doesn't stop this from being a regression.

I like your idea of using Python's own reference counting to solve this, assuming it works.  Given the circular references I would have some concerns about references not being collected when you expect them to be (e.g. they will be collected when the GC runs, not when the variables go out of scope).  This could have user-visible impact becuase gst_mini_object_unref mutates the GstMiniObject.

(In reply to John Slade from comment #22)
> I have tested 3d19875eb7c20cedb4ab64e295c338ebd32f4b04 and it doesn't fix
> the problem of the sample being un-writable.

That was expected :)
 
> What do your patches in
> http://cgit.collabora.com/git/user/tsaunier/gst-python/ break?  I have been
> using them for a while and haven't found any problems.

It breaks rendering in Pitivi, I did not investigate further why tbh, from what I can tell, PyGObject thinks it owns the boxed structure but it actually doesn't with that patch.

(In reply to Nicolas Dufresne (stormer) from comment #24)
> My two cents. While this is being called a regression in the title,
> returning a copy of a boxed type with (transfer none) is a safe thing to do.
> Before that, it was really easy to crash the interpretor. Though, pygobject
> could rather put a "ref" to the original object to actually bind the
> lifetime (doing this at the Python reference level rather then touching the
> object references). Effectively creating a cirular reference though, which
> will hopefully be fixed by the GC.

Where and how do you imagine that to be done ?

(In reply to Will Manley from comment #25)
> > While this is being called a regression in the title
> 
> It is a regression because there were perfectly reasonable (and correct)
> programs that worked previously that no longer work.

I agree, this is a regression, it used to work but does not anymore.

As it is right now we can crash the interpretor too easily. Crashing the interpreter is a big fault for a binding. I have strong opinion about never letting this happen:

  buf = get_buf()
  buf.mini_object.unref()
  buf.anything -> possible crash

For this reason, ref/unref() method will be removed in 1.6 from GI even if this is an API break. If you think this must come back, box the GstMiniObject, then you can un-skip (normally the ref/unref annotation should cause ref/uner to be hidden to users anyway, but you need a boxed type to add this annotation). It make no sense to expose this. The binding must handle that, internally or live without. I am definitive on that.

In the past, GI was simply doing nothing with (transfer-none) returned object. So you could have:

  buf = object.get_buf() #transfer none
  object = somethingelse;
  buf.blabla # random crash

Where I think we have side effects, is that GST has this incompatible writability based on ref-counting. This is not how ref-counting was suppose to work in the GI design. GI idea to solve this wasn't bad, it's the simplest and the most correct way to solve the previous crash.

  buf = object.get_buf() #transfer none
  # here, if the the boxed type has a reference, take a ref, otherwise call
  # the copy function. Anything not boxed must from now on be skipped or boxed.

And then it's all safe in the interpreter, but breaks with GStreamer writability scheme. Causing the regression mention here (and making anything in the GstMiniObject world that need writability to work to not work anymore). IMHO, this ref bound writability was a bad idea, makes everything complex. Explicit locking (like for GstMemory) would have done the job. But we can't change it. If we want to support that, without overrides, we'll need to fix it at the interpreter level. Imho, there can be some gain at not copying boxed types received using (transfer none). It would enable more API, as in C, the side effect of modifying a (transfer none) is to be expected. Now, the binding level fix is quite simple for python. This is just pseudo solution, as we will want to protect the private member more then what python interpreter do.

  buf = object.get_buf() # transfer none
    # Internally
    buf.__owner_object = object
  # The rest is safe, when we leave the scope/context (assuming we know
  # in the bindings)
    buf.__owner_object = none;

*IF* we can know that buf go out of scope (or context go away / last ref) then we can manually break the circular dependency and prevent having to wait for the GC. Though in fact, pure GC base languages (unlike python) don't offer you this. For that, the automatic binding would need to add explicit destroy() or dispose() (whatever convention is recommended for native types).

Mathieu Duponchelle has worked last cycle to make pygobject only ref objects
in callbacks after the callback returns. So, some API like this might be
possible now:

def buf_func(buf):
    # no ref taken
    buf.change()
    # when buf_func() returns, buf gets reffed, and ideally unreffed because
    # the wrapper dies shortly after

object.for_buf(buf_func)

Regarding the owner object:

If we'd have some notion of an "owner object" that might work, but something
like that doesn't exist in GI afaik.

You could emulate it by having a proxy buffer object which takes a ref on the
owner and has methods which allow modifying the buffer.

proxy = object.get_proxy_buf()
proxy.change() # proxy has a ref on object internally and
               # has just a pointer to the real buffer
buf = proxy.get_buf()

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gstreamer/issues/76.