From 84afa3b51ac48f84ed62489529ce78cba7fca00e Mon Sep 17 00:00:00 2001 From: Glenn Kasten Date: Wed, 25 Jan 2012 15:28:08 -0800 Subject: [PATCH] Constructor initialization and const fields In constructors, initialize member fields in the initialization list rather than constructor body where possible. This allows more fields to be const, provided they are never modified. Also initialize POD fields in constructor, unless it's obvious they don't need to be initialized. In that case, put a comment instead. Remove explicit clear() in destructors on fields that are now const. Give AudioSessionRef a default constructor, so it's immutable fields can be marked const. Add comment about ~TrackBase() trick. Initialize fields in declaration order to make it easier to confirm that all fields are set. Move initialization of mHardwareStatus from onFirstRef() to constructor. Use NULL not 0 to initialize raw pointers in initialization list. Rename field mClient to mAudioFlingerClient, and getter from client() to audioFlingerClient(). Change-Id: Ib36cf6ed32f3cd19003f40a5d84046eb4c122052 --- services/audioflinger/AudioFlinger.cpp | 70 ++++++++++++++++++++-------------- services/audioflinger/AudioFlinger.h | 47 ++++++++++++----------- services/audioflinger/AudioMixer.cpp | 8 ++-- 3 files changed, 72 insertions(+), 53 deletions(-) diff --git a/services/audioflinger/AudioFlinger.cpp b/services/audioflinger/AudioFlinger.cpp index a43afacff0..7d87d92c24 100644 --- a/services/audioflinger/AudioFlinger.cpp +++ b/services/audioflinger/AudioFlinger.cpp @@ -160,7 +160,10 @@ static const char * const audio_interfaces[] = { AudioFlinger::AudioFlinger() : BnAudioFlinger(), - mPrimaryHardwareDev(NULL), mMasterVolume(1.0f), mMasterMute(false), mNextUniqueId(1), + mPrimaryHardwareDev(NULL), + mHardwareStatus(AUDIO_HW_IDLE), // see also onFirstRef() + mMasterVolume(1.0f), mMasterMute(false), mNextUniqueId(1), + mMode(AUDIO_MODE_INVALID), mBtNrecIsOff(false) { } @@ -172,7 +175,6 @@ void AudioFlinger::onFirstRef() Mutex::Autolock _l(mLock); /* TODO: move all this work into an Init() function */ - mHardwareStatus = AUDIO_HW_IDLE; for (size_t i = 0; i < ARRAY_SIZE(audio_interfaces); i++) { const hw_module_t *mod; @@ -971,7 +973,8 @@ void AudioFlinger::audioConfigChanged_l(int event, int ioHandle, void *param2) { size_t size = mNotificationClients.size(); for (size_t i = 0; i < size; i++) { - mNotificationClients.valueAt(i)->client()->ioConfigChanged(event, ioHandle, param2); + mNotificationClients.valueAt(i)->audioFlingerClient()->ioConfigChanged(event, ioHandle, + param2); } } @@ -989,11 +992,15 @@ AudioFlinger::ThreadBase::ThreadBase(const sp& audioFlinger, int i type_t type) : Thread(false), mType(type), - mAudioFlinger(audioFlinger), mSampleRate(0), mFrameCount(0), mChannelCount(0), - mFrameSize(1), mFormat(AUDIO_FORMAT_INVALID), mStandby(false), mId(id), mExiting(false), - mDevice(device) + mAudioFlinger(audioFlinger), mSampleRate(0), mFrameCount(0), + // mChannelMask + mChannelCount(0), + mFrameSize(1), mFormat(AUDIO_FORMAT_INVALID), + mParamStatus(NO_ERROR), + mStandby(false), mId(id), mExiting(false), + mDevice(device), + mDeathRecipient(new PMDeathRecipient(this)) { - mDeathRecipient = new PMDeathRecipient(this); } AudioFlinger::ThreadBase::~ThreadBase() @@ -1377,18 +1384,21 @@ AudioFlinger::PlaybackThread::PlaybackThread(const sp& audioFlinge uint32_t device, type_t type) : ThreadBase(audioFlinger, id, device, type), - mMixBuffer(NULL), mSuspended(0), mBytesWritten(0), mOutput(output), + mMixBuffer(NULL), mSuspended(0), mBytesWritten(0), + // Assumes constructor is called by AudioFlinger with it's mLock held, + // but it would be safer to explicitly pass initial masterMute as parameter + mMasterMute(audioFlinger->masterMute_l()), + // mStreamTypes[] initialized in constructor body + mOutput(output), + // Assumes constructor is called by AudioFlinger with it's mLock held, + // but it would be safer to explicitly pass initial masterVolume as parameter + mMasterVolume(audioFlinger->masterVolume_l()), mLastWriteTime(0), mNumWrites(0), mNumDelayedWrites(0), mInWrite(false) { snprintf(mName, kNameLength, "AudioOut_%d", id); readOutputParameters(); - // Assumes constructor is called by AudioFlinger with it's mLock held, - // but it would be safer to explicitly pass these as parameters - mMasterVolume = mAudioFlinger->masterVolume_l(); - mMasterMute = mAudioFlinger->masterMute_l(); - // mStreamTypes[AUDIO_STREAM_CNT] is initialized by stream_type_t default constructor // There is no AUDIO_STREAM_MIN, and ++ operator does not compile for (audio_stream_type_t stream = (audio_stream_type_t) 0; stream < AUDIO_STREAM_CNT; @@ -1851,10 +1861,9 @@ uint32_t AudioFlinger::PlaybackThread::activeSleepTimeUs() AudioFlinger::MixerThread::MixerThread(const sp& audioFlinger, AudioStreamOut* output, int id, uint32_t device, type_t type) : PlaybackThread(audioFlinger, output, id, device, type), - mAudioMixer(NULL), mPrevMixerStatus(MIXER_IDLE) + mAudioMixer(new AudioMixer(mFrameCount, mSampleRate)), + mPrevMixerStatus(MIXER_IDLE) { - mAudioMixer = new AudioMixer(mFrameCount, mSampleRate); - // FIXME - Current mixer implementation only supports stereo output if (mChannelCount == 1) { ALOGE("Invalid audio hardware channel count"); @@ -2519,6 +2528,8 @@ uint32_t AudioFlinger::MixerThread::suspendSleepTimeUs() // ---------------------------------------------------------------------------- AudioFlinger::DirectOutputThread::DirectOutputThread(const sp& audioFlinger, AudioStreamOut* output, int id, uint32_t device) : PlaybackThread(audioFlinger, output, id, device, DIRECT) + // mLeftVolFloat, mRightVolFloat + // mLeftVolShort, mRightVolShort { } @@ -3249,13 +3260,17 @@ AudioFlinger::ThreadBase::TrackBase::TrackBase( : RefBase(), mThread(thread), mClient(client), - mCblk(0), + mCblk(NULL), + // mBuffer + // mBufferEnd mFrameCount(0), mState(IDLE), mClientTid(-1), mFormat(format), mFlags(flags & ~SYSTEM_FLAGS_MASK), mSessionId(sessionId) + // mChannelCount + // mChannelMask { ALOGV_IF(sharedBuffer != 0, "sharedBuffer: %p, size: %d", sharedBuffer->pointer(), sharedBuffer->size()); @@ -3322,6 +3337,7 @@ AudioFlinger::ThreadBase::TrackBase::~TrackBase() } mCblkMemory.clear(); // and free the shared memory if (mClient != NULL) { + // Client destructor must run with AudioFlinger mutex locked Mutex::Autolock _l(mClient->audioFlinger()->mLock); mClient.clear(); } @@ -4079,13 +4095,12 @@ sp AudioFlinger::Client::heap() const AudioFlinger::NotificationClient::NotificationClient(const sp& audioFlinger, const sp& client, pid_t pid) - : mAudioFlinger(audioFlinger), mPid(pid), mClient(client) + : mAudioFlinger(audioFlinger), mPid(pid), mAudioFlingerClient(client) { } AudioFlinger::NotificationClient::~NotificationClient() { - mClient.clear(); } void AudioFlinger::NotificationClient::binderDied(const wp& who) @@ -4271,12 +4286,15 @@ AudioFlinger::RecordThread::RecordThread(const sp& audioFlinger, int id, uint32_t device) : ThreadBase(audioFlinger, id, device, RECORD), - mInput(input), mTrack(NULL), mResampler(NULL), mRsmpOutBuffer(NULL), mRsmpInBuffer(NULL) + mInput(input), mTrack(NULL), mResampler(NULL), mRsmpOutBuffer(NULL), mRsmpInBuffer(NULL), + // mRsmpInIndex and mInputBytes set by readInputParameters() + mReqChannelCount(popcount(channels)), + mReqSampleRate(sampleRate) + // mBytesRead is only meaningful while active, and so is cleared in start() + // (but might be better to also clear here for dump?) { snprintf(mName, kNameLength, "AudioIn_%d", id); - mReqChannelCount = popcount(channels); - mReqSampleRate = sampleRate; readInputParameters(); } @@ -5245,12 +5263,8 @@ void AudioFlinger::acquireAudioSessionId(int audioSession) return; } } - AudioSessionRef *ref = new AudioSessionRef(); - ref->sessionid = audioSession; - ref->pid = caller; - ref->cnt = 1; - mAudioSessionRefs.push(ref); - ALOGV(" added new entry for %d", ref->sessionid); + mAudioSessionRefs.push(new AudioSessionRef(audioSession, caller)); + ALOGV(" added new entry for %d", audioSession); } void AudioFlinger::releaseAudioSessionId(int audioSession) diff --git a/services/audioflinger/AudioFlinger.h b/services/audioflinger/AudioFlinger.h index 76beeafe65..ab5ec34681 100644 --- a/services/audioflinger/AudioFlinger.h +++ b/services/audioflinger/AudioFlinger.h @@ -233,9 +233,9 @@ private: private: Client(const Client&); Client& operator = (const Client&); - sp mAudioFlinger; - sp mMemoryDealer; - pid_t mPid; + const sp mAudioFlinger; + const sp mMemoryDealer; + const pid_t mPid; }; // --- Notification Client --- @@ -246,7 +246,7 @@ private: pid_t pid); virtual ~NotificationClient(); - sp client() { return mClient; } + sp audioFlingerClient() const { return mAudioFlingerClient; } // IBinder::DeathRecipient virtual void binderDied(const wp& who); @@ -255,9 +255,9 @@ private: NotificationClient(const NotificationClient&); NotificationClient& operator = (const NotificationClient&); - sp mAudioFlinger; - pid_t mPid; - sp mClient; + const sp mAudioFlinger; + const pid_t mPid; + const sp mAudioFlingerClient; }; class TrackHandle; @@ -367,8 +367,8 @@ private: bool step(); void reset(); - wp mThread; - sp mClient; + const wp mThread; + /*const*/ sp mClient; // see explanation at ~TrackBase() why not const sp mCblkMemory; audio_track_cblk_t* mCblk; void* mBuffer; @@ -377,9 +377,9 @@ private: // we don't really need a lock for these track_state mState; int mClientTid; - audio_format_t mFormat; + const audio_format_t mFormat; uint32_t mFlags; - int mSessionId; + const int mSessionId; uint8_t mChannelCount; uint32_t mChannelMask; }; @@ -532,7 +532,7 @@ private: const type_t mType; Condition mWaitWorkCV; - sp mAudioFlinger; + const sp mAudioFlinger; uint32_t mSampleRate; size_t mFrameCount; uint32_t mChannelMask; @@ -553,7 +553,7 @@ private: char mName[kNameLength]; sp mPowerManager; sp mWakeLockToken; - sp mDeathRecipient; + const sp mDeathRecipient; // list of suspended effects per session and per type. The first vector is // keyed by session ID, the second by type UUID timeLow field KeyedVector< int, KeyedVector< int, sp > > mSuspendedSessions; @@ -671,7 +671,7 @@ private: bool write(int16_t* data, uint32_t frames); bool bufferQueueEmpty() { return (mBufferQueue.size() == 0) ? true : false; } bool isActive() { return mActive; } - wp& thread() { return mThread; } + const wp& thread() { return mThread; } private: @@ -688,7 +688,7 @@ private: Vector < Buffer* > mBufferQueue; AudioBufferProvider::Buffer mOutBuffer; bool mActive; - DuplicatingThread* mSourceThread; + DuplicatingThread* const mSourceThread; // for waitTimeMs() in write() }; // end of OutputTrack PlaybackThread (const sp& audioFlinger, AudioStreamOut* output, int id, @@ -919,7 +919,7 @@ private: virtual status_t onTransact( uint32_t code, const Parcel& data, Parcel* reply, uint32_t flags); private: - sp mTrack; + const sp mTrack; }; friend class Client; @@ -1023,8 +1023,8 @@ private: int16_t *mRsmpInBuffer; size_t mRsmpInIndex; size_t mInputBytes; - int mReqChannelCount; - uint32_t mReqSampleRate; + const int mReqChannelCount; + const uint32_t mReqSampleRate; ssize_t mBytesRead; }; @@ -1038,7 +1038,7 @@ private: virtual status_t onTransact( uint32_t code, const Parcel& data, Parcel* reply, uint32_t flags); private: - sp mRecordTrack; + const sp mRecordTrack; }; //--- Audio Effect Management @@ -1107,7 +1107,7 @@ private: int16_t *outBuffer() { return mConfig.outputCfg.buffer.s16; } void setChain(const wp& chain) { mChain = chain; } void setThread(const wp& thread) { mThread = thread; } - wp& thread() { return mThread; } + const wp& thread() { return mThread; } status_t addHandle(const sp& handle); void disconnect(const wp& handle, bool unpiniflast); @@ -1379,8 +1379,11 @@ mutable Mutex mLock; // mutex for process, commands and handl }; struct AudioSessionRef { - int sessionid; - pid_t pid; + // FIXME rename parameter names when fields get "m" prefix + AudioSessionRef(int sessionid_, pid_t pid_) : + sessionid(sessionid_), pid(pid_), cnt(1) {} + const int sessionid; + const pid_t pid; int cnt; }; diff --git a/services/audioflinger/AudioMixer.cpp b/services/audioflinger/AudioMixer.cpp index a01c6a84b2..1f7d883068 100644 --- a/services/audioflinger/AudioMixer.cpp +++ b/services/audioflinger/AudioMixer.cpp @@ -48,9 +48,10 @@ AudioMixer::AudioMixer(size_t frameCount, uint32_t sampleRate) mState.enabledTracks= 0; mState.needsChanged = 0; mState.frameCount = frameCount; + mState.hook = process__nop; mState.outputTemp = NULL; mState.resampleTemp = NULL; - mState.hook = process__nop; + // mState.reserved track_t* t = mState.tracks; for (unsigned i=0 ; i < MAX_NUM_TRACKS ; i++) { t->needs = 0; @@ -70,12 +71,13 @@ AudioMixer::AudioMixer(size_t frameCount, uint32_t sampleRate) t->enabled = 0; t->format = 16; t->channelMask = AUDIO_CHANNEL_OUT_STEREO; - t->buffer.raw = 0; t->bufferProvider = NULL; + t->buffer.raw = NULL; + // t->buffer.frameCount t->hook = NULL; + t->in = NULL; t->resampler = NULL; t->sampleRate = mSampleRate; - t->in = NULL; t->mainBuffer = NULL; t->auxBuffer = NULL; t++; -- 2.11.0