Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2592253004:
Make a the decisions of ANA optional for the opus encoder. (Closed)
DescriptionMake a the decisions of ANA optional for the opus encoder.
BUG=webrtc:6936, webrtc:6303
Review-Url: https://codereview.webrtc.org/2592253004
Cr-Commit-Position: refs/heads/master@{#15807}
Committed: https://chromium.googlesource.com/external/webrtc/+/584c35a3341873779abc5651e25ea08d10be6f85
Patch Set 1 #
Total comments: 4
Patch Set 2 : Respond to comments. #
Total comments: 1
Messages
Total messages: 17 (7 generated)
Description was changed from ========== Make a the decisions of ANA optional for the opus encoder. BUG=webrtc:6936 ========== to ========== Make a the decisions of ANA optional for the opus encoder. BUG=webrtc:6936 ==========
michaelt@webrtc.org changed reviewers: + minyue@webrtc.org
Hi minyue, Here the same cl we talked about. I let you decide how else should review this.
nice! but see my suggestion. https://codereview.webrtc.org/2592253004/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc (right): https://codereview.webrtc.org/2592253004/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:97: void CheckEncoderRuntimeConfig( I suggest we somehow adding a new feature to this method to see that empty fields follows the old encoder mode. https://codereview.webrtc.org/2592253004/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:447: TEST(AudioEncoderOpusTest, ApplyAudioNetworkAdaptorCanHandlerNotDefinedEntrys) { Handler -> Handle NotDefined -> Empty Entrys -> Entries https://codereview.webrtc.org/2592253004/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:455: // Donne to force a call of ApplyAudioNetworkAdaptor. I suggest remove this comment.
lgtm https://codereview.webrtc.org/2592253004/diff/1/webrtc/modules/audio_coding/c... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc (right): https://codereview.webrtc.org/2592253004/diff/1/webrtc/modules/audio_coding/c... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:97: void CheckEncoderRuntimeConfig( On 2016/12/22 14:11:26, minyue-webrtc wrote: > I suggest we somehow adding a new feature to this method to see that empty > fields follows the old encoder mode. Do this void ApplyEncoderRuntimeConfig(const AudioEncoderOpus* encoder, const AudioNetworkAdaptor::EncoderRuntimeConfig& config) { int bitrate = encoder->GetTargetBitrate(); ... // Other method like ... can also trigger a new codec configuration. We use ... here encoder->On... EXPECT_EQ(config.bitrate_bps.value_or(bitrate), encoder->GetTargetBitrate()) ... }
On 2016/12/22 16:00:43, minyue-webrtc wrote: > lgtm > > https://codereview.webrtc.org/2592253004/diff/1/webrtc/modules/audio_coding/c... > File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc > (right): > > https://codereview.webrtc.org/2592253004/diff/1/webrtc/modules/audio_coding/c... > webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:97: void > CheckEncoderRuntimeConfig( > On 2016/12/22 14:11:26, minyue-webrtc wrote: > > I suggest we somehow adding a new feature to this method to see that empty > > fields follows the old encoder mode. > > Do this > > void ApplyEncoderRuntimeConfig(const AudioEncoderOpus* encoder, > const > AudioNetworkAdaptor::EncoderRuntimeConfig& config) { > > int bitrate = encoder->GetTargetBitrate(); > ... > // Other method like ... can also trigger a new codec configuration. We use > ... here > encoder->On... > EXPECT_EQ(config.bitrate_bps.value_or(bitrate), encoder->GetTargetBitrate()) > ... > } forget my comment up there. It was drafted before you uploaded the new patch, which is better than my suggestion.
minyue@webrtc.org changed reviewers: + ivoc@webrtc.org
Hi Ivo, Would you help with this small CL? We were stricter than necessary on the output of AudioNetworkAdaptor: all codec parameters were required. This CL relax the requirement.
Description was changed from ========== Make a the decisions of ANA optional for the opus encoder. BUG=webrtc:6936 ========== to ========== Make a the decisions of ANA optional for the opus encoder. BUG=webrtc:6936, webrtc:6303 ==========
lgtm with nit. https://codereview.webrtc.org/2592253004/diff/20001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc (right): https://codereview.webrtc.org/2592253004/diff/20001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:447: TEST(AudioEncoderOpusTest, EmptyConfigDontAffectEncoderSettings) { nit: Please rename to EmptyConfigDoesntAffectEncoderSettings.
On 2016/12/27 13:56:56, ivoc wrote: > lgtm with nit. > > https://codereview.webrtc.org/2592253004/diff/20001/webrtc/modules/audio_codi... > File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc > (right): > > https://codereview.webrtc.org/2592253004/diff/20001/webrtc/modules/audio_codi... > webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:447: > TEST(AudioEncoderOpusTest, EmptyConfigDontAffectEncoderSettings) { > nit: Please rename to EmptyConfigDoesntAffectEncoderSettings. Since michaelt@ owns the CL. I cannot make change on it. I'd like to commit this first and patch on a new CL,
On 2016/12/27 14:59:53, minyue-webrtc wrote: > On 2016/12/27 13:56:56, ivoc wrote: > > lgtm with nit. > > > > > https://codereview.webrtc.org/2592253004/diff/20001/webrtc/modules/audio_codi... > > File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc > > (right): > > > > > https://codereview.webrtc.org/2592253004/diff/20001/webrtc/modules/audio_codi... > > webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:447: > > TEST(AudioEncoderOpusTest, EmptyConfigDontAffectEncoderSettings) { > > nit: Please rename to EmptyConfigDoesntAffectEncoderSettings. > > Since michaelt@ owns the CL. I cannot make change on it. I'd like to commit this > first and patch on a new CL, Ok, that's fine with me, it's only a small detail anyway. lgtm.
The CQ bit was checked by minyue@webrtc.org
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": 20001, "attempt_start_ts": 1482851357143490,
"parent_rev": "e1c2d9b0a8c25d95401fe9535f411f2ce08b5e02", "commit_rev":
"584c35a3341873779abc5651e25ea08d10be6f85"}
Message was sent while issue was closed.
Description was changed from ========== Make a the decisions of ANA optional for the opus encoder. BUG=webrtc:6936, webrtc:6303 ========== to ========== Make a the decisions of ANA optional for the opus encoder. BUG=webrtc:6936, webrtc:6303 Review-Url: https://codereview.webrtc.org/2592253004 Cr-Commit-Position: refs/heads/master@{#15807} Committed: https://chromium.googlesource.com/external/webrtc/+/584c35a3341873779abc5651e... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/external/webrtc/+/584c35a3341873779abc5651e... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
