[{"id":20770,"web_url":"https://patchwork.libcamera.org/comment/20770/","msgid":"<163645973070.1606134.529580468315728097@Monstersaurus>","date":"2021-11-09T12:08:50","subject":"Re: [libcamera-devel] [PATCH 01/10] libcamera: request: Make Request\n\tclass Extensible","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi (2021-10-28 12:15:11)\n> From: Laurent Pinchart \n> \n> Implement the D-Pointer design pattern in the Request class to allow\n> changing internal data without affecting the public ABI.\n> \n> Move a few internal fields that are not needed to implement the public\n> API to the Request::Private class already. More fields may be moved\n> later.\n> \n> Signed-off-by: Laurent Pinchart \n> Reviewed-by: Jacopo Mondi \n\n\nReviewed-by: Kieran Bingham \n\nHappy to get this patch merged earlier if it helps too, as it unblocks\nother patches.\n\n\n> ---\n> include/libcamera/internal/meson.build | 1 +\n> include/libcamera/internal/request.h | 34 ++++++++++++++++++\n> include/libcamera/request.h | 6 ++--\n> src/libcamera/pipeline_handler.cpp | 7 ++--\n> src/libcamera/request.cpp | 50 +++++++++++++++++++++-----\n> 5 files changed, 84 insertions(+), 14 deletions(-)\n> create mode 100644 include/libcamera/internal/request.h\n> \n> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> index 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',\n> diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h\n> new file mode 100644\n> index 000000000000..df0cc014067e\n> --- /dev/null\n> +++ b/include/libcamera/internal/request.h\n> @@ -0,0 +1,34 @@\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 FrameBuffer;\n> +\n> +class Request::Private : public Extensible::Private\n> +{\n> + LIBCAMERA_DECLARE_PUBLIC(Request)\n> +\n> +public:\n> + Private(Camera *camera);\n> + ~Private();\n> +\n> + Camera *camera() const { return camera_; }\n> +\n> +private:\n> + Camera *camera_;\n> + bool cancelled_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_INTERNAL_REQUEST_H__ */\n> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> index d16904e6b679..d6bd0ba2ac17 100644\n> --- a/include/libcamera/request.h\n> +++ b/include/libcamera/request.h\n> @@ -25,8 +25,10 @@ class CameraControlValidator;\n> class FrameBuffer;\n> class Stream;\n> \n> -class Request\n> +class Request : public Extensible\n> {\n> + LIBCAMERA_DECLARE_PRIVATE()\n> +\n> public:\n> enum Status {\n> RequestPending,\n> @@ -70,7 +72,6 @@ private:\n> \n> bool completeBuffer(FrameBuffer *buffer);\n> \n> - Camera *camera_;\n> ControlList *controls_;\n> ControlList *metadata_;\n> BufferMap bufferMap_;\n> @@ -79,7 +80,6 @@ private:\n> uint32_t sequence_;\n> const uint64_t cookie_;\n> Status status_;\n> - bool cancelled_;\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 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> LIBCAMERA_TRACEPOINT(request_queue, request);\n> \n> - Camera *camera = request->camera_;\n> + Camera *camera = request->_d()->camera();\n> Camera::Private *data = camera->_d();\n> data->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> - Camera *camera = request->camera_;\n> + Camera *camera = request->_d()->camera();\n> camera->bufferCompleted.emit(request, buffer);\n> return 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> - Camera *camera = request->camera_;\n> + Camera *camera = request->_d()->camera();\n> \n> request->complete();\n> \n> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> index 17fefab7ad0e..33fee1ac05ba 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -20,10 +20,11 @@\n> #include \"libcamera/internal/camera.h\"\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> @@ -31,6 +32,37 @@ 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> +/**\n> + * \\brief Create a Request::Private\n> + * \\param camera The Camera that creates the request\n> + */\n> +Request::Private::Private(Camera *camera)\n> + : camera_(camera), cancelled_(false)\n> +{\n> +}\n> +\n> +Request::Private::~Private()\n> +{\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> @@ -75,8 +107,8 @@ LOG_DEFINE_CATEGORY(Request)\n> * completely opaque to libcamera.\n> */\n> Request::Request(Camera *camera, uint64_t cookie)\n> - : camera_(camera), sequence_(0), cookie_(cookie),\n> - status_(RequestPending), cancelled_(false)\n> + : Extensible(std::make_unique(camera)), sequence_(0),\n> + cookie_(cookie), status_(RequestPending)\n> {\n> controls_ = new ControlList(controls::controls,\n> camera->_d()->validator());\n> @@ -126,7 +158,7 @@ void Request::reuse(ReuseFlag flags)\n> \n> sequence_ = 0;\n> status_ = RequestPending;\n> - cancelled_ = false;\n> + _d()->cancelled_ = false;\n> \n> controls_->clear();\n> metadata_->clear();\n> @@ -282,7 +314,7 @@ void Request::complete()\n> ASSERT(status_ == RequestPending);\n> ASSERT(!hasPendingBuffers());\n> \n> - status_ = cancelled_ ? RequestCancelled : RequestComplete;\n> + status_ = _d()->cancelled_ ? RequestCancelled : RequestComplete;\n> \n> LOG(Request, Debug) << toString();\n> \n> @@ -299,17 +331,19 @@ void Request::complete()\n> */\n> void Request::cancel()\n> {\n> + Private *const d = _d();\n> +\n> LIBCAMERA_TRACEPOINT(request_cancel, this);\n> \n> ASSERT(status_ == RequestPending);\n> \n> for (FrameBuffer *buffer : pending_) {\n> buffer->cancel();\n> - camera_->bufferCompleted.emit(this, buffer);\n> + d->camera_->bufferCompleted.emit(this, buffer);\n> }\n> \n> pending_.clear();\n> - cancelled_ = true;\n> + d->cancelled_ = true;\n> }\n> \n> /**\n> @@ -335,7 +369,7 @@ bool Request::completeBuffer(FrameBuffer *buffer)\n> buffer->_d()->setRequest(nullptr);\n> \n> if (buffer->metadata().status == FrameMetadata::FrameCancelled)\n> - cancelled_ = true;\n> + _d()->cancelled_ = true;\n> \n> return !hasPendingBuffers();\n> }\n> -- \n> 2.33.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id EF02EBDB1C\n\tfor ;\n\tTue, 9 Nov 2021 17:43:32 +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 8CA83DEE\n\tfor ;\n\tTue, 9 Nov 2021 18:43:32 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636479812;\n\tbh=y6wOlbgU9bD1jg7Xwl4KN9tSweFywiHQb7As7rqgog0=;\n\th=In-Reply-To:References:Subject:From:To:Date:Resent-From:Resent-To:\n\tFrom;\n\tb=JsH/ChrUutKaishNmCUdsWPdCCpjyNg5rjU0vgPZZAZdibiN1qON+FxQ2enhpPbwC\n\tX9eTPAYVa3H0CpK86SGO1p9cB/0RyIfjCKhLmRvfhcue5uAL5qOMH8EUwQo43UIGfA\n\tNXYaHuLNPz43IA7HxUEeVadFgiUg+tM5DqkWHs2Y=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211028111520.256612-2-jacopo@jmondi.org>","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-2-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 01/10] libcamera: request: Make Request\n\tclass Extensible","From":"Kieran Bingham ","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","Date":"Tue, 09 Nov 2021 12:08:50 +0000","Message-ID":"<163645973070.1606134.529580468315728097@Monstersaurus>","User-Agent":"alot/0.9.1","Resent-From":"Kieran Bingham ","Resent-To":"parsemail@patchwork.libcamera.org"}}]