Bug 140119 – [GStreamer][MSE] ASSERT in MediaSourceClientGStreamer::addSourceBuffer

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Philippe Normand]

(gdb) bt
#0  0x00007fffed2950a8 in WTFCrash () at ../../Source/WTF/wtf/Assertions.cpp:321
#1  0x00007ffff3ed2183 in WTF::adoptGRef<_GstPad> (ptr=0xb44f40) at ../../Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp:50
#2  0x00007ffff3ef0029 in WebCore::MediaSourceClientGStreamer::addSourceBuffer (this=0x7fff52839180, sourceBufferPrivate=..., contentType=...)
    at ../../Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:346
#3  0x00007ffff3ee8ba6 in WebCore::MediaSourceGStreamer::addSourceBuffer (this=0x7fff5281fcb0, contentType=..., sourceBufferPrivate=...)
    at ../../Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.cpp:68
#4  0x00007ffff2c1ef98 in WebCore::MediaSource::createSourceBufferPrivate (this=0x7fff52c5dc60, type=..., ec=@0x7fffffffc39c: 0) at ../../Source/WebCore/Modules/mediasource/MediaSource.cpp:850
#5  0x00007ffff2c1ddf4 in WebCore::MediaSource::addSourceBuffer (this=0x7fff52c5dc60, type="audio/mp4; codecs=\"mp4a.40.2\"", ec=@0x7fffffffc39c: 0)
    at ../../Source/WebCore/Modules/mediasource/MediaSource.cpp:541
#6  0x00007ffff3fcf927 in WebCore::jsMediaSourcePrototypeFunctionAddSourceBuffer (exec=0x7fffffffc420) at DerivedSources/WebCore/JSMediaSource.cpp:304
#7  0x00007fff9b2e50b4 in ?? ()
#8  0x00007fffffffc490 in ?? ()
#9  0x00007fffed23d3a7 in llint_entry () from /home/phil/WebKit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18
Backtrace stopped: frame did not save the PC

[Philippe Normand]

I suppose we could store the ghost pad in the Source struct and also use webkitGstGhostPadFromStaticTemplate(). Will upload a patch :)

[Philippe Normand]

Created attachment 244048 [details]
patch

[Sebastian Dröge (slomo)]

Comment on attachment 244048 [details]
patch

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

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:-340
> -    GRefPtr<GstPad> ghostPad = adoptGRef(gst_ghost_pad_new_from_template(padName.get(), pad.get(),

Or alternatively just store it here in a GstPad* instead of the fancy smart pointers

[Philippe Normand]

(In reply to comment #3)
> Comment on attachment 244048 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=244048&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:-340
> > -    GRefPtr<GstPad> ghostPad = adoptGRef(gst_ghost_pad_new_from_template(padName.get(), pad.get(),
> 
> Or alternatively just store it here in a GstPad* instead of the fancy smart
> pointers

Wouldn't that trigger issues when going out of scope?

[Sebastian Dröge (slomo)]

Not if it's just a normal C pointer. The one and only reference to the pad would be taken by gst_element_add_pad() and then the variable goes out of scope and nothing happens (while currently the smart pointer unrefs the pad when it goes out of scope, leading to a refcount of 0).

[Philippe Normand]

Created attachment 244051 [details]
patch

[Philippe Normand]

Created attachment 244052 [details]
patch

[Carlos Garcia Campos]

Comment on attachment 244052 [details]
patch

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

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:-341
> -    GRefPtr<GstPad> ghostPad = adoptGRef(gst_ghost_pad_new_from_template(padName.get(), pad.get(),
> -        gst_static_pad_template_get(&srcTemplate)));

I think the actual bug here was that you were adopting a floating reference.

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:340
> +    GstPad* ghostPad = webkitGstGhostPadFromStaticTemplate(&srcTemplate, padName.get(), pad.get());

What's the difference between gst_ghost_pad_new_from_template and webkitGstGhostPadFromStaticTemplate? is this change unrelated to the fix?

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:343
> +    gst_element_add_pad(GST_ELEMENT(m_src.get()), ghostPad);

This consumes the floating reference, so you could either use a raw pointer or keep the smart pointer but without adopting the floating ref. the good thing of using a smart pointer is that if at some point and early return is added between the gst_ghost_pad_new_from_template and the gst_element_add_pad the GstPad will not be leaked.

[Philippe Normand]

Created attachment 244266 [details]
patch

[Carlos Garcia Campos]

Comment on attachment 244266 [details]
patch

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

> Source/WebCore/platform/graphics/gstreamer/WebKitMediaSourceGStreamer.cpp:-344
> -    gst_element_add_pad(GST_ELEMENT(m_src.get()), ghostPad.leakRef());

Oh, I see why this worked, because of the leakRef :-)

[Philippe Normand]

Committed r178169: <http://trac.webkit.org/changeset/178169>