Use correct thread for audio capturing and encoding.

Use correct thread for audio capturing and encoding.

Previously video capture thread was passed to AudioScheduler instead of the dedicated audio thread. Fixed ChromotingHost so that the correct thread is given to AudioScheduler.

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

[Wez, 2012-10-26 03:30:57]

We have the same issue, in principle, with VideoFrameCapturer, don't we?

Do you really need the AudioFactory interface?  Why not just replace the
DesktopEnvironment's AudioCapturer getter with a creater?

[Sergey Ulanov, 2012-10-26 18:38:51]

On 2012/10/26 03:30:57, Wez wrote:
> We have the same issue, in principle, with VideoFrameCapturer, don't we?

Yes, but it can be done in a separate CL. I'll add TODO.

> 
> Do you really need the AudioFactory interface?  Why not just replace the
> DesktopEnvironment's AudioCapturer getter with a creater?

In some cases it may be necessary to share resources between AudioCapturer
instances, and factory helps to do it. E.g.  crbug.com/153090 will be easy to
fix with a factory.

[Sergey Ulanov, 2012-10-26 21:07:01]

On 2012/10/26 18:38:51, sergeyu wrote:
> On 2012/10/26 03:30:57, Wez wrote:
> > We have the same issue, in principle, with VideoFrameCapturer, don't we?
> 
> Yes, but it can be done in a separate CL. I'll add TODO.
> 
> > 
> > Do you really need the AudioFactory interface?  Why not just replace the
> > DesktopEnvironment's AudioCapturer getter with a creater?
> 
> In some cases it may be necessary to share resources between AudioCapturer
> instances, and factory helps to do it. E.g.  crbug.com/153090 will be easy to
> fix with a factory.

Actually, after thinking more about it, the factory will not be that useful for
this particular bug. I'll remove the factory.

[Sergey Ulanov, 2012-10-26 21:23:12]

Done. PTAL.

On 2012/10/26 21:07:01, sergeyu wrote:
> On 2012/10/26 18:38:51, sergeyu wrote:
> > On 2012/10/26 03:30:57, Wez wrote:
> > > We have the same issue, in principle, with VideoFrameCapturer, don't we?
> > 
> > Yes, but it can be done in a separate CL. I'll add TODO.
> > 
> > > 
> > > Do you really need the AudioFactory interface?  Why not just replace the
> > > DesktopEnvironment's AudioCapturer getter with a creater?
> > 
> > In some cases it may be necessary to share resources between AudioCapturer
> > instances, and factory helps to do it. E.g.  crbug.com/153090 will be easy
to
> > fix with a factory.
> 
> Actually, after thinking more about it, the factory will not be that useful
for
> this particular bug. I'll remove the factory.

[Wez, 2012-10-27 03:43:15]

http://codereview.chromium.org/11260053/diff/7001/remoting/host/audio_schedul...
File remoting/host/audio_scheduler.cc (right):

http://codereview.chromium.org/11260053/diff/7001/remoting/host/audio_schedul...
remoting/host/audio_scheduler.cc:37: desktop_environment));
base::Unretained()?

http://codereview.chromium.org/11260053/diff/7001/remoting/host/audio_schedul...
remoting/host/audio_scheduler.cc:37: desktop_environment));
Add a comment reminding that |desktop_environment| is required to remain valid
until Stop() has completed.

http://codereview.chromium.org/11260053/diff/7001/remoting/host/audio_schedul...
remoting/host/audio_scheduler.cc:62:
base::Bind(&AudioScheduler::EncodeAudioPacket, this))) {
nit: Indentation.

http://codereview.chromium.org/11260053/diff/7001/remoting/host/audio_schedul...
remoting/host/audio_scheduler.cc:63: LOG(ERROR) << "Failed to create audio
capturer";
nit: Failed to start audio capturer?

http://codereview.chromium.org/11260053/diff/7001/remoting/host/audio_schedul...
remoting/host/audio_scheduler.cc:71: audio_capturer_->Stop();
Not for this CL, but I wonder if we even need AudioCapturer::Stop() to be
separate from deleting it any more?

http://codereview.chromium.org/11260053/diff/7001/remoting/host/desktop_envir...
File remoting/host/desktop_environment.cc (right):

http://codereview.chromium.org/11260053/diff/7001/remoting/host/desktop_envir...
remoting/host/desktop_environment.cc:31: scoped_ptr<AudioCapturer>
DesktopEnvironment::CreateAudioCapturer() {
We could remove the requirement for DesktopEnvironment to remain valid for the
Audio/VideoScheduler's lifetime by passing a TaskRunner and callback to this
Create...() API; the scheduler would make the create call during the ctor, and
be called back on the selected TaskRunner with the resulting capturer.

http://codereview.chromium.org/11260053/diff/7001/remoting/host/desktop_envir...
File remoting/host/desktop_environment.h (right):

http://codereview.chromium.org/11260053/diff/7001/remoting/host/desktop_envir...
remoting/host/desktop_environment.h:35: scoped_ptr<AudioCapturer>
CreateAudioCapturer();
nit: We should make this interface pure virtual and provide default and fake
implementations.

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

http://codereview.chromium.org/11260053/diff/7001/remoting/host/audio_schedul...
File remoting/host/audio_scheduler.cc (right):

http://codereview.chromium.org/11260053/diff/7001/remoting/host/audio_schedul...
remoting/host/audio_scheduler.cc:37: desktop_environment));
On 2012/10/27 03:43:15, Wez wrote:
> base::Unretained()?

AudioScheduler is ref-counted

http://codereview.chromium.org/11260053/diff/7001/remoting/host/audio_schedul...
remoting/host/audio_scheduler.cc:37: desktop_environment));
On 2012/10/27 03:43:15, Wez wrote:
> Add a comment reminding that |desktop_environment| is required to remain valid
> until Stop() has completed.

Done.

http://codereview.chromium.org/11260053/diff/7001/remoting/host/audio_schedul...
remoting/host/audio_scheduler.cc:62:
base::Bind(&AudioScheduler::EncodeAudioPacket, this))) {
On 2012/10/27 03:43:15, Wez wrote:
> nit: Indentation.

It is indented correctly - 4 spaces relative to start of the expression.

http://codereview.chromium.org/11260053/diff/7001/remoting/host/audio_schedul...
remoting/host/audio_scheduler.cc:63: LOG(ERROR) << "Failed to create audio
capturer";
On 2012/10/27 03:43:15, Wez wrote:
> nit: Failed to start audio capturer?

Done.

http://codereview.chromium.org/11260053/diff/7001/remoting/host/audio_schedul...
remoting/host/audio_scheduler.cc:71: audio_capturer_->Stop();
On 2012/10/27 03:43:15, Wez wrote:
> Not for this CL, but I wonder if we even need AudioCapturer::Stop() to be
> separate from deleting it any more?

No. I'll remove it in a separate CL.

http://codereview.chromium.org/11260053/diff/7001/remoting/host/desktop_envir...
File remoting/host/desktop_environment.cc (right):

http://codereview.chromium.org/11260053/diff/7001/remoting/host/desktop_envir...
remoting/host/desktop_environment.cc:31: scoped_ptr<AudioCapturer>
DesktopEnvironment::CreateAudioCapturer() {
On 2012/10/27 03:43:15, Wez wrote:
> We could remove the requirement for DesktopEnvironment to remain valid for the
> Audio/VideoScheduler's lifetime by passing a TaskRunner and callback to this
> Create...() API; the scheduler would make the create call during the ctor, and
> be called back on the selected TaskRunner with the resulting capturer.

Not sure if it's a good idea - it would only make DesktopEnvironment
implementation more complicated, and will require making DesktopEnvironment
ref-counted, at least internally. Looks like it would be better to just make
DesktopEnvironment ref-counted.

http://codereview.chromium.org/11260053/diff/7001/remoting/host/desktop_envir...
File remoting/host/desktop_environment.h (right):

http://codereview.chromium.org/11260053/diff/7001/remoting/host/desktop_envir...
remoting/host/desktop_environment.h:35: scoped_ptr<AudioCapturer>
CreateAudioCapturer();
On 2012/10/27 03:43:15, Wez wrote:
> nit: We should make this interface pure virtual and provide default and fake
> implementations.

I think it's better to do it in a separate CL.

[Wez, 2012-10-30 03:43:04]

The AudioCapturer model is currently that we create and destroy it on the
main/network thread, but Start/Stop it on the audio thread.

As far as I can tell, all we need to do to fix this bug is store the TaskRunner
on which the AudioCapturer is Start()ed and post the capture-completed task to
that when the capture completes.

I'd prefer that we do that, for consistency with the rest of the capturer code,
and revise the threading model separately.

http://codereview.chromium.org/11260053/diff/7001/remoting/host/desktop_envir...
File remoting/host/desktop_environment.cc (right):

http://codereview.chromium.org/11260053/diff/7001/remoting/host/desktop_envir...
remoting/host/desktop_environment.cc:31: scoped_ptr<AudioCapturer>
DesktopEnvironment::CreateAudioCapturer() {
On 2012/10/29 23:43:04, sergeyu wrote:
> On 2012/10/27 03:43:15, Wez wrote:
> > We could remove the requirement for DesktopEnvironment to remain valid for
the
> > Audio/VideoScheduler's lifetime by passing a TaskRunner and callback to this
> > Create...() API; the scheduler would make the create call during the ctor,
and
> > be called back on the selected TaskRunner with the resulting capturer.
> 
> Not sure if it's a good idea - it would only make DesktopEnvironment
> implementation more complicated, and will require making DesktopEnvironment
> ref-counted, at least internally. Looks like it would be better to just make
> DesktopEnvironment ref-counted.

You wouldn't need to ref-count DesktopEnvironment - it would only be reference
by the Audio/VideoScheduler ctor, to call the Create method.  The scheduler
would pass in the callback to be invoked once creation completes, and the
TaskRunner to call it on.

[Sergey Ulanov, 2012-10-30 18:47:13]

Reduced this CL to the changes that just pass correct audio thread to the
scheduler. AudioCapturerLinux will behave correctly after I land
http://codereview.chromium.org/11316010. AudioCapturerWin should already work on
the thread that starts it instead of the one that creates it.

On 2012/10/30 03:43:04, Wez wrote:
> The AudioCapturer model is currently that we create and destroy it on the
> main/network thread, but Start/Stop it on the audio thread.
> 
> As far as I can tell, all we need to do to fix this bug is store the
TaskRunner
> on which the AudioCapturer is Start()ed and post the capture-completed task to
> that when the capture completes.
> 
> I'd prefer that we do that, for consistency with the rest of the capturer
code,
> and revise the threading model separately.

It would be helpful if you mentioned this when reviewing patch set 3 :(

[Sergey Ulanov, 2012-10-30 18:47:24]

http://codereview.chromium.org/11260053/diff/7001/remoting/host/desktop_envir...
File remoting/host/desktop_environment.cc (right):

http://codereview.chromium.org/11260053/diff/7001/remoting/host/desktop_envir...
remoting/host/desktop_environment.cc:31: scoped_ptr<AudioCapturer>
DesktopEnvironment::CreateAudioCapturer() {
On 2012/10/30 03:43:04, Wez wrote:
> On 2012/10/29 23:43:04, sergeyu wrote:
> > On 2012/10/27 03:43:15, Wez wrote:
> > > We could remove the requirement for DesktopEnvironment to remain valid for
> the
> > > Audio/VideoScheduler's lifetime by passing a TaskRunner and callback to
this
> > > Create...() API; the scheduler would make the create call during the ctor,
> and
> > > be called back on the selected TaskRunner with the resulting capturer.
> > 
> > Not sure if it's a good idea - it would only make DesktopEnvironment
> > implementation more complicated, and will require making DesktopEnvironment
> > ref-counted, at least internally. Looks like it would be better to just make
> > DesktopEnvironment ref-counted.
> 
> You wouldn't need to ref-count DesktopEnvironment - it would only be reference
> by the Audio/VideoScheduler ctor, to call the Create method.  The scheduler
> would pass in the callback to be invoked once creation completes, and the
> TaskRunner to call it on.

Right, but DesktopEnvironment would still need to switch threads internally,
which will require a ref-counted object.

[alexeypa (please no reviews), 2012-10-31 23:24:15]

This CL seems to fix http://crbug.com/158804.

[Wez, 2012-10-31 23:34:39]

LGTM.

Let's follow-up by re-naming capture_task_runner to video_task_runner to avoid
it being mis-used for any other capture activities in future!

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

On 2012/10/31 23:34:39, Wez wrote:
> LGTM.
> 
> Let's follow-up by re-naming capture_task_runner to video_task_runner to avoid
> it being mis-used for any other capture activities in future!

maybe better to call it video_capture_task_runner. Because we also have a
separate thread for video encoding

[commit-bot: I haz the power, 2012-10-31 23:44:34]

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

[commit-bot: I haz the power, 2012-11-01 07:20:30]

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