Refactor code related to I/O handles to reduce chance for leaks
The AudioRecord input handle code was refactored earlier
to fix a potential handle leak, and to simplify the code:
> Change-Id: I124dce344b1d11c2dd66ca5e2c9aec0c52c230e2
This changelist refactors AudioTrack similarly,
and adds further cleanup of both AudioTrack and AudioRecord.
We attempt to implement the rules for referencing counting I/O handles,
but there is still the possibility of a handle leak if the client process
dies after allocating the handle reference but before releasing it.
That issue is being tracked separately.
Details:
- AudioSystem::getOutput() is now called within createTrack_l
- restoreTrack_l was missing offload info
now it has the info available,
but is not yet being called for offloaded tracks
- AudioTrack::getOutput() is now const
- Remove getOutput_l()
Change-Id: I44a0a623d24fc5847bcac0939c276400568adbca
diff --git a/include/media/AudioRecord.h b/include/media/AudioRecord.h
index 45134c4..3d839fc 100644
--- a/include/media/AudioRecord.h
+++ b/include/media/AudioRecord.h
@@ -486,12 +486,11 @@
int mSessionId;
transfer_type mTransfer;
- audio_io_handle_t mInput; // returned by AudioSystem::getInput()
-
- // Next 3 fields may be changed if IAudioRecord is re-created, but always != 0
+ // Next 4 fields may be changed if IAudioRecord is re-created, but always != 0
sp<IAudioRecord> mAudioRecord;
sp<IMemory> mCblkMemory;
audio_track_cblk_t* mCblk; // re-load after mLock.unlock()
+ audio_io_handle_t mInput; // returned by AudioSystem::getInput()
int mPreviousPriority; // before start()
SchedPolicy mPreviousSchedulingGroup;
diff --git a/include/media/AudioTrack.h b/include/media/AudioTrack.h
index 644e55c..cb67321 100644
--- a/include/media/AudioTrack.h
+++ b/include/media/AudioTrack.h
@@ -454,7 +454,7 @@
* Returned value:
* handle on audio hardware output
*/
- audio_io_handle_t getOutput();
+ audio_io_handle_t getOutput() const;
/* Returns the unique session ID associated with this track.
*
@@ -640,14 +640,12 @@
size_t frameCount,
audio_output_flags_t flags,
const sp<IMemory>& sharedBuffer,
- audio_io_handle_t output,
size_t epoch);
// can only be called when mState != STATE_ACTIVE
void flush_l();
void setLoop_l(uint32_t loopStart, uint32_t loopEnd, int loopCount);
- audio_io_handle_t getOutput_l();
// FIXME enum is faster than strcmp() for parameter 'from'
status_t restoreTrack_l(const char *from);
@@ -655,10 +653,11 @@
bool isOffloaded_l() const
{ return (mFlags & AUDIO_OUTPUT_FLAG_COMPRESS_OFFLOAD) != 0; }
- // Next 3 fields may be changed if IAudioTrack is re-created, but always != 0
+ // Next 4 fields may be changed if IAudioTrack is re-created, but always != 0
sp<IAudioTrack> mAudioTrack;
sp<IMemory> mCblkMemory;
audio_track_cblk_t* mCblk; // re-load after mLock.unlock()
+ audio_io_handle_t mOutput; // returned by AudioSystem::getOutput()
sp<AudioTrackThread> mAudioTrackThread;
@@ -763,7 +762,6 @@
sp<DeathNotifier> mDeathNotifier;
uint32_t mSequence; // incremented for each new IAudioTrack attempt
- audio_io_handle_t mOutput; // cached output io handle
int mClientUid;
};
diff --git a/media/libmedia/AudioRecord.cpp b/media/libmedia/AudioRecord.cpp
index 0673079..a999e7e 100644
--- a/media/libmedia/AudioRecord.cpp
+++ b/media/libmedia/AudioRecord.cpp
@@ -244,7 +244,7 @@
// create the IAudioRecord
status = openRecord_l(0 /*epoch*/);
- if (status) {
+ if (status != NO_ERROR) {
return status;
}
@@ -463,6 +463,9 @@
ALOGE("Could not get audio input for record source %d", mInputSource);
return BAD_VALUE;
}
+ {
+ // Now that we have a reference to an I/O handle and have not yet handed it off to AudioFlinger,
+ // we must release it ourselves if anything goes wrong.
size_t temp = mFrameCount; // temp may be replaced by a revised value of frameCount,
// but we will still need the original value also
@@ -480,9 +483,11 @@
if (record == 0 || status != NO_ERROR) {
ALOGE("AudioFlinger could not create record track, status: %d", status);
- AudioSystem::releaseInput(input);
- return status;
+ goto release;
}
+ // AudioFlinger now owns the reference to the I/O handle,
+ // so we are no longer responsible for releasing it.
+
sp<IMemory> iMem = record->getCblk();
if (iMem == 0) {
ALOGE("Could not get control block");
@@ -497,6 +502,8 @@
mAudioRecord->asBinder()->unlinkToDeath(mDeathNotifier, this);
mDeathNotifier.clear();
}
+
+ // We retain a copy of the I/O handle, but don't own the reference
mInput = input;
mAudioRecord = record;
mCblkMemory = iMem;
@@ -540,6 +547,14 @@
mAudioRecord->asBinder()->linkToDeath(mDeathNotifier, this);
return NO_ERROR;
+ }
+
+release:
+ AudioSystem::releaseInput(input);
+ if (status == NO_ERROR) {
+ status = NO_INIT;
+ }
+ return status;
}
status_t AudioRecord::obtainBuffer(Buffer* audioBuffer, int32_t waitCount)
diff --git a/media/libmedia/AudioTrack.cpp b/media/libmedia/AudioTrack.cpp
index c8dc38b..8e91f12 100644
--- a/media/libmedia/AudioTrack.cpp
+++ b/media/libmedia/AudioTrack.cpp
@@ -245,8 +245,6 @@
return INVALID_OPERATION;
}
- mOutput = 0;
-
// handle default values first.
if (streamType == AUDIO_STREAM_DEFAULT) {
streamType = AUDIO_STREAM_MUSIC;
@@ -319,17 +317,6 @@
mFrameSizeAF = sizeof(uint8_t);
}
- audio_io_handle_t output = AudioSystem::getOutput(
- streamType,
- sampleRate, format, channelMask,
- flags,
- offloadInfo);
-
- if (output == 0) {
- ALOGE("Could not get audio output for stream type %d", streamType);
- return BAD_VALUE;
- }
-
// Make copy of input parameter offloadInfo so that in the future:
// (a) createTrack_l doesn't need it as an input parameter
// (b) we can support re-creation of offloaded tracks
@@ -369,7 +356,6 @@
frameCount,
flags,
sharedBuffer,
- output,
0 /*epoch*/);
if (status != NO_ERROR) {
@@ -379,9 +365,15 @@
mAudioTrackThread.clear();
}
// Use of direct and offloaded output streams is ref counted by audio policy manager.
+#if 0 // FIXME This should no longer be needed
+ //Use of direct and offloaded output streams is ref counted by audio policy manager.
// As getOutput was called above and resulted in an output stream to be opened,
// we need to release it.
- AudioSystem::releaseOutput(output);
+ if (mOutput != 0) {
+ AudioSystem::releaseOutput(mOutput);
+ mOutput = 0;
+ }
+#endif
return status;
}
@@ -397,7 +389,6 @@
mSequence = 1;
mObservedSequence = mSequence;
mInUnderrun = false;
- mOutput = output;
return NO_ERROR;
}
@@ -821,23 +812,12 @@
return NO_ERROR;
}
-audio_io_handle_t AudioTrack::getOutput()
+audio_io_handle_t AudioTrack::getOutput() const
{
AutoMutex lock(mLock);
return mOutput;
}
-// must be called with mLock held
-audio_io_handle_t AudioTrack::getOutput_l()
-{
- if (mOutput) {
- return mOutput;
- } else {
- return AudioSystem::getOutput(mStreamType,
- mSampleRate, mFormat, mChannelMask, mFlags);
- }
-}
-
status_t AudioTrack::attachAuxEffect(int effectId)
{
AutoMutex lock(mLock);
@@ -858,7 +838,6 @@
size_t frameCount,
audio_output_flags_t flags,
const sp<IMemory>& sharedBuffer,
- audio_io_handle_t output,
size_t epoch)
{
status_t status;
@@ -868,27 +847,39 @@
return NO_INIT;
}
+ audio_io_handle_t output = AudioSystem::getOutput(mStreamType, mSampleRate, mFormat,
+ mChannelMask, mFlags, mOffloadInfo);
+ if (output == 0) {
+ ALOGE("Could not get audio output for stream type %d, sample rate %u, format %#x, "
+ "channel mask %#x, flags %#x",
+ mStreamType, mSampleRate, mFormat, mChannelMask, mFlags);
+ return BAD_VALUE;
+ }
+ {
+ // Now that we have a reference to an I/O handle and have not yet handed it off to AudioFlinger,
+ // we must release it ourselves if anything goes wrong.
+
// Not all of these values are needed under all conditions, but it is easier to get them all
uint32_t afLatency;
status = AudioSystem::getLatency(output, streamType, &afLatency);
if (status != NO_ERROR) {
ALOGE("getLatency(%d) failed status %d", output, status);
- return NO_INIT;
+ goto release;
}
size_t afFrameCount;
status = AudioSystem::getFrameCount(output, streamType, &afFrameCount);
if (status != NO_ERROR) {
ALOGE("getFrameCount(output=%d, streamType=%d) status %d", output, streamType, status);
- return NO_INIT;
+ goto release;
}
uint32_t afSampleRate;
status = AudioSystem::getSamplingRate(output, streamType, &afSampleRate);
if (status != NO_ERROR) {
ALOGE("getSamplingRate(output=%d, streamType=%d) status %d", output, streamType, status);
- return NO_INIT;
+ goto release;
}
// Client decides whether the track is TIMED (see below), but can only express a preference
@@ -940,7 +931,8 @@
if (((size_t)sharedBuffer->pointer() & (alignment - 1)) != 0) {
ALOGE("Invalid buffer alignment: address %p, channel count %u",
sharedBuffer->pointer(), mChannelCount);
- return BAD_VALUE;
+ status = BAD_VALUE;
+ goto release;
}
// When initializing a shared buffer AudioTrack via constructors,
@@ -1020,8 +1012,11 @@
if (track == 0) {
ALOGE("AudioFlinger could not create track, status: %d", status);
- return status;
+ goto release;
}
+ // AudioFlinger now owns the reference to the I/O handle,
+ // so we are no longer responsible for releasing it.
+
sp<IMemory> iMem = track->getCblk();
if (iMem == 0) {
ALOGE("Could not get control block");
@@ -1081,10 +1076,13 @@
ALOGW("AUDIO_OUTPUT_FLAG_OFFLOAD denied by server");
flags = (audio_output_flags_t) (flags & ~AUDIO_OUTPUT_FLAG_COMPRESS_OFFLOAD);
mFlags = flags;
- return NO_INIT;
+ // FIXME This is a warning, not an error, so don't return error status
+ //return NO_INIT;
}
}
+ // We retain a copy of the I/O handle, but don't own the reference
+ mOutput = output;
mRefreshRemaining = true;
// Starting address of buffers in shared memory. If there is a shared buffer, buffers
@@ -1127,6 +1125,14 @@
mAudioTrack->asBinder()->linkToDeath(mDeathNotifier, this);
return NO_ERROR;
+ }
+
+release:
+ AudioSystem::releaseOutput(output);
+ if (status == NO_ERROR) {
+ status = NO_INIT;
+ }
+ return status;
}
status_t AudioTrack::obtainBuffer(Buffer* audioBuffer, int32_t waitCount)
@@ -1708,7 +1714,7 @@
status_t result;
// refresh the audio configuration cache in this process to make sure we get new
- // output parameters in getOutput_l() and createTrack_l()
+ // output parameters in createTrack_l()
AudioSystem::clearAudioConfigCache();
if (isOffloaded_l()) {
@@ -1716,10 +1722,6 @@
return DEAD_OBJECT;
}
- // force new output query from audio policy manager;
- mOutput = 0;
- audio_io_handle_t output = getOutput_l();
-
// if the new IAudioTrack is created, createTrack_l() will modify the
// following member variables: mAudioTrack, mCblkMemory and mCblk.
// It will also delete the strong references on previous IAudioTrack and IMemory
@@ -1733,7 +1735,6 @@
mReqFrameCount, // so that frame count never goes down
mFlags,
mSharedBuffer,
- output,
position /*epoch*/);
if (result == NO_ERROR) {
@@ -1763,9 +1764,15 @@
}
if (result != NO_ERROR) {
// Use of direct and offloaded output streams is ref counted by audio policy manager.
+#if 0 // FIXME This should no longer be needed
+ //Use of direct and offloaded output streams is ref counted by audio policy manager.
// As getOutput was called above and resulted in an output stream to be opened,
// we need to release it.
- AudioSystem::releaseOutput(output);
+ if (mOutput != 0) {
+ AudioSystem::releaseOutput(mOutput);
+ mOutput = 0;
+ }
+#endif
ALOGW("restoreTrack_l() failed status %d", result);
mState = STATE_STOPPED;
}