Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2407143002:
Remove GetFeedbackInterval in sender side BWE. (Closed)
DescriptionRemove GetFeedbackInterval in sender side BWE.
And changed the minimum increase rate in |aimd_rate_control| to prevent the system from overusing on short twcc report send interval.
BUG=webrtc:6514
Review-Url: https://codereview.webrtc.org/2407143002
Cr-Commit-Position: refs/heads/master@{#17794}
Committed: https://chromium.googlesource.com/external/webrtc/+/8490f8af2112f7dfd45ce4009b05e167b2cae565
Patch Set 1 #Patch Set 2 : Adapt unittest to the change. #
Total comments: 1
Patch Set 3 : responde to comments #Patch Set 4 : rebased #Patch Set 5 : remove wait time on overuse, and adapted unit-tests #Patch Set 6 : Update aimd_rate_control just after a incoming feedback vector. #Patch Set 7 : rebased #
Total comments: 8
Patch Set 8 : Rebased #
Total comments: 12
Patch Set 9 : Responsed to comments #
Total comments: 8
Patch Set 10 : Respond to comments #
Total comments: 6
Patch Set 11 : Respond to comments #
Total comments: 4
Patch Set 12 : Responsed to comments #Patch Set 13 : Rebased #
Total comments: 2
Patch Set 14 : Rebased #
Total comments: 2
Patch Set 15 : Solve issue with delayed_feedback. #
Total comments: 1
Patch Set 16 : Respond to comments #Messages
Total messages: 99 (51 generated)
Description was changed from ========== Replace GetFeedbackInterval by RTT BUG= ========== to ========== Replace GetFeedbackInterval by RTT BUG=webrtc:6514 ==========
michaelt@webrtc.org changed reviewers: + minyue@webrtc.org, stefan@webrtc.org
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: win_x64_clang_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_dbg/build...)
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/2407143002/diff/20001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2407143002/diff/20001/webrtc/modules/congestion... webrtc/modules/congestion_controller/delay_based_bwe.cc:137: (last_update_ms_ == -1 || now_ms - last_update_ms_ > rtt_ms_)) { Do you think there's a true value having this check? I think it's possible to remove it as it doesn't seem meaningful when the BWE is running on the send-side and already is limited by the frequency of the feedback reports.
Also mention that you change the min increase to 250 bps from 1000 bps, as that's probably the main functionality change here, right?
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: ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/11664) ios64_gyp_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gyp_dbg/builds/1567) ios64_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/11686) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/14012) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/17922) 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 ========== Replace GetFeedbackInterval by RTT BUG=webrtc:6514 ========== to ========== Remove GetFeedbackInterval in sender side BWE. And changed the minimum increase rate in |aimd_rate_control| to prevent the system from overusing on short twcc report send interval. BUG=webrtc:6514 ==========
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: win_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/19162)
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.
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: ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/11827) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/14179) ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/14068) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/19416)
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/2407143002/diff/120001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/delay_based_bwe.cc (left): https://codereview.webrtc.org/2407143002/diff/120001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe.cc:121: rate_control_.TimeToReduceFurther(now_ms, *incoming_rate)) { Isn't this check still desirable? We shouldn't reduce the bitrate more than once per rtt since we can't see the effect of it before that? https://codereview.webrtc.org/2407143002/diff/120001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2407143002/diff/120001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe.cc:121: return Result(); I find this method a bit strange now. It basically never returns a target bitrate, even if it actually has updated it? What's the point of that? https://codereview.webrtc.org/2407143002/diff/120001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc (right): https://codereview.webrtc.org/2407143002/diff/120001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc:359: EXPECT_LT(bitrate_observer_.latest_bitrate(), bitrate_bps); I don't really understand this expectation. I also don't fully understand the previous expectation at the moment... WDYT the reasoning behind it is? https://codereview.webrtc.org/2407143002/diff/120001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe_unittest_helper.cc:377: const int kFramerate = 50; Could you comment on this change?
https://codereview.webrtc.org/2407143002/diff/120001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/delay_based_bwe.cc (left): https://codereview.webrtc.org/2407143002/diff/120001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe.cc:121: rate_control_.TimeToReduceFurther(now_ms, *incoming_rate)) { I removed it because it seamed strange to increase the the bit-rate without waiting time, but on the other hand wait in the overuse case. To me it would make most sense to wait mean_rtt between all bit-rate changes. https://codereview.webrtc.org/2407143002/diff/120001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2407143002/diff/120001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe.cc:121: return Result(); Since we work with a vector of packets, it seamed strange to me, asking aimd_rate_control for a new bit rate after each packet. Doing this after the vector seams to me the better solution. But of course there might be a better impl. for this.
https://codereview.webrtc.org/2407143002/diff/120001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/delay_based_bwe.cc (left): https://codereview.webrtc.org/2407143002/diff/120001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe.cc:121: rate_control_.TimeToReduceFurther(now_ms, *incoming_rate)) { On 2016/10/27 14:42:59, michaelt wrote: > I removed it because it seamed strange to increase the the bit-rate without > waiting time, but on the other hand wait in the overuse case. To me it would > make most sense to wait mean_rtt between all bit-rate changes. > I don't think I agree. Increasing is fine to do in small steps every time we get an ack of a packet delivery. That gives us a smooth increase over time. Reducing the bitrate is better done in one big step, and then waiting to see if it had the desired effect. If it did not we could decrease more. The logic we applied for increasing the rate doesn't apply since it doesn't make sense to take small steps down for every ack we get... https://codereview.webrtc.org/2407143002/diff/120001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2407143002/diff/120001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe.cc:121: return Result(); On 2016/10/27 14:42:59, michaelt wrote: > Since we work with a vector of packets, it seamed strange to me, asking > aimd_rate_control for a new bit rate after each packet. Doing this after the > vector seams to me the better solution. > > But of course there might be a better impl. for this. I agree with that. Seems odd to me that we return an empty result here and a result with bitrate 0 on line 118 though... Should we just have this method return bool instead?
Hi after a long time, i took a second look at this cl. The Rebase changed quite a lot. I tried to take care on the old comments.
stefan@webrtc.org changed reviewers: + terelius@webrtc.org
I'd like terelius review on this as well https://codereview.webrtc.org/2407143002/diff/140001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2407143002/diff/140001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe.cc:265: if (detector_.State() == kBwOverusing) { Given that you have to check the state also in IncomingPacketInfo it's not clear to me that this is becoming simpler. https://codereview.webrtc.org/2407143002/diff/140001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe.cc:346: // No overuse, but probing measured a bitrate. You have to check for overuse. Maybe you should move this out to IncomingPacketFeedbackVector as well and only Update the probe_bitrate_estimator_ with new packets (basically split HandleProbeAndEstimateBitrate() in two functions, Update() and GetEstimate())?
https://codereview.webrtc.org/2407143002/diff/140001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2407143002/diff/140001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe.cc:265: if (detector_.State() == kBwOverusing) { The idea was to split the Update in two parts. 1. Update the overuse detector and the rate_control (both done in the loop) 2. Updated the BWE and inform the listeners (not in the loop) The advantage of this would be not to do unnecessary calls on Updated the BWE. If you think the additional added complexity overweight this advantage i can revert this part of the CL.
https://codereview.webrtc.org/2407143002/diff/140001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2407143002/diff/140001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe.cc:265: if (detector_.State() == kBwOverusing) { On 2017/02/07 15:07:07, michaelt wrote: > The idea was to split the Update in two parts. > 1. Update the overuse detector and the rate_control (both done in the loop) > 2. Updated the BWE and inform the listeners (not in the loop) > The advantage of this would be not to do unnecessary calls on Updated the BWE. > > If you think the additional added complexity overweight this advantage i can > revert this part of the CL. > > Yes, I think that might be a good idea. It however seems like we are updating the rate control both inside and outside the loop. I suggest we move all of it outside.
https://codereview.webrtc.org/2407143002/diff/140001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2407143002/diff/140001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe.cc:265: if (detector_.State() == kBwOverusing) { Ok will try to clean this up :)
https://codereview.webrtc.org/2407143002/diff/140001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2407143002/diff/140001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe.cc:144: last_result_was_valid_ = true; Please be aware that the "!in_experiment"-path will be removed at some point. If you rely an a very specific behavior, you should check that it works with the new BitrateEstimator. https://codereview.webrtc.org/2407143002/diff/140001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe.cc:260: I like the idea of checking for overuse once per RTCP packet instead of for every acked RTP packet. However, it introduces a subtle but possibly significant change. The trendline estimators store the delay for the most recent packets to decide whether the delay is increasing. If we only check the slope occasionally (marked with ^ in the figure), we might miss the place where the delay increases. delay | ______________ | / ^ ^ | / | / | ___/ |__^___________________ time This is probably not a problem in practice, because the most likely reason for the delay leveling off is that we have filled the network buffers, and in this case the loss based estimator should kick in. However, it shows that this is non-trivial, and why we should expect this change to make the estimators less sensitive. Updating the detector thresholds may be necessary to compensate.
https://codereview.webrtc.org/2407143002/diff/140001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2407143002/diff/140001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe.cc:144: last_result_was_valid_ = true; This just e fix for the old bitrate_estimate_. It behave strange when it gets unordered packet arrival times. https://codereview.webrtc.org/2407143002/diff/140001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe.cc:260: I hope my impl. change will not influence the behavior of the trendline estimators. As far that i can see it it still gets the same input. https://codereview.webrtc.org/2407143002/diff/140001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe.cc:346: // No overuse, but probing measured a bitrate. I used now the target_bitrate_bps to pass the probing rate to MaybeUpdateEstimate. Would you still prefer to split HandleProbeAndEstimateBitrate.
https://codereview.webrtc.org/2407143002/diff/140001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2407143002/diff/140001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe.cc:346: // No overuse, but probing measured a bitrate. On 2017/02/08 12:05:32, michaelt wrote: > I used now the target_bitrate_bps to pass the probing rate to > MaybeUpdateEstimate. > Would you still prefer to split HandleProbeAndEstimateBitrate. I don't see the change, did you forgot to upload?
https://codereview.webrtc.org/2407143002/diff/140001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2407143002/diff/140001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe.cc:260: On 2017/02/08 12:05:32, michaelt wrote: > I hope my impl. change will not influence the behavior of the trendline > estimators. > As far that i can see it it still gets the same input. > Yes, it will get the same input and the detector will end up in the same state. The difference is that you are checking the state less often. This means that if the detector goes from "normal" to "overusing" and then back to "normal", we might miss the overusing period. This is not necessarily a problem, but it is definitely a change in behavior.
Oops :) now I have uploaded the change
Its hard to judge if something like that, could lead to a problem. Do you think we should run some special test on this ?
https://codereview.webrtc.org/2407143002/diff/160001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2407143002/diff/160001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe.cc:262: MaybeUpdateEstimate(&aggregated_result); Yes, I think we should run some tests and/or monitor Chrome stats. In the interest of landing this now, how about moving the MaybeUpdateEstimate into the for-loop above?
https://codereview.webrtc.org/2407143002/diff/160001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2407143002/diff/160001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe.cc:262: MaybeUpdateEstimate(&aggregated_result); I'm a bit afraid that if we move the function in to the loop that we'll never move it out again. And i would i general prefer to have this function not in the loop. Do you have any particular test in mind which could proof that we did nothing wrong here? Or should we use a field_trial for this ?
https://codereview.webrtc.org/2407143002/diff/160001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2407143002/diff/160001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe.cc:262: MaybeUpdateEstimate(&aggregated_result); On 2017/02/08 12:57:11, michaelt wrote: > I'm a bit afraid that if we move the function in to the loop that we'll never > move it out again. And i would i general prefer to have this function not in the > loop. > > Do you have any particular test in mind which could proof that we did nothing > wrong here? > > Or should we use a field_trial for this ? > > Note that the current version of the code essentially calls MaybeUpdateEstimate inside the loop. Is there any particular reason for updating the estimate after the loop? Yes, one option is to set up a field trial and do an experiment in Chrome canary build. This will require some extra work. Stefan, what do you think?
https://codereview.webrtc.org/2407143002/diff/160001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2407143002/diff/160001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe.cc:262: MaybeUpdateEstimate(&aggregated_result); To me, it is cleaner when we feed the overused defector in the loop and then update UpdateEstimate after the loop. Since It seams a bit strange to UpdateEstimate for each packets, if we get the packets in a vector. Especially since we have some time dependent functions in UpdateEstimate.
https://codereview.webrtc.org/2407143002/diff/160001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2407143002/diff/160001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe.cc:262: MaybeUpdateEstimate(&aggregated_result); On 2017/02/21 14:52:43, michaelt wrote: > To me, it is cleaner when we feed the overused defector in the loop and then > update UpdateEstimate after the loop. Since It seams a bit strange to > UpdateEstimate for each packets, if we get the packets in a vector. Especially > since we have some time dependent functions in UpdateEstimate. > I agree it is cleaner, but the rest of the code is tuned based on the assumption that we get an overuse signal for each packet. It is really hard to say what happens if we only check for overuse every 10-15 packets. It might work great, but then again, it might not...
https://codereview.webrtc.org/2407143002/diff/160001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2407143002/diff/160001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe.cc:262: MaybeUpdateEstimate(&aggregated_result); On 2017/02/21 14:52:43, michaelt wrote: > To me, it is cleaner when we feed the overused defector in the loop and then > update UpdateEstimate after the loop. Since It seams a bit strange to > UpdateEstimate for each packets, if we get the packets in a vector. Especially > since we have some time dependent functions in UpdateEstimate. > > I don't think we should run a finch experiment since it's quite a lot of work and it isn't very likely to reveal a noticeable difference. What do you think of changing MaybeUpdateEstimate so that it acts on kBwOverusing if any of the acked packets in packet_feedback_vector led to an overuse? That'd be closer to today's behavior I think.
https://codereview.webrtc.org/2407143002/diff/160001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2407143002/diff/160001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe.cc:262: MaybeUpdateEstimate(&aggregated_result); Sounds ok to me.
https://codereview.webrtc.org/2407143002/diff/160001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2407143002/diff/160001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe.cc:262: MaybeUpdateEstimate(&aggregated_result); On 2017/02/21 15:27:56, michaelt wrote: > Sounds ok to me. > Done.
https://codereview.webrtc.org/2407143002/diff/180001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2407143002/diff/180001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe.cc:327: probe_bitrate_estimator_.HandleProbeAndEstimateBitrate(info); I wonder if it makes sense to also split this up, so we update the prober here, but we don't read the result until later. Then we could return bool from DelayBasedBwe::IncomingPacketInfo instead of Result, and Result wouldn't have to contain "overusing". In addition, MaybeUpdateEstimate could simply return a Result instead of updating an existing Result and returning void. WDYT?
https://codereview.webrtc.org/2407143002/diff/180001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2407143002/diff/180001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe.cc:327: probe_bitrate_estimator_.HandleProbeAndEstimateBitrate(info); In general I like the idea. I took a look how we could split HandleProbeAndEstimateBitrate in to a Update and a GetEstimateBitrate function. The Problem i run in to was that GetEstimateBitrate would need the cluster_id to get the right bitrate. So we would need to pass the cluster_id as well to MaybeUpdateEstimate. This will lead to a problem when we have two different cluster_id's in the vector.
https://codereview.webrtc.org/2407143002/diff/180001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2407143002/diff/180001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe.cc:327: probe_bitrate_estimator_.HandleProbeAndEstimateBitrate(info); On 2017/02/27 10:45:41, michaelt wrote: > In general I like the idea. > I took a look how we could split HandleProbeAndEstimateBitrate in to a Update > and a GetEstimateBitrate function. > The Problem i run in to was that GetEstimateBitrate would need the cluster_id to > get the right bitrate. > So we would need to pass the cluster_id as well to MaybeUpdateEstimate. > This will lead to a problem when we have two different cluster_id's in the > vector. > Now you lost me, which GetEstimateBitrate? Are we talking about after you have split HandleProbeAndEstimateBitrate into two methods: void HandleProbe(info) and int GetEstimatedBitrate() ? I can't easily see how that's needed, but I could be missing something.
https://codereview.webrtc.org/2407143002/diff/180001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2407143002/diff/180001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe.cc:327: probe_bitrate_estimator_.HandleProbeAndEstimateBitrate(info); Yea exactly. Sorry if i was not clear enough. void HandleProbe(info) => would be easy to impl. but int GetEstimatedBitrate() would not work. We would have to make the interface like that: int GetEstimatedBitrate(int cluster_id) Since we need the cluster id to get the probe estimations.
https://codereview.webrtc.org/2407143002/diff/180001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2407143002/diff/180001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe.cc:327: probe_bitrate_estimator_.HandleProbeAndEstimateBitrate(info); On 2017/03/07 14:35:53, michaelt wrote: > Yea exactly. Sorry if i was not clear enough. > void HandleProbe(info) => would be easy to impl. > but > int GetEstimatedBitrate() would not work. > We would have to make the interface like that: > int GetEstimatedBitrate(int cluster_id) > Since we need the cluster id to get the probe estimations. But can't we just get the last probe estimation without an id?
https://codereview.webrtc.org/2407143002/diff/180001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2407143002/diff/180001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe.cc:327: probe_bitrate_estimator_.HandleProbeAndEstimateBitrate(info); On 2017/03/07 15:56:06, stefan-webrtc wrote: > On 2017/03/07 14:35:53, michaelt wrote: > > Yea exactly. Sorry if i was not clear enough. > > void HandleProbe(info) => would be easy to impl. > > but > > int GetEstimatedBitrate() would not work. > > We would have to make the interface like that: > > int GetEstimatedBitrate(int cluster_id) > > Since we need the cluster id to get the probe estimations. > > But can't we just get the last probe estimation without an id? I'm thinking basically to have HandleProbeAndEstimateBitrate() unchanged, just that it won't return an estimate, but instead store it. And then have a getter to get it later?
OK i think see your idea. Done.
I think this looks pretty good. Now we should be catching all over-uses, and based on that the aimd rate control increases linearly with time, it should be mostly independent on the number of updates. Probing is also handled correctly. lgtm from me, but terelius may still be able to find some other issues. :) https://codereview.webrtc.org/2407143002/diff/200001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2407143002/diff/200001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe.cc:329: rtc::Optional<int> porbe_bitrate_bps = probe_bitrate_bps https://codereview.webrtc.org/2407143002/diff/200001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/probe_bitrate_estimator.cc (right): https://codereview.webrtc.org/2407143002/diff/200001/webrtc/modules/congestio... webrtc/modules/congestion_controller/probe_bitrate_estimator.cc:120: rtc::Optional<int> ProbeBitrateEstimator::FetchLastEstimatedBitrateBps() { FetchAndReset...
https://codereview.webrtc.org/2407143002/diff/200001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2407143002/diff/200001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe.cc:329: rtc::Optional<int> porbe_bitrate_bps = On 2017/03/28 08:38:28, stefan-webrtc wrote: > probe_bitrate_bps Done. https://codereview.webrtc.org/2407143002/diff/200001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/probe_bitrate_estimator.cc (right): https://codereview.webrtc.org/2407143002/diff/200001/webrtc/modules/congestio... webrtc/modules/congestion_controller/probe_bitrate_estimator.cc:120: rtc::Optional<int> ProbeBitrateEstimator::FetchLastEstimatedBitrateBps() { On 2017/03/28 08:38:28, stefan-webrtc wrote: > FetchAndReset... Done.
This does not appear to be rebased recently. In particular, there is no rtc_event_log in the DelayBasedBwe.
On 2017/03/28 14:28:59, terelius wrote: > This does not appear to be rebased recently. In particular, there is no > rtc_event_log in the DelayBasedBwe. I reabsed the CL
looks good https://codereview.webrtc.org/2407143002/diff/240001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2407143002/diff/240001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe.cc:168: last_result_was_valid_ = true; I'm a little bit worried that this might reset the estimator too often. Stefan has a CL up that removes old_estimator and the "if (!in_experiment_) {...}" code (https://codereview.webrtc.org/2749803002/). If we can land his CL first we wouldn't need this hack.
https://codereview.webrtc.org/2407143002/diff/240001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2407143002/diff/240001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe.cc:168: last_result_was_valid_ = true; Ok the hack is removed.
lgtm
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: linux_msan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_msan/builds/19607)
Hi the unittest reviled a behavior change in the case of delayed feedback. I changed the impl of MaybeUpdateEstimate to fix this. Pleas take a look.
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/...
https://codereview.webrtc.org/2407143002/diff/260001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2407143002/diff/260001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe.cc:205: return MaybeUpdateEstimate(overusing); Wouldn't it be easier and cleaner to just not call MaybeUpdateEstimate if the feedback is delayed? https://codereview.webrtc.org/2407143002/diff/280001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2407143002/diff/280001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe.cc:269: if (delayed_feedback) { If delayed and normal feedback are handled by mutually exclusive code paths and operating on completely separate states, is there any reason for handling both by the same function?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2407143002/diff/260001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/delay_based_bwe.cc (right): https://codereview.webrtc.org/2407143002/diff/260001/webrtc/modules/congestio... webrtc/modules/congestion_controller/delay_based_bwe.cc:205: return MaybeUpdateEstimate(overusing); Hmm i don't have a strong opinion here. On one hand both paths may would update/set the estimation. So they would both fit in to a function which is called "MaybeUpdateEstimate". On the other hand MaybeUpdateEstimate gets another path which makes the function almost too complex. So if you prefer the have the delayed_feedback logic in IncomingPacketFeedbackVector i would be Ok with it as well.
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, terelius@webrtc.org Link to the patchset: https://codereview.webrtc.org/2407143002/#ps280001 (title: "Solve issue with delayed_feedback.")
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 michaelt@webrtc.org
Ok I moved the OnLongFeedbackDelay back in IncomingPacketFeedbackVector
thanks, still lgtm
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.
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 Link to the patchset: https://codereview.webrtc.org/2407143002/#ps300001 (title: "Respond to comments")
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": 300001, "attempt_start_ts": 1492708046573690,
"parent_rev": "1c23e944ab55ffb054eeb27c824a495406356055", "commit_rev":
"8490f8af2112f7dfd45ce4009b05e167b2cae565"}
Message was sent while issue was closed.
Description was changed from ========== Remove GetFeedbackInterval in sender side BWE. And changed the minimum increase rate in |aimd_rate_control| to prevent the system from overusing on short twcc report send interval. BUG=webrtc:6514 ========== to ========== Remove GetFeedbackInterval in sender side BWE. And changed the minimum increase rate in |aimd_rate_control| to prevent the system from overusing on short twcc report send interval. BUG=webrtc:6514 Review-Url: https://codereview.webrtc.org/2407143002 Cr-Commit-Position: refs/heads/master@{#17794} Committed: https://chromium.googlesource.com/external/webrtc/+/8490f8af2112f7dfd45ce4009... ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001) as https://chromium.googlesource.com/external/webrtc/+/8490f8af2112f7dfd45ce4009... |
