1337046 - Deliver decoded DXVA frames asynchronously

Status: RESOLVED WONTFIX

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

Description

[Bas Schouten (:bas.schouten)]

Right now, on every frame we block the decoder thread until the GPU is done decoding that frame, this will in theory often starve the GPU of work. We can probably do better.

Comment 1

[Bas Schouten (:bas.schouten)]

Attachment: Part 1: MozPromise-ify MFTManager::Output

This functionally doesn't change anything and theoretically just makes things slower by bouncing to the event loop a couple of times. But it is the basis for any further modifications I'm going to make. Let me know if this looks alright. I've confirmed it still plays fine at least.

Comment 2

[Jean-Yves Avenard [:jya]]

Comment on attachment 8834055 [details] [diff] [review]
Part 1: MozPromise-ify MFTManager::Output

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

Any chance you can use reviewboard, using bugzilla to review is so inconvenient (and so 2015 :) )

This approach isn't really extendable, and the problem right now is that draining is broken.

::: dom/media/platforms/wmf/WMFAudioMFTManager.cpp
@@ +229,5 @@
>    int typeChangeCount = 0;
>    while (true) {
>      hr = mDecoder->Output(&sample);
>      if (hr == MF_E_TRANSFORM_NEED_MORE_INPUT) {
> +      return MediaDataPromise::CreateAndReject(MF_E_TRANSFORM_NEED_MORE_INPUT, "WMFAudioMFTManager::Output");

80 columns formatting,
use __func__ as second argument

@@ +345,5 @@
>    LOG("Decoded audio sample! timestamp=%lld duration=%lld currentLength=%u",
>        timestamp.ToMicroseconds(), duration.ToMicroseconds(), currentLength);
>    #endif
>  
> +  return MediaDataPromise::CreateAndResolve(output, "WMFAudioMFTManager::Output");;

you can use __func__ rather than set the full text all the time

::: dom/media/platforms/wmf/WMFMediaDataDecoder.cpp
@@ +139,5 @@
>  WMFMediaDataDecoder::ProcessOutput()
>  {
>    RefPtr<MediaData> output;
>    HRESULT hr = S_OK;
>    DecodedData results;

there's no longer a need for these variables.

@@ +141,5 @@
>    RefPtr<MediaData> output;
>    HRESULT hr = S_OK;
>    DecodedData results;
> +
> +  mMFTManager->Output(mLastStreamOffset)->Then(mTaskQueue, __func__, this,

There's something wrong in this code, draining will be broken now.
WMFMediaDataDecoder::ProcessOutput() used to loop until all available frames were returned.

This code can only ever return the first one now.

That's fine with typical usage, as 99% of the time, the MFTManager would only return a single frame. But that's not always true, and definitely not true when draining.

You also need to use a MozPromiseRequestHolder, which would allow you to disconnect that promise.
It is possible for the decoder to have been shutdown before the promise got resolved, meaning the taskqueue may no longer be usable by the time the promise gets resolved.
So it's important that we can disconnect it.

So either:
1- serialise the call to mMFTManager->Output so that whenever WMFMediaDataDecoder::OnMediaDataResolved gets resolved it calls again ProcessOutput; or
2- use MozPromise::All on an array of promises.

The problem with 1, is that it assumes you treat all decoded samples sequentially, and maybe it would be nice to really parallelise the entire thing, which is what 2 would allow, in which case you can't assume that MF_E_TRANSFORM_NEED_MORE_INPUT will only ever be returned once all promises have resolved.

So maybe mMFTManager->Output should return an array of promises instead and you move the looping from MWFMediaDataDecoder to the MFTManager instead. Not sure if that's the best approach.

@@ +148,5 @@
> +}
> +
> +void
> +WMFMediaDataDecoder::OnMediaDataResolved(RefPtr<MediaData> aMediaData)
> +{ 

trailing space

@@ +168,2 @@
>      if (!mDecodePromise.IsEmpty()) {
> +      mDecodePromise.Resolve(Move(mDecodedData), __func__);

As per review in bug 1319987, don't use Move(mDecodedData); instead

Resolve(mDecodedData);
mDecodedData.Clear();

it would also be easier to simply do:
mDecodePromise.ResolveIfExists(...)

@@ +179,5 @@
>      if (!mDecodePromise.IsEmpty()) {
>        mDecodePromise.Reject(error, __func__);
>      }
>      else {
>        mDrainPromise.Reject(error, __func__);

mDecoderPromise.RejectIfExists(...)
mDrainPromise.RejectIfExists(...)

I understand that I did the same in my earlier patch, but this got modified since following reviews.

::: dom/media/platforms/wmf/WMFMediaDataDecoder.h
@@ +17,5 @@
>  
>  namespace mozilla {
>  
> +#define REJECT_IF_UNTRUE(x, type, ret)                              \
> +  do {                                                        \

may want to align the \ seeing that they are mostly aligned.

I personally strongly dislike the use of macro to do such task.
And do we need the use for type? we're always dealing with MediaDataPromise

::: dom/media/platforms/wmf/WMFVideoMFTManager.cpp
@@ +909,5 @@
>    // handle occurs.
>    while (true) {
>      hr = mDecoder->Output(&sample);
>      if (hr == MF_E_TRANSFORM_NEED_MORE_INPUT) {
> +      return MediaDataPromise::CreateAndReject(MF_E_TRANSFORM_NEED_MORE_INPUT, "WMFVideoMFTManager::Output");

use __func__ and 80 columns formatting

Comment 3

[Jean-Yves Avenard [:jya]]

Wit bug 1336947, the draining issue will be fixed. Instead the MediaFormatReader will call drain several times until there's no more frames coming

Comment 4

[Bas Schouten (:bas.schouten)]

Most review comments seem pretty straight forward, a couple of questions:

(In reply to Jean-Yves Avenard [:jya] from comment #2)
> Comment on attachment 8834055 [details] [diff] [review]
> Part 1: MozPromise-ify MFTManager::Output
> 
> Review of attachment 8834055 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Any chance you can use reviewboard, using bugzilla to review is so
> inconvenient (and so 2015 :) )

Yup, I wanted to do so anyway initially, but then I realized I wanted feedback? and not review? which it doesn't allow afaik. I don't know why I ended up doing r?, force of habit I think :-).

> @@ +141,5 @@
> >    RefPtr<MediaData> output;
> >    HRESULT hr = S_OK;
> >    DecodedData results;
> > +
> > +  mMFTManager->Output(mLastStreamOffset)->Then(mTaskQueue, __func__, this,
> 
> There's something wrong in this code, draining will be broken now.
> WMFMediaDataDecoder::ProcessOutput() used to loop until all available frames
> were returned.
> 
> This code can only ever return the first one now.
> 
> That's fine with typical usage, as 99% of the time, the MFTManager would
> only return a single frame. But that's not always true, and definitely not
> true when draining.

Don't I keep looping through to the event loop until I don't get a new frame before resolving the actual promise? I -think- this part is okay, although I didn't test it.

> You also need to use a MozPromiseRequestHolder, which would allow you to
> disconnect that promise.
> It is possible for the decoder to have been shutdown before the promise got
> resolved, meaning the taskqueue may no longer be usable by the time the
> promise gets resolved.
> So it's important that we can disconnect it.

Ah! Good to know, I didn't realize mTaskQueue was a per decoder object!

> So either:
> 1- serialise the call to mMFTManager->Output so that whenever
> WMFMediaDataDecoder::OnMediaDataResolved gets resolved it calls again
> ProcessOutput; or

I serialize them now except just my calling MFTManager::Output again on every MediaDataResolved code, afaict this is functionally equivelant.

> ::: dom/media/platforms/wmf/WMFMediaDataDecoder.h
> @@ +17,5 @@
> >  
> >  namespace mozilla {
> >  
> > +#define REJECT_IF_UNTRUE(x, type, ret)                              \
> > +  do {                                                        \
> 
> may want to align the \ seeing that they are mostly aligned.
> 
> I personally strongly dislike the use of macro to do such task.
> And do we need the use for type? we're always dealing with MediaDataPromise

Hmm, I wanted to make it potentially re-usable as you guys have a -lot- of NS_ENSURE_TRUE's in your code, from their usage I figured this was your preferred style of dealing with this? Considering the amount of failure paths and to avoid code clutter and Mozilla doesn't support exceptions, what is your preferred style of dealing with this?

Comment 5

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

This change is no longer required.