Merge "audio: Cleanups and refactorings"

audio/aidl/common/include/Utils.h

@@ -16,8 +16,13 @@
 
 #pragma once
 
+#include <initializer_list>
+#include <type_traits>
+
 #include <aidl/android/media/audio/common/AudioChannelLayout.h>
 #include <aidl/android/media/audio/common/AudioFormatDescription.h>
+#include <aidl/android/media/audio/common/AudioInputFlags.h>
+#include <aidl/android/media/audio/common/AudioOutputFlags.h>
 #include <aidl/android/media/audio/common/PcmType.h>
 
 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 <typename E>
+using is_bit_position_enum = std::integral_constant<
+        bool, std::is_same_v<E, ::aidl::android::media::audio::common::AudioInputFlags> ||
+                      std::is_same_v<E, ::aidl::android::media::audio::common::AudioOutputFlags>>;
+
+template <typename E, typename U = std::underlying_type_t<E>,
+          typename = std::enable_if_t<is_bit_position_enum<E>::value>>
+constexpr U makeBitPositionFlagMask(E flag) {
+    return 1 << static_cast<U>(flag);
+}
+
+template <typename E, typename U = std::underlying_type_t<E>,
+          typename = std::enable_if_t<is_bit_position_enum<E>::value>>
+constexpr bool isBitPositionFlagSet(U mask, E flag) {
+    return (mask & makeBitPositionFlagMask(flag)) != 0;
+}
+
+template <typename E, typename U = std::underlying_type_t<E>,
+          typename = std::enable_if_t<is_bit_position_enum<E>::value>>
+constexpr U makeBitPositionFlagMask(std::initializer_list<E> flags) {
+    U result = 0;
+    for (const auto flag : flags) {
+        result |= makeBitPositionFlagMask(flag);
+    }
+    return result;
+}
+
 }  // namespace android::hardware::audio::common

audio/aidl/default/Configuration.cpp

@@ -14,6 +14,7 @@
  * limitations under the License.
  */
 
+#include <Utils.h>
 #include <aidl/android/media/audio/common/AudioChannelLayout.h>
 #include <aidl/android/media/audio/common/AudioDeviceType.h>
 #include <aidl/android/media/audio/common/AudioFormatDescription.h>
@@ -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<int32_t>(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<int32_t>(AudioOutputFlags::DIRECT) |
+                           makeBitPositionFlagMask({AudioOutputFlags::DIRECT,
-                                   1 << static_cast<int32_t>(AudioOutputFlags::COMPRESS_OFFLOAD) |
+                                                    AudioOutputFlags::COMPRESS_OFFLOAD,
-                                   1 << static_cast<int32_t>(AudioOutputFlags::NON_BLOCKING),
+                                                    AudioOutputFlags::NON_BLOCKING}),
                            false, createPortMixExt(1, 1));
         compressedOffloadOutMix.profiles.push_back(
                 createProfile(::android::MEDIA_MIMETYPE_AUDIO_MPEG,

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<AudioIoFlags::Tag::input>() &
+         !isBitPositionFlagSet(flags.get<AudioIoFlags::Tag::input>(),
-          1 << static_cast<int32_t>(AudioInputFlags::MMAP_NOIRQ)) == 0) ||
+                               AudioInputFlags::MMAP_NOIRQ)) ||
         (flags.getTag() == AudioIoFlags::Tag::output &&
-         (flags.get<AudioIoFlags::Tag::output>() &
+         !isBitPositionFlagSet(flags.get<AudioIoFlags::Tag::output>(),
-          1 << static_cast<int32_t>(AudioOutputFlags::MMAP_NOIRQ)) == 0)) {
+                               AudioOutputFlags::MMAP_NOIRQ))) {
         StreamContext temp(
                 std::make_unique<StreamContext::CommandMQ>(1, true /*configureEventFlagWord*/),
                 std::make_unique<StreamContext::ReplyMQ>(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<AudioIoFlags::Tag::output>() &
+    const bool isOffload = isBitPositionFlagSet(port->flags.get<AudioIoFlags::Tag::output>(),
-         1 << static_cast<int32_t>(AudioOutputFlags::COMPRESS_OFFLOAD)) != 0 &&
+                                                AudioOutputFlags::COMPRESS_OFFLOAD);
-        !in_args.offloadInfo.has_value()) {
+    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);

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()) {

audio/aidl/vts/ModuleConfig.cpp

@@ -17,6 +17,7 @@
 #include <algorithm>
 #include <chrono>
 
+#include <Utils.h>
 #include <aidl/android/media/audio/common/AudioIoFlags.h>
 #include <aidl/android/media/audio/common/AudioOutputFlags.h>
 
@@ -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<AudioOffloadInfo> ModuleConfig::generateOffloadInfoIfNeeded(
         const AudioPortConfig& portConfig) {
     if (portConfig.flags.has_value() &&
         portConfig.flags.value().getTag() == AudioIoFlags::Tag::output &&
-        (portConfig.flags.value().get<AudioIoFlags::Tag::output>() &
+        isBitPositionFlagSet(portConfig.flags.value().get<AudioIoFlags::Tag::output>(),
-         1 << static_cast<int>(AudioOutputFlags::COMPRESS_OFFLOAD)) != 0) {
+                             AudioOutputFlags::COMPRESS_OFFLOAD)) {
         AudioOffloadInfo offloadInfo;
         offloadInfo.base.sampleRate = portConfig.sampleRate.value().value;
         offloadInfo.base.channelMask = portConfig.channelMask.value();
@@ -123,6 +125,23 @@ std::vector<AudioPort> ModuleConfig::getOutputMixPorts() const {
     return result;
 }
 
+std::vector<AudioPort> ModuleConfig::getOffloadMixPorts(bool attachedOnly, bool singlePort) const {
+    std::vector<AudioPort> 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<AudioIoFlags::Tag::output>(),
+                                        AudioOutputFlags::COMPRESS_OFFLOAD) &&
+                   (!attachedOnly || !getAttachedSinkDevicesPortsForMixPort(port).empty());
+        });
+        if (offloadPortIt == mixPorts.end()) break;
+        result.push_back(*offloadPortIt++);
+        if (singlePort) break;
+    }
+    return result;
+}
+
 std::vector<AudioPort> ModuleConfig::getAttachedDevicesPortsForMixPort(
         bool isInput, const AudioPortConfig& mixPortConfig) const {
     const auto mixPortIt = findById<AudioPort>(mPorts, mixPortConfig.portId);

audio/aidl/vts/ModuleConfig.h

@@ -48,6 +48,8 @@ class ModuleConfig {
     std::vector<aidl::android::media::audio::common::AudioPort> getMixPorts(bool isInput) const {
         return isInput ? getInputMixPorts() : getOutputMixPorts();
     }
+    std::vector<aidl::android::media::audio::common::AudioPort> getOffloadMixPorts(
+            bool attachedOnly, bool singlePort) const;
 
     std::vector<aidl::android::media::audio::common::AudioPort> getAttachedDevicesPortsForMixPort(
             bool isInput, const aidl::android::media::audio::common::AudioPort& mixPort) const {

audio/aidl/vts/VtsHalAudioCoreTargetTest.cpp

@@ -26,6 +26,7 @@
 #include <android-base/logging.h>
 
 #include <StreamWorker.h>
+#include <Utils.h>
 #include <aidl/Gtest.h>
 #include <aidl/Vintf.h>
 #include <aidl/android/hardware/audio/core/IConfig.h>
@@ -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<AudioDeviceAddress::Tag::id>(std::to_string(++nextId));
 }
 
-template <typename T>
-struct IsInput {
-    constexpr operator bool() const;
-};
-
-template <>
-constexpr IsInput<IStreamIn>::operator bool() const {
-    return true;
-}
-template <>
-constexpr IsInput<IStreamOut>::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<int32_t> primaryMixPort;
-    constexpr int primaryOutputFlag = 1 << static_cast<int>(AudioOutputFlags::PRIMARY);
     for (const auto& port : ports) {
         if (port.ext.getTag() != AudioPortExt::Tag::mix) continue;
         const auto& mixPort = port.ext.get<AudioPortExt::Tag::mix>();
         if (port.flags.getTag() == AudioIoFlags::Tag::output &&
-            ((port.flags.get<AudioIoFlags::Tag::output>() & primaryOutputFlag) != 0)) {
+            isBitPositionFlagSet(port.flags.get<AudioIoFlags::Tag::output>(),
+                                 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<Stream>::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,
+    void ReadOrWriteImpl(const AudioPortConfig& portConfig, bool useSetupSequence2,
-                         bool testObservablePosition) {
+                         bool validateObservablePosition) {
-        if (!useImpl2) {
+        if (!useSetupSequence2) {
-            ASSERT_NO_FATAL_FAILURE(ReadOrWriteImpl1(portConfig, testObservablePosition));
+            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<Stream>::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> stream(portConfig);
         ASSERT_NO_FATAL_FAILURE(stream.SetUp(module.get(), kDefaultBufferSizeFrames));
         typename IOTraits<Stream>::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);
+// Use of constants makes comprehensible test names.
-TEST_IO_STREAM_2(ReadOrWrite, true, false);
+constexpr bool SetupSequence1 = false;
-TEST_IO_STREAM_2(ReadOrWrite, false, true);
+constexpr bool SetupSequence2 = true;
-TEST_IO_STREAM_2(ReadOrWrite, true, 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<int>(AudioOutputFlags::PRIMARY);
         return port.flags.getTag() == AudioIoFlags::Tag::output &&
-               (port.flags.get<AudioIoFlags::Tag::output>() & primaryOutputFlag) != 0;
+               isBitPositionFlagSet(port.flags.get<AudioIoFlags::Tag::output>(),
+                                    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);
+    const auto offloadMixPorts =
-    auto offloadPortIt = std::find_if(mixPorts.begin(), mixPorts.end(), [&](const AudioPort& port) {
+            moduleConfig->getOffloadMixPorts(true /*attachedOnly*/, true /*singlePort*/);
-        constexpr int compressOffloadFlag = 1
+    if (offloadMixPorts.empty()) {
-                                            << static_cast<int>(AudioOutputFlags::COMPRESS_OFFLOAD);
-        return port.flags.getTag() == AudioIoFlags::Tag::output &&
-               (port.flags.get<AudioIoFlags::Tag::output>() & compressOffloadFlag) != 0 &&
-               !moduleConfig->getAttachedSinkDevicesPortsForMixPort(port).empty();
-    });
-    if (offloadPortIt == mixPorts.end()) {
         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;