1179667 - Use MediaPromise for Gonk PlatformDecodeModule Init()

Status: RESOLVED FIXED

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

Comment 1

[Alfredo Yang (:alfredo)]

Attachment: pdm_init_by_promise_gonk

Comment 2

[Jean-Yves Avenard [:jya]]

Comment on attachment 8647415 [details] [diff] [review]
pdm_init_by_promise_gonk

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

You need to sort out the threading model, and remove the data races. This is the most important part.

Now, for the implementation details:
You have duplicated the code of Shutdown() in both the audio and video decoder manager ; and they do exactly the same thing.
There's now duplicated code for no real benefit.

You could instead make the MozPromiseHolder<InitPromise> mInitPromise;  member a protected member of GonkDecoderManager and have a unique, common implementation for both audio and video when it comes to shutdown.

you have a mDecoder member in each of the audio and video class, make mDecoder (android::sp<android::MediaCodecProxy> mDecoder) a protected member of the base class as well as Shutdown()

This will greatly simplify the implementation I think.

Ping me on IRC if you have any questions, we can do a virtual whiteboard session too over video if you want...

::: dom/media/platforms/gonk/GonkMediaDataDecoder.h
@@ +39,5 @@
>  
>    // Flush the queued sample.
>    virtual nsresult Flush() = 0;
>  
> +  // Shutdown decoder and rejects the init promise.

Why would you need this?

This is called from the main Shutdown() and the implementation in each of the audio or video decoder manager is identical.
So why the need to duplicate the code ?

::: dom/media/platforms/gonk/GonkVideoDecoderManager.cpp
@@ +109,5 @@
>        MediaCodecProxy::kCanExposeGraphicBuffer)) {
>      mNativeWindow = new GonkNativeWindow();
>    }
>  
> +  return mInitPromise.Ensure(__func__);

this is likely racy.

Which thread will call codecReserved, I'm guessing a different thread.

What if it calls the callback before you have mInitPromise being initialized?

It used to wait for the mediacodec to complete so it wasn't as big of a problem (there was still a data race whose behaviour is undefined in C++11); but now you have the promise holder initialized in one thread ; and it can be resolved in either the reader's task queue (if Shutdown is called) or the media codec's thread (if initialization completed): and both could be happening at the same time.

You need to sort out the threading model first. I suggest ensuring every tasks is done in the MediaDataDecoder's task queue, and everything not running in that task queue queue a task. This is what's done in the WMF PDM and now the Apple's PDM

A good way would be to have Init() use ProxyMediaCall to call say a ProcessInit() which will return the init promise.

Then codecReserved need to schedule a task on the task queue so that all the initialization of mDecoder is done on the same thread (the GonkMediaDataDecoder's task queue)

*then* you can resolve the promise.

Doing so would also let you have a common GonkDecoderManager::Init() and a virtual ProcessInit().

@@ +492,5 @@
>    }
>  
>    if (rv != OK) {
>      GVDM_LOG("Failed to configure codec!!!!");
>      mReaderCallback->Error();

you don't want to call the callback->Error() during initialisation. MediaFormatReader has no handling for this case. That's the whole point of rejecting the promise :)

Comment 3

[Alfredo Yang (:alfredo)]

Attachment: pdm_init_by_promise_gonk

Remove duplicate codes and use reader's TaskQueue as the task dispatch target for codec resource management.

Comment 4

[Jean-Yves Avenard [:jya]]

Comment on attachment 8648636 [details] [diff] [review]
pdm_init_by_promise_gonk

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

much better !

r=me with comments addressed.

::: dom/media/platforms/gonk/GonkMediaDataDecoder.cpp
@@ +5,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  #include "GonkMediaDataDecoder.h"
>  #include "VideoUtils.h"
>  #include "nsTArray.h"
> +#include "MediaData.h"

why do you need to include MediaData.h?

::: dom/media/platforms/gonk/GonkMediaDataDecoder.h
@@ +41,5 @@
>    // Flush the queued sample.
>    virtual nsresult Flush() = 0;
>  
> +  // Shutdown decoder and rejects the init promise.
> +  virtual nsresult Shutdown();

why did you make this virtual? it's not overridden anywhere.

::: dom/media/platforms/gonk/GonkVideoDecoderManager.cpp
@@ +544,5 @@
>  
>  void
>  GonkVideoDecoderManager::VideoResourceListener::codecReserved()
>  {
> +  if (mManager && !mManager->mInitPromise.IsEmpty()) {

this is not thread-safe as you're accessing the mInitPromise from two different threads and this operation isn't atomic.

Dispatch your task, and simply do mInitPromise.RejectIfExists() there.

@@ +554,5 @@
>  
>  void
>  GonkVideoDecoderManager::VideoResourceListener::codecCanceled()
>  {
> +  if (mManager && !mManager->mInitPromise.IsEmpty()) {

same here.

Plus, even if it failed you want to set mDecoder = nullptr no ? so seems appropriate to always call codecCanceled() anyway

::: dom/media/platforms/gonk/GonkVideoDecoderManager.h
@@ +168,4 @@
>    // This monitor protects mQueueSample.
>    Monitor mMonitor;
>  
> +  // This TaskQueue should be the same one in mReaderCallback->OnReaderTaskQueue().

This really needs to be on a dedicate task queue ; too bad they can't be created outside the main thread.

Comment 5

[Alfredo Yang (:alfredo)]

(In reply to Jean-Yves Avenard [:jya] from comment #4)
> Comment on attachment 8648636 [details] [diff] [review]
> pdm_init_by_promise_gonk
> 
> Review of attachment 8648636 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> much better !
> 
> r=me with comments addressed.
> 

Thanks for review. I'll address all review comments at next patch.

Comment 6

[Alfredo Yang (:alfredo)]

Attachment: pdm_init_by_promise_gonk

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

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/37c52f612c75

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/37c52f612c75