From d041930df973ead927260d1f317a81a16276d8ae Mon Sep 17 00:00:00 2001 From: Mikhail Naganov Date: Thu, 14 Nov 2019 13:57:15 -0800 Subject: [PATCH 1/5] 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 | 11 +++++++++ audio/6.0/IStream.hal | 6 ++++- audio/core/all-versions/default/Device.cpp | 17 ++++++++++--- .../all-versions/default/PrimaryDevice.cpp | 11 ++++++++- audio/core/all-versions/default/StreamIn.cpp | 15 +++++++----- audio/core/all-versions/default/StreamOut.cpp | 17 +++++++------ .../default/include/core/default/Device.h | 8 ++++++- .../include/core/default/PrimaryDevice.h | 3 +++ .../default/include/core/default/StreamIn.h | 1 - .../default/include/core/default/StreamOut.h | 1 - .../4.0/AudioPrimaryHidlHalTest.cpp | 24 +++++++++---------- .../vts/functional/AudioPrimaryHidlHalTest.h | 2 ++ .../vts/functional/DeviceManager.h | 24 +++++++++++++++---- audio/effect/6.0/IEffect.hal | 5 +++- audio/effect/all-versions/default/Effect.cpp | 19 ++++++++++----- audio/effect/all-versions/default/Effect.h | 1 - 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 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 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..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 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,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 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..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 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)); } ////////////////////////////////////////////////////////////////////////////// @@ -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 e76e24e725..88494f5d70 100644 --- a/audio/core/all-versions/vts/functional/AudioPrimaryHidlHalTest.h +++ b/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: 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; From a4b693f5dae50dbaa1effc59f7bffeff1f07ba9a Mon Sep 17 00:00:00 2001 From: Mikhail Naganov Date: Fri, 15 Nov 2019 16:21:38 -0800 Subject: [PATCH 2/5] Audio VTS: Fix MMAP tests Ensure stream test runs for output MMAP profiles. Enhance checks for MMAP buffer size. Bug: 144575694 Test: atest VtsHalAudioV6_0TargetTest Change-Id: I93e66b12c93c466d661e65c4dbbb5deb32772848 Merged-In: I93e66b12c93c466d661e65c4dbbb5deb32772848 --- audio/6.0/IStream.hal | 2 +- .../6.0/AudioPrimaryHidlHalTest.cpp | 3 ++- .../vts/functional/AudioPrimaryHidlHalTest.h | 20 +++++++++---------- 3 files changed, 12 insertions(+), 13 deletions(-) diff --git a/audio/6.0/IStream.hal b/audio/6.0/IStream.hal index 1f62017cae..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. */ 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..7e931ff1b1 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; diff --git a/audio/core/all-versions/vts/functional/AudioPrimaryHidlHalTest.h b/audio/core/all-versions/vts/functional/AudioPrimaryHidlHalTest.h index 88494f5d70..6fc9339c54 100644 --- a/audio/core/all-versions/vts/functional/AudioPrimaryHidlHalTest.h +++ b/audio/core/all-versions/vts/functional/AudioPrimaryHidlHalTest.h @@ -1203,19 +1203,17 @@ TEST_IO_STREAM(closeTwice, "Make sure a stream can not be closed twice", waitForStreamDestruction()) // 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; From 13b99b4cf4fbb770d4abd53f5504054fea2286ff Mon Sep 17 00:00:00 2001 From: Mikhail Naganov Date: Wed, 20 Nov 2019 21:56:36 -0800 Subject: [PATCH 3/5] audio: Cleanup VTS tests Remove getSupported*AudioConfig methods as they were always returning empty vector, and for V6 test parameter generation has been changed. Test: atest VtsHalAudioV5_0TargetTest Change-Id: Ib6f53c52b3ee2769cb02730d02decd97390fe091 --- .../vts/functional/AudioPrimaryHidlHalTest.h | 22 ++----------------- .../vts/functional/ConfigHelper.h | 11 ---------- 2 files changed, 2 insertions(+), 31 deletions(-) diff --git a/audio/core/all-versions/vts/functional/AudioPrimaryHidlHalTest.h b/audio/core/all-versions/vts/functional/AudioPrimaryHidlHalTest.h index 6fc9339c54..38a06c4740 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( @@ -930,12 +924,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( @@ -946,7 +934,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); @@ -991,12 +979,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( @@ -1007,7 +989,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); 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, From 422afc131a003d9958ac306bc9360da3e3569157 Mon Sep 17 00:00:00 2001 From: Mikhail Naganov Date: Wed, 20 Nov 2019 16:58:12 -0800 Subject: [PATCH 4/5] audio: Factor out IStream operations into a helper class Bug: 114451103 Test: atest VtsHalAudioV5_0TargetTest atest VtsHalAudioV6_0TargetTest Change-Id: I7c1b16df2b52fd5279ff1f431bfc7269fb9cd8f6 --- .../4.0/AudioPrimaryHidlHalTest.cpp | 28 ++--- .../vts/functional/AudioPrimaryHidlHalTest.h | 104 +++++++++++------- 2 files changed, 71 insertions(+), 61 deletions(-) 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 81f963d34d..709b7cd369 100644 --- a/audio/core/all-versions/vts/functional/4.0/AudioPrimaryHidlHalTest.cpp +++ b/audio/core/all-versions/vts/functional/4.0/AudioPrimaryHidlHalTest.cpp @@ -52,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; @@ -65,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; @@ -110,13 +105,8 @@ TEST_P(AudioHidlDeviceTest, GetMicrophonesTest) { ASSERT_OK(res); 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 + helper.close(true /*clear*/, &res); + ASSERT_OK(res); 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 38a06c4740..d0d39e8683 100644 --- a/audio/core/all-versions/vts/functional/AudioPrimaryHidlHalTest.h +++ b/audio/core/all-versions/vts/functional/AudioPrimaryHidlHalTest.h @@ -809,62 +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() { -#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: void TearDown() override { if (open) { @@ -877,6 +899,7 @@ class OpenStreamTest : public AudioHidlTestWithDeviceConfigParameter { AudioConfig audioConfig; DeviceAddress address = {}; sp stream; + StreamHelper helper; bool open = false; }; @@ -1178,11 +1201,8 @@ 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 testMmapBufferOfInvalidSize(IStream* stream) { From ed261bbfb1b44e15aeb8fce208f22860c21d4a67 Mon Sep 17 00:00:00 2001 From: Mikhail Naganov Date: Wed, 20 Nov 2019 15:17:08 -0800 Subject: [PATCH 5/5] audio: Add check to IDevice.close for currently opened streams IDevice.close must not proceed if there are streams that are currently opened on this device. Bug: 114451103 Test: atest VtsHalAudioV6_0TargetTest Change-Id: I61d81bc0333098c341d5d551bf59331e49fcf682 --- audio/6.0/IDevice.hal | 6 ++- audio/core/all-versions/default/Device.cpp | 8 +++- .../default/include/core/default/Device.h | 1 + .../6.0/AudioPrimaryHidlHalTest.cpp | 46 +++++++++++++++++++ 4 files changed, 59 insertions(+), 2 deletions(-) diff --git a/audio/6.0/IDevice.hal b/audio/6.0/IDevice.hal index 520e776c21..122c550ee7 100644 --- a/audio/6.0/IDevice.hal +++ b/audio/6.0/IDevice.hal @@ -286,8 +286,12 @@ interface IDevice { * 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. + * INVALID_STATE if the device was already closed + * or there are streams currently opened. */ @exit close() generates (Result retval); diff --git a/audio/core/all-versions/default/Device.cpp b/audio/core/all-versions/default/Device.cpp index 5ea4c8df59..21dab00387 100644 --- a/audio/core/all-versions/default/Device.cpp +++ b/audio/core/all-versions/default/Device.cpp @@ -53,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) { @@ -158,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}; @@ -184,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,7 +389,7 @@ Return Device::setConnectedState(const DeviceAddress& address, bool conn #endif Result Device::doClose() { - if (mIsClosed) return Result::INVALID_STATE; + if (mIsClosed || mOpenedStreamsCount != 0) return Result::INVALID_STATE; mIsClosed = true; return analyzeStatus("close", audio_hw_device_close(mDevice)); } 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 dc53a3961a..11ab6077ab 100644 --- a/audio/core/all-versions/default/include/core/default/Device.h +++ b/audio/core/all-versions/default/include/core/default/Device.h @@ -130,6 +130,7 @@ struct Device : public IDevice, public ParametersUtil { private: bool mIsClosed; audio_hw_device_t* mDevice; + int mOpenedStreamsCount = 0; virtual ~Device(); 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 7e931ff1b1..22e60be380 100644 --- a/audio/core/all-versions/vts/functional/6.0/AudioPrimaryHidlHalTest.cpp +++ b/audio/core/all-versions/vts/functional/6.0/AudioPrimaryHidlHalTest.cpp @@ -145,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()); +}