[{"id":22798,"web_url":"https://patchwork.libcamera.org/comment/22798/","msgid":"","date":"2022-04-26T21:55:18","subject":"Re: [libcamera-devel] [PATCH v3 1/4] Allow inheritance of\n\tFrameBuffer","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Harvey,\n\nThank you for the patch.\n\nOn Tue, Apr 26, 2022 at 09:14:06AM +0000, Harvey Yang via libcamera-devel wrote:\n> To add buffer_handle_t access in android, this CL allows inheritance of\n\nWe use the term \"patch\" or \"change\", not \"CL\". Apart from that, the\npatch looks good, assuming review of the subsequent patches don't reveal\nissues that affect this one.\n\nOn a side note, if we allow inheriting from the FrameBuffer class, I\nwonder if we should drop the cookie() and setCookie() functions. Users\nof the class that need to associate private data with it can always\ninherit to extend FrameBuffer. That's a candidate for a separate change\nof course, but what does everybody think ?\n\n> FrameBuffer to add a derived class in android.\n> \n> Signed-off-by: Harvey Yang \n> ---\n> include/libcamera/framebuffer.h | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n> \n> diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> index 3b1118d1..e6c769b4 100644\n> --- a/include/libcamera/framebuffer.h\n> +++ b/include/libcamera/framebuffer.h\n> @@ -46,7 +46,7 @@ private:\n> \tstd::vector planes_;\n> };\n> \n> -class FrameBuffer final : public Extensible\n> +class FrameBuffer : public Extensible\n> {\n> \tLIBCAMERA_DECLARE_PRIVATE()\n> \n> @@ -61,6 +61,7 @@ public:\n> \tFrameBuffer(const std::vector &planes, unsigned int cookie = 0);\n> \tFrameBuffer(std::unique_ptr d,\n> \t\t const std::vector &planes, unsigned int cookie = 0);\n> +\tvirtual ~FrameBuffer() {}\n> \n> \tconst std::vector &planes() const { return planes_; }\n> \tRequest *request() const;","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 0FD88C0F2A\n\tfor ;\n\tTue, 26 Apr 2022 21:55:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 391DD65642;\n\tTue, 26 Apr 2022 23:55:21 +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 300DF60431\n\tfor ;\n\tTue, 26 Apr 2022 23:55:19 +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 8080630B;\n\tTue, 26 Apr 2022 23:55:18 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1651010121;\n\tbh=oq3umi5+BDNEFPURtWLz/c4mn3xMLrWrreSKvzUJ97M=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=P+HSQ0Oamx+PCghFGD7K1PkasVR5xFGC1Zg5zM2gbW+l+mz5xzBL4BM8eiIVXVBM2\n\tpkSRhav4d49/z2JxEiXyF2WtbzHFAWkSgP5kXeT8q16S/HBp9P3/ByRgQxvIJJtNh9\n\tt4bJD0nPiuJqpVmnKuJ+wYjkqx505kzUODGbXqDdBvO8YVEXQrayjzHsTElCUwNo+o\n\tOaDbnn0OY9AGzQsKggqisB+fEGn6XojdBYc43r0RhKv7MWezRO1JkN5me7btxS8oZR\n\tTGx7WZ0heD0GUtSlcPNmvcrJv8ZQsACoak+FgomXsKD9+nx9hSZDDGL6dLuxiwrst9\n\tiCJlLno/7wGVg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1651010118;\n\tbh=oq3umi5+BDNEFPURtWLz/c4mn3xMLrWrreSKvzUJ97M=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=RSv42AkBWxiUmbx5CBfTUyjKMHLk7/nRMpuoslfU5xeEoLmQ7QtafCA4mT4CfMiUs\n\tE3jOijCpYPhMAk+jmCfjiB9zsBlKFJjVh3JDcWRcTYaXBLhyJZJkwZSkm2YOepJ8X6\n\tby9KlCRguZF/IuVLGQkjaG3atK1sCBuPA6qG4mJg="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"RSv42AkB\"; dkim-atps=neutral","Date":"Wed, 27 Apr 2022 00:55:18 +0300","To":"Harvey Yang ","Message-ID":"","References":"<20220426091409.1352047-1-chenghaoyang@chromium.org>\n\t<20220426091409.1352047-2-chenghaoyang@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220426091409.1352047-2-chenghaoyang@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v3 1/4] Allow inheritance of\n\tFrameBuffer","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":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22820,"web_url":"https://patchwork.libcamera.org/comment/22820/","msgid":"","date":"2022-05-03T03:29:40","subject":"Re: [libcamera-devel] [PATCH v3 1/4] Allow inheritance of\n\tFrameBuffer","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Laurent,\n\nSorry I forgot to send this reply before going on vacation...\n\nThanks for your detailed review! I spent quite some time to update my\npatches, so v4 is a bit delayed...\nI used ts=4 in previous patches. Now I use 8 instead, so the new version\nmight look a bit different from the previous ones.\n\nUpdated \"CL\" to \"patch\".\n\nAs for the cookie() and setCookie() functions, I notice that currently\npipeline handlers use them a lot, and src/android/camera_device.cpp relies\non the value (cookie() only, without setCookie(), as the cookie is supposed\nto be used). If it's used under src/android and src/libcamera/pipeline, I\nthink the functions are worth being defined in the base class? I don't\nthink I fully understand the code hierarchy though. What do others think?\n\nBR,\nHarvey\n\nOn Wed, Apr 27, 2022 at 5:55 AM Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Harvey,\n>\n> Thank you for the patch.\n>\n> On Tue, Apr 26, 2022 at 09:14:06AM +0000, Harvey Yang via libcamera-devel\n> wrote:\n> > To add buffer_handle_t access in android, this CL allows inheritance of\n>\n> We use the term \"patch\" or \"change\", not \"CL\". Apart from that, the\n> patch looks good, assuming review of the subsequent patches don't reveal\n> issues that affect this one.\n>\n> On a side note, if we allow inheriting from the FrameBuffer class, I\n> wonder if we should drop the cookie() and setCookie() functions. Users\n> of the class that need to associate private data with it can always\n> inherit to extend FrameBuffer. That's a candidate for a separate change\n> of course, but what does everybody think ?\n>\n> > FrameBuffer to add a derived class in android.\n> >\n> > Signed-off-by: Harvey Yang \n> > ---\n> > include/libcamera/framebuffer.h | 3 ++-\n> > 1 file changed, 2 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/include/libcamera/framebuffer.h\n> b/include/libcamera/framebuffer.h\n> > index 3b1118d1..e6c769b4 100644\n> > --- a/include/libcamera/framebuffer.h\n> > +++ b/include/libcamera/framebuffer.h\n> > @@ -46,7 +46,7 @@ private:\n> > std::vector planes_;\n> > };\n> >\n> > -class FrameBuffer final : public Extensible\n> > +class FrameBuffer : public Extensible\n> > {\n> > LIBCAMERA_DECLARE_PRIVATE()\n> >\n> > @@ -61,6 +61,7 @@ public:\n> > FrameBuffer(const std::vector &planes, unsigned int cookie\n> = 0);\n> > FrameBuffer(std::unique_ptr d,\n> > const std::vector &planes, unsigned int cookie\n> = 0);\n> > + virtual ~FrameBuffer() {}\n> >\n> > const std::vector &planes() const { return planes_; }\n> > Request *request() const;\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 52C4FC3256\n\tfor ;\n\tTue, 3 May 2022 03:29:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0DC5C65642;\n\tTue, 3 May 2022 05:29:53 +0200 (CEST)","from mail-lj1-x229.google.com (mail-lj1-x229.google.com\n\t[IPv6:2a00:1450:4864:20::229])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 00BE8604A1\n\tfor ;\n\tTue, 3 May 2022 05:29:52 +0200 (CEST)","by mail-lj1-x229.google.com with SMTP id s27so20601668ljd.2\n\tfor ;\n\tMon, 02 May 2022 20:29:51 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1651548593;\n\tbh=O3vMa1GSuwsnnT0Z0JSJRhIfC0bdVw5O7a6094fGYfo=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=oxbTbu1YbSZ4w+U0TEEz51mxJwiWavpBhhALyJIhP5aeLsrRd7BHryq8veVS2mWWO\n\tYsTzoy6vZ2ymYvZQYmwcp+hOxkeQKdqlURViapP3iyAZRLvWcXhvfcBUUqegKrj8i7\n\tPOxBnu3HQAKRCKH6eSlEgLk5tHH6lgEMWm5p+EZPUvxPsN/imrE6fQt3/O82eyexY6\n\tG1CBU6+zrON/by+cD1lsEkEVfjvUcuWC6oy+OBM4aXDVz1UKYF99obYkKFE/530Mfx\n\trbcn11+AGL22yDPRX4AKUm/nXYO5FkUkFr6Fct2hZV5ho+9syZHep++RvGY9KOiIa6\n\t0he6nvf0Riuww==","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=zHTINQ40nI3TEQHENQDFQRgQoXmCBod4As6Ms7PdiDY=;\n\tb=jcDRkQrG+edBqmGx7JMvvddb9LK4lqbrq75BSa3yfA5A+PBYZl/EU3GFX64y4RjEOj\n\t7tcg636X67WvPbf1vFd9dqwb9aDluOKX+r9dRH8riBFP16VHAHFSlNMFo6XJDxt2KaqI\n\tzmnqqYDcSTjKZzIVV6M5asEWZoxceorRTGJx4="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=chromium.org\n\theader.i=@chromium.org header.b=\"jcDRkQrG\"; \n\tdkim-atps=neutral","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=zHTINQ40nI3TEQHENQDFQRgQoXmCBod4As6Ms7PdiDY=;\n\tb=ejUmOWWQrQzcJ7pvkmh0e2Dfc93xY6sEk4yJQh2T4h8opdk6hTErPWiqUFQZ4S78mn\n\tSAP+HdxGBazBZlZk/8i9J5eiBBi0iXdkaSeCywN7y1yX6uZAD98iXNMEolnx9rJ4/+47\n\ttcAbyaL5j2NIbTzBFEBtFyTAvZa0WALA606vysD6ZpwqCp38F1NMjtoWbQnV16M6DM+1\n\tjAVxnGqYRE9MakEzZBxLMUeNe++nby5rijDVTk00IIPJ2nmrzoxXzPCYSfZOnOfAdpeN\n\tsqi26ce6WXVzYnOI76LcDYomZDYaWC2X4vdP5NRn30d3OOUr0W545OmXK0RZxGzZAkzB\n\tNQ3Q==","X-Gm-Message-State":"AOAM533C4l7jbcNTE5QolvGXPr+1jqnoNO2IjYTwmD+7MuwGculEt/L/\n\tzKvSLaxOtCpRnsk4RmHkCQgcjqlgDh/TobdxZ1ldLA2r0PuESA==","X-Google-Smtp-Source":"ABdhPJx0SsXUIcvvMAY6xTR+fF0oAgWBRg2IC7LdyuXTLjOmDjTmE17NEzgt8MyhCpD1pZkKuCN6oPh6wNZCBAy+OUs=","X-Received":"by 2002:a05:651c:a09:b0:250:628a:e7d0 with SMTP id\n\tk9-20020a05651c0a0900b00250628ae7d0mr2477177ljq.66.1651548591360;\n\tMon, 02 May 2022 20:29:51 -0700 (PDT)","MIME-Version":"1.0","References":"<20220426091409.1352047-1-chenghaoyang@chromium.org>\n\t<20220426091409.1352047-2-chenghaoyang@chromium.org>\n\t","In-Reply-To":"","Date":"Tue, 3 May 2022 11:29:40 +0800","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"0000000000001e249905de131f5e\"","Subject":"Re: [libcamera-devel] [PATCH v3 1/4] Allow inheritance of\n\tFrameBuffer","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":"Cheng-Hao Yang via libcamera-devel\n\t","Reply-To":"Cheng-Hao Yang ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24886,"web_url":"https://patchwork.libcamera.org/comment/24886/","msgid":"","date":"2022-09-01T13:42:46","subject":"Re: [libcamera-devel] [PATCH v3 1/4] Allow inheritance of\n\tFrameBuffer","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Harvey,\n\nReviving an old discussion.\n\nOn Tue, May 03, 2022 at 11:29:40AM +0800, Cheng-Hao Yang wrote:\n> Hi Laurent,\n> \n> Sorry I forgot to send this reply before going on vacation...\n> \n> Thanks for your detailed review! I spent quite some time to update my\n> patches, so v4 is a bit delayed...\n> I used ts=4 in previous patches. Now I use 8 instead, so the new version\n> might look a bit different from the previous ones.\n> \n> Updated \"CL\" to \"patch\".\n> \n> As for the cookie() and setCookie() functions, I notice that currently\n> pipeline handlers use them a lot, and src/android/camera_device.cpp relies\n> on the value (cookie() only, without setCookie(), as the cookie is supposed\n> to be used). If it's used under src/android and src/libcamera/pipeline, I\n> think the functions are worth being defined in the base class? I don't\n> think I fully understand the code hierarchy though. What do others think?\n\nThe purpose of the FrameBuffer and Request cookies is to associate\nprivate data with an instance of those classes.\n\nThe cookie is clearly documented as being opaque to libcamera, and set\nby the creator of the Request or FrameBuffer. For the former, the\ncreator is an application, for the latter, it can be an application, or\nan internal component inside libcamera (IPA modules and pipeline\nhandlers are mentioned in the documentation, adaptation layers such as\nthe Android camera HAL or the V4L2 adaptation layer cound as\napplications from that point of view).\n\nI think this may be a bit of an API design mistake. Generally speaking,\nI don't think libcamera should try to imagine ancillary application\nneeds and provide support for them, in this specific case I think it\nshould be handled by applications (a trivial option would be a\nstd::map or std::map).\n\nFor frame buffers, I would however differentiate between data that is\nneeded by the user of the frame buffer, and data that is needed by the\nimplementor of the frame buffer. An example of the former would be\napplications mapping the frame buffer memory to the CPU and wanting to\nassociate that mapping with the FrameBuffer object for easy lookup. That\nis something that I believe should be handled by the user itself (again,\nmaybe with a std::map). Data used by the implementor of the frame buffer\nis what you're after here, you're creating frame buffers of a specific\nkind, and thus want to inherit from them to store additional data in the\ninstance itself. Cookies are not needed for this.\n\nThe bottom line is that I don't think cookies are actually needed.\n\nThis being said, this particular patch changes the meaning of the\nFrameBuffer class quite fundamentally. FrameBuffer, so far, has been an\nobject that groups together data representing a frame buffe, but an\ninstance of the class *isn't* a frame buffer. The distinction is\nimportant, the frame buffer is the memory in which frames are stored,\nand the FrameBuffer class stores handles to that memory, but it doesn't\nmanage the memory. When using the FrameBufferAllocator this line is\nblurred, as the FrameBuffer instances end up storing the only instance\nof the dmabuf fds, so when a FrameBuffer is deleted, the memory is\nfreed. This may have been a design mistake, I'm not entirely sure, but\nin any case, inheriting from FrameBuffer would allow creating frame\nbuffer objects that *are* a frame buffer, in addition to being used as\nhandles for frame buffers. I think it's an interesting change, but I'm\nnot sure to fully see all the implications this will have on the API.\n\n> On Wed, Apr 27, 2022 at 5:55 AM Laurent Pinchart wrote:\n> > On Tue, Apr 26, 2022 at 09:14:06AM +0000, Harvey Yang via libcamera-devel wrote:\n> > > To add buffer_handle_t access in android, this CL allows inheritance of\n> >\n> > We use the term \"patch\" or \"change\", not \"CL\". Apart from that, the\n> > patch looks good, assuming review of the subsequent patches don't reveal\n> > issues that affect this one.\n> >\n> > On a side note, if we allow inheriting from the FrameBuffer class, I\n> > wonder if we should drop the cookie() and setCookie() functions. Users\n> > of the class that need to associate private data with it can always\n> > inherit to extend FrameBuffer. That's a candidate for a separate change\n> > of course, but what does everybody think ?\n> >\n> > > FrameBuffer to add a derived class in android.\n> > >\n> > > Signed-off-by: Harvey Yang \n> > > ---\n> > > include/libcamera/framebuffer.h | 3 ++-\n> > > 1 file changed, 2 insertions(+), 1 deletion(-)\n> > >\n> > > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> > > index 3b1118d1..e6c769b4 100644\n> > > --- a/include/libcamera/framebuffer.h\n> > > +++ b/include/libcamera/framebuffer.h\n> > > @@ -46,7 +46,7 @@ private:\n> > > std::vector planes_;\n> > > };\n> > >\n> > > -class FrameBuffer final : public Extensible\n> > > +class FrameBuffer : public Extensible\n> > > {\n> > > LIBCAMERA_DECLARE_PRIVATE()\n> > >\n> > > @@ -61,6 +61,7 @@ public:\n> > > FrameBuffer(const std::vector &planes, unsigned int cookie = 0);\n> > > FrameBuffer(std::unique_ptr d,\n> > > const std::vector &planes, unsigned int cookie = 0);\n> > > + virtual ~FrameBuffer() {}\n> > >\n> > > const std::vector &planes() const { return planes_; }\n> > > Request *request() const;","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 797E8C3272\n\tfor ;\n\tThu, 1 Sep 2022 13:43:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C2E1361FD4;\n\tThu, 1 Sep 2022 15:43:00 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0216061FB9\n\tfor ;\n\tThu, 1 Sep 2022 15:42:58 +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 454BB6CD;\n\tThu, 1 Sep 2022 15:42:58 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1662039780;\n\tbh=UBV43irKC9K5sH3OPFDMwJZlpWb+OsFWFbIcNUKT5ec=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=CNs8edNqqwDadGUm9qcItaSYddnmkhdTtU53ukT5BrnDiDaSAbzLH5dl2oPZbaCYG\n\tHhXmrPw/xSJht0L6k2/620vwmD1XUQ+tCvcxNF3NMLEuLGojJ68AxGTAJ5YJVvyxzf\n\tyccECteDIUFFWMEr7EVplt1mh7r79NkELWTaaz29XREgv32E735As3Fhf5aGTAwoen\n\tyY1htzz6p+D8FXGR3s0LJG5xnRoLN6WBNcvonXME6FUH+cNEwiU+zc9Ev/UhLzEqbI\n\tCRxYTmDpZYDmPsnbnBkRhM1+UTgzyQYyd0vNC0Z68g/kFZb9kyPvjo+V6LD+zDq+Vx\n\tne0IMegF4qmyg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1662039778;\n\tbh=UBV43irKC9K5sH3OPFDMwJZlpWb+OsFWFbIcNUKT5ec=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=r3NmS7jFLb3BpTcYibjAENdeYd5REqQ08MmJ6/iWRCxDtfm7dMBCFCEheXOtzaVPv\n\tiCNwHrDwpCDQUA1eZ+bu2ZXaTWf2fgILcy5r0JLF04fLoCYYzZtwRNQpFie4Y9GURt\n\ttrQsfKZudAsrnAsREoUFj+fMbNg5tAU2Gzb0VS04="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"r3NmS7jF\"; dkim-atps=neutral","Date":"Thu, 1 Sep 2022 16:42:46 +0300","To":"Cheng-Hao Yang ","Message-ID":"","References":"<20220426091409.1352047-1-chenghaoyang@chromium.org>\n\t<20220426091409.1352047-2-chenghaoyang@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] [PATCH v3 1/4] Allow inheritance of\n\tFrameBuffer","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":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24893,"web_url":"https://patchwork.libcamera.org/comment/24893/","msgid":"","date":"2022-09-02T10:48:08","subject":"Re: [libcamera-devel] [PATCH v3 1/4] Allow inheritance of\n\tFrameBuffer","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Thanks Laurent for the clear explanation!\n\nOn Thu, Sep 1, 2022 at 9:42 PM Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Harvey,\n>\n> Reviving an old discussion.\n>\n> On Tue, May 03, 2022 at 11:29:40AM +0800, Cheng-Hao Yang wrote:\n> > Hi Laurent,\n> >\n> > Sorry I forgot to send this reply before going on vacation...\n> >\n> > Thanks for your detailed review! I spent quite some time to update my\n> > patches, so v4 is a bit delayed...\n> > I used ts=4 in previous patches. Now I use 8 instead, so the new version\n> > might look a bit different from the previous ones.\n> >\n> > Updated \"CL\" to \"patch\".\n> >\n> > As for the cookie() and setCookie() functions, I notice that currently\n> > pipeline handlers use them a lot, and src/android/camera_device.cpp\n> relies\n> > on the value (cookie() only, without setCookie(), as the cookie is\n> supposed\n> > to be used). If it's used under src/android and src/libcamera/pipeline, I\n> > think the functions are worth being defined in the base class? I don't\n> > think I fully understand the code hierarchy though. What do others think?\n>\n> The purpose of the FrameBuffer and Request cookies is to associate\n> private data with an instance of those classes.\n>\n> The cookie is clearly documented as being opaque to libcamera, and set\n> by the creator of the Request or FrameBuffer. For the former, the\n> creator is an application, for the latter, it can be an application, or\n> an internal component inside libcamera (IPA modules and pipeline\n> handlers are mentioned in the documentation, adaptation layers such as\n> the Android camera HAL or the V4L2 adaptation layer cound as\n> applications from that point of view).\n>\n> I think this may be a bit of an API design mistake. Generally speaking,\n> I don't think libcamera should try to imagine ancillary application\n> needs and provide support for them, in this specific case I think it\n> should be handled by applications (a trivial option would be a\n> std::map or std::map).\n>\n> For frame buffers, I would however differentiate between data that is\n> needed by the user of the frame buffer, and data that is needed by the\n> implementor of the frame buffer. An example of the former would be\n> applications mapping the frame buffer memory to the CPU and wanting to\n> associate that mapping with the FrameBuffer object for easy lookup. That\n> is something that I believe should be handled by the user itself (again,\n> maybe with a std::map). Data used by the implementor of the frame buffer\n> is what you're after here, you're creating frame buffers of a specific\n> kind, and thus want to inherit from them to store additional data in the\n> instance itself. Cookies are not needed for this.\n>\n> The bottom line is that I don't think cookies are actually needed.\n>\n> This being said, this particular patch changes the meaning of the\n> FrameBuffer class quite fundamentally. FrameBuffer, so far, has been an\n> object that groups together data representing a frame buffe, but an\n> instance of the class *isn't* a frame buffer. The distinction is\n> important, the frame buffer is the memory in which frames are stored,\n> and the FrameBuffer class stores handles to that memory, but it doesn't\n> manage the memory. When using the FrameBufferAllocator this line is\n> blurred, as the FrameBuffer instances end up storing the only instance\n> of the dmabuf fds, so when a FrameBuffer is deleted, the memory is\n> freed. This may have been a design mistake, I'm not entirely sure, but\n> in any case, inheriting from FrameBuffer would allow creating frame\n> buffer objects that *are* a frame buffer, in addition to being used as\n> handles for frame buffers. I think it's an interesting change, but I'm\n> not sure to fully see all the implications this will have on the API.\n>\n>\nI agree with you that the purpose of the data \"cookie\" in base classes\nRequest and FrameBuffer is unclear, and it's not used by other base\nclasses. Android adapter even stores the memory address of\nCamera3RequestDescriptor into a Request's cookie.\n\nA pretty common use case of FrameBuffer's cookie is the buffer id\nidentified between the pipeline handler and IPA, which is defined as\nlibcamera::IPABuffer::id. However, we should specifically define\nFrameBuffer's member variable \"id\", instead of utilizing the opaque member\nvariable \"cookie\". Or we can simply leave it to pipeline handlers to store\nthe mappings with something like std::map.\n\nIf others also agree with this, I can help with the migration and remove\nthe member variable \"cookie\" in Request & FrameBuffer for good :)\n\n\n> > On Wed, Apr 27, 2022 at 5:55 AM Laurent Pinchart wrote:\n> > > On Tue, Apr 26, 2022 at 09:14:06AM +0000, Harvey Yang via\n> libcamera-devel wrote:\n> > > > To add buffer_handle_t access in android, this CL allows inheritance\n> of\n> > >\n> > > We use the term \"patch\" or \"change\", not \"CL\". Apart from that, the\n> > > patch looks good, assuming review of the subsequent patches don't\n> reveal\n> > > issues that affect this one.\n> > >\n> > > On a side note, if we allow inheriting from the FrameBuffer class, I\n> > > wonder if we should drop the cookie() and setCookie() functions. Users\n> > > of the class that need to associate private data with it can always\n> > > inherit to extend FrameBuffer. That's a candidate for a separate change\n> > > of course, but what does everybody think ?\n> > >\n> > > > FrameBuffer to add a derived class in android.\n> > > >\n> > > > Signed-off-by: Harvey Yang \n> > > > ---\n> > > > include/libcamera/framebuffer.h | 3 ++-\n> > > > 1 file changed, 2 insertions(+), 1 deletion(-)\n> > > >\n> > > > diff --git a/include/libcamera/framebuffer.h\n> b/include/libcamera/framebuffer.h\n> > > > index 3b1118d1..e6c769b4 100644\n> > > > --- a/include/libcamera/framebuffer.h\n> > > > +++ b/include/libcamera/framebuffer.h\n> > > > @@ -46,7 +46,7 @@ private:\n> > > > std::vector planes_;\n> > > > };\n> > > >\n> > > > -class FrameBuffer final : public Extensible\n> > > > +class FrameBuffer : public Extensible\n> > > > {\n> > > > LIBCAMERA_DECLARE_PRIVATE()\n> > > >\n> > > > @@ -61,6 +61,7 @@ public:\n> > > > FrameBuffer(const std::vector &planes, unsigned int\n> cookie = 0);\n> > > > FrameBuffer(std::unique_ptr d,\n> > > > const std::vector &planes, unsigned int\n> cookie = 0);\n> > > > + virtual ~FrameBuffer() {}\n> > > >\n> > > > const std::vector &planes() const { return planes_; }\n> > > > Request *request() const;\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 62ED9C0DA4\n\tfor ;\n\tFri, 2 Sep 2022 10:48:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C5AD1603E1;\n\tFri, 2 Sep 2022 12:48:20 +0200 (CEST)","from mail-wr1-x42e.google.com (mail-wr1-x42e.google.com\n\t[IPv6:2a00:1450:4864:20::42e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5A34F603E1\n\tfor ;\n\tFri, 2 Sep 2022 12:48:19 +0200 (CEST)","by mail-wr1-x42e.google.com with SMTP id v16so1780065wrm.8\n\tfor ;\n\tFri, 02 Sep 2022 03:48:19 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1662115700;\n\tbh=Uh6twCK4NRotc17YgPlUBj2br12otorDveTiko32U3g=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=YY2ZkCpC/qBdDSdYJRBuAeVkhbqkB0AMT7wez/eNMAtrxMJLYJtTctqPqfII646r4\n\tQ/2O+bJH/XbBIAynQD4Ao/EVSTvpJ46lqa0mnyaoBrEvYImqDW+agrW08UgghL17Pp\n\tzVD3USCMd0QqDR3zDeHpV+0YcUvfieQKBF/RIKcWcalwDVcXbS24+EkCh0S3iOhvjJ\n\tSO6Q9yLcbXbjketWJ+5gTD+m4Ex4ZAsB7GuG4+E2SBs/8Ph0aMIi+vjntL581SWQBV\n\ts7kBpXVyZNb4Ke0sYoDAlPiXgpPUKeQr8O1llzXvtKKBGLOXWgV9k/+kyg5DIYJvqt\n\te+qWSO5ztPsFg==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date;\n\tbh=c24Kp5PaGSiJTz5NeL5tPKdytgdv7eyKYEK4iEvRSrA=;\n\tb=AbelOpplHzf22+wA+srQx0BT/c8Nzfug/ZHbWmCsb1t8UD1lUXDg36+xchwPndiDsF\n\tTTxQKKyQ4f1rDXktZ3ri9AJjLUU51Me7gCHtjVaBsF4iDKDNVugp8bbWLKFOzYcXyZIZ\n\t60Un9KCKXt1boyMq6QRFuOu5BGS8J2avPid/k="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=chromium.org\n\theader.i=@chromium.org header.b=\"AbelOppl\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date;\n\tbh=c24Kp5PaGSiJTz5NeL5tPKdytgdv7eyKYEK4iEvRSrA=;\n\tb=leMZr5LQRRpbKJMcfG4mB4U6PhBB6jlv0BkL6cvg5g65YG4UaL9RtUC/uDSpE/3xH4\n\toSKthvwAh9XV4P5XNsh82CTgjfmSa6cL3/46iA7QToQ2ketMDYwSlAYz9BSTyKaI+tbr\n\tqohNK4zQGbHQUyLgQQ1SDeqLqsJnsOx7VmwT6/O2hNALEX2mInYN6CGbtERkyyRjPRYO\n\tzSnGafJPgBEeEPz6SyiEb2QTMjkPkTsnzvrjbpNnUY3VbZ6toEhEB4WWt1LXd5jLE3jV\n\t/A51wy9bMdtgGfKQfITwgAiZCuqDYwt7vjbHZy75acYSkIhdaf3Tb4MsAzN8lCTuwBAm\n\t0BvQ==","X-Gm-Message-State":"ACgBeo0mlpJYwDisxPyUaQYJJG0lE8ymS0QtxJMKeMdHM6f36EubvPfv\n\tZkSWog+zR+feZnOrhGsU7f7MXPMCfxTAMAw+jkh5cb4YzZ0/Zg==","X-Google-Smtp-Source":"AA6agR6zbDgITtmEB/ORTg+ckxorqIdSmlKD0mKtsXHTHjMc1XZVEZN4hAVWkkeqm1sdugEXaNieLgodddKvGb+7qk4=","X-Received":"by 2002:a5d:6d07:0:b0:220:68a1:9ecb with SMTP id\n\te7-20020a5d6d07000000b0022068a19ecbmr17869534wrq.116.1662115698976;\n\tFri, 02 Sep 2022 03:48:18 -0700 (PDT)","MIME-Version":"1.0","References":"<20220426091409.1352047-1-chenghaoyang@chromium.org>\n\t<20220426091409.1352047-2-chenghaoyang@chromium.org>\n\t\n\t\n\t","In-Reply-To":"","Date":"Fri, 2 Sep 2022 18:48:08 +0800","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000d042d905e7af77b3\"","Subject":"Re: [libcamera-devel] [PATCH v3 1/4] Allow inheritance of\n\tFrameBuffer","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":"Cheng-Hao Yang via libcamera-devel\n\t","Reply-To":"Cheng-Hao Yang ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]