Fix AudioCapturer implementation to read from audio pipe even when there are no active clients.

remoting/host/audio_capturer_linux.h

Index: remoting/host/audio_capturer_linux.h
diff --git a/remoting/host/audio_capturer_linux.h b/remoting/host/audio_capturer_linux.h
index 8a825fffbf0b3fee922c56d750c7e7d4e0caf9a6..0a757ff2b2c226756a873b6932fc6b129b44308e 100644
--- a/remoting/host/audio_capturer_linux.h
+++ b/remoting/host/audio_capturer_linux.h
@@ -5,43 +5,63 @@
 #ifndef REMOTING_HOST_AUDIO_CAPTURER_LINUX_H_
 #define REMOTING_HOST_AUDIO_CAPTURER_LINUX_H_
 
-#include "remoting/host/audio_capturer.h"
-
+#include "base/memory/ref_counted_memory.h"
 #include "base/message_loop.h"
+#include "base/observer_list_threadsafe.h"
 #include "base/time.h"
 #include "base/timer.h"
+#include "remoting/host/audio_capturer.h"
 
 class FilePath;
 
 namespace remoting {
 
-class AudioCapturerLinux : public AudioCapturer,
-                           public MessageLoopForIO::Watcher {
+class AudioCapturerLinux;
+
+// On Linux audio capturer is implemented by redirecting audio stream from
+// pulseaudio to the host process via a named pipe. The daemon that starts the
+// host sets up pulseaudio environment variables and writes config for it such
+// that pulseaudio uses module-pipe-sink for all playback streams.
+// AudioCapturerLinuxCore class reads from the pipe and then sends data to all
+// instances of AudioCapturerLinux. This is necessary because we need to read
+// from the pipe even when there are no active Chromoting clients. Otherwise
+// pulseaudio would block all playback streams.

[Wez, 2012/10/30 04:20:11]

Can we reword this comment to describe the roles of AudioCapturerLinux vs
AudioCapturerLinuxCore, i.e. that we start the Core as soon as the calling
process initializes it, to read the audio stream from pulse-audio, and use a
thread-safe observer to notify zero or more AudioCapturer instances as each
chunk of sample data is read?

[Sergey Ulanov, 2012/10/30 23:59:48]

Done.

+class AudioCapturerLinuxCore

[Wez, 2012/10/30 04:20:11]

nit: Since this class is effectively the sink for an audio stream from
PulseAudio, perhaps call it something more descriptive like PulseAudioPipeSink?

[Sergey Ulanov, 2012/10/30 23:59:48]

Good idea. Renamed it to PulseaudioPipeSinkReader and moved to a separate file.

[Wez, 2012/10/31 00:56:27]

nit: If this class would work with _any_ pipe-based audio source (assuming it
provides the right format) then this could just be AudioPipeReader; the comments
on AudioCapturerLinux would then clarify that we use it with Pulseaudio.

+    : public base::RefCountedThreadSafe<AudioCapturerLinuxCore>,

[Wez, 2012/10/30 04:20:11]

Does this need to be ref-counted?  When will we ever delete it?

[Sergey Ulanov, 2012/10/30 23:59:48]

We should try to avoid global variables. Ideally this class should be stored a
audio capturer factory, at which point ref-counting will be useful.

+      public MessageLoopForIO::Watcher {
  public:
-  // Must be called to configure the capturer before the first instance is
-  // created.
-  static void SetPipeName(const FilePath& pipe_name);
+  // Must be called to configure the capturer before the first capturer instance
+  // is created.
+  static void Initialize(
+      scoped_refptr<base::SingleThreadTaskRunner> task_runner,

[Wez, 2012/10/30 04:20:11]

nit: Clarify the role of |task_runner|.

[Sergey Ulanov, 2012/10/30 23:59:48]

Done.

+      const FilePath& pipe_name);
 
-  explicit AudioCapturerLinux(const FilePath& pipe_name);
-  virtual ~AudioCapturerLinux();
+  explicit AudioCapturerLinuxCore(

[Wez, 2012/10/30 04:20:11]

nit: Don't need explicit here.

[Wez, 2012/10/30 04:20:11]

Does the ctor need to be public?

[Sergey Ulanov, 2012/10/30 23:59:48]

Done.

[Sergey Ulanov, 2012/10/30 23:59:48]

Done.

+      scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+      const FilePath& pipe_name);
 
   // AudioCapturer interface.

[Wez, 2012/10/30 04:20:11]

nit: This isn't the AudioCapturer interface. :)

[Sergey Ulanov, 2012/10/30 23:59:48]

Done.

-  virtual bool Start(const PacketCapturedCallback& callback) OVERRIDE;
-  virtual void Stop() OVERRIDE;
-  virtual bool IsStarted() OVERRIDE;
+  void AddCapturer(AudioCapturerLinux* capturer);
+  void RemoveCapturer(AudioCapturerLinux* capturer);
 
   // MessageLoopForIO::Watcher interface.
   virtual void OnFileCanReadWithoutBlocking(int fd) OVERRIDE;
   virtual void OnFileCanWriteWithoutBlocking(int fd) OVERRIDE;
 
  private:
+  friend class base::RefCountedThreadSafe<AudioCapturerLinuxCore>;
+  virtual ~AudioCapturerLinuxCore();
+
+  void StartReading(const FilePath& pipe_name);

[Wez, 2012/10/30 04:20:11]

nit: Suggest renaming to StartOnCaptureThread.

[Wez, 2012/10/30 04:20:11]

nit: Do you really need to pass |pipe_name|, rather than having a class member?

[Sergey Ulanov, 2012/10/30 23:59:48]

We need to use this name only once, so there is no need to add a member for it.

[Sergey Ulanov, 2012/10/30 23:59:48]

Renamed to StartOnAudioThread().

   void StartTimer();
   void DoCapture();
   void WaitForPipeReadable();
 
+  scoped_refptr<base::SingleThreadTaskRunner> main_task_runner_;

[Wez, 2012/10/30 04:20:11]

nit: Name this something more helpful, e.g. |capture_task_runner_|?

[Sergey Ulanov, 2012/10/30 23:59:48]

It's unambiguous as there is only one thread this class needs to know about.
Renamed it to |task_runner_|

+
   int pipe_fd_;
-  base::RepeatingTimer<AudioCapturerLinux> timer_;
-  PacketCapturedCallback callback_;
+  base::RepeatingTimer<AudioCapturerLinuxCore> timer_;
+  scoped_refptr<ObserverListThreadSafe<AudioCapturerLinux> > capturers_;
 
   // Time when capturing was started.
   base::TimeTicks started_time_;
@@ -54,6 +74,28 @@ class AudioCapturerLinux : public AudioCapturer,
 
   MessageLoopForIO::FileDescriptorWatcher file_descriptor_watcher_;
 
+  DISALLOW_COPY_AND_ASSIGN(AudioCapturerLinuxCore);
+};
+
+class AudioCapturerLinux : public AudioCapturer {
+ public:
+  explicit AudioCapturerLinux(scoped_refptr<AudioCapturerLinuxCore> core);

[Wez, 2012/10/30 04:20:11]

nit: Do you need to pass |core| in here?  Can't you add the capturer to the
global core instance in Start() and remove it in Stop()?

[Sergey Ulanov, 2012/10/30 23:59:48]

In the future I'd like to avoid the need to have to store
PulseaudioPipeSinkReader instance in a global variable. Instead it should be
stored in some factory object.

+  virtual ~AudioCapturerLinux();
+
+  // AudioCapturer interface.
+  virtual bool Start(const PacketCapturedCallback& callback) OVERRIDE;
+  virtual void Stop() OVERRIDE;
+  virtual bool IsStarted() OVERRIDE;
+
+ private:
+  friend class AudioCapturerLinuxCore;
+
+  // Called by AudioCapturerLinuxCore;
+  void OnDataRead(scoped_refptr<base::RefCountedString> data);
+
+  scoped_refptr<AudioCapturerLinuxCore> core_;
+  PacketCapturedCallback callback_;
+
   DISALLOW_COPY_AND_ASSIGN(AudioCapturerLinux);
 };