Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2362703002:
Adding audio network adaptor to AudioEncoderOpus. (Closed)
|
Created:
4 years, 3 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. |
DescriptionAdding audio network adaptor to AudioEncoderOpus.
BUG=webrtc:6303
Committed: https://crrev.com/41b9c801c2b8153e43485a583ae512e7ad486123
Cr-Commit-Position: refs/heads/master@{#14555}
Patch Set 1 #
Total comments: 5
Patch Set 2 : some updates #
Total comments: 30
Patch Set 3 : updates #
Total comments: 20
Patch Set 4 : rebasing #Patch Set 5 : on Karl's comments #
Total comments: 5
Patch Set 6 : using factory #
Total comments: 9
Patch Set 7 : repolishing #
Total comments: 3
Patch Set 8 : using old struct #
Total comments: 3
Patch Set 9 : fixing #
Total comments: 7
Patch Set 10 : final polish #Patch Set 11 : fixing bots #
Total comments: 2
Patch Set 12 : adding a missing deps #
Total comments: 1
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 58 (18 generated)
Description was changed from ========== aaa build Merge branch 'master' of https://chromium.googlesource.com/external/webrtc into ana_1 half to continue half way adding contents Merge branch 'master' of https://chromium.googlesource.com/external/webrtc into ana_1 adding apis BUG= ========== to ========== Adding audio network adaptor to AudioEncoderOpus. BUG= ==========
minyue@webrtc.org changed reviewers: + michaelt@webrtc.org
hi Michael, This is not truly finished, but I'd like to hear you opinion before I move on. Thanks!
https://codereview.webrtc.org/2362703002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2362703002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:312: if (config.fec_enabled) { Use new function to set FEC and DTX https://codereview.webrtc.org/2362703002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:330: num_channels_to_encode_ = 0; // Opus automatic mode. I think we should create a enum class for num_channels_to_encode
minyue@webrtc.org changed reviewers: + kwiberg@webrtc.org
Hi Karl, Michael has taken a look and had no big concern. I want to let you give further look at it. Thanks! https://codereview.webrtc.org/2362703002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2362703002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:312: if (config.fec_enabled) { On 2016/09/22 12:13:14, michaelt wrote: > Use new function to set FEC and DTX ok, will do https://codereview.webrtc.org/2362703002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:330: num_channels_to_encode_ = 0; // Opus automatic mode. On 2016/09/22 12:13:14, michaelt wrote: > I think we should create a enum class for num_channels_to_encode I think we should not use "0", since audio network adaptor does not understand zero. We can simply initialize num_channels_to_encode_ = config.num_channels
https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/mock/mock_controller_manager.h (left): https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/mock/mock_controller_manager.h:16: This seems unrelated to the rest of the CL. Also, why are you removing the <vector> include when std::vector is still used below? https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_encoder.cc (right): https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_encoder.cc:72: } Wouldn't return !enable; be a better choice for the default implementation? https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_encoder.h:165: // Enable/disable audio network adaptor. Return whether setting is succeeded. Is it supposed to be default enabled or default disabled? Also, verbs should be third person singular present tense (a.k.a., tack on an "s" at the end). And the grammar in the second sentence is a bit off. Maybe Returns true if successful. ? https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_encoder.h:184: int max_frame_length_ms); How is the encoder supposed to behave prior to the first call to this function? Also, "receivers". https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:218: } Hmm. I don't quite understand in what circumstances this function is going to be called, I think. It destroys and recreates the ANA that we got in the constructor, so the state change is much more invasive than I guessed from just seeing the method name and signature. Specifically, who is going to be calling this function, and to what end? https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:376: // num_channels_to_encode)); Remove commented-out code. https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:380: void AudioEncoderOpus::ApplyAudioNetworkAdaptor() { "ApplyAudioNetworkAdaptorConfig"? https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:385: config.enable_dtx && config.num_channels); It's often better to split things like this up into separate DCHECK calls. That way, if they trigger you'll know which field was false. Also, it looks like not all of these are Optional or bool or pointers? When testing if an integer is nonzero, perfer to write x != 0 instead of just x. https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.h (right): https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.h:51: bool audio_network_adaptor_enabled = false; Do you need this? Isn't it enough that the constructor takes an audio network adaptor argument? https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.h:67: AudioEncoderOpus(const CodecInst& codec_inst); This constructor should still be explicit, right? https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc (left): https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:46: TEST_F(AudioEncoderOpusTest, ChangeApplicationMode) { It's great that you remove the stateful test class and use a non-member CreateCodec function everywhere! So great that I won't complain about how this would have fit better in a separate CL. ;-) https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc (right): https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:212: auto config = CreateEncoderRuntimeConfig(); const?
Hi Karl, Thanks for the comments! Before I continue, it would be good to discuss whether to create AudioNetworkAdaptor in AudioEncoderOpus's ctor. You may look at my comments first and I can discuss with you face-to-face later. https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/mock/mock_controller_manager.h (left): https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/mock/mock_controller_manager.h:16: On 2016/09/27 09:35:54, kwiberg-webrtc wrote: > This seems unrelated to the rest of the CL. Also, why are you removing the > <vector> include when std::vector is still used below? They are truly unrelated. just a small cleaning-up. vector is (and has to be) defined in controller_manager.h. Do we normally skip include in such cases. https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_encoder.cc (right): https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_encoder.cc:72: } On 2016/09/27 09:35:54, kwiberg-webrtc wrote: > Wouldn't > > return !enable; > > be a better choice for the default implementation? True. Will change. https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_encoder.h:165: // Enable/disable audio network adaptor. Return whether setting is succeeded. On 2016/09/27 09:35:54, kwiberg-webrtc wrote: > Is it supposed to be default enabled or default disabled? > > Also, verbs should be third person singular present tense (a.k.a., tack on an > "s" at the end). And the grammar in the second sentence is a bit off. Maybe > > Returns true if successful. > > ? As FEC, the default value is given by VoE. A difference between FEC and audio network adaptor is that FEC can be parsed from SDP, while audio network adaptor cannot yet be set through SDP. I am not sure how to handle that. But I think it is good for now to let VoE decide it, through AudioOption, specifically. and will improve wording here. https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_encoder.h:184: int max_frame_length_ms); On 2016/09/27 09:35:54, kwiberg-webrtc wrote: > How is the encoder supposed to behave prior to the first call to this function? > > Also, "receivers". Audio network adaptor is configured so that it only uses initial frame length because receiver frame length range is given. https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:218: } On 2016/09/27 09:35:54, kwiberg-webrtc wrote: > Hmm. I don't quite understand in what circumstances this function is going to be > called, I think. It destroys and recreates the ANA that we got in the > constructor, so the state change is much more invasive than I guessed from just > seeing the method name and signature. > > Specifically, who is going to be calling this function, and to what end? My intention is to make it hook up in similar manner as SetDtx. https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:376: // num_channels_to_encode)); On 2016/09/27 09:35:54, kwiberg-webrtc wrote: > Remove commented-out code. Yes, we can do it now. Since WebRtcOpus_SetForceChannels has landed. https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:380: void AudioEncoderOpus::ApplyAudioNetworkAdaptor() { On 2016/09/27 09:35:55, kwiberg-webrtc wrote: > "ApplyAudioNetworkAdaptorConfig"? I think ApplyAudioNetworkAdaptor may be better since the intention is clear: let AudioNetworkAdaptor to adapt the encoding. ApplyAudioNetworkAdaptorConfig sounds like we need to give a config? https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:385: config.enable_dtx && config.num_channels); On 2016/09/27 09:35:54, kwiberg-webrtc wrote: > It's often better to split things like this up into separate DCHECK calls. That > way, if they trigger you'll know which field was false. > > Also, it looks like not all of these are Optional or bool or pointers? When > testing if an integer is nonzero, perfer to write x != 0 instead of just x. ok. will change in next patch set. https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.h (right): https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.h:51: bool audio_network_adaptor_enabled = false; On 2016/09/27 09:35:55, kwiberg-webrtc wrote: > Do you need this? Isn't it enough that the constructor takes an audio network > adaptor argument? I wanted to do that. But it will need a lot of other changes, e.g., changing APIs in RentACodec. So I decided to treat it as other Opus features like SetDtx. https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.h:67: AudioEncoderOpus(const CodecInst& codec_inst); On 2016/09/27 09:35:55, kwiberg-webrtc wrote: > This constructor should still be explicit, right? oh, right, I tried to add audio network adaptor argument as you suggested, but then realize that there could be too many changes. So I changed it back, but forgot explicit. Sorry. https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc (left): https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:46: TEST_F(AudioEncoderOpusTest, ChangeApplicationMode) { On 2016/09/27 09:35:55, kwiberg-webrtc wrote: > It's great that you remove the stateful test class and use a non-member > CreateCodec function everywhere! So great that I won't complain about how this > would have fit better in a separate CL. ;-) I thought of using a separate CL, the problem is the test name of newly added tests have to differ from the old ones. And I see that the changes are small (e.g., function name like CreateCodec is reused.) https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc (right): https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:212: auto config = CreateEncoderRuntimeConfig(); On 2016/09/27 09:35:55, kwiberg-webrtc wrote: > const? ok.
https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/mock/mock_controller_manager.h (left): https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/mock/mock_controller_manager.h:16: On 2016/09/27 16:02:32, minyue-webrtc wrote: > On 2016/09/27 09:35:54, kwiberg-webrtc wrote: > > This seems unrelated to the rest of the CL. Also, why are you removing the > > <vector> include when std::vector is still used below? > > They are truly unrelated. just a small cleaning-up. > > vector is (and has to be) defined in controller_manager.h. Do we normally skip > include in such cases. The policy is to include all the headers that are supposed to be exporting the stuff that you're using. If you use stuff from both a.h and b.h, and b.h happens to include a.h for some reason, it's technically possible for you to include just b.h, but you're *supposed* to include a.h too. There's one exception: In foo.cc, you always include foo.h first, and if both foo.cc and foo.h use stuff from a.h, you are allowed to only include a.h in foo.h. https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_encoder.h:165: // Enable/disable audio network adaptor. Return whether setting is succeeded. On 2016/09/27 16:02:32, minyue-webrtc wrote: > On 2016/09/27 09:35:54, kwiberg-webrtc wrote: > > Is it supposed to be default enabled or default disabled? > > > > Also, verbs should be third person singular present tense (a.k.a., tack on an > > "s" at the end). And the grammar in the second sentence is a bit off. Maybe > > > > Returns true if successful. > > > > ? > > As FEC, the default value is given by VoE. A difference between FEC and audio > network adaptor is that FEC can be parsed from SDP, while audio network adaptor > cannot yet be set through SDP. I am not sure how to handle that. But I think it > is good for now to let VoE decide it, through AudioOption, specifically. I think it makes more sense for it to be default disabled; someone would then have to enable it for new encoder objects. This will probably become obvious if this method starts requiring an ANA configuration when you want to switch ANA on.
Hi reviewers, Now this is rebased on https://codereview.webrtc.org/2364403004/ which is almost ready to land. Sorry for mixing some unrelated changes from rebasing. I marked them. I would like you to take another look at the CL. Happy reviewing! https://codereview.webrtc.org/2362703002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2362703002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:312: if (config.fec_enabled) { On 2016/09/27 08:30:36, minyue-webrtc wrote: > On 2016/09/22 12:13:14, michaelt wrote: > > Use new function to set FEC and DTX > > ok, will do I gave it a second thought, and feel that it may not read good if we do that. since SetDtx/Fec is supposed to change |config_|, which will be overwritten again on 328. So we may need to differentiate before-|config_| setting of Dtx/Fec and after-|config_|. What may happen is that we do AudioEncoderOpus::SetDtx(bool enable) { if (config_.dtx == enable) return; ... } to remove redundant calling. Then this will fail. https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_encoder.cc (right): https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_encoder.cc:72: } On 2016/09/27 16:02:32, minyue-webrtc wrote: > On 2016/09/27 09:35:54, kwiberg-webrtc wrote: > > Wouldn't > > > > return !enable; > > > > be a better choice for the default implementation? > > True. Will change. since I split it into two functions now. The problem is self-solved. https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_encoder.h:165: // Enable/disable audio network adaptor. Return whether setting is succeeded. On 2016/09/28 08:24:09, kwiberg-webrtc wrote: > On 2016/09/27 16:02:32, minyue-webrtc wrote: > > On 2016/09/27 09:35:54, kwiberg-webrtc wrote: > > > Is it supposed to be default enabled or default disabled? > > > > > > Also, verbs should be third person singular present tense (a.k.a., tack on > an > > > "s" at the end). And the grammar in the second sentence is a bit off. Maybe > > > > > > Returns true if successful. > > > > > > ? > > > > As FEC, the default value is given by VoE. A difference between FEC and audio > > network adaptor is that FEC can be parsed from SDP, while audio network > adaptor > > cannot yet be set through SDP. I am not sure how to handle that. But I think > it > > is good for now to let VoE decide it, through AudioOption, specifically. > > I think it makes more sense for it to be default disabled; someone would then > have to enable it for new encoder objects. This will probably become obvious if > this method starts requiring an ANA configuration when you want to switch ANA > on. Now it is default disabled since |audio_network_adaptor_| is initialized as nullptr. https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.h (right): https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.h:51: bool audio_network_adaptor_enabled = false; On 2016/09/27 16:02:33, minyue-webrtc wrote: > On 2016/09/27 09:35:55, kwiberg-webrtc wrote: > > Do you need this? Isn't it enough that the constructor takes an audio network > > adaptor argument? > > I wanted to do that. But it will need a lot of other changes, e.g., changing > APIs in RentACodec. > > So I decided to treat it as other Opus features like SetDtx. But I removed due to another reason: it becomes redundant to |audio_network_adaptor_|, one which "nullptr" means disabled. https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc (right): https://codereview.webrtc.org/2362703002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:212: auto config = CreateEncoderRuntimeConfig(); On 2016/09/27 16:02:33, minyue-webrtc wrote: > On 2016/09/27 09:35:55, kwiberg-webrtc wrote: > > const? > > ok. Oh, forgot to add. Will do. https://codereview.webrtc.org/2362703002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc (right): https://codereview.webrtc.org/2362703002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:45: void AudioNetworkAdaptorImpl::SetTargetAudioBitrate( due to rebasing https://codereview.webrtc.org/2362703002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:57: void AudioNetworkAdaptorImpl::SetReceiverFrameLengthRange( due to rebasing https://codereview.webrtc.org/2362703002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.h (right): https://codereview.webrtc.org/2362703002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.h:46: void SetTargetAudioBitrate(int target_audio_bitrate_bps) override; due to rebasing https://codereview.webrtc.org/2362703002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl_unittest.cc (right): https://codereview.webrtc.org/2362703002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl_unittest.cc:109: constexpr int kRtt = 100; all changes in this file due to rebasing. https://codereview.webrtc.org/2362703002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h (right): https://codereview.webrtc.org/2362703002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h:48: virtual void SetTargetAudioBitrate(int target_audio_bitrate_bps) = 0; due to rebasing https://codereview.webrtc.org/2362703002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/mock/mock_controller_manager.h (left): https://codereview.webrtc.org/2362703002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/mock/mock_controller_manager.h:14: #include <memory> will add <vector> back
Hi reviewers, Since the landing of a CL that this one is based on, this CL is ready for review. Thanks!
https://codereview.webrtc.org/2362703002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc (left): https://codereview.webrtc.org/2362703002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:65: void AudioNetworkAdaptorImpl::SetReceiverFrameLengthRange( Also because rebase? https://codereview.webrtc.org/2362703002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc (right): https://codereview.webrtc.org/2362703002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:45: void AudioNetworkAdaptorImpl::SetTargetAudioBitrate( On 2016/09/29 15:34:25, minyue-webrtc wrote: > due to rebasing Acknowledged. https://codereview.webrtc.org/2362703002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:57: void AudioNetworkAdaptorImpl::SetReceiverFrameLengthRange( On 2016/09/29 15:34:25, minyue-webrtc wrote: > due to rebasing Acknowledged. https://codereview.webrtc.org/2362703002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.h (right): https://codereview.webrtc.org/2362703002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.h:46: void SetTargetAudioBitrate(int target_audio_bitrate_bps) override; On 2016/09/29 15:34:25, minyue-webrtc wrote: > due to rebasing OK. In general, it's better to do a separate patch set just for the rebase, to avoid this sort of thing entirely. https://codereview.webrtc.org/2362703002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl_unittest.cc (right): https://codereview.webrtc.org/2362703002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl_unittest.cc:109: constexpr int kRtt = 100; On 2016/09/29 15:34:25, minyue-webrtc wrote: > all changes in this file due to rebasing. Acknowledged. https://codereview.webrtc.org/2362703002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/mock/mock_controller_manager.h (left): https://codereview.webrtc.org/2362703002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/mock/mock_controller_manager.h:14: #include <memory> On 2016/09/29 15:34:25, minyue-webrtc wrote: > will add <vector> back Acknowledged. https://codereview.webrtc.org/2362703002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://codereview.webrtc.org/2362703002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_encoder.h:167: // Enables audio network adaptor. Returns whether enabling is succeeded. "Returns true if successful." https://codereview.webrtc.org/2362703002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2362703002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:117: audio_network_adaptor_(std::move(audio_network_adaptor)) { You still take the ANA as an argument in the constructor *and* destroy/create it in the code below. You were going to stop doing one of them, right? https://codereview.webrtc.org/2362703002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:221: arraysize(kSupportedFrameLengths)), 1. Use the constructor that takes only an array argument. That way, you don't have to give the size manually and thus can't get it wrong. 2. That constructor is implicit, so you should be able to pass just the array without mentioning ArrayView at all.
Hi Karl, Thanks for the comments! Some quick replies from me, a patch set will follow. https://codereview.webrtc.org/2362703002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc (left): https://codereview.webrtc.org/2362703002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:65: void AudioNetworkAdaptorImpl::SetReceiverFrameLengthRange( On 2016/10/03 12:48:10, kwiberg-webrtc wrote: > Also because rebase? yes, a landed cl moved the position of this function to let it be consistent with the header file. https://codereview.webrtc.org/2362703002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.h (right): https://codereview.webrtc.org/2362703002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.h:46: void SetTargetAudioBitrate(int target_audio_bitrate_bps) override; On 2016/10/03 12:48:10, kwiberg-webrtc wrote: > On 2016/09/29 15:34:25, minyue-webrtc wrote: > > due to rebasing > > OK. > > In general, it's better to do a separate patch set just for the rebase, to avoid > this sort of thing entirely. Yes, absolutely. I changed the upstream branch and then it became messy. My apologies. https://codereview.webrtc.org/2362703002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://codereview.webrtc.org/2362703002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_encoder.h:167: // Enables audio network adaptor. Returns whether enabling is succeeded. On 2016/10/03 12:48:10, kwiberg-webrtc wrote: > "Returns true if successful." will do https://codereview.webrtc.org/2362703002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2362703002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:117: audio_network_adaptor_(std::move(audio_network_adaptor)) { On 2016/10/03 12:48:10, kwiberg-webrtc wrote: > You still take the ANA as an argument in the constructor *and* destroy/create it > in the code below. You were going to stop doing one of them, right? This ctor is only used for integration test. I think I should mention it as a comment. https://codereview.webrtc.org/2362703002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:221: arraysize(kSupportedFrameLengths)), On 2016/10/03 12:48:10, kwiberg-webrtc wrote: > 1. Use the constructor that takes only an array argument. That way, you don't > have to give the size manually and thus can't get it wrong. > > 2. That constructor is implicit, so you should be able to pass just the array > without mentioning ArrayView at all. sure, will do.
Hi Karl, Here is a new patch set that addressed your comments. PTAL again! Thanks! https://codereview.webrtc.org/2362703002/diff/80001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/mock/mock_controller_manager.h (right): https://codereview.webrtc.org/2362703002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/mock/mock_controller_manager.h:14: #include <vector> add <vector> back as promised earlier
lgtm. You don't need to take my advice on ANA injection if you don't want to. https://codereview.webrtc.org/2362703002/diff/80001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/mock/mock_controller_manager.h (right): https://codereview.webrtc.org/2362703002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/mock/mock_controller_manager.h:14: #include <vector> On 2016/10/03 14:51:37, minyue-webrtc wrote: > add <vector> back as promised earlier Acknowledged. https://codereview.webrtc.org/2362703002/diff/80001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2362703002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:15: #include "webrtc/base/arraysize.h" Do you still need this one? https://codereview.webrtc.org/2362703002/diff/80001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.h (right): https://codereview.webrtc.org/2362703002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.h:65: std::unique_ptr<AudioNetworkAdaptor> audio_network_adaptor = nullptr); I'm still not convinced it's a good idea to inject the ANA like this, since AudioEncoderOpus will sometimes delete and recreate the object and thus lose whatever is special with the ANA you provide here. It would make much more sense to inject an ANA *factory* here, which would then be used whenever we needed an ANA. I was going to suggest using a simple std::function for this, but from the .cc file it looks like AudioEncoderOpus needs to pass a ton of arguments to the ANA factory, so maybe just define an interface for it (with just one function, CreateAna), and have the AudioEncoderOpus constructor look like this: AudioEncoderOpus( const Config& config, std::unique_ptr<AudioNetworkAdaptorFactory> audio_network_adaptor_factory = nullptr); Yes, this gets a little hairy (mainly because of all the arguments that AudioNetworkAdaptorFactory::CreateAna will need to take), so I'll understand if you don't want to take this advice, but what you want to do is allow the caller to change the type of an object used internally in AudioEncoderOpus; this (or something similar) is the general way to do that. Injecting a ready-made ANA would only make sense if AudioEncoderOpus never destroyed or recreated its ANA.
Hi Karl, I followed your suggestion in using factory. I did my best, but I know that you can help me making it better.
I found a nit, but it should not affect reviewing. Please go ahead. https://codereview.webrtc.org/2362703002/diff/80001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc (right): https://codereview.webrtc.org/2362703002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:28: bool enable_audio_network_adaptor) { this should have been removed. I just noticed it. https://codereview.webrtc.org/2362703002/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc (right): https://codereview.webrtc.org/2362703002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:60: // states.encoder->EnableAudioNetworkAdaptor("", nullptr); Oh, for testing, I added //, this should be removed.
https://codereview.webrtc.org/2362703002/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2362703002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:122: std::placeholders::_2)) { (Almost) Never use std::bind when you have C++11; use lambdas instead. Something like this: audio_network_adaptor_creator_( audio_network_adaptor_creator ? std::move(audio_network_adaptor_creator) : [this](const std::string& config_string, const Clock* clock) { return DefaultAudioNetworkAdaptorCreator(config_string, clock); }) std::bind behaves just fine, but lambdas are much easier to read. https://codereview.webrtc.org/2362703002/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.h (right): https://codereview.webrtc.org/2362703002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.h:64: AudioNetworkAdaptorCreator; "using" is much better, primarily because it puts stuff in the right order: using AudioNetworkAdaptorCreator = std::function<std::unique_ptr<AudioNetworkAdaptor>( const std::string&, const Clock*)>; As far as I know, there's no reason to ever use "typedef" in code where you can assume C++11 or later. https://codereview.webrtc.org/2362703002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.h:69: AudioNetworkAdaptorCreator* audio_network_adaptor_creator = nullptr); Pass the std::function by value. Or maybe rvalue reference would be better, to force the caller to use std::move and not suffer the cost of a copy: AudioNetworkAdaptorCreator&& audio_network_adaptor_creator = nullptr); https://codereview.webrtc.org/2362703002/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc (right): https://codereview.webrtc.org/2362703002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:56: }; This creator doesn't work, if I'm not mistaken. The lambda just saves a reference to mock_audio_network_adaptor; when CreateCodec returns, mock_audio_network_adaptor goes out of scope and is returned. Calling the lambda after that is undefined behavior (and will lead to all sorts of badness, because who knows what's stored in that stack location at that time). Luckily, the style guide states firmly that capturing everything by reference with [&] is only allowed if the lambda won't outlive the scope in which it's defined, which is not the case here. You should create the mock ANA in the lambda; that will solve the problem. (Unfortunately, this also makes it impossible to store a pointer to it in |states|. If this is unacceptable, you can create it outside the lambda and move it in. But then you have an ANA creator that can only be called once (subsequent calls will return null).)
Hi Karl, Thanks for the inspiring comments! Now some further polish is made. https://codereview.webrtc.org/2362703002/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2362703002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:122: std::placeholders::_2)) { On 2016/10/04 16:04:06, kwiberg-webrtc wrote: > (Almost) Never use std::bind when you have C++11; use lambdas instead. Something > like this: > > audio_network_adaptor_creator_( > audio_network_adaptor_creator > ? std::move(audio_network_adaptor_creator) > : [this](const std::string& config_string, const Clock* clock) { > return DefaultAudioNetworkAdaptorCreator(config_string, clock); > }) > > std::bind behaves just fine, but lambdas are much easier to read. Done. https://codereview.webrtc.org/2362703002/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.h (right): https://codereview.webrtc.org/2362703002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.h:64: AudioNetworkAdaptorCreator; On 2016/10/04 16:04:06, kwiberg-webrtc wrote: > "using" is much better, primarily because it puts stuff in the right order: > > using AudioNetworkAdaptorCreator = > std::function<std::unique_ptr<AudioNetworkAdaptor>( > const std::string&, const Clock*)>; > > As far as I know, there's no reason to ever use "typedef" in code where you can > assume C++11 or later. Done. https://codereview.webrtc.org/2362703002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.h:69: AudioNetworkAdaptorCreator* audio_network_adaptor_creator = nullptr); On 2016/10/04 16:04:06, kwiberg-webrtc wrote: > Pass the std::function by value. Or maybe rvalue reference would be better, to > force the caller to use std::move and not suffer the cost of a copy: > > AudioNetworkAdaptorCreator&& audio_network_adaptor_creator = nullptr); Done. https://codereview.webrtc.org/2362703002/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc (right): https://codereview.webrtc.org/2362703002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:56: }; On 2016/10/04 16:04:06, kwiberg-webrtc wrote: > This creator doesn't work, if I'm not mistaken. The lambda just saves a > reference to mock_audio_network_adaptor; when CreateCodec returns, > mock_audio_network_adaptor goes out of scope and is returned. Calling the lambda > after that is undefined behavior (and will lead to all sorts of badness, because > who knows what's stored in that stack location at that time). > > Luckily, the style guide states firmly that capturing everything by reference > with [&] is only allowed if the lambda won't outlive the scope in which it's > defined, which is not the case here. > > You should create the mock ANA in the lambda; that will solve the problem. > (Unfortunately, this also makes it impossible to store a pointer to it in > |states|. If this is unacceptable, you can create it outside the lambda and move > it in. But then you have an ANA creator that can only be called once (subsequent > calls will return null).) Line 60 invokes |creator| and mock_audio_network_adaptor is still alive, and I took this advantage (and risk), so it worked. But yes, EnableAudioNetworkAdaptor cannot be called elsewhere. And it reads bad. I can try to solve it.
lgtm https://codereview.webrtc.org/2362703002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc (right): https://codereview.webrtc.org/2362703002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:58: } Much better! However, you could have kept the old structure with a struct and a free Create function; all you'd have to do is put the lines that create the mock ANA in the lambda, and set the lamda's capture list to [&states] so that it could write to states.mock_audio_network_adaptor. Keeping this structure is fine too.
https://codereview.webrtc.org/2362703002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc (right): https://codereview.webrtc.org/2362703002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:58: } On 2016/10/04 17:40:37, kwiberg-webrtc wrote: > Much better! > > However, you could have kept the old structure with a struct and a free Create > function; all you'd have to do is put the lines that create the mock ANA in the > lambda, and set the lamda's capture list to [&states] so that it could write to > states.mock_audio_network_adaptor. > > Keeping this structure is fine too. I prefer keeping it this way. I now can put states.encoder->EnableAudioNetworkAdaptor("", nullptr); in the test code, instead of in the creation of AudioEncoderOpusStates (with the old define). This illustrates the need of EnableAudioNetworkAdaptor(). It may still be possible in the old structure, but can be trickier.
https://codereview.webrtc.org/2362703002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc (right): https://codereview.webrtc.org/2362703002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:58: } On 2016/10/04 20:31:03, minyue-webrtc wrote: > On 2016/10/04 17:40:37, kwiberg-webrtc wrote: > > Much better! > > > > However, you could have kept the old structure with a struct and a free Create > > function; all you'd have to do is put the lines that create the mock ANA in > the > > lambda, and set the lamda's capture list to [&states] so that it could write > to > > states.mock_audio_network_adaptor. > > > > Keeping this structure is fine too. > > I prefer keeping it this way. I now can put > states.encoder->EnableAudioNetworkAdaptor("", nullptr); > in the test code, instead of in the creation of AudioEncoderOpusStates (with the > old define). This illustrates the need of EnableAudioNetworkAdaptor(). > > It may still be possible in the old structure, but can be trickier. You'd still get precisely that with my suggestion, which was to replace the creator declaration in the previous patch set with this: AudioEncoderOpus::AudioNetworkAdaptorCreator creator = [&states]( const std::string&, const Clock*) { std::unique_ptr<MockAudioNetworkAdaptor> adaptor( new NiceMock<MockAudioNetworkAdaptor>()); EXPECT_CALL(*adaptor, Die()); states.mock_audio_network_adaptor = adaptor.get(); return adaptor; }; I find the struct with no methods + free Create function version easier to follow than the struct with methods version, but pick the one you like best. For example the one that requires the least amount of rewriting... :-)
https://codereview.webrtc.org/2362703002/diff/140001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc (right): https://codereview.webrtc.org/2362703002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:49: AudioEncoderOpus::AudioNetworkAdaptorCreator creator = [&states]( I now adopt your suggestion, but I do not fully understand it. states will be copied and dead after being returned, how should |creator| work when being called outside this function?
https://codereview.webrtc.org/2362703002/diff/140001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc (right): https://codereview.webrtc.org/2362703002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:49: AudioEncoderOpus::AudioNetworkAdaptorCreator creator = [&states]( On 2016/10/05 06:08:28, minyue-webrtc wrote: > I now adopt your suggestion, but I do not fully understand it. > > states will be copied and dead after being returned, how should |creator| work > when being called outside this function? Ummm... you're right. I didn't think of that. Your solution avoided that since it used a constructor, but it would still have run into the problem if the user had e.g. copied the struct (which nothing would have prevented). Solvable with another level of indirection. :-) Change AudioEncoderOpusStates::mock_audio_network_adaptor to be of type std::unique_ptr<MockAudioNetworkAdaptor*>, and do something like this: states.mock_audio_network_adaptor = std::unique_ptr<MockAudioNetworkAdaptor*>( static_cast<MockAudioNetworkAdaptor*>(nullptr)); MockAudioNetworkAdaptor** mock_ptr = states.mock_audio_network_adaptor.get(); AudioEncoderOpus::AudioNetworkAdaptorCreator creator = [mock_ptr]( That way, even if you move/copy the states struct, the MockAudioNetworkAdaptor pointer you've given the lambda a pointer to will stay in the same place. For extra correctness bonus points, use shared_ptr instead in the states struct, and let the lambda have a weak_ptr to it. That way nothing bad will happen even if the lambda outlives the states struct.
https://codereview.webrtc.org/2362703002/diff/140001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc (right): https://codereview.webrtc.org/2362703002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:49: AudioEncoderOpus::AudioNetworkAdaptorCreator creator = [&states]( On 2016/10/05 10:43:41, kwiberg-webrtc wrote: > On 2016/10/05 06:08:28, minyue-webrtc wrote: > > I now adopt your suggestion, but I do not fully understand it. > > > > states will be copied and dead after being returned, how should |creator| work > > when being called outside this function? > > Ummm... you're right. I didn't think of that. Your solution avoided that since > it used a constructor, but it would still have run into the problem if the user > had e.g. copied the struct (which nothing would have prevented). > > Solvable with another level of indirection. :-) Change > AudioEncoderOpusStates::mock_audio_network_adaptor to be of type > std::unique_ptr<MockAudioNetworkAdaptor*>, and do something like this: > > states.mock_audio_network_adaptor = std::unique_ptr<MockAudioNetworkAdaptor*>( > static_cast<MockAudioNetworkAdaptor*>(nullptr)); > MockAudioNetworkAdaptor** mock_ptr = states.mock_audio_network_adaptor.get(); > AudioEncoderOpus::AudioNetworkAdaptorCreator creator = [mock_ptr]( > > That way, even if you move/copy the states struct, the MockAudioNetworkAdaptor > pointer you've given the lambda a pointer to will stay in the same place. > > For extra correctness bonus points, use shared_ptr instead in the states struct, > and let the lambda have a weak_ptr to it. That way nothing bad will happen even > if the lambda outlives the states struct. This teases my brain a lot, I will try my best to understand it. However, I am not quite sure that this works. Whatever we place in [capture] is locally defined, and will die, even though the memory, to which the captured pointer refers, do lives longer.
https://codereview.webrtc.org/2362703002/diff/160001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc (right): https://codereview.webrtc.org/2362703002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:46: new MockAudioNetworkAdaptor*(nullptr)); This is different from your suggestion. We have to allocate a MockAudioNetworkAdaptor* https://codereview.webrtc.org/2362703002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:50: AudioEncoderOpus::AudioNetworkAdaptorCreator creator = [mock_ptr]( I see the point now. We don't use reference, we use a copy.
Hi Karl, Does this read good now?
Description was changed from ========== Adding audio network adaptor to AudioEncoderOpus. BUG= ========== to ========== Adding audio network adaptor to AudioEncoderOpus. BUG=webrtc:6303 ==========
lgtm. The comments are optional. https://codereview.webrtc.org/2362703002/diff/160001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc (right): https://codereview.webrtc.org/2362703002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:46: new MockAudioNetworkAdaptor*(nullptr)); On 2016/10/05 14:18:24, minyue-webrtc wrote: > This is different from your suggestion. We have to allocate a > MockAudioNetworkAdaptor* Yes, of course. I tried to to that in my suggestion, but obviously I was confused... You should be able to do this with make_shared if you want, like so: states.mock_audio_network_adaptor = std::make_shared<MockAudioNetworkAdaptor*>(nullptr); https://codereview.webrtc.org/2362703002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:50: AudioEncoderOpus::AudioNetworkAdaptorCreator creator = [mock_ptr]( On 2016/10/05 14:18:24, minyue-webrtc wrote: > I see the point now. We don't use reference, we use a copy. Yes---we copy the weak_ptr. And the thing that it points to is allocated on the heap and by construction safely accessible since we have a weak_ptr to it. https://codereview.webrtc.org/2362703002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:55: *mock_ptr.lock() = adaptor.get(); If all shared_ptrs (just one in this test) that pointed to the thing have previously expired, .lock() will give you a null shared_ptr. This is never supposed to happen in this test, but in order to fail more intelligibly and be easier to read, the code should probably do something more verbose instead, like this: if (auto sp = mock_ptr.lock()) { *sp = adaptor.get(); } else { FAIL() << "ANA creator was called later than expected"; }
https://codereview.webrtc.org/2362703002/diff/160001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc (right): https://codereview.webrtc.org/2362703002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:46: new MockAudioNetworkAdaptor*(nullptr)); On 2016/10/06 08:51:10, kwiberg-webrtc wrote: > On 2016/10/05 14:18:24, minyue-webrtc wrote: > > This is different from your suggestion. We have to allocate a > > MockAudioNetworkAdaptor* > > Yes, of course. I tried to to that in my suggestion, but obviously I was > confused... > > You should be able to do this with make_shared if you want, like so: > > states.mock_audio_network_adaptor = > std::make_shared<MockAudioNetworkAdaptor*>(nullptr); yes, that reads better. https://codereview.webrtc.org/2362703002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:55: *mock_ptr.lock() = adaptor.get(); On 2016/10/06 08:51:10, kwiberg-webrtc wrote: > If all shared_ptrs (just one in this test) that pointed to the thing have > previously expired, .lock() will give you a null shared_ptr. This is never > supposed to happen in this test, but in order to fail more intelligibly and be > easier to read, the code should probably do something more verbose instead, like > this: > > if (auto sp = mock_ptr.lock()) { > *sp = adaptor.get(); > } else { > FAIL() << "ANA creator was called later than expected"; > } FAIL() does not work since it is only allowed in void function(). I'd use RTC_NOTREACHED(). It also reads better since it is more of an error in writing a test than error in using AudioEncoderOpus.
Hi Michael, I will try to commit this CL due to Karl's approval. I'd like you to take a look when you come back from vacation. We can continue to fix errors if you find any later.
The CQ bit was checked by minyue@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2362703002/#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: linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...) win_drmemory_light on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_drmemory_light/buil...) win_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/18804)
The CQ bit was checked by minyue@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2362703002/#ps200001 (title: "adding missing include")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/14931) win_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/18806)
Patchset #11 (id:200001) has been deleted
https://codereview.webrtc.org/2362703002/diff/220001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/opus_interface.h (right): https://codereview.webrtc.org/2362703002/diff/220001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/opus_interface.h:218: int16_t WebRtcOpus_SetForceChannels(OpusEncInst* inst, size_t num_channels); I found that size_t suits better here and can avoid casting when being called from AudioEncoderOpus. Since WebRtcOpus_SetForceChannels's only use case is in this CL, I think it is fine to fix this nit in this CL.
The CQ bit was checked by minyue@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2362703002/#ps220001 (title: "fixing bots")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)
https://codereview.webrtc.org/2362703002/diff/220001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/opus_interface.h (right): https://codereview.webrtc.org/2362703002/diff/220001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/opus_interface.h:218: int16_t WebRtcOpus_SetForceChannels(OpusEncInst* inst, size_t num_channels); On 2016/10/06 11:24:05, minyue-webrtc wrote: > I found that size_t suits better here and can avoid casting when being called > from AudioEncoderOpus. > > Since WebRtcOpus_SetForceChannels's only use case is in this CL, I think it is > fine to fix this nit in this CL. Acknowledged.
https://codereview.webrtc.org/2362703002/diff/240001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/BUILD.gn (right): https://codereview.webrtc.org/2362703002/diff/240001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/BUILD.gn:743: "../../system_wrappers", ANA depends on FileWrapper, which belongs to system_wrappers. This did not show up since missing deps get merged at the final targets. The problem appears on the libfuzzertest, which does not have system_wrappers merged in from other libs. I add webrtc_common to make it complete.
The CQ bit was checked by minyue@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2362703002/#ps240001 (title: "adding a missing deps")
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 audio network adaptor to AudioEncoderOpus. BUG=webrtc:6303 ========== to ========== Adding audio network adaptor to AudioEncoderOpus. BUG=webrtc:6303 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Adding audio network adaptor to AudioEncoderOpus. BUG=webrtc:6303 ========== to ========== Adding audio network adaptor to AudioEncoderOpus. BUG=webrtc:6303 Committed: https://crrev.com/41b9c801c2b8153e43485a583ae512e7ad486123 Cr-Commit-Position: refs/heads/master@{#14555} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/41b9c801c2b8153e43485a583ae512e7ad486123 Cr-Commit-Position: refs/heads/master@{#14555} |
