[{"id":21672,"web_url":"https://patchwork.libcamera.org/comment/21672/","msgid":"","date":"2021-12-08T12:02:42","subject":"Re: [libcamera-devel] [v4.1] android: Remove CameraWorker","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 Tue, Dec 07, 2021 at 12:17:10PM +0100, Jacopo Mondi wrote:\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> \n> While at it make CameraRequest::StreamBuffer::fence a UniqueFD to ease\n> the management of the fences file descriptor values.\n> \n> Signed-off-by: Jacopo Mondi \n\nReviewed-by: Laurent Pinchart \n\n> ---\n> \n> Rebased on top of the latest version of UniqueFD, which does not include\n> patch:\n> \"[v4 22/22] libcamera: base: unique_fd: Pass rvalue reference to constructor and reset()\"\n> \n> ---\n> \n> src/android/camera_device.cpp | 42 +++++------\n> src/android/camera_device.h | 3 -\n> src/android/camera_request.cpp | 3 +-\n> src/android/camera_request.h | 6 +-\n> src/android/camera_stream.cpp | 10 +--\n> src/android/camera_worker.cpp | 129 ---------------------------------\n> src/android/camera_worker.h | 70 ------------------\n> src/android/meson.build | 1 -\n> 8 files changed, 28 insertions(+), 236 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 9f772b58d80d..f28fdd6a2955 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -14,10 +14,12 @@\n> #include \n> \n> #include \n> +#include \n> #include \n> \n> #include \n> #include \n> +#include \n> #include \n> #include \n> \n> @@ -420,7 +422,6 @@ void CameraDevice::flush()\n> \t\tstate_ = State::Flushing;\n> \t}\n> \n> -\tworker_.stop();\n> \tcamera_->stop();\n> \n> \tMutexLocker stateLock(stateMutex_);\n> @@ -433,7 +434,6 @@ void CameraDevice::stop()\n> \tif (state_ == State::Stopped)\n> \t\treturn;\n> \n> -\tworker_.stop();\n> \tcamera_->stop();\n> \n> \t{\n> @@ -930,13 +930,10 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \n> \t\t/*\n> \t\t * Inspect the camera stream type, create buffers opportunely\n> -\t\t * and add them to the Request if required. Only acquire fences\n> -\t\t * for streams of type Direct are handled by the CameraWorker,\n> -\t\t * while fences for streams of type Internal and Mapped are\n> -\t\t * handled at post-processing time.\n> +\t\t * and add them to the Request if required.\n> \t\t */\n> \t\tFrameBuffer *frameBuffer = nullptr;\n> -\t\tint acquireFence = -1;\n> +\t\tUniqueFD acquireFence;\n> \n> \t\tMutexLocker lock(descriptor->streamsProcessMutex_);\n> \n> @@ -964,7 +961,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \t\t\t\t\t\t cameraStream->configuration().pixelFormat,\n> \t\t\t\t\t\t cameraStream->configuration().size);\n> \t\t\tframeBuffer = buffer.frameBuffer.get();\n> -\t\t\tacquireFence = buffer.fence;\n> +\t\t\tacquireFence = std::move(buffer.fence);\n> \t\t\tLOG(HAL, Debug) << ss.str() << \" (direct)\";\n> \t\t\tbreak;\n> \n> @@ -990,13 +987,14 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \t\t\treturn -ENOMEM;\n> \t\t}\n> \n> +\t\tauto fence = std::make_unique(std::move(acquireFence));\n> \t\tdescriptor->request_->addBuffer(cameraStream->stream(),\n> -\t\t\t\t\t\tframeBuffer, acquireFence);\n> +\t\t\t\t\t\tframeBuffer, std::move(fence));\n> \t}\n> \n> \t/*\n> \t * Translate controls from Android to libcamera and queue the request\n> -\t * to the CameraWorker thread.\n> +\t * to the camera.\n> \t */\n> \tint ret = processControls(descriptor.get());\n> \tif (ret)\n> @@ -1022,26 +1020,23 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \t}\n> \n> \tif (state_ == State::Stopped) {\n> -\t\tworker_.start();\n> -\n> \t\tret = camera_->start();\n> \t\tif (ret) {\n> \t\t\tLOG(HAL, Error) << \"Failed to start camera\";\n> -\t\t\tworker_.stop();\n> \t\t\treturn ret;\n> \t\t}\n> \n> \t\tstate_ = State::Running;\n> \t}\n> \n> -\tCaptureRequest *request = descriptor->request_.get();\n> +\tRequest *request = descriptor->request_.get();\n> \n> \t{\n> \t\tMutexLocker descriptorsLock(descriptorsMutex_);\n> \t\tdescriptors_.push(std::move(descriptor));\n> \t}\n> \n> -\tworker_.queueRequest(request);\n> +\tcamera_->queueRequest(request);\n> \n> \treturn 0;\n> }\n> @@ -1063,16 +1058,17 @@ void CameraDevice::requestComplete(Request *request)\n> \t\t/*\n> \t\t * Streams of type Direct have been queued to the\n> \t\t * libcamera::Camera and their acquire fences have\n> -\t\t * already been waited on by the CameraWorker.\n> +\t\t * already been waited on by the library.\n> \t\t *\n> \t\t * Acquire fences of streams of type Internal and Mapped\n> \t\t * will be handled during post-processing.\n> -\t\t *\n> -\t\t * \\todo Instrument the CameraWorker to set the acquire\n> -\t\t * fence to -1 once it has handled it and remove this check.\n> \t\t */\n> -\t\tif (stream->type() == CameraStream::Type::Direct)\n> -\t\t\tbuffer.fence = -1;\n> +\t\tif (stream->type() == CameraStream::Type::Direct) {\n> +\t\t\t/* If handling of the fence has failed restore buffer.fence. */\n> +\t\t\tstd::unique_ptr fence = buffer.frameBuffer->releaseFence();\n> +\t\t\tif (fence)\n> +\t\t\t\tbuffer.fence = fence->release();\n> +\t\t}\n> \t\tbuffer.status = Camera3RequestDescriptor::Status::Success;\n> \t}\n> \n> @@ -1193,7 +1189,7 @@ void CameraDevice::sendCaptureResults()\n> \t\tstd::vector resultBuffers;\n> \t\tresultBuffers.reserve(descriptor->buffers_.size());\n> \n> -\t\tfor (const auto &buffer : descriptor->buffers_) {\n> +\t\tfor (auto &buffer : descriptor->buffers_) {\n> \t\t\tcamera3_buffer_status status = CAMERA3_BUFFER_STATUS_ERROR;\n> \n> \t\t\tif (buffer.status == Camera3RequestDescriptor::Status::Success)\n> @@ -1207,7 +1203,7 @@ void CameraDevice::sendCaptureResults()\n> \t\t\t */\n> \t\t\tresultBuffers.push_back({ buffer.stream->camera3Stream(),\n> \t\t\t\t\t\t buffer.camera3Buffer, status,\n> -\t\t\t\t\t\t -1, buffer.fence });\n> +\t\t\t\t\t\t -1, buffer.fence.release() });\n> \t\t}\n> \n> \t\tcaptureResult.num_output_buffers = resultBuffers.size();\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index e5d9cda2ce3a..64050416e67e 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -29,7 +29,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> @@ -105,8 +104,6 @@ private:\n> \tunsigned int id_;\n> \tcamera3_device_t camera3Device_;\n> \n> -\tCameraWorker worker_;\n> -\n> \tlibcamera::Mutex stateMutex_; /* Protects access to the camera state. */\n> \tState state_ LIBCAMERA_TSA_GUARDED_BY(stateMutex_);\n> \n> diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp\n> index 8162aa78e953..917769070cd9 100644\n> --- a/src/android/camera_request.cpp\n> +++ b/src/android/camera_request.cpp\n> @@ -47,8 +47,7 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(\n> \t * Create the CaptureRequest, stored as a unique_ptr<> to tie its\n> \t * lifetime to the descriptor.\n> \t */\n> -\trequest_ = std::make_unique(camera,\n> -\t\t\t\t\t\t reinterpret_cast(this));\n> +\trequest_ = 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 d9b04f166fd7..37b6ae326f3e 100644\n> --- a/src/android/camera_request.h\n> +++ b/src/android/camera_request.h\n> @@ -13,6 +13,7 @@\n> \n> #include \n> #include \n> +#include \n> \n> #include \n> #include \n> @@ -20,7 +21,6 @@\n> #include \n> \n> #include \"camera_metadata.h\"\n> -#include \"camera_worker.h\"\n> \n> class CameraBuffer;\n> class CameraStream;\n> @@ -45,7 +45,7 @@ public:\n> \t\tCameraStream *stream;\n> \t\tbuffer_handle_t *camera3Buffer;\n> \t\tstd::unique_ptr frameBuffer;\n> -\t\tint fence;\n> +\t\tlibcamera::UniqueFD fence;\n> \t\tStatus status = Status::Success;\n> \t\tlibcamera::FrameBuffer *internalBuffer = nullptr;\n> \t\tconst libcamera::FrameBuffer *srcBuffer = nullptr;\n> @@ -72,7 +72,7 @@ public:\n> \tstd::vector buffers_;\n> \n> \tCameraMetadata settings_;\n> -\tstd::unique_ptr request_;\n> +\tstd::unique_ptr request_;\n> \tstd::unique_ptr resultMetadata_;\n> \n> \tbool complete_ = false;\n> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> index ee9bbe48993a..c21574501916 100644\n> --- a/src/android/camera_stream.cpp\n> +++ b/src/android/camera_stream.cpp\n> @@ -170,16 +170,16 @@ int CameraStream::process(Camera3RequestDescriptor::StreamBuffer *streamBuffer)\n> \tASSERT(type_ != Type::Direct);\n> \n> \t/* Handle waiting on fences on the destination buffer. */\n> -\tif (streamBuffer->fence != -1) {\n> -\t\tint ret = waitFence(streamBuffer->fence);\n> +\tif (streamBuffer->fence.isValid()) {\n> +\t\tint ret = waitFence(streamBuffer->fence.get());\n> \t\tif (ret < 0) {\n> \t\t\tLOG(HAL, Error) << \"Failed waiting for fence: \"\n> -\t\t\t\t\t<< streamBuffer->fence << \": \" << strerror(-ret);\n> +\t\t\t\t\t<< streamBuffer->fence.get() << \": \"\n> +\t\t\t\t\t<< strerror(-ret);\n> \t\t\treturn ret;\n> \t\t}\n> \n> -\t\t::close(streamBuffer->fence);\n> -\t\tstreamBuffer->fence = -1;\n> +\t\tstreamBuffer->fence.reset();\n> \t}\n> \n> \tconst StreamConfiguration &output = configuration();\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> -\t: camera_(camera)\n> -{\n> -\trequest_ = camera_->createRequest(cookie);\n> -}\n> -\n> -void CaptureRequest::addBuffer(Stream *stream, FrameBuffer *buffer, int fence)\n> -{\n> -\trequest_->addBuffer(stream, buffer);\n> -\tacquireFences_.push_back(fence);\n> -}\n> -\n> -void CaptureRequest::queue()\n> -{\n> -\tcamera_->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> -\tworker_.moveToThread(this);\n> -}\n> -\n> -void CameraWorker::start()\n> -{\n> -\tThread::start();\n> -}\n> -\n> -void CameraWorker::stop()\n> -{\n> -\texit();\n> -\twait();\n> -}\n> -\n> -void CameraWorker::run()\n> -{\n> -\texec();\n> -\tdispatchMessages(Message::Type::InvokeMessage);\n> -}\n> -\n> -void CameraWorker::queueRequest(CaptureRequest *request)\n> -{\n> -\t/* Async process the request on the worker which runs its own thread. */\n> -\tworker_.invokeMethod(&Worker::processRequest, ConnectionTypeQueued,\n> -\t\t\t 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> -\t/*\n> -\t * \\todo Better characterize the timeout. Currently equal to the one\n> -\t * used by the Rockchip Camera HAL on ChromeOS.\n> -\t */\n> -\tconstexpr unsigned int timeoutMs = 300;\n> -\tstruct pollfd fds = { fence, POLLIN, 0 };\n> -\n> -\tdo {\n> -\t\tint ret = poll(&fds, 1, timeoutMs);\n> -\t\tif (ret == 0)\n> -\t\t\treturn -ETIME;\n> -\n> -\t\tif (ret > 0) {\n> -\t\t\tif (fds.revents & (POLLERR | POLLNVAL))\n> -\t\t\t\treturn -EINVAL;\n> -\n> -\t\t\treturn 0;\n> -\t\t}\n> -\t} while (errno == EINTR || errno == EAGAIN);\n> -\n> -\treturn -errno;\n> -}\n> -\n> -void CameraWorker::Worker::processRequest(CaptureRequest *request)\n> -{\n> -\t/* Wait on all fences before queuing the Request. */\n> -\tfor (int fence : request->fences()) {\n> -\t\tif (fence == -1)\n> -\t\t\tcontinue;\n> -\n> -\t\tint ret = waitFence(fence);\n> -\t\tclose(fence);\n> -\t\tif (ret < 0) {\n> -\t\t\tLOG(HAL, Error) << \"Failed waiting for fence: \"\n> -\t\t\t\t\t<< fence << \": \" << strerror(-ret);\n> -\t\t\treturn;\n> -\t\t}\n> -\t}\n> -\n> -\trequest->queue();\n> -}\n> diff --git a/src/android/camera_worker.h b/src/android/camera_worker.h\n> deleted file mode 100644\n> index 26ecc2732046..000000000000\n> --- a/src/android/camera_worker.h\n> +++ /dev/null\n> @@ -1,70 +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> -\n> -#pragma once\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> -\tCaptureRequest(libcamera::Camera *camera, uint64_t cookie);\n> -\n> -\tconst std::vector &fences() const { return acquireFences_; }\n> -\tlibcamera::ControlList &controls() { return request_->controls(); }\n> -\tconst libcamera::ControlList &metadata() const\n> -\t{\n> -\t\treturn request_->metadata();\n> -\t}\n> -\tunsigned long cookie() const { return request_->cookie(); }\n> -\n> -\tvoid addBuffer(libcamera::Stream *stream,\n> -\t\t libcamera::FrameBuffer *buffer, int fence);\n> -\tvoid queue();\n> -\n> -private:\n> -\tlibcamera::Camera *camera_;\n> -\tstd::vector acquireFences_;\n> -\tstd::unique_ptr request_;\n> -};\n> -\n> -class CameraWorker : private libcamera::Thread\n> -{\n> -public:\n> -\tCameraWorker();\n> -\n> -\tvoid start();\n> -\tvoid stop();\n> -\n> -\tvoid queueRequest(CaptureRequest *request);\n> -\n> -protected:\n> -\tvoid run() override;\n> -\n> -private:\n> -\tclass Worker : public libcamera::Object\n> -\t{\n> -\tpublic:\n> -\t\tvoid processRequest(CaptureRequest *request);\n> -\n> -\tprivate:\n> -\t\tint waitFence(int fence);\n> -\t};\n> -\n> -\tWorker worker_;\n> -};\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',","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 6CBD7BF415\n\tfor ;\n\tWed, 8 Dec 2021 12:03:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C0B856087A;\n\tWed, 8 Dec 2021 13:03:13 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 182C860225\n\tfor ;\n\tWed, 8 Dec 2021 13:03:12 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6E7A18BB;\n\tWed, 8 Dec 2021 13:03:11 +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=\"s2V3HAjZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1638964991;\n\tbh=fX2VPm0hrJmX/gnmVLcaOi/0CpA5W4QYOGw1C5kTDYc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=s2V3HAjZSq1QhKlg/wmSfASF9gEWwir/9mkKXWeWONhsWEN9BLiAOSnoNFapZ2ok6\n\t/m6EFc7P+Nk2hBMr+cVugDrgM1KM+oIo4h+p4jxTPYijYwnis07Dp6CNjNv/qpmIs6\n\tP8l40lJaEo08kIPqicSnM5SeGXoZkl2AAUas8CIw=","Date":"Wed, 8 Dec 2021 14:02:42 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20211201142936.107405-1-jacopo@jmondi.org>\n\t<20211207111710.17550-1-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211207111710.17550-1-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [v4.1] 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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]