Added piping for sending audio packets from host to client.

Added piping for sending audio packets from host to client.


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

[Wez, 2012-06-19 03:29:31]

http://codereview.chromium.org/10532211/diff/1/remoting/client/chromoting_cli...
File remoting/client/chromoting_client.h (right):

http://codereview.chromium.org/10532211/diff/1/remoting/client/chromoting_cli...
remoting/client/chromoting_client.h:76: virtual int GetPendingPackets()
OVERRIDE;
nit: This should be renamed PendingVideoPacketCount(), since we now have both
video and audio packets.

http://codereview.chromium.org/10532211/diff/1/remoting/protocol/connection_t...
File remoting/protocol/connection_to_client.cc (right):

http://codereview.chromium.org/10532211/diff/1/remoting/protocol/connection_t...
remoting/protocol/connection_to_client.cc:171: audio_writer_.get() &&
audio_writer_->is_connected()) {
This assumes that the remote party also wants an audio channel, so that one will
get set up - what if the remote end doesn't ask for/offer audio?

[kxing, 2012-06-20 22:30:06]

http://codereview.chromium.org/10532211/diff/1/remoting/client/chromoting_cli...
File remoting/client/chromoting_client.h (right):

http://codereview.chromium.org/10532211/diff/1/remoting/client/chromoting_cli...
remoting/client/chromoting_client.h:76: virtual int GetPendingPackets()
OVERRIDE;
This CL is already getting pretty huge.

I'll do this in https://code.google.com/p/chromium/issues/detail?id=133823

http://codereview.chromium.org/10532211/diff/1/remoting/protocol/connection_t...
File remoting/protocol/connection_to_client.cc (right):

http://codereview.chromium.org/10532211/diff/1/remoting/protocol/connection_t...
remoting/protocol/connection_to_client.cc:171: audio_writer_.get() &&
audio_writer_->is_connected()) {
On 2012/06/19 03:29:31, Wez wrote:
> This assumes that the remote party also wants an audio channel, so that one
will
> get set up - what if the remote end doesn't ask for/offer audio?

Done.

[kxing, 2012-06-21 13:45:04]

Ping

[Sergey Ulanov, 2012-06-21 17:24:56]

Looks mostly good. I disagree with the changes in ScreenRecorder and have some
other minor suggestions.

http://codereview.chromium.org/10532211/diff/9001/remoting/host/screen_record...
File remoting/host/screen_recorder.h (right):

http://codereview.chromium.org/10532211/diff/9001/remoting/host/screen_record...
remoting/host/screen_recorder.h:137: void
DoSendAudioPacket(scoped_ptr<AudioPacket> packet);
I'm not convinced we should put audio-related code in ScreenRecorder. I think we
will need a separate class that will worry about managing audio (capturing +
encoding), call it e.g. AudioRecorder (though there must be a better name for
it).
For this CL you can just revert all changes in ScreenRecorder - there is nothing
that depends on these changes in this CL anyway.

http://codereview.chromium.org/10532211/diff/9001/remoting/protocol/connectio...
File remoting/protocol/connection_to_client.cc (right):

http://codereview.chromium.org/10532211/diff/9001/remoting/protocol/connectio...
remoting/protocol/connection_to_client.cc:131: audio_writer_ =
AudioWriter::Create(session_->config());
Change AudioWriter::Create() to return NULL when audio is disabled, then this
code will be simpler.

http://codereview.chromium.org/10532211/diff/9001/remoting/protocol/connectio...
remoting/protocol/connection_to_client.cc:175: (!use_audio_ ||
(audio_writer_.get() && audio_writer_->is_connected()))) {
This code potentially may be called even before you set use_audio_ in line 128.
I suggest that you remove use_audio_ flag and check
session_->config().audio_config().transport directly in this method. Or maybe
add audio_enabled() method in this class.

http://codereview.chromium.org/10532211/diff/9001/remoting/protocol/connectio...
File remoting/protocol/connection_to_client.h (right):

http://codereview.chromium.org/10532211/diff/9001/remoting/protocol/connectio...
remoting/protocol/connection_to_client.h:130: // whether there is audio
Comments should start with a capital letter and end with a period. E.g. "Set to
true when audio stream is present.".

http://codereview.chromium.org/10532211/diff/9001/remoting/protocol/session_c...
File remoting/protocol/session_config.cc (right):

http://codereview.chromium.org/10532211/diff/9001/remoting/protocol/session_c...
remoting/protocol/session_config.cc:181:
result->mutable_audio_configs()->push_back(
Please make TRANSPORT_NONE the first audio config in the list, so that it's not
used  until it works properly.

[kxing, 2012-06-21 18:09:22]

http://codereview.chromium.org/10532211/diff/9001/remoting/host/screen_record...
File remoting/host/screen_recorder.h (right):

http://codereview.chromium.org/10532211/diff/9001/remoting/host/screen_record...
remoting/host/screen_recorder.h:137: void
DoSendAudioPacket(scoped_ptr<AudioPacket> packet);
On 2012/06/21 17:24:56, sergeyu wrote:
> I'm not convinced we should put audio-related code in ScreenRecorder. I think
we
> will need a separate class that will worry about managing audio (capturing +
> encoding), call it e.g. AudioRecorder (though there must be a better name for
> it).
> For this CL you can just revert all changes in ScreenRecorder - there is
nothing
> that depends on these changes in this CL anyway.

Done.

http://codereview.chromium.org/10532211/diff/9001/remoting/protocol/connectio...
File remoting/protocol/connection_to_client.cc (right):

http://codereview.chromium.org/10532211/diff/9001/remoting/protocol/connectio...
remoting/protocol/connection_to_client.cc:131: audio_writer_ =
AudioWriter::Create(session_->config());
Even if AudioWriter::Create() returns NULL, wouldn't we still have to check
accordingly, since the next thing we do is call Init() on the scoped_ptr?

http://codereview.chromium.org/10532211/diff/9001/remoting/protocol/connectio...
remoting/protocol/connection_to_client.cc:175: (!use_audio_ ||
(audio_writer_.get() && audio_writer_->is_connected()))) {
On 2012/06/21 17:24:56, sergeyu wrote:
> This code potentially may be called even before you set use_audio_ in line
128.
> I suggest that you remove use_audio_ flag and check
> session_->config().audio_config().transport directly in this method. Or maybe
> add audio_enabled() method in this class.

Done.

http://codereview.chromium.org/10532211/diff/9001/remoting/protocol/connectio...
File remoting/protocol/connection_to_client.h (right):

http://codereview.chromium.org/10532211/diff/9001/remoting/protocol/connectio...
remoting/protocol/connection_to_client.h:130: // whether there is audio
On 2012/06/21 17:24:56, sergeyu wrote:
> Comments should start with a capital letter and end with a period. E.g. "Set
to
> true when audio stream is present.".

Done.

http://codereview.chromium.org/10532211/diff/9001/remoting/protocol/session_c...
File remoting/protocol/session_config.cc (right):

http://codereview.chromium.org/10532211/diff/9001/remoting/protocol/session_c...
remoting/protocol/session_config.cc:181:
result->mutable_audio_configs()->push_back(
On 2012/06/21 17:24:56, sergeyu wrote:
> Please make TRANSPORT_NONE the first audio config in the list, so that it's
not
> used  until it works properly.

Done.

[kxing, 2012-06-21 18:12:45]

Also, should we call the host audio recorder class AudioRecorder? Or is there a
better name?

[Sergey Ulanov, 2012-06-21 18:20:45]

http://codereview.chromium.org/10532211/diff/9001/remoting/protocol/connectio...
File remoting/protocol/connection_to_client.cc (right):

http://codereview.chromium.org/10532211/diff/9001/remoting/protocol/connectio...
remoting/protocol/connection_to_client.cc:131: audio_writer_ =
AudioWriter::Create(session_->config());
On 2012/06/21 18:09:22, kxing wrote:
> Even if AudioWriter::Create() returns NULL, wouldn't we still have to check
> accordingly, since the next thing we do is call Init() on the scoped_ptr?

Yes, you will need to check NULL before calling Init.

[Sergey Ulanov, 2012-06-21 18:36:04]

http://codereview.chromium.org/10532211/diff/14002/remoting/protocol/connecti...
File remoting/protocol/connection_to_client.cc (right):

http://codereview.chromium.org/10532211/diff/14002/remoting/protocol/connecti...
remoting/protocol/connection_to_client.cc:191: bool
ConnectionToClient::AudioEnabled() {
Actually it's better to call it is_audio_enabled()

http://codereview.chromium.org/10532211/diff/14002/remoting/protocol/connecti...
remoting/protocol/connection_to_client.cc:192: return
(session_->config().audio_config().transport !=
nit: don't need parenthesis here.

http://codereview.chromium.org/10532211/diff/14002/remoting/protocol/session_...
File remoting/protocol/session_config.cc (right):

http://codereview.chromium.org/10532211/diff/14002/remoting/protocol/session_...
remoting/protocol/session_config.cc:182: ChannelConfig::CODEC_UNDEFINED));
Also need to define codec? CODEC_VERBATIM for raw audio stream, and CODEC_VORBIS
for vorbis-encoded stream.

http://codereview.chromium.org/10532211/diff/14002/remoting/protocol/session_...
File remoting/protocol/session_config.h (right):

http://codereview.chromium.org/10532211/diff/14002/remoting/protocol/session_...
remoting/protocol/session_config.h:33: CODEC_VP8,
add CODEC_VORBIS here for the audio?

[kxing, 2012-06-21 18:53:28]

http://codereview.chromium.org/10532211/diff/14002/remoting/protocol/connecti...
File remoting/protocol/connection_to_client.cc (right):

http://codereview.chromium.org/10532211/diff/14002/remoting/protocol/connecti...
remoting/protocol/connection_to_client.cc:191: bool
ConnectionToClient::AudioEnabled() {
On 2012/06/21 18:36:04, sergeyu wrote:
> Actually it's better to call it is_audio_enabled()

Done.

http://codereview.chromium.org/10532211/diff/14002/remoting/protocol/connecti...
remoting/protocol/connection_to_client.cc:192: return
(session_->config().audio_config().transport !=
On 2012/06/21 18:36:04, sergeyu wrote:
> nit: don't need parenthesis here.

Done.

http://codereview.chromium.org/10532211/diff/14002/remoting/protocol/session_...
File remoting/protocol/session_config.cc (right):

http://codereview.chromium.org/10532211/diff/14002/remoting/protocol/session_...
remoting/protocol/session_config.cc:182: ChannelConfig::CODEC_UNDEFINED));
On 2012/06/21 18:36:04, sergeyu wrote:
> Also need to define codec? CODEC_VERBATIM for raw audio stream, and
CODEC_VORBIS
> for vorbis-encoded stream.

Done.

http://codereview.chromium.org/10532211/diff/14002/remoting/protocol/session_...
File remoting/protocol/session_config.h (right):

http://codereview.chromium.org/10532211/diff/14002/remoting/protocol/session_...
remoting/protocol/session_config.h:33: CODEC_VP8,
On 2012/06/21 18:36:04, sergeyu wrote:
> add CODEC_VORBIS here for the audio?

Done.

[kxing, 2012-06-22 14:11:41]

Ping

[Wez, 2012-06-22 18:29:52]

http://codereview.chromium.org/10532211/diff/21001/remoting/client/chromoting...
File remoting/client/chromoting_client.cc (right):

http://codereview.chromium.org/10532211/diff/21001/remoting/client/chromoting...
remoting/client/chromoting_client.cc:11: #include "remoting/proto/audio.pb.h"
nit: Why do we need to include audio.pb.h here; we don't need to include
video.pb.h?  If VideoPacket is being implicitly pulled in via some other header
then we should also include video.pb.h for IWYU.

http://codereview.chromium.org/10532211/diff/21001/remoting/protocol/connecti...
File remoting/protocol/connection_to_client.cc (right):

http://codereview.chromium.org/10532211/diff/21001/remoting/protocol/connecti...
remoting/protocol/connection_to_client.cc:129: if (audio_writer_ != NULL) {
nit: if (audio_writer_.get()) seems clearer here, since audio_writer_ is a
scoped_ptr<>, not a bare pointer. Consider adding a comment to indicate that the
audio channel is optional.

http://codereview.chromium.org/10532211/diff/21001/remoting/protocol/connecti...
remoting/protocol/connection_to_client.cc:174: (audio_writer_.get() &&
audio_writer_->is_connected()))) {
nit: This if has become silly; we should re-express this as a series of
early-exits, i.e.

if (!control_dispatcher_.get() || !control_dispatcher_.is_connected())
  return;
...

http://codereview.chromium.org/10532211/diff/21001/remoting/protocol/connecti...
File remoting/protocol/connection_to_client.h (right):

http://codereview.chromium.org/10532211/diff/21001/remoting/protocol/connecti...
remoting/protocol/connection_to_client.h:87: // Send audio stream data to the
viewer.
nit: Is it possible that this be NULL if audio is not enabled?

http://codereview.chromium.org/10532211/diff/21001/remoting/protocol/connecti...
File remoting/protocol/connection_to_host.cc (right):

http://codereview.chromium.org/10532211/diff/21001/remoting/protocol/connecti...
remoting/protocol/connection_to_host.cc:226: (audio_reader_.get() &&
audio_reader_->is_connected())) &&
nit: Similarly, this would be clearer as a series of early-exits.

http://codereview.chromium.org/10532211/diff/21001/remoting/protocol/connecti...
remoting/protocol/connection_to_host.cc:262: return
(session_->config().audio_config().transport !=
nit: SessionConfig has an is_audio_enabled() member that does this, I think?

http://codereview.chromium.org/10532211/diff/21001/remoting/protocol/connecti...
File remoting/protocol/connection_to_host.h (right):

http://codereview.chromium.org/10532211/diff/21001/remoting/protocol/connecti...
remoting/protocol/connection_to_host.h:123: bool AudioEnabled();
nit: Get rid of this and replace the call-sites with
config()->is_audio_enabled()?

http://codereview.chromium.org/10532211/diff/21001/remoting/protocol/content_...
File remoting/protocol/content_description.cc (right):

http://codereview.chromium.org/10532211/diff/21001/remoting/protocol/content_...
remoting/protocol/content_description.cc:46: const char kVorbisCodec[] =
"vorbis";
Have you checked that all calling codesites cope gracefully with unexpected
codecs, e.g. if I specify a vorbis video tag, will the host correctly reject the
config?

http://codereview.chromium.org/10532211/diff/21001/remoting/protocol/content_...
remoting/protocol/content_description.cc:282: }
nit: We've got four all-but-identical blocks of code here; can we fold them into
a single block w/ iterator over the known channel names?  Since calling code
can't cope with "none" for anything but the audio-channel, the audio-channel
special-case can be handled by checking the number of audio_configs() after
we've processed the XML, and adding a "none" config if that is zero.

http://codereview.chromium.org/10532211/diff/21001/remoting/protocol/session_...
File remoting/protocol/session_config.cc (right):

http://codereview.chromium.org/10532211/diff/21001/remoting/protocol/session_...
remoting/protocol/session_config.cc:48: ChannelConfig::CODEC_UNDEFINED));
Why is the default audio channel config "undefined"? Surely we want
CODEC_VERBATIM (I assume we'll use this for PCM audio?) or CODEC_VORBIS, or not
to specify audio in the default config at all?  Or did you mean transport to be
"none" here?

[kxing, 2012-06-22 19:12:31]

http://codereview.chromium.org/10532211/diff/21001/remoting/client/chromoting...
File remoting/client/chromoting_client.cc (right):

http://codereview.chromium.org/10532211/diff/21001/remoting/client/chromoting...
remoting/client/chromoting_client.cc:11: #include "remoting/proto/audio.pb.h"
On 2012/06/22 18:29:53, Wez wrote:
> nit: Why do we need to include audio.pb.h here; we don't need to include
> video.pb.h?  If VideoPacket is being implicitly pulled in via some other
header
> then we should also include video.pb.h for IWYU.

Done.

http://codereview.chromium.org/10532211/diff/21001/remoting/protocol/connecti...
File remoting/protocol/connection_to_client.cc (right):

http://codereview.chromium.org/10532211/diff/21001/remoting/protocol/connecti...
remoting/protocol/connection_to_client.cc:129: if (audio_writer_ != NULL) {
On 2012/06/22 18:29:53, Wez wrote:
> nit: if (audio_writer_.get()) seems clearer here, since audio_writer_ is a
> scoped_ptr<>, not a bare pointer. Consider adding a comment to indicate that
the
> audio channel is optional.

Done.

http://codereview.chromium.org/10532211/diff/21001/remoting/protocol/connecti...
remoting/protocol/connection_to_client.cc:174: (audio_writer_.get() &&
audio_writer_->is_connected()))) {
On 2012/06/22 18:29:53, Wez wrote:
> nit: This if has become silly; we should re-express this as a series of
> early-exits, i.e.
> 
> if (!control_dispatcher_.get() || !control_dispatcher_.is_connected())
>   return;
> ...

Done.

http://codereview.chromium.org/10532211/diff/21001/remoting/protocol/connecti...
File remoting/protocol/connection_to_client.h (right):

http://codereview.chromium.org/10532211/diff/21001/remoting/protocol/connecti...
remoting/protocol/connection_to_client.h:87: // Send audio stream data to the
viewer.
On 2012/06/22 18:29:53, Wez wrote:
> nit: Is it possible that this be NULL if audio is not enabled?

Done.

http://codereview.chromium.org/10532211/diff/21001/remoting/protocol/connecti...
File remoting/protocol/connection_to_host.cc (right):

http://codereview.chromium.org/10532211/diff/21001/remoting/protocol/connecti...
remoting/protocol/connection_to_host.cc:226: (audio_reader_.get() &&
audio_reader_->is_connected())) &&
On 2012/06/22 18:29:53, Wez wrote:
> nit: Similarly, this would be clearer as a series of early-exits.

Done.

http://codereview.chromium.org/10532211/diff/21001/remoting/protocol/connecti...
remoting/protocol/connection_to_host.cc:262: return
(session_->config().audio_config().transport !=
On 2012/06/22 18:29:53, Wez wrote:
> nit: SessionConfig has an is_audio_enabled() member that does this, I think?

Done.

http://codereview.chromium.org/10532211/diff/21001/remoting/protocol/connecti...
File remoting/protocol/connection_to_host.h (right):

http://codereview.chromium.org/10532211/diff/21001/remoting/protocol/connecti...
remoting/protocol/connection_to_host.h:123: bool AudioEnabled();
On 2012/06/22 18:29:53, Wez wrote:
> nit: Get rid of this and replace the call-sites with
> config()->is_audio_enabled()?

Done.

http://codereview.chromium.org/10532211/diff/21001/remoting/protocol/content_...
File remoting/protocol/content_description.cc (right):

http://codereview.chromium.org/10532211/diff/21001/remoting/protocol/content_...
remoting/protocol/content_description.cc:46: const char kVorbisCodec[] =
"vorbis";
I think so:

remoting/client/rectangle_update_decoder.cc checks for it correctly
remoting/host/chromoting_host.cc checks for it correctly
remoting/host/simple_host_process.cc checks for it correctly
remoting/protocol/video_reader.cc checks for it correctly

Let me know if I'm missing something.

http://codereview.chromium.org/10532211/diff/21001/remoting/protocol/content_...
remoting/protocol/content_description.cc:282: }
This might be a bit tricky, since video tags require a codec (the second
parameter to ParseChannelConfig is true). Do you think doing special case
handling in the middle of the loop is worth it?

http://codereview.chromium.org/10532211/diff/21001/remoting/protocol/session_...
File remoting/protocol/session_config.cc (right):

http://codereview.chromium.org/10532211/diff/21001/remoting/protocol/session_...
remoting/protocol/session_config.cc:48: ChannelConfig::CODEC_UNDEFINED));
Right, I meant CODEC_VERBATIM.

[Wez, 2012-06-22 21:14:06]

LGTM but see comment.

http://codereview.chromium.org/10532211/diff/21001/remoting/protocol/content_...
File remoting/protocol/content_description.cc (right):

http://codereview.chromium.org/10532211/diff/21001/remoting/protocol/content_...
remoting/protocol/content_description.cc:282: }
On 2012/06/22 19:12:31, kxing wrote:
> This might be a bit tricky, since video tags require a codec (the second
> parameter to ParseChannelConfig is true). Do you think doing special case
> handling in the middle of the loop is worth it?

Good point.  You also want audio tags to have a codec property, I think, but
you're passing false to ParseChannelConfig() for those right now.

You'd need a structure to iterate over that contains the channel name and a
pointer to the mutable configs structure for it, so adding a needs-codec flag to
that would seem reasonable. Alternatively you could just have a static method
that, given a channel name, returns true if that channel needs a codec, false
otherwise.

[kxing, 2012-06-22 21:21:37]

I'll commit pretty soon, unless someone objects.

http://codereview.chromium.org/10532211/diff/21001/remoting/protocol/content_...
File remoting/protocol/content_description.cc (right):

http://codereview.chromium.org/10532211/diff/21001/remoting/protocol/content_...
remoting/protocol/content_description.cc:282: }
Sure. I'll do that in https://code.google.com/p/chromium/issues/detail?id=134222

[commit-bot: I haz the power, 2012-06-22 21:48:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kxing@chromium.org/10532211/19003

[commit-bot: I haz the power, 2012-06-22 22:01:14]

Try job failure for 10532211-19003 (retry) on mac_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-06-25 15:18:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kxing@chromium.org/10532211/31007

[commit-bot: I haz the power, 2012-06-25 15:54:58]

Try job failure for 10532211-31007 (retry) on linux_rel for step
"remoting_unittests".
It's a second try, previously, step "remoting_unittests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[kxing, 2012-06-25 17:29:38]

PTAL

[Wez, 2012-06-25 17:47:46]

lgtm

[commit-bot: I haz the power, 2012-06-25 17:48:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kxing@chromium.org/10532211/38002

[commit-bot: I haz the power, 2012-06-25 19:26:35]

Change committed as 143977