Bug 133174 – [MSE] Stored samples are not freed when SourceBuffer is removed from MediaSource

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

[MSE] Stored samples are not freed when SourceBuffer is removed from MediaSource

[Jer Noble]

Created attachment 231866 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/Modules/mediasource/SampleMap.h:47:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 8 files


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

[Eric Carlson]

Comment on attachment 231866 [details]
Patch

r=me with GTK build and style fixes.

[Darin Adler]

Comment on attachment 231866 [details]
Patch

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

I wanted to set commit-queue+ on this, but I shouldn’t since it will break the GTK build.

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:279
> +    for (auto& trackBufferPair : m_trackBufferMap) {
> +        trackBufferPair.value.samples.clear();
> +        trackBufferPair.value.decodeQueue.clear();
> +    }

A better, alternate way to write this is:

    for (auto& trackBuffer : m_trackBufferMap.values()) {
        trackBuffer.samples.clear();
        trackBuffer.decodeQueue.clear();
    }

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1441
> +    for (auto& trackBufferPair : m_trackBufferMap)
> +        extraMemoryCost += trackBufferPair.value.samples.sizeInBytes();

A better, alternate way to write this is:

    for (auto& trackBuffer : m_trackBufferMap.values())
        extraMemoryCost += trackBuffer.samples.sizeInBytes();

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1446
> +    JSC::JSLockHolder lock(scriptExecutionContext()->vm());

Apparently, to get this to compile on GTK, we need to add #include <runtime/JSLock.h> to the top of this file.

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:402
> +    virtual size_t sizeInBytes() const override { return CMSampleBufferGetTotalSampleSize(m_sample.get()); }

Note that this returns the sample size, not the overhead.

> Source/WebCore/platform/mock/mediasource/MockSourceBufferPrivate.cpp:53
> +    virtual size_t sizeInBytes() const override { return sizeof(m_box); }

And this returns an estimate of the overhead. I don’t think that really makes much sense. But it’s a mock, so I guess no big deal.

[Jon Lee]

<rdar://problem/16933311>

[Jer Noble]

Comment on attachment 231866 [details]
Patch

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

>> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:279
>> +    }
> 
> A better, alternate way to write this is:
> 
>     for (auto& trackBuffer : m_trackBufferMap.values()) {
>         trackBuffer.samples.clear();
>         trackBuffer.decodeQueue.clear();
>     }

Yes, that is much better!

>> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1441
>> +        extraMemoryCost += trackBufferPair.value.samples.sizeInBytes();
> 
> A better, alternate way to write this is:
> 
>     for (auto& trackBuffer : m_trackBufferMap.values())
>         extraMemoryCost += trackBuffer.samples.sizeInBytes();

DItto.

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:402
>> +    virtual size_t sizeInBytes() const override { return CMSampleBufferGetTotalSampleSize(m_sample.get()); }
> 
> Note that this returns the sample size, not the overhead.

Yes, I realize, but as a first-order approximation, it will be largely correct.

>> Source/WebCore/platform/mock/mediasource/MockSourceBufferPrivate.cpp:53
>> +    virtual size_t sizeInBytes() const override { return sizeof(m_box); }
> 
> And this returns an estimate of the overhead. I don’t think that really makes much sense. But it’s a mock, so I guess no big deal.

Well, sort of.  The MockSampleBox is effectively "the original sample data", since the data representation is just a serialized MockSampleBox. But I get that CMSampleBuffers may also have the same kind of internal caching of the sample data that I'm not accounting for.

[Jer Noble]

Created attachment 231895 [details]
Patch for landing.

[Jer Noble]

Created attachment 231898 [details]
Patch for landing.

[Jer Noble]

Committed r169209: <http://trac.webkit.org/changeset/169209>