Closed Bug 1122358 Opened 9 years ago Closed 9 years ago

Implement Reset Parser State


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




Tracking Status
firefox36 --- unaffected
firefox37 --- fixed
firefox38 --- fixed


(Reporter: jya, Assigned: jya)


(Blocks 1 open bug)



(1 file, 3 obsolete files)

Spawned from bug 1120084.

From w3c spec:

"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.

Depends on: 1123198
Priority: -- → P2
Attached patch Implement partial reset parser (obsolete) — Splinter Review
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: nobody → jyavenard
Attached patch Implement partial reset parser (obsolete) — Splinter Review
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)
Attachment #8557585 - Attachment is obsolete: true
Attachment #8557585 - Flags: review?(cajbir.bugzilla)
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


::: 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+
Attached patch Implement partial reset parser (obsolete) — Splinter Review
carrying r+
Attachment #8557666 - Attachment is obsolete: true
Attachment #8557706 - Attachment is obsolete: true
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.

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.
Closed: 9 years ago
Keywords: leave-open
Resolution: --- → FIXED
Is there a test that shows before and after for this?
Flags: needinfo?(jyavenard)
(In reply to Syd Polk :sydpolk from comment #13)
> Is there a test that shows before and after for this?


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.