Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2390883004:
Hooking up audio network adaptor to VoE. (Closed)
|
Created:
4 years, 2 months ago by minyue-webrtc Modified:
4 years, 2 months ago CC:
webrtc-reviews_webrtc.org, kwiberg-webrtc, Andrew MacDonald, henrika_webrtc, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, peah-webrtc, the sun Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionHooking up audio network adaptor to VoE.
BUG=webrtc:6303
Committed: https://crrev.com/7e30432b3663ca64aa4776becc536b4a64fc45c9
Cr-Commit-Position: refs/heads/master@{#14611}
Patch Set 1 #
Total comments: 23
Patch Set 2 : on Karl's comments #
Total comments: 4
Patch Set 3 : final polish #Patch Set 4 : rebasing #
Total comments: 5
Patch Set 5 : moving packet loss rate smoothing to AudioEncoderOpus. #
Total comments: 6
Patch Set 6 : updates #Patch Set 7 : go back to old API for some tests. #
Total comments: 2
Patch Set 8 : adding a comment #Depends on Patchset: Messages
Total messages: 37 (10 generated)
Description was changed from ========== Hooking up audio network adaptor to VoE. BUG=webrtc:6303 ========== to ========== Hooking up audio network adaptor to VoE. BUG=webrtc:6303 ==========
minyue@webrtc.org changed reviewers: + kwiberg@webrtc.org, michaelt@webrtc.org
Patchset #1 (id:1) has been deleted
Hi Karl, This is a CL following the changes made in AudioEncoderOpus. See if you like the solution. ~~~~ Michael is on vacation and but is cc-ed. https://codereview.webrtc.org/2390883004/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_encoder.cc (right): https://codereview.webrtc.org/2390883004/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_encoder.cc:81: SetProjectedPacketLossRate(uplink_packet_loss_fraction); Change the default behavior to call SetProjectedPacketLossRate() so that channel.cc needs to call OnReceivedUplinkPacketLossFraction() only. https://codereview.webrtc.org/2390883004/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_encoder.cc:85: SetTargetBitrate(target_audio_bitrate_bps); Change the default behavior to call SetTargetBitrate() so that channel.cc needs to call OnReceivedTargetAudioBitrate() only.
solenberg@webrtc.org changed reviewers: + solenberg@webrtc.org
https://codereview.webrtc.org/2390883004/diff/20001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/2390883004/diff/20001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.h:211: bool EnableAudioNetworkAdaptor(const std::string& config_string); Where are these supposed to be called? Since they are in the VoECodec section, is the intention to expose them through the VoECodec API?
https://codereview.webrtc.org/2390883004/diff/20001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/2390883004/diff/20001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.h:211: bool EnableAudioNetworkAdaptor(const std::string& config_string); On 2016/10/05 09:49:46, the sun wrote: > Where are these supposed to be called? Since they are in the VoECodec section, > is the intention to expose them through the VoECodec API? Yes, they will be added in VoECodec in a follow up CL.
https://codereview.webrtc.org/2390883004/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_encoder.cc (right): https://codereview.webrtc.org/2390883004/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_encoder.cc:81: SetProjectedPacketLossRate(uplink_packet_loss_fraction); On 2016/10/04 11:03:55, minyue-webrtc wrote: > Change the default behavior to call SetProjectedPacketLossRate() so that > channel.cc needs to call OnReceivedUplinkPacketLossFraction() only. Hmmm... would anyone ever have any reason to override this method to make it do something other than this default? If we can't think of any such reason, that may be a sign that there should be just one method... https://codereview.webrtc.org/2390883004/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_encoder.cc:85: SetTargetBitrate(target_audio_bitrate_bps); On 2016/10/04 11:03:55, minyue-webrtc wrote: > Change the default behavior to call SetTargetBitrate() so that > channel.cc needs to call OnReceivedTargetAudioBitrate() only. Same question to ponder as for OnReceivedUplinkPacketLossFraction. https://codereview.webrtc.org/2390883004/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2390883004/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:242: } Better to just duplicate that one line from the default implementation, instead of calling it explicitly from here. (Because it's easier to read, and that's the behavior you want regardless of what the default behavior was. But see also the question of whether this default behavior may be what everyone wants all the time.) https://codereview.webrtc.org/2390883004/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:251: return AudioEncoder::OnReceivedTargetAudioBitrate(target_audio_bitrate_bps); Same. https://codereview.webrtc.org/2390883004/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/test/PacketLossTest.cc (right): https://codereview.webrtc.org/2390883004/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/test/PacketLossTest.cc:115: return success; Excellent. https://codereview.webrtc.org/2390883004/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/test/TestRedFec.cc (right): https://codereview.webrtc.org/2390883004/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/test/TestRedFec.cc:346: (*encoder)->SetProjectedPacketLossRate(25.0f / 100.0f); Or 0.25... (yes, the argument to that function is a double) https://codereview.webrtc.org/2390883004/diff/20001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2390883004/diff/20001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.cc:1294: }); You can probably just do if (*encoder) { (*encoder)->OnReceivedTargetAudioBitrate(bitrate_bps); } That !*encoder means we have no encoder is probably not that hard to guess. https://codereview.webrtc.org/2390883004/diff/20001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/2390883004/diff/20001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.h:211: bool EnableAudioNetworkAdaptor(const std::string& config_string); On 2016/10/05 09:51:34, minyue-webrtc wrote: > On 2016/10/05 09:49:46, the sun wrote: > > Where are these supposed to be called? Since they are in the VoECodec section, > > is the intention to expose them through the VoECodec API? > > Yes, they will be added in VoECodec in a follow up CL. Is this stuff that we genuinely want to change at run time, or should it in principle be set once when things are created?
https://codereview.webrtc.org/2390883004/diff/20001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/2390883004/diff/20001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.h:211: bool EnableAudioNetworkAdaptor(const std::string& config_string); On 2016/10/06 09:46:13, kwiberg-webrtc wrote: > On 2016/10/05 09:51:34, minyue-webrtc wrote: > > On 2016/10/05 09:49:46, the sun wrote: > > > Where are these supposed to be called? Since they are in the VoECodec > section, > > > is the intention to expose them through the VoECodec API? > > > > Yes, they will be added in VoECodec in a follow up CL. > > Is this stuff that we genuinely want to change at run time, or should it in > principle be set once when things are created? FYI: We're discussing how to best do this in https://codereview.webrtc.org/2397573006/
Hi Karl, Please see my comments on AudioEncoder's default behavior. We can also discuss offline. https://codereview.webrtc.org/2390883004/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_encoder.cc (right): https://codereview.webrtc.org/2390883004/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_encoder.cc:81: SetProjectedPacketLossRate(uplink_packet_loss_fraction); On 2016/10/06 09:46:13, kwiberg-webrtc wrote: > On 2016/10/04 11:03:55, minyue-webrtc wrote: > > Change the default behavior to call SetProjectedPacketLossRate() so that > > channel.cc needs to call OnReceivedUplinkPacketLossFraction() only. > > Hmmm... would anyone ever have any reason to override this method to make it do > something other than this default? If we can't think of any such reason, that > may be a sign that there should be just one method... See OnReceivedTargetAudioBitrate, which is easier to explain. https://codereview.webrtc.org/2390883004/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_encoder.cc:85: SetTargetBitrate(target_audio_bitrate_bps); On 2016/10/06 09:46:13, kwiberg-webrtc wrote: > On 2016/10/04 11:03:55, minyue-webrtc wrote: > > Change the default behavior to call SetTargetBitrate() so that > > channel.cc needs to call OnReceivedTargetAudioBitrate() only. > > Same question to ponder as for OnReceivedUplinkPacketLossFraction. SetTargetBitrate has a defined behavior, which is to change encoder's bit rate, while OnReceivedTargetAudioBitrate can invoke AudioNetworkAdaptor. Of course, we may consider putting such logic in this default behavior. But then AudioNetworkAdaptor should be defined in AudioEncoder, while currently it only belongs to AudioEncoderOpus.
https://codereview.webrtc.org/2390883004/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_encoder.cc (right): https://codereview.webrtc.org/2390883004/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_encoder.cc:85: SetTargetBitrate(target_audio_bitrate_bps); On 2016/10/06 10:32:38, minyue-webrtc wrote: > On 2016/10/06 09:46:13, kwiberg-webrtc wrote: > > On 2016/10/04 11:03:55, minyue-webrtc wrote: > > > Change the default behavior to call SetTargetBitrate() so that > > > channel.cc needs to call OnReceivedTargetAudioBitrate() only. > > > > Same question to ponder as for OnReceivedUplinkPacketLossFraction. > > SetTargetBitrate has a defined behavior, which is to change encoder's bit rate, > while OnReceivedTargetAudioBitrate can invoke AudioNetworkAdaptor. > > Of course, we may consider putting such logic in this default behavior. But then > AudioNetworkAdaptor should be defined in AudioEncoder, while currently it only > belongs to AudioEncoderOpus. No, SetTargetBitrate is defined like this: // Tells the encoder what average bitrate we'd like it to produce. The // encoder is free to adjust or disregard the given bitrate (the default // implementation does the latter). virtual void SetTargetBitrate(int target_bps); How is this different from OnReceivedTargetAudioBitrate: // Provides target audio bitrate to this encoder to allow it to adapt. virtual void OnReceivedTargetAudioBitrate(int target_audio_bitrate_bps); The way I read it, both of them say, "Here's a target bitrate that I'd like you to meet if you feel like it." Whether the encoder does so by making use of an internal ANA object or not is not something the interface should be concerned with. The packet loss methods are also similar in exactly this fashion. Sorry for not catching this in the review of the CL that adds these methods.
On 2016/10/06 10:57:11, kwiberg-webrtc wrote: > https://codereview.webrtc.org/2390883004/diff/20001/webrtc/modules/audio_codi... > File webrtc/modules/audio_coding/codecs/audio_encoder.cc (right): > > https://codereview.webrtc.org/2390883004/diff/20001/webrtc/modules/audio_codi... > webrtc/modules/audio_coding/codecs/audio_encoder.cc:85: > SetTargetBitrate(target_audio_bitrate_bps); > On 2016/10/06 10:32:38, minyue-webrtc wrote: > > On 2016/10/06 09:46:13, kwiberg-webrtc wrote: > > > On 2016/10/04 11:03:55, minyue-webrtc wrote: > > > > Change the default behavior to call SetTargetBitrate() so that > > > > channel.cc needs to call OnReceivedTargetAudioBitrate() only. > > > > > > Same question to ponder as for OnReceivedUplinkPacketLossFraction. > > > > SetTargetBitrate has a defined behavior, which is to change encoder's bit > rate, > > while OnReceivedTargetAudioBitrate can invoke AudioNetworkAdaptor. > > > > Of course, we may consider putting such logic in this default behavior. But > then > > AudioNetworkAdaptor should be defined in AudioEncoder, while currently it only > > belongs to AudioEncoderOpus. > > No, SetTargetBitrate is defined like this: > > // Tells the encoder what average bitrate we'd like it to produce. The > // encoder is free to adjust or disregard the given bitrate (the default > // implementation does the latter). > virtual void SetTargetBitrate(int target_bps); > > How is this different from OnReceivedTargetAudioBitrate: > > // Provides target audio bitrate to this encoder to allow it to adapt. > virtual void OnReceivedTargetAudioBitrate(int target_audio_bitrate_bps); > > The way I read it, both of them say, "Here's a target bitrate that I'd like you > to meet if you feel like it." Whether the encoder does so by making use of an > internal ANA object or not is not something the interface should be concerned > with. > > The packet loss methods are also similar in exactly this fashion. > > Sorry for not catching this in the review of the CL that adds these methods. Ok, per offline discussion, we will make a CL (a follow-up of this CL) to remove SetTargetBitrate and SetProjectedPacketLossRate
Hi Karl, I have addressed your comments in a new PatchSet, and created a follow-up CL to address the redundant API https://codereview.webrtc.org/2411613002/. PTAL again, thanks! https://codereview.webrtc.org/2390883004/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_encoder.cc (right): https://codereview.webrtc.org/2390883004/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_encoder.cc:85: SetTargetBitrate(target_audio_bitrate_bps); On 2016/10/06 10:57:11, kwiberg-webrtc wrote: > On 2016/10/06 10:32:38, minyue-webrtc wrote: > > On 2016/10/06 09:46:13, kwiberg-webrtc wrote: > > > On 2016/10/04 11:03:55, minyue-webrtc wrote: > > > > Change the default behavior to call SetTargetBitrate() so that > > > > channel.cc needs to call OnReceivedTargetAudioBitrate() only. > > > > > > Same question to ponder as for OnReceivedUplinkPacketLossFraction. > > > > SetTargetBitrate has a defined behavior, which is to change encoder's bit > rate, > > while OnReceivedTargetAudioBitrate can invoke AudioNetworkAdaptor. > > > > Of course, we may consider putting such logic in this default behavior. But > then > > AudioNetworkAdaptor should be defined in AudioEncoder, while currently it only > > belongs to AudioEncoderOpus. > > No, SetTargetBitrate is defined like this: > > // Tells the encoder what average bitrate we'd like it to produce. The > // encoder is free to adjust or disregard the given bitrate (the default > // implementation does the latter). > virtual void SetTargetBitrate(int target_bps); > > How is this different from OnReceivedTargetAudioBitrate: > > // Provides target audio bitrate to this encoder to allow it to adapt. > virtual void OnReceivedTargetAudioBitrate(int target_audio_bitrate_bps); > > The way I read it, both of them say, "Here's a target bitrate that I'd like you > to meet if you feel like it." Whether the encoder does so by making use of an > internal ANA object or not is not something the interface should be concerned > with. > > The packet loss methods are also similar in exactly this fashion. > > Sorry for not catching this in the review of the CL that adds these methods. Per offline discussion, we may replace SetTargetBitrate and SetProjectedPacketLossRate with OnReceivedTargetAudioBitrate and OnReceivedUplinkPacketLossFraction. This will invoke a PSA and it will be carried out in a follow-up CL: https://codereview.webrtc.org/2411613002/ https://codereview.webrtc.org/2390883004/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2390883004/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:242: } On 2016/10/06 09:46:13, kwiberg-webrtc wrote: > Better to just duplicate that one line from the default implementation, instead > of calling it explicitly from here. (Because it's easier to read, and that's the > behavior you want regardless of what the default behavior was. But see also the > question of whether this default behavior may be what everyone wants all the > time.) Done. https://codereview.webrtc.org/2390883004/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:251: return AudioEncoder::OnReceivedTargetAudioBitrate(target_audio_bitrate_bps); On 2016/10/06 09:46:13, kwiberg-webrtc wrote: > Same. Done. https://codereview.webrtc.org/2390883004/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/test/TestRedFec.cc (right): https://codereview.webrtc.org/2390883004/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/test/TestRedFec.cc:346: (*encoder)->SetProjectedPacketLossRate(25.0f / 100.0f); On 2016/10/06 09:46:13, kwiberg-webrtc wrote: > Or 0.25... (yes, the argument to that function is a double) Done. https://codereview.webrtc.org/2390883004/diff/20001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2390883004/diff/20001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.cc:1294: }); On 2016/10/06 09:46:13, kwiberg-webrtc wrote: > You can probably just do > > if (*encoder) { > (*encoder)->OnReceivedTargetAudioBitrate(bitrate_bps); > } > > That !*encoder means we have no encoder is probably not that hard to guess. Done. https://codereview.webrtc.org/2390883004/diff/20001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/2390883004/diff/20001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.h:211: bool EnableAudioNetworkAdaptor(const std::string& config_string); On 2016/10/06 09:46:13, kwiberg-webrtc wrote: > On 2016/10/05 09:51:34, minyue-webrtc wrote: > > On 2016/10/05 09:49:46, the sun wrote: > > > Where are these supposed to be called? Since they are in the VoECodec > section, > > > is the intention to expose them through the VoECodec API? > > > > Yes, they will be added in VoECodec in a follow up CL. > > Is this stuff that we genuinely want to change at run time, or should it in > principle be set once when things are created? Creation time is good. But it may not be set through SDP. So I now leave it as a separate API.
lgtm, but see comments https://codereview.webrtc.org/2390883004/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2390883004/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:118: ? audio_network_adaptor_creator You should probably std::move here. Why else require this argument to be an rvalue reference? https://codereview.webrtc.org/2390883004/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/test/TestRedFec.cc (right): https://codereview.webrtc.org/2390883004/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/test/TestRedFec.cc:346: (*encoder)->SetProjectedPacketLossRate(25.0 / 100.0); Why is "25.0 / 100.0" better than just "0.25"?
https://codereview.webrtc.org/2390883004/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2390883004/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:118: ? audio_network_adaptor_creator On 2016/10/11 10:18:42, kwiberg-webrtc wrote: > You should probably std::move here. Why else require this argument to be an > rvalue reference? Done. https://codereview.webrtc.org/2390883004/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/test/TestRedFec.cc (right): https://codereview.webrtc.org/2390883004/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/test/TestRedFec.cc:346: (*encoder)->SetProjectedPacketLossRate(25.0 / 100.0); On 2016/10/11 10:18:42, kwiberg-webrtc wrote: > Why is "25.0 / 100.0" better than just "0.25"? sure. of course.
https://codereview.webrtc.org/2390883004/diff/80001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2390883004/diff/80001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.cc:1288: audio_coding_->ModifyEncoder([&](std::unique_ptr<AudioEncoder>* encoder) { Have you done a real call with this ? ModifyEncoder will destroy the encoder_factory. So we have to be sure that all actions with the encoder_factory are done before we call ModifyEncoder. https://codereview.webrtc.org/2390883004/diff/80001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.cc:1297: uint8_t average_fraction_loss = network_predictor_->GetLossRate(); shouldn't we remove this double smoothing for ANA ?
Thanks for the comments! https://codereview.webrtc.org/2390883004/diff/80001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2390883004/diff/80001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.cc:1288: audio_coding_->ModifyEncoder([&](std::unique_ptr<AudioEncoder>* encoder) { On 2016/10/11 11:38:40, michaelt wrote: > Have you done a real call with this ? > ModifyEncoder will destroy the encoder_factory. So we have to be sure that all > actions with the encoder_factory are done before we call ModifyEncoder. I think this should work. But thank you for bring it up. I will try it. https://codereview.webrtc.org/2390883004/diff/80001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.cc:1297: uint8_t average_fraction_loss = network_predictor_->GetLossRate(); On 2016/10/11 11:38:40, michaelt wrote: > shouldn't we remove this double smoothing for ANA ? Good question. Now the forking between using old SetPacketLossRate or AudioNetworkAdaptor is done within OnReceivedUplinkPacketLossFraction, which makes much better sense. I think OnReceivedUplinkPacketLossFraction is supposed to receive non-filtered packet loss fractions. Any filtering is encoder's responsibility. I will add another patch set to move |network_predictor_| to AudioEncoderOpus. WDYT?
lgtm https://codereview.webrtc.org/2390883004/diff/80001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2390883004/diff/80001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.cc:1297: uint8_t average_fraction_loss = network_predictor_->GetLossRate(); Moving |network_predictor_| to the encoder and smooth PL in the case we don't use ANA seams OK for the moment. We me move in future to make ANA not optional. In this case we could remove |network_predictor_| and let ANA care of the smoothing.
Hi Michael, I have moved the packet loss rate smoother. PTAL. ~~~~ Hi Karl, It would be nice if you also take a took.
https://codereview.webrtc.org/2390883004/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2390883004/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:91: ~PacketLossFractionSmoother() = default; I don't think this line does anything. This is what you'd get by default if you didn't mention the destructor at all. https://codereview.webrtc.org/2390883004/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:112: std::unique_ptr<rtc::ExpFilter> smoother_; Do you need to point to the ExpFilter? Can't you embed it directly? https://codereview.webrtc.org/2390883004/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.h (right): https://codereview.webrtc.org/2390883004/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.h:27: } Style guide says "Don't use unnamed namespaces in .h files." (Because the compiler will invent a unique name for the anonymous namespace when it encounters it, so if the .h file is included in more than one compilation unit, the names will be different.) If I'm not overlooking something, it'd work well to forward-declare this class in the private section of AudioEncoderOpus instead.
Thanks for the comments! now updated. https://codereview.webrtc.org/2390883004/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2390883004/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:91: ~PacketLossFractionSmoother() = default; On 2016/10/11 14:56:19, kwiberg-webrtc wrote: > I don't think this line does anything. This is what you'd get by default if you > didn't mention the destructor at all. Done. https://codereview.webrtc.org/2390883004/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:112: std::unique_ptr<rtc::ExpFilter> smoother_; On 2016/10/11 14:56:19, kwiberg-webrtc wrote: > Do you need to point to the ExpFilter? Can't you embed it directly? no. I simply copied from the old implementation. But of course, good to get rid of pointer now. https://codereview.webrtc.org/2390883004/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.h (right): https://codereview.webrtc.org/2390883004/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.h:27: } On 2016/10/11 14:56:19, kwiberg-webrtc wrote: > Style guide says "Don't use unnamed namespaces in .h files." (Because the > compiler will invent a unique name for the anonymous namespace when it > encounters it, so if the .h file is included in more than one compilation unit, > the names will be different.) > > If I'm not overlooking something, it'd work well to forward-declare this class > in the private section of AudioEncoderOpus instead. Thanks! It reads better.
lgtm
lgtm
The CQ bit was checked by minyue@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/11414)
https://codereview.webrtc.org/2390883004/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/test/PacketLossTest.cc (right): https://codereview.webrtc.org/2390883004/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/test/PacketLossTest.cc:115: return success; On 2016/10/06 09:46:13, kwiberg-webrtc wrote: > Excellent. Hi Karl, I know that you liked this change, me too. but the codec creation in Sender uses the old fashion and that prevents this to take place. I have to roll back. Sorry.
lgtm https://codereview.webrtc.org/2390883004/diff/140001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2390883004/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:91: smoother_(0.9999f) {} Would be nice with a comment about what the filter coeff means physically - e.g. settles to within 1% in N millisecs.
https://codereview.webrtc.org/2390883004/diff/140001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2390883004/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:91: smoother_(0.9999f) {} On 2016/10/11 17:52:42, the sun wrote: > Would be nice with a comment about what the filter coeff means physically - e.g. > settles to within 1% in N millisecs. I do not quite recall what that number means :P, that is why comments are important. Now added.
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, michaelt@webrtc.org, solenberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2390883004/#ps160001 (title: "adding a comment")
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 ========== Hooking up audio network adaptor to VoE. BUG=webrtc:6303 ========== to ========== Hooking up audio network adaptor to VoE. BUG=webrtc:6303 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Hooking up audio network adaptor to VoE. BUG=webrtc:6303 ========== to ========== Hooking up audio network adaptor to VoE. BUG=webrtc:6303 Committed: https://crrev.com/7e30432b3663ca64aa4776becc536b4a64fc45c9 Cr-Commit-Position: refs/heads/master@{#14611} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/7e30432b3663ca64aa4776becc536b4a64fc45c9 Cr-Commit-Position: refs/heads/master@{#14611} |
