Summary: | [MSE] Implement a maximum buffer size for SourceBuffer | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jer Noble <jer.noble> | ||||||||||||
Component: | Media | Assignee: | Eric Carlson <eric.carlson> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | calvaris, commit-queue, eric.carlson, esprehn+autocc, glenn, gyuyoung.kim, jer.noble, ltilve, philipj, sergio, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Jer Noble
2014-08-05 10:57:13 PDT
Created attachment 236035 [details]
WIP
Created attachment 236295 [details]
Updated WIP
Attachment 236295 [details] did not pass style-queue:
ERROR: Source/WebCore/html/HTMLMediaSession.cpp:305: One space before end of line comments [whitespace/comments] [5]
ERROR: Source/WebCore/html/HTMLMediaSession.cpp:306: One space before end of line comments [whitespace/comments] [5]
Total errors found: 2 in 10 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 236295 [details] Updated WIP This doesn't have any tests yet, but we need to make significant changes to the mock MSE implementation and JS library so I have filed https://bugs.webkit.org/show_bug.cgi?id=135847 for adding tests. Comment on attachment 236295 [details] Updated WIP View in context: https://bugs.webkit.org/attachment.cgi?id=236295&action=review r=me, with a couple nits: > Source/WebCore/Modules/mediasource/SourceBuffer.cpp:663 > + m_bufferFull = extraMemoryCost() + newDataSize > maximumBufferSize; This is technically against the spec. The buffer full flag should be modified in step 6.4 of the segment parser loop algorithm, which would best fit in sourceBufferPrivateAppendComplete(). > Source/WebCore/html/HTMLMediaSession.cpp:333 > +size_t HTMLMediaSession::maximumMediaSourceBufferSize(const HTMLMediaElement& element) const > +{ > + // A good quality 1080p video uses 8,000 kbps and stereo audio uses 384 kbps, so assume 95% for video and 5% for audio. > + const float bufferBudgetPercentageForVideo = .95; > + const float bufferBudgetPercentageForAudio = .05; > + > + size_t maximum; > + Settings* settings = element.document().settings(); > + if (settings) > + maximum = settings->maximumSourceBufferSize(); > + else > + maximum = fiveMinutesOf1080PVideo + fiveMinutesStereoAudio; > + > + // Allow an element to buffer as though it as audio-only even if it doesn't have any active tracks. > + size_t bufferSize = static_cast<size_t>(maximum * bufferBudgetPercentageForAudio); > + if (element.hasVideo()) > + bufferSize += static_cast<size_t>(maximum * bufferBudgetPercentageForVideo); > + > + // FIXME: we might want to modify this algorithm to: > + // - decrease the maximum size for background tabs > + // - decrease the maximum size allowed for inactive elements when a process has more than one > + // element, eg. so a page with many elements which are played one at a time doesn't keep all > + // everything buffered after an element has been played. > + > + return bufferSize; > +} So this code will give the same buffer to a SourceBuffer with a single audio track and a SourceBuffer with a single video track, if both those buffers are attached to the same HTMLMediaElement. I think we should add to the list of FIXMEs that the budget should be based on what tracks each SourceBuffer contains, rather than the HTMLMediaElement as a whole. (In reply to comment #6) > (From update of attachment 236295 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=236295&action=review > > r=me, with a couple nits: > > > Source/WebCore/Modules/mediasource/SourceBuffer.cpp:663 > > + m_bufferFull = extraMemoryCost() + newDataSize > maximumBufferSize; > > This is technically against the spec. The buffer full flag should be modified in step 6.4 of the segment parser loop algorithm, which would best fit in sourceBufferPrivateAppendComplete(). > Fixed, we only ever set m_bufferFull to true in sourceBufferPrivateAppendComplete. > > Source/WebCore/html/HTMLMediaSession.cpp:333 > > +size_t HTMLMediaSession::maximumMediaSourceBufferSize(const HTMLMediaElement& element) const > > +{ > > + // A good quality 1080p video uses 8,000 kbps and stereo audio uses 384 kbps, so assume 95% for video and 5% for audio. > > + const float bufferBudgetPercentageForVideo = .95; > > + const float bufferBudgetPercentageForAudio = .05; > > + > > + size_t maximum; > > + Settings* settings = element.document().settings(); > > + if (settings) > > + maximum = settings->maximumSourceBufferSize(); > > + else > > + maximum = fiveMinutesOf1080PVideo + fiveMinutesStereoAudio; > > + > > + // Allow an element to buffer as though it as audio-only even if it doesn't have any active tracks. > > + size_t bufferSize = static_cast<size_t>(maximum * bufferBudgetPercentageForAudio); > > + if (element.hasVideo()) > > + bufferSize += static_cast<size_t>(maximum * bufferBudgetPercentageForVideo); > > + > > + // FIXME: we might want to modify this algorithm to: > > + // - decrease the maximum size for background tabs > > + // - decrease the maximum size allowed for inactive elements when a process has more than one > > + // element, eg. so a page with many elements which are played one at a time doesn't keep all > > + // everything buffered after an element has been played. > > + > > + return bufferSize; > > +} > > So this code will give the same buffer to a SourceBuffer with a single audio track and a SourceBuffer with a single video track, if both those buffers are attached to the same HTMLMediaElement. I think we should add to the list of FIXMEs that the budget should be based on what tracks each SourceBuffer contains, rather than the HTMLMediaElement as a whole. Good point! I fixed this by having this method take a SourceBuffer and adding hasAudio and hasVideo to SourceBuffer. Created attachment 236639 [details]
Updated patch.
Comment on attachment 236639 [details] Updated patch. View in context: https://bugs.webkit.org/attachment.cgi?id=236639&action=review r=me, with nits: > Source/WebCore/Modules/mediasource/SourceBuffer.cpp:92 > +#if !LOG_DISABLED > +static void logRanges(const String& prefix, const TimeRanges* ranges) > +{ > + > + if (!ranges->length()) > + return; > + > + StringBuilder log; > + > + log.append(prefix); > + for (size_t i = 0; i < ranges->length(); ++i) > + log.append(String::format("[%lf .. %lf] ", ranges->start(i, ASSERT_NO_EXCEPTION), ranges->end(i, ASSERT_NO_EXCEPTION))); > + > + LOG(MediaSource, "%s", log.toString().utf8().data()); > +} > + > +#define LOG_RANGES(prefix, ranges) logRanges(prefix, ranges) > +#else > +#define LOG_RANGES(prefix, ranges) ((void)0) > +#endif It would be better here to add a "void dump(PrintStream& out) const" member variable to PlatformTimeRanges which does this. We have an existing one for MediaTime which you could use as an example. > Source/WebCore/Modules/mediasource/SourceBuffer.cpp:756 > +#if !LOG_DISABLED > + LOG(MediaSource, "SourceBuffer::evictCodedFrames(%p) - evicted %zu bytes", this, initialBufferedSize - extraMemoryCost()); > +#endif This doesn't need a #if guard. > Source/WebCore/Modules/mediasource/SourceBuffer.cpp:763 > + size_t currentTimeRange = m_buffered->find(m_source->currentTime()); TimeRanges methods take doubles, but PlatformTimeRanges methods take MediaTimes. You could re-use the existing "currentMediaTime" by doing a "m_buffered->ranges().find(currentMediaTime)" here, or by caching the result of m_buffered->ranges() in a local to use later. > Source/WebCore/Modules/mediasource/SourceBuffer.cpp:767 > +#if !LOG_DISABLED > + LOG(MediaSource, "SourceBuffer::evictCodedFrames(%p) - evicted %zu bytes but FAILED to free enough", this, initialBufferedSize - extraMemoryCost()); > +#endif No #if guard necessary. > Source/WebCore/Modules/mediasource/SourceBuffer.cpp:778 > + size_t startTimeRange = m_buffered->find(rangeStart.toDouble()); Ditto here about PlatformTimeRanges. This becomes "m_buffered->ranges().find(rangeStart)", or "bufferedRanges.find(rangeStart)" if you use a local variable. > Source/WebCore/Modules/mediasource/SourceBuffer.cpp:780 > + size_t endTimeRange = m_buffered->find(rangeEnd.toDouble()); Ditto. > Source/WebCore/Modules/mediasource/SourceBuffer.cpp:784 > + rangeEnd = MediaTime::createWithDouble(m_buffered->start(endTimeRange, ASSERT_NO_EXCEPTION)); ditto. > Source/WebCore/Modules/mediasource/SourceBuffer.cpp:801 > +#if !LOG_DISABLED > + LOG(MediaSource, "SourceBuffer::evictCodedFrames(%p) - evicted %zu bytes%s", this, initialBufferedSize - extraMemoryCost(), m_bufferFull ? "" : " but FAILED to free enough"); > +#endif Ditto. Created attachment 236676 [details]
Patch for landing.
(In reply to comment #9) > (From update of attachment 236639 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=236639&action=review > > It would be better here to add a "void dump(PrintStream& out) const" member variable to PlatformTimeRanges which does this. We have an existing one for MediaTime which you could use as an example. > Fixed. > > Source/WebCore/Modules/mediasource/SourceBuffer.cpp:756 > > +#if !LOG_DISABLED > > + LOG(MediaSource, "SourceBuffer::evictCodedFrames(%p) - evicted %zu bytes", this, initialBufferedSize - extraMemoryCost()); > > +#endif > > This doesn't need a #if guard. > Ditto. > > Source/WebCore/Modules/mediasource/SourceBuffer.cpp:763 > > + size_t currentTimeRange = m_buffered->find(m_source->currentTime()); > > TimeRanges methods take doubles, but PlatformTimeRanges methods take MediaTimes. You could re-use the existing "currentMediaTime" by doing a "m_buffered->ranges().find(currentMediaTime)" here, or by caching the result of m_buffered->ranges() in a local to use later. > Ditto. > > Source/WebCore/Modules/mediasource/SourceBuffer.cpp:767 > > +#if !LOG_DISABLED > > + LOG(MediaSource, "SourceBuffer::evictCodedFrames(%p) - evicted %zu bytes but FAILED to free enough", this, initialBufferedSize - extraMemoryCost()); > > +#endif > > No #if guard necessary. > Ditto. > > Source/WebCore/Modules/mediasource/SourceBuffer.cpp:778 > > + size_t startTimeRange = m_buffered->find(rangeStart.toDouble()); > > Ditto here about PlatformTimeRanges. This becomes "m_buffered->ranges().find(rangeStart)", or "bufferedRanges.find(rangeStart)" if you use a local variable. > Ditto. > > Source/WebCore/Modules/mediasource/SourceBuffer.cpp:780 > > + size_t endTimeRange = m_buffered->find(rangeEnd.toDouble()); > > Ditto. > Ditto. > > Source/WebCore/Modules/mediasource/SourceBuffer.cpp:784 > > + rangeEnd = MediaTime::createWithDouble(m_buffered->start(endTimeRange, ASSERT_NO_EXCEPTION)); > > ditto. > Ditto. > > Source/WebCore/Modules/mediasource/SourceBuffer.cpp:801 > > +#if !LOG_DISABLED > > + LOG(MediaSource, "SourceBuffer::evictCodedFrames(%p) - evicted %zu bytes%s", this, initialBufferedSize - extraMemoryCost(), m_bufferFull ? "" : " but FAILED to free enough"); > > +#endif > > Ditto. Ditto. Created attachment 236677 [details]
Rebased patch for landing.
Attachment 236677 [details] did not pass style-queue:
ERROR: Source/WebCore/platform/graphics/PlatformTimeRanges.h:36: Code inside a namespace should not be indented. [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/PlatformTimeRanges.cpp:29: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Source/WebCore/Modules/mediasource/SourceBuffer.cpp:610: One line control clauses should not use braces. [whitespace/braces] [4]
Total errors found: 3 in 16 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Committed r172657: https://trac.webkit.org/r172657 |