Closed Bug 1312337 Opened 3 years ago Closed 3 years ago

Refactor ReaderQueue

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(4 files)

It is complicated for MDSM to handle reader-suspend which could happen any time in any state.

http://searchfox.org/mozilla-central/rev/84075be5067b68dc6cb3b89f999645650e68c05b/dom/media/MediaFormatReader.cpp#405

My idea is to add a wrapper around |mPlatform->CreateDecoder| to return a promise which won't be resolved until the decoder count is within limit (what ReaderQueue currently does) and a decoder is initialized.

This will greatly simplify the dormant code of MDSM. We might also want to put more policy code into the wrapper in the future like using software decoder for low-res videos (which are ads mostly).
Blocks: 1286129
Note, the decoder count won't be restored until a decoder is shut down. The logic is simplified because we don't allow preemption (reader-suspend). Once a decoder is acquired, it won't be taken away until the reader decides to release it.
I'm not too sure on how an internal state of the MFR would help anything when it comes to the MDSM.

May as well merge creating a decoder / initialising it together. So that when you create the decoder what comes out is always an initialised one.

But to me, this is refactoring for the sake of it and I'm unclear on how that would help MDSM and suspend.
At no time is the MDSM aware on what the MFR internal state is.
The resource model of ReaderQueue is preemptive which forces MDSM to do something (dormant) to handle resource allocation and deallocation.

When a reader is suspended, ReaderQueue calls ReleaseResources() to shut down its decoders and notifies MDSM to enter dormant state for playback can't continue without decoders. When a reader is resumed, ReaderQueue notifies MDSM which will exit dormant and seek to the original position before resuming playback.

It is complicated for MDSM to handle reader-suspend which could happen anytime, e.g. in the middle of seeking, buffering, or decoding 1st frames. MDSM has to remember what it is currently doing and resume its job when reader is resumed.

It is even more complicated when taking suspend-video-decoding into account. We do a video-only seek when resuming video decoding. However, if we enter dormant in the middle of a video-only seek, a full-seek [1] is required when coming out of dormant because we release both audio and video decoders when entering dormant.

So the idea of this bug is to change the preemptive model into a collaborative one. Once a decoder is acquired, it will never be lost until MDSM decides to give it up by calling ReleaseResources() on the reader. The state transition will be a lot simpler because MDSM  has full control over when to enter/exit dormant.

Also, all the resource management is internal to MFR so MDSM won't have to worry about it at all.

[1] http://searchfox.org/mozilla-central/rev/3079f45ce59e105e2490b1c8b3a5a52cd47a5ac0/dom/media/MediaDecoderStateMachine.cpp#1393
The interface is like:

class DecoderManager {
  typedef MozPromise<Ref<MediaDataDecoder>, MediaResult, true> Promise;
  Ref<Promise> Create(const CreateDecoderParams& aParams);
};

DecoderManager::Create {
  1. check if the number of allocated decoders is within the limit.
  2. call PDMFactory::CreateDecoder() to create a decoder
  3. call MediaDataDecoder::Init() to init the decoder
  4. resolve the promise with the initialized decoder.
}

DecoderManager::Create is used by MFR in place of PDMFactory::CreateDecoder().
Depends on: 1313557
Assignee: nobody → jwwang
Priority: -- → P3
Attachment #8805466 - Flags: review?(jyavenard)
Attachment #8805467 - Flags: review?(jyavenard)
Attachment #8805468 - Flags: review?(jyavenard)
Attachment #8805469 - Flags: review?(jyavenard)
Comment on attachment 8805466 [details]
Bug 1312337. Part 1 - move creation/initialization of decoders into DecoderFactory.

https://reviewboard.mozilla.org/r/89208/#review88378

::: dom/media/MediaFormatReader.cpp:40
(Diff revision 1)
>  #define LOG(arg, ...) MOZ_LOG(sFormatDecoderLog, mozilla::LogLevel::Debug, ("MediaFormatReader(%p)::%s: " arg, this, __func__, ##__VA_ARGS__))
>  #define LOGV(arg, ...) MOZ_LOG(sFormatDecoderLog, mozilla::LogLevel::Verbose, ("MediaFormatReader(%p)::%s: " arg, this, __func__, ##__VA_ARGS__))
>  
>  namespace mozilla {
>  
> +class MediaFormatReader::DecoderFactory {

class
{
...
}

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Classes

::: dom/media/MediaFormatReader.cpp:48
(Diff revision 1)
> +public:
> +  explicit DecoderFactory(MediaFormatReader* aOwner) : mOwner(aOwner) {}
> +  void CreateDecoder(TrackType aTrack);
> +
> +private:
> +  enum class Stage : int8_t {

enum
{
}

::: dom/media/MediaFormatReader.cpp:53
(Diff revision 1)
> +  enum class Stage : int8_t {
> +    None,
> +    WaitForInit
> +  };
> +
> +  struct Data {

struct
{
...
}

::: dom/media/MediaFormatReader.cpp:57
(Diff revision 1)
> +
> +  struct Data {
> +    Stage mStage = Stage::None;
> +    RefPtr<MediaDataDecoder> mDecoder;
> +    MozPromiseRequestHolder<InitPromise> mInitPromise;
> +    ~Data() {

~Data()
{

::: dom/media/MediaFormatReader.cpp:70
(Diff revision 1)
> +  void RunStage(TrackType aTrack);
> +
> +  MediaFormatReader* const mOwner; // guaranteed to be valid by the owner.
> +};
> +
> +void MediaFormatReader::

This isn't a style defined anywhere.

void
MediaFormatReader::DecoderFactory::FunctionName(
  argumentList...)

and everywhere this appears

::: dom/media/MediaFormatReader.cpp:82
(Diff revision 1)
> +
> +void
> +MediaFormatReader::
> +DecoderFactory::RunStage(TrackType aTrack)
> +{
> +  auto owner = mOwner;

why not use mOwner directly?

::: dom/media/MediaFormatReader.cpp:113
(Diff revision 1)
> +          ownerData.mCallback.get(),
> +          owner->mCrashHelper,
> +          ownerData.mIsBlankDecode,
> +          &result
> +        });
> +      } else {

You've dropped some of the comments from the original code, please re-add them.

And I'd like to keep the original switch/case structure.

Please reuse as much of the original code without reformatting or change in behaviours or comment.

::: dom/media/MediaFormatReader.cpp:138
(Diff revision 1)
> +        return;
> +      }
> +
> +      data.mInitPromise.Begin(data.mDecoder->Init()->Then(
> +        owner->OwnerThread(), __func__,
> +        [this, &data, &ownerData] (TrackType aTrack) {

Please keep the original code, as most of it has been moved. 
While I agree that as the init promise is disconnected upon destruction so we don't have to keep an extra strong reference.

If you want to change the lifecycle model later, please do so in another patch, this makes it too hard to review otherwise and too many changes at once.
Attachment #8805466 - Flags: review?(jyavenard) → review+
Attachment #8805467 - Flags: review?(jyavenard) → review?(gsquelart)
Comment on attachment 8805468 [details]
Bug 1312337. Part 3 - employ DecoderAllocPolicy for controlling decoder creation.

https://reviewboard.mozilla.org/r/89212/#review88384

::: dom/media/MediaFormatReader.cpp:164
(Diff revision 2)
>  DecoderAllocPolicy::operator=(decltype(nullptr))
>  {
>    delete this;
>  }
>  
>  class MediaFormatReader::DecoderFactory {

Please have a read of https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Classes

:)

::: dom/media/MediaFormatReader.cpp:226
(Diff revision 2)
> +  void Flush() override { mDecoder->Flush(); }
> +  void Drain() override { mDecoder->Drain(); }
> +  bool IsHardwareAccelerated(nsACString& aFailureReason) const override {
> +    return mDecoder->IsHardwareAccelerated(aFailureReason);
> +  }
> +  const char* GetDescriptionName() const override {

{ on a new line

::: dom/media/MediaFormatReader.cpp:229
(Diff revision 2)
> +    return mDecoder->IsHardwareAccelerated(aFailureReason);
> +  }
> +  const char* GetDescriptionName() const override {
> +    return mDecoder->GetDescriptionName();
> +  }
> +  void SetSeekThreshold(const media::TimeUnit& aTime) override {

same { on a new line
Attachment #8805468 - Flags: review?(jyavenard) → review+
Comment on attachment 8805468 [details]
Bug 1312337. Part 3 - employ DecoderAllocPolicy for controlling decoder creation.

https://reviewboard.mozilla.org/r/89212/#review88388

The functionality doesn't appear to be identical.

You now limit to how many decoders can be created at once, preventing a new decoder to be created if there are too many at once.

The old functionality was to put the oldest one in dormant mode
Attachment #8805468 - Flags: review+ → review?(jyavenard)
Comment on attachment 8805467 [details]
Bug 1312337. Part 2 - implement a policy object to control the number of decoders to be created concurrently.

https://reviewboard.mozilla.org/r/89210/#review88382

::: dom/media/MediaFormatReader.cpp:45
(Diff revision 2)
>  
>  namespace mozilla {
>  
> +/**
> + * This is a singleton which controls the number of decoders that can be
> + * created. Before calling PDMFactory::CreateDecoder(), Alloc() must be called

Please amend that it is to control how many decoder that can be created concurrently.

::: dom/media/MediaFormatReader.cpp:51
(Diff revision 2)
> + * to get a token object as a permission to create a decoder. The token should
> + * stay alive until Shutdown() is called on the decoder. The destructor of the
> + * token will restore the decoder count so it is available for next calls of
> + * Alloc().
> + */
> +class DecoderAllocPolicy {

class blah
{

::: dom/media/MediaFormatReader.cpp:89
(Diff revision 2)
> +  int mDecoderLimit;
> +  // Requests to acquire tokens.
> +  std::deque<RefPtr<PromisePrivate>> mPromises;
> +};
> +
> +class DecoderAllocPolicy::TokenImpl : public Token {

class
{
...
}

::: dom/media/MediaFormatReader.cpp:117
(Diff revision 2)
> +}
> +
> +DecoderAllocPolicy&
> +DecoderAllocPolicy::Instance()
> +{
> +  static auto sPolicy = new DecoderAllocPolicy();

I'm fairly certain this isn't thread-safe on Windows.

I've passed the review to Gerald.
Attachment #8805467 - Flags: review?(jyavenard)
Comment on attachment 8805467 [details]
Bug 1312337. Part 2 - implement a policy object to control the number of decoders to be created concurrently.

https://reviewboard.mozilla.org/r/89210/#review88412

r+ with nits/suggestions:

::: dom/media/MediaFormatReader.cpp:64
(Diff revision 2)
> +  };
> +
> +  using Promise = MozPromise<RefPtr<Token>, bool, true>;
> +
> +  // Acquire a token for decoder creation. Thread-safe.
> +  auto Alloc(TrackType aTrack) -> RefPtr<Promise>;

Why the delayed return type declaration style? It seems unnecessary here.

::: dom/media/MediaFormatReader.cpp:67
(Diff revision 2)
> +
> +  // Acquire a token for decoder creation. Thread-safe.
> +  auto Alloc(TrackType aTrack) -> RefPtr<Promise>;
> +
> +  // Called by ClearOnShutdown() to delete the singleton.
> +  void operator=(decltype(nullptr));

Why not just use nullptr_t?

::: dom/media/MediaFormatReader.cpp:73
(Diff revision 2)
> +
> +  // Get the singleton. Thread-safe.
> +  static DecoderAllocPolicy& Instance();
> +
> +private:
> +  class TokenImpl;

I'm not sure 'TokenImpl' is the best name here, as it's not really an implementation of some abstract base.
How about 'AutoDeallocToken'?

::: dom/media/MediaFormatReader.cpp:86
(Diff revision 2)
> +
> +  ReentrantMonitor mMonitor;
> +  // The number of decoders available for creation.
> +  int mDecoderLimit;
> +  // Requests to acquire tokens.
> +  std::deque<RefPtr<PromisePrivate>> mPromises;

Why not use a simple std::queue?

::: dom/media/MediaFormatReader.cpp:98
(Diff revision 2)
> +  }
> +};
> +
> +DecoderAllocPolicy::DecoderAllocPolicy()
> +  : mMonitor("DecoderAllocPolicy::mMonitor")
> +  , mDecoderLimit(MediaPrefs::MediaDecoderLimit())

If the pref is set to 0, it will block all decoder creations, is that intended?
If not, you may want to use MediaPrefs::MediaDecoderLimitDefault() when given a 0.

::: dom/media/MediaFormatReader.cpp:117
(Diff revision 2)
> +}
> +
> +DecoderAllocPolicy&
> +DecoderAllocPolicy::Instance()
> +{
> +  static auto sPolicy = new DecoderAllocPolicy();

Function-static initialization is not thread-safe in VS 2013. I told jya about that last year, which he remembered, and I confirmed it was still the case (hence his comment).

However, it was finally implemented in VS 2015 [1], which we are using since FF50 [2].

So if you don't intend to uplift this code to 45esr, it should actually be fine.

[1] "Magic statics" in https://msdn.microsoft.com/en-au/library/hh567368.aspx#concurrencytable
[2] https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code

::: dom/media/MediaFormatReader.cpp:145
(Diff revision 2)
> +DecoderAllocPolicy::ResolvePromise()
> +{
> +  mMonitor.AssertCurrentThreadIn();

If you use the "proof of lock" idiom (take 'ReentrantMonitorAutoEnter& aProofOfLock' as parameter), you won't need to do 'mMonitor.AssertCurrentThreadIn();' and it would prevent any misuse at compilation time.
Attachment #8805467 - Flags: review?(gsquelart) → review+
Comment on attachment 8805466 [details]
Bug 1312337. Part 1 - move creation/initialization of decoders into DecoderFactory.

https://reviewboard.mozilla.org/r/89208/#review88378

> Please keep the original code, as most of it has been moved. 
> While I agree that as the init promise is disconnected upon destruction so we don't have to keep an extra strong reference.
> 
> If you want to change the lifecycle model later, please do so in another patch, this makes it too hard to review otherwise and too many changes at once.

Since DecoderFactory is not a ref-counted class, we can't keep a strong reference of it.
Comment on attachment 8805467 [details]
Bug 1312337. Part 2 - implement a policy object to control the number of decoders to be created concurrently.

https://reviewboard.mozilla.org/r/89210/#review88412

> Why the delayed return type declaration style? It seems unnecessary here.

To be consistent with its definition outside the class. It is an error to say:
RefPtr<Promise> DecoderAllocPolicy::Alloc(TrackType aTrack) {}

> Why not just use nullptr_t?

I copy the code from UniquePtr. It is good to learn nullptr_t. Thanks!

> If the pref is set to 0, it will block all decoder creations, is that intended?
> If not, you may want to use MediaPrefs::MediaDecoderLimitDefault() when given a 0.

Yes, that is intended. I didn't change the definition of MediaDecoderLimitDefault():
http://searchfox.org/mozilla-central/rev/445097654c1cc1098ee0171a29c439afe363a588/dom/media/MediaPrefs.h#173

|>= 0|: the number of decoders that can be created concurrently. (0 means no one can be created)
|< 0|: no limit. You can create as many as you want.

> Function-static initialization is not thread-safe in VS 2013. I told jya about that last year, which he remembered, and I confirmed it was still the case (hence his comment).
> 
> However, it was finally implemented in VS 2015 [1], which we are using since FF50 [2].
> 
> So if you don't intend to uplift this code to 45esr, it should actually be fine.
> 
> [1] "Magic statics" in https://msdn.microsoft.com/en-au/library/hh567368.aspx#concurrencytable
> [2] https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code

Thanks for the notice. I will add comments to note the code shouldn't be uplifted to esr45.

> If you use the "proof of lock" idiom (take 'ReentrantMonitorAutoEnter& aProofOfLock' as parameter), you won't need to do 'mMonitor.AssertCurrentThreadIn();' and it would prevent any misuse at compilation time.

It's good to learn this idiom. Thanks!
Comment on attachment 8805468 [details]
Bug 1312337. Part 3 - employ DecoderAllocPolicy for controlling decoder creation.

https://reviewboard.mozilla.org/r/89212/#review88388

No. They are not equivalent. Recaps from comment 3:
ReaderQueue(preemptive) v.s. DecoderAllocPolicy(collaborative)

ReaderQueue:
1. when there are too many concurrent decoders: force one existing reader to release its decoders.
2. actions taken by MDSM: dormant

DecoderAllocPolicy:
1. when there are too many concurrent decoders: wait
2. actions taken by MDSM: none

The resource management of DecoderAllocPolicy is simpler and is required by bug 1311872 where MDSM will have full control over when to enter/exit dormant.
Comment on attachment 8805467 [details]
Bug 1312337. Part 2 - implement a policy object to control the number of decoders to be created concurrently.

https://reviewboard.mozilla.org/r/89210/#review88412

> To be consistent with its definition outside the class. It is an error to say:
> RefPtr<Promise> DecoderAllocPolicy::Alloc(TrackType aTrack) {}

Outside the class you just need to add 'DecoderAllocPolicy::' in front of 'Promise'.

It just seems strange to use a trailing return type when it's not needed (as it doesn't depend on argument names).
I don't see adding a class qualifier as an inconsistency issue -- but that's subjective I guess!
In the end it does the job, so up to you if you'd prefer to keep it.
Comment on attachment 8805468 [details]
Bug 1312337. Part 3 - employ DecoderAllocPolicy for controlling decoder creation.

https://reviewboard.mozilla.org/r/89212/#review88388

I'm not sure this is what we want to do.
So we just display nothing because we have too many decoders opened?
Comment on attachment 8805469 [details]
Bug 1312337. Part 4 - remove ReaderQueue and its friends.

https://reviewboard.mozilla.org/r/89214/#review88386

::: dom/media/MediaDecoderStateMachine.cpp
(Diff revision 2)
>  
> -void MediaDecoderStateMachine::ReaderSuspendedChanged()
> -{
> -  MOZ_ASSERT(OnTaskQueue());
> -  DECODER_LOG("ReaderSuspendedChanged: %d", mIsReaderSuspended.Ref());
> -  SetDormant(mIsReaderSuspended);

How do you handle going dormant and resuming now ? this functionality doesn't appear to have been duplicated
Attachment #8805469 - Flags: review?(jyavenard) → review+
Comment on attachment 8805468 [details]
Bug 1312337. Part 3 - employ DecoderAllocPolicy for controlling decoder creation.

https://reviewboard.mozilla.org/r/89212/#review88388

Yes. It has to wait until the decoders are released. The situation is not worse than what we have with ReaderQueue but the code is a lot simpler:

with ReaderQueue:
1. M1 is decoding metadata
2. M2 comes up and put M1 to suspend
3. M2 continues to play and M1 displays nothing because its first frame is not decoded.

with DecoderAllocPolicy:
1. M1 is decoding metadata
2. M2 comes up and has to wait for the decoder to be released.
3. M1 continues to play and M2 displays nothing because its first frame is not decoded.
Comment on attachment 8805468 [details]
Bug 1312337. Part 3 - employ DecoderAllocPolicy for controlling decoder creation.

https://reviewboard.mozilla.org/r/89212/#review89130

Following discussion with JW... Seeing that this makes no difference on desktop, which policy we adopt likely doesn't matter much.
Attachment #8805468 - Flags: review?(jyavenard) → review+
Comment on attachment 8805469 [details]
Bug 1312337. Part 4 - remove ReaderQueue and its friends.

https://reviewboard.mozilla.org/r/89214/#review88386

> How do you handle going dormant and resuming now ? this functionality doesn't appear to have been duplicated

There is no need to enter dormant because no one (ReaderQueue) will force MDSM to give up its decoders until it is willing to.
Thanks for reviews!
Blocks: 1311872
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2ec79c5f0d27
Part 1 - move creation/initialization of decoders into DecoderFactory. r=jya
https://hg.mozilla.org/integration/autoland/rev/ee955b61be2c
Part 2 - implement a policy object to control the number of decoders to be created concurrently. r=gerald
https://hg.mozilla.org/integration/autoland/rev/ce23f78a319e
Part 3 - employ DecoderAllocPolicy for controlling decoder creation. r=jya
https://hg.mozilla.org/integration/autoland/rev/3be9eca3776d
Part 4 - remove ReaderQueue and its friends. r=jya
Depends on: 1315631
No longer depends on: 1315631
Depends on: 1317779
You need to log in before you can comment on or make changes to this bug.