Fold AudioEncoderMutable into AudioEncoder

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.

Committed: https://crrev.com/12cfc9b4dacd6942377df1f29a64bdbec591920e
Cr-Commit-Position: refs/heads/master@{#9894}

[kwiberg-webrtc, 2015-09-07 12:57:57]

kwiberg@webrtc.org changed reviewers:
+ henrik.lundin@webrtc.org, minyue@webrtc.org

[kwiberg-webrtc, 2015-09-07 12:57:57]

The two of you have reviewed the previous incarnation of this CL here:
https://webrtc-codereview.appspot.com/50099004/

This version differs from its predecessor in two significant ways: (1) I've
fixed all the things you pointed out the last time it was up for review, and (2)
it's based on a later version of the WebRTC tree, where the iSAC AudioEncoder
and AudioDecoder are no longer the same object.

[hlundin-webrtc, 2015-09-07 20:00:02]

Looks good in general. I have a few comments and questions.

https://codereview.webrtc.org/1322973004/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/audio_encoder.h (right):

https://codereview.webrtc.org/1322973004/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/audio_encoder.h:98: virtual EncodedInfo
EncodeInternal(uint32_t rtp_timestamp,
I wouldn't mind a blank line here.

https://codereview.webrtc.org/1322973004/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.cc (right):

https://codereview.webrtc.org/1322973004/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.cc:165: void
AudioEncoderCng::Reset() {
There is state in AudioEncoderCng itself, too. Shouldn't that be reset as well?

https://codereview.webrtc.org/1322973004/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc (right):

https://codereview.webrtc.org/1322973004/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc:119:
AudioEncoderPcmA::AudioEncoderPcmA(const CodecInst& codec_inst)
With this construct, there is no way we to verify whether a codec_inst is valid
without actually creating a codec using it, which will lead to a crash if it
wasn't valid. Comments?

https://codereview.webrtc.org/1322973004/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/ilbc/audio_encoder_ilbc.cc (right):

https://codereview.webrtc.org/1322973004/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/ilbc/audio_encoder_ilbc.cc:45: :
num_10ms_frames_per_packet_(config.frame_size_ms / 10),
Why is cast gone?

https://codereview.webrtc.org/1322973004/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/ilbc/interface/audio_encoder_ilbc.h
(right):

https://codereview.webrtc.org/1322973004/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/ilbc/interface/audio_encoder_ilbc.h:57:
Config config_;
const

https://codereview.webrtc.org/1322973004/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t.h (right):

https://codereview.webrtc.org/1322973004/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t.h:33: Config();
Do you still need this one? In audio_encoder_cng.h you removed it.

https://codereview.webrtc.org/1322973004/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t_impl.h
(right):

https://codereview.webrtc.org/1322973004/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t_impl.h:177: //
Recreate the iSAC encoder instance with the given settings, and save them.
Move comment to declaration.

https://codereview.webrtc.org/1322973004/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right):

https://codereview.webrtc.org/1322973004/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:45:
DCHECK_LE(new_loss_rate, 1.0);
DCHECKs for old_loss_rate too?

[kwiberg-webrtc, 2015-09-08 10:47:45]

https://codereview.webrtc.org/1322973004/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/audio_encoder.h (right):

https://codereview.webrtc.org/1322973004/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/audio_encoder.h:98: virtual EncodedInfo
EncodeInternal(uint32_t rtp_timestamp,
On 2015/09/07 20:00:01, hlundin-webrtc wrote:
> I wouldn't mind a blank line here.

I did it this way on purpose to emphasize that these two methods are closely
related. But that's probably easy enough to see anyway... Done.

https://codereview.webrtc.org/1322973004/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.cc (right):

https://codereview.webrtc.org/1322973004/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.cc:165: void
AudioEncoderCng::Reset() {
On 2015/09/07 20:00:01, hlundin-webrtc wrote:
> There is state in AudioEncoderCng itself, too. Shouldn't that be reset as
well?

Done. This required adding reset support to the Vad class.

https://codereview.webrtc.org/1322973004/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc (right):

https://codereview.webrtc.org/1322973004/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc:119:
AudioEncoderPcmA::AudioEncoderPcmA(const CodecInst& codec_inst)
On 2015/09/07 20:00:01, hlundin-webrtc wrote:
> With this construct, there is no way we to verify whether a codec_inst is
valid
> without actually creating a codec using it, which will lead to a crash if it
> wasn't valid. Comments?

If we wanted to handle these errors, I guess the best way would be to expose a
static function in each encoder that converts CodecInst to Config. I'll probably
end up doing that while refactoring all the built-in codecs into the
factory/owner object you know as the "klump", since it'll have a function that
returns an AudioEncoder* given a CodecInst.

Changing that doesn't belong in this CL, though.

https://codereview.webrtc.org/1322973004/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/ilbc/audio_encoder_ilbc.cc (right):

https://codereview.webrtc.org/1322973004/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/ilbc/audio_encoder_ilbc.cc:45: :
num_10ms_frames_per_packet_(config.frame_size_ms / 10),
On 2015/09/07 20:00:01, hlundin-webrtc wrote:
> Why is cast gone?

It got lost in the rebase. Fixed now.

https://codereview.webrtc.org/1322973004/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/ilbc/interface/audio_encoder_ilbc.h
(right):

https://codereview.webrtc.org/1322973004/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/ilbc/interface/audio_encoder_ilbc.h:57:
Config config_;
On 2015/09/07 20:00:01, hlundin-webrtc wrote:
> const

Done.

https://codereview.webrtc.org/1322973004/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t.h (right):

https://codereview.webrtc.org/1322973004/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t.h:33: Config();
On 2015/09/07 20:00:01, hlundin-webrtc wrote:
> Do you still need this one? In audio_encoder_cng.h you removed it.

No, it seems not. Removing.

https://codereview.webrtc.org/1322973004/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t_impl.h
(right):

https://codereview.webrtc.org/1322973004/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t_impl.h:177: //
Recreate the iSAC encoder instance with the given settings, and save them.
On 2015/09/07 20:00:01, hlundin-webrtc wrote:
> Move comment to declaration.

Done.

https://codereview.webrtc.org/1322973004/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right):

https://codereview.webrtc.org/1322973004/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:45:
DCHECK_LE(new_loss_rate, 1.0);
On 2015/09/07 20:00:01, hlundin-webrtc wrote:
> DCHECKs for old_loss_rate too?

Good idea. Done.

[hlundin-webrtc, 2015-09-08 11:21:26]

Awesome work! Ship it. LGTM.

https://codereview.webrtc.org/1322973004/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.cc (right):

https://codereview.webrtc.org/1322973004/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.cc:165: void
AudioEncoderCng::Reset() {
On 2015/09/08 10:47:45, kwiberg-webrtc wrote:
> On 2015/09/07 20:00:01, hlundin-webrtc wrote:
> > There is state in AudioEncoderCng itself, too. Shouldn't that be reset as
> well?
> 
> Done. This required adding reset support to the Vad class.

Acknowledged.

https://codereview.webrtc.org/1322973004/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc (right):

https://codereview.webrtc.org/1322973004/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc:119:
AudioEncoderPcmA::AudioEncoderPcmA(const CodecInst& codec_inst)
On 2015/09/08 10:47:45, kwiberg-webrtc wrote:
> On 2015/09/07 20:00:01, hlundin-webrtc wrote:
> > With this construct, there is no way we to verify whether a codec_inst is
> valid
> > without actually creating a codec using it, which will lead to a crash if it
> > wasn't valid. Comments?
> 
> If we wanted to handle these errors, I guess the best way would be to expose a
> static function in each encoder that converts CodecInst to Config. I'll
probably
> end up doing that while refactoring all the built-in codecs into the
> factory/owner object you know as the "klump", since it'll have a function that
> returns an AudioEncoder* given a CodecInst.
> 
> Changing that doesn't belong in this CL, though.

Acknowledged.

[kwiberg-webrtc, 2015-09-08 11:24:30]

The CQ bit was checked by kwiberg@webrtc.org

[commit-bot: I haz the power, 2015-09-08 11:24:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1322973004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1322973004/80001

[commit-bot: I haz the power, 2015-09-08 11:26:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-08 11:26:34]

Try jobs failed on following builders:
  presubmit on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/667)

[kwiberg-webrtc, 2015-09-08 11:28:52]

kwiberg@webrtc.org changed reviewers:
+ andrew@webrtc.org

[kwiberg-webrtc, 2015-09-08 11:32:22]

kwiberg@webrtc.org changed reviewers:
+ tina.legrand@webrtc.org
- andrew@webrtc.org

[kwiberg-webrtc, 2015-09-08 11:34:20]

kwiberg@webrtc.org changed reviewers:
- tina.legrand@webrtc.org

[kwiberg-webrtc, 2015-09-08 12:56:25]

The CQ bit was checked by kwiberg@webrtc.org

[commit-bot: I haz the power, 2015-09-08 12:56:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1322973004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1322973004/80001

[commit-bot: I haz the power, 2015-09-08 12:57:56]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-09-08 12:58:07]

Patchset 5 (id:??) landed as
https://crrev.com/12cfc9b4dacd6942377df1f29a64bdbec591920e
Cr-Commit-Position: refs/heads/master@{#9894}