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)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla38
People
(Reporter: jya, Assigned: jya)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
8.19 KB,
patch
|
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
Is this related to the discussion in bug 1065215? In particular bug 1065215 comment 8 and on.
Assignee | ||
Comment 2•10 years ago
|
||
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".
Assignee | ||
Comment 3•10 years ago
|
||
(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.
Assignee | ||
Comment 4•10 years ago
|
||
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 | ||
Updated•10 years ago
|
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
Comment 5•10 years ago
|
||
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+
Assignee | ||
Comment 6•10 years ago
|
||
Carrying r+ changes
Assignee | ||
Updated•10 years ago
|
Attachment #8547238 -
Attachment is obsolete: true
Updated•10 years ago
|
Priority: -- → P2
Assignee | ||
Comment 7•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Comment 9•10 years ago
|
||
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?
Updated•10 years ago
|
Updated•10 years ago
|
Attachment #8547469 -
Flags: approval-mozilla-beta?
Attachment #8547469 -
Flags: approval-mozilla-beta+
Attachment #8547469 -
Flags: approval-mozilla-aurora?
Attachment #8547469 -
Flags: approval-mozilla-aurora+
Comment 10•10 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•