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

[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 236639: Updated patch.
https://bugs.webkit.org/attachment.cgi?id=236639&action=review

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