AIDL effect: Refine some implementation and test logic.
Bug: 238913361
Test: atest VtsHalAudioEffectTargetTest
Change-Id: I5a9bb542872de6c5700fa6b14e124e9b9e206da6
diff --git a/audio/aidl/android/hardware/audio/effect/IEffect.aidl b/audio/aidl/android/hardware/audio/effect/IEffect.aidl
index 44e916b..d7a9501 100644
--- a/audio/aidl/android/hardware/audio/effect/IEffect.aidl
+++ b/audio/aidl/android/hardware/audio/effect/IEffect.aidl
@@ -31,24 +31,26 @@
*
* @throws a EX_UNSUPPORTED_OPERATION if device capability/resource is not enough or system
* failure happens.
- * @note Open an already-opened effect instance should do nothing and not result in throw error.
+ * @note Open an already-opened effect instance should do nothing and should not throw an error.
*/
void open();
/**
- * Called by the client to close the effect instance, instance context will be kept after
- * close, but processing thread should be destroyed and consume no CPU. It is recommended to
- * close the effect on the client side as soon as it becomes unused, it's client responsibility
- * to make sure all parameter/buffer is correct if client wants to reopen a closed instance.
+ * Called by the client to close the effect instance, processing thread should be destroyed and
+ * consume no CPU after close.
*
- * Effect instance close interface should always success unless:
+ * It is recommended to close the effect on the client side as soon as it becomes unused, it's
+ * client responsibility to make sure all parameter/buffer is correct if client wants to reopen
+ * a closed instance.
+ *
+ * Effect instance close interface should always succeed unless:
* 1. The effect instance is not in a proper state to be closed, for example it's still in
* processing state.
* 2. There is system/hardware related failure when close.
*
* @throws EX_ILLEGAL_STATE if the effect instance is not in a proper state to be closed.
* @throws EX_UNSUPPORTED_OPERATION if the effect instance failed to close for any other reason.
- * @note Close an already-closed effect should do nothing and not result in throw error.
+ * @note Close an already-closed effect should do nothing and should not throw an error.
*/
void close();
diff --git a/audio/aidl/default/EffectFactory.cpp b/audio/aidl/default/EffectFactory.cpp
index ea9d470..a9848fd 100644
--- a/audio/aidl/default/EffectFactory.cpp
+++ b/audio/aidl/default/EffectFactory.cpp
@@ -29,23 +29,9 @@
// TODO: implement this with xml parser on audio_effect.xml, and filter with optional
// parameters.
Descriptor::Identity id;
- id.type = {static_cast<int32_t>(0x0bed4300),
- 0xddd6,
- 0x11db,
- 0x8f34,
- {0x00, 0x02, 0xa5, 0xd5, 0xc5, 0x1b}};
- id.uuid = EqualizerUUID;
+ id.type = EqualizerTypeUUID;
+ id.uuid = EqualizerSwImplUUID;
mIdentityList.push_back(id);
- // TODO: Add visualizer with default implementation later
-#if 0
- id.type = {static_cast<int32_t>(0xd3467faa),
- 0xacc7,
- 0x4d34,
- 0xacaf,
- {0x00, 0x02, 0xa5, 0xd5, 0xc5, 0x1b}};
- id.uuid = VisualizerUUID;
- mIdentityList.push_back(id);
-#endif
}
ndk::ScopedAStatus Factory::queryEffects(const std::optional<AudioUuid>& in_type,
@@ -63,7 +49,7 @@
const AudioUuid& in_impl_uuid,
std::shared_ptr<::aidl::android::hardware::audio::effect::IEffect>* _aidl_return) {
LOG(DEBUG) << __func__ << ": UUID " << in_impl_uuid.toString();
- if (in_impl_uuid == EqualizerUUID) {
+ if (in_impl_uuid == EqualizerSwImplUUID) {
*_aidl_return = ndk::SharedRefBase::make<Equalizer>();
} else {
LOG(ERROR) << __func__ << ": UUID "
diff --git a/audio/aidl/default/EffectMain.cpp b/audio/aidl/default/EffectMain.cpp
index b30f2e7..3219dd6 100644
--- a/audio/aidl/default/EffectMain.cpp
+++ b/audio/aidl/default/EffectMain.cpp
@@ -23,7 +23,7 @@
int main() {
// This is a debug implementation, always enable debug logging.
android::base::SetMinimumLogSeverity(::android::base::DEBUG);
- ABinderProcess_setThreadPoolMaxThreadCount(1);
+ ABinderProcess_setThreadPoolMaxThreadCount(0);
auto effectFactory =
ndk::SharedRefBase::make<aidl::android::hardware::audio::effect::Factory>();
diff --git a/audio/aidl/default/equalizer/Equalizer.cpp b/audio/aidl/default/equalizer/Equalizer.cpp
index dae3ab7..8b157fa 100644
--- a/audio/aidl/default/equalizer/Equalizer.cpp
+++ b/audio/aidl/default/equalizer/Equalizer.cpp
@@ -21,15 +21,6 @@
namespace aidl::android::hardware::audio::effect {
-Equalizer::Equalizer() {
- // Implementation UUID
- mDesc.common.id.uuid = {static_cast<int32_t>(0xce772f20),
- 0x847d,
- 0x11df,
- 0xbb17,
- {0x00, 0x02, 0xa5, 0xd5, 0xc5, 0x1b}};
-}
-
ndk::ScopedAStatus Equalizer::open() {
LOG(DEBUG) << __func__;
return ndk::ScopedAStatus::ok();
diff --git a/audio/aidl/default/include/equalizer-impl/Equalizer.h b/audio/aidl/default/include/equalizer-impl/Equalizer.h
index 44b1d6d..ea16cb9 100644
--- a/audio/aidl/default/include/equalizer-impl/Equalizer.h
+++ b/audio/aidl/default/include/equalizer-impl/Equalizer.h
@@ -21,23 +21,31 @@
namespace aidl::android::hardware::audio::effect {
-// Equalizer implementation UUID.
-static const ::aidl::android::media::audio::common::AudioUuid EqualizerUUID = {
+// Equalizer type UUID.
+static const ::aidl::android::media::audio::common::AudioUuid EqualizerTypeUUID = {
static_cast<int32_t>(0x0bed4300),
0xddd6,
0x11db,
0x8f34,
{0x00, 0x02, 0xa5, 0xd5, 0xc5, 0x1b}};
+// Equalizer implementation UUID.
+static const ::aidl::android::media::audio::common::AudioUuid EqualizerSwImplUUID = {
+ static_cast<int32_t>(0x0bed4300),
+ 0x847d,
+ 0x11df,
+ 0xbb17,
+ {0x00, 0x02, 0xa5, 0xd5, 0xc5, 0x1b}};
+
class Equalizer : public BnEffect {
public:
- Equalizer();
+ Equalizer() = default;
ndk::ScopedAStatus open() override;
ndk::ScopedAStatus close() override;
ndk::ScopedAStatus getDescriptor(Descriptor* _aidl_return) override;
private:
// Effect descriptor.
- Descriptor mDesc = {.common.id.type = EqualizerUUID};
+ Descriptor mDesc = {.common = {.id = {.type = EqualizerTypeUUID, .uuid = EqualizerSwImplUUID}}};
};
} // namespace aidl::android::hardware::audio::effect
diff --git a/audio/aidl/vts/VtsHalAudioEffectTargetTest.cpp b/audio/aidl/vts/VtsHalAudioEffectTargetTest.cpp
index 9b100b1..8b5eb13 100644
--- a/audio/aidl/vts/VtsHalAudioEffectTargetTest.cpp
+++ b/audio/aidl/vts/VtsHalAudioEffectTargetTest.cpp
@@ -33,6 +33,7 @@
#include <aidl/android/hardware/audio/effect/IFactory.h>
#include "AudioHalBinderServiceUtil.h"
+#include "TestUtils.h"
using namespace android;
@@ -45,7 +46,7 @@
class EffectFactoryHelper {
public:
- EffectFactoryHelper(const std::string& name) : mServiceName(name) {}
+ explicit EffectFactoryHelper(const std::string& name) : mServiceName(name) {}
void ConnectToFactoryService() {
mEffectFactory = IFactory::fromBinder(binderUtil.connectToService(mServiceName));
@@ -60,27 +61,22 @@
void QueryAllEffects() {
EXPECT_NE(mEffectFactory, nullptr);
- ScopedAStatus status =
- mEffectFactory->queryEffects(std::nullopt, std::nullopt, &mCompleteIds);
- EXPECT_EQ(status.getExceptionCode(), EX_NONE);
+ EXPECT_IS_OK(mEffectFactory->queryEffects(std::nullopt, std::nullopt, &mCompleteIds));
}
void QueryEffects(const std::optional<AudioUuid>& in_type,
const std::optional<AudioUuid>& in_instance,
std::vector<Descriptor::Identity>* _aidl_return) {
EXPECT_NE(mEffectFactory, nullptr);
- ScopedAStatus status = mEffectFactory->queryEffects(in_type, in_instance, _aidl_return);
- EXPECT_EQ(status.getExceptionCode(), EX_NONE);
+ EXPECT_IS_OK(mEffectFactory->queryEffects(in_type, in_instance, _aidl_return));
mIds = *_aidl_return;
}
void CreateEffects() {
EXPECT_NE(mEffectFactory, nullptr);
- ScopedAStatus status;
for (const auto& id : mIds) {
std::shared_ptr<IEffect> effect;
- status = mEffectFactory->createEffect(id.uuid, &effect);
- EXPECT_EQ(status.getExceptionCode(), EX_NONE) << id.toString();
+ EXPECT_IS_OK(mEffectFactory->createEffect(id.uuid, &effect));
EXPECT_NE(effect, nullptr) << id.toString();
mEffectIdMap[effect] = id;
}
@@ -88,10 +84,8 @@
void DestroyEffects() {
EXPECT_NE(mEffectFactory, nullptr);
- ScopedAStatus status;
for (const auto& it : mEffectIdMap) {
- status = mEffectFactory->destroyEffect(it.first);
- EXPECT_EQ(status.getExceptionCode(), EX_NONE) << it.second.toString();
+ EXPECT_IS_OK(mEffectFactory->destroyEffect(it.first));
}
mEffectIdMap.clear();
}
@@ -143,7 +137,7 @@
TEST_P(EffectFactoryTest, QueriedDescriptorList) {
std::vector<Descriptor::Identity> descriptors;
mFactory.QueryEffects(std::nullopt, std::nullopt, &descriptors);
- EXPECT_NE(static_cast<int>(descriptors.size()), 0);
+ EXPECT_NE(descriptors.size(), 0UL);
}
TEST_P(EffectFactoryTest, DescriptorUUIDNotNull) {
@@ -159,52 +153,52 @@
TEST_P(EffectFactoryTest, QueriedDescriptorNotExistType) {
std::vector<Descriptor::Identity> descriptors;
mFactory.QueryEffects(nullUuid, std::nullopt, &descriptors);
- EXPECT_EQ(static_cast<int>(descriptors.size()), 0);
+ EXPECT_EQ(descriptors.size(), 0UL);
}
TEST_P(EffectFactoryTest, QueriedDescriptorNotExistInstance) {
std::vector<Descriptor::Identity> descriptors;
mFactory.QueryEffects(std::nullopt, nullUuid, &descriptors);
- EXPECT_EQ(static_cast<int>(descriptors.size()), 0);
+ EXPECT_EQ(descriptors.size(), 0UL);
}
TEST_P(EffectFactoryTest, CreateAndDestroyRepeat) {
std::vector<Descriptor::Identity> descriptors;
mFactory.QueryEffects(std::nullopt, std::nullopt, &descriptors);
- int numIds = static_cast<int>(mFactory.GetEffectIds().size());
- EXPECT_NE(numIds, 0);
+ auto numIds = mFactory.GetEffectIds().size();
+ EXPECT_NE(numIds, 0UL);
- EXPECT_EQ(static_cast<int>(mFactory.GetEffectMap().size()), 0);
+ EXPECT_EQ(mFactory.GetEffectMap().size(), 0UL);
mFactory.CreateEffects();
- EXPECT_EQ(static_cast<int>(mFactory.GetEffectMap().size()), numIds);
+ EXPECT_EQ(mFactory.GetEffectMap().size(), numIds);
mFactory.DestroyEffects();
- EXPECT_EQ(static_cast<int>(mFactory.GetEffectMap().size()), 0);
+ EXPECT_EQ(mFactory.GetEffectMap().size(), 0UL);
// Create and destroy again
mFactory.CreateEffects();
- EXPECT_EQ(static_cast<int>(mFactory.GetEffectMap().size()), numIds);
+ EXPECT_EQ(mFactory.GetEffectMap().size(), numIds);
mFactory.DestroyEffects();
- EXPECT_EQ(static_cast<int>(mFactory.GetEffectMap().size()), 0);
+ EXPECT_EQ(mFactory.GetEffectMap().size(), 0UL);
}
TEST_P(EffectFactoryTest, CreateMultipleInstanceOfSameEffect) {
std::vector<Descriptor::Identity> descriptors;
mFactory.QueryEffects(std::nullopt, std::nullopt, &descriptors);
- int numIds = static_cast<int>(mFactory.GetEffectIds().size());
- EXPECT_NE(numIds, 0);
+ auto numIds = mFactory.GetEffectIds().size();
+ EXPECT_NE(numIds, 0UL);
- EXPECT_EQ(static_cast<int>(mFactory.GetEffectMap().size()), 0);
+ EXPECT_EQ(mFactory.GetEffectMap().size(), 0UL);
mFactory.CreateEffects();
- EXPECT_EQ(static_cast<int>(mFactory.GetEffectMap().size()), numIds);
+ EXPECT_EQ(mFactory.GetEffectMap().size(), numIds);
// Create effect instances of same implementation
mFactory.CreateEffects();
- EXPECT_EQ(static_cast<int>(mFactory.GetEffectMap().size()), 2 * numIds);
+ EXPECT_EQ(mFactory.GetEffectMap().size(), 2 * numIds);
mFactory.CreateEffects();
- EXPECT_EQ(static_cast<int>(mFactory.GetEffectMap().size()), 3 * numIds);
+ EXPECT_EQ(mFactory.GetEffectMap().size(), 3 * numIds);
mFactory.DestroyEffects();
- EXPECT_EQ(static_cast<int>(mFactory.GetEffectMap().size()), 0);
+ EXPECT_EQ(mFactory.GetEffectMap().size(), 0UL);
}
INSTANTIATE_TEST_SUITE_P(EffectFactoryTest, EffectFactoryTest,
@@ -226,26 +220,19 @@
}
void OpenEffects() {
- auto open = [](const std::shared_ptr<IEffect>& effect) {
- ScopedAStatus status = effect->open();
- EXPECT_EQ(status.getExceptionCode(), EX_NONE);
- };
+ auto open = [](const std::shared_ptr<IEffect>& effect) { EXPECT_IS_OK(effect->open()); };
EXPECT_NO_FATAL_FAILURE(ForEachEffect(open));
}
void CloseEffects() {
- auto close = [](const std::shared_ptr<IEffect>& effect) {
- ScopedAStatus status = effect->close();
- EXPECT_EQ(status.getExceptionCode(), EX_NONE);
- };
+ auto close = [](const std::shared_ptr<IEffect>& effect) { EXPECT_IS_OK(effect->close()); };
EXPECT_NO_FATAL_FAILURE(ForEachEffect(close));
}
void GetEffectDescriptors() {
auto get = [](const std::shared_ptr<IEffect>& effect) {
Descriptor desc;
- ScopedAStatus status = effect->getDescriptor(&desc);
- EXPECT_EQ(status.getExceptionCode(), EX_NONE);
+ EXPECT_IS_OK(effect->getDescriptor(&desc));
};
EXPECT_NO_FATAL_FAILURE(ForEachEffect(get));
}
@@ -253,7 +240,6 @@
template <typename Functor>
void ForEachEffect(Functor functor) {
auto effectMap = mFactory.GetEffectMap();
- ScopedAStatus status;
for (const auto& it : effectMap) {
SCOPED_TRACE(it.second.toString());
functor(it.first);
@@ -299,10 +285,9 @@
auto checker = [&](const std::shared_ptr<IEffect>& effect) {
Descriptor desc;
std::vector<Descriptor::Identity> idList;
- ScopedAStatus status = effect->getDescriptor(&desc);
- EXPECT_EQ(status.getExceptionCode(), EX_NONE);
+ EXPECT_IS_OK(effect->getDescriptor(&desc));
mFactory.QueryEffects(desc.common.id.type, desc.common.id.uuid, &idList);
- EXPECT_EQ(static_cast<int>(idList.size()), 1);
+ EXPECT_EQ(idList.size(), 1UL);
};
EXPECT_NO_FATAL_FAILURE(ForEachEffect(checker));
@@ -313,7 +298,7 @@
auto vec = mFactory.GetCompleteEffectIdList();
std::unordered_set<Descriptor::Identity, decltype(stringHash)> idSet(0, stringHash);
for (auto it : vec) {
- EXPECT_EQ(static_cast<int>(idSet.count(it)), 0);
+ EXPECT_EQ(idSet.count(it), 0UL);
idSet.insert(it);
}
}