Closed Bug 1120079 Opened 10 years ago Closed 10 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox36 --- fixed
firefox37 --- fixed
firefox38 --- fixed

People

(Reporter: jya, Assigned: jya)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.");"
Is this related to the discussion in bug 1065215? In particular bug 1065215 comment 8 and on.
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".
(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.
Do not call the Range Removal algorith after a call to endOfStream or when appendBuffer updated the duration
Attachment #8547238 - Flags: review?(cajbir.bugzilla)
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
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.
Attachment #8547238 - Flags: review?(cajbir.bugzilla) → review+
Attachment #8547238 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
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.
Attachment #8547469 - Flags: approval-mozilla-beta?
Attachment #8547469 - Flags: approval-mozilla-aurora?
Attachment #8547469 - Flags: approval-mozilla-beta?
Attachment #8547469 - Flags: approval-mozilla-beta+
Attachment #8547469 - Flags: approval-mozilla-aurora?
Attachment #8547469 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: