1366362 - seekToNextFrame is broken

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

dom/media/test/test_SeekToNextFrame.html shows decoding error happening.

The mochitest only works because we ignore decoding error and skip to the next keyframe

Looking at the log, we can see why:

GECKO(37570) | [MediaPlayback #1]: V/MediaFormatReader MediaFormatReader(0x117260000)::Update: No need for additional input (pending:0)
GECKO(37570) | [MediaPlayback #2]: V/MediaDecoder Decoder=0x115c89800 OnVideoDecoded [33000,67000]
GECKO(37570) | [MediaPlayback #2]: V/MediaDecoder Decoder=0x115c89800 UpdatePlaybackPositionInternal(33000)
GECKO(37570) | [MediaPlayback #2]: D/MediaDecoder Decoder=0x115c89800 state=SEEKING Seek completed, mCurrentPosition=33000
GECKO(37570) | [MediaPlayback #2]: D/MediaDecoder Decoder=0x115c89800 state=SEEKING change state to: DECODING
GECKO(37570) | [MediaPlayback #2]: D/MediaDecoder Decoder=0x115c89800 Changed mNextFrameStatus to NEXT_FRAME_AVAILABLE
GECKO(37570) | [MediaPlayback #2]: D/MediaDecoder Decoder=0x115c89800 state=DECODING change state to: DORMANT
GECKO(37570) | [MediaPlayback #2]: D/MediaDecoder Decoder=0x115c89800 state=DECODING Exiting DECODING, decoded for 0.000s
GECKO(37570) | [MediaPlayback #1]: V/MediaFormatReader MediaFormatReader(0x117260000)::ReleaseResources:
GECKO(37570) | [MediaPlayback #1]: V/MediaFormatReader MediaFormatReader(0x117260000)::ShutdownDecoder: Audio
GECKO(37570) | [MediaPlayback #1]: V/MediaFormatReader MediaFormatReader(0x117260000)::ShutdownDecoder: Video
GECKO(37570) | [MediaPlayback #1]: V/MediaFormatReader MediaFormatReader(0x117260000)::ScheduleUpdate: SchedulingUpdate(Video)
GECKO(37570) | [MediaPlayback #1]: V/MediaFormatReader MediaFormatReader(0x117260000)::Update: Processing update for Video
GECKO(37570) | [MediaPlayback #1]: V/MediaFormatReader MediaFormatReader(0x117260000)::Update: Update(Video) ni=0 no=0 in:0 out:0 qs=0 decoding:0 flushing:0 desc:shutdown pending:0 waiting:0 sid:4294967295
GECKO(37570) | [MediaPlayback #1]: V/MediaFormatReader MediaFormatReader(0x117260000)::Update: No need for additional input (pending:0)
8 INFO TEST-PASS | dom/media/test/test_seekToNextFrame.html | Should yet receive seeking event. 
9 INFO TEST-PASS | dom/media/test/test_seekToNextFrame.html | Should have already received seeking event. 
GECKO(37570) | [MediaPlayback #1]: D/MediaDecoder Decoder=0x115c89800 state=DORMANT Changed state to SEEKING (to 33000)
GECKO(37570) | [MediaPlayback #1]: D/MediaDecoder Decoder=0x115c89800 state=DORMANT change state to: SEEKING
GECKO(37570) | [MediaPlayback #1]: D/MediaDecoder Decoder=0x115c89800 StopPlayback()
GECKO(37570) | [MediaPlayback #1]: V/MediaDecoder Decoder=0x115c89800 UpdatePlaybackPositionInternal(33000)
GECKO(37570) | [MediaPlayback #1]: D/MediaDecoder Decoder=0x115c89800 Changed mNextFrameStatus to NEXT_FRAME_UNAVAILABLE_SEEKING
GECKO(37570) | [MediaPlayback #1]: V/MediaDecoder Decoder=0x115c89800 Queueing video task - queued=0, decoder-queued=0, skip=0, time=0
GECKO(37570) | [MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(0x117260000)::RequestVideoData: RequestVideoData(0, 0)
GECKO(37570) | [MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(0x117260000)::ScheduleUpdate: SchedulingUpdate(Video)
GECKO(37570) | [MediaPlayback #1]: V/MediaFormatReader MediaFormatReader(0x117260000)::Update: Processing update for Video
GECKO(37570) | [MediaPlayback #1]: V/MediaFormatReader MediaFormatReader(0x117260000)::Update: Update(Video) ni=1 no=1 in:0 out:0 qs=0 decoding:0 flushing:0 desc:shutdown pending:0 waiting:0 sid:4294967295
GECKO(37570) | [MediaPlayback #1]: V/MediaFormatReader MediaFormatReader(0x117260000)::RequestDemuxSamples: Requesting extra demux Video
GECKO(37570) | [MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(0x117260000)::OnVideoDemuxCompleted: 1 video samples demuxed (sid:0)
GECKO(37570) | [MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(0x117260000)::ScheduleUpdate: SchedulingUpdate(Video)
GECKO(37570) | [MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(0x117260000)::Update: Processing update for Video
GECKO(37570) | [MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(0x117260000)::Update: Update(Video) ni=1 no=1 in:0 out:0 qs=0 decoding:0 flushing:0 desc:shutdown pending:0 waiting:0 sid:4294967295
GECKO(37570) | [MediaPlayback #2]: D/PlatformDecoderModule Initialising FFmpeg decoder.
GECKO(37570) | [MediaPlayback #2]: D/PlatformDecoderModule FFmpeg init successful.
GECKO(37570) | [MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(0x117260000)::ScheduleUpdate: SchedulingUpdate(Video)
GECKO(37570) | [MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(0x117260000)::Update: Processing update for Video
GECKO(37570) | [MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(0x117260000)::Update: Update(Video) ni=1 no=1 in:0 out:0 qs=0 decoding:0 flushing:0 desc:ffvpx video decoder pending:0 waiting:0 sid:4294967295
GECKO(37570) | [MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(0x117260000)::HandleDemuxedSamples: Giving Video input to decoder
GECKO(37570) | [MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(0x117260000)::HandleDemuxedSamples: Input:100000 (dts:100000 kf:0)
GECKO(37570) | [vp9 @ 0x114a12000] Not all references are available
GECKO(37570) | [MediaPDecoder #1]: D/PlatformDecoderModule DoDecodeFrame:decode_video: rv=82 decoded=0 (Input: pts(100000) dts(100000) Output: pts(-9223372036854775808) opaque(0) pkt_pts(-9223372036854775808) pkt_dts(-9223372036854775808))
GECKO(37570) | [MediaPlayback #1]: D/MediaFormatReader MediaFormatReader(0x117260000)::NotifyNewOutput: Done processing new Video samples
GECKO(37570) | [MediaPlayback #1]: V/MediaFormatReader MediaFormatReader(0x117260000)::ScheduleUpdate: SchedulingUpdate(Video)
GECKO(37570) | [MediaPlayback #1]: V/MediaFormatReader MediaFormatReader(0x117260000)::Update: Processing update for Video
GECKO(37570) | [MediaPlayback #1]: V/MediaFormatReader MediaFormatReader(0x117260000)::Update: Update(Video) ni=1 no=1 in:1 out:0 qs=1 decoding:0 flushing:0 desc:ffvpx video decoder pending:0 waiting:0 sid:4294967295
GECKO(37570) | [MediaPlayback #1]: V/MediaFormatReader MediaFormatReader(0x117260000)::RequestDemuxSamples: Requesting extra demux Video
GECKO(37570) | [MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(0x117260000)::OnVideoDemuxCompleted: 1 video samples demuxed (sid:0)
GECKO(37570) | [MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(0x117260000)::ScheduleUpdate: SchedulingUpdate(Video)
GECKO(37570) | [MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(0x117260000)::Update: Processing update for Video
GECKO(37570) | [MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(0x117260000)::Update: Update(Video) ni=1 no=1 in:1 out:0 qs=1 decoding:0 flushing:0 desc:ffvpx video decoder pending:0 waiting:0 sid:4294967295
GECKO(37570) | [MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(0x117260000)::HandleDemuxedSamples: Giving Video input to decoder
GECKO(37570) | [MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(0x117260000)::HandleDemuxedSamples: Input:133000 (dts:133000 kf:0)
GECKO(37570) | [vp9 @ 0x114a16000] Not all references are available
GECKO(37570) | [MediaPDecoder #1]: D/PlatformDecoderModule DoDecodeFrame:decode_video: rv=-1094995529 decoded=0 (Input: pts(133000) dts(133000) Output: pts(-9223372036854775808) opaque(0) pkt_pts(-9223372036854775808) pkt_dts(-9223372036854775808))
GECKO(37570) | [37570] WARNING: NS_ERROR_DOM_MEDIA_DECODE_ERR (0x806e0004) - mozilla::MediaResult mozilla::FFmpegVideoDecoder<46465650>::DoDecode(mozilla::MediaRawData *, uint8_t *, int, bool *, MediaDataDecoder::DecodedData &): FFmpeg video error:-1094995529: file /Users/jyavenard/Work/Mozilla/mozilla-central/dom/media/MediaFormatReader.cpp, line 1783
GECKO(37570) | [MediaPlayback #1]: V/MediaFormatReader MediaFormatReader(0x117260000)::NotifyError: Video Decoding error

When seeking, the MDSM move from state DECODING to DORMANT, this cause MediaFormatReader::ReleaseResources to be called.

This will destroy the decoder.

The MDSM then request a new frame. This demux the next frame (which is not a keyframe as no reset or seek on the demuxer was performed).

Then decoder is then recreated and that non keyframe is passed to the decoder.
The decoder can't decode that non keyframe.

The MDSM much either:
- Not enter dormant mode
- Not call ReleaseResources
- Call MFR::ResetDecoder() followed by a MFR::Seek

This bug is yet another example on why decoding errors shouldn't be ignored during the mochitest (see bug 1364872)

Comment 1

[Jean-Yves Avenard [:jya]]

One of the consequence is that seekToNextFrame doesn't do what it's supposed to do, it skips to the next keyframe if any.

If there aren't future next keyframe, it could fail with a decoding error.

This is likely the reason on why we have intermittent error where seekToNextFrame is used (like bug 1356212, bug 1357484 described that seekToNextFrame did nothing, bug 1360452)

Comment 2

[Jean-Yves Avenard [:jya]]

The issue is the transition between state MediaDecoderStateMachine::DormantState to MediaDecoderStateMachine::SeekingState, it assumes that the Seeking state will do the right thing by resetting the MediaDecoderReader.

However, MediaDecoderStateMachine::NextFrameSeekingState doesn't perform such operation. It continues decoding regardless of the previous state; even if the previous state (DormantState) made the MediaDecoderReader unusable.

The transition out of DormantState shouldn't assume that the next state will reset things properly.

Comment 3

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

We can enforce a full seek when coming out of dormant.

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1366362: P1. Fix style.

Review commit: https://reviewboard.mozilla.org/r/141602/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/141602/

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1366362: P3. Have seekToNextFrame properly handle dormant mode.

When coming out of dormant mode, we first perform a video only accurate seek, that will ensure the MediaDecoderReader has been reinitialised correctly.

Review commit: https://reviewboard.mozilla.org/r/141604/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/141604/

Comment 6

[Jean-Yves Avenard [:jya]]

Comment on attachment 8870155 [details]
Bug 1366362: P3. Have seekToNextFrame properly handle dormant mode.

https://reviewboard.mozilla.org/r/141604/#review145266

It's a bit messy...

Some issues prevented easily chaining the promises / SeekingState object

1- Each SeekingState set the next StateObject via SetState<T>
2- We need to provide MDSM::Seek() with the promise that will be resolved once both SeekingState have completed.
3- Each SeekState can be cancelled / shutdown / errored at any time.

Making the SeekPromise non exclusive could have greatly simplified the code... But I felt it was too much for what should have been an easy handling.

Comment 7

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8870154 [details]
Bug 1366362: P1. Fix style.

https://reviewboard.mozilla.org/r/141602/#review145282

Comment 8

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8870155 [details]
Bug 1366362: P3. Have seekToNextFrame properly handle dormant mode.

https://reviewboard.mozilla.org/r/141604/#review145284

Fly-by comments on comments: (I did not review the code -- good luck JW!)

::: dom/media/MediaDecoderStateMachine.cpp:1613
(Diff revision 1)
>  
> +  void DoSeek() override
> +  {
> +    // We need to do the seek operation asynchronously. Because for a special
> +    // case (bug504613.ogv) which has no data at all, the 1st seekToNextFrame()
> +    // operation reaches to the end of the media. If we did the seek operation

Remove 'to'.

::: dom/media/MediaDecoderStateMachine.cpp:1613
(Diff revision 1)
> +    // operation reaches to the end of the media. If we did the seek operation
> +    // synchronously, we immediately resolve the SeekPromise in mSeekJob and

Grammar: 'If we *do* ...'. (Otherwise change the rest of the paragraph to the past tense.)

Comment 9

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

Comment on attachment 8870155 [details]
Bug 1366362: P3. Have seekToNextFrame properly handle dormant mode.

https://reviewboard.mozilla.org/r/141604/#review145318

::: dom/media/MediaDecoderStateMachine.cpp:1020
(Diff revision 1)
>    {
>      // We set mVideoDecodeSuspended to false in Enter().
>      MOZ_ASSERT(false, "Shouldn't have suspended video decoding.");
>    }
>  
> +  virtual void DoSeek() = 0;

DoSeek() is not part of the public interface and doesn't have to be public.

A private virtual function allows the sub-classes to customize the behavior and is effectively used in Template Method Pattern.

::: dom/media/MediaDecoderStateMachine.cpp:1727
(Diff revision 1)
> +    mAccurateSeek = MakeUnique<AccurateSeekingState>(mMaster);
> +    SeekTarget originalTarget = aSeekJob.mTarget.ref();
> +
> +    SeekJob seekJob(Move(aSeekJob));
> +    // Ensure that we don't transition to DecodingState once this seek complete.
> +    seekJob.mTransition = false;

This is not elegant but I don't have a better solution for now. :(

::: dom/media/MediaDecoderStateMachine.cpp:1729
(Diff revision 1)
> +
> +    SeekJob seekJob(Move(aSeekJob));
> +    // Ensure that we don't transition to DecodingState once this seek complete.
> +    seekJob.mTransition = false;
> +    seekJob.mTarget->SetType(SeekTarget::Accurate);
> +    seekJob.mTarget->SetVideoOnly(true);

We need a full seek since the user might want to play after doing some NextFrameSeeks.

::: dom/media/MediaDecoderStateMachine.cpp:1741
(Diff revision 1)
> +               mAccurateSeek->Exit();
> +               mAccurateSeek = nullptr;
> +               SeekJob seekJob;
> +               seekJob.mTransition = false;
> +               seekJob.mTarget = Some(originalTarget);
> +               mNextFrameSeek = MakeUnique<NextFrameSeekingState>(mMaster);

Just transition to NextFrameSeekingState and you can remove half of the code from this state.

Comment 10

[Jean-Yves Avenard [:jya]]

Comment on attachment 8870155 [details]
Bug 1366362: P3. Have seekToNextFrame properly handle dormant mode.

https://reviewboard.mozilla.org/r/141604/#review145318

> We need a full seek since the user might want to play after doing some NextFrameSeeks.

we are coming from dormant. the audio is continuing to play. we only want to resume the video no?
only then will seek continue

> Just transition to NextFrameSeekingState and you can remove half of the code from this state.

i had first attempted that. however doing SetState will delete this. as such, you can no longer resolve the original seek promise.

So calling SetState is not an option.

unless we make StateObject refcounted

Comment 11

[Jean-Yves Avenard [:jya]]

Comment on attachment 8870155 [details]
Bug 1366362: P3. Have seekToNextFrame properly handle dormant mode.

https://reviewboard.mozilla.org/r/141604/#review145318

> we are coming from dormant. the audio is continuing to play. we only want to resume the video no?
> only then will seek continue

that seek is only for coming out of dormant.

Comment 12

[Jean-Yves Avenard [:jya]]

Comment on attachment 8870155 [details]
Bug 1366362: P3. Have seekToNextFrame properly handle dormant mode.

https://reviewboard.mozilla.org/r/141604/#review145284

> Grammar: 'If we *do* ...'. (Otherwise change the rest of the paragraph to the past tense.)

that is a copy of earlier text above. need to fix both instances

Comment 13

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

Comment on attachment 8870155 [details]
Bug 1366362: P3. Have seekToNextFrame properly handle dormant mode.

https://reviewboard.mozilla.org/r/141604/#review145318

> that seek is only for coming out of dormant.

We enter only after playback is paused. So we need to resume both audio and video.

> i had first attempted that. however doing SetState will delete this. as such, you can no longer resolve the original seek promise.
> 
> So calling SetState is not an option.
> 
> unless we make StateObject refcounted

You can:
1. make NextFrameSeekingFromDormantState::mSeekPromise a MediaDecoder::SeekPromise::Private
2. Move mSeekPromise to a stack variable and chain it to the result of NextFrameSeekingState::Enter().

Comment 14

[Jean-Yves Avenard [:jya]]

Comment on attachment 8870155 [details]
Bug 1366362: P3. Have seekToNextFrame properly handle dormant mode.

https://reviewboard.mozilla.org/r/141604/#review145318

> We enter only after playback is paused. So we need to resume both audio and video.

but the audio decoder hasnt been paused / destroyed right? 

the audio can resume where it left off. that video seek only put us back where we were prior going to dormant

Comment 15

[Jean-Yves Avenard [:jya]]

Comment on attachment 8870155 [details]
Bug 1366362: P3. Have seekToNextFrame properly handle dormant mode.

https://reviewboard.mozilla.org/r/141604/#review145318

> You can:
> 1. make NextFrameSeekingFromDormantState::mSeekPromise a MediaDecoder::SeekPromise::Private
> 2. Move mSeekPromise to a stack variable and chain it to the result of NextFrameSeekingState::Enter().

we would still have to override every method to handle the accurate seek.

so i dont see the advantage of doing so.

feel free to take over if you want it done in a particular way.

Comment 16

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

Comment on attachment 8870155 [details]
Bug 1366362: P3. Have seekToNextFrame properly handle dormant mode.

https://reviewboard.mozilla.org/r/141604/#review145318

> but the audio decoder hasnt been paused / destroyed right? 
> 
> the audio can resume where it left off. that video seek only put us back where we were prior going to dormant

We delete both audio and video decoders when entering dormant. So we need to resume both decoders.

Comment 17

[Jean-Yves Avenard [:jya]]

Comment on attachment 8870155 [details]
Bug 1366362: P3. Have seekToNextFrame properly handle dormant mode.

https://reviewboard.mozilla.org/r/141604/#review145318

> we would still have to override every method to handle the accurate seek.
> 
> so i dont see the advantage of doing so.
> 
> feel free to take over if you want it done in a particular way.

My apologies...

you were right... much easier that way..

thanks for showing me the light :)

Comment 18

[Jean-Yves Avenard [:jya]]

Comment on attachment 8870155 [details]
Bug 1366362: P3. Have seekToNextFrame properly handle dormant mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141604/diff/1-2/

Comment 19

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

Comment on attachment 8870155 [details]
Bug 1366362: P3. Have seekToNextFrame properly handle dormant mode.

https://reviewboard.mozilla.org/r/141604/#review145778

::: dom/media/MediaDecoderStateMachine.cpp:1020
(Diff revision 2)
>    {
>      // We set mVideoDecodeSuspended to false in Enter().
>      MOZ_ASSERT(false, "Shouldn't have suspended video decoding.");
>    }
>  
> +  virtual void DoSeek() = 0;

Can you put the changes that move functions to public in one patch, and NextFrameSeekingFromDormantState in another?

::: dom/media/MediaDecoderStateMachine.cpp:1719
(Diff revision 2)
> +    : SeekingState(aPtr)
> +  {
> +  }
> +
> +  RefPtr<MediaDecoder::SeekPromise> Enter(SeekJob&& aSeekJob,
> +                                          EventVisibility aVisibility)

Remove this parameter and assume EventVisibility::Observable. See the comment below to see why.

::: dom/media/MediaDecoderStateMachine.cpp:1729
(Diff revision 2)
> +
> +    SeekJob seekJob(Move(aSeekJob));
> +    // Ensure that we don't transition to DecodingState once this seek
> +    // completes.
> +    seekJob.mTransition = false;
> +    seekJob.mTarget->SetType(SeekTarget::Accurate);

You don't need this line if you pass DormantState::mPendingSeek to this function. See comments below.

::: dom/media/MediaDecoderStateMachine.cpp:1746
(Diff revision 2)
> +               SeekJob seekJob;
> +               seekJob.mTarget = Some(originalTarget);
> +               // SetState will delete this, so we can't use any members or
> +               // methods after its been called.
> +               RefPtr<TaskQueue> taskQueue = OwnerThread();
> +               SetState<NextFrameSeekingState>(Move(seekJob), aVisibility)

Change the type of mSeekPromise to MozPromiseHolder<MediaDecoder::SeekPromise>. (This differs from my previous suggestion, sorry.)

Then you will be able to simply do:

SeekJob seekJob;
seekJob.mTarget = ...;
seekJob.mPromise = Move(mSeekPromise);
SetState<NextFrameSeekingState>(Move(seekJob), EventVisibility::Observable);

without doing ->Then(...).

::: dom/media/MediaDecoderStateMachine.cpp:1753
(Diff revision 2)
> +                        [p]() { p->Resolve(true, __func__); },
> +                        [p]() { p->Reject(true, __func__); });
> +             },
> +             [this]() {
> +               mAccurateSeekRequest.Complete();
> +               Exit();

You can't call Exit() here for it will be called again when exiting this state.

::: dom/media/MediaDecoderStateMachine.cpp:1776
(Diff revision 2)
> +    MOZ_ASSERT(mAccurateSeekRequest.Exists(), "enter must have been called");
> +    mAccurateSeek->DoSeek();
> +  }
> +
> +protected:
> +  TimeUnit CalculateNewCurrentTime() const override

You can remove these functions by letting NextFrameSeekingFromDormantState inherit AccurateSeekingState.

::: dom/media/MediaDecoderStateMachine.cpp:1829
(Diff revision 2)
> +
> +RefPtr<MediaDecoder::SeekPromise>
> +MediaDecoderStateMachine::DormantState::HandleSeek(SeekTarget aTarget)
> +{
> +  if (aTarget.IsNextFrame()) {
> +    SLOG("Changed state to SEEKING (to %" PRId64 ")",

You've got one in NextFrameSeekingFromDormantState::Enter().  Remove this log message.

::: dom/media/MediaDecoderStateMachine.cpp:1833
(Diff revision 2)
> +  if (aTarget.IsNextFrame()) {
> +    SLOG("Changed state to SEEKING (to %" PRId64 ")",
> +        aTarget.GetTime().ToMicroseconds());
> +    SeekJob seekJob;
> +    seekJob.mTarget = Some(aTarget);
> +    return StateObject::SetState<NextFrameSeekingFromDormantState>(

You can do |SetState<NextFrameSeekingFromDormantState>(Move(mPendingSeek))| for mPendingSeek stores all the information about seeking to the original position before entering dormant.

Also, NextFrameSeekingFromDormantState should always assume EventVisibility::Observable since HandleSeek() is initiated by the user. Then you can remove the 2nd parameter of NextFrameSeekingFromDormantState::Enter().

Comment 20

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1366362: P2. Change access restrictions of members.

We try to make the definition in the same order as the base class and with the same access.

We make DoSeek protected as we'll need it in the next change.

Review commit: https://reviewboard.mozilla.org/r/142192/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/142192/

Comment 21

[Jean-Yves Avenard [:jya]]

Comment on attachment 8870155 [details]
Bug 1366362: P3. Have seekToNextFrame properly handle dormant mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141604/diff/2-3/

Comment 22

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

Comment on attachment 8870709 [details]
Bug 1366362: P2. Change access restrictions of members.

https://reviewboard.mozilla.org/r/142192/#review145846

Comment 23

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

Comment on attachment 8870155 [details]
Bug 1366362: P3. Have seekToNextFrame properly handle dormant mode.

https://reviewboard.mozilla.org/r/141604/#review145848

::: dom/media/MediaDecoderStateMachine.cpp:1719
(Diff revision 3)
> +  explicit NextFrameSeekingFromDormantState(Master* aPtr)
> +    : AccurateSeekingState(aPtr)
> +  {
> +  }
> +
> +  RefPtr<MediaDecoder::SeekPromise> Enter(SeekJob&& aPendingSeekJob,

Call them "aCurrentSeekJob" and "aFutureSeekJob".

::: dom/media/MediaDecoderStateMachine.cpp:1750
(Diff revision 3)
> +    mFutureSeekJob.RejectIfExists(__func__);
> +    AccurateSeekingState::Exit();
> +  }
> +
> +private:
> +  UniquePtr<AccurateSeekingState> mAccurateSeek;

Remove this unused member.

Comment 24

[Jean-Yves Avenard [:jya]]

Comment on attachment 8870155 [details]
Bug 1366362: P3. Have seekToNextFrame properly handle dormant mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141604/diff/3-4/

Comment 25

[Jean-Yves Avenard [:jya]]

Comment on attachment 8870155 [details]
Bug 1366362: P3. Have seekToNextFrame properly handle dormant mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141604/diff/4-5/

Comment 26

[Mozilla Autoland]

hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 3 changesets with 7 changes to 4 files
remote: 
remote: 
remote: ************************** ERROR ****************************
remote: Rev 292534902651 needs "Bug N" or "No bug" in the commit message.
remote: Jean-Yves Avenard <[e-mail]>
remote: 1366362: P3. Have seekToNextFrame properly handle dormant mode. r=jwwang
remote: 
remote: When coming out of dormant mode, we first perform an accurate seek, that will ensure the MediaDecoderReader has been reinitialised correctly.
remote: 
remote: MozReview-Commit-ID: 2rWgu9AGAcY
remote: *************************************************************
remote: 
remote: 
remote: 
remote: 
remote: ************************** ERROR ****************************
remote: Rev 08021dc9bddf needs "Bug N" or "No bug" in the commit message.
remote: Jean-Yves Avenard <[e-mail]>
remote: 1366362: P2. Change access restrictions of members. r=jwwang
remote: 
remote: We try to make the definition in the same order as the base class and with the same access.
remote: 
remote: We make DoSeek protected as we'll need it in the next change.
remote: 
remote: MozReview-Commit-ID: KAn1Jj3mAB7
remote: *************************************************************
remote: 
remote: 
remote: 
remote: 
remote: ************************** ERROR ****************************
remote: Rev a742708ddf7e needs "Bug N" or "No bug" in the commit message.
remote: Jean-Yves Avenard <[e-mail]>
remote: 1366362: P1. Fix style. r=gerald
remote: 
remote: MozReview-Commit-ID: 3eDT8kY1tCZ
remote: *************************************************************
remote: 
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.c_commitmessage hook failed
abort: push failed on remote

Comment 27

[Jean-Yves Avenard [:jya]]

Comment on attachment 8870154 [details]
Bug 1366362: P1. Fix style.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141602/diff/1-2/

Comment 28

[Jean-Yves Avenard [:jya]]

Comment on attachment 8870709 [details]
Bug 1366362: P2. Change access restrictions of members.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142192/diff/1-2/

Comment 29

[Jean-Yves Avenard [:jya]]

Comment on attachment 8870155 [details]
Bug 1366362: P3. Have seekToNextFrame properly handle dormant mode.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141604/diff/5-6/

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f415315d63be
P1. Fix style. r=gerald
https://hg.mozilla.org/integration/autoland/rev/82572291a255
P2. Change access restrictions of members. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/63fdec8b12fa
P3. Have seekToNextFrame properly handle dormant mode. r=jwwang

Comment 31

[Tzuhao Kuo [:kaku]]

Comment on attachment 8870155 [details]
Bug 1366362: P3. Have seekToNextFrame properly handle dormant mode.

https://reviewboard.mozilla.org/r/141604/#review145920

Sorry for adding comments while landing. If the comments are accepted, I can do it in another bug.

::: dom/media/MediaDecoderStateMachine.cpp:1737
(Diff revision 6)
> +             [this]() {
> +               mAccurateSeekRequest.Complete();
> +               SetState<NextFrameSeekingState>(Move(mFutureSeekJob),
> +                                               EventVisibility::Observable);
> +             },
> +             [this]() { mAccurateSeekRequest.Complete(); })

I think we need to call `mSeekJob.RejectIfExists()` here, or the SeekToNextFrame() might never be resolved/rejected and the APP hangs.

::: dom/media/MediaDecoderStateMachine.cpp:2526
(Diff revision 6)
> +  if (mSeekJob.mTransition) {
> -  SetState<DecodingState>();
> +    SetState<DecodingState>();
> -}
> +  }

How about replacing this logic with a virtual function, say `virtual void Transit();`, as a template method here. 

We can have a default behavior as
```
void SeekingState::Transit()
{
  SetState<DecodingState>();
}
```

and we can override it for the special behavior (no transition) for the NextFrameSeekingFromDormantState.

```
void NextFrameSeekingFromDormantState::Transit() { // No transition. }
```

In this way, we don't need to add the `SeekJob::mTransition` variable and keep the logics controled by state objects themselve.

Comment 32

[Jean-Yves Avenard [:jya]]

Comment on attachment 8870155 [details]
Bug 1366362: P3. Have seekToNextFrame properly handle dormant mode.

https://reviewboard.mozilla.org/r/141604/#review145920

> I think we need to call `mSeekJob.RejectIfExists()` here, or the SeekToNextFrame() might never be resolved/rejected and the APP hangs.

the SeekPromise is only ever rejected in Exit() as such we really don't even need a rejection handling. Exit is called first and will disconnect the request.

when AccurateSeek fail, the promise is not rejected directly. it calls DecodeError which will later makes MDSM switch to ShutdownState. The change of state causes Exit() to be called which will reset the promise.

> How about replacing this logic with a virtual function, say `virtual void Transit();`, as a template method here. 
> 
> We can have a default behavior as
> ```
> void SeekingState::Transit()
> {
>   SetState<DecodingState>();
> }
> ```
> 
> and we can override it for the special behavior (no transition) for the NextFrameSeekingFromDormantState.
> 
> ```
> void NextFrameSeekingFromDormantState::Transit() { // No transition. }
> ```
> 
> In this way, we don't need to add the `SeekJob::mTransition` variable and keep the logics controled by state objects themselve.

i had thought about that originally. but couldn't do it originally.

now, with the latest chsnges it makes sense, we could even schedule a new next frame seek from there. it would simplify the code even further, removing most of code in Enter

feel free to continue that

Comment 33

[Jean-Yves Avenard [:jya]]

Comment on attachment 8870155 [details]
Bug 1366362: P3. Have seekToNextFrame properly handle dormant mode.

https://reviewboard.mozilla.org/r/141604/#review145778

> You can remove these functions by letting NextFrameSeekingFromDormantState inherit AccurateSeekingState.

hmmmm will do

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f415315d63be
https://hg.mozilla.org/mozilla-central/rev/82572291a255
https://hg.mozilla.org/mozilla-central/rev/63fdec8b12fa