[{"id":18888,"web_url":"https://patchwork.libcamera.org/comment/18888/","msgid":"","date":"2021-08-18T01:42:26","subject":"Re: [libcamera-devel] [RFC PATCH 10/10] libcamera: framebuffer: Add\n\tassertion to detect offset is unfilled","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nThank you for the patch.\n\nOn Mon, Aug 16, 2021 at 01:31:38PM +0900, Hirokazu Honda wrote:\n> The offset variable is introduced to FrameBuffer::Plane. In order to\n> detect that the plane is used while the offset is not set, this adds\n> the assertion to FrameBuffer::planes(). It should be removed in the\n> future.\n> \n> Signed-off-by: Hirokazu Honda \n> ---\n> include/libcamera/framebuffer.h | 13 +++++++++++--\n> 1 file changed, 11 insertions(+), 2 deletions(-)\n> \n> diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> index 5de3c744..79541557 100644\n> --- a/include/libcamera/framebuffer.h\n> +++ b/include/libcamera/framebuffer.h\n> @@ -7,6 +7,8 @@\n> #ifndef __LIBCAMERA_FRAMEBUFFER_H__\n> #define __LIBCAMERA_FRAMEBUFFER_H__\n> \n> +#include \n> +#include \n> #include \n> #include \n> \n> @@ -41,14 +43,21 @@ class FrameBuffer final : public Extensible\n> \n> public:\n> \tstruct Plane {\n> +\t\tstatic constexpr unsigned int kInvalidOffset = std::numeric_limits::max();\n\nMaybe a blank line here ?\n\nThis also generates a doxygen warning as kInvalidOffset is not\ndocumented. As it's not meant to be used by anyone else than the code\nbelow, this could be addressed by making the constant private.\n\n> \t\tFileDescriptor fd;\n> -\t\tunsigned int offset;\n> +\t\tunsigned int offset = kInvalidOffset;\n> \t\tunsigned int length;\n> \t};\n> \n> \tFrameBuffer(const std::vector &planes, unsigned int cookie = 0);\n> \n> -\tconst std::vector &planes() const { return planes_; }\n> +\tconst std::vector &planes() const\n> +\t{\n> +\t\t/* \\todo remove this assert. */\n\nWhen do you foresee this being removed ?\n\n> +\t\tfor (const auto &plane : planes_)\n> +\t\t\tassert(plane.offset != Plane::kInvalidOffset);\n> +\t\treturn planes_;\n> +\t}\n> \n> \tRequest *request() const;\n> \tconst FrameMetadata &metadata() const { return metadata_; }","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 03D91BD87D\n\tfor ;\n\tWed, 18 Aug 2021 01:42:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 718CF68894;\n\tWed, 18 Aug 2021 03:42:35 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 22DE06025D\n\tfor ;\n\tWed, 18 Aug 2021 03:42:34 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A19CE466;\n\tWed, 18 Aug 2021 03:42:33 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"kOHw+OBB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629250953;\n\tbh=v0a6c7fe+/rPosFgPcPaV67z2tnPLsceOe1FRJHoyBY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=kOHw+OBBF2DJYeNZXpcKx0TTKYRFlcbIdpQERLv+lTEGTTl+XEa0uo06LWVpnSG0w\n\t766PrUohOANTRcEMBM4qSsWmrvElXWwdoLOydGpT2+t4ysDla4jvBq6jmrrZb+PeZe\n\twZsl5mKoST7OGHA5nDYlA40e6lAqME1ceWdcQIqk=","Date":"Wed, 18 Aug 2021 04:42:26 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210816043138.957984-1-hiroh@chromium.org>\n\t<20210816043138.957984-11-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210816043138.957984-11-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [RFC PATCH 10/10] libcamera: framebuffer: Add\n\tassertion to detect offset is unfilled","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18974,"web_url":"https://patchwork.libcamera.org/comment/18974/","msgid":"","date":"2021-08-20T11:18:47","subject":"Re: [libcamera-devel] [RFC PATCH 10/10] libcamera: framebuffer: Add\n\tassertion to detect offset is unfilled","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\nOn Wed, Aug 18, 2021 at 10:42 AM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> Thank you for the patch.\n>\n> On Mon, Aug 16, 2021 at 01:31:38PM +0900, Hirokazu Honda wrote:\n> > The offset variable is introduced to FrameBuffer::Plane. In order to\n> > detect that the plane is used while the offset is not set, this adds\n> > the assertion to FrameBuffer::planes(). It should be removed in the\n> > future.\n> >\n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > include/libcamera/framebuffer.h | 13 +++++++++++--\n> > 1 file changed, 11 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> > index 5de3c744..79541557 100644\n> > --- a/include/libcamera/framebuffer.h\n> > +++ b/include/libcamera/framebuffer.h\n> > @@ -7,6 +7,8 @@\n> > #ifndef __LIBCAMERA_FRAMEBUFFER_H__\n> > #define __LIBCAMERA_FRAMEBUFFER_H__\n> >\n> > +#include \n> > +#include \n> > #include \n> > #include \n> >\n> > @@ -41,14 +43,21 @@ class FrameBuffer final : public Extensible\n> >\n> > public:\n> > struct Plane {\n> > + static constexpr unsigned int kInvalidOffset = std::numeric_limits::max();\n>\n> Maybe a blank line here ?\n>\n> This also generates a doxygen warning as kInvalidOffset is not\n> documented. As it's not meant to be used by anyone else than the code\n> below, this could be addressed by making the constant private.\n>\n\nI couldn't move it to private because it is used in the below assert.\nI document the variable.\n\n> > FileDescriptor fd;\n> > - unsigned int offset;\n> > + unsigned int offset = kInvalidOffset;\n> > unsigned int length;\n> > };\n> >\n> > FrameBuffer(const std::vector &planes, unsigned int cookie = 0);\n> >\n> > - const std::vector &planes() const { return planes_; }\n> > + const std::vector &planes() const\n> > + {\n> > + /* \\todo remove this assert. */\n>\n> When do you foresee this being removed ?\n>\n\nMaybe for a month. Just an arbitrary foresee.\n\nBest Regards,\n-Hiro\n> > + for (const auto &plane : planes_)\n> > + assert(plane.offset != Plane::kInvalidOffset);\n> > + return planes_;\n> > + }\n> >\n> > Request *request() const;\n> > const FrameMetadata &metadata() const { return metadata_; }\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 65523BD87D\n\tfor ;\n\tFri, 20 Aug 2021 11:19:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D4DDA68891;\n\tFri, 20 Aug 2021 13:19:00 +0200 (CEST)","from mail-ed1-x534.google.com (mail-ed1-x534.google.com\n\t[IPv6:2a00:1450:4864:20::534])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6A5D76025E\n\tfor ;\n\tFri, 20 Aug 2021 13:18:59 +0200 (CEST)","by mail-ed1-x534.google.com with SMTP id b7so13532361edu.3\n\tfor ;\n\tFri, 20 Aug 2021 04:18:59 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"jfjr5hDO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=edv5px5AuarTfJee0u72OeDiYQ/w4QNGSqvqrwc6JbY=;\n\tb=jfjr5hDOnkCSu4cnXPk+f0gtr0Mtcfzj5pwuyWqpuJEBr7fahvTDgHhADAqZQ7nCCc\n\tYpjPN2CdBfDu96O/ERHrPtNeEAs5x4UWcNPZgx6UPjGCCuhXjsNOshd2t9zubJgVzggj\n\t8pZB3Nl4KSuA8GlLcuuOZmTRNfEVylv2CTvkI=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=edv5px5AuarTfJee0u72OeDiYQ/w4QNGSqvqrwc6JbY=;\n\tb=EDVSkguXWMB5f7MZ9eIVDJezvcD6a5pKbTLExIgLL1JB0QzT+B7zZ5TmhL77qtvGYA\n\tpONUH8IQHTW+b3DZu1gQGoevoZLPVqGeaeYv05QJPjaoSTJbANFDHD+B/7hDYADL1lrr\n\tJwNU37ErqF0xM//cL+917/ns0DsxPx1Fu2/0tY1SGqCPm7WfrpEu1pyFlyrmyZqCSFuN\n\tfnbOp/SyFhor+PyKMeXLV7bo2FoKyVv+LLgG2X3/qiaDqW2oJ8pXkUXoYZ6giOjUtqay\n\tIT6deoLWvSp2dI3tKK0W/fgqJxzxfs63ZTS0S4upnWlY7DjrJ+/0MVCf2Vg2NlwzTisO\n\tLa9Q==","X-Gm-Message-State":"AOAM532cnWGbLlm0ec9Dr8e9sxcQZ41kcmTp+VZoKx6lg9A5B1R2pAwq\n\te1vIDc3tYJt8iL8nj2oQeEkALYdTHY3TwOqym4BgAA==","X-Google-Smtp-Source":"ABdhPJx4lHLVmPb5P++PIalzX2Coj+13mCYgjwaCKUmcaAnjpXxZ3c/xsQz7pBYb8+NVngyInFT3h6FKCYyEeESgCSA=","X-Received":"by 2002:a50:ed99:: with SMTP id\n\th25mr22305333edr.327.1629458339079; \n\tFri, 20 Aug 2021 04:18:59 -0700 (PDT)","MIME-Version":"1.0","References":"<20210816043138.957984-1-hiroh@chromium.org>\n\t<20210816043138.957984-11-hiroh@chromium.org>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Fri, 20 Aug 2021 20:18:47 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH 10/10] libcamera: framebuffer: Add\n\tassertion to detect offset is unfilled","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18987,"web_url":"https://patchwork.libcamera.org/comment/18987/","msgid":"","date":"2021-08-20T15:31:24","subject":"Re: [libcamera-devel] [RFC PATCH 10/10] libcamera: framebuffer: Add\n\tassertion to detect offset is unfilled","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Fri, Aug 20, 2021 at 08:18:47PM +0900, Hirokazu Honda wrote:\n> On Wed, Aug 18, 2021 at 10:42 AM Laurent Pinchart wrote:\n> > On Mon, Aug 16, 2021 at 01:31:38PM +0900, Hirokazu Honda wrote:\n> > > The offset variable is introduced to FrameBuffer::Plane. In order to\n> > > detect that the plane is used while the offset is not set, this adds\n> > > the assertion to FrameBuffer::planes(). It should be removed in the\n> > > future.\n> > >\n> > > Signed-off-by: Hirokazu Honda \n> > > ---\n> > > include/libcamera/framebuffer.h | 13 +++++++++++--\n> > > 1 file changed, 11 insertions(+), 2 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> > > index 5de3c744..79541557 100644\n> > > --- a/include/libcamera/framebuffer.h\n> > > +++ b/include/libcamera/framebuffer.h\n> > > @@ -7,6 +7,8 @@\n> > > #ifndef __LIBCAMERA_FRAMEBUFFER_H__\n> > > #define __LIBCAMERA_FRAMEBUFFER_H__\n> > >\n> > > +#include \n> > > +#include \n> > > #include \n> > > #include \n> > >\n> > > @@ -41,14 +43,21 @@ class FrameBuffer final : public Extensible\n> > >\n> > > public:\n> > > struct Plane {\n> > > + static constexpr unsigned int kInvalidOffset = std::numeric_limits::max();\n> >\n> > Maybe a blank line here ?\n> >\n> > This also generates a doxygen warning as kInvalidOffset is not\n> > documented. As it's not meant to be used by anyone else than the code\n> > below, this could be addressed by making the constant private.\n> >\n> \n> I couldn't move it to private because it is used in the below assert.\n> I document the variable.\n> \n> > > FileDescriptor fd;\n> > > - unsigned int offset;\n> > > + unsigned int offset = kInvalidOffset;\n> > > unsigned int length;\n> > > };\n> > >\n> > > FrameBuffer(const std::vector &planes, unsigned int cookie = 0);\n> > >\n> > > - const std::vector &planes() const { return planes_; }\n> > > + const std::vector &planes() const\n> > > + {\n> > > + /* \\todo remove this assert. */\n> >\n> > When do you foresee this being removed ?\n> \n> Maybe for a month. Just an arbitrary foresee.\n\nThen let's write\n\n\t\t/* \\todo Remove the assertions after sufficient testing */\n\n> > > + for (const auto &plane : planes_)\n> > > + assert(plane.offset != Plane::kInvalidOffset);\n> > > + return planes_;\n> > > + }\n> > >\n> > > Request *request() const;\n> > > const FrameMetadata &metadata() const { return metadata_; }","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 C56D8BD87C\n\tfor ;\n\tFri, 20 Aug 2021 15:31:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 38ABF688A2;\n\tFri, 20 Aug 2021 17:31: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 5F9986888E\n\tfor ;\n\tFri, 20 Aug 2021 17:31:34 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CEAB68C8;\n\tFri, 20 Aug 2021 17:31:33 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"WkxCJzz3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629473494;\n\tbh=IqklkxPDkCgBj5kMC4/eIKpp+g85cTdqfgqkPDnraW4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WkxCJzz3oruPG9kN6tY4BxjAqz800T3DlGvQK9WU5435tPgeB1Mi5YY2cQ+tnrC/l\n\tECFtgF/XQ4SqlAboAuR6gZ/kAHTX2Eqok5yf9V1H+5h+sNBKl1+6Gm9EOecKnG0cCD\n\tpqjCZdx7IEmS73xqL3fhEg1QuqdusayTDbrlnppo=","Date":"Fri, 20 Aug 2021 18:31:24 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210816043138.957984-1-hiroh@chromium.org>\n\t<20210816043138.957984-11-hiroh@chromium.org>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH 10/10] libcamera: framebuffer: Add\n\tassertion to detect offset is unfilled","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]