Closed
Bug 1340969
Opened 7 years ago
Closed 7 years ago
Add a class to manage shutdown process of decoders for MFR
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
(2 files)
This addresses concern 1 of bug 1340940.
Assignee | ||
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8839336 -
Flags: review?(jyavenard)
Attachment #8839337 -
Flags: review?(jyavenard)
Assignee | ||
Comment 3•7 years ago
|
||
Note P1 happens to fix bug 1340580 by removing Data::mShutdownRequest.
See Also: → 1340580
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8839336 [details] Bug 1340969. Part 1 - add ShutdownPromisePool to manage ShutdownPromises. https://reviewboard.mozilla.org/r/114008/#review115544 very nice thank you... couldn't have done it better. ::: dom/media/MediaFormatReader.cpp:322 (Diff revision 1) > +class MediaFormatReader::ShutdownPromisePool > +{ > +public: > + ShutdownPromisePool() > + : mOnShutdownComplete(new ShutdownPromise::Private(__func__)) > + { } { } on two lines ::: dom/media/MediaFormatReader.cpp:333 (Diff revision 1) > + > + // Track a shutdown promise. > + void Track(RefPtr<ShutdownPromise> aPromise); > + > + // Shut down a decoder and track its shutdown promise. > + void ShutdownDecoder(RefPtr<MediaDataDecoder> aDecoder) We're only ever passing an already_AddRefed<MediaDataDecoder> shouldn't we change the prototype of the function to reflect that? ::: dom/media/MediaFormatReader.cpp:397 (Diff revision 1) > auto& data = aTrack == TrackInfo::kAudioTrack ? mAudio : mVideo; > data.mPolicy->Cancel(); > data.mTokenRequest.DisconnectIfExists(); > data.mInitRequest.DisconnectIfExists(); > - if (!data.mDecoder) { > - return ShutdownPromise::CreateAndResolve(true, __func__); > + if (data.mDecoder) { > + mOwner->mShutdownPromisePool->ShutdownDecoder(data.mDecoder.forget()); oh, that's what it was that caused the intermittent assertion, data.mState = Stage::None here.
Attachment #8839336 -
Flags: review?(jyavenard) → review+
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8839337 [details] Bug 1340969. Part 2 - remove MFR::mShutdownPromise by using promise chaining. https://reviewboard.mozilla.org/r/114010/#review115550
Attachment #8839337 -
Flags: review?(jyavenard) → review+
Assignee | ||
Comment 6•7 years ago
|
||
Thanks for the reviews!
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 11•7 years ago
|
||
Pushed by jwwang@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3af20242d67e Part 1 - add ShutdownPromisePool to manage ShutdownPromises. r=jya https://hg.mozilla.org/integration/autoland/rev/eead03c9de72 Part 2 - remove MFR::mShutdownPromise by using promise chaining. r=jya
Comment 13•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3af20242d67e https://hg.mozilla.org/mozilla-central/rev/eead03c9de72
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
•