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..ea240a2ff3c3f13008717ad21c24e057957c1460 100644
--- a/Source/modules/mediasource/SourceBuffer.cpp
+++ b/Source/modules/mediasource/SourceBuffer.cpp
@@ -530,7 +530,12 @@ void SourceBuffer::appendBufferInternal(const unsigned char* data, unsigned 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.
+    //  4. Run the coded frame eviction algorithm.
+    if (!evictCodedFrames()) {
+        //  5. 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.");

[philipj_slow, 2015/06/01 14:37:27]

This is the new bit covered by the test.

[servolk, 2015/06/02 19:24:31]

Acknowledged.

+        return;
+    }
 
     // 2. Add data to the end of the input buffer.
     ASSERT(data || size == 0);
@@ -628,6 +633,11 @@ void SourceBuffer::removeAsyncPart()
     scheduleEvent(EventTypeNames::updateend);
 }
 
+bool SourceBuffer::evictCodedFrames()
+{
+    return m_webSourceBuffer->evictCodedFrames();
+}
+
 void SourceBuffer::appendStreamInternal(Stream* stream, ExceptionState& exceptionState)
 {
     // Section 3.2 appendStream()
@@ -651,7 +661,12 @@ void SourceBuffer::appendStreamInternal(Stream* stream, ExceptionState& exceptio
     //  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.
+    // 4. Run the coded frame eviction algorithm.
+    if (!evictCodedFrames()) {
+        //  5. 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 stream.");

[philipj_slow, 2015/06/01 14:37:27]

Is it possible to test the appendStream() case?

[servolk, 2015/06/02 19:24:31]

Done. But actually I think the test is hitting the other case for appendStream
(the one in appendStreamDone). Here we run evictCodedFrames before we started
appending data, so this one should succeed.

[servolk, 2015/06/02 23:50:05]

Actually, no, I was wrong. The new test case that I've added hits both code
paths. I've added logging in all three places where we invoke evictCodedFrames
and re-ran the tests and I can see these log messages from all three places in
stderr output.
Here is how it works: the test keeps appending the same stream to SB over and
over again and eventually the SB fills up and we hit the appendError code path
in SourceBuffer::appendStreamDone. But then the test doesn't stop, it ignores
the error event and sees updateend event, and then tries once again to append
more data to SB that's already full. Since now the buffer is already full at the
beginning of append operation, we hit this code path right here and throw
QuotaExceededErr. The test catches the QuotaExceededErr exception and completes
successfully.

+        return;
+    }
 
     // 2. Set the updating attribute to true.
     m_updating = true;
@@ -683,7 +698,7 @@ void SourceBuffer::appendStreamAsyncPart()
         return;
     }
 
-    // Steps 3-11 are handled by m_loader.
+    // Steps 3-9 are handled by m_loader.
     // Note: Passing 0 here signals that maxSize was not set. (i.e. Read all the data in the stream).
     m_loader->start(executionContext(), *m_stream, m_streamMaxSizeValid ? m_streamMaxSize : 0);
 }
@@ -697,24 +712,22 @@ 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();
         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|.
+    // Steps 1-9 are handled by appendStreamAsyncPart(), |m_loader|, and |m_webSourceBuffer|.
+
+    // 10. Run the coded frame eviction algorithm.
+    if (!evictCodedFrames()) {
+        // 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.
+        appendError();

[philipj_slow, 2015/06/01 14:37:27]

Is it possible to reach this case and write a test for the events that are fired
as a result? It seems like a strange case, might it be dead code in both spec
and implementation?

[servolk, 2015/06/02 19:24:31]

See above, I think the new test that I've added is hitting this code path, since
we are doing evictCodedFrames after appending stream data here and we might be
over memory limit and GC still might fail if data is not consumed by the media
pipeline (which will be the case during test, since the test doesn't start
playback, only keeps appending data).

+        TRACE_EVENT_ASYNC_END0("media", "SourceBuffer::appendStream", this);
+        return;
+    }
+
     // 12. Loop Done: Set the updating attribute to false.

[philipj_slow, 2015/06/01 14:37:27]

Can you also renumber the remaining steps, this one starting at 16?

     m_updating = false;
 
@@ -734,6 +747,22 @@ void SourceBuffer::clearAppendStreamState()
     m_stream = nullptr;
 }
 
+void SourceBuffer::appendError()

[philipj_slow, 2015/06/01 14:37:27]

Per spec there's a "decode error" argument that's always false where this is
called, but is there a place in "Initialization Segment Received" where there
should be a TODO to call appendError(true) or similar?

[servolk, 2015/06/02 19:24:31]

You are right. I've added a decodeError parameter, but I'm not sure how to call
endOfStream here, since it expects ExceptionState as an input parameter and we
don't have it here. Should we pass exception state from the point where async
call is initiated? Does Javascript expect this async method to throw exception
in the async part of the method?

+{
+    // 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);
+}
+
 void SourceBuffer::didStartLoading()
 {
     WTF_LOG(Media, "SourceBuffer::didStartLoading() %p", this);