1325317 - Move StopMediaSink() out of MediaDecoderStateMachine::Reset()

Status: RESOLVED FIXED

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

Description

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

http://searchfox.org/mozilla-central/rev/4f5f9f3222f35fa4635ea96a0c963c130854ef19/dom/media/MediaDecoderStateMachine.cpp#3511

Stopping media sink shouldn't be a concern of Reset() which is about resetting decoders. We should move StopMediaSink() out of MediaDecoderStateMachine::Reset() and let the caller of Reset() decide whether to stop media sink along with Reset().

Comment 1

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

Attachment: Bug 1325317. Part 1 - move StopMediaSink() out of MediaDecoderStateMachine::Reset().

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

Comment 2

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

Attachment: Bug 1325317. Part 2 - rename the function and fix comments.

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

Comment 3

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8822408 [details]
Bug 1325317. Part 1 - move StopMediaSink() out of MediaDecoderStateMachine::Reset().

https://reviewboard.mozilla.org/r/101328/#review101974

::: dom/media/MediaDecoderStateMachine.cpp:974
(Diff revision 1)
>      if (mSeekJob.mTarget->IsVideoOnly()) {
>        mMaster->Reset(TrackInfo::kVideoTrack);
>      } else {
>        mMaster->Reset();
> +      mMaster->StopMediaSink();
>      }

Is there AudioOnly case in the 'else' block ? Can we identify the case here ?

I saw a default argument for |void MDSM::Reset(TrackSet aTracks = TrackSet(TrackInfo::kAudioTrack,TrackInfo::kVideoTrack))|, which means now there are only 2 cases where |MDSM::Reset| is called, that is, (1) with both A/V, (2) V only.

We put assertion in |MDSM::Reset| to make sure not to reset decoder if audio only.

::: dom/media/MediaDecoderStateMachine.cpp:3383
(Diff revision 1)
>    // Assert that aTracks specifies to reset the video track because we
>    // don't currently support resetting just the audio track.
>    MOZ_ASSERT(aTracks.contains(TrackInfo::kVideoTrack));

We never call |MDSM::Reset()| with aTracks containing audio only. Do you know why we don't support resetting decode with only audio track ?

::: dom/media/MediaDecoderStateMachine.cpp:3387
(Diff revision 1)
> -    // outside of the decoder monitor while we are clearing the queue and causes
> -    // crash for no samples to be popped.
> -    StopMediaSink();
> -  }
> -
>    if (aTracks.contains(TrackInfo::kVideoTrack)) {

If we're sure about to reset video track every time, maybe we don't need this 'if' anymore.

Comment 4

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

Comment on attachment 8822408 [details]
Bug 1325317. Part 1 - move StopMediaSink() out of MediaDecoderStateMachine::Reset().

https://reviewboard.mozilla.org/r/101328/#review101978

::: dom/media/MediaDecoderStateMachine.cpp:974
(Diff revision 1)
>      if (mSeekJob.mTarget->IsVideoOnly()) {
>        mMaster->Reset(TrackInfo::kVideoTrack);
>      } else {
>        mMaster->Reset();
> +      mMaster->StopMediaSink();
>      }

No. There is no use case for audio-only seek. And there is no 'IsAudioOnly' function exposed by SeekTarget().

::: dom/media/MediaDecoderStateMachine.cpp:3383
(Diff revision 1)
>    // Assert that aTracks specifies to reset the video track because we
>    // don't currently support resetting just the audio track.
>    MOZ_ASSERT(aTracks.contains(TrackInfo::kVideoTrack));

It is just as simple as no such use case for now.

::: dom/media/MediaDecoderStateMachine.cpp:3387
(Diff revision 1)
> -    // outside of the decoder monitor while we are clearing the queue and causes
> -    // crash for no samples to be popped.
> -    StopMediaSink();
> -  }
> -
>    if (aTracks.contains(TrackInfo::kVideoTrack)) {

Good point. But that should deserve another bug.

Comment 5

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8822408 [details]
Bug 1325317. Part 1 - move StopMediaSink() out of MediaDecoderStateMachine::Reset().

https://reviewboard.mozilla.org/r/101328/#review101978

> No. There is no use case for audio-only seek. And there is no 'IsAudioOnly' function exposed by SeekTarget().

I tried to seek this file http://searchfox.org/mozilla-central/source/testing/web-platform/tests/media/sound_5.oga, and the inforamtion in |MSDM::AccurateSeekingState::DoSeek| shows

'''
Info().HasVideo() => false
mSeekJob.mTarget->IsVideoOnly() => false
'''

For me, if I don't look up the declaration of MDSM::Reset(), I assume that's an AudioOnly seeking operation.
BTW, the seeking operation works fine.

> It is just as simple as no such use case for now.

Based on above comment, what I'm thinking is that maybe we could remove the comment and assertion here to eliminate the confusion.

Comment 6

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8822408 [details]
Bug 1325317. Part 1 - move StopMediaSink() out of MediaDecoderStateMachine::Reset().

https://reviewboard.mozilla.org/r/101328/#review102008

::: dom/media/MediaDecoderStateMachine.cpp:3387
(Diff revision 1)
> -    // outside of the decoder monitor while we are clearing the queue and causes
> -    // crash for no samples to be popped.
> -    StopMediaSink();
> -  }
> -
>    if (aTracks.contains(TrackInfo::kVideoTrack)) {

Cool, I'm ok with that.

Comment 7

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

Comment on attachment 8822408 [details]
Bug 1325317. Part 1 - move StopMediaSink() out of MediaDecoderStateMachine::Reset().

https://reviewboard.mozilla.org/r/101328/#review101978

> I tried to seek this file http://searchfox.org/mozilla-central/source/testing/web-platform/tests/media/sound_5.oga, and the inforamtion in |MSDM::AccurateSeekingState::DoSeek| shows
> 
> '''
> Info().HasVideo() => false
> mSeekJob.mTarget->IsVideoOnly() => false
> '''
> 
> For me, if I don't look up the declaration of MDSM::Reset(), I assume that's an AudioOnly seeking operation.
> BTW, the seeking operation works fine.

To make it clear:
1. seek on an audio-only file -> full seek
2. seek on a video-only file -> full seek
3. seek video on a file with both audio and video -> video-only seek
4. seek audio on a file with both audio and video -> audio-only seek (we don't have such a use case)

Your example is case 1, not case case 4.

Comment 8

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8822408 [details]
Bug 1325317. Part 1 - move StopMediaSink() out of MediaDecoderStateMachine::Reset().

https://reviewboard.mozilla.org/r/101328/#review101978

> To make it clear:
> 1. seek on an audio-only file -> full seek
> 2. seek on a video-only file -> full seek
> 3. seek video on a file with both audio and video -> video-only seek
> 4. seek audio on a file with both audio and video -> audio-only seek (we don't have such a use case)
> 
> Your example is case 1, not case case 4.

Thanks for the explaination, now I understand.  Though the naming VideoOnly/Audioonly here is a bit confusing...

Comment 9

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8822408 [details]
Bug 1325317. Part 1 - move StopMediaSink() out of MediaDecoderStateMachine::Reset().

https://reviewboard.mozilla.org/r/101328/#review102034

Comment 10

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8822409 [details]
Bug 1325317. Part 2 - rename the function and fix comments.

https://reviewboard.mozilla.org/r/101330/#review102036

Comment 11

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

Thanks!

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9ac746c78f4f
Part 1 - move StopMediaSink() out of MediaDecoderStateMachine::Reset(). r=kikuo
https://hg.mozilla.org/integration/autoland/rev/ae88fa85a86b
Part 2 - rename the function and fix comments. r=kikuo

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9ac746c78f4f
https://hg.mozilla.org/mozilla-central/rev/ae88fa85a86b