[{"id":2966,"web_url":"https://patchwork.libcamera.org/comment/2966/","msgid":"<20191027233813.GD28970@pendragon.ideasonboard.com>","date":"2019-10-27T23:38:13","subject":"Re: [libcamera-devel] [PATCH 6/6] android: Replace ThreadRPC with\n\tblocking method call","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Sun, Oct 27, 2019 at 09:33:35PM +0100, Jacopo Mondi wrote:\n> Use the newly introduced InvocationTypeBlocking message type to replace\n> the blocking message delivery implemented with the ThreadRPC class in the\n> Android camera HAL.\n> \n> Signed-off-by: Jacopo Mondi \n\nI really like this !!\n\nReviewed-by: Laurent Pinchart \n\n> ---\n> src/android/camera_device.cpp | 34 ++++++------------------------\n> src/android/camera_device.h | 5 +----\n> src/android/camera_proxy.cpp | 21 ++++---------------\n> src/android/camera_proxy.h | 3 ---\n> src/android/meson.build | 1 -\n> src/android/thread_rpc.cpp | 26 -----------------------\n> src/android/thread_rpc.h | 39 -----------------------------------\n> 7 files changed, 11 insertions(+), 118 deletions(-)\n> delete mode 100644 src/android/thread_rpc.cpp\n> delete mode 100644 src/android/thread_rpc.h\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index c7c9b3fd1724..897f545864a9 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -11,7 +11,6 @@\n> #include \"utils.h\"\n> \n> #include \"camera_metadata.h\"\n> -#include \"thread_rpc.h\"\n> \n> using namespace libcamera;\n> \n> @@ -64,25 +63,6 @@ CameraDevice::~CameraDevice()\n> \t\tdelete it.second;\n> }\n> \n> -/*\n> - * Handle RPC request received from the associated proxy.\n> - */\n> -void CameraDevice::call(ThreadRpc *rpc)\n> -{\n> -\tswitch (rpc->tag) {\n> -\tcase ThreadRpc::ProcessCaptureRequest:\n> -\t\tprocessCaptureRequest(rpc->request);\n> -\t\tbreak;\n> -\tcase ThreadRpc::Close:\n> -\t\tclose();\n> -\t\tbreak;\n> -\tdefault:\n> -\t\tLOG(HAL, Error) << \"Unknown RPC operation: \" << rpc->tag;\n> -\t}\n> -\n> -\trpc->notifyReception();\n> -}\n> -\n> int CameraDevice::open()\n> {\n> \tint ret = camera_->acquire();\n> @@ -698,7 +678,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> \treturn 0;\n> }\n> \n> -int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)\n> +void CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)\n> {\n> \tStreamConfiguration *streamConfiguration = &config_->at(0);\n> \tStream *stream = streamConfiguration->stream();\n> @@ -706,7 +686,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \tif (camera3Request->num_output_buffers != 1) {\n> \t\tLOG(HAL, Error) << \"Invalid number of output buffers: \"\n> \t\t\t\t<< camera3Request->num_output_buffers;\n> -\t\treturn -EINVAL;\n> +\t\treturn;\n> \t}\n> \n> \t/* Start the camera if that's the first request we handle. */\n> @@ -714,14 +694,14 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \t\tint ret = camera_->allocateBuffers();\n> \t\tif (ret) {\n> \t\t\tLOG(HAL, Error) << \"Failed to allocate buffers\";\n> -\t\t\treturn ret;\n> +\t\t\treturn;\n> \t\t}\n> \n> \t\tret = camera_->start();\n> \t\tif (ret) {\n> \t\t\tLOG(HAL, Error) << \"Failed to start camera\";\n> \t\t\tcamera_->freeBuffers();\n> -\t\t\treturn ret;\n> +\t\t\treturn;\n> \t\t}\n> \n> \t\trunning_ = true;\n> @@ -769,7 +749,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \tif (!buffer) {\n> \t\tLOG(HAL, Error) << \"Failed to create buffer\";\n> \t\tdelete descriptor;\n> -\t\treturn -EINVAL;\n> +\t\treturn;\n> \t}\n> \n> \tRequest *request =\n> @@ -782,13 +762,11 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \t\tgoto error;\n> \t}\n> \n> -\treturn 0;\n> +\treturn;\n> \n> error:\n> \tdelete request;\n> \tdelete descriptor;\n> -\n> -\treturn ret;\n> }\n> \n> void CameraDevice::requestComplete(Request *request,\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index d5d136a74f4a..2105b5b9a1e7 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -20,7 +20,6 @@\n> #include \"message.h\"\n> \n> class CameraMetadata;\n> -class ThreadRpc;\n> \n> class CameraDevice : public libcamera::Object\n> {\n> @@ -28,15 +27,13 @@ public:\n> \tCameraDevice(unsigned int id, const std::shared_ptr &camera);\n> \t~CameraDevice();\n> \n> -\tvoid call(ThreadRpc *rpc);\n> -\n> \tint open();\n> \tvoid close();\n> \tvoid setCallbacks(const camera3_callback_ops_t *callbacks);\n> \tcamera_metadata_t *getStaticMetadata();\n> \tconst camera_metadata_t *constructDefaultRequestSettings(int type);\n> \tint configureStreams(camera3_stream_configuration_t *stream_list);\n> -\tint processCaptureRequest(camera3_capture_request_t *request);\n> +\tvoid processCaptureRequest(camera3_capture_request_t *request);\n> \tvoid requestComplete(libcamera::Request *request,\n> \t\t\t const std::map &buffers);\n> \n> diff --git a/src/android/camera_proxy.cpp b/src/android/camera_proxy.cpp\n> index 5f8428cfddfb..a48d8d265328 100644\n> --- a/src/android/camera_proxy.cpp\n> +++ b/src/android/camera_proxy.cpp\n> @@ -16,7 +16,6 @@\n> #include \"utils.h\"\n> \n> #include \"camera_device.h\"\n> -#include \"thread_rpc.h\"\n> \n> using namespace libcamera;\n> \n> @@ -148,10 +147,8 @@ int CameraProxy::open(const hw_module_t *hardwareModule)\n> \n> void CameraProxy::close()\n> {\n> -\tThreadRpc rpcRequest;\n> -\trpcRequest.tag = ThreadRpc::Close;\n> -\n> -\tthreadRpcCall(rpcRequest);\n> +\tcameraDevice_->invokeMethod(&CameraDevice::close,\n> +\t\t\t\t InvocationTypeBlocking);\n> }\n> \n> void CameraProxy::initialize(const camera3_callback_ops_t *callbacks)\n> @@ -176,18 +173,8 @@ int CameraProxy::configureStreams(camera3_stream_configuration_t *stream_list)\n> \n> int CameraProxy::processCaptureRequest(camera3_capture_request_t *request)\n> {\n> -\tThreadRpc rpcRequest;\n> -\trpcRequest.tag = ThreadRpc::ProcessCaptureRequest;\n> -\trpcRequest.request = request;\n> -\n> -\tthreadRpcCall(rpcRequest);\n> +\tcameraDevice_->invokeMethod(&CameraDevice::processCaptureRequest,\n> +\t\t\t\t InvocationTypeBlocking, request);\n> \n> \treturn 0;\n> }\n> -\n> -void CameraProxy::threadRpcCall(ThreadRpc &rpcRequest)\n> -{\n> -\tcameraDevice_->invokeMethod(&CameraDevice::call, InvocationTypeQueued,\n> -\t\t\t\t &rpcRequest);\n> -\trpcRequest.waitDelivery();\n> -}\n> diff --git a/src/android/camera_proxy.h b/src/android/camera_proxy.h\n> index 7940eac4e376..e8cfbc9dd526 100644\n> --- a/src/android/camera_proxy.h\n> +++ b/src/android/camera_proxy.h\n> @@ -14,7 +14,6 @@\n> #include \n> \n> class CameraDevice;\n> -class ThreadRpc;\n> \n> class CameraProxy\n> {\n> @@ -35,8 +34,6 @@ public:\n> \tcamera3_device_t *camera3Device() { return &camera3Device_; }\n> \n> private:\n> -\tvoid threadRpcCall(ThreadRpc &rpcRequest);\n> -\n> \tunsigned int id_;\n> \tCameraDevice *cameraDevice_;\n> \tcamera3_device_t camera3Device_;\n> diff --git a/src/android/meson.build b/src/android/meson.build\n> index b5e4eeeb73a8..70dfcc1df27a 100644\n> --- a/src/android/meson.build\n> +++ b/src/android/meson.build\n> @@ -4,7 +4,6 @@ android_hal_sources = files([\n> 'camera_device.cpp',\n> 'camera_metadata.cpp',\n> 'camera_proxy.cpp',\n> - 'thread_rpc.cpp'\n> ])\n> \n> android_camera_metadata_sources = files([\n> diff --git a/src/android/thread_rpc.cpp b/src/android/thread_rpc.cpp\n> deleted file mode 100644\n> index f57891ff56bf..000000000000\n> --- a/src/android/thread_rpc.cpp\n> +++ /dev/null\n> @@ -1,26 +0,0 @@\n> -/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> -/*\n> - * Copyright (C) 2019, Google Inc.\n> - *\n> - * thread_rpc.cpp - Inter-thread procedure call\n> - */\n> -\n> -#include \"thread.h\"\n> -#include \"thread_rpc.h\"\n> -\n> -using namespace libcamera;\n> -\n> -void ThreadRpc::notifyReception()\n> -{\n> -\t{\n> -\t\tlibcamera::MutexLocker locker(mutex_);\n> -\t\tdelivered_ = true;\n> -\t}\n> -\tcv_.notify_one();\n> -}\n> -\n> -void ThreadRpc::waitDelivery()\n> -{\n> -\tlibcamera::MutexLocker locker(mutex_);\n> -\tcv_.wait(locker, [&] { return delivered_; });\n> -}\n> diff --git a/src/android/thread_rpc.h b/src/android/thread_rpc.h\n> deleted file mode 100644\n> index f577a5d9fb32..000000000000\n> --- a/src/android/thread_rpc.h\n> +++ /dev/null\n> @@ -1,39 +0,0 @@\n> -/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> -/*\n> - * Copyright (C) 2019, Google Inc.\n> - *\n> - * thread_rpc.h - Inter-thread procedure call\n> - */\n> -#ifndef __ANDROID_THREAD_RPC_H__\n> -#define __ANDROID_THREAD_RPC_H__\n> -\n> -#include \n> -#include \n> -\n> -#include \n> -\n> -class ThreadRpc\n> -{\n> -public:\n> -\tenum RpcTag {\n> -\t\tProcessCaptureRequest,\n> -\t\tClose,\n> -\t};\n> -\n> -\tThreadRpc()\n> -\t\t: delivered_(false) {}\n> -\n> -\tvoid notifyReception();\n> -\tvoid waitDelivery();\n> -\n> -\tRpcTag tag;\n> -\n> -\tcamera3_capture_request_t *request;\n> -\n> -private:\n> -\tbool delivered_;\n> -\tstd::mutex mutex_;\n> -\tstd::condition_variable cv_;\n> -};\n> -\n> -#endif /* __ANDROID_THREAD_RPC_H__ */","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 181476017C\n\tfor ;\n\tMon, 28 Oct 2019 00:38:22 +0100 (CET)","from pendragon.ideasonboard.com (143.121.2.93.rev.sfr.net\n\t[93.2.121.143])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 80BEF325;\n\tMon, 28 Oct 2019 00:38:21 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1572219501;\n\tbh=YNjQ2V+OAQOBFyTbppo0C/MMm2CxHgLSZdruJJTDcaQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LU8qLQtc5rLkeEyxmoDHhfcP3HQCjciA4YjdNUF5ijlBy3MLCFm8XMHBp4AsKkleB\n\t3qU3z9ydhhCNnSFTCGAeVXE6O7Wr2i3V4s8m+DOABi/6tgc1uTQgMtZUeZqzxjs6TB\n\tFHbFrMFejtrc+9gLojkD2CKmADfeIVn2Up/D7Hfw=","Date":"Mon, 28 Oct 2019 01:38:13 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191027233813.GD28970@pendragon.ideasonboard.com>","References":"<20191027203335.26888-1-jacopo@jmondi.org>\n\t<20191027203335.26888-7-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20191027203335.26888-7-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 6/6] android: Replace ThreadRPC with\n\tblocking method call","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sun, 27 Oct 2019 23:38:22 -0000"}}]