[{"id":20749,"web_url":"https://patchwork.libcamera.org/comment/20749/","msgid":"<163645973070.1606134.529580468315728097@Monstersaurus>","date":"2021-11-09T12:08:50","subject":"Re: [libcamera-devel] [PATCH 01/10] libcamera: request: Make\n\tRequest class 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 lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 2AEC0BF415\n\tfor ;\n\tTue, 9 Nov 2021 12:08:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5B3FD6035D;\n\tTue, 9 Nov 2021 13:08:55 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D9842600BF\n\tfor ;\n\tTue, 9 Nov 2021 13:08:53 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 804EB9FF;\n\tTue, 9 Nov 2021 13:08:53 +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=\"epjEVbhF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636459733;\n\tbh=y6wOlbgU9bD1jg7Xwl4KN9tSweFywiHQb7As7rqgog0=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=epjEVbhFdcSHohd3rFtQeiFpKsSWeGUrwGxx1H0MHcthu9HPBR4VdIJdAVrW3uDL6\n\tNeZr4dBHvouaQl2rCK1NBVaD5HUzxDtoE+rife7V4CX3nY/Ru54AwOE8FUoxLPZZwK\n\tabJrmK7Are85sZ4IrdTFS2R9y3hXTIzBi8De9Jlo=","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>","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","Subject":"Re: [libcamera-devel] [PATCH 01/10] libcamera: request: Make\n\tRequest class Extensible","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\" "}},{"id":20846,"web_url":"https://patchwork.libcamera.org/comment/20846/","msgid":"<20211111083008.smwxroca4trnzdjd@uno.localdomain>","date":"2021-11-11T08:30:08","subject":"Re: [libcamera-devel] [PATCH 01/10] libcamera: request: Make\n\tRequest class Extensible","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"On Tue, Nov 09, 2021 at 12:08:50PM +0000, Kieran Bingham wrote:\n> 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>\n> Reviewed-by: Kieran Bingham \n>\n> Happy to get this patch merged earlier if it helps too, as it unblocks\n> other patches.\n\nFTR I get this warning from checkstyle on this patch.\nWarning that I'm afraid I don't understand\n\n--- src/libcamera/request.cpp\n+++ src/libcamera/request.cpp\n@@ -15,12 +15,12 @@\n #include \n #include \n #include \n+#include \n #include \n\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\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 lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 2155EBF415\n\tfor ;\n\tThu, 11 Nov 2021 08:29:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 90D1860345;\n\tThu, 11 Nov 2021 09:29:17 +0100 (CET)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 396CD6032C\n\tfor ;\n\tThu, 11 Nov 2021 09:29:16 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 9DE891C000C;\n\tThu, 11 Nov 2021 08:29:15 +0000 (UTC)"],"Date":"Thu, 11 Nov 2021 09:30:08 +0100","From":"Jacopo Mondi ","To":"Kieran Bingham ","Message-ID":"<20211111083008.smwxroca4trnzdjd@uno.localdomain>","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-2-jacopo@jmondi.org>\n\t<163645973070.1606134.529580468315728097@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<163645973070.1606134.529580468315728097@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH 01/10] libcamera: request: Make\n\tRequest class Extensible","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":20889,"web_url":"https://patchwork.libcamera.org/comment/20889/","msgid":"","date":"2021-11-12T00:37:43","subject":"Re: [libcamera-devel] [PATCH 01/10] libcamera: request: Make\n\tRequest class Extensible","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Nov 11, 2021 at 09:30:08AM +0100, Jacopo Mondi wrote:\n> On Tue, Nov 09, 2021 at 12:08:50PM +0000, Kieran Bingham wrote:\n> > 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> > Reviewed-by: Kieran Bingham \n> >\n> > Happy to get this patch merged earlier if it helps too, as it unblocks\n> > other patches.\n> \n> FTR I get this warning from checkstyle on this patch.\n> Warning that I'm afraid I don't understand\n> \n> --- src/libcamera/request.cpp\n> +++ src/libcamera/request.cpp\n> @@ -15,12 +15,12 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> \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\nThat's because clang-format moved the libcamera/internal/request.h\nheader to the top (to follow our self-containted compilation check\nthrough top inclusion rule), and moved the licamera/request.h header\nthat was at the top back in the list of libcamera public headers. Given\nthat libcamera/internal/request.h includes libcamera/request.h, I think\nthis is better. Feel free to include this diff in the next version.\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> > > }","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 18334BDB1C\n\tfor ;\n\tFri, 12 Nov 2021 00:38:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 58F4B6035D;\n\tFri, 12 Nov 2021 01:38:07 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0836660232\n\tfor ;\n\tFri, 12 Nov 2021 01:38:06 +0100 (CET)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7D5862F3;\n\tFri, 12 Nov 2021 01:38:05 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"XuKYJl9F\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636677485;\n\tbh=+KjQENVtDtbXx7nmzi4UpyTiQKZUUyhySe+o80HCAfI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=XuKYJl9FNGQ/MwGVd7xFwg3rJ0USHbpTVEGQqVb9uRHBG+u4NSmrELbFRO8abypjV\n\tkAqEYvOVm0Yni+3R/JjFHx1JOx3Ye/Q6W61GYwUDq7npPr/NYFtRKflTZN7qteDJKT\n\tmW2oCciEEj4FshJQmCa7tcvtPRurBkxd/BtHpftc=","Date":"Fri, 12 Nov 2021 02:37:43 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20211028111520.256612-1-jacopo@jmondi.org>\n\t<20211028111520.256612-2-jacopo@jmondi.org>\n\t<163645973070.1606134.529580468315728097@Monstersaurus>\n\t<20211111083008.smwxroca4trnzdjd@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211111083008.smwxroca4trnzdjd@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 01/10] libcamera: request: Make\n\tRequest class Extensible","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\" "}}]