Merge changes from topic "upstream-close-fixes"

* changes:
  audio: Add check to IDevice.close for currently opened streams
  audio: Factor out IStream operations into a helper class
  audio: Cleanup VTS tests
  Audio VTS: Fix MMAP tests
  Audio V6 wrapper: IDevice|IStream|IEffect.close releases HAL resource

audio/6.0/IDevice.hal

@@ -280,4 +280,19 @@ 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.
+     *
+     * Note that all streams must be closed by the client before
+     * attempting to close the device they belong to.
+     *
+     * @return retval OK in case the success.
+     *                INVALID_STATE if the device was already closed
+     *                or there are streams currently opened.
+     */
+    @exit
+    close() generates (Result retval);
 };

audio/6.0/IStream.hal

@@ -277,7 +277,7 @@ interface IStream {
      * @return retval OK in case the success.
      *                NOT_SUPPORTED on non mmap mode streams
      *                NOT_INITIALIZED in case of memory allocation error
-     *                INVALID_ARGUMENTS if the requested buffer size is too large
+     *                INVALID_ARGUMENTS if the requested buffer size is invalid
      *                INVALID_STATE if called out of sequence
      * @return info    a MmapBufferInfo struct containing information on the MMMAP buffer created.
      */
@@ -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;
 }
 
@@ -54,10 +53,14 @@ Result Device::analyzeStatus(const char* funcName, int status,
 
 void Device::closeInputStream(audio_stream_in_t* stream) {
     mDevice->close_input_stream(mDevice, stream);
+    LOG_ALWAYS_FATAL_IF(mOpenedStreamsCount == 0, "mOpenedStreamsCount is already 0");
+    --mOpenedStreamsCount;
 }
 
 void Device::closeOutputStream(audio_stream_out_t* stream) {
     mDevice->close_output_stream(mDevice, stream);
+    LOG_ALWAYS_FATAL_IF(mOpenedStreamsCount == 0, "mOpenedStreamsCount is already 0");
+    --mOpenedStreamsCount;
 }
 
 char* Device::halGetParameters(const char* keys) {
@@ -159,6 +162,7 @@ std::tuple<Result, sp<IStreamOut>> Device::openOutputStreamImpl(int32_t ioHandle
     sp<IStreamOut> streamOut;
     if (status == OK) {
         streamOut = new StreamOut(this, halStream);
+        ++mOpenedStreamsCount;
     }
     HidlUtils::audioConfigFromHal(halConfig, suggestedConfig);
     return {analyzeStatus("open_output_stream", status, {EINVAL} /*ignore*/), streamOut};
@@ -185,6 +189,7 @@ std::tuple<Result, sp<IStreamIn>> Device::openInputStreamImpl(
     sp<IStreamIn> streamIn;
     if (status == OK) {
         streamIn = new StreamIn(this, halStream);
+        ++mOpenedStreamsCount;
     }
     HidlUtils::audioConfigFromHal(halConfig, suggestedConfig);
     return {analyzeStatus("open_input_stream", status, {EINVAL} /*ignore*/), streamIn};
@@ -383,6 +388,18 @@ Return<Result> Device::setConnectedState(const DeviceAddress& address, bool conn
 }
 #endif
 
+Result Device::doClose() {
+    if (mIsClosed || mOpenedStreamsCount != 0) 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,15 @@ 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;
+    int mOpenedStreamsCount = 0;
 
     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));
 }
 
 //////////////////////////////////////////////////////////////////////////////
@@ -54,7 +52,6 @@ TEST_P(AudioHidlDeviceTest, GetMicrophonesTest) {
         doc::test(
             "Make sure getMicrophones always succeeds"
             "and getActiveMicrophones always succeeds when recording from these microphones.");
-        AudioIoHandle ioHandle = (AudioIoHandle)AudioHandleConsts::AUDIO_IO_HANDLE_NONE;
         AudioConfig config{};
         config.channelMask = mkEnumBitfield(AudioChannelMask::IN_MONO);
         config.sampleRateHz = 8000;
@@ -67,18 +64,14 @@ TEST_P(AudioHidlDeviceTest, GetMicrophonesTest) {
                 continue;
             }
             sp<IStreamIn> stream;
+            StreamHelper<IStreamIn> helper(stream);
             AudioConfig suggestedConfig{};
-            ASSERT_OK(getDevice()->openInputStream(ioHandle, microphone.deviceAddress, config,
-                                                   flags, initMetadata,
-                                                   returnIn(res, stream, suggestedConfig)));
-            if (res != Result::OK) {
-                ASSERT_TRUE(stream == nullptr);
-                AudioConfig suggestedConfigRetry{};
-                ASSERT_OK(getDevice()->openInputStream(
-                        ioHandle, microphone.deviceAddress, suggestedConfig, flags, initMetadata,
-                        returnIn(res, stream, suggestedConfigRetry)));
-            }
-            ASSERT_OK(res);
+            ASSERT_NO_FATAL_FAILURE(helper.open(
+                    [&](AudioIoHandle handle, AudioConfig config, auto cb) {
+                        return getDevice()->openInputStream(handle, microphone.deviceAddress,
+                                                            config, flags, initMetadata, cb);
+                    },
+                    config, &res, &suggestedConfig));
             hidl_vec<MicrophoneInfo> activeMicrophones;
             Result readRes;
             typedef MessageQueue<IStreamIn::ReadParameters, kSynchronizedReadWrite> CommandMQ;
@@ -112,11 +105,8 @@ TEST_P(AudioHidlDeviceTest, GetMicrophonesTest) {
                 ASSERT_OK(res);
                 ASSERT_NE(0U, activeMicrophones.size());
             }
-            stream->close();
-            // Workaround for b/139329877. Ensures the stream gets closed on the audio hal side.
-            stream.clear();
-            IPCThreadState::self()->flushCommands();
-            usleep(1000);
+            helper.close(true /*clear*/, &res);
+            ASSERT_OK(res);
             if (efGroup) {
                 EventFlag::deleteEventFlag(&efGroup);
             }

audio/core/all-versions/vts/functional/6.0/AudioPrimaryHidlHalTest.cpp

@@ -100,7 +100,8 @@ const std::vector<DeviceConfigParameter>& getOutputDeviceConfigParameters() {
                             special = true;
                         }
                         if ((flags & AUDIO_OUTPUT_FLAG_DIRECT) &&
-                            !(flags & AUDIO_OUTPUT_FLAG_HW_AV_SYNC)) {
+                            !(flags &
+                              (AUDIO_OUTPUT_FLAG_HW_AV_SYNC | AUDIO_OUTPUT_FLAG_MMAP_NOIRQ))) {
                             result.emplace_back(device, config,
                                                 AudioOutputFlag(AUDIO_OUTPUT_FLAG_DIRECT));
                             special = true;
@@ -144,3 +145,49 @@ const std::vector<DeviceConfigParameter>& getInputDeviceConfigParameters() {
     }();
     return parameters;
 }
+
+TEST_P(AudioHidlDeviceTest, CloseDeviceWithOpenedOutputStreams) {
+    doc::test("Verify that a device can't be closed if there are streams opened");
+    DeviceAddress address{.device = AudioDevice::OUT_DEFAULT};
+    AudioConfig config{};
+    auto flags = hidl_bitfield<AudioOutputFlag>(AudioOutputFlag::NONE);
+    SourceMetadata initMetadata = {{{AudioUsage::MEDIA, AudioContentType::MUSIC, 1 /* gain */}}};
+    sp<IStreamOut> stream;
+    StreamHelper<IStreamOut> helper(stream);
+    AudioConfig suggestedConfig{};
+    ASSERT_NO_FATAL_FAILURE(helper.open(
+            [&](AudioIoHandle handle, AudioConfig config, auto cb) {
+                return getDevice()->openOutputStream(handle, address, config, flags, initMetadata,
+                                                     cb);
+            },
+            config, &res, &suggestedConfig));
+    ASSERT_RESULT(Result::INVALID_STATE, getDevice()->close());
+    ASSERT_NO_FATAL_FAILURE(helper.close(true /*clear*/, &res));
+    ASSERT_OK(getDevice()->close());
+    ASSERT_TRUE(resetDevice());
+}
+
+TEST_P(AudioHidlDeviceTest, CloseDeviceWithOpenedInputStreams) {
+    doc::test("Verify that a device can't be closed if there are streams opened");
+    auto module = getCachedPolicyConfig().getModuleFromName(getDeviceName());
+    if (module->getInputProfiles().empty()) {
+        GTEST_SKIP() << "Device doesn't have input profiles";
+    }
+    DeviceAddress address{.device = AudioDevice::IN_DEFAULT};
+    AudioConfig config{};
+    auto flags = hidl_bitfield<AudioInputFlag>(AudioInputFlag::NONE);
+    SinkMetadata initMetadata = {{{.source = AudioSource::MIC, .gain = 1}}};
+    sp<IStreamIn> stream;
+    StreamHelper<IStreamIn> helper(stream);
+    AudioConfig suggestedConfig{};
+    ASSERT_NO_FATAL_FAILURE(helper.open(
+            [&](AudioIoHandle handle, AudioConfig config, auto cb) {
+                return getDevice()->openInputStream(handle, address, config, flags, initMetadata,
+                                                    cb);
+            },
+            config, &res, &suggestedConfig));
+    ASSERT_RESULT(Result::INVALID_STATE, getDevice()->close());
+    ASSERT_NO_FATAL_FAILURE(helper.close(true /*clear*/, &res));
+    ASSERT_OK(getDevice()->close());
+    ASSERT_TRUE(resetDevice());
+}

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

@@ -717,12 +717,6 @@ INSTANTIATE_TEST_CASE_P(
                 ::testing::ValuesIn(ConfigHelper::getRequiredSupportCaptureAudioConfig()),
                 ::testing::Values(AudioInputFlag::NONE)),
         &DeviceConfigParameterToString);
-INSTANTIATE_TEST_CASE_P(
-        SupportedInputBufferSize, RequiredInputBufferSizeTest,
-        ::testing::Combine(::testing::ValuesIn(getDeviceParameters()),
-                           ::testing::ValuesIn(ConfigHelper::getSupportedCaptureAudioConfig()),
-                           ::testing::Values(AudioInputFlag::NONE)),
-        &DeviceConfigParameterToString);
 INSTANTIATE_TEST_CASE_P(
         RecommendedCaptureAudioConfigSupport, OptionalInputBufferSizeTest,
         ::testing::Combine(
@@ -815,60 +809,84 @@ TEST_P(AudioHidlDeviceTest, DebugDumpInvalidArguments) {
 ////////////////////////// open{Output,Input}Stream //////////////////////////
 //////////////////////////////////////////////////////////////////////////////
 
+// This class is also used by some device tests.
 template <class Stream>
-class OpenStreamTest : public AudioHidlTestWithDeviceConfigParameter {
-  protected:
+class StreamHelper {
+  public:
+    // StreamHelper doesn't own the stream, this is for simpler stream lifetime management.
+    explicit StreamHelper(sp<Stream>& stream) : mStream(stream) {}
     template <class Open>
-    void testOpen(Open openStream, const AudioConfig& config) {
+    void open(Open openStream, const AudioConfig& config, Result* res,
+              AudioConfig* suggestedConfigPtr) {
         // FIXME: Open a stream without an IOHandle
         //        This is not required to be accepted by hal implementations
         AudioIoHandle ioHandle = (AudioIoHandle)AudioHandleConsts::AUDIO_IO_HANDLE_NONE;
         AudioConfig suggestedConfig{};
-        ASSERT_OK(openStream(ioHandle, config, returnIn(res, stream, suggestedConfig)));
-
-        // TODO: only allow failure for RecommendedPlaybackAudioConfig
-        switch (res) {
+        bool retryWithSuggestedConfig = true;
+        if (suggestedConfigPtr == nullptr) {
+            suggestedConfigPtr = &suggestedConfig;
+            retryWithSuggestedConfig = false;
+        }
+        ASSERT_OK(openStream(ioHandle, config, returnIn(*res, mStream, *suggestedConfigPtr)));
+        switch (*res) {
             case Result::OK:
-                ASSERT_TRUE(stream != nullptr);
-                audioConfig = config;
+                ASSERT_TRUE(mStream != nullptr);
+                *suggestedConfigPtr = config;
                 break;
             case Result::INVALID_ARGUMENTS:
-                ASSERT_TRUE(stream == nullptr);
-                AudioConfig suggestedConfigRetry;
-                // Could not open stream with config, try again with the
-                // suggested one
-                ASSERT_OK(openStream(ioHandle, suggestedConfig,
-                                     returnIn(res, stream, suggestedConfigRetry)));
-                // This time it must succeed
-                ASSERT_OK(res);
-                ASSERT_TRUE(stream != nullptr);
-                audioConfig = suggestedConfig;
+                ASSERT_TRUE(mStream == nullptr);
+                if (retryWithSuggestedConfig) {
+                    AudioConfig suggestedConfigRetry;
+                    ASSERT_OK(openStream(ioHandle, *suggestedConfigPtr,
+                                         returnIn(*res, mStream, suggestedConfigRetry)));
+                    ASSERT_OK(*res);
+                    ASSERT_TRUE(mStream != nullptr);
+                }
                 break;
             default:
-                FAIL() << "Invalid return status: " << ::testing::PrintToString(res);
+                FAIL() << "Invalid return status: " << ::testing::PrintToString(*res);
         }
+    }
+    void close(bool clear, Result* res) {
+        auto ret = mStream->close();
+        EXPECT_TRUE(ret.isOk());
+        *res = ret;
+        if (clear) {
+            mStream.clear();
+#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
+            //        the remote device has the time to be destroyed.
+            //        flushCommand makes sure all local command are sent, thus should reduce
+            //        the latency between local and remote destruction.
+            IPCThreadState::self()->flushCommands();
+            usleep(100 * 1000);
+#endif
+        }
+    }
+
+  private:
+    sp<Stream>& mStream;
+};
+
+template <class Stream>
+class OpenStreamTest : public AudioHidlTestWithDeviceConfigParameter {
+  protected:
+    OpenStreamTest() : AudioHidlTestWithDeviceConfigParameter(), helper(stream) {}
+    template <class Open>
+    void testOpen(Open openStream, const AudioConfig& config) {
+        // TODO: only allow failure for RecommendedPlaybackAudioConfig
+        ASSERT_NO_FATAL_FAILURE(helper.open(openStream, config, &res, &audioConfig));
         open = true;
     }
 
-    Return<Result> closeStream() {
+    Result closeStream(bool clear = true) {
         open = false;
-        auto res = stream->close();
-        stream.clear();
-        waitForStreamDestruction();
+        helper.close(clear, &res);
         return res;
     }
 
-    static void waitForStreamDestruction() {
-        // 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
-        //        the remote device has the time to be destroyed.
-        //        flushCommand makes sure all local command are sent, thus should reduce
-        //        the latency between local and remote destruction.
-        IPCThreadState::self()->flushCommands();
-        usleep(100 * 1000);
-    }
-
   private:
     void TearDown() override {
         if (open) {
@@ -881,6 +899,7 @@ class OpenStreamTest : public AudioHidlTestWithDeviceConfigParameter {
     AudioConfig audioConfig;
     DeviceAddress address = {};
     sp<Stream> stream;
+    StreamHelper<Stream> helper;
     bool open = false;
 };
 
@@ -928,12 +947,6 @@ INSTANTIATE_TEST_CASE_P(
                 ::testing::ValuesIn(ConfigHelper::getRequiredSupportPlaybackAudioConfig()),
                 ::testing::Values(AudioOutputFlag::NONE)),
         &DeviceConfigParameterToString);
-INSTANTIATE_TEST_CASE_P(
-        SupportedOutputStreamConfig, OutputStreamTest,
-        ::testing::Combine(::testing::ValuesIn(getDeviceParameters()),
-                           ::testing::ValuesIn(ConfigHelper::getSupportedPlaybackAudioConfig()),
-                           ::testing::Values(AudioOutputFlag::NONE)),
-        &DeviceConfigParameterToString);
 INSTANTIATE_TEST_CASE_P(
         RecommendedOutputStreamConfigSupport, OutputStreamTest,
         ::testing::Combine(
@@ -944,7 +957,7 @@ INSTANTIATE_TEST_CASE_P(
 #elif MAJOR_VERSION >= 6
 // For V6 and above test according to the audio policy manager configuration.
 // This is more correct as CDD is written from the apps perspective.
-// Audio system provides necessary format conversions for the missing configurations.
+// Audio system provides necessary format conversions for missing configurations.
 INSTANTIATE_TEST_CASE_P(DeclaredOutputStreamConfigSupport, OutputStreamTest,
                         ::testing::ValuesIn(getOutputDeviceConfigParameters()),
                         &DeviceConfigParameterToString);
@@ -989,12 +1002,6 @@ INSTANTIATE_TEST_CASE_P(
                 ::testing::ValuesIn(ConfigHelper::getRequiredSupportCaptureAudioConfig()),
                 ::testing::Values(AudioInputFlag::NONE)),
         &DeviceConfigParameterToString);
-INSTANTIATE_TEST_CASE_P(
-        SupportedInputStreamConfig, InputStreamTest,
-        ::testing::Combine(::testing::ValuesIn(getDeviceParameters()),
-                           ::testing::ValuesIn(ConfigHelper::getSupportedCaptureAudioConfig()),
-                           ::testing::Values(AudioInputFlag::NONE)),
-        &DeviceConfigParameterToString);
 INSTANTIATE_TEST_CASE_P(
         RecommendedInputStreamConfigSupport, InputStreamTest,
         ::testing::Combine(
@@ -1005,7 +1012,7 @@ INSTANTIATE_TEST_CASE_P(
 #elif MAJOR_VERSION >= 6
 // For V6 and above test according to the audio policy manager configuration.
 // This is more correct as CDD is written from the apps perspective.
-// Audio system provides necessary format conversions for the missing configurations.
+// Audio system provides necessary format conversions for missing configurations.
 INSTANTIATE_TEST_CASE_P(DeclaredInputStreamConfigSupport, InputStreamTest,
                         ::testing::ValuesIn(getInputDeviceConfigParameters()),
                         &DeviceConfigParameterToString);
@@ -1194,26 +1201,21 @@ TEST_IO_STREAM(getMmapPositionNoMmap, "Get a stream Mmap position before mapping
 TEST_IO_STREAM(close, "Make sure a stream can be closed", ASSERT_OK(closeStream()))
 // clang-format off
 TEST_IO_STREAM(closeTwice, "Make sure a stream can not be closed twice",
-        auto streamCopy = stream;
-        ASSERT_OK(closeStream());
-        ASSERT_RESULT(Result::INVALID_STATE, streamCopy->close());
-        streamCopy.clear();
-        waitForStreamDestruction())
+        ASSERT_OK(closeStream(false /*clear*/));
+        ASSERT_EQ(Result::INVALID_STATE, closeStream()))
 // clang-format on
 
-static void testCreateTooBigMmapBuffer(IStream* stream) {
-    MmapBufferInfo info;
-    Result res;
-    // Assume that int max is a value too big to be allocated
-    // This is true currently with a 32bit media server, but might not when it
-    // will run in 64 bit
-    auto minSizeFrames = std::numeric_limits<int32_t>::max();
-    ASSERT_OK(stream->createMmapBuffer(minSizeFrames, returnIn(res, info)));
-    ASSERT_RESULT(invalidArgsOrNotSupported, res);
+static void testMmapBufferOfInvalidSize(IStream* stream) {
+    for (int32_t value : {-1, 0, std::numeric_limits<int32_t>::max()}) {
+        MmapBufferInfo info;
+        Result res;
+        EXPECT_OK(stream->createMmapBuffer(value, returnIn(res, info)));
+        EXPECT_RESULT(invalidArgsOrNotSupported, res) << "value=" << value;
+    }
 }
 
-TEST_IO_STREAM(CreateTooBigMmapBuffer, "Create mmap buffer too big should fail",
-               testCreateTooBigMmapBuffer(stream.get()))
+TEST_IO_STREAM(CreateTooBigMmapBuffer, "Create mmap buffer of invalid size must fail",
+               testMmapBufferOfInvalidSize(stream.get()))
 
 static void testGetMmapPositionOfNonMmapedStream(IStream* stream) {
     Result res;

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

@@ -57,12 +57,6 @@ struct ConfigHelper {
                                   {24000, 48000}, {AudioFormat::PCM_16_BIT});
     }
 
-    static const vector<AudioConfig> getSupportedPlaybackAudioConfig() {
-        // TODO: retrieve audio config supported by the platform
-        // as declared in the policy configuration
-        return {};
-    }
-
     static const vector<AudioConfig> getRequiredSupportCaptureAudioConfig() {
         if (!primaryHasMic()) return {};
         return combineAudioConfig({AudioChannelMask::IN_MONO}, {8000, 11025, 16000, 44100},
@@ -73,11 +67,6 @@ struct ConfigHelper {
         return combineAudioConfig({AudioChannelMask::IN_STEREO}, {22050, 48000},
                                   {AudioFormat::PCM_16_BIT});
     }
-    static const vector<AudioConfig> getSupportedCaptureAudioConfig() {
-        // TODO: retrieve audio config supported by the platform
-        // as declared in the policy configuration
-        return {};
-    }
 
     static vector<AudioConfig> combineAudioConfig(vector<audio_channel_mask_t> channelMasks,
                                                   vector<uint32_t> sampleRates,

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;