Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2516993002:
Pass SdpAudioFormat through Channel, without converting to CodecInst (Closed)
|
Created:
4 years, 1 month ago by kwiberg-webrtc Modified:
3 years, 7 months ago CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, peah-webrtc, Andrew MacDonald, henrika_webrtc, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc, minyue-webrtc Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionPass SdpAudioFormat through Channel, without converting to CodecInst
BUG=webrtc:5805
Review-Url: https://codereview.webrtc.org/2516993002
Cr-Commit-Position: refs/heads/master@{#16165}
Committed: https://chromium.googlesource.com/external/webrtc/+/d32bf7572138812dda9bd0028ac099a6e440fc61
Patch Set 1 #
Total comments: 2
Patch Set 2 : . #Patch Set 3 : . #Patch Set 4 : . #Patch Set 5 : . #
Total comments: 37
Patch Set 6 : easy comments #
Total comments: 3
Patch Set 7 : no pass-by-T&& #Patch Set 8 : Check that codecs are supported #Patch Set 9 : Better check for supported codecs #Patch Set 10 : Fix SetRecvCodecsAfterAddingStreams #
Total comments: 21
Patch Set 11 : Better IsSupportedDecoder implementation #Patch Set 12 : Review comments #
Total comments: 2
Patch Set 13 : Erase Potemkin from the history books #Patch Set 14 : rebase #
Total comments: 6
Patch Set 15 : nits + rebase #Patch Set 16 : opus default stereo #
Total comments: 1
Patch Set 17 : fix #Patch Set 18 : more fix #Patch Set 19 : rebase #Patch Set 20 : rebase #Patch Set 21 : emulate bug 6986 #
Total comments: 2
Patch Set 22 : rebase #Patch Set 23 : don't fail if we have to ignore extra parameters #Patch Set 24 : rebase #Patch Set 25 : add TODO #Messages
Total messages: 103 (75 generated)
The CQ bit was checked by kwiberg@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.
kwiberg@webrtc.org changed reviewers: + ossu@webrtc.org
solenberg@webrtc.org changed reviewers: + solenberg@webrtc.org
https://codereview.webrtc.org/2516993002/diff/1/webrtc/voice_engine/include/v... File webrtc/voice_engine/include/voe_codec.h (right): https://codereview.webrtc.org/2516993002/diff/1/webrtc/voice_engine/include/v... webrtc/voice_engine/include/voe_codec.h:83: virtual int SetRecPayloadType(int channel, Why do we need to add this to the legacy API?
https://codereview.webrtc.org/2516993002/diff/1/webrtc/voice_engine/include/v... File webrtc/voice_engine/include/voe_codec.h (right): https://codereview.webrtc.org/2516993002/diff/1/webrtc/voice_engine/include/v... webrtc/voice_engine/include/voe_codec.h:83: virtual int SetRecPayloadType(int channel, On 2016/11/21 08:37:05, the sun wrote: > Why do we need to add this to the legacy API? Should we go from AudioReceiveStream directly through channel_proxy_ onto the Channel instead?
On 2016/11/30 12:43:43, ossu wrote: > https://codereview.webrtc.org/2516993002/diff/1/webrtc/voice_engine/include/v... > File webrtc/voice_engine/include/voe_codec.h (right): > > https://codereview.webrtc.org/2516993002/diff/1/webrtc/voice_engine/include/v... > webrtc/voice_engine/include/voe_codec.h:83: virtual int SetRecPayloadType(int > channel, > On 2016/11/21 08:37:05, the sun wrote: > > Why do we need to add this to the legacy API? > > Should we go from AudioReceiveStream directly through channel_proxy_ onto the > Channel instead? Yes.
The CQ bit was checked by kwiberg@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: ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/15460) mac_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/20001)
The CQ bit was checked by kwiberg@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: linux_ubsan_vptr on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)
The CQ bit was checked by kwiberg@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.
The CQ bit was checked by kwiberg@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.
Please have a look. (But don't try to diff against previous patch sets.) https://codereview.webrtc.org/2516993002/diff/80001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2516993002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:758: //EXPECT_FALSE(channel_->SetRecvParameters(parameters)); Should this test simply be changed, or do we want to change things around so that we verify SdpAudioFormat earlier? For example with the two-stage factory we discussed earlier? https://codereview.webrtc.org/2516993002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:889: //EXPECT_STREQ("ISAC", gcodec.plname); As the comment says, I think this is fundamentally caused by FakeWebRtcVoiceEngine not coping with AudioReceiveStream. Is fixing that the right thing to do?
Good stuff! https://codereview.webrtc.org/2516993002/diff/80001/webrtc/call/audio_receive... File webrtc/call/audio_receive_stream.h (right): https://codereview.webrtc.org/2516993002/diff/80001/webrtc/call/audio_receive... webrtc/call/audio_receive_stream.h:105: // Decoders for every payload that we can receive. nit: "Decoders" is not strictly true ... "Formats"? https://codereview.webrtc.org/2516993002/diff/80001/webrtc/media/engine/paylo... File webrtc/media/engine/payload_type_mapper.h (right): https://codereview.webrtc.org/2516993002/diff/80001/webrtc/media/engine/paylo... webrtc/media/engine/payload_type_mapper.h:23: webrtc::SdpAudioFormat AudioCodecToSdp(const AudioCodec& ac); nit: "Sdp" -> "SdpAudioFormat", or maybe just call it FromAudioCodec()? https://codereview.webrtc.org/2516993002/diff/80001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2516993002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1511: for (const auto& item : config_.decoder_map) { This is a good check, but it has (or *should* have) already been done in WVoMC::SetRecvCodecs(). I think we should put it as a lambda inside a DCHECK so we strip it out in release builds. https://codereview.webrtc.org/2516993002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1670: auto ext = recv_rtp_extensions_; // Make a copy. To me, the fact that you need to write a comment indicates this practise is more obscure than simply pushing the extensions as a const& to RecreateAudioReceiveStream(). Is there really any benefit except for consistency here? https://codereview.webrtc.org/2516993002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1859: for (auto& it : recv_streams_) { nit: it -> kv https://codereview.webrtc.org/2516993002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1860: auto dm = decoder_map; // Make a copy. Same comment as on line 1670 https://codereview.webrtc.org/2516993002/diff/80001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2516993002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:758: //EXPECT_FALSE(channel_->SetRecvParameters(parameters)); On 2016/12/08 09:44:06, kwiberg-webrtc wrote: > Should this test simply be changed, or do we want to change things around so > that we verify SdpAudioFormat earlier? For example with the two-stage factory we > discussed earlier? The call to SetRecvParameters() should still fail if we're given an unsupported codec, and this may be for a number of reason by looking at the old code. First, I'd like for WVoMC::SetRecvCodecs() to validate as much as possible, so that the "objective" attributes (payload type range, duplicate payload type assignments, etc). Then, is it possible to do something simple initially, e.g. just compare the list of codecs provided by the decoder factory and reject anything not included (compare on name, rate), or may there still be cases instantiation may later fail so we need to do the two-stage? https://codereview.webrtc.org/2516993002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:885: // voe_. Not sure how to handle this---should FakeWebRtcVoiceEngine be fixed? You should remove the call to FakeWebRtcVoiceEngine (and make a WEBRTC_STUB of the SetRecPayloadType fake impl, removing anything it references that becomes stale). For this test it seems appropriate to use GetRecvStreamConfig(kSsrc1) (local utility in this test) and EXPECT on contents of the stream config. https://codereview.webrtc.org/2516993002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:889: //EXPECT_STREQ("ISAC", gcodec.plname); On 2016/12/08 09:44:06, kwiberg-webrtc wrote: > As the comment says, I think this is fundamentally caused by > FakeWebRtcVoiceEngine not coping with AudioReceiveStream. Is fixing that the > right thing to do? See above https://codereview.webrtc.org/2516993002/diff/80001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_format_conversion.cc (right): https://codereview.webrtc.org/2516993002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_format_conversion.cc:13: #include <cstring> The code base generally uses string.h https://codereview.webrtc.org/2516993002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_format_conversion.cc:25: CodecInst MakeCI(int payload_type, MakeCodecInst https://codereview.webrtc.org/2516993002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_format_conversion.cc:32: rtc::MsanMarkUninitialized(rtc::MakeArrayView(&ci, 1)); nice! https://codereview.webrtc.org/2516993002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_format_conversion.cc:35: ci.plname[sizeof(ci.plname) - 1] = '\0'; don't you need to init the trailing zero as well? won't we get sanitizer reports if comparing against it otherwise? https://codereview.webrtc.org/2516993002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_format_conversion.cc:59: return MakeCI(payload_type, "g722", 16000, audio_format.num_channels); no other parameters that would need to be copied? https://codereview.webrtc.org/2516993002/diff/80001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel_proxy.h (right): https://codereview.webrtc.org/2516993002/diff/80001/webrtc/voice_engine/chann... webrtc/voice_engine/channel_proxy.h:77: virtual int SetRecPayloadType(int payload_type, const SdpAudioFormat& format); remove return code since you're not checking it in AudioReceiveStream. DCHECK it in the impl instead.
Posted new patch set with the easy stuff fixed. https://codereview.webrtc.org/2516993002/diff/80001/webrtc/call/audio_receive... File webrtc/call/audio_receive_stream.h (right): https://codereview.webrtc.org/2516993002/diff/80001/webrtc/call/audio_receive... webrtc/call/audio_receive_stream.h:105: // Decoders for every payload that we can receive. On 2016/12/08 10:53:52, the sun wrote: > nit: "Decoders" is not strictly true ... "Formats"? I picked "decoder specifications". https://codereview.webrtc.org/2516993002/diff/80001/webrtc/media/engine/paylo... File webrtc/media/engine/payload_type_mapper.h (right): https://codereview.webrtc.org/2516993002/diff/80001/webrtc/media/engine/paylo... webrtc/media/engine/payload_type_mapper.h:23: webrtc::SdpAudioFormat AudioCodecToSdp(const AudioCodec& ac); On 2016/12/08 10:53:52, the sun wrote: > nit: "Sdp" -> "SdpAudioFormat", or maybe just call it FromAudioCodec()? Done. https://codereview.webrtc.org/2516993002/diff/80001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2516993002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1511: for (const auto& item : config_.decoder_map) { On 2016/12/08 10:53:52, the sun wrote: > This is a good check, but it has (or *should* have) already been done in > WVoMC::SetRecvCodecs(). I think we should put it as a lambda inside a DCHECK so > we strip it out in release builds. Done. Also changed return type to void. https://codereview.webrtc.org/2516993002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1670: auto ext = recv_rtp_extensions_; // Make a copy. On 2016/12/08 10:53:52, the sun wrote: > To me, the fact that you need to write a comment indicates this practise is more > obscure than simply pushing the extensions as a const& to > RecreateAudioReceiveStream(). Is there really any benefit except for consistency > here? I declared RecreateAudioReceiveStreamn to take an rvalue reference, so the copy needs to be explicit. (This is on purpose, since copying a std::map isn't cheap.) Do you think this is a bad idea? https://codereview.webrtc.org/2516993002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1859: for (auto& it : recv_streams_) { On 2016/12/08 10:53:52, the sun wrote: > nit: it -> kv Done. https://codereview.webrtc.org/2516993002/diff/80001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2516993002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:758: //EXPECT_FALSE(channel_->SetRecvParameters(parameters)); On 2016/12/08 10:53:52, the sun wrote: > On 2016/12/08 09:44:06, kwiberg-webrtc wrote: > > Should this test simply be changed, or do we want to change things around so > > that we verify SdpAudioFormat earlier? For example with the two-stage factory > we > > discussed earlier? > > The call to SetRecvParameters() should still fail if we're given an unsupported > codec, and this may be for a number of reason by looking at the old code. First, > I'd like for WVoMC::SetRecvCodecs() to validate as much as possible, so that the > "objective" attributes (payload type range, duplicate payload type assignments, > etc). Sounds good. > Then, is it possible to do something simple initially, e.g. just compare the > list of codecs provided by the decoder factory and reject anything not included > (compare on name, rate), or may there still be cases instantiation may later > fail so we need to do the two-stage? The supported clockrate and number of channels etc. differ from codec to codec, so I'd rather do the check properly. But that's going to be a chunk of work. (Talking about the proposal where the factory emits a second-stage factory instead of a decoder; the second-stage factory takes no arguments, and cannot fail. And then we pass around pt -> 2nd-stage factory maps to all the places.) Hmm. Maybe a compromise, where the factory can simply vet the SdpAudioFormat values? Not as type safe as the full solution, but a much smaller change. https://codereview.webrtc.org/2516993002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:885: // voe_. Not sure how to handle this---should FakeWebRtcVoiceEngine be fixed? On 2016/12/08 10:53:52, the sun wrote: > You should remove the call to FakeWebRtcVoiceEngine (and make a WEBRTC_STUB of > the SetRecPayloadType fake impl, removing anything it references that becomes > stale). > > For this test it seems appropriate to use GetRecvStreamConfig(kSsrc1) (local > utility in this test) and EXPECT on contents of the stream config. Acknowledged. https://codereview.webrtc.org/2516993002/diff/80001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_format_conversion.cc (right): https://codereview.webrtc.org/2516993002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_format_conversion.cc:13: #include <cstring> On 2016/12/08 10:53:52, the sun wrote: > The code base generally uses string.h Yes, unfortunately. Fixed. https://codereview.webrtc.org/2516993002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_format_conversion.cc:25: CodecInst MakeCI(int payload_type, On 2016/12/08 10:53:52, the sun wrote: > MakeCodecInst Done. https://codereview.webrtc.org/2516993002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_format_conversion.cc:35: ci.plname[sizeof(ci.plname) - 1] = '\0'; On 2016/12/08 10:53:52, the sun wrote: > don't you need to init the trailing zero as well? won't we get sanitizer reports > if comparing against it otherwise? No, strncpy copies the null terminator---unless the copying was cut short by the count argument before we reached it. The explicit null write here guards against that case. https://codereview.webrtc.org/2516993002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_format_conversion.cc:59: return MakeCI(payload_type, "g722", 16000, audio_format.num_channels); On 2016/12/08 10:53:52, the sun wrote: > no other parameters that would need to be copied? Like what? CodecInst doesn't have the extra parameter map, and SdpAudioFormat doesn't have a rate field. https://codereview.webrtc.org/2516993002/diff/80001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel_proxy.h (right): https://codereview.webrtc.org/2516993002/diff/80001/webrtc/voice_engine/chann... webrtc/voice_engine/channel_proxy.h:77: virtual int SetRecPayloadType(int payload_type, const SdpAudioFormat& format); On 2016/12/08 10:53:53, the sun wrote: > remove return code since you're not checking it in AudioReceiveStream. DCHECK it > in the impl instead. Done.
https://codereview.webrtc.org/2516993002/diff/80001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2516993002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1670: auto ext = recv_rtp_extensions_; // Make a copy. On 2016/12/09 02:39:01, kwiberg-webrtc wrote: > On 2016/12/08 10:53:52, the sun wrote: > > To me, the fact that you need to write a comment indicates this practise is > more > > obscure than simply pushing the extensions as a const& to > > RecreateAudioReceiveStream(). Is there really any benefit except for > consistency > > here? > > I declared RecreateAudioReceiveStreamn to take an rvalue reference, so the copy > needs to be explicit. (This is on purpose, since copying a std::map isn't > cheap.) Do you think this is a bad idea? I think it's a little unnecessary. Sometimes we need to make copies. Yes, copying a map is more expensive than copying a string, but in this case it happens relatively rarely. Making the code easy on the eye is also important. https://codereview.webrtc.org/2516993002/diff/80001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2516993002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:758: //EXPECT_FALSE(channel_->SetRecvParameters(parameters)); On 2016/12/09 02:39:01, kwiberg-webrtc wrote: > On 2016/12/08 10:53:52, the sun wrote: > > On 2016/12/08 09:44:06, kwiberg-webrtc wrote: > > > Should this test simply be changed, or do we want to change things around so > > > that we verify SdpAudioFormat earlier? For example with the two-stage > factory > > we > > > discussed earlier? > > > > The call to SetRecvParameters() should still fail if we're given an > unsupported > > codec, and this may be for a number of reason by looking at the old code. > First, > > I'd like for WVoMC::SetRecvCodecs() to validate as much as possible, so that > the > > "objective" attributes (payload type range, duplicate payload type > assignments, > > etc). > > Sounds good. > > > Then, is it possible to do something simple initially, e.g. just compare the > > list of codecs provided by the decoder factory and reject anything not > included > > (compare on name, rate), or may there still be cases instantiation may later > > fail so we need to do the two-stage? > > The supported clockrate and number of channels etc. differ from codec to codec, > so I'd rather do the check properly. But that's going to be a chunk of work. > (Talking about the proposal where the factory emits a second-stage factory > instead of a decoder; the second-stage factory takes no arguments, and cannot > fail. And then we pass around pt -> 2nd-stage factory maps to all the places.) > > Hmm. Maybe a compromise, where the factory can simply vet the SdpAudioFormat > values? Not as type safe as the full solution, but a much smaller change. I don't think we should do the 2-stage right now. I'd go for the compromise, or even simpler - can you implement the "vetting" function locally in WVoMC and have it operate on data returned by the factory's GetSupportedDecoders()? https://codereview.webrtc.org/2516993002/diff/80001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_format_conversion.cc (right): https://codereview.webrtc.org/2516993002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_format_conversion.cc:35: ci.plname[sizeof(ci.plname) - 1] = '\0'; On 2016/12/09 02:39:01, kwiberg-webrtc wrote: > On 2016/12/08 10:53:52, the sun wrote: > > don't you need to init the trailing zero as well? won't we get sanitizer > reports > > if comparing against it otherwise? > > No, strncpy copies the null terminator---unless the copying was cut short by the > count argument before we reached it. The explicit null write here guards against > that case. Ah, sorry. Read wrong. https://codereview.webrtc.org/2516993002/diff/80001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_format_conversion.cc:59: return MakeCI(payload_type, "g722", 16000, audio_format.num_channels); On 2016/12/09 02:39:01, kwiberg-webrtc wrote: > On 2016/12/08 10:53:52, the sun wrote: > > no other parameters that would need to be copied? > > Like what? CodecInst doesn't have the extra parameter map, and SdpAudioFormat > doesn't have a rate field. Acknowledged. https://codereview.webrtc.org/2516993002/diff/100001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2516993002/diff/100001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1857: LOG(LS_INFO) << ToString(codec); nit: Is there a ToString() in SdpAudioFormat() that we can use instead - and remove this local one?
https://codereview.webrtc.org/2516993002/diff/80001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2516993002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1670: auto ext = recv_rtp_extensions_; // Make a copy. On 2016/12/09 12:29:49, the sun wrote: > On 2016/12/09 02:39:01, kwiberg-webrtc wrote: > > On 2016/12/08 10:53:52, the sun wrote: > > > To me, the fact that you need to write a comment indicates this practise is > > more > > > obscure than simply pushing the extensions as a const& to > > > RecreateAudioReceiveStream(). Is there really any benefit except for > > consistency > > > here? > > > > I declared RecreateAudioReceiveStreamn to take an rvalue reference, so the > copy > > needs to be explicit. (This is on purpose, since copying a std::map isn't > > cheap.) Do you think this is a bad idea? > > I think it's a little unnecessary. Sometimes we need to make copies. Yes, > copying a map is more expensive than copying a string, but in this case it > happens relatively rarely. Making the code easy on the eye is also important. I think I agree with solenberg here. This map isn't very large, and we're not recreating streams that often. Also, we're doing the copy anyway, so we're not saving anything at this call site. Are we at any other? https://codereview.webrtc.org/2516993002/diff/80001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2516993002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:758: //EXPECT_FALSE(channel_->SetRecvParameters(parameters)); On 2016/12/09 02:39:01, kwiberg-webrtc wrote: > On 2016/12/08 10:53:52, the sun wrote: > > On 2016/12/08 09:44:06, kwiberg-webrtc wrote: > > > Should this test simply be changed, or do we want to change things around so > > > that we verify SdpAudioFormat earlier? For example with the two-stage > factory > > we > > > discussed earlier? > > > > The call to SetRecvParameters() should still fail if we're given an > unsupported > > codec, and this may be for a number of reason by looking at the old code. > First, > > I'd like for WVoMC::SetRecvCodecs() to validate as much as possible, so that > the > > "objective" attributes (payload type range, duplicate payload type > assignments, > > etc). > > Sounds good. > > > Then, is it possible to do something simple initially, e.g. just compare the > > list of codecs provided by the decoder factory and reject anything not > included > > (compare on name, rate), or may there still be cases instantiation may later > > fail so we need to do the two-stage? > > The supported clockrate and number of channels etc. differ from codec to codec, > so I'd rather do the check properly. But that's going to be a chunk of work. > (Talking about the proposal where the factory emits a second-stage factory > instead of a decoder; the second-stage factory takes no arguments, and cannot > fail. And then we pass around pt -> 2nd-stage factory maps to all the places.) > > Hmm. Maybe a compromise, where the factory can simply vet the SdpAudioFormat > values? Not as type safe as the full solution, but a much smaller change. I don't see the immediate benefit of adding another level of indirection, compared to, say, adding an IsSupported() method to the factory as we discussed a number of months back. Both rely on the caller not messing up the return value, i.e. checking that it gets a "true" from IsSupported() or that it gets a non-null factory delegate from the other call. However, it might be that even IsSupported() is unnecessary. If we initiate negotiation, we'll only present the values the factory claims to support. Maybe it could be useful as the answerer (accepting stuff that doesn't match 100% what we want) but I don't think we're plumbed to support that at the moment - the code essentially matches two lists. https://codereview.webrtc.org/2516993002/diff/100001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2516993002/diff/100001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1857: LOG(LS_INFO) << ToString(codec); On 2016/12/09 12:29:50, the sun wrote: > nit: Is there a ToString() in SdpAudioFormat() that we can use instead - and > remove this local one? Oh, that would be nice!
The CQ bit was checked by kwiberg@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: linux_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/21260) win_x64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/20800)
https://codereview.webrtc.org/2516993002/diff/80001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2516993002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.cc:1670: auto ext = recv_rtp_extensions_; // Make a copy. On 2016/12/09 13:23:39, ossu wrote: > On 2016/12/09 12:29:49, the sun wrote: > > On 2016/12/09 02:39:01, kwiberg-webrtc wrote: > > > On 2016/12/08 10:53:52, the sun wrote: > > > > To me, the fact that you need to write a comment indicates this practise > is > > > more > > > > obscure than simply pushing the extensions as a const& to > > > > RecreateAudioReceiveStream(). Is there really any benefit except for > > > consistency > > > > here? > > > > > > I declared RecreateAudioReceiveStreamn to take an rvalue reference, so the > > copy > > > needs to be explicit. (This is on purpose, since copying a std::map isn't > > > cheap.) Do you think this is a bad idea? > > > > I think it's a little unnecessary. Sometimes we need to make copies. Yes, > > copying a map is more expensive than copying a string, but in this case it > > happens relatively rarely. Making the code easy on the eye is also important. > > I think I agree with solenberg here. This map isn't very large, and we're not > recreating streams that often. Also, we're doing the copy anyway, so we're not > saving anything at this call site. Are we at any other? Not sure. We may be, but I guess it doesn't matter that much. The idea is that for expensive-to-copy but copyable types like this, you get the optimal behavior by always taking arguments of the form T&& if the callee will always keep a copy, since that way, if a copy is going to be made it has to be an explicit copy at the call site. (Ideally, there'd be a utility function for this, such as std::copy, analogously to std::move. But since there isn't such a function, you get the extra auto statement instead.) The same could be done for move-only types, and it would be more efficient for them too since passing references around is at least as cheap as moving. (But for the overwhelmingly most important move-only type, std::unique_ptr, the benefit would be negligible at best since a move is just a copying a pointer and zeroing the source pointer.) This is analogous with the idiom of passing arguments as const T& whenever the callee will not keep a copy, or if the caller is expected to want to keep a copy. So I'll take your advice and pass const T& instead. I still think the pass-by-T&& idiom makes sense, in this particular case and in general, but the performance benefits may not be great enough in this case to justify it given that the pass-by-T&& idiom isn't familiar to people. https://codereview.webrtc.org/2516993002/diff/80001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2516993002/diff/80001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:758: //EXPECT_FALSE(channel_->SetRecvParameters(parameters)); On 2016/12/09 13:23:39, ossu wrote: > On 2016/12/09 02:39:01, kwiberg-webrtc wrote: > > On 2016/12/08 10:53:52, the sun wrote: > > > On 2016/12/08 09:44:06, kwiberg-webrtc wrote: > > > > Should this test simply be changed, or do we want to change things around > so > > > > that we verify SdpAudioFormat earlier? For example with the two-stage > > factory > > > we > > > > discussed earlier? > > > > > > The call to SetRecvParameters() should still fail if we're given an > > unsupported > > > codec, and this may be for a number of reason by looking at the old code. > > First, > > > I'd like for WVoMC::SetRecvCodecs() to validate as much as possible, so that > > the > > > "objective" attributes (payload type range, duplicate payload type > > assignments, > > > etc). > > > > Sounds good. > > > > > Then, is it possible to do something simple initially, e.g. just compare the > > > list of codecs provided by the decoder factory and reject anything not > > included > > > (compare on name, rate), or may there still be cases instantiation may later > > > fail so we need to do the two-stage? > > > > The supported clockrate and number of channels etc. differ from codec to > codec, > > so I'd rather do the check properly. But that's going to be a chunk of work. > > (Talking about the proposal where the factory emits a second-stage factory > > instead of a decoder; the second-stage factory takes no arguments, and cannot > > fail. And then we pass around pt -> 2nd-stage factory maps to all the places.) > > > > Hmm. Maybe a compromise, where the factory can simply vet the SdpAudioFormat > > values? Not as type safe as the full solution, but a much smaller change. > > I don't see the immediate benefit of adding another level of indirection, > compared to, say, adding an IsSupported() method to the factory as we discussed > a number of months back. Both rely on the caller not messing up the return > value, i.e. checking that it gets a "true" from IsSupported() or that it gets a > non-null factory delegate from the other call. > > However, it might be that even IsSupported() is unnecessary. If we initiate > negotiation, we'll only present the values the factory claims to support. Maybe > it could be useful as the answerer (accepting stuff that doesn't match 100% what > we want) but I don't think we're plumbed to support that at the moment - the > code essentially matches two lists. Checked against GetSupportedCodecs(), since IsSupported() is a pain to implement (lots of code duplication from the create function, which is one reason I wanted them to be the same). https://codereview.webrtc.org/2516993002/diff/100001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2516993002/diff/100001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1857: LOG(LS_INFO) << ToString(codec); On 2016/12/09 13:23:39, ossu wrote: > On 2016/12/09 12:29:50, the sun wrote: > > nit: Is there a ToString() in SdpAudioFormat() that we can use instead - and > > remove this local one? > > Oh, that would be nice! Sure, there's an operator<< overload for SdpAudioFormat; I'll use that instead. (Can't remove the ToString function though, since it has multiple other callers.)
The CQ bit was checked by kwiberg@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: android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/19438)
The CQ bit was checked by kwiberg@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.
OK, a bunch of new patch sets uploaded. As of the last one (#10), I think I'm done. PTAL. I had to switch from checking codecs against the factory's list to having an IsSupported method, because the list isn't comprehensive enough, and I don't think we really want it to be. The 2-stage factory idea still seems like the better option in the long run though.
Almost there! I have some reservations against the way IsSupportedDecoder is implemented. As you say in your comment in builtin_audio_decoder_factory.h, the functionality for construction should probably be moved into the implementations and with that any supported checks. I'm not going to insist on that for this CL, though. There might be good reasons for keeping the codec implementations separate from the SdpAudioFormat struct. We can contemplate that further later. However, constructing a decoder just to make sure it would be possible to construct a decoder is ... not ideal. :) https://codereview.webrtc.org/2516993002/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc (right): https://codereview.webrtc.org/2516993002/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc:166: return static_cast<bool>(MakeAudioDecoder(format)); I realize you want to land this one, and that this is probably just used in tests, but I don't approve of this implementation of what should be a simple checker function - i.e. something expected to be lightweight. If you want to keep the structure with decoder_constructors[] above, you could split it up like so: struct NamedDecoderConstructor { const char* name; bool (*supported)(const SdpAudioFormat&); std::unique_ptr<AudioDecoder> (*constructor)(const SdpAudioFormat&); }; and {"pcmu", [](const SdpAudioFormat& format) { return format.clockrate_hz == 8000 && format.num_channels >= 1; }, [](const SdpAudioFormat& format) { return Unique(new AudioDecoderPcmU(format.num_channels)); }}, for example. Or even fold the name check into the supported lambda.
https://codereview.webrtc.org/2516993002/diff/180001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream.cc (right): https://codereview.webrtc.org/2516993002/diff/180001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:104: for (const auto& dec : config.decoder_map) { nit: dec -> kv dec makes it read like it's a decoder https://codereview.webrtc.org/2516993002/diff/180001/webrtc/call/audio_receiv... File webrtc/call/audio_receive_stream.h (right): https://codereview.webrtc.org/2516993002/diff/180001/webrtc/call/audio_receiv... webrtc/call/audio_receive_stream.h:106: std::map<int, SdpAudioFormat> decoder_map; nit: is decoder_map really the right name nowadays? payload_type_map? https://codereview.webrtc.org/2516993002/diff/180001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (left): https://codereview.webrtc.org/2516993002/diff/180001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1474: RecreateAudioReceiveStream(local_ssrc, Thanks for fixing this btw! https://codereview.webrtc.org/2516993002/diff/180001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1853: if (WebRtcVoiceEngine::ToCodecInst(codec, &voe_codec)) { What about the other logic that ToCodecInst() has, such as setting ac.bitrate to zero if the codec is "multi rate", and ci.rate to -1 for ISAC when ac.bitrate <= 0. Perhaps there's more, the function is rather subtle. https://codereview.webrtc.org/2516993002/diff/180001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2516993002/diff/180001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1852: auto saf = AudioCodecToSdpAudioFormat(codec); How about "format" instead of "saf"? https://codereview.webrtc.org/2516993002/diff/180001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2516993002/diff/180001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:111: webrtc::MockAudioDecoderFactory::CreateUnusedPotemkinFactory(); I guess that will be explained later, but I'm not sure everyone would immediately understand what Potemkin means https://codereview.webrtc.org/2516993002/diff/180001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:876: EXPECT_EQ(webrtc::SdpAudioFormat("isac", 16000, 1), dm.at(106)); Any reason "ISAC" changed to "isac"?
https://codereview.webrtc.org/2516993002/diff/180001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2516993002/diff/180001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:111: webrtc::MockAudioDecoderFactory::CreateUnusedPotemkinFactory(); On 2016/12/14 08:58:24, the sun wrote: > I guess that will be explained later, but I'm not sure everyone would > immediately understand what Potemkin means I had to google it. That said, it's pretty witty. :)
https://codereview.webrtc.org/2516993002/diff/180001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2516993002/diff/180001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:2227: if (engine()->voe()->codec()->SetRecPayloadType(channel, voe_codec) == -1) { We should be able to remove these calls to SetRecPayloadType() as well?
The CQ bit was checked by kwiberg@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/...
New patch sets posted. https://codereview.webrtc.org/2516993002/diff/180001/webrtc/audio/audio_recei... File webrtc/audio/audio_receive_stream.cc (right): https://codereview.webrtc.org/2516993002/diff/180001/webrtc/audio/audio_recei... webrtc/audio/audio_receive_stream.cc:104: for (const auto& dec : config.decoder_map) { On 2016/12/14 08:58:21, the sun wrote: > nit: dec -> kv > dec makes it read like it's a decoder Done. https://codereview.webrtc.org/2516993002/diff/180001/webrtc/call/audio_receiv... File webrtc/call/audio_receive_stream.h (right): https://codereview.webrtc.org/2516993002/diff/180001/webrtc/call/audio_receiv... webrtc/call/audio_receive_stream.h:106: std::map<int, SdpAudioFormat> decoder_map; On 2016/12/14 08:58:21, the sun wrote: > nit: is decoder_map really the right name nowadays? payload_type_map? It's a payload type -> decoder specification map, so either of those names could work. In my mind, it's more important that its a map *to* decoders (or decoder specs) than that it's a map *from* payload types, so I'm inclined to leave the name as is. If you feel strongly about it, I can change it. https://codereview.webrtc.org/2516993002/diff/180001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (left): https://codereview.webrtc.org/2516993002/diff/180001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1853: if (WebRtcVoiceEngine::ToCodecInst(codec, &voe_codec)) { On 2016/12/14 08:58:21, the sun wrote: > What about the other logic that ToCodecInst() has, such as setting ac.bitrate to > zero if the codec is "multi rate", and ci.rate to -1 for ISAC when ac.bitrate <= > 0. Perhaps there's more, the function is rather subtle. There's a bunch of logic to handle G722's clock rate, but we handle that elsewhere when doing the SdpAudioFormat -> AudioDecoder step. There's also a bunch of logic for twiddling the bitrate, but (1) SdpAudioFormat doesn't have a bitrate field, and (2) we don't need it when creating decoders (none of our AudioDecoder constructors take a bitrate argument). Other than that, I don't think there's anything. https://codereview.webrtc.org/2516993002/diff/180001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2516993002/diff/180001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1852: auto saf = AudioCodecToSdpAudioFormat(codec); On 2016/12/14 08:58:21, the sun wrote: > How about "format" instead of "saf"? Done. https://codereview.webrtc.org/2516993002/diff/180001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:2227: if (engine()->voe()->codec()->SetRecPayloadType(channel, voe_codec) == -1) { On 2016/12/14 10:37:38, the sun wrote: > We should be able to remove these calls to SetRecPayloadType() as well? Maybe. But removing this for loop breaks a half-dozen tests, so I'd rather not try to do it in this CL. https://codereview.webrtc.org/2516993002/diff/180001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2516993002/diff/180001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:111: webrtc::MockAudioDecoderFactory::CreateUnusedPotemkinFactory(); On 2016/12/14 10:21:36, ossu wrote: > On 2016/12/14 08:58:24, the sun wrote: > > I guess that will be explained later, but I'm not sure everyone would > > immediately understand what Potemkin means > > I had to google it. That said, it's pretty witty. :) "I had to google it" and "pretty witty"---what more could you possibly want from a class name? :-) If you have any good suggestions that will play well with our 80-column lines, I'm all ears. https://codereview.webrtc.org/2516993002/diff/180001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:876: EXPECT_EQ(webrtc::SdpAudioFormat("isac", 16000, 1), dm.at(106)); On 2016/12/14 08:58:25, the sun wrote: > Any reason "ISAC" changed to "isac"? I re-wrote it by hand rather than copy+pasting, and I'm not found of SHOUTY CASE. (We always compare these strings case-insensitively, so the actual case doesn't matter.) https://codereview.webrtc.org/2516993002/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc (right): https://codereview.webrtc.org/2516993002/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc:166: return static_cast<bool>(MakeAudioDecoder(format)); On 2016/12/12 13:24:46, ossu wrote: > I realize you want to land this one, and that this is probably just used in > tests, but I don't approve of this implementation of what should be a simple > checker function - i.e. something expected to be lightweight. > > If you want to keep the structure with decoder_constructors[] above, you could > split it up like so: > > struct NamedDecoderConstructor { > const char* name; > bool (*supported)(const SdpAudioFormat&); > std::unique_ptr<AudioDecoder> (*constructor)(const SdpAudioFormat&); > }; > > and > > {"pcmu", > [](const SdpAudioFormat& format) { > return format.clockrate_hz == 8000 && format.num_channels >= 1; > }, > [](const SdpAudioFormat& format) { > return Unique(new AudioDecoderPcmU(format.num_channels)); > }}, > > for example. Or even fold the name check into the supported lambda. Done (not exactly what you suggested, but something to the same effect).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm provided the questions below about the Potemkin mock are resolved, either through a patch set or a really good answer. :) https://codereview.webrtc.org/2516993002/diff/180001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2516993002/diff/180001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:111: webrtc::MockAudioDecoderFactory::CreateUnusedPotemkinFactory(); On 2016/12/14 13:09:36, kwiberg-webrtc wrote: > On 2016/12/14 10:21:36, ossu wrote: > > On 2016/12/14 08:58:24, the sun wrote: > > > I guess that will be explained later, but I'm not sure everyone would > > > immediately understand what Potemkin means > > > > I had to google it. That said, it's pretty witty. :) > > "I had to google it" and "pretty witty"---what more could you possibly want from > a class name? :-) > > If you have any good suggestions that will play well with our 80-column lines, > I'm all ears. I think I'd just set up the mock manually in this function, possibly by basing it on a MockAudioDecoderFactory gotten from CreateUnusedFactory(). I.e. adding the ON_CALLs for GetSupportedDecoders and IsSupportedDecoder here. That said, if we're basing it on the builtin factory, is there a reason to not just use the builtin factory directly? Just making sure it never gets used to create a decoder? If the tests require the specific list of codecs to match what we (for the time being) support internally, I think it's fine to just put the builtin factory there and add a TODO about changing/moving that test once we're further along with the modularization. The CreateUnusedFactory and CreateEmptyFactory are there because sometimes you just need to plug something in that's irrelevant to the test you're performing, except that without it you might stumble on DCHECKs. Looking at it now, CreateEmptyFactory isn't even used, so maybe it should go (not in this CL). I don't think my intention was to set a precedent on how to set up mocks in general. https://codereview.webrtc.org/2516993002/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc (right): https://codereview.webrtc.org/2516993002/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc:166: return static_cast<bool>(MakeAudioDecoder(format)); On 2016/12/14 13:09:36, kwiberg-webrtc wrote: > On 2016/12/12 13:24:46, ossu wrote: > > I realize you want to land this one, and that this is probably just used in > > tests, but I don't approve of this implementation of what should be a simple > > checker function - i.e. something expected to be lightweight. > > > > If you want to keep the structure with decoder_constructors[] above, you could > > split it up like so: > > > > struct NamedDecoderConstructor { > > const char* name; > > bool (*supported)(const SdpAudioFormat&); > > std::unique_ptr<AudioDecoder> (*constructor)(const SdpAudioFormat&); > > }; > > > > and > > > > {"pcmu", > > [](const SdpAudioFormat& format) { > > return format.clockrate_hz == 8000 && format.num_channels >= 1; > > }, > > [](const SdpAudioFormat& format) { > > return Unique(new AudioDecoderPcmU(format.num_channels)); > > }}, > > > > for example. Or even fold the name check into the supported lambda. > > Done (not exactly what you suggested, but something to the same effect). Alright! IsSupportedDecoder now behaves as I'd expect it to. https://codereview.webrtc.org/2516993002/diff/220001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/mock/mock_audio_decoder_factory.h (right): https://codereview.webrtc.org/2516993002/diff/220001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/mock/mock_audio_decoder_factory.h:66: .WillByDefault(testing::Return(std::vector<webrtc::AudioCodecSpec>())); Why does it not propose any decoders but further down says it does support a number of them? I mean, it's not strictly wrong, but strange.
The CQ bit was checked by kwiberg@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/...
New patch sets uploaded. https://codereview.webrtc.org/2516993002/diff/180001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2516993002/diff/180001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:111: webrtc::MockAudioDecoderFactory::CreateUnusedPotemkinFactory(); On 2016/12/14 13:53:35, ossu wrote: > On 2016/12/14 13:09:36, kwiberg-webrtc wrote: > > On 2016/12/14 10:21:36, ossu wrote: > > > On 2016/12/14 08:58:24, the sun wrote: > > > > I guess that will be explained later, but I'm not sure everyone would > > > > immediately understand what Potemkin means > > > > > > I had to google it. That said, it's pretty witty. :) > > > > "I had to google it" and "pretty witty"---what more could you possibly want > from > > a class name? :-) > > > > If you have any good suggestions that will play well with our 80-column lines, > > I'm all ears. > > I think I'd just set up the mock manually in this function, possibly by basing > it on a MockAudioDecoderFactory gotten from CreateUnusedFactory(). I.e. adding > the ON_CALLs for GetSupportedDecoders and IsSupportedDecoder here. That said, if > we're basing it on the builtin factory, is there a reason to not just use the > builtin factory directly? Just making sure it never gets used to create a > decoder? > > If the tests require the specific list of codecs to match what we (for the time > being) support internally, I think it's fine to just put the builtin factory > there and add a TODO about changing/moving that test once we're further along > with the modularization. > > The CreateUnusedFactory and CreateEmptyFactory are there because sometimes you > just need to plug something in that's irrelevant to the test you're performing, > except that without it you might stumble on DCHECKs. Looking at it now, > CreateEmptyFactory isn't even used, so maybe it should go (not in this CL). I > don't think my intention was to set a precedent on how to set up mocks in > general. OK, using the normal factory and adding a TODO sounds good. (There are already somewhat overlapping TODOs on you later in this file, but I'll add one anyway.) https://codereview.webrtc.org/2516993002/diff/220001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/mock/mock_audio_decoder_factory.h (right): https://codereview.webrtc.org/2516993002/diff/220001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/mock/mock_audio_decoder_factory.h:66: .WillByDefault(testing::Return(std::vector<webrtc::AudioCodecSpec>())); On 2016/12/14 13:53:35, ossu wrote: > Why does it not propose any decoders but further down says it does support a > number of them? I mean, it's not strictly wrong, but strange. It's the minimal functionality required by the test. Moot point now, since the Potemkin factory is going away. (R.I.P., pretty name.)
AWESOMEPANTS!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_mips_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_mips_db...) android_compile_x86_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_rel...)
LGTM Solid! https://codereview.webrtc.org/2516993002/diff/260001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2516993002/diff/260001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine_unittest.cc:120: // factory. Those tests should probably be moved elsewhere. Yes, good call to not do that in this CL though. https://codereview.webrtc.org/2516993002/diff/260001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc (right): https://codereview.webrtc.org/2516993002/diff/260001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc:220: std::unique_ptr<AudioDecoder> dec; nit: dec -> decoder https://codereview.webrtc.org/2516993002/diff/260001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc:222: RTC_CHECK_EQ(ok, dec != nullptr); That's a DCHECK, since you're testing the logic, not a runtime condition. https://codereview.webrtc.org/2516993002/diff/260001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/mock/mock_audio_decoder_factory.h (right): https://codereview.webrtc.org/2516993002/diff/260001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/mock/mock_audio_decoder_factory.h:18: #include "webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.h" Why?
The CQ bit was checked by kwiberg@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/...
https://codereview.webrtc.org/2516993002/diff/260001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc (right): https://codereview.webrtc.org/2516993002/diff/260001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc:220: std::unique_ptr<AudioDecoder> dec; On 2016/12/15 15:34:13, the sun wrote: > nit: dec -> decoder Done. https://codereview.webrtc.org/2516993002/diff/260001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc:222: RTC_CHECK_EQ(ok, dec != nullptr); On 2016/12/15 15:34:13, the sun wrote: > That's a DCHECK, since you're testing the logic, not a runtime condition. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #16 (id:300001) has been deleted
The CQ bit was checked by kwiberg@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.
Drive-by reviewing! https://codereview.webrtc.org/2516993002/diff/320001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc (right): https://codereview.webrtc.org/2516993002/diff/320001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc:179: { { "opus", 48000, 2, { Should we use stereo by default w/ Opus? If so, I believe we need to set stereo=1 here. The RFC says: stereo: specifies whether the decoder prefers receiving stereo or mono signals. Possible values are 1 and 0, where 1 specifies that stereo signals are preferred, and 0 specifies that only mono signals are preferred. [...] If no value is specified, the default is 0 (mono).
The CQ bit was checked by kwiberg@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 checked by kwiberg@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.
The CQ bit was checked by kwiberg@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.
The CQ bit was checked by kwiberg@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.
The CQ bit was checked by kwiberg@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/...
Patchset #22 (id:440001) has been deleted
Patchset #22 (id:460001) has been deleted
OK, it looks like I've gotten all the bots to like this... take a look at patch sets 21 and 23 if you want.
https://codereview.webrtc.org/2516993002/diff/420001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2516993002/diff/420001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:1393: // Bug 6986: Emulate an old bug that caused us to always choose to decode nit: Add TODO
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/19 12:22:36, kwiberg-webrtc wrote: > OK, it looks like I've gotten all the bots to like this... take a look at patch > sets 21 and 23 if you want. I've looked through those changes and it lgtm. As the sun mentioned, a TODO in the comment about the bug would be nice. Good work!
https://codereview.webrtc.org/2516993002/diff/420001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2516993002/diff/420001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:1393: // Bug 6986: Emulate an old bug that caused us to always choose to decode On 2017/01/19 13:05:14, the sun wrote: > nit: Add TODO Done. I neglected to do this because I felt that mentioning it was a bug strongly implied that it should be removed, but I guess adding an explicit TODO can't hurt.
The CQ bit was checked by kwiberg@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org, ossu@webrtc.org Link to the patchset: https://codereview.webrtc.org/2516993002/#ps540001 (title: "add TODO")
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": 540001, "attempt_start_ts": 1484832535857030,
"parent_rev": "093dac142bcb097592f61165ddfb94a70d74f702", "commit_rev":
"d32bf7572138812dda9bd0028ac099a6e440fc61"}
Message was sent while issue was closed.
Description was changed from ========== Pass SdpAudioFormat through Channel, without converting to CodecInst BUG=webrtc:5805 ========== to ========== Pass SdpAudioFormat through Channel, without converting to CodecInst BUG=webrtc:5805 Review-Url: https://codereview.webrtc.org/2516993002 Cr-Commit-Position: refs/heads/master@{#16165} Committed: https://chromium.googlesource.com/external/webrtc/+/d32bf7572138812dda9bd0028... ==========
Message was sent while issue was closed.
Committed patchset #25 (id:540001) as https://chromium.googlesource.com/external/webrtc/+/d32bf7572138812dda9bd0028... |
