Audio V6 wrapper: IDevice|IStream|IEffect.close releases HAL resource

Fixed behavior of IStream|IEffect.close to release the underlying
HAL resource synchronously. This is to avoid adding artificial
delays in VTS that become totally unpractical in V6.

Added clarification about expected client behavior for
IStream|IEffect.close w.r.t. audio data transfer.

Added IDevice.close method which releases HAL device resource.

Updated VTS tests to remove delays in V6.

Bug: 114451103
Bug: 141989952
Test: atest VtsHalAudioV6_0TargetTest
Change-Id: I439f0f923c091af2ab234d15ca847cfade341f25

16 files changed, 120 insertions, 45 deletions

diff --git a/audio/6.0/IDevice.hal b/audio/6.0/IDevice.hal
index e885fe2267..520e776c21 100644
--- a/audio/6.0/IDevice.hal
+++ b/audio/6.0/IDevice.hal
@@ -280,4 +280,15 @@ interface IDevice {
      */
     setConnectedState(DeviceAddress address, bool connected)
             generates (Result retval);
+
+    /**
+     * Called by the framework to deinitialize the device and free up
+     * all currently allocated resources. It is recommended to close
+     * the device on the client side as soon as it is becomes unused.
+     *
+     * @return retval OK in case the success.
+     *                INVALID_STATE if the device was already closed.
+     */
+    @exit
+    close() generates (Result retval);
 };
diff --git a/audio/6.0/IStream.hal b/audio/6.0/IStream.hal
index 451e1162bf..1f62017cae 100644
--- a/audio/6.0/IStream.hal
+++ b/audio/6.0/IStream.hal
@@ -300,13 +300,17 @@ interface IStream {
 
     /**
      * Called by the framework to deinitialize the stream and free up
-     * all the currently allocated resources. It is recommended to close
+     * all currently allocated resources. It is recommended to close
      * the stream on the client side as soon as it is becomes unused.
      *
+     * The client must ensure that this function is not called while
+     * audio data is being transferred through the stream's message queues.
+     *
      * @return retval OK in case the success.
      *                NOT_SUPPORTED if called on IStream instead of input or
      *                              output stream interface.
      *                INVALID_STATE if the stream was already closed.
      */
+    @exit
     close() generates (Result retval);
 };
diff --git a/audio/core/all-versions/default/Device.cpp b/audio/core/all-versions/default/Device.cpp
index 1a9df217e1..5ea4c8df59 100644
--- a/audio/core/all-versions/default/Device.cpp
+++ b/audio/core/all-versions/default/Device.cpp
@@ -39,11 +39,10 @@ namespace implementation {
 
 using ::android::hardware::audio::common::CPP_VERSION::implementation::HidlUtils;
 
-Device::Device(audio_hw_device_t* device) : mDevice(device) {}
+Device::Device(audio_hw_device_t* device) : mIsClosed(false), mDevice(device) {}
 
 Device::~Device() {
-    int status = audio_hw_device_close(mDevice);
-    ALOGW_IF(status, "Error closing audio hw device %p: %s", mDevice, strerror(-status));
+    (void)doClose();
     mDevice = nullptr;
 }
 
@@ -383,6 +382,18 @@ Return<Result> Device::setConnectedState(const DeviceAddress& address, bool conn
 }
 #endif
 
+Result Device::doClose() {
+    if (mIsClosed) return Result::INVALID_STATE;
+    mIsClosed = true;
+    return analyzeStatus("close", audio_hw_device_close(mDevice));
+}
+
+#if MAJOR_VERSION >= 6
+Return<Result> Device::close() {
+    return doClose();
+}
+#endif
+
 }  // namespace implementation
 }  // namespace CPP_VERSION
 }  // namespace audio
diff --git a/audio/core/all-versions/default/PrimaryDevice.cpp b/audio/core/all-versions/default/PrimaryDevice.cpp
index 99590b0bdc..3cf09320aa 100644
--- a/audio/core/all-versions/default/PrimaryDevice.cpp
+++ b/audio/core/all-versions/default/PrimaryDevice.cpp
@@ -31,7 +31,11 @@ namespace implementation {
 
 PrimaryDevice::PrimaryDevice(audio_hw_device_t* device) : mDevice(new Device(device)) {}
 
-PrimaryDevice::~PrimaryDevice() {}
+PrimaryDevice::~PrimaryDevice() {
+    // Do not call mDevice->close here. If there are any unclosed streams,
+    // they only hold IDevice instance, not IPrimaryDevice, thus IPrimaryDevice
+    // "part" of a device can be destroyed before the streams.
+}
 
 // Methods from ::android::hardware::audio::CPP_VERSION::IDevice follow.
 Return<Result> PrimaryDevice::initCheck() {
@@ -160,6 +164,11 @@ Return<Result> PrimaryDevice::setConnectedState(const DeviceAddress& address, bo
     return mDevice->setConnectedState(address, connected);
 }
 #endif
+#if MAJOR_VERSION >= 6
+Return<Result> PrimaryDevice::close() {
+    return mDevice->close();
+}
+#endif
 
 // Methods from ::android::hardware::audio::CPP_VERSION::IPrimaryDevice follow.
 Return<Result> PrimaryDevice::setVoiceVolume(float volume) {
diff --git a/audio/core/all-versions/default/StreamIn.cpp b/audio/core/all-versions/default/StreamIn.cpp
index d316f83617..f1152ca542 100644
--- a/audio/core/all-versions/default/StreamIn.cpp
+++ b/audio/core/all-versions/default/StreamIn.cpp
@@ -139,8 +139,7 @@ bool ReadThread::threadLoop() {
 }  // namespace
 
 StreamIn::StreamIn(const sp<Device>& device, audio_stream_in_t* stream)
-    : mIsClosed(false),
-      mDevice(device),
+    : mDevice(device),
       mStream(stream),
       mStreamCommon(new Stream(&stream->common)),
       mStreamMmap(new StreamMmap<audio_stream_in_t>(stream)),
@@ -159,7 +158,9 @@ StreamIn::~StreamIn() {
         status_t status = EventFlag::deleteEventFlag(&mEfGroup);
         ALOGE_IF(status, "read MQ event flag deletion error: %s", strerror(-status));
     }
+#if MAJOR_VERSION <= 5
     mDevice->closeInputStream(mStream);
+#endif
     mStream = nullptr;
 }
 
@@ -303,14 +304,16 @@ Return<void> StreamIn::getMmapPosition(getMmapPosition_cb _hidl_cb) {
 }
 
 Return<Result> StreamIn::close() {
-    if (mIsClosed) return Result::INVALID_STATE;
-    mIsClosed = true;
-    if (mReadThread.get()) {
-        mStopReadThread.store(true, std::memory_order_release);
+    if (mStopReadThread.load(std::memory_order_relaxed)) {  // only this thread writes
+        return Result::INVALID_STATE;
     }
+    mStopReadThread.store(true, std::memory_order_release);
     if (mEfGroup) {
         mEfGroup->wake(static_cast<uint32_t>(MessageQueueFlagBits::NOT_FULL));
     }
+#if MAJOR_VERSION >= 6
+    mDevice->closeInputStream(mStream);
+#endif
     return Result::OK;
 }
 
diff --git a/audio/core/all-versions/default/StreamOut.cpp b/audio/core/all-versions/default/StreamOut.cpp
index 82cc408e99..396d354179 100644
--- a/audio/core/all-versions/default/StreamOut.cpp
+++ b/audio/core/all-versions/default/StreamOut.cpp
@@ -138,8 +138,7 @@ bool WriteThread::threadLoop() {
 }  // namespace
 
 StreamOut::StreamOut(const sp<Device>& device, audio_stream_out_t* stream)
-    : mIsClosed(false),
-      mDevice(device),
+    : mDevice(device),
       mStream(stream),
       mStreamCommon(new Stream(&stream->common)),
       mStreamMmap(new StreamMmap<audio_stream_out_t>(stream)),
@@ -148,7 +147,7 @@ StreamOut::StreamOut(const sp<Device>& device, audio_stream_out_t* stream)
 
 StreamOut::~StreamOut() {
     ATRACE_CALL();
-    close();
+    (void)close();
     if (mWriteThread.get()) {
         ATRACE_NAME("mWriteThread->join");
         status_t status = mWriteThread->join();
@@ -159,10 +158,12 @@ StreamOut::~StreamOut() {
         ALOGE_IF(status, "write MQ event flag deletion error: %s", strerror(-status));
     }
     mCallback.clear();
+#if MAJOR_VERSION <= 5
     mDevice->closeOutputStream(mStream);
     // Closing the output stream in the HAL waits for the callback to finish,
     // and joins the callback thread. Thus is it guaranteed that the callback
     // thread will not be accessing our object anymore.
+#endif
     mStream = nullptr;
 }
 
@@ -291,14 +292,16 @@ Return<Result> StreamOut::setParameters(const hidl_vec<ParameterValue>& context,
 #endif
 
 Return<Result> StreamOut::close() {
-    if (mIsClosed) return Result::INVALID_STATE;
-    mIsClosed = true;
-    if (mWriteThread.get()) {
-        mStopWriteThread.store(true, std::memory_order_release);
+    if (mStopWriteThread.load(std::memory_order_relaxed)) {  // only this thread writes
+        return Result::INVALID_STATE;
     }
+    mStopWriteThread.store(true, std::memory_order_release);
     if (mEfGroup) {
         mEfGroup->wake(static_cast<uint32_t>(MessageQueueFlagBits::NOT_EMPTY));
     }
+#if MAJOR_VERSION >= 6
+    mDevice->closeOutputStream(mStream);
+#endif
     return Result::OK;
 }
 
diff --git a/audio/core/all-versions/default/include/core/default/Device.h b/audio/core/all-versions/default/include/core/default/Device.h
index e64f00f205..dc53a3961a 100644
--- a/audio/core/all-versions/default/include/core/default/Device.h
+++ b/audio/core/all-versions/default/include/core/default/Device.h
@@ -114,6 +114,9 @@ struct Device : public IDevice, public ParametersUtil {
     Return<void> getMicrophones(getMicrophones_cb _hidl_cb) override;
     Return<Result> setConnectedState(const DeviceAddress& address, bool connected) override;
 #endif
+#if MAJOR_VERSION >= 6
+    Return<Result> close() override;
+#endif
 
     Return<void> debug(const hidl_handle& fd, const hidl_vec<hidl_string>& options) override;
 
@@ -124,11 +127,14 @@ struct Device : public IDevice, public ParametersUtil {
     void closeOutputStream(audio_stream_out_t* stream);
     audio_hw_device_t* device() const { return mDevice; }
 
-   private:
+  private:
+    bool mIsClosed;
     audio_hw_device_t* mDevice;
 
     virtual ~Device();
 
+    Result doClose();
+
     // Methods from ParametersUtil.
     char* halGetParameters(const char* keys) override;
     int halSetParameters(const char* keysAndValues) override;
diff --git a/audio/core/all-versions/default/include/core/default/PrimaryDevice.h b/audio/core/all-versions/default/include/core/default/PrimaryDevice.h
index 9d69cb0994..f5f38482ee 100644
--- a/audio/core/all-versions/default/include/core/default/PrimaryDevice.h
+++ b/audio/core/all-versions/default/include/core/default/PrimaryDevice.h
@@ -96,6 +96,9 @@ struct PrimaryDevice : public IPrimaryDevice {
     Return<void> getMicrophones(getMicrophones_cb _hidl_cb) override;
     Return<Result> setConnectedState(const DeviceAddress& address, bool connected) override;
 #endif
+#if MAJOR_VERSION >= 6
+    Return<Result> close() override;
+#endif
 
     Return<void> debug(const hidl_handle& fd, const hidl_vec<hidl_string>& options) override;
 
diff --git a/audio/core/all-versions/default/include/core/default/StreamIn.h b/audio/core/all-versions/default/include/core/default/StreamIn.h
index 6209b8f996..24f994406c 100644
--- a/audio/core/all-versions/default/include/core/default/StreamIn.h
+++ b/audio/core/all-versions/default/include/core/default/StreamIn.h
@@ -120,7 +120,6 @@ struct StreamIn : public IStreamIn {
                                          uint64_t* time);
 
    private:
-    bool mIsClosed;
     const sp<Device> mDevice;
     audio_stream_in_t* mStream;
     const sp<Stream> mStreamCommon;
diff --git a/audio/core/all-versions/default/include/core/default/StreamOut.h b/audio/core/all-versions/default/include/core/default/StreamOut.h
index b0980053e4..6334785a03 100644
--- a/audio/core/all-versions/default/include/core/default/StreamOut.h
+++ b/audio/core/all-versions/default/include/core/default/StreamOut.h
@@ -126,7 +126,6 @@ struct StreamOut : public IStreamOut {
                                               TimeSpec* timeStamp);
 
    private:
-    bool mIsClosed;
     const sp<Device> mDevice;
     audio_stream_out_t* mStream;
     const sp<Stream> mStreamCommon;
diff --git a/audio/core/all-versions/vts/functional/4.0/AudioPrimaryHidlHalTest.cpp b/audio/core/all-versions/vts/functional/4.0/AudioPrimaryHidlHalTest.cpp
index e267a5ea72..81f963d34d 100644
--- a/audio/core/all-versions/vts/functional/4.0/AudioPrimaryHidlHalTest.cpp
+++ b/audio/core/all-versions/vts/functional/4.0/AudioPrimaryHidlHalTest.cpp
@@ -22,18 +22,16 @@ TEST_P(AudioHidlTest, OpenPrimaryDeviceUsingGetDevice) {
         GTEST_SKIP() << "No primary device on this factory";  // returns
     }
 
-    struct WaitExecutor {
-        ~WaitExecutor() { DeviceManager::waitForInstanceDestruction(); }
-    } waitExecutor;  // Make sure we wait for the device destruction on exiting from the test.
-    Result result;
-    sp<IDevice> baseDevice;
-    ASSERT_OK(getDevicesFactory()->openDevice("primary", returnIn(result, baseDevice)));
-    ASSERT_OK(result);
-    ASSERT_TRUE(baseDevice != nullptr);
-
-    Return<sp<IPrimaryDevice>> primaryDevice = IPrimaryDevice::castFrom(baseDevice);
-    ASSERT_TRUE(primaryDevice.isOk());
-    ASSERT_TRUE(sp<IPrimaryDevice>(primaryDevice) != nullptr);
+    {  // Scope for device SPs
+        sp<IDevice> baseDevice =
+                DeviceManager::getInstance().get(getFactoryName(), DeviceManager::kPrimaryDevice);
+        ASSERT_TRUE(baseDevice != nullptr);
+        Return<sp<IPrimaryDevice>> primaryDevice = IPrimaryDevice::castFrom(baseDevice);
+        EXPECT_TRUE(primaryDevice.isOk());
+        EXPECT_TRUE(sp<IPrimaryDevice>(primaryDevice) != nullptr);
+    }
+    EXPECT_TRUE(
+            DeviceManager::getInstance().reset(getFactoryName(), DeviceManager::kPrimaryDevice));
 }
 
 //////////////////////////////////////////////////////////////////////////////
@@ -113,10 +111,12 @@ TEST_P(AudioHidlDeviceTest, GetMicrophonesTest) {
                 ASSERT_NE(0U, activeMicrophones.size());
             }
             stream->close();
+#if MAJOR_VERSION <= 5
             // Workaround for b/139329877. Ensures the stream gets closed on the audio hal side.
             stream.clear();
             IPCThreadState::self()->flushCommands();
             usleep(1000);
+#endif
             if (efGroup) {
                 EventFlag::deleteEventFlag(&efGroup);
             }
diff --git a/audio/core/all-versions/vts/functional/AudioPrimaryHidlHalTest.h b/audio/core/all-versions/vts/functional/AudioPrimaryHidlHalTest.h
index 468f9b2da6..52917fd10f 100644
--- a/audio/core/all-versions/vts/functional/AudioPrimaryHidlHalTest.h
+++ b/audio/core/all-versions/vts/functional/AudioPrimaryHidlHalTest.h
@@ -860,6 +860,7 @@ class OpenStreamTest : public AudioHidlTestWithDeviceConfigParameter {
     }
 
     static void waitForStreamDestruction() {
+#if MAJOR_VERSION <= 5
         // FIXME: there is no way to know when the remote IStream is being destroyed
         //        Binder does not support testing if an object is alive, thus
         //        wait for 100ms to let the binder destruction propagates and
@@ -868,6 +869,7 @@ class OpenStreamTest : public AudioHidlTestWithDeviceConfigParameter {
         //        the latency between local and remote destruction.
         IPCThreadState::self()->flushCommands();
         usleep(100 * 1000);
+#endif
     }
 
   private:
diff --git a/audio/core/all-versions/vts/functional/DeviceManager.h b/audio/core/all-versions/vts/functional/DeviceManager.h
index b6e2db0685..d849f85eba 100644
--- a/audio/core/all-versions/vts/functional/DeviceManager.h
+++ b/audio/core/all-versions/vts/functional/DeviceManager.h
@@ -22,25 +22,33 @@
 template <class Derived, class Key, class Interface>
 class InterfaceManager {
   public:
+    sp<Interface> getExisting(const Key& name) {
+        auto existing = instances.find(name);
+        return existing != instances.end() ? existing->second : sp<Interface>();
+    }
+
     sp<Interface> get(const Key& name) {
         auto existing = instances.find(name);
         if (existing != instances.end()) return existing->second;
         auto [inserted, _] = instances.emplace(name, Derived::createInterfaceInstance(name));
         if (inserted->second) {
-            environment->registerTearDown([name]() { (void)Derived::getInstance().reset(name); });
+            environment->registerTearDown(
+                    [name]() { (void)Derived::getInstance().reset(name, false); });
         }
         return inserted->second;
     }
 
     // The test must check that reset was successful. Reset failure means that the test code
     // is holding a strong reference to the device.
-    bool reset(const Key& name) __attribute__((warn_unused_result)) {
+    bool reset(const Key& name, bool waitForDestruction) __attribute__((warn_unused_result)) {
         auto iter = instances.find(name);
         if (iter == instances.end()) return true;
         ::android::wp<Interface> weak = iter->second;
         instances.erase(iter);
         if (weak.promote() != nullptr) return false;
-        waitForInstanceDestruction();
+        if (waitForDestruction) {
+            waitForInstanceDestruction();
+        }
         return true;
     }
 
@@ -100,7 +108,15 @@ class DeviceManager : public InterfaceManager<DeviceManager, FactoryAndDevice, I
     }
     bool reset(const std::string& factoryName, const std::string& name)
             __attribute__((warn_unused_result)) {
-        return InterfaceManager::reset(std::make_tuple(factoryName, name));
+#if MAJOR_VERSION <= 5
+        return InterfaceManager::reset(std::make_tuple(factoryName, name), true);
+#elif MAJOR_VERSION >= 6
+        {
+            sp<IDevice> device = getExisting(std::make_tuple(factoryName, name));
+            if (device != nullptr) device->close();
+        }
+        return InterfaceManager::reset(std::make_tuple(factoryName, name), false);
+#endif
     }
     bool resetPrimary(const std::string& factoryName) __attribute__((warn_unused_result)) {
         return reset(factoryName, kPrimaryDevice);
diff --git a/audio/effect/6.0/IEffect.hal b/audio/effect/6.0/IEffect.hal
index b35afee260..f4c50a20aa 100644
--- a/audio/effect/6.0/IEffect.hal
+++ b/audio/effect/6.0/IEffect.hal
@@ -407,9 +407,12 @@ interface IEffect {
 
     /**
      * Called by the framework to deinitialize the effect and free up
-     * all the currently allocated resources. It is recommended to close
+     * all currently allocated resources. It is recommended to close
      * the effect on the client side as soon as it is becomes unused.
      *
+     * The client must ensure that this function is not called while
+     * audio data is being transferred through the effect's message queues.
+     *
      * @return retval OK in case the success.
      *                INVALID_STATE if the effect was already closed.
      */
diff --git a/audio/effect/all-versions/default/Effect.cpp b/audio/effect/all-versions/default/Effect.cpp
index 3c0d8788ab..0afa779f03 100644
--- a/audio/effect/all-versions/default/Effect.cpp
+++ b/audio/effect/all-versions/default/Effect.cpp
@@ -138,11 +138,11 @@ const char* Effect::sContextCallToCommand = "error";
 const char* Effect::sContextCallFunction = sContextCallToCommand;
 
 Effect::Effect(effect_handle_t handle)
-    : mIsClosed(false), mHandle(handle), mEfGroup(nullptr), mStopProcessThread(false) {}
+    : mHandle(handle), mEfGroup(nullptr), mStopProcessThread(false) {}
 
 Effect::~Effect() {
     ATRACE_CALL();
-    close();
+    (void)close();
     if (mProcessThread.get()) {
         ATRACE_NAME("mProcessThread->join");
         status_t status = mProcessThread->join();
@@ -154,8 +154,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));
+#endif
     EffectMap::getInstance().remove(mHandle);
     mHandle = 0;
 }
@@ -699,15 +701,20 @@ Return<Result> Effect::setCurrentConfigForFeature(uint32_t featureId,
 }
 
 Return<Result> Effect::close() {
-    if (mIsClosed) return Result::INVALID_STATE;
-    mIsClosed = true;
-    if (mProcessThread.get()) {
-        mStopProcessThread.store(true, std::memory_order_release);
+    if (mStopProcessThread.load(std::memory_order_relaxed)) {  // only this thread modifies
+        return Result::INVALID_STATE;
     }
+    mStopProcessThread.store(true, std::memory_order_release);
     if (mEfGroup) {
         mEfGroup->wake(static_cast<uint32_t>(MessageQueueFlagBits::REQUEST_QUIT));
     }
+#if MAJOR_VERSION <= 5
     return Result::OK;
+#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.
+    return analyzeStatus("EffectRelease", "", sContextCallFunction, EffectRelease(mHandle));
+#endif
 }
 
 Return<void> Effect::debug(const hidl_handle& fd, const hidl_vec<hidl_string>& /* options */) {
diff --git a/audio/effect/all-versions/default/Effect.h b/audio/effect/all-versions/default/Effect.h
index 3d99a0e42f..181e54241a 100644
--- a/audio/effect/all-versions/default/Effect.h
+++ b/audio/effect/all-versions/default/Effect.h
@@ -170,7 +170,6 @@ struct Effect : public IEffect {
     static const char* sContextCallToCommand;
     static const char* sContextCallFunction;
 
-    bool mIsClosed;
     effect_handle_t mHandle;
     sp<AudioBufferWrapper> mInBuffer;
     sp<AudioBufferWrapper> mOutBuffer;