Closed Bug 1306186 Opened 5 years ago Closed 5 years ago

Merge MDSM::mIsVideoPrerolling and MDSM::mIsAudioPrerolling

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(6 files)

We just need one bool flag to decide whether MDSM is still prerolling.
Assignee: nobody → jwwang
Blocks: 1295892
Priority: -- → P3
Attachment #8796429 - Flags: review?(kikuo)
Attachment #8796430 - Flags: review?(kikuo)
Attachment #8796431 - Flags: review?(kikuo)
Attachment #8796432 - Flags: review?(kikuo)
Comment on attachment 8796429 [details]
Bug 1306186. Part 1 - remove checks for mIsAudioPrerolling and mIsVideoPrerolling from NeedToSkipToNextKeyframe().

https://reviewboard.mozilla.org/r/82296/#review80930
Attachment #8796429 - Flags: review?(kikuo) → review+
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 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 on attachment 8796430 [details]
Bug 1306186. Part 2 - reset prerolling flags in MaybeStartPlayback().

https://reviewboard.mozilla.org/r/82298/#review81258
Attachment #8796430 - Flags: review?(kikuo) → review+
Comment on attachment 8796431 [details]
Bug 1306186. Part 3 - remove StopPrerolling{Audio,Video}.

https://reviewboard.mozilla.org/r/82300/#review81260
Attachment #8796431 - Flags: review?(kikuo) → review+
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 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.
Attachment #8797039 - Flags: review?(kikuo)
Comment on attachment 8796432 [details]
Bug 1306186. Part 4 - merge mIsAudioPrerolling and mIsVideoPrerolling into mIsPrerolling.

https://reviewboard.mozilla.org/r/82302/#review81322
Attachment #8796432 - Flags: review?(kikuo) → review+
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()..."
Attachment #8797039 - Flags: review?(kikuo) → review+
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!
Pushed by jwwang@mozilla.com:
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
Attachment #8797111 - Flags: review?(kikuo)
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.
Attachment #8797111 - Flags: review?(kikuo) → review+
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  ...
Pushed by jwwang@mozilla.com:
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
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
Part 6 fixes the problem. Can you land P1 ~ P6 again?
Flags: needinfo?(aryx.bugmail)
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).
Flags: needinfo?(aryx.bugmail)
You need to log in before you can comment on or make changes to this bug.