Added a protobuf field for the audio processing module to store the status of experiments

Added a protobuf field for the audio processing module to store the status of temporary experimental features that
are active in the module and its submodules.

BUG=webrtc:5778, webrtc:5777
Committed: https://crrev.com/7789fe7ab1283fb4cbf6fa26d445b748d10928a7
Cr-Commit-Position: refs/heads/master@{#12371}

[peah-webrtc, 2016-04-14 06:54:32]

Description was changed from

==========
Added a protobuf field for the audio processing module to store the status
of temporary experimental features that active in the module and its
submodules.



BUG=webrtc:5778, webrtc:5777
==========

to

==========
Added a protobuf field for the audio processing module to store the status of
temporary experimental features that 
are active in the module and its submodules.

BUG=webrtc:5778, webrtc:5777
==========

[peah-webrtc, 2016-04-14 06:58:01]

peah@webrtc.org changed reviewers:
+ henrik.lundin@webrtc.org, minyue@webrtc.org

[hlundin-webrtc, 2016-04-14 07:50:07]

LG, but see comments.

https://codereview.webrtc.org/1886233003/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/audio_processing_impl.cc (right):

https://codereview.webrtc.org/1886233003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:1443:
experiments_description +=
You don't have to define the string empty first, and then append to it. Simply
do
std::string experiments_description =
public_submodules_->echo_cancellation->GetExperimentsDescription();

https://codereview.webrtc.org/1886233003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:1445: // TODO(peah):
Add semicolon separated concatenations of experiment
Language nit: "semicolon-separated" (with a hyphen).
Cf. https://en.wikipedia.org/wiki/Comma-separated_values

https://codereview.webrtc.org/1886233003/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/debug.proto (right):

https://codereview.webrtc.org/1886233003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/debug.proto:72: // Semicolon separated string
containing experimental feature
"Semicolon-separated"

https://codereview.webrtc.org/1886233003/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/echo_cancellation_impl.cc (right):

https://codereview.webrtc.org/1886233003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_cancellation_impl.cc:418: if
(aec3_enabled_) {
return aec3_enabled_ ? "AEC3" : "";

[minyue-webrtc, 2016-04-14 07:53:19]

On 2016/04/14 06:58:02, peah-webrtc wrote:

Good idea!

What missing now is how experiment affect APM settings. I think this should be
addressed in this CL. An objective can be that, it is clear and easy for
developer to add/change/remove experiment.

[peah-webrtc, 2016-04-14 08:58:32]

On 2016/04/14 07:53:19, minyue-webrtc wrote:
> On 2016/04/14 06:58:02, peah-webrtc wrote:
> 
> Good idea!
> 
> What missing now is how experiment affect APM settings. I think this should be
> addressed in this CL. An objective can be that, it is clear and easy for
> developer to add/change/remove experiment.

How do you mean activate the experiment? By using audioproc/audioproc_f or by
adding it in the submodule?

[minyue-webrtc, 2016-04-14 10:05:10]

On 2016/04/14 08:58:32, peah-webrtc wrote:
> On 2016/04/14 07:53:19, minyue-webrtc wrote:
> > On 2016/04/14 06:58:02, peah-webrtc wrote:
> > 
> > Good idea!
> > 
> > What missing now is how experiment affect APM settings. I think this should
be
> > addressed in this CL. An objective can be that, it is clear and easy for
> > developer to add/change/remove experiment.
> 
> How do you mean activate the experiment? By using audioproc/audioproc_f or by
> adding it in the submodule?

It would be great to show how the string can be used in replaying the dump. You
may just modify debug_dump_test.cc to illustrate it.

Also I'd prefer the string to be written and applied in a unified way. Now it
seems that one need to add module one-by-one at around line 1442 in
audio_processing_impl.cc

[peah-webrtc, 2016-04-14 11:11:06]

https://codereview.webrtc.org/1886233003/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/audio_processing_impl.cc (right):

https://codereview.webrtc.org/1886233003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:1443:
experiments_description +=
On 2016/04/14 07:50:07, hlundin-webrtc wrote:
> You don't have to define the string empty first, and then append to it. Simply
> do
> std::string experiments_description =
> public_submodules_->echo_cancellation->GetExperimentsDescription();
> 

Done.

https://codereview.webrtc.org/1886233003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:1445: // TODO(peah):
Add semicolon separated concatenations of experiment
On 2016/04/14 07:50:07, hlundin-webrtc wrote:
> Language nit: "semicolon-separated" (with a hyphen).
> Cf. https://en.wikipedia.org/wiki/Comma-separated_values

Done.

https://codereview.webrtc.org/1886233003/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/debug.proto (right):

https://codereview.webrtc.org/1886233003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/debug.proto:72: // Semicolon separated string
containing experimental feature
On 2016/04/14 07:50:07, hlundin-webrtc wrote:
> "Semicolon-separated"

Done.

https://codereview.webrtc.org/1886233003/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/echo_cancellation_impl.cc (right):

https://codereview.webrtc.org/1886233003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/echo_cancellation_impl.cc:418: if
(aec3_enabled_) {
On 2016/04/14 07:50:07, hlundin-webrtc wrote:
> return aec3_enabled_ ? "AEC3" : "";

Done.

[hlundin-webrtc, 2016-04-14 14:39:19]

lgtm

[minyue-webrtc, 2016-04-14 14:43:18]

On 2016/04/14 14:39:19, hlundin-webrtc wrote:
> lgtm

Per offline discussion, this CL looks good to me. But I'd like to see an update
on debug_dump_test.cc to see that the string is logged as it should.

[peah-webrtc, 2016-04-14 15:22:44]

On 2016/04/14 14:43:18, minyue-webrtc wrote:
> On 2016/04/14 14:39:19, hlundin-webrtc wrote:
> > lgtm
> 
> Per offline discussion, this CL looks good to me. But I'd like to see an
update
> on debug_dump_test.cc to see that the string is logged as it should.

I guess that you mean that we should ensure that the experiment is part of the
string. Makes sense. Will add that.

[peah-webrtc, 2016-04-15 04:03:50]

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

[commit-bot: I haz the power, 2016-04-15 04:03:55]

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

[commit-bot: I haz the power, 2016-04-15 05:15:24]

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

[commit-bot: I haz the power, 2016-04-15 05:15:25]

Dry run: This issue passed the CQ dry run.

[minyue-webrtc, 2016-04-15 07:55:37]

lgtm

[peah-webrtc, 2016-04-15 08:07:06]

The CQ bit was checked by peah@webrtc.org

[peah-webrtc, 2016-04-15 08:07:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
henrik.lundin@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1886233003/#ps40001 (title:
"Added unittest for reading of the added protobuf field")

[commit-bot: I haz the power, 2016-04-15 08:07:10]

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

[commit-bot: I haz the power, 2016-04-15 08:19:49]

Description was changed from

==========
Added a protobuf field for the audio processing module to store the status of
temporary experimental features that 
are active in the module and its submodules.

BUG=webrtc:5778, webrtc:5777
==========

to

==========
Added a protobuf field for the audio processing module to store the status of
temporary experimental features that 
are active in the module and its submodules.

BUG=webrtc:5778, webrtc:5777
==========

[commit-bot: I haz the power, 2016-04-15 08:19:51]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-15 08:19:54]

Description was changed from

==========
Added a protobuf field for the audio processing module to store the status of
temporary experimental features that 
are active in the module and its submodules.

BUG=webrtc:5778, webrtc:5777
==========

to

==========
Added a protobuf field for the audio processing module to store the status of
temporary experimental features that
are active in the module and its submodules.

BUG=webrtc:5778, webrtc:5777

Committed: https://crrev.com/7789fe7ab1283fb4cbf6fa26d445b748d10928a7
Cr-Commit-Position: refs/heads/master@{#12371}
==========

[commit-bot: I haz the power, 2016-04-15 08:19:55]

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