NetEqTest: Extend the callback structure

NetEqTest: Extend the callback structure

This change allows more callbacks to be registered to the test object.
The callbacks are used to give the user of the test object the ability
to instrument the test object. This CL specifically adds
instrumentation points just after a packet is inserted into NetEq, and
just after audio is pulled out of NetEq.

BUG=webrtc:7467
Review-Url: https://codereview.webrtc.org/2851383004
Cr-Commit-Position: refs/heads/master@{#18014}
Committed: https://chromium.googlesource.com/external/webrtc/+/02739d9149c3b131703cc3df31c349afb9b097a5

[hlundin-webrtc, 2017-05-02 08:45:44]

henrik.lundin@webrtc.org changed reviewers:
+ alessiob@webrtc.org, ivoc@webrtc.org, kjellander@webrtc.org

[hlundin-webrtc, 2017-05-02 08:45:44]

henrik.lundin@webrtc.org changed required reviewers:
+ ivoc@webrtc.org, kjellander@webrtc.org

[hlundin-webrtc, 2017-05-02 08:45:45]

ivoc, alessiob: please, review it all.
kjellander: please, take a look at the trivial change in the fuzzer test.

Thanks!

[AleBzk, 2017-05-02 10:36:56]

LGTM with a few questions (see the comments).

Plus, I'm curious to know why there's no client code yet. In this CL, the
default NetEqTest::Callbacks instance (i.e., with nullptrs) is passed. I noticed
that there was no client code also for NetEqTestErrorCallback. Did you plan to
submit another CL which makes use of NetEqTest::Callbacks?

https://codereview.webrtc.org/2851383004/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/tools/neteq_test.cc (right):

https://codereview.webrtc.org/2851383004/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/tools/neteq_test.cc:72: if (error !=
NetEq::kOK && callbacks_.error_callback) {
For sure, it's fine as it is now. Just wondering if it'd be better (mainly for
readability) to explicitly check if pointers != nullptr. I found that this is
done elsewhere in webrtc (see
https://cs.chromium.org/search/?q=%22!%3D+nullptr%22+package:%5Echromium$+web...).

If you decide to change, also adapt the other if(callbacks_.*) statements below.

https://codereview.webrtc.org/2851383004/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/tools/neteq_test.cc:78: neteq_.get());
I wonder if there was an error for which a packet has not been inserted (not
sure if this can happen). If so, do you want to call the post_insert_packet
callback?

https://codereview.webrtc.org/2851383004/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/tools/neteq_test.h (right):

https://codereview.webrtc.org/2851383004/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/tools/neteq_test.h:85: Callbacks callbacks);
Copying callbacks is fine because it's just 3 pointers, right?

[ivoc, 2017-05-02 11:31:42]

Lgtm with two questions.

https://codereview.webrtc.org/2851383004/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/tools/neteq_test.h (right):

https://codereview.webrtc.org/2851383004/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/tools/neteq_test.h:43: virtual void
AfterInsertPacket(const NetEqInput::PacketData& packet,
Why no BeforeInsertPacket? Not useful?

https://codereview.webrtc.org/2851383004/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/tools/neteq_test.h:44: NetEq* neteq) = 0;
Shouldn't this be a const reference as well? Or is it expected that the NetEq
will be changed during the callback?

[kjellander_webrtc, 2017-05-02 17:52:46]

rs lgtm for webrtc/test/

[hlundin-webrtc, 2017-05-04 12:50:07]

Thanks for reviewing! I'm going to commit this as is.

https://codereview.webrtc.org/2851383004/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/tools/neteq_test.cc (right):

https://codereview.webrtc.org/2851383004/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/tools/neteq_test.cc:72: if (error !=
NetEq::kOK && callbacks_.error_callback) {
On 2017/05/02 10:36:55, AleBzk wrote:
> For sure, it's fine as it is now. Just wondering if it'd be better (mainly for
> readability) to explicitly check if pointers != nullptr. I found that this is
> done elsewhere in webrtc (see
>
https://cs.chromium.org/search/?q=%22!%3D+nullptr%22+package:%5Echromium$+web...).
> 
> If you decide to change, also adapt the other if(callbacks_.*) statements
below.

I think we have a fair amount of the shorter form too. I'd rather stick to that.

https://codereview.webrtc.org/2851383004/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/tools/neteq_test.cc:78: neteq_.get());
On 2017/05/02 10:36:55, AleBzk wrote:
> I wonder if there was an error for which a packet has not been inserted (not
> sure if this can happen). If so, do you want to call the post_insert_packet
> callback?

There could be. But I think we can leave that up to the callback object to sort
it out.

https://codereview.webrtc.org/2851383004/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/tools/neteq_test.h (right):

https://codereview.webrtc.org/2851383004/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/tools/neteq_test.h:43: virtual void
AfterInsertPacket(const NetEqInput::PacketData& packet,
On 2017/05/02 11:31:42, ivoc wrote:
> Why no BeforeInsertPacket? Not useful?

Not at the moment. Easy enough to add if needed.

https://codereview.webrtc.org/2851383004/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/tools/neteq_test.h:44: NetEq* neteq) = 0;
On 2017/05/02 11:31:42, ivoc wrote:
> Shouldn't this be a const reference as well? Or is it expected that the NetEq
> will be changed during the callback?

The callback object may want to poll NetEq for statistics, which is a non-const
operation.

https://codereview.webrtc.org/2851383004/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/tools/neteq_test.h:85: Callbacks callbacks);
On 2017/05/02 10:36:55, AleBzk wrote:
> Copying callbacks is fine because it's just 3 pointers, right?

Yes.

[hlundin-webrtc, 2017-05-04 12:50:12]

The CQ bit was checked by henrik.lundin@webrtc.org

[commit-bot: I haz the power, 2017-05-04 12:50:18]

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-04 13:09:07]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1493902212106940, "parent_rev":
"f25a22041c5b5043ba170dca3c0591bc714991d8", "commit_rev":
"02739d9149c3b131703cc3df31c349afb9b097a5"}

[commit-bot: I haz the power, 2017-05-04 13:09:10]

Description was changed from

==========
NetEqTest: Extend the callback structure

This change allows more callbacks to be registered to the test object.
The callbacks are used to give the user of the test object the ability
to instrument the test object. This CL specifically adds
instrumentation points just after a packet is inserted into NetEq, and
just after audio is pulled out of NetEq.

BUG=webrtc:7467
==========

to

==========
NetEqTest: Extend the callback structure

This change allows more callbacks to be registered to the test object.
The callbacks are used to give the user of the test object the ability
to instrument the test object. This CL specifically adds
instrumentation points just after a packet is inserted into NetEq, and
just after audio is pulled out of NetEq.

BUG=webrtc:7467

Review-Url: https://codereview.webrtc.org/2851383004
Cr-Commit-Position: refs/heads/master@{#18014}
Committed:
https://chromium.googlesource.com/external/webrtc/+/02739d9149c3b131703cc3df3...
==========

[commit-bot: I haz the power, 2017-05-04 13:09:11]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/external/webrtc/+/02739d9149c3b131703cc3df3...