From c852ca3c965e1931489b6c066a638d27022649c9 Mon Sep 17 00:00:00 2001 From: Dichen Zhang Date: Thu, 13 Jun 2019 11:32:07 -0700 Subject: [PATCH 1/4] fix bug: release mDisconnectLock when early terminate Bug: 134995545 Tests: (1) described in buganizer comment #2 (2) atest CtsCameraTestCases:android.hardware.camera2.cts.FastBasicsTest#testCamera1 Change-Id: Ie134e503cd7602a754b57bcc5c1355dea19d4eab (cherry picked from commit c4270cc3a9287d72163cacd0cadfa91523764855) --- media/libmediaplayerservice/nuplayer/GenericSource.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/media/libmediaplayerservice/nuplayer/GenericSource.cpp b/media/libmediaplayerservice/nuplayer/GenericSource.cpp index 5c77e41332..0f72c0dd80 100644 --- a/media/libmediaplayerservice/nuplayer/GenericSource.cpp +++ b/media/libmediaplayerservice/nuplayer/GenericSource.cpp @@ -389,7 +389,6 @@ void NuPlayer::GenericSource::onPrepareAsync() { if (httpSource == NULL) { ALOGE("Failed to create http source!"); notifyPreparedAndCleanup(UNKNOWN_ERROR); - mDisconnectLock.lock(); return; } mDisconnectLock.lock(); @@ -449,6 +448,7 @@ void NuPlayer::GenericSource::onPrepareAsync() { if (mDataSource == NULL) { ALOGE("Failed to create data source!"); + mDisconnectLock.unlock(); notifyPreparedAndCleanup(UNKNOWN_ERROR); return; } From 4763c183fce28e9935a1389aefdb8cd6b877cba8 Mon Sep 17 00:00:00 2001 From: Jayant Chowdhary Date: Thu, 20 Jun 2019 20:30:07 -0700 Subject: [PATCH 2/4] libcamera2ndk_vendor: stop looper thread on ~ACameraDevice() Bug: 135641415 Test: enroll; while(1) auth; Change-Id: I59c522a0e8827c5990926f0cf7c7960e1cea2e5e Signed-off-by: Jayant Chowdhary (cherry picked from commit b233eaef6a0b9a0b38845de252322c5e7418cdcf) --- camera/ndk/ndk_vendor/impl/ACameraDevice.cpp | 10 ++++++++++ camera/ndk/ndk_vendor/impl/ACameraDevice.h | 4 ++-- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/camera/ndk/ndk_vendor/impl/ACameraDevice.cpp b/camera/ndk/ndk_vendor/impl/ACameraDevice.cpp index a43d707e8e..8a3bb46711 100644 --- a/camera/ndk/ndk_vendor/impl/ACameraDevice.cpp +++ b/camera/ndk/ndk_vendor/impl/ACameraDevice.cpp @@ -44,6 +44,16 @@ using namespace android; +ACameraDevice::~ACameraDevice() { + Mutex::Autolock _l(mDevice->mDeviceLock); + if (mDevice->mCbLooper != nullptr) { + mDevice->mCbLooper->unregisterHandler(mDevice->mHandler->id()); + mDevice->mCbLooper->stop(); + } + mDevice->mCbLooper.clear(); + mDevice->mHandler.clear(); +} + namespace android { namespace acam { diff --git a/camera/ndk/ndk_vendor/impl/ACameraDevice.h b/camera/ndk/ndk_vendor/impl/ACameraDevice.h index 829b08452e..d8df5684a7 100644 --- a/camera/ndk/ndk_vendor/impl/ACameraDevice.h +++ b/camera/ndk/ndk_vendor/impl/ACameraDevice.h @@ -135,6 +135,7 @@ class CameraDevice final : public RefBase { private: friend ACameraCaptureSession; + friend ACameraDevice; camera_status_t checkCameraClosedOrErrorLocked() const; @@ -383,8 +384,7 @@ struct ACameraDevice { sp chars) : mDevice(new android::acam::CameraDevice(id, cb, std::move(chars), this)) {} - ~ACameraDevice() {}; - + ~ACameraDevice(); /******************* * NDK public APIs * *******************/ From 3f87440cd3e9087a3e2537813f0d47cc7d2de633 Mon Sep 17 00:00:00 2001 From: Wonsik Kim Date: Mon, 1 Jul 2019 12:00:07 -0700 Subject: [PATCH 3/4] codec2: initialize delays at start Bug: 136446346 Test: bug repro steps Change-Id: Id5811411404625ca0316ebbdd75e075ca7fd42c3 (cherry picked from commit bdffeadde6b279391001bbf8a61d3c86d2b31ea2) --- media/codec2/sfplugin/CCodecBufferChannel.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/media/codec2/sfplugin/CCodecBufferChannel.cpp b/media/codec2/sfplugin/CCodecBufferChannel.cpp index 9778498be1..09049b9e84 100644 --- a/media/codec2/sfplugin/CCodecBufferChannel.cpp +++ b/media/codec2/sfplugin/CCodecBufferChannel.cpp @@ -886,6 +886,8 @@ status_t CCodecBufferChannel::start( bool forceArrayMode = false; Mutexed::Locked input(mInput); + input->inputDelay = inputDelayValue; + input->pipelineDelay = pipelineDelayValue; input->numSlots = numInputSlots; input->extraBuffers.flush(); input->numExtraSlots = 0u; @@ -1052,6 +1054,7 @@ status_t CCodecBufferChannel::start( } Mutexed::Locked output(mOutput); + output->outputDelay = outputDelayValue; output->numSlots = numOutputSlots; if (graphic) { if (outputSurface) { From ccc32cb95525c87461b388811e442120deef98eb Mon Sep 17 00:00:00 2001 From: Sungtak Lee Date: Tue, 20 Aug 2019 18:07:54 -0700 Subject: [PATCH 4/4] bufferpool2: handle transfer to closed connection Handle transfer to closed connection properly. Also avoid spinlocking during invalidation. Bug: 139506073 Test: atest CtsMediaTestCases -- --module-arg CtsMediaTestCases:size:small Merged-In: I9037ab0bde48a51b2b97158ecd0086dac84f8b26 Change-Id: I9037ab0bde48a51b2b97158ecd0086dac84f8b26 --- media/bufferpool/1.0/AccessorImpl.cpp | 9 +++++++- media/bufferpool/1.0/AccessorImpl.h | 1 + media/bufferpool/2.0/AccessorImpl.cpp | 32 ++++++++++++++++++++++++--- media/bufferpool/2.0/AccessorImpl.h | 1 + 4 files changed, 39 insertions(+), 4 deletions(-) diff --git a/media/bufferpool/1.0/AccessorImpl.cpp b/media/bufferpool/1.0/AccessorImpl.cpp index fa17f15f8c..a5366f62fc 100644 --- a/media/bufferpool/1.0/AccessorImpl.cpp +++ b/media/bufferpool/1.0/AccessorImpl.cpp @@ -151,6 +151,7 @@ ResultStatus Accessor::Impl::connect( newConnection->initialize(accessor, id); *connection = newConnection; *pConnectionId = id; + mBufferPool.mConnectionIds.insert(id); ++sSeqId; } } @@ -305,7 +306,12 @@ bool Accessor::Impl::BufferPool::handleTransferTo(const BufferStatusMessage &mes found->second->mSenderValidated = true; return true; } - // TODO: verify there is target connection Id + if (mConnectionIds.find(message.targetConnectionId) == mConnectionIds.end()) { + // N.B: it could be fake or receive connection already closed. + ALOGD("bufferpool %p receiver connection %lld is no longer valid", + this, (long long)message.targetConnectionId); + return false; + } mStats.onBufferSent(); mTransactions.insert(std::make_pair( message.transactionId, @@ -450,6 +456,7 @@ bool Accessor::Impl::BufferPool::handleClose(ConnectionId connectionId) { } } } + mConnectionIds.erase(connectionId); return true; } diff --git a/media/bufferpool/1.0/AccessorImpl.h b/media/bufferpool/1.0/AccessorImpl.h index c04dbf3687..84cb6854b0 100644 --- a/media/bufferpool/1.0/AccessorImpl.h +++ b/media/bufferpool/1.0/AccessorImpl.h @@ -94,6 +94,7 @@ private: std::map> mBuffers; std::set mFreeBuffers; + std::set mConnectionIds; /// Buffer pool statistics which tracks allocation and transfer statistics. struct Stats { diff --git a/media/bufferpool/2.0/AccessorImpl.cpp b/media/bufferpool/2.0/AccessorImpl.cpp index 94cf006952..cacd465706 100644 --- a/media/bufferpool/2.0/AccessorImpl.cpp +++ b/media/bufferpool/2.0/AccessorImpl.cpp @@ -163,6 +163,7 @@ ResultStatus Accessor::Impl::connect( *connection = newConnection; *pConnectionId = id; *pMsgId = mBufferPool.mInvalidation.mInvalidationId; + mBufferPool.mConnectionIds.insert(id); mBufferPool.mInvalidationChannel.getDesc(invDescPtr); mBufferPool.mInvalidation.onConnect(id, observer); ++sSeqId; @@ -474,7 +475,12 @@ bool Accessor::Impl::BufferPool::handleTransferTo(const BufferStatusMessage &mes found->second->mSenderValidated = true; return true; } - // TODO: verify there is target connection Id + if (mConnectionIds.find(message.targetConnectionId) == mConnectionIds.end()) { + // N.B: it could be fake or receive connection already closed. + ALOGD("bufferpool2 %p receiver connection %lld is no longer valid", + this, (long long)message.targetConnectionId); + return false; + } mStats.onBufferSent(); mTransactions.insert(std::make_pair( message.transactionId, @@ -644,6 +650,7 @@ bool Accessor::Impl::BufferPool::handleClose(ConnectionId connectionId) { } } } + mConnectionIds.erase(connectionId); return true; } @@ -774,11 +781,19 @@ void Accessor::Impl::invalidatorThread( std::mutex &mutex, std::condition_variable &cv, bool &ready) { + constexpr uint32_t NUM_SPIN_TO_INCREASE_SLEEP = 1024; + constexpr uint32_t NUM_SPIN_TO_LOG = 1024*8; + constexpr useconds_t MAX_SLEEP_US = 10000; + uint32_t numSpin = 0; + useconds_t sleepUs = 1; + while(true) { std::map> copied; { std::unique_lock lock(mutex); if (!ready) { + numSpin = 0; + sleepUs = 1; cv.wait(lock); } copied.insert(accessors.begin(), accessors.end()); @@ -800,9 +815,20 @@ void Accessor::Impl::invalidatorThread( if (accessors.size() == 0) { ready = false; } else { - // prevent draining cpu. + // TODO Use an efficient way to wait over FMQ. + // N.B. Since there is not a efficient way to wait over FMQ, + // polling over the FMQ is the current way to prevent draining + // CPU. lock.unlock(); - std::this_thread::yield(); + ++numSpin; + if (numSpin % NUM_SPIN_TO_INCREASE_SLEEP == 0 && + sleepUs < MAX_SLEEP_US) { + sleepUs *= 10; + } + if (numSpin % NUM_SPIN_TO_LOG == 0) { + ALOGW("invalidator thread spinning"); + } + ::usleep(sleepUs); } } } diff --git a/media/bufferpool/2.0/AccessorImpl.h b/media/bufferpool/2.0/AccessorImpl.h index eea72b98fb..807e0f1444 100644 --- a/media/bufferpool/2.0/AccessorImpl.h +++ b/media/bufferpool/2.0/AccessorImpl.h @@ -111,6 +111,7 @@ private: std::map> mBuffers; std::set mFreeBuffers; + std::set mConnectionIds; struct Invalidation { static std::atomic sInvSeqId;