Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2072753002:
WebRtcVoiceEngine: Use AudioDecoderFactory to generate recv codecs. (Closed)
|
Created:
4 years, 6 months ago by ossu Modified:
4 years, 5 months ago CC:
webrtc-reviews_webrtc.org, peah-webrtc, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc, minyue-webrtc, the sun Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionWebRtcVoiceEngine: Use AudioDecoderFactory to generate recv codecs.
Changed WebRtcVoiceEngine to present receive codecs from the formats
provided by its decoder factory. Added supported formats to
BuiltinAudioDecoderFactory. Added helper functions for creating some
simple decoder factories for mocking.
Created a PayloadTypeMapper for assigning payload types to formats. I
think we'll eventually want to use this further up, or possibly in
both the audio and video sides. It would be best if the engines didn't
have to talk payload types at all, but it might be more difficult to
get around when payload types depend on each-other, like the RTX
codecs for video.
This CL also includes some changes to rtc::Optional. I've put them in
a separate CL that should (or should not) land first, making these
changes void.
See: https://codereview.webrtc.org/2072713002/
BUG=webrtc:5805
Committed: https://crrev.com/95eb1ba0db79d8fd134ae61b0a24648598684e8a
Cr-Commit-Position: refs/heads/master@{#13459}
Patch Set 1 #
Total comments: 27
Patch Set 2 : Rewrote PayloadMapperTest to not rely on value comparison operators of rtc::Optional. #
Total comments: 15
Patch Set 3 : Moved CN generation into main loop in CollectRecvCodecs. Also small cleanups. #Patch Set 4 : Fixed PayloadTypeMapper not increasing next_unused_payload_type_. #
Total comments: 5
Patch Set 5 : Combined map_format, used initializer list and improved PayloadTypeMapperTest. #
Total comments: 1
Patch Set 6 : Moved SdpAudioFormat comparator into PayloadTypeMapper for now. #Patch Set 7 : BuiltinAudioDecoderFactory::GetSupportedFormats now considers build flags, CN codecs ordered like before CL #
Total comments: 2
Patch Set 8 : #include <functional> #
Total comments: 8
Patch Set 9 : Rebase #Patch Set 10 : Addressed tommi's comments. #Messages
Total messages: 51 (28 generated)
Description was changed from ========== WebRtcVoiceEngine: Use AudioDecoderFactory to generate recv codecs. Changed WebRtcVoiceEngine to present receive codecs from the formats provided by its decoder factory. Added supported formats to BuiltinAudioDecoderFactory. Added helper functions for creating some simple decoder factories for mocking. Created a PayloadTypeMapper for assigning payload types to formats. I think we'll eventually want to use this further up, or possibly in both the audio and video sides. It would be best if the engines didn't have to talk payload types at all, but it might be more difficult to get around when payload types depend on each-other, like the RTX codecs for video. This CL also includes some changes to rtc::Optional. I've put them in a separate CL that should (or should not) land first, making these changes void. BUG=webrtc:5805 ========== to ========== WebRtcVoiceEngine: Use AudioDecoderFactory to generate recv codecs. Changed WebRtcVoiceEngine to present receive codecs from the formats provided by its decoder factory. Added supported formats to BuiltinAudioDecoderFactory. Added helper functions for creating some simple decoder factories for mocking. Created a PayloadTypeMapper for assigning payload types to formats. I think we'll eventually want to use this further up, or possibly in both the audio and video sides. It would be best if the engines didn't have to talk payload types at all, but it might be more difficult to get around when payload types depend on each-other, like the RTX codecs for video. This CL also includes some changes to rtc::Optional. I've put them in a separate CL that should (or should not) land first, making these changes void. See: https://codereview.webrtc.org/2072713002/ BUG=webrtc:5805 ==========
ossu@webrtc.org changed reviewers: + kwiberg@webrtc.org
More code reviews! \o_o/ https://codereview.webrtc.org/2072753002/diff/1/webrtc/base/optional.h File webrtc/base/optional.h (right): https://codereview.webrtc.org/2072753002/diff/1/webrtc/base/optional.h#newcode65 webrtc/base/optional.h:65: Optional(const T& value) : has_value_(true) { See: https://codereview.webrtc.org/2072713002/ https://codereview.webrtc.org/2072753002/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2072753002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1102: auto opt_codec = mapper.ToAudioCodec(format); Although we use the mapper here to assign payload types, it doesn't need to get it completely right. If there are any overlaps with, for example, video payload types, that will get resolved in mediasession.cc. I've tried to make PayloadTypeMapper return the same payload types as we'd otherwise use, and not step on any types we already use somewhere else, so as to not trigger some unforeseen change in payload types that may (it really shouldn't) break something somewhere. https://codereview.webrtc.org/2072753002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_format.h (right): https://codereview.webrtc.org/2072753002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_format.h:40: bool operator<(const SdpAudioFormat& b) const; So we can put them in a map/set. https://codereview.webrtc.org/2072753002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc (right): https://codereview.webrtc.org/2072753002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc:133: static std::vector<SdpAudioFormat> formats = { kwiberg: If you've got this in one of your CLs, maybe this one should be landed after that one.
Removed the rtc::Optional changes and rewrote the unittests to not rely on them, since they're now part of a bigger thing.
ossu@webrtc.org changed reviewers: + ivoc@webrtc.org, tina.legrand@webrtc.org - kwiberg@webrtc.org
Hi! I'm reassigning this one to you Ivo and Tina. You can fight among yourselves who will do the initial review. :) Thanks!
Looks like a nice change! See some comments below. https://codereview.webrtc.org/2072753002/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2072753002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:528: recv_codecs_ = CollectRecvCodecs(); So why can't this be part of WebRtcVoiceCodecs like the SupportedSendCodecs function? https://codereview.webrtc.org/2072753002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1092: auto map_format = [&mapper, &out] (const webrtc::SdpAudioFormat& format) { I don't really see the point of this lambda, and I think it can be eliminated since it does almost the same thing as the loop below. You can add {kDtmfCodecName, 8000, 1} to |formats| (eliminates the call on 1136), and on line 1120 you can add a check to test if the bool is currently false, and if so add {kCnCodecName, cn.first, 1} to |out| (eliminates the loop starting on 1129). https://codereview.webrtc.org/2072753002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1115: // we Looks like a strange linebreak here, please move "we" to the previous or next line. https://codereview.webrtc.org/2072753002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1120: if (cn_entry != generate_cn.end() /* && format.allow_comfort_noise */) { Remove commented code please. https://codereview.webrtc.org/2072753002/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.h (right): https://codereview.webrtc.org/2072753002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.h:146: std::vector<AudioCodec> recv_codecs_; Out of curiosity: in what scenario is it useful to have different sets of send/receive codecs? https://codereview.webrtc.org/2072753002/diff/20001/webrtc/media/engine/paylo... File webrtc/media/engine/payload_type_mapper.cc (right): https://codereview.webrtc.org/2072753002/diff/20001/webrtc/media/engine/paylo... webrtc/media/engine/payload_type_mapper.cc:92: int payload_type = next_unused_payload_type_; Shouldn't the value of next_unused_payload_type_ be updated below in this function? https://codereview.webrtc.org/2072753002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.h (right): https://codereview.webrtc.org/2072753002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.h:134: AudioCodecs CollectRecvCodecs() const; I would prefer not to abbreviate "Recv" in the method name (unless this is commonly done?). https://codereview.webrtc.org/2072753002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (left): https://codereview.webrtc.org/2072753002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:3407: // TODO(ossu): This test should move into the builtin audio codecs module So why doesn't it have to be moved anymore? https://codereview.webrtc.org/2072753002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2072753002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:3461: engine.send_codecs().begin(); Formatting looks a bit weird to me here, did you run git cl format? https://codereview.webrtc.org/2072753002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_format.cc (right): https://codereview.webrtc.org/2072753002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_format.cc:42: int name_cmp = STR_CASE_CMP(name.c_str(), b.name.c_str()); Isn't it possible to just do: if (name == b.name) here?
https://codereview.webrtc.org/2072753002/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2072753002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:528: recv_codecs_ = CollectRecvCodecs(); On 2016/07/05 14:43:50, ivoc wrote: > So why can't this be part of WebRtcVoiceCodecs like the SupportedSendCodecs > function? WebRtcVoiceCodecs uses hard-coded information to generate the set of codecs to use. Once we get the encoder side of this reworking in as well, WebRtcVoiceCodecs should go away and be replaced by a similar method to this one: CollectSendCodecs(). https://codereview.webrtc.org/2072753002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1092: auto map_format = [&mapper, &out] (const webrtc::SdpAudioFormat& format) { On 2016/07/05 14:43:50, ivoc wrote: > I don't really see the point of this lambda, and I think it can be eliminated > since it does almost the same thing as the loop below. > > You can add {kDtmfCodecName, 8000, 1} to |formats| (eliminates the call on > 1136), and on line 1120 you can add a check to test if the bool is currently > false, and if so add {kCnCodecName, cn.first, 1} to |out| (eliminates the loop > starting on 1129). Well, I'd like to use it for all calls to mapper.ToAudioCodec and I believe I can do so with a change to the loop below. I'd prefer not to change |formats| in this function. It's basically the input and could (should?) probably be const&. Is the use of a lambda in itself off-putting to you? You're right, though, I could add comfort noise codecs directly in the inner loop. If I were to imagine a reason for not doing so, it would be to keep the "proper" codecs grouped together. Maybe if we run out of payload types, it's better if the CN codecs get left out - not that that should ever happen. I'll give it a think! :) https://codereview.webrtc.org/2072753002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1107: // TODO(ossu): Should we set this specifically for just this codec? I've just gotten feedback (no pun intended) that it should be fine to add this for every codec, so I'll rewrite this bit to do just that. https://codereview.webrtc.org/2072753002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1115: // we On 2016/07/05 14:43:50, ivoc wrote: > Looks like a strange linebreak here, please move "we" to the previous or next > line. Yes, something went very wrong here. https://codereview.webrtc.org/2072753002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1120: if (cn_entry != generate_cn.end() /* && format.allow_comfort_noise */) { On 2016/07/05 14:43:50, ivoc wrote: > Remove commented code please. No, I'd rather not. It's directly related to the TODO above. Once we have this augmented information from the factory (it'll probably be a struct with a flag and an SdpAudioFormat in it) this is where that check'll be added. https://codereview.webrtc.org/2072753002/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.h (right): https://codereview.webrtc.org/2072753002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.h:146: std::vector<AudioCodec> recv_codecs_; On 2016/07/05 14:43:51, ivoc wrote: > Out of curiosity: in what scenario is it useful to have different sets of > send/receive codecs? Well, it's largely a design choice: since we've got two separate factories, possibly from two different sources, we'll need to handle the possibility that they return different things. From a practical standpoint, if you're designing some sort of WebRTC client that serves a limited purpose, like, say, only broadcasting, it could (eventually) be built without a single decoder. There could also be cases where you're limited by hardware, and you're perhaps not able to encode some formats that you are able to decode. If using something other than SDP for signalling, it should be possible to actually set up two-way communication with different codecs in each direction. https://codereview.webrtc.org/2072753002/diff/20001/webrtc/media/engine/paylo... File webrtc/media/engine/payload_type_mapper.cc (right): https://codereview.webrtc.org/2072753002/diff/20001/webrtc/media/engine/paylo... webrtc/media/engine/payload_type_mapper.cc:92: int payload_type = next_unused_payload_type_; On 2016/07/05 14:43:51, ivoc wrote: > Shouldn't the value of next_unused_payload_type_ be updated below in this > function? Yup! https://codereview.webrtc.org/2072753002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine.h (right): https://codereview.webrtc.org/2072753002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine.h:134: AudioCodecs CollectRecvCodecs() const; On 2016/07/05 14:43:51, ivoc wrote: > I would prefer not to abbreviate "Recv" in the method name (unless this is > commonly done?). It's done eeeeverywhere, except where it isn't. :) We really should move for uniformity at some point. In this part of the code, it's common with recv, though, and it does update recv_codecs_, which makes its name apt. https://codereview.webrtc.org/2072753002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (left): https://codereview.webrtc.org/2072753002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:3407: // TODO(ossu): This test should move into the builtin audio codecs module On 2016/07/05 14:43:51, ivoc wrote: > So why doesn't it have to be moved anymore? Well it does, but it doesn't need two comments saying that. :) (There's one just inside the function as well - a botched merge, I reckon.) https://codereview.webrtc.org/2072753002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2072753002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:3461: engine.send_codecs().begin(); On 2016/07/05 14:43:51, ivoc wrote: > Formatting looks a bit weird to me here, did you run git cl format? I'm not sure. I'll look into it. https://codereview.webrtc.org/2072753002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_format.cc (right): https://codereview.webrtc.org/2072753002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_format.cc:42: int name_cmp = STR_CASE_CMP(name.c_str(), b.name.c_str()); On 2016/07/05 14:43:51, ivoc wrote: > Isn't it possible to just do: if (name == b.name) here? Well, no, we allow any capitalization of format names in the SDP and it's reflected by doing case-insensitive comparisons everywhere. Maybe a normalizing step would be better at some point? I'm not sure.
https://codereview.webrtc.org/2072753002/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2072753002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1107: // TODO(ossu): Should we set this specifically for just this codec? On 2016/07/05 15:31:56, ossu wrote: > I've just gotten feedback (no pun intended) that it should be fine to add this > for every codec, so I'll rewrite this bit to do just that. Turns out that, no, I still can't get this to use map_format cleanly, and I'd also rather avoid functionality changes in this CL. It'll come in as a different change, later. https://codereview.webrtc.org/2072753002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2072753002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:3461: engine.send_codecs().begin(); On 2016/07/05 15:31:57, ossu wrote: > On 2016/07/05 14:43:51, ivoc wrote: > > Formatting looks a bit weird to me here, did you run git cl format? > > I'm not sure. I'll look into it. Yeah, this is what clang-format wants. :/
https://codereview.webrtc.org/2072753002/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2072753002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:528: recv_codecs_ = CollectRecvCodecs(); On 2016/07/05 15:31:56, ossu wrote: > On 2016/07/05 14:43:50, ivoc wrote: > > So why can't this be part of WebRtcVoiceCodecs like the SupportedSendCodecs > > function? > > WebRtcVoiceCodecs uses hard-coded information to generate the set of codecs to > use. Once we get the encoder side of this reworking in as well, > WebRtcVoiceCodecs should go away and be replaced by a similar method to this > one: CollectSendCodecs(). Sorry, I'm probably missing the big picture here :-) https://codereview.webrtc.org/2072753002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1092: auto map_format = [&mapper, &out] (const webrtc::SdpAudioFormat& format) { On 2016/07/05 15:31:56, ossu wrote: > On 2016/07/05 14:43:50, ivoc wrote: > > I don't really see the point of this lambda, and I think it can be eliminated > > since it does almost the same thing as the loop below. > > > > You can add {kDtmfCodecName, 8000, 1} to |formats| (eliminates the call on > > 1136), and on line 1120 you can add a check to test if the bool is currently > > false, and if so add {kCnCodecName, cn.first, 1} to |out| (eliminates the loop > > starting on 1129). > > Well, I'd like to use it for all calls to mapper.ToAudioCodec and I believe I > can do so with a change to the loop below. I'd prefer not to change |formats| in > this function. It's basically the input and could (should?) probably be const&. > Is the use of a lambda in itself off-putting to you? > > You're right, though, I could add comfort noise codecs directly in the inner > loop. If I were to imagine a reason for not doing so, it would be to keep the > "proper" codecs grouped together. Maybe if we run out of payload types, it's > better if the CN codecs get left out - not that that should ever happen. I'll > give it a think! :) It's not that I mind lambda's (I kinda like them), but more that the code in the lambda looks pretty similar to the loop below, so I thought it would be nice to get rid of some duplication if possible. Would it be possible to get rid of the loop and put the functionality in map_format instead? But maybe I'm wrong and it's just a bad idea. https://codereview.webrtc.org/2072753002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1120: if (cn_entry != generate_cn.end() /* && format.allow_comfort_noise */) { On 2016/07/05 15:31:56, ossu wrote: > On 2016/07/05 14:43:50, ivoc wrote: > > Remove commented code please. > > No, I'd rather not. It's directly related to the TODO above. Once we have this > augmented information from the factory (it'll probably be a struct with a flag > and an SdpAudioFormat in it) this is where that check'll be added. Hmm, somehow I was sure this was in the style guide, but I just tried to look for it and couldn't find it. I guess you can keep your commented code then. :-) https://codereview.webrtc.org/2072753002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_format.h (right): https://codereview.webrtc.org/2072753002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_format.h:40: bool operator<(const SdpAudioFormat& b) const; On 2016/06/16 16:21:24, ossu wrote: > So we can put them in a map/set. Do they need to be sorted? If not, how about unordered_map/unordered_set instead? https://codereview.webrtc.org/2072753002/diff/20001/webrtc/media/engine/paylo... File webrtc/media/engine/payload_type_mapper.cc (right): https://codereview.webrtc.org/2072753002/diff/20001/webrtc/media/engine/paylo... webrtc/media/engine/payload_type_mapper.cc:92: int payload_type = next_unused_payload_type_; On 2016/07/05 15:31:57, ossu wrote: > On 2016/07/05 14:43:51, ivoc wrote: > > Shouldn't the value of next_unused_payload_type_ be updated below in this > > function? > > Yup! So... why isn't it? :-) https://codereview.webrtc.org/2072753002/diff/20001/webrtc/media/engine/webrt... File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right): https://codereview.webrtc.org/2072753002/diff/20001/webrtc/media/engine/webrt... webrtc/media/engine/webrtcvoiceengine_unittest.cc:3461: engine.send_codecs().begin(); On 2016/07/06 13:20:15, ossu wrote: > On 2016/07/05 15:31:57, ossu wrote: > > On 2016/07/05 14:43:51, ivoc wrote: > > > Formatting looks a bit weird to me here, did you run git cl format? > > > > I'm not sure. I'll look into it. > > Yeah, this is what clang-format wants. :/ I guess we'll have to live with it then. :/ https://codereview.webrtc.org/2072753002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_format.cc (right): https://codereview.webrtc.org/2072753002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_format.cc:42: int name_cmp = STR_CASE_CMP(name.c_str(), b.name.c_str()); On 2016/07/05 15:31:57, ossu wrote: > On 2016/07/05 14:43:51, ivoc wrote: > > Isn't it possible to just do: if (name == b.name) here? > > Well, no, we allow any capitalization of format names in the SDP and it's > reflected by doing case-insensitive comparisons everywhere. Maybe a normalizing > step would be better at some point? I'm not sure. Sounds like it would be nice to just convert everything to lower case at some point then, but okay.
https://codereview.webrtc.org/2072753002/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2072753002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:528: recv_codecs_ = CollectRecvCodecs(); On 2016/07/06 15:24:50, ivoc wrote: > On 2016/07/05 15:31:56, ossu wrote: > > On 2016/07/05 14:43:50, ivoc wrote: > > > So why can't this be part of WebRtcVoiceCodecs like the SupportedSendCodecs > > > function? > > > > WebRtcVoiceCodecs uses hard-coded information to generate the set of codecs to > > use. Once we get the encoder side of this reworking in as well, > > WebRtcVoiceCodecs should go away and be replaced by a similar method to this > > one: CollectSendCodecs(). > > Sorry, I'm probably missing the big picture here :-) No worries. :) https://codereview.webrtc.org/2072753002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1092: auto map_format = [&mapper, &out] (const webrtc::SdpAudioFormat& format) { On 2016/07/06 15:24:51, ivoc wrote: > On 2016/07/05 15:31:56, ossu wrote: > > On 2016/07/05 14:43:50, ivoc wrote: > > > I don't really see the point of this lambda, and I think it can be > eliminated > > > since it does almost the same thing as the loop below. > > > > > > You can add {kDtmfCodecName, 8000, 1} to |formats| (eliminates the call on > > > 1136), and on line 1120 you can add a check to test if the bool is currently > > > false, and if so add {kCnCodecName, cn.first, 1} to |out| (eliminates the > loop > > > starting on 1129). > > > > Well, I'd like to use it for all calls to mapper.ToAudioCodec and I believe I > > can do so with a change to the loop below. I'd prefer not to change |formats| > in > > this function. It's basically the input and could (should?) probably be > const&. > > Is the use of a lambda in itself off-putting to you? > > > > You're right, though, I could add comfort noise codecs directly in the inner > > loop. If I were to imagine a reason for not doing so, it would be to keep the > > "proper" codecs grouped together. Maybe if we run out of payload types, it's > > better if the CN codecs get left out - not that that should ever happen. I'll > > give it a think! :) > > It's not that I mind lambda's (I kinda like them), but more that the code in the > lambda looks pretty similar to the loop below, so I thought it would be nice to > get rid of some duplication if possible. Would it be possible to get rid of the > loop and put the functionality in map_format instead? But maybe I'm wrong and > it's just a bad idea. Well, the lambda deals with the possibility that the mapping may fail when creating a mapping for a single format, whereas the loop maps all formats. Once the infrastructure is in place, I think we could put the transport-cc feedback parameter on all codecs as with a single rtcp-fb line using a wildcard. https://codereview.webrtc.org/2072753002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1120: if (cn_entry != generate_cn.end() /* && format.allow_comfort_noise */) { On 2016/07/06 15:24:51, ivoc wrote: > On 2016/07/05 15:31:56, ossu wrote: > > On 2016/07/05 14:43:50, ivoc wrote: > > > Remove commented code please. > > > > No, I'd rather not. It's directly related to the TODO above. Once we have this > > augmented information from the factory (it'll probably be a struct with a flag > > and an SdpAudioFormat in it) this is where that check'll be added. > > Hmm, somehow I was sure this was in the style guide, but I just tried to look > for it and couldn't find it. I guess you can keep your commented code then. :-) Well, I agree with you that we shouldn't keep commented code laying around. I feel this is special since it documents the change in the TODO, it's not just something that we've commented out because we don't want it anymore. https://codereview.webrtc.org/2072753002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_format.h (right): https://codereview.webrtc.org/2072753002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_format.h:40: bool operator<(const SdpAudioFormat& b) const; On 2016/07/06 15:24:51, ivoc wrote: > On 2016/06/16 16:21:24, ossu wrote: > > So we can put them in a map/set. > > Do they need to be sorted? If not, how about unordered_map/unordered_set > instead? For unordered_map, we'd need to define a hasher instead, I believe. Having operator< here allows us to put it in sets or maps, which is generally a nice thing to be able to do, and generally what's done. Only a handful of places in our code use unordered_map. https://codereview.webrtc.org/2072753002/diff/20001/webrtc/media/engine/paylo... File webrtc/media/engine/payload_type_mapper.cc (right): https://codereview.webrtc.org/2072753002/diff/20001/webrtc/media/engine/paylo... webrtc/media/engine/payload_type_mapper.cc:92: int payload_type = next_unused_payload_type_; On 2016/07/06 15:24:51, ivoc wrote: > On 2016/07/05 15:31:57, ossu wrote: > > On 2016/07/05 14:43:51, ivoc wrote: > > > Shouldn't the value of next_unused_payload_type_ be updated below in this > > > function? > > > > Yup! > > So... why isn't it? :-) Just wanted to check if you were awake. :) But really I forgot :(
https://codereview.webrtc.org/2072753002/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2072753002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1092: auto map_format = [&mapper, &out] (const webrtc::SdpAudioFormat& format) { On 2016/07/06 16:23:25, ossu wrote: > On 2016/07/06 15:24:51, ivoc wrote: > > On 2016/07/05 15:31:56, ossu wrote: > > > On 2016/07/05 14:43:50, ivoc wrote: > > > > I don't really see the point of this lambda, and I think it can be > > eliminated > > > > since it does almost the same thing as the loop below. > > > > > > > > You can add {kDtmfCodecName, 8000, 1} to |formats| (eliminates the call on > > > > 1136), and on line 1120 you can add a check to test if the bool is > currently > > > > false, and if so add {kCnCodecName, cn.first, 1} to |out| (eliminates the > > loop > > > > starting on 1129). > > > > > > Well, I'd like to use it for all calls to mapper.ToAudioCodec and I believe > I > > > can do so with a change to the loop below. I'd prefer not to change > |formats| > > in > > > this function. It's basically the input and could (should?) probably be > > const&. > > > Is the use of a lambda in itself off-putting to you? > > > > > > You're right, though, I could add comfort noise codecs directly in the inner > > > loop. If I were to imagine a reason for not doing so, it would be to keep > the > > > "proper" codecs grouped together. Maybe if we run out of payload types, it's > > > better if the CN codecs get left out - not that that should ever happen. > I'll > > > give it a think! :) > > > > It's not that I mind lambda's (I kinda like them), but more that the code in > the > > lambda looks pretty similar to the loop below, so I thought it would be nice > to > > get rid of some duplication if possible. Would it be possible to get rid of > the > > loop and put the functionality in map_format instead? But maybe I'm wrong and > > it's just a bad idea. > > Well, the lambda deals with the possibility that the mapping may fail when > creating a mapping for a single format, whereas the loop maps all formats. Once > the infrastructure is in place, I think we could put the transport-cc feedback > parameter on all codecs as with a single rtcp-fb line using a wildcard. It looks to me like the only difference between the lambda and what happens in each iteration of the loop is lines 1106-1111 and lines 1114-1122. I think it would be safe to add those lines to the lambda, and instead of the loop just call the lambda for each element in the vector. There would be no duplicated code that way, WDYT? https://codereview.webrtc.org/2072753002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_format.h (right): https://codereview.webrtc.org/2072753002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_format.h:40: bool operator<(const SdpAudioFormat& b) const; On 2016/07/06 16:23:25, ossu wrote: > On 2016/07/06 15:24:51, ivoc wrote: > > On 2016/06/16 16:21:24, ossu wrote: > > > So we can put them in a map/set. > > > > Do they need to be sorted? If not, how about unordered_map/unordered_set > > instead? > > For unordered_map, we'd need to define a hasher instead, I believe. Having > operator< here allows us to put it in sets or maps, which is generally a nice > thing to be able to do, and generally what's done. Only a handful of places in > our code use unordered_map. Oh right, I didn't realize that, I guess I learned something new :-). https://codereview.webrtc.org/2072753002/diff/60001/webrtc/media/engine/paylo... File webrtc/media/engine/payload_type_mapper.cc (right): https://codereview.webrtc.org/2072753002/diff/60001/webrtc/media/engine/paylo... webrtc/media/engine/payload_type_mapper.cc:21: mappings_[{"PCMU", 8000, 1}] = 0; How about an initializer list here? https://codereview.webrtc.org/2072753002/diff/60001/webrtc/media/engine/paylo... File webrtc/media/engine/payload_type_mapper_unittest.cc (right): https://codereview.webrtc.org/2072753002/diff/60001/webrtc/media/engine/paylo... webrtc/media/engine/payload_type_mapper_unittest.cc:107: std::set<int> used_payload_types; I think it would be good to add a check at the end of the test to see if used_payload_types has the correct amount of items (or that its size is greater than zero if we don't want to fix an exact amount).
https://codereview.webrtc.org/2072753002/diff/1/webrtc/media/engine/webrtcvoi... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2072753002/diff/1/webrtc/media/engine/webrtcvoi... webrtc/media/engine/webrtcvoiceengine.cc:1092: auto map_format = [&mapper, &out] (const webrtc::SdpAudioFormat& format) { On 2016/07/08 12:50:32, ivoc wrote: > On 2016/07/06 16:23:25, ossu wrote: > > On 2016/07/06 15:24:51, ivoc wrote: > > > On 2016/07/05 15:31:56, ossu wrote: > > > > On 2016/07/05 14:43:50, ivoc wrote: > > > > > I don't really see the point of this lambda, and I think it can be > > > eliminated > > > > > since it does almost the same thing as the loop below. > > > > > > > > > > You can add {kDtmfCodecName, 8000, 1} to |formats| (eliminates the call > on > > > > > 1136), and on line 1120 you can add a check to test if the bool is > > currently > > > > > false, and if so add {kCnCodecName, cn.first, 1} to |out| (eliminates > the > > > loop > > > > > starting on 1129). > > > > > > > > Well, I'd like to use it for all calls to mapper.ToAudioCodec and I > believe > > I > > > > can do so with a change to the loop below. I'd prefer not to change > > |formats| > > > in > > > > this function. It's basically the input and could (should?) probably be > > > const&. > > > > Is the use of a lambda in itself off-putting to you? > > > > > > > > You're right, though, I could add comfort noise codecs directly in the > inner > > > > loop. If I were to imagine a reason for not doing so, it would be to keep > > the > > > > "proper" codecs grouped together. Maybe if we run out of payload types, > it's > > > > better if the CN codecs get left out - not that that should ever happen. > > I'll > > > > give it a think! :) > > > > > > It's not that I mind lambda's (I kinda like them), but more that the code in > > the > > > lambda looks pretty similar to the loop below, so I thought it would be nice > > to > > > get rid of some duplication if possible. Would it be possible to get rid of > > the > > > loop and put the functionality in map_format instead? But maybe I'm wrong > and > > > it's just a bad idea. > > > > Well, the lambda deals with the possibility that the mapping may fail when > > creating a mapping for a single format, whereas the loop maps all formats. > Once > > the infrastructure is in place, I think we could put the transport-cc feedback > > parameter on all codecs as with a single rtcp-fb line using a wildcard. > > It looks to me like the only difference between the lambda and what happens in > each iteration of the loop is lines 1106-1111 and lines 1114-1122. I think it > would be safe to add those lines to the lambda, and instead of the loop just > call the lambda for each element in the vector. There would be no duplicated > code that way, WDYT? Yeah, I guess you're right. With the check for Opus in it, it's fine if we send in other types of codecs as well. It just won't apply to CN or DTMF either. https://codereview.webrtc.org/2072753002/diff/60001/webrtc/media/engine/paylo... File webrtc/media/engine/payload_type_mapper.cc (right): https://codereview.webrtc.org/2072753002/diff/60001/webrtc/media/engine/paylo... webrtc/media/engine/payload_type_mapper.cc:21: mappings_[{"PCMU", 8000, 1}] = 0; On 2016/07/08 12:50:32, ivoc wrote: > How about an initializer list here? Do you think that would make it more readable? I'll give it a try to see how it looks. https://codereview.webrtc.org/2072753002/diff/60001/webrtc/media/engine/paylo... File webrtc/media/engine/payload_type_mapper_unittest.cc (right): https://codereview.webrtc.org/2072753002/diff/60001/webrtc/media/engine/paylo... webrtc/media/engine/payload_type_mapper_unittest.cc:107: std::set<int> used_payload_types; On 2016/07/08 12:50:32, ivoc wrote: > I think it would be good to add a check at the end of the test to see if > used_payload_types has the correct amount of items (or that its size is greater > than zero if we don't want to fix an exact amount). Ah, to check that we've mapped anything? Makes sense. At this point, I guess the test would succeed without actually supporting any dynamic types. https://codereview.webrtc.org/2072753002/diff/80001/webrtc/media/engine/paylo... File webrtc/media/engine/payload_type_mapper.h (right): https://codereview.webrtc.org/2072753002/diff/80001/webrtc/media/engine/paylo... webrtc/media/engine/payload_type_mapper.h:42: int next_unused_payload_type_; To make the constructor initializer less horrible to read while keeping things ordered.
After reading up on the style guide, I moved the SdpAudioFormat comparator into PayloadTypeMapper, rather than implementing it as an operator<(). I think it probably defines a natural enough ordering for SdpAudioFormats (which is one of the criteria for making an operator<) but it should be accompanied by all the other comparison operators as well. I might do that when/if it turns out we need it in more places.
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: android_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/14822)
LGTM, nice work :) https://codereview.webrtc.org/2072753002/diff/60001/webrtc/media/engine/paylo... File webrtc/media/engine/payload_type_mapper.cc (right): https://codereview.webrtc.org/2072753002/diff/60001/webrtc/media/engine/paylo... webrtc/media/engine/payload_type_mapper.cc:21: mappings_[{"PCMU", 8000, 1}] = 0; On 2016/07/08 14:39:14, ossu wrote: > On 2016/07/08 12:50:32, ivoc wrote: > > How about an initializer list here? > > Do you think that would make it more readable? I'll give it a try to see how it > looks. Looks glorious :-)
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/...
Thanks! :) Bumped into a couple of issues highlighted when running the tests on android. They've been addressed in the latest patch set. Nothing major, but I did put CN codecs back after all the "normal" ones. https://codereview.webrtc.org/2072753002/diff/120001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2072753002/diff/120001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:1125: // Add CN codecs after "proper" audio codecs I put these back after all the regular codecs, and in decreasing order of clockrate, to mimic how they were previously ordered. It also makes sense, since the list is supposed to be in priority order and in no case should a comfort noise codec be higher priority than a "normal" one. https://codereview.webrtc.org/2072753002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc (right): https://codereview.webrtc.org/2072753002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc:144: #if (defined(WEBRTC_CODEC_ISAC)) This is why the tests failed on android: we pretended we could support isac at 32k when we really couldn't.
The CQ bit was unchecked by commit-bot@chromium.org to run a CQ dry run
Dry run: Try jobs failed on following builders: win_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/12670) win_x64_clang_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_dbg/build...)
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 to run a CQ dry run
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ossu@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from ivoc@webrtc.org Link to the patchset: https://codereview.webrtc.org/2072753002/#ps140001 (title: "#include <functional>")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/6832)
ossu@webrtc.org changed reviewers: + tommi@webrtc.org
Hey! This one needs owner approval and most people are on vacation. PTAL.
rs lgtm % nits and questions https://codereview.webrtc.org/2072753002/diff/140001/webrtc/media/engine/payl... File webrtc/media/engine/payload_type_mapper.cc (right): https://codereview.webrtc.org/2072753002/diff/140001/webrtc/media/engine/payl... webrtc/media/engine/payload_type_mapper.cc:19: : next_unused_payload_type_(96), what's this number based on? If it can be derived at compile time by the compiler so that it stays in sync with the data it's based on, that would be preferable. https://codereview.webrtc.org/2072753002/diff/140001/webrtc/media/engine/payl... webrtc/media/engine/payload_type_mapper.cc:20: max_payload_type_(127), max signed 8 bit integer? (or something else?) https://codereview.webrtc.org/2072753002/diff/140001/webrtc/media/engine/payl... webrtc/media/engine/payload_type_mapper.cc:95: if (iter != mappings_.end()) { nit: consistent {} or no {} https://codereview.webrtc.org/2072753002/diff/140001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcmediaengine.h (right): https://codereview.webrtc.org/2072753002/diff/140001/webrtc/media/engine/webr... webrtc/media/engine/webrtcmediaengine.h:22: class AudioDecoderFactory; sort
Patchset #9 (id:160001) has been deleted
https://codereview.webrtc.org/2072753002/diff/140001/webrtc/media/engine/payl... File webrtc/media/engine/payload_type_mapper.cc (right): https://codereview.webrtc.org/2072753002/diff/140001/webrtc/media/engine/payl... webrtc/media/engine/payload_type_mapper.cc:19: : next_unused_payload_type_(96), On 2016/07/13 11:23:58, tommi-webrtc wrote: > what's this number based on? If it can be derived at compile time by the > compiler so that it stays in sync with the data it's based on, that would be > preferable. It's based on RFC 3551: "This profile reserves payload type numbers in the range 96-127 exclusively for dynamic assignment." I could add a comment about that to make it clearer. https://codereview.webrtc.org/2072753002/diff/140001/webrtc/media/engine/payl... webrtc/media/engine/payload_type_mapper.cc:20: max_payload_type_(127), On 2016/07/13 11:23:58, tommi-webrtc wrote: > max signed 8 bit integer? (or something else?) Other parts of the code have local kMaxPayloadType or kMaxPayloadId, or sometimes just 127. It's not a value that's liable to change or really depends on the data type used right here. Would it be worth it to introduce a local constant just to use it here? I could make one in mediaconstants.h and change the other uses in another CL. (not promising that I'll do that CL right-away, though). https://codereview.webrtc.org/2072753002/diff/140001/webrtc/media/engine/payl... webrtc/media/engine/payload_type_mapper.cc:95: if (iter != mappings_.end()) { On 2016/07/13 11:23:58, tommi-webrtc wrote: > nit: consistent {} or no {} Acknowledged. https://codereview.webrtc.org/2072753002/diff/140001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcmediaengine.h (right): https://codereview.webrtc.org/2072753002/diff/140001/webrtc/media/engine/webr... webrtc/media/engine/webrtcmediaengine.h:22: class AudioDecoderFactory; On 2016/07/13 11:23:58, tommi-webrtc wrote: > sort Acknowledged.
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/...
On 2016/07/13 11:48:17, ossu wrote: > https://codereview.webrtc.org/2072753002/diff/140001/webrtc/media/engine/payl... > File webrtc/media/engine/payload_type_mapper.cc (right): > > https://codereview.webrtc.org/2072753002/diff/140001/webrtc/media/engine/payl... > webrtc/media/engine/payload_type_mapper.cc:19: : next_unused_payload_type_(96), > On 2016/07/13 11:23:58, tommi-webrtc wrote: > > what's this number based on? If it can be derived at compile time by the > > compiler so that it stays in sync with the data it's based on, that would be > > preferable. > > It's based on RFC 3551: > "This profile reserves payload type numbers in the range 96-127 exclusively for > dynamic assignment." > > I could add a comment about that to make it clearer. > > https://codereview.webrtc.org/2072753002/diff/140001/webrtc/media/engine/payl... > webrtc/media/engine/payload_type_mapper.cc:20: max_payload_type_(127), > On 2016/07/13 11:23:58, tommi-webrtc wrote: > > max signed 8 bit integer? (or something else?) > > Other parts of the code have local kMaxPayloadType or kMaxPayloadId, or > sometimes just 127. It's not a value that's liable to change or really depends > on the data type used right here. Would it be worth it to introduce a local > constant just to use it here? I could make one in mediaconstants.h and change > the other uses in another CL. (not promising that I'll do that CL right-away, > though). > > https://codereview.webrtc.org/2072753002/diff/140001/webrtc/media/engine/payl... > webrtc/media/engine/payload_type_mapper.cc:95: if (iter != mappings_.end()) { > On 2016/07/13 11:23:58, tommi-webrtc wrote: > > nit: consistent {} or no {} > > Acknowledged. > > https://codereview.webrtc.org/2072753002/diff/140001/webrtc/media/engine/webr... > File webrtc/media/engine/webrtcmediaengine.h (right): > > https://codereview.webrtc.org/2072753002/diff/140001/webrtc/media/engine/webr... > webrtc/media/engine/webrtcmediaengine.h:22: class AudioDecoderFactory; > On 2016/07/13 11:23:58, tommi-webrtc wrote: > > sort > > Acknowledged. Thanks - yes, documenting the values would at least help with wondering code reviewers.
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 ossu@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from ivoc@webrtc.org, tommi@webrtc.org Link to the patchset: https://codereview.webrtc.org/2072753002/#ps200001 (title: "Addressed tommi's comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== WebRtcVoiceEngine: Use AudioDecoderFactory to generate recv codecs. Changed WebRtcVoiceEngine to present receive codecs from the formats provided by its decoder factory. Added supported formats to BuiltinAudioDecoderFactory. Added helper functions for creating some simple decoder factories for mocking. Created a PayloadTypeMapper for assigning payload types to formats. I think we'll eventually want to use this further up, or possibly in both the audio and video sides. It would be best if the engines didn't have to talk payload types at all, but it might be more difficult to get around when payload types depend on each-other, like the RTX codecs for video. This CL also includes some changes to rtc::Optional. I've put them in a separate CL that should (or should not) land first, making these changes void. See: https://codereview.webrtc.org/2072713002/ BUG=webrtc:5805 ========== to ========== WebRtcVoiceEngine: Use AudioDecoderFactory to generate recv codecs. Changed WebRtcVoiceEngine to present receive codecs from the formats provided by its decoder factory. Added supported formats to BuiltinAudioDecoderFactory. Added helper functions for creating some simple decoder factories for mocking. Created a PayloadTypeMapper for assigning payload types to formats. I think we'll eventually want to use this further up, or possibly in both the audio and video sides. It would be best if the engines didn't have to talk payload types at all, but it might be more difficult to get around when payload types depend on each-other, like the RTX codecs for video. This CL also includes some changes to rtc::Optional. I've put them in a separate CL that should (or should not) land first, making these changes void. See: https://codereview.webrtc.org/2072713002/ BUG=webrtc:5805 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== WebRtcVoiceEngine: Use AudioDecoderFactory to generate recv codecs. Changed WebRtcVoiceEngine to present receive codecs from the formats provided by its decoder factory. Added supported formats to BuiltinAudioDecoderFactory. Added helper functions for creating some simple decoder factories for mocking. Created a PayloadTypeMapper for assigning payload types to formats. I think we'll eventually want to use this further up, or possibly in both the audio and video sides. It would be best if the engines didn't have to talk payload types at all, but it might be more difficult to get around when payload types depend on each-other, like the RTX codecs for video. This CL also includes some changes to rtc::Optional. I've put them in a separate CL that should (or should not) land first, making these changes void. See: https://codereview.webrtc.org/2072713002/ BUG=webrtc:5805 ========== to ========== WebRtcVoiceEngine: Use AudioDecoderFactory to generate recv codecs. Changed WebRtcVoiceEngine to present receive codecs from the formats provided by its decoder factory. Added supported formats to BuiltinAudioDecoderFactory. Added helper functions for creating some simple decoder factories for mocking. Created a PayloadTypeMapper for assigning payload types to formats. I think we'll eventually want to use this further up, or possibly in both the audio and video sides. It would be best if the engines didn't have to talk payload types at all, but it might be more difficult to get around when payload types depend on each-other, like the RTX codecs for video. This CL also includes some changes to rtc::Optional. I've put them in a separate CL that should (or should not) land first, making these changes void. See: https://codereview.webrtc.org/2072713002/ BUG=webrtc:5805 Committed: https://crrev.com/95eb1ba0db79d8fd134ae61b0a24648598684e8a Cr-Commit-Position: refs/heads/master@{#13459} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/95eb1ba0db79d8fd134ae61b0a24648598684e8a Cr-Commit-Position: refs/heads/master@{#13459}
Message was sent while issue was closed.
A revert of this CL (patchset #10 id:200001) has been created in https://codereview.webrtc.org/2151453002/ by ossu@webrtc.org. The reason for reverting is: For some reason, payload_type_mapper.cc is not being picked up in Chrome builds, leading to undefined references. Reverting while investigating.. |
