1306186 - Merge MDSM::mIsVideoPrerolling and MDSM::mIsAudioPrerolling

Status: RESOLVED FIXED

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

Description

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

We just need one bool flag to decide whether MDSM is still prerolling.

Comment 1

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

Attachment: Bug 1306186. Part 1 - remove checks for mIsAudioPrerolling and mIsVideoPrerolling from NeedToSkipToNextKeyframe().

When we reach |isLowOnDecodedAudio = ...|, mMediaSink->IsStarted() must be true which means prerolling is done
and both mIsAudioPrerolling and mIsVideoPrerolling are false.

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

Comment 2

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

Attachment: Bug 1306186. Part 2 - reset prerolling flags in MaybeStartPlayback().

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

Comment 3

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

Attachment: Bug 1306186. Part 3 - remove StopPrerolling{Audio,Video}.

Note we remove the calls from OnSeekTask{Resolved,Rejected} because DecodingState will decide whether to reset those flags.

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

Comment 4

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

Attachment: Bug 1306186. Part 4 - merge mIsAudioPrerolling and mIsVideoPrerolling into mIsPrerolling.

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

Comment 5

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8796429 [details]
Bug 1306186. Part 1 - remove checks for mIsAudioPrerolling and mIsVideoPrerolling from NeedToSkipToNextKeyframe().

https://reviewboard.mozilla.org/r/82296/#review80930

Comment 6

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8796430 [details]
Bug 1306186. Part 2 - reset prerolling flags in MaybeStartPlayback().

https://reviewboard.mozilla.org/r/82298/#review80936

::: dom/media/MediaDecoderStateMachine.cpp:2787
(Diff revision 1)
> -    StopPrerollingVideo();
> -  }
> -
> -  ScheduleStateMachine();
> +    ScheduleStateMachine();
> -}
> +  }
> +}

Originally, |ScheduleStateMachine| is invoked whether Audio/Video is prerolling or not.
I suggest to call |ScheduleStateMachine| at the end of function, and remove the '''if''' conditiona and comment to make it simplier and clear.

What do you think ?

Comment 7

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

Comment on attachment 8796430 [details]
Bug 1306186. Part 2 - reset prerolling flags in MaybeStartPlayback().

https://reviewboard.mozilla.org/r/82298/#review80936

> Originally, |ScheduleStateMachine| is invoked whether Audio/Video is prerolling or not.
> I suggest to call |ScheduleStateMachine| at the end of function, and remove the '''if''' conditiona and comment to make it simplier and clear.
> 
> What do you think ?

In fact,  ScheduleStateMachine() should be called only when prerolling is done to start playback. However, since SetPlaybackRate() is not called very often, it is fine to call ScheduleStateMachine() unconditionaly without incurring performance overhead. I am fine with that.

Comment 8

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

Comment on attachment 8796429 [details]
Bug 1306186. Part 1 - remove checks for mIsAudioPrerolling and mIsVideoPrerolling from NeedToSkipToNextKeyframe().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82296/diff/1-2/

Comment 9

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

Comment on attachment 8796430 [details]
Bug 1306186. Part 2 - reset prerolling flags in MaybeStartPlayback().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82298/diff/1-2/

Comment 10

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

Comment on attachment 8796431 [details]
Bug 1306186. Part 3 - remove StopPrerolling{Audio,Video}.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82300/diff/1-2/

Comment 11

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

Comment on attachment 8796432 [details]
Bug 1306186. Part 4 - merge mIsAudioPrerolling and mIsVideoPrerolling into mIsPrerolling.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82302/diff/1-2/

Comment 12

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8796430 [details]
Bug 1306186. Part 2 - reset prerolling flags in MaybeStartPlayback().

https://reviewboard.mozilla.org/r/82298/#review81258

Comment 13

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8796431 [details]
Bug 1306186. Part 3 - remove StopPrerolling{Audio,Video}.

https://reviewboard.mozilla.org/r/82300/#review81260

Comment 14

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8796432 [details]
Bug 1306186. Part 4 - merge mIsAudioPrerolling and mIsVideoPrerolling into mIsPrerolling.

https://reviewboard.mozilla.org/r/82302/#review81274

::: dom/media/MediaDecoderStateMachine.cpp:1132
(Diff revision 2)
> -    if ((isAudio && mIsAudioPrerolling) || (!isAudio && mIsVideoPrerolling)) {
> +    if (mIsPrerolling) {
>        // Schedule next cycle to stop prerolling so we can play the frames we've
>        // decoded so far.
>        ScheduleStateMachine();
>      }
>      return;

I like the idea to merge mIs{A/V}prerolling into one. But the code segement above appears in serveral places.  Would it be better to encapsulate it into a funciton, i.e. |MaybeStopPrerolling|, to make it easier to understand at a glane ?


Further, it could be even possible to update the  value of mIsPrerolling in that function when MDSM is in DECODER_STATE_DECODING without another cycle. Though I think it's also fine to update mIsPrerolling in |MaybeStartPlayback|.  Any thoughts ?

Comment 15

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

Comment on attachment 8796432 [details]
Bug 1306186. Part 4 - merge mIsAudioPrerolling and mIsVideoPrerolling into mIsPrerolling.

https://reviewboard.mozilla.org/r/82302/#review81274

> I like the idea to merge mIs{A/V}prerolling into one. But the code segement above appears in serveral places.  Would it be better to encapsulate it into a funciton, i.e. |MaybeStopPrerolling|, to make it easier to understand at a glane ?
> 
> 
> Further, it could be even possible to update the  value of mIsPrerolling in that function when MDSM is in DECODER_STATE_DECODING without another cycle. Though I think it's also fine to update mIsPrerolling in |MaybeStartPlayback|.  Any thoughts ?

I will have patch 5 to do that. Let's have P4 focus on merging the flags.

Comment 16

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

Attachment: Bug 1306186. Part 5 - add MaybeStopPrerolling() to check if we can stop prerolling.

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

Comment 17

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8796432 [details]
Bug 1306186. Part 4 - merge mIsAudioPrerolling and mIsVideoPrerolling into mIsPrerolling.

https://reviewboard.mozilla.org/r/82302/#review81322

Comment 18

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8797039 [details]
Bug 1306186. Part 5 - add MaybeStopPrerolling() to check if we can stop prerolling.

https://reviewboard.mozilla.org/r/82686/#review81324

r+ with following comment addressed.


Patch description "...Part 5 - add MaybeStartPlayback()..." 

should be 

"...Part 5 - add MaybeStopPrerolling()..."

Comment 19

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

Comment on attachment 8797039 [details]
Bug 1306186. Part 5 - add MaybeStopPrerolling() to check if we can stop prerolling.

https://reviewboard.mozilla.org/r/82686/#review81324

Looks like I copied the wrong string. :( Thanks for the catch!

Comment 20

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

Comment on attachment 8797039 [details]
Bug 1306186. Part 5 - add MaybeStopPrerolling() to check if we can stop prerolling.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82686/diff/1-2/

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f0eef1e1acc4
Part 1 - remove checks for mIsAudioPrerolling and mIsVideoPrerolling from NeedToSkipToNextKeyframe(). r=kikuo
https://hg.mozilla.org/integration/autoland/rev/e70e297b2942
Part 2 - reset prerolling flags in MaybeStartPlayback(). r=kikuo
https://hg.mozilla.org/integration/autoland/rev/ea57d7034a0d
Part 3 - remove StopPrerolling{Audio,Video}. r=kikuo
https://hg.mozilla.org/integration/autoland/rev/1510da178dbd
Part 4 - merge mIsAudioPrerolling and mIsVideoPrerolling into mIsPrerolling. r=kikuo
https://hg.mozilla.org/integration/autoland/rev/2b6868859c21
Part 5 - add MaybeStopPrerolling() to check if we can stop prerolling. r=kikuo

Comment 22

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

Attachment: Bug 1306186. Part 6 - check if we can stop prerolling in the entry action of DecodingState.

Chances are we've decoded enough data when entering DecodingState. Otherwis we will never
start playback.

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

Comment 23

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8797111 [details]
Bug 1306186. Part 6 - check if we can stop prerolling in the entry action of DecodingState.

https://reviewboard.mozilla.org/r/82742/#review81352

My bad, didn't caught it in Part 5.

Comment 24

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8797111 [details]
Bug 1306186. Part 6 - check if we can stop prerolling in the entry action of DecodingState.

https://reviewboard.mozilla.org/r/82742/#review81352

s/caught/catch  ...

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d5cc0a54f8b8
Part 1 - remove checks for mIsAudioPrerolling and mIsVideoPrerolling from NeedToSkipToNextKeyframe(). r=kikuo
https://hg.mozilla.org/integration/autoland/rev/613397fea462
Part 2 - reset prerolling flags in MaybeStartPlayback(). r=kikuo
https://hg.mozilla.org/integration/autoland/rev/31b07a151f53
Part 3 - remove StopPrerolling{Audio,Video}. r=kikuo
https://hg.mozilla.org/integration/autoland/rev/b19f904b6127
Part 4 - merge mIsAudioPrerolling and mIsVideoPrerolling into mIsPrerolling. r=kikuo
https://hg.mozilla.org/integration/autoland/rev/0621d50a7455
Part 5 - add MaybeStopPrerolling() to check if we can stop prerolling. r=kikuo
https://hg.mozilla.org/integration/autoland/rev/73e1dac8253d
Part 6 - check if we can stop prerolling in the entry action of DecodingState. r=kikuo

Comment 26

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for timeouts in mda test /test_background_video_suspend.html on Linux and OS X:

https://hg.mozilla.org/integration/autoland/rev/5e44ca63f9d94e5bb9c9f18ecb3c1f91ad90271d
https://hg.mozilla.org/integration/autoland/rev/44d738cc1696e8cad81aa70bb81364741f7e20b8
https://hg.mozilla.org/integration/autoland/rev/8b988a8236a52a19f219f1832070443304d081f9
https://hg.mozilla.org/integration/autoland/rev/710566a1315fb1e39a10d424f39274cae9c4ca79
https://hg.mozilla.org/integration/autoland/rev/04ed7900f6cb2e618918223cdc5f63671dd54520

On a Windows, there is a different, new failure: test_reset_src.html | Test timed out. https://treeherder.mozilla.org/logviewer.html#?job_id=4361838&repo=autoland

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=2b6868859c21af4e24651be7e308fe96f66680cf
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=4359826&repo=autoland

[task 2016-10-03T08:52:21.562049Z] 08:52:21     INFO -  182 INFO [08:47:23.511] gizmo-noaudio.webm-3 Waiting for ended
[task 2016-10-03T08:52:21.563188Z] 08:52:21     INFO -  183 INFO TEST-PASS | dom/media/test/test_background_video_suspend.html | gizmo-noaudio.webm-3 ended
[task 2016-10-03T08:52:21.564331Z] 08:52:21     INFO -  184 INFO [finished gizmo-noaudio.webm-3] remaining= gizmo-noaudio.mp4-1
[task 2016-10-03T08:52:21.565514Z] 08:52:21     INFO -  185 INFO TEST-PASS | dom/media/test/test_background_video_suspend.html | [finished gizmo-noaudio.webm-3 t=20.396] Length of array should match number of running tests
[task 2016-10-03T08:52:21.566643Z] 08:52:21     INFO -  186 INFO TEST-UNEXPECTED-FAIL | dom/media/test/test_background_video_suspend.html | Test timed out.
[task 2016-10-03T08:52:21.568305Z] 08:52:21     INFO -      reportError@SimpleTest/TestRunner.js:114:7
[task 2016-10-03T08:52:21.569459Z] 08:52:21     INFO -      TestRunner._checkForHangs@SimpleTest/TestRunner.js:135:7

Comment 27

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

Part 6 fixes the problem. Can you land P1 ~ P6 again?

Comment 28

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Hi, your latest push didn't get backed out, but the one before. The post of the backout push just happened after you pushed the patch stack with the fix. There should be nothing to be done here (if builds and test will turn out green).

Comment 29

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d5cc0a54f8b8
https://hg.mozilla.org/mozilla-central/rev/613397fea462
https://hg.mozilla.org/mozilla-central/rev/31b07a151f53
https://hg.mozilla.org/mozilla-central/rev/b19f904b6127
https://hg.mozilla.org/mozilla-central/rev/0621d50a7455
https://hg.mozilla.org/mozilla-central/rev/73e1dac8253d