Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1868143002:
Convert CNG into C++ and remove it from AudioDecoder (Closed)
|
Created:
4 years, 8 months ago by ossu Modified:
4 years, 7 months ago CC:
webrtc-reviews_webrtc.org, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, peah-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. |
DescriptionConvert CNG into C++ and remove it from AudioDecoder
Broke out CNG from AudioDecoder as they didn't really share an interface.
Converted the CNG code to C++, to make initialization and resource handling easier. This includes several changes to the behavior, favoring RTC_CHECKs over returning error codes.
Committed: https://crrev.com/97ba30eedf99e84de33200b7546d5d578fe358e6
Cr-Commit-Position: refs/heads/master@{#12491}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Converted WebRtcCng to C++ #
Total comments: 74
Patch Set 3 : Addressed issues from comments. #
Total comments: 15
Patch Set 4 : Separated Encode tests, added GTEST_HAS_DEATH_TEST check, fixed comments. #
Total comments: 2
Patch Set 5 : CheckedDivExact #Patch Set 6 : Fixed a few show-stopping (albeit small) compiler complaints #
Total comments: 4
Patch Set 7 : Disabled CNG death tests on android. Nulled CNG_encoder[k] in RTPencode.cc upon delete. #
Total comments: 2
Patch Set 8 : Enlarged the buffer in UpdateSidErroneous, changed constants to typed enum. #Patch Set 9 : Rebase #
Total comments: 2
Patch Set 10 : Implicit conversion to ArrayView in TestSidErroneous #Dependent Patchsets: Messages
Total messages: 53 (11 generated)
Description was changed from ========== WIP: Cleaning up AudioDecoder interface and make it externally usable. Broke out CNG from AudioDecoder. They didn't really share an interface, anyway. The goal is to get the AudioDecoder interface cleaned up enough to be exposed publicly, and implemented by outside parties. A first step is to at least make that interface reasonable for speech codecs, seeing as CNG is given special treatment all throughout neteq. I believe the ultimate goal should be to have CNG adhere to the same interface as other decoders and minimize the amount of special handling put in for it. ========== to ========== WIP: Cleaning up AudioDecoder interface and make it externally usable. Broke out CNG from AudioDecoder. They didn't really share an interface, anyway. The goal is to get the AudioDecoder interface cleaned up enough to be exposed publicly, and implemented by outside parties. A first step is to at least make that interface reasonable for speech codecs, seeing as CNG is given special treatment all throughout neteq. I believe the ultimate goal should be to have CNG adhere to the same interface as other decoders and minimize the amount of special handling put in for it. ==========
ossu@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, kwiberg@webrtc.org, solenberg@webrtc.org
Please take a look at this suggested change. As part of making audio decoders (and eventually encoders) externally implementable, I believe we should minimize their interfaces and remove most external dependencies. Part of this is untangling the specially-treated CNG decoder from the general AudioDecoder interface. Down the line, I'm open to more interface simplifications. For example, at this point it seems DecodeRedundant and PacketHasFec are only implemented by the Opus decoder. That logic should probably be pushed into the Opus decoder proper.
Moving myself to CC: line - I think there are enough reviewers already, who know the ins/outs of the ACM much better than myself!
This looks like a good idea. And I'm guessing we can make the AudioDecoder interface change without telling users, since no one should be overriding the removed method. (When this isn't WIP anymore, please write a more specific commit message.) https://codereview.webrtc.org/1868143002/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/audio_decoder_impl.cc (right): https://codereview.webrtc.org/1868143002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/audio_decoder_impl.cc:194: RTC_CHECK(false && "CNG should not be created like this right now!"); RTC_CHECK(false) << "CNG should not be created like this right now!"; https://codereview.webrtc.org/1868143002/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/decoder_database.cc (right): https://codereview.webrtc.org/1868143002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/decoder_database.cc:28: WebRtcCng_FreeDec(active_cng_dec_inst_); Having to write destructor code manually is almost always a sign that you're doing something wrong... Please use something like unique_ptr to own the CNG decoder. (You'll either have to ensure that the decoder's destructor calls WebRtcCng_FreeDec, or have a custom deleter that does that.) (Having looked through the rest of the CL, I think wrapping the CNG struct in a C++ class with a proper destructor would be best. That way, you could also wrap the CNG methods you use and give them more convenient signatures.) https://codereview.webrtc.org/1868143002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/decoder_database.cc:235: RTC_CHECK(!(*it).second.external); Since you're rewriting this line anyway, please change (*a).b to a->b. https://codereview.webrtc.org/1868143002/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/payload_splitter.cc (right): https://codereview.webrtc.org/1868143002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/payload_splitter.cc:147: // Decoder. Please avoid referring to how things used to be, since that'll eventually cease to be enlightening. // This should either be CNG or a real decoder. RTC_DCHECK(decoder || decoder_database->IsComfortNoise(payload_type));
https://codereview.webrtc.org/1868143002/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/audio_decoder_impl.cc (right): https://codereview.webrtc.org/1868143002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/audio_decoder_impl.cc:194: RTC_CHECK(false && "CNG should not be created like this right now!"); On 2016/04/09 07:34:49, kwiberg-webrtc wrote: > RTC_CHECK(false) << "CNG should not be created like this right now!"; Acknowledged. https://codereview.webrtc.org/1868143002/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/decoder_database.cc (right): https://codereview.webrtc.org/1868143002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/decoder_database.cc:28: WebRtcCng_FreeDec(active_cng_dec_inst_); On 2016/04/09 07:34:49, kwiberg-webrtc wrote: > Having to write destructor code manually is almost always a sign that you're > doing something wrong... Please use something like unique_ptr to own the CNG > decoder. (You'll either have to ensure that the decoder's destructor calls > WebRtcCng_FreeDec, or have a custom deleter that does that.) > > (Having looked through the rest of the CL, I think wrapping the CNG struct in a > C++ class with a proper destructor would be best. That way, you could also wrap > the CNG methods you use and give them more convenient signatures.) I think wrapping it is probably the best. This is just the minimal amount needed to get CNG out of the AudioDecoder interface. I was hoping to just put it aside for a while, do the whole external decoder thing, then see how one would fold CNG back into that. Still, doesn't hurt to have better code during that work. :) https://codereview.webrtc.org/1868143002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/decoder_database.cc:235: RTC_CHECK(!(*it).second.external); On 2016/04/09 07:34:49, kwiberg-webrtc wrote: > Since you're rewriting this line anyway, please change (*a).b to a->b. Acknowledged. https://codereview.webrtc.org/1868143002/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/payload_splitter.cc (right): https://codereview.webrtc.org/1868143002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/payload_splitter.cc:147: // Decoder. On 2016/04/09 07:34:49, kwiberg-webrtc wrote: > Please avoid referring to how things used to be, since that'll eventually cease > to be enlightening. > > // This should either be CNG or a real decoder. > RTC_DCHECK(decoder || decoder_database->IsComfortNoise(payload_type)); Acknowledged.
Speaking of which: is there any reason to keep the WebRtcCng in straight C? It looks like a small job to just convert it into C++ classes and get proper construction/destruction in, rather than having to wrap them. hlundin?
henrik.lundin@webrtc.org changed required reviewers: + henrik.lundin@webrtc.org
Nice change! Just a few comments from me. https://codereview.webrtc.org/1868143002/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/audio_decoder_impl.cc (right): https://codereview.webrtc.org/1868143002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/audio_decoder_impl.cc:194: RTC_CHECK(false && "CNG should not be created like this right now!"); On 2016/04/11 08:31:10, ossu wrote: > On 2016/04/09 07:34:49, kwiberg-webrtc wrote: > > RTC_CHECK(false) << "CNG should not be created like this right now!"; > > Acknowledged. I guess RTC_NOTREACHED() is yet another way to say it. I think you should drop the "right now", since I think we want to keep CNG outside of the AudioDecoder interface. https://codereview.webrtc.org/1868143002/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/decoder_database.cc (right): https://codereview.webrtc.org/1868143002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/decoder_database.cc:28: WebRtcCng_FreeDec(active_cng_dec_inst_); On 2016/04/11 08:31:10, ossu wrote: > On 2016/04/09 07:34:49, kwiberg-webrtc wrote: > > Having to write destructor code manually is almost always a sign that you're > > doing something wrong... Please use something like unique_ptr to own the CNG > > decoder. (You'll either have to ensure that the decoder's destructor calls > > WebRtcCng_FreeDec, or have a custom deleter that does that.) > > > > (Having looked through the rest of the CL, I think wrapping the CNG struct in > a > > C++ class with a proper destructor would be best. That way, you could also > wrap > > the CNG methods you use and give them more convenient signatures.) > > I think wrapping it is probably the best. This is just the minimal amount needed > to get CNG out of the AudioDecoder interface. I was hoping to just put it aside > for a while, do the whole external decoder thing, then see how one would fold > CNG back into that. Still, doesn't hurt to have better code during that work. :) I think it is ok to leave some of this debt behind for now; the ends justify the means. As I said in another place, I'm not so sure the CNG functionality should be folded back in with the AudioDecoder interface. The calling pattern is different between a regular decoder and a CNG decoder. https://codereview.webrtc.org/1868143002/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right): https://codereview.webrtc.org/1868143002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/neteq_impl.cc:667: // Update bandwidth estimate, if the packet is not sync-packet. Update the comment to mention that CNG packets are excluded too.
On 2016/04/11 10:46:12, ossu wrote: > Speaking of which: is there any reason to keep the WebRtcCng in straight C? It > looks like a small job to just convert it into C++ classes and get proper > construction/destruction in, rather than having to wrap them. > > hlundin? No, I cannot see any reason to keep it in C. The best approach here is probably to convert that into C++ and avoid a specific wrapper layer for it in NetEq. But I can see that kind of work is slightly to the side of your main track right now, and I'm fine with leaving some debt behind. Either way, you did not make the debt any worse with this change in my opinion.
https://codereview.webrtc.org/1868143002/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/audio_decoder_impl.cc (right): https://codereview.webrtc.org/1868143002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/audio_decoder_impl.cc:194: RTC_CHECK(false && "CNG should not be created like this right now!"); On 2016/04/11 11:33:00, hlundin-webrtc wrote: > On 2016/04/11 08:31:10, ossu wrote: > > On 2016/04/09 07:34:49, kwiberg-webrtc wrote: > > > RTC_CHECK(false) << "CNG should not be created like this right now!"; > > > > Acknowledged. > > I guess RTC_NOTREACHED() is yet another way to say it. Yes, but bear in mind that that one uses DCHECK, not CHECK. (Hmm. Perhaps it ought to use CHECK instead? Since the line should never be reached, there won't be any overhead... hmm... except that the compiler could optimize based on our assertion... need to think more about this.)
https://codereview.webrtc.org/1868143002/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/audio_decoder_impl.cc (right): https://codereview.webrtc.org/1868143002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/audio_decoder_impl.cc:194: RTC_CHECK(false && "CNG should not be created like this right now!"); On 2016/04/11 11:33:00, hlundin-webrtc wrote: > On 2016/04/11 08:31:10, ossu wrote: > > On 2016/04/09 07:34:49, kwiberg-webrtc wrote: > > > RTC_CHECK(false) << "CNG should not be created like this right now!"; > > > > Acknowledged. > > I guess RTC_NOTREACHED() is yet another way to say it. > > I think you should drop the "right now", since I think we want to keep CNG > outside of the AudioDecoder interface. Acknowledged. https://codereview.webrtc.org/1868143002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/audio_decoder_impl.cc:194: RTC_CHECK(false && "CNG should not be created like this right now!"); On 2016/04/11 12:42:56, kwiberg-webrtc wrote: > On 2016/04/11 11:33:00, hlundin-webrtc wrote: > > On 2016/04/11 08:31:10, ossu wrote: > > > On 2016/04/09 07:34:49, kwiberg-webrtc wrote: > > > > RTC_CHECK(false) << "CNG should not be created like this right now!"; > > > > > > Acknowledged. > > > > I guess RTC_NOTREACHED() is yet another way to say it. > > Yes, but bear in mind that that one uses DCHECK, not CHECK. (Hmm. Perhaps it > ought to use CHECK instead? Since the line should never be reached, there won't > be any overhead... hmm... except that the compiler could optimize based on our > assertion... need to think more about this.) It makes sense that NOTREACHED should be in even in release builds. As you say, since it should _never_ happen, its cost (except in code size) shouldn't matter. I'll keep it as RTC_CHECK(false) until this is resolved, though. https://codereview.webrtc.org/1868143002/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right): https://codereview.webrtc.org/1868143002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/neteq_impl.cc:667: // Update bandwidth estimate, if the packet is not sync-packet. On 2016/04/11 11:33:00, hlundin-webrtc wrote: > Update the comment to mention that CNG packets are excluded too. Acknowledged.
https://codereview.webrtc.org/1868143002/diff/1/webrtc/modules/audio_coding/n... File webrtc/modules/audio_coding/neteq/audio_decoder_impl.cc (right): https://codereview.webrtc.org/1868143002/diff/1/webrtc/modules/audio_coding/n... webrtc/modules/audio_coding/neteq/audio_decoder_impl.cc:194: RTC_CHECK(false && "CNG should not be created like this right now!"); On 2016/04/11 12:59:57, ossu wrote: > On 2016/04/11 12:42:56, kwiberg-webrtc wrote: > > On 2016/04/11 11:33:00, hlundin-webrtc wrote: > > > On 2016/04/11 08:31:10, ossu wrote: > > > > On 2016/04/09 07:34:49, kwiberg-webrtc wrote: > > > > > RTC_CHECK(false) << "CNG should not be created like this right now!"; > > > > > > > > Acknowledged. > > > > > > I guess RTC_NOTREACHED() is yet another way to say it. > > > > Yes, but bear in mind that that one uses DCHECK, not CHECK. (Hmm. Perhaps it > > ought to use CHECK instead? Since the line should never be reached, there > won't > > be any overhead... hmm... except that the compiler could optimize based on our > > assertion... need to think more about this.) > > It makes sense that NOTREACHED should be in even in release builds. As you say, > since it should _never_ happen, its cost (except in code size) shouldn't matter. Yes. Except that the compiler can optimize the surrounding code based on our assertion that this statement can't be reached. > I'll keep it as RTC_CHECK(false) until this is resolved, though. Good.
I've converted the CNG code to C++, at least structurally. I've tried to keep the actual implementation as intact as possible to not inadvertently introduce errors. I did, however, go pretty heavy on simplifying its interface - please have a look and see if it makes sense. Neither the encoder nor decoder has a gettable error code anymore. Since they cannot be created without also being initialized, there's only one thing that may go wrong, it seems, and that is during Generate / Encode - if too much data is requested (or put in). I'm quite happy with Generate just returning a bool, but not so much with Encode returning the number of bytes written, or -1 of something went wrong. Maybe Encode shouldn't be able to fail with any reasonable input? A number of tests ended up on the chopping block as well, since I removed separate initialization and turned other errors into RTC_CHECKs. Maybe some of them should be reintroduced as death tests?
(I haven't reviewed the actual code yet.) On 2016/04/12 08:48:50, ossu wrote: > Neither the encoder nor decoder has a gettable > error code anymore. Since they cannot be created without also being initialized, > there's only one thing that may go wrong, it seems, and that is during Generate > / Encode - if too much data is requested (or put in). Sounds very good. > I'm quite happy with > Generate just returning a bool, Excellent. > but not so much with Encode returning the number > of bytes written, or -1 of something went wrong. Maybe Encode shouldn't be able > to fail with any reasonable input? rtc::Optional is better than using -1, but I agree that Encode shouldn't fail for any input. [D]CHECK is probably the error checking tool of choice here. > A number of tests ended up on the chopping block as well, since I removed > separate initialization and turned other errors into RTC_CHECKs. Maybe some of > them should be reintroduced as death tests? Yes, if the conditions being CHECKed are nontrivial and/or can cause bad problems if they slip by, death tests are probably nice.
https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.cc (right): https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.cc:180: num_cng_coefficients_)); A small utility function in an anonymous namespace that returns a unique_ptr<ComfortNoiseEncoder>? https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.cc:228: RTC_CHECK_GE(encoded_bytes_tmp, 0); Use rtc::Optional<size_t> if you need to return an error. But as I said, I agree that Encode can probably just explode if called with invalid inputs. Also, consider passing the Buffer* into Encode instead of an ArrayView. That way, you completely remove the possibility of it being too small. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/cng/cng_helpfuns.c (right): https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/cng_helpfuns.c:14: #include "webrtc/typedefs.h" Why not remove this file? https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/cng/cng_helpfuns.h (right): https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/cng_helpfuns.h:24: #endif // WEBRTC_MODULES_AUDIO_CODING_CODECS_CNG_CNG_HELPFUNS_H_ Why not remove this file? https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h (right): https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:31: ComfortNoiseDecoder& operator=(const ComfortNoiseDecoder&) = delete; We have a macro for this in constructormagic.h: RTC_DISALLOW_COPY_AND_ASSIGN. I haven't decided yet if I prefer deleting the methods manually or using that macro... https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:75: void Reset(int fs, int interval, int quality); Is this necessary? Can't the callers just make a new instance instead? https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:86: bool forceSID); Consider outputting to an rtc::Buffer* instead, so that the caller doesn't have to care how much space is needed. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/comfort_noise.cc (right): https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/comfort_noise.cc:38: packet->payload, packet->payload_length)); You probably don't need to store the cng decoder in a variable here, since the return value is never null. (Hmm. That function can return null. I guess it will never do so in this case?) https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/decoder_database.cc (right): https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/decoder_database.cc:27: } =default, to save one line. :-) https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right): https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/neteq_impl.cc:669: // noise. Is this additional requirement because CN's IncomingPacket method didn't do anything anyway? https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/neteq_impl.cc:672: if (!decoder_database_->IsComfortNoise(main_header.payloadType)) { Merge these two ifs? https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/test/RTPencode.cc (right): https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/test/RTPencode.cc:933: CNG_encoder[k].reset(new webrtc::ComfortNoiseEncoder(sampfreq, 200, 5)); The old code created but didn't initialize a CNG encoder if sampfreq > 16000. Was that an error?
https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.cc (right): https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.cc:180: num_cng_coefficients_)); On 2016/04/12 13:35:30, kwiberg-webrtc wrote: > A small utility function in an anonymous namespace that returns a > unique_ptr<ComfortNoiseEncoder>? Actually, there's a Reset method in it right now, that I bet I could use... not sure it should remain in, though. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.cc:228: RTC_CHECK_GE(encoded_bytes_tmp, 0); On 2016/04/12 13:35:30, kwiberg-webrtc wrote: > Use rtc::Optional<size_t> if you need to return an error. But as I said, I agree > that Encode can probably just explode if called with invalid inputs. > > Also, consider passing the Buffer* into Encode instead of an ArrayView. That > way, you completely remove the possibility of it being too small. Yeah, Buffer is probably better. It's what we want to move to in general, internally, anyway, right? https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/cng/cng_helpfuns.c (right): https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/cng_helpfuns.c:14: #include "webrtc/typedefs.h" On 2016/04/12 13:35:30, kwiberg-webrtc wrote: > Why not remove this file? See cng_helpfuns.h :) https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/cng/cng_helpfuns.h (right): https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/cng_helpfuns.h:24: #endif // WEBRTC_MODULES_AUDIO_CODING_CODECS_CNG_CNG_HELPFUNS_H_ On 2016/04/12 13:35:31, kwiberg-webrtc wrote: > Why not remove this file? BUT IT'S SO PRETTY! (also I apparently forgot. :() https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h (right): https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:31: ComfortNoiseDecoder& operator=(const ComfortNoiseDecoder&) = delete; On 2016/04/12 13:35:31, kwiberg-webrtc wrote: > We have a macro for this in constructormagic.h: RTC_DISALLOW_COPY_AND_ASSIGN. I > haven't decided yet if I prefer deleting the methods manually or using that > macro... There's been conversations on c-style, where using macros for this is discouraged over doing it manually. I'm not sure it's made its way all through to official Google C style yet, but it seems to be where we're headed. On the other hand, I'm not sure they _need_ to be noncopyable. The state is just POD. I could just leave them as copyable, for whenever that might be useful. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:75: void Reset(int fs, int interval, int quality); On 2016/04/12 13:35:31, kwiberg-webrtc wrote: > Is this necessary? Can't the callers just make a new instance instead? There was some code that used InitEnc before, so I thought I'd keep that implementation around for now. I'm all for getting rid of Reset - preferably in both Encoder and Decoder. Hmm, maybe get them back as copyable and remove Reset? Would make it easy to just reinitialize your local CNG-whatever without reallocations. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:86: bool forceSID); On 2016/04/12 13:35:31, kwiberg-webrtc wrote: > Consider outputting to an rtc::Buffer* instead, so that the caller doesn't have > to care how much space is needed. Acknowledged. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/comfort_noise.cc (right): https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/comfort_noise.cc:38: packet->payload, packet->payload_length)); On 2016/04/12 13:35:31, kwiberg-webrtc wrote: > You probably don't need to store the cng decoder in a variable here, since the > return value is never null. (Hmm. That function can return null. I guess it will > never do so in this case?) Yeah, it can but it never will. RTC_DCHECK? It's going to be a long, ugly line if I combine them, though. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/decoder_database.cc (right): https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/decoder_database.cc:27: } On 2016/04/12 13:35:31, kwiberg-webrtc wrote: > =default, to save one line. :-) Acknowledged. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right): https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/neteq_impl.cc:669: // noise. On 2016/04/12 13:35:31, kwiberg-webrtc wrote: > Is this additional requirement because CN's IncomingPacket method didn't do > anything anyway? Well, it just returned -1 before, so the call was just a noop. We still want to assert (actually, probably RTC_DCHECK) that decoder is valid below, though. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/neteq_impl.cc:672: if (!decoder_database_->IsComfortNoise(main_header.payloadType)) { On 2016/04/12 13:35:31, kwiberg-webrtc wrote: > Merge these two ifs? Acknowledged. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/test/RTPencode.cc (right): https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/test/RTPencode.cc:933: CNG_encoder[k].reset(new webrtc::ComfortNoiseEncoder(sampfreq, 200, 5)); On 2016/04/12 13:35:31, kwiberg-webrtc wrote: > The old code created but didn't initialize a CNG encoder if sampfreq > 16000. > Was that an error? I ... don't know. I put a TODO comment about that just above, hoping someone would enlighten me. It seems CNG only supports up to 16k (according to its docs). Maybe there needed to be a valid, bad state for all encoders, so that if something used it later, it would fail with NOT_INITIALIZED rather than probably generate garbage that no-one would notice.
https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.cc (right): https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.cc:180: num_cng_coefficients_)); On 2016/04/12 13:35:30, kwiberg-webrtc wrote: > A small utility function in an anonymous namespace that returns a > unique_ptr<ComfortNoiseEncoder>? Actually, there's a Reset method in it right now, that I bet I could use... not sure it should remain in, though. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.cc:228: RTC_CHECK_GE(encoded_bytes_tmp, 0); On 2016/04/12 13:35:30, kwiberg-webrtc wrote: > Use rtc::Optional<size_t> if you need to return an error. But as I said, I agree > that Encode can probably just explode if called with invalid inputs. > > Also, consider passing the Buffer* into Encode instead of an ArrayView. That > way, you completely remove the possibility of it being too small. Yeah, Buffer is probably better. It's what we want to move to in general, internally, anyway, right? https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/cng/cng_helpfuns.c (right): https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/cng_helpfuns.c:14: #include "webrtc/typedefs.h" On 2016/04/12 13:35:30, kwiberg-webrtc wrote: > Why not remove this file? See cng_helpfuns.h :) https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/cng/cng_helpfuns.h (right): https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/cng_helpfuns.h:24: #endif // WEBRTC_MODULES_AUDIO_CODING_CODECS_CNG_CNG_HELPFUNS_H_ On 2016/04/12 13:35:31, kwiberg-webrtc wrote: > Why not remove this file? BUT IT'S SO PRETTY! (also I apparently forgot. :() https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h (right): https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:31: ComfortNoiseDecoder& operator=(const ComfortNoiseDecoder&) = delete; On 2016/04/12 13:35:31, kwiberg-webrtc wrote: > We have a macro for this in constructormagic.h: RTC_DISALLOW_COPY_AND_ASSIGN. I > haven't decided yet if I prefer deleting the methods manually or using that > macro... There's been conversations on c-style, where using macros for this is discouraged over doing it manually. I'm not sure it's made its way all through to official Google C style yet, but it seems to be where we're headed. On the other hand, I'm not sure they _need_ to be noncopyable. The state is just POD. I could just leave them as copyable, for whenever that might be useful. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:75: void Reset(int fs, int interval, int quality); On 2016/04/12 13:35:31, kwiberg-webrtc wrote: > Is this necessary? Can't the callers just make a new instance instead? There was some code that used InitEnc before, so I thought I'd keep that implementation around for now. I'm all for getting rid of Reset - preferably in both Encoder and Decoder. Hmm, maybe get them back as copyable and remove Reset? Would make it easy to just reinitialize your local CNG-whatever without reallocations. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:86: bool forceSID); On 2016/04/12 13:35:31, kwiberg-webrtc wrote: > Consider outputting to an rtc::Buffer* instead, so that the caller doesn't have > to care how much space is needed. Acknowledged. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/comfort_noise.cc (right): https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/comfort_noise.cc:38: packet->payload, packet->payload_length)); On 2016/04/12 13:35:31, kwiberg-webrtc wrote: > You probably don't need to store the cng decoder in a variable here, since the > return value is never null. (Hmm. That function can return null. I guess it will > never do so in this case?) Yeah, it can but it never will. RTC_DCHECK? It's going to be a long, ugly line if I combine them, though. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/decoder_database.cc (right): https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/decoder_database.cc:27: } On 2016/04/12 13:35:31, kwiberg-webrtc wrote: > =default, to save one line. :-) Acknowledged. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right): https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/neteq_impl.cc:669: // noise. On 2016/04/12 13:35:31, kwiberg-webrtc wrote: > Is this additional requirement because CN's IncomingPacket method didn't do > anything anyway? Well, it just returned -1 before, so the call was just a noop. We still want to assert (actually, probably RTC_DCHECK) that decoder is valid below, though. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/neteq_impl.cc:672: if (!decoder_database_->IsComfortNoise(main_header.payloadType)) { On 2016/04/12 13:35:31, kwiberg-webrtc wrote: > Merge these two ifs? Acknowledged. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/test/RTPencode.cc (right): https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/test/RTPencode.cc:933: CNG_encoder[k].reset(new webrtc::ComfortNoiseEncoder(sampfreq, 200, 5)); On 2016/04/12 13:35:31, kwiberg-webrtc wrote: > The old code created but didn't initialize a CNG encoder if sampfreq > 16000. > Was that an error? I ... don't know. I put a TODO comment about that just above, hoping someone would enlighten me. It seems CNG only supports up to 16k (according to its docs). Maybe there needed to be a valid, bad state for all encoders, so that if something used it later, it would fail with NOT_INITIALIZED rather than probably generate garbage that no-one would notice.
https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.cc (right): https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.cc:228: RTC_CHECK_GE(encoded_bytes_tmp, 0); On 2016/04/12 13:54:49, ossu wrote: > On 2016/04/12 13:35:30, kwiberg-webrtc wrote: > > Use rtc::Optional<size_t> if you need to return an error. But as I said, I > agree > > that Encode can probably just explode if called with invalid inputs. > > > > Also, consider passing the Buffer* into Encode instead of an ArrayView. That > > way, you completely remove the possibility of it being too small. > > Yeah, Buffer is probably better. It's what we want to move to in general, > internally, anyway, right? I'm fine with using a Buffer as well. But bear in mind that CNG is a special breed. The length of the encoded data has no correlation with the number of samples inserted. It will just estimate a predetermined number of noise parameters from the input. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/cng/cng_unittest.cc (right): https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/cng_unittest.cc:41: std::unique_ptr<ComfortNoiseEncoder> cng_encoder_; Does it make sense to keep the encoder and decoder as members in the test fixture? Feel free to move them to local variables in each TEST_F. Up to you... https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/cng/webrtc_cng.cc (right): https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.cc:15: #include <cstring> We tend to use the C-version of header files that are available both as C and C++ versions. That is, string.h and stdlib.h. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.cc:19: Add a TODO to update variable an member names to follow code style. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h (right): https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:21: #define WEBRTC_CNG_MAX_OUTSIZE_ORDER 640 This is only used inside the class, right? Make it a private const. Or is it used inside both classes? https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:31: ComfortNoiseDecoder& operator=(const ComfortNoiseDecoder&) = delete; On 2016/04/12 13:54:49, ossu wrote: > On 2016/04/12 13:35:31, kwiberg-webrtc wrote: > > We have a macro for this in constructormagic.h: RTC_DISALLOW_COPY_AND_ASSIGN. > I > > haven't decided yet if I prefer deleting the methods manually or using that > > macro... > > There's been conversations on c-style, where using macros for this is > discouraged over doing it manually. I'm not sure it's made its way all through > to official Google C style yet, but it seems to be where we're headed. > > On the other hand, I'm not sure they _need_ to be noncopyable. The state is just > POD. I could just leave them as copyable, for whenever that might be useful. Don't leave them as copyable. Use either way to ban copying. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:35: /* Update the CN state when a new SID packet arrives. Updates... Also, I think you should make these block comments using // instead of /* */. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:40: /* Generate comfort noise. Generates... https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:41: * |outData| will be filled with samples - its size determines the number of out_data https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:46: bool Generate(rtc::ArrayView<int16_t> outData, bool new_period); out_data https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:46: bool Generate(rtc::ArrayView<int16_t> outData, bool new_period); Change order of parameters. Input params go before (input/)output params. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:63: // Create a comfort noise encoder. Creates... https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:64: // |fs| selects sample rate: 8000 for narrowband and 16000 for wideband End all sentences with . https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:73: // Reset the comfort noise encoder to its initial state. Resets... https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:77: // Analyze background noise from |speech| and put coefficients in |output|. Analyzes... https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:77: // Analyze background noise from |speech| and put coefficients in |output|. puts... https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:79: // provided. If |forceSID| is true, a SID frame is forced and the encoder force_sid https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:81: // TODO(ossu): Clarify this behavior. Does it reset before or after encoding? I don't think it should reset all of the encoder state. What should happen is that the internal SID interval counter should reset. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:83: // the rest? Or maybe just DCHECK? https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:86: bool forceSID); force_sid https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/comfort_noise.cc (right): https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/comfort_noise.cc:38: packet->payload, packet->payload_length)); On 2016/04/12 13:54:50, ossu wrote: > On 2016/04/12 13:35:31, kwiberg-webrtc wrote: > > You probably don't need to store the cng decoder in a variable here, since the > > return value is never null. (Hmm. That function can return null. I guess it > will > > never do so in this case?) > > Yeah, it can but it never will. RTC_DCHECK? > It's going to be a long, ugly line if I combine them, though. I vote for a DCHECK. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/test/RTPencode.cc (right): https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/test/RTPencode.cc:1: /* I'm truly sorry that you have to deal with this really old and equally ugly piece of code. It will be the first against the wall when the revolution comes... https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/test/RTPencode.cc:933: CNG_encoder[k].reset(new webrtc::ComfortNoiseEncoder(sampfreq, 200, 5)); On 2016/04/12 13:54:50, ossu wrote: > On 2016/04/12 13:35:31, kwiberg-webrtc wrote: > > The old code created but didn't initialize a CNG encoder if sampfreq > 16000. > > Was that an error? > > I ... don't know. I put a TODO comment about that just above, hoping someone > would enlighten me. It seems CNG only supports up to 16k (according to its > docs). Maybe there needed to be a valid, bad state for all encoders, so that if > something used it later, it would fail with NOT_INITIALIZED rather than probably > generate garbage that no-one would notice. I think we've always said that we only support CNG up to 16 kHz. There is no technical limitation that prevents higher sample rates, but from a protocol point of view we have not supported the combination of a super-wideband (>16 kHz) codec with CNG. As for the other peculiarities in this file, all bets are off. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/test/RTPencode.cc:1595: int tempLen; Why change this? https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/test/RTPencode.cc:1602: const int sampleRate_10 = 10 * sampleRate / 1000; And these?
https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/cng/cng_unittest.cc (right): https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/cng_unittest.cc:41: std::unique_ptr<ComfortNoiseEncoder> cng_encoder_; On 2016/04/13 07:05:23, hlundin-webrtc wrote: > Does it make sense to keep the encoder and decoder as members in the test > fixture? Feel free to move them to local variables in each TEST_F. Up to you... Yeah, it's probably neater to put them into the tests. I'll do that! https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/cng/webrtc_cng.cc (right): https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.cc:15: #include <cstring> On 2016/04/13 07:05:23, hlundin-webrtc wrote: > We tend to use the C-version of header files that are available both as C and > C++ versions. That is, string.h and stdlib.h. Honestly, these should probably go away anyway. Also: why do you include them like that? :) https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.cc:19: On 2016/04/13 07:05:23, hlundin-webrtc wrote: > Add a TODO to update variable an member names to follow code style. Acknowledged. I'll do that as a separate CL, I guess. Might be easier to see that the C++ conversion worked with the implementation generally untouched, before going through and changing names etc. I believe clang-format changed a number of things as well, that may be included in such a CL. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h (right): https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:21: #define WEBRTC_CNG_MAX_OUTSIZE_ORDER 640 On 2016/04/13 07:05:24, hlundin-webrtc wrote: > This is only used inside the class, right? Make it a private const. Or is it > used inside both classes? It is used inside both, to check that the data input and output (respectively) is not too large. I also mention it in the comments in ComfortNoiseDecoder. We should probably inform the user _somewhere_ about the required amount of data to input / expect out. I guess this depends on the sample rate? https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:31: ComfortNoiseDecoder& operator=(const ComfortNoiseDecoder&) = delete; On 2016/04/13 07:05:24, hlundin-webrtc wrote: > On 2016/04/12 13:54:49, ossu wrote: > > On 2016/04/12 13:35:31, kwiberg-webrtc wrote: > > > We have a macro for this in constructormagic.h: > RTC_DISALLOW_COPY_AND_ASSIGN. > > I > > > haven't decided yet if I prefer deleting the methods manually or using that > > > macro... > > > > There's been conversations on c-style, where using macros for this is > > discouraged over doing it manually. I'm not sure it's made its way all through > > to official Google C style yet, but it seems to be where we're headed. > > > > On the other hand, I'm not sure they _need_ to be noncopyable. The state is > just > > POD. I could just leave them as copyable, for whenever that might be useful. > > Don't leave them as copyable. Use either way to ban copying. Alright, no copying! https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:35: /* Update the CN state when a new SID packet arrives. On 2016/04/13 07:05:24, hlundin-webrtc wrote: > Updates... > Also, I think you should make these block comments using // instead of /* */. Acknowledged. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:40: /* Generate comfort noise. On 2016/04/13 07:05:23, hlundin-webrtc wrote: > Generates... Acknowledged. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:41: * |outData| will be filled with samples - its size determines the number of On 2016/04/13 07:05:24, hlundin-webrtc wrote: > out_data Acknowledged. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:46: bool Generate(rtc::ArrayView<int16_t> outData, bool new_period); On 2016/04/13 07:05:24, hlundin-webrtc wrote: > Change order of parameters. Input params go before (input/)output params. Acknowledged. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:63: // Create a comfort noise encoder. On 2016/04/13 07:05:24, hlundin-webrtc wrote: > Creates... Acknowledged. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:64: // |fs| selects sample rate: 8000 for narrowband and 16000 for wideband On 2016/04/13 07:05:24, hlundin-webrtc wrote: > End all sentences with . Acknowledged. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:73: // Reset the comfort noise encoder to its initial state. On 2016/04/13 07:05:23, hlundin-webrtc wrote: > Resets... Acknowledged. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:83: // the rest? On 2016/04/13 07:05:24, hlundin-webrtc wrote: > Or maybe just DCHECK? DCHECK works. Is the right size always 640 or does it depend on the sample rate? https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:86: bool forceSID); On 2016/04/13 07:05:24, hlundin-webrtc wrote: > force_sid Acknowledged. Also, I guess force_sid should go before output. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/comfort_noise.cc (right): https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/comfort_noise.cc:38: packet->payload, packet->payload_length)); On 2016/04/13 07:05:24, hlundin-webrtc wrote: > On 2016/04/12 13:54:50, ossu wrote: > > On 2016/04/12 13:35:31, kwiberg-webrtc wrote: > > > You probably don't need to store the cng decoder in a variable here, since > the > > > return value is never null. (Hmm. That function can return null. I guess it > > will > > > never do so in this case?) > > > > Yeah, it can but it never will. RTC_DCHECK? > > It's going to be a long, ugly line if I combine them, though. > > I vote for a DCHECK. Acknowledged. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/test/RTPencode.cc (right): https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/test/RTPencode.cc:1: /* On 2016/04/13 07:05:24, hlundin-webrtc wrote: > I'm truly sorry that you have to deal with this really old and equally ugly > piece of code. It will be the first against the wall when the revolution > comes... Aww. It's alright. I've seen worse. :) https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/test/RTPencode.cc:933: CNG_encoder[k].reset(new webrtc::ComfortNoiseEncoder(sampfreq, 200, 5)); On 2016/04/13 07:05:24, hlundin-webrtc wrote: > On 2016/04/12 13:54:50, ossu wrote: > > On 2016/04/12 13:35:31, kwiberg-webrtc wrote: > > > The old code created but didn't initialize a CNG encoder if sampfreq > > 16000. > > > Was that an error? > > > > I ... don't know. I put a TODO comment about that just above, hoping someone > > would enlighten me. It seems CNG only supports up to 16k (according to its > > docs). Maybe there needed to be a valid, bad state for all encoders, so that > if > > something used it later, it would fail with NOT_INITIALIZED rather than > probably > > generate garbage that no-one would notice. > > I think we've always said that we only support CNG up to 16 kHz. There is no > technical limitation that prevents higher sample rates, but from a protocol > point of view we have not supported the combination of a super-wideband (>16 > kHz) codec with CNG. As for the other peculiarities in this file, all bets are > off. Acknowledged. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/test/RTPencode.cc:1595: int tempLen; On 2016/04/13 07:05:24, hlundin-webrtc wrote: > Why change this? Because ComfortNoiseEncoder->Encode returns an int that needs to be checked for -1. Though if we change that I can change this back. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/test/RTPencode.cc:1602: const int sampleRate_10 = 10 * sampleRate / 1000; On 2016/04/13 07:05:24, hlundin-webrtc wrote: > And these? Because changing tempLen to int caused a warning about unsigned vs. signed comparisons. As with that one, I'll probably put it back and change Encode instead.
Updated the code based on your comments. Just one last question. https://codereview.webrtc.org/1868143002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h (right): https://codereview.webrtc.org/1868143002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:21: #define WEBRTC_CNG_MAX_LPC_ORDER 12 Since this is shared (and used to size the arrays below) should I: - put it alone in a subnamespace cng, as a const size_t kCngMaxLpOrder = 12 - put a copy in each class below or - just leave it for now?
lgtm, but see comments https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.cc (right): https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.cc:228: RTC_CHECK_GE(encoded_bytes_tmp, 0); On 2016/04/13 07:05:23, hlundin-webrtc wrote: > On 2016/04/12 13:54:49, ossu wrote: > > On 2016/04/12 13:35:30, kwiberg-webrtc wrote: > > > Use rtc::Optional<size_t> if you need to return an error. But as I said, I > > agree > > > that Encode can probably just explode if called with invalid inputs. > > > > > > Also, consider passing the Buffer* into Encode instead of an ArrayView. That > > > way, you completely remove the possibility of it being too small. > > > > Yeah, Buffer is probably better. It's what we want to move to in general, > > internally, anyway, right? > > I'm fine with using a Buffer as well. But bear in mind that CNG is a special > breed. The length of the encoded data has no correlation with the number of > samples inserted. It will just estimate a predetermined number of noise > parameters from the input. Meaning its output length is a small constant? It probably doesn't hurt to use a Buffer anyway---that way, callers won't depend on that constant, and the interface is simpler. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/cng/cng_unittest.cc (right): https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/cng_unittest.cc:41: std::unique_ptr<ComfortNoiseEncoder> cng_encoder_; On 2016/04/13 11:57:06, ossu wrote: > On 2016/04/13 07:05:23, hlundin-webrtc wrote: > > Does it make sense to keep the encoder and decoder as members in the test > > fixture? Feel free to move them to local variables in each TEST_F. Up to > you... > > Yeah, it's probably neater to put them into the tests. I'll do that! Good. Test fixtures are a good idea much more seldom than people in general seem to believe. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/cng/webrtc_cng.cc (right): https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.cc:15: #include <cstring> On 2016/04/13 11:57:06, ossu wrote: > On 2016/04/13 07:05:23, hlundin-webrtc wrote: > > We tend to use the C-version of header files that are available both as C and > > C++ versions. That is, string.h and stdlib.h. > > Honestly, these should probably go away anyway. Also: why do you include them > like that? :) Because that's how we've always done it. Also, it means we don't have to prefix common things like size_t with std::. No, I don't like it either. If std:: makes some common names too long, we could just have a header file that re-exported those names (and only those names!) under shorter aliases. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h (right): https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:75: void Reset(int fs, int interval, int quality); On 2016/04/12 13:54:49, ossu wrote: > On 2016/04/12 13:35:31, kwiberg-webrtc wrote: > > Is this necessary? Can't the callers just make a new instance instead? > > There was some code that used InitEnc before, so I thought I'd keep that > implementation around for now. I'm all for getting rid of Reset - preferably in > both Encoder and Decoder. Yes please. > Hmm, maybe get them back as copyable and remove Reset? Would make it easy to > just reinitialize your local CNG-whatever without reallocations. I don't think resetting a CN encoder/decoder is a frequent enough event to justify the extra effort. https://codereview.webrtc.org/1868143002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h (right): https://codereview.webrtc.org/1868143002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:21: #define WEBRTC_CNG_MAX_LPC_ORDER 12 On 2016/04/14 08:56:30, ossu wrote: > Since this is shared (and used to size the arrays below) should I: > - put it alone in a subnamespace cng, as a const size_t kCngMaxLpOrder = 12 > - put a copy in each class below > or > - just leave it for now? I don't have a suggestion that I would be happy with. Ideally, we'd put this somewhere hidden and let both classes re-export it to their users, but there's no way to not expose everything in a header to the compilation units that #include it. https://codereview.webrtc.org/1868143002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:42: // before any audio is generated. Returns |false| if outData is too large. As you said, you no longer tell callers what "too large" is. Also, why do we have this restriction? What's stopping us from generating any number of output samples? https://codereview.webrtc.org/1868143002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:74: // Analyzes background noise from |speech| and puts coefficients in |output|. appends coefficients to https://codereview.webrtc.org/1868143002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/comfort_noise.cc (right): https://codereview.webrtc.org/1868143002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/comfort_noise.cc:37: RTC_DCHECK(cng_decoder != nullptr); Just "RTC_DCHECK(cng_decoder)" is sufficient.
LGTM, but I'd like you to consolidate one of the tests. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/cng/webrtc_cng.cc (right): https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.cc:15: #include <cstring> On 2016/04/14 09:42:47, kwiberg-webrtc wrote: > On 2016/04/13 11:57:06, ossu wrote: > > On 2016/04/13 07:05:23, hlundin-webrtc wrote: > > > We tend to use the C-version of header files that are available both as C > and > > > C++ versions. That is, string.h and stdlib.h. > > > > Honestly, these should probably go away anyway. Also: why do you include them > > like that? :) > > Because that's how we've always done it. Also, it means we don't have to prefix > common things like size_t with std::. > > No, I don't like it either. If std:: makes some common names too long, we could > just have a header file that re-exported those names (and only those names!) > under shorter aliases. I don't know much more, but I know pbos did an effort a few years ago to unify these cases to use the C-versions in cases like this. https://codereview.webrtc.org/1868143002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.cc:19: On 2016/04/13 11:57:06, ossu wrote: > On 2016/04/13 07:05:23, hlundin-webrtc wrote: > > Add a TODO to update variable an member names to follow code style. > > Acknowledged. I'll do that as a separate CL, I guess. Might be easier to see > that the C++ conversion worked with the implementation generally untouched, > before going through and changing names etc. I believe clang-format changed a > number of things as well, that may be included in such a CL. Acknowledged. https://codereview.webrtc.org/1868143002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/cng/cng_unittest.cc (right): https://codereview.webrtc.org/1868143002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/cng_unittest.cc:55: TEST_F(CngTest, CngInitFail) { You should encapsulate death tests in #if GTEST_HAS_DEATH_TEST ... test test test ... #endif https://codereview.webrtc.org/1868143002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/cng_unittest.cc:64: TEST_F(CngTest, CngEncode) { Now you can consolidate these 5 test cases. Make a method in the test fixture that takes the sample rate as input and performs the test. void CngTest::TestCngEncode(int sample_rate_hz) { ... } TEST_F(CngTest, CngEncode8000) { TestCngEncode(8000); } ... (You could use a parametrized gtest for this, but I think they are a bit obscure.) https://codereview.webrtc.org/1868143002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/cng_unittest.cc:160: TEST_F(CngTest, CngUpdateSidErroneous) { So you're testing Sid Erroneous – why not Sid Vicious? :-P https://codereview.webrtc.org/1868143002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h (right): https://codereview.webrtc.org/1868143002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:21: #define WEBRTC_CNG_MAX_LPC_ORDER 12 On 2016/04/14 09:42:48, kwiberg-webrtc wrote: > On 2016/04/14 08:56:30, ossu wrote: > > Since this is shared (and used to size the arrays below) should I: > > - put it alone in a subnamespace cng, as a const size_t kCngMaxLpOrder = 12 > > - put a copy in each class below > > or > > - just leave it for now? > > I don't have a suggestion that I would be happy with. Ideally, we'd put this > somewhere hidden and let both classes re-export it to their users, but there's > no way to not expose everything in a header to the compilation units that > #include it. Leave it.
solenberg@webrtc.org changed reviewers: - solenberg@webrtc.org
Description was changed from ========== WIP: Cleaning up AudioDecoder interface and make it externally usable. Broke out CNG from AudioDecoder. They didn't really share an interface, anyway. The goal is to get the AudioDecoder interface cleaned up enough to be exposed publicly, and implemented by outside parties. A first step is to at least make that interface reasonable for speech codecs, seeing as CNG is given special treatment all throughout neteq. I believe the ultimate goal should be to have CNG adhere to the same interface as other decoders and minimize the amount of special handling put in for it. ========== to ========== Convert CNG into C++ and remove it from AudioDecoder Broke out CNG from AudioDecoder as they didn't really share an interface. Converted the CNG code to C++, to make initialization and resource handling easier. This includes several changes to the behavior, favoring RTC_CHECKs over returning error codes. ==========
Addressed your comments and updated the CR description. https://codereview.webrtc.org/1868143002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/cng/cng_unittest.cc (right): https://codereview.webrtc.org/1868143002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/cng_unittest.cc:55: TEST_F(CngTest, CngInitFail) { On 2016/04/14 15:20:51, hlundin-webrtc wrote: > You should encapsulate death tests in > #if GTEST_HAS_DEATH_TEST > ... test test test ... > #endif Acknowledged. https://codereview.webrtc.org/1868143002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/cng_unittest.cc:64: TEST_F(CngTest, CngEncode) { On 2016/04/14 15:20:51, hlundin-webrtc wrote: > Now you can consolidate these 5 test cases. Make a method in the test fixture > that takes the sample rate as input and performs the test. > > void CngTest::TestCngEncode(int sample_rate_hz) { > ... > } > > TEST_F(CngTest, CngEncode8000) { > TestCngEncode(8000); > } > > ... > > (You could use a parametrized gtest for this, but I think they are a bit > obscure.) Acknowledged. https://codereview.webrtc.org/1868143002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/cng_unittest.cc:160: TEST_F(CngTest, CngUpdateSidErroneous) { On 2016/04/14 15:20:51, hlundin-webrtc wrote: > So you're testing Sid Erroneous – why not Sid Vicious? :-P Yeah, I put it back. I guess it still makes sense. Had there been a CngUpdateSidVicious, I definitely would have put that one back as well. :) https://codereview.webrtc.org/1868143002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h (right): https://codereview.webrtc.org/1868143002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:42: // before any audio is generated. Returns |false| if outData is too large. On 2016/04/14 09:42:48, kwiberg-webrtc wrote: > As you said, you no longer tell callers what "too large" is. > > Also, why do we have this restriction? What's stopping us from generating any > number of output samples? So I expect this to be 10ms of samples. Right now, the limit is "10ms of samples at 64kHz" (ie. 640). I think I'll mention 640 here explicitly, and add a TODO at this point. I think we should either limit to 10ms in the current sample rate, or change it to allow any output size (possibly a multiple of 10ms frames, but I'm not sure). https://codereview.webrtc.org/1868143002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/webrtc_cng.h:74: // Analyzes background noise from |speech| and puts coefficients in |output|. On 2016/04/14 09:42:48, kwiberg-webrtc wrote: > appends coefficients to Good point! https://codereview.webrtc.org/1868143002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/neteq/comfort_noise.cc (right): https://codereview.webrtc.org/1868143002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/neteq/comfort_noise.cc:37: RTC_DCHECK(cng_decoder != nullptr); On 2016/04/14 09:42:48, kwiberg-webrtc wrote: > Just "RTC_DCHECK(cng_decoder)" is sufficient. Acknowledged.
LGTM with one nit. https://codereview.webrtc.org/1868143002/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/cng/cng_unittest.cc (right): https://codereview.webrtc.org/1868143002/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/cng_unittest.cc:57: const size_t num_samples_10ms = sample_rate_hz / 100; CheckedDivExact
https://codereview.webrtc.org/1868143002/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/cng/cng_unittest.cc (right): https://codereview.webrtc.org/1868143002/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/cng/cng_unittest.cc:57: const size_t num_samples_10ms = sample_rate_hz / 100; On 2016/04/15 12:29:54, hlundin-webrtc wrote: > CheckedDivExact Acknowledged.
Fixed a few compilation issues that were caught by the trybots. The only interesting of which is the following (on VC): rtpencode.cc(268): warning C4592: 'CNG_encoder': symbol will be dynamically initialized (implementation limitation) This warning happens (possibly wrongly) because VC claims it cannot initialize the std::unique_ptr statically - CNG_encoder is a global. I've thus changed it to be raw pointers instead. It's not very nice but RTPencode.cc is a bit funky in general. Who do I talk to about tooling issues such as this? (Specifically, Visual C++ compiler in this case). Also added a comment about an issue caught by asan. I'm not sure where to go with the UpdateSidErronous test. Any ideas? https://codereview.webrtc.org/1868143002/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/cng/cng_unittest.cc (right): https://codereview.webrtc.org/1868143002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/cng/cng_unittest.cc:167: cng_decoder.UpdateSid( ASAN explodes here. The old test had a fixed buffer large enough for this call, whereas now the buffer is allocated during the call to Encode above, and it is thus only kCNGNumParamsNormal + 1 bytes large. What is the correct course of action here? The test itself is limping a bit right now, since it doesn't actually test anything explicitly - it just expects to run without errors and, honestly, it really can't since it's invoked erroneously.
https://codereview.webrtc.org/1868143002/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/neteq/test/RTPencode.cc (right): https://codereview.webrtc.org/1868143002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/test/RTPencode.cc:1454: delete CNG_encoder[k]; I should probably null CNG_encoder[k] here as well...
https://codereview.webrtc.org/1868143002/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/cng/cng_unittest.cc (right): https://codereview.webrtc.org/1868143002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/cng/cng_unittest.cc:167: cng_decoder.UpdateSid( On 2016/04/19 09:48:40, ossu wrote: > ASAN explodes here. The old test had a fixed buffer large enough for this call, > whereas now the buffer is allocated during the call to Encode above, and it is > thus only kCNGNumParamsNormal + 1 bytes large. > > What is the correct course of action here? The test itself is limping a bit > right now, since it doesn't actually test anything explicitly - it just expects > to run without errors and, honestly, it really can't since it's invoked > erroneously. sid_data.SetSize to a large enough size? The current situation is actively asking for out-of-bounds reads, as you say. https://codereview.webrtc.org/1868143002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/cng/cng_unittest.cc (right): https://codereview.webrtc.org/1868143002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/cng/cng_unittest.cc:27: #endif // GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID) It looks like just one test is using this constant. Can you move it there?
On 2016/04/19 09:48:40, ossu wrote: > Fixed a few compilation issues that were caught by the trybots. The only > interesting of which is the following (on VC): > rtpencode.cc(268): warning C4592: 'CNG_encoder': symbol will be dynamically > initialized (implementation limitation) > > This warning happens (possibly wrongly) because VC claims it cannot initialize > the std::unique_ptr statically - CNG_encoder is a global. I've thus changed it > to be raw pointers instead. It's not very nice but RTPencode.cc is a bit funky > in general. Who do I talk to about tooling issues such as this? (Specifically, > Visual C++ compiler in this case). You shouldn't do that: https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables ("Variables of class type with static storage duration are forbidden: they cause hard-to-find bugs due to indeterminate order of construction and destruction.") Ideally, find a way to not use global variables; less ideally, do something along the lines of what you ended up doing with raw pointers.
On 2016/04/19 11:18:57, kwiberg-webrtc wrote: > On 2016/04/19 09:48:40, ossu wrote: > > Fixed a few compilation issues that were caught by the trybots. The only > > interesting of which is the following (on VC): > > rtpencode.cc(268): warning C4592: 'CNG_encoder': symbol will be dynamically > > initialized (implementation limitation) > > > > This warning happens (possibly wrongly) because VC claims it cannot initialize > > the std::unique_ptr statically - CNG_encoder is a global. I've thus changed it > > to be raw pointers instead. It's not very nice but RTPencode.cc is a bit funky > > in general. Who do I talk to about tooling issues such as this? (Specifically, > > Visual C++ compiler in this case). > > You shouldn't do that: > https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables > ("Variables of class type with static storage duration are forbidden: they cause > hard-to-find bugs due to indeterminate order of construction and destruction.") > Ideally, find a way to not use global variables; less ideally, do something > along the lines of what you ended up doing with raw pointers. That is true. I was under the impression that this warning was specific to the newly adopted VC 2015.1 here - it warns even on things it shouldn't according to stackoverflow. That said, in this case it's really wrong to have it like that. I didn't check properly and expected I was dealing with class variables. :/ Do we have something that properly checks for this? The linter doesn't seem to catch it.
On 2016/04/19 12:09:15, ossu wrote: > Do we have something that properly checks for this? The linter doesn't seem to > catch it. I don't think so.
https://codereview.webrtc.org/1868143002/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/cng/cng_unittest.cc (right): https://codereview.webrtc.org/1868143002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/cng/cng_unittest.cc:167: cng_decoder.UpdateSid( On 2016/04/19 11:14:30, kwiberg-webrtc wrote: > On 2016/04/19 09:48:40, ossu wrote: > > ASAN explodes here. The old test had a fixed buffer large enough for this > call, > > whereas now the buffer is allocated during the call to Encode above, and it is > > thus only kCNGNumParamsNormal + 1 bytes large. > > > > What is the correct course of action here? The test itself is limping a bit > > right now, since it doesn't actually test anything explicitly - it just > expects > > to run without errors and, honestly, it really can't since it's invoked > > erroneously. > > sid_data.SetSize to a large enough size? The current situation is actively > asking for out-of-bounds reads, as you say. Thing is: I'm not clear on the point of this test anymore. I removed this test for a while, since there was no return value to check. Now it runs but, well, clearly the second invocation is wrong and there's no longer a way to reasonably introduce too large a buffer here. If the purpose is to check that the decoder _doesn't_ touch more than kCNGNumParamsNormal + 1, then ASAN tells us that the implementation is wrong. I guess the point is that we should only use up to WEBRTC_CNG_MAX_LPC_ORDER. Why is that interesting to check? The data in-between ought to be garbage. https://codereview.webrtc.org/1868143002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/cng/cng_unittest.cc (right): https://codereview.webrtc.org/1868143002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/cng/cng_unittest.cc:27: #endif // GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID) On 2016/04/19 11:14:30, kwiberg-webrtc wrote: > It looks like just one test is using this constant. Can you move it there? I can. Guess it's ok to keep it separate from the others.
On 2016/04/19 12:21:52, ossu wrote: > https://codereview.webrtc.org/1868143002/diff/100001/webrtc/modules/audio_cod... > File webrtc/modules/audio_coding/codecs/cng/cng_unittest.cc (right): > > https://codereview.webrtc.org/1868143002/diff/100001/webrtc/modules/audio_cod... > webrtc/modules/audio_coding/codecs/cng/cng_unittest.cc:167: > cng_decoder.UpdateSid( > On 2016/04/19 11:14:30, kwiberg-webrtc wrote: > > On 2016/04/19 09:48:40, ossu wrote: > > > ASAN explodes here. The old test had a fixed buffer large enough for this > > call, > > > whereas now the buffer is allocated during the call to Encode above, and it > is > > > thus only kCNGNumParamsNormal + 1 bytes large. > > > > > > What is the correct course of action here? The test itself is limping a bit > > > right now, since it doesn't actually test anything explicitly - it just > > expects > > > to run without errors and, honestly, it really can't since it's invoked > > > erroneously. > > > > sid_data.SetSize to a large enough size? The current situation is actively > > asking for out-of-bounds reads, as you say. > > Thing is: I'm not clear on the point of this test anymore. I removed this test > for a while, since there was no return value to check. Now it runs but, well, > clearly the second invocation is wrong and there's no longer a way to reasonably > introduce too large a buffer here. If the purpose is to check that the decoder > _doesn't_ touch more than kCNGNumParamsNormal + 1, then ASAN tells us that the > implementation is wrong. I guess the point is that we should only use up to > WEBRTC_CNG_MAX_LPC_ORDER. Why is that interesting to check? The data in-between > ought to be garbage. > > https://codereview.webrtc.org/1868143002/diff/120001/webrtc/modules/audio_cod... > File webrtc/modules/audio_coding/codecs/cng/cng_unittest.cc (right): > > https://codereview.webrtc.org/1868143002/diff/120001/webrtc/modules/audio_cod... > webrtc/modules/audio_coding/codecs/cng/cng_unittest.cc:27: #endif // > GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID) > On 2016/04/19 11:14:30, kwiberg-webrtc wrote: > > It looks like just one test is using this constant. Can you move it there? > > I can. Guess it's ok to keep it separate from the others. Is anything preventing this CL from landing?
On 2016/04/20 13:42:32, kwiberg-webrtc wrote: > On 2016/04/19 12:21:52, ossu wrote: > > > https://codereview.webrtc.org/1868143002/diff/100001/webrtc/modules/audio_cod... > > File webrtc/modules/audio_coding/codecs/cng/cng_unittest.cc (right): > > > > > https://codereview.webrtc.org/1868143002/diff/100001/webrtc/modules/audio_cod... > > webrtc/modules/audio_coding/codecs/cng/cng_unittest.cc:167: > > cng_decoder.UpdateSid( > > On 2016/04/19 11:14:30, kwiberg-webrtc wrote: > > > On 2016/04/19 09:48:40, ossu wrote: > > > > ASAN explodes here. The old test had a fixed buffer large enough for this > > > call, > > > > whereas now the buffer is allocated during the call to Encode above, and > it > > is > > > > thus only kCNGNumParamsNormal + 1 bytes large. > > > > > > > > What is the correct course of action here? The test itself is limping a > bit > > > > right now, since it doesn't actually test anything explicitly - it just > > > expects > > > > to run without errors and, honestly, it really can't since it's invoked > > > > erroneously. > > > > > > sid_data.SetSize to a large enough size? The current situation is actively > > > asking for out-of-bounds reads, as you say. > > > > Thing is: I'm not clear on the point of this test anymore. I removed this test > > for a while, since there was no return value to check. Now it runs but, well, > > clearly the second invocation is wrong and there's no longer a way to > reasonably > > introduce too large a buffer here. If the purpose is to check that the decoder > > _doesn't_ touch more than kCNGNumParamsNormal + 1, then ASAN tells us that the > > implementation is wrong. I guess the point is that we should only use up to > > WEBRTC_CNG_MAX_LPC_ORDER. Why is that interesting to check? The data > in-between > > ought to be garbage. > > > > > https://codereview.webrtc.org/1868143002/diff/120001/webrtc/modules/audio_cod... > > File webrtc/modules/audio_coding/codecs/cng/cng_unittest.cc (right): > > > > > https://codereview.webrtc.org/1868143002/diff/120001/webrtc/modules/audio_cod... > > webrtc/modules/audio_coding/codecs/cng/cng_unittest.cc:27: #endif // > > GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID) > > On 2016/04/19 11:14:30, kwiberg-webrtc wrote: > > > It looks like just one test is using this constant. Can you move it there? > > > > I can. Guess it's ok to keep it separate from the others. > > Is anything preventing this CL from landing? Yes, I need to resolve how the test UpdateSidErroneous should be designed - what we're actually testing. Probably needs another RTC_CHECK in UpdateSid as well. hlundin: Do you have any insight into that test? Should it really check against WEBRTC_CNG_MAX_LPC_ORDER? How many bytes should UpdateSid really eat during an invocation?
Fixed the two remaining issues: UpdateSidErroneous and the sometimes unused constant in the cng unittests. I fixed the latter by reverting them to being part of an enum, but now typed to be size_t. After this I did a rebase, since kwibergs fixes to DecoderDatabase clashed with mine. PTAL on those files as well. I had to do a small amount of manual interference to get the rebase to go through.
Still LGTM. https://codereview.webrtc.org/1868143002/diff/160001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/neteq/decoder_database.cc (right): https://codereview.webrtc.org/1868143002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/decoder_database.cc:23: : active_decoder_type_(-1), active_cng_decoder_type_(-1) { I just realized that these are probably good candidates for rtc::Optional. But let's not clutter this CL with that.
Still lgtm. https://codereview.webrtc.org/1868143002/diff/160001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/cng/cng_unittest.cc (right): https://codereview.webrtc.org/1868143002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/cng/cng_unittest.cc:173: rtc::ArrayView<const uint8_t>(sid_data.data(), kCNGNumParamsTooHigh + 1)); Now that we pass an ArrayView to UpdateSid, it's very clear that UpdateSid isnt't supposed to read outside it. Also, you should take advantage of implicit conversion to ArrayView. So maybe just something like this: // First run with valid parameters, then with too many CNG parameters. EXPECT_EQ(kCNGNumParamsNormal + 1, sid_data.size()); cng_decoder.UpdateSid(sid_data); sid_data.SetSize(kCNGNumParamsTooHigh + 1); cng_decoder.UpdateSid(sid_data);
On 2016/04/25 11:35:12, kwiberg-webrtc wrote: > Still lgtm. > > https://codereview.webrtc.org/1868143002/diff/160001/webrtc/modules/audio_cod... > File webrtc/modules/audio_coding/codecs/cng/cng_unittest.cc (right): > > https://codereview.webrtc.org/1868143002/diff/160001/webrtc/modules/audio_cod... > webrtc/modules/audio_coding/codecs/cng/cng_unittest.cc:173: rtc::ArrayView<const > uint8_t>(sid_data.data(), kCNGNumParamsTooHigh + 1)); > Now that we pass an ArrayView to UpdateSid, it's very clear that UpdateSid > isnt't supposed to read outside it. Also, you should take advantage of implicit > conversion to ArrayView. So maybe just something like this: > > // First run with valid parameters, then with too many CNG parameters. > EXPECT_EQ(kCNGNumParamsNormal + 1, sid_data.size()); > cng_decoder.UpdateSid(sid_data); > sid_data.SetSize(kCNGNumParamsTooHigh + 1); > cng_decoder.UpdateSid(sid_data); Agreed.
The CQ bit was checked by ossu@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org, kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/1868143002/#ps180001 (title: "Implicit conversion to ArrayView in TestSidErroneous")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1868143002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1868143002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by ossu@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1868143002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1868143002/180001
Message was sent while issue was closed.
Description was changed from ========== Convert CNG into C++ and remove it from AudioDecoder Broke out CNG from AudioDecoder as they didn't really share an interface. Converted the CNG code to C++, to make initialization and resource handling easier. This includes several changes to the behavior, favoring RTC_CHECKs over returning error codes. ========== to ========== Convert CNG into C++ and remove it from AudioDecoder Broke out CNG from AudioDecoder as they didn't really share an interface. Converted the CNG code to C++, to make initialization and resource handling easier. This includes several changes to the behavior, favoring RTC_CHECKs over returning error codes. ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Convert CNG into C++ and remove it from AudioDecoder Broke out CNG from AudioDecoder as they didn't really share an interface. Converted the CNG code to C++, to make initialization and resource handling easier. This includes several changes to the behavior, favoring RTC_CHECKs over returning error codes. ========== to ========== Convert CNG into C++ and remove it from AudioDecoder Broke out CNG from AudioDecoder as they didn't really share an interface. Converted the CNG code to C++, to make initialization and resource handling easier. This includes several changes to the behavior, favoring RTC_CHECKs over returning error codes. Committed: https://crrev.com/97ba30eedf99e84de33200b7546d5d578fe358e6 Cr-Commit-Position: refs/heads/master@{#12491} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/97ba30eedf99e84de33200b7546d5d578fe358e6 Cr-Commit-Position: refs/heads/master@{#12491} |
