Binding media stream audio track to speech recognition [renderer]

content/renderer/speech_recognition_audio_source_provider.cc

+// Copyright 2014 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 "content/renderer/speech_recognition_audio_source_provider.h"
+
+#include "base/logging.h"
+#include "base/memory/shared_memory.h"
+#include "base/time/time.h"
+#include "media/audio/audio_parameters.h"
+#include "media/base/audio_fifo.h"
+
+namespace content {
+
+SpeechRecognitionAudioSourceProvider::SpeechRecognitionAudioSourceProvider(
+    const blink::WebMediaStreamTrack& track,
+    const media::AudioParameters& params,
+    base::SharedMemoryHandle memory,
+    int memory_length,
+    const OnDataCB& on_data_callback,
+    const OnErrorCB& on_error_callback
+    )

[tommi (sloooow) - chröme, 2014/08/25 14:38:40]

put on previous line?

[burnik, 2014/08/29 09:18:15]

Done.

+    : track_(track),
+      shared_memory_(memory, false),
+      output_params_(params),

[tommi (sloooow) - chröme, 2014/08/25 14:38:45]

should output_params_ be const?

[burnik, 2014/08/29 09:18:16]

I think so. AudioConverter marks them const. I'm only reading them here.
On 2014/08/25 14:38:45, tommi wrote:

[tommi (sloooow) - chröme, 2014/08/29 11:25:30]

OK, but I'm not seeing that it has been changed to const.

+      on_data_callback_(on_data_callback),
+      on_error_callback_(on_error_callback),
+      attached_converter_(false),
+      track_stopped_(false),
+      unconsumed_audio_buffers_(0) {
+  DCHECK(shared_memory_.Map(memory_length));

[tommi (sloooow) - chröme, 2014/08/25 14:38:45]

I think this is a bug... DCHECK()ed code isn't included in release builds, so
Map() will never run.

[burnik, 2014/08/29 09:18:15]

Done.

+  DCHECK_EQ(memory_length, media::AudioBus::CalculateMemorySize(params));
+  DCHECK(params.IsValid());
+  DCHECK(!on_data_callback_.is_null());
+
+  output_bus_ = media::AudioBus::WrapMemory(params, shared_memory_.memory());
+
+  // Connect the source provider to the track as a sink.
+  MediaStreamAudioSink::AddToAudioTrack(this, track_);
+}
+
+SpeechRecognitionAudioSourceProvider::~SpeechRecognitionAudioSourceProvider() {

[tommi (sloooow) - chröme, 2014/08/25 14:38:45]

missing thread check for dtor

[burnik, 2014/08/29 09:18:16]

Done.

+  if (audio_converter_.get() && attached_converter_)
+    audio_converter_->RemoveInput(this);
+  // If the track is still active, it is necessary to notify the track before
+  // the sink goes away.
+  if (!track_stopped_)
+    MediaStreamAudioSink::RemoveFromAudioTrack(this, track_);
+}
+
+void SpeechRecognitionAudioSourceProvider::OnSetFormat(
+    const media::AudioParameters& input_params) {
+  // We need detach the thread here because it will be a new capture thread
+  // calling OnSetFormat() and OnData() if the source is restarted.
+  capture_thread_checker_.DetachFromThread();
+  DCHECK(capture_thread_checker_.CalledOnValidThread());

[tommi (sloooow) - chröme, 2014/08/25 14:38:45]

this dcheck will never hit because of the Detach on the previous line

[tommi (sloooow) - chröme, 2014/08/29 11:25:30]

Anything needed to be done here?

+  DCHECK(input_params.IsValid());

[tommi (sloooow) - chröme, 2014/08/25 14:38:45]

would it make sense to assert that input_params_ (member variable) has not been
set when we get here?

[burnik, 2014/08/29 09:18:15]

I believe previous state of input_params_ does not matter. We overwrite it (or
write it for the first time) when this event occurs.
On 2014/08/25 14:38:45, tommi wrote:

+
+  input_params_ = input_params;
+  // TODO(burnik): Check if this is necessary:
+  // Create the audio converter with |disable_fifo| as false so that the
+  // converter will request input_params.frames_per_buffer() each time.
+  // This will not increase the complexity as there is only one client to
+  // the converter.
+  audio_converter_.reset(
+      new media::AudioConverter(input_params, output_params_, false));
+
+  DCHECK_EQ(0,output_params_.frames_per_buffer() * input_params_.sample_rate() %

[tommi (sloooow) - chröme, 2014/08/25 14:38:45]

space after ,

[burnik, 2014/08/29 09:18:16]

Done.

+    output_params_.sample_rate());

[tommi (sloooow) - chröme, 2014/08/25 14:38:45]

4 space indent

+  fifo_buffer_size_ = output_params_.frames_per_buffer() *
+    input_params_.sample_rate() / output_params_.sample_rate();

[tommi (sloooow) - chröme, 2014/08/25 14:38:44]

4 spaces

[burnik, 2014/08/29 09:18:15]

Done.

+
+  int frames_in_fifo = kNumberOfBuffersInFifo * fifo_buffer_size_;
+
+  fifo_.reset(new media::AudioFifo(input_params.channels(), frames_in_fifo));
+  input_bus_ = media::AudioBus::Create(input_params.channels(),
+                                       input_params.frames_per_buffer());
+}
+
+void SpeechRecognitionAudioSourceProvider::OnReadyStateChanged(

[tommi (sloooow) - chröme, 2014/08/25 14:38:45]

thread check here?

[burnik, 2014/08/29 09:18:15]

Done.

+      blink::WebMediaStreamSource::ReadyState state) {
+  if (state == blink::WebMediaStreamSource::ReadyStateEnded)
+    track_stopped_ = true;

[tommi (sloooow) - chröme, 2014/08/25 14:38:40]

would it make sense to add

else
  DCHECK(!track_stopped_);

?

[burnik, 2014/08/29 09:18:15]

Done.

+}
+
+void SpeechRecognitionAudioSourceProvider::OnData(

[tommi (sloooow) - chröme, 2014/08/25 14:38:45]

on which thread does this function run?

[burnik, 2014/08/29 09:18:16]

capturer thread. Done.
On 2014/08/25 14:38:45, tommi wrote:

+    const int16* audio_data,
+    int sample_rate,
+    int number_of_channels,
+    int number_of_frames) {
+

[tommi (sloooow) - chröme, 2014/08/25 14:38:45]

remove empty line

[burnik, 2014/08/29 09:18:15]

Done.

+  // TODO(burnik): Remove this if sync socket proves a good alternative to IPCs
+  // Notify client the FIFO is overflowing and stop
+  if (fifo_->frames() + number_of_frames > fifo_->max_frames()) {

[tommi (sloooow) - chröme, 2014/08/25 14:38:45]

it's not clear to me if fifo_ needs protection etc.  I think it would be good if
every method would have a thread check so that the threading model can be easily
understood and verified.

[burnik, 2014/08/29 09:18:16]

Done.

+    if (attached_converter_) {
+      audio_converter_->RemoveInput(this);
+      attached_converter_ = false;
+      on_error_callback_.Run();
+    }
+    return;
+  }
+
+  DCHECK(capture_thread_checker_.CalledOnValidThread());

[tommi (sloooow) - chröme, 2014/08/25 14:38:40]

ah... this should be at the top of the function.  As is, you don't run this
check if you get into the if() statement above.

[burnik, 2014/08/29 09:18:15]

Done.

+  DCHECK(input_bus_->frames() == number_of_frames);
+  DCHECK(input_bus_->channels() == number_of_channels);
+  DCHECK_LE(fifo_->frames() + number_of_frames, fifo_->max_frames());
+
+  // TODO(xians): A better way to handle the interleaved and deinterleaved
+  // format switching, see issue/317710.
+  input_bus_->FromInterleaved(audio_data, number_of_frames,
+                              sizeof(audio_data[0]));
+
+  fifo_->Push(input_bus_.get());
+
+  // wait for FIFO to have at least |fifo_buffer_size_| frames ready
+  if (fifo_->frames() < fifo_buffer_size_)
+    return;
+  // Attach converter when we first reach |fifo_buffer_size_| frames in the FIFO
+  if (!attached_converter_) {
+    audio_converter_->AddInput(this);
+    attached_converter_ = true;
+    // we need one more buffer of |number_of_frames| before we start converting
+    return;
+  }
+
+  // make sure the previous output buffer was consumed by the client

[tommi (sloooow) - chröme, 2014/08/25 14:38:45]

why is it safe to touch the above member variables without the lock but not the
ones below? (some documentation would be good to have to explain - I may have
missed it too, so just point me to it if that's the case)

[burnik, 2014/08/29 09:18:16]

This was only intended for protecting unconsumed_audio_buffers_. That's because
the NotifyAudioBusConsumed() was called on the main render thread, and OnData on
the capture thread. This event model has been removed in the next patchset.
On 2014/08/25 14:38:45, tommi wrote:

+  base::AutoLock auto_lock(lock_);
+  if (unconsumed_audio_buffers_ > 0) {
+    DLOG(WARNING) << "Client still consuming buffers: "
+                  << unconsumed_audio_buffers_;
+    return;
+  }
+  // first call to |Convert| must have |fifo_buffer_size_| + |number_of_frames|
+  // waiting on the FIFO since it will trigger one extra |ProvideInput| call.
+  // This way it is also guaranteed to have a non-empty first output buffer
+  audio_converter_->Convert(output_bus_.get());
+
+  DCHECK_EQ(unconsumed_audio_buffers_, 0);
+  ++unconsumed_audio_buffers_;
+  // Notify client to consume buffer on |output_bus_|
+  on_data_callback_.Run();

[tommi (sloooow) - chröme, 2014/08/25 14:38:45]

if there's a way to avoid holding the lock when firing this callback, then that
would be good.  I think that on_data_callback_ is const and won't ever change
throughout the lifetime of |this|, so we likely do not need to hold the lock
here.

[burnik, 2014/08/29 09:18:16]

Locks removed from the design.
On 2014/08/25 14:38:45, tommi wrote:

+}
+
+double SpeechRecognitionAudioSourceProvider::ProvideInput(
+    media::AudioBus* audio_bus, base::TimeDelta buffer_delay) {
+  DCHECK(capture_thread_checker_.CalledOnValidThread());
+  DCHECK_GE(fifo_->frames(), audio_bus->frames());
+  // Consume queued input frames by passing them to |audio_converter_|
+  fifo_->Consume(audio_bus, 0, audio_bus->frames());
+  return 1.0;
+}
+
+void SpeechRecognitionAudioSourceProvider::NotifyAudioBusConsumed() {
+  // client notifies us previous output buffer was consumed
+  base::AutoLock auto_lock(lock_);
+  --unconsumed_audio_buffers_;
+}
+
+}  // namespace content