From f9f27197eb8732dad8cac15c9c30880489fab425 Mon Sep 17 00:00:00 2001 From: John Grossman Date: Mon, 26 Mar 2012 17:51:46 -0700 Subject: [PATCH] TimedAudio: Fix a cause of audio popping. Fix an issue with buffer lifecycle management which could cause audio pops on timed outputs. There were two issues at work here. 1) During trim operations for the queued timed audio data, buffers were being trimmed based on their starting PTS instead of when the chunk of audio data actually ended. This means that if you have a very large chunk of audio data (larger than the mixer lead time), then a buffer at the head of the queue could be eligible to be trimmed before its data had been completely mixed into the output stream, even though the output stream was fully buffered and in no danger of underflow. 2) The implementation of getNextBuffer and releaseBuffer for timed audio tracks was not keeping anything like a reference to the data that it handed out to the mixer. The original architecture here seemed to be expecting a ring buffer design, but timed audio tracks use a packet based design. Pieces of packets are handed out to the mixer which then frequently will hold onto that chunk of data across two mix operations, using the first part of the chunk to finish a mix buffer and then using the end of the chunk for the start of the next mix buffer. If the buffer that the mixer is holding a piece of got trimmed before the start of the next mix operation, it would return to its heap and could be filled with who knows what by the time it actually got mixed. On debug builds, they seem to get zero'ed out as they go back to the heap causing obvious pops in presentation. This change addresses both issues. Trim operations are now based on ending presentation time for a chunk of audio, not the start. Also, when the head of the queue is in flight to the mixer, it can no longer be trimmed immediately, merely flagged for trim by the mixer when the mixer finally does call releaseBuffer. Signed-off-by: John Grossman Change-Id: Ia1ba08cb9dea35a698723ab2d9bcbf804f1682fe --- services/audioflinger/AudioFlinger.cpp | 93 +++++++++++++++++++++++++++------- services/audioflinger/AudioFlinger.h | 11 ++-- 2 files changed, 84 insertions(+), 20 deletions(-) diff --git a/services/audioflinger/AudioFlinger.cpp b/services/audioflinger/AudioFlinger.cpp index 08621bb351d2..2e7449e92949 100644 --- a/services/audioflinger/AudioFlinger.cpp +++ b/services/audioflinger/AudioFlinger.cpp @@ -3841,6 +3841,8 @@ AudioFlinger::PlaybackThread::TimedTrack::TimedTrack( int sessionId) : Track(thread, client, streamType, sampleRate, format, channelMask, frameCount, sharedBuffer, sessionId), + mQueueHeadInFlight(false), + mTrimQueueHeadOnRelease(false), mTimedSilenceBuffer(NULL), mTimedSilenceBufferSize(0), mTimedAudioOutputOnTime(false), @@ -3855,6 +3857,13 @@ AudioFlinger::PlaybackThread::TimedTrack::TimedTrack( mLocalTimeToSampleTransform.a_to_b_denom = mLocalTimeFreq; LinearTransform::reduce(&mLocalTimeToSampleTransform.a_to_b_numer, &mLocalTimeToSampleTransform.a_to_b_denom); + + mMediaTimeToSampleTransform.a_zero = 0; + mMediaTimeToSampleTransform.b_zero = 0; + mMediaTimeToSampleTransform.a_to_b_numer = sampleRate; + mMediaTimeToSampleTransform.a_to_b_denom = 1000000; + LinearTransform::reduce(&mMediaTimeToSampleTransform.a_to_b_numer, + &mMediaTimeToSampleTransform.a_to_b_denom); } AudioFlinger::PlaybackThread::TimedTrack::~TimedTrack() { @@ -3914,12 +3923,35 @@ void AudioFlinger::PlaybackThread::TimedTrack::trimTimedBufferQueue_l() { size_t trimIndex; for (trimIndex = 0; trimIndex < mTimedBufferQueue.size(); trimIndex++) { - if (mTimedBufferQueue[trimIndex].pts() > mediaTimeNow) + int64_t frameCount = mTimedBufferQueue[trimIndex].buffer()->size() + / mCblk->frameSize; + int64_t bufEnd; + + if (!mMediaTimeToSampleTransform.doReverseTransform(frameCount, + &bufEnd)) { + LOGE("Failed to convert frame count of %lld to media time duration" + " (scale factor %d/%u) in %s", frameCount, + mMediaTimeToSampleTransform.a_to_b_numer, + mMediaTimeToSampleTransform.a_to_b_denom, + __PRETTY_FUNCTION__); + break; + } + bufEnd += mTimedBufferQueue[trimIndex].pts(); + + if (bufEnd > mediaTimeNow) break; + + // Is the buffer we want to use in the middle of a mix operation right + // now? If so, don't actually trim it. Just wait for the releaseBuffer + // from the mixer which should be coming back shortly. + if (!trimIndex && mQueueHeadInFlight) { + mTrimQueueHeadOnRelease = true; + } } - if (trimIndex) { - mTimedBufferQueue.removeItemsAt(0, trimIndex); + size_t trimStart = mTrimQueueHeadOnRelease ? 1 : 0; + if (trimStart < trimIndex) { + mTimedBufferQueue.removeItemsAt(trimStart, trimIndex); } } @@ -3982,6 +4014,9 @@ status_t AudioFlinger::PlaybackThread::TimedTrack::getNextBuffer( Mutex::Autolock _l(mTimedBufferQueueLock); + LOG_ASSERT(!mQueueHeadInFlight, + "getNextBuffer called without releaseBuffer!"); + while (true) { // if we have no timed buffers, then fail @@ -4003,7 +4038,7 @@ status_t AudioFlinger::PlaybackThread::TimedTrack::getNextBuffer( if (mMediaTimeTransform.a_to_b_denom == 0) { // the transform represents a pause, so yield silence - timedYieldSilence(buffer->frameCount, buffer); + timedYieldSilence_l(buffer->frameCount, buffer); return NO_ERROR; } @@ -4073,7 +4108,7 @@ status_t AudioFlinger::PlaybackThread::TimedTrack::getNextBuffer( (!mTimedAudioOutputOnTime && llabs(sampleDelta) <= kSampleStartupThreshold)) { // the next input is close enough to being on time, so concatenate it // with the last output - timedYieldSamples(buffer); + timedYieldSamples_l(buffer); LOGV("*** on time: head.pos=%d frameCount=%u", head.position(), buffer->frameCount); return NO_ERROR; @@ -4082,7 +4117,7 @@ status_t AudioFlinger::PlaybackThread::TimedTrack::getNextBuffer( // the next input sample is too big, so fill it with silence uint32_t framesUntilNextInput = (sampleDelta + 0x80000000) >> 32; - timedYieldSilence(framesUntilNextInput, buffer); + timedYieldSilence_l(framesUntilNextInput, buffer); LOGV("*** silence: frameCount=%u", buffer->frameCount); return NO_ERROR; } else { @@ -4102,7 +4137,7 @@ status_t AudioFlinger::PlaybackThread::TimedTrack::getNextBuffer( head.setPosition(onTimeSamplePosition); // yield the available samples - timedYieldSamples(buffer); + timedYieldSamples_l(buffer); LOGV("*** late: head.pos=%d frameCount=%u", head.position(), buffer->frameCount); return NO_ERROR; @@ -4115,7 +4150,7 @@ status_t AudioFlinger::PlaybackThread::TimedTrack::getNextBuffer( // buffer's capacity. // // Caller must hold mTimedBufferQueueLock -void AudioFlinger::PlaybackThread::TimedTrack::timedYieldSamples( +void AudioFlinger::PlaybackThread::TimedTrack::timedYieldSamples_l( AudioBufferProvider::Buffer* buffer) { const TimedBuffer& head = mTimedBufferQueue[0]; @@ -4128,13 +4163,14 @@ void AudioFlinger::PlaybackThread::TimedTrack::timedYieldSamples( size_t framesRequested = buffer->frameCount; buffer->frameCount = min(framesLeftInHead, framesRequested); + mQueueHeadInFlight = true; mTimedAudioOutputOnTime = true; } // Yield samples of silence up to the given output buffer's capacity // // Caller must hold mTimedBufferQueueLock -void AudioFlinger::PlaybackThread::TimedTrack::timedYieldSilence( +void AudioFlinger::PlaybackThread::TimedTrack::timedYieldSilence_l( uint32_t numFrames, AudioBufferProvider::Buffer* buffer) { // lazily allocate a buffer filled with silence @@ -4163,21 +4199,44 @@ void AudioFlinger::PlaybackThread::TimedTrack::releaseBuffer( // queue, its either because the buffer is part of the silence buffer, or // because the head of the timed queue was trimmed after the mixer called // getNextBuffer but before the mixer called releaseBuffer. - if ((buffer->raw != mTimedSilenceBuffer) && mTimedBufferQueue.size()) { + if (buffer->raw == mTimedSilenceBuffer) { + LOG_ASSERT(!mQueueHeadInFlight, + "Queue head in flight during release of silence buffer!"); + goto done; + } + + LOG_ASSERT(mQueueHeadInFlight, + "TimedTrack::releaseBuffer of non-silence buffer, but no queue" + " head in flight."); + + if (mTimedBufferQueue.size()) { TimedBuffer& head = mTimedBufferQueue.editItemAt(0); void* start = head.buffer()->pointer(); - void* end = head.buffer()->pointer() + head.buffer()->size(); + void* end = reinterpret_cast( + reinterpret_cast(head.buffer()->pointer()) + + head.buffer()->size()); - if ((buffer->raw >= start) && (buffer->raw <= end)) { - head.setPosition(head.position() + - (buffer->frameCount * mCblk->frameSize)); - if (static_cast(head.position()) >= head.buffer()->size()) { - mTimedBufferQueue.removeAt(0); - } + LOG_ASSERT((buffer->raw >= start) && (buffer->raw < end), + "released buffer not within the head of the timed buffer" + " queue; qHead = [%p, %p], released buffer = %p", + start, end, buffer->raw); + + head.setPosition(head.position() + + (buffer->frameCount * mCblk->frameSize)); + mQueueHeadInFlight = false; + + if ((static_cast(head.position()) >= head.buffer()->size()) + || mTrimQueueHeadOnRelease) { + mTimedBufferQueue.removeAt(0); + mTrimQueueHeadOnRelease = false; } + } else { + LOG_FATAL("TimedTrack::releaseBuffer of non-silence buffer with no" + " buffers in the timed buffer queue"); } +done: buffer->raw = 0; buffer->frameCount = 0; } diff --git a/services/audioflinger/AudioFlinger.h b/services/audioflinger/AudioFlinger.h index 71870a2a912e..0eed5f997e58 100644 --- a/services/audioflinger/AudioFlinger.h +++ b/services/audioflinger/AudioFlinger.h @@ -712,9 +712,9 @@ private: virtual status_t getNextBuffer(AudioBufferProvider::Buffer* buffer, int64_t pts); virtual void releaseBuffer(AudioBufferProvider::Buffer* buffer); - void timedYieldSamples(AudioBufferProvider::Buffer* buffer); - void timedYieldSilence(uint32_t numFrames, - AudioBufferProvider::Buffer* buffer); + void timedYieldSamples_l(AudioBufferProvider::Buffer* buffer); + void timedYieldSilence_l(uint32_t numFrames, + AudioBufferProvider::Buffer* buffer); status_t allocateTimedBuffer(size_t size, sp* buffer); @@ -737,8 +737,13 @@ private: uint64_t mLocalTimeFreq; LinearTransform mLocalTimeToSampleTransform; + LinearTransform mMediaTimeToSampleTransform; sp mTimedMemoryDealer; + Vector mTimedBufferQueue; + bool mQueueHeadInFlight; + bool mTrimQueueHeadOnRelease; + uint8_t* mTimedSilenceBuffer; uint32_t mTimedSilenceBufferSize; mutable Mutex mTimedBufferQueueLock; -- 2.11.0