{"id":15146,"url":"https://patchwork.libcamera.org/api/patches/15146/?format=json","web_url":"https://patchwork.libcamera.org/patch/15146/","project":{"id":1,"url":"https://patchwork.libcamera.org/api/projects/1/?format=json","name":"libcamera","link_name":"libcamera","list_id":"libcamera_core","list_email":"libcamera-devel@lists.libcamera.org","web_url":"","scm_url":"","webscm_url":""},"msgid":"<20211211165714.23067-12-jacopo@jmondi.org>","date":"2021-12-11T16:57:14","name":"[libcamera-devel,v6,11/11] android: Remove CameraWorker","commit_ref":null,"pull_url":null,"state":"accepted","archived":false,"hash":"472849ba33d2976e4a1c5b1a3c6a0958e022a4a2","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/?format=json","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"delegate":null,"mbox":"https://patchwork.libcamera.org/patch/15146/mbox/","series":[{"id":2838,"url":"https://patchwork.libcamera.org/api/series/2838/?format=json","web_url":"https://patchwork.libcamera.org/project/libcamera/list/?series=2838","date":"2021-12-11T16:57:03","name":"libcamera: Add Fence support","version":6,"mbox":"https://patchwork.libcamera.org/series/2838/mbox/"}],"comments":"https://patchwork.libcamera.org/api/patches/15146/comments/","check":"pending","checks":"https://patchwork.libcamera.org/api/patches/15146/checks/","tags":{},"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 EBD2AC324B\n\tfor ;\n\tSat, 11 Dec 2021 16:56:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9910F608A0;\n\tSat, 11 Dec 2021 17:56:39 +0100 (CET)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A244F608ED\n\tfor ;\n\tSat, 11 Dec 2021 17:56:33 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 04D6F20004;\n\tSat, 11 Dec 2021 16:56:32 +0000 (UTC)"],"From":"Jacopo Mondi ","To":"libcamera-devel@lists.libcamera.org","Date":"Sat, 11 Dec 2021 17:57:14 +0100","Message-Id":"<20211211165714.23067-12-jacopo@jmondi.org>","X-Mailer":"git-send-email 2.33.1","In-Reply-To":"<20211211165714.23067-1-jacopo@jmondi.org>","References":"<20211211165714.23067-1-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Transfer-Encoding":"8bit","Subject":"[libcamera-devel] [PATCH v6 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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "},"content":"The CameraWorker class purpose was to handle acquire fences for incoming\ncapture requests directed to libcamera.\n\nNow that fences are handled by the core library, it is not required to\nhandle them in the HAL and the CameraWorker and CaptureRequest classes\ncan be dropped.\n\nUpdate the core in CameraDevice class accordingly to queue Requests\ndirectly to the libcamera::Camera and set the release_fence to the\nvalue of the FrameBuffer::fence() for streams of type ::Direct.\n\nWhile at it make CameraRequest::StreamBuffer::fence a UniqueFD to ease\nthe management of the fences file descriptor values.\n\nSigned-off-by: Jacopo Mondi \nReviewed-by: Laurent Pinchart \n---\n src/android/camera_device.cpp | 42 +++++------\n src/android/camera_device.h | 3 -\n src/android/camera_request.cpp | 3 +-\n src/android/camera_request.h | 6 +-\n src/android/camera_stream.cpp | 10 +--\n src/android/camera_worker.cpp | 129 ---------------------------------\n src/android/camera_worker.h | 70 ------------------\n src/android/meson.build | 1 -\n 8 files changed, 28 insertions(+), 236 deletions(-)\n delete mode 100644 src/android/camera_worker.cpp\n delete mode 100644 src/android/camera_worker.h","diff":"diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\nindex 9f772b58d80d..f28fdd6a2955 100644\n--- a/src/android/camera_device.cpp\n+++ b/src/android/camera_device.cpp\n@@ -14,10 +14,12 @@\n #include \n \n #include \n+#include \n #include \n \n #include \n #include \n+#include \n #include \n #include \n \n@@ -420,7 +422,6 @@ void CameraDevice::flush()\n \t\tstate_ = State::Flushing;\n \t}\n \n-\tworker_.stop();\n \tcamera_->stop();\n \n \tMutexLocker stateLock(stateMutex_);\n@@ -433,7 +434,6 @@ void CameraDevice::stop()\n \tif (state_ == State::Stopped)\n \t\treturn;\n \n-\tworker_.stop();\n \tcamera_->stop();\n \n \t{\n@@ -930,13 +930,10 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n \n \t\t/*\n \t\t * Inspect the camera stream type, create buffers opportunely\n-\t\t * and add them to the Request if required. Only acquire fences\n-\t\t * for streams of type Direct are handled by the CameraWorker,\n-\t\t * while fences for streams of type Internal and Mapped are\n-\t\t * handled at post-processing time.\n+\t\t * and add them to the Request if required.\n \t\t */\n \t\tFrameBuffer *frameBuffer = nullptr;\n-\t\tint acquireFence = -1;\n+\t\tUniqueFD acquireFence;\n \n \t\tMutexLocker lock(descriptor->streamsProcessMutex_);\n \n@@ -964,7 +961,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n \t\t\t\t\t\t cameraStream->configuration().pixelFormat,\n \t\t\t\t\t\t cameraStream->configuration().size);\n \t\t\tframeBuffer = buffer.frameBuffer.get();\n-\t\t\tacquireFence = buffer.fence;\n+\t\t\tacquireFence = std::move(buffer.fence);\n \t\t\tLOG(HAL, Debug) << ss.str() << \" (direct)\";\n \t\t\tbreak;\n \n@@ -990,13 +987,14 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n \t\t\treturn -ENOMEM;\n \t\t}\n \n+\t\tauto fence = std::make_unique(std::move(acquireFence));\n \t\tdescriptor->request_->addBuffer(cameraStream->stream(),\n-\t\t\t\t\t\tframeBuffer, acquireFence);\n+\t\t\t\t\t\tframeBuffer, std::move(fence));\n \t}\n \n \t/*\n \t * Translate controls from Android to libcamera and queue the request\n-\t * to the CameraWorker thread.\n+\t * to the camera.\n \t */\n \tint ret = processControls(descriptor.get());\n \tif (ret)\n@@ -1022,26 +1020,23 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n \t}\n \n \tif (state_ == State::Stopped) {\n-\t\tworker_.start();\n-\n \t\tret = camera_->start();\n \t\tif (ret) {\n \t\t\tLOG(HAL, Error) << \"Failed to start camera\";\n-\t\t\tworker_.stop();\n \t\t\treturn ret;\n \t\t}\n \n \t\tstate_ = State::Running;\n \t}\n \n-\tCaptureRequest *request = descriptor->request_.get();\n+\tRequest *request = descriptor->request_.get();\n \n \t{\n \t\tMutexLocker descriptorsLock(descriptorsMutex_);\n \t\tdescriptors_.push(std::move(descriptor));\n \t}\n \n-\tworker_.queueRequest(request);\n+\tcamera_->queueRequest(request);\n \n \treturn 0;\n }\n@@ -1063,16 +1058,17 @@ void CameraDevice::requestComplete(Request *request)\n \t\t/*\n \t\t * Streams of type Direct have been queued to the\n \t\t * libcamera::Camera and their acquire fences have\n-\t\t * already been waited on by the CameraWorker.\n+\t\t * already been waited on by the library.\n \t\t *\n \t\t * Acquire fences of streams of type Internal and Mapped\n \t\t * will be handled during post-processing.\n-\t\t *\n-\t\t * \\todo Instrument the CameraWorker to set the acquire\n-\t\t * fence to -1 once it has handled it and remove this check.\n \t\t */\n-\t\tif (stream->type() == CameraStream::Type::Direct)\n-\t\t\tbuffer.fence = -1;\n+\t\tif (stream->type() == CameraStream::Type::Direct) {\n+\t\t\t/* If handling of the fence has failed restore buffer.fence. */\n+\t\t\tstd::unique_ptr fence = buffer.frameBuffer->releaseFence();\n+\t\t\tif (fence)\n+\t\t\t\tbuffer.fence = fence->release();\n+\t\t}\n \t\tbuffer.status = Camera3RequestDescriptor::Status::Success;\n \t}\n \n@@ -1193,7 +1189,7 @@ void CameraDevice::sendCaptureResults()\n \t\tstd::vector resultBuffers;\n \t\tresultBuffers.reserve(descriptor->buffers_.size());\n \n-\t\tfor (const auto &buffer : descriptor->buffers_) {\n+\t\tfor (auto &buffer : descriptor->buffers_) {\n \t\t\tcamera3_buffer_status status = CAMERA3_BUFFER_STATUS_ERROR;\n \n \t\t\tif (buffer.status == Camera3RequestDescriptor::Status::Success)\n@@ -1207,7 +1203,7 @@ void CameraDevice::sendCaptureResults()\n \t\t\t */\n \t\t\tresultBuffers.push_back({ buffer.stream->camera3Stream(),\n \t\t\t\t\t\t buffer.camera3Buffer, status,\n-\t\t\t\t\t\t -1, buffer.fence });\n+\t\t\t\t\t\t -1, buffer.fence.release() });\n \t\t}\n \n \t\tcaptureResult.num_output_buffers = resultBuffers.size();\ndiff --git a/src/android/camera_device.h b/src/android/camera_device.h\nindex e5d9cda2ce3a..64050416e67e 100644\n--- a/src/android/camera_device.h\n+++ b/src/android/camera_device.h\n@@ -29,7 +29,6 @@\n #include \"camera_capabilities.h\"\n #include \"camera_metadata.h\"\n #include \"camera_stream.h\"\n-#include \"camera_worker.h\"\n #include \"jpeg/encoder.h\"\n \n class Camera3RequestDescriptor;\n@@ -105,8 +104,6 @@ private:\n \tunsigned int id_;\n \tcamera3_device_t camera3Device_;\n \n-\tCameraWorker worker_;\n-\n \tlibcamera::Mutex stateMutex_; /* Protects access to the camera state. */\n \tState state_ LIBCAMERA_TSA_GUARDED_BY(stateMutex_);\n \ndiff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp\nindex 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;\ndiff --git a/src/android/camera_request.h b/src/android/camera_request.h\nindex d9b04f166fd7..37b6ae326f3e 100644\n--- a/src/android/camera_request.h\n+++ b/src/android/camera_request.h\n@@ -13,6 +13,7 @@\n \n #include \n #include \n+#include \n \n #include \n #include \n@@ -20,7 +21,6 @@\n #include \n \n #include \"camera_metadata.h\"\n-#include \"camera_worker.h\"\n \n class CameraBuffer;\n class CameraStream;\n@@ -45,7 +45,7 @@ public:\n \t\tCameraStream *stream;\n \t\tbuffer_handle_t *camera3Buffer;\n \t\tstd::unique_ptr frameBuffer;\n-\t\tint fence;\n+\t\tlibcamera::UniqueFD fence;\n \t\tStatus status = Status::Success;\n \t\tlibcamera::FrameBuffer *internalBuffer = nullptr;\n \t\tconst libcamera::FrameBuffer *srcBuffer = nullptr;\n@@ -72,7 +72,7 @@ public:\n \tstd::vector buffers_;\n \n \tCameraMetadata settings_;\n-\tstd::unique_ptr request_;\n+\tstd::unique_ptr request_;\n \tstd::unique_ptr resultMetadata_;\n \n \tbool complete_ = false;\ndiff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\nindex ee9bbe48993a..c21574501916 100644\n--- a/src/android/camera_stream.cpp\n+++ b/src/android/camera_stream.cpp\n@@ -170,16 +170,16 @@ int CameraStream::process(Camera3RequestDescriptor::StreamBuffer *streamBuffer)\n \tASSERT(type_ != Type::Direct);\n \n \t/* Handle waiting on fences on the destination buffer. */\n-\tif (streamBuffer->fence != -1) {\n-\t\tint ret = waitFence(streamBuffer->fence);\n+\tif (streamBuffer->fence.isValid()) {\n+\t\tint ret = waitFence(streamBuffer->fence.get());\n \t\tif (ret < 0) {\n \t\t\tLOG(HAL, Error) << \"Failed waiting for fence: \"\n-\t\t\t\t\t<< streamBuffer->fence << \": \" << strerror(-ret);\n+\t\t\t\t\t<< streamBuffer->fence.get() << \": \"\n+\t\t\t\t\t<< strerror(-ret);\n \t\t\treturn ret;\n \t\t}\n \n-\t\t::close(streamBuffer->fence);\n-\t\tstreamBuffer->fence = -1;\n+\t\tstreamBuffer->fence.reset();\n \t}\n \n \tconst StreamConfiguration &output = configuration();\ndiff --git a/src/android/camera_worker.cpp b/src/android/camera_worker.cpp\ndeleted file mode 100644\nindex 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-}\ndiff --git a/src/android/camera_worker.h b/src/android/camera_worker.h\ndeleted file mode 100644\nindex 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-};\ndiff --git a/src/android/meson.build b/src/android/meson.build\nindex 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","prefixes":["libcamera-devel","v6","11/11"]}