Add opus audio codec support in remoting

remoting/codec/audio_decoder_opus.cc

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "remoting/codec/audio_decoder_opus.h"
+
+#include "base/logging.h"
+#include "base/stl_util.h"
+#include "base/time.h"
+#include "remoting/proto/audio.pb.h"
+#include "third_party/opus/src/include/opus.h"
+
+namespace remoting {
+
+namespace {
+
+const int kMaxPacketSizeMs = 120;
+
+const AudioPacket::SamplingRate kSamplingRate =
+    AudioPacket::SAMPLING_RATE_48000;
+
+}  // namespace
+
+AudioDecoderOpus::AudioDecoderOpus()
+    : sampling_rate_(0),
+      channels_(0),
+      decoder_(NULL) {
+}
+
+AudioDecoderOpus::~AudioDecoderOpus() {

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

Call DestroyDecoder() here?

[Sergey Ulanov, 2012/10/19 20:54:30]

Done.

+}
+
+void AudioDecoderOpus::InitDecoder() {
+  DCHECK(!decoder_);
+  int error;
+  decoder_ = opus_decoder_create(kSamplingRate, channels_, &error);
+  CHECK(decoder_);

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

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.

[Sergey Ulanov, 2012/10/19 20:54:30]

Done.

+  CHECK(!error);
+}
+
+void AudioDecoderOpus::DestroyDecoder() {
+  if (decoder_) {
+    opus_decoder_destroy(decoder_);
+    decoder_ = NULL;
+  }
+}
+
+bool AudioDecoderOpus::ResetForPacket(AudioPacket* packet) {
+  if (packet->channels() != channels_ ||
+      packet->sampling_rate() != sampling_rate_) {
+    DestroyDecoder();
+
+    channels_ = packet->channels();
+    sampling_rate_ = packet->sampling_rate();
+
+    if (channels_ <= 0 || channels_ > 2 ||
+        sampling_rate_ != kSamplingRate) {
+      LOG(WARNING) << "Unsupported OPUS parameters: "
+                   << channels_ << " channels with "
+                   << sampling_rate_ << " samples per second.";
+      return false;
+    }
+
+    InitDecoder();
+  }
+
+  return decoder_ != NULL;
+}
+
+
+scoped_ptr<AudioPacket> AudioDecoderOpus::Decode(
+    scoped_ptr<AudioPacket> packet) {
+  if (packet->encoding() != AudioPacket::ENCODING_OPUS) {
+    LOG(WARNING) << "Received a packet with encoding " << packet->encoding()
+                 << "when an OPUS packet was expected.";

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

nit: This should be trapped by the calling code and CHECK()d here.

[Sergey Ulanov, 2012/10/19 20:54:30]

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.

+    return scoped_ptr<AudioPacket>();
+  }
+
+  if (!ResetForPacket(packet.get())) {
+    return scoped_ptr<AudioPacket>();
+  }
+
+  // Create a new packet of decoded data.
+  scoped_ptr<AudioPacket> decoded_packet(new AudioPacket());
+  decoded_packet->set_encoding(AudioPacket::ENCODING_RAW);
+  decoded_packet->set_sampling_rate(kSamplingRate);
+  decoded_packet->set_bytes_per_sample(AudioPacket::BYTES_PER_SAMPLE_2);
+  decoded_packet->set_channels(packet->channels());
+
+  int max_frame_samples = kMaxPacketSizeMs * kSamplingRate /
+      base::Time::kMillisecondsPerSecond;
+  int max_frame_bytes = max_frame_samples * channels_ *
+      decoded_packet->bytes_per_sample();
+
+  std::string* decoded_data = decoded_packet->add_data();
+  decoded_data->resize(packet->data_size() * max_frame_bytes);
+  int buffer_pos = 0;
+
+  for (int i = 0; i < packet->data_size(); ++i) {
+    int16* pcm_buffer =
+        reinterpret_cast<int16*>(string_as_array(decoded_data) + buffer_pos);
+    CHECK_LE(buffer_pos + max_frame_bytes,
+             static_cast<int>(decoded_data->size()));

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

nit: DCHECK_LE

[Sergey Ulanov, 2012/10/19 20:54:30]

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

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

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

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

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.

+    std::string* frame = packet->mutable_data(i);
+    unsigned char* frame_data =
+        reinterpret_cast<unsigned char*>(string_as_array(frame));
+    int result = opus_decode(decoder_, frame_data, frame->size(),
+                             pcm_buffer, max_frame_samples, 0);
+    if (result < 0) {
+      LOG(ERROR) << "Failed decoding Opus frame. Error code: " << result;

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

nit: Reset the decoder in this case, and return a NULL packet?

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

nit: Consider using continue here rather than if...else, since this is the
exceptional error-handling case.

[Sergey Ulanov, 2012/10/19 20:54:30]

Done.

[Sergey Ulanov, 2012/10/19 20:54:30]

Done.

+    } else {
+      buffer_pos += result * packet->channels() *
+         decoded_packet->bytes_per_sample();
+    }
+  }
+
+  decoded_data->resize(buffer_pos);
+
+  if (!decoded_data->size()) {

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

nit: if (decoded_data->empty())

OR

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

[Sergey Ulanov, 2012/10/19 20:54:30]

Done.

+    return scoped_ptr<AudioPacket>();
+  }
+
+  return decoded_packet.Pass();
+}
+
+}  // namespace remoting