Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2106183002:
Fix bug where min transmit bitrate wasn't working (Closed)
|
Created:
4 years, 5 months ago by sprang_webrtc Modified:
4 years, 5 months ago CC:
webrtc-reviews_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, pbos-webrtc, perkj_webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/branch-heads/53 Project:
webrtc Visibility:
Public. |
DescriptionFix bug where min transmit bitrate wasn't working
A recent refactoring (r13192) introduced a bug where the min transmit
config wasn't being respected. Specifically, if a VideoSendStream was
created without it and the reconfigured, the min transmit bitrate would
not take effect. Probably the other way around as well.
BUG=webrtc::5687
Committed: https://crrev.com/9c0b55142533ebfa20836609777aa7f3681cadbe
Cr-Commit-Position: refs/heads/master@{#13390}
Patch Set 1 #Patch Set 2 : Added new end to end test with mid-call reconfiguration #Patch Set 3 : Add min transmit bitrate to Call::Stats, update test #
Total comments: 10
Patch Set 4 : Addressed comments #
Total comments: 4
Patch Set 5 : Stats contain current configured min bitrate #
Total comments: 12
Patch Set 6 : Addressed comments #
Total comments: 4
Patch Set 7 : Fixed nits #
Messages
Total messages: 36 (13 generated)
sprang@webrtc.org changed reviewers: + pbos@webrtc.org, perkj@webrtc.org
This can severely impact screenshare performance and both the problem and fix have been verified by manual testing. I'd prefer to be able to land this soon, and add better test coverage in a follow-up (as it is non-trivial to do).
fwiw, lgtm, but let perkj@ review before landing
pbos, I've added a test; you may wanna take a second pass.
lgtm, make sure someone agrees with adding this to stats (because I'm not sure it's useful for stats itself)
sprang@webrtc.org changed reviewers: + mflodman@webrtc.org
+mflodman wdyt re stats?
Thanks for fixing. Can you please add webrtc::5687 to BUG= and you probably must file a new bug as well so it can be merged to M53. https://codereview.webrtc.org/2106183002/diff/40001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2106183002/diff/40001/webrtc/call/call.cc#newco... webrtc/call/call.cc:721: current_min_transmit_bitrate_bps_ = max_padding_bitrate_bps; this is the max possible padding bitrate and does not take into account the current estimate. max_padding_bitrate may be higher than what we actually send. https://codereview.webrtc.org/2106183002/diff/40001/webrtc/video/end_to_end_t... File webrtc/video/end_to_end_tests.cc (right): https://codereview.webrtc.org/2106183002/diff/40001/webrtc/video/end_to_end_t... webrtc/video/end_to_end_tests.cc:2502: class MinTransmitBitrateTest : public test::EndToEndTest, Why is this not a VideoSendStream test? What is the difference with EndToEnd tests? https://codereview.webrtc.org/2106183002/diff/40001/webrtc/video/end_to_end_t... webrtc/video/end_to_end_tests.cc:2502: class MinTransmitBitrateTest : public test::EndToEndTest, How about testing padding instead? There are tests that tests padding I think, so why not change one of those tests to check that padding is sent if we start with no padding and switch content type that needs padding? https://codereview.webrtc.org/2106183002/diff/40001/webrtc/video/end_to_end_t... webrtc/video/end_to_end_tests.cc:2509: explicit MinTransmitBitrateTest(bool run_init_phase_without_mtb) bool test_switch_content_type ? https://codereview.webrtc.org/2106183002/diff/40001/webrtc/video/video_send_s... File webrtc/video/video_send_stream.cc (right): https://codereview.webrtc.org/2106183002/diff/40001/webrtc/video/video_send_s... webrtc/video/video_send_stream.cc:592: bitrate_allocator_->AddObserver( We should not add as observer unless the stream i started. i.e if (state_ == kStarted) ... bitrate_allocator->...
Description was changed from ========== Fix bug where min transmit bitrate wasn't working A recent refactoring (r13192) introduced a bug where the min transmit config wasn't being respected. Specifically, if a VideoSendStream was created without it and the reconfigured, the min transmit bitrate would not take effect. Probably the other way around as well. BUG= ========== to ========== Fix bug where min transmit bitrate wasn't working A recent refactoring (r13192) introduced a bug where the min transmit config wasn't being respected. Specifically, if a VideoSendStream was created without it and the reconfigured, the min transmit bitrate would not take effect. Probably the other way around as well. BUG=webrtc::5687 ==========
https://codereview.webrtc.org/2106183002/diff/40001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2106183002/diff/40001/webrtc/call/call.cc#newco... webrtc/call/call.cc:721: current_min_transmit_bitrate_bps_ = max_padding_bitrate_bps; On 2016/07/04 07:24:49, perkj_webrtc wrote: > this is the max possible padding bitrate and does not take into account the > current estimate. max_padding_bitrate may be higher than what we actually send. Right, good catch. https://codereview.webrtc.org/2106183002/diff/40001/webrtc/video/end_to_end_t... File webrtc/video/end_to_end_tests.cc (right): https://codereview.webrtc.org/2106183002/diff/40001/webrtc/video/end_to_end_t... webrtc/video/end_to_end_tests.cc:2502: class MinTransmitBitrateTest : public test::EndToEndTest, On 2016/07/04 07:24:49, perkj_webrtc wrote: > Why is this not a VideoSendStream test? What is the difference with EndToEnd > tests? I initially had planned to check received padding, but I ended up begin way too flaky. Now it doesn't make sense anymore so have moved it to send stream test. https://codereview.webrtc.org/2106183002/diff/40001/webrtc/video/end_to_end_t... webrtc/video/end_to_end_tests.cc:2502: class MinTransmitBitrateTest : public test::EndToEndTest, On 2016/07/04 07:24:49, perkj_webrtc wrote: > How about testing padding instead? There are tests that tests padding I think, > so why not change one of those tests to check that padding is sent if we start > with no padding and switch content type that needs padding? I did that at first, it turned out to flake way too easily, so I abandoned it. https://codereview.webrtc.org/2106183002/diff/40001/webrtc/video/end_to_end_t... webrtc/video/end_to_end_tests.cc:2509: explicit MinTransmitBitrateTest(bool run_init_phase_without_mtb) On 2016/07/04 07:24:49, perkj_webrtc wrote: > bool test_switch_content_type ? Done. https://codereview.webrtc.org/2106183002/diff/40001/webrtc/video/video_send_s... File webrtc/video/video_send_stream.cc (right): https://codereview.webrtc.org/2106183002/diff/40001/webrtc/video/video_send_s... webrtc/video/video_send_stream.cc:592: bitrate_allocator_->AddObserver( On 2016/07/04 07:24:49, perkj_webrtc wrote: > We should not add as observer unless the stream i started. > i.e if (state_ == kStarted) ... bitrate_allocator->... Done.
https://codereview.webrtc.org/2106183002/diff/60001/webrtc/call.h File webrtc/call.h (right): https://codereview.webrtc.org/2106183002/diff/60001/webrtc/call.h#newcode95 webrtc/call.h:95: int min_transmit_bitrate_bps = 0; // Bitrate to send, padding as needed. Discussed off line- remove min_transmit_bitrate_bps for now. https://codereview.webrtc.org/2106183002/diff/60001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2106183002/diff/60001/webrtc/call/call.cc#newco... webrtc/call/call.cc:726: current_min_transmit_bitrate_bps_ = send_bandwidth; Sorry but I don't think this make sense. Now you will only update current_min_transmit_bitrate_bps_ when the stream is configured and not each time the bitrate changes. I would much rather just get rid of current_min_transmit_bitrate_bps_ all together.
https://codereview.webrtc.org/2106183002/diff/60001/webrtc/call.h File webrtc/call.h (right): https://codereview.webrtc.org/2106183002/diff/60001/webrtc/call.h#newcode95 webrtc/call.h:95: int min_transmit_bitrate_bps = 0; // Bitrate to send, padding as needed. On 2016/07/04 11:24:24, perkj_webrtc wrote: > Discussed off line- remove min_transmit_bitrate_bps for now. As discussed off line, reinstate this, but reporting configured min transmit bitrate rather than current active. https://codereview.webrtc.org/2106183002/diff/60001/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2106183002/diff/60001/webrtc/call/call.cc#newco... webrtc/call/call.cc:726: current_min_transmit_bitrate_bps_ = send_bandwidth; On 2016/07/04 11:24:24, perkj_webrtc wrote: > Sorry but I don't think this make sense. Now you will only update > current_min_transmit_bitrate_bps_ when the stream is configured and not each > time the bitrate changes. I would much rather just get rid of > current_min_transmit_bitrate_bps_ all together. As discussed off-line. Changed to report cumulative configured min transmit bitrate.
https://codereview.webrtc.org/2106183002/diff/80001/webrtc/video/end_to_end_t... File webrtc/video/end_to_end_tests.cc (right): https://codereview.webrtc.org/2106183002/diff/80001/webrtc/video/end_to_end_t... webrtc/video/end_to_end_tests.cc:28: #include "webrtc/modules/rtp_rtcp/source/rtp_utility.h" revert this change too? https://codereview.webrtc.org/2106183002/diff/80001/webrtc/video/video_send_s... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/2106183002/diff/80001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:1124: class MinTransmitBitrateTest : public test::SendTest, public test::FakeEncoder { name MaxPaddingSetTest? https://codereview.webrtc.org/2106183002/diff/80001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:1150: encoder_config->streams[0].target_bitrate_bps = kMinTransmitBitrateBps * 2; are all these modifications really needed? https://codereview.webrtc.org/2106183002/diff/80001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:1163: int32_t SetRates(uint32_t new_target_bitrate, uint32_t framerate) override { not needed any more? https://codereview.webrtc.org/2106183002/diff/80001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:1190: EXPECT_EQ(0, call_->GetStats().min_transmit_bitrate_bps); why not simply call this max_padding_bitrate_bps ? This is not the min transmit bitrate. https://codereview.webrtc.org/2106183002/diff/80001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:1194: if (++frames_sent_ < kMinFramesToSend) nit frames_encoded_ ?
https://codereview.webrtc.org/2106183002/diff/80001/webrtc/video/end_to_end_t... File webrtc/video/end_to_end_tests.cc (right): https://codereview.webrtc.org/2106183002/diff/80001/webrtc/video/end_to_end_t... webrtc/video/end_to_end_tests.cc:28: #include "webrtc/modules/rtp_rtcp/source/rtp_utility.h" On 2016/07/04 14:51:12, perkj_webrtc wrote: > revert this change too? Done. https://codereview.webrtc.org/2106183002/diff/80001/webrtc/video/video_send_s... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/2106183002/diff/80001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:1124: class MinTransmitBitrateTest : public test::SendTest, public test::FakeEncoder { On 2016/07/04 14:51:12, perkj_webrtc wrote: > name MaxPaddingSetTest? Done. https://codereview.webrtc.org/2106183002/diff/80001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:1150: encoder_config->streams[0].target_bitrate_bps = kMinTransmitBitrateBps * 2; On 2016/07/04 14:51:12, perkj_webrtc wrote: > are all these modifications really needed? Nope. Removed most. https://codereview.webrtc.org/2106183002/diff/80001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:1163: int32_t SetRates(uint32_t new_target_bitrate, uint32_t framerate) override { On 2016/07/04 14:51:12, perkj_webrtc wrote: > not needed any more? Done. https://codereview.webrtc.org/2106183002/diff/80001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:1190: EXPECT_EQ(0, call_->GetStats().min_transmit_bitrate_bps); On 2016/07/04 14:51:12, perkj_webrtc wrote: > why not simply call this max_padding_bitrate_bps ? This is not the min transmit > bitrate. As discussed offline, yes and no. :) Changed anyhow. https://codereview.webrtc.org/2106183002/diff/80001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:1194: if (++frames_sent_ < kMinFramesToSend) On 2016/07/04 14:51:12, perkj_webrtc wrote: > nit frames_encoded_ ? Acknowledged.
sprang@webrtc.org changed reviewers: + tommi@webrtc.org - mflodman@webrtc.org
-mflodman +tommi ptal for call.h
lgtm
lgtm with two nits. https://codereview.webrtc.org/2106183002/diff/100001/webrtc/video/video_send_... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/2106183002/diff/100001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:1169: // Wait until at least kMinFramesToSend frames have been encoded, so that kMinPacketsToSend ? https://codereview.webrtc.org/2106183002/diff/100001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:1175: // We've sent kMinFramesToSend frames with default configuration, switch kMinPacketsToSend
https://codereview.webrtc.org/2106183002/diff/100001/webrtc/video/video_send_... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/2106183002/diff/100001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:1169: // Wait until at least kMinFramesToSend frames have been encoded, so that On 2016/07/05 12:41:16, perkj_webrtc wrote: > kMinPacketsToSend ? Done. https://codereview.webrtc.org/2106183002/diff/100001/webrtc/video/video_send_... webrtc/video/video_send_stream_tests.cc:1175: // We've sent kMinFramesToSend frames with default configuration, switch On 2016/07/05 12:41:16, perkj_webrtc wrote: > kMinPacketsToSend Done.
The CQ bit was checked by sprang@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org, tommi@webrtc.org, perkj@webrtc.org Link to the patchset: https://codereview.webrtc.org/2106183002/#ps120001 (title: "Fixed nits")
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: android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by perkj@webrtc.org
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: android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by perkj@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Fix bug where min transmit bitrate wasn't working A recent refactoring (r13192) introduced a bug where the min transmit config wasn't being respected. Specifically, if a VideoSendStream was created without it and the reconfigured, the min transmit bitrate would not take effect. Probably the other way around as well. BUG=webrtc::5687 ========== to ========== Fix bug where min transmit bitrate wasn't working A recent refactoring (r13192) introduced a bug where the min transmit config wasn't being respected. Specifically, if a VideoSendStream was created without it and the reconfigured, the min transmit bitrate would not take effect. Probably the other way around as well. BUG=webrtc::5687 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Fix bug where min transmit bitrate wasn't working A recent refactoring (r13192) introduced a bug where the min transmit config wasn't being respected. Specifically, if a VideoSendStream was created without it and the reconfigured, the min transmit bitrate would not take effect. Probably the other way around as well. BUG=webrtc::5687 ========== to ========== Fix bug where min transmit bitrate wasn't working A recent refactoring (r13192) introduced a bug where the min transmit config wasn't being respected. Specifically, if a VideoSendStream was created without it and the reconfigured, the min transmit bitrate would not take effect. Probably the other way around as well. BUG=webrtc::5687 Committed: https://crrev.com/9c0b55142533ebfa20836609777aa7f3681cadbe Cr-Commit-Position: refs/heads/master@{#13390} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/9c0b55142533ebfa20836609777aa7f3681cadbe Cr-Commit-Position: refs/heads/master@{#13390}
Message was sent while issue was closed.
Patchset #8 (id:140001) has been deleted |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
