Bug 140116 – [GStreamer][MSE] ASSERT in MediaSourceGStreamer::addSourceBuffer

• Status: RESOLVED
• Resolution: LATER
• Component: Platform

[Philippe Normand]

ASSERTION FAILED: !m_adoptionIsRequired
0x00007fffed2980a8 in WTFCrash () at ../../Source/WTF/wtf/Assertions.cpp:321
321         *(int *)(uintptr_t)0xbbadbeef = 0;
(gdb) bt
#0  0x00007fffed2980a8 in WTFCrash () at ../../Source/WTF/wtf/Assertions.cpp:321
#1  0x00007ffff272740e in WTF::RefCountedBase::ref (this=0x7fff52867e78) at ../../Source/WTF/wtf/RefCounted.h:45
#2  0x00007ffff3eeabdb in WTF::refIfNotNull<WebCore::SourceBufferPrivateGStreamer> (ptr=0x7fff52867e70) at ../../Source/WTF/wtf/PassRefPtr.h:36
#3  0x00007ffff3eea93f in WTF::RefPtr<WebCore::SourceBufferPrivateGStreamer>::RefPtr (this=0x7fffffffc1e0, ptr=0x7fff52867e70) at ../../Source/WTF/wtf/RefPtr.h:43
#4  0x00007ffff3eea235 in WebCore::MediaSourceGStreamer::addSourceBuffer (this=0x7fff5283cc80, contentType=..., sourceBufferPrivate=...)
    at ../../Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.cpp:66
#5  0x00007ffff2c206d8 in WebCore::MediaSource::createSourceBufferPrivate (this=0x7fff527c4c60, type=..., ec=@0x7fffffffc39c: 0) at ../../Source/WebCore/Modules/mediasource/MediaSource.cpp:850
#6  0x00007ffff2c1f534 in WebCore::MediaSource::addSourceBuffer (this=0x7fff527c4c60, type="audio/mp4; codecs=\"mp4a.40.2\"", ec=@0x7fffffffc39c: 0)
    at ../../Source/WebCore/Modules/mediasource/MediaSource.cpp:541
#7  0x00007ffff3fd07e7 in WebCore::jsMediaSourcePrototypeFunctionAddSourceBuffer (exec=0x7fffffffc420) at DerivedSources/WebCore/JSMediaSource.cpp:304
#8  0x00007fff9b2e80b4 in ?? ()
#9  0x00007fffffffc490 in ?? ()
#10 0x00007fffed2403a7 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]

Sebastian fixed in his patch for Bug 140078, I'll upload the fix based on his patch here.

[Philippe Normand]

Created attachment 244045 [details]
patch

[Philippe Normand]

Created attachment 244046 [details]
patch

[Sebastian Dröge (slomo)]

Seems good to me obviously ;) Thanks for extracting that part from my huge patch!

[Philippe Normand]

Created attachment 244054 [details]
patch

[Philippe Normand]

Comment on attachment 244054 [details]
patch

oops this is incomplete.

[Philippe Normand]

Created attachment 244057 [details]
patch

[WebKit Commit Bot]

Attachment 244057 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.cpp:40:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

Created attachment 244058 [details]
patch

[Carlos Garcia Campos]

Comment on attachment 244058 [details]
patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.cpp:65
> +    for (auto it = m_sourceBuffers.begin(), end = m_sourceBuffers.end(); it != end; ++it)
> +        (*it)->clearMediaSource();

This could be 

for (auto& sourceBuffer : m_sourceBuffers)
    sourceBuffer.clearMediaSource();

> Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.cpp:79
> +    return m_client->addSourceBuffer(m_sourceBuffers.last(), contentType);

You could use sourceBufferPrivate.get() here instead of calling last() again.

> Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.cpp:87
> +    size_t index = m_sourceBuffers.find(buffer);
> +    m_sourceBuffers.remove(index);

Shouldn't you also call clearMediaSource() here? Since it's removed from the vector here, it won't be called for this source bufer in the destructor. Since the order doesn't really matter, and we are always adding a new SourceBufferPrivateGStreamer, I wonder whether we could use a HashSet instead, and here you would just take the buffer can call clearMediaSource (if it's actually needed)

> Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.h:50
> +    MediaSourceClientGStreamer* client() const { return m_client.get(); }

Since m_client can't be null, you could return a reference instead.

[Zan Dobersek]

Comment on attachment 244058 [details]
patch

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

> Source/WebCore/platform/graphics/gstreamer/SourceBufferPrivateGStreamer.cpp:68
> +    SourceBufferPrivateClient::AppendResult result = m_mediaSource->client()->append(this, data, length);

Not in the scope of this bug, but MediaSourceClientGStreamer::append() doesn't have to operate on a PassRefPtr<SourceBufferPrivate> -- a simple reference to the SourceBufferPrivate object would do.

> Source/WebCore/platform/graphics/gstreamer/SourceBufferPrivateGStreamer.cpp:85
> +    m_mediaSource->client()->removedFromMediaSource(this);

Same here, MediaSourceClientGStreamer::removedFromMediaSource() could operate with a reference to the SourceBufferPrivate object.

[Philippe Normand]

(In reply to comment #11)
> Comment on attachment 244058 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=244058&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/SourceBufferPrivateGStreamer.cpp:68
> > +    SourceBufferPrivateClient::AppendResult result = m_mediaSource->client()->append(this, data, length);
> 
> Not in the scope of this bug, but MediaSourceClientGStreamer::append()
> doesn't have to operate on a PassRefPtr<SourceBufferPrivate> -- a simple
> reference to the SourceBufferPrivate object would do.
> 
> > Source/WebCore/platform/graphics/gstreamer/SourceBufferPrivateGStreamer.cpp:85
> > +    m_mediaSource->client()->removedFromMediaSource(this);
> 
> Same here, MediaSourceClientGStreamer::removedFromMediaSource() could
> operate with a reference to the SourceBufferPrivate object.

Right, I think I'll fix this in a separate patch. Thanks for pointing it out!

[Philippe Normand]

Created attachment 244262 [details]
patch

[Carlos Garcia Campos]

Comment on attachment 244262 [details]
patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaSourceGStreamer.cpp:79
> +    RefPtr<SourceBufferPrivateGStreamer> buffer = SourceBufferPrivateGStreamer::create(this);
> +    m_sourceBuffers.add(buffer.get());
> +    sourceBufferPrivate = buffer;

I'm confused about the ownership now. In the previous patch you kept a reference in the Vector, why are you using raw pointers now? Is removeSourceBuffer always called before the SourceBufferPrivate object is deleted?

> Source/WebCore/platform/graphics/gstreamer/SourceBufferPrivateGStreamer.cpp:86
> +    m_mediaSource->removeSourceBuffer(this);
> +    m_mediaSource->client().removedFromMediaSource(this);

hmm, now that I see this. It looks a bit weird to me that MediaSourceGStreamer::addSourceBuffer() calls MediaSourceClientGStreamer::addSourceBuffer() but removedFromMediaSource is called here instead of in MediaSourceGStreamer::removeSourceBuffer(). Also why does MediaSourceClientGStreamer::removedFromMediaSource() receive a PassRefPtr?

[Philippe Normand]

This likely isn't going to apply to the reworked MSE backend.