Skip to content

Commit

Permalink
Revert change that causes boot failure in chromeos
Browse files Browse the repository at this point in the history 
Revert "Cache constant values from CRAS"
This reverts commit 59c38b0.

Revert "Query AGC and NS support from CRAS"
This reverts commit 434fa55.

Bug: 1418369
Change-Id: I27538cb935c7d0837328efdac24e51103fcd8993
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4282397
Reviewed-by: Yu-Hsuan Hsu <yuhsuan@chromium.org>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Commit-Queue: Peter McNeeley <petermcneeley@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1108749}
  • Loading branch information
@petermcneeleychromium
petermcneeleychromium authored and Chromium LUCI CQ committed Feb 23, 2023
1 parent 0941ad5 commit ebb3d5f
Show file tree
Hide file tree
Showing 4 changed files with 39 additions and 178 deletions.
9 changes: 7 additions & 2 deletions media/audio/cras/audio_manager_cras.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,7 @@ AudioParameters AudioManagerCras::GetStreamParametersForSystem(
// Rephrase the field aec_supported to properly reflect its meaning in this
// context (since it currently signals whether an CrAS APM with tuned settings
// is available).
// TODO(crbug.com/1307680): add unit tests and caching cras_util_ results.
const bool tuned_system_apm_available = cras_util_->CrasGetAecSupported();

// Don't use the system AEC if it is deactivated for this group ID. Also never
Expand All @@ -217,8 +218,12 @@ AudioParameters AudioManagerCras::GetStreamParametersForSystem(
(tuned_system_apm_available && tuned_system_aec_allowed) ||
enforce_system_aec;

bool system_ns_supported = cras_util_->CrasGetNsSupported();
bool system_agc_supported = cras_util_->CrasGetAgcSupported();
// TODO(b/266242770): Reintroduce the scheme for setting this to follow what
// was previously done in (the now removed)
// media/audio/cras/audio_manager_chromeos.cc. Until then, the NS and AGC
// effects are hardcoded to never run in CRAS.
bool system_ns_supported = false;
bool system_agc_supported = false;

int aec_group_id = cras_util_->CrasGetAecGroupId();
if (!use_system_aec || IsSystemAecDeactivated(aec_group_id)) {
Expand Down
104 changes: 6 additions & 98 deletions media/audio/cras/audio_manager_cras_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@
#include "media/audio/cras/cras_util.h"
#include "media/audio/fake_audio_log_factory.h"
#include "media/audio/test_audio_thread.h"
#include "media/base/limits.h"
#include "media/base/media_switches.h"
#include "media/base/limits.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"

Expand All @@ -33,8 +33,6 @@ class MockCrasUtil : public CrasUtil {
MOCK_METHOD(int, CrasGetAecSupported, (), (override));
MOCK_METHOD(int, CrasGetAecGroupId, (), (override));
MOCK_METHOD(int, CrasGetDefaultOutputBufferSize, (), (override));
MOCK_METHOD(int, CrasGetNsSupported, (), (override));
MOCK_METHOD(int, CrasGetAgcSupported, (), (override));
};

class AudioManagerCrasUnderTest : public AudioManagerCras {
Expand Down Expand Up @@ -399,35 +397,19 @@ bool DspAecAllowed(const AudioParameters& params) {
params.effects() & AudioParameters::ECHO_CANCELLER;
}

bool DspNsAllowed(const AudioParameters& params) {
return params.effects() & AudioParameters::ALLOW_DSP_NOISE_SUPPRESSION &&
params.effects() & AudioParameters::NOISE_SUPPRESSION;
}

bool DspAgcAllowed(const AudioParameters& params) {
return params.effects() & AudioParameters::ALLOW_DSP_AUTOMATIC_GAIN_CONTROL &&
params.effects() & AudioParameters::AUTOMATIC_GAIN_CONTROL;
}

class AudioManagerCrasTestAEC
: public AudioManagerCrasTest,
public ::testing::WithParamInterface<std::tuple<bool, bool, bool, bool>> {
public ::testing::WithParamInterface<std::tuple<bool, bool>> {
protected:
void SetUp() override {
std::unique_ptr<MockCrasUtil> util = std::make_unique<MockCrasUtil>();
auto aec_supported = std::get<0>(GetParam());
auto aec_group = std::get<1>(GetParam());
auto ns_supported = std::get<2>(GetParam());
auto agc_supported = std::get<3>(GetParam());

EXPECT_CALL(*util, CrasGetAecSupported())
.WillOnce(testing::Return(aec_supported));
EXPECT_CALL(*util, CrasGetAecGroupId())
.WillOnce(testing::Return(aec_group));
EXPECT_CALL(*util, CrasGetNsSupported())
.WillOnce(testing::Return(ns_supported));
EXPECT_CALL(*util, CrasGetAgcSupported())
.WillOnce(testing::Return(agc_supported));

audio_manager_->SetCrasUtil(std::move(util));
}
Expand All @@ -437,9 +419,8 @@ INSTANTIATE_TEST_SUITE_P(
AllInputParameters,
AudioManagerCrasTestAEC,
::testing::Combine(::testing::Values(false, true),
::testing::Values(kNoAecFlaggedGroupId, kAecTestGroupId),
::testing::Values(false, true),
::testing::Values(false, true)));
::testing::Values(kNoAecFlaggedGroupId,
kAecTestGroupId)));

TEST_P(AudioManagerCrasTestAEC, DefaultBehavior) {
AudioParameters params = audio_manager_->GetInputStreamParameters("");
Expand Down Expand Up @@ -532,7 +513,8 @@ TEST_P(AudioManagerCrasTestAEC,
BehaviorWithCrOSEnforceSystemAecNsAgcAndDisallowedSystemAec) {
base::test::ScopedFeatureList feature_list;
feature_list.InitWithFeaturesAndParameters(
{{media::kCrOSEnforceSystemAecNsAgc, {}}}, {{media::kCrOSSystemAEC}});
{{media::kCrOSEnforceSystemAecNsAgc, {}}},
{{media::kCrOSSystemAEC}});
AudioParameters params = audio_manager_->GetInputStreamParameters("");

auto aec_supported = std::get<0>(GetParam());
Expand All @@ -548,64 +530,6 @@ TEST_P(AudioManagerCrasTestAEC,
}
}

TEST_P(AudioManagerCrasTestAEC, BehaviorWithCrOSEnforceSystemAecNs) {
base::test::ScopedFeatureList feature_list;
feature_list.InitAndEnableFeature(media::kCrOSEnforceSystemAecNs);
AudioParameters params = audio_manager_->GetInputStreamParameters("");

auto aec_supported = std::get<0>(GetParam());
auto agc_supported = std::get<3>(GetParam());

EXPECT_TRUE(ExperimentalAecActive(params));
EXPECT_TRUE(AecActive(params));
if (aec_supported) {
EXPECT_FALSE(NsActive(params));
EXPECT_FALSE(AgcActive(params));
} else {
EXPECT_TRUE(NsActive(params));
EXPECT_EQ(AgcActive(params), agc_supported);
}
}

TEST_P(AudioManagerCrasTestAEC, BehaviorWithCrOSEnforceSystemAecAgc) {
base::test::ScopedFeatureList feature_list;
feature_list.InitAndEnableFeature(media::kCrOSEnforceSystemAecAgc);
AudioParameters params = audio_manager_->GetInputStreamParameters("");

auto aec_supported = std::get<0>(GetParam());
auto ns_supported = std::get<2>(GetParam());

EXPECT_TRUE(ExperimentalAecActive(params));
EXPECT_TRUE(AecActive(params));
if (aec_supported) {
EXPECT_FALSE(NsActive(params));
EXPECT_FALSE(AgcActive(params));
} else {
EXPECT_EQ(NsActive(params), ns_supported);
EXPECT_TRUE(AgcActive(params));
}
}

TEST_P(AudioManagerCrasTestAEC, BehaviorWithCrOSEnforceSystemAec) {
base::test::ScopedFeatureList feature_list;
feature_list.InitAndEnableFeature(media::kCrOSEnforceSystemAec);
AudioParameters params = audio_manager_->GetInputStreamParameters("");

auto aec_supported = std::get<0>(GetParam());
auto ns_supported = std::get<2>(GetParam());
auto agc_supported = std::get<3>(GetParam());

EXPECT_TRUE(ExperimentalAecActive(params));
EXPECT_TRUE(AecActive(params));
if (aec_supported) {
EXPECT_FALSE(NsActive(params));
EXPECT_FALSE(AgcActive(params));
} else {
EXPECT_EQ(NsActive(params), ns_supported);
EXPECT_EQ(AgcActive(params), agc_supported);
}
}

class AudioManagerCrasTestDSP
: public AudioManagerCrasTest,
public ::testing::WithParamInterface<std::tuple<bool, bool, bool>> {
Expand Down Expand Up @@ -636,8 +560,6 @@ class AudioManagerCrasTestDSP

EXPECT_CALL(*util, CrasGetAecSupported()).WillOnce(testing::Return(false));
EXPECT_CALL(*util, CrasGetAecGroupId()).WillOnce(testing::Return(0));
EXPECT_CALL(*util, CrasGetNsSupported()).WillOnce(testing::Return(false));
EXPECT_CALL(*util, CrasGetAgcSupported()).WillOnce(testing::Return(false));

audio_manager_->SetCrasUtil(std::move(util));
}
Expand All @@ -660,8 +582,6 @@ TEST_P(AudioManagerCrasTestDSP, BehaviorWithoutAnyEnforcedEffects) {
AudioParameters params = audio_manager_->GetInputStreamParameters("");

EXPECT_FALSE(DspAecAllowed(params));
EXPECT_FALSE(DspNsAllowed(params));
EXPECT_FALSE(DspAgcAllowed(params));
}

TEST_P(AudioManagerCrasTestDSP, BehaviorWithCrOSEnforceSystemAec) {
Expand All @@ -672,8 +592,6 @@ TEST_P(AudioManagerCrasTestDSP, BehaviorWithCrOSEnforceSystemAec) {

EXPECT_TRUE(DspAecAllowed(params) && aec_on_dsp_allowed_ ||
!DspAecAllowed(params) && !aec_on_dsp_allowed_);
EXPECT_FALSE(DspNsAllowed(params));
EXPECT_FALSE(DspAgcAllowed(params));
}

TEST_P(AudioManagerCrasTestDSP, BehaviorWithCrOSEnforceSystemAecNs) {
Expand All @@ -684,9 +602,6 @@ TEST_P(AudioManagerCrasTestDSP, BehaviorWithCrOSEnforceSystemAecNs) {

EXPECT_TRUE(DspAecAllowed(params) && aec_on_dsp_allowed_ ||
!DspAecAllowed(params) && !aec_on_dsp_allowed_);
EXPECT_TRUE(DspNsAllowed(params) && ns_on_dsp_allowed_ ||
!DspNsAllowed(params) && !ns_on_dsp_allowed_);
EXPECT_FALSE(DspAgcAllowed(params));
}

TEST_P(AudioManagerCrasTestDSP, BehaviorWithCrOSEnforceSystemAecAgc) {
Expand All @@ -697,9 +612,6 @@ TEST_P(AudioManagerCrasTestDSP, BehaviorWithCrOSEnforceSystemAecAgc) {

EXPECT_TRUE(DspAecAllowed(params) && aec_on_dsp_allowed_ ||
!DspAecAllowed(params) && !aec_on_dsp_allowed_);
EXPECT_FALSE(DspNsAllowed(params));
EXPECT_TRUE(DspAgcAllowed(params) && agc_on_dsp_allowed_ ||
!DspAgcAllowed(params) && !agc_on_dsp_allowed_);
}

TEST_P(AudioManagerCrasTestDSP, BehaviorWithCrOSEnforceSystemAecNsAgc) {
Expand All @@ -710,10 +622,6 @@ TEST_P(AudioManagerCrasTestDSP, BehaviorWithCrOSEnforceSystemAecNsAgc) {

EXPECT_TRUE(DspAecAllowed(params) && aec_on_dsp_allowed_ ||
!DspAecAllowed(params) && !aec_on_dsp_allowed_);
EXPECT_TRUE(DspNsAllowed(params) && ns_on_dsp_allowed_ ||
!DspNsAllowed(params) && !ns_on_dsp_allowed_);
EXPECT_TRUE(DspAgcAllowed(params) && agc_on_dsp_allowed_ ||
!DspAgcAllowed(params) && !agc_on_dsp_allowed_);
}

} // namespace
Expand Down
81 changes: 26 additions & 55 deletions media/audio/cras/cras_util.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -180,43 +180,10 @@ void mergeDevices(CrasDevice& old_dev, CrasDevice& new_dev) {
old_dev.active |= new_dev.active;
}

CrasUtil::CrasUtil() {
cras_effects_cached_ = CacheEffects();
}
CrasUtil::CrasUtil() = default;

CrasUtil::~CrasUtil() = default;

bool CrasUtil::CacheEffects() {
libcras_client* client = CrasConnect();
if (!client) {
LOG(ERROR) << "Failed to cache effects";
return false;
}
if (libcras_client_get_aec_supported(client, &aec_supported_) < 0) {
LOG(ERROR) << "Fail to query AEC supported";
aec_supported_ = false;
}
if (libcras_client_get_agc_supported(client, &agc_supported_) < 0) {
LOG(ERROR) << "Fail to query AGC supported";
agc_supported_ = false;
}
if (libcras_client_get_ns_supported(client, &ns_supported_) < 0) {
LOG(ERROR) << "Fail to query NS supported";
ns_supported_ = false;
}
if (libcras_client_get_aec_group_id(client, &aec_group_id_) < 0) {
LOG(ERROR) << "Fail to query AEC group ID";
aec_group_id_ = -1; // The default group ID is -1
}
if (libcras_client_get_default_output_buffer_size(
client, &default_output_buffer_size_) < 0) {
LOG(ERROR) << "Fail to query default output buffer size";
default_output_buffer_size_ = 0;
}
CrasDisconnect(&client);
return true;
}

std::vector<CrasDevice> CrasUtil::CrasGetAudioDevices(DeviceType type) {
std::vector<CrasDevice> devices;

Expand Down Expand Up @@ -269,38 +236,42 @@ std::vector<CrasDevice> CrasUtil::CrasGetAudioDevices(DeviceType type) {
}

int CrasUtil::CrasGetAecSupported() {
if (!cras_effects_cached_) {
cras_effects_cached_ = CacheEffects();
libcras_client* client = CrasConnect();
if (!client) {
return 0;
}
return aec_supported_;
}

int CrasUtil::CrasGetAgcSupported() {
if (!cras_effects_cached_) {
cras_effects_cached_ = CacheEffects();
}
return agc_supported_;
}
int supported;
libcras_client_get_aec_supported(client, &supported);
CrasDisconnect(&client);

int CrasUtil::CrasGetNsSupported() {
if (!cras_effects_cached_) {
cras_effects_cached_ = CacheEffects();
}
return ns_supported_;
return supported;
}

int CrasUtil::CrasGetAecGroupId() {
if (!cras_effects_cached_) {
cras_effects_cached_ = CacheEffects();
libcras_client* client = CrasConnect();
if (!client) {
return -1;
}
return aec_group_id_;

int id;
int rc = libcras_client_get_aec_group_id(client, &id);
CrasDisconnect(&client);

return rc < 0 ? rc : id;
}

int CrasUtil::CrasGetDefaultOutputBufferSize() {
if (!cras_effects_cached_) {
cras_effects_cached_ = CacheEffects();
libcras_client* client = CrasConnect();
if (!client) {
return -1;
}
return default_output_buffer_size_;

int size;
int rc = libcras_client_get_default_output_buffer_size(client, &size);
CrasDisconnect(&client);

return rc < 0 ? rc : size;
}

} // namespace media
23 changes: 0 additions & 23 deletions media/audio/cras/cras_util.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,40 +48,17 @@ class MEDIA_EXPORT CrasUtil {
virtual ~CrasUtil();

// Enumerates all devices of |type|.
// Virtual for testing.
virtual std::vector<CrasDevice> CrasGetAudioDevices(DeviceType type);

// Returns if system AEC is supported in CRAS.
// Virtual for testing.
virtual int CrasGetAecSupported();

// Returns if system AGC is supported in CRAS.
// Virtual for testing.
virtual int CrasGetAgcSupported();

// Returns if system NS is supported in CRAS.
// Virtual for testing.
virtual int CrasGetNsSupported();

// Returns the system AEC group ID. If no group ID is specified, -1 is
// returned.
// Virtual for testing.
virtual int CrasGetAecGroupId();

// Returns the default output buffer size.
// Virtual for testing.
virtual int CrasGetDefaultOutputBufferSize();

private:
int aec_supported_ = false;
int agc_supported_ = false;
int ns_supported_ = false;
int aec_group_id_ = -1;
int default_output_buffer_size_ = 0;
bool cras_effects_cached_ = false;

// Caches constant effect config from CRAS.
bool CacheEffects();
};

} // namespace media
Expand Down

0 comments on commit ebb3d5f

Please sign in to comment.