Add opus audio codec support in remoting

Add opus audio codec support in remoting

BUG=154714
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=163675

[Sergey Ulanov, 2012-10-17 23:23:10]

Dale - please review media changes.
Wez - everything else.

I'm still missing unittests for this code. Will add them in this CL today or
tomorrow. It shouldn't affect rest of the code.

[DaleCurtis, 2012-10-18 00:45:01]

Haven't had a chance to look at your usage case yet.

http://codereview.chromium.org/11189047/diff/2001/media/base/sinc_resampler.h
File media/base/sinc_resampler.h (right):

http://codereview.chromium.org/11189047/diff/2001/media/base/sinc_resampler.h...
media/base/sinc_resampler.h:21: static const int kLookAheadSize;
Hmm, this is a misleading comment / name, the priming step will ask for
"kBlockSize + kKernelSize / 2" while the refresh step will ask for "kBlockSize".
Maybe MaximumLookAheadSize.

Also shouldn't this be an extern? I forget the rules for this.

[Wez, 2012-10-19 01:51:32]

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_decode...
File remoting/codec/audio_decoder_opus.cc (right):

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_decode...
remoting/codec/audio_decoder_opus.cc:30: AudioDecoderOpus::~AudioDecoderOpus() {
Call DestroyDecoder() here?

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_decode...
remoting/codec/audio_decoder_opus.cc:37: CHECK(decoder_);
Don't CHECK on error; just LOG(ERROR) if an error is returned, since the
ResetForPacket() check will catch the case where there's no decoder.

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_decode...
remoting/codec/audio_decoder_opus.cc:75: << "when an OPUS packet was expected.";
nit: This should be trapped by the calling code and CHECK()d here.

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_decode...
remoting/codec/audio_decoder_opus.cc:103:
static_cast<int>(decoded_data->size()));
nit: DCHECK_LE

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_decode...
remoting/codec/audio_decoder_opus.cc:110: LOG(ERROR) << "Failed decoding Opus
frame. Error code: " << result;
nit: Reset the decoder in this case, and return a NULL packet?

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_decode...
remoting/codec/audio_decoder_opus.cc:110: LOG(ERROR) << "Failed decoding Opus
frame. Error code: " << result;
nit: Consider using continue here rather than if...else, since this is the
exceptional error-handling case.

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_decode...
remoting/codec/audio_decoder_opus.cc:119: if (!decoded_data->size()) {
nit: if (decoded_data->empty())

OR

Move this test above the resize() call and test |buffer_pos| instead.

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_decode...
File remoting/codec/audio_decoder_opus.h (right):

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_decode...
remoting/codec/audio_decoder_opus.h:16: class AudioPacket;
I think this is in the protocol namespace?

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_encode...
File remoting/codec/audio_encoder_opus.cc (right):

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_encode...
remoting/codec/audio_encoder_opus.cc:18: // Use 160 kb/s bitrate.
nit: When you say "use", do you mean "output"?

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_encode...
remoting/codec/audio_encoder_opus.cc:18: // Use 160 kb/s bitrate.
You mean kilobits/second?

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_encode...
remoting/codec/audio_encoder_opus.cc:19: const int kBitrateBps = 160 * 1024;
nit: kOutputBps or kOutputBitRateBps

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_encode...
remoting/codec/audio_encoder_opus.cc:67: CHECK(!error);
Don't CHECK on error; just cope w/ |encoder_| being NULL elsewhere, e.g. by
early-exiting here.

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_encode...
remoting/codec/audio_encoder_opus.cc:78: resampler_.reset(new
media::MultiChannelResampler(
nit: Indentation.

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_encode...
remoting/codec/audio_encoder_opus.cc:90: 
nit: Lose blank line.

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_encode...
remoting/codec/audio_encoder_opus.cc:119: DCHECK(encoder_);
nit: Remove DCHECK

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_encode...
remoting/codec/audio_encoder_opus.cc:125: void
AudioEncoderOpus::ResamplerRead(media::AudioBus* audio_bus) {
This needs a better name, e.g. SupplyResampler / FeedResampler /
FetchBytesToResample

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_encode...
remoting/codec/audio_encoder_opus.cc:126: CHECK(resampling_data_);
nit: DCHECK, and add a line after the check

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_encode...
remoting/codec/audio_encoder_opus.cc:129: CHECK_LE(audio_bus->frames(),
samples_left);
nit: DCHECK_LE and add a line after the check

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_encode...
remoting/codec/audio_encoder_opus.cc:148: int samples_left =
packet->data(0).size() / kBytesPerSample / channels_;
nit: samples_in_packet?

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_encode...
remoting/codec/audio_encoder_opus.cc:158: int preferch_samples =
typo: prefetch_samples

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_encode...
remoting/codec/audio_encoder_opus.cc:162: while (leftover_samples_ +
samples_left >= samples_wanted) {
This loop could use a liberal sprinkling of concise comments to explain what the
various steps are.

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_encode...
remoting/codec/audio_encoder_opus.cc:181: media::AudioBus::Create(channels_,
kFrameSamples);
Do we really want to create a new AudioBus for each Resample() call?

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_encode...
remoting/codec/audio_encoder_opus.cc:207: CHECK_LE(result,
static_cast<int>(data->length()));
DCHECK_LE?

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_encode...
File remoting/codec/audio_encoder_opus.h (right):

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_encode...
remoting/codec/audio_encoder_opus.h:53: scoped_array<int16> leftover_buffer_;
nit: Rather than a separate |leftover_buffer_|, why not store the partially-read
AudioPacket here, with an indication of how far into it we've read so far, and
avoid the extra copying?

[Sergey Ulanov, 2012-10-19 20:54:29]

Addressed all comments and added unittests.

http://codereview.chromium.org/11189047/diff/2001/media/base/sinc_resampler.h
File media/base/sinc_resampler.h (right):

http://codereview.chromium.org/11189047/diff/2001/media/base/sinc_resampler.h...
media/base/sinc_resampler.h:21: static const int kLookAheadSize;
On 2012/10/18 00:45:01, DaleCurtis wrote:
> Hmm, this is a misleading comment / name, the priming step will ask for
> "kBlockSize + kKernelSize / 2" while the refresh step will ask for
"kBlockSize".
> Maybe MaximumLookAheadSize.

Done.

> 
> Also shouldn't this be an extern? I forget the rules for this.

No, because it's a class member.

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_decode...
File remoting/codec/audio_decoder_opus.cc (right):

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_decode...
remoting/codec/audio_decoder_opus.cc:30: AudioDecoderOpus::~AudioDecoderOpus() {
On 2012/10/19 01:51:32, Wez wrote:
> Call DestroyDecoder() here?

Done.

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_decode...
remoting/codec/audio_decoder_opus.cc:37: CHECK(decoder_);
On 2012/10/19 01:51:32, Wez wrote:
> Don't CHECK on error; just LOG(ERROR) if an error is returned, since the
> ResetForPacket() check will catch the case where there's no decoder.

Done.

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_decode...
remoting/codec/audio_decoder_opus.cc:75: << "when an OPUS packet was expected.";
On 2012/10/19 01:51:32, Wez wrote:
> nit: This should be trapped by the calling code and CHECK()d here.

Actually calling code doesn't have any knowledge of the audio codec and never
looks inside audio packets. It creates decoder based on the codec that was
negotiated over the signaling channel, and then just passes received audio
packets to the decoder.

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_decode...
remoting/codec/audio_decoder_opus.cc:103:
static_cast<int>(decoded_data->size()));
On 2012/10/19 01:51:32, Wez wrote:
> nit: DCHECK_LE

CHECK is better here to avoid potential buffer overflow (e.g. if there is a bug
in opus_decoder()).

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_decode...
remoting/codec/audio_decoder_opus.cc:110: LOG(ERROR) << "Failed decoding Opus
frame. Error code: " << result;
On 2012/10/19 01:51:32, Wez wrote:
> nit: Reset the decoder in this case, and return a NULL packet?

Done.

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_decode...
remoting/codec/audio_decoder_opus.cc:110: LOG(ERROR) << "Failed decoding Opus
frame. Error code: " << result;
On 2012/10/19 01:51:32, Wez wrote:
> nit: Consider using continue here rather than if...else, since this is the
> exceptional error-handling case.

Done.

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_decode...
remoting/codec/audio_decoder_opus.cc:119: if (!decoded_data->size()) {
On 2012/10/19 01:51:32, Wez wrote:
> nit: if (decoded_data->empty())
> 
> OR
> 
> Move this test above the resize() call and test |buffer_pos| instead.

Done.

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_decode...
File remoting/codec/audio_decoder_opus.h (right):

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_decode...
remoting/codec/audio_decoder_opus.h:16: class AudioPacket;
On 2012/10/19 01:51:32, Wez wrote:
> I think this is in the protocol namespace?

Nope. All protobuf types are in remoting NS.

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_encode...
File remoting/codec/audio_encoder_opus.cc (right):

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_encode...
remoting/codec/audio_encoder_opus.cc:18: // Use 160 kb/s bitrate.
On 2012/10/19 01:51:32, Wez wrote:
> You mean kilobits/second?

Yes, It would be kB/s for kilobytes :)

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_encode...
remoting/codec/audio_encoder_opus.cc:18: // Use 160 kb/s bitrate.
On 2012/10/19 01:51:32, Wez wrote:
> nit: When you say "use", do you mean "output"?

Done.

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_encode...
remoting/codec/audio_encoder_opus.cc:19: const int kBitrateBps = 160 * 1024;
On 2012/10/19 01:51:32, Wez wrote:
> nit: kOutputBps or kOutputBitRateBps

Done.

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_encode...
remoting/codec/audio_encoder_opus.cc:67: CHECK(!error);
On 2012/10/19 01:51:32, Wez wrote:
> Don't CHECK on error; just cope w/ |encoder_| being NULL elsewhere, e.g. by
> early-exiting here.

Done.

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_encode...
remoting/codec/audio_encoder_opus.cc:78: resampler_.reset(new
media::MultiChannelResampler(
On 2012/10/19 01:51:32, Wez wrote:
> nit: Indentation.

Done.

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_encode...
remoting/codec/audio_encoder_opus.cc:90: 
On 2012/10/19 01:51:32, Wez wrote:
> nit: Lose blank line.

Done.

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_encode...
remoting/codec/audio_encoder_opus.cc:119: DCHECK(encoder_);
On 2012/10/19 01:51:32, Wez wrote:
> nit: Remove DCHECK

Done.

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_encode...
remoting/codec/audio_encoder_opus.cc:125: void
AudioEncoderOpus::ResamplerRead(media::AudioBus* audio_bus) {
On 2012/10/19 01:51:32, Wez wrote:
> This needs a better name, e.g. SupplyResampler / FeedResampler /
> FetchBytesToResample

Done.

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_encode...
remoting/codec/audio_encoder_opus.cc:126: CHECK(resampling_data_);
On 2012/10/19 01:51:32, Wez wrote:
> nit: DCHECK, and add a line after the check

Done.

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_encode...
remoting/codec/audio_encoder_opus.cc:129: CHECK_LE(audio_bus->frames(),
samples_left);
On 2012/10/19 01:51:32, Wez wrote:
> nit: DCHECK_LE and add a line after the check

Done.

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_encode...
remoting/codec/audio_encoder_opus.cc:148: int samples_left =
packet->data(0).size() / kBytesPerSample / channels_;
On 2012/10/19 01:51:32, Wez wrote:
> nit: samples_in_packet?

Done.

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_encode...
remoting/codec/audio_encoder_opus.cc:158: int preferch_samples =
On 2012/10/19 01:51:32, Wez wrote:
> typo: prefetch_samples

Done.

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_encode...
remoting/codec/audio_encoder_opus.cc:162: while (leftover_samples_ +
samples_left >= samples_wanted) {
On 2012/10/19 01:51:32, Wez wrote:
> This loop could use a liberal sprinkling of concise comments to explain what
the
> various steps are.

Done.

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_encode...
remoting/codec/audio_encoder_opus.cc:181: media::AudioBus::Create(channels_,
kFrameSamples);
On 2012/10/19 01:51:32, Wez wrote:
> Do we really want to create a new AudioBus for each Resample() call?

Done.

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_encode...
remoting/codec/audio_encoder_opus.cc:207: CHECK_LE(result,
static_cast<int>(data->length()));
On 2012/10/19 01:51:32, Wez wrote:
> DCHECK_LE?

Done.

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_encode...
File remoting/codec/audio_encoder_opus.h (right):

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_encode...
remoting/codec/audio_encoder_opus.h:53: scoped_array<int16> leftover_buffer_;
On 2012/10/19 01:51:32, Wez wrote:
> nit: Rather than a separate |leftover_buffer_|, why not store the
partially-read
> AudioPacket here, with an indication of how far into it we've read so far, and
> avoid the extra copying?

We need to handle the case when we receive a sequence of small packets, so
storing just one AudioPacket may not be enough and we will need to copy some
data anyway to handle this case. Having just one buffer simplifies
implementation.

[Sergey Ulanov, 2012-10-22 17:53:17]

wez: ping

[DaleCurtis, 2012-10-22 20:48:41]

SincResampler stuff lgtm % nit.

http://codereview.chromium.org/11189047/diff/15007/media/base/sinc_resampler.h
File media/base/sinc_resampler.h (right):

http://codereview.chromium.org/11189047/diff/15007/media/base/sinc_resampler....
media/base/sinc_resampler.h:19: // How many samples may be requested from the
callback ahead of the current
Comment still makes it sound like this is the only amount that may be requested.
s/How many samples/The maximum number of samples that/

[Sergey Ulanov, 2012-10-22 22:07:23]

http://codereview.chromium.org/11189047/diff/15007/media/base/sinc_resampler.h
File media/base/sinc_resampler.h (right):

http://codereview.chromium.org/11189047/diff/15007/media/base/sinc_resampler....
media/base/sinc_resampler.h:19: // How many samples may be requested from the
callback ahead of the current
On 2012/10/22 20:48:41, DaleCurtis wrote:
> Comment still makes it sound like this is the only amount that may be
requested.
> s/How many samples/The maximum number of samples that/

Done.

[Wez, 2012-10-22 22:50:21]

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_decode...
File remoting/codec/audio_decoder_opus.cc (right):

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_decode...
remoting/codec/audio_decoder_opus.cc:103:
static_cast<int>(decoded_data->size()));
On 2012/10/19 20:54:30, sergeyu wrote:
> On 2012/10/19 01:51:32, Wez wrote:
> > nit: DCHECK_LE
> 
> CHECK is better here to avoid potential buffer overflow (e.g. if there is a
bug
> in opus_decoder()).

If opus_decode() returns more than it should, though, then it'll likely have
overrun the buffer already, before we reach here?

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_decode...
File remoting/codec/audio_decoder_opus.h (right):

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_decode...
remoting/codec/audio_decoder_opus.h:16: class AudioPacket;
On 2012/10/19 20:54:30, sergeyu wrote:
> On 2012/10/19 01:51:32, Wez wrote:
> > I think this is in the protocol namespace?
> 
> Nope. All protobuf types are in remoting NS.

Weird... MouseEvent certainly isn't...

http://codereview.chromium.org/11189047/diff/15007/remoting/codec/audio_encod...
File remoting/codec/audio_encoder_opus.cc (right):

http://codereview.chromium.org/11189047/diff/15007/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus.cc:80: resampler_.reset(new
media::MultiChannelResampler(
This appears in codereview still over-indented; not sure whether it's actually
indented by four bytes in the patch?

http://codereview.chromium.org/11189047/diff/15007/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus.cc:127: void
AudioEncoderOpus::FetchBytesToResampler(media::AudioBus* audio_bus) {
nit: FetchBytesToResample or PumpBytesToResampler

http://codereview.chromium.org/11189047/diff/15007/remoting/codec/audio_encod...
File remoting/codec/audio_encoder_opus_unittest.cc (right):

http://codereview.chromium.org/11189047/diff/15007/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:20: const double
kChannelPhaseShift = 2 * M_PI / 3;
nit: Add comments explaining what this is used for.

http://codereview.chromium.org/11189047/diff/15007/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:36: double frequency,
nit: Specify units.

http://codereview.chromium.org/11189047/diff/15007/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:37: double pos,
nit: How can we have sub-sample position?

http://codereview.chromium.org/11189047/diff/15007/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:50: std::vector<int16>
data(samples * kChannels);
nit: Add a comment to explain that we're filling the channels with sine wave
sample data with different phase-shifts.

http://codereview.chromium.org/11189047/diff/15007/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:66: // Decoded data is normally
shifted in phase relative to the raw date. This
typo: date -> data

http://codereview.chromium.org/11189047/diff/15007/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:67: // function returns
approximate the phase difference by finding the first
typo: approximate the phase ... -> ... the approximate phase ...

http://codereview.chromium.org/11189047/diff/15007/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:69: double GetSignalShift(const
std::vector<int16>& received_data) {
nit: So this function is supposed to be getting the phase shift using a
heuristic -> call it GetPhaseShift or EstimatePhaseShift?

http://codereview.chromium.org/11189047/diff/15007/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:85: const std::vector<int16>&
received_data) {
nit: Add a comment explaining that we validate the data by estimating the source
phase shift, generating data with that shift and calculating the difference
between that and the decoded version.

http://codereview.chromium.org/11189047/diff/15007/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:97: double averate_difference =
sqrt(diff_sqare_sum / received_data.size());
typo: average_difference

http://codereview.chromium.org/11189047/diff/15007/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:97: double averate_difference =
sqrt(diff_sqare_sum / received_data.size());
nit: Clarify the units you're using for the difference, to explain the magic
value of 5000

http://codereview.chromium.org/11189047/diff/15007/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:98: EXPECT_LE(averate_difference,
5000);
nit: Consider moving 5000 to a constant, above

http://codereview.chromium.org/11189047/diff/15007/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:101: void ResetEncoder() {
nit: ResetEncoder -> Reset or ResetEncoderAndDecoder

http://codereview.chromium.org/11189047/diff/15007/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:106: void TestEncoderDecoder(int
packet_size,
typo: TestEncodeDecode

http://codereview.chromium.org/11189047/diff/15007/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:117: scoped_ptr<AudioPacket>
encoded = encoder_->Encode(source_packet.Pass());
Line too long?

http://codereview.chromium.org/11189047/diff/15007/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:118: if (encoded.get()) {
EXPECT(encoded.get())?

http://codereview.chromium.org/11189047/diff/15007/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:160: TEST_F(OpusAudioEncoderTest,
BufferSize) {
nit: BufferingAndResampling / BufferSizeAndResampling?

http://codereview.chromium.org/11189047/diff/15007/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:163: TestEncoderDecoder(5000,
3000, AudioPacket::SAMPLING_RATE_44100);
How long to these tests take?

[Sergey Ulanov, 2012-10-23 00:43:49]

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_decode...
File remoting/codec/audio_decoder_opus.cc (right):

http://codereview.chromium.org/11189047/diff/2001/remoting/codec/audio_decode...
remoting/codec/audio_decoder_opus.cc:103:
static_cast<int>(decoded_data->size()));
On 2012/10/22 22:50:21, Wez wrote:
> On 2012/10/19 20:54:30, sergeyu wrote:
> > On 2012/10/19 01:51:32, Wez wrote:
> > > nit: DCHECK_LE
> > 
> > CHECK is better here to avoid potential buffer overflow (e.g. if there is a
> bug
> > in opus_decoder()).
> 
> If opus_decode() returns more than it should, though, then it'll likely have
> overrun the buffer already, before we reach here?

Right, but this CHECK still makes it harder to exploit such vulnerability if it
exists. From chromium style guide: 
  Use CHECK() if the consequence of a failed assertion would be a security
vulnerability, where crashing the browser is preferable.

http://codereview.chromium.org/11189047/diff/15007/remoting/codec/audio_encod...
File remoting/codec/audio_encoder_opus.cc (right):

http://codereview.chromium.org/11189047/diff/15007/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus.cc:80: resampler_.reset(new
media::MultiChannelResampler(
On 2012/10/22 22:50:21, Wez wrote:
> This appears in codereview still over-indented; not sure whether it's actually
> indented by four bytes in the patch?

Looks correctly to me both in rietveld and in my editor (2 spaces relative to
the if 3 lines above).

http://codereview.chromium.org/11189047/diff/15007/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus.cc:127: void
AudioEncoderOpus::FetchBytesToResampler(media::AudioBus* audio_bus) {
On 2012/10/22 22:50:21, Wez wrote:
> nit: FetchBytesToResample or PumpBytesToResampler

Done.

http://codereview.chromium.org/11189047/diff/15007/remoting/codec/audio_encod...
File remoting/codec/audio_encoder_opus_unittest.cc (right):

http://codereview.chromium.org/11189047/diff/15007/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:20: const double
kChannelPhaseShift = 2 * M_PI / 3;
On 2012/10/22 22:50:21, Wez wrote:
> nit: Add comments explaining what this is used for.

Done.

http://codereview.chromium.org/11189047/diff/15007/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:36: double frequency,
On 2012/10/22 22:50:21, Wez wrote:
> nit: Specify units.

Done.

http://codereview.chromium.org/11189047/diff/15007/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:37: double pos,
On 2012/10/22 22:50:21, Wez wrote:
> nit: How can we have sub-sample position?

That's used when comparing results - GetSignalShift() returns sub-sample
position.

http://codereview.chromium.org/11189047/diff/15007/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:50: std::vector<int16>
data(samples * kChannels);
On 2012/10/22 22:50:21, Wez wrote:
> nit: Add a comment to explain that we're filling the channels with sine wave
> sample data with different phase-shifts.

Done.

http://codereview.chromium.org/11189047/diff/15007/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:66: // Decoded data is normally
shifted in phase relative to the raw date. This
On 2012/10/22 22:50:21, Wez wrote:
> typo: date -> data

Done.

http://codereview.chromium.org/11189047/diff/15007/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:67: // function returns
approximate the phase difference by finding the first
On 2012/10/22 22:50:21, Wez wrote:
> typo: approximate the phase ... -> ... the approximate phase ...

Done.

http://codereview.chromium.org/11189047/diff/15007/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:69: double GetSignalShift(const
std::vector<int16>& received_data) {
On 2012/10/22 22:50:21, Wez wrote:
> nit: So this function is supposed to be getting the phase shift using a
> heuristic -> call it GetPhaseShift or EstimatePhaseShift?

Renamed to EstimateSignalShift(). I don't think it's correct to call it phase
shift because it's measured in samples instead of radians (updated comments to
make it clear).

http://codereview.chromium.org/11189047/diff/15007/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:85: const std::vector<int16>&
received_data) {
On 2012/10/22 22:50:21, Wez wrote:
> nit: Add a comment explaining that we validate the data by estimating the
source
> phase shift, generating data with that shift and calculating the difference
> between that and the decoded version.

Done.

http://codereview.chromium.org/11189047/diff/15007/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:97: double averate_difference =
sqrt(diff_sqare_sum / received_data.size());
On 2012/10/22 22:50:21, Wez wrote:
> typo: average_difference

Done.

http://codereview.chromium.org/11189047/diff/15007/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:97: double averate_difference =
sqrt(diff_sqare_sum / received_data.size());
On 2012/10/22 22:50:21, Wez wrote:
> nit: Clarify the units you're using for the difference, to explain the magic
> value of 5000

Done.

http://codereview.chromium.org/11189047/diff/15007/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:98: EXPECT_LE(averate_difference,
5000);
On 2012/10/22 22:50:21, Wez wrote:
> nit: Consider moving 5000 to a constant, above

Done.

http://codereview.chromium.org/11189047/diff/15007/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:101: void ResetEncoder() {
On 2012/10/22 22:50:21, Wez wrote:
> nit: ResetEncoder -> Reset or ResetEncoderAndDecoder
Removed this function.

http://codereview.chromium.org/11189047/diff/15007/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:106: void TestEncoderDecoder(int
packet_size,
On 2012/10/22 22:50:21, Wez wrote:
> typo: TestEncodeDecode

Done.

http://codereview.chromium.org/11189047/diff/15007/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:117: scoped_ptr<AudioPacket>
encoded = encoder_->Encode(source_packet.Pass());
On 2012/10/22 22:50:21, Wez wrote:
> Line too long?

Done.

http://codereview.chromium.org/11189047/diff/15007/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:118: if (encoded.get()) {
On 2012/10/22 22:50:21, Wez wrote:
> EXPECT(encoded.get())?

Encoder is allowed to return NULL packet sometimes, particularly when original
raw packet is smaller than the encoder frame size.

http://codereview.chromium.org/11189047/diff/15007/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:160: TEST_F(OpusAudioEncoderTest,
BufferSize) {
On 2012/10/22 22:50:21, Wez wrote:
> nit: BufferingAndResampling / BufferSizeAndResampling?

Done.

http://codereview.chromium.org/11189047/diff/15007/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:163: TestEncoderDecoder(5000,
3000, AudioPacket::SAMPLING_RATE_44100);
On 2012/10/22 22:50:21, Wez wrote:
> How long to these tests take?

About 0.6 seconds total for all tests in this file with Debug build, i.e. about
60ms each small test.

[Wez, 2012-10-23 05:10:09]

lgtm

http://codereview.chromium.org/11189047/diff/29019/remoting/codec/audio_encod...
File remoting/codec/audio_encoder_opus_unittest.cc (right):

http://codereview.chromium.org/11189047/diff/29019/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:42: // The value is choose such
that all the tests pass normally, but fail with
typo: choose -> chosen

http://codereview.chromium.org/11189047/diff/29019/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:43: // small small change (e.g.
one sample shift between signals).
typo: small small change -> small changes?

http://codereview.chromium.org/11189047/diff/29019/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:87: // Decoded data is normally
shifted in phase relative to the raw data. This
nit: by raw data do you mean the original data?

http://codereview.chromium.org/11189047/diff/29019/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:103: // Compares decoded signal
with the test signal that was decoded. It estimates
typo: test signal that was encoded

http://codereview.chromium.org/11189047/diff/29019/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:105: // of the difference between
original and decoded signal.
typo: signal -> signals

[Sergey Ulanov, 2012-10-23 17:36:31]

http://codereview.chromium.org/11189047/diff/29019/remoting/codec/audio_encod...
File remoting/codec/audio_encoder_opus_unittest.cc (right):

http://codereview.chromium.org/11189047/diff/29019/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:42: // The value is choose such
that all the tests pass normally, but fail with
On 2012/10/23 05:10:10, Wez wrote:
> typo: choose -> chosen

Done.

http://codereview.chromium.org/11189047/diff/29019/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:43: // small small change (e.g.
one sample shift between signals).
On 2012/10/23 05:10:10, Wez wrote:
> typo: small small change -> small changes?

Done.

http://codereview.chromium.org/11189047/diff/29019/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:87: // Decoded data is normally
shifted in phase relative to the raw data. This
On 2012/10/23 05:10:10, Wez wrote:
> nit: by raw data do you mean the original data?

Done.

http://codereview.chromium.org/11189047/diff/29019/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:103: // Compares decoded signal
with the test signal that was decoded. It estimates
On 2012/10/23 05:10:10, Wez wrote:
> typo: test signal that was encoded

Done.

http://codereview.chromium.org/11189047/diff/29019/remoting/codec/audio_encod...
remoting/codec/audio_encoder_opus_unittest.cc:105: // of the difference between
original and decoded signal.
On 2012/10/23 05:10:10, Wez wrote:
> typo: signal -> signals

Done.

[commit-bot: I haz the power, 2012-10-23 17:38:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/11189047/21022

[commit-bot: I haz the power, 2012-10-23 19:55:43]

Change committed as 163650