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

Issue 2827333005: Moving overhead counting to bitrate estimators.

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 months, 4 weeks ago by minyue-webrtc(BackIn2018March)
Modified:
5 months, 4 weeks 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: Sign in to try more bots
Commit queue not available (can’t edit this change).

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 ...
5 months, 4 weeks 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 ...
5 months, 4 weeks 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 ...
5 months, 4 weeks 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 ...
5 months, 4 weeks 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 ...
5 months, 4 weeks ago (2017-04-21 13:04:44 UTC) #7
minyue-webrtc(BackIn2018March)
5 months, 4 weeks 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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 81bcdb8aa