[{"id":12420,"web_url":"https://patchwork.libcamera.org/comment/12420/","msgid":"<20200910111953.GP4095624@oden.dyn.berto.se>","date":"2020-09-10T11:19:53","subject":"Re: [libcamera-devel] [PATCH 3/8] libcamera:\n\tframe_buffer_allocator: Add clear()","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your work.\n\nOn 2020-09-09 17:54:52 +0200, Jacopo Mondi wrote:\n> Add a clear() method to the FrameBufferAllocator class that\n> frees all the buffers previously reserved by the allocator.\n> \n> Signed-off-by: Jacopo Mondi \n\nReviewed-by: Niklas Söderlund \n\n> ---\n> include/libcamera/framebuffer_allocator.h | 1 +\n> src/libcamera/framebuffer_allocator.cpp | 8 ++++++++\n> 2 files changed, 9 insertions(+)\n> \n> diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h\n> index 78f1353964eb..2a4d538a0cb2 100644\n> --- a/include/libcamera/framebuffer_allocator.h\n> +++ b/include/libcamera/framebuffer_allocator.h\n> @@ -28,6 +28,7 @@ public:\n> \n> \tint allocate(Stream *stream);\n> \tint free(Stream *stream);\n> +\tvoid clear();\n> \n> \tbool allocated() const { return !buffers_.empty(); }\n> \tconst std::vector> &buffers(Stream *stream) const;\n> diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp\n> index 2fbba37a1b0b..7ed80011c845 100644\n> --- a/src/libcamera/framebuffer_allocator.cpp\n> +++ b/src/libcamera/framebuffer_allocator.cpp\n> @@ -125,6 +125,14 @@ int FrameBufferAllocator::free(Stream *stream)\n> \treturn 0;\n> }\n> \n> +/**\n> + * \\brief Free all the buffers previously allocated\n> + */\n> +void FrameBufferAllocator::clear()\n> +{\n> +\tbuffers_.clear();\n> +}\n> +\n> /**\n> * \\fn FrameBufferAllocator::allocated()\n> * \\brief Check if the allocator has allocated buffers for any stream\n> -- \n> 2.28.0\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 6008AC3B5B\n\tfor ;\n\tThu, 10 Sep 2020 11:19:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2DC4862D86;\n\tThu, 10 Sep 2020 13:19:56 +0200 (CEST)","from mail-lj1-x242.google.com (mail-lj1-x242.google.com\n\t[IPv6:2a00:1450:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 17B5C62D27\n\tfor ;\n\tThu, 10 Sep 2020 13:19:55 +0200 (CEST)","by mail-lj1-x242.google.com with SMTP id b19so7599258lji.11\n\tfor ;\n\tThu, 10 Sep 2020 04:19:55 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tp4sm1279681lfr.68.2020.09.10.04.19.54\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 10 Sep 2020 04:19:54 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"NVK0Tfc8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=5rEgYhJNc2S7ka8FyYbkxKS2r+32c0PppaFLj/XTYfU=;\n\tb=NVK0Tfc8Pdd2O01gtr26pE7d2Us3mzIpnd2X6pScAh6Qw6nSmOsj0+c9QIpAi6hGKP\n\t+DG4aWifm0D5osW2svV41xj8B3W3+OsK43R4qAhZioOFF3/INYmCfNoba6Qv290oR1kA\n\tLxqPCN4LOmXR6jY+Q6TqJ7A97TfRYmkd/9LSSDQDyFiX0E8fSQo1hgEoRP2Yxcf71mAR\n\tjRO+dgDJ+4/r1shk8SEVU3DKS4a3HaFOp4NWPKWqmZnGvw3okuJ8VXBf4vHN4cfID5t9\n\toXNrIezrgsXF9OkZBOmHFhfxuxhRd0iY5e9b9irEcN/JFIzsn9rs8MexF4eQ12xL5ATw\n\txoFw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=5rEgYhJNc2S7ka8FyYbkxKS2r+32c0PppaFLj/XTYfU=;\n\tb=oarLIEJZCk/oJBkm43VgascpkIBeiNDSQXfuGqAQJ3eeoF92SHMK/WeC/l0dBsDdHk\n\tWZOYi/7T2S4msjPaf6/E3OsbPyYB/1jkdhXuQGmJtk3rUJ96AP60vNiNjzdB2UUhtHI+\n\tSLFzdbk3x2NwyIIOSxxujtmxUK0XG2px8+rHLlwCPPjDs9mJmledinyHaP3zDk5vIB9n\n\tLbo+yHfbG5rbybHgXusFBNsoGU+/c9LRD7u78MIKkagbtBWzLLQMNkCt365AnkQysGdh\n\twmKHOqnF23yeKWa1VvFo707EFPQVS4AQ3U+/2C8cE1pjOA6D83SOv4j7kO5MV5Bd+hJk\n\tbwfQ==","X-Gm-Message-State":"AOAM53005Fd9j+tfYW0bzgCDyjP7Fi9oey2qBp2nKqcYFZyiwy/v1LJh\n\tDqRfCw0YOys5BHl0st6cQFH9yA==","X-Google-Smtp-Source":"ABdhPJw6mJV9OvkHDRklYOGpQS1b1VMT1Pyy/H1L0fxsecld1J8YkKxpq/nt2ixtU5O8BQvWpRG8Sw==","X-Received":"by 2002:a05:651c:124e:: with SMTP id\n\th14mr4028988ljh.384.1599736794590; \n\tThu, 10 Sep 2020 04:19:54 -0700 (PDT)","Date":"Thu, 10 Sep 2020 13:19:53 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"<20200910111953.GP4095624@oden.dyn.berto.se>","References":"<20200909155457.153907-1-jacopo@jmondi.org>\n\t<20200909155457.153907-4-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200909155457.153907-4-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 3/8] libcamera:\n\tframe_buffer_allocator: Add clear()","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":"hanlinchen@chromium.org, libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12444,"web_url":"https://patchwork.libcamera.org/comment/12444/","msgid":"","date":"2020-09-11T04:42:58","subject":"Re: [libcamera-devel] [PATCH 3/8] libcamera:\n\tframe_buffer_allocator: Add clear()","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"On Thu, Sep 10, 2020 at 8:19 PM Niklas Söderlund\n wrote:\n>\n> Hi Jacopo,\n>\n> Thanks for your work.\n>\n> On 2020-09-09 17:54:52 +0200, Jacopo Mondi wrote:\n> > Add a clear() method to the FrameBufferAllocator class that\n> > frees all the buffers previously reserved by the allocator.\n> >\n> > Signed-off-by: Jacopo Mondi \n>\n> Reviewed-by: Niklas Söderlund \n>\n> > ---\n> > include/libcamera/framebuffer_allocator.h | 1 +\n> > src/libcamera/framebuffer_allocator.cpp | 8 ++++++++\n> > 2 files changed, 9 insertions(+)\n> >\n> > diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h\n> > index 78f1353964eb..2a4d538a0cb2 100644\n> > --- a/include/libcamera/framebuffer_allocator.h\n> > +++ b/include/libcamera/framebuffer_allocator.h\n> > @@ -28,6 +28,7 @@ public:\n> >\n> > int allocate(Stream *stream);\n> > int free(Stream *stream);\n> > + void clear();\n> >\n> > bool allocated() const { return !buffers_.empty(); }\n> > const std::vector> &buffers(Stream *stream) const;\n> > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp\n> > index 2fbba37a1b0b..7ed80011c845 100644\n> > --- a/src/libcamera/framebuffer_allocator.cpp\n> > +++ b/src/libcamera/framebuffer_allocator.cpp\n> > @@ -125,6 +125,14 @@ int FrameBufferAllocator::free(Stream *stream)\n> > return 0;\n> > }\n> >\n> > +/**\n> > + * \\brief Free all the buffers previously allocated\n> > + */\n> > +void FrameBufferAllocator::clear()\n> > +{\n> > + buffers_.clear();\n> > +}\n> > +\n\nThis is the first time I looked at FrameBufferAllocator.\nThose comments are not so related to this patch.\nI didn't get the difference between free() and clear() from the name.\nI would rename free() to erase(). If it sounds good to you, can I ask\nyou to do it later?\nBesides, clear() in dtor seems to be unnecessary.\n\nReviewed-by: Hirokazu Honda \n\n> > /**\n> > * \\fn FrameBufferAllocator::allocated()\n> > * \\brief Check if the allocator has allocated buffers for any stream\n> > --\n> > 2.28.0\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n>\n> --\n> Regards,\n> Niklas Söderlund\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 1A4D5C3B5B\n\tfor ;\n\tFri, 11 Sep 2020 04:43:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A4B2462D38;\n\tFri, 11 Sep 2020 06:43:11 +0200 (CEST)","from mail-ed1-x544.google.com (mail-ed1-x544.google.com\n\t[IPv6:2a00:1450:4864:20::544])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 46E106036C\n\tfor ;\n\tFri, 11 Sep 2020 06:43:10 +0200 (CEST)","by mail-ed1-x544.google.com with SMTP id ay8so8587076edb.8\n\tfor ;\n\tThu, 10 Sep 2020 21:43:10 -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=\"OpJsgk0r\"; 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:content-transfer-encoding;\n\tbh=lgLksYv0aRIuO0gSfO8J79cfwOmlDtCFrk9X9shMJNU=;\n\tb=OpJsgk0r+jVF0MO6mP+pODIfnutdLLbnQSCzSC8X8q6Jx3CK78jx9kLpU+begXzMH4\n\tT/thpQ1QUIAYntDaInISHX8ybNmq+JCTNfGaVD6FIVqQfPxQFTIRmHQxCAMmyDm/hyIH\n\tGAHLYqa2h4s8BoYt4gXwfu1Z2iG6YVm0lOXy4=","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:content-transfer-encoding;\n\tbh=lgLksYv0aRIuO0gSfO8J79cfwOmlDtCFrk9X9shMJNU=;\n\tb=j+aJUPAa7JS0zgU9rBXj/b57/EdS5EQoziszVLyLnqWYICp1pMTHddP0fCw9xZ+Jcn\n\tAdhFhpRJjVdNIZ4Kbh1ealKmvKtsSLo5Gi7WVhJlaEKUai2jFEsv0bW7MoUaK5PIyRl2\n\tBiAC8/Evjeo02WmjYyMl4w0VoyjkIH3NmNXi5R9f+qAv1gfPcbwEPSBw+K6svECrHYeK\n\tdeI5EssE81vByj8PG1FST6inBEoiSY7kAYy/fgzW/mXAEfnmPRS52rstKsy0FxKnfcQc\n\tzjJ/rHwDvToGdUf7/Z4krLI0+0/hP4DbrhDWxKgtxOP6jSQK+pakTGUK1brml7RFWQTM\n\tVQGQ==","X-Gm-Message-State":"AOAM5308Wj1vBXxSehGvi6WKVrNXVQrFreFhUkaDOgJ3EayEFgtZSY5p\n\t24nuSInZ7nqDGYFQzGCCbHVf65uKHl+oMlCvXmaanhMRsUrdOw==","X-Google-Smtp-Source":"ABdhPJwLLnOtDfuLbtpgwupFf1BLqMNtcdF1awjaIQ2AfBqpV+RNHnkiv/otI+xiPIWEPE/THMWOBIPQ/1Y9KJUnCoE=","X-Received":"by 2002:aa7:d296:: with SMTP id w22mr111219edq.327.1599799389933;\n\tThu, 10 Sep 2020 21:43:09 -0700 (PDT)","MIME-Version":"1.0","References":"<20200909155457.153907-1-jacopo@jmondi.org>\n\t<20200909155457.153907-4-jacopo@jmondi.org>\n\t<20200910111953.GP4095624@oden.dyn.berto.se>","In-Reply-To":"<20200910111953.GP4095624@oden.dyn.berto.se>","From":"Hirokazu Honda ","Date":"Fri, 11 Sep 2020 13:42:58 +0900","Message-ID":"","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= ","Subject":"Re: [libcamera-devel] [PATCH 3/8] libcamera:\n\tframe_buffer_allocator: Add clear()","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":"Hanlin Chen ,\n\tlibcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12456,"web_url":"https://patchwork.libcamera.org/comment/12456/","msgid":"<20200911163108.2qgevqrem4qlunpq@uno.localdomain>","date":"2020-09-11T16:31:08","subject":"Re: [libcamera-devel] [PATCH 3/8] libcamera:\n\tframe_buffer_allocator: Add clear()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Fri, Sep 11, 2020 at 01:42:58PM +0900, Hirokazu Honda wrote:\n> On Thu, Sep 10, 2020 at 8:19 PM Niklas Söderlund\n> wrote:\n> >\n> > Hi Jacopo,\n> >\n> > Thanks for your work.\n> >\n> > On 2020-09-09 17:54:52 +0200, Jacopo Mondi wrote:\n> > > Add a clear() method to the FrameBufferAllocator class that\n> > > frees all the buffers previously reserved by the allocator.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> >\n> > Reviewed-by: Niklas Söderlund \n> >\n> > > ---\n> > > include/libcamera/framebuffer_allocator.h | 1 +\n> > > src/libcamera/framebuffer_allocator.cpp | 8 ++++++++\n> > > 2 files changed, 9 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h\n> > > index 78f1353964eb..2a4d538a0cb2 100644\n> > > --- a/include/libcamera/framebuffer_allocator.h\n> > > +++ b/include/libcamera/framebuffer_allocator.h\n> > > @@ -28,6 +28,7 @@ public:\n> > >\n> > > int allocate(Stream *stream);\n> > > int free(Stream *stream);\n> > > + void clear();\n> > >\n> > > bool allocated() const { return !buffers_.empty(); }\n> > > const std::vector> &buffers(Stream *stream) const;\n> > > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp\n> > > index 2fbba37a1b0b..7ed80011c845 100644\n> > > --- a/src/libcamera/framebuffer_allocator.cpp\n> > > +++ b/src/libcamera/framebuffer_allocator.cpp\n> > > @@ -125,6 +125,14 @@ int FrameBufferAllocator::free(Stream *stream)\n> > > return 0;\n> > > }\n> > >\n> > > +/**\n> > > + * \\brief Free all the buffers previously allocated\n> > > + */\n> > > +void FrameBufferAllocator::clear()\n> > > +{\n> > > + buffers_.clear();\n> > > +}\n> > > +\n>\n> This is the first time I looked at FrameBufferAllocator.\n> Those comments are not so related to this patch.\n> I didn't get the difference between free() and clear() from the name.\n> I would rename free() to erase(). If it sounds good to you, can I ask\n> you to do it later?\n\nBeing this a map underneath, using erase would align with std::map\n\n> Besides, clear() in dtor seems to be unnecessary.\n>\n\nYou mean the one it's introduced in the next patch ? Probably not as\nthe newly added availableBuffers_ vector is just a vector of pointers,\nthere's not need to release memory...\n\n\n> Reviewed-by: Hirokazu Honda \n>\n> > > /**\n> > > * \\fn FrameBufferAllocator::allocated()\n> > > * \\brief Check if the allocator has allocated buffers for any stream\n> > > --\n> > > 2.28.0\n> > >\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\n> >\n> > --\n> > Regards,\n> > Niklas Söderlund\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel","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 DBB53BDB1D\n\tfor ;\n\tFri, 11 Sep 2020 16:27:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 61DAB62D86;\n\tFri, 11 Sep 2020 18:27:21 +0200 (CEST)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EAD1260534\n\tfor ;\n\tFri, 11 Sep 2020 18:27:19 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id CD9361BF20B;\n\tFri, 11 Sep 2020 16:27:18 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Fri, 11 Sep 2020 18:31:08 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20200911163108.2qgevqrem4qlunpq@uno.localdomain>","References":"<20200909155457.153907-1-jacopo@jmondi.org>\n\t<20200909155457.153907-4-jacopo@jmondi.org>\n\t<20200910111953.GP4095624@oden.dyn.berto.se>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 3/8] libcamera:\n\tframe_buffer_allocator: Add clear()","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":"Hanlin Chen ,\n\tlibcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12516,"web_url":"https://patchwork.libcamera.org/comment/12516/","msgid":"<20200915014148.GA8166@pendragon.ideasonboard.com>","date":"2020-09-15T01:41:48","subject":"Re: [libcamera-devel] [PATCH 3/8] libcamera:\n\tframe_buffer_allocator: Add clear()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Fri, Sep 11, 2020 at 06:31:08PM +0200, Jacopo Mondi wrote:\n> On Fri, Sep 11, 2020 at 01:42:58PM +0900, Hirokazu Honda wrote:\n> > On Thu, Sep 10, 2020 at 8:19 PM Niklas Söderlund wrote:\n> > > On 2020-09-09 17:54:52 +0200, Jacopo Mondi wrote:\n> > > > Add a clear() method to the FrameBufferAllocator class that\n> > > > frees all the buffers previously reserved by the allocator.\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > >\n> > > Reviewed-by: Niklas Söderlund \n> > >\n> > > > ---\n> > > > include/libcamera/framebuffer_allocator.h | 1 +\n> > > > src/libcamera/framebuffer_allocator.cpp | 8 ++++++++\n> > > > 2 files changed, 9 insertions(+)\n> > > >\n> > > > diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h\n> > > > index 78f1353964eb..2a4d538a0cb2 100644\n> > > > --- a/include/libcamera/framebuffer_allocator.h\n> > > > +++ b/include/libcamera/framebuffer_allocator.h\n> > > > @@ -28,6 +28,7 @@ public:\n> > > >\n> > > > int allocate(Stream *stream);\n> > > > int free(Stream *stream);\n> > > > + void clear();\n> > > >\n> > > > bool allocated() const { return !buffers_.empty(); }\n> > > > const std::vector> &buffers(Stream *stream) const;\n> > > > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp\n> > > > index 2fbba37a1b0b..7ed80011c845 100644\n> > > > --- a/src/libcamera/framebuffer_allocator.cpp\n> > > > +++ b/src/libcamera/framebuffer_allocator.cpp\n> > > > @@ -125,6 +125,14 @@ int FrameBufferAllocator::free(Stream *stream)\n> > > > return 0;\n> > > > }\n> > > >\n> > > > +/**\n> > > > + * \\brief Free all the buffers previously allocated\n> > > > + */\n> > > > +void FrameBufferAllocator::clear()\n> > > > +{\n> > > > + buffers_.clear();\n> > > > +}\n> > > > +\n> >\n> > This is the first time I looked at FrameBufferAllocator.\n> > Those comments are not so related to this patch.\n> > I didn't get the difference between free() and clear() from the name.\n> > I would rename free() to erase(). If it sounds good to you, can I ask\n> > you to do it later?\n> \n> Being this a map underneath, using erase would align with std::map\n\nThe free() operation is the counterpart of allocate(), I don't think\nallocate/erase would be a very good pair. I'd rather keep free(), or\nrename allocate too.\n\nRegarding clear(), I don't think that's a very intuitive name. freeAll()\nwould be better.\n\n> > Besides, clear() in dtor seems to be unnecessary.\n> \n> You mean the one it's introduced in the next patch ? Probably not as\n> the newly added availableBuffers_ vector is just a vector of pointers,\n> there's not need to release memory...\n> \n> > Reviewed-by: Hirokazu Honda \n> >\n> > > > /**\n> > > > * \\fn FrameBufferAllocator::allocated()\n> > > > * \\brief Check if the allocator has allocated buffers for any stream","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 E8918BF01C\n\tfor ;\n\tTue, 15 Sep 2020 01:42:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8153162D27;\n\tTue, 15 Sep 2020 03:42:18 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C83AB62901\n\tfor ;\n\tTue, 15 Sep 2020 03:42:16 +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 4BCA5275;\n\tTue, 15 Sep 2020 03:42:16 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"mO2S8MCu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1600134136;\n\tbh=j3oWBNj0Oz9ZVbBsRqMalPizaBnK+qydFJJ/MgkPYAo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=mO2S8MCuAmSuJv5XGKotTRTBRI24jelrR6giLowYwhoQcZt9PmN4KvOj2LPxPbGKz\n\ty4dtUaaGiqtg4k3lbuqh+3XJ3XI6alk7fDoEYX1xMESnL9TISJTSER4shrx+duVi3T\n\t5fJRpaZF9quXPOPX0Ve0ZNeOkfkltWYcYtzuLAWo=","Date":"Tue, 15 Sep 2020 04:41:48 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20200915014148.GA8166@pendragon.ideasonboard.com>","References":"<20200909155457.153907-1-jacopo@jmondi.org>\n\t<20200909155457.153907-4-jacopo@jmondi.org>\n\t<20200910111953.GP4095624@oden.dyn.berto.se>\n\t\n\t<20200911163108.2qgevqrem4qlunpq@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200911163108.2qgevqrem4qlunpq@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 3/8] libcamera:\n\tframe_buffer_allocator: Add clear()","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":"Hanlin Chen ,\n\tlibcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12557,"web_url":"https://patchwork.libcamera.org/comment/12557/","msgid":"<20200917104155.wp43razeikebr6aj@uno.localdomain>","date":"2020-09-17T10:41:55","subject":"Re: [libcamera-devel] [PATCH 3/8] libcamera:\n\tframe_buffer_allocator: Add clear()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Tue, Sep 15, 2020 at 04:41:48AM +0300, Laurent Pinchart wrote:\n> Hello,\n>\n> On Fri, Sep 11, 2020 at 06:31:08PM +0200, Jacopo Mondi wrote:\n> > On Fri, Sep 11, 2020 at 01:42:58PM +0900, Hirokazu Honda wrote:\n> > > On Thu, Sep 10, 2020 at 8:19 PM Niklas Söderlund wrote:\n> > > > On 2020-09-09 17:54:52 +0200, Jacopo Mondi wrote:\n> > > > > Add a clear() method to the FrameBufferAllocator class that\n> > > > > frees all the buffers previously reserved by the allocator.\n> > > > >\n> > > > > Signed-off-by: Jacopo Mondi \n> > > >\n> > > > Reviewed-by: Niklas Söderlund \n> > > >\n> > > > > ---\n> > > > > include/libcamera/framebuffer_allocator.h | 1 +\n> > > > > src/libcamera/framebuffer_allocator.cpp | 8 ++++++++\n> > > > > 2 files changed, 9 insertions(+)\n> > > > >\n> > > > > diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h\n> > > > > index 78f1353964eb..2a4d538a0cb2 100644\n> > > > > --- a/include/libcamera/framebuffer_allocator.h\n> > > > > +++ b/include/libcamera/framebuffer_allocator.h\n> > > > > @@ -28,6 +28,7 @@ public:\n> > > > >\n> > > > > int allocate(Stream *stream);\n> > > > > int free(Stream *stream);\n> > > > > + void clear();\n> > > > >\n> > > > > bool allocated() const { return !buffers_.empty(); }\n> > > > > const std::vector> &buffers(Stream *stream) const;\n> > > > > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp\n> > > > > index 2fbba37a1b0b..7ed80011c845 100644\n> > > > > --- a/src/libcamera/framebuffer_allocator.cpp\n> > > > > +++ b/src/libcamera/framebuffer_allocator.cpp\n> > > > > @@ -125,6 +125,14 @@ int FrameBufferAllocator::free(Stream *stream)\n> > > > > return 0;\n> > > > > }\n> > > > >\n> > > > > +/**\n> > > > > + * \\brief Free all the buffers previously allocated\n> > > > > + */\n> > > > > +void FrameBufferAllocator::clear()\n> > > > > +{\n> > > > > + buffers_.clear();\n> > > > > +}\n> > > > > +\n> > >\n> > > This is the first time I looked at FrameBufferAllocator.\n> > > Those comments are not so related to this patch.\n> > > I didn't get the difference between free() and clear() from the name.\n> > > I would rename free() to erase(). If it sounds good to you, can I ask\n> > > you to do it later?\n> >\n> > Being this a map underneath, using erase would align with std::map\n>\n> The free() operation is the counterpart of allocate(), I don't think\n> allocate/erase would be a very good pair. I'd rather keep free(), or\n> rename allocate too.\n\nOk. Even thoughI think erase() is in line with the std naming scheme\nwhich we try to respect when possible.\n\n>\n> Regarding clear(), I don't think that's a very intuitive name. freeAll()\n> would be better.\n\nWhat's not intuitive in clear() ? All the std containers have a\nclear() method with the same description: \"Clear the content\".\nIsn't that what it happens here?\n\nI would agree more on this if there was any explicity buffer releasing\noperation, but since memory release is a consequence of deleting the\nFrameBuffer instances that wraps the dmabuf fds, what we actually do\nis just call clear() on the buffes_ map.\n\nfreeAll() is more clunky to me.\n\nI'll keep clear() and free().\n\nThanks\n j\n\n>\n> > > Besides, clear() in dtor seems to be unnecessary.\n> >\n> > You mean the one it's introduced in the next patch ? Probably not as\n> > the newly added availableBuffers_ vector is just a vector of pointers,\n> > there's not need to release memory...\n> >\n> > > Reviewed-by: Hirokazu Honda \n> > >\n> > > > > /**\n> > > > > * \\fn FrameBufferAllocator::allocated()\n> > > > > * \\brief Check if the allocator has allocated buffers for any stream\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 1EBCDBF01C\n\tfor ;\n\tThu, 17 Sep 2020 10:38:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A2F7E6036C;\n\tThu, 17 Sep 2020 12:38:06 +0200 (CEST)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1D7D56036A\n\tfor ;\n\tThu, 17 Sep 2020 12:38:05 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id CBCEDFF80A;\n\tThu, 17 Sep 2020 10:38:03 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Thu, 17 Sep 2020 12:41:55 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20200917104155.wp43razeikebr6aj@uno.localdomain>","References":"<20200909155457.153907-1-jacopo@jmondi.org>\n\t<20200909155457.153907-4-jacopo@jmondi.org>\n\t<20200910111953.GP4095624@oden.dyn.berto.se>\n\t\n\t<20200911163108.2qgevqrem4qlunpq@uno.localdomain>\n\t<20200915014148.GA8166@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200915014148.GA8166@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 3/8] libcamera:\n\tframe_buffer_allocator: Add clear()","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":"Hanlin Chen ,\n\tlibcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12561,"web_url":"https://patchwork.libcamera.org/comment/12561/","msgid":"<20200917120508.GA3969@pendragon.ideasonboard.com>","date":"2020-09-17T12:05:08","subject":"Re: [libcamera-devel] [PATCH 3/8] libcamera:\n\tframe_buffer_allocator: Add clear()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Thu, Sep 17, 2020 at 12:41:55PM +0200, Jacopo Mondi wrote:\n> On Tue, Sep 15, 2020 at 04:41:48AM +0300, Laurent Pinchart wrote:\n> > On Fri, Sep 11, 2020 at 06:31:08PM +0200, Jacopo Mondi wrote:\n> > > On Fri, Sep 11, 2020 at 01:42:58PM +0900, Hirokazu Honda wrote:\n> > > > On Thu, Sep 10, 2020 at 8:19 PM Niklas Söderlund wrote:\n> > > > > On 2020-09-09 17:54:52 +0200, Jacopo Mondi wrote:\n> > > > > > Add a clear() method to the FrameBufferAllocator class that\n> > > > > > frees all the buffers previously reserved by the allocator.\n> > > > > >\n> > > > > > Signed-off-by: Jacopo Mondi \n> > > > >\n> > > > > Reviewed-by: Niklas Söderlund \n> > > > >\n> > > > > > ---\n> > > > > > include/libcamera/framebuffer_allocator.h | 1 +\n> > > > > > src/libcamera/framebuffer_allocator.cpp | 8 ++++++++\n> > > > > > 2 files changed, 9 insertions(+)\n> > > > > >\n> > > > > > diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h\n> > > > > > index 78f1353964eb..2a4d538a0cb2 100644\n> > > > > > --- a/include/libcamera/framebuffer_allocator.h\n> > > > > > +++ b/include/libcamera/framebuffer_allocator.h\n> > > > > > @@ -28,6 +28,7 @@ public:\n> > > > > >\n> > > > > > int allocate(Stream *stream);\n> > > > > > int free(Stream *stream);\n> > > > > > + void clear();\n> > > > > >\n> > > > > > bool allocated() const { return !buffers_.empty(); }\n> > > > > > const std::vector> &buffers(Stream *stream) const;\n> > > > > > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp\n> > > > > > index 2fbba37a1b0b..7ed80011c845 100644\n> > > > > > --- a/src/libcamera/framebuffer_allocator.cpp\n> > > > > > +++ b/src/libcamera/framebuffer_allocator.cpp\n> > > > > > @@ -125,6 +125,14 @@ int FrameBufferAllocator::free(Stream *stream)\n> > > > > > return 0;\n> > > > > > }\n> > > > > >\n> > > > > > +/**\n> > > > > > + * \\brief Free all the buffers previously allocated\n> > > > > > + */\n> > > > > > +void FrameBufferAllocator::clear()\n> > > > > > +{\n> > > > > > + buffers_.clear();\n> > > > > > +}\n> > > > > > +\n> > > >\n> > > > This is the first time I looked at FrameBufferAllocator.\n> > > > Those comments are not so related to this patch.\n> > > > I didn't get the difference between free() and clear() from the name.\n> > > > I would rename free() to erase(). If it sounds good to you, can I ask\n> > > > you to do it later?\n> > >\n> > > Being this a map underneath, using erase would align with std::map\n> >\n> > The free() operation is the counterpart of allocate(), I don't think\n> > allocate/erase would be a very good pair. I'd rather keep free(), or\n> > rename allocate too.\n> \n> Ok. Even thoughI think erase() is in line with the std naming scheme\n> which we try to respect when possible.\n\nSorry, I meant allocate/free.\n\n> > Regarding clear(), I don't think that's a very intuitive name. freeAll()\n> > would be better.\n> \n> What's not intuitive in clear() ? All the std containers have a\n> clear() method with the same description: \"Clear the content\".\n> Isn't that what it happens here?\n\nBut this isn't a container, it's an allocator...\n\n> I would agree more on this if there was any explicity buffer releasing\n> operation, but since memory release is a consequence of deleting the\n> FrameBuffer instances that wraps the dmabuf fds, what we actually do\n> is just call clear() on the buffes_ map.\n> \n> freeAll() is more clunky to me.\n> \n> I'll keep clear() and free().\n> \n> > > > Besides, clear() in dtor seems to be unnecessary.\n> > >\n> > > You mean the one it's introduced in the next patch ? Probably not as\n> > > the newly added availableBuffers_ vector is just a vector of pointers,\n> > > there's not need to release memory...\n> > >\n> > > > Reviewed-by: Hirokazu Honda \n> > > >\n> > > > > > /**\n> > > > > > * \\fn FrameBufferAllocator::allocated()\n> > > > > > * \\brief Check if the allocator has allocated buffers for any stream","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 D65F0BF01C\n\tfor ;\n\tThu, 17 Sep 2020 12:05:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4F4F562F53;\n\tThu, 17 Sep 2020 14:05:41 +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 3645A6036A\n\tfor ;\n\tThu, 17 Sep 2020 14:05: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 A33BB2DB;\n\tThu, 17 Sep 2020 14:05:38 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"vYGXz2Rx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1600344338;\n\tbh=h3zhDalomr44o9f+dQYfJ0+M+5rnavR3HXkce2RbmZU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=vYGXz2RxT0xf2TS2HbEjus3KRgeSzHEgdxXzkyPESR43M91JksTiSYxo+9pKAg2sE\n\t4YCnn4OFPGN0/DAYPM2TdIIotqNA5Iz9p2+ggtZhzipyTweef71sU6/FDcwZw1L8EG\n\tqDFUQYGjyxmankUVBnDJ2cAQnzbv9ewASq+QgUA0=","Date":"Thu, 17 Sep 2020 15:05:08 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20200917120508.GA3969@pendragon.ideasonboard.com>","References":"<20200909155457.153907-1-jacopo@jmondi.org>\n\t<20200909155457.153907-4-jacopo@jmondi.org>\n\t<20200910111953.GP4095624@oden.dyn.berto.se>\n\t\n\t<20200911163108.2qgevqrem4qlunpq@uno.localdomain>\n\t<20200915014148.GA8166@pendragon.ideasonboard.com>\n\t<20200917104155.wp43razeikebr6aj@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200917104155.wp43razeikebr6aj@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 3/8] libcamera:\n\tframe_buffer_allocator: Add clear()","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":"Hanlin Chen ,\n\tlibcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]