1322800 - Remove NextFrameSeekTask

Status: RESOLVED FIXED

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

Comment 1

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1322800 part 1 - move NextFrameSeekTask::Seek();

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

Comment 2

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1322800 part 2 - move NextFrameSeekTask::CalculateNewCurrentTime();

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

Comment 3

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1322800 part 3 - move NextFrameSeekTask::Handle{Audio,Video,Not}{Decoded,Waited}();

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

Comment 4

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1322800 part 4 - move NextFrameSeekTask::MaybeFinishSeek();

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

Comment 5

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1322800 part 5 - move NextFrameSeekTask::RequestVideoData();

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

Comment 6

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1322800 part 6 - move NextFrameSeekTask::Is{Audio,Video}SeekComplete();

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

Comment 7

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1322800 part 7 - move NextFrameSeekTask::NeedMoreVideo();

rt

rt
hG: Enter commit message.  Lines beginning with 'HG:' are removed.
rt
Hemove the event-based play() method out from nsIDOMHTMLMediaElement.idl (Bug 1299096 - Remove the event-based play() method out from nsIDOMHTMLMediaElement.idl (


rt
rt
hG: Enter commit message.  Lines beginning with 'HG:' are removed.
rt
Hemove the event-based play() method out from nsIDOMHTMLMediaElement.idl (Bug 1299096 - Remove the event-based play() method out from nsIDOMHTMLMediaElement.idl (

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

Comment 8

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1322800 part 8 - move NextFrameSeekTask::IsVideoRequestPending();

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

Comment 9

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1322800 part 9 - move NextFrameSeekTask::NextFrameSeekTask();

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

Comment 10

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1322800 part 10 - use StateObject::{Audio,Video}Queue() to replate NextFrameSeekTask::m{Audio,Video}Queue;

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

Comment 11

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1322800 part 11 - use SeekingState::mSeekJob.mTarget to replace NextFrameSeekTask::mTarget;

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

Comment 12

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1322800 part 12 - move all SeekingState data members;

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

Comment 13

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1322800 part 13 - disconnect NextFrameSeekingState and SeekTask;

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

Comment 14

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1322800 part 14 - remove SeekTask and NextFrameSeekTask;

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

Comment 15

[Tzuhao Kuo [:kaku]]

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc83ab1d21f7b86d24fee83010975509257be0b5
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d68253f298aacc5d4dd4f10c620f10fb84aa6edc

Comment 16

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

Comment on attachment 8819235 [details]
Bug 1322800 part 7 - move NextFrameSeekTask::NeedMoreVideo();

https://reviewboard.mozilla.org/r/98792/#review99330

The commit message contains unwanted lines.

Comment 17

[Tzuhao Kuo [:kaku]]

Comment on attachment 8819235 [details]
Bug 1322800 part 7 - move NextFrameSeekTask::NeedMoreVideo();

https://reviewboard.mozilla.org/r/98792/#review99330

@@ What's that..., will remove them.

Comment 18

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

Comment on attachment 8819242 [details]
Bug 1322800 part 14 - remove SeekTask and NextFrameSeekTask;

https://reviewboard.mozilla.org/r/99080/#review99334

Comment 19

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

Comment on attachment 8819229 [details]
Bug 1322800 part 1 - move NextFrameSeekTask::Seek();

https://reviewboard.mozilla.org/r/98780/#review99336

Comment 20

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

Comment on attachment 8819230 [details]
Bug 1322800 part 2 - move NextFrameSeekTask::CalculateNewCurrentTime();

https://reviewboard.mozilla.org/r/98782/#review99338

Comment 21

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

Comment on attachment 8819231 [details]
Bug 1322800 part 3 - move NextFrameSeekTask::Handle{Audio,Video,Not}{Decoded,Waited}();

https://reviewboard.mozilla.org/r/98784/#review99340

Comment 22

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

Comment on attachment 8819232 [details]
Bug 1322800 part 4 - move NextFrameSeekTask::MaybeFinishSeek();

https://reviewboard.mozilla.org/r/98786/#review99342

Comment 23

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

Comment on attachment 8819233 [details]
Bug 1322800 part 5 - move NextFrameSeekTask::RequestVideoData();

https://reviewboard.mozilla.org/r/98788/#review99344

Comment 24

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

Comment on attachment 8819234 [details]
Bug 1322800 part 6 - move NextFrameSeekTask::Is{Audio,Video}SeekComplete();

https://reviewboard.mozilla.org/r/98790/#review99346

Comment 25

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

Comment on attachment 8819236 [details]
Bug 1322800 part 8 - move NextFrameSeekTask::IsVideoRequestPending();

https://reviewboard.mozilla.org/r/98794/#review99348

Comment 26

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

Comment on attachment 8819237 [details]
Bug 1322800 part 9 - move NextFrameSeekTask::NextFrameSeekTask();

https://reviewboard.mozilla.org/r/98796/#review99350

Comment 27

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

Comment on attachment 8819238 [details]
Bug 1322800 part 10 - use StateObject::{Audio,Video}Queue() to replate NextFrameSeekTask::m{Audio,Video}Queue;

https://reviewboard.mozilla.org/r/98798/#review99352

Comment 28

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

Comment on attachment 8819239 [details]
Bug 1322800 part 11 - use SeekingState::mSeekJob.mTarget to replace NextFrameSeekTask::mTarget;

https://reviewboard.mozilla.org/r/98800/#review99354

Comment 29

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

Comment on attachment 8819240 [details]
Bug 1322800 part 12 - move all SeekingState data members;

https://reviewboard.mozilla.org/r/98802/#review99356

Comment 30

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

Comment on attachment 8819241 [details]
Bug 1322800 part 13 - disconnect NextFrameSeekingState and SeekTask;

https://reviewboard.mozilla.org/r/98804/#review99358

Comment 31

[Tzuhao Kuo [:kaku]]

Comment on attachment 8819241 [details]
Bug 1322800 part 13 - disconnect NextFrameSeekingState and SeekTask;

https://reviewboard.mozilla.org/r/98804/#review99366

::: dom/media/MediaDecoderStateMachine.cpp:1393
(Diff revision 1)
> +
> +    NS_IMETHOD Run()
> +    {
> +      if (!mIsCancelled) {
> +        auto currentTime = mStateObj->mCurrentTime;
> +        DiscardFrames(mStateObj->VideoQueue(), [currentTime] (int64_t aSampleTime) {

It seems that VisualStudio does not allow inner class to access protected member of the outer class' parent class.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc83ab1d21f7b86d24fee83010975509257be0b5&selectedJob=32817014

Comment 32

[Tzuhao Kuo [:kaku]]

Comment on attachment 8819235 [details]
Bug 1322800 part 7 - move NextFrameSeekTask::NeedMoreVideo();

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

Comment 33

[Tzuhao Kuo [:kaku]]

Comment on attachment 8819236 [details]
Bug 1322800 part 8 - move NextFrameSeekTask::IsVideoRequestPending();

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

Comment 34

[Tzuhao Kuo [:kaku]]

Comment on attachment 8819237 [details]
Bug 1322800 part 9 - move NextFrameSeekTask::NextFrameSeekTask();

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

Comment 35

[Tzuhao Kuo [:kaku]]

Comment on attachment 8819238 [details]
Bug 1322800 part 10 - use StateObject::{Audio,Video}Queue() to replate NextFrameSeekTask::m{Audio,Video}Queue;

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

Comment 36

[Tzuhao Kuo [:kaku]]

Comment on attachment 8819239 [details]
Bug 1322800 part 11 - use SeekingState::mSeekJob.mTarget to replace NextFrameSeekTask::mTarget;

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

Comment 37

[Tzuhao Kuo [:kaku]]

Comment on attachment 8819240 [details]
Bug 1322800 part 12 - move all SeekingState data members;

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

Comment 38

[Tzuhao Kuo [:kaku]]

Comment on attachment 8819241 [details]
Bug 1322800 part 13 - disconnect NextFrameSeekingState and SeekTask;

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

Comment 39

[Tzuhao Kuo [:kaku]]

Comment on attachment 8819242 [details]
Bug 1322800 part 14 - remove SeekTask and NextFrameSeekTask;

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

Comment 40

[Tzuhao Kuo [:kaku]]

Try again:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f1502a17709e05208c1134bc5239a436eb44952
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bd3d1fe111e58eca70acbf34b0b2f1b3f49e902

Comment 41

[Tzuhao Kuo [:kaku]]

Try looks good, thanks for the review!

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/46ccc52fb0d2
part 1 - move NextFrameSeekTask::Seek(); r=jwwang
https://hg.mozilla.org/integration/autoland/rev/4bb06a1689c0
part 2 - move NextFrameSeekTask::CalculateNewCurrentTime(); r=jwwang
https://hg.mozilla.org/integration/autoland/rev/9d669db8e5de
part 3 - move NextFrameSeekTask::Handle{Audio,Video,Not}{Decoded,Waited}(); r=jwwang
https://hg.mozilla.org/integration/autoland/rev/44a6aa7daa10
part 4 - move NextFrameSeekTask::MaybeFinishSeek(); r=jwwang
https://hg.mozilla.org/integration/autoland/rev/c315df18fcb1
part 5 - move NextFrameSeekTask::RequestVideoData(); r=jwwang
https://hg.mozilla.org/integration/autoland/rev/f508d130f769
part 6 - move NextFrameSeekTask::Is{Audio,Video}SeekComplete(); r=jwwang
https://hg.mozilla.org/integration/autoland/rev/72eee3d1f63e
part 7 - move NextFrameSeekTask::NeedMoreVideo(); r=jwwang
https://hg.mozilla.org/integration/autoland/rev/6d8def90f67c
part 8 - move NextFrameSeekTask::IsVideoRequestPending(); r=jwwang
https://hg.mozilla.org/integration/autoland/rev/5d46e89683d4
part 9 - move NextFrameSeekTask::NextFrameSeekTask(); r=jwwang
https://hg.mozilla.org/integration/autoland/rev/4c25211782ff
part 10 - use StateObject::{Audio,Video}Queue() to replate NextFrameSeekTask::m{Audio,Video}Queue; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/704a929bcd95
part 11 - use SeekingState::mSeekJob.mTarget to replace NextFrameSeekTask::mTarget; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/2cd7cbf856db
part 12 - move all SeekingState data members; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/da2d76e10cbf
part 13 - disconnect NextFrameSeekingState and SeekTask; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/77c3f2e77448
part 14 - remove SeekTask and NextFrameSeekTask; r=jwwang

Comment 43

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/46ccc52fb0d2
https://hg.mozilla.org/mozilla-central/rev/4bb06a1689c0
https://hg.mozilla.org/mozilla-central/rev/9d669db8e5de
https://hg.mozilla.org/mozilla-central/rev/44a6aa7daa10
https://hg.mozilla.org/mozilla-central/rev/c315df18fcb1
https://hg.mozilla.org/mozilla-central/rev/f508d130f769
https://hg.mozilla.org/mozilla-central/rev/72eee3d1f63e
https://hg.mozilla.org/mozilla-central/rev/6d8def90f67c
https://hg.mozilla.org/mozilla-central/rev/5d46e89683d4
https://hg.mozilla.org/mozilla-central/rev/4c25211782ff
https://hg.mozilla.org/mozilla-central/rev/704a929bcd95
https://hg.mozilla.org/mozilla-central/rev/2cd7cbf856db
https://hg.mozilla.org/mozilla-central/rev/da2d76e10cbf
https://hg.mozilla.org/mozilla-central/rev/77c3f2e77448

Comment 44

[Joel Maher ( :jmaher ) (UTC -8)]

thanks for landing this, on debug builds of linux there is an improvement in the measurement of num_constructors:
== Change summary for alert #4546 (as of December 18 2016 03:36 UTC) ==

Improvements:

  1%  compiler_metrics num_constructors linux32 debug     179 -> 178
  1%  compiler_metrics num_constructors linux64 debug     179 -> 178

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=4546