[{"id":21079,"web_url":"https://patchwork.libcamera.org/comment/21079/","msgid":"","date":"2021-11-21T19:00:00","subject":"Re: [libcamera-devel] [PATCH v2 10/12] 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 Sat, Nov 20, 2021 at 12:13:11PM +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> Signed-off-by: Jacopo Mondi \n> ---\n> src/android/camera_device.cpp | 43 +++++------\n> src/android/camera_device.h | 3 -\n> src/android/camera_request.cpp | 3 +-\n> src/android/camera_request.h | 3 +-\n> src/android/camera_worker.cpp | 129 ---------------------------------\n> src/android/camera_worker.h | 72 ------------------\n> src/android/meson.build | 1 -\n> 7 files changed, 24 insertions(+), 230 deletions(-)\n> delete mode 100644 src/android/camera_worker.cpp\n> delete mode 100644 src/android/camera_worker.h\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index f2e0bdbdbbf6..29f0abd96ba9 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -19,6 +19,7 @@\n> \n> #include \n> #include \n> +#include \n> #include \n> #include \n> \n> @@ -406,7 +407,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 +419,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 +911,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\tFileDescriptor fenceFd;\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,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\tfenceFd = FileDescriptor(std::move(buffer.fence));\n> \t\t\tLOG(HAL, Debug) << ss.str() << \" (direct)\";\n> \t\t\tbreak;\n> \n> @@ -969,13 +965,14 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \t\t\treturn -ENOMEM;\n> \t\t}\n> \n> +\t\tstd::unique_ptr fence = std::make_unique(fenceFd);\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\n\t\t\t\t\t\tframeBuffer,\n\t\t\t\t\t\tstd::make_unique(fenceFd));\n\ncould work too, up to you.\n\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 +998,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 +1036,23 @@ 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 * been handled by the library.\n> +\t\t *\n> +\t\t * If the fence has been signalled correctly it will be -1 and\n> +\t\t * closed, otherwise it is set to the 'acquire_fence' value and\n> +\t\t * the framework will have to close it.\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\tFence *fence = buffer.frameBuffer->fence();\n> +\t\t\tif (!fence)\n> +\t\t\t\tbreak;\n\nCan this happen ? If not I'd ASSERT(), if it can we need better error\nhandling.\n\n> +\n> +\t\t\tFileDescriptor fenceFd = std::move(fence->fd());\n\nThis won't work as you expect. fence is a Fence *, and Fence::fd()\nreturns a const FileDescriptor &. std::move() will thus return a const\nFileDescriptor &&. FileDescriptor has no assignment operator=() that\ntakes a *const* rvalue reference, so the copy assignment operator will\nbe called, creating a copy of FileDescriptor.\n\nThis in itself isn't a big issue, as copying a FileDescriptor won't\nduplicate the underlying fd. The construct is however misleading, so I'd\ndrop the std::move.\n\n> +\t\t\tbuffer.fence = fenceFd.fd();\n\nThis will also not work as expected, and it's a problem. The value\nreturned by fd() is valid, but only until the last FileDescriptor\ninstance referencing the same fd gets destroyed. Due to the copy above,\nthis happens when the FrameBuffer is destroyed (or added to a new\nrequest for applications that reuse FrameBuffer instances, as you call\ncloseFence() in addRequest()). The fd you send back to the camera\nservice will thus not be valid.\n\nTo fix this, I think we need\n\n- A FrameBuffer::resetFence() that returns a unique pointer to the\n fence, to make the lifetime of the fence deterministric when requests\n complete.\n\n- A way to extract the file descriptor from the fence without closing\n it. This requires a new function in the FileDescriptor class, which\n would be dangerous to call if there are multiple copies of the\n FileDescriptor. This is where a unique fd class could help.\n\n We could also have a Fence::resetFileDescriptor() function to extract\n the FileDescriptor from the Fence object, but we can call the new\n function to extract the numerical fd from the FileDescriptor directly\n on Fence::fd(), which should I think be good enough.\n\n> +\t\t}\n> \t\tbuffer.status = Camera3RequestDescriptor::Status::Success;\n> \t}\n> \n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index 2a414020f1ad..c8cc78688bec 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..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 8d1204e5590b..69e1fe9fc379 100644\n> --- a/src/android/camera_request.h\n> +++ b/src/android/camera_request.h\n> @@ -20,7 +20,6 @@\n> #include \n> \n> #include \"camera_metadata.h\"\n> -#include \"camera_worker.h\"\n> \n> class CameraBuffer;\n> class CameraStream;\n> @@ -71,7 +70,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_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 c94f16325925..000000000000\n> --- a/src/android/camera_worker.h\n> +++ /dev/null\n> @@ -1,72 +0,0 @@\n> -/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> -/*\n> - * Copyright (C) 2020, Google Inc.\n> - *\n> - * camera_worker.h - Process capture requests on behalf of the Camera HAL\n> - */\n> -#ifndef __ANDROID_CAMERA_WORKER_H__\n> -#define __ANDROID_CAMERA_WORKER_H__\n> -\n> -#include \n> -#include \n> -\n> -#include \n> -#include \n> -\n> -#include \n> -#include \n> -#include \n> -#include \n> -\n> -class CameraDevice;\n> -\n> -class CaptureRequest\n> -{\n> -public:\n> -\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> -\n> -#endif /* __ANDROID_CAMERA_WORKER_H__ */\n> diff --git a/src/android/meson.build b/src/android/meson.build\n> index 332b177ca805..75b4bf207085 100644\n> --- a/src/android/meson.build\n> +++ b/src/android/meson.build\n> @@ -47,7 +47,6 @@ android_hal_sources = files([\n> 'camera_ops.cpp',\n> 'camera_request.cpp',\n> 'camera_stream.cpp',\n> - 'camera_worker.cpp',\n> 'jpeg/encoder_libjpeg.cpp',\n> 'jpeg/exif.cpp',\n> 'jpeg/post_processor_jpeg.cpp',","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 E6EF7BDB13\n\tfor ;\n\tSun, 21 Nov 2021 19:00:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5E1C360371;\n\tSun, 21 Nov 2021 20:00:24 +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 1E38960233\n\tfor ;\n\tSun, 21 Nov 2021 20:00:23 +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 834F59DD;\n\tSun, 21 Nov 2021 20:00:22 +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=\"gj/Ae2rZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637521222;\n\tbh=BJb6keMki6sDeLRHY3OV1+HfIBEGuhL1FQTl2oFq8xU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=gj/Ae2rZ9Qy/oWNF/O86wK2i90TXRk6GfYqfPzWlSqrBOFDv2TkYGnAHcOGsWcJHW\n\t2L3trxpCdoAls3FwdnTy9qzeFzlPwUYfCuOgwOAoShJpbcpXO4/mDOunjhZskk0UAa\n\t/PzCym/Zfs+oKMlkL4MYznSBIBvR8e/oR7WZxXgY=","Date":"Sun, 21 Nov 2021 21:00:00 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20211120111313.106621-1-jacopo@jmondi.org>\n\t<20211120111313.106621-11-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211120111313.106621-11-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v2 10/12] 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":21283,"web_url":"https://patchwork.libcamera.org/comment/21283/","msgid":"<20211126122529.2wtfdxgngaa7lpa3@uno.localdomain>","date":"2021-11-26T12:25:29","subject":"Re: [libcamera-devel] [PATCH v2 10/12] 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 Sun, Nov 21, 2021 at 09:00:00PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Sat, Nov 20, 2021 at 12:13:11PM +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> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/android/camera_device.cpp | 43 +++++------\n> > src/android/camera_device.h | 3 -\n> > src/android/camera_request.cpp | 3 +-\n> > src/android/camera_request.h | 3 +-\n> > src/android/camera_worker.cpp | 129 ---------------------------------\n> > src/android/camera_worker.h | 72 ------------------\n> > src/android/meson.build | 1 -\n> > 7 files changed, 24 insertions(+), 230 deletions(-)\n> > delete mode 100644 src/android/camera_worker.cpp\n> > delete mode 100644 src/android/camera_worker.h\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index f2e0bdbdbbf6..29f0abd96ba9 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -19,6 +19,7 @@\n> >\n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> >\n> > @@ -406,7 +407,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 +419,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 +911,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\tFileDescriptor fenceFd;\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,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\tfenceFd = FileDescriptor(std::move(buffer.fence));\n\nThis also is a concern for me: the FileDescriptor that is passed to\nthe Fence should be created by moving the file descriptor, in order\nnot to duplicate it. There's no way I found to enforce that through\nthe Fence API.\n\n> > \t\t\tLOG(HAL, Debug) << ss.str() << \" (direct)\";\n> > \t\t\tbreak;\n> >\n> > @@ -969,13 +965,14 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > \t\t\treturn -ENOMEM;\n> > \t\t}\n> >\n> > +\t\tstd::unique_ptr fence = std::make_unique(fenceFd);\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>\n> \t\t\t\t\t\tframeBuffer,\n> \t\t\t\t\t\tstd::make_unique(fenceFd));\n>\n> could work too, up to you.\n\nBetter indeed, thanks\n\n>\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 +998,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 +1036,23 @@ 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 * been handled by the library.\n> > +\t\t *\n> > +\t\t * If the fence has been signalled correctly it will be -1 and\n> > +\t\t * closed, otherwise it is set to the 'acquire_fence' value and\n> > +\t\t * the framework will have to close it.\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\tFence *fence = buffer.frameBuffer->fence();\n> > +\t\t\tif (!fence)\n> > +\t\t\t\tbreak;\n>\n> Can this happen ? If not I'd ASSERT(), if it can we need better error\n> handling.\n>\n\nYes it does. If the fence has correctly been signalled it has been\nreset by the core.\n\n> > +\n> > +\t\t\tFileDescriptor fenceFd = std::move(fence->fd());\n>\n> This won't work as you expect. fence is a Fence *, and Fence::fd()\n> returns a const FileDescriptor &. std::move() will thus return a const\n> FileDescriptor &&. FileDescriptor has no assignment operator=() that\n> takes a *const* rvalue reference, so the copy assignment operator will\n> be called, creating a copy of FileDescriptor.\n>\n> This in itself isn't a big issue, as copying a FileDescriptor won't\n> duplicate the underlying fd. The construct is however misleading, so I'd\n> drop the std::move.\n>\n\nRight, misleading indeed\n\n> > +\t\t\tbuffer.fence = fenceFd.fd();\n>\n> This will also not work as expected, and it's a problem. The value\n> returned by fd() is valid, but only until the last FileDescriptor\n> instance referencing the same fd gets destroyed. Due to the copy above,\n> this happens when the FrameBuffer is destroyed (or added to a new\n> request for applications that reuse FrameBuffer instances, as you call\n> closeFence() in addRequest()). The fd you send back to the camera\n> service will thus not be valid.\n>\n> To fix this, I think we need\n>\n> - A FrameBuffer::resetFence() that returns a unique pointer to the\n> fence, to make the lifetime of the fence deterministric when requests\n> complete.\n>\n> - A way to extract the file descriptor from the fence without closing\n> it. This requires a new function in the FileDescriptor class, which\n> would be dangerous to call if there are multiple copies of the\n> FileDescriptor. This is where a unique fd class could help.\n>\n> We could also have a Fence::resetFileDescriptor() function to extract\n> the FileDescriptor from the Fence object, but we can call the new\n> function to extract the numerical fd from the FileDescriptor directly\n> on Fence::fd(), which should I think be good enough.\n\nOuch.\n\nWould something like this be better?\n\ninclude/libcamera/file_descriptor.h\n\n int fd() const { return fd_; }\n\n - private:\n int fd_;\n };\n\n\nsrc/libcamera/file_descriptor.cpp\n\n int FileDescriptor::reset()\n {\n int fd = fd_->fd();\n\n fd_->fd_ = -1;\n fd_.reset();\n\n return fd;\n }\n\nsrc/android/camera_device.cpp\n\n std::unique_ptr fence =\n buffer.frameBuffer->resetFence();\n if (!fence)\n break;\n\n buffer.fence = fence->fd().reset();\n\n\n\n>\n> > +\t\t}\n> > \t\tbuffer.status = Camera3RequestDescriptor::Status::Success;\n> > \t}\n> >\n> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > index 2a414020f1ad..c8cc78688bec 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..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 8d1204e5590b..69e1fe9fc379 100644\n> > --- a/src/android/camera_request.h\n> > +++ b/src/android/camera_request.h\n> > @@ -20,7 +20,6 @@\n> > #include \n> >\n> > #include \"camera_metadata.h\"\n> > -#include \"camera_worker.h\"\n> >\n> > class CameraBuffer;\n> > class CameraStream;\n> > @@ -71,7 +70,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_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 c94f16325925..000000000000\n> > --- a/src/android/camera_worker.h\n> > +++ /dev/null\n> > @@ -1,72 +0,0 @@\n> > -/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > -/*\n> > - * Copyright (C) 2020, Google Inc.\n> > - *\n> > - * camera_worker.h - Process capture requests on behalf of the Camera HAL\n> > - */\n> > -#ifndef __ANDROID_CAMERA_WORKER_H__\n> > -#define __ANDROID_CAMERA_WORKER_H__\n> > -\n> > -#include \n> > -#include \n> > -\n> > -#include \n> > -#include \n> > -\n> > -#include \n> > -#include \n> > -#include \n> > -#include \n> > -\n> > -class CameraDevice;\n> > -\n> > -class CaptureRequest\n> > -{\n> > -public:\n> > -\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> > -\n> > -#endif /* __ANDROID_CAMERA_WORKER_H__ */\n> > diff --git a/src/android/meson.build b/src/android/meson.build\n> > index 332b177ca805..75b4bf207085 100644\n> > --- a/src/android/meson.build\n> > +++ b/src/android/meson.build\n> > @@ -47,7 +47,6 @@ android_hal_sources = files([\n> > 'camera_ops.cpp',\n> > 'camera_request.cpp',\n> > 'camera_stream.cpp',\n> > - 'camera_worker.cpp',\n> > 'jpeg/encoder_libjpeg.cpp',\n> > 'jpeg/exif.cpp',\n> > 'jpeg/post_processor_jpeg.cpp',\n>\n> --\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 786A0BF415\n\tfor ;\n\tFri, 26 Nov 2021 12:24:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BDED8604FD;\n\tFri, 26 Nov 2021 13:24:39 +0100 (CET)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1DC1A6011E\n\tfor ;\n\tFri, 26 Nov 2021 13:24:38 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 916F840008;\n\tFri, 26 Nov 2021 12:24:37 +0000 (UTC)"],"Date":"Fri, 26 Nov 2021 13:25:29 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20211126122529.2wtfdxgngaa7lpa3@uno.localdomain>","References":"<20211120111313.106621-1-jacopo@jmondi.org>\n\t<20211120111313.106621-11-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 v2 10/12] 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":21287,"web_url":"https://patchwork.libcamera.org/comment/21287/","msgid":"","date":"2021-11-26T23:13:10","subject":"Re: [libcamera-devel] [PATCH v2 10/12] 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\nOn Fri, Nov 26, 2021 at 01:25:29PM +0100, Jacopo Mondi wrote:\n> On Sun, Nov 21, 2021 at 09:00:00PM +0200, Laurent Pinchart wrote:\n> > On Sat, Nov 20, 2021 at 12:13:11PM +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> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/android/camera_device.cpp | 43 +++++------\n> > > src/android/camera_device.h | 3 -\n> > > src/android/camera_request.cpp | 3 +-\n> > > src/android/camera_request.h | 3 +-\n> > > src/android/camera_worker.cpp | 129 ---------------------------------\n> > > src/android/camera_worker.h | 72 ------------------\n> > > src/android/meson.build | 1 -\n> > > 7 files changed, 24 insertions(+), 230 deletions(-)\n> > > delete mode 100644 src/android/camera_worker.cpp\n> > > delete mode 100644 src/android/camera_worker.h\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index f2e0bdbdbbf6..29f0abd96ba9 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -19,6 +19,7 @@\n> > >\n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > >\n> > > @@ -406,7 +407,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 +419,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 +911,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\tFileDescriptor fenceFd;\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,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\tfenceFd = FileDescriptor(std::move(buffer.fence));\n> \n> This also is a concern for me: the FileDescriptor that is passed to\n> the Fence should be created by moving the file descriptor, in order\n> not to duplicate it. There's no way I found to enforce that through\n> the Fence API.\n\nGoing for a unique fd could fix that (assuming it would only have a\nconstructor that takes ownership of the int fd).\n\n> > > \t\t\tLOG(HAL, Debug) << ss.str() << \" (direct)\";\n> > > \t\t\tbreak;\n> > >\n> > > @@ -969,13 +965,14 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > > \t\t\treturn -ENOMEM;\n> > > \t\t}\n> > >\n> > > +\t\tstd::unique_ptr fence = std::make_unique(fenceFd);\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> >\n> > \t\t\t\t\t\tframeBuffer,\n> > \t\t\t\t\t\tstd::make_unique(fenceFd));\n> >\n> > could work too, up to you.\n> \n> Better indeed, thanks\n> \n> >\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 +998,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 +1036,23 @@ 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 * been handled by the library.\n> > > +\t\t *\n> > > +\t\t * If the fence has been signalled correctly it will be -1 and\n> > > +\t\t * closed, otherwise it is set to the 'acquire_fence' value and\n> > > +\t\t * the framework will have to close it.\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\tFence *fence = buffer.frameBuffer->fence();\n> > > +\t\t\tif (!fence)\n> > > +\t\t\t\tbreak;\n> >\n> > Can this happen ? If not I'd ASSERT(), if it can we need better error\n> > handling.\n> \n> Yes it does. If the fence has correctly been signalled it has been\n> reset by the core.\n> \n> > > +\n> > > +\t\t\tFileDescriptor fenceFd = std::move(fence->fd());\n> >\n> > This won't work as you expect. fence is a Fence *, and Fence::fd()\n> > returns a const FileDescriptor &. std::move() will thus return a const\n> > FileDescriptor &&. FileDescriptor has no assignment operator=() that\n> > takes a *const* rvalue reference, so the copy assignment operator will\n> > be called, creating a copy of FileDescriptor.\n> >\n> > This in itself isn't a big issue, as copying a FileDescriptor won't\n> > duplicate the underlying fd. The construct is however misleading, so I'd\n> > drop the std::move.\n> \n> Right, misleading indeed\n> \n> > > +\t\t\tbuffer.fence = fenceFd.fd();\n> >\n> > This will also not work as expected, and it's a problem. The value\n> > returned by fd() is valid, but only until the last FileDescriptor\n> > instance referencing the same fd gets destroyed. Due to the copy above,\n> > this happens when the FrameBuffer is destroyed (or added to a new\n> > request for applications that reuse FrameBuffer instances, as you call\n> > closeFence() in addRequest()). The fd you send back to the camera\n> > service will thus not be valid.\n> >\n> > To fix this, I think we need\n> >\n> > - A FrameBuffer::resetFence() that returns a unique pointer to the\n> > fence, to make the lifetime of the fence deterministric when requests\n> > complete.\n> >\n> > - A way to extract the file descriptor from the fence without closing\n> > it. This requires a new function in the FileDescriptor class, which\n> > would be dangerous to call if there are multiple copies of the\n> > FileDescriptor. This is where a unique fd class could help.\n> >\n> > We could also have a Fence::resetFileDescriptor() function to extract\n> > the FileDescriptor from the Fence object, but we can call the new\n> > function to extract the numerical fd from the FileDescriptor directly\n> > on Fence::fd(), which should I think be good enough.\n> \n> Ouch.\n> \n> Would something like this be better?\n> \n> include/libcamera/file_descriptor.h\n> \n> int fd() const { return fd_; }\n> \n> - private:\n> int fd_;\n> };\n> \n> \n> src/libcamera/file_descriptor.cpp\n> \n> int FileDescriptor::reset()\n> {\n> int fd = fd_->fd();\n> \n> fd_->fd_ = -1;\n> fd_.reset();\n> \n> return fd;\n> }\n\nThe FileDescriptor function should be called release() to match the\nstd::unique_ptr<> nomenclature.\n\n> src/android/camera_device.cpp\n> \n> std::unique_ptr fence =\n> buffer.frameBuffer->resetFence();\n> if (!fence)\n> break;\n> \n> buffer.fence = fence->fd().reset();\n\nIt would work, but I don't think it's a good idea I'm afraid.\nFileDescriptor is analog to std::shared_ptr<>, and there's a reason why\nshared_ptr has no release() function. Its behaviour would be ill-defined\nwhen multiple instances of shared_ptr point to the same object.\n\nTo solve this, I think we'll need a std::unique_ptr<>-like wrapper for\nfile descriptors. If you don't beat me to it, I'll have a look at that\nover the weekend, based on the ScopedFD class that Hiro has proposed a\nwhile ago (possibly renamed to UniqueFD, or to something else, I'm not\nsure yet).\n\n> > > +\t\t}\n> > > \t\tbuffer.status = Camera3RequestDescriptor::Status::Success;\n> > > \t}\n> > >\n> > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > > index 2a414020f1ad..c8cc78688bec 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..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 8d1204e5590b..69e1fe9fc379 100644\n> > > --- a/src/android/camera_request.h\n> > > +++ b/src/android/camera_request.h\n> > > @@ -20,7 +20,6 @@\n> > > #include \n> > >\n> > > #include \"camera_metadata.h\"\n> > > -#include \"camera_worker.h\"\n> > >\n> > > class CameraBuffer;\n> > > class CameraStream;\n> > > @@ -71,7 +70,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_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 c94f16325925..000000000000\n> > > --- a/src/android/camera_worker.h\n> > > +++ /dev/null\n> > > @@ -1,72 +0,0 @@\n> > > -/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > -/*\n> > > - * Copyright (C) 2020, Google Inc.\n> > > - *\n> > > - * camera_worker.h - Process capture requests on behalf of the Camera HAL\n> > > - */\n> > > -#ifndef __ANDROID_CAMERA_WORKER_H__\n> > > -#define __ANDROID_CAMERA_WORKER_H__\n> > > -\n> > > -#include \n> > > -#include \n> > > -\n> > > -#include \n> > > -#include \n> > > -\n> > > -#include \n> > > -#include \n> > > -#include \n> > > -#include \n> > > -\n> > > -class CameraDevice;\n> > > -\n> > > -class CaptureRequest\n> > > -{\n> > > -public:\n> > > -\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> > > -\n> > > -#endif /* __ANDROID_CAMERA_WORKER_H__ */\n> > > diff --git a/src/android/meson.build b/src/android/meson.build\n> > > index 332b177ca805..75b4bf207085 100644\n> > > --- a/src/android/meson.build\n> > > +++ b/src/android/meson.build\n> > > @@ -47,7 +47,6 @@ android_hal_sources = files([\n> > > 'camera_ops.cpp',\n> > > 'camera_request.cpp',\n> > > 'camera_stream.cpp',\n> > > - 'camera_worker.cpp',\n> > > 'jpeg/encoder_libjpeg.cpp',\n> > > 'jpeg/exif.cpp',\n> > > 'jpeg/post_processor_jpeg.cpp',","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 65E47BF415\n\tfor ;\n\tFri, 26 Nov 2021 23:13:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C5E2060565;\n\tSat, 27 Nov 2021 00:13:36 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0D8CC60423\n\tfor ;\n\tSat, 27 Nov 2021 00:13:35 +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 7FF8A1850;\n\tSat, 27 Nov 2021 00:13:34 +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=\"DtiPwgt4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637968414;\n\tbh=Je/NQ1eQneHNgtb+CQ66/1iMI4xy2DeCA9bioVWTSvY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DtiPwgt4wE1rsLIB6d1UXhIv9NMEKzTwrq8aFpNYtzl6URwAw2/9umEdHNIbvgt7F\n\tx5xS6fniG1fkUceI6i5EbYdfEzxnuOqC+al+77o71JttxTrwbV0ec9Bdel+gwM7MoK\n\tI+m4B4hFAY0CZibsVg7ho0RcucCz0P1IvcUwXmcI=","Date":"Sat, 27 Nov 2021 01:13:10 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20211120111313.106621-1-jacopo@jmondi.org>\n\t<20211120111313.106621-11-jacopo@jmondi.org>\n\t\n\t<20211126122529.2wtfdxgngaa7lpa3@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211126122529.2wtfdxgngaa7lpa3@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 10/12] 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\" "}}]