[{"id":20640,"web_url":"https://patchwork.libcamera.org/comment/20640/","msgid":"<20211029144046.337jjp7qebu6t55w@uno.localdomain>","date":"2021-10-29T14:40:46","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: framebuffer: Enable\n\tattaching additional data to FrameBuffer","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Thu, Oct 28, 2021 at 04:30:37PM +0900, Hirokazu Honda wrote:\n> We cannot have a subclass of FrameBuffer because it is marked as final.\n> This adds a FrameBuffer constructor with FrameBuffer::Private. So we\n> can attach some additional resources with FrameBuffer through a\n> customized FrameBuffer::Private class.\n>\n> Signed-off-by: Hirokazu Honda \n> ---\n> include/libcamera/framebuffer.h | 2 ++\n> include/libcamera/internal/framebuffer.h | 1 +\n> src/libcamera/framebuffer.cpp | 17 +++++++++++++++--\n> 3 files changed, 18 insertions(+), 2 deletions(-)\n>\n> diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> index 7f2f176a..02bf1533 100644\n> --- a/include/libcamera/framebuffer.h\n> +++ b/include/libcamera/framebuffer.h\n> @@ -57,6 +57,8 @@ public:\n> \t\tunsigned int length;\n> \t};\n>\n> +\tFrameBuffer(std::unique_ptr d,\n> +\t\t const std::vector &planes, unsigned int cookie = 0);\n\nI wonder if it's right to make this available from a public\nheader. We have this pattern in use for Camera, which cannot however be\nconstructed from the libcamera public API, while FrameBuffer can.\n\nApplications cannot extend FrameBuffer::Private though, so only\ncomponents with access to the internal headers can, so I guess this is\nfine!\n\n\n> \tFrameBuffer(const std::vector &planes, unsigned int cookie = 0);\n\nCould you align the declaration order in the .cpp and .h files ?\n\nAlternatively, we can decide FrameBuffer can potentially store some\nPrivateData in its FrameBuffer::Private in the case we ever get to\nprovide an allocator based on dmabuf heaps. But that's maybe for\nlater.\n\n>\n> \tconst std::vector &planes() const { return planes_; }\n> diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h\n> index cd33c295..5ab9b3b2 100644\n> --- a/include/libcamera/internal/framebuffer.h\n> +++ b/include/libcamera/internal/framebuffer.h\n> @@ -19,6 +19,7 @@ class FrameBuffer::Private : public Extensible::Private\n>\n> public:\n> \tPrivate();\n> +\tvirtual ~Private() = default;\n\nRecently I think we're pushing to move the '= default' declarations to\nthe .cpp files to avoid inlining then in the headers.\n\nOverall I think that's good!\n\nReviewed-by: Jacopo Mondi \n\n>\n> \tvoid setRequest(Request *request) { request_ = request; }\n> \tbool isContiguous() const { return isContiguous_; }\n> diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> index d44a98ba..26587ff3 100644\n> --- a/src/libcamera/framebuffer.cpp\n> +++ b/src/libcamera/framebuffer.cpp\n> @@ -213,8 +213,21 @@ FrameBuffer::Private::Private()\n> * \\param[in] cookie Cookie\n> */\n> FrameBuffer::FrameBuffer(const std::vector &planes, unsigned int cookie)\n> -\t: Extensible(std::make_unique()), planes_(planes),\n> -\t cookie_(cookie)\n> +\t: FrameBuffer(std::make_unique(), planes, cookie)\n> +{\n> +}\n> +\n> +/**\n> + * \\brief Construct a FrameBuffer with an extensible private class and an array\n> + * of planes\n> + * \\param[in] d The extensible private class\n> + * \\param[in] planes The frame memory planes\n> + * \\param[in] cookie Cookie\n> + */\n> +FrameBuffer::FrameBuffer(std::unique_ptr d,\n> +\t\t\t const std::vector &planes,\n> +\t\t\t unsigned int cookie)\n> +\t: Extensible(std::move(d)), planes_(planes), cookie_(cookie)\n> {\n> \tmetadata_.planes_.resize(planes_.size());\n>\n> --\n> 2.33.1.1089.g2158813163f-goog\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 397EABF415\n\tfor ;\n\tFri, 29 Oct 2021 14:39:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9F893600BD;\n\tFri, 29 Oct 2021 16:39:58 +0200 (CEST)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5A4B9600B8\n\tfor ;\n\tFri, 29 Oct 2021 16:39:57 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id A80334000E;\n\tFri, 29 Oct 2021 14:39:56 +0000 (UTC)"],"Date":"Fri, 29 Oct 2021 16:40:46 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20211029144046.337jjp7qebu6t55w@uno.localdomain>","References":"<20211028073038.653786-1-hiroh@chromium.org>\n\t<20211028073038.653786-2-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211028073038.653786-2-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: framebuffer: Enable\n\tattaching additional data to FrameBuffer","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":20644,"web_url":"https://patchwork.libcamera.org/comment/20644/","msgid":"","date":"2021-11-01T04:54:09","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: framebuffer: Enable\n\tattaching additional data to FrameBuffer","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo,\n\nOn Fri, Oct 29, 2021 at 11:39 PM Jacopo Mondi wrote:\n>\n> Hi Hiro,\n>\n> On Thu, Oct 28, 2021 at 04:30:37PM +0900, Hirokazu Honda wrote:\n> > We cannot have a subclass of FrameBuffer because it is marked as final.\n> > This adds a FrameBuffer constructor with FrameBuffer::Private. So we\n> > can attach some additional resources with FrameBuffer through a\n> > customized FrameBuffer::Private class.\n> >\n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > include/libcamera/framebuffer.h | 2 ++\n> > include/libcamera/internal/framebuffer.h | 1 +\n> > src/libcamera/framebuffer.cpp | 17 +++++++++++++++--\n> > 3 files changed, 18 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> > index 7f2f176a..02bf1533 100644\n> > --- a/include/libcamera/framebuffer.h\n> > +++ b/include/libcamera/framebuffer.h\n> > @@ -57,6 +57,8 @@ public:\n> > unsigned int length;\n> > };\n> >\n> > + FrameBuffer(std::unique_ptr d,\n> > + const std::vector &planes, unsigned int cookie = 0);\n>\n> I wonder if it's right to make this available from a public\n> header. We have this pattern in use for Camera, which cannot however be\n> constructed from the libcamera public API, while FrameBuffer can.\n\nCould you elaborate?\nCamera::create() takes Camera::Private and it can be called by\nlibcamera public API.\n\n-Hiro\n>\n> Applications cannot extend FrameBuffer::Private though, so only\n> components with access to the internal headers can, so I guess this is\n> fine!\n>\n>\n> > FrameBuffer(const std::vector &planes, unsigned int cookie = 0);\n>\n> Could you align the declaration order in the .cpp and .h files ?\n>\n> Alternatively, we can decide FrameBuffer can potentially store some\n> PrivateData in its FrameBuffer::Private in the case we ever get to\n> provide an allocator based on dmabuf heaps. But that's maybe for\n> later.\n>\n> >\n> > const std::vector &planes() const { return planes_; }\n> > diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h\n> > index cd33c295..5ab9b3b2 100644\n> > --- a/include/libcamera/internal/framebuffer.h\n> > +++ b/include/libcamera/internal/framebuffer.h\n> > @@ -19,6 +19,7 @@ class FrameBuffer::Private : public Extensible::Private\n> >\n> > public:\n> > Private();\n> > + virtual ~Private() = default;\n>\n> Recently I think we're pushing to move the '= default' declarations to\n> the .cpp files to avoid inlining then in the headers.\n>\n> Overall I think that's good!\n>\n> Reviewed-by: Jacopo Mondi \n>\n> >\n> > void setRequest(Request *request) { request_ = request; }\n> > bool isContiguous() const { return isContiguous_; }\n> > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> > index d44a98ba..26587ff3 100644\n> > --- a/src/libcamera/framebuffer.cpp\n> > +++ b/src/libcamera/framebuffer.cpp\n> > @@ -213,8 +213,21 @@ FrameBuffer::Private::Private()\n> > * \\param[in] cookie Cookie\n> > */\n> > FrameBuffer::FrameBuffer(const std::vector &planes, unsigned int cookie)\n> > - : Extensible(std::make_unique()), planes_(planes),\n> > - cookie_(cookie)\n> > + : FrameBuffer(std::make_unique(), planes, cookie)\n> > +{\n> > +}\n> > +\n> > +/**\n> > + * \\brief Construct a FrameBuffer with an extensible private class and an array\n> > + * of planes\n> > + * \\param[in] d The extensible private class\n> > + * \\param[in] planes The frame memory planes\n> > + * \\param[in] cookie Cookie\n> > + */\n> > +FrameBuffer::FrameBuffer(std::unique_ptr d,\n> > + const std::vector &planes,\n> > + unsigned int cookie)\n> > + : Extensible(std::move(d)), planes_(planes), cookie_(cookie)\n> > {\n> > metadata_.planes_.resize(planes_.size());\n> >\n> > --\n> > 2.33.1.1089.g2158813163f-goog\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 704A0BF415\n\tfor ;\n\tMon, 1 Nov 2021 04:54:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C5773600C3;\n\tMon, 1 Nov 2021 05:54:21 +0100 (CET)","from mail-ed1-x52a.google.com (mail-ed1-x52a.google.com\n\t[IPv6:2a00:1450:4864:20::52a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B5621600B6\n\tfor ;\n\tMon, 1 Nov 2021 05:54:20 +0100 (CET)","by mail-ed1-x52a.google.com with SMTP id s1so60406932edd.3\n\tfor ;\n\tSun, 31 Oct 2021 21:54:20 -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=\"UGooC/rC\"; 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=nGgURuOtxYKqE5nl40NTHTZlcu8+GVZ6eSiFWjkSviA=;\n\tb=UGooC/rCDdmmb0RncULzHUxbsfEpMVHr6rSHkiPgnSjqpr8NKoIZa9Ej961B4gKBvD\n\t9sw4RpdP0djrqzTvPveD6U5rC5cf51wOochbAcQ9PtJK80fcpNeeleQrNJeQhbTA5yOD\n\te0pV+k5WnNiGNQdePx621jxxHu73A85MM7dy8=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=nGgURuOtxYKqE5nl40NTHTZlcu8+GVZ6eSiFWjkSviA=;\n\tb=or/1OpDqi58cES6zNbs8DHp16SXv141hAOh+lMmVPqIgsBZcVhz6rVZ3cd/ZeEN7yn\n\tNU3L0flOrG1DyRv8w6ZbjGIQZ4y5gsqzrjgfVWnNSVp8oLjgYyW8QEw/yAA5gyGEJLh9\n\tn8cuNU+9lIozZ7wP/U/+akIcYGSweR7HFvFVZcO5PxPrLq8YESlkTY7pdhEqB69pXoLW\n\t5dGfr2qVFnjaqcmTmbCm6FHsXYa62rthR0iSL6k0Ws+IzLg7UV4dC/GCBFmEoljvcCvd\n\tiHjlPWylfxxn+V8CRj61YDFP5L1BRbpZ2TCNzRZVjHUAvPTeVrlb66lceHcXbE9QT0JF\n\tPPGw==","X-Gm-Message-State":"AOAM530vX4ymyiRzg0oWePxBjcTXSY56QJJpKe0o9FddP4wLMpgVD6dZ\n\tJgbprs2CAPaV2PSPj6DPPIutirSgOm257UF+xrBGSA==","X-Google-Smtp-Source":"ABdhPJzLr/sWxnlqWHoes8/nuRwIhjdaiNToN26/5TKqEas4+BauiND6x56lEc8tp+LZ4ijjJumeAKj1uuv9P/M7c/8=","X-Received":"by 2002:a50:d80d:: with SMTP id\n\to13mr37463102edj.204.1635742460105; \n\tSun, 31 Oct 2021 21:54:20 -0700 (PDT)","MIME-Version":"1.0","References":"<20211028073038.653786-1-hiroh@chromium.org>\n\t<20211028073038.653786-2-hiroh@chromium.org>\n\t<20211029144046.337jjp7qebu6t55w@uno.localdomain>","In-Reply-To":"<20211029144046.337jjp7qebu6t55w@uno.localdomain>","From":"Hirokazu Honda ","Date":"Mon, 1 Nov 2021 13:54:09 +0900","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: framebuffer: Enable\n\tattaching additional data to FrameBuffer","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":20655,"web_url":"https://patchwork.libcamera.org/comment/20655/","msgid":"<20211102090046.gllcbitl42kgzh3h@uno.localdomain>","date":"2021-11-02T09:00:46","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: framebuffer: Enable\n\tattaching additional data to FrameBuffer","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Mon, Nov 01, 2021 at 01:54:09PM +0900, Hirokazu Honda wrote:\n> Hi Jacopo,\n>\n> On Fri, Oct 29, 2021 at 11:39 PM Jacopo Mondi wrote:\n> >\n> > Hi Hiro,\n> >\n> > On Thu, Oct 28, 2021 at 04:30:37PM +0900, Hirokazu Honda wrote:\n> > > We cannot have a subclass of FrameBuffer because it is marked as final.\n> > > This adds a FrameBuffer constructor with FrameBuffer::Private. So we\n> > > can attach some additional resources with FrameBuffer through a\n> > > customized FrameBuffer::Private class.\n> > >\n> > > Signed-off-by: Hirokazu Honda \n> > > ---\n> > > include/libcamera/framebuffer.h | 2 ++\n> > > include/libcamera/internal/framebuffer.h | 1 +\n> > > src/libcamera/framebuffer.cpp | 17 +++++++++++++++--\n> > > 3 files changed, 18 insertions(+), 2 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> > > index 7f2f176a..02bf1533 100644\n> > > --- a/include/libcamera/framebuffer.h\n> > > +++ b/include/libcamera/framebuffer.h\n> > > @@ -57,6 +57,8 @@ public:\n> > > unsigned int length;\n> > > };\n> > >\n> > > + FrameBuffer(std::unique_ptr d,\n> > > + const std::vector &planes, unsigned int cookie = 0);\n> >\n> > I wonder if it's right to make this available from a public\n> > header. We have this pattern in use for Camera, which cannot however be\n> > constructed from the libcamera public API, while FrameBuffer can.\n>\n> Could you elaborate?\n> Camera::create() takes Camera::Private and it can be called by\n> libcamera public API.\n\nSorry, I wrote the first paragraph, then the second.\n\nCamera::create() cannot be called by application as they don't have\naccess to the full definition of Camera::Private so they cannot extend\nthat\n\n>\n> -Hiro\n> >\n> > Applications cannot extend FrameBuffer::Private though, so only\n> > components with access to the internal headers can, so I guess this is\n> > fine!\n\nThe same happens in your patches for FrameBuffer::Private, so this is\nfine.\n\nSorry, I should have deleted the first paragraph :)\n\n> >\n> >\n> > > FrameBuffer(const std::vector &planes, unsigned int cookie = 0);\n> >\n> > Could you align the declaration order in the .cpp and .h files ?\n> >\n> > Alternatively, we can decide FrameBuffer can potentially store some\n> > PrivateData in its FrameBuffer::Private in the case we ever get to\n> > provide an allocator based on dmabuf heaps. But that's maybe for\n> > later.\n> >\n> > >\n> > > const std::vector &planes() const { return planes_; }\n> > > diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h\n> > > index cd33c295..5ab9b3b2 100644\n> > > --- a/include/libcamera/internal/framebuffer.h\n> > > +++ b/include/libcamera/internal/framebuffer.h\n> > > @@ -19,6 +19,7 @@ class FrameBuffer::Private : public Extensible::Private\n> > >\n> > > public:\n> > > Private();\n> > > + virtual ~Private() = default;\n> >\n> > Recently I think we're pushing to move the '= default' declarations to\n> > the .cpp files to avoid inlining then in the headers.\n> >\n> > Overall I think that's good!\n> >\n> > Reviewed-by: Jacopo Mondi \n> >\n> > >\n> > > void setRequest(Request *request) { request_ = request; }\n> > > bool isContiguous() const { return isContiguous_; }\n> > > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> > > index d44a98ba..26587ff3 100644\n> > > --- a/src/libcamera/framebuffer.cpp\n> > > +++ b/src/libcamera/framebuffer.cpp\n> > > @@ -213,8 +213,21 @@ FrameBuffer::Private::Private()\n> > > * \\param[in] cookie Cookie\n> > > */\n> > > FrameBuffer::FrameBuffer(const std::vector &planes, unsigned int cookie)\n> > > - : Extensible(std::make_unique()), planes_(planes),\n> > > - cookie_(cookie)\n> > > + : FrameBuffer(std::make_unique(), planes, cookie)\n> > > +{\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Construct a FrameBuffer with an extensible private class and an array\n> > > + * of planes\n> > > + * \\param[in] d The extensible private class\n> > > + * \\param[in] planes The frame memory planes\n> > > + * \\param[in] cookie Cookie\n> > > + */\n> > > +FrameBuffer::FrameBuffer(std::unique_ptr d,\n> > > + const std::vector &planes,\n> > > + unsigned int cookie)\n> > > + : Extensible(std::move(d)), planes_(planes), cookie_(cookie)\n> > > {\n> > > metadata_.planes_.resize(planes_.size());\n> > >\n> > > --\n> > > 2.33.1.1089.g2158813163f-goog\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 1089CBDB1C\n\tfor ;\n\tTue, 2 Nov 2021 08:59:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4CF2760156;\n\tTue, 2 Nov 2021 09:59:57 +0100 (CET)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AFB17600B9\n\tfor ;\n\tTue, 2 Nov 2021 09:59:55 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id B4AA4C0015;\n\tTue, 2 Nov 2021 08:59:54 +0000 (UTC)"],"Date":"Tue, 2 Nov 2021 10:00:46 +0100","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20211102090046.gllcbitl42kgzh3h@uno.localdomain>","References":"<20211028073038.653786-1-hiroh@chromium.org>\n\t<20211028073038.653786-2-hiroh@chromium.org>\n\t<20211029144046.337jjp7qebu6t55w@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: framebuffer: Enable\n\tattaching additional data to FrameBuffer","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":20670,"web_url":"https://patchwork.libcamera.org/comment/20670/","msgid":"","date":"2021-11-04T01:23:13","subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: framebuffer: Enable\n\tattaching additional data to FrameBuffer","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo,\n\nOn Tue, Nov 2, 2021 at 5:59 PM Jacopo Mondi wrote:\n>\n> Hi Hiro,\n>\n> On Mon, Nov 01, 2021 at 01:54:09PM +0900, Hirokazu Honda wrote:\n> > Hi Jacopo,\n> >\n> > On Fri, Oct 29, 2021 at 11:39 PM Jacopo Mondi wrote:\n> > >\n> > > Hi Hiro,\n> > >\n> > > On Thu, Oct 28, 2021 at 04:30:37PM +0900, Hirokazu Honda wrote:\n> > > > We cannot have a subclass of FrameBuffer because it is marked as final.\n> > > > This adds a FrameBuffer constructor with FrameBuffer::Private. So we\n> > > > can attach some additional resources with FrameBuffer through a\n> > > > customized FrameBuffer::Private class.\n> > > >\n> > > > Signed-off-by: Hirokazu Honda \n> > > > ---\n> > > > include/libcamera/framebuffer.h | 2 ++\n> > > > include/libcamera/internal/framebuffer.h | 1 +\n> > > > src/libcamera/framebuffer.cpp | 17 +++++++++++++++--\n> > > > 3 files changed, 18 insertions(+), 2 deletions(-)\n> > > >\n> > > > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> > > > index 7f2f176a..02bf1533 100644\n> > > > --- a/include/libcamera/framebuffer.h\n> > > > +++ b/include/libcamera/framebuffer.h\n> > > > @@ -57,6 +57,8 @@ public:\n> > > > unsigned int length;\n> > > > };\n> > > >\n> > > > + FrameBuffer(std::unique_ptr d,\n> > > > + const std::vector &planes, unsigned int cookie = 0);\n> > >\n> > > I wonder if it's right to make this available from a public\n> > > header. We have this pattern in use for Camera, which cannot however be\n> > > constructed from the libcamera public API, while FrameBuffer can.\n> >\n> > Could you elaborate?\n> > Camera::create() takes Camera::Private and it can be called by\n> > libcamera public API.\n>\n> Sorry, I wrote the first paragraph, then the second.\n>\n> Camera::create() cannot be called by application as they don't have\n> access to the full definition of Camera::Private so they cannot extend\n> that\n>\n> >\n> > -Hiro\n> > >\n> > > Applications cannot extend FrameBuffer::Private though, so only\n> > > components with access to the internal headers can, so I guess this is\n> > > fine!\n>\n> The same happens in your patches for FrameBuffer::Private, so this is\n> fine.\n>\n> Sorry, I should have deleted the first paragraph :)\n>\n\nI got it. Thanks for clarifying.\n\n-Hiro\n> > >\n> > >\n> > > > FrameBuffer(const std::vector &planes, unsigned int cookie = 0);\n> > >\n> > > Could you align the declaration order in the .cpp and .h files ?\n> > >\n> > > Alternatively, we can decide FrameBuffer can potentially store some\n> > > PrivateData in its FrameBuffer::Private in the case we ever get to\n> > > provide an allocator based on dmabuf heaps. But that's maybe for\n> > > later.\n> > >\n> > > >\n> > > > const std::vector &planes() const { return planes_; }\n> > > > diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h\n> > > > index cd33c295..5ab9b3b2 100644\n> > > > --- a/include/libcamera/internal/framebuffer.h\n> > > > +++ b/include/libcamera/internal/framebuffer.h\n> > > > @@ -19,6 +19,7 @@ class FrameBuffer::Private : public Extensible::Private\n> > > >\n> > > > public:\n> > > > Private();\n> > > > + virtual ~Private() = default;\n> > >\n> > > Recently I think we're pushing to move the '= default' declarations to\n> > > the .cpp files to avoid inlining then in the headers.\n> > >\n> > > Overall I think that's good!\n> > >\n> > > Reviewed-by: Jacopo Mondi \n> > >\n> > > >\n> > > > void setRequest(Request *request) { request_ = request; }\n> > > > bool isContiguous() const { return isContiguous_; }\n> > > > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> > > > index d44a98ba..26587ff3 100644\n> > > > --- a/src/libcamera/framebuffer.cpp\n> > > > +++ b/src/libcamera/framebuffer.cpp\n> > > > @@ -213,8 +213,21 @@ FrameBuffer::Private::Private()\n> > > > * \\param[in] cookie Cookie\n> > > > */\n> > > > FrameBuffer::FrameBuffer(const std::vector &planes, unsigned int cookie)\n> > > > - : Extensible(std::make_unique()), planes_(planes),\n> > > > - cookie_(cookie)\n> > > > + : FrameBuffer(std::make_unique(), planes, cookie)\n> > > > +{\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\brief Construct a FrameBuffer with an extensible private class and an array\n> > > > + * of planes\n> > > > + * \\param[in] d The extensible private class\n> > > > + * \\param[in] planes The frame memory planes\n> > > > + * \\param[in] cookie Cookie\n> > > > + */\n> > > > +FrameBuffer::FrameBuffer(std::unique_ptr d,\n> > > > + const std::vector &planes,\n> > > > + unsigned int cookie)\n> > > > + : Extensible(std::move(d)), planes_(planes), cookie_(cookie)\n> > > > {\n> > > > metadata_.planes_.resize(planes_.size());\n> > > >\n> > > > --\n> > > > 2.33.1.1089.g2158813163f-goog\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 9DBC1BF415\n\tfor ;\n\tThu, 4 Nov 2021 01:23:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E9FD66033D;\n\tThu, 4 Nov 2021 02:23:25 +0100 (CET)","from mail-ed1-x529.google.com (mail-ed1-x529.google.com\n\t[IPv6:2a00:1450:4864:20::529])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7A271600B8\n\tfor ;\n\tThu, 4 Nov 2021 02:23:23 +0100 (CET)","by mail-ed1-x529.google.com with SMTP id g10so15627998edj.1\n\tfor ;\n\tWed, 03 Nov 2021 18:23:23 -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=\"GhIneg7p\"; 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=KP1sPnywbhUzuHaLjOH24KRsvHjcPzsDIlxQAp0z5ow=;\n\tb=GhIneg7pns2J1VwHVm1Y/JzazIfH04e5fSgZ3HSPXolR4Oe5LpYOdZUE3oL7KnZkdP\n\tWRqWkpf0OsUYXXzFaO/B6P168a3D7bq+y7FrikN4gs5AsKQPefM6U6F8VyPVm7XCY5eB\n\tBD8++ZL+LRIvYxws1eDPJ1ZZXS+/C9EQZxBBo=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=KP1sPnywbhUzuHaLjOH24KRsvHjcPzsDIlxQAp0z5ow=;\n\tb=k62xQ/2jDB7ShKGHLk6JikiK9RgThUnyDrCpJS+I8VlXyz4hRvrH9FIqdaAjErmszG\n\tOyfiUw6FQkz6QTs2kFYElLYdytQwtJwdLQteko6GFzXd+iTQXgV2c9mM2pXuzlgPxSG1\n\tSZyh61MV62XvRRTAK83rrDIHLENUFFfHDTQR/XzSqQDz47J3nXyvgX8LRjXMT+9Ct7nL\n\t50DTUsnezZsY9/ARrkc0J3KZG8VZV3Co3kMRVJ2uIznN1vrLUKANthLY8twMRUSK96Ao\n\tIhG9wk9OC8xbsvkAMWd6XrA6iNO3qy+bupEcafNJfRaeVMRR5aiGqUOcJi5YSH9Tx8DB\n\trKBw==","X-Gm-Message-State":"AOAM530dORI9YB4KKQU+lnF8ruG79dYRTJcxP/03KBDh350vbPGGV9cf\n\tvOab8C0wNVNg4V2Cl8+U9nsy4aMLfp2t9bvTFm7pIQ==","X-Google-Smtp-Source":"ABdhPJwJPHb8FPQa5dd1PZ5ZIteKSZIoJaAzeUCDvuy+J/eb7HfZT9K/x2roYHFEGJ86DyQfXlB2pUXDsd21+Tk5yww=","X-Received":"by 2002:aa7:df8f:: with SMTP id\n\tb15mr52394983edy.202.1635989003028; \n\tWed, 03 Nov 2021 18:23:23 -0700 (PDT)","MIME-Version":"1.0","References":"<20211028073038.653786-1-hiroh@chromium.org>\n\t<20211028073038.653786-2-hiroh@chromium.org>\n\t<20211029144046.337jjp7qebu6t55w@uno.localdomain>\n\t\n\t<20211102090046.gllcbitl42kgzh3h@uno.localdomain>","In-Reply-To":"<20211102090046.gllcbitl42kgzh3h@uno.localdomain>","From":"Hirokazu Honda ","Date":"Thu, 4 Nov 2021 10:23:13 +0900","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 1/2] libcamera: framebuffer: Enable\n\tattaching additional data to FrameBuffer","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\" "}}]