AIDL effect: Refine some implementation and test logic.

Bug: 238913361
Test: atest VtsHalAudioEffectTargetTest
Change-Id: I5a9bb542872de6c5700fa6b14e124e9b9e206da6

audio/aidl/android/hardware/audio/effect/IEffect.aidl

@@ -31,24 +31,26 @@ interface IEffect {
      *
      * @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
+     * Called by the client to close the effect instance, processing thread should be destroyed and
-     * close, but processing thread should be destroyed and consume no CPU. It is recommended to
+     * consume no CPU after close.
-     * 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 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();
 

audio/aidl/default/EffectFactory.cpp

@@ -29,23 +29,9 @@ Factory::Factory() {
     // 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),
+    id.type = EqualizerTypeUUID;
-               0xddd6,
+    id.uuid = EqualizerSwImplUUID;
-               0x11db,
-               0x8f34,
-               {0x00, 0x02, 0xa5, 0xd5, 0xc5, 0x1b}};
-    id.uuid = EqualizerUUID;
     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 @@ ndk::ScopedAStatus Factory::createEffect(
         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 "

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>();

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();

audio/aidl/default/include/equalizer-impl/Equalizer.h

@@ -21,23 +21,31 @@
 
 namespace aidl::android::hardware::audio::effect {
 
-// Equalizer implementation UUID.
+// Equalizer type UUID.
-static const ::aidl::android::media::audio::common::AudioUuid EqualizerUUID = {
+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

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 @@ using aidl::android::media::audio::common::AudioUuid;
 
 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 @@ class EffectFactoryHelper {
 
     void QueryAllEffects() {
         EXPECT_NE(mEffectFactory, nullptr);
-        ScopedAStatus status =
+        EXPECT_IS_OK(mEffectFactory->queryEffects(std::nullopt, std::nullopt, &mCompleteIds));
-                mEffectFactory->queryEffects(std::nullopt, std::nullopt, &mCompleteIds);
-        EXPECT_EQ(status.getExceptionCode(), EX_NONE);
     }
 
     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_IS_OK(mEffectFactory->queryEffects(in_type, in_instance, _aidl_return));
-        EXPECT_EQ(status.getExceptionCode(), EX_NONE);
         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_IS_OK(mEffectFactory->createEffect(id.uuid, &effect));
-            EXPECT_EQ(status.getExceptionCode(), EX_NONE) << id.toString();
             EXPECT_NE(effect, nullptr) << id.toString();
             mEffectIdMap[effect] = id;
         }
@@ -88,10 +84,8 @@ class EffectFactoryHelper {
 
     void DestroyEffects() {
         EXPECT_NE(mEffectFactory, nullptr);
-        ScopedAStatus status;
         for (const auto& it : mEffectIdMap) {
-            status = mEffectFactory->destroyEffect(it.first);
+            EXPECT_IS_OK(mEffectFactory->destroyEffect(it.first));
-            EXPECT_EQ(status.getExceptionCode(), EX_NONE) << it.second.toString();
         }
         mEffectIdMap.clear();
     }
@@ -143,7 +137,7 @@ TEST_P(EffectFactoryTest, CanBeRestarted) {
 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, DescriptorUUIDNotNull) {
 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());
+    auto numIds = mFactory.GetEffectIds().size();
-    EXPECT_NE(numIds, 0);
+    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());
+    auto numIds = mFactory.GetEffectIds().size();
-    EXPECT_NE(numIds, 0);
+    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 @@ class AudioEffect : public testing::TestWithParam<std::string> {
     }
 
     void OpenEffects() {
-        auto open = [](const std::shared_ptr<IEffect>& effect) {
+        auto open = [](const std::shared_ptr<IEffect>& effect) { EXPECT_IS_OK(effect->open()); };
-            ScopedAStatus status = effect->open();
-            EXPECT_EQ(status.getExceptionCode(), EX_NONE);
-        };
         EXPECT_NO_FATAL_FAILURE(ForEachEffect(open));
     }
 
     void CloseEffects() {
-        auto close = [](const std::shared_ptr<IEffect>& effect) {
+        auto close = [](const std::shared_ptr<IEffect>& effect) { EXPECT_IS_OK(effect->close()); };
-            ScopedAStatus status = effect->close();
-            EXPECT_EQ(status.getExceptionCode(), EX_NONE);
-        };
         EXPECT_NO_FATAL_FAILURE(ForEachEffect(close));
     }
 
     void GetEffectDescriptors() {
         auto get = [](const std::shared_ptr<IEffect>& effect) {
             Descriptor desc;
-            ScopedAStatus status = effect->getDescriptor(&desc);
+            EXPECT_IS_OK(effect->getDescriptor(&desc));
-            EXPECT_EQ(status.getExceptionCode(), EX_NONE);
         };
         EXPECT_NO_FATAL_FAILURE(ForEachEffect(get));
     }
@@ -253,7 +240,6 @@ class AudioEffect : public testing::TestWithParam<std::string> {
     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 @@ TEST_P(AudioEffect, DescriptorIdExistAndUnique) {
     auto checker = [&](const std::shared_ptr<IEffect>& effect) {
         Descriptor desc;
         std::vector<Descriptor::Identity> idList;
-        ScopedAStatus status = effect->getDescriptor(&desc);
+        EXPECT_IS_OK(effect->getDescriptor(&desc));
-        EXPECT_EQ(status.getExceptionCode(), EX_NONE);
         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 @@ TEST_P(AudioEffect, DescriptorIdExistAndUnique) {
     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);
     }
 }