sourcebuffer.abort() shouldn't abort range removal algorithm

RESOLVED FIXED in Firefox 37

Status

()

P2
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

(Blocks: 1 bug)

Trunk
mozilla38
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox37 fixed, firefox38 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
As per W3C spec:
http://w3c.github.io/media-source/#widl-SourceBuffer-abort-void

"If the updating attribute equals true, then run the following steps:

    Abort the buffer append and stream append loop algorithms if they are running.
    Set the updating attribute to false.
    Queue a task to fire a simple event named abort at this SourceBuffer object.
    Queue a task to fire a simple event named updateend at this SourceBuffer object.

"

Right now, abort will abort the current buffer append as per spec; and set the updating attribute to false.

When the queued range removal gets to run, it checks if the updating attribute is true ; if not it aborts.

This is the cause for failing YouTube MSE conformance test 42 (bug 1116651).

Now, I'm rather confused at what events should be fired once abort() is run.

Right now, we always have a matching number of updatestart and updateend , even if aborted.

If we do as per spec as I'm reading them, we would have:
updatestart (caused by range removal algorithm starting), then abort / updateend (due to abort(), and then update/updatend (once range removal gets to complete)
(Assignee)

Comment 1

4 years ago
Something rather inconsistent:
http://w3c.github.io/media-source/#sourcebuffer-events

states:
"The append or remove was aborted by an abort() call. updating transitions from true to false."

which indicates that remove will be aborted by abort.
Now range removal algorithm can be started by modifying the mediasource.duration attribute.

Would this mean that the remove() function can be aborted, but changing the duration won't ?

Actually, upon re-reading the spec, I think we got it wrong: changing the duration currently immediately change the duration of the mediasource object; however this should only queue the range removal algorithm with the new duration ; and this is what will set the duration later ; and as such can be aborted.
Flags: needinfo?(karlt)
(Assignee)

Comment 2

4 years ago
Further to comment 2, actually we are supposed to immediately change the duration and that the range removal algorithm is to be run (and that's asynchronous)..

So back to the original problem on what events to fired...
Summary: sourcebuffer.abort() shouldn't abort range removal algorithm → Changing mediasource.duration attribute should
(Assignee)

Comment 3

4 years ago
http://people.mozilla.org/~jyavenard/tests/mse_mp4/1130826.html

This adds 10 x 1s segment ; and then set the mediasource object's duration to 5s ; and immediately call sourceBuffer.abort()

Chrome issues:
updatestart
abort
updateend

and the sourceBuffer.buffered appears as { 0, 5 }, and mediaSource.duration = 5

Nightly shows that buffered range is still { 0, 10.03 }, and mediaSource.duration = 5

IE: Doesn't fire the abort() event but only updatestart/update/updatend, and the buffered range is {0,5ish} and mediaSource.duration = 5

IE behaviour seems more correct, though none of those behaviour are right according to the spec.
(Assignee)

Updated

4 years ago
Summary: Changing mediasource.duration attribute should → sourcebuffer.abort() shouldn't abort range removal algorithm
(Assignee)

Updated

4 years ago
Blocks: 1116651
(Assignee)

Comment 5

4 years ago
Created attachment 8560988 [details] [diff] [review]
Run range removal algorithm when setting mediasource duration

Only run range removal asynchronously when called following sourcebuffer.remove() ; when run following a change to mediasource.duration, we always run it, so data is properly removed. Events fired followed the rules of the last command run (e.g. if abort(): abort/updateend, otherwise update/updateend)
Attachment #8560988 - Flags: review?(cajbir.bugzilla)
(Assignee)

Updated

4 years ago
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED

Updated

4 years ago
Attachment #8560988 - Flags: review?(cajbir.bugzilla) → review+
(In reply to Jean-Yves Avenard [:jya] from comment #4)
> Lodged a bug at W3C:

Thanks for filing this.  abort() is useful for resetting the segment parser if the client decides to start appending a different stream.  In this situation, aborting an append currently in progress seems more reasonable than aborting a remove.  However, perhaps the only *need* for aborting an algorithm might be to allow subsequent appends without waiting for updateend, and that situation is similar with removes.

I also filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=27982 about asynchronicity in the duration change algorithm when reducing duration.
Flags: needinfo?(karlt)

Updated

4 years ago
Priority: -- → P2
https://hg.mozilla.org/mozilla-central/rev/5adf82c926e3
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Comment on attachment 8560988 [details] [diff] [review]
Run range removal algorithm when setting mediasource duration

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Spec compliance, less consistent testing. Harder to port later fixes.
[Describe test coverage new/current, TreeHerder]: Landed on m-c.
[Risks and why]: MSE-specific so low.
[String/UUID change made/needed]: None.
Attachment #8560988 - Flags: approval-mozilla-aurora?
status-firefox37: --- → affected
Pushed to aurora with pre-approval from lmandel.

https://hg.mozilla.org/releases/mozilla-aurora/rev/d8e630336213
status-firefox37: affected → fixed
Comment on attachment 8560988 [details] [diff] [review]
Run range removal algorithm when setting mediasource duration

As stated, this bug was pre-approved to land with a set of changes for MSE. Marking the approval after the fact.
Attachment #8560988 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.