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

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

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

Fix seeking state problem.

(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.

(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 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.

(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.

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 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?

(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

cpearce, how do you think about comment 15?

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!

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

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

(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.

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.

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.

Quick fix seem to change DECODER_STATE_DORMANT as lower than DECODER_STATE_DECODING_FIRSTFRAME.

(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.

Fix test failure. Carry "r=cpearce".

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

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

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

(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.

(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.

(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.

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

(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.

(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().

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.

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.

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

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 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 on attachment 8535711 [details] [diff] [review] patch - Remove dormant related state from MediaDecoder No worries (Ryan mentioned it on IRC yesterday)