From e2924d555d85fe3656608ea36955ce02f9b712b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patrik=20H=C3=B6glund?= Date: Wed, 5 Sep 2018 08:52:40 +0000 Subject: [PATCH] Revert "Adds WebRTC.Audio.Record/PlayoutSampleRateOffsetInPercent UMA stats to native WebRTC." MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit f217903a67995496a1d67674d77d5f237772b01b. Reason for revert: Breaks downstream tests Original change's description: > Adds WebRTC.Audio.Record/PlayoutSampleRateOffsetInPercent UMA stats to native WebRTC. > > Also ensures that audio parameters are accessed atomically. > > Bug: b/113648245 > Change-Id: Ic812bfe2b2c4cfb3b00d9d411bb4986dfeda1028 > Reviewed-on: https://webrtc-review.googlesource.com/97331 > Reviewed-by: Minyue Li > Reviewed-by: Ivo Creusen > Commit-Queue: Henrik Andreassson > Cr-Commit-Position: refs/heads/master@{#24550} TBR=henrika@webrtc.org,ivoc@webrtc.org,minyue@webrtc.org Change-Id: I620406f25762cf76db0470b3b29b50bc146935c7 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: b/113648245 Reviewed-on: https://webrtc-review.googlesource.com/97941 Reviewed-by: Patrik Höglund Commit-Queue: Patrik Höglund Cr-Commit-Position: refs/heads/master@{#24569} --- .../android/audio_device_unittest.cc | 4 +- modules/audio_device/audio_device_buffer.cc | 49 ++++++------------- modules/audio_device/audio_device_buffer.h | 28 ++++++----- 3 files changed, 33 insertions(+), 48 deletions(-) diff --git a/modules/audio_device/android/audio_device_unittest.cc b/modules/audio_device/android/audio_device_unittest.cc index b0cf42edec..06ae706632 100644 --- a/modules/audio_device/android/audio_device_unittest.cc +++ b/modules/audio_device/android/audio_device_unittest.cc @@ -755,9 +755,9 @@ TEST_F(AudioDeviceTest, UsesCorrectDelayEstimateForLowLatencyOutputPath) { // correct set of parameters. TEST_F(AudioDeviceTest, VerifyAudioDeviceBufferParameters) { EXPECT_EQ(playout_parameters_.sample_rate(), - static_cast(audio_device_buffer()->PlayoutSampleRate())); + audio_device_buffer()->PlayoutSampleRate()); EXPECT_EQ(record_parameters_.sample_rate(), - static_cast(audio_device_buffer()->RecordingSampleRate())); + audio_device_buffer()->RecordingSampleRate()); EXPECT_EQ(playout_parameters_.channels(), audio_device_buffer()->PlayoutChannels()); EXPECT_EQ(record_parameters_.channels(), diff --git a/modules/audio_device/audio_device_buffer.cc b/modules/audio_device/audio_device_buffer.cc index f96a572f7b..d872da5fc9 100644 --- a/modules/audio_device/audio_device_buffer.cc +++ b/modules/audio_device/audio_device_buffer.cc @@ -188,36 +188,43 @@ int32_t AudioDeviceBuffer::SetRecordingSampleRate(uint32_t fsHz) { } int32_t AudioDeviceBuffer::SetPlayoutSampleRate(uint32_t fsHz) { + RTC_DCHECK(main_thread_checker_.CalledOnValidThread()); RTC_LOG(INFO) << "SetPlayoutSampleRate(" << fsHz << ")"; play_sample_rate_ = fsHz; return 0; } -uint32_t AudioDeviceBuffer::RecordingSampleRate() const { +int32_t AudioDeviceBuffer::RecordingSampleRate() const { + RTC_DCHECK(main_thread_checker_.CalledOnValidThread()); return rec_sample_rate_; } -uint32_t AudioDeviceBuffer::PlayoutSampleRate() const { +int32_t AudioDeviceBuffer::PlayoutSampleRate() const { + RTC_DCHECK(main_thread_checker_.CalledOnValidThread()); return play_sample_rate_; } int32_t AudioDeviceBuffer::SetRecordingChannels(size_t channels) { + RTC_DCHECK(main_thread_checker_.CalledOnValidThread()); RTC_LOG(INFO) << "SetRecordingChannels(" << channels << ")"; rec_channels_ = channels; return 0; } int32_t AudioDeviceBuffer::SetPlayoutChannels(size_t channels) { + RTC_DCHECK(main_thread_checker_.CalledOnValidThread()); RTC_LOG(INFO) << "SetPlayoutChannels(" << channels << ")"; play_channels_ = channels; return 0; } size_t AudioDeviceBuffer::RecordingChannels() const { + RTC_DCHECK(main_thread_checker_.CalledOnValidThread()); return rec_channels_; } size_t AudioDeviceBuffer::PlayoutChannels() const { + RTC_DCHECK(main_thread_checker_.CalledOnValidThread()); return play_channels_; } @@ -412,54 +419,28 @@ void AudioDeviceBuffer::LogStats(LogState state) { stats_.max_play_level = 0; } - // Cache current sample rate from atomic members. - const uint32_t rec_sample_rate = rec_sample_rate_; - const uint32_t play_sample_rate = play_sample_rate_; - RTC_DCHECK_GT(rec_sample_rate, 0u); - RTC_DCHECK_GT(play_sample_rate, 0u); - - // Log the latest statistics but skip the first two rounds just after state - // was set to LOG_START to ensure that we have at least one full stable - // 10-second interval for sample-rate estimation. Hence, first printed log - // will be after ~20 seconds. - if (++num_stat_reports_ > 2 && time_since_last > 0) { + // Log the latest statistics but skip the first round just after state was + // set to LOG_START. Hence, first printed log will be after ~10 seconds. + if (++num_stat_reports_ > 1 && time_since_last > 0) { uint32_t diff_samples = stats.rec_samples - last_stats_.rec_samples; float rate = diff_samples / (static_cast(time_since_last) / 1000.0); - uint32_t abs_diff_rate_in_percent = 0; - if (rec_sample_rate > 0) { - abs_diff_rate_in_percent = static_cast( - 0.5f + - ((100.0f * std::abs(rate - rec_sample_rate)) / rec_sample_rate)); - RTC_HISTOGRAM_PERCENTAGE("WebRTC.Audio.RecordSampleRateOffsetInPercent", - abs_diff_rate_in_percent); - } RTC_LOG(INFO) << "[REC : " << time_since_last << "msec, " - << rec_sample_rate / 1000 << "kHz] callbacks: " + << rec_sample_rate_ / 1000 << "kHz] callbacks: " << stats.rec_callbacks - last_stats_.rec_callbacks << ", " << "samples: " << diff_samples << ", " << "rate: " << static_cast(rate + 0.5) << ", " - << "rate diff: " << abs_diff_rate_in_percent << "%, " << "level: " << stats.max_rec_level; diff_samples = stats.play_samples - last_stats_.play_samples; rate = diff_samples / (static_cast(time_since_last) / 1000.0); - abs_diff_rate_in_percent = 0; - if (play_sample_rate > 0) { - abs_diff_rate_in_percent = static_cast( - 0.5f + - ((100.0f * std::abs(rate - play_sample_rate)) / play_sample_rate)); - RTC_HISTOGRAM_PERCENTAGE("WebRTC.Audio.PlayoutSampleRateOffsetInPercent", - abs_diff_rate_in_percent); - } RTC_LOG(INFO) << "[PLAY: " << time_since_last << "msec, " - << play_sample_rate / 1000 << "kHz] callbacks: " + << play_sample_rate_ / 1000 << "kHz] callbacks: " << stats.play_callbacks - last_stats_.play_callbacks << ", " << "samples: " << diff_samples << ", " << "rate: " << static_cast(rate + 0.5) << ", " - << "rate diff: " << abs_diff_rate_in_percent << "%, " << "level: " << stats.max_play_level; + last_stats_ = stats; } - last_stats_ = stats; int64_t time_to_wait_ms = next_callback_time - rtc::TimeMillis(); RTC_DCHECK_GT(time_to_wait_ms, 0) << "Invalid timer interval"; diff --git a/modules/audio_device/audio_device_buffer.h b/modules/audio_device/audio_device_buffer.h index 3658be04e4..8dd9550194 100644 --- a/modules/audio_device/audio_device_buffer.h +++ b/modules/audio_device/audio_device_buffer.h @@ -11,8 +11,6 @@ #ifndef MODULES_AUDIO_DEVICE_AUDIO_DEVICE_BUFFER_H_ #define MODULES_AUDIO_DEVICE_AUDIO_DEVICE_BUFFER_H_ -#include - #include "modules/audio_device/include/audio_device.h" #include "rtc_base/buffer.h" #include "rtc_base/criticalsection.h" @@ -85,8 +83,8 @@ class AudioDeviceBuffer { int32_t SetRecordingSampleRate(uint32_t fsHz); int32_t SetPlayoutSampleRate(uint32_t fsHz); - uint32_t RecordingSampleRate() const; - uint32_t PlayoutSampleRate() const; + int32_t RecordingSampleRate() const; + int32_t PlayoutSampleRate() const; int32_t SetRecordingChannels(size_t channels); int32_t SetPlayoutChannels(size_t channels); @@ -138,7 +136,7 @@ class AudioDeviceBuffer { // called on that same thread. When audio has started some methods will be // called on either a native audio thread for playout or a native thread for // recording. Some members are not annotated since they are "protected by - // design" and adding e.g. a race checker can cause failures for very few + // design" and adding e.g. a race checker can cause failuries for very few // edge cases and it is IMHO not worth the risk to use them in this class. // TODO(henrika): see if it is possible to refactor and annotate all members. @@ -162,17 +160,23 @@ class AudioDeviceBuffer { // and it must outlive this object. It is not possible to change this member // while any media is active. It is possible to start media without calling // RegisterAudioCallback() but that will lead to ignored audio callbacks in - // both directions where native audio will be active but no audio samples will + // both directions where native audio will be acive but no audio samples will // be transported. AudioTransport* audio_transport_cb_; - // Sample rate in Hertz. Accessed atomically. - std::atomic rec_sample_rate_; - std::atomic play_sample_rate_; + // The members below that are not annotated are protected by design. They are + // all set on the main thread (verified by |main_thread_checker_|) and then + // read on either the playout or recording audio thread. But, media will never + // be active when the member is set; hence no conflict exists. It is too + // complex to ensure and verify that this is actually the case. - // Number of audio channels. Accessed atomically. - std::atomic rec_channels_; - std::atomic play_channels_; + // Sample rate in Hertz. + uint32_t rec_sample_rate_; + uint32_t play_sample_rate_; + + // Number of audio channels. + size_t rec_channels_; + size_t play_channels_; // Keeps track of if playout/recording are active or not. A combination // of these states are used to determine when to start and stop the timer.