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
diff --git a/tv/tuner/aidl/default/Filter.cpp b/tv/tuner/aidl/default/Filter.cpp
index 5e6d001..54037be 100644
--- a/tv/tuner/aidl/default/Filter.cpp
+++ b/tv/tuner/aidl/default/Filter.cpp
@@ -35,7 +35,11 @@
#define WAIT_TIMEOUT 3000000000
FilterCallbackScheduler::FilterCallbackScheduler(const std::shared_ptr<IFilterCallback>& cb)
- : mCallback(cb), mDataLength(0), mTimeDelayInMs(0), mDataSizeDelayInBytes(0) {
+ : mCallback(cb),
+ mIsConditionMet(false),
+ mDataLength(0),
+ mTimeDelayInMs(0),
+ mDataSizeDelayInBytes(0) {
start();
}
@@ -44,12 +48,14 @@
}
void FilterCallbackScheduler::onFilterEvent(DemuxFilterEvent&& event) {
- std::lock_guard<std::mutex> lock(mLock);
+ std::unique_lock<std::mutex> 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::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<std::mutex> 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<std::mutex> lock(mLock);
mDataSizeDelayInBytes = dataSizeDelay;
+ if (isDataSizeDelayConditionMetLocked()) {
+ mIsConditionMet = true;
+ lock.unlock();
+ mCv.notify_all();
+ }
}
bool FilterCallbackScheduler::hasCallbackRegistered() const {
@@ -96,6 +103,10 @@
void FilterCallbackScheduler::stop() {
mIsRunning = false;
if (mCallbackThread.joinable()) {
+ {
+ std::lock_guard<std::mutex> lock(mLock);
+ mIsConditionMet = true;
+ }
mCv.notify_all();
mCallbackThread.join();
}
@@ -109,17 +120,15 @@
void FilterCallbackScheduler::threadLoopOnce() {
std::unique_lock<std::mutex> 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 @@
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 7298561..8388c98 100644
--- a/tv/tuner/aidl/default/Filter.h
+++ b/tv/tuner/aidl/default/Filter.h
@@ -77,6 +77,9 @@
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 @@
std::thread mCallbackThread;
std::atomic<bool> mIsRunning;
- // mLock protects mCallbackBuffer, mCv, and mDataLength
+ // mLock protects mCallbackBuffer, mIsConditionMet, mCv, mDataLength,
+ // mTimeDelayInMs, and mDataSizeDelayInBytes
std::mutex mLock;
std::vector<DemuxFilterEvent> 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<int> mTimeDelayInMs;
- std::atomic<int> mDataSizeDelayInBytes;
+ int mTimeDelayInMs;
+ int mDataSizeDelayInBytes;
};
class Filter : public BnFilter {