1097849 - Verify sane audio sample rates in MediaEncoder

Status: RESOLVED FIXED

Description

[Ralph Giles (:rillian) | needinfo me]

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.

Comment 1

[Ralph Giles (:rillian) | needinfo me]

Attachment: Assert valid audio sample rates

Per irc discussion.

Comment 2

[Ralph Giles (:rillian) | needinfo me]

Attachment: Assert valid audio sample rates v2

s/196/192/ carrying forward r=derf

Comment 3

[Ralph Giles (:rillian) | needinfo me]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3fe4db1f0877

Comment 4

[Ralph Giles (:rillian) | needinfo me]

Attachment: Assert valid audio sample rates v3

Actual updated patch. This is what landed. Sorry for the confusion.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

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

Comment 6

[Ralph Giles (:rillian) | needinfo me]

Attachment: Assert valid audio sample rates v4

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.

Comment 7

[Timothy B. Terriberry (:derf)]

Comment on attachment 8521896 [details] [diff] [review]
Assert valid audio sample rates v4

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

Hooray tests!

Comment 8

[Ralph Giles (:rillian) | needinfo me]

This one builds on try. https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7e51fe546d1d

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f9e282d1adb

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4f9e282d1adb