Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1524763002:
Don't account for audio in the pacer budget. (Closed)
|
Created:
5 years ago by stefan-webrtc Modified:
5 years ago Reviewers:
mflodman CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionDon't account for audio in the pacer budget.
We should only account for audio packets in the pacer budget if we also
are allocating bandwidth for the audio streams.
BUG=chromium:567659, webrtc:5263
R=mflodman@webrtc.org
Committed: https://crrev.com/c482eb3c84e958118451fbc443a0b2ba296e7441
Cr-Commit-Position: refs/heads/master@{#11053}
Patch Set 1 #Patch Set 2 : Fixed tests. #
Total comments: 4
Patch Set 3 : Added test. #
Messages
Total messages: 23 (10 generated)
Description was changed from ========== Don't account for audio in the pacer budget. We should only account for audio packets in the pacer budget if we also are allocating bandwidth for the audio streams. BUG=chromium:567659 ========== to ========== Don't account for audio in the pacer budget. We should only account for audio packets in the pacer budget if we also are allocating bandwidth for the audio streams. BUG=chromium:567659,webrtc:5263 ==========
The CQ bit was checked by stefan@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1524763002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1524763002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/11547)
Fixed tests.
The CQ bit was checked by stefan@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1524763002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1524763002/20001
stefan@webrtc.org changed reviewers: + mflodman@webrtc.org
https://codereview.webrtc.org/1524763002/diff/20001/webrtc/modules/pacing/pac... File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.webrtc.org/1524763002/diff/20001/webrtc/modules/pacing/pac... webrtc/modules/pacing/paced_sender.cc:435: if (success && packet.priority != kHighPriority) { Is this budget excluding logic tested in the unit test?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/1524763002/diff/20001/webrtc/modules/pacing/pac... File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.webrtc.org/1524763002/diff/20001/webrtc/modules/pacing/pac... webrtc/modules/pacing/paced_sender.cc:435: if (success && packet.priority != kHighPriority) { On 2015/12/14 13:19:29, mflodman wrote: > Is this budget excluding logic tested in the unit test? Not explicitly, but it is verified by the tests I had to modify. Do you want me to add a separate test for this?
LGTM https://codereview.webrtc.org/1524763002/diff/20001/webrtc/modules/pacing/pac... File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.webrtc.org/1524763002/diff/20001/webrtc/modules/pacing/pac... webrtc/modules/pacing/paced_sender.cc:435: if (success && packet.priority != kHighPriority) { Might be good, but not necessary if tested anyway.
https://codereview.webrtc.org/1524763002/diff/20001/webrtc/modules/pacing/pac... File webrtc/modules/pacing/paced_sender.cc (right): https://codereview.webrtc.org/1524763002/diff/20001/webrtc/modules/pacing/pac... webrtc/modules/pacing/paced_sender.cc:435: if (success && packet.priority != kHighPriority) { On 2015/12/16 12:53:14, mflodman wrote: > Might be good, but not necessary if tested anyway. Might as well do it. :)
The CQ bit was checked by stefan@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from mflodman@webrtc.org Link to the patchset: https://codereview.webrtc.org/1524763002/#ps40001 (title: "Added test.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1524763002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1524763002/40001
Description was changed from ========== Don't account for audio in the pacer budget. We should only account for audio packets in the pacer budget if we also are allocating bandwidth for the audio streams. BUG=chromium:567659,webrtc:5263 ========== to ========== Don't account for audio in the pacer budget. We should only account for audio packets in the pacer budget if we also are allocating bandwidth for the audio streams. BUG=chromium:567659,webrtc:5263 R=mflodman@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/c482eb3c84e958118451fbc44... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) manually as c482eb3c84e958118451fbc443a0b2ba296e7441 (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== Don't account for audio in the pacer budget. We should only account for audio packets in the pacer budget if we also are allocating bandwidth for the audio streams. BUG=chromium:567659,webrtc:5263 R=mflodman@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/c482eb3c84e958118451fbc44... ========== to ========== Don't account for audio in the pacer budget. We should only account for audio packets in the pacer budget if we also are allocating bandwidth for the audio streams. BUG=chromium:567659,webrtc:5263 R=mflodman@webrtc.org Committed: https://crrev.com/c482eb3c84e958118451fbc443a0b2ba296e7441 Cr-Commit-Position: refs/heads/master@{#11053} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c482eb3c84e958118451fbc443a0b2ba296e7441 Cr-Commit-Position: refs/heads/master@{#11053} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
