[GStreamer][1.18] mediastreamsrc element hits assert in gst -core

• Component: Platform
• Status: RESOLVED FIXED

[Philippe Normand]

Because of these commits in -core:
https://gitlab.freedesktop.org/gstreamer/gstreamer/commit/a90220cce1f4bbe9380c46b9f5458b90b0178a7c
https://gitlab.freedesktop.org/gstreamer/gstreamer/commit/e79def14a5c47ef7c3660e17fd183d25dc82ae3f

(WebKitWebProcess:64): GStreamer-CRITICAL **: 10:34:54.644: The created element should be floating, this is probably caused by faulty bindings

Thanks to GRefPtrs passed to WebKitMediaStreamTrackObserver and WebKitMediaStreamObserver...

[Philippe Normand]

Created attachment 403892 [details]
Patch

[Víctor M. Jáquez L.]

informal r+

[Xabier Rodríguez Calvar]

Comment on attachment 403892 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403892&action=review

> Source/WebCore/ChangeLog:9
> +        Switch stream and track observers back to refcounted raw GstElement pointers. The magic of
> +        GRefPtr is doing more harm than good here.

You state you have a problem but you don't say which one. I guess you have two extra refs, right? Your problem is that you're explicitly reffing the object when you're implicitly creating the observers. GRefPtr constructor (which is implicitly called) already ref_sinks the object you're passing, hence, you either ref and adopt or you don't ref and let the implicit GRefPtr constructor do the ref for you.

[Philippe Normand]

Comment on attachment 403892 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403892&action=review

>> Source/WebCore/ChangeLog:9
>> +        GRefPtr is doing more harm than good here.
> 
> You state you have a problem but you don't say which one. I guess you have two extra refs, right? Your problem is that you're explicitly reffing the object when you're implicitly creating the observers. GRefPtr constructor (which is implicitly called) already ref_sinks the object you're passing, hence, you either ref and adopt or you don't ref and let the implicit GRefPtr constructor do the ref for you.

Of course I tried that already, and it doesn't work. I still get the critical warning, for which the backtrace is not very useful anyway. The problem is that we end up calling gst_object_ref_sink() from `template <> GstElement* refGPtr<GstElement>(GstElement* ptr)` called from the src element constructor. I don't think we can use a GRefPtr like this, here. If you disagree, I'm happy to let you take ownership of this bug.

[Xabier Rodríguez Calvar]

Comment on attachment 403892 [details]
Patch

For what I studied the issue the whole concept of the observers keeping a referenced to the element is a bad idea because the finalize method will never be called. The moment we create the media stream src the are reffing ourselves in the Constructed method twice and keeping an returning an object that was refcount 3 (without taking into account other leaks involving GRefPtr). This means that when you return create an object and unref it just afterwards, it won't be destroyed (unless I am missing anything). I did a lousy review at the original bug and didn't realize about this. This needs to be addressed here. I'm sorry for that.

Then we need to consider that with the unpatched code, we're implicitly calling the GRefPtr constructor.

Besides, current code with the smart pointers is adding a another extra reference as the ref is bumped again the second time the GRefPtr constructor is run but not the first. The first is ref_sinking the reference, not increasing it, but the second does, so with the current unpatched code we are returning an object with ref count 4.

As I said the first implicit GRefPtr constructor call is ref_sinking the reference so in the end of the Constructed method, the element has a sinked referenced that is causing the warning. The problem is that this warning is just the tip of the iceberg.

For me, the best solution of this problem would be using weak refs to keep the element in the observer, but there is no smart pointer for that and it would just be an aesthetic improvement over the solution I'll just propose.

Considering that the life of the observers is 1-1 with the life of the source, we can just keep a plain GstElement* inside the observer and forget the smart ptr and gobject reffing at all. In the Constructed method, we pass self down and we keep a GstElement*.

Another thing is that from what I see of how the observers are being passed to the callers, they are always passed as reference (l-value), not as a pointer, so I think we we could even keep the objects in the struct. The problem of keeping the objects in the struct is that you have the chicken/egg problem when constructing so either you add a method to the observer to set the GstElement* calling it in the Constructed method or you leave it as it is.

Independently of leaving the observers as unique_ptrs or not, there is another problem with this object and it is that I don't think the destructors of be objects inside the struct are being called, since this is allocated with C, right? I think this needs to be checked as well and ensure the destructor is called as it happens for example in the case of the decryptors.

I think we opened quite a can of worms not related to your patch here!

[Xabier Rodríguez Calvar]

Created attachment 404797 [details]
Patch

[EWS]

Committed r264648: <https://trac.webkit.org/changeset/264648>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 404797 [details].

[Radar WebKit Bug Importer]

<rdar://problem/65872930>