1283370 - Some NextFrameSeekTask code cleanup

Status: RESOLVED FIXED

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

Comment 1

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

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

Comment 2

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Mass change P2 -> P3

Comment 3

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

Attachment: Bug 1283370. Part 1 - remove unnecessary checks for Exists().

1. On{Audio,Video}[Not]Decoded() happens after |return mSeekTaskPromise.Ensure(__func__)|
   in Seek(). mSeekTaskPromise is non-empty and Exists() will return true.
2. Is{Audio,Video}SeekComplete() is called indirectly from On{Audio,Video}[Not]Decoded()
   where Exists() is guaranteed to be true.

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

Comment 4

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

Attachment: Bug 1283370. Part 2 - remove unnecessary checks for HasVideo() for we assert it in the constructor.

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

Comment 5

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

Attachment: Bug 1283370. Part 3 - remove unused HasVideo().

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

Comment 6

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

Attachment: Bug 1283370. Part 4 - remove HasAudio().

We guarantee to have |!mReader->IsRequestingAudioData() && !mReader->IsWaitingAudioData()|
when there is no audio at all. We can remove the check for HasAudio().

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

Comment 7

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

Attachment: Bug 1283370. Part 5 - make EnsureVideoDecodeTaskQueued() return void.

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

Comment 8

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

Attachment: Bug 1283370. Part 6 - remove asserting |mReader->IsWaitForDataSupported()| for mReader->WaitForData() will crash if the method has no override.

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

Comment 9

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

Attachment: Bug 1283370. Part 7 - remove setting mNeedToStopPrerollingVideo per discussion in https://reviewboard.mozilla.org/r/43689/#comment54421.

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

Comment 10

[Tzuhao Kuo [:kaku]]

Comment on attachment 8768292 [details]
Bug 1283370. Part 1 - remove unnecessary checks for Exists().

https://reviewboard.mozilla.org/r/62606/#review59596

Comment 11

[Tzuhao Kuo [:kaku]]

Comment on attachment 8768293 [details]
Bug 1283370. Part 2 - remove unnecessary checks for HasVideo() for we assert it in the constructor.

https://reviewboard.mozilla.org/r/62608/#review59598

Comment 12

[Tzuhao Kuo [:kaku]]

https://reviewboard.mozilla.org/r/62608/#review59600

Comment 13

[Tzuhao Kuo [:kaku]]

Comment on attachment 8768294 [details]
Bug 1283370. Part 3 - remove unused HasVideo().

https://reviewboard.mozilla.org/r/62610/#review59602

Comment 14

[Tzuhao Kuo [:kaku]]

Comment on attachment 8768295 [details]
Bug 1283370. Part 4 - remove HasAudio().

https://reviewboard.mozilla.org/r/62612/#review59606

Comment 15

[Tzuhao Kuo [:kaku]]

Comment on attachment 8768296 [details]
Bug 1283370. Part 5 - make EnsureVideoDecodeTaskQueued() return void.

https://reviewboard.mozilla.org/r/62614/#review59608

Comment 16

[Tzuhao Kuo [:kaku]]

Comment on attachment 8768297 [details]
Bug 1283370. Part 6 - remove asserting |mReader->IsWaitForDataSupported()| for mReader->WaitForData() will crash if the method has no override.

https://reviewboard.mozilla.org/r/62616/#review59610

Comment 17

[Tzuhao Kuo [:kaku]]

Comment on attachment 8768298 [details]
Bug 1283370. Part 7 - remove setting mNeedToStopPrerollingVideo per discussion in https://reviewboard.mozilla.org/r/43689/#comment54421.

https://reviewboard.mozilla.org/r/62618/#review59612

Comment 18

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

Thanks for the review!

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f08c25ed6e87
Part 1 - remove unnecessary checks for Exists(). r=kaku
https://hg.mozilla.org/integration/autoland/rev/933fa4644790
Part 2 - remove unnecessary checks for HasVideo() for we assert it in the constructor. r=kaku
https://hg.mozilla.org/integration/autoland/rev/030c194e154d
Part 3 - remove unused HasVideo(). r=kaku
https://hg.mozilla.org/integration/autoland/rev/bba3e109af22
Part 4 - remove HasAudio(). r=kaku
https://hg.mozilla.org/integration/autoland/rev/29b0c523b59a
Part 5 - make EnsureVideoDecodeTaskQueued() return void. r=kaku
https://hg.mozilla.org/integration/autoland/rev/86697ab137dc
Part 6 - remove asserting |mReader->IsWaitForDataSupported()| for mReader->WaitForData() will crash if the method has no override. r=kaku
https://hg.mozilla.org/integration/autoland/rev/989bd85d2f8e
Part 7 - remove setting mNeedToStopPrerollingVideo per discussion in https://reviewboard.mozilla.org/r/43689/#comment54421. r=kaku

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f08c25ed6e87
https://hg.mozilla.org/mozilla-central/rev/933fa4644790
https://hg.mozilla.org/mozilla-central/rev/030c194e154d
https://hg.mozilla.org/mozilla-central/rev/bba3e109af22
https://hg.mozilla.org/mozilla-central/rev/29b0c523b59a
https://hg.mozilla.org/mozilla-central/rev/86697ab137dc
https://hg.mozilla.org/mozilla-central/rev/989bd85d2f8e