Fix ownership of returned Opaque with GetOpaque

[ylatuya]

GetOpaque is returning Opaque objects with owner set to the opposite value as the one requested.
It seems like the existing fix was incorrectly applied to the wrong if branch.

This behaviour can be seen when overriding virtual methods like BaseSink.OnQuery that have as parameters Gst.MiniObject's and where GetOpaque is called with owned=false.

The new Query object is owned and with an extra Ref, making it isWritable=false, while it should be !owned and with refcount of 1 to make it IsWritable=false

[ylatuya]

The fix does not seams to be fully working. It needs some more work.

[ylatuya]

After checking, the fix works as expected. It actually raised an issue in the GLOverlayCompositor.AddCaps binding, caused by a lack of annotations of the gst_overlay_compositor_add_caps API.

[ylatuya]

@thiblahute can you take a look at this one?

[ylatuya]

@thiblahute is there a change you can look at this PR? Thanks

[ylatuya]

@sdroege Can you please review this?

[sdroege]

The new Query object is owned and with an extra Ref, making it isWritable=false, while it should be !owned and with refcount of 1 to make it IsWritable=false

How does this fix prevent the unowned wrapper object to stay around longer than the C GstQuery*?

[ylatuya]

The new Query object is owned and with an extra Ref, making it isWritable=false, while it should be !owned and with refcount of 1 to make it IsWritable=false

How does this fix prevent the unowned wrapper object to stay around longer than the C GstQuery*?

I think I now understand a bit more the different problems around the Opaque implementation.

"unowned" Opaque objects created with GetOpaque are currently copied to ensure that the wrapper object stays around longer than the C pointer. It's definitively a problem for uses cases like the OnQuery overrides were you need to modify the real GstQuery* and not a copy of it. It's working right now because there is a bug in the Copy that does not do any copy (I will expand in this later). Omitting this bug, this is the status:

• ✅ Expected behaviour: An owned copy of the object is created with a refcount of 1
• ❌ Current implementation: An owned copy of the object is created with a refcount of 2
• ❌ PR patch: An unowned copy of the object is created with a refcount of 1

The patch in this PR works because no real copy is being done, but it's not memory safe.

I would propose to handle the creation of unowned Opaque in different ways:

1. Non refcounted (GDate): the only memory-safe solution is creating a copy.
2. Refcounted (GDateTime): instead of creating a copy, take a ref to keep the native pointer alive while the Opaque is alive.
3. Weakly refcounted (GstMiniObject): adding an extra ref in GstMiniObject makes them non-writable. Instead a week reference can be added to the GstMiniObject and the Opaque object is Disposed once the weak reference is lost and we are notified that the GstMiniObject was destroyed. In addition, a weaver like https://github.com/Fody/Janitor can be used to throw Exceptions when a an Opaque object is being used after being disposed.

Back to the existing bugs, there are several issues that needs to be fixed:

1. Copy functions are ignored 70f976e. So no actual copies are done when a copy is required.
2. All instances of Opaque subclases created with the contructors Opaque (IntPtr ptr) are leaking a Ref. Raw setter is taking a Ref, in Dispose() Raw setter will remove that Ref and also set Handle to IntPtr.Zero, Dispose will not release the last ref since Handle is IntPtr.zero.
3. Opaques that do not implement Copy are silently copied in a non memory-safe way. There should be a configurable warning about this.

@sdroege based on your experience with bindings, do you think the proposal makes sense? If so I will proceed to do the changes and write unit test for it.