Adds fuzzer for the residual echo detector.

Adds fuzzer for the residual echo detector.

This is a robustness test for the residual echo detector, that can help to detect numerical issues.

BUG=b/38014838

Review-Url: https://codereview.webrtc.org/2877803002
Cr-Commit-Position: refs/heads/master@{#18165}
Committed: https://chromium.googlesource.com/external/webrtc/+/860249ec62d371f84f1a885c4a73c783aac609c2

[ivoc, 2017-05-11 16:27:07]

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

[ivoc, 2017-05-11 16:27:08]

Hi Henrik,

Please have a look at this fuzzer for the echo detector.

[ivoc, 2017-05-11 16:30:26]

Patchset #1 (id:1) has been deleted

[hlundin-webrtc, 2017-05-12 07:32:31]

Nice! I have a few comments on the test itself, but mostly LG.

https://codereview.webrtc.org/2877803002/diff/20001/webrtc/test/fuzzers/resid...
File webrtc/test/fuzzers/residual_echo_detector_fuzzer.cc (right):

https://codereview.webrtc.org/2877803002/diff/20001/webrtc/test/fuzzers/resid...
webrtc/test/fuzzers/residual_echo_detector_fuzzer.cc:24:
RTC_DCHECK_EQ(sizeof(float), sizeof(unsigned int));
Make this a static_assert.

And, do you want to make sure the size is equal to that of an unsigned int
(which in itself may vary), or do you want to compare to a fixed-width type
(e.g., uint32_t)?

https://codereview.webrtc.org/2877803002/diff/20001/webrtc/test/fuzzers/resid...
webrtc/test/fuzzers/residual_echo_detector_fuzzer.cc:41: memcpy(&call_order_int,
&data[0], 2);
I would like to see either a read index (size_t) or a read ptr (uint8_t*) and
keep updating that throughout this function.

size_t read_ix = 0;
memcpy(&call_order_int, &data[read_ix], 2);
read_ix += 2;

- or -

uint8_t* read_ptr = data;
memcpy(&call_order_int, read_ptr, 2);
read_ptr += 2;


And then in the loop below:

memcpy(input.data(), &data[read_ix], 4);
read_ix += 4;

- or -
memcpy(input.data(), read_ptr, 4);
read_ptr += 4;

https://codereview.webrtc.org/2877803002/diff/20001/webrtc/test/fuzzers/resid...
webrtc/test/fuzzers/residual_echo_detector_fuzzer.cc:50: memcpy(input.data(),
&data[i * 4 + 2], 4);
Add a DCHECK that you are not reading outside of the array.

[ivoc, 2017-05-12 09:43:22]

Thanks for the comments!

https://codereview.webrtc.org/2877803002/diff/20001/webrtc/test/fuzzers/resid...
File webrtc/test/fuzzers/residual_echo_detector_fuzzer.cc (right):

https://codereview.webrtc.org/2877803002/diff/20001/webrtc/test/fuzzers/resid...
webrtc/test/fuzzers/residual_echo_detector_fuzzer.cc:24:
RTC_DCHECK_EQ(sizeof(float), sizeof(unsigned int));
On 2017/05/12 07:32:31, hlundin-webrtc wrote:
> Make this a static_assert.
> 
> And, do you want to make sure the size is equal to that of an unsigned int
> (which in itself may vary), or do you want to compare to a fixed-width type
> (e.g., uint32_t)?

Good question. I added this DCHECK earlier when I used a different way to
convert from the bytes to float, but I think I don't need it anymore now. I'll
just remove it.

https://codereview.webrtc.org/2877803002/diff/20001/webrtc/test/fuzzers/resid...
webrtc/test/fuzzers/residual_echo_detector_fuzzer.cc:41: memcpy(&call_order_int,
&data[0], 2);
On 2017/05/12 07:32:31, hlundin-webrtc wrote:
> I would like to see either a read index (size_t) or a read ptr (uint8_t*) and
> keep updating that throughout this function.
> 
> size_t read_ix = 0;
> memcpy(&call_order_int, &data[read_ix], 2);
> read_ix += 2;
> 
> - or -
> 
> uint8_t* read_ptr = data;
> memcpy(&call_order_int, read_ptr, 2);
> read_ptr += 2;
> 
> 
> And then in the loop below:
> 
> memcpy(input.data(), &data[read_ix], 4);
> read_ix += 4;
> 
> - or -
> memcpy(input.data(), read_ptr, 4);
> read_ptr += 4;

Good idea, I updated the code to use a read_ix.

https://codereview.webrtc.org/2877803002/diff/20001/webrtc/test/fuzzers/resid...
webrtc/test/fuzzers/residual_echo_detector_fuzzer.cc:50: memcpy(input.data(),
&data[i * 4 + 2], 4);
On 2017/05/12 07:32:31, hlundin-webrtc wrote:
> Add a DCHECK that you are not reading outside of the array.

Done.

[hlundin-webrtc, 2017-05-12 11:39:48]

lgtm

https://codereview.webrtc.org/2877803002/diff/20001/webrtc/test/fuzzers/resid...
File webrtc/test/fuzzers/residual_echo_detector_fuzzer.cc (right):

https://codereview.webrtc.org/2877803002/diff/20001/webrtc/test/fuzzers/resid...
webrtc/test/fuzzers/residual_echo_detector_fuzzer.cc:24:
RTC_DCHECK_EQ(sizeof(float), sizeof(unsigned int));
On 2017/05/12 09:43:22, ivoc wrote:
> On 2017/05/12 07:32:31, hlundin-webrtc wrote:
> > Make this a static_assert.
> > 
> > And, do you want to make sure the size is equal to that of an unsigned int
> > (which in itself may vary), or do you want to compare to a fixed-width type
> > (e.g., uint32_t)?
> 
> Good question. I added this DCHECK earlier when I used a different way to
> convert from the bytes to float, but I think I don't need it anymore now. I'll
> just remove it.

Acknowledged.

[ivoc, 2017-05-12 12:02:39]

The CQ bit was checked by ivoc@webrtc.org

[commit-bot: I haz the power, 2017-05-12 12:02:48]

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, 2017-05-12 12:07:56]

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

[commit-bot: I haz the power, 2017-05-12 12:07:57]

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

[ivoc, 2017-05-12 12:11:59]

ivoc@webrtc.org changed reviewers:
+ kjellander@webrtc.org

[ivoc, 2017-05-12 12:12:00]

Adding kjellander@ for review of the changes in webrtc/test.

[kjellander_webrtc, 2017-05-12 12:16:03]

rs lgtm for webrtc/test.
Someone more familiar with the code the fuzzer tests should review that code.

[hlundin-webrtc, 2017-05-12 12:35:00]

On 2017/05/12 12:16:03, kjellander_webrtc wrote:
> rs lgtm for webrtc/test.
> Someone more familiar with the code the fuzzer tests should review that code.

I did review that code and it looks good to me. Should we do something about the
ownership situation with the fuzzers. We now have one bucket were we put a lot
of specializations from all parts of the code base.

[kjellander_webrtc, 2017-05-12 12:42:59]

On 2017/05/12 12:35:00, hlundin-webrtc wrote:
> On 2017/05/12 12:16:03, kjellander_webrtc wrote:
> > rs lgtm for webrtc/test.
> > Someone more familiar with the code the fuzzer tests should review that
code.
> 
> I did review that code and it looks good to me. Should we do something about
the
> ownership situation with the fuzzers. We now have one bucket were we put a lot
> of specializations from all parts of the code base.

Sure. Please expand
https://chromium.googlesource.com/external/webrtc/+/master/webrtc/test/fuzzer...
and add me as *.gn owner only or something like that? It should send a clear
signal

[ivoc, 2017-05-16 13:29:29]

The CQ bit was checked by ivoc@webrtc.org

[commit-bot: I haz the power, 2017-05-16 13:29:33]

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, 2017-05-16 13:50:11]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494941369305910,
"parent_rev": "52775841f0212b0a61ec17ac86080ccbb1c19812", "commit_rev":
"860249ec62d371f84f1a885c4a73c783aac609c2"}

[commit-bot: I haz the power, 2017-05-16 13:50:17]

Description was changed from

==========
Adds fuzzer for the residual echo detector.

This is a robustness test for the residual echo detector, that can help to
detect numerical issues.

BUG=b/38014838
==========

to

==========
Adds fuzzer for the residual echo detector.

This is a robustness test for the residual echo detector, that can help to
detect numerical issues.

BUG=b/38014838

Review-Url: https://codereview.webrtc.org/2877803002
Cr-Commit-Position: refs/heads/master@{#18165}
Committed:
https://chromium.googlesource.com/external/webrtc/+/860249ec62d371f84f1a885c4...
==========

[commit-bot: I haz the power, 2017-05-16 13:50:18]

Committed patchset #2 (id:40001) as
https://chromium.googlesource.com/external/webrtc/+/860249ec62d371f84f1a885c4...