Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2931873002:
Test and fix for huge bwe drop after alr state. (Closed)
|
Created:
3 years, 6 months ago by tschumi Modified:
3 years, 6 months ago Reviewers:
holmer, terelius, stefan-webrtc, philipel, kwiberg-webrtc, minyue-webrtc, the sun, oprypin_webrtc, Stefan CC:
webrtc-reviews_webrtc.org, zhuangzesen_agora.io, tlegrand-webrtc, tterriberry_mozilla.com, the sun, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionTest and fix for huge bwe drop after alr state.
BUG=webrtc:7746
Review-Url: https://codereview.webrtc.org/2931873002
Cr-Commit-Position: refs/heads/master@{#18692}
Committed: https://chromium.googlesource.com/external/webrtc/+/37aa8ba61641962119071646175bfe3bc2bda063
Patch Set 1 #
Total comments: 25
Patch Set 2 : Respond to commants #
Total comments: 6
Patch Set 3 : Removed BweTrendlineFilter field trial. #Patch Set 4 : Renamed SentBeforeLeftAlr to SentInAlrState added a test. #
Total comments: 10
Patch Set 5 : Fixed field trial string. #Patch Set 6 : Response to comments. #
Total comments: 15
Patch Set 7 : . #
Total comments: 4
Patch Set 8 : Respons to comments #
Total comments: 4
Patch Set 9 : . #Patch Set 10 : Moved Dtx integraion test. #Patch Set 11 : Rebased #Patch Set 12 : Added missing dep. #Patch Set 13 : fix_for_unittest #Messages
Total messages: 78 (44 generated)
Description was changed from ========== Test and fix for huge bwe drop after alr state. BUG=webrtc:7746 ========== to ========== Test and fix for huge bwe drop after alr state. BUG=webrtc:7746 ==========
tschumim@webrtc.org changed reviewers: + kwiberg@google.com
@kwiberg, Used the std::function in acknowledge_bitrate_estimator.h for acknowledge_bitrate_estimator_unittest.cc.
tschumim@webrtc.org changed reviewers: + solenberg@webrtc.org
kwiberg@webrtc.org changed reviewers: + kwiberg@webrtc.org - kwiberg@google.com
Review of just the std::function bits. https://codereview.webrtc.org/2931873002/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.cc (right): https://codereview.webrtc.org/2931873002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.cc:32: : &rtc::MakeUnique<BitrateEstimator>), Can you remove this fallback, and force all callers to pass a non-empty bitrate estimator creator? That makes the interface between AcknowledgedBitrateEstimator and its callers simpler---at the cost of adding some complexity at the call sites---which is usually a net win. https://codereview.webrtc.org/2931873002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.cc:70: bitrate_estimator_ = bitrate_estimator_creator_(); Hmm. If BitrateEstimator had a Reset() method, you wouldn't have to have a creator object at all. Is that a viable approach? (That's what we ended up doing with AudioEncoder, and it made our lives much simpler.) https://codereview.webrtc.org/2931873002/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.h (right): https://codereview.webrtc.org/2931873002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.h:17: #include "webrtc/base/function_view.h" Remove this? You don't appear to use it. https://codereview.webrtc.org/2931873002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.h:26: std::function<std::unique_ptr<BitrateEstimator>()>; Hmm. I wonder if it wouldn't be simpler (in the sense of making the code easier to read) with an interface instead: class BitrateEstimatorCreator { public: virtual ~BitrateEstimatorCreator() = default; virtual std::unique_ptr<BitrateEstimator> CreateBitrateEstimator() = 0; }; And have the AcknowledgedBitrateEstimator constructor take a std::unique_ptr<BitrateEstimatorCreator>. It would be more lines of code at each call site, though. How bad would that be? https://codereview.webrtc.org/2931873002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.h:31: BitrateEstimatorCreator&& bitrate_estimator_creator = nullptr); The style guide says to not use rvalue ref arguments. Pass by value instead?
tschumim@webrtc.org changed reviewers: + minyue@webrtc.org
tschumim@webrtc.org changed reviewers: + holmer@chromium.org, philipel@webrtc.org, terelius@webrtc.org
lg, mostly stylistic comments. https://codereview.webrtc.org/2931873002/diff/1/webrtc/audio/test/low_bandwid... File webrtc/audio/test/low_bandwidth_audio_test.cc (right): https://codereview.webrtc.org/2931873002/diff/1/webrtc/audio/test/low_bandwid... webrtc/audio/test/low_bandwidth_audio_test.cc:234: TEST_F(LowBandwidthAudioTest, DTXTest) { Could you find some more descriptive name for this test? https://codereview.webrtc.org/2931873002/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.cc (right): https://codereview.webrtc.org/2931873002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.cc:52: bool AcknowledgedBitrateEstimator::ShouldSkipPreAlrPacket( WDYT about renaming this to SentInAlr? https://codereview.webrtc.org/2931873002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.cc:69: if (HasLeftAlrState(alr_state)) { If you only expect HasLeftAlrState to be called from here, remove it and replace it with something like: bool has_left_alr = last_alr_state_ && !alr_state;
minyue@webrtc.org changed reviewers: + oprypin@webrtc.org
On 2017/06/09 14:49:53, philipel wrote: > lg, mostly stylistic comments. > > https://codereview.webrtc.org/2931873002/diff/1/webrtc/audio/test/low_bandwid... > File webrtc/audio/test/low_bandwidth_audio_test.cc (right): > > https://codereview.webrtc.org/2931873002/diff/1/webrtc/audio/test/low_bandwid... > webrtc/audio/test/low_bandwidth_audio_test.cc:234: TEST_F(LowBandwidthAudioTest, > DTXTest) { > Could you find some more descriptive name for this test? > > https://codereview.webrtc.org/2931873002/diff/1/webrtc/modules/congestion_con... > File webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.cc > (right): > > https://codereview.webrtc.org/2931873002/diff/1/webrtc/modules/congestion_con... > webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.cc:52: bool > AcknowledgedBitrateEstimator::ShouldSkipPreAlrPacket( > WDYT about renaming this to SentInAlr? > > https://codereview.webrtc.org/2931873002/diff/1/webrtc/modules/congestion_con... > webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.cc:69: if > (HasLeftAlrState(alr_state)) { > If you only expect HasLeftAlrState to be called from here, remove it and replace > it with something like: > bool has_left_alr = last_alr_state_ && !alr_state; +oleh for the low_bandwidth_audio_test
Good job! Take a look at my comments. I would like to understand why not combining the old acknowledged_bwe with the new one. https://codereview.webrtc.org/2931873002/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.h (right): https://codereview.webrtc.org/2931873002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.h:28: class AcknowledgedBitrateEstimator { shouldn't the file name be called acknowledge"d"_bitrate_estimator? https://codereview.webrtc.org/2931873002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.h:35: bool alr_state); what is alr? good to mention it in a comment https://codereview.webrtc.org/2931873002/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/bitrate_estimator.h (right): https://codereview.webrtc.org/2931873002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/bitrate_estimator.h:25: class BitrateEstimator { why not just modifying the old one? https://codereview.webrtc.org/2931873002/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/send_side_congestion_controller.cc (right): https://codereview.webrtc.org/2931873002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/send_side_congestion_controller.cc:283: static_cast<bool>(pacer_->GetApplicationLimitedRegionStartTime())); no need for explicit cast
https://codereview.webrtc.org/2931873002/diff/1/webrtc/audio/test/low_bandwid... File webrtc/audio/test/low_bandwidth_audio_test.cc (right): https://codereview.webrtc.org/2931873002/diff/1/webrtc/audio/test/low_bandwid... webrtc/audio/test/low_bandwidth_audio_test.cc:234: TEST_F(LowBandwidthAudioTest, DTXTest) { On 2017/06/09 14:49:53, philipel wrote: > Could you find some more descriptive name for this test? Done. https://codereview.webrtc.org/2931873002/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.cc (right): https://codereview.webrtc.org/2931873002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.cc:32: : &rtc::MakeUnique<BitrateEstimator>), Done. But I'm not completely sure if the added complexity is worth the better readability of the ctor. WDYT ? https://codereview.webrtc.org/2931873002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.cc:52: bool AcknowledgedBitrateEstimator::ShouldSkipPreAlrPacket( On 2017/06/09 14:49:53, philipel wrote: > WDYT about renaming this to SentInAlr? Hmm "SentInAlr" tells just parts of what this function try's to do. What about "SentBeforeLeftAlr" ? https://codereview.webrtc.org/2931873002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.cc:69: if (HasLeftAlrState(alr_state)) { I assumed that the compiler will inline this anyway. So we would end-up with more or less the same code. And in terms of code readability, I would go for the "if (HasLeftAlrState(alr_state))" WDYT ? https://codereview.webrtc.org/2931873002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.cc:70: bitrate_estimator_ = bitrate_estimator_creator_(); It would be much simpler. Having a reset function can be a pain as well. Since you have to make sure the all the states are handled the right way. So for now i would go with this no reset approach :) https://codereview.webrtc.org/2931873002/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.h (right): https://codereview.webrtc.org/2931873002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.h:17: #include "webrtc/base/function_view.h" On 2017/06/08 23:00:29, kwiberg-webrtc wrote: > Remove this? You don't appear to use it. Done. https://codereview.webrtc.org/2931873002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.h:26: std::function<std::unique_ptr<BitrateEstimator>()>; Ok I went for that solution. Looks fine to me :) https://codereview.webrtc.org/2931873002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.h:28: class AcknowledgedBitrateEstimator { On 2017/06/12 10:08:07, minyue-webrtc wrote: > shouldn't the file name be called acknowledge"d"_bitrate_estimator? Done. https://codereview.webrtc.org/2931873002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.h:31: BitrateEstimatorCreator&& bitrate_estimator_creator = nullptr); Pass now a std::unique_ptr<BitrateEstimatorCreator> https://codereview.webrtc.org/2931873002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/acknowledge_bitrate_estimator.h:35: bool alr_state); alr = ApplicationLimitedRegion I'm not sure if we should mention this at every point we use it. It feels to me this is kind of domain based knowledge, and its ok to use it like that. But I don't have a strong opinion on that. I general i like to do as less comment as possible. :) https://codereview.webrtc.org/2931873002/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/bitrate_estimator.h (right): https://codereview.webrtc.org/2931873002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/bitrate_estimator.h:25: class BitrateEstimator { Tow reasons: 1. I tried to not create a reset function, since this is always a bit of a pain in terms of code maintenance. And since i would like to reset the estimator, the easiest way was just to recreated it. 2. BitrateEstimator seamed already to be complicated enough in its own. So having two classes should make the code better readable and testable. https://codereview.webrtc.org/2931873002/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/send_side_congestion_controller.cc (right): https://codereview.webrtc.org/2931873002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/send_side_congestion_controller.cc:283: static_cast<bool>(pacer_->GetApplicationLimitedRegionStartTime())); It seams its needed, the compiler don't do a implicit cast from Optional to bool.
holmer@google.com changed reviewers: + holmer@google.com
https://codereview.webrtc.org/2931873002/diff/20001/webrtc/audio/test/low_ban... File webrtc/audio/test/low_bandwidth_audio_test.cc (right): https://codereview.webrtc.org/2931873002/diff/20001/webrtc/audio/test/low_ban... webrtc/audio/test/low_bandwidth_audio_test.cc:238: "Enabled/WebRTC-SendSideBwe-WithOverhead/Enabled/"); You can drop "WebRTC-BweTrendlineFilter/Enabled-20,0.9,4/" as it's already the default. https://codereview.webrtc.org/2931873002/diff/20001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.h (right): https://codereview.webrtc.org/2931873002/diff/20001/webrtc/modules/congestion... webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.h:41: bool SentBeforLeftAlr(const PacketFeedback& packet); SentBeforeLeftAlr What does left alr mean?
https://codereview.webrtc.org/2931873002/diff/20001/webrtc/audio/test/low_ban... File webrtc/audio/test/low_bandwidth_audio_test.cc (right): https://codereview.webrtc.org/2931873002/diff/20001/webrtc/audio/test/low_ban... webrtc/audio/test/low_bandwidth_audio_test.cc:238: "Enabled/WebRTC-SendSideBwe-WithOverhead/Enabled/"); On 2017/06/12 11:45:14, holmer wrote: > You can drop "WebRTC-BweTrendlineFilter/Enabled-20,0.9,4/" as it's already the > default. Acknowledged. https://codereview.webrtc.org/2931873002/diff/20001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.h (right): https://codereview.webrtc.org/2931873002/diff/20001/webrtc/modules/congestion... webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.h:41: bool SentBeforLeftAlr(const PacketFeedback& packet); The the function should return true when we left alr state and the packet we give to it was sent while we still where in alr state. :) I was not bale to come up with a good name till now.
stefan@webrtc.org changed reviewers: + stefan@webrtc.org
https://codereview.webrtc.org/2931873002/diff/20001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.h (right): https://codereview.webrtc.org/2931873002/diff/20001/webrtc/modules/congestion... webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.h:41: bool SentBeforLeftAlr(const PacketFeedback& packet); On 2017/06/12 11:53:04, tschumi wrote: > The the function should return true when we left alr state and the packet we > give to it was sent while we still where in alr state. :) > > I was not bale to come up with a good name till now. > Maybe split in two and make this: if (SentInAlrState(packet) && HasLeftAlrState(alr_state))...
https://codereview.webrtc.org/2931873002/diff/1/webrtc/modules/congestion_con... File webrtc/modules/congestion_controller/bitrate_estimator.h (right): https://codereview.webrtc.org/2931873002/diff/1/webrtc/modules/congestion_con... webrtc/modules/congestion_controller/bitrate_estimator.h:25: class BitrateEstimator { On 2017/06/12 11:33:28, tschumi wrote: > Tow reasons: > > 1. I tried to not create a reset function, since this is always a bit of a pain > in terms of code > maintenance. And since i would like to reset the estimator, the easiest way was > just to recreated it. > > 2. BitrateEstimator seamed already to be complicated enough in its own. So > having two classes should make > the code better readable and testable. > > > OK. But why was this one called AcknowledgedBitrateEstimator and why does the new one to be called AcknowledgedBitrateEstimator?
This is caused by a refactoring CL which I did to make this CL possible. https://codereview.webrtc.org/2917873002/ So it is kind of my fault :)
This is caused by a refactoring CL which I did to make this CL possible. https://codereview.webrtc.org/2917873002/ So it is kind of my fault :)
https://codereview.webrtc.org/2931873002/diff/20001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.h (right): https://codereview.webrtc.org/2931873002/diff/20001/webrtc/modules/congestion... webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.h:41: bool SentBeforLeftAlr(const PacketFeedback& packet); I tried to impl. this but the results where kind of silly. So I decided to go for "SentInAlrState". I hope everyone can live with that.
https://codereview.webrtc.org/2931873002/diff/60001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.cc (right): https://codereview.webrtc.org/2931873002/diff/60001/webrtc/modules/congestion... webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.cc:57: bool AcknowledgedBitrateEstimator::SentInAlrState( I'd also prefer a more descriptive name for this function. Or just manually inline it. https://codereview.webrtc.org/2931873002/diff/60001/webrtc/modules/congestion... webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.cc:63: left_alr_state_ms_.reset(); Do we really want to forget that we have left ALR just because we received a packet that was sent after the change?? If packets are acknowledged out of order we might still receive packets that were sent in ALR. Or we assume that we never reorder packets for more than 1 RTT? https://codereview.webrtc.org/2931873002/diff/60001/webrtc/modules/congestion... webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.cc:70: return last_alr_state_ && !alr_state; Maybe call them was_in_alr and currently_in_alr? Adding "_state" seems redundant for a boolean, and "Application Limited Region state" sounds strage to me. https://codereview.webrtc.org/2931873002/diff/60001/webrtc/modules/congestion... webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.cc:74: if (HasLeftAlrState(alr_state)) { Manually inline this function? https://codereview.webrtc.org/2931873002/diff/60001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/bitrate_estimator.cc (right): https://codereview.webrtc.org/2931873002/diff/60001/webrtc/modules/congestion... webrtc/modules/congestion_controller/bitrate_estimator.cc:22: BitrateEstimator::BitrateEstimator() I preferred the old AcknowledgedBitrateEstimator. Just BitrateEstimator could refer to the congestion controller, the delay based estimator, the loss based estimator or something else. Maybe a follow-up CL that moves towards a Reset()-based approach that also produces an estimate even before a full window has been received?
https://codereview.webrtc.org/2931873002/diff/60001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.cc (right): https://codereview.webrtc.org/2931873002/diff/60001/webrtc/modules/congestion... webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.cc:57: bool AcknowledgedBitrateEstimator::SentInAlrState( Renamed it to "SentBeforeAlrEnded" https://codereview.webrtc.org/2931873002/diff/60001/webrtc/modules/congestion... webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.cc:63: left_alr_state_ms_.reset(); I'm not sure if it's worth to add additional complexity here to handle the case of unordered packets. https://codereview.webrtc.org/2931873002/diff/60001/webrtc/modules/congestion... webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.cc:70: return last_alr_state_ && !alr_state; On 2017/06/13 14:03:20, terelius wrote: > Maybe call them was_in_alr and currently_in_alr? Adding "_state" seems redundant > for a boolean, and "Application Limited Region state" sounds strage to me. Acknowledged. https://codereview.webrtc.org/2931873002/diff/60001/webrtc/modules/congestion... webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.cc:74: if (HasLeftAlrState(alr_state)) { Don't you think the function name adds additional value in terms of readability ? Renamed it to "AlrEnded" according to "SentBeforeAlrEnded". https://codereview.webrtc.org/2931873002/diff/60001/webrtc/modules/congestion... File webrtc/modules/congestion_controller/bitrate_estimator.cc (right): https://codereview.webrtc.org/2931873002/diff/60001/webrtc/modules/congestion... webrtc/modules/congestion_controller/bitrate_estimator.cc:22: BitrateEstimator::BitrateEstimator() Since we have two classes at the moment which doing things with the acknowledged bitrate its hard tho call them both AcknowledgedBitrateEstimator. This was the reason i used here BitrateEstimator. If you have better ideas of how to name the classes, i'm open for it. A follow up CL which changes the behavior in direction of faster update and a better reset strategic is the right thing to do.
https://codereview.webrtc.org/2931873002/diff/100001/webrtc/audio/test/low_ba... File webrtc/audio/test/low_bandwidth_audio_test.cc (right): https://codereview.webrtc.org/2931873002/diff/100001/webrtc/audio/test/low_ba... webrtc/audio/test/low_bandwidth_audio_test.cc:209: void LogDelayBasedBweUpdate(int32_t bitrate_bps, Remove if not used. https://codereview.webrtc.org/2931873002/diff/100001/webrtc/audio/test/low_ba... webrtc/audio/test/low_bandwidth_audio_test.cc:218: // EXPECT_CALL(mock_event_log_, LogDelayBasedBweUpdate(_, _)) Remove commented code. https://codereview.webrtc.org/2931873002/diff/100001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.cc (right): https://codereview.webrtc.org/2931873002/diff/100001/webrtc/modules/congestio... webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.cc:63: alr_ended_time_ms_.reset(); What happens if feedback messages are reordered? Why do we need to reset this at all? https://codereview.webrtc.org/2931873002/diff/100001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.h (right): https://codereview.webrtc.org/2931873002/diff/100001/webrtc/modules/congestio... webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.h:32: AcknowledgedBitrateEstimator( I would suggest using one ctor with no arguments and one ctor with the creator function. Also, mark as explicit. https://codereview.webrtc.org/2931873002/diff/100001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/acknowledged_bitrate_estimator_unittest.cc (right): https://codereview.webrtc.org/2931873002/diff/100001/webrtc/modules/congestio... webrtc/modules/congestion_controller/acknowledged_bitrate_estimator_unittest.cc:36: struct AcknowledgedBitrateEstimatorTestStates { Maybe change this to a test fixture and use TEST_F instead? https://codereview.webrtc.org/2931873002/diff/100001/webrtc/modules/congestio... webrtc/modules/congestion_controller/acknowledged_bitrate_estimator_unittest.cc:68: constexpr size_t payload_size = 10; move these to the anonymous namespace
https://codereview.webrtc.org/2931873002/diff/100001/webrtc/audio/test/low_ba... File webrtc/audio/test/low_bandwidth_audio_test.cc (right): https://codereview.webrtc.org/2931873002/diff/100001/webrtc/audio/test/low_ba... webrtc/audio/test/low_bandwidth_audio_test.cc:209: void LogDelayBasedBweUpdate(int32_t bitrate_bps, On 2017/06/14 09:34:05, philipel wrote: > Remove if not used. Oops, this should not be like that. https://codereview.webrtc.org/2931873002/diff/100001/webrtc/audio/test/low_ba... webrtc/audio/test/low_bandwidth_audio_test.cc:218: // EXPECT_CALL(mock_event_log_, LogDelayBasedBweUpdate(_, _)) On 2017/06/14 09:34:05, philipel wrote: > Remove commented code. Oops, this should not be like that. https://codereview.webrtc.org/2931873002/diff/100001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.cc (right): https://codereview.webrtc.org/2931873002/diff/100001/webrtc/modules/congestio... webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.cc:63: alr_ended_time_ms_.reset(); Right, this reset is kind of senseless. https://codereview.webrtc.org/2931873002/diff/100001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.h (right): https://codereview.webrtc.org/2931873002/diff/100001/webrtc/modules/congestio... webrtc/modules/congestion_controller/acknowledged_bitrate_estimator.h:32: AcknowledgedBitrateEstimator( On 2017/06/14 09:34:05, philipel wrote: > I would suggest using one ctor with no arguments and one ctor with the creator > function. Also, mark as explicit. Acknowledged. https://codereview.webrtc.org/2931873002/diff/100001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/acknowledged_bitrate_estimator_unittest.cc (right): https://codereview.webrtc.org/2931873002/diff/100001/webrtc/modules/congestio... webrtc/modules/congestion_controller/acknowledged_bitrate_estimator_unittest.cc:36: struct AcknowledgedBitrateEstimatorTestStates { @kwiberg-webrtc https://codereview.webrtc.org/2931873002/diff/100001/webrtc/modules/congestio... webrtc/modules/congestion_controller/acknowledged_bitrate_estimator_unittest.cc:68: constexpr size_t payload_size = 10; They already are in the anonymous namespace. The name space ends at line 82. I moved them to the top of the anonymous namespace now.
https://codereview.webrtc.org/2931873002/diff/100001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/acknowledged_bitrate_estimator_unittest.cc (right): https://codereview.webrtc.org/2931873002/diff/100001/webrtc/modules/congestio... webrtc/modules/congestion_controller/acknowledged_bitrate_estimator_unittest.cc:36: struct AcknowledgedBitrateEstimatorTestStates { On 2017/06/14 10:57:46, tschumi wrote: > @kwiberg-webrtc Doing it this way instead of using a gtest test fixture with TEST_F() makes the code easier to read, because you can see where the state comes from and how it's initialized. Also, you don't force all the test cases to initialize their state the same way.
lgtm https://codereview.webrtc.org/2931873002/diff/100001/webrtc/modules/congestio... File webrtc/modules/congestion_controller/acknowledged_bitrate_estimator_unittest.cc (right): https://codereview.webrtc.org/2931873002/diff/100001/webrtc/modules/congestio... webrtc/modules/congestion_controller/acknowledged_bitrate_estimator_unittest.cc:36: struct AcknowledgedBitrateEstimatorTestStates { On 2017/06/14 11:49:21, kwiberg-webrtc wrote: > On 2017/06/14 10:57:46, tschumi wrote: > > @kwiberg-webrtc > > Doing it this way instead of using a gtest test fixture with TEST_F() makes the > code easier to read, because you can see where the state comes from and how it's > initialized. Also, you don't force all the test cases to initialize their state > the same way. I'm not sure I understand, when should we use test fixtures then? IMO doing it like this or using a test fixture is equal in terms of complexity, but since test fixtures are wildly used throughout the rest of the codebase I think we should use those instead for the sake of a uniform style. https://codereview.webrtc.org/2931873002/diff/100001/webrtc/modules/congestio... webrtc/modules/congestion_controller/acknowledged_bitrate_estimator_unittest.cc:68: constexpr size_t payload_size = 10; On 2017/06/14 10:57:46, tschumi wrote: > They already are in the anonymous namespace. > The name space ends at line 82. > > I moved them to the top of the anonymous namespace now. oops
lg, but i'm a bit worried about the event log mocking. can we get around that? https://codereview.webrtc.org/2931873002/diff/120001/webrtc/audio/test/low_ba... File webrtc/audio/test/low_bandwidth_audio_test.cc (right): https://codereview.webrtc.org/2931873002/diff/120001/webrtc/audio/test/low_ba... webrtc/audio/test/low_bandwidth_audio_test.cc:181: class NoBandwidthDropAfterDTX : public AudioQualityTest { s/DTX/Dtx https://codereview.webrtc.org/2931873002/diff/120001/webrtc/audio/test/low_ba... webrtc/audio/test/low_bandwidth_audio_test.cc:209: void LogDelayBasedBweUpdate(int32_t bitrate_bps, Can't we use Call::GetStats() instead of mocking the event log?
https://codereview.webrtc.org/2931873002/diff/120001/webrtc/audio/test/low_ba... File webrtc/audio/test/low_bandwidth_audio_test.cc (right): https://codereview.webrtc.org/2931873002/diff/120001/webrtc/audio/test/low_ba... webrtc/audio/test/low_bandwidth_audio_test.cc:181: class NoBandwidthDropAfterDTX : public AudioQualityTest { On 2017/06/16 13:53:27, stefan-webrtc wrote: > s/DTX/Dtx Done. https://codereview.webrtc.org/2931873002/diff/120001/webrtc/audio/test/low_ba... webrtc/audio/test/low_bandwidth_audio_test.cc:209: void LogDelayBasedBweUpdate(int32_t bitrate_bps, Sure. Done.
lgtm % nit https://codereview.webrtc.org/2931873002/diff/140001/webrtc/audio/test/low_ba... File webrtc/audio/test/low_bandwidth_audio_test.h (right): https://codereview.webrtc.org/2931873002/diff/140001/webrtc/audio/test/low_ba... webrtc/audio/test/low_bandwidth_audio_test.h:54: Call* sender_call_; AFAICT this can still be private?
https://codereview.webrtc.org/2931873002/diff/140001/webrtc/audio/test/low_ba... File webrtc/audio/test/low_bandwidth_audio_test.h (right): https://codereview.webrtc.org/2931873002/diff/140001/webrtc/audio/test/low_ba... webrtc/audio/test/low_bandwidth_audio_test.h:54: Call* sender_call_; I think i could move it in to the NoBandwidthDropAfterDtx class. Then it could be private. But this is the AudioQualityTest.
https://codereview.webrtc.org/2931873002/diff/140001/webrtc/audio/test/low_ba... File webrtc/audio/test/low_bandwidth_audio_test.h (right): https://codereview.webrtc.org/2931873002/diff/140001/webrtc/audio/test/low_ba... webrtc/audio/test/low_bandwidth_audio_test.h:54: Call* sender_call_; On 2017/06/19 10:36:03, tschumi wrote: > I think i could move it in to the NoBandwidthDropAfterDtx class. > Then it could be private. But this is the AudioQualityTest. > Ok, maybe worth doing? Or protected is enough?
https://codereview.webrtc.org/2931873002/diff/140001/webrtc/audio/test/low_ba... File webrtc/audio/test/low_bandwidth_audio_test.h (right): https://codereview.webrtc.org/2931873002/diff/140001/webrtc/audio/test/low_ba... webrtc/audio/test/low_bandwidth_audio_test.h:54: Call* sender_call_; Ok moved sender_call_ to NoBandwidthDropAfterDtx.
Regarding low_bandwidth_audio_test.cc: it's meant to be a performance test. It doesn't seem quite right to use it for assertions, but I'm not against it. But it will affect low_bandwidth_audio_test.py which runs the actual performance metrics. Have you ensured that these newly added metrics are useful at all? And also I think that since it expects the input and output audio files' bitrates to match, the results are going to be garbage (or possibly even break) at 48000 Hz, because you're providing only a 16000 Hz file (guessing from filename)
I think you are right that the test don't relay belong to each other since mine is more an integration test, than a quality tests. But on the other hand the test setup is very similar. We may could use a kind of a the same test framework but, then not run the test in the same way. Will take a look if i could separate the tests without being force to copy a lot of code.
Moved Dtx test to webrtc pref test.
The CQ bit was checked by tschumim@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_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/22490) mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...) presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/18376)
The CQ bit was checked by tschumim@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_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...) mac_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/22500) mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
The CQ bit was checked by tschumim@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/25655) mac_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/26750)
The CQ bit was checked by tschumim@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_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/26578)
The CQ bit was checked by tschumim@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_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/10066)
Patchset #16 (id:300001) has been deleted
Patchset #15 (id:280001) has been deleted
Patchset #14 (id:260001) has been deleted
Patchset #13 (id:240001) has been deleted
The CQ bit was checked by tschumim@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 tschumim@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from philipel@webrtc.org, holmer@google.com Link to the patchset: https://codereview.webrtc.org/2931873002/#ps320001 (title: "fix_for_unittest")
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: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/18415)
On 2017/06/21 06:26:57, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > presubmit on master.tryserver.webrtc (JOB_FAILED, > http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/18415) LGTM from right account
The CQ bit was checked by stefan@webrtc.org
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": 320001, "attempt_start_ts": 1498027213977760,
"parent_rev": "bd2220a9c496ef2e8567b68d4be9435a110bdc34", "commit_rev":
"37aa8ba61641962119071646175bfe3bc2bda063"}
Message was sent while issue was closed.
Description was changed from ========== Test and fix for huge bwe drop after alr state. BUG=webrtc:7746 ========== to ========== Test and fix for huge bwe drop after alr state. BUG=webrtc:7746 Review-Url: https://codereview.webrtc.org/2931873002 Cr-Commit-Position: refs/heads/master@{#18692} Committed: https://chromium.googlesource.com/external/webrtc/+/37aa8ba616419621190716461... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:320001) as https://chromium.googlesource.com/external/webrtc/+/37aa8ba616419621190716461...
Message was sent while issue was closed.
Description was changed from ========== Test and fix for huge bwe drop after alr state. BUG=webrtc:7746 Review-Url: https://codereview.webrtc.org/2931873002 Cr-Commit-Position: refs/heads/master@{#18692} Committed: https://chromium.googlesource.com/external/webrtc/+/37aa8ba616419621190716461... ========== to ========== Test and fix for huge bwe drop after alr state. BUG=webrtc:7746 Review-Url: https://codereview.webrtc.org/2931873002 Cr-Commit-Position: refs/heads/master@{#18692} Committed: https://chromium.googlesource.com/external/webrtc/+/37aa8ba616419621190716461... ==========
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:320001) has been created in https://codereview.webrtc.org/2964213002/ by terelius@webrtc.org. The reason for reverting is: Resetting the estimate means that we need to start gathering data from scratch again. The combination of 1) DelayBasedEstimator not reacting to overuse unless there is a valid estimate of the acknowledged bitrate, and 2) AcknowledgedBitrateEstimator needing a significant amount of time/data to obtain an provide an estimate causes poor performance in simulations/tests. It is not clear whether this will affect real networks negatively, but I suggest reverting this to be on the safe side. See also https://bugs.chromium.org/p/webrtc/issues/detail?id=7884. |
