Closed Bug 1108728 Opened 10 years ago Closed 10 years ago

Remove dormant related state from MediaDecoder

Categories

(Core :: Audio/Video, defect)

Product:
Core
Component:
Audio/Video
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla37
Tracking Flags:
Tracking Status
firefox36 --- fixed
firefox37 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

Attachments

(1 file, 2 obsolete files)

patch - Remove dormant related state from MediaDecoder
9.55 KB, patch
Sylvestre
: approval-mozilla-beta+
Details | Diff | Splinter Review 
MediaDecoder and MediaDecoderStateMachine both holding dormant related state. It seems redundant and it seems to cause code complex.
Assignee: nobody → sotaro.ikeda.g
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.
Attachment #8533713 - Attachment is obsolete: true
(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.
Attachment #8533917 - Flags: review?(cpearce)
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.
Attachment #8533917 - Flags: feedback+
(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?
Flags: needinfo?(cpearce)
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!
Attachment #8533917 - Flags: review?(cpearce) → review+
Flags: needinfo?(cpearce)
Blocks: 1110343
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.
Flags: needinfo?(sotaro.ikeda.g)
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".
Attachment #8533917 - Attachment is obsolete: true
(In reply to Sotaro Ikeda [:sotaro] from comment #27)
> https://tbpl.mozilla.org/?tree=Try&rev=22afb4b84d95

Passed the failed test.
(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.
https://hg.mozilla.org/mozilla-central/rev/0f5cdda7a611
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Blocks: 1121064
Blocks: 1050031
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
status-firefox36: --- → fixed
status-firefox37: --- → fixed
Flags: needinfo?(giles)
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.
Flags: needinfo?(giles)
Attachment #8535711 - Flags: approval-mozilla-beta?
Comment on attachment 8535711 [details] [diff] [review]
patch - Remove dormant related state from MediaDecoder

No worries (Ryan mentioned it on IRC yesterday)
Attachment #8535711 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: