AudioRecord: Clean up state handling
Handle spurious wakeups in stop.
Handle track invalidation.
Clarify state.
Note: start and stop should not be called simultaneously from different
binder threads.
Test: audio sanity
Bug: 117303225
Change-Id: I587d9cda7b0eac5215eb6a88f93ee08e396e3ef9
diff --git a/services/audioflinger/Threads.cpp b/services/audioflinger/Threads.cpp
index 01c5ea2..52a8fa8 100644
--- a/services/audioflinger/Threads.cpp
+++ b/services/audioflinger/Threads.cpp
@@ -6654,6 +6654,7 @@
case TrackBase::PAUSING:
mActiveTracks.remove(activeTrack);
+ activeTrack->mState = TrackBase::PAUSED;
doBroadcast = true;
size--;
continue;
@@ -6675,12 +6676,12 @@
allStopped = false;
break;
- case TrackBase::IDLE:
- i++;
- continue;
-
+ case TrackBase::IDLE: // cannot be on ActiveTracks if idle
+ case TrackBase::PAUSED: // cannot be on ActiveTracks if paused
+ case TrackBase::STOPPED: // cannot be on ActiveTracks if destroyed/terminated
default:
- LOG_ALWAYS_FATAL("Unexpected activeTrackState %d", activeTrackState);
+ LOG_ALWAYS_FATAL("%s: Unexpected active track state:%d, id:%d, tracks:%zu",
+ __func__, activeTrackState, activeTrack->id(), size);
}
activeTracks.add(activeTrack);
@@ -7323,8 +7324,14 @@
{
// This section is a rendezvous between binder thread executing start() and RecordThread
AutoMutex lock(mLock);
+ if (recordTrack->isInvalid()) {
+ recordTrack->clearSyncStartEvent();
+ return INVALID_OPERATION;
+ }
if (mActiveTracks.indexOf(recordTrack) >= 0) {
if (recordTrack->mState == TrackBase::PAUSING) {
+ // We haven't stopped yet (moved to PAUSED and not in mActiveTracks)
+ // so no need to startInput().
ALOGV("active record track PAUSING -> ACTIVE");
recordTrack->mState = TrackBase::ACTIVE;
} else {
@@ -7344,11 +7351,30 @@
bool silenced;
status = AudioSystem::startInput(recordTrack->portId(), &silenced);
mLock.lock();
- // FIXME should verify that recordTrack is still in mActiveTracks
+ if (recordTrack->isInvalid()) {
+ recordTrack->clearSyncStartEvent();
+ if (status == NO_ERROR && recordTrack->mState == TrackBase::STARTING_1) {
+ recordTrack->mState = TrackBase::STARTING_2;
+ // STARTING_2 forces destroy to call stopInput.
+ }
+ return INVALID_OPERATION;
+ }
+ if (recordTrack->mState != TrackBase::STARTING_1) {
+ ALOGW("%s(%d): unsynchronized mState:%d change",
+ __func__, recordTrack->id(), recordTrack->mState);
+ // Someone else has changed state, let them take over,
+ // leave mState in the new state.
+ recordTrack->clearSyncStartEvent();
+ return INVALID_OPERATION;
+ }
+ // we're ok, but perhaps startInput has failed
if (status != NO_ERROR) {
+ ALOGW("%s(%d): startInput failed, status %d",
+ __func__, recordTrack->id(), status);
+ // We are in ActiveTracks if STARTING_1 and valid, so remove from ActiveTracks,
+ // leave in STARTING_1, so destroy() will not call stopInput.
mActiveTracks.remove(recordTrack);
recordTrack->clearSyncStartEvent();
- ALOGV("RecordThread::start error %d", status);
return status;
}
recordTrack->setSilenced(silenced);
@@ -7366,21 +7392,8 @@
recordTrack->mState = TrackBase::STARTING_2;
// signal thread to start
mWaitWorkCV.broadcast();
- if (mActiveTracks.indexOf(recordTrack) < 0) {
- ALOGV("Record failed to start");
- status = BAD_VALUE;
- goto startError;
- }
return status;
}
-
-startError:
- if (recordTrack->isExternalTrack()) {
- AudioSystem::stopInput(recordTrack->portId());
- }
- recordTrack->clearSyncStartEvent();
- // FIXME I wonder why we do not reset the state here?
- return status;
}
void AudioFlinger::RecordThread::syncStartEventCallback(const wp<SyncEvent>& event)
@@ -7399,24 +7412,26 @@
bool AudioFlinger::RecordThread::stop(RecordThread::RecordTrack* recordTrack) {
ALOGV("RecordThread::stop");
AutoMutex _l(mLock);
+ // if we're invalid, we can't be on the ActiveTracks.
if (mActiveTracks.indexOf(recordTrack) < 0 || recordTrack->mState == TrackBase::PAUSING) {
return false;
}
// note that threadLoop may still be processing the track at this point [without lock]
recordTrack->mState = TrackBase::PAUSING;
- // signal thread to stop
- mWaitWorkCV.broadcast();
- // do not wait for mStartStopCond if exiting
- if (exitPending()) {
- return true;
+
+ while (recordTrack->mState == TrackBase::PAUSING && !recordTrack->isInvalid()) {
+ mWaitWorkCV.broadcast(); // signal thread to stop
+ mStartStopCond.wait(mLock);
}
- // FIXME incorrect usage of wait: no explicit predicate or loop
- mStartStopCond.wait(mLock);
- // if we have been restarted, recordTrack is in mActiveTracks here
- if (exitPending() || mActiveTracks.indexOf(recordTrack) < 0) {
+
+ if (recordTrack->mState == TrackBase::PAUSED) { // successful stop
ALOGV("Record stopped OK");
return true;
}
+
+ // don't handle anything - we've been invalidated or restarted and in a different state
+ ALOGW_IF("%s(%d): unsynchronized stop, state: %d",
+ __func__, recordTrack->id(), recordTrack->mState);
return false;
}
diff --git a/services/audioflinger/TrackBase.h b/services/audioflinger/TrackBase.h
index 92e79f2..c94639b 100644
--- a/services/audioflinger/TrackBase.h
+++ b/services/audioflinger/TrackBase.h
@@ -25,13 +25,13 @@
public:
enum track_state {
IDLE,
- FLUSHED,
+ FLUSHED, // for PlaybackTracks only
STOPPED,
// next 2 states are currently used for fast tracks
// and offloaded tracks only
STOPPING_1, // waiting for first underrun
STOPPING_2, // waiting for presentation complete
- RESUMING,
+ RESUMING, // for PlaybackTracks only
ACTIVE,
PAUSING,
PAUSED,
diff --git a/services/audioflinger/Tracks.cpp b/services/audioflinger/Tracks.cpp
index a99bbe1..f2617ae 100644
--- a/services/audioflinger/Tracks.cpp
+++ b/services/audioflinger/Tracks.cpp
@@ -1823,17 +1823,34 @@
// see comments at AudioFlinger::PlaybackThread::Track::destroy()
sp<RecordTrack> keep(this);
{
- if (isExternalTrack()) {
- if (mState == ACTIVE || mState == RESUMING) {
- AudioSystem::stopInput(mPortId);
- }
- AudioSystem::releaseInput(mPortId);
- }
+ track_state priorState = mState;
sp<ThreadBase> thread = mThread.promote();
if (thread != 0) {
Mutex::Autolock _l(thread->mLock);
RecordThread *recordThread = (RecordThread *) thread.get();
- recordThread->destroyTrack_l(this);
+ priorState = mState;
+ recordThread->destroyTrack_l(this); // move mState to STOPPED, terminate
+ }
+ // APM portid/client management done outside of lock.
+ // NOTE: if thread doesn't exist, the input descriptor probably doesn't either.
+ if (isExternalTrack()) {
+ switch (priorState) {
+ case ACTIVE: // invalidated while still active
+ case STARTING_2: // invalidated/start-aborted after startInput successfully called
+ case PAUSING: // invalidated while in the middle of stop() pausing (still active)
+ AudioSystem::stopInput(mPortId);
+ break;
+
+ case STARTING_1: // invalidated/start-aborted and startInput not successful
+ case PAUSED: // OK, not active
+ case IDLE: // OK, not active
+ break;
+
+ case STOPPED: // unexpected (destroyed)
+ default:
+ LOG_ALWAYS_FATAL("%s(%d): invalid prior state: %d", __func__, mId, priorState);
+ }
+ AudioSystem::releaseInput(mPortId);
}
}
}