Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2705093002:
Injectable audio encoders: WebRtcVoiceEngine and company (Closed)
|
Created:
3 years, 10 months ago by ossu Modified:
3 years, 8 months ago CC:
webrtc-reviews_webrtc.org, video-team_agora.io, yujie_mao (webrtc), Andrew MacDonald, zhengzhonghou_agora.io, henrika_webrtc, stefan-webrtc, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, peah-webrtc, mflodman, lliuu Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionInjectable audio encoders: WebRtcVoiceEngine and company
These are the changes made to WebRtcVoiceEngine and surrounding
code. It still contains some things that are inelegant, like how
AudioCodecSpec and AudioFormatInfo is ferried around in
SendCodecSpec. This should probably be resolved before landing.
There are also a few test still that are disabled. They should be
removed or fixed, as the case may be.
I've put this CL up to get a better overview of the changes made and
how reviewable they are.
BUG=webrtc:5806
Review-Url: https://codereview.webrtc.org/2705093002
Cr-Commit-Position: refs/heads/master@{#17904}
Committed: https://chromium.googlesource.com/external/webrtc/+/20a4b3fb2ae5502488739f852aa73ba6ab26f7ec
Patch Set 1 #
Total comments: 39
Patch Set 2 : Rebased onto different parent CL. #
Total comments: 2
Patch Set 3 : Moved encoder creation up into AudioSendStream, bypassing most of Channel. #
Total comments: 45
Patch Set 4 : audio_send_spec made optional<>, EnableAudioNetworkAdapter now called directly on encoder, VAD supp… #
Total comments: 15
Patch Set 5 : AudioSendStream::Reconfigure() #
Total comments: 21
Patch Set 6 : Rebase (and removed 'virtual' from Channel::ModifyEncoder) #
Total comments: 4
Patch Set 7 : Moved duplicated configuration from constructor to ConfigureStream (previously Reconfigure) #
Total comments: 14
Patch Set 8 : Simplified ConfigureStream, fixed BitrateObserver setup, added ModifyEncoder mock #Patch Set 9 : Rebase. #
Total comments: 44
Patch Set 10 : Cleaned up WebRtcAudioSendStream a bit. Reinstated check of bitrate in test. Comment cleanups. #Patch Set 11 : Rebase #Patch Set 12 : Channel::GetSendCodec asks both its acm and its codec manager. #
Total comments: 2
Depends on Patchset: Messages
Total messages: 79 (28 generated)
ossu@webrtc.org changed reviewers: + kwiberg@webrtc.org, solenberg@webrtc.org
The second of, what I expect to be, three CLs for Injectable audio encoders. This one has a few open questions still, so PTAL and comment on them. There are also a few more tests that should be cleaned up or removed. I'll be working on that next. The third CL will contain the plumbing of AudioEncoderFactory up into PeerConnectionFactory, as well as move the encoder stuff into api/audio_codecs/. https://codereview.webrtc.org/2705093002/diff/1/webrtc/api/DEPS File webrtc/api/DEPS (right): https://codereview.webrtc.org/2705093002/diff/1/webrtc/api/DEPS#newcode15 webrtc/api/DEPS:15: # TODO(ossu): Remove this exception when audio_encoder_factory.h I have a follow-up CL which plumbs the audio encoder factory all the way to PeerConnectionFactory and also moves the requisite headers etc. into api/. https://codereview.webrtc.org/2705093002/diff/1/webrtc/audio/audio_send_strea... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2705093002/diff/1/webrtc/audio/audio_send_strea... webrtc/audio/audio_send_stream.cc:282: // TODO(ossu): Formalize bandwidth parameters and send along to encoder This is one of the open questions: cricket::AudioCodec has bitrate, which is currently propagated through CodecInst and into the encoder. From what I can find, it's never set to anything but 0, except when being filled in with values from ACMCodecDB. If there's no way to signal a fixed bitrate - and I'm not sure there should be - then the application developer will have much more control over bitrates by customizing an AudioEncoderFactory over anything that could be changed at higher levels of the API. https://codereview.webrtc.org/2705093002/diff/1/webrtc/call/audio_send_stream.cc File webrtc/call/audio_send_stream.cc (right): https://codereview.webrtc.org/2705093002/diff/1/webrtc/call/audio_send_stream... webrtc/call/audio_send_stream.cc:59: : format{webrtc::SdpAudioFormat("", 0, 0), Eugh. They should probably have default constructors, so I don't have to fake this here. https://codereview.webrtc.org/2705093002/diff/1/webrtc/call/audio_send_stream.h File webrtc/call/audio_send_stream.h (right): https://codereview.webrtc.org/2705093002/diff/1/webrtc/call/audio_send_stream... webrtc/call/audio_send_stream.h:121: webrtc::AudioCodecSpec format; This isn't very nice. It leads to us having to refer to format.format and format.info. I believe I'd prefer it if instead the AudioFormatInfo was generated by the stream calling encoder_factory->QueryAudioFormat(), but that call fits so well in WebRtcVoiceEngine, since it allows us to check if the format is supported as well, and IIRC, we want the information to be able to set target_bitrate_bps. If that one goes (see previous discussion about bitrate) then cleaning this one up should also be easier. https://codereview.webrtc.org/2705093002/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2705093002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:197: static bool ToCodecInst(const AudioCodec& in, webrtc::CodecInst* out) { With the cleanup of a couple of more tests, this one could go as well. https://codereview.webrtc.org/2705093002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:258: // TODO(ossu): ???? I ... don't know why this early-out was here before. https://codereview.webrtc.org/2705093002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1250: webrtc::AudioFormatInfo audio_format_info_; Not used. Will remove. https://codereview.webrtc.org/2705093002/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (left): https://codereview.webrtc.org/2705093002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine_unittest.cc:680: // Tests that we can find codecs by name or id, and that we interpret the These are just removed because the underlying functionality is/should be removed as well. https://codereview.webrtc.org/2705093002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine_unittest.cc:1234: // Verify that G722 is set with 16000 samples per second to WebRTC. The clock rate <-> sample rate functionality is now moved into the codecs, so shouldn't be tested here. I believe the implementation in AudioEncoderG722 is trivial enough it might not need a separate test even there. https://codereview.webrtc.org/2705093002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine_unittest.cc:1429: // Test that bitrate will be overridden by the "maxaveragebitrate" parameter. Moved into AudioEncoderOpus unit tests. https://codereview.webrtc.org/2705093002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine_unittest.cc:1585: // Test that without useinbandfec, Opus FEC is off. These are all moved into AudioEncoderOpus unit tests or, in the case of the Isac-related tests, completely removed, since there's no FEC to set or test for Isac. https://codereview.webrtc.org/2705093002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine_unittest.cc:1684: // Test maxplaybackrate <= 8000 triggers Opus narrow band mode. Moved into AudioEncoderOpus unit tests. https://codereview.webrtc.org/2705093002/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2705093002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine_unittest.cc:40: const cricket::AudioCodec kOpusCodec(111, "opus", 48000, 32000, 2); Stereo opus is 64kbps, mono is 32kbps. This value needs to match what the codec implementation's QueryAudioFormat says.
This CL is very large. Is it reasonable to split it into smaller pieces? https://codereview.webrtc.org/2705093002/diff/1/webrtc/api/DEPS File webrtc/api/DEPS (right): https://codereview.webrtc.org/2705093002/diff/1/webrtc/api/DEPS#newcode15 webrtc/api/DEPS:15: # TODO(ossu): Remove this exception when audio_encoder_factory.h On 2017/02/21 11:04:14, ossu wrote: > I have a follow-up CL which plumbs the audio encoder factory all the way to > PeerConnectionFactory and also moves the requisite headers etc. into api/. Acknowledged. https://codereview.webrtc.org/2705093002/diff/1/webrtc/audio/audio_send_strea... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2705093002/diff/1/webrtc/audio/audio_send_strea... webrtc/audio/audio_send_stream.cc:282: // TODO(ossu): Formalize bandwidth parameters and send along to encoder On 2017/02/21 11:04:14, ossu wrote: > This is one of the open questions: cricket::AudioCodec has bitrate, which is > currently propagated through CodecInst and into the encoder. From what I can > find, it's never set to anything but 0, except when being filled in with values > from ACMCodecDB. > > If there's no way to signal a fixed bitrate - and I'm not sure there should be - > then the application developer will have much more control over bitrates by > customizing an AudioEncoderFactory over anything that could be changed at higher > levels of the API. It'd be good if we could do that. But that's only accessible from C++---I don't know if that's a problem. https://codereview.webrtc.org/2705093002/diff/1/webrtc/audio/audio_send_strea... File webrtc/audio/audio_send_stream_unittest.cc (right): https://codereview.webrtc.org/2705093002/diff/1/webrtc/audio/audio_send_strea... webrtc/audio/audio_send_stream_unittest.cc:185: .Times(0); Won't clang-format remove this line break? https://codereview.webrtc.org/2705093002/diff/1/webrtc/audio/audio_send_strea... webrtc/audio/audio_send_stream_unittest.cc:390: const webrtc::SdpAudioFormat kOpusFormat = {"opus", 48000, 2}; Too bad we can't make this constexpr. Remind me to talk about a nice small vector optimization I learned about when we're both at the office at the same time... https://codereview.webrtc.org/2705093002/diff/1/webrtc/call/audio_send_stream.cc File webrtc/call/audio_send_stream.cc (right): https://codereview.webrtc.org/2705093002/diff/1/webrtc/call/audio_send_stream... webrtc/call/audio_send_stream.cc:59: : format{webrtc::SdpAudioFormat("", 0, 0), On 2017/02/21 11:04:14, ossu wrote: > Eugh. They should probably have default constructors, so I don't have to fake > this here. IIRC, SdpAudioFormat used to have a default constructor because rtc::Optional required it, but we removed it once that wasn't the case anymore, because there isn't a natural "default" or "empty" state for SdpAudioFormat. If you want SdpAudioFormat plus an empty default state, use rtc::Optional<SdpAudioFormat>. Alternatively, maybe SendCodecSpec doesn't have a natural "default" or "empty" state... https://codereview.webrtc.org/2705093002/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2705093002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:266: // bitrate then ignore. This comment doesn't seem entirely accurate. And it should probably be before line 263, since it applies to all the remaining statements in this function. https://codereview.webrtc.org/2705093002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1037: // TODO(ossu): Should not recreate stream! Because we should mutate the one we have? https://codereview.webrtc.org/2705093002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1213: // OverheadPerPacket = Ipv4(20B) + UDP(8B) + SRTP(10B) + RTP(12) Missing B https://codereview.webrtc.org/2705093002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1711: IsCodec(codec, kRedCodecName))) { To reduce indentation, invert this condition and "continue" if it's true. https://codereview.webrtc.org/2705093002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1716: auto info = engine()->encoder_factory_->QueryAudioFormat(format); It may be useful to write out this type. https://codereview.webrtc.org/2705093002/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (left): https://codereview.webrtc.org/2705093002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine_unittest.cc:1234: // Verify that G722 is set with 16000 samples per second to WebRTC. On 2017/02/21 11:04:15, ossu wrote: > The clock rate <-> sample rate functionality is now moved into the codecs, so > shouldn't be tested here. I believe the implementation in AudioEncoderG722 is > trivial enough it might not need a separate test even there. Acknowledged. https://codereview.webrtc.org/2705093002/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2705093002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine_unittest.cc:40: const cricket::AudioCodec kOpusCodec(111, "opus", 48000, 32000, 2); On 2017/02/21 11:04:14, ossu wrote: > Stereo opus is 64kbps, mono is 32kbps. This value needs to match what the codec > implementation's QueryAudioFormat says. Speculation: whoever wrote this thought that the "2" in the number of channels field meant this was stereo. :-) https://codereview.webrtc.org/2705093002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine_unittest.cc:883: TestMaxSendBandwidth(kIsacCodec, 100000, true, 32000); Why did this change? https://codereview.webrtc.org/2705093002/diff/1/webrtc/video/video_quality_te... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/2705093002/diff/1/webrtc/video/video_quality_te... webrtc/video/video_quality_test.cc:1435: // TODO(ossu): "Add stereo here?" Presumably yes---the "2" in the CodecInst in the old code meant stereo, and that's also consistent with the 64k bitrate. Hard to know if it was truly intentional, though. https://codereview.webrtc.org/2705093002/diff/1/webrtc/voice_engine/channel.cc File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2705093002/diff/1/webrtc/voice_engine/channel.c... webrtc/voice_engine/channel.cc:1225: strncpy(lies.plname, format.name.c_str(), RTP_PAYLOAD_NAME_SIZE); The length limiter should also involve sizeof(lies.plname). https://codereview.webrtc.org/2705093002/diff/1/webrtc/voice_engine/channel.c... webrtc/voice_engine/channel.cc:1228: // carry sample rate, but only clock rate seems sensible to send to the "CodecInst supposedly carries the sample rate," https://codereview.webrtc.org/2705093002/diff/1/webrtc/voice_engine/channel.h File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/2705093002/diff/1/webrtc/voice_engine/channel.h... webrtc/voice_engine/channel.h:191: AudioEncoderFactory* factory); // Not part of VoECodec! Then maybe don't put it under the "// VoECodec" header?
https://codereview.webrtc.org/2705093002/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2705093002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine_unittest.cc:1609: #if 0 I think these should be removed. The encoders that support setting packet times should have their own tests for it. https://codereview.webrtc.org/2705093002/diff/1/webrtc/voice_engine/channel.cc File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2705093002/diff/1/webrtc/voice_engine/channel.c... webrtc/voice_engine/channel.cc:1215: auto encoder = factory->MakeAudioEncoder(payload_type, format); I put the MakeAudioEncoder call here, rather than webrtcvoiceengine or audio_send_stream, because we need to call RegisterSendPayload with stuff in format, so we'll need to pass format along anyway. This could be changed, i.e. by being able to ask the encoder about its format, or maybe RegisterSendPayload could be redesigned? https://codereview.webrtc.org/2705093002/diff/1/webrtc/voice_engine/channel.c... webrtc/voice_engine/channel.cc:1225: strncpy(lies.plname, format.name.c_str(), RTP_PAYLOAD_NAME_SIZE); On 2017/02/21 23:35:04, kwiberg-webrtc wrote: > The length limiter should also involve sizeof(lies.plname). But sizeof(lies.plname) _is_ RTP_PAYLOAD_NAME_SIZE. https://codereview.webrtc.org/2705093002/diff/1/webrtc/voice_engine/channel.c... webrtc/voice_engine/channel.cc:1228: // carry sample rate, but only clock rate seems sensible to send to the On 2017/02/21 23:35:04, kwiberg-webrtc wrote: > "CodecInst supposedly carries the sample rate," Alright. I think this comment is more for the review, rather than being left in. But maybe nobody really knows which one should be in there? It SeemsToWork(tm) regardless of the value I choose. If that's the case, then the comment should remain and I'll rephrase it.
On 2017/02/21 23:35:04, kwiberg-webrtc wrote: > This CL is very large. Is it reasonable to split it into smaller pieces? I looked through it several times trying to find a way to do that, but I was unable to, at the time. It's quite large in the number of changed lines, but most of those lines are trivial - either just big blocks removed or mechanical changes of test lines. Looking at it again, though, I _think_ I can put the audio_send_stream and channel changes in their own CL and staggering it before this one. That should leave mostly webrtcvoiceengine in this one. I also noticed changes to peerconnectioninterface that probably shouldn't be here: they should either be in the first CL, that actually implements the factory, or the last CL that plumbs it all the way.
https://codereview.webrtc.org/2705093002/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2705093002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine_unittest.cc:1609: #if 0 On 2017/02/22 10:24:23, ossu wrote: > I think these should be removed. The encoders that support setting packet times > should have their own tests for it. Acknowledged. https://codereview.webrtc.org/2705093002/diff/1/webrtc/voice_engine/channel.cc File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2705093002/diff/1/webrtc/voice_engine/channel.c... webrtc/voice_engine/channel.cc:1215: auto encoder = factory->MakeAudioEncoder(payload_type, format); On 2017/02/22 10:24:23, ossu wrote: > I put the MakeAudioEncoder call here, rather than webrtcvoiceengine or > audio_send_stream, because we need to call RegisterSendPayload with stuff in > format, so we'll need to pass format along anyway. This could be changed, i.e. > by being able to ask the encoder about its format, or maybe RegisterSendPayload > could be redesigned? We want RegisterSendPayload to not take a CodecInst, at any rate. If we can figure out what data it should actually take, it'll hopefully become clearer whether AudioEncoder instances should reasonably carry it. https://codereview.webrtc.org/2705093002/diff/1/webrtc/voice_engine/channel.c... webrtc/voice_engine/channel.cc:1225: strncpy(lies.plname, format.name.c_str(), RTP_PAYLOAD_NAME_SIZE); On 2017/02/22 10:24:23, ossu wrote: > On 2017/02/21 23:35:04, kwiberg-webrtc wrote: > > The length limiter should also involve sizeof(lies.plname). > > But sizeof(lies.plname) _is_ RTP_PAYLOAD_NAME_SIZE. You know that, and I know that (obviously!), but I have a friend who didn't know this for certain and would have had to look it up, but was too lazy. So I'd still suggest you use sizeof(lies.plname). If you want to use RTP_PAYLOAD_NAME_SIZE, please static_assert that they're the same. https://codereview.webrtc.org/2705093002/diff/1/webrtc/voice_engine/channel.c... webrtc/voice_engine/channel.cc:1228: // carry sample rate, but only clock rate seems sensible to send to the On 2017/02/22 10:24:23, ossu wrote: > On 2017/02/21 23:35:04, kwiberg-webrtc wrote: > > "CodecInst supposedly carries the sample rate," > > Alright. I think this comment is more for the review, rather than being left in. > But maybe nobody really knows which one should be in there? It SeemsToWork(tm) > regardless of the value I choose. If that's the case, then the comment should > remain and I'll rephrase it. Acknowledged.
https://codereview.webrtc.org/2705093002/diff/1/webrtc/voice_engine/channel.cc File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2705093002/diff/1/webrtc/voice_engine/channel.c... webrtc/voice_engine/channel.cc:1225: strncpy(lies.plname, format.name.c_str(), RTP_PAYLOAD_NAME_SIZE); On 2017/02/22 10:42:06, kwiberg-webrtc wrote: > On 2017/02/22 10:24:23, ossu wrote: > > On 2017/02/21 23:35:04, kwiberg-webrtc wrote: > > > The length limiter should also involve sizeof(lies.plname). > > > > But sizeof(lies.plname) _is_ RTP_PAYLOAD_NAME_SIZE. > > You know that, and I know that (obviously!), but I have a friend who didn't know > this for certain and would have had to look it up, but was too lazy. > > So I'd still suggest you use sizeof(lies.plname). If you want to use > RTP_PAYLOAD_NAME_SIZE, please static_assert that they're the same. You're right. I'll use the sizeof instead. I think I just copied this phrasing from somewhere else.
On 2017/02/22 10:26:44, ossu wrote: > On 2017/02/21 23:35:04, kwiberg-webrtc wrote: > > This CL is very large. Is it reasonable to split it into smaller pieces? > > I looked through it several times trying to find a way to do that, but > I was unable to, at the time. It's quite large in the number of > changed lines, but most of those lines are trivial - either just big > blocks removed or mechanical changes of test lines. > > Looking at it again, though, I _think_ I can put the audio_send_stream > and channel changes in their own CL and staggering it before this > one. That should leave mostly webrtcvoiceengine in this one. > > I also noticed changes to peerconnectioninterface that probably > shouldn't be here: they should either be in the first CL, that > actually implements the factory, or the last CL that plumbs it all the > way. I wasn't able to split it at audio_send_stream, but I was able to put the channel changes into their own, now really short, CL and also push the peerconnection stuff earlier. There's now a third CL in the middle of this sequence: https://codereview.webrtc.org/2703373006/ Surprisingly, rietveld seems to have dealt with this just fine, with this latest patch set just containing the changes from that new CL. I usually manage to confuse it when I try something clever. :)
https://codereview.webrtc.org/2705093002/diff/20001/webrtc/audio/audio_send_s... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2705093002/diff/20001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.cc:279: channel_proxy_->SetSendFormat(send_codec_spec.payload_type, I'd like to get to a point when AudioSendStream doesn't create the encoder, but that is taken care of in WVoMC. Perhaps I'm naive but I thought some of the things below (SetBitrate, or anything that calls ModifyEncoder) and which is not dynamic, would be handled by the encoder factory or the encoder's factory method.
https://codereview.webrtc.org/2705093002/diff/20001/webrtc/audio/audio_send_s... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2705093002/diff/20001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.cc:279: channel_proxy_->SetSendFormat(send_codec_spec.payload_type, On 2017/02/22 14:35:01, the sun wrote: > I'd like to get to a point when AudioSendStream doesn't create the encoder, but > that is taken care of in WVoMC. Perhaps I'm naive but I thought some of the > things below (SetBitrate, or anything that calls ModifyEncoder) and which is not > dynamic, would be handled by the encoder factory or the encoder's factory > method. I've not be able to satisfactorily solve the bitrate thing, unfortunately. I was hoping to combine it with other bandwidth related things into a struct that could be set during construction and updated dynamically. Now it looks like we're trying to have the encoder listen directly to the bandwidth estimator, which I believe is good. Then we won't have to do plumbing through several layers each time something in that interface changes. So I've left the bitrate thing for now. I'm not sure how widely it's used. Nothing in our code seems to set AudioCodec.bitrate to anything but 0 or what's taken from AcmCodecDB (which is static). It might be that we can remove this call. The only other ModifyEncoder call is EnableAudioNetworkAdaptor. Since it's still a WIP, I've left it as-is for now. At least it doesn't rely on all the other Setters being called first, in the correct order.
I've moved audio encoder creation into AudioSendStream. I think it looks much neater than before. PTAL. By setting up the whole encoder "stack" directly in AudioSendStream, we bypass a lot of the stateful mayhem inside Channel which, now that I've looked at it, I'm not certain wouldn't try to recreate the codec using its internal machinations in previous CLs. As a bonus, it turns out we don't even need to know the clockrate of the CNG "encoder" when setting it up. It just picks whatever the speech encoder uses. :)
Looks good! https://codereview.webrtc.org/2705093002/diff/40001/webrtc/audio/audio_send_s... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2705093002/diff/40001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.cc:167: if (config_.send_codec_spec.format.name != "") { Is this a test for whether the format is "empty"? In that case, shouldn't it be Optional? https://codereview.webrtc.org/2705093002/diff/40001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.cc:298: std::unique_ptr<AudioEncoder>(new AudioEncoderCng(std::move(config))); encoder.reset(...) would be shorter here. https://codereview.webrtc.org/2705093002/diff/40001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.cc:306: // Audio network adaptor is only allowed for Opus currently. This comment doesn't appear to have any corresponding code. https://codereview.webrtc.org/2705093002/diff/40001/webrtc/audio/audio_send_s... File webrtc/audio/audio_send_stream_unittest.cc (right): https://codereview.webrtc.org/2705093002/diff/40001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream_unittest.cc:61: const SdpAudioFormat kIsacFormat = {"isac", 16000, 1}; The first one can be constexpr. It makes me sad that the second one can't be. https://codereview.webrtc.org/2705093002/diff/40001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream_unittest.cc:71: explicit ConfigHelper(bool audio_bwe_enabled, bool expect_set_encoder_call) Drop "explicit". https://codereview.webrtc.org/2705093002/diff/40001/webrtc/call/audio_send_st... File webrtc/call/audio_send_stream.cc (right): https://codereview.webrtc.org/2705093002/diff/40001/webrtc/call/audio_send_st... webrtc/call/audio_send_stream.cc:59: : format("", 0, 0) {} Why have a default constructor for this type? rtc::Optional<SendCodecSpec> seems like a more natural choice. But we usually default-construct these and then set one field at a time, don't we... ick. https://codereview.webrtc.org/2705093002/diff/40001/webrtc/call/audio_send_st... File webrtc/call/audio_send_stream.h (right): https://codereview.webrtc.org/2705093002/diff/40001/webrtc/call/audio_send_st... webrtc/call/audio_send_stream.h:120: rtc::Optional<int> target_bitrate_bps; Does an unset value mean default bitrate? https://codereview.webrtc.org/2705093002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2705093002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:124: } Broken indentation. Also, isn't there a second parameter map? https://codereview.webrtc.org/2705093002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:996: if (send_codec_spec_.format.name != "") { One of these again...
https://codereview.webrtc.org/2705093002/diff/40001/webrtc/audio/audio_send_s... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2705093002/diff/40001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.cc:298: std::unique_ptr<AudioEncoder>(new AudioEncoderCng(std::move(config))); On 2017/03/01 12:26:47, kwiberg-webrtc wrote: > encoder.reset(...) would be shorter here. Yupp! Will change. https://codereview.webrtc.org/2705093002/diff/40001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.cc:306: // Audio network adaptor is only allowed for Opus currently. On 2017/03/01 12:26:47, kwiberg-webrtc wrote: > This comment doesn't appear to have any corresponding code. Hmm... I agree that it doesn't describe the code that follows, but it does indicate in what circumstance audio_network_adaptor_config could be not false. https://codereview.webrtc.org/2705093002/diff/40001/webrtc/audio/audio_send_s... File webrtc/audio/audio_send_stream_unittest.cc (right): https://codereview.webrtc.org/2705093002/diff/40001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream_unittest.cc:71: explicit ConfigHelper(bool audio_bwe_enabled, bool expect_set_encoder_call) On 2017/03/01 12:26:47, kwiberg-webrtc wrote: > Drop "explicit". Sure. Was in a rush! https://codereview.webrtc.org/2705093002/diff/40001/webrtc/call/audio_send_st... File webrtc/call/audio_send_stream.cc (right): https://codereview.webrtc.org/2705093002/diff/40001/webrtc/call/audio_send_st... webrtc/call/audio_send_stream.cc:59: : format("", 0, 0) {} On 2017/03/01 12:26:47, kwiberg-webrtc wrote: > Why have a default constructor for this type? rtc::Optional<SendCodecSpec> seems > like a more natural choice. > > But we usually default-construct these and then set one field at a time, don't > we... ick. That's the big problem, currently. Eventually, the SendCodecSpec could be created from, say, an SdpAudioFormat and optionally a CNG payload id, and then the rest of the fields could be filled in. In the current implementation, the WebRtcAudioSendStream wrapper uses a SendCodecSpec and gets called repeatedly with different parts of the information, and not necessarily in a reasonable order. I'll fix that at some point, but not right now. https://codereview.webrtc.org/2705093002/diff/40001/webrtc/call/audio_send_st... File webrtc/call/audio_send_stream.h (right): https://codereview.webrtc.org/2705093002/diff/40001/webrtc/call/audio_send_st... webrtc/call/audio_send_stream.h:120: rtc::Optional<int> target_bitrate_bps; On 2017/03/01 12:26:47, kwiberg-webrtc wrote: > Does an unset value mean default bitrate? Yes. I'll add a comment clarifying that. https://codereview.webrtc.org/2705093002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2705093002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:124: } On 2017/03/01 12:26:47, kwiberg-webrtc wrote: > Broken indentation. Also, isn't there a second parameter map? Wow, what happened here? :) I guess you're talking about the feedback params? Yes, there is. It would probably make sense to print them as well. https://codereview.webrtc.org/2705093002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:996: if (send_codec_spec_.format.name != "") { On 2017/03/01 12:26:47, kwiberg-webrtc wrote: > One of these again... Yes. This is the most important one. There are two factors that conspire to this: 1. AudioSendStream should never allow an invalid or nonexistent format in its SendCodecSpec, so an rtc::Optional doesn't make sense for the interface. 2. WebRtcAudioSendStream keeps a SendCodecSpec internally and builds it up during repeated calls to RecreateAudioSendStream. Sometimes, the WebRtcAudioSendStream is created without a valid SendCodecSpec. Clearly, this should be fixed, but not in this CL. For the time being, I'd either have to re-engineer WebRtcAudioSendStream to use a similar, but slightly different format for building up the SendCodecSpec (maybe ok) or I could just leave this check in place for now, and deal with it when cleaning up the way SendStreams are initialized. WDYT?
https://codereview.webrtc.org/2705093002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2705093002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:996: if (send_codec_spec_.format.name != "") { On 2017/03/02 01:30:28, ossu wrote: > On 2017/03/01 12:26:47, kwiberg-webrtc wrote: > > One of these again... > > Yes. This is the most important one. There are two factors that conspire to > this: > 1. AudioSendStream should never allow an invalid or nonexistent format in its > SendCodecSpec, so an rtc::Optional doesn't make sense for the interface. > 2. WebRtcAudioSendStream keeps a SendCodecSpec internally and builds it up > during repeated calls to RecreateAudioSendStream. Sometimes, the > WebRtcAudioSendStream is created without a valid SendCodecSpec. > > Clearly, this should be fixed, but not in this CL. For the time being, I'd > either have to re-engineer WebRtcAudioSendStream to use a similar, but slightly > different format for building up the SendCodecSpec (maybe ok) or I could just > leave this check in place for now, and deal with it when cleaning up the way > SendStreams are initialized. WDYT? Dealing with it when cleaning up sounds OK.
https://codereview.webrtc.org/2705093002/diff/40001/webrtc/audio/audio_send_s... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2705093002/diff/40001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.cc:167: if (config_.send_codec_spec.format.name != "") { nit: use spec.format.name https://codereview.webrtc.org/2705093002/diff/40001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.cc:307: channel_proxy_->EnableAudioNetworkAdaptor( Is it possible to avoid this call to voe::Channel and apply the setting directly to the encoder object? https://codereview.webrtc.org/2705093002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2705093002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:199: static bool ToCodecInst(const AudioCodec& in, webrtc::CodecInst* out) { So this is just needed for tests now? And it is static? If you don't want to update the tests and get rid of it in this CL, could you at least move it into webrtcvoiceengine_unittest.cc?
https://codereview.webrtc.org/2705093002/diff/40001/webrtc/audio/audio_send_s... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2705093002/diff/40001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.cc:167: if (config_.send_codec_spec.format.name != "") { On 2017/03/02 20:37:24, the sun wrote: > nit: use spec.format.name Sure thing! https://codereview.webrtc.org/2705093002/diff/40001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.cc:307: channel_proxy_->EnableAudioNetworkAdaptor( On 2017/03/02 20:37:24, the sun wrote: > Is it possible to avoid this call to voe::Channel and apply the setting directly > to the encoder object? I'll look into it. The AudioEncoder version of this call requires a couple of objects that live within Channel, but I'll see if I can get to them and avoid Channel altogether. https://codereview.webrtc.org/2705093002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2705093002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:199: static bool ToCodecInst(const AudioCodec& in, webrtc::CodecInst* out) { On 2017/03/02 20:37:24, the sun wrote: > So this is just needed for tests now? And it is static? If you don't want to > update the tests and get rid of it in this CL, could you at least move it into > webrtcvoiceengine_unittest.cc? I'll try to clean up the tests, so we won't need this one at all. Wanted to get the functional change up as a patch set so we could have a look at it and decide if it was better than the other way around. I think so.
A few more comments. Haven't dug through the tricky stuff in WVoE/MC yet. https://codereview.webrtc.org/2705093002/diff/40001/webrtc/audio/audio_send_s... File webrtc/audio/audio_send_stream.cc (left): https://codereview.webrtc.org/2705093002/diff/40001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.cc:174: if (channel_proxy_->GetSendCodec(&codec_inst)) { Do you have a separate CL lined up to clean out the unnecessary calls from ChannelProxy/voe::Channel? https://codereview.webrtc.org/2705093002/diff/40001/webrtc/audio/audio_send_s... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2705093002/diff/40001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.cc:270: // without a reasonable config. Is that because they're explicitly setting up the send codec via VoECodec? Can we update the test instead? https://codereview.webrtc.org/2705093002/diff/40001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.cc:279: if (!encoder) { Any chance we could make this a CHECK or DCHECK going forward? WVoMC should already have verified the format is supported, right? https://codereview.webrtc.org/2705093002/diff/40001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.cc:290: // Set the CN payloadtype and the VAD status. Comment is a bit off. https://codereview.webrtc.org/2705093002/diff/40001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.cc:303: // TODO(ossu): The encoder interface wants an RtcEventLogProxy and a Clock, We've decided to use the webrtc/base/timeutils.h API and rely on a global clock. You could either change the encoder(s) to use that, or add an extra method on Channel to return the Clock*... https://codereview.webrtc.org/2705093002/diff/40001/webrtc/audio/audio_send_s... File webrtc/audio/audio_send_stream_unittest.cc (right): https://codereview.webrtc.org/2705093002/diff/40001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream_unittest.cc:185: EXPECT_CALL(*channel_proxy_, SetVADStatus(_)).Times(0); Well, or you could just remove those methods right now... https://codereview.webrtc.org/2705093002/diff/40001/webrtc/call/audio_send_st... File webrtc/call/audio_send_stream.h (right): https://codereview.webrtc.org/2705093002/diff/40001/webrtc/call/audio_send_st... webrtc/call/audio_send_stream.h:118: int payload_type; default init https://codereview.webrtc.org/2705093002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2705093002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:76: // For SendSideBwe, Opus bitrate should be in the range between 6000 and 32000. 32000? https://codereview.webrtc.org/2705093002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1720: // TODO(solenberg): Refactor this logic once we create AudioEncoders here. Comment doesn't apply anymore?
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
Patchset #4 (id:100001) has been deleted
Fixed a bunch of stuff here. Unfortunately, looks like a rebase made it in as well, so some of the changes highlighted are unrelated. https://codereview.webrtc.org/2705093002/diff/40001/webrtc/audio/audio_send_s... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2705093002/diff/40001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.cc:270: // without a reasonable config. On 2017/03/16 08:48:19, the sun wrote: > Is that because they're explicitly setting up the send codec via VoECodec? Can > we update the test instead? After working through it, I realized streams getting created without a proper SendCodecSpec is completely valid: they get created from a call to SetLocalContent but the codec to use isn't decided until negotiation is done and SetRemoteContent gets called. I've made SendCodecSpec optional in lots of places, and we no longer try to setup codecs unless it's properly set, unlike before this CL, where we'd just error out with a warning. https://codereview.webrtc.org/2705093002/diff/40001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.cc:279: if (!encoder) { On 2017/03/16 08:48:19, the sun wrote: > Any chance we could make this a CHECK or DCHECK going forward? WVoMC should > already have verified the format is supported, right? I've rephrased it and made it log an error instead. There might be a case where actually constructing the encoder fails, even though the input is alright. We probably don't want to take the whole app down with us then. Or do we? https://codereview.webrtc.org/2705093002/diff/40001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.cc:290: // Set the CN payloadtype and the VAD status. On 2017/03/16 08:48:19, the sun wrote: > Comment is a bit off. Acknowledged. https://codereview.webrtc.org/2705093002/diff/40001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream.cc:307: channel_proxy_->EnableAudioNetworkAdaptor( On 2017/03/02 20:43:27, ossu wrote: > On 2017/03/02 20:37:24, the sun wrote: > > Is it possible to avoid this call to voe::Channel and apply the setting > directly > > to the encoder object? > > I'll look into it. The AudioEncoder version of this call requires a couple of > objects that live within Channel, but I'll see if I can get to them and avoid > Channel altogether. Turns out avoiding the call through ChannelProxy here was a piece of cake! https://codereview.webrtc.org/2705093002/diff/40001/webrtc/audio/audio_send_s... File webrtc/audio/audio_send_stream_unittest.cc (right): https://codereview.webrtc.org/2705093002/diff/40001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream_unittest.cc:61: const SdpAudioFormat kIsacFormat = {"isac", 16000, 1}; On 2017/03/01 12:26:47, kwiberg-webrtc wrote: > The first one can be constexpr. > > It makes me sad that the second one can't be. Acknowledged. https://codereview.webrtc.org/2705093002/diff/40001/webrtc/audio/audio_send_s... webrtc/audio/audio_send_stream_unittest.cc:185: EXPECT_CALL(*channel_proxy_, SetVADStatus(_)).Times(0); On 2017/03/16 08:48:19, the sun wrote: > Well, or you could just remove those methods right now... Cleaned up these, and probably a few other, unused functions from ChannelProxy. https://codereview.webrtc.org/2705093002/diff/40001/webrtc/call/audio_send_st... File webrtc/call/audio_send_stream.cc (right): https://codereview.webrtc.org/2705093002/diff/40001/webrtc/call/audio_send_st... webrtc/call/audio_send_stream.cc:59: : format("", 0, 0) {} On 2017/03/02 01:30:28, ossu wrote: > On 2017/03/01 12:26:47, kwiberg-webrtc wrote: > > Why have a default constructor for this type? rtc::Optional<SendCodecSpec> > seems > > like a more natural choice. > > > > But we usually default-construct these and then set one field at a time, don't > > we... ick. > > That's the big problem, currently. Eventually, the SendCodecSpec could be > created from, say, an SdpAudioFormat and optionally a CNG payload id, and then > the rest of the fields could be filled in. In the current implementation, the > WebRtcAudioSendStream wrapper uses a SendCodecSpec and gets called repeatedly > with different parts of the information, and not necessarily in a reasonable > order. I'll fix that at some point, but not right now. Seems I didn't really understand the flow previously. Now it seems to me creating a stream before a SendCodecSpec can be constructed is completely reasonable. I've made it optional and made its constructor better. https://codereview.webrtc.org/2705093002/diff/40001/webrtc/call/audio_send_st... File webrtc/call/audio_send_stream.h (right): https://codereview.webrtc.org/2705093002/diff/40001/webrtc/call/audio_send_st... webrtc/call/audio_send_stream.h:118: int payload_type; On 2017/03/16 08:48:19, the sun wrote: > default init It's now required by the constructor. https://codereview.webrtc.org/2705093002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2705093002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:76: // For SendSideBwe, Opus bitrate should be in the range between 6000 and 32000. On 2017/03/16 08:48:19, the sun wrote: > 32000? Yup, seems so. I've just rearranged the code a bit and added an explanatory comment. Perhaps I should remove the comment and move the constants close to their use instead? https://codereview.webrtc.org/2705093002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:199: static bool ToCodecInst(const AudioCodec& in, webrtc::CodecInst* out) { On 2017/03/02 20:43:27, ossu wrote: > On 2017/03/02 20:37:24, the sun wrote: > > So this is just needed for tests now? And it is static? If you don't want to > > update the tests and get rid of it in this CL, could you at least move it into > > webrtcvoiceengine_unittest.cc? > > I'll try to clean up the tests, so we won't need this one at all. Wanted to get > the functional change up as a patch set so we could have a look at it and decide > if it was better than the other way around. I think so. Cleaned up (or plain removed) all remaining tests using it. Turns out it's still needed until https://codereview.webrtc.org/2686043006/ lands. I guess whoever lands last gets to remove this function and all it depends on. :) https://codereview.webrtc.org/2705093002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:996: if (send_codec_spec_.format.name != "") { On 2017/03/02 02:17:55, kwiberg-webrtc wrote: > On 2017/03/02 01:30:28, ossu wrote: > > On 2017/03/01 12:26:47, kwiberg-webrtc wrote: > > > One of these again... > > > > Yes. This is the most important one. There are two factors that conspire to > > this: > > 1. AudioSendStream should never allow an invalid or nonexistent format in its > > SendCodecSpec, so an rtc::Optional doesn't make sense for the interface. > > 2. WebRtcAudioSendStream keeps a SendCodecSpec internally and builds it up > > during repeated calls to RecreateAudioSendStream. Sometimes, the > > WebRtcAudioSendStream is created without a valid SendCodecSpec. > > > > Clearly, this should be fixed, but not in this CL. For the time being, I'd > > either have to re-engineer WebRtcAudioSendStream to use a similar, but > slightly > > different format for building up the SendCodecSpec (maybe ok) or I could just > > leave this check in place for now, and deal with it when cleaning up the way > > SendStreams are initialized. WDYT? > > Dealing with it when cleaning up sounds OK. SendCodecSpec is now optional. No more string comparison! https://codereview.webrtc.org/2705093002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1720: // TODO(solenberg): Refactor this logic once we create AudioEncoders here. On 2017/03/16 08:48:19, the sun wrote: > Comment doesn't apply anymore? Acknowledged. https://codereview.webrtc.org/2705093002/diff/40001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2705093002/diff/40001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:3211: #ifdef WEBRTC_CODEC_OPUS Turns out these #ifdef'ed tests never activated, because those flags aren't defined for the test. We've got this covered in other tests now, and keeping them here would be wrong in the long run. I've just removed them.
Mostly nits, and one concern. https://codereview.webrtc.org/2705093002/diff/120001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2705093002/diff/120001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:86: LOG(LS_ERROR) << "Failed to set up send codec state."; nit: indent off https://codereview.webrtc.org/2705093002/diff/120001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:293: if (spec.cng_payload_type != -1) { Is it time to make cng_payload_type an Optional? https://codereview.webrtc.org/2705093002/diff/120001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:302: channel_proxy_->SetEncoder(spec.payload_type, std::move(encoder)); This is so great - we won't have the half-setup states anymore, but only set the new codec once we know ALL configuration was successful! But, previously we would only reconfigure the channel's AudioEncoder object, whereas now we're creating a complete new one for each new AudioSendStream. Are we losing state? How will that work with hardware encoders? I think this may have been the reason we originally envisioned WVoMC creating the encoder and passing it down to the stream. https://codereview.webrtc.org/2705093002/diff/120001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2705093002/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1692: for (const AudioCodec& voice_codec : codecs) { voice_codec -> audio_codec? https://codereview.webrtc.org/2705093002/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1698: CodecParameterMap(voice_codec.params)); Is there any parsing going on in CodecParameterMap() which may fail? https://codereview.webrtc.org/2705093002/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1700: voice_codec_info = engine()->encoder_factory_->QueryAudioEncoder(format); voice_codec_info -> audio_codec_info?
https://codereview.webrtc.org/2705093002/diff/120001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2705093002/diff/120001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:86: LOG(LS_ERROR) << "Failed to set up send codec state."; On 2017/03/20 20:17:25, the sun wrote: > nit: indent off Acknowledged. https://codereview.webrtc.org/2705093002/diff/120001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:293: if (spec.cng_payload_type != -1) { On 2017/03/20 20:17:25, the sun wrote: > Is it time to make cng_payload_type an Optional? Yeah, why not? It's clearer imo. -1 is just HowItUsedToBe(tm). https://codereview.webrtc.org/2705093002/diff/120001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:302: channel_proxy_->SetEncoder(spec.payload_type, std::move(encoder)); On 2017/03/20 20:17:25, the sun wrote: > This is so great - we won't have the half-setup states anymore, but only set the > new codec once we know ALL configuration was successful! > > But, previously we would only reconfigure the channel's AudioEncoder object, > whereas now we're creating a complete new one for each new AudioSendStream. Are > we losing state? How will that work with hardware encoders? I think this may > have been the reason we originally envisioned WVoMC creating the encoder and > passing it down to the stream. Hmm, I see what you mean. I'll have a look at moving creation into WebRtcAudioSendStream or possibly stop reconstructing AudioSendStreams. Regardless of where we create it, with the ACM taking ownership of the AudioEncoder object, we still can't reliably hang on to it outside of the ACM. Maybe we should add some checks to not reconstruct the encoder object unless the SendCodecSpec has actually changed. Perhaps even allow unwrapping and rewrapping the encoder with CNG, if that's the only thing that changed. I have a couple of TODOs about not reconstructing the stream when we change the target bitrate. I should probably address those as well. https://codereview.webrtc.org/2705093002/diff/120001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2705093002/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1692: for (const AudioCodec& voice_codec : codecs) { On 2017/03/20 20:17:25, the sun wrote: > voice_codec -> audio_codec? I wanted to separate the main codec from auxiliary ones, like CN and DTMF. Maybe speech_codec or main_codec or the_real_proper_codec_you_know is a better name? https://codereview.webrtc.org/2705093002/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1698: CodecParameterMap(voice_codec.params)); On 2017/03/20 20:17:25, the sun wrote: > Is there any parsing going on in CodecParameterMap() which may fail? Nope, it's just a copy. When I started this, SdpAudioFormat took only an rvalue-reference to its params, so they had to be explicitly copied. Now it takes a const&, so I can remove the explicit CodecParameterMap construction.
I'm out of things to complain about, I think... https://codereview.webrtc.org/2705093002/diff/120001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2705093002/diff/120001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:293: if (spec.cng_payload_type != -1) { On 2017/03/21 14:53:25, ossu wrote: > On 2017/03/20 20:17:25, the sun wrote: > > Is it time to make cng_payload_type an Optional? > > Yeah, why not? It's clearer imo. -1 is just HowItUsedToBe(tm). +1 https://codereview.webrtc.org/2705093002/diff/120001/webrtc/video/video_quali... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/2705093002/diff/120001/webrtc/video/video_quali... webrtc/video/video_quality_test.cc:1711: // TODO(ossu): "Add stereo here?" Probably. Looks like it was stereo before.
https://codereview.webrtc.org/2705093002/diff/120001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2705093002/diff/120001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:302: channel_proxy_->SetEncoder(spec.payload_type, std::move(encoder)); On 2017/03/21 14:53:25, ossu wrote: > On 2017/03/20 20:17:25, the sun wrote: > > This is so great - we won't have the half-setup states anymore, but only set > the > > new codec once we know ALL configuration was successful! > > > > But, previously we would only reconfigure the channel's AudioEncoder object, > > whereas now we're creating a complete new one for each new AudioSendStream. > Are > > we losing state? How will that work with hardware encoders? I think this may > > have been the reason we originally envisioned WVoMC creating the encoder and > > passing it down to the stream. > > Hmm, I see what you mean. I'll have a look at moving creation into > WebRtcAudioSendStream or possibly stop reconstructing AudioSendStreams. > > Regardless of where we create it, with the ACM taking ownership of the > AudioEncoder object, we still can't reliably hang on to it outside of the ACM. > Maybe we should add some checks to not reconstruct the encoder object unless the > SendCodecSpec has actually changed. Perhaps even allow unwrapping and rewrapping > the encoder with CNG, if that's the only thing that changed. > > I have a couple of TODOs about not reconstructing the stream when we change the > target bitrate. I should probably address those as well. I have now looked at it and I cannot move the construction of the AudioEncoder higher without breaking at least one design constraint: - From reading the design docs on VoE2 and the Call API, it's been explicitly designed to recreate the internal streams, rather than reconfigure them. The internal implementation of AudioSendStream is explicitly hidden from view by having call return its base class webrtc::AudioSendStream. - VoiceEngine as it looks today takes ownership of the encoder sent to it. Looking at the video side of Call, it seems designed and implemented much the same, with streams getting reconstructed and encoders reconstructed with the streams. What they do differently, though, is to gather all the changes caused by SetSendParameters before reconstructing the stream. We don't, causing our encoder to get reconstructed a couple of times in quick succession. So without major redesign, which I'd consider outside the scope of this CL, I believe encoder construction is where it should be. I will, however, take a look at restructuring SetSendParameters a bit, so we don't end up reconstructing the stream more than once during that call.
On 2017/03/22 13:28:32, ossu wrote: > https://codereview.webrtc.org/2705093002/diff/120001/webrtc/audio/audio_send_... > File webrtc/audio/audio_send_stream.cc (right): > > https://codereview.webrtc.org/2705093002/diff/120001/webrtc/audio/audio_send_... > webrtc/audio/audio_send_stream.cc:302: > channel_proxy_->SetEncoder(spec.payload_type, std::move(encoder)); > On 2017/03/21 14:53:25, ossu wrote: > > On 2017/03/20 20:17:25, the sun wrote: > > > This is so great - we won't have the half-setup states anymore, but only set > > the > > > new codec once we know ALL configuration was successful! > > > > > > But, previously we would only reconfigure the channel's AudioEncoder object, > > > whereas now we're creating a complete new one for each new AudioSendStream. > > Are > > > we losing state? How will that work with hardware encoders? I think this may > > > have been the reason we originally envisioned WVoMC creating the encoder and > > > passing it down to the stream. > > > > Hmm, I see what you mean. I'll have a look at moving creation into > > WebRtcAudioSendStream or possibly stop reconstructing AudioSendStreams. > > > > Regardless of where we create it, with the ACM taking ownership of the > > AudioEncoder object, we still can't reliably hang on to it outside of the ACM. > > Maybe we should add some checks to not reconstruct the encoder object unless > the > > SendCodecSpec has actually changed. Perhaps even allow unwrapping and > rewrapping > > the encoder with CNG, if that's the only thing that changed. > > > > I have a couple of TODOs about not reconstructing the stream when we change > the > > target bitrate. I should probably address those as well. > > I have now looked at it and I cannot move the construction of the AudioEncoder > higher without breaking at least one design constraint: > - From reading the design docs on VoE2 and the Call API, it's been explicitly > designed to recreate the internal streams, rather than reconfigure them. The > internal implementation of AudioSendStream is explicitly hidden from view by > having call return its base class webrtc::AudioSendStream. > - VoiceEngine as it looks today takes ownership of the encoder sent to it. You mean voe::Channel? > > Looking at the video side of Call, it seems designed and implemented much the > same, with streams getting reconstructed and encoders reconstructed with the > streams. What they do differently, though, is to gather all the changes caused > by SetSendParameters before reconstructing the stream. We don't, causing our > encoder to get reconstructed a couple of times in quick succession. Yes, the goal is of course not to do that. > So without major redesign, which I'd consider outside the scope of this CL, I > believe encoder construction is where it should be. Ok, will that cause us any problems, when encoder state is discarded? Say, when the SDP is renegotiated during a call? > I will, however, take a look at restructuring SetSendParameters a bit, so we > don't end up reconstructing the stream more than once during that call. Agree on the approach. Which needs to come first, to maintain current behavior WRT offer/answer handling?
On 2017/03/22 14:05:01, the sun wrote: > On 2017/03/22 13:28:32, ossu wrote: > > > https://codereview.webrtc.org/2705093002/diff/120001/webrtc/audio/audio_send_... > > File webrtc/audio/audio_send_stream.cc (right): > > > > > https://codereview.webrtc.org/2705093002/diff/120001/webrtc/audio/audio_send_... > > webrtc/audio/audio_send_stream.cc:302: > > channel_proxy_->SetEncoder(spec.payload_type, std::move(encoder)); > > On 2017/03/21 14:53:25, ossu wrote: > > > On 2017/03/20 20:17:25, the sun wrote: > > > > This is so great - we won't have the half-setup states anymore, but only > set > > > the > > > > new codec once we know ALL configuration was successful! > > > > > > > > But, previously we would only reconfigure the channel's AudioEncoder > object, > > > > whereas now we're creating a complete new one for each new > AudioSendStream. > > > Are > > > > we losing state? How will that work with hardware encoders? I think this > may > > > > have been the reason we originally envisioned WVoMC creating the encoder > and > > > > passing it down to the stream. > > > > > > Hmm, I see what you mean. I'll have a look at moving creation into > > > WebRtcAudioSendStream or possibly stop reconstructing AudioSendStreams. > > > > > > Regardless of where we create it, with the ACM taking ownership of the > > > AudioEncoder object, we still can't reliably hang on to it outside of the > ACM. > > > Maybe we should add some checks to not reconstruct the encoder object unless > > the > > > SendCodecSpec has actually changed. Perhaps even allow unwrapping and > > rewrapping > > > the encoder with CNG, if that's the only thing that changed. > > > > > > I have a couple of TODOs about not reconstructing the stream when we change > > the > > > target bitrate. I should probably address those as well. > > > > I have now looked at it and I cannot move the construction of the AudioEncoder > > higher without breaking at least one design constraint: > > - From reading the design docs on VoE2 and the Call API, it's been explicitly > > designed to recreate the internal streams, rather than reconfigure them. The > > internal implementation of AudioSendStream is explicitly hidden from view by > > having call return its base class webrtc::AudioSendStream. > > - VoiceEngine as it looks today takes ownership of the encoder sent to it. > > You mean voe::Channel? I meant AudioCodingModule, so ... yes? Sort-of? :) > > > > Looking at the video side of Call, it seems designed and implemented much the > > same, with streams getting reconstructed and encoders reconstructed with the > > streams. What they do differently, though, is to gather all the changes caused > > by SetSendParameters before reconstructing the stream. We don't, causing our > > encoder to get reconstructed a couple of times in quick succession. > > Yes, the goal is of course not to do that. > > > So without major redesign, which I'd consider outside the scope of this CL, I > > believe encoder construction is where it should be. > > Ok, will that cause us any problems, when encoder state is discarded? Say, when > the SDP is renegotiated during a call? I don't know. I can hand-wave a bit about it probably not happening and probably being ok, but I can't say for sure. It probably depends on which codec it is and how large the frames are: Since we stuff 10 ms at a time into the encoder, it has to buffer audio data internally to get enough for a whole frame. Even if the encoder has no other internal state, I believe that data will be discarded on renegotiation. The same thing would happen if renegotiation chose a different codec. Currently, the stream gets recreated when: - The set of RTP extensions changes - If the SendCodecSpec changes (slightly more than before, since we used to compare CodecInsts and they ignore most fmtp changes, also we don't rewrap the encoder when moving to/from VAD) - If the target_bitrate_bps changes (I've a TODO to fix this) - If the ANA configuration changes. VideoSendStream has a ReconfigureVideoEncoder() in its public interface. I could create something similar for us, updating the encoder on ANA configuration change, target_bitrate_bps change and SendCodecSpec change. It could selectively recreate the encoder or just reconfigure it, depending on what changed in the Config. Though, at this point, I might just add a general Reconfigure call to AudioSendStream. Looking at Call::Create/DestroyAudioSendStream, this looks like it could work unless the SSRC changed, which we already disallow in WebRtcAudioSendStream. Then any options that don't require replacing the encoder could be applied in one fell swoop, without recreating the encoder. If I go down that route, then we've reached the point where AudioSendStreams are no longer immutable. They are, however, configurable using a single call, rather than the myriad setters on Channel. Perhaps that's the important aspect? To me, Reconfigure(const AudioSendStream::Config&) seems like a tempting concept. Perhaps we could eventually allow the AudioEncoder itself to Reconfigure()? > > I will, however, take a look at restructuring SetSendParameters a bit, so we > > don't end up reconstructing the stream more than once during that call. > > Agree on the approach. Which needs to come first, to maintain current behavior > WRT offer/answer handling? I'm not sure I understand the question; which of what set of options needs to come first? Is current behavior WRT offer/answer handling == not reconstruct the encoder unless absolutely necessary? How often do we renegotiate something in the list above, that would inadvertently recreate the encoder?
On 2017/03/22 15:31:01, ossu wrote: > On 2017/03/22 14:05:01, the sun wrote: > > On 2017/03/22 13:28:32, ossu wrote: > > > > > > https://codereview.webrtc.org/2705093002/diff/120001/webrtc/audio/audio_send_... > > > File webrtc/audio/audio_send_stream.cc (right): > > > > > > > > > https://codereview.webrtc.org/2705093002/diff/120001/webrtc/audio/audio_send_... > > > webrtc/audio/audio_send_stream.cc:302: > > > channel_proxy_->SetEncoder(spec.payload_type, std::move(encoder)); > > > On 2017/03/21 14:53:25, ossu wrote: > > > > On 2017/03/20 20:17:25, the sun wrote: > > > > > This is so great - we won't have the half-setup states anymore, but only > > set > > > > the > > > > > new codec once we know ALL configuration was successful! > > > > > > > > > > But, previously we would only reconfigure the channel's AudioEncoder > > object, > > > > > whereas now we're creating a complete new one for each new > > AudioSendStream. > > > > Are > > > > > we losing state? How will that work with hardware encoders? I think this > > may > > > > > have been the reason we originally envisioned WVoMC creating the encoder > > and > > > > > passing it down to the stream. > > > > > > > > Hmm, I see what you mean. I'll have a look at moving creation into > > > > WebRtcAudioSendStream or possibly stop reconstructing AudioSendStreams. > > > > > > > > Regardless of where we create it, with the ACM taking ownership of the > > > > AudioEncoder object, we still can't reliably hang on to it outside of the > > ACM. > > > > Maybe we should add some checks to not reconstruct the encoder object > unless > > > the > > > > SendCodecSpec has actually changed. Perhaps even allow unwrapping and > > > rewrapping > > > > the encoder with CNG, if that's the only thing that changed. > > > > > > > > I have a couple of TODOs about not reconstructing the stream when we > change > > > the > > > > target bitrate. I should probably address those as well. > > > > > > I have now looked at it and I cannot move the construction of the > AudioEncoder > > > higher without breaking at least one design constraint: > > > - From reading the design docs on VoE2 and the Call API, it's been > explicitly > > > designed to recreate the internal streams, rather than reconfigure them. The > > > internal implementation of AudioSendStream is explicitly hidden from view by > > > having call return its base class webrtc::AudioSendStream. > > > - VoiceEngine as it looks today takes ownership of the encoder sent to it. > > > > You mean voe::Channel? > > I meant AudioCodingModule, so ... yes? Sort-of? :) > > > > > > > Looking at the video side of Call, it seems designed and implemented much > the > > > same, with streams getting reconstructed and encoders reconstructed with the > > > streams. What they do differently, though, is to gather all the changes > caused > > > by SetSendParameters before reconstructing the stream. We don't, causing our > > > encoder to get reconstructed a couple of times in quick succession. > > > > Yes, the goal is of course not to do that. > > > > > So without major redesign, which I'd consider outside the scope of this CL, > I > > > believe encoder construction is where it should be. > > > > Ok, will that cause us any problems, when encoder state is discarded? Say, > when > > the SDP is renegotiated during a call? > > I don't know. I can hand-wave a bit about it probably not happening and probably > being ok, but I can't say for sure. It probably depends on which codec it is and > how large the frames are: Since we stuff 10 ms at a time into the encoder, it > has to buffer audio data internally to get enough for a whole frame. Even if the > encoder has no other internal state, I believe that data will be discarded on > renegotiation. The same thing would happen if renegotiation chose a different > codec. > > Currently, the stream gets recreated when: > - The set of RTP extensions changes > - If the SendCodecSpec changes (slightly more than before, since we used to > compare CodecInsts and they ignore most fmtp changes, also we don't rewrap the > encoder when moving to/from VAD) > - If the target_bitrate_bps changes (I've a TODO to fix this) > - If the ANA configuration changes. > > VideoSendStream has a ReconfigureVideoEncoder() in its public interface. I could > create something similar for us, updating the encoder on ANA configuration > change, target_bitrate_bps change and SendCodecSpec change. It could selectively > recreate the encoder or just reconfigure it, depending on what changed in the > Config. Though, at this point, I might just add a general Reconfigure call to > AudioSendStream. Looking at Call::Create/DestroyAudioSendStream, this looks like > it could work unless the SSRC changed, which we already disallow in > WebRtcAudioSendStream. Then any options that don't require replacing the encoder > could be applied in one fell swoop, without recreating the encoder. > > If I go down that route, then we've reached the point where AudioSendStreams are > no longer immutable. They are, however, configurable using a single call, rather > than the myriad setters on Channel. Perhaps that's the important aspect? To me, > Reconfigure(const AudioSendStream::Config&) seems like a tempting concept. > Perhaps we could eventually allow the AudioEncoder itself to Reconfigure()? I agree the important aspect is to have as few ways to mutate AudioSendStream as possible. Sure, perhaps a Reconfigure() method is the best solution (that would allow us to move away from the intermediate WebRtcAudioSendStream). But if we do that I'd like as little as possible to be reconfigurable (perhaps that is everything except SSRC though - I don't know OTOH). So then IIUC AudioSendStream would create and own the encoder, lend it to Channel/ACM, and do as little as possible when the config changes? > > > > I will, however, take a look at restructuring SetSendParameters a bit, so we > > > don't end up reconstructing the stream more than once during that call. > > > > Agree on the approach. Which needs to come first, to maintain current behavior > > WRT offer/answer handling? > > I'm not sure I understand the question; which of what set of options needs to > come first? > Is current behavior WRT offer/answer handling == not reconstruct the encoder > unless absolutely necessary? > How often do we renegotiate something in the list above, that would > inadvertently recreate the encoder?
Made AudioSendStream reconfigurable. Also addressed a few nits and ran git cl format. Next patch set will be a big-ole rebase, so get it while it's hot! https://codereview.webrtc.org/2705093002/diff/120001/webrtc/video/video_quali... File webrtc/video/video_quality_test.cc (right): https://codereview.webrtc.org/2705093002/diff/120001/webrtc/video/video_quali... webrtc/video/video_quality_test.cc:1711: // TODO(ossu): "Add stereo here?" On 2017/03/22 00:00:45, kwiberg-webrtc wrote: > Probably. Looks like it was stereo before. Agreed. https://codereview.webrtc.org/2705093002/diff/140001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2705093002/diff/140001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:494: if (new_config.min_bitrate_bps != -1 && new_config.max_bitrate_bps != -1) { Do I need to check if we're sending here first? The bitrate observer is usually configured just as we're about to start sending. Is it important that it's not active for a long duration without us also sending? https://codereview.webrtc.org/2705093002/diff/140001/webrtc/call/audio_send_s... File webrtc/call/audio_send_stream.cc (right): https://codereview.webrtc.org/2705093002/diff/140001/webrtc/call/audio_send_s... webrtc/call/audio_send_stream.cc:60: int payload_type, git cl format did this. https://codereview.webrtc.org/2705093002/diff/140001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2705093002/diff/140001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:1163: TEST_F(WebRtcVoiceEngineTestFake, DontRecreateSendStream) { Also added an AudioSendStream test to check that the encoder doesn't get unnecessarily recreated on Reconfigure. https://codereview.webrtc.org/2705093002/diff/140001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/2705093002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:173: virtual void ModifyEncoder( Needed to plumb this all the way out. Once we get rid of these extra layers, we can do these calls directly on the encoder object instead.
As usual, a lot to take in. Here are a few initial comments. https://codereview.webrtc.org/2705093002/diff/140001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2705093002/diff/140001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:48: } nit: // namespace https://codereview.webrtc.org/2705093002/diff/140001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:71: VoiceEngineImpl* voe_impl = static_cast<VoiceEngineImpl*>(voice_engine()); Can you call Reconfigure() from here (or the some internal non-virtual helper which both calls) to avoid code duplication? https://codereview.webrtc.org/2705093002/diff/140001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:114: void AudioSendStream::Reconfigure( Many of the lazy-updated attributes here amount to very little work inside voe::Channel: SetRTCP_CNAME() - a string copy DeRegisterExternalTransport() - a memory write RegisterExternalTransport(new_config.send_transport) - a memory write SetSendAudioLevelIndicationStatus() - 4 memory writes to arrays ... I think we should make this code as simple as possible now and optimize later, and where the optimizations should really be done - in voe::Channel and the RtpRtcp module. https://codereview.webrtc.org/2705093002/diff/140001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:134: // RFC 5285: Each distinct extension MUST have a unique ID. The value 0 is This is already guaranteed by WVoMC::SetSendParameters(). Treat that as the API layer and just DCHECK in this layer. https://codereview.webrtc.org/2705093002/diff/140001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.h (right): https://codereview.webrtc.org/2705093002/diff/140001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.h:48: void Reconfigure(const webrtc::AudioSendStream::Config& config) override; nit: looks like it should go under the below comment
https://codereview.webrtc.org/2705093002/diff/140001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2705093002/diff/140001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:43: [&lambda](std::unique_ptr<AudioEncoder>* encoder_ptr) { Or just [&]. That way, you make it very plain that the lambda is run immediately, since capturing everything is very forbidden if the lambda outlives the current scope. https://codereview.webrtc.org/2705093002/diff/140001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:48: } On 2017/04/04 23:02:54, the sun wrote: > nit: // namespace clang-format will fix this for you nowadays... https://codereview.webrtc.org/2705093002/diff/140001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:464: auto sub_encoders = old_encoder->ReclaimContainedEncoders(); Eugh. I'm having second thoughts about having this method in AudioEncoder. I *really* wish we could use dynamic_cast instead... How about subclassing AudioEncoder, and putting a corresponding method in that subclass instead? Such a subclass could promise to have exactly one sub-encoder, which would make life much easier. It could also be non-public. For the case of no CN, we'd need a forwarding wrapper, though. We could avoid that by making a separate interface for the reclamation function, but then we have to tackle lifetime issues instead. https://codereview.webrtc.org/2705093002/diff/140001/webrtc/call/audio_send_s... File webrtc/call/audio_send_stream.cc (right): https://codereview.webrtc.org/2705093002/diff/140001/webrtc/call/audio_send_s... webrtc/call/audio_send_stream.cc:69: ss << ", cng_payload_type: " << (cng_payload_type ? *cng_payload_type : -1); cng_payload_type.value_or(-1)
https://codereview.webrtc.org/2705093002/diff/140001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2705093002/diff/140001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:48: } On 2017/04/04 23:02:54, the sun wrote: > nit: // namespace Acknowledged. https://codereview.webrtc.org/2705093002/diff/140001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:71: VoiceEngineImpl* voe_impl = static_cast<VoiceEngineImpl*>(voice_engine()); On 2017/04/04 23:02:53, the sun wrote: > Can you call Reconfigure() from here (or the some internal non-virtual helper > which both calls) to avoid code duplication? I considered it but I don't think I'll be able to make it more readable while trying to avoid duplication. For proper operation, I'd need to make config_ optional, so that Reconfigure could configure from nothing, as well as from some other state. I'm not sure it's worth it considering most of this is a stop-gap before we can stop using Channel altogether. If you think it's important, I'll take another look at it. https://codereview.webrtc.org/2705093002/diff/140001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:114: void AudioSendStream::Reconfigure( On 2017/04/04 23:02:53, the sun wrote: > Many of the lazy-updated attributes here amount to very little work inside > voe::Channel: > SetRTCP_CNAME() - a string copy > DeRegisterExternalTransport() - a memory write > RegisterExternalTransport(new_config.send_transport) - a memory write > SetSendAudioLevelIndicationStatus() - 4 memory writes to arrays > ... > > I think we should make this code as simple as possible now and optimize later, > and where the optimizations should really be done - in voe::Channel and the > RtpRtcp module. Very little work, and locks. Do you want me to remove the checks for change before the calls? What would that accomplish, other than us taking a handful of unnecessary locks? https://codereview.webrtc.org/2705093002/diff/140001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:134: // RFC 5285: Each distinct extension MUST have a unique ID. The value 0 is On 2017/04/04 23:02:54, the sun wrote: > This is already guaranteed by WVoMC::SetSendParameters(). Treat that as the API > layer and just DCHECK in this layer. I've added it as an explanation to why I can use 0 as a "not configured" value here. I'll make that clearer in the comment.
https://codereview.webrtc.org/2705093002/diff/140001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2705093002/diff/140001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:43: [&lambda](std::unique_ptr<AudioEncoder>* encoder_ptr) { On 2017/04/06 10:13:29, kwiberg-webrtc wrote: > Or just [&]. That way, you make it very plain that the lambda is run > immediately, since capturing everything is very forbidden if the lambda outlives > the current scope. I'm all for [&] capture. In this case, capturing the lambda by reference is just as forbidden if it outlives the current scope. I'll shorten it. https://codereview.webrtc.org/2705093002/diff/140001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:464: auto sub_encoders = old_encoder->ReclaimContainedEncoders(); On 2017/04/06 10:13:29, kwiberg-webrtc wrote: > Eugh. I'm having second thoughts about having this method in AudioEncoder. I > *really* wish we could use dynamic_cast instead... > > How about subclassing AudioEncoder, and putting a corresponding method in that > subclass instead? Such a subclass could promise to have exactly one sub-encoder, > which would make life much easier. It could also be non-public. > > For the case of no CN, we'd need a forwarding wrapper, though. We could avoid > that by making a separate interface for the reclamation function, but then we > have to tackle lifetime issues instead. Yeah, it's a bit nasty, but slightly less so in this case, since we know there'd be exactly one level of nesting. I think we should eventually move away from wrapping encoders like this, instead driving the components separately, perhaps with VAD extracted from AudioEncoderCNG and working as a switch between the two. Regardless, I'm not going to redo our AudioEncoder hierarchy in this CL. I'd love to do it later, though! :) https://codereview.webrtc.org/2705093002/diff/140001/webrtc/call/audio_send_s... File webrtc/call/audio_send_stream.cc (right): https://codereview.webrtc.org/2705093002/diff/140001/webrtc/call/audio_send_s... webrtc/call/audio_send_stream.cc:69: ss << ", cng_payload_type: " << (cng_payload_type ? *cng_payload_type : -1); On 2017/04/06 10:13:29, kwiberg-webrtc wrote: > cng_payload_type.value_or(-1) I don't know why I didn't think of that. Thanks!
https://codereview.webrtc.org/2705093002/diff/140001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2705093002/diff/140001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:114: void AudioSendStream::Reconfigure( On 2017/04/06 10:15:21, ossu wrote: > On 2017/04/04 23:02:53, the sun wrote: > > Many of the lazy-updated attributes here amount to very little work inside > > voe::Channel: > > SetRTCP_CNAME() - a string copy > > DeRegisterExternalTransport() - a memory write > > RegisterExternalTransport(new_config.send_transport) - a memory write > > SetSendAudioLevelIndicationStatus() - 4 memory writes to arrays > > ... > > > > I think we should make this code as simple as possible now and optimize later, > > and where the optimizations should really be done - in voe::Channel and the > > RtpRtcp module. > > Very little work, and locks. > > Do you want me to remove the checks for change before the calls? What would that > accomplish, other than us taking a handful of unnecessary locks? I think avoiding the duplicated code between here and the ctor wins in this case. To me it looks like you could basically take the code in the ctor and put it here, then call Reconfigure() from the ctor, and that'd be it. (In reality you'll need to put it in a utility function called from both places, because of the issues of calling virtual functions from the ctor.) Poof, 70 lines gone. And we take as many locks as we do now, although quite rarely.
https://codereview.webrtc.org/2705093002/diff/160001/webrtc/media/BUILD.gn File webrtc/media/BUILD.gn (right): https://codereview.webrtc.org/2705093002/diff/160001/webrtc/media/BUILD.gn#ne... webrtc/media/BUILD.gn:46: "../modules/audio_coding:audio_encoder_factory_interface", Why isn't there anything else in this deps array? Looks like your additions should be down with the others, line 102-111? https://codereview.webrtc.org/2705093002/diff/160001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2705093002/diff/160001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:40: #include "webrtc/modules/audio_coding/acm2/rent_a_codec.h" remove?
Description was changed from ========== Injectable audio encoders: WebRtcVoiceEngine and company These are the changes made to WebRtcVoiceEngine and surrounding code. It still contains some things that are inelegant, like how AudioCodecSpec and AudioFormatInfo is ferried around in SendCodecSpec. This should probably be resolved before landing. There are also a few test still that are disabled. They should be removed or fixed, as the case may be. I've put this CL up to get a better overview of the changes made and how reviewable they are. BUG=webrtc:5806 ========== to ========== Injectable audio encoders: WebRtcVoiceEngine and company These are the changes made to WebRtcVoiceEngine and surrounding code. It still contains some things that are inelegant, like how AudioCodecSpec and AudioFormatInfo is ferried around in SendCodecSpec. This should probably be resolved before landing. There are also a few test still that are disabled. They should be removed or fixed, as the case may be. I've put this CL up to get a better overview of the changes made and how reviewable they are. BUG=webrtc:5806 ==========
Uploaded a new patch set on Friday, but forgot to send an email about it. In it, I moved the stuff duplicated in Reconfigure and the constructor to just Reconfigure (and renamed it ConfigureStream). I also made all the reconfiguring calls static, so it'd be more difficult to accidentally reference config_ (which is the old settings). https://codereview.webrtc.org/2705093002/diff/160001/webrtc/media/BUILD.gn File webrtc/media/BUILD.gn (right): https://codereview.webrtc.org/2705093002/diff/160001/webrtc/media/BUILD.gn#ne... webrtc/media/BUILD.gn:46: "../modules/audio_coding:audio_encoder_factory_interface", On 2017/04/07 11:52:56, the sun wrote: > Why isn't there anything else in this deps array? Looks like your additions > should be down with the others, line 102-111? Probably an ocular failure on my behalf. I'll move 'em! https://codereview.webrtc.org/2705093002/diff/160001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2705093002/diff/160001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:40: #include "webrtc/modules/audio_coding/acm2/rent_a_codec.h" On 2017/04/07 11:52:56, the sun wrote: > remove? Oh, yes! https://codereview.webrtc.org/2705093002/diff/180001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2705093002/diff/180001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:551: RTC_DCHECK_GE(max_bitrate_bps, min_bitrate_bps); I believe config_.min_bitrate_bps and max_bitrate_bps may have to be updated here, since adding the observer may cause an immediate callback to be called, which does checks against at least config_.min_bitrate_bps.
lgtm
minyue@webrtc.org changed reviewers: + minyue@webrtc.org
The configure/reconfigure logic makes me worried. I'd prefer to make it simpler, trading away some efficiency and make more of the conditional calls on Channel/AudioEncoder unconditional. What you're doing here is essentially caching, which very easily breaks down. https://codereview.webrtc.org/2705093002/diff/180001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2705093002/diff/180001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:113: if (old_config.rtp.ssrc != new_config.rtp.ssrc) { Refresh my memory - is SSRC=0 valid or not? If it is, this condition will cause some machinery to not be configured (see e.g. ModuleRtpRtcpImpl::SetRtcpReceiverSsrcs()). https://codereview.webrtc.org/2705093002/diff/180001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:127: if (!is_configured || This is_configured flag is used inconsistently in this function. Is that intended? https://codereview.webrtc.org/2705093002/diff/180001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:159: if (!is_configured || new_ids.audio_level != old_ids.audio_level) { IIUC "!is_configured" is unnecessary here since the old ids will be 0 the first time? https://codereview.webrtc.org/2705093002/diff/180001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:474: if (new_config.audio_network_adaptor_config == You must also do the below if SetupSendCodec() has created a new codec, right? Goes for all these functions, I believe. https://codereview.webrtc.org/2705093002/diff/180001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:506: stream->channel_proxy_->ModifyEncoder( CallEncoder? https://codereview.webrtc.org/2705093002/diff/180001/webrtc/test/mock_voe_cha... File webrtc/test/mock_voe_channel_proxy.h (right): https://codereview.webrtc.org/2705093002/diff/180001/webrtc/test/mock_voe_cha... webrtc/test/mock_voe_channel_proxy.h:23: class MockVoEChannelProxy : public voe::ChannelProxy { ModifyEncoder is missing, and presumably one or more test cases.
On 2017/04/18 08:24:12, the sun wrote: > The configure/reconfigure logic makes me worried. I'd prefer to make it simpler, > trading away some efficiency and make more of the conditional calls on > Channel/AudioEncoder unconditional. I'm avoiding repeating these calls for reasons of correctness, not for efficiency. I tried having more of these unconditional while making this patch set, but the underlying systems didn't like that at all. I recall some of these things breaking if getting called once already set, at least without doing a whole teardown/setup first. > What you're doing here is essentially caching, which very easily breaks down. I guess this could be alleviated by having the called functions themselves validate whether the call was actually a change, but they've instead been written to disallow such behavior. Or if there were getters to rely on, but that's not always the case either... Do you know of any other part of WebRTC that would change our Channels external from AudioSendStream/AudioReceiveStream? If not, I'm not sure what would cause these to go out of sync. https://codereview.webrtc.org/2705093002/diff/180001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2705093002/diff/180001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:113: if (old_config.rtp.ssrc != new_config.rtp.ssrc) { On 2017/04/18 08:24:12, the sun wrote: > Refresh my memory - is SSRC=0 valid or not? If it is, this condition will cause > some machinery to not be configured (see e.g. > ModuleRtpRtcpImpl::SetRtcpReceiverSsrcs()). A channel defaults to SSRC 0, so it seems weird if setting it to zero again would change anything. Not sure if that's the case, though. I think these all could be made more robust by always checking is_configured as well (see below). https://codereview.webrtc.org/2705093002/diff/180001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:127: if (!is_configured || On 2017/04/18 08:24:12, the sun wrote: > This is_configured flag is used inconsistently in this function. Is that > intended? I think it was, but I'm doubting if that's good. See the next comment. https://codereview.webrtc.org/2705093002/diff/180001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:159: if (!is_configured || new_ids.audio_level != old_ids.audio_level) { On 2017/04/18 08:24:12, the sun wrote: > IIUC "!is_configured" is unnecessary here since the old ids will be 0 the first > time? I think I should just uniformly check !is_configured for all of these, to make it all a bit simpler to understand. Perhaps turn it into a function argument rather than a stream member variable: it will only be unconfigured during the first call, from the constructor. https://codereview.webrtc.org/2705093002/diff/180001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:474: if (new_config.audio_network_adaptor_config == On 2017/04/18 08:24:12, the sun wrote: > You must also do the below if SetupSendCodec() has created a new codec, right? > Goes for all these functions, I believe. SetupSendCodec already has code to call EnableAudioNetworkAdaptor directly on the encoder object, before it's handed over to the ACM. It also wraps the new encoder in a CNG encoder if necessary, so ReconfigureCNG should not be called. I'll take another look at (Re)configureBitrateObserver. It does look odd that there's no chance to update these bitrate parameters if there's also a codec change at the same time. https://codereview.webrtc.org/2705093002/diff/180001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:506: stream->channel_proxy_->ModifyEncoder( On 2017/04/18 08:24:12, the sun wrote: > CallEncoder? No, CallEncoder doesn't allow for replacing the encoder, which is what we need to do here. https://codereview.webrtc.org/2705093002/diff/180001/webrtc/test/mock_voe_cha... File webrtc/test/mock_voe_channel_proxy.h (right): https://codereview.webrtc.org/2705093002/diff/180001/webrtc/test/mock_voe_cha... webrtc/test/mock_voe_channel_proxy.h:23: class MockVoEChannelProxy : public voe::ChannelProxy { On 2017/04/18 08:24:12, the sun wrote: > ModifyEncoder is missing, and presumably one or more test cases. True.
https://codereview.webrtc.org/2705093002/diff/180001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2705093002/diff/180001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:159: if (!is_configured || new_ids.audio_level != old_ids.audio_level) { On 2017/04/20 10:11:17, ossu wrote: > On 2017/04/18 08:24:12, the sun wrote: > > IIUC "!is_configured" is unnecessary here since the old ids will be 0 the > first > > time? > > I think I should just uniformly check !is_configured for all of these, to make > it all a bit simpler to understand. Perhaps turn it into a function argument > rather than a stream member variable: it will only be unconfigured during the > first call, from the constructor. That sgtm!
I've made ConfigureStream take a first_time argument, instead of the stream having to keep track of whether it's configured or not. That would only happen in the call from the constructor. Also cleaned up the BitrateObserver stuff a bit. Turns out there was an error in our callback that stefan@ had already fixed in master. So now it should be safe to set it up immediately, rather than waiting until Start().
The CQ bit was checked by ossu@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.
On 2017/04/21 16:10:18, ossu wrote: > I've made ConfigureStream take a first_time argument, instead of the stream > having to keep track of whether it's configured or not. That would only happen > in the call from the constructor. > > Also cleaned up the BitrateObserver stuff a bit. Turns out there was an error in > our callback that stefan@ had already fixed in master. So now it should be safe > to set it up immediately, rather than waiting until Start(). lgtm
lg https://codereview.webrtc.org/2705093002/diff/220001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2705093002/diff/220001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:390: // Explicitly hide config_ here, so we don't accidentally setup a send codec Don't understand this comment https://codereview.webrtc.org/2705093002/diff/220001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:450: if (!new_config.send_codec_spec) { RTC_CHECK(new_config.send_codec_spec) << "Cannot replace the current encoder with no encoder"; ? https://codereview.webrtc.org/2705093002/diff/220001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:462: if (new_target_bitrate_bps && Should also execute if "first_time" is true? https://codereview.webrtc.org/2705093002/diff/220001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:478: if (new_config.audio_network_adaptor_config == Shouldn't you push down the "first_time" flag all the way here? Same goes for ReconfigureCNG. https://codereview.webrtc.org/2705093002/diff/220001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:538: if (stream->config_.min_bitrate_bps == new_config.min_bitrate_bps && first_time? https://codereview.webrtc.org/2705093002/diff/220001/webrtc/call/audio_send_s... File webrtc/call/audio_send_stream.cc (right): https://codereview.webrtc.org/2705093002/diff/220001/webrtc/call/audio_send_s... webrtc/call/audio_send_stream.cc:69: ss << ", cng_payload_type: " << cng_payload_type.value_or(-1); <unset>? Would be good to use one way to denote an unset Optional in logs. https://codereview.webrtc.org/2705093002/diff/220001/webrtc/call/audio_send_s... File webrtc/call/audio_send_stream.h (right): https://codereview.webrtc.org/2705093002/diff/220001/webrtc/call/audio_send_s... webrtc/call/audio_send_stream.h:115: int payload_type; nit: always init. Someone will change the codec later on.
https://codereview.webrtc.org/2705093002/diff/220001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2705093002/diff/220001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:390: // Explicitly hide config_ here, so we don't accidentally setup a send codec On 2017/04/24 14:48:49, the sun wrote: > Don't understand this comment No, it's not applicable anymore as SetupSendCodec now takes a stream explicitly for this purpose. I will remove it. https://codereview.webrtc.org/2705093002/diff/220001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:450: if (!new_config.send_codec_spec) { On 2017/04/24 14:48:48, the sun wrote: > RTC_CHECK(new_config.send_codec_spec) << "Cannot replace the current encoder > with no encoder"; > > ? My guess is as good as yours. This really shouldn't happen. Maybe just replace it with an RTC_CHECK(new_config.send_codec_spec)? https://codereview.webrtc.org/2705093002/diff/220001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:462: if (new_target_bitrate_bps && On 2017/04/24 14:48:49, the sun wrote: > Should also execute if "first_time" is true? No. Once we have a SendCodecSpec to use, this gets set up by SetupSendCodec above. Before we have an encoder, this call would do nothing. https://codereview.webrtc.org/2705093002/diff/220001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:478: if (new_config.audio_network_adaptor_config == On 2017/04/24 14:48:49, the sun wrote: > Shouldn't you push down the "first_time" flag all the way here? Same goes for > ReconfigureCNG. No. See previous comment. https://codereview.webrtc.org/2705093002/diff/220001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:538: if (stream->config_.min_bitrate_bps == new_config.min_bitrate_bps && On 2017/04/24 14:48:49, the sun wrote: > first_time? That should not be necessary, though I should probably add a comment explaining why. Here's the gist: They're both by default -1, indicating no bitrate observer should be used. We don't want to remove ourselves in that case, since we haven't been added. If configured with other values than -1, the comparison will catch that and configure the bitrate observer. https://codereview.webrtc.org/2705093002/diff/220001/webrtc/call/audio_send_s... File webrtc/call/audio_send_stream.cc (right): https://codereview.webrtc.org/2705093002/diff/220001/webrtc/call/audio_send_s... webrtc/call/audio_send_stream.cc:69: ss << ", cng_payload_type: " << cng_payload_type.value_or(-1); On 2017/04/24 14:48:49, the sun wrote: > <unset>? Would be good to use one way to denote an unset Optional in logs. Unfortunately, operator<< can only take one type and *cng_payload_type is (at best) int. I wrote some code to properly print Optionals, but was asked to only include it in test builds since "it would be nice if we could avoid using iostream." I guess I could refactor it into: ss << ", cng_payload_type: " << (cng_payload_type ? std::to_string(*cng_payload_type) : "<unset>") https://codereview.webrtc.org/2705093002/diff/220001/webrtc/call/audio_send_s... File webrtc/call/audio_send_stream.h (right): https://codereview.webrtc.org/2705093002/diff/220001/webrtc/call/audio_send_s... webrtc/call/audio_send_stream.h:115: int payload_type; On 2017/04/24 14:48:49, the sun wrote: > nit: always init. Someone will change the codec later on. The SendCodecSpec constructor explicitly asks for payload_type and format, so they will be initialized.
https://codereview.webrtc.org/2705093002/diff/220001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2705093002/diff/220001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:450: if (!new_config.send_codec_spec) { On 2017/04/24 16:20:30, ossu wrote: > On 2017/04/24 14:48:48, the sun wrote: > > RTC_CHECK(new_config.send_codec_spec) << "Cannot replace the current encoder > > with no encoder"; > > > > ? > > My guess is as good as yours. This really shouldn't happen. Maybe just replace > it with an RTC_CHECK(new_config.send_codec_spec)? ... or is that "Your guess is as good as mine?" Hmm... :)
https://codereview.webrtc.org/2705093002/diff/220001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2705093002/diff/220001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:462: if (new_target_bitrate_bps && On 2017/04/24 16:20:30, ossu wrote: > On 2017/04/24 14:48:49, the sun wrote: > > Should also execute if "first_time" is true? > > No. Once we have a SendCodecSpec to use, this gets set up by SetupSendCodec > above. Before we have an encoder, this call would do nothing. Ah, sorry! https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:107: // Dumps an AudioCodec in RFC 2327-ish format. nit: does this comment really apply anymore? https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1047: // encoder's bit rate immediately (through call_->CreateAudioSendStream), bad comment https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1114: void CreateAudioSendStream() { Don't need a function for this anymore https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1125: UpdateSendState(); Should no longer be needed - the stream already has the previous send state. https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1663: if (!send_codec_spec) nit: use {} like elsewhere in this file https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (left): https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:1516: TEST_F(WebRtcVoiceEngineTestFake, SetSendCodecOpusMaxAverageBitrate) { Why is this test removed? https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:1670: TEST_F(WebRtcVoiceEngineTestFake, SetSendCodecNoOpusFec) { Where are these test cases living now? https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:1213: // EXPECT_EQ(48000, send_codec_spec.codec_inst.rate); Why is this commented out? https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:1217: // EXPECT_EQ(webrtc::kFreq8000Hz, send_codec_spec.cng_plfreq); likewise https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:1723: // EXPECT_EQ(webrtc::kFreq16000Hz, send_codec_spec.cng_plfreq); commented out - why?
Went through and annotated the removed tests. Most were moved into codec or factory specific tests, which happened in a previous CL, so it's not apparent here. https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (left): https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:721: // Tests that the list of supported codecs is created properly and ordered A similar test is now in place for BuiltinAudioEncoderFactory. https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:746: // Tests that we can find codecs by name or id, and that we interpret the This one just tests ToCodecInst, which is removed. https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:1318: // Verify that G722 is set with 16000 samples per second to WebRTC. This one isn't applicable anymore, since we're not special-casing G722 in WebRtcVoiceEngine anymore. Also, clock rate and sample rate are available separately now. https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:1516: TEST_F(WebRtcVoiceEngineTestFake, SetSendCodecOpusMaxAverageBitrate) { On 2017/04/25 11:38:53, the sun wrote: > Why is this test removed? It has been put in AudioEncoderOpusTest instead, where it belongs. https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:1670: TEST_F(WebRtcVoiceEngineTestFake, SetSendCodecNoOpusFec) { On 2017/04/25 11:38:53, the sun wrote: > Where are these test cases living now? They have been moved to their respective codec implementations. https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:1768: // Test maxplaybackrate <= 8000 triggers Opus narrow band mode. These have been moved to Opus's own tests. https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:1867: // Test maxplaybackrate can be set on two streams. I don't think this one is applicable anymore, since max playback rate isn't handled by WebRtcVoiceEngine. It's tested on an Opus instance basis in AudioEncoderOpusTest. https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:2015: // Test that we could set packet size specified in kCodecParamPTime. These have been moved to the respective codec tests. https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:3674: // Tests that the library is configured with the codecs we want. Generally tests ToCodecInst, which no longer exists. A similar test exists for BuiltinAudioEncoderFactory.
On 2017/04/25 11:58:11, ossu wrote: > Went through and annotated the removed tests. Most were moved into codec or > factory specific tests, which happened in a previous CL, so it's not apparent > here. > > https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... > File webrtc/media/engine/webrtcvoiceengine_unittest.cc (left): > > https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... > webrtc/media/engine/webrtcvoiceengine_unittest.cc:721: // Tests that the list of > supported codecs is created properly and ordered > A similar test is now in place for BuiltinAudioEncoderFactory. > > https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... > webrtc/media/engine/webrtcvoiceengine_unittest.cc:746: // Tests that we can find > codecs by name or id, and that we interpret the > This one just tests ToCodecInst, which is removed. > > https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... > webrtc/media/engine/webrtcvoiceengine_unittest.cc:1318: // Verify that G722 is > set with 16000 samples per second to WebRTC. > This one isn't applicable anymore, since we're not special-casing G722 in > WebRtcVoiceEngine anymore. Also, clock rate and sample rate are available > separately now. > > https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... > webrtc/media/engine/webrtcvoiceengine_unittest.cc:1516: > TEST_F(WebRtcVoiceEngineTestFake, SetSendCodecOpusMaxAverageBitrate) { > On 2017/04/25 11:38:53, the sun wrote: > > Why is this test removed? > > It has been put in AudioEncoderOpusTest instead, where it belongs. > > https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... > webrtc/media/engine/webrtcvoiceengine_unittest.cc:1670: > TEST_F(WebRtcVoiceEngineTestFake, SetSendCodecNoOpusFec) { > On 2017/04/25 11:38:53, the sun wrote: > > Where are these test cases living now? > > They have been moved to their respective codec implementations. > > https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... > webrtc/media/engine/webrtcvoiceengine_unittest.cc:1768: // Test maxplaybackrate > <= 8000 triggers Opus narrow band mode. > These have been moved to Opus's own tests. > > https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... > webrtc/media/engine/webrtcvoiceengine_unittest.cc:1867: // Test maxplaybackrate > can be set on two streams. > I don't think this one is applicable anymore, since max playback rate isn't > handled by WebRtcVoiceEngine. It's tested on an Opus instance basis in > AudioEncoderOpusTest. > > https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... > webrtc/media/engine/webrtcvoiceengine_unittest.cc:2015: // Test that we could > set packet size specified in kCodecParamPTime. > These have been moved to the respective codec tests. > > https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... > webrtc/media/engine/webrtcvoiceengine_unittest.cc:3674: // Tests that the > library is configured with the codecs we want. > Generally tests ToCodecInst, which no longer exists. A similar test exists for > BuiltinAudioEncoderFactory. LGTM % the other comments
https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:1213: // EXPECT_EQ(48000, send_codec_spec.codec_inst.rate); On 2017/04/25 11:38:53, the sun wrote: > Why is this commented out? Oh, I believe I should still test that this gets set properly. AFAIK, rate is never set explicitly like this anymore within WebRTC (it used to be, but only from data taken from the ACM) but might as well make sure it works if someone else does it. Good catch! https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:1217: // EXPECT_EQ(webrtc::kFreq8000Hz, send_codec_spec.cng_plfreq); On 2017/04/25 11:38:53, the sun wrote: > likewise CNG payload frequency is no longer tracked separate from the main codec's payload frequency. Just forgot to remove the line. https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:1723: // EXPECT_EQ(webrtc::kFreq16000Hz, send_codec_spec.cng_plfreq); On 2017/04/25 11:38:53, the sun wrote: > commented out - why? Forgot to remove it. It's no longer possible to indicate a different cng payload frequency from the main codec's payload frequency (changed that in an earlier CL, not specifically related to this CL but fixing a problem unearthed by it).
Patchset #10 (id:240001) has been deleted
The CQ bit was checked by ossu@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.
https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:107: // Dumps an AudioCodec in RFC 2327-ish format. On 2017/04/25 11:38:53, the sun wrote: > nit: does this comment really apply anymore? Depends on how far one's willing to stretch that "-ish". :) https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1047: // encoder's bit rate immediately (through call_->CreateAudioSendStream), On 2017/04/25 11:38:53, the sun wrote: > bad comment Done. https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1114: void CreateAudioSendStream() { On 2017/04/25 11:38:53, the sun wrote: > Don't need a function for this anymore Done. https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1125: UpdateSendState(); On 2017/04/25 11:38:53, the sun wrote: > Should no longer be needed - the stream already has the previous send state. Done. https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1663: if (!send_codec_spec) On 2017/04/25 11:38:53, the sun wrote: > nit: use {} like elsewhere in this file Done. https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2705093002/diff/220001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:1213: // EXPECT_EQ(48000, send_codec_spec.codec_inst.rate); On 2017/04/25 12:58:38, ossu wrote: > On 2017/04/25 11:38:53, the sun wrote: > > Why is this commented out? > > Oh, I believe I should still test that this gets set properly. AFAIK, rate is > never set explicitly like this anymore within WebRTC (it used to be, but only > from data taken from the ACM) but might as well make sure it works if someone > else does it. > > Good catch! Turns out this failed at 48000, since mono ISAC only allows bitrates up to 32000. I've lowered the value and reinstated the check.
The CQ bit was checked by ossu@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_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/21032)
Patchset #12 (id:300001) has been deleted
The CQ bit was checked by ossu@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/...
kwiberg@ PTAL at the change to channel.cc in patchset 12. It fixes the previously failing chromium test but is a little bit more involved than the solution we talked about earlier. If just asking audio_coding_, it would lie to us unnecessarily, even for codecs set by Channel::SetSendCodec. https://codereview.webrtc.org/2705093002/diff/320001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2705093002/diff/320001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:1346: { At first, I had this code just ask the ACM directly, however it turns out that when Channel does SetSendCodec, all the codec info within the ACM is discarded, and it's treated as just another external codec. The ACM has its own codec_manager_ instance it (I guess) sometimes uses to create encoders. Now, we'll use the CodecInst passed to SetSendCodec if it's available (it's set up with PCMU as the default codec in Channel::Init), or ask the ACM for info if it's not - since it has the ability to make something reasonable up for external encoders. When calling Channel::SetEncoder, it will ensure we discard any previously configured CodecInst by calling CodecManager::UnsetCodecInst() above.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.webrtc.org/2705093002/diff/320001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2705093002/diff/320001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:1346: { On 2017/04/26 15:45:36, ossu wrote: > At first, I had this code just ask the ACM directly, however it turns out that > when Channel does SetSendCodec, all the codec info within the ACM is discarded, > and it's treated as just another external codec. The ACM has its own > codec_manager_ instance it (I guess) sometimes uses to create encoders. Yes. Hopefully we can remove that soon-ish. > Now, we'll use the CodecInst passed to SetSendCodec if it's available (it's set > up with PCMU as the default codec in Channel::Init), or ask the ACM for info if > it's not - since it has the ability to make something reasonable up for external > encoders. > > When calling Channel::SetEncoder, it will ensure we discard any previously > configured CodecInst by calling CodecManager::UnsetCodecInst() above. Ack. We really need to make it a priority to remove all uses of CodecInst sooner rather than later; this sort of code feels like it could cause much weeping and gnashing of teeth the next time we need to change it and have to figure out how it works...
On 2017/04/26 20:03:36, kwiberg-webrtc wrote: > lgtm > > https://codereview.webrtc.org/2705093002/diff/320001/webrtc/voice_engine/chan... > File webrtc/voice_engine/channel.cc (right): > > https://codereview.webrtc.org/2705093002/diff/320001/webrtc/voice_engine/chan... > webrtc/voice_engine/channel.cc:1346: { > On 2017/04/26 15:45:36, ossu wrote: > > At first, I had this code just ask the ACM directly, however it turns out that > > when Channel does SetSendCodec, all the codec info within the ACM is > discarded, > > and it's treated as just another external codec. The ACM has its own > > codec_manager_ instance it (I guess) sometimes uses to create encoders. > > Yes. Hopefully we can remove that soon-ish. > > > Now, we'll use the CodecInst passed to SetSendCodec if it's available (it's > set > > up with PCMU as the default codec in Channel::Init), or ask the ACM for info > if > > it's not - since it has the ability to make something reasonable up for > external > > encoders. > > > > When calling Channel::SetEncoder, it will ensure we discard any previously > > configured CodecInst by calling CodecManager::UnsetCodecInst() above. > > Ack. > > We really need to make it a priority to remove all uses of CodecInst sooner > rather than later; this sort of code feels like it could cause much weeping and > gnashing of teeth the next time we need to change it and have to figure out how > it works... Agreed. I'd even like to see us go one further and stop using the ACM completely. Instead interfacing to the transport, the encoder and, I guess, the audio input directly in AudioSendStream. I believe we could make this all work with less code and in considerably fewer layers.
The CQ bit was checked by ossu@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org, minyue@webrtc.org Link to the patchset: https://codereview.webrtc.org/2705093002/#ps320001 (title: "Channel::GetSendCodec asks both its acm and its codec manager.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 320001, "attempt_start_ts": 1493283966730810,
"parent_rev": "66753c34ad91d82919cefab13c08c28492c0a1a1", "commit_rev":
"20a4b3fb2ae5502488739f852aa73ba6ab26f7ec"}
Message was sent while issue was closed.
Description was changed from ========== Injectable audio encoders: WebRtcVoiceEngine and company These are the changes made to WebRtcVoiceEngine and surrounding code. It still contains some things that are inelegant, like how AudioCodecSpec and AudioFormatInfo is ferried around in SendCodecSpec. This should probably be resolved before landing. There are also a few test still that are disabled. They should be removed or fixed, as the case may be. I've put this CL up to get a better overview of the changes made and how reviewable they are. BUG=webrtc:5806 ========== to ========== Injectable audio encoders: WebRtcVoiceEngine and company These are the changes made to WebRtcVoiceEngine and surrounding code. It still contains some things that are inelegant, like how AudioCodecSpec and AudioFormatInfo is ferried around in SendCodecSpec. This should probably be resolved before landing. There are also a few test still that are disabled. They should be removed or fixed, as the case may be. I've put this CL up to get a better overview of the changes made and how reviewable they are. BUG=webrtc:5806 Review-Url: https://codereview.webrtc.org/2705093002 Cr-Commit-Position: refs/heads/master@{#17904} Committed: https://chromium.googlesource.com/external/webrtc/+/20a4b3fb2ae5502488739f852... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:320001) as https://chromium.googlesource.com/external/webrtc/+/20a4b3fb2ae5502488739f852... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
