From ef1345a869352cbcb1d035c9aa114eec2e86fe07 Mon Sep 17 00:00:00 2001 From: Mikhail Naganov Date: Fri, 23 Jun 2023 13:39:40 -0700 Subject: [PATCH] audio: Propagate errors from Driver::setConnectedDevices Plumb propagation of errors reported by the implementations of DriverInterface::setConnectedDevices up to the Module. This allows returning the reported errors from the corresponding IModule interface methods. Implement handling of errors from connection state update by the Module implementation. When updating streams about the connection status ensure that the list of connected devices is not empty. Also, add an extra check to AudioStreamIn#ActiveMicrophones to validate the size of the returned active microphones list. Bug: 282568751 Test: atest VtsHalAudioCoreTargetTest Merged-In: I62a422d95c37a672fce4ad221bea435cc7b4ebfa Change-Id: I62a422d95c37a672fce4ad221bea435cc7b4ebfa (cherry picked from commit 75b59dfb4e19c7f60f80e942aebfaae730eb7df2) --- audio/aidl/default/Module.cpp | 86 ++++++++++++++----- audio/aidl/default/include/core-impl/Module.h | 3 +- audio/aidl/default/include/core-impl/Stream.h | 16 ++-- audio/aidl/default/usb/StreamUsb.cpp | 2 +- .../vts/VtsHalAudioCoreModuleTargetTest.cpp | 1 + 5 files changed, 77 insertions(+), 31 deletions(-) diff --git a/audio/aidl/default/Module.cpp b/audio/aidl/default/Module.cpp index ab71ef4ed2..c0c6c4889d 100644 --- a/audio/aidl/default/Module.cpp +++ b/audio/aidl/default/Module.cpp @@ -340,30 +340,61 @@ void Module::registerPatch(const AudioPatch& patch) { do_insert(patch.sinkPortConfigIds); } -void Module::updateStreamsConnectedState(const AudioPatch& oldPatch, const AudioPatch& newPatch) { +ndk::ScopedAStatus Module::updateStreamsConnectedState(const AudioPatch& oldPatch, + const AudioPatch& newPatch) { // Streams from the old patch need to be disconnected, streams from the new // patch need to be connected. If the stream belongs to both patches, no need // to update it. - std::set idsToDisconnect, idsToConnect; + auto maybeFailure = ndk::ScopedAStatus::ok(); + std::set idsToDisconnect, idsToConnect, idsToDisconnectOnFailure; idsToDisconnect.insert(oldPatch.sourcePortConfigIds.begin(), oldPatch.sourcePortConfigIds.end()); idsToDisconnect.insert(oldPatch.sinkPortConfigIds.begin(), oldPatch.sinkPortConfigIds.end()); idsToConnect.insert(newPatch.sourcePortConfigIds.begin(), newPatch.sourcePortConfigIds.end()); idsToConnect.insert(newPatch.sinkPortConfigIds.begin(), newPatch.sinkPortConfigIds.end()); std::for_each(idsToDisconnect.begin(), idsToDisconnect.end(), [&](const auto& portConfigId) { - if (idsToConnect.count(portConfigId) == 0) { - LOG(DEBUG) << "The stream on port config id " << portConfigId << " is not connected"; - mStreams.setStreamIsConnected(portConfigId, {}); + if (idsToConnect.count(portConfigId) == 0 && mStreams.count(portConfigId) != 0) { + if (auto status = mStreams.setStreamConnectedDevices(portConfigId, {}); status.isOk()) { + LOG(DEBUG) << "updateStreamsConnectedState: The stream on port config id " + << portConfigId << " has been disconnected"; + } else { + // Disconnection is tricky to roll back, just register a failure. + maybeFailure = std::move(status); + } } }); + if (!maybeFailure.isOk()) return maybeFailure; std::for_each(idsToConnect.begin(), idsToConnect.end(), [&](const auto& portConfigId) { - if (idsToDisconnect.count(portConfigId) == 0) { + if (idsToDisconnect.count(portConfigId) == 0 && mStreams.count(portConfigId) != 0) { const auto connectedDevices = findConnectedDevices(portConfigId); - LOG(DEBUG) << "The stream on port config id " << portConfigId - << " is connected to: " << ::android::internal::ToString(connectedDevices); - mStreams.setStreamIsConnected(portConfigId, connectedDevices); + if (connectedDevices.empty()) { + // This is important as workers use the vector size to derive the connection status. + LOG(FATAL) << "updateStreamsConnectedState: No connected devices found for port " + "config id " + << portConfigId; + } + if (auto status = mStreams.setStreamConnectedDevices(portConfigId, connectedDevices); + status.isOk()) { + LOG(DEBUG) << "updateStreamsConnectedState: The stream on port config id " + << portConfigId << " has been connected to: " + << ::android::internal::ToString(connectedDevices); + } else { + maybeFailure = std::move(status); + idsToDisconnectOnFailure.insert(portConfigId); + } } }); + if (!maybeFailure.isOk()) { + LOG(WARNING) << __func__ << ": Due to a failure, disconnecting streams on port config ids " + << ::android::internal::ToString(idsToDisconnectOnFailure); + std::for_each(idsToDisconnectOnFailure.begin(), idsToDisconnectOnFailure.end(), + [&](const auto& portConfigId) { + auto status = mStreams.setStreamConnectedDevices(portConfigId, {}); + (void)status.isOk(); // Can't do much about a failure here. + }); + return maybeFailure; + } + return ndk::ScopedAStatus::ok(); } ndk::ScopedAStatus Module::setModuleDebug( @@ -659,12 +690,12 @@ ndk::ScopedAStatus Module::openInputStream(const OpenInputStreamArguments& in_ar RETURN_STATUS_IF_ERROR(getStreamInCreator(mType)(in_args.sinkMetadata, std::move(context), mConfig->microphones, &stream)); StreamWrapper streamWrapper(stream); + if (auto patchIt = mPatches.find(in_args.portConfigId); patchIt != mPatches.end()) { + RETURN_STATUS_IF_ERROR( + streamWrapper.setConnectedDevices(findConnectedDevices(in_args.portConfigId))); + } AIBinder_setMinSchedulerPolicy(streamWrapper.getBinder().get(), SCHED_NORMAL, ANDROID_PRIORITY_AUDIO); - auto patchIt = mPatches.find(in_args.portConfigId); - if (patchIt != mPatches.end()) { - streamWrapper.setStreamIsConnected(findConnectedDevices(in_args.portConfigId)); - } mStreams.insert(port->id, in_args.portConfigId, std::move(streamWrapper)); _aidl_return->stream = std::move(stream); return ndk::ScopedAStatus::ok(); @@ -705,12 +736,12 @@ ndk::ScopedAStatus Module::openOutputStream(const OpenOutputStreamArguments& in_ RETURN_STATUS_IF_ERROR(getStreamOutCreator(mType)(in_args.sourceMetadata, std::move(context), in_args.offloadInfo, &stream)); StreamWrapper streamWrapper(stream); + if (auto patchIt = mPatches.find(in_args.portConfigId); patchIt != mPatches.end()) { + RETURN_STATUS_IF_ERROR( + streamWrapper.setConnectedDevices(findConnectedDevices(in_args.portConfigId))); + } AIBinder_setMinSchedulerPolicy(streamWrapper.getBinder().get(), SCHED_NORMAL, ANDROID_PRIORITY_AUDIO); - auto patchIt = mPatches.find(in_args.portConfigId); - if (patchIt != mPatches.end()) { - streamWrapper.setStreamIsConnected(findConnectedDevices(in_args.portConfigId)); - } mStreams.insert(port->id, in_args.portConfigId, std::move(streamWrapper)); _aidl_return->stream = std::move(stream); return ndk::ScopedAStatus::ok(); @@ -813,13 +844,20 @@ ndk::ScopedAStatus Module::setAudioPatch(const AudioPatch& in_requested, AudioPa if (existing == patches.end()) { _aidl_return->id = getConfig().nextPatchId++; patches.push_back(*_aidl_return); - existing = patches.begin() + (patches.size() - 1); } else { oldPatch = *existing; - *existing = *_aidl_return; } - registerPatch(*existing); - updateStreamsConnectedState(oldPatch, *_aidl_return); + patchesBackup = mPatches; + registerPatch(*_aidl_return); + if (auto status = updateStreamsConnectedState(oldPatch, *_aidl_return); !status.isOk()) { + mPatches = std::move(*patchesBackup); + if (existing == patches.end()) { + patches.pop_back(); + } else { + *existing = oldPatch; + } + return status; + } LOG(DEBUG) << __func__ << ": " << (oldPatch.id == 0 ? "created" : "updated") << " patch " << _aidl_return->toString(); @@ -971,8 +1009,12 @@ ndk::ScopedAStatus Module::resetAudioPatch(int32_t in_patchId) { auto& patches = getConfig().patches; auto patchIt = findById(patches, in_patchId); if (patchIt != patches.end()) { + auto patchesBackup = mPatches; cleanUpPatch(patchIt->id); - updateStreamsConnectedState(*patchIt, AudioPatch{}); + if (auto status = updateStreamsConnectedState(*patchIt, AudioPatch{}); !status.isOk()) { + mPatches = std::move(patchesBackup); + return status; + } patches.erase(patchIt); LOG(DEBUG) << __func__ << ": erased patch " << in_patchId; return ndk::ScopedAStatus::ok(); diff --git a/audio/aidl/default/include/core-impl/Module.h b/audio/aidl/default/include/core-impl/Module.h index e8b3d899c3..212cfd9570 100644 --- a/audio/aidl/default/include/core-impl/Module.h +++ b/audio/aidl/default/include/core-impl/Module.h @@ -219,7 +219,8 @@ class Module : public BnModule { template std::set portIdsFromPortConfigIds(C portConfigIds); void registerPatch(const AudioPatch& patch); - void updateStreamsConnectedState(const AudioPatch& oldPatch, const AudioPatch& newPatch); + ndk::ScopedAStatus updateStreamsConnectedState(const AudioPatch& oldPatch, + const AudioPatch& newPatch); }; } // namespace aidl::android::hardware::audio::core diff --git a/audio/aidl/default/include/core-impl/Stream.h b/audio/aidl/default/include/core-impl/Stream.h index 4f84de900b..826b0f1ec9 100644 --- a/audio/aidl/default/include/core-impl/Stream.h +++ b/audio/aidl/default/include/core-impl/Stream.h @@ -395,11 +395,11 @@ class StreamCommonImpl : public StreamCommonInterface { : ndk::ScopedAStatus::fromExceptionCode(EX_ILLEGAL_STATE); } bool isClosed() const { return mWorker->isClosed(); } - void setIsConnected( + ndk::ScopedAStatus setConnectedDevices( const std::vector<::aidl::android::media::audio::common::AudioDevice>& devices) { mWorker->setIsConnected(!devices.empty()); mConnectedDevices = devices; - mDriver->setConnectedDevices(devices); + return ndk::ScopedAStatus::fromStatus(mDriver->setConnectedDevices(devices)); } ndk::ScopedAStatus updateMetadata(const Metadata& metadata); @@ -547,12 +547,13 @@ class StreamWrapper { }, mStream); } - void setStreamIsConnected( + ndk::ScopedAStatus setConnectedDevices( const std::vector<::aidl::android::media::audio::common::AudioDevice>& devices) { - std::visit( + return std::visit( [&](auto&& ws) { auto s = ws.lock(); - if (s) s->setIsConnected(devices); + if (s) return s->setConnectedDevices(devices); + return ndk::ScopedAStatus::ok(); }, mStream); } @@ -576,12 +577,13 @@ class Streams { mStreams.insert(std::pair{portConfigId, sw}); mStreams.insert(std::pair{portId, std::move(sw)}); } - void setStreamIsConnected( + ndk::ScopedAStatus setStreamConnectedDevices( int32_t portConfigId, const std::vector<::aidl::android::media::audio::common::AudioDevice>& devices) { if (auto it = mStreams.find(portConfigId); it != mStreams.end()) { - it->second.setStreamIsConnected(devices); + return it->second.setConnectedDevices(devices); } + return ndk::ScopedAStatus::ok(); } private: diff --git a/audio/aidl/default/usb/StreamUsb.cpp b/audio/aidl/default/usb/StreamUsb.cpp index 9ac1cc9b77..d2ee484c2d 100644 --- a/audio/aidl/default/usb/StreamUsb.cpp +++ b/audio/aidl/default/usb/StreamUsb.cpp @@ -72,7 +72,7 @@ DriverUsb::DriverUsb(const StreamContext& context, bool isInput) if (mIsInput && connectedDevices.size() > 1) { LOG(ERROR) << __func__ << ": wrong device size(" << connectedDevices.size() << ") for input stream"; - return ::android::BAD_VALUE; + return ::android::INVALID_OPERATION; } for (const auto& connectedDevice : connectedDevices) { if (connectedDevice.address.getTag() != AudioDeviceAddress::alsa) { diff --git a/audio/aidl/vts/VtsHalAudioCoreModuleTargetTest.cpp b/audio/aidl/vts/VtsHalAudioCoreModuleTargetTest.cpp index 825b865ef1..0012cd559e 100644 --- a/audio/aidl/vts/VtsHalAudioCoreModuleTargetTest.cpp +++ b/audio/aidl/vts/VtsHalAudioCoreModuleTargetTest.cpp @@ -2763,6 +2763,7 @@ TEST_P(AudioStreamIn, ActiveMicrophones) { ASSERT_NO_FATAL_FAILURE(patch.SetUp(module.get())); std::vector activeMics; EXPECT_IS_OK(stream.get()->getActiveMicrophones(&activeMics)); + EXPECT_FALSE(activeMics.empty()); for (const auto& mic : activeMics) { EXPECT_NE(micInfos.end(), std::find_if(micInfos.begin(), micInfos.end(),