Closed Bug 1340943 Opened 6 years ago Closed 6 years ago

Add a class to sequence the order of decoder creation and shutdown for MFR

Categories

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

defect

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: nobody → jwwang
Blocks: 1340940
Priority: -- → P3
Attachment #8839061 - Flags: review?(jyavenard)
Attachment #8839062 - Flags: review?(jyavenard)
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 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+
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. :)
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.
Thanks for the reviews!
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
https://hg.mozilla.org/mozilla-central/rev/a882ebcaba28
https://hg.mozilla.org/mozilla-central/rev/7453e21a7a3b
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.