ffmpeg decoder returning errors if no frame decoded.

VERIFIED FIXED in Firefox -esr52

Status

()

Core
Audio/Video: Playback
P1
normal
VERIFIED FIXED
a year ago
10 months ago

People

(Reporter: jya, Assigned: jya)

Tracking

({regression})

52 Branch
mozilla55
regression
Points:
---

Firefox Tracking Flags

(firefox-esr52 fixed, firefox53 wontfix, firefox54 verified, firefox55 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

a year ago
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.
status-firefox53: --- → wontfix
status-firefox54: --- → affected
status-firefox55: --- → affected
Version: unspecified → 52 Branch

Updated

11 months ago
Priority: -- → P3
(Assignee)

Comment 1

11 months ago
The problem is in FFmpeg MediaDataDecoder...
Summary: mochitest short media files are invalid → ffmpeg decoder returning errors if no frame decoded.
(Assignee)

Updated

11 months ago
Priority: P3 → P1
Comment hidden (mozreview-request)

Updated

11 months ago
Assignee: nobody → jyavenard

Comment 3

11 months ago
mozreview-review
Comment on attachment 8869350 [details]
Bug 1364870: [ffmpeg] only check format if frame has been decoded.

https://reviewboard.mozilla.org/r/140988/#review144514
Attachment #8869350 - Flags: review?(gsquelart) → review+

Comment 4

11 months ago
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0333314aa0e9
[ffmpeg] only check format if frame has been decoded. r=gerald

Comment 5

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0333314aa0e9
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Can this ride the 55 train or should it be considered for backport?
status-firefox-esr52: --- → affected
Flags: needinfo?(jyavenard)
(Assignee)

Comment 7

11 months ago
I'd like this backported. Yes.
Flags: needinfo?(jyavenard)
(In reply to Jean-Yves Avenard [:jya] from comment #7)
> I'd like this backported. Yes.

Can you request uplift, then, please? :)
Flags: needinfo?(jyavenard)
(Assignee)

Comment 9

11 months ago
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
Flags: needinfo?(jyavenard)
Attachment #8869350 - Flags: approval-mozilla-beta?

Comment 10

11 months ago
Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(brindusa.tot)
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.
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
Flags: needinfo?(brindusa.tot)

Comment 12

11 months ago
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.
Attachment #8869350 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 13

11 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/808da66ef9e2
status-firefox54: affected → fixed
Flags: qe-verify+
Reproduced on Firefox 54 beta 11 under Ubuntu 16.04 LTS 64-bit.
Verified as fixed on Firefox 54 beta 12.
status-firefox54: fixed → verified
Flags: qe-verify+
Is this something we should consider for ESR52 too? If so, it'll need a rebased patch.
Flags: needinfo?(jyavenard)
(Assignee)

Comment 16

10 months ago
Yes, we should....

I'll wrap a patch (though I'm surprised the current one wouldn't apply)
Flags: needinfo?(jyavenard)
(Assignee)

Updated

10 months ago
Flags: needinfo?(jyavenard)
(Assignee)

Comment 17

10 months ago
Created attachment 8883881 [details] [diff] [review]
[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
(Assignee)

Comment 18

10 months ago
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
Flags: needinfo?(jyavenard)
Attachment #8883881 - Flags: approval-mozilla-esr52?

Comment 19

10 months ago
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.
Attachment #8883881 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+

Comment 20

10 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-esr52/rev/de8deecbcb02
status-firefox-esr52: affected → fixed
You need to log in before you can comment on or make changes to this bug.