[{"id":21485,"web_url":"https://patchwork.libcamera.org/comment/21485/","msgid":"","date":"2021-12-01T02:15:58","subject":"Re: [libcamera-devel] [PATCH v3 11/11] 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 Wed, Dec 01, 2021 at 12:36:34AM +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> ---\n> src/android/camera_device.cpp | 46 ++++++------\n> src/android/camera_device.h | 3 -\n> src/android/camera_request.cpp | 8 +-\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, 36 insertions(+), 237 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 1938b10509fa..b7114c615733 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -15,10 +15,12 @@\n> \n> #include \n> #include \n> +#include \n> #include \n> \n> #include \n> #include \n> +#include \n> #include \n> #include \n> \n> @@ -406,7 +408,6 @@ void CameraDevice::flush()\n> \t\tstate_ = State::Flushing;\n> \t}\n> \n> -\tworker_.stop();\n> \tcamera_->stop();\n> \n> \tMutexLocker stateLock(stateMutex_);\n> @@ -419,7 +420,6 @@ void CameraDevice::stop()\n> \tif (state_ == State::Stopped)\n> \t\treturn;\n> \n> -\tworker_.stop();\n> \tcamera_->stop();\n> \n> \tdescriptors_ = {};\n> @@ -912,13 +912,9 @@ 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\tswitch (cameraStream->type()) {\n> \t\tcase CameraStream::Type::Mapped:\n> \t\t\t/*\n> @@ -943,7 +939,6 @@ 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\tLOG(HAL, Debug) << ss.str() << \" (direct)\";\n> \t\t\tbreak;\n> \n> @@ -970,12 +965,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \t\t}\n> \n> \t\tdescriptor->request_->addBuffer(cameraStream->stream(),\n> -\t\t\t\t\t\tframeBuffer, acquireFence);\n> +\t\t\tframeBuffer, std::make_unique(std::move(buffer.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> @@ -1001,26 +996,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> @@ -1042,16 +1034,24 @@ 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 the fence has been closed, send -1 to the framework. */\n> +\t\t\tstd::unique_ptr fence = buffer.frameBuffer->resetFence();\n> +\t\t\tif (!fence || !fence->isValid())\n\nI'm still not thrilled by the fact that we have two checks that both\nreport the same condition. Let's leave this aside for now, we can\naddress the issue by improving the API on top (a \\todo somewhere would\nbe good though).\n\n> +\t\t\t\tbreak;\n\nIs this correct, don't you need to process the other buffers ? It seems\nthat\n\n\t\t\t/*\n\t\t\t * If handling of the fence has failed, send to the\n\t\t\t * framework the acquire_fence fd as release_fence so\n\t\t\t * it can close it.\n\t\t\t */\n\t\t\tstd::unique_ptr fence = buffer.frameBuffer->resetFence();\n\t\t\tif (fence)\n\t\t\t\tbuffer.fence = fence->reset();\n\nshould be enough.\n\n> +\n> +\t\t\t/*\n> +\t\t\t * If handling of the fence has failed, send to the\n> +\t\t\t * framework the acquire_fence fd as release_fence so\n> +\t\t\t * it can close it.\n> +\t\t\t */\n> +\t\t\tbuffer.fence = std::move(fence->reset());\n\nNo need for std::move().\n\n> +\t\t}\n> \t\tbuffer.status = Camera3RequestDescriptor::Status::Success;\n> \t}\n> \n> @@ -1172,7 +1172,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> @@ -1186,7 +1186,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 51fe7da2cb47..494d79d2a3ea 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> @@ -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_;\n> \n> diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp\n> index 8162aa78e953..c46689a52d6d 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> @@ -57,8 +56,11 @@ Camera3RequestDescriptor::StreamBuffer::StreamBuffer(\n> \tCameraStream *cameraStream, const camera3_stream_buffer_t &buffer,\n> \tCamera3RequestDescriptor *requestDescriptor)\n> \t: stream(cameraStream), camera3Buffer(buffer.buffer),\n> -\t fence(buffer.acquire_fence), request(requestDescriptor)\n> +\t request(requestDescriptor)\n> {\n> +\t/* Do not reset buffer.acquire_fence by moving it to fence. */\n> +\tint fenceFd = buffer.acquire_fence;\n> +\tfence = UniqueFD(std::move(fenceFd));\n\nWould it be a problem to move buffer.acquire_fence ?\n\n> }\n> \n> Camera3RequestDescriptor::StreamBuffer::~StreamBuffer() = default;\n> diff --git a/src/android/camera_request.h b/src/android/camera_request.h\n> index f3cb6d643961..4372ad775c19 100644\n> --- a/src/android/camera_request.h\n> +++ b/src/android/camera_request.h\n> @@ -13,6 +13,7 @@\n> #include \n> \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> @@ -71,7 +71,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 9023c13c40f0..08252341e9cf 100644\n> --- a/src/android/camera_stream.cpp\n> +++ b/src/android/camera_stream.cpp\n> @@ -169,16 +169,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 9E9A8BDB13\n\tfor ;\n\tWed, 1 Dec 2021 02:16:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 051E060592;\n\tWed, 1 Dec 2021 03:16:27 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 80CBC60592\n\tfor ;\n\tWed, 1 Dec 2021 03:16:25 +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 8225F8AE;\n\tWed, 1 Dec 2021 03:16:24 +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=\"ZeZ5jN22\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1638324985;\n\tbh=c7e/qgbpsugYQ9WCRM6p9AZO0QsR46Wooes02ZA9tP8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ZeZ5jN22aJwMaIUq06BMPANOlo4Q93WKeIbsR+hiMHV/0CLrWOphS7aKZSp3oerWJ\n\tuWSB65an5Xl5Qmwe4gpBSB3p0L7R9mHZko81Ht5oq2uGSVu1m+ZPLl214CrQ8ox6JV\n\t3AWi+u5LbNtXstWZdvN6kgu3ySy2IMGl6fFJ2Nwk=","Date":"Wed, 1 Dec 2021 04:15:58 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20211130233634.34173-1-jacopo@jmondi.org>\n\t<20211130233634.34173-12-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211130233634.34173-12-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v3 11/11] 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\" "}},{"id":21510,"web_url":"https://patchwork.libcamera.org/comment/21510/","msgid":"<20211201104249.4zx26nq6bpmmrq43@uno.localdomain>","date":"2021-12-01T10:42:49","subject":"Re: [libcamera-devel] [PATCH v3 11/11] android: Remove CameraWorker","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent\n\nOn Wed, Dec 01, 2021 at 04:15:58AM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Wed, Dec 01, 2021 at 12:36:34AM +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> > ---\n> > src/android/camera_device.cpp | 46 ++++++------\n> > src/android/camera_device.h | 3 -\n> > src/android/camera_request.cpp | 8 +-\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, 36 insertions(+), 237 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 1938b10509fa..b7114c615733 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -15,10 +15,12 @@\n> >\n> > #include \n> > #include \n> > +#include \n> > #include \n> >\n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> >\n> > @@ -406,7 +408,6 @@ void CameraDevice::flush()\n> > \t\tstate_ = State::Flushing;\n> > \t}\n> >\n> > -\tworker_.stop();\n> > \tcamera_->stop();\n> >\n> > \tMutexLocker stateLock(stateMutex_);\n> > @@ -419,7 +420,6 @@ void CameraDevice::stop()\n> > \tif (state_ == State::Stopped)\n> > \t\treturn;\n> >\n> > -\tworker_.stop();\n> > \tcamera_->stop();\n> >\n> > \tdescriptors_ = {};\n> > @@ -912,13 +912,9 @@ 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\tswitch (cameraStream->type()) {\n> > \t\tcase CameraStream::Type::Mapped:\n> > \t\t\t/*\n> > @@ -943,7 +939,6 @@ 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\tLOG(HAL, Debug) << ss.str() << \" (direct)\";\n> > \t\t\tbreak;\n> >\n> > @@ -970,12 +965,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > \t\t}\n> >\n> > \t\tdescriptor->request_->addBuffer(cameraStream->stream(),\n> > -\t\t\t\t\t\tframeBuffer, acquireFence);\n> > +\t\t\tframeBuffer, std::make_unique(std::move(buffer.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> > @@ -1001,26 +996,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> > @@ -1042,16 +1034,24 @@ 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 the fence has been closed, send -1 to the framework. */\n> > +\t\t\tstd::unique_ptr fence = buffer.frameBuffer->resetFence();\n> > +\t\t\tif (!fence || !fence->isValid())\n>\n> I'm still not thrilled by the fact that we have two checks that both\n> report the same condition. Let's leave this aside for now, we can\n> address the issue by improving the API on top (a \\todo somewhere would\n> be good though).\n>\n> > +\t\t\t\tbreak;\n>\n> Is this correct, don't you need to process the other buffers ? It seems\n> that\n>\n> \t\t\t/*\n> \t\t\t * If handling of the fence has failed, send to the\n> \t\t\t * framework the acquire_fence fd as release_fence so\n> \t\t\t * it can close it.\n> \t\t\t */\n> \t\t\tstd::unique_ptr fence = buffer.frameBuffer->resetFence();\n> \t\t\tif (fence)\n> \t\t\t\tbuffer.fence = fence->reset();\n>\n> should be enough.\n>\n\nUps, I was sure the break was there already, I wen throught too many\nversions! Indeed other buffers should be processed (and the buffer\nstatus set!). It's weird this doesn't cause huge issues :/\n\n\n> > +\n> > +\t\t\t/*\n> > +\t\t\t * If handling of the fence has failed, send to the\n> > +\t\t\t * framework the acquire_fence fd as release_fence so\n> > +\t\t\t * it can close it.\n> > +\t\t\t */\n> > +\t\t\tbuffer.fence = std::move(fence->reset());\n>\n> No need for std::move().\n>\n> > +\t\t}\n> > \t\tbuffer.status = Camera3RequestDescriptor::Status::Success;\n> > \t}\n> >\n> > @@ -1172,7 +1172,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> > @@ -1186,7 +1186,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 51fe7da2cb47..494d79d2a3ea 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> > @@ -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_;\n> >\n> > diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp\n> > index 8162aa78e953..c46689a52d6d 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> > @@ -57,8 +56,11 @@ Camera3RequestDescriptor::StreamBuffer::StreamBuffer(\n> > \tCameraStream *cameraStream, const camera3_stream_buffer_t &buffer,\n> > \tCamera3RequestDescriptor *requestDescriptor)\n> > \t: stream(cameraStream), camera3Buffer(buffer.buffer),\n> > -\t fence(buffer.acquire_fence), request(requestDescriptor)\n> > +\t request(requestDescriptor)\n> > {\n> > +\t/* Do not reset buffer.acquire_fence by moving it to fence. */\n> > +\tint fenceFd = buffer.acquire_fence;\n> > +\tfence = UniqueFD(std::move(fenceFd));\n>\n> Would it be a problem to move buffer.acquire_fence ?\n\nI don't know. There are no requirements about that that I found, but I\nthought that not resetting buffer.acquire_fence (which if I'm not\nmistaken is owned by the framework) was safer.. Probably it's not an\nissue but who knows what the camera service does with it...\n\n>\n> > }\n> >\n> > Camera3RequestDescriptor::StreamBuffer::~StreamBuffer() = default;\n> > diff --git a/src/android/camera_request.h b/src/android/camera_request.h\n> > index f3cb6d643961..4372ad775c19 100644\n> > --- a/src/android/camera_request.h\n> > +++ b/src/android/camera_request.h\n> > @@ -13,6 +13,7 @@\n> > #include \n> >\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> > @@ -71,7 +71,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 9023c13c40f0..08252341e9cf 100644\n> > --- a/src/android/camera_stream.cpp\n> > +++ b/src/android/camera_stream.cpp\n> > @@ -169,16 +169,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',\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 4532CBF415\n\tfor ;\n\tWed, 1 Dec 2021 10:42:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8643860723;\n\tWed, 1 Dec 2021 11:41:59 +0100 (CET)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4D92E6011D\n\tfor ;\n\tWed, 1 Dec 2021 11:41:58 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id 06F7724000B;\n\tWed, 1 Dec 2021 10:41:56 +0000 (UTC)"],"Date":"Wed, 1 Dec 2021 11:42:49 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20211201104249.4zx26nq6bpmmrq43@uno.localdomain>","References":"<20211130233634.34173-1-jacopo@jmondi.org>\n\t<20211130233634.34173-12-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 11/11] 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\" "}},{"id":21518,"web_url":"https://patchwork.libcamera.org/comment/21518/","msgid":"","date":"2021-12-01T11:59:46","subject":"Re: [libcamera-devel] [PATCH v3 11/11] android: Remove CameraWorker","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Dec 01, 2021 at 11:42:49AM +0100, Jacopo Mondi wrote:\n> Hi Laurent\n> \n> On Wed, Dec 01, 2021 at 04:15:58AM +0200, Laurent Pinchart wrote:\n> > Hi Jacopo,\n> >\n> > Thank you for the patch.\n> >\n> > On Wed, Dec 01, 2021 at 12:36:34AM +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> > > ---\n> > > src/android/camera_device.cpp | 46 ++++++------\n> > > src/android/camera_device.h | 3 -\n> > > src/android/camera_request.cpp | 8 +-\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, 36 insertions(+), 237 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 1938b10509fa..b7114c615733 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -15,10 +15,12 @@\n> > >\n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > >\n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > >\n> > > @@ -406,7 +408,6 @@ void CameraDevice::flush()\n> > > \t\tstate_ = State::Flushing;\n> > > \t}\n> > >\n> > > -\tworker_.stop();\n> > > \tcamera_->stop();\n> > >\n> > > \tMutexLocker stateLock(stateMutex_);\n> > > @@ -419,7 +420,6 @@ void CameraDevice::stop()\n> > > \tif (state_ == State::Stopped)\n> > > \t\treturn;\n> > >\n> > > -\tworker_.stop();\n> > > \tcamera_->stop();\n> > >\n> > > \tdescriptors_ = {};\n> > > @@ -912,13 +912,9 @@ 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\tswitch (cameraStream->type()) {\n> > > \t\tcase CameraStream::Type::Mapped:\n> > > \t\t\t/*\n> > > @@ -943,7 +939,6 @@ 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\tLOG(HAL, Debug) << ss.str() << \" (direct)\";\n> > > \t\t\tbreak;\n> > >\n> > > @@ -970,12 +965,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > > \t\t}\n> > >\n> > > \t\tdescriptor->request_->addBuffer(cameraStream->stream(),\n> > > -\t\t\t\t\t\tframeBuffer, acquireFence);\n> > > +\t\t\tframeBuffer, std::make_unique(std::move(buffer.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> > > @@ -1001,26 +996,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> > > @@ -1042,16 +1034,24 @@ 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 the fence has been closed, send -1 to the framework. */\n> > > +\t\t\tstd::unique_ptr fence = buffer.frameBuffer->resetFence();\n> > > +\t\t\tif (!fence || !fence->isValid())\n> >\n> > I'm still not thrilled by the fact that we have two checks that both\n> > report the same condition. Let's leave this aside for now, we can\n> > address the issue by improving the API on top (a \\todo somewhere would\n> > be good though).\n> >\n> > > +\t\t\t\tbreak;\n> >\n> > Is this correct, don't you need to process the other buffers ? It seems\n> > that\n> >\n> > \t\t\t/*\n> > \t\t\t * If handling of the fence has failed, send to the\n> > \t\t\t * framework the acquire_fence fd as release_fence so\n> > \t\t\t * it can close it.\n> > \t\t\t */\n> > \t\t\tstd::unique_ptr fence = buffer.frameBuffer->resetFence();\n> > \t\t\tif (fence)\n> > \t\t\t\tbuffer.fence = fence->reset();\n> >\n> > should be enough.\n> >\n> \n> Ups, I was sure the break was there already, I wen throught too many\n> versions! Indeed other buffers should be processed (and the buffer\n> status set!). It's weird this doesn't cause huge issues :/\n> \n> \n> > > +\n> > > +\t\t\t/*\n> > > +\t\t\t * If handling of the fence has failed, send to the\n> > > +\t\t\t * framework the acquire_fence fd as release_fence so\n> > > +\t\t\t * it can close it.\n> > > +\t\t\t */\n> > > +\t\t\tbuffer.fence = std::move(fence->reset());\n> >\n> > No need for std::move().\n> >\n> > > +\t\t}\n> > > \t\tbuffer.status = Camera3RequestDescriptor::Status::Success;\n> > > \t}\n> > >\n> > > @@ -1172,7 +1172,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> > > @@ -1186,7 +1186,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 51fe7da2cb47..494d79d2a3ea 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> > > @@ -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_;\n> > >\n> > > diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp\n> > > index 8162aa78e953..c46689a52d6d 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> > > @@ -57,8 +56,11 @@ Camera3RequestDescriptor::StreamBuffer::StreamBuffer(\n> > > \tCameraStream *cameraStream, const camera3_stream_buffer_t &buffer,\n> > > \tCamera3RequestDescriptor *requestDescriptor)\n> > > \t: stream(cameraStream), camera3Buffer(buffer.buffer),\n> > > -\t fence(buffer.acquire_fence), request(requestDescriptor)\n> > > +\t request(requestDescriptor)\n> > > {\n> > > +\t/* Do not reset buffer.acquire_fence by moving it to fence. */\n> > > +\tint fenceFd = buffer.acquire_fence;\n> > > +\tfence = UniqueFD(std::move(fenceFd));\n> >\n> > Would it be a problem to move buffer.acquire_fence ?\n> \n> I don't know. There are no requirements about that that I found, but I\n> thought that not resetting buffer.acquire_fence (which if I'm not\n> mistaken is owned by the framework) was safer.. Probably it's not an\n> issue but who knows what the camera service does with it...\n\nOnwership of the fence is transferred to the HAL when the request is\nqueued, isn't it ?\n\n> > > }\n> > >\n> > > Camera3RequestDescriptor::StreamBuffer::~StreamBuffer() = default;\n> > > diff --git a/src/android/camera_request.h b/src/android/camera_request.h\n> > > index f3cb6d643961..4372ad775c19 100644\n> > > --- a/src/android/camera_request.h\n> > > +++ b/src/android/camera_request.h\n> > > @@ -13,6 +13,7 @@\n> > > #include \n> > >\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> > > @@ -71,7 +71,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 9023c13c40f0..08252341e9cf 100644\n> > > --- a/src/android/camera_stream.cpp\n> > > +++ b/src/android/camera_stream.cpp\n> > > @@ -169,16 +169,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 8DE0FBDB13\n\tfor ;\n\tWed, 1 Dec 2021 12:00:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E608E6071A;\n\tWed, 1 Dec 2021 13:00: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 514B56011A\n\tfor ;\n\tWed, 1 Dec 2021 13:00: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 81E76A15;\n\tWed, 1 Dec 2021 13:00: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=\"vbWA804c\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1638360011;\n\tbh=T7Z5iarzMoqdYARnU5Q9ZyfAdbNNKOVTf/jp3QrYqjU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=vbWA804ca/cRBaHL+IlnC5BqHNEya7vSFDk5O44KNbO0U20H7EeZrcBq2MhTRMjlS\n\tRNJyED6DHBBE+V+CFUbj/iNJpdY5Ctq7IyFTBu20aaeErQ8Rr38tuOI9Vwxq5O7fHI\n\tFtVrTGVmFwBflJCovAhhUcyYfiu7qi09hV34hfKU=","Date":"Wed, 1 Dec 2021 13:59:46 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20211130233634.34173-1-jacopo@jmondi.org>\n\t<20211130233634.34173-12-jacopo@jmondi.org>\n\t\n\t<20211201104249.4zx26nq6bpmmrq43@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211201104249.4zx26nq6bpmmrq43@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 11/11] 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\" "}},{"id":21519,"web_url":"https://patchwork.libcamera.org/comment/21519/","msgid":"<20211201121016.u4qic3qpjqnjsrt4@uno.localdomain>","date":"2021-12-01T12:10:16","subject":"Re: [libcamera-devel] [PATCH v3 11/11] android: Remove CameraWorker","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent\n\nOn Wed, Dec 01, 2021 at 01:59:46PM +0200, Laurent Pinchart wrote:\n> On Wed, Dec 01, 2021 at 11:42:49AM +0100, Jacopo Mondi wrote:\n> > > >\n> > > > Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;\n> > > > @@ -57,8 +56,11 @@ Camera3RequestDescriptor::StreamBuffer::StreamBuffer(\n> > > > \tCameraStream *cameraStream, const camera3_stream_buffer_t &buffer,\n> > > > \tCamera3RequestDescriptor *requestDescriptor)\n> > > > \t: stream(cameraStream), camera3Buffer(buffer.buffer),\n> > > > -\t fence(buffer.acquire_fence), request(requestDescriptor)\n> > > > +\t request(requestDescriptor)\n> > > > {\n> > > > +\t/* Do not reset buffer.acquire_fence by moving it to fence. */\n> > > > +\tint fenceFd = buffer.acquire_fence;\n> > > > +\tfence = UniqueFD(std::move(fenceFd));\n> > >\n> > > Would it be a problem to move buffer.acquire_fence ?\n> >\n> > I don't know. There are no requirements about that that I found, but I\n> > thought that not resetting buffer.acquire_fence (which if I'm not\n> > mistaken is owned by the framework) was safer.. Probably it's not an\n> > issue but who knows what the camera service does with it...\n>\n> Onwership of the fence is transferred to the HAL when the request is\n> queued, isn't it ?\n>\n\nYes, at the same time the HAL is suggested to copy in the values\npassed in by the framework.\n\nI can however try and if nothing seems wrong move buffer.fence without\ngoing through a temporary variable.\n\n> > > > }\n> > > >\n> > > > Camera3RequestDescriptor::StreamBuffer::~StreamBuffer() = default;\n> > > > diff --git a/src/android/camera_request.h b/src/android/camera_request.h\n> > > > index f3cb6d643961..4372ad775c19 100644\n> > > > --- a/src/android/camera_request.h\n> > > > +++ b/src/android/camera_request.h\n> > > > @@ -13,6 +13,7 @@\n> > > > #include \n> > > >\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> > > > @@ -71,7 +71,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 9023c13c40f0..08252341e9cf 100644\n> > > > --- a/src/android/camera_stream.cpp\n> > > > +++ b/src/android/camera_stream.cpp\n> > > > @@ -169,16 +169,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',\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 F32FBBF415\n\tfor ;\n\tWed, 1 Dec 2021 12:09:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 50764604FC;\n\tWed, 1 Dec 2021 13:09:26 +0100 (CET)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3DC736011A\n\tfor ;\n\tWed, 1 Dec 2021 13:09:25 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 48DD6C0005;\n\tWed, 1 Dec 2021 12:09:24 +0000 (UTC)"],"Date":"Wed, 1 Dec 2021 13:10:16 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20211201121016.u4qic3qpjqnjsrt4@uno.localdomain>","References":"<20211130233634.34173-1-jacopo@jmondi.org>\n\t<20211130233634.34173-12-jacopo@jmondi.org>\n\t\n\t<20211201104249.4zx26nq6bpmmrq43@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 11/11] 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\" "}}]