Add OpenSL configurations

media/audio/android/opensles_output.cc

Index: media/audio/android/opensles_output.cc
===================================================================
--- media/audio/android/opensles_output.cc	(revision 189519)
+++ media/audio/android/opensles_output.cc	(working copy)
@@ -79,7 +79,6 @@
 
   // Start streaming data by setting the play state to |SL_PLAYSTATE_PLAYING|.
   SLresult err = (*player_)->SetPlayState(player_, SL_PLAYSTATE_PLAYING);
-  DCHECK_EQ(SL_RESULT_SUCCESS, err);
   if (SL_RESULT_SUCCESS != err) {
     DLOG(WARNING) << "SetPlayState() failed to start playing";

[Ami GONE FROM CHROMIUM, 2013/03/22 16:24:41]

Why is this a WARNING when the rest of the failure logs are ERROR?

   }
@@ -141,24 +140,27 @@
   };
   SLresult err = slCreateEngine(engine_object_.Receive(), 1, option, 0,
                                 NULL, NULL);
-  DCHECK_EQ(SL_RESULT_SUCCESS, err);
-  if (SL_RESULT_SUCCESS != err)
+  if (SL_RESULT_SUCCESS != err) {
+    DLOG(ERROR) << "CreatePlayer slCreateEngine: " << err;

[Ami GONE FROM CHROMIUM, 2013/03/22 16:24:41]

When there is this much duplicated code, it's usually a good idea to do
something about it.  One idea is to do something like:
#define LOG_ON_FAILURE_AND_RETURN(op, retvalue) \
    do { \
      SLresult err = (op); \
      if (err != SL_RESULT_SUCCESS) { \
        DLOG(ERROR) << #op << " failed: " << err; \
        return retvalue; \
      } \
    } while (0)

Then callsites can replace stanzas like:

err = slCreateEngine(engine_object_.Receive(), 1, option, 0, NULL, NULL);
if (SL_RESULT_SUCCESS != err) {
  DLOG(ERROR) << "CreatePlayer slCreateEngine: " << err;
  return false;
}

with:

LOG_ON_FAILURE_AND_RETURN(slCreateEngine(engine_object_.Receive(), 1, option, 0,
NULL, NULL), false);

The benefits of doing this are:
- consistency in error handling (don't get some as WARNINGs others as ERRORs)
- less clutter makes code more readable - what's actually being done is easier
to see when it is not obscured by log statements.

I don't really care if this is done in this CL or not, but since you're touching
all these callsites it seems like an easy choice.

[leozwang1, 2013/03/23 07:00:39]

Done.

     return false;
+  }
 
   // Realize the SL engine object in synchronous mode.
   err = engine_object_->Realize(engine_object_.Get(), SL_BOOLEAN_FALSE);
-  DCHECK_EQ(SL_RESULT_SUCCESS, err);
-  if (SL_RESULT_SUCCESS != err)
+  if (SL_RESULT_SUCCESS != err) {
+    DLOG(ERROR) << "CreatePlayer Realize: " << err;
     return false;
+  }
 
   // Get the SL engine interface which is implicit.
   SLEngineItf engine;
   err = engine_object_->GetInterface(engine_object_.Get(),
                                      SL_IID_ENGINE,
                                      &engine);
-  DCHECK_EQ(SL_RESULT_SUCCESS, err);
-  if (SL_RESULT_SUCCESS != err)
+  if (SL_RESULT_SUCCESS != err) {
+    DLOG(ERROR) << "GetInterface(SL_IID_ENGINE): " << err;

[Ami GONE FROM CHROMIUM, 2013/03/22 16:24:41]

These log messages are pretty inconsistent; some have CreatePlayer in them, some
don't.
Again, a macro would help with this too (as you wouldn't have to hand-craft the
log message).

[leozwang1, 2013/03/23 07:00:39]

Done.

     return false;
+  }
 
   // Create ouput mixer object to be used by the player.
   // TODO(xians): Do we need the environmental reverb auxiliary effect?
@@ -167,15 +169,17 @@
                                    0,
                                    NULL,
                                    NULL);
-  DCHECK_EQ(SL_RESULT_SUCCESS, err);
-  if (SL_RESULT_SUCCESS != err)
+  if (SL_RESULT_SUCCESS != err) {
+    DLOG(ERROR) << "CreateOutputMix: " << err;
     return false;
+  }
 
   // Realizing the output mix object in synchronous mode.
   err = output_mixer_->Realize(output_mixer_.Get(), SL_BOOLEAN_FALSE);
-  DCHECK_EQ(SL_RESULT_SUCCESS, err);
-  if (SL_RESULT_SUCCESS != err)
+  if (SL_RESULT_SUCCESS != err) {
+    DLOG(ERROR) << "mixer Realize: " << err;
     return false;
+  }
 
   // Audio source configuration.
   SLDataLocator_AndroidSimpleBufferQueue simple_buffer_queue = {
@@ -191,11 +195,15 @@
   SLDataSink audio_sink = { &locator_output_mix, NULL };
 
   // Create an audio player.
-  const SLuint32 number_of_interfaces = 1;
-  const SLInterfaceID interface_id[number_of_interfaces] = {
-    SL_IID_BUFFERQUEUE
+  const SLInterfaceID interface_id[] = {
+    SL_IID_BUFFERQUEUE,
+    SL_IID_VOLUME,
+    SL_IID_ANDROIDCONFIGURATION
   };
+  const SLuint32 number_of_interfaces = arraysize(interface_id);

[Ami GONE FROM CHROMIUM, 2013/03/22 16:24:41]

inline into call below.

   const SLboolean interface_required[number_of_interfaces] = {

[Ami GONE FROM CHROMIUM, 2013/03/22 16:24:41]

drop number_of_interfaces

[leozwang1, 2013/03/22 21:37:09]

Done.

+    SL_BOOLEAN_TRUE,
+    SL_BOOLEAN_TRUE,
     SL_BOOLEAN_TRUE
   };
   err = (*engine)->CreateAudioPlayer(engine,
@@ -205,15 +213,32 @@
                                      number_of_interfaces,
                                      interface_id,
                                      interface_required);
-  DCHECK_EQ(SL_RESULT_SUCCESS, err);
   if (SL_RESULT_SUCCESS != err) {
     DLOG(ERROR) << "CreateAudioPlayer() failed with error code " << err;
     return false;
   }
 
+  // Create AudioPlayer and specify SL_IID_ANDROIDCONFIGURATION.
+  SLAndroidConfigurationItf player_config;
+  err = player_object_->GetInterface(player_object_.Get(),
+                                     SL_IID_ANDROIDCONFIGURATION,
+                                     &player_config);
+  if (SL_RESULT_SUCCESS != err) {
+    DLOG(ERROR) << "GetInterface(SL_IID_ANDROIDCONFIGURATION): " << err;
+    return false;
+  }
+
+  SLint32 stream_type = SL_ANDROID_STREAM_VOICE;

[Ami GONE FROM CHROMIUM, 2013/03/22 16:24:41]

this variable is obscuring what the call below actually does.  please inline the
value into the call.

[Ami GONE FROM CHROMIUM, 2013/03/22 16:24:41]

This seems to be the heart of the potential problem with this CL (and I wish
you'd called it out explicitly as such).

If there is a difference in the system implementation between VOICE and MUSIC
then it is likely to be a bad idea to do this unconditionally and apply VOICE to
<audio> generally.

What is the audible effect of commenting out l.231-238 vs. not?
What is the audible effect of using VOICE vs. MUSIC for webrtc?
What is the audible effect of using VOICE vs. MUSIC for go/ocnyf?

What this comes down to is that I don't know enough about the implementations of
this feature to make a call in the abstract.
If you find someone who does, that's great.
If you can experimentally show that VOICE is always better for MUSIC for our
usecases, then great.

[leozwang1, 2013/03/22 21:37:09]

It's my concern too, we're going to do some tests. 
The current design of input/output doesn't consider android use case, we may
need to change it in future. Basically on android, audioplayer and audiorecor
need to what type of stream they're going to play. Any comments on it? 

[Ami GONE FROM CHROMIUM, 2013/03/22 22:29:55]

I don't think you'd want to make the stream contain its purpose.  Possibly the
best move here would be to expose a global state button (mirroring the
android-side SDK surface, which is a global state button) to set/unset voice
mode.  The real question is when would you push/unpush the button.  I suspect
that would have to be driven by webrtc code sensing activation/deactivation of a
call.

[leozwang1, 2013/03/22 22:44:48]

can you teach me how this "global state button (mirroring the android-side SDK
surface, which is a global state button)" work? Is there sample code or doc?

[Ami GONE FROM CHROMIUM, 2013/03/22 23:34:20]

I just mean that the toggle of what global mode to use shouldn't be tied to a
stream.  Instead it should be a method on AudioManager that renderers can call
via IPC.

+  err = (*player_config)->SetConfiguration(player_config,
+                                           SL_ANDROID_KEY_STREAM_TYPE,
+                                           &stream_type, sizeof(SLint32));
+  if (SL_RESULT_SUCCESS != err) {
+    DLOG(ERROR) << "SetConfiguration(SL_ANDROID_STREAM_VOICE): " << err;
+    return false;
+  }
+
   // Realize the player object in synchronous mode.
   err = player_object_->Realize(player_object_.Get(), SL_BOOLEAN_FALSE);
-  DCHECK_EQ(SL_RESULT_SUCCESS, err);
   if (SL_RESULT_SUCCESS != err) {
     DLOG(ERROR) << "Player Realize() failed with error code " << err;
     return false;
@@ -222,26 +247,31 @@
   // Get an implicit player interface.
   err = player_object_->GetInterface(
       player_object_.Get(), SL_IID_PLAY, &player_);
-  DCHECK_EQ(SL_RESULT_SUCCESS, err);
-  if (SL_RESULT_SUCCESS != err)
+  if (SL_RESULT_SUCCESS != err) {
+    DLOG(ERROR) << "GetInterface(SL_IID_PLAYN): " << err;
     return false;
+  }
 
   // Get the simple buffer queue interface.
   err = player_object_->GetInterface(player_object_.Get(),
                                      SL_IID_BUFFERQUEUE,
                                      &simple_buffer_queue_);
-  DCHECK_EQ(SL_RESULT_SUCCESS, err);
-  if (SL_RESULT_SUCCESS != err)
+  if (SL_RESULT_SUCCESS != err) {
+    DLOG(ERROR) << "GetInterface(SL_IID_BUFFERQUEUE): " << err;
     return false;
+  }
 
   // Register the input callback for the simple buffer queue.
   // This callback will be called when the soundcard needs data.
   err = (*simple_buffer_queue_)->RegisterCallback(simple_buffer_queue_,
                                                   SimpleBufferQueueCallback,
                                                   this);
-  DCHECK_EQ(SL_RESULT_SUCCESS, err);
+  if (SL_RESULT_SUCCESS != err) {
+    DLOG(ERROR) << "AudioPlayer RegisterCallback: " << err;
+    return false;
+  }
 
-  return (SL_RESULT_SUCCESS == err);
+  return true;
 }
 
 void OpenSLESOutputStream::SimpleBufferQueueCallback(