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/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 {
+
+void SpeechRecognizerImplAndroid::Init(JNIEnv* env) {
+  RegisterNativesImpl(env);
+}
+
+SpeechRecognizerImplAndroid::SpeechRecognizerImplAndroid(
+    SpeechRecognitionEventListener* listener,
+    const SpeechRecognitionSessionConfig& config,
+    int session_id)
+    : SpeechRecognizer(listener, session_id),
+      config_(config) {
+}
+
+SpeechRecognizerImplAndroid::~SpeechRecognizerImplAndroid() { }
+
+void SpeechRecognizerImplAndroid::StartRecognition() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  listener_->OnRecognitionStart(session_id_);

[Primiano Tucci (use gerrit), 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?

+  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+      base::Bind(&content::SpeechRecognizerImplAndroid::StartRecognitionJNI,
+                 this, config_.continuous, config_.interim_results));
+}
+void SpeechRecognizerImplAndroid::StartRecognitionJNI(bool continuous,

[Primiano Tucci (use gerrit), 2013/06/03 16:24:11]

Nit: \n

[janx, 2013/06/05 13:51:00]

I was collating coupled functions together, but since that is not a standard
thing I will add empty lines everywhere.

+                                                      bool partial_results) {

[Primiano Tucci (use gerrit), 2013/06/03 16:24:11]

Why not keeping the interim_results name?
I understand that in the Android world our "interim" results are called
"partial", but I'd prefer to not see this name translation here, while we are
still in the chromium/blink world :)

[janx, 2013/06/05 13:51:00]

Let's go with "interim" then.

+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  j_recognition_.Reset(Java_SpeechRecognition_createSpeechRecognition(
+      AttachCurrentThread(),
+      GetApplicationContext(),
+      reinterpret_cast<jlong>(this)));

[Primiano Tucci (use gerrit), 2013/06/03 16:24:11]

I am definitely not a JNI expert, so I'd like someone else taking a look at
this: is this the right way to pass the "this" reference for receiving callback
events? Searching in cs.chromium.org for "reinterpret_cast<jlong>(this)" returns
just one result, which is not really encouraging :)
Maybe s/jlong/jint/?

[janx, 2013/06/05 13:51:00]

Fixed both occurences (this CL and http://crrev.com/15793008/).

+  Java_SpeechRecognition_StartRecognition(AttachCurrentThread(),
+      j_recognition_.obj(), continuous, partial_results);
+}
+
+void SpeechRecognizerImplAndroid::AbortRecognition() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+      base::Bind(&content::SpeechRecognizerImplAndroid::CancelRecognitionJNI,
+                 this));
+  OnRecognitionError(SpeechRecognitionError(SPEECH_RECOGNITION_ERROR_ABORTED));
+  listener_->OnRecognitionEnd(session_id_);

[Primiano Tucci (use gerrit), 2013/06/03 16:24:11]

Keep in mind that OnRecognitionEnd is the final event that causes the manager to
free the recognizer(_impl) in the following code path: OnRecognitionEnd(...) >
DispatchEvent(EVENT_RECOGNITION_EVENT) > SessionDelete(...).
Therefore, are you sure that, after this call, the JNI object has been cleaned
and will never ever issue any further callbacks? 
After an offline discussion seems that this call should not be performed here,
rather you should have a further JNI event (see my other comment on the .h file)
and move the OnRecognitionEnd call to such method.

In order to be 100% safe, I think (but I've very little JNI knowledge) that just
before calling listener_->OnRecognitionEnd, you should call a JNI method on your
SpeechRecognition.java class that cleans up the native pointer
(mNativeSpeechRecognizerImplAndroid).
Or, even better, you should have only a single call site in which you call
listener_->OnRecognitionEnd (the OnRecognitionEndJNI seems the best candidate),
and you should "promise" that when the  OnRecognitionEndJNI method is called,
the Java object has released the mNativeSpeechRecognizerImplAndroid pointer, so
that no further events can come after.

+}
+void SpeechRecognizerImplAndroid::CancelRecognitionJNI() {

[Primiano Tucci (use gerrit), 2013/06/03 16:24:11]

Similarly to my previous comment (on partial_results). The name of the Java
methods (specifically, the Android SpeechRecognizer's API) going to be called
should not be perceived here.

[Primiano Tucci (use gerrit), 2013/06/03 16:24:11]

Nit: \n (here and similarly below)

[janx, 2013/06/05 13:51:00]

Done.

+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  Java_SpeechRecognition_CancelRecognition(AttachCurrentThread(),
+                                           j_recognition_.obj());

[Primiano Tucci (use gerrit), 2013/06/03 16:24:11]

At this point we must be sure that the JNI object has been "killed" and won't
deliver any more callback. The reason is that as soon as this method returns,
the SpeechRecognizerAndroidImpl object (i.e.: this) will be freed.

+}
+
+void SpeechRecognizerImplAndroid::StopAudioCapture() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+      base::Bind(&content::SpeechRecognizerImplAndroid::StopRecognitionJNI,
+                 this));
+  listener_->OnRecognitionEnd(session_id_);

[Primiano Tucci (use gerrit), 2013/06/03 16:24:11]

Again: OnRecognitionEnd will cause the manager to destroy the
SpeechRecognizerImplAndroid object.
The sense of StopAudioCapture, instead, is just stopping audio but allowing last
results to come, and deliver further OnResult events.
Therefore the OnRecognitionEnd call here seems a bit premature.
 

+}
+void SpeechRecognizerImplAndroid::StopRecognitionJNI() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  Java_SpeechRecognition_StopRecognition(AttachCurrentThread(),
+                                         j_recognition_.obj());
+}
+
+bool SpeechRecognizerImplAndroid::IsActive() const {
+  return false; // FIXME

[janx, 2013/05/31 17:22:24]

Recognition seems to work when IsActive() and IsCapturingAudio() always return
false. Those methods are called by SpeechRecognitionManagerImpl to control a
state machine. Primiano thinks that this state machine is probably not useful
anymore, since its goal was to handle an edge case when a user clicked outside
of a UI bubble while recognition was almost finished.

Should we try to return sensible values for IsActive() and IsCapturingAudio(),
or should we remove those methods?

[Primiano Tucci (use gerrit), 2013/06/03 16:24:11]

Hmm, actually I think you should implement this. I know that it seems to work
keeping it stuck at false, but it can cause some odd behaviors.
For instance, it will cause the manager to restart the recognition if two
subsequent StartRecognition() calls are performed on the same session id. The
expected behavior, instead, is that the second start() call is ignored.
Second side effect: It seems that the primary_session_id_ is never reset in the
manager, basically because without implementing properly this (and the
IsCapturingAudio) method, the manager will think that the recognition never
begun.

P.S. In the immediate future we'll need all this cases to be covered by an
extensive unittest.

[janx, 2013/06/05 13:51:00]

Thanks for your input, I'll go ahead and reimplement IsActive() and
IsCapturingAudio() correctly, using a state_ member variable.

+}
+
+bool SpeechRecognizerImplAndroid::IsCapturingAudio() const {
+  return false; // FIXME

[janx, 2013/05/31 17:22:24]

ditto

[Primiano Tucci (use gerrit), 2013/06/03 16:24:11]

Ditto :)

Btw, you should be able to implement these two methods (this and IsActive) just
using two booleans (or even better a single enum), which are simply updated in
the Start/Stop/Abort methods.

[janx, 2013/06/05 13:51:00]

Done.

+}
+
+void SpeechRecognizerImplAndroid::OnAudioStartJNI(JNIEnv* env, jobject obj) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+      base::Bind(&SpeechRecognizerImplAndroid::OnAudioStart, this));
+}
+void SpeechRecognizerImplAndroid::OnAudioStart() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  listener_->OnAudioStart(session_id_);
+}
+
+void SpeechRecognizerImplAndroid::OnSoundStartJNI(JNIEnv* env, jobject obj) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+      base::Bind(&SpeechRecognizerImplAndroid::OnSoundStart, this));
+}
+void SpeechRecognizerImplAndroid::OnSoundStart() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  listener_->OnSoundStart(session_id_);
+}
+
+void SpeechRecognizerImplAndroid::OnSoundEndJNI(JNIEnv* env, jobject obj) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+      base::Bind(&SpeechRecognizerImplAndroid::OnSoundEnd, this));
+}
+void SpeechRecognizerImplAndroid::OnSoundEnd() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  listener_->OnSoundEnd(session_id_);
+  // Since Android's speech API doesn't have an AudioEnd event, we emulate it.

[Primiano Tucci (use gerrit), 2013/06/03 16:24:11]

It's ok emulating the event but not here. The same principle (see my other
comments) applies here about encapsulating the Android's API.
Please do it in the .java file (so introduce an OnAudioEndJNI method here).

[janx, 2013/06/05 13:51:00]

Ok, I'll emulate the AudioEnd event in the Java file instead.

+  listener_->OnAudioEnd(session_id_);
+}
+
+void SpeechRecognizerImplAndroid::OnRecognitionResultsJNI(JNIEnv* env,
+    jobject obj, jobjectArray strings, jfloatArray floats,
+    jboolean partial) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  std::vector<string16> options;
+  AppendJavaStringArrayToStringVector(env, strings, &options);
+  std::vector<float> scores;
+  JavaFloatArrayToFloatVector(env, floats, &scores);
+  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+      base::Bind(&SpeechRecognizerImplAndroid::OnRecognitionResults,
+                 this, options, scores, partial));
+}
+void SpeechRecognizerImplAndroid::OnRecognitionResults(
+    std::vector<string16> options, std::vector<float> scores,
+    bool partial) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  SpeechRecognitionResults results;
+  results.push_back(SpeechRecognitionResult());
+  SpeechRecognitionResult& result = results.back();
+  for (unsigned int i = 0; i < options.size(); i++) {
+    result.hypotheses.push_back(
+        SpeechRecognitionHypothesis(options[i],
+                                    static_cast<double>(scores[i])));
+  }
+  result.is_provisional = partial;
+  listener_->OnRecognitionResults(session_id_, results);
+}
+
+void SpeechRecognizerImplAndroid::OnRecognitionErrorJNI(JNIEnv* env,
+    jobject obj, jint error) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  SpeechRecognitionErrorCode code = SPEECH_RECOGNITION_ERROR_NONE;

[Primiano Tucci (use gerrit), 2013/06/03 16:24:11]

Is it actually possible to get another error code here?
Perhaps it make more sense to add a default case in the switch like this:

default:
  NOTREACHED();
  return;

[janx, 2013/06/05 13:51:00]

SPEECH_RECOGNITION_ERROR_NONE is a hack and should not be used, so "default:
NOTREACHED()" is a great idea!

+
+  // Translate Android speech recognition errors to Web Speech API errors.

[Primiano Tucci (use gerrit), 2013/06/03 16:24:11]

Can we move this translation in the .java file ?
If I understand correctly these integers here are the error codes defined by
Android's Speech recognition API. However, in this file there is no trace (as it
should be) about the fact that we are using Android's S.R. API.

+  switch(error) {
+    case 3: // ERROR_AUDIO
+      code = SPEECH_RECOGNITION_ERROR_AUDIO;
+      break;
+    case 5: // ERROR_CLIENT
+      code = SPEECH_RECOGNITION_ERROR_ABORTED;
+      break;
+    case 8: // ERROR_RECOGNIZER_BUSY
+    case 9: // ERROR_INSUFFICIENT_PERMISSIONS
+      code = SPEECH_RECOGNITION_ERROR_NOT_ALLOWED;
+      break;
+    case 1: // ERROR_NETWORK_TIMEOUT
+    case 2: // ERROR_NETWORK
+    case 4: // ERROR_SERVER
+      code = SPEECH_RECOGNITION_ERROR_NETWORK;
+      break;
+    case 7: // ERROR_NO_MATCH
+      code = SPEECH_RECOGNITION_ERROR_NO_MATCH;
+      break;
+    case 6: // ERROR_SPEECH_TIMEOUT
+      code = SPEECH_RECOGNITION_ERROR_NO_SPEECH;
+      break;
+  }
+
+  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+      base::Bind(&SpeechRecognizerImplAndroid::OnRecognitionError, this,
+                 SpeechRecognitionError(code)));
+}
+void SpeechRecognizerImplAndroid::OnRecognitionError(
+    SpeechRecognitionError error) {
+  if (error.code != SPEECH_RECOGNITION_ERROR_NONE)

[Primiano Tucci (use gerrit), 2013/06/03 16:24:11]

See previous comment, this shouldn't be required.

[janx, 2013/06/05 13:51:00]

Removed.

+    listener_->OnRecognitionError(session_id_, error);
+}
+
+void SpeechRecognizerImplAndroid::OnAudioLevelsChangeJNI(JNIEnv* env,
+    jobject obj, jfloat rms) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+      base::Bind(&SpeechRecognizerImplAndroid::OnAudioLevelsChange, this, rms));
+}
+void SpeechRecognizerImplAndroid::OnAudioLevelsChange(float rms) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  // FIXME set audio_level_ and noise_level from rms

[janx, 2013/05/31 17:22:24]

The audio and noise levels are calculated from the rms on desktop with a lot of
floating point arithmetic. Their only use if for an animation in the speech
bubble which won't be used on Android. Should we remove OnAudioLevelsChange?

[Primiano Tucci (use gerrit), 2013/06/03 16:24:11]

I thinks that you can safely remove completely this and the
OnAudioLevelsChangeJNI method, as, IIRC (and as you mentioned), the  rms levels
are only required for the x-webkit-speech support  (which is not supported by
Chrome for Android), and not for the Web Speech API.

[janx, 2013/06/05 13:51:00]

Agreed, let's completely remove OnAudioLevelsChange{,JNI}.

+  //listener_->OnAudioLevelsChange(session_id_,
+  //                               clip_detected ? 1.0f : audio_level_,
+  //                               noise_level);
+}
+
+}  // namespace content