From a2c5ddf993746c8eb55f6f2b2a376912af968253 Mon Sep 17 00:00:00 2001 From: Mikhail Naganov Date: Mon, 12 Sep 2022 22:57:14 +0000 Subject: [PATCH] audio: Cleanups and refactorings Added utility functions for operating on positional bit flags. Moved retrieval of offload mix ports to ModuleConfig utility class. Clarify the names of read/write tests. Bug: 205884982 Test: atest VtsHalAudioCoreTargetTest Change-Id: Id20881c2e62bc1b95d8fc3c268f99e36337dce7a --- audio/aidl/common/include/Utils.h | 35 ++++++++ audio/aidl/default/Configuration.cpp | 10 ++- audio/aidl/default/Module.cpp | 15 ++-- audio/aidl/default/include/core-impl/Stream.h | 5 +- audio/aidl/vts/ModuleConfig.cpp | 23 +++++- audio/aidl/vts/ModuleConfig.h | 2 + audio/aidl/vts/VtsHalAudioCoreTargetTest.cpp | 79 +++++++++---------- 7 files changed, 110 insertions(+), 59 deletions(-) diff --git a/audio/aidl/common/include/Utils.h b/audio/aidl/common/include/Utils.h index 74549d4d35..10261344f4 100644 --- a/audio/aidl/common/include/Utils.h +++ b/audio/aidl/common/include/Utils.h @@ -16,8 +16,13 @@ #pragma once +#include +#include + #include #include +#include +#include #include namespace android::hardware::audio::common { @@ -78,4 +83,34 @@ constexpr size_t getFrameSizeInBytes( return 0; } +// The helper functions defined below are only applicable to the case when an enum type +// specifies zero-based bit positions, not bit masks themselves. This is why instantiation +// is restricted to certain enum types. +template +using is_bit_position_enum = std::integral_constant< + bool, std::is_same_v || + std::is_same_v>; + +template , + typename = std::enable_if_t::value>> +constexpr U makeBitPositionFlagMask(E flag) { + return 1 << static_cast(flag); +} + +template , + typename = std::enable_if_t::value>> +constexpr bool isBitPositionFlagSet(U mask, E flag) { + return (mask & makeBitPositionFlagMask(flag)) != 0; +} + +template , + typename = std::enable_if_t::value>> +constexpr U makeBitPositionFlagMask(std::initializer_list flags) { + U result = 0; + for (const auto flag : flags) { + result |= makeBitPositionFlagMask(flag); + } + return result; +} + } // namespace android::hardware::audio::common diff --git a/audio/aidl/default/Configuration.cpp b/audio/aidl/default/Configuration.cpp index f5d679bd3e..a3e5ff7d6a 100644 --- a/audio/aidl/default/Configuration.cpp +++ b/audio/aidl/default/Configuration.cpp @@ -14,6 +14,7 @@ * limitations under the License. */ +#include #include #include #include @@ -40,6 +41,7 @@ using aidl::android::media::audio::common::AudioPortMixExt; using aidl::android::media::audio::common::AudioProfile; using aidl::android::media::audio::common::Int; using aidl::android::media::audio::common::PcmType; +using android::hardware::audio::common::makeBitPositionFlagMask; namespace aidl::android::hardware::audio::core::internal { @@ -193,7 +195,7 @@ Configuration& getNullPrimaryConfiguration() { createDeviceExt(AudioDeviceType::OUT_SPEAKER, 0))); AudioPort primaryOutMix = createPort(c.nextPortId++, "primary output", - 1 << static_cast(AudioOutputFlags::PRIMARY), + makeBitPositionFlagMask(AudioOutputFlags::PRIMARY), false, createPortMixExt(1, 1)); primaryOutMix.profiles.insert(primaryOutMix.profiles.begin(), standardPcmAudioProfiles.begin(), @@ -202,9 +204,9 @@ Configuration& getNullPrimaryConfiguration() { AudioPort compressedOffloadOutMix = createPort(c.nextPortId++, "compressed offload", - 1 << static_cast(AudioOutputFlags::DIRECT) | - 1 << static_cast(AudioOutputFlags::COMPRESS_OFFLOAD) | - 1 << static_cast(AudioOutputFlags::NON_BLOCKING), + makeBitPositionFlagMask({AudioOutputFlags::DIRECT, + AudioOutputFlags::COMPRESS_OFFLOAD, + AudioOutputFlags::NON_BLOCKING}), false, createPortMixExt(1, 1)); compressedOffloadOutMix.profiles.push_back( createProfile(::android::MEDIA_MIMETYPE_AUDIO_MPEG, diff --git a/audio/aidl/default/Module.cpp b/audio/aidl/default/Module.cpp index af033d0e9b..deaca49989 100644 --- a/audio/aidl/default/Module.cpp +++ b/audio/aidl/default/Module.cpp @@ -43,6 +43,7 @@ using aidl::android::media::audio::common::AudioProfile; using aidl::android::media::audio::common::Int; using aidl::android::media::audio::common::PcmType; using android::hardware::audio::common::getFrameSizeInBytes; +using android::hardware::audio::common::isBitPositionFlagSet; namespace aidl::android::hardware::audio::core { @@ -125,11 +126,11 @@ ndk::ScopedAStatus Module::createStreamContext(int32_t in_portConfigId, int64_t } const auto& flags = portConfigIt->flags.value(); if ((flags.getTag() == AudioIoFlags::Tag::input && - (flags.get() & - 1 << static_cast(AudioInputFlags::MMAP_NOIRQ)) == 0) || + !isBitPositionFlagSet(flags.get(), + AudioInputFlags::MMAP_NOIRQ)) || (flags.getTag() == AudioIoFlags::Tag::output && - (flags.get() & - 1 << static_cast(AudioOutputFlags::MMAP_NOIRQ)) == 0)) { + !isBitPositionFlagSet(flags.get(), + AudioOutputFlags::MMAP_NOIRQ))) { StreamContext temp( std::make_unique(1, true /*configureEventFlagWord*/), std::make_unique(1, true /*configureEventFlagWord*/), @@ -478,9 +479,9 @@ ndk::ScopedAStatus Module::openOutputStream(const OpenOutputStreamArguments& in_ << " does not correspond to an output mix port"; return ndk::ScopedAStatus::fromExceptionCode(EX_ILLEGAL_ARGUMENT); } - if ((port->flags.get() & - 1 << static_cast(AudioOutputFlags::COMPRESS_OFFLOAD)) != 0 && - !in_args.offloadInfo.has_value()) { + const bool isOffload = isBitPositionFlagSet(port->flags.get(), + AudioOutputFlags::COMPRESS_OFFLOAD); + if (isOffload && !in_args.offloadInfo.has_value()) { LOG(ERROR) << __func__ << ": port id " << port->id << " has COMPRESS_OFFLOAD flag set, requires offload info"; return ndk::ScopedAStatus::fromExceptionCode(EX_ILLEGAL_ARGUMENT); diff --git a/audio/aidl/default/include/core-impl/Stream.h b/audio/aidl/default/include/core-impl/Stream.h index 816cdb1f0d..488edf116a 100644 --- a/audio/aidl/default/include/core-impl/Stream.h +++ b/audio/aidl/default/include/core-impl/Stream.h @@ -229,10 +229,9 @@ class StreamWrapper { } void setStreamIsConnected(bool connected) { std::visit( - [&](auto&& ws) -> bool { + [&](auto&& ws) { auto s = ws.lock(); if (s) s->setIsConnected(connected); - return !!s; }, mStream); } @@ -253,7 +252,7 @@ class Streams { } void insert(int32_t portId, int32_t portConfigId, StreamWrapper sw) { mStreams.insert(std::pair{portConfigId, sw}); - mStreams.insert(std::pair{portId, sw}); + mStreams.insert(std::pair{portId, std::move(sw)}); } void setStreamIsConnected(int32_t portConfigId, bool connected) { if (auto it = mStreams.find(portConfigId); it != mStreams.end()) { diff --git a/audio/aidl/vts/ModuleConfig.cpp b/audio/aidl/vts/ModuleConfig.cpp index e36ab4a9bd..33c5b72ad4 100644 --- a/audio/aidl/vts/ModuleConfig.cpp +++ b/audio/aidl/vts/ModuleConfig.cpp @@ -17,6 +17,7 @@ #include #include +#include #include #include @@ -39,14 +40,15 @@ using aidl::android::media::audio::common::AudioPortExt; using aidl::android::media::audio::common::AudioProfile; using aidl::android::media::audio::common::AudioUsage; using aidl::android::media::audio::common::Int; +using android::hardware::audio::common::isBitPositionFlagSet; // static std::optional ModuleConfig::generateOffloadInfoIfNeeded( const AudioPortConfig& portConfig) { if (portConfig.flags.has_value() && portConfig.flags.value().getTag() == AudioIoFlags::Tag::output && - (portConfig.flags.value().get() & - 1 << static_cast(AudioOutputFlags::COMPRESS_OFFLOAD)) != 0) { + isBitPositionFlagSet(portConfig.flags.value().get(), + AudioOutputFlags::COMPRESS_OFFLOAD)) { AudioOffloadInfo offloadInfo; offloadInfo.base.sampleRate = portConfig.sampleRate.value().value; offloadInfo.base.channelMask = portConfig.channelMask.value(); @@ -123,6 +125,23 @@ std::vector ModuleConfig::getOutputMixPorts() const { return result; } +std::vector ModuleConfig::getOffloadMixPorts(bool attachedOnly, bool singlePort) const { + std::vector result; + const auto mixPorts = getMixPorts(false /*isInput*/); + auto offloadPortIt = mixPorts.begin(); + while (offloadPortIt != mixPorts.end()) { + offloadPortIt = std::find_if(offloadPortIt, mixPorts.end(), [&](const AudioPort& port) { + return isBitPositionFlagSet(port.flags.get(), + AudioOutputFlags::COMPRESS_OFFLOAD) && + (!attachedOnly || !getAttachedSinkDevicesPortsForMixPort(port).empty()); + }); + if (offloadPortIt == mixPorts.end()) break; + result.push_back(*offloadPortIt++); + if (singlePort) break; + } + return result; +} + std::vector ModuleConfig::getAttachedDevicesPortsForMixPort( bool isInput, const AudioPortConfig& mixPortConfig) const { const auto mixPortIt = findById(mPorts, mixPortConfig.portId); diff --git a/audio/aidl/vts/ModuleConfig.h b/audio/aidl/vts/ModuleConfig.h index 552f971ab9..dc109a792f 100644 --- a/audio/aidl/vts/ModuleConfig.h +++ b/audio/aidl/vts/ModuleConfig.h @@ -48,6 +48,8 @@ class ModuleConfig { std::vector getMixPorts(bool isInput) const { return isInput ? getInputMixPorts() : getOutputMixPorts(); } + std::vector getOffloadMixPorts( + bool attachedOnly, bool singlePort) const; std::vector getAttachedDevicesPortsForMixPort( bool isInput, const aidl::android::media::audio::common::AudioPort& mixPort) const { diff --git a/audio/aidl/vts/VtsHalAudioCoreTargetTest.cpp b/audio/aidl/vts/VtsHalAudioCoreTargetTest.cpp index ab70ec4c29..2659f79edc 100644 --- a/audio/aidl/vts/VtsHalAudioCoreTargetTest.cpp +++ b/audio/aidl/vts/VtsHalAudioCoreTargetTest.cpp @@ -26,6 +26,7 @@ #include #include +#include #include #include #include @@ -64,6 +65,7 @@ using aidl::android::media::audio::common::AudioPortDeviceExt; using aidl::android::media::audio::common::AudioPortExt; using aidl::android::media::audio::common::AudioSource; using aidl::android::media::audio::common::AudioUsage; +using android::hardware::audio::common::isBitPositionFlagSet; using android::hardware::audio::common::StreamLogic; using android::hardware::audio::common::StreamWorker; using ndk::ScopedAStatus; @@ -97,20 +99,6 @@ AudioDeviceAddress GenerateUniqueDeviceAddress() { return AudioDeviceAddress::make(std::to_string(++nextId)); } -template -struct IsInput { - constexpr operator bool() const; -}; - -template <> -constexpr IsInput::operator bool() const { - return true; -} -template <> -constexpr IsInput::operator bool() const { - return false; -} - // All 'With*' classes are move-only because they are associated with some // resource or state of a HAL module. class WithDebugFlags { @@ -864,12 +852,12 @@ TEST_P(AudioCoreModule, CheckMixPorts) { ASSERT_EQ(EX_NONE, status.getExceptionCode()) << status; } std::optional primaryMixPort; - constexpr int primaryOutputFlag = 1 << static_cast(AudioOutputFlags::PRIMARY); for (const auto& port : ports) { if (port.ext.getTag() != AudioPortExt::Tag::mix) continue; const auto& mixPort = port.ext.get(); if (port.flags.getTag() == AudioIoFlags::Tag::output && - ((port.flags.get() & primaryOutputFlag) != 0)) { + isBitPositionFlagSet(port.flags.get(), + AudioOutputFlags::PRIMARY)) { EXPECT_FALSE(primaryMixPort.has_value()) << "At least two mix ports have PRIMARY flag set: " << primaryMixPort.value() << " and " << port.id; @@ -1447,14 +1435,15 @@ class AudioStream : public AudioCoreModule { EXPECT_NO_FATAL_FAILURE(OpenTwiceSamePortConfigImpl(portConfig.value())); } - void ReadOrWrite(bool useImpl2, bool testObservablePosition) { + void ReadOrWrite(bool useSetupSequence2, bool validateObservablePosition) { const auto allPortConfigs = moduleConfig->getPortConfigsForMixPorts(IOTraits::is_input); if (allPortConfigs.empty()) { GTEST_SKIP() << "No mix ports have attached devices"; } for (const auto& portConfig : allPortConfigs) { - EXPECT_NO_FATAL_FAILURE(ReadOrWriteImpl(portConfig, useImpl2, testObservablePosition)) + EXPECT_NO_FATAL_FAILURE( + ReadOrWriteImpl(portConfig, useSetupSequence2, validateObservablePosition)) << portConfig.toString(); } } @@ -1510,17 +1499,20 @@ class AudioStream : public AudioCoreModule { EXPECT_GT(frames, framesInitial); } - void ReadOrWriteImpl(const AudioPortConfig& portConfig, bool useImpl2, - bool testObservablePosition) { - if (!useImpl2) { - ASSERT_NO_FATAL_FAILURE(ReadOrWriteImpl1(portConfig, testObservablePosition)); + void ReadOrWriteImpl(const AudioPortConfig& portConfig, bool useSetupSequence2, + bool validateObservablePosition) { + if (!useSetupSequence2) { + ASSERT_NO_FATAL_FAILURE( + ReadOrWriteSetupSequence1(portConfig, validateObservablePosition)); } else { - ASSERT_NO_FATAL_FAILURE(ReadOrWriteImpl2(portConfig, testObservablePosition)); + ASSERT_NO_FATAL_FAILURE( + ReadOrWriteSetupSequence2(portConfig, validateObservablePosition)); } } // Set up a patch first, then open a stream. - void ReadOrWriteImpl1(const AudioPortConfig& portConfig, bool testObservablePosition) { + void ReadOrWriteSetupSequence1(const AudioPortConfig& portConfig, + bool validateObservablePosition) { auto devicePorts = moduleConfig->getAttachedDevicesPortsForMixPort( IOTraits::is_input, portConfig); ASSERT_FALSE(devicePorts.empty()); @@ -1534,13 +1526,14 @@ class AudioStream : public AudioCoreModule { ASSERT_TRUE(worker.start()); ASSERT_TRUE(worker.waitForAtLeastOneCycle()); - if (testObservablePosition) { + if (validateObservablePosition) { ASSERT_NO_FATAL_FAILURE(WaitForObservablePositionAdvance(worker)); } } // Open a stream, then set up a patch for it. - void ReadOrWriteImpl2(const AudioPortConfig& portConfig, bool testObservablePosition) { + void ReadOrWriteSetupSequence2(const AudioPortConfig& portConfig, + bool validateObservablePosition) { WithStream stream(portConfig); ASSERT_NO_FATAL_FAILURE(stream.SetUp(module.get(), kDefaultBufferSizeFrames)); typename IOTraits::Worker worker(*stream.getContext()); @@ -1554,7 +1547,7 @@ class AudioStream : public AudioCoreModule { ASSERT_TRUE(worker.start()); ASSERT_TRUE(worker.waitForAtLeastOneCycle()); - if (testObservablePosition) { + if (validateObservablePosition) { ASSERT_NO_FATAL_FAILURE(WaitForObservablePositionAdvance(worker)); } } @@ -1609,19 +1602,24 @@ TEST_IO_STREAM(OpenInvalidBufferSize); TEST_IO_STREAM(OpenInvalidDirection); TEST_IO_STREAM(OpenOverMaxCount); TEST_IO_STREAM(OpenTwiceSamePortConfig); -TEST_IO_STREAM_2(ReadOrWrite, false, false); -TEST_IO_STREAM_2(ReadOrWrite, true, false); -TEST_IO_STREAM_2(ReadOrWrite, false, true); -TEST_IO_STREAM_2(ReadOrWrite, true, true); +// Use of constants makes comprehensible test names. +constexpr bool SetupSequence1 = false; +constexpr bool SetupSequence2 = true; +constexpr bool SetupOnly = false; +constexpr bool ValidateObservablePosition = true; +TEST_IO_STREAM_2(ReadOrWrite, SetupSequence1, SetupOnly); +TEST_IO_STREAM_2(ReadOrWrite, SetupSequence2, SetupOnly); +TEST_IO_STREAM_2(ReadOrWrite, SetupSequence1, ValidateObservablePosition); +TEST_IO_STREAM_2(ReadOrWrite, SetupSequence2, ValidateObservablePosition); TEST_IO_STREAM(ResetPortConfigWithOpenStream); TEST_IO_STREAM(SendInvalidCommand); TEST_P(AudioStreamOut, OpenTwicePrimary) { const auto mixPorts = moduleConfig->getMixPorts(false); auto primaryPortIt = std::find_if(mixPorts.begin(), mixPorts.end(), [](const AudioPort& port) { - constexpr int primaryOutputFlag = 1 << static_cast(AudioOutputFlags::PRIMARY); return port.flags.getTag() == AudioIoFlags::Tag::output && - (port.flags.get() & primaryOutputFlag) != 0; + isBitPositionFlagSet(port.flags.get(), + AudioOutputFlags::PRIMARY); }); if (primaryPortIt == mixPorts.end()) { GTEST_SKIP() << "No primary mix port"; @@ -1635,19 +1633,14 @@ TEST_P(AudioStreamOut, OpenTwicePrimary) { } TEST_P(AudioStreamOut, RequireOffloadInfo) { - const auto mixPorts = moduleConfig->getMixPorts(false); - auto offloadPortIt = std::find_if(mixPorts.begin(), mixPorts.end(), [&](const AudioPort& port) { - constexpr int compressOffloadFlag = 1 - << static_cast(AudioOutputFlags::COMPRESS_OFFLOAD); - return port.flags.getTag() == AudioIoFlags::Tag::output && - (port.flags.get() & compressOffloadFlag) != 0 && - !moduleConfig->getAttachedSinkDevicesPortsForMixPort(port).empty(); - }); - if (offloadPortIt == mixPorts.end()) { + const auto offloadMixPorts = + moduleConfig->getOffloadMixPorts(true /*attachedOnly*/, true /*singlePort*/); + if (offloadMixPorts.empty()) { GTEST_SKIP() << "No mix port for compressed offload that could be routed to attached devices"; } - const auto portConfig = moduleConfig->getSingleConfigForMixPort(false, *offloadPortIt); + const auto portConfig = + moduleConfig->getSingleConfigForMixPort(false, *offloadMixPorts.begin()); ASSERT_TRUE(portConfig.has_value()) << "No profiles specified for the compressed offload mix port"; StreamDescriptor descriptor;