Verify sane audio sample rates in MediaEncoder

RESOLVED FIXED in mozilla36

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: rillian, Assigned: rillian)

Tracking

Trunk
mozilla36
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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.
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: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.