1108728 - Remove dormant related state from MediaDecoder

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Sotaro Ikeda [:sotaro]]

MediaDecoder and MediaDecoderStateMachine both holding dormant related state. It seems redundant and it seems to cause code complex.

Comment 1

[Sotaro Ikeda [:sotaro]]

The following is MediaDecoder's state diagram. Its transition has a dependency to dormant state.

https://github.com/sotaroikeda/firefox-diagrams/blob/master/media/dom_media_MediaDecoder_state_FirefoxOS_2_2.pdf?raw=true

Comment 2

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Remove dormant related state from MediaDecoder

Comment 3

[Sotaro Ikeda [:sotaro]]

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

Comment 4

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #3)
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b446f2c1f3e8

some tests were failed.

Comment 5

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=b446f2c1f3e8

Comment 6

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Remove dormant related state from MediaDecoder

Fix seeking state problem.

Comment 7

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=256b00798eb2

Comment 8

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #7)
> https://tbpl.mozilla.org/?tree=Try&rev=256b00798eb2

Almost all test failures are fixed. But 2 test is failed on media. It is not clear that they are related to the patch.

Comment 9

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=cce196c68e24

Comment 10

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #8)
> (In reply to Sotaro Ikeda [:sotaro] from comment #7)
> > https://tbpl.mozilla.org/?tree=Try&rev=256b00798eb2
> 
> Almost all test failures are fixed. But 2 test is failed on media. It is not
> clear that they are related to the patch.

One test failure seems to be related to Bug 1095438.

Comment 11

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

Comment on attachment 8533917 [details] [diff] [review]
patch - Remove dormant related state from MediaDecoder

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

::: dom/media/MediaDecoder.cpp
@@ +825,5 @@
>  bool MediaDecoder::IsSeeking() const
>  {
>    MOZ_ASSERT(NS_IsMainThread());
> +  return mPlayState == PLAY_STATE_SEEKING ||
> +    (mPlayState == PLAY_STATE_LOADING && mRequestedSeekTarget.IsValid());

I think you can check mPlayState == PLAY_STATE_SEEKING || mRequestedSeekTarget.IsValid() only.

Comment 12

[Sotaro Ikeda [:sotaro]]

(In reply to JW Wang [:jwwang] from comment #11)
> Comment on attachment 8533917 [details] [diff] [review]
> patch - Remove dormant related state from MediaDecoder
> 
> Review of attachment 8533917 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/MediaDecoder.cpp
> @@ +825,5 @@
> >  bool MediaDecoder::IsSeeking() const
> >  {
> >    MOZ_ASSERT(NS_IsMainThread());
> > +  return mPlayState == PLAY_STATE_SEEKING ||
> > +    (mPlayState == PLAY_STATE_LOADING && mRequestedSeekTarget.IsValid());
> 
> I think you can check mPlayState == PLAY_STATE_SEEKING ||
> mRequestedSeekTarget.IsValid() only.

Thanks for the suggestion. But the suggestion make IsSeeking() as to include PLAY_STATE_SHUTDOWN.

Comment 13

[Sotaro Ikeda [:sotaro]]

Intent of this bug is just to remove dormant related state from MediaDecoder, I want to limit the change only to related part. attachment 8533917 [details] [diff] [review] try to keep MediaDecoder's act as same as possible.

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8533917 [details] [diff] [review]
patch - Remove dormant related state from MediaDecoder

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

::: dom/media/MediaDecoder.cpp
@@ +825,5 @@
>  bool MediaDecoder::IsSeeking() const
>  {
>    MOZ_ASSERT(NS_IsMainThread());
> +  return mPlayState == PLAY_STATE_SEEKING ||
> +    (mPlayState == PLAY_STATE_LOADING && mRequestedSeekTarget.IsValid());

Why do you need this? This looks like a behaviour change?

Comment 15

[Sotaro Ikeda [:sotaro]]

(In reply to Chris Pearce (:cpearce) from comment #14)
> Comment on attachment 8533917 [details] [diff] [review]
> patch - Remove dormant related state from MediaDecoder
> 
> Review of attachment 8533917 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/MediaDecoder.cpp
> @@ +825,5 @@
> >  bool MediaDecoder::IsSeeking() const
> >  {
> >    MOZ_ASSERT(NS_IsMainThread());
> > +  return mPlayState == PLAY_STATE_SEEKING ||
> > +    (mPlayState == PLAY_STATE_LOADING && mRequestedSeekTarget.IsValid());
> 
> Why do you need this? This looks like a behaviour change?

It is not behavior change. Current MediaDecoder implementation permits status change from PLAY_STATE_LOADING to PLAY_STATE_PLAYING without FirstFrameLoaded. Just call MediaDecoder::Play() do this. attachment 8533917 [details] [diff] [review] forbid this transition to simplify the implementation. And I think that current transition does not make sense. It is not consistent.

When MediaDecoder::Seek() is called during PLAY_STATE_LOADING state, it does not change state. But when MediaDecoder::Seek() is called after MediaDecoder::Play(), it change to PLAY_STATE_SEEKING state via PLAY_STATE_PLAYING without FirstFrameLoaded.

The following test code expect that MediaDecoder::IsSeeking() returns true  without waiting FirstFrameLoaded.

>  v.play();
>  v.currentTime=seekTime;
>  seekFlagStart = v.seeking;

> http://dxr.mozilla.org/mozilla-central/source/dom/media/test/test_seek-1.html#42

To keep the media element behavior same, therefore such change becomes necessary. Without it, the tests failed like the following.

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

Comment 16

[Sotaro Ikeda [:sotaro]]

cpearce, how do you think about comment 15?

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8533917 [details] [diff] [review]
patch - Remove dormant related state from MediaDecoder

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

OK. That explanation makes sense. Thanks Sotaro!

Comment 18

[Sotaro Ikeda [:sotaro]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/22484b7bbff3

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/fabce979edd7 for b2g mochitest-11 bustage:

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4570955&repo=mozilla-inbound

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Well, the bustage was real, but I accidentally backed out the wrong commit.
The actual backout is https://hg.mozilla.org/integration/mozilla-inbound/rev/5ee80f974980

Comment 21

[Sotaro Ikeda [:sotaro]]

(In reply to Wes Kocher (:KWierso) from comment #19)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/fabce979edd7 for b2g
> mochitest-11 bustage:
> 
> https://treeherder.mozilla.org/ui/logviewer.
> html#?job_id=4570955&repo=mozilla-inbound

I do not think the patch has a bug. Instead, it seems to trigger a hidden bug because of timing change.

Comment 22

[Jean-Yves Avenard [:jya]]

While MediaDecoder will immediately update its state upon a seek request (as it should), the MediaDecoderStateMachine will only process the seek once FirstFrameLoaded is reached.

So IMHO, MediaDecoder::IsSeeking should only test if PLAY_STATE_SEEKING is true

Your changes change that predicament, and trigger the assert.

Comment 23

[Sotaro Ikeda [:sotaro]]

test_bug448534.html was failed. When test started, video element is removed from the document. It set the MediaDecoderStateMachine state to DECODER_STATE_DORMANT on gonk.

http://mxr.mozilla.org/mozilla-central/source/dom/media/test/test_bug448534.html?force=1#27

There is inconsistency between MediaDecoderStateMachine::StartSeek() and MediaDecoderStateMachine::Seek(). Seek() checks if the state is more than DECODER_STATE_DECODING_FIRSTFRAME. StartSeek() checks if the state is more than DECODER_STATE_DECODING. Between 2 states, DECODER_STATE_DORMANT states exist. If the state is DECODER_STATE_DORMANT, seek is not correctly handled by current MediaDecoderStateMachine.

Comment 24

[Sotaro Ikeda [:sotaro]]

Quick fix seem to change DECODER_STATE_DORMANT as lower than DECODER_STATE_DECODING_FIRSTFRAME.

Comment 25

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #24)
> Quick fix seem to change DECODER_STATE_DORMANT as lower than
> DECODER_STATE_DECODING_FIRSTFRAME.

Use DECODER_STATE_DECODING in Seek() seems safer as a quick fix.

Comment 26

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Remove dormant related state from MediaDecoder

Fix test failure. Carry "r=cpearce".

Comment 27

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=22afb4b84d95
https://tbpl.mozilla.org/?tree=Try&rev=22afb4b84d95

Comment 28

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #27)
> https://tbpl.mozilla.org/?tree=Try&rev=22afb4b84d95

Passed the failed test.

Comment 29

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5bfe30e00d27
https://tbpl.mozilla.org/?tree=Try&rev=5bfe30e00d27

reconfirm for b2g.

Comment 30

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #29)
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5bfe30e00d27
> https://tbpl.mozilla.org/?tree=Try&rev=5bfe30e00d27
> 
> reconfirm for b2g.

Tests results seem good.

Comment 31

[Jean-Yves Avenard [:jya]]

(In reply to Sotaro Ikeda [:sotaro] from comment #23)
> test_bug448534.html was failed. When test started, video element is removed
> from the document. It set the MediaDecoderStateMachine state to
> DECODER_STATE_DORMANT on gonk.
> 
> http://mxr.mozilla.org/mozilla-central/source/dom/media/test/test_bug448534.
> html?force=1#27
> 
> There is inconsistency between MediaDecoderStateMachine::StartSeek() and
> MediaDecoderStateMachine::Seek(). Seek() checks if the state is more than
> DECODER_STATE_DECODING_FIRSTFRAME. StartSeek() checks if the state is more
> than DECODER_STATE_DECODING. Between 2 states, DECODER_STATE_DORMANT states
> exist. If the state is DECODER_STATE_DORMANT, seek is not correctly handled
> by current MediaDecoderStateMachine.

You should only ever start seeking once you've decoded the first frame and the MDSM is in DECODER_STATE_DECODING_FIRSTFRAME

MediaDecoderStateMachine::Seek() only queue a seek if we haven't decoded the first frame yet.
MediaDecoderStateMachine::StartSeek() is the actual seek.

Changing the assert so your code works seems wrong to me.

Comment 32

[Sotaro Ikeda [:sotaro]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0f5cdda7a611

Comment 33

[Sotaro Ikeda [:sotaro]]

(In reply to Jean-Yves Avenard [:jya] from comment #31)
> (In reply to Sotaro Ikeda [:sotaro] from comment #23)
> > test_bug448534.html was failed. When test started, video element is removed
> > from the document. It set the MediaDecoderStateMachine state to
> > DECODER_STATE_DORMANT on gonk.
> > 
> > http://mxr.mozilla.org/mozilla-central/source/dom/media/test/test_bug448534.
> > html?force=1#27
> > 
> > There is inconsistency between MediaDecoderStateMachine::StartSeek() and
> > MediaDecoderStateMachine::Seek(). Seek() checks if the state is more than
> > DECODER_STATE_DECODING_FIRSTFRAME. StartSeek() checks if the state is more
> > than DECODER_STATE_DECODING. Between 2 states, DECODER_STATE_DORMANT states
> > exist. If the state is DECODER_STATE_DORMANT, seek is not correctly handled
> > by current MediaDecoderStateMachine.
> 
> You should only ever start seeking once you've decoded the first frame and
> the MDSM is in DECODER_STATE_DECODING_FIRSTFRAME
> 
> MediaDecoderStateMachine::Seek() only queue a seek if we haven't decoded the
> first frame yet.
> MediaDecoderStateMachine::StartSeek() is the actual seek.
> 
> Changing the assert so your code works seems wrong to me.

MediaDecoder call Seek only when MediaDecoder::FirstFrameLoaded() is called. But there is a interaction related to dormant state. Current MediaDecoderStateMachine does not handle it correctly.

Comment 34

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #33)
> MediaDecoder call Seek only when MediaDecoder::FirstFrameLoaded() is called.

It is after applying the patch.

Comment 35

[Jean-Yves Avenard [:jya]]

(In reply to Sotaro Ikeda [:sotaro] from comment #34)
> (In reply to Sotaro Ikeda [:sotaro] from comment #33)
> > MediaDecoder call Seek only when MediaDecoder::FirstFrameLoaded() is called.
> 
> It is after applying the patch.

You should be able to call seek before first frame loaded is called. Typical use would be with MSE only loading the init segment (which cause loadedmetadata to be fired). 
If you can only call seek after loadeddata/first frame loaded is issued, you break that behaviour. 

Unfortunately I don't believe we have a mochitest for that.

Comment 36

[Sotaro Ikeda [:sotaro]]

(In reply to Jean-Yves Avenard [:jya] from comment #35)
> (In reply to Sotaro Ikeda [:sotaro] from comment #34)
> > (In reply to Sotaro Ikeda [:sotaro] from comment #33)
> > > MediaDecoder call Seek only when MediaDecoder::FirstFrameLoaded() is called.
> > 
> > It is after applying the patch.
> 
> You should be able to call seek before first frame loaded is called. Typical
> use would be with MSE only loading the init segment (which cause
> loadedmetadata to be fired). 

jya, in this case, first frame is loaded from where?

In current MediaDecoderStateMachine, when Seek() is called before "first frame loaded", actual seek task is queued at the end of MediaDecoderStateMachine::FinishDecodeFirstFrame(). And before EnqueueStartQueuedSeekTask() is called, StartDecoding() is called in FinishDecodeFirstFrame().

Comment 37

[Sotaro Ikeda [:sotaro]]

Seek task is posted to main thread, because MediaDecoderStateMachine::StartSeek() need to be called on main thread. But StartDecoding() post task to decoding thread.

And FirstFrameLoadedEventRunner is also posted to main thread. Then MediaDecoder::FirstFrameLoaded() is called on main thread and call MediaDecoderStateMachine::Seek() in the function if the seek is already requested. Therefore, after applying the patch, the timing of calling MediaDecoderStateMachine::StartSeek() is same between before applying the patch and after applying the patch.

Comment 38

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

For such a call sequence:
1. MediaDecoder::MetadataLoaded
2. MediaDecoder::SetDormantIfNecessary(true)
3. MediaDecoder::FirstFrameLoaded

MediaDecoder::FirstFrameLoaded will exit immediately before the patch. However, it will continue and call MediaDecoderStateMachine::Seek after the patch. Since we are removing dormant state from MediaDecoder, MediaDecoderStateMachine::Seek should handle the dormant state and exit immediately when mState is dormant.

Comment 39

[Sotaro Ikeda [:sotaro]]

Current implementation has some problems. During fixing them, it seems also better fix it.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0f5cdda7a611

Comment 41

[Ryan VanderMeulen [:RyanVM]]

This was blocking bug 1121692 from uplifting cleanly, so I went ahead and uplifted it to beta. We'll need a retroactive beta approval request on it, though.

https://hg.mozilla.org/releases/mozilla-beta/rev/9ad34e90e339

Comment 42

[Ralph Giles (:rillian)]

Comment on attachment 8535711 [details] [diff] [review]
patch - Remove dormant related state from MediaDecoder

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: YouTube playback uses more resources. Less consistent testing.
[Describe test coverage new/current, TreeHerder]: Landed on 37, 38 some time ago.
[Risks and why]: Low. This affects non-MSE video playback, but we would have seen regressions by now.
[String/UUID change made/needed]: None.

Retroactive approval request, since this turned out to be a dependency of the seek fixes bug 112692 we wanted in 36 beta 2.

Comment 43

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8535711 [details] [diff] [review]
patch - Remove dormant related state from MediaDecoder

No worries (Ryan mentioned it on IRC yesterday)