Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2335163002:
Adding FrameLengthController to audio network adaptor. (Closed)
|
Created:
4 years, 3 months ago by minyue-webrtc Modified:
4 years, 3 months ago CC:
webrtc-reviews_webrtc.org, kwiberg-webrtc, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, peah-webrtc, minyue-webrtc Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdding FrameLengthController to audio network adaptor.
BUG=webrtc:6303
Committed: https://crrev.com/e35d329315af6f57acf63b0a1037cfe5a8232317
Cr-Commit-Position: refs/heads/master@{#14339}
Patch Set 1 #
Total comments: 24
Patch Set 2 : on Henrik's comments and some fixing #
Total comments: 16
Patch Set 3 : on Henrik's further comments. #Patch Set 4 : rebasing #Patch Set 5 : small fixing after rebasing #Patch Set 6 : fixing windows bots #
Depends on Patchset: Messages
Total messages: 30 (14 generated)
Description was changed from ========== Adding FrameLengthController to audio network adaptor. BUG=webrtc:6303webrtc:6303 ========== to ========== Adding FrameLengthController to audio network adaptor. BUG=webrtc:6303 ==========
minyue@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, michaelt@webrtc.org
Hi Henrik and Michael, This is a yet another controller. PTAL, thanks!
See comments inline. https://codereview.webrtc.org/2335163002/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.cc (right): https://codereview.webrtc.org/2335163002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.cc:38: FrameLengthController::FrameLengthChange::FrameLengthChange( Order of definitions in the .cc file should match the order in the .h file. https://codereview.webrtc.org/2335163002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.cc:46: bool FrameLengthController::FrameLengthChange::operator<( Where is this operator used? https://codereview.webrtc.org/2335163002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.cc:53: FrameLengthController::FrameLengthController(const Config& config) So config.encoder_frame_lengths_ms is not applied until SetReceiverFrameLengthRange is called. Why is this? https://codereview.webrtc.org/2335163002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.cc:95: // 4. FEC is not decided or OFF. "or OFF" -> "or is OFF" This avoids the misinterpretation that FEC should be neither decided nor OFF. https://codereview.webrtc.org/2335163002/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.h (right): https://codereview.webrtc.org/2335163002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.h:22: // Determines target frame length based on the Network metrics and the decisions network metrics (small 'n') - or- NetworkMetrics (matching the type name) https://codereview.webrtc.org/2335163002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.h:62: struct FrameLengthChange { struct declarations should go before methods and data members (within each public/protected/private section). https://codereview.webrtc.org/2335163002/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc (right): https://codereview.webrtc.org/2335163002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:14: #include "testing/gtest/include/gtest/gtest.h" Alphabetic order https://codereview.webrtc.org/2335163002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:22: constexpr int kFl20msTo60msBandwidthBps = 22000; There is a general problem with the FrameLengthController class and the tests. The class has some logic that is generalized to more than 2 frame sizes, but parts of the class enforce exactly 2 sizes. This makes it impossible to test the generalizations. I would have liked to see some tests for more than 2 sizes. https://codereview.webrtc.org/2335163002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:49: void CheckFlControllersDecision(FrameLengthController* controller, CheckFlControllerDecision -> CheckDecision The FlController part goes without saying in this file. https://codereview.webrtc.org/2335163002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:108: Maintain60MsToIf20MsNotInReceiverFrameLengthRange) { Name is strange: "Maintain60MsToIf20Ms..." https://codereview.webrtc.org/2335163002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:129: constexpr int kBandwidth = You don't have to introduce these intermediate constexpressions.
Hi, I updated the CL and PTAL again. Thanks! https://codereview.webrtc.org/2335163002/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.cc (right): https://codereview.webrtc.org/2335163002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.cc:38: FrameLengthController::FrameLengthChange::FrameLengthChange( On 2016/09/13 12:34:59, hlundin-webrtc wrote: > Order of definitions in the .cc file should match the order in the .h file. so you mean moving this after SetConstraints(), it feels a bit strange, but I think it is right to go. https://codereview.webrtc.org/2335163002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.cc:46: bool FrameLengthController::FrameLengthChange::operator<( On 2016/09/13 12:34:59, hlundin-webrtc wrote: > Where is this operator used? std::find() needs it https://codereview.webrtc.org/2335163002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.cc:53: FrameLengthController::FrameLengthController(const Config& config) On 2016/09/13 12:34:59, hlundin-webrtc wrote: > So config.encoder_frame_lengths_ms is not applied until > SetReceiverFrameLengthRange is called. Why is this? Because before knowing what receiver can accept, we only use initial_frame_length_ms. https://codereview.webrtc.org/2335163002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.cc:95: // 4. FEC is not decided or OFF. On 2016/09/13 12:34:59, hlundin-webrtc wrote: > "or OFF" -> "or is OFF" > > This avoids the misinterpretation that FEC should be neither decided nor OFF. > Done. https://codereview.webrtc.org/2335163002/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.h (right): https://codereview.webrtc.org/2335163002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.h:22: // Determines target frame length based on the Network metrics and the decisions On 2016/09/13 12:34:59, hlundin-webrtc wrote: > network metrics (small 'n') > - or- > NetworkMetrics (matching the type name) Done. https://codereview.webrtc.org/2335163002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.h:62: struct FrameLengthChange { On 2016/09/13 12:34:59, hlundin-webrtc wrote: > struct declarations should go before methods and data members (within each > public/protected/private section). Done. https://codereview.webrtc.org/2335163002/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc (right): https://codereview.webrtc.org/2335163002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:14: #include "testing/gtest/include/gtest/gtest.h" On 2016/09/13 12:34:59, hlundin-webrtc wrote: > Alphabetic order Done. https://codereview.webrtc.org/2335163002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:22: constexpr int kFl20msTo60msBandwidthBps = 22000; On 2016/09/13 12:34:59, hlundin-webrtc wrote: > There is a general problem with the FrameLengthController class and the tests. > The class has some logic that is generalized to more than 2 frame sizes, but > parts of the class enforce exactly 2 sizes. This makes it impossible to test the > generalizations. I would have liked to see some tests for more than 2 sizes. Thanks. Yes, now added new test cases. https://codereview.webrtc.org/2335163002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:49: void CheckFlControllersDecision(FrameLengthController* controller, On 2016/09/13 12:34:59, hlundin-webrtc wrote: > CheckFlControllerDecision -> CheckDecision > The FlController part goes without saying in this file. Done. https://codereview.webrtc.org/2335163002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:108: Maintain60MsToIf20MsNotInReceiverFrameLengthRange) { On 2016/09/13 12:34:59, hlundin-webrtc wrote: > Name is strange: "Maintain60MsToIf20Ms..." Oh, my bad. https://codereview.webrtc.org/2335163002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:129: constexpr int kBandwidth = On 2016/09/13 12:34:59, hlundin-webrtc wrote: > You don't have to introduce these intermediate constexpressions. I put them in namespace instead and renamed. That can reduce some repetition. https://codereview.webrtc.org/2335163002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.cc (right): https://codereview.webrtc.org/2335163002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.cc:125: bool FrameLengthController::FrameLengthIncreasingDecision( moved the code in MakeDecision to two helper functions. https://codereview.webrtc.org/2335163002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.h (right): https://codereview.webrtc.org/2335163002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.h:27: Config(const std::vector<int>& encoder_frame_lengths_ms, changed the order a bit, insistent with ChannelController https://codereview.webrtc.org/2335163002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc (right): https://codereview.webrtc.org/2335163002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:54: void CheckDecision(FrameLengthController* controller, and extended the method to make the test code more concise
More comments. https://codereview.webrtc.org/2335163002/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.cc (right): https://codereview.webrtc.org/2335163002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.cc:38: FrameLengthController::FrameLengthChange::FrameLengthChange( On 2016/09/15 11:01:13, minyue-webrtc wrote: > On 2016/09/13 12:34:59, hlundin-webrtc wrote: > > Order of definitions in the .cc file should match the order in the .h file. > > so you mean moving this after SetConstraints(), it feels a bit strange, but I > think it is right to go. Acknowledged. https://codereview.webrtc.org/2335163002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.cc:46: bool FrameLengthController::FrameLengthChange::operator<( On 2016/09/15 11:01:13, minyue-webrtc wrote: > On 2016/09/13 12:34:59, hlundin-webrtc wrote: > > Where is this operator used? > > std::find() needs it Acknowledged. https://codereview.webrtc.org/2335163002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.cc (right): https://codereview.webrtc.org/2335163002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.cc:144: if ((metrics.uplink_bandwidth_bps && You can just return the argument to the if-statement. https://codereview.webrtc.org/2335163002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.cc:173: if ((metrics.uplink_bandwidth_bps && Same here. https://codereview.webrtc.org/2335163002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.h (right): https://codereview.webrtc.org/2335163002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.h:53: friend class FrameLengthControllerForTest; I think the proper way to do this is to use FRIEND_TEST or FRIEND_TEST_ALL_PREFIXES from gtest. https://codereview.webrtc.org/2335163002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc (right): https://codereview.webrtc.org/2335163002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:106: Maintain60MsToIf20MsNotInReceiverFrameLengthRange) { Still strange... https://codereview.webrtc.org/2335163002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:213: std::unique_ptr<FrameLengthController> frame_length_controller_; You can make this a plain data member instead of a unique_ptr. https://codereview.webrtc.org/2335163002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:238: // It takes two steps for frame length to go from 120ms to 20ms. Why does it take two steps?
https://codereview.webrtc.org/2335163002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc (right): https://codereview.webrtc.org/2335163002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:238: // It takes two steps for frame length to go from 120ms to 20ms. We use a hysteresis to prevent toggling from one frame length to another. Because of the hysteresis we have to use a state machine. There was no forcing reason to add direct state changes form 20ms to 120ms. Since this would add more complexity and will not bring much benefits.
Hi, I uploaded my replies to the new comments and a new patch set. PTAL again, thanks! https://codereview.webrtc.org/2335163002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.h (right): https://codereview.webrtc.org/2335163002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.h:53: friend class FrameLengthControllerForTest; On 2016/09/15 11:35:17, hlundin-webrtc wrote: > I think the proper way to do this is to use FRIEND_TEST or > FRIEND_TEST_ALL_PREFIXES from gtest. The macros are defined in the following way. It requires the test to be in a fixture and in one test case. We do not use fixture and use the private members in several tests. So I don't think these two macros apply. And given that they are very simple, and do not add advanced features, I think a pure "friend" does not loose any benefits of the micro. ~~~~~ #define FRIEND_TEST(test_case_name, test_name)\ friend class test_case_name##_##test_name##_Test #define FRIEND_TEST_ALL_PREFIXES(test_case_name, test_name) \ FRIEND_TEST(test_case_name, test_name); \ FRIEND_TEST(test_case_name, DISABLED_##test_name); \ FRIEND_TEST(test_case_name, FLAKY_##test_name) https://codereview.webrtc.org/2335163002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc (right): https://codereview.webrtc.org/2335163002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:106: Maintain60MsToIf20MsNotInReceiverFrameLengthRange) { On 2016/09/15 11:35:17, hlundin-webrtc wrote: > Still strange... sorry, forgot this https://codereview.webrtc.org/2335163002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:213: std::unique_ptr<FrameLengthController> frame_length_controller_; On 2016/09/15 11:35:17, hlundin-webrtc wrote: > You can make this a plain data member instead of a unique_ptr. yes, will do https://codereview.webrtc.org/2335163002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:238: // It takes two steps for frame length to go from 120ms to 20ms. On 2016/09/15 12:56:28, michaelt wrote: > We use a hysteresis to prevent toggling from one frame length to another. > Because of the hysteresis we have to use a state machine. > There was no forcing reason to add direct state changes form 20ms to 120ms. > Since this would add more complexity and will not bring much benefits. > To add, it may be good to make changes graceful. I agree that there can be drawbacks. For example, if FEC is on, we may stay at sending 60ms frames with FEC enabled until next MakeDecision is called, which may need network condition to change. But I think we may modify it in the future when we see a need.
lgtm https://codereview.webrtc.org/2335163002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.h (right): https://codereview.webrtc.org/2335163002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.h:53: friend class FrameLengthControllerForTest; On 2016/09/16 08:53:22, minyue-webrtc wrote: > On 2016/09/15 11:35:17, hlundin-webrtc wrote: > > I think the proper way to do this is to use FRIEND_TEST or > > FRIEND_TEST_ALL_PREFIXES from gtest. > > The macros are defined in the following way. It requires the test to be in a > fixture and in one test case. We do not use fixture and use the private members > in several tests. So I don't think these two macros apply. > > And given that they are very simple, and do not add advanced features, I think a > pure "friend" does not loose any benefits of the micro. > > ~~~~~ > #define FRIEND_TEST(test_case_name, test_name)\ > friend class test_case_name##_##test_name##_Test > > #define FRIEND_TEST_ALL_PREFIXES(test_case_name, test_name) \ > FRIEND_TEST(test_case_name, test_name); \ > FRIEND_TEST(test_case_name, DISABLED_##test_name); \ > FRIEND_TEST(test_case_name, FLAKY_##test_name) Acknowledged. https://codereview.webrtc.org/2335163002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc (right): https://codereview.webrtc.org/2335163002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller_unittest.cc:238: // It takes two steps for frame length to go from 120ms to 20ms. On 2016/09/16 08:53:22, minyue-webrtc wrote: > On 2016/09/15 12:56:28, michaelt wrote: > > We use a hysteresis to prevent toggling from one frame length to another. > > Because of the hysteresis we have to use a state machine. > > There was no forcing reason to add direct state changes form 20ms to 120ms. > > Since this would add more complexity and will not bring much benefits. > > > > To add, it may be good to make changes graceful. > > I agree that there can be drawbacks. For example, if FEC is on, we may stay at > sending 60ms frames with FEC enabled until next MakeDecision is called, which > may need network condition to change. But I think we may modify it in the future > when we see a need. Acknowledged.
lgtm
The CQ bit was checked by minyue@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org, michaelt@webrtc.org Link to the patchset: https://codereview.webrtc.org/2335163002/#ps60001 (title: "rebasing")
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 minyue@webrtc.org
The CQ bit was checked by minyue@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org, michaelt@webrtc.org Link to the patchset: https://codereview.webrtc.org/2335163002/#ps80001 (title: "small fixing after rebasing")
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: win_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/14485)
The CQ bit was checked by minyue@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org, michaelt@webrtc.org Link to the patchset: https://codereview.webrtc.org/2335163002/#ps100001 (title: "fixing windows bots")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by minyue@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Adding FrameLengthController to audio network adaptor. BUG=webrtc:6303 ========== to ========== Adding FrameLengthController to audio network adaptor. BUG=webrtc:6303 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Adding FrameLengthController to audio network adaptor. BUG=webrtc:6303 ========== to ========== Adding FrameLengthController to audio network adaptor. BUG=webrtc:6303 Committed: https://crrev.com/e35d329315af6f57acf63b0a1037cfe5a8232317 Cr-Commit-Position: refs/heads/master@{#14339} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/e35d329315af6f57acf63b0a1037cfe5a8232317 Cr-Commit-Position: refs/heads/master@{#14339} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
