From 892366fe60043b4a8e2a9bfac95f4d7de231202a Mon Sep 17 00:00:00 2001 From: Patrick Rohr Date: Thu, 25 Nov 2021 12:06:02 +0100 Subject: [PATCH] fix tuner filter callback scheduler This fixes multiple issues in filter callback scheduler: - currently, when mDataSizeDelayInBytes is 0, filter events are sent every time onFilterEvent is called. When mTimeDelayInMs is set (to something else than 0), this will falsely override the time delay. - when datasize delay or time delays are updated, the cv needs to be notified so the new delay goes into effect right away. - std::condition_variables *must* make use of a shared variable in order to prevent lost and spurious wakeups. Test: atest VtsHalTvTunerTargetTest Bug: 183057734 Change-Id: I9fb4e87e8ba887f0ce891ccb9981bfa49a3ceada --- tv/tuner/aidl/default/Filter.cpp | 72 +++++++++++++++++++++----------- tv/tuner/aidl/default/Filter.h | 14 ++++--- 2 files changed, 56 insertions(+), 30 deletions(-) diff --git a/tv/tuner/aidl/default/Filter.cpp b/tv/tuner/aidl/default/Filter.cpp index 5e6d001346..54037be492 100644 --- a/tv/tuner/aidl/default/Filter.cpp +++ b/tv/tuner/aidl/default/Filter.cpp @@ -35,7 +35,11 @@ namespace tuner { #define WAIT_TIMEOUT 3000000000 FilterCallbackScheduler::FilterCallbackScheduler(const std::shared_ptr& cb) - : mCallback(cb), mDataLength(0), mTimeDelayInMs(0), mDataSizeDelayInBytes(0) { + : mCallback(cb), + mIsConditionMet(false), + mDataLength(0), + mTimeDelayInMs(0), + mDataSizeDelayInBytes(0) { start(); } @@ -44,12 +48,14 @@ FilterCallbackScheduler::~FilterCallbackScheduler() { } void FilterCallbackScheduler::onFilterEvent(DemuxFilterEvent&& event) { - std::lock_guard lock(mLock); + std::unique_lock lock(mLock); mCallbackBuffer.push_back(std::move(event)); mDataLength += getDemuxFilterEventDataLength(event); - if (mDataLength >= mDataSizeDelayInBytes) { - // size limit has been reached, send out events + if (isDataSizeDelayConditionMetLocked()) { + mIsConditionMet = true; + // unlock, so thread is not immediately blocked when it is notified. + lock.unlock(); mCv.notify_all(); } } @@ -67,21 +73,22 @@ void FilterCallbackScheduler::flushEvents() { } void FilterCallbackScheduler::setTimeDelayHint(int timeDelay) { - // updating the setTimeDelay does not go into effect until the condition - // variable times out or is notified. - // One possibility is to notify the condition variable right away when the - // time delay changes, but I don't see the benefit over waiting for the next - // timeout / push, since -- in any case -- this will not change very often. + std::unique_lock lock(mLock); mTimeDelayInMs = timeDelay; + // always notify condition variable to update timeout + mIsConditionMet = true; + lock.unlock(); + mCv.notify_all(); } void FilterCallbackScheduler::setDataSizeDelayHint(int dataSizeDelay) { - // similar to updating the time delay hint, when mDataSizeDelayInBytes - // changes, this will not go into immediate effect, but will wait until the - // next filterEvent. - // We could technically check the current data length and notify the - // condition variable if we wanted to, but again, this may be overkill. + std::unique_lock lock(mLock); mDataSizeDelayInBytes = dataSizeDelay; + if (isDataSizeDelayConditionMetLocked()) { + mIsConditionMet = true; + lock.unlock(); + mCv.notify_all(); + } } bool FilterCallbackScheduler::hasCallbackRegistered() const { @@ -96,6 +103,10 @@ void FilterCallbackScheduler::start() { void FilterCallbackScheduler::stop() { mIsRunning = false; if (mCallbackThread.joinable()) { + { + std::lock_guard lock(mLock); + mIsConditionMet = true; + } mCv.notify_all(); mCallbackThread.join(); } @@ -109,17 +120,15 @@ void FilterCallbackScheduler::threadLoop() { void FilterCallbackScheduler::threadLoopOnce() { std::unique_lock lock(mLock); - // mTimeDelayInMs is an atomic value, so it should be copied into a local - // variable before use (to make sure both the if statement and wait_for use - // the same value). - int timeDelayInMs = mTimeDelayInMs; - if (timeDelayInMs > 0) { - mCv.wait_for(lock, std::chrono::milliseconds(timeDelayInMs)); + if (mTimeDelayInMs > 0) { + // Note: predicate protects from lost and spurious wakeups + mCv.wait_for(lock, std::chrono::milliseconds(mTimeDelayInMs), + [this] { return mIsConditionMet; }); } else { - // no reason to timeout, just wait until main thread determines it's - // okay to send data. - mCv.wait(lock); + // Note: predicate protects from lost and spurious wakeups + mCv.wait(lock, [this] { return mIsConditionMet; }); } + mIsConditionMet = false; // condition_variable wait locks mutex on timeout / notify // Note: if stop() has been called in the meantime, do not send more filter @@ -131,7 +140,22 @@ void FilterCallbackScheduler::threadLoopOnce() { mCallbackBuffer.clear(); mDataLength = 0; } - lock.unlock(); +} + +// mLock needs to be held to call this function +bool FilterCallbackScheduler::isDataSizeDelayConditionMetLocked() { + if (mDataSizeDelayInBytes == 0) { + // Data size delay is disabled. + if (mTimeDelayInMs == 0) { + // Events should only be sent immediately if time delay is disabled + // as well. + return true; + } + return false; + } + + // Data size delay is enabled. + return mDataLength >= mDataSizeDelayInBytes; } int FilterCallbackScheduler::getDemuxFilterEventDataLength(const DemuxFilterEvent& event) { diff --git a/tv/tuner/aidl/default/Filter.h b/tv/tuner/aidl/default/Filter.h index 7298561d6a..8388c9861f 100644 --- a/tv/tuner/aidl/default/Filter.h +++ b/tv/tuner/aidl/default/Filter.h @@ -77,6 +77,9 @@ class FilterCallbackScheduler final { void threadLoop(); void threadLoopOnce(); + // function needs to be called while holding mLock + bool isDataSizeDelayConditionMetLocked(); + static int getDemuxFilterEventDataLength(const DemuxFilterEvent& event); private: @@ -84,16 +87,15 @@ class FilterCallbackScheduler final { std::thread mCallbackThread; std::atomic mIsRunning; - // mLock protects mCallbackBuffer, mCv, and mDataLength + // mLock protects mCallbackBuffer, mIsConditionMet, mCv, mDataLength, + // mTimeDelayInMs, and mDataSizeDelayInBytes std::mutex mLock; std::vector mCallbackBuffer; + bool mIsConditionMet; std::condition_variable mCv; int mDataLength; - - // both of these need to be atomic (not just mTimeDelayInMs) as this class - // needs to be threadsafe. - std::atomic mTimeDelayInMs; - std::atomic mDataSizeDelayInBytes; + int mTimeDelayInMs; + int mDataSizeDelayInBytes; }; class Filter : public BnFilter {