New targets call:rtp_interfaces, call:rtp_receiver, call:rtp_sender.

New targets call:rtp_interfaces, call:rtp_receiver, call:rtp_sender.

BUG=webrtc:7135
TBR=sprang@webrtc.org

Review-Url: https://codereview.webrtc.org/2913143003
Cr-Commit-Position: refs/heads/master@{#18371}
Committed: https://chromium.googlesource.com/external/webrtc/+/d76b7b294a9c36e1f2f827913452842fdcaa62cf

[nisse-webrtc, 2017-05-31 11:45:20]

nisse@webrtc.org changed reviewers:
+ danilchap@webrtc.org, solenberg@webrtc.org

[nisse-webrtc, 2017-05-31 11:45:21]

PTAL. This moves the code related to RtpTransportController out from call:call
into separate gn targets. The code stays in the call/ directory for now, it
should be moved later when it's closer to finished.

The rtp_interfaces targets should perhaps be deleted, it was needed to avoid a
circular dependency prior to cl https://codereview.webrtc.org/2907403003.

[danilchap, 2017-05-31 11:56:59]

lgtm

https://codereview.webrtc.org/2913143003/diff/1/webrtc/call/BUILD.gn
File webrtc/call/BUILD.gn (right):

https://codereview.webrtc.org/2913143003/diff/1/webrtc/call/BUILD.gn#newcode52
webrtc/call/BUILD.gn:52: ":rtp_interfaces",
is this dependency needed?

[nisse-webrtc, 2017-05-31 12:01:09]

https://codereview.webrtc.org/2913143003/diff/1/webrtc/call/BUILD.gn
File webrtc/call/BUILD.gn (right):

https://codereview.webrtc.org/2913143003/diff/1/webrtc/call/BUILD.gn#newcode52
webrtc/call/BUILD.gn:52: ":rtp_interfaces",
On 2017/05/31 11:56:59, danilchap wrote:
> is this dependency needed?

It looks like rtp_demuxer.h was accidentally listed in two targets. If it's only
in rtp_interfaces, this dependency is required.

Question is, should we have the rtp_interfaces target or not?

[danilchap, 2017-05-31 12:08:58]

On 2017/05/31 12:01:09, nisse-webrtc wrote:
> https://codereview.webrtc.org/2913143003/diff/1/webrtc/call/BUILD.gn
> File webrtc/call/BUILD.gn (right):
> 
> https://codereview.webrtc.org/2913143003/diff/1/webrtc/call/BUILD.gn#newcode52
> webrtc/call/BUILD.gn:52: ":rtp_interfaces",
> On 2017/05/31 11:56:59, danilchap wrote:
> > is this dependency needed?
> 
> It looks like rtp_demuxer.h was accidentally listed in two targets. If it's
only
> in rtp_interfaces, this dependency is required.
> 
> Question is, should we have the rtp_interfaces target or not?

May be not, may be all new three targets can be merged into single one for now:
'rtp_transport_controller'
and split into several when needed, e.g. to break cycles or to reduce heavy
dependencies.

[the sun, 2017-05-31 13:06:29]

https://codereview.webrtc.org/2913143003/diff/1/webrtc/call/BUILD.gn
File webrtc/call/BUILD.gn (right):

https://codereview.webrtc.org/2913143003/diff/1/webrtc/call/BUILD.gn#newcode46
webrtc/call/BUILD.gn:46: "rtp_demuxer.cc",
It seems rtp_demuxer should be split so that the clients only needing the
interface (RtpPacketSinkinterface?) would only pull that in. The .cc and .h
shouldn't live in separate targets.

[nisse-webrtc, 2017-05-31 13:16:50]

It would be nice if reviewers can get to an agreement on the appropriate number
of targets.

https://codereview.webrtc.org/2913143003/diff/1/webrtc/call/BUILD.gn
File webrtc/call/BUILD.gn (right):

https://codereview.webrtc.org/2913143003/diff/1/webrtc/call/BUILD.gn#newcode46
webrtc/call/BUILD.gn:46: "rtp_demuxer.cc",
On 2017/05/31 13:06:29, the sun wrote:
> It seems rtp_demuxer should be split so that the clients only needing the
> interface (RtpPacketSinkinterface?) would only pull that in. The .cc and .h
> shouldn't live in separate targets.

Ok, so I'd split out the RtpPacketSinkInterface to it's own header file, and put
that in the rtp_interface target?

[danilchap, 2017-05-31 16:09:28]

On 2017/05/31 13:16:50, nisse-webrtc wrote:
> It would be nice if reviewers can get to an agreement on the appropriate
number
> of targets.
> 
I do not have strong opinion about number of targets.

> https://codereview.webrtc.org/2913143003/diff/1/webrtc/call/BUILD.gn
> File webrtc/call/BUILD.gn (right):
> 
> https://codereview.webrtc.org/2913143003/diff/1/webrtc/call/BUILD.gn#newcode46
> webrtc/call/BUILD.gn:46: "rtp_demuxer.cc",
> On 2017/05/31 13:06:29, the sun wrote:
> > It seems rtp_demuxer should be split so that the clients only needing the
> > interface (RtpPacketSinkinterface?) would only pull that in. The .cc and .h
> > shouldn't live in separate targets.
> 
> Ok, so I'd split out the RtpPacketSinkInterface to it's own header file, and
put
> that in the rtp_interface target?

sgtm.
Will it be 2 (sink+controller interfaces; implementation) or 3 targets
(interfaces; receive; send) then?
Both ways are good to me.

[nisse-webrtc, 2017-06-01 06:55:09]

On 2017/05/31 16:09:28, danilchap wrote:
> On 2017/05/31 13:16:50, nisse-webrtc wrote:
> > It would be nice if reviewers can get to an agreement on the appropriate
> number
> > of targets.
> > 
> I do not have strong opinion about number of targets.
> 
> > https://codereview.webrtc.org/2913143003/diff/1/webrtc/call/BUILD.gn
> > File webrtc/call/BUILD.gn (right):
> > 
> >
https://codereview.webrtc.org/2913143003/diff/1/webrtc/call/BUILD.gn#newcode46
> > webrtc/call/BUILD.gn:46: "rtp_demuxer.cc",
> > On 2017/05/31 13:06:29, the sun wrote:
> > > It seems rtp_demuxer should be split so that the clients only needing the
> > > interface (RtpPacketSinkinterface?) would only pull that in. The .cc and
.h
> > > shouldn't live in separate targets.
> > 
> > Ok, so I'd split out the RtpPacketSinkInterface to it's own header file, and
> put
> > that in the rtp_interface target?
> 
> sgtm.
> Will it be 2 (sink+controller interfaces; implementation) or 3 targets
> (interfaces; receive; send) then?
> Both ways are good to me.

I think I'll stick to three tartgets, interfaces, receive, send, even if that
might be slight overkill at the moment.

[nisse-webrtc, 2017-06-01 07:15:39]

I have now moved the RtpPacketSinkInterface to it's own file, and include that
instead of rtp_demuxer.h where appropriate.

[the sun, 2017-06-01 07:28:21]

lgtm!

https://codereview.webrtc.org/2913143003/diff/20001/webrtc/call/BUILD.gn
File webrtc/call/BUILD.gn (right):

https://codereview.webrtc.org/2913143003/diff/20001/webrtc/call/BUILD.gn#newc...
webrtc/call/BUILD.gn:72: "bitrate_allocator.cc",
Why isn't bitrate_allocator.h in this target?

https://codereview.webrtc.org/2913143003/diff/20001/webrtc/call/rtp_demuxer.h
File webrtc/call/rtp_demuxer.h (right):

https://codereview.webrtc.org/2913143003/diff/20001/webrtc/call/rtp_demuxer.h...
webrtc/call/rtp_demuxer.h:15: #include "webrtc/call/rtp_packet_sink_interface.h"
forward decl would suffice

[nisse-webrtc, 2017-06-01 07:48:05]

https://codereview.webrtc.org/2913143003/diff/20001/webrtc/call/BUILD.gn
File webrtc/call/BUILD.gn (right):

https://codereview.webrtc.org/2913143003/diff/20001/webrtc/call/BUILD.gn#newc...
webrtc/call/BUILD.gn:72: "bitrate_allocator.cc",
On 2017/06/01 07:28:21, the sun wrote:
> Why isn't bitrate_allocator.h in this target?

Looks like an omission, it isn't included in any target.

But adding it results in the following failure:

ERROR at //webrtc/audio/audio_send_stream.h:21:11: Include not allowed.
#include "webrtc/call/bitrate_allocator.h"
          ^------------------------------
It is not in any dependency of
  //webrtc/audio:audio
The include file is in the target(s):
  //webrtc/call:call
which should somehow be reachable.

and a similar for video_send_stream.h.

Should I add it to call_interfaces (together with call.h) instead, even if that
violates the rule that .h and .cc should be together?

https://codereview.webrtc.org/2913143003/diff/20001/webrtc/call/rtp_demuxer.h
File webrtc/call/rtp_demuxer.h (right):

https://codereview.webrtc.org/2913143003/diff/20001/webrtc/call/rtp_demuxer.h...
webrtc/call/rtp_demuxer.h:15: #include "webrtc/call/rtp_packet_sink_interface.h"
On 2017/06/01 07:28:21, the sun wrote:
> forward decl would suffice

True. Not entirely clear if it makes sense to use the demuxer without the real
definition. But it seems Call doesn't need it.

So I'm changing to a forward declaration.

[nisse-webrtc, 2017-06-01 09:57:17]

https://codereview.webrtc.org/2913143003/diff/20001/webrtc/call/BUILD.gn
File webrtc/call/BUILD.gn (right):

https://codereview.webrtc.org/2913143003/diff/20001/webrtc/call/BUILD.gn#newc...
webrtc/call/BUILD.gn:72: "bitrate_allocator.cc",
On 2017/06/01 07:48:05, nisse-webrtc wrote:
> On 2017/06/01 07:28:21, the sun wrote:
> > Why isn't bitrate_allocator.h in this target?
> 
> Looks like an omission, it isn't included in any target.
> 
> But adding it results in the following failure:
> 
> ERROR at //webrtc/audio/audio_send_stream.h:21:11: Include not allowed.
> #include "webrtc/call/bitrate_allocator.h"
>           ^------------------------------
> It is not in any dependency of
>   //webrtc/audio:audio
> The include file is in the target(s):
>   //webrtc/call:call
> which should somehow be reachable.
> 
> and a similar for video_send_stream.h.
> 
> Should I add it to call_interfaces (together with call.h) instead, even if
that
> violates the rule that .h and .cc should be together?

I'm leaving this unchanged for this cl, trying to land.

[nisse-webrtc, 2017-06-01 09:57:26]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2017-06-01 09:57:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
danilchap@webrtc.org, solenberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2913143003/#ps40001 (title:
"Forward declare RtpPacketSinkInterface.")

[commit-bot: I haz the power, 2017-06-01 09:57:29]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[nisse-webrtc, 2017-06-01 09:57:35]

The CQ bit was unchecked by nisse@webrtc.org

[nisse-webrtc, 2017-06-01 09:57:44]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-01 09:57:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-06-01 10:00:49]

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

[commit-bot: I haz the power, 2017-06-01 10:00:49]

Dry run: Try jobs failed on following builders:
  linux_more_configs on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_more_configs/buil...)

[nisse-webrtc, 2017-06-01 10:08:21]

The CQ bit was checked by nisse@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-01 10:08:26]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-06-01 10:43:20]

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

[commit-bot: I haz the power, 2017-06-01 10:43:21]

Dry run: This issue passed the CQ dry run.

[nisse-webrtc, 2017-06-01 10:52:16]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2017-06-01 10:52:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
solenberg@webrtc.org, danilchap@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2913143003/#ps60001 (title:
"Rebase, needed additional include in unit test.")

[commit-bot: I haz the power, 2017-06-01 10:52:27]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-06-01 10:56:11]

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

[commit-bot: I haz the power, 2017-06-01 10:56:12]

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

[nisse-webrtc, 2017-06-01 11:00:23]

Description was changed from

==========
New targets call:rtp_interfaces, call:rtp_receiver, call:rtp_sender.

BUG=webrtc:7135
==========

to

==========
New targets call:rtp_interfaces, call:rtp_receiver, call:rtp_sender.

BUG=webrtc:7135
TBR=sprang@webrtc.org
==========

[nisse-webrtc, 2017-06-01 11:00:23]

nisse@webrtc.org changed reviewers:
+ sprang@webrtc.org

[nisse-webrtc, 2017-06-01 11:00:34]

The CQ bit was checked by nisse@webrtc.org

[commit-bot: I haz the power, 2017-06-01 11:00:44]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-06-01 11:02:35]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496314834722910,
"parent_rev": "405f90c871a4d11b09e1a868a9f4754d28d4b809", "commit_rev":
"d76b7b294a9c36e1f2f827913452842fdcaa62cf"}

[commit-bot: I haz the power, 2017-06-01 11:02:39]

Description was changed from

==========
New targets call:rtp_interfaces, call:rtp_receiver, call:rtp_sender.

BUG=webrtc:7135
TBR=sprang@webrtc.org
==========

to

==========
New targets call:rtp_interfaces, call:rtp_receiver, call:rtp_sender.

BUG=webrtc:7135
TBR=sprang@webrtc.org

Review-Url: https://codereview.webrtc.org/2913143003
Cr-Commit-Position: refs/heads/master@{#18371}
Committed:
https://chromium.googlesource.com/external/webrtc/+/d76b7b294a9c36e1f2f827913...
==========

[commit-bot: I haz the power, 2017-06-01 11:02:41]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/external/webrtc/+/d76b7b294a9c36e1f2f827913...