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"
+#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,
+    base::NativeSyncSocket::Descriptor socket,
+    int memory_length)
+    : track_(track),
+      shared_memory_(memory, false),
+      socket_(base::NativeSyncSocket::Unwrap(socket)),
+      output_params_(params),
+      attached_converter_(false),
+      track_stopped_(false),
+      buffer_index_(0) {
+  DCHECK_EQ(memory_length, media::AudioBus::CalculateMemorySize(params));
+  DCHECK(params.IsValid());
+  if (!shared_memory_.Map(memory_length)) {
+    DLOG(ERROR) << "Could not map the shared memory";

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

Would this be a serious enough of an error to just do this?

CHECK(shared_memory_.Map(memory_length));

(maybe you could check other places where we call SharedMemory::Map in the
renderer process)

[burnik, 2014/08/29 13:26:17]

Done. Although I will revisit this when I do more work on errors. 
On 2014/08/29 11:25:31, tommi wrote:

+    return;
+  }
+  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() {
+  DCHECK(main_render_thread_checker_.CalledOnValidThread());
+  if (audio_converter_.get() && attached_converter_)

[no longer working on chromium, 2014/08/29 12:23:06]

you are complicating things, you should just add the input to the converter when
you construct the converter and you probably don't even need to call
RemoveInput() in the destructor since the object is going away.

[burnik, 2014/08/29 13:26:16]

Adding the converter on construction will trigger problems with |ProvideInput()|
since we won't have enough data on the FIFO. Therefore I would have to Zero()
out the bus delivering empty data to the browser. I will check if RemoveInput is
necessary or not.

On 2014/08/29 12:23:06, xians1 wrote:

[burnik, 2014/09/12 12:09:12]

Done.

+    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_);
+}
+
+bool SpeechRecognitionAudioSourceProvider::IsAllowedAudioTrack(

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

this method is static, right?  If so, there should be a comment above indicating
that:

// static
bool SpeechRecognitionAudioSourceProvider::IsAllowedAudioTrack(

If it is not static, then it's missing a thread check.

[burnik, 2014/08/29 13:26:17]

It is static. Done.
On 2014/08/29 11:25:31, tommi wrote:

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

[no longer working on chromium, 2014/08/29 12:23:06]

you need to check if the track is local or not, if it is a remote track, it
might not have MediaStreamAudioSource

[burnik, 2014/08/29 13:26:16]

Here I check if it's WebAudio.
On 2014/08/29 12:23:06, xians1 wrote:

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

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

no space after static_cast

[burnik, 2014/08/29 13:26:17]

Done.

+  StreamDeviceInfo device_info = native_source->device_info();

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

no need to create a new StreamDeviceInfo instance.  if you need a variable, just
use a const reference:
const StreamDeviceInfo& device_info = native_source->device_info();

or, just call device_info() inline in expression you return:

return native_source->device_info().device.type ==
       content::MEDIA_DEVICE_AUDIO_CAPTURE;

[burnik, 2014/08/29 13:26:17]

Done.

+  return (device_info.device.type == content::MEDIA_DEVICE_AUDIO_CAPTURE);

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

this seems to me to be a 'supported' check rather than an 'allowed' check. 
"Allowed" usually refers to policy or permission checks.  If this function is
simply supposed to answer the question whether the implementation supports this
type of track, I'd prefer to name it IsAudioTrackSupported() or possibly
IsTrackTypeSupported().

[burnik, 2014/08/29 13:26:17]

This is implemented as a response to a suggestion from an e-mail thread saying
WebAudio should not be allowed yet, although it is supported (e.g. if we return
true here, we can feed a WebAudio track). Policy might change or even be moved
from here.
On 2014/08/29 11:25:31, tommi wrote:

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

[no longer working on chromium, 2014/08/29 12:23:06]

Call AddInput here.

[burnik, 2014/08/29 13:26:17]

Same comment regarding the way |ProvideInput()| is called.
On 2014/08/29 12:23:06, xians1 wrote:

+
+  DCHECK_EQ(0, output_params_.frames_per_buffer() *
+      input_params_.sample_rate() % output_params_.sample_rate());
+  fifo_buffer_size_ = output_params_.frames_per_buffer() *
+      input_params_.sample_rate() / output_params_.sample_rate();
+
+  int frames_in_fifo = kNumberOfBuffersInFifo * fifo_buffer_size_;
+
+  max_sync_delay_time_delta_ = base::TimeDelta::FromMilliseconds(
+      input_params_.sample_rate() / 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(
+    blink::WebMediaStreamSource::ReadyState state) {
+  DCHECK(main_render_thread_checker_.CalledOnValidThread());
+  if (state == blink::WebMediaStreamSource::ReadyStateEnded) {
+    track_stopped_ = true;
+  } else {
+    DCHECK(!track_stopped_);
+  }
+}
+
+void SpeechRecognitionAudioSourceProvider::OnData(
+    const int16* audio_data,
+    int sample_rate,
+    int number_of_channels,
+    int number_of_frames) {
+  DCHECK(capture_thread_checker_.CalledOnValidThread());
+  DCHECK_EQ(input_bus_->frames(), number_of_frames);
+  DCHECK_EQ(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);

[no longer working on chromium, 2014/08/29 12:23:06]

I guess these code is workaround to fix AudioConverter calling ProvideInput
twice at the beginning, I don't really think it is a good idea, instead, trying
changing the code a few lines above to:
if (fifo_->frames() <= fifo_buffer_size_)
  return;

And add a comment to explain why you do this.

[burnik, 2014/08/29 13:26:17]

I might try this out and see what happens.
On 2014/08/29 12:23:06, xians1 wrote:

+    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 client before we send
+  // the next buffer.
+  if (buffer_index_ > 0) {
+    uint32 peer_buffer_index;
+    size_t bytes_received = socket_.ReceiveWithTimeout(&peer_buffer_index,

[no longer working on chromium, 2014/08/29 12:23:06]

you have remove this ReceiveWithTimeout call, this OnData() is called on the
real time audio thread, you can't block it. we need to figure out some other way
to do the synchronization.

[burnik, 2014/08/29 13:26:17]

Acknowledged.

+        sizeof(peer_buffer_index), max_sync_delay_time_delta_);
+    // TODO(burnik): This should not happen. Make sure FIFO doesn't fill up.
+    if (bytes_received == 0)

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

should there be a NOTREACHED() in the body of this if statement?

[burnik, 2014/08/29 13:26:16]

Good point. Done. 
And I'm still considering ways to handle these dangerous situations which could
potentially fill up the FIFO.
On 2014/08/29 11:25:30, tommi wrote:

+      return;
+    DCHECK_EQ(peer_buffer_index, buffer_index_ - 1);
+  }
+
+  // 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());
+  // Notify client to consume buffer |buffer_index_| on |output_bus_|.

[no longer working on chromium, 2014/08/29 12:23:06]

empty line.

+  size_t bytes_sent = socket_.Send(&buffer_index_, sizeof(buffer_index_));
+  // The send can fail if the user changes his input audio device

[no longer working on chromium, 2014/08/29 12:23:06]

is it true?

[burnik, 2014/08/29 13:26:17]

As far as I've tested.
On 2014/08/29 12:23:06, xians1 wrote:

+  if (bytes_sent != sizeof(buffer_index_)) {
+    // TODO(burnik): See if we can stop the passing of data if this happens.
+    DLOG(ERROR) << "Missing a buffer";
+  }
+  ++buffer_index_;
+}
+
+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_|

[no longer working on chromium, 2014/08/29 12:23:06]

nit, empty line.

[burnik, 2014/08/29 13:26:17]

Done.

+  fifo_->Consume(audio_bus, 0, audio_bus->frames());
+  return 1.0;
+}
+
+}  // namespace content