Simplify AudioRendererImpl by using AudioDevice.

content/renderer/media/audio_device.h

Index: content/renderer/media/audio_device.h
===================================================================
--- content/renderer/media/audio_device.h	(revision 107169)
+++ content/renderer/media/audio_device.h	(working copy)
@@ -25,21 +25,23 @@
 //
 // Start -> InitializeOnIOThread ------> AudioHostMsg_CreateStream -------->

[henrika (OOO until Aug 14), 2011/11/09 17:05:22]

Care to add similar sequence for Play/Resume to clearly illustrate the
difference?

[Chris Rogers, 2011/11/15 22:48:29]

Good idea - DONE

On 2011/11/09 17:05:22, henrika1 wrote:

 //       <- OnLowLatencyCreated <- AudioMsg_NotifyLowLatencyStreamCreated <-
-//       ---> StartOnIOThread -----------> AudioHostMsg_PlayStream -------->
+//       ---> PlayOnIOThread -----------> AudioHostMsg_PlayStream -------->
 //
 // AudioDevice::Render => audio transport on audio thread with low latency =>
 //                               |
 // Stop --> ShutDownOnIOThread -------->  AudioHostMsg_CloseStream -> Close
 //
-// This class utilizes three threads during its lifetime, namely:
+// This class utilizes several threads during its lifetime, namely:
 // 1. Creating thread.
-//    Must be the main render thread. Start and Stop should be called on
-//    this thread.
-// 2. IO thread.
+//    Must be the main render thread.
+// 2. Control thread (may be the main render thread or another thread).
+//    The methods: Start(), Stop(), Play(), Pause(), SetVolume()
+//    Must be called on the same thread.
+// 2. IO thread (internal implementation detail - not exposed to public API)

[acolwell GONE FROM CHROMIUM, 2011/11/08 21:44:20]

Change the second 2. to a 3. and update the 3. below.

[Chris Rogers, 2011/11/15 22:48:29]

Done.

 //    The thread within which this class receives all the IPC messages and
 //    IPC communications can only happen in this thread.
 // 3. Audio transport thread.
-//    Responsible for calling the RenderCallback and feed audio samples to
+//    Responsible for calling the RenderCallback and feeding audio samples to
 //    the audio layer in the browser process using sync sockets and shared
 //    memory.
 //
@@ -60,9 +62,8 @@
 #include "base/shared_memory.h"
 #include "base/threading/simple_thread.h"
 #include "content/renderer/media/audio_message_filter.h"
+#include "media/audio/audio_parameters.h"
 
-struct AudioParameters;
-
 class AudioDevice
     : public AudioMessageFilter::Delegate,
       public base::DelegateSimpleThread::Delegate,
@@ -78,18 +79,38 @@
   };
 
   // Methods called on main render thread -------------------------------------
+
+  // Minimal constructor where Initialize() must later be called.
+  explicit AudioDevice(RenderCallback* callback);

[acolwell GONE FROM CHROMIUM, 2011/11/08 21:44:20]

Why not pass this callback in Initialize()? Is it needed before then?

[Chris Rogers, 2011/11/15 22:48:29]

Done.

+
   AudioDevice(size_t buffer_size,

[acolwell GONE FROM CHROMIUM, 2011/11/08 21:44:20]

How about we remove this constructor so all AudioDevice users call Initialize()
explicitly.

[Chris Rogers, 2011/11/10 02:17:22]

Henrik any thoughts here?

My feeling is that in the common case, the audio format (sample-rate, etc.)
*are* actually known at construction time, so it's a bit clunky to not have the
"normal" constructor supplied as its being currently used.  This constructor is
currently being used by both WebRTC and WebAudio.

I'm open to changing it if we can get consensus, but it seems like we're making
things more complicated for the normal use of this class if we remove it.

On 2011/11/08 21:44:20, acolwell wrote:

[acolwell GONE FROM CHROMIUM, 2011/11/10 21:58:15]

I guess I'm looking for consistency between use cases here. Does it make sense
to defer AudioDevice creation until we actually have the required information?
If so then we can just remove the new constructor & Initialize() method.

On 2011/11/10 02:17:22, Chris Rogers wrote:

[Chris Rogers, 2011/11/15 22:48:29]

I definitely agree that it would be nice to defer AudioDevice construction and
just avoid the Initialize() altogether, but unfortunately we can't because the
constructors *must* be called on the main renderer thread because of the
initialization of the "filter_" member.

There's a comment at the top of this header file (thread 1.) about this and also
a comment in AudioRendererImpl::AudioRendererImpl() describing why we construct
at this point.

On 2011/11/10 21:58:15, acolwell wrote:

               int channels,
               double sample_rate,
               RenderCallback* callback);
   virtual ~AudioDevice();
 
+  void Initialize(size_t buffer_size,

[acolwell GONE FROM CHROMIUM, 2011/11/08 21:44:20]

Any reason we can't just make this an int?

[Chris Rogers, 2011/11/10 02:17:22]

I'm just following the type we are currently using in the constructor - to be
consistent.

On 2011/11/08 21:44:20, acolwell wrote:

+                  int channels,
+                  double sample_rate,
+                  AudioParameters::Format latency_format);
+  bool IsInitialized();
+
   // Starts audio playback.
   void Start();
 
   // Stops audio playback. Returns |true| on success.
   bool Stop();
 
+  // Resumes playback if currently paused.
+  void Play();

[acolwell GONE FROM CHROMIUM, 2011/11/08 21:44:20]

Why do we need Start()/Stop() AND Play()/Pause()? This is confusing to me. If
Play() is only called after Pause() would Resume() be a more fitting name?

[henrika (OOO until Aug 14), 2011/11/09 17:05:22]

If it is intended to resume, then why not call it Resume()?

[Chris Rogers, 2011/11/10 02:17:22]

The intention is to closely follow the existing low-level control messages that
are passed to the browser process:
AudioHostMsg_CreateStream / AudioHostMsg_PlayStream <-> Start()
AudioHostMsg_PlayStream <-> Play()
AudioHostMsg_PauseStream / AudioHostMsg_FlushStream<-> Pause()
AudioHostMsg_CloseStream <-> Stop()

Because AudioRendererImpl was using these *exact* control messages, I wanted to
replicate as closely as possible the same logic and behavior in the new
AudioRendererImpl.  Because the AudioRendererImpl code is delicate, I want to be
very careful that the <audio> and <video> tags continue to work.  I've done my
best to stress test these changes to make sure everything still works correctly.

I agree that in the long-term, it's probably not ideal to introduce the concepts
of Play() / Pause().  And I think we can continue to do additional re-factorings
here (and in AudioRendererImpl) to simplify this to Start() / Stop().  But, I
think we should be very careful and first make sure that this first major
AudioRendererImpl re-factoring works correctly by following the existing logic
fairly closely, before attempting to simplify.  Any simplification will need to
take into account that the Stop() method is a synchronous call, and since
AudioRendererImpl makes pretty liberal use of the Play() method, there are some
pretty serious performance issues (and raciness with Start()) to consider there.
 Also, the existence of AudioHostMsg_PlayStream and AudioHostMsg_PauseStream,
means we're following the IPC message protocol pretty closely.

I can add a FIXME comment here, describing the reasons for Play() / Pause() and
the desire to simplify this with further re-factoring.

On 2011/11/08 21:44:20, acolwell wrote:

[henrika (OOO until Aug 14), 2011/11/10 11:45:07]

I am fine with trying to mimic the existing AudioRendererImpl  as much as
possible. As you say, the code *is* delicate and larger changes might have to
wait. A TODO/FIXME would not hurt though.

[Chris Rogers, 2011/11/15 22:48:29]

I've added a TODO(crogers) for Play()/Pause() describing how we can get rid of
Play()/Pause() upon further re-factoring of AudioRendererImpl

Keeping for now because of the delicate logic (and to make sure we don't change
too many things all at once).

On 2011/11/10 11:45:07, henrika1 wrote:

+
+  // Pauses playback.
+  // If |flush| is true then any pending audio which is in the pipeline
+  // (has not yet reached the hardware) will be discarded.  In this case,
+  // When Play() is later called, no previous pending audio will be
+  // rendered.
+  void Pause(bool flush);
+
   // Sets the playback volume, with range [0.0, 1.0] inclusive.
   // Returns |true| on success.
   bool SetVolume(double volume);
@@ -119,7 +140,8 @@
   // be executed on that thread. They interact with AudioMessageFilter and
   // sends IPC messages on that thread.
   void InitializeOnIOThread(const AudioParameters& params);
-  void StartOnIOThread();
+  void PlayOnIOThread();
+  void PauseOnIOThread(bool flush);
   void ShutDownOnIOThread(base::WaitableEvent* completion);
   void SetVolumeOnIOThread(double volume);
 
@@ -138,6 +160,7 @@
   int channels_;
   int bits_per_sample_;
   double sample_rate_;
+  AudioParameters::Format latency_format_;
 
   RenderCallback* callback_;