[webkit-reviews] review granted: [Bug 135614] [MSE] Implement a maximum buffer size for SourceBuffer : [Attachment 236295] Updated WIP

[bugzilla-daemon at webkit.org]

Jer Noble <jer.noble at apple.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 135614: [MSE] Implement a maximum buffer size for SourceBuffer
https://bugs.webkit.org/show_bug.cgi?id=135614

Attachment 236295: Updated WIP
https://bugs.webkit.org/attachment.cgi?id=236295&action=review

------- Additional Comments from Jer Noble <jer.noble at apple.com>
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.