1120079 - MSE: SourceBuffer RangeRemoval algorithm is run even when there is nothing to remove.

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P2)

Description

[Jean-Yves Avenard [:jya]]

The specs are rather ambiguous on the matter...
In http://w3c.github.io/media-source/#duration-change
"If the new duration is less than old duration, then run the range removal algorithm with new duration and old duration as the start and end of the removal range."

http://w3c.github.io/media-source/#sourcebuffer-range-removal:
Which:
"Set the updating attribute to true."

Now, calling MediaSource::endOfStream will run the End Of Stream algorithm:
http://w3c.github.io/media-source/#end-of-stream-algorithm

which states:
"Run the duration change algorithm with new duration set to the highest end time reported by the buffered attribute across all SourceBuffer objects in sourceBuffers."

So according to the spec, regardless is there's something to remove or not, we should be running the RangeRemove Algorithm that will set sourcebuffer::updating to true, and fire updatestart/update/updateend event.

However, our current implementation currently queues all tasks for later run in the main thread.
The following code:
  mediaSource.addEventListener('sourceended', function(e) {
     sourceBuffer.appendBuffer(data)
  });
  mediaSource.endOfStream();

endOfStream() queue a call to RangeRemoval and sets sourceBuffer.updating to true.

When appendBuffer gets to run, as sourceBuffer.updating is true; it throws an InvalidStateError exception. 

Now it could be argued that it's the valid thing to do, and after a call to endOfStream() we should also be waiting to "updateend" event. However this event may not be fired (if MediaSource::duration was inferior to the current sourceBuffer duration) so it's not a reliable wait.

Also, the W3C tests suite, as well as YouTube tests suite expects that appendBuffer following an endOfStream always succeeds and set the mediasource::readyState back to "open" (from "ended").

As the new duration is set to the highest end time across all source buffers: there will never be anything to remove from any of the source buffers: and so there's no point to run the Range Removal algorithm.

The code should be able to detect that the new duration is set following a call to endOfStream ; and not call SourceBuffer::RangeRemoval then.

Test case:
http://people.mozilla.org/~jyavenard/tests/mse_mp4/gizmo.html?eos=1&eosat=2

w3c test failing: media-source/mediasource-append-buffer.html
""Test SourceBuffer.appendBuffer() triggering an 'ended' to 'open' transition.");"

Comment 1

[cajbir (:cajbir)]

Is this related to the discussion in bug 1065215? In particular bug 1065215 comment 8 and on.

Comment 2

[Jean-Yves Avenard [:jya]]

They are related. But the issue here isn't with endOfStream() per say; but how actions following it may fail. 

I have an easy fix for it in my queue. 

But there are still more problems preventing readyState to go from "ended" back to "open".

Comment 3

[Jean-Yves Avenard [:jya]]

(In reply to cajbir (:cajbir) from comment #1)
> Is this related to the discussion in bug 1065215? In particular bug 1065215
> comment 8 and on.

After re-reading.

Yes, this is identical to what karlt wrote in bug 1065215 comment 9 and his recommendation with step 1.

Regardless of the duration, a call to endOfStream, even if it reduces the duration (and hence should trigger Range Removal) ; as there's by definition nothing to removed (as we use the highest end time reported by the buffered attribute) ; there's no point running Range Removal.

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: Do not call Range Removal algorithm after endOfStream

Do not call the Range Removal algorith after a call to endOfStream or when appendBuffer updated the duration

Comment 5

[cajbir (:cajbir)]

Comment on attachment 8547238 [details] [diff] [review]
Do not call Range Removal algorithm after endOfStream

Review of attachment 8547238 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/mediasource/MediaSource.h
@@ +128,5 @@
>  
>    void InitializationEvent();
>  
>    // SetDuration with no checks.
> +  void SetDuration(double aDuration, bool aSkipRangeRemoval);

Don't use a boolean parameter. Replace with a enum with a name indicating range removal/no range removal. This will allow you to skip the comments in all the calls documenting the intent.

::: dom/media/mediasource/MediaSourceDecoder.h
@@ +55,5 @@
>    void Ended();
>    bool IsExpectingMoreData() MOZ_OVERRIDE;
>  
>    void SetDecodedDuration(int64_t aDuration);
> +  void SetMediaSourceDuration(double aDuration, bool aSkipRangeRemoval);

Same as my comment in MediaSource.h regarding using an enum.

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: Do not call Range Removal algorithm after endOfStream

Carrying r+ changes

Comment 7

[Jean-Yves Avenard [:jya]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/1532023f986d

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1532023f986d

Comment 9

[Ralph Giles (:rillian)]

Comment on attachment 8547469 [details] [diff] [review]
Do not call Range Removal algorithm after endOfStream

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Less consistent testing, sites more likely to serve flash video.
[Describe test coverage new/current, TBPL]: Landed on m-c.
[Risks and why]: Low, MSE-specific.
[String/UUID change made/needed]: None.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b967fc6c3ad9
https://hg.mozilla.org/releases/mozilla-beta/rev/2a36e0243edd