1304994 - WMF decoder could potentially lose a frame, causing decoding failures

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

It is possible for the WMF decoder to return MF_E_NOTACCEPTING which per MS documentation that it has enough data to produce one or more output samples.

ProcessOutput should be then called to retrieve the pending frames.

However, here the original input is never resubmitted to the decoder and will be discarded.

Follow up frames won't be able to be decoded if they depend on this dropped frame.

When the MFT decoder returns MF_E_NOTACCEPTING, then we should call ProcessOutput() and right after resubmit the sample via ProcessInput

Comment 1

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

Alastor, 
Can you check it?

Comment 2

[Alastor Wu [:alwu]]

Attachment: bug 1304994 - resubmit input data when MFT returns MF_E_NOTACCEPTING.

When MFT returns MF_E_NOTACCEPTING means the input buffer is full and it can't
accept input data anymore, so we need to output the data first and then resummit
the input data.

Review commit: https://reviewboard.mozilla.org/r/103736/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/103736/

Comment 3

[Alastor Wu [:alwu]]

Comment on attachment 8825621 [details]
bug 1304994 - resubmit input data when MFT returns MF_E_NOTACCEPTING.

Hi, Jya,
Could you give me some feedback?
Thanks!

Comment 4

[Jean-Yves Avenard [:jya]]

Comment on attachment 8825621 [details]
bug 1304994 - resubmit input data when MFT returns MF_E_NOTACCEPTING.

https://reviewboard.mozilla.org/r/103736/#review104748

::: dom/media/platforms/wmf/WMFMediaDataDecoder.cpp:127
(Diff revision 1)
>  
>    HRESULT hr = mMFTManager->Input(aSample);
> +  if (hr == MF_E_NOTACCEPTING) {
> +    while (hr != MF_E_NOTACCEPTING) {
> +      ProcessOutput();
> +      hr = mMFTManager->Input(aSample);

doesnt this mean that the same sample will be submitted multiple times (and likely in an infinite loop)

Example:
first input returns MF_NOTACCEPTING
so we output
no input returns NS_OK

thats different to MF_E_NOTACCEPTING! so it will resubmit it again, and will loop forever if the sample is a keyframe)

Comment 5

[Alastor Wu [:alwu]]

(In reply to Jean-Yves Avenard [:jya] from comment #4)
> doesnt this mean that the same sample will be submitted multiple times (and
> likely in an infinite loop)

Ah, sorry, I want to loop it until the return value is not MF_E_NOTACCEPTING.
I'll change the condition to 
> while (hr == MF_E_NOTACCEPTING)

Comment 6

[Alastor Wu [:alwu]]

Attachment: bug 1304994 - resubmit input data when MFT returns MF_E_NOTACCEPTING.

When MFT returns MF_E_NOTACCEPTING means the input buffer is full and it can't
accept input data anymore, so we need to output the data first and then resummit
the input data.

Review commit: https://reviewboard.mozilla.org/r/104074/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104074/

Comment 7

[Jean-Yves Avenard [:jya]]

Comment on attachment 8825621 [details]
bug 1304994 - resubmit input data when MFT returns MF_E_NOTACCEPTING.

https://reviewboard.mozilla.org/r/103736/#review105038

Comment 8

[Alastor Wu [:alwu]]

Sorry, I don't know why the new patch on review board becomes another new commit, and I can't merge them into the one. The new patch fixes the issue mentioned in the comment4.

Comment 9

[Alastor Wu [:alwu]]

Hi, Jya,
Could you help me check the new patch?
Thanks!

Comment 10

[Jean-Yves Avenard [:jya]]

Comment on attachment 8826024 [details]
bug 1304994 - resubmit input data when MFT returns MF_E_NOTACCEPTING.

https://reviewboard.mozilla.org/r/104074/#review106222

::: dom/media/platforms/wmf/WMFMediaDataDecoder.cpp:124
(Diff revision 1)
>      // Skip sample, to be released by runnable.
>      return;
>    }
>  
>    HRESULT hr = mMFTManager->Input(aSample);
> +  while (hr == MF_E_NOTACCEPTING) {

I don't think there's any reason to loop.

ProcessOutput already output in a loop everything there is to output.
As such, it is not possible for input to return MF_E_NOTACCEPTING more than once. Or if it does, it's an error condition and calling Output won't help.

So you would still loop infinitely if it were to occur.

May as well, simply call ProcessOutput once, and if Input return MF_E_NOTACCEPTING treat it as an error.

So the code can simply be:
if (hr == MF_E_NOTACCEPTING) {
  ProcessOutput();
  hr = mMFTManager=>Input(aSample);
}

and that's it.
That's also what chrome does.

Comment 11

[Alastor Wu [:alwu]]

(In reply to Jean-Yves Avenard [:jya] from comment #10)
> I don't think there's any reason to loop.
> 
> ProcessOutput already output in a loop everything there is to output.
> As such, it is not possible for input to return MF_E_NOTACCEPTING more than
> once. Or if it does, it's an error condition and calling Output won't help.

But Chrome did the double check in their code, it seems we have possibility to get the MF_E_NOTACCEPTING again [1].

[1] https://cs.chromium.org/chromium/src/media/gpu/dxva_video_decode_accelerator_win.cc?l=2239

[1]

Comment 12

[Jean-Yves Avenard [:jya]]

That's just because how they queue task for all decoding. simpler for them that way.
it's not a loop per say

Comment 13

[Alastor Wu [:alwu]]

Comment on attachment 8826024 [details]
bug 1304994 - resubmit input data when MFT returns MF_E_NOTACCEPTING.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104074/diff/1-2/

Comment 14

[Jean-Yves Avenard [:jya]]

Comment on attachment 8826024 [details]
bug 1304994 - resubmit input data when MFT returns MF_E_NOTACCEPTING.

https://reviewboard.mozilla.org/r/104074/#review106626

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2d39564d82f3