[MSE] Stored samples are not freed when SourceBuffer is removed from MediaSource
Created attachment 231866 [details] Patch
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.
Comment on attachment 231866 [details] Patch r=me with GTK build and style fixes.
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.
<rdar://problem/16933311>
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.
Created attachment 231895 [details] Patch for landing.
Created attachment 231898 [details] Patch for landing.
Committed r169209: <http://trac.webkit.org/changeset/169209>