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/threading/thread_restrictions.h"

[no longer working on chromium, 2014/09/16 12:44:05]

why do you have this base/threading/thread_restrictions.h in both the header and
implementation?

[burnik, 2014/09/16 19:10:22]

Removed from implementation.
On 2014/09/16 12:44:05, xians1 wrote:

+#include "base/time/time.h"

[no longer working on chromium, 2014/09/15 08:31:28]

nit, alphabet order

[burnik, 2014/09/15 15:00:06]

Alphabetic order of what?
On 2014/09/15 08:31:28, xians1 wrote:

[no longer working on chromium, 2014/09/16 12:44:05]

I was wrong, ignore it.

[burnik, 2014/09/16 19:10:22]

Acknowledged.

+#include "media/audio/audio_parameters.h"
+#include "media/base/audio_fifo.h"
+
+namespace content {
+
+SpeechRecognitionAudioSourceProvider::SpeechRecognitionAudioSourceProvider(
+    const blink::WebMediaStreamTrack& track,
+    const media::AudioParameters& params, const base::SharedMemoryHandle memory,
+    base::SyncSocket* socket, OnErrorCB on_error_cb)
+    : track_(track),
+      shared_memory_(memory, false),
+      socket_(socket),
+      output_params_(params),
+      track_stopped_(false),
+      buffer_index_(0),
+      on_error_cb_(on_error_cb) {
+  DCHECK(main_render_thread_checker_.CalledOnValidThread());
+  DCHECK(params.IsValid());
+  const size_t memory_length = media::AudioBus::CalculateMemorySize(params) +
+                               sizeof(media::AudioInputBufferParameters);
+  CHECK(shared_memory_.Map(memory_length));
+
+  uint8* ptr = static_cast<uint8*>(shared_memory_.memory());
+  media::AudioInputBuffer* buffer =
+      reinterpret_cast<media::AudioInputBuffer*>(ptr);
+  // Keep params for sync with client via |params.size| on the shared memory.
+  peer_buffer_index_ = &(buffer->params.size);

[no longer working on chromium, 2014/09/15 08:31:29]

I think it is a bit wrong, the shared_memory_ has not been used before, why
should you read the value there?
Simply, you can initialize peer_buffer_index_ to 0 here.

[burnik, 2014/09/15 15:00:05]

It has been used, on the browser process. Was init to 0 upon alloc and share.
Makes sense to me to alloc and init in the same place.
On 2014/09/15 08:31:29, xians1 wrote:

+  // Client must manage his own counter and reset it.
+  DCHECK_EQ(0U, *peer_buffer_index_);
+  output_bus_ = media::AudioBus::WrapMemory(params, buffer->audio);
+  // Connect the source provider to the track as a sink.
+  MediaStreamAudioSink::AddToAudioTrack(this, track_);
+}
+
+SpeechRecognitionAudioSourceProvider::~SpeechRecognitionAudioSourceProvider() {
+  DCHECK(main_render_thread_checker_.CalledOnValidThread());
+  if (audio_converter_.get()) audio_converter_->RemoveInput(this);
+  // Notify the track before this sink goes away.
+  if (!track_stopped_) MediaStreamAudioSink::RemoveFromAudioTrack(this, track_);
+}
+
+// static
+bool SpeechRecognitionAudioSourceProvider::IsAllowedAudioTrack(

[no longer working on chromium, 2014/09/15 08:31:29]

IsAudioTrackSupported() seems a more suitable name here.

[burnik, 2014/09/15 15:00:05]

"Supported" would indicate there is a technical barrier to supporting. Here it's
actually a policy because of the dreaded *abuse* SR could experience.
On 2014/09/15 08:31:29, xians1 wrote:

[no longer working on chromium, 2014/09/16 12:44:05]

The policy you mentioned is just one of the purposes why we have this method,
the |track| can be any track JS injects, like video track, remote audio track,
or screen cast track.

+    const blink::WebMediaStreamTrack& track) {
+  DCHECK(track.source().type() == blink::WebMediaStreamSource::TypeAudio);

[no longer working on chromium, 2014/09/15 08:31:28]

you can't put DCHECK here, this method is trigger by JS, and developer can do
whatever they want.
Just return false if it is not TypeAudio

[burnik, 2014/09/15 15:00:05]

True, no checks were done elsewhere. Done.
On 2014/09/15 08:31:28, xians1 wrote:

+  MediaStreamAudioSource* native_source =
+      static_cast<MediaStreamAudioSource*>(track.source().extraData());
+  DCHECK(native_source);

[no longer working on chromium, 2014/09/15 08:31:28]

Same here, return false if native_source does not exist.

[burnik, 2014/09/15 15:00:05]

Done.

+  const StreamDeviceInfo& device_info = native_source->device_info();
+  // Purposely only support tracks from an audio device. Dissallow WebAudio.
+  return (device_info.device.type == content::MEDIA_DEVICE_AUDIO_CAPTURE);
+}
+
+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());
+  DCHECK(input_params.IsValid());
+
+  input_params_ = input_params;
+  fifo_buffer_size_ = output_params_.frames_per_buffer() *

[no longer working on chromium, 2014/09/15 08:31:28]

how is this cast?

[burnik, 2014/09/15 15:00:05]

Floored. Integer division.
On 2014/09/15 08:31:28, xians1 wrote:

[no longer working on chromium, 2014/09/16 12:44:05]

Do it in C++ way, add static_cast<int>() here.

Also, could you explain why floor is used instead of ceiling?

[burnik, 2014/09/16 19:10:22]

Input and output params are of media::AudioParameters type.
All members here are int. Integer division omits decimals.
Added DCHECK(output_params_.IsValid()); to next patchset which will check if
output sample rate is 0.
In production - input will be 44100 with 441 frames and output will be 16000
with 1600 frames. 
Also, DCHECKS which follow check if we have enough buffer.

On 2014/09/16 12:44:05, xians1 wrote:

[no longer working on chromium, 2014/09/17 15:55:19]

The example you are taking is just what it is on your machine, the input sample
rate can be any of the hardware sample rates, from 8k up to 192k

[burnik, 2014/09/18 19:09:21]

Ok, Agreed. 

So if I do it this way:

fifo_buffer_size_ =
      std::ceil(output_params_.frames_per_buffer() *
                static_cast<double>(input_params_.sample_rate()) /
                output_params_.sample_rate());

I've tested, and it would work properly for these:

================================
 in.sr  in.fpb  out.sr  out.fpb
--------------------------------
  8000      80   16000     1600
  8000     800   16000     1600
 16000     160   16000     1600
 16000    1600   16000     1600
 32000     320   16000     1600
 32000    3200   16000     1600
 44100     441   16000     1600
 44100    4410   16000     1600
 48000     480   16000     1600
 48000    4800   16000     1600
 96000     960   16000     1600
 96000    9600   16000     1600
 11025     111*  16000     1600
 11025    1103*  16000     1600
 22050     221*  16000     1600
 22050    2205   16000     1600
 88200     882   16000     1600
 88200    8820   16000     1600
 176400   1764   16000     1600
 176400  17640   16000     1600
 192000   1920   16000     1600
 192000  19200   16000     1600
================================

* These starred are always rounded up, right?

[no longer working on chromium, 2014/09/19 08:58:56]

I think this looks correct.

+                      input_params_.sample_rate() /
+                      output_params_.sample_rate();
+  DCHECK_GE(fifo_buffer_size_, input_params_.frames_per_buffer());
+  DCHECK_GE(fifo_buffer_size_, output_params_.frames_per_buffer());
+
+  // Allows for some delays on the endpoint client.
+  static const int kNumberOfBuffersInFifo = 2;
+  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());
+
+  // 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));
+  audio_converter_->AddInput(this);
+}
+
+void SpeechRecognitionAudioSourceProvider::OnReadyStateChanged(
+    blink::WebMediaStreamSource::ReadyState state) {
+  DCHECK(main_render_thread_checker_.CalledOnValidThread());
+  if (track_stopped_) return;

[no longer working on chromium, 2014/09/15 08:31:27]

new line.

[burnik, 2014/09/15 15:00:06]

Done. However, clang-format proposes this way.
On 2014/09/15 08:31:27, xians1 wrote:

+  if (state == blink::WebMediaStreamSource::ReadyStateEnded) {

[no longer working on chromium, 2014/09/15 08:31:28]

add an empty line before the second if (

[burnik, 2014/09/15 15:00:05]

Done.

+    track_stopped_ = true;
+    MediaStreamAudioSink::RemoveFromAudioTrack(this, track_);

[no longer working on chromium, 2014/09/15 08:31:28]

Remove this line of code.
track_ has already been ended, you should not call into the track_ any more.

[burnik, 2014/09/15 15:00:06]

Are you sure? Will the MediaStreamAudioSink remove the track on it's own? Can
you point me to that code, please?
This is paired with the dtor of the class.
On 2014/09/15 08:31:28, xians1 wrote:

+    NotifyErrorState(ErrorState::TRACK_STOPPED);

[no longer working on chromium, 2014/09/15 08:31:29]

hmm, track ended state is not an error, ErrorState should not include
TRACK_STOPPED at all.

[burnik, 2014/09/15 15:00:06]

Agreed. It's here for now as I refactor.
On 2014/09/15 08:31:29, xians1 wrote:

+  }
+}
+
+void SpeechRecognitionAudioSourceProvider::OnData(const int16* audio_data,
+                                                  int sample_rate,
+                                                  int number_of_channels,
+                                                  int number_of_frames) {
+  DCHECK(capture_thread_checker_.CalledOnValidThread());
+  DCHECK(peer_buffer_index_);
+  DCHECK_EQ(input_bus_->frames(), number_of_frames);
+  DCHECK_EQ(input_bus_->channels(), number_of_channels);
+  if (fifo_->frames() + number_of_frames > fifo_->max_frames()) {
+    NotifyErrorState(ErrorState::AUDIO_FIFO_OVERFLOW);

[no longer working on chromium, 2014/09/15 08:31:28]

Log it.
Also, could you please explain what the client supposes to do when getting a
AUDIO_FIFO_OVERFLOW callback?

[burnik, 2014/09/15 15:00:06]

Logged via DLOG(ERROR).
Client can destroy the audio source provider and potentially end the session
early.
On 2014/09/15 08:31:28, xians1 wrote:

+    return;
+  }
+  // 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;

[no longer working on chromium, 2014/09/15 08:31:28]

empty line for the return

[burnik, 2014/09/15 15:00:06]

Done.

+
+  // Make sure the previous output buffer was consumed by client before we send
+  // the next buffer. |peer_buffer_index_| is pointing to shared memory.
+  // The client must write to it (incrementing by 1) once the the buffer was
+  // consumed. This is intentional not to block this audio capturing thread.
+  if (buffer_index_ != (*peer_buffer_index_)) {
+    NotifyErrorState(ErrorState::BUFFER_SYNC_LAG);
+    return;
+  }
+
+  audio_converter_->Convert(output_bus_.get());
+
+  // Notify client to consume buffer |buffer_index_| on |output_bus_|.
+  const size_t bytes_sent =
+      socket_->Send(&buffer_index_, sizeof(buffer_index_));
+  if (bytes_sent != sizeof(buffer_index_)) {
+    // The send usually fails if the user changes his input audio device.
+    NotifyErrorState(ErrorState::SEND_FAILED);
+    // We have discarded this buffer, but could still recover on the next one.
+    // Although, if the socket was closed, this will shortly end up
+    // in |ErrorState::AUDIO_FIFO_OVERFLOW|.
+    return;
+  }
+
+  // Count the sent buffer. We expect the client to do the same on his end.
+  ++buffer_index_;
+}
+
+double SpeechRecognitionAudioSourceProvider::ProvideInput(
+    media::AudioBus* audio_bus, base::TimeDelta buffer_delay) {
+  DCHECK(capture_thread_checker_.CalledOnValidThread());
+  if (fifo_->frames() >= audio_bus->frames())
+    fifo_->Consume(audio_bus, 0, audio_bus->frames());
+  else
+    audio_bus->Zero();

[no longer working on chromium, 2014/09/15 08:31:29]

do you know if the else case can happen here?

[burnik, 2014/09/15 15:00:05]

Yes. The else happens when we attach to the converter in |OnSetFormat|.
Otherwise wouldn't be removing the |attached_converter_|.
On 2014/09/15 08:31:29, xians1 wrote:

+  return 1.0;

[no longer working on chromium, 2014/09/15 08:31:28]

empty line before the return.

[burnik, 2014/09/15 15:00:05]

Done.

+}
+
+void SpeechRecognitionAudioSourceProvider::NotifyErrorState(ErrorState error) {
+  // TODO(burnik): Runs on capture thread. Should run on main renderer thread!
+  DCHECK(capture_thread_checker_.CalledOnValidThread());
+  if (on_error_cb_.is_null()) return;
+  on_error_cb_.Run(error);
+}
+
+}  // namespace content