Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2103483002:
Add UMA stats for AEC filter divergence metric. (Closed)
|
Created:
4 years, 5 months ago by Henrik Grunell Modified:
4 years, 5 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, asvitkine+watch_chromium.org, posciak+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, mcasas+watch+vc_chromium.org, miu+watch_chromium.org, o1ka Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd UMA stats for AEC filter divergence metric.
Stats are reported when MediaStreamAudioProcessor::Stop() is called. This is typically when a WebRTC call is ended.
BUG=623607
Committed: https://crrev.com/89d302f64f2bfc4967ec88e0eecd6c4cebcd45d2
Cr-Commit-Position: refs/heads/master@{#402801}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Code review fixes (minyue@). #Patch Set 3 : Code review fix (minyue@). #
Total comments: 6
Patch Set 4 : Code review (tommi@). #Patch Set 5 : Fixed unit test. #
Messages
Total messages: 29 (11 generated)
Description was changed from ========== Add UMA stats for AEC filter divergence metric. Stats are reported when MediaStreamAudioProcessor::Stop() is called. This is typically when a WebRTC call is ended. BUG=623607 ========== to ========== Add UMA stats for AEC filter divergence metric. Stats are reported when MediaStreamAudioProcessor::Stop() is called. This is typically when a WebRTC call is ended. BUG=623607 ==========
grunell@chromium.org changed reviewers: + holte@chromium.org, tommi@chromium.org
tommi@: All holte@: histograms.xml
lgtm
grunell@chromium.org changed reviewers: + minyue@chromium.org
Minyue: please review.
Excellent work! See my minor comments. https://codereview.chromium.org/2103483002/diff/1/content/renderer/media/medi... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/2103483002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_audio_processor_options.cc:359: const int kNumChunksInOneSecond = 100; kNumChunksInOneSecond and kNumChunksInFiveSeconds can be calculated by kChunkDurationMs (can be defined in anonymous namespace) But if you take my suggestion of counting time, you need only kChunkDurationMs. https://codereview.chromium.org/2103483002/diff/1/content/renderer/media/medi... File content/renderer/media/media_stream_audio_processor_options.h (right): https://codereview.chromium.org/2103483002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_audio_processor_options.h:114: int chunks_for_delay_stats_; maybe better to count in time, e.g., time_for_delay_stats_ms if you still want to use a counter of chunks, maybe better to use type "size_t" https://codereview.chromium.org/2103483002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_audio_processor_options.h:120: int chunks_for_divergent_filter_stats_; maybe better to count in time, e.g., time_for_divergent_filter_stats_ms https://codereview.chromium.org/2103483002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2103483002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:62696: +<histogram name="WebRTC.AecDivergentFilterNonZeroPeriods" units="%"> "Periods" may be confusing The full name should be AecDivergentFilterFractionNonZeroPercentage It is obviously too complicated. Alternative 2: AecDivergentFilterPercentage This is too similar to AecDivergentFilterFraction and can make people think they are the same Alternative 3: AecFilterHasDivergencePercentage Emm, I think this is nicer https://codereview.chromium.org/2103483002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:62702: + once. A diverged filter could mean that there was echo experienced. The echo -> echo or ducking https://codereview.chromium.org/2103483002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:62704: + shorter lifetimes (i.e. no data available) is not recorded here. Have you checked data comes in right after 1 second? Which test can I run to test it?
https://codereview.chromium.org/2103483002/diff/1/content/renderer/media/medi... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/2103483002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_audio_processor_options.cc:359: const int kNumChunksInOneSecond = 100; On 2016/06/28 08:18:41, minyue wrote: > kNumChunksInOneSecond and kNumChunksInFiveSeconds can be calculated by > kChunkDurationMs (can be defined in anonymous namespace) > > But if you take my suggestion of counting time, you need only kChunkDurationMs. > Using time and kChunkDurationMs. https://codereview.chromium.org/2103483002/diff/1/content/renderer/media/medi... File content/renderer/media/media_stream_audio_processor_options.h (right): https://codereview.chromium.org/2103483002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_audio_processor_options.h:114: int chunks_for_delay_stats_; On 2016/06/28 08:18:41, minyue wrote: > maybe better to count in time, e.g., > > time_for_delay_stats_ms > > if you still want to use a counter of chunks, maybe better to use type "size_t" Yes, that's better. Done. https://codereview.chromium.org/2103483002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_audio_processor_options.h:120: int chunks_for_divergent_filter_stats_; On 2016/06/28 08:18:41, minyue wrote: > maybe better to count in time, e.g., > > time_for_divergent_filter_stats_ms Done. https://codereview.chromium.org/2103483002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2103483002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:62696: +<histogram name="WebRTC.AecDivergentFilterNonZeroPeriods" units="%"> On 2016/06/28 08:18:41, minyue wrote: > "Periods" may be confusing > > The full name should be > AecDivergentFilterFractionNonZeroPercentage > It is obviously too complicated. > > Alternative 2: AecDivergentFilterPercentage > This is too similar to AecDivergentFilterFraction and can make people think they > are the same > > Alternative 3: AecFilterHasDivergencePercentage > Emm, I think this is nicer I like (3), but I think it can be slimmed down to AecFilterDivergence. The unit ("Percentage") can be skipped in the name. It's seen in the histogram, thus redundant. Wdyt? Changed. https://codereview.chromium.org/2103483002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:62702: + once. A diverged filter could mean that there was echo experienced. The On 2016/06/28 08:18:41, minyue wrote: > echo -> echo or ducking Done. https://codereview.chromium.org/2103483002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:62704: + shorter lifetimes (i.e. no data available) is not recorded here. On 2016/06/28 08:18:42, minyue wrote: > Have you checked data comes in right after 1 second? Which test can I run to > test it? It doesn't come in until after >2 seconds, then it seems yo be updated every 1 second. You can use this CL and add CHECK_GE(metrics.divergent_filter_fraction, 0.0f); at line 371 in media_stream_audio_processor_options.cc. It will crash, and you'll need to increase the check period from 1 second to over 2 seconds to avoid it. I added a printf in the update of the fraction in AEC, and saw it was updated with an interval that seemed to be 1 second.
https://codereview.chromium.org/2103483002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2103483002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:62696: +<histogram name="WebRTC.AecDivergentFilterNonZeroPeriods" units="%"> On 2016/06/28 09:33:10, Henrik Grunell wrote: > On 2016/06/28 08:18:41, minyue wrote: > > "Periods" may be confusing > > > > The full name should be > > AecDivergentFilterFractionNonZeroPercentage > > It is obviously too complicated. > > > > Alternative 2: AecDivergentFilterPercentage > > This is too similar to AecDivergentFilterFraction and can make people think > they > > are the same > > > > Alternative 3: AecFilterHasDivergencePercentage > > Emm, I think this is nicer > > I like (3), but I think it can be slimmed down to AecFilterDivergence. The unit > ("Percentage") can be skipped in the name. It's seen in the histogram, thus > redundant. Wdyt? Changed. Yes, it is good to remove percentage. But I am not very much in favor of removing "Has", since FilterDivergence% means how much time it diverged, and with "Has", it means whether it is showed divergence. The latter implies that we do not continuously check divergence, and we only check "whether it had divergence in past 1 second".
https://codereview.chromium.org/2103483002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2103483002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:62696: +<histogram name="WebRTC.AecDivergentFilterNonZeroPeriods" units="%"> On 2016/06/28 10:01:26, minyue wrote: > On 2016/06/28 09:33:10, Henrik Grunell wrote: > > On 2016/06/28 08:18:41, minyue wrote: > > > "Periods" may be confusing > > > > > > The full name should be > > > AecDivergentFilterFractionNonZeroPercentage > > > It is obviously too complicated. > > > > > > Alternative 2: AecDivergentFilterPercentage > > > This is too similar to AecDivergentFilterFraction and can make people think > > they > > > are the same > > > > > > Alternative 3: AecFilterHasDivergencePercentage > > > Emm, I think this is nicer > > > > I like (3), but I think it can be slimmed down to AecFilterDivergence. The > unit > > ("Percentage") can be skipped in the name. It's seen in the histogram, thus > > redundant. Wdyt? Changed. > > Yes, it is good to remove percentage. But I am not very much in favor of > removing "Has", since FilterDivergence% means how much time it diverged, and > with "Has", it means whether it is showed divergence. The latter implies that we > do not continuously check divergence, and we only check "whether it had > divergence in past 1 second". That makes sense. Done.
lgtm
Tommi?
https://codereview.chromium.org/2103483002/diff/40001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/2103483002/diff/40001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:53: const int kChunkDurationMs = 10; If this value is defined somewhere else (as I suspect), it would be good to use that constant instead of adding a new one. https://codereview.chromium.org/2103483002/diff/40001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:385: void EchoInformation::ReportAndResetAecDivergentFilterStats() { can you add thread checks to these methods? As is, it's not clear to me that they're all called on the same thread. https://codereview.chromium.org/2103483002/diff/40001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_options.h (right): https://codereview.chromium.org/2103483002/diff/40001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.h:109: void UpdateAecDivergentFilterStats( Do we need two methods?
Patchset #4 (id:60001) has been deleted
https://codereview.chromium.org/2103483002/diff/40001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_options.cc (right): https://codereview.chromium.org/2103483002/diff/40001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:53: const int kChunkDurationMs = 10; On 2016/06/28 11:43:56, tommi-chrömium wrote: > If this value is defined somewhere else (as I suspect), it would be good to use > that constant instead of adding a new one. It is! In AudioProcessing. :) Done. https://codereview.chromium.org/2103483002/diff/40001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.cc:385: void EchoInformation::ReportAndResetAecDivergentFilterStats() { On 2016/06/28 11:43:56, tommi-chrömium wrote: > can you add thread checks to these methods? As is, it's not clear to me that > they're all called on the same thread. Hmm, they are not accessed on the same thread. That should be safe as it and MSAP is used now, but needs to be guaranteed. Actually it looks like the existing code is problematic, since we access |echo_information_| in MSAP on two different threads. We could have a use after free. I've changed to posting the update to the main thread, where the new reporting function is called. Now |echo_information_| is only accessed on the main render thread. I've also updated the comments around this, as they seemed stale. MSAP assumes it's created on the main render thread (that thread checker is not detached in the ctor), so I've done that as well to get the message loop in the ctor. https://codereview.chromium.org/2103483002/diff/40001/content/renderer/media/... File content/renderer/media/media_stream_audio_processor_options.h (right): https://codereview.chromium.org/2103483002/diff/40001/content/renderer/media/... content/renderer/media/media_stream_audio_processor_options.h:109: void UpdateAecDivergentFilterStats( On 2016/06/28 11:43:56, tommi-chrömium wrote: > Do we need two methods? No, done. Kept them internally.
lgtm
The CQ bit was checked by grunell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from holte@chromium.org, minyue@chromium.org Link to the patchset: https://codereview.chromium.org/2103483002/#ps80001 (title: "Code review (tommi@).")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by grunell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from minyue@chromium.org, tommi@chromium.org, holte@chromium.org Link to the patchset: https://codereview.chromium.org/2103483002/#ps90001 (title: "Fixed unit test.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add UMA stats for AEC filter divergence metric. Stats are reported when MediaStreamAudioProcessor::Stop() is called. This is typically when a WebRTC call is ended. BUG=623607 ========== to ========== Add UMA stats for AEC filter divergence metric. Stats are reported when MediaStreamAudioProcessor::Stop() is called. This is typically when a WebRTC call is ended. BUG=623607 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:90001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Add UMA stats for AEC filter divergence metric. Stats are reported when MediaStreamAudioProcessor::Stop() is called. This is typically when a WebRTC call is ended. BUG=623607 ========== to ========== Add UMA stats for AEC filter divergence metric. Stats are reported when MediaStreamAudioProcessor::Stop() is called. This is typically when a WebRTC call is ended. BUG=623607 Committed: https://crrev.com/89d302f64f2bfc4967ec88e0eecd6c4cebcd45d2 Cr-Commit-Position: refs/heads/master@{#402801} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/89d302f64f2bfc4967ec88e0eecd6c4cebcd45d2 Cr-Commit-Position: refs/heads/master@{#402801} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
