Bug 140119 - [GStreamer][MSE] ASSERT in MediaSourceClientGStreamer::addSourceBuffer
Summary: [GStreamer][MSE] ASSERT in MediaSourceClientGStreamer::addSourceBuffer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-01-06 02:25 PST by Philippe Normand
Modified: 2015-01-09 05:34 PST (History)
3 users (show)

See Also:


Attachments
patch (2.59 KB, patch)
2015-01-06 03:00 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
patch (2.37 KB, patch)
2015-01-06 05:18 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
patch (2.40 KB, patch)
2015-01-06 05:21 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
patch (2.44 KB, patch)
2015-01-08 09:38 PST, Philippe Normand
cgarcia: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2015-01-06 02:25:34 PST
(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
Comment 1 Philippe Normand 2015-01-06 02:26:40 PST
I suppose we could store the ghost pad in the Source struct and also use webkitGstGhostPadFromStaticTemplate(). Will upload a patch :)
Comment 2 Philippe Normand 2015-01-06 03:00:59 PST
Created attachment 244048 [details]
patch
Comment 3 Sebastian Dröge (slomo) 2015-01-06 03:53:25 PST
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
Comment 4 Philippe Normand 2015-01-06 03:57:00 PST
(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?
Comment 5 Sebastian Dröge (slomo) 2015-01-06 03:59:36 PST
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).
Comment 6 Philippe Normand 2015-01-06 05:18:58 PST
Created attachment 244051 [details]
patch
Comment 7 Philippe Normand 2015-01-06 05:21:23 PST
Created attachment 244052 [details]
patch
Comment 8 Carlos Garcia Campos 2015-01-08 02:22:27 PST
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.
Comment 9 Philippe Normand 2015-01-08 09:38:28 PST
Created attachment 244266 [details]
patch
Comment 10 Carlos Garcia Campos 2015-01-09 02:51:28 PST
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 :-)
Comment 11 Philippe Normand 2015-01-09 05:34:12 PST
Committed r178169: <http://trac.webkit.org/changeset/178169>