[{"id":26010,"web_url":"https://patchwork.libcamera.org/comment/26010/","msgid":"","date":"2022-12-07T03:20:31","subject":"Re: [libcamera-devel] [PATCH v7 1/6] 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 Thu, Dec 01, 2022 at 09:27:28AM +0000, Harvey Yang via libcamera-devel wrote:\n> From: Harvey Yang \n> \n> To add buffer_handle_t access in android, this patch allows inheritance\n> of FrameBuffer to add a derived class in android.\n> \n> Signed-off-by: Harvey Yang \n\nAssuming the other patches in the series that require this change are\nfine,\n\nReviewed-by: Laurent Pinchart \n\nI'm interested to see where this will lead us, as it's an interesting\nsemantics change. I'll copy here the comments I made in the review of\nv3 if anyone is interested in studying the philosophy of libcamera:\n\nThis particular patch changes the meaning of the FrameBuffer class quite\nfundamentally. FrameBuffer, so far, has been an object that groups\ntogether data representing a frame buffe, but an instance of the class\n*isn't* a frame buffer. The distinction is important, the frame buffer\nis the memory in which frames are stored, and the FrameBuffer class\nstores handles to that memory, but it doesn't manage the memory. When\nusing the FrameBufferAllocator this line is blurred, as the FrameBuffer\ninstances end up storing the only instance of the dmabuf fds, so when a\nFrameBuffer is deleted, the memory is freed. This may have been a design\nmistake, I'm not entirely sure, but in any case, inheriting from\nFrameBuffer would allow creating frame buffer objects that *are* a frame\nbuffer, in addition to being used as handles for frame buffers. I think\nit's an interesting change, but I'm not sure to fully see all the\nimplications this will have on the API.\n\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 69553999..61244829 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> @@ -60,6 +60,7 @@ public:\n> \n> \tFrameBuffer(const std::vector &planes, unsigned int cookie = 0);\n> \tFrameBuffer(std::unique_ptr d);\n> +\tvirtual ~FrameBuffer() {}\n> \n> \tconst std::vector &planes() const;\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 D3439BDE6B\n\tfor ;\n\tWed, 7 Dec 2022 03:20:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 05BA66333F;\n\tWed, 7 Dec 2022 04:20:36 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 45D9061F22\n\tfor ;\n\tWed, 7 Dec 2022 04:20:35 +0100 (CET)","from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi\n\t[213.243.189.158])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9AFDF3D7;\n\tWed, 7 Dec 2022 04:20:34 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1670383236;\n\tbh=iVp0E18iOD5ppV5dp9uqIIfxkWqzSb5VM2xzyuWRYvM=;\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=V5taDycJ6ArwITdGTKWEUU7XNJFA9JReuqkGRhyVqP+54geEhM78/IfTiUIDiz0mC\n\t+G/WNkOfFsXETD/rlNPL0826SVeMtYKkF13GFx9Jnqi9REwPsQDGiBaSoDMf8z/OrA\n\teTZNq0oN3fgrJsjhYO2ZXF3VO/h3Ki83rXk7DtlhVfRU7EIpRZOUz4p/m+2OZbJrH8\n\tC4EB5VsHvbziiRV8vlv04QVokh8j4qjjuNEXHbNzni1gvrxBPZwcffmMWRWNiLOWjZ\n\tNjvODEE0a8mACnKazrSB0paa8r+MI38ys9VRg4ELN39HY1Q8Z7+0+OvN3+elnE6axQ\n\t2nJdBXYCEqDvQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1670383234;\n\tbh=iVp0E18iOD5ppV5dp9uqIIfxkWqzSb5VM2xzyuWRYvM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=sXhloCAQ4xMrMDdOyoI+QXQhohas4qwLKGHz6X+AL7rpg/rywE+7rpyHgob3vtcJY\n\tnEM+8XvHxzuBzrkFy/5lcaqH3PthlJlM5v3Zdmcc5g2izMJNHaCP+TSImIZ1fMU+Df\n\tOAhKxEcYsSQ9VoQY4rqYYGqQO8GMGTOuDJ0CCYyg="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"sXhloCAQ\"; dkim-atps=neutral","Date":"Wed, 7 Dec 2022 05:20:31 +0200","To":"Harvey Yang ","Message-ID":"","References":"<20221201092733.2042078-1-chenghaoyang@google.com>\n\t<20221201092733.2042078-2-chenghaoyang@google.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221201092733.2042078-2-chenghaoyang@google.com>","Subject":"Re: [libcamera-devel] [PATCH v7 1/6] 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\" "}}]