diff --git a/audio/6.0/IDevice.hal b/audio/6.0/IDevice.hal index e885fe2267..122c550ee7 100644 --- a/audio/6.0/IDevice.hal +++ b/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); }; diff --git a/audio/6.0/IStream.hal b/audio/6.0/IStream.hal index 451e1162bf..d7d3c8437c 100644 --- a/audio/6.0/IStream.hal +++ b/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); }; diff --git a/audio/core/all-versions/default/Device.cpp b/audio/core/all-versions/default/Device.cpp index 1a9df217e1..21dab00387 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; } @@ -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> Device::openOutputStreamImpl(int32_t ioHandle sp 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> Device::openInputStreamImpl( sp 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 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 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 PrimaryDevice::initCheck() { @@ -160,6 +164,11 @@ Return PrimaryDevice::setConnectedState(const DeviceAddress& address, bo return mDevice->setConnectedState(address, connected); } #endif +#if MAJOR_VERSION >= 6 +Return PrimaryDevice::close() { + return mDevice->close(); +} +#endif // Methods from ::android::hardware::audio::CPP_VERSION::IPrimaryDevice follow. Return 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, audio_stream_in_t* stream) - : mIsClosed(false), - mDevice(device), + : mDevice(device), mStream(stream), mStreamCommon(new Stream(&stream->common)), mStreamMmap(new StreamMmap(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 StreamIn::getMmapPosition(getMmapPosition_cb _hidl_cb) { } Return 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(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, audio_stream_out_t* stream) - : mIsClosed(false), - mDevice(device), + : mDevice(device), mStream(stream), mStreamCommon(new Stream(&stream->common)), mStreamMmap(new StreamMmap(stream)), @@ -148,7 +147,7 @@ StreamOut::StreamOut(const sp& 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 StreamOut::setParameters(const hidl_vec& context, #endif Return 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(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..11ab6077ab 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 getMicrophones(getMicrophones_cb _hidl_cb) override; Return setConnectedState(const DeviceAddress& address, bool connected) override; #endif +#if MAJOR_VERSION >= 6 + Return close() override; +#endif Return debug(const hidl_handle& fd, const hidl_vec& 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; 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 getMicrophones(getMicrophones_cb _hidl_cb) override; Return setConnectedState(const DeviceAddress& address, bool connected) override; #endif +#if MAJOR_VERSION >= 6 + Return close() override; +#endif Return debug(const hidl_handle& fd, const hidl_vec& 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 mDevice; audio_stream_in_t* mStream; const sp 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 mDevice; audio_stream_out_t* mStream; const sp 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..709b7cd369 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 baseDevice; - ASSERT_OK(getDevicesFactory()->openDevice("primary", returnIn(result, baseDevice))); - ASSERT_OK(result); - ASSERT_TRUE(baseDevice != nullptr); - - Return> primaryDevice = IPrimaryDevice::castFrom(baseDevice); - ASSERT_TRUE(primaryDevice.isOk()); - ASSERT_TRUE(sp(primaryDevice) != nullptr); + { // Scope for device SPs + sp baseDevice = + DeviceManager::getInstance().get(getFactoryName(), DeviceManager::kPrimaryDevice); + ASSERT_TRUE(baseDevice != nullptr); + Return> primaryDevice = IPrimaryDevice::castFrom(baseDevice); + EXPECT_TRUE(primaryDevice.isOk()); + EXPECT_TRUE(sp(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 stream; + StreamHelper 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 activeMicrophones; Result readRes; typedef MessageQueue 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); } diff --git a/audio/core/all-versions/vts/functional/6.0/AudioPrimaryHidlHalTest.cpp b/audio/core/all-versions/vts/functional/6.0/AudioPrimaryHidlHalTest.cpp index 30f8a7ade7..22e60be380 100644 --- a/audio/core/all-versions/vts/functional/6.0/AudioPrimaryHidlHalTest.cpp +++ b/audio/core/all-versions/vts/functional/6.0/AudioPrimaryHidlHalTest.cpp @@ -100,7 +100,8 @@ const std::vector& 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& 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::NONE); + SourceMetadata initMetadata = {{{AudioUsage::MEDIA, AudioContentType::MUSIC, 1 /* gain */}}}; + sp stream; + StreamHelper 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::NONE); + SinkMetadata initMetadata = {{{.source = AudioSource::MIC, .gain = 1}}}; + sp stream; + StreamHelper 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()); +} diff --git a/audio/core/all-versions/vts/functional/AudioPrimaryHidlHalTest.h b/audio/core/all-versions/vts/functional/AudioPrimaryHidlHalTest.h index e76e24e725..d0d39e8683 100644 --- a/audio/core/all-versions/vts/functional/AudioPrimaryHidlHalTest.h +++ b/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 OpenStreamTest : public AudioHidlTestWithDeviceConfigParameter { - protected: +class StreamHelper { + public: + // StreamHelper doesn't own the stream, this is for simpler stream lifetime management. + explicit StreamHelper(sp& stream) : mStream(stream) {} template - 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& mStream; +}; + +template +class OpenStreamTest : public AudioHidlTestWithDeviceConfigParameter { + protected: + OpenStreamTest() : AudioHidlTestWithDeviceConfigParameter(), helper(stream) {} + template + 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 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; + StreamHelper 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::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::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; diff --git a/audio/core/all-versions/vts/functional/ConfigHelper.h b/audio/core/all-versions/vts/functional/ConfigHelper.h index 48aae8c5b3..a2f4116baa 100644 --- a/audio/core/all-versions/vts/functional/ConfigHelper.h +++ b/audio/core/all-versions/vts/functional/ConfigHelper.h @@ -57,12 +57,6 @@ struct ConfigHelper { {24000, 48000}, {AudioFormat::PCM_16_BIT}); } - static const vector getSupportedPlaybackAudioConfig() { - // TODO: retrieve audio config supported by the platform - // as declared in the policy configuration - return {}; - } - static const vector 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 getSupportedCaptureAudioConfig() { - // TODO: retrieve audio config supported by the platform - // as declared in the policy configuration - return {}; - } static vector combineAudioConfig(vector channelMasks, vector sampleRates, 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 InterfaceManager { public: + sp getExisting(const Key& name) { + auto existing = instances.find(name); + return existing != instances.end() ? existing->second : sp(); + } + sp 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 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= 6 + { + sp 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 Effect::setCurrentConfigForFeature(uint32_t featureId, } Return 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(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 Effect::debug(const hidl_handle& fd, const hidl_vec& /* 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 mInBuffer; sp mOutBuffer;