1340969 - Add a class to manage shutdown process of decoders for MFR

Status: RESOLVED FIXED

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

Description

[JW Wang [:jwwang] [:jw_wang]]

This addresses concern 1 of bug 1340940.

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1340969. Part 1 - add ShutdownPromisePool to manage ShutdownPromises.

Review commit: https://reviewboard.mozilla.org/r/114008/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/114008/

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1340969. Part 2 - remove MFR::mShutdownPromise by using promise chaining.

Review commit: https://reviewboard.mozilla.org/r/114010/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/114010/

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

Note P1 happens to fix bug 1340580 by removing Data::mShutdownRequest.

Comment 4

[Jean-Yves Avenard [:jya]]

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.

Comment 5

[Jean-Yves Avenard [:jya]]

Comment on attachment 8839337 [details]
Bug 1340969. Part 2 - remove MFR::mShutdownPromise by using promise chaining.

https://reviewboard.mozilla.org/r/114010/#review115550

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

Thanks for the reviews!

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8839336 [details]
Bug 1340969. Part 1 - add ShutdownPromisePool to manage ShutdownPromises.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114008/diff/1-2/

Comment 8

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8839337 [details]
Bug 1340969. Part 2 - remove MFR::mShutdownPromise by using promise chaining.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114010/diff/1-2/

Comment 9

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8839336 [details]
Bug 1340969. Part 1 - add ShutdownPromisePool to manage ShutdownPromises.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114008/diff/1-2/

Comment 10

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8839337 [details]
Bug 1340969. Part 2 - remove MFR::mShutdownPromise by using promise chaining.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114010/diff/1-2/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3af20242d67e
https://hg.mozilla.org/mozilla-central/rev/eead03c9de72