WebRTC Code Reviews
Help | Chromium Project | Sign in
(10876)

Issue 50099004: Fold AudioEncoderMutable into AudioEncoder (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 5 months ago by kwiberg
Modified:
2 years, 3 months ago
Reviewers:
HL, minyue, Jelena, mkaslann12
CC:
webrtc-reviews_webrtc.org, tterriberry, HL, Tina
Base URL:
https://chromium.googlesource.com/external/webrtc.git@decoder-init-reset
Target Ref:
refs/pending/heads/master
Project:
webrtc
Visibility:
Public.

Description

Fold AudioEncoderMutable into AudioEncoder It makes more sense to combine the two interfaces, since there wasn't a clear line separating them. The result is a combined interface with just over a dozen methods, half of which need to be implemented by every subclass, while the other half have sensible (and trivial) default implementations and are implemented only by the few subclasses that need non-default behavior.

Patch Set 1 #

Total comments: 4

Patch Set 2 : improve doc wording #

Total comments: 21
Unified diffs Side-by-side diffs Delta from patch set Stats (+771 lines, -1121 lines) Patch
M webrtc/modules/audio_coding/codecs/audio_encoder.h View 1 4 chunks +74 lines, -72 lines 5 comments Download
M webrtc/modules/audio_coding/codecs/audio_encoder.cc View 2 chunks +31 lines, -5 lines 0 comments Download
D webrtc/modules/audio_coding/codecs/audio_encoder_mutable_impl.h View 1 chunk +0 lines, -95 lines 0 comments Download
M webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.cc View 3 chunks +45 lines, -20 lines 0 comments Download
M webrtc/modules/audio_coding/codecs/cng/include/audio_encoder_cng.h View 1 chunk +10 lines, -3 lines 0 comments Download
M webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc View 5 chunks +27 lines, -26 lines 0 comments Download
M webrtc/modules/audio_coding/codecs/g711/include/audio_encoder_pcm.h View 7 chunks +8 lines, -18 lines 4 comments Download
M webrtc/modules/audio_coding/codecs/g722/audio_encoder_g722.cc View 7 chunks +25 lines, -23 lines 0 comments Download
M webrtc/modules/audio_coding/codecs/g722/include/audio_encoder_g722.h View 3 chunks +9 lines, -15 lines 2 comments Download
M webrtc/modules/audio_coding/codecs/ilbc/audio_encoder_ilbc.cc View 5 chunks +32 lines, -27 lines 0 comments Download
M webrtc/modules/audio_coding/codecs/ilbc/interface/audio_encoder_ilbc.h View 1 chunk +9 lines, -15 lines 0 comments Download
M webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t.h View 5 chunks +35 lines, -29 lines 2 comments Download
M webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t_impl.h View 7 chunks +91 lines, -67 lines 2 comments Download
M webrtc/modules/audio_coding/codecs/isac/fix/interface/audio_encoder_isacfix.h View 2 chunks +0 lines, -41 lines 0 comments Download
M webrtc/modules/audio_coding/codecs/isac/fix/source/audio_encoder_isacfix.cc View 1 chunk +0 lines, -109 lines 0 comments Download
M webrtc/modules/audio_coding/codecs/isac/main/interface/audio_encoder_isac.h View 2 chunks +1 line, -42 lines 0 comments Download
M webrtc/modules/audio_coding/codecs/isac/main/source/audio_encoder_isac.cc View 1 chunk +1 line, -111 lines 0 comments Download
M webrtc/modules/audio_coding/codecs/isac/main/source/audio_encoder_isac_unittest.cc View 2 chunks +4 lines, -4 lines 0 comments Download
M webrtc/modules/audio_coding/codecs/mock/mock_audio_encoder.h View 1 chunk +6 lines, -25 lines 0 comments Download
M webrtc/modules/audio_coding/codecs/opus/audio_encoder_mutable_opus_test.cc View 3 chunks +11 lines, -13 lines 0 comments Download
M webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc View 4 chunks +132 lines, -159 lines 2 comments Download
M webrtc/modules/audio_coding/codecs/opus/interface/audio_encoder_opus.h View 3 chunks +49 lines, -43 lines 0 comments Download
M webrtc/modules/audio_coding/codecs/pcm16b/audio_encoder_pcm16b.cc View 2 chunks +15 lines, -16 lines 0 comments Download
M webrtc/modules/audio_coding/codecs/pcm16b/include/audio_encoder_pcm16b.h View 3 chunks +4 lines, -10 lines 0 comments Download
M webrtc/modules/audio_coding/codecs/red/audio_encoder_copy_red.h View 2 chunks +11 lines, -4 lines 0 comments Download
M webrtc/modules/audio_coding/codecs/red/audio_encoder_copy_red.cc View 3 chunks +42 lines, -17 lines 2 comments Download
M webrtc/modules/audio_coding/main/acm2/acm_send_test_oldapi.h View 2 chunks +2 lines, -2 lines 0 comments Download
M webrtc/modules/audio_coding/main/acm2/acm_send_test_oldapi.cc View 1 chunk +1 line, -1 line 0 comments Download
M webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.h View 1 chunk +1 line, -1 line 0 comments Download
M webrtc/modules/audio_coding/main/acm2/audio_coding_module_impl.cc View 8 chunks +13 lines, -14 lines 0 comments Download
M webrtc/modules/audio_coding/main/acm2/audio_coding_module_unittest_oldapi.cc View 3 chunks +10 lines, -11 lines 0 comments Download
M webrtc/modules/audio_coding/main/acm2/codec_manager.h View 3 chunks +1 line, -5 lines 0 comments Download
M webrtc/modules/audio_coding/main/acm2/codec_manager.cc View 4 chunks +11 lines, -13 lines 0 comments Download
M webrtc/modules/audio_coding/main/acm2/codec_owner.h View 3 chunks +7 lines, -9 lines 0 comments Download
M webrtc/modules/audio_coding/main/acm2/codec_owner.cc View 11 chunks +37 lines, -41 lines 0 comments Download
M webrtc/modules/audio_coding/main/acm2/codec_owner_unittest.cc View 1 chunk +1 line, -1 line 0 comments Download
M webrtc/modules/audio_coding/main/interface/audio_coding_module.h View 2 chunks +2 lines, -2 lines 0 comments Download
M webrtc/modules/audio_coding/neteq/audio_decoder_impl.cc View 1 chunk +4 lines, -4 lines 0 comments Download
M webrtc/modules/audio_coding/neteq/audio_decoder_unittest.cc View 3 chunks +9 lines, -8 lines 2 comments Download
Trybot results:
Project "webrtc" does not have a commit queue.

Messages

Total messages: 21 (2 generated)
kwiberg
This is just a preview for now, since several CLs need to go in before ...
4 years, 5 months ago (2015-05-26 11:31:30 UTC) #2
Jelena
lgtm https://webrtc-codereview.appspot.com/50099004/diff/1/webrtc/modules/audio_coding/codecs/audio_encoder.cc File webrtc/modules/audio_coding/codecs/audio_encoder.cc (right): https://webrtc-codereview.appspot.com/50099004/diff/1/webrtc/modules/audio_coding/codecs/audio_encoder.cc#newcode38 webrtc/modules/audio_coding/codecs/audio_encoder.cc:38: return !enable; Maybe add a comment to explain ...
4 years, 5 months ago (2015-05-26 12:38:22 UTC) #3
kwiberg
https://webrtc-codereview.appspot.com/50099004/diff/1/webrtc/modules/audio_coding/codecs/audio_encoder.cc File webrtc/modules/audio_coding/codecs/audio_encoder.cc (right): https://webrtc-codereview.appspot.com/50099004/diff/1/webrtc/modules/audio_coding/codecs/audio_encoder.cc#newcode38 webrtc/modules/audio_coding/codecs/audio_encoder.cc:38: return !enable; On 2015/05/26 12:38:22, Jelena wrote: > Maybe ...
4 years, 5 months ago (2015-05-26 13:03:08 UTC) #4
kwiberg
Tried to improve wording.
4 years, 5 months ago (2015-05-26 14:05:58 UTC) #5
Jelena
https://webrtc-codereview.appspot.com/50099004/diff/20001/webrtc/modules/audio_coding/codecs/audio_encoder.h File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://webrtc-codereview.appspot.com/50099004/diff/20001/webrtc/modules/audio_coding/codecs/audio_encoder.h#newcode106 webrtc/modules/audio_coding/codecs/audio_encoder.h:106: virtual bool SetFec(bool enable); Better, thanks
4 years, 5 months ago (2015-05-26 14:12:49 UTC) #6
minyue
Thank you for the CL. I found that they fashion of struct Config and how ...
4 years, 5 months ago (2015-05-27 08:56:03 UTC) #7
HL
Two comments and a question. Otherwise looks good. https://webrtc-codereview.appspot.com/50099004/diff/20001/webrtc/modules/audio_coding/codecs/audio_encoder.h File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://webrtc-codereview.appspot.com/50099004/diff/20001/webrtc/modules/audio_coding/codecs/audio_encoder.h#newcode141 webrtc/modules/audio_coding/codecs/audio_encoder.h:141: virtual ...
4 years, 5 months ago (2015-05-27 13:22:12 UTC) #8
kwiberg
On 2015/05/27 08:56:03, minyue wrote: > I found that they fashion of struct Config and ...
4 years, 5 months ago (2015-05-27 14:08:32 UTC) #9
minyue
On 2015/05/27 14:08:32, kwiberg wrote: > On 2015/05/27 08:56:03, minyue wrote: > > I found ...
4 years, 5 months ago (2015-05-27 14:21:46 UTC) #10
kwiberg
Minyue, I'll look into using Config as a member variable in more cases, as you ...
4 years, 5 months ago (2015-05-27 14:36:46 UTC) #11
kwiberg
On 2015/05/27 14:21:46, minyue wrote: > On 2015/05/27 14:08:32, kwiberg wrote: > > On 2015/05/27 ...
4 years, 5 months ago (2015-05-27 14:40:20 UTC) #12
minyue
On 2015/05/27 14:40:20, kwiberg wrote: > On 2015/05/27 14:21:46, minyue wrote: > > On 2015/05/27 ...
4 years, 5 months ago (2015-05-28 14:34:33 UTC) #13
kwiberg
On 2015/05/28 14:34:33, minyue wrote: > On 2015/05/27 14:40:20, kwiberg wrote: > > On 2015/05/27 ...
4 years, 5 months ago (2015-05-28 14:45:36 UTC) #14
kwiberg
https://webrtc-codereview.appspot.com/50099004/diff/20001/webrtc/modules/audio_coding/codecs/audio_encoder.h File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://webrtc-codereview.appspot.com/50099004/diff/20001/webrtc/modules/audio_coding/codecs/audio_encoder.h#newcode141 webrtc/modules/audio_coding/codecs/audio_encoder.h:141: virtual void SetMaxBitrate(int max_bps); On 2015/05/27 13:22:12, HL wrote: ...
4 years, 5 months ago (2015-05-28 15:23:46 UTC) #15
minyue
On 2015/05/28 14:45:36, kwiberg wrote: > On 2015/05/28 14:34:33, minyue wrote: > > On 2015/05/27 ...
4 years, 5 months ago (2015-05-28 16:53:30 UTC) #16
kwiberg
On 2015/05/28 16:53:30, minyue wrote: > I might have just made this sound too complicated. ...
4 years, 5 months ago (2015-05-28 22:43:00 UTC) #17
minyue
On 2015/05/28 22:43:00, kwiberg wrote: > On 2015/05/28 16:53:30, minyue wrote: > > I might ...
4 years, 5 months ago (2015-06-01 09:19:35 UTC) #18
kwiberg
This CL lives here now: https://codereview.webrtc.org/1322973004/
4 years, 2 months ago (2015-09-07 13:00:09 UTC) #19
mkaslann12
2 years, 3 months ago (2017-08-16 15:18:52 UTC) #21
Message was sent while issue was closed.
lgtm

Http://www.moneygram.com
$5.00
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 245c2c2-tainted