Binding media stream audio track to speech recognition [renderer]

content/renderer/speech_recognition_audio_source_provider.h

Index: content/renderer/speech_recognition_audio_source_provider.h
diff --git a/content/renderer/speech_recognition_audio_source_provider.h b/content/renderer/speech_recognition_audio_source_provider.h
new file mode 100644
index 0000000000000000000000000000000000000000..375d6e09a61d5778886d6f2713428205c83a083a
--- /dev/null
+++ b/content/renderer/speech_recognition_audio_source_provider.h
@@ -0,0 +1,141 @@
+// 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.
+
+#ifndef CONTENT_RENDERER_SPEECH_RECOGNITION_AUDIO_SOURCE_PROVIDER_H_
+#define CONTENT_RENDERER_SPEECH_RECOGNITION_AUDIO_SOURCE_PROVIDER_H_
+
+#include "base/callback.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/shared_memory.h"
+#include "base/sync_socket.h"
+#include "base/threading/thread_checker.h"
+#include "base/threading/thread_restrictions.h"
+#include "content/common/content_export.h"
+#include "content/public/renderer/media_stream_audio_sink.h"
+#include "content/renderer/media/media_stream_audio_source.h"
+#include "media/audio/audio_device_thread.h"
+#include "media/audio/audio_parameters.h"
+#include "media/base/audio_converter.h"
+#include "third_party/WebKit/public/platform/WebMediaStreamTrack.h"
+
+namespace media {
+class AudioBus;
+class AudioConverter;
+class AudioFifo;
+class AudioParameters;
+}
+
+namespace content {
+
+// SpeechRecognitionAudioSourceProvider works as a sink to the
+// WebRtcLocalAudioTrack and stores the capture data to a FIFO.

[henrika (OOO until Aug 14), 2014/09/12 12:27:46]

I would say, "and stores the captured data in a FIFO". And "When there is enough
data in the FIFO..." and perhaps also explain what is meant by enough.

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

Comment expanded.

On 2014/09/12 12:27:46, henrika wrote:

+// When the FIFO has enough buffer, it passes the buffer to
+// the speech recognizer.
+//
+class CONTENT_EXPORT SpeechRecognitionAudioSourceProvider
+    : NON_EXPORTED_BASE(public media::AudioConverter::InputCallback),
+      NON_EXPORTED_BASE(public MediaStreamAudioSink) {
+ public:
+  // Used for notifying the renderer client there is an issue with

[henrika (OOO until Aug 14), 2014/09/12 12:27:46]

"...if/when there is an issue"

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

Removed enum.
On 2014/09/12 12:27:46, henrika wrote:

+  // delivering audio frames to the endpoint.
+  enum ErrorState {
+    // Indicates a notification send failed. Recoverable.

[henrika (OOO until Aug 14), 2014/09/12 12:27:47]

"Indicates that sending a notification failed"

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

Ditto.
On 2014/09/12 12:27:47, henrika wrote:

+    SEND_FAILED = 0,
+    // Indicates client hasn't consumed last buffer. Recoverable.

[henrika (OOO until Aug 14), 2014/09/12 12:27:47]

"indicates client" feels wrong; can you rewrite? What about: "Indicates that a
client..."?

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

Ditto.
On 2014/09/12 12:27:47, henrika wrote:

+    BUFFER_SYNC_LAG,
+    // Indiciates serious error. E.g. a disconnect on socket. Non-recoverable.
+    AUDIO_FIFO_OVERFLOW,

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

I am not sure if there is any value to most of these error codes here? what the
client is supposed to do when getting errors like SEND_FAILED, BUFFER_SYNC_LAG
and AUIDO_FIFO_OVERFLOW?

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

Logged instead. Only stop propagated via callback.
On 2014/09/15 08:31:29, xians1 wrote:

+    // Indicates the audio track has stopped. Provider can then be destroyed.

[henrika (OOO until Aug 14), 2014/09/12 12:27:46]

"..that the audio track..."

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

Ditto.
On 2014/09/12 12:27:46, henrika wrote:

+    TRACK_STOPPED
+  };
+
+  typedef base::Callback<void(ErrorState)> OnErrorCB;
+
+  SpeechRecognitionAudioSourceProvider(const blink::WebMediaStreamTrack& track,
+                                       const media::AudioParameters& params,
+                                       const base::SharedMemoryHandle memory,
+                                       base::SyncSocket* socket,
+                                       OnErrorCB on_error_cb);
+
+  virtual ~SpeechRecognitionAudioSourceProvider();
+
+  // Determines the policy on what types of tracks are allowed.

[henrika (OOO until Aug 14), 2014/09/12 12:27:47]

Is this correct. How can a method which returns true or false determine
anything? It is a plain getter, right?

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

Good point. We determine. Implementation enforces. And is used outside via the
client.
On 2014/09/12 12:27:47, henrika wrote:

+  static bool IsAllowedAudioTrack(const blink::WebMediaStreamTrack& track);
+
+ protected:
+  // MediaStreamAudioSink implementation.

[henrika (OOO until Aug 14), 2014/09/12 12:27:46]

No namespace here?

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

It's |content| as well. But added.
On 2014/09/12 12:27:46, henrika wrote:

+  virtual void OnReadyStateChanged(
+      blink::WebMediaStreamSource::ReadyState state) OVERRIDE;
+
+  virtual void OnData(const int16* audio_data, int sample_rate,
+                      int number_of_channels, int number_of_frames) OVERRIDE;
+  virtual void OnSetFormat(const media::AudioParameters& params) OVERRIDE;
+
+  // media::AudioConverter::Inputcallback implementation.
+  // This function is triggered by provideInput() on the WebAudio audio thread,
+  // so it has been under the protection of |lock_|.

[henrika (OOO until Aug 14), 2014/09/12 12:27:46]

"so it has been under..." sounds odd to me. Do you mean "..and a lock is
therefore added to make the method thread safe"?

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

Comment deprecated.
On 2014/09/12 12:27:46, henrika wrote:

+  virtual double ProvideInput(media::AudioBus* audio_bus,
+                              base::TimeDelta buffer_delay) OVERRIDE;
+
+  // Notifies client there is an issue with delivering frames.

[henrika (OOO until Aug 14), 2014/09/12 12:27:46]

"Notifies client there is" does not sound correct. I would say "Notifies the
client when there is an issue..."

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

Removed from design.
On 2014/09/12 12:27:46, henrika wrote:

+  // TODO(burnik): Runs on capture thread. Should run on main renderer thread!

[henrika (OOO until Aug 14), 2014/09/12 12:27:47]

This TODO needs a corresponding crbug.

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

TODO was for before landing. Removed from design.
On 2014/09/12 12:27:47, henrika wrote:

+  void NotifyErrorState(ErrorState error);
+
+ private:
+  // Number of frames per buffer in FIFO. When the buffer is full we convert and
+  // consume it on the |output_bus_|. Size of the buffer is depends on the

[henrika (OOO until Aug 14), 2014/09/12 12:27:46]

"is depends"??

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

Done.

+  // resampler. Example: for 44.1 to 16.0 conversion, it should be 4100 frames.
+  int fifo_buffer_size_;
+
+  // Used to DCHECK that some methods are called on the main render thread.
+  base::ThreadChecker main_render_thread_checker_;
+
+  // Used to DCHECK that some methods are called on the capture audio thread.
+  base::ThreadChecker capture_thread_checker_;
+
+  // The audio track that this source provider is connected to.
+  const blink::WebMediaStreamTrack track_;
+
+  // Shared memory used by audio buses on both browser and renderer processes.
+  base::SharedMemory shared_memory_;
+
+  // Socket for syncronization of audio bus reads/writes.
+  base::SyncSocket* socket_;

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

why the socket_ is raw pointer? who owns it?

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

Client owns it (renderer - dispatcher). This is for dependency injection as
well.
On 2014/09/15 08:31:29, xians1 wrote:

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

I looked at the dispatcher code, it has:
audio_source_provider_.reset(new SpeechRecognitionAudioSourceProvider(
     audio_track_, params, memory, socket.release(), ...

Which clearly passes the ownership to the audio_source_provider_. The socket is
leaking here.

Also, I don't think the dispatcher should own the socket at all.

+
+  // Used as a resampler to deliver appropriate format to speech recognition.
+  scoped_ptr<media::AudioConverter> audio_converter_;
+
+  // FIFO is used for queueing audio frames before we resample.
+  scoped_ptr<media::AudioFifo> fifo_;
+
+  // Audio delivered from source.
+  scoped_ptr<media::AudioBus> input_bus_;
+
+  // Audio bus shared with the browser process via |shared_memory_|.
+  scoped_ptr<media::AudioBus> output_bus_;
+
+  // Params of the source audio. Can change when |OnSetFormat| occurs.
+  media::AudioParameters input_params_;
+
+  // Params used by speech recognition.
+  const media::AudioParameters output_params_;
+
+  // Whether the track has been stopped on the input.

[henrika (OOO until Aug 14), 2014/09/12 12:27:47]

"stopped on the input"??

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

Done.

+  bool track_stopped_;
+
+  // Local counter of audio buffers for synchronization on consumed buffers.

[henrika (OOO until Aug 14), 2014/09/12 12:27:47]

Isn't "of consumed" better? 

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

Looks excessive actually. Removed.
On 2014/09/12 12:27:47, henrika wrote:

+  uint32 buffer_index_;
+
+  // Peer's counter of audio buffers for synchronization on consumed buffers.
+  const uint32* peer_buffer_index_;
+
+  // Callback notifying an error has occured.

[henrika (OOO until Aug 14), 2014/09/12 12:27:47]

.."notifying that an...", or "Callback which is activated when..."

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

Removed from design. Replaced by OnStoppedCB and commented.
On 2014/09/12 12:27:47, henrika wrote:

+  const OnErrorCB on_error_cb_;
+
+  DISALLOW_COPY_AND_ASSIGN(SpeechRecognitionAudioSourceProvider);
+};
+
+}  // namespace content
+
+#endif  // CONTENT_RENDERER_SPEECH_RECOGNITION_AUDIO_SOURCE_PROVIDER_H_