Remove the deprecated EncodeInternal interface from AudioEncoder

Remove the deprecated EncodeInternal interface from AudioEncoder

Also hid MaxEncodedBytes by making it private. It will get removed as soon as subclasses have had time to remove their overrides.

BUG=webrtc:5591
Committed: https://crrev.com/5222d315dbea8f3563c100cc9f2451907f70b05f
Cr-Commit-Position: refs/heads/master@{#12329}

[ossu, 2016-04-06 16:01:46]

Description was changed from

==========
Removed the deprecated EncodeInternal and MaxEncodedbytes from AudioEncoder.

MaxEncodedBytes now only lives on as a private method in the Opus codec.
Also did some small lint cleanups.

BUG=webrtc:5591
==========

to

==========
Removed the deprecated EncodeInternal and MaxEncodedbytes from AudioEncoder.

MaxEncodedBytes now only lives on as a private method in the Opus codec.
Also did some small lint cleanups.

BUG=webrtc:5591
==========

[ossu, 2016-04-06 16:01:46]

ossu@webrtc.org changed reviewers:
+ kwiberg@webrtc.org, solenberg@webrtc.org

[ossu, 2016-04-06 16:04:06]

This change was targeted for some time around Chrome M51. Might as well leave it
until next week before submitting, but if you have a few spare minutes, you'll
have something to review. :)

[kwiberg-webrtc, 2016-04-07 08:23:16]

https://codereview.webrtc.org/1864993002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/audio_encoder.h (left):

https://codereview.webrtc.org/1864993002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_encoder.h:61: virtual size_t
MaxEncodedBytes() const = 0;
If you go straight from having this as a virtual method to not having it at all,
users who've marked their implementations "override" will get compilation
errors. Plus, users who call this method will also get errors. Deprecation
cycle... :-)

https://codereview.webrtc.org/1864993002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng_unittest.cc
(right):

https://codereview.webrtc.org/1864993002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng_unittest.cc:91: //
preceding ones 0 bytes.
Am I reading this correctly when I conclude that this comment was actually wrong
before?

https://codereview.webrtc.org/1864993002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/mock/mock_audio_encoder.h (right):

https://codereview.webrtc.org/1864993002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/mock/mock_audio_encoder.h:58: explicit
FakeEncoding(size_t encoded_bytes);
Commendable, but unrelated to this CL. Prefer to put unrelated cleanups in
separate CLs to make reviews, reverts, code archaeology, etc. easier.

https://codereview.webrtc.org/1864993002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right):

https://codereview.webrtc.org/1864993002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:13: #include
<algorithm>
Why is this needed?

[ossu, 2016-04-07 08:52:54]

https://codereview.webrtc.org/1864993002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/audio_encoder.h (left):

https://codereview.webrtc.org/1864993002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_encoder.h:61: virtual size_t
MaxEncodedBytes() const = 0;
On 2016/04/07 08:23:16, kwiberg-webrtc wrote:
> If you go straight from having this as a virtual method to not having it at
all,
> users who've marked their implementations "override" will get compilation
> errors. Plus, users who call this method will also get errors. Deprecation
> cycle... :-)

Didn't we just have a deprecation cycle for this? I put out a message and
everything: https://groups.google.com/forum/#!topic/discuss-webrtc/HGxtfrQtxw4

Don't see what else I could've done: marking it as deprecated would've caused
compile warnings which would stop all our builds, since we treat warnings as
errors, and probably others' as well.

https://codereview.webrtc.org/1864993002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng_unittest.cc
(right):

https://codereview.webrtc.org/1864993002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng_unittest.cc:91: //
preceding ones 0 bytes.
On 2016/04/07 08:23:16, kwiberg-webrtc wrote:
> Am I reading this correctly when I conclude that this comment was actually
wrong
> before?

That's my conclusion. The code checked for kMockReturnEcodedBytes, not
kMockMaxEncodedBytes - older versions as well.

https://codereview.webrtc.org/1864993002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/mock/mock_audio_encoder.h (right):

https://codereview.webrtc.org/1864993002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/mock/mock_audio_encoder.h:58: explicit
FakeEncoding(size_t encoded_bytes);
On 2016/04/07 08:23:16, kwiberg-webrtc wrote:
> Commendable, but unrelated to this CL. Prefer to put unrelated cleanups in
> separate CLs to make reviews, reverts, code archaeology, etc. easier.

Acknowledged.

https://codereview.webrtc.org/1864993002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right):

https://codereview.webrtc.org/1864993002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:13: #include
<algorithm>
On 2016/04/07 08:23:16, kwiberg-webrtc wrote:
> Why is this needed?

It's for std::min and std::max. It's another lint issue that I might've put in a
separate CL. Would you like me to put that separate in together with the other
small lint fix?

[kwiberg-webrtc, 2016-04-07 09:53:26]

https://codereview.webrtc.org/1864993002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/audio_encoder.h (left):

https://codereview.webrtc.org/1864993002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_encoder.h:61: virtual size_t
MaxEncodedBytes() const = 0;
On 2016/04/07 08:52:53, ossu wrote:
> On 2016/04/07 08:23:16, kwiberg-webrtc wrote:
> > If you go straight from having this as a virtual method to not having it at
> all,
> > users who've marked their implementations "override" will get compilation
> > errors. Plus, users who call this method will also get errors. Deprecation
> > cycle... :-)
> 
> Didn't we just have a deprecation cycle for this? I put out a message and
> everything: https://groups.google.com/forum/#!topic/discuss-webrtc/HGxtfrQtxw4

Yes, you said it would be going away. But that mail didn't magically let
AudioEncoder implementations skip implementing MaxEncodedBytes; only after this
CL lands will they be free to do that. And if this CL removes the method rather
than deprecate it and provide a default implementation, AudioEncoder
implementations that use "override" will break. You need a second deprecation
cycle.

> Don't see what else I could've done: marking it as deprecated would've caused
> compile warnings which would stop all our builds, since we treat warnings as
> errors, and probably others' as well.

Exactly. Even if we could go back in time and do things differently, there's no
way to do this change in a single deprecation cycle. (This is the case for a lot
of API changes. Two deprecation cycles should always be sufficient, though.)

https://codereview.webrtc.org/1864993002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right):

https://codereview.webrtc.org/1864993002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:13: #include
<algorithm>
On 2016/04/07 08:52:53, ossu wrote:
> On 2016/04/07 08:23:16, kwiberg-webrtc wrote:
> > Why is this needed?
> 
> It's for std::min and std::max. It's another lint issue that I might've put in
a
> separate CL. Would you like me to put that separate in together with the other
> small lint fix?

That would be ideal. (I won't insist, though, since this one time isn't a big
deal. But there's no time like the present when it comes to establishing a new
good habit...)

[ossu, 2016-04-07 10:42:39]

https://codereview.webrtc.org/1864993002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/audio_encoder.h (left):

https://codereview.webrtc.org/1864993002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_encoder.h:61: virtual size_t
MaxEncodedBytes() const = 0;
On 2016/04/07 09:53:26, kwiberg-webrtc wrote:
> On 2016/04/07 08:52:53, ossu wrote:
> > On 2016/04/07 08:23:16, kwiberg-webrtc wrote:
> > > If you go straight from having this as a virtual method to not having it
at
> > all,
> > > users who've marked their implementations "override" will get compilation
> > > errors. Plus, users who call this method will also get errors. Deprecation
> > > cycle... :-)
> > 
> > Didn't we just have a deprecation cycle for this? I put out a message and
> > everything:
https://groups.google.com/forum/#!topic/discuss-webrtc/HGxtfrQtxw4
> 
> Yes, you said it would be going away. But that mail didn't magically let
> AudioEncoder implementations skip implementing MaxEncodedBytes; only after
this
> CL lands will they be free to do that. And if this CL removes the method
rather
> than deprecate it and provide a default implementation, AudioEncoder
> implementations that use "override" will break. You need a second deprecation
> cycle.
> 
> > Don't see what else I could've done: marking it as deprecated would've
caused
> > compile warnings which would stop all our builds, since we treat warnings as
> > errors, and probably others' as well.
> 
> Exactly. Even if we could go back in time and do things differently, there's
no
> way to do this change in a single deprecation cycle. (This is the case for a
lot
> of API changes. Two deprecation cycles should always be sufficient, though.)

Ah, right, that's true. So, if I put it back in with just an RTC_CHECK(false) as
a default implementation, maybe? Push it down into protected or private as well?
Not that it would matter much. And then put in a reply on discuss-webrtc that
the change has been made and MaxEncodedBytes should be removed from subclasses,
at least as an override.

[kwiberg-webrtc, 2016-04-07 12:31:24]

https://codereview.webrtc.org/1864993002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/audio_encoder.h (left):

https://codereview.webrtc.org/1864993002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/audio_encoder.h:61: virtual size_t
MaxEncodedBytes() const = 0;
On 2016/04/07 10:42:39, ossu wrote:
> On 2016/04/07 09:53:26, kwiberg-webrtc wrote:
> > On 2016/04/07 08:52:53, ossu wrote:
> > > On 2016/04/07 08:23:16, kwiberg-webrtc wrote:
> > > > If you go straight from having this as a virtual method to not having it
> at
> > > all,
> > > > users who've marked their implementations "override" will get
compilation
> > > > errors. Plus, users who call this method will also get errors.
Deprecation
> > > > cycle... :-)
> > > 
> > > Didn't we just have a deprecation cycle for this? I put out a message and
> > > everything:
> https://groups.google.com/forum/#!topic/discuss-webrtc/HGxtfrQtxw4
> > 
> > Yes, you said it would be going away. But that mail didn't magically let
> > AudioEncoder implementations skip implementing MaxEncodedBytes; only after
> this
> > CL lands will they be free to do that. And if this CL removes the method
> rather
> > than deprecate it and provide a default implementation, AudioEncoder
> > implementations that use "override" will break. You need a second
deprecation
> > cycle.
> > 
> > > Don't see what else I could've done: marking it as deprecated would've
> caused
> > > compile warnings which would stop all our builds, since we treat warnings
as
> > > errors, and probably others' as well.
> > 
> > Exactly. Even if we could go back in time and do things differently, there's
> no
> > way to do this change in a single deprecation cycle. (This is the case for a
> lot
> > of API changes. Two deprecation cycles should always be sufficient, though.)
> 
> Ah, right, that's true. So, if I put it back in with just an RTC_CHECK(false)
as
> a default implementation, maybe? Push it down into protected or private as
well?
> Not that it would matter much. And then put in a reply on discuss-webrtc that
> the change has been made and MaxEncodedBytes should be removed from
subclasses,
> at least as an override.

Yes, private + RTC_CHECK(false) sounds good.

[the sun, 2016-04-08 11:06:26]

lgtm

[ossu, 2016-04-08 12:14:07]

Put MaxEncodedBytes back in as a private method. Removed unrelated cleanups.

[kwiberg-webrtc, 2016-04-09 06:53:18]

lgtm

But the commit message is no longer accurate. (Also, please consider using the
imperative present tense for the subject line of the commit message, with no
period at the end. (When you change this, note that IIUC, the way Rietveld
works, you have to update both the subject AND the first line of the
description.))

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

https://codereview.webrtc.org/1864993002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/audio_encoder.h:141: // after these
subclasses have been given a change to change.
Either "as soon as" or "after", not both. :-)

[ossu, 2016-04-11 08:23:25]

Description was changed from

==========
Removed the deprecated EncodeInternal and MaxEncodedbytes from AudioEncoder.

MaxEncodedBytes now only lives on as a private method in the Opus codec.
Also did some small lint cleanups.

BUG=webrtc:5591
==========

to

==========
Removed the deprecated EncodeInternal interface from AudioEncoder

Also hid MaxEncodedBytes by making it private. It will get removed as soon as
subclasses have had time to remove their overrides.

In WebRTC, only Opus has a MaxEncodedBytes implemented, and it is only used
internally by that codec. It could have any name, really.

BUG=webrtc:5591
==========

[ossu, 2016-04-11 08:25:41]

Well, it's not a commit message yet. :)
Anyhow, it's been updated. Does Rietveld not include the subject line if it's
the same as the first line in the message, or why do both have to be updated?

[kwiberg-webrtc, 2016-04-11 08:47:57]

On 2016/04/11 08:25:41, ossu wrote:
> Well, it's not a commit message yet. :)
> Anyhow, it's been updated. Does Rietveld not include the subject line if it's
> the same as the first line in the message, or why do both have to be updated?

For some reason, Rietveld seems to copy the first line of the commit message
when you first upload the CL, and from then on maintains the CL subject separate
from the CL commit message. I've simply made a habit to always update them both.

[kwiberg-webrtc, 2016-04-11 08:50:26]

On 2016/04/11 08:25:41, ossu wrote:
> Anyhow, it's been updated.

Not completely. "Removed" -> "Remove"

(The reason I suggest this form is that it's the shortest and simplest one.)

[ossu, 2016-04-11 10:30:37]

Description was changed from

==========
Removed the deprecated EncodeInternal interface from AudioEncoder

Also hid MaxEncodedBytes by making it private. It will get removed as soon as
subclasses have had time to remove their overrides.

In WebRTC, only Opus has a MaxEncodedBytes implemented, and it is only used
internally by that codec. It could have any name, really.

BUG=webrtc:5591
==========

to

==========
Remove the deprecated EncodeInternal interface from AudioEncoder

Also hid MaxEncodedBytes by making it private. It will get removed as soon as
subclasses have had time to remove their overrides.

In WebRTC, only Opus has a MaxEncodedBytes implemented, and it is only used
internally by that codec. It could have any name, really.

BUG=webrtc:5591
==========

[ossu, 2016-04-11 10:32:19]

Aach... I'm not sure I prefer that form. It makes it sound like a request rather
than something that's actually been done. I'll go with it for now, though.

[ossu, 2016-04-11 12:07:34]

The CQ bit was checked by ossu@webrtc.org

[ossu, 2016-04-11 12:07:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
solenberg@webrtc.org, kwiberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1864993002/#ps40001 (title:
"Fixed two typos.")

[commit-bot: I haz the power, 2016-04-11 12:07:44]

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

[commit-bot: I haz the power, 2016-04-11 12:10:35]

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

[commit-bot: I haz the power, 2016-04-11 12:10:36]

Try jobs failed on following builders:
  mac_asan on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/13738)
  mac_gn_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/8440)

[ossu, 2016-04-11 12:48:11]

Description was changed from

==========
Remove the deprecated EncodeInternal interface from AudioEncoder

Also hid MaxEncodedBytes by making it private. It will get removed as soon as
subclasses have had time to remove their overrides.

In WebRTC, only Opus has a MaxEncodedBytes implemented, and it is only used
internally by that codec. It could have any name, really.

BUG=webrtc:5591
==========

to

==========
Remove the deprecated EncodeInternal interface from AudioEncoder

Also hid MaxEncodedBytes by making it private. It will get removed as soon as
subclasses have had time to remove their overrides.

BUG=webrtc:5591
==========

[ossu, 2016-04-12 10:29:14]

The CQ bit was checked by ossu@webrtc.org

[ossu, 2016-04-12 10:29:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
solenberg@webrtc.org, kwiberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1864993002/#ps60001 (title:
"AudioEncoderOpus: Renamed MaxEncodedBytes to ApproximateEncodedBytes")

[commit-bot: I haz the power, 2016-04-12 10:29:17]

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

[commit-bot: I haz the power, 2016-04-12 10:30:59]

Description was changed from

==========
Remove the deprecated EncodeInternal interface from AudioEncoder

Also hid MaxEncodedBytes by making it private. It will get removed as soon as
subclasses have had time to remove their overrides.

BUG=webrtc:5591
==========

to

==========
Remove the deprecated EncodeInternal interface from AudioEncoder

Also hid MaxEncodedBytes by making it private. It will get removed as soon as
subclasses have had time to remove their overrides.

BUG=webrtc:5591
==========

[commit-bot: I haz the power, 2016-04-12 10:31:00]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-12 10:31:08]

Description was changed from

==========
Remove the deprecated EncodeInternal interface from AudioEncoder

Also hid MaxEncodedBytes by making it private. It will get removed as soon as
subclasses have had time to remove their overrides.

BUG=webrtc:5591
==========

to

==========
Remove the deprecated EncodeInternal interface from AudioEncoder

Also hid MaxEncodedBytes by making it private. It will get removed as soon as
subclasses have had time to remove their overrides.

BUG=webrtc:5591

Committed: https://crrev.com/5222d315dbea8f3563c100cc9f2451907f70b05f
Cr-Commit-Position: refs/heads/master@{#12329}
==========

[commit-bot: I haz the power, 2016-04-12 10:31:09]

Patchset 4 (id:??) landed as
https://crrev.com/5222d315dbea8f3563c100cc9f2451907f70b05f
Cr-Commit-Position: refs/heads/master@{#12329}

[ossu, 2016-04-12 10:57:54]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.webrtc.org/1883543002/ by ossu@webrtc.org.

The reason for reverting is: Broke import. Implementations of the old interface
still exists somewhere..