Reject invalid WebM/Opus files

RESOLVED FIXED in Firefox 29

Status

()

Core
Audio/Video
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: rillian, Assigned: rillian)

Tracking

Trunk
mozilla30
x86_64
Linux
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox27 unaffected, firefox28 affected, firefox29+ fixed, firefox30 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

In constructing tests for VP9 playback, we discovered Firefox will play badly muxed Opus tracks in webm. We should reject them instead.

In particular, we should reject files whose CodecDelay track property doesn't match the value from the OpusHeader CodecPrivate.

Comment 1

4 years ago
Created attachment 8350500 [details] [diff] [review]
0001-Bug-951770-Reject-invalid-WebM-Opus-files-r-rillian.patch
Attachment #8350500 - Flags: review?(giles)
(Assignee)

Comment 2

4 years ago
Comment on attachment 8350500 [details] [diff] [review]
0001-Bug-951770-Reject-invalid-WebM-Opus-files-r-rillian.patch

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

Can you add a test file for this as well?
Attachment #8350500 - Flags: review?(giles) → review+
(Assignee)

Comment 3

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cddbc0d5183

Better to get this in even without a test.
Flags: in-testsuite-
(Assignee)

Comment 4

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4978efcdad9

The code generates a signed-compare warning because mCodecDelay is an unsigned long long, but CheckedInt64::value() returns an signed int64_t. Casting this warning away is safe.

I built this locally, but without -Werror, so I didn't notice the problem until tbpl failed to build.
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/2b6516c8713e for timeouts in test_playback.html.
(Assignee)

Comment 7

4 years ago
Created attachment 8369293 [details] [diff] [review]
reject invalid Opus pre-skip in WebM v2

Roll up warning and semicolon fixes from the previous landing attempt.

Tests failed because detodos.webm had exactly the corrupt header we now reject.

- move detodos.webm to invalid-preskip.webm
- make a new detodos.webm.

Also changed the debug message to use PR_LOG_WARNING and mention the file is invalid.
Assignee: nobody → giles
Attachment #8350500 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8369293 - Flags: review?(chris.double)

Updated

4 years ago
Attachment #8369293 - Flags: review?(chris.double) → review+
https://hg.mozilla.org/mozilla-central/rev/06e99d1896ce
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
(Assignee)

Updated

4 years ago
Flags: in-testsuite- → in-testsuite+
(Assignee)

Updated

4 years ago
status-firefox27: --- → unaffected
status-firefox28: --- → affected
status-firefox29: --- → affected
tracking-firefox29: --- → ?
Comment on attachment 8369293 [details] [diff] [review]
reject invalid Opus pre-skip in WebM v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

Feature Bug 938686.

User impact if declined:

There's some buggy encoder software generating invalid files. We want user agents to reject these as soon as possible to ensure fixed encoders get deployed.

Testing completed (on m-c, etc.):

Landed on m-c with tests, verified manually against wild files.

Risk to taking this patch (and alternatives if risky):

Risk is low. This is a new media format and use by content developers is still in the exploratory stage. The larger risk is that some bug in the patch affects playback of non-Opus WebM files, but we have tests.

String or IDL/UUID changes made by this patch:

none.
Attachment #8369293 - Flags: approval-mozilla-aurora?
Attachment #8369293 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
tracking-firefox29: ? → +
https://hg.mozilla.org/releases/mozilla-aurora/rev/dd51a2464396
status-firefox29: affected → fixed
status-firefox30: --- → fixed
You need to log in before you can comment on or make changes to this bug.