From f8197a6a9d9363cb52bb8a2c15c0e5a52064355e Mon Sep 17 00:00:00 2001 From: Glenn Kasten Date: Tue, 23 Apr 2013 12:39:37 -0700 Subject: [PATCH] Remove timing jitter during startup of audio This fixes a regression introduced recently, that increased timing jitter during the startup of the FastMixer and AudioTrack callback threads. The regression was to make requestPriority() asynchronous as a way to avoid an apparent priority inversion in system_server. This means that the target thread could run briefly with the initial priority, before the new priority takes effect. This change removes the startup jitter for FastMixer, by making the requestPriority() synchronous again for that case. It doesn't matter that this restores the priority inversion involving normal mixer thread, because it happens during startup of both threads. The change also removes the startup jitter for the AudioTrack callback thread, by having the target thread check whether the requestPriority() has completed yet. If not, the target thread blocks with a timeout until the priority boost finishes. Finally, we now log an error message if the expected priority boost doesn't happen. Bug: 8698989 Change-Id: Id590e9a274b70ec1ba85b44a585ee37a22e41cbc --- include/media/AudioTrack.h | 1 + media/libmedia/AudioTrack.cpp | 21 +++++++++++++++++++++ services/audioflinger/FastMixer.cpp | 4 ++++ services/audioflinger/ISchedulingPolicyService.cpp | 5 +++-- services/audioflinger/ISchedulingPolicyService.h | 2 +- services/audioflinger/SchedulingPolicyService.cpp | 4 ++-- services/audioflinger/SchedulingPolicyService.h | 5 ++++- services/audioflinger/Threads.cpp | 4 +++- 8 files changed, 39 insertions(+), 7 deletions(-) diff --git a/include/media/AudioTrack.h b/include/media/AudioTrack.h index db5a7abb61..64f82bbcc6 100644 --- a/include/media/AudioTrack.h +++ b/include/media/AudioTrack.h @@ -599,6 +599,7 @@ protected: int mPreviousPriority; // before start() SchedPolicy mPreviousSchedulingGroup; AudioTrackClientProxy* mProxy; + bool mAwaitBoost; // thread should wait for priority boost before running }; class TimedAudioTrack : public AudioTrack diff --git a/media/libmedia/AudioTrack.cpp b/media/libmedia/AudioTrack.cpp index 1bd839f8da..7eeb4f8d03 100644 --- a/media/libmedia/AudioTrack.cpp +++ b/media/libmedia/AudioTrack.cpp @@ -893,9 +893,11 @@ status_t AudioTrack::createTrack_l( ALOGW("Requested frameCount %u but received frameCount %u", frameCount, temp); } frameCount = temp; + mAwaitBoost = false; if (flags & AUDIO_OUTPUT_FLAG_FAST) { if (trackFlags & IAudioFlinger::TRACK_FAST) { ALOGV("AUDIO_OUTPUT_FLAG_FAST successful; frameCount %u", frameCount); + mAwaitBoost = true; } else { ALOGV("AUDIO_OUTPUT_FLAG_FAST denied by server; frameCount %u", frameCount); // once denied, do not request again if IAudioTrack is re-created @@ -1219,6 +1221,25 @@ bool AudioTrack::processAudioBuffer(const sp& thread) size_t writtenSize; mLock.lock(); + if (mAwaitBoost) { + mAwaitBoost = false; + mLock.unlock(); + static const int32_t kMaxTries = 5; + int32_t tryCounter = kMaxTries; + uint32_t pollUs = 10000; + do { + int policy = sched_getscheduler(0); + if (policy == SCHED_FIFO || policy == SCHED_RR) { + break; + } + usleep(pollUs); + pollUs <<= 1; + } while (tryCounter-- > 0); + if (tryCounter < 0) { + ALOGE("did not receive expected priority boost on time"); + } + return true; + } // acquire a strong reference on the IMemory and IAudioTrack so that they cannot be destroyed // while we are accessing the cblk sp audioTrack = mAudioTrack; diff --git a/services/audioflinger/FastMixer.cpp b/services/audioflinger/FastMixer.cpp index 24a6dfe516..21df1d7666 100644 --- a/services/audioflinger/FastMixer.cpp +++ b/services/audioflinger/FastMixer.cpp @@ -170,6 +170,10 @@ bool FastMixer::threadLoop() if (old <= 0) { __futex_syscall4(coldFutexAddr, FUTEX_WAIT_PRIVATE, old - 1, NULL); } + int policy = sched_getscheduler(0); + if (!(policy == SCHED_FIFO || policy == SCHED_RR)) { + ALOGE("did not receive expected priority boost"); + } // This may be overly conservative; there could be times that the normal mixer // requests such a brief cold idle that it doesn't require resetting this flag. isWarm = false; diff --git a/services/audioflinger/ISchedulingPolicyService.cpp b/services/audioflinger/ISchedulingPolicyService.cpp index 218aa6b070..00799680f4 100644 --- a/services/audioflinger/ISchedulingPolicyService.cpp +++ b/services/audioflinger/ISchedulingPolicyService.cpp @@ -37,14 +37,15 @@ public: { } - virtual int requestPriority(int32_t pid, int32_t tid, int32_t prio) + virtual int requestPriority(int32_t pid, int32_t tid, int32_t prio, bool asynchronous) { Parcel data, reply; data.writeInterfaceToken(ISchedulingPolicyService::getInterfaceDescriptor()); data.writeInt32(pid); data.writeInt32(tid); data.writeInt32(prio); - remote()->transact(REQUEST_PRIORITY_TRANSACTION, data, &reply, IBinder::FLAG_ONEWAY); + uint32_t flags = asynchronous ? IBinder::FLAG_ONEWAY : 0; + remote()->transact(REQUEST_PRIORITY_TRANSACTION, data, &reply, flags); // fail on exception if (reply.readExceptionCode() != 0) return -1; return reply.readInt32(); diff --git a/services/audioflinger/ISchedulingPolicyService.h b/services/audioflinger/ISchedulingPolicyService.h index a38e67e238..b94b191d99 100644 --- a/services/audioflinger/ISchedulingPolicyService.h +++ b/services/audioflinger/ISchedulingPolicyService.h @@ -27,7 +27,7 @@ public: DECLARE_META_INTERFACE(SchedulingPolicyService); virtual int requestPriority(/*pid_t*/int32_t pid, /*pid_t*/int32_t tid, - int32_t prio) = 0; + int32_t prio, bool asynchronous) = 0; }; diff --git a/services/audioflinger/SchedulingPolicyService.cpp b/services/audioflinger/SchedulingPolicyService.cpp index 59cc99ae42..36e62e9972 100644 --- a/services/audioflinger/SchedulingPolicyService.cpp +++ b/services/audioflinger/SchedulingPolicyService.cpp @@ -25,7 +25,7 @@ static sp sSchedulingPolicyService; static const String16 _scheduling_policy("scheduling_policy"); static Mutex sMutex; -int requestPriority(pid_t pid, pid_t tid, int32_t prio) +int requestPriority(pid_t pid, pid_t tid, int32_t prio, bool asynchronous) { // FIXME merge duplicated code related to service lookup, caching, and error recovery sp sps; @@ -46,7 +46,7 @@ int requestPriority(pid_t pid, pid_t tid, int32_t prio) } sleep(1); } - return sps->requestPriority(pid, tid, prio); + return sps->requestPriority(pid, tid, prio, asynchronous); } } // namespace android diff --git a/services/audioflinger/SchedulingPolicyService.h b/services/audioflinger/SchedulingPolicyService.h index 7ac84545e1..a9870d4233 100644 --- a/services/audioflinger/SchedulingPolicyService.h +++ b/services/audioflinger/SchedulingPolicyService.h @@ -21,7 +21,10 @@ namespace android { // Request elevated priority for thread tid, whose thread group leader must be pid. // The priority parameter is currently restricted to either 1 or 2. -int requestPriority(pid_t pid, pid_t tid, int32_t prio); +// The asynchronous parameter should be 'true' to return immediately, +// after the request is enqueued but not necessarily executed. +// The default value 'false' means to return after request has been enqueued and executed. +int requestPriority(pid_t pid, pid_t tid, int32_t prio, bool asynchronous = false); } // namespace android diff --git a/services/audioflinger/Threads.cpp b/services/audioflinger/Threads.cpp index 47ca100e4d..539bb4fb70 100644 --- a/services/audioflinger/Threads.cpp +++ b/services/audioflinger/Threads.cpp @@ -373,7 +373,9 @@ void AudioFlinger::ThreadBase::processConfigEvents() switch(event->type()) { case CFG_EVENT_PRIO: { PrioConfigEvent *prioEvent = static_cast(event); - int err = requestPriority(prioEvent->pid(), prioEvent->tid(), prioEvent->prio()); + // FIXME Need to understand why this has be done asynchronously + int err = requestPriority(prioEvent->pid(), prioEvent->tid(), prioEvent->prio(), + true /*asynchronous*/); if (err != 0) { ALOGW("Policy SCHED_FIFO priority %d is unavailable for pid %d tid %d; " "error %d", -- 2.11.0