[{"id":19276,"web_url":"https://patchwork.libcamera.org/comment/19276/","msgid":"<85b93468-5aa4-966f-3a01-85adbd7b96cb@ideasonboard.com>","date":"2021-09-02T09:27:51","subject":"Re: [libcamera-devel] [RFC PATCH v1 05/12] libcamera: framebuffer:\n\tMove planes check to constructor","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 02/09/2021 05:22, Laurent Pinchart wrote:\n> The FrameBuffer::planes() function checks that planes are correctly\n> initialized with an offset. This can be done at construction time\n> instead, as the planes are constant. The runtime overhead is reduced,\n> and the backtrace generated by the assertion will show where the faulty\n> frame buffer is created instead of where it is used, easing debugging.\n\nThat sounds very enticing...\n\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> include/libcamera/framebuffer.h | 9 +--------\n> src/libcamera/framebuffer.cpp | 3 +++\n> 2 files changed, 4 insertions(+), 8 deletions(-)\n> \n> diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> index d5aeff00387b..fd68ed0a139d 100644\n> --- a/include/libcamera/framebuffer.h\n> +++ b/include/libcamera/framebuffer.h\n> @@ -51,14 +51,7 @@ public:\n> \n> \tFrameBuffer(const std::vector &planes, unsigned int cookie = 0);\n> \n> -\tconst std::vector &planes() const\n> -\t{\n> -\t\t/* \\todo Remove the assertions after sufficient testing */\n> -\t\tfor (const auto &plane : planes_)\n> -\t\t\tassert(plane.offset != Plane::kInvalidOffset);\n> -\t\treturn planes_;\n> -\t}\n> -\n> +\tconst std::vector &planes() const { return planes_; }\n> \tRequest *request() const;\n> \tconst FrameMetadata &metadata() const { return metadata_; }\n> \n> diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> index c99f5b15e6ff..53ef89bf458f 100644\n> --- a/src/libcamera/framebuffer.cpp\n> +++ b/src/libcamera/framebuffer.cpp\n> @@ -199,6 +199,9 @@ FrameBuffer::FrameBuffer(const std::vector &planes, unsigned int cookie)\n> \t: Extensible(std::make_unique()), planes_(planes),\n> \t cookie_(cookie)\n> {\n> +\t/* \\todo Remove the assertions after sufficient testing */\n\nAs a construction time validation event, which is compiled/optimised out\nin release builds anyway, I'm tempted to remove the todo here and keep\nthe ASSERT ...\n\nBut either way,\n\nReviewed-by: Kieran Bingham \n\n> +\tfor (const auto &plane : planes_)\n> +\t\tASSERT(plane.offset != Plane::kInvalidOffset);\n> }\n> \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 B7593BDC71\n\tfor ;\n\tThu, 2 Sep 2021 09:27:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 28AB86916A;\n\tThu, 2 Sep 2021 11:27:56 +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 1633560254\n\tfor ;\n\tThu, 2 Sep 2021 11:27:54 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8424245E;\n\tThu, 2 Sep 2021 11:27:53 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ekHaqk5M\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630574873;\n\tbh=eJHVugkQT79+WOQmwCMfR89aN5FpnaSC218vHf1MWlk=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=ekHaqk5MWfdKEG9JfXOeeUdkyp89giL4SoUIDWEGgsvnj/rB1GgJD6JJiHaVZv07Y\n\t7jhcGhRdBgmb32UpD2xQM2mQphX+ktLOVR2DH+tBJKzifCeJN5WXf3YH3kZiJO3c1R\n\tXF5Cz0Ie79eBMxuKJWhhlegzHl7GQiiqGZkES+8c=","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210902042303.2254-1-laurent.pinchart@ideasonboard.com>\n\t<20210902042303.2254-6-laurent.pinchart@ideasonboard.com>","From":"Kieran Bingham ","Message-ID":"<85b93468-5aa4-966f-3a01-85adbd7b96cb@ideasonboard.com>","Date":"Thu, 2 Sep 2021 10:27:51 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210902042303.2254-6-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [RFC PATCH v1 05/12] libcamera: framebuffer:\n\tMove planes check to constructor","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19302,"web_url":"https://patchwork.libcamera.org/comment/19302/","msgid":"","date":"2021-09-02T18:03:22","subject":"Re: [libcamera-devel] [RFC PATCH v1 05/12] libcamera: framebuffer:\n\tMove planes check to constructor","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Thu, Sep 02, 2021 at 10:27:51AM +0100, Kieran Bingham wrote:\n> On 02/09/2021 05:22, Laurent Pinchart wrote:\n> > The FrameBuffer::planes() function checks that planes are correctly\n> > initialized with an offset. This can be done at construction time\n> > instead, as the planes are constant. The runtime overhead is reduced,\n> > and the backtrace generated by the assertion will show where the faulty\n> > frame buffer is created instead of where it is used, easing debugging.\n> \n> That sounds very enticing...\n> \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > include/libcamera/framebuffer.h | 9 +--------\n> > src/libcamera/framebuffer.cpp | 3 +++\n> > 2 files changed, 4 insertions(+), 8 deletions(-)\n> > \n> > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> > index d5aeff00387b..fd68ed0a139d 100644\n> > --- a/include/libcamera/framebuffer.h\n> > +++ b/include/libcamera/framebuffer.h\n> > @@ -51,14 +51,7 @@ public:\n> > \n> > \tFrameBuffer(const std::vector &planes, unsigned int cookie = 0);\n> > \n> > -\tconst std::vector &planes() const\n> > -\t{\n> > -\t\t/* \\todo Remove the assertions after sufficient testing */\n> > -\t\tfor (const auto &plane : planes_)\n> > -\t\t\tassert(plane.offset != Plane::kInvalidOffset);\n> > -\t\treturn planes_;\n> > -\t}\n> > -\n> > +\tconst std::vector &planes() const { return planes_; }\n> > \tRequest *request() const;\n> > \tconst FrameMetadata &metadata() const { return metadata_; }\n> > \n> > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> > index c99f5b15e6ff..53ef89bf458f 100644\n> > --- a/src/libcamera/framebuffer.cpp\n> > +++ b/src/libcamera/framebuffer.cpp\n> > @@ -199,6 +199,9 @@ FrameBuffer::FrameBuffer(const std::vector &planes, unsigned int cookie)\n> > \t: Extensible(std::make_unique()), planes_(planes),\n> > \t cookie_(cookie)\n> > {\n> > +\t/* \\todo Remove the assertions after sufficient testing */\n> \n> As a construction time validation event, which is compiled/optimised out\n> in release builds anyway, I'm tempted to remove the todo here and keep\n> the ASSERT ...\n\nI agree, I'll drop the comment and explain why in the commit message:\n\nlibcamera: framebuffer: Move planes check to constructor\n\nThe FrameBuffer::planes() function checks that planes are correctly\ninitialized with an offset. This can be done at construction time\ninstead, as the planes are constant. The backtrace generated by the\nassertion will show where the faulty frame buffer is created instead of\nwhere it is used, easing debugging.\n\nAs the runtime overhead is reduced, there's no real need to drop the\nassertion in the future anymore, it can be useful to ensure that the\nplanes are correctly populated by the caller. Drop the comment that\ncalls for removing the check.\n\n> But either way,\n> \n> Reviewed-by: Kieran Bingham \n> \n> > +\tfor (const auto &plane : planes_)\n> > +\t\tASSERT(plane.offset != Plane::kInvalidOffset);\n> > }\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 B5AC2BDC71\n\tfor ;\n\tThu, 2 Sep 2021 18:03:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 03AE26916A;\n\tThu, 2 Sep 2021 20:03:41 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B6EC560255\n\tfor ;\n\tThu, 2 Sep 2021 20:03:39 +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 417411544;\n\tThu, 2 Sep 2021 20:03:39 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"KMDHtgOh\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630605819;\n\tbh=jxXeIfHqazjZMIJ9ocg5g3WSWbN08guc9rOVDStC/VI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=KMDHtgOhsIfWuxv1Xyc3AM8a888aXrRt63D54fSC14GXa12gUJ2atmvnS0qCzu0oB\n\tlCIxORzJVfCd4/akSoG6zVo59QB9FF/tMzq1XYiMGJoZkljvNkVQ3TfT5wg05qdXgF\n\tV5oilPgr5rARXap2StDqBBv0uT86yk3swcRFZQtM=","Date":"Thu, 2 Sep 2021 21:03:22 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210902042303.2254-1-laurent.pinchart@ideasonboard.com>\n\t<20210902042303.2254-6-laurent.pinchart@ideasonboard.com>\n\t<85b93468-5aa4-966f-3a01-85adbd7b96cb@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<85b93468-5aa4-966f-3a01-85adbd7b96cb@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH v1 05/12] libcamera: framebuffer:\n\tMove planes check to constructor","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":19348,"web_url":"https://patchwork.libcamera.org/comment/19348/","msgid":"","date":"2021-09-03T10:57:21","subject":"Re: [libcamera-devel] [RFC PATCH v1 05/12] libcamera: framebuffer:\n\tMove planes check to constructor","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent, thank you for the patch.\n\nOn Fri, Sep 3, 2021 at 3:03 AM Laurent Pinchart\n wrote:\n>\n> Hi Kieran,\n>\n> On Thu, Sep 02, 2021 at 10:27:51AM +0100, Kieran Bingham wrote:\n> > On 02/09/2021 05:22, Laurent Pinchart wrote:\n> > > The FrameBuffer::planes() function checks that planes are correctly\n> > > initialized with an offset. This can be done at construction time\n> > > instead, as the planes are constant. The runtime overhead is reduced,\n> > > and the backtrace generated by the assertion will show where the faulty\n> > > frame buffer is created instead of where it is used, easing debugging.\n> >\n> > That sounds very enticing...\n> >\n> > > Signed-off-by: Laurent Pinchart \n\n\nReviewed-by: Hirokazu Honda \n\n-Hiro\n> > > ---\n> > > include/libcamera/framebuffer.h | 9 +--------\n> > > src/libcamera/framebuffer.cpp | 3 +++\n> > > 2 files changed, 4 insertions(+), 8 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> > > index d5aeff00387b..fd68ed0a139d 100644\n> > > --- a/include/libcamera/framebuffer.h\n> > > +++ b/include/libcamera/framebuffer.h\n> > > @@ -51,14 +51,7 @@ public:\n> > >\n> > > FrameBuffer(const std::vector &planes, unsigned int cookie = 0);\n> > >\n> > > - const std::vector &planes() const\n> > > - {\n> > > - /* \\todo Remove the assertions after sufficient testing */\n> > > - for (const auto &plane : planes_)\n> > > - assert(plane.offset != Plane::kInvalidOffset);\n> > > - return planes_;\n> > > - }\n> > > -\n> > > + const std::vector &planes() const { return planes_; }\n> > > Request *request() const;\n> > > const FrameMetadata &metadata() const { return metadata_; }\n> > >\n> > > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> > > index c99f5b15e6ff..53ef89bf458f 100644\n> > > --- a/src/libcamera/framebuffer.cpp\n> > > +++ b/src/libcamera/framebuffer.cpp\n> > > @@ -199,6 +199,9 @@ FrameBuffer::FrameBuffer(const std::vector &planes, unsigned int cookie)\n> > > : Extensible(std::make_unique()), planes_(planes),\n> > > cookie_(cookie)\n> > > {\n> > > + /* \\todo Remove the assertions after sufficient testing */\n> >\n> > As a construction time validation event, which is compiled/optimised out\n> > in release builds anyway, I'm tempted to remove the todo here and keep\n> > the ASSERT ...\n>\n> I agree, I'll drop the comment and explain why in the commit message:\n>\n> libcamera: framebuffer: Move planes check to constructor\n>\n> The FrameBuffer::planes() function checks that planes are correctly\n> initialized with an offset. This can be done at construction time\n> instead, as the planes are constant. The backtrace generated by the\n> assertion will show where the faulty frame buffer is created instead of\n> where it is used, easing debugging.\n>\n> As the runtime overhead is reduced, there's no real need to drop the\n> assertion in the future anymore, it can be useful to ensure that the\n> planes are correctly populated by the caller. Drop the comment that\n> calls for removing the check.\n>\n> > But either way,\n> >\n> > Reviewed-by: Kieran Bingham \n> >\n> > > + for (const auto &plane : planes_)\n> > > + ASSERT(plane.offset != Plane::kInvalidOffset);\n> > > }\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 71FB4BD87D\n\tfor ;\n\tFri, 3 Sep 2021 10:57:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E853369169;\n\tFri, 3 Sep 2021 12:57:35 +0200 (CEST)","from mail-yb1-xb2f.google.com (mail-yb1-xb2f.google.com\n\t[IPv6:2607:f8b0:4864:20::b2f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8B14B69168\n\tfor ;\n\tFri, 3 Sep 2021 12:57:34 +0200 (CEST)","by mail-yb1-xb2f.google.com with SMTP id a93so9402507ybi.1\n\tfor ;\n\tFri, 03 Sep 2021 03:57:34 -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=\"YqTP2K8I\"; 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=u9Y0XS7DBo8lG2lamqVibxnYMzpsFkcKofGm65kMHjc=;\n\tb=YqTP2K8IVy+GtHNp0T2p19frAdoG6wfTRq4NULtq49PhuZb+pfFBog4dgX1UrKsoJA\n\tkuEKrric35IpKbfZYZUiB+EFj8tC0y1sm1tK2QlRRT5oshgpFKaaxmNeywirwe0gwa6B\n\tpKNLxJCXLxkh0F0hGeVF9SHONPVn9VpTvXqSY=","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=u9Y0XS7DBo8lG2lamqVibxnYMzpsFkcKofGm65kMHjc=;\n\tb=STBPNO9KQy34ZHgtzbnnxB3jDlTYvp+imVx+Xy28na0VgDC9pMcCzPrwsm3Fei9Ow+\n\tSR12ia5yz1S3gjVVw77RGIOMzksFkQ7KFdXInNv9aq7+I7qYUCpW93EzcjwXvxQ8FLCl\n\tgq69yyGxKF9VIhiEHlJpoGWjGWhoIuD8YbwQi0P6CexW41+PW3Zbf/5ysemi17nCGLJy\n\t42ihn4cpKJfFd6vrufyTvvP2qlix76b0C2X5ezR+yFT2pG81PjaunICe3sY8kVXj7ii/\n\t5ZkQv255vBfXA/0ZBpuuL3pWTFs15kNE42PgFkgNvsvFVm4u3ePsIkhGxZd80u3qtxfq\n\tdmZQ==","X-Gm-Message-State":"AOAM53324/5URXbkLy98Wpg+sTsIh6UItgXczR43XyZNkQYG8jRD6BYk\n\tzGpAgUfXLdbtzC/VxhepdS1qYTA9FoEkgosBLnjcbQ==","X-Google-Smtp-Source":"ABdhPJyCoScSfWF3YzxIL5Ym+ZPCWoZq5I5LviIOCtAXf8zLbeh8wVauAZjOcS3vydD8BgJ3J7UsBr2HLlmxzo3vQn8=","X-Received":"by 2002:a25:c184:: with SMTP id\n\tr126mr3898036ybf.123.1630666653188; \n\tFri, 03 Sep 2021 03:57:33 -0700 (PDT)","MIME-Version":"1.0","References":"<20210902042303.2254-1-laurent.pinchart@ideasonboard.com>\n\t<20210902042303.2254-6-laurent.pinchart@ideasonboard.com>\n\t<85b93468-5aa4-966f-3a01-85adbd7b96cb@ideasonboard.com>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Fri, 3 Sep 2021 19:57:21 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH v1 05/12] libcamera: framebuffer:\n\tMove planes check to constructor","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\" "}}]