Make OverheadObserver::OnOverheadChanged count RTP headers only

Make OverheadObserver::OnOverheadChanged count RTP headers only

This lets the RTP code be unaware of lower layers, and the
SetTransportOverhead method is deleted from RTPSender and RtpRtcp.

Instead, that method is added to CongestionController and
TransportFeedbackAdapter, where it is more appropriate.

BUG=wertc:6847
Review-Url: https://codereview.webrtc.org/2589743002
Cr-Commit-Position: refs/heads/master@{#15995}
Committed: https://chromium.googlesource.com/external/webrtc/+/284542b8827b3379a9be227b3563e2b2c7742b47

[nisse-webrtc, 2016-12-19 12:37:23]

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

[nisse-webrtc, 2016-12-19 12:37:23]

PTAL. Currently fails  VideoSendStreamTest.ChangingTransportOverhead, it
produces too large RTP packets.

https://codereview.webrtc.org/2589743002/diff/1/webrtc/video/video_send_strea...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/2589743002/diff/1/webrtc/video/video_send_strea...
webrtc/video/video_send_stream.cc:1284: // TODO(nisse): Also call
SetMaxPayloadLength?
I don't quite get the notation here. The deleted SetTransportOverhead also
called SetMaxPayloadSize, as MaxPayloadLength() + (old transport overhead) -
(new transport overhead).

But why do we need both that and the MTU? It would make more sense to me to give
the RTP module tha maximum size of an RTP packet (i.e., including RTP headers
and any padding, but not transport headers).

Why does the code pass both MTU and MaxPayloadSize? The latter, is that
including RTP headers or not?

[stefan-webrtc, 2016-12-19 13:55:40]

stefan@webrtc.org changed reviewers:
+ michaelt@webrtc.org

[stefan-webrtc, 2016-12-19 13:55:41]

+michaelt who wrote most of this code, and may have additional comments.

https://codereview.webrtc.org/2589743002/diff/1/webrtc/video/video_send_strea...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/2589743002/diff/1/webrtc/video/video_send_strea...
webrtc/video/video_send_stream.cc:1203: (uint64_t) bitrate_bps *
static_cast please

https://codereview.webrtc.org/2589743002/diff/1/webrtc/video/video_send_strea...
webrtc/video/video_send_stream.cc:1284: // TODO(nisse): Also call
SetMaxPayloadLength?
On 2016/12/19 12:37:23, nisse-webrtc wrote:
> I don't quite get the notation here. The deleted SetTransportOverhead also
> called SetMaxPayloadSize, as MaxPayloadLength() + (old transport overhead) -
> (new transport overhead).
> 
> But why do we need both that and the MTU? It would make more sense to me to
give
> the RTP module tha maximum size of an RTP packet (i.e., including RTP headers
> and any padding, but not transport headers).
> 
> Why does the code pass both MTU and MaxPayloadSize? The latter, is that
> including RTP headers or not?

This is probably legacy. We should clean it up the way we want it, and I agree
that the RTP code should know only about RTP header sizes if possible.

[michaelt, 2016-12-19 14:17:02]

https://codereview.webrtc.org/2589743002/diff/1/webrtc/video/video_send_strea...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/2589743002/diff/1/webrtc/video/video_send_strea...
webrtc/video/video_send_stream.cc:1284: // TODO(nisse): Also call
SetMaxPayloadLength?
Calling SetMaxTransferUnit here will be senseless. We can remove this code. But
we have to set mtu at the creation of the rtp_rtcp modules. After the Removal of
SetTransportOverhead in rtp_rtcp, "SetMaxTransferUnit" seams to be a misleading
name. How about "SetMaxRtpSize". And we may have to adapt/ simply the function a
little.

https://codereview.webrtc.org/2589743002/diff/1/webrtc/voice_engine/channel.h
File webrtc/voice_engine/channel.h (right):

https://codereview.webrtc.org/2589743002/diff/1/webrtc/voice_engine/channel.h...
webrtc/voice_engine/channel.h:535: int _rtp_overhead_per_packet;
it seams that we use in old member-vars "_var" while we use for new ones "var_".
Should we clean that up ?

[nisse-webrtc, 2016-12-19 14:41:04]

https://codereview.webrtc.org/2589743002/diff/1/webrtc/video/video_send_strea...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/2589743002/diff/1/webrtc/video/video_send_strea...
webrtc/video/video_send_stream.cc:1203: (uint64_t) bitrate_bps *
On 2016/12/19 13:55:41, stefan-webrtc (holmer) wrote:
> static_cast please

Done.

https://codereview.webrtc.org/2589743002/diff/1/webrtc/video/video_send_strea...
webrtc/video/video_send_stream.cc:1284: // TODO(nisse): Also call
SetMaxPayloadLength?
On 2016/12/19 14:17:01, michaelt wrote:
> Calling SetMaxTransferUnit here will be senseless. We can remove this code.
But
> we have to set mtu at the creation of the rtp_rtcp modules. After the Removal
of
> SetTransportOverhead in rtp_rtcp, "SetMaxTransferUnit" seams to be a
misleading
> name. How about "SetMaxRtpSize". And we may have to adapt/ simply the function
a
> little.

I'll give it a try.

https://codereview.webrtc.org/2589743002/diff/1/webrtc/voice_engine/channel.h
File webrtc/voice_engine/channel.h (right):

https://codereview.webrtc.org/2589743002/diff/1/webrtc/voice_engine/channel.h...
webrtc/voice_engine/channel.h:535: int _rtp_overhead_per_packet;
On 2016/12/19 14:17:01, michaelt wrote:
> it seams that we use in old member-vars "_var" while we use for new ones
"var_".
> Should we clean that up ?

Ooops, I thought all members here were using the old convention, that's why I
followed that for the new members. It would be nice with a separate cl making
the class use the new convention consistently.

[nisse-webrtc, 2016-12-20 08:24:20]

On 2016/12/19 14:41:04, nisse-webrtc wrote:

> On 2016/12/19 14:17:01, michaelt wrote:
> > Calling SetMaxTransferUnit here will be senseless. We can remove this code.
> But
> > we have to set mtu at the creation of the rtp_rtcp modules. After the
Removal
> of
> > SetTransportOverhead in rtp_rtcp, "SetMaxTransferUnit" seams to be a
> misleading
> > name. How about "SetMaxRtpSize". And we may have to adapt/ simply the
function
> a
> > little.
> 
> I'll give it a try.

I wonder what the logic really should be for setting the rtp packet size.
There's a config setting of
webrtc::VideoSendStream::Config::Rtp::max_packet_size. This is set to 1200
(why?) for video streams, and to the default value of 1500-40 in all other cases
(at least I've not been able to find any other assignments to it).

In the case it is set to 1200 (or any other value significantly smaller than the
MTU), it seems reasonable to use it as is and never change it.

And in the case it is set to 1500-40, I guess the intention really is to set it
as large as possible without getting packet fragmentation, i.e., MTU - size of
transport headers.

The code contains some logic to derive size of transport headers (not sure if it
makes sense, though. For TCP it but it looks a little brittle to me, e.g.,
likely broken if we use TCP and the kernel adds any fancy TCP options. But on
the other hand, RTP packet size shouldn't be very critical when using TCP.

Ideally, we ought to find the MTU automatically, and ask the network stack for
the amount of headers. But that's not easy; for UDP, we'd have to implement path
MTU discovery ourselves, and for TCP, I don't think we can query header size,
and it could vary from packet to packet if the kernel uses the SACK extension).
So we're unlikely to do that anytime soon.

If instead we just guess we have an MTU of 1500 bytes, then it seems reasonable
to set the RtpPacket size to

  min(config.rtp.max_packet_size, 1500 - transport_overhead)

Do you agree?

[michaelt, 2016-12-20 08:52:55]

On 2016/12/20 08:24:20, nisse-webrtc wrote:
> On 2016/12/19 14:41:04, nisse-webrtc wrote:
> 
> > On 2016/12/19 14:17:01, michaelt wrote:
> > > Calling SetMaxTransferUnit here will be senseless. We can remove this
code.
> > But
> > > we have to set mtu at the creation of the rtp_rtcp modules. After the
> Removal
> > of
> > > SetTransportOverhead in rtp_rtcp, "SetMaxTransferUnit" seams to be a
> > misleading
> > > name. How about "SetMaxRtpSize". And we may have to adapt/ simply the
> function
> > a
> > > little.
> > 
> > I'll give it a try.
> 
> I wonder what the logic really should be for setting the rtp packet size.
> There's a config setting of
> webrtc::VideoSendStream::Config::Rtp::max_packet_size. This is set to 1200
> (why?) for video streams, and to the default value of 1500-40 in all other
cases
> (at least I've not been able to find any other assignments to it).
> 
> In the case it is set to 1200 (or any other value significantly smaller than
the
> MTU), it seems reasonable to use it as is and never change it.
> 
> And in the case it is set to 1500-40, I guess the intention really is to set
it
> as large as possible without getting packet fragmentation, i.e., MTU - size of
> transport headers.
> 
> The code contains some logic to derive size of transport headers (not sure if
it
> makes sense, though. For TCP it but it looks a little brittle to me, e.g.,
> likely broken if we use TCP and the kernel adds any fancy TCP options. But on
> the other hand, RTP packet size shouldn't be very critical when using TCP.
> 
> Ideally, we ought to find the MTU automatically, and ask the network stack for
> the amount of headers. But that's not easy; for UDP, we'd have to implement
path
> MTU discovery ourselves, and for TCP, I don't think we can query header size,
> and it could vary from packet to packet if the kernel uses the SACK
extension).
> So we're unlikely to do that anytime soon.
> 
> If instead we just guess we have an MTU of 1500 bytes, then it seems
reasonable
> to set the RtpPacket size to
> 
>   min(config.rtp.max_packet_size, 1500 - transport_overhead)
> 
> Do you agree?

min(config.rtp.max_packet_size, 1500 - transport_overhead)
Sounds fine to me.

[nisse-webrtc, 2016-12-20 10:03:46]

Reorganized to take out the MTU logic from the RTP code. (cl descriptions needs
some editing).

Also includes the code removal from https://codereview.webrtc.org/2588343002/,
which I'd like to land separately.

[stefan-webrtc, 2016-12-20 13:05:02]

https://codereview.webrtc.org/2589743002/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc (right):

https://codereview.webrtc.org/2589743002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:125:
SetMaxRtpPacketSize(IP_PACKET_SIZE - 40);
I think it would make sense to make 40 a named constant.

https://codereview.webrtc.org/2589743002/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h (right):

https://codereview.webrtc.org/2589743002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h:214: void
SetMaxRtpPacketSize(size_t size) override;
max_packet_size to align with rtp_rtcp.h

https://codereview.webrtc.org/2589743002/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/2589743002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1279:
overhead_observer_->OnOverheadChanged(overhead_bytes_per_packet);
This should now be renamed OnRtpOverheadChanged()

https://codereview.webrtc.org/2589743002/diff/60001/webrtc/video/video_send_s...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/2589743002/diff/60001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:42: // We don't to MTU discovery, so assume
that we have the standard ethernet MTU.
we don't do

https://codereview.webrtc.org/2589743002/diff/60001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:1193: // Substract total overhead (transport +
rtp) from bitrate.
Subtract

https://codereview.webrtc.org/2589743002/diff/60001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:1194: int rtp_overhead;
Should probably change this to a size_t to avoid casting

https://codereview.webrtc.org/2589743002/diff/60001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:1202: bitrate_bps = 0;
I would log a warning if this ever happens... :)

https://codereview.webrtc.org/2589743002/diff/60001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:1278: LOG(LS_ERROR) << "transport_overhead
exceeds size of ethrnet frame";
"Transport overhead exceeds size of ethernet frame"

[nisse-webrtc, 2016-12-20 13:38:13]

https://codereview.webrtc.org/2589743002/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc (right):

https://codereview.webrtc.org/2589743002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:125:
SetMaxRtpPacketSize(IP_PACKET_SIZE - 40);
On 2016/12/20 13:05:01, stefan-webrtc (holmer) wrote:
> I think it would make sense to make 40 a named constant.

There's just too many named constants for the same numerical values...

Would it make sense to use
webrtc::VideoSendStream::Config::Rtp::kDefaultMaxPacketSize here, or do I have
to duplicate it?

https://codereview.webrtc.org/2589743002/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h (right):

https://codereview.webrtc.org/2589743002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h:214: void
SetMaxRtpPacketSize(size_t size) override;
On 2016/12/20 13:05:01, stefan-webrtc (holmer) wrote:
> max_packet_size to align with rtp_rtcp.h

Done.

https://codereview.webrtc.org/2589743002/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/2589743002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1279:
overhead_observer_->OnOverheadChanged(overhead_bytes_per_packet);
On 2016/12/20 13:05:01, stefan-webrtc (holmer) wrote:
> This should now be renamed OnRtpOverheadChanged()

And the abstract-looking OverheadObserver interface should also be renamed to
the more specific RtpOverheadObserver. And perhaps be moved from common_types.h
to rtp_rtcp.h. Do you want me to do that? I'm not sure that's the right way to
go. 

If this stays the only use, it doesn't matter much, but if we ever want to keep
track of overhead with non-RTP-transport, I think it would be nice to not have
to introduce another interface with a different name but identical purpose.

https://codereview.webrtc.org/2589743002/diff/60001/webrtc/video/video_send_s...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/2589743002/diff/60001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:42: // We don't to MTU discovery, so assume
that we have the standard ethernet MTU.
On 2016/12/20 13:05:01, stefan-webrtc (holmer) wrote:
> we don't do

Done.

https://codereview.webrtc.org/2589743002/diff/60001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:1193: // Substract total overhead (transport +
rtp) from bitrate.
On 2016/12/20 13:05:01, stefan-webrtc (holmer) wrote:
> Subtract

Done.

https://codereview.webrtc.org/2589743002/diff/60001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:1194: int rtp_overhead;
On 2016/12/20 13:05:01, stefan-webrtc (holmer) wrote:
> Should probably change this to a size_t to avoid casting

Done. (For some reason I thought the overhead_bytes_per_packet_ member was an
int, but it's also a size_t).

https://codereview.webrtc.org/2589743002/diff/60001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:1202: bitrate_bps = 0;
On 2016/12/20 13:05:01, stefan-webrtc (holmer) wrote:
> I would log a warning if this ever happens... :)

Done.

https://codereview.webrtc.org/2589743002/diff/60001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:1278: LOG(LS_ERROR) << "transport_overhead
exceeds size of ethrnet frame";
On 2016/12/20 13:05:01, stefan-webrtc (holmer) wrote:
> "Transport overhead exceeds size of ethernet frame"

Done.

[michaelt, 2016-12-20 14:11:42]

https://codereview.webrtc.org/2589743002/diff/80001/webrtc/modules/rtp_rtcp/i...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp.h (left):

https://codereview.webrtc.org/2589743002/diff/80001/webrtc/modules/rtp_rtcp/i...
webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:144: // configuration
MaxTransferUnit, headers and TransportOverhead.
nit: You removed this function. Right ?

[stefan-webrtc, 2016-12-20 14:18:23]

lgtm

https://codereview.webrtc.org/2589743002/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc (right):

https://codereview.webrtc.org/2589743002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:125:
SetMaxRtpPacketSize(IP_PACKET_SIZE - 40);
On 2016/12/20 13:38:13, nisse-webrtc wrote:
> On 2016/12/20 13:05:01, stefan-webrtc (holmer) wrote:
> > I think it would make sense to make 40 a named constant.
> 
> There's just too many named constants for the same numerical values...
> 
> Would it make sense to use
> webrtc::VideoSendStream::Config::Rtp::kDefaultMaxPacketSize here, or do I have
> to duplicate it?

Probably, since then you won't be able to build this without depending on
webrtc::VideoSendStream.

Maybe just add a comment, or make it a local named constant?

https://codereview.webrtc.org/2589743002/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right):

https://codereview.webrtc.org/2589743002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1279:
overhead_observer_->OnOverheadChanged(overhead_bytes_per_packet);
On 2016/12/20 13:38:13, nisse-webrtc wrote:
> On 2016/12/20 13:05:01, stefan-webrtc (holmer) wrote:
> > This should now be renamed OnRtpOverheadChanged()
> 
> And the abstract-looking OverheadObserver interface should also be renamed to
> the more specific RtpOverheadObserver. And perhaps be moved from
common_types.h
> to rtp_rtcp.h. Do you want me to do that? I'm not sure that's the right way to
> go. 
> 
> If this stays the only use, it doesn't matter much, but if we ever want to
keep
> track of overhead with non-RTP-transport, I think it would be nice to not have
> to introduce another interface with a different name but identical purpose.

True. Maybe it's clear enough since it's only possible to register it to the rtp
module.

[the sun, 2016-12-21 10:33:03]

LGTM for voice engine channel, % nits, and then a question...

https://codereview.webrtc.org/2589743002/diff/80001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/transport_feedback_adapter.cc (right):

https://codereview.webrtc.org/2589743002/diff/80001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/transport_feedback_adapter.cc:83: //
TODO(nisse): Which lock?
figure out before landing

https://codereview.webrtc.org/2589743002/diff/80001/webrtc/modules/rtp_rtcp/i...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp.h (right):

https://codereview.webrtc.org/2589743002/diff/80001/webrtc/modules/rtp_rtcp/i...
webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:118: // TODO(nisse): Use int or
size_t?
size_t looks like the right call.

https://codereview.webrtc.org/2589743002/diff/80001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2589743002/diff/80001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:2898: void Channel::OnOverheadChanged(size_t
overhead_bytes_per_packet) {
Not strictly part of this CL, but... "transport_overhead_per_packet" and
"overhead_bytes_per_packet" seem to me, judging from their names, that they
would overlap. We're talking about overhead for UDP/RTP per packet? Wouldn't
this be constant and therefore just amount to an offset in the final estimate?
Is our BWE really so accurate that we need this information? And coming from two
different sources at that? I can see that would at best contribute little, and
at worst cause racing bugs...

stefan, michaelt?

https://codereview.webrtc.org/2589743002/diff/80001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.h (right):

https://codereview.webrtc.org/2589743002/diff/80001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.h:534: int _transport_overhead_per_packet;
How about we stick to size_t here as well?

[stefan-webrtc, 2016-12-21 11:31:43]

https://codereview.webrtc.org/2589743002/diff/80001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2589743002/diff/80001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:2898: void Channel::OnOverheadChanged(size_t
overhead_bytes_per_packet) {
On 2016/12/21 10:33:02, the sun wrote:
> Not strictly part of this CL, but... "transport_overhead_per_packet" and
> "overhead_bytes_per_packet" seem to me, judging from their names, that they
> would overlap. We're talking about overhead for UDP/RTP per packet? Wouldn't
> this be constant and therefore just amount to an offset in the final estimate?
> Is our BWE really so accurate that we need this information? And coming from
two
> different sources at that? I can see that would at best contribute little, and
> at worst cause racing bugs...
> 
> stefan, michaelt?

It's important for really low bitrates where the overhead is significant. Take
for instance opus at 6 kbps, with 20 ms frames we have about 15 payload bytes
per packet, while with 100 ms frames we have 75 bytes per packet.

The rtp overhead (around 20 bytes) will amount to 133% in the former case and
only 27% in the latter case. Switching frame lengths may therefore be detected
as a link capacity change by the BWE if we don't include headers.

[michaelt, 2017-01-09 12:42:52]

lgtm

[nisse-webrtc, 2017-01-09 16:02:24]

https://codereview.webrtc.org/2589743002/diff/60001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc (right):

https://codereview.webrtc.org/2589743002/diff/60001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:125:
SetMaxRtpPacketSize(IP_PACKET_SIZE - 40);
On 2016/12/20 14:18:23, stefan-webrtc_OOO_Jan9 wrote:
> On 2016/12/20 13:38:13, nisse-webrtc wrote:
> > On 2016/12/20 13:05:01, stefan-webrtc (holmer) wrote:
> > > I think it would make sense to make 40 a named constant.
> > 
> > There's just too many named constants for the same numerical values...
> > 
> > Would it make sense to use
> > webrtc::VideoSendStream::Config::Rtp::kDefaultMaxPacketSize here, or do I
have
> > to duplicate it?
> 
> Probably, since then you won't be able to build this without depending on
> webrtc::VideoSendStream.
> 
> Maybe just add a comment, or make it a local named constant?

Done.

https://codereview.webrtc.org/2589743002/diff/80001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/transport_feedback_adapter.cc (right):

https://codereview.webrtc.org/2589743002/diff/80001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/transport_feedback_adapter.cc:83: //
TODO(nisse): Which lock?
On 2016/12/21 10:33:02, the sun wrote:
> figure out before landing

It's accessed with lock_ held, so that seems right.

Can anyone (Michael, Stefan?) say if this is safe? This lock mainly protects
send_time_history_, and is held while calling methods on that object. Is there
any danger that those code paths interact in some way with the code paths
calling SetTransportOverhead, with risk of dead lock?

https://codereview.webrtc.org/2589743002/diff/80001/webrtc/modules/rtp_rtcp/i...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp.h (left):

https://codereview.webrtc.org/2589743002/diff/80001/webrtc/modules/rtp_rtcp/i...
webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:144: // configuration
MaxTransferUnit, headers and TransportOverhead.
On 2016/12/20 14:11:42, michaelt wrote:
> nit: You removed this function. Right ?

Rephrased comment.

https://codereview.webrtc.org/2589743002/diff/80001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.h (right):

https://codereview.webrtc.org/2589743002/diff/80001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.h:534: int _transport_overhead_per_packet;
On 2016/12/21 10:33:02, the sun wrote:
> How about we stick to size_t here as well?

Done (for both this one and _rtp_overhead_per_packet).

I'm also renaming to move underscore to the end.

[nisse-webrtc, 2017-01-10 08:36:42]

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

[commit-bot: I haz the power, 2017-01-10 08:36:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[stefan-webrtc, 2017-01-10 08:42:58]

https://codereview.webrtc.org/2589743002/diff/80001/webrtc/modules/congestion...
File webrtc/modules/congestion_controller/transport_feedback_adapter.cc (right):

https://codereview.webrtc.org/2589743002/diff/80001/webrtc/modules/congestion...
webrtc/modules/congestion_controller/transport_feedback_adapter.cc:83: //
TODO(nisse): Which lock?
On 2017/01/09 16:02:24, nisse-webrtc wrote:
> On 2016/12/21 10:33:02, the sun wrote:
> > figure out before landing
> 
> It's accessed with lock_ held, so that seems right.
> 
> Can anyone (Michael, Stefan?) say if this is safe? This lock mainly protects
> send_time_history_, and is held while calling methods on that object. Is there
> any danger that those code paths interact in some way with the code paths
> calling SetTransportOverhead, with risk of dead lock?

SendTimeHistory doesn't have any callbacks, so I don't think there's any risk.

[commit-bot: I haz the power, 2017-01-10 08:51:36]

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

[commit-bot: I haz the power, 2017-01-10 08:51:37]

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

[nisse-webrtc, 2017-01-10 14:40:09]

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

[commit-bot: I haz the power, 2017-01-10 14:40:13]

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-01-10 15:00:02]

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

[commit-bot: I haz the power, 2017-01-10 15:00:03]

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

[nisse-webrtc, 2017-01-10 16:09:23]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2017-01-10 16:09:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
solenberg@webrtc.org, stefan@webrtc.org, michaelt@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2589743002/#ps160001 (title:
"Add explicit cast.")

[commit-bot: I haz the power, 2017-01-10 16:09:30]

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-01-10 16:58:33]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1484064563279840,
"parent_rev": "c0ce6cc85e0929277dc378eefa5ec766da9e36c3", "commit_rev":
"284542b8827b3379a9be227b3563e2b2c7742b47"}

[commit-bot: I haz the power, 2017-01-10 16:58:36]

Description was changed from

==========
Make OverheadObserver::OnOverheadChanged count RTP headers only

This lets the RTP code be unaware of lower layers, and the
SetTransportOverhead method is deleted from RTPSender and RtpRtcp.

Instead, that method is added to CongestionController and
TransportFeedbackAdapter, where it is more appropriate.

BUG=wertc:6847
==========

to

==========
Make OverheadObserver::OnOverheadChanged count RTP headers only

This lets the RTP code be unaware of lower layers, and the
SetTransportOverhead method is deleted from RTPSender and RtpRtcp.

Instead, that method is added to CongestionController and
TransportFeedbackAdapter, where it is more appropriate.

BUG=wertc:6847

Review-Url: https://codereview.webrtc.org/2589743002
Cr-Commit-Position: refs/heads/master@{#15995}
Committed:
https://chromium.googlesource.com/external/webrtc/+/284542b8827b3379a9be227b3...
==========

[commit-bot: I haz the power, 2017-01-10 16:58:38]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/external/webrtc/+/284542b8827b3379a9be227b3...