Closed Bug 1341200 Opened 7 years ago Closed 7 years ago

Use LocalAllocPolicy and ShutdownPromisePool to streamline DecoderData::ShutdownDecoder()

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(10 files)

Bug 1341200. Part 1 - satisfy least privilege principle by capturing |mSamplesWaitingForKey| instead of |this|.
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1341200. Part 2 - replace mFlushRequest with a bool for mFlushRequest.Disconnect() is never used and a bool is sufficient to do the job.
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1341200. Part 3 - replace mShutdownRequest with a bool as P2.
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1341200. Part 4 - remove the check that is unnecessary.
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1341200. Part 5 - move the definition to .cpp as they will access ShutdownPromisePool in next patches.
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1341200. Part 6 - let ShutdownDecoderWithPromise() return void by tracking the shutdown promise.
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1341200. Part 7 - merge ShutdownDecoderWithPromise() and ShutdownDecoder().
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1341200. Part 8 - let DecoderData::ShutdownDecoder() handle shutdown in the middle of flush.
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1341200. Part 9 - remove unused mShuttingDown.
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1341200. Part 10 - streamline DecoderData::ShutdownDecoder() so it feels like a sync function and MFR doesn't need to explicitly wait for flush/shutdown to complete before creating new decoders.
59 bytes, text/x-review-board-request
jya
: review+
Details 
Once bug 1340940 is done, we have LocalAllocPolicy and ShutdownPromisePool to manage the order of decoder creation/shutdown and we are able to remove MFR::DecoderData::mShutdownPromise/mShutdownRequest. DecoderData::ShutdownDecoder() will feel like a sync function and MFR can create a new decoder without waiting for the previous one to shut down completely by using shutdown promises.
Assignee: nobody → jwwang
Depends on: 1340940
Priority: -- → P3
Attachment #8841415 - Flags: review?(jyavenard)
Attachment #8841416 - Flags: review?(jyavenard)
Attachment #8841417 - Flags: review?(jyavenard)
Attachment #8841418 - Flags: review?(jyavenard)
Attachment #8841419 - Flags: review?(jyavenard)
Attachment #8841420 - Flags: review?(jyavenard)
Attachment #8841421 - Flags: review?(jyavenard)
Attachment #8841422 - Flags: review?(jyavenard)
Attachment #8841423 - Flags: review?(jyavenard)
Attachment #8841424 - Flags: review?(jyavenard)
Comment on attachment 8841418 [details]
Bug 1341200. Part 4 - remove the check that is unnecessary.

https://reviewboard.mozilla.org/r/115642/#review117056

::: dom/media/MediaFormatReader.h:356
(Diff revision 1)
>        mDrainState = DrainState::None;
>        mOutput.Clear();
>        mNumSamplesInput = 0;
>        mNumSamplesOutput = 0;
>        mSizeOfQueue = 0;
> -      if (mDecoder && !mFlushed) {
> +      if (mDecoder) {

How is that unecessary?
I won't review this until next week being on PTO

Disclaimer: I see this as yet another unnecessary rewrite...
Comment on attachment 8841418 [details]
Bug 1341200. Part 4 - remove the check that is unnecessary.

https://reviewboard.mozilla.org/r/115642/#review117056

> How is that unecessary?

We get here only when |!mFlushing && !mFlushed|, so we don't need to check |!mFlushed| again.
Comment on attachment 8841415 [details]
Bug 1341200. Part 1 - satisfy least privilege principle by capturing |mSamplesWaitingForKey| instead of |this|.

https://reviewboard.mozilla.org/r/115636/#review118196
Attachment #8841415 - Flags: review?(jyavenard) → review+
Comment on attachment 8841416 [details]
Bug 1341200. Part 2 - replace mFlushRequest with a bool for mFlushRequest.Disconnect() is never used and a bool is sufficient to do the job.

https://reviewboard.mozilla.org/r/115638/#review118198

::: dom/media/MediaFormatReader.h:267
(Diff revision 1)
>        return mWaitingForData || mWaitingForKey;
>      }
>  
>      // MediaDataDecoder handler's variables.
>      MozPromiseRequestHolder<MediaDataDecoder::DecodePromise> mDecodeRequest;
> -    MozPromiseRequestHolder<MediaDataDecoder::FlushPromise> mFlushRequest;
> +    bool mFlushing = false; // True if flush is in action.

initialisation is a bit inconsistent with the rest of the code; nothing else is initialised the C++11 way.

so please initialise in the constructor; you cna create another patch to move everything C++11
Attachment #8841416 - Flags: review?(jyavenard) → review+
Comment on attachment 8841417 [details]
Bug 1341200. Part 3 - replace mShutdownRequest with a bool as P2.

https://reviewboard.mozilla.org/r/115640/#review118200
Attachment #8841417 - Flags: review?(jyavenard) → review+
Comment on attachment 8841418 [details]
Bug 1341200. Part 4 - remove the check that is unnecessary.

https://reviewboard.mozilla.org/r/115642/#review118202
Attachment #8841418 - Flags: review?(jyavenard) → review+
Comment on attachment 8841419 [details]
Bug 1341200. Part 5 - move the definition to .cpp as they will access ShutdownPromisePool in next patches.

https://reviewboard.mozilla.org/r/115644/#review118204
Attachment #8841419 - Flags: review?(jyavenard) → review+
Comment on attachment 8841420 [details]
Bug 1341200. Part 6 - let ShutdownDecoderWithPromise() return void by tracking the shutdown promise.

https://reviewboard.mozilla.org/r/115646/#review118206

::: dom/media/MediaFormatReader.h:538
(Diff revision 1)
>  
>    // A flag indicating if the start time is known or not.
>    bool mHasStartTime = false;
>  
>    void ShutdownDecoder(TrackType aTrack);
> -  RefPtr<ShutdownPromise> ShutdownDecoderWithPromise(TrackType aTrack);
> +  void ShutdownDecoderWithPromise(TrackType aTrack);

the name is no longer accurate or representative of what's being done.

There's no promise there anymore

::: dom/media/MediaFormatReader.cpp:1174
(Diff revision 1)
>    }
>  
>    if (decoder.mFlushing || decoder.mShuttingDown) {
>      // Let the current flush or shutdown operation complete, Flush will continue
>      // shutting down the current decoder now that the shutdown promise is set.
> -    return decoder.mShutdownPromise.Ensure(__func__);
> +    mShutdownPromisePool->Track(decoder.mShutdownPromise.Ensure(__func__));

This doesn't work the way it used to work.

decoder.mShutdownPromise.Ensure(__func__) when the decoder was currently being shut down would have returned the same promise (as Ensure returns the currently pending promise if one or create a new one if none).

So here you may potentially track the same promise twice.
How will the ShutdownPromisePool handle this condition?
Attachment #8841420 - Flags: review?(jyavenard) → review+
Comment on attachment 8841421 [details]
Bug 1341200. Part 7 - merge ShutdownDecoderWithPromise() and ShutdownDecoder().

https://reviewboard.mozilla.org/r/115648/#review118208
Attachment #8841421 - Flags: review?(jyavenard) → review+
Comment on attachment 8841422 [details]
Bug 1341200. Part 8 - let DecoderData::ShutdownDecoder() handle shutdown in the middle of flush.

https://reviewboard.mozilla.org/r/115650/#review118222

::: dom/media/MediaFormatReader.cpp:392
(Diff revision 1)
> +    }
> +    return;
> +  }
> +
> +  if (!mShutdownPromise.IsEmpty()) {
> +    // This is called from the resolve/reject function of Flush.

wouldn't it be more readable if this part was moved within ::Flush() ?

::: dom/media/MediaFormatReader.cpp:405
(Diff revision 1)
> +  // mShutdownPromisePool will handle the order of decoder shutdown so
> +  // we can forget mDecoder and be ready to create a new one.
>    mDecoder = nullptr;
> +  mDescription = "shutdown";
> +  mOwner->ScheduleUpdate(mType == MediaData::AUDIO_DATA
> +                               ? TrackType::kAudioTrack

? is to be aligned with the previous operand. So no 2 spaces indent
Attachment #8841422 - Flags: review?(jyavenard) → review+
Comment on attachment 8841423 [details]
Bug 1341200. Part 9 - remove unused mShuttingDown.

https://reviewboard.mozilla.org/r/115652/#review118232
Attachment #8841423 - Flags: review?(jyavenard) → review+
Comment on attachment 8841424 [details]
Bug 1341200. Part 10 - streamline DecoderData::ShutdownDecoder() so it feels like a sync function and MFR doesn't need to explicitly wait for flush/shutdown to complete before creating new decoders.

https://reviewboard.mozilla.org/r/115654/#review118234
Attachment #8841424 - Flags: review?(jyavenard) → review+
Comment on attachment 8841420 [details]
Bug 1341200. Part 6 - let ShutdownDecoderWithPromise() return void by tracking the shutdown promise.

https://reviewboard.mozilla.org/r/115646/#review118206

> the name is no longer accurate or representative of what's being done.
> 
> There's no promise there anymore

This function will be merged in next patch soon.

> This doesn't work the way it used to work.
> 
> decoder.mShutdownPromise.Ensure(__func__) when the decoder was currently being shut down would have returned the same promise (as Ensure returns the currently pending promise if one or create a new one if none).
> 
> So here you may potentially track the same promise twice.
> How will the ShutdownPromisePool handle this condition?

http://searchfox.org/mozilla-central/rev/546a05fec017cb785fca62a5199d42812a6a1fd3/dom/media/MediaFormatReader.cpp#360
ShutdownPromisePool::Track() asserts the same promise is not tracked twice. The current call flow of MFR ensure this won't happen otherwise the assertion will notify us.
Comment on attachment 8841422 [details]
Bug 1341200. Part 8 - let DecoderData::ShutdownDecoder() handle shutdown in the middle of flush.

https://reviewboard.mozilla.org/r/115650/#review118222

> wouldn't it be more readable if this part was moved within ::Flush() ?

Then we will also need to move |mDecoder = nullptr;   mDescription = "shutdown"; mOwner->ScheduleUpdate();| into Flush(). Since this if branch is going to be removed in next patch, I will just leave it.
Thanks for the reviews!
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 0715cfe9f86a -d eb6b0553d079: rebasing 379329:0715cfe9f86a "Bug 1341200. Part 1 - satisfy least privilege principle by capturing |mSamplesWaitingForKey| instead of |this|. r=jya"
rebasing 379330:54a3e07e64bf "Bug 1341200. Part 2 - replace mFlushRequest with a bool for mFlushRequest.Disconnect() is never used and a bool is sufficient to do the job. r=jya"
merging dom/media/MediaFormatReader.cpp
merging dom/media/MediaFormatReader.h
warning: conflicts while merging dom/media/MediaFormatReader.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fead77db2529
Part 1 - satisfy least privilege principle by capturing |mSamplesWaitingForKey| instead of |this|. r=jya
https://hg.mozilla.org/integration/autoland/rev/e9fc566fda85
Part 2 - replace mFlushRequest with a bool for mFlushRequest.Disconnect() is never used and a bool is sufficient to do the job. r=jya
https://hg.mozilla.org/integration/autoland/rev/5f0dca380e84
Part 3 - replace mShutdownRequest with a bool as P2. r=jya
https://hg.mozilla.org/integration/autoland/rev/e353098c37d2
Part 4 - remove the check that is unnecessary. r=jya
https://hg.mozilla.org/integration/autoland/rev/fb30eeb72b85
Part 5 - move the definition to .cpp as they will access ShutdownPromisePool in next patches. r=jya
https://hg.mozilla.org/integration/autoland/rev/5abf28d47683
Part 6 - let ShutdownDecoderWithPromise() return void by tracking the shutdown promise. r=jya
https://hg.mozilla.org/integration/autoland/rev/4acb3ce34d94
Part 7 - merge ShutdownDecoderWithPromise() and ShutdownDecoder(). r=jya
https://hg.mozilla.org/integration/autoland/rev/161f4237b402
Part 8 - let DecoderData::ShutdownDecoder() handle shutdown in the middle of flush. r=jya
https://hg.mozilla.org/integration/autoland/rev/28e9aedd34dc
Part 9 - remove unused mShuttingDown. r=jya
https://hg.mozilla.org/integration/autoland/rev/bcc6b0e45020
Part 10 - streamline DecoderData::ShutdownDecoder() so it feels like a sync function and MFR doesn't need to explicitly wait for flush/shutdown to complete before creating new decoders. r=jya
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: