Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(194)

Issue 2609113003: BitrateProber::CreateProbeCluster now only accept one parameter (bitrate_bps). (Closed)

Created:
3 years, 11 months ago by philipel
Modified:
3 years, 11 months ago
Reviewers:
Sergey Ulanov, terelius
CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, stefan-webrtc, mflodman
Target Ref:
refs/heads/master
Project:
webrtc
Visibility:
Public.

Description

BitrateProber::CreateProbeCluster now only accept one parameter (bitrate_bps). Instead of having to specify a bitrate and how many packets to use, the BitrateProber will now use the bitrate to calculate how many bytes it will use to probe that bitrate instead. For now, |kMinProbeDurationMs| is set to 15 ms which means that probing at 1900 kbps will result in 1900/8*0.015 = 3.5 kB used. Since we can expect packets to be smaller at the beginning of a stream (500 to 700 bytes) this will result in 7 to 5 packets sent for that bitrate, and should work very similar to how the current initial probing works. A minimum of 5 packets are always sent. BUG=webrtc:6822 Review-Url: https://codereview.webrtc.org/2609113003 Cr-Commit-Position: refs/heads/master@{#15899} Committed: https://chromium.googlesource.com/external/webrtc/+/fd58b6106840c77538e864a22db68f6a8e28d848

Patch Set 1 #

Patch Set 2 : Comment fix #

Total comments: 2

Patch Set 3 : Removed |first_packet| + added unittests. #

Patch Set 4 : static_cast<int> + PacedSenderTest fix. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+106 lines, -75 lines) Patch
M webrtc/modules/congestion_controller/probe_controller.cc View 1 2 3 chunks +1 line, -15 lines 0 comments Download
M webrtc/modules/congestion_controller/probe_controller_unittest.cc View 10 chunks +16 lines, -16 lines 0 comments Download
M webrtc/modules/pacing/bitrate_prober.h View 2 chunks +5 lines, -3 lines 0 comments Download
M webrtc/modules/pacing/bitrate_prober.cc View 1 2 3 6 chunks +25 lines, -14 lines 0 comments Download
M webrtc/modules/pacing/bitrate_prober_unittest.cc View 1 2 4 chunks +40 lines, -5 lines 0 comments Download
M webrtc/modules/pacing/mock/mock_paced_sender.h View 1 chunk +1 line, -1 line 0 comments Download
M webrtc/modules/pacing/paced_sender.h View 2 chunks +3 lines, -3 lines 0 comments Download
M webrtc/modules/pacing/paced_sender.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M webrtc/modules/pacing/paced_sender_unittest.cc View 1 2 3 8 chunks +13 lines, -16 lines 0 comments Download

Messages

Total messages: 19 (12 generated)
philipel
3 years, 11 months ago (2017-01-03 16:56:28 UTC) #4
philipel
3 years, 11 months ago (2017-01-04 10:09:06 UTC) #6
terelius
Is any unit test checking that at least 5 packets are sent even on low ...
3 years, 11 months ago (2017-01-04 13:03:04 UTC) #7
philipel
https://codereview.webrtc.org/2609113003/diff/20001/webrtc/modules/congestion_controller/probe_controller.cc File webrtc/modules/congestion_controller/probe_controller.cc (right): https://codereview.webrtc.org/2609113003/diff/20001/webrtc/modules/congestion_controller/probe_controller.cc#newcode205 webrtc/modules/congestion_controller/probe_controller.cc:205: if (first_cluster) { On 2017/01/04 13:03:04, terelius wrote: > ...
3 years, 11 months ago (2017-01-04 13:31:14 UTC) #8
terelius
lgtm
3 years, 11 months ago (2017-01-04 13:35:11 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2609113003/60001
3 years, 11 months ago (2017-01-04 14:22:10 UTC) #16
commit-bot: I haz the power
3 years, 11 months ago (2017-01-04 15:05:29 UTC) #19
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/external/webrtc/+/fd58b6106840c77538e864a22...

Powered by Google App Engine
This is Rietveld 408576698