Add Audio support in Chromoting host when using WebRTC.

Add Audio support in Chromoting host when using WebRTC.

This change adds WebrtcVideoStream that sends audio from the host to
the client. Audio is sent in a media stream separate from video.

BUG=638505
Committed: https://crrev.com/24b257a192875a00ab9de76f52d893ff9b9cad21
Cr-Commit-Position: refs/heads/master@{#424090}

[nicholss, 2016-10-04 21:56:59]

nicholss@chromium.org changed reviewers:
+ nicholss@chromium.org

[nicholss, 2016-10-04 21:56:59]

LGTM

[Sergey Ulanov, 2016-10-04 22:33:12]

sergeyu@chromium.org changed reviewers:
+ jamiewalch@chromium.org

[Sergey Ulanov, 2016-10-04 22:33:12]

+jamiewalch@

[Sergey Ulanov, 2016-10-04 22:39:44]

Also updated WebrtcConnectionToClient to use WebrtcAudioStream. Sorry I forgot
to add that change.

[Jamie, 2016-10-04 23:36:06]

https://codereview.chromium.org/2392963003/diff/40001/remoting/protocol/webrt...
File remoting/protocol/webrtc_audio_source_adapter.cc (right):

https://codereview.chromium.org/2392963003/diff/40001/remoting/protocol/webrt...
remoting/protocol/webrtc_audio_source_adapter.cc:24: class
WebrtcAudioSourceAdapter::Core{
Nit: Space after Core

https://codereview.chromium.org/2392963003/diff/40001/remoting/protocol/webrt...
remoting/protocol/webrtc_audio_source_adapter.cc:42: std::vector<uint8_t>
leftover_samples_;
I'm not sure I fully understand the use of this member. If my reading of the
code is correct, I think something like |partial_sample_| (note the singular)
might be a better name.

https://codereview.chromium.org/2392963003/diff/40001/remoting/protocol/webrt...
remoting/protocol/webrtc_audio_source_adapter.cc:52: }
Nit: Add blank line

https://codereview.chromium.org/2392963003/diff/40001/remoting/protocol/webrt...
remoting/protocol/webrtc_audio_source_adapter.cc:106: 
If I'm reading this correctly, the intent is to ensure that each audio sink only
receives complete frames of data, even if the audio packets contain less than
that. I don't think it does that though; consider the case where two consecutive
packets arrive with total payload < bytes_per_frame. The first will be queued in
leftover_samples_, only to be sent when the second arrives; if that's the
intent, then why not send the first one as soon as it arrives, as would have
been done if it had been >= bytes_per_frame?

https://codereview.chromium.org/2392963003/diff/40001/remoting/protocol/webrt...
File remoting/protocol/webrtc_connection_to_client.cc (right):

https://codereview.chromium.org/2392963003/diff/40001/remoting/protocol/webrt...
remoting/protocol/webrtc_connection_to_client.cc:93: return nullptr;
Do we get an error log in Start()? If not then please add one here.

[Sergey Ulanov, 2016-10-05 21:52:24]

https://codereview.chromium.org/2392963003/diff/40001/remoting/protocol/webrt...
File remoting/protocol/webrtc_audio_source_adapter.cc (right):

https://codereview.chromium.org/2392963003/diff/40001/remoting/protocol/webrt...
remoting/protocol/webrtc_audio_source_adapter.cc:24: class
WebrtcAudioSourceAdapter::Core{
On 2016/10/04 23:36:05, Jamie wrote:
> Nit: Space after Core

Done.

https://codereview.chromium.org/2392963003/diff/40001/remoting/protocol/webrt...
remoting/protocol/webrtc_audio_source_adapter.cc:42: std::vector<uint8_t>
leftover_samples_;
On 2016/10/04 23:36:05, Jamie wrote:
> I'm not sure I fully understand the use of this member. If my reading of the
> code is correct, I think something like |partial_sample_| (note the singular)
> might be a better name.

webrtc::AudioTrackSinkInterface expects to get audio in 10ms frames (see
kAudioFrameDuration), i.e. 480 samples per frame at 48kHz. But
remoting::AudioSource doesn't have any guarantees the size of the AudioPacket's
it generates (each incoming packet still contains whole samples, so
leftover_samples_ will never contains partial sample). So if we get 45ms of
samples then 40ms are passed to the sinks and 5ms is kept until we receive 5
more ms.
I've renamed this vector to |partial_frame_| now. Also added comments to make it
clear why we need it here.

https://codereview.chromium.org/2392963003/diff/40001/remoting/protocol/webrt...
remoting/protocol/webrtc_audio_source_adapter.cc:52: }
On 2016/10/04 23:36:05, Jamie wrote:
> Nit: Add blank line

Done.

https://codereview.chromium.org/2392963003/diff/40001/remoting/protocol/webrt...
remoting/protocol/webrtc_audio_source_adapter.cc:106: 
On 2016/10/04 23:36:05, Jamie wrote:
> If I'm reading this correctly, the intent is to ensure that each audio sink
only
> receives complete frames of data, even if the audio packets contain less than
> that. I don't think it does that though; consider the case where two
consecutive
> packets arrive with total payload < bytes_per_frame. The first will be queued
in
> leftover_samples_, only to be sent when the second arrives; if that's the
> intent, then why not send the first one as soon as it arrives, as would have
> been done if it had been >= bytes_per_frame?

This is a good catch. I wrote this code with incorrect assumption that
AudioPacket is always bigger than a single frame (e.g. 34ms and then 26ms would
generate 6 frames, 10ms each), but it doesn't work correctly when packets are
smaller than 10ms. Fixed now. Also added a unittest for this adapter.

https://codereview.chromium.org/2392963003/diff/40001/remoting/protocol/webrt...
File remoting/protocol/webrtc_connection_to_client.cc (right):

https://codereview.chromium.org/2392963003/diff/40001/remoting/protocol/webrt...
remoting/protocol/webrtc_connection_to_client.cc:93: return nullptr;
On 2016/10/04 23:36:06, Jamie wrote:
> Do we get an error log in Start()? If not then please add one here.

Actually Start() is not expected to fail. Updated it to return void and added
DCHECK in Start().
Also updated WebrtcVideoStream for consistency

[Jamie, 2016-10-05 22:08:11]

lgtm

https://codereview.chromium.org/2392963003/diff/60001/remoting/protocol/webrt...
File remoting/protocol/webrtc_audio_source_adapter.cc (right):

https://codereview.chromium.org/2392963003/diff/60001/remoting/protocol/webrt...
remoting/protocol/webrtc_audio_source_adapter.cc:123: }
Here, |partial_frame_| actually contains a complete frame. I don't think it's
worth changing the member name, but I think a comment (and perhaps a
DCHECK(partial_frame_.size() == bytes_per_frame)) would avoid confusion at this
point.

[Sergey Ulanov, 2016-10-08 05:33:57]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-10-08 05:33:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
nicholss@chromium.org, jamiewalch@chromium.org
Link to the patchset: https://codereview.chromium.org/2392963003/#ps80001
(title: ".")

[commit-bot: I haz the power, 2016-10-08 05:34:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sergey Ulanov, 2016-10-08 05:52:47]

https://codereview.chromium.org/2392963003/diff/60001/remoting/protocol/webrt...
File remoting/protocol/webrtc_audio_source_adapter.cc (right):

https://codereview.chromium.org/2392963003/diff/60001/remoting/protocol/webrt...
remoting/protocol/webrtc_audio_source_adapter.cc:123: }
On 2016/10/05 22:08:11, Jamie wrote:
> Here, |partial_frame_| actually contains a complete frame. I don't think it's
> worth changing the member name, but I think a comment (and perhaps a
> DCHECK(partial_frame_.size() == bytes_per_frame)) would avoid confusion at
this
> point.

Done.

[commit-bot: I haz the power, 2016-10-08 05:55:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-08 05:55:03]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Sergey Ulanov, 2016-10-08 06:17:50]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-10-08 06:17:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
nicholss@chromium.org, jamiewalch@chromium.org
Link to the patchset: https://codereview.chromium.org/2392963003/#ps100001
(title: "<numeric>")

[commit-bot: I haz the power, 2016-10-08 06:18:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-08 07:08:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-08 07:08:51]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sergey Ulanov, 2016-10-08 21:28:50]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-10-08 21:28:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
nicholss@chromium.org, jamiewalch@chromium.org
Link to the patchset: https://codereview.chromium.org/2392963003/#ps120001
(title: "fix leak")

[commit-bot: I haz the power, 2016-10-08 21:29:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-08 21:41:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-08 21:41:07]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Sergey Ulanov, 2016-10-09 01:54:34]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-10-09 01:54:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
nicholss@chromium.org, jamiewalch@chromium.org
Link to the patchset: https://codereview.chromium.org/2392963003/#ps130001
(title: ".")

[commit-bot: I haz the power, 2016-10-09 01:54:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-09 02:47:30]

Committed patchset #8 (id:130001)

[commit-bot: I haz the power, 2016-10-09 02:49:15]

Description was changed from

==========
Add Audio support in Chromoting host when using WebRTC.

This change adds WebrtcVideoStream that sends audio from the host to
the client. Audio is sent in a media stream separate from video.

BUG=638505
==========

to

==========
Add Audio support in Chromoting host when using WebRTC.

This change adds WebrtcVideoStream that sends audio from the host to
the client. Audio is sent in a media stream separate from video.

BUG=638505

Committed: https://crrev.com/24b257a192875a00ab9de76f52d893ff9b9cad21
Cr-Commit-Position: refs/heads/master@{#424090}
==========

[commit-bot: I haz the power, 2016-10-09 02:49:16]

Patchset 8 (id:??) landed as
https://crrev.com/24b257a192875a00ab9de76f52d893ff9b9cad21
Cr-Commit-Position: refs/heads/master@{#424090}