[{"id":23619,"web_url":"https://patchwork.libcamera.org/comment/23619/","msgid":"<9abae4d23a766feba5893d92a59b9eb46947c024.camel@collabora.com>","date":"2022-06-27T20:44:23","subject":"Re: [libcamera-devel] [PATCH 02/13] gstreamer: Inline\n\tgst_libcamera_buffer_get_frame_buffer()","submitter":{"id":31,"url":"https://patchwork.libcamera.org/api/people/31/","name":"Nicolas Dufresne","email":"nicolas.dufresne@collabora.com"},"content":"Hi Laurent,\n\nthanks for the patch.\n\nLe vendredi 24 juin 2022 à 02:21 +0300, Laurent Pinchart a écrit :\n> The gst_libcamera_buffer_get_frame_buffer() function retrieves a\n> FrameBuffer corresponding to the GstBuffer. While the GstLibcameraPool\n> class allocates the GstBuffer instances, associating them to a\n> FrameBuffer is handled by GstLibcameraAllocator. The\n> gst_libcamera_buffer_get_frame_buffer() function is ill-placed in\n> gstlibcamerapool.cpp. Move it to gstlibcameraallocator.cpp, which allows\n> inlining it in its single caller, simplifying the code flow.\n\nEncapsulation wise, I think it was more appropriate before this change. The\ndetails that the FrameBuffer is stored in GstMemory as a Quark data, is internal\nimplementation of the allocator class, while the detail of the GstMemory being\nonly memory from our allocator is internal implementation detail of the pool\nclass (since the pool is the buffer factory.\n\nAs you are seeking for an optimized call stack, you could perhaps must inline\nthese two helpers into their respective headers ?\n\nregards,\nNicolas\n\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> src/gstreamer/gstlibcameraallocator.cpp | 3 ++-\n> src/gstreamer/gstlibcameraallocator.h | 2 +-\n> src/gstreamer/gstlibcamerapool.cpp | 7 -------\n> src/gstreamer/gstlibcamerapool.h | 2 --\n> 4 files changed, 3 insertions(+), 11 deletions(-)\n> \n> diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp\n> index c740b8fc82a8..53693834eeff 100644\n> --- a/src/gstreamer/gstlibcameraallocator.cpp\n> +++ b/src/gstreamer/gstlibcameraallocator.cpp\n> @@ -252,8 +252,9 @@ gst_libcamera_allocator_get_pool_size(GstLibcameraAllocator *self,\n> }\n> \n> FrameBuffer *\n> -gst_libcamera_memory_get_frame_buffer(GstMemory *mem)\n> +gst_libcamera_buffer_get_frame_buffer(GstBuffer *buffer)\n> {\n> +\tGstMemory *mem = gst_buffer_peek_memory(buffer, 0);\n> \tauto *frame = reinterpret_cast(gst_mini_object_get_qdata(GST_MINI_OBJECT_CAST(mem),\n> \t\t\t\t\t\t\t\t\t FrameWrap::getQuark()));\n> \treturn frame->buffer_;\n> diff --git a/src/gstreamer/gstlibcameraallocator.h b/src/gstreamer/gstlibcameraallocator.h\n> index 0a08c3bb3bbe..b1c038c50257 100644\n> --- a/src/gstreamer/gstlibcameraallocator.h\n> +++ b/src/gstreamer/gstlibcameraallocator.h\n> @@ -28,4 +28,4 @@ bool gst_libcamera_allocator_prepare_buffer(GstLibcameraAllocator *self,\n> gsize gst_libcamera_allocator_get_pool_size(GstLibcameraAllocator *allocator,\n> \t\t\t\t\t libcamera::Stream *stream);\n> \n> -libcamera::FrameBuffer *gst_libcamera_memory_get_frame_buffer(GstMemory *mem);\n> +libcamera::FrameBuffer *gst_libcamera_buffer_get_frame_buffer(GstBuffer *buffer);\n> diff --git a/src/gstreamer/gstlibcamerapool.cpp b/src/gstreamer/gstlibcamerapool.cpp\n> index 1fde42135119..118bc6db7067 100644\n> --- a/src/gstreamer/gstlibcamerapool.cpp\n> +++ b/src/gstreamer/gstlibcamerapool.cpp\n> @@ -140,10 +140,3 @@ gst_libcamera_buffer_get_stream(GstBuffer *buffer)\n> \tauto *self = (GstLibcameraPool *)buffer->pool;\n> \treturn self->stream;\n> }\n> -\n> -FrameBuffer *\n> -gst_libcamera_buffer_get_frame_buffer(GstBuffer *buffer)\n> -{\n> -\tGstMemory *mem = gst_buffer_peek_memory(buffer, 0);\n> -\treturn gst_libcamera_memory_get_frame_buffer(mem);\n> -}\n> diff --git a/src/gstreamer/gstlibcamerapool.h b/src/gstreamer/gstlibcamerapool.h\n> index 05795d21223e..06b38cb296fc 100644\n> --- a/src/gstreamer/gstlibcamerapool.h\n> +++ b/src/gstreamer/gstlibcamerapool.h\n> @@ -26,5 +26,3 @@ GstLibcameraPool *gst_libcamera_pool_new(GstLibcameraAllocator *allocator,\n> libcamera::Stream *gst_libcamera_pool_get_stream(GstLibcameraPool *self);\n> \n> libcamera::Stream *gst_libcamera_buffer_get_stream(GstBuffer *buffer);\n> -\n> -libcamera::FrameBuffer *gst_libcamera_buffer_get_frame_buffer(GstBuffer *buffer);","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 4F42BBE173\n\tfor ;\n\tMon, 27 Jun 2022 20:44:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 168C765635;\n\tMon, 27 Jun 2022 22:44:35 +0200 (CEST)","from madras.collabora.co.uk (madras.collabora.co.uk\n\t[46.235.227.172])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 04C166059B\n\tfor ;\n\tMon, 27 Jun 2022 22:44:33 +0200 (CEST)","from nicolas-tpx395.localdomain (192-222-136-102.qc.cable.ebox.net\n\t[192.222.136.102])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\tkey-exchange X25519 server-signature RSA-PSS (4096 bits))\n\t(No client certificate requested) (Authenticated sender: nicolas)\n\tby madras.collabora.co.uk (Postfix) with ESMTPSA id 41BEC6601667;\n\tMon, 27 Jun 2022 21:44:32 +0100 (BST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1656362675;\n\tbh=V/Tj6JWkbOIxnqlrjBr0HtTmlq464bKEdeaRmlqzwxk=;\n\th=To:Date:In-Reply-To:References:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=nLSEAYAvvwtL2K2zNyX2rGDn41XrVqUahxaMMpZtYc73jl7ieIwR2b80ku3AFoosd\n\th3alZwt3Ft1gaNONzbUT1ptUrKlz8lb4VGSqPUhgqBAJZzSBm9dHswSsI6FRS78yrX\n\tR9DMhj1yWOFUGJ9fxP9G0N65qED13b7sB+SpiqESV7TD1Dv6WuKAva3sD6R8Gbm8fb\n\ts0A+To755Kd2abx2i19iJUiRMBqI5EnO6PD8bUEt9uzGEf6/cINzOwHGuIgOXsG0XW\n\tLvzWGlCGzn9Cvzt0f20kHuFzLQ54nVU4WVM9SeDJZviH1prUtIsRDh9khsiXXvz7ED\n\tCCOC4RrcVk0FQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com;\n\ts=mail; t=1656362672;\n\tbh=V/Tj6JWkbOIxnqlrjBr0HtTmlq464bKEdeaRmlqzwxk=;\n\th=Subject:From:To:Cc:Date:In-Reply-To:References:From;\n\tb=CxdbCwMuaZogGE1du73kfmmp/acTRxG+4cGInSP71nX9HnTznrBN/49tUBj0v41nN\n\tl/di2E641vx7VTpK6RWek+oIiPJMTiONtroOc6NaXr0F4DAkKMA/CFn2F2BOMMUzXE\n\tNeiXwRPTgphJ6PPYtVjuF5/0aIowlomjs+PaMYGMBlpCBSVxLCM6FsbNUbROnZw7WR\n\tlQZHJ5MXM60YLg4/G22KDn5zlgl4fEL1CpADG2MDLm40FxZ8rMBOvxC7XLR0AfnJTG\n\tG37UU0BnUnLvYxlr3VDmFwWKoAtvzeo23oZkRyi2ZdYu2aOyKoQlbO7eUm4C7FPnFR\n\tyXvWFSniysveQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=collabora.com\n\theader.i=@collabora.com\n\theader.b=\"CxdbCwMu\"; dkim-atps=neutral","Message-ID":"<9abae4d23a766feba5893d92a59b9eb46947c024.camel@collabora.com>","To":"Laurent Pinchart , \n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 27 Jun 2022 16:44:23 -0400","In-Reply-To":"<20220623232210.18742-3-laurent.pinchart@ideasonboard.com>","References":"<20220623232210.18742-1-laurent.pinchart@ideasonboard.com>\n\t<20220623232210.18742-3-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","User-Agent":"Evolution 3.44.2 (3.44.2-1.fc36) ","MIME-Version":"1.0","Subject":"Re: [libcamera-devel] [PATCH 02/13] gstreamer: Inline\n\tgst_libcamera_buffer_get_frame_buffer()","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":"Nicolas Dufresne via libcamera-devel\n\t","Reply-To":"Nicolas Dufresne ","Cc":"Vedant Paranjape ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23628,"web_url":"https://patchwork.libcamera.org/comment/23628/","msgid":"","date":"2022-06-27T22:38:07","subject":"Re: [libcamera-devel] [PATCH 02/13] gstreamer: Inline\n\tgst_libcamera_buffer_get_frame_buffer()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Nicolas,\n\nOn Mon, Jun 27, 2022 at 04:44:23PM -0400, Nicolas Dufresne wrote:\n> Hi Laurent,\n> \n> thanks for the patch.\n> \n> Le vendredi 24 juin 2022 à 02:21 +0300, Laurent Pinchart a écrit :\n> > The gst_libcamera_buffer_get_frame_buffer() function retrieves a\n> > FrameBuffer corresponding to the GstBuffer. While the GstLibcameraPool\n> > class allocates the GstBuffer instances, associating them to a\n> > FrameBuffer is handled by GstLibcameraAllocator. The\n> > gst_libcamera_buffer_get_frame_buffer() function is ill-placed in\n> > gstlibcamerapool.cpp. Move it to gstlibcameraallocator.cpp, which allows\n> > inlining it in its single caller, simplifying the code flow.\n> \n> Encapsulation wise, I think it was more appropriate before this change. The\n> details that the FrameBuffer is stored in GstMemory as a Quark data, is internal\n> implementation of the allocator class, while the detail of the GstMemory being\n> only memory from our allocator is internal implementation detail of the pool\n> class (since the pool is the buffer factory.\n> \n> As you are seeking for an optimized call stack, you could perhaps must inline\n> these two helpers into their respective headers ?\n\nI wrote this patch when trying to understand the libcamerasrc code, and\nthought it was first and foremost an improvement in the abstraction,\nwith the nice side effect that the\ngst_libcamera_buffer_get_frame_buffer() function could then be inlined.\nIf you don't think it makes the abstraction better, then I'll drop it,\nthe rest of the series doesn't depend on it. I'd still like to improve\nthe allocator and pool implementation as I find the way they're\nintertwined a bit confusing, but that can be done later, maybe when\nadding a pool of requests.\n\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/gstreamer/gstlibcameraallocator.cpp | 3 ++-\n> > src/gstreamer/gstlibcameraallocator.h | 2 +-\n> > src/gstreamer/gstlibcamerapool.cpp | 7 -------\n> > src/gstreamer/gstlibcamerapool.h | 2 --\n> > 4 files changed, 3 insertions(+), 11 deletions(-)\n> > \n> > diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp\n> > index c740b8fc82a8..53693834eeff 100644\n> > --- a/src/gstreamer/gstlibcameraallocator.cpp\n> > +++ b/src/gstreamer/gstlibcameraallocator.cpp\n> > @@ -252,8 +252,9 @@ gst_libcamera_allocator_get_pool_size(GstLibcameraAllocator *self,\n> > }\n> > \n> > FrameBuffer *\n> > -gst_libcamera_memory_get_frame_buffer(GstMemory *mem)\n> > +gst_libcamera_buffer_get_frame_buffer(GstBuffer *buffer)\n> > {\n> > +\tGstMemory *mem = gst_buffer_peek_memory(buffer, 0);\n> > \tauto *frame = reinterpret_cast(gst_mini_object_get_qdata(GST_MINI_OBJECT_CAST(mem),\n> > \t\t\t\t\t\t\t\t\t FrameWrap::getQuark()));\n> > \treturn frame->buffer_;\n> > diff --git a/src/gstreamer/gstlibcameraallocator.h b/src/gstreamer/gstlibcameraallocator.h\n> > index 0a08c3bb3bbe..b1c038c50257 100644\n> > --- a/src/gstreamer/gstlibcameraallocator.h\n> > +++ b/src/gstreamer/gstlibcameraallocator.h\n> > @@ -28,4 +28,4 @@ bool gst_libcamera_allocator_prepare_buffer(GstLibcameraAllocator *self,\n> > gsize gst_libcamera_allocator_get_pool_size(GstLibcameraAllocator *allocator,\n> > \t\t\t\t\t libcamera::Stream *stream);\n> > \n> > -libcamera::FrameBuffer *gst_libcamera_memory_get_frame_buffer(GstMemory *mem);\n> > +libcamera::FrameBuffer *gst_libcamera_buffer_get_frame_buffer(GstBuffer *buffer);\n> > diff --git a/src/gstreamer/gstlibcamerapool.cpp b/src/gstreamer/gstlibcamerapool.cpp\n> > index 1fde42135119..118bc6db7067 100644\n> > --- a/src/gstreamer/gstlibcamerapool.cpp\n> > +++ b/src/gstreamer/gstlibcamerapool.cpp\n> > @@ -140,10 +140,3 @@ gst_libcamera_buffer_get_stream(GstBuffer *buffer)\n> > \tauto *self = (GstLibcameraPool *)buffer->pool;\n> > \treturn self->stream;\n> > }\n> > -\n> > -FrameBuffer *\n> > -gst_libcamera_buffer_get_frame_buffer(GstBuffer *buffer)\n> > -{\n> > -\tGstMemory *mem = gst_buffer_peek_memory(buffer, 0);\n> > -\treturn gst_libcamera_memory_get_frame_buffer(mem);\n> > -}\n> > diff --git a/src/gstreamer/gstlibcamerapool.h b/src/gstreamer/gstlibcamerapool.h\n> > index 05795d21223e..06b38cb296fc 100644\n> > --- a/src/gstreamer/gstlibcamerapool.h\n> > +++ b/src/gstreamer/gstlibcamerapool.h\n> > @@ -26,5 +26,3 @@ GstLibcameraPool *gst_libcamera_pool_new(GstLibcameraAllocator *allocator,\n> > libcamera::Stream *gst_libcamera_pool_get_stream(GstLibcameraPool *self);\n> > \n> > libcamera::Stream *gst_libcamera_buffer_get_stream(GstBuffer *buffer);\n> > -\n> > -libcamera::FrameBuffer *gst_libcamera_buffer_get_frame_buffer(GstBuffer *buffer);","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 E5AAFBE173\n\tfor ;\n\tMon, 27 Jun 2022 22:38:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0532965635;\n\tTue, 28 Jun 2022 00:38:28 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9BE30633A7\n\tfor ;\n\tTue, 28 Jun 2022 00:38:26 +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 F115555A;\n\tTue, 28 Jun 2022 00:38:25 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1656369508;\n\tbh=LrpPVhzPLyRf8h6m0HpALzhi+gIkTXfhDjPuHk2H4bc=;\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=oa8RpyAE44eFPI7TZSijFzQNoFhLGRPbW+TOQSxyLVD3XKh2yh9pDTWJ5RKgCS8h9\n\tXbW2YzdiUT8NoZC23Uv7nFry+MDJ0DCXIxIygejMmQBO/eWviu8BmFb7sL8WKD8OUG\n\tiFdiL2Bs7j7K4SQuV47BTwj/fFSzqosfgWp3jeOmz1j7hJasV50p2ZpblNH7aJvG44\n\tGkFijyXuePecIZhHAqcZvhYtwsocCGBgDkmX5H/vzR8YfIYq8D19YPdSdkJ1GsAaSm\n\tKrk181J1fKyNbPoY+ZUt0C6tX2VY6i1psUE25+uGLQHYnVCiJUnChhG4FyrFlFdLWv\n\tR3V3lMhH8i7kA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1656369506;\n\tbh=LrpPVhzPLyRf8h6m0HpALzhi+gIkTXfhDjPuHk2H4bc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=rUlgSPQZ8rUezGgaMD34dyNOIa637F4VCk8vjpkNwdte0kwJJWAyFI3pouhNkfp1k\n\tDEd//7T0/jT3KUXSQmnVvqaSRKq1S/qDcXjEn92xELboDpfjN1uo5M28YYsSIgZsu7\n\tCcXW4RajkXnEslb3WhPOsBQExwnxvhsgASP27z18="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"rUlgSPQZ\"; dkim-atps=neutral","Date":"Tue, 28 Jun 2022 01:38:07 +0300","To":"Nicolas Dufresne ","Message-ID":"","References":"<20220623232210.18742-1-laurent.pinchart@ideasonboard.com>\n\t<20220623232210.18742-3-laurent.pinchart@ideasonboard.com>\n\t<9abae4d23a766feba5893d92a59b9eb46947c024.camel@collabora.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<9abae4d23a766feba5893d92a59b9eb46947c024.camel@collabora.com>","Subject":"Re: [libcamera-devel] [PATCH 02/13] gstreamer: Inline\n\tgst_libcamera_buffer_get_frame_buffer()","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,\n\tVedant Paranjape ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]