[libcamera-devel,6/6] android: Replace ThreadRPC with blocking method call
diff mbox series

Message ID 20191027203335.26888-7-jacopo@jmondi.org
State Accepted
Headers show
Series
  • Add support for blocking method invocation
Related show

Commit Message

Jacopo Mondi Oct. 27, 2019, 8:33 p.m. UTC 
Use the newly introduced InvocationTypeBlocking message type to replace
the blocking message delivery implemented with the ThreadRPC class in the
Android camera HAL.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/android/camera_device.cpp | 34 ++++++------------------------
 src/android/camera_device.h   |  5 +----
 src/android/camera_proxy.cpp  | 21 ++++---------------
 src/android/camera_proxy.h    |  3 ---
 src/android/meson.build       |  1 -
 src/android/thread_rpc.cpp    | 26 -----------------------
 src/android/thread_rpc.h      | 39 -----------------------------------
 7 files changed, 11 insertions(+), 118 deletions(-)
 delete mode 100644 src/android/thread_rpc.cpp
 delete mode 100644 src/android/thread_rpc.h

Comments

Laurent Pinchart Oct. 27, 2019, 11:38 p.m. UTC | #1 
Hi Jacopo,

Thank you for the patch.

On Sun, Oct 27, 2019 at 09:33:35PM +0100, Jacopo Mondi wrote:
> Use the newly introduced InvocationTypeBlocking message type to replace
> the blocking message delivery implemented with the ThreadRPC class in the
> Android camera HAL.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

I really like this !!

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  src/android/camera_device.cpp | 34 ++++++------------------------
>  src/android/camera_device.h   |  5 +----
>  src/android/camera_proxy.cpp  | 21 ++++---------------
>  src/android/camera_proxy.h    |  3 ---
>  src/android/meson.build       |  1 -
>  src/android/thread_rpc.cpp    | 26 -----------------------
>  src/android/thread_rpc.h      | 39 -----------------------------------
>  7 files changed, 11 insertions(+), 118 deletions(-)
>  delete mode 100644 src/android/thread_rpc.cpp
>  delete mode 100644 src/android/thread_rpc.h
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index c7c9b3fd1724..897f545864a9 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -11,7 +11,6 @@
>  #include "utils.h"
>  
>  #include "camera_metadata.h"
> -#include "thread_rpc.h"
>  
>  using namespace libcamera;
>  
> @@ -64,25 +63,6 @@ CameraDevice::~CameraDevice()
>  		delete it.second;
>  }
>  
> -/*
> - * Handle RPC request received from the associated proxy.
> - */
> -void CameraDevice::call(ThreadRpc *rpc)
> -{
> -	switch (rpc->tag) {
> -	case ThreadRpc::ProcessCaptureRequest:
> -		processCaptureRequest(rpc->request);
> -		break;
> -	case ThreadRpc::Close:
> -		close();
> -		break;
> -	default:
> -		LOG(HAL, Error) << "Unknown RPC operation: " << rpc->tag;
> -	}
> -
> -	rpc->notifyReception();
> -}
> -
>  int CameraDevice::open()
>  {
>  	int ret = camera_->acquire();
> @@ -698,7 +678,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  	return 0;
>  }
>  
> -int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)
> +void CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)
>  {
>  	StreamConfiguration *streamConfiguration = &config_->at(0);
>  	Stream *stream = streamConfiguration->stream();
> @@ -706,7 +686,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  	if (camera3Request->num_output_buffers != 1) {
>  		LOG(HAL, Error) << "Invalid number of output buffers: "
>  				<< camera3Request->num_output_buffers;
> -		return -EINVAL;
> +		return;
>  	}
>  
>  	/* Start the camera if that's the first request we handle. */
> @@ -714,14 +694,14 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  		int ret = camera_->allocateBuffers();
>  		if (ret) {
>  			LOG(HAL, Error) << "Failed to allocate buffers";
> -			return ret;
> +			return;
>  		}
>  
>  		ret = camera_->start();
>  		if (ret) {
>  			LOG(HAL, Error) << "Failed to start camera";
>  			camera_->freeBuffers();
> -			return ret;
> +			return;
>  		}
>  
>  		running_ = true;
> @@ -769,7 +749,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  	if (!buffer) {
>  		LOG(HAL, Error) << "Failed to create buffer";
>  		delete descriptor;
> -		return -EINVAL;
> +		return;
>  	}
>  
>  	Request *request =
> @@ -782,13 +762,11 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  		goto error;
>  	}
>  
> -	return 0;
> +	return;
>  
>  error:
>  	delete request;
>  	delete descriptor;
> -
> -	return ret;
>  }
>  
>  void CameraDevice::requestComplete(Request *request,
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index d5d136a74f4a..2105b5b9a1e7 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -20,7 +20,6 @@
>  #include "message.h"
>  
>  class CameraMetadata;
> -class ThreadRpc;
>  
>  class CameraDevice : public libcamera::Object
>  {
> @@ -28,15 +27,13 @@ public:
>  	CameraDevice(unsigned int id, const std::shared_ptr<libcamera::Camera> &camera);
>  	~CameraDevice();
>  
> -	void call(ThreadRpc *rpc);
> -
>  	int open();
>  	void close();
>  	void setCallbacks(const camera3_callback_ops_t *callbacks);
>  	camera_metadata_t *getStaticMetadata();
>  	const camera_metadata_t *constructDefaultRequestSettings(int type);
>  	int configureStreams(camera3_stream_configuration_t *stream_list);
> -	int processCaptureRequest(camera3_capture_request_t *request);
> +	void processCaptureRequest(camera3_capture_request_t *request);
>  	void requestComplete(libcamera::Request *request,
>  			     const std::map<libcamera::Stream *, libcamera::Buffer *> &buffers);
>  
> diff --git a/src/android/camera_proxy.cpp b/src/android/camera_proxy.cpp
> index 5f8428cfddfb..a48d8d265328 100644
> --- a/src/android/camera_proxy.cpp
> +++ b/src/android/camera_proxy.cpp
> @@ -16,7 +16,6 @@
>  #include "utils.h"
>  
>  #include "camera_device.h"
> -#include "thread_rpc.h"
>  
>  using namespace libcamera;
>  
> @@ -148,10 +147,8 @@ int CameraProxy::open(const hw_module_t *hardwareModule)
>  
>  void CameraProxy::close()
>  {
> -	ThreadRpc rpcRequest;
> -	rpcRequest.tag = ThreadRpc::Close;
> -
> -	threadRpcCall(rpcRequest);
> +	cameraDevice_->invokeMethod(&CameraDevice::close,
> +				    InvocationTypeBlocking);
>  }
>  
>  void CameraProxy::initialize(const camera3_callback_ops_t *callbacks)
> @@ -176,18 +173,8 @@ int CameraProxy::configureStreams(camera3_stream_configuration_t *stream_list)
>  
>  int CameraProxy::processCaptureRequest(camera3_capture_request_t *request)
>  {
> -	ThreadRpc rpcRequest;
> -	rpcRequest.tag = ThreadRpc::ProcessCaptureRequest;
> -	rpcRequest.request = request;
> -
> -	threadRpcCall(rpcRequest);
> +	cameraDevice_->invokeMethod(&CameraDevice::processCaptureRequest,
> +				    InvocationTypeBlocking, request);
>  
>  	return 0;
>  }
> -
> -void CameraProxy::threadRpcCall(ThreadRpc &rpcRequest)
> -{
> -	cameraDevice_->invokeMethod(&CameraDevice::call, InvocationTypeQueued,
> -				    &rpcRequest);
> -	rpcRequest.waitDelivery();
> -}
> diff --git a/src/android/camera_proxy.h b/src/android/camera_proxy.h
> index 7940eac4e376..e8cfbc9dd526 100644
> --- a/src/android/camera_proxy.h
> +++ b/src/android/camera_proxy.h
> @@ -14,7 +14,6 @@
>  #include <libcamera/camera.h>
>  
>  class CameraDevice;
> -class ThreadRpc;
>  
>  class CameraProxy
>  {
> @@ -35,8 +34,6 @@ public:
>  	camera3_device_t *camera3Device() { return &camera3Device_; }
>  
>  private:
> -	void threadRpcCall(ThreadRpc &rpcRequest);
> -
>  	unsigned int id_;
>  	CameraDevice *cameraDevice_;
>  	camera3_device_t camera3Device_;
> diff --git a/src/android/meson.build b/src/android/meson.build
> index b5e4eeeb73a8..70dfcc1df27a 100644
> --- a/src/android/meson.build
> +++ b/src/android/meson.build
> @@ -4,7 +4,6 @@ android_hal_sources = files([
>      'camera_device.cpp',
>      'camera_metadata.cpp',
>      'camera_proxy.cpp',
> -    'thread_rpc.cpp'
>  ])
>  
>  android_camera_metadata_sources = files([
> diff --git a/src/android/thread_rpc.cpp b/src/android/thread_rpc.cpp
> deleted file mode 100644
> index f57891ff56bf..000000000000
> --- a/src/android/thread_rpc.cpp
> +++ /dev/null
> @@ -1,26 +0,0 @@
> -/* SPDX-License-Identifier: LGPL-2.1-or-later */
> -/*
> - * Copyright (C) 2019, Google Inc.
> - *
> - * thread_rpc.cpp - Inter-thread procedure call
> - */
> -
> -#include "thread.h"
> -#include "thread_rpc.h"
> -
> -using namespace libcamera;
> -
> -void ThreadRpc::notifyReception()
> -{
> -	{
> -		libcamera::MutexLocker locker(mutex_);
> -		delivered_ = true;
> -	}
> -	cv_.notify_one();
> -}
> -
> -void ThreadRpc::waitDelivery()
> -{
> -	libcamera::MutexLocker locker(mutex_);
> -	cv_.wait(locker, [&] { return delivered_; });
> -}
> diff --git a/src/android/thread_rpc.h b/src/android/thread_rpc.h
> deleted file mode 100644
> index f577a5d9fb32..000000000000
> --- a/src/android/thread_rpc.h
> +++ /dev/null
> @@ -1,39 +0,0 @@
> -/* SPDX-License-Identifier: LGPL-2.1-or-later */
> -/*
> - * Copyright (C) 2019, Google Inc.
> - *
> - * thread_rpc.h - Inter-thread procedure call
> - */
> -#ifndef __ANDROID_THREAD_RPC_H__
> -#define __ANDROID_THREAD_RPC_H__
> -
> -#include <condition_variable>
> -#include <mutex>
> -
> -#include <hardware/camera3.h>
> -
> -class ThreadRpc
> -{
> -public:
> -	enum RpcTag {
> -		ProcessCaptureRequest,
> -		Close,
> -	};
> -
> -	ThreadRpc()
> -		: delivered_(false) {}
> -
> -	void notifyReception();
> -	void waitDelivery();
> -
> -	RpcTag tag;
> -
> -	camera3_capture_request_t *request;
> -
> -private:
> -	bool delivered_;
> -	std::mutex mutex_;
> -	std::condition_variable cv_;
> -};
> -
> -#endif /* __ANDROID_THREAD_RPC_H__ */

Patch
diff mbox series 

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index c7c9b3fd1724..897f545864a9 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -11,7 +11,6 @@ 
 #include "utils.h"
 
 #include "camera_metadata.h"
-#include "thread_rpc.h"
 
 using namespace libcamera;
 
@@ -64,25 +63,6 @@  CameraDevice::~CameraDevice()
 		delete it.second;
 }
 
-/*
- * Handle RPC request received from the associated proxy.
- */
-void CameraDevice::call(ThreadRpc *rpc)
-{
-	switch (rpc->tag) {
-	case ThreadRpc::ProcessCaptureRequest:
-		processCaptureRequest(rpc->request);
-		break;
-	case ThreadRpc::Close:
-		close();
-		break;
-	default:
-		LOG(HAL, Error) << "Unknown RPC operation: " << rpc->tag;
-	}
-
-	rpc->notifyReception();
-}
-
 int CameraDevice::open()
 {
 	int ret = camera_->acquire();
@@ -698,7 +678,7 @@  int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
 	return 0;
 }
 
-int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)
+void CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)
 {
 	StreamConfiguration *streamConfiguration = &config_->at(0);
 	Stream *stream = streamConfiguration->stream();
@@ -706,7 +686,7 @@  int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
 	if (camera3Request->num_output_buffers != 1) {
 		LOG(HAL, Error) << "Invalid number of output buffers: "
 				<< camera3Request->num_output_buffers;
-		return -EINVAL;
+		return;
 	}
 
 	/* Start the camera if that's the first request we handle. */
@@ -714,14 +694,14 @@  int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
 		int ret = camera_->allocateBuffers();
 		if (ret) {
 			LOG(HAL, Error) << "Failed to allocate buffers";
-			return ret;
+			return;
 		}
 
 		ret = camera_->start();
 		if (ret) {
 			LOG(HAL, Error) << "Failed to start camera";
 			camera_->freeBuffers();
-			return ret;
+			return;
 		}
 
 		running_ = true;
@@ -769,7 +749,7 @@  int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
 	if (!buffer) {
 		LOG(HAL, Error) << "Failed to create buffer";
 		delete descriptor;
-		return -EINVAL;
+		return;
 	}
 
 	Request *request =
@@ -782,13 +762,11 @@  int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
 		goto error;
 	}
 
-	return 0;
+	return;
 
 error:
 	delete request;
 	delete descriptor;
-
-	return ret;
 }
 
 void CameraDevice::requestComplete(Request *request,
diff --git a/src/android/camera_device.h b/src/android/camera_device.h
index d5d136a74f4a..2105b5b9a1e7 100644
--- a/src/android/camera_device.h
+++ b/src/android/camera_device.h
@@ -20,7 +20,6 @@ 
 #include "message.h"
 
 class CameraMetadata;
-class ThreadRpc;
 
 class CameraDevice : public libcamera::Object
 {
@@ -28,15 +27,13 @@  public:
 	CameraDevice(unsigned int id, const std::shared_ptr<libcamera::Camera> &camera);
 	~CameraDevice();
 
-	void call(ThreadRpc *rpc);
-
 	int open();
 	void close();
 	void setCallbacks(const camera3_callback_ops_t *callbacks);
 	camera_metadata_t *getStaticMetadata();
 	const camera_metadata_t *constructDefaultRequestSettings(int type);
 	int configureStreams(camera3_stream_configuration_t *stream_list);
-	int processCaptureRequest(camera3_capture_request_t *request);
+	void processCaptureRequest(camera3_capture_request_t *request);
 	void requestComplete(libcamera::Request *request,
 			     const std::map<libcamera::Stream *, libcamera::Buffer *> &buffers);
 
diff --git a/src/android/camera_proxy.cpp b/src/android/camera_proxy.cpp
index 5f8428cfddfb..a48d8d265328 100644
--- a/src/android/camera_proxy.cpp
+++ b/src/android/camera_proxy.cpp
@@ -16,7 +16,6 @@ 
 #include "utils.h"
 
 #include "camera_device.h"
-#include "thread_rpc.h"
 
 using namespace libcamera;
 
@@ -148,10 +147,8 @@  int CameraProxy::open(const hw_module_t *hardwareModule)
 
 void CameraProxy::close()
 {
-	ThreadRpc rpcRequest;
-	rpcRequest.tag = ThreadRpc::Close;
-
-	threadRpcCall(rpcRequest);
+	cameraDevice_->invokeMethod(&CameraDevice::close,
+				    InvocationTypeBlocking);
 }
 
 void CameraProxy::initialize(const camera3_callback_ops_t *callbacks)
@@ -176,18 +173,8 @@  int CameraProxy::configureStreams(camera3_stream_configuration_t *stream_list)
 
 int CameraProxy::processCaptureRequest(camera3_capture_request_t *request)
 {
-	ThreadRpc rpcRequest;
-	rpcRequest.tag = ThreadRpc::ProcessCaptureRequest;
-	rpcRequest.request = request;
-
-	threadRpcCall(rpcRequest);
+	cameraDevice_->invokeMethod(&CameraDevice::processCaptureRequest,
+				    InvocationTypeBlocking, request);
 
 	return 0;
 }
-
-void CameraProxy::threadRpcCall(ThreadRpc &rpcRequest)
-{
-	cameraDevice_->invokeMethod(&CameraDevice::call, InvocationTypeQueued,
-				    &rpcRequest);
-	rpcRequest.waitDelivery();
-}
diff --git a/src/android/camera_proxy.h b/src/android/camera_proxy.h
index 7940eac4e376..e8cfbc9dd526 100644
--- a/src/android/camera_proxy.h
+++ b/src/android/camera_proxy.h
@@ -14,7 +14,6 @@ 
 #include <libcamera/camera.h>
 
 class CameraDevice;
-class ThreadRpc;
 
 class CameraProxy
 {
@@ -35,8 +34,6 @@  public:
 	camera3_device_t *camera3Device() { return &camera3Device_; }
 
 private:
-	void threadRpcCall(ThreadRpc &rpcRequest);
-
 	unsigned int id_;
 	CameraDevice *cameraDevice_;
 	camera3_device_t camera3Device_;
diff --git a/src/android/meson.build b/src/android/meson.build
index b5e4eeeb73a8..70dfcc1df27a 100644
--- a/src/android/meson.build
+++ b/src/android/meson.build
@@ -4,7 +4,6 @@  android_hal_sources = files([
     'camera_device.cpp',
     'camera_metadata.cpp',
     'camera_proxy.cpp',
-    'thread_rpc.cpp'
 ])
 
 android_camera_metadata_sources = files([
diff --git a/src/android/thread_rpc.cpp b/src/android/thread_rpc.cpp
deleted file mode 100644
index f57891ff56bf..000000000000
--- a/src/android/thread_rpc.cpp
+++ /dev/null
@@ -1,26 +0,0 @@ 
-/* SPDX-License-Identifier: LGPL-2.1-or-later */
-/*
- * Copyright (C) 2019, Google Inc.
- *
- * thread_rpc.cpp - Inter-thread procedure call
- */
-
-#include "thread.h"
-#include "thread_rpc.h"
-
-using namespace libcamera;
-
-void ThreadRpc::notifyReception()
-{
-	{
-		libcamera::MutexLocker locker(mutex_);
-		delivered_ = true;
-	}
-	cv_.notify_one();
-}
-
-void ThreadRpc::waitDelivery()
-{
-	libcamera::MutexLocker locker(mutex_);
-	cv_.wait(locker, [&] { return delivered_; });
-}
diff --git a/src/android/thread_rpc.h b/src/android/thread_rpc.h
deleted file mode 100644
index f577a5d9fb32..000000000000
--- a/src/android/thread_rpc.h
+++ /dev/null
@@ -1,39 +0,0 @@ 
-/* SPDX-License-Identifier: LGPL-2.1-or-later */
-/*
- * Copyright (C) 2019, Google Inc.
- *
- * thread_rpc.h - Inter-thread procedure call
- */
-#ifndef __ANDROID_THREAD_RPC_H__
-#define __ANDROID_THREAD_RPC_H__
-
-#include <condition_variable>
-#include <mutex>
-
-#include <hardware/camera3.h>
-
-class ThreadRpc
-{
-public:
-	enum RpcTag {
-		ProcessCaptureRequest,
-		Close,
-	};
-
-	ThreadRpc()
-		: delivered_(false) {}
-
-	void notifyReception();
-	void waitDelivery();
-
-	RpcTag tag;
-
-	camera3_capture_request_t *request;
-
-private:
-	bool delivered_;
-	std::mutex mutex_;
-	std::condition_variable cv_;
-};
-
-#endif /* __ANDROID_THREAD_RPC_H__ */