Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2397573006:
Using AudioOption to enable audio network adaptor. (Closed)
|
Created:
4 years, 2 months ago by minyue-webrtc Modified:
4 years, 1 month ago CC:
webrtc-reviews_webrtc.org, peah-webrtc, Andrew MacDonald, henrika_webrtc, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionUsing AudioOption to enable audio network adaptor.
BUG=webrtc:6303
Committed: https://crrev.com/6b825df37e49398bac89abedc6a45048b87d54ed
Cr-Commit-Position: refs/heads/master@{#14845}
Patch Set 1 #
Total comments: 5
Patch Set 2 : new approach: go through AudioSendStream's ctor #
Total comments: 4
Patch Set 3 : fixing some problems #
Total comments: 21
Patch Set 4 : rebasing #Patch Set 5 : on comments #
Total comments: 17
Patch Set 6 : EnableAudioNetworkAdaptor return void and other fixes #
Total comments: 2
Patch Set 7 : fixing a unittest #
Depends on Patchset: Messages
Total messages: 55 (24 generated)
Description was changed from ========== Using AudioOption to enable audio network adaptor. BUG=webrtc:6303 ========== to ========== Using AudioOption to enable audio network adaptor. BUG=webrtc:6303 ==========
minyue@webrtc.org changed reviewers: + solenberg@webrtc.org
Hi Fredrik, I tried to use audio send stream to set up ANA, but it feels quite cumbersome. Please advise in this CL.
https://codereview.webrtc.org/2397573006/diff/1/webrtc/api/call/audio_send_st... File webrtc/api/call/audio_send_stream.h (right): https://codereview.webrtc.org/2397573006/diff/1/webrtc/api/call/audio_send_st... webrtc/api/call/audio_send_stream.h:114: virtual bool EnableAudioNetworkAdaptor(const std::string& config_string) = 0; One of the major ideas with the new API is to have as few state-mutating functions as possible, and instead move all of these types of settings to the Config struct, so that they are only ever initialized in the constructor. When settings change (because e.g. an Offer-Answer exchange), streams are instead *recreated*. That being said, IIUC you're also adding the information to AudioCodingModule::Config. Since there is an ACM per VoE Channel (and a VoE Channel per send stream), does that mean you do not need any of these API calls at all, but that it is enough to propagate the information that path? https://codereview.webrtc.org/2397573006/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2397573006/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:2017: acm_config.audio_network_adaptor_config)) { If this information is already propagated to the Channel's ACM, why do you need to act on it here as well?
https://codereview.webrtc.org/2397573006/diff/1/webrtc/api/call/audio_send_st... File webrtc/api/call/audio_send_stream.h (right): https://codereview.webrtc.org/2397573006/diff/1/webrtc/api/call/audio_send_st... webrtc/api/call/audio_send_stream.h:114: virtual bool EnableAudioNetworkAdaptor(const std::string& config_string) = 0; On 2016/10/06 08:03:46, the sun wrote: > One of the major ideas with the new API is to have as few state-mutating > functions as possible, and instead move all of these types of settings to the > Config struct, so that they are only ever initialized in the constructor. When > settings change (because e.g. an Offer-Answer exchange), streams are instead > *recreated*. > > That being said, IIUC you're also adding the information to > AudioCodingModule::Config. Since there is an ACM per VoE Channel (and a VoE > Channel per send stream), does that mean you do not need any of these API calls > at all, but that it is enough to propagate the information that path? Actually, this is audio network adaptor is a codec feature. It belongs to DTX/FEC etc. But differently, it is set by AudioOption instead of SDP. I see that SetOption only touches call_config_, and therefore, I tried to reuse call_config_ (call_config_.acm_config in particular), but this is not necessary. What makes it cumbersome is that I need to call to Channel::EnableAudioNetworkAdaptor in SetSendCodec, which makes it similar to many VoECodec functions. As long as SetSendCodec is called outside Ctor, I cannot find a good way to avoid adding these APIs.
On 2016/10/06 08:16:53, minyue-webrtc wrote: > https://codereview.webrtc.org/2397573006/diff/1/webrtc/api/call/audio_send_st... > File webrtc/api/call/audio_send_stream.h (right): > > https://codereview.webrtc.org/2397573006/diff/1/webrtc/api/call/audio_send_st... > webrtc/api/call/audio_send_stream.h:114: virtual bool > EnableAudioNetworkAdaptor(const std::string& config_string) = 0; > On 2016/10/06 08:03:46, the sun wrote: > > One of the major ideas with the new API is to have as few state-mutating > > functions as possible, and instead move all of these types of settings to the > > Config struct, so that they are only ever initialized in the constructor. When > > settings change (because e.g. an Offer-Answer exchange), streams are instead > > *recreated*. > > > > That being said, IIUC you're also adding the information to > > AudioCodingModule::Config. Since there is an ACM per VoE Channel (and a VoE > > Channel per send stream), does that mean you do not need any of these API > calls > > at all, but that it is enough to propagate the information that path? > > Actually, this is audio network adaptor is a codec feature. It belongs to > DTX/FEC etc. But differently, it is set by AudioOption instead of SDP. I see > that SetOption only touches call_config_, and therefore, I tried to reuse > call_config_ (call_config_.acm_config in particular), but this is not necessary. > > What makes it cumbersome is that I need to call to > Channel::EnableAudioNetworkAdaptor in SetSendCodec, which makes it similar to > many VoECodec functions. As long as SetSendCodec is called outside Ctor, I > cannot find a good way to avoid adding these APIs. The AudioSendStream:s are recreated as one of the last steps of WVoMC::SetSendCodecs(). Why can't you pass the information in AudioSendStream::Config at that point?
On 2016/10/06 08:21:28, the sun wrote: > On 2016/10/06 08:16:53, minyue-webrtc wrote: > > > https://codereview.webrtc.org/2397573006/diff/1/webrtc/api/call/audio_send_st... > > File webrtc/api/call/audio_send_stream.h (right): > > > > > https://codereview.webrtc.org/2397573006/diff/1/webrtc/api/call/audio_send_st... > > webrtc/api/call/audio_send_stream.h:114: virtual bool > > EnableAudioNetworkAdaptor(const std::string& config_string) = 0; > > On 2016/10/06 08:03:46, the sun wrote: > > > One of the major ideas with the new API is to have as few state-mutating > > > functions as possible, and instead move all of these types of settings to > the > > > Config struct, so that they are only ever initialized in the constructor. > When > > > settings change (because e.g. an Offer-Answer exchange), streams are instead > > > *recreated*. > > > > > > That being said, IIUC you're also adding the information to > > > AudioCodingModule::Config. Since there is an ACM per VoE Channel (and a VoE > > > Channel per send stream), does that mean you do not need any of these API > > calls > > > at all, but that it is enough to propagate the information that path? > > > > Actually, this is audio network adaptor is a codec feature. It belongs to > > DTX/FEC etc. But differently, it is set by AudioOption instead of SDP. I see > > that SetOption only touches call_config_, and therefore, I tried to reuse > > call_config_ (call_config_.acm_config in particular), but this is not > necessary. > > > > What makes it cumbersome is that I need to call to > > Channel::EnableAudioNetworkAdaptor in SetSendCodec, which makes it similar to > > many VoECodec functions. As long as SetSendCodec is called outside Ctor, I > > cannot find a good way to avoid adding these APIs. > > The AudioSendStream:s are recreated as one of the last steps of > WVoMC::SetSendCodecs(). Why can't you pass the information in > AudioSendStream::Config at that point? sounds promising. Let me try.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
minyue@webrtc.org changed reviewers: + michaelt@webrtc.org
Patchset #2 (id:80001) has been deleted
https://codereview.webrtc.org/2397573006/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2397573006/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1335: void SetReceiverFrameLengthRange(int min_frame_length_ms, Will you add a interface for ana_dump as well ? https://codereview.webrtc.org/2397573006/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:2016: if (it->second->EnableAudioNetworkAdaptor( You will have to change the order of Enable and set frame length when "Simplifying audio network adaptor by moving receiver frame length range to ctor." is landed.
On 2016/10/19 13:32:55, michaelt wrote: > https://codereview.webrtc.org/2397573006/diff/1/webrtc/media/engine/webrtcvoi... > File webrtc/media/engine/webrtcvoiceengine.cc (right): > > https://codereview.webrtc.org/2397573006/diff/1/webrtc/media/engine/webrtcvoi... > webrtc/media/engine/webrtcvoiceengine.cc:1335: void > SetReceiverFrameLengthRange(int min_frame_length_ms, > Will you add a interface for ana_dump as well ? > > https://codereview.webrtc.org/2397573006/diff/1/webrtc/media/engine/webrtcvoi... > webrtc/media/engine/webrtcvoiceengine.cc:2016: if > (it->second->EnableAudioNetworkAdaptor( > You will have to change the order of Enable and set frame length when > "Simplifying audio network adaptor by moving receiver frame length range to > ctor." is landed. Yes. I have not added dump. That is to be decided. And please not that PS1 is obsolete. Do not spend time on it. Sooner will I add a new patch set.
Hi Fredrik and Michael, As https://codereview.webrtc.org/2405183002/ is close to finishing. I added a new patch set that applies the optimal solution so far. PTAL again.
https://codereview.webrtc.org/2397573006/diff/100001/webrtc/api/call/audio_se... File webrtc/api/call/audio_send_stream.h (right): https://codereview.webrtc.org/2397573006/diff/100001/webrtc/api/call/audio_se... webrtc/api/call/audio_send_stream.h:103: if (nack_enabled != rhs.nack_enabled) { wouldn't this be the cleaner impl. ? if (nack_enabled == rhs.nack_enabled && transport_cc_enabled == rhs.transport_cc_enabled && ...) return true; return false https://codereview.webrtc.org/2397573006/diff/100001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2397573006/diff/100001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:329: codec->SetFECStatus(channel, false); shouldn't we move this code to the corresponding settings ? if (codec->SetFECStatus(channel, send_codec_spec.enable_codec_fec) == -1) { LOG_RTCERR2(SetFECStatus, channel, send_codec_spec.enable_codec_fec, base->LastError()); }
Thanks for the quick comments! BTW, please ignore PS1, and do not try to compare PS2 against PS1, since that diff shows many changes from https://codereview.webrtc.org/2405183002/ which is about to land. https://codereview.webrtc.org/2397573006/diff/100001/webrtc/api/call/audio_se... File webrtc/api/call/audio_send_stream.h (right): https://codereview.webrtc.org/2397573006/diff/100001/webrtc/api/call/audio_se... webrtc/api/call/audio_send_stream.h:103: if (nack_enabled != rhs.nack_enabled) { On 2016/10/20 09:10:04, michaelt wrote: > wouldn't this be the cleaner impl. ? > > if (nack_enabled == rhs.nack_enabled && > transport_cc_enabled == rhs.transport_cc_enabled && > ...) > return true; > return false Yes, that can be an improvement. https://codereview.webrtc.org/2397573006/diff/100001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2397573006/diff/100001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:329: codec->SetFECStatus(channel, false); On 2016/10/20 09:10:04, michaelt wrote: > shouldn't we move this code to the corresponding settings ? > > if (codec->SetFECStatus(channel, send_codec_spec.enable_codec_fec) == -1) { > LOG_RTCERR2(SetFECStatus, channel, send_codec_spec.enable_codec_fec, > base->LastError()); > } > > I agree. This part should be refactored. This is not the purpose of this CL though.
Hi Fredrik, Would you take a look at this CL?
Hi, please wait a second. It is not completely valid. I need to make a change. Will ping when ready.
Hi Fredrik and Michael, I made a retouch of the CL. Now it is ready to review. PTAL. Thanks! https://codereview.chromium.org/2397573006/diff/120001/webrtc/media/base/medi... File webrtc/media/base/mediachannel.h (right): https://codereview.chromium.org/2397573006/diff/120001/webrtc/media/base/medi... webrtc/media/base/mediachannel.h:276: rtc::Optional<bool> audio_network_adaptor; I initially wanted to let audio_network_adaptor_config.has_value to determine whether audio network adaptor should be on or off. However, there is a normally a third status for audio options (don't care / don't override). So I need to add an optional<bool> here too. https://codereview.chromium.org/2397573006/diff/120001/webrtc/media/engine/we... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.chromium.org/2397573006/diff/120001/webrtc/media/engine/we... webrtc/media/engine/webrtcvoiceengine.cc:1727: bool WebRtcVoiceMediaChannel::SetOptions(const AudioOptions& options) { When setoptions, the .audio_network_adaptor/audio_network_adaptor_config can be overridden, we add a RecreateAudioSendStream to do the job.
https://codereview.webrtc.org/2397573006/diff/120001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2397573006/diff/120001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:391: if (config_.audio_network_adaptor_config) { Is the invariant that max/min_ptime_ms must be set when the config is specified? DCHECK that here then. What if max < min, etc? I can't make out what happens from the convoluted structure below this level. https://codereview.webrtc.org/2397573006/diff/120001/webrtc/media/base/mediac... File webrtc/media/base/mediachannel.h (right): https://codereview.webrtc.org/2397573006/diff/120001/webrtc/media/base/mediac... webrtc/media/base/mediachannel.h:234: ost << ToStringIfSet("audio_network_adaptor", audio_network_adaptor); audio_network_adaptor_config missing? https://codereview.webrtc.org/2397573006/diff/120001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2397573006/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:194: int value; = 0 always initializing is a good habit https://codereview.webrtc.org/2397573006/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1745: if (!*options_.audio_network_adaptor) { Simplify the logic to use a single loop. https://codereview.webrtc.org/2397573006/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:2052: if (options_.audio_network_adaptor && *options_.audio_network_adaptor && So this is exactly the same logic as in SetOptions(), but written in a different way. Make a utility function? https://codereview.webrtc.org/2397573006/diff/120001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel_proxy.cc (right): https://codereview.webrtc.org/2397573006/diff/120001/webrtc/voice_engine/chan... webrtc/voice_engine/channel_proxy.cc:219: return channel()->EnableAudioNetworkAdaptor(config_string); AFAICT this call cannot fail (that would mean new() failed for AudioNetworkAdaptorImpl). Please remove the return value in the ChannelProxy and DCHECK the assumption instead. Preferably, you could also create a CL to simplify things down the chain - I don't see how this settable NetworkAdaptorCreator is ever used - only the default one seems to be used?
https://codereview.webrtc.org/2397573006/diff/120001/webrtc/media/base/mediac... File webrtc/media/base/mediachannel.h (right): https://codereview.webrtc.org/2397573006/diff/120001/webrtc/media/base/mediac... webrtc/media/base/mediachannel.h:276: rtc::Optional<bool> audio_network_adaptor; On 2016/10/24 10:57:11, minyue-webrtc wrote: > I initially wanted to let audio_network_adaptor_config.has_value to determine > whether audio network adaptor should be on or off. However, there is a normally > a third status for audio options (don't care / don't override). So I need to add > an optional<bool> here too. How are you going to roll the feature out? If you're planning to use an experiment (i.e. using the webrtc::field_trial:: API), could you just check that in the WVoMC ctor? That would allow you to use just one field here.
Patchset #4 (id:140001) has been deleted
Hi Fredrik and Michael, I've updated the CL according to your comments. PTAL again! Thanks! https://codereview.webrtc.org/2397573006/diff/120001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2397573006/diff/120001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:391: if (config_.audio_network_adaptor_config) { On 2016/10/25 09:25:46, the sun wrote: > Is the invariant that max/min_ptime_ms must be set when the config is specified? > DCHECK that here then. > > What if max < min, etc? I can't make out what happens from the convoluted > structure below this level. They are parsed from SDP. This is checked down in Audio Network Adaptor which DCHECKs that the initial sending frame length is covered in the range. It would be good to check it in SDP parser too. But I found that pMinTime is not really parsed there. see https://cs.chromium.org/chromium/src/third_party/webrtc/api/webrtcsdp.cc?q=kC... The consequence is that we will use default value and the maxptime, which is parsed, needs to be meaningful. Therefore I added a DCHECK at the place where the eventual values are determined. BTW, maybe pmintime should be parsed (in a separate CL). https://codereview.webrtc.org/2397573006/diff/120001/webrtc/media/base/mediac... File webrtc/media/base/mediachannel.h (right): https://codereview.webrtc.org/2397573006/diff/120001/webrtc/media/base/mediac... webrtc/media/base/mediachannel.h:234: ost << ToStringIfSet("audio_network_adaptor", audio_network_adaptor); On 2016/10/25 09:25:46, the sun wrote: > audio_network_adaptor_config missing? I don't know if to String will be useful for audio_network_adaptor_config because it is not human readable. Do you think it is still good to add it? https://codereview.webrtc.org/2397573006/diff/120001/webrtc/media/base/mediac... webrtc/media/base/mediachannel.h:276: rtc::Optional<bool> audio_network_adaptor; On 2016/10/25 09:46:32, the sun wrote: > On 2016/10/24 10:57:11, minyue-webrtc wrote: > > I initially wanted to let audio_network_adaptor_config.has_value to determine > > whether audio network adaptor should be on or off. However, there is a > normally > > a third status for audio options (don't care / don't override). So I need to > add > > an optional<bool> here too. > > How are you going to roll the feature out? If you're planning to use an > experiment (i.e. using the webrtc::field_trial:: API), could you just check that > in the WVoMC ctor? That would allow you to use just one field here. The feature is configured in a way that it is up to the application to decide whether to turn it on and how to configure audio network adaptor. https://codereview.webrtc.org/2397573006/diff/120001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2397573006/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:194: int value; On 2016/10/25 09:25:46, the sun wrote: > = 0 > > always initializing is a good habit Done. https://codereview.webrtc.org/2397573006/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:2052: if (options_.audio_network_adaptor && *options_.audio_network_adaptor && On 2016/10/25 09:25:46, the sun wrote: > So this is exactly the same logic as in SetOptions(), but written in a different > way. Make a utility function? Not really exactly the same, here we interpret "options_.audio_network_adaptor.has_value() == false" as turning off audio network adaptor, since this is the first time creating AudioSendStream, while, in SetOptions() we take "options_.audio_network_adaptor.has_value() == false" as keep current status. The way it is written in SetOptions() currently is to avoid calling RecreateAudioSendStream when "options_.audio_network_adaptor.has_value() == false". That makes it a bit hard to read. We can, nevertheless, just always call RecreateAudioSendStream since it, anyways, will not recreate audio send stream if the config_ is the same. Then we can unify the two cases and make a helper function. See new patch set. https://codereview.webrtc.org/2397573006/diff/180001/webrtc/api/call/audio_se... File webrtc/api/call/audio_send_stream.cc (right): https://codereview.webrtc.org/2397573006/diff/180001/webrtc/api/call/audio_se... webrtc/api/call/audio_send_stream.cc:96: if (nack_enabled == rhs.nack_enabled && refactored per Michael's request
https://codereview.webrtc.org/2397573006/diff/120001/webrtc/media/base/mediac... File webrtc/media/base/mediachannel.h (right): https://codereview.webrtc.org/2397573006/diff/120001/webrtc/media/base/mediac... webrtc/media/base/mediachannel.h:234: ost << ToStringIfSet("audio_network_adaptor", audio_network_adaptor); On 2016/10/27 14:33:10, minyue-webrtc wrote: > On 2016/10/25 09:25:46, the sun wrote: > > audio_network_adaptor_config missing? > > I don't know if to String will be useful for audio_network_adaptor_config > because it is not human readable. Do you think it is still good to add it? Oh? What does it look like? Is it really not possible to understand anything about the configuration by looking at the string? Then, why is it a string? :) https://codereview.webrtc.org/2397573006/diff/180001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream_unittest.cc (right): https://codereview.webrtc.org/2397573006/diff/180001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream_unittest.cc:381: EXPECT_CALL(*helper.voice_engine(), SetVADStatus(kChannelId, true, _, _)) Do you need and expectation on DisableAudioNetworkAdaptor? https://codereview.webrtc.org/2397573006/diff/180001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2397573006/diff/180001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:262: RTC_DCHECK_GE(*max_ptime_ms, *min_ptime_ms); You cannot DCHECK this - it is a runtime condition and we're acting on possibly random data from the network. If this is a requirement, you should handle max < min and somehow provide a way out. CHECK is not ok btw. https://codereview.webrtc.org/2397573006/diff/180001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2397573006/diff/180001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:2379: EXPECT_TRUE(channel_->SetSendParameters(send_parameters_)); You may need to use the test utility "SetSendParameters" if the mock complains. https://codereview.webrtc.org/2397573006/diff/180001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:2395: EXPECT_TRUE(channel_->SetAudioSend(kSsrc1, true, &options, nullptr)); There's a utility for SetAudioSend as well https://codereview.webrtc.org/2397573006/diff/180001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel_proxy.cc (right): https://codereview.webrtc.org/2397573006/diff/180001/webrtc/voice_engine/chan... webrtc/voice_engine/channel_proxy.cc:219: return channel()->EnableAudioNetworkAdaptor(config_string); AFAICT this call cannot fail (that would mean new() failed for AudioNetworkAdaptorImpl). Please remove the return value in the ChannelProxy and DCHECK the assumption instead. Preferably, you could also create a CL to simplify things down the chain - I don't see how this settable NetworkAdaptorCreator is ever used - only the default one seems to be used? https://codereview.webrtc.org/2397573006/diff/180001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel_proxy.h (right): https://codereview.webrtc.org/2397573006/diff/180001/webrtc/voice_engine/chan... webrtc/voice_engine/channel_proxy.h:97: remove blank lines
Hi Fredrik, Thanks for the feedback! See my replies first before I go for a new patch set. https://codereview.webrtc.org/2397573006/diff/120001/webrtc/media/base/mediac... File webrtc/media/base/mediachannel.h (right): https://codereview.webrtc.org/2397573006/diff/120001/webrtc/media/base/mediac... webrtc/media/base/mediachannel.h:234: ost << ToStringIfSet("audio_network_adaptor", audio_network_adaptor); On 2016/10/28 18:21:52, the sun wrote: > On 2016/10/27 14:33:10, minyue-webrtc wrote: > > On 2016/10/25 09:25:46, the sun wrote: > > > audio_network_adaptor_config missing? > > > > I don't know if to String will be useful for audio_network_adaptor_config > > because it is not human readable. Do you think it is still good to add it? > > Oh? What does it look like? Is it really not possible to understand anything > about the configuration by looking at the string? Then, why is it a string? :) It is a proto buffer serialized string. it is a std::string since proto buffer c++ api uses it as default type. https://codereview.webrtc.org/2397573006/diff/180001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream_unittest.cc (right): https://codereview.webrtc.org/2397573006/diff/180001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream_unittest.cc:381: EXPECT_CALL(*helper.voice_engine(), SetVADStatus(kChannelId, true, _, _)) On 2016/10/28 18:21:52, the sun wrote: > Do you need and expectation on DisableAudioNetworkAdaptor? I wrote it in the way such that both EnableAudioNetworkAdaptor and DisableAudioNetworkAdaptor are called only when codec is opus. https://codereview.webrtc.org/2397573006/diff/180001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2397573006/diff/180001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:262: RTC_DCHECK_GE(*max_ptime_ms, *min_ptime_ms); On 2016/10/28 18:21:52, the sun wrote: > You cannot DCHECK this - it is a runtime condition and we're acting on possibly > random data from the network. If this is a requirement, you should handle max < > min and somehow provide a way out. CHECK is not ok btw. sure, will do if (*max_ptime_ms < *min_ptime_ms) { *max_ptime_ms = kCodecParamMaxPTime; *min_ptime_ms = kCodecParamMinPTime; } if you like it. https://codereview.webrtc.org/2397573006/diff/180001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2397573006/diff/180001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:2379: EXPECT_TRUE(channel_->SetSendParameters(send_parameters_)); On 2016/10/28 18:21:52, the sun wrote: > You may need to use the test utility "SetSendParameters" if the mock complains. I see, SetSendParameters is recently added. Will do. https://codereview.webrtc.org/2397573006/diff/180001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:2395: EXPECT_TRUE(channel_->SetAudioSend(kSsrc1, true, &options, nullptr)); On 2016/10/28 18:21:52, the sun wrote: > There's a utility for SetAudioSend as well will do https://codereview.webrtc.org/2397573006/diff/180001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel_proxy.cc (right): https://codereview.webrtc.org/2397573006/diff/180001/webrtc/voice_engine/chan... webrtc/voice_engine/channel_proxy.cc:219: return channel()->EnableAudioNetworkAdaptor(config_string); On 2016/10/28 18:21:52, the sun wrote: > AFAICT this call cannot fail (that would mean new() failed for > AudioNetworkAdaptorImpl). Please remove the return value in the ChannelProxy and > DCHECK the assumption instead. Preferably, you could also create a CL to > simplify things down the chain - I don't see how this settable > NetworkAdaptorCreator is ever used - only the default one seems to be used? I see. I can remove return value and DCHECH result from channel here in this CL. General question: do we plan to add a CreateAudioSendStream which gives nullptr or failure info when creation is failed? https://codereview.webrtc.org/2397573006/diff/180001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel_proxy.h (right): https://codereview.webrtc.org/2397573006/diff/180001/webrtc/voice_engine/chan... webrtc/voice_engine/channel_proxy.h:97: On 2016/10/28 18:21:53, the sun wrote: > remove blank lines will do
https://codereview.webrtc.org/2397573006/diff/120001/webrtc/media/base/mediac... File webrtc/media/base/mediachannel.h (right): https://codereview.webrtc.org/2397573006/diff/120001/webrtc/media/base/mediac... webrtc/media/base/mediachannel.h:234: ost << ToStringIfSet("audio_network_adaptor", audio_network_adaptor); On 2016/10/28 18:41:57, minyue-webrtc wrote: > On 2016/10/28 18:21:52, the sun wrote: > > On 2016/10/27 14:33:10, minyue-webrtc wrote: > > > On 2016/10/25 09:25:46, the sun wrote: > > > > audio_network_adaptor_config missing? > > > > > > I don't know if to String will be useful for audio_network_adaptor_config > > > because it is not human readable. Do you think it is still good to add it? > > > > Oh? What does it look like? Is it really not possible to understand anything > > about the configuration by looking at the string? Then, why is it a string? :) > > It is a proto buffer serialized string. it is a std::string since proto buffer > c++ api uses it as default type. Ah, in that case, just leave: // The adaptor config is a serialized proto buffer and therefore not human readable. // ost << ToStringIfSet("audio_network_adaptor_config", audio_network_adaptor_config); https://codereview.webrtc.org/2397573006/diff/120001/webrtc/media/base/mediac... webrtc/media/base/mediachannel.h:276: rtc::Optional<bool> audio_network_adaptor; On 2016/10/27 14:33:10, minyue-webrtc wrote: > On 2016/10/25 09:46:32, the sun wrote: > > On 2016/10/24 10:57:11, minyue-webrtc wrote: > > > I initially wanted to let audio_network_adaptor_config.has_value to > determine > > > whether audio network adaptor should be on or off. However, there is a > > normally > > > a third status for audio options (don't care / don't override). So I need to > > add > > > an optional<bool> here too. > > > > How are you going to roll the feature out? If you're planning to use an > > experiment (i.e. using the webrtc::field_trial:: API), could you just check > that > > in the WVoMC ctor? That would allow you to use just one field here. > > The feature is configured in a way that it is up to the application to decide > whether to turn it on and how to configure audio network adaptor. Btw, what happened to your idea of only having the config string? https://codereview.webrtc.org/2397573006/diff/180001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream_unittest.cc (right): https://codereview.webrtc.org/2397573006/diff/180001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream_unittest.cc:381: EXPECT_CALL(*helper.voice_engine(), SetVADStatus(kChannelId, true, _, _)) On 2016/10/28 18:41:57, minyue-webrtc wrote: > On 2016/10/28 18:21:52, the sun wrote: > > Do you need and expectation on DisableAudioNetworkAdaptor? > > I wrote it in the way such that both EnableAudioNetworkAdaptor and > DisableAudioNetworkAdaptor are called only when codec is opus. Acknowledged. https://codereview.webrtc.org/2397573006/diff/180001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2397573006/diff/180001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:262: RTC_DCHECK_GE(*max_ptime_ms, *min_ptime_ms); On 2016/10/28 18:41:57, minyue-webrtc wrote: > On 2016/10/28 18:21:52, the sun wrote: > > You cannot DCHECK this - it is a runtime condition and we're acting on > possibly > > random data from the network. If this is a requirement, you should handle max > < > > min and somehow provide a way out. CHECK is not ok btw. > > sure, will do > > if (*max_ptime_ms < *min_ptime_ms) { > *max_ptime_ms = kCodecParamMaxPTime; > *min_ptime_ms = kCodecParamMinPTime; > } > > if you like it. You mean: if (*max_ptime_ms < *min_ptime_ms) { *max_ptime_ms = kOpusDefaultMaxPTime; *min_ptime_ms = kOpusDefaultMinPTime; } ? sgtm https://codereview.webrtc.org/2397573006/diff/180001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel_proxy.cc (right): https://codereview.webrtc.org/2397573006/diff/180001/webrtc/voice_engine/chan... webrtc/voice_engine/channel_proxy.cc:219: return channel()->EnableAudioNetworkAdaptor(config_string); On 2016/10/28 18:41:57, minyue-webrtc wrote: > On 2016/10/28 18:21:52, the sun wrote: > > AFAICT this call cannot fail (that would mean new() failed for > > AudioNetworkAdaptorImpl). Please remove the return value in the ChannelProxy > and > > DCHECK the assumption instead. Preferably, you could also create a CL to > > simplify things down the chain - I don't see how this settable > > NetworkAdaptorCreator is ever used - only the default one seems to be used? > > I see. I can remove return value and DCHECH result from channel here in this CL. sgtm > General question: do we plan to add a CreateAudioSendStream which gives nullptr > or failure info when creation is failed? That's a good question. Currently, the answer is 'no', no such plans. This is pretty deep subject. Personally I think return codes should only be used when there's a possibility for resource exhaustion (e.g. some piece of hardware or OS function cannot be accessed) AND the API client can conceivably use the return code to work around it. RAM is obviously a hardware resources, but one that we depend on throughout the stack to such a large extent that, if we'd run out, we'd already be in deep trouble and might as well terminate the process. So, for most things we do, where RAM is the only resource requirement, return codes are not that useful.
Patchset #6 (id:200001) has been deleted
Hi Fredrik, Now comes a new patch set. https://codereview.webrtc.org/2397573006/diff/120001/webrtc/media/base/mediac... File webrtc/media/base/mediachannel.h (right): https://codereview.webrtc.org/2397573006/diff/120001/webrtc/media/base/mediac... webrtc/media/base/mediachannel.h:276: rtc::Optional<bool> audio_network_adaptor; On 2016/10/28 19:04:47, the sun wrote: > On 2016/10/27 14:33:10, minyue-webrtc wrote: > > On 2016/10/25 09:46:32, the sun wrote: > > > On 2016/10/24 10:57:11, minyue-webrtc wrote: > > > > I initially wanted to let audio_network_adaptor_config.has_value to > > determine > > > > whether audio network adaptor should be on or off. However, there is a > > > normally > > > > a third status for audio options (don't care / don't override). So I need > to > > > add > > > > an optional<bool> here too. > > > > > > How are you going to roll the feature out? If you're planning to use an > > > experiment (i.e. using the webrtc::field_trial:: API), could you just check > > that > > > in the WVoMC ctor? That would allow you to use just one field here. > > > > The feature is configured in a way that it is up to the application to decide > > whether to turn it on and how to configure audio network adaptor. > > Btw, what happened to your idea of only having the config string? I initially wanted to let audio_network_adaptor_config.has_value to determine whether audio network adaptor should be on or off. However, there is a a third status for audio options (don't care / don't override). So I need to add an optional<bool> here too. https://codereview.webrtc.org/2397573006/diff/180001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2397573006/diff/180001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:262: RTC_DCHECK_GE(*max_ptime_ms, *min_ptime_ms); On 2016/10/28 19:04:47, the sun wrote: > On 2016/10/28 18:41:57, minyue-webrtc wrote: > > On 2016/10/28 18:21:52, the sun wrote: > > > You cannot DCHECK this - it is a runtime condition and we're acting on > > possibly > > > random data from the network. If this is a requirement, you should handle > max > > < > > > min and somehow provide a way out. CHECK is not ok btw. > > > > sure, will do > > > > if (*max_ptime_ms < *min_ptime_ms) { > > *max_ptime_ms = kCodecParamMaxPTime; > > *min_ptime_ms = kCodecParamMinPTime; > > } > > > > if you like it. > > You mean: > > if (*max_ptime_ms < *min_ptime_ms) { > *max_ptime_ms = kOpusDefaultMaxPTime; > *min_ptime_ms = kOpusDefaultMinPTime; > } > > ? > sgtm yes, you are right. see new patch set.
The CQ bit was checked by minyue@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/19622)
LGTM https://codereview.webrtc.org/2397573006/diff/120001/webrtc/media/base/mediac... File webrtc/media/base/mediachannel.h (right): https://codereview.webrtc.org/2397573006/diff/120001/webrtc/media/base/mediac... webrtc/media/base/mediachannel.h:276: rtc::Optional<bool> audio_network_adaptor; On 2016/10/28 20:15:03, minyue-webrtc wrote: > On 2016/10/28 19:04:47, the sun wrote: > > On 2016/10/27 14:33:10, minyue-webrtc wrote: > > > On 2016/10/25 09:46:32, the sun wrote: > > > > On 2016/10/24 10:57:11, minyue-webrtc wrote: > > > > > I initially wanted to let audio_network_adaptor_config.has_value to > > > determine > > > > > whether audio network adaptor should be on or off. However, there is a > > > > normally > > > > > a third status for audio options (don't care / don't override). So I > need > > to > > > > add > > > > > an optional<bool> here too. > > > > > > > > How are you going to roll the feature out? If you're planning to use an > > > > experiment (i.e. using the webrtc::field_trial:: API), could you just > check > > > that > > > > in the WVoMC ctor? That would allow you to use just one field here. > > > > > > The feature is configured in a way that it is up to the application to > decide > > > whether to turn it on and how to configure audio network adaptor. > > > > Btw, what happened to your idea of only having the config string? > > I initially wanted to let audio_network_adaptor_config.has_value to determine > whether audio network adaptor should be on or off. However, there is a > a third status for audio options (don't care / don't override). So I need to add > an optional<bool> here too. I probably misunderstood then - I thought you said since the config string is always required when the adaptor is enabled, that you could use the empty string ("") as the third state. But I may be getting the logic wrong. https://codereview.webrtc.org/2397573006/diff/220001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2397573006/diff/220001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:264: // the default values. What if they are too small/too large? Is that handled at the lower level our do we need to cap them here? Or reject the SDP?
https://codereview.webrtc.org/2397573006/diff/120001/webrtc/media/base/mediac... File webrtc/media/base/mediachannel.h (right): https://codereview.webrtc.org/2397573006/diff/120001/webrtc/media/base/mediac... webrtc/media/base/mediachannel.h:276: rtc::Optional<bool> audio_network_adaptor; On 2016/10/28 20:33:08, the sun wrote: > On 2016/10/28 20:15:03, minyue-webrtc wrote: > > On 2016/10/28 19:04:47, the sun wrote: > > > On 2016/10/27 14:33:10, minyue-webrtc wrote: > > > > On 2016/10/25 09:46:32, the sun wrote: > > > > > On 2016/10/24 10:57:11, minyue-webrtc wrote: > > > > > > I initially wanted to let audio_network_adaptor_config.has_value to > > > > determine > > > > > > whether audio network adaptor should be on or off. However, there is a > > > > > normally > > > > > > a third status for audio options (don't care / don't override). So I > > need > > > to > > > > > add > > > > > > an optional<bool> here too. > > > > > > > > > > How are you going to roll the feature out? If you're planning to use an > > > > > experiment (i.e. using the webrtc::field_trial:: API), could you just > > check > > > > that > > > > > in the WVoMC ctor? That would allow you to use just one field here. > > > > > > > > The feature is configured in a way that it is up to the application to > > decide > > > > whether to turn it on and how to configure audio network adaptor. > > > > > > Btw, what happened to your idea of only having the config string? > > > > I initially wanted to let audio_network_adaptor_config.has_value to determine > > whether audio network adaptor should be on or off. However, there is a > > a third status for audio options (don't care / don't override). So I need to > add > > an optional<bool> here too. > > I probably misunderstood then - I thought you said since the config string is > always required when the adaptor is enabled, that you could use the empty string > ("") as the third state. But I may be getting the logic wrong. If we use only a Option<string>, when a second set of options comes in, we may not be easily distinguish between "don't care" and "turn off". I cannot find better solutions. In principle, Optional<bool> |audio_network_adaptor| must be there, and string |audio_network_adaptor_config| is obligatory when |audio_network_adaptor| is true. But the obligatoriness of |audio_network_adaptor_config| can be changed in the future, if a default config is introduced. https://codereview.webrtc.org/2397573006/diff/220001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2397573006/diff/220001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:264: // the default values. On 2016/10/28 20:33:08, the sun wrote: > What if they are too small/too large? Is that handled at the lower level our do > we need to cap them here? Or reject the SDP? actually, this protect is not 100% strong. We also need initial frame length (ptime) to lay between min_ptime and max_ptime. since this will be corner case, let me add another CL to fix it.
The CQ bit was checked by minyue@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by minyue@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2397573006/#ps240001 (title: "fixing a unittest")
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: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/9664)
minyue@webrtc.org changed reviewers: + kjellander@webrtc.org
+kjellander Hi Henrik, Adding you to approve test/mock_voe_channel_proxy.h.
On 2016/10/31 08:56:56, minyue-webrtc wrote: > +kjellander > > Hi Henrik, > > Adding you to approve test/mock_voe_channel_proxy.h. lgtm for webrtc/test/mock_voe_channel_proxy.h. Ideally such mocks should be moved closer to the headers they're mocking.
On 2016/10/31 09:50:05, kjellander_webrtc wrote: > On 2016/10/31 08:56:56, minyue-webrtc wrote: > > +kjellander > > > > Hi Henrik, > > > > Adding you to approve test/mock_voe_channel_proxy.h. > > lgtm for webrtc/test/mock_voe_channel_proxy.h. > Ideally such mocks should be moved closer to the headers they're mocking. agree. I created https://bugs.chromium.org/p/webrtc/issues/detail?id=6632 for a coming work on it.
The CQ bit was checked by minyue@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Using AudioOption to enable audio network adaptor. BUG=webrtc:6303 ========== to ========== Using AudioOption to enable audio network adaptor. BUG=webrtc:6303 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Using AudioOption to enable audio network adaptor. BUG=webrtc:6303 ========== to ========== Using AudioOption to enable audio network adaptor. BUG=webrtc:6303 Committed: https://crrev.com/6b825df37e49398bac89abedc6a45048b87d54ed Cr-Commit-Position: refs/heads/master@{#14845} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/6b825df37e49398bac89abedc6a45048b87d54ed Cr-Commit-Position: refs/heads/master@{#14845} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
