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

Commit 620f3201 authored by Shunkai Yao's avatar Shunkai Yao
Browse files

Support effect destroy at any state 

Flag: EXEMPT bugfix
Bug: 379776482
Test: atest --test-mapping hardware/interfaces/audio/aidl/vts:presubmit
Test: YouTubeMusic Equalizer on Pixel
Change-Id: I98c2ece090cf1d691180e10b3d92b7fb57d9f229
Merged-In: I98c2ece090cf1d691180e10b3d92b7fb57d9f229
parent 27ae74d5

audio/aidl/android/hardware/audio/effect/IFactory.aidl

+8 −3
Original line number Diff line number Diff line
@@ -74,13 +74,18 @@ interface IFactory { 


    /**

     * Called by the framework to destroy the effect and free up all currently allocated resources.

     * It is recommended to destroy the effect from the client side as soon as it is becomes unused.

     * This method can be called at any time to destroy an effect instance. It is recommended to

     * destroy the effect from the client side as soon as it becomes unused to free up resources.

     *

     * The client must ensure effect instance is closed before destroy.

     * The effect instance must handle any necessary cleanup and resource deallocation.

     * If the effect is in the **PROCESSING** or **DRAINING** state, it must gracefully stop

     * processing before destruction.

     * The effect must ensure that all internal states are properly cleaned up to prevent resource

     * leaks.

     *

     * @param handle The handle of effect instance to be destroyed.

     * @throws EX_ILLEGAL_ARGUMENT if the effect handle is not valid.

     * @throws EX_ILLEGAL_STATE if the effect instance is not in a proper state to be destroyed.

     * @throws EX_ILLEGAL_STATE if the effect instance can not be destroyed.

     */

    void destroyEffect(in IEffect handle);

}

audio/aidl/android/hardware/audio/effect/State.aidl

+5 −0
Original line number Diff line number Diff line
@@ -24,6 +24,8 @@ package android.hardware.audio.effect; 
 * it should transfer to IDLE state after handle the command successfully. Effect instance should

 * consume minimal resource and transfer to INIT state after it was close().

 *

 * An effect instance can be destroyed from any state using `IFactory.destroyEffect()`.

 *

 * Refer to the state machine diagram `state.gv` for a detailed state diagram.

 */

@VintfStability
@@ -66,6 +68,7 @@ enum State { 
     * - Transitions to **INIT** on `IEffect.close()`.

     * - Remains in **IDLE** on `IEffect.getParameter()`, `IEffect.setParameter()`,

     *   `IEffect.getDescriptor()`, `IEffect.command(CommandId.RESET)`, and `IEffect.reopen()`.

     * - Transitions to the final state on `IFactory.destroyEffect()`.

     */

    IDLE,
@@ -98,6 +101,7 @@ enum State { 
     *   stop processing with `CommandId.STOP` before closing.

     * - If `IEffect.close()` is called in this state, the effect instance should stop processing,

     *   transition to **IDLE**, and then close.

     * - Transitions to the final state on `IFactory.destroyEffect()`.

     */

    PROCESSING,
@@ -123,6 +127,7 @@ enum State { 
     * - If not implemented, the effect instance may transition directly from **PROCESSING** to

     *   **IDLE** without this intermediate state.

     * - Any `CommandId.STOP` commands received during **DRAINING** should be ignored.

     * - Transitions to the final state on `IFactory.destroyEffect()`.

     */

    DRAINING,

}

audio/aidl/android/hardware/audio/effect/state.gv

+19 −29
Original line number Diff line number Diff line
@@ -13,8 +13,8 @@ 
 * See the License for the specific language governing permissions and

 * limitations under the License.

 */

// To render: "dot -Tpng state.gv -o state.png"



// To render: "dot -Tpng state.gv -o state.png"

digraph effect_state_machine {



    rankdir=LR; // Left to Right layout
@@ -29,40 +29,30 @@ digraph effect_state_machine { 
    I [shape=point, fillcolor=black, width=0.2];



    // Final state node

    F [shape=doublecircle, fillcolor=white, width=0.2];

    F [shape=doublecircle, label="Destroyed"];



    // Define other nodes with colors

    INIT [shape=ellipse, fillcolor=lightgreen];

    IDLE [shape=ellipse, fillcolor=lightblue];

    PROCESSING [shape=ellipse, fillcolor=lightyellow];

    DRAINING [shape=ellipse, fillcolor=lightgrey];

    INIT [shape=ellipse, fillcolor=lightgreen, label="INIT"];

    IDLE [shape=ellipse, fillcolor=lightblue, label="IDLE"];

    PROCESSING [shape=ellipse, fillcolor=lightyellow, label="PROCESSING"];

    DRAINING [shape=ellipse, fillcolor=lightgrey, label="DRAINING"];

    ANY_STATE [shape=ellipse, style=dashed, label="Any State", fillcolor=white];



    // Transitions

    // Main transitions

    I -> INIT [label="IFactory.createEffect", fontcolor="navy"];



    INIT -> F [label="IFactory.destroyEffect"];



    INIT -> IDLE [label="IEffect.open()", fontcolor="lime"];



    IDLE -> PROCESSING [label="IEffect.command(START)"];



    PROCESSING -> IDLE [label="IEffect.command(STOP)\nIEffect.command(RESET)"];



    PROCESSING -> DRAINING [label="IEffect.command(STOP)", fontcolor="orange"];



    DRAINING -> IDLE [label="Draining complete\n(IEffect.command(RESET)\nautomatic)"];



    DRAINING -> PROCESSING [label="IEffect.command(START)\n(Interrupt draining)"];



    PROCESSING -> IDLE [label="IEffect.command(STOP) (if draining not required)\nIEffect.command(RESET)"];

    PROCESSING -> DRAINING [label="IEffect.command(STOP) (if draining required)", fontcolor="orange"];

    DRAINING -> IDLE [label="IEffect.command(RESET)\nDraining complete (automatic transition)"];

    DRAINING -> PROCESSING [label="IEffect.command(START) (Interrupt draining)"];

    IDLE -> INIT [label="IEffect.close()"];



    // Self-loops

    INIT -> INIT [label="IEffect.getState\nIEffect.getDescriptor"];



    IDLE -> IDLE [label="IEffect.getParameter\nIEffect.setParameter\nIEffect.getDescriptor\nIEffect.command(RESET)\nIEffect.reopen"];



    PROCESSING -> PROCESSING [label="IEffect.getParameter\nIEffect.setParameter\nIEffect.getDescriptor\nIEffect.reopen"];



    DRAINING -> DRAINING [label="IEffect.getParameter\nIEffect.setParameter\nIEffect.getDescriptor\nIEffect.reopen\nFading"];



    // Global transitions

    subgraph cluster_global_transitions {

        label="Global Transitions (Any State)";

        style=dashed;

        ANY_STATE -> F [label="IFactory.destroyEffect", style="bold"];

        ANY_STATE -> ANY_STATE [label="IEffect.getParameter\nIEffect.setParameter\nIEffect.getDescriptor\nIEffect.getState\nIEffect.reopen", fontsize=10];

    }

}

audio/aidl/default/EffectImpl.cpp

+38 −3
Original line number Diff line number Diff line
@@ -22,7 +22,10 @@ 
#include "effect-impl/EffectTypes.h"

#include "include/effect-impl/EffectTypes.h"



using aidl::android::hardware::audio::effect::CommandId;

using aidl::android::hardware::audio::effect::Descriptor;

using aidl::android::hardware::audio::effect::IEffect;

using aidl::android::hardware::audio::effect::kDestroyAnyStateSupportedVersion;

using aidl::android::hardware::audio::effect::kEventFlagDataMqNotEmpty;

using aidl::android::hardware::audio::effect::kEventFlagNotEmpty;

using aidl::android::hardware::audio::effect::kReopenSupportedVersion;
@@ -31,13 +34,45 @@ using aidl::android::media::audio::common::PcmType; 
using ::android::hardware::EventFlag;



extern "C" binder_exception_t destroyEffect(const std::shared_ptr<IEffect>& instanceSp) {

    State state;

    ndk::ScopedAStatus status = instanceSp->getState(&state);

    if (!status.isOk() || State::INIT != state) {

    if (!instanceSp) {

        LOG(ERROR) << __func__ << " nullptr";

        return EX_ILLEGAL_ARGUMENT;

    }



    Descriptor desc;

    ndk::ScopedAStatus status = instanceSp->getDescriptor(&desc);

    if (!status.isOk()) {

        LOG(ERROR) << __func__ << " instance " << instanceSp.get()

                   << " failed to get descriptor, status: " << status.getDescription();

        return EX_ILLEGAL_STATE;

    }



    State state;

    status = instanceSp->getState(&state);

    if (!status.isOk()) {

        LOG(ERROR) << __func__ << " " << desc.common.name << " instance " << instanceSp.get()

                   << " in state: " << toString(state) << ", status: " << status.getDescription();

        return EX_ILLEGAL_STATE;

    }



    int effectVersion = 0;

    if (!instanceSp->getInterfaceVersion(&effectVersion).isOk()) {

        LOG(WARNING) << __func__ << " " << desc.common.name << " failed to get interface version";

    }



    if (effectVersion < kDestroyAnyStateSupportedVersion) {

        if (State::INIT != state) {

            LOG(ERROR) << __func__ << " " << desc.common.name << " can not destroy instance "

                       << instanceSp.get() << " in state: " << toString(state);

            return EX_ILLEGAL_STATE;

        }

    } else {

        instanceSp->command(CommandId::RESET);

        instanceSp->close();

    }



    LOG(DEBUG) << __func__ << " " << desc.common.name << " instance " << instanceSp.get()

               << " destroyed";

    return EX_NONE;

}

audio/aidl/vts/VtsHalAudioEffectTargetTest.cpp

+24 −12
Original line number Diff line number Diff line
@@ -42,6 +42,7 @@ using aidl::android::hardware::audio::effect::Descriptor; 
using aidl::android::hardware::audio::effect::Flags;

using aidl::android::hardware::audio::effect::IEffect;

using aidl::android::hardware::audio::effect::IFactory;

using aidl::android::hardware::audio::effect::kDestroyAnyStateSupportedVersion;

using aidl::android::hardware::audio::effect::Parameter;

using aidl::android::hardware::audio::effect::State;

using aidl::android::media::audio::common::AudioDeviceDescription;
@@ -316,28 +317,39 @@ TEST_P(AudioEffectTest, CloseProcessingStateEffects) { 
    ASSERT_NO_FATAL_FAILURE(destroy(mFactory, mEffect));

}



// Expect EX_ILLEGAL_STATE if the effect instance is not in a proper state to be destroyed.

// Expect EX_ILLEGAL_STATE if the effect instance is not in a proper state to be destroyed before

// `kDestroyAnyStateSupportedVersion`.

// For any version after `kDestroyAnyStateSupportedVersion`, expect `destroy` to always success.

TEST_P(AudioEffectTest, DestroyOpenEffects) {

    ASSERT_NO_FATAL_FAILURE(create(mFactory, mEffect, mDescriptor));

    ASSERT_NO_FATAL_FAILURE(open(mEffect));

    if (mVersion < kDestroyAnyStateSupportedVersion) {

        ASSERT_NO_FATAL_FAILURE(destroy(mFactory, mEffect, EX_ILLEGAL_STATE));



        // cleanup

        ASSERT_NO_FATAL_FAILURE(close(mEffect));

        ASSERT_NO_FATAL_FAILURE(destroy(mFactory, mEffect));

    } else {

        ASSERT_NO_FATAL_FAILURE(destroy(mFactory, mEffect));

    }

}



// Expect EX_ILLEGAL_STATE if the effect instance is not in a proper state to be destroyed.

// Expect EX_ILLEGAL_STATE if the effect instance is not in a proper state to be destroyed before

// `kDestroyAnyStateSupportedVersion`.

// For any version after `kDestroyAnyStateSupportedVersion`, expect `destroy` to always success.

TEST_P(AudioEffectTest, DestroyProcessingEffects) {

    ASSERT_NO_FATAL_FAILURE(create(mFactory, mEffect, mDescriptor));

    ASSERT_NO_FATAL_FAILURE(open(mEffect));

    ASSERT_NO_FATAL_FAILURE(command(mEffect, CommandId::START));

    ASSERT_NO_FATAL_FAILURE(destroy(mFactory, mEffect, EX_ILLEGAL_STATE));



    if (mVersion < kDestroyAnyStateSupportedVersion) {

        ASSERT_NO_FATAL_FAILURE(destroy(mFactory, mEffect, EX_ILLEGAL_STATE));

        // cleanup

        ASSERT_NO_FATAL_FAILURE(command(mEffect, CommandId::STOP));

        ASSERT_NO_FATAL_FAILURE(close(mEffect));

        ASSERT_NO_FATAL_FAILURE(destroy(mFactory, mEffect));

    } else {

        ASSERT_NO_FATAL_FAILURE(destroy(mFactory, mEffect));

    }

}



TEST_P(AudioEffectTest, NormalSequenceStates) {