[{"id":16393,"web_url":"https://patchwork.libcamera.org/comment/16393/","msgid":"<3cf114af-a467-bc6a-8070-65c2e0c5a6bd@ideasonboard.com>","date":"2021-04-20T17:31:10","subject":"Re: [libcamera-devel] [PATCH v4 6/6] libcamera: request: Make it\n\textensible","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Kieran,\n\nThanks for the patch !\n\nOn 20/04/2021 15:07, Kieran Bingham wrote:\n> Provide an extensible private object for the Request class.\n> This allows us to make modifications to the private API and storage of\n> requests without affecting the public API or ABI.\n> \n> The D pointer is obtained in all Request functions implemented in the\n> request.cpp file along with an assertion that the D pointer was valid to\n> provide extra validation checking that the Request has not been\n> deleted, while in use as it is 'owned' by the application.\n> \n> Signed-off-by: Kieran Bingham \n> \nReviewed-by: Jean-Michel Hautbois \n> ---\n> The assertions added in findBuffer, complete() and completeBuffer()\n> allow us to ensure that the Request is still valid while asynchronous\n> actions occur on the Request internally in libcamera, and provide\n> (almost) equivalent functionality as the Request Canary previously\n> proposed.\n> \n> The additions in reuse() and addBuffer() are called from applications,\n> so the assertions may be less helpful there, but I've added them for\n> consistency.\n> \n> include/libcamera/request.h | 4 +++-\n> src/libcamera/request.cpp | 34 ++++++++++++++++++++++++++++++++--\n> 2 files changed, 35 insertions(+), 3 deletions(-)\n> \n> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> index 4cf5ff3f7d3b..909a1aebc2d2 100644\n> --- a/include/libcamera/request.h\n> +++ b/include/libcamera/request.h\n> @@ -24,8 +24,10 @@ class CameraControlValidator;\n> class FrameBuffer;\n> class Stream;\n> \n> -class Request\n> +class Request : public Extensible\n> {\n> +\tLIBCAMERA_DECLARE_PRIVATE(Request)\n> +\n> public:\n> \tenum Status {\n> \t\tRequestPending,\n> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> index ce2dd7b17f10..977bfac4fce6 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -28,6 +28,19 @@ namespace libcamera {\n> \n> LOG_DEFINE_CATEGORY(Request)\n> \n> +class Request::Private : public Extensible::Private\n> +{\n> +\tLIBCAMERA_DECLARE_PUBLIC(Request)\n> +\n> +public:\n> +\tPrivate(Request *request);\n> +};\n> +\n> +Request::Private::Private(Request *request)\n> +\t: Extensible::Private(request)\n> +{\n> +}\n> +\n> /**\n> * \\enum Request::Status\n> * Request completion status\n> @@ -73,8 +86,8 @@ LOG_DEFINE_CATEGORY(Request)\n> *\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(new Private(this)), camera_(camera), sequence_(0),\n> +\t cookie_(cookie), status_(RequestPending), cancelled_(false)\n> {\n> \t/**\n> \t * \\todo Should the Camera expose a validator instance, to avoid\n> @@ -114,6 +127,9 @@ Request::~Request()\n> */\n> void Request::reuse(ReuseFlag flags)\n> {\n> +\tPrivate *const d = LIBCAMERA_D_PTR();\n> +\tASSERT(d);\n> +\n> \tLIBCAMERA_TRACEPOINT(request_reuse, this);\n> \n> \tpending_.clear();\n> @@ -179,6 +195,9 @@ void Request::reuse(ReuseFlag flags)\n> */\n> int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)\n> {\n> +\tPrivate *const d = LIBCAMERA_D_PTR();\n> +\tASSERT(d);\n> +\n> \tif (!stream) {\n> \t\tLOG(Request, Error) << \"Invalid stream reference\";\n> \t\treturn -EINVAL;\n> @@ -214,6 +233,9 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)\n> */\n> FrameBuffer *Request::findBuffer(const Stream *stream) const\n> {\n> +\tconst Private *const d = LIBCAMERA_D_PTR();\n> +\tASSERT(d);\n> +\n> \tconst auto it = bufferMap_.find(stream);\n> \tif (it == bufferMap_.end())\n> \t\treturn nullptr;\n> @@ -282,6 +304,8 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const\n> */\n> void Request::complete()\n> {\n> +\tPrivate *const d = LIBCAMERA_D_PTR();\n> +\tASSERT(d);\n> \tASSERT(status_ == RequestPending);\n> \tASSERT(!hasPendingBuffers());\n> \n> @@ -307,6 +331,9 @@ void Request::complete()\n> */\n> bool Request::completeBuffer(FrameBuffer *buffer)\n> {\n> +\tPrivate *const d = LIBCAMERA_D_PTR();\n> +\tASSERT(d);\n> +\n> \tLIBCAMERA_TRACEPOINT(request_complete_buffer, this, buffer);\n> \n> \tint ret = pending_.erase(buffer);\n> @@ -330,6 +357,9 @@ bool Request::completeBuffer(FrameBuffer *buffer)\n> */\n> std::string Request::toString() const\n> {\n> +\tconst Private *const d = LIBCAMERA_D_PTR();\n> +\tASSERT(d);\n> +\n> \tstd::stringstream ss;\n> \n> \t/* Pending, Completed, Cancelled(X). */\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 EE1C0BDB16\n\tfor ;\n\tTue, 20 Apr 2021 17:31:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7A9A96883E;\n\tTue, 20 Apr 2021 19:31:12 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DFE5360516\n\tfor ;\n\tTue, 20 Apr 2021 19:31:10 +0200 (CEST)","from [IPv6:2a01:e0a:169:7140:6781:d49:79ac:cf13] (unknown\n\t[IPv6:2a01:e0a:169:7140:6781:d49:79ac:cf13])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 619DF411;\n\tTue, 20 Apr 2021 19:31:10 +0200 (CEST)"],"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=\"L3Ujymr0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1618939870;\n\tbh=pr/JiKvUYc3Y/sApR6Kfa6mpV4zc9tJlHBMiUgepOjM=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=L3Ujymr06gqft8hErZimS7/g2Avii4mEebYk/VGv356NbIvpzKwo1tMyXr9Qq3hYq\n\ts4gwy9RrISb8Bc5boP/NWTnd7Uxll5ygQTDc2Ov9NG+Zaq7TSFY/cu9zTce1W4SmPc\n\tAU9MegdzHIJ9yHjt3sx1iVlxiHDSAPsHWyQdjBQE=","To":"Kieran Bingham ,\n\tlibcamera devel ","References":"<20210420130741.236848-1-kieran.bingham@ideasonboard.com>\n\t<20210420130741.236848-7-kieran.bingham@ideasonboard.com>","From":"Jean-Michel Hautbois ","Message-ID":"<3cf114af-a467-bc6a-8070-65c2e0c5a6bd@ideasonboard.com>","Date":"Tue, 20 Apr 2021 19:31:10 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.7.1","MIME-Version":"1.0","In-Reply-To":"<20210420130741.236848-7-kieran.bingham@ideasonboard.com>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v4 6/6] libcamera: request: Make it\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","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16414,"web_url":"https://patchwork.libcamera.org/comment/16414/","msgid":"","date":"2021-04-20T22:18:11","subject":"Re: [libcamera-devel] [PATCH v4 6/6] libcamera: request: Make it\n\textensible","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Tue, Apr 20, 2021 at 02:07:41PM +0100, Kieran Bingham wrote:\n> Provide an extensible private object for the Request class.\n> This allows us to make modifications to the private API and storage of\n> requests without affecting the public API or ABI.\n> \n> The D pointer is obtained in all Request functions implemented in the\n> request.cpp file along with an assertion that the D pointer was valid to\n> provide extra validation checking that the Request has not been\n> deleted, while in use as it is 'owned' by the application.\n\ns/, while in use/ while in use,/\n\n> \n> Signed-off-by: Kieran Bingham \n> \n> ---\n> The assertions added in findBuffer, complete() and completeBuffer()\n> allow us to ensure that the Request is still valid while asynchronous\n> actions occur on the Request internally in libcamera, and provide\n> (almost) equivalent functionality as the Request Canary previously\n> proposed.\n\nDoes std::unique_ptr<> guarantees that it will reset its internal\npointer when deleted ? libc++ calls reset() in the destructor, and\nstdlibc++ seems to set the pointer to nullptr manually, but that doesn't\nseem to be guaranteed by the C++ standard.\n\n> The additions in reuse() and addBuffer() are called from applications,\n> so the assertions may be less helpful there, but I've added them for\n> consistency.\n> \n> include/libcamera/request.h | 4 +++-\n> src/libcamera/request.cpp | 34 ++++++++++++++++++++++++++++++++--\n> 2 files changed, 35 insertions(+), 3 deletions(-)\n> \n> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> index 4cf5ff3f7d3b..909a1aebc2d2 100644\n> --- a/include/libcamera/request.h\n> +++ b/include/libcamera/request.h\n> @@ -24,8 +24,10 @@ class CameraControlValidator;\n> class FrameBuffer;\n> class Stream;\n> \n> -class Request\n> +class Request : public Extensible\n> {\n> +\tLIBCAMERA_DECLARE_PRIVATE(Request)\n> +\n> public:\n> \tenum Status {\n> \t\tRequestPending,\n> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> index ce2dd7b17f10..977bfac4fce6 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -28,6 +28,19 @@ namespace libcamera {\n> \n> LOG_DEFINE_CATEGORY(Request)\n> \n> +class Request::Private : public Extensible::Private\n> +{\n> +\tLIBCAMERA_DECLARE_PUBLIC(Request)\n> +\n> +public:\n> +\tPrivate(Request *request);\n> +};\n> +\n> +Request::Private::Private(Request *request)\n> +\t: Extensible::Private(request)\n> +{\n> +}\n> +\n> /**\n> * \\enum Request::Status\n> * Request completion status\n> @@ -73,8 +86,8 @@ LOG_DEFINE_CATEGORY(Request)\n> *\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(new Private(this)), camera_(camera), sequence_(0),\n> +\t cookie_(cookie), status_(RequestPending), cancelled_(false)\n\nShould we move some of the data to Private (in a subsequent patch) ? As\nan exercise, how about moving the member data that we think will be\nsubject to change when we'll rework the request completion API, and see\nif we manage to complete that rework without breaking the API of the\nrequest class ?\n\nA subsequent patch should also move the public functions that are not\ncalled by applications to the Private class.\n\n> {\n> \t/**\n> \t * \\todo Should the Camera expose a validator instance, to avoid\n> @@ -114,6 +127,9 @@ Request::~Request()\n> */\n> void Request::reuse(ReuseFlag flags)\n> {\n> +\tPrivate *const d = LIBCAMERA_D_PTR();\n> +\tASSERT(d);\n> +\n> \tLIBCAMERA_TRACEPOINT(request_reuse, this);\n> \n> \tpending_.clear();\n> @@ -179,6 +195,9 @@ void Request::reuse(ReuseFlag flags)\n> */\n> int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)\n> {\n> +\tPrivate *const d = LIBCAMERA_D_PTR();\n> +\tASSERT(d);\n> +\n> \tif (!stream) {\n> \t\tLOG(Request, Error) << \"Invalid stream reference\";\n> \t\treturn -EINVAL;\n> @@ -214,6 +233,9 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)\n> */\n> FrameBuffer *Request::findBuffer(const Stream *stream) const\n> {\n> +\tconst Private *const d = LIBCAMERA_D_PTR();\n> +\tASSERT(d);\n> +\n> \tconst auto it = bufferMap_.find(stream);\n> \tif (it == bufferMap_.end())\n> \t\treturn nullptr;\n> @@ -282,6 +304,8 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const\n> */\n> void Request::complete()\n> {\n> +\tPrivate *const d = LIBCAMERA_D_PTR();\n> +\tASSERT(d);\n> \tASSERT(status_ == RequestPending);\n> \tASSERT(!hasPendingBuffers());\n> \n> @@ -307,6 +331,9 @@ void Request::complete()\n> */\n> bool Request::completeBuffer(FrameBuffer *buffer)\n> {\n> +\tPrivate *const d = LIBCAMERA_D_PTR();\n> +\tASSERT(d);\n> +\n> \tLIBCAMERA_TRACEPOINT(request_complete_buffer, this, buffer);\n> \n> \tint ret = pending_.erase(buffer);\n> @@ -330,6 +357,9 @@ bool Request::completeBuffer(FrameBuffer *buffer)\n> */\n> std::string Request::toString() const\n> {\n> +\tconst Private *const d = LIBCAMERA_D_PTR();\n> +\tASSERT(d);\n> +\n> \tstd::stringstream ss;\n> \n> \t/* Pending, Completed, Cancelled(X). */","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 5DD75BDB15\n\tfor ;\n\tTue, 20 Apr 2021 22:18:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BDECF68842;\n\tWed, 21 Apr 2021 00:18:18 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 331C2602C8\n\tfor ;\n\tWed, 21 Apr 2021 00:18:17 +0200 (CEST)","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 846AE45E;\n\tWed, 21 Apr 2021 00:18:15 +0200 (CEST)"],"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=\"o7kCnBNU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1618957096;\n\tbh=vEy+Btp7xLz8UqzSYjcf+VHJyk4tYKRc2f9+Gt4mn1I=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=o7kCnBNU0fYTd1vpKzqY0MyY2E5/NmWaz9ecNkVbhbAXfjDMTA1OwuhjWL6XqZVya\n\tcjzJt1Y0Ae2YmBlfHq+QZSkzIXMrjfIRv8JevBzfhy1qu5IN79+VcO1omKaib/0w7m\n\t8GEARXe5NyksUFyiHSrC2JkakyqpGHtNPsCKPOAM=","Date":"Wed, 21 Apr 2021 01:18:11 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210420130741.236848-1-kieran.bingham@ideasonboard.com>\n\t<20210420130741.236848-7-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210420130741.236848-7-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 6/6] libcamera: request: Make it\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","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16428,"web_url":"https://patchwork.libcamera.org/comment/16428/","msgid":"","date":"2021-04-21T06:40:36","subject":"Re: [libcamera-devel] [PATCH v4 6/6] libcamera: request: Make it\n\textensible","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"On Wed, Apr 21, 2021 at 7:18 AM Laurent Pinchart\n wrote:\n>\n> Hi Kieran,\n>\n> Thank you for the patch.\n>\n> On Tue, Apr 20, 2021 at 02:07:41PM +0100, Kieran Bingham wrote:\n> > Provide an extensible private object for the Request class.\n> > This allows us to make modifications to the private API and storage of\n> > requests without affecting the public API or ABI.\n> >\n> > The D pointer is obtained in all Request functions implemented in the\n> > request.cpp file along with an assertion that the D pointer was valid to\n> > provide extra validation checking that the Request has not been\n> > deleted, while in use as it is 'owned' by the application.\n>\n> s/, while in use/ while in use,/\n>\n> >\n> > Signed-off-by: Kieran Bingham \n> >\n> > ---\n> > The assertions added in findBuffer, complete() and completeBuffer()\n> > allow us to ensure that the Request is still valid while asynchronous\n> > actions occur on the Request internally in libcamera, and provide\n> > (almost) equivalent functionality as the Request Canary previously\n> > proposed.\n>\n> Does std::unique_ptr<> guarantees that it will reset its internal\n> pointer when deleted ? libc++ calls reset() in the destructor, and\n> stdlibc++ seems to set the pointer to nullptr manually, but that doesn't\n> seem to be guaranteed by the C++ standard.\n>\n\nI wonder if this is worth doing. When d_ is invalid, Request itself is\nalso invalid.\nRegardless of unique_ptr implementation, calling a function of a\ndeleted class (Request here) causes an intended behavior.\nI would not add these ASSERTs.\n-Hiro\n\n> > The additions in reuse() and addBuffer() are called from applications,\n> > so the assertions may be less helpful there, but I've added them for\n> > consistency.\n> >\n> > include/libcamera/request.h | 4 +++-\n> > src/libcamera/request.cpp | 34 ++++++++++++++++++++++++++++++++--\n> > 2 files changed, 35 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> > index 4cf5ff3f7d3b..909a1aebc2d2 100644\n> > --- a/include/libcamera/request.h\n> > +++ b/include/libcamera/request.h\n> > @@ -24,8 +24,10 @@ class CameraControlValidator;\n> > class FrameBuffer;\n> > class Stream;\n> >\n> > -class Request\n> > +class Request : public Extensible\n> > {\n> > + LIBCAMERA_DECLARE_PRIVATE(Request)\n> > +\n> > public:\n> > enum Status {\n> > RequestPending,\n> > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > index ce2dd7b17f10..977bfac4fce6 100644\n> > --- a/src/libcamera/request.cpp\n> > +++ b/src/libcamera/request.cpp\n> > @@ -28,6 +28,19 @@ namespace libcamera {\n> >\n> > LOG_DEFINE_CATEGORY(Request)\n> >\n> > +class Request::Private : public Extensible::Private\n> > +{\n> > + LIBCAMERA_DECLARE_PUBLIC(Request)\n> > +\n> > +public:\n> > + Private(Request *request);\n> > +};\n> > +\n> > +Request::Private::Private(Request *request)\n> > + : Extensible::Private(request)\n> > +{\n> > +}\n> > +\n> > /**\n> > * \\enum Request::Status\n> > * Request completion status\n> > @@ -73,8 +86,8 @@ LOG_DEFINE_CATEGORY(Request)\n> > *\n> > */\n> > Request::Request(Camera *camera, uint64_t cookie)\n> > - : camera_(camera), sequence_(0), cookie_(cookie),\n> > - status_(RequestPending), cancelled_(false)\n> > + : Extensible(new Private(this)), camera_(camera), sequence_(0),\n> > + cookie_(cookie), status_(RequestPending), cancelled_(false)\n>\n> Should we move some of the data to Private (in a subsequent patch) ? As\n> an exercise, how about moving the member data that we think will be\n> subject to change when we'll rework the request completion API, and see\n> if we manage to complete that rework without breaking the API of the\n> request class ?\n>\n> A subsequent patch should also move the public functions that are not\n> called by applications to the Private class.\n>\n> > {\n> > /**\n> > * \\todo Should the Camera expose a validator instance, to avoid\n> > @@ -114,6 +127,9 @@ Request::~Request()\n> > */\n> > void Request::reuse(ReuseFlag flags)\n> > {\n> > + Private *const d = LIBCAMERA_D_PTR();\n> > + ASSERT(d);\n> > +\n> > LIBCAMERA_TRACEPOINT(request_reuse, this);\n> >\n> > pending_.clear();\n> > @@ -179,6 +195,9 @@ void Request::reuse(ReuseFlag flags)\n> > */\n> > int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)\n> > {\n> > + Private *const d = LIBCAMERA_D_PTR();\n> > + ASSERT(d);\n> > +\n> > if (!stream) {\n> > LOG(Request, Error) << \"Invalid stream reference\";\n> > return -EINVAL;\n> > @@ -214,6 +233,9 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)\n> > */\n> > FrameBuffer *Request::findBuffer(const Stream *stream) const\n> > {\n> > + const Private *const d = LIBCAMERA_D_PTR();\n> > + ASSERT(d);\n> > +\n> > const auto it = bufferMap_.find(stream);\n> > if (it == bufferMap_.end())\n> > return nullptr;\n> > @@ -282,6 +304,8 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const\n> > */\n> > void Request::complete()\n> > {\n> > + Private *const d = LIBCAMERA_D_PTR();\n> > + ASSERT(d);\n> > ASSERT(status_ == RequestPending);\n> > ASSERT(!hasPendingBuffers());\n> >\n> > @@ -307,6 +331,9 @@ void Request::complete()\n> > */\n> > bool Request::completeBuffer(FrameBuffer *buffer)\n> > {\n> > + Private *const d = LIBCAMERA_D_PTR();\n> > + ASSERT(d);\n> > +\n> > LIBCAMERA_TRACEPOINT(request_complete_buffer, this, buffer);\n> >\n> > int ret = pending_.erase(buffer);\n> > @@ -330,6 +357,9 @@ bool Request::completeBuffer(FrameBuffer *buffer)\n> > */\n> > std::string Request::toString() const\n> > {\n> > + const Private *const d = LIBCAMERA_D_PTR();\n> > + ASSERT(d);\n> > +\n> > std::stringstream ss;\n> >\n> > /* Pending, Completed, Cancelled(X). */\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 471EDBDB15\n\tfor ;\n\tWed, 21 Apr 2021 06:40:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B070B6883E;\n\tWed, 21 Apr 2021 08:40:49 +0200 (CEST)","from mail-ej1-x633.google.com (mail-ej1-x633.google.com\n\t[IPv6:2a00:1450:4864:20::633])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C818D602C3\n\tfor ;\n\tWed, 21 Apr 2021 08:40:47 +0200 (CEST)","by mail-ej1-x633.google.com with SMTP id w23so46047406ejb.9\n\tfor ;\n\tTue, 20 Apr 2021 23:40:47 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"HtfcfMOt\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=oXKSrGdWGRZjwghWbxd9I0XAx8zG1kAR6yoNtaFa3Mg=;\n\tb=HtfcfMOtktfYeSIGmdiXySOakedI9CZLvK7yu6g/1LbruvjOTtRu/5xzUDRjxY45MZ\n\teFt8XVRW6KfXZC9Lqdp/S8Zlt7G2qiXhHukzvOEUjoV2KCyA7glbNZy8QdvV5taipmDL\n\t1MVjTOWc/3CRQz4rJc8HjpU00i5+XH+ye3MBw=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=oXKSrGdWGRZjwghWbxd9I0XAx8zG1kAR6yoNtaFa3Mg=;\n\tb=dv9WJsbHmLMluE6H7e15+2X54dZsC6Ab9DebP8G/8UQoT+LDrpXgSyLbXv9D6ApM9M\n\tUBA+rm5rS8po8d5NPaMxd26szMfAIQlvwIsp8htPeyx8wOBNQeezNkWYUQTcFJrKOWa8\n\t3sClsBXTsIzoe4ttCer+D63UUTz+YuAq/bN62o1XIjeBZxKfCAp7g9SA0Ue2sMSk2BGh\n\tDAFhnGmtfqdVEkC6rlJgx+k2xSinIzDM5C4udj5Yk5j6De3iPlt4RGEmAE1Rk/n0nHR+\n\tvXQ6xDUdNqB1MLWSEWbgIEJWihgeoVwfSzXWm8SC9sGUTQ0V9TIYU1BKMMyvI3cJKOzp\n\tKj1Q==","X-Gm-Message-State":"AOAM530WJMFKkycHfpVOrABWQatR2USMpmvczgthcQgzD4gcMzPQM7zD\n\t2JS6oVYjnetDX+ZmCnFw0HWM80FE408osCueVpCXKZGcnRY=","X-Google-Smtp-Source":"ABdhPJyE+utLX6gn2wnO8wbKQjleIWOoh9PJB4G++XxjomBWAUXPDfNBY66i0Tq0AWyEEBmKX32moLIqgywQrI1T/S4=","X-Received":"by 2002:a17:907:1b06:: with SMTP id\n\tmp6mr32099720ejc.292.1618987247386; \n\tTue, 20 Apr 2021 23:40:47 -0700 (PDT)","MIME-Version":"1.0","References":"<20210420130741.236848-1-kieran.bingham@ideasonboard.com>\n\t<20210420130741.236848-7-kieran.bingham@ideasonboard.com>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Wed, 21 Apr 2021 15:40:36 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v4 6/6] libcamera: request: Make it\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","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16436,"web_url":"https://patchwork.libcamera.org/comment/16436/","msgid":"","date":"2021-04-21T09:01:33","subject":"Re: [libcamera-devel] [PATCH v4 6/6] libcamera: request: Make it\n\textensible","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Wed, Apr 21, 2021 at 03:40:36PM +0900, Hirokazu Honda wrote:\n> On Wed, Apr 21, 2021 at 7:18 AM Laurent Pinchart wrote:\n> > On Tue, Apr 20, 2021 at 02:07:41PM +0100, Kieran Bingham wrote:\n> > > Provide an extensible private object for the Request class.\n> > > This allows us to make modifications to the private API and storage of\n> > > requests without affecting the public API or ABI.\n> > >\n> > > The D pointer is obtained in all Request functions implemented in the\n> > > request.cpp file along with an assertion that the D pointer was valid to\n> > > provide extra validation checking that the Request has not been\n> > > deleted, while in use as it is 'owned' by the application.\n> >\n> > s/, while in use/ while in use,/\n> >\n> > > Signed-off-by: Kieran Bingham \n> > >\n> > > ---\n> > > The assertions added in findBuffer, complete() and completeBuffer()\n> > > allow us to ensure that the Request is still valid while asynchronous\n> > > actions occur on the Request internally in libcamera, and provide\n> > > (almost) equivalent functionality as the Request Canary previously\n> > > proposed.\n> >\n> > Does std::unique_ptr<> guarantees that it will reset its internal\n> > pointer when deleted ? libc++ calls reset() in the destructor, and\n> > stdlibc++ seems to set the pointer to nullptr manually, but that doesn't\n> > seem to be guaranteed by the C++ standard.\n> \n> I wonder if this is worth doing. When d_ is invalid, Request itself is\n> also invalid.\n> Regardless of unique_ptr implementation, calling a function of a\n> deleted class (Request here) causes an intended behavior.\n> I would not add these ASSERTs.\n\nIt's definitely an invalid behaviour, and shouldn't happen. The purpose\nof the ASSERT() is to ensure it will be caught right away, instead of\nrunning incorrectly and possibly corrupting memory that will lead to a\ncrash later. Delayed crashes are very annoying to debug.\n\n> > > The additions in reuse() and addBuffer() are called from applications,\n> > > so the assertions may be less helpful there, but I've added them for\n> > > consistency.\n> > >\n> > > include/libcamera/request.h | 4 +++-\n> > > src/libcamera/request.cpp | 34 ++++++++++++++++++++++++++++++++--\n> > > 2 files changed, 35 insertions(+), 3 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> > > index 4cf5ff3f7d3b..909a1aebc2d2 100644\n> > > --- a/include/libcamera/request.h\n> > > +++ b/include/libcamera/request.h\n> > > @@ -24,8 +24,10 @@ class CameraControlValidator;\n> > > class FrameBuffer;\n> > > class Stream;\n> > >\n> > > -class Request\n> > > +class Request : public Extensible\n> > > {\n> > > + LIBCAMERA_DECLARE_PRIVATE(Request)\n> > > +\n> > > public:\n> > > enum Status {\n> > > RequestPending,\n> > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > > index ce2dd7b17f10..977bfac4fce6 100644\n> > > --- a/src/libcamera/request.cpp\n> > > +++ b/src/libcamera/request.cpp\n> > > @@ -28,6 +28,19 @@ namespace libcamera {\n> > >\n> > > LOG_DEFINE_CATEGORY(Request)\n> > >\n> > > +class Request::Private : public Extensible::Private\n> > > +{\n> > > + LIBCAMERA_DECLARE_PUBLIC(Request)\n> > > +\n> > > +public:\n> > > + Private(Request *request);\n> > > +};\n> > > +\n> > > +Request::Private::Private(Request *request)\n> > > + : Extensible::Private(request)\n> > > +{\n> > > +}\n> > > +\n> > > /**\n> > > * \\enum Request::Status\n> > > * Request completion status\n> > > @@ -73,8 +86,8 @@ LOG_DEFINE_CATEGORY(Request)\n> > > *\n> > > */\n> > > Request::Request(Camera *camera, uint64_t cookie)\n> > > - : camera_(camera), sequence_(0), cookie_(cookie),\n> > > - status_(RequestPending), cancelled_(false)\n> > > + : Extensible(new Private(this)), camera_(camera), sequence_(0),\n> > > + cookie_(cookie), status_(RequestPending), cancelled_(false)\n> >\n> > Should we move some of the data to Private (in a subsequent patch) ? As\n> > an exercise, how about moving the member data that we think will be\n> > subject to change when we'll rework the request completion API, and see\n> > if we manage to complete that rework without breaking the API of the\n> > request class ?\n> >\n> > A subsequent patch should also move the public functions that are not\n> > called by applications to the Private class.\n> >\n> > > {\n> > > /**\n> > > * \\todo Should the Camera expose a validator instance, to avoid\n> > > @@ -114,6 +127,9 @@ Request::~Request()\n> > > */\n> > > void Request::reuse(ReuseFlag flags)\n> > > {\n> > > + Private *const d = LIBCAMERA_D_PTR();\n> > > + ASSERT(d);\n> > > +\n> > > LIBCAMERA_TRACEPOINT(request_reuse, this);\n> > >\n> > > pending_.clear();\n> > > @@ -179,6 +195,9 @@ void Request::reuse(ReuseFlag flags)\n> > > */\n> > > int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)\n> > > {\n> > > + Private *const d = LIBCAMERA_D_PTR();\n> > > + ASSERT(d);\n> > > +\n> > > if (!stream) {\n> > > LOG(Request, Error) << \"Invalid stream reference\";\n> > > return -EINVAL;\n> > > @@ -214,6 +233,9 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)\n> > > */\n> > > FrameBuffer *Request::findBuffer(const Stream *stream) const\n> > > {\n> > > + const Private *const d = LIBCAMERA_D_PTR();\n> > > + ASSERT(d);\n> > > +\n> > > const auto it = bufferMap_.find(stream);\n> > > if (it == bufferMap_.end())\n> > > return nullptr;\n> > > @@ -282,6 +304,8 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const\n> > > */\n> > > void Request::complete()\n> > > {\n> > > + Private *const d = LIBCAMERA_D_PTR();\n> > > + ASSERT(d);\n> > > ASSERT(status_ == RequestPending);\n> > > ASSERT(!hasPendingBuffers());\n> > >\n> > > @@ -307,6 +331,9 @@ void Request::complete()\n> > > */\n> > > bool Request::completeBuffer(FrameBuffer *buffer)\n> > > {\n> > > + Private *const d = LIBCAMERA_D_PTR();\n> > > + ASSERT(d);\n> > > +\n> > > LIBCAMERA_TRACEPOINT(request_complete_buffer, this, buffer);\n> > >\n> > > int ret = pending_.erase(buffer);\n> > > @@ -330,6 +357,9 @@ bool Request::completeBuffer(FrameBuffer *buffer)\n> > > */\n> > > std::string Request::toString() const\n> > > {\n> > > + const Private *const d = LIBCAMERA_D_PTR();\n> > > + ASSERT(d);\n> > > +\n> > > std::stringstream ss;\n> > >\n> > > /* Pending, Completed, Cancelled(X). */","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 15440BDB16\n\tfor ;\n\tWed, 21 Apr 2021 09:01:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 81FC768806;\n\tWed, 21 Apr 2021 11:01:39 +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 00AB4602C8\n\tfor ;\n\tWed, 21 Apr 2021 11:01:37 +0200 (CEST)","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 735553EE;\n\tWed, 21 Apr 2021 11:01:37 +0200 (CEST)"],"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=\"sl74Gj3t\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1618995697;\n\tbh=iK1Es4V6w8q/dUDwWW7rBRWEEJJDcDIWIkFgyVPP+x4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=sl74Gj3t1HTJeoz2CCo418vICKTH+OLm/3Fy7dd2oTxJ6Xlnpzw77KrTu9L9JNIse\n\tBJE3wi64HFGRl30shL/edK9TerjL/4JYwaK3zoXrUn9v4qmMAGFCnAxNlkQnTB24LG\n\te1zPr4QjvAwPXSUkw7y45WshX+b3Q1D/WjYcB+cY=","Date":"Wed, 21 Apr 2021 12:01:33 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210420130741.236848-1-kieran.bingham@ideasonboard.com>\n\t<20210420130741.236848-7-kieran.bingham@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v4 6/6] libcamera: request: Make it\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","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16512,"web_url":"https://patchwork.libcamera.org/comment/16512/","msgid":"","date":"2021-04-22T10:22:24","subject":"Re: [libcamera-devel] [PATCH v4 6/6] libcamera: request: Make it\n\textensible","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 20/04/2021 23:18, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Tue, Apr 20, 2021 at 02:07:41PM +0100, Kieran Bingham wrote:\n>> Provide an extensible private object for the Request class.\n>> This allows us to make modifications to the private API and storage of\n>> requests without affecting the public API or ABI.\n>>\n>> The D pointer is obtained in all Request functions implemented in the\n>> request.cpp file along with an assertion that the D pointer was valid to\n>> provide extra validation checking that the Request has not been\n>> deleted, while in use as it is 'owned' by the application.\n> \n> s/, while in use/ while in use,/\n> \n>>\n>> Signed-off-by: Kieran Bingham \n>>\n>> ---\n>> The assertions added in findBuffer, complete() and completeBuffer()\n>> allow us to ensure that the Request is still valid while asynchronous\n>> actions occur on the Request internally in libcamera, and provide\n>> (almost) equivalent functionality as the Request Canary previously\n>> proposed.\n> \n> Does std::unique_ptr<> guarantees that it will reset its internal\n> pointer when deleted ? libc++ calls reset() in the destructor, and\n> stdlibc++ seems to set the pointer to nullptr manually, but that doesn't\n> seem to be guaranteed by the C++ standard.\n\nI've manually tested that it gets reset, but that doesn't offer any\nguarantee beyond that.\n\n\n\n\n>> The additions in reuse() and addBuffer() are called from applications,\n>> so the assertions may be less helpful there, but I've added them for\n>> consistency.\n>>\n>> include/libcamera/request.h | 4 +++-\n>> src/libcamera/request.cpp | 34 ++++++++++++++++++++++++++++++++--\n>> 2 files changed, 35 insertions(+), 3 deletions(-)\n>>\n>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n>> index 4cf5ff3f7d3b..909a1aebc2d2 100644\n>> --- a/include/libcamera/request.h\n>> +++ b/include/libcamera/request.h\n>> @@ -24,8 +24,10 @@ class CameraControlValidator;\n>> class FrameBuffer;\n>> class Stream;\n>> \n>> -class Request\n>> +class Request : public Extensible\n>> {\n>> +\tLIBCAMERA_DECLARE_PRIVATE(Request)\n>> +\n>> public:\n>> \tenum Status {\n>> \t\tRequestPending,\n>> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n>> index ce2dd7b17f10..977bfac4fce6 100644\n>> --- a/src/libcamera/request.cpp\n>> +++ b/src/libcamera/request.cpp\n>> @@ -28,6 +28,19 @@ namespace libcamera {\n>> \n>> LOG_DEFINE_CATEGORY(Request)\n>> \n>> +class Request::Private : public Extensible::Private\n>> +{\n>> +\tLIBCAMERA_DECLARE_PUBLIC(Request)\n>> +\n>> +public:\n>> +\tPrivate(Request *request);\n>> +};\n>> +\n>> +Request::Private::Private(Request *request)\n>> +\t: Extensible::Private(request)\n>> +{\n>> +}\n>> +\n>> /**\n>> * \\enum Request::Status\n>> * Request completion status\n>> @@ -73,8 +86,8 @@ LOG_DEFINE_CATEGORY(Request)\n>> *\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(new Private(this)), camera_(camera), sequence_(0),\n>> +\t cookie_(cookie), status_(RequestPending), cancelled_(false)\n> \n> Should we move some of the data to Private (in a subsequent patch) ? As\n> an exercise, how about moving the member data that we think will be\n> subject to change when we'll rework the request completion API, and see\n> if we manage to complete that rework without breaking the API of the\n> request class ?\n\nOf course, the goal of this patch isn't just to add assertions, it's to\nmake it extensible so we can extend it in subsequent patches ;-)\n\n\n> A subsequent patch should also move the public functions that are not\n> called by applications to the Private class.\n\nAgreed, I wanted to keep this patch as simple as possible to start with.\n\n> \n>> {\n>> \t/**\n>> \t * \\todo Should the Camera expose a validator instance, to avoid\n>> @@ -114,6 +127,9 @@ Request::~Request()\n>> */\n>> void Request::reuse(ReuseFlag flags)\n>> {\n>> +\tPrivate *const d = LIBCAMERA_D_PTR();\n>> +\tASSERT(d);\n>> +\n>> \tLIBCAMERA_TRACEPOINT(request_reuse, this);\n>> \n>> \tpending_.clear();\n>> @@ -179,6 +195,9 @@ void Request::reuse(ReuseFlag flags)\n>> */\n>> int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)\n>> {\n>> +\tPrivate *const d = LIBCAMERA_D_PTR();\n>> +\tASSERT(d);\n>> +\n>> \tif (!stream) {\n>> \t\tLOG(Request, Error) << \"Invalid stream reference\";\n>> \t\treturn -EINVAL;\n>> @@ -214,6 +233,9 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)\n>> */\n>> FrameBuffer *Request::findBuffer(const Stream *stream) const\n>> {\n>> +\tconst Private *const d = LIBCAMERA_D_PTR();\n>> +\tASSERT(d);\n>> +\n>> \tconst auto it = bufferMap_.find(stream);\n>> \tif (it == bufferMap_.end())\n>> \t\treturn nullptr;\n>> @@ -282,6 +304,8 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const\n>> */\n>> void Request::complete()\n>> {\n>> +\tPrivate *const d = LIBCAMERA_D_PTR();\n>> +\tASSERT(d);\n>> \tASSERT(status_ == RequestPending);\n>> \tASSERT(!hasPendingBuffers());\n>> \n>> @@ -307,6 +331,9 @@ void Request::complete()\n>> */\n>> bool Request::completeBuffer(FrameBuffer *buffer)\n>> {\n>> +\tPrivate *const d = LIBCAMERA_D_PTR();\n>> +\tASSERT(d);\n>> +\n>> \tLIBCAMERA_TRACEPOINT(request_complete_buffer, this, buffer);\n>> \n>> \tint ret = pending_.erase(buffer);\n>> @@ -330,6 +357,9 @@ bool Request::completeBuffer(FrameBuffer *buffer)\n>> */\n>> std::string Request::toString() const\n>> {\n>> +\tconst Private *const d = LIBCAMERA_D_PTR();\n>> +\tASSERT(d);\n>> +\n>> \tstd::stringstream ss;\n>> \n>> \t/* Pending, Completed, Cancelled(X). */\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 C9ED9BDB15\n\tfor ;\n\tThu, 22 Apr 2021 10:22:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1925668851;\n\tThu, 22 Apr 2021 12:22:29 +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 8C4D168806\n\tfor ;\n\tThu, 22 Apr 2021 12:22:27 +0200 (CEST)","from [192.168.0.20]\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 F3EF73EE;\n\tThu, 22 Apr 2021 12:22:26 +0200 (CEST)"],"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=\"D6CSMzwW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619086947;\n\tbh=gCiiFUkiTB4d+65k08htpFPaMtPLexkfsv3LKoU6ONE=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=D6CSMzwWffpPNHqpcyuWIVJQQb2JvBddK5aLoIFHO330wxZU7A0mvHKh2vH+ICE5s\n\tcnXosKM5bqPhFNaZPoyXpekDp4R0etA+MMBTE4sYdopwRsSSExKO1PYFzjAAj89rxa\n\tVZE4qbHJNx+b/OoQFMG/ZE2p3cA4+lu/X/rErAAQ=","To":"Laurent Pinchart ","References":"<20210420130741.236848-1-kieran.bingham@ideasonboard.com>\n\t<20210420130741.236848-7-kieran.bingham@ideasonboard.com>\n\t","From":"Kieran Bingham ","Organization":"Ideas on Board","Message-ID":"","Date":"Thu, 22 Apr 2021 11:22:24 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.7.1","MIME-Version":"1.0","In-Reply-To":"","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v4 6/6] libcamera: request: Make it\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","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16513,"web_url":"https://patchwork.libcamera.org/comment/16513/","msgid":"","date":"2021-04-22T10:27:51","subject":"Re: [libcamera-devel] [PATCH v4 6/6] libcamera: request: Make it\n\textensible","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 21/04/2021 10:01, Laurent Pinchart wrote:\n> Hi Hiro,\n> \n> On Wed, Apr 21, 2021 at 03:40:36PM +0900, Hirokazu Honda wrote:\n>> On Wed, Apr 21, 2021 at 7:18 AM Laurent Pinchart wrote:\n>>> On Tue, Apr 20, 2021 at 02:07:41PM +0100, Kieran Bingham wrote:\n>>>> Provide an extensible private object for the Request class.\n>>>> This allows us to make modifications to the private API and storage of\n>>>> requests without affecting the public API or ABI.\n>>>>\n>>>> The D pointer is obtained in all Request functions implemented in the\n>>>> request.cpp file along with an assertion that the D pointer was valid to\n>>>> provide extra validation checking that the Request has not been\n>>>> deleted, while in use as it is 'owned' by the application.\n>>>\n>>> s/, while in use/ while in use,/\n>>>\n>>>> Signed-off-by: Kieran Bingham \n>>>>\n>>>> ---\n>>>> The assertions added in findBuffer, complete() and completeBuffer()\n>>>> allow us to ensure that the Request is still valid while asynchronous\n>>>> actions occur on the Request internally in libcamera, and provide\n>>>> (almost) equivalent functionality as the Request Canary previously\n>>>> proposed.\n>>>\n>>> Does std::unique_ptr<> guarantees that it will reset its internal\n>>> pointer when deleted ? libc++ calls reset() in the destructor, and\n>>> stdlibc++ seems to set the pointer to nullptr manually, but that doesn't\n>>> seem to be guaranteed by the C++ standard.\n>>\n>> I wonder if this is worth doing. When d_ is invalid, Request itself is\n>> also invalid.\n>> Regardless of unique_ptr implementation, calling a function of a\n>> deleted class (Request here) causes an intended behavior.\n>> I would not add these ASSERTs.\n> \n> It's definitely an invalid behaviour, and shouldn't happen. The purpose\n> of the ASSERT() is to ensure it will be caught right away, instead of\n> running incorrectly and possibly corrupting memory that will lead to a\n> crash later. Delayed crashes are very annoying to debug.\n\nExactly that.\n\nI spent quite some time debugging IPU3 issues and one of the rabbit\nholes was that we were sending 'completed' Requests back to the\napplication before they were really completed.\n\nThis meant that the application could free the request while it was\nstill in use by libcamera (and thus crashed in arbitrary distant places).\n\nNow granted, I've (in this series) added some extra protection to try to\ncatch that early to make sure when we stop all requests are guaranteed\nto be handed back, but there's still the scope that the application is\nin control of and expected to delete requests. If anything happens on a\nrequest after it has been deleted - I want to make sure we fire an\nassertion as quickly as possible, declaring that.\n\nThat's what led to the 'REQUEST_CANARY', which is now (almost\nequivalently, but not exactly) handled by this.\n\n\n>>>> The additions in reuse() and addBuffer() are called from applications,\n>>>> so the assertions may be less helpful there, but I've added them for\n>>>> consistency.\n>>>>\n>>>> include/libcamera/request.h | 4 +++-\n>>>> src/libcamera/request.cpp | 34 ++++++++++++++++++++++++++++++++--\n>>>> 2 files changed, 35 insertions(+), 3 deletions(-)\n>>>>\n>>>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n>>>> index 4cf5ff3f7d3b..909a1aebc2d2 100644\n>>>> --- a/include/libcamera/request.h\n>>>> +++ b/include/libcamera/request.h\n>>>> @@ -24,8 +24,10 @@ class CameraControlValidator;\n>>>> class FrameBuffer;\n>>>> class Stream;\n>>>>\n>>>> -class Request\n>>>> +class Request : public Extensible\n>>>> {\n>>>> + LIBCAMERA_DECLARE_PRIVATE(Request)\n>>>> +\n>>>> public:\n>>>> enum Status {\n>>>> RequestPending,\n>>>> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n>>>> index ce2dd7b17f10..977bfac4fce6 100644\n>>>> --- a/src/libcamera/request.cpp\n>>>> +++ b/src/libcamera/request.cpp\n>>>> @@ -28,6 +28,19 @@ namespace libcamera {\n>>>>\n>>>> LOG_DEFINE_CATEGORY(Request)\n>>>>\n>>>> +class Request::Private : public Extensible::Private\n>>>> +{\n>>>> + LIBCAMERA_DECLARE_PUBLIC(Request)\n>>>> +\n>>>> +public:\n>>>> + Private(Request *request);\n>>>> +};\n>>>> +\n>>>> +Request::Private::Private(Request *request)\n>>>> + : Extensible::Private(request)\n>>>> +{\n>>>> +}\n>>>> +\n>>>> /**\n>>>> * \\enum Request::Status\n>>>> * Request completion status\n>>>> @@ -73,8 +86,8 @@ LOG_DEFINE_CATEGORY(Request)\n>>>> *\n>>>> */\n>>>> Request::Request(Camera *camera, uint64_t cookie)\n>>>> - : camera_(camera), sequence_(0), cookie_(cookie),\n>>>> - status_(RequestPending), cancelled_(false)\n>>>> + : Extensible(new Private(this)), camera_(camera), sequence_(0),\n>>>> + cookie_(cookie), status_(RequestPending), cancelled_(false)\n>>>\n>>> Should we move some of the data to Private (in a subsequent patch) ? As\n>>> an exercise, how about moving the member data that we think will be\n>>> subject to change when we'll rework the request completion API, and see\n>>> if we manage to complete that rework without breaking the API of the\n>>> request class ?\n>>>\n>>> A subsequent patch should also move the public functions that are not\n>>> called by applications to the Private class.\n>>>\n>>>> {\n>>>> /**\n>>>> * \\todo Should the Camera expose a validator instance, to avoid\n>>>> @@ -114,6 +127,9 @@ Request::~Request()\n>>>> */\n>>>> void Request::reuse(ReuseFlag flags)\n>>>> {\n>>>> + Private *const d = LIBCAMERA_D_PTR();\n>>>> + ASSERT(d);\n>>>> +\n>>>> LIBCAMERA_TRACEPOINT(request_reuse, this);\n>>>>\n>>>> pending_.clear();\n>>>> @@ -179,6 +195,9 @@ void Request::reuse(ReuseFlag flags)\n>>>> */\n>>>> int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)\n>>>> {\n>>>> + Private *const d = LIBCAMERA_D_PTR();\n>>>> + ASSERT(d);\n>>>> +\n>>>> if (!stream) {\n>>>> LOG(Request, Error) << \"Invalid stream reference\";\n>>>> return -EINVAL;\n>>>> @@ -214,6 +233,9 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)\n>>>> */\n>>>> FrameBuffer *Request::findBuffer(const Stream *stream) const\n>>>> {\n>>>> + const Private *const d = LIBCAMERA_D_PTR();\n>>>> + ASSERT(d);\n>>>> +\n>>>> const auto it = bufferMap_.find(stream);\n>>>> if (it == bufferMap_.end())\n>>>> return nullptr;\n>>>> @@ -282,6 +304,8 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const\n>>>> */\n>>>> void Request::complete()\n>>>> {\n>>>> + Private *const d = LIBCAMERA_D_PTR();\n>>>> + ASSERT(d);\n>>>> ASSERT(status_ == RequestPending);\n>>>> ASSERT(!hasPendingBuffers());\n>>>>\n>>>> @@ -307,6 +331,9 @@ void Request::complete()\n>>>> */\n>>>> bool Request::completeBuffer(FrameBuffer *buffer)\n>>>> {\n>>>> + Private *const d = LIBCAMERA_D_PTR();\n>>>> + ASSERT(d);\n>>>> +\n>>>> LIBCAMERA_TRACEPOINT(request_complete_buffer, this, buffer);\n>>>>\n>>>> int ret = pending_.erase(buffer);\n>>>> @@ -330,6 +357,9 @@ bool Request::completeBuffer(FrameBuffer *buffer)\n>>>> */\n>>>> std::string Request::toString() const\n>>>> {\n>>>> + const Private *const d = LIBCAMERA_D_PTR();\n>>>> + ASSERT(d);\n>>>> +\n>>>> std::stringstream ss;\n>>>>\n>>>> /* Pending, Completed, Cancelled(X). */\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 9445EBDB17\n\tfor ;\n\tThu, 22 Apr 2021 10:27:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EF4D668852;\n\tThu, 22 Apr 2021 12:27:55 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C8ECA60516\n\tfor ;\n\tThu, 22 Apr 2021 12:27:54 +0200 (CEST)","from [192.168.0.20]\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 279463EE;\n\tThu, 22 Apr 2021 12:27:54 +0200 (CEST)"],"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=\"TH04wYXI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619087274;\n\tbh=VfM7I9cSfAl/FkjjoTnMxqdMd+5Rupzldr3lXPDnW+k=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=TH04wYXIiTnvctI7ipUgKKg/P5StOUJaAFHL7aWFZw5TNS/IlTXz3LwabwUsfJPE8\n\ttwXh+a6J4oA0AXIbdqi7LEpjLnloJJeOnLWxyjg4bnbObY/NKdH19f0DFRDsp7t+pp\n\tfQt3uuOwISmOUb84E1J934ZdToOmrwDT1x/yVlNY=","To":"Laurent Pinchart ,\n\tHirokazu Honda ","References":"<20210420130741.236848-1-kieran.bingham@ideasonboard.com>\n\t<20210420130741.236848-7-kieran.bingham@ideasonboard.com>\n\t\n\t\n\t","From":"Kieran Bingham ","Organization":"Ideas on Board","Message-ID":"","Date":"Thu, 22 Apr 2021 11:27:51 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.7.1","MIME-Version":"1.0","In-Reply-To":"","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v4 6/6] libcamera: request: Make it\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","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]