Let default device in PulseAudio be the system default device

media/audio/pulse/pulse_output.cc

Index: media/audio/pulse/pulse_output.cc
diff --git a/media/audio/pulse/pulse_output.cc b/media/audio/pulse/pulse_output.cc
index 953f9acebfacf10e06b754c2c28b93adb6bd23e8..13cf0d10ad8c10e17a1ee649ff9d468251f60c63 100644
--- a/media/audio/pulse/pulse_output.cc
+++ b/media/audio/pulse/pulse_output.cc
@@ -16,6 +16,15 @@ namespace media {
 using pulse::AutoPulseLock;
 using pulse::WaitForOperationCompletion;
 
+// Helper macro to avoid code spam and string bloat.
+#define RETURN_ON_FAILURE(expression, message) \

[tommi (sloooow) - chröme, 2016/03/16 15:21:47]

we try to avoid macros as much as we can.  Even when we do use them, they
shouldn't hide a return statement.  I'd rather have C++ code inline

[rchtara, 2016/03/19 12:07:29]

Done.

[Henrik Grunell, 2016/03/21 09:44:18]

As background, the macro came along with the code below that was moved from
pulse_util.cc. Agree though we shouldn't use it.

[rchtara, 2016/03/22 16:45:48]

Done.

+  do {                                         \
+    if (!(expression)) {                       \
+      DLOG(ERROR) << message;                  \
+      return false;                            \
+    }                                          \
+  } while (0)
+
 // static, pa_stream_notify_cb
 void PulseAudioOutputStream::StreamNotifyCallback(pa_stream* s, void* p_this) {
   PulseAudioOutputStream* stream = static_cast<PulseAudioOutputStream*>(p_this);
@@ -61,12 +70,74 @@ PulseAudioOutputStream::~PulseAudioOutputStream() {
   DCHECK(!pa_mainloop_);
 }
 
+// static, used by pa_context_get_server_info.
+void PulseAudioOutputStream::UseSystemDefaultOutputDeviceCallback(
+    pa_context* context,
+    const pa_server_info* info,
+    void* user_data) {
+  PulseAudioOutputStream* stream =
+      reinterpret_cast<PulseAudioOutputStream*>(user_data);
+  stream->device_id_ = info->default_sink_name;

[tommi (sloooow) - chröme, 2016/03/16 15:21:47]

if device_id_ was set to "default" will this perhaps change it to be something
else and potentially then cause checks for the default device (assuming we have
those somewhere), fail?

[rchtara, 2016/03/19 12:07:30]

A new attribute of PulseAudioOutputStream called |default_system_device_name_|
is going to be used instead

+  pa_threaded_mainloop* pa_mainloop =
+      static_cast<pa_threaded_mainloop*>(stream->pa_mainloop_);
+  pa_threaded_mainloop_signal(pa_mainloop, 0);
+}
+
+void PulseAudioOutputStream::UseSystemDefaultOutputDevice() {
+  DCHECK(pa_mainloop_);
+  DCHECK(pa_context_);
+  pa_operation* operation = pa_context_get_server_info(
+      pa_context_, PulseAudioOutputStream::UseSystemDefaultOutputDeviceCallback,
+      this);
+  WaitForOperationCompletion(pa_mainloop_, operation);

[tommi (sloooow) - chröme, 2016/03/16 15:21:47]

are we on the audiomanager thread?  If we can avoid waiting synchronously like
this, then that would be preferable.

[rchtara, 2016/03/19 12:07:29]

I'm not sure about that. I don't know how we could do that.

[tommi (sloooow) - chröme, 2016/03/19 13:44:17]

You can see it from where this function is called.  Looking at it now, I see
that it's called from Open() below, that function has a thread check, so you can
deduce that this method is called on that same thread.  Would be good to also
have DCHECK(thread_checker_.CalledOnValidThread()); at the top of this function.

[Henrik Grunell, 2016/03/21 09:44:18]

Add DCHECK here is good. We'd have to wait though since we need the device name
when opening the device. Right now Open() returns success or failure, so we'd
have to change that to being asynchronous.

Maybe we could test measure the time it takes to get the device name to get an
understanding of the size of the cost. Wdyt?

[rchtara, 2016/03/22 16:45:48]

Done.

[rchtara, 2016/03/22 16:56:25]

So, I created a timer to get the duration needed: it was:  
0.000147 s
0.00032 s
0.000908 s

So, It's probably not that slow.

+}
+
 bool PulseAudioOutputStream::Open() {
+  DCHECK(!pa_mainloop_);

[Henrik Grunell, 2016/03/07 11:37:17]

Put the moved code in a new function, InitializeMainloopAndContext(). Add thread
check.

I wonder why the functions CreateOutput/InputStream() are in pulse_util.cc at
all? It's only used by this and the corresponding input function and could be
member functions. I don't think you need to move them now though.

Another interesting note is that in the audio manager we create the input
mainloop and context and hand it to the input class. In the manager, it's also
used for device enumeration, both input and output. Same here, no need to change
that.

[rchtara, 2016/03/08 17:28:16]

Done.

+  DCHECK(!pa_context_);
+  std::string app_name = AudioManager::GetGlobalAppName();
+  pa_mainloop_ = pa_threaded_mainloop_new();
+  RETURN_ON_FAILURE(pa_mainloop_, "Failed to create PulseAudio main loop.");
+
+  pa_mainloop_api* pa_mainloop_api = pa_threaded_mainloop_get_api(pa_mainloop_);
+  pa_context_ = pa_context_new(
+      pa_mainloop_api, app_name.empty() ? "Chromium" : app_name.c_str());
+  RETURN_ON_FAILURE(pa_context_, "Failed to create PulseAudio context.");
+
+  // A state callback must be set before calling pa_threaded_mainloop_lock() or
+  // pa_threaded_mainloop_wait() calls may lead to dead lock.
+  pa_context_set_state_callback(pa_context_, &pulse::ContextStateCallback,
+                                pa_mainloop_);
+
+  // Lock the main loop while setting up the context.  Failure to do so may lead
+  // to crashes as the PulseAudio thread tries to run before things are ready.
+  AutoPulseLock auto_lock(pa_mainloop_);

[Henrik Grunell, 2016/03/07 11:37:17]

Scope the code that should execute under this lock (until after the while loop I
believe), i.e. with { }.

[rchtara, 2016/03/08 17:28:16]

Done.

+
+  RETURN_ON_FAILURE(pa_threaded_mainloop_start(pa_mainloop_) == 0,
+                    "Failed to start PulseAudio main loop.");
+  RETURN_ON_FAILURE(
+      pa_context_connect(pa_context_, NULL, PA_CONTEXT_NOAUTOSPAWN, NULL) == 0,
+      "Failed to connect PulseAudio context.");
+
+  // Wait until |pa_context_| is ready.  pa_threaded_mainloop_wait() must be
+  // called after pa_context_get_state() in case the context is already ready,
+  // otherwise pa_threaded_mainloop_wait() will hang indefinitely.
+  while (true) {

[tommi (sloooow) - chröme, 2016/03/16 15:21:47]

same here

[rchtara, 2016/03/19 12:07:30]

Not sure here too.

[tommi (sloooow) - chröme, 2016/03/19 13:44:17]

See thread check below (which should be at the top like it was).

[Henrik Grunell, 2016/03/21 09:44:18]

Tommi, did you perhaps not review the latest patch set? I think some of the
comments were already addressed. If you just want the thread check on top of the
functions, that's already done.

[rchtara, 2016/03/22 16:45:48]

Done.

+    pa_context_state_t context_state = pa_context_get_state(pa_context_);
+    RETURN_ON_FAILURE(PA_CONTEXT_IS_GOOD(context_state),
+                      "Invalid PulseAudio context state.");
+    if (context_state == PA_CONTEXT_READY)
+      break;
+    pa_threaded_mainloop_wait(pa_mainloop_);
+  }
+
+  if (device_id_ == AudioManagerBase::kDefaultDeviceId) {
+    UseSystemDefaultOutputDevice();
+  }
   DCHECK(thread_checker_.CalledOnValidThread());

[Henrik Grunell, 2016/03/07 11:37:17]

Put thread checker first in function.

[rchtara, 2016/03/08 17:28:16]

Done.

   return pulse::CreateOutputStream(
-      &pa_mainloop_, &pa_context_, &pa_stream_, params_, device_id_,
-      AudioManager::GetGlobalAppName(), &StreamNotifyCallback,
-      &StreamRequestCallback, this);
+      &pa_mainloop_, &pa_context_, &pa_stream_, params_, device_id_, app_name,
+      &StreamNotifyCallback, &StreamRequestCallback, this);
 }
 
 void PulseAudioOutputStream::Reset() {
@@ -235,4 +306,6 @@ void PulseAudioOutputStream::GetVolume(double* volume) {
   *volume = volume_;
 }
 
+#undef RETURN_ON_FAILURE
+
 }  // namespace media