summaryrefslogtreecommitdiffstats
path: root/media/libmedia
diff options
context:
space:
mode:
authorGlenn Kasten <gkasten@google.com>2014-01-13 10:21:48 -0800
committerGlenn Kasten <gkasten@google.com>2014-01-24 13:19:59 -0800
commit38e905b3cbba4da443d799b16999989781afc6d8 (patch)
tree47b872b97f1af95c3e0ceefb1f8d56c9ae7a67fc /media/libmedia
parentf0002d142e6d24c5438600b2c259679de710f8ac (diff)
downloadframeworks_av-38e905b3cbba4da443d799b16999989781afc6d8.zip
frameworks_av-38e905b3cbba4da443d799b16999989781afc6d8.tar.gz
frameworks_av-38e905b3cbba4da443d799b16999989781afc6d8.tar.bz2
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
Diffstat (limited to 'media/libmedia')
-rw-r--r--media/libmedia/AudioRecord.cpp21
-rw-r--r--media/libmedia/AudioTrack.cpp91
2 files changed, 67 insertions, 45 deletions
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 @@ status_t AudioRecord::set(
// create the IAudioRecord
status = openRecord_l(0 /*epoch*/);
- if (status) {
+ if (status != NO_ERROR) {
return status;
}
@@ -463,6 +463,9 @@ status_t AudioRecord::openRecord_l(size_t epoch)
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 @@ status_t AudioRecord::openRecord_l(size_t epoch)
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 @@ status_t AudioRecord::openRecord_l(size_t epoch)
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 @@ status_t AudioRecord::openRecord_l(size_t epoch)
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 @@ status_t AudioTrack::set(
return INVALID_OPERATION;
}
- mOutput = 0;
-
// handle default values first.
if (streamType == AUDIO_STREAM_DEFAULT) {
streamType = AUDIO_STREAM_MUSIC;
@@ -319,17 +317,6 @@ status_t AudioTrack::set(
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 @@ status_t AudioTrack::set(
frameCount,
flags,
sharedBuffer,
- output,
0 /*epoch*/);
if (status != NO_ERROR) {
@@ -379,9 +365,15 @@ status_t AudioTrack::set(
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 @@ status_t AudioTrack::set(
mSequence = 1;
mObservedSequence = mSequence;
mInUnderrun = false;
- mOutput = output;
return NO_ERROR;
}
@@ -821,23 +812,12 @@ status_t AudioTrack::reload()
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 @@ status_t AudioTrack::createTrack_l(
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 @@ status_t AudioTrack::createTrack_l(
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 @@ status_t AudioTrack::createTrack_l(
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 @@ status_t AudioTrack::createTrack_l(
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 @@ status_t AudioTrack::createTrack_l(
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 @@ status_t AudioTrack::createTrack_l(
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 AudioTrack::restoreTrack_l(const char *from)
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 @@ status_t AudioTrack::restoreTrack_l(const char *from)
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 @@ status_t AudioTrack::restoreTrack_l(const char *from)
mReqFrameCount, // so that frame count never goes down
mFlags,
mSharedBuffer,
- output,
position /*epoch*/);
if (result == NO_ERROR) {
@@ -1763,9 +1764,15 @@ status_t AudioTrack::restoreTrack_l(const char *from)
}
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;
}