Binding media stream audio track to speech recognition [browser]

content/browser/speech/speech_recognizer_impl.cc

Index: content/browser/speech/speech_recognizer_impl.cc
diff --git a/content/browser/speech/speech_recognizer_impl.cc b/content/browser/speech/speech_recognizer_impl.cc
index 8824bf0dd92c52061aa2762457ad89dc22e15c67..24295b50b4830e97210c0e6fd0858311016935c1 100644
--- a/content/browser/speech/speech_recognizer_impl.cc
+++ b/content/browser/speech/speech_recognizer_impl.cc
@@ -6,11 +6,15 @@
 
 #include "base/basictypes.h"
 #include "base/bind.h"
+#include "base/memory/shared_memory.h"
+#include "base/process/process_handle.h"
+#include "base/threading/thread.h"
 #include "base/time/time.h"
 #include "content/browser/browser_main_loop.h"
 #include "content/browser/media/media_internals.h"
 #include "content/browser/speech/audio_buffer.h"
 #include "content/browser/speech/google_one_shot_remote_engine.h"
+#include "content/public/browser/browser_message_filter.h"
 #include "content/public/browser/speech_recognition_event_listener.h"
 #include "media/base/audio_converter.h"
 #include "net/url_request/url_request_context_getter.h"
@@ -21,6 +25,7 @@
 
 using media::AudioBus;
 using media::AudioConverter;
+using media::AudioInputBufferParameters;
 using media::AudioInputController;
 using media::AudioManager;
 using media::AudioParameters;
@@ -41,6 +46,7 @@ class SpeechRecognizerImpl::OnDataConverter
   // Converts input audio |data| bus into an AudioChunk where the input format
   // is given by |input_parameters_| and the output format by
   // |output_parameters_|.
+  // Skips conversion on |audio_converter_| when input and output formats match.
   scoped_refptr<AudioChunk> Convert(const AudioBus* data);
 
  private:
@@ -57,6 +63,7 @@ class SpeechRecognizerImpl::OnDataConverter
   const AudioParameters input_parameters_;
   const AudioParameters output_parameters_;
   bool waiting_for_input_;
+  bool need_conversion_;
   scoped_ptr<uint8[]> converted_data_;
 
   DISALLOW_COPY_AND_ASSIGN(OnDataConverter);
@@ -124,6 +131,7 @@ SpeechRecognizerImpl::OnDataConverter::OnDataConverter(
       waiting_for_input_(false),
       converted_data_(new uint8[output_parameters_.GetBytesPerBuffer()]) {
   audio_converter_.AddInput(this);
+  need_conversion_ = !input_parameters_.Equals(output_parameters_);
 }
 
 SpeechRecognizerImpl::OnDataConverter::~OnDataConverter() {
@@ -136,10 +144,15 @@ scoped_refptr<AudioChunk> SpeechRecognizerImpl::OnDataConverter::Convert(
     const AudioBus* data) {
   CHECK_EQ(data->frames(), input_parameters_.frames_per_buffer());
 
-  data->CopyTo(input_bus_.get());
-
-  waiting_for_input_ = true;
-  audio_converter_.Convert(output_bus_.get());
+  // Do conversion if needed - only when |input_parameters_| differ from
+  // |output_parameters_|.
+  if (need_conversion_) {
+    data->CopyTo(input_bus_.get());
+    waiting_for_input_ = true;
+    audio_converter_.Convert(output_bus_.get());
+  } else {
+    data->CopyTo(output_bus_.get());
+  }
 
   output_bus_->ToInterleaved(
       output_bus_->frames(), output_parameters_.bits_per_sample() / 8,
@@ -175,6 +188,8 @@ SpeechRecognizerImpl::SpeechRecognizerImpl(
     int session_id,
     bool continuous,
     bool provisional_results,
+    bool using_audio_track,
+    base::ProcessHandle peer_process_handle,
     SpeechRecognitionEngine* engine)
     : SpeechRecognizer(listener, session_id),
       recognition_engine_(engine),
@@ -183,6 +198,8 @@ SpeechRecognizerImpl::SpeechRecognizerImpl(
           media::AudioLogFactory::AUDIO_INPUT_CONTROLLER)),
       is_dispatching_event_(false),
       provisional_results_(provisional_results),
+      using_audio_track_(using_audio_track),
+      peer_process_handle_(peer_process_handle),
       state_(STATE_IDLE) {
   DCHECK(recognition_engine_ != NULL);
   if (!continuous) {
@@ -214,7 +231,7 @@ SpeechRecognizerImpl::SpeechRecognizerImpl(
 // synchronous callbacks.
 
 void SpeechRecognizerImpl::StartRecognition(const std::string& device_id) {
-  DCHECK(!device_id.empty());
+  DCHECK(!device_id.empty() ^ using_audio_track_);
   device_id_ = device_id;
 
   BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
@@ -245,7 +262,8 @@ bool SpeechRecognizerImpl::IsCapturingAudio() const {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); // See IsActive().
   const bool is_capturing_audio = state_ >= STATE_STARTING &&
                                   state_ <= STATE_RECOGNIZING;
-  DCHECK((is_capturing_audio && (audio_controller_.get() != NULL)) ||
+  DCHECK((is_capturing_audio &&
+          (using_audio_track_ || (audio_controller_.get() != NULL))) ||
          (!is_capturing_audio && audio_controller_.get() == NULL));
   return is_capturing_audio;
 }
@@ -263,6 +281,12 @@ SpeechRecognizerImpl::~SpeechRecognizerImpl() {
         base::Bind(&KeepAudioControllerRefcountedForDtor, audio_controller_));
     audio_log_->OnClosed(0);
   }
+
+  if (audio_sync_thread_.get()) {
+    base::ThreadRestrictions::SetIOAllowed(true);

[tommi (sloooow) - chröme, 2014/10/02 11:23:19]

can we avoid this synchronous operation by e.g. passing object ownership to a
closure that does this on a thread where blocking IO is allowed?

[burnik, 2014/10/02 16:04:11]

Ok, but what would happen if the object goes away (dtor completes)
before that posted task completes? Would the socket and thread still
shutdown/stop?

Is it good practice to post tasks in the destructor?
Or maybe I misunderstood, do you have an example?

[no longer working on chromium, 2014/10/03 08:57:53]

I think there are two ways doing that:
1) in browser_main_loop.cc, make sure speech_recognition_manager_ gets deleted
before file thread goes away, and post a task to the file thread, which is IO
allowed, there it closes the audio thread.
2)
speech_recognition_manager_ is depending on AudioManager, shuts the thread down
using the audio thread of the AudioManager.
Tommi, do you know if the audio thread of the AudioManager is IO allowed?

[burnik, 2014/10/06 08:32:50]

I've tried a few different approaches to avoid this.
1) Using audio managers task runner.
2) Using the BrowserThread::FILE task runner.

Both approaches produce same or similar issues.

The issue here is that we cannot allow the object(speech_recognizer_impl) to go
away before the thread completes. If we do so, we are freeing the members and
causing segfault in the thread task (crashing the browser with exit code -11 and
no stack trace). That's why this needs to be a synchronous operation.

Next patchset moves this to SpeechRecognitionAudioReceiver and does it more
carefully not to block for too long.

A way out would be I guess having a singleton which could be reused during the
browser process lifetime.

+    audio_sync_socket_->Shutdown();
+    audio_sync_thread_->Stop();
+  }
 }
 
 // Invoked in the audio thread.
@@ -482,7 +506,7 @@ void SpeechRecognizerImpl::ProcessAudioPipeline(const AudioChunk& raw_audio) {
     endpointer_.ProcessAudio(raw_audio, &rms);
 
   if (route_to_vumeter) {
-    DCHECK(route_to_endpointer); // Depends on endpointer due to |rms|.
+    DCHECK(route_to_endpointer);  // Depends on endpointer due to |rms|.
     UpdateSignalAndNoiseLevels(rms, clip_detected);
   }
   if (route_to_sr_engine) {
@@ -529,6 +553,15 @@ SpeechRecognizerImpl::StartRecording(const FSMEventArgs&) {
       AudioParameters::AUDIO_PCM_LOW_LATENCY, kChannelLayout, kAudioSampleRate,
       kNumBitsPerAudioSample, frames_per_buffer);
 
+  // Check if using audio track as source (audio data processed on renderer).
+  // If |StartRecordingUsingAudioTrack()| fails, we abort the recognition.
+  if (using_audio_track_)

[tommi (sloooow) - chröme, 2014/10/02 11:23:19]

{}

[burnik, 2014/10/02 16:04:11]

Done.

+    return StartRecordingUsingAudioTrack(output_parameters)
+               ? STATE_STARTING
+               : Abort(SpeechRecognitionError(SPEECH_RECOGNITION_ERROR_AUDIO));
+
+  // Get audio data from microphone on the browser.
+
   // Audio converter will receive audio based on these parameters as input.
   // On Windows we start by verifying that Core Audio is supported. If not,
   // the WaveIn API is used and we might as well avoid all audio conversations
@@ -585,6 +618,108 @@ SpeechRecognizerImpl::StartRecording(const FSMEventArgs&) {
   return STATE_STARTING;
 }
 
+bool SpeechRecognizerImpl::StartRecordingUsingAudioTrack(
+    const media::AudioParameters& params) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  // Create the shared memory and share with the renderer process.
+  uint32 shared_memory_size = sizeof(AudioInputBufferParameters) +
+                              AudioBus::CalculateMemorySize(params);
+  shared_memory_.reset(new base::SharedMemory());
+  if (!shared_memory_->CreateAndMapAnonymous(shared_memory_size)) {
+    DLOG(ERROR) << "Failed to create shared memory for the speech recognizer";
+    return false;
+  }
+  base::SharedMemoryHandle foreign_memory_handle;

[no longer working on chromium, 2014/10/02 11:55:40]

nit, add an empty line

[burnik, 2014/10/02 16:04:11]

Done.

+  if (!shared_memory_->ShareToProcess(base::GetCurrentProcessHandle(),
+                                      &foreign_memory_handle)) {
+    DLOG(ERROR) << "Failed to map and share the shared memory";
+    return false;

[tommi (sloooow) - chröme, 2014/10/02 11:23:19]

free shared_memory_?

[burnik, 2014/10/02 16:04:11]

Added:
shared_memory_.reset();

+  }
+  media::AudioInputBuffer* buffer =

[no longer working on chromium, 2014/10/02 11:55:40]

nit add an empty line.

[burnik, 2014/10/02 16:04:11]

Done.

+      static_cast<media::AudioInputBuffer*>(shared_memory_->memory());
+  audio_track_bus_ = AudioBus::WrapMemory(params, buffer->audio);
+  // Use |buffer->params.size| from shared memory to synchronize by counting
+  // audio buffers when receiving audio data from the renderer.
+  buffer_index_ = &(buffer->params.size);
+  *buffer_index_ = 0U;
+  // Reusing the audio converter only to create chunks from the bus.

[no longer working on chromium, 2014/10/02 11:55:40]

empty line.

[burnik, 2014/10/02 16:04:11]

Done.

+  // Same input and output format indicate no additional conversion here.
+  audio_converter_.reset(new OnDataConverter(params, params));
+
+  // The endpointer needs to estimate the environment/background noise before
+  // starting to treat the audio as user input. We wait in the state
+  // ESTIMATING_ENVIRONMENT until such interval has elapsed before switching
+  // to user input mode.
+  endpointer_.SetEnvironmentEstimationMode();
+
+  foreign_sync_socket_.reset(new base::CancelableSyncSocket());
+  audio_sync_socket_.reset(new base::CancelableSyncSocket());
+  if (!base::SyncSocket::CreatePair(audio_sync_socket_.get(),

[no longer working on chromium, 2014/10/02 11:55:40]

move these sockets code right after the shared memory code, so we don't create
any resource upon failure.

[burnik, 2014/10/02 16:04:11]

Done.

+                                    foreign_sync_socket_.get())) {
+    DLOG(ERROR) << "Cannot create SyncSocket pair";
+    return false;

[tommi (sloooow) - chröme, 2014/10/02 11:23:19]

free resources hitherto allocated?

same for all other early returns.

[burnik, 2014/10/02 16:04:12]

Done.

+  }
+
+  base::SyncSocket::TransitDescriptor foreign_socket_descriptor;
+  if (!foreign_sync_socket_->PrepareTransitDescriptor(
+          peer_process_handle_, &foreign_socket_descriptor)) {
+    DLOG(ERROR) << "Cannot prepare syncsocket transit descriptor";
+    return false;
+  }
+
+  // The thread check will be first invoked in |OnAudioTrackData()|.
+  audio_sync_thread_checker_.DetachFromThread();
+  audio_sync_thread_.reset(
+      new base::Thread("SpeechRecognitionAudioTrackThread"));
+  audio_sync_thread_->Start();

[no longer working on chromium, 2014/10/02 11:55:40]

why is it called audio_sync_thread_? how about audio_track_thread_ or simply
audio_thread_

[burnik, 2014/10/02 16:04:12]

Done. audio_thread_.

I think keyword "track" should be removed.

+
+  DCHECK_EQ(0U, *buffer_index_);
+  if (!audio_sync_thread_->message_loop_proxy()

[no longer working on chromium, 2014/10/02 11:55:40]

add a DCHECK, like:
bool ret = audio_sync_thread_->message_loop_proxy()->PostTask(
    FROM_HERE,
    base::Bind(&SpeechRecognizerImpl::OnAudioTrackData,
base::Unretained(this)));
DCHECK(ret);

[burnik, 2014/10/02 16:04:11]

This will then be an unused variable in the Relase build.

+           ->PostTask(FROM_HERE,
+                      base::Bind(&SpeechRecognizerImpl::OnAudioTrackData,
+                                 base::Unretained(this)))) {
+    DLOG(ERROR) << "Cannot post audio sync thread task";
+    return false;
+  }
+
+  listener()->OnAudioTrackReady(session_id(), params, foreign_memory_handle,

[no longer working on chromium, 2014/10/02 11:55:40]

OnAudioTrackReady sounds a wrong name, change it to 
OnAudioStreamReady

[burnik, 2014/10/02 16:04:11]

How about OnSharedAudioBusReady or OnSharedBusReady?

Both stream and track are unknowns to this class...

+                                foreign_socket_descriptor);
+
+  return true;

[burnik, 2014/10/02 16:04:12]

Regardin the "bool ret":
I could return ret; here. What do you think?

+}
+
+void SpeechRecognizerImpl::OnAudioTrackData() {
+  DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::IO));
+  DCHECK(audio_sync_thread_checker_.CalledOnValidThread());
+  base::ThreadRestrictions::SetIOAllowed(true);
+
+  // TODO(burnik): We should receive with a timeout here. And when it occurs,
+  // we should stop the recognition assuming there is no more audio data.
+  size_t bytes_received =
+      audio_sync_socket_->Receive(buffer_index_, sizeof(*buffer_index_));

[no longer working on chromium, 2014/10/02 11:55:40]

Receive will block the thread. In case there is no data coming from the renderer
any more, you will wait infinitely here when shutting down the thread.

[burnik, 2014/10/02 16:04:11]

This would indicate the renderer is not sending data but the socket is still
open.
That means the AudioSink is still alive but not sending anything. How??

How does AudioDeviceThread handle this?

Another problem would be that we don't wait enough with the timeout -- for
example when we signal back the renderer to create the AudioSink, if it takes
more time than we wait here, we will fail to receive and the session will fail.
This leads to multiple calls to Receive or Reposting the task, but then it's
unclear when we should stop trying to receive...

[no longer working on chromium, 2014/10/03 09:13:18]

It is better for you do some tests here. Like closing the tab, and print logs
here to see how it behaves.

If the socket gets closed automatically after some time, then we don't need the
timeout.

[burnik, 2014/10/06 08:32:50]

Reimplemented as ReceiveWithTimeout in patchset 2.

+
+  if (bytes_received != sizeof(*buffer_index_)) {
+    DCHECK_EQ(bytes_received, 0U);
+    return;

[no longer working on chromium, 2014/10/02 11:55:40]

I think this is wrong, you silently stop the audio thread here when getting an
error from Receive(), you should either post a task to check it in a later state
or notify the client that the session fails. I prefer the first solution here.

[burnik, 2014/10/02 16:04:11]

Are you sure about this? Can a socket receive again after it hadn't been able to
receive? I believe that a receive with 0 bytes indicates the socket is closed.

Second point would go into direction of stopping the recognition which then IPCs
the renderer. Do you think we should post an abort event? BTW, as I recall, this
occurs when page reloads or goes away.

Third question: Is it possible that a Receive could transfer less than 4B? --
the very reason for the DCHECK here.

[no longer working on chromium, 2014/10/03 08:57:53]

Yes, I can sure that once you return here, the OnAudioTrackData will not be
executed any more. You can test it by adding logs there.
You should test the code to see if the session will be closed when the page
reloads or goes away.
If yes, you don't need to post an abort event. Otherwise you might need to do
it.
Please test it first, then we start the discussion there.
If the renderer sends only 4B, then the receiver should receive only 4B or 0
(renderer socket is closed). Unless the renderer is hacked and sends different
bytes.

[no longer working on chromium, 2014/10/03 09:13:18]

hmm, I actually answer my own question, if the socket should only receive 4B or
0 (socket is shutdown), then doing a return here is correct.

again, test the behaviours, and we can have the discussion after making sure
what happens there.

[burnik, 2014/10/06 08:32:50]

Acknowledged.

+  }
+
+  FSMEventArgs event_args(EVENT_AUDIO_DATA);
+  // Convert audio from native format to fixed format used by WebSpeech.
+  event_args.audio_data = audio_converter_->Convert(audio_track_bus_.get());
+  // Process on browser thread.
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(&SpeechRecognizerImpl::DispatchEvent, this, event_args));
+
+  // Notify host that the buffer was consumed via shared memory.
+  ++(*buffer_index_);
+
+  // Post this task for next round of data.
+  audio_sync_thread_->message_loop_proxy()->PostTask(
+      FROM_HERE, base::Bind(&SpeechRecognizerImpl::OnAudioTrackData,
+                            base::Unretained(this)));
+}
+
 SpeechRecognizerImpl::FSMState
 SpeechRecognizerImpl::StartRecognitionEngine(const FSMEventArgs& event_args) {
   // This is the first audio packet captured, so the recognition engine is
@@ -635,7 +770,7 @@ SpeechRecognizerImpl::StopCaptureAndWaitForResult(const FSMEventArgs&) {
   DCHECK(state_ >= STATE_ESTIMATING_ENVIRONMENT && state_ <= STATE_RECOGNIZING);
 
   DVLOG(1) << "Concluding recognition";
-  CloseAudioControllerAsynchronously();
+  if (!using_audio_track_) CloseAudioControllerAsynchronously();

[tommi (sloooow) - chröme, 2014/10/02 11:23:19]

two lines

[burnik, 2014/10/02 16:04:11]

Done.

   recognition_engine_->AudioChunksEnded();
 
   if (state_ > STATE_WAITING_FOR_SPEECH)
@@ -664,7 +799,7 @@ SpeechRecognizerImpl::AbortWithError(const FSMEventArgs& event_args) {
 
 SpeechRecognizerImpl::FSMState SpeechRecognizerImpl::Abort(
     const SpeechRecognitionError& error) {
-  if (IsCapturingAudio())
+  if (IsCapturingAudio() && !using_audio_track_)
     CloseAudioControllerAsynchronously();
 
   DVLOG(1) << "SpeechRecognizerImpl canceling recognition. ";
@@ -770,11 +905,13 @@ SpeechRecognizerImpl::NotFeasible(const FSMEventArgs& event_args) {
 
 void SpeechRecognizerImpl::CloseAudioControllerAsynchronously() {
   DCHECK(IsCapturingAudio());
+  DCHECK(!using_audio_track_);

[no longer working on chromium, 2014/10/02 11:55:40]

what is the alternative of CloseAudioControllerAsynchronously() when the audio
track is being  used?

[burnik, 2014/10/02 16:04:11]

The audio track is managed by the renderer and the cleanup should be there
regarding audio.

We may clean up the thread and the socket right away but does that mean we
should do the same for other members?

[no longer working on chromium, 2014/10/03 08:57:53]

The audio thread and its relevant resource like shared memory, sockets are
belonging to this SpeechRecognizerImpl, how can the renderer cleans up those
resource?

You need to do the same things as how CloseAudioControllerAsynchronously() does,
and closes all the resource when the session is closed.

[burnik, 2014/10/06 08:32:50]

Acknowledged.

   DVLOG(1) << "SpeechRecognizerImpl closing audio controller.";
   // Issues a Close on the audio controller, passing an empty callback. The only
   // purpose of such callback is to keep the audio controller refcounted until
   // Close has completed (in the audio thread) and automatically destroy it
   // afterwards (upon return from OnAudioClosed).
+
   audio_controller_->Close(base::Bind(&SpeechRecognizerImpl::OnAudioClosed,
                                       this, audio_controller_));
   audio_controller_ = NULL;  // The controller is still refcounted by Bind.