Switching AudioOutputAuthorizationHandler from using AudioManager interface to AudioSystem one.

media/audio/audio_system_impl.cc

Index: media/audio/audio_system_impl.cc
diff --git a/media/audio/audio_system_impl.cc b/media/audio/audio_system_impl.cc
index fdffa70fda0f70cbce1e60457eb2044e9287a254..bde76d5f2a1a192b1611e0c9e94e15d4388e9c96 100644
--- a/media/audio/audio_system_impl.cc
+++ b/media/audio/audio_system_impl.cc
@@ -7,6 +7,7 @@
 #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
@@ -28,6 +29,22 @@ AudioParameters GetInputParametersOnDeviceThread(AudioManager* audio_manager,
   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)
@@ -62,6 +79,22 @@ void AudioSystemImpl::GetInputStreamParameters(
       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(