Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(7)

Issue 2827333005: Moving overhead counting to bitrate estimators.

Created:
7 months, 1 week ago by minyue-webrtc(BackIn2018March)
Modified:
7 months, 1 week ago
CC:
webrtc-reviews_webrtc.org, danilchap, AleBzk, zhuangzesen_agora.io, Andrew MacDonald, henrika_webrtc, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, peah-webrtc, the sun, mflodman
Target Ref:
refs/heads/master
Project:
webrtc
Visibility:
Public.

Description

Moving overhead counting to bitrate estimators. BUG=

Patch Set 1 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+282 lines, -162 lines) Patch
M webrtc/modules/congestion_controller/congestion_controller.cc View 1 chunk +4 lines, -2 lines 0 comments Download
M webrtc/modules/congestion_controller/congestion_controller_unittest.cc View 8 chunks +48 lines, -30 lines 0 comments Download
M webrtc/modules/congestion_controller/delay_based_bwe.h View 1 chunk +1 line, -0 lines 0 comments Download
M webrtc/modules/congestion_controller/delay_based_bwe.cc View 3 chunks +11 lines, -4 lines 0 comments Download
M webrtc/modules/congestion_controller/delay_based_bwe_unittest.cc View 2 chunks +8 lines, -2 lines 0 comments Download
M webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc View 3 chunks +10 lines, -2 lines 0 comments Download
M webrtc/modules/congestion_controller/include/congestion_controller.h View 1 chunk +2 lines, -1 line 2 comments Download
M webrtc/modules/congestion_controller/include/send_side_congestion_controller.h View 1 chunk +2 lines, -1 line 0 comments Download
M webrtc/modules/congestion_controller/probe_bitrate_estimator.h View 1 chunk +1 line, -0 lines 0 comments Download
M webrtc/modules/congestion_controller/probe_bitrate_estimator.cc View 4 chunks +14 lines, -5 lines 0 comments Download
M webrtc/modules/congestion_controller/probe_bitrate_estimator_unittest.cc View 2 chunks +6 lines, -3 lines 0 comments Download
M webrtc/modules/congestion_controller/send_side_congestion_controller.cc View 1 chunk +4 lines, -3 lines 0 comments Download
M webrtc/modules/congestion_controller/transport_feedback_adapter.h View 2 chunks +2 lines, -2 lines 0 comments Download
M webrtc/modules/congestion_controller/transport_feedback_adapter.cc View 3 chunks +6 lines, -10 lines 0 comments Download
M webrtc/modules/congestion_controller/transport_feedback_adapter_unittest.cc View 10 chunks +67 lines, -37 lines 0 comments Download
M webrtc/modules/remote_bitrate_estimator/send_time_history_unittest.cc View 7 chunks +47 lines, -27 lines 0 comments Download
M webrtc/modules/remote_bitrate_estimator/test/estimators/send_side.cc View 1 chunk +1 line, -1 line 0 comments Download
M webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h View 8 chunks +24 lines, -2 lines 0 comments Download
M webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc View 1 chunk +5 lines, -3 lines 0 comments Download
M webrtc/modules/rtp_rtcp/source/rtp_sender.h View 1 chunk +0 lines, -2 lines 0 comments Download
M webrtc/modules/rtp_rtcp/source/rtp_sender.cc View 3 chunks +4 lines, -11 lines 0 comments Download
M webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc View 4 chunks +7 lines, -10 lines 0 comments Download
M webrtc/tools/event_log_visualizer/analyzer.cc View 2 chunks +4 lines, -2 lines 0 comments Download
M webrtc/voice_engine/channel.cc View 1 chunk +4 lines, -2 lines 0 comments Download
Trybot results:

Messages

Total messages: 8 (2 generated)
minyue-webrtc(BackIn2018March)
This CL may be still immature. But I'd like to listen to your opinions before ...
7 months, 1 week ago (2017-04-21 12:14:44 UTC) #3
stefan-webrtc
I think I need more information about how this makes things simpler for you. To ...
7 months, 1 week ago (2017-04-21 12:35:28 UTC) #4
minyue-webrtc(BackIn2018March)
On 2017/04/21 12:35:28, stefan-webrtc wrote: > I think I need more information about how this ...
7 months, 1 week ago (2017-04-21 12:58:08 UTC) #5
minyue-webrtc(BackIn2018March)
https://codereview.webrtc.org/2827333005/diff/1/webrtc/modules/congestion_controller/include/congestion_controller.h File webrtc/modules/congestion_controller/include/congestion_controller.h (right): https://codereview.webrtc.org/2827333005/diff/1/webrtc/modules/congestion_controller/include/congestion_controller.h#newcode128 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 ...
7 months, 1 week ago (2017-04-21 12:58:14 UTC) #6
stefan-webrtc
On 2017/04/21 12:58:08, minyue-webrtc wrote: > On 2017/04/21 12:35:28, stefan-webrtc wrote: > > I think ...
7 months, 1 week ago (2017-04-21 13:04:44 UTC) #7
minyue-webrtc(BackIn2018March)
7 months, 1 week ago (2017-04-21 13:27:03 UTC) #8
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.

Powered by Google App Engine
This is Rietveld efc10ee0f