From 48dfab5c58119a4e65c52506ed55f8de79725bcf Mon Sep 17 00:00:00 2001 From: ivoc Date: Fri, 28 Oct 2016 03:29:31 -0700 Subject: [PATCH] Revert of New statistics interface for APM (patchset #11 id:200001 of https://codereview.webrtc.org/2433153003/ ) Reason for revert: This CL breaks internal dependencies. Original issue's description: > 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} TBR=peah@webrtc.org,solenberg@webrtc.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=webrtc:6525 Review-Url: https://codereview.webrtc.org/2456333002 Cr-Commit-Position: refs/heads/master@{#14814} --- .../audio_processing/audio_processing_impl.cc | 20 ----- .../audio_processing/audio_processing_impl.h | 2 - .../audio_processing_unittest.cc | 20 ++--- .../echo_cancellation_impl.cc | 4 +- .../audio_processing/echo_cancellation_impl.h | 20 ++--- .../include/audio_processing.h | 79 ++----------------- .../include/mock_audio_processing.h | 1 - .../auto_test/extended/ec_metrics_test.cc | 12 +-- 8 files changed, 26 insertions(+), 132 deletions(-) diff --git a/webrtc/modules/audio_processing/audio_processing_impl.cc b/webrtc/modules/audio_processing/audio_processing_impl.cc index b1a41934a0..acbc80c9dd 100644 --- a/webrtc/modules/audio_processing/audio_processing_impl.cc +++ b/webrtc/modules/audio_processing/audio_processing_impl.cc @@ -440,10 +440,6 @@ int AudioProcessingImpl::InitializeLocked() { num_proc_channels()); AllocateRenderQueue(); - int success = public_submodules_->echo_cancellation->enable_metrics(true); - RTC_DCHECK_EQ(0, success); - success = public_submodules_->echo_cancellation->enable_delay_logging(true); - RTC_DCHECK_EQ(0, success); public_submodules_->echo_control_mobile->Initialize( proc_split_sample_rate_hz(), num_reverse_channels(), num_output_channels()); @@ -1422,22 +1418,6 @@ int AudioProcessingImpl::StopDebugRecording() { #endif // WEBRTC_AUDIOPROC_DEBUG_DUMP } -AudioProcessing::AudioProcessingStatistics AudioProcessingImpl::GetStatistics() - const { - AudioProcessingStatistics stats; - EchoCancellation::Metrics metrics; - public_submodules_->echo_cancellation->GetMetrics(&metrics); - stats.a_nlp.Set(metrics.a_nlp); - stats.divergent_filter_fraction = metrics.divergent_filter_fraction; - stats.echo_return_loss.Set(metrics.echo_return_loss); - stats.echo_return_loss_enhancement.Set(metrics.echo_return_loss_enhancement); - stats.residual_echo_return_loss.Set(metrics.residual_echo_return_loss); - public_submodules_->echo_cancellation->GetDelayMetrics( - &stats.delay_median, &stats.delay_standard_deviation, - &stats.fraction_poor_delays); - return stats; -} - EchoCancellation* AudioProcessingImpl::echo_cancellation() const { return public_submodules_->echo_cancellation.get(); } diff --git a/webrtc/modules/audio_processing/audio_processing_impl.h b/webrtc/modules/audio_processing/audio_processing_impl.h index eb14308301..aec4ed7086 100644 --- a/webrtc/modules/audio_processing/audio_processing_impl.h +++ b/webrtc/modules/audio_processing/audio_processing_impl.h @@ -116,8 +116,6 @@ class AudioProcessingImpl : public AudioProcessing { bool was_stream_delay_set() const override EXCLUSIVE_LOCKS_REQUIRED(crit_capture_); - AudioProcessingStatistics GetStatistics() const override; - // Methods returning pointers to APM submodules. // No locks are aquired in those, as those locks // would offer no protection (the submodules are diff --git a/webrtc/modules/audio_processing/audio_processing_unittest.cc b/webrtc/modules/audio_processing/audio_processing_unittest.cc index f865c87c3b..d4faa81205 100644 --- a/webrtc/modules/audio_processing/audio_processing_unittest.cc +++ b/webrtc/modules/audio_processing/audio_processing_unittest.cc @@ -929,9 +929,6 @@ TEST_F(ApmTest, EchoCancellation) { EXPECT_EQ(apm_->kNotEnabledError, apm_->echo_cancellation()->GetMetrics(&metrics)); - EXPECT_EQ(apm_->kNoError, apm_->echo_cancellation()->Enable(true)); - EXPECT_TRUE(apm_->echo_cancellation()->is_enabled()); - EXPECT_EQ(apm_->kNoError, apm_->echo_cancellation()->enable_metrics(true)); EXPECT_TRUE(apm_->echo_cancellation()->are_metrics_enabled()); @@ -939,16 +936,6 @@ TEST_F(ApmTest, EchoCancellation) { apm_->echo_cancellation()->enable_metrics(false)); EXPECT_FALSE(apm_->echo_cancellation()->are_metrics_enabled()); - EXPECT_EQ(apm_->kNoError, - apm_->echo_cancellation()->enable_delay_logging(true)); - EXPECT_TRUE(apm_->echo_cancellation()->is_delay_logging_enabled()); - EXPECT_EQ(apm_->kNoError, - apm_->echo_cancellation()->enable_delay_logging(false)); - EXPECT_FALSE(apm_->echo_cancellation()->is_delay_logging_enabled()); - - EXPECT_EQ(apm_->kNoError, apm_->echo_cancellation()->Enable(false)); - EXPECT_FALSE(apm_->echo_cancellation()->is_enabled()); - int median = 0; int std = 0; float poor_fraction = 0; @@ -956,6 +943,13 @@ TEST_F(ApmTest, EchoCancellation) { apm_->echo_cancellation()->GetDelayMetrics(&median, &std, &poor_fraction)); + EXPECT_EQ(apm_->kNoError, + apm_->echo_cancellation()->enable_delay_logging(true)); + EXPECT_TRUE(apm_->echo_cancellation()->is_delay_logging_enabled()); + EXPECT_EQ(apm_->kNoError, + apm_->echo_cancellation()->enable_delay_logging(false)); + EXPECT_FALSE(apm_->echo_cancellation()->is_delay_logging_enabled()); + EXPECT_EQ(apm_->kNoError, apm_->echo_cancellation()->Enable(true)); EXPECT_TRUE(apm_->echo_cancellation()->is_enabled()); EXPECT_EQ(apm_->kNoError, apm_->echo_cancellation()->Enable(false)); diff --git a/webrtc/modules/audio_processing/echo_cancellation_impl.cc b/webrtc/modules/audio_processing/echo_cancellation_impl.cc index 0b8fc141ee..e06b2faa84 100644 --- a/webrtc/modules/audio_processing/echo_cancellation_impl.cc +++ b/webrtc/modules/audio_processing/echo_cancellation_impl.cc @@ -270,7 +270,7 @@ int EchoCancellationImpl::enable_metrics(bool enable) { bool EchoCancellationImpl::are_metrics_enabled() const { rtc::CritScope cs(crit_capture_); - return enabled_ && metrics_enabled_; + return metrics_enabled_; } // TODO(ajm): we currently just use the metrics from the first AEC. Think more @@ -333,7 +333,7 @@ int EchoCancellationImpl::enable_delay_logging(bool enable) { bool EchoCancellationImpl::is_delay_logging_enabled() const { rtc::CritScope cs(crit_capture_); - return enabled_ && delay_logging_enabled_; + return delay_logging_enabled_; } bool EchoCancellationImpl::is_delay_agnostic_enabled() const { diff --git a/webrtc/modules/audio_processing/echo_cancellation_impl.h b/webrtc/modules/audio_processing/echo_cancellation_impl.h index 03be98529a..8a4ea71846 100644 --- a/webrtc/modules/audio_processing/echo_cancellation_impl.h +++ b/webrtc/modules/audio_processing/echo_cancellation_impl.h @@ -58,20 +58,6 @@ class EchoCancellationImpl : public EchoCancellation { static size_t NumCancellersRequired(size_t num_output_channels, size_t num_reverse_channels); - // Enable logging of various AEC statistics. - int enable_metrics(bool enable) override; - - // Provides various statistics about the AEC. - int GetMetrics(Metrics* metrics) override; - - // Enable logging of delay metrics. - int enable_delay_logging(bool enable) override; - - // Provides delay metrics. - int GetDelayMetrics(int* median, - int* std, - float* fraction_poor_delays) override; - private: class Canceller; struct StreamProperties; @@ -81,10 +67,16 @@ class EchoCancellationImpl : public EchoCancellation { int enable_drift_compensation(bool enable) override; void set_stream_drift_samples(int drift) override; int set_suppression_level(SuppressionLevel level) override; + int enable_metrics(bool enable) override; bool are_metrics_enabled() const override; bool stream_has_echo() const override; + int GetMetrics(Metrics* metrics) override; + int enable_delay_logging(bool enable) override; bool is_delay_logging_enabled() const override; int GetDelayMetrics(int* median, int* std) override; + int GetDelayMetrics(int* median, + int* std, + float* fraction_poor_delays) override; struct AecCore* aec_core() const override; diff --git a/webrtc/modules/audio_processing/include/audio_processing.h b/webrtc/modules/audio_processing/include/audio_processing.h index d50c93d6e6..0fdbfd2d75 100644 --- a/webrtc/modules/audio_processing/include/audio_processing.h +++ b/webrtc/modules/audio_processing/include/audio_processing.h @@ -469,74 +469,6 @@ class AudioProcessing { // specific member variables are reset. virtual void UpdateHistogramsOnCallEnd() = 0; - // TODO(ivoc): Remove when the calling code no longer uses the old Statistics - // API. - struct Statistic { - int instant = 0; // Instantaneous value. - int average = 0; // Long-term average. - int maximum = 0; // Long-term maximum. - int minimum = 0; // Long-term minimum. - }; - - struct Stat { - void Set(const Statistic& other) { - Set(other.instant, other.average, other.maximum, other.minimum); - } - void Set(float instant, float average, float maximum, float minimum) { - RTC_DCHECK_LE(instant, maximum); - RTC_DCHECK_GE(instant, minimum); - RTC_DCHECK_LE(average, maximum); - RTC_DCHECK_GE(average, minimum); - instant_ = instant; - average_ = average; - maximum_ = maximum; - minimum_ = minimum; - } - float instant() const { return instant_; } - float average() const { return average_; } - float maximum() const { return maximum_; } - float minimum() const { return minimum_; } - - private: - float instant_ = 0.0f; // Instantaneous value. - float average_ = 0.0f; // Long-term average. - float maximum_ = 0.0f; // Long-term maximum. - float minimum_ = 0.0f; // Long-term minimum. - }; - - struct AudioProcessingStatistics { - // AEC Statistics. - // RERL = ERL + ERLE - Stat residual_echo_return_loss; - // ERL = 10log_10(P_far / P_echo) - Stat echo_return_loss; - // ERLE = 10log_10(P_echo / P_out) - Stat echo_return_loss_enhancement; - // (Pre non-linear processing suppression) A_NLP = 10log_10(P_echo / P_a) - Stat a_nlp; - // Fraction of time that the AEC linear filter is divergent, in a 1-second - // non-overlapped aggregation window. - float divergent_filter_fraction = 0.0f; - - // The delay metrics consists of the delay median and standard deviation. It - // also consists of the fraction of delay estimates that can make the echo - // cancellation perform poorly. The values are aggregated until the first - // call to |GetStatistics()| and afterwards aggregated and updated every - // second. Note that if there are several clients pulling metrics from - // |GetStatistics()| during a session the first call from any of them will - // change to one second aggregation window for all. - int delay_median = 0; - int delay_standard_deviation = 0; - float fraction_poor_delays = 0.0f; - - // Residual echo detector likelihood. This value is not yet calculated and - // is currently always set to zero. - // TODO(ivoc): Implement this stat. - float residual_echo_likelihood = 0.0f; - }; - - virtual AudioProcessingStatistics GetStatistics() const = 0; - // These provide access to the component interfaces and should never return // NULL. The pointers will be valid for the lifetime of the APM instance. // The memory for these objects is entirely managed internally. @@ -548,6 +480,13 @@ class AudioProcessing { virtual NoiseSuppression* noise_suppression() const = 0; virtual VoiceDetection* voice_detection() const = 0; + struct Statistic { + int instant; // Instantaneous value. + int average; // Long-term average. + int maximum; // Long-term maximum. + int minimum; // Long-term minimum. + }; + enum Error { // Fatal errors. kNoError = 0, @@ -769,7 +708,6 @@ class EchoCancellation { float divergent_filter_fraction; }; - // Deprecated. Use GetStatistics on the AudioProcessing interface instead. // TODO(ajm): discuss the metrics update period. virtual int GetMetrics(Metrics* metrics) = 0; @@ -786,9 +724,8 @@ class EchoCancellation { // Note that if there are several clients pulling metrics from // |GetDelayMetrics()| during a session the first call from any of them will // change to one second aggregation window for all. - // Deprecated. Use GetStatistics on the AudioProcessing interface instead. + // TODO(bjornv): Deprecated, remove. virtual int GetDelayMetrics(int* median, int* std) = 0; - // Deprecated. Use GetStatistics on the AudioProcessing interface instead. virtual int GetDelayMetrics(int* median, int* std, float* fraction_poor_delays) = 0; diff --git a/webrtc/modules/audio_processing/include/mock_audio_processing.h b/webrtc/modules/audio_processing/include/mock_audio_processing.h index b5a1fa4a88..8acb83ba23 100644 --- a/webrtc/modules/audio_processing/include/mock_audio_processing.h +++ b/webrtc/modules/audio_processing/include/mock_audio_processing.h @@ -183,7 +183,6 @@ class MockAudioProcessing : public AudioProcessing { int(rtc::PlatformFile handle)); MOCK_METHOD0(StopDebugRecording, int()); MOCK_METHOD0(UpdateHistogramsOnCallEnd, void()); - MOCK_CONST_METHOD0(GetStatistics, AudioProcessingStatistics()); virtual MockEchoCancellation* echo_cancellation() const { return echo_cancellation_.get(); } diff --git a/webrtc/voice_engine/test/auto_test/extended/ec_metrics_test.cc b/webrtc/voice_engine/test/auto_test/extended/ec_metrics_test.cc index 758a0850ed..a202aea280 100644 --- a/webrtc/voice_engine/test/auto_test/extended/ec_metrics_test.cc +++ b/webrtc/voice_engine/test/auto_test/extended/ec_metrics_test.cc @@ -13,19 +13,13 @@ class EcMetricsTest : public AfterStreamingFixture { }; -TEST_F(EcMetricsTest, EcMetricsAreOnByDefault) { - // AEC must be enabled fist. - EXPECT_EQ(0, voe_apm_->SetEcStatus(true, webrtc::kEcAec)); - - bool enabled = false; +TEST_F(EcMetricsTest, EcMetricsAreOffByDefault) { + bool enabled = true; EXPECT_EQ(0, voe_apm_->GetEcMetricsStatus(enabled)); - EXPECT_TRUE(enabled); + EXPECT_FALSE(enabled); } TEST_F(EcMetricsTest, CanEnableAndDisableEcMetrics) { - // AEC must be enabled fist. - EXPECT_EQ(0, voe_apm_->SetEcStatus(true, webrtc::kEcAec)); - EXPECT_EQ(0, voe_apm_->SetEcMetricsStatus(true)); bool ec_on = false; EXPECT_EQ(0, voe_apm_->GetEcMetricsStatus(ec_on));