Merge "cameraserver: fix deadlock scenario in torchModeStatusChanged... (2c0c472a) · Commits · e / os / android_frameworks_av

services/camera/libcameraservice/CameraService.cpp

+7 −0

Original line number	Original line	Diff line number	Diff line
	@@ -560,6 +560,13 @@ void CameraService::onTorchStatusChanged(const String8& cameraId,
	onTorchStatusChangedLocked(cameraId, newStatus, systemCameraKind);		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,		void CameraService::onTorchStatusChangedLocked(const String8& cameraId,
	TorchModeStatus newStatus, SystemCameraKind systemCameraKind) {		TorchModeStatus newStatus, SystemCameraKind systemCameraKind) {
	ALOGI("%s: Torch status changed for cameraId=%s, newStatus=%d",		ALOGI("%s: Torch status changed for cameraId=%s, newStatus=%d",

services/camera/libcameraservice/CameraService.h

+8 −0

Original line number	Original line	Diff line number	Diff line
	@@ -110,8 +110,16 @@ public:
	virtual void onDeviceStatusChanged(const String8 &cameraId,		virtual void onDeviceStatusChanged(const String8 &cameraId,
	const String8 &physicalCameraId,		const String8 &physicalCameraId,
	hardware::camera::common::V1_0::CameraDeviceStatus newHalStatus) override;		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,		virtual void onTorchStatusChanged(const String8& cameraId,
	hardware::camera::common::V1_0::TorchModeStatus newStatus) override;		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;		virtual void onNewProviderRegistered() override;

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

services/camera/libcameraservice/common/CameraProviderManager.cpp

+15 −4

Original line number	Original line	Diff line number	Diff line
	@@ -1960,16 +1960,19 @@ hardware::Return<void> CameraProviderManager::ProviderInfo::torchModeStatusChang
	const hardware::hidl_string& cameraDeviceName,		const hardware::hidl_string& cameraDeviceName,
	TorchModeStatus newStatus) {		TorchModeStatus newStatus) {
	sp<StatusListener> listener;		sp<StatusListener> listener;
			SystemCameraKind systemCameraKind = SystemCameraKind::PUBLIC;
	std::string id;		std::string id;
	{
	std::lock_guard<std::mutex> lock(mManager->mStatusListenerMutex);
	bool known = false;		bool known = false;
			{
			// Hold mLock for accessing mDevices
			std::lock_guard<std::mutex> lock(mLock);
	for (auto& deviceInfo : mDevices) {		for (auto& deviceInfo : mDevices) {
	if (deviceInfo->mName == cameraDeviceName) {		if (deviceInfo->mName == cameraDeviceName) {
	ALOGI("Camera device %s torch status is now %s", cameraDeviceName.c_str(),		ALOGI("Camera device %s torch status is now %s", cameraDeviceName.c_str(),
	torchStatusToString(newStatus));		torchStatusToString(newStatus));
	id = deviceInfo->mId;		id = deviceInfo->mId;
	known = true;		known = true;
			systemCameraKind = deviceInfo->mSystemCameraKind;
	if (TorchModeStatus::AVAILABLE_ON != newStatus) {		if (TorchModeStatus::AVAILABLE_ON != newStatus) {
	mManager->removeRef(DeviceMode::TORCH, id);		mManager->removeRef(DeviceMode::TORCH, id);
	}		}
	@@ -1981,11 +1984,19 @@ hardware::Return<void> CameraProviderManager::ProviderInfo::torchModeStatusChang
	mProviderName.c_str(), cameraDeviceName.c_str(), newStatus);		mProviderName.c_str(), cameraDeviceName.c_str(), newStatus);
	return hardware::Void();		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();		listener = mManager->getStatusListener();
	}		}
	// Call without lock held to allow reentrancy into provider manager		// 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) {		if (listener != nullptr) {
	listener->onTorchStatusChanged(String8(id.c_str()), newStatus);		listener->onTorchStatusChanged(String8(id.c_str()), newStatus, systemCameraKind);
	}		}
	return hardware::Void();		return hardware::Void();
	}		}

services/camera/libcameraservice/common/CameraProviderManager.h

+3 −2

Original line number	Original line	Diff line number	Diff line
	@@ -154,6 +154,9 @@ public:
	virtual void onDeviceStatusChanged(const String8 &cameraId,		virtual void onDeviceStatusChanged(const String8 &cameraId,
	const String8 &physicalCameraId,		const String8 &physicalCameraId,
	hardware::camera::common::V1_0::CameraDeviceStatus newStatus) = 0;		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,		virtual void onTorchStatusChanged(const String8 &cameraId,
	hardware::camera::common::V1_0::TorchModeStatus newStatus) = 0;		hardware::camera::common::V1_0::TorchModeStatus newStatus) = 0;
	virtual void onNewProviderRegistered() = 0;		virtual void onNewProviderRegistered() = 0;
	@@ -329,8 +332,6 @@ private:
	// All private members, unless otherwise noted, expect mInterfaceMutex to be locked before use		// All private members, unless otherwise noted, expect mInterfaceMutex to be locked before use
	mutable std::mutex mInterfaceMutex;		mutable std::mutex mInterfaceMutex;

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

services/camera/libcameraservice/tests/CameraProviderManagerTest.cpp

+2 −0

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