Move DispatchDecodeTasksIfNeeded() out of MDSM::Push()

RESOLVED FIXED in Firefox 53

Status

()

Core
Audio/Video: Playback
P3
normal
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: jwwang, Assigned: jwwang)

Tracking

unspecified
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(7 attachments)

(Assignee)

Description

11 months ago
http://searchfox.org/mozilla-central/rev/cc2a84852bd4e6f6d8d4d5b17b8382bb5d005749/dom/media/MediaDecoderStateMachine.cpp#2694

We should let state objects decide whether to continue decoding after pushing a sample to the queue.

E.g. the seeking state will need at most one audio sample and one video sample to finish seeking.
(Assignee)

Updated

11 months ago
Assignee: nobody → jwwang
Blocks: 1324999
Priority: -- → P3
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 8

11 months ago
mozreview-review
Comment on attachment 8821717 [details]
Bug 1325004. Part 1 - add PushAudio() and PushVideo().

https://reviewboard.mozilla.org/r/100892/#review101512

::: dom/media/MediaDecoderStateMachine.cpp:2562
(Diff revision 1)
> +
> +void
> +MediaDecoderStateMachine::PushVideo(MediaData* aSample)
> +{
> +  MOZ_ASSERT(OnTaskQueue());
> +  aSample->As<VideoData>()->mFrameID = ++mCurrentFrameID;

Though there's alreayd a null check inside MediaQueue::Push, I'd suggest to add MOZ_ASSERT(aSample) for PushAudio and PushVideo in particular, so that we could hit the case if there's any potential A or V null sample as early as possible.
Attachment #8821717 - Flags: review?(kikuo) → review+

Comment 9

11 months ago
mozreview-review
Comment on attachment 8821718 [details]
Bug 1325004. Part 2 - let AccurateSeekingState call Push{Audio,Video} instead.

https://reviewboard.mozilla.org/r/100894/#review101514

::: dom/media/MediaDecoderStateMachine.cpp:956
(Diff revision 1)
>          if (mFirstVideoFrameAfterSeek) {
>            // Hit the end of stream. Move mFirstVideoFrameAfterSeek into
>            // mSeekedVideoData so we have something to display after seeking.
> -          mMaster->Push(mFirstVideoFrameAfterSeek);
> +          mMaster->PushVideo(mFirstVideoFrameAfterSeek);
>          }
> +        VideoQueue().Finish();

Nice move here ! The frame could be displayed ASAP.
Attachment #8821718 - Flags: review?(kikuo) → review+

Comment 10

11 months ago
mozreview-review
Comment on attachment 8821719 [details]
Bug 1325004. Part 3 - let DecodingFirstFrameState call Push{Audio,Video} instead.

https://reviewboard.mozilla.org/r/100896/#review101516
Attachment #8821719 - Flags: review?(kikuo) → review+

Comment 11

11 months ago
mozreview-review
Comment on attachment 8821720 [details]
Bug 1325004. Part 4 - let NextFrameSeekingState call Push{Audio,Video} instead.

https://reviewboard.mozilla.org/r/100898/#review101576
Attachment #8821720 - Flags: review?(kikuo) → review+

Comment 12

11 months ago
mozreview-review
Comment on attachment 8821721 [details]
Bug 1325004. Part 5 - let BufferingState call Push{Audio,Video} instead.

https://reviewboard.mozilla.org/r/100900/#review101578
Attachment #8821721 - Flags: review?(kikuo) → review+

Comment 13

11 months ago
mozreview-review
Comment on attachment 8821722 [details]
Bug 1325004. Part 6 - let DecodingState call Push{Audio,Video} instead.

https://reviewboard.mozilla.org/r/100902/#review101580
Attachment #8821722 - Flags: review?(kikuo) → review+

Comment 14

11 months ago
mozreview-review
Comment on attachment 8821723 [details]
Bug 1325004. Part 7 - remove Push().

https://reviewboard.mozilla.org/r/100904/#review101582
Attachment #8821723 - Flags: review?(kikuo) → review+
(Assignee)

Comment 15

11 months ago
Thanks for the review!
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 23

11 months ago
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/96a7a5fbf4c7
Part 1 - add PushAudio() and PushVideo(). r=kikuo
https://hg.mozilla.org/integration/autoland/rev/539597822ce4
Part 2 - let AccurateSeekingState call Push{Audio,Video} instead. r=kikuo
https://hg.mozilla.org/integration/autoland/rev/e98a9c9afc04
Part 3 - let DecodingFirstFrameState call Push{Audio,Video} instead. r=kikuo
https://hg.mozilla.org/integration/autoland/rev/40c6d295a18a
Part 4 - let NextFrameSeekingState call Push{Audio,Video} instead. r=kikuo
https://hg.mozilla.org/integration/autoland/rev/63f638b93222
Part 5 - let BufferingState call Push{Audio,Video} instead. r=kikuo
https://hg.mozilla.org/integration/autoland/rev/91de75a6fa7b
Part 6 - let DecodingState call Push{Audio,Video} instead. r=kikuo
https://hg.mozilla.org/integration/autoland/rev/7a09d7223cf6
Part 7 - remove Push(). r=kikuo

Comment 24

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/96a7a5fbf4c7
https://hg.mozilla.org/mozilla-central/rev/539597822ce4
https://hg.mozilla.org/mozilla-central/rev/e98a9c9afc04
https://hg.mozilla.org/mozilla-central/rev/40c6d295a18a
https://hg.mozilla.org/mozilla-central/rev/63f638b93222
https://hg.mozilla.org/mozilla-central/rev/91de75a6fa7b
https://hg.mozilla.org/mozilla-central/rev/7a09d7223cf6
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.