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
diff --git a/audio/aidl/android/hardware/audio/effect/IFactory.aidl b/audio/aidl/android/hardware/audio/effect/IFactory.aidl
index b80e6ac..75d4d9e 100644
--- a/audio/aidl/android/hardware/audio/effect/IFactory.aidl
+++ b/audio/aidl/android/hardware/audio/effect/IFactory.aidl
@@ -74,13 +74,18 @@
/**
* 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);
}
diff --git a/audio/aidl/android/hardware/audio/effect/State.aidl b/audio/aidl/android/hardware/audio/effect/State.aidl
index 1b698d7..ecd1bbe 100644
--- a/audio/aidl/android/hardware/audio/effect/State.aidl
+++ b/audio/aidl/android/hardware/audio/effect/State.aidl
@@ -24,6 +24,8 @@
* 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 @@
* - 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 @@
* 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 @@
* - 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,
}
diff --git a/audio/aidl/android/hardware/audio/effect/state.gv b/audio/aidl/android/hardware/audio/effect/state.gv
index 2a8194e..8590296 100644
--- a/audio/aidl/android/hardware/audio/effect/state.gv
+++ b/audio/aidl/android/hardware/audio/effect/state.gv
@@ -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 @@
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];
+ }
}
diff --git a/audio/aidl/default/EffectImpl.cpp b/audio/aidl/default/EffectImpl.cpp
index 7857f53..97f7286 100644
--- a/audio/aidl/default/EffectImpl.cpp
+++ b/audio/aidl/default/EffectImpl.cpp
@@ -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 ::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;
}
diff --git a/audio/aidl/vts/VtsHalAudioEffectTargetTest.cpp b/audio/aidl/vts/VtsHalAudioEffectTargetTest.cpp
index d23bdc9..5b78981 100644
--- a/audio/aidl/vts/VtsHalAudioEffectTargetTest.cpp
+++ b/audio/aidl/vts/VtsHalAudioEffectTargetTest.cpp
@@ -42,6 +42,7 @@
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 @@
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));
- ASSERT_NO_FATAL_FAILURE(destroy(mFactory, mEffect, EX_ILLEGAL_STATE));
-
- // cleanup
- ASSERT_NO_FATAL_FAILURE(close(mEffect));
- ASSERT_NO_FATAL_FAILURE(destroy(mFactory, 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));
- // cleanup
- ASSERT_NO_FATAL_FAILURE(command(mEffect, CommandId::STOP));
- ASSERT_NO_FATAL_FAILURE(close(mEffect));
- ASSERT_NO_FATAL_FAILURE(destroy(mFactory, mEffect));
+ 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) {