Closed Bug 951770 Opened 6 years ago Closed 6 years ago

Reject invalid WebM/Opus files

Categories

(Core :: Audio/Video, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30
Tracking Status
firefox27 --- unaffected
firefox28 --- affected
firefox29 + fixed
firefox30 --- fixed

People

(Reporter: rillian, Assigned: rillian)

References

Details

Attachments

(1 file, 1 obsolete file)

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 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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cddbc0d5183

Better to get this in even without a test.
Flags: in-testsuite-
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.
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)
Attachment #8369293 - Flags: review?(chris.double) → review+
https://hg.mozilla.org/mozilla-central/rev/06e99d1896ce
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Flags: in-testsuite- → in-testsuite+
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+
You need to log in before you can comment on or make changes to this bug.