diff --git a/camera/ndk/impl/ACameraDevice.cpp b/camera/ndk/impl/ACameraDevice.cpp index d24cb814a3..46a8dae317 100644 --- a/camera/ndk/impl/ACameraDevice.cpp +++ b/camera/ndk/impl/ACameraDevice.cpp @@ -29,7 +29,7 @@ #include "ACameraCaptureSession.inc" ACameraDevice::~ACameraDevice() { - mDevice->stopLooper(); + mDevice->stopLooperAndDisconnect(); } namespace android { @@ -112,19 +112,7 @@ CameraDevice::CameraDevice( } } -// Device close implementaiton -CameraDevice::~CameraDevice() { - sp session = mCurrentSession.promote(); - { - Mutex::Autolock _l(mDeviceLock); - if (!isClosed()) { - disconnectLocked(session); - } - LOG_ALWAYS_FATAL_IF(mCbLooper != nullptr, - "CameraDevice looper should've been stopped before ~CameraDevice"); - mCurrentSession = nullptr; - } -} +CameraDevice::~CameraDevice() { } void CameraDevice::postSessionMsgAndCleanup(sp& msg) { @@ -892,8 +880,14 @@ CameraDevice::onCaptureErrorLocked( return; } -void CameraDevice::stopLooper() { +void CameraDevice::stopLooperAndDisconnect() { Mutex::Autolock _l(mDeviceLock); + sp session = mCurrentSession.promote(); + if (!isClosed()) { + disconnectLocked(session); + } + mCurrentSession = nullptr; + if (mCbLooper != nullptr) { mCbLooper->unregisterHandler(mHandler->id()); mCbLooper->stop(); diff --git a/camera/ndk/impl/ACameraDevice.h b/camera/ndk/impl/ACameraDevice.h index 7a35bf0e81..6c2ceb300e 100644 --- a/camera/ndk/impl/ACameraDevice.h +++ b/camera/ndk/impl/ACameraDevice.h @@ -40,6 +40,7 @@ #include #include + #include "ACameraMetadata.h" namespace android { @@ -110,7 +111,7 @@ class CameraDevice final : public RefBase { inline ACameraDevice* getWrapper() const { return mWrapper; }; // Stop the looper thread and unregister the handler - void stopLooper(); + void stopLooperAndDisconnect(); private: friend ACameraCaptureSession; diff --git a/camera/ndk/ndk_vendor/impl/ACameraDevice.cpp b/camera/ndk/ndk_vendor/impl/ACameraDevice.cpp index 35c83551c2..e511a3f81d 100644 --- a/camera/ndk/ndk_vendor/impl/ACameraDevice.cpp +++ b/camera/ndk/ndk_vendor/impl/ACameraDevice.cpp @@ -45,7 +45,7 @@ using namespace android; ACameraDevice::~ACameraDevice() { - mDevice->stopLooper(); + mDevice->stopLooperAndDisconnect(); } namespace android { @@ -125,19 +125,7 @@ CameraDevice::CameraDevice( } } -// Device close implementaiton -CameraDevice::~CameraDevice() { - sp session = mCurrentSession.promote(); - { - Mutex::Autolock _l(mDeviceLock); - if (!isClosed()) { - disconnectLocked(session); - } - mCurrentSession = nullptr; - LOG_ALWAYS_FATAL_IF(mCbLooper != nullptr, - "CameraDevice looper should've been stopped before ~CameraDevice"); - } -} +CameraDevice::~CameraDevice() { } void CameraDevice::postSessionMsgAndCleanup(sp& msg) { @@ -1388,6 +1376,7 @@ CameraDevice::checkAndFireSequenceCompleteLocked() { // before cbh goes out of scope and causing we call the session // destructor while holding device lock cbh.mSession.clear(); + postSessionMsgAndCleanup(msg); } @@ -1400,8 +1389,13 @@ CameraDevice::checkAndFireSequenceCompleteLocked() { } } -void CameraDevice::stopLooper() { +void CameraDevice::stopLooperAndDisconnect() { Mutex::Autolock _l(mDeviceLock); + sp session = mCurrentSession.promote(); + if (!isClosed()) { + disconnectLocked(session); + } + mCurrentSession = nullptr; if (mCbLooper != nullptr) { mCbLooper->unregisterHandler(mHandler->id()); mCbLooper->stop(); diff --git a/camera/ndk/ndk_vendor/impl/ACameraDevice.h b/camera/ndk/ndk_vendor/impl/ACameraDevice.h index 3328a85686..0b882eebe5 100644 --- a/camera/ndk/ndk_vendor/impl/ACameraDevice.h +++ b/camera/ndk/ndk_vendor/impl/ACameraDevice.h @@ -36,6 +36,7 @@ #include #include + #include "ACameraMetadata.h" #include "utils.h" @@ -134,7 +135,7 @@ class CameraDevice final : public RefBase { inline ACameraDevice* getWrapper() const { return mWrapper; }; // Stop the looper thread and unregister the handler - void stopLooper(); + void stopLooperAndDisconnect(); private: friend ACameraCaptureSession; diff --git a/camera/ndk/ndk_vendor/tests/AImageReaderVendorTest.cpp b/camera/ndk/ndk_vendor/tests/AImageReaderVendorTest.cpp index 37de30ab01..7ab0124be1 100644 --- a/camera/ndk/ndk_vendor/tests/AImageReaderVendorTest.cpp +++ b/camera/ndk/ndk_vendor/tests/AImageReaderVendorTest.cpp @@ -24,6 +24,7 @@ #include #include #include +#include #include #include #include @@ -49,6 +50,7 @@ static constexpr int kTestImageHeight = 480; static constexpr int kTestImageFormat = AIMAGE_FORMAT_YUV_420_888; using android::hardware::camera::common::V1_0::helper::VendorTagDescriptorCache; +using ConfiguredWindows = std::set; class CameraHelper { public: @@ -60,9 +62,12 @@ class CameraHelper { const char* physicalCameraId; native_handle_t* anw; }; - int initCamera(native_handle_t* imgReaderAnw, + + // Retaining the error code in case the caller needs to analyze it. + std::variant initCamera(native_handle_t* imgReaderAnw, const std::vector& physicalImgReaders, bool usePhysicalSettings) { + ConfiguredWindows configuredWindows; if (imgReaderAnw == nullptr) { ALOGE("Cannot initialize camera before image reader get initialized."); return -1; @@ -78,7 +83,7 @@ class CameraHelper { ret = ACameraManager_openCamera(mCameraManager, mCameraId, &mDeviceCb, &mDevice); if (ret != AMEDIA_OK || mDevice == nullptr) { ALOGE("Failed to open camera, ret=%d, mDevice=%p.", ret, mDevice); - return -1; + return ret; } // Create capture session @@ -97,8 +102,9 @@ class CameraHelper { ALOGE("ACaptureSessionOutputContainer_add failed, ret=%d", ret); return ret; } - + configuredWindows.insert(mImgReaderAnw); std::vector idPointerList; + std::set physicalStreamMap; for (auto& physicalStream : physicalImgReaders) { ACaptureSessionOutput* sessionOutput = nullptr; ret = ACaptureSessionPhysicalOutput_create(physicalStream.anw, @@ -112,21 +118,25 @@ class CameraHelper { ALOGE("ACaptureSessionOutputContainer_add failed, ret=%d", ret); return ret; } - mExtraOutputs.push_back(sessionOutput); + ret = ACameraDevice_isSessionConfigurationSupported(mDevice, mOutputs); + if (ret != ACAMERA_OK && ret != ACAMERA_ERROR_UNSUPPORTED_OPERATION) { + ALOGW("ACameraDevice_isSessionConfigurationSupported failed, ret=%d camera id %s", + ret, mCameraId); + ACaptureSessionOutputContainer_remove(mOutputs, sessionOutput); + ACaptureSessionOutput_free(sessionOutput); + continue; + } + configuredWindows.insert(physicalStream.anw); // Assume that at most one physical stream per physical camera. mPhysicalCameraIds.push_back(physicalStream.physicalCameraId); idPointerList.push_back(physicalStream.physicalCameraId); + physicalStreamMap.insert(physicalStream.anw); + mSessionPhysicalOutputs.push_back(sessionOutput); } ACameraIdList cameraIdList; cameraIdList.numCameras = idPointerList.size(); cameraIdList.cameraIds = idPointerList.data(); - ret = ACameraDevice_isSessionConfigurationSupported(mDevice, mOutputs); - if (ret != ACAMERA_OK && ret != ACAMERA_ERROR_UNSUPPORTED_OPERATION) { - ALOGE("ACameraDevice_isSessionConfigurationSupported failed, ret=%d", ret); - return ret; - } - ret = ACameraDevice_createCaptureSession(mDevice, mOutputs, &mSessionCb, &mSession); if (ret != AMEDIA_OK) { ALOGE("ACameraDevice_createCaptureSession failed, ret=%d", ret); @@ -157,6 +167,10 @@ class CameraHelper { } for (auto& physicalStream : physicalImgReaders) { + if (physicalStreamMap.find(physicalStream.anw) == physicalStreamMap.end()) { + ALOGI("Skipping physicalStream anw=%p", physicalStream.anw); + continue; + } ACameraOutputTarget* outputTarget = nullptr; ret = ACameraOutputTarget_create(physicalStream.anw, &outputTarget); if (ret != AMEDIA_OK) { @@ -168,11 +182,11 @@ class CameraHelper { ALOGE("ACaptureRequest_addTarget failed, ret=%d", ret); return ret; } - mReqExtraOutputs.push_back(outputTarget); + mReqPhysicalOutputs.push_back(outputTarget); } mIsCameraReady = true; - return 0; + return configuredWindows; } @@ -184,10 +198,10 @@ class CameraHelper { ACameraOutputTarget_free(mReqImgReaderOutput); mReqImgReaderOutput = nullptr; } - for (auto& outputTarget : mReqExtraOutputs) { + for (auto& outputTarget : mReqPhysicalOutputs) { ACameraOutputTarget_free(outputTarget); } - mReqExtraOutputs.clear(); + mReqPhysicalOutputs.clear(); if (mStillRequest) { ACaptureRequest_free(mStillRequest); mStillRequest = nullptr; @@ -201,10 +215,10 @@ class CameraHelper { ACaptureSessionOutput_free(mImgReaderOutput); mImgReaderOutput = nullptr; } - for (auto& extraOutput : mExtraOutputs) { + for (auto& extraOutput : mSessionPhysicalOutputs) { ACaptureSessionOutput_free(extraOutput); } - mExtraOutputs.clear(); + mSessionPhysicalOutputs.clear(); if (mOutputs) { ACaptureSessionOutputContainer_free(mOutputs); mOutputs = nullptr; @@ -262,13 +276,13 @@ class CameraHelper { // Capture session ACaptureSessionOutputContainer* mOutputs = nullptr; ACaptureSessionOutput* mImgReaderOutput = nullptr; - std::vector mExtraOutputs; + std::vector mSessionPhysicalOutputs; ACameraCaptureSession* mSession = nullptr; // Capture request ACaptureRequest* mStillRequest = nullptr; ACameraOutputTarget* mReqImgReaderOutput = nullptr; - std::vector mReqExtraOutputs; + std::vector mReqPhysicalOutputs; bool mIsCameraReady = false; const char* mCameraId; @@ -581,9 +595,11 @@ class AImageReaderVendorTest : public ::testing::Test { } CameraHelper cameraHelper(id, mCameraManager); - ret = cameraHelper.initCamera(testCase.getNativeWindow(), - {}/*physicalImageReaders*/, false/*usePhysicalSettings*/); - if (ret < 0) { + std::variant retInit = + cameraHelper.initCamera(testCase.getNativeWindow(), {}/*physicalImageReaders*/, + false/*usePhysicalSettings*/); + int *retp = std::get_if(&retInit); + if (retp) { ALOGE("Unable to initialize camera helper"); return false; } @@ -751,10 +767,15 @@ class AImageReaderVendorTest : public ::testing::Test { physicalImgReaderInfo.push_back({physicalCameraIds[0], testCases[1]->getNativeWindow()}); physicalImgReaderInfo.push_back({physicalCameraIds[1], testCases[2]->getNativeWindow()}); - int ret = cameraHelper.initCamera(testCases[0]->getNativeWindow(), - physicalImgReaderInfo, usePhysicalSettings); - ASSERT_EQ(ret, 0); - + std::variant retInit = + cameraHelper.initCamera(testCases[0]->getNativeWindow(), physicalImgReaderInfo, + usePhysicalSettings); + int *retp = std::get_if(&retInit); + ASSERT_EQ(retp, nullptr); + ConfiguredWindows *configuredWindowsp = std::get_if(&retInit); + ASSERT_NE(configuredWindowsp, nullptr); + ASSERT_LE(configuredWindowsp->size(), testCases.size()); + int ret = 0; if (!cameraHelper.isCameraReady()) { ALOGW("Camera is not ready after successful initialization. It's either due to camera " "on board lacks BACKWARDS_COMPATIBLE capability or the device does not have " @@ -776,9 +797,15 @@ class AImageReaderVendorTest : public ::testing::Test { break; } } - ASSERT_EQ(testCases[0]->getAcquiredImageCount(), pictureCount); - ASSERT_EQ(testCases[1]->getAcquiredImageCount(), pictureCount); - ASSERT_EQ(testCases[2]->getAcquiredImageCount(), pictureCount); + for(auto &testCase : testCases) { + auto it = configuredWindowsp->find(testCase->getNativeWindow()); + if (it == configuredWindowsp->end()) { + continue; + } + ALOGI("Testing window %p", testCase->getNativeWindow()); + ASSERT_EQ(testCase->getAcquiredImageCount(), pictureCount); + } + ASSERT_TRUE(cameraHelper.checkCallbacks(pictureCount)); ACameraMetadata_free(staticMetadata); diff --git a/media/ndk/NdkImage.cpp b/media/ndk/NdkImage.cpp index 1883f6356d..1145b7b79f 100644 --- a/media/ndk/NdkImage.cpp +++ b/media/ndk/NdkImage.cpp @@ -35,6 +35,7 @@ AImage::AImage(AImageReader* reader, int32_t format, uint64_t usage, BufferItem* int64_t timestamp, int32_t width, int32_t height, int32_t numPlanes) : mReader(reader), mFormat(format), mUsage(usage), mBuffer(buffer), mLockedBuffer(nullptr), mTimestamp(timestamp), mWidth(width), mHeight(height), mNumPlanes(numPlanes) { + LOG_FATAL_IF(reader == nullptr, "AImageReader shouldn't be null while creating AImage"); } AImage::~AImage() { @@ -57,14 +58,9 @@ AImage::close(int releaseFenceFd) { if (mIsClosed) { return; } - sp reader = mReader.promote(); - if (reader != nullptr) { - reader->releaseImageLocked(this, releaseFenceFd); - } else if (mBuffer != nullptr) { - LOG_ALWAYS_FATAL("%s: parent AImageReader closed without releasing image %p", - __FUNCTION__, this); + if (!mReader->mIsClosed) { + mReader->releaseImageLocked(this, releaseFenceFd); } - // Should have been set to nullptr in releaseImageLocked // Set to nullptr here for extra safety only mBuffer = nullptr; @@ -83,22 +79,12 @@ AImage::free() { void AImage::lockReader() const { - sp reader = mReader.promote(); - if (reader == nullptr) { - // Reader has been closed - return; - } - reader->mLock.lock(); + mReader->mLock.lock(); } void AImage::unlockReader() const { - sp reader = mReader.promote(); - if (reader == nullptr) { - // Reader has been closed - return; - } - reader->mLock.unlock(); + mReader->mLock.unlock(); } media_status_t diff --git a/media/ndk/NdkImagePriv.h b/media/ndk/NdkImagePriv.h index e0f16dadf9..0e8cbcbf78 100644 --- a/media/ndk/NdkImagePriv.h +++ b/media/ndk/NdkImagePriv.h @@ -72,7 +72,7 @@ struct AImage { uint32_t getJpegSize() const; // When reader is close, AImage will only accept close API call - wp mReader; + const sp mReader; const int32_t mFormat; const uint64_t mUsage; // AHARDWAREBUFFER_USAGE_* flags. BufferItem* mBuffer; diff --git a/media/ndk/NdkImageReader.cpp b/media/ndk/NdkImageReader.cpp index baa4fc77ab..c0ceb3d70b 100644 --- a/media/ndk/NdkImageReader.cpp +++ b/media/ndk/NdkImageReader.cpp @@ -113,12 +113,12 @@ AImageReader::getNumPlanesForFormat(int32_t format) { void AImageReader::FrameListener::onFrameAvailable(const BufferItem& /*item*/) { - Mutex::Autolock _l(mLock); sp reader = mReader.promote(); if (reader == nullptr) { ALOGW("A frame is available after AImageReader closed!"); return; // reader has been closed } + Mutex::Autolock _l(mLock); if (mListener.onImageAvailable == nullptr) { return; // No callback registered } @@ -143,12 +143,12 @@ AImageReader::FrameListener::setImageListener(AImageReader_ImageListener* listen void AImageReader::BufferRemovedListener::onBufferFreed(const wp& graphicBuffer) { - Mutex::Autolock _l(mLock); sp reader = mReader.promote(); if (reader == nullptr) { ALOGW("A frame is available after AImageReader closed!"); return; // reader has been closed } + Mutex::Autolock _l(mLock); if (mListener.onBufferRemoved == nullptr) { return; // No callback registered } @@ -272,6 +272,11 @@ AImageReader::AImageReader(int32_t width, mFrameListener(new FrameListener(this)), mBufferRemovedListener(new BufferRemovedListener(this)) {} +AImageReader::~AImageReader() { + Mutex::Autolock _l(mLock); + LOG_FATAL_IF("AImageReader not closed before destruction", mIsClosed != true); +} + media_status_t AImageReader::init() { PublicFormat publicFormat = static_cast(mFormat); @@ -347,8 +352,12 @@ AImageReader::init() { return AMEDIA_OK; } -AImageReader::~AImageReader() { +void AImageReader::close() { Mutex::Autolock _l(mLock); + if (mIsClosed) { + return; + } + mIsClosed = true; AImageReader_ImageListener nullListener = {nullptr, nullptr}; setImageListenerLocked(&nullListener); @@ -741,6 +750,7 @@ EXPORT void AImageReader_delete(AImageReader* reader) { ALOGV("%s", __FUNCTION__); if (reader != nullptr) { + reader->close(); reader->decStrong((void*) AImageReader_delete); } return; diff --git a/media/ndk/NdkImageReaderPriv.h b/media/ndk/NdkImageReaderPriv.h index e328cb1429..0779a716bf 100644 --- a/media/ndk/NdkImageReaderPriv.h +++ b/media/ndk/NdkImageReaderPriv.h @@ -76,6 +76,7 @@ struct AImageReader : public RefBase { int32_t getHeight() const { return mHeight; }; int32_t getFormat() const { return mFormat; }; int32_t getMaxImages() const { return mMaxImages; }; + void close(); private: @@ -134,7 +135,7 @@ struct AImageReader : public RefBase { private: AImageReader_ImageListener mListener = {nullptr, nullptr}; - wp mReader; + const wp mReader; Mutex mLock; }; sp mFrameListener; @@ -149,7 +150,7 @@ struct AImageReader : public RefBase { private: AImageReader_BufferRemovedListener mListener = {nullptr, nullptr}; - wp mReader; + const wp mReader; Mutex mLock; }; sp mBufferRemovedListener; @@ -165,6 +166,7 @@ struct AImageReader : public RefBase { native_handle_t* mWindowHandle = nullptr; List mAcquiredImages; + bool mIsClosed = false; Mutex mLock; };