Adding a some checks and switching out a few assert for RTC_[D]CHECK.

Adding a some checks and switching out a few assert for RTC_[D]CHECK.
These changes are around use of AudioFrame.data_ to help us catch issues earlier since assert() is left out in release builds, including builds with DCHECK enabled.  I've also added a few full-on CHECKs to avoid reading past buffer boundaries or continuing on in a failed state.

BUG=chromium:613482
NOTRY=true
(using notry due to offline android_arm64_rel bot)

Committed: https://crrev.com/d36df89d40bde3c62ee5cbff841933e50b3c007b
Cr-Commit-Position: refs/heads/master@{#12870}

[tommi, 2016-05-23 13:54:09]

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

[tommi, 2016-05-23 13:54:15]

The CQ bit was checked by tommi@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-23 13:54:23]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2007563002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2007563002/1

[commit-bot: I haz the power, 2016-05-23 14:03:24]

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

[commit-bot: I haz the power, 2016-05-23 14:03:25]

Dry run: Try jobs failed on following builders:
  linux_ubsan on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/2281)
  linux_ubsan_vptr on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)

[hlundin-webrtc, 2016-05-24 06:38:03]

LG, but consider my suggestions. And, you've got some red tests to figure out...
:)

https://codereview.webrtc.org/2007563002/diff/1/webrtc/voice_engine/channel.cc
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2007563002/diff/1/webrtc/voice_engine/channel.c...
webrtc/voice_engine/channel.cc:2996: RTC_CHECK(length <=
sizeof(_audioFrame.data_));
RTC_CHECK_LE

https://codereview.webrtc.org/2007563002/diff/1/webrtc/voice_engine/utility.cc
File webrtc/voice_engine/utility.cc (right):

https://codereview.webrtc.org/2007563002/diff/1/webrtc/voice_engine/utility.c...
webrtc/voice_engine/utility.cc:56: RTC_CHECK(false) << "InitializeIfNeeded
failed: sample_rate_hz = "
Consider FATAL().

https://codereview.webrtc.org/2007563002/diff/1/webrtc/voice_engine/utility.c...
webrtc/voice_engine/utility.cc:66: RTC_CHECK(false) << "Resample failed:
audio_ptr = " << audio_ptr
And here.

https://codereview.webrtc.org/2007563002/diff/1/webrtc/voice_engine/utility.c...
webrtc/voice_engine/utility.cc:86: RTC_DCHECK(target_channel == 1 ||
target_channel == 2);
Consider rewriting these as
RTC_DCHECK_GE(target_channel, 1);
RTC_DCHECK_LE(target_channel, 2);
...

or, stream the actual value to the DCHECK
RTC_DCHECK(target_channel == 1 || target_channel == 2) << "target_channel=" <<
target_channel;

Either will give the actual value in the printout.

[tommi, 2016-05-24 07:07:24]

On 2016/05/24 06:38:03, hlundin-webrtc wrote:
> LG, but consider my suggestions. And, you've got some red tests to figure
out...
> :)
> 
> https://codereview.webrtc.org/2007563002/diff/1/webrtc/voice_engine/channel.cc
> File webrtc/voice_engine/channel.cc (right):
> 
>
https://codereview.webrtc.org/2007563002/diff/1/webrtc/voice_engine/channel.c...
> webrtc/voice_engine/channel.cc:2996: RTC_CHECK(length <=
> sizeof(_audioFrame.data_));
> RTC_CHECK_LE
> 
> https://codereview.webrtc.org/2007563002/diff/1/webrtc/voice_engine/utility.cc
> File webrtc/voice_engine/utility.cc (right):
> 
>
https://codereview.webrtc.org/2007563002/diff/1/webrtc/voice_engine/utility.c...
> webrtc/voice_engine/utility.cc:56: RTC_CHECK(false) << "InitializeIfNeeded
> failed: sample_rate_hz = "
> Consider FATAL().
> 
>
https://codereview.webrtc.org/2007563002/diff/1/webrtc/voice_engine/utility.c...
> webrtc/voice_engine/utility.cc:66: RTC_CHECK(false) << "Resample failed:
> audio_ptr = " << audio_ptr
> And here.
> 
>
https://codereview.webrtc.org/2007563002/diff/1/webrtc/voice_engine/utility.c...
> webrtc/voice_engine/utility.cc:86: RTC_DCHECK(target_channel == 1 ||
> target_channel == 2);
> Consider rewriting these as
> RTC_DCHECK_GE(target_channel, 1);
> RTC_DCHECK_LE(target_channel, 2);
> ...
> 
> or, stream the actual value to the DCHECK
> RTC_DCHECK(target_channel == 1 || target_channel == 2) << "target_channel=" <<
> target_channel;
> 
> Either will give the actual value in the printout.

Regarding the failures.  What do you think about removing the tests that pass
invalid arguments?

My thinking is that I would consider it a programmer error if the methods are
called with invalid arguments (that's generally the model used in Chrome and
Google).
Since the return value can be ignored I would like to make a hard stop and force
proper error handling to be done at the source.

[tommi, 2016-05-24 07:10:19]

On 2016/05/24 07:07:24, tommi-webrtc wrote:
> On 2016/05/24 06:38:03, hlundin-webrtc wrote:
> > LG, but consider my suggestions. And, you've got some red tests to figure
> out...
> > :)
> > 
> >
https://codereview.webrtc.org/2007563002/diff/1/webrtc/voice_engine/channel.cc
> > File webrtc/voice_engine/channel.cc (right):
> > 
> >
>
https://codereview.webrtc.org/2007563002/diff/1/webrtc/voice_engine/channel.c...
> > webrtc/voice_engine/channel.cc:2996: RTC_CHECK(length <=
> > sizeof(_audioFrame.data_));
> > RTC_CHECK_LE
> > 
> >
https://codereview.webrtc.org/2007563002/diff/1/webrtc/voice_engine/utility.cc
> > File webrtc/voice_engine/utility.cc (right):
> > 
> >
>
https://codereview.webrtc.org/2007563002/diff/1/webrtc/voice_engine/utility.c...
> > webrtc/voice_engine/utility.cc:56: RTC_CHECK(false) << "InitializeIfNeeded
> > failed: sample_rate_hz = "
> > Consider FATAL().
> > 
> >
>
https://codereview.webrtc.org/2007563002/diff/1/webrtc/voice_engine/utility.c...
> > webrtc/voice_engine/utility.cc:66: RTC_CHECK(false) << "Resample failed:
> > audio_ptr = " << audio_ptr
> > And here.
> > 
> >
>
https://codereview.webrtc.org/2007563002/diff/1/webrtc/voice_engine/utility.c...
> > webrtc/voice_engine/utility.cc:86: RTC_DCHECK(target_channel == 1 ||
> > target_channel == 2);
> > Consider rewriting these as
> > RTC_DCHECK_GE(target_channel, 1);
> > RTC_DCHECK_LE(target_channel, 2);
> > ...
> > 
> > or, stream the actual value to the DCHECK
> > RTC_DCHECK(target_channel == 1 || target_channel == 2) << "target_channel="
<<
> > target_channel;
> > 
> > Either will give the actual value in the printout.
> 
> Regarding the failures.  What do you think about removing the tests that pass
> invalid arguments?
> 
> My thinking is that I would consider it a programmer error if the methods are
> called with invalid arguments (that's generally the model used in Chrome and
> Google).
> Since the return value can be ignored I would like to make a hard stop and
force
> proper error handling to be done at the source.

ehm.... that would mean to remove the entire suite of tests for the resampler
:-|
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...

[tommi, 2016-05-24 07:17:16]

Split input verification test into multiple and include death tests

[tommi, 2016-05-24 07:17:31]

The CQ bit was checked by tommi@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-24 07:17:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2007563002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2007563002/20001

[commit-bot: I haz the power, 2016-05-24 07:23:43]

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

[commit-bot: I haz the power, 2016-05-24 07:23:44]

Dry run: Try jobs failed on following builders:
  linux_ubsan on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/2310)

[tommi, 2016-05-24 07:25:36]

Improve checks

[tommi, 2016-05-24 07:27:52]

Turn FailOnZeroDesiredFrequency test into a death test

[tommi, 2016-05-24 07:47:55]

The CQ bit was checked by tommi@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-24 07:47:57]

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

[tommi, 2016-05-24 07:49:03]

I've changed the existing test code into death tests where appropriate (no test
code actually removed as I initially suggested), but I don't see that the death
tests actually run anywhere.  Looking into it.

[tommi, 2016-05-24 07:49:14]

https://codereview.webrtc.org/2007563002/diff/1/webrtc/voice_engine/channel.cc
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2007563002/diff/1/webrtc/voice_engine/channel.c...
webrtc/voice_engine/channel.cc:2996: RTC_CHECK(length <=
sizeof(_audioFrame.data_));
On 2016/05/24 06:38:03, hlundin-webrtc wrote:
> RTC_CHECK_LE

Done.

https://codereview.webrtc.org/2007563002/diff/1/webrtc/voice_engine/utility.cc
File webrtc/voice_engine/utility.cc (right):

https://codereview.webrtc.org/2007563002/diff/1/webrtc/voice_engine/utility.c...
webrtc/voice_engine/utility.cc:56: RTC_CHECK(false) << "InitializeIfNeeded
failed: sample_rate_hz = "
On 2016/05/24 06:38:03, hlundin-webrtc wrote:
> Consider FATAL().

Done.

https://codereview.webrtc.org/2007563002/diff/1/webrtc/voice_engine/utility.c...
webrtc/voice_engine/utility.cc:66: RTC_CHECK(false) << "Resample failed:
audio_ptr = " << audio_ptr
On 2016/05/24 06:38:03, hlundin-webrtc wrote:
> And here.

Done.

https://codereview.webrtc.org/2007563002/diff/1/webrtc/voice_engine/utility.c...
webrtc/voice_engine/utility.cc:86: RTC_DCHECK(target_channel == 1 ||
target_channel == 2);
On 2016/05/24 06:38:03, hlundin-webrtc wrote:
> Consider rewriting these as
> RTC_DCHECK_GE(target_channel, 1);
> RTC_DCHECK_LE(target_channel, 2);
> ...
> 
> or, stream the actual value to the DCHECK
> RTC_DCHECK(target_channel == 1 || target_channel == 2) << "target_channel=" <<
> target_channel;
> 
> Either will give the actual value in the printout.

Done.

[commit-bot: I haz the power, 2016-05-24 07:52:34]

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

[commit-bot: I haz the power, 2016-05-24 07:52:35]

Dry run: Try jobs failed on following builders:
  mac_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/15380)

[tommi, 2016-05-24 08:13:37]

Fix death tests and problem with RTC_DCHECK_ON not being defined (+format)

[tommi, 2016-05-24 08:14:03]

The CQ bit was checked by tommi@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-24 08:14:09]

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

[tommi, 2016-05-24 08:14:53]

OK, there was a pebkac problem.  I had to include checks.h to actually force
RTC_DCHECK_ON to kick in.  All should go green now.

[henrika_webrtc, 2016-05-24 08:18:09]

Nice work Tommi ;-) Tried it in Chrome with a few clients just in case?

[tommi, 2016-05-24 08:31:42]

On 2016/05/24 08:18:09, henrika_webrtc wrote:
> Nice work Tommi ;-) Tried it in Chrome with a few clients just in case?

nope.  This is mostly a change from assert()->[D]CHECK.  So, unless I'm missing
something, Chrome should fail deterministically now where it before might fail
randomly.

[commit-bot: I haz the power, 2016-05-24 08:56:07]

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

[commit-bot: I haz the power, 2016-05-24 08:56:08]

Dry run: Try jobs failed on following builders:
  win_baremetal on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_baremetal/builds/11535)

[tommi, 2016-05-24 09:47:13]

Remove ExternalMixingResamplingToInvalidFrequenciesFails since voe_auto_test has
multiple threads running and death tests don't cope well with that

[tommi, 2016-05-24 09:48:19]

The CQ bit was checked by tommi@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-24 09:48:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2007563002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2007563002/100001

[hlundin-webrtc, 2016-05-24 10:46:27]

lgtm

[commit-bot: I haz the power, 2016-05-24 11:48:58]

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

[commit-bot: I haz the power, 2016-05-24 11:48:59]

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

[tommi, 2016-05-24 12:44:09]

The CQ bit was checked by tommi@webrtc.org

[commit-bot: I haz the power, 2016-05-24 12:44:18]

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

[tommi, 2016-05-24 12:46:39]

Description was changed from

==========
Adding a some checks and switching out a few assert for RTC_[D]CHECK.
These changes are around use of AudioFrame.data_ to help us catch issues earlier
since assert() is left out in release builds, including builds with DCHECK
enabled.  I've also added a few full-on CHECKs to avoid reading past buffer
boundaries or continuing on in a failed state.

BUG=chromium:613482
==========

to

==========
Adding a some checks and switching out a few assert for RTC_[D]CHECK.
These changes are around use of AudioFrame.data_ to help us catch issues earlier
since assert() is left out in release builds, including builds with DCHECK
enabled.  I've also added a few full-on CHECKs to avoid reading past buffer
boundaries or continuing on in a failed state.

BUG=chromium:613482
NOTRY=true
(using notry due to offline android_arm64_rel bot)
==========

[tommi, 2016-05-24 12:46:46]

The CQ bit was unchecked by tommi@webrtc.org

[tommi, 2016-05-24 12:46:53]

The CQ bit was checked by tommi@webrtc.org

[commit-bot: I haz the power, 2016-05-24 12:47:05]

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

[commit-bot: I haz the power, 2016-05-24 12:49:08]

Description was changed from

==========
Adding a some checks and switching out a few assert for RTC_[D]CHECK.
These changes are around use of AudioFrame.data_ to help us catch issues earlier
since assert() is left out in release builds, including builds with DCHECK
enabled.  I've also added a few full-on CHECKs to avoid reading past buffer
boundaries or continuing on in a failed state.

BUG=chromium:613482
NOTRY=true
(using notry due to offline android_arm64_rel bot)
==========

to

==========
Adding a some checks and switching out a few assert for RTC_[D]CHECK.
These changes are around use of AudioFrame.data_ to help us catch issues earlier
since assert() is left out in release builds, including builds with DCHECK
enabled.  I've also added a few full-on CHECKs to avoid reading past buffer
boundaries or continuing on in a failed state.

BUG=chromium:613482
NOTRY=true
(using notry due to offline android_arm64_rel bot)
==========

[commit-bot: I haz the power, 2016-05-24 12:49:10]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-05-24 12:49:17]

Description was changed from

==========
Adding a some checks and switching out a few assert for RTC_[D]CHECK.
These changes are around use of AudioFrame.data_ to help us catch issues earlier
since assert() is left out in release builds, including builds with DCHECK
enabled.  I've also added a few full-on CHECKs to avoid reading past buffer
boundaries or continuing on in a failed state.

BUG=chromium:613482
NOTRY=true
(using notry due to offline android_arm64_rel bot)
==========

to

==========
Adding a some checks and switching out a few assert for RTC_[D]CHECK.
These changes are around use of AudioFrame.data_ to help us catch issues earlier
since assert() is left out in release builds, including builds with DCHECK
enabled.  I've also added a few full-on CHECKs to avoid reading past buffer
boundaries or continuing on in a failed state.

BUG=chromium:613482
NOTRY=true
(using notry due to offline android_arm64_rel bot)

Committed: https://crrev.com/d36df89d40bde3c62ee5cbff841933e50b3c007b
Cr-Commit-Position: refs/heads/master@{#12870}
==========

[commit-bot: I haz the power, 2016-05-24 12:49:18]

Patchset 6 (id:??) landed as
https://crrev.com/d36df89d40bde3c62ee5cbff841933e50b3c007b
Cr-Commit-Position: refs/heads/master@{#12870}

[tommi, 2016-05-24 13:44:10]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.webrtc.org/2006243002/ by tommi@webrtc.org.

The reason for reverting is: Reverting temporarily.  Need to fix tests
downstream that pass invalid arguments..