diff options
author | Glenn Kasten <gkasten@google.com> | 2012-02-21 10:32:45 -0800 |
---|---|---|
committer | Glenn Kasten <gkasten@google.com> | 2012-07-17 18:06:19 -0700 |
commit | 955e78180ac6111c54f50930b0c4c12395e86cf7 (patch) | |
tree | b4c12804643f73e9acbbf1db58f5ffa7e0bd7bc7 | |
parent | ea682976030a3930f6ee49b33b7e21abfc68174a (diff) | |
download | frameworks_av-955e78180ac6111c54f50930b0c4c12395e86cf7.zip frameworks_av-955e78180ac6111c54f50930b0c4c12395e86cf7.tar.gz frameworks_av-955e78180ac6111c54f50930b0c4c12395e86cf7.tar.bz2 |
AudioRecord locking
Fix race conditions for EVENT_MARKER and EVENT_NEW_POS callbacks.
Marker and new position update fields are protected by lock.
getSampleRate() doesn't need a lock because it reads from shared memory
control block.
Enforce that the parameter passed with EVENT_MARKER and EVENT_NEW_POS
cannot not be changed by the callback handler, and will not change during
the call by another thread.
Session ID should never change; log if it does.
Change-Id: Ia2c63cf1a71b10bb06c37981bd76437f83fffa91
-rw-r--r-- | include/media/AudioRecord.h | 4 | ||||
-rw-r--r-- | media/libmedia/AudioRecord.cpp | 45 |
2 files changed, 33 insertions, 16 deletions
diff --git a/include/media/AudioRecord.h b/include/media/AudioRecord.h index 7e66ac9..156c592 100644 --- a/include/media/AudioRecord.h +++ b/include/media/AudioRecord.h @@ -84,8 +84,8 @@ public: * more bytes than indicated by 'size' field and update 'size' if less bytes are * read. * - EVENT_OVERRUN: unused. - * - EVENT_MARKER: pointer to an uin32_t containing the marker position in frames. - * - EVENT_NEW_POS: pointer to an uin32_t containing the new position in frames. + * - EVENT_MARKER: pointer to const uint32_t containing the marker position in frames. + * - EVENT_NEW_POS: pointer to const uint32_t containing the new position in frames. */ typedef void (*callback_t)(int event, void* user, void *info); diff --git a/media/libmedia/AudioRecord.cpp b/media/libmedia/AudioRecord.cpp index 9d5813b..5060525 100644 --- a/media/libmedia/AudioRecord.cpp +++ b/media/libmedia/AudioRecord.cpp @@ -355,7 +355,6 @@ bool AudioRecord::stopped() const uint32_t AudioRecord::getSampleRate() const { - AutoMutex lock(mLock); return mCblk->sampleRate; } @@ -363,6 +362,7 @@ status_t AudioRecord::setMarkerPosition(uint32_t marker) { if (mCbf == NULL) return INVALID_OPERATION; + AutoMutex lock(mLock); mMarkerPosition = marker; mMarkerReached = false; @@ -373,6 +373,7 @@ status_t AudioRecord::getMarkerPosition(uint32_t *marker) const { if (marker == NULL) return BAD_VALUE; + AutoMutex lock(mLock); *marker = mMarkerPosition; return NO_ERROR; @@ -384,6 +385,8 @@ status_t AudioRecord::setPositionUpdatePeriod(uint32_t updatePeriod) uint32_t curPosition; getPosition(&curPosition); + + AutoMutex lock(mLock); mNewPosition = curPosition + updatePeriod; mUpdatePeriod = updatePeriod; @@ -394,6 +397,7 @@ status_t AudioRecord::getPositionUpdatePeriod(uint32_t *updatePeriod) const { if (updatePeriod == NULL) return BAD_VALUE; + AutoMutex lock(mLock); *updatePeriod = mUpdatePeriod; return NO_ERROR; @@ -434,6 +438,7 @@ status_t AudioRecord::openRecord_l( pid_t tid = -1; // FIXME see similar logic at AudioTrack + int originalSessionId = mSessionId; sp<IAudioRecord> record = audioFlinger->openRecord(getpid(), input, sampleRate, format, channelMask, @@ -442,6 +447,8 @@ status_t AudioRecord::openRecord_l( tid, &mSessionId, &status); + ALOGE_IF(originalSessionId != 0 && mSessionId != originalSessionId, + "session ID changed from %d to %d", originalSessionId, mSessionId); if (record == 0) { ALOGE("AudioFlinger could not create record track, status: %d", status); @@ -587,6 +594,7 @@ audio_io_handle_t AudioRecord::getInput_l() int AudioRecord::getSessionId() const { + // no lock needed because session ID doesn't change after first set() return mSessionId; } @@ -658,22 +666,31 @@ bool AudioRecord::processAudioBuffer(const sp<AudioRecordThread>& thread) sp<IMemory> iMem = mCblkMemory; audio_track_cblk_t* cblk = mCblk; bool active = mActive; + uint32_t markerPosition = mMarkerPosition; + uint32_t newPosition = mNewPosition; + uint32_t user = cblk->user; + // determine whether a marker callback will be needed, while locked + bool needMarker = !mMarkerReached && (mMarkerPosition > 0) && (user >= mMarkerPosition); + if (needMarker) { + mMarkerReached = true; + } + // determine the number of new position callback(s) that will be needed, while locked + uint32_t updatePeriod = mUpdatePeriod; + uint32_t needNewPos = updatePeriod > 0 && user >= newPosition ? + ((user - newPosition) / updatePeriod) + 1 : 0; + mNewPosition = newPosition + updatePeriod * needNewPos; mLock.unlock(); - // Manage marker callback - if (!mMarkerReached && (mMarkerPosition > 0)) { - if (cblk->user >= mMarkerPosition) { - mCbf(EVENT_MARKER, mUserData, (void *)&mMarkerPosition); - mMarkerReached = true; - } + // perform marker callback, while unlocked + if (needMarker) { + mCbf(EVENT_MARKER, mUserData, &markerPosition); } - // Manage new position callback - if (mUpdatePeriod > 0) { - while (cblk->user >= mNewPosition) { - mCbf(EVENT_NEW_POS, mUserData, (void *)&mNewPosition); - mNewPosition += mUpdatePeriod; - } + // perform new position callback(s), while unlocked + for (; needNewPos > 0; --needNewPos) { + uint32_t temp = newPosition; + mCbf(EVENT_NEW_POS, mUserData, &temp); + newPosition += updatePeriod; } do { @@ -721,7 +738,7 @@ bool AudioRecord::processAudioBuffer(const sp<AudioRecordThread>& thread) // otherwise we would have exited when obtainBuffer returned STOPPED earlier. ALOGV("Overrun user: %x, server: %x, flags %04x", cblk->user, cblk->server, cblk->flags); if (!(android_atomic_or(CBLK_UNDERRUN_ON, &cblk->flags) & CBLK_UNDERRUN_MSK)) { - mCbf(EVENT_OVERRUN, mUserData, 0); + mCbf(EVENT_OVERRUN, mUserData, NULL); } } |