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
Merged-In: I439f0f923c091af2ab234d15ca847cfade341f25

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);
 };

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);
 };

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

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) {

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;
 }
 

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;
 }
 

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;

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;
 

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;

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;

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);
             }

audio/core/all-versions/vts/functional/AudioPrimaryHidlHalTest.h

@@ -859,6 +859,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
@@ -867,6 +868,7 @@ class OpenStreamTest : public AudioHidlTestWithDeviceConfigParameter {
         //        the latency between local and remote destruction.
         IPCThreadState::self()->flushCommands();
         usleep(100 * 1000);
+#endif
     }
 
   private:

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);

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.
      */

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 */) {

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;