Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 0d92059a authored by Mikhail Naganov's avatar Mikhail Naganov
Browse files

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
parent 9c35c525

audio/effect/all-versions/default/Effect.cpp

+45 −18
Original line number Diff line number Diff line
@@ -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<void> 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<void> 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<void> Effect::prepareForProcessing(prepareForProcessing_cb _hidl_cb) { 


Return<Result> Effect::setProcessBuffers(const AudioBuffer& inBuffer,

                                         const AudioBuffer& outBuffer) {

    RETURN_IF_EFFECT_CLOSED();

    AudioBufferManager& manager = AudioBufferManager::getInstance();

    sp<AudioBufferWrapper> 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<Result> Effect::setConfigReverse( 
}



Return<void> 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<void> 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<void> Effect::getSupportedAuxChannelsConfigs(uint32_t maxConfigs,
@@ -845,6 +860,7 @@ Return<Result> Effect::offload(const EffectOffloadParameter& param) { 
}



Return<void> 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<void> Effect::getDescriptor(getDescriptor_cb _hidl_cb) { 


Return<void> Effect::command(uint32_t commandId, const hidl_vec<uint8_t>& data,

                             uint32_t resultMaxSize, command_cb _hidl_cb) {

    if (mHandle == kInvalidEffectHandle) {

        _hidl_cb(-ENODATA, hidl_vec<uint8_t>());

        return Void();

    }

    uint32_t halDataSize;

    std::unique_ptr<uint8_t[]> halData = hidlVecToHal(data, &halDataSize);

    uint32_t halResultSize = resultMaxSize;
@@ -942,26 +962,33 @@ Return<Result> Effect::setCurrentConfigForFeature(uint32_t featureId, 
                                      halCmd.size(), &halCmd[0]);

}



Return<Result> Effect::close() {

std::tuple<Result, effect_handle_t> 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<uint32_t>(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<Result> Effect::close() {

    RETURN_IF_EFFECT_CLOSED();

    auto [result, _] = closeImpl();

    return result;

}



Return<void> Effect::debug(const hidl_handle& fd, const hidl_vec<hidl_string>& /* options */) {

    if (fd.getNativeHandle() != nullptr && fd->numFds == 1) {

        uint32_t cmdData = fd->data[0];

audio/effect/all-versions/default/Effect.h

+5 −1
Original line number Diff line number Diff line
@@ -23,6 +23,7 @@ 


#include <atomic>

#include <memory>

#include <tuple>

#include <vector>



#include <fmq/EventFlag.h>
@@ -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 <typename T>

    std::unique_ptr<uint8_t[]> hidlVecToHal(const hidl_vec<T>& vec, uint32_t* halDataSize);

    std::tuple<Result, effect_handle_t> 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<uint8_t>* halParamBuffer);



    Result analyzeCommandStatus(const char* commandName, const char* context, status_t status);

    void getConfigImpl(int commandCode, const char* commandName, GetConfigCallback cb);

    Return<void> 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,

audio/effect/all-versions/vts/functional/VtsHalAudioEffectTargetTest.cpp

+187 −81

File changed.

Preview size limit exceeded, changes collapsed.