Add record thread locking comments and FIXMEs
Change-Id: Ia5bdc9b8b013c2e40af17c82833051290bf4df70
diff --git a/services/audioflinger/Threads.cpp b/services/audioflinger/Threads.cpp
index 9c17574..387ed4e 100644
--- a/services/audioflinger/Threads.cpp
+++ b/services/audioflinger/Threads.cpp
@@ -4206,6 +4206,9 @@
bool readOnce = false;
// start recording
+ // FIXME Race here: exitPending could become true immediately after testing.
+ // It is only set to true while mLock held, but we don't hold mLock yet.
+ // Probably a benign race, but it would be safer to check exitPending with mLock held.
while (!exitPending()) {
processConfigEvents();
@@ -4272,7 +4275,10 @@
lockEffectChains_l(effectChains);
}
+ // thread mutex is now unlocked
+ // FIXME RecordThread::start assigns to mActiveTrack under lock, but we read without lock
if (mActiveTrack != 0) {
+ // FIXME RecordThread::stop assigns to mState under lock, but we read without lock
if (mActiveTrack->mState != TrackBase::ACTIVE &&
mActiveTrack->mState != TrackBase::RESUMING) {
unlockEffectChains(effectChains);
@@ -4280,6 +4286,7 @@
continue;
}
for (size_t i = 0; i < effectChains.size(); i ++) {
+ // thread mutex is not locked, but effect chain is locked
effectChains[i]->process_l();
}
@@ -4325,12 +4332,14 @@
mBytesRead = mInput->stream->read(mInput->stream, readInto,
mBufferSize);
if (mBytesRead <= 0) {
+ // FIXME read mState without lock
if ((mBytesRead < 0) && (mActiveTrack->mState == TrackBase::ACTIVE))
{
ALOGE("Error reading audio input");
// Force input into standby so that it tries to
// recover at next read attempt
inputStandBy();
+ // FIXME sleep with effect chains locked
usleep(kRecordThreadSleepUs);
}
mRsmpInIndex = mFrameCount;
@@ -4405,6 +4414,7 @@
// Release the processor for a while before asking for a new buffer.
// This will give the application more chance to read from the buffer and
// clear the overflow.
+ // FIXME sleep with effect chains locked
usleep(kRecordThreadSleepUs);
}
}
@@ -4573,6 +4583,7 @@
}
{
+ // This section is a rendezvous between binder thread executing start() and RecordThread
AutoMutex lock(mLock);
if (mActiveTrack != 0) {
if (recordTrack != mActiveTrack.get()) {
@@ -4583,11 +4594,13 @@
return status;
}
+ // FIXME why? already set in constructor, 'STARTING_1' would be more accurate
recordTrack->mState = TrackBase::IDLE;
mActiveTrack = recordTrack;
mLock.unlock();
status_t status = AudioSystem::startInput(mId);
mLock.lock();
+ // FIXME should verify that mActiveTrack is still == recordTrack
if (status != NO_ERROR) {
mActiveTrack.clear();
clearSyncStartEvent();
@@ -4598,6 +4611,8 @@
if (mResampler != NULL) {
mResampler->reset();
}
+ // FIXME hijacking a playback track state name which was intended for start after pause;
+ // here 'STARTING_2' would be more accurate
mActiveTrack->mState = TrackBase::RESUMING;
// signal thread to start
ALOGV("Signal record thread");
@@ -4608,6 +4623,7 @@
status = INVALID_OPERATION;
goto startError;
}
+ // FIXME incorrect usage of wait: no explicit predicate or loop
mStartStopCond.wait(mLock);
if (mActiveTrack == 0) {
ALOGV("Record failed to start");
@@ -4658,11 +4674,13 @@
if (recordTrack != mActiveTrack.get() || recordTrack->mState == TrackBase::PAUSING) {
return false;
}
+ // note that threadLoop may still be processing the track at this point [without lock]
recordTrack->mState = TrackBase::PAUSING;
// do not wait for mStartStopCond if exiting
if (exitPending()) {
return true;
}
+ // FIXME incorrect usage of wait: no explicit predicate or loop
mStartStopCond.wait(mLock);
// if we have been restarted, recordTrack == mActiveTrack.get() here
if (exitPending() || recordTrack != mActiveTrack.get()) {