Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 328d6774 authored by Andy Hung's avatar Andy Hung
Browse files

Effects: Add atomic_wp<T>, update setThread.

atomic_wp<T> is used to allow concurrent read and write to the wp<>.
We use this to fix EffectCallback::setThread to resolve multithreaded
access to the wp<T>.

Note that setThread is used for EffectChain migration between
PlaybackThreads; we also need to refine higher level transactional
locking to ensure enable/disable of effect is done consistently
during migration.

Test: basic audio works
Test: atest media_synchronization_tests
Test: AudioEffectTest AudioPreProcessingTest BassBoostTest
Test: EnvReverbTest EqualizerTest LoudnessEnhancerTest
Test: PresetReverbTest VirtualizerTest VisualizerTest
Bug: 161341295
Change-Id: If80ee4373859c832d6fd10fec0385d69064f50c6
parent d8888d48
Loading
Loading
Loading
Loading
+7 −0
Original line number Diff line number Diff line
@@ -60,3 +60,10 @@ cc_library {
    local_include_dirs: ["include"],
    export_include_dirs: ["include"],
}

cc_library_headers {
    name: "libmediautils_headers",
    vendor_available: true,  // required for platform/hardware/interfaces

    export_include_dirs: ["include"],
}
+119 −0
Original line number Diff line number Diff line
/*
 * Copyright 2021, The Android Open Source Project
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *     http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

#pragma once
#include <mutex>
#include <utils/RefBase.h>

namespace android::mediautils {

/**
 * The LockItem class introduces a simple template which mimics atomic<T>
 * for non-trivially copyable types.  For trivially copyable types,
 * the LockItem will statically assert that an atomic<T> should be used instead.
 *
 * The default lock mutex is std::mutex which is suitable for all but rare cases
 * e.g. recursive constructors that might be found in tree construction,
 * setters that might recurse onto the same object.
 */

template <typename T, typename L = std::mutex, int FLAGS = 0>
class LockItem {
protected:
    mutable L mLock;
    mutable T mT;

public:
    enum {
        // Best practices for smart pointers and complex containers is to move to a temp
        // and invoke destructor outside of lock.  This reduces time under lock and in
        // some cases eliminates deadlock.
        FLAG_DTOR_OUT_OF_LOCK = 1,
    };

    // Check type, suggest std::atomic if possible.
    static_assert(!std::is_trivially_copyable_v<T>,
            "type is trivially copyable, please use std::atomic instead");

    // Allow implicit conversions as expected for some types, e.g. sp -> wp.
    template <typename... Args>
    LockItem(Args&&... args) : mT(std::forward<Args>(args)...) {
    }

    // NOT copy or move / assignable or constructible.

    // Do not enable this because it may lead to confusion because it returns
    // a copy-value not a reference.
    // operator T() const { return load(); }

    // any conversion done under lock.
    template <typename U>
    void operator=(U&& u) {
        store(std::forward<U>(u));
    }

    // returns a copy-value not a reference.
    T load() const {
        std::lock_guard lock(mLock);
        return mT;
    }

    // any conversion done under lock.
    template <typename U>
    void store(U&& u) {
        if constexpr ((FLAGS & FLAG_DTOR_OUT_OF_LOCK) != 0) {
             std::unique_lock lock(mLock);
             T temp = std::move(mT);
             mT = std::forward<U>(u);
             lock.unlock();
        } else {
            std::lock_guard lock(mLock);
            mT = std::forward<U>(u);
        }
    }
};

/**
 * atomic_wp<> and atomic_sp<> are used for concurrent access to Android
 * sp<> and wp<> smart pointers, including their modifiers.  We
 * return a copy of the smart pointer with load().
 *
 * Historical: The importance of an atomic<std::shared_ptr<T>> class is described
 * by Herb Sutter in the following ISO document https://isocpp.org/files/papers/N4162.pdf
 * and is part of C++20.  Lock free versions of atomic smart pointers are available
 * publicly but usually require specialized smart pointer structs.
 * See also https://en.cppreference.com/w/cpp/memory/shared_ptr/atomic
 * and https://en.cppreference.com/w/cpp/memory/shared_ptr/atomic2
 *
 * We offer lock based atomic_wp<> and atomic_sp<> objects here. This is useful to
 * copy the Android smart pointer to a different variable for subsequent local access,
 * where the change of the original object after copy is acceptable.
 *
 * Note: Instead of atomics, it is often preferrable to create an explicit visible lock to
 * ensure complete transaction consistency.  For example, one might want to ensure
 * that the method called from the smart pointer is also done under lock.
 * This may not be possible for callbacks due to inverted lock ordering.
 */

template <typename T>
using atomic_wp = LockItem<::android::wp<T>>;

template <typename T>
using atomic_sp = LockItem<
        ::android::sp<T>, std::mutex, LockItem<::android::sp<T>>::FLAG_DTOR_OUT_OF_LOCK>;

} // namespace android::mediautils
+19 −0
Original line number Diff line number Diff line
cc_test {
    name: "media_synchronization_tests",

    cflags: [
        "-Wall",
        "-Werror",
        "-Wextra",
    ],

    shared_libs: [
        "liblog",
        "libmediautils",
        "libutils",
    ],

    srcs: [
        "media_synchronization_tests.cpp",
    ],
}
+82 −0
Original line number Diff line number Diff line
/*
 * Copyright (C) 2021 The Android Open Source Project
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *      http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

#define LOG_TAG "media_synchronization_tests"

#include <mediautils/Synchronization.h>

#include <gtest/gtest.h>
#include <utils/Log.h>

using namespace android;
using namespace android::mediautils;

// Simple Test Class
template <typename T>
class MyObject : public RefBase {
    T value_;
  public:
    MyObject(const T& value) : value_(value) {}
    MyObject(const MyObject<T>& mo) : value_(mo.get()) {}
    T get() const { return value_; }
    void set(const T& value) { value_ = value; }
};

TEST(media_synchronization_tests, atomic_wp) {
  sp<MyObject<int>> refobj = new MyObject<int>(20);
  atomic_wp<MyObject<int>> wpobj = refobj;

  // we can promote.
  ASSERT_EQ(20, wpobj.load().promote()->get());

  // same underlying object for sp and atomic_wp.
  ASSERT_EQ(refobj.get(), wpobj.load().promote().get());

  // behavior is consistent with same underlying object.
  wpobj.load().promote()->set(10);
  ASSERT_EQ(10, refobj->get());
  refobj->set(5);
  ASSERT_EQ(5, wpobj.load().promote()->get());

  // we can clear our weak ptr.
  wpobj = nullptr;
  ASSERT_EQ(nullptr, wpobj.load().promote());

  // didn't affect our original obj.
  ASSERT_NE(nullptr, refobj.get());
}

TEST(media_synchronization_tests, atomic_sp) {
  sp<MyObject<int>> refobj = new MyObject<int>(20);
  atomic_sp<MyObject<int>> spobj = refobj;

  // same underlying object for sp and atomic_sp.
  ASSERT_EQ(refobj.get(), spobj.load().get());

  // behavior is consistent with same underlying object.
  ASSERT_EQ(20, spobj.load()->get());
  spobj.load()->set(10);
  ASSERT_EQ(10, refobj->get());
  refobj->set(5);
  ASSERT_EQ(5, spobj.load()->get());

  // we can clear spobj.
  spobj = nullptr;
  ASSERT_EQ(nullptr, spobj.load().get());

  // didn't affect our original obj.
  ASSERT_NE(nullptr, refobj.get());
}
+1 −0
Original line number Diff line number Diff line
@@ -73,6 +73,7 @@
#include <media/ExtendedAudioBufferProvider.h>
#include <media/VolumeShaper.h>
#include <mediautils/ServiceUtilities.h>
#include <mediautils/Synchronization.h>

#include <audio_utils/clock.h>
#include <audio_utils/FdToString.h>
Loading