Remove CommandLine dependency from the Linux's AudioCapturer.

Remove CommandLine dependency from the Linux's AudioCapturer.

Now ChromotingHostContext is used to pass Pipe name to the audio capturer.

BUG=148742
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=156739

[Lambros, 2012-09-13 00:18:26]

lgtm
Do you still need the IsSupported() method?  Looks like you could just inline
the code in the Create() method, and have that method return NULL if
unsupported.

http://codereview.chromium.org/10911271/diff/1/remoting/host/chromoting_host_...
File remoting/host/chromoting_host_context.h (right):

http://codereview.chromium.org/10911271/diff/1/remoting/host/chromoting_host_...
remoting/host/chromoting_host_context.h:76: // The path for the pipe used by the
Linux's audio capturer.
nit: remove "'s" ?

[Wez, 2012-09-13 00:32:55]

On 2012/09/13 00:18:26, Lambros wrote:
> lgtm
> Do you still need the IsSupported() method?  Looks like you could just inline
> the code in the Create() method, and have that method return NULL if
> unsupported.

It's needed when Alexey's DesktopEnvironment refactoring goes in, since the host
needs to know whether audio is supported before actually creating the capturer
once that lands.

[Sergey Ulanov, 2012-09-13 01:13:02]

http://codereview.chromium.org/10911271/diff/1/remoting/host/chromoting_host_...
File remoting/host/chromoting_host_context.h (right):

http://codereview.chromium.org/10911271/diff/1/remoting/host/chromoting_host_...
remoting/host/chromoting_host_context.h:76: // The path for the pipe used by the
Linux's audio capturer.
On 2012/09/13 00:18:27, Lambros wrote:
> nit: remove "'s" ?

Done.

[Wez, 2012-09-13 01:24:51]

ChromotingHostContext is used to manage the threads on which the host runs. The
pipe name for the audio capturer is part of the desktop environment
configuration, so it should be set on DesktopEnvironment, and passed to
AudioCapturer from there.

[Sergey Ulanov, 2012-09-13 01:56:10]

On 2012/09/13 01:24:51, Wez wrote:
> ChromotingHostContext is used to manage the threads on which the host runs.
The
> pipe name for the audio capturer is part of the desktop environment
> configuration, so it should be set on DesktopEnvironment, and passed to
> AudioCapturer from there.

I agree that this approach is not perfect, it's actually butt-ugly, but I wanted
something simple that fixes It2Me for now.
There is nothing that says that the host context must be used for threads only.
We want AudioCapturer interface to be platform-agnostic, yet pass platform
specific parameters to the implementation. So we can't just add parameter for
audio pipe name in the AudioCapturer::Create() function. Also I don't want to
add platform-specific details in DesktopEnvironment.
It feels that there will be other cases when we need to pass some configuration
details to platform specific code, VideoFrameCapturer::EnableXDamage() is an
example of that.
Probably the right way do that is to have some platform configuration object and
use it to pass platform-specific configuration from Host initialization code to
the actual capturers. That way DesktopEnvironment will not need to know about
platform-specific details. What do you thing about this approach?

[alexeypa (please no reviews), 2012-09-13 15:49:11]

On 2012/09/13 01:56:10, sergeyu wrote:
> On 2012/09/13 01:24:51, Wez wrote:
> > ChromotingHostContext is used to manage the threads on which the host runs.
> The
> > pipe name for the audio capturer is part of the desktop environment
> > configuration, so it should be set on DesktopEnvironment, and passed to
> > AudioCapturer from there.
> 
> I agree that this approach is not perfect, it's actually butt-ugly, but I
wanted
> something simple that fixes It2Me for now.
> There is nothing that says that the host context must be used for threads
only.
> We want AudioCapturer interface to be platform-agnostic, yet pass platform
> specific parameters to the implementation. So we can't just add parameter for
> audio pipe name in the AudioCapturer::Create() function. Also I don't want to
> add platform-specific details in DesktopEnvironment.
> It feels that there will be other cases when we need to pass some
configuration
> details to platform specific code, VideoFrameCapturer::EnableXDamage() is an
> example of that.
> Probably the right way do that is to have some platform configuration object
and
> use it to pass platform-specific configuration from Host initialization code
to
> the actual capturers. That way DesktopEnvironment will not need to know about
> platform-specific details. What do you thing about this approach?

DesktopEnvoronmentFactory is in the tree now. Platform-specific parameters can
be fed to platform-specific version of DesktopEnvoronmentFactory that will in
turn know how to create capturers.

[Do not use (sergeyu), 2012-09-13 20:23:09]

Adding chromium-reviews@ back to save this discussion where it belongs.

On Thu, Sep 13, 2012 at 12:59 PM, Wez <wez@google.com> wrote:

> Alexey: Thanks for the update on DesktopEnvironment, and congratulations!
> :P
>
> Sergey:  I think you're mixing up implementation vs interface concerns;
> you're right that DesktopEnvironment, AudioCapturer and event
> DesktopEnvironmentFactory are cross-platform and should not be aware of
> these details, but the actual implementation of AudioCapturer *is*
> platform-specific, so it's quite reasonable to pass platform-specific
> parameters to it at construction time.
>

Right, problem is that the code that creates platform-specific capturers
(DesktopEnvironmentFactory) is cross-platform and it uses cross-platform
static method (i.e. AudioCapturer::Create()) to create platform-specific
implementation. So you cannot easily pass platform-specific configuration
to it.


>  This CL avoids polluting interfaces that platform-specific code
> implements by instead polluting platform-agnostic code with the extra
> fields. ;)
>

Right I didn't argue with that.


>  In this case the FilePath should be passed to the
> platform/application-specific DesktopEnvironmentFactory, which should in
> turn pass it to the DE's it creates, to pass on to the AC's.
>

Isn't DesktopEnvironmentFactory supposed to be cross-platform? With what
Alex landed it's not an interface yet. Do we plan to make it an interface
with platform-specific implementations?


>
>
> ---------- Forwarded message ----------
> From: <alexeypa@chromium.org>
> Date: 13 September 2012 08:49
> Subject: Re: Remove CommandLine dependency from the Linux's AudioCapturer.
> (issue 10911271)
> To: sergeyu@chromium.org, wez@chromium.org, lambroslambrou@chromium.org
> Cc: chromium-reviews@chromium.org, jamiewalch+watch@chromium.org,
> dcaiafa+watch@chromium.org, simonmorris+watch@chromium.org,
> hclam+watch@chromium.org, wez+watch@chromium.org, amit@chromium.org,
> sanjeevr@chromium.org, garykac+watch@chromium.org,
> lambroslambrou+watch@chromium.org, rmsousa+watch@chromium.org,
> alexeypa+watch@chromium.org, sergeyu+watch@chromium.org
>
>
> On 2012/09/13 01:56:10, sergeyu wrote:
>
>> On 2012/09/13 01:24:51, Wez wrote:
>> > ChromotingHostContext is used to manage the threads on which the host
>> runs.
>> The
>> > pipe name for the audio capturer is part of the desktop environment
>> > configuration, so it should be set on DesktopEnvironment, and passed to
>> > AudioCapturer from there.
>>
>
>  I agree that this approach is not perfect, it's actually butt-ugly, but I
>>
> wanted
>
>> something simple that fixes It2Me for now.
>> There is nothing that says that the host context must be used for threads
>>
> only.
>
>> We want AudioCapturer interface to be platform-agnostic, yet pass platform
>> specific parameters to the implementation. So we can't just add parameter
>> for
>> audio pipe name in the AudioCapturer::Create() function. Also I don't
>> want to
>> add platform-specific details in DesktopEnvironment.
>> It feels that there will be other cases when we need to pass some
>>
> configuration
>
>> details to platform specific code, VideoFrameCapturer::**EnableXDamage()
>> is an
>> example of that.
>> Probably the right way do that is to have some platform configuration
>> object
>>
> and
>
>> use it to pass platform-specific configuration from Host initialization
>> code
>>
> to
>
>> the actual capturers. That way DesktopEnvironment will not need to know
>> about
>> platform-specific details. What do you thing about this approach?
>>
>
> DesktopEnvoronmentFactory is in the tree now. Platform-specific parameters
> can
> be fed to platform-specific version of DesktopEnvoronmentFactory that will
> in
> turn know how to create capturers.
>
>
http://codereview.chromium.**org/10911271/<http://codereview.chromium.org/109...
>
>


-- 
Sergey Ulanov

[alexeypa (please no reviews), 2012-09-13 20:31:24]

On 2012/09/13 20:23:09, sergeyu1 wrote:
> Isn't DesktopEnvironmentFactory supposed to be cross-platform? With what
> Alex landed it's not an interface yet. Do we plan to make it an interface
> with platform-specific implementations?

DesktopEnvironmentFactory is indeed supposed to be an interface. It is just that
this change will go in as a separate CL.

[Do not use (sergeyu), 2012-09-13 20:46:17]

On Thu, Sep 13, 2012 at 1:31 PM, <alexeypa@chromium.org> wrote:

> On 2012/09/13 20:23:09, sergeyu1 wrote:
>
>> Isn't DesktopEnvironmentFactory supposed to be cross-platform? With what
>> Alex landed it's not an interface yet. Do we plan to make it an interface
>> with platform-specific implementations?
>>
>
> DesktopEnvironmentFactory is indeed supposed to be an interface. It is
> just that
> this change will go in as a separate CL.
>

Right, but is it going to be interface with separate implementations per
platform? We may want to have separate implementations for IT2Me and for
Me2Me, but not sure if we want to have 6 of them (3 for it2me, 3 for me2me).




>
>
http://codereview.chromium.**org/10911271/<http://codereview.chromium.org/109...
>


-- 
Sergey Ulanov

[Wez, 2012-09-13 20:57:48]

On 2012/09/13 20:46:17, sergeyu1 wrote:
> On Thu, Sep 13, 2012 at 1:31 PM, <mailto:alexeypa@chromium.org> wrote:
> 
> > On 2012/09/13 20:23:09, sergeyu1 wrote:
> >
> >> Isn't DesktopEnvironmentFactory supposed to be cross-platform? With what
> >> Alex landed it's not an interface yet. Do we plan to make it an interface
> >> with platform-specific implementations?
> >>
> >
> > DesktopEnvironmentFactory is indeed supposed to be an interface. It is
> > just that
> > this change will go in as a separate CL.
> >
> 
> Right, but is it going to be interface with separate implementations per
> platform? We may want to have separate implementations for IT2Me and for
> Me2Me, but not sure if we want to have 6 of them (3 for it2me, 3 for me2me).

DesktopEnvironmentFactory is the cross-platform interface that ChromotingHost
uses to get platform/app-specific DesktopEnvironment implementations, etc. The
implementation of DesktopEnvironmentFactory is intrinsically platform and/or app
specific; it _may_ differ between applications such as IT2Me or Me2Me, or they
may use the same implementation, but either way, it's the platform-specific
application's responsibility to pass in any platform-specific data that the
particular implementation they want to use requires.

As we move to the daemon architecture Me2Me will move to running the guts of
current DesktopEnvironment code, shared with IT2Me, in the integration process,
and will interface to that through a daemon-aware DesktopEnvironment
implementation - the latter could be mostly cross-platform, but with different
ctor parameters to cope with the differences in e.g. IPC on the three platforms,
for example.

[Sergey Ulanov, 2012-09-13 21:58:09]

Ok, then, I've uploaded new version of this CL - it uses static value to pass
the parameter from main() to AudioCapturerLinux. I'd like to commit this first
to fix problem so that It2Me is not broken in M23. I'll refactor this code to
use DesktopEnvironmentFactory later. PTAL.

On 2012/09/13 20:57:48, Wez wrote:
> On 2012/09/13 20:46:17, sergeyu1 wrote:
> > On Thu, Sep 13, 2012 at 1:31 PM, <mailto:alexeypa@chromium.org> wrote:
> > 
> > > On 2012/09/13 20:23:09, sergeyu1 wrote:
> > >
> > >> Isn't DesktopEnvironmentFactory supposed to be cross-platform? With what
> > >> Alex landed it's not an interface yet. Do we plan to make it an interface
> > >> with platform-specific implementations?
> > >>
> > >
> > > DesktopEnvironmentFactory is indeed supposed to be an interface. It is
> > > just that
> > > this change will go in as a separate CL.
> > >
> > 
> > Right, but is it going to be interface with separate implementations per
> > platform? We may want to have separate implementations for IT2Me and for
> > Me2Me, but not sure if we want to have 6 of them (3 for it2me, 3 for me2me).
> 
> DesktopEnvironmentFactory is the cross-platform interface that ChromotingHost
> uses to get platform/app-specific DesktopEnvironment implementations, etc. The
> implementation of DesktopEnvironmentFactory is intrinsically platform and/or
app
> specific; it _may_ differ between applications such as IT2Me or Me2Me, or they
> may use the same implementation, but either way, it's the platform-specific
> application's responsibility to pass in any platform-specific data that the
> particular implementation they want to use requires.
> 
> As we move to the daemon architecture Me2Me will move to running the guts of
> current DesktopEnvironment code, shared with IT2Me, in the integration
process,
> and will interface to that through a daemon-aware DesktopEnvironment
> implementation - the latter could be mostly cross-platform, but with different
> ctor parameters to cope with the differences in e.g. IPC on the three
platforms,
> for example.

[Wez, 2012-09-14 01:20:53]

lgtm

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

http://codereview.chromium.org/10911271/diff/11001/remoting/host/audio_captur...
remoting/host/audio_capturer_linux.h:21: // Must be called to initialize the
capturer before the first instance is
nit: initialize -> configure

[Sergey Ulanov, 2012-09-14 01:25:32]

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

http://codereview.chromium.org/10911271/diff/11001/remoting/host/audio_captur...
remoting/host/audio_capturer_linux.h:21: // Must be called to initialize the
capturer before the first instance is
On 2012/09/14 01:20:54, Wez wrote:
> nit: initialize -> configure

Done.

[commit-bot: I haz the power, 2012-09-14 01:27:40]

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

[commit-bot: I haz the power, 2012-09-14 04:22:23]

Change committed as 156739