1185972 - Seeking won't start until the first frame is decoded.

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jean-Yves Avenard [:jya]]

This causes MSE Live Stream with the DASH-IF player to never start.

They append the init segment, and then seek to the live edge time.

Upon receiving the "seeking" event, they start appending new data.

Unfortunately, our MDSM will not start the seeking machinery until we have completed the DECODER_STATE_DECODING_FIRSTFRAME state.

It's a catch-22.

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

MSE always assumes zero start time. We should be able to skip DECODER_STATE_DECODING_FIRSTFRAME for MDSM when start time is already there.

Comment 2

[Jean-Yves Avenard [:jya]]

yes, was thinking along the same line.

We would still run through DECODER_STATE_DECODING_FIRSTFRAME thanks to the StartTimeRendezVous machinery.
And we want to run it anyway, so loadeddata is properly fired.

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

By skip I mean something like:
https://hg.mozilla.org/mozilla-central/file/e7e69cc8c07b/dom/media/MediaDecoderStateMachine.cpp#l2007

Doesn't IsRealTime() return true for a MSE Live Stream?

Comment 4

[Jean-Yves Avenard [:jya]]

The issue isn't just for MSE Live Stream, it just happens that's what the DASH-IF player does for those.

Ultimately, there's nothing to tell you MSE is a live stream or not.

If we simply skip decoding the first frame, we also have the issue that HE-AAC decoding will now be broken, as the decoding would update the sampling rate and we would miss it.

We really need to decode that first frame. Just start seeking earlier.

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Jean-Yves Avenard [:jya] from comment #4)
> If we simply skip decoding the first frame, we also have the issue that
> HE-AAC decoding will now be broken, as the decoding would update the
> sampling rate and we would miss it.
Do you mean the sampling rate could change along decoding to a different value from the metadata?

Comment 6

[Jean-Yves Avenard [:jya]]

Yes...

Typically with a HE-AAC streams, on the AAC-LC is indicated in the metadata.
Upon decoding, the SBR is found which ends up returning a sampling rate that is twice as much as what the metadata indicate.

This is the purpose of MediaDecoderReader::ReadUpdatedMetadata()

after decoding the first frame, we re-read the metadata, and only then set up the audio sink.

Comment 7

[Jean-Yves Avenard [:jya]]

Attachment: P1. Remove DECODER_STATE_DECODING_FIRSTFRAME state.

Instead use mSentFirstFrameLoadedEvent to differentiate the DECODER_STATE_DECODING state.

Comment 8

[Jean-Yves Avenard [:jya]]

Attachment: P2. Start seeking as soon as we can.

Also disambiguate conditions on when we can seek in a media.
The requirement isn't that we have decoded the first frame, but that we have a duration and start time.
This also fix the issue on firing the loadeddata event upon decoding the first frame following a seek.

Comment 9

[Jean-Yves Avenard [:jya]]

Attachment: P3. Add mochitest testing new seek behaviour.

Test ensure that seeking starts right after metadata is processed if we have a known start time.

Comment 10

[Jean-Yves Avenard [:jya]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e566454f9e90

Comment 11

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8637606 [details] [diff] [review]
P1. Remove DECODER_STATE_DECODING_FIRSTFRAME state.

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +2125,5 @@
>    if (mQueuedSeek.Exists()) {
>      mPendingSeek.Steal(mQueuedSeek);
>      SetState(DECODER_STATE_SEEKING);
>      ScheduleStateMachine();
> +  } else if (mState == DECODER_STATE_DECODING) {

This is misleading. Why would we need to StartDecoding when the state is already DECODING? Please test |mState == DECODER_STATE_DECODING && !mSentFirstFrameLoadedEvent| and you might need to reorder the call to EnqueueFirstFrameLoadedEvent() above.

Comment 12

[Jean-Yves Avenard [:jya]]

(In reply to JW Wang [:jwwang] from comment #11)
> Comment on attachment 8637606 [details] [diff] [review]
> P1. Remove DECODER_STATE_DECODING_FIRSTFRAME state.
> 
> Review of attachment 8637606 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/MediaDecoderStateMachine.cpp
> @@ +2125,5 @@
> >    if (mQueuedSeek.Exists()) {
> >      mPendingSeek.Steal(mQueuedSeek);
> >      SetState(DECODER_STATE_SEEKING);
> >      ScheduleStateMachine();
> > +  } else if (mState == DECODER_STATE_DECODING) {
> 
> This is misleading. Why would we need to StartDecoding when the state is
> already DECODING? Please test |mState == DECODER_STATE_DECODING &&
> !mSentFirstFrameLoadedEvent| and you might need to reorder the call to
> EnqueueFirstFrameLoadedEvent() above.

Yes, it is...

However, my understanding was that back when this was first written, and the MDSM could be accessed from any threads), there was the possibility that Shutdown() was called. After all, why test for DECODER_STATE_DECODING_FIRSTFRAME when we're already in that state. The ambiguity was already there.

Having said that, this is fixed in patch #2.

Comment 13

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8637607 [details] [diff] [review]
P2. Start seeking as soon as we can.

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

The code is fine. However, I think we can apply aspect programming to solve the issues individually. In fact, we have 2 issues to solve here. The 1st one is we want to start seeking as soon as we have the start time which is also this bug tries to fix. The other is comment 6 where we need the first frames to handle changes in sampling rate. The 2nd issue can be solved without taking mState into account and independently from the 1st issue. This should make the logic simpler and more coherent.

::: dom/media/MediaDecoderStateMachine.cpp
@@ +1331,5 @@
> +  } else {
> +    // We start the process of decodign the first frame.
> +    if (IsRealTime()) {
> +      // We will re-enter StartDecoding() once FinishDecodeFirstFrame() completes.
> +      FinishDecodeFirstFrame();

I am uncomfortable that FinishDecodeFirstFrame will cause reentrance of StartDecoding which makes the code flow complicated. Can you refactor this out?

Comment 14

[Jean-Yves Avenard [:jya]]

(In reply to JW Wang [:jwwang] from comment #13)
> Comment on attachment 8637607 [details] [diff] [review]
> P2. Start seeking as soon as we can.
> 
> Review of attachment 8637607 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The code is fine. However, I think we can apply aspect programming to solve
> the issues individually. In fact, we have 2 issues to solve here. The 1st
> one is we want to start seeking as soon as we have the start time which is
> also this bug tries to fix. The other is comment 6 where we need the first
> frames to handle changes in sampling rate. The 2nd issue can be solved
> without taking mState into account and independently from the 1st issue.
> This should make the logic simpler and more coherent.

Would you want this patch split in two then?

If I was to simply seek early at the end of OnMetadataRead, then we wouldn't enter the first frame decoded logic and would never fire loadeddata event, so we do need the separate handling of the first frame.

Or maybe I first do the removal of the DecodeFirstFrame stuff ; and not perform the seek early. This would be by itself completely the same as what is there now.

> 
> ::: dom/media/MediaDecoderStateMachine.cpp
> @@ +1331,5 @@
> > +  } else {
> > +    // We start the process of decodign the first frame.
> > +    if (IsRealTime()) {
> > +      // We will re-enter StartDecoding() once FinishDecodeFirstFrame() completes.
> > +      FinishDecodeFirstFrame();
> 
> I am uncomfortable that FinishDecodeFirstFrame will cause reentrance of
> StartDecoding which makes the code flow complicated. Can you refactor this
> out?

I can make FinishDecodeFirstFrame not call StartDecoding() then. Which is something I've never felt comfortable with either: that it's up to OnMetadataRead to start decoding rather than MDSM::Run().

Comment 15

[Jean-Yves Avenard [:jya]]

I can't explain why, but try runs timeout consistently on test_played.html.

Can't reproduce it locally however. investigating

Comment 16

[JW Wang [:jwwang] [:jw_wang]]

Let's fix the current issue first and then you can file another bug to do the refactoring later.

Comment 17

[Jean-Yves Avenard [:jya]]

(In reply to JW Wang [:jwwang] from comment #16)
> Let's fix the current issue first and then you can file another bug to do
> the refactoring later.

You lost me.
Which refactoring? So that FinishDecodeFirstFrame doesn't call StartDecoding()
or to split the logic between seek early, and handle post-first frame decoding?

The assert on windows is due to coming out of dormant, this retriggers a AsyncReadMetadata which would normally call EnqueueDecodeFirstFrameTask.
However, now that we don't have DECODER_STATE_DECODING_FIRSTFRAME and instead rely on mSentFirstFrameLoadedEvent to be clear, coming out of dormant that flag is set...

I'm not sure the point of requeuing a "decode first frame" task then... not familiar enough with dormant task.

Got an idea?

Comment 18

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Jean-Yves Avenard [:jya] from comment #17)
> You lost me.
> Which refactoring? So that FinishDecodeFirstFrame doesn't call StartDecoding()
> or to split the logic between seek early, and handle post-first frame decoding?
The latter one.

Comment 19

[Jean-Yves Avenard [:jya]]

Attachment: P1. Remove DECODER_STATE_DECODING_FIRSTFRAME state.

We want to be able to process the steps following decoding of the first frame
independent of a state.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad7b9480c934

Comment 20

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8638380 [details] [diff] [review]
P1. Remove DECODER_STATE_DECODING_FIRSTFRAME state.

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

LGTM.

::: dom/media/MediaDecoderStateMachine.cpp
@@ +782,3 @@
>      case DECODER_STATE_BUFFERING:
>      case DECODER_STATE_DECODING: {
> +      if (mDecodingFirstFrame) {

Since DECODING_FIRSTFRAME becomes a sub-step of DECODING, we should only read mDecodingFirstFrame when mState is DECODER_STATE_DECODING. So the logic can stay the same.

@@ +1159,5 @@
>  
>    mState = aState;
>  
> +  if (mState != DECODER_STATE_DECODING) {
> +    mDecodingFirstFrame = false;

mDecodingFirstFrame will be a don't-care term when we are able to relate mDecodingFirstFrame to DECODER_STATE_DECODING. So we don't need to reset mDecodingFirstFrame when switch from DECODER_STATE_DECODING to other states.

@@ +1350,5 @@
>      ScheduleStateMachine();
>    } else if (mState == DECODER_STATE_WAIT_FOR_CDM &&
>               !mReader->IsWaitingOnCDMResource()) {
> +    SetState(DECODER_STATE_DECODING);
> +    mDecodingFirstFrame = true;

We can set mDecodingFirstFrame to true when switching to DECODER_STATE_DECODING. It makes the code more coherent.

@@ +1989,5 @@
>      return;
>    }
>  
> +  SetState(DECODER_STATE_DECODING);
> +  mDecodingFirstFrame = true;

Ditto.

@@ +2088,5 @@
>  nsresult
>  MediaDecoderStateMachine::FinishDecodeFirstFrame()
>  {
>    MOZ_ASSERT(OnTaskQueue());
> +  MOZ_ASSERT(mState == DECODER_STATE_DECODING && mDecodingFirstFrame);

This will not necessary hold since decoding frames is async and state could change in between.

@@ -2093,4 @@
>    AssertCurrentThreadInMonitor();
>    DECODER_LOG("FinishDecodeFirstFrame");
>  
> -  if (IsShutdown()) {

Don't we need to bail out early when shutting down?

@@ +2130,5 @@
>    if (mQueuedSeek.Exists()) {
>      mPendingSeek.Steal(mQueuedSeek);
>      SetState(DECODER_STATE_SEEKING);
>      ScheduleStateMachine();
> +  } else {

Don't we check |mState == DECODER_STATE_DECODING && mDecodingFirstFrame| which is equivalent to DECODER_STATE_DECODING_FIRSTFRAME?

@@ +2885,5 @@
>  
>    MediaDecoderOwner::NextFrameStatus status;
>    const char* statusString;
> +  if (mState <= DECODER_STATE_WAIT_FOR_CDM ||
> +      (mState == DECODER_STATE_DECODING && mDecodingFirstFrame)) {

|mState == DECODER_STATE_DECODING && mDecodingFirstFrame| appears many times. Extract them to a helper function to present a high level idea.

Comment 21

[Jean-Yves Avenard [:jya]]

(In reply to JW Wang [:jwwang] from comment #20)
> Comment on attachment 8638380 [details] [diff] [review]
> P1. Remove DECODER_STATE_DECODING_FIRSTFRAME state.
> 
> Review of attachment 8638380 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM.
> 
> ::: dom/media/MediaDecoderStateMachine.cpp
> @@ +782,3 @@
> >      case DECODER_STATE_BUFFERING:
> >      case DECODER_STATE_DECODING: {
> > +      if (mDecodingFirstFrame) {
> 
> Since DECODING_FIRSTFRAME becomes a sub-step of DECODING, we should only
> read mDecodingFirstFrame when mState is DECODER_STATE_DECODING. So the logic
> can stay the same.

good catch. though we currently can't have  mDecodingFirstFrame = true while mState == DECODER_STATE_BUFFERING

> @@ +1159,5 @@
> >  
> >    mState = aState;
> >  
> > +  if (mState != DECODER_STATE_DECODING) {
> > +    mDecodingFirstFrame = false;
> 
> mDecodingFirstFrame will be a don't-care term when we are able to relate
> mDecodingFirstFrame to DECODER_STATE_DECODING. So we don't need to reset
> mDecodingFirstFrame when switch from DECODER_STATE_DECODING to other states.

I agree, and this will be removed in P2 (currently waiting on try)
However, to be identical in behaviour at before, changing the state would have clerared DECODER_STATE_FIRSTFRAME ; it I felt better that way being 100% sure there can't be unwanted regression

> 
> @@ +1350,5 @@
> >      ScheduleStateMachine();
> >    } else if (mState == DECODER_STATE_WAIT_FOR_CDM &&
> >               !mReader->IsWaitingOnCDMResource()) {
> > +    SetState(DECODER_STATE_DECODING);
> > +    mDecodingFirstFrame = true;
> 
> We can set mDecodingFirstFrame to true when switching to
> DECODER_STATE_DECODING. It makes the code more coherent.
You mean do
mDecodingFirstFrame = true;
SetState(DECODER_STATE_DECODING);
instead?

how is that more coherent?

> >  nsresult
> >  MediaDecoderStateMachine::FinishDecodeFirstFrame()
> >  {
> >    MOZ_ASSERT(OnTaskQueue());
> > +  MOZ_ASSERT(mState == DECODER_STATE_DECODING && mDecodingFirstFrame);
> 
> This will not necessary hold since decoding frames is async and state could
> change in between.
FinishDecodeFirstFrame can only be called from MaybeFinishDecodeFirstFrame() which is always when mState == DECODER_STATE_DECODING.

That expression can only ever be true unless there's a logic error.

> 
> @@ -2093,4 @@
> >    AssertCurrentThreadInMonitor();
> >    DECODER_LOG("FinishDecodeFirstFrame");
> >  
> > -  if (IsShutdown()) {
> 
> Don't we need to bail out early when shutting down?

That's no longer possible now that everything runs on the MDSM task queue.
If we are shutdown, then mSTATE != DECODER_STATE_DECODING, so we can't ever get there.

> 
> @@ +2130,5 @@
> >    if (mQueuedSeek.Exists()) {
> >      mPendingSeek.Steal(mQueuedSeek);
> >      SetState(DECODER_STATE_SEEKING);
> >      ScheduleStateMachine();
> > +  } else {
> 
> Don't we check |mState == DECODER_STATE_DECODING && mDecodingFirstFrame|
> which is equivalent to DECODER_STATE_DECODING_FIRSTFRAME?

Yes, however we already have asserted earlier that mState == DECODER_STATE_DECODING && mDecodingFirstFrame

All of this is from old time, that code was no longer relevant

> >    MediaDecoderOwner::NextFrameStatus status;
> >    const char* statusString;
> > +  if (mState <= DECODER_STATE_WAIT_FOR_CDM ||
> > +      (mState == DECODER_STATE_DECODING && mDecodingFirstFrame)) {
> 
> |mState == DECODER_STATE_DECODING && mDecodingFirstFrame| appears many
> times. Extract them to a helper function to present a high level idea.

indeed.

Note that P2 rework most of those...

my aim here was to be 100% compatible with the earlier code.

Comment 22

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Jean-Yves Avenard [:jya] from comment #21)
> You mean do
> mDecodingFirstFrame = true;
> SetState(DECODER_STATE_DECODING);
> instead?
> 
> how is that more coherent?

I mean:
MediaDecoderStateMachine::SetState(State aState)
{
  if (mState == DECODER_STATE_DECODING) {
    mDecodingFirstFrame = true;
  }
}

This would couple mDecodingFirstFrame with |mState == DECODER_STATE_DECODING| more tightly.

Comment 23

[Jean-Yves Avenard [:jya]]

not sure you would want that.

SetState(DECODER_STATE_DECODING) can be called where we don't want to reset mDecodingFirstFrame, like coming out of buffering mode etc

Comment 24

[JW Wang [:jwwang] [:jw_wang]]

I see. So mDecodingFirstFrame is only set to true when mState is changing from DECODER_STATE_DECODING_METADATA to DECODER_STATE_DECODING. Not when all other states changes to DECODER_STATE_DECODING.

Comment 25

[JW Wang [:jwwang] [:jw_wang]]

Or from DECODER_STATE_WAIT_FOR_CDM to DECODER_STATE_DECODING...

Comment 26

[Jean-Yves Avenard [:jya]]

Attachment: P1. Remove DECODER_STATE_DECODING_FIRSTFRAME state.

We want to be able to process the steps following decoding of the first frame
independent of a state.

Carrying r+. Use IsDecodingFirstFrame thorough code, limiting number of differences greatly

Comment 27

[Jean-Yves Avenard [:jya]]

Attachment: P1. Remove DECODER_STATE_DECODING_FIRSTFRAME state.

We want to be able to process the steps following decoding of the first frame
independent of a state.

Comment 28

[Jean-Yves Avenard [:jya]]

Attachment: P2. Refactor handling of first frame decoded.

Comment 29

[Jean-Yves Avenard [:jya]]

Attachment: P3. Don't reduce our buffer threshold coming out of dormant mode.

Each time we would come out of dormant mode, the buffer threshold for audio-only stream would be reduced.

Comment 30

[Jean-Yves Avenard [:jya]]

Attachment: P4. Initiate seek as early as conditions permit.

We can initiate a seek as soon as we have a start time and metadata have been read.
However, only enable this feature for MSE as some tests expect events in a specific order.

Comment 31

[Jean-Yves Avenard [:jya]]

Attachment: P5. Add mochitest testing new seek behaviour.

Test ensures that seeking starts right after metadata is processed if we have a known start time.

Comment 32

[Jean-Yves Avenard [:jya]]

Attachment: P6. Ensure exiting dormant mode has completed seek before notifying decoder.

Comment 33

[Jean-Yves Avenard [:jya]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3677d698023

This also includes bug 1186718 and bug 1185886.

Comment 34

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8639071 [details] [diff] [review]
P2. Refactor handling of first frame decoded.

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +827,2 @@
>        (IsVideoDecoding() && VideoQueue().GetSize() == 0)) {
> +    return false;

The logic seems wrong to me. It will return false when IsDecodingFirstFrame() is true and there is nothing in audio/video queues. So MaybeFinishDecodeFirstFrame() returns false at #574 and it goes on to #577.

It should not go on to #577 since there is nothing in the queue and decoding-first-frame is not finished.

I would suggest:
1. Leave checking mQueuedSeek.Exists() in FinishDecodeFirstFrame(). Checking mQueuedSeek.Exists() always appears right after calls to FinishDecodeFirstFrame() in your patch. It is a sign that these 2 things are strongly related (coherent).
2. Leave #574, #785, and #863 as-is. The original code is clearer about its intention and you won't have to check IsDecodingFirstFrame() at #896.

@@ +1332,5 @@
> +    // We're resuming from dormant state, so we don't need to request
> +    // the first samples in order to determine the media start time,
> +    // we have the start time from last time we loaded.
> +    FinishDecodeFirstFrame();
> +    if (mQueuedSeek.Exists()) {

suggestion cont.
3. As checking mQueuedSeek.Exists() is moved into FinishDecodeFirstFrame(), you just need to check |mState==DECODER_STATE_DECODING| to decide whether to early return from StartDecoding().

@@ -2098,5 @@
>    MOZ_ASSERT(OnTaskQueue());
>    AssertCurrentThreadInMonitor();
>    DECODER_LOG("FinishDecodeFirstFrame");
>  
> -  if (IsShutdown()) {

Since this check is removed, assert this function is only called when DECODING or BUFFERING.

@@ -2143,5 @@
> -    ScheduleStateMachine();
> -  } else if (IsDecodingFirstFrame()) {
> -    // StartDecoding() will also check if decode is completed.
> -    StartDecoding();
> -  }

Keep |if (mQueuedSeek.Exists())| for the suggestions above.
Remove |else if (IsDecodingFirstFrame())| we are already in DECODING.

Comment 35

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8639072 [details] [diff] [review]
P3. Don't reduce our buffer threshold coming out of dormant mode.

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +2071,5 @@
>    DECODER_LOG("Media duration %lld, "
>                "transportSeekable=%d, mediaSeekable=%d",
>                Duration().ToMicroseconds(), mDecoder->IsTransportSeekable(), mDecoder->IsMediaSeekable());
>  
> +  if (HasAudio() && !HasVideo() && !mSentFirstFrameLoadedEvent) {

It is kinda unclear to relate buffer threshold to mSentFirstFrameLoadedEvent. Can we do early return when mSentFirstFrameLoadedEvent is true to avoid mAmpleAudioThresholdUsecs is modified more than once?

Comment 36

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8639075 [details] [diff] [review]
P6. Ensure exiting dormant mode has completed seek before notifying decoder.

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +1336,5 @@
> +      // we have the start time from last time we loaded.
> +      // FinishDecodeFirstFrame will be launched upon completion of the seek when
> +      // we have data ready to play.
> +      MOZ_ASSERT(mQueuedSeek.Exists() && mSentFirstFrameLoadedEvent,
> +                 "Return from dormant must have queued seek");

Will dormant never happen before the 1st frames are decoded?

Comment 37

[Jean-Yves Avenard [:jya]]

Comment on attachment 8639071 [details] [diff] [review]
P2. Refactor handling of first frame decoded.

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +827,2 @@
>        (IsVideoDecoding() && VideoQueue().GetSize() == 0)) {
> +    return false;

the logic is identical to what was there before in that
we had for every call to MaybeFinishDecodeFirstFrame:

if (IsDecodingFirstFrame()) { MaybeFinishDecodeFirstFrame(); }

I simply moved the test within MaybeFinishDecodeFirstFrame.

*However*, the first frame handling is now no different to any other frames, and I don't see why it should be any different. We only want to abort the process if we have seeked which really has interrupted decoding and can only happen once we have a start time.

"It should not go on to #577 since there is nothing in the queue and decoding-first-frame is not finished." but there is something in the queue ! We are in either OnAudioDecoder or OnVideoDecoder and we have just pushed either a video or audio frame to the queue.
The only thing we don't know at this stage is if we have both audio *and* video and we can start the finish first frame logic.

To me this is now much clearer that it used to be.

The only exception is in OnNotDecoded where we check that suddenly both audio and video are empty, and for some reasons we still want to fire loadeddata.

BTW, I believe this is wrong too. We fire loadeddata if the metadata states that both audio and video are present, just the two streams are empty.
Chrome does the same; IE and Safari do not.

@@ +1332,5 @@
> +    // We're resuming from dormant state, so we don't need to request
> +    // the first samples in order to determine the media start time,
> +    // we have the start time from last time we loaded.
> +    FinishDecodeFirstFrame();
> +    if (mQueuedSeek.Exists()) {

I explicitly didn't want that. This is what I had done in my first implementation, but it was only make it more confusing with the follow up patches.

FinishDecodeFirstFrame job is now to only handle calling MetadataLoaded if it wasn't called before, and call FirstFrameDecoded().

@@ -2143,5 @@
> -    ScheduleStateMachine();
> -  } else if (IsDecodingFirstFrame()) {
> -    // StartDecoding() will also check if decode is completed.
> -    StartDecoding();
> -  }

this goes in the follow up patch.

Comment 38

[Jean-Yves Avenard [:jya]]

Comment on attachment 8639072 [details] [diff] [review]
P3. Don't reduce our buffer threshold coming out of dormant mode.

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +2071,5 @@
>    DECODER_LOG("Media duration %lld, "
>                "transportSeekable=%d, mediaSeekable=%d",
>                Duration().ToMicroseconds(), mDecoder->IsTransportSeekable(), mDecoder->IsMediaSeekable());
>  
> +  if (HasAudio() && !HasVideo() && !mSentFirstFrameLoadedEvent) {

but then you change the existing logic and that would prevent EnqueueFirstFrameLoadedEvent and EnqueueLoadedMetadataEvent to be called.

Calling EnqueueFirstFrameLoadedEvent is required to make dormant resume work.

I could swap things around instead... not sure that there's much to gain in clarity when doing so.
Can add a comment line

Comment 39

[Jean-Yves Avenard [:jya]]

(In reply to JW Wang [:jwwang] from comment #36)
> Comment on attachment 8639075 [details] [diff] [review]
> P6. Ensure exiting dormant mode has completed seek before notifying decoder.
> 
> Review of attachment 8639075 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/MediaDecoderStateMachine.cpp
> @@ +1336,5 @@
> > +      // we have the start time from last time we loaded.
> > +      // FinishDecodeFirstFrame will be launched upon completion of the seek when
> > +      // we have data ready to play.
> > +      MOZ_ASSERT(mQueuedSeek.Exists() && mSentFirstFrameLoadedEvent,
> > +                 "Return from dormant must have queued seek");
> 
> Will dormant never happen before the 1st frames are decoded?

that's right.

We must be in pause mode and already have started playback and have a current time (which is where we're going to seek to)

Comment 40

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8639071 [details] [diff] [review]
P2. Refactor handling of first frame decoded.

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +827,2 @@
>        (IsVideoDecoding() && VideoQueue().GetSize() == 0)) {
> +    return false;

The logic is different. Say we have both audio and video tracks and we have one sample in the audio queue and nothing in the video queue.

Before the patch, it will not go on to #578. After the patch, it will go on to #577. However, I think it doesn't really matter.

@@ +1332,5 @@
> +    // We're resuming from dormant state, so we don't need to request
> +    // the first samples in order to determine the media start time,
> +    // we have the start time from last time we loaded.
> +    FinishDecodeFirstFrame();
> +    if (mQueuedSeek.Exists()) {

OK. That is fine.

Comment 41

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a5306e6d457
https://hg.mozilla.org/integration/mozilla-inbound/rev/54241a963ada
https://hg.mozilla.org/integration/mozilla-inbound/rev/712cb240d405
https://hg.mozilla.org/integration/mozilla-inbound/rev/93a8a293b393
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9ad5b9c6324
https://hg.mozilla.org/integration/mozilla-inbound/rev/e192ba2bd045

Comment 42

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/27ae736ef960

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6a5306e6d457
https://hg.mozilla.org/mozilla-central/rev/54241a963ada
https://hg.mozilla.org/mozilla-central/rev/712cb240d405
https://hg.mozilla.org/mozilla-central/rev/93a8a293b393
https://hg.mozilla.org/mozilla-central/rev/e192ba2bd045

Comment 44

[Jean-Yves Avenard [:jya]]

Attachment: P5. Add mochitest testing new seek behaviour. r=jwwang

Test ensures that seeking starts right after metadata is processed if we have a known start time.

Comment 45

[Jean-Yves Avenard [:jya]]

Comment on attachment 8639074 [details] [diff] [review]
P5. Add mochitest testing new seek behaviour.

this test was racy and could miss the seeked event

Comment 46

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/db403f6e75f9

Comment 47

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/db403f6e75f9