Closed Bug 1340969 Opened 3 years ago Closed 3 years ago

Add a class to manage shutdown process of decoders 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 addresses concern 1 of bug 1340940.
Assignee: nobody → jwwang
Blocks: 1340940
Priority: -- → P3
Attachment #8839336 - Flags: review?(jyavenard)
Attachment #8839337 - Flags: review?(jyavenard)
Note P1 happens to fix bug 1340580 by removing Data::mShutdownRequest.
See Also: → 1340580
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 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+
Thanks for the reviews!
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
Duplicate of this bug: 1340580
https://hg.mozilla.org/mozilla-central/rev/3af20242d67e
https://hg.mozilla.org/mozilla-central/rev/eead03c9de72
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Duplicate of this bug: 1338612
Duplicate of this bug: 1337265
You need to log in before you can comment on or make changes to this bug.