Add send transports to individual webrtc::Call streams.

Add send transports to individual webrtc::Call streams.

BUG=webrtc:4690
Committed: https://crrev.com/4fbae2b79134572135d9d5fe35a7d1ccdeea3a4d
Cr-Commit-Position: refs/heads/master@{#9807}

[the sun, 2015-08-12 12:04:45]

solenberg@webrtc.org changed reviewers:
+ mflodman@webrtc.org

[the sun, 2015-08-21 09:30:22]

On 2015/08/12 12:04:46, the sun wrote:

Ping! Would be great to have feedback on this by monday.

[mflodman, 2015-08-24 07:03:12]

On 2015/08/21 09:30:22, the sun wrote:
> On 2015/08/12 12:04:46, the sun wrote:
> 
> Ping! Would be great to have feedback on this by monday.

Review started...

[the sun, 2015-08-25 08:26:59]

solenberg@webrtc.org changed reviewers:
+ pbos@webrtc.org

[pbos-webrtc, 2015-08-26 12:21:22]

looks good, consider rtcp_send_transport

https://codereview.webrtc.org/1273363005/diff/80001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvoiceengine_unittest.cc (right):

https://codereview.webrtc.org/1273363005/diff/80001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine_unittest.cc:3357: cricket::FakeCall
call(webrtc::Call::Config{});
Is {} or () preferred? Or choose whatever you want? Haven't seen this before, so
I prefer () since it's familiar?

https://codereview.webrtc.org/1273363005/diff/80001/webrtc/audio_send_stream.h
File webrtc/audio_send_stream.h (right):

https://codereview.webrtc.org/1273363005/diff/80001/webrtc/audio_send_stream....
webrtc/audio_send_stream.h:49: newapi::Transport* send_transport;
= nullptr for consistency

https://codereview.webrtc.org/1273363005/diff/80001/webrtc/call.h
File webrtc/call.h (right):

https://codereview.webrtc.org/1273363005/diff/80001/webrtc/call.h#newcode72
webrtc/call.h:72: Config() = default;
Do you still want this line?

https://codereview.webrtc.org/1273363005/diff/80001/webrtc/video/transport_ad...
File webrtc/video/transport_adapter.cc (right):

https://codereview.webrtc.org/1273363005/diff/80001/webrtc/video/transport_ad...
webrtc/video/transport_adapter.cc:19: DCHECK(nullptr != transport);
pref transport != nullptr

https://codereview.webrtc.org/1273363005/diff/80001/webrtc/video_receive_stre...
File webrtc/video_receive_stream.h (right):

https://codereview.webrtc.org/1273363005/diff/80001/webrtc/video_receive_stre...
webrtc/video_receive_stream.h:145: newapi::Transport* send_transport = nullptr;
Rename this rtcp_send_transport, it'll help future readers

[Andrew MacDonald, 2015-08-26 16:32:34]

andrew@webrtc.org changed reviewers:
+ andrew@webrtc.org

[Andrew MacDonald, 2015-08-26 16:32:35]

Drive-by

https://codereview.webrtc.org/1273363005/diff/80001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvoiceengine_unittest.cc (right):

https://codereview.webrtc.org/1273363005/diff/80001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine_unittest.cc:3357: cricket::FakeCall
call(webrtc::Call::Config{});
On 2015/08/26 12:21:21, pbos-webrtc wrote:
> Is {} or () preferred? Or choose whatever you want? Haven't seen this before,
so
> I prefer () since it's familiar?

goog c-style arbiters prefer () in general:
https://g3doc.corp.google.com/devtools/library_club/g3doc/totw/totw-088.md?cl...

but at the end you can see they present either brace initialization or extra
parentheses as a solution to most vexing parse.

However, uniform initialization syntax is banned in Chromium anyway:
https://chromium-cpp.appspot.com/

So I suppose extra parentheses is the way to go?

[the sun, 2015-08-27 08:42:22]

https://codereview.webrtc.org/1273363005/diff/80001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvoiceengine_unittest.cc (right):

https://codereview.webrtc.org/1273363005/diff/80001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine_unittest.cc:3357: cricket::FakeCall
call(webrtc::Call::Config{});
On 2015/08/26 16:32:35, Andrew MacDonald wrote:
> On 2015/08/26 12:21:21, pbos-webrtc wrote:
> > Is {} or () preferred? Or choose whatever you want? Haven't seen this
before,
> so
> > I prefer () since it's familiar?
> 
> goog c-style arbiters prefer () in general:
>
https://g3doc.corp.google.com/devtools/library_club/g3doc/totw/totw-088.md?cl...
> 
> but at the end you can see they present either brace initialization or extra
> parentheses as a solution to most vexing parse.
> 
> However, uniform initialization syntax is banned in Chromium anyway:
> https://chromium-cpp.appspot.com/
> 
> So I suppose extra parentheses is the way to go?

Thanks for pointing this out! Using extra parenthesis.

https://codereview.webrtc.org/1273363005/diff/80001/webrtc/audio_send_stream.h
File webrtc/audio_send_stream.h (right):

https://codereview.webrtc.org/1273363005/diff/80001/webrtc/audio_send_stream....
webrtc/audio_send_stream.h:49: newapi::Transport* send_transport;
On 2015/08/26 12:21:21, pbos-webrtc wrote:
> = nullptr for consistency

Done.

https://codereview.webrtc.org/1273363005/diff/80001/webrtc/call.h
File webrtc/call.h (right):

https://codereview.webrtc.org/1273363005/diff/80001/webrtc/call.h#newcode72
webrtc/call.h:72: Config() = default;
On 2015/08/26 12:21:22, pbos-webrtc wrote:
> Do you still want this line?

Not really.

https://codereview.webrtc.org/1273363005/diff/80001/webrtc/video/transport_ad...
File webrtc/video/transport_adapter.cc (right):

https://codereview.webrtc.org/1273363005/diff/80001/webrtc/video/transport_ad...
webrtc/video/transport_adapter.cc:19: DCHECK(nullptr != transport);
On 2015/08/26 12:21:22, pbos-webrtc wrote:
> pref transport != nullptr

Why? I usually write conditionals with a non-assignable operand (if any) on the
left to avoid bugs when '=' was used instead of '=='.

https://codereview.webrtc.org/1273363005/diff/80001/webrtc/video_receive_stre...
File webrtc/video_receive_stream.h (right):

https://codereview.webrtc.org/1273363005/diff/80001/webrtc/video_receive_stre...
webrtc/video_receive_stream.h:145: newapi::Transport* send_transport = nullptr;
On 2015/08/26 12:21:22, pbos-webrtc wrote:
> Rename this rtcp_send_transport, it'll help future readers

Done.

[pbos-webrtc, 2015-08-27 09:02:01]

lgtm

https://codereview.webrtc.org/1273363005/diff/80001/webrtc/video/transport_ad...
File webrtc/video/transport_adapter.cc (right):

https://codereview.webrtc.org/1273363005/diff/80001/webrtc/video/transport_ad...
webrtc/video/transport_adapter.cc:19: DCHECK(nullptr != transport);
On 2015/08/27 08:42:21, the sun wrote:
> On 2015/08/26 12:21:22, pbos-webrtc wrote:
> > pref transport != nullptr
> 
> Why? I usually write conditionals with a non-assignable operand (if any) on
the
> left to avoid bugs when '=' was used instead of '=='.

Reads more like english "check that transport isn't null" vs "check that null
isn't transport". I won't hold you to it though. :)

[the sun, 2015-08-27 09:22:00]

mflodman@, do you have any comments?

[mflodman, 2015-08-28 08:30:18]

One minor optional comment and one Q, rest LG!

https://codereview.webrtc.org/1273363005/diff/120001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy_unittest.cc (right):

https://codereview.webrtc.org/1273363005/diff/120001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy_unittest.cc:37: VideoSendStream::Config
config(nullptr);
Why doesn't this trigger the DCHEK at line 19 in
https://codereview.webrtc.org/1273363005/diff/120001/webrtc/video/transport_a...
?

https://codereview.webrtc.org/1273363005/diff/120001/webrtc/video/transport_a...
File webrtc/video/transport_adapter.cc (right):

https://codereview.webrtc.org/1273363005/diff/120001/webrtc/video/transport_a...
webrtc/video/transport_adapter.cc:12: #include "webrtc/base/checks.h"
I prefer an empty line here since we've done it elsewhere.

[the sun, 2015-08-28 08:47:35]

https://codereview.webrtc.org/1273363005/diff/120001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy_unittest.cc (right):

https://codereview.webrtc.org/1273363005/diff/120001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy_unittest.cc:37: VideoSendStream::Config
config(nullptr);
On 2015/08/28 08:30:17, mflodman wrote:
> Why doesn't this trigger the DCHEK at line 19 in
>
https://codereview.webrtc.org/1273363005/diff/120001/webrtc/video/transport_a...
> ?

This test doesn't create a VideoSendStream (and subsequently no
TransportAdapter) - just a StatisticsProxy, which doesn't use the
send_transport. One could argue that the StatisticsProxy has no business knowing
about the VideoSendStream::Config, but that is a different matter from the focus
of this CL...

https://codereview.webrtc.org/1273363005/diff/120001/webrtc/video/transport_a...
File webrtc/video/transport_adapter.cc (right):

https://codereview.webrtc.org/1273363005/diff/120001/webrtc/video/transport_a...
webrtc/video/transport_adapter.cc:12: #include "webrtc/base/checks.h"
On 2015/08/28 08:30:17, mflodman wrote:
> I prefer an empty line here since we've done it elsewhere.

Done.

[mflodman, 2015-08-28 08:49:11]

On 2015/08/28 08:47:35, the sun wrote:
>
https://codereview.webrtc.org/1273363005/diff/120001/webrtc/video/send_statis...
> File webrtc/video/send_statistics_proxy_unittest.cc (right):
> 
>
https://codereview.webrtc.org/1273363005/diff/120001/webrtc/video/send_statis...
> webrtc/video/send_statistics_proxy_unittest.cc:37: VideoSendStream::Config
> config(nullptr);
> On 2015/08/28 08:30:17, mflodman wrote:
> > Why doesn't this trigger the DCHEK at line 19 in
> >
>
https://codereview.webrtc.org/1273363005/diff/120001/webrtc/video/transport_a...
> > ?
> 
> This test doesn't create a VideoSendStream (and subsequently no
> TransportAdapter) - just a StatisticsProxy, which doesn't use the
> send_transport. One could argue that the StatisticsProxy has no business
knowing
> about the VideoSendStream::Config, but that is a different matter from the
focus
> of this CL...

Doh, my mistake!

> 
>
https://codereview.webrtc.org/1273363005/diff/120001/webrtc/video/transport_a...
> File webrtc/video/transport_adapter.cc (right):
> 
>
https://codereview.webrtc.org/1273363005/diff/120001/webrtc/video/transport_a...
> webrtc/video/transport_adapter.cc:12: #include "webrtc/base/checks.h"
> On 2015/08/28 08:30:17, mflodman wrote:
> > I prefer an empty line here since we've done it elsewhere.
> 
> Done.

[mflodman, 2015-08-28 08:49:16]

lgtm

[the sun, 2015-08-28 08:56:25]

The CQ bit was checked by solenberg@webrtc.org

[the sun, 2015-08-28 08:56:25]

The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1273363005/#ps140001 (title:
"rebase+comment")

[commit-bot: I haz the power, 2015-08-28 08:56:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1273363005/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1273363005/140001

[commit-bot: I haz the power, 2015-08-28 11:07:15]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-08-28 11:07:24]

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