Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1917163002:
New interface (AudioDecoderFactory), with an implementation (Closed)
|
Created:
4 years, 8 months ago by kwiberg-webrtc Modified:
4 years, 7 months ago CC:
webrtc-reviews_webrtc.org, kwiberg-webrtc, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, peah-webrtc, minyue-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionNew interface (AudioDecoderFactory), with an implementation
This is a first draft of what we're hoping to use to create all
AudioDecoder instances. Follow-up CLs will start using this internally
in NetEq instead of calling constructors manually.
BUG=webrtc:5801
Committed: https://crrev.com/c01c6a423cfe60e2d0bf1f700bdc331ba1d29572
Cr-Commit-Position: refs/heads/master@{#12548}
Patch Set 1 #
Total comments: 46
Patch Set 2 : review #Messages
Total messages: 17 (4 generated)
kwiberg@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, ossu@webrtc.org, solenberg@webrtc.org
On 2016/04/26 22:56:09, kwiberg-webrtc wrote: LGTM. I've really no comments at this point. :)
LGTM with a number of minor comments. https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_coding.gypi (right): https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_coding.gypi:72: 'target_name': 'audio_codec_factory_interface', I assume you are intending to extend this target (and the one below) with encoder capabilities too. Otherwise, the naming is off. https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_format.cc (right): https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_format.cc:41: const char* sep = ""; I think the const qualifier is confusing here, since you are in fact changing the separator string further down. (I realize that you are not violating the constness, since you are setting a new value for the (non-const) pointer, not modifying the (const) pointed data.) https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_format.h (right): https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_format.h:29: SdpAudioFormat(const char* name, int clockrate_hz, int num_channels); I would prefer to rename clockrate_hz to rtp_timestamp_rate_hz. https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc (right): https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc:46: std::unique_ptr<AudioDecoder> Uniq(AudioDecoder* d) { Did you mean to name it Unique, or were you paying tribute to the linux/unix-inspired Uniq command? https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc:103: return Uniq(nullptr); Why Uniq(nullptr) here, but not in other places? https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.h (right): https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.h:20: // Create a new factory that can create the built-in types of audio decoders. Creates...
https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/B... File webrtc/modules/audio_coding/BUILD.gn (right): https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/B... webrtc/modules/audio_coding/BUILD.gn:55: source_set("builtin_audio_codec_factory") { _factories? Or is the intent to have a single class implementing both enc and dec factory? https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_coding.gypi (right): https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_coding.gypi:92: 'target_name': 'builtin_audio_codec_factory', _factories? https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_decoder_factory_unittest.cc (right): https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder_factory_unittest.cc:25: EXPECT_FALSE(adf->MakeAudioDecoder(SdpAudioFormat("pcmu", 8000, 0))); It would be good to have documentation along the lines of: // pcmu supports only 8kHz clock rate, with 1 to 3 channels. https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder_factory_unittest.cc:58: EXPECT_FALSE(adf->MakeAudioDecoder(SdpAudioFormat("isac", 48000, 1))); ch count 3? https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder_factory_unittest.cc:72: for (int channels = 1; channels < 10; ++channels) { Do we really need to test all channel counts? Can we just test 1, 2 and "many"? https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_format.h (right): https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_format.h:23: struct SdpAudioFormat { How about naming this EncodedAudioFormat instead? https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_format.h:26: SdpAudioFormat(); I'd prefer if you put all the "= default" in here. https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_format.h:29: SdpAudioFormat(const char* name, int clockrate_hz, int num_channels); On 2016/04/27 14:35:56, hlundin-webrtc wrote: > I would prefer to rename clockrate_hz to rtp_timestamp_rate_hz. If so, use the same name in the decoder factory unit test. But: would this ever be used with QUIC? https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc (right): https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc:46: std::unique_ptr<AudioDecoder> Uniq(AudioDecoder* d) { On 2016/04/27 14:35:56, hlundin-webrtc wrote: > Did you mean to name it Unique, or were you paying tribute to the > linux/unix-inspired Uniq command? I think the name is confusing because of that connotation too. How about returning naked AudioDecoder*s from the constructor lambdas, since you're already wrapping the result in a unique_ptr? https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc:54: [](int clockrate_hz, int num_channels) { nit: Indent 1 looks weird - did git cl format do this? https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc:90: [](int clockrate_hz, int num_channels) { L16 isn't supported in WVoE. See: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc:119: FATAL() << "Not implemented yet!"; We should implement this already now, as it forces us to think more carefully about the contents SdpAudioFormat and how the factory accepts or refuses the various combinations.
Looks like I had a few comments after all... https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_format.cc (right): https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_format.cc:41: const char* sep = ""; On 2016/04/27 14:35:56, hlundin-webrtc wrote: > I think the const qualifier is confusing here, since you are in fact changing > the separator string further down. (I realize that you are not violating the > constness, since you are setting a new value for the (non-const) pointer, not > modifying the (const) pointed data.) Well, it's still correct - the char * can't point to "" without being const, since "" is const. (I mean, it probably can but it's wrong). https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_format.h (right): https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_format.h:23: struct SdpAudioFormat { On 2016/04/27 20:34:14, the sun wrote: > How about naming this EncodedAudioFormat instead? The important distinction is that it is not an audio format like how you would normally think of it, rather a set of parameters that's used in Sdp - actually, I guess something like RtpAvp (the Audio/Video Profile for RTP that we're implementing) would be more correct. From the brief insight into QUIC we got yesterday, I believe this'll need to change eventually - maybe sooner than we think - but I see no reason to block this CL over it. It's clearly marked as being in flux. https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_format.h:29: SdpAudioFormat(const char* name, int clockrate_hz, int num_channels); On 2016/04/27 20:34:14, the sun wrote: > On 2016/04/27 14:35:56, hlundin-webrtc wrote: > > I would prefer to rename clockrate_hz to rtp_timestamp_rate_hz. > > If so, use the same name in the decoder factory unit test. > > But: would this ever be used with QUIC? Didn't sound like it, but this class was never designed with QUIC in mind. See previous comment. https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc (right): https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc:90: [](int clockrate_hz, int num_channels) { On 2016/04/27 20:34:15, the sun wrote: > L16 isn't supported in WVoE. See: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... Good to know, however isn't this just a straight mapping of the previous codec support in audio_coding? The actual list of formats to use is provided below (well, not provided yet, it seems).
https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_format.cc (right): https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_format.cc:41: const char* sep = ""; On 2016/04/28 08:35:53, ossu wrote: > On 2016/04/27 14:35:56, hlundin-webrtc wrote: > > I think the const qualifier is confusing here, since you are in fact changing > > the separator string further down. (I realize that you are not violating the > > constness, since you are setting a new value for the (non-const) pointer, not > > modifying the (const) pointed data.) > > Well, it's still correct - the char * can't point to "" without being const, > since "" is const. (I mean, it probably can but it's wrong). Oh. Yes. Well... eeh... [voice tapering off as Shame Cube is lowered over me]
New patch set posted. https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/B... File webrtc/modules/audio_coding/BUILD.gn (right): https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/B... webrtc/modules/audio_coding/BUILD.gn:55: source_set("builtin_audio_codec_factory") { On 2016/04/27 20:34:14, the sun wrote: > _factories? > > Or is the intent to have a single class implementing both enc and dec factory? Changed this to "builtin_audio_decoder_factory" for now. We can change it to "builtin_audio_codec_factories" if and when we put the encoder factory in here too. https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_coding.gypi (right): https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_coding.gypi:72: 'target_name': 'audio_codec_factory_interface', On 2016/04/27 14:35:56, hlundin-webrtc wrote: > I assume you are intending to extend this target (and the one below) with > encoder capabilities too. Otherwise, the naming is off. Yes, that was my plan. But perhaps it's better to name it for the current contents, and change the name later. https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_coding.gypi:92: 'target_name': 'builtin_audio_codec_factory', On 2016/04/27 20:34:14, the sun wrote: > _factories? I'll change codec to decoder instead; see previous comment. https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_decoder_factory_unittest.cc (right): https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder_factory_unittest.cc:25: EXPECT_FALSE(adf->MakeAudioDecoder(SdpAudioFormat("pcmu", 8000, 0))); On 2016/04/27 20:34:14, the sun wrote: > It would be good to have documentation along the lines of: > // pcmu supports only 8kHz clock rate, with 1 to 3 channels. Right. I'll add that. https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder_factory_unittest.cc:58: EXPECT_FALSE(adf->MakeAudioDecoder(SdpAudioFormat("isac", 48000, 1))); On 2016/04/27 20:34:14, the sun wrote: > ch count 3? I already test that 2 doesn't work. If I'm gonna test more than one past the working variants, I should simply test all reasonably small values and have a predicate that says which should succeed. https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_decoder_factory_unittest.cc:72: for (int channels = 1; channels < 10; ++channels) { On 2016/04/27 20:34:14, the sun wrote: > Do we really need to test all channel counts? Can we just test 1, 2 and "many"? Sure, that's probably fine. Will do. https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_format.h (right): https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_format.h:23: struct SdpAudioFormat { On 2016/04/28 08:35:53, ossu wrote: > On 2016/04/27 20:34:14, the sun wrote: > > How about naming this EncodedAudioFormat instead? > > The important distinction is that it is not an audio format like how you would > normally think of it, rather a set of parameters that's used in Sdp - actually, > I guess something like RtpAvp (the Audio/Video Profile for RTP that we're > implementing) would be more correct. > > From the brief insight into QUIC we got yesterday, I believe this'll need to > change eventually - maybe sooner than we think - but I see no reason to block > this CL over it. It's clearly marked as being in flux. I'll stick with the current name for now. When we remove the "in flux" comments, we can revisit the naming question and see if the naming question seems more straight-forward then. https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_format.h:26: SdpAudioFormat(); On 2016/04/27 20:34:14, the sun wrote: > I'd prefer if you put all the "= default" in here. Can't---the clang style checks complain about complex constructors and destructor being inline if I do. I agree it would aid readability, but that check is making Chromium smaller. :-) https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_format.h:29: SdpAudioFormat(const char* name, int clockrate_hz, int num_channels); On 2016/04/28 08:35:54, ossu wrote: > On 2016/04/27 20:34:14, the sun wrote: > > On 2016/04/27 14:35:56, hlundin-webrtc wrote: > > > I would prefer to rename clockrate_hz to rtp_timestamp_rate_hz. > > > > If so, use the same name in the decoder factory unit test. > > > > But: would this ever be used with QUIC? > > Didn't sound like it, but this class was never designed with QUIC in mind. See > previous comment. Hmm. What's the relationship between SDP and RTP? Does a field called rtp_timestamp_rate_hz make sense in a class called SdpAudioFormat? https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc (right): https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc:46: std::unique_ptr<AudioDecoder> Uniq(AudioDecoder* d) { On 2016/04/27 20:34:15, the sun wrote: > On 2016/04/27 14:35:56, hlundin-webrtc wrote: > > Did you mean to name it Unique, or were you paying tribute to the > > linux/unix-inspired Uniq command? > > I think the name is confusing because of that connotation too. > > How about returning naked AudioDecoder*s from the constructor lambdas, since > you're already wrapping the result in a unique_ptr? I was initially going to name it U, but I foresaw lots of reviewer grief in that future, so I chose a slightly longer name. :-) I'll change it to Unique. Yes, I could return raw pointers, but (1) that would violate the rule about passing ownership in raw pointers, if only locally, but (2) I'm planning to replace all these lambdas with factory functions owned by each decoder, so the violation would no longer be local and therefore much worse. https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc:54: [](int clockrate_hz, int num_channels) { On 2016/04/27 20:34:14, the sun wrote: > nit: Indent 1 looks weird - did git cl format do this? Yes. And it's not weird; it's how multiline array and struct literals are usually formatted: {1, 2, 3} https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc:90: [](int clockrate_hz, int num_channels) { On 2016/04/28 08:35:54, ossu wrote: > On 2016/04/27 20:34:15, the sun wrote: > > L16 isn't supported in WVoE. See: > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... > > Good to know, however isn't this just a straight mapping of the previous codec > support in audio_coding? The actual list of formats to use is provided below > (well, not provided yet, it seems). Interesting. But ossu@ is right, I've just tried to preserve what NetEq can currently do. https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc:103: return Uniq(nullptr); On 2016/04/27 14:35:56, hlundin-webrtc wrote: > Why Uniq(nullptr) here, but not in other places? Because the other places all use ?: with nullptr as one case and std::unique_ptr<AudioDecoder> as the other case, so that the resulting expression has type std::unique_ptr<AudioDecoder>, whereas this return expression would have type nullptr_t without the Uniq call. And lambdas without a declared return type are required to have the same type on all their return expressions. IOW, I tried that first and it didn't work. :-) https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc:119: FATAL() << "Not implemented yet!"; On 2016/04/27 20:34:15, the sun wrote: > We should implement this already now, as it forces us to think more carefully > about the contents SdpAudioFormat and how the factory accepts or refuses the > various combinations. Are you sure? I could implement it, but since no one is using it yet, I don't think the "forcing us to think" part is going to happen. https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.h (right): https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.h:20: // Create a new factory that can create the built-in types of audio decoders. On 2016/04/27 14:35:56, hlundin-webrtc wrote: > Creates... $#@^$#@ Done.
lgtm https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_format.h (right): https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_format.h:23: struct SdpAudioFormat { On 2016/04/28 12:35:52, kwiberg-webrtc wrote: > On 2016/04/28 08:35:53, ossu wrote: > > On 2016/04/27 20:34:14, the sun wrote: > > > How about naming this EncodedAudioFormat instead? > > > > The important distinction is that it is not an audio format like how you would > > normally think of it, rather a set of parameters that's used in Sdp - > actually, > > I guess something like RtpAvp (the Audio/Video Profile for RTP that we're > > implementing) would be more correct. > > > > From the brief insight into QUIC we got yesterday, I believe this'll need to > > change eventually - maybe sooner than we think - but I see no reason to block > > this CL over it. It's clearly marked as being in flux. > > I'll stick with the current name for now. When we remove the "in flux" comments, > we can revisit the naming question and see if the naming question seems more > straight-forward then. Acknowledged. https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_format.h:26: SdpAudioFormat(); On 2016/04/28 12:35:52, kwiberg-webrtc wrote: > On 2016/04/27 20:34:14, the sun wrote: > > I'd prefer if you put all the "= default" in here. > > Can't---the clang style checks complain about complex constructors and > destructor being inline if I do. I agree it would aid readability, but that > check is making Chromium smaller. :-) Oh, how sweet! :) https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc (right): https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc:46: std::unique_ptr<AudioDecoder> Uniq(AudioDecoder* d) { On 2016/04/28 12:35:52, kwiberg-webrtc wrote: > On 2016/04/27 20:34:15, the sun wrote: > > On 2016/04/27 14:35:56, hlundin-webrtc wrote: > > > Did you mean to name it Unique, or were you paying tribute to the > > > linux/unix-inspired Uniq command? > > > > I think the name is confusing because of that connotation too. > > > > How about returning naked AudioDecoder*s from the constructor lambdas, since > > you're already wrapping the result in a unique_ptr? > > I was initially going to name it U, but I foresaw lots of reviewer grief in that > future, so I chose a slightly longer name. :-) I'll change it to Unique. > > Yes, I could return raw pointers, but (1) that would violate the rule about > passing ownership in raw pointers, if only locally, but (2) I'm planning to > replace all these lambdas with factory functions owned by each decoder, so the > violation would no longer be local and therefore much worse. Ok, (2) makes it make sense. https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc:90: [](int clockrate_hz, int num_channels) { On 2016/04/28 12:35:52, kwiberg-webrtc wrote: > On 2016/04/28 08:35:54, ossu wrote: > > On 2016/04/27 20:34:15, the sun wrote: > > > L16 isn't supported in WVoE. See: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... > > > > Good to know, however isn't this just a straight mapping of the previous > codec > > support in audio_coding? The actual list of formats to use is provided below > > (well, not provided yet, it seems). > > Interesting. But ossu@ is right, I've just tried to preserve what NetEq can > currently do. Ok, so once we are more comfortable with the structure, the #defines and codecs we don't need to support will go away, and we'll just have something similar to the list in WVoE? That SGTM. Right now, we have this divide between what we say we support (WVoE filtered list of codecs), and what we actually do support (what's builtin to ACM. See: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc.... https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc:119: FATAL() << "Not implemented yet!"; On 2016/04/28 12:35:52, kwiberg-webrtc wrote: > On 2016/04/27 20:34:15, the sun wrote: > > We should implement this already now, as it forces us to think more carefully > > about the contents SdpAudioFormat and how the factory accepts or refuses the > > various combinations. > > Are you sure? I could implement it, but since no one is using it yet, I don't > think the "forcing us to think" part is going to happen. Nah, you're right, leaving it out makes sense from the approach of weeding out how to use the factory from within NetEq.
https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_format.h (right): https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_format.h:29: SdpAudioFormat(const char* name, int clockrate_hz, int num_channels); On 2016/04/28 12:35:51, kwiberg-webrtc wrote: > On 2016/04/28 08:35:54, ossu wrote: > > On 2016/04/27 20:34:14, the sun wrote: > > > On 2016/04/27 14:35:56, hlundin-webrtc wrote: > > > > I would prefer to rename clockrate_hz to rtp_timestamp_rate_hz. > > > > > > If so, use the same name in the decoder factory unit test. > > > > > > But: would this ever be used with QUIC? > > > > Didn't sound like it, but this class was never designed with QUIC in mind. See > > previous comment. > > Hmm. What's the relationship between SDP and RTP? Does a field called > rtp_timestamp_rate_hz make sense in a class called SdpAudioFormat? Yeeeaaah, I mean, sort-of. Although the SDP RFC refers to this field as clock rate, we use SDP to specifically set up communication over RTP (most of these values are from the rtpmap attribute to a media stream). If the type is going to be called SdpAudioFormat, then I prefer if the field is still uses the term clock rate. Calling the type RtpAudioFormat might be better, in which case changing the name of clockrate_hz might be beneficial. On the other hand, why mention rtp_ in a type that's already called RtpAudioFormat?
https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_format.h (right): https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_format.h:29: SdpAudioFormat(const char* name, int clockrate_hz, int num_channels); On 2016/04/28 14:02:36, ossu wrote: > On 2016/04/28 12:35:51, kwiberg-webrtc wrote: > > On 2016/04/28 08:35:54, ossu wrote: > > > On 2016/04/27 20:34:14, the sun wrote: > > > > On 2016/04/27 14:35:56, hlundin-webrtc wrote: > > > > > I would prefer to rename clockrate_hz to rtp_timestamp_rate_hz. > > > > > > > > If so, use the same name in the decoder factory unit test. > > > > > > > > But: would this ever be used with QUIC? > > > > > > Didn't sound like it, but this class was never designed with QUIC in mind. > See > > > previous comment. > > > > Hmm. What's the relationship between SDP and RTP? Does a field called > > rtp_timestamp_rate_hz make sense in a class called SdpAudioFormat? > > Yeeeaaah, I mean, sort-of. Although the SDP RFC refers to this field as clock > rate, we use SDP to specifically set up communication over RTP (most of these > values are from the rtpmap attribute to a media stream). > If the type is going to be called SdpAudioFormat, then I prefer if the field is > still uses the term clock rate. > Calling the type RtpAudioFormat might be better, in which case changing the name > of clockrate_hz might be beneficial. On the other hand, why mention rtp_ in a > type that's already called RtpAudioFormat? I think I'll leave the names as they are for now, but it may make sense to call the class RtpAudioFormat if RTP is where the parameters come from. https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc (right): https://codereview.webrtc.org/1917163002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory.cc:90: [](int clockrate_hz, int num_channels) { On 2016/04/28 13:02:42, the sun wrote: > On 2016/04/28 12:35:52, kwiberg-webrtc wrote: > > On 2016/04/28 08:35:54, ossu wrote: > > > On 2016/04/27 20:34:15, the sun wrote: > > > > L16 isn't supported in WVoE. See: > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... > > > > > > Good to know, however isn't this just a straight mapping of the previous > > codec > > > support in audio_coding? The actual list of formats to use is provided below > > > (well, not provided yet, it seems). > > > > Interesting. But ossu@ is right, I've just tried to preserve what NetEq can > > currently do. > > Ok, so once we are more comfortable with the structure, the #defines and codecs > we don't need to support will go away, and we'll just have something similar to > the list in WVoE? That SGTM. > > Right now, we have this divide between what we say we support (WVoE filtered > list of codecs), and what we actually do support (what's builtin to ACM. See: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc.... Yes, once we inject the factory from all the way out in WVoE, it would make perfect sense to change the set of codecs supported by the factory to the set we actually want.
The CQ bit was checked by kwiberg@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from ossu@webrtc.org, henrik.lundin@webrtc.org Link to the patchset: https://codereview.webrtc.org/1917163002/#ps20001 (title: "review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1917163002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1917163002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== New interface (AudioDecoderFactory), with an implementation This is a first draft of what we're hoping to use to create all AudioDecoder instances. Follow-up CLs will start using this internally in NetEq instead of calling constructors manually. BUG=webrtc:5801 ========== to ========== New interface (AudioDecoderFactory), with an implementation This is a first draft of what we're hoping to use to create all AudioDecoder instances. Follow-up CLs will start using this internally in NetEq instead of calling constructors manually. BUG=webrtc:5801 Committed: https://crrev.com/c01c6a423cfe60e2d0bf1f700bdc331ba1d29572 Cr-Commit-Position: refs/heads/master@{#12548} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/c01c6a423cfe60e2d0bf1f700bdc331ba1d29572 Cr-Commit-Position: refs/heads/master@{#12548} |
