Add audio support in WebrtcConnectionToHost, audio unittest

Add audio support in WebrtcConnectionToHost, audio unittest

WebrtcConnectionToHost now supports audio streaming.
Also added a unittests that tests audio streaming, both for ICE and
WebRTC protocols.

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

[Sergey Ulanov, 2016-10-04 22:41:48]

Patchset #1 (id:1) has been deleted

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

Description was changed from

==========
Audio
==========

to

==========
Add audio support in WebrtcConnectionToHost, audio unittest

WebrtcConnectionToHost now supports audio streaming.
Also added a unittests that tests audio streaming, both for ICE and 
WebRTC protocols.

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

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

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

[nicholss, 2016-10-04 23:18:42]

LGTM

I am wondering why there is so much implementation going on inside of the
protocol lib. It seems like it would be more portable if we had no logic inside
of the protocol and had implementation libs that were for webrtc and the old
way. This is one of the things that makes it very hard to be able to reuse the
chromoting code in another repo, this heavily ties the protocol to all the
dependencies needed for all implementations rather than just what is required
for passing the raw data between client and host.

https://codereview.chromium.org/2371323007/diff/20001/remoting/protocol/webrt...
File remoting/protocol/webrtc_audio_sink_adapter.cc (right):

https://codereview.chromium.org/2371323007/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_audio_sink_adapter.cc:76:
audio_stub_->ProcessAudioPacket(std::move(audio_packet), base::Closure());
It feels wrong to have this amount of implementation in the protocol layer. Is
this the best place to have the details of how we pump data out of webrtc and
into our audio sub system?

[Jamie, 2016-10-05 17:51:30]

lgtm

[Sergey Ulanov, 2016-10-05 22:26:24]

On 2016/10/04 23:18:42, nicholss wrote:
> LGTM
> 
> I am wondering why there is so much implementation going on inside of the
> protocol lib.

Currently we need to support two versions of protocols, which means that all
webrtc dependencies need to be abstracted in the protocol layer.

> It seems like it would be more portable if we had no logic inside
> of the protocol and had implementation libs that were for webrtc and the old
> way. This is one of the things that makes it very hard to be able to reuse the
> chromoting code in another repo, this heavily ties the protocol to all the
> dependencies needed for all implementations rather than just what is required
> for passing the raw data between client and host.

WebRTC doesn't just pass raw data between host and client. Particularly for
audio and video it's responsible for encoding and sending that data and then
decoding it on the receiver side. In other words the functionality that WebRTC
provides doesn't match what we had in the protocol layer for the old protocol.
That's the reason more logic had to be moved to the protocol layer. This CL just
adds adapters between WebRTC and remoting interfaces, which is necessary to
support two different protocols.

https://codereview.chromium.org/2371323007/diff/20001/remoting/protocol/webrt...
File remoting/protocol/webrtc_audio_sink_adapter.cc (right):

https://codereview.chromium.org/2371323007/diff/20001/remoting/protocol/webrt...
remoting/protocol/webrtc_audio_sink_adapter.cc:76:
audio_stub_->ProcessAudioPacket(std::move(audio_packet), base::Closure());
On 2016/10/04 23:18:42, nicholss wrote:
> It feels wrong to have this amount of implementation in the protocol layer. Is
> this the best place to have the details of how we pump data out of webrtc and
> into our audio sub system?

This function just packages the data in AudioPacket object. AudioPacket is
actually specific to the old protocol, and makes little sense with WebRTC, but
we have dependencies on AudioPacket both in the host and client, so I kept if
for now. I think in the future we will want to refactor AudioStub to avoid
AudioPacket dependency or replace it with a different interface.

[Sergey Ulanov, 2016-10-09 16:52:30]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-10-09 16:52:31]

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/2371323007/#ps40001
(title: ".")

[commit-bot: I haz the power, 2016-10-09 16:52:39]

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 17:21:12]

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

[commit-bot: I haz the power, 2016-10-09 17:21:14]

Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Sergey Ulanov, 2016-10-10 18:31:29]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-10-10 18:31:30]

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/2371323007/#ps60001
(title: "more reliable test")

[commit-bot: I haz the power, 2016-10-10 18:32:01]

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-10 20:47:15]

Description was changed from

==========
Add audio support in WebrtcConnectionToHost, audio unittest

WebrtcConnectionToHost now supports audio streaming.
Also added a unittests that tests audio streaming, both for ICE and 
WebRTC protocols.

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

to

==========
Add audio support in WebrtcConnectionToHost, audio unittest

WebrtcConnectionToHost now supports audio streaming.
Also added a unittests that tests audio streaming, both for ICE and 
WebRTC protocols.

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

[commit-bot: I haz the power, 2016-10-10 20:47:16]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-10-10 20:49:34]

Description was changed from

==========
Add audio support in WebrtcConnectionToHost, audio unittest

WebrtcConnectionToHost now supports audio streaming.
Also added a unittests that tests audio streaming, both for ICE and 
WebRTC protocols.

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

to

==========
Add audio support in WebrtcConnectionToHost, audio unittest

WebrtcConnectionToHost now supports audio streaming.
Also added a unittests that tests audio streaming, both for ICE and
WebRTC protocols.

BUG=638505

Committed: https://crrev.com/f06b552d5632f5d07c1b81640dd1ce6b45772532
Cr-Commit-Position: refs/heads/master@{#424232}
==========

[commit-bot: I haz the power, 2016-10-10 20:49:36]

Patchset 3 (id:??) landed as
https://crrev.com/f06b552d5632f5d07c1b81640dd1ce6b45772532
Cr-Commit-Position: refs/heads/master@{#424232}