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

Source/modules/mediasource/SourceBuffer.cpp

 /*
  * Copyright (C) 2013 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 512 matching lines @@
     //  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))
         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.
+    //  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);
     if (data)
         m_pendingAppendData.append(data, size);
     m_pendingAppendDataOffset = 0;
 
     // 3. Set the updating attribute to true.
     m_updating = true;
 
@@ skipping 77 matching lines @@
     m_pendingRemoveStart = -1;
     m_pendingRemoveEnd = -1;
 
     // 11. Queue a task to fire a simple event named update at this SourceBuffer object.
     scheduleEvent(EventTypeNames::update);
 
     // 12. Queue a task to fire a simple event named updateend at this SourceBuffer object.
     scheduleEvent(EventTypeNames::updateend);
 }
 
+bool SourceBuffer::evictCodedFrames()
+{
+    return m_webSourceBuffer->evictCodedFrames();
+}
+
 void SourceBuffer::appendStreamInternal(Stream* stream, ExceptionState& exceptionState)
 {
     // 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
     // (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.");
         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))
         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.
+    // 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;
 
     // 3. Queue a task to fire a simple event named updatestart at this SourceBuffer object.
     scheduleEvent(EventTypeNames::updatestart);
 
     // 4. Asynchronously run the stream append loop algorithm with stream and maxSize.
 
     stream->neuter();
@@ skipping 11 matching lines @@
     // 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
 
     // 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.
     if (m_streamMaxSizeValid && !m_streamMaxSize) {
         appendStreamDone(true);
         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);
 }
 
 void SourceBuffer::appendStreamDone(bool success)
 {
     ASSERT(m_updating);
     ASSERT(m_loader);
     ASSERT(m_stream);
 
     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;
 
     // 13. Queue a task to fire a simple event named update at this SourceBuffer object.
     scheduleEvent(EventTypeNames::update);
 
     // 14. Queue a task to fire a simple event named updateend at this SourceBuffer object.
     scheduleEvent(EventTypeNames::updateend);
     TRACE_EVENT_ASYNC_END0("media", "SourceBuffer::appendStream", this);
 }
 
 void SourceBuffer::clearAppendStreamState()
 {
     m_streamMaxSizeValid = false;
     m_streamMaxSize = 0;
     m_loader.clear();
     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);
 }
 
 void SourceBuffer::didReceiveDataForClient(const char* data, unsigned dataLength)
 {
     WTF_LOG(Media, "SourceBuffer::didReceiveDataForClient(%d) %p", dataLength, this);
     ASSERT(m_updating);
     ASSERT(m_loader);
@@ skipping 16 matching lines @@
 {
     visitor->trace(m_source);
     visitor->trace(m_stream);
     visitor->trace(m_trackDefaults);
     visitor->trace(m_asyncEventQueue);
     RefCountedGarbageCollectedEventTargetWithInlineData<SourceBuffer>::trace(visitor);
     ActiveDOMObject::trace(visitor);
 }
 
 } // namespace blink