1122358 - Implement Reset Parser State

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

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.

"

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: Implement partial reset parser

Reset current parser when abort() is done on an incomplete init segment. This partially implement the reset parser state algorithm. Long way to go

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Implement partial reset parser

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.

Comment 3

[cajbir (:cajbir)]

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.

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: Implement partial reset parser

carrying r+

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: Implement partial reset parser

rebase

Comment 6

[Jean-Yves Avenard [:jya]]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=15a0aaae5dd7

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/22fb2f111021

Comment 8

[Jean-Yves Avenard [:jya]]

uplift ?

Comment 9

[Ralph Giles (:rillian)]

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.

Comment 10

[Lawrence Mandel [:lmandel] (use needinfo)]

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+

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 12

[Ralph Giles (:rillian)]

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.

Comment 13

[Syd Polk :sydpolk]

Is there a test that shows before and after for this?

Comment 14

[Jean-Yves Avenard [:jya]]

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

Comment 15

[Ralph Giles (:rillian)]

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.