Merge "cameraserver: fix deadlock scenario in torchModeStatusChanged callback." (2658976c) · Commits · e / os / android_frameworks_av

services/camera/libcameraservice/CameraService.cpp

+7 −0

Original line number	Diff line number	Diff line
		@@ -560,6 +560,13 @@ void CameraService::onTorchStatusChanged(const String8& cameraId,
		onTorchStatusChangedLocked(cameraId, newStatus, systemCameraKind);
		}


		void CameraService::onTorchStatusChanged(const String8& cameraId,
		TorchModeStatus newStatus, SystemCameraKind systemCameraKind) {
		Mutex::Autolock al(mTorchStatusMutex);
		onTorchStatusChangedLocked(cameraId, newStatus, systemCameraKind);
		}

		void CameraService::onTorchStatusChangedLocked(const String8& cameraId,
		TorchModeStatus newStatus, SystemCameraKind systemCameraKind) {
		ALOGI("%s: Torch status changed for cameraId=%s, newStatus=%d",

services/camera/libcameraservice/CameraService.h

+8 −0

Original line number	Diff line number	Diff line
		@@ -110,8 +110,16 @@ public:
		virtual void onDeviceStatusChanged(const String8 &cameraId,
		const String8 &physicalCameraId,
		hardware::camera::common::V1_0::CameraDeviceStatus newHalStatus) override;
		// This method may hold CameraProviderManager::mInterfaceMutex as a part
		// of calling getSystemCameraKind() internally. Care should be taken not to
		// directly / indirectly call this from callers who also hold
		// mInterfaceMutex.
		virtual void onTorchStatusChanged(const String8& cameraId,
		hardware::camera::common::V1_0::TorchModeStatus newStatus) override;
		// Does not hold CameraProviderManager::mInterfaceMutex.
		virtual void onTorchStatusChanged(const String8& cameraId,
		hardware::camera::common::V1_0::TorchModeStatus newStatus,
		SystemCameraKind kind) override;
		virtual void onNewProviderRegistered() override;

		/////////////////////////////////////////////////////////////////////

services/camera/libcameraservice/common/CameraProviderManager.cpp

+15 −4

Original line number	Diff line number	Diff line
		@@ -1950,16 +1950,19 @@ hardware::Return<void> CameraProviderManager::ProviderInfo::torchModeStatusChang
		const hardware::hidl_string& cameraDeviceName,
		TorchModeStatus newStatus) {
		sp<StatusListener> listener;
		SystemCameraKind systemCameraKind = SystemCameraKind::PUBLIC;
		std::string id;
		{
		std::lock_guard<std::mutex> lock(mManager->mStatusListenerMutex);
		bool known = false;
		{
		// Hold mLock for accessing mDevices
		std::lock_guard<std::mutex> lock(mLock);
		for (auto& deviceInfo : mDevices) {
		if (deviceInfo->mName == cameraDeviceName) {
		ALOGI("Camera device %s torch status is now %s", cameraDeviceName.c_str(),
		torchStatusToString(newStatus));
		id = deviceInfo->mId;
		known = true;
		systemCameraKind = deviceInfo->mSystemCameraKind;
		if (TorchModeStatus::AVAILABLE_ON != newStatus) {
		mManager->removeRef(DeviceMode::TORCH, id);
		}
		@@ -1971,11 +1974,19 @@ hardware::Return<void> CameraProviderManager::ProviderInfo::torchModeStatusChang
		mProviderName.c_str(), cameraDeviceName.c_str(), newStatus);
		return hardware::Void();
		}
		// no lock needed since listener is set up only once during
		// CameraProviderManager initialization and then never changed till it is
		// destructed.
		listener = mManager->getStatusListener();
		}
		// Call without lock held to allow reentrancy into provider manager
		// The problem with holding mLock here is that we
		// might be limiting re-entrancy : CameraService::onTorchStatusChanged calls
		// back into CameraProviderManager which might try to hold mLock again (eg:
		// findDeviceInfo, which should be holding mLock while iterating through
		// each provider's devices).
		if (listener != nullptr) {
		listener->onTorchStatusChanged(String8(id.c_str()), newStatus);
		listener->onTorchStatusChanged(String8(id.c_str()), newStatus, systemCameraKind);
		}
		return hardware::Void();
		}

services/camera/libcameraservice/common/CameraProviderManager.h

+3 −2

Original line number	Diff line number	Diff line
		@@ -155,6 +155,9 @@ public:
		virtual void onDeviceStatusChanged(const String8 &cameraId,
		const String8 &physicalCameraId,
		hardware::camera::common::V1_0::CameraDeviceStatus newStatus) = 0;
		virtual void onTorchStatusChanged(const String8 &cameraId,
		hardware::camera::common::V1_0::TorchModeStatus newStatus,
		SystemCameraKind kind) = 0;
		virtual void onTorchStatusChanged(const String8 &cameraId,
		hardware::camera::common::V1_0::TorchModeStatus newStatus) = 0;
		virtual void onNewProviderRegistered() = 0;
		@@ -336,8 +339,6 @@ private:
		// All private members, unless otherwise noted, expect mInterfaceMutex to be locked before use
		mutable std::mutex mInterfaceMutex;

		// the status listener update callbacks will lock mStatusMutex
		mutable std::mutex mStatusListenerMutex;
		wp<StatusListener> mListener;
		ServiceInteractionProxy* mServiceProxy;

services/camera/libcameraservice/tests/CameraProviderManagerTest.cpp

+2 −0

Original line number	Diff line number	Diff line
		@@ -274,6 +274,8 @@ struct TestStatusListener : public CameraProviderManager::StatusListener {
		hardware::camera::common::V1_0::CameraDeviceStatus) override {}
		void onTorchStatusChanged(const String8 &,
		hardware::camera::common::V1_0::TorchModeStatus) override {}
		void onTorchStatusChanged(const String8 &,
		hardware::camera::common::V1_0::TorchModeStatus, SystemCameraKind) override {}
		void onNewProviderRegistered() override {}
		};