Closed
Bug 1065215
Opened 10 years ago
Closed 10 years ago
MSE endOfStream() called within an 'updateend' event can fail with 'object no longer usable'
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: cajbir, Assigned: cajbir)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
Attachments
(3 files, 6 obsolete files)
1.19 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
856 bytes,
patch
|
karlt
:
review+
|
Details | Diff | Splinter Review |
18.06 KB,
patch
|
cajbir
:
review+
|
Details | Diff | Splinter Review |
If endOfStream() is called on a MediaSource in an 'updateend' event, an error 'object no longer usable' can occur depending on timing.
I discovered this with code like:
sb.addEventListener("updateend", function() {
updateCount++;
alert('updateend: ' + updateCount);
if (updateCount == 1) {
ms.endOfStream();
v.play();
}
}
This failed on the 'endOfStream' call with an object no longer usable error. If I took out the alert it worked. The error can also be demonstrated using a setTimeout instead of using the alert to delay things:
sb.addEventListener("updateend", function() {
updateCount++;
if (updateCount == 1) {
setTimeout(function() {
ms.endOfStream();
v.play();
}, 1000);
}
}
Testing in Chrome results in no error with both variants of the code.
Assignee | ||
Comment 1•10 years ago
|
||
Adds a mochitest which fails with the error.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → cajbir.bugzilla
Assignee | ||
Comment 2•10 years ago
|
||
The failure here is due to reaching SourceBuffer::Remove which raises an error if the MediaSource Ready State is not Open. At the time of failure it is Ended.
Assignee | ||
Comment 3•10 years ago
|
||
SourceBuffer::Remove is called during the DurationChange handling as a result of MediaSource::EndOfStream.
The spec outlines what needs to be done in SourceBuffer::Remove when the MediaSource is Ended here: http://www.w3.org/TR/media-source/#widl-SourceBuffer-remove-void-double-start-unrestricted-double-end. This should be addressed in bug 1068483.
SourceBuffer::Remove ends up being called because of the duration change removing data outside of the new duration range. The spec for EndOfStream states (http://www.w3.org/TR/media-source/#end-of-stream-algorithm):
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.
The duration change algorithm (http://www.w3.org/TR/media-source/#duration-change-algorithm) is what specifies the call to Remove:
If the new duration is less than old duration, then call remove(new duration, old duration) on all objects in sourceBuffers.
Assignee | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•10 years ago
|
||
Changing SourceBuffer::Remove so that it handles an ended media source per spec results in approximately 50 failures in the test_MediaSource test.
This is because at end of stream the highest buffered end time is 4.000 but the duration is 4.001. This causes SourceBuffer::Remove to reopen the stream which results in the tests for the ready state being "ended" to fail.
I'm investgating the cause for the duration discrepancy. In general though I'm unsure if test_MediaSource should be checking if the ready state is "ended" in the "souceended" event. Since the event is asynchronous it can be reopened before the event is processed. Something that bug 1068483 may have to deal with.
Assignee | ||
Comment 5•10 years ago
|
||
Duration discrepancy is due to the final frame duration in WebMBufferedParser::CalculateBufferedForRange being 34000000 whereis on other frames it is 33000000. This value is computed by subtracting the timecode of the previous frame from the current one. It is added to the timecode of the current frame to compute the end time of that frame. This results in the end time of the last buffered range being 1000000 higher.
Assignee | ||
Comment 6•10 years ago
|
||
Bug 1065207 is relevant here.
Assignee | ||
Comment 7•10 years ago
|
||
Fix here, as discussed in team standups, is to store the duration as a fuzzy value which includes the possible error in the time based on the container type. This is similar to the way the time is compared in the TrackBuffer.
A solution to the problem of not knowing the end frame duration time is still to be investigated in bug 1065207.
Test is included which fails before patch, succeeds after.
Attachment #8486911 -
Attachment is obsolete: true
Attachment #8509827 -
Flags: review?(karlt)
Comment 8•10 years ago
|
||
Thank you for the helpful details of what is happening here and the spec
references. The patch changes SourceBuffer::Remove() to comply with the spec.
My main comments on the patch would be that perhaps
GetHighestBufferedEndTime() should return something with the highest ended +
fuzz, and TimestampFuzz depends on the associated WebMBufferedParser, which
changes when new init segments are processed, suggesting that fuzz should be
more closely tracked with timestamps. However I'm wondering whether we can
avoid this complication.
When error is not set and duration has not been reached, the relevant parts of
the end of stream algorithm are essentially.
* Change the readyState attribute value to "ended", and dispatch sourceended.
* Change the readyState attribute value to "open", and dispatch sourceopen (duration change algorithm)
* Notify the media element that it now has all of the media data.
This doesn't make sense to me because the media element shouldn't be notified
that it now has all the media data if the MediaSource is "open".
I wonder whether there is a spec bug here.
Blink doesn't run the duration change algorithm here.
See the log of
http://people.mozilla.org/~karlt/mse/mediasource_demo.html?file=mediasource_test.webm&chunks=5&eos_chunk=3&eos_delay=-1&delay_chunk=1000&start=-1
readyState remains at ended until appendBuffer is called again.
Comment 9•10 years ago
|
||
Two possible ways of making the end of stream algorithm more sane might be:
1. Like Blink, don't run the entire duration change algorithm.
2. Change the duration change algorithm to only call remove on Sourcebuffers
when the new duration is less than the highest end time reported by the
buffered attribute across all SourceBuffer objects in sourceBuffers.
i.e. only when there is something to remove.
Option 1 might be better because it avoids some use of the fuzzy highest end time.
Comment 10•10 years ago
|
||
The spec bug is fixed in https://dvcs.w3.org/hg/html-media/rev/b3f086e62b53
by using a new range removal algorithm from duration-changed instead of doing all the steps that would be performed when calling remove().
https://dvcs.w3.org/hg/html-media/raw-file/default/media-source/media-source.html#duration-change-algorithm
Still I don't think the range removal algorithm does anything when the duration change algorithm is invoked from endOfStream().
Comment 11•10 years ago
|
||
(In reply to Karl Tomlinson (:karlt) from comment #10)
> Still I don't think the range removal algorithm does anything when the
> duration change algorithm is invoked from endOfStream().
Well, it does dispatch apparently pointless updatestart update and updateend events.
Comment 12•10 years ago
|
||
Comment on attachment 8509827 [details] [diff] [review]
Store duration in MediaSource as a time+possible error
With the spec clearer now, I think we want to go with a different approach
here, one that doesn't ever re-open on end of stream. That might either be
skipping much of the duration change algorithm, like Blink, or changing the duration change algorithm so as to run the range removal algorithm instead of calling remove(), as in the editors draft.
That will make the fuzz changes here unnecessary IIUC. If we do need some
fuzz, for other reasons, then I think that can be dealt with separately and
will probably be based on a larger fuzz like EOS_FUZZ_US.
>+ v.preload = "auto";
What does this do?
>+ v.addEventListener("ended", function () {
>+ v.parentNode.removeChild(v);
Is it necessary to remove the element from the document?
Attachment #8509827 -
Flags: review?(karlt) → review-
Comment 13•10 years ago
|
||
(In reply to Karl Tomlinson (:karlt) from comment #12)
> >+ v.preload = "auto";
>
> What does this do?
>
> >+ v.addEventListener("ended", function () {
> >+ v.parentNode.removeChild(v);
>
> Is it necessary to remove the element from the document?
Both of these are unnecessary. runWithMSE handles this already.
Assignee | ||
Comment 14•10 years ago
|
||
Addresses review comments by implementing the revised spec range removal step and calling that from the duration change code. This results in extra update{start,end} events so test are modified to allow for this.
The actual coded frame removal algorithm isn't implemented. There was a TODO for this and I've raised bug 1096089 for it so it doesn't get lost.
Attachment #8509827 -
Attachment is obsolete: true
Attachment #8519695 -
Flags: review?(karlt)
Assignee | ||
Comment 15•10 years ago
|
||
Comment 16•10 years ago
|
||
Comment on attachment 8519695 [details] [diff] [review]
Fix with review comments addressed
> Bug 1065215 - MSE endOfStream() called within an 'updateend' event can fail with 'object no longer usable' - r=karl
Please update the comment to say how the behavior of the code is changed.
There are two changes here: reopening in remove and not running all of remove in the duration change algorithm.
>+ // Runs the range removal steps from the MSE specification on each SourceBUffer.
Bu
Attachment #8519695 -
Flags: review?(karlt) → review+
Comment 17•10 years ago
|
||
Comment on attachment 8519695 [details] [diff] [review]
Fix with review comments addressed
> if (mUpdating || mMediaSource->ReadyState() != MediaSourceReadyState::Open) {
> aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
> return;
> }
>+ if (mMediaSource->ReadyState() == MediaSourceReadyState::Ended) {
>+ mMediaSource->SetReadyState(MediaSourceReadyState::Open);
>+ }
Actually this block is never reached because of the test above.
If you'd like to fix this in this patch then please remove the Open test.
Otherwise please leave out the Ended block.
Assignee | ||
Comment 18•10 years ago
|
||
Address additional review comments. I removed the open and suggested. r+ carried forward.
Attachment #8519695 -
Attachment is obsolete: true
Attachment #8519702 -
Flags: review+
Assignee | ||
Comment 19•10 years ago
|
||
Try build for updated patch: https://tbpl.mozilla.org/?tree=Try&rev=a19b34f11390
Assignee | ||
Comment 20•10 years ago
|
||
Fix for EME test failure. I have no idea if this is the right fix, but the test passes. Hopeing an EME expert can confirm if it is. This bug results in update{start,end} firing more than once and this fix attempts to allow for that.
Attachment #8519847 -
Flags: review?(cpearce)
Assignee | ||
Comment 21•10 years ago
|
||
Try run with EME fix: https://tbpl.mozilla.org/?tree=Try&rev=c763f379a89f
Assignee | ||
Comment 22•10 years ago
|
||
Updated patch to use media source readyState to check if ended and add a comment explaining why.
Attachment #8519847 -
Attachment is obsolete: true
Attachment #8519847 -
Flags: review?(cpearce)
Attachment #8520217 -
Flags: review?(cpearce)
Updated•10 years ago
|
Attachment #8520217 -
Flags: review?(cpearce) → review+
Assignee | ||
Comment 23•10 years ago
|
||
Try build for updated patch: https://tbpl.mozilla.org/?tree=Try&rev=05facb6502ba
Assignee | ||
Comment 24•10 years ago
|
||
Assignee | ||
Comment 25•10 years ago
|
||
Backed out for:
mediasource-remove.html | Test remove transitioning readyState from 'ended' to 'open'. - expected FAIL
https://hg.mozilla.org/integration/mozilla-inbound/rev/b538f9579103
https://hg.mozilla.org/integration/mozilla-inbound/rev/81d9177f109c
Assignee | ||
Comment 26•10 years ago
|
||
There is a web platform test for SourceBuffer.remove causing a transition from MediaSource.readyState ended=>open. This used to fail. With the patches in this bug it now passes. Change the metadata for the test so that it expects the pass.
Attachment #8521132 -
Flags: review?(karlt)
Assignee | ||
Comment 27•10 years ago
|
||
try build with web platform tests enabled: https://tbpl.mozilla.org/?tree=Try&rev=35f234214d58
Updated•10 years ago
|
Attachment #8521132 -
Flags: review?(karlt) → review+
Assignee | ||
Comment 28•10 years ago
|
||
Rebased on top of recent MSE landings. Try build: https://tbpl.mozilla.org/?tree=Try&rev=3197ed8b8d26
Attachment #8519702 -
Attachment is obsolete: true
Attachment #8521692 -
Flags: review+
Assignee | ||
Comment 29•10 years ago
|
||
Assignee | ||
Comment 30•10 years ago
|
||
Backed out for test failure on Android:
https://hg.mozilla.org/integration/mozilla-inbound/rev/23d179a16cd4
https://hg.mozilla.org/integration/mozilla-inbound/rev/d008dbf3e9f0
https://hg.mozilla.org/integration/mozilla-inbound/rev/722fca966383
INFO TEST-UNEXPECTED-FAIL | /tests/dom/media/mediasource/test/test_EndOfStream.html | Test timed out. - expected PASS
WARNING - TEST-UNEXPECTED-FAIL | /tests/dom/media/mediasource/test/test_WaitingOnMissingData.html | application timed out after 330 seconds with no output
WARNING - PROCESS-CRASH | /tests/dom/media/mediasource/test/test_WaitingOnMissingData.html | application crashed [@ libc.so + 0xcff0]
Assignee | ||
Comment 31•10 years ago
|
||
Discussed the Android timeout with team and decision made to disable it on that platform. Spun bug 1101187 to investigate/fix/enable. Carry r+ forward.
Attachment #8521692 -
Attachment is obsolete: true
Attachment #8524876 -
Flags: review+
Assignee | ||
Comment 32•10 years ago
|
||
Assignee | ||
Comment 33•10 years ago
|
||
Assignee | ||
Comment 34•10 years ago
|
||
Backed out for failure on Mulet:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ccae5be1b381
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3335fb1f0b8
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f9f48366319
36 INFO TEST-UNEXPECTED-FAIL | /tests/dom/media/mediasource/test/test_EndOfStream.html | Test timed out. - expected PASS
This was on the try run in comment 32 but I missed it due to being below the fold. I'll disable on Mulet, comment in bug 1101187 and reland.
Assignee | ||
Comment 35•10 years ago
|
||
Comment 36•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/dc01d68c1c7a
https://hg.mozilla.org/mozilla-central/rev/2be3cdd2ed7c
https://hg.mozilla.org/mozilla-central/rev/2beca8e05628
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•