Set actual transport overhead in rtp_rtcp

Set actual transport overhead in rtp_rtcp

BUG=webrtc:6557
Committed: https://crrev.com/79e05888e8f47ac7e19eb10decf0f1b490aa31ca
Cr-Commit-Position: refs/heads/master@{#14968}

[michaelt, 2016-10-19 10:31:16]

Description was changed from

==========
Set actual transport overhead in rtp_rtcp

BUG=webrtc:6557
==========

to

==========
Set actual transport overhead in rtp_rtcp

BUG=webrtc:6557
==========

[michaelt, 2016-10-19 10:31:16]

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

[michaelt, 2016-10-19 10:31:24]

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

[commit-bot: I haz the power, 2016-10-19 10:31:34]

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

[michaelt, 2016-10-19 10:34:59]

Hi,
Here a cl which set transport overhead in the rtp_rtcp module.

[commit-bot: I haz the power, 2016-10-19 15:26:56]

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

[commit-bot: I haz the power, 2016-10-19 15:26:57]

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

[the sun, 2016-10-20 08:56:45]

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

[the sun, 2016-10-20 08:56:46]

lgtm % comments for the audio stuff

https://codereview.webrtc.org/2437503004/diff/1/webrtc/audio/audio_send_strea...
File webrtc/audio/audio_send_stream.cc (right):

https://codereview.webrtc.org/2437503004/diff/1/webrtc/audio/audio_send_strea...
webrtc/audio/audio_send_stream.cc:282: int transport_overhead_per_packet_byte) {
RTC_DCHECK(thread_checker_.CalledOnValidThread());

https://codereview.webrtc.org/2437503004/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc (right):

https://codereview.webrtc.org/2437503004/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:415: int32_t
ModuleRtpRtcpImpl::SetTransportOverhead(
Remove the return value from this API - the result is always 0.

https://codereview.webrtc.org/2437503004/diff/1/webrtc/voice_engine/channel.cc
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2437503004/diff/1/webrtc/voice_engine/channel.c...
webrtc/voice_engine/channel.cc:2848:
_rtpRtcpModule->SetTransportOverhead(transport_overhead_per_packet_byte);
Either remove the return value of RtpRtcp::SetTransportOverhead(), or DCHECK the
result is always true here

[michaelt, 2016-10-20 09:38:05]

https://codereview.webrtc.org/2437503004/diff/1/webrtc/audio/audio_send_strea...
File webrtc/audio/audio_send_stream.cc (right):

https://codereview.webrtc.org/2437503004/diff/1/webrtc/audio/audio_send_strea...
webrtc/audio/audio_send_stream.cc:282: int transport_overhead_per_packet_byte) {
On 2016/10/20 08:56:46, the sun wrote:
> RTC_DCHECK(thread_checker_.CalledOnValidThread());

Done.

https://codereview.webrtc.org/2437503004/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc (right):

https://codereview.webrtc.org/2437503004/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:415: int32_t
ModuleRtpRtcpImpl::SetTransportOverhead(
On 2016/10/20 08:56:46, the sun wrote:
> Remove the return value from this API - the result is always 0.

Done.

https://codereview.webrtc.org/2437503004/diff/1/webrtc/voice_engine/channel.cc
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2437503004/diff/1/webrtc/voice_engine/channel.c...
webrtc/voice_engine/channel.cc:2848:
_rtpRtcpModule->SetTransportOverhead(transport_overhead_per_packet_byte);
I remove the return value of the function.

[minyue-webrtc, 2016-10-20 09:45:22]

Good work % a comment

https://codereview.webrtc.org/2437503004/diff/1/webrtc/modules/rtp_rtcp/inclu...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp.h (right):

https://codereview.webrtc.org/2437503004/diff/1/webrtc/modules/rtp_rtcp/inclu...
webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:114: virtual int32_t
SetTransportOverhead(
Removing/Modifying API may require to send some notice to webrtc-discussion 2
weeks ahead of time. 

Easy way is to 

1) keep the old one, let new one direct to the old one
2) prepare a separate cleaning up CL and send to webrtc-discussion, then you can
land this CL first
3) after two weeks time without objection, land the cleaning up CL.

[the sun, 2016-10-20 10:40:52]

https://codereview.webrtc.org/2437503004/diff/1/webrtc/modules/rtp_rtcp/inclu...
File webrtc/modules/rtp_rtcp/include/rtp_rtcp.h (right):

https://codereview.webrtc.org/2437503004/diff/1/webrtc/modules/rtp_rtcp/inclu...
webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:114: virtual int32_t
SetTransportOverhead(
On 2016/10/20 09:45:22, minyue-webrtc wrote:
> Removing/Modifying API may require to send some notice to webrtc-discussion 2
> weeks ahead of time. 
> 
> Easy way is to 
> 
> 1) keep the old one, let new one direct to the old one
> 2) prepare a separate cleaning up CL and send to webrtc-discussion, then you
can
> land this CL first
> 3) after two weeks time without objection, land the cleaning up CL.

Sorry, I didn't think about the rtprtcp module being "public"... :/ Perhaps best
to just change back to returning the useless int32.

[michaelt, 2016-10-20 12:02:54]

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

[commit-bot: I haz the power, 2016-10-20 12:02:56]

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

[minyue-webrtc, 2016-10-20 12:24:21]

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

https://codereview.webrtc.org/2437503004/diff/40001/webrtc/modules/rtp_rtcp/i...
webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:117: // Returns -1 on failure else 0
add todo to deprecate this

https://codereview.webrtc.org/2437503004/diff/40001/webrtc/modules/rtp_rtcp/i...
webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:123: // Returns -1 on failure else 0
no return?

[commit-bot: I haz the power, 2016-10-20 12:37:31]

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

[commit-bot: I haz the power, 2016-10-20 12:37:33]

Dry run: This issue passed the CQ dry run.

[michaelt, 2016-10-20 13:28:04]

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

https://codereview.webrtc.org/2437503004/diff/40001/webrtc/modules/rtp_rtcp/i...
webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:117: // Returns -1 on failure else 0
On 2016/10/20 12:24:20, minyue-webrtc wrote:
> add todo to deprecate this

Done.

https://codereview.webrtc.org/2437503004/diff/40001/webrtc/modules/rtp_rtcp/i...
webrtc/modules/rtp_rtcp/include/rtp_rtcp.h:123: // Returns -1 on failure else 0
Oops :)

[the sun, 2016-10-24 08:35:36]

Still lgtm, but I had some more comments

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

https://codereview.chromium.org/2437503004/diff/60001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:449: if
(transport_overhead_per_packet_byte == packet_overhead_) {
Is this optimization really necessary?

https://codereview.chromium.org/2437503004/diff/60001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:449: if
(transport_overhead_per_packet_byte == packet_overhead_) {
Is there a valid range for the input, which you can DCHECK?

https://codereview.chromium.org/2437503004/diff/60001/webrtc/voice_engine/cha...
File webrtc/voice_engine/channel_proxy.h (right):

https://codereview.chromium.org/2437503004/diff/60001/webrtc/voice_engine/cha...
webrtc/voice_engine/channel_proxy.h:94: virtual void SetTransportOverhead(int
transport_overhead_per_packet_byte);
There's a MockVoEChannelProxy which you should update too.

[michaelt, 2016-10-24 09:15:26]

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

https://codereview.chromium.org/2437503004/diff/60001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:449: if
(transport_overhead_per_packet_byte == packet_overhead_) {
On 2016/10/24 08:35:35, the sun wrote:
> Is there a valid range for the input, which you can DCHECK?

Yes added DCHECKS

https://codereview.chromium.org/2437503004/diff/60001/webrtc/modules/rtp_rtcp...
webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc:449: if
(transport_overhead_per_packet_byte == packet_overhead_) {
On 2016/10/24 08:35:35, the sun wrote:
> Is this optimization really necessary?

We can remove it. SetTransportOverhead should not be called often during a call.
So the better readable code will be more worth than the optimization.

https://codereview.chromium.org/2437503004/diff/60001/webrtc/voice_engine/cha...
File webrtc/voice_engine/channel_proxy.h (right):

https://codereview.chromium.org/2437503004/diff/60001/webrtc/voice_engine/cha...
webrtc/voice_engine/channel_proxy.h:94: virtual void SetTransportOverhead(int
transport_overhead_per_packet_byte);
On 2016/10/24 08:35:35, the sun wrote:
> There's a MockVoEChannelProxy which you should update too.

Done.

[minyue, 2016-10-26 09:46:09]

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

[minyue, 2016-10-26 09:46:10]

lgtm

[stefan-webrtc, 2016-10-26 15:08:41]

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

[stefan-webrtc, 2016-10-26 15:08:41]

+honghai for OWNER review of webrtc/pc/*

https://codereview.webrtc.org/2437503004/diff/80001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2437503004/diff/80001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:1671: constexpr int kSrtpAuthOverhaed = 4;
kSrtpAuthOverhead

I'm not sure about this, but I think the srtp overhead can be larger? Can we get
it from somewhere perhaps?

[honghaiz3, 2016-10-26 17:44:56]

https://codereview.webrtc.org/2437503004/diff/80001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2437503004/diff/80001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:1671: constexpr int kSrtpAuthOverhaed = 4;
On 2016/10/26 15:08:41, stefan-webrtc (holmer) wrote:
> kSrtpAuthOverhead
> 
> I'm not sure about this, but I think the srtp overhead can be larger? Can we
get
> it from somewhere perhaps?

In this site, it shows the srtp overhead is 12 bytes if we include the overhead
after the payload. 
http://www.networksorcery.com/enp/protocol/srtp.htm

https://codereview.webrtc.org/2437503004/diff/80001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:1673: int ransport_overhead_per_packet = 0;
ransport... =>transport...

[michaelt, 2016-10-27 13:05:17]

https://codereview.webrtc.org/2437503004/diff/80001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2437503004/diff/80001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:1671: constexpr int kSrtpAuthOverhaed = 4;
According to https://tools.ietf.org/html/rfc3711,
SRTP could at max just add two params (auth_tag, mki (optional)).
I misinterpreted the rfc3711 auth_tag is not necessary just 4bit long. Its just
4 bit alignment with a variable size.
When I'm not wrong again :) , MKI is not used in webrtc. So just the size of the
auth_tag is relevant for the overhead.

Additional to this problem, i found that a dtls connection setup si done after
the route change. So I had to change the concept of getting the Overhead a
little.

Thanks for catching this.

https://codereview.webrtc.org/2437503004/diff/80001/webrtc/pc/channel.cc#newc...
webrtc/pc/channel.cc:1673: int ransport_overhead_per_packet = 0;
On 2016/10/26 17:44:56, honghaiz3 wrote:
> ransport... =>transport...

Done.

[honghaiz3, 2016-10-27 18:57:44]

https://codereview.webrtc.org/2437503004/diff/100001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2437503004/diff/100001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:1678: transport_channel_->GetStats(&connection_infos);

Does it make sense to just remember the last signaled candidate pair so that you
don't have to find the best connection via GetStats?
That way, you don't need two methods to get the overhead. 
Another choice would be to add a virtual method selected_connection in the
transportchannel.

https://codereview.webrtc.org/2437503004/diff/100001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:1682: break;
You don't need a break after return.

[michaelt, 2016-10-31 09:59:26]

https://codereview.webrtc.org/2437503004/diff/100001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2437503004/diff/100001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:1678: transport_channel_->GetStats(&connection_infos);
store now the last signaled candidate pair in channel.

https://codereview.webrtc.org/2437503004/diff/100001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:1682: break;
:)
I removed the function.

[honghaiz3, 2016-10-31 17:42:09]

https://codereview.webrtc.org/2437503004/diff/100001/webrtc/pc/channel.h
File webrtc/pc/channel.h (right):

https://codereview.webrtc.org/2437503004/diff/100001/webrtc/pc/channel.h#newc...
webrtc/pc/channel.h:364: int GetTransportOverheadPerPacket(const Candidate&
local_candidate);
Remove this method declaration.

https://codereview.webrtc.org/2437503004/diff/120001/webrtc/p2p/base/transpor...
File webrtc/p2p/base/transportchannel.h (right):

https://codereview.webrtc.org/2437503004/diff/120001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportchannel.h:171: // virtual CandidatePairInterface*
selected_candidate_pair();
You don't need this unless you want to choose the other option of implementing
this method in all subclasses.

https://codereview.webrtc.org/2437503004/diff/120001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2437503004/diff/120001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:586:
rtc::Optional<CandidatePair>(CandidatePair(*selected_candidate_pair));
You don't need to save a copy of CandidatePair here. 
It should be sufficient to just use the pointer as the pointer is only accessed
in this class on the network thread and if the candidate pair is deleted, it
should be notified here. 

If you really want to save a copy to avoid the risk of accessing the pointer in
the future, I think we can save either
1. The local candidate, or
2. the overhead itself. 

By doing that, we don't need the changes in p2p/base dir.

https://codereview.webrtc.org/2437503004/diff/120001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:1682: constexpr int kTcpOverhaed = 20;
If you just use the saved selected_candidate_pair pointer, add a RTC_DCHECK to
make sure it is in the network thread.

https://codereview.webrtc.org/2437503004/diff/120001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:1686: if (selected_candidate_pair_) {
Can you use an early return? That would be more readable. 
if (!selected_candidate_pair_) {
  return 0;
}
....

[michaelt, 2016-11-01 08:31:37]

https://codereview.webrtc.org/2437503004/diff/100001/webrtc/pc/channel.h
File webrtc/pc/channel.h (right):

https://codereview.webrtc.org/2437503004/diff/100001/webrtc/pc/channel.h#newc...
webrtc/pc/channel.h:364: int GetTransportOverheadPerPacket(const Candidate&
local_candidate);
On 2016/10/31 17:42:09, honghaiz3 wrote:
> Remove this method declaration. 

Done.

https://codereview.webrtc.org/2437503004/diff/120001/webrtc/p2p/base/transpor...
File webrtc/p2p/base/transportchannel.h (right):

https://codereview.webrtc.org/2437503004/diff/120001/webrtc/p2p/base/transpor...
webrtc/p2p/base/transportchannel.h:171: // virtual CandidatePairInterface*
selected_candidate_pair();
On 2016/10/31 17:42:09, honghaiz3 wrote:
> You don't need this unless you want to choose the other option of implementing
> this method in all subclasses. 

Done.

https://codereview.webrtc.org/2437503004/diff/120001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2437503004/diff/120001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:1682: constexpr int kTcpOverhaed = 20;
On 2016/10/31 17:42:09, honghaiz3 wrote:
> If you just use the saved selected_candidate_pair pointer, add a RTC_DCHECK to
> make sure it is in the network thread. 

Done.

https://codereview.webrtc.org/2437503004/diff/120001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:1686: if (selected_candidate_pair_) {
On 2016/10/31 17:42:09, honghaiz3 wrote:
> Can you use an early return? That would be more readable. 
> if (!selected_candidate_pair_) {
>   return 0;
> }
> ....

Done.

[stefan-webrtc, 2016-11-02 13:21:43]

I'd prefer if we added a test of this new Call API, for instance in
video_send_stream_tests.cc, see for instance
TEST_F(VideoSendStreamTest, ChangingNetworkRoute)

BTW, did we introduce a new API because the encryption overhead can change after
the network route is set? Does encryption overhead change throughout a session?

https://codereview.webrtc.org/2437503004/diff/140001/webrtc/audio/audio_send_...
File webrtc/audio/audio_send_stream.h (right):

https://codereview.webrtc.org/2437503004/diff/140001/webrtc/audio/audio_send_...
webrtc/audio/audio_send_stream.h:60: void SetTransportOverhead(int
transport_overhead_per_packet_byte);
What does per_packet_byte mean? Isn't it just per packet?

https://codereview.webrtc.org/2437503004/diff/140001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/2437503004/diff/140001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:1529:
call_->SignalTransportOverheadChange(webrtc::MediaType::VIDEO,
Why SignalTransportOverheadChange instead of OnTransportOverheadChange?

https://codereview.webrtc.org/2437503004/diff/140001/webrtc/video/video_send_...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/2437503004/diff/140001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:1128: for (RtpRtcp* rtp_rtcp :
rtp_rtcp_modules_) {
You can remove {}

[michaelt, 2016-11-02 13:35:34]

https://codereview.webrtc.org/2437503004/diff/140001/webrtc/audio/audio_send_...
File webrtc/audio/audio_send_stream.h (right):

https://codereview.webrtc.org/2437503004/diff/140001/webrtc/audio/audio_send_...
webrtc/audio/audio_send_stream.h:60: void SetTransportOverhead(int
transport_overhead_per_packet_byte);
_byte should indicate that the size is in byte.
But this is maybe obvious and i can remove it.

https://codereview.webrtc.org/2437503004/diff/140001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/2437503004/diff/140001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:1529:
call_->SignalTransportOverheadChange(webrtc::MediaType::VIDEO,
On 2016/11/02 13:21:43, stefan-webrtc (holmer) wrote:
> Why SignalTransportOverheadChange instead of OnTransportOverheadChange?

SignalNetworkState was the prototype for this function. But beside that there is
no reason why I called it "SignalTransportOverheadChange" and not
"OnTransportOverheadChange".

https://codereview.webrtc.org/2437503004/diff/140001/webrtc/video/video_send_...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/2437503004/diff/140001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:1128: for (RtpRtcp* rtp_rtcp :
rtp_rtcp_modules_) {
On 2016/11/02 13:21:43, stefan-webrtc (holmer) wrote:
> You can remove {}

Acknowledged.

[honghaiz3, 2016-11-02 13:40:54]

lgtm on the pc dir.

[stefan-webrtc, 2016-11-02 13:41:09]

https://codereview.webrtc.org/2437503004/diff/140001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/2437503004/diff/140001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:1529:
call_->SignalTransportOverheadChange(webrtc::MediaType::VIDEO,
On 2016/11/02 13:35:34, michaelt wrote:
> On 2016/11/02 13:21:43, stefan-webrtc (holmer) wrote:
> > Why SignalTransportOverheadChange instead of OnTransportOverheadChange?
> 
> SignalNetworkState was the prototype for this function. But beside that there
is
> no reason why I called it "SignalTransportOverheadChange" and not
> "OnTransportOverheadChange".
> 

I'd prefer On to align with the other functions in this chain of calls.

[michaelt, 2016-11-02 15:13:38]

https://codereview.webrtc.org/2437503004/diff/140001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvideoengine2.cc (right):

https://codereview.webrtc.org/2437503004/diff/140001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvideoengine2.cc:1529:
call_->SignalTransportOverheadChange(webrtc::MediaType::VIDEO,
On 2016/11/02 13:41:09, stefan-webrtc (holmer) wrote:
> On 2016/11/02 13:35:34, michaelt wrote:
> > On 2016/11/02 13:21:43, stefan-webrtc (holmer) wrote:
> > > Why SignalTransportOverheadChange instead of OnTransportOverheadChange?
> > 
> > SignalNetworkState was the prototype for this function. But beside that
there
> is
> > no reason why I called it "SignalTransportOverheadChange" and not
> > "OnTransportOverheadChange".
> > 
> 
> I'd prefer On to align with the other functions in this chain of calls.

Done.

[stefan-webrtc, 2016-11-02 15:15:43]

Will you add the test I requested in this CL?

[michaelt, 2016-11-02 15:23:54]

Sorry I missed this message.

Did we introduce a new API because the encryption overhead can change after the
network route is set:
Yes
Does encryption overhead change throughout a session:
No

And yes I will add the test to the CL.

[stefan-webrtc, 2016-11-03 13:36:38]

lgtm

[minyue-webrtc, 2016-11-03 14:01:53]

I rely on Stefan and Honghai in the actual calculation of header size. I only
have a couple of small cosmetic suggestions.

https://codereview.webrtc.org/2437503004/diff/180001/webrtc/media/sctp/sctpda...
File webrtc/media/sctp/sctpdataengine.h (right):

https://codereview.webrtc.org/2437503004/diff/180001/webrtc/media/sctp/sctpda...
webrtc/media/sctp/sctpdataengine.h:166: virtual void
OnTransportOverheadChange(int transport_overhead_per_packet) {}
May be better to call this OnTransportOverheadChange"d"

https://codereview.webrtc.org/2437503004/diff/180001/webrtc/pc/channel.h
File webrtc/pc/channel.h (right):

https://codereview.webrtc.org/2437503004/diff/180001/webrtc/pc/channel.h#newc...
webrtc/pc/channel.h:369: int GetTransportOverheadPerPacket();
const ?

[michaelt, 2016-11-03 14:33:23]

https://codereview.webrtc.org/2437503004/diff/180001/webrtc/media/sctp/sctpda...
File webrtc/media/sctp/sctpdataengine.h (right):

https://codereview.webrtc.org/2437503004/diff/180001/webrtc/media/sctp/sctpda...
webrtc/media/sctp/sctpdataengine.h:166: virtual void
OnTransportOverheadChange(int transport_overhead_per_packet) {}
On 2016/11/03 14:01:53, minyue-webrtc wrote:
> May be better to call this OnTransportOverheadChange"d"

Done.

https://codereview.webrtc.org/2437503004/diff/180001/webrtc/pc/channel.h
File webrtc/pc/channel.h (right):

https://codereview.webrtc.org/2437503004/diff/180001/webrtc/pc/channel.h#newc...
webrtc/pc/channel.h:369: int GetTransportOverheadPerPacket();
On 2016/11/03 14:01:53, minyue-webrtc wrote:
> const ?

Done.

[minyue-webrtc, 2016-11-03 14:51:38]

lgtm

[michaelt, 2016-11-03 14:52:06]

The CQ bit was checked by michaelt@webrtc.org

[michaelt, 2016-11-03 14:52:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
solenberg@webrtc.org, minyue@chromium.org, honghaiz@webrtc.org,
stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2437503004/#ps200001 (title:
"Change OnTransportOverheadChange to OnTransportOverheadChanged.")

[commit-bot: I haz the power, 2016-11-03 14:52:13]

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, 2016-11-03 15:17:44]

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

[commit-bot: I haz the power, 2016-11-03 15:17:46]

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

[michaelt, 2016-11-07 09:21:55]

Hi had to change the impl of the CL a little. Since I discover some problems on
running the unittest.

[michaelt, 2016-11-07 09:30:36]

honghai, I would like you take a look again.

[honghaiz3, 2016-11-07 18:56:14]

lgtm

https://codereview.webrtc.org/2437503004/diff/220001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2437503004/diff/220001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:595: SignalTransportOverheadChanged();
I prefer SignalXXXX being reserved for names for sig-slot Signals. 
Perhaps change it to something like "UpdateTransportOverhead" (or something
else)?

[michaelt, 2016-11-08 08:29:31]

https://codereview.webrtc.org/2437503004/diff/220001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/2437503004/diff/220001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:595: SignalTransportOverheadChanged();
On 2016/11/07 18:56:14, honghaiz3 wrote:
> I prefer SignalXXXX being reserved for names for sig-slot Signals. 
> Perhaps change it to something like "UpdateTransportOverhead" (or something
> else)?

Done.

[michaelt, 2016-11-08 08:29:41]

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

[commit-bot: I haz the power, 2016-11-08 08:29:43]

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, 2016-11-08 08:45:42]

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

[commit-bot: I haz the power, 2016-11-08 08:45:43]

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

[michaelt, 2016-11-08 08:52:41]

The CQ bit was checked by michaelt@webrtc.org

[michaelt, 2016-11-08 08:52:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
solenberg@webrtc.org, minyue@chromium.org, stefan@webrtc.org, minyue@webrtc.org,
honghaiz@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2437503004/#ps240001 (title:
"Rename SignalTransportOverheadChanged to UpdateTransportOverhead")

[commit-bot: I haz the power, 2016-11-08 08:52:52]

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, 2016-11-08 08:58:02]

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

[commit-bot: I haz the power, 2016-11-08 08:58:04]

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

[michaelt, 2016-11-08 09:28:48]

Patchset #13 (id:240001) has been deleted

[michaelt, 2016-11-08 09:30:15]

The CQ bit was checked by michaelt@webrtc.org

[michaelt, 2016-11-08 09:30:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
solenberg@webrtc.org, minyue@chromium.org, stefan@webrtc.org,
honghaiz@webrtc.org, minyue@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2437503004/#ps260001 (title:
"Rename SignalTransportOverheadChanged to UpdateTransportOverhead.")

[commit-bot: I haz the power, 2016-11-08 09:30:21]

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, 2016-11-08 09:32:30]

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

[commit-bot: I haz the power, 2016-11-08 09:32:34]

Try jobs failed on following builders:
  ios64_sim_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/12284)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/14624)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/19855)
  mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)

[michaelt, 2016-11-08 10:12:47]

Patchset #13 (id:260001) has been deleted

[michaelt, 2016-11-08 10:20:22]

The CQ bit was checked by michaelt@webrtc.org

[michaelt, 2016-11-08 10:20:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
solenberg@webrtc.org, minyue@chromium.org, stefan@webrtc.org,
honghaiz@webrtc.org, minyue@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2437503004/#ps280001 (title:
"Rename SignalTransportOverheadChanged to UpdateTransportOverhead.")

[commit-bot: I haz the power, 2016-11-08 10:20:29]

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, 2016-11-08 10:29:40]

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

[commit-bot: I haz the power, 2016-11-08 10:29:43]

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

[michaelt, 2016-11-08 10:33:58]

The CQ bit was checked by michaelt@webrtc.org

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

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, 2016-11-08 10:50:12]

Description was changed from

==========
Set actual transport overhead in rtp_rtcp

BUG=webrtc:6557
==========

to

==========
Set actual transport overhead in rtp_rtcp

BUG=webrtc:6557
==========

[commit-bot: I haz the power, 2016-11-08 10:50:14]

Committed patchset #13 (id:280001)

[commit-bot: I haz the power, 2016-11-08 10:50:24]

Description was changed from

==========
Set actual transport overhead in rtp_rtcp

BUG=webrtc:6557
==========

to

==========
Set actual transport overhead in rtp_rtcp

BUG=webrtc:6557

Committed: https://crrev.com/79e05888e8f47ac7e19eb10decf0f1b490aa31ca
Cr-Commit-Position: refs/heads/master@{#14968}
==========

[commit-bot: I haz the power, 2016-11-08 10:50:27]

Patchset 13 (id:??) landed as
https://crrev.com/79e05888e8f47ac7e19eb10decf0f1b490aa31ca
Cr-Commit-Position: refs/heads/master@{#14968}