Closed
Bug 1340943
Opened 7 years ago
Closed 7 years ago
Add a class to sequence the order of decoder creation and shutdown 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 address concern 2 of bug 1340940.
Assignee | ||
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8839061 -
Flags: review?(jyavenard)
Attachment #8839062 -
Flags: review?(jyavenard)
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8839061 [details] Bug 1340943. Part 1 - rename DecoderAllocPolicy to GlobalAllocPolicy. https://reviewboard.mozilla.org/r/113812/#review115372 this isn't used anywhere else but the MFR to allocate a decoder. So why the need to change the name?
Attachment #8839061 -
Flags: review?(jyavenard) → review+
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8839062 [details] Bug 1340943. Part 2 - add LocalAllocPolicy which enforces the order of decoder creation and shutdown. https://reviewboard.mozilla.org/r/113814/#review115376 Should the other bugs related to the decoder wrapper not being cleaned up be made a duplicate of this ? Still need to set the State when ShutdownDecoder is called. ::: dom/media/MediaFormatReader.cpp:193 (Diff revision 1) > } > > +/** > + * This class addresses the concern of bug 1339310 comment 4 where the Widevine > + * CDM doesn't support running multiple instances of a video decoder at once per > + * CDM instance by sequencing the order of decoder creattion and shutdown. Note creation ::: dom/media/MediaFormatReader.cpp:195 (Diff revision 1) > +/** > + * This class addresses the concern of bug 1339310 comment 4 where the Widevine > + * CDM doesn't support running multiple instances of a video decoder at once per > + * CDM instance by sequencing the order of decoder creattion and shutdown. Note > + * this class addresses a different concern from that of GlobalAllocPolicy which > + * controls a system-wise number of decoders while this class control a per-MFR system-wide ::: dom/media/MediaFormatReader.cpp:208 (Diff revision 1) > + > + NS_INLINE_DECL_THREADSAFE_REFCOUNTING(LocalAllocPolicy) > + > +public: > + LocalAllocPolicy(TrackType aTrack, TaskQueue* aOwnerThread) > + : mTrack(aTrack) , mOwnerThread(aOwnerThread) { } One member initialisation per line. { } on a new line ::: dom/media/MediaFormatReader.cpp:227 (Diff revision 1) > + * An RAII class to manage LocalAllocPolicy::mDecoderLimit. > + */ > + class AutoDeallocToken : public Token > + { > + public: > + explicit AutoDeallocToken(LocalAllocPolicy* aOwner) : mOwner(aOwner) : mOwner(aOwner) on its own line ::: dom/media/MediaFormatReader.cpp:263 (Diff revision 1) > + MozPromiseHolder<Promise> mPendingPromise; > + MozPromiseRequestHolder<Promise> mTokenRequest; > +}; > + > +auto > +LocalAllocPolicy::Alloc() -> RefPtr<Promise> Why this definition style? looks awkward and so different from the rest. All this to avoid typing the fully qualified type name ::: dom/media/MediaFormatReader.cpp:328 (Diff revision 1) > RefPtr<ShutdownPromise> ShutdownDecoder(TrackType aTrack) > { > MOZ_ASSERT(aTrack == TrackInfo::kAudioTrack > || aTrack == TrackInfo::kVideoTrack); > auto& data = aTrack == TrackInfo::kAudioTrack ? mAudio : mVideo; > + data.mPolicy->Cancel(); shouldn't this return a promise so that when the ShutdownPromise returned by the ShudownDecoder resolve, we're guaranteed that the taskqueue won't be used again. We don't get that certainty otherwise.
Attachment #8839062 -
Flags: review?(jyavenard) → review+
Assignee | ||
Comment 5•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8839061 [details] Bug 1340943. Part 1 - rename DecoderAllocPolicy to GlobalAllocPolicy. https://reviewboard.mozilla.org/r/113812/#review115372 For we will have LocalAllocPolicy in P2. :)
Assignee | ||
Comment 6•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8839062 [details] Bug 1340943. Part 2 - add LocalAllocPolicy which enforces the order of decoder creation and shutdown. https://reviewboard.mozilla.org/r/113814/#review115376 Yes, that will be fixed in bug 1340969. > shouldn't this return a promise so that when the ShutdownPromise returned by the ShudownDecoder resolve, we're guaranteed that the taskqueue won't be used again. > > We don't get that certainty otherwise. No. Cancel() calls |mTokenRequest.DisconnectIfExists()| which prevents resolve/reject functions from being called in the future in a synchronous way.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•7 years ago
|
||
Thanks for the reviews!
Comment 10•7 years ago
|
||
Pushed by jwwang@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a882ebcaba28 Part 1 - rename DecoderAllocPolicy to GlobalAllocPolicy. r=jya https://hg.mozilla.org/integration/autoland/rev/7453e21a7a3b Part 2 - add LocalAllocPolicy which enforces the order of decoder creation and shutdown. r=jya
Comment 11•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a882ebcaba28 https://hg.mozilla.org/mozilla-central/rev/7453e21a7a3b
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
•