[{"id":20762,"web_url":"https://patchwork.libcamera.org/comment/20762/","msgid":"<163647493790.1606134.4971636680442657532@Monstersaurus>","date":"2021-11-09T16:22:17","subject":"Re: [libcamera-devel] [PATCH 10/10] android: Remove CameraWorker","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi (2021-10-28 12:15:20)\n> The CameraWorker class purpose was to handle acquire fences for incoming\n> capture requests directed to libcamera.\n> \n> Now that fences are handled by the core library, it is not required to\n> handle them in the HAL and the CameraWorker and CaptureRequest classes\n> can be dropped.\n> \n> Update the core in CameraDevice class accordingly to queue Requests\n> directly to the libcamera::Camera and set the release_fence to the\n> value of the FrameBuffer::fence() for streams of type ::Direct.\n\nWho doesn't love a huge code remove ... ;-)\n\n\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/android/camera_device.cpp | 39 ++++------\n> src/android/camera_device.h | 5 +-\n> src/android/camera_request.cpp | 3 +-\n> src/android/camera_request.h | 3 +-\n> src/android/camera_worker.cpp | 129 ---------------------------------\n> src/android/camera_worker.h | 72 ------------------\n> src/android/meson.build | 1 -\n> 7 files changed, 19 insertions(+), 233 deletions(-)\n> delete mode 100644 src/android/camera_worker.cpp\n> delete mode 100644 src/android/camera_worker.h\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index f2e0bdbdbbf6..16237674ea24 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -406,7 +406,6 @@ void CameraDevice::flush()\n> state_ = State::Flushing;\n> }\n> \n> - worker_.stop();\n> camera_->stop();\n> \n> MutexLocker stateLock(stateMutex_);\n> @@ -419,7 +418,6 @@ void CameraDevice::stop()\n> if (state_ == State::Stopped)\n> return;\n> \n> - worker_.stop();\n> camera_->stop();\n> \n> descriptors_ = {};\n> @@ -714,9 +712,10 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> }\n> \n> std::unique_ptr\n> -CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer,\n> +CameraDevice::createFrameBuffer(const Camera3RequestDescriptor::StreamBuffer &streamBuffer,\n> PixelFormat pixelFormat, const Size &size)\n> {\n> + const buffer_handle_t camera3buffer = *streamBuffer.camera3Buffer;\n> CameraBuffer buf(camera3buffer, pixelFormat, size, PROT_READ);\n> if (!buf.isValid()) {\n> LOG(HAL, Fatal) << \"Failed to create CameraBuffer\";\n> @@ -736,7 +735,8 @@ CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer,\n> planes[i].length = buf.size(i);\n> }\n> \n> - return std::make_unique(planes);\n> + return std::make_unique(planes, 0,\n\nAha - 0 is the cookie.\nCould we make that clearer somehow? I was confused...\n\nperhaps something like\n\n\treturn std::make_unique(planes, 0 /* cookie */,\n\t\t\t\t\t streamBuffer.fence);\n\nor \n\n\tconstexpr unsigned int cookie = 0;\n\treturn std::make_unique(planes, cookie,\n\t\t\t\t\t streamBuffer.fence);\n?\n\nBut otherwise, this looks like it cleans up and uses the new given API\naccordingly.\n\n\nReviewed-by: Kieran Bingham \n\n> + streamBuffer.fence);\n> }\n> \n> int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)\n> @@ -912,13 +912,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \n> /*\n> * Inspect the camera stream type, create buffers opportunely\n> - * and add them to the Request if required. Only acquire fences\n> - * for streams of type Direct are handled by the CameraWorker,\n> - * while fences for streams of type Internal and Mapped are\n> - * handled at post-processing time.\n> + * and add them to the Request if required.\n> */\n> FrameBuffer *frameBuffer = nullptr;\n> - int acquireFence = -1;\n> switch (cameraStream->type()) {\n> case CameraStream::Type::Mapped:\n> /*\n> @@ -939,11 +935,10 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> * lifetime management only.\n> */\n> buffer.frameBuffer =\n> - createFrameBuffer(*buffer.camera3Buffer,\n> + createFrameBuffer(buffer,\n> cameraStream->configuration().pixelFormat,\n> cameraStream->configuration().size);\n> frameBuffer = buffer.frameBuffer.get();\n> - acquireFence = buffer.fence;\n> LOG(HAL, Debug) << ss.str() << \" (direct)\";\n> break;\n> \n> @@ -970,12 +965,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> }\n> \n> descriptor->request_->addBuffer(cameraStream->stream(),\n> - frameBuffer, acquireFence);\n> + frameBuffer);\n> }\n> \n> /*\n> * Translate controls from Android to libcamera and queue the request\n> - * to the CameraWorker thread.\n> + * to the camera.\n> */\n> int ret = processControls(descriptor.get());\n> if (ret)\n> @@ -1001,26 +996,23 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> }\n> \n> if (state_ == State::Stopped) {\n> - worker_.start();\n> -\n> ret = camera_->start();\n> if (ret) {\n> LOG(HAL, Error) << \"Failed to start camera\";\n> - worker_.stop();\n> return ret;\n> }\n> \n> state_ = State::Running;\n> }\n> \n> - CaptureRequest *request = descriptor->request_.get();\n> + Request *request = descriptor->request_.get();\n> \n> {\n> MutexLocker descriptorsLock(descriptorsMutex_);\n> descriptors_.push(std::move(descriptor));\n> }\n> \n> - worker_.queueRequest(request);\n> + camera_->queueRequest(request);\n> \n> return 0;\n> }\n> @@ -1042,16 +1034,17 @@ void CameraDevice::requestComplete(Request *request)\n> /*\n> * Streams of type Direct have been queued to the\n> * libcamera::Camera and their acquire fences have\n> - * already been waited on by the CameraWorker.\n> + * been handled by the library.\n> + *\n> + * If the fence has been signalled correctly it will be -1 and\n> + * closed, otherwise it is set to the 'acquire_fence' value and\n> + * the framework will have to close it.\n> *\n> * Acquire fences of streams of type Internal and Mapped\n> * will be handled during post-processing.\n> - *\n> - * \\todo Instrument the CameraWorker to set the acquire\n> - * fence to -1 once it has handled it and remove this check.\n> */\n> if (stream->type() == CameraStream::Type::Direct)\n> - buffer.fence = -1;\n> + buffer.fence = buffer.frameBuffer->fence();\n> buffer.status = Camera3RequestDescriptor::Status::Success;\n> }\n> \n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index 2a414020f1ad..d83f77a8cf22 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -30,7 +30,6 @@\n> #include \"camera_capabilities.h\"\n> #include \"camera_metadata.h\"\n> #include \"camera_stream.h\"\n> -#include \"camera_worker.h\"\n> #include \"jpeg/encoder.h\"\n> \n> class Camera3RequestDescriptor;\n> @@ -86,7 +85,7 @@ private:\n> void stop();\n> \n> std::unique_ptr\n> - createFrameBuffer(const buffer_handle_t camera3buffer,\n> + createFrameBuffer(const Camera3RequestDescriptor::StreamBuffer &streamBuffer,\n> libcamera::PixelFormat pixelFormat,\n> const libcamera::Size &size);\n> void abortRequest(Camera3RequestDescriptor *descriptor) const;\n> @@ -105,8 +104,6 @@ private:\n> unsigned int id_;\n> camera3_device_t camera3Device_;\n> \n> - CameraWorker worker_;\n> -\n> libcamera::Mutex stateMutex_; /* Protects access to the camera state. */\n> State state_;\n> \n> diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp\n> index 5bac1b8f7a5f..f656c3ebe448 100644\n> --- a/src/android/camera_request.cpp\n> +++ b/src/android/camera_request.cpp\n> @@ -49,8 +49,7 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(\n> * Create the CaptureRequest, stored as a unique_ptr<> to tie its\n> * lifetime to the descriptor.\n> */\n> - request_ = std::make_unique(camera,\n> - reinterpret_cast(this));\n> + request_ = camera->createRequest(reinterpret_cast(this));\n> }\n> \n> Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;\n> diff --git a/src/android/camera_request.h b/src/android/camera_request.h\n> index cfafa4450d71..e019c38784ba 100644\n> --- a/src/android/camera_request.h\n> +++ b/src/android/camera_request.h\n> @@ -20,7 +20,6 @@\n> #include \n> \n> #include \"camera_metadata.h\"\n> -#include \"camera_worker.h\"\n> \n> class CameraBuffer;\n> class CameraStream;\n> @@ -60,7 +59,7 @@ public:\n> std::vector buffers_;\n> \n> CameraMetadata settings_;\n> - std::unique_ptr request_;\n> + std::unique_ptr request_;\n> std::unique_ptr resultMetadata_;\n> \n> bool complete_ = false;\n> diff --git a/src/android/camera_worker.cpp b/src/android/camera_worker.cpp\n> deleted file mode 100644\n> index dabb305407a2..000000000000\n> --- a/src/android/camera_worker.cpp\n> +++ /dev/null\n> @@ -1,129 +0,0 @@\n> -/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> -/*\n> - * Copyright (C) 2020, Google Inc.\n> - *\n> - * camera_worker.cpp - Process capture requests on behalf of the Camera HAL\n> - */\n> -\n> -#include \"camera_worker.h\"\n> -\n> -#include \n> -#include \n> -#include \n> -#include \n> -\n> -#include \"camera_device.h\"\n> -\n> -using namespace libcamera;\n> -\n> -LOG_DECLARE_CATEGORY(HAL)\n> -\n> -/*\n> - * \\class CaptureRequest\n> - * \\brief Wrap a libcamera::Request associated with buffers and fences\n> - *\n> - * A CaptureRequest is constructed by the CameraDevice, filled with\n> - * buffers and fences provided by the camera3 framework and then processed\n> - * by the CameraWorker which queues it to the libcamera::Camera after handling\n> - * fences.\n> - */\n> -CaptureRequest::CaptureRequest(Camera *camera, uint64_t cookie)\n> - : camera_(camera)\n> -{\n> - request_ = camera_->createRequest(cookie);\n> -}\n> -\n> -void CaptureRequest::addBuffer(Stream *stream, FrameBuffer *buffer, int fence)\n> -{\n> - request_->addBuffer(stream, buffer);\n> - acquireFences_.push_back(fence);\n> -}\n> -\n> -void CaptureRequest::queue()\n> -{\n> - camera_->queueRequest(request_.get());\n> -}\n> -\n> -/*\n> - * \\class CameraWorker\n> - * \\brief Process a CaptureRequest on an internal thread\n> - *\n> - * The CameraWorker class wraps a Worker that runs on an internal thread\n> - * and schedules processing of CaptureRequest through it.\n> - */\n> -CameraWorker::CameraWorker()\n> -{\n> - worker_.moveToThread(this);\n> -}\n> -\n> -void CameraWorker::start()\n> -{\n> - Thread::start();\n> -}\n> -\n> -void CameraWorker::stop()\n> -{\n> - exit();\n> - wait();\n> -}\n> -\n> -void CameraWorker::run()\n> -{\n> - exec();\n> - dispatchMessages(Message::Type::InvokeMessage);\n> -}\n> -\n> -void CameraWorker::queueRequest(CaptureRequest *request)\n> -{\n> - /* Async process the request on the worker which runs its own thread. */\n> - worker_.invokeMethod(&Worker::processRequest, ConnectionTypeQueued,\n> - request);\n> -}\n> -\n> -/*\n> - * \\class CameraWorker::Worker\n> - * \\brief Process a CaptureRequest handling acquisition fences\n> - */\n> -int CameraWorker::Worker::waitFence(int fence)\n> -{\n> - /*\n> - * \\todo Better characterize the timeout. Currently equal to the one\n> - * used by the Rockchip Camera HAL on ChromeOS.\n> - */\n> - constexpr unsigned int timeoutMs = 300;\n> - struct pollfd fds = { fence, POLLIN, 0 };\n> -\n> - do {\n> - int ret = poll(&fds, 1, timeoutMs);\n> - if (ret == 0)\n> - return -ETIME;\n> -\n> - if (ret > 0) {\n> - if (fds.revents & (POLLERR | POLLNVAL))\n> - return -EINVAL;\n> -\n> - return 0;\n> - }\n> - } while (errno == EINTR || errno == EAGAIN);\n> -\n> - return -errno;\n> -}\n> -\n> -void CameraWorker::Worker::processRequest(CaptureRequest *request)\n> -{\n> - /* Wait on all fences before queuing the Request. */\n> - for (int fence : request->fences()) {\n> - if (fence == -1)\n> - continue;\n> -\n> - int ret = waitFence(fence);\n> - close(fence);\n> - if (ret < 0) {\n> - LOG(HAL, Error) << \"Failed waiting for fence: \"\n> - << fence << \": \" << strerror(-ret);\n> - return;\n> - }\n> - }\n> -\n> - request->queue();\n> -}\n> diff --git a/src/android/camera_worker.h b/src/android/camera_worker.h\n> deleted file mode 100644\n> index c94f16325925..000000000000\n> --- a/src/android/camera_worker.h\n> +++ /dev/null\n> @@ -1,72 +0,0 @@\n> -/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> -/*\n> - * Copyright (C) 2020, Google Inc.\n> - *\n> - * camera_worker.h - Process capture requests on behalf of the Camera HAL\n> - */\n> -#ifndef __ANDROID_CAMERA_WORKER_H__\n> -#define __ANDROID_CAMERA_WORKER_H__\n> -\n> -#include \n> -#include \n> -\n> -#include \n> -#include \n> -\n> -#include \n> -#include \n> -#include \n> -#include \n> -\n> -class CameraDevice;\n> -\n> -class CaptureRequest\n> -{\n> -public:\n> - CaptureRequest(libcamera::Camera *camera, uint64_t cookie);\n> -\n> - const std::vector &fences() const { return acquireFences_; }\n> - libcamera::ControlList &controls() { return request_->controls(); }\n> - const libcamera::ControlList &metadata() const\n> - {\n> - return request_->metadata();\n> - }\n> - unsigned long cookie() const { return request_->cookie(); }\n> -\n> - void addBuffer(libcamera::Stream *stream,\n> - libcamera::FrameBuffer *buffer, int fence);\n> - void queue();\n> -\n> -private:\n> - libcamera::Camera *camera_;\n> - std::vector acquireFences_;\n> - std::unique_ptr request_;\n> -};\n> -\n> -class CameraWorker : private libcamera::Thread\n> -{\n> -public:\n> - CameraWorker();\n> -\n> - void start();\n> - void stop();\n> -\n> - void queueRequest(CaptureRequest *request);\n> -\n> -protected:\n> - void run() override;\n> -\n> -private:\n> - class Worker : public libcamera::Object\n> - {\n> - public:\n> - void processRequest(CaptureRequest *request);\n> -\n> - private:\n> - int waitFence(int fence);\n> - };\n> -\n> - Worker worker_;\n> -};\n> -\n> -#endif /* __ANDROID_CAMERA_WORKER_H__ */\n> diff --git a/src/android/meson.build b/src/android/meson.build\n> index 332b177ca805..75b4bf207085 100644\n> --- a/src/android/meson.build\n> +++ b/src/android/meson.build\n> @@ -47,7 +47,6 @@ android_hal_sources = files([\n> 'camera_ops.cpp',\n> 'camera_request.cpp',\n> 'camera_stream.cpp',\n> - 'camera_worker.cpp',\n> 'jpeg/encoder_libjpeg.cpp',\n> 'jpeg/exif.cpp',\n> 'jpeg/post_processor_jpeg.cpp',\n> -- \n> 2.33.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id DE9D0BF415\n\tfor ;\n\tTue, 9 Nov 2021 16:22:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4E16A6034E;\n\tTue, 9 Nov 2021 17:22:21 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 214B8600BF\n\tfor ;\n\tTue, 9 Nov 2021 17:22:20 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AEC0F3F6;\n\tTue, 9 Nov 2021 17:22:19 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"vEQo6jvj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636474939;\n\tbh=81Ino0gmFGQxSKnSRqA/f+d+cUYyNDsmf8qGgxOsAiw=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=vEQo6jvj0QuPZjQ6UD7Wee6PCsO/3JKpvRG4dO8SFYm7r8iRzAS/s/Ux8lAiCNLeW\n\t8eo4VteI6E0gx2rGsmtaKiKD3A7DD47mFLAiJbm1Y3IkbIMN17671tAFMWug9Q9cre\n\t4rUzpFLfJhiBd8fq9dyXIRcipwGU/lqH2x2g8BQM=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211028111520.256612-11-jacopo@jmondi.org>","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-11-jacopo@jmondi.org>","From":"Kieran Bingham ","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Date":"Tue, 09 Nov 2021 16:22:17 +0000","Message-ID":"<163647493790.1606134.4971636680442657532@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH 10/10] android: Remove CameraWorker","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]