Removed LEGACY_BITEXACT from neteq_impl.cc and updated the ACM unit tests.

Removed LEGACY_BITEXACT from neteq_impl.cc and updated the ACM unit tests.

I'll be rewriting AcmReceiver soon and am trying to reduce the amount of
old stuff that needs to be supported.

I've manually checked the outputs of the AcmReceiver bitexactness
tests with this change. A large part of the tests are still bitexact,
with one section only differing slightly in timings. Nothing audible
unless playing the old and new versions back simultaneously.

The output of NetEqDecoderTest were also changed due to this CL, although only on android. I built and ran the test locally and compared the audio output manually - the changes were the same as for the other tests; i.e. very slight timing changes for a part of the output.

I updated the network stats checksum for android without analyzing it further. I expect it goes hand-in-hand with the changes to the output; i.e. the changes in it are fine because the audio output is fine. Likely, the stats will show changes in the usage of CNG, since that is what the code changes.

BUG=webrtc:1361
Committed: https://crrev.com/108ecec51ce5d55bcbe455f7a1cb778dd3cb2b22
Cr-Commit-Position: refs/heads/master@{#13415}

[ossu, 2016-07-01 12:31:39]

Description was changed from

==========
Removed LEGACY_BITEXACT from neteq_impl.cc and updated the ACM unit tests.

I'll be rewriting AcmReceiver soon and am trying to reduce the amount of
old stuff that needs to be supported.

I've manually checked the outputs of the AcmReceiver bitexactness
tests with this change. A large part of the tests are still bitexact,
with one section only differing slightly in timings. Nothing audible
unless playing them back simultaneously.

BUG=webrtc:1361
==========

to

==========
Removed LEGACY_BITEXACT from neteq_impl.cc and updated the ACM unit tests.

I'll be rewriting AcmReceiver soon and am trying to reduce the amount of
old stuff that needs to be supported.

I've manually checked the outputs of the AcmReceiver bitexactness
tests with this change. A large part of the tests are still bitexact,
with one section only differing slightly in timings. Nothing audible
unless playing the old and new versions back simultaneously.

BUG=webrtc:1361
==========

[ossu, 2016-07-01 16:05:52]

Description was changed from

==========
Removed LEGACY_BITEXACT from neteq_impl.cc and updated the ACM unit tests.

I'll be rewriting AcmReceiver soon and am trying to reduce the amount of
old stuff that needs to be supported.

I've manually checked the outputs of the AcmReceiver bitexactness
tests with this change. A large part of the tests are still bitexact,
with one section only differing slightly in timings. Nothing audible
unless playing the old and new versions back simultaneously.

BUG=webrtc:1361
==========

to

==========
Removed LEGACY_BITEXACT from neteq_impl.cc and updated the ACM unit tests.

I'll be rewriting AcmReceiver soon and am trying to reduce the amount of
old stuff that needs to be supported.

I've manually checked the outputs of the AcmReceiver bitexactness
tests with this change. A large part of the tests are still bitexact,
with one section only differing slightly in timings. Nothing audible
unless playing the old and new versions back simultaneously.

The output of NetEqDecoderTest were also changed due to this CL, although only
on android. I built and ran the test locally and compared the audio output
manually - the changes were the same as for the other tests; i.e. very slight
timing changes for a part of the output.

I updated the network stats checksum for android without analyzing it further. I
expect it goes hand-in-hand with the changes to the output; i.e. the changes in
it are fine because the audio output is fine. Likely, the stats will show
changes in the usage of CNG, since that is what the code changes.

BUG=webrtc:1361
==========

[ossu, 2016-07-04 11:36:09]

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

[ossu, 2016-07-04 11:36:10]

PTAL at this change. I've verified the audio and ran it by @hlundin as well.
I've not explicitly verified the changes to the stats on android_dbg; do you
know how I'd go about doing that, if at all necessary?

[minyue-webrtc, 2016-07-07 15:22:06]

https://codereview.webrtc.org/2117763002/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_impl.cc (left):

https://codereview.webrtc.org/2117763002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:1380: if (*operation ==
kRfc3389Cng) {
is it still possible to run in here?

though it is wrapped in LEGACY_BITEXACT, it seems to mean more than BITEXACT

Seems we never tested "return 0", what would that mean?

https://codereview.webrtc.org/2117763002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:1779: #ifdef LEGACY_BITEXACT
is it still possible to run in here?

though it is wrapped in LEGACY_BITEXACT, it seems to mean more than BITEXACT

Seems we never tested "return kOtherError", what would that mean?

https://codereview.webrtc.org/2117763002/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right):

https://codereview.webrtc.org/2117763002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:1148: } else if (*operation !=
kRfc3389Cng) {
is "else if (*operation != kRfc3389Cng)" still valid,

Can *operation become undefined?

[ossu, 2016-07-08 09:08:26]

https://codereview.webrtc.org/2117763002/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_impl.cc (left):

https://codereview.webrtc.org/2117763002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:1380: if (*operation ==
kRfc3389Cng) {
On 2016/07/07 15:22:05, minyue-webrtc wrote:
> is it still possible to run in here?
> 
> though it is wrapped in LEGACY_BITEXACT, it seems to mean more than BITEXACT
> 
> Seems we never tested "return 0", what would that mean?

Hmm, maybe. If we did, we'd call DecodeLoop() below with a NULL decoder (since
CngDecoder != AudioDecoder). But DecodeLoop seems fine with that - it does
checks to basically do nothing if the packet is comfort noise. return_value will
be zero and decoded_length will be untouched. Basically, it will just fall
through everything and return 0 anyways.

https://codereview.webrtc.org/2117763002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:1779: #ifdef LEGACY_BITEXACT
On 2016/07/07 15:22:05, minyue-webrtc wrote:
> is it still possible to run in here?
> 
> though it is wrapped in LEGACY_BITEXACT, it seems to mean more than BITEXACT
> 
> Seems we never tested "return kOtherError", what would that mean?

Well, we do return other types of errors, e.g. kUnknownRtpPayloadType. Maybe the
LOG should be replaced with an RTC_UNREACHED (or RTC_CHECK(false)) and the
return removed. With this change, we really should never reach the point where
we try to generate CNG from a non-CNG packet.

https://codereview.webrtc.org/2117763002/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right):

https://codereview.webrtc.org/2117763002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:1148: } else if (*operation !=
kRfc3389Cng) {
On 2016/07/07 15:22:05, minyue-webrtc wrote:
> is "else if (*operation != kRfc3389Cng)" still valid,
> 
> Can *operation become undefined?

Hmm, this looks weird. If operation is CngNoPacket and we don't have a packet,
we'll automatically change it to kNormal? That seems off. I guess the
IsComfortNoise check should be in its own if-clause. That way, we'll either have
CngNoPacket, Cng or Normal at the end of these if/else-if blocks.

I don't see how operation could become Undefined after passing through here,
though.

[minyue-webrtc, 2016-07-08 12:36:25]

lgtm

https://codereview.webrtc.org/2117763002/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right):

https://codereview.webrtc.org/2117763002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:1148: } else if (*operation !=
kRfc3389Cng) {
On 2016/07/08 09:08:26, ossu wrote:
> On 2016/07/07 15:22:05, minyue-webrtc wrote:
> > is "else if (*operation != kRfc3389Cng)" still valid,
> > 
> > Can *operation become undefined?
> 
> Hmm, this looks weird. If operation is CngNoPacket and we don't have a packet,
> we'll automatically change it to kNormal? That seems off. I guess the
> IsComfortNoise check should be in its own if-clause. That way, we'll either
have
> CngNoPacket, Cng or Normal at the end of these if/else-if blocks.
> 
> I don't see how operation could become Undefined after passing through here,
> though.

Ah, I see. The original code did not capture all cases either. Therefore
*operation can remain undefined at the end of the function. and then the caller
has an assert.

[ossu, 2016-07-08 12:39:19]

The CQ bit was checked by ossu@webrtc.org

[commit-bot: I haz the power, 2016-07-08 12:39:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-07-08 14:40:06]

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

[commit-bot: I haz the power, 2016-07-08 14:40:07]

Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[ossu, 2016-07-08 15:43:32]

The CQ bit was checked by ossu@webrtc.org

[commit-bot: I haz the power, 2016-07-08 15:43:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-07-08 15:45:23]

Description was changed from

==========
Removed LEGACY_BITEXACT from neteq_impl.cc and updated the ACM unit tests.

I'll be rewriting AcmReceiver soon and am trying to reduce the amount of
old stuff that needs to be supported.

I've manually checked the outputs of the AcmReceiver bitexactness
tests with this change. A large part of the tests are still bitexact,
with one section only differing slightly in timings. Nothing audible
unless playing the old and new versions back simultaneously.

The output of NetEqDecoderTest were also changed due to this CL, although only
on android. I built and ran the test locally and compared the audio output
manually - the changes were the same as for the other tests; i.e. very slight
timing changes for a part of the output.

I updated the network stats checksum for android without analyzing it further. I
expect it goes hand-in-hand with the changes to the output; i.e. the changes in
it are fine because the audio output is fine. Likely, the stats will show
changes in the usage of CNG, since that is what the code changes.

BUG=webrtc:1361
==========

to

==========
Removed LEGACY_BITEXACT from neteq_impl.cc and updated the ACM unit tests.

I'll be rewriting AcmReceiver soon and am trying to reduce the amount of
old stuff that needs to be supported.

I've manually checked the outputs of the AcmReceiver bitexactness
tests with this change. A large part of the tests are still bitexact,
with one section only differing slightly in timings. Nothing audible
unless playing the old and new versions back simultaneously.

The output of NetEqDecoderTest were also changed due to this CL, although only
on android. I built and ran the test locally and compared the audio output
manually - the changes were the same as for the other tests; i.e. very slight
timing changes for a part of the output.

I updated the network stats checksum for android without analyzing it further. I
expect it goes hand-in-hand with the changes to the output; i.e. the changes in
it are fine because the audio output is fine. Likely, the stats will show
changes in the usage of CNG, since that is what the code changes.

BUG=webrtc:1361
==========

[commit-bot: I haz the power, 2016-07-08 15:45:24]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-08 15:45:32]

Description was changed from

==========
Removed LEGACY_BITEXACT from neteq_impl.cc and updated the ACM unit tests.

I'll be rewriting AcmReceiver soon and am trying to reduce the amount of
old stuff that needs to be supported.

I've manually checked the outputs of the AcmReceiver bitexactness
tests with this change. A large part of the tests are still bitexact,
with one section only differing slightly in timings. Nothing audible
unless playing the old and new versions back simultaneously.

The output of NetEqDecoderTest were also changed due to this CL, although only
on android. I built and ran the test locally and compared the audio output
manually - the changes were the same as for the other tests; i.e. very slight
timing changes for a part of the output.

I updated the network stats checksum for android without analyzing it further. I
expect it goes hand-in-hand with the changes to the output; i.e. the changes in
it are fine because the audio output is fine. Likely, the stats will show
changes in the usage of CNG, since that is what the code changes.

BUG=webrtc:1361
==========

to

==========
Removed LEGACY_BITEXACT from neteq_impl.cc and updated the ACM unit tests.

I'll be rewriting AcmReceiver soon and am trying to reduce the amount of
old stuff that needs to be supported.

I've manually checked the outputs of the AcmReceiver bitexactness
tests with this change. A large part of the tests are still bitexact,
with one section only differing slightly in timings. Nothing audible
unless playing the old and new versions back simultaneously.

The output of NetEqDecoderTest were also changed due to this CL, although only
on android. I built and ran the test locally and compared the audio output
manually - the changes were the same as for the other tests; i.e. very slight
timing changes for a part of the output.

I updated the network stats checksum for android without analyzing it further. I
expect it goes hand-in-hand with the changes to the output; i.e. the changes in
it are fine because the audio output is fine. Likely, the stats will show
changes in the usage of CNG, since that is what the code changes.

BUG=webrtc:1361

Committed: https://crrev.com/108ecec51ce5d55bcbe455f7a1cb778dd3cb2b22
Cr-Commit-Position: refs/heads/master@{#13415}
==========

[commit-bot: I haz the power, 2016-07-08 15:45:33]

Patchset 3 (id:??) landed as
https://crrev.com/108ecec51ce5d55bcbe455f7a1cb778dd3cb2b22
Cr-Commit-Position: refs/heads/master@{#13415}