Closed Bug 1304994 Opened 3 years ago Closed 3 years ago

WMF decoder could potentially lose a frame, causing decoding failures

Categories

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

x86
Windows
defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: jya, Assigned: alwu)

Details

Attachments

(1 file, 1 obsolete file)

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
Alastor, 
Can you check it?
Assignee: nobody → 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!
Attachment #8825621 - Flags: feedback?(jyavenard)
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)
(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)
Attachment #8825621 - Attachment is obsolete: true
Attachment #8825621 - Flags: feedback?(jyavenard)
Attachment #8826024 - Flags: feedback?(jyavenard)
Comment on attachment 8825621 [details]
bug 1304994 - resubmit input data when MFT returns MF_E_NOTACCEPTING.

https://reviewboard.mozilla.org/r/103736/#review105038
Attachment #8825621 - Flags: review-
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.
Hi, Jya,
Could you help me check the new patch?
Thanks!
Flags: needinfo?(jyavenard)
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.
(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]
That's just because how they queue task for all decoding. simpler for them that way.
it's not a loop per say
Flags: needinfo?(jyavenard)
Comment on attachment 8826024 [details]
bug 1304994 - resubmit input data when MFT returns MF_E_NOTACCEPTING.

https://reviewboard.mozilla.org/r/104074/#review106626
Attachment #8826024 - Flags: review?(jyavenard) → review+
https://hg.mozilla.org/mozilla-central/rev/2d39564d82f3
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.