According to the Opus documentation[1], the sampling rate of input signal(Hz) must be one of 8000, 12000, 16000, 24000, or 48000. However, our current implementation: if (!((aSamplingRate >= 8000) && (kOpusSamplingRate / aSamplingRate) * aSamplingRate == kOpusSamplingRate)) { ... } misses the case where aSamplingRate is 9600, where it should be resampled by resampler, but not. [1] http://www.opus-codec.org/docs/html_api-1.0.2/group__opus__encoder.html#gaa89264fd93c9da70362a0c9b96b9ca88

Hi Ralph, this should be a better solution to check whether the input sampling rate is supported by Opus, could you review it at your convenience? Thanks :)

Comment on attachment 8342132 [details] [diff] [review] Correct the checking of input sampling rate Review of attachment 8342132 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, Shelly. r=me with the comment updated. ::: content/media/encoder/OpusTrackEncoder.cpp @@ +116,5 @@ > + > +static bool IsOpusSupportedSamplingRate(int aSamplingRate) > +{ > + for (int32_t i = 0; gOpusSupportedInputSamplingRate[i]; ++i) { > + if (gOpusSupportedInputSamplingRate[i] == aSamplingRate) { BTW, you can use nsTArray::Contains for this sort of linear search, which might make this easier to read. or std::set, but that's not much better. @@ +160,5 @@ > return NS_ERROR_FAILURE; > } > // The granule position is required to be incremented at a rate of 48KHz, and > // it is simply calculated as |granulepos = samples * (48000/source_rate)|, > // that is, the source sampling rate must divide 48000 evenly. Please update the comment as well. It's true that the sample rate needs to divide 48kHz evenly so we can calculate our granule position, but the real condition for resampling is that the input is not one of the supported sample rates.

Although it's okay to mark r=you, but I'd like to have you take a second look since there's a lot changes from the last version :). Thanks!

Try result: https://tbpl.mozilla.org/?tree=Try&rev=e5ffa761630b

Comment on attachment 8344553 [details] [diff] [review] Correct the checking of input sampling rate, v2 Review of attachment 8344553 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/encoder/OpusTrackEncoder.cpp @@ +33,5 @@ > static const int kFrameDurationMs = 20; > > +// The supported sampling rate of input signal (Hz), > +// must be one of the following. Will resampled to 48kHz otherwise. > +static const int kOpusSupportedInputSamplingRate[5] = please make this plural as well. @@ +156,5 @@ > + // sampling rate of source signal be one of 8000, 12000, 16000, 24000, or > + // 48000. If this constraint is not satisfied, we resample the input to 48kHz. > + nsTArray<int> supportedSamplingRates; > + supportedSamplingRates.AppendElements(kOpusSupportedInputSamplingRate, > + sizeof(kOpusSupportedInputSamplingRate)/sizeof(int)); ArrayLength(kOpusSuportedInputSamplingRates) from "mozilla/ArrayUtils.h" so you don't have to duplicate the type name. @@ +157,5 @@ > + // 48000. If this constraint is not satisfied, we resample the input to 48kHz. > + nsTArray<int> supportedSamplingRates; > + supportedSamplingRates.AppendElements(kOpusSupportedInputSamplingRate, > + sizeof(kOpusSupportedInputSamplingRate)/sizeof(int)); > + if (!supportedSamplingRates.Contains(aSamplingRate)) { Thanks. I think this is simpler.

Carry r+ from Ralph. Thank you:)