Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2306083002:
Adding basic implementation of AudioNetworkAdaptor. (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 Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdding basic implementation of AudioNetworkAdaptor.
The basic implementation of AudioNetworkAdaptor include the introduction of
1. Controller
2. ControllerManager
ControllerManager is to hold all needed controllers. It also orders them according to their significance in dealing with current network condition.
Controller provides an interface MakeDecision, which has to be implemented by specific controllers. AudioNetworkAdaptorImpl calls MakeDecision of the controllers in the order decided by ControllerManager to collect EncoderRuntimeConfig.
BUG=webrtc:6303
Committed: https://crrev.com/caa9cb2ceaeeff80def6255eead3d3232efc23ed
Cr-Commit-Position: refs/heads/master@{#14201}
Patch Set 1 #
Total comments: 2
Patch Set 2 : rebasing #Patch Set 3 : removing unnecessary code for abstract class Controller #
Total comments: 11
Patch Set 4 : on Henrik's comments #Patch Set 5 : using -> of Optional #
Total comments: 27
Patch Set 6 : on Karl's comments #
Total comments: 8
Patch Set 7 : on Karl's further comments #
Total comments: 11
Patch Set 8 : more polish #Depends on Patchset: Dependent Patchsets: Messages
Total messages: 50 (23 generated)
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/13924)
Description was changed from ========== adding include adding include adding include formatting ready to submit roughly Revert "cleaning up for commit 1" This reverts commit e85ac75a7bde1be14753ec2d4dc1fbd32d0b66ba. BUG= ========== to ========== Adding basic implementation of AudioNetworkAdaptor. The basic implementation of AudioNetworkAdaptor include the introduction of 1. Controller 2. ControllerManager ControllerManager is to hold all needed controllers. It also orders them according to their significance in dealing with current network condition. Controller provides an interface MakeDecision, which has to be implemented by specific controllers. AudioNetworkAdaptorImpl calls MakeDecision of the controllers in the order decided by ControllerManager to collect EncoderRuntimeConfig. BUG=webrtc:6303 ==========
minyue@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, kwiberg@webrtc.org
Patchset #1 (id:1) 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 Karl, This is the second CL. It gives the basic structure and logic of an implementation of AudioNetworkAdaptor. Your feedbacks are very welcomed. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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) android_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_clang_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_clang_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_drmemory_light on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_x64_clang_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_x64_clang_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_x64_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_x64_gyp_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_x64_gyp_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_x64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
minyue@webrtc.org changed reviewers: + michaelt@webrtc.org
+michaelt@ as reviewer
Short review test https://codereview.webrtc.org/2306083002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc (right): https://codereview.webrtc.org/2306083002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:33: RTC_DCHECK(controller_manager_.get()); .get() is useless here
Patchset #2 (id:40001) has been deleted
I updated the CL by making Controller more abstract class like. Please continue the review. https://codereview.webrtc.org/2306083002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc (right): https://codereview.webrtc.org/2306083002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:33: RTC_DCHECK(controller_manager_.get()); On 2016/09/07 08:15:37, michaelt wrote: > .get() is useless here Fixed in PS3.
Nice! Just a few comments. https://codereview.webrtc.org/2306083002/diff/80001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc (right): https://codereview.webrtc.org/2306083002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:15: #include "webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h" You don't have to repeat #includes that are already present in the corresponding .h file. Delete this and the next. https://codereview.webrtc.org/2306083002/diff/80001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl_unittest.cc (right): https://codereview.webrtc.org/2306083002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl_unittest.cc:35: return (*arg.receiver_frame_length_range).min_frame_length_ms == *arg. is the same as arg-> https://codereview.webrtc.org/2306083002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl_unittest.cc:64: .WillRepeatedly(Return(controllers)); Won't controllers have been gone out of scope when the expected calls happen? https://codereview.webrtc.org/2306083002/diff/80001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/controller.cc (right): https://codereview.webrtc.org/2306083002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller.cc:28: RTC_DCHECK(min_frame_length_ms <= max_frame_length_ms); RTC_DCHECK_LE
Thanks for the comments! I made slight changes, PTAL again. https://codereview.webrtc.org/2306083002/diff/80001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc (right): https://codereview.webrtc.org/2306083002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:15: #include "webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h" On 2016/09/07 14:20:31, hlundin-webrtc wrote: > You don't have to repeat #includes that are already present in the corresponding > .h file. Delete this and the next. Yes, of course, my mistake. https://codereview.webrtc.org/2306083002/diff/80001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl_unittest.cc (right): https://codereview.webrtc.org/2306083002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl_unittest.cc:35: return (*arg.receiver_frame_length_range).min_frame_length_ms == On 2016/09/07 14:20:31, hlundin-webrtc wrote: > *arg. is the same as arg-> Not true, |receiver_frame_length_range| is Optional, and * is to get its value. https://codereview.webrtc.org/2306083002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl_unittest.cc:64: .WillRepeatedly(Return(controllers)); On 2016/09/07 14:20:31, hlundin-webrtc wrote: > Won't controllers have been gone out of scope when the expected calls happen? This is what I asked myself too. I think gmock automatically makes a ReturnRefOfCopyAction, and hence handle it properly. https://cs.chromium.org/chromium/src/testing/gmock/include/gmock/gmock-action... https://codereview.webrtc.org/2306083002/diff/80001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/controller.cc (right): https://codereview.webrtc.org/2306083002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller.cc:28: RTC_DCHECK(min_frame_length_ms <= max_frame_length_ms); On 2016/09/07 14:20:32, hlundin-webrtc wrote: > RTC_DCHECK_LE Right. But since this does not guarantee the correctness of FrameLengthRange when it is used, we'd better move this check there.
https://codereview.webrtc.org/2306083002/diff/80001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl_unittest.cc (right): https://codereview.webrtc.org/2306083002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl_unittest.cc:35: return (*arg.receiver_frame_length_range).min_frame_length_ms == On 2016/09/08 11:06:14, minyue-webrtc wrote: > On 2016/09/07 14:20:31, hlundin-webrtc wrote: > > *arg. is the same as arg-> > > Not true, |receiver_frame_length_range| is Optional, and * is to get its value. Still, (*foo).bar is the same as foo->bar even when foo is an Optional, so you should be able to write arg.receiver_frame_length_range->min_frame_length_ms
On 2016/09/08 11:25:34, kwiberg-webrtc wrote: > https://codereview.webrtc.org/2306083002/diff/80001/webrtc/modules/audio_codi... > File > webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl_unittest.cc > (right): > > https://codereview.webrtc.org/2306083002/diff/80001/webrtc/modules/audio_codi... > webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl_unittest.cc:35: > return (*arg.receiver_frame_length_range).min_frame_length_ms == > On 2016/09/08 11:06:14, minyue-webrtc wrote: > > On 2016/09/07 14:20:31, hlundin-webrtc wrote: > > > *arg. is the same as arg-> > > > > Not true, |receiver_frame_length_range| is Optional, and * is to get its > value. > > Still, (*foo).bar is the same as foo->bar even when foo is an Optional, so you > should be able to write > > arg.receiver_frame_length_range->min_frame_length_ms ok, right. thanks
Please find PS5 that includes the latest change.
lgtm https://codereview.webrtc.org/2306083002/diff/80001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl_unittest.cc (right): https://codereview.webrtc.org/2306083002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl_unittest.cc:64: .WillRepeatedly(Return(controllers)); On 2016/09/08 11:06:14, minyue-webrtc wrote: > On 2016/09/07 14:20:31, hlundin-webrtc wrote: > > Won't controllers have been gone out of scope when the expected calls happen? > > This is what I asked myself too. I think gmock automatically makes a > ReturnRefOfCopyAction, and hence handle it properly. > > https://cs.chromium.org/chromium/src/testing/gmock/include/gmock/gmock-action... Acknowledged. https://codereview.webrtc.org/2306083002/diff/80001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/controller.cc (right): https://codereview.webrtc.org/2306083002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller.cc:28: RTC_DCHECK(min_frame_length_ms <= max_frame_length_ms); On 2016/09/08 11:06:14, minyue-webrtc wrote: > On 2016/09/07 14:20:32, hlundin-webrtc wrote: > > RTC_DCHECK_LE > > Right. But since this does not guarantee the correctness of FrameLengthRange > when it is used, we'd better move this check there. Acknowledged.
Hi Karl and Michael, Do you have other comments?
Yes, more comments. Sorry for the delay! https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc (right): https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:13: #include "webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.h" [insert bleeding-eyes emoji here] https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:31: } Would it be a huge burden on a lot of callers to have just one constructor, and have all callers create and inject a controller manager? https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:53: for (auto& controller : controllers) Do you have to store the controllers in a temporary variable? https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl_unittest.cc (right): https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl_unittest.cc:46: : audio_network_adaptor_(nullptr), mock_controller_manager_(nullptr) {} Initialize these down where you declare them instead---that should be more readable. (Also, you don't need to initialize unique_ptrs, since they're always initialized to null.) https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl_unittest.cc:78: }; It'd be easier to read the tests if you skipped the fixture, and replaced it with a free function that returned a struct with the stuff you need. That way, there are fewer (potential) indirections to keep track of, and the test uses only local variables. https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/controller.cc (right): https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/controller.cc:31: void Controller::SetConstraints(const Constraints& constraints) {} Do you expect typical controllers to not override this method? Unless overriding it is rare, it's simpler to just set it to = 0 and require all subclasses to override it---that way, one doesn't have to look in so many places when trying to see what a Controller subclass does. In other words, providing a default implementation carries a cost in increased complexity, so it should be correspondingly useful. https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h (right): https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h:51: }; I can't tell if this class is supposed to be an interface (in which case it shouldn't have state) or an implementation (in which case it doesn't seem to actually do anything; only the "dependency injection" constructor actually puts anything in controllers_). https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc (right): https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc:44: }; Again, this test fixture should probably be a free function that returns a struct with the stuff it creates. https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc:49: // |controllers_|. |mock_controllers_| ?
Hi Karl, Thanks for the comments! I have updated a new patch set. PTAL again. https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc (right): https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:13: #include "webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.h" On 2016/09/09 11:56:15, kwiberg-webrtc wrote: > [insert bleeding-eyes emoji here] sorry for the long line, but what should I better do? https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:31: } On 2016/09/09 11:56:15, kwiberg-webrtc wrote: > Would it be a huge burden on a lot of callers to have just one constructor, and > have all callers create and inject a controller manager? removed AudioNetworkAdaptorImpl(const Config& config). Also removed config_.controller_manager_config so that one does not need to nest the configs. https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:53: for (auto& controller : controllers) On 2016/09/09 11:56:15, kwiberg-webrtc wrote: > Do you have to store the controllers in a temporary variable? Good point. I think I can make it cleaner. See new patch. https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl_unittest.cc (right): https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl_unittest.cc:46: : audio_network_adaptor_(nullptr), mock_controller_manager_(nullptr) {} On 2016/09/09 11:56:15, kwiberg-webrtc wrote: > Initialize these down where you declare them instead---that should be more > readable. (Also, you don't need to initialize unique_ptrs, since they're always > initialized to null.) Done. https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl_unittest.cc:78: }; On 2016/09/09 11:56:15, kwiberg-webrtc wrote: > It'd be easier to read the tests if you skipped the fixture, and replaced it > with a free function that returned a struct with the stuff you need. That way, > there are fewer (potential) indirections to keep track of, and the test uses > only local variables. Done. https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/controller.cc (right): https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/controller.cc:31: void Controller::SetConstraints(const Constraints& constraints) {} On 2016/09/09 11:56:15, kwiberg-webrtc wrote: > Do you expect typical controllers to not override this method? Unless overriding > it is rare, it's simpler to just set it to = 0 and require all subclasses to > override it---that way, one doesn't have to look in so many places when trying > to see what a Controller subclass does. > > In other words, providing a default implementation carries a cost in increased > complexity, so it should be correspondingly useful. Most controllers will not override this method, like AudioEncoder::SetDtx() :) Do you think the cost of default implementation is acceptable in this case? https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h (right): https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h:51: }; On 2016/09/09 11:56:15, kwiberg-webrtc wrote: > I can't tell if this class is supposed to be an interface (in which case it > shouldn't have state) or an implementation (in which case it doesn't seem to > actually do anything; only the "dependency injection" constructor actually puts > anything in controllers_). Some implementations will be added in later CLs: 1. creating controllers in ControllerManager(const Config& config) 2. Sort controllers in GetSortedControllers(). https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc (right): https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc:44: }; On 2016/09/09 11:56:15, kwiberg-webrtc wrote: > Again, this test fixture should probably be a free function that returns a > struct with the stuff it creates. Done. https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc:49: // |controllers_|. On 2016/09/09 11:56:15, kwiberg-webrtc wrote: > |mock_controllers_| > ? nice pick. thanks,
https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc (right): https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:13: #include "webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.h" On 2016/09/12 11:36:11, minyue-webrtc wrote: > On 2016/09/09 11:56:15, kwiberg-webrtc wrote: > > [insert bleeding-eyes emoji here] > > sorry for the long line, but what should I better do? The only thing you can do is make the file and directory names shorter and/or fewer. You don't have to, if that would have worse effects than a couple of ugly preprocessor lines. https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:31: } On 2016/09/12 11:36:11, minyue-webrtc wrote: > On 2016/09/09 11:56:15, kwiberg-webrtc wrote: > > Would it be a huge burden on a lot of callers to have just one constructor, > and > > have all callers create and inject a controller manager? > > removed AudioNetworkAdaptorImpl(const Config& config). Also removed > config_.controller_manager_config so that one does not need to nest the configs. Excellent! https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:53: for (auto& controller : controllers) On 2016/09/12 11:36:11, minyue-webrtc wrote: > On 2016/09/09 11:56:15, kwiberg-webrtc wrote: > > Do you have to store the controllers in a temporary variable? > > Good point. I think I can make it cleaner. See new patch. Yes, that was exactly what I was thinking. https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/controller.cc (right): https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/controller.cc:31: void Controller::SetConstraints(const Constraints& constraints) {} On 2016/09/12 11:36:11, minyue-webrtc wrote: > On 2016/09/09 11:56:15, kwiberg-webrtc wrote: > > Do you expect typical controllers to not override this method? Unless > overriding > > it is rare, it's simpler to just set it to = 0 and require all subclasses to > > override it---that way, one doesn't have to look in so many places when trying > > to see what a Controller subclass does. > > > > In other words, providing a default implementation carries a cost in increased > > complexity, so it should be correspondingly useful. > > Most controllers will not override this method, like AudioEncoder::SetDtx() :) > Do you think the cost of default implementation is acceptable in this case? Yes, probably. Thanks for checking. https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h (right): https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h:51: }; On 2016/09/12 11:36:11, minyue-webrtc wrote: > On 2016/09/09 11:56:15, kwiberg-webrtc wrote: > > I can't tell if this class is supposed to be an interface (in which case it > > shouldn't have state) or an implementation (in which case it doesn't seem to > > actually do anything; only the "dependency injection" constructor actually > puts > > anything in controllers_). > > Some implementations will be added in later CLs: > 1. creating controllers in ControllerManager(const Config& config) > 2. Sort controllers in GetSortedControllers(). OK. Sounds like an implementation, then. In which case this class should either (1) have no virtual methods, or (2) inherit from a pure virtual base class where all the virtual methods are defined. (These aren't the only two options, but in 95% of cases one of them is the right one.) https://codereview.webrtc.org/2306083002/diff/140001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl_unittest.cc (right): https://codereview.webrtc.org/2306083002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl_unittest.cc:43: ~AudioNetworkAdaptorStates() = default; I think you can omit these two lines. https://codereview.webrtc.org/2306083002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl_unittest.cc:77: CreateAudioNetworkAdaptor(&states); Even simpler: return the struct. (Because it contains unique_ptrs, it can be moved but not copied, but that's all that's needed.) https://codereview.webrtc.org/2306083002/diff/140001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc (right): https://codereview.webrtc.org/2306083002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc:27: ~ControllerManagerStates() = default; Again, I don't think you need to mention the constructor and destructor. https://codereview.webrtc.org/2306083002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc:49: CreateControllerManager(&states); And again, I think you can just return the struct, so that you don't need two statements at each call site.
Hi Karl, Nice comments! I uploaded a new patch and PTAL again. https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h (right): https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h:51: }; On 2016/09/12 13:33:50, kwiberg-webrtc wrote: > On 2016/09/12 11:36:11, minyue-webrtc wrote: > > On 2016/09/09 11:56:15, kwiberg-webrtc wrote: > > > I can't tell if this class is supposed to be an interface (in which case it > > > shouldn't have state) or an implementation (in which case it doesn't seem to > > > actually do anything; only the "dependency injection" constructor actually > > puts > > > anything in controllers_). > > > > Some implementations will be added in later CLs: > > 1. creating controllers in ControllerManager(const Config& config) > > 2. Sort controllers in GetSortedControllers(). > > OK. Sounds like an implementation, then. In which case this class should either > (1) have no virtual methods, or (2) inherit from a pure virtual base class where > all the virtual methods are defined. (These aren't the only two options, but in > 95% of cases one of them is the right one.) The need for virtual methods is due to the need of a mock controller manager. Then I think it fits 2) better, and yes, the code becomes cleaner by introducing an abstract ControllerManager, e.g., no need for Config in the mock class. See the new patch. https://codereview.webrtc.org/2306083002/diff/140001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl_unittest.cc (right): https://codereview.webrtc.org/2306083002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl_unittest.cc:43: ~AudioNetworkAdaptorStates() = default; On 2016/09/12 13:33:50, kwiberg-webrtc wrote: > I think you can omit these two lines. Done. https://codereview.webrtc.org/2306083002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl_unittest.cc:77: CreateAudioNetworkAdaptor(&states); On 2016/09/12 13:33:50, kwiberg-webrtc wrote: > Even simpler: return the struct. (Because it contains unique_ptrs, it can be > moved but not copied, but that's all that's needed.) Done. https://codereview.webrtc.org/2306083002/diff/140001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc (right): https://codereview.webrtc.org/2306083002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc:27: ~ControllerManagerStates() = default; On 2016/09/12 13:33:50, kwiberg-webrtc wrote: > Again, I don't think you need to mention the constructor and destructor. Done. https://codereview.webrtc.org/2306083002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc:49: CreateControllerManager(&states); On 2016/09/12 13:33:50, kwiberg-webrtc wrote: > And again, I think you can just return the struct, so that you don't need two > statements at each call site. Done.
https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc (right): https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:24: new ControllerManager(config_.controller_manager_config)) {} I think you still declare this constructor in the header. https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/mock/mock_controller_manager.h (right): https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/mock/mock_controller_manager.h:27: Nice that this can be removed! That's why it's good to have the interface and the primary implementation in different classes. https://codereview.webrtc.org/2306083002/diff/160001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.h (right): https://codereview.webrtc.org/2306083002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.h:28: }; Soooo..... I spot something you can get rid of. :-) https://codereview.webrtc.org/2306083002/diff/160001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl_unittest.cc (right): https://codereview.webrtc.org/2306083002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl_unittest.cc:77: AudioNetworkAdaptorStates states = CreateAudioNetworkAdaptor(); Excellent! Optionally, shorten this to auto states = CreateAudioNetworkAdaptor(); on the theory that the exact type of states isn't that interesting. https://codereview.webrtc.org/2306083002/diff/160001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h (right): https://codereview.webrtc.org/2306083002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h:31: }; Clean split between interface and implementation. Good! https://codereview.webrtc.org/2306083002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h:38: }; This one doesn't seem to be doing anything important... https://codereview.webrtc.org/2306083002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h:40: explicit ControllerManagerImpl(const Config& config); This constructor seems useless. I assume later CLs will change that? https://codereview.webrtc.org/2306083002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h:44: std::vector<std::unique_ptr<Controller>> controllers); Consider whether the same arguments as for AudioNetworkAdaptorImpl would argue for not having a separate dependency injection constructor here too. The current state of the code definitely says there doesn't need to be two constructors, but you have more changes lined up that aren't visible here.
Hi Karl, Thank you for picking out the error and proposing the use of auto. Regarding some seemingly redundant codes, they are mainly place-holding for future changes (coming soon). See detailed comments inline. https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc (right): https://codereview.webrtc.org/2306083002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:24: new ControllerManager(config_.controller_manager_config)) {} On 2016/09/13 11:11:42, kwiberg-webrtc wrote: > I think you still declare this constructor in the header. oh, my bad, how could I have missed this change. https://codereview.webrtc.org/2306083002/diff/160001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.h (right): https://codereview.webrtc.org/2306083002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.h:28: }; On 2016/09/13 11:11:42, kwiberg-webrtc wrote: > Soooo..... I spot something you can get rid of. :-) This is also a place-holding code. In this way, the ctor, which take Config does not need to change its signature in future. https://codereview.webrtc.org/2306083002/diff/160001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl_unittest.cc (right): https://codereview.webrtc.org/2306083002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl_unittest.cc:77: AudioNetworkAdaptorStates states = CreateAudioNetworkAdaptor(); On 2016/09/13 11:11:42, kwiberg-webrtc wrote: > Excellent! Optionally, shorten this to > > auto states = CreateAudioNetworkAdaptor(); > > on the theory that the exact type of states isn't that interesting. yes, shorter is preferred. Thanks! https://codereview.webrtc.org/2306083002/diff/160001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h (right): https://codereview.webrtc.org/2306083002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h:44: std::vector<std::unique_ptr<Controller>> controllers); On 2016/09/13 11:11:42, kwiberg-webrtc wrote: > Consider whether the same arguments as for AudioNetworkAdaptorImpl would argue > for not having a separate dependency injection constructor here too. The current > state of the code definitely says there doesn't need to be two constructors, but > you have more changes lined up that aren't visible here. Yes, ControllerManagerImpl(const Config& config) is supposed to create all controllers according to Config. This is not at all seen in this CL. Leaving some place-holding code here is to illustrate what it will look like in the future.
lgtm https://codereview.webrtc.org/2306083002/diff/160001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.h (right): https://codereview.webrtc.org/2306083002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.h:28: }; On 2016/09/13 11:48:50, minyue-webrtc wrote: > On 2016/09/13 11:11:42, kwiberg-webrtc wrote: > > Soooo..... I spot something you can get rid of. :-) > > This is also a place-holding code. In this way, the ctor, which take Config does > not need to change its signature in future. Acknowledged. https://codereview.webrtc.org/2306083002/diff/160001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h (right): https://codereview.webrtc.org/2306083002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h:44: std::vector<std::unique_ptr<Controller>> controllers); On 2016/09/13 11:48:50, minyue-webrtc wrote: > On 2016/09/13 11:11:42, kwiberg-webrtc wrote: > > Consider whether the same arguments as for AudioNetworkAdaptorImpl would argue > > for not having a separate dependency injection constructor here too. The > current > > state of the code definitely says there doesn't need to be two constructors, > but > > you have more changes lined up that aren't visible here. > > Yes, ControllerManagerImpl(const Config& config) is supposed to create all > controllers according to Config. This is not at all seen in this CL. Leaving > some place-holding code here is to illustrate what it will look like in the > future. OK. I expect I'll have further comments on this topic when I see the actual code, but that can wait for later.
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 Link to the patchset: https://codereview.webrtc.org/2306083002/#ps180001 (title: "more 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) android_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/...
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/...
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
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 basic implementation of AudioNetworkAdaptor. The basic implementation of AudioNetworkAdaptor include the introduction of 1. Controller 2. ControllerManager ControllerManager is to hold all needed controllers. It also orders them according to their significance in dealing with current network condition. Controller provides an interface MakeDecision, which has to be implemented by specific controllers. AudioNetworkAdaptorImpl calls MakeDecision of the controllers in the order decided by ControllerManager to collect EncoderRuntimeConfig. BUG=webrtc:6303 ========== to ========== Adding basic implementation of AudioNetworkAdaptor. The basic implementation of AudioNetworkAdaptor include the introduction of 1. Controller 2. ControllerManager ControllerManager is to hold all needed controllers. It also orders them according to their significance in dealing with current network condition. Controller provides an interface MakeDecision, which has to be implemented by specific controllers. AudioNetworkAdaptorImpl calls MakeDecision of the controllers in the order decided by ControllerManager to collect EncoderRuntimeConfig. BUG=webrtc:6303 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Adding basic implementation of AudioNetworkAdaptor. The basic implementation of AudioNetworkAdaptor include the introduction of 1. Controller 2. ControllerManager ControllerManager is to hold all needed controllers. It also orders them according to their significance in dealing with current network condition. Controller provides an interface MakeDecision, which has to be implemented by specific controllers. AudioNetworkAdaptorImpl calls MakeDecision of the controllers in the order decided by ControllerManager to collect EncoderRuntimeConfig. BUG=webrtc:6303 ========== to ========== Adding basic implementation of AudioNetworkAdaptor. The basic implementation of AudioNetworkAdaptor include the introduction of 1. Controller 2. ControllerManager ControllerManager is to hold all needed controllers. It also orders them according to their significance in dealing with current network condition. Controller provides an interface MakeDecision, which has to be implemented by specific controllers. AudioNetworkAdaptorImpl calls MakeDecision of the controllers in the order decided by ControllerManager to collect EncoderRuntimeConfig. BUG=webrtc:6303 Committed: https://crrev.com/caa9cb2ceaeeff80def6255eead3d3232efc23ed Cr-Commit-Position: refs/heads/master@{#14201} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/caa9cb2ceaeeff80def6255eead3d3232efc23ed Cr-Commit-Position: refs/heads/master@{#14201} |
