Status

()

defect
P2
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

(Blocks 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox36 unaffected, firefox37 fixed, firefox38 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Assignee

Description

4 years ago
Spawned from bug 1120084.

From w3c spec:
http://w3c.github.io/media-source/#sourcebuffer-append-error


"3.5.2 Reset Parser State

When the parser state needs to be reset, run the following steps:

    1.If the append state equals PARSING_MEDIA_SEGMENT and the input buffer contains some complete coded frames, then run the coded frame processing algorithm until all of these complete coded frames have been processed.
    2.Unset the last decode timestamp on all track buffers.
    3.Unset the last frame duration on all track buffers.
    4.Unset the highest presentation timestamp on all track buffers.
    5.Set the need random access point flag on all track buffers to true.
    6.Remove all bytes from the input buffer.
    7.Set append state to WAITING_FOR_SEGMENT.

"
Assignee

Updated

4 years ago
Depends on: 1123198

Updated

4 years ago
Priority: -- → P2
Assignee

Comment 1

4 years ago
Reset current parser when abort() is done on an incomplete init segment. This partially implement the reset parser state algorithm. Long way to go
Attachment #8557585 - Flags: review?(cajbir.bugzilla)
Assignee

Updated

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

Comment 2

4 years ago
Fixed a bug I noticed when we fail to initialize the decoder. It called TrackBuffer::DiscardDecoder() which would reject the promise with abort, when it should have been an error.
Attachment #8557666 - Flags: review?(cajbir.bugzilla)
Assignee

Updated

4 years ago
Attachment #8557585 - Attachment is obsolete: true
Attachment #8557585 - Flags: review?(cajbir.bugzilla)

Comment 3

4 years ago
Comment on attachment 8557666 [details] [diff] [review]
Implement partial reset parser

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

::: dom/media/mediasource/SourceBuffer.cpp
@@ +489,5 @@
>  SourceBuffer::AppendDataErrored(nsresult aError)
>  {
> +  switch (aError) {
> +    case NS_ERROR_ABORT:
> +      // Nothing further to do as then handling of the events is being done

s/then/the/

::: dom/media/mediasource/TrackBuffer.cpp
@@ +662,5 @@
>    MOZ_ASSERT(NS_IsMainThread());
>    if (mCurrentDecoder) {
>      DiscardDecoder();
>    }
> +  // Cancel the promise should the current decoder hadn't be initialized yet.

Please reword this so it is grammatically correct.
Attachment #8557666 - Flags: review?(cajbir.bugzilla) → review+
Assignee

Comment 4

4 years ago
carrying r+
Assignee

Updated

4 years ago
Attachment #8557666 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8557706 - Attachment is obsolete: true
Assignee

Comment 8

4 years ago
uplift ?
Flags: needinfo?(giles)
Depends on: 1125776
Flags: needinfo?(giles)
Comment on attachment 8558404 [details] [diff] [review]
Implement partial reset parser

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Spec compliance issue. Less consistent testing and difficulty backporting critical fixes.
[Describe test coverage new/current, TreeHerder]: Landed on m-c.
[Risks and why]: Very low. This only affects MSE code and has been working fine on Nightly.
[String/UUID change made/needed]: None.
Attachment #8558404 - Flags: approval-mozilla-aurora?
Comment on attachment 8558404 [details] [diff] [review]
Implement partial reset parser

Relatively small change that has been on m-c for ~2 weeks already. Aurora+
Attachment #8558404 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This was landed on Aurora.
https://hg.mozilla.org/releases/mozilla-aurora/rev/378dac73d66d

Not sure that setting the flags to fixed is right here, though.
Closing. Jean-Yves, it's easier to track uplifts if you use separate bugs for patches that land more than a day apart. If you want to have a tracking bug, that's fine, but please make it separate from patch bugs.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Keywords: leave-open
Resolution: --- → FIXED
Is there a test that shows before and after for this?

Updated

4 years ago
Flags: needinfo?(jyavenard)
Assignee

Comment 14

4 years ago
(In reply to Syd Polk :sydpolk from comment #13)
> Is there a test that shows before and after for this?

no..

it caused failure of some compliance YouTube tests.

WE haven't fully implemented the Reset Parser State algorithm. We currently only handle reset of partial init segment. We don't handle partial media segment in particular.

This is the reason why I wanted to keep the bug left open. And would have closed it once the implementation was complete.
Flags: needinfo?(jyavenard) → needinfo?(giles)
Again, it's fine to have tracking bugs, but it's best if all patches on a particular bug land together, and then the bug is closed to indicate the changes are on m-c. If patches from the same bug land in an ongoing manner, it's hard to use the binary flags we have in bugzilla to track the state of the bug, and it's easy to miss commits when doing uplift.

So open new bugs for each new episode of work after a previous bit has landed. If you know it's going to take a while, make all those block the original bug and don't land patches from there. In this case I suggest you open a new tracking bug, cut&paste the description from here, and make this one and subsequent work bugs block it.
Flags: needinfo?(giles)
You need to log in before you can comment on or make changes to this bug.