Simplify AudioRendererImpl by using AudioDevice.

content/renderer/media/audio_device.h

 // Copyright (c) 2011 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.
 //
 // Low-latency audio rendering unit utilizing audio output stream provided
 // by browser process through IPC.
 //
 // Relationship of classes.
 //
 //  AudioOutputController                AudioDevice
 //           ^                                ^
 //           |                                |
 //           v               IPC              v
 //    AudioRendererHost  <---------> AudioMessageFilter
 //
 // Transportation of audio samples from the render to the browser process
 // is done by using shared memory in combination with a sync socket pair
 // to generate a low latency transport. The AudioDevice user registers an
 // AudioDevice::RenderCallback at construction and will be polled by the
 // AudioDevice for audio to be played out by the underlying audio layers.
 //
 // State sequences.
 //
 //            Task [IO thread]                  IPC [IO thread]
 //
 // 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.
 //
 // Implementation notes:
 //
 // - Start() is asynchronous/non-blocking.

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

Merge with comments about Play/Pause?

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

Done.

 // - Stop() is synchronous/blocking.
 // - The user must call Stop() before deleting the class instance.
 
 #ifndef CONTENT_RENDERER_MEDIA_AUDIO_DEVICE_H_
 #define CONTENT_RENDERER_MEDIA_AUDIO_DEVICE_H_
 #pragma once
 
 #include <vector>
 
 #include "base/basictypes.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/shared_memory.h"
 #include "base/threading/simple_thread.h"
 #include "content/renderer/media/audio_message_filter.h"
-
-struct AudioParameters;
+#include "media/audio/audio_parameters.h"
 
 class AudioDevice
     : public AudioMessageFilter::Delegate,
       public base::DelegateSimpleThread::Delegate,
       public base::RefCountedThreadSafe<AudioDevice> {
  public:
   class RenderCallback {

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

Is there a reason we can't use base::Callback here? It would be more consistent
with the rest of the media code.

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

This isn't part of my CL - it's the way the class currently exists.  We might
consider further cleanup here possibly as a separate CL.

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

    public:
     virtual void Render(const std::vector<float*>& audio_data,
                         size_t number_of_frames,
                         size_t audio_delay_milliseconds) = 0;
    protected:
     virtual ~RenderCallback() {}
   };
 
   // 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);
 
   // Gets the playback volume, with range [0.0, 1.0] inclusive.
   void GetVolume(double* volume);
 
   double sample_rate() const { return sample_rate_; }
   size_t buffer_size() const { return buffer_size_; }
 
   static double GetAudioHardwareSampleRate();
   static size_t GetAudioHardwareBufferSize();
 
   // Methods called on IO thread ----------------------------------------------
   // AudioMessageFilter::Delegate methods, called by AudioMessageFilter.
   virtual void OnRequestPacket(AudioBuffersState buffers_state);
   virtual void OnStateChanged(AudioStreamState state);
   virtual void OnCreated(base::SharedMemoryHandle handle, uint32 length);
   virtual void OnLowLatencyCreated(base::SharedMemoryHandle handle,
                                    base::SyncSocket::Handle socket_handle,
                                    uint32 length);
   virtual void OnVolume(double volume);
 
  private:
   // Methods called on IO thread ----------------------------------------------
   // The following methods are tasks posted on the IO thread that needs to
   // 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);
 
   void Send(IPC::Message* message);
 
   // Method called on the audio thread (+ one call on the IO thread) ----------
   // Calls the client's callback for rendering audio. There will also be one
   // initial call on the IO thread before the audio thread has been created.
   void FireRenderCallback();
 
   // DelegateSimpleThread::Delegate implementation.
   virtual void Run();
 
   // Format
   size_t buffer_size_;  // in sample-frames
   int channels_;
   int bits_per_sample_;
   double sample_rate_;
+  AudioParameters::Format latency_format_;
 
   RenderCallback* callback_;
 
   // The client callback renders audio into here.
   std::vector<float*> audio_data_;
 
   // The client stores the last reported audio delay in this member.
   // The delay shall reflect the amount of audio which still resides in
   // the output buffer, i.e., the expected audio output delay.
   int audio_delay_milliseconds_;
@@ skipping 13 matching lines @@
   scoped_refptr<AudioMessageFilter> filter_;
 
   // Our stream ID on the message filter. Only accessed on the IO thread.
   int32 stream_id_;
 
   // Data transfer between browser and render process uses a combination
   // of sync sockets and shared memory to provide lowest possible latency.
   scoped_ptr<base::SharedMemory> shared_memory_;
   scoped_ptr<base::SyncSocket> socket_;
 
   DISALLOW_IMPLICIT_CONSTRUCTORS(AudioDevice);

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

Reinstate if you decide to remove noarg constructor. Use
DISALLOW_COPY_AND_ASSIGN(AudioDevice) if the constructor stays.

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

Added DISALLOW_COPY_AND_ASSIGN(AudioDevice)

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

 };
 
 #endif  // CONTENT_RENDERER_MEDIA_AUDIO_DEVICE_H_