1364870 - ffmpeg decoder returning errors if no frame decoded.

Status: VERIFIED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

In bug 1257116, many media files use in mochitests were shortened.

However, those files are invalid and start with data that can't be decoded.

They happen to work because we skip data with error.

Those files should be remade so that they don't contain errors.

Comment 1

[Jean-Yves Avenard [:jya]]

The problem is in FFmpeg MediaDataDecoder...

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1364870: [ffmpeg] only check format if frame has been decoded.

The format member isn't set when an audio frame hasn't been decoded yet.

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

Comment 3

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8869350 [details]
Bug 1364870: [ffmpeg] only check format if frame has been decoded.

https://reviewboard.mozilla.org/r/140988/#review144514

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0333314aa0e9
[ffmpeg] only check format if frame has been decoded. r=gerald

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0333314aa0e9

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Can this ride the 55 train or should it be considered for backport?

Comment 7

[Jean-Yves Avenard [:jya]]

I'd like this backported. Yes.

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to Jean-Yves Avenard [:jya] from comment #7)
> I'd like this backported. Yes.

Can you request uplift, then, please? :)

Comment 9

[Jean-Yves Avenard [:jya]]

Comment on attachment 8869350 [details]
Bug 1364870: [ffmpeg] only check format if frame has been decoded.

Approval Request Comment
[Feature/Bug causing the regression]: 1270016
[User impact if declined]: audio decoding error with some contents. 
[Is this code covered by automated tests?]: yes. Though as errors are ignored, the regression didn't get spotted. This is being worked out. 
[Has the fix been verified in Nightly?]: manually tested
[Needs manual test from QE? If yes, steps to reproduce]: create/set the pref media.audio-max-decode-error and set it to 0. Attempt to play the gizmo.mp4 in dom/media/test on Linux. Without the fix, the file won't play
[List of other uplifts needed for the feature/fix]: no
[Is the change risky?]: no
[Why is the change risky/not risky?]: we only check the file format once decoded. 
[String changes made/needed]: None

Comment 10

[Gerry Chang [:gchang]]

Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!

Comment 11

[Brindusa Tot, Desktop QA]

Reproduced the issue on Nightly 55.0a1, on a build from 05/06/2017 on Ubuntu 16.04 x64.

Verified as fixed on the latest Nightly, 55.0a1, build ID 20170525100253 on Ubuntu 16.04 x64.

Comment 12

[Gerry Chang [:gchang]]

Comment on attachment 8869350 [details]
Bug 1364870: [ffmpeg] only check format if frame has been decoded.

Fix a regression and was verified. Beta54+. Should be in 54 beta 12.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/808da66ef9e2

Comment 14

[Petruta Horea [:phorea], Desktop QA]

Reproduced on Firefox 54 beta 11 under Ubuntu 16.04 LTS 64-bit.
Verified as fixed on Firefox 54 beta 12.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider for ESR52 too? If so, it'll need a rebased patch.

Comment 16

[Jean-Yves Avenard [:jya]]

Yes, we should....

I'll wrap a patch (though I'm surprised the current one wouldn't apply)

Comment 17

[Jean-Yves Avenard [:jya]]

Attachment: [ffmpeg] only check format if frame has been decoded. r=gerald, a=gchang

The format member isn't set when an audio frame hasn't been decoded yet.

MozReview-Commit-ID: IgUj6bjVzdF

Comment 18

[Jean-Yves Avenard [:jya]]

Comment on attachment 8883881 [details] [diff] [review]
[ffmpeg] only check format if frame has been decoded. r=gerald, a=gchang

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Decoding error with some audio files
Fix Landed on Version: 54
Risk to taking this patch (and alternatives if risky): no risk
String or UUID changes made by this patch:  none

Comment 19

[Gerry Chang [:gchang]]

Comment on attachment 8883881 [details] [diff] [review]
[ffmpeg] only check format if frame has been decoded. r=gerald, a=gchang

Fix an audio playback issue. Let's uplift this to ESR52.3.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/de8deecbcb02