Changed AudioEncoder::Encode to take an rtc::Buffer* instead of uint8_t* and a maximum size.

Changed AudioEncoder::Encode to take an rtc::Buffer* instead of uint8_t* and a maximum size.

For backwards compatibility, I've added kept the old interface to
Encode() and EncodeInternal and created default implementations of both
variants of EncodeInternal(), each calling the other. At least one of
the variants must be implemented in a subclass or we'll run out of stack
and explode. Would be nice if we could catch that before runtime. :/

The new interface to EncodeInternal() is protected, since it should
never be called from the outside.

Was unable to mark the old EncodeInternal() as RTC_DEPRECATED, since the
default implementaion of the new variant needs to call it to work around
old implementations. The old Encode() variant is deprecated, at least.

Added a test for backwards compatibility in audio_encoder_unittest.cc.
For the added test I broke out MockEncodeHelper from
audio_encoder_copy_red_unittest.cc and renamed it MockAudioEncoderHelper.

Committed: https://crrev.com/10a029e95216c568d9bba9714e52edd761cf9054
Cr-Commit-Position: refs/heads/master@{#11823}

[ossu, 2016-02-24 13:20:19]

Description was changed from

==========
Changed AudioEncoder::Encode to take an rtc::Buffer* instead of uint8_t* and a
maximum size.

For backwards compatibility, I've added kept the old interface to
Encode() and EncodeInternal and created default implementations of both
variants of EncodeInternal(), each calling the other. At least one of
the variants must be implemented in a subclass or we'll run out of stack
and explode. Would be nice if we could catch that before runtime. :/

The new interface to EncodeInternal() is protected, since it should
never be called from the outside.

Was unable to mark the old EncodeInternal() as RTC_DEPRECATED, since the
default implementaion of the new variant needs to call it to work around
old implementations. The old Encode() variant is deprecated, at least.

Added a test for backwards compatibility in audio_encoder_unittest.cc.
For the added test I broke out MockEncodeHelper from
audio_encoder_copy_red_unittest.cc and renamed it MockAudioEncoderHelper.
==========

to

==========
Changed AudioEncoder::Encode to take an rtc::Buffer* instead of uint8_t* and a
maximum size.

For backwards compatibility, I've added kept the old interface to
Encode() and EncodeInternal and created default implementations of both
variants of EncodeInternal(), each calling the other. At least one of
the variants must be implemented in a subclass or we'll run out of stack
and explode. Would be nice if we could catch that before runtime. :/

The new interface to EncodeInternal() is protected, since it should
never be called from the outside.

Was unable to mark the old EncodeInternal() as RTC_DEPRECATED, since the
default implementaion of the new variant needs to call it to work around
old implementations. The old Encode() variant is deprecated, at least.

Added a test for backwards compatibility in audio_encoder_unittest.cc.
For the added test I broke out MockEncodeHelper from
audio_encoder_copy_red_unittest.cc and renamed it MockAudioEncoderHelper.
==========

[ossu, 2016-02-24 13:20:19]

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

[ossu, 2016-02-24 13:23:00]

ossu@webrtc.org changed reviewers:
+ turaj@webrtc.org

[ossu, 2016-02-24 13:24:34]

Please take a look at these changes. Should anyone else review this as well?

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/audio_encoder_unittest.cc (right):

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/audio_encoder_unittest.cc:2: *  Copyright (c)
2015 The WebRTC project authors. All Rights Reserved.
I should change this to 2016.

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/g722/audio_encoder_g722.cc (right):

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/g722/audio_encoder_g722.cc:117: const size_t
bytes_encoded = WebRtcG722_Encode(
This used to shadow the input parameter encoded, which didn't seem right to me.

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t_impl.h (left):

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t_impl.h:137:
RTC_CHECK_LE(static_cast<size_t>(r), max_encoded_bytes);
This check should now be covered by the overrun check in AppendData().

[kwiberg-webrtc, 2016-02-25 00:29:04]

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/acm2/audio_coding_module_unittest_oldapi.cc
(right):

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/acm2/audio_coding_module_unittest_oldapi.cc:1636:
.WillRepeatedly(Invoke(&encoder, &AudioEncoderPcmU::MaxEncodedBytes));
Do you still need this one?

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

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/audio_encoder.cc:33: // ossu: EncodeInternal
in implementations. Should this not suffice?
Well, maybe. But having an assert close to the code serves as documentation for
that code, and also provides protection in case an assert far away (like this
one) is changed or lost. So they're probably useful, I'd say.

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/audio_encoder.cc:39: return info;
You can eliminate the local variable and turn these 3 statements into 1...
except you can't do that if you follow my other piece of advice for these lines,
which is to CHECK that the buffer has grown by at most MaxEncodedBytes() bytes.
The reason for doing this is that otherwise new-style encode calls won't break
for encoders with broken MaxEncodedBytes methods.

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

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/audio_encoder.h:103: RTC_DEPRECATED
EncodedInfo Encode(uint32_t rtp_timestamp,
Before landing this, add a bug reference and a date for removal (see e.g.
buffer.h for an example).

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/audio_encoder_unittest.cc (right):

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/audio_encoder_unittest.cc:2: *  Copyright (c)
2015 The WebRTC project authors. All Rights Reserved.
On 2016/02/24 13:24:33, ossu wrote:
> I should change this to 2016.

Acknowledged.

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/audio_encoder_unittest.cc:28: rtc::Buffer
output_buffer;
Please don't declare variables so far away from their first point of use unless
there's a good reason for it.

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/audio_encoder_unittest.cc:36:
MockAudioEncoderBase* impls[2] = { &old_impl, &new_impl };
You can drop the explicit 2.

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/audio_encoder_unittest.cc:46:
.WillRepeatedly(Invoke(&helper,
WillOnce?

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/audio_encoder_unittest.cc:55:
rtc::ArrayView<const int16_t> audio_view(raw_audio, 80);
IIRC, ArrayView has a constructor for C arrays that infers the length. Also, the
ArrayView constructors are not explicit, so you can just pass raw_audio in the
calls below and it will all work.

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/audio_encoder_unittest.cc:67:
EXPECT_EQ(output_buffer.data()[i], payload[i]);
Hmm, Buffer doesn't have an operator[]? Are you up for some community service?
;-)

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

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc:100:
encoded.data());
Wouldn't it be better to change EncodeCall to also take a Buffer*, and do the
AppendData in there?

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/g722/audio_encoder_g722.cc (right):

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/g722/audio_encoder_g722.cc:117: const size_t
bytes_encoded = WebRtcG722_Encode(
On 2016/02/24 13:24:33, ossu wrote:
> This used to shadow the input parameter encoded, which didn't seem right to
me.

Acknowledged.

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/g722/audio_encoder_g722.cc:145:
info.encoded_bytes = bytes_to_encode;
Have the AppendData call here and set .encoded_bytes to its return value?

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t_impl.h (left):

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t_impl.h:137:
RTC_CHECK_LE(static_cast<size_t>(r), max_encoded_bytes);
On 2016/02/24 13:24:34, ossu wrote:
> This check should now be covered by the overrun check in AppendData().

Acknowledged.

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

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t_impl.h:138: <<
T::GetErrorCode(isac_state_) << ")";
Indentation looks off. Did you run clang-format?

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t_impl.h:140: return
(r >= 0) ? static_cast<size_t>(r) : 0;
Just static_cast<size_t>(r) will do, because of the CHECK on the previous line.

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/mock/mock_audio_encoder.h (right):

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/mock/mock_audio_encoder.h:64:
MockAudioEncoderHelper() : write_payload_(false), payload_(NULL) {
nullptr

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

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:149: size_t
max_encoded_bytes = MaxEncodedBytes();
const

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:163: return
(status >= 0) ? static_cast<size_t>(status) : 0;
Just static_cast<size_t>(status) because of the CHECK on the previous line.

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/red/audio_encoder_copy_red.cc (right):

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/red/audio_encoder_copy_red.cc:60: size_t
primary_offset = encoded->size();
const

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/audio_decoder_unittest.cc (right):

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/audio_decoder_unittest.cc:118:
encoded_.Clear();
Can you allocate a Buffer on the stack for each test instead? Reducing the state
is always good.

[kwiberg-webrtc, 2016-02-25 09:23:36]

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

[kwiberg-webrtc, 2016-02-25 09:25:02]

On 2016/02/24 13:24:34, ossu wrote:
> Please take a look at these changes. Should anyone else review this as well?

Henrik and I coauthored the AudioEncoder interface, so he might have something
to say.

[ossu, 2016-02-25 10:39:52]

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/acm2/audio_coding_module_unittest_oldapi.cc
(right):

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/acm2/audio_coding_module_unittest_oldapi.cc:1636:
.WillRepeatedly(Invoke(&encoder, &AudioEncoderPcmU::MaxEncodedBytes));
On 2016/02/25 00:29:04, kwiberg-webrtc wrote:
> Do you still need this one?

Hmm... Come to think of it, probably not. I'll try and remove it.

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

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/audio_encoder.cc:39: return info;
On 2016/02/25 00:29:04, kwiberg-webrtc wrote:
> You can eliminate the local variable and turn these 3 statements into 1...
> except you can't do that if you follow my other piece of advice for these
lines,
> which is to CHECK that the buffer has grown by at most MaxEncodedBytes()
bytes.
> The reason for doing this is that otherwise new-style encode calls won't break
> for encoders with broken MaxEncodedBytes methods.

I believe I split this up while debugging and never put it back. I'm not sure we
need an extra check here, though. The AppendData call in the
backwards-compatible EncodeInternal(_, _, Buffer *) already does that check. As
with your previous comment, though, adding a check here would work as
documentation of our expectations. Is the extra doc-ing worth the redundant
CHECK call, or should I just document that this check is performed elsewhere
with a comment?

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

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/audio_encoder.h:103: RTC_DEPRECATED
EncodedInfo Encode(uint32_t rtp_timestamp,
On 2016/02/25 00:29:04, kwiberg-webrtc wrote:
> Before landing this, add a bug reference and a date for removal (see e.g.
> buffer.h for an example).

Acknowledged.

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/audio_encoder_unittest.cc (right):

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/audio_encoder_unittest.cc:28: rtc::Buffer
output_buffer;
On 2016/02/25 00:29:04, kwiberg-webrtc wrote:
> Please don't declare variables so far away from their first point of use
unless
> there's a good reason for it.

Acknowledged.

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/audio_encoder_unittest.cc:36:
MockAudioEncoderBase* impls[2] = { &old_impl, &new_impl };
On 2016/02/25 00:29:04, kwiberg-webrtc wrote:
> You can drop the explicit 2.

Acknowledged.

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/audio_encoder_unittest.cc:46:
.WillRepeatedly(Invoke(&helper,
On 2016/02/25 00:29:04, kwiberg-webrtc wrote:
> WillOnce?

Yeah, that makes sense. The same for the one below, I presume.

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/audio_encoder_unittest.cc:55:
rtc::ArrayView<const int16_t> audio_view(raw_audio, 80);
On 2016/02/25 00:29:04, kwiberg-webrtc wrote:
> IIRC, ArrayView has a constructor for C arrays that infers the length. Also,
the
> ArrayView constructors are not explicit, so you can just pass raw_audio in the
> calls below and it will all work.

Acknowledged.

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/audio_encoder_unittest.cc:67:
EXPECT_EQ(output_buffer.data()[i], payload[i]);
On 2016/02/25 00:29:04, kwiberg-webrtc wrote:
> Hmm, Buffer doesn't have an operator[]? Are you up for some community service?
> ;-)

I am! However, I guess there might be an issue with picking types (well,
signedness) that's available in the other accessor functions: data<T>,
SetData<T>, etc... operator[] would not be (reasonably) templatable, however we
could allow it as a shorthand when working with uint8_t, which I guess is the
main use case anyway.

I'll put in a separate CL for that.

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

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc:100:
encoded.data());
On 2016/02/25 00:29:04, kwiberg-webrtc wrote:
> Wouldn't it be better to change EncodeCall to also take a Buffer*, and do the
> AppendData in there?

I thought about it but decided that EncodeCall is basically just an alias for
the underlying C-style function that performs the actual encoding. Rather than
duplicating the AppendData in each implementation, it made sense to keep it
here.

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/g722/audio_encoder_g722.cc (right):

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/g722/audio_encoder_g722.cc:145:
info.encoded_bytes = bytes_to_encode;
On 2016/02/25 00:29:04, kwiberg-webrtc wrote:
> Have the AppendData call here and set .encoded_bytes to its return value?

Yeah.

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

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t_impl.h:138: <<
T::GetErrorCode(isac_state_) << ")";
On 2016/02/25 00:29:04, kwiberg-webrtc wrote:
> Indentation looks off. Did you run clang-format?

I did not, though I've been using google-c-style.el. Will check!

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/isac/audio_encoder_isac_t_impl.h:140: return
(r >= 0) ? static_cast<size_t>(r) : 0;
On 2016/02/25 00:29:04, kwiberg-webrtc wrote:
> Just static_cast<size_t>(r) will do, because of the CHECK on the previous
line.

Acknowledged.

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/mock/mock_audio_encoder.h (right):

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/mock/mock_audio_encoder.h:64:
MockAudioEncoderHelper() : write_payload_(false), payload_(NULL) {
On 2016/02/25 00:29:04, kwiberg-webrtc wrote:
> nullptr

Acknowledged.

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

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:163: return
(status >= 0) ? static_cast<size_t>(status) : 0;
On 2016/02/25 00:29:04, kwiberg-webrtc wrote:
> Just static_cast<size_t>(status) because of the CHECK on the previous line.

Acknowledged.

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/red/audio_encoder_copy_red.cc (right):

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/red/audio_encoder_copy_red.cc:60: size_t
primary_offset = encoded->size();
On 2016/02/25 00:29:04, kwiberg-webrtc wrote:
> const

Acknowledged.

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/audio_decoder_unittest.cc (right):

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/audio_decoder_unittest.cc:118:
encoded_.Clear();
On 2016/02/25 00:29:04, kwiberg-webrtc wrote:
> Can you allocate a Buffer on the stack for each test instead? Reducing the
state
> is always good.

Sure.

[kwiberg-webrtc, 2016-02-25 12:24:25]

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

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/audio_encoder.cc:39: return info;
On 2016/02/25 10:39:51, ossu wrote:
> On 2016/02/25 00:29:04, kwiberg-webrtc wrote:
> > You can eliminate the local variable and turn these 3 statements into 1...
> > except you can't do that if you follow my other piece of advice for these
> lines,
> > which is to CHECK that the buffer has grown by at most MaxEncodedBytes()
> bytes.
> > The reason for doing this is that otherwise new-style encode calls won't
break
> > for encoders with broken MaxEncodedBytes methods.
> 
> I believe I split this up while debugging and never put it back. I'm not sure
we
> need an extra check here, though. The AppendData call in the
> backwards-compatible EncodeInternal(_, _, Buffer *) already does that check.
As
> with your previous comment, though, adding a check here would work as
> documentation of our expectations. Is the extra doc-ing worth the redundant
> CHECK call, or should I just document that this check is performed elsewhere
> with a comment?

The backwards-compatible EncodeInternal is only called when a new-style encode
call is made to an old-style encoder. For the new/new case, MaxEncodedBytes is
never run, so encoders that are only tested in new/new mode can have a broken
MaxEncodedBytes and no one will notice until someone makes an old-style encode
call to it.

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/audio_encoder_unittest.cc (right):

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/audio_encoder_unittest.cc:46:
.WillRepeatedly(Invoke(&helper,
On 2016/02/25 10:39:51, ossu wrote:
> On 2016/02/25 00:29:04, kwiberg-webrtc wrote:
> > WillOnce?
> 
> Yeah, that makes sense. The same for the one below, I presume.

Yes.

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/audio_encoder_unittest.cc:67:
EXPECT_EQ(output_buffer.data()[i], payload[i]);
On 2016/02/25 10:39:51, ossu wrote:
> On 2016/02/25 00:29:04, kwiberg-webrtc wrote:
> > Hmm, Buffer doesn't have an operator[]? Are you up for some community
service?
> > ;-)
> 
> I am! However, I guess there might be an issue with picking types (well,
> signedness) that's available in the other accessor functions: data<T>,
> SetData<T>, etc... operator[] would not be (reasonably) templatable, however
we
> could allow it as a shorthand when working with uint8_t, which I guess is the
> main use case anyway.

Good point---I didn't think of that. Having it for just uint8_t makes sense,
though, as that's the "main" element type for Buffer.

> I'll put in a separate CL for that.

Excellent!

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

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc:100:
encoded.data());
On 2016/02/25 10:39:51, ossu wrote:
> On 2016/02/25 00:29:04, kwiberg-webrtc wrote:
> > Wouldn't it be better to change EncodeCall to also take a Buffer*, and do
the
> > AppendData in there?
> 
> I thought about it but decided that EncodeCall is basically just an alias for
> the underlying C-style function that performs the actual encoding. Rather than
> duplicating the AppendData in each implementation, it made sense to keep it
> here.

It will stop making sense when you have to keep the MaxEncodedBytes function
just to support this call, I believe.

[ossu, 2016-02-25 15:58:46]

A whole slew of small updates.
The biggest thing is I rewrote MockAudioEncoderHelper, since it was starting to
become a hassle to keep state manually between calls. The added check in
EncodeInternal caused a number of tests to explode, since the buffer wasn't
adjusted properly in the mocks.

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

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/audio_encoder.cc:33: // ossu: EncodeInternal
in implementations. Should this not suffice?
On 2016/02/25 00:29:04, kwiberg-webrtc wrote:
> Well, maybe. But having an assert close to the code serves as documentation
for
> that code, and also provides protection in case an assert far away (like this
> one) is changed or lost. So they're probably useful, I'd say.

Acknowledged.

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/audio_encoder.cc:39: return info;
On 2016/02/25 12:24:25, kwiberg-webrtc wrote:
> On 2016/02/25 10:39:51, ossu wrote:
> > On 2016/02/25 00:29:04, kwiberg-webrtc wrote:
> > > You can eliminate the local variable and turn these 3 statements into 1...
> > > except you can't do that if you follow my other piece of advice for these
> > lines,
> > > which is to CHECK that the buffer has grown by at most MaxEncodedBytes()
> > bytes.
> > > The reason for doing this is that otherwise new-style encode calls won't
> break
> > > for encoders with broken MaxEncodedBytes methods.
> > 
> > I believe I split this up while debugging and never put it back. I'm not
sure
> we
> > need an extra check here, though. The AppendData call in the
> > backwards-compatible EncodeInternal(_, _, Buffer *) already does that check.
> As
> > with your previous comment, though, adding a check here would work as
> > documentation of our expectations. Is the extra doc-ing worth the redundant
> > CHECK call, or should I just document that this check is performed elsewhere
> > with a comment?
> 
> The backwards-compatible EncodeInternal is only called when a new-style encode
> call is made to an old-style encoder. For the new/new case, MaxEncodedBytes is
> never run, so encoders that are only tested in new/new mode can have a broken
> MaxEncodedBytes and no one will notice until someone makes an old-style encode
> call to it.

Ah, yes. Right you are!

[kwiberg-webrtc, 2016-02-27 19:21:46]

It’s converging! I’ve almost run out of things to complain about. :-)

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

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc:100:
encoded.data());
On 2016/02/25 12:24:25, kwiberg-webrtc wrote:
> On 2016/02/25 10:39:51, ossu wrote:
> > On 2016/02/25 00:29:04, kwiberg-webrtc wrote:
> > > Wouldn't it be better to change EncodeCall to also take a Buffer*, and do
> the
> > > AppendData in there?
> > 
> > I thought about it but decided that EncodeCall is basically just an alias
for
> > the underlying C-style function that performs the actual encoding. Rather
than
> > duplicating the AppendData in each implementation, it made sense to keep it
> > here.
> 
> It will stop making sense when you have to keep the MaxEncodedBytes function
> just to support this call, I believe.

You never answered this one.

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

https://codereview.webrtc.org/1725143003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/audio_encoder.h:98: // Deprecated interface
to Encode (remove eventually, bug 5591). May incur a
Set a date! You'll be landing this in March, so July should be good. Or did we
consider removing them sooner?

https://codereview.webrtc.org/1725143003/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/mock/mock_audio_encoder.h (right):

https://codereview.webrtc.org/1725143003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/mock/mock_audio_encoder.h:70: namespace
MockAudioEncoderHelper {
Why a namespace? And if you do end up having one after all, see
https://google.github.io/styleguide/cppguide.html#Namespace_Names

https://codereview.webrtc.org/1725143003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/mock/mock_audio_encoder.h:72:
std::function<AudioEncoder::EncodedInfo(uint32_t timestamp,
Bad news: std::function is not yet allowed. https://chromium-cpp.appspot.com/

I dunno if using the actual lambda types is an alternative, or if a more radical
restructuring is called for.

[ossu, 2016-02-29 08:51:24]

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

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc:100:
encoded.data());
On 2016/02/27 19:21:46, kwiberg-webrtc wrote:
> On 2016/02/25 12:24:25, kwiberg-webrtc wrote:
> > On 2016/02/25 10:39:51, ossu wrote:
> > > On 2016/02/25 00:29:04, kwiberg-webrtc wrote:
> > > > Wouldn't it be better to change EncodeCall to also take a Buffer*, and
do
> > the
> > > > AppendData in there?
> > > 
> > > I thought about it but decided that EncodeCall is basically just an alias
> for
> > > the underlying C-style function that performs the actual encoding. Rather
> than
> > > duplicating the AppendData in each implementation, it made sense to keep
it
> > > here.
> > 
> > It will stop making sense when you have to keep the MaxEncodedBytes function
> > just to support this call, I believe.
> 
> You never answered this one.

It wasn't a question. Also, I didn't understand it. MaxEncodedBytes is not
required for keeping it this way.

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

https://codereview.webrtc.org/1725143003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/audio_encoder.h:98: // Deprecated interface
to Encode (remove eventually, bug 5591). May incur a
On 2016/02/27 19:21:46, kwiberg-webrtc wrote:
> Set a date! You'll be landing this in March, so July should be good. Or did we
> consider removing them sooner?

See bug 5591. :)
We've set it for milestone 51, if no-one complains. Keeping the deadline in one
place (i.e. the bug) should make it easier to update it if anything changes.

https://codereview.webrtc.org/1725143003/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/mock/mock_audio_encoder.h (right):

https://codereview.webrtc.org/1725143003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/mock/mock_audio_encoder.h:70: namespace
MockAudioEncoderHelper {
On 2016/02/27 19:21:46, kwiberg-webrtc wrote:
> Why a namespace? And if you do end up having one after all, see
> https://google.github.io/styleguide/cppguide.html#Namespace_Names

Namespace because it's no longer a class. The name, well, because it's the same
name it had before. I'll rework it - the name's too long anyway.

https://codereview.webrtc.org/1725143003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/mock/mock_audio_encoder.h:72:
std::function<AudioEncoder::EncodedInfo(uint32_t timestamp,
On 2016/02/27 19:21:46, kwiberg-webrtc wrote:
> Bad news: std::function is not yet allowed. https://chromium-cpp.appspot.com/
> 
> I dunno if using the actual lambda types is an alternative, or if a more
radical
> restructuring is called for.

Awww, daaang. :/
Well, I'll see what I can do. Not sure getting the type of the lambda function
is actually possible in this case. Making them regular functors should work, but
is a bit of a hassle.

[kwiberg-webrtc, 2016-02-29 11:42:47]

lgtm

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

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc:100:
encoded.data());
On 2016/02/29 08:51:23, ossu wrote:
> On 2016/02/27 19:21:46, kwiberg-webrtc wrote:
> > On 2016/02/25 12:24:25, kwiberg-webrtc wrote:
> > > On 2016/02/25 10:39:51, ossu wrote:
> > > > On 2016/02/25 00:29:04, kwiberg-webrtc wrote:
> > > > > Wouldn't it be better to change EncodeCall to also take a Buffer*, and
> do
> > > the
> > > > > AppendData in there?
> > > > 
> > > > I thought about it but decided that EncodeCall is basically just an
alias
> > for
> > > > the underlying C-style function that performs the actual encoding.
Rather
> > than
> > > > duplicating the AppendData in each implementation, it made sense to keep
> it
> > > > here.
> > > 
> > > It will stop making sense when you have to keep the MaxEncodedBytes
function
> > > just to support this call, I believe.
> > 
> > You never answered this one.
> 
> It wasn't a question. Also, I didn't understand it. MaxEncodedBytes is not
> required for keeping it this way.

Well, to call AppendData you need *some* encoder-specific function that returns
the maximum number of bytes the encoder will produce. Right now, you can use
MaxEncodedBytes. When you remove MaxEncodedBytes, you'll have to use something
else. Or pass a Buffer* to EncodeCall as I suggested.

There's no harm in leaving it like this for now, though, I suppose.

[kwiberg-webrtc, 2016-02-29 11:42:54]

lgtm

[ossu, 2016-02-29 11:56:52]

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

https://codereview.webrtc.org/1725143003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/g711/audio_encoder_pcm.cc:100:
encoded.data());
On 2016/02/29 11:42:47, kwiberg-webrtc wrote:
> On 2016/02/29 08:51:23, ossu wrote:
> > On 2016/02/27 19:21:46, kwiberg-webrtc wrote:
> > > On 2016/02/25 12:24:25, kwiberg-webrtc wrote:
> > > > On 2016/02/25 10:39:51, ossu wrote:
> > > > > On 2016/02/25 00:29:04, kwiberg-webrtc wrote:
> > > > > > Wouldn't it be better to change EncodeCall to also take a Buffer*,
and
> > do
> > > > the
> > > > > > AppendData in there?
> > > > > 
> > > > > I thought about it but decided that EncodeCall is basically just an
> alias
> > > for
> > > > > the underlying C-style function that performs the actual encoding.
> Rather
> > > than
> > > > > duplicating the AppendData in each implementation, it made sense to
keep
> > it
> > > > > here.
> > > > 
> > > > It will stop making sense when you have to keep the MaxEncodedBytes
> function
> > > > just to support this call, I believe.
> > > 
> > > You never answered this one.
> > 
> > It wasn't a question. Also, I didn't understand it. MaxEncodedBytes is not
> > required for keeping it this way.
> 
> Well, to call AppendData you need *some* encoder-specific function that
returns
> the maximum number of bytes the encoder will produce. Right now, you can use
> MaxEncodedBytes. When you remove MaxEncodedBytes, you'll have to use something
> else. Or pass a Buffer* to EncodeCall as I suggested.
> 
> There's no harm in leaving it like this for now, though, I suppose.

Ah, right! I think that's already in place in the virtual BytesPerSample()
function of AudioEncoderPcm. That's what's used to calculate MaxEncodedBytes()
atm. AudioEncoderPcm (and children) seems set-up to encode full_frame_samples_
at a time, so we should be fine.

[hlundin-webrtc, 2016-02-29 12:46:47]

Nice CL! I have a few comments and questions.

https://codereview.webrtc.org/1725143003/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/audio_encoder.cc (right):

https://codereview.webrtc.org/1725143003/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/audio_encoder.cc:57:
EncodeInternal(rtp_timestamp, audio,
What was wrong with the old formatting?

https://codereview.webrtc.org/1725143003/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/g722/audio_encoder_g722.cc (right):

https://codereview.webrtc.org/1725143003/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/g722/audio_encoder_g722.cc:125:
info.encoded_bytes = encoded->AppendData(
Just out of curiosity: Is AppendData as used here superior to simply writing to
encoded->data()?

https://codereview.webrtc.org/1725143003/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/g722/audio_encoder_g722.cc:149: void
AudioEncoderG722::Reset() {
The order of method definitions should match the order of declarations.

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

https://codereview.webrtc.org/1725143003/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/ilbc/audio_encoder_ilbc.cc:125: return (r >=
0) ? static_cast<size_t>(r) : 0;
You just CHECKed that r >= 0; no need to take care of the negative case.

Alternative: just return rtc::checked_cast<size_t>(r);

https://codereview.webrtc.org/1725143003/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/mock/mock_audio_encoder.h (right):

https://codereview.webrtc.org/1725143003/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/mock/mock_audio_encoder.h:2: *  Copyright (c)
2016 The WebRTC project authors. All Rights Reserved.
This file is not new, right? Keep the old copyright year.

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

https://codereview.webrtc.org/1725143003/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:133:
AudioEncoder::EncodedInfo AudioEncoderOpus::EncodeInternal(
Order no longer matches that of the .h file. I do realize that the diff will be
much larger if you fix it in this CL. Consider doing it in a last step, or in a
follow-up.

https://codereview.webrtc.org/1725143003/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/red/audio_encoder_copy_red_unittest.cc
(right):

https://codereview.webrtc.org/1725143003/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/red/audio_encoder_copy_red_unittest.cc:138:
.WillOnce(Invoke(MockAudioEncoder::FakeEncoding(kEncodedSize)))
I like the new mock helpers! :)

https://codereview.webrtc.org/1725143003/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/audio_decoder_unittest.cc (right):

https://codereview.webrtc.org/1725143003/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/audio_decoder_unittest.cc:139:
encoded_info.encoded_bytes = 0;
No need to set encoded_bytes = 0, since the ctor does it for you.

https://codereview.webrtc.org/1725143003/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/test/neteq_isac_quality_test.cc (right):

https://codereview.webrtc.org/1725143003/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/test/neteq_isac_quality_test.cc:92:
payload->AppendData(max_bytes, [&] (rtc::ArrayView<uint8_t> payload) {
Bonus points for re-writing this test to use AudioEncoderIsacFix instead. :)

[ossu, 2016-02-29 13:23:01]

https://codereview.webrtc.org/1725143003/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/audio_encoder.cc (right):

https://codereview.webrtc.org/1725143003/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/audio_encoder.cc:57:
EncodeInternal(rtp_timestamp, audio,
On 2016/02/29 12:46:47, hlundin-webrtc wrote:
> What was wrong with the old formatting?

I ... don't know. :(
Chances are it got wrecked while I was doing things and I missed putting it
back.
Will do!

https://codereview.webrtc.org/1725143003/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/g722/audio_encoder_g722.cc (right):

https://codereview.webrtc.org/1725143003/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/g722/audio_encoder_g722.cc:125:
info.encoded_bytes = encoded->AppendData(
On 2016/02/29 12:46:47, hlundin-webrtc wrote:
> Just out of curiosity: Is AppendData as used here superior to simply writing
to
> encoded->data()?

Well, yes. encoded might be empty, so encoded->data() would be null, for
example.
AppendData ensures the buffer is large enough for an additional bytes_to_encode
bytes of data before running the setter.

https://codereview.webrtc.org/1725143003/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/g722/audio_encoder_g722.cc:149: void
AudioEncoderG722::Reset() {
On 2016/02/29 12:46:47, hlundin-webrtc wrote:
> The order of method definitions should match the order of declarations.

Acknowledged.

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

https://codereview.webrtc.org/1725143003/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/ilbc/audio_encoder_ilbc.cc:125: return (r >=
0) ? static_cast<size_t>(r) : 0;
On 2016/02/29 12:46:47, hlundin-webrtc wrote:
> You just CHECKed that r >= 0; no need to take care of the negative case.
> 
> Alternative: just return rtc::checked_cast<size_t>(r);

Acknowledged.

https://codereview.webrtc.org/1725143003/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/mock/mock_audio_encoder.h (right):

https://codereview.webrtc.org/1725143003/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/mock/mock_audio_encoder.h:2: *  Copyright (c)
2016 The WebRTC project authors. All Rights Reserved.
On 2016/02/29 12:46:47, hlundin-webrtc wrote:
> This file is not new, right? Keep the old copyright year.

Acknowledged.

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

https://codereview.webrtc.org/1725143003/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:133:
AudioEncoder::EncodedInfo AudioEncoderOpus::EncodeInternal(
On 2016/02/29 12:46:47, hlundin-webrtc wrote:
> Order no longer matches that of the .h file. I do realize that the diff will
be
> much larger if you fix it in this CL. Consider doing it in a last step, or in
a
> follow-up.

Acknowledged.

https://codereview.webrtc.org/1725143003/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/red/audio_encoder_copy_red_unittest.cc
(right):

https://codereview.webrtc.org/1725143003/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/red/audio_encoder_copy_red_unittest.cc:138:
.WillOnce(Invoke(MockAudioEncoder::FakeEncoding(kEncodedSize)))
On 2016/02/29 12:46:47, hlundin-webrtc wrote:
> I like the new mock helpers! :)

Hooray! \o_o/

https://codereview.webrtc.org/1725143003/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/audio_decoder_unittest.cc (right):

https://codereview.webrtc.org/1725143003/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/audio_decoder_unittest.cc:139:
encoded_info.encoded_bytes = 0;
On 2016/02/29 12:46:47, hlundin-webrtc wrote:
> No need to set encoded_bytes = 0, since the ctor does it for you.

Acknowledged.

https://codereview.webrtc.org/1725143003/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/test/neteq_isac_quality_test.cc (right):

https://codereview.webrtc.org/1725143003/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/test/neteq_isac_quality_test.cc:92:
payload->AppendData(max_bytes, [&] (rtc::ArrayView<uint8_t> payload) {
On 2016/02/29 12:46:47, hlundin-webrtc wrote:
> Bonus points for re-writing this test to use AudioEncoderIsacFix instead. :)

As a separate CL, maybe?

[hlundin-webrtc, 2016-02-29 14:46:10]

LGTM. Well done.

We can resolve my one remaining discussion point offline.

https://codereview.webrtc.org/1725143003/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/g722/audio_encoder_g722.cc (right):

https://codereview.webrtc.org/1725143003/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/g722/audio_encoder_g722.cc:125:
info.encoded_bytes = encoded->AppendData(
On 2016/02/29 13:23:01, ossu wrote:
> On 2016/02/29 12:46:47, hlundin-webrtc wrote:
> > Just out of curiosity: Is AppendData as used here superior to simply writing
> to
> > encoded->data()?
> 
> Well, yes. encoded might be empty, so encoded->data() would be null, for
> example.
> AppendData ensures the buffer is large enough for an additional
bytes_to_encode
> bytes of data before running the setter.

Ah, but I meant was to first ensure that encoded->data() is capable of accepting
bytes_to_encode bytes of data.

https://codereview.webrtc.org/1725143003/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/test/neteq_isac_quality_test.cc (right):

https://codereview.webrtc.org/1725143003/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/test/neteq_isac_quality_test.cc:92:
payload->AppendData(max_bytes, [&] (rtc::ArrayView<uint8_t> payload) {
On 2016/02/29 13:23:01, ossu wrote:
> On 2016/02/29 12:46:47, hlundin-webrtc wrote:
> > Bonus points for re-writing this test to use AudioEncoderIsacFix instead. :)
> 
> As a separate CL, maybe?

Absolutely. Or leave it up to some of the code owners in that sub-tree. [Insert
"Homer disappears into bushes" meme here.]

[ossu, 2016-02-29 16:02:24]

The CQ bit was checked by ossu@webrtc.org

[ossu, 2016-02-29 16:02:25]

The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1725143003/#ps80001 (title:
"Fixed a number of issues based on comments")

[commit-bot: I haz the power, 2016-02-29 16:02:34]

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

[commit-bot: I haz the power, 2016-02-29 16:10:18]

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

[commit-bot: I haz the power, 2016-02-29 16:10:19]

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

[ossu, 2016-02-29 16:45:25]

Alright. Fixed an issue where implementing one EncodeInternal variant in a
subclass would hide the other, causing build warnings (treated as errors) in
GCC. Ran a separate trybot test that succeeded.

[ossu, 2016-02-29 16:45:34]

The CQ bit was checked by ossu@webrtc.org

[ossu, 2016-02-29 16:45:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
henrik.lundin@webrtc.org, kwiberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1725143003/#ps120001 (title:
"Added more fixes for override hiding in AudioEncoder implementations.")

[commit-bot: I haz the power, 2016-02-29 16:45:41]

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

[commit-bot: I haz the power, 2016-02-29 18:46:28]

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

[commit-bot: I haz the power, 2016-02-29 18:46:30]

Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_dbg on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  mac_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[ossu, 2016-03-01 08:31:39]

The CQ bit was checked by ossu@webrtc.org

[commit-bot: I haz the power, 2016-03-01 08:31:45]

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

[commit-bot: I haz the power, 2016-03-01 08:41:34]

Description was changed from

==========
Changed AudioEncoder::Encode to take an rtc::Buffer* instead of uint8_t* and a
maximum size.

For backwards compatibility, I've added kept the old interface to
Encode() and EncodeInternal and created default implementations of both
variants of EncodeInternal(), each calling the other. At least one of
the variants must be implemented in a subclass or we'll run out of stack
and explode. Would be nice if we could catch that before runtime. :/

The new interface to EncodeInternal() is protected, since it should
never be called from the outside.

Was unable to mark the old EncodeInternal() as RTC_DEPRECATED, since the
default implementaion of the new variant needs to call it to work around
old implementations. The old Encode() variant is deprecated, at least.

Added a test for backwards compatibility in audio_encoder_unittest.cc.
For the added test I broke out MockEncodeHelper from
audio_encoder_copy_red_unittest.cc and renamed it MockAudioEncoderHelper.
==========

to

==========
Changed AudioEncoder::Encode to take an rtc::Buffer* instead of uint8_t* and a
maximum size.

For backwards compatibility, I've added kept the old interface to
Encode() and EncodeInternal and created default implementations of both
variants of EncodeInternal(), each calling the other. At least one of
the variants must be implemented in a subclass or we'll run out of stack
and explode. Would be nice if we could catch that before runtime. :/

The new interface to EncodeInternal() is protected, since it should
never be called from the outside.

Was unable to mark the old EncodeInternal() as RTC_DEPRECATED, since the
default implementaion of the new variant needs to call it to work around
old implementations. The old Encode() variant is deprecated, at least.

Added a test for backwards compatibility in audio_encoder_unittest.cc.
For the added test I broke out MockEncodeHelper from
audio_encoder_copy_red_unittest.cc and renamed it MockAudioEncoderHelper.
==========

[commit-bot: I haz the power, 2016-03-01 08:41:38]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-03-01 08:41:45]

Description was changed from

==========
Changed AudioEncoder::Encode to take an rtc::Buffer* instead of uint8_t* and a
maximum size.

For backwards compatibility, I've added kept the old interface to
Encode() and EncodeInternal and created default implementations of both
variants of EncodeInternal(), each calling the other. At least one of
the variants must be implemented in a subclass or we'll run out of stack
and explode. Would be nice if we could catch that before runtime. :/

The new interface to EncodeInternal() is protected, since it should
never be called from the outside.

Was unable to mark the old EncodeInternal() as RTC_DEPRECATED, since the
default implementaion of the new variant needs to call it to work around
old implementations. The old Encode() variant is deprecated, at least.

Added a test for backwards compatibility in audio_encoder_unittest.cc.
For the added test I broke out MockEncodeHelper from
audio_encoder_copy_red_unittest.cc and renamed it MockAudioEncoderHelper.
==========

to

==========
Changed AudioEncoder::Encode to take an rtc::Buffer* instead of uint8_t* and a
maximum size.

For backwards compatibility, I've added kept the old interface to
Encode() and EncodeInternal and created default implementations of both
variants of EncodeInternal(), each calling the other. At least one of
the variants must be implemented in a subclass or we'll run out of stack
and explode. Would be nice if we could catch that before runtime. :/

The new interface to EncodeInternal() is protected, since it should
never be called from the outside.

Was unable to mark the old EncodeInternal() as RTC_DEPRECATED, since the
default implementaion of the new variant needs to call it to work around
old implementations. The old Encode() variant is deprecated, at least.

Added a test for backwards compatibility in audio_encoder_unittest.cc.
For the added test I broke out MockEncodeHelper from
audio_encoder_copy_red_unittest.cc and renamed it MockAudioEncoderHelper.

Committed: https://crrev.com/10a029e95216c568d9bba9714e52edd761cf9054
Cr-Commit-Position: refs/heads/master@{#11823}
==========

[commit-bot: I haz the power, 2016-03-01 08:41:46]

Patchset 7 (id:??) landed as
https://crrev.com/10a029e95216c568d9bba9714e52edd761cf9054
Cr-Commit-Position: refs/heads/master@{#11823}

[The Sun (google.com), 2016-03-01 21:06:14]

solenberg@google.com changed reviewers:
+ solenberg@google.com

[The Sun (google.com), 2016-03-01 21:06:15]

Good stuff!

https://codereview.webrtc.org/1725143003/diff/120001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.cc (right):

https://codereview.webrtc.org/1725143003/diff/120001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.cc:211:
encoded->AppendData(bytes_to_encode, [&] (rtc::ArrayView<uint8_t> encoded) {
FYI: Chromium C++11 style guide says to not use default captures:
https://chromium-cpp.appspot.com/

[ossu, 2016-03-02 08:39:43]

https://codereview.webrtc.org/1725143003/diff/120001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.cc (right):

https://codereview.webrtc.org/1725143003/diff/120001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.cc:211:
encoded->AppendData(bytes_to_encode, [&] (rtc::ArrayView<uint8_t> encoded) {
On 2016/03/01 21:06:15, solenberg wrote:
> FYI: Chromium C++11 style guide says to not use default captures:
> https://chromium-cpp.appspot.com/

Well yes, and no. It says don't use it, referring to the Google Style Guide,
which says "Use default captures judiciously, when it aids readability [...]".
You can surely get turned around by default captures, but I believe this usage
is straight-forward enough: the code in the lambda is code that would otherwise
be in the function's scope (probably at that same point), which does have access
to this and all the local variables.

[kwiberg-webrtc, 2016-03-02 09:40:24]

https://codereview.webrtc.org/1725143003/diff/120001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.cc (right):

https://codereview.webrtc.org/1725143003/diff/120001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.cc:211:
encoded->AppendData(bytes_to_encode, [&] (rtc::ArrayView<uint8_t> encoded) {
On 2016/03/02 08:39:42, ossu wrote:
> On 2016/03/01 21:06:15, solenberg wrote:
> > FYI: Chromium C++11 style guide says to not use default captures:
> > https://chromium-cpp.appspot.com/
> 
> Well yes, and no. It says don't use it, referring to the Google Style Guide,
> which says "Use default captures judiciously, when it aids readability [...]".
> You can surely get turned around by default captures, but I believe this usage
> is straight-forward enough: the code in the lambda is code that would
otherwise
> be in the function's scope (probably at that same point), which does have
access
> to this and all the local variables.

Yes. The Google style guide used to blanket ban default capture, but that was
changed to the current text, which says "Avoid default lambda captures when
capturing this or if the lambda will escape the current scope.". I'm guessing
the Chromium C++11 text simply hasn't been updated.

By the letter of the text, I guess you should write [&, this], though---but I
certainly won't insist, since I don't think it makes sense to special-case
"this" once we have the "no default capture if the lambda escapes" rule.

[the sun, 2016-03-02 10:00:29]

On 2016/03/02 09:40:24, kwiberg-webrtc wrote:
>
https://codereview.webrtc.org/1725143003/diff/120001/webrtc/modules/audio_cod...
> File webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.cc (right):
> 
>
https://codereview.webrtc.org/1725143003/diff/120001/webrtc/modules/audio_cod...
> webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.cc:211:
> encoded->AppendData(bytes_to_encode, [&] (rtc::ArrayView<uint8_t> encoded) {
> On 2016/03/02 08:39:42, ossu wrote:
> > On 2016/03/01 21:06:15, solenberg wrote:
> > > FYI: Chromium C++11 style guide says to not use default captures:
> > > https://chromium-cpp.appspot.com/
> > 
> > Well yes, and no. It says don't use it, referring to the Google Style Guide,
> > which says "Use default captures judiciously, when it aids readability
[...]".
> > You can surely get turned around by default captures, but I believe this
usage
> > is straight-forward enough: the code in the lambda is code that would
> otherwise
> > be in the function's scope (probably at that same point), which does have
> access
> > to this and all the local variables.
> 
> Yes. The Google style guide used to blanket ban default capture, but that was
> changed to the current text, which says "Avoid default lambda captures when
> capturing this or if the lambda will escape the current scope.". I'm guessing
> the Chromium C++11 text simply hasn't been updated.
> 
> By the letter of the text, I guess you should write [&, this], though---but I
> certainly won't insist, since I don't think it makes sense to special-case
> "this" once we have the "no default capture if the lambda escapes" rule.

Ack. Thanks for the info!