Fix MSE GC, make it less aggressive, more spec-compliant (Blink CL)

Source/modules/mediasource/SourceBuffer.cpp

Index: Source/modules/mediasource/SourceBuffer.cpp
diff --git a/Source/modules/mediasource/SourceBuffer.cpp b/Source/modules/mediasource/SourceBuffer.cpp
index ceea5a8db2bc65ba31173ef4588b53b514fd3e84..09311f4b8d0bb08917efa2c45d9b030f27c18d14 100644
--- a/Source/modules/mediasource/SourceBuffer.cpp
+++ b/Source/modules/mediasource/SourceBuffer.cpp
@@ -40,6 +40,8 @@
 #include "core/events/Event.h"
 #include "core/events/GenericEventQueue.h"
 #include "core/fileapi/FileReaderLoader.h"
+#include "core/html/HTMLMediaElement.h"
+#include "core/html/MediaError.h"
 #include "core/html/TimeRanges.h"
 #include "core/streams/Stream.h"
 #include "modules/mediasource/MediaSource.h"
@@ -513,24 +515,60 @@ void SourceBuffer::scheduleEvent(const AtomicString& eventName)
     m_asyncEventQueue->enqueueEvent(event.release());
 }
 
+bool SourceBuffer::prepareAppend(ExceptionState& exceptionState)
+{
+    TRACE_EVENT_ASYNC_BEGIN0("media", "SourceBuffer::prepareAppend", this);
+    // http://w3c.github.io/media-source/#sourcebuffer-prepare-append
+    // 3.5.4 Prepare Append Algorithm

[philipj_slow, 2015/07/02 09:48:21]

Section numbers tend to change, but they are consistently used elsewhere in
modules/mediasource/ (didn't check) and perhaps even correct elsewhere, then OK
:)

[servolk, 2015/07/07 23:01:52]

Acknowledged.

+    //  1. If the SourceBuffer has been removed from the sourceBuffers attribute of the parent media source then throw an InvalidStateError exception and abort these steps.

[philipj_slow, 2015/07/02 09:48:21]

This is indented more than (some of) the steps that follow.

[servolk, 2015/07/07 23:01:52]

Done.

+    //  2. If the updating attribute equals true, then throw an InvalidStateError exception and abort these steps.
+    if (throwExceptionIfRemovedOrUpdating(isRemoved(), m_updating, exceptionState)) {
+        TRACE_EVENT_ASYNC_END0("media", "SourceBuffer::prepareAppend", this);
+        return false;
+    }
+
+    // 3. If the HTMLMediaElement.error attribute is not null. then throw an InvalidStateError exception and abort these steps.

[philipj_slow, 2015/07/02 09:48:21]

https://www.w3.org/Bugs/Public/show_bug.cgi?id=28881

[servolk, 2015/07/07 23:01:52]

Done.

+    if (m_source->getHTMLMediaElement()->error()) {
+        exceptionState.throwDOMException(InvalidStateError, "Error detected in HTMLMediaElement this SourceBuffer is attached to.");

[philipj_slow, 2015/07/02 09:48:21]

This error message doesn't appear in any test expectations, so I guess this is
untested? I'd perhaps make it more boring by saying just "The
HTMLMediaElement.error attribute is not null."

[servolk, 2015/07/07 23:01:52]

Done.

+        TRACE_EVENT_ASYNC_END0("media", "SourceBuffer::prepareAppend", this);
+        return false;
+    }
+
+    //  4. If the readyState attribute of the parent media source is in the "ended" state then run the following steps:
+    //     1. Set the readyState attribute of the parent media source to "open"
+    //     2. Queue a task to fire a simple event named sourceopen at the parent media source .

[philipj_slow, 2015/07/02 09:48:21]

https://www.w3.org/Bugs/Public/show_bug.cgi?id=28882

[servolk, 2015/07/07 23:01:52]

Done.

+    m_source->openIfInEndedState();
+
+    //  5. Run the coded frame eviction algorithm.
+    if (!evictCodedFrames()) {
+        //  6. If the buffer full flag equals true, then throw a QUOTA_EXCEEDED_ERR exception and abort these steps.
+        exceptionState.throwDOMException(QuotaExceededError, "The SourceBuffer is full, and cannot free space to append additional buffers.");
+        TRACE_EVENT_ASYNC_END0("media", "SourceBuffer::prepareAppend", this);
+        return false;
+    }
+
+    TRACE_EVENT_ASYNC_END0("media", "SourceBuffer::prepareAppend", this);
+    return true;
+}
+
+bool SourceBuffer::evictCodedFrames()
+{
+    double currentTime = m_source->getHTMLMediaElement()->currentTime();

[philipj_slow, 2015/07/02 09:48:21]

Can m_source or m_source->getHTMLMediaElement() be null?

[servolk, 2015/07/07 23:01:52]

It shouldn't be null here, since evictCodedFrame is an internal method that is
called only from appendBuffer/appendStream, and we should error out sooner (via
throwExceptionIfRemovedOrUpdating) if we are not in the opened state. So I'll
just add asserts here.

+    return m_webSourceBuffer->evictCodedFrames(currentTime);
+}
+
 void SourceBuffer::appendBufferInternal(const unsigned char* data, unsigned size, ExceptionState& exceptionState)
 {
+    TRACE_EVENT_ASYNC_BEGIN1("media", "SourceBuffer::appendBuffer", this, "size", size);
     // Section 3.2 appendBuffer()
     // https://dvcs.w3.org/hg/html-media/raw-file/default/media-source/media-source.html#widl-SourceBuffer-appendBuffer-void-ArrayBufferView-data
 
     // 1. Run the prepare append algorithm.
-    // https://dvcs.w3.org/hg/html-media/raw-file/default/media-source/media-source.html#sourcebuffer-prepare-append
-    //  1. If this object has been removed from the sourceBuffers attribute of the parent media source then throw an InvalidStateError exception and abort these steps.
-    //  2. If the updating attribute equals true, then throw an InvalidStateError exception and abort these steps.
-    if (throwExceptionIfRemovedOrUpdating(isRemoved(), m_updating, exceptionState))
+    if (!prepareAppend(exceptionState)) {
+        TRACE_EVENT_ASYNC_END0("media", "SourceBuffer::appendBuffer", this);
         return;
-
-    TRACE_EVENT_ASYNC_BEGIN1("media", "SourceBuffer::appendBuffer", this, "size", size);
-
-    //  3. If the readyState attribute of the parent media source is in the "ended" state then run the following steps: ...
-    m_source->openIfInEndedState();
-
-    //  Steps 4-5 - end "prepare append" algorithm.
+    }
+    TRACE_EVENT_ASYNC_STEP_INTO0("media", "SourceBuffer::appendBuffer", this, "prepareAppend");
 
     // 2. Add data to the end of the input buffer.
     ASSERT(data || size == 0);
@@ -630,28 +668,22 @@ void SourceBuffer::removeAsyncPart()
 
 void SourceBuffer::appendStreamInternal(Stream* stream, ExceptionState& exceptionState)
 {
+    TRACE_EVENT_ASYNC_BEGIN0("media", "SourceBuffer::appendStream", this);
+
     // Section 3.2 appendStream()
-    // https://dvcs.w3.org/hg/html-media/raw-file/default/media-source/media-source.html#widl-SourceBuffer-appendStream-void-Stream-stream-unsigned-long-long-maxSize
+    // http://w3c.github.io/media-source/#widl-SourceBuffer-appendStream-void-ReadableStream-stream-unsigned-long-long-maxSize
     // (0. If the stream has been neutered, then throw an InvalidAccessError exception and abort these steps.)
     if (stream->isNeutered()) {
         exceptionState.throwDOMException(InvalidAccessError, "The stream provided has been neutered.");
+        TRACE_EVENT_ASYNC_END0("media", "SourceBuffer::appendStream", this);
         return;
     }
 
     // 1. Run the prepare append algorithm.
-    //  Section 3.5.4 Prepare Append Algorithm.
-    //  https://dvcs.w3.org/hg/html-media/raw-file/default/media-source/media-source.html#sourcebuffer-prepare-append
-    //  1. If this object has been removed from the sourceBuffers attribute of the parent media source then throw an InvalidStateError exception and abort these steps.
-    //  2. If the updating attribute equals true, then throw an InvalidStateError exception and abort these steps.
-    if (throwExceptionIfRemovedOrUpdating(isRemoved(), m_updating, exceptionState))
+    if (!prepareAppend(exceptionState)) {
+        TRACE_EVENT_ASYNC_END0("media", "SourceBuffer::appendStream", this);
         return;
-
-    TRACE_EVENT_ASYNC_BEGIN0("media", "SourceBuffer::appendStream", this);
-
-    //  3. If the readyState attribute of the parent media source is in the "ended" state then run the following steps: ...
-    m_source->openIfInEndedState();
-
-    // Steps 4-5 of the prepare append algorithm are handled by m_webSourceBuffer.
+    }
 
     // 2. Set the updating attribute to true.
     m_updating = true;
@@ -660,7 +692,6 @@ void SourceBuffer::appendStreamInternal(Stream* stream, ExceptionState& exceptio
     scheduleEvent(EventTypeNames::updatestart);
 
     // 4. Asynchronously run the stream append loop algorithm with stream and maxSize.
-
     stream->neuter();
     m_loader = adoptPtr(new FileReaderLoader(FileReaderLoader::ReadByClient, this));
     m_stream = stream;
@@ -672,9 +703,10 @@ void SourceBuffer::appendStreamAsyncPart()
     ASSERT(m_updating);
     ASSERT(m_loader);
     ASSERT(m_stream);
+    TRACE_EVENT_ASYNC_STEP_INTO0("media", "SourceBuffer::appendStream", this, "appendStreamAsyncPart");
 
     // Section 3.5.6 Stream Append Loop
-    // https://dvcs.w3.org/hg/html-media/raw-file/default/media-source/media-source.html#sourcebuffer-stream-append-loop
+    // http://w3c.github.io/media-source/#sourcebuffer-stream-append-loop
 
     // 1. If maxSize is set, then let bytesLeft equal maxSize.
     // 2. Loop Top: If maxSize is set and bytesLeft equals 0, then jump to the loop done step below.
@@ -697,24 +729,14 @@ void SourceBuffer::appendStreamDone(bool success)
     clearAppendStreamState();
 
     if (!success) {
-        // Section 3.5.3 Append Error Algorithm
-        // https://dvcs.w3.org/hg/html-media/raw-file/default/media-source/media-source.html#sourcebuffer-append-error
-        //
-        // 1. Run the reset parser state algorithm. (Handled by caller)
-        // 2. Set the updating attribute to false.
-        m_updating = false;
-
-        // 3. Queue a task to fire a simple event named error at this SourceBuffer object.
-        scheduleEvent(EventTypeNames::error);
-
-        // 4. Queue a task to fire a simple event named updateend at this SourceBuffer object.
-        scheduleEvent(EventTypeNames::updateend);
+        appendError(false);
         TRACE_EVENT_ASYNC_END0("media", "SourceBuffer::appendStream", this);
         return;
     }
 
     // Section 3.5.6 Stream Append Loop
     // Steps 1-11 are handled by appendStreamAsyncPart(), |m_loader|, and |m_webSourceBuffer|.
+
     // 12. Loop Done: Set the updating attribute to false.
     m_updating = false;
 
@@ -734,6 +756,28 @@ void SourceBuffer::clearAppendStreamState()
     m_stream = nullptr;
 }
 
+void SourceBuffer::appendError(bool decodeError)
+{
+    // Section 3.5.3 Append Error Algorithm
+    // https://dvcs.w3.org/hg/html-media/raw-file/default/media-source/media-source.html#sourcebuffer-append-error
+    //
+    // 1. Run the reset parser state algorithm. (Handled by caller)

[philipj_slow, 2015/07/02 09:48:21]

Is there anything that can be asserted to check that the caller did what it
should?

[servolk, 2015/07/07 23:01:52]

TBH, initially I just copied this block of code from
appendBufferInternal/appendStreamInternal. But I guess we could call
m_webSourceBuffer->abort() here, that should ensure that parser state is reset
(see step #4 of SourceBuffer::about here).

+    // 2. Set the updating attribute to false.
+    m_updating = false;
+
+    // 3. Queue a task to fire a simple event named error at this SourceBuffer object.
+    scheduleEvent(EventTypeNames::error);
+
+    // 4. Queue a task to fire a simple event named updateend at this SourceBuffer object.
+    scheduleEvent(EventTypeNames::updateend);
+
+    // 5. If decode error is true, then run the end of stream algorithm with the
+    // error parameter set to "decode".
+    if (decodeError) {

[philipj_slow, 2015/07/02 09:48:22]

Don't need the {} for one line.

[servolk, 2015/07/07 23:01:52]

Done.

+        m_source->endOfStream("decode", ASSERT_NO_EXCEPTION);
+    }
+}
+
 void SourceBuffer::didStartLoading()
 {
     WTF_LOG(Media, "SourceBuffer::didStartLoading() %p", this);
@@ -750,7 +794,15 @@ void SourceBuffer::didReceiveDataForClient(const char* data, unsigned dataLength
 void SourceBuffer::didFinishLoading()
 {
     WTF_LOG(Media, "SourceBuffer::didFinishLoading() %p", this);
-    appendStreamDone(true);
+    // Section 3.5.6 Stream Append Loop
+    // Steps 1-9 are handled by appendStreamAsyncPart(), |m_loader|, and |m_webSourceBuffer|.
+
+    // 10. Run the coded frame eviction algorithm.
+    bool evictCodedFramesResult = evictCodedFrames();
+
+    // (will be done inside appendStreamDone)
+    // 11. If the buffer full flag equals true, then run the append error algorithm with the decode error parameter set to false and abort this algorithm.

[philipj_slow, 2015/07/02 09:48:21]

This and some other comments is a bit long, if you can find some rough max line
length in this file and keep comments to that (probably 80, 100 or 120 columns)
that would make it more readable.

[servolk, 2015/07/07 23:01:52]

I was just trying to be consistent with the rest of the code in this file. I've
noticed that comments and even code in other methods don't seem to have any line
length limits (e.g. take a look at SourceBuffer::remove in this file, line 336
is 265 chars long, and that code, not even something like comment with URL,
which can't be wrapped). So should I reformat the whole file or keep it the old
way?

[philipj_slow, 2015/07/08 12:47:43]

Looks like different parts of this file use different wrapping widths for
comments, but no limit for code. No need to do anything, everything is a mess :)

[servolk, 2015/07/08 19:03:47]

Acknowledged.

+    appendStreamDone(evictCodedFramesResult);
 }
 
 void SourceBuffer::didFail(FileError::ErrorCode errorCode)