Bug 135614 – [MSE] Implement a maximum buffer size for SourceBuffer

• Component: Media
• Status: RESOLVED FIXED

[Jer Noble]

[MSE] Implement a maximum buffer size for SourceBuffer

[Jer Noble]

Created attachment 236035 [details]
WIP

[Eric Carlson]

Created attachment 236295 [details]
Updated WIP

[WebKit Commit Bot]

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.

[Eric Carlson]

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.

[Eric Carlson]

<rdar://problem/17896828>

[Jer Noble]

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.

[Eric Carlson]

(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.

[Eric Carlson]

Created attachment 236639 [details]
Updated patch.

[Jer Noble]

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.

[Eric Carlson]

Created attachment 236676 [details]
Patch for landing.

[Eric Carlson]

(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.

[Eric Carlson]

Created attachment 236677 [details]
Rebased patch for landing.

[WebKit Commit Bot]

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.

[Eric Carlson]

Committed r172657: https://trac.webkit.org/r172657