Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2775483004:
Set max bitrate for audio send stream based on RtpParameters. (Closed)
|
Created:
3 years, 9 months ago by minyue-webrtc Modified:
3 years, 9 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, michaelt Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionSet max bitrate for audio send stream based on RtpParameters.
BUG=webrtc:7392
Review-Url: https://codereview.webrtc.org/2775483004
Cr-Commit-Position: refs/heads/master@{#17399}
Committed: https://chromium.googlesource.com/external/webrtc/+/cecec1060fff99e78935e21d1cfe46af236af1f9
Patch Set 1 #
Total comments: 4
Patch Set 2 : fixing #
Total comments: 3
Patch Set 3 : updating comments #
Total comments: 8
Patch Set 4 : comments. #Patch Set 5 : fixing #
Total comments: 1
Patch Set 6 : fixing #
Total comments: 1
Messages
Total messages: 33 (13 generated)
Patchset #2 (id:20001) has been deleted
minyue@webrtc.org changed reviewers: + solenberg@webrtc.org, stefan@webrtc.org
Hi, This is an urgent issue. Please take a look.
I think we should add a test, but otherwise I think it looks good % nit. https://codereview.webrtc.org/2775483004/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2775483004/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1396: // BWE is done through a dedicated API. I think this is something that should be here even after we add a BWE api. It's still possible that someone would want to change the allocation of audio bitrate.
lgtm
On 2017/03/24 20:41:23, the sun wrote: > lgtm Please address Stefan's comments.
and will try to add a test. https://codereview.webrtc.org/2775483004/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2775483004/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1396: // BWE is done through a dedicated API. On 2017/03/24 15:09:14, stefan-webrtc wrote: > I think this is something that should be here even after we add a BWE api. It's > still possible that someone would want to change the allocation of audio > bitrate. right, I will remove this todo now. https://codereview.webrtc.org/2775483004/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1432: config_.max_bitrate_bps += max_overhead_bps; I see a problem here. To make sure that the payload rate will never go above rtp_parameters_.encodings[0].max_bitrate_bps, we have to add min_overhead_bps. The other one is add max_overhead_bps.
michaelt@webrtc.org changed reviewers: + michaelt@webrtc.org
lgtm
https://codereview.webrtc.org/2775483004/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2775483004/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1432: config_.max_bitrate_bps += max_overhead_bps; On 2017/03/24 21:13:59, minyue-webrtc wrote: > I see a problem here. To make sure that the payload rate will never go above > rtp_parameters_.encodings[0].max_bitrate_bps, we have to add min_overhead_bps. > > The other one is add max_overhead_bps. Sounds good to me. As long as the max BWE is set high enough to allow some overhead it shouldn't affect calls on good networks.
I changed the logic as proposed (we know that it will have a problem, and I added comments around that). Tests are added too. PTAL again.
https://codereview.webrtc.org/2775483004/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2775483004/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1397: // TODO(minyue): remove dependency on RtpParameters when the signaling to I did not change the TODO here since we may need to reconsider this due to the overhead issue.
LGTM % comments addressed https://codereview.webrtc.org/2775483004/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2775483004/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1397: // TODO(minyue): remove dependency on RtpParameters when the signaling to On 2017/03/27 13:29:01, minyue-webrtc wrote: > I did not change the TODO here since we may need to reconsider this due to the > overhead issue. I still think it makes sense to have this wired up to the bw allocator, so please rephrase this comment explaining what work is to be done https://codereview.webrtc.org/2775483004/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1438: // In contrast to max_bitrate_bps, we let min_bitrate_bps be always always be reachable
updated the comments, I will land it soon if no further comments
lgtm % comments https://codereview.webrtc.org/2775483004/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2775483004/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:1216: // SetRtpSendParameters is called nit: trailing "." https://codereview.webrtc.org/2775483004/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:1226: // Expect emptu on parameters.encodings[0].max_bitrate_bps; emptu -> empty https://codereview.webrtc.org/2775483004/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:1243: "WebRTC-Audio-SendSideBwe/Enabled/WebRTC-SendSideBwe-WithOverhead/" is "/" really separator between trials? nit: split on two lines after "Enabled/" instead - easier to read. https://codereview.webrtc.org/2775483004/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:1251: // Expect emptu on parameters.encodings[0].max_bitrate_bps; emptu->empty
https://codereview.webrtc.org/2775483004/diff/60001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2775483004/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:1216: // SetRtpSendParameters is called On 2017/03/27 15:34:21, the sun wrote: > nit: trailing "." Done. https://codereview.webrtc.org/2775483004/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:1226: // Expect emptu on parameters.encodings[0].max_bitrate_bps; On 2017/03/27 15:34:21, the sun wrote: > emptu -> empty oh, thanks. I hate my ide that give a bad color on comments. I will change the color theme. https://codereview.webrtc.org/2775483004/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:1243: "WebRTC-Audio-SendSideBwe/Enabled/WebRTC-SendSideBwe-WithOverhead/" "/" is On 2017/03/27 15:34:21, the sun wrote: > is "/" really separator between trials? > > nit: split on two lines after "Enabled/" instead - easier to read. sure https://codereview.webrtc.org/2775483004/diff/60001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:1251: // Expect emptu on parameters.encodings[0].max_bitrate_bps; On 2017/03/27 15:34:21, the sun wrote: > emptu->empty Done.
The CQ bit was checked by minyue@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaelt@webrtc.org, stefan@webrtc.org, solenberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2775483004/#ps80001 (title: "comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...) ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/18766) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/23070)
The CQ bit was checked by minyue@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org, stefan@webrtc.org, michaelt@webrtc.org Link to the patchset: https://codereview.webrtc.org/2775483004/#ps100001 (title: "fixing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/23461) linux_ubsan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/11005) linux_ubsan_vptr on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)
https://codereview.webrtc.org/2775483004/diff/100001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2775483004/diff/100001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:1240: TEST_F(WebRtcVoiceEngineTestFake, found a better place for this test, see PS6. https://codereview.webrtc.org/2775483004/diff/120001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2775483004/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:2656: EXPECT_EQ(kOpusBitrateFbBps + kMinOverheadBps, changed the behavior intentionally.
The CQ bit was checked by minyue@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org, stefan@webrtc.org, michaelt@webrtc.org Link to the patchset: https://codereview.webrtc.org/2775483004/#ps120001 (title: "fixing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1490643332735650,
"parent_rev": "488b85e502e150c4dcfa64452af2873ea1163c6f", "commit_rev":
"cecec1060fff99e78935e21d1cfe46af236af1f9"}
Message was sent while issue was closed.
Description was changed from ========== Set max bitrate for audio send stream based on RtpParameters. BUG=webrtc:7392 ========== to ========== Set max bitrate for audio send stream based on RtpParameters. BUG=webrtc:7392 Review-Url: https://codereview.webrtc.org/2775483004 Cr-Commit-Position: refs/heads/master@{#17399} Committed: https://chromium.googlesource.com/external/webrtc/+/cecec1060fff99e78935e21d1... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as https://chromium.googlesource.com/external/webrtc/+/cecec1060fff99e78935e21d1... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
