Closed
Bug 1341200
Opened 7 years ago
Closed 7 years ago
Use LocalAllocPolicy and ShutdownPromisePool to streamline DecoderData::ShutdownDecoder()
Categories
(Core :: Audio/Video: Playback, defect, P3)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: jwwang, Assigned: jwwang)
References
Details
Attachments
(10 files)
59 bytes,
text/x-review-board-request
|
jya
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jya
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jya
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jya
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jya
:
review+
|
Details |
Bug 1341200. Part 6 - let ShutdownDecoderWithPromise() return void by tracking the shutdown promise.
59 bytes,
text/x-review-board-request
|
jya
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jya
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jya
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jya
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jya
:
review+
|
Details |
Once bug 1340940 is done, we have LocalAllocPolicy and ShutdownPromisePool to manage the order of decoder creation/shutdown and we are able to remove MFR::DecoderData::mShutdownPromise/mShutdownRequest. DecoderData::ShutdownDecoder() will feel like a sync function and MFR can create a new decoder without waiting for the previous one to shut down completely by using shutdown promises.
Assignee | ||
Updated•7 years ago
|
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8841415 -
Flags: review?(jyavenard)
Attachment #8841416 -
Flags: review?(jyavenard)
Attachment #8841417 -
Flags: review?(jyavenard)
Attachment #8841418 -
Flags: review?(jyavenard)
Attachment #8841419 -
Flags: review?(jyavenard)
Attachment #8841420 -
Flags: review?(jyavenard)
Attachment #8841421 -
Flags: review?(jyavenard)
Attachment #8841422 -
Flags: review?(jyavenard)
Attachment #8841423 -
Flags: review?(jyavenard)
Attachment #8841424 -
Flags: review?(jyavenard)
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8841418 [details] Bug 1341200. Part 4 - remove the check that is unnecessary. https://reviewboard.mozilla.org/r/115642/#review117056 ::: dom/media/MediaFormatReader.h:356 (Diff revision 1) > mDrainState = DrainState::None; > mOutput.Clear(); > mNumSamplesInput = 0; > mNumSamplesOutput = 0; > mSizeOfQueue = 0; > - if (mDecoder && !mFlushed) { > + if (mDecoder) { How is that unecessary?
Comment 12•7 years ago
|
||
I won't review this until next week being on PTO Disclaimer: I see this as yet another unnecessary rewrite...
Assignee | ||
Comment 13•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8841418 [details] Bug 1341200. Part 4 - remove the check that is unnecessary. https://reviewboard.mozilla.org/r/115642/#review117056 > How is that unecessary? We get here only when |!mFlushing && !mFlushed|, so we don't need to check |!mFlushed| again.
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8841415 [details] Bug 1341200. Part 1 - satisfy least privilege principle by capturing |mSamplesWaitingForKey| instead of |this|. https://reviewboard.mozilla.org/r/115636/#review118196
Attachment #8841415 -
Flags: review?(jyavenard) → review+
Comment 15•7 years ago
|
||
mozreview-review |
Comment on attachment 8841416 [details] Bug 1341200. Part 2 - replace mFlushRequest with a bool for mFlushRequest.Disconnect() is never used and a bool is sufficient to do the job. https://reviewboard.mozilla.org/r/115638/#review118198 ::: dom/media/MediaFormatReader.h:267 (Diff revision 1) > return mWaitingForData || mWaitingForKey; > } > > // MediaDataDecoder handler's variables. > MozPromiseRequestHolder<MediaDataDecoder::DecodePromise> mDecodeRequest; > - MozPromiseRequestHolder<MediaDataDecoder::FlushPromise> mFlushRequest; > + bool mFlushing = false; // True if flush is in action. initialisation is a bit inconsistent with the rest of the code; nothing else is initialised the C++11 way. so please initialise in the constructor; you cna create another patch to move everything C++11
Attachment #8841416 -
Flags: review?(jyavenard) → review+
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8841417 [details] Bug 1341200. Part 3 - replace mShutdownRequest with a bool as P2. https://reviewboard.mozilla.org/r/115640/#review118200
Attachment #8841417 -
Flags: review?(jyavenard) → review+
Comment 17•7 years ago
|
||
mozreview-review |
Comment on attachment 8841418 [details] Bug 1341200. Part 4 - remove the check that is unnecessary. https://reviewboard.mozilla.org/r/115642/#review118202
Attachment #8841418 -
Flags: review?(jyavenard) → review+
Comment 18•7 years ago
|
||
mozreview-review |
Comment on attachment 8841419 [details] Bug 1341200. Part 5 - move the definition to .cpp as they will access ShutdownPromisePool in next patches. https://reviewboard.mozilla.org/r/115644/#review118204
Attachment #8841419 -
Flags: review?(jyavenard) → review+
Comment 19•7 years ago
|
||
mozreview-review |
Comment on attachment 8841420 [details] Bug 1341200. Part 6 - let ShutdownDecoderWithPromise() return void by tracking the shutdown promise. https://reviewboard.mozilla.org/r/115646/#review118206 ::: dom/media/MediaFormatReader.h:538 (Diff revision 1) > > // A flag indicating if the start time is known or not. > bool mHasStartTime = false; > > void ShutdownDecoder(TrackType aTrack); > - RefPtr<ShutdownPromise> ShutdownDecoderWithPromise(TrackType aTrack); > + void ShutdownDecoderWithPromise(TrackType aTrack); the name is no longer accurate or representative of what's being done. There's no promise there anymore ::: dom/media/MediaFormatReader.cpp:1174 (Diff revision 1) > } > > if (decoder.mFlushing || decoder.mShuttingDown) { > // Let the current flush or shutdown operation complete, Flush will continue > // shutting down the current decoder now that the shutdown promise is set. > - return decoder.mShutdownPromise.Ensure(__func__); > + mShutdownPromisePool->Track(decoder.mShutdownPromise.Ensure(__func__)); This doesn't work the way it used to work. decoder.mShutdownPromise.Ensure(__func__) when the decoder was currently being shut down would have returned the same promise (as Ensure returns the currently pending promise if one or create a new one if none). So here you may potentially track the same promise twice. How will the ShutdownPromisePool handle this condition?
Attachment #8841420 -
Flags: review?(jyavenard) → review+
Comment 20•7 years ago
|
||
mozreview-review |
Comment on attachment 8841421 [details] Bug 1341200. Part 7 - merge ShutdownDecoderWithPromise() and ShutdownDecoder(). https://reviewboard.mozilla.org/r/115648/#review118208
Attachment #8841421 -
Flags: review?(jyavenard) → review+
Comment 21•7 years ago
|
||
mozreview-review |
Comment on attachment 8841422 [details] Bug 1341200. Part 8 - let DecoderData::ShutdownDecoder() handle shutdown in the middle of flush. https://reviewboard.mozilla.org/r/115650/#review118222 ::: dom/media/MediaFormatReader.cpp:392 (Diff revision 1) > + } > + return; > + } > + > + if (!mShutdownPromise.IsEmpty()) { > + // This is called from the resolve/reject function of Flush. wouldn't it be more readable if this part was moved within ::Flush() ? ::: dom/media/MediaFormatReader.cpp:405 (Diff revision 1) > + // mShutdownPromisePool will handle the order of decoder shutdown so > + // we can forget mDecoder and be ready to create a new one. > mDecoder = nullptr; > + mDescription = "shutdown"; > + mOwner->ScheduleUpdate(mType == MediaData::AUDIO_DATA > + ? TrackType::kAudioTrack ? is to be aligned with the previous operand. So no 2 spaces indent
Attachment #8841422 -
Flags: review?(jyavenard) → review+
Comment 22•7 years ago
|
||
mozreview-review |
Comment on attachment 8841423 [details] Bug 1341200. Part 9 - remove unused mShuttingDown. https://reviewboard.mozilla.org/r/115652/#review118232
Attachment #8841423 -
Flags: review?(jyavenard) → review+
Comment 23•7 years ago
|
||
mozreview-review |
Comment on attachment 8841424 [details] Bug 1341200. Part 10 - streamline DecoderData::ShutdownDecoder() so it feels like a sync function and MFR doesn't need to explicitly wait for flush/shutdown to complete before creating new decoders. https://reviewboard.mozilla.org/r/115654/#review118234
Attachment #8841424 -
Flags: review?(jyavenard) → review+
Assignee | ||
Comment 24•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8841420 [details] Bug 1341200. Part 6 - let ShutdownDecoderWithPromise() return void by tracking the shutdown promise. https://reviewboard.mozilla.org/r/115646/#review118206 > the name is no longer accurate or representative of what's being done. > > There's no promise there anymore This function will be merged in next patch soon. > This doesn't work the way it used to work. > > decoder.mShutdownPromise.Ensure(__func__) when the decoder was currently being shut down would have returned the same promise (as Ensure returns the currently pending promise if one or create a new one if none). > > So here you may potentially track the same promise twice. > How will the ShutdownPromisePool handle this condition? http://searchfox.org/mozilla-central/rev/546a05fec017cb785fca62a5199d42812a6a1fd3/dom/media/MediaFormatReader.cpp#360 ShutdownPromisePool::Track() asserts the same promise is not tracked twice. The current call flow of MFR ensure this won't happen otherwise the assertion will notify us.
Assignee | ||
Comment 25•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8841422 [details] Bug 1341200. Part 8 - let DecoderData::ShutdownDecoder() handle shutdown in the middle of flush. https://reviewboard.mozilla.org/r/115650/#review118222 > wouldn't it be more readable if this part was moved within ::Flush() ? Then we will also need to move |mDecoder = nullptr; mDescription = "shutdown"; mOwner->ScheduleUpdate();| into Flush(). Since this if branch is going to be removed in next patch, I will just leave it.
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 36•7 years ago
|
||
Thanks for the reviews!
Comment 37•7 years ago
|
||
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s 0715cfe9f86a -d eb6b0553d079: rebasing 379329:0715cfe9f86a "Bug 1341200. Part 1 - satisfy least privilege principle by capturing |mSamplesWaitingForKey| instead of |this|. r=jya" rebasing 379330:54a3e07e64bf "Bug 1341200. Part 2 - replace mFlushRequest with a bool for mFlushRequest.Disconnect() is never used and a bool is sufficient to do the job. r=jya" merging dom/media/MediaFormatReader.cpp merging dom/media/MediaFormatReader.h warning: conflicts while merging dom/media/MediaFormatReader.cpp! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 48•7 years ago
|
||
Pushed by jwwang@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/fead77db2529 Part 1 - satisfy least privilege principle by capturing |mSamplesWaitingForKey| instead of |this|. r=jya https://hg.mozilla.org/integration/autoland/rev/e9fc566fda85 Part 2 - replace mFlushRequest with a bool for mFlushRequest.Disconnect() is never used and a bool is sufficient to do the job. r=jya https://hg.mozilla.org/integration/autoland/rev/5f0dca380e84 Part 3 - replace mShutdownRequest with a bool as P2. r=jya https://hg.mozilla.org/integration/autoland/rev/e353098c37d2 Part 4 - remove the check that is unnecessary. r=jya https://hg.mozilla.org/integration/autoland/rev/fb30eeb72b85 Part 5 - move the definition to .cpp as they will access ShutdownPromisePool in next patches. r=jya https://hg.mozilla.org/integration/autoland/rev/5abf28d47683 Part 6 - let ShutdownDecoderWithPromise() return void by tracking the shutdown promise. r=jya https://hg.mozilla.org/integration/autoland/rev/4acb3ce34d94 Part 7 - merge ShutdownDecoderWithPromise() and ShutdownDecoder(). r=jya https://hg.mozilla.org/integration/autoland/rev/161f4237b402 Part 8 - let DecoderData::ShutdownDecoder() handle shutdown in the middle of flush. r=jya https://hg.mozilla.org/integration/autoland/rev/28e9aedd34dc Part 9 - remove unused mShuttingDown. r=jya https://hg.mozilla.org/integration/autoland/rev/bcc6b0e45020 Part 10 - streamline DecoderData::ShutdownDecoder() so it feels like a sync function and MFR doesn't need to explicitly wait for flush/shutdown to complete before creating new decoders. r=jya
Comment 49•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fead77db2529 https://hg.mozilla.org/mozilla-central/rev/e9fc566fda85 https://hg.mozilla.org/mozilla-central/rev/5f0dca380e84 https://hg.mozilla.org/mozilla-central/rev/e353098c37d2 https://hg.mozilla.org/mozilla-central/rev/fb30eeb72b85 https://hg.mozilla.org/mozilla-central/rev/5abf28d47683 https://hg.mozilla.org/mozilla-central/rev/4acb3ce34d94 https://hg.mozilla.org/mozilla-central/rev/161f4237b402 https://hg.mozilla.org/mozilla-central/rev/28e9aedd34dc https://hg.mozilla.org/mozilla-central/rev/bcc6b0e45020
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•