Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2319873002:
Adding ChannelController 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 ChannelController to audio network adaptor.
BUG=webrtc:6303
Committed: https://crrev.com/2e164c6b531a70d4cc0a3294f5a9e3c38fe9d828
Cr-Commit-Position: refs/heads/master@{#14216}
Patch Set 1 #
Total comments: 10
Patch Set 2 : on Henrik's comments #
Total comments: 2
Patch Set 3 : splitting a test and fixing std::min() #Patch Set 4 : further simplying test #
Total comments: 2
Patch Set 5 : final polish #
Total comments: 3
Depends on Patchset: Messages
Total messages: 36 (23 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== reformat Adding ChannelController to audio network adaptor. BUG= ========== to ========== Adding ChannelController to audio network adaptor. BUG=webrtc:6303 ==========
minyue@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, michaelt@webrtc.org
The CQ bit was checked by minyue@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/14055)
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by minyue@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Hi Henrik and Michael, This CL adds a first controller to audio network adaptor: ChannelController, which decides how many channels to encode . PTAL, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_drmemory_light on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_drmemory_light/buil...) win_x64_clang_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_dbg/build...) win_x64_gyp_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_gyp_rel/builds/399)
Nice and clean. Please, see a few comments inline. https://codereview.webrtc.org/2319873002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/channel_controller.cc (right): https://codereview.webrtc.org/2319873002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/channel_controller.cc:26: RTC_DCHECK_GT(intial_channels_to_encode, 0lu); I think these checks should go in the ChannelController constructor. A caller may start out with constructing a valid config, and then modify the parameters before passing it to the ChannelController constructor. https://codereview.webrtc.org/2319873002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/channel_controller.cc:33: : config_(config), channels_to_encode_(config.intial_channels_to_encode) {} You will have to make sure that config.intial_channels_to_encode is either 1 or 2. Otherwise, you will be never enter the switching logic below. https://codereview.webrtc.org/2319873002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/channel_controller_unittest.cc (right): https://codereview.webrtc.org/2319873002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/channel_controller_unittest.cc:24: class ChannelControllerTest : public ::testing::Test { I think you should avoid having a test fixture. The tests are simple enough without them. You can still use some convenience functions (in an unnamed namespace). For instance: std::unique_ptr<ChannelController> CreateChannelController(); void SetUplinkBandwidth(int uplink_bandwidth_bps, Controller::NetworkMetrics* metrics); https://codereview.webrtc.org/2319873002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/channel_controller_unittest.cc:40: ASSERT_TRUE(config.num_channels); You can replace these two lines with one: EXPECT_EQ(rtc::Optional<size_t>(channel), config.num_channels); https://codereview.webrtc.org/2319873002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/channel_controller_unittest.cc:73: CheckChannelDecision(1); Add tests to verify that it switches immediately from 1 to 2 and back when the uplink bw changes across the hysteresis zone without first landing in the dead-zone.
Patchset #2 (id:60001) has been deleted
Thanks for the comment! PTAL again. The compiler error on not finding correct std::min is caused by comparing 2ul with a size_t number on all platforms. This will be fixed in a follow up patch set. https://codereview.webrtc.org/2319873002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/channel_controller.cc (right): https://codereview.webrtc.org/2319873002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/channel_controller.cc:26: RTC_DCHECK_GT(intial_channels_to_encode, 0lu); On 2016/09/08 07:47:44, hlundin-webrtc wrote: > I think these checks should go in the ChannelController constructor. A caller > may start out with constructing a valid config, and then modify the parameters > before passing it to the ChannelController constructor. True, good concern. https://codereview.webrtc.org/2319873002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/channel_controller.cc:33: : config_(config), channels_to_encode_(config.intial_channels_to_encode) {} On 2016/09/08 07:47:44, hlundin-webrtc wrote: > You will have to make sure that config.intial_channels_to_encode is either 1 or > 2. Otherwise, you will be never enter the switching logic below. True, will add DCHECK. https://codereview.webrtc.org/2319873002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/channel_controller_unittest.cc (right): https://codereview.webrtc.org/2319873002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/channel_controller_unittest.cc:24: class ChannelControllerTest : public ::testing::Test { On 2016/09/08 07:47:44, hlundin-webrtc wrote: > I think you should avoid having a test fixture. The tests are simple enough > without them. You can still use some convenience functions (in an unnamed > namespace). For instance: > > std::unique_ptr<ChannelController> CreateChannelController(); > void SetUplinkBandwidth(int uplink_bandwidth_bps, Controller::NetworkMetrics* > metrics); Ok. I removed the fixture. https://codereview.webrtc.org/2319873002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/channel_controller_unittest.cc:40: ASSERT_TRUE(config.num_channels); On 2016/09/08 07:47:44, hlundin-webrtc wrote: > You can replace these two lines with one: > EXPECT_EQ(rtc::Optional<size_t>(channel), config.num_channels); Done. https://codereview.webrtc.org/2319873002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/channel_controller_unittest.cc:73: CheckChannelDecision(1); On 2016/09/08 07:47:45, hlundin-webrtc wrote: > Add tests to verify that it switches immediately from 1 to 2 and back when the > uplink bw changes across the hysteresis zone without first landing in the > dead-zone. Done.
https://codereview.webrtc.org/2319873002/diff/80001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/channel_controller_unittest.cc (right): https://codereview.webrtc.org/2319873002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/channel_controller_unittest.cc:72: TEST(ChannelControllerTest, MaintainChannelNumberOnMediumUplinkBandwidth) { Would split this test to a keep on 1 channel and a keep on 2 channel. Otherwise you retest the switch behavior you have already tested.
LGTM, but please consider Michael's suggestion to split one test. https://codereview.webrtc.org/2319873002/diff/80001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/channel_controller_unittest.cc (right): https://codereview.webrtc.org/2319873002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/channel_controller_unittest.cc:72: TEST(ChannelControllerTest, MaintainChannelNumberOnMediumUplinkBandwidth) { On 2016/09/08 09:40:54, michaelt wrote: > Would split this test to a keep on 1 channel and a keep on 2 channel. > Otherwise you retest the switch behavior you have already tested. +1
The CQ bit was checked by minyue@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Patchset #4 (id:120001) has been deleted
Patchset #4 (id:140001) has been deleted
I split the test as requested in PS3. Then I found a way to further simplify the tests, and therefore added PS4. https://codereview.webrtc.org/2319873002/diff/160001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/channel_controller.cc (right): https://codereview.webrtc.org/2319873002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/channel_controller.cc:30: : config_(config), channels_to_encode_(config_.intial_channels_to_encode) { this is a minor change. it is just a style thingy.
lgtm
Inspired by other CLs, I made some final polish. They are minor changes. https://codereview.webrtc.org/2319873002/diff/160001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/channel_controller.h (right): https://codereview.webrtc.org/2319873002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/channel_controller.h:25: int channel_2_to_1_bandwidth_bps); not a complex struct, explicit dtor not needed. https://codereview.webrtc.org/2319873002/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/channel_controller_unittest.cc (right): https://codereview.webrtc.org/2319873002/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/channel_controller_unittest.cc:26: std::unique_ptr<ChannelController> CreateChannelController(int init_channels) { added this helper function to make test code more concise. https://codereview.webrtc.org/2319873002/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/channel_controller_unittest.cc:34: void CheckDecision(ChannelController* controller, renamed this function since ChannelControllers in CheckChannelControllersDecision is redundant. https://codereview.webrtc.org/2319873002/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/channel_controller_unittest.cc:91: TEST(ChannelControllerTest, CheckBehaviorOnChangingUplinkBandwidth) { added a test to cover sequential changes
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/2319873002/#ps180001 (title: "final polish")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by 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 ChannelController to audio network adaptor. BUG=webrtc:6303 ========== to ========== Adding ChannelController to audio network adaptor. BUG=webrtc:6303 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Adding ChannelController to audio network adaptor. BUG=webrtc:6303 ========== to ========== Adding ChannelController to audio network adaptor. BUG=webrtc:6303 Committed: https://crrev.com/2e164c6b531a70d4cc0a3294f5a9e3c38fe9d828 Cr-Commit-Position: refs/heads/master@{#14216} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/2e164c6b531a70d4cc0a3294f5a9e3c38fe9d828 Cr-Commit-Position: refs/heads/master@{#14216} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
