Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2433153003:
New statistics interface for APM (Closed)
|
Created:
4 years, 2 months ago by ivoc Modified:
4 years, 1 month ago CC:
webrtc-reviews_webrtc.org, peah-webrtc, Andrew MacDonald, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc, minyue-webrtc, aluebs-webrtc, bjornv1 Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionNew statistics interface for APM
This adds functions to enable and retrieve statistics from APM. These functions are not yet called, which will happen in a follow-up CL.
BUG=webrtc:6525
Committed: https://crrev.com/8b8d3e4c30e8ea3846b58dfd36d1fd35a7799df4
Cr-Commit-Position: refs/heads/master@{#14810}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Review comments. #
Total comments: 11
Patch Set 3 : More comments. #
Total comments: 1
Patch Set 4 : Added const. #Patch Set 5 : Rebase. #Patch Set 6 : Fixed unittest that checks that EC metrics are disabled. #Patch Set 7 : Fix for Android crashes. #Patch Set 8 : Rebase. #Patch Set 9 : Fix for mock #Patch Set 10 : Another fix for a failing testcase. #Patch Set 11 : Another test fix. #Messages
Total messages: 42 (17 generated)
ivoc@webrtc.org changed reviewers: + peah@webrtc.org, solenberg@webrtc.org
Hi guys, please have a look at this CL to add a unified function to get all APM statistics to the APM interface. I didn't modify the calling code yet, that will happen in a follow-up CL.
Nice!!! lgtm but with one question for the author as well as the other reviewers (also posted as a comment): Do we really need to enable metrics collection? Can't it always be on? https://codereview.chromium.org/2433153003/diff/1/webrtc/modules/audio_proces... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.chromium.org/2433153003/diff/1/webrtc/modules/audio_proces... webrtc/modules/audio_processing/include/audio_processing.h:509: virtual void SetStatisticsEnabled(bool enabled) = 0; Do we need to have an enable statistics method? Can't we just always compute statistics? They should anyway not be anything computationally complex.
https://codereview.chromium.org/2433153003/diff/1/webrtc/modules/audio_proces... File webrtc/modules/audio_processing/audio_processing_impl.h (right): https://codereview.chromium.org/2433153003/diff/1/webrtc/modules/audio_proces... webrtc/modules/audio_processing/audio_processing_impl.h:118: void GetStatistics(AudioProcessingStatistics* stats) const override; How about just returning this struct by value? Return value optimization is here to help. https://codereview.chromium.org/2433153003/diff/1/webrtc/modules/audio_proces... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.chromium.org/2433153003/diff/1/webrtc/modules/audio_proces... webrtc/modules/audio_processing/include/audio_processing.h:473: int instant; // Instantaneous value. Since you're changing the API, I think it'd be better if these were floats. Also, I think you should make them values private and have a setter, so that we can enforce the invariant (instant, avg should lie within min/max). Please also default init to 0.0f. https://codereview.chromium.org/2433153003/diff/1/webrtc/modules/audio_proces... webrtc/modules/audio_processing/include/audio_processing.h:481: Statistic residual_echo_return_loss = {0, 0, 0, 0}; With default init, no need for this (error-prone) manual initing. https://codereview.chromium.org/2433153003/diff/1/webrtc/modules/audio_proces... webrtc/modules/audio_processing/include/audio_processing.h:509: virtual void SetStatisticsEnabled(bool enabled) = 0; On 2016/10/21 07:02:45, peah-webrtc wrote: > Do we need to have an enable statistics method? Can't we just always compute > statistics? They should anyway not be anything computationally complex. Agree, but are you sure they're not expensive?
https://codereview.chromium.org/2433153003/diff/1/webrtc/modules/audio_proces... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.chromium.org/2433153003/diff/1/webrtc/modules/audio_proces... webrtc/modules/audio_processing/include/audio_processing.h:509: virtual void SetStatisticsEnabled(bool enabled) = 0; On 2016/10/24 08:26:58, the sun wrote: > On 2016/10/21 07:02:45, peah-webrtc wrote: > > Do we need to have an enable statistics method? Can't we just always compute > > statistics? They should anyway not be anything computationally complex. > > Agree, but are you sure they're not expensive? Almost, yes, in particular since -The AEC metrics are not run on mobile platforms and their complexity should be negligible to the complexity of the AEC. -The residual echo detector metrics are only run when the residual echo detector is on, in which case they are obtained for free. The question is, however a bit bigger than that I think since it also applies on future metrics, will they be too complex? For a metric, I think we should be able to rely on it being extremely lightweight. Does that make sense?
On 2016/10/24 09:44:38, peah-webrtc wrote: > https://codereview.chromium.org/2433153003/diff/1/webrtc/modules/audio_proces... > File webrtc/modules/audio_processing/include/audio_processing.h (right): > > https://codereview.chromium.org/2433153003/diff/1/webrtc/modules/audio_proces... > webrtc/modules/audio_processing/include/audio_processing.h:509: virtual void > SetStatisticsEnabled(bool enabled) = 0; > On 2016/10/24 08:26:58, the sun wrote: > > On 2016/10/21 07:02:45, peah-webrtc wrote: > > > Do we need to have an enable statistics method? Can't we just always compute > > > statistics? They should anyway not be anything computationally complex. > > > > Agree, but are you sure they're not expensive? > > Almost, yes, in particular since > -The AEC metrics are not run on mobile platforms and their complexity should be > negligible to the complexity of the AEC. > -The residual echo detector metrics are only run when the residual echo detector > is on, in which case they are obtained for free. > > The question is, however a bit bigger than that I think since it also applies on > future metrics, will they be too complex? For a metric, I think we should be > able to rely on it being extremely lightweight. > > Does that make sense? To me that makes sense.
Thanks for the comments. I removed the enable/disable function, so stats are now always enabled. See below for other changes. https://codereview.chromium.org/2433153003/diff/1/webrtc/modules/audio_proces... File webrtc/modules/audio_processing/audio_processing_impl.h (right): https://codereview.chromium.org/2433153003/diff/1/webrtc/modules/audio_proces... webrtc/modules/audio_processing/audio_processing_impl.h:118: void GetStatistics(AudioProcessingStatistics* stats) const override; On 2016/10/24 08:26:58, the sun wrote: > How about just returning this struct by value? Return value optimization is here > to help. Done. https://codereview.chromium.org/2433153003/diff/1/webrtc/modules/audio_proces... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.chromium.org/2433153003/diff/1/webrtc/modules/audio_proces... webrtc/modules/audio_processing/include/audio_processing.h:473: int instant; // Instantaneous value. On 2016/10/24 08:26:58, the sun wrote: > Since you're changing the API, I think it'd be better if these were floats. > Also, I think you should make them values private and have a setter, so that we > can enforce the invariant (instant, avg should lie within min/max). > > Please also default init to 0.0f. > I'm actually trying not to change the old statistics API, since it's used in a lot of places. So instead of changing this struct I will add a new Statistic struct to AudioProcessingStatics that behaves like you suggest (I named it ValueStatistic, not sure if that makes sense), and leave this one in place for the old API for now. Once the code that uses the statistics is updated we can get rid of the old API and this struct as well. I made one setter for all 4 values, otherwise the order of calling the setters would determine if the invariants hold, which is not nice. https://codereview.chromium.org/2433153003/diff/1/webrtc/modules/audio_proces... webrtc/modules/audio_processing/include/audio_processing.h:481: Statistic residual_echo_return_loss = {0, 0, 0, 0}; On 2016/10/24 08:26:58, the sun wrote: > With default init, no need for this (error-prone) manual initing. Good idea. https://codereview.chromium.org/2433153003/diff/1/webrtc/modules/audio_proces... webrtc/modules/audio_processing/include/audio_processing.h:509: virtual void SetStatisticsEnabled(bool enabled) = 0; On 2016/10/24 09:44:38, peah-webrtc wrote: > On 2016/10/24 08:26:58, the sun wrote: > > On 2016/10/21 07:02:45, peah-webrtc wrote: > > > Do we need to have an enable statistics method? Can't we just always compute > > > statistics? They should anyway not be anything computationally complex. > > > > Agree, but are you sure they're not expensive? > > Almost, yes, in particular since > -The AEC metrics are not run on mobile platforms and their complexity should be > negligible to the complexity of the AEC. > -The residual echo detector metrics are only run when the residual echo detector > is on, in which case they are obtained for free. > > The question is, however a bit bigger than that I think since it also applies on > future metrics, will they be too complex? For a metric, I think we should be > able to rely on it being extremely lightweight. > > Does that make sense? > > > Ok, I removed this function.
lgtm % comments https://codereview.chromium.org/2433153003/diff/20001/webrtc/modules/audio_pr... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.chromium.org/2433153003/diff/20001/webrtc/modules/audio_pr... webrtc/modules/audio_processing/include/audio_processing.h:481: struct ValueStatistic { How about "Stat"? I'm thinking in the future we may want to templatize the type (Statzfloat>, Stat<int>, etc) an then this will feel unnecessarily long. Don't make it a template right now. https://codereview.chromium.org/2433153003/diff/20001/webrtc/modules/audio_pr... webrtc/modules/audio_processing/include/audio_processing.h:483: ValueStatistic(ValueStatistic& other) { why not "default"? https://codereview.chromium.org/2433153003/diff/20001/webrtc/modules/audio_pr... webrtc/modules/audio_processing/include/audio_processing.h:489: ValueStatistic(Statistic& other) { Make the conversion explicit https://codereview.chromium.org/2433153003/diff/20001/webrtc/modules/audio_pr... webrtc/modules/audio_processing/include/audio_processing.h:496: void SetValues(float instant, float average, float maximum, float minimum) { nit: Just "Set"
https://codereview.chromium.org/2433153003/diff/20001/webrtc/modules/audio_pr... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.chromium.org/2433153003/diff/20001/webrtc/modules/audio_pr... webrtc/modules/audio_processing/include/audio_processing.h:481: struct ValueStatistic { On 2016/10/24 15:15:43, the sun wrote: > How about "Stat"? > > I'm thinking in the future we may want to templatize the type (Statzfloat>, > Stat<int>, etc) an then this will feel unnecessarily long. Don't make it a > template right now. Sounds good. https://codereview.chromium.org/2433153003/diff/20001/webrtc/modules/audio_pr... webrtc/modules/audio_processing/include/audio_processing.h:483: ValueStatistic(ValueStatistic& other) { On 2016/10/24 15:15:44, the sun wrote: > why not "default"? Turns out it's not actually necessary to define this or the constructor at all. I got a message about it previously, but with the current changes it seems to no longer be needed. https://codereview.chromium.org/2433153003/diff/20001/webrtc/modules/audio_pr... webrtc/modules/audio_processing/include/audio_processing.h:489: ValueStatistic(Statistic& other) { On 2016/10/24 15:15:43, the sun wrote: > Make the conversion explicit I changed this into another Set function that takes an old Statistics object, is that okay? https://codereview.chromium.org/2433153003/diff/20001/webrtc/modules/audio_pr... webrtc/modules/audio_processing/include/audio_processing.h:496: void SetValues(float instant, float average, float maximum, float minimum) { On 2016/10/24 15:15:44, the sun wrote: > nit: Just "Set" Done.
https://codereview.webrtc.org/2433153003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2433153003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:483: ValueStatistic(ValueStatistic& other) { On 2016/10/24 15:50:42, ivoc wrote: > On 2016/10/24 15:15:44, the sun wrote: > > why not "default"? > > Turns out it's not actually necessary to define this or the constructor at all. > I got a message about it previously, but with the current changes it seems to no > longer be needed. have you tried building everywhere? clang complains if the default methods generate too much code - in which case you may need to declare them and put the definition in the .cc https://codereview.webrtc.org/2433153003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:489: ValueStatistic(Statistic& other) { On 2016/10/24 15:50:42, ivoc wrote: > On 2016/10/24 15:15:43, the sun wrote: > > Make the conversion explicit > > I changed this into another Set function that takes an old Statistics object, is > that okay? sgtm
https://codereview.webrtc.org/2433153003/diff/40001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2433153003/diff/40001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:482: void Set(Statistic& other) { const &
https://codereview.webrtc.org/2433153003/diff/20001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/include/audio_processing.h (right): https://codereview.webrtc.org/2433153003/diff/20001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/include/audio_processing.h:483: ValueStatistic(ValueStatistic& other) { On 2016/10/25 06:44:12, the sun wrote: > On 2016/10/24 15:50:42, ivoc wrote: > > On 2016/10/24 15:15:44, the sun wrote: > > > why not "default"? > > > > Turns out it's not actually necessary to define this or the constructor at > all. > > I got a message about it previously, but with the current changes it seems to > no > > longer be needed. > > have you tried building everywhere? clang complains if the default methods > generate too much code - in which case you may need to declare them and put the > definition in the .cc It seems to build fine on the trybots, so I guess the class is small enough that this is not an issue.
The CQ bit was checked by ivoc@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from peah@webrtc.org, solenberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2433153003/#ps80001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_baremetal/builds/...) mac_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/15498)
The CQ bit was checked by ivoc@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org, peah@webrtc.org Link to the patchset: https://codereview.webrtc.org/2433153003/#ps100001 (title: "Fixed unittest that checks that EC metrics are disabled.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...) linux_compile_dbg on master.tryserver.webrtc (JOB_FAILED, no build URL)
The CQ bit was checked by ivoc@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/17938)
The CQ bit was checked by ivoc@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org, peah@webrtc.org Link to the patchset: https://codereview.webrtc.org/2433153003/#ps180001 (title: "Another fix for a failing testcase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/2499)
The CQ bit was checked by ivoc@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org, peah@webrtc.org Link to the patchset: https://codereview.webrtc.org/2433153003/#ps200001 (title: "Another test fix.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/2524)
The CQ bit was checked by ivoc@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== New statistics interface for APM This adds functions to enable and retrieve statistics from APM. These functions are not yet called, which will happen in a follow-up CL. BUG=webrtc:6525 ========== to ========== New statistics interface for APM This adds functions to enable and retrieve statistics from APM. These functions are not yet called, which will happen in a follow-up CL. BUG=webrtc:6525 Committed: https://crrev.com/8b8d3e4c30e8ea3846b58dfd36d1fd35a7799df4 Cr-Commit-Position: refs/heads/master@{#14810} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/8b8d3e4c30e8ea3846b58dfd36d1fd35a7799df4 Cr-Commit-Position: refs/heads/master@{#14810}
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:200001) has been created in https://codereview.webrtc.org/2456333002/ by ivoc@webrtc.org. The reason for reverting is: This CL breaks internal dependencies.. |
