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

Issue 1412293003: Allow pacer to boost bitrate in order to meet time constraints. (Closed)

Created:
5 years, 1 month ago by sprang_webrtc
Modified:
5 years, 1 month ago
Reviewers:
stefan-webrtc, mflodman
CC:
webrtc-reviews_webrtc.org, yujie_mao (webrtc), stefan-webrtc, tterriberry_mozilla.com, mflodman, the sun, perkj_webrtc, andresp
Base URL:
https://chromium.googlesource.com/external/webrtc.git@master
Target Ref:
refs/pending/heads/master
Project:
webrtc
Visibility:
Public.

Description

Allow pacer to boost bitrate in order to meet time constraints. Currently we limit the enocder so that frames aren't encoded if the expected pacer queue is longer than 2s. However, if the queue is full and the bitrate suddenly drops (or there is a large overshoot), the queue time can be long than the limit. This CL allows the pacer to temporarily boost the pacing bitrate over the 2s window. BUG= Committed: https://crrev.com/0a43fef6dc8ce95a3ec52921870e08799ee9a250 Cr-Commit-Position: refs/heads/master@{#10729}

Patch Set 1 #

Total comments: 10

Patch Set 2 : Addressed comment #

Patch Set 3 : Use average queue time and size determine boost rate #

Total comments: 6

Patch Set 4 : Addressed comments #

Patch Set 5 : Cast #

Unified diffs Side-by-side diffs Delta from patch set Stats (+72 lines, -38 lines) Patch
M webrtc/modules/pacing/paced_sender.h View 1 2 2 chunks +8 lines, -1 line 0 comments Download
M webrtc/modules/pacing/paced_sender.cc View 1 2 3 4 17 chunks +57 lines, -25 lines 0 comments Download
M webrtc/modules/pacing/paced_sender_unittest.cc View 1 2 2 chunks +6 lines, -11 lines 0 comments Download
M webrtc/video_engine/vie_encoder.cc View 1 2 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 21 (5 generated)
sprang_webrtc
5 years, 1 month ago (2015-10-26 14:23:18 UTC) #2
stefan-webrtc
https://codereview.webrtc.org/1412293003/diff/1/webrtc/modules/pacing/paced_sender.cc File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.webrtc.org/1412293003/diff/1/webrtc/modules/pacing/paced_sender.cc#newcode328 webrtc/modules/pacing/paced_sender.cc:328: if (bitrate_boost_start_us_ != -1 && Is the bitrate_boost_start_us_ really ...
5 years, 1 month ago (2015-10-28 15:45:23 UTC) #3
sprang_webrtc
https://codereview.webrtc.org/1412293003/diff/1/webrtc/modules/pacing/paced_sender.cc File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.webrtc.org/1412293003/diff/1/webrtc/modules/pacing/paced_sender.cc#newcode328 webrtc/modules/pacing/paced_sender.cc:328: if (bitrate_boost_start_us_ != -1 && On 2015/10/28 15:45:23, stefan-webrtc ...
5 years, 1 month ago (2015-11-02 12:17:56 UTC) #4
stefan-webrtc
https://codereview.webrtc.org/1412293003/diff/1/webrtc/modules/pacing/paced_sender.cc File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.webrtc.org/1412293003/diff/1/webrtc/modules/pacing/paced_sender.cc#newcode328 webrtc/modules/pacing/paced_sender.cc:328: if (bitrate_boost_start_us_ != -1 && On 2015/11/02 12:17:56, språng ...
5 years, 1 month ago (2015-11-10 14:57:02 UTC) #5
stefan-webrtc
https://codereview.webrtc.org/1412293003/diff/1/webrtc/modules/pacing/paced_sender.cc File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.webrtc.org/1412293003/diff/1/webrtc/modules/pacing/paced_sender.cc#newcode328 webrtc/modules/pacing/paced_sender.cc:328: if (bitrate_boost_start_us_ != -1 && On 2015/11/10 14:57:02, stefan-webrtc ...
5 years, 1 month ago (2015-11-10 14:59:44 UTC) #6
sprang_webrtc
https://codereview.webrtc.org/1412293003/diff/1/webrtc/modules/pacing/paced_sender.cc File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.webrtc.org/1412293003/diff/1/webrtc/modules/pacing/paced_sender.cc#newcode328 webrtc/modules/pacing/paced_sender.cc:328: if (bitrate_boost_start_us_ != -1 && On 2015/11/10 14:59:44, stefan-webrtc ...
5 years, 1 month ago (2015-11-19 10:21:48 UTC) #7
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412293003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412293003/40001
5 years, 1 month ago (2015-11-19 10:22:08 UTC) #9
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: ios_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/10847) mac_x64_gn_rel on ...
5 years, 1 month ago (2015-11-19 10:23:18 UTC) #11
stefan-webrtc
This looks like a good solution. lgtm https://codereview.webrtc.org/1412293003/diff/40001/webrtc/modules/pacing/paced_sender.cc File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.webrtc.org/1412293003/diff/40001/webrtc/modules/pacing/paced_sender.cc#newcode140 webrtc/modules/pacing/paced_sender.cc:140: if (now ...
5 years, 1 month ago (2015-11-19 17:13:47 UTC) #12
stefan-webrtc
https://codereview.webrtc.org/1412293003/diff/40001/webrtc/modules/pacing/paced_sender.cc File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.webrtc.org/1412293003/diff/40001/webrtc/modules/pacing/paced_sender.cc#newcode120 webrtc/modules/pacing/paced_sender.cc:120: queue_time_sum_ -= (clock_->TimeInMilliseconds() - packet.enqueue_time_ms); Can this cause queue_time_sum_ ...
5 years, 1 month ago (2015-11-19 18:55:04 UTC) #13
mflodman
https://codereview.webrtc.org/1412293003/diff/40001/webrtc/modules/pacing/paced_sender.cc File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.webrtc.org/1412293003/diff/40001/webrtc/modules/pacing/paced_sender.cc#newcode120 webrtc/modules/pacing/paced_sender.cc:120: queue_time_sum_ -= (clock_->TimeInMilliseconds() - packet.enqueue_time_ms); I'm not sure clock_->TimeInMilliseconds() ...
5 years, 1 month ago (2015-11-19 18:58:56 UTC) #14
sprang_webrtc
https://codereview.webrtc.org/1412293003/diff/40001/webrtc/modules/pacing/paced_sender.cc File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.webrtc.org/1412293003/diff/40001/webrtc/modules/pacing/paced_sender.cc#newcode120 webrtc/modules/pacing/paced_sender.cc:120: queue_time_sum_ -= (clock_->TimeInMilliseconds() - packet.enqueue_time_ms); On 2015/11/19 18:58:55, mflodman ...
5 years, 1 month ago (2015-11-20 11:07:52 UTC) #15
mflodman
LGTM
5 years, 1 month ago (2015-11-20 16:59:05 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412293003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412293003/80001
5 years, 1 month ago (2015-11-20 16:59:42 UTC) #19
commit-bot: I haz the power
Committed patchset #5 (id:80001)
5 years, 1 month ago (2015-11-20 17:00:41 UTC) #20
commit-bot: I haz the power
5 years, 1 month ago (2015-11-20 17:01:03 UTC) #21
Message was sent while issue was closed.
Patchset 5 (id:??) landed as
https://crrev.com/0a43fef6dc8ce95a3ec52921870e08799ee9a250
Cr-Commit-Position: refs/heads/master@{#10729}

Powered by Google App Engine
This is Rietveld 408576698