[{"id":21066,"web_url":"https://patchwork.libcamera.org/comment/21066/","msgid":"","date":"2021-11-19T21:13:41","subject":"Re: [libcamera-devel] [PATCH] FrameBuffer: Make FrameBuffer::cancel\n\tprivate","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Dorota,\n\nThank you for the patch.\n\nThe subject line should be\n\nlibcamera: framebuffer: Make FrameBuffer::cancel() private\n\nto match the usual style (running `git log` on the file(s) you change is\na good way to see what the style is).\n\nOn Fri, Nov 19, 2021 at 04:05:58PM +0100, Dorota Czaplejewicz wrote:\n> FrameBuffer::cancel is not meant to be used by the API consumer. It was moved to FrameBuffer::Private::cancel .\n\nCommit messages should be wrapped to 72 columns (git + vim does that by\ndefault, I'm not sure about other editors).\n\nI'd write\n\nFrameBuffer::cancel() is not meant to be used by applications. Move it\nto the FrameBuffer::Private class.\n\nas \"API consumer\" could be understood as either the application, or the\ninternal API users.\n\nYour Signed-off-by line is missing (see\nhttps://libcamera.org/contributing.html#submitting-patches).\n\n> ---\n> Hi,\n> \n> I'm sending this as discussed on the mailing list.\n> \n> Regards,\n> Dorota\n> \n> include/libcamera/framebuffer.h | 2 --\n> include/libcamera/internal/framebuffer.h | 2 ++\n> src/libcamera/framebuffer.cpp | 16 ++++++++--------\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 5 +++--\n> .../pipeline/raspberrypi/raspberrypi.cpp | 2 +-\n> src/libcamera/pipeline/vimc/vimc.cpp | 3 ++-\n> src/libcamera/request.cpp | 2 +-\n> 7 files changed, 17 insertions(+), 15 deletions(-)\n> \n> diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> index 7f2f176a..367a8a71 100644\n> --- a/include/libcamera/framebuffer.h\n> +++ b/include/libcamera/framebuffer.h\n> @@ -66,8 +66,6 @@ public:\n> \tunsigned int cookie() const { return cookie_; }\n> \tvoid setCookie(unsigned int cookie) { cookie_ = cookie; }\n> \n> -\tvoid cancel() { metadata_.status = FrameMetadata::FrameCancelled; }\n> -\n> private:\n> \tLIBCAMERA_DISABLE_COPY_AND_MOVE(FrameBuffer)\n> \n> diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h\n> index cd33c295..27676212 100644\n> --- a/include/libcamera/internal/framebuffer.h\n> +++ b/include/libcamera/internal/framebuffer.h\n> @@ -23,6 +23,8 @@ public:\n> \tvoid setRequest(Request *request) { request_ = request; }\n> \tbool isContiguous() const { return isContiguous_; }\n> \n> +\tvoid cancel() { LIBCAMERA_O_PTR()->metadata_.status = FrameMetadata::FrameCancelled; }\n\nWe try to keep line within a 80 columns soft limit when it doesn't\notherwise hinder readability, so this could be\n\n\tvoid cancel()\n\t{\n\t\tFrameBuffer *const o = LIBCAMERA_O_PTR();\n\t\to->metadata_.status = FrameMetadata::FrameCancelled;\n\t}\n\n> +\n> private:\n> \tRequest *request_;\n> \tbool isContiguous_;\n> diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> index 337ea115..dd344ed8 100644\n> --- a/src/libcamera/framebuffer.cpp\n> +++ b/src/libcamera/framebuffer.cpp\n> @@ -137,6 +137,14 @@ FrameBuffer::Private::Private()\n> * \\return True if the planes are stored contiguously in memory, false otherwise\n> */\n> \n> +/**\n> + * \\fn FrameBuffer::Private::cancel()\n> + * \\brief Marks the buffer as cancelled\n> + *\n> + * If a buffer is not used by a request, it shall be marked as cancelled to\n> + * indicate that the metadata is invalid.\n\nWe've discussed the fact that the documentation isn't very explicit, but\nthat should be addressed in a separate patch, not here.\n\nThe patch looks good overall. With these small issues fixed,\n\nReviewed-by: Laurent Pinchart \n\nYou can include that line in your v2.\n\n> + */\n> +\n> /**\n> * \\class FrameBuffer\n> * \\brief Frame buffer data and its associated dynamic metadata\n> @@ -305,12 +313,4 @@ Request *FrameBuffer::request() const\n> * libcamera core never modifies the buffer cookie.\n> */\n> \n> -/**\n> - * \\fn FrameBuffer::cancel()\n> - * \\brief Marks the buffer as cancelled\n> - *\n> - * If a buffer is not used by a request, it shall be marked as cancelled to\n> - * indicate that the metadata is invalid.\n> - */\n> -\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index c65afdb2..f8516cba 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -27,6 +27,7 @@\n> #include \"libcamera/internal/camera_sensor.h\"\n> #include \"libcamera/internal/delayed_controls.h\"\n> #include \"libcamera/internal/device_enumerator.h\"\n> +#include \"libcamera/internal/framebuffer.h\"\n> #include \"libcamera/internal/ipa_manager.h\"\n> #include \"libcamera/internal/media_device.h\"\n> #include \"libcamera/internal/pipeline_handler.h\"\n> @@ -816,7 +817,7 @@ void IPU3CameraData::cancelPendingRequests()\n> \n> \t\tfor (auto it : request->buffers()) {\n> \t\t\tFrameBuffer *buffer = it.second;\n> -\t\t\tbuffer->cancel();\n> +\t\t\tbuffer->_d()->cancel();\n> \t\t\tpipe()->completeBuffer(request, buffer);\n> \t\t}\n> \n> @@ -1333,7 +1334,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n> \tif (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> \t\tfor (auto it : request->buffers()) {\n> \t\t\tFrameBuffer *b = it.second;\n> -\t\t\tb->cancel();\n> +\t\t\tb->_d()->cancel();\n> \t\t\tpipe()->completeBuffer(request, b);\n> \t\t}\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 5e1f2273..b7cabf5e 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -1573,7 +1573,7 @@ void RPiCameraData::clearIncompleteRequests()\n> \t\t\t * request? If not, do so now.\n> \t\t\t */\n> \t\t\tif (buffer->request()) {\n> -\t\t\t\tbuffer->cancel();\n> +\t\t\t\tbuffer->_d()->cancel();\n> \t\t\t\tpipe()->completeBuffer(request, buffer);\n> \t\t\t}\n> \t\t}\n> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> index e453091d..1ec814d1 100644\n> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> @@ -32,6 +32,7 @@\n> #include \"libcamera/internal/camera.h\"\n> #include \"libcamera/internal/camera_sensor.h\"\n> #include \"libcamera/internal/device_enumerator.h\"\n> +#include \"libcamera/internal/framebuffer.h\"\n> #include \"libcamera/internal/ipa_manager.h\"\n> #include \"libcamera/internal/media_device.h\"\n> #include \"libcamera/internal/pipeline_handler.h\"\n> @@ -574,7 +575,7 @@ void VimcCameraData::bufferReady(FrameBuffer *buffer)\n> \tif (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> \t\tfor (auto it : request->buffers()) {\n> \t\t\tFrameBuffer *b = it.second;\n> -\t\t\tb->cancel();\n> +\t\t\tb->_d()->cancel();\n> \t\t\tpipe->completeBuffer(request, b);\n> \t\t}\n> \n> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> index 17fefab7..a4dbf750 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -304,7 +304,7 @@ void Request::cancel()\n> \tASSERT(status_ == RequestPending);\n> \n> \tfor (FrameBuffer *buffer : pending_) {\n> -\t\tbuffer->cancel();\n> +\t\tbuffer->_d()->cancel();\n> \t\tcamera_->bufferCompleted.emit(this, buffer);\n> \t}\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 EFCF8BF415\n\tfor ;\n\tFri, 19 Nov 2021 21:14:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 45A9C60378;\n\tFri, 19 Nov 2021 22:14:06 +0100 (CET)","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 35909600B5\n\tfor ;\n\tFri, 19 Nov 2021 22:14:05 +0100 (CET)","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 9BA0F1C19;\n\tFri, 19 Nov 2021 22:14:04 +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=\"p4rSlocJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637356444;\n\tbh=pu64+Gu2CJzKwZ5f5Dk2+mZiSZV3AWpnV0aom2sAvcw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=p4rSlocJCNv/ojkgJPneFsbbqE8yVcDCtDPlsIK9zxItalJaBhzbH3Tnua4A0L5Xm\n\tjdeECPcKSTv8fTxDsMNipHfXTS3TxvXlGp4ddvF2DMBOPQDu/qp0pM4BsEFFqwLhCF\n\t12h2+KB9czI1I51kilUaXvXc8nL2lSzK3q+7aPbI=","Date":"Fri, 19 Nov 2021 23:13:41 +0200","From":"Laurent Pinchart ","To":"Dorota Czaplejewicz ","Message-ID":"","References":"<20211119150421.974848-1-dorota.czaplejewicz@puri.sm>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211119150421.974848-1-dorota.czaplejewicz@puri.sm>","Subject":"Re: [libcamera-devel] [PATCH] FrameBuffer: Make FrameBuffer::cancel\n\tprivate","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":"Librem5-team ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21067,"web_url":"https://patchwork.libcamera.org/comment/21067/","msgid":"<20211119233343.66b9ff94.dorota.czaplejewicz@puri.sm>","date":"2021-11-19T22:33:43","subject":"Re: [libcamera-devel] [PATCH] FrameBuffer: Make FrameBuffer::cancel\n\tprivate","submitter":{"id":96,"url":"https://patchwork.libcamera.org/api/people/96/","name":"Dorota Czaplejewicz","email":"dorota.czaplejewicz@puri.sm"},"content":"Hi,\n\nOn Fri, 19 Nov 2021 23:13:41 +0200\nLaurent Pinchart wrote:\n\n> Hi Dorota,\n> \n> Thank you for the patch.\n> \n> The subject line should be\n> \n> libcamera: framebuffer: Make FrameBuffer::cancel() private\n> \n> to match the usual style (running `git log` on the file(s) you change is\n> a good way to see what the style is).\n> \n> On Fri, Nov 19, 2021 at 04:05:58PM +0100, Dorota Czaplejewicz wrote:\n> > FrameBuffer::cancel is not meant to be used by the API consumer. It was moved to FrameBuffer::Private::cancel . \n> \n> Commit messages should be wrapped to 72 columns (git + vim does that by\n> default, I'm not sure about other editors).\n> \n> I'd write\n> \n> FrameBuffer::cancel() is not meant to be used by applications. Move it\n> to the FrameBuffer::Private class.\n> \n> as \"API consumer\" could be understood as either the application, or the\n> internal API users.\n> \n> Your Signed-off-by line is missing (see\n> https://libcamera.org/contributing.html#submitting-patches).\n> \n> > ---\n> > Hi,\n> > \n> > I'm sending this as discussed on the mailing list.\n> > \n> > Regards,\n> > Dorota\n> > \n> > include/libcamera/framebuffer.h | 2 --\n> > include/libcamera/internal/framebuffer.h | 2 ++\n> > src/libcamera/framebuffer.cpp | 16 ++++++++--------\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 5 +++--\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 2 +-\n> > src/libcamera/pipeline/vimc/vimc.cpp | 3 ++-\n> > src/libcamera/request.cpp | 2 +-\n> > 7 files changed, 17 insertions(+), 15 deletions(-)\n> > \n> > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> > index 7f2f176a..367a8a71 100644\n> > --- a/include/libcamera/framebuffer.h\n> > +++ b/include/libcamera/framebuffer.h\n> > @@ -66,8 +66,6 @@ public:\n> > \tunsigned int cookie() const { return cookie_; }\n> > \tvoid setCookie(unsigned int cookie) { cookie_ = cookie; }\n> > \n> > -\tvoid cancel() { metadata_.status = FrameMetadata::FrameCancelled; }\n> > -\n> > private:\n> > \tLIBCAMERA_DISABLE_COPY_AND_MOVE(FrameBuffer)\n> > \n> > diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h\n> > index cd33c295..27676212 100644\n> > --- a/include/libcamera/internal/framebuffer.h\n> > +++ b/include/libcamera/internal/framebuffer.h\n> > @@ -23,6 +23,8 @@ public:\n> > \tvoid setRequest(Request *request) { request_ = request; }\n> > \tbool isContiguous() const { return isContiguous_; }\n> > \n> > +\tvoid cancel() { LIBCAMERA_O_PTR()->metadata_.status = FrameMetadata::FrameCancelled; } \n> \n> We try to keep line within a 80 columns soft limit when it doesn't\n> otherwise hinder readability, so this could be\n> \n> \tvoid cancel()\n> \t{\n> \t\tFrameBuffer *const o = LIBCAMERA_O_PTR();\n> \t\to->metadata_.status = FrameMetadata::FrameCancelled;\n> \t}\n> \n> > +\n> > private:\n> > \tRequest *request_;\n> > \tbool isContiguous_;\n> > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> > index 337ea115..dd344ed8 100644\n> > --- a/src/libcamera/framebuffer.cpp\n> > +++ b/src/libcamera/framebuffer.cpp\n> > @@ -137,6 +137,14 @@ FrameBuffer::Private::Private()\n> > * \\return True if the planes are stored contiguously in memory, false otherwise\n> > */\n> > \n> > +/**\n> > + * \\fn FrameBuffer::Private::cancel()\n> > + * \\brief Marks the buffer as cancelled\n> > + *\n> > + * If a buffer is not used by a request, it shall be marked as cancelled to\n> > + * indicate that the metadata is invalid. \n> \n> We've discussed the fact that the documentation isn't very explicit, but\n> that should be addressed in a separate patch, not here.\n> \nWhy should it go in a separate patch? I only moved the doc because I got a warning from doxygen.\n\n--Dorota\n\n> The patch looks good overall. With these small issues fixed,\n> \n> Reviewed-by: Laurent Pinchart \n> \n> You can include that line in your v2.\n> \n> > + */\n> > +\n> > /**\n> > * \\class FrameBuffer\n> > * \\brief Frame buffer data and its associated dynamic metadata\n> > @@ -305,12 +313,4 @@ Request *FrameBuffer::request() const\n> > * libcamera core never modifies the buffer cookie.\n> > */\n> > \n> > -/**\n> > - * \\fn FrameBuffer::cancel()\n> > - * \\brief Marks the buffer as cancelled\n> > - *\n> > - * If a buffer is not used by a request, it shall be marked as cancelled to\n> > - * indicate that the metadata is invalid.\n> > - */\n> > -\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index c65afdb2..f8516cba 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -27,6 +27,7 @@\n> > #include \"libcamera/internal/camera_sensor.h\"\n> > #include \"libcamera/internal/delayed_controls.h\"\n> > #include \"libcamera/internal/device_enumerator.h\"\n> > +#include \"libcamera/internal/framebuffer.h\"\n> > #include \"libcamera/internal/ipa_manager.h\"\n> > #include \"libcamera/internal/media_device.h\"\n> > #include \"libcamera/internal/pipeline_handler.h\"\n> > @@ -816,7 +817,7 @@ void IPU3CameraData::cancelPendingRequests()\n> > \n> > \t\tfor (auto it : request->buffers()) {\n> > \t\t\tFrameBuffer *buffer = it.second;\n> > -\t\t\tbuffer->cancel();\n> > +\t\t\tbuffer->_d()->cancel();\n> > \t\t\tpipe()->completeBuffer(request, buffer);\n> > \t\t}\n> > \n> > @@ -1333,7 +1334,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n> > \tif (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> > \t\tfor (auto it : request->buffers()) {\n> > \t\t\tFrameBuffer *b = it.second;\n> > -\t\t\tb->cancel();\n> > +\t\t\tb->_d()->cancel();\n> > \t\t\tpipe()->completeBuffer(request, b);\n> > \t\t}\n> > \n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 5e1f2273..b7cabf5e 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -1573,7 +1573,7 @@ void RPiCameraData::clearIncompleteRequests()\n> > \t\t\t * request? If not, do so now.\n> > \t\t\t */\n> > \t\t\tif (buffer->request()) {\n> > -\t\t\t\tbuffer->cancel();\n> > +\t\t\t\tbuffer->_d()->cancel();\n> > \t\t\t\tpipe()->completeBuffer(request, buffer);\n> > \t\t\t}\n> > \t\t}\n> > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> > index e453091d..1ec814d1 100644\n> > --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> > +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> > @@ -32,6 +32,7 @@\n> > #include \"libcamera/internal/camera.h\"\n> > #include \"libcamera/internal/camera_sensor.h\"\n> > #include \"libcamera/internal/device_enumerator.h\"\n> > +#include \"libcamera/internal/framebuffer.h\"\n> > #include \"libcamera/internal/ipa_manager.h\"\n> > #include \"libcamera/internal/media_device.h\"\n> > #include \"libcamera/internal/pipeline_handler.h\"\n> > @@ -574,7 +575,7 @@ void VimcCameraData::bufferReady(FrameBuffer *buffer)\n> > \tif (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> > \t\tfor (auto it : request->buffers()) {\n> > \t\t\tFrameBuffer *b = it.second;\n> > -\t\t\tb->cancel();\n> > +\t\t\tb->_d()->cancel();\n> > \t\t\tpipe->completeBuffer(request, b);\n> > \t\t}\n> > \n> > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > index 17fefab7..a4dbf750 100644\n> > --- a/src/libcamera/request.cpp\n> > +++ b/src/libcamera/request.cpp\n> > @@ -304,7 +304,7 @@ void Request::cancel()\n> > \tASSERT(status_ == RequestPending);\n> > \n> > \tfor (FrameBuffer *buffer : pending_) {\n> > -\t\tbuffer->cancel();\n> > +\t\tbuffer->_d()->cancel();\n> > \t\tcamera_->bufferCompleted.emit(this, buffer);\n> > \t}\n> > \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 C17BBBDB13\n\tfor ;\n\tFri, 19 Nov 2021 22:34:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E40DD60371;\n\tFri, 19 Nov 2021 23:34:31 +0100 (CET)","from comms.puri.sm (comms.puri.sm [159.203.221.185])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5E7FC600B5\n\tfor ;\n\tFri, 19 Nov 2021 23:34:30 +0100 (CET)","from localhost (localhost [127.0.0.1])\n\tby comms.puri.sm (Postfix) with ESMTP id BC2D3E01ED;\n\tFri, 19 Nov 2021 14:33:58 -0800 (PST)","from comms.puri.sm ([127.0.0.1])\n\tby localhost (comms.puri.sm [127.0.0.1]) (amavisd-new, port 10024)\n\twith ESMTP id vwSskrRjDuGM; Fri, 19 Nov 2021 14:33:57 -0800 (PST)"],"Date":"Fri, 19 Nov 2021 23:33:43 +0100","From":"Dorota Czaplejewicz ","To":"Laurent Pinchart ","Message-ID":"<20211119233343.66b9ff94.dorota.czaplejewicz@puri.sm>","In-Reply-To":"","References":"<20211119150421.974848-1-dorota.czaplejewicz@puri.sm>\n\t","Organization":"Purism","MIME-Version":"1.0","Content-Type":"multipart/signed; boundary=\"Sig_/Ag0jCggAHzlcyihwtlM4u1/\";\n\tprotocol=\"application/pgp-signature\"; micalg=pgp-sha256","Subject":"Re: [libcamera-devel] [PATCH] FrameBuffer: Make FrameBuffer::cancel\n\tprivate","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":"Librem5-team ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21069,"web_url":"https://patchwork.libcamera.org/comment/21069/","msgid":"","date":"2021-11-19T22:37:30","subject":"Re: [libcamera-devel] [PATCH] FrameBuffer: Make FrameBuffer::cancel\n\tprivate","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Dorota,\n\nOn Fri, Nov 19, 2021 at 11:33:43PM +0100, Dorota Czaplejewicz wrote:\n> On Fri, 19 Nov 2021 23:13:41 +0200 Laurent Pinchart wrote:\n> \n> > Hi Dorota,\n> > \n> > Thank you for the patch.\n> > \n> > The subject line should be\n> > \n> > libcamera: framebuffer: Make FrameBuffer::cancel() private\n> > \n> > to match the usual style (running `git log` on the file(s) you change is\n> > a good way to see what the style is).\n> > \n> > On Fri, Nov 19, 2021 at 04:05:58PM +0100, Dorota Czaplejewicz wrote:\n> > > FrameBuffer::cancel is not meant to be used by the API consumer. It was moved to FrameBuffer::Private::cancel . \n> > \n> > Commit messages should be wrapped to 72 columns (git + vim does that by\n> > default, I'm not sure about other editors).\n> > \n> > I'd write\n> > \n> > FrameBuffer::cancel() is not meant to be used by applications. Move it\n> > to the FrameBuffer::Private class.\n> > \n> > as \"API consumer\" could be understood as either the application, or the\n> > internal API users.\n> > \n> > Your Signed-off-by line is missing (see\n> > https://libcamera.org/contributing.html#submitting-patches).\n> > \n> > > ---\n> > > Hi,\n> > > \n> > > I'm sending this as discussed on the mailing list.\n> > > \n> > > Regards,\n> > > Dorota\n> > > \n> > > include/libcamera/framebuffer.h | 2 --\n> > > include/libcamera/internal/framebuffer.h | 2 ++\n> > > src/libcamera/framebuffer.cpp | 16 ++++++++--------\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 5 +++--\n> > > .../pipeline/raspberrypi/raspberrypi.cpp | 2 +-\n> > > src/libcamera/pipeline/vimc/vimc.cpp | 3 ++-\n> > > src/libcamera/request.cpp | 2 +-\n> > > 7 files changed, 17 insertions(+), 15 deletions(-)\n> > > \n> > > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> > > index 7f2f176a..367a8a71 100644\n> > > --- a/include/libcamera/framebuffer.h\n> > > +++ b/include/libcamera/framebuffer.h\n> > > @@ -66,8 +66,6 @@ public:\n> > > \tunsigned int cookie() const { return cookie_; }\n> > > \tvoid setCookie(unsigned int cookie) { cookie_ = cookie; }\n> > > \n> > > -\tvoid cancel() { metadata_.status = FrameMetadata::FrameCancelled; }\n> > > -\n> > > private:\n> > > \tLIBCAMERA_DISABLE_COPY_AND_MOVE(FrameBuffer)\n> > > \n> > > diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h\n> > > index cd33c295..27676212 100644\n> > > --- a/include/libcamera/internal/framebuffer.h\n> > > +++ b/include/libcamera/internal/framebuffer.h\n> > > @@ -23,6 +23,8 @@ public:\n> > > \tvoid setRequest(Request *request) { request_ = request; }\n> > > \tbool isContiguous() const { return isContiguous_; }\n> > > \n> > > +\tvoid cancel() { LIBCAMERA_O_PTR()->metadata_.status = FrameMetadata::FrameCancelled; } \n> > \n> > We try to keep line within a 80 columns soft limit when it doesn't\n> > otherwise hinder readability, so this could be\n> > \n> > \tvoid cancel()\n> > \t{\n> > \t\tFrameBuffer *const o = LIBCAMERA_O_PTR();\n> > \t\to->metadata_.status = FrameMetadata::FrameCancelled;\n> > \t}\n> > \n> > > +\n> > > private:\n> > > \tRequest *request_;\n> > > \tbool isContiguous_;\n> > > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> > > index 337ea115..dd344ed8 100644\n> > > --- a/src/libcamera/framebuffer.cpp\n> > > +++ b/src/libcamera/framebuffer.cpp\n> > > @@ -137,6 +137,14 @@ FrameBuffer::Private::Private()\n> > > * \\return True if the planes are stored contiguously in memory, false otherwise\n> > > */\n> > > \n> > > +/**\n> > > + * \\fn FrameBuffer::Private::cancel()\n> > > + * \\brief Marks the buffer as cancelled\n> > > + *\n> > > + * If a buffer is not used by a request, it shall be marked as cancelled to\n> > > + * indicate that the metadata is invalid. \n> > \n> > We've discussed the fact that the documentation isn't very explicit, but\n> > that should be addressed in a separate patch, not here.\n> \n> Why should it go in a separate patch? I only moved the doc because I\n> got a warning from doxygen.\n\nI meant that we should improve the documentation on top. This patch only\nmoves it, which is right. There's nothing to change.\n\n> > The patch looks good overall. With these small issues fixed,\n> > \n> > Reviewed-by: Laurent Pinchart \n> > \n> > You can include that line in your v2.\n> > \n> > > + */\n> > > +\n> > > /**\n> > > * \\class FrameBuffer\n> > > * \\brief Frame buffer data and its associated dynamic metadata\n> > > @@ -305,12 +313,4 @@ Request *FrameBuffer::request() const\n> > > * libcamera core never modifies the buffer cookie.\n> > > */\n> > > \n> > > -/**\n> > > - * \\fn FrameBuffer::cancel()\n> > > - * \\brief Marks the buffer as cancelled\n> > > - *\n> > > - * If a buffer is not used by a request, it shall be marked as cancelled to\n> > > - * indicate that the metadata is invalid.\n> > > - */\n> > > -\n> > > } /* namespace libcamera */\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index c65afdb2..f8516cba 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -27,6 +27,7 @@\n> > > #include \"libcamera/internal/camera_sensor.h\"\n> > > #include \"libcamera/internal/delayed_controls.h\"\n> > > #include \"libcamera/internal/device_enumerator.h\"\n> > > +#include \"libcamera/internal/framebuffer.h\"\n> > > #include \"libcamera/internal/ipa_manager.h\"\n> > > #include \"libcamera/internal/media_device.h\"\n> > > #include \"libcamera/internal/pipeline_handler.h\"\n> > > @@ -816,7 +817,7 @@ void IPU3CameraData::cancelPendingRequests()\n> > > \n> > > \t\tfor (auto it : request->buffers()) {\n> > > \t\t\tFrameBuffer *buffer = it.second;\n> > > -\t\t\tbuffer->cancel();\n> > > +\t\t\tbuffer->_d()->cancel();\n> > > \t\t\tpipe()->completeBuffer(request, buffer);\n> > > \t\t}\n> > > \n> > > @@ -1333,7 +1334,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n> > > \tif (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> > > \t\tfor (auto it : request->buffers()) {\n> > > \t\t\tFrameBuffer *b = it.second;\n> > > -\t\t\tb->cancel();\n> > > +\t\t\tb->_d()->cancel();\n> > > \t\t\tpipe()->completeBuffer(request, b);\n> > > \t\t}\n> > > \n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index 5e1f2273..b7cabf5e 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -1573,7 +1573,7 @@ void RPiCameraData::clearIncompleteRequests()\n> > > \t\t\t * request? If not, do so now.\n> > > \t\t\t */\n> > > \t\t\tif (buffer->request()) {\n> > > -\t\t\t\tbuffer->cancel();\n> > > +\t\t\t\tbuffer->_d()->cancel();\n> > > \t\t\t\tpipe()->completeBuffer(request, buffer);\n> > > \t\t\t}\n> > > \t\t}\n> > > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> > > index e453091d..1ec814d1 100644\n> > > --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> > > +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> > > @@ -32,6 +32,7 @@\n> > > #include \"libcamera/internal/camera.h\"\n> > > #include \"libcamera/internal/camera_sensor.h\"\n> > > #include \"libcamera/internal/device_enumerator.h\"\n> > > +#include \"libcamera/internal/framebuffer.h\"\n> > > #include \"libcamera/internal/ipa_manager.h\"\n> > > #include \"libcamera/internal/media_device.h\"\n> > > #include \"libcamera/internal/pipeline_handler.h\"\n> > > @@ -574,7 +575,7 @@ void VimcCameraData::bufferReady(FrameBuffer *buffer)\n> > > \tif (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> > > \t\tfor (auto it : request->buffers()) {\n> > > \t\t\tFrameBuffer *b = it.second;\n> > > -\t\t\tb->cancel();\n> > > +\t\t\tb->_d()->cancel();\n> > > \t\t\tpipe->completeBuffer(request, b);\n> > > \t\t}\n> > > \n> > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > > index 17fefab7..a4dbf750 100644\n> > > --- a/src/libcamera/request.cpp\n> > > +++ b/src/libcamera/request.cpp\n> > > @@ -304,7 +304,7 @@ void Request::cancel()\n> > > \tASSERT(status_ == RequestPending);\n> > > \n> > > \tfor (FrameBuffer *buffer : pending_) {\n> > > -\t\tbuffer->cancel();\n> > > +\t\tbuffer->_d()->cancel();\n> > > \t\tcamera_->bufferCompleted.emit(this, buffer);\n> > > \t}\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 A35EEBDB13\n\tfor ;\n\tFri, 19 Nov 2021 22:37:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5C66860378;\n\tFri, 19 Nov 2021 23:37: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 EE3B7600B5\n\tfor ;\n\tFri, 19 Nov 2021 23:37:53 +0100 (CET)","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 9019A1C19;\n\tFri, 19 Nov 2021 23:37: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=\"LAwIDz6n\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637361473;\n\tbh=XaYmmkUdivCGaMrCRnj9RFwpcUzF6UEr74xXWJ9CDWs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LAwIDz6n/nGruU911BGeU6js42I2FEi88J0LhhBdsHjZle8bgMtvGwCs4/UIA8epK\n\trElktAHL9eTOADQsctMJ5rPYjUiV0liOcBXtUMr8M97joPC5gJQv2hgdn/dynubu8e\n\th/1XCHgmCT3PXgh7agcrrEd1VQIjopsOafRSUhTc=","Date":"Sat, 20 Nov 2021 00:37:30 +0200","From":"Laurent Pinchart ","To":"Dorota Czaplejewicz ","Message-ID":"","References":"<20211119150421.974848-1-dorota.czaplejewicz@puri.sm>\n\t\n\t<20211119233343.66b9ff94.dorota.czaplejewicz@puri.sm>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211119233343.66b9ff94.dorota.czaplejewicz@puri.sm>","Subject":"Re: [libcamera-devel] [PATCH] FrameBuffer: Make FrameBuffer::cancel\n\tprivate","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":"Librem5-team ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21096,"web_url":"https://patchwork.libcamera.org/comment/21096/","msgid":"<163757966785.1089182.1511028075107359458@Monstersaurus>","date":"2021-11-22T11:14:27","subject":"Re: [libcamera-devel] [PATCH] FrameBuffer: Make FrameBuffer::cancel\n\tprivate","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Dorota,\n\nQuoting Laurent Pinchart (2021-11-19 22:37:30)\n> Hi Dorota,\n> \n> On Fri, Nov 19, 2021 at 11:33:43PM +0100, Dorota Czaplejewicz wrote:\n> > On Fri, 19 Nov 2021 23:13:41 +0200 Laurent Pinchart wrote:\n> > \n> > > Hi Dorota,\n> > > \n> > > Thank you for the patch.\n> > > \n> > > The subject line should be\n> > > \n> > > libcamera: framebuffer: Make FrameBuffer::cancel() private\n> > > \n> > > to match the usual style (running `git log` on the file(s) you change is\n> > > a good way to see what the style is).\n> > > \n> > > On Fri, Nov 19, 2021 at 04:05:58PM +0100, Dorota Czaplejewicz wrote:\n> > > > FrameBuffer::cancel is not meant to be used by the API consumer. It was moved to FrameBuffer::Private::cancel . \n> > > \n> > > Commit messages should be wrapped to 72 columns (git + vim does that by\n> > > default, I'm not sure about other editors).\n> > > \n> > > I'd write\n> > > \n> > > FrameBuffer::cancel() is not meant to be used by applications. Move it\n> > > to the FrameBuffer::Private class.\n> > > \n> > > as \"API consumer\" could be understood as either the application, or the\n> > > internal API users.\n> > > \n> > > Your Signed-off-by line is missing (see\n> > > https://libcamera.org/contributing.html#submitting-patches).\n> > > \n> > > > ---\n> > > > Hi,\n> > > > \n> > > > I'm sending this as discussed on the mailing list.\n> > > > \n> > > > Regards,\n> > > > Dorota\n> > > > \n> > > > include/libcamera/framebuffer.h | 2 --\n> > > > include/libcamera/internal/framebuffer.h | 2 ++\n> > > > src/libcamera/framebuffer.cpp | 16 ++++++++--------\n> > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 5 +++--\n> > > > .../pipeline/raspberrypi/raspberrypi.cpp | 2 +-\n> > > > src/libcamera/pipeline/vimc/vimc.cpp | 3 ++-\n> > > > src/libcamera/request.cpp | 2 +-\n> > > > 7 files changed, 17 insertions(+), 15 deletions(-)\n> > > > \n> > > > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> > > > index 7f2f176a..367a8a71 100644\n> > > > --- a/include/libcamera/framebuffer.h\n> > > > +++ b/include/libcamera/framebuffer.h\n> > > > @@ -66,8 +66,6 @@ public:\n> > > > unsigned int cookie() const { return cookie_; }\n> > > > void setCookie(unsigned int cookie) { cookie_ = cookie; }\n> > > > \n> > > > - void cancel() { metadata_.status = FrameMetadata::FrameCancelled; }\n> > > > -\n> > > > private:\n> > > > LIBCAMERA_DISABLE_COPY_AND_MOVE(FrameBuffer)\n> > > > \n> > > > diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h\n> > > > index cd33c295..27676212 100644\n> > > > --- a/include/libcamera/internal/framebuffer.h\n> > > > +++ b/include/libcamera/internal/framebuffer.h\n> > > > @@ -23,6 +23,8 @@ public:\n> > > > void setRequest(Request *request) { request_ = request; }\n> > > > bool isContiguous() const { return isContiguous_; }\n> > > > \n> > > > + void cancel() { LIBCAMERA_O_PTR()->metadata_.status = FrameMetadata::FrameCancelled; } \n> > > \n> > > We try to keep line within a 80 columns soft limit when it doesn't\n> > > otherwise hinder readability, so this could be\n> > > \n> > > void cancel()\n> > > {\n> > > FrameBuffer *const o = LIBCAMERA_O_PTR();\n> > > o->metadata_.status = FrameMetadata::FrameCancelled;\n> > > }\n> > > \n> > > > +\n> > > > private:\n> > > > Request *request_;\n> > > > bool isContiguous_;\n> > > > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> > > > index 337ea115..dd344ed8 100644\n> > > > --- a/src/libcamera/framebuffer.cpp\n> > > > +++ b/src/libcamera/framebuffer.cpp\n> > > > @@ -137,6 +137,14 @@ FrameBuffer::Private::Private()\n> > > > * \\return True if the planes are stored contiguously in memory, false otherwise\n> > > > */\n> > > > \n> > > > +/**\n> > > > + * \\fn FrameBuffer::Private::cancel()\n> > > > + * \\brief Marks the buffer as cancelled\n> > > > + *\n> > > > + * If a buffer is not used by a request, it shall be marked as cancelled to\n> > > > + * indicate that the metadata is invalid. \n> > > \n> > > We've discussed the fact that the documentation isn't very explicit, but\n> > > that should be addressed in a separate patch, not here.\n> > \n> > Why should it go in a separate patch? I only moved the doc because I\n> > got a warning from doxygen.\n> \n> I meant that we should improve the documentation on top. This patch only\n> moves it, which is right. There's nothing to change.\n> \n> > > The patch looks good overall. With these small issues fixed,\n> > > \n> > > Reviewed-by: Laurent Pinchart \n> > > \n> > > You can include that line in your v2.\n\nAs previously discussed moving cancel() certainly makes it\nclear that it can not be used by external public interfaces.\n\nWith the small changes highlighted by Laurent:\n\nReviewed-by: Kieran Bingham \n\n\n> > > \n> > > > + */\n> > > > +\n> > > > /**\n> > > > * \\class FrameBuffer\n> > > > * \\brief Frame buffer data and its associated dynamic metadata\n> > > > @@ -305,12 +313,4 @@ Request *FrameBuffer::request() const\n> > > > * libcamera core never modifies the buffer cookie.\n> > > > */\n> > > > \n> > > > -/**\n> > > > - * \\fn FrameBuffer::cancel()\n> > > > - * \\brief Marks the buffer as cancelled\n> > > > - *\n> > > > - * If a buffer is not used by a request, it shall be marked as cancelled to\n> > > > - * indicate that the metadata is invalid.\n> > > > - */\n> > > > -\n> > > > } /* namespace libcamera */\n> > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > index c65afdb2..f8516cba 100644\n> > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > @@ -27,6 +27,7 @@\n> > > > #include \"libcamera/internal/camera_sensor.h\"\n> > > > #include \"libcamera/internal/delayed_controls.h\"\n> > > > #include \"libcamera/internal/device_enumerator.h\"\n> > > > +#include \"libcamera/internal/framebuffer.h\"\n> > > > #include \"libcamera/internal/ipa_manager.h\"\n> > > > #include \"libcamera/internal/media_device.h\"\n> > > > #include \"libcamera/internal/pipeline_handler.h\"\n> > > > @@ -816,7 +817,7 @@ void IPU3CameraData::cancelPendingRequests()\n> > > > \n> > > > for (auto it : request->buffers()) {\n> > > > FrameBuffer *buffer = it.second;\n> > > > - buffer->cancel();\n> > > > + buffer->_d()->cancel();\n> > > > pipe()->completeBuffer(request, buffer);\n> > > > }\n> > > > \n> > > > @@ -1333,7 +1334,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n> > > > if (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> > > > for (auto it : request->buffers()) {\n> > > > FrameBuffer *b = it.second;\n> > > > - b->cancel();\n> > > > + b->_d()->cancel();\n> > > > pipe()->completeBuffer(request, b);\n> > > > }\n> > > > \n> > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > index 5e1f2273..b7cabf5e 100644\n> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > @@ -1573,7 +1573,7 @@ void RPiCameraData::clearIncompleteRequests()\n> > > > * request? If not, do so now.\n> > > > */\n> > > > if (buffer->request()) {\n> > > > - buffer->cancel();\n> > > > + buffer->_d()->cancel();\n> > > > pipe()->completeBuffer(request, buffer);\n> > > > }\n> > > > }\n> > > > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> > > > index e453091d..1ec814d1 100644\n> > > > --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> > > > +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> > > > @@ -32,6 +32,7 @@\n> > > > #include \"libcamera/internal/camera.h\"\n> > > > #include \"libcamera/internal/camera_sensor.h\"\n> > > > #include \"libcamera/internal/device_enumerator.h\"\n> > > > +#include \"libcamera/internal/framebuffer.h\"\n> > > > #include \"libcamera/internal/ipa_manager.h\"\n> > > > #include \"libcamera/internal/media_device.h\"\n> > > > #include \"libcamera/internal/pipeline_handler.h\"\n> > > > @@ -574,7 +575,7 @@ void VimcCameraData::bufferReady(FrameBuffer *buffer)\n> > > > if (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> > > > for (auto it : request->buffers()) {\n> > > > FrameBuffer *b = it.second;\n> > > > - b->cancel();\n> > > > + b->_d()->cancel();\n> > > > pipe->completeBuffer(request, b);\n> > > > }\n> > > > \n> > > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > > > index 17fefab7..a4dbf750 100644\n> > > > --- a/src/libcamera/request.cpp\n> > > > +++ b/src/libcamera/request.cpp\n> > > > @@ -304,7 +304,7 @@ void Request::cancel()\n> > > > ASSERT(status_ == RequestPending);\n> > > > \n> > > > for (FrameBuffer *buffer : pending_) {\n> > > > - buffer->cancel();\n> > > > + buffer->_d()->cancel();\n> > > > camera_->bufferCompleted.emit(this, buffer);\n> > > > }\n> > > > \n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 9F304BF415\n\tfor ;\n\tMon, 22 Nov 2021 11:14:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E717A6036F;\n\tMon, 22 Nov 2021 12:14:31 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 94D1460230\n\tfor ;\n\tMon, 22 Nov 2021 12:14:30 +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 2B35B14C3;\n\tMon, 22 Nov 2021 12:14:30 +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=\"QW/BRsQq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637579670;\n\tbh=Vb1ysjAS78sERTqzLXmaSzfqMaEnuoNxh8rVabptoj4=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=QW/BRsQqPgD+T6vRG9bAwxDK138ACJ2QO0H5moZI8XQuAgychOHHSx/+rZJ3oK7Ug\n\ta9+/52QY3laPGjzCaNzram+5tnVEhmW+LPD7fAlKyLM/zJXrF8a/foxilna2cwqlCK\n\tLUKMCp0rgTJaKIPv8GB4OeYo3fpfBEdD9wmrGveM=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20211119150421.974848-1-dorota.czaplejewicz@puri.sm>\n\t\n\t<20211119233343.66b9ff94.dorota.czaplejewicz@puri.sm>\n\t","From":"Kieran Bingham ","To":"Dorota Czaplejewicz ,\n\tLaurent Pinchart ","Date":"Mon, 22 Nov 2021 11:14:27 +0000","Message-ID":"<163757966785.1089182.1511028075107359458@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH] FrameBuffer: Make FrameBuffer::cancel\n\tprivate","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":"Librem5-team ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21870,"web_url":"https://patchwork.libcamera.org/comment/21870/","msgid":"<164017472044.3986460.262907047419586856@Monstersaurus>","date":"2021-12-22T12:05:20","subject":"Re: [libcamera-devel] [PATCH] FrameBuffer: Make FrameBuffer::cancel\n\tprivate","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Dorota,\n\nQuoting Kieran Bingham (2021-11-22 11:14:27)\n> Hi Dorota,\n> \n> Quoting Laurent Pinchart (2021-11-19 22:37:30)\n> > Hi Dorota,\n> > \n> > On Fri, Nov 19, 2021 at 11:33:43PM +0100, Dorota Czaplejewicz wrote:\n> > > On Fri, 19 Nov 2021 23:13:41 +0200 Laurent Pinchart wrote:\n> > > \n> > > > Hi Dorota,\n> > > > \n> > > > Thank you for the patch.\n> > > > \n> > > > The subject line should be\n> > > > \n> > > > libcamera: framebuffer: Make FrameBuffer::cancel() private\n> > > > \n> > > > to match the usual style (running `git log` on the file(s) you change is\n> > > > a good way to see what the style is).\n> > > > \n> > > > On Fri, Nov 19, 2021 at 04:05:58PM +0100, Dorota Czaplejewicz wrote:\n> > > > > FrameBuffer::cancel is not meant to be used by the API consumer. It was moved to FrameBuffer::Private::cancel . \n> > > > \n> > > > Commit messages should be wrapped to 72 columns (git + vim does that by\n> > > > default, I'm not sure about other editors).\n> > > > \n> > > > I'd write\n> > > > \n> > > > FrameBuffer::cancel() is not meant to be used by applications. Move it\n> > > > to the FrameBuffer::Private class.\n> > > > \n> > > > as \"API consumer\" could be understood as either the application, or the\n> > > > internal API users.\n> > > > \n> > > > Your Signed-off-by line is missing (see\n> > > > https://libcamera.org/contributing.html#submitting-patches).\n> > > > \n> > > > > ---\n> > > > > Hi,\n> > > > > \n> > > > > I'm sending this as discussed on the mailing list.\n> > > > > \n> > > > > Regards,\n> > > > > Dorota\n> > > > > \n> > > > > include/libcamera/framebuffer.h | 2 --\n> > > > > include/libcamera/internal/framebuffer.h | 2 ++\n> > > > > src/libcamera/framebuffer.cpp | 16 ++++++++--------\n> > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 5 +++--\n> > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 2 +-\n> > > > > src/libcamera/pipeline/vimc/vimc.cpp | 3 ++-\n> > > > > src/libcamera/request.cpp | 2 +-\n> > > > > 7 files changed, 17 insertions(+), 15 deletions(-)\n> > > > > \n> > > > > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> > > > > index 7f2f176a..367a8a71 100644\n> > > > > --- a/include/libcamera/framebuffer.h\n> > > > > +++ b/include/libcamera/framebuffer.h\n> > > > > @@ -66,8 +66,6 @@ public:\n> > > > > unsigned int cookie() const { return cookie_; }\n> > > > > void setCookie(unsigned int cookie) { cookie_ = cookie; }\n> > > > > \n> > > > > - void cancel() { metadata_.status = FrameMetadata::FrameCancelled; }\n> > > > > -\n> > > > > private:\n> > > > > LIBCAMERA_DISABLE_COPY_AND_MOVE(FrameBuffer)\n> > > > > \n> > > > > diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h\n> > > > > index cd33c295..27676212 100644\n> > > > > --- a/include/libcamera/internal/framebuffer.h\n> > > > > +++ b/include/libcamera/internal/framebuffer.h\n> > > > > @@ -23,6 +23,8 @@ public:\n> > > > > void setRequest(Request *request) { request_ = request; }\n> > > > > bool isContiguous() const { return isContiguous_; }\n> > > > > \n> > > > > + void cancel() { LIBCAMERA_O_PTR()->metadata_.status = FrameMetadata::FrameCancelled; } \n> > > > \n> > > > We try to keep line within a 80 columns soft limit when it doesn't\n> > > > otherwise hinder readability, so this could be\n> > > > \n> > > > void cancel()\n> > > > {\n> > > > FrameBuffer *const o = LIBCAMERA_O_PTR();\n> > > > o->metadata_.status = FrameMetadata::FrameCancelled;\n> > > > }\n> > > > \n> > > > > +\n> > > > > private:\n> > > > > Request *request_;\n> > > > > bool isContiguous_;\n> > > > > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> > > > > index 337ea115..dd344ed8 100644\n> > > > > --- a/src/libcamera/framebuffer.cpp\n> > > > > +++ b/src/libcamera/framebuffer.cpp\n> > > > > @@ -137,6 +137,14 @@ FrameBuffer::Private::Private()\n> > > > > * \\return True if the planes are stored contiguously in memory, false otherwise\n> > > > > */\n> > > > > \n> > > > > +/**\n> > > > > + * \\fn FrameBuffer::Private::cancel()\n> > > > > + * \\brief Marks the buffer as cancelled\n> > > > > + *\n> > > > > + * If a buffer is not used by a request, it shall be marked as cancelled to\n> > > > > + * indicate that the metadata is invalid. \n> > > > \n> > > > We've discussed the fact that the documentation isn't very explicit, but\n> > > > that should be addressed in a separate patch, not here.\n> > > \n> > > Why should it go in a separate patch? I only moved the doc because I\n> > > got a warning from doxygen.\n> > \n> > I meant that we should improve the documentation on top. This patch only\n> > moves it, which is right. There's nothing to change.\n> > \n> > > > The patch looks good overall. With these small issues fixed,\n> > > > \n> > > > Reviewed-by: Laurent Pinchart \n> > > > \n> > > > You can include that line in your v2.\n> \n> As previously discussed moving cancel() certainly makes it\n> clear that it can not be used by external public interfaces.\n> \n> With the small changes highlighted by Laurent:\n> \n> Reviewed-by: Kieran Bingham \n\nDo you plan to send a revised version of this patch, or would you like\nme to handle it for you?\n\n--\nRegards\n\nKieran\n\n> \n> \n> > > > \n> > > > > + */\n> > > > > +\n> > > > > /**\n> > > > > * \\class FrameBuffer\n> > > > > * \\brief Frame buffer data and its associated dynamic metadata\n> > > > > @@ -305,12 +313,4 @@ Request *FrameBuffer::request() const\n> > > > > * libcamera core never modifies the buffer cookie.\n> > > > > */\n> > > > > \n> > > > > -/**\n> > > > > - * \\fn FrameBuffer::cancel()\n> > > > > - * \\brief Marks the buffer as cancelled\n> > > > > - *\n> > > > > - * If a buffer is not used by a request, it shall be marked as cancelled to\n> > > > > - * indicate that the metadata is invalid.\n> > > > > - */\n> > > > > -\n> > > > > } /* namespace libcamera */\n> > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > index c65afdb2..f8516cba 100644\n> > > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > @@ -27,6 +27,7 @@\n> > > > > #include \"libcamera/internal/camera_sensor.h\"\n> > > > > #include \"libcamera/internal/delayed_controls.h\"\n> > > > > #include \"libcamera/internal/device_enumerator.h\"\n> > > > > +#include \"libcamera/internal/framebuffer.h\"\n> > > > > #include \"libcamera/internal/ipa_manager.h\"\n> > > > > #include \"libcamera/internal/media_device.h\"\n> > > > > #include \"libcamera/internal/pipeline_handler.h\"\n> > > > > @@ -816,7 +817,7 @@ void IPU3CameraData::cancelPendingRequests()\n> > > > > \n> > > > > for (auto it : request->buffers()) {\n> > > > > FrameBuffer *buffer = it.second;\n> > > > > - buffer->cancel();\n> > > > > + buffer->_d()->cancel();\n> > > > > pipe()->completeBuffer(request, buffer);\n> > > > > }\n> > > > > \n> > > > > @@ -1333,7 +1334,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n> > > > > if (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> > > > > for (auto it : request->buffers()) {\n> > > > > FrameBuffer *b = it.second;\n> > > > > - b->cancel();\n> > > > > + b->_d()->cancel();\n> > > > > pipe()->completeBuffer(request, b);\n> > > > > }\n> > > > > \n> > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > index 5e1f2273..b7cabf5e 100644\n> > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > @@ -1573,7 +1573,7 @@ void RPiCameraData::clearIncompleteRequests()\n> > > > > * request? If not, do so now.\n> > > > > */\n> > > > > if (buffer->request()) {\n> > > > > - buffer->cancel();\n> > > > > + buffer->_d()->cancel();\n> > > > > pipe()->completeBuffer(request, buffer);\n> > > > > }\n> > > > > }\n> > > > > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> > > > > index e453091d..1ec814d1 100644\n> > > > > --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> > > > > +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> > > > > @@ -32,6 +32,7 @@\n> > > > > #include \"libcamera/internal/camera.h\"\n> > > > > #include \"libcamera/internal/camera_sensor.h\"\n> > > > > #include \"libcamera/internal/device_enumerator.h\"\n> > > > > +#include \"libcamera/internal/framebuffer.h\"\n> > > > > #include \"libcamera/internal/ipa_manager.h\"\n> > > > > #include \"libcamera/internal/media_device.h\"\n> > > > > #include \"libcamera/internal/pipeline_handler.h\"\n> > > > > @@ -574,7 +575,7 @@ void VimcCameraData::bufferReady(FrameBuffer *buffer)\n> > > > > if (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> > > > > for (auto it : request->buffers()) {\n> > > > > FrameBuffer *b = it.second;\n> > > > > - b->cancel();\n> > > > > + b->_d()->cancel();\n> > > > > pipe->completeBuffer(request, b);\n> > > > > }\n> > > > > \n> > > > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > > > > index 17fefab7..a4dbf750 100644\n> > > > > --- a/src/libcamera/request.cpp\n> > > > > +++ b/src/libcamera/request.cpp\n> > > > > @@ -304,7 +304,7 @@ void Request::cancel()\n> > > > > ASSERT(status_ == RequestPending);\n> > > > > \n> > > > > for (FrameBuffer *buffer : pending_) {\n> > > > > - buffer->cancel();\n> > > > > + buffer->_d()->cancel();\n> > > > > camera_->bufferCompleted.emit(this, buffer);\n> > > > > }\n> > > > > \n> > \n> > -- \n> > Regards,\n> > \n> > Laurent Pinchart","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 C70EDBE080\n\tfor ;\n\tWed, 22 Dec 2021 12:05:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DC031608E7;\n\tWed, 22 Dec 2021 13:05:24 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E6D7D60115\n\tfor ;\n\tWed, 22 Dec 2021 13:05:22 +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 80D85894;\n\tWed, 22 Dec 2021 13:05:22 +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=\"W+jTE+xJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1640174722;\n\tbh=b536QzqUp4DtA5i9XM4so3x2kO5Itk6Gw54OM6wVrwg=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=W+jTE+xJOplNq6KjW/piFIomigR6VGo+u/7iey0uIIDBabbaES+VIaFSslUAFBSWI\n\ts5UyYtCBbFcfrwldT7zeQvhB7Uj7x2yXfXPm1VJKAVWj0mh0kJtcUlst8FTRAF1gNf\n\tEEYCrIdPS4ZuA17/eVxu/STyfminW3iUhbr9krqQ=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<163757966785.1089182.1511028075107359458@Monstersaurus>","References":"<20211119150421.974848-1-dorota.czaplejewicz@puri.sm>\n\t\n\t<20211119233343.66b9ff94.dorota.czaplejewicz@puri.sm>\n\t\n\t<163757966785.1089182.1511028075107359458@Monstersaurus>","From":"Kieran Bingham ","To":"Dorota Czaplejewicz ,\n\tLaurent Pinchart ","Date":"Wed, 22 Dec 2021 12:05:20 +0000","Message-ID":"<164017472044.3986460.262907047419586856@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH] FrameBuffer: Make FrameBuffer::cancel\n\tprivate","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":"Librem5-team ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21872,"web_url":"https://patchwork.libcamera.org/comment/21872/","msgid":"<20211222131629.6c889224.dorota.czaplejewicz@puri.sm>","date":"2021-12-22T12:16:29","subject":"Re: [libcamera-devel] [PATCH] FrameBuffer: Make FrameBuffer::cancel\n\tprivate","submitter":{"id":96,"url":"https://patchwork.libcamera.org/api/people/96/","name":"Dorota Czaplejewicz","email":"dorota.czaplejewicz@puri.sm"},"content":"On Wed, 22 Dec 2021 12:05:20 +0000\nKieran Bingham wrote:\n\n> Hi Dorota,\n> \n> Quoting Kieran Bingham (2021-11-22 11:14:27)\n> > Hi Dorota,\n> > \n> > Quoting Laurent Pinchart (2021-11-19 22:37:30) \n> > > Hi Dorota,\n> > > \n> > > On Fri, Nov 19, 2021 at 11:33:43PM +0100, Dorota Czaplejewicz wrote: \n> > > > On Fri, 19 Nov 2021 23:13:41 +0200 Laurent Pinchart wrote:\n> > > > \n> > > > > Hi Dorota,\n> > > > > \n> > > > > Thank you for the patch.\n> > > > > \n> > > > > The subject line should be\n> > > > > \n> > > > > libcamera: framebuffer: Make FrameBuffer::cancel() private\n> > > > > \n> > > > > to match the usual style (running `git log` on the file(s) you change is\n> > > > > a good way to see what the style is).\n> > > > > \n> > > > > On Fri, Nov 19, 2021 at 04:05:58PM +0100, Dorota Czaplejewicz wrote: \n> > > > > > FrameBuffer::cancel is not meant to be used by the API consumer. It was moved to FrameBuffer::Private::cancel . \n> > > > > \n> > > > > Commit messages should be wrapped to 72 columns (git + vim does that by\n> > > > > default, I'm not sure about other editors).\n> > > > > \n> > > > > I'd write\n> > > > > \n> > > > > FrameBuffer::cancel() is not meant to be used by applications. Move it\n> > > > > to the FrameBuffer::Private class.\n> > > > > \n> > > > > as \"API consumer\" could be understood as either the application, or the\n> > > > > internal API users.\n> > > > > \n> > > > > Your Signed-off-by line is missing (see\n> > > > > https://libcamera.org/contributing.html#submitting-patches).\n> > > > > \n> > > > > > ---\n> > > > > > Hi,\n> > > > > > \n> > > > > > I'm sending this as discussed on the mailing list.\n> > > > > > \n> > > > > > Regards,\n> > > > > > Dorota\n> > > > > > \n> > > > > > include/libcamera/framebuffer.h | 2 --\n> > > > > > include/libcamera/internal/framebuffer.h | 2 ++\n> > > > > > src/libcamera/framebuffer.cpp | 16 ++++++++--------\n> > > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 5 +++--\n> > > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 2 +-\n> > > > > > src/libcamera/pipeline/vimc/vimc.cpp | 3 ++-\n> > > > > > src/libcamera/request.cpp | 2 +-\n> > > > > > 7 files changed, 17 insertions(+), 15 deletions(-)\n> > > > > > \n> > > > > > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> > > > > > index 7f2f176a..367a8a71 100644\n> > > > > > --- a/include/libcamera/framebuffer.h\n> > > > > > +++ b/include/libcamera/framebuffer.h\n> > > > > > @@ -66,8 +66,6 @@ public:\n> > > > > > unsigned int cookie() const { return cookie_; }\n> > > > > > void setCookie(unsigned int cookie) { cookie_ = cookie; }\n> > > > > > \n> > > > > > - void cancel() { metadata_.status = FrameMetadata::FrameCancelled; }\n> > > > > > -\n> > > > > > private:\n> > > > > > LIBCAMERA_DISABLE_COPY_AND_MOVE(FrameBuffer)\n> > > > > > \n> > > > > > diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h\n> > > > > > index cd33c295..27676212 100644\n> > > > > > --- a/include/libcamera/internal/framebuffer.h\n> > > > > > +++ b/include/libcamera/internal/framebuffer.h\n> > > > > > @@ -23,6 +23,8 @@ public:\n> > > > > > void setRequest(Request *request) { request_ = request; }\n> > > > > > bool isContiguous() const { return isContiguous_; }\n> > > > > > \n> > > > > > + void cancel() { LIBCAMERA_O_PTR()->metadata_.status = FrameMetadata::FrameCancelled; } \n> > > > > \n> > > > > We try to keep line within a 80 columns soft limit when it doesn't\n> > > > > otherwise hinder readability, so this could be\n> > > > > \n> > > > > void cancel()\n> > > > > {\n> > > > > FrameBuffer *const o = LIBCAMERA_O_PTR();\n> > > > > o->metadata_.status = FrameMetadata::FrameCancelled;\n> > > > > }\n> > > > > \n> > > > > > +\n> > > > > > private:\n> > > > > > Request *request_;\n> > > > > > bool isContiguous_;\n> > > > > > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> > > > > > index 337ea115..dd344ed8 100644\n> > > > > > --- a/src/libcamera/framebuffer.cpp\n> > > > > > +++ b/src/libcamera/framebuffer.cpp\n> > > > > > @@ -137,6 +137,14 @@ FrameBuffer::Private::Private()\n> > > > > > * \\return True if the planes are stored contiguously in memory, false otherwise\n> > > > > > */\n> > > > > > \n> > > > > > +/**\n> > > > > > + * \\fn FrameBuffer::Private::cancel()\n> > > > > > + * \\brief Marks the buffer as cancelled\n> > > > > > + *\n> > > > > > + * If a buffer is not used by a request, it shall be marked as cancelled to\n> > > > > > + * indicate that the metadata is invalid. \n> > > > > \n> > > > > We've discussed the fact that the documentation isn't very explicit, but\n> > > > > that should be addressed in a separate patch, not here. \n> > > > \n> > > > Why should it go in a separate patch? I only moved the doc because I\n> > > > got a warning from doxygen. \n> > > \n> > > I meant that we should improve the documentation on top. This patch only\n> > > moves it, which is right. There's nothing to change.\n> > > \n> > > > > The patch looks good overall. With these small issues fixed,\n> > > > > \n> > > > > Reviewed-by: Laurent Pinchart \n> > > > > \n> > > > > You can include that line in your v2. \n> > \n> > As previously discussed moving cancel() certainly makes it\n> > clear that it can not be used by external public interfaces.\n> > \n> > With the small changes highlighted by Laurent:\n> > \n> > Reviewed-by: Kieran Bingham \n> \n> Do you plan to send a revised version of this patch, or would you like\n> me to handle it for you?\n> \nOh, sorry, this slipped my attention.\n\nI don't mind if you handle it before I get to it.\n\nCheers,\nDorota\n\n> --\n> Regards\n> \n> Kieran\n> \n> > \n> > \n> > > > > \n> > > > > > + */\n> > > > > > +\n> > > > > > /**\n> > > > > > * \\class FrameBuffer\n> > > > > > * \\brief Frame buffer data and its associated dynamic metadata\n> > > > > > @@ -305,12 +313,4 @@ Request *FrameBuffer::request() const\n> > > > > > * libcamera core never modifies the buffer cookie.\n> > > > > > */\n> > > > > > \n> > > > > > -/**\n> > > > > > - * \\fn FrameBuffer::cancel()\n> > > > > > - * \\brief Marks the buffer as cancelled\n> > > > > > - *\n> > > > > > - * If a buffer is not used by a request, it shall be marked as cancelled to\n> > > > > > - * indicate that the metadata is invalid.\n> > > > > > - */\n> > > > > > -\n> > > > > > } /* namespace libcamera */\n> > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > > index c65afdb2..f8516cba 100644\n> > > > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > > @@ -27,6 +27,7 @@\n> > > > > > #include \"libcamera/internal/camera_sensor.h\"\n> > > > > > #include \"libcamera/internal/delayed_controls.h\"\n> > > > > > #include \"libcamera/internal/device_enumerator.h\"\n> > > > > > +#include \"libcamera/internal/framebuffer.h\"\n> > > > > > #include \"libcamera/internal/ipa_manager.h\"\n> > > > > > #include \"libcamera/internal/media_device.h\"\n> > > > > > #include \"libcamera/internal/pipeline_handler.h\"\n> > > > > > @@ -816,7 +817,7 @@ void IPU3CameraData::cancelPendingRequests()\n> > > > > > \n> > > > > > for (auto it : request->buffers()) {\n> > > > > > FrameBuffer *buffer = it.second;\n> > > > > > - buffer->cancel();\n> > > > > > + buffer->_d()->cancel();\n> > > > > > pipe()->completeBuffer(request, buffer);\n> > > > > > }\n> > > > > > \n> > > > > > @@ -1333,7 +1334,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n> > > > > > if (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> > > > > > for (auto it : request->buffers()) {\n> > > > > > FrameBuffer *b = it.second;\n> > > > > > - b->cancel();\n> > > > > > + b->_d()->cancel();\n> > > > > > pipe()->completeBuffer(request, b);\n> > > > > > }\n> > > > > > \n> > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > index 5e1f2273..b7cabf5e 100644\n> > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > @@ -1573,7 +1573,7 @@ void RPiCameraData::clearIncompleteRequests()\n> > > > > > * request? If not, do so now.\n> > > > > > */\n> > > > > > if (buffer->request()) {\n> > > > > > - buffer->cancel();\n> > > > > > + buffer->_d()->cancel();\n> > > > > > pipe()->completeBuffer(request, buffer);\n> > > > > > }\n> > > > > > }\n> > > > > > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> > > > > > index e453091d..1ec814d1 100644\n> > > > > > --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> > > > > > +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> > > > > > @@ -32,6 +32,7 @@\n> > > > > > #include \"libcamera/internal/camera.h\"\n> > > > > > #include \"libcamera/internal/camera_sensor.h\"\n> > > > > > #include \"libcamera/internal/device_enumerator.h\"\n> > > > > > +#include \"libcamera/internal/framebuffer.h\"\n> > > > > > #include \"libcamera/internal/ipa_manager.h\"\n> > > > > > #include \"libcamera/internal/media_device.h\"\n> > > > > > #include \"libcamera/internal/pipeline_handler.h\"\n> > > > > > @@ -574,7 +575,7 @@ void VimcCameraData::bufferReady(FrameBuffer *buffer)\n> > > > > > if (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> > > > > > for (auto it : request->buffers()) {\n> > > > > > FrameBuffer *b = it.second;\n> > > > > > - b->cancel();\n> > > > > > + b->_d()->cancel();\n> > > > > > pipe->completeBuffer(request, b);\n> > > > > > }\n> > > > > > \n> > > > > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > > > > > index 17fefab7..a4dbf750 100644\n> > > > > > --- a/src/libcamera/request.cpp\n> > > > > > +++ b/src/libcamera/request.cpp\n> > > > > > @@ -304,7 +304,7 @@ void Request::cancel()\n> > > > > > ASSERT(status_ == RequestPending);\n> > > > > > \n> > > > > > for (FrameBuffer *buffer : pending_) {\n> > > > > > - buffer->cancel();\n> > > > > > + buffer->_d()->cancel();\n> > > > > > camera_->bufferCompleted.emit(this, buffer);\n> > > > > > }\n> > > > > > \n> > > \n> > > -- \n> > > Regards,\n> > > \n> > > Laurent Pinchart","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 BCF86BE080\n\tfor ;\n\tWed, 22 Dec 2021 12:16:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0E343608F9;\n\tWed, 22 Dec 2021 13:16:46 +0100 (CET)","from comms.puri.sm (comms.puri.sm [159.203.221.185])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ED45760115\n\tfor ;\n\tWed, 22 Dec 2021 13:16:44 +0100 (CET)","from localhost (localhost [127.0.0.1])\n\tby comms.puri.sm (Postfix) with ESMTP id 6530DE1224;\n\tWed, 22 Dec 2021 04:16:43 -0800 (PST)","from comms.puri.sm ([127.0.0.1])\n\tby localhost (comms.puri.sm [127.0.0.1]) (amavisd-new, port 10024)\n\twith ESMTP id IoV7lZrV-WzB; Wed, 22 Dec 2021 04:16:42 -0800 (PST)"],"Date":"Wed, 22 Dec 2021 13:16:29 +0100","From":"Dorota Czaplejewicz ","To":"Kieran Bingham ","Message-ID":"<20211222131629.6c889224.dorota.czaplejewicz@puri.sm>","In-Reply-To":"<164017472044.3986460.262907047419586856@Monstersaurus>","References":"<20211119150421.974848-1-dorota.czaplejewicz@puri.sm>\n\t\n\t<20211119233343.66b9ff94.dorota.czaplejewicz@puri.sm>\n\t\n\t<163757966785.1089182.1511028075107359458@Monstersaurus>\n\t<164017472044.3986460.262907047419586856@Monstersaurus>","Organization":"Purism","MIME-Version":"1.0","Content-Type":"multipart/signed; boundary=\"Sig_/pYrE0/.9vlLItelRlcPD/ee\";\n\tprotocol=\"application/pgp-signature\"; micalg=pgp-sha256","Subject":"Re: [libcamera-devel] [PATCH] FrameBuffer: Make FrameBuffer::cancel\n\tprivate","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":"Librem5-team ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22693,"web_url":"https://patchwork.libcamera.org/comment/22693/","msgid":"<164984145155.22830.5042825075821688379@Monstersaurus>","date":"2022-04-13T09:17:31","subject":"Re: [libcamera-devel] [PATCH] FrameBuffer: Make FrameBuffer::cancel\n\tprivate","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Dorota,\n\nQuoting Dorota Czaplejewicz (2021-12-22 12:16:29)\n> On Wed, 22 Dec 2021 12:05:20 +0000\n> Kieran Bingham wrote:\n> \n> > Hi Dorota,\n> > \n> > Quoting Kieran Bingham (2021-11-22 11:14:27)\n> > > Hi Dorota,\n> > > \n> > > Quoting Laurent Pinchart (2021-11-19 22:37:30) \n> > > > Hi Dorota,\n> > > > \n> > > > On Fri, Nov 19, 2021 at 11:33:43PM +0100, Dorota Czaplejewicz wrote: \n> > > > > On Fri, 19 Nov 2021 23:13:41 +0200 Laurent Pinchart wrote:\n> > > > > \n> > > > > > Hi Dorota,\n> > > > > > \n> > > > > > Thank you for the patch.\n> > > > > > \n> > > > > > The subject line should be\n> > > > > > \n> > > > > > libcamera: framebuffer: Make FrameBuffer::cancel() private\n> > > > > > \n> > > > > > to match the usual style (running `git log` on the file(s) you change is\n> > > > > > a good way to see what the style is).\n> > > > > > \n> > > > > > On Fri, Nov 19, 2021 at 04:05:58PM +0100, Dorota Czaplejewicz wrote: \n> > > > > > > FrameBuffer::cancel is not meant to be used by the API consumer. It was moved to FrameBuffer::Private::cancel . \n> > > > > > \n> > > > > > Commit messages should be wrapped to 72 columns (git + vim does that by\n> > > > > > default, I'm not sure about other editors).\n> > > > > > \n> > > > > > I'd write\n> > > > > > \n> > > > > > FrameBuffer::cancel() is not meant to be used by applications. Move it\n> > > > > > to the FrameBuffer::Private class.\n> > > > > > \n> > > > > > as \"API consumer\" could be understood as either the application, or the\n> > > > > > internal API users.\n> > > > > > \n> > > > > > Your Signed-off-by line is missing (see\n> > > > > > https://libcamera.org/contributing.html#submitting-patches).\n\nI was hoping to clean this up and merge it, but to retain your\nauthorship I need to add your Signed-off by tag. Could you reply with it\nto this email to signify adding it to the patch please?\n\n--\nKieran\n\n\n> > > > > > \n> > > > > > > ---\n> > > > > > > Hi,\n> > > > > > > \n> > > > > > > I'm sending this as discussed on the mailing list.\n> > > > > > > \n> > > > > > > Regards,\n> > > > > > > Dorota\n> > > > > > > \n> > > > > > > include/libcamera/framebuffer.h | 2 --\n> > > > > > > include/libcamera/internal/framebuffer.h | 2 ++\n> > > > > > > src/libcamera/framebuffer.cpp | 16 ++++++++--------\n> > > > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 5 +++--\n> > > > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 2 +-\n> > > > > > > src/libcamera/pipeline/vimc/vimc.cpp | 3 ++-\n> > > > > > > src/libcamera/request.cpp | 2 +-\n> > > > > > > 7 files changed, 17 insertions(+), 15 deletions(-)\n> > > > > > > \n> > > > > > > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> > > > > > > index 7f2f176a..367a8a71 100644\n> > > > > > > --- a/include/libcamera/framebuffer.h\n> > > > > > > +++ b/include/libcamera/framebuffer.h\n> > > > > > > @@ -66,8 +66,6 @@ public:\n> > > > > > > unsigned int cookie() const { return cookie_; }\n> > > > > > > void setCookie(unsigned int cookie) { cookie_ = cookie; }\n> > > > > > > \n> > > > > > > - void cancel() { metadata_.status = FrameMetadata::FrameCancelled; }\n> > > > > > > -\n> > > > > > > private:\n> > > > > > > LIBCAMERA_DISABLE_COPY_AND_MOVE(FrameBuffer)\n> > > > > > > \n> > > > > > > diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h\n> > > > > > > index cd33c295..27676212 100644\n> > > > > > > --- a/include/libcamera/internal/framebuffer.h\n> > > > > > > +++ b/include/libcamera/internal/framebuffer.h\n> > > > > > > @@ -23,6 +23,8 @@ public:\n> > > > > > > void setRequest(Request *request) { request_ = request; }\n> > > > > > > bool isContiguous() const { return isContiguous_; }\n> > > > > > > \n> > > > > > > + void cancel() { LIBCAMERA_O_PTR()->metadata_.status = FrameMetadata::FrameCancelled; } \n> > > > > > \n> > > > > > We try to keep line within a 80 columns soft limit when it doesn't\n> > > > > > otherwise hinder readability, so this could be\n> > > > > > \n> > > > > > void cancel()\n> > > > > > {\n> > > > > > FrameBuffer *const o = LIBCAMERA_O_PTR();\n> > > > > > o->metadata_.status = FrameMetadata::FrameCancelled;\n> > > > > > }\n> > > > > > \n> > > > > > > +\n> > > > > > > private:\n> > > > > > > Request *request_;\n> > > > > > > bool isContiguous_;\n> > > > > > > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> > > > > > > index 337ea115..dd344ed8 100644\n> > > > > > > --- a/src/libcamera/framebuffer.cpp\n> > > > > > > +++ b/src/libcamera/framebuffer.cpp\n> > > > > > > @@ -137,6 +137,14 @@ FrameBuffer::Private::Private()\n> > > > > > > * \\return True if the planes are stored contiguously in memory, false otherwise\n> > > > > > > */\n> > > > > > > \n> > > > > > > +/**\n> > > > > > > + * \\fn FrameBuffer::Private::cancel()\n> > > > > > > + * \\brief Marks the buffer as cancelled\n> > > > > > > + *\n> > > > > > > + * If a buffer is not used by a request, it shall be marked as cancelled to\n> > > > > > > + * indicate that the metadata is invalid. \n> > > > > > \n> > > > > > We've discussed the fact that the documentation isn't very explicit, but\n> > > > > > that should be addressed in a separate patch, not here. \n> > > > > \n> > > > > Why should it go in a separate patch? I only moved the doc because I\n> > > > > got a warning from doxygen. \n> > > > \n> > > > I meant that we should improve the documentation on top. This patch only\n> > > > moves it, which is right. There's nothing to change.\n> > > > \n> > > > > > The patch looks good overall. With these small issues fixed,\n> > > > > > \n> > > > > > Reviewed-by: Laurent Pinchart \n> > > > > > \n> > > > > > You can include that line in your v2. \n> > > \n> > > As previously discussed moving cancel() certainly makes it\n> > > clear that it can not be used by external public interfaces.\n> > > \n> > > With the small changes highlighted by Laurent:\n> > > \n> > > Reviewed-by: Kieran Bingham \n> > \n> > Do you plan to send a revised version of this patch, or would you like\n> > me to handle it for you?\n> > \n> Oh, sorry, this slipped my attention.\n> \n> I don't mind if you handle it before I get to it.\n> \n> Cheers,\n> Dorota\n> \n> > --\n> > Regards\n> > \n> > Kieran\n> > \n> > > \n> > > \n> > > > > > \n> > > > > > > + */\n> > > > > > > +\n> > > > > > > /**\n> > > > > > > * \\class FrameBuffer\n> > > > > > > * \\brief Frame buffer data and its associated dynamic metadata\n> > > > > > > @@ -305,12 +313,4 @@ Request *FrameBuffer::request() const\n> > > > > > > * libcamera core never modifies the buffer cookie.\n> > > > > > > */\n> > > > > > > \n> > > > > > > -/**\n> > > > > > > - * \\fn FrameBuffer::cancel()\n> > > > > > > - * \\brief Marks the buffer as cancelled\n> > > > > > > - *\n> > > > > > > - * If a buffer is not used by a request, it shall be marked as cancelled to\n> > > > > > > - * indicate that the metadata is invalid.\n> > > > > > > - */\n> > > > > > > -\n> > > > > > > } /* namespace libcamera */\n> > > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > > > index c65afdb2..f8516cba 100644\n> > > > > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > > > @@ -27,6 +27,7 @@\n> > > > > > > #include \"libcamera/internal/camera_sensor.h\"\n> > > > > > > #include \"libcamera/internal/delayed_controls.h\"\n> > > > > > > #include \"libcamera/internal/device_enumerator.h\"\n> > > > > > > +#include \"libcamera/internal/framebuffer.h\"\n> > > > > > > #include \"libcamera/internal/ipa_manager.h\"\n> > > > > > > #include \"libcamera/internal/media_device.h\"\n> > > > > > > #include \"libcamera/internal/pipeline_handler.h\"\n> > > > > > > @@ -816,7 +817,7 @@ void IPU3CameraData::cancelPendingRequests()\n> > > > > > > \n> > > > > > > for (auto it : request->buffers()) {\n> > > > > > > FrameBuffer *buffer = it.second;\n> > > > > > > - buffer->cancel();\n> > > > > > > + buffer->_d()->cancel();\n> > > > > > > pipe()->completeBuffer(request, buffer);\n> > > > > > > }\n> > > > > > > \n> > > > > > > @@ -1333,7 +1334,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n> > > > > > > if (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> > > > > > > for (auto it : request->buffers()) {\n> > > > > > > FrameBuffer *b = it.second;\n> > > > > > > - b->cancel();\n> > > > > > > + b->_d()->cancel();\n> > > > > > > pipe()->completeBuffer(request, b);\n> > > > > > > }\n> > > > > > > \n> > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > > index 5e1f2273..b7cabf5e 100644\n> > > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > > @@ -1573,7 +1573,7 @@ void RPiCameraData::clearIncompleteRequests()\n> > > > > > > * request? If not, do so now.\n> > > > > > > */\n> > > > > > > if (buffer->request()) {\n> > > > > > > - buffer->cancel();\n> > > > > > > + buffer->_d()->cancel();\n> > > > > > > pipe()->completeBuffer(request, buffer);\n> > > > > > > }\n> > > > > > > }\n> > > > > > > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> > > > > > > index e453091d..1ec814d1 100644\n> > > > > > > --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> > > > > > > +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> > > > > > > @@ -32,6 +32,7 @@\n> > > > > > > #include \"libcamera/internal/camera.h\"\n> > > > > > > #include \"libcamera/internal/camera_sensor.h\"\n> > > > > > > #include \"libcamera/internal/device_enumerator.h\"\n> > > > > > > +#include \"libcamera/internal/framebuffer.h\"\n> > > > > > > #include \"libcamera/internal/ipa_manager.h\"\n> > > > > > > #include \"libcamera/internal/media_device.h\"\n> > > > > > > #include \"libcamera/internal/pipeline_handler.h\"\n> > > > > > > @@ -574,7 +575,7 @@ void VimcCameraData::bufferReady(FrameBuffer *buffer)\n> > > > > > > if (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> > > > > > > for (auto it : request->buffers()) {\n> > > > > > > FrameBuffer *b = it.second;\n> > > > > > > - b->cancel();\n> > > > > > > + b->_d()->cancel();\n> > > > > > > pipe->completeBuffer(request, b);\n> > > > > > > }\n> > > > > > > \n> > > > > > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > > > > > > index 17fefab7..a4dbf750 100644\n> > > > > > > --- a/src/libcamera/request.cpp\n> > > > > > > +++ b/src/libcamera/request.cpp\n> > > > > > > @@ -304,7 +304,7 @@ void Request::cancel()\n> > > > > > > ASSERT(status_ == RequestPending);\n> > > > > > > \n> > > > > > > for (FrameBuffer *buffer : pending_) {\n> > > > > > > - buffer->cancel();\n> > > > > > > + buffer->_d()->cancel();\n> > > > > > > camera_->bufferCompleted.emit(this, buffer);\n> > > > > > > }\n> > > > > > > \n> > > > \n> > > > -- \n> > > > Regards,\n> > > > \n> > > > Laurent Pinchart \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 C4624C3256\n\tfor ;\n\tWed, 13 Apr 2022 09:17:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 213FB65647;\n\tWed, 13 Apr 2022 11:17:35 +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 2E19C6563E\n\tfor ;\n\tWed, 13 Apr 2022 11:17:34 +0200 (CEST)","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 B3BEB25B;\n\tWed, 13 Apr 2022 11:17:33 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649841455;\n\tbh=ftV81xRxvAFCTAWOcd1gL3KlGdTwOvOt/jMEfSTvT78=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=oLqsj5ToUmJi+CD0+nz+1rgZnCEqoEB32WiLqn7cjaPvswZ/fIl7Owqc97CtLCHoU\n\ts49G8l1wsQEcO9rBuMhd8dVTZH8E7eNMTGxAebnCSvSG/1LUCTET9stamUTC8OEbnw\n\tGUnuf27AwPppiT45WLpkcIRH7ootFJ8De+4X/J4Fo7yR76gWhjerr8iptHrfEPcEdf\n\ttL/FT4Ay/tvPr1TfvDlTPJrz69aQm7oNhWnjY6u/z/6PLcmu90mxhL80UEEfLBV08v\n\tgB39dUowtcR4zeOnH3YEBo2ooQ2F3TnfnbtRBJ9ep/9BEM/DUrUfdRefRc55YYexZq\n\tbm2Q9pojzSA1g==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1649841453;\n\tbh=ftV81xRxvAFCTAWOcd1gL3KlGdTwOvOt/jMEfSTvT78=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=pDNf1sKBYWWI2ZW8LEUEAhmmmWkiu0fW3tKMjK6yjtrltf4nSoDBeZPG0IIYzooZL\n\tqGfMWxOqTJJ1XoI3JFk8ztSSCzBoJKZLB9Uk9nsJmQREq1yoaY2w7kBO2ucIpcXNjJ\n\t0QUT1xxgYYuHyUlAGnNmUTlAaivu6wgbQ2x5OTKQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"pDNf1sKB\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211222131629.6c889224.dorota.czaplejewicz@puri.sm>","References":"<20211119150421.974848-1-dorota.czaplejewicz@puri.sm>\n\t\n\t<20211119233343.66b9ff94.dorota.czaplejewicz@puri.sm>\n\t\n\t<163757966785.1089182.1511028075107359458@Monstersaurus>\n\t<164017472044.3986460.262907047419586856@Monstersaurus>\n\t<20211222131629.6c889224.dorota.czaplejewicz@puri.sm>","To":"Dorota Czaplejewicz ","Date":"Wed, 13 Apr 2022 10:17:31 +0100","Message-ID":"<164984145155.22830.5042825075821688379@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH] FrameBuffer: Make FrameBuffer::cancel\n\tprivate","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"Librem5-team ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22702,"web_url":"https://patchwork.libcamera.org/comment/22702/","msgid":"<20220413142357.5b101129.dorota.czaplejewicz@puri.sm>","date":"2022-04-13T12:23:57","subject":"Re: [libcamera-devel] [PATCH] FrameBuffer: Make FrameBuffer::cancel\n\tprivate","submitter":{"id":96,"url":"https://patchwork.libcamera.org/api/people/96/","name":"Dorota Czaplejewicz","email":"dorota.czaplejewicz@puri.sm"},"content":"Hi,\n\nSigned-off-by: Dorota Czaplejewicz \n\nDid I do that right?\n--Dorota\n\nOn Wed, 13 Apr 2022 10:17:31 +0100\nKieran Bingham wrote:\n\n> Hi Dorota,\n> \n> Quoting Dorota Czaplejewicz (2021-12-22 12:16:29)\n> > On Wed, 22 Dec 2021 12:05:20 +0000\n> > Kieran Bingham wrote:\n> > \n> > > Hi Dorota,\n> > > \n> > > Quoting Kieran Bingham (2021-11-22 11:14:27) \n> > > > Hi Dorota,\n> > > > \n> > > > Quoting Laurent Pinchart (2021-11-19 22:37:30) \n> > > > > Hi Dorota,\n> > > > > \n> > > > > On Fri, Nov 19, 2021 at 11:33:43PM +0100, Dorota Czaplejewicz wrote: \n> > > > > > On Fri, 19 Nov 2021 23:13:41 +0200 Laurent Pinchart wrote:\n> > > > > > \n> > > > > > > Hi Dorota,\n> > > > > > > \n> > > > > > > Thank you for the patch.\n> > > > > > > \n> > > > > > > The subject line should be\n> > > > > > > \n> > > > > > > libcamera: framebuffer: Make FrameBuffer::cancel() private\n> > > > > > > \n> > > > > > > to match the usual style (running `git log` on the file(s) you change is\n> > > > > > > a good way to see what the style is).\n> > > > > > > \n> > > > > > > On Fri, Nov 19, 2021 at 04:05:58PM +0100, Dorota Czaplejewicz wrote: \n> > > > > > > > FrameBuffer::cancel is not meant to be used by the API consumer. It was moved to FrameBuffer::Private::cancel . \n> > > > > > > \n> > > > > > > Commit messages should be wrapped to 72 columns (git + vim does that by\n> > > > > > > default, I'm not sure about other editors).\n> > > > > > > \n> > > > > > > I'd write\n> > > > > > > \n> > > > > > > FrameBuffer::cancel() is not meant to be used by applications. Move it\n> > > > > > > to the FrameBuffer::Private class.\n> > > > > > > \n> > > > > > > as \"API consumer\" could be understood as either the application, or the\n> > > > > > > internal API users.\n> > > > > > > \n> > > > > > > Your Signed-off-by line is missing (see\n> > > > > > > https://libcamera.org/contributing.html#submitting-patches). \n> \n> I was hoping to clean this up and merge it, but to retain your\n> authorship I need to add your Signed-off by tag. Could you reply with it\n> to this email to signify adding it to the patch please?\n> \n> --\n> Kieran\n> \n> \n> > > > > > > \n> > > > > > > > ---\n> > > > > > > > Hi,\n> > > > > > > > \n> > > > > > > > I'm sending this as discussed on the mailing list.\n> > > > > > > > \n> > > > > > > > Regards,\n> > > > > > > > Dorota\n> > > > > > > > \n> > > > > > > > include/libcamera/framebuffer.h | 2 --\n> > > > > > > > include/libcamera/internal/framebuffer.h | 2 ++\n> > > > > > > > src/libcamera/framebuffer.cpp | 16 ++++++++--------\n> > > > > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 5 +++--\n> > > > > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 2 +-\n> > > > > > > > src/libcamera/pipeline/vimc/vimc.cpp | 3 ++-\n> > > > > > > > src/libcamera/request.cpp | 2 +-\n> > > > > > > > 7 files changed, 17 insertions(+), 15 deletions(-)\n> > > > > > > > \n> > > > > > > > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> > > > > > > > index 7f2f176a..367a8a71 100644\n> > > > > > > > --- a/include/libcamera/framebuffer.h\n> > > > > > > > +++ b/include/libcamera/framebuffer.h\n> > > > > > > > @@ -66,8 +66,6 @@ public:\n> > > > > > > > unsigned int cookie() const { return cookie_; }\n> > > > > > > > void setCookie(unsigned int cookie) { cookie_ = cookie; }\n> > > > > > > > \n> > > > > > > > - void cancel() { metadata_.status = FrameMetadata::FrameCancelled; }\n> > > > > > > > -\n> > > > > > > > private:\n> > > > > > > > LIBCAMERA_DISABLE_COPY_AND_MOVE(FrameBuffer)\n> > > > > > > > \n> > > > > > > > diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h\n> > > > > > > > index cd33c295..27676212 100644\n> > > > > > > > --- a/include/libcamera/internal/framebuffer.h\n> > > > > > > > +++ b/include/libcamera/internal/framebuffer.h\n> > > > > > > > @@ -23,6 +23,8 @@ public:\n> > > > > > > > void setRequest(Request *request) { request_ = request; }\n> > > > > > > > bool isContiguous() const { return isContiguous_; }\n> > > > > > > > \n> > > > > > > > + void cancel() { LIBCAMERA_O_PTR()->metadata_.status = FrameMetadata::FrameCancelled; } \n> > > > > > > \n> > > > > > > We try to keep line within a 80 columns soft limit when it doesn't\n> > > > > > > otherwise hinder readability, so this could be\n> > > > > > > \n> > > > > > > void cancel()\n> > > > > > > {\n> > > > > > > FrameBuffer *const o = LIBCAMERA_O_PTR();\n> > > > > > > o->metadata_.status = FrameMetadata::FrameCancelled;\n> > > > > > > }\n> > > > > > > \n> > > > > > > > +\n> > > > > > > > private:\n> > > > > > > > Request *request_;\n> > > > > > > > bool isContiguous_;\n> > > > > > > > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> > > > > > > > index 337ea115..dd344ed8 100644\n> > > > > > > > --- a/src/libcamera/framebuffer.cpp\n> > > > > > > > +++ b/src/libcamera/framebuffer.cpp\n> > > > > > > > @@ -137,6 +137,14 @@ FrameBuffer::Private::Private()\n> > > > > > > > * \\return True if the planes are stored contiguously in memory, false otherwise\n> > > > > > > > */\n> > > > > > > > \n> > > > > > > > +/**\n> > > > > > > > + * \\fn FrameBuffer::Private::cancel()\n> > > > > > > > + * \\brief Marks the buffer as cancelled\n> > > > > > > > + *\n> > > > > > > > + * If a buffer is not used by a request, it shall be marked as cancelled to\n> > > > > > > > + * indicate that the metadata is invalid. \n> > > > > > > \n> > > > > > > We've discussed the fact that the documentation isn't very explicit, but\n> > > > > > > that should be addressed in a separate patch, not here. \n> > > > > > \n> > > > > > Why should it go in a separate patch? I only moved the doc because I\n> > > > > > got a warning from doxygen. \n> > > > > \n> > > > > I meant that we should improve the documentation on top. This patch only\n> > > > > moves it, which is right. There's nothing to change.\n> > > > > \n> > > > > > > The patch looks good overall. With these small issues fixed,\n> > > > > > > \n> > > > > > > Reviewed-by: Laurent Pinchart \n> > > > > > > \n> > > > > > > You can include that line in your v2. \n> > > > \n> > > > As previously discussed moving cancel() certainly makes it\n> > > > clear that it can not be used by external public interfaces.\n> > > > \n> > > > With the small changes highlighted by Laurent:\n> > > > \n> > > > Reviewed-by: Kieran Bingham \n> > > \n> > > Do you plan to send a revised version of this patch, or would you like\n> > > me to handle it for you?\n> > > \n> > Oh, sorry, this slipped my attention.\n> > \n> > I don't mind if you handle it before I get to it.\n> > \n> > Cheers,\n> > Dorota\n> > \n> > > --\n> > > Regards\n> > > \n> > > Kieran\n> > > \n> > > > \n> > > > \n> > > > > > > \n> > > > > > > > + */\n> > > > > > > > +\n> > > > > > > > /**\n> > > > > > > > * \\class FrameBuffer\n> > > > > > > > * \\brief Frame buffer data and its associated dynamic metadata\n> > > > > > > > @@ -305,12 +313,4 @@ Request *FrameBuffer::request() const\n> > > > > > > > * libcamera core never modifies the buffer cookie.\n> > > > > > > > */\n> > > > > > > > \n> > > > > > > > -/**\n> > > > > > > > - * \\fn FrameBuffer::cancel()\n> > > > > > > > - * \\brief Marks the buffer as cancelled\n> > > > > > > > - *\n> > > > > > > > - * If a buffer is not used by a request, it shall be marked as cancelled to\n> > > > > > > > - * indicate that the metadata is invalid.\n> > > > > > > > - */\n> > > > > > > > -\n> > > > > > > > } /* namespace libcamera */\n> > > > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > > > > index c65afdb2..f8516cba 100644\n> > > > > > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > > > > @@ -27,6 +27,7 @@\n> > > > > > > > #include \"libcamera/internal/camera_sensor.h\"\n> > > > > > > > #include \"libcamera/internal/delayed_controls.h\"\n> > > > > > > > #include \"libcamera/internal/device_enumerator.h\"\n> > > > > > > > +#include \"libcamera/internal/framebuffer.h\"\n> > > > > > > > #include \"libcamera/internal/ipa_manager.h\"\n> > > > > > > > #include \"libcamera/internal/media_device.h\"\n> > > > > > > > #include \"libcamera/internal/pipeline_handler.h\"\n> > > > > > > > @@ -816,7 +817,7 @@ void IPU3CameraData::cancelPendingRequests()\n> > > > > > > > \n> > > > > > > > for (auto it : request->buffers()) {\n> > > > > > > > FrameBuffer *buffer = it.second;\n> > > > > > > > - buffer->cancel();\n> > > > > > > > + buffer->_d()->cancel();\n> > > > > > > > pipe()->completeBuffer(request, buffer);\n> > > > > > > > }\n> > > > > > > > \n> > > > > > > > @@ -1333,7 +1334,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n> > > > > > > > if (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> > > > > > > > for (auto it : request->buffers()) {\n> > > > > > > > FrameBuffer *b = it.second;\n> > > > > > > > - b->cancel();\n> > > > > > > > + b->_d()->cancel();\n> > > > > > > > pipe()->completeBuffer(request, b);\n> > > > > > > > }\n> > > > > > > > \n> > > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > > > index 5e1f2273..b7cabf5e 100644\n> > > > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > > > @@ -1573,7 +1573,7 @@ void RPiCameraData::clearIncompleteRequests()\n> > > > > > > > * request? If not, do so now.\n> > > > > > > > */\n> > > > > > > > if (buffer->request()) {\n> > > > > > > > - buffer->cancel();\n> > > > > > > > + buffer->_d()->cancel();\n> > > > > > > > pipe()->completeBuffer(request, buffer);\n> > > > > > > > }\n> > > > > > > > }\n> > > > > > > > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> > > > > > > > index e453091d..1ec814d1 100644\n> > > > > > > > --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> > > > > > > > +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> > > > > > > > @@ -32,6 +32,7 @@\n> > > > > > > > #include \"libcamera/internal/camera.h\"\n> > > > > > > > #include \"libcamera/internal/camera_sensor.h\"\n> > > > > > > > #include \"libcamera/internal/device_enumerator.h\"\n> > > > > > > > +#include \"libcamera/internal/framebuffer.h\"\n> > > > > > > > #include \"libcamera/internal/ipa_manager.h\"\n> > > > > > > > #include \"libcamera/internal/media_device.h\"\n> > > > > > > > #include \"libcamera/internal/pipeline_handler.h\"\n> > > > > > > > @@ -574,7 +575,7 @@ void VimcCameraData::bufferReady(FrameBuffer *buffer)\n> > > > > > > > if (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> > > > > > > > for (auto it : request->buffers()) {\n> > > > > > > > FrameBuffer *b = it.second;\n> > > > > > > > - b->cancel();\n> > > > > > > > + b->_d()->cancel();\n> > > > > > > > pipe->completeBuffer(request, b);\n> > > > > > > > }\n> > > > > > > > \n> > > > > > > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > > > > > > > index 17fefab7..a4dbf750 100644\n> > > > > > > > --- a/src/libcamera/request.cpp\n> > > > > > > > +++ b/src/libcamera/request.cpp\n> > > > > > > > @@ -304,7 +304,7 @@ void Request::cancel()\n> > > > > > > > ASSERT(status_ == RequestPending);\n> > > > > > > > \n> > > > > > > > for (FrameBuffer *buffer : pending_) {\n> > > > > > > > - buffer->cancel();\n> > > > > > > > + buffer->_d()->cancel();\n> > > > > > > > camera_->bufferCompleted.emit(this, buffer);\n> > > > > > > > }\n> > > > > > > > \n> > > > > \n> > > > > -- \n> > > > > Regards,\n> > > > > \n> > > > > Laurent Pinchart \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 0B13BC3256\n\tfor ;\n\tWed, 13 Apr 2022 12:24:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0C76B65643;\n\tWed, 13 Apr 2022 14:24:37 +0200 (CEST)","from comms.puri.sm (comms.puri.sm [159.203.221.185])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AF04D604B4\n\tfor ;\n\tWed, 13 Apr 2022 14:24:34 +0200 (CEST)","from localhost (localhost [127.0.0.1])\n\tby comms.puri.sm (Postfix) with ESMTP id DCE6CDFDE3;\n\tWed, 13 Apr 2022 05:24:02 -0700 (PDT)","from comms.puri.sm ([127.0.0.1])\n\tby localhost (comms.puri.sm [127.0.0.1]) (amavisd-new, port 10024)\n\twith ESMTP id 9yBhYLHPBUkq; Wed, 13 Apr 2022 05:24:01 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649852677;\n\tbh=oSIHFj7wgOAeStfJb/L+dv5iE4KTv4zkMmtS8CisT0c=;\n\th=Date:To:In-Reply-To:References:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=GftBUuu1YhjyzdFOYYUenk2vM6Nbxu2R0uIX6O+gnC8g2dDdAPZZCNaOOmFVD3AXW\n\tja8cFJyY/DvkSJXwLYzdA1U+AQ4orPW09PdnfRCHL43LZi0IEemHE+z0lh0RzPCfLS\n\tFr07EYmEp0FtKG9Gsk+kYGK8bOOtFS6lTjrwnDgGwelcP1LvxBtSM37speSrWUGlac\n\tD1352+gmB8RBEbzWSyK6a0jqlszM6xj0kxgoUqekUZlVkgpGP/hN48dxVnZyyF52ul\n\tcazfJgYIQUzuNHcwUecknW8MfXk9tJRDsytbbvlXlevEgH5tmBIDF5G18u/fP1KMao\n\t+bqcgq2th8u0A==","v=1; a=rsa-sha256; c=relaxed/simple; d=puri.sm; s=comms;\n\tt=1649852641; bh=oSIHFj7wgOAeStfJb/L+dv5iE4KTv4zkMmtS8CisT0c=;\n\th=Date:From:To:Cc:Subject:In-Reply-To:References:From;\n\tb=ZNK2CIIQ+DKo8zWsu6cWXQgQLJw0wAyD+RBe09hwNHFzzkBIyd2cbT+SSknU+pbgk\n\t4BAEtD0aAuX8mrhmPtquMvX4cd5kR1qwpfpCDehyYq6SVivsAKlZl3p4zQsdy4F8AN\n\t5h2XfXcxbmwAkaJs4mWqR3b0Mc6ldGkHbd288ugFRsc/DpssDHJJ6hRROro17XHUDl\n\tM2X+aGQxja9wMlTiqXFlw0DwxvNSt+XqaT91rRVbAqlmSYkRzH9zAVaDq3StowT2FP\n\tz7oYJ6aYRoktCRm5qBuwmbRmizjx1oJXGk+RVHnmarjxpgjeoJlRlhNqPconj8tvMR\n\t9Pgy3qDQ3gKRQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=puri.sm header.i=@puri.sm\n\theader.b=\"ZNK2CIIQ\"; dkim-atps=neutral","Date":"Wed, 13 Apr 2022 14:23:57 +0200","To":"Kieran Bingham ","Message-ID":"<20220413142357.5b101129.dorota.czaplejewicz@puri.sm>","In-Reply-To":"<164984145155.22830.5042825075821688379@Monstersaurus>","References":"<20211119150421.974848-1-dorota.czaplejewicz@puri.sm>\n\t\n\t<20211119233343.66b9ff94.dorota.czaplejewicz@puri.sm>\n\t\n\t<163757966785.1089182.1511028075107359458@Monstersaurus>\n\t<164017472044.3986460.262907047419586856@Monstersaurus>\n\t<20211222131629.6c889224.dorota.czaplejewicz@puri.sm>\n\t<164984145155.22830.5042825075821688379@Monstersaurus>","Organization":"Purism","MIME-Version":"1.0","Content-Type":"multipart/signed; boundary=\"Sig_/2fSoGJI+ZpbdH38picC9pxU\";\n\tprotocol=\"application/pgp-signature\"; micalg=pgp-sha256","Subject":"Re: [libcamera-devel] [PATCH] FrameBuffer: Make FrameBuffer::cancel\n\tprivate","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","From":"Dorota Czaplejewicz via libcamera-devel\n\t","Reply-To":"Dorota Czaplejewicz ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22704,"web_url":"https://patchwork.libcamera.org/comment/22704/","msgid":"<164985458844.22830.17335612204647495753@Monstersaurus>","date":"2022-04-13T12:56:28","subject":"Re: [libcamera-devel] [PATCH] FrameBuffer: Make FrameBuffer::cancel\n\tprivate","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Dorota Czaplejewicz (2022-04-13 13:23:57)\n> Hi,\n> \n> Signed-off-by: Dorota Czaplejewicz \n> \n> Did I do that right?\n\nPerfectly, thanks.\n\n> --Dorota\n> \n> On Wed, 13 Apr 2022 10:17:31 +0100\n> Kieran Bingham wrote:\n> \n> > Hi Dorota,\n> > \n> > Quoting Dorota Czaplejewicz (2021-12-22 12:16:29)\n> > > On Wed, 22 Dec 2021 12:05:20 +0000\n> > > Kieran Bingham wrote:\n> > > \n> > > > Hi Dorota,\n> > > > \n> > > > Quoting Kieran Bingham (2021-11-22 11:14:27) \n> > > > > Hi Dorota,\n> > > > > \n> > > > > Quoting Laurent Pinchart (2021-11-19 22:37:30) \n> > > > > > Hi Dorota,\n> > > > > > \n> > > > > > On Fri, Nov 19, 2021 at 11:33:43PM +0100, Dorota Czaplejewicz wrote: \n> > > > > > > On Fri, 19 Nov 2021 23:13:41 +0200 Laurent Pinchart wrote:\n> > > > > > > \n> > > > > > > > Hi Dorota,\n> > > > > > > > \n> > > > > > > > Thank you for the patch.\n> > > > > > > > \n> > > > > > > > The subject line should be\n> > > > > > > > \n> > > > > > > > libcamera: framebuffer: Make FrameBuffer::cancel() private\n> > > > > > > > \n> > > > > > > > to match the usual style (running `git log` on the file(s) you change is\n> > > > > > > > a good way to see what the style is).\n> > > > > > > > \n> > > > > > > > On Fri, Nov 19, 2021 at 04:05:58PM +0100, Dorota Czaplejewicz wrote: \n> > > > > > > > > FrameBuffer::cancel is not meant to be used by the API consumer. It was moved to FrameBuffer::Private::cancel . \n> > > > > > > > \n> > > > > > > > Commit messages should be wrapped to 72 columns (git + vim does that by\n> > > > > > > > default, I'm not sure about other editors).\n> > > > > > > > \n> > > > > > > > I'd write\n> > > > > > > > \n> > > > > > > > FrameBuffer::cancel() is not meant to be used by applications. Move it\n> > > > > > > > to the FrameBuffer::Private class.\n> > > > > > > > \n> > > > > > > > as \"API consumer\" could be understood as either the application, or the\n> > > > > > > > internal API users.\n> > > > > > > > \n> > > > > > > > Your Signed-off-by line is missing (see\n> > > > > > > > https://libcamera.org/contributing.html#submitting-patches). \n> > \n> > I was hoping to clean this up and merge it, but to retain your\n> > authorship I need to add your Signed-off by tag. Could you reply with it\n> > to this email to signify adding it to the patch please?\n> > \n> > --\n> > Kieran\n> > \n> > \n> > > > > > > > \n> > > > > > > > > ---\n> > > > > > > > > Hi,\n> > > > > > > > > \n> > > > > > > > > I'm sending this as discussed on the mailing list.\n> > > > > > > > > \n> > > > > > > > > Regards,\n> > > > > > > > > Dorota\n> > > > > > > > > \n> > > > > > > > > include/libcamera/framebuffer.h | 2 --\n> > > > > > > > > include/libcamera/internal/framebuffer.h | 2 ++\n> > > > > > > > > src/libcamera/framebuffer.cpp | 16 ++++++++--------\n> > > > > > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 5 +++--\n> > > > > > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 2 +-\n> > > > > > > > > src/libcamera/pipeline/vimc/vimc.cpp | 3 ++-\n> > > > > > > > > src/libcamera/request.cpp | 2 +-\n> > > > > > > > > 7 files changed, 17 insertions(+), 15 deletions(-)\n> > > > > > > > > \n> > > > > > > > > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> > > > > > > > > index 7f2f176a..367a8a71 100644\n> > > > > > > > > --- a/include/libcamera/framebuffer.h\n> > > > > > > > > +++ b/include/libcamera/framebuffer.h\n> > > > > > > > > @@ -66,8 +66,6 @@ public:\n> > > > > > > > > unsigned int cookie() const { return cookie_; }\n> > > > > > > > > void setCookie(unsigned int cookie) { cookie_ = cookie; }\n> > > > > > > > > \n> > > > > > > > > - void cancel() { metadata_.status = FrameMetadata::FrameCancelled; }\n> > > > > > > > > -\n> > > > > > > > > private:\n> > > > > > > > > LIBCAMERA_DISABLE_COPY_AND_MOVE(FrameBuffer)\n> > > > > > > > > \n> > > > > > > > > diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h\n> > > > > > > > > index cd33c295..27676212 100644\n> > > > > > > > > --- a/include/libcamera/internal/framebuffer.h\n> > > > > > > > > +++ b/include/libcamera/internal/framebuffer.h\n> > > > > > > > > @@ -23,6 +23,8 @@ public:\n> > > > > > > > > void setRequest(Request *request) { request_ = request; }\n> > > > > > > > > bool isContiguous() const { return isContiguous_; }\n> > > > > > > > > \n> > > > > > > > > + void cancel() { LIBCAMERA_O_PTR()->metadata_.status = FrameMetadata::FrameCancelled; } \n> > > > > > > > \n> > > > > > > > We try to keep line within a 80 columns soft limit when it doesn't\n> > > > > > > > otherwise hinder readability, so this could be\n> > > > > > > > \n> > > > > > > > void cancel()\n> > > > > > > > {\n> > > > > > > > FrameBuffer *const o = LIBCAMERA_O_PTR();\n> > > > > > > > o->metadata_.status = FrameMetadata::FrameCancelled;\n> > > > > > > > }\n> > > > > > > > \n> > > > > > > > > +\n> > > > > > > > > private:\n> > > > > > > > > Request *request_;\n> > > > > > > > > bool isContiguous_;\n> > > > > > > > > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> > > > > > > > > index 337ea115..dd344ed8 100644\n> > > > > > > > > --- a/src/libcamera/framebuffer.cpp\n> > > > > > > > > +++ b/src/libcamera/framebuffer.cpp\n> > > > > > > > > @@ -137,6 +137,14 @@ FrameBuffer::Private::Private()\n> > > > > > > > > * \\return True if the planes are stored contiguously in memory, false otherwise\n> > > > > > > > > */\n> > > > > > > > > \n> > > > > > > > > +/**\n> > > > > > > > > + * \\fn FrameBuffer::Private::cancel()\n> > > > > > > > > + * \\brief Marks the buffer as cancelled\n> > > > > > > > > + *\n> > > > > > > > > + * If a buffer is not used by a request, it shall be marked as cancelled to\n> > > > > > > > > + * indicate that the metadata is invalid. \n> > > > > > > > \n> > > > > > > > We've discussed the fact that the documentation isn't very explicit, but\n> > > > > > > > that should be addressed in a separate patch, not here. \n> > > > > > > \n> > > > > > > Why should it go in a separate patch? I only moved the doc because I\n> > > > > > > got a warning from doxygen. \n> > > > > > \n> > > > > > I meant that we should improve the documentation on top. This patch only\n> > > > > > moves it, which is right. There's nothing to change.\n> > > > > > \n> > > > > > > > The patch looks good overall. With these small issues fixed,\n> > > > > > > > \n> > > > > > > > Reviewed-by: Laurent Pinchart \n> > > > > > > > \n> > > > > > > > You can include that line in your v2. \n> > > > > \n> > > > > As previously discussed moving cancel() certainly makes it\n> > > > > clear that it can not be used by external public interfaces.\n> > > > > \n> > > > > With the small changes highlighted by Laurent:\n> > > > > \n> > > > > Reviewed-by: Kieran Bingham \n> > > > \n> > > > Do you plan to send a revised version of this patch, or would you like\n> > > > me to handle it for you?\n> > > > \n> > > Oh, sorry, this slipped my attention.\n> > > \n> > > I don't mind if you handle it before I get to it.\n> > > \n> > > Cheers,\n> > > Dorota\n> > > \n> > > > --\n> > > > Regards\n> > > > \n> > > > Kieran\n> > > > \n> > > > > \n> > > > > \n> > > > > > > > \n> > > > > > > > > + */\n> > > > > > > > > +\n> > > > > > > > > /**\n> > > > > > > > > * \\class FrameBuffer\n> > > > > > > > > * \\brief Frame buffer data and its associated dynamic metadata\n> > > > > > > > > @@ -305,12 +313,4 @@ Request *FrameBuffer::request() const\n> > > > > > > > > * libcamera core never modifies the buffer cookie.\n> > > > > > > > > */\n> > > > > > > > > \n> > > > > > > > > -/**\n> > > > > > > > > - * \\fn FrameBuffer::cancel()\n> > > > > > > > > - * \\brief Marks the buffer as cancelled\n> > > > > > > > > - *\n> > > > > > > > > - * If a buffer is not used by a request, it shall be marked as cancelled to\n> > > > > > > > > - * indicate that the metadata is invalid.\n> > > > > > > > > - */\n> > > > > > > > > -\n> > > > > > > > > } /* namespace libcamera */\n> > > > > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > > > > > index c65afdb2..f8516cba 100644\n> > > > > > > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > > > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > > > > > > @@ -27,6 +27,7 @@\n> > > > > > > > > #include \"libcamera/internal/camera_sensor.h\"\n> > > > > > > > > #include \"libcamera/internal/delayed_controls.h\"\n> > > > > > > > > #include \"libcamera/internal/device_enumerator.h\"\n> > > > > > > > > +#include \"libcamera/internal/framebuffer.h\"\n> > > > > > > > > #include \"libcamera/internal/ipa_manager.h\"\n> > > > > > > > > #include \"libcamera/internal/media_device.h\"\n> > > > > > > > > #include \"libcamera/internal/pipeline_handler.h\"\n> > > > > > > > > @@ -816,7 +817,7 @@ void IPU3CameraData::cancelPendingRequests()\n> > > > > > > > > \n> > > > > > > > > for (auto it : request->buffers()) {\n> > > > > > > > > FrameBuffer *buffer = it.second;\n> > > > > > > > > - buffer->cancel();\n> > > > > > > > > + buffer->_d()->cancel();\n> > > > > > > > > pipe()->completeBuffer(request, buffer);\n> > > > > > > > > }\n> > > > > > > > > \n> > > > > > > > > @@ -1333,7 +1334,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)\n> > > > > > > > > if (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> > > > > > > > > for (auto it : request->buffers()) {\n> > > > > > > > > FrameBuffer *b = it.second;\n> > > > > > > > > - b->cancel();\n> > > > > > > > > + b->_d()->cancel();\n> > > > > > > > > pipe()->completeBuffer(request, b);\n> > > > > > > > > }\n> > > > > > > > > \n> > > > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > > > > index 5e1f2273..b7cabf5e 100644\n> > > > > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > > > > > @@ -1573,7 +1573,7 @@ void RPiCameraData::clearIncompleteRequests()\n> > > > > > > > > * request? If not, do so now.\n> > > > > > > > > */\n> > > > > > > > > if (buffer->request()) {\n> > > > > > > > > - buffer->cancel();\n> > > > > > > > > + buffer->_d()->cancel();\n> > > > > > > > > pipe()->completeBuffer(request, buffer);\n> > > > > > > > > }\n> > > > > > > > > }\n> > > > > > > > > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> > > > > > > > > index e453091d..1ec814d1 100644\n> > > > > > > > > --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> > > > > > > > > +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> > > > > > > > > @@ -32,6 +32,7 @@\n> > > > > > > > > #include \"libcamera/internal/camera.h\"\n> > > > > > > > > #include \"libcamera/internal/camera_sensor.h\"\n> > > > > > > > > #include \"libcamera/internal/device_enumerator.h\"\n> > > > > > > > > +#include \"libcamera/internal/framebuffer.h\"\n> > > > > > > > > #include \"libcamera/internal/ipa_manager.h\"\n> > > > > > > > > #include \"libcamera/internal/media_device.h\"\n> > > > > > > > > #include \"libcamera/internal/pipeline_handler.h\"\n> > > > > > > > > @@ -574,7 +575,7 @@ void VimcCameraData::bufferReady(FrameBuffer *buffer)\n> > > > > > > > > if (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> > > > > > > > > for (auto it : request->buffers()) {\n> > > > > > > > > FrameBuffer *b = it.second;\n> > > > > > > > > - b->cancel();\n> > > > > > > > > + b->_d()->cancel();\n> > > > > > > > > pipe->completeBuffer(request, b);\n> > > > > > > > > }\n> > > > > > > > > \n> > > > > > > > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > > > > > > > > index 17fefab7..a4dbf750 100644\n> > > > > > > > > --- a/src/libcamera/request.cpp\n> > > > > > > > > +++ b/src/libcamera/request.cpp\n> > > > > > > > > @@ -304,7 +304,7 @@ void Request::cancel()\n> > > > > > > > > ASSERT(status_ == RequestPending);\n> > > > > > > > > \n> > > > > > > > > for (FrameBuffer *buffer : pending_) {\n> > > > > > > > > - buffer->cancel();\n> > > > > > > > > + buffer->_d()->cancel();\n> > > > > > > > > camera_->bufferCompleted.emit(this, buffer);\n> > > > > > > > > }\n> > > > > > > > > \n> > > > > > \n> > > > > > -- \n> > > > > > Regards,\n> > > > > > \n> > > > > > Laurent Pinchart \n> > > \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 26B20C3256\n\tfor ;\n\tWed, 13 Apr 2022 12:56:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8B75965644;\n\tWed, 13 Apr 2022 14:56:32 +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 432FD604B4\n\tfor ;\n\tWed, 13 Apr 2022 14:56:31 +0200 (CEST)","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 BEA2825B;\n\tWed, 13 Apr 2022 14:56:30 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649854592;\n\tbh=0IT9gQE2Kv4N27Dk2HYk652s/F11W8QQI0ET9l/ImJ4=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=uaVgPZ0oL++roZ70kHhukg4B+UQMrs9L989WVA0NbZiQFKd96DqnM/qzZX+EB/P6R\n\tw7DJYFo3/UalCXXFP5JiO7g4/eNwumz2I9XbetUfemW+80VC/PjFitqwftW0CJ82Gq\n\tBgt3EfJ86CBQDnzr8s5IDOYPMypQ5qMdyb/K5QMFwhUliLsURsKdmq10L0HOXfVkB+\n\t2bE7kh0B9147BQLm1vO8gsP3/V8tMgTeXXSCfsyGkfbtJEJ7F3IT1iDNycccHHqdxX\n\tBi18KK7FN3ArEtyoXBzfMG33uk+1a57AcpovxTiCESO6u/BKk5++Qgcd/aZxS3AbHJ\n\tKRLkCQFOAvy0Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1649854590;\n\tbh=0IT9gQE2Kv4N27Dk2HYk652s/F11W8QQI0ET9l/ImJ4=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=Q8dWP6uFKiAEz/e+MM0KgyWAveqaRevYy8IQfmmzTJJmDQ7/SSiEpLWoyBCMwoBgp\n\t9KPuPybcAYhUN6bfukz7BW2cjOXpPrYDu3pNNy56DnWbi6I8iYY1whriL15PBUje9y\n\toFXdkaW57OXuuwwronETZjcmF+IxnLHP9NKMa/Xw="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Q8dWP6uF\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220413142357.5b101129.dorota.czaplejewicz@puri.sm>","References":"<20211119150421.974848-1-dorota.czaplejewicz@puri.sm>\n\t\n\t<20211119233343.66b9ff94.dorota.czaplejewicz@puri.sm>\n\t\n\t<163757966785.1089182.1511028075107359458@Monstersaurus>\n\t<164017472044.3986460.262907047419586856@Monstersaurus>\n\t<20211222131629.6c889224.dorota.czaplejewicz@puri.sm>\n\t<164984145155.22830.5042825075821688379@Monstersaurus>\n\t<20220413142357.5b101129.dorota.czaplejewicz@puri.sm>","To":"Dorota Czaplejewicz ","Date":"Wed, 13 Apr 2022 13:56:28 +0100","Message-ID":"<164985458844.22830.17335612204647495753@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH] FrameBuffer: Make FrameBuffer::cancel\n\tprivate","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]