{"id":14525,"url":"https://patchwork.libcamera.org/api/patches/14525/?format=json","web_url":"https://patchwork.libcamera.org/patch/14525/","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":"<20211018115431.17946-1-laurent.pinchart@ideasonboard.com>","date":"2021-10-18T11:54:31","name":"[libcamera-devel] libcamera: request: Make Request class Extensible","commit_ref":null,"pull_url":null,"state":"superseded","archived":false,"hash":"b6241ef040bb045d128ebcbdceca4f505bdd4424","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/?format=json","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"delegate":null,"mbox":"https://patchwork.libcamera.org/patch/14525/mbox/","series":[{"id":2705,"url":"https://patchwork.libcamera.org/api/series/2705/?format=json","web_url":"https://patchwork.libcamera.org/project/libcamera/list/?series=2705","date":"2021-10-18T11:54:31","name":"[libcamera-devel] libcamera: request: Make Request class Extensible","version":1,"mbox":"https://patchwork.libcamera.org/series/2705/mbox/"}],"comments":"https://patchwork.libcamera.org/api/patches/14525/comments/","check":"pending","checks":"https://patchwork.libcamera.org/api/patches/14525/checks/","tags":{},"headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":["parsemail@patchwork.libcamera.org","kbingham@ideasonboard.com"],"Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 1FDA8BDB1C\n\tfor ;\n\tWed, 10 Nov 2021 14:01:27 +0000 (UTC)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net\n\t[86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B1760501\n\tfor ;\n\tWed, 10 Nov 2021 15:01:26 +0100 (CET)","from perceval.ideasonboard.com\n\tby perceval.ideasonboard.com with LMTP id WO00MJFgbWFUCQAA4E0KoQ\n\t(envelope-from )\n\tfor ; Mon, 18 Oct 2021 13:54:57 +0200","from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[IPv6:2001:4b98:dc0:43:216:3eff:fe33:f827])\tby\n\tperceval.ideasonboard.com (Postfix) with ESMTPS id A49898C6;\n\tMon, 18 Oct 2021 13:54:57 +0200 (CEST)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EAB5F68F56;\n\tMon, 18 Oct 2021 13:54:56 +0200 (CEST)","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 EBBF168F56\n\tfor ;\n\tMon, 18 Oct 2021 13:54:54 +0200 (CEST)","from pendragon.lan (62-78-145-57.bb.dnainternet.fi [62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 70E6A8C6\n\tfor ;\n\tMon, 18 Oct 2021 13:54:54 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636552886;\n\tbh=IW45UBEXrWxqza+XU8+8MMsAX0+1SBOwgKbSev9XWK4=;\n\th=From:To:Date:Subject:List-Id:List-Unsubscribe:List-Archive:\n\tList-Post:List-Help:List-Subscribe:Resent-From:Resent-To:From;\n\tb=PpqS5stxRWbuWx2pb4XvfafjIYYlmTupsmHYiS975qQ3IURpNPtYElNJXmo3EgwN2\n\toJnlcj74x2bFzmcYnvYarvFwvemPSywXBDs7KAYiEJd8fSwec+RYDVKUg3vE9/QqA9\n\tcZUKY8YnBqRAseQdtU3U7S2ekhQUaJ0l8PtTYazE=","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634558094;\n\tbh=IW45UBEXrWxqza+XU8+8MMsAX0+1SBOwgKbSev9XWK4=;\n\th=From:To:Subject:Date:From;\n\tb=wN7LEOc3nbd5sicgyrP/BFPJUsIkmiry9t62QL449SkyvYkhNk0xZXAFPijQaVCxB\n\tpwwW0n7GL+vHVcDSQscoIv7tYsrauV7Xt/lkZSBLHLq762uzyk1BIsntv7iseDW2cT\n\txYbfhuHlWoOA8fJlYIGJp+373z2u3S765eRqsMI8="],"Authentication-Results":["perceval.ideasonboard.com;\n\tdkim=fail reason=\"signature\n\tverification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com header.b=\"wN7LEOc3\";\tdkim-atps=neutral","lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature\n\tverification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com header.b=\"wN7LEOc3\";\tdkim-atps=neutral"],"From":"Laurent Pinchart ","To":"libcamera-devel@lists.libcamera.org","Date":"Mon, 18 Oct 2021 14:54:31 +0300","Message-Id":"<20211018115431.17946-1-laurent.pinchart@ideasonboard.com>","X-Mailer":"git-send-email 2.32.0","MIME-Version":"1.0","Content-Transfer-Encoding":"8bit","Subject":"[libcamera-devel] [PATCH] libcamera: request: Make Request class\n\tExtensible","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\" ","X-TUID":"pRSGHd+qrqpx","Resent-From":"Kieran Bingham ","Resent-To":"parsemail@patchwork.libcamera.org"},"content":"Implement the D-Pointer design pattern in the Request class to allow\nchanging internal data without affecting the public ABI.\n\nMove a few internal fields that are not needed to implement the public\nAPI to the Request::Private class already. More fields may be moved\nlater.\n\nSigned-off-by: Laurent Pinchart \n---\nI've had this patch in my tree for some time now, and in a recent\nconversation with Jacopo I realized it could be useful to address fence\nhandling correctly. It's a bit of a work in progress in the sense that\nit doesn't go through all the members of the Request class to decide\nwhich ones to move to the private class, but I think it can be built\nupon.\n---\n include/libcamera/internal/meson.build | 1 +\n include/libcamera/internal/request.h | 36 ++++++++++++++++\n include/libcamera/request.h | 8 ++--\n src/libcamera/pipeline_handler.cpp | 7 +--\n src/libcamera/request.cpp | 60 +++++++++++++++++++-------\n 5 files changed, 89 insertions(+), 23 deletions(-)\n create mode 100644 include/libcamera/internal/request.h\n\n\nbase-commit: 2f75a7e5b8c6258dc12e9e3128cb30133f66b4f9","diff":"diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\nindex 665fd6de4ed3..cae65b0604ff 100644\n--- a/include/libcamera/internal/meson.build\n+++ b/include/libcamera/internal/meson.build\n@@ -34,6 +34,7 @@ libcamera_internal_headers = files([\n 'pipeline_handler.h',\n 'process.h',\n 'pub_key.h',\n+ 'request.h',\n 'source_paths.h',\n 'sysfs.h',\n 'v4l2_device.h',\ndiff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h\nnew file mode 100644\nindex 000000000000..ca954f06f497\n--- /dev/null\n+++ b/include/libcamera/internal/request.h\n@@ -0,0 +1,36 @@\n+/* SPDX-License-Identifier: LGPL-2.1-or-later */\n+/*\n+ * Copyright (C) 2019, Google Inc.\n+ *\n+ * request.h - Request class private data\n+ */\n+#ifndef __LIBCAMERA_INTERNAL_REQUEST_H__\n+#define __LIBCAMERA_INTERNAL_REQUEST_H__\n+\n+#include \n+\n+namespace libcamera {\n+\n+class Camera;\n+class CameraControlValidator;\n+class FrameBuffer;\n+\n+class Request::Private : public Extensible::Private\n+{\n+\tLIBCAMERA_DECLARE_PUBLIC(Request)\n+\n+public:\n+\tPrivate(Camera *camera);\n+\t~Private();\n+\n+\tCamera *camera() const { return camera_; }\n+\n+private:\n+\tCamera *camera_;\n+\tCameraControlValidator *validator_;\n+\tbool cancelled_;\n+};\n+\n+} /* namespace libcamera */\n+\n+#endif /* __LIBCAMERA_INTERNAL_REQUEST_H__ */\ndiff --git a/include/libcamera/request.h b/include/libcamera/request.h\nindex 2d361c9d97dc..e378355f18fc 100644\n--- a/include/libcamera/request.h\n+++ b/include/libcamera/request.h\n@@ -21,12 +21,13 @@\n namespace libcamera {\n \n class Camera;\n-class CameraControlValidator;\n class FrameBuffer;\n class Stream;\n \n-class Request\n+class Request : public Extensible\n {\n+\tLIBCAMERA_DECLARE_PRIVATE()\n+\n public:\n \tenum Status {\n \t\tRequestPending,\n@@ -70,8 +71,6 @@ private:\n \n \tbool completeBuffer(FrameBuffer *buffer);\n \n-\tCamera *camera_;\n-\tCameraControlValidator *validator_;\n \tControlList *controls_;\n \tControlList *metadata_;\n \tBufferMap bufferMap_;\n@@ -80,7 +79,6 @@ private:\n \tuint32_t sequence_;\n \tconst uint64_t cookie_;\n \tStatus status_;\n-\tbool cancelled_;\n };\n \n } /* namespace libcamera */\ndiff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\nindex f69c4f03b80f..cada864687ff 100644\n--- a/src/libcamera/pipeline_handler.cpp\n+++ b/src/libcamera/pipeline_handler.cpp\n@@ -19,6 +19,7 @@\n #include \"libcamera/internal/camera.h\"\n #include \"libcamera/internal/device_enumerator.h\"\n #include \"libcamera/internal/media_device.h\"\n+#include \"libcamera/internal/request.h\"\n #include \"libcamera/internal/tracepoints.h\"\n \n /**\n@@ -311,7 +312,7 @@ void PipelineHandler::queueRequest(Request *request)\n {\n \tLIBCAMERA_TRACEPOINT(request_queue, request);\n \n-\tCamera *camera = request->camera_;\n+\tCamera *camera = request->_d()->camera();\n \tCamera::Private *data = camera->_d();\n \tdata->queuedRequests_.push_back(request);\n \n@@ -360,7 +361,7 @@ void PipelineHandler::queueRequest(Request *request)\n */\n bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)\n {\n-\tCamera *camera = request->camera_;\n+\tCamera *camera = request->_d()->camera();\n \tcamera->bufferCompleted.emit(request, buffer);\n \treturn request->completeBuffer(buffer);\n }\n@@ -381,7 +382,7 @@ bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)\n */\n void PipelineHandler::completeRequest(Request *request)\n {\n-\tCamera *camera = request->camera_;\n+\tCamera *camera = request->_d()->camera();\n \n \trequest->complete();\n \ndiff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\nindex f95ce4db5eaa..7863269f3139 100644\n--- a/src/libcamera/request.cpp\n+++ b/src/libcamera/request.cpp\n@@ -19,10 +19,11 @@\n \n #include \"libcamera/internal/camera_controls.h\"\n #include \"libcamera/internal/framebuffer.h\"\n+#include \"libcamera/internal/request.h\"\n #include \"libcamera/internal/tracepoints.h\"\n \n /**\n- * \\file request.h\n+ * \\file libcamera/request.h\n * \\brief Describes a frame capture request to be processed by a camera\n */\n \n@@ -30,6 +31,39 @@ namespace libcamera {\n \n LOG_DEFINE_CATEGORY(Request)\n \n+/**\n+ * \\class Request::Private\n+ * \\brief Request private data\n+ *\n+ * The Request::Private class stores all private data associated with a\n+ * request. It implements the d-pointer design pattern to hide core\n+ * Request data from the public API, and exposes utility functions to\n+ * internal users of the request (namely the PipelineHandler class and its\n+ * subclasses).\n+ */\n+\n+Request::Private::Private(Camera *camera)\n+\t: camera_(camera), cancelled_(false)\n+{\n+\t/**\n+\t * \\todo Should the Camera expose a validator instance, to avoid\n+\t * creating a new instance for each request?\n+\t */\n+\tvalidator_ = new CameraControlValidator(camera);\n+}\n+\n+Request::Private::~Private()\n+{\n+\tdelete validator_;\n+}\n+\n+/**\n+ * \\fn Request::Private::camera()\n+ * \\brief Retrieve the camera this request has been queued to\n+ * \\return The Camera this request has been queued to, or nullptr if the\n+ * request hasn't been queued\n+ */\n+\n /**\n * \\enum Request::Status\n * Request completion status\n@@ -74,15 +108,10 @@ LOG_DEFINE_CATEGORY(Request)\n * completely opaque to libcamera.\n */\n Request::Request(Camera *camera, uint64_t cookie)\n-\t: camera_(camera), sequence_(0), cookie_(cookie),\n-\t status_(RequestPending), cancelled_(false)\n+\t: Extensible(std::make_unique(camera)), sequence_(0),\n+\t cookie_(cookie), status_(RequestPending)\n {\n-\t/**\n-\t * \\todo Should the Camera expose a validator instance, to avoid\n-\t * creating a new instance for each request?\n-\t */\n-\tvalidator_ = new CameraControlValidator(camera);\n-\tcontrols_ = new ControlList(controls::controls, validator_);\n+\tcontrols_ = new ControlList(controls::controls, _d()->validator_);\n \n \t/**\n \t * \\todo: Add a validator for metadata controls.\n@@ -100,7 +129,6 @@ Request::~Request()\n \n \tdelete metadata_;\n \tdelete controls_;\n-\tdelete validator_;\n }\n \n /**\n@@ -130,7 +158,7 @@ void Request::reuse(ReuseFlag flags)\n \n \tsequence_ = 0;\n \tstatus_ = RequestPending;\n-\tcancelled_ = false;\n+\t_d()->cancelled_ = false;\n \n \tcontrols_->clear();\n \tmetadata_->clear();\n@@ -286,7 +314,7 @@ void Request::complete()\n \tASSERT(status_ == RequestPending);\n \tASSERT(!hasPendingBuffers());\n \n-\tstatus_ = cancelled_ ? RequestCancelled : RequestComplete;\n+\tstatus_ = _d()->cancelled_ ? RequestCancelled : RequestComplete;\n \n \tLOG(Request, Debug) << toString();\n \n@@ -303,17 +331,19 @@ void Request::complete()\n */\n void Request::cancel()\n {\n+\tPrivate *const d = _d();\n+\n \tLIBCAMERA_TRACEPOINT(request_cancel, this);\n \n \tASSERT(status_ == RequestPending);\n \n \tfor (FrameBuffer *buffer : pending_) {\n \t\tbuffer->cancel();\n-\t\tcamera_->bufferCompleted.emit(this, buffer);\n+\t\td->camera_->bufferCompleted.emit(this, buffer);\n \t}\n \n \tpending_.clear();\n-\tcancelled_ = true;\n+\td->cancelled_ = true;\n }\n \n /**\n@@ -339,7 +369,7 @@ bool Request::completeBuffer(FrameBuffer *buffer)\n \tbuffer->_d()->setRequest(nullptr);\n \n \tif (buffer->metadata().status == FrameMetadata::FrameCancelled)\n-\t\tcancelled_ = true;\n+\t\t_d()->cancelled_ = true;\n \n \treturn !hasPendingBuffers();\n }\n","prefixes":["libcamera-devel"]}