Implement SpeechRecognizerImplAndroid

content/browser/speech/speech_recognizer_impl_android.cc

+// Copyright (c) 2013 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/browser/speech/speech_recognizer_impl_android.h"
+
+#include "base/android/jni_android.h"
+#include "base/android/jni_array.h"
+#include "base/android/jni_string.h"
+#include "base/android/scoped_java_ref.h"
+#include "base/bind.h"
+#include "base/utf_string_conversions.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/speech_recognition_event_listener.h"
+#include "content/public/browser/speech_recognition_manager.h"
+#include "content/public/browser/speech_recognition_session_config.h"
+#include "content/public/common/speech_recognition_grammar.h"
+#include "content/public/common/speech_recognition_result.h"
+#include "jni/SpeechRecognition_jni.h"
+
+using base::android::AppendJavaStringArrayToStringVector;
+using base::android::AttachCurrentThread;
+using base::android::GetApplicationContext;
+using base::android::JavaFloatArrayToFloatVector;
+
+namespace content {
+
+SpeechRecognizerImplAndroid::SpeechRecognizerImplAndroid(
+    SpeechRecognitionEventListener* listener,
+    int session_id)
+    : SpeechRecognizer(listener, session_id),
+      state_(STATE_IDLE) {
+}
+
+SpeechRecognizerImplAndroid::~SpeechRecognizerImplAndroid() { }
+
+void SpeechRecognizerImplAndroid::StartRecognition() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  listener()->OnRecognitionStart(session_id());

[janx, 2013/06/10 16:51:19]

Primiano Tucci 2013/06/03 16:24:11
I'd like to have feedback by some other reviewer on this comment.
I think is a bit safer posting the OnRecognitionStart (and similar below), even
if the source and target threads are the same (IO), rather than calling the
callback method directly.
What I fear about calling callbacks synchronously is that you can end up in
unpleasant nested/reentrant call stacks.
For instance, imagine that for some reason the listener (i.e. the S.R. manager
in our case) has a code path that calls AbortRecognition in the
OnRecognitionStart event handler (e.g., in a error/not authorized code path).
In such a case you would end up having a call stack like this:
SRManager.Something()
 SRImplAndroid.StartRecognition()
  SRManager.OnRecognitionStart(...)
   SRImplAndroid.AbortRecognition()
    SRManager.OnRecognitionEnd()

I don't know if this particular example is possible with the current S.R.
manager implementation, but IMHO if some code path will change in future, a
nested call stack like the one depicted above could cause serious headaches.
Conversely, if you post call-backs on the same (IO) thread, you basically will
break any possiblity of nesting/reentrancy (at the price of a little higher
latency).


If you take a look to the current speech_recognizer_impl.cc implementation, all
the listener_->CallbackMethods() are always called inside the FSM, which by
design is always decoupled in a separate message loop task (read: all evolution
functions of the FSM are "posted").

What do you think?

[bulach, 2013/06/11 07:24:48]

if this is asking for my opinion, I'd say go with primiano's :)
if the other implementation is always posting to the callback rather than
synchronously calling it, it'd be better to keep the same behavior on this
platform.

[janx, 2013/06/12 14:47:14]

Converted all

"listener()->OnSomeEvent(session_id())"

to

"BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, base::Bind(
&SpeechRecognitionEventListener::OnSomeEvent,
base::Unretained(listener()),
session_id()))"

to avoid nested/reentrant call stacks.

+  SpeechRecognitionSessionConfig config_ =
+      SpeechRecognitionManager::GetInstance()->GetSessionConfig(session_id());
+  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, base::Bind(
+      &content::SpeechRecognizerImplAndroid::StartRecognitionOnUIThread, this,
+      config_.continuous, config_.interim_results));
+}
+
+void SpeechRecognizerImplAndroid::StartRecognitionOnUIThread(bool continuous,
+    bool interim_results) {

[bulach, 2013/06/11 07:24:48]

nit: either left-align, or keep the previous ( by itself with the first param
idented by 4

[janx, 2013/06/12 14:47:14]

Went for \n after ( and first param indented by 4.

+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  JNIEnv* env = AttachCurrentThread();
+  j_recognition_.Reset(Java_SpeechRecognition_createSpeechRecognition(env,
+      GetApplicationContext(), reinterpret_cast<jint>(this)));
+  Java_SpeechRecognition_startRecognition(env, j_recognition_.obj(), continuous,
+      interim_results);
+}
+
+void SpeechRecognizerImplAndroid::AbortRecognition() {
+  if (BrowserThread::CurrentlyOn(BrowserThread::IO)) {
+    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+        base::Bind(&content::SpeechRecognizerImplAndroid::AbortRecognition,
+                   this));
+    listener()->OnRecognitionError(session_id(),

[bulach, 2013/06/11 07:24:48]

as above, perhaps better to post this task after the java side has been
cancelled below.

[janx, 2013/06/12 14:47:14]

Now handled in the Java side, call removed.

+        SpeechRecognitionError(SPEECH_RECOGNITION_ERROR_ABORTED));
+    return;
+  }
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  JNIEnv* env = AttachCurrentThread();
+  if (!j_recognition_.is_null())
+    Java_SpeechRecognition_abortRecognition(env, j_recognition_.obj());
+}
+
+void SpeechRecognizerImplAndroid::StopAudioCapture() {
+  if (BrowserThread::CurrentlyOn(BrowserThread::IO)) {
+    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+        base::Bind(&content::SpeechRecognizerImplAndroid::StopAudioCapture,
+                   this));
+    return;
+  }
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  JNIEnv* env = AttachCurrentThread();
+  if (!j_recognition_.is_null())
+    Java_SpeechRecognition_stopRecognition(env, j_recognition_.obj());
+}
+
+bool SpeechRecognizerImplAndroid::IsActive() const {
+  return state_ != STATE_IDLE;

[bulach, 2013/06/11 07:24:48]

just confirming: these are always called in the IO thread, right? perhaps a
comment in the .h saying this member is only ever accessed on the IO thread, and
also a DCHECK here and the following method.

[janx, 2013/06/12 14:47:14]

Added "DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO))".

+}
+
+bool SpeechRecognizerImplAndroid::IsCapturingAudio() const {
+  return state_ == STATE_CAPTURING_AUDIO;
+}
+
+void SpeechRecognizerImplAndroid::OnAudioStart(JNIEnv* env, jobject obj) {
+  if (BrowserThread::CurrentlyOn(BrowserThread::UI)) {
+    BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+        base::Bind(&SpeechRecognizerImplAndroid::OnAudioStart, this, env,

[bulach, 2013/06/11 07:24:48]

oh, an important detail: neither env or obj are thread safe, and can't be used
in any other thread but the one that originated from the JNI. thankfully it's
not being used :) however, it'd be clearer by sending NULL for both...
same for all the IO => UI posts below..

[janx, 2013/06/12 14:47:14]

Fixed by sending "static_cast<JNIEnv*>(NULL), static_cast<jobject>(NULL)"
instead of "env, obj".

+                   obj));
+    return;
+  }
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  state_ = STATE_CAPTURING_AUDIO;
+  listener()->OnAudioStart(session_id());
+}
+
+void SpeechRecognizerImplAndroid::OnSoundStart(JNIEnv* env, jobject obj) {
+  if (BrowserThread::CurrentlyOn(BrowserThread::UI)) {
+    BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+        base::Bind(&SpeechRecognizerImplAndroid::OnSoundStart, this, env,
+                   obj));
+    return;
+  }
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  listener()->OnSoundStart(session_id());
+}
+
+void SpeechRecognizerImplAndroid::OnSoundEnd(JNIEnv* env, jobject obj) {
+  if (BrowserThread::CurrentlyOn(BrowserThread::UI)) {
+    BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+        base::Bind(&SpeechRecognizerImplAndroid::OnSoundEnd, this, env, obj));
+    return;
+  }
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  listener()->OnSoundEnd(session_id());
+}
+
+void SpeechRecognizerImplAndroid::OnAudioEnd(JNIEnv* env, jobject obj) {
+  if (BrowserThread::CurrentlyOn(BrowserThread::UI)) {
+    BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+        base::Bind(&SpeechRecognizerImplAndroid::OnAudioEnd, this, env, obj));
+    return;
+  }
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  state_ = STATE_AWAITING_FINAL_RESULT;
+  listener()->OnAudioEnd(session_id());
+}
+
+void SpeechRecognizerImplAndroid::OnRecognitionResults(JNIEnv* env,
+    jobject obj, jobjectArray strings, jfloatArray floats,
+    jboolean provisional) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  std::vector<string16> options;
+  AppendJavaStringArrayToStringVector(env, strings, &options);
+  std::vector<float> scores(options.size(), 0.0);
+  if (floats != NULL)
+    JavaFloatArrayToFloatVector(env, floats, &scores);
+  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+      base::Bind(&SpeechRecognizerImplAndroid::OnRecognitionResultsOnIOThread,
+                 this, options, scores, provisional));
+}
+
+void SpeechRecognizerImplAndroid::OnRecognitionResultsOnIOThread(
+    std::vector<string16> options, std::vector<float> scores,
+    bool provisional) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  SpeechRecognitionResults results;

[bulach, 2013/06/11 07:24:48]

nit: is this copy-constructable? if so, you may want to do this transformation
on 145 above rather than Post both vectors..

[janx, 2013/06/12 14:47:14]

SpeechRecognitionResults is just a std::vector, so it is copy-constructable. I
moved the transformation to the UI thread.

+  results.push_back(SpeechRecognitionResult());
+  SpeechRecognitionResult& result = results.back();

[bulach, 2013/06/11 07:24:48]

CHECK_EQ(options.size(), scores.size());

[janx, 2013/06/12 14:47:14]

Done.

+  for (unsigned int i = 0; i < options.size(); i++) {

[bulach, 2013/06/11 07:24:48]

nit: s/unsigned int/size_t/ and also ++i

[janx, 2013/06/12 14:47:14]

Done.

+    result.hypotheses.push_back(
+        SpeechRecognitionHypothesis(options[i],
+                                    static_cast<double>(scores[i])));
+  }
+  result.is_provisional = provisional;
+  listener()->OnRecognitionResults(session_id(), results);
+}
+
+void SpeechRecognizerImplAndroid::OnRecognitionError(JNIEnv* env,
+    jobject obj, jint error) {
+  if (BrowserThread::CurrentlyOn(BrowserThread::UI)) {
+    BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+        base::Bind(&SpeechRecognizerImplAndroid::OnRecognitionError, this, env,
+                   obj, error));
+    return;
+  }
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  SpeechRecognitionErrorCode code =
+      static_cast<SpeechRecognitionErrorCode>(error);
+  listener()->OnRecognitionError(session_id(), SpeechRecognitionError(code));
+}
+
+void SpeechRecognizerImplAndroid::OnRecognitionEnd(JNIEnv* env,
+    jobject obj) {
+  if (BrowserThread::CurrentlyOn(BrowserThread::UI)) {
+    BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+        base::Bind(&SpeechRecognizerImplAndroid::OnRecognitionEnd, this, env,
+                   obj));
+    return;
+  }
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  state_ = STATE_IDLE;
+  listener()->OnRecognitionEnd(session_id());
+}
+
+// static
+bool SpeechRecognizerImplAndroid::RegisterSpeechRecognizer(JNIEnv* env) {
+  return RegisterNativesImpl(env);
+}
+
+}  // namespace content