Closed Bug 1283370 Opened 7 years ago Closed 7 years ago

Some NextFrameSeekTask code cleanup

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(7 files)

No description provided.
Assignee: nobody → jwwang
Blocks: 1283751
Mass change P2 -> P3
Priority: P2 → P3
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/
Attachment #8768292 - Flags: review?(kaku)
Attachment #8768293 - Flags: review?(kaku)
Attachment #8768294 - Flags: review?(kaku)
Attachment #8768295 - Flags: review?(kaku)
Attachment #8768296 - Flags: review?(kaku)
Attachment #8768297 - Flags: review?(kaku)
Attachment #8768298 - Flags: review?(kaku)
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/
Attachment #8768292 - Flags: review?(kaku) → review+
Comment on attachment 8768292 [details]
Bug 1283370. Part 1 - remove unnecessary checks for Exists().

https://reviewboard.mozilla.org/r/62606/#review59596
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
Attachment #8768293 - Flags: review?(kaku) → review+
Comment on attachment 8768294 [details]
Bug 1283370. Part 3 - remove unused HasVideo().

https://reviewboard.mozilla.org/r/62610/#review59602
Attachment #8768294 - Flags: review?(kaku) → review+
Comment on attachment 8768295 [details]
Bug 1283370. Part 4 - remove HasAudio().

https://reviewboard.mozilla.org/r/62612/#review59606
Attachment #8768295 - Flags: review?(kaku) → review+
Attachment #8768296 - Flags: review?(kaku) → review+
Comment on attachment 8768296 [details]
Bug 1283370. Part 5 - make EnsureVideoDecodeTaskQueued() return void.

https://reviewboard.mozilla.org/r/62614/#review59608
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
Attachment #8768297 - Flags: review?(kaku) → review+
Attachment #8768298 - Flags: review?(kaku) → review+
Thanks for the review!
Pushed by jwwang@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.