From 0d92059a6e300dc9f854512a913aac348450439e Mon Sep 17 00:00:00 2001 From: Mikhail Naganov Date: Fri, 25 Aug 2023 11:10:37 -0700 Subject: [PATCH] audio: Properly handle closed effects in HIDL implementation Ensure that the default HIDL implementation (legacy wrapper) does not attempt to call into the legacy implementation after it has been released. Added new option for VTS tests to verify this behavior. This does not require any new functionality from existing implementations. The test ensures that after a call to 'IEffect.close', calls of all other interface methods do not crash and return an error. This is a natural expectation, thus HIDL implementations not passing these new checks need to be fixed. Bug: 294273146 Test: atest VtsHalAudioEffectV5_0TargetTest Test: atest VtsHalAudioEffectV6_0TargetTest Test: atest VtsHalAudioEffectV7_0TargetTest Change-Id: If83e0a5f8f51f3f87c62fcfbfba469a421ad1cf8 --- audio/effect/all-versions/default/Effect.cpp | 63 ++-- audio/effect/all-versions/default/Effect.h | 6 +- .../VtsHalAudioEffectTargetTest.cpp | 268 ++++++++++++------ 3 files changed, 237 insertions(+), 100 deletions(-) diff --git a/audio/effect/all-versions/default/Effect.cpp b/audio/effect/all-versions/default/Effect.cpp index 5aecd324eb..4a9e144627 100644 --- a/audio/effect/all-versions/default/Effect.cpp +++ b/audio/effect/all-versions/default/Effect.cpp @@ -315,7 +315,7 @@ Effect::Effect(bool isInput, effect_handle_t handle) Effect::~Effect() { ATRACE_CALL(); - (void)close(); + auto [_, handle] = closeImpl(); if (mProcessThread.get()) { ATRACE_NAME("mProcessThread->join"); status_t status = mProcessThread->join(); @@ -328,11 +328,10 @@ Effect::~Effect() { mInBuffer.clear(); mOutBuffer.clear(); #if MAJOR_VERSION <= 5 - int status = EffectRelease(mHandle); - ALOGW_IF(status, "Error releasing effect %p: %s", mHandle, strerror(-status)); + int status = EffectRelease(handle); + ALOGW_IF(status, "Error releasing effect %p: %s", handle, strerror(-status)); #endif - EffectMap::getInstance().remove(mHandle); - mHandle = 0; + EffectMap::getInstance().remove(handle); } // static @@ -459,7 +458,19 @@ Result Effect::analyzeStatus(const char* funcName, const char* subFuncName, } } -void Effect::getConfigImpl(int commandCode, const char* commandName, GetConfigCallback cb) { +#define RETURN_IF_EFFECT_CLOSED() \ + if (mHandle == kInvalidEffectHandle) { \ + return Result::INVALID_STATE; \ + } +#define RETURN_RESULT_IF_EFFECT_CLOSED(result) \ + if (mHandle == kInvalidEffectHandle) { \ + _hidl_cb(Result::INVALID_STATE, result); \ + return Void(); \ + } + +Return Effect::getConfigImpl(int commandCode, const char* commandName, + GetConfigCallback _hidl_cb) { + RETURN_RESULT_IF_EFFECT_CLOSED(EffectConfig()); uint32_t halResultSize = sizeof(effect_config_t); effect_config_t halConfig{}; status_t status = @@ -468,7 +479,8 @@ void Effect::getConfigImpl(int commandCode, const char* commandName, GetConfigCa if (status == OK) { status = EffectUtils::effectConfigFromHal(halConfig, mIsInput, &config); } - cb(analyzeCommandStatus(commandName, sContextCallToCommand, status), config); + _hidl_cb(analyzeCommandStatus(commandName, sContextCallToCommand, status), config); + return Void(); } Result Effect::getCurrentConfigImpl(uint32_t featureId, uint32_t configSize, @@ -530,6 +542,7 @@ Result Effect::getSupportedConfigsImpl(uint32_t featureId, uint32_t maxConfigs, } Return Effect::prepareForProcessing(prepareForProcessing_cb _hidl_cb) { + RETURN_RESULT_IF_EFFECT_CLOSED(StatusMQ::Descriptor()); status_t status; // Create message queue. if (mStatusMQ) { @@ -576,6 +589,7 @@ Return Effect::prepareForProcessing(prepareForProcessing_cb _hidl_cb) { Return Effect::setProcessBuffers(const AudioBuffer& inBuffer, const AudioBuffer& outBuffer) { + RETURN_IF_EFFECT_CLOSED(); AudioBufferManager& manager = AudioBufferManager::getInstance(); sp tempInBuffer, tempOutBuffer; if (!manager.wrap(inBuffer, &tempInBuffer)) { @@ -600,6 +614,7 @@ Result Effect::sendCommand(int commandCode, const char* commandName) { } Result Effect::sendCommand(int commandCode, const char* commandName, uint32_t size, void* data) { + RETURN_IF_EFFECT_CLOSED(); status_t status = (*mHandle)->command(mHandle, commandCode, size, data, 0, NULL); return analyzeCommandStatus(commandName, sContextCallToCommand, status); } @@ -611,6 +626,7 @@ Result Effect::sendCommandReturningData(int commandCode, const char* commandName Result Effect::sendCommandReturningData(int commandCode, const char* commandName, uint32_t size, void* data, uint32_t* replySize, void* replyData) { + RETURN_IF_EFFECT_CLOSED(); uint32_t expectedReplySize = *replySize; status_t status = (*mHandle)->command(mHandle, commandCode, size, data, replySize, replyData); if (status == OK && *replySize != expectedReplySize) { @@ -635,6 +651,7 @@ Result Effect::sendCommandReturningStatusAndData(int commandCode, const char* co uint32_t size, void* data, uint32_t* replySize, void* replyData, uint32_t minReplySize, CommandSuccessCallback onSuccess) { + RETURN_IF_EFFECT_CLOSED(); status_t status = (*mHandle)->command(mHandle, commandCode, size, data, replySize, replyData); Result retval; if (status == OK && minReplySize >= sizeof(uint32_t) && *replySize >= minReplySize) { @@ -792,13 +809,11 @@ Return Effect::setConfigReverse( } Return Effect::getConfig(getConfig_cb _hidl_cb) { - getConfigImpl(EFFECT_CMD_GET_CONFIG, "GET_CONFIG", _hidl_cb); - return Void(); + return getConfigImpl(EFFECT_CMD_GET_CONFIG, "GET_CONFIG", _hidl_cb); } Return Effect::getConfigReverse(getConfigReverse_cb _hidl_cb) { - getConfigImpl(EFFECT_CMD_GET_CONFIG_REVERSE, "GET_CONFIG_REVERSE", _hidl_cb); - return Void(); + return getConfigImpl(EFFECT_CMD_GET_CONFIG_REVERSE, "GET_CONFIG_REVERSE", _hidl_cb); } Return Effect::getSupportedAuxChannelsConfigs(uint32_t maxConfigs, @@ -845,6 +860,7 @@ Return Effect::offload(const EffectOffloadParameter& param) { } Return Effect::getDescriptor(getDescriptor_cb _hidl_cb) { + RETURN_RESULT_IF_EFFECT_CLOSED(EffectDescriptor()); effect_descriptor_t halDescriptor; memset(&halDescriptor, 0, sizeof(effect_descriptor_t)); status_t status = (*mHandle)->get_descriptor(mHandle, &halDescriptor); @@ -858,6 +874,10 @@ Return Effect::getDescriptor(getDescriptor_cb _hidl_cb) { Return Effect::command(uint32_t commandId, const hidl_vec& data, uint32_t resultMaxSize, command_cb _hidl_cb) { + if (mHandle == kInvalidEffectHandle) { + _hidl_cb(-ENODATA, hidl_vec()); + return Void(); + } uint32_t halDataSize; std::unique_ptr halData = hidlVecToHal(data, &halDataSize); uint32_t halResultSize = resultMaxSize; @@ -942,26 +962,33 @@ Return Effect::setCurrentConfigForFeature(uint32_t featureId, halCmd.size(), &halCmd[0]); } -Return Effect::close() { +std::tuple Effect::closeImpl() { if (mStopProcessThread.load(std::memory_order_relaxed)) { // only this thread modifies - return Result::INVALID_STATE; + return {Result::INVALID_STATE, kInvalidEffectHandle}; } mStopProcessThread.store(true, std::memory_order_release); if (mEfGroup) { mEfGroup->wake(static_cast(MessageQueueFlagBits::REQUEST_QUIT)); } + effect_handle_t handle = mHandle; + mHandle = kInvalidEffectHandle; #if MAJOR_VERSION <= 5 - return Result::OK; + return {Result::OK, handle}; #elif MAJOR_VERSION >= 6 // No need to join the processing thread, it is part of the API contract that the client // must finish processing before closing the effect. - Result retval = - analyzeStatus("EffectRelease", "", sContextCallFunction, EffectRelease(mHandle)); - EffectMap::getInstance().remove(mHandle); - return retval; + Result retval = analyzeStatus("EffectRelease", "", sContextCallFunction, EffectRelease(handle)); + EffectMap::getInstance().remove(handle); + return {retval, handle}; #endif } +Return Effect::close() { + RETURN_IF_EFFECT_CLOSED(); + auto [result, _] = closeImpl(); + return result; +} + Return Effect::debug(const hidl_handle& fd, const hidl_vec& /* options */) { if (fd.getNativeHandle() != nullptr && fd->numFds == 1) { uint32_t cmdData = fd->data[0]; diff --git a/audio/effect/all-versions/default/Effect.h b/audio/effect/all-versions/default/Effect.h index 5d8dcccba6..2bcecec4c7 100644 --- a/audio/effect/all-versions/default/Effect.h +++ b/audio/effect/all-versions/default/Effect.h @@ -23,6 +23,7 @@ #include #include +#include #include #include @@ -186,6 +187,7 @@ struct Effect : public IEffect { // Sets the limit on the maximum size of vendor-provided data structures. static constexpr size_t kMaxDataSize = 1 << 20; + static constexpr effect_handle_t kInvalidEffectHandle = nullptr; static const char* sContextResultOfCommand; static const char* sContextCallToCommand; @@ -208,6 +210,7 @@ struct Effect : public IEffect { static size_t alignedSizeIn(size_t s); template std::unique_ptr hidlVecToHal(const hidl_vec& vec, uint32_t* halDataSize); + std::tuple closeImpl(); void effectAuxChannelsConfigFromHal(const channel_config_t& halConfig, EffectAuxChannelsConfig* config); static void effectAuxChannelsConfigToHal(const EffectAuxChannelsConfig& config, @@ -218,7 +221,8 @@ struct Effect : public IEffect { const void** valueData, std::vector* halParamBuffer); Result analyzeCommandStatus(const char* commandName, const char* context, status_t status); - void getConfigImpl(int commandCode, const char* commandName, GetConfigCallback cb); + Return getConfigImpl(int commandCode, const char* commandName, + GetConfigCallback _hidl_cb); Result getCurrentConfigImpl(uint32_t featureId, uint32_t configSize, GetCurrentConfigSuccessCallback onSuccess); Result getSupportedConfigsImpl(uint32_t featureId, uint32_t maxConfigs, uint32_t configSize, diff --git a/audio/effect/all-versions/vts/functional/VtsHalAudioEffectTargetTest.cpp b/audio/effect/all-versions/vts/functional/VtsHalAudioEffectTargetTest.cpp index d95bb06c3a..ff84f9d562 100644 --- a/audio/effect/all-versions/vts/functional/VtsHalAudioEffectTargetTest.cpp +++ b/audio/effect/all-versions/vts/functional/VtsHalAudioEffectTargetTest.cpp @@ -169,13 +169,15 @@ static const Uuid LOUDNESS_ENHANCER_EFFECT_TYPE = { 0xfe3199be, 0xaed0, 0x413f, 0x87bb, std::array{{0x11, 0x26, 0x0e, 0xb6, 0x3c, 0xf1}}}; -enum { PARAM_FACTORY_NAME, PARAM_EFFECT_UUID }; -using EffectParameter = std::tuple; +enum { PARAM_FACTORY_NAME, PARAM_EFFECT_UUID, PARAM_USE_AFTER_CLOSE }; +using EffectParameter = std::tuple; static inline std::string EffectParameterToString( const ::testing::TestParamInfo& info) { - return ::android::hardware::PrintInstanceNameToString(::testing::TestParamInfo{ - std::get(info.param), info.index}); + std::string prefix = std::get(info.param) ? "UseAfterClose_" : ""; + return prefix.append( + ::android::hardware::PrintInstanceNameToString(::testing::TestParamInfo{ + std::get(info.param), info.index})); } // The main test class for Audio Effect HIDL HAL. @@ -191,6 +193,13 @@ class AudioEffectHidlTest : public ::testing::TestWithParam { Return ret = effect->init(); ASSERT_TRUE(ret.isOk()); ASSERT_EQ(Result::OK, ret); + + useAfterClose = std::get(GetParam()); + if (useAfterClose) { + Return ret = effect->close(); + ASSERT_TRUE(ret.isOk()); + ASSERT_EQ(Result::OK, ret); + } } void TearDown() override { @@ -205,14 +214,34 @@ class AudioEffectHidlTest : public ::testing::TestWithParam { Uuid getEffectType() const { return std::get(GetParam()); } + void checkResult(const Result& result); + void checkResultForUseAfterClose(const Result& result); void findAndCreateEffect(const Uuid& type); void findEffectInstance(const Uuid& type, Uuid* uuid); void getChannelCount(uint32_t* channelCount); sp effectsFactory; sp effect; + bool useAfterClose; }; +void AudioEffectHidlTest::checkResult(const Result& result) { + if (!useAfterClose) { + ASSERT_EQ(Result::OK, result); + } else { + ASSERT_NO_FATAL_FAILURE(checkResultForUseAfterClose(result)); + } +} + +void AudioEffectHidlTest::checkResultForUseAfterClose(const Result& result) { + if (useAfterClose) { + // The actual error does not matter. It's important that the effect did not crash + // while executing any command after a call to "close", and that the returned status + // is not OK. + ASSERT_NE(Result::OK, result); + } +} + void AudioEffectHidlTest::findAndCreateEffect(const Uuid& type) { Uuid effectUuid; ASSERT_NO_FATAL_FAILURE(findEffectInstance(type, &effectUuid)); @@ -257,7 +286,11 @@ void AudioEffectHidlTest::getChannelCount(uint32_t* channelCount) { } }); ASSERT_TRUE(ret.isOk()); - ASSERT_EQ(Result::OK, retval); + ASSERT_NO_FATAL_FAILURE(checkResult(retval)); + if (useAfterClose) { + *channelCount = 1; + return; + } #if MAJOR_VERSION <= 6 ASSERT_TRUE(audio_channel_mask_is_valid( static_cast(currentConfig.outputCfg.channels))); @@ -276,7 +309,7 @@ TEST_P(AudioEffectHidlTest, Close) { description("Verify that an effect can be closed"); Return ret = effect->close(); EXPECT_TRUE(ret.isOk()); - EXPECT_EQ(Result::OK, ret); + ASSERT_NO_FATAL_FAILURE(checkResult(ret)); } TEST_P(AudioEffectHidlTest, GetDescriptor) { @@ -290,7 +323,8 @@ TEST_P(AudioEffectHidlTest, GetDescriptor) { } }); EXPECT_TRUE(ret.isOk()); - EXPECT_EQ(Result::OK, retval); + ASSERT_NO_FATAL_FAILURE(checkResult(retval)); + if (useAfterClose) return; EXPECT_EQ(getEffectType(), actualType); } @@ -307,7 +341,8 @@ TEST_P(AudioEffectHidlTest, GetSetConfig) { } }); EXPECT_TRUE(ret.isOk()); - EXPECT_EQ(Result::OK, retval); + EXPECT_NO_FATAL_FAILURE(checkResult(retval)); + if (useAfterClose) return; Return ret2 = effect->setConfig(currentConfig, nullptr, nullptr); EXPECT_TRUE(ret2.isOk()); EXPECT_EQ(Result::OK, ret2); @@ -336,7 +371,8 @@ TEST_P(AudioEffectHidlTest, SetConfigInvalidArguments) { } }); EXPECT_TRUE(ret.isOk()); - EXPECT_EQ(Result::OK, retval); + EXPECT_NO_FATAL_FAILURE(checkResult(retval)); + if (useAfterClose) return; for (const auto& invalidInputCfg : generateInvalidConfigs(currentConfig.inputCfg)) { EffectConfig invalidConfig = currentConfig; invalidConfig.inputCfg = invalidInputCfg; @@ -356,27 +392,35 @@ TEST_P(AudioEffectHidlTest, SetConfigInvalidArguments) { TEST_P(AudioEffectHidlTest, GetConfigReverse) { description("Verify that GetConfigReverse does not crash"); - Return ret = effect->getConfigReverse([&](Result, const EffectConfig&) {}); + Result retval = Result::OK; + Return ret = effect->getConfigReverse([&](Result r, const EffectConfig&) { retval = r; }); EXPECT_TRUE(ret.isOk()); + EXPECT_NO_FATAL_FAILURE(checkResultForUseAfterClose(retval)); } TEST_P(AudioEffectHidlTest, GetSupportedAuxChannelsConfigs) { description("Verify that GetSupportedAuxChannelsConfigs does not crash"); + Result retval = Result::OK; Return ret = effect->getSupportedAuxChannelsConfigs( - 0, [&](Result, const hidl_vec&) {}); + 0, [&](Result r, const hidl_vec&) { retval = r; }); EXPECT_TRUE(ret.isOk()); + EXPECT_NO_FATAL_FAILURE(checkResultForUseAfterClose(retval)); } TEST_P(AudioEffectHidlTest, GetAuxChannelsConfig) { description("Verify that GetAuxChannelsConfig does not crash"); - Return ret = effect->getAuxChannelsConfig([&](Result, const EffectAuxChannelsConfig&) {}); + Result retval = Result::OK; + Return ret = effect->getAuxChannelsConfig( + [&](Result r, const EffectAuxChannelsConfig&) { retval = r; }); EXPECT_TRUE(ret.isOk()); + EXPECT_NO_FATAL_FAILURE(checkResultForUseAfterClose(retval)); } TEST_P(AudioEffectHidlTest, SetAuxChannelsConfig) { description("Verify that SetAuxChannelsConfig does not crash"); Return ret = effect->setAuxChannelsConfig(EffectAuxChannelsConfig()); EXPECT_TRUE(ret.isOk()); + EXPECT_NO_FATAL_FAILURE(checkResultForUseAfterClose(ret)); } // Not generated automatically because AudioBuffer contains @@ -427,45 +471,56 @@ TEST_P(AudioEffectHidlTest, Reset) { description("Verify that Reset preserves effect configuration"); Result retval = Result::NOT_INITIALIZED; EffectConfig originalConfig; - Return ret = effect->getConfig([&](Result r, const EffectConfig& conf) { - retval = r; - if (r == Result::OK) { - originalConfig = conf; - } - }); - ASSERT_TRUE(ret.isOk()); - ASSERT_EQ(Result::OK, retval); + if (!useAfterClose) { + Return ret = effect->getConfig([&](Result r, const EffectConfig& conf) { + retval = r; + if (r == Result::OK) { + originalConfig = conf; + } + }); + ASSERT_TRUE(ret.isOk()); + ASSERT_EQ(Result::OK, retval); + } Return ret2 = effect->reset(); EXPECT_TRUE(ret2.isOk()); - EXPECT_EQ(Result::OK, ret2); - EffectConfig configAfterReset; - ret = effect->getConfig([&](Result r, const EffectConfig& conf) { - retval = r; - if (r == Result::OK) { - configAfterReset = conf; - } - }); - EXPECT_EQ(originalConfig, configAfterReset); + EXPECT_NO_FATAL_FAILURE(checkResult(ret2)); + if (!useAfterClose) { + EffectConfig configAfterReset; + Return ret = effect->getConfig([&](Result r, const EffectConfig& conf) { + retval = r; + if (r == Result::OK) { + configAfterReset = conf; + } + }); + EXPECT_EQ(originalConfig, configAfterReset); + } } TEST_P(AudioEffectHidlTest, DisableEnableDisable) { description("Verify Disable -> Enable -> Disable sequence for an effect"); Return ret = effect->disable(); EXPECT_TRUE(ret.isOk()); - // Note: some legacy effects may return -EINVAL (INVALID_ARGUMENTS), - // more canonical is to return -ENOSYS (NOT_SUPPORTED) - EXPECT_TRUE(ret == Result::NOT_SUPPORTED || ret == Result::INVALID_ARGUMENTS); + if (!useAfterClose) { + // Note: some legacy effects may return -EINVAL (INVALID_ARGUMENTS), + // more canonical is to return -ENOSYS (NOT_SUPPORTED) + EXPECT_TRUE(ret == Result::NOT_SUPPORTED || ret == Result::INVALID_ARGUMENTS); + } else { + EXPECT_NO_FATAL_FAILURE(checkResultForUseAfterClose(ret)); + } ret = effect->enable(); EXPECT_TRUE(ret.isOk()); - EXPECT_EQ(Result::OK, ret); + EXPECT_NO_FATAL_FAILURE(checkResult(ret)); ret = effect->disable(); EXPECT_TRUE(ret.isOk()); - EXPECT_EQ(Result::OK, ret); + EXPECT_NO_FATAL_FAILURE(checkResult(ret)); } #if MAJOR_VERSION >= 7 TEST_P(AudioEffectHidlTest, SetDeviceInvalidDeviceAddress) { description("Verify that invalid device address is rejected by SetDevice"); + if (useAfterClose) { + GTEST_SKIP() << "Does not make sense for the useAfterClose case"; + } DeviceAddress device{.deviceType = "random_string"}; Return ret = effect->setDevice(device); EXPECT_TRUE(ret.isOk()); @@ -482,13 +537,13 @@ TEST_P(AudioEffectHidlTest, SetDevice) { Return ret = effect->setDevice(device); #endif EXPECT_TRUE(ret.isOk()); - EXPECT_EQ(Result::OK, ret); + EXPECT_NO_FATAL_FAILURE(checkResult(ret)); } TEST_P(AudioEffectHidlTest, SetAndGetVolume) { description("Verify that SetAndGetVolume method works for an effect"); uint32_t channelCount; - getChannelCount(&channelCount); + ASSERT_NO_FATAL_FAILURE(getChannelCount(&channelCount)); hidl_vec volumes; volumes.resize(channelCount); for (uint32_t i = 0; i < channelCount; ++i) { @@ -498,13 +553,13 @@ TEST_P(AudioEffectHidlTest, SetAndGetVolume) { Return ret = effect->setAndGetVolume(volumes, [&](Result r, const hidl_vec&) { retval = r; }); EXPECT_TRUE(ret.isOk()); - EXPECT_EQ(Result::OK, retval); + EXPECT_NO_FATAL_FAILURE(checkResult(retval)); } TEST_P(AudioEffectHidlTest, VolumeChangeNotification) { description("Verify that effect accepts VolumeChangeNotification"); uint32_t channelCount; - getChannelCount(&channelCount); + ASSERT_NO_FATAL_FAILURE(getChannelCount(&channelCount)); hidl_vec volumes; volumes.resize(channelCount); for (uint32_t i = 0; i < channelCount; ++i) { @@ -512,25 +567,29 @@ TEST_P(AudioEffectHidlTest, VolumeChangeNotification) { } Return ret = effect->volumeChangeNotification(volumes); EXPECT_TRUE(ret.isOk()); - EXPECT_EQ(Result::OK, ret); + EXPECT_NO_FATAL_FAILURE(checkResult(ret)); } TEST_P(AudioEffectHidlTest, SetAudioMode) { description("Verify that SetAudioMode works for an effect"); Return ret = effect->setAudioMode(AudioMode::NORMAL); EXPECT_TRUE(ret.isOk()); - EXPECT_EQ(Result::OK, ret); + EXPECT_NO_FATAL_FAILURE(checkResult(ret)); } TEST_P(AudioEffectHidlTest, SetConfigReverse) { description("Verify that SetConfigReverse does not crash"); Return ret = effect->setConfigReverse(EffectConfig(), nullptr, nullptr); EXPECT_TRUE(ret.isOk()); + EXPECT_NO_FATAL_FAILURE(checkResultForUseAfterClose(ret)); } #if MAJOR_VERSION >= 7 TEST_P(AudioEffectHidlTest, SetInputDeviceInvalidDeviceAddress) { description("Verify that invalid device address is rejected by SetInputDevice"); + if (useAfterClose) { + GTEST_SKIP() << "Does not make sense for the useAfterClose case"; + } DeviceAddress device{.deviceType = "random_string"}; Return ret = effect->setInputDevice(device); EXPECT_TRUE(ret.isOk()); @@ -548,11 +607,15 @@ TEST_P(AudioEffectHidlTest, SetInputDevice) { Return ret = effect->setInputDevice(device); #endif EXPECT_TRUE(ret.isOk()); + EXPECT_NO_FATAL_FAILURE(checkResultForUseAfterClose(ret)); } #if MAJOR_VERSION >= 7 TEST_P(AudioEffectHidlTest, SetInvalidAudioSource) { description("Verify that an invalid audio source is rejected by SetAudioSource"); + if (useAfterClose) { + GTEST_SKIP() << "Does not make sense for the useAfterClose case"; + } Return ret = effect->setAudioSource("random_string"); ASSERT_TRUE(ret.isOk()); EXPECT_TRUE(ret == Result::INVALID_ARGUMENTS || ret == Result::NOT_SUPPORTED) @@ -568,12 +631,14 @@ TEST_P(AudioEffectHidlTest, SetAudioSource) { Return ret = effect->setAudioSource(toString(xsd::AudioSource::AUDIO_SOURCE_MIC)); #endif EXPECT_TRUE(ret.isOk()); + EXPECT_NO_FATAL_FAILURE(checkResultForUseAfterClose(ret)); } TEST_P(AudioEffectHidlTest, Offload) { description("Verify that calling Offload method does not crash"); Return ret = effect->offload(EffectOffloadParameter{}); EXPECT_TRUE(ret.isOk()); + EXPECT_NO_FATAL_FAILURE(checkResultForUseAfterClose(ret)); } TEST_P(AudioEffectHidlTest, PrepareForProcessing) { @@ -582,7 +647,7 @@ TEST_P(AudioEffectHidlTest, PrepareForProcessing) { Return ret = effect->prepareForProcessing( [&](Result r, const MQDescriptorSync&) { retval = r; }); EXPECT_TRUE(ret.isOk()); - EXPECT_EQ(Result::OK, retval); + EXPECT_NO_FATAL_FAILURE(checkResult(retval)); } TEST_P(AudioEffectHidlTest, SetProcessBuffers) { @@ -601,7 +666,7 @@ TEST_P(AudioEffectHidlTest, SetProcessBuffers) { ASSERT_TRUE(success); Return ret2 = effect->setProcessBuffers(buffer, buffer); EXPECT_TRUE(ret2.isOk()); - EXPECT_EQ(Result::OK, ret2); + EXPECT_NO_FATAL_FAILURE(checkResult(ret2)); } TEST_P(AudioEffectHidlTest, Command) { @@ -615,6 +680,7 @@ TEST_P(AudioEffectHidlTest, SetParameter) { description("Verify that SetParameter does not crash"); Return ret = effect->setParameter(hidl_vec(), hidl_vec()); EXPECT_TRUE(ret.isOk()); + EXPECT_NO_FATAL_FAILURE(checkResultForUseAfterClose(ret)); } TEST_P(AudioEffectHidlTest, GetParameter) { @@ -630,6 +696,9 @@ TEST_P(AudioEffectHidlTest, GetParameterInvalidMaxReplySize) { if (!isNewDeviceLaunchingOnTPlus) { GTEST_SKIP() << "The test only applies to devices launching on T or later"; } + if (useAfterClose) { + GTEST_SKIP() << "Does not make sense for the useAfterClose case"; + } // Use a non-empty parameter to avoid being rejected by any earlier checks. hidl_vec parameter; parameter.resize(16); @@ -647,16 +716,20 @@ TEST_P(AudioEffectHidlTest, GetParameterInvalidMaxReplySize) { TEST_P(AudioEffectHidlTest, GetSupportedConfigsForFeature) { description("Verify that GetSupportedConfigsForFeature does not crash"); + Result retval = Result::OK; Return ret = effect->getSupportedConfigsForFeature( - 0, 0, 0, [&](Result, uint32_t, const hidl_vec&) {}); + 0, 0, 0, [&](Result r, uint32_t, const hidl_vec&) { retval = r; }); EXPECT_TRUE(ret.isOk()); + EXPECT_NO_FATAL_FAILURE(checkResultForUseAfterClose(retval)); } TEST_P(AudioEffectHidlTest, GetCurrentConfigForFeature) { description("Verify that GetCurrentConfigForFeature does not crash"); - Return ret = - effect->getCurrentConfigForFeature(0, 0, [&](Result, const hidl_vec&) {}); + Result retval = Result::OK; + Return ret = effect->getCurrentConfigForFeature( + 0, 0, [&](Result r, const hidl_vec&) { retval = r; }); EXPECT_TRUE(ret.isOk()); + EXPECT_NO_FATAL_FAILURE(checkResultForUseAfterClose(retval)); } TEST_P(AudioEffectHidlTest, SetCurrentConfigForFeature) { @@ -671,6 +744,9 @@ TEST_P(AudioEffectHidlTest, GetSupportedConfigsForFeatureInvalidConfigSize) { if (!isNewDeviceLaunchingOnTPlus) { GTEST_SKIP() << "The test only applies to devices launching on T or later"; } + if (useAfterClose) { + GTEST_SKIP() << "Does not make sense for the useAfterClose case"; + } // Use very large size to ensure that the service does not crash. const uint32_t veryLargeConfigSize = std::numeric_limits::max() - 100; Result retval = Result::OK; @@ -687,6 +763,9 @@ TEST_P(AudioEffectHidlTest, GetCurrentConfigForFeatureInvalidConfigSize) { if (!isNewDeviceLaunchingOnTPlus) { GTEST_SKIP() << "The test only applies to devices launching on T or later"; } + if (useAfterClose) { + GTEST_SKIP() << "Does not make sense for the useAfterClose case"; + } // Use very large size to ensure that the service does not crash. const uint32_t veryLargeConfigSize = std::numeric_limits::max() - 100; Result retval = Result::OK; @@ -729,7 +808,8 @@ void EqualizerAudioEffectHidlTest::getNumBands(uint16_t* numBands) { } }); ASSERT_TRUE(ret.isOk()); - ASSERT_EQ(Result::OK, retval); + ASSERT_NO_FATAL_FAILURE(checkResult(retval)); + if (useAfterClose) *numBands = 1; } void EqualizerAudioEffectHidlTest::getLevelRange(int16_t* minLevel, int16_t* maxLevel) { @@ -742,7 +822,11 @@ void EqualizerAudioEffectHidlTest::getLevelRange(int16_t* minLevel, int16_t* max } }); ASSERT_TRUE(ret.isOk()); - ASSERT_EQ(Result::OK, retval); + ASSERT_NO_FATAL_FAILURE(checkResult(retval)); + if (useAfterClose) { + *minLevel = 0; + *maxLevel = 255; + } } void EqualizerAudioEffectHidlTest::getBandFrequencyRange(uint16_t band, uint32_t* minFreq, @@ -757,7 +841,7 @@ void EqualizerAudioEffectHidlTest::getBandFrequencyRange(uint16_t band, uint32_t } }); ASSERT_TRUE(ret.isOk()); - ASSERT_EQ(Result::OK, retval); + ASSERT_NO_FATAL_FAILURE(checkResult(retval)); ret = equalizer->getBandCenterFrequency(band, [&](Result r, uint32_t center) { retval = r; if (retval == Result::OK) { @@ -765,7 +849,12 @@ void EqualizerAudioEffectHidlTest::getBandFrequencyRange(uint16_t band, uint32_t } }); ASSERT_TRUE(ret.isOk()); - ASSERT_EQ(Result::OK, retval); + ASSERT_NO_FATAL_FAILURE(checkResult(retval)); + if (useAfterClose) { + *minFreq = 20; + *centerFreq = 10000; + *maxFreq = 20000; + } } void EqualizerAudioEffectHidlTest::getPresetCount(size_t* count) { @@ -777,37 +866,38 @@ void EqualizerAudioEffectHidlTest::getPresetCount(size_t* count) { } }); ASSERT_TRUE(ret.isOk()); - ASSERT_EQ(Result::OK, retval); + ASSERT_NO_FATAL_FAILURE(checkResult(retval)); + if (useAfterClose) *count = 1; } TEST_P(EqualizerAudioEffectHidlTest, GetNumBands) { description("Verify that Equalizer effect reports at least one band"); uint16_t numBands = 0; - getNumBands(&numBands); + ASSERT_NO_FATAL_FAILURE(getNumBands(&numBands)); EXPECT_GT(numBands, 0); } TEST_P(EqualizerAudioEffectHidlTest, GetLevelRange) { description("Verify that Equalizer effect reports adequate band level range"); int16_t minLevel = 0x7fff, maxLevel = 0; - getLevelRange(&minLevel, &maxLevel); + ASSERT_NO_FATAL_FAILURE(getLevelRange(&minLevel, &maxLevel)); EXPECT_GT(maxLevel, minLevel); } TEST_P(EqualizerAudioEffectHidlTest, GetSetBandLevel) { description("Verify that manipulating band levels works for Equalizer effect"); uint16_t numBands = 0; - getNumBands(&numBands); + ASSERT_NO_FATAL_FAILURE(getNumBands(&numBands)); ASSERT_GT(numBands, 0); int16_t levels[3]{0x7fff, 0, 0}; - getLevelRange(&levels[0], &levels[2]); + ASSERT_NO_FATAL_FAILURE(getLevelRange(&levels[0], &levels[2])); ASSERT_GT(levels[2], levels[0]); levels[1] = (levels[2] + levels[0]) / 2; for (uint16_t i = 0; i < numBands; ++i) { for (size_t j = 0; j < ARRAY_SIZE(levels); ++j) { Return ret = equalizer->setBandLevel(i, levels[j]); EXPECT_TRUE(ret.isOk()); - EXPECT_EQ(Result::OK, ret); + EXPECT_NO_FATAL_FAILURE(checkResult(ret)); Result retval = Result::NOT_INITIALIZED; int16_t actualLevel; Return ret2 = equalizer->getBandLevel(i, [&](Result r, int16_t l) { @@ -817,8 +907,10 @@ TEST_P(EqualizerAudioEffectHidlTest, GetSetBandLevel) { } }); EXPECT_TRUE(ret2.isOk()); - EXPECT_EQ(Result::OK, retval); - EXPECT_EQ(levels[j], actualLevel); + EXPECT_NO_FATAL_FAILURE(checkResult(retval)); + if (!useAfterClose) { + EXPECT_EQ(levels[j], actualLevel); + } } } } @@ -826,11 +918,11 @@ TEST_P(EqualizerAudioEffectHidlTest, GetSetBandLevel) { TEST_P(EqualizerAudioEffectHidlTest, GetBandCenterFrequencyAndRange) { description("Verify that Equalizer effect reports adequate band frequency range"); uint16_t numBands = 0; - getNumBands(&numBands); + ASSERT_NO_FATAL_FAILURE(getNumBands(&numBands)); ASSERT_GT(numBands, 0); for (uint16_t i = 0; i < numBands; ++i) { uint32_t minFreq = 0xffffffff, centerFreq = 0xffffffff, maxFreq = 0xffffffff; - getBandFrequencyRange(i, &minFreq, ¢erFreq, &maxFreq); + ASSERT_NO_FATAL_FAILURE(getBandFrequencyRange(i, &minFreq, ¢erFreq, &maxFreq)); // Note: NXP legacy implementation reports "1" as upper bound for last band, // so this check fails. EXPECT_GE(maxFreq, centerFreq); @@ -841,7 +933,7 @@ TEST_P(EqualizerAudioEffectHidlTest, GetBandCenterFrequencyAndRange) { TEST_P(EqualizerAudioEffectHidlTest, GetBandForFrequency) { description("Verify that Equalizer effect supports GetBandForFrequency correctly"); uint16_t numBands = 0; - getNumBands(&numBands); + ASSERT_NO_FATAL_FAILURE(getNumBands(&numBands)); ASSERT_GT(numBands, 0); for (uint16_t i = 0; i < numBands; ++i) { uint32_t freqs[3]{0, 0, 0}; @@ -861,8 +953,10 @@ TEST_P(EqualizerAudioEffectHidlTest, GetBandForFrequency) { } }); EXPECT_TRUE(ret.isOk()); - EXPECT_EQ(Result::OK, retval); - EXPECT_EQ(i, actualBand) << "Frequency: " << freqs[j]; + EXPECT_NO_FATAL_FAILURE(checkResult(retval)); + if (!useAfterClose) { + EXPECT_EQ(i, actualBand) << "Frequency: " << freqs[j]; + } } } } @@ -870,19 +964,19 @@ TEST_P(EqualizerAudioEffectHidlTest, GetBandForFrequency) { TEST_P(EqualizerAudioEffectHidlTest, GetPresetNames) { description("Verify that Equalizer effect reports at least one preset"); size_t presetCount; - getPresetCount(&presetCount); + ASSERT_NO_FATAL_FAILURE(getPresetCount(&presetCount)); EXPECT_GT(presetCount, 0u); } TEST_P(EqualizerAudioEffectHidlTest, GetSetCurrentPreset) { description("Verify that manipulating the current preset for Equalizer effect"); size_t presetCount; - getPresetCount(&presetCount); + ASSERT_NO_FATAL_FAILURE(getPresetCount(&presetCount)); ASSERT_GT(presetCount, 0u); for (uint16_t i = 0; i < presetCount; ++i) { Return ret = equalizer->setCurrentPreset(i); EXPECT_TRUE(ret.isOk()); - EXPECT_EQ(Result::OK, ret); + EXPECT_NO_FATAL_FAILURE(checkResult(ret)); Result retval = Result::NOT_INITIALIZED; uint16_t actualPreset = 0xffff; Return ret2 = equalizer->getCurrentPreset([&](Result r, uint16_t p) { @@ -892,8 +986,10 @@ TEST_P(EqualizerAudioEffectHidlTest, GetSetCurrentPreset) { } }); EXPECT_TRUE(ret2.isOk()); - EXPECT_EQ(Result::OK, retval); - EXPECT_EQ(i, actualPreset); + EXPECT_NO_FATAL_FAILURE(checkResult(retval)); + if (!useAfterClose) { + EXPECT_EQ(i, actualPreset); + } } } @@ -904,7 +1000,7 @@ TEST_P(EqualizerAudioEffectHidlTest, GetSetAllProperties) { using AllProperties = ::android::hardware::audio::effect::CPP_VERSION::IEqualizerEffect::AllProperties; uint16_t numBands = 0; - getNumBands(&numBands); + ASSERT_NO_FATAL_FAILURE(getNumBands(&numBands)); ASSERT_GT(numBands, 0); AllProperties props; props.bandLevels.resize(numBands); @@ -919,7 +1015,7 @@ TEST_P(EqualizerAudioEffectHidlTest, GetSetAllProperties) { props.curPreset = -1; Return ret = equalizer->setAllProperties(props); EXPECT_TRUE(ret.isOk()); - EXPECT_EQ(Result::OK, ret); + EXPECT_NO_FATAL_FAILURE(checkResult(ret)); Return ret2 = equalizer->getAllProperties([&](Result r, AllProperties p) { retval = r; if (retval == Result::OK) { @@ -927,14 +1023,16 @@ TEST_P(EqualizerAudioEffectHidlTest, GetSetAllProperties) { } }); EXPECT_TRUE(ret2.isOk()); - EXPECT_EQ(Result::OK, retval); - EXPECT_EQ(props.bandLevels, actualProps.bandLevels); + EXPECT_NO_FATAL_FAILURE(checkResult(retval)); + if (!useAfterClose) { + EXPECT_EQ(props.bandLevels, actualProps.bandLevels); + } // Verify setting of the current preset via properties. props.curPreset = 0; // Assuming there is at least one preset. ret = equalizer->setAllProperties(props); EXPECT_TRUE(ret.isOk()); - EXPECT_EQ(Result::OK, ret); + EXPECT_NO_FATAL_FAILURE(checkResult(ret)); ret2 = equalizer->getAllProperties([&](Result r, AllProperties p) { retval = r; if (retval == Result::OK) { @@ -942,8 +1040,10 @@ TEST_P(EqualizerAudioEffectHidlTest, GetSetAllProperties) { } }); EXPECT_TRUE(ret2.isOk()); - EXPECT_EQ(Result::OK, retval); - EXPECT_EQ(props.curPreset, actualProps.curPreset); + EXPECT_NO_FATAL_FAILURE(checkResult(retval)); + if (!useAfterClose) { + EXPECT_EQ(props.curPreset, actualProps.curPreset); + } } // The main test class for Equalizer Audio Effect HIDL HAL. @@ -971,7 +1071,7 @@ TEST_P(LoudnessEnhancerAudioEffectHidlTest, GetSetTargetGain) { const int32_t gain = 100; Return ret = enhancer->setTargetGain(gain); EXPECT_TRUE(ret.isOk()); - EXPECT_EQ(Result::OK, ret); + EXPECT_NO_FATAL_FAILURE(checkResult(ret)); int32_t actualGain = 0; Result retval; Return ret2 = enhancer->getTargetGain([&](Result r, int32_t g) { @@ -981,8 +1081,10 @@ TEST_P(LoudnessEnhancerAudioEffectHidlTest, GetSetTargetGain) { } }); EXPECT_TRUE(ret2.isOk()); - EXPECT_EQ(Result::OK, retval); - EXPECT_EQ(gain, actualGain); + EXPECT_NO_FATAL_FAILURE(checkResult(retval)); + if (!useAfterClose) { + EXPECT_EQ(gain, actualGain); + } } INSTANTIATE_TEST_SUITE_P(EffectsFactory, AudioEffectsFactoryHidlTest, @@ -993,25 +1095,29 @@ INSTANTIATE_TEST_SUITE_P( Equalizer_IEffect, AudioEffectHidlTest, ::testing::Combine(::testing::ValuesIn(::android::hardware::getAllHalInstanceNames( IEffectsFactory::descriptor)), - ::testing::Values(EQUALIZER_EFFECT_TYPE)), + ::testing::Values(EQUALIZER_EFFECT_TYPE), + ::testing::Values(false, true) /*useAfterClose*/), EffectParameterToString); INSTANTIATE_TEST_SUITE_P( LoudnessEnhancer_IEffect, AudioEffectHidlTest, ::testing::Combine(::testing::ValuesIn(::android::hardware::getAllHalInstanceNames( IEffectsFactory::descriptor)), - ::testing::Values(LOUDNESS_ENHANCER_EFFECT_TYPE)), + ::testing::Values(LOUDNESS_ENHANCER_EFFECT_TYPE), + ::testing::Values(false, true) /*useAfterClose*/), EffectParameterToString); INSTANTIATE_TEST_SUITE_P( Equalizer, EqualizerAudioEffectHidlTest, ::testing::Combine(::testing::ValuesIn(::android::hardware::getAllHalInstanceNames( IEffectsFactory::descriptor)), - ::testing::Values(EQUALIZER_EFFECT_TYPE)), + ::testing::Values(EQUALIZER_EFFECT_TYPE), + ::testing::Values(false, true) /*useAfterClose*/), EffectParameterToString); INSTANTIATE_TEST_SUITE_P( LoudnessEnhancer, LoudnessEnhancerAudioEffectHidlTest, ::testing::Combine(::testing::ValuesIn(::android::hardware::getAllHalInstanceNames( IEffectsFactory::descriptor)), - ::testing::Values(LOUDNESS_ENHANCER_EFFECT_TYPE)), + ::testing::Values(LOUDNESS_ENHANCER_EFFECT_TYPE), + ::testing::Values(false, true) /*useAfterClose*/), EffectParameterToString); // When the VTS test runs on a device lacking the corresponding HAL version the parameter // list is empty, this isn't a problem.