Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2364403004:
Creating controller manager from config string in audio network adaptor. (Closed)
|
Created:
4 years, 2 months ago by minyue-webrtc Modified:
4 years, 2 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. |
DescriptionCreating controller manager from config string in audio network adaptor.
BUG=webrtc:6303
Committed: https://crrev.com/a1d9ad0b5819ab3f9745acb7a00992e5e19d933e
Cr-Commit-Position: refs/heads/master@{#14466}
Patch Set 1 #
Total comments: 42
Patch Set 2 : Updates #
Total comments: 12
Patch Set 3 : enum->enum class #
Total comments: 4
Patch Set 4 : int8_t on enum class #Patch Set 5 : rebasing #Depends on Patchset: Messages
Total messages: 49 (21 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Creating controller manager from config string in audio network adaptor. BUG=webrtc:6303 ========== to ========== Creating controller manager from config string in audio network adaptor. BUG=webrtc:6303 ==========
minyue@webrtc.org changed reviewers: + michaelt@webrtc.org
Hi Michael, Would you take a look at this CL? https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.h (right): https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.h:23: Config(int initial_bitrate_bps, int initial_frame_length_ms); fixing an old error (independent on this CL). https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h (right): https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h:59: const Clock* clock); fixing an old error (dependent on this CL)
https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h (right): https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h:51: const std::vector<int>& encoder_frame_lengths_ms, we could pass a array_view here. Since the supported frame lengths from the encoder will be const. https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h (right): https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h:95: std::unique_ptr<SmoothingFilter> packet_loss_smoothed_; Completely unrelated to this diff. But we should make this "const std::unique_ptr<SmoothingFilter>"
minyue@webrtc.org changed reviewers: + henrik.lundin@webrtc.org
Hi Micheal, Thank you for the comments! ~~~~ +Henrik, Hi Henrik, Would you take a look at this CL? Michael has reviewed it and had no big concerns. Thanks! https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h (right): https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h:51: const std::vector<int>& encoder_frame_lengths_ms, On 2016/09/27 15:51:26, michaelt wrote: > we could pass a array_view here. Since the supported frame lengths from the > encoder will be const. good. with do. https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h (right): https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h:95: std::unique_ptr<SmoothingFilter> packet_loss_smoothed_; On 2016/09/27 15:51:26, michaelt wrote: > Completely unrelated to this diff. > But we should make this "const std::unique_ptr<SmoothingFilter>" ok. will consider in a separate CL.
Hi Micheal, Thank you for the comments! ~~~~ +Henrik, Hi Henrik, Would you take a look at this CL? Michael has reviewed it and had no big concerns. Thanks!
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/...
See comments. https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc (right): https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc:45: auto fec_enabling_threshold = config.fec_enabling_threshold(); Why create this copy? https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc:59: FecController::Config::Threshold( What is the type for fec_enabling_threshold? Isn't it an FecController::Config::Threshold? If so, why re-packetize it as yet another one? This pattern repeats several times below. https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc:177: default: What is controller_config.controller_case()? Is it an enum? If so, you can skip the default: case. If you skip the default case, the compiler will enforce that you handle all values in the enum. Otherwise, change RTC_DCHECK(false) to RTC_NOTREACHED(). https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc:199: RTC_DCHECK(false); RTC_NOTREACHED? https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h (right): https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h:51: const std::vector<int>& encoder_frame_lengths_ms, On 2016/09/27 19:43:59, minyue-webrtc wrote: > On 2016/09/27 15:51:26, michaelt wrote: > > we could pass a array_view here. Since the supported frame lengths from the > > encoder will be const. > > good. with do. I wouldn't mind keeping this like this. It keeps us out of liftetime management problems at a low cost. https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc (right): https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc:288: EXPECT_EQ(expected_types.size(), controllers.size()); ASSERT_EQ (You might risk indexing beyond the end of expected_types otherwise.) https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc:323: RTC_DCHECK(false); RTC_NOTREACHED() https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h (right): https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h:64: const Clock* clock; double const, right?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/18497)
thanks for the prompt feedback. You may look at my replies first, new patch set will follow. https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc (right): https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc:45: auto fec_enabling_threshold = config.fec_enabling_threshold(); On 2016/09/27 20:21:15, hlundin-webrtc wrote: > Why create this copy? right. should be auto& https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc:59: FecController::Config::Threshold( On 2016/09/27 20:21:15, hlundin-webrtc wrote: > What is the type for fec_enabling_threshold? Isn't it an > FecController::Config::Threshold? If so, why re-packetize it as yet another one? > This pattern repeats several times below. because this functions are mainly converting from protobuf to xxxController::Config https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc:177: default: On 2016/09/27 20:21:15, hlundin-webrtc wrote: > What is controller_config.controller_case()? Is it an enum? If so, you can skip > the default: case. If you skip the default case, the compiler will enforce that > you handle all values in the enum. > > Otherwise, change RTC_DCHECK(false) to RTC_NOTREACHED(). yes, it is enum generated by protobuf. It has an additional ONEOF_NAME_NOT_SET thing. We can put that explicitly. https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc:199: RTC_DCHECK(false); On 2016/09/27 20:21:15, hlundin-webrtc wrote: > RTC_NOTREACHED? ok, will do. https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc (right): https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc:288: EXPECT_EQ(expected_types.size(), controllers.size()); On 2016/09/27 20:21:15, hlundin-webrtc wrote: > ASSERT_EQ > (You might risk indexing beyond the end of expected_types otherwise.) will do https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc:323: RTC_DCHECK(false); On 2016/09/27 20:21:16, hlundin-webrtc wrote: > RTC_NOTREACHED() can remove default, I think. https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h (right): https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h:64: const Clock* clock; On 2016/09/27 20:21:16, hlundin-webrtc wrote: > double const, right? not sure. since Config as a struct can allow people to overwrite fields before commit to ctor. The controller takes whole Config as const.
minyue@webrtc.org changed reviewers: + kwiberg@webrtc.org
+karl Good to hear your suggestions, too. Thanks!
https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/config.proto (right): https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/config.proto:55: } I think I see why it makes sense to have this as a protobuf instead of a struct: complicated structure, and you'll probably want to add new knobs and stuff in the future. Was that the reason? Since this is part of the API, it should probably be quite heavily commented; this is *the* place where all of these knobs should be explained. Also, make sure that someone with protobuf experience takes a look at it (I'm not such a person). https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc (right): https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc:43: config.has_time_constant_ms()); Perfer to do CHECK(a); CHECK(b); instead of CHECK(a && b); You'll get better error messages that way. https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc:49: fec_enabling_threshold.has_high_bandwidth_packet_loss()); What will happen if you read a protobuf variable that isn't present? If it isn't safe, you should probably change all these DCHECKs to CHECKs. https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc:59: FecController::Config::Threshold( On 2016/09/27 20:50:20, minyue-webrtc wrote: > On 2016/09/27 20:21:15, hlundin-webrtc wrote: > > What is the type for fec_enabling_threshold? Isn't it an > > FecController::Config::Threshold? If so, why re-packetize it as yet another > one? > > This pattern repeats several times below. > > because this functions are mainly converting from protobuf to > xxxController::Config Why do you convert? https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc:178: RTC_DCHECK(false); RTC_NOTREACHED? https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h (right): https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h:49: const std::string& config_string, Is this the protobuf? In that case, isn't there a way to pass it around without serializing it? You want the flexibility and version support of a protobuf, but since you're not sending it outside the process, you shouldn't be encoding it, I think. (Caveat: I know little about protobuf in particular; this is reasoning from how I assume it probably works.) https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h:51: const std::vector<int>& encoder_frame_lengths_ms, On 2016/09/27 20:21:15, hlundin-webrtc wrote: > On 2016/09/27 19:43:59, minyue-webrtc wrote: > > On 2016/09/27 15:51:26, michaelt wrote: > > > we could pass a array_view here. Since the supported frame lengths from the > > > encoder will be const. > > > > good. with do. > > I wouldn't mind keeping this like this. It keeps us out of liftetime management > problems at a low cost. ArrayView is the right choice here; it wouldn't cause any lifetime problems. Both ArrayView and const vector& are things the callee can read from (and copy) but not save a pointer to or steal. The difference is that ArrayView doesn't dictate how the caller must have allocated the array.
Patchset #2 (id:80001) has been deleted
Hi, Here is a new patch set. Please take a look again, thanks! https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/config.proto (right): https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/config.proto:55: } On 2016/09/28 08:59:22, kwiberg-webrtc wrote: > I think I see why it makes sense to have this as a protobuf instead of a struct: > complicated structure, and you'll probably want to add new knobs and stuff in > the future. Was that the reason? Right, we also would like it to be platform and language independent. > > Since this is part of the API, it should probably be quite heavily commented; > this is *the* place where all of these knobs should be explained. Good point, will do. > > Also, make sure that someone with protobuf experience takes a look at it (I'm > not such a person). Good point, will do. https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc (right): https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc:43: config.has_time_constant_ms()); On 2016/09/28 08:59:22, kwiberg-webrtc wrote: > Perfer to do > > CHECK(a); > CHECK(b); > > instead of > > CHECK(a && b); > > You'll get better error messages that way. Done. https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc:59: FecController::Config::Threshold( On 2016/09/28 08:59:22, kwiberg-webrtc wrote: > On 2016/09/27 20:50:20, minyue-webrtc wrote: > > On 2016/09/27 20:21:15, hlundin-webrtc wrote: > > > What is the type for fec_enabling_threshold? Isn't it an > > > FecController::Config::Threshold? If so, why re-packetize it as yet another > > one? > > > This pattern repeats several times below. > > > > because this functions are mainly converting from protobuf to > > xxxController::Config > > Why do you convert? Any better ideas? https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h (right): https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h:49: const std::string& config_string, On 2016/09/28 08:59:22, kwiberg-webrtc wrote: > Is this the protobuf? In that case, isn't there a way to pass it around without > serializing it? You want the flexibility and version support of a protobuf, but > since you're not sending it outside the process, you shouldn't be encoding it, I > think. (Caveat: I know little about protobuf in particular; this is reasoning > from how I assume it probably works.) Serializing makes cross-platform/language possible. That is an advantage we want to achieve here. https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h:51: const std::vector<int>& encoder_frame_lengths_ms, On 2016/09/28 08:59:22, kwiberg-webrtc wrote: > On 2016/09/27 20:21:15, hlundin-webrtc wrote: > > On 2016/09/27 19:43:59, minyue-webrtc wrote: > > > On 2016/09/27 15:51:26, michaelt wrote: > > > > we could pass a array_view here. Since the supported frame lengths from > the > > > > encoder will be const. > > > > > > good. with do. > > > > I wouldn't mind keeping this like this. It keeps us out of liftetime > management > > problems at a low cost. > > ArrayView is the right choice here; it wouldn't cause any lifetime problems. > Both ArrayView and const vector& are things the callee can read from (and copy) > but not save a pointer to or steal. The difference is that ArrayView doesn't > dictate how the caller must have allocated the array. Done. https://codereview.webrtc.org/2364403004/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h (right): https://codereview.webrtc.org/2364403004/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h:43: // Least time since last reordering for a new reordering to be made. added together with the comments in config.proto https://codereview.webrtc.org/2364403004/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.h (right): https://codereview.webrtc.org/2364403004/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/frame_length_controller.h:37: // Uplink packet loss fraction below which frame length can increase. added together with the comments in config.proto.
minyue@webrtc.org changed reviewers: + ivoc@webrtc.org
+ivoc, Ivo, Could you help review the protobuf?
https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc (right): https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc:59: FecController::Config::Threshold( On 2016/09/28 12:47:26, minyue-webrtc wrote: > On 2016/09/28 08:59:22, kwiberg-webrtc wrote: > > On 2016/09/27 20:50:20, minyue-webrtc wrote: > > > On 2016/09/27 20:21:15, hlundin-webrtc wrote: > > > > What is the type for fec_enabling_threshold? Isn't it an > > > > FecController::Config::Threshold? If so, why re-packetize it as yet > another > > > one? > > > > This pattern repeats several times below. > > > > > > because this functions are mainly converting from protobuf to > > > xxxController::Config > > > > Why do you convert? > > Any better ideas? Using protobuf directly, instead of converting it to a struct. There may presumably be good reasons for not doing that (centralized parsing+error checking?, performance?, ...), which is why I was asking. https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h (right): https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h:49: const std::string& config_string, On 2016/09/28 12:47:26, minyue-webrtc wrote: > On 2016/09/28 08:59:22, kwiberg-webrtc wrote: > > Is this the protobuf? In that case, isn't there a way to pass it around > without > > serializing it? You want the flexibility and version support of a protobuf, > but > > since you're not sending it outside the process, you shouldn't be encoding it, > I > > think. (Caveat: I know little about protobuf in particular; this is reasoning > > from how I assume it probably works.) > > Serializing makes cross-platform/language possible. That is an advantage we want > to achieve here. But that argues for serialization at language and process boundaries, right? Not anywhere else? And this isn't such a boundary?
Hi Karl, See my answer to your questions. https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc (right): https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc:59: FecController::Config::Threshold( On 2016/09/28 13:10:24, kwiberg-webrtc wrote: > On 2016/09/28 12:47:26, minyue-webrtc wrote: > > On 2016/09/28 08:59:22, kwiberg-webrtc wrote: > > > On 2016/09/27 20:50:20, minyue-webrtc wrote: > > > > On 2016/09/27 20:21:15, hlundin-webrtc wrote: > > > > > What is the type for fec_enabling_threshold? Isn't it an > > > > > FecController::Config::Threshold? If so, why re-packetize it as yet > > another > > > > one? > > > > > This pattern repeats several times below. > > > > > > > > because this functions are mainly converting from protobuf to > > > > xxxController::Config > > > > > > Why do you convert? > > > > Any better ideas? > > Using protobuf directly, instead of converting it to a struct. There may > presumably be good reasons for not doing that (centralized parsing+error > checking?, performance?, ...), which is why I was asking. We thought about using protobuf for all Controller configs. The main reason for ending it here is because we do not want to use protobuf too extensively, since it may not be supported. When we disable protobuf, we should still be able to build Controllers. https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h (right): https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.h:49: const std::string& config_string, On 2016/09/28 13:10:24, kwiberg-webrtc wrote: > On 2016/09/28 12:47:26, minyue-webrtc wrote: > > On 2016/09/28 08:59:22, kwiberg-webrtc wrote: > > > Is this the protobuf? In that case, isn't there a way to pass it around > > without > > > serializing it? You want the flexibility and version support of a protobuf, > > but > > > since you're not sending it outside the process, you shouldn't be encoding > it, > > I > > > think. (Caveat: I know little about protobuf in particular; this is > reasoning > > > from how I assume it probably works.) > > > > Serializing makes cross-platform/language possible. That is an advantage we > want > > to achieve here. > > But that argues for serialization at language and process boundaries, right? Not > anywhere else? And this isn't such a boundary? This is also because we want to limit the use of protobuf, which may be unavailable. So by serializing it here, we let all middle layers carry a generic type (string).
LGTM with one nit (enum class). https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc (right): https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc:177: default: On 2016/09/27 20:50:20, minyue-webrtc wrote: > On 2016/09/27 20:21:15, hlundin-webrtc wrote: > > What is controller_config.controller_case()? Is it an enum? If so, you can > skip > > the default: case. If you skip the default case, the compiler will enforce > that > > you handle all values in the enum. > > > > Otherwise, change RTC_DCHECK(false) to RTC_NOTREACHED(). > > yes, it is enum generated by protobuf. It has an additional ONEOF_NAME_NOT_SET > thing. We can put that explicitly. Then I think RTC_NOTREACHED() is the correct way. https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h (right): https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h:64: const Clock* clock; On 2016/09/27 20:50:20, minyue-webrtc wrote: > On 2016/09/27 20:21:16, hlundin-webrtc wrote: > > double const, right? > > not sure. since Config as a struct can allow people to overwrite fields before > commit to ctor. The controller takes whole Config as const. Yes, it's a struct. Now I see. https://codereview.webrtc.org/2364403004/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc (right): https://codereview.webrtc.org/2364403004/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc:284: enum ControllerType { FEC, CHANNEL, DTX, FRAME_LENGTH, BIT_RATE }; enum class
lgtm
minyue@webrtc.org changed reviewers: + terelius@webrtc.org
+terelius to give a second look at protobuf since Ivo is on vacation. https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc (right): https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc:49: fec_enabling_threshold.has_high_bandwidth_packet_loss()); On 2016/09/28 08:59:22, kwiberg-webrtc wrote: > What will happen if you read a protobuf variable that isn't present? If it isn't > safe, you should probably change all these DCHECKs to CHECKs. Done. https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc:177: default: On 2016/09/29 09:31:52, hlundin-webrtc wrote: > On 2016/09/27 20:50:20, minyue-webrtc wrote: > > On 2016/09/27 20:21:15, hlundin-webrtc wrote: > > > What is controller_config.controller_case()? Is it an enum? If so, you can > > skip > > > the default: case. If you skip the default case, the compiler will enforce > > that > > > you handle all values in the enum. > > > > > > Otherwise, change RTC_DCHECK(false) to RTC_NOTREACHED(). > > > > yes, it is enum generated by protobuf. It has an additional ONEOF_NAME_NOT_SET > > thing. We can put that explicitly. > > Then I think RTC_NOTREACHED() is the correct way. Done. https://codereview.webrtc.org/2364403004/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager.cc:178: RTC_DCHECK(false); On 2016/09/28 08:59:22, kwiberg-webrtc wrote: > RTC_NOTREACHED? Done. https://codereview.webrtc.org/2364403004/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc (right): https://codereview.webrtc.org/2364403004/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc:284: enum ControllerType { FEC, CHANNEL, DTX, FRAME_LENGTH, BIT_RATE }; On 2016/09/29 09:31:53, hlundin-webrtc wrote: > enum class I figured that it would a bit of pain to use enum class. All elements have to add ControllerType::. For a unittest and anonymously namespaced type, we may not lose too much benefit of enum class vs enum.
https://codereview.webrtc.org/2364403004/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc (right): https://codereview.webrtc.org/2364403004/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc:284: enum ControllerType { FEC, CHANNEL, DTX, FRAME_LENGTH, BIT_RATE }; On 2016/09/29 11:56:26, minyue-webrtc wrote: > On 2016/09/29 09:31:53, hlundin-webrtc wrote: > > enum class > > I figured that it would a bit of pain to use enum class. All elements have to > add ControllerType::. For a unittest and anonymously namespaced type, we may not > lose too much benefit of enum class vs enum. But on the other hand, since you'd have to write "ControllerType::" in just a few places, there's also not much to lose... +1 for using enum class.
https://codereview.webrtc.org/2364403004/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc (right): https://codereview.webrtc.org/2364403004/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc:284: enum ControllerType { FEC, CHANNEL, DTX, FRAME_LENGTH, BIT_RATE }; +1 for using enum class.
yes. changed.
https://codereview.webrtc.org/2364403004/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/config.proto (right): https://codereview.webrtc.org/2364403004/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/config.proto:42: optional float fl_increasing_packet_loss_fraction = 1; nit: I read this name as meaning that the packet loss is increasing, but the comment suggests that it's the frame length that may/should increase. On the other hand, I can't think of a better name. Change if you can think of a better name, otherwise leave as is. https://codereview.webrtc.org/2364403004/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/config.proto:83: // the significance of this controller with respect that network condition. I don't quite understand the goal, but can a controller really be reduced to "perfect decisions at a single point"? Maybe one controller is reliable at all bitrates assuming no packet loss? https://codereview.webrtc.org/2364403004/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/config.proto:90: oneof controller { Can we use oneof? IIRC there was some reason why we couldn't use it in the event log. (If there are no problems with it, we should start using it in the event log.)
Hi Bjorn, Thanks for your comments! See my reply inline. https://codereview.webrtc.org/2364403004/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/config.proto (right): https://codereview.webrtc.org/2364403004/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/config.proto:42: optional float fl_increasing_packet_loss_fraction = 1; On 2016/09/29 13:01:13, terelius wrote: > nit: I read this name as meaning that the packet loss is increasing, but the > comment suggests that it's the frame length that may/should increase. On the > other hand, I can't think of a better name. Change if you can think of a better > name, otherwise leave as is. We have been struggling for naming. We ended up using A_D-ing_B, which stands for "B that D-s A", which is similar to "water boiling point". If you have better ideas, we would be happy to change. https://codereview.webrtc.org/2364403004/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/config.proto:83: // the significance of this controller with respect that network condition. On 2016/09/29 13:01:13, terelius wrote: > I don't quite understand the goal, but can a controller really be reduced to > "perfect decisions at a single point"? Maybe one controller is reliable at all > bitrates assuming no packet loss? That is a good point. If a controller wants to show a higher significance to another controller at (any rates, 0 packet loss), the comparison of the two controllers should be carried out with a different definition of ScoringPoint. So ideally, each comparison of controllers should use its own criterion. Fortunately, this definition is enough for current implementation. If we need to introduce other definition of ScoringPoint, the proto can be modified into message Controller { message ScoringPoint; message ScoringPoint_2; oneof controller { ... } repeated Compare { optional Controller compare_with; oneof scoring_points { ScoringPoint s1; ScoringPoint_2 s2; } } } But this change should only be made when we actually find its need. https://codereview.webrtc.org/2364403004/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/config.proto:90: oneof controller { On 2016/09/29 13:01:13, terelius wrote: > Can we use oneof? IIRC there was some reason why we couldn't use it in the event > log. (If there are no problems with it, we should start using it in the event > log.) I found cases in Chromium before I went for this. See https://cs.chromium.org/chromium/src/blimp/common/proto/blimp_message.proto?q... and https://cs.chromium.org/chromium/src/media/remoting/proto/remoting_rpc_messag... and it is relatively new. Follow the tracker: https://bugs.chromium.org/p/chromium/issues/detail?id=570371 and https://chromium.googlesource.com/chromium/src/+/1fc58a4b0bd5d0a58ce8a353a0c7...
config.proto lgtm
lgtm, but fix that comment if you agree that it's an error. https://codereview.webrtc.org/2364403004/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/config.proto (right): https://codereview.webrtc.org/2364403004/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/config.proto:19: optional float high_bandwidth_packet_loss = 4; Ooohh, very nice with docs. I hadn't realized this was how it worked. So the curve is composed of three line segments: (low_bandwidth_bps, infinity) to (low_bandwidth_bps, high_bandwidth_packet_loss) to (high_bandwidth_bps, low_bandwidth_packet_loss) to (infinity, low_bandwidth_packet_loss)? In that case, the comment is wrong, since it pairs (low, low) and (high, high) instead of (high, low) and (low, high). https://codereview.webrtc.org/2364403004/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc (right): https://codereview.webrtc.org/2364403004/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc:284: enum class ControllerType { FEC, CHANNEL, DTX, FRAME_LENGTH, BIT_RATE }; If you say ": int8_t" after the enum name, your enum will magically occupy just 1 byte instead of 4.
https://codereview.webrtc.org/2364403004/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/config.proto (right): https://codereview.webrtc.org/2364403004/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/config.proto:19: optional float high_bandwidth_packet_loss = 4; On 2016/10/01 01:41:45, kwiberg-webrtc wrote: > Ooohh, very nice with docs. I hadn't realized this was how it worked. > > So the curve is composed of three line segments: (low_bandwidth_bps, infinity) > to (low_bandwidth_bps, high_bandwidth_packet_loss) to (high_bandwidth_bps, > low_bandwidth_packet_loss) to (infinity, low_bandwidth_packet_loss)? In that > case, the comment is wrong, since it pairs (low, low) and (high, high) instead > of (high, low) and (low, high). The comment was not wrong, although the naming may be a bit confusing. Take point A as example, The coordinate of A is (low_bandwidth_bps, low_bandwidth_packet_loss) rather than (low_bandwidth_bps, high_packet_loss) or (low_bandwidth_bps, high_bandwidth_packet_loss) See the difference? The y-coordinate is called "low_bandwidth_packet_loss" because it is the "packet_loss" corresponding to "low_bandwidth" https://codereview.webrtc.org/2364403004/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc (right): https://codereview.webrtc.org/2364403004/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc:284: enum class ControllerType { FEC, CHANNEL, DTX, FRAME_LENGTH, BIT_RATE }; On 2016/10/01 01:41:45, kwiberg-webrtc wrote: > If you say ": int8_t" after the enum name, your enum will magically occupy just > 1 byte instead of 4. Done.
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, kwiberg@webrtc.org, terelius@webrtc.org Link to the patchset: https://codereview.webrtc.org/2364403004/#ps140001 (title: "int8_t on enum class")
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: linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...) 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 minyue@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org, michaelt@webrtc.org, kwiberg@webrtc.org, terelius@webrtc.org Link to the patchset: https://codereview.webrtc.org/2364403004/#ps160001 (title: "rebasing")
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 ========== Creating controller manager from config string in audio network adaptor. BUG=webrtc:6303 ========== to ========== Creating controller manager from config string in audio network adaptor. BUG=webrtc:6303 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Creating controller manager from config string in audio network adaptor. BUG=webrtc:6303 ========== to ========== Creating controller manager from config string in audio network adaptor. BUG=webrtc:6303 Committed: https://crrev.com/a1d9ad0b5819ab3f9745acb7a00992e5e19d933e Cr-Commit-Position: refs/heads/master@{#14466} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/a1d9ad0b5819ab3f9745acb7a00992e5e19d933e Cr-Commit-Position: refs/heads/master@{#14466} |
