Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1967503002:
Audio codec usage statistics (Closed)
|
Created:
4 years, 7 months ago by aleloi 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. |
DescriptionAdded UMA logging for audio codec usage. A histogram statistic named "WebRTC.Audio.Encoder.CodecType" has been created.
Committed: https://crrev.com/8bce67b7451d2d75390410dd7bbc82078ccfb2c9
Cr-Commit-Position: refs/heads/master@{#12760}
Patch Set 1 #
Total comments: 38
Patch Set 2 : CL attempt 2. Added/responded to most comments #
Total comments: 13
Patch Set 3 : Moved logging to AudioCodingModuleImpl #
Total comments: 50
Patch Set 4 : Minor changes to comply with style guide #Patch Set 5 : Small fixes, formatting, responding to comments #Patch Set 6 : Removed strings and added enum classes #
Total comments: 10
Patch Set 7 : Minor fixes + counting bugfix #
Total comments: 21
Patch Set 8 : Minor fixes #
Total comments: 9
Patch Set 9 : Very small changes from ossu's and hlundin's comments. #
Total comments: 4
Patch Set 10 : Hopefully last version, minimal changes. #
Total comments: 10
Patch Set 11 : Removed unnecessary scoping operators #Patch Set 12 : Removed unneeded 'virtual' #Patch Set 13 : int conversion #Patch Set 14 : explicit casts #Messages
Total messages: 77 (23 generated)
Description was changed from ========== Formatting. Added codec naming and logging to WebRTC audio BUG= ========== to ========== Added UMA logging for audio codec usage. A histogram statistic named "WebRTC.Audio.Encoder.CodecType" has been created. ==========
aleloi@webrtc.org changed reviewers: + henrik.lundin@webrtc.org, kwiberg@webrtc.org
Hi! This is a CL of my first Noogler project. Please take a look!
aleloi@webrtc.org changed reviewers: + solenberg@webrtc.org
Looks good, but a few comments: https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_encoder.cc (right): https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_encoder.cc:36: const float kProbabilityToLogCodecType = 1.0 / 500.0; constexpr? https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_encoder.cc:39: // one of the codec ID:s in CodecID "ID:s" -> "IDs". What is CodecID? https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_encoder.cc:41: if (strcmp("Opus", codec_name) == 0) codec_name can be null, but the behavior of strcmp is undefined if you pass it anything but pointers to null-terminated strings. https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_encoder.cc:43: else if (strcmp("iSac", codec_name) == 0) iSAC https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_encoder.cc:53: } Hmm. You end up having to basically maintain two copies of the codec list, one enum and one in the code. Do you think a table-driven approach like this might work well?: static const struct { const char* name; int id; } histogram_id = { {"Opus", 1}, ... }; for (const auto& codec : histogram_id) { if (std::strcmp(codec.name, codec_name) == 0) return codec.id; } https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_encoder.cc:85: if (rand() / RAND_MAX < kProbabilityToLogCodecType) std::rand Also, since std::rand() returns an int, and RAND_MAX is an int, the quotient will always be 0, so the condition will always be true. Maybe try something like constexpr int threshold = RAND_MAX * kProbabilityToLogCodecType; if (rand() < threshold) https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_encoder.h:139: // Subclasses implements this and return a name, e.g. "Opus" or "iSAC". Despite the example set by EncodeImpl, you probably don't need to say that subclasses can implement the method---the "virtual" already says that. But it would be useful to document the lifetime of the returned string, and what the default implementation does. Maybe // Returns a pointer to a statically allocated string with the codec name, // e.g. "Opus" or "iSAC". The default implementation returns null, // which means no name. https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.h (right): https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.h:58: const char* GetCodecName() const override; AudioEncoderPcm is also base class for the 16-bit pcm encoder, which IIRC isn't called g711. You probably need to move this override to the leaf classes.
https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_encoder.cc (right): https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_encoder.cc:85: if (rand() / RAND_MAX < kProbabilityToLogCodecType) On 2016/05/11 00:47:00, kwiberg-webrtc wrote: > std::rand > > Also, since std::rand() returns an int, and RAND_MAX is an int, the quotient > will always be 0, so the condition will always be true. Maybe try something like > > constexpr int threshold = RAND_MAX * kProbabilityToLogCodecType; > if (rand() < threshold) We shouldn't be using std::rand() since: a) it relies on shared global state b) the PRNG used is implementation dependent so we have no control over how fair the generator is (it may even skew results depending on platform). c) the implementations I've seen use a simple mult+add which is known to not generate very long cycles. The alternative is to add the PRNG in the class, but then we have state again so might as well resort to using an int counter like the design doc proposes. Once we have injectable encoders, we'll nevertheless be able to pull this bookkeeping from the public interface and keep it in an internal decorator object, so I think we can live with adding state to the class for now.
Nice first CL! See comments inline. https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_encoder.cc (right): https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_encoder.cc:23: enum HistogramAudioCodecType { Can you use enum class here, or is there something in the UMA mechanics that prevents it? https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_encoder.cc:41: if (strcmp("Opus", codec_name) == 0) We should make sure that the naming is consistent with _some_ standard. I think looking at the SDP "encoding name" value for different codecs is good. That would give us: - "opus" (https://tools.ietf.org/html/draft-ietf-payload-rtp-opus-11#section-7) - "isac-16000" and "isac-32000" (https://tools.ietf.org/html/draft-ietf-payload-rtp-opus-11#section-7), - "iLBC" (https://tools.ietf.org/html/rfc3952#section-5), - Bunch of "fixed payload types defined in https://tools.ietf.org/html/rfc3551#section-4.5 -- G722, -- PCMA (for g711A), -- PCMU (for g711u), -- L16-8000 for PCM16@8kHz, etc. The formatting is a bit mixed, I agree, but I think it is nice to stick to some standard. WDYT? https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_encoder.h:139: // Subclasses implements this and return a name, e.g. "Opus" or "iSAC". On 2016/05/11 00:47:00, kwiberg-webrtc wrote: > Despite the example set by EncodeImpl, you probably don't need to say that > subclasses can implement the method---the "virtual" already says that. But it > would be useful to document the lifetime of the returned string, and what the > default implementation does. Maybe > > // Returns a pointer to a statically allocated string with the codec name, > // e.g. "Opus" or "iSAC". The default implementation returns null, > // which means no name. I'm not sure we should promise statically allocated strings. We talked about wanting to distinguish between different codec settings, e.g., Opus with and without DTX enabled. Also, for both iSAC and the PCM16 "codec", the sample rate may vary. https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc (right): https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc:107: return "g711"; Must distinguish between types "A" and "U".
https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_encoder.cc (right): https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_encoder.cc:85: if (rand() / RAND_MAX < kProbabilityToLogCodecType) On 2016/05/11 08:18:43, the sun wrote: > On 2016/05/11 00:47:00, kwiberg-webrtc wrote: > > std::rand > > > > Also, since std::rand() returns an int, and RAND_MAX is an int, the quotient > > will always be 0, so the condition will always be true. Maybe try something > like > > > > constexpr int threshold = RAND_MAX * kProbabilityToLogCodecType; > > if (rand() < threshold) > > We shouldn't be using std::rand() since: > a) it relies on shared global state Yeah, that's suboptimal because locking. > b) the PRNG used is implementation dependent so we have no control over how fair > the generator is (it may even skew results depending on platform). I haven't done the math, but it seems like you'd need a very poor PRNG indeed to botch the job here. After all, we're almost just as happy with a "PRNG" that simply says no 499 times, then yes once, then repeats. > c) the implementations I've seen use a simple mult+add which is known to not > generate very long cycles. > > The alternative is to add the PRNG in the class, but then we have state again so > might as well resort to using an int counter like the design doc proposes. Or to have a thread-local PRNG. > Once we have injectable encoders, we'll nevertheless be able to pull this > bookkeeping from the public interface and keep it in an internal decorator > object, so I think we can live with adding state to the class for now. There's nothing stopping us from doing that right away, is there? We can just put the state (and the logging) at the call site (IIRC from our discussions, there is only one call site that matters). That's probably a pragmatic approach to getting this CL landed in a timely manner... https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_encoder.h:139: // Subclasses implements this and return a name, e.g. "Opus" or "iSAC". On 2016/05/11 08:27:26, hlundin-webrtc wrote: > On 2016/05/11 00:47:00, kwiberg-webrtc wrote: > > Despite the example set by EncodeImpl, you probably don't need to say that > > subclasses can implement the method---the "virtual" already says that. But it > > would be useful to document the lifetime of the returned string, and what the > > default implementation does. Maybe > > > > // Returns a pointer to a statically allocated string with the codec name, > > // e.g. "Opus" or "iSAC". The default implementation returns null, > > // which means no name. > > I'm not sure we should promise statically allocated strings. We talked about > wanting to distinguish between different codec settings, e.g., Opus with and > without DTX enabled. Also, for both iSAC and the PCM16 "codec", the sample rate > may vary. OK. What lifetime should we promise, then? That of the AudioEncoder instance?
https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_encoder.h:139: // Subclasses implements this and return a name, e.g. "Opus" or "iSAC". On 2016/05/11 08:52:24, kwiberg-webrtc wrote: > On 2016/05/11 08:27:26, hlundin-webrtc wrote: > > On 2016/05/11 00:47:00, kwiberg-webrtc wrote: > > > Despite the example set by EncodeImpl, you probably don't need to say that > > > subclasses can implement the method---the "virtual" already says that. But > it > > > would be useful to document the lifetime of the returned string, and what > the > > > default implementation does. Maybe > > > > > > // Returns a pointer to a statically allocated string with the codec name, > > > // e.g. "Opus" or "iSAC". The default implementation returns null, > > > // which means no name. > > > > I'm not sure we should promise statically allocated strings. We talked about > > wanting to distinguish between different codec settings, e.g., Opus with and > > without DTX enabled. Also, for both iSAC and the PCM16 "codec", the sample > rate > > may vary. > > OK. What lifetime should we promise, then? That of the AudioEncoder instance? That seems reasonable, but promising the correct value is another issue. I mean, _if_ we are going to distinguish between Opus with and without DTX, that is going to be a runtime parameter. So, correct string value can only be guaranteed until the settings change. Can we be conservative and just promise lifetime until the next call to any other method (or lifetime, whichever is shorter)?
https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_encoder.h:139: // Subclasses implements this and return a name, e.g. "Opus" or "iSAC". On 2016/05/11 08:59:18, hlundin-webrtc wrote: > On 2016/05/11 08:52:24, kwiberg-webrtc wrote: > > On 2016/05/11 08:27:26, hlundin-webrtc wrote: > > > On 2016/05/11 00:47:00, kwiberg-webrtc wrote: > > > > Despite the example set by EncodeImpl, you probably don't need to say that > > > > subclasses can implement the method---the "virtual" already says that. But > > it > > > > would be useful to document the lifetime of the returned string, and what > > the > > > > default implementation does. Maybe > > > > > > > > // Returns a pointer to a statically allocated string with the codec > name, > > > > // e.g. "Opus" or "iSAC". The default implementation returns null, > > > > // which means no name. > > > > > > I'm not sure we should promise statically allocated strings. We talked about > > > wanting to distinguish between different codec settings, e.g., Opus with and > > > without DTX enabled. Also, for both iSAC and the PCM16 "codec", the sample > > rate > > > may vary. > > > > OK. What lifetime should we promise, then? That of the AudioEncoder instance? > > That seems reasonable, but promising the correct value is another issue. I mean, > _if_ we are going to distinguish between Opus with and without DTX, that is > going to be a runtime parameter. So, correct string value can only be guaranteed > until the settings change. Can we be conservative and just promise lifetime > until the next call to any other method (or lifetime, whichever is shorter)? I think it's problematic if we have an interface that requires the encoder to generate these strings on the fly. That's costly, and besides means there are a lot of them, which will cause problems for the consumer (remember the limit of 64 codec names from elsewhere in this CL?). If the number of possible names an encoder can return is small, they can all be kept in a table with encoder instance or even static life time.
https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_encoder.cc (right): https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_encoder.cc:85: if (rand() / RAND_MAX < kProbabilityToLogCodecType) On 2016/05/11 08:52:24, kwiberg-webrtc wrote: > On 2016/05/11 08:18:43, the sun wrote: > > On 2016/05/11 00:47:00, kwiberg-webrtc wrote: > > > std::rand > > > > > > Also, since std::rand() returns an int, and RAND_MAX is an int, the quotient > > > will always be 0, so the condition will always be true. Maybe try something > > like > > > > > > constexpr int threshold = RAND_MAX * kProbabilityToLogCodecType; > > > if (rand() < threshold) > > > > We shouldn't be using std::rand() since: > > a) it relies on shared global state > > Yeah, that's suboptimal because locking. > > > b) the PRNG used is implementation dependent so we have no control over how > fair > > the generator is (it may even skew results depending on platform). > > I haven't done the math, but it seems like you'd need a very poor PRNG indeed to > botch the job here. After all, we're almost just as happy with a "PRNG" that > simply says no 499 times, then yes once, then repeats. > > > c) the implementations I've seen use a simple mult+add which is known to not > > generate very long cycles. > > > > The alternative is to add the PRNG in the class, but then we have state again > so > > might as well resort to using an int counter like the design doc proposes. > > Or to have a thread-local PRNG. > > > Once we have injectable encoders, we'll nevertheless be able to pull this > > bookkeeping from the public interface and keep it in an internal decorator > > object, so I think we can live with adding state to the class for now. > > There's nothing stopping us from doing that right away, is there? We can just > put the state (and the logging) at the call site (IIRC from our discussions, > there is only one call site that matters). That's probably a pragmatic approach > to getting this CL landed in a timely manner... Two sites: Encode() and dtor.
https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_encoder.cc (right): https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_encoder.cc:85: if (rand() / RAND_MAX < kProbabilityToLogCodecType) On 2016/05/11 10:35:01, the sun wrote: > On 2016/05/11 08:52:24, kwiberg-webrtc wrote: > > On 2016/05/11 08:18:43, the sun wrote: > > > On 2016/05/11 00:47:00, kwiberg-webrtc wrote: > > > > std::rand > > > > > > > > Also, since std::rand() returns an int, and RAND_MAX is an int, the > quotient > > > > will always be 0, so the condition will always be true. Maybe try > something > > > like > > > > > > > > constexpr int threshold = RAND_MAX * kProbabilityToLogCodecType; > > > > if (rand() < threshold) > > > > > > We shouldn't be using std::rand() since: > > > a) it relies on shared global state > > > > Yeah, that's suboptimal because locking. > > > > > b) the PRNG used is implementation dependent so we have no control over how > > fair > > > the generator is (it may even skew results depending on platform). > > > > I haven't done the math, but it seems like you'd need a very poor PRNG indeed > to > > botch the job here. After all, we're almost just as happy with a "PRNG" that > > simply says no 499 times, then yes once, then repeats. > > > > > c) the implementations I've seen use a simple mult+add which is known to not > > > generate very long cycles. > > > > > > The alternative is to add the PRNG in the class, but then we have state > again > > so > > > might as well resort to using an int counter like the design doc proposes. > > > > Or to have a thread-local PRNG. > > > > > Once we have injectable encoders, we'll nevertheless be able to pull this > > > bookkeeping from the public interface and keep it in an internal decorator > > > object, so I think we can live with adding state to the class for now. > > > > There's nothing stopping us from doing that right away, is there? We can just > > put the state (and the logging) at the call site (IIRC from our discussions, > > there is only one call site that matters). That's probably a pragmatic > approach > > to getting this CL landed in a timely manner... > > Two sites: Encode() and dtor. dtor only if we care about the "remainder" (fraction of 5 seconds) that is unlogged on destruction. All codecs will suffer the same bias if we omit the dtor logging. Just saying, if the dtor logging makes the solution much more complicated. https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_encoder.h:139: // Subclasses implements this and return a name, e.g. "Opus" or "iSAC". On 2016/05/11 09:49:43, kwiberg-webrtc wrote: > On 2016/05/11 08:59:18, hlundin-webrtc wrote: > > On 2016/05/11 08:52:24, kwiberg-webrtc wrote: > > > On 2016/05/11 08:27:26, hlundin-webrtc wrote: > > > > On 2016/05/11 00:47:00, kwiberg-webrtc wrote: > > > > > Despite the example set by EncodeImpl, you probably don't need to say > that > > > > > subclasses can implement the method---the "virtual" already says that. > But > > > it > > > > > would be useful to document the lifetime of the returned string, and > what > > > the > > > > > default implementation does. Maybe > > > > > > > > > > // Returns a pointer to a statically allocated string with the codec > > name, > > > > > // e.g. "Opus" or "iSAC". The default implementation returns null, > > > > > // which means no name. > > > > > > > > I'm not sure we should promise statically allocated strings. We talked > about > > > > wanting to distinguish between different codec settings, e.g., Opus with > and > > > > without DTX enabled. Also, for both iSAC and the PCM16 "codec", the sample > > > rate > > > > may vary. > > > > > > OK. What lifetime should we promise, then? That of the AudioEncoder > instance? > > > > That seems reasonable, but promising the correct value is another issue. I > mean, > > _if_ we are going to distinguish between Opus with and without DTX, that is > > going to be a runtime parameter. So, correct string value can only be > guaranteed > > until the settings change. Can we be conservative and just promise lifetime > > until the next call to any other method (or lifetime, whichever is shorter)? > > I think it's problematic if we have an interface that requires the encoder to > generate these strings on the fly. That's costly, and besides means there are a > lot of them, which will cause problems for the consumer (remember the limit of > 64 codec names from elsewhere in this CL?). > > If the number of possible names an encoder can return is small, they can all be > kept in a table with encoder instance or even static life time. I too was thinking of a "small set" of statically defined strings. But still, the output from GetCodecName will not be statically known, and the correctness-time of the string may be shorter than the lifetime of the object.
https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_encoder.cc (right): https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_encoder.cc:85: if (rand() / RAND_MAX < kProbabilityToLogCodecType) On 2016/05/11 10:37:20, hlundin-webrtc wrote: > On 2016/05/11 10:35:01, the sun wrote: > > On 2016/05/11 08:52:24, kwiberg-webrtc wrote: > > > On 2016/05/11 08:18:43, the sun wrote: > > > > On 2016/05/11 00:47:00, kwiberg-webrtc wrote: > > > > > std::rand > > > > > > > > > > Also, since std::rand() returns an int, and RAND_MAX is an int, the > > quotient > > > > > will always be 0, so the condition will always be true. Maybe try > > something > > > > like > > > > > > > > > > constexpr int threshold = RAND_MAX * kProbabilityToLogCodecType; > > > > > if (rand() < threshold) > > > > > > > > We shouldn't be using std::rand() since: > > > > a) it relies on shared global state > > > > > > Yeah, that's suboptimal because locking. > > > > > > > b) the PRNG used is implementation dependent so we have no control over > how > > > fair > > > > the generator is (it may even skew results depending on platform). > > > > > > I haven't done the math, but it seems like you'd need a very poor PRNG > indeed > > to > > > botch the job here. After all, we're almost just as happy with a "PRNG" that > > > simply says no 499 times, then yes once, then repeats. > > > > > > > c) the implementations I've seen use a simple mult+add which is known to > not > > > > generate very long cycles. > > > > > > > > The alternative is to add the PRNG in the class, but then we have state > > again > > > so > > > > might as well resort to using an int counter like the design doc proposes. > > > > > > Or to have a thread-local PRNG. > > > > > > > Once we have injectable encoders, we'll nevertheless be able to pull this > > > > bookkeeping from the public interface and keep it in an internal decorator > > > > object, so I think we can live with adding state to the class for now. > > > > > > There's nothing stopping us from doing that right away, is there? We can > just > > > put the state (and the logging) at the call site (IIRC from our discussions, > > > there is only one call site that matters). That's probably a pragmatic > > approach > > > to getting this CL landed in a timely manner... > > > > Two sites: Encode() and dtor. > > dtor only if we care about the "remainder" (fraction of 5 seconds) that is > unlogged on destruction. All codecs will suffer the same bias if we omit the > dtor logging. Just saying, if the dtor logging makes the solution much more > complicated. Short-lived codecs will have a different bias than long-lived codecs if you use a counter and don't update it in the destructor. But unless my math intuition fails me, this will not be the case if we log randomly. https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_encoder.h:139: // Subclasses implements this and return a name, e.g. "Opus" or "iSAC". On 2016/05/11 10:37:20, hlundin-webrtc wrote: > On 2016/05/11 09:49:43, kwiberg-webrtc wrote: > > On 2016/05/11 08:59:18, hlundin-webrtc wrote: > > > On 2016/05/11 08:52:24, kwiberg-webrtc wrote: > > > > On 2016/05/11 08:27:26, hlundin-webrtc wrote: > > > > > On 2016/05/11 00:47:00, kwiberg-webrtc wrote: > > > > > > Despite the example set by EncodeImpl, you probably don't need to say > > that > > > > > > subclasses can implement the method---the "virtual" already says that. > > But > > > > it > > > > > > would be useful to document the lifetime of the returned string, and > > what > > > > the > > > > > > default implementation does. Maybe > > > > > > > > > > > > // Returns a pointer to a statically allocated string with the codec > > > name, > > > > > > // e.g. "Opus" or "iSAC". The default implementation returns null, > > > > > > // which means no name. > > > > > > > > > > I'm not sure we should promise statically allocated strings. We talked > > about > > > > > wanting to distinguish between different codec settings, e.g., Opus with > > and > > > > > without DTX enabled. Also, for both iSAC and the PCM16 "codec", the > sample > > > > rate > > > > > may vary. > > > > > > > > OK. What lifetime should we promise, then? That of the AudioEncoder > > instance? > > > > > > That seems reasonable, but promising the correct value is another issue. I > > mean, > > > _if_ we are going to distinguish between Opus with and without DTX, that is > > > going to be a runtime parameter. So, correct string value can only be > > guaranteed > > > until the settings change. Can we be conservative and just promise lifetime > > > until the next call to any other method (or lifetime, whichever is shorter)? > > > > I think it's problematic if we have an interface that requires the encoder to > > generate these strings on the fly. That's costly, and besides means there are > a > > lot of them, which will cause problems for the consumer (remember the limit of > > 64 codec names from elsewhere in this CL?). > > > > If the number of possible names an encoder can return is small, they can all > be > > kept in a table with encoder instance or even static life time. > > I too was thinking of a "small set" of statically defined strings. But still, > the output from GetCodecName will not be statically known, and the > correctness-time of the string may be shorter than the lifetime of the object. But the complete set of strings possibly returned by the encoder needs to be known by the caller, so it can't be so large that it's infeasible for the encoder to just store them all in a small table.
https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_encoder.cc (right): https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_encoder.cc:85: if (rand() / RAND_MAX < kProbabilityToLogCodecType) On 2016/05/11 10:46:21, kwiberg-webrtc wrote: > On 2016/05/11 10:37:20, hlundin-webrtc wrote: > > On 2016/05/11 10:35:01, the sun wrote: > > > On 2016/05/11 08:52:24, kwiberg-webrtc wrote: > > > > On 2016/05/11 08:18:43, the sun wrote: > > > > > On 2016/05/11 00:47:00, kwiberg-webrtc wrote: > > > > > > std::rand > > > > > > > > > > > > Also, since std::rand() returns an int, and RAND_MAX is an int, the > > > quotient > > > > > > will always be 0, so the condition will always be true. Maybe try > > > something > > > > > like > > > > > > > > > > > > constexpr int threshold = RAND_MAX * kProbabilityToLogCodecType; > > > > > > if (rand() < threshold) > > > > > > > > > > We shouldn't be using std::rand() since: > > > > > a) it relies on shared global state > > > > > > > > Yeah, that's suboptimal because locking. > > > > > > > > > b) the PRNG used is implementation dependent so we have no control over > > how > > > > fair > > > > > the generator is (it may even skew results depending on platform). > > > > > > > > I haven't done the math, but it seems like you'd need a very poor PRNG > > indeed > > > to > > > > botch the job here. After all, we're almost just as happy with a "PRNG" > that > > > > simply says no 499 times, then yes once, then repeats. > > > > > > > > > c) the implementations I've seen use a simple mult+add which is known to > > not > > > > > generate very long cycles. > > > > > > > > > > The alternative is to add the PRNG in the class, but then we have state > > > again > > > > so > > > > > might as well resort to using an int counter like the design doc > proposes. > > > > > > > > Or to have a thread-local PRNG. > > > > > > > > > Once we have injectable encoders, we'll nevertheless be able to pull > this > > > > > bookkeeping from the public interface and keep it in an internal > decorator > > > > > object, so I think we can live with adding state to the class for now. > > > > > > > > There's nothing stopping us from doing that right away, is there? We can > > just > > > > put the state (and the logging) at the call site (IIRC from our > discussions, > > > > there is only one call site that matters). That's probably a pragmatic > > > approach > > > > to getting this CL landed in a timely manner... > > > > > > Two sites: Encode() and dtor. > > > > dtor only if we care about the "remainder" (fraction of 5 seconds) that is > > unlogged on destruction. All codecs will suffer the same bias if we omit the > > dtor logging. Just saying, if the dtor logging makes the solution much more > > complicated. > > Short-lived codecs will have a different bias than long-lived codecs if you use > a counter and don't update it in the destructor. But unless my math intuition > fails me, this will not be the case if we log randomly. Are some codecs expected to live shorter than others? https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_encoder.h:139: // Subclasses implements this and return a name, e.g. "Opus" or "iSAC". On 2016/05/11 10:46:21, kwiberg-webrtc wrote: > On 2016/05/11 10:37:20, hlundin-webrtc wrote: > > On 2016/05/11 09:49:43, kwiberg-webrtc wrote: > > > On 2016/05/11 08:59:18, hlundin-webrtc wrote: > > > > On 2016/05/11 08:52:24, kwiberg-webrtc wrote: > > > > > On 2016/05/11 08:27:26, hlundin-webrtc wrote: > > > > > > On 2016/05/11 00:47:00, kwiberg-webrtc wrote: > > > > > > > Despite the example set by EncodeImpl, you probably don't need to > say > > > that > > > > > > > subclasses can implement the method---the "virtual" already says > that. > > > But > > > > > it > > > > > > > would be useful to document the lifetime of the returned string, and > > > what > > > > > the > > > > > > > default implementation does. Maybe > > > > > > > > > > > > > > // Returns a pointer to a statically allocated string with the > codec > > > > name, > > > > > > > // e.g. "Opus" or "iSAC". The default implementation returns null, > > > > > > > // which means no name. > > > > > > > > > > > > I'm not sure we should promise statically allocated strings. We talked > > > about > > > > > > wanting to distinguish between different codec settings, e.g., Opus > with > > > and > > > > > > without DTX enabled. Also, for both iSAC and the PCM16 "codec", the > > sample > > > > > rate > > > > > > may vary. > > > > > > > > > > OK. What lifetime should we promise, then? That of the AudioEncoder > > > instance? > > > > > > > > That seems reasonable, but promising the correct value is another issue. I > > > mean, > > > > _if_ we are going to distinguish between Opus with and without DTX, that > is > > > > going to be a runtime parameter. So, correct string value can only be > > > guaranteed > > > > until the settings change. Can we be conservative and just promise > lifetime > > > > until the next call to any other method (or lifetime, whichever is > shorter)? > > > > > > I think it's problematic if we have an interface that requires the encoder > to > > > generate these strings on the fly. That's costly, and besides means there > are > > a > > > lot of them, which will cause problems for the consumer (remember the limit > of > > > 64 codec names from elsewhere in this CL?). > > > > > > If the number of possible names an encoder can return is small, they can all > > be > > > kept in a table with encoder instance or even static life time. > > > > I too was thinking of a "small set" of statically defined strings. But still, > > the output from GetCodecName will not be statically known, and the > > correctness-time of the string may be shorter than the lifetime of the object. > > But the complete set of strings possibly returned by the encoder needs to be > known by the caller, so it can't be so large that it's infeasible for the > encoder to just store them all in a small table. Acknowledged.
https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_encoder.cc (right): https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_encoder.cc:36: const float kProbabilityToLogCodecType = 1.0 / 500.0; On 2016/05/11 00:47:00, kwiberg-webrtc wrote: > constexpr? Thanks, fixed in next cl https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_encoder.cc:39: // one of the codec ID:s in CodecID On 2016/05/11 00:47:00, kwiberg-webrtc wrote: > "ID:s" -> "IDs". > > What is CodecID? CodecID was HistogramAudioCodecType earlier. I changed to be consistent with https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... Since we are considering to remove the randomness, this may not be relevant any more. https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_encoder.cc:41: if (strcmp("Opus", codec_name) == 0) On 2016/05/11 00:47:00, kwiberg-webrtc wrote: > codec_name can be null, but the behavior of strcmp is undefined if you pass it > anything but pointers to null-terminated strings. Thanks, fixed in next cl. I took the names from the design document. I will rename when you have agreed. https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_encoder.cc:43: else if (strcmp("iSac", codec_name) == 0) On 2016/05/11 00:47:00, kwiberg-webrtc wrote: > iSAC Thanks, fixed in next CL https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_encoder.cc:53: } On 2016/05/11 00:47:00, kwiberg-webrtc wrote: > Hmm. You end up having to basically maintain two copies of the codec list, one > enum and one in the code. Do you think a table-driven approach like this might > work well?: > > static const struct { > const char* name; > int id; > } histogram_id = { > {"Opus", 1}, > ... > }; > for (const auto& codec : histogram_id) { > if (std::strcmp(codec.name, codec_name) == 0) > return codec.id; > } Is there a reason for declaring the 'histogram_id' array 'static const' rather that 'const' or 'constexpr'? The array declaration was a little hard for me to read. Would it be better to move the type definition out like this: struct CodecNameIDPair { const char* name; int id; }; static const const CodecNameIDPair histogram_id[] = { {"Opus", 1}, ... }; What do you think about replacing the struct type with std::pair<const char*, int>? I think it may be shorter and more readable. Since initializer lists can construct pairs, it will not add much syntactical overhead. But it seems that the array 'histogram_id' cannot be made constexpr with the current compiler version and flags. (It works in g++ 4.8.4 -std=c++11) https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_encoder.cc:85: if (rand() / RAND_MAX < kProbabilityToLogCodecType) On 2016/05/11 00:47:00, kwiberg-webrtc wrote: > std::rand > > Also, since std::rand() returns an int, and RAND_MAX is an int, the quotient > will always be 0, so the condition will always be true. Maybe try something like > > constexpr int threshold = RAND_MAX * kProbabilityToLogCodecType; > if (rand() < threshold) I talked with solenberg@ about using randomness. If I understood correctly, there were several drawbacks: 1. rand() has state, and we would like to keep the encoder state-less 2. rand() shares its state across all threads 3. it is currently not given a seed 4. it is a relatively bad random number generator The conclusion was that since we need state anyway, we can as well put a counter in a private member variable. Another matter is that (contrary to what we believed and said earlier) the random trial has quite bad statistic properties. E.g. if you want to be 90% sure that the counter will be more than 10% off, you have to run the encoder for at least 8 days. (Here is the math: https://drive.google.com/a/google.com/file/d/0B0ohpnzd-DTsTkJyZ29OdmpEdVk/vie... ) https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_encoder.h:139: // Subclasses implements this and return a name, e.g. "Opus" or "iSAC". On 2016/05/11 00:47:00, kwiberg-webrtc wrote: > Despite the example set by EncodeImpl, you probably don't need to say that > subclasses can implement the method---the "virtual" already says that. But it > would be useful to document the lifetime of the returned string, and what the > default implementation does. Maybe > > // Returns a pointer to a statically allocated string with the codec name, > // e.g. "Opus" or "iSAC". The default implementation returns null, > // which means no name. Can a C++ string be returned dynamically instead? Then one does not have to worry about lifetime and deallocation. https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc (right): https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc:107: return "g711"; On 2016/05/11 08:27:26, hlundin-webrtc wrote: > Must distinguish between types "A" and "U". Acknowledged. https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.h (right): https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.h:58: const char* GetCodecName() const override; On 2016/05/11 00:47:00, kwiberg-webrtc wrote: > AudioEncoderPcm is also base class for the 16-bit pcm encoder, which IIRC isn't > called g711. You probably need to move this override to the leaf classes. Acknowledged.
solenberg@google.com changed reviewers: + solenberg@google.com
https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_encoder.h:139: // Subclasses implements this and return a name, e.g. "Opus" or "iSAC". On 2016/05/11 11:16:13, aleloi wrote: > On 2016/05/11 00:47:00, kwiberg-webrtc wrote: > > Despite the example set by EncodeImpl, you probably don't need to say that > > subclasses can implement the method---the "virtual" already says that. But it > > would be useful to document the lifetime of the returned string, and what the > > default implementation does. Maybe > > > > // Returns a pointer to a statically allocated string with the codec name, > > // e.g. "Opus" or "iSAC". The default implementation returns null, > > // which means no name. > > Can a C++ string be returned dynamically instead? Then one does not have to > worry about lifetime and deallocation. A string returned by value will likely require heap allocations, which is what we want to avoid here (some string implementations won't use heap allocs for short strings though, which we certainly have here, but it is fragile to depend on std lib implementations).
https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_encoder.cc (right): https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_encoder.cc:53: } On 2016/05/11 11:16:13, aleloi wrote: > On 2016/05/11 00:47:00, kwiberg-webrtc wrote: > > Hmm. You end up having to basically maintain two copies of the codec list, one > > enum and one in the code. Do you think a table-driven approach like this might > > work well?: > > > > static const struct { > > const char* name; > > int id; > > } histogram_id = { > > {"Opus", 1}, > > ... > > }; > > for (const auto& codec : histogram_id) { > > if (std::strcmp(codec.name, codec_name) == 0) > > return codec.id; > > } > > Is there a reason for declaring the 'histogram_id' array 'static const' rather > that 'const' or 'constexpr'? static const means it's a global constant that doesn't get reinitialized every time you enter its scope. I haven't tried it, but I suspect the char pointers can't be constexpr since the value of the pointers may not be known at compile time. But I may be wrong. > The array declaration was a little hard for me to read. Would it be better to > move the type definition out like this: > > struct CodecNameIDPair { > const char* name; > int id; > }; > > static const const CodecNameIDPair histogram_id[] = { > {"Opus", 1}, > ... > }; Well, then you have to name the struct, but sure, you can do that. > What do you think about replacing the struct type with std::pair<const char*, > int>? I think it may be shorter and more readable. Since initializer lists can > construct pairs, it will not add much syntactical overhead. The good thing about using a struct is that the members have meaningful names. So I like this variant less than the other two. https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_encoder.cc:85: if (rand() / RAND_MAX < kProbabilityToLogCodecType) On 2016/05/11 11:16:13, aleloi wrote: > On 2016/05/11 00:47:00, kwiberg-webrtc wrote: > > std::rand > > > > Also, since std::rand() returns an int, and RAND_MAX is an int, the quotient > > will always be 0, so the condition will always be true. Maybe try something > like > > > > constexpr int threshold = RAND_MAX * kProbabilityToLogCodecType; > > if (rand() < threshold) > > I talked with solenberg@ about using randomness. If I understood correctly, > there were several drawbacks: > > 1. rand() has state, and we would like to keep the encoder state-less Not an issue. rand's state is global. > 2. rand() shares its state across all threads Yes, bad for performance reasons. > 3. it is currently not given a seed > 4. it is a relatively bad random number generator Possibly relevant, but easily solvable by using a better PRNG. > The conclusion was that since we need state anyway, we can as well put a counter > in a private member variable. I really don't like to put state in interfaces. Much preferable to put the state outside the class in that case. > Another matter is that (contrary to what we believed and said earlier) the > random trial has quite bad statistic properties. E.g. if you want to be 90% sure > that the counter will be more than 10% off, you have to run the encoder for at > least 8 days. (Here is the math: > https://drive.google.com/a/google.com/file/d/0B0ohpnzd-DTsTkJyZ29OdmpEdVk/vie... > ) Well mathed! So tl;dr is that the expected value has all the properties we want, but the variance is high? However, this is the total time for all users, right? Not per user? Then 8 days isn't necessarily all that much. But what's the story if we want e.g. 99% confidence that we got within 1%? Am I right that no reasonable number of users will save us then? https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_encoder.h:139: // Subclasses implements this and return a name, e.g. "Opus" or "iSAC". On 2016/05/11 11:36:44, The Sun (google.com) wrote: > On 2016/05/11 11:16:13, aleloi wrote: > > On 2016/05/11 00:47:00, kwiberg-webrtc wrote: > > > Despite the example set by EncodeImpl, you probably don't need to say that > > > subclasses can implement the method---the "virtual" already says that. But > it > > > would be useful to document the lifetime of the returned string, and what > the > > > default implementation does. Maybe > > > > > > // Returns a pointer to a statically allocated string with the codec name, > > > // e.g. "Opus" or "iSAC". The default implementation returns null, > > > // which means no name. > > > > Can a C++ string be returned dynamically instead? Then one does not have to > > worry about lifetime and deallocation. > > A string returned by value will likely require heap allocations, which is what > we want to avoid here (some string implementations won't use heap allocs for > short strings though, which we certainly have here, but it is fragile to depend > on std lib implementations). I agree. We definitely want to return a pointer to a string that's no on the heap. And my point is that there should be few enough of them that they can be stored in a small static table, which implies we gain nothing by allowing the lifetime of the strings to be limited.
https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/audio_encoder.cc (right): https://codereview.webrtc.org/1967503002/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/audio_encoder.cc:41: if (strcmp("Opus", codec_name) == 0) On 2016/05/11 11:16:13, aleloi wrote: > On 2016/05/11 00:47:00, kwiberg-webrtc wrote: > > codec_name can be null, but the behavior of strcmp is undefined if you pass it > > anything but pointers to null-terminated strings. > > Thanks, fixed in next cl. I took the names from the design document. I will > rename when you have agreed. Acknowledged. https://codereview.webrtc.org/1967503002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_encoder.cc (right): https://codereview.webrtc.org/1967503002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_encoder.cc:23: constexpr int kAudioMax = 64; The name kAudioMax is a bit too generic. Can you come up with something that describes what it is used for. And, now that we don't have the enum any longer, this constant is not enforced. Can you come up with some way to enforce it, maybe using a static_assert? https://codereview.webrtc.org/1967503002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_encoder.cc:35: for (const auto & codec : histogram_id) I think I'll prefer braces on the for loop. Omitting them on the if statement is fine. https://codereview.webrtc.org/1967503002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_encoder.cc:36: if (codec_name != NULL && strcmp(codec.first, codec_name) == 0) nullptr https://codereview.webrtc.org/1967503002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_encoder.cc:36: if (codec_name != NULL && strcmp(codec.first, codec_name) == 0) nullptr https://codereview.webrtc.org/1967503002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc (right): https://codereview.webrtc.org/1967503002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc:120: return "g711"; g711A, or rather PCMA, per my earlier comments. https://codereview.webrtc.org/1967503002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc:137: return "g711"; g711u, or rather PCMU.
RE static_asserts for histogram boundary https://codereview.webrtc.org/1967503002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_encoder.cc (right): https://codereview.webrtc.org/1967503002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_encoder.cc:23: constexpr int kAudioMax = 64; On 2016/05/11 13:03:55, hlundin-webrtc wrote: > The name kAudioMax is a bit too generic. Can you come up with something that > describes what it is used for. And, now that we don't have the enum any longer, > this constant is not enforced. Can you come up with some way to enforce it, > maybe using a static_assert? I renamed it into kMaxCodecNames. Re static_assert for the histogram boundary: here is a (complex and ugly) solution: // Ensures that every element of passed array har .id field less that // kMaxCodecNames template<int N> constexpr bool idSLessThanMaxCodecNames(const CodecNameIDPair (&array) [N], size_t index) { return (index >= N) || (array[index].id < kMaxCodecNames && idSLessThanMaxCodecNames(array, index + 1)); } static_assert(histogram_id, idSLessThanMaxCodecNames(0), "Cannot guarantee that codec ID does not exceed boundary."); I'm not sure my solution a good idea though. Code has to be refactored for it to work: * the type CodecNameIDPair must be visible to the constexpr function * it is a very complex way (with constexprs and templates) to make sure that every number in a constexpr array is smaller than 64 The first point could possibly be mitigated by making idSLessThanMaxCodecNames into a lambda-function inside of CodecNameToHistogramCodecType, but I'm not sure if it can be constexpr then.
responses to hlundin@'s comments. https://codereview.webrtc.org/1967503002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/audio_encoder.cc (right): https://codereview.webrtc.org/1967503002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_encoder.cc:35: for (const auto & codec : histogram_id) On 2016/05/11 13:03:55, hlundin-webrtc wrote: > I think I'll prefer braces on the for loop. Omitting them on the if statement is > fine. Done. https://codereview.webrtc.org/1967503002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/audio_encoder.cc:36: if (codec_name != NULL && strcmp(codec.first, codec_name) == 0) On 2016/05/11 13:03:55, hlundin-webrtc wrote: > nullptr Done. https://codereview.webrtc.org/1967503002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc (right): https://codereview.webrtc.org/1967503002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc:120: return "g711"; On 2016/05/11 13:03:55, hlundin-webrtc wrote: > g711A, or rather PCMA, per my earlier comments. Is this one of the codecs we are interested in having statistics on? Meaning, should I put "g711A" into the histogram_id table and update histograms.xml in chromium? https://codereview.webrtc.org/1967503002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc:137: return "g711"; On 2016/05/11 13:03:55, hlundin-webrtc wrote: > g711u, or rather PCMU. Same question as for g711A
aleloi@webrtc.org changed reviewers: + ossu@webrtc.org - solenberg@google.com
A few minor comments. https://codereview.webrtc.org/1967503002/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc (right): https://codereview.webrtc.org/1967503002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc:120: return "g711"; On 2016/05/11 14:10:49, aleloi wrote: > On 2016/05/11 13:03:55, hlundin-webrtc wrote: > > g711A, or rather PCMA, per my earlier comments. > > Is this one of the codecs we are interested in having statistics on? Meaning, > should I put "g711A" into the histogram_id table and update histograms.xml in > chromium? Yes. https://codereview.webrtc.org/1967503002/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc:137: return "g711"; On 2016/05/11 14:10:49, aleloi wrote: > On 2016/05/11 13:03:55, hlundin-webrtc wrote: > > g711u, or rather PCMU. > > Same question as for g711A Yes. https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc (right): https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:34: struct CodecNameIDPair { The style guide says you should write CodecNameIdPair (with small 'd'). https://google.github.io/styleguide/cppguide.html#Function_Names " Prefer to capitalize acronyms as single words (i.e. StartRpc(), not StartRPC())." https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:40: // Ensures that every element of passed array har .id field less than har? https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:44: constexpr bool idSLessThanMaxCodecNames(const CodecNameIDPair (&array) [N], Name formatting... https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/acm2/audio_coding_module_impl.h (right): https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.h:301: size_t codec_histogram_bins_log[kMaxAudioCodecNames]; Trailing underscore on member variables. https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.h:302: size_t number_of_consecutive_empty_packets; And here. https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc (right): https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc:125: return "g711A"; Please, educate me. What is the lifetime of a literal constant like this?
Made a new CL according to Karl's and Oskar's suggestions. The logging is moved from AudioEncoder to AudioCodingModuleImpl. The codec type is extracted from EncodedInfoLeaf (which has changed to allow this).
I found your CL! Also made a few comments. :) https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc (right): https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:68: if (codec_name != nullptr && strcmp(codec.name, codec_name) == 0) You should check codec_name before entering the loop. No reason to go through that every pass. https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:289: number_of_consecutive_empty_packets += 1; I'd prefer ++number_of_consecutive_empty_packets. ++ incrementation seems to be the most widely used form in the codebase. https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:291: else { else on the same line as the closing curly seems to be preferred by the style guide. Also by me. :) https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:292: size_t codec_type = So the codec id is looked up based on its name once for each packet? Seems a bit expensive. With just six formats its probably fine but if the list were expanded, maybe a faster lookup would be preferred (like a std::map<string, size_t>, for example). https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/acm2/audio_coding_module_impl.h (right): https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.h:39: static constexpr size_t kMaxAudioCodecNames = 64; Maybe include something about logging in this name, since it's only for logging purposes that this limit is applicable. https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc (right): https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc:125: return "g711A"; On 2016/05/12 12:23:39, hlundin-webrtc wrote: > Please, educate me. What is the lifetime of a literal constant like this? FOREVER! Well, it's static data, so I guess until the binary gets unloaded from memory. I guess this could happen with dynamic libraries but otherwise it's until the end of the program. https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.h (right): https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.h:62: virtual const char* GetCodecName() const; I think this should have a name that refers to logging as well, since that's what it's used for (and the name must match something in a chromium XML file, IIRC).
Minor changes to comply with style guide's naming conventions. https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/acm2/audio_coding_module_impl.h (right): https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.h:301: size_t codec_histogram_bins_log[kMaxAudioCodecNames]; On 2016/05/12 12:23:39, hlundin-webrtc wrote: > Trailing underscore on member variables. Done. https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.h:302: size_t number_of_consecutive_empty_packets; On 2016/05/12 12:23:39, hlundin-webrtc wrote: > And here. Done. https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc (right): https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc:125: return "g711A"; On 2016/05/12 12:23:39, hlundin-webrtc wrote: > Please, educate me. What is the lifetime of a literal constant like this? The same as the lifetime of the whole program according to SO (http://stackoverflow.com/questions/9970295/life-time-of-string-literal-in-c)
https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc (right): https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:37: }; Please document. Alternatively, move this definition to just before the one place where it's used (inside CodecNameToHistogramCodecType). https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:50: } This is a lot of hard-to-read code, and it doesn't even check that ID's are >=1 or that you don't use the same ID twice. I'd argue that it isn't worth it. https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:52: // Translates the name of a codec returned by GetCodecName() into That's no longer where the names come from. https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:55: constexpr CodecNameIDPair histogram_id[] = { static constexpr. Otherwise the compiler might put the table on the stack each time you call this function, I believe. https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:56: {"Opus", 1}, Add a comment before the first entry explaining what 0 means. https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:61: {"iLBC", 6} Use a trailing comma here too. Otherwise, the diff will look ugly when you add new stuff to the end of the table. https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:67: for (const auto & codec : histogram_id) { No space between auto and &. (Did you run git cl format?) https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:68: if (codec_name != nullptr && strcmp(codec.name, codec_name) == 0) Pointers are true iff they're not null, so just if (codec_name && ... works just as well. https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:240: codec_histogram_bins_log(), Don't you need to fill this with zeros? https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:292: size_t codec_type = On 2016/05/12 12:50:14, ossu wrote: > So the codec id is looked up based on its name once for each packet? Seems a bit > expensive. With just six formats its probably fine but if the list were > expanded, maybe a faster lookup would be preferred (like a std::map<string, > size_t>, for example). I'm guessing we'd need quite a lot of entries before std::map outperformed an array... More than we'll probably ever end up using. Keeping the array until such time as we unexpectedly find ourselves wanting to keep statistics for a hundred codecs seems like a wise choice. https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:295: number_of_consecutive_empty_packets = 0; Here you zero number_of_consecutive_empty_packets without having used the value. :-) https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/acm2/audio_coding_module_impl.h (right): https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.h:39: static constexpr size_t kMaxAudioCodecNames = 64; In case some compiler (such as Visual Studio) complains that you need to put this in the .cc file as well, try enum : size_t { kMaxAudioCodecNames = 64 }; instead. https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.h:302: size_t number_of_consecutive_empty_packets; 1. Please document these. (As a rule of thumb, it's much more useful to put comments on data than on code.) 2. Why are these size_t? https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc (right): https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc:105: } There are only three subclasses. It'll be easier to read if you simply implement the method for each of them.
Responses to comments, new CL https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc (right): https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:50: } On 2016/05/12 13:01:51, kwiberg-webrtc wrote: > This is a lot of hard-to-read code, and it doesn't even check that ID's are >=1 > or that you don't use the same ID twice. I'd argue that it isn't worth it. I claim it does check that ID:s are different and >= 1. If it returns 'true' on index 'i', it means that every pair at indices [i, i+1, ... N-1] has id exactly 'index+1' (thus no repetitions) and is less than the number of bins. Since 'index' is at least 0, they must be all at least 1. But I agree that it's probably not worth it. https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:52: // Translates the name of a codec returned by GetCodecName() into On 2016/05/12 13:01:51, kwiberg-webrtc wrote: > That's no longer where the names come from. Acknowledged. https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:55: constexpr CodecNameIDPair histogram_id[] = { On 2016/05/12 13:01:51, kwiberg-webrtc wrote: > static constexpr. Otherwise the compiler might put the table on the stack each > time you call this function, I believe. Done. https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:56: {"Opus", 1}, On 2016/05/12 13:01:51, kwiberg-webrtc wrote: > Add a comment before the first entry explaining what 0 means. Done. https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:61: {"iLBC", 6} On 2016/05/12 13:01:51, kwiberg-webrtc wrote: > Use a trailing comma here too. Otherwise, the diff will look ugly when you add > new stuff to the end of the table. I didn't know that was allowed in C++! Always learning new stuff... https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:67: for (const auto & codec : histogram_id) { On 2016/05/12 13:01:51, kwiberg-webrtc wrote: > No space between auto and &. (Did you run git cl format?) Nope, forgot! https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:68: if (codec_name != nullptr && strcmp(codec.name, codec_name) == 0) On 2016/05/12 12:50:14, ossu wrote: > You should check codec_name before entering the loop. No reason to go through > that every pass. Done. https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:68: if (codec_name != nullptr && strcmp(codec.name, codec_name) == 0) On 2016/05/12 13:01:51, kwiberg-webrtc wrote: > Pointers are true iff they're not null, so just > > if (codec_name && ... > > works just as well. I followed ossu's recommendation and moved the check outside. It is now if (codec_name == nullptr) ... which I think is more readable than "if (!codec_name)" https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:240: codec_histogram_bins_log(), On 2016/05/12 13:01:51, kwiberg-webrtc wrote: > Don't you need to fill this with zeros? It gets filled with zeroes with codec_histogram_bins_log(): http://stackoverflow.com/a/6499281 citation: if T is an array type, then each element is value-initialized; An object whose initializer is an empty set of parentheses, i.e., (), shall be value-initialized. https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:289: number_of_consecutive_empty_packets += 1; On 2016/05/12 12:50:14, ossu wrote: > I'd prefer ++number_of_consecutive_empty_packets. ++ incrementation seems to be > the most widely used form in the codebase. Done. https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:291: else { On 2016/05/12 12:50:14, ossu wrote: > else on the same line as the closing curly seems to be preferred by the style > guide. Also by me. :) Done. https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:292: size_t codec_type = On 2016/05/12 12:50:14, ossu wrote: > So the codec id is looked up based on its name once for each packet? Seems a bit > expensive. With just six formats its probably fine but if the list were > expanded, maybe a faster lookup would be preferred (like a std::map<string, > size_t>, for example). We only do trivial operations in the lookup. A map lookup also does lots of comparisons (which have linear cost in the length of the strings). My gut feeling says that may<string> lookup (which is implemented as a red-black tree, I think) is only faster for double-digit amounts of formats. https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:295: number_of_consecutive_empty_packets = 0; On 2016/05/12 13:01:51, kwiberg-webrtc wrote: > Here you zero number_of_consecutive_empty_packets without having used the value. > :-) Acknowledged. https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/acm2/audio_coding_module_impl.h (right): https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.h:39: static constexpr size_t kMaxAudioCodecNames = 64; On 2016/05/12 12:50:14, ossu wrote: > Maybe include something about logging in this name, since it's only for logging > purposes that this limit is applicable. Added explaining comment and changed name to kMaxLoggedAudioCodecNames https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.h (right): https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.h:62: virtual const char* GetCodecName() const; On 2016/05/12 12:50:14, ossu wrote: > I think this should have a name that refers to logging as well, since that's > what it's used for (and the name must match something in a chromium XML file, > IIRC). The names in WebRTC and histograms.xml do not have to match. Only numbers are sent in the UMA communication between WebRTC and chromium. I define names in the XML to know what the numbers correspond to. I am not sure about this and about what to change to. It's not specifically tailored to logging, the name could in principle be used for something else.
Responses to comments, new CL
On 2016/05/12 13:25:50, aleloi wrote: > Responses to comments, new CL I'll be OOO and removing myself from reviewer's list.
solenberg@webrtc.org changed reviewers: - solenberg@webrtc.org
I removed the string names and added an enum class in AudioEncoder. There are large simplifications.
It's getting there! I added a couple of comments - only one of them is really functionally important. https://codereview.webrtc.org/1967503002/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc (right): https://codereview.webrtc.org/1967503002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:254: number_of_consecutive_empty_packets_; Well, now, this is clearly wrong. If we're not getting any empty packets, we'll be adding zero here each time. Actually, we'll always be adding one less than we'd want to. https://codereview.webrtc.org/1967503002/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/acm2/audio_coding_module_impl.h (right): https://codereview.webrtc.org/1967503002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.h:300: size_t codec_histogram_bins_log_[static_cast<size_t>( This line is very long and makes me sad when I try to read it. I can't think of a better way of doing this, though. Anyone? https://codereview.webrtc.org/1967503002/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://codereview.webrtc.org/1967503002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder.h:28: // Used for UMA logging of codec usage. I think you should mention that they must match whatever mapping is done in this XML file I've been hearing so much about, so people know where to look if they need to add/change these. https://codereview.webrtc.org/1967503002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder.h:41: // numbers. number (remove the s) https://codereview.webrtc.org/1967503002/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t_impl.h (right): https://codereview.webrtc.org/1967503002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t_impl.h:149: info.encoder_type = AudioEncoder::CodecType::kIsac; Did we decide on logging different isac implementations separately, or should we ignore that for now?
Replies to ossu's comments and new CL with small fixes. https://codereview.webrtc.org/1967503002/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc (right): https://codereview.webrtc.org/1967503002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:254: number_of_consecutive_empty_packets_; On 2016/05/12 15:18:46, ossu wrote: > Well, now, this is clearly wrong. If we're not getting any empty packets, we'll > be adding zero here each time. Actually, we'll always be adding one less than > we'd want to. Acknowledged. https://codereview.webrtc.org/1967503002/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://codereview.webrtc.org/1967503002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder.h:28: // Used for UMA logging of codec usage. On 2016/05/12 15:18:46, ossu wrote: > I think you should mention that they must match whatever mapping is done in this > XML file I've been hearing so much about, so people know where to look if they > need to add/change these. Acknowledged. https://codereview.webrtc.org/1967503002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder.h:41: // numbers. On 2016/05/12 15:18:46, ossu wrote: > number (remove the s) Acknowledged. https://codereview.webrtc.org/1967503002/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t_impl.h (right): https://codereview.webrtc.org/1967503002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t_impl.h:149: info.encoder_type = AudioEncoder::CodecType::kIsac; On 2016/05/12 15:18:46, ossu wrote: > Did we decide on logging different isac implementations separately, or should we > ignore that for now? I have not heard anything about differentiating between different isacs.
https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc (right): https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:50: } On 2016/05/12 13:25:46, aleloi wrote: > On 2016/05/12 13:01:51, kwiberg-webrtc wrote: > > This is a lot of hard-to-read code, and it doesn't even check that ID's are > >=1 > > or that you don't use the same ID twice. I'd argue that it isn't worth it. > > I claim it does check that ID:s are different and >= 1. If it returns 'true' on > index 'i', it means that every pair at indices [i, i+1, ... N-1] has id exactly > 'index+1' (thus no repetitions) and is less than the number of bins. Since > 'index' is at least 0, they must be all at least 1. > > But I agree that it's probably not worth it. Oh, I see. I didn't read it carefully, just enough to see that you only checked each entry in isolation, which wouldn't be enough to verify what I had in mind (that the first element was >0, that each element was greater than the one before it, and that the last element was <max). But it is enough to check this stricter property. https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:61: {"iLBC", 6} On 2016/05/12 13:25:47, aleloi wrote: > On 2016/05/12 13:01:51, kwiberg-webrtc wrote: > > Use a trailing comma here too. Otherwise, the diff will look ugly when you add > > new stuff to the end of the table. > > I didn't know that was allowed in C++! Always learning new stuff... I've never actually checked what the standard says, but if all our trybots accept it (which I think they will) you should go ahead and use it IMO, since it's rather useful. https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:68: if (codec_name != nullptr && strcmp(codec.name, codec_name) == 0) On 2016/05/12 13:25:47, aleloi wrote: > On 2016/05/12 13:01:51, kwiberg-webrtc wrote: > > Pointers are true iff they're not null, so just > > > > if (codec_name && ... > > > > works just as well. > > I followed ossu's recommendation and moved the check outside. It is now > > if (codec_name == nullptr) ... > > which I think is more readable than "if (!codec_name)" OK. You're not alone in inexplicably preferring the longer form, so I'll just note that my recommendation still stands and try to not make a scene. :-) https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:240: codec_histogram_bins_log(), On 2016/05/12 13:25:47, aleloi wrote: > On 2016/05/12 13:01:51, kwiberg-webrtc wrote: > > Don't you need to fill this with zeros? > > It gets filled with zeroes with codec_histogram_bins_log(): > > http://stackoverflow.com/a/6499281 > > citation: > if T is an array type, then each element is value-initialized; > An object whose initializer is an empty set of parentheses, i.e., (), shall be > value-initialized. Right. Thanks! https://codereview.webrtc.org/1967503002/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:292: size_t codec_type = On 2016/05/12 13:25:47, aleloi wrote: > On 2016/05/12 12:50:14, ossu wrote: > > So the codec id is looked up based on its name once for each packet? Seems a > bit > > expensive. With just six formats its probably fine but if the list were > > expanded, maybe a faster lookup would be preferred (like a std::map<string, > > size_t>, for example). > > We only do trivial operations in the lookup. A map lookup also does lots of > comparisons (which have linear cost in the length of the strings). My gut > feeling says that may<string> lookup (which is implemented as a red-black tree, > I think) is only faster for double-digit amounts of formats. Yes. std::map will also do a lot of cache-unfriendly pointer chasing. If we do end up with a table with 100+ rather than ~10 entries, std::unordered_map would probably be the data structure of choice. https://codereview.webrtc.org/1967503002/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/acm2/audio_coding_module_impl.h (right): https://codereview.webrtc.org/1967503002/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.h:300: size_t codec_histogram_bins_log_[static_cast<size_t>( On 2016/05/12 15:18:46, ossu wrote: > This line is very long and makes me sad when I try to read it. I can't think of > a better way of doing this, though. Anyone? No, not without moving kMaxLoggedAudioCodecNames out of the enum so that we don't have to cast it. https://codereview.webrtc.org/1967503002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc (right): https://codereview.webrtc.org/1967503002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:42: } // namespace You have a blank line before this function, but not after. It's prettier of you are consistent. https://codereview.webrtc.org/1967503002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/acm2/audio_coding_module_impl.h (right): https://codereview.webrtc.org/1967503002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.h:302: size_t number_of_consecutive_empty_packets_; Why size_t? I would have thought that the counters should be the same size on 32- and 64-bit machines. (I believe I already asked this. Sorry if I missed your reply.) https://codereview.webrtc.org/1967503002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://codereview.webrtc.org/1967503002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder.h:31: // correct values. Does that file list the actual number for each name, or is the number implicit from the order there too? If the former is the case (which I would hope, because it makes mistakes less likely), you should probably use explicit numbers here too. https://codereview.webrtc.org/1967503002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder.h:34: kOther = 0, Maybe kOther = 0, // Codec not specified, and/or not listed in this enum. ? https://codereview.webrtc.org/1967503002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder.h:45: kMaxLoggedAudioCodecNames = 64 Why is this explicitly 64, rather than just implicitly the next integer? Doesn't UMA allow us to increase this number later? https://codereview.webrtc.org/1967503002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.h (right): https://codereview.webrtc.org/1967503002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.h:61: virtual AudioEncoder::CodecType GetCodecType() const; Again, it's probably much simpler to =0 here and just force all three subclasses to define the method. https://codereview.webrtc.org/1967503002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/g722/audio_encoder_g722.cc (right): https://codereview.webrtc.org/1967503002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/g722/audio_encoder_g722.cc:149: info.encoder_type = AudioEncoder::CodecType::kG722; Just CodecType::kG722 ought to be enough here.
Responses to commends & minor fixes. https://codereview.webrtc.org/1967503002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc (right): https://codereview.webrtc.org/1967503002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.cc:42: } // namespace On 2016/05/12 23:30:21, kwiberg-webrtc wrote: > You have a blank line before this function, but not after. It's prettier of you > are consistent. Done. https://codereview.webrtc.org/1967503002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/acm2/audio_coding_module_impl.h (right): https://codereview.webrtc.org/1967503002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.h:302: size_t number_of_consecutive_empty_packets_; On 2016/05/12 23:30:21, kwiberg-webrtc wrote: > Why size_t? I would have thought that the counters should be the same size on > 32- and 64-bit machines. (I believe I already asked this. Sorry if I missed your > reply.) No, it was I who probably missed your question. After reading SO, I have become convinced that changing to 'unsigned int' is more consistent. https://codereview.webrtc.org/1967503002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://codereview.webrtc.org/1967503002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder.h:31: // correct values. On 2016/05/12 23:30:21, kwiberg-webrtc wrote: > Does that file list the actual number for each name, or is the number implicit > from the order there too? If the former is the case (which I would hope, because > it makes mistakes less likely), you should probably use explicit numbers here > too. Here is what the relevant section of the histogram file looks like: <enum name="WebRtcAudioCodecs" type="int"> <int value="0" label="Unknown"/> <int value="1" label="Opus"/> <int value="2" label="iSAC"/> <int value="3" label="pcmA"/> <int value="4" label="pcmU"/> <int value="5" label="g722"/> <int value="6" label="iLBC"/> </enum> So yes, it makes sense to use numbers for every codec. I have changed. https://codereview.webrtc.org/1967503002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder.h:34: kOther = 0, On 2016/05/12 23:30:21, kwiberg-webrtc wrote: > Maybe > > kOther = 0, // Codec not specified, and/or not listed in this enum. > > ? That's a little more clear, I think. Changed. https://codereview.webrtc.org/1967503002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder.h:45: kMaxLoggedAudioCodecNames = 64 On 2016/05/12 23:30:21, kwiberg-webrtc wrote: > Why is this explicitly 64, rather than just implicitly the next integer? Doesn't > UMA allow us to increase this number later? I was inspired by the logging of video codecs, https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... I think it's OK to change later. https://codereview.webrtc.org/1967503002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.h (right): https://codereview.webrtc.org/1967503002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.h:61: virtual AudioEncoder::CodecType GetCodecType() const; On 2016/05/12 23:30:21, kwiberg-webrtc wrote: > Again, it's probably much simpler to =0 here and just force all three subclasses > to define the method. To clarify: you suggest changing this to =0, make pcmA return kPcmA, pcmB return kPcmB and pcm16b return kOther? Why do you think is it much simpler? (It doesn't matter for me, but I'd like to follow your reasoning). https://codereview.webrtc.org/1967503002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/g722/audio_encoder_g722.cc (right): https://codereview.webrtc.org/1967503002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/g722/audio_encoder_g722.cc:149: info.encoder_type = AudioEncoder::CodecType::kG722; On 2016/05/12 23:30:21, kwiberg-webrtc wrote: > Just CodecType::kG722 ought to be enough here. Yes, since EncodeImpl has AudioEncoderG722 and therefore AudioEncoder scope already. Done, thanks!
Just one little, tiny comment. Other than that, I'm satisfied with the CL. lgtm https://codereview.webrtc.org/1967503002/diff/140001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://codereview.webrtc.org/1967503002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder.h:44: kMaxLoggedAudioCodecNames = 64 I'm gonna be picky and say it should be kMaxLoggedAudioCodecTypes, since the enum is CodecType.
LGTM with a few nits. https://codereview.webrtc.org/1967503002/diff/140001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://codereview.webrtc.org/1967503002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder.h:33: kOther = 0, // Codec not specifiec, and/or not listed in this enum specifiec -> specified https://codereview.webrtc.org/1967503002/diff/140001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/g722/audio_encoder_g722.cc (right): https://codereview.webrtc.org/1967503002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/g722/audio_encoder_g722.cc:17: #include "webrtc/modules/audio_coding/codecs/audio_encoder.h" You don't have to include audio_encoder.h. From the style guide: "any includes present in the related header do not need to be included again in the related cc." (https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes). https://codereview.webrtc.org/1967503002/diff/140001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/ilbc/audio_encoder_ilbc.cc (right): https://codereview.webrtc.org/1967503002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/ilbc/audio_encoder_ilbc.cc:18: #include "webrtc/modules/audio_coding/codecs/audio_encoder.h" Not needed. https://codereview.webrtc.org/1967503002/diff/140001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t_impl.h (right): https://codereview.webrtc.org/1967503002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t_impl.h:18: #include "webrtc/modules/audio_coding/codecs/audio_encoder.h" Not needed. https://codereview.webrtc.org/1967503002/diff/140001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/1967503002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:19: #include "webrtc/modules/audio_coding/codecs/audio_encoder.h" Not needed.
Very small changes from ossu's and hlundin's comments. Waiting for kwiberg to comment. https://codereview.webrtc.org/1967503002/diff/140001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/g722/audio_encoder_g722.cc (right): https://codereview.webrtc.org/1967503002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/g722/audio_encoder_g722.cc:17: #include "webrtc/modules/audio_coding/codecs/audio_encoder.h" On 2016/05/13 09:54:23, hlundin-webrtc wrote: > You don't have to include audio_encoder.h. From the style guide: "any includes > present in the related header do not need to be included again in the related > cc." > (https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes). Done. https://codereview.webrtc.org/1967503002/diff/140001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/ilbc/audio_encoder_ilbc.cc (right): https://codereview.webrtc.org/1967503002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/ilbc/audio_encoder_ilbc.cc:18: #include "webrtc/modules/audio_coding/codecs/audio_encoder.h" On 2016/05/13 09:54:23, hlundin-webrtc wrote: > Not needed. Done.
https://codereview.webrtc.org/1967503002/diff/140001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://codereview.webrtc.org/1967503002/diff/140001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder.h:44: kMaxLoggedAudioCodecNames = 64 On 2016/05/13 09:14:30, ossu wrote: > I'm gonna be picky and say it should be kMaxLoggedAudioCodecTypes, since the > enum is CodecType. Done.
https://codereview.webrtc.org/1967503002/diff/160001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t_impl.h (right): https://codereview.webrtc.org/1967503002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t_impl.h:18: #include "webrtc/modules/audio_coding/codecs/audio_encoder.h" Still not needed... or?
holte@chromium.org changed reviewers: + holte@chromium.org
https://codereview.webrtc.org/1967503002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://codereview.webrtc.org/1967503002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder.h:45: kMaxLoggedAudioCodecNames = 64 On 2016/05/13 09:02:27, aleloi wrote: > On 2016/05/12 23:30:21, kwiberg-webrtc wrote: > > Why is this explicitly 64, rather than just implicitly the next integer? > Doesn't > > UMA allow us to increase this number later? > > I was inspired by the logging of video codecs, > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... > I think it's OK to change later. > This does seem unnecessary and wastes a tiny bit of memory, since we will allocate memory to hold 64 counters. Not horrible, but still odd. Not sure why video codecs are logged that way either.
Getting close now... https://codereview.webrtc.org/1967503002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/acm2/audio_coding_module_impl.h (right): https://codereview.webrtc.org/1967503002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.h:302: size_t number_of_consecutive_empty_packets_; On 2016/05/13 09:02:27, aleloi wrote: > On 2016/05/12 23:30:21, kwiberg-webrtc wrote: > > Why size_t? I would have thought that the counters should be the same size on > > 32- and 64-bit machines. (I believe I already asked this. Sorry if I missed > your > > reply.) > > No, it was I who probably missed your question. After reading SO, I have become > convinced that changing to 'unsigned int' is more consistent. You still haven't changed this. Also, use int, not unsigned int. See https://google.github.io/styleguide/cppguide.html#Integer_Types https://codereview.webrtc.org/1967503002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://codereview.webrtc.org/1967503002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder.h:45: kMaxLoggedAudioCodecNames = 64 On 2016/05/13 21:20:58, Steven Holte wrote: > On 2016/05/13 09:02:27, aleloi wrote: > > On 2016/05/12 23:30:21, kwiberg-webrtc wrote: > > > Why is this explicitly 64, rather than just implicitly the next integer? > > Doesn't > > > UMA allow us to increase this number later? > > > > I was inspired by the logging of video codecs, > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... > > I think it's OK to change later. > > > > This does seem unnecessary and wastes a tiny bit of memory, since we will > allocate memory to hold 64 counters. Not horrible, but still odd. Not sure why > video codecs are logged that way either. OK. Alex, would you check if we are allowed to change this value later, and drop the =64 if so? https://codereview.webrtc.org/1967503002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.h (right): https://codereview.webrtc.org/1967503002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.h:61: virtual AudioEncoder::CodecType GetCodecType() const; On 2016/05/13 09:02:27, aleloi wrote: > On 2016/05/12 23:30:21, kwiberg-webrtc wrote: > > Again, it's probably much simpler to =0 here and just force all three > subclasses > > to define the method. > > To clarify: you suggest changing this to =0, make pcmA return kPcmA, pcmB return > kPcmB and pcm16b return kOther? Exactly. > Why do you think is it much simpler? (It doesn't matter for me, but I'd like to > follow your reasoning). Because then the value used by each class is specified with that class. It means you'll have to look in fewer places when reading the code. Also because overriding an =0 method is a simpler concept than overriding a method with a default value; instead of "each codec has to say what value it wants", you get "each codec may optionally say what value it wants, and if it doesn't, it gets this default value instead". IOW, I'm lazy, and I don't want to have to think more than necessary when reading code. (This is a surprisingly useful design principle.) https://codereview.webrtc.org/1967503002/diff/160001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://codereview.webrtc.org/1967503002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder.h:29: // order must be listed in "The same codecs, with the same values, must be..."
I fixed hlundin's and kwiberg's last comments. kMaxLoggedAudioCodecTypes is just as large as it needs to be, the PCM codec types are simplified, an int is used for the histogram bins and 2 unnecessary header inclusions are removed (as per style guide, pointed out by hlundin). https://codereview.webrtc.org/1967503002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/acm2/audio_coding_module_impl.h (right): https://codereview.webrtc.org/1967503002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/acm2/audio_coding_module_impl.h:302: size_t number_of_consecutive_empty_packets_; On 2016/05/15 02:06:13, kwiberg-webrtc wrote: > On 2016/05/13 09:02:27, aleloi wrote: > > On 2016/05/12 23:30:21, kwiberg-webrtc wrote: > > > Why size_t? I would have thought that the counters should be the same size > on > > > 32- and 64-bit machines. (I believe I already asked this. Sorry if I missed > > your > > > reply.) > > > > No, it was I who probably missed your question. After reading SO, I have > become > > convinced that changing to 'unsigned int' is more consistent. > > You still haven't changed this. > > Also, use int, not unsigned int. See > https://google.github.io/styleguide/cppguide.html#Integer_Types Done. https://codereview.webrtc.org/1967503002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://codereview.webrtc.org/1967503002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder.h:45: kMaxLoggedAudioCodecNames = 64 On 2016/05/15 02:06:13, kwiberg-webrtc wrote: > On 2016/05/13 21:20:58, Steven Holte wrote: > > On 2016/05/13 09:02:27, aleloi wrote: > > > On 2016/05/12 23:30:21, kwiberg-webrtc wrote: > > > > Why is this explicitly 64, rather than just implicitly the next integer? > > > Doesn't > > > > UMA allow us to increase this number later? > > > > > > I was inspired by the logging of video codecs, > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc... > > > I think it's OK to change later. > > > > > > > This does seem unnecessary and wastes a tiny bit of memory, since we will > > allocate memory to hold 64 counters. Not horrible, but still odd. Not sure > why > > video codecs are logged that way either. > > OK. Alex, would you check if we are allowed to change this value later, and drop > the =64 if so? Done. https://codereview.webrtc.org/1967503002/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.h (right): https://codereview.webrtc.org/1967503002/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.h:61: virtual AudioEncoder::CodecType GetCodecType() const; On 2016/05/15 02:06:13, kwiberg-webrtc wrote: > On 2016/05/13 09:02:27, aleloi wrote: > > On 2016/05/12 23:30:21, kwiberg-webrtc wrote: > > > Again, it's probably much simpler to =0 here and just force all three > > subclasses > > > to define the method. > > > > To clarify: you suggest changing this to =0, make pcmA return kPcmA, pcmB > return > > kPcmB and pcm16b return kOther? > > Exactly. > > > Why do you think is it much simpler? (It doesn't matter for me, but I'd like > to > > follow your reasoning). > > Because then the value used by each class is specified with that class. It means > you'll have to look in fewer places when reading the code. > > Also because overriding an =0 method is a simpler concept than overriding a > method with a default value; instead of "each codec has to say what value it > wants", you get "each codec may optionally say what value it wants, and if it > doesn't, it gets this default value instead". > > IOW, I'm lazy, and I don't want to have to think more than necessary when > reading code. (This is a surprisingly useful design principle.) Done. https://codereview.webrtc.org/1967503002/diff/160001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://codereview.webrtc.org/1967503002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder.h:29: // order must be listed in On 2016/05/15 02:06:14, kwiberg-webrtc wrote: > "The same codecs, with the same values, must be..." Done. https://codereview.webrtc.org/1967503002/diff/160001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t_impl.h (right): https://codereview.webrtc.org/1967503002/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t_impl.h:18: #include "webrtc/modules/audio_coding/codecs/audio_encoder.h" On 2016/05/13 11:18:19, hlundin-webrtc wrote: > Still not needed... or? No, missed that one.
lgtm, with some minor comments Good job! https://codereview.webrtc.org/1967503002/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.h (right): https://codereview.webrtc.org/1967503002/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.h:60: // in AudioEncoderPcm::EncodeImpl There is no longer a default implementation. Just removing the first sentence ought to work. https://codereview.webrtc.org/1967503002/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/ilbc/audio_encoder_ilbc.cc (right): https://codereview.webrtc.org/1967503002/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/ilbc/audio_encoder_ilbc.cc:130: info.encoder_type = AudioEncoder::CodecType::kIlbc; Nit: Just CodecType::kIlbc should be enough. https://codereview.webrtc.org/1967503002/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t_impl.h (right): https://codereview.webrtc.org/1967503002/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t_impl.h:148: info.encoder_type = AudioEncoder::CodecType::kIsac; And here. https://codereview.webrtc.org/1967503002/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/1967503002/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:215: info.encoder_type = AudioEncoder::CodecType::kOpus; And here. https://codereview.webrtc.org/1967503002/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/pcm16b/audio_encoder_pcm16b.cc (right): https://codereview.webrtc.org/1967503002/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/pcm16b/audio_encoder_pcm16b.cc:30: return AudioEncoder::CodecType::kOther; And here.
Removed unnecessary AudioEncoder:: scoping operator. https://codereview.webrtc.org/1967503002/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.h (right): https://codereview.webrtc.org/1967503002/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.h:60: // in AudioEncoderPcm::EncodeImpl On 2016/05/16 08:13:26, kwiberg-webrtc wrote: > There is no longer a default implementation. Just removing the first sentence > ought to work. Done. https://codereview.webrtc.org/1967503002/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/ilbc/audio_encoder_ilbc.cc (right): https://codereview.webrtc.org/1967503002/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/ilbc/audio_encoder_ilbc.cc:130: info.encoder_type = AudioEncoder::CodecType::kIlbc; On 2016/05/16 08:13:26, kwiberg-webrtc wrote: > Nit: Just CodecType::kIlbc should be enough. Done. https://codereview.webrtc.org/1967503002/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t_impl.h (right): https://codereview.webrtc.org/1967503002/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t_impl.h:148: info.encoder_type = AudioEncoder::CodecType::kIsac; On 2016/05/16 08:13:26, kwiberg-webrtc wrote: > And here. Done. https://codereview.webrtc.org/1967503002/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/1967503002/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:215: info.encoder_type = AudioEncoder::CodecType::kOpus; On 2016/05/16 08:13:26, kwiberg-webrtc wrote: > And here. Done. https://codereview.webrtc.org/1967503002/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/pcm16b/audio_encoder_pcm16b.cc (right): https://codereview.webrtc.org/1967503002/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/pcm16b/audio_encoder_pcm16b.cc:30: return AudioEncoder::CodecType::kOther; On 2016/05/16 08:13:26, kwiberg-webrtc wrote: > And here. Done.
The CQ bit was checked by aleloi@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org, ossu@webrtc.org, kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/1967503002/#ps200001 (title: "Removed unnecessary scoping operators")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1967503002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1967503002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_x64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/14719)
The CQ bit was checked by aleloi@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org, henrik.lundin@webrtc.org, ossu@webrtc.org Link to the patchset: https://codereview.webrtc.org/1967503002/#ps220001 (title: "Removed unneeded 'virtual'")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1967503002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1967503002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_x64_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_gn_rel/builds/9263)
The CQ bit was checked by aleloi@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org, henrik.lundin@webrtc.org, ossu@webrtc.org Link to the patchset: https://codereview.webrtc.org/1967503002/#ps240001 (title: "int conversion")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1967503002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1967503002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_x64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/14722)
The CQ bit was checked by aleloi@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org, henrik.lundin@webrtc.org, ossu@webrtc.org Link to the patchset: https://codereview.webrtc.org/1967503002/#ps260001 (title: "explicit casts")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1967503002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1967503002/260001
The CQ bit was unchecked by aleloi@webrtc.org
The CQ bit was checked by aleloi@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1967503002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1967503002/260001
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 aleloi@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1967503002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1967503002/260001
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Added UMA logging for audio codec usage. A histogram statistic named "WebRTC.Audio.Encoder.CodecType" has been created. ========== to ========== Added UMA logging for audio codec usage. A histogram statistic named "WebRTC.Audio.Encoder.CodecType" has been created. Committed: https://crrev.com/8bce67b7451d2d75390410dd7bbc82078ccfb2c9 Cr-Commit-Position: refs/heads/master@{#12760} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/8bce67b7451d2d75390410dd7bbc82078ccfb2c9 Cr-Commit-Position: refs/heads/master@{#12760}
Message was sent while issue was closed.
Hurray! Thanks for all the comments :)
Message was sent while issue was closed.
lgtm |
