Moving overhead counting to bitrate estimators.

Moving overhead counting to bitrate estimators.

BUG=

[minyue-webrtc, 2017-04-21 12:11:10]

Description was changed from

==========
Moving overhead counting to bitrate estimators.

BUG=
==========

to

==========
Moving overhead counting to bitrate estimators.

BUG=
==========

[minyue-webrtc, 2017-04-21 12:11:10]

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

[minyue-webrtc, 2017-04-21 12:14:44]

This CL may be still immature. But I'd like to listen to your opinions before
going deeper.

The idea is to add overhead as explicit fields in PacketFeedback, and let
bitrate estimators (send_side & prober) to deal with them. This will make
reading and debugging easier.

[stefan-webrtc, 2017-04-21 12:35:28]

I think I need more information about how this makes things simpler for you. To
me it looks like we're just splitting packet_length into payload_length and
header_length in the API and adding them on several places inside
CongestionController instead of once outside?

https://codereview.webrtc.org/2827333005/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/include/congestion_controller.h
(right):

https://codereview.webrtc.org/2827333005/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/include/congestion_controller.h:128: size_t
rtp_headers_size,
I don't like this since I want to move towards CongestionController being
transport agnostic. We shouldn't know about RTP here.

[minyue-webrtc, 2017-04-21 12:58:08]

On 2017/04/21 12:35:28, stefan-webrtc wrote:
> I think I need more information about how this makes things simpler for you.
To
> me it looks like we're just splitting packet_length into payload_length and
> header_length in the API and adding them on several places inside
> CongestionController instead of once outside?
> 
>
https://codereview.webrtc.org/2827333005/diff/1/webrtc/modules/congestion_con...
> File webrtc/modules/congestion_controller/include/congestion_controller.h
> (right):
> 
>
https://codereview.webrtc.org/2827333005/diff/1/webrtc/modules/congestion_con...
> webrtc/modules/congestion_controller/include/congestion_controller.h:128:
size_t
> rtp_headers_size,
> I don't like this since I want to move towards CongestionController being
> transport agnostic. We shouldn't know about RTP here.

The reason for doing this is that the headers are added in a layered manner, it
is hard to track how headers accumulates.

Additionally, it is only the bit rate estimator and bit rate observers that need
to know how to deal with the headers. It is better not let the middle layers to
modify the sizes.

[minyue-webrtc, 2017-04-21 12:58:14]

https://codereview.webrtc.org/2827333005/diff/1/webrtc/modules/congestion_con...
File webrtc/modules/congestion_controller/include/congestion_controller.h
(right):

https://codereview.webrtc.org/2827333005/diff/1/webrtc/modules/congestion_con...
webrtc/modules/congestion_controller/include/congestion_controller.h:128: size_t
rtp_headers_size,
On 2017/04/21 12:35:27, stefan-webrtc wrote:
> I don't like this since I want to move towards CongestionController being
> transport agnostic. We shouldn't know about RTP here.

I know that and would like to avoid this. But the superclass
TransportFeedbackObserver is defined in rtp_rtcp. Any suggestions?

[stefan-webrtc, 2017-04-21 13:04:44]

On 2017/04/21 12:58:08, minyue-webrtc wrote:
> On 2017/04/21 12:35:28, stefan-webrtc wrote:
> > I think I need more information about how this makes things simpler for you.
> To
> > me it looks like we're just splitting packet_length into payload_length and
> > header_length in the API and adding them on several places inside
> > CongestionController instead of once outside?
> > 
> >
>
https://codereview.webrtc.org/2827333005/diff/1/webrtc/modules/congestion_con...
> > File webrtc/modules/congestion_controller/include/congestion_controller.h
> > (right):
> > 
> >
>
https://codereview.webrtc.org/2827333005/diff/1/webrtc/modules/congestion_con...
> > webrtc/modules/congestion_controller/include/congestion_controller.h:128:
> size_t
> > rtp_headers_size,
> > I don't like this since I want to move towards CongestionController being
> > transport agnostic. We shouldn't know about RTP here.
> 
> The reason for doing this is that the headers are added in a layered manner,
it
> is hard to track how headers accumulates.
> 
> Additionally, it is only the bit rate estimator and bit rate observers that
need
> to know how to deal with the headers. It is better not let the middle layers
to
> modify the sizes.

To me it would be simpler to say that all inputs into CongestionController
should include all headers, and all outputs from the CongestionController
(estimated bandwidth) also includes all headers. Then it's up to the ones
listening on the bandwidth updates (BitrateAllocator) to subtract any estimated
overhead if it needs to know what it can use for payload bitrate. 

This sounds like we only have two places where we need to keep track of
overhead? Maybe it'd be easier to follow your thinking with an example

[minyue-webrtc, 2017-04-21 13:27:03]

On 2017/04/21 13:04:44, stefan-webrtc wrote:
> On 2017/04/21 12:58:08, minyue-webrtc wrote:
> > On 2017/04/21 12:35:28, stefan-webrtc wrote:
> > > I think I need more information about how this makes things simpler for
you.
> > To
> > > me it looks like we're just splitting packet_length into payload_length
and
> > > header_length in the API and adding them on several places inside
> > > CongestionController instead of once outside?
> > > 
> > >
> >
>
https://codereview.webrtc.org/2827333005/diff/1/webrtc/modules/congestion_con...
> > > File webrtc/modules/congestion_controller/include/congestion_controller.h
> > > (right):
> > > 
> > >
> >
>
https://codereview.webrtc.org/2827333005/diff/1/webrtc/modules/congestion_con...
> > > webrtc/modules/congestion_controller/include/congestion_controller.h:128:
> > size_t
> > > rtp_headers_size,
> > > I don't like this since I want to move towards CongestionController being
> > > transport agnostic. We shouldn't know about RTP here.
> > 
> > The reason for doing this is that the headers are added in a layered manner,
> it
> > is hard to track how headers accumulates.
> > 
> > Additionally, it is only the bit rate estimator and bit rate observers that
> need
> > to know how to deal with the headers. It is better not let the middle layers
> to
> > modify the sizes.
> 
> To me it would be simpler to say that all inputs into CongestionController
> should include all headers, and all outputs from the CongestionController
> (estimated bandwidth) also includes all headers. Then it's up to the ones
> listening on the bandwidth updates (BitrateAllocator) to subtract any
estimated
> overhead if it needs to know what it can use for payload bitrate. 
> 
I agree with this. 

> This sounds like we only have two places where we need to keep track of
> overhead? Maybe it'd be easier to follow your thinking with an example
But currently, RtpSender adds Rtp overhead and TransportFeedbackAdaptor then
adds transport header size on top of it. You find two
send_side_bwe_with_overhead_ along the AddPacket line.

Other idea would be to let RtpSender add both, as I suggested in an earlier
email. But people may argue that RtpSender should not know transport header
size.