1112219 - Implement platform independent MediaResourceManager

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Sotaro Ikeda [:sotaro]]

MediaResourceManager is implement as gonk specific. It is necessary by platform independent implementation.

http://mxr.mozilla.org/mozilla-central/source/dom/media/omx/mediaresourcemanager/

Comment 1

[Sotaro Ikeda [:sotaro]]

gecko IPC communication need to be handled as Off-Main-Thread way. It seems nice to implement it by using PBackground.

Comment 2

[Sotaro Ikeda [:sotaro]]

FYI: diagram of PBackground related classes.

https://github.com/sotaroikeda/firefox-diagrams/blob/master/ipc/ipc_PBackground_FirefoxOS_2_2.pdf?raw=true

Comment 3

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch - Add MediaResourceManager

wip patch under implementing.

Comment 4

[Sotaro Ikeda [:sotaro]]

Attachment: class diagram of MediaResourceManager

Comment 5

[Sotaro Ikeda [:sotaro]]

Attachment: class diagram of MediaResourceManager

Comment 6

[Sotaro Ikeda [:sotaro]]

Attachment: class diagram of MediaResourceManager

Comment 7

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch - Add MediaResourceManager

un-bitrot.

Comment 8

[Sotaro Ikeda [:sotaro]]

If we want MediaResourceManager to be efficient, its IPC thread need to be non-main thread. Then at first, I thought that PBackground could handle this. But it has one big problem. PBackground and its actor could not be created synchronously.

MediaResourceManager need to handle synchronous wait use case. If ipc actor is not created when the wait is requested, it can not handle the request. If we want to avoid this case, we need to create the ipc actor as soon as possible when the process is loaded. It means add one more thread to the process.

Comment 9

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #8)
> possible when the process is loaded. It means add one more thread to the
> process.

We already have similar thread. It is ImageBridge thread. Until PBackground's creation problem is addressed, it seems better to use ImageBridge thread. On b2g gonk, gecko supports only ImageBridge enabled environment.

Comment 10

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add MediaResourceManager

Comment 11

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add MediaResourceManager

Rebased

Comment 12

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add MediaResourceManager

Fix nits.

Comment 13

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 5 - Remove gonk specific MediaResourceManagerService

Comment 14

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Add MediaResourceManager

Comment 15

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Change to ImageBridge

Comment 16

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 3 - Change to MediaCodecProxy

Comment 17

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 4 - Change to OMXCodecProxy

Comment 18

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Add MediaResourceManager

Fix assert checks.

Comment 19

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0a7875705e3

Comment 20

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Add MediaResourceManager

Fix constructor.

Comment 21

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6ad3c1721bb

Comment 22

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Add MediaResourceManager

Fix shutdown.

Comment 23

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Change to ImageBridge

Fix shutdown.

Comment 24

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=68bf9bd297f8

Comment 25

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Add MediaResourceManager

Add error handling

Comment 26

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5bec5581a7c0

Comment 27

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Add MediaResourceManager

Remove ClearOnShutdown from MediaResourceService.

Comment 28

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Add MediaResourceManager

correct patch.

Comment 29

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=de406d31bfad

Comment 30

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Add MediaResourceManager

Remove unnecessary assert.

Comment 31

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6362d39f068c

Comment 32

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Add MediaResourceManager

Fix around shutdown handling.

Comment 33

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b27dcb0a858

Comment 34

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Add MediaResourceManager

Fix build failure.

Comment 35

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ddc4015f7c2c

Comment 36

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Add MediaResourceManager

Update a function name.

Comment 37

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 4 - Change to OMXCodecProxy

Update a function name.

Comment 38

[Sotaro Ikeda [:sotaro]]

Attachment: class diagram of MediaResourceManager

Update a diagram based on latest implementation.

At first, I planed to use PBackground, but it could not be used for MediaResourceManager, because it request to use main thread for actor creation. For the time being, I chose ImageBridge thread for MediaResourceManager.

Comment 39

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8615978 [details] [diff] [review]
patch part 1 - Add MediaResourceManager

Review of attachment 8615978 [details] [diff] [review]:
-----------------------------------------------------------------

You should also get an IPC peer to look at this. Maybe nical?

Can you use MediaPromises here for async operations? We should use them for all async operations in new code if possible.

See also:
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Hashtables

::: dom/media/systemservices/MediaResourceClient.h
@@ +49,5 @@
> +  bool SetListener(MediaResourceReservationListener* aListener);
> +
> +  // Try to acquire media resource asynchronously.
> +  // If the resource is used by others, wait until acquired.
> +  void Acquire();

An Acquire() call results in only one callback to the MediaResourceReservationListener, right? Can this return a MediaPromise instead, and resolve/reject the promise when complete, rather than calling the callback? MediaPromises are how we should do async operations in new code.

::: dom/media/systemservices/MediaResourceManager.cpp
@@ +94,5 @@
> +  ImageBridgeChild::GetSingleton()->GetMessageLoop()->PostTask(
> +    FROM_HERE,
> +    NewRunnableFunction(&MediaResourceManager_InitSync, &barrier, &done));
> +  // should stop the thread until done.
> +  while (!done) {

Can this deadlock? What if the imagebridge messageloop thread is blocked waiting on this thread (which thread is that? the main thread?). This seems like it could be a source of deadlocks to me.

Can Init() return a MediaPromise and become properly async? It's hard to tell as the rest of the patch is not present?

@@ +265,5 @@
> +      &MediaResourceManager::DoAcquire,
> +      aClient->mId));
> +
> +  // should stop the thread until done.
> +  while (!done) {

Same comment here; can this deadlock?

::: dom/media/systemservices/MediaResourceManager.h
@@ +71,5 @@
> +  bool mShutDown;
> +
> +  media::MediaResourceManagerChild* mChild;
> +
> +  std::map<int32_t, MediaResourceClient*> mResourceClients;

You should use  nsRefPtrHashtable<nsUint32HashKey, MediaResourceClient> here, as that will addref/release the MediaResourceClients you put in.

::: dom/media/systemservices/MediaResourceManagerParent.h
@@ +42,5 @@
> +  bool mDestroyed;
> +
> +  nsRefPtr<MediaResourceService> mMediaResourceService;
> +
> +  std::map<int32_t, MediaResourceRequest> mResourceRequests;

nsClassHashtable

::: dom/media/systemservices/PMediaResourceManager.ipdl
@@ +11,5 @@
> +namespace mozilla {
> +namespace media {
> +
> +/*
> + * The PMediaResourceManager is a sub-protocol in PImageBridge

Can we rename "*MediaResource*" to "*CodecResource*", so that it's clear that this is different from a MediaResource, i.e. the thing that we read data from?

Comment 40

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8615978 [details] [diff] [review]
patch part 1 - Add MediaResourceManager

Review of attachment 8615978 [details] [diff] [review]:
-----------------------------------------------------------------

Will drop review request for now; if you can use MediaPromieses here, please rework your patch to. If not, re-request review, assuming you can't remove the sync dispatches.

Comment 41

[Sotaro Ikeda [:sotaro]]

(In reply to Chris Pearce (:cpearce) from comment #40)
> -----------------------------------------------------------------
> 
> Will drop review request for now; if you can use MediaPromieses here, please
> rework your patch to. If not, re-request review, assuming you can't remove
> the sync dispatches.

We can't remove sync operation API. It is required by WebRTC code. And IIRC, MediaPromieses is expected to be used only on main thread and MediaTaskQueue thread.

Comment 42

[Sotaro Ikeda [:sotaro]]

(In reply to Chris Pearce (:cpearce) from comment #39)
> Comment on attachment 8615978 [details] [diff] [review]
> patch part 1 - Add MediaResourceManager
> 
> Review of attachment 8615978 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You should also get an IPC peer to look at this. Maybe nical?
> 
> Can you use MediaPromises here for async operations? We should use them for
> all async operations in new code if possible.

I do not think it is a good idea to use MediaPromises here at this time. If we use MediaPromise for async operation, caller side is also necessary to be prepared to handle it. We could not assume it for all future use cases. In future, Firefox OS TV might use this capability. But we do not know which code uses this capability in which purpose.

If callback is used, it is easy to connect to MediaPromise. It is already done in this bug in MediaOMXReader and MediaCodecReader cases.

> 
> See also:
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Hashtables
> 
> ::: dom/media/systemservices/MediaResourceClient.h
> @@ +49,5 @@
> > +  bool SetListener(MediaResourceReservationListener* aListener);
> > +
> > +  // Try to acquire media resource asynchronously.
> > +  // If the resource is used by others, wait until acquired.
> > +  void Acquire();
> 
> An Acquire() call results in only one callback to the
> MediaResourceReservationListener, right?

Yes.

> Can this return a MediaPromise
> instead, and resolve/reject the promise when complete, rather than calling
> the callback? MediaPromises are how we should do async operations in new
> code.

In this bug, I want to keep current callback implementation for the time being as explained in the above.

> 
> ::: dom/media/systemservices/MediaResourceManager.cpp
> @@ +94,5 @@
> > +  ImageBridgeChild::GetSingleton()->GetMessageLoop()->PostTask(
> > +    FROM_HERE,
> > +    NewRunnableFunction(&MediaResourceManager_InitSync, &barrier, &done));
> > +  // should stop the thread until done.
> > +  while (!done) {
> 
> Can this deadlock? What if the imagebridge messageloop thread is blocked
> waiting on this thread (which thread is that? the main thread?). This seems
> like it could be a source of deadlocks to me.

ImageBridge thread is not blocked by other threads except IPC operation. Therefore deadlock by main thread or other thread in process should not happen.

> 
> Can Init() return a MediaPromise and become properly async? It's hard to
> tell as the rest of the patch is not present?

We need to support MediaResourceClient::AcquireSyncNoWait(). It is sync operation. To support it, we can not make Init() as to return a MediaPromise.


> 
> @@ +265,5 @@
> > +      &MediaResourceManager::DoAcquire,
> > +      aClient->mId));
> > +
> > +  // should stop the thread until done.
> > +  while (!done) {
> 
> Same comment here; can this deadlock?

I do not think there is a risk of deadlock as explained above except ImageBridge shutdown.

> 
> ::: dom/media/systemservices/MediaResourceManager.h
> @@ +71,5 @@
> > +  bool mShutDown;
> > +
> > +  media::MediaResourceManagerChild* mChild;
> > +
> > +  std::map<int32_t, MediaResourceClient*> mResourceClients;
> 
> You should use  nsRefPtrHashtable<nsUint32HashKey, MediaResourceClient>
> here, as that will addref/release the MediaResourceClients you put in.

I'll update it.

> 
> ::: dom/media/systemservices/MediaResourceManagerParent.h
> @@ +42,5 @@
> > +  bool mDestroyed;
> > +
> > +  nsRefPtr<MediaResourceService> mMediaResourceService;
> > +
> > +  std::map<int32_t, MediaResourceRequest> mResourceRequests;
> 
> nsClassHashtable

I'll update it.

> 
> ::: dom/media/systemservices/PMediaResourceManager.ipdl
> @@ +11,5 @@
> > +namespace mozilla {
> > +namespace media {
> > +
> > +/*
> > + * The PMediaResourceManager is a sub-protocol in PImageBridge
> 
> Can we rename "*MediaResource*" to "*CodecResource*", so that it's clear
> that this is different from a MediaResource, i.e. the thing that we read
> data from?

For example, Firefox OS TV might use this capability in ture, in this case, we could not expect the resource is only CodecResource. Is there another better name?

Comment 43

[Sotaro Ikeda [:sotaro]]

> > 
> > Can Init() return a MediaPromise and become properly async? It's hard to
> > tell as the rest of the patch is not present?
> 
> We need to support MediaResourceClient::AcquireSyncNoWait(). It is sync
> operation. To support it, we can not make Init() as to return a MediaPromise.
> 

AcquireSyncNoWait() support and on demand MediaResourceManager creation make Init() to sync.

Comment 44

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #42)
> (In reply to Chris Pearce (:cpearce) from comment #39)
> > Comment on attachment 8615978 [details] [diff] [review]
> > patch part 1 - Add MediaResourceManager
> > 
> > Review of attachment 8615978 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > You should also get an IPC peer to look at this. Maybe nical?
> > 
> > Can you use MediaPromises here for async operations? We should use them for
> > all async operations in new code if possible.
> 
> I do not think it is a good idea to use MediaPromises here at this time. If
> we use MediaPromise for async operation, caller side is also necessary to be
> prepared to handle it. We could not assume it for all future use cases. In
> future, Firefox OS TV might use this capability. But we do not know which
> code uses this capability in which purpose.

Actually, WebRTC code does not fit to MediaPromises and use sync operation to reserve the resources.

Comment 45

[Sotaro Ikeda [:sotaro]]

> > 
> > Can you use MediaPromises here for async operations? We should use them for
> > all async operations in new code if possible.
> 
> I do not think it is a good idea to use MediaPromises here at this time. If
> we use MediaPromise for async operation, caller side is also necessary to be
> prepared to handle it. We could not assume it for all future use cases. In
> future, Firefox OS TV might use this capability. But we do not know which
> code uses this capability in which purpose.
> 
> If callback is used, it is easy to connect to MediaPromise. It is already
> done in this bug in MediaOMXReader and MediaCodecReader cases.

Sorry, it is not done in this bug. It is already done by Bug 1168456 and Bug 1167608.

Comment 46

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #42)
> ImageBridge thread is not blocked by other threads except IPC operation.
> Therefore deadlock by main thread or other thread in process should not
> happen.
> 

ImageBridge is designed as not to be blocked by other threads like main thread. If it happens, it is a defect.

Comment 47

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Sotaro Ikeda [:sotaro] from comment #42)
> (In reply to Chris Pearce (:cpearce) from comment #39)
> > Comment on attachment 8615978 [details] [diff] [review]
> > patch part 1 - Add MediaResourceManager
> > 
> > Review of attachment 8615978 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > You should also get an IPC peer to look at this. Maybe nical?
> > 
> > Can you use MediaPromises here for async operations? We should use them for
> > all async operations in new code if possible.
> 
> I do not think it is a good idea to use MediaPromises here at this time. If
> we use MediaPromise for async operation, caller side is also necessary to be
> prepared to handle it. We could not assume it for all future use cases. In
> future, Firefox OS TV might use this capability. But we do not know which
> code uses this capability in which purpose.
> 
> If callback is used, it is easy to connect to MediaPromise. It is already
> done in this bug in MediaOMXReader and MediaCodecReader cases.

OK.



> > 
> > ::: dom/media/systemservices/PMediaResourceManager.ipdl
> > @@ +11,5 @@
> > > +namespace mozilla {
> > > +namespace media {
> > > +
> > > +/*
> > > + * The PMediaResourceManager is a sub-protocol in PImageBridge
> > 
> > Can we rename "*MediaResource*" to "*CodecResource*", so that it's clear
> > that this is different from a MediaResource, i.e. the thing that we read
> > data from?
> 
> For example, Firefox OS TV might use this capability in ture, in this case,
> we could not expect the resource is only CodecResource. Is there another
> better name?

Naming things is hard.

MediaSystemResource, MediaHardwareResource?

Comment 48

[Sotaro Ikeda [:sotaro]]

> ::: dom/media/systemservices/MediaResourceManager.h
>> @@ +71,5 @@
>> > +  bool mShutDown;
>> > +
>> > +  media::MediaResourceManagerChild* mChild;
>> > +
>> > +  std::map<int32_t, MediaResourceClient*> mResourceClients;
>> 
>> You should use  nsRefPtrHashtable<nsUint32HashKey, MediaResourceClient>
>> here, as that will addref/release the MediaResourceClients you put in.
>
>I'll update it.

Correction:
Here should not addref/release, MediaResourceClient's lifetime is controlled only by its user.

Comment 49

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Add MediaResourceManager

Apply the comment.

Comment 50

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Change to ImageBridge

Comment 51

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 3 - Change to MediaCodecProxy

Comment 52

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 4 - Change to OMXCodecProxy

Comment 53

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Add MediaResourceManager

Fix debug build failures.

Comment 54

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6b0459cc43d
tryserver's build is very unstable.

Comment 55

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8627221 [details] [diff] [review]
patch part 1 - Add MediaResourceManager

Review of attachment 8627221 [details] [diff] [review]:
-----------------------------------------------------------------

Overall looks good. I would prefer to not have the synchronous acquires used in new code. We're trying to remove synchronous waits, as they're a source of bugs. New code (i.e. the Gonk backend for the MediaFormatReader, not the old MediaOmxReader) should be async, and it's easy to do now that we have MediaPromises.

::: dom/media/systemservices/MediaSystemResourceClient.h
@@ +25,5 @@
> +};
> +
> +/**
> + * MediaSystemResourceClient is used to reserve a media resource like hw decoder.
> + * When system has a limitation of a media resrouce, use this class

s/resrouce/resource/

@@ +52,5 @@
> +  // If the resource is used by others, wait until acquired.
> +  void Acquire();
> +
> +  // Try to acquire media resource synchronously. If the resource is not
> +  // available, fail to acquire it.

If the resource is not *immediately* available...

@@ +53,5 @@
> +  void Acquire();
> +
> +  // Try to acquire media resource synchronously. If the resource is not
> +  // available, fail to acquire it.
> +  // return false if resource is not acquired.

"return false if resource is not acquired." does not need to be said twice.

@@ +56,5 @@
> +  // available, fail to acquire it.
> +  // return false if resource is not acquired.
> +  // return false if resource is not acquired.
> +  //
> +  // This function should not be called on ImageBridge thread.

Add a comment here saying that synchronous AcqureSyncNoWait() is only here for compatibility with legacy code, and that new code should use the async Acquire().

::: dom/media/systemservices/MediaSystemResourceManager.cpp
@@ +76,5 @@
> +    return;
> +  }
> +
> +  if (InImageBridgeChildThread()) {
> +    if (!sSingleton) {

It's confusing that you call MediaSystemResourceManager::Init() which dispatches a task which calls MediaSystemResourceManager_InitSync which calls MediaSystemResourceManager::Init() to unblock the original thread waiting in MediaSystemResourceManager::Init().

Can't you just allocate MediaSystemResourceManager() in MediaSystemResourceManager_InitSync instead of calling back into MediaSystemResourceManager::Init() to allocate it?

Or even, use a lambda for the task to allocate sSingleton from inside MediaSystemResourceManager::Init()? Like so:

  ReentrantMonitor barrier("MediaSystemResourceManager::Init");
  ReentrantMonitorAutoEnter autoMon(barrier);
  bool done = false;

  nsCOMPtr<nsIRunnable> task =
    NS_NewRunnableFunction([&]() {
    if (!sSingleton) {
      sSingleton = new MediaSystemResourceManager();
    }
    ReentrantMonitorAutoEnter autoMon(barrier);
    done = true;
    barrier->NotifyAll();
  });

  ImageBridgeChild::GetSingleton()->GetMessageLoop()->PostTask(
    FROM_HERE, NewRunnableFunction(&task));

  // should stop the thread until done.
  while (!done) {
    barrier.Wait();
  }

@@ +86,5 @@
> +      sSingleton = new MediaSystemResourceManager();
> +    }
> +    return;
> +  }
> +  ReentrantMonitor barrier("MediaSystemResourceManager::Init");

I don't see why you need another monitor here; why can't you use mReentrantMonitor? Whenever you lock the barrier monitor, you also lock mReentrantMonitor, so it doesn't seem necessary to track both monitors?

@@ +198,5 @@
> +{
> +  MOZ_ASSERT(aClient);
> +  MOZ_ASSERT(!InImageBridgeChildThread());
> +
> +  {

You don't need the extra {} scope here; the function scope is sufficient to take the lock.

::: dom/media/systemservices/MediaSystemResourceManagerChild.h
@@ +48,5 @@
> +  virtual bool RecvResponse(const uint32_t& aId,
> +                            const bool& aSuccess) override;
> +
> +private:
> +  virtual void ActorDestroy(ActorDestroyReason aActorDestroyReason) override;

You only need one of "virtual" and "override" now, the Mozilla C++ Style guide no longer requires you to have both.

::: dom/media/systemservices/MediaSystemResourceManagerParent.cpp
@@ +59,5 @@
> +MediaSystemResourceManagerParent::ActorDestroy(ActorDestroyReason aReason)
> +{
> +  MOZ_ASSERT(!mDestroyed);
> +
> +  // Release all resource requests of the MediaSystemResourceManagerParent

Also mention in your comment that this clears all remaining pointers to this object, so that there's no possibility of something trying to use-after-free this object.

::: dom/media/systemservices/MediaSystemResourceService.cpp
@@ +99,5 @@
> +    return;
> +  }
> +
> +  // Try to acquire a resource
> +  if(resource->mAcquiredRequests.size() < resource->mResourceCount)

if (...) {

That is, space between "if" and "(", and "{" at the end of line, not start of following line.

@@ +186,5 @@
> +    return;
> +  }
> +
> +  std::deque<MediaSystemResourceRequest>::iterator it;
> +  std::deque<MediaSystemResourceRequest>& acquiredRequests =

You've written pretty much the same loop to erase a ResourceRequest from a std::deque<MediaSystemResourceRequest> four times in this class. I think you should have a helper function to do it.

Comment 56

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8626808 [details] [diff] [review]
patch part 4 - Change to OMXCodecProxy

Review of attachment 8626808 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/omx/OMXCodecWrapper.cpp
@@ +45,5 @@
>  bool
>  OMXCodecReservation::ReserveOMXCodec()
>  {
> +  mClient = new mozilla::MediaSystemResourceClient(mType);
> +  return mClient->AcquireSyncNoWait(); // don't wait if resrouce is not available

s/resrouce/resource/

Comment 57

[Nicolas Silva [:nical]]

(In reply to Chris Pearce (:cpearce) from comment #39)
> 
> You should also get an IPC peer to look at this. Maybe nical?

Oh my, I am doomed!


Sotaro, could you add a comment at the top of the main classes involved in this patch, explaining what they do? I don't know this code and it isn't quite clear from a quick glance what a MediaResourceManager is for. Should help with the review and for people discovering this code in general.

Comment 58

[Sotaro Ikeda [:sotaro]]

(In reply to Chris Pearce (:cpearce) from comment #55)
> Comment on attachment 8627221 [details] [diff] [review]
> patch part 1 - Add MediaResourceManager
> 
> Review of attachment 8627221 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall looks good. I would prefer to not have the synchronous acquires used
> in new code. We're trying to remove synchronous waits, as they're a source
> of bugs. New code (i.e. the Gonk backend for the MediaFormatReader, not the
> old MediaOmxReader) should be async, and it's easy to do now that we have
> MediaPromises.

As in comment 42, sync api is necessary for WebRTC. MediaPromises does not work the webrtc use case.

> 
> ::: dom/media/systemservices/MediaSystemResourceClient.h
> @@ +25,5 @@
> > +};
> > +
> > +/**
> > + * MediaSystemResourceClient is used to reserve a media resource like hw decoder.
> > + * When system has a limitation of a media resrouce, use this class
> 
> s/resrouce/resource/

Update in a next patch.

> 
> @@ +52,5 @@
> > +  // If the resource is used by others, wait until acquired.
> > +  void Acquire();
> > +
> > +  // Try to acquire media resource synchronously. If the resource is not
> > +  // available, fail to acquire it.
> 
> If the resource is not *immediately* available...


Update in a next patch.

> 
> @@ +53,5 @@
> > +  void Acquire();
> > +
> > +  // Try to acquire media resource synchronously. If the resource is not
> > +  // available, fail to acquire it.
> > +  // return false if resource is not acquired.
> 
> "return false if resource is not acquired." does not need to be said twice.


Update in a next patch.

> 
> @@ +56,5 @@
> > +  // available, fail to acquire it.
> > +  // return false if resource is not acquired.
> > +  // return false if resource is not acquired.
> > +  //
> > +  // This function should not be called on ImageBridge thread.
> 
> Add a comment here saying that synchronous AcqureSyncNoWait() is only here
> for compatibility with legacy code, and that new code should use the async
> Acquire().

As the above explanation, the function is not for media playback's legacy code. It is for WebRTC code and async api does not work for it.


> 
> ::: dom/media/systemservices/MediaSystemResourceManager.cpp
> @@ +76,5 @@
> > +    return;
> > +  }
> > +
> > +  if (InImageBridgeChildThread()) {
> > +    if (!sSingleton) {
> 
> It's confusing that you call MediaSystemResourceManager::Init() which
> dispatches a task which calls MediaSystemResourceManager_InitSync which
> calls MediaSystemResourceManager::Init() to unblock the original thread
> waiting in MediaSystemResourceManager::Init().
> 
> Can't you just allocate MediaSystemResourceManager() in
> MediaSystemResourceManager_InitSync instead of calling back into
> MediaSystemResourceManager::Init() to allocate it?
> 
> Or even, use a lambda for the task to allocate sSingleton from inside
> MediaSystemResourceManager::Init()? Like so:
> 
>   ReentrantMonitor barrier("MediaSystemResourceManager::Init");
>   ReentrantMonitorAutoEnter autoMon(barrier);
>   bool done = false;
> 
>   nsCOMPtr<nsIRunnable> task =
>     NS_NewRunnableFunction([&]() {
>     if (!sSingleton) {
>       sSingleton = new MediaSystemResourceManager();
>     }
>     ReentrantMonitorAutoEnter autoMon(barrier);
>     done = true;
>     barrier->NotifyAll();
>   });
> 
>   ImageBridgeChild::GetSingleton()->GetMessageLoop()->PostTask(
>     FROM_HERE, NewRunnableFunction(&task));
> 
>   // should stop the thread until done.
>   while (!done) {
>     barrier.Wait();
>   }

I'll update in a next patch.

> 
> @@ +86,5 @@
> > +      sSingleton = new MediaSystemResourceManager();
> > +    }
> > +    return;
> > +  }
> > +  ReentrantMonitor barrier("MediaSystemResourceManager::Init");
> 
> I don't see why you need another monitor here; why can't you use
> mReentrantMonitor? Whenever you lock the barrier monitor, you also lock
> mReentrantMonitor, so it doesn't seem necessary to track both monitors?

It is because MediaSystemResourceManager::Init() is static function. It could be called before MediaSystemResourceManager instance creation.


> 
> @@ +198,5 @@
> > +{
> > +  MOZ_ASSERT(aClient);
> > +  MOZ_ASSERT(!InImageBridgeChildThread());
> > +
> > +  {
> 
> You don't need the extra {} scope here; the function scope is sufficient to
> take the lock.

I'll update it in a next patch. In old patches, it was necessary.


> 
> ::: dom/media/systemservices/MediaSystemResourceManagerChild.h
> @@ +48,5 @@
> > +  virtual bool RecvResponse(const uint32_t& aId,
> > +                            const bool& aSuccess) override;
> > +
> > +private:
> > +  virtual void ActorDestroy(ActorDestroyReason aActorDestroyReason) override;
> 
> You only need one of "virtual" and "override" now, the Mozilla C++ Style
> guide no longer requires you to have both.

I'll update it in a next patch.

> 
> ::: dom/media/systemservices/MediaSystemResourceManagerParent.cpp
> @@ +59,5 @@
> > +MediaSystemResourceManagerParent::ActorDestroy(ActorDestroyReason aReason)
> > +{
> > +  MOZ_ASSERT(!mDestroyed);
> > +
> > +  // Release all resource requests of the MediaSystemResourceManagerParent
> 
> Also mention in your comment that this clears all remaining pointers to this
> object, so that there's no possibility of something trying to use-after-free
> this object.

Update in a next patch.

> 
> ::: dom/media/systemservices/MediaSystemResourceService.cpp
> @@ +99,5 @@
> > +    return;
> > +  }
> > +
> > +  // Try to acquire a resource
> > +  if(resource->mAcquiredRequests.size() < resource->mResourceCount)
> 
> if (...) {
> 
> That is, space between "if" and "(", and "{" at the end of line, not start
> of following line.

Update in a next patch.

> 
> @@ +186,5 @@
> > +    return;
> > +  }
> > +
> > +  std::deque<MediaSystemResourceRequest>::iterator it;
> > +  std::deque<MediaSystemResourceRequest>& acquiredRequests =
> 
> You've written pretty much the same loop to erase a ResourceRequest from a
> std::deque<MediaSystemResourceRequest> four times in this class. I think you
> should have a helper function to do it.

Apply it in a next patch.

Comment 59

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8627221 [details] [diff] [review]
patch part 1 - Add MediaResourceManager

Review of attachment 8627221 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/systemservices/MediaSystemResourceService.cpp
@@ +186,5 @@
> +    return;
> +  }
> +
> +  std::deque<MediaSystemResourceRequest>::iterator it;
> +  std::deque<MediaSystemResourceRequest>& acquiredRequests =

I re-checked the code, there are 4 time similar loop, but actually they are categorized to 2. Each has only 2 loops. Therefore, I think helper is not necessary.

Comment 60

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Add MediaResourceManager (r=cpearce)

Apply the comments.

Comment 61

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Change to ImageBridge

rebased.

Comment 62

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 3 - Change to MediaCodecProxy

rebased.

Comment 63

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 4 - Change to OMXCodecProxy

rebased.

Comment 64

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #57)
> Sotaro, could you add a comment at the top of the main classes involved in
> this patch, explaining what they do? I don't know this code and it isn't
> quite clear from a quick glance what a MediaResourceManager is for. Should
> help with the review and for people discovering this code in general.

I added some comments. And attachment 8615990 [details] (class diagram of MediaResourceManager) might help to understand the overview.

Comment 65

[Blake Wu [:bwu][:blakewu]]

Comment on attachment 8629084 [details] [diff] [review]
patch part 3 - Change to MediaCodecProxy

Review of attachment 8629084 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me! 
It would be nice to add some comments to explicitly let users know for audio case it is required to use AskMediaCodecAndWait() instead of AsyncAskMediaCodec().

Comment 66

[Blake Wu [:bwu][:blakewu]]

Comment on attachment 8629087 [details] [diff] [review]
patch part 4 - Change to OMXCodecProxy

Review of attachment 8629087 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/omx/OMXCodecWrapper.cpp
@@ +43,5 @@
>  };
>  
>  bool
>  OMXCodecReservation::ReserveOMXCodec()
>  {

Should we check mClient is already assigned or not, like the original check for mManagerService?

Comment 67

[Nicolas Silva [:nical]]

Comment on attachment 8629003 [details] [diff] [review]
patch part 1 - Add MediaResourceManager (r=cpearce)

Review of attachment 8629003 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/systemservices/MediaSystemResourceClient.h
@@ +30,5 @@
> +  virtual void ResourceReserveFailed() = 0;
> +};
> +
> +/**
> + * MediaSystemResourceClient is used to reserve a media system resource 

nit: trailing space

::: dom/media/systemservices/MediaSystemResourceManager.cpp
@@ +92,5 @@
> +
> +  ReentrantMonitor barrier("MediaSystemResourceManager::Init");
> +  ReentrantMonitorAutoEnter autoMon(barrier);
> +  bool done = false;
> + 

bit: trailing space

@@ +105,5 @@
> +    });
> +
> +  ImageBridgeChild::GetSingleton()->GetMessageLoop()->PostTask(
> +    FROM_HERE, new RunnableCallTask(runnable));
> + 

nit: trailing space

::: dom/media/systemservices/PMediaSystemResourceManager.ipdl
@@ +23,5 @@
> +
> +parent:
> +  async Acquire(uint32_t aId, MediaSystemResourceType aResourceType, bool aWillWait);
> +  async Release(uint32_t aId);
> +  async __delete__();

Is there something that guarantees that __delete__ cannot cross the Response message? In gfx, most of the times we have tried to send __delete__ from the child side, we've eventually run into this kind of issue and had to change the destruction sequence to have a Destroy message sent from child to parent answered by the __delete__ message from parent to child. If there is something in place (explicitly or implicitly) to prevent the issue in the case of PMediaSystemResourceManager, please explain it in a comment here. Issues with ipdl protocol shutdown keep coming back, so let's be careful and explicit about that part of the logic.

Comment 68

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 3 - Change to MediaCodecProxy

Apply the comment. Carry "r=bwu".

Comment 69

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 4 - Change to OMXCodecProxy

Apply the comment. Carry "r=bwu".

Comment 70

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8629003 [details] [diff] [review]
patch part 1 - Add MediaResourceManager (r=cpearce)

Review of attachment 8629003 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/systemservices/PMediaSystemResourceManager.ipdl
@@ +23,5 @@
> +
> +parent:
> +  async Acquire(uint32_t aId, MediaSystemResourceType aResourceType, bool aWillWait);
> +  async Release(uint32_t aId);
> +  async __delete__();

Yhea, it seems better to change the __delete__ like PTexture.

Comment 71

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Add MediaResourceManager

Apply the comment. Carry "r=cpearce,nical".

Comment 72

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=614e55679260

Comment 73

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a1f3980b9cb

Comment 74

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4a1f3980b9cb