Moving SwitchOutputDevice out of OutputDevice interface, eliminating OutputDevice

content/public/renderer/media_stream_audio_renderer.h

 // Copyright 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.
 
 #ifndef CONTENT_PUBLIC_RENDERER_MEDIA_STREAM_AUDIO_RENDERER_H_
 #define CONTENT_PUBLIC_RENDERER_MEDIA_STREAM_AUDIO_RENDERER_H_
 
 #include <string>
 
 #include "base/callback.h"
 #include "base/memory/ref_counted.h"
 #include "base/time/time.h"
-#include "media/base/output_device.h"
-#include "url/gurl.h"
+#include "media/base/output_device_info.h"
+#include "url/origin.h"
 
 namespace content {
 
 class MediaStreamAudioRenderer
     : public base::RefCountedThreadSafe<MediaStreamAudioRenderer> {
  public:
   // Starts rendering audio.
   virtual void Start() = 0;
 
   // Stops rendering audio.
   virtual void Stop() = 0;
 
   // Resumes rendering audio after being paused.
   virtual void Play() = 0;
 
   // Temporarily suspends rendering audio. The audio stream might still be
   // active but new audio data is not provided to the consumer.
   virtual void Pause() = 0;
 
   // Sets the output volume.
   virtual void SetVolume(float volume) = 0;
 
-  // Returns a pointer to the internal output device.
-  // The pointer is not to be owned by the caller and is valid only during
-  // the lifetime of the MediaStreamAudioRenderer.
-  // It can be null, which means that access to the output device is not
-  // supported.
-  virtual media::OutputDevice* GetOutputDevice() = 0;
+  // Returns current output device information. If the information is not
+  // available yet, this method may block until it becomes available.
+  // If output device is not supported, |device_status| of
+  // OutputDeviceInfo should be set to OUTPUT_DEVICE_STATUS_ERROR_INTERNAL.
+  // Must never be called on the IO thread.

[miu, 2016/03/24 21:32:02]

nit: You can probably omit the "never be called on the IO thread" comment.  It's
generally assumed a MediaStreamAudioRenderer's methods are all called on the
main thread.

[o1ka, 2016/03/29 13:39:53]

I would probably leave it as it is now: Since the method looks like a simple
getter, somebody can probably attempt to call it in the render pipeline to
calculate some parameters and will end up debugging all this stuff. It's no harm
to have an extra warning. WDYT?

+  virtual media::OutputDeviceInfo GetOutputDeviceInfo() = 0;
+
+  // Attempts to switch the audio output device.
+  // Once the attempt is finished, |callback| is invoked with the
+  // result of the operation passed as a parameter. The result is a value from
+  // the media::OutputDeviceStatus enum.
+  // There is no guarantee about the thread where |callback| will
+  // be invoked, so users are advised to use media::BindToCurrentLoop() to

[miu, 2016/03/24 21:32:02]

Because this is a public interface, it's usually best not to discuss internal
implementation details.  So, I'd suggest NOT mentioning "advised to use
media::BindToCurrentLoop()".  It's enough to just say that |callback| could be
run an any thread.

Alternatively, consider guaranteeing that, since this method is called on the
main thread, then the callback will also be run on the same thread. 
Thread-hopping is something the implementation has to do internally in order to
"get the job done," so philosophically this is probably not something we should
be burdening client code to deal with.

[o1ka, 2016/03/29 13:39:53]

I just copied the old SwitchOutputDevice() comment here, and I agree it's TMI -
fixing it.
Yes, callbacks seem to always run on the same thread now, but I don't quite want
to weaken callback requirements now since the client code deals with that
anyways and we may probably need to utilize that again at some point.
WDYT?

[miu, 2016/03/29 19:46:27]

I suppose it's fine to leave it alone for this change.  However, for code
readability and maintainability, interfaces should avoid the "any old thread
will do" pattern.  They force people to write ugly code like this:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

[o1ka, 2016/03/30 14:02:42]

Agree. We need to come back to it after a couple of other refactoring efforts.

+  // ensure that |callback| runs on the correct thread.
+  // Note also that copy constructors and destructors for arguments bound to

[miu, 2016/03/24 21:32:02]

You can remove lines 51-54.  This is generally understood throughout the
Chromium project (i.e., if it needs to be documented, it should be in
base/bind.h).

[o1ka, 2016/03/29 13:39:54]

Done.

+  // |callback| may run on arbitrary threads as |callback| is moved across
+  // threads. It is advisable to bind arguments such that they are released by
+  // |callback| when it runs in order to avoid surprises.
+  virtual void SwitchOutputDevice(
+      const std::string& device_id,
+      const url::Origin& security_origin,
+      const media::OutputDeviceStatusCB& callback) = 0;
 
   // Time stamp that reflects the current render time. Should not be updated
   // when paused.
   virtual base::TimeDelta GetCurrentRenderTime() const = 0;
 
   // Returns true if the implementation is a local renderer and false
   // otherwise.
   virtual bool IsLocalRenderer() const = 0;
 
  protected:
   friend class base::RefCountedThreadSafe<MediaStreamAudioRenderer>;
 
   virtual ~MediaStreamAudioRenderer() {}
 };
 
 }  // namespace content
 
 #endif  // CONTENT_PUBLIC_RENDERER_MEDIA_STREAM_AUDIO_RENDERER_H_