Closed Bug 1158448 Opened 9 years ago Closed 9 years ago

Replace MDSM::Play with a state-watcher on mPlayState and eliminate ApplyStateToStateMachine

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: bholley, Unassigned)

References

Details

Attachments

(2 files, 5 obsolete files)

I was going to do this as part of bug 1157803, but decided to split it out.
Depends on: 1159558
So, it looks like ApplyStateToStateMachine is actually overridden for various
b2g decoders.

The uses in RtspMediaCodecDecoder and RtspOmxDecoder should be pretty easy to
fix - I think we can just override ChangeState instead. But the usage in
MediaOmxCommonDecoder is trickier.

I could make MediaDecoderStateMachine::PlayStateChange bail out if we're in
dormant mode. But it's not clear to me exactly what we want to do about the
seeking state, and that comment is ominous.

Sotaro, how do you think we should proceed?
Attachment #8599635 - Flags: review?(sotaro.ikeda.g)
(In reply to Bobby Holley (:bholley) from comment #1)
> Created attachment 8599635 [details] [diff] [review]
> Replace MDSM::Play with a state-watcher on mPlayState and eliminate
> ApplyStateToStateMachine. v1
> 
> So, it looks like ApplyStateToStateMachine is actually overridden for various
> b2g decoders.
> 
> The uses in RtspMediaCodecDecoder and RtspOmxDecoder should be pretty easy to
> fix - I think we can just override ChangeState instead. But the usage in
> MediaOmxCommonDecoder is trickier.
> 
> I could make MediaDecoderStateMachine::PlayStateChange bail out if we're in
> dormant mode. But it's not clear to me exactly what we want to do about the
> seeking state, and that comment is ominous.
> 
> Sotaro, how do you think we should proceed?

I also think we should proceed. ApplyStateToStateMachine() just seem to degrade the code readability.
Attachment #8599635 - Flags: review?(sotaro.ikeda.g) → review+
Ok. What should I do for the AudioOffload stuff? Just delete the override and flag you for ni?
Flags: needinfo?(sotaro.ikeda.g)
(In reply to Bobby Holley (:bholley) from comment #4)
> Ok. What should I do for the AudioOffload stuff? Just delete the override
> and flag you for ni?

We need to do a bit more. Can I take b2g decoder staff?
Flags: needinfo?(sotaro.ikeda.g)
(In reply to Sotaro Ikeda [:sotaro] from comment #5)
> (In reply to Bobby Holley (:bholley) from comment #4)
> > Ok. What should I do for the AudioOffload stuff? Just delete the override
> > and flag you for ni?
> 
> We need to do a bit more. Can I take b2g decoder staff?

Sure, thank you!

I'll leave you needinfoed in the bug to indicate that we're waiting on a second patch from you.
Flags: needinfo?(sotaro.ikeda.g)
If we remove PLAY_STATE_SEEKING, removing ApplyStateToStateMachine seems to becomes simpler.
Flags: needinfo?(sotaro.ikeda.g)
Comment on attachment 8600694 [details] [diff] [review]
patch - Remove ApplyStateToStateMachine and PLAY_STATE_SEEKING

bholley, can you give a feedback?
Attachment #8600694 - Flags: feedback?(bobbyholley)
Comment on attachment 8600694 [details] [diff] [review]
patch - Remove ApplyStateToStateMachine and PLAY_STATE_SEEKING

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

This looks great aside from the mIsSeeking mirroring. Fix that up and maybe have jww give it a look-over?

Thanks for helping make this stuff better!

::: dom/media/MediaDecoder.cpp
@@ +818,5 @@
>  nsresult MediaDecoder::Seek(double aTime, SeekTarget::Type aSeekType)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
> +
> +  if (mShuttingDown) {

This should probably go inside the monitor, right?

I'd also use NS_ENSURE_TRUE(!mShuttingDown, NS_ERROR_FAILURE) here.

::: dom/media/MediaDecoderStateMachine.cpp
@@ +216,5 @@
>               "MediaDecoderStateMachine::mPlayState (Mirror)"),
>    mNextPlayState(mTaskQueue, MediaDecoder::PLAY_STATE_PAUSED,
>                   "MediaDecoderStateMachine::mNextPlayState (Mirror)"),
> +  mIsSeeking(mTaskQueue, false,
> +             "MediaDecoderStateMachine::mIsSeeking (Mirror)"),

I think we should call this mLogicallySeeking to differentiate this from DECODER_STATE_SEEKING.

@@ +2931,5 @@
>    AssertCurrentThreadInMonitor();
>    NS_ASSERTION(!HasAudio() || mAudioStartTime != -1,
>                 "Should know audio start time if we have audio.");
>  
> +  if (mPlayState != MediaDecoder::PLAY_STATE_PLAYING || mIsSeeking) {

Is this the only reason why we need to mirror the seeking state to the MDSM? I think we should just find a way to get rid of this. In general, it seems like the MDSM shouldn't worry about logical seeking state, it should only worry about DECODER_STATE_SEEKING.
Attachment #8600694 - Flags: feedback?(bobbyholley) → feedback+
(In reply to Bobby Holley (:bholley) from comment #11)
> Comment on attachment 8600694 [details] [diff] [review]
> patch - Remove ApplyStateToStateMachine and PLAY_STATE_SEEKING
> 
> Review of attachment 8600694 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks great aside from the mIsSeeking mirroring. Fix that up and maybe
> have jww give it a look-over?
> 
> Thanks for helping make this stuff better!
> 
> ::: dom/media/MediaDecoder.cpp
> @@ +818,5 @@
> >  nsresult MediaDecoder::Seek(double aTime, SeekTarget::Type aSeekType)
> >  {
> >    MOZ_ASSERT(NS_IsMainThread());
> > +
> > +  if (mShuttingDown) {
> 
> This should probably go inside the monitor, right?

Yes, I will update in a next patch.

> 
> I'd also use NS_ENSURE_TRUE(!mShuttingDown, NS_ERROR_FAILURE) here.
> 
> ::: dom/media/MediaDecoderStateMachine.cpp
> @@ +216,5 @@
> >               "MediaDecoderStateMachine::mPlayState (Mirror)"),
> >    mNextPlayState(mTaskQueue, MediaDecoder::PLAY_STATE_PAUSED,
> >                   "MediaDecoderStateMachine::mNextPlayState (Mirror)"),
> > +  mIsSeeking(mTaskQueue, false,
> > +             "MediaDecoderStateMachine::mIsSeeking (Mirror)"),
> 
> I think we should call this mLogicallySeeking to differentiate this from
> DECODER_STATE_SEEKING.

Update in a next patch.

> 
> @@ +2931,5 @@
> >    AssertCurrentThreadInMonitor();
> >    NS_ASSERTION(!HasAudio() || mAudioStartTime != -1,
> >                 "Should know audio start time if we have audio.");
> >  
> > +  if (mPlayState != MediaDecoder::PLAY_STATE_PLAYING || mIsSeeking) {
> 
> Is this the only reason why we need to mirror the seeking state to the MDSM?
> I think we should just find a way to get rid of this. In general, it seems
> like the MDSM shouldn't worry about logical seeking state, it should only
> worry about DECODER_STATE_SEEKING.

Yes, it is a main reason. And also to pass NS_ASSERTION() check. Without it.

Without it, tryserver test did not pass when the test uses DecodedStreamData as a sink. Because, its blocking state is updated at MediaDecoder on main thread. It could cause "incorrect clock time" like the following
  > GetMediaTime() <= clock_time

The following was actual tryserver result without the mIsSeeking mirroring.
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ddd230d5423

In a next patch, I am going to put off the remove of mIsSeeking mirroring. And then next try to find other workaround.
Apply the comments except removing mLogicallySeeking mirror and update of seek hadling of MediaOmxCommonDecoder::FirstFrameLoaded().
Attachment #8600694 - Attachment is obsolete: true
un-bitrot.
Attachment #8601520 - Attachment is obsolete: true
I rechecked attachment 8601538 [details] [diff] [review]. For me, it seems OK to mirror mLogicallySeeking to MediaDecoderStateMachine.
The patch split mPlayState to mPlayState and mLogicallySeeking. If mPlayState is OK to mirror, mLogicallySeeking also seems OK to mirror.

As in Comment 12, MediaDecoderStateMachine have a dependence to MediaDecoder::mDecodedStream. It causes the necessity to mirror mLogicallySeeking. It seems simple way to address the mDecodedStream's dependency problem.
(In reply to Sotaro Ikeda [:sotaro] from comment #17)
> I rechecked attachment 8601538 [details] [diff] [review]. For me, it seems
> OK to mirror mLogicallySeeking to MediaDecoderStateMachine.
> The patch split mPlayState to mPlayState and mLogicallySeeking. If
> mPlayState is OK to mirror, mLogicallySeeking also seems OK to mirror.

It's not incorrect, but I think it adds more state-coupling and boilerplate than we need.

> As in Comment 12, MediaDecoderStateMachine have a dependence to
> MediaDecoder::mDecodedStream. It causes the necessity to mirror
> mLogicallySeeking. It seems simple way to address the mDecodedStream's
> dependency problem.

I don't understand what you mean here. The only non-debug usage of mLogicallySeeking is in the bailout case of AdvanceFrame, and I'm skeptical that it's really necessary. Is there another case that I'm missing?
(In reply to Bobby Holley (:bholley) from comment #18)
> 
> > As in Comment 12, MediaDecoderStateMachine have a dependence to
> > MediaDecoder::mDecodedStream. It causes the necessity to mirror
> > mLogicallySeeking. It seems simple way to address the mDecodedStream's
> > dependency problem.
> 
> I don't understand what you mean here. The only non-debug usage of
> mLogicallySeeking is in the bailout case of AdvanceFrame, and I'm skeptical
> that it's really necessary. Is there another case that I'm missing?

AdvanceFrame and Trigger ScheduleStateMachine() on seek end.
I think it is a bit redundant but acceptable level.
bholley, what is your preference to it?
Flags: needinfo?(bobbyholley)
And I already did my part. You can modify the patch as you want:)
(In reply to Sotaro Ikeda [:sotaro] from comment #20)
> I think it is a bit redundant but acceptable level.
> bholley, what is your preference to it?

I'm happy fixing it in a followup bug.

(In reply to Sotaro Ikeda [:sotaro] from comment #21)
> And I already did my part. You can modify the patch as you want:)

Can you flag jww and an OMX person for review?
Flags: needinfo?(bobbyholley)
Comment on attachment 8601538 [details] [diff] [review]
patch - Remove ApplyStateToStateMachine and PLAY_STATE_SEEKING

Flagging jww on behalf of sotaro.
Attachment #8601538 - Flags: review?(jwwang)
Comment on attachment 8601538 [details] [diff] [review]
patch - Remove ApplyStateToStateMachine and PLAY_STATE_SEEKING

Flagging bwu to look over the omx bits.
Attachment #8601538 - Flags: review?(bwu)
Comment on attachment 8601538 [details] [diff] [review]
patch - Remove ApplyStateToStateMachine and PLAY_STATE_SEEKING

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

MediaDecoer*.* look fine to me. However, I am curious how mLogicallySeeking is useful to the state machine.

::: dom/media/MediaDecoder.cpp
@@ +435,5 @@
>  {
>    mListener->Forget();
>  }
>  
> +void MediaDecoder::UpdateDecodedStream()

Assert holding the monitor.

::: dom/media/MediaDecoderStateMachine.cpp
@@ +1674,5 @@
> +void MediaDecoderStateMachine::LogicallySeekingChanged()
> +{
> +  MOZ_ASSERT(OnTaskQueue());
> +  ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
> +  ScheduleStateMachine();

Why do we need to schedule next cycle when logical seeking changes?

@@ +2950,5 @@
>    AssertCurrentThreadInMonitor();
>    NS_ASSERTION(!HasAudio() || mAudioStartTime != -1,
>                 "Should know audio start time if we have audio.");
>  
> +  if (mPlayState != MediaDecoder::PLAY_STATE_PLAYING || mLogicallySeeking) {

Early bail out when mLogicallySeeking is ture doesn't seem necessary to me for if we are seeking, we will change state from DECODING to SEEKING soon.
Attachment #8601538 - Flags: review?(jwwang) → review+
(In reply to JW Wang [:jwwang] from comment #25)
> Comment on attachment 8601538 [details] [diff] [review]
> patch - Remove ApplyStateToStateMachine and PLAY_STATE_SEEKING
> 
> Review of attachment 8601538 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> MediaDecoer*.* look fine to me. However, I am curious how mLogicallySeeking
> is useful to the state machine.
> 
> ::: dom/media/MediaDecoder.cpp
> @@ +435,5 @@
> >  {
> >    mListener->Forget();
> >  }
> >  
> > +void MediaDecoder::UpdateDecodedStream()
> 
> Assert holding the monitor.
> 
> ::: dom/media/MediaDecoderStateMachine.cpp
> @@ +1674,5 @@
> > +void MediaDecoderStateMachine::LogicallySeekingChanged()
> > +{
> > +  MOZ_ASSERT(OnTaskQueue());
> > +  ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
> > +  ScheduleStateMachine();
> 
> Why do we need to schedule next cycle when logical seeking changes?

mirrored mLogicallySeeking is used not advance frame in AdvanceFrame(). Then MediaDecoderStateMachine stop scheduling. It means "IsStateMachineScheduled() == false". Therefore some one need to tirgger re-start advance frame when mLogicallySeeking becomes false. ScheduleStateMachine() do the re-start. It is virtually same to current MediaDecoderStateMachine.

> 
> @@ +2950,5 @@
> >    AssertCurrentThreadInMonitor();
> >    NS_ASSERTION(!HasAudio() || mAudioStartTime != -1,
> >                 "Should know audio start time if we have audio.");
> >  
> > +  if (mPlayState != MediaDecoder::PLAY_STATE_PLAYING || mLogicallySeeking) {
> 
> Early bail out when mLogicallySeeking is ture doesn't seem necessary to me
> for if we are seeking, we will change state from DECODING to SEEKING soon.

It is necessary as in Comment 12. Current code just virtually work same way to current MediaDecoderStateMachine. If we do not have it, MediaDecoderStateMachine re-start output data to mDecodedStream before DecodedStream update its state in main thread. It causes clock time inconsistency in MediaDecoderStateMachine. And in some tests cases, some playback did not progress until end.

It might be possible to remove it by modifying other more codes, but it is not scope of this bug.
(In reply to Bobby Holley (:bholley) from comment #24)
> Comment on attachment 8601538 [details] [diff] [review]
> patch - Remove ApplyStateToStateMachine and PLAY_STATE_SEEKING
> 
> Flagging bwu to look over the omx bits.

Thanks!
Apply the comment.
Attachment #8601538 - Attachment is obsolete: true
Attachment #8601538 - Flags: review?(bwu)
Attachment #8602122 - Flags: review?(bwu)
Remove one unnecessary variable.
Attachment #8602122 - Attachment is obsolete: true
Attachment #8602122 - Flags: review?(bwu)
Attachment #8602127 - Flags: review?(bwu)
Comment on attachment 8602127 [details] [diff] [review]
patch - Remove ApplyStateToStateMachine and PLAY_STATE_SEEKING(r=jwwang)

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

LGTM for most omx parts! 
Just have one place not clear to me.

::: dom/media/omx/MediaOmxCommonDecoder.cpp
@@ +101,5 @@
> +    mSeekRequest.DisconnectIfExists();
> +    mSeekRequest.Begin(mAudioOffloadPlayer->Seek(target)
> +      ->RefableThen(AbstractThread::MainThread(), __func__, static_cast<MediaDecoder*>(this),
> +                    &MediaDecoder::OnSeekResolved, &MediaDecoder::OnSeekRejected));
> +  }

IIUC, the above seek is for the case that callSeek() is called before this FirstFrameLoaded(), so seek is not done on AudioOffloadPlayer since it is not created yet. If it is that case, mLogicallySeeking could be false due to finished seek in MediaDecoder and seek will be still not done.
AFAIK, Seek can never be called by the MediaDecoderStateMachine before the first frame has been decoded
MediaDecoderStateMachine does not do seek until FirstFrame is loaded at MediaDecoderStateMachine, therefore, if MediaDecoder emit seek before FirstFrameLoaded() is called, mLogicallySeeking should be kept true.
I am clear now. Thanks for your explanations.
Comment on attachment 8602127 [details] [diff] [review]
patch - Remove ApplyStateToStateMachine and PLAY_STATE_SEEKING(r=jwwang)

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

LGTM for most omx parts! 
Just have one place not clear to me.

::: dom/media/omx/MediaOmxCommonDecoder.cpp
@@ +101,5 @@
> +    mSeekRequest.DisconnectIfExists();
> +    mSeekRequest.Begin(mAudioOffloadPlayer->Seek(target)
> +      ->RefableThen(AbstractThread::MainThread(), __func__, static_cast<MediaDecoder*>(this),
> +                    &MediaDecoder::OnSeekResolved, &MediaDecoder::OnSeekRejected));
> +  }

IIUC, the above seek is for the case that callSeek() is called before this FirstFrameLoaded(), so seek is not done on AudioOffloadPlayer since it is not created yet. If it is that case, mLogicallySeeking could be false due to finished seek in MediaDecoder and seek will be still not done.

@@ +247,5 @@
> +
> +  mSeekRequest.DisconnectIfExists();
> +  mSeekRequest.Begin(mAudioOffloadPlayer->Seek(aTarget)
> +    ->RefableThen(AbstractThread::MainThread(), __func__, static_cast<MediaDecoder*>(this),
> +                  &MediaDecoder::OnSeekResolved, &MediaDecoder::OnSeekRejected));

I'm wondering if it would make codes simpler to put the same seek operations to a function and call it here and FirstFrameLoaded().
Attachment #8602127 - Flags: review?(bwu) → review+
oops...Sorry to re-post the old comment again.
(In reply to Blake Wu [:bwu][:blakewu] from comment #35)
> 
> @@ +247,5 @@
> > +
> > +  mSeekRequest.DisconnectIfExists();
> > +  mSeekRequest.Begin(mAudioOffloadPlayer->Seek(aTarget)
> > +    ->RefableThen(AbstractThread::MainThread(), __func__, static_cast<MediaDecoder*>(this),
> > +                  &MediaDecoder::OnSeekResolved, &MediaDecoder::OnSeekRejected));
> 
> I'm wondering if it would make codes simpler to put the same seek operations
> to a function and call it here and FirstFrameLoaded().

It is intentional. At first, I thought like the above. But after that, I changed my mind. On current patch is more explicit what it does and creating a function just for the above seems too much.
You need to log in before you can comment on or make changes to this bug.