Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2695243005:
Injectable audio encoders: BuiltinAudioEncoderFactory (Closed)
|
Created:
3 years, 10 months ago by ossu Modified:
3 years, 8 months ago CC:
webrtc-reviews_webrtc.org, peah-webrtc, Andrew MacDonald, henrika_webrtc, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc, minyue-webrtc, the sun, lliuu Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionInjectable audio encoders: BuiltinAudioEncoderFactory
This CL contains all the changes made to audio_coding while making
audio encoders injectable. Apart from some small changes to
webrtcvoiceengine, nothing here is hooked up to the outside
world. Those changes will be added to a follow-up CL.
BUG=webrtc:5806
Review-Url: https://codereview.webrtc.org/2695243005
Cr-Commit-Position: refs/heads/master@{#17569}
Committed: https://chromium.googlesource.com/external/webrtc/+/a1a040a4a492c14af068811c6e853b03f1e70b9e
Patch Set 1 #
Total comments: 27
Patch Set 2 : Rebase #Patch Set 3 : Removed stub versions of AudioEncoderFactory from peerconnectioninterface.h #
Total comments: 1
Patch Set 4 : Rebase #Patch Set 5 : Cleaned up parameter parsing in AudioCodecOpus #
Total comments: 119
Patch Set 6 : Wrote tests for BuiltinAudioEncoderFactory; addressed many comments. #
Total comments: 6
Patch Set 7 : Moved ANA frame length calculation into its own function. Improved "ptime" parsing in non-opus codeā¦ #
Total comments: 20
Patch Set 8 : Cleaned up Opus code a bit. kHz -> Hz in comment picked a bunch of lint #
Total comments: 55
Patch Set 9 : Rebase #Patch Set 10 : Fixed test, made AudioEncoderOpus treat stereo parameter more leniently, formatting and lints. #
Total comments: 2
Patch Set 11 : num_channels: int -> size_t #Patch Set 12 : Rebase #Patch Set 13 : Fix build problems on Windows, Android and downstream. #Dependent Patchsets: Messages
Total messages: 66 (33 generated)
ossu@webrtc.org changed reviewers: + solenberg@webrtc.org
Here is the audio encoder factory and the changes it requires to the encoders, etc. Apart from a fair number of changes to webrtcvoiceengine.cc, this makes up the bulk of the code for injectable audio encoders. I'll follow up with a CL of those changes (but not tonight).
kwiberg@webrtc.org changed reviewers: + kwiberg@webrtc.org
https://codereview.webrtc.org/2695243005/diff/1/webrtc/api/audio_codecs/audio... File webrtc/api/audio_codecs/audio_format.cc (right): https://codereview.webrtc.org/2695243005/diff/1/webrtc/api/audio_codecs/audio... webrtc/api/audio_codecs/audio_format.cc:84: max_bitrate_bps(0) {} Also initialize default_bitrate_bps? In general, I prefer to make all but one of the constructors call the last constructor if possible, precisely to avoid this sort of thing. https://codereview.webrtc.org/2695243005/diff/1/webrtc/api/audio_codecs/audio... File webrtc/api/audio_codecs/audio_format.h (right): https://codereview.webrtc.org/2695243005/diff/1/webrtc/api/audio_codecs/audio... webrtc/api/audio_codecs/audio_format.h:115: } Why not the usual !(a==b) ? https://codereview.webrtc.org/2695243005/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/acm2/audio_coding_module_unittest.cc (right): https://codereview.webrtc.org/2695243005/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/acm2/audio_coding_module_unittest.cc:1566: int payload_type) { clang-format https://codereview.webrtc.org/2695243005/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc (right): https://codereview.webrtc.org/2695243005/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc:50: } To reduce the scope, this could be a private member of NamedEncoderFactory. Or even better, a lambda in ForEncoder. https://codereview.webrtc.org/2695243005/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc:94: // TODO(ossu): Make this a one-time initialization, preferable static. Yes. https://codereview.webrtc.org/2695243005/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc:144: } Maybe I'm just confused, but why are you looking in encoder_factories AND supported_encoders_? https://codereview.webrtc.org/2695243005/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc (right): https://codereview.webrtc.org/2695243005/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc:56: if (config.IsOk()) { if (CreateConfig<T>(0, format).IsOk()) { And then you can merge it into the previous if (...). https://codereview.webrtc.org/2695243005/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.h (right): https://codereview.webrtc.org/2695243005/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.h:84: explicit AudioEncoderPcmA(int payload_type, const SdpAudioFormat& format); Why explicit? https://codereview.webrtc.org/2695243005/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2695243005/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:134: format.clockrate_hz == 48000 && format.num_channels == 2) { Invert this test and return early, to save lots of indentation. https://codereview.webrtc.org/2695243005/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:160: if (max_playback_rate && max_playback_rate <= 8000) { max_playback_rate is an int, isn't it? Is 0 special? https://codereview.webrtc.org/2695243005/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:274: } This function is very long. It has several parts that could be separate functions, I believe. (As a bonus, writing unit tests will become much easier!) https://codereview.webrtc.org/2695243005/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:379: : AudioEncoderOpus(webrtc::CreateConfig(codec_inst), nullptr) {} Wait... webrtc::CreateConfig? https://codereview.webrtc.org/2695243005/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc (right): https://codereview.webrtc.org/2695243005/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:676: EXPECT_EQ(12000, *config.bitrate_bps); You can EXPECT Optional values like this: EXPECT_EQ(rtc::Optional<int>(12000), config.bitrate_bps);
https://codereview.webrtc.org/2695243005/diff/1/webrtc/api/audio_codecs/audio... File webrtc/api/audio_codecs/audio_format.cc (right): https://codereview.webrtc.org/2695243005/diff/1/webrtc/api/audio_codecs/audio... webrtc/api/audio_codecs/audio_format.cc:84: max_bitrate_bps(0) {} On 2017/02/19 21:41:09, kwiberg-webrtc wrote: > Also initialize default_bitrate_bps? > > In general, I prefer to make all but one of the constructors call the last > constructor if possible, precisely to avoid this sort of thing. Uh-oh, will fix! https://codereview.webrtc.org/2695243005/diff/1/webrtc/api/audio_codecs/audio... File webrtc/api/audio_codecs/audio_format.h (right): https://codereview.webrtc.org/2695243005/diff/1/webrtc/api/audio_codecs/audio... webrtc/api/audio_codecs/audio_format.h:115: } On 2017/02/19 21:41:10, kwiberg-webrtc wrote: > Why not the usual !(a==b) ? Great question! Brain-fart? :) https://codereview.webrtc.org/2695243005/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/acm2/audio_coding_module_unittest.cc (right): https://codereview.webrtc.org/2695243005/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/acm2/audio_coding_module_unittest.cc:1566: int payload_type) { On 2017/02/19 21:41:10, kwiberg-webrtc wrote: > clang-format Alright. https://codereview.webrtc.org/2695243005/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc (right): https://codereview.webrtc.org/2695243005/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc:50: } On 2017/02/19 21:41:10, kwiberg-webrtc wrote: > To reduce the scope, this could be a private member of NamedEncoderFactory. Or > even better, a lambda in ForEncoder. True. Will do. https://codereview.webrtc.org/2695243005/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc:144: } On 2017/02/19 21:41:10, kwiberg-webrtc wrote: > Maybe I'm just confused, but why are you looking in encoder_factories AND > supported_encoders_? Hmm... either I forgot to remove my temporary implementation (the second block), or I made a mistake while pulling out just these changes using git. Either way, the second block should go. https://codereview.webrtc.org/2695243005/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc (right): https://codereview.webrtc.org/2695243005/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc:56: if (config.IsOk()) { On 2017/02/19 21:41:10, kwiberg-webrtc wrote: > if (CreateConfig<T>(0, format).IsOk()) { > > And then you can merge it into the previous if (...). Alright. https://codereview.webrtc.org/2695243005/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.h (right): https://codereview.webrtc.org/2695243005/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.h:84: explicit AudioEncoderPcmA(int payload_type, const SdpAudioFormat& format); On 2017/02/19 21:41:10, kwiberg-webrtc wrote: > Why explicit? Copied the line above, made changes, didn't think about it. https://codereview.webrtc.org/2695243005/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2695243005/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:160: if (max_playback_rate && max_playback_rate <= 8000) { On 2017/02/19 21:41:10, kwiberg-webrtc wrote: > max_playback_rate is an int, isn't it? Is 0 special? I'll look through this whole function. It duplicates some of what CreatConfig does - it would be nice to DRY that up. https://codereview.webrtc.org/2695243005/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:274: } On 2017/02/19 21:41:10, kwiberg-webrtc wrote: > This function is very long. It has several parts that could be separate > functions, I believe. (As a bonus, writing unit tests will become much easier!) Agreed. https://codereview.webrtc.org/2695243005/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:379: : AudioEncoderOpus(webrtc::CreateConfig(codec_inst), nullptr) {} On 2017/02/19 21:41:10, kwiberg-webrtc wrote: > Wait... webrtc::CreateConfig? When I added a CreateConfig for my case, it needed to live in AudioEncoderOpus, but the old one didn't. So just CreateConfig was confusing the compiler. I'll tidy this up, so they're both under AudioEncoderOpus. At least one of them, I'd like to keep public as well, since that would help other users set up their own factories including Opus, without having to duplicate all the logic in CreateConfig. https://codereview.webrtc.org/2695243005/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc (right): https://codereview.webrtc.org/2695243005/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:676: EXPECT_EQ(12000, *config.bitrate_bps); On 2017/02/19 21:41:10, kwiberg-webrtc wrote: > You can EXPECT Optional values like this: > > EXPECT_EQ(rtc::Optional<int>(12000), config.bitrate_bps); Yeah, that's probably better, all things considered.
I haven't addressed any of the comments in this change, but pushed the peerconnection changes from later CLs here. Otherwise, we'd have two definitions of AudioEncoderFactory in the tree, which is weird. It's only a few includes added and requisite deps in BUILD.gn and DEPS files.
Description was changed from ========== Injectable audio encoders: BuiltinAudioEncoderFactory This CL contains all the changes made to audio_coding while making audio encoders injectable. Apart from some small changes to webrtcvoiceengine, nothing here is hooked up to the outside world. Those changes will be added to a follow-up CL. BUG=webrtc:5806 ========== to ========== Injectable audio encoders: BuiltinAudioEncoderFactory This CL contains all the changes made to audio_coding while making audio encoders injectable. Apart from some small changes to webrtcvoiceengine, nothing here is hooked up to the outside world. Those changes will be added to a follow-up CL. BUG=webrtc:5806 ==========
Patchset #4 (id:60001) has been deleted
I believe this latest patch set addresses all comments on this CL. The only thing left to consider is if AudioFormatInfo should still be its own thing, or if that information can be put back into AudioCodecSpec (as mentioned in another of these CLs). I've some work done on that already, and will put up a second patch set with those changes. Please don't wait to review the rest of this, though. :) https://codereview.webrtc.org/2695243005/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2695243005/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:134: format.clockrate_hz == 48000 && format.num_channels == 2) { On 2017/02/19 21:41:10, kwiberg-webrtc wrote: > Invert this test and return early, to save lots of indentation. I didn't do this, but I simplified the function enough that I've now only one failure return, and it's hit if any of the two nested ifs fail. https://codereview.webrtc.org/2695243005/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:274: } On 2017/02/20 12:20:26, ossu wrote: > On 2017/02/19 21:41:10, kwiberg-webrtc wrote: > > This function is very long. It has several parts that could be separate > > functions, I believe. (As a bonus, writing unit tests will become much > easier!) > > Agreed. Moved common functionality from QueryAudioFormat and CreateConfig into separate functions, making everything easier to follow. The only big lump still in CreateConfig is the stuff pertaining to frame lengths. I don't know the specifics well enough, but I believe this could be cleaned up a bit, especially if we get info about ANA during creation. Maybe this can wait until after this set of CLs, though. https://codereview.webrtc.org/2695243005/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc (right): https://codereview.webrtc.org/2695243005/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:676: EXPECT_EQ(12000, *config.bitrate_bps); On 2017/02/20 12:20:26, ossu wrote: > On 2017/02/19 21:41:10, kwiberg-webrtc wrote: > > You can EXPECT Optional values like this: > > > > EXPECT_EQ(rtc::Optional<int>(12000), config.bitrate_bps); > > Yeah, that's probably better, all things considered. With the changes to Optional, it's now possible to just expect without * and still get usable messages. https://codereview.webrtc.org/2695243005/diff/40001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/acm2/audio_coding_module_unittest.cc (right): https://codereview.webrtc.org/2695243005/diff/40001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/acm2/audio_coding_module_unittest.cc:1495: static const SdpAudioFormat kOpusFormat("opus", 48000, 2, {{"stereo", "1"}}); Replicate the tests for Opus, but using an SdpAudioFormat, so that we ensure the output is consistent. Should I perhaps add similar versions of the other format tests? Opus is the one with the most number of parameters that can go wrong, but the others will eventually need changing over to the new API. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:53: const int kOpusMaxBitrateBps = 510000; I removed the old kMin/MaxBitrateBps and replaced with these, that originally come from webrtcvoiceengine.cc. This means I also changed the max to 510 kbps, instead of 512 kbps. From a quick web search, I can only find references to 510 kbps as being the max for Opus.
ossu@webrtc.org changed reviewers: + henrik.lundin@webrtc.org
Adding you, hlundin, to possibly take a quick look on the changes to Opus max bitrate from 512 -> 510 kbps. I'm hoping you've got the insight, as minuye is OOO for the rest of this week.
Great stuff, almost there! https://codereview.webrtc.org/2695243005/diff/100001/webrtc/api/audio_codecs/... File webrtc/api/audio_codecs/audio_format.h (right): https://codereview.webrtc.org/2695243005/diff/100001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_format.h:77: AudioFormatInfo(const AudioFormatInfo& b) = default; No need for default move ctor? I forget when that is added automatically... https://codereview.webrtc.org/2695243005/diff/100001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_format.h:94: bool HasFixedBitrate() const { return min_bitrate_bps == max_bitrate_bps; } In this case, should there be a DCHECK that default_bitrate_bps is the same as well? https://codereview.webrtc.org/2695243005/diff/100001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_format.h:102: bool allow_comfort_noise = true; // This codec can be used with an external Since you mention "codec" here - should this struct be called "AudioCodecInfo" instead? That makes a lot of sense to me - AudioCodecSpec ties together the codec-specific info with the SDP format. WDYT? https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/audio_encoder_factory.h (right): https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder_factory.h:31: virtual rtc::Optional<AudioFormatInfo> QueryAudioFormat( May be good to add "Encoder" to this method name - decoder/encoder factory will sometimes be implemented on the same object. I guess a way to get by with just "IsSupportedEncoder()" would be to let that return an rtc::Optional<AudioCodecInfo>? https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc (right): https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc:20: #ifdef WEBRTC_CODEC_G722 Can you file an issue to deprecate these compiler flags? https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc:95: return !!QueryAudioFormat(format); Ah, haha. What about just having one method then? https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc:156: I'm missing a unit test for this class. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.h (right): https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.h:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. Time flies... https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:51: // bitrate should be in the range between 6000 and 510000. nit: "between" excludes 6000 and 510000 - but below (line 149) you include them
https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:51: // bitrate should be in the range between 6000 and 510000. On 2017/03/14 20:48:29, the sun wrote: > nit: "between" excludes 6000 and 510000 - but below (line 149) you include them Well, I'd say it *sometimes* excludes the endpoints, but that there's no hard rule---which is at least as bad, of course. I would say "in the range [6000, 510000]" or maybe "between 6000 and 510000, inclusive". The mathematical notation has the advantage of being both short and precise, and is probably well-known enough.
Just some answers. No new patch set. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/api/audio_codecs/... File webrtc/api/audio_codecs/audio_format.h (right): https://codereview.webrtc.org/2695243005/diff/100001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_format.h:77: AudioFormatInfo(const AudioFormatInfo& b) = default; On 2017/03/14 20:48:28, the sun wrote: > No need for default move ctor? I forget when that is added automatically... There's nothing that can be moved, so there's no need for a move constructor. It's all just fundamental types that will be copied. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_format.h:94: bool HasFixedBitrate() const { return min_bitrate_bps == max_bitrate_bps; } On 2017/03/14 20:48:29, the sun wrote: > In this case, should there be a DCHECK that default_bitrate_bps is the same as > well? There's a check in the constructor, but we might as well have one here as well. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_format.h:102: bool allow_comfort_noise = true; // This codec can be used with an external On 2017/03/14 20:48:29, the sun wrote: > Since you mention "codec" here - should this struct be called "AudioCodecInfo" > instead? That makes a lot of sense to me - AudioCodecSpec ties together the > codec-specific info with the SDP format. WDYT? Yeah, that's probably better. I found AudioCodecSpec and AudioCodecInfo to be a bit too similar - what's a spec vs. the info - but maybe that's good? I'll take another look at folding AudioCodecInfo back into AudioCodecSpec and we can see how it looks first. I think having them separated like this might still be beneficial. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/audio_encoder_factory.h (right): https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder_factory.h:31: virtual rtc::Optional<AudioFormatInfo> QueryAudioFormat( On 2017/03/14 20:48:29, the sun wrote: > May be good to add "Encoder" to this method name - decoder/encoder factory will > sometimes be implemented on the same object. > > I guess a way to get by with just "IsSupportedEncoder()" would be to let that > return an rtc::Optional<AudioCodecInfo>? The decoder has no QueryAudioFormat method as it stands today, but maybe it should have in future. I think QueryAudioEncoder and renaming to AudioCodecInfo makes a lot of sense. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc (right): https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc:20: #ifdef WEBRTC_CODEC_G722 On 2017/03/14 20:48:29, the sun wrote: > Can you file an issue to deprecate these compiler flags? How do we want this to work in future? Should our builtin one always include all of these? For those that want to pick and choose, they can create their own, but maybe some are target dependent (like ISAC/ISAC_FIX) https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc:95: return !!QueryAudioFormat(format); On 2017/03/14 20:48:29, the sun wrote: > Ah, haha. What about just having one method then? I'm considering whether there is enough semantic difference between the two to keep them both. For correctness, they'll need to match 100% anyways, so I think not. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc:156: On 2017/03/14 20:48:29, the sun wrote: > I'm missing a unit test for this class. I'll add a few generic tests that could be re-used by other implementations, like: - Its list of supported encoders isn't empty - The encoders it claims to support are all queryable (and the values make sense) - All those encoders are also constructable and can deal with simple calls, like Reset, and the values from GetSampleRateHz etc. match those we get from querying the encoder. I'll also make specific test for this class, to ensure that the compilation flags are picked up properly. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.h (right): https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.h:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. On 2017/03/14 20:48:29, the sun wrote: > Time flies... I'M WORKING ON IT! :C :) https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:51: // bitrate should be in the range between 6000 and 510000. On 2017/03/15 10:09:14, kwiberg-webrtc wrote: > On 2017/03/14 20:48:29, the sun wrote: > > nit: "between" excludes 6000 and 510000 - but below (line 149) you include > them > > Well, I'd say it *sometimes* excludes the endpoints, but that there's no hard > rule---which is at least as bad, of course. > > I would say "in the range [6000, 510000]" or maybe "between 6000 and 510000, > inclusive". The mathematical notation has the advantage of being both short and > precise, and is probably well-known enough. I don't interpret it as excluding. Quick, pick a number between one and five! I'll make the comment clearer, though, by explicitly mentioning it being inclusive.
https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/audio_encoder_factory.h (right): https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder_factory.h:31: virtual rtc::Optional<AudioFormatInfo> QueryAudioFormat( On 2017/03/15 11:26:52, ossu wrote: > On 2017/03/14 20:48:29, the sun wrote: > > May be good to add "Encoder" to this method name - decoder/encoder factory > will > > sometimes be implemented on the same object. > > > > I guess a way to get by with just "IsSupportedEncoder()" would be to let that > > return an rtc::Optional<AudioCodecInfo>? > > The decoder has no QueryAudioFormat method as it stands today, but maybe it > should have in future. I think QueryAudioEncoder and renaming to AudioCodecInfo > makes a lot of sense. FWIW, I think I'd prefer to drop IsSupported[Encoder|Decoder]() in favor of Query[Encoder|Decoder](). Perhaps IsSupported... could live on as a non-virtual helper method, with implementation right here in the class decl? https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.h (right): https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.h:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. On 2017/03/15 11:26:52, ossu wrote: > On 2017/03/14 20:48:29, the sun wrote: > > Time flies... > > I'M WORKING ON IT! :C > > :) Not really what I meant - maybe 2016 should be 2017? https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:51: // bitrate should be in the range between 6000 and 510000. On 2017/03/15 11:26:52, ossu wrote: > On 2017/03/15 10:09:14, kwiberg-webrtc wrote: > > On 2017/03/14 20:48:29, the sun wrote: > > > nit: "between" excludes 6000 and 510000 - but below (line 149) you include > > them > > > > Well, I'd say it *sometimes* excludes the endpoints, but that there's no hard > > rule---which is at least as bad, of course. > > > > I would say "in the range [6000, 510000]" or maybe "between 6000 and 510000, > > inclusive". The mathematical notation has the advantage of being both short > and > > precise, and is probably well-known enough. > > I don't interpret it as excluding. Quick, pick a number between one and five! > I'll make the comment clearer, though, by explicitly mentioning it being > inclusive. I'd actually say "from 1 to 5". :) Use the interval notation - that's unambiguous!
https://codereview.webrtc.org/2695243005/diff/100001/webrtc/api/audio_codecs/... File webrtc/api/audio_codecs/audio_format.cc (right): https://codereview.webrtc.org/2695243005/diff/100001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_format.cc:81: : AudioFormatInfo(0, 0, 0) {} Sorry if I've already asked about this, but why is a default constructor a good idea? It would be nicer if all AudioFormatInfo objects were in a good state. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_format.cc:104: RTC_DCHECK_GE(max_bitrate_bps, default_bitrate_bps); Also check that sample_rate_hz and num_channels are good? https://codereview.webrtc.org/2695243005/diff/100001/webrtc/api/audio_codecs/... File webrtc/api/audio_codecs/audio_format.h (right): https://codereview.webrtc.org/2695243005/diff/100001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_format.h:77: AudioFormatInfo(const AudioFormatInfo& b) = default; On 2017/03/14 20:48:28, the sun wrote: > No need for default move ctor? I forget when that is added automatically... You get an implicitly-declared copy constructor unless you manually declare a copy constructor. You get an implicitly-declared move constructor unless you manually declare a copy constructor, copy assignment operator, move assignment operator, or destructor. For a value-like class such as this one, you should probably either declare all five special thingys with =default, or not mention any of them. I'd go with the latter, especially considering that the class is trivially copyable. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_format.h:94: bool HasFixedBitrate() const { return min_bitrate_bps == max_bitrate_bps; } On 2017/03/14 20:48:29, the sun wrote: > In this case, should there be a DCHECK that default_bitrate_bps is the same as > well? It might be a good idea to have an IsOK() function that tests all sorts of things like this, that we can DCHECK wherever we can. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_format.h:119: }; Define swap() for these two? They'll probably end up being stored in vectors etc. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/audio_encoder_factory.h (right): https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder_factory.h:31: virtual rtc::Optional<AudioFormatInfo> QueryAudioFormat( On 2017/03/14 20:48:29, the sun wrote: > May be good to add "Encoder" to this method name - decoder/encoder factory will > sometimes be implemented on the same object. Hmm. Having one object implement multiple interfaces isn't usually a good idea, I think. Shared ownership makes it technically possible, but still... code is easier to read if objects are small and do just one job. +1 for having unique method names, though. Makes them easier to search for. > I guess a way to get by with just "IsSupportedEncoder()" would be to let that > return an rtc::Optional<AudioCodecInfo>? +1 for combining those two methods, since this isn't performance critical---we can live with creating a few AudioFormatInfos when all the caller wanted was a bool. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder_factory.h:31: virtual rtc::Optional<AudioFormatInfo> QueryAudioFormat( On 2017/03/15 11:57:39, the sun wrote: > On 2017/03/15 11:26:52, ossu wrote: > > On 2017/03/14 20:48:29, the sun wrote: > > > May be good to add "Encoder" to this method name - decoder/encoder factory > > will > > > sometimes be implemented on the same object. > > > > > > I guess a way to get by with just "IsSupportedEncoder()" would be to let > that > > > return an rtc::Optional<AudioCodecInfo>? > > > > The decoder has no QueryAudioFormat method as it stands today, but maybe it > > should have in future. I think QueryAudioEncoder and renaming to > AudioCodecInfo > > makes a lot of sense. > > FWIW, I think I'd prefer to drop IsSupported[Encoder|Decoder]() in favor of > Query[Encoder|Decoder](). > > Perhaps IsSupported... could live on as a non-virtual helper method, with > implementation right here in the class decl? The helper is unnecessary---rtc::Optional converts to bool: if (foo.QueryAudioFormat(...)) { https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory_internal.cc (right): https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory_internal.cc:189: // clang-format on Why change the formatting? https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory_internal.cc:191: opus_info.supports_network_adaption = true; Move these two to just after line 183? https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc (right): https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc:2: * Copyright (c) 2016 The WebRTC project authors. All Rights Reserved. +1 https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc:20: #ifdef WEBRTC_CODEC_G722 On 2017/03/14 20:48:29, the sun wrote: > Can you file an issue to deprecate these compiler flags? Should we deprecate them? Isn't the idea to provide gn flags for basically every built-in codec? If so, that would be implemented with preprocessor defines... I'd love to have them defined to 1 or 0, though, instead of defined/undefined. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc:54: if (opt_info) { Why store the return value in a local variable? https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc:86: class BuiltinAudioEncoderFactory : public AudioEncoderFactory { Put this class too in the anonymous namespace? Also, final? You even call one of your own virtual methods below... https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc:88: BuiltinAudioEncoderFactory() {} Is this necessary? https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc:91: return GetSupportedEncodersInternal(); Why delegate to a private method? https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc:130: }; Move this into the lambda? https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc:133: // initialized once, and that that initialization is thread-safe. That static variables are initialized once, on first use is sufficiently well-known, I think. But if you must say something, just // This is initialized once, on first use. will do. That it's thread safe goes without saying, because if it weren't, it would be broken. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc:134: static std::vector<AudioCodecSpec> supported_encoders = [] { const? For thread safety... https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc:135: std::vector<AudioCodecSpec> supported_encoders; Call this something else, since shadowing is confusing? Maybe even something short? https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t_impl.h (right): https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t_impl.h:111: (config.sample_rate_hz == 16000) ? 32000 : 56000}); You repeat this expression twice, in ways that are not trivially identical. Subroutine? https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:51: // bitrate should be in the range between 6000 and 510000. On 2017/03/15 11:26:52, ossu wrote: > On 2017/03/15 10:09:14, kwiberg-webrtc wrote: > > On 2017/03/14 20:48:29, the sun wrote: > > > nit: "between" excludes 6000 and 510000 - but below (line 149) you include > > them > > > > Well, I'd say it *sometimes* excludes the endpoints, but that there's no hard > > rule---which is at least as bad, of course. > > > > I would say "in the range [6000, 510000]" or maybe "between 6000 and 510000, > > inclusive". The mathematical notation has the advantage of being both short > and > > precise, and is probably well-known enough. > > I don't interpret it as excluding. Quick, pick a number between one and five! Exactly. > I'll make the comment clearer, though, by explicitly mentioning it being > inclusive. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:51: // bitrate should be in the range between 6000 and 510000. On 2017/03/15 11:57:40, the sun wrote: > On 2017/03/15 11:26:52, ossu wrote: > > On 2017/03/15 10:09:14, kwiberg-webrtc wrote: > > > On 2017/03/14 20:48:29, the sun wrote: > > > > nit: "between" excludes 6000 and 510000 - but below (line 149) you include > > > them > > > > > > Well, I'd say it *sometimes* excludes the endpoints, but that there's no > hard > > > rule---which is at least as bad, of course. > > > > > > I would say "in the range [6000, 510000]" or maybe "between 6000 and 510000, > > > inclusive". The mathematical notation has the advantage of being both short > > and > > > precise, and is probably well-known enough. > > > > I don't interpret it as excluding. Quick, pick a number between one and five! > > I'll make the comment clearer, though, by explicitly mentioning it being > > inclusive. > > I'd actually say "from 1 to 5". :) Use the interval notation - that's > unambiguous! "[...] the bitrate should be an integer n such that 6000 ā¤ n ā¤ 510000". :-) https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:53: const int kOpusMaxBitrateBps = 510000; On 2017/03/14 20:25:11, ossu wrote: > I removed the old kMin/MaxBitrateBps and replaced with these, that originally > come from webrtcvoiceengine.cc. This means I also changed the max to 510 kbps, > instead of 512 kbps. From a quick web search, I can only find references to 510 > kbps as being the max for Opus. Make these five constexpr? https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:111: const std::string& param) { Change this to return a const char* that points to the string? https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:125: int bitrate = 0; Ick. Default value that's never used, mutable variable... lambda? https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:142: rtc::Optional<std::string> bitrate_param) { Make bitrate_param a const char*? https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:146: if (bitrate_param) { Invert condition + early return? https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:148: if (bitrate) { Invert condition + early return? https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:158: return new_bitrate; Don't we have at least one library function to do clamping? https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:195: format.clockrate_hz == 48000 && format.num_channels == 2) { Invert condition + early return? https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:235: // Normally, the first two parameters should already have been checked buy by https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:236: // QueryAudioFormat At this point, we might as well fall back to something . https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:254: kOpusSupportedFrameLengths[arraysize(kOpusSupportedFrameLengths) - 1]; Maybe *(std::end(kOpusSupportedFrameLengths) - 1) https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:270: // of this range, it will get adjusted once ANA takes hold. Ideally, we'd now know https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:282: if (min_frame_length_ms <= max_frame_length_ms) { This conditional is just an optimization; since this isn't performance critical, remove it. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:294: config.supported_frame_lengths_ms.push_back(frame_length_ms); std::vector::assign? https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc (right): https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:50: webrtc::SdpAudioFormat format("opus", 48000, 2, params); Is the webrtc:: prefix necessary? https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:560: EXPECT_EQ(1U, config.num_channels); Lower-case "u"? It's easier to read, because all digits are full-height. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:596: } EXPECT the full list, to make the test easier to read? Unless you expect it to change with any frequency, of course. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/pcm16b/audio_encoder_pcm16b.h (right): https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/pcm16b/audio_encoder_pcm16b.h:36: static constexpr const char* GetPayloadName() { return "l16"; } Since you're not consistently using lowercase in these names, use capital L here. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/neteq/audio_decoder_unittest.cc (right): https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/audio_decoder_unittest.cc:615: EXPECT_EQ(510000, SetAndGetTargetBitrate(audio_encoder, 510000)); The last of these four should be 511000, I believe.
https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/audio_encoder_factory.h (right): https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder_factory.h:31: virtual rtc::Optional<AudioFormatInfo> QueryAudioFormat( On 2017/03/15 13:33:17, kwiberg-webrtc wrote: > On 2017/03/15 11:57:39, the sun wrote: > > On 2017/03/15 11:26:52, ossu wrote: > > > On 2017/03/14 20:48:29, the sun wrote: > > > > May be good to add "Encoder" to this method name - decoder/encoder factory > > > will > > > > sometimes be implemented on the same object. > > > > > > > > I guess a way to get by with just "IsSupportedEncoder()" would be to let > > that > > > > return an rtc::Optional<AudioCodecInfo>? > > > > > > The decoder has no QueryAudioFormat method as it stands today, but maybe it > > > should have in future. I think QueryAudioEncoder and renaming to > > AudioCodecInfo > > > makes a lot of sense. > > > > FWIW, I think I'd prefer to drop IsSupported[Encoder|Decoder]() in favor of > > Query[Encoder|Decoder](). > > > > Perhaps IsSupported... could live on as a non-virtual helper method, with > > implementation right here in the class decl? > > The helper is unnecessary---rtc::Optional converts to bool: > > if (foo.QueryAudioFormat(...)) { Yeah, but it may make the code more readable... https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder_factory.h:31: virtual rtc::Optional<AudioFormatInfo> QueryAudioFormat( On 2017/03/15 13:33:17, kwiberg-webrtc wrote: > On 2017/03/14 20:48:29, the sun wrote: > > May be good to add "Encoder" to this method name - decoder/encoder factory > will > > sometimes be implemented on the same object. > > Hmm. Having one object implement multiple interfaces isn't usually a good idea, > I think. Shared ownership makes it technically possible, but still... code is > easier to read if objects are small and do just one job. Agree, but in certain cases there may be shared resources backing the codec objects (e.g. hardware enc/dec). > > +1 for having unique method names, though. Makes them easier to search for. > > > I guess a way to get by with just "IsSupportedEncoder()" would be to let that > > return an rtc::Optional<AudioCodecInfo>? > > +1 for combining those two methods, since this isn't performance critical---we > can live with creating a few AudioFormatInfos when all the caller wanted was a > bool.
https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/audio_encoder_factory.h (right): https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder_factory.h:31: virtual rtc::Optional<AudioFormatInfo> QueryAudioFormat( On 2017/03/15 13:37:44, the sun wrote: > On 2017/03/15 13:33:17, kwiberg-webrtc wrote: > > On 2017/03/14 20:48:29, the sun wrote: > > > May be good to add "Encoder" to this method name - decoder/encoder factory > > will > > > sometimes be implemented on the same object. > > > > Hmm. Having one object implement multiple interfaces isn't usually a good > idea, > > I think. Shared ownership makes it technically possible, but still... code is > > easier to read if objects are small and do just one job. > > Agree, but in certain cases there may be shared resources backing the codec > objects (e.g. hardware enc/dec). In which case the encoder and decoder objects should share owership of the object that manages the resource. I'm not saying that not implementing more than one interface is an end in itself, just that it's a common consequence of not making objects that are too large and/or do too many things. > > > > +1 for having unique method names, though. Makes them easier to search for. > > > > > I guess a way to get by with just "IsSupportedEncoder()" would be to let > that > > > return an rtc::Optional<AudioCodecInfo>? > > > > +1 for combining those two methods, since this isn't performance critical---we > > can live with creating a few AudioFormatInfos when all the caller wanted was a > > bool. > https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder_factory.h:31: virtual rtc::Optional<AudioFormatInfo> QueryAudioFormat( On 2017/03/15 13:37:44, the sun wrote: > On 2017/03/15 13:33:17, kwiberg-webrtc wrote: > > On 2017/03/15 11:57:39, the sun wrote: > > > On 2017/03/15 11:26:52, ossu wrote: > > > > On 2017/03/14 20:48:29, the sun wrote: > > > > > May be good to add "Encoder" to this method name - decoder/encoder > factory > > > > will > > > > > sometimes be implemented on the same object. > > > > > > > > > > I guess a way to get by with just "IsSupportedEncoder()" would be to let > > > that > > > > > return an rtc::Optional<AudioCodecInfo>? > > > > > > > > The decoder has no QueryAudioFormat method as it stands today, but maybe > it > > > > should have in future. I think QueryAudioEncoder and renaming to > > > AudioCodecInfo > > > > makes a lot of sense. > > > > > > FWIW, I think I'd prefer to drop IsSupported[Encoder|Decoder]() in favor of > > > Query[Encoder|Decoder](). > > > > > > Perhaps IsSupported... could live on as a non-virtual helper method, with > > > implementation right here in the class decl? > > > > The helper is unnecessary---rtc::Optional converts to bool: > > > > if (foo.QueryAudioFormat(...)) { > > Yeah, but it may make the code more readable... In this case not much, I'd argue. It's pretty obvious what the boolean version of the return value of "QueryAudioFormat" could be. But I agree in the general case.
LGTM on the Opus bitrate. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:53: const int kOpusMaxBitrateBps = 510000; On 2017/03/14 20:25:11, ossu wrote: > I removed the old kMin/MaxBitrateBps and replaced with these, that originally > come from webrtcvoiceengine.cc. This means I also changed the max to 510 kbps, > instead of 512 kbps. From a quick web search, I can only find references to 510 > kbps as being the max for Opus. Looks good. This is in line with the documentation in opus source: https://git.xiph.org/?p=opus.git;a=blob;f=include/opus.h;h=5be73ddf4e5dd2e0cd....
New patch set up. Mostly small fixes; the biggest new thing is the test for BuiltinAudioEncoderFactory. It's value-parameterized, so at some point we can break it out and allow others to use it for their own implementations. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/api/audio_codecs/... File webrtc/api/audio_codecs/audio_format.cc (right): https://codereview.webrtc.org/2695243005/diff/100001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_format.cc:81: : AudioFormatInfo(0, 0, 0) {} On 2017/03/15 13:33:17, kwiberg-webrtc wrote: > Sorry if I've already asked about this, but why is a default constructor a good > idea? It would be nicer if all AudioFormatInfo objects were in a good state. It was necessary for one of the previous incarnations of this, but now it's not. So I've removed it. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_format.cc:104: RTC_DCHECK_GE(max_bitrate_bps, default_bitrate_bps); On 2017/03/15 13:33:17, kwiberg-webrtc wrote: > Also check that sample_rate_hz and num_channels are good? Sure. I've just done the most basic of checks, that they are greater than zero. I don't think this is the right place to enforce that an encoder is compatible with specific sample rates or number of channels supported in WebRTC, for example. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory_internal.cc (right): https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory_internal.cc:189: // clang-format on On 2017/03/15 13:33:17, kwiberg-webrtc wrote: > Why change the formatting? I have no idea. I must have reindented it without thinking. // ossu-format off https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory_internal.cc:191: opus_info.supports_network_adaption = true; On 2017/03/15 13:33:17, kwiberg-webrtc wrote: > Move these two to just after line 183? Agreed! https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc (right): https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc:86: class BuiltinAudioEncoderFactory : public AudioEncoderFactory { On 2017/03/15 13:33:17, kwiberg-webrtc wrote: > Put this class too in the anonymous namespace? > Also, final? You even call one of your own virtual methods below... It cannot be final, since it requires wrapping in rtc::RefCountedObject to fulfill AddRef / Release. Why does it matter that I'm calling one of my (possibly) virtual methods below? Possibly avoiding an indirect call? https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc:88: BuiltinAudioEncoderFactory() {} On 2017/03/15 13:33:17, kwiberg-webrtc wrote: > Is this necessary? Probably not. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc:91: return GetSupportedEncodersInternal(); On 2017/03/15 13:33:17, kwiberg-webrtc wrote: > Why delegate to a private method? I believe I used the internal method at some point, since it returns a reference rather than a copy. It's not necessary anymore, though. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc:130: }; On 2017/03/15 13:33:17, kwiberg-webrtc wrote: > Move this into the lambda? I can't honestly tell you why it is reachable by the lambda at this point? Seems like its staticness, but I'm unsure if this is correct or just happens to work. I kind-of like it outside the lambda, though. I find it easier to read when separate from the code that uses it. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc:133: // initialized once, and that that initialization is thread-safe. On 2017/03/15 13:33:17, kwiberg-webrtc wrote: > That static variables are initialized once, on first use is sufficiently > well-known, I think. But if you must say something, just > > // This is initialized once, on first use. > > will do. That it's thread safe goes without saying, because if it weren't, it > would be broken. Sure, it would be broken if it weren't, but I'm not sure that's well known enough that it doesn't warrant a comment. I think this is a relatively new construct in our code-base, so if someone isn't sure, it's good they don't have to spend time researching it. I'll try to slim down the comment a bit. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc:134: static std::vector<AudioCodecSpec> supported_encoders = [] { On 2017/03/15 13:33:17, kwiberg-webrtc wrote: > const? For thread safety... Is that more thread-safe? https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc:135: std::vector<AudioCodecSpec> supported_encoders; On 2017/03/15 13:33:17, kwiberg-webrtc wrote: > Call this something else, since shadowing is confusing? Maybe even something > short? The shadowing here is intended, but perhaps unnecessary. My intention was that nothing would manage to refer to the outer supported_encoders, but the empty lambda capture should've seen to that. I've shortened the name a bit, though, and moved it all up into GetSupportedEncoders. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t_impl.h (right): https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t_impl.h:111: (config.sample_rate_hz == 16000) ? 32000 : 56000}); On 2017/03/15 13:33:18, kwiberg-webrtc wrote: > You repeat this expression twice, in ways that are not trivially identical. > Subroutine? Acknowledged. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:51: // bitrate should be in the range between 6000 and 510000. On 2017/03/15 13:33:18, kwiberg-webrtc wrote: > On 2017/03/15 11:57:40, the sun wrote: > > On 2017/03/15 11:26:52, ossu wrote: > > > On 2017/03/15 10:09:14, kwiberg-webrtc wrote: > > > > On 2017/03/14 20:48:29, the sun wrote: > > > > > nit: "between" excludes 6000 and 510000 - but below (line 149) you > include > > > > them > > > > > > > > Well, I'd say it *sometimes* excludes the endpoints, but that there's no > > hard > > > > rule---which is at least as bad, of course. > > > > > > > > I would say "in the range [6000, 510000]" or maybe "between 6000 and > 510000, > > > > inclusive". The mathematical notation has the advantage of being both > short > > > and > > > > precise, and is probably well-known enough. > > > > > > I don't interpret it as excluding. Quick, pick a number between one and > five! > > > I'll make the comment clearer, though, by explicitly mentioning it being > > > inclusive. > > > > I'd actually say "from 1 to 5". :) Use the interval notation - that's > > unambiguous! > > "[...] the bitrate should be an integer n such that 6000 ā¤ n ā¤ 510000". :-) This has got to be the biggest bike-shed in quite a while! :) https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:111: const std::string& param) { On 2017/03/15 13:33:18, kwiberg-webrtc wrote: > Change this to return a const char* that points to the string? I looked at it, but it makes a number of things less nice, like having to explicitly type std::string("1") in the comparisons, or do nasty strcmp calls. Ideally, we'd have a StringPiece like what chromium does, to ferry around string data with very low cost. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:125: int bitrate = 0; On 2017/03/15 13:33:18, kwiberg-webrtc wrote: > Ick. Default value that's never used, mutable variable... lambda? It's not that bad :) https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:142: rtc::Optional<std::string> bitrate_param) { On 2017/03/15 13:33:18, kwiberg-webrtc wrote: > Make bitrate_param a const char*? I've kept this as-is since it'll interface directly with GetFormatParameter, above. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:146: if (bitrate_param) { On 2017/03/15 13:33:18, kwiberg-webrtc wrote: > Invert condition + early return? No. I like this flow. There's only one way we can return the default bitrate, and that's if we've no usable maxaveragebitrate. The nesting is only a couple of levels. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:148: if (bitrate) { On 2017/03/15 13:33:18, kwiberg-webrtc wrote: > Invert condition + early return? No, see above. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:158: return new_bitrate; On 2017/03/15 13:33:18, kwiberg-webrtc wrote: > Don't we have at least one library function to do clamping? None that I can find. We probably should have one that looks like std::clamp. Many places could be cleaned up by this. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:195: format.clockrate_hz == 48000 && format.num_channels == 2) { On 2017/03/15 13:33:18, kwiberg-webrtc wrote: > Invert condition + early return? As with the other one, I prefer having the error/fallback return at the end, rather than spread throughout the code. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:235: // Normally, the first two parameters should already have been checked buy On 2017/03/15 13:33:18, kwiberg-webrtc wrote: > by Acknowledged. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:236: // QueryAudioFormat At this point, we might as well fall back to something On 2017/03/15 13:33:18, kwiberg-webrtc wrote: > . Acknowledged. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:254: kOpusSupportedFrameLengths[arraysize(kOpusSupportedFrameLengths) - 1]; On 2017/03/15 13:33:18, kwiberg-webrtc wrote: > Maybe > > *(std::end(kOpusSupportedFrameLengths) - 1) Tried it but the compiler claims thats's not constexpr. Looks like it might be in newer versions of the standard. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:270: // of this range, it will get adjusted once ANA takes hold. Ideally, we'd now On 2017/03/15 13:33:18, kwiberg-webrtc wrote: > know Maan.. I was typing in a hurry last night! https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:282: if (min_frame_length_ms <= max_frame_length_ms) { On 2017/03/15 13:33:18, kwiberg-webrtc wrote: > This conditional is just an optimization; since this isn't performance critical, > remove it. Acknowledged. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:294: config.supported_frame_lengths_ms.push_back(frame_length_ms); On 2017/03/15 13:33:18, kwiberg-webrtc wrote: > std::vector::assign? I find it a bit less clear what's going on (for-loops are common, std::vector::assign isn't) but it might be slightly faster - not that that should matter at this point. They're both a bit messy. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc (right): https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:50: webrtc::SdpAudioFormat format("opus", 48000, 2, params); On 2017/03/15 13:33:18, kwiberg-webrtc wrote: > Is the webrtc:: prefix necessary? Nope. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:560: EXPECT_EQ(1U, config.num_channels); On 2017/03/15 13:33:18, kwiberg-webrtc wrote: > Lower-case "u"? It's easier to read, because all digits are full-height. Acknowledged. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:596: } On 2017/03/15 13:33:18, kwiberg-webrtc wrote: > EXPECT the full list, to make the test easier to read? Unless you expect it to > change with any frequency, of course. I don't want to make the test overly strict. If we provide a reasonable maximum ptime, we should only get ptimes <= to it. I don't specifically care which those are. The Opus codec seems to be getting the most attention of our codecs, so chances are this will change a few times more. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/pcm16b/audio_encoder_pcm16b.h (right): https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/pcm16b/audio_encoder_pcm16b.h:36: static constexpr const char* GetPayloadName() { return "l16"; } On 2017/03/15 13:33:19, kwiberg-webrtc wrote: > Since you're not consistently using lowercase in these names, use capital L > here. Acknowledged. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/neteq/audio_decoder_unittest.cc (right): https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/neteq/audio_decoder_unittest.cc:615: EXPECT_EQ(510000, SetAndGetTargetBitrate(audio_encoder, 510000)); On 2017/03/15 13:33:19, kwiberg-webrtc wrote: > The last of these four should be 511000, I believe. Acknowledged.
LGTM! Very nice! https://codereview.webrtc.org/2695243005/diff/120001/webrtc/api/audio_codecs/... File webrtc/api/audio_codecs/audio_format.h (right): https://codereview.webrtc.org/2695243005/diff/120001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_format.h:95: min_bitrate_bps == default_bitrate_bps == max_bitrate_bps); Do you really want to compare max_bitrate_bps with a bool? Or you may point me at information explaining that this is parsed and evaluated the way you intend... https://codereview.webrtc.org/2695243005/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/audio_encoder_factory.h (right): https://codereview.webrtc.org/2695243005/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder_factory.h:29: virtual rtc::Optional<AudioCodecInfo> QueryAudioEncoder( nit: A short line describing what the method is supposed to do, would be good. For all of the methods in this interface. https://codereview.webrtc.org/2695243005/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2695243005/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:13: #include <stdlib.h> nit: what for?
Thanks! :) https://codereview.webrtc.org/2695243005/diff/120001/webrtc/api/audio_codecs/... File webrtc/api/audio_codecs/audio_format.h (right): https://codereview.webrtc.org/2695243005/diff/120001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_format.h:95: min_bitrate_bps == default_bitrate_bps == max_bitrate_bps); On 2017/03/17 09:07:31, the sun wrote: > Do you really want to compare max_bitrate_bps with a bool? Or you may point me > at information explaining that this is parsed and evaluated the way you > intend... I... no, I don't know what I was thinking here. I'll fix it. https://codereview.webrtc.org/2695243005/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/audio_encoder_factory.h (right): https://codereview.webrtc.org/2695243005/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder_factory.h:29: virtual rtc::Optional<AudioCodecInfo> QueryAudioEncoder( On 2017/03/17 09:07:31, the sun wrote: > nit: A short line describing what the method is supposed to do, would be good. > For all of the methods in this interface. I'll document them all! https://codereview.webrtc.org/2695243005/diff/120001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2695243005/diff/120001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:13: #include <stdlib.h> On 2017/03/17 09:07:31, the sun wrote: > nit: what for? Not sure. Will look into it. May be a hold-over from an older version.
I haven't looket at the new patch set, just answered comments. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/api/audio_codecs/... File webrtc/api/audio_codecs/audio_format.cc (right): https://codereview.webrtc.org/2695243005/diff/100001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_format.cc:104: RTC_DCHECK_GE(max_bitrate_bps, default_bitrate_bps); On 2017/03/16 18:03:56, ossu wrote: > On 2017/03/15 13:33:17, kwiberg-webrtc wrote: > > Also check that sample_rate_hz and num_channels are good? > > Sure. I've just done the most basic of checks, that they are greater than zero. > I don't think this is the right place to enforce that an encoder is compatible > with specific sample rates or number of channels supported in WebRTC, for > example. Excellent, that was exactly what I meant. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc (right): https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc:86: class BuiltinAudioEncoderFactory : public AudioEncoderFactory { On 2017/03/16 18:03:57, ossu wrote: > On 2017/03/15 13:33:17, kwiberg-webrtc wrote: > > Put this class too in the anonymous namespace? > > Also, final? You even call one of your own virtual methods below... > > It cannot be final, since it requires wrapping in rtc::RefCountedObject to > fulfill AddRef / Release. Ah, OK. > Why does it matter that I'm calling one of my (possibly) virtual methods below? > Possibly avoiding an indirect call? Yes---the primary benefit of "final" is to reassure the reader that there are no subclasses, but the compiler can also use it to devirtualize function calls. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc:95: return !!QueryAudioFormat(format); On 2017/03/15 11:26:52, ossu wrote: > On 2017/03/14 20:48:29, the sun wrote: > > Ah, haha. What about just having one method then? > > I'm considering whether there is enough semantic difference between the two to > keep them both. For correctness, they'll need to match 100% anyways, so I think > not. +1 to not having two separate methods, both because it's one more than we need, and because of the possibility of them not matching. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc:130: }; On 2017/03/16 18:03:57, ossu wrote: > On 2017/03/15 13:33:17, kwiberg-webrtc wrote: > > Move this into the lambda? > > I can't honestly tell you why it is reachable by the lambda at this point? Seems > like its staticness, but I'm unsure if this is correct or just happens to work. Yeah, you're right, that's surprising---I wouldn't have thought it'd be available. > I kind-of like it outside the lambda, though. I find it easier to read when > separate from the code that uses it. OK. It's not as if bringing it inside the lambda would reduce its scope a whole lot anyway... https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc:133: // initialized once, and that that initialization is thread-safe. On 2017/03/16 18:03:57, ossu wrote: > On 2017/03/15 13:33:17, kwiberg-webrtc wrote: > > That static variables are initialized once, on first use is sufficiently > > well-known, I think. But if you must say something, just > > > > // This is initialized once, on first use. > > > > will do. That it's thread safe goes without saying, because if it weren't, it > > would be broken. > > Sure, it would be broken if it weren't, but I'm not sure that's well known > enough that it doesn't warrant a comment. I think this is a relatively new > construct in our code-base, so if someone isn't sure, it's good they don't have > to spend time researching it. I'll try to slim down the comment a bit. Acknowledged. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc:134: static std::vector<AudioCodecSpec> supported_encoders = [] { On 2017/03/16 18:03:57, ossu wrote: > On 2017/03/15 13:33:17, kwiberg-webrtc wrote: > > const? For thread safety... > > Is that more thread-safe? Well, yes---initialization of mutable statics is thread safe, but no one is taking care of protecting the mutating operations for you. Now, you don't happen to actually make any mutating operations, but unless you say "const" here, the compiler won't hold you to that. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc:135: std::vector<AudioCodecSpec> supported_encoders; On 2017/03/16 18:03:57, ossu wrote: > On 2017/03/15 13:33:17, kwiberg-webrtc wrote: > > Call this something else, since shadowing is confusing? Maybe even something > > short? > > The shadowing here is intended, but perhaps unnecessary. My intention was that > nothing would manage to refer to the outer supported_encoders, but the empty > lambda capture should've seen to that. I've shortened the name a bit, though, > and moved it all up into GetSupportedEncoders. Acknowledged. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc:156: On 2017/03/15 11:26:52, ossu wrote: > On 2017/03/14 20:48:29, the sun wrote: > > I'm missing a unit test for this class. > > I'll add a few generic tests that could be re-used by other implementations, > like: > - Its list of supported encoders isn't empty > - The encoders it claims to support are all queryable (and the values make > sense) > - All those encoders are also constructable and can deal with simple calls, like > Reset, and the values from GetSampleRateHz etc. match those we get from querying > the encoder. > > I'll also make specific test for this class, to ensure that the compilation > flags are picked up properly. > > Acknowledged. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:51: // bitrate should be in the range between 6000 and 510000. On 2017/03/16 18:03:58, ossu wrote: > On 2017/03/15 13:33:18, kwiberg-webrtc wrote: > > On 2017/03/15 11:57:40, the sun wrote: > > > On 2017/03/15 11:26:52, ossu wrote: > > > > On 2017/03/15 10:09:14, kwiberg-webrtc wrote: > > > > > On 2017/03/14 20:48:29, the sun wrote: > > > > > > nit: "between" excludes 6000 and 510000 - but below (line 149) you > > include > > > > > them > > > > > > > > > > Well, I'd say it *sometimes* excludes the endpoints, but that there's no > > > hard > > > > > rule---which is at least as bad, of course. > > > > > > > > > > I would say "in the range [6000, 510000]" or maybe "between 6000 and > > 510000, > > > > > inclusive". The mathematical notation has the advantage of being both > > short > > > > and > > > > > precise, and is probably well-known enough. > > > > > > > > I don't interpret it as excluding. Quick, pick a number between one and > > five! > > > > I'll make the comment clearer, though, by explicitly mentioning it being > > > > inclusive. > > > > > > I'd actually say "from 1 to 5". :) Use the interval notation - that's > > > unambiguous! > > > > "[...] the bitrate should be an integer n such that 6000 ā¤ n ā¤ 510000". :-) > > This has got to be the biggest bike-shed in quite a while! :) Well, fortunately I was just pulling your leg. I think we reached consensus on the [6000, 510000] notation. :-) https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:111: const std::string& param) { On 2017/03/16 18:03:57, ossu wrote: > On 2017/03/15 13:33:18, kwiberg-webrtc wrote: > > Change this to return a const char* that points to the string? > > I looked at it, but it makes a number of things less nice, like having to > explicitly type std::string("1") in the comparisons, or do nasty strcmp calls. Hmm. std::string::operator== accepts const char* arguments. Are you talking about comparing two const char*? > Ideally, we'd have a StringPiece like what chromium does, to ferry around string > data with very low cost. Yes. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:125: int bitrate = 0; On 2017/03/16 18:03:57, ossu wrote: > On 2017/03/15 13:33:18, kwiberg-webrtc wrote: > > Ick. Default value that's never used, mutable variable... lambda? > > It's not that bad :) Hmm. const int bitrate = num_channels * (max_playback_rate <= 8000 ? kOpusBitrateNbBps : max_playback_rate <= 16000 ? kOpusBitrateWbBps : kOpusBitrateFbBps); If you don't like that either, I've been toying with the idea of defining a function that returns a value unchanged, but msan-poisoned. So this line would become something like int bitrate = rtc::MsanUninitialized(0); which would behave exactly like the current code in normal builds, but additionally (1) clearly signal that the author thinks the value will always be overwritten, and (2) let MSan verify this. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:142: rtc::Optional<std::string> bitrate_param) { On 2017/03/16 18:03:58, ossu wrote: > On 2017/03/15 13:33:18, kwiberg-webrtc wrote: > > Make bitrate_param a const char*? > > I've kept this as-is since it'll interface directly with GetFormatParameter, > above. Acknowledged. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:146: if (bitrate_param) { On 2017/03/16 18:03:58, ossu wrote: > On 2017/03/15 13:33:18, kwiberg-webrtc wrote: > > Invert condition + early return? > > No. I like this flow. There's only one way we can return the default bitrate, > and that's if we've no usable maxaveragebitrate. The nesting is only a couple of > levels. Acknowledged. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:148: if (bitrate) { On 2017/03/16 18:03:58, ossu wrote: > On 2017/03/15 13:33:18, kwiberg-webrtc wrote: > > Invert condition + early return? > > No, see above. Acknowledged. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:158: return new_bitrate; On 2017/03/16 18:03:57, ossu wrote: > On 2017/03/15 13:33:18, kwiberg-webrtc wrote: > > Don't we have at least one library function to do clamping? > > None that I can find. We probably should have one that looks like std::clamp. > Many places could be cleaned up by this. You're right, we actually don't appear to have one. :-( https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:195: format.clockrate_hz == 48000 && format.num_channels == 2) { On 2017/03/16 18:03:58, ossu wrote: > On 2017/03/15 13:33:18, kwiberg-webrtc wrote: > > Invert condition + early return? > > As with the other one, I prefer having the error/fallback return at the end, > rather than spread throughout the code. Acknowledged. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:254: kOpusSupportedFrameLengths[arraysize(kOpusSupportedFrameLengths) - 1]; On 2017/03/16 18:03:57, ossu wrote: > On 2017/03/15 13:33:18, kwiberg-webrtc wrote: > > Maybe > > > > *(std::end(kOpusSupportedFrameLengths) - 1) > > Tried it but the compiler claims thats's not constexpr. Looks like it might be > in newer versions of the standard. Ah well. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:294: config.supported_frame_lengths_ms.push_back(frame_length_ms); On 2017/03/16 18:03:58, ossu wrote: > On 2017/03/15 13:33:18, kwiberg-webrtc wrote: > > std::vector::assign? > > I find it a bit less clear what's going on (for-loops are common, > std::vector::assign isn't) but it might be slightly faster - not that that > should matter at this point. They're both a bit messy. Acknowledged. https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc (right): https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:596: } On 2017/03/16 18:03:58, ossu wrote: > On 2017/03/15 13:33:18, kwiberg-webrtc wrote: > > EXPECT the full list, to make the test easier to read? Unless you expect it to > > change with any frequency, of course. > > I don't want to make the test overly strict. If we provide a reasonable maximum > ptime, we should only get ptimes <= to it. I don't specifically care which those > are. The Opus codec seems to be getting the most attention of our codecs, so > chances are this will change a few times more. Acknowledged.
ossu@webrtc.org changed reviewers: + minyue@webrtc.org
Adding minyue@ to take a look at the changes to AudioEncoderOpus, specifically the parsing of Opus parameters, which have moved in from webrtcvoiceengine.cc. Welcome back! :)
Patchset #7 (id:140001) has been deleted
A few small changes to this one still. https://codereview.webrtc.org/2695243005/diff/160001/webrtc/media/engine/webr... File webrtc/media/engine/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/2695243005/diff/160001/webrtc/media/engine/webr... webrtc/media/engine/webrtcvoiceengine.cc:505: const WebRtcVoiceCodecs::CodecPref WebRtcVoiceCodecs::kCodecPrefs[14] = { This is the list of information I'm basing ptimes and bitrates on. https://codereview.webrtc.org/2695243005/diff/160001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t_impl.h (right): https://codereview.webrtc.org/2695243005/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t_impl.h:54: if (ptime && *ptime >= 60) { The ptime parsers are based on the values found in kCodecPrefs in webrtcvoiceengine.cc. https://codereview.webrtc.org/2695243005/diff/160001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2695243005/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:189: void FindSupportedFrameLengths(int min_frame_length_ms, int max_frame_length_ms, Decided to reuse the code from SetReceiverFrameLengthRange instead. It does not fall back to using the whole list if the result is empty.
https://codereview.webrtc.org/2695243005/diff/160001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t_impl.h (right): https://codereview.webrtc.org/2695243005/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t_impl.h:49: // We only support different frame sizes at 16000 kHz. That rate is *way* too high. :P https://codereview.webrtc.org/2695243005/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t_impl.h:54: if (ptime && *ptime >= 60) { On 2017/03/20 18:18:53, ossu wrote: > The ptime parsers are based on the values found in kCodecPrefs in > webrtcvoiceengine.cc. Acknowledged.
good work! Some readability comments from me (I only looked at audio encoder opus for the moment) https://codereview.webrtc.org/2695243005/diff/160001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2695243005/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:57: constexpr int kANASupportedFrameLengths[] = {20, 60, 120}; I think ANA should do the filtering internally. I will check it, if not, we shall implement it. Anyways, I will follow this in a separate CL. https://codereview.webrtc.org/2695243005/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:123: const int bitrate = [&] { just curious, what is benefit of lambda here? https://codereview.webrtc.org/2695243005/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:133: RTC_DCHECK_LE(bitrate, kOpusMaxBitrateBps); These are more of a compile time check, i.e., kOpusMinBitrateBps < kOpusBitrateNbBps < kOpusMaxBitrateBps / 2, etc something like static_assert would fit best, but I am not sure if we can use it. https://codereview.webrtc.org/2695243005/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:149: return *bitrate; 148 - 157 is fairly ugly. can we refactor this part? how about int chosen_bitrate = min(max(*bitrate, kOpusMinBitrateBps), kOpusMaxBitrateBps); if (*bitrate != chosen_bitrate ) { LOG(LS_WARNING) << } https://codereview.webrtc.org/2695243005/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:180: return rtc::Optional<int>(48000); can we constexpr kDefaultMaxPlaybackRate = 48000 https://codereview.webrtc.org/2695243005/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:186: return rtc::Optional<int>(); It is hard for me to understand rtc::Optional<int>()? does that mean wrong parsed value, then should the Get functions decide to use the default value? Or we can get rid of all Get functions? https://codereview.webrtc.org/2695243005/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:268: const auto ptime = GetFormatParameter<int>(format, "ptime"); add a helper function on pTime?
https://codereview.webrtc.org/2695243005/diff/160001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t_impl.h (right): https://codereview.webrtc.org/2695243005/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t_impl.h:49: // We only support different frame sizes at 16000 kHz. On 2017/03/20 20:53:40, the sun wrote: > That rate is *way* too high. :P :D https://codereview.webrtc.org/2695243005/diff/160001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2695243005/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:57: constexpr int kANASupportedFrameLengths[] = {20, 60, 120}; On 2017/03/21 08:29:44, minyue-webrtc wrote: > I think ANA should do the filtering internally. I will check it, if not, we > shall implement it. Anyways, I will follow this in a separate CL. That makes sense. This is currently a bit confusing, having two separate sets of supported frame lengths next to each other. https://codereview.webrtc.org/2695243005/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:123: const int bitrate = [&] { On 2017/03/21 08:29:44, minyue-webrtc wrote: > just curious, what is benefit of lambda here? Nothing major. It allows us to make bitrate const by initializing up front. If it weren't for the DCHECKs, below, we could just have returned CalculateDefaultBitrate directly. For background, see kwiberg@s comments here: https://codereview.webrtc.org/2695243005/diff/100001/webrtc/modules/audio_cod... IMO it's not necessary, but slightly better if the reader isn't put off by lambdas. (and they shouldn't be - lambdas are great!) https://codereview.webrtc.org/2695243005/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:133: RTC_DCHECK_LE(bitrate, kOpusMaxBitrateBps); On 2017/03/21 08:29:44, minyue-webrtc wrote: > These are more of a compile time check, i.e., > > kOpusMinBitrateBps < kOpusBitrateNbBps < kOpusMaxBitrateBps / 2, etc > > something like static_assert would fit best, but I am not sure if we can use it. Yeah. I bet these could only hit if something majorly strange was going on, and num_channels was biig. https://codereview.webrtc.org/2695243005/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:149: return *bitrate; On 2017/03/21 08:29:44, minyue-webrtc wrote: > 148 - 157 is fairly ugly. can we refactor this part? > > how about > > int chosen_bitrate = min(max(*bitrate, kOpusMinBitrateBps), kOpusMaxBitrateBps); > if (*bitrate != chosen_bitrate ) { > LOG(LS_WARNING) << > } Alright. Makes sense. https://codereview.webrtc.org/2695243005/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:180: return rtc::Optional<int>(48000); On 2017/03/21 08:29:44, minyue-webrtc wrote: > can we > > constexpr kDefaultMaxPlaybackRate = 48000 There is a kSampleRateHz above. I could use that! https://codereview.webrtc.org/2695243005/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:186: return rtc::Optional<int>(); On 2017/03/21 08:29:44, minyue-webrtc wrote: > It is hard for me to understand rtc::Optional<int>()? does that mean wrong > parsed value, then should the Get functions decide to use the default value? > > Or we can get rid of all Get functions? Yes, it means the "maxplaybackrate" option was invalid. It is used to reject bad formats in QueryAudioFormat, but I'm not sure that's completely necessary. It could just as well return 48 kHz in that case. If so, I'd just have it return an int instead. WDYT? https://codereview.webrtc.org/2695243005/diff/160001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:268: const auto ptime = GetFormatParameter<int>(format, "ptime"); On 2017/03/21 08:29:44, minyue-webrtc wrote: > add a helper function on pTime? Sure!
lgtm % comments https://codereview.webrtc.org/2695243005/diff/180001/webrtc/api/audio_codecs/... File webrtc/api/audio_codecs/audio_format.cc (right): https://codereview.webrtc.org/2695243005/diff/180001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_format.cc:84: num_channels, alignment error https://codereview.webrtc.org/2695243005/diff/180001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_format.cc:90: int num_channels, alignment error looks like quite a few of them. do clang-format https://codereview.webrtc.org/2695243005/diff/180001/webrtc/api/audio_codecs/... File webrtc/api/audio_codecs/audio_format.h (right): https://codereview.webrtc.org/2695243005/diff/180001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_format.h:78: int num_channels, needs alignment https://codereview.webrtc.org/2695243005/diff/180001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_format.h:100: RTC_DCHECK_GE(min_bitrate_bps, 0); oh, this kind of checks suggest that we may need to check it everywhere these values are used? would it make sense to make member vars private and add set functions (if they are subject to change after ctor) https://codereview.webrtc.org/2695243005/diff/180001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_format.h:107: int num_channels; size_t on countable stuff. https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/acm2/audio_coding_module_unittest.cc (right): https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/acm2/audio_coding_module_unittest.cc:1497: TEST_F(AcmSenderBitExactnessNewApi, OpusFromFormat_stereo_20ms) { need to carefully exclude newer Opus version, do the same as in "Opus_stereo_20ms" after rebasing https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/acm2/audio_coding_module_unittest.cc:1638: #define MAYBE_OpusFromFormat_48khz_20ms_100kbps \ same here https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/audio_encoder_factory.h (right): https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder_factory.h:27: // Returns a prioritized list of audio codecs, to use for signalling etc. super nit: signaling. I thought you were more American :) https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder_factory.h:31: // supported. More format and format variations may be supported than listed "listed in the list" sounds too wordy. removing "listed" should be fine https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder_factory.h:37: // identify the outputted data. outputted -> output https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder_factory.h:38: // TODO(ossu): Try to avoid audio encoders having to know their payload type. +1 https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.h (right): https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.h:19: // Creates a new factory that can create the built-in types of audio encoders. can a factory produce more than one audio encoder. The sentence sounded that way https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:171: return rtc::Optional<int>(); put default value here as well? https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:177: return std::min(*param, kSampleRateHz); kDefaultMaxPlaybackRate and kSampleRateHz are different concepts, even though they have the same value in current implementation. Please separate them https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:258: config.num_channels = GetChannelCount(format).value_or(1); try also to default in the Get function
https://codereview.webrtc.org/2695243005/diff/180001/webrtc/api/audio_codecs/... File webrtc/api/audio_codecs/audio_format.cc (right): https://codereview.webrtc.org/2695243005/diff/180001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_format.cc:84: num_channels, On 2017/03/23 11:36:52, minyue-webrtc wrote: > alignment error Acknowledged. https://codereview.webrtc.org/2695243005/diff/180001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_format.cc:90: int num_channels, On 2017/03/23 11:36:53, minyue-webrtc wrote: > alignment error > > looks like quite a few of them. do clang-format Yup, yup, yup! https://codereview.webrtc.org/2695243005/diff/180001/webrtc/api/audio_codecs/... File webrtc/api/audio_codecs/audio_format.h (right): https://codereview.webrtc.org/2695243005/diff/180001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_format.h:78: int num_channels, On 2017/03/23 11:36:53, minyue-webrtc wrote: > needs alignment Acknowledged. https://codereview.webrtc.org/2695243005/diff/180001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_format.h:100: RTC_DCHECK_GE(min_bitrate_bps, 0); On 2017/03/23 11:36:53, minyue-webrtc wrote: > oh, this kind of checks suggest that we may need to check it everywhere these > values are used? > > would it make sense to make member vars private and add set functions (if they > are subject to change after ctor) They shouldn't change after construction, and the constructor already checks this. These checks are here to ensure the comparison determining of it's a fixed bitrate codec is reasonable. https://codereview.webrtc.org/2695243005/diff/180001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_format.h:107: int num_channels; On 2017/03/23 11:36:53, minyue-webrtc wrote: > size_t on countable stuff. We don't use size_t for num_channels in AudioFormat either... maybe we should. Both CodecInst and cricket::AudioCodec has them as size_t... https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/acm2/audio_coding_module_unittest.cc (right): https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/acm2/audio_coding_module_unittest.cc:1497: TEST_F(AcmSenderBitExactnessNewApi, OpusFromFormat_stereo_20ms) { On 2017/03/23 11:36:53, minyue-webrtc wrote: > need to carefully exclude newer Opus version, do the same as in > "Opus_stereo_20ms" after rebasing Alright. Will keep that in mind. https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/audio_encoder_factory.h (right): https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder_factory.h:27: // Returns a prioritized list of audio codecs, to use for signalling etc. On 2017/03/23 11:36:53, minyue-webrtc wrote: > super nit: signaling. > > I thought you were more American :) I really do prefer doubling the consonant for -ing and -ed forms. We'll see if I change it. :) https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder_factory.h:31: // supported. More format and format variations may be supported than listed On 2017/03/23 11:36:53, minyue-webrtc wrote: > "listed in the list" sounds too wordy. > > removing "listed" should be fine You're right. Thanks! https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder_factory.h:37: // identify the outputted data. On 2017/03/23 11:36:53, minyue-webrtc wrote: > outputted -> output Maybe... I'm not happy with the description here, but I'm not happy about having to provide payload_type either. I'll reconsider the wording. https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.h (right): https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.h:19: // Creates a new factory that can create the built-in types of audio encoders. On 2017/03/23 11:36:53, minyue-webrtc wrote: > can a factory produce more than one audio encoder. The sentence sounded that way Yes. It can create more than one and of more than one type. https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:171: return rtc::Optional<int>(); On 2017/03/23 11:36:54, minyue-webrtc wrote: > put default value here as well? Depends on if we want to validate the stereo parameter and reject an invalid one, or if we'd just happily accept whatever we get and try to make do. Which do you prefer? https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:177: return std::min(*param, kSampleRateHz); On 2017/03/23 11:36:53, minyue-webrtc wrote: > kDefaultMaxPlaybackRate and kSampleRateHz are different concepts, even though > they have the same value in current implementation. Please separate them Acknowledged. https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:258: config.num_channels = GetChannelCount(format).value_or(1); On 2017/03/23 11:36:53, minyue-webrtc wrote: > try also to default in the Get function See comment above.
https://codereview.webrtc.org/2695243005/diff/180001/webrtc/api/audio_codecs/... File webrtc/api/audio_codecs/audio_format.h (right): https://codereview.webrtc.org/2695243005/diff/180001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_format.h:107: int num_channels; On 2017/03/23 12:01:59, ossu wrote: > On 2017/03/23 11:36:53, minyue-webrtc wrote: > > size_t on countable stuff. > > We don't use size_t for num_channels in AudioFormat either... maybe we should. > Both CodecInst and cricket::AudioCodec has them as size_t... IMO we use size_t a bit too much. We should use it for things like array indices and array sizes, and int pretty much everywhere else.
https://codereview.webrtc.org/2695243005/diff/180001/webrtc/api/audio_codecs/... File webrtc/api/audio_codecs/audio_format.h (right): https://codereview.webrtc.org/2695243005/diff/180001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_format.h:100: RTC_DCHECK_GE(min_bitrate_bps, 0); On 2017/03/23 12:01:59, ossu wrote: > On 2017/03/23 11:36:53, minyue-webrtc wrote: > > oh, this kind of checks suggest that we may need to check it everywhere these > > values are used? > > > > would it make sense to make member vars private and add set functions (if they > > are subject to change after ctor) > > They shouldn't change after construction, and the constructor already checks > this. These checks are here to ensure the comparison determining of it's a fixed > bitrate codec is reasonable. then can you "const" the member vars. and it looks to me that we do not need to DCHECK these again. https://codereview.webrtc.org/2695243005/diff/180001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_format.h:107: int num_channels; On 2017/03/23 12:01:59, ossu wrote: > On 2017/03/23 11:36:53, minyue-webrtc wrote: > > size_t on countable stuff. > > We don't use size_t for num_channels in AudioFormat either... maybe we should. > Both CodecInst and cricket::AudioCodec has them as size_t... There was an effort that changed all these kind to size_t in WebRTC. If size_t still makes sense, I would not divert from there. https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/acm2/audio_coding_module_unittest.cc (right): https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/acm2/audio_coding_module_unittest.cc:1497: TEST_F(AcmSenderBitExactnessNewApi, OpusFromFormat_stereo_20ms) { On 2017/03/23 12:01:59, ossu wrote: > On 2017/03/23 11:36:53, minyue-webrtc wrote: > > need to carefully exclude newer Opus version, do the same as in > > "Opus_stereo_20ms" after rebasing > > Alright. Will keep that in mind. Make sure you do it before submit, otherwise may break internal bots. https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.h (right): https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.h:19: // Creates a new factory that can create the built-in types of audio encoders. On 2017/03/23 12:01:59, ossu wrote: > On 2017/03/23 11:36:53, minyue-webrtc wrote: > > can a factory produce more than one audio encoder. The sentence sounded that > way > > Yes. It can create more than one and of more than one type. Acknowledged. https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:171: return rtc::Optional<int>(); On 2017/03/23 12:01:59, ossu wrote: > On 2017/03/23 11:36:54, minyue-webrtc wrote: > > put default value here as well? > > Depends on if we want to validate the stereo parameter and reject an invalid > one, or if we'd just happily accept whatever we get and try to make do. Which > do you prefer? I think no validation here. This is just sdp parsing. Just give a value if it does not exist. We do it anyway when calling GetChannelCount
lgtm, but see comments https://codereview.webrtc.org/2695243005/diff/180001/webrtc/api/audio_codecs/... File webrtc/api/audio_codecs/audio_format.cc (right): https://codereview.webrtc.org/2695243005/diff/180001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_format.cc:103: RTC_DCHECK_GE(max_bitrate_bps, default_bitrate_bps); You check some of the same things in the .h file. Extract to a private method? https://codereview.webrtc.org/2695243005/diff/180001/webrtc/api/audio_codecs/... File webrtc/api/audio_codecs/audio_format.h (right): https://codereview.webrtc.org/2695243005/diff/180001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_format.h:107: int num_channels; On 2017/03/23 12:20:22, minyue-webrtc wrote: > On 2017/03/23 12:01:59, ossu wrote: > > On 2017/03/23 11:36:53, minyue-webrtc wrote: > > > size_t on countable stuff. > > > > We don't use size_t for num_channels in AudioFormat either... maybe we should. > > Both CodecInst and cricket::AudioCodec has them as size_t... > > There was an effort that changed all these kind to size_t in WebRTC. If size_t > still makes sense, I would not divert from there. See https://google.github.io/styleguide/cppguide.html#Integer_Types. Specifically, it says "You should not use the unsigned integer types such as uint32_t, unless there is a valid reason such as representing a bit pattern rather than a number, or you need defined overflow modulo 2^N. In particular, do not use unsigned types to say a number will never be negative. Instead, use assertions for this." There's a subsection "On Unsigned Integers" that explains why. https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/acm2/audio_coding_module_unittest.cc (right): https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/acm2/audio_coding_module_unittest.cc:1495: static const SdpAudioFormat kOpusFormat("opus", 48000, 2, {{"stereo", "1"}}); The style guide prohibits this: https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables Make a function that returns a reference to a local const static variable? https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/acm2/audio_coding_module_unittest.cc:1499: new AudioEncoderOpus(120, kOpusFormat)); Put this on the stack instead of on the heap? (I count 4 more occurrences below.) https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/audio_encoder_factory.h (right): https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder_factory.h:37: // identify the outputted data. On 2017/03/23 12:01:59, ossu wrote: > On 2017/03/23 11:36:53, minyue-webrtc wrote: > > outputted -> output > > Maybe... I'm not happy with the description here, but I'm not happy about having > to provide payload_type either. I'll reconsider the wording. "It will tag the encoded payloads with the specified payload type." ? https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory_internal.cc (right): https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory_internal.cc:192: specs.push_back({opus_format, opus_info}); std::move(opus_format)? Or make opus_format const? https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc (right): https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc:58: return std::unique_ptr<AudioEncoder>(); A.k.a. nullptr. Or doesn't that work without an explicitly specified return type? https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory_unittest.cc (right): https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory_unittest.cc:65: audio[i] = static_cast<int16_t>(i); Overflow when casting from unsigned to signed is implementation-defined, IIRC. Consider using something like i % (std::numeric_limits<int16_t>::max() + 1) instead. https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory_unittest.cc:95: } You never test that the encoder actually returns a packet, or did I miss something? https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory_unittest.cc:136: ASSERT_TRUE(next_is_format({"pcma", 8000, 1})); Can you use ASSERT_THAT with ElementsAre() or something like that? https://github.com/google/googletest/blob/master/googlemock/docs/CheatSheet.m...
Patchset #9 (id:200001) has been deleted
Addressed a number of issues and did a rebase. https://codereview.webrtc.org/2695243005/diff/180001/webrtc/api/audio_codecs/... File webrtc/api/audio_codecs/audio_format.h (right): https://codereview.webrtc.org/2695243005/diff/180001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_format.h:100: RTC_DCHECK_GE(min_bitrate_bps, 0); On 2017/03/23 12:20:22, minyue-webrtc wrote: > On 2017/03/23 12:01:59, ossu wrote: > > On 2017/03/23 11:36:53, minyue-webrtc wrote: > > > oh, this kind of checks suggest that we may need to check it everywhere > these > > > values are used? > > > > > > would it make sense to make member vars private and add set functions (if > they > > > are subject to change after ctor) > > > > They shouldn't change after construction, and the constructor already checks > > this. These checks are here to ensure the comparison determining of it's a > fixed > > bitrate codec is reasonable. > > then can you "const" the member vars. and it looks to me that we do not need to > DCHECK these again. const is a bit impractical and makes it so that you can't copy it. Since its just a simple value type, I think you should be able to manipulate it as you wish once you've gotten one. The only sure-fire way to ensure the values aren't wrong would be to add accessors and I'm not sure the mental overhead is worth it. Checking on construction makes sense, since the code generating these should output correct data but after that, the contents are up to the user. https://codereview.webrtc.org/2695243005/diff/180001/webrtc/api/audio_codecs/... webrtc/api/audio_codecs/audio_format.h:107: int num_channels; On 2017/03/24 12:43:24, kwiberg-webrtc wrote: > On 2017/03/23 12:20:22, minyue-webrtc wrote: > > On 2017/03/23 12:01:59, ossu wrote: > > > On 2017/03/23 11:36:53, minyue-webrtc wrote: > > > > size_t on countable stuff. > > > > > > We don't use size_t for num_channels in AudioFormat either... maybe we > should. > > > Both CodecInst and cricket::AudioCodec has them as size_t... > > > > There was an effort that changed all these kind to size_t in WebRTC. If size_t > > still makes sense, I would not divert from there. > > See https://google.github.io/styleguide/cppguide.html#Integer_Types. > Specifically, it says "You should not use the unsigned integer types such as > uint32_t, unless there is a valid reason such as representing a bit pattern > rather than a number, or you need defined overflow modulo 2^N. In particular, do > not use unsigned types to say a number will never be negative. Instead, use > assertions for this." There's a subsection "On Unsigned Integers" that explains > why. I'll leave this as int for now, since there seems to be conflicting ideals here. I think it makes sense to use int for everything that doesn't have any special range requirements. https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/acm2/audio_coding_module_unittest.cc (right): https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/acm2/audio_coding_module_unittest.cc:1495: static const SdpAudioFormat kOpusFormat("opus", 48000, 2, {{"stereo", "1"}}); On 2017/03/24 12:43:24, kwiberg-webrtc wrote: > The style guide prohibits this: > https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables > > Make a function that returns a reference to a local const static variable? I don't see how this would break in practice, even if it is against the letter of the style guide. It's not visible outside of this translation unit and its construction does not depend on other static objects. Similar constructs exist in other parts of the code. I'd rather move it inside of the two tests using it, than introducing a separate function for this. https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/acm2/audio_coding_module_unittest.cc:1497: TEST_F(AcmSenderBitExactnessNewApi, OpusFromFormat_stereo_20ms) { On 2017/03/23 12:20:22, minyue-webrtc wrote: > On 2017/03/23 12:01:59, ossu wrote: > > On 2017/03/23 11:36:53, minyue-webrtc wrote: > > > need to carefully exclude newer Opus version, do the same as in > > > "Opus_stereo_20ms" after rebasing > > > > Alright. Will keep that in mind. > > Make sure you do it before submit, otherwise may break internal bots. I've added the MAYBE_ macros for the tests below. I'll make sure to run internal bots before landing. https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/acm2/audio_coding_module_unittest.cc:1499: new AudioEncoderOpus(120, kOpusFormat)); On 2017/03/24 12:43:24, kwiberg-webrtc wrote: > Put this on the stack instead of on the heap? (I count 4 more occurrences > below.) Acknowledged. https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory_internal.cc (right): https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_decoder_factory_internal.cc:192: specs.push_back({opus_format, opus_info}); On 2017/03/24 12:43:25, kwiberg-webrtc wrote: > std::move(opus_format)? Or make opus_format const? Acknowledged. https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc (right): https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory.cc:58: return std::unique_ptr<AudioEncoder>(); On 2017/03/24 12:43:25, kwiberg-webrtc wrote: > A.k.a. nullptr. Or doesn't that work without an explicitly specified return > type? Exactly. It won't deduce {nullptr} properly either, although all the hints are there. The compiler is probably right to refuse it, though. https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory_unittest.cc (right): https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory_unittest.cc:65: audio[i] = static_cast<int16_t>(i); On 2017/03/24 12:43:25, kwiberg-webrtc wrote: > Overflow when casting from unsigned to signed is implementation-defined, IIRC. > Consider using something like i % (std::numeric_limits<int16_t>::max() + 1) > instead. Acknowledged. https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory_unittest.cc:95: } On 2017/03/24 12:43:25, kwiberg-webrtc wrote: > You never test that the encoder actually returns a packet, or did I miss > something? I was thinking this check did just that, though the comment above doesn't specifically mention it. If info.encoded_bytes is never > 0, then we will fail on this ASSERT. https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/builtin_audio_encoder_factory_unittest.cc:136: ASSERT_TRUE(next_is_format({"pcma", 8000, 1})); On 2017/03/24 12:43:25, kwiberg-webrtc wrote: > Can you use ASSERT_THAT with ElementsAre() or something like that? > https://github.com/google/googletest/blob/master/googlemock/docs/CheatSheet.m... I can! And it gives a neat printout when it fails! https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2695243005/diff/180001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:171: return rtc::Optional<int>(); On 2017/03/23 12:20:22, minyue-webrtc wrote: > On 2017/03/23 12:01:59, ossu wrote: > > On 2017/03/23 11:36:54, minyue-webrtc wrote: > > > put default value here as well? > > > > Depends on if we want to validate the stereo parameter and reject an invalid > > one, or if we'd just happily accept whatever we get and try to make do. Which > > do you prefer? > > I think no validation here. This is just sdp parsing. Just give a value if it > does not exist. We do it anyway when calling GetChannelCount It's not just about it not existing, it's about it being invalid. Perhaps its better to get an error when tying "stereo=true" rather than silently getting mono. Maybe it's not? Either way, I've removed the Optional here and just default to mono.
The CQ bit was checked by ossu@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_x64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/24430)
Looks great! https://codereview.webrtc.org/2695243005/diff/240001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2695243005/diff/240001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:169: } else { nit: Add comment: When "stereo" is unset, the default is mono.
still lgtm https://codereview.webrtc.org/2695243005/diff/240001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2695243005/diff/240001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:169: } else { On 2017/04/05 15:05:45, the sun wrote: > nit: Add comment: > When "stereo" is unset, the default is mono. I liked how the old version recognized unset and "0" as mono, "1" as stereo, and returned an empty Optional in all other cases.
The CQ bit was checked by ossu@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/19282) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/24585) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/23414)
The CQ bit was checked by ossu@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_more_configs on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_more_configs/bu...)
The CQ bit was checked by ossu@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/25055)
Patchset #13 (id:300001) has been deleted
The CQ bit was checked by ossu@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ossu@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from henrik.lundin@webrtc.org, solenberg@webrtc.org, minyue@webrtc.org, kwiberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2695243005/#ps320001 (title: "Fix build problems on Windows, Android and downstream.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 320001, "attempt_start_ts": 1491498003439940,
"parent_rev": "ac4bbdf9d6618dc13356cda7b56aff1b048c2b56", "commit_rev":
"a1a040a4a492c14af068811c6e853b03f1e70b9e"}
Message was sent while issue was closed.
Description was changed from ========== Injectable audio encoders: BuiltinAudioEncoderFactory This CL contains all the changes made to audio_coding while making audio encoders injectable. Apart from some small changes to webrtcvoiceengine, nothing here is hooked up to the outside world. Those changes will be added to a follow-up CL. BUG=webrtc:5806 ========== to ========== Injectable audio encoders: BuiltinAudioEncoderFactory This CL contains all the changes made to audio_coding while making audio encoders injectable. Apart from some small changes to webrtcvoiceengine, nothing here is hooked up to the outside world. Those changes will be added to a follow-up CL. BUG=webrtc:5806 Review-Url: https://codereview.webrtc.org/2695243005 Cr-Commit-Position: refs/heads/master@{#17569} Committed: https://chromium.googlesource.com/external/webrtc/+/a1a040a4a492c14af068811c6... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:320001) as https://chromium.googlesource.com/external/webrtc/+/a1a040a4a492c14af068811c6... |
