Add AudioTrackRecorder for audio component of MediaStream recording.

content/renderer/media/audio_track_recorder.cc

+// Copyright 2015 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 "base/bind.h"
+#include "base/stl_util.h"
+#include "base/threading/thread.h"
+#include "content/renderer/media/audio_track_recorder.h"
+
+#include "third_party/opus/src/include/opus.h"
+
+namespace content {
+
+namespace {
+
+// TODO(ajose): This likely shouldn't be hardcoded.
+const int kDefaultFramesPerSecond = 100;

[miu, 2015/10/21 20:35:38]

Consider using enums for these constants rather than global ints (per recent
discussion about a month ago on chromium-dev@):

  enum {
    DEFAULT_FRAMES_PER_SECOND = 100,
    OPUS_MAX_PAYLOAD_SIZE = 4000
  };

[ajose, 2015/10/23 22:46:33]

Done.

+// This is the recommended value, according to documentation in
+// third_party/opus/src/include/opus.h, so that the Opus encoder does not
+// degrade the audio due to memory constraints.
+//
+// Note: Whereas other RTP implementations do not, the cast library is

[miu, 2015/10/21 20:35:38]

This comment seems....out of place?  ;-)

[ajose, 2015/10/23 22:46:33]

Whoops...

+// perfectly capable of transporting larger than MTU-sized audio frames.
+static const int kOpusMaxPayloadSize = 4000;
+
+// TODO(ajose): Removed code that might be useful for ensuring A/V sync.
+// See cast/sender/AudioEncoder.
+
+}  // anonymous namespace
+
+class AudioTrackRecorder::AudioEncoder

[miu, 2015/10/21 20:35:38]

General statement about the entire AudioEncoder class: Can you make it so most
(or all) of the code is being run on the same thread (i.e., an encoder thread
owned by AudioTrackRecorder)?  This would make creation/destruction much cleaner
and less error-prone.

[ajose, 2015/10/23 22:46:33]

Done.

+    : public base::RefCountedThreadSafe<AudioEncoder> {
+ public:
+  AudioEncoder(const OnEncodedAudioCB& on_encoded_audio_cb)

[miu, 2015/10/21 20:35:38]

Need explicit keyword here.

[ajose, 2015/10/23 22:46:33]

Done.

+      : on_encoded_audio_cb_(on_encoded_audio_cb),

[mcasas, 2015/10/21 19:19:51]

Don't inline complex ctor.

[miu, 2015/10/21 20:35:38]

It doesn't matter for classes private to .cc modules.  ;-)

[ajose, 2015/10/23 22:46:33]

Acknowledged.

[ajose, 2015/10/23 22:46:33]

Done.

+        main_task_runner_(base::MessageLoop::current()->task_runner()) {}
+
+  void OnSetFormat(const media::AudioParameters& params) { InitOpus(params); }
+
+  void EncodeAudio(const media::AudioBus& audio_bus,
+                   const base::TimeTicks& recorded_time);
+
+  bool IsInitialized() { return audio_params_.get() != nullptr; }
+
+ private:
+  friend class base::RefCountedThreadSafe<AudioEncoder>;
+
+  ~AudioEncoder();
+
+  void InitOpus(const media::AudioParameters& params);
+
+  void TransferSamplesIntoBuffer(const media::AudioBus& audio_bus,
+                                 int source_offset,
+                                 int buffer_fill_offset,
+                                 int num_samples);
+  bool EncodeFromFilledBuffer(std::string* out);
+
+  static bool IsValidFrameDuration(base::TimeDelta duration);
+
+  int samples_per_frame_;
+  const OnEncodedAudioCB on_encoded_audio_cb_;
+
+  // In the case where a call to EncodeAudio() cannot completely fill the
+  // buffer, this points to the position at which to populate data in a later
+  // call.
+  int buffer_fill_end_;
+
+  // Used to shutdown properly on the same thread we were created on.
+  const scoped_refptr<base::SingleThreadTaskRunner> main_task_runner_;

[mcasas, 2015/10/21 19:19:52]

I think this fellow is not used now, perhaps you wanted to 
use it for DCHECK()ing? If the latter is the case, you can
sandwich it in an #if DCHECK_IS_ON().

[ajose, 2015/10/23 22:46:33]

ThreadChecker already does something similar to this [1]. Will change if you'd
prefer it.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/thr...

+
+  // Task runner where frames to encode and reply callbacks must happen.
+  scoped_refptr<base::SingleThreadTaskRunner> origin_task_runner_;
+
+  scoped_ptr<media::AudioParameters> audio_params_;

[miu, 2015/10/21 20:35:38]

No need for separate heap allocation here.  Just:

  media::AudioParams params_;

[ajose, 2015/10/23 22:46:33]

Done.

+
+  // Buffer for passing AudioBus data to OpusEncoder.
+  scoped_ptr<float[]> buffer_;
+
+  OpusEncoder* opus_encoder_;

[mcasas, 2015/10/21 19:19:52]

Hmm this smells like a base::WeakPtr<>. However, there's another 
problem: |opus_encoder_| is created and destroyed on Main
thread, but is used on EncodeFromFilledBuffer() on 
|origin_task_runner_| thread. How's that not a race?

I'd suggest following a pattern like VideoTrackRecorder with the
ScopedVpxCodecCtxPtr [1], then from ~AudioEncoder(), send
|opus_encoder_| to be destroyed in |origin_task_runner_| via
DeleteSoon().

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

[ajose, 2015/10/23 22:46:33]

Think this is resolved with new structure - AudioEncoder is created and
destroyed on main, but all AudioEncoder work should happen on |encoding_thread_|
(with tasks potentialy being posted from main and audio capture thread). WDYT?

+
+  DISALLOW_COPY_AND_ASSIGN(AudioEncoder);
+};
+
+AudioTrackRecorder::AudioEncoder::~AudioEncoder() {
+  LOG(INFO) << "AudioEncoder dtor";
+  if (opus_encoder_) {
+    LOG(INFO) << "destroying opus";
+    opus_encoder_destroy(opus_encoder_);
+    opus_encoder_ = nullptr;
+  }
+}
+
+void AudioTrackRecorder::AudioEncoder::EncodeAudio(
+    const media::AudioBus& audio_bus,
+    const base::TimeTicks& recorded_time) {
+  DCHECK(IsInitialized());
+  DCHECK(!recorded_time.is_null());
+  DCHECK_EQ(audio_bus.channels(), audio_params_->channels());
+  DCHECK_EQ(audio_bus.frames(), audio_params_->frames_per_buffer());
+
+  if (!origin_task_runner_.get())
+    origin_task_runner_ = base::MessageLoop::current()->task_runner();

[miu, 2015/10/21 20:35:38]

This doesn't seem safe to me.  Usually encoding should happen on its own thread.
 (See comment below.)

[ajose, 2015/10/23 22:46:33]

Done.

+  DCHECK(origin_task_runner_->BelongsToCurrentThread());
+
+  // Encode all audio in |audio_bus| into zero or more frames.
+  int src_pos = 0;
+  while (src_pos < audio_bus.frames()) {
+    const int num_samples_to_xfer = std::min(
+        samples_per_frame_ - buffer_fill_end_, audio_bus.frames() - src_pos);
+    TransferSamplesIntoBuffer(audio_bus, src_pos, buffer_fill_end_,
+                              num_samples_to_xfer);
+    src_pos += num_samples_to_xfer;
+    buffer_fill_end_ += num_samples_to_xfer;
+
+    if (buffer_fill_end_ < samples_per_frame_)
+      break;
+
+    scoped_ptr<std::string> encoded_data(new std::string());
+    if (EncodeFromFilledBuffer(encoded_data.get())) {
+      origin_task_runner_->PostTask(
+          FROM_HERE, base::Bind(on_encoded_audio_cb_, *audio_params_,
+                                base::Passed(&encoded_data), recorded_time));
+    }
+
+    // Reset the internal buffer for the next frame.
+    buffer_fill_end_ = 0;
+  }
+}
+
+void AudioTrackRecorder::AudioEncoder::InitOpus(
+    const media::AudioParameters& params) {

[mcasas, 2015/10/21 19:19:52]

Thread check plz, here and elsewhere.

[ajose, 2015/10/23 22:46:33]

Done.

+  DCHECK(params.IsValid());
+  DCHECK_EQ(params.bits_per_sample(), 16);
+  if (audio_params_.get() && audio_params_->Equals(params))

[mcasas, 2015/10/21 19:19:52]

How can |audio_params_.get()| be nullptr if it's a reference?

+    return;
+
+  const int sampling_rate = params.sample_rate();
+  const int num_channels = params.channels();
+  samples_per_frame_ = sampling_rate / kDefaultFramesPerSecond;
+  const base::TimeDelta frame_duration = base::TimeDelta::FromMicroseconds(
+      base::Time::kMicrosecondsPerSecond * samples_per_frame_ / sampling_rate);
+
+  if (frame_duration == base::TimeDelta() ||
+      !IsValidFrameDuration(frame_duration)) {
+    DVLOG(1) << __FUNCTION__ << ": bad frame duration: " << frame_duration;
+  }
+
+  // Support for max sampling rate of 48KHz, 2 channels, 100 ms duration.
+  const int kMaxSamplesTimesChannelsPerFrame = 48 * 2 * 100;
+  if (num_channels <= 0 || samples_per_frame_ <= 0 ||
+      samples_per_frame_ * num_channels > kMaxSamplesTimesChannelsPerFrame ||
+      sampling_rate % samples_per_frame_ != 0) {

[mcasas, 2015/10/21 19:19:52]

I see what you do here but is more customary to 
check values as close as possible to the assignments.

Also, you do a DCHECK(params.IsValid()) on l.132
but then act upon (part of) its tests (num_channels <= 0).

I'd suggest, instead of l.132
if (!params.IsValid()) { 
  DLOG(ERROR) << "Invalid params: " << params.AsHumanReadableString();
  return false();
}
DCHECK_EQ(params.bits_per_sample(), 16);
// etc

And check only for specific validity on every assignment afterwards,
either via DCHECK or if(badstuff) { DLOG(ERROR); return false; }

[ajose, 2015/10/23 22:46:33]

Might have gone overboard...

+    DVLOG(1) << __FUNCTION__ << ": bad inputs:"
+             << "\nnum channels: " << num_channels
+             << "\nsamples per frame: " << samples_per_frame_
+             << "\nsampling rate: " << sampling_rate;
+    return;
+  }
+
+  // Initialize AudioBus buffer for OpusEncoder.
+  buffer_fill_end_ = 0;
+  buffer_.reset(new float[num_channels * samples_per_frame_]);
+
+  // Initialize OpusEncoder.
+  int opus_result;
+  opus_encoder_ = opus_encoder_create(sampling_rate, num_channels,

[miu, 2015/10/21 20:35:38]

Can this InitOpus() method be called multiple times?  (IIUC, the answer is 'yes'
since the audio format could change via calls to OnSetFormat().)  Therefore, you
should check whether |opus_encoder_| is not null, and destroy the old one first
(or keep the current instance and change its configuration, if possible).

[ajose, 2015/10/23 22:46:33]

Done.

+                                      OPUS_APPLICATION_AUDIO, &opus_result);
+  if (opus_result < 0) {
+    DVLOG(1) << __FUNCTION__ << ": couldn't initialize opus encoder: "

[mcasas, 2015/10/21 19:19:52]

There's no real need for __FUNCTION__ here, and I'd write DLOG(ERROR) <<...

[ajose, 2015/10/23 22:46:33]

Done.

+             << opus_strerror(opus_result);
+    return;
+  }
+
+  audio_params_.reset(new media::AudioParameters(params));
+
+  // Note: As of 2013-10-31, the encoder in "auto bitrate" mode would use a
+  // variable bitrate up to 102kbps for 2-channel, 48 kHz audio and a 10 ms
+  // frame size.  The opus library authors may, of course, adjust this in
+  // later versions.

[mcasas, 2015/10/21 19:19:52]

Could we codify this assumption?

  int bitrate;
  DCHECK(opus_encoder_ctl(enc, OPUS_GET_BITRATE(&bitrate)) == OPUS_OK &&
         bitrate == kWhateverBitrateIsAssumed);

+  CHECK_EQ(opus_encoder_ctl(opus_encoder_, OPUS_SET_BITRATE(OPUS_AUTO)),

[mcasas, 2015/10/21 19:19:51]

Here you say: crash the renderer tab if opus_encoder_ctl() can't set
the bitrate, isn't that a bit drastic? Perhaps we could make 
InitOpus() return a boolean and act upon it on AudioTrackRecorder()?

[ajose, 2015/10/23 22:46:33]

Done.

+           OPUS_OK);
+}
+
+void AudioTrackRecorder::AudioEncoder::TransferSamplesIntoBuffer(
+    // const media::AudioBus* audio_bus,
+    const media::AudioBus& audio_bus,
+    int source_offset,
+    int buffer_fill_offset,
+    int num_samples) {
+  // Opus requires channel-interleaved samples in a single array.
+  for (int ch = 0; ch < audio_bus.channels(); ++ch) {
+    const float* src = audio_bus.channel(ch) + source_offset;
+    const float* const src_end = src + num_samples;
+    float* dest =
+        buffer_.get() + buffer_fill_offset * audio_params_->channels() + ch;
+    for (; src < src_end; ++src, dest += audio_params_->channels())
+      *dest = *src;
+  }
+}
+
+bool AudioTrackRecorder::AudioEncoder::EncodeFromFilledBuffer(
+    std::string* out) {
+  out->resize(kOpusMaxPayloadSize);
+  const opus_int32 result = opus_encode_float(
+      opus_encoder_, buffer_.get(), samples_per_frame_,
+      reinterpret_cast<uint8*>(string_as_array(out)), kOpusMaxPayloadSize);

[mcasas, 2015/10/21 19:19:52]

What about 
s/reinterpret_cast<uint8*>(string_as_array(out))/out->data()/ ?

[ajose, 2015/10/23 22:46:33]

out->data() is const char*, need unsigned char* but the internet says const_cast
might be finicky - worth changing?

+  if (result > 1) {
+    out->resize(result);
+    return true;
+  } else if (result < 0) {
+    LOG(ERROR) << __FUNCTION__ << ": Error code from opus_encode_float(): "
+               << opus_strerror(result);
+    return false;
+  } else {
+    // Do nothing: The documentation says that a return value of zero or
+    // one means the packet does not need to be transmitted.
+    return false;
+  }

[mcasas, 2015/10/21 19:19:52]

What about:

if (result > 1) {
  out->resize(result);
  return true;
}
// Comment on possible <=1 return values...
DLOG_IF(ERROR, result < 0) << "Error! resistance is futile" << ...;
return false;

[ajose, 2015/10/23 22:46:33]

Done.

+}
+
+// static
+bool AudioTrackRecorder::AudioEncoder::IsValidFrameDuration(
+    base::TimeDelta duration) {
+  // See https://tools.ietf.org/html/rfc6716#section-2.1.4

[mcasas, 2015/10/21 19:19:52]

Maybe also mention opus.h? [1]

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/opus/s...

+  return duration == base::TimeDelta::FromMicroseconds(2500) ||
+         duration == base::TimeDelta::FromMilliseconds(5) ||
+         duration == base::TimeDelta::FromMilliseconds(10) ||
+         duration == base::TimeDelta::FromMilliseconds(20) ||
+         duration == base::TimeDelta::FromMilliseconds(40) ||
+         duration == base::TimeDelta::FromMilliseconds(60);

[mcasas, 2015/10/21 19:19:52]

It sucks that we have to check for this duration ourselves!
Is there no (perhaps obscure) opus function for that? (I 
realize the answer might be "no!" ).

[ajose, 2015/10/23 22:46:33]

Not that I can find :(

+}
+
+AudioTrackRecorder::AudioTrackRecorder(
+    const blink::WebMediaStreamTrack& track,
+    const OnEncodedAudioCB& on_encoded_audio_cb)
+    : track_(track),
+      encoder_(new AudioEncoder(on_encoded_audio_cb)),

[miu, 2015/10/21 20:35:38]

Note: If AudioEncoder was only used on a separate encode thread, you could wrap
this callback like so:

  new AudioEncoder(media::BindToCurrentLoop(on_encoded_audio_cb))

This would let the encoder thread trampoline the run of the callback to the
current thread.

[ajose, 2015/10/23 22:46:33]

Nice!

+      on_encoded_audio_cb_(on_encoded_audio_cb) {
+  DCHECK(main_render_thread_checker_.CalledOnValidThread());
+  DCHECK(!track_.isNull());
+  DCHECK(track_.extraData());
+  // Connect the source provider to the track as a sink.
+  MediaStreamAudioSink::AddToAudioTrack(this, track_);
+}
+
+AudioTrackRecorder::~AudioTrackRecorder() {
+  DCHECK(main_render_thread_checker_.CalledOnValidThread());
+  MediaStreamAudioSink::RemoveFromAudioTrack(this, track_);
+  track_.reset();
+  LOG(INFO) << "ATR dtor";
+}
+
+void AudioTrackRecorder::OnData(const media::AudioBus& audio_bus,
+                                base::TimeTicks estimated_capture_time) {
+  DCHECK(capture_thread_checker_.CalledOnValidThread());
+  encoder_->EncodeAudio(audio_bus, estimated_capture_time);

[miu, 2015/10/21 20:35:38]

Is this method being called on a real-time audio thread?  If so, you should not
be doing audio encoding on this thread.  Instead, a separate encoder thread
should be used.

[ajose, 2015/10/23 22:46:33]

Done.

+}
+
+void AudioTrackRecorder::OnSetFormat(const media::AudioParameters& params) {
+  // If the source is restarted, might have changed to another capture thread.
+  capture_thread_checker_.DetachFromThread();
+  DCHECK(capture_thread_checker_.CalledOnValidThread());
+  encoder_->OnSetFormat(params);
+}
+
+}  // namespace content