Closed Bug 1097849 Opened 7 years ago Closed 7 years ago

Verify sane audio sample rates in MediaEncoder

Categories

(Core :: Audio/Video, defect)

Product:
Core
Component:
Audio/Video
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla36

People

(Reporter: rillian, Assigned: rillian)

Details

Attachments

(1 file, 3 obsolete files)

Assert valid audio sample rates v4
5.20 KB, patch
derf
: review+
Details | Diff | Splinter Review 
We think the encoder sample rate is controlled but the audio driver, but we should verify this assumption to avoid passing unreasonable sample rates to the encoding implementation.
Attached patch Assert valid audio sample rates (obsolete) — — Splinter Review 
Per irc discussion.
Assignee: nobody → giles
Attachment #8521554 - Flags: review?(tterribe)
Attachment #8521554 - Flags: review?(tterribe) → review+
s/196/192/ carrying forward r=derf
Attachment #8521554 - Attachment is obsolete: true
Attachment #8521564 - Flags: review+
Actual updated patch. This is what landed. Sorry for the confusion.
Attachment #8521564 - Attachment is obsolete: true
Attachment #8521569 - Flags: review+
Turns out we have unit tests that hit this assert. Backed out. Please make sure this is green on Try before attempting to re-land ;)
https://hg.mozilla.org/integration/mozilla-inbound/rev/cff5f009a657

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3824640&repo=mozilla-inbound
Updated patch to address gtest failures.

- Return an error rather than asserting.
- Update unit tests to verify the valid sample rate boundaries.

MOZ_ASSERT isn't caught by the gtest framework and so is always fatal. Since we have unit tests verifying that invalid values are rejected, this doesn't pass automated tests. Returning an error instead is reasonable for an Init method, and we already do this for other parameters.
Attachment #8521569 - Attachment is obsolete: true
Attachment #8521896 - Flags: review?(tterribe)
Comment on attachment 8521896 [details] [diff] [review]
Assert valid audio sample rates v4

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

Hooray tests!
Attachment #8521896 - Flags: review?(tterribe) → review+
https://hg.mozilla.org/mozilla-central/rev/4f9e282d1adb
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.