Switching AudioOutputAuthorizationHandler from using AudioManager interface to AudioSystem one.

media/audio/audio_system_impl.cc

 // Copyright 2017 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 "media/audio/audio_system_impl.h"
 
 #include "base/memory/ptr_util.h"
 #include "base/single_thread_task_runner.h"
 #include "base/task_runner_util.h"
+#include "media/audio/audio_device_description.h"
 #include "media/audio/audio_manager.h"
 
 // Using base::Unretained for |audio_manager_| is safe since it is deleted after
 // its task runner, and AudioSystemImpl is deleted on the UI thread after the IO
 // thread has been stopped and before |audio_manager_| deletion is scheduled.
 namespace media {
 
 namespace {
 
 AudioParameters GetInputParametersOnDeviceThread(AudioManager* audio_manager,
                                                  const std::string& device_id) {
   DCHECK(audio_manager->GetTaskRunner()->BelongsToCurrentThread());
 
   // TODO(olka): remove this when AudioManager::GetInputStreamParameters()
   // works this way on all the platforms.
   if (!audio_manager->HasAudioInputDevices())
     return AudioParameters();
 
   return audio_manager->GetInputStreamParameters(device_id);
 }
 
+AudioParameters GetOutputParametersOnDeviceThread(
+    AudioManager* audio_manager,
+    const std::string& device_id) {
+  DCHECK(audio_manager->GetTaskRunner()->BelongsToCurrentThread());
+
+  // TODO(olka): remove this when
+  // AudioManager::Get[Default]OutputStreamParameters() works this way on all
+  // the platforms.

[tommi (sloooow) - chröme, 2017/02/14 12:14:26]

This covers a particular set of cases but not when a specific device is being
asked for.
As an example, I've seen calls make it to the AudioManager layer where the
device_id is actually an id for tab audio capture.

So, in this case, even if HasAudioOutputDevices() returns true, it still could
be that the device_id is invalid or points to a device that isn't available.  In
those cases, we should also be returning invalid audio parameters.

[o1ka, 2017/02/14 15:00:49]

TabAudioCapture are input devices, not output ones.
Same if it is just a random string. This is up to per-platform AudioManager
implementations to handle it, and it can't be addressed in this CL.

[tommi (sloooow) - chröme, 2017/02/14 15:30:32]

I am aware of that and I didn't say it was output.

In my comment, I mention it as an example of a request going to a layer it
should not go to, unchecked.  The pattern being used here, is the same pattern
as being used a couple of lines up, in GetInputParametersOnDeviceThread. That
means that both have the same limitation.
I didn't say that it should.  However, this CL attempts to handle it partially
but not completely and checks are being done in two places. - Which is OK as a
temporary situation.

The comment however says "remove this when [...] works this way on all the
platforms".  I'm pointing out that a proper check needs to do more than the
error check being added here.  So in this CL, it would be good to at least
update the comment.

[o1ka, 2017/02/14 15:47:12]

You haven't said that you meant both Input and Output methods either  :)
I'm not sure I completely follow you. AudioManager should return invalid
parameters if device is not found. When it (all its implementations) is fixed to
do so, we can remove HasDevices check. Right? That's what I meant by the
comment. I'll update the header with more precise description of when invalid
parameters are returned.

[tommi (sloooow) - chröme, 2017/02/14 16:30:48]

I wasn't talking specifically about either input or output. I was talking about
this layer vs the AudioManager layer.  Applicable to both input and output. Can
you read the comment again and perhaps tell me how I should have phrased them?
That's correct. The HasDevices check can be removed when all implementations do
proper error handling.
Just the HasDevices() check, is not enough.  It only does something useful if
there are no devices at all.

+  if (!audio_manager->HasAudioOutputDevices())
+    return AudioParameters();
+
+  return media::AudioDeviceDescription::IsDefaultDevice(device_id)

[o1ka, 2017/02/14 11:37:03]

guidou@: it seems that all the usecases make this check anyways, so I decided to
combine the two cases into one interface method. Are you ok with it, or may it
lead to a confusion?

[tommi (sloooow) - chröme, 2017/02/14 12:14:26]

Doesn't GetOutputStreamParameters already handle the default device too? One
method at the audiomanager level (for now), makes sense to me instead of making
this level have to check the id.

[o1ka, 2017/02/14 15:00:49]

No, it does not.

One
The goal is to introduce the correct interface. This particular implementation
is just what it was before the CL - it's just in a different place now, hidden
by the interface. Modifications of AudioManager within this CL will lead to a
much larger diff (just check all the occurrences of
AudioManager::GetDefaultOutputStreamParameters). If we want to modify
AudioManager, it can be addressed in a separate CL after all the clients are
switch to AudioSystem. This way we will have better locality of changes and a
smaller diff.

Overall, for Audio Process work we want to avoid AudioManager modifications as
much as possible, because each platform-specific implementation requires a
separate attention, and we are very limited in resources. AudioManager interface
is not nice at all, but we will deal with it later - now it's enough to hide it.

[tommi (sloooow) - chröme, 2017/02/14 15:30:32]

Uhm, ok, time for fact checking! :)

On Mac and Windows they actually do and those are the most widely used
implementations.
I understand, I'm one of those resources and I'm not trying to block the cl.

[o1ka, 2017/02/14 15:47:12]

No, it does not :) Take a look here:
AudioParameters AudioManagerBase::GetDefaultOutputStreamParameters() {
  return GetPreferredOutputStreamParameters(GetDefaultOutputDeviceID(),
      AudioParameters());
}

We need to pass a result of a non-public
AudioManager::GetDefaultOutputDeviceID() into
AudioManager::GetOutputStreamParameters(), to get the parameters for the default
device.

With the proposed AudioSystem interface, we are passing "default" and getting
the default parameters.
I'm glad that we are on the same page :)

[tommi (sloooow) - chröme, 2017/02/14 16:30:48]

Windows:
https://cs.chromium.org/chromium/src/media/audio/win/core_audio_util_win.cc?r...
Mac:
https://cs.chromium.org/chromium/src/media/audio/mac/audio_manager_mac.cc?rcl...

That's where GetOutputStreamParameters("default") would end up (from
AudioManagerBase::GetOutputStreamParameters to platform specific
GetPreferredOutputStreamParameters).
That's all I'm saying.

+             ? audio_manager->GetDefaultOutputStreamParameters()
+             : audio_manager->GetOutputStreamParameters(device_id);
+}
+
 }  // namespace
 
 AudioSystemImpl::AudioSystemImpl(AudioManager* audio_manager)
     : audio_manager_(audio_manager) {
   DCHECK(audio_manager_);
   AudioSystem::SetInstance(this);
 }
 
 AudioSystemImpl::~AudioSystemImpl() {
   AudioSystem::ClearInstance(this);
@@ skipping 14 matching lines @@
                                                 audio_manager_, device_id)));
     return;
   }
   base::PostTaskAndReplyWithResult(
       GetTaskRunner(), FROM_HERE,
       base::Bind(&GetInputParametersOnDeviceThread,
                  base::Unretained(audio_manager_), device_id),
       std::move(on_params_cb));
 }
 
+void AudioSystemImpl::GetOutputStreamParameters(
+    const std::string& device_id,
+    OnAudioParamsCallback on_params_cb) const {
+  if (GetTaskRunner()->BelongsToCurrentThread()) {
+    GetTaskRunner()->PostTask(
+        FROM_HERE, base::Bind(on_params_cb, GetOutputParametersOnDeviceThread(
+                                                audio_manager_, device_id)));
+    return;
+  }
+  base::PostTaskAndReplyWithResult(
+      GetTaskRunner(), FROM_HERE,
+      base::Bind(&GetOutputParametersOnDeviceThread,
+                 base::Unretained(audio_manager_), device_id),
+      std::move(on_params_cb));
+}
+
 void AudioSystemImpl::HasInputDevices(OnBoolCallback on_has_devices_cb) const {
   if (GetTaskRunner()->BelongsToCurrentThread()) {
     GetTaskRunner()->PostTask(
         FROM_HERE,
         base::Bind(on_has_devices_cb, audio_manager_->HasAudioInputDevices()));
     return;
   }
   base::PostTaskAndReplyWithResult(
       GetTaskRunner(), FROM_HERE,
       base::Bind(&AudioManager::HasAudioInputDevices,
                  base::Unretained(audio_manager_)),
       std::move(on_has_devices_cb));
 }
 
 AudioManager* AudioSystemImpl::GetAudioManager() const {
   return audio_manager_;
 }
 
 base::SingleThreadTaskRunner* AudioSystemImpl::GetTaskRunner() const {
   return audio_manager_->GetTaskRunner();
 }
 
 }  // namespace media