Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2381833003:
Change TWCC send interval to reduce overhead on low BW situations. (Closed)
DescriptionChange TWCC send interval to reduce overhead on low BW situations.
BUG=webrtc:6442
Committed: https://crrev.com/861e9374640eaa37ba5d905e3e0971df04b4fc9e
Cr-Commit-Position: refs/heads/master@{#14910}
Patch Set 1 #Patch Set 2 : Rebased #
Total comments: 2
Patch Set 3 : Change TWCC send interval according to the available bandwidth. #
Total comments: 5
Patch Set 4 : fix unit-test build #
Total comments: 18
Patch Set 5 : response to comments #Patch Set 6 : Changed average size of TWCC report. #Patch Set 7 : Response to comments #
Total comments: 2
Patch Set 8 : Added a lock for bitrate_bps. #Patch Set 9 : Move send interval calc. to OnBitrateChanged. #
Total comments: 2
Patch Set 10 : fix nit's #
Messages
Total messages: 49 (27 generated)
Description was changed from ========== Change TWCC send interval to reduce overhead on low BW situations. Change TWCC send interval according to the incoming bitrate. BUG= ========== to ========== Change TWCC send interval to reduce overhead on low BW situations. Change TWCC send interval according to the incoming bitrate. BUG= ==========
michaelt@webrtc.org changed reviewers: + minyue@webrtc.org
The CQ bit was checked by minyue@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: ios64_gyp_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gyp_dbg/builds/956) linux_ubsan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/5459) mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
Description was changed from ========== Change TWCC send interval to reduce overhead on low BW situations. Change TWCC send interval according to the incoming bitrate. BUG= ========== to ========== Change TWCC send interval to reduce overhead on low BW situations. BUG= ==========
Description was changed from ========== Change TWCC send interval to reduce overhead on low BW situations. BUG= ========== to ========== Change TWCC send interval to reduce overhead on low BW situations. BUG=6442 ==========
Description was changed from ========== Change TWCC send interval to reduce overhead on low BW situations. BUG=6442 ========== to ========== Change TWCC send interval to reduce overhead on low BW situations. BUG=webrtc:6442 ==========
The CQ bit was checked by michaelt@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
michaelt@webrtc.org changed reviewers: + stefan@webrtc.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2381833003/diff/20001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc (right): https://codereview.webrtc.org/2381833003/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc:29: const int RemoteEstimatorProxy::kSwitchToHighBitrateProcessIntervalBps = 100000; I'm starting to lean towards making the interval a linear function of bitrate instead. Something like this: interval = std::max(250 - bitrate, 50) WDYT? https://codereview.webrtc.org/2381833003/diff/20001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc (right): https://codereview.webrtc.org/2381833003/diff/20001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc:63: testing::NiceMock<rtc::MockRateTracker>* mock_received_bitrate_tracker_bps_; Instead of using a mock I'd suggest that we call IncomingPacket() with the right packet size and interval to simulate the bitrate we want to test with. That more fully tests the functionality, and avoids having to create a mock class.
I change the concept of the impl. because when i was rethinking this, i became aware the the twcc send interval should be dependent on the BWE and not on the incoming bit-rate. Since its our own sending bandwidth we use for the twcc reports. The minimum send interval is set to 50ms. The maximum send intervale is set to 250ms. Between min and max the send interval is set, so that we use 5% of the available bandwidth.
The CQ bit was checked by michaelt@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: mac_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/18637) mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
Patchset #4 (id:60001) has been deleted
The CQ bit was checked by michaelt@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2381833003/diff/40001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc (right): https://codereview.webrtc.org/2381833003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc:77: int64_t RemoteEstimatorProxy::TimeUntilNextProcess() { general question: should we update send_interval_ms in this function? It feels to me that this function is not necessarily called on a regular time basis. https://codereview.webrtc.org/2381833003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc:86: kTwccReportSize * 8.0 * 1000.0 / (0.05 * bitrate) + 0.5; Would you add a comment, something like: "Let TWCC occupy 5% of total bandwidth."
https://codereview.webrtc.org/2381833003/diff/80001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc (right): https://codereview.webrtc.org/2381833003/diff/80001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc:83: // Ipv4(20B) + UDP(8B) + SRTP(4B) + AverageTwccReport(20B) Is SRTP always 4B? https://codereview.webrtc.org/2381833003/diff/80001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc:86: kTwccReportSize * 8.0 * 1000.0 / (0.05 * bitrate) + 0.5; You may have to explicitly cast to int. https://codereview.webrtc.org/2381833003/diff/80001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h (right): https://codereview.webrtc.org/2381833003/diff/80001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h:40: BitrateController* bitrate_controller); I'm not sure I like this dependency... We can't know if the receiver is also a sender, and if it's a pure receiver it won't have a valid bandwidth estimate on its uplink. In that case I think it's better to base this on the bitrate received? https://codereview.webrtc.org/2381833003/diff/80001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h:58: static const int kMinPacketCountRecievedBitrate; Received https://codereview.webrtc.org/2381833003/diff/80001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc (right): https://codereview.webrtc.org/2381833003/diff/80001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc:50: clock_.AdvanceTimeMilliseconds(50); Why did you change this?
https://codereview.webrtc.org/2381833003/diff/80001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc (right): https://codereview.webrtc.org/2381833003/diff/80001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc:83: // Ipv4(20B) + UDP(8B) + SRTP(4B) + AverageTwccReport(20B) No i depends on the crypto. which is used. Will take 10B as first guess. https://codereview.webrtc.org/2381833003/diff/80001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc:86: kTwccReportSize * 8.0 * 1000.0 / (0.05 * bitrate) + 0.5; Ok https://codereview.webrtc.org/2381833003/diff/80001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h (right): https://codereview.webrtc.org/2381833003/diff/80001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h:40: BitrateController* bitrate_controller); But if we use bitrate received we assume that we are in a symmetrical network witch is not necessary the case. https://codereview.webrtc.org/2381833003/diff/80001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h:58: static const int kMinPacketCountRecievedBitrate; Removed this const since its not used anymore. https://codereview.webrtc.org/2381833003/diff/80001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc (right): https://codereview.webrtc.org/2381833003/diff/80001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc:50: clock_.AdvanceTimeMilliseconds(50); Because there is no default process interval. The interval became with this CL bit-rate depended. I replaced it with RemoteEstimatorProxy::kMinSendIntervalMs
Hi Michael, I had some comments, good to take a look.
https://codereview.webrtc.org/2381833003/diff/80001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc (right): https://codereview.webrtc.org/2381833003/diff/80001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc:83: // Ipv4(20B) + UDP(8B) + SRTP(4B) + AverageTwccReport(20B) On 2016/10/27 15:15:44, michaelt wrote: > No i depends on the crypto. which is used. Will take 10B as first guess. > Could you also add a short comment on how you derive the average twcc report to be 20 bytes? https://codereview.webrtc.org/2381833003/diff/80001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h (right): https://codereview.webrtc.org/2381833003/diff/80001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h:40: BitrateController* bitrate_controller); On 2016/10/27 15:15:44, michaelt wrote: > But if we use bitrate received we assume that we are in a symmetrical network > witch is not necessary the case. > Right, when the client is receive-only we have to decide on something. Either we base it on the receive bitrate or we hard code it. WDYT? If we want the RemoteEstimatorProxy to have access to the send bandwidth I think I'd prefer to do it through an observer interface, for instance BitrateObserver (see webrtc/modules/bitrate_controller/include/bitrate_controller.h).
https://codereview.webrtc.org/2381833003/diff/80001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc (right): https://codereview.webrtc.org/2381833003/diff/80001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc:83: // Ipv4(20B) + UDP(8B) + SRTP(4B) + AverageTwccReport(20B) Did a mistake in the first measurement. I used the same concept for doing the measurement again. Measured the size at a interval of 50ms and at 250ms and took the average size. https://codereview.webrtc.org/2381833003/diff/80001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h (right): https://codereview.webrtc.org/2381833003/diff/80001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h:40: BitrateController* bitrate_controller); For this CL i would use a hard coded rate on the receive-only case. I discover some problems on using BitrateObserver. 1. BitrateObserver seams to be deprecated. 2. It seams that in the current impl. there is no way to inform multiple BitrateObserver. What do you think about set the current send bandwidth in RemoteEstimatorProxy with a function "OnBitrateChanged" called in the function "MaybeTriggerOnNetworkChanged" in the CongestionController ?
https://codereview.webrtc.org/2381833003/diff/80001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc (right): https://codereview.webrtc.org/2381833003/diff/80001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc:83: // Ipv4(20B) + UDP(8B) + SRTP(4B) + AverageTwccReport(20B) On 2016/10/31 13:03:15, michaelt wrote: > Did a mistake in the first measurement. I used the same concept for doing the > measurement again. > Measured the size at a interval of 50ms and at 250ms and took the average size. I see. Could you just add a comment on that so that future updates to the constant can be easily done? https://codereview.webrtc.org/2381833003/diff/80001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h (right): https://codereview.webrtc.org/2381833003/diff/80001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h:40: BitrateController* bitrate_controller); On 2016/10/31 13:03:15, michaelt wrote: > For this CL i would use a hard coded rate on the receive-only case. > > I discover some problems on using BitrateObserver. > 1. BitrateObserver seams to be deprecated. > 2. It seams that in the current impl. there is no way to inform multiple > BitrateObserver. > > What do you think about set the current send bandwidth in RemoteEstimatorProxy > with a function "OnBitrateChanged" called in the function > "MaybeTriggerOnNetworkChanged" in the CongestionController ? Constant rate for receive only and using OnBitrateChanged() both sound good to me.
https://codereview.webrtc.org/2381833003/diff/80001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc (right): https://codereview.webrtc.org/2381833003/diff/80001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc:83: // Ipv4(20B) + UDP(8B) + SRTP(4B) + AverageTwccReport(20B) On 2016/10/31 14:04:19, stefan-webrtc (holmer) wrote: > On 2016/10/31 13:03:15, michaelt wrote: > > Did a mistake in the first measurement. I used the same concept for doing the > > measurement again. > > Measured the size at a interval of 50ms and at 250ms and took the average > size. > > I see. Could you just add a comment on that so that future updates to the > constant can be easily done? Done. https://codereview.webrtc.org/2381833003/diff/80001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h (right): https://codereview.webrtc.org/2381833003/diff/80001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h:40: BitrateController* bitrate_controller); On 2016/10/31 14:04:19, stefan-webrtc (holmer) wrote: > On 2016/10/31 13:03:15, michaelt wrote: > > For this CL i would use a hard coded rate on the receive-only case. > > > > I discover some problems on using BitrateObserver. > > 1. BitrateObserver seams to be deprecated. > > 2. It seams that in the current impl. there is no way to inform multiple > > BitrateObserver. > > > > What do you think about set the current send bandwidth in RemoteEstimatorProxy > > with a function "OnBitrateChanged" called in the function > > "MaybeTriggerOnNetworkChanged" in the CongestionController ? > > Constant rate for receive only and using OnBitrateChanged() both sound good to > me. Done.
lg, but we need to ensure the thread safety... https://codereview.webrtc.org/2381833003/diff/140001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc (right): https://codereview.webrtc.org/2381833003/diff/140001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc:113: bitrate_bps_ = rtc::Optional<int>(bitrate_bps); Is this accessed on the same thread as TimeUntilNextProcess? In that case we may want to use a SequenceChecker to verify. Otherwise you'll need to lock.
The function are not called from the same thread. Added a lock. https://codereview.webrtc.org/2381833003/diff/140001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc (right): https://codereview.webrtc.org/2381833003/diff/140001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc:113: bitrate_bps_ = rtc::Optional<int>(bitrate_bps); On 2016/11/02 13:10:24, stefan-webrtc (holmer) wrote: > Is this accessed on the same thread as TimeUntilNextProcess? In that case we may > want to use a SequenceChecker to verify. Otherwise you'll need to lock. Done.
lgtm
https://codereview.webrtc.org/2381833003/diff/40001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc (right): https://codereview.webrtc.org/2381833003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc:77: int64_t RemoteEstimatorProxy::TimeUntilNextProcess() { On 2016/10/26 10:11:35, minyue-webrtc wrote: > general question: should we update send_interval_ms in this function? > > It feels to me that this function is not necessarily called on a regular time > basis. Since send_interval_ms is just dependent on the actual BWE. I moved the calculation of send_interval_ms to OnBitrateChanged. https://codereview.webrtc.org/2381833003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc:86: kTwccReportSize * 8.0 * 1000.0 / (0.05 * bitrate) + 0.5; On 2016/10/26 10:11:35, minyue-webrtc wrote: > Would you add a comment, something like: > > "Let TWCC occupy 5% of total bandwidth." Done.
lgtm % nit https://codereview.webrtc.org/2381833003/diff/180001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h (right): https://codereview.webrtc.org/2381833003/diff/180001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h:76: int send_interval_ms_ GUARDED_BY(&lock_); int64_t for time
lgtm https://codereview.webrtc.org/2381833003/diff/40001/webrtc/modules/remote_bit... File webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc (right): https://codereview.webrtc.org/2381833003/diff/40001/webrtc/modules/remote_bit... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.cc:77: int64_t RemoteEstimatorProxy::TimeUntilNextProcess() { On 2016/11/03 10:20:20, michaelt wrote: > On 2016/10/26 10:11:35, minyue-webrtc wrote: > > general question: should we update send_interval_ms in this function? > > > > It feels to me that this function is not necessarily called on a regular time > > basis. > > Since send_interval_ms is just dependent on the actual BWE. I moved the > calculation of send_interval_ms to OnBitrateChanged. That reads much nicer to me.
https://codereview.webrtc.org/2381833003/diff/180001/webrtc/modules/remote_bi... File webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h (right): https://codereview.webrtc.org/2381833003/diff/180001/webrtc/modules/remote_bi... webrtc/modules/remote_bitrate_estimator/remote_estimator_proxy.h:76: int send_interval_ms_ GUARDED_BY(&lock_); On 2016/11/03 10:28:43, stefan-webrtc (holmer) wrote: > int64_t for time Done.
The CQ bit was checked by michaelt@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org, minyue@webrtc.org Link to the patchset: https://codereview.webrtc.org/2381833003/#ps200001 (title: "fix nit's")
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 ========== Change TWCC send interval to reduce overhead on low BW situations. BUG=webrtc:6442 ========== to ========== Change TWCC send interval to reduce overhead on low BW situations. BUG=webrtc:6442 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Change TWCC send interval to reduce overhead on low BW situations. BUG=webrtc:6442 ========== to ========== Change TWCC send interval to reduce overhead on low BW situations. BUG=webrtc:6442 Committed: https://crrev.com/861e9374640eaa37ba5d905e3e0971df04b4fc9e Cr-Commit-Position: refs/heads/master@{#14910} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/861e9374640eaa37ba5d905e3e0971df04b4fc9e Cr-Commit-Position: refs/heads/master@{#14910}
Message was sent while issue was closed.
A revert of this CL (patchset #10 id:200001) has been created in https://codereview.webrtc.org/2468413009/ by ivoc@webrtc.org. The reason for reverting is: Breaks internal tests.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
