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

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

Pulseaudio blocks playback when the host stops reading from the audio pipe. 
Previously the host was reading from the pipe only when there is an active 
client. Fixed the capturer to always read from the pipe, so that we don't
halt playback when user disconnects. Also changed sampling rate to 48000 to 
avoid resampling in the host. 

BUG=153090
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=165275

[Sergey Ulanov, 2012-10-29 23:43:31]

ping

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

http://codereview.chromium.org/11316010/diff/3002/remoting/host/audio_capture...
File remoting/host/audio_capturer_linux.cc (right):

http://codereview.chromium.org/11316010/diff/3002/remoting/host/audio_capture...
remoting/host/audio_capturer_linux.cc:168: scoped_refptr<base::RefCountedString>
data_ref =
nit: Add a comment e.g. "Dispatch asynchronous notification to capturers"

http://codereview.chromium.org/11316010/diff/3002/remoting/host/audio_capture...
remoting/host/audio_capturer_linux.cc:190: core_->AddCapturer(this);
nit: Use |g_audio_capturer_core| directly here?

http://codereview.chromium.org/11316010/diff/3002/remoting/host/audio_capture...
File remoting/host/audio_capturer_linux.h (right):

http://codereview.chromium.org/11316010/diff/3002/remoting/host/audio_capture...
remoting/host/audio_capturer_linux.h:28: // pulseaudio would block all playback
streams.
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?

http://codereview.chromium.org/11316010/diff/3002/remoting/host/audio_capture...
remoting/host/audio_capturer_linux.h:29: class AudioCapturerLinuxCore
nit: Since this class is effectively the sink for an audio stream from
PulseAudio, perhaps call it something more descriptive like PulseAudioPipeSink?

http://codereview.chromium.org/11316010/diff/3002/remoting/host/audio_capture...
remoting/host/audio_capturer_linux.h:30: : public
base::RefCountedThreadSafe<AudioCapturerLinuxCore>,
Does this need to be ref-counted?  When will we ever delete it?

http://codereview.chromium.org/11316010/diff/3002/remoting/host/audio_capture...
remoting/host/audio_capturer_linux.h:36:
scoped_refptr<base::SingleThreadTaskRunner> task_runner,
nit: Clarify the role of |task_runner|.

http://codereview.chromium.org/11316010/diff/3002/remoting/host/audio_capture...
remoting/host/audio_capturer_linux.h:39: explicit AudioCapturerLinuxCore(
nit: Don't need explicit here.

http://codereview.chromium.org/11316010/diff/3002/remoting/host/audio_capture...
remoting/host/audio_capturer_linux.h:39: explicit AudioCapturerLinuxCore(
Does the ctor need to be public?

http://codereview.chromium.org/11316010/diff/3002/remoting/host/audio_capture...
remoting/host/audio_capturer_linux.h:43: // AudioCapturer interface.
nit: This isn't the AudioCapturer interface. :)

http://codereview.chromium.org/11316010/diff/3002/remoting/host/audio_capture...
remoting/host/audio_capturer_linux.h:55: void StartReading(const FilePath&
pipe_name);
nit: Suggest renaming to StartOnCaptureThread.

http://codereview.chromium.org/11316010/diff/3002/remoting/host/audio_capture...
remoting/host/audio_capturer_linux.h:55: void StartReading(const FilePath&
pipe_name);
nit: Do you really need to pass |pipe_name|, rather than having a class member?

http://codereview.chromium.org/11316010/diff/3002/remoting/host/audio_capture...
remoting/host/audio_capturer_linux.h:60:
scoped_refptr<base::SingleThreadTaskRunner> main_task_runner_;
nit: Name this something more helpful, e.g. |capture_task_runner_|?

http://codereview.chromium.org/11316010/diff/3002/remoting/host/audio_capture...
remoting/host/audio_capturer_linux.h:82: explicit
AudioCapturerLinux(scoped_refptr<AudioCapturerLinuxCore> core);
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()?

http://codereview.chromium.org/11316010/diff/3002/remoting/host/remoting_me2m...
File remoting/host/remoting_me2me_host.cc (right):

http://codereview.chromium.org/11316010/diff/3002/remoting/host/remoting_me2m...
remoting/host/remoting_me2me_host.cc:454: kAudioPipeSwitchName));
nit: I think this call has become sufficiently complex to justify a comment
explaining that we're passing the PulseAudio source path, and the TaskRunner to
use to monitor it.

[Sergey Ulanov, 2012-10-30 23:59:47]

http://codereview.chromium.org/11316010/diff/3002/remoting/host/audio_capture...
File remoting/host/audio_capturer_linux.cc (right):

http://codereview.chromium.org/11316010/diff/3002/remoting/host/audio_capture...
remoting/host/audio_capturer_linux.cc:168: scoped_refptr<base::RefCountedString>
data_ref =
On 2012/10/30 04:20:11, Wez wrote:
> nit: Add a comment e.g. "Dispatch asynchronous notification to capturers"

Done.

http://codereview.chromium.org/11316010/diff/3002/remoting/host/audio_capture...
remoting/host/audio_capturer_linux.cc:190: core_->AddCapturer(this);
On 2012/10/30 04:20:11, Wez wrote:
> nit: Use |g_audio_capturer_core| directly here?

See my other comments. I'd like to avoid usage of globals as much as possible.

http://codereview.chromium.org/11316010/diff/3002/remoting/host/audio_capture...
File remoting/host/audio_capturer_linux.h (right):

http://codereview.chromium.org/11316010/diff/3002/remoting/host/audio_capture...
remoting/host/audio_capturer_linux.h:28: // pulseaudio would block all playback
streams.
On 2012/10/30 04:20:11, Wez wrote:
> 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?

Done.

http://codereview.chromium.org/11316010/diff/3002/remoting/host/audio_capture...
remoting/host/audio_capturer_linux.h:29: class AudioCapturerLinuxCore
On 2012/10/30 04:20:11, Wez wrote:
> nit: Since this class is effectively the sink for an audio stream from
> PulseAudio, perhaps call it something more descriptive like
PulseAudioPipeSink?

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

http://codereview.chromium.org/11316010/diff/3002/remoting/host/audio_capture...
remoting/host/audio_capturer_linux.h:30: : public
base::RefCountedThreadSafe<AudioCapturerLinuxCore>,
On 2012/10/30 04:20:11, Wez wrote:
> Does this need to be ref-counted?  When will we ever delete it?

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.

http://codereview.chromium.org/11316010/diff/3002/remoting/host/audio_capture...
remoting/host/audio_capturer_linux.h:36:
scoped_refptr<base::SingleThreadTaskRunner> task_runner,
On 2012/10/30 04:20:11, Wez wrote:
> nit: Clarify the role of |task_runner|.

Done.

http://codereview.chromium.org/11316010/diff/3002/remoting/host/audio_capture...
remoting/host/audio_capturer_linux.h:39: explicit AudioCapturerLinuxCore(
On 2012/10/30 04:20:11, Wez wrote:
> Does the ctor need to be public?

Done.

http://codereview.chromium.org/11316010/diff/3002/remoting/host/audio_capture...
remoting/host/audio_capturer_linux.h:39: explicit AudioCapturerLinuxCore(
On 2012/10/30 04:20:11, Wez wrote:
> nit: Don't need explicit here.

Done.

http://codereview.chromium.org/11316010/diff/3002/remoting/host/audio_capture...
remoting/host/audio_capturer_linux.h:43: // AudioCapturer interface.
On 2012/10/30 04:20:11, Wez wrote:
> nit: This isn't the AudioCapturer interface. :)

Done.

http://codereview.chromium.org/11316010/diff/3002/remoting/host/audio_capture...
remoting/host/audio_capturer_linux.h:55: void StartReading(const FilePath&
pipe_name);
On 2012/10/30 04:20:11, Wez wrote:
> nit: Do you really need to pass |pipe_name|, rather than having a class
member?

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

http://codereview.chromium.org/11316010/diff/3002/remoting/host/audio_capture...
remoting/host/audio_capturer_linux.h:55: void StartReading(const FilePath&
pipe_name);
On 2012/10/30 04:20:11, Wez wrote:
> nit: Suggest renaming to StartOnCaptureThread.

Renamed to StartOnAudioThread().

http://codereview.chromium.org/11316010/diff/3002/remoting/host/audio_capture...
remoting/host/audio_capturer_linux.h:60:
scoped_refptr<base::SingleThreadTaskRunner> main_task_runner_;
On 2012/10/30 04:20:11, Wez wrote:
> nit: Name this something more helpful, e.g. |capture_task_runner_|?

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

http://codereview.chromium.org/11316010/diff/3002/remoting/host/audio_capture...
remoting/host/audio_capturer_linux.h:82: explicit
AudioCapturerLinux(scoped_refptr<AudioCapturerLinuxCore> core);
On 2012/10/30 04:20:11, Wez wrote:
> 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()?

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.

http://codereview.chromium.org/11316010/diff/3002/remoting/host/remoting_me2m...
File remoting/host/remoting_me2me_host.cc (right):

http://codereview.chromium.org/11316010/diff/3002/remoting/host/remoting_me2m...
remoting/host/remoting_me2me_host.cc:454: kAudioPipeSwitchName));
On 2012/10/30 04:20:11, Wez wrote:
> nit: I think this call has become sufficiently complex to justify a comment
> explaining that we're passing the PulseAudio source path, and the TaskRunner
to
> use to monitor it.

Done.

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

LGTM w/ nits!

http://codereview.chromium.org/11316010/diff/3002/remoting/host/audio_capture...
File remoting/host/audio_capturer_linux.h (right):

http://codereview.chromium.org/11316010/diff/3002/remoting/host/audio_capture...
remoting/host/audio_capturer_linux.h:29: class AudioCapturerLinuxCore
On 2012/10/30 23:59:48, sergeyu wrote:
> On 2012/10/30 04:20:11, Wez wrote:
> > nit: Since this class is effectively the sink for an audio stream from
> > PulseAudio, perhaps call it something more descriptive like
> PulseAudioPipeSink?
> 
> Good idea. Renamed it to PulseaudioPipeSinkReader and moved to a separate
file.

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.

http://codereview.chromium.org/11316010/diff/10001/remoting/host/audio_captur...
File remoting/host/audio_capturer_linux.cc (right):

http://codereview.chromium.org/11316010/diff/10001/remoting/host/audio_captur...
remoting/host/audio_capturer_linux.cc:74: g_pulseaudio_pipe_sink_reader.Get();
Perhaps the style guide prohibits it, but if you make
|g_pulseaudio_pipe_sink_reader| a static local to this method then you reduce
scope for it being (ab)used elsewhere in this class.

http://codereview.chromium.org/11316010/diff/10001/remoting/host/audio_captur...
File remoting/host/audio_capturer_linux.h (right):

http://codereview.chromium.org/11316010/diff/10001/remoting/host/audio_captur...
remoting/host/audio_capturer_linux.h:16: // Linux implementation of
AudioCapturer interface.
nit: Suggest elaborating a little e.g. adding "... which pumps audio data to
clients from from a configured Pulseaudio "pipe" sink." to make the naming later
on a little clearer.

http://codereview.chromium.org/11316010/diff/10001/remoting/host/audio_captur...
remoting/host/audio_capturer_linux.h:21: // is created. |task_runner| is an IO
thread that is passed
nit: "... passed to PulseaudioPipeSinkReader ..."

http://codereview.chromium.org/11316010/diff/10001/remoting/host/audio_captur...
remoting/host/audio_capturer_linux.h:40: scoped_refptr<PulseaudioPipeSinkReader>
reader_;
nit: pipe_reader_

http://codereview.chromium.org/11316010/diff/10001/remoting/host/linux/pulsea...
File remoting/host/linux/pulseaudio_pipe_sink_reader.cc (right):

http://codereview.chromium.org/11316010/diff/10001/remoting/host/linux/pulsea...
remoting/host/linux/pulseaudio_pipe_sink_reader.cc:23: const int kSamplingRate =
48000;
nit: Add mention of the change in sample rate to the CL description; I assume
this is to avoid the need to resample when we switch to Opus?

http://codereview.chromium.org/11316010/diff/10001/remoting/host/linux/pulsea...
File remoting/host/linux/pulseaudio_pipe_sink_reader.h (right):

http://codereview.chromium.org/11316010/diff/10001/remoting/host/linux/pulsea...
remoting/host/linux/pulseaudio_pipe_sink_reader.h:19: //
PulseaudioPipeSinkReader class reads from a named pipe to which pulseaudio
nit: Suggest "PulseaudioPipeSinkReader reads raw audio samples from a configured
Pulseaudio sink via a named pipe, and delivers it to registered observers."

http://codereview.chromium.org/11316010/diff/10001/remoting/host/linux/pulsea...
remoting/host/linux/pulseaudio_pipe_sink_reader.h:21: // registered observers.
ObserverListThreadSafe is used to send data to the
nit: Move this comment to the |observers_| member, e.g. "Used to deliver audio
samples to StreamObservers on the threads on which they registered."

http://codereview.chromium.org/11316010/diff/10001/remoting/host/linux/pulsea...
remoting/host/linux/pulseaudio_pipe_sink_reader.h:39: // Functions to register
and unregister observers.
nit: Suggest "Register or unregister an observer. An observer will receive
notifications only while registered."

http://codereview.chromium.org/11316010/diff/10001/remoting/host/remoting_me2...
File remoting/host/remoting_me2me_host.cc (right):

http://codereview.chromium.org/11316010/diff/10001/remoting/host/remoting_me2...
remoting/host/remoting_me2me_host.cc:453: // provide it with a task runner for
the thread it should work on.
nit: Suggest "If an audio pipe is specific on the command-line then initialize
AudioCapturerLinux to capture from it."

[Sergey Ulanov, 2012-10-31 18:41:24]

http://codereview.chromium.org/11316010/diff/10001/remoting/host/audio_captur...
File remoting/host/audio_capturer_linux.cc (right):

http://codereview.chromium.org/11316010/diff/10001/remoting/host/audio_captur...
remoting/host/audio_capturer_linux.cc:74: g_pulseaudio_pipe_sink_reader.Get();
On 2012/10/31 00:56:28, Wez wrote:
> Perhaps the style guide prohibits it, but if you make
> |g_pulseaudio_pipe_sink_reader| a static local to this method then you reduce
> scope for it being (ab)used elsewhere in this class.

Static locals are not prohibited as long as they don't require static
finalizers, but I can't use it here because this variable is also accessed in
AudioCapturerLinux::InitializePipeReader().

http://codereview.chromium.org/11316010/diff/10001/remoting/host/audio_captur...
File remoting/host/audio_capturer_linux.h (right):

http://codereview.chromium.org/11316010/diff/10001/remoting/host/audio_captur...
remoting/host/audio_capturer_linux.h:16: // Linux implementation of
AudioCapturer interface.
On 2012/10/31 00:56:28, Wez wrote:
> nit: Suggest elaborating a little e.g. adding "... which pumps audio data to
> clients from from a configured Pulseaudio "pipe" sink." to make the naming
later
> on a little clearer.

Done.

http://codereview.chromium.org/11316010/diff/10001/remoting/host/audio_captur...
remoting/host/audio_capturer_linux.h:21: // is created. |task_runner| is an IO
thread that is passed
On 2012/10/31 00:56:28, Wez wrote:
> nit: "... passed to PulseaudioPipeSinkReader ..."

Done.

http://codereview.chromium.org/11316010/diff/10001/remoting/host/audio_captur...
remoting/host/audio_capturer_linux.h:40: scoped_refptr<PulseaudioPipeSinkReader>
reader_;
On 2012/10/31 00:56:28, Wez wrote:
> nit: pipe_reader_

Done.

http://codereview.chromium.org/11316010/diff/10001/remoting/host/linux/pulsea...
File remoting/host/linux/pulseaudio_pipe_sink_reader.cc (right):

http://codereview.chromium.org/11316010/diff/10001/remoting/host/linux/pulsea...
remoting/host/linux/pulseaudio_pipe_sink_reader.cc:23: const int kSamplingRate =
48000;
On 2012/10/31 00:56:28, Wez wrote:
> nit: Add mention of the change in sample rate to the CL description; I assume
> this is to avoid the need to resample when we switch to Opus?

Done. Yes, it's so that we don't need to resample when using opus (resampling is
still needed for 44.1k sources, but now it will be happening in pulseaudio)

http://codereview.chromium.org/11316010/diff/10001/remoting/host/linux/pulsea...
File remoting/host/linux/pulseaudio_pipe_sink_reader.h (right):

http://codereview.chromium.org/11316010/diff/10001/remoting/host/linux/pulsea...
remoting/host/linux/pulseaudio_pipe_sink_reader.h:19: //
PulseaudioPipeSinkReader class reads from a named pipe to which pulseaudio
On 2012/10/31 00:56:28, Wez wrote:
> nit: Suggest "PulseaudioPipeSinkReader reads raw audio samples from a
configured
> Pulseaudio sink via a named pipe, and delivers it to registered observers."

Done.

http://codereview.chromium.org/11316010/diff/10001/remoting/host/linux/pulsea...
remoting/host/linux/pulseaudio_pipe_sink_reader.h:21: // registered observers.
ObserverListThreadSafe is used to send data to the
On 2012/10/31 00:56:28, Wez wrote:
> nit: Move this comment to the |observers_| member, e.g. "Used to deliver audio
> samples to StreamObservers on the threads on which they registered."

I think that it's important for calling code on which thread the observer
interface is called. Moved this comment to AddObserver()

http://codereview.chromium.org/11316010/diff/10001/remoting/host/linux/pulsea...
remoting/host/linux/pulseaudio_pipe_sink_reader.h:39: // Functions to register
and unregister observers.
On 2012/10/31 00:56:28, Wez wrote:
> nit: Suggest "Register or unregister an observer. An observer will receive
> notifications only while registered."

Done.

http://codereview.chromium.org/11316010/diff/10001/remoting/host/remoting_me2...
File remoting/host/remoting_me2me_host.cc (right):

http://codereview.chromium.org/11316010/diff/10001/remoting/host/remoting_me2...
remoting/host/remoting_me2me_host.cc:453: // provide it with a task runner for
the thread it should work on.
On 2012/10/31 00:56:28, Wez wrote:
> nit: Suggest "If an audio pipe is specific on the command-line then initialize
> AudioCapturerLinux to capture from it."

Done.

[commit-bot: I haz the power, 2012-10-31 18:41:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/11316010/16001

[commit-bot: I haz the power, 2012-10-31 21:12:34]

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-11-01 00:39:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/11316010/16001