[{"id":12422,"web_url":"https://patchwork.libcamera.org/comment/12422/","msgid":"<20200910113016.GR4095624@oden.dyn.berto.se>","date":"2020-09-10T11:30:16","subject":"Re: [libcamera-devel] [PATCH 5/8] libcamera: framebuffer_allocator:\n\tGet and return buffers","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:54 +0200, Jacopo Mondi wrote:\n> Add to the FrameBufferAllocator class two methods to get and\n> return buffers from the pool of buffers allocated for a Stream.\n> \n> The two methods return pointer to the allocated buffers without\n> transferring ownership to the caller.\n\nI'm sorry I don't like this patch. If it was an interface that was \ninternal I would be more OK with this change. The FrameBufferAllocator \nis facing applications and this change introduces a duality between the \nnew getBuffer() and the existing buffers() functionality.\n\nI think it creates complexity in this user-facing API which is not \nreally needed. How about creating a HALFrameBufferAllocator that is \nlocal to the HAL and inherits from FrameBufferAllocator while adding the \nfeatures you want?\n\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/framebuffer_allocator.h | 5 ++\n> src/libcamera/framebuffer_allocator.cpp | 60 +++++++++++++++++++++--\n> 2 files changed, 62 insertions(+), 3 deletions(-)\n> \n> diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h\n> index 2a4d538a0cb2..1f3f10d4ec03 100644\n> --- a/include/libcamera/framebuffer_allocator.h\n> +++ b/include/libcamera/framebuffer_allocator.h\n> @@ -9,6 +9,7 @@\n> \n> #include \n> #include \n> +#include \n> #include \n> \n> namespace libcamera {\n> @@ -33,9 +34,13 @@ public:\n> \tbool allocated() const { return !buffers_.empty(); }\n> \tconst std::vector> &buffers(Stream *stream) const;\n> \n> +\tFrameBuffer *getBuffer(Stream *stream);\n> +\tvoid returnBuffer(Stream *stream, FrameBuffer *buffer);\n> +\n> private:\n> \tstd::shared_ptr camera_;\n> \tstd::map>> buffers_;\n> +\tstd::map> availableBuffers_;\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp\n> index 7ed80011c845..7429d6b9edb7 100644\n> --- a/src/libcamera/framebuffer_allocator.cpp\n> +++ b/src/libcamera/framebuffer_allocator.cpp\n> @@ -64,7 +64,7 @@ FrameBufferAllocator::FrameBufferAllocator(std::shared_ptr camera)\n> \n> FrameBufferAllocator::~FrameBufferAllocator()\n> {\n> -\tbuffers_.clear();\n> +\tclear();\n> }\n> \n> /**\n> @@ -93,11 +93,17 @@ int FrameBufferAllocator::allocate(Stream *stream)\n> \t}\n> \n> \tint ret = camera_->exportFrameBuffers(stream, &buffers_[stream]);\n> -\tif (ret == -EINVAL)\n> +\tif (ret == -EINVAL) {\n> \t\tLOG(Allocator, Error)\n> \t\t\t<< \"Stream is not part of \" << camera_->id()\n> \t\t\t<< \" active configuration\";\n> -\treturn ret;\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tfor (const auto &buffer : buffers_[stream])\n> +\t\tavailableBuffers_[stream].push(buffer.get());\n> +\n> +\treturn 0;\n> }\n> \n> /**\n> @@ -122,6 +128,9 @@ int FrameBufferAllocator::free(Stream *stream)\n> \tbuffers.clear();\n> \tbuffers_.erase(iter);\n> \n> +\tavailableBuffers_[stream] = {};\n> +\tavailableBuffers_.erase(availableBuffers_.find(stream));\n> +\n> \treturn 0;\n> }\n> \n> @@ -131,6 +140,7 @@ int FrameBufferAllocator::free(Stream *stream)\n> void FrameBufferAllocator::clear()\n> {\n> \tbuffers_.clear();\n> +\tavailableBuffers_.clear();\n> }\n> \n> /**\n> @@ -162,4 +172,48 @@ FrameBufferAllocator::buffers(Stream *stream) const\n> \treturn iter->second;\n> }\n> \n> +/**\n> + * \\brief Get a pointer to a \\a buffer for the \\a stream\n> + * \\param[in] stream The stream to get a buffer for\n> + *\n> + * The method returns a pointer to a FrameBuffer but does transfer the buffer\n> + * ownership to the caller: the returned pointer remains valid until the\n> + * FrameBufferAllocator does not get deleted or the allocated buffers do not get\n> + * released with a call for free() or clear().\n> + *\n> + * \\return A FrameBuffer pointer or nullptr if the no buffers is available\n> + */\n> +FrameBuffer *FrameBufferAllocator::getBuffer(Stream *stream)\n> +{\n> +\tif (!allocated() || buffers_[stream].empty())\n> +\t\treturn nullptr;\n> +\n> +\tFrameBuffer *frameBuffer = availableBuffers_[stream].front();\n> +\tavailableBuffers_[stream].pop();\n> +\n> +\treturn frameBuffer;\n> +}\n> +\n> +/**\n> + * \\brief Return a \\a buffer to the list of buffers available for the a \\a stream\n> + * \\param[in] stream The stream to return buffer to\n> + * \\param[in] buffer The buffer to return\n> + */\n> +void FrameBufferAllocator::returnBuffer(Stream *stream, FrameBuffer *buffer)\n> +{\n> +\tif (!allocated())\n> +\t\treturn;\n> +\n> +\tfor (const auto &b : buffers_[stream]) {\n> +\t\t/*\n> +\t\t * Return the buffer to the available queue only if it was part\n> +\t\t * of the vector of buffers allocated for the Stream.\n> +\t\t */\n> +\t\tif (b.get() != buffer)\n> +\t\t\tcontinue;\n> +\n> +\t\tavailableBuffers_[stream].push(buffer);\n> +\t}\n> +}\n> +\n> } /* namespace libcamera */\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 61724BDB1D\n\tfor ;\n\tThu, 10 Sep 2020 11:30:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EF9DA62D8E;\n\tThu, 10 Sep 2020 13:30:19 +0200 (CEST)","from mail-lj1-x22f.google.com (mail-lj1-x22f.google.com\n\t[IPv6:2a00:1450:4864:20::22f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 765A162D38\n\tfor ;\n\tThu, 10 Sep 2020 13:30:18 +0200 (CEST)","by mail-lj1-x22f.google.com with SMTP id n25so7684649ljj.4\n\tfor ;\n\tThu, 10 Sep 2020 04:30:18 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tm10sm1497224ljg.138.2020.09.10.04.30.17\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 10 Sep 2020 04:30:17 -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=\"nAY5c5c5\"; 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=uxg6YVnlsMwrFX6zAA8O2lUm1JbA+4XatzbRPfY6jZ4=;\n\tb=nAY5c5c5OKYWaQbqMc49QrX4hfZfewpsQjxq1ZBSNMx5WCpURwm38ecdung28J0JTm\n\tWcgX7KlLmj7yz78vEKPTF4VJHHBzp9AOrA9VS1DKVGozd9ol4P5IzNJN/RBDXp4Z46vz\n\trozZZWaVfq1l2xV63FGX1lJ9Cnw8Wi7o0M5w+uWiXmH9p0BjWX/lRDVmuu/Re0kCBqiK\n\tnShhgQJ59MLn/US6dmqNBuY/Kh/a12lftvsEF3RhoOpV4B1nTxzMEpQF8ymHq5ZrIKIQ\n\tuRB5i5pUwuQpuHoF26KxV/c7kLIgD3zT45MdFaIUaob9Dh/QxYQPH4qw/bTusAfWtE6z\n\trF5w==","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=uxg6YVnlsMwrFX6zAA8O2lUm1JbA+4XatzbRPfY6jZ4=;\n\tb=XRI46td1QAM8zajCRd4CZup8guGvQDUMwlIeTywFEg3HsZEqTGs8+adSkfIW0dHkNL\n\tXn5jICQJfZDgaSEnGyU/h9eWwbuTTTucV0FV/5n2ND0pnW+kM23kICPtE59L6zIj1/eA\n\tBHsJ0Yr6/Kr6ZZvvsw3LOEAFyg+kn/gYzfBqeq/2UYDEwMZk0PsVjMmZZi4fiM5QAz0w\n\t5esR2TeeyMz68Ty4+cBVnd3qtveoy4vb4TQyLGbXN4129rEUne4f2WN6mNzKwlcHP1ZZ\n\tDlIsDicVggEGTAwIEw0cZnm9DEPBt7DlGPuUXJZIKYEKLc2dZ0Rw6zcj0ZJoVgVjALlS\n\tXXmg==","X-Gm-Message-State":"AOAM530m1XNy5SF9L7lqAdjukmU5WZtvLRJ6HAIzJazQGcSTsHShputN\n\tosMZr96oH9eOurWxlWbi/ml78Q==","X-Google-Smtp-Source":"ABdhPJxcjqmxkiKZnL0Jc3dxr58jxOf5qCMtiGwpmCtdsK8paNYwWTFK5LIqEH4VaaXeuorUlYgKRg==","X-Received":"by 2002:a2e:85d9:: with SMTP id h25mr821625ljj.279.1599737417893;\n\tThu, 10 Sep 2020 04:30:17 -0700 (PDT)","Date":"Thu, 10 Sep 2020 13:30:16 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"<20200910113016.GR4095624@oden.dyn.berto.se>","References":"<20200909155457.153907-1-jacopo@jmondi.org>\n\t<20200909155457.153907-6-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200909155457.153907-6-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 5/8] libcamera: framebuffer_allocator:\n\tGet and return buffers","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":12426,"web_url":"https://patchwork.libcamera.org/comment/12426/","msgid":"<20200910113654.GV4095624@oden.dyn.berto.se>","date":"2020-09-10T11:36:54","subject":"Re: [libcamera-devel] [PATCH 5/8] libcamera: framebuffer_allocator:\n\tGet and return buffers","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hello again,\n\nOn 2020-09-09 17:54:54 +0200, Jacopo Mondi wrote:\n> Add to the FrameBufferAllocator class two methods to get and\n> return buffers from the pool of buffers allocated for a Stream.\n> \n> The two methods return pointer to the allocated buffers without\n> transferring ownership to the caller.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> include/libcamera/framebuffer_allocator.h | 5 ++\n> src/libcamera/framebuffer_allocator.cpp | 60 +++++++++++++++++++++--\n> 2 files changed, 62 insertions(+), 3 deletions(-)\n> \n> diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h\n> index 2a4d538a0cb2..1f3f10d4ec03 100644\n> --- a/include/libcamera/framebuffer_allocator.h\n> +++ b/include/libcamera/framebuffer_allocator.h\n> @@ -9,6 +9,7 @@\n> \n> #include \n> #include \n> +#include \n> #include \n> \n> namespace libcamera {\n> @@ -33,9 +34,13 @@ public:\n> \tbool allocated() const { return !buffers_.empty(); }\n> \tconst std::vector> &buffers(Stream *stream) const;\n> \n> +\tFrameBuffer *getBuffer(Stream *stream);\n> +\tvoid returnBuffer(Stream *stream, FrameBuffer *buffer);\n> +\n> private:\n> \tstd::shared_ptr camera_;\n> \tstd::map>> buffers_;\n> +\tstd::map> availableBuffers_;\n> };\n> \n> } /* namespace libcamera */\n> diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp\n> index 7ed80011c845..7429d6b9edb7 100644\n> --- a/src/libcamera/framebuffer_allocator.cpp\n> +++ b/src/libcamera/framebuffer_allocator.cpp\n> @@ -64,7 +64,7 @@ FrameBufferAllocator::FrameBufferAllocator(std::shared_ptr camera)\n> \n> FrameBufferAllocator::~FrameBufferAllocator()\n> {\n> -\tbuffers_.clear();\n> +\tclear();\n\nI think this could be moved to the patch that adds \nFrameBufferAllocator::clear().\n\n> }\n> \n> /**\n> @@ -93,11 +93,17 @@ int FrameBufferAllocator::allocate(Stream *stream)\n> \t}\n> \n> \tint ret = camera_->exportFrameBuffers(stream, &buffers_[stream]);\n> -\tif (ret == -EINVAL)\n> +\tif (ret == -EINVAL) {\n> \t\tLOG(Allocator, Error)\n> \t\t\t<< \"Stream is not part of \" << camera_->id()\n> \t\t\t<< \" active configuration\";\n> -\treturn ret;\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tfor (const auto &buffer : buffers_[stream])\n> +\t\tavailableBuffers_[stream].push(buffer.get());\n> +\n> +\treturn 0;\n> }\n> \n> /**\n> @@ -122,6 +128,9 @@ int FrameBufferAllocator::free(Stream *stream)\n> \tbuffers.clear();\n> \tbuffers_.erase(iter);\n> \n> +\tavailableBuffers_[stream] = {};\n> +\tavailableBuffers_.erase(availableBuffers_.find(stream));\n> +\n> \treturn 0;\n> }\n> \n> @@ -131,6 +140,7 @@ int FrameBufferAllocator::free(Stream *stream)\n> void FrameBufferAllocator::clear()\n> {\n> \tbuffers_.clear();\n> +\tavailableBuffers_.clear();\n> }\n> \n> /**\n> @@ -162,4 +172,48 @@ FrameBufferAllocator::buffers(Stream *stream) const\n> \treturn iter->second;\n> }\n> \n> +/**\n> + * \\brief Get a pointer to a \\a buffer for the \\a stream\n> + * \\param[in] stream The stream to get a buffer for\n> + *\n> + * The method returns a pointer to a FrameBuffer but does transfer the buffer\n> + * ownership to the caller: the returned pointer remains valid until the\n> + * FrameBufferAllocator does not get deleted or the allocated buffers do not get\n> + * released with a call for free() or clear().\n> + *\n> + * \\return A FrameBuffer pointer or nullptr if the no buffers is available\n> + */\n> +FrameBuffer *FrameBufferAllocator::getBuffer(Stream *stream)\n> +{\n> +\tif (!allocated() || buffers_[stream].empty())\n> +\t\treturn nullptr;\n> +\n> +\tFrameBuffer *frameBuffer = availableBuffers_[stream].front();\n> +\tavailableBuffers_[stream].pop();\n> +\n> +\treturn frameBuffer;\n> +}\n> +\n> +/**\n> + * \\brief Return a \\a buffer to the list of buffers available for the a \\a stream\n> + * \\param[in] stream The stream to return buffer to\n> + * \\param[in] buffer The buffer to return\n> + */\n> +void FrameBufferAllocator::returnBuffer(Stream *stream, FrameBuffer *buffer)\n> +{\n> +\tif (!allocated())\n> +\t\treturn;\n> +\n> +\tfor (const auto &b : buffers_[stream]) {\n> +\t\t/*\n> +\t\t * Return the buffer to the available queue only if it was part\n> +\t\t * of the vector of buffers allocated for the Stream.\n> +\t\t */\n> +\t\tif (b.get() != buffer)\n> +\t\t\tcontinue;\n> +\n> +\t\tavailableBuffers_[stream].push(buffer);\n> +\t}\n> +}\n> +\n> } /* namespace libcamera */\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 252B2C3B5B\n\tfor ;\n\tThu, 10 Sep 2020 11:36:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E765862D9C;\n\tThu, 10 Sep 2020 13:36:57 +0200 (CEST)","from mail-lj1-x243.google.com (mail-lj1-x243.google.com\n\t[IPv6:2a00:1450:4864:20::243])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1081462D60\n\tfor ;\n\tThu, 10 Sep 2020 13:36:56 +0200 (CEST)","by mail-lj1-x243.google.com with SMTP id u21so7700576ljl.6\n\tfor ;\n\tThu, 10 Sep 2020 04:36:56 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tz25sm58687ljz.114.2020.09.10.04.36.54\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 10 Sep 2020 04:36:55 -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=\"jkMCy+dg\"; 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=XZwoxiiAHbh4ddxIQdml5H4UOkBLQ/E9sZO/5PAQ9is=;\n\tb=jkMCy+dgBAkoK2yRpR68JkaN2ouYke0Ugz4vJWGRxS1JxzS2NRZ9/Vo43tIp8SUVgD\n\towOlByKsWHqX1y7FNFJzg57F6GGSMwcA4aAN75ea8bKbSmfo4xOrKOFm+pwyGdNcYXhy\n\tdr7MQvCydHRXkIQ/WM54HTF6S+AuAv8Hg0FhRvZFeFTIpw2p3GBZ4e3jMdzLZxTmzLq0\n\tLwP0KFoS4+Xq9G8xrtj7Hmdm/khvbCy/MrLDO3sLXU6ImbeZHwR3brmXEOvhIWaTJC78\n\tpQCzM0cYMq/knCXur4K8Wi/1oebSaF/s42XwlXwaE4hywUU0SvDDkn7IdknDQdx6gehY\n\t8a3Q==","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=XZwoxiiAHbh4ddxIQdml5H4UOkBLQ/E9sZO/5PAQ9is=;\n\tb=LPd/OEGFEmxw6gu4/XRSRPZWg4DEPL0O4SlXD3NYBdDwe1owoWzYPA6rykWx/ulQv3\n\tGF5TKZzxmEWF2gf0A8BQDe1X0HwEvR8Dpk/F2CcsZYv0Ka+tabsXpKTfr+t2MrbzNou8\n\tYuPes10nH3uBDShe9KoqKz7yKZppH1PISF+lzcVrw79gixKP2z/mQgDjW66rpW6m4ClN\n\tv21yETTJyXuFszlZD4C0rAQ56SWktG+JURaxJ6zTsDAng9M3lw2+hEEQist2RIb2WDrR\n\tVAo9oKCxZoAyIfjC7s2TR2j+PYbXWwU2hlJ2zqtExT0kVErxg/MMDfnYOCVpZjJD23BL\n\tJ/Zg==","X-Gm-Message-State":"AOAM5306oEu1F+CN4DpqWKDWSggWj4t1XQT9tSioZOoM6gyT1Sz8tdH3\n\tJpn2Tl0Gr/osMu40MWVtToPkUg==","X-Google-Smtp-Source":"ABdhPJwYJHix2bQigNc/hzACJe36PebN2ueSj0zXG+YUq9x9N7kJtEaraCEKukNY+fvO3jI0wHXAcQ==","X-Received":"by 2002:a2e:88d6:: with SMTP id a22mr4500910ljk.9.1599737815474; \n\tThu, 10 Sep 2020 04:36:55 -0700 (PDT)","Date":"Thu, 10 Sep 2020 13:36:54 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"<20200910113654.GV4095624@oden.dyn.berto.se>","References":"<20200909155457.153907-1-jacopo@jmondi.org>\n\t<20200909155457.153907-6-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200909155457.153907-6-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 5/8] libcamera: framebuffer_allocator:\n\tGet and return buffers","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":12428,"web_url":"https://patchwork.libcamera.org/comment/12428/","msgid":"<20200910122746.le3ndavmu6btkouk@uno.localdomain>","date":"2020-09-10T12:27:46","subject":"Re: [libcamera-devel] [PATCH 5/8] libcamera: framebuffer_allocator:\n\tGet and return buffers","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Thu, Sep 10, 2020 at 01:30:16PM +0200, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> Thanks for your work.\n>\n> On 2020-09-09 17:54:54 +0200, Jacopo Mondi wrote:\n> > Add to the FrameBufferAllocator class two methods to get and\n> > return buffers from the pool of buffers allocated for a Stream.\n> >\n> > The two methods return pointer to the allocated buffers without\n> > transferring ownership to the caller.\n>\n> I'm sorry I don't like this patch. If it was an interface that was\n> internal I would be more OK with this change. The FrameBufferAllocator\n> is facing applications and this change introduces a duality between the\n> new getBuffer() and the existing buffers() functionality.\n>\n> I think it creates complexity in this user-facing API which is not\n> really needed. How about creating a HALFrameBufferAllocator that is\n> local to the HAL and inherits from FrameBufferAllocator while adding the\n> features you want?\n\nThat would end up repeating the same thing we do in pipeline handlers\nhere. Actually I found safer to provide a get/return interface than\ngiving applications full access to the pool and let them deal with\nthat. At the same time I won't prevent that completely by removing\nbuffers(), but I don't see why the 'duality' is an issue.\n\nI'm more than open to discuss the implementation which is surely\nimprovable and if we want to implement this feature in\nFrameBufferAllocator or as you suggested sub-class it somehow, but I'm\nnot sure why the problem lies in having the three methods.\n\n>\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > include/libcamera/framebuffer_allocator.h | 5 ++\n> > src/libcamera/framebuffer_allocator.cpp | 60 +++++++++++++++++++++--\n> > 2 files changed, 62 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h\n> > index 2a4d538a0cb2..1f3f10d4ec03 100644\n> > --- a/include/libcamera/framebuffer_allocator.h\n> > +++ b/include/libcamera/framebuffer_allocator.h\n> > @@ -9,6 +9,7 @@\n> >\n> > #include \n> > #include \n> > +#include \n> > #include \n> >\n> > namespace libcamera {\n> > @@ -33,9 +34,13 @@ public:\n> > \tbool allocated() const { return !buffers_.empty(); }\n> > \tconst std::vector> &buffers(Stream *stream) const;\n> >\n> > +\tFrameBuffer *getBuffer(Stream *stream);\n> > +\tvoid returnBuffer(Stream *stream, FrameBuffer *buffer);\n> > +\n> > private:\n> > \tstd::shared_ptr camera_;\n> > \tstd::map>> buffers_;\n> > +\tstd::map> availableBuffers_;\n> > };\n> >\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp\n> > index 7ed80011c845..7429d6b9edb7 100644\n> > --- a/src/libcamera/framebuffer_allocator.cpp\n> > +++ b/src/libcamera/framebuffer_allocator.cpp\n> > @@ -64,7 +64,7 @@ FrameBufferAllocator::FrameBufferAllocator(std::shared_ptr camera)\n> >\n> > FrameBufferAllocator::~FrameBufferAllocator()\n> > {\n> > -\tbuffers_.clear();\n> > +\tclear();\n> > }\n> >\n> > /**\n> > @@ -93,11 +93,17 @@ int FrameBufferAllocator::allocate(Stream *stream)\n> > \t}\n> >\n> > \tint ret = camera_->exportFrameBuffers(stream, &buffers_[stream]);\n> > -\tif (ret == -EINVAL)\n> > +\tif (ret == -EINVAL) {\n> > \t\tLOG(Allocator, Error)\n> > \t\t\t<< \"Stream is not part of \" << camera_->id()\n> > \t\t\t<< \" active configuration\";\n> > -\treturn ret;\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\tfor (const auto &buffer : buffers_[stream])\n> > +\t\tavailableBuffers_[stream].push(buffer.get());\n> > +\n> > +\treturn 0;\n> > }\n> >\n> > /**\n> > @@ -122,6 +128,9 @@ int FrameBufferAllocator::free(Stream *stream)\n> > \tbuffers.clear();\n> > \tbuffers_.erase(iter);\n> >\n> > +\tavailableBuffers_[stream] = {};\n> > +\tavailableBuffers_.erase(availableBuffers_.find(stream));\n> > +\n> > \treturn 0;\n> > }\n> >\n> > @@ -131,6 +140,7 @@ int FrameBufferAllocator::free(Stream *stream)\n> > void FrameBufferAllocator::clear()\n> > {\n> > \tbuffers_.clear();\n> > +\tavailableBuffers_.clear();\n> > }\n> >\n> > /**\n> > @@ -162,4 +172,48 @@ FrameBufferAllocator::buffers(Stream *stream) const\n> > \treturn iter->second;\n> > }\n> >\n> > +/**\n> > + * \\brief Get a pointer to a \\a buffer for the \\a stream\n> > + * \\param[in] stream The stream to get a buffer for\n> > + *\n> > + * The method returns a pointer to a FrameBuffer but does transfer the buffer\n> > + * ownership to the caller: the returned pointer remains valid until the\n> > + * FrameBufferAllocator does not get deleted or the allocated buffers do not get\n> > + * released with a call for free() or clear().\n> > + *\n> > + * \\return A FrameBuffer pointer or nullptr if the no buffers is available\n> > + */\n> > +FrameBuffer *FrameBufferAllocator::getBuffer(Stream *stream)\n> > +{\n> > +\tif (!allocated() || buffers_[stream].empty())\n> > +\t\treturn nullptr;\n> > +\n> > +\tFrameBuffer *frameBuffer = availableBuffers_[stream].front();\n> > +\tavailableBuffers_[stream].pop();\n> > +\n> > +\treturn frameBuffer;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Return a \\a buffer to the list of buffers available for the a \\a stream\n> > + * \\param[in] stream The stream to return buffer to\n> > + * \\param[in] buffer The buffer to return\n> > + */\n> > +void FrameBufferAllocator::returnBuffer(Stream *stream, FrameBuffer *buffer)\n> > +{\n> > +\tif (!allocated())\n> > +\t\treturn;\n> > +\n> > +\tfor (const auto &b : buffers_[stream]) {\n> > +\t\t/*\n> > +\t\t * Return the buffer to the available queue only if it was part\n> > +\t\t * of the vector of buffers allocated for the Stream.\n> > +\t\t */\n> > +\t\tif (b.get() != buffer)\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tavailableBuffers_[stream].push(buffer);\n> > +\t}\n> > +}\n> > +\n> > } /* namespace libcamera */\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","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 B8DB7C3B5B\n\tfor ;\n\tThu, 10 Sep 2020 12:23:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4871562D64;\n\tThu, 10 Sep 2020 14:23:59 +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 0245762D27\n\tfor ;\n\tThu, 10 Sep 2020 14:23:57 +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 5DF991BF204;\n\tThu, 10 Sep 2020 12:23:57 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Thu, 10 Sep 2020 14:27:46 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200910122746.le3ndavmu6btkouk@uno.localdomain>","References":"<20200909155457.153907-1-jacopo@jmondi.org>\n\t<20200909155457.153907-6-jacopo@jmondi.org>\n\t<20200910113016.GR4095624@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200910113016.GR4095624@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH 5/8] libcamera: framebuffer_allocator:\n\tGet and return buffers","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=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12429,"web_url":"https://patchwork.libcamera.org/comment/12429/","msgid":"<20200910130255.GW4095624@oden.dyn.berto.se>","date":"2020-09-10T13:02:55","subject":"Re: [libcamera-devel] [PATCH 5/8] libcamera: framebuffer_allocator:\n\tGet and return buffers","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nOn 2020-09-10 14:27:46 +0200, Jacopo Mondi wrote:\n> Hi Niklas,\n> \n> On Thu, Sep 10, 2020 at 01:30:16PM +0200, Niklas Söderlund wrote:\n> > Hi Jacopo,\n> >\n> > Thanks for your work.\n> >\n> > On 2020-09-09 17:54:54 +0200, Jacopo Mondi wrote:\n> > > Add to the FrameBufferAllocator class two methods to get and\n> > > return buffers from the pool of buffers allocated for a Stream.\n> > >\n> > > The two methods return pointer to the allocated buffers without\n> > > transferring ownership to the caller.\n> >\n> > I'm sorry I don't like this patch. If it was an interface that was\n> > internal I would be more OK with this change. The FrameBufferAllocator\n> > is facing applications and this change introduces a duality between the\n> > new getBuffer() and the existing buffers() functionality.\n> >\n> > I think it creates complexity in this user-facing API which is not\n> > really needed. How about creating a HALFrameBufferAllocator that is\n> > local to the HAL and inherits from FrameBufferAllocator while adding the\n> > features you want?\n> \n> That would end up repeating the same thing we do in pipeline handlers\n> here. Actually I found safer to provide a get/return interface than\n> giving applications full access to the pool and let them deal with\n> that. At the same time I won't prevent that completely by removing\n> buffers(), but I don't see why the 'duality' is an issue.\n> \n> I'm more than open to discuss the implementation which is surely\n> improvable and if we want to implement this feature in\n> FrameBufferAllocator or as you suggested sub-class it somehow, but I'm\n> not sure why the problem lies in having the three methods.\n\nMy concern is having an application call getBuffer() to prepare one set \nof Requests, maybe one with a viewfinder + raw and then iterating over \nbuffers() to create as many Requests with just viewfinder as available. \nAs buffers() don't consider if it have \"given\" out any buffer prior to \nthe buffers() call we have the potential to have Requests that can't be \nqueued at the same time as they contain a subset of the same buffers. I \nfear this could get even worse once Request becomes reusable.\n\nUnderstand me correctly, I'm not advocating in favor of buffers() or \ngetBuffers() interface. I do however think we should only have one as it \nmakes the interface easier to use for applications. As you suggest this \nnew interface could replace boilerplait code in pipelines and possibly \neven applications to maybe it should replace the old buffers() \ninterface?\n\n> \n> >\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > include/libcamera/framebuffer_allocator.h | 5 ++\n> > > src/libcamera/framebuffer_allocator.cpp | 60 +++++++++++++++++++++--\n> > > 2 files changed, 62 insertions(+), 3 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h\n> > > index 2a4d538a0cb2..1f3f10d4ec03 100644\n> > > --- a/include/libcamera/framebuffer_allocator.h\n> > > +++ b/include/libcamera/framebuffer_allocator.h\n> > > @@ -9,6 +9,7 @@\n> > >\n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > >\n> > > namespace libcamera {\n> > > @@ -33,9 +34,13 @@ public:\n> > > \tbool allocated() const { return !buffers_.empty(); }\n> > > \tconst std::vector> &buffers(Stream *stream) const;\n> > >\n> > > +\tFrameBuffer *getBuffer(Stream *stream);\n> > > +\tvoid returnBuffer(Stream *stream, FrameBuffer *buffer);\n> > > +\n> > > private:\n> > > \tstd::shared_ptr camera_;\n> > > \tstd::map>> buffers_;\n> > > +\tstd::map> availableBuffers_;\n> > > };\n> > >\n> > > } /* namespace libcamera */\n> > > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp\n> > > index 7ed80011c845..7429d6b9edb7 100644\n> > > --- a/src/libcamera/framebuffer_allocator.cpp\n> > > +++ b/src/libcamera/framebuffer_allocator.cpp\n> > > @@ -64,7 +64,7 @@ FrameBufferAllocator::FrameBufferAllocator(std::shared_ptr camera)\n> > >\n> > > FrameBufferAllocator::~FrameBufferAllocator()\n> > > {\n> > > -\tbuffers_.clear();\n> > > +\tclear();\n> > > }\n> > >\n> > > /**\n> > > @@ -93,11 +93,17 @@ int FrameBufferAllocator::allocate(Stream *stream)\n> > > \t}\n> > >\n> > > \tint ret = camera_->exportFrameBuffers(stream, &buffers_[stream]);\n> > > -\tif (ret == -EINVAL)\n> > > +\tif (ret == -EINVAL) {\n> > > \t\tLOG(Allocator, Error)\n> > > \t\t\t<< \"Stream is not part of \" << camera_->id()\n> > > \t\t\t<< \" active configuration\";\n> > > -\treturn ret;\n> > > +\t\treturn ret;\n> > > +\t}\n> > > +\n> > > +\tfor (const auto &buffer : buffers_[stream])\n> > > +\t\tavailableBuffers_[stream].push(buffer.get());\n> > > +\n> > > +\treturn 0;\n> > > }\n> > >\n> > > /**\n> > > @@ -122,6 +128,9 @@ int FrameBufferAllocator::free(Stream *stream)\n> > > \tbuffers.clear();\n> > > \tbuffers_.erase(iter);\n> > >\n> > > +\tavailableBuffers_[stream] = {};\n> > > +\tavailableBuffers_.erase(availableBuffers_.find(stream));\n> > > +\n> > > \treturn 0;\n> > > }\n> > >\n> > > @@ -131,6 +140,7 @@ int FrameBufferAllocator::free(Stream *stream)\n> > > void FrameBufferAllocator::clear()\n> > > {\n> > > \tbuffers_.clear();\n> > > +\tavailableBuffers_.clear();\n> > > }\n> > >\n> > > /**\n> > > @@ -162,4 +172,48 @@ FrameBufferAllocator::buffers(Stream *stream) const\n> > > \treturn iter->second;\n> > > }\n> > >\n> > > +/**\n> > > + * \\brief Get a pointer to a \\a buffer for the \\a stream\n> > > + * \\param[in] stream The stream to get a buffer for\n> > > + *\n> > > + * The method returns a pointer to a FrameBuffer but does transfer the buffer\n> > > + * ownership to the caller: the returned pointer remains valid until the\n> > > + * FrameBufferAllocator does not get deleted or the allocated buffers do not get\n> > > + * released with a call for free() or clear().\n> > > + *\n> > > + * \\return A FrameBuffer pointer or nullptr if the no buffers is available\n> > > + */\n> > > +FrameBuffer *FrameBufferAllocator::getBuffer(Stream *stream)\n> > > +{\n> > > +\tif (!allocated() || buffers_[stream].empty())\n> > > +\t\treturn nullptr;\n> > > +\n> > > +\tFrameBuffer *frameBuffer = availableBuffers_[stream].front();\n> > > +\tavailableBuffers_[stream].pop();\n> > > +\n> > > +\treturn frameBuffer;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Return a \\a buffer to the list of buffers available for the a \\a stream\n> > > + * \\param[in] stream The stream to return buffer to\n> > > + * \\param[in] buffer The buffer to return\n> > > + */\n> > > +void FrameBufferAllocator::returnBuffer(Stream *stream, FrameBuffer *buffer)\n> > > +{\n> > > +\tif (!allocated())\n> > > +\t\treturn;\n> > > +\n> > > +\tfor (const auto &b : buffers_[stream]) {\n> > > +\t\t/*\n> > > +\t\t * Return the buffer to the available queue only if it was part\n> > > +\t\t * of the vector of buffers allocated for the Stream.\n> > > +\t\t */\n> > > +\t\tif (b.get() != buffer)\n> > > +\t\t\tcontinue;\n> > > +\n> > > +\t\tavailableBuffers_[stream].push(buffer);\n> > > +\t}\n> > > +}\n> > > +\n> > > } /* namespace libcamera */\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","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 F23C8BDB1D\n\tfor ;\n\tThu, 10 Sep 2020 13:02:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7D3C562D68;\n\tThu, 10 Sep 2020 15:02:59 +0200 (CEST)","from mail-lj1-x241.google.com (mail-lj1-x241.google.com\n\t[IPv6:2a00:1450:4864:20::241])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C080E62D38\n\tfor ;\n\tThu, 10 Sep 2020 15:02:57 +0200 (CEST)","by mail-lj1-x241.google.com with SMTP id a15so8094482ljk.2\n\tfor ;\n\tThu, 10 Sep 2020 06:02:57 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tk14sm1345974lfm.90.2020.09.10.06.02.55\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 10 Sep 2020 06:02:56 -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=\"XwzNbaYJ\"; 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=Z2MXkCQVM+68kSoEw4QpEP4273kiTy1Zo0BVh9+lX2E=;\n\tb=XwzNbaYJZDYiVzW8bc7mnzthAHLWKFk14RfAyTCJrP+19691LTWs7+mWNkFRvmAR9Z\n\tE1ZtJGV8gnsbdwa7YL43AMCUzxY7JNIEd9/GSYWN46LNt1Bc8vuSolsdcFr1SFwXeExX\n\tpyeqGMrCx39eRDgIKb4O4Rm2kqjJtvKLAGkCVHJKmALuc4NfOi4DplTD8VOcgIGsIbYv\n\txwiACl8/K3dhQR08n9rt0ZSMwX4PrqCQp46pZjEHfPKzMSeKkNFBWfuGb7Wt7FGAF+E+\n\t+OOmjsO6XEpwGkd7M3keMawUc8PY6ytVALEBPN+eXEMfXt8GRZUqIbM9A2A5CzOVbfsx\n\tqDaA==","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=Z2MXkCQVM+68kSoEw4QpEP4273kiTy1Zo0BVh9+lX2E=;\n\tb=GckQiDozos7jOStRHGlh3Ji+ymwkokFjBFvbcmsWPAqZiZciXMAvPw61AvOoKJ7Hg6\n\tWk2cq0zyTGcBKiZzDDJjnQhNDrR2LbTALm1hMwlrCZaTkOg0Wa9SBYeY4UqEVmDbxPCp\n\tbto1un0xldhiFWwZZfRZJFCYpPJewj5w3EwVnL0L7+ojXEnHewP4sFUeKmRpVS38qpZS\n\tmp3pLCHH2WkWsoJB7Sl/PipvTnhIiBVC94Jm4b9YmVgLwGu9lbqWWoygS9LYMEr4Xzm5\n\tem/DAnYNjhoJErKlX/XgwVt4W6umA/nFGCcOZ2fHzbhKqXfKefdn0Esw4vt600s6LWTt\n\tgrOg==","X-Gm-Message-State":"AOAM5334yKgOZ81Eu2CUNk67hAnOWPvgIm8+8jUniTBQdgpmjdIfcWhe\n\thiQfPrTIeLd51QGlUeJEihtHmkg9aPQo3Q==","X-Google-Smtp-Source":"ABdhPJyhXOMAOAW2/plvMC77LN4ahWKF2qN6mYF5t5YrNpwnW3TYMF7W/n3Mv1mx5yAcsBgqgX1ZLQ==","X-Received":"by 2002:a2e:800e:: with SMTP id\n\tj14mr4315533ljg.145.1599742977155; \n\tThu, 10 Sep 2020 06:02:57 -0700 (PDT)","Date":"Thu, 10 Sep 2020 15:02:55 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"<20200910130255.GW4095624@oden.dyn.berto.se>","References":"<20200909155457.153907-1-jacopo@jmondi.org>\n\t<20200909155457.153907-6-jacopo@jmondi.org>\n\t<20200910113016.GR4095624@oden.dyn.berto.se>\n\t<20200910122746.le3ndavmu6btkouk@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200910122746.le3ndavmu6btkouk@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 5/8] libcamera: framebuffer_allocator:\n\tGet and return buffers","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":12430,"web_url":"https://patchwork.libcamera.org/comment/12430/","msgid":"<20200910132255.pa4jos6xlellzrxh@uno.localdomain>","date":"2020-09-10T13:22:55","subject":"Re: [libcamera-devel] [PATCH 5/8] libcamera: framebuffer_allocator:\n\tGet and return buffers","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Thu, Sep 10, 2020 at 03:02:55PM +0200, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> On 2020-09-10 14:27:46 +0200, Jacopo Mondi wrote:\n> > Hi Niklas,\n> >\n> > On Thu, Sep 10, 2020 at 01:30:16PM +0200, Niklas Söderlund wrote:\n> > > Hi Jacopo,\n> > >\n> > > Thanks for your work.\n> > >\n> > > On 2020-09-09 17:54:54 +0200, Jacopo Mondi wrote:\n> > > > Add to the FrameBufferAllocator class two methods to get and\n> > > > return buffers from the pool of buffers allocated for a Stream.\n> > > >\n> > > > The two methods return pointer to the allocated buffers without\n> > > > transferring ownership to the caller.\n> > >\n> > > I'm sorry I don't like this patch. If it was an interface that was\n> > > internal I would be more OK with this change. The FrameBufferAllocator\n> > > is facing applications and this change introduces a duality between the\n> > > new getBuffer() and the existing buffers() functionality.\n> > >\n> > > I think it creates complexity in this user-facing API which is not\n> > > really needed. How about creating a HALFrameBufferAllocator that is\n> > > local to the HAL and inherits from FrameBufferAllocator while adding the\n> > > features you want?\n> >\n> > That would end up repeating the same thing we do in pipeline handlers\n> > here. Actually I found safer to provide a get/return interface than\n> > giving applications full access to the pool and let them deal with\n> > that. At the same time I won't prevent that completely by removing\n> > buffers(), but I don't see why the 'duality' is an issue.\n> >\n> > I'm more than open to discuss the implementation which is surely\n> > improvable and if we want to implement this feature in\n> > FrameBufferAllocator or as you suggested sub-class it somehow, but I'm\n> > not sure why the problem lies in having the three methods.\n>\n> My concern is having an application call getBuffer() to prepare one set\n> of Requests, maybe one with a viewfinder + raw and then iterating over\n> buffers() to create as many Requests with just viewfinder as available.\n> As buffers() don't consider if it have \"given\" out any buffer prior to\n> the buffers() call we have the potential to have Requests that can't be\n> queued at the same time as they contain a subset of the same buffers. I\n> fear this could get even worse once Request becomes reusable.\n>\n\nTrue that a good API is one you cannot get wrong, but if an\napplication behaves like that [1] it means it -really- is trying to\nshoot itself in the foot. I know it's a thin line though..\n\nI won't push for this, but if asked which interface I would prefer, I\nwould ask for a get/return one instead of a \"here you have all you buffers\"\none. There's also the question about buffer ownership, which are\nstored as unique_ptr<> and thus connected to the lifetime of the\nallocator. A 'get all the buffers' interface would allow application\nto move ownership to themselves and really screw up. A stricter\ninterface would allow us to decide what to do: right now we 'borrow'\nthe buffer ownership but we might as well decide to transfer that,\nbut we have a single behavior and I think that's better.\n\nThanks\n j\n\n[1] I'm not sure I got your example fully. Are you suggesting an\napplication might initially use getBuffer/returnBuffer then decide to\naccess the buffer vector returned by buffers() and use all of them\nignoring those are the same buffers it already got access to through\nthe get/return API ?\n\n> Understand me correctly, I'm not advocating in favor of buffers() or\n> getBuffers() interface. I do however think we should only have one as it\n> makes the interface easier to use for applications. As you suggest this\n> new interface could replace boilerplait code in pipelines and possibly\n> even applications to maybe it should replace the old buffers()\n> interface?\n>\n> >\n> > >\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > include/libcamera/framebuffer_allocator.h | 5 ++\n> > > > src/libcamera/framebuffer_allocator.cpp | 60 +++++++++++++++++++++--\n> > > > 2 files changed, 62 insertions(+), 3 deletions(-)\n> > > >\n> > > > diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h\n> > > > index 2a4d538a0cb2..1f3f10d4ec03 100644\n> > > > --- a/include/libcamera/framebuffer_allocator.h\n> > > > +++ b/include/libcamera/framebuffer_allocator.h\n> > > > @@ -9,6 +9,7 @@\n> > > >\n> > > > #include \n> > > > #include \n> > > > +#include \n> > > > #include \n> > > >\n> > > > namespace libcamera {\n> > > > @@ -33,9 +34,13 @@ public:\n> > > > \tbool allocated() const { return !buffers_.empty(); }\n> > > > \tconst std::vector> &buffers(Stream *stream) const;\n> > > >\n> > > > +\tFrameBuffer *getBuffer(Stream *stream);\n> > > > +\tvoid returnBuffer(Stream *stream, FrameBuffer *buffer);\n> > > > +\n> > > > private:\n> > > > \tstd::shared_ptr camera_;\n> > > > \tstd::map>> buffers_;\n> > > > +\tstd::map> availableBuffers_;\n> > > > };\n> > > >\n> > > > } /* namespace libcamera */\n> > > > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp\n> > > > index 7ed80011c845..7429d6b9edb7 100644\n> > > > --- a/src/libcamera/framebuffer_allocator.cpp\n> > > > +++ b/src/libcamera/framebuffer_allocator.cpp\n> > > > @@ -64,7 +64,7 @@ FrameBufferAllocator::FrameBufferAllocator(std::shared_ptr camera)\n> > > >\n> > > > FrameBufferAllocator::~FrameBufferAllocator()\n> > > > {\n> > > > -\tbuffers_.clear();\n> > > > +\tclear();\n> > > > }\n> > > >\n> > > > /**\n> > > > @@ -93,11 +93,17 @@ int FrameBufferAllocator::allocate(Stream *stream)\n> > > > \t}\n> > > >\n> > > > \tint ret = camera_->exportFrameBuffers(stream, &buffers_[stream]);\n> > > > -\tif (ret == -EINVAL)\n> > > > +\tif (ret == -EINVAL) {\n> > > > \t\tLOG(Allocator, Error)\n> > > > \t\t\t<< \"Stream is not part of \" << camera_->id()\n> > > > \t\t\t<< \" active configuration\";\n> > > > -\treturn ret;\n> > > > +\t\treturn ret;\n> > > > +\t}\n> > > > +\n> > > > +\tfor (const auto &buffer : buffers_[stream])\n> > > > +\t\tavailableBuffers_[stream].push(buffer.get());\n> > > > +\n> > > > +\treturn 0;\n> > > > }\n> > > >\n> > > > /**\n> > > > @@ -122,6 +128,9 @@ int FrameBufferAllocator::free(Stream *stream)\n> > > > \tbuffers.clear();\n> > > > \tbuffers_.erase(iter);\n> > > >\n> > > > +\tavailableBuffers_[stream] = {};\n> > > > +\tavailableBuffers_.erase(availableBuffers_.find(stream));\n> > > > +\n> > > > \treturn 0;\n> > > > }\n> > > >\n> > > > @@ -131,6 +140,7 @@ int FrameBufferAllocator::free(Stream *stream)\n> > > > void FrameBufferAllocator::clear()\n> > > > {\n> > > > \tbuffers_.clear();\n> > > > +\tavailableBuffers_.clear();\n> > > > }\n> > > >\n> > > > /**\n> > > > @@ -162,4 +172,48 @@ FrameBufferAllocator::buffers(Stream *stream) const\n> > > > \treturn iter->second;\n> > > > }\n> > > >\n> > > > +/**\n> > > > + * \\brief Get a pointer to a \\a buffer for the \\a stream\n> > > > + * \\param[in] stream The stream to get a buffer for\n> > > > + *\n> > > > + * The method returns a pointer to a FrameBuffer but does transfer the buffer\n> > > > + * ownership to the caller: the returned pointer remains valid until the\n> > > > + * FrameBufferAllocator does not get deleted or the allocated buffers do not get\n> > > > + * released with a call for free() or clear().\n> > > > + *\n> > > > + * \\return A FrameBuffer pointer or nullptr if the no buffers is available\n> > > > + */\n> > > > +FrameBuffer *FrameBufferAllocator::getBuffer(Stream *stream)\n> > > > +{\n> > > > +\tif (!allocated() || buffers_[stream].empty())\n> > > > +\t\treturn nullptr;\n> > > > +\n> > > > +\tFrameBuffer *frameBuffer = availableBuffers_[stream].front();\n> > > > +\tavailableBuffers_[stream].pop();\n> > > > +\n> > > > +\treturn frameBuffer;\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\brief Return a \\a buffer to the list of buffers available for the a \\a stream\n> > > > + * \\param[in] stream The stream to return buffer to\n> > > > + * \\param[in] buffer The buffer to return\n> > > > + */\n> > > > +void FrameBufferAllocator::returnBuffer(Stream *stream, FrameBuffer *buffer)\n> > > > +{\n> > > > +\tif (!allocated())\n> > > > +\t\treturn;\n> > > > +\n> > > > +\tfor (const auto &b : buffers_[stream]) {\n> > > > +\t\t/*\n> > > > +\t\t * Return the buffer to the available queue only if it was part\n> > > > +\t\t * of the vector of buffers allocated for the Stream.\n> > > > +\t\t */\n> > > > +\t\tif (b.get() != buffer)\n> > > > +\t\t\tcontinue;\n> > > > +\n> > > > +\t\tavailableBuffers_[stream].push(buffer);\n> > > > +\t}\n> > > > +}\n> > > > +\n> > > > } /* namespace libcamera */\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> --\n> Regards,\n> Niklas Söderlund","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 7A3C9C3B5B\n\tfor ;\n\tThu, 10 Sep 2020 13:19:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0D24062D53;\n\tThu, 10 Sep 2020 15:19:09 +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 1D4B862C43\n\tfor ;\n\tThu, 10 Sep 2020 15:19:08 +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 relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 518C2FF80F;\n\tThu, 10 Sep 2020 13:19:06 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Thu, 10 Sep 2020 15:22:55 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200910132255.pa4jos6xlellzrxh@uno.localdomain>","References":"<20200909155457.153907-1-jacopo@jmondi.org>\n\t<20200909155457.153907-6-jacopo@jmondi.org>\n\t<20200910113016.GR4095624@oden.dyn.berto.se>\n\t<20200910122746.le3ndavmu6btkouk@uno.localdomain>\n\t<20200910130255.GW4095624@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200910130255.GW4095624@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH 5/8] libcamera: framebuffer_allocator:\n\tGet and return buffers","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=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12446,"web_url":"https://patchwork.libcamera.org/comment/12446/","msgid":"","date":"2020-09-11T08:12:35","subject":"Re: [libcamera-devel] [PATCH 5/8] libcamera: framebuffer_allocator:\n\tGet and return buffers","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 10:19 PM Jacopo Mondi wrote:\n>\n> Hi Niklas,\n>\n> On Thu, Sep 10, 2020 at 03:02:55PM +0200, Niklas Söderlund wrote:\n> > Hi Jacopo,\n> >\n> > On 2020-09-10 14:27:46 +0200, Jacopo Mondi wrote:\n> > > Hi Niklas,\n> > >\n> > > On Thu, Sep 10, 2020 at 01:30:16PM +0200, Niklas Söderlund wrote:\n> > > > Hi Jacopo,\n> > > >\n> > > > Thanks for your work.\n> > > >\n> > > > On 2020-09-09 17:54:54 +0200, Jacopo Mondi wrote:\n> > > > > Add to the FrameBufferAllocator class two methods to get and\n> > > > > return buffers from the pool of buffers allocated for a Stream.\n> > > > >\n> > > > > The two methods return pointer to the allocated buffers without\n> > > > > transferring ownership to the caller.\n> > > >\n> > > > I'm sorry I don't like this patch. If it was an interface that was\n> > > > internal I would be more OK with this change. The FrameBufferAllocator\n> > > > is facing applications and this change introduces a duality between the\n> > > > new getBuffer() and the existing buffers() functionality.\n> > > >\n> > > > I think it creates complexity in this user-facing API which is not\n> > > > really needed. How about creating a HALFrameBufferAllocator that is\n> > > > local to the HAL and inherits from FrameBufferAllocator while adding the\n> > > > features you want?\n> > >\n> > > That would end up repeating the same thing we do in pipeline handlers\n> > > here. Actually I found safer to provide a get/return interface than\n> > > giving applications full access to the pool and let them deal with\n> > > that. At the same time I won't prevent that completely by removing\n> > > buffers(), but I don't see why the 'duality' is an issue.\n> > >\n> > > I'm more than open to discuss the implementation which is surely\n> > > improvable and if we want to implement this feature in\n> > > FrameBufferAllocator or as you suggested sub-class it somehow, but I'm\n> > > not sure why the problem lies in having the three methods.\n> >\n> > My concern is having an application call getBuffer() to prepare one set\n> > of Requests, maybe one with a viewfinder + raw and then iterating over\n> > buffers() to create as many Requests with just viewfinder as available.\n> > As buffers() don't consider if it have \"given\" out any buffer prior to\n> > the buffers() call we have the potential to have Requests that can't be\n> > queued at the same time as they contain a subset of the same buffers. I\n> > fear this could get even worse once Request becomes reusable.\n> >\n>\n> True that a good API is one you cannot get wrong, but if an\n> application behaves like that [1] it means it -really- is trying to\n> shoot itself in the foot. I know it's a thin line though..\n>\n> I won't push for this, but if asked which interface I would prefer, I\n> would ask for a get/return one instead of a \"here you have all you buffers\"\n> one. There's also the question about buffer ownership, which are\n> stored as unique_ptr<> and thus connected to the lifetime of the\n> allocator. A 'get all the buffers' interface would allow application\n> to move ownership to themselves and really screw up. A stricter\n> interface would allow us to decide what to do: right now we 'borrow'\n> the buffer ownership but we might as well decide to transfer that,\n> but we have a single behavior and I think that's better.\n>\n\nI am ok with this API change.\nI can understand the opinions of both Jacopo and Niklas.\nSince buffers() returns const reference of vector, client\ncannot takes\nthe ownership by std::move(). Having Get/ReturnBuffer() and buffers() should not\ngo wrong if the client adopts either way.\nThe thing is more like, where the buffer usage is managed, in client\n(with buffers()), or\nin FrameBufferAllocator (with Get/ReturnBuffer()).\nFor simplicity, I think eventually FrameBufferAllocator should have\nGetBuffer() and ReturnBuffer().\n\n> Thanks\n> j\n>\n> [1] I'm not sure I got your example fully. Are you suggesting an\n> application might initially use getBuffer/returnBuffer then decide to\n> access the buffer vector returned by buffers() and use all of them\n> ignoring those are the same buffers it already got access to through\n> the get/return API ?\n>\n> > Understand me correctly, I'm not advocating in favor of buffers() or\n> > getBuffers() interface. I do however think we should only have one as it\n> > makes the interface easier to use for applications. As you suggest this\n> > new interface could replace boilerplait code in pipelines and possibly\n> > even applications to maybe it should replace the old buffers()\n> > interface?\n> >\n> > >\n> > > >\n> > > > >\n> > > > > Signed-off-by: Jacopo Mondi \n> > > > > ---\n> > > > > include/libcamera/framebuffer_allocator.h | 5 ++\n> > > > > src/libcamera/framebuffer_allocator.cpp | 60 +++++++++++++++++++++--\n> > > > > 2 files changed, 62 insertions(+), 3 deletions(-)\n> > > > >\n> > > > > diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h\n> > > > > index 2a4d538a0cb2..1f3f10d4ec03 100644\n> > > > > --- a/include/libcamera/framebuffer_allocator.h\n> > > > > +++ b/include/libcamera/framebuffer_allocator.h\n> > > > > @@ -9,6 +9,7 @@\n> > > > >\n> > > > > #include \n> > > > > #include \n> > > > > +#include \n> > > > > #include \n> > > > >\n> > > > > namespace libcamera {\n> > > > > @@ -33,9 +34,13 @@ public:\n> > > > > bool allocated() const { return !buffers_.empty(); }\n> > > > > const std::vector> &buffers(Stream *stream) const;\n> > > > >\n> > > > > + FrameBuffer *getBuffer(Stream *stream);\n> > > > > + void returnBuffer(Stream *stream, FrameBuffer *buffer);\n> > > > > +\n> > > > > private:\n> > > > > std::shared_ptr camera_;\n> > > > > std::map>> buffers_;\n> > > > > + std::map> availableBuffers_;\n> > > > > };\n> > > > >\n\nI think we don't have to think of order. So how about using\nstd::vector and use the last entry of the vector?\n\n> > > > > } /* namespace libcamera */\n> > > > > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp\n> > > > > index 7ed80011c845..7429d6b9edb7 100644\n> > > > > --- a/src/libcamera/framebuffer_allocator.cpp\n> > > > > +++ b/src/libcamera/framebuffer_allocator.cpp\n> > > > > @@ -64,7 +64,7 @@ FrameBufferAllocator::FrameBufferAllocator(std::shared_ptr camera)\n> > > > >\n> > > > > FrameBufferAllocator::~FrameBufferAllocator()\n> > > > > {\n> > > > > - buffers_.clear();\n> > > > > + clear();\n> > > > > }\n> > > > >\n\nclear() is unnecessary.\n\n> > > > > /**\n> > > > > @@ -93,11 +93,17 @@ int FrameBufferAllocator::allocate(Stream *stream)\n> > > > > }\n> > > > >\n> > > > > int ret = camera_->exportFrameBuffers(stream, &buffers_[stream]);\n> > > > > - if (ret == -EINVAL)\n> > > > > + if (ret == -EINVAL) {\n> > > > > LOG(Allocator, Error)\n> > > > > << \"Stream is not part of \" << camera_->id()\n> > > > > << \" active configuration\";\n> > > > > - return ret;\n> > > > > + return ret;\n> > > > > + }\n> > > > > +\n> > > > > + for (const auto &buffer : buffers_[stream])\n> > > > > + availableBuffers_[stream].push(buffer.get());\n> > > > > +\n> > > > > + return 0;\n> > > > > }\n> > > > >\n> > > > > /**\n> > > > > @@ -122,6 +128,9 @@ int FrameBufferAllocator::free(Stream *stream)\n> > > > > buffers.clear();\n> > > > > buffers_.erase(iter);\n> > > > >\n> > > > > + availableBuffers_[stream] = {};\n> > > > > + availableBuffers_.erase(availableBuffers_.find(stream));\n> > > > > +\n\nI would just availableBuffers_.erase(stream);\nditto to buffers_.\n\n> > > > > return 0;\n> > > > > }\n> > > > >\n> > > > > @@ -131,6 +140,7 @@ int FrameBufferAllocator::free(Stream *stream)\n> > > > > void FrameBufferAllocator::clear()\n> > > > > {\n> > > > > buffers_.clear();\n> > > > > + availableBuffers_.clear();\n> > > > > }\n> > > > >\n> > > > > /**\n> > > > > @@ -162,4 +172,48 @@ FrameBufferAllocator::buffers(Stream *stream) const\n> > > > > return iter->second;\n> > > > > }\n> > > > >\n> > > > > +/**\n> > > > > + * \\brief Get a pointer to a \\a buffer for the \\a stream\n> > > > > + * \\param[in] stream The stream to get a buffer for\n> > > > > + *\n> > > > > + * The method returns a pointer to a FrameBuffer but does transfer the buffer\n> > > > > + * ownership to the caller: the returned pointer remains valid until the\n> > > > > + * FrameBufferAllocator does not get deleted or the allocated buffers do not get\n> > > > > + * released with a call for free() or clear().\n> > > > > + *\n> > > > > + * \\return A FrameBuffer pointer or nullptr if the no buffers is available\n> > > > > + */\n> > > > > +FrameBuffer *FrameBufferAllocator::getBuffer(Stream *stream)\n> > > > > +{\n> > > > > + if (!allocated() || buffers_[stream].empty())\n> > > > > + return nullptr;\n> > > > > +\n> > > > > + FrameBuffer *frameBuffer = availableBuffers_[stream].front();\n> > > > > + availableBuffers_[stream].pop();\n> > > > > +\n> > > > > + return frameBuffer;\n> > > > > +}\n> > > > > +\n> > > > > +/**\n> > > > > + * \\brief Return a \\a buffer to the list of buffers available for the a \\a stream\n> > > > > + * \\param[in] stream The stream to return buffer to\n> > > > > + * \\param[in] buffer The buffer to return\n> > > > > + */\n> > > > > +void FrameBufferAllocator::returnBuffer(Stream *stream, FrameBuffer *buffer)\n> > > > > +{\n> > > > > + if (!allocated())\n> > > > > + return;\n> > > > > +\n> > > > > + for (const auto &b : buffers_[stream]) {\n> > > > > + /*\n> > > > > + * Return the buffer to the available queue only if it was part\n> > > > > + * of the vector of buffers allocated for the Stream.\n> > > > > + */\n> > > > > + if (b.get() != buffer)\n> > > > > + continue;\n> > > > > +\n> > > > > + availableBuffers_[stream].push(buffer);\n> > > > > + }\n> > > > > +}\n> > > > > +\n\nI expect you may want to do as below. buffers_[stream] must not have\nthe same buffers because they are unique_ptr.\nconst auto& buffers = buffers_[stream];\nif (std::find_if(buffers.begin(), buffers.end(), [buffer](const auto&\nb) { return b.get() == buffer; } )) {\n availableBuffers_[stream].push(buffer);\n}\n\n> > > > > } /* namespace libcamera */\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> > --\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 6B272C3B5B\n\tfor ;\n\tFri, 11 Sep 2020 08:12:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0130062D53;\n\tFri, 11 Sep 2020 10:12:47 +0200 (CEST)","from mail-ed1-x542.google.com (mail-ed1-x542.google.com\n\t[IPv6:2a00:1450:4864:20::542])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B875962901\n\tfor ;\n\tFri, 11 Sep 2020 10:12:46 +0200 (CEST)","by mail-ed1-x542.google.com with SMTP id g4so9101424edk.0\n\tfor ;\n\tFri, 11 Sep 2020 01:12:46 -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=\"YDqcvA8W\"; 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=O8/HjdGplkLbD1arhAnNCj36gVcQGAIT51WL8A85nPE=;\n\tb=YDqcvA8Wy0JG77Bv441jiBb58scvsstKl+i4AUt1vslyHkmvDbVaf7Rf+1PMZ1Ntql\n\tn1HdNrIdpw1yr2XotD+adpVTqQHR0KSkGEm3B1CWkUNavnI9fruDB5uA5qE+y8debkqY\n\tvrWPM2HXQBUq9JBjKUDiJ8CscvQrt0knb14Vw=","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=O8/HjdGplkLbD1arhAnNCj36gVcQGAIT51WL8A85nPE=;\n\tb=rW2TYms+RSiSjXS5Z8TCjILEIz3Bslt6ASwOqIqpiCLQi5ZUof0B04TDO6p6l9eJSj\n\tUPXscBE64qsk5hgizgfo/AJedJWSNZLYJWbmWMKKojTuQMl9s9Sxwl/Gw9cTR7cqwtjV\n\tQ+QeZtKUFwYxMLTJuusTrgpf+XOUfxCn4MvEaWFA9r7S8cQe7q8W95iMy32mW0JV/yWQ\n\tGQLmZQ5vkSbt/GDRnDNzuSC1ORvruLmqoVR6aJfP1g/PgaTyeo3hVB5Id9SfzcN4wvEK\n\tT45U59o10a+riV1ZKe5VtwCI1Kn9Ouc0zHbR90ka5R3dGu1IvInzP0LFTA6eX5/Zcbqs\n\tjz+A==","X-Gm-Message-State":"AOAM530tIpHZWPbaGm/X6w4rP6azymWTbmyL/uUqPxcKtxqRcGJibOAN\n\tskCszmlhjZ90zh/nGPtkbrPrl4yGuccZbJHD0e6+Lw==","X-Google-Smtp-Source":"ABdhPJwnvLo9slpvzJCBVj1MOC+YoYuOsPL2RbH2cTxbh7tH4q1D5fKUH7BE5Zx2PwKYvDr5K1HX8y/hCl/t3XQGlU0=","X-Received":"by 2002:a05:6402:70f:: with SMTP id\n\tw15mr829666edx.202.1599811966096; \n\tFri, 11 Sep 2020 01:12:46 -0700 (PDT)","MIME-Version":"1.0","References":"<20200909155457.153907-1-jacopo@jmondi.org>\n\t<20200909155457.153907-6-jacopo@jmondi.org>\n\t<20200910113016.GR4095624@oden.dyn.berto.se>\n\t<20200910122746.le3ndavmu6btkouk@uno.localdomain>\n\t<20200910130255.GW4095624@oden.dyn.berto.se>\n\t<20200910132255.pa4jos6xlellzrxh@uno.localdomain>","In-Reply-To":"<20200910132255.pa4jos6xlellzrxh@uno.localdomain>","From":"Hirokazu Honda ","Date":"Fri, 11 Sep 2020 17:12:35 +0900","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH 5/8] libcamera: framebuffer_allocator:\n\tGet and return buffers","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":12454,"web_url":"https://patchwork.libcamera.org/comment/12454/","msgid":"<20200911162504.flumjjap5haaqpfw@uno.localdomain>","date":"2020-09-11T16:25:04","subject":"Re: [libcamera-devel] [PATCH 5/8] libcamera: framebuffer_allocator:\n\tGet and return buffers","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 05:12:35PM +0900, Hirokazu Honda wrote:\n> On Thu, Sep 10, 2020 at 10:19 PM Jacopo Mondi wrote:\n> >\n> > Hi Niklas,\n> >\n> > On Thu, Sep 10, 2020 at 03:02:55PM +0200, Niklas Söderlund wrote:\n> > > Hi Jacopo,\n> > >\n> > > On 2020-09-10 14:27:46 +0200, Jacopo Mondi wrote:\n> > > > Hi Niklas,\n> > > >\n> > > > On Thu, Sep 10, 2020 at 01:30:16PM +0200, Niklas Söderlund wrote:\n> > > > > Hi Jacopo,\n> > > > >\n> > > > > Thanks for your work.\n> > > > >\n> > > > > On 2020-09-09 17:54:54 +0200, Jacopo Mondi wrote:\n> > > > > > Add to the FrameBufferAllocator class two methods to get and\n> > > > > > return buffers from the pool of buffers allocated for a Stream.\n> > > > > >\n> > > > > > The two methods return pointer to the allocated buffers without\n> > > > > > transferring ownership to the caller.\n> > > > >\n> > > > > I'm sorry I don't like this patch. If it was an interface that was\n> > > > > internal I would be more OK with this change. The FrameBufferAllocator\n> > > > > is facing applications and this change introduces a duality between the\n> > > > > new getBuffer() and the existing buffers() functionality.\n> > > > >\n> > > > > I think it creates complexity in this user-facing API which is not\n> > > > > really needed. How about creating a HALFrameBufferAllocator that is\n> > > > > local to the HAL and inherits from FrameBufferAllocator while adding the\n> > > > > features you want?\n> > > >\n> > > > That would end up repeating the same thing we do in pipeline handlers\n> > > > here. Actually I found safer to provide a get/return interface than\n> > > > giving applications full access to the pool and let them deal with\n> > > > that. At the same time I won't prevent that completely by removing\n> > > > buffers(), but I don't see why the 'duality' is an issue.\n> > > >\n> > > > I'm more than open to discuss the implementation which is surely\n> > > > improvable and if we want to implement this feature in\n> > > > FrameBufferAllocator or as you suggested sub-class it somehow, but I'm\n> > > > not sure why the problem lies in having the three methods.\n> > >\n> > > My concern is having an application call getBuffer() to prepare one set\n> > > of Requests, maybe one with a viewfinder + raw and then iterating over\n> > > buffers() to create as many Requests with just viewfinder as available.\n> > > As buffers() don't consider if it have \"given\" out any buffer prior to\n> > > the buffers() call we have the potential to have Requests that can't be\n> > > queued at the same time as they contain a subset of the same buffers. I\n> > > fear this could get even worse once Request becomes reusable.\n> > >\n> >\n> > True that a good API is one you cannot get wrong, but if an\n> > application behaves like that [1] it means it -really- is trying to\n> > shoot itself in the foot. I know it's a thin line though..\n> >\n> > I won't push for this, but if asked which interface I would prefer, I\n> > would ask for a get/return one instead of a \"here you have all you buffers\"\n> > one. There's also the question about buffer ownership, which are\n> > stored as unique_ptr<> and thus connected to the lifetime of the\n> > allocator. A 'get all the buffers' interface would allow application\n> > to move ownership to themselves and really screw up. A stricter\n> > interface would allow us to decide what to do: right now we 'borrow'\n> > the buffer ownership but we might as well decide to transfer that,\n> > but we have a single behavior and I think that's better.\n> >\n>\n> I am ok with this API change.\n> I can understand the opinions of both Jacopo and Niklas.\n> Since buffers() returns const reference of vector, client\n> cannot takes\n> the ownership by std::move(). Having Get/ReturnBuffer() and buffers() should not\n> go wrong if the client adopts either way.\n\nmmm, you're right here, the reference is a const, there's not risk of\nmessing up the buffer ownership..\n\n> The thing is more like, where the buffer usage is managed, in client\n> (with buffers()), or\n> in FrameBufferAllocator (with Get/ReturnBuffer()).\n> For simplicity, I think eventually FrameBufferAllocator should have\n> GetBuffer() and ReturnBuffer().\n>\n\nYes, I think getBuffer/returnBuffer provide a managed interface that\nease the application life, but at the same time I won't be super happy\nof preventing application to implement custom ones...\n\nNot sure, I still feel the two interfaces can live together, and if\nsomeone mis-uses the two it's the same kind of error that one would do\nif buffers are get but never returned == bad application code :)\n\n> > Thanks\n> > j\n> >\n> > [1] I'm not sure I got your example fully. Are you suggesting an\n> > application might initially use getBuffer/returnBuffer then decide to\n> > access the buffer vector returned by buffers() and use all of them\n> > ignoring those are the same buffers it already got access to through\n> > the get/return API ?\n> >\n> > > Understand me correctly, I'm not advocating in favor of buffers() or\n> > > getBuffers() interface. I do however think we should only have one as it\n> > > makes the interface easier to use for applications. As you suggest this\n> > > new interface could replace boilerplait code in pipelines and possibly\n> > > even applications to maybe it should replace the old buffers()\n> > > interface?\n> > >\n> > > >\n> > > > >\n> > > > > >\n> > > > > > Signed-off-by: Jacopo Mondi \n> > > > > > ---\n> > > > > > include/libcamera/framebuffer_allocator.h | 5 ++\n> > > > > > src/libcamera/framebuffer_allocator.cpp | 60 +++++++++++++++++++++--\n> > > > > > 2 files changed, 62 insertions(+), 3 deletions(-)\n> > > > > >\n> > > > > > diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h\n> > > > > > index 2a4d538a0cb2..1f3f10d4ec03 100644\n> > > > > > --- a/include/libcamera/framebuffer_allocator.h\n> > > > > > +++ b/include/libcamera/framebuffer_allocator.h\n> > > > > > @@ -9,6 +9,7 @@\n> > > > > >\n> > > > > > #include \n> > > > > > #include \n> > > > > > +#include \n> > > > > > #include \n> > > > > >\n> > > > > > namespace libcamera {\n> > > > > > @@ -33,9 +34,13 @@ public:\n> > > > > > bool allocated() const { return !buffers_.empty(); }\n> > > > > > const std::vector> &buffers(Stream *stream) const;\n> > > > > >\n> > > > > > + FrameBuffer *getBuffer(Stream *stream);\n> > > > > > + void returnBuffer(Stream *stream, FrameBuffer *buffer);\n> > > > > > +\n> > > > > > private:\n> > > > > > std::shared_ptr camera_;\n> > > > > > std::map>> buffers_;\n> > > > > > + std::map> availableBuffers_;\n> > > > > > };\n> > > > > >\n>\n> I think we don't have to think of order. So how about using\n> std::vector and use the last entry of the vector?\n>\n> > > > > > } /* namespace libcamera */\n> > > > > > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp\n> > > > > > index 7ed80011c845..7429d6b9edb7 100644\n> > > > > > --- a/src/libcamera/framebuffer_allocator.cpp\n> > > > > > +++ b/src/libcamera/framebuffer_allocator.cpp\n> > > > > > @@ -64,7 +64,7 @@ FrameBufferAllocator::FrameBufferAllocator(std::shared_ptr camera)\n> > > > > >\n> > > > > > FrameBufferAllocator::~FrameBufferAllocator()\n> > > > > > {\n> > > > > > - buffers_.clear();\n> > > > > > + clear();\n> > > > > > }\n> > > > > >\n>\n> clear() is unnecessary.\n>\n> > > > > > /**\n> > > > > > @@ -93,11 +93,17 @@ int FrameBufferAllocator::allocate(Stream *stream)\n> > > > > > }\n> > > > > >\n> > > > > > int ret = camera_->exportFrameBuffers(stream, &buffers_[stream]);\n> > > > > > - if (ret == -EINVAL)\n> > > > > > + if (ret == -EINVAL) {\n> > > > > > LOG(Allocator, Error)\n> > > > > > << \"Stream is not part of \" << camera_->id()\n> > > > > > << \" active configuration\";\n> > > > > > - return ret;\n> > > > > > + return ret;\n> > > > > > + }\n> > > > > > +\n> > > > > > + for (const auto &buffer : buffers_[stream])\n> > > > > > + availableBuffers_[stream].push(buffer.get());\n> > > > > > +\n> > > > > > + return 0;\n> > > > > > }\n> > > > > >\n> > > > > > /**\n> > > > > > @@ -122,6 +128,9 @@ int FrameBufferAllocator::free(Stream *stream)\n> > > > > > buffers.clear();\n> > > > > > buffers_.erase(iter);\n> > > > > >\n> > > > > > + availableBuffers_[stream] = {};\n> > > > > > + availableBuffers_.erase(availableBuffers_.find(stream));\n> > > > > > +\n>\n> I would just availableBuffers_.erase(stream);\n> ditto to buffers_.\n>\n> > > > > > return 0;\n> > > > > > }\n> > > > > >\n> > > > > > @@ -131,6 +140,7 @@ int FrameBufferAllocator::free(Stream *stream)\n> > > > > > void FrameBufferAllocator::clear()\n> > > > > > {\n> > > > > > buffers_.clear();\n> > > > > > + availableBuffers_.clear();\n> > > > > > }\n> > > > > >\n> > > > > > /**\n> > > > > > @@ -162,4 +172,48 @@ FrameBufferAllocator::buffers(Stream *stream) const\n> > > > > > return iter->second;\n> > > > > > }\n> > > > > >\n> > > > > > +/**\n> > > > > > + * \\brief Get a pointer to a \\a buffer for the \\a stream\n> > > > > > + * \\param[in] stream The stream to get a buffer for\n> > > > > > + *\n> > > > > > + * The method returns a pointer to a FrameBuffer but does transfer the buffer\n> > > > > > + * ownership to the caller: the returned pointer remains valid until the\n> > > > > > + * FrameBufferAllocator does not get deleted or the allocated buffers do not get\n> > > > > > + * released with a call for free() or clear().\n> > > > > > + *\n> > > > > > + * \\return A FrameBuffer pointer or nullptr if the no buffers is available\n> > > > > > + */\n> > > > > > +FrameBuffer *FrameBufferAllocator::getBuffer(Stream *stream)\n> > > > > > +{\n> > > > > > + if (!allocated() || buffers_[stream].empty())\n> > > > > > + return nullptr;\n> > > > > > +\n> > > > > > + FrameBuffer *frameBuffer = availableBuffers_[stream].front();\n> > > > > > + availableBuffers_[stream].pop();\n> > > > > > +\n> > > > > > + return frameBuffer;\n> > > > > > +}\n> > > > > > +\n> > > > > > +/**\n> > > > > > + * \\brief Return a \\a buffer to the list of buffers available for the a \\a stream\n> > > > > > + * \\param[in] stream The stream to return buffer to\n> > > > > > + * \\param[in] buffer The buffer to return\n> > > > > > + */\n> > > > > > +void FrameBufferAllocator::returnBuffer(Stream *stream, FrameBuffer *buffer)\n> > > > > > +{\n> > > > > > + if (!allocated())\n> > > > > > + return;\n> > > > > > +\n> > > > > > + for (const auto &b : buffers_[stream]) {\n> > > > > > + /*\n> > > > > > + * Return the buffer to the available queue only if it was part\n> > > > > > + * of the vector of buffers allocated for the Stream.\n> > > > > > + */\n> > > > > > + if (b.get() != buffer)\n> > > > > > + continue;\n> > > > > > +\n> > > > > > + availableBuffers_[stream].push(buffer);\n> > > > > > + }\n> > > > > > +}\n> > > > > > +\n>\n> I expect you may want to do as below. buffers_[stream] must not have\n> the same buffers because they are unique_ptr.\n> const auto& buffers = buffers_[stream];\n> if (std::find_if(buffers.begin(), buffers.end(), [buffer](const auto&\n> b) { return b.get() == buffer; } )) {\n> availableBuffers_[stream].push(buffer);\n> }\n>\n> > > > > > } /* namespace libcamera */\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> > > --\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 91490C3B5B\n\tfor ;\n\tFri, 11 Sep 2020 16:21:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0BDFD62D86;\n\tFri, 11 Sep 2020 18:21:17 +0200 (CEST)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2C58760534\n\tfor ;\n\tFri, 11 Sep 2020 18:21:16 +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 relay11.mail.gandi.net (Postfix) with ESMTPSA id 3E967100008;\n\tFri, 11 Sep 2020 16:21:14 +0000 (UTC)"],"Date":"Fri, 11 Sep 2020 18:25:04 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20200911162504.flumjjap5haaqpfw@uno.localdomain>","References":"<20200909155457.153907-1-jacopo@jmondi.org>\n\t<20200909155457.153907-6-jacopo@jmondi.org>\n\t<20200910113016.GR4095624@oden.dyn.berto.se>\n\t<20200910122746.le3ndavmu6btkouk@uno.localdomain>\n\t<20200910130255.GW4095624@oden.dyn.berto.se>\n\t<20200910132255.pa4jos6xlellzrxh@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 5/8] libcamera: framebuffer_allocator:\n\tGet and return buffers","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":12469,"web_url":"https://patchwork.libcamera.org/comment/12469/","msgid":"<20200912094918.pqkrtsu3mj252g7l@uno.localdomain>","date":"2020-09-12T09:49:18","subject":"Re: [libcamera-devel] [PATCH 5/8] libcamera: framebuffer_allocator:\n\tGet and return buffers","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n I missed part of your reply in the previous email\n\nLet me continue here :)\n\nOn Fri, Sep 11, 2020 at 06:25:04PM +0200, Jacopo Mondi wrote:\n> Hi Hiro,\n>\n> On Fri, Sep 11, 2020 at 05:12:35PM +0900, Hirokazu Honda wrote:\n> > On Thu, Sep 10, 2020 at 10:19 PM Jacopo Mondi wrote:\n> > >\n> > > Hi Niklas,\n> > >\n> > > On Thu, Sep 10, 2020 at 03:02:55PM +0200, Niklas Söderlund wrote:\n> > > > Hi Jacopo,\n> > > >\n> > > > On 2020-09-10 14:27:46 +0200, Jacopo Mondi wrote:\n> > > > > Hi Niklas,\n> > > > >\n> > > > > On Thu, Sep 10, 2020 at 01:30:16PM +0200, Niklas Söderlund wrote:\n> > > > > > Hi Jacopo,\n> > > > > >\n> > > > > > Thanks for your work.\n> > > > > >\n> > > > > > On 2020-09-09 17:54:54 +0200, Jacopo Mondi wrote:\n> > > > > > > Add to the FrameBufferAllocator class two methods to get and\n> > > > > > > return buffers from the pool of buffers allocated for a Stream.\n> > > > > > >\n> > > > > > > The two methods return pointer to the allocated buffers without\n> > > > > > > transferring ownership to the caller.\n> > > > > >\n> > > > > > I'm sorry I don't like this patch. If it was an interface that was\n> > > > > > internal I would be more OK with this change. The FrameBufferAllocator\n> > > > > > is facing applications and this change introduces a duality between the\n> > > > > > new getBuffer() and the existing buffers() functionality.\n> > > > > >\n> > > > > > I think it creates complexity in this user-facing API which is not\n> > > > > > really needed. How about creating a HALFrameBufferAllocator that is\n> > > > > > local to the HAL and inherits from FrameBufferAllocator while adding the\n> > > > > > features you want?\n> > > > >\n> > > > > That would end up repeating the same thing we do in pipeline handlers\n> > > > > here. Actually I found safer to provide a get/return interface than\n> > > > > giving applications full access to the pool and let them deal with\n> > > > > that. At the same time I won't prevent that completely by removing\n> > > > > buffers(), but I don't see why the 'duality' is an issue.\n> > > > >\n> > > > > I'm more than open to discuss the implementation which is surely\n> > > > > improvable and if we want to implement this feature in\n> > > > > FrameBufferAllocator or as you suggested sub-class it somehow, but I'm\n> > > > > not sure why the problem lies in having the three methods.\n> > > >\n> > > > My concern is having an application call getBuffer() to prepare one set\n> > > > of Requests, maybe one with a viewfinder + raw and then iterating over\n> > > > buffers() to create as many Requests with just viewfinder as available.\n> > > > As buffers() don't consider if it have \"given\" out any buffer prior to\n> > > > the buffers() call we have the potential to have Requests that can't be\n> > > > queued at the same time as they contain a subset of the same buffers. I\n> > > > fear this could get even worse once Request becomes reusable.\n> > > >\n> > >\n> > > True that a good API is one you cannot get wrong, but if an\n> > > application behaves like that [1] it means it -really- is trying to\n> > > shoot itself in the foot. I know it's a thin line though..\n> > >\n> > > I won't push for this, but if asked which interface I would prefer, I\n> > > would ask for a get/return one instead of a \"here you have all you buffers\"\n> > > one. There's also the question about buffer ownership, which are\n> > > stored as unique_ptr<> and thus connected to the lifetime of the\n> > > allocator. A 'get all the buffers' interface would allow application\n> > > to move ownership to themselves and really screw up. A stricter\n> > > interface would allow us to decide what to do: right now we 'borrow'\n> > > the buffer ownership but we might as well decide to transfer that,\n> > > but we have a single behavior and I think that's better.\n> > >\n> >\n> > I am ok with this API change.\n> > I can understand the opinions of both Jacopo and Niklas.\n> > Since buffers() returns const reference of vector, client\n> > cannot takes\n> > the ownership by std::move(). Having Get/ReturnBuffer() and buffers() should not\n> > go wrong if the client adopts either way.\n>\n> mmm, you're right here, the reference is a const, there's not risk of\n> messing up the buffer ownership..\n>\n> > The thing is more like, where the buffer usage is managed, in client\n> > (with buffers()), or\n> > in FrameBufferAllocator (with Get/ReturnBuffer()).\n> > For simplicity, I think eventually FrameBufferAllocator should have\n> > GetBuffer() and ReturnBuffer().\n> >\n>\n> Yes, I think getBuffer/returnBuffer provide a managed interface that\n> ease the application life, but at the same time I won't be super happy\n> of preventing application to implement custom ones...\n>\n> Not sure, I still feel the two interfaces can live together, and if\n> someone mis-uses the two it's the same kind of error that one would do\n> if buffers are get but never returned == bad application code :)\n>\n> > > Thanks\n> > > j\n> > >\n> > > [1] I'm not sure I got your example fully. Are you suggesting an\n> > > application might initially use getBuffer/returnBuffer then decide to\n> > > access the buffer vector returned by buffers() and use all of them\n> > > ignoring those are the same buffers it already got access to through\n> > > the get/return API ?\n> > >\n> > > > Understand me correctly, I'm not advocating in favor of buffers() or\n> > > > getBuffers() interface. I do however think we should only have one as it\n> > > > makes the interface easier to use for applications. As you suggest this\n> > > > new interface could replace boilerplait code in pipelines and possibly\n> > > > even applications to maybe it should replace the old buffers()\n> > > > interface?\n> > > >\n> > > > >\n> > > > > >\n> > > > > > >\n> > > > > > > Signed-off-by: Jacopo Mondi \n> > > > > > > ---\n> > > > > > > include/libcamera/framebuffer_allocator.h | 5 ++\n> > > > > > > src/libcamera/framebuffer_allocator.cpp | 60 +++++++++++++++++++++--\n> > > > > > > 2 files changed, 62 insertions(+), 3 deletions(-)\n> > > > > > >\n> > > > > > > diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h\n> > > > > > > index 2a4d538a0cb2..1f3f10d4ec03 100644\n> > > > > > > --- a/include/libcamera/framebuffer_allocator.h\n> > > > > > > +++ b/include/libcamera/framebuffer_allocator.h\n> > > > > > > @@ -9,6 +9,7 @@\n> > > > > > >\n> > > > > > > #include \n> > > > > > > #include \n> > > > > > > +#include \n> > > > > > > #include \n> > > > > > >\n> > > > > > > namespace libcamera {\n> > > > > > > @@ -33,9 +34,13 @@ public:\n> > > > > > > bool allocated() const { return !buffers_.empty(); }\n> > > > > > > const std::vector> &buffers(Stream *stream) const;\n> > > > > > >\n> > > > > > > + FrameBuffer *getBuffer(Stream *stream);\n> > > > > > > + void returnBuffer(Stream *stream, FrameBuffer *buffer);\n> > > > > > > +\n> > > > > > > private:\n> > > > > > > std::shared_ptr camera_;\n> > > > > > > std::map>> buffers_;\n> > > > > > > + std::map> availableBuffers_;\n> > > > > > > };\n> > > > > > >\n> >\n> > I think we don't have to think of order. So how about using\n> > std::vector and use the last entry of the vector?\n\nUsing the last entry doesn't seem the best idea, as you can only push\nto the end of an std::vector, so you could potentially cycle over a\nsingle or a limited set of buffers. It wonder about concurrency also,\nif an application access the end of the vector from two different\nthreads (is this possible?) we might need some kind of protection.\n\nOne possible way forward would be to push to the end of the vector and\npop from its front. We can reserve space in advance knowing the number\nof buffers, so that would be more efficient than a queue indeed.\n\n> >\n> > > > > > > } /* namespace libcamera */\n> > > > > > > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp\n> > > > > > > index 7ed80011c845..7429d6b9edb7 100644\n> > > > > > > --- a/src/libcamera/framebuffer_allocator.cpp\n> > > > > > > +++ b/src/libcamera/framebuffer_allocator.cpp\n> > > > > > > @@ -64,7 +64,7 @@ FrameBufferAllocator::FrameBufferAllocator(std::shared_ptr camera)\n> > > > > > >\n> > > > > > > FrameBufferAllocator::~FrameBufferAllocator()\n> > > > > > > {\n> > > > > > > - buffers_.clear();\n> > > > > > > + clear();\n> > > > > > > }\n> > > > > > >\n> >\n> > clear() is unnecessary.\n\nAs stated in the previous email, you're probably right\n\n> >\n> > > > > > > /**\n> > > > > > > @@ -93,11 +93,17 @@ int FrameBufferAllocator::allocate(Stream *stream)\n> > > > > > > }\n> > > > > > >\n> > > > > > > int ret = camera_->exportFrameBuffers(stream, &buffers_[stream]);\n> > > > > > > - if (ret == -EINVAL)\n> > > > > > > + if (ret == -EINVAL) {\n> > > > > > > LOG(Allocator, Error)\n> > > > > > > << \"Stream is not part of \" << camera_->id()\n> > > > > > > << \" active configuration\";\n> > > > > > > - return ret;\n> > > > > > > + return ret;\n> > > > > > > + }\n> > > > > > > +\n> > > > > > > + for (const auto &buffer : buffers_[stream])\n> > > > > > > + availableBuffers_[stream].push(buffer.get());\n> > > > > > > +\n> > > > > > > + return 0;\n> > > > > > > }\n> > > > > > >\n> > > > > > > /**\n> > > > > > > @@ -122,6 +128,9 @@ int FrameBufferAllocator::free(Stream *stream)\n> > > > > > > buffers.clear();\n> > > > > > > buffers_.erase(iter);\n> > > > > > >\n> > > > > > > + availableBuffers_[stream] = {};\n> > > > > > > + availableBuffers_.erase(availableBuffers_.find(stream));\n> > > > > > > +\n> >\n> > I would just availableBuffers_.erase(stream);\n\nCorrect\n\n> > ditto to buffers_.\n\nAlso probably correct as buffers_.erase(iter) would delete the vector\nof unique pointers there stored, causing memory to be released without\nhaving to\n\n\tstd::vector> &buffers = iter->second;\n\tbuffers.clear();\n\n> >\n> > > > > > > return 0;\n> > > > > > > }\n> > > > > > >\n> > > > > > > @@ -131,6 +140,7 @@ int FrameBufferAllocator::free(Stream *stream)\n> > > > > > > void FrameBufferAllocator::clear()\n> > > > > > > {\n> > > > > > > buffers_.clear();\n> > > > > > > + availableBuffers_.clear();\n> > > > > > > }\n> > > > > > >\n> > > > > > > /**\n> > > > > > > @@ -162,4 +172,48 @@ FrameBufferAllocator::buffers(Stream *stream) const\n> > > > > > > return iter->second;\n> > > > > > > }\n> > > > > > >\n> > > > > > > +/**\n> > > > > > > + * \\brief Get a pointer to a \\a buffer for the \\a stream\n> > > > > > > + * \\param[in] stream The stream to get a buffer for\n> > > > > > > + *\n> > > > > > > + * The method returns a pointer to a FrameBuffer but does transfer the buffer\n> > > > > > > + * ownership to the caller: the returned pointer remains valid until the\n> > > > > > > + * FrameBufferAllocator does not get deleted or the allocated buffers do not get\n> > > > > > > + * released with a call for free() or clear().\n> > > > > > > + *\n> > > > > > > + * \\return A FrameBuffer pointer or nullptr if the no buffers is available\n> > > > > > > + */\n> > > > > > > +FrameBuffer *FrameBufferAllocator::getBuffer(Stream *stream)\n> > > > > > > +{\n> > > > > > > + if (!allocated() || buffers_[stream].empty())\n> > > > > > > + return nullptr;\n> > > > > > > +\n> > > > > > > + FrameBuffer *frameBuffer = availableBuffers_[stream].front();\n> > > > > > > + availableBuffers_[stream].pop();\n> > > > > > > +\n> > > > > > > + return frameBuffer;\n> > > > > > > +}\n> > > > > > > +\n> > > > > > > +/**\n> > > > > > > + * \\brief Return a \\a buffer to the list of buffers available for the a \\a stream\n> > > > > > > + * \\param[in] stream The stream to return buffer to\n> > > > > > > + * \\param[in] buffer The buffer to return\n> > > > > > > + */\n> > > > > > > +void FrameBufferAllocator::returnBuffer(Stream *stream, FrameBuffer *buffer)\n> > > > > > > +{\n> > > > > > > + if (!allocated())\n> > > > > > > + return;\n> > > > > > > +\n> > > > > > > + for (const auto &b : buffers_[stream]) {\n> > > > > > > + /*\n> > > > > > > + * Return the buffer to the available queue only if it was part\n> > > > > > > + * of the vector of buffers allocated for the Stream.\n> > > > > > > + */\n> > > > > > > + if (b.get() != buffer)\n> > > > > > > + continue;\n> > > > > > > +\n> > > > > > > + availableBuffers_[stream].push(buffer);\n> > > > > > > + }\n> > > > > > > +}\n> > > > > > > +\n> >\n> > I expect you may want to do as below. buffers_[stream] must not have\n> > the same buffers because they are unique_ptr.\n> > const auto& buffers = buffers_[stream];\n> > if (std::find_if(buffers.begin(), buffers.end(), [buffer](const auto&\n> > b) { return b.get() == buffer; } )) {\n> > availableBuffers_[stream].push(buffer);\n> > }\n\nIf I got this right, your proposal is functionally equivalent but\nI find it less readable. This surely is me and my not being exactly in\nlove with the ability C++ has to make easy things look complex, but in\nthis case I find open coding much more explict that going through an\nstd function. I suspect we could discuss this for days,\nfor sure your proposal is much more C++-ish than mine, but I found\nopen coding more explicit.\n\nAnyway, seems like a minor style issue, or have I missed any\nfunctional change?\n\nThanks\n j\n\n\n> >\n> > > > > > > } /* namespace libcamera */\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> > > > --\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\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 A0603BF01C\n\tfor ;\n\tSat, 12 Sep 2020 09:45:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0A9CD62DA1;\n\tSat, 12 Sep 2020 11:45:31 +0200 (CEST)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 63F7A62B90\n\tfor ;\n\tSat, 12 Sep 2020 11:45:30 +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 relay10.mail.gandi.net (Postfix) with ESMTPSA id 58590240008;\n\tSat, 12 Sep 2020 09:45:28 +0000 (UTC)"],"Date":"Sat, 12 Sep 2020 11:49:18 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20200912094918.pqkrtsu3mj252g7l@uno.localdomain>","References":"<20200909155457.153907-1-jacopo@jmondi.org>\n\t<20200909155457.153907-6-jacopo@jmondi.org>\n\t<20200910113016.GR4095624@oden.dyn.berto.se>\n\t<20200910122746.le3ndavmu6btkouk@uno.localdomain>\n\t<20200910130255.GW4095624@oden.dyn.berto.se>\n\t<20200910132255.pa4jos6xlellzrxh@uno.localdomain>\n\t\n\t<20200911162504.flumjjap5haaqpfw@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200911162504.flumjjap5haaqpfw@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 5/8] libcamera: framebuffer_allocator:\n\tGet and return buffers","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":12470,"web_url":"https://patchwork.libcamera.org/comment/12470/","msgid":"<20200912114337.GA674140@oden.dyn.berto.se>","date":"2020-09-12T11:43:37","subject":"Re: [libcamera-devel] [PATCH 5/8] libcamera: framebuffer_allocator:\n\tGet and return buffers","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nOn 2020-09-10 15:22:55 +0200, Jacopo Mondi wrote:\n> Hi Niklas,\n> \n> On Thu, Sep 10, 2020 at 03:02:55PM +0200, Niklas Söderlund wrote:\n> > Hi Jacopo,\n> >\n> > On 2020-09-10 14:27:46 +0200, Jacopo Mondi wrote:\n> > > Hi Niklas,\n> > >\n> > > On Thu, Sep 10, 2020 at 01:30:16PM +0200, Niklas Söderlund wrote:\n> > > > Hi Jacopo,\n> > > >\n> > > > Thanks for your work.\n> > > >\n> > > > On 2020-09-09 17:54:54 +0200, Jacopo Mondi wrote:\n> > > > > Add to the FrameBufferAllocator class two methods to get and\n> > > > > return buffers from the pool of buffers allocated for a Stream.\n> > > > >\n> > > > > The two methods return pointer to the allocated buffers without\n> > > > > transferring ownership to the caller.\n> > > >\n> > > > I'm sorry I don't like this patch. If it was an interface that was\n> > > > internal I would be more OK with this change. The FrameBufferAllocator\n> > > > is facing applications and this change introduces a duality between the\n> > > > new getBuffer() and the existing buffers() functionality.\n> > > >\n> > > > I think it creates complexity in this user-facing API which is not\n> > > > really needed. How about creating a HALFrameBufferAllocator that is\n> > > > local to the HAL and inherits from FrameBufferAllocator while adding the\n> > > > features you want?\n> > >\n> > > That would end up repeating the same thing we do in pipeline handlers\n> > > here. Actually I found safer to provide a get/return interface than\n> > > giving applications full access to the pool and let them deal with\n> > > that. At the same time I won't prevent that completely by removing\n> > > buffers(), but I don't see why the 'duality' is an issue.\n> > >\n> > > I'm more than open to discuss the implementation which is surely\n> > > improvable and if we want to implement this feature in\n> > > FrameBufferAllocator or as you suggested sub-class it somehow, but I'm\n> > > not sure why the problem lies in having the three methods.\n> >\n> > My concern is having an application call getBuffer() to prepare one set\n> > of Requests, maybe one with a viewfinder + raw and then iterating over\n> > buffers() to create as many Requests with just viewfinder as available.\n> > As buffers() don't consider if it have \"given\" out any buffer prior to\n> > the buffers() call we have the potential to have Requests that can't be\n> > queued at the same time as they contain a subset of the same buffers. I\n> > fear this could get even worse once Request becomes reusable.\n> >\n> \n> True that a good API is one you cannot get wrong, but if an\n> application behaves like that [1] it means it -really- is trying to\n> shoot itself in the foot. I know it's a thin line though..\n\nI think we should aim to have an API where it's impossible to shoot \nyourself in the foot ;-)\n\n> \n> I won't push for this, but if asked which interface I would prefer, I\n> would ask for a get/return one instead of a \"here you have all you buffers\"\n> one. There's also the question about buffer ownership, which are\n> stored as unique_ptr<> and thus connected to the lifetime of the\n> allocator. A 'get all the buffers' interface would allow application\n> to move ownership to themselves and really screw up. A stricter\n> interface would allow us to decide what to do: right now we 'borrow'\n> the buffer ownership but we might as well decide to transfer that,\n> but we have a single behavior and I think that's better.\n\nI think it's a good point about ownership and I agree with you we can \nprobably improve and replace on the current buffers() interface. While \ndoing so I think we should also keep in mind that we aim to make \nRequests reusable as this may impact on how we wish to model the \nownership model.\n\n> \n> Thanks\n> j\n> \n> [1] I'm not sure I got your example fully. Are you suggesting an\n> application might initially use getBuffer/returnBuffer then decide to\n> access the buffer vector returned by buffers() and use all of them\n> ignoring those are the same buffers it already got access to through\n> the get/return API ?\n\nYes, sorry if I was unclear :-)\n\n> \n> > Understand me correctly, I'm not advocating in favor of buffers() or\n> > getBuffers() interface. I do however think we should only have one as it\n> > makes the interface easier to use for applications. As you suggest this\n> > new interface could replace boilerplait code in pipelines and possibly\n> > even applications to maybe it should replace the old buffers()\n> > interface?\n> >\n> > >\n> > > >\n> > > > >\n> > > > > Signed-off-by: Jacopo Mondi \n> > > > > ---\n> > > > > include/libcamera/framebuffer_allocator.h | 5 ++\n> > > > > src/libcamera/framebuffer_allocator.cpp | 60 +++++++++++++++++++++--\n> > > > > 2 files changed, 62 insertions(+), 3 deletions(-)\n> > > > >\n> > > > > diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h\n> > > > > index 2a4d538a0cb2..1f3f10d4ec03 100644\n> > > > > --- a/include/libcamera/framebuffer_allocator.h\n> > > > > +++ b/include/libcamera/framebuffer_allocator.h\n> > > > > @@ -9,6 +9,7 @@\n> > > > >\n> > > > > #include \n> > > > > #include \n> > > > > +#include \n> > > > > #include \n> > > > >\n> > > > > namespace libcamera {\n> > > > > @@ -33,9 +34,13 @@ public:\n> > > > > \tbool allocated() const { return !buffers_.empty(); }\n> > > > > \tconst std::vector> &buffers(Stream *stream) const;\n> > > > >\n> > > > > +\tFrameBuffer *getBuffer(Stream *stream);\n> > > > > +\tvoid returnBuffer(Stream *stream, FrameBuffer *buffer);\n> > > > > +\n> > > > > private:\n> > > > > \tstd::shared_ptr camera_;\n> > > > > \tstd::map>> buffers_;\n> > > > > +\tstd::map> availableBuffers_;\n> > > > > };\n> > > > >\n> > > > > } /* namespace libcamera */\n> > > > > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp\n> > > > > index 7ed80011c845..7429d6b9edb7 100644\n> > > > > --- a/src/libcamera/framebuffer_allocator.cpp\n> > > > > +++ b/src/libcamera/framebuffer_allocator.cpp\n> > > > > @@ -64,7 +64,7 @@ FrameBufferAllocator::FrameBufferAllocator(std::shared_ptr camera)\n> > > > >\n> > > > > FrameBufferAllocator::~FrameBufferAllocator()\n> > > > > {\n> > > > > -\tbuffers_.clear();\n> > > > > +\tclear();\n> > > > > }\n> > > > >\n> > > > > /**\n> > > > > @@ -93,11 +93,17 @@ int FrameBufferAllocator::allocate(Stream *stream)\n> > > > > \t}\n> > > > >\n> > > > > \tint ret = camera_->exportFrameBuffers(stream, &buffers_[stream]);\n> > > > > -\tif (ret == -EINVAL)\n> > > > > +\tif (ret == -EINVAL) {\n> > > > > \t\tLOG(Allocator, Error)\n> > > > > \t\t\t<< \"Stream is not part of \" << camera_->id()\n> > > > > \t\t\t<< \" active configuration\";\n> > > > > -\treturn ret;\n> > > > > +\t\treturn ret;\n> > > > > +\t}\n> > > > > +\n> > > > > +\tfor (const auto &buffer : buffers_[stream])\n> > > > > +\t\tavailableBuffers_[stream].push(buffer.get());\n> > > > > +\n> > > > > +\treturn 0;\n> > > > > }\n> > > > >\n> > > > > /**\n> > > > > @@ -122,6 +128,9 @@ int FrameBufferAllocator::free(Stream *stream)\n> > > > > \tbuffers.clear();\n> > > > > \tbuffers_.erase(iter);\n> > > > >\n> > > > > +\tavailableBuffers_[stream] = {};\n> > > > > +\tavailableBuffers_.erase(availableBuffers_.find(stream));\n> > > > > +\n> > > > > \treturn 0;\n> > > > > }\n> > > > >\n> > > > > @@ -131,6 +140,7 @@ int FrameBufferAllocator::free(Stream *stream)\n> > > > > void FrameBufferAllocator::clear()\n> > > > > {\n> > > > > \tbuffers_.clear();\n> > > > > +\tavailableBuffers_.clear();\n> > > > > }\n> > > > >\n> > > > > /**\n> > > > > @@ -162,4 +172,48 @@ FrameBufferAllocator::buffers(Stream *stream) const\n> > > > > \treturn iter->second;\n> > > > > }\n> > > > >\n> > > > > +/**\n> > > > > + * \\brief Get a pointer to a \\a buffer for the \\a stream\n> > > > > + * \\param[in] stream The stream to get a buffer for\n> > > > > + *\n> > > > > + * The method returns a pointer to a FrameBuffer but does transfer the buffer\n> > > > > + * ownership to the caller: the returned pointer remains valid until the\n> > > > > + * FrameBufferAllocator does not get deleted or the allocated buffers do not get\n> > > > > + * released with a call for free() or clear().\n> > > > > + *\n> > > > > + * \\return A FrameBuffer pointer or nullptr if the no buffers is available\n> > > > > + */\n> > > > > +FrameBuffer *FrameBufferAllocator::getBuffer(Stream *stream)\n> > > > > +{\n> > > > > +\tif (!allocated() || buffers_[stream].empty())\n> > > > > +\t\treturn nullptr;\n> > > > > +\n> > > > > +\tFrameBuffer *frameBuffer = availableBuffers_[stream].front();\n> > > > > +\tavailableBuffers_[stream].pop();\n> > > > > +\n> > > > > +\treturn frameBuffer;\n> > > > > +}\n> > > > > +\n> > > > > +/**\n> > > > > + * \\brief Return a \\a buffer to the list of buffers available for the a \\a stream\n> > > > > + * \\param[in] stream The stream to return buffer to\n> > > > > + * \\param[in] buffer The buffer to return\n> > > > > + */\n> > > > > +void FrameBufferAllocator::returnBuffer(Stream *stream, FrameBuffer *buffer)\n> > > > > +{\n> > > > > +\tif (!allocated())\n> > > > > +\t\treturn;\n> > > > > +\n> > > > > +\tfor (const auto &b : buffers_[stream]) {\n> > > > > +\t\t/*\n> > > > > +\t\t * Return the buffer to the available queue only if it was part\n> > > > > +\t\t * of the vector of buffers allocated for the Stream.\n> > > > > +\t\t */\n> > > > > +\t\tif (b.get() != buffer)\n> > > > > +\t\t\tcontinue;\n> > > > > +\n> > > > > +\t\tavailableBuffers_[stream].push(buffer);\n> > > > > +\t}\n> > > > > +}\n> > > > > +\n> > > > > } /* namespace libcamera */\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> > --\n> > Regards,\n> > Niklas Söderlund","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 495ECBF01C\n\tfor ;\n\tSat, 12 Sep 2020 11:43:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C8A1D62D5B;\n\tSat, 12 Sep 2020 13:43:41 +0200 (CEST)","from mail-lf1-x143.google.com (mail-lf1-x143.google.com\n\t[IPv6:2a00:1450:4864:20::143])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 732DA62C8C\n\tfor ;\n\tSat, 12 Sep 2020 13:43:39 +0200 (CEST)","by mail-lf1-x143.google.com with SMTP id y11so8524051lfl.5\n\tfor ;\n\tSat, 12 Sep 2020 04:43:39 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tr4sm1389966ljg.123.2020.09.12.04.43.37\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSat, 12 Sep 2020 04:43:37 -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=\"mOB+cGXq\"; 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=UqoLNCk3TOwwxvQnPNTuHiJqLni3FyNURC5n6FJ0QQg=;\n\tb=mOB+cGXqvNsOE1bPVh8PBd84Kx4IzHPGT/VmF+oRvlJBEgkkbkI8aZv3Iwym3Hi0HF\n\thhpmRV7bTGozxP+HUAmQg/ZUjCnlae75FQ01xMit4951RSeTVsDEz9V+zHgK7qvXuZnv\n\tcPgJcZao4CnrloF+jmcrGuCebbgn0Gk2jmhHDfls4jUViPH77x9S5koGGQdoCqneQMer\n\tLDhLdyUd1X9fr5CyikNLm+3ndQTKkW1cjmzgGeOnMzvATcsIJqnNSzt9/bLs47sdd1wv\n\tpLFdvbI+5QQvZUrstKYTxHZO5MV1QJkSu7GayDlPtk0gVEo7jHv3DYbEvsGrP8psVwfy\n\tQp2w==","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=UqoLNCk3TOwwxvQnPNTuHiJqLni3FyNURC5n6FJ0QQg=;\n\tb=cUhXYdTPBvnsAPMc34e3BJK5H7Pj/NUjqjq9b9b+etjMvrAJcN41Ytvi0k7DG8AB/j\n\tLWeGcnFCVEU97j5nvCT7uqNe0gv3ivuMXoJLnqS39SX1Tu256wLAEG+HciMXNyFWgDgY\n\tpfBIG2zVJtOA+o3FvI/4zMxHiJmolG/RDVMAJWcQQ4ykP1QkHbHfkXey7U2MJOpVWIVr\n\tG6ujsbJ/zUzdAVI7XJsEWHtpCfikbkdQet34APjL6Jxqdu0YlxnWvuD046rBF9Mz+LkK\n\tCQRIGPb7fuARLeslGMzNVI3IVx+1SX3h+HSGeRs9W4YBE5XULqqIZ9EWx39I8JpCXfwf\n\t4rjg==","X-Gm-Message-State":"AOAM533YhnJ58XuRcAPWT0pPz3fBbRpA7sipw7TT/RKv969KX8rhU6YF\n\tMr2wEIIlOKnHM+q/lTro2LYlEQ==","X-Google-Smtp-Source":"ABdhPJyBvIusRxg5bOemUGDK3UsT/lPQQZNrgzmC8mSeKDJSofgnVTiL7Bw7PqdNXui+VzuN9BzI5w==","X-Received":"by 2002:a19:4c87:: with SMTP id\n\tz129mr2118076lfa.189.1599911018417; \n\tSat, 12 Sep 2020 04:43:38 -0700 (PDT)","Date":"Sat, 12 Sep 2020 13:43:37 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"<20200912114337.GA674140@oden.dyn.berto.se>","References":"<20200909155457.153907-1-jacopo@jmondi.org>\n\t<20200909155457.153907-6-jacopo@jmondi.org>\n\t<20200910113016.GR4095624@oden.dyn.berto.se>\n\t<20200910122746.le3ndavmu6btkouk@uno.localdomain>\n\t<20200910130255.GW4095624@oden.dyn.berto.se>\n\t<20200910132255.pa4jos6xlellzrxh@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200910132255.pa4jos6xlellzrxh@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 5/8] libcamera: framebuffer_allocator:\n\tGet and return buffers","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":12487,"web_url":"https://patchwork.libcamera.org/comment/12487/","msgid":"","date":"2020-09-14T05:21:59","subject":"Re: [libcamera-devel] [PATCH 5/8] libcamera: framebuffer_allocator:\n\tGet and return buffers","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"On Sat, Sep 12, 2020 at 6:45 PM Jacopo Mondi wrote:\n>\n> Hi Hiro,\n> I missed part of your reply in the previous email\n>\n> Let me continue here :)\n>\n> On Fri, Sep 11, 2020 at 06:25:04PM +0200, Jacopo Mondi wrote:\n> > Hi Hiro,\n> >\n> > On Fri, Sep 11, 2020 at 05:12:35PM +0900, Hirokazu Honda wrote:\n> > > On Thu, Sep 10, 2020 at 10:19 PM Jacopo Mondi wrote:\n> > > >\n> > > > Hi Niklas,\n> > > >\n> > > > On Thu, Sep 10, 2020 at 03:02:55PM +0200, Niklas Söderlund wrote:\n> > > > > Hi Jacopo,\n> > > > >\n> > > > > On 2020-09-10 14:27:46 +0200, Jacopo Mondi wrote:\n> > > > > > Hi Niklas,\n> > > > > >\n> > > > > > On Thu, Sep 10, 2020 at 01:30:16PM +0200, Niklas Söderlund wrote:\n> > > > > > > Hi Jacopo,\n> > > > > > >\n> > > > > > > Thanks for your work.\n> > > > > > >\n> > > > > > > On 2020-09-09 17:54:54 +0200, Jacopo Mondi wrote:\n> > > > > > > > Add to the FrameBufferAllocator class two methods to get and\n> > > > > > > > return buffers from the pool of buffers allocated for a Stream.\n> > > > > > > >\n> > > > > > > > The two methods return pointer to the allocated buffers without\n> > > > > > > > transferring ownership to the caller.\n> > > > > > >\n> > > > > > > I'm sorry I don't like this patch. If it was an interface that was\n> > > > > > > internal I would be more OK with this change. The FrameBufferAllocator\n> > > > > > > is facing applications and this change introduces a duality between the\n> > > > > > > new getBuffer() and the existing buffers() functionality.\n> > > > > > >\n> > > > > > > I think it creates complexity in this user-facing API which is not\n> > > > > > > really needed. How about creating a HALFrameBufferAllocator that is\n> > > > > > > local to the HAL and inherits from FrameBufferAllocator while adding the\n> > > > > > > features you want?\n> > > > > >\n> > > > > > That would end up repeating the same thing we do in pipeline handlers\n> > > > > > here. Actually I found safer to provide a get/return interface than\n> > > > > > giving applications full access to the pool and let them deal with\n> > > > > > that. At the same time I won't prevent that completely by removing\n> > > > > > buffers(), but I don't see why the 'duality' is an issue.\n> > > > > >\n> > > > > > I'm more than open to discuss the implementation which is surely\n> > > > > > improvable and if we want to implement this feature in\n> > > > > > FrameBufferAllocator or as you suggested sub-class it somehow, but I'm\n> > > > > > not sure why the problem lies in having the three methods.\n> > > > >\n> > > > > My concern is having an application call getBuffer() to prepare one set\n> > > > > of Requests, maybe one with a viewfinder + raw and then iterating over\n> > > > > buffers() to create as many Requests with just viewfinder as available.\n> > > > > As buffers() don't consider if it have \"given\" out any buffer prior to\n> > > > > the buffers() call we have the potential to have Requests that can't be\n> > > > > queued at the same time as they contain a subset of the same buffers. I\n> > > > > fear this could get even worse once Request becomes reusable.\n> > > > >\n> > > >\n> > > > True that a good API is one you cannot get wrong, but if an\n> > > > application behaves like that [1] it means it -really- is trying to\n> > > > shoot itself in the foot. I know it's a thin line though..\n> > > >\n> > > > I won't push for this, but if asked which interface I would prefer, I\n> > > > would ask for a get/return one instead of a \"here you have all you buffers\"\n> > > > one. There's also the question about buffer ownership, which are\n> > > > stored as unique_ptr<> and thus connected to the lifetime of the\n> > > > allocator. A 'get all the buffers' interface would allow application\n> > > > to move ownership to themselves and really screw up. A stricter\n> > > > interface would allow us to decide what to do: right now we 'borrow'\n> > > > the buffer ownership but we might as well decide to transfer that,\n> > > > but we have a single behavior and I think that's better.\n> > > >\n> > >\n> > > I am ok with this API change.\n> > > I can understand the opinions of both Jacopo and Niklas.\n> > > Since buffers() returns const reference of vector, client\n> > > cannot takes\n> > > the ownership by std::move(). Having Get/ReturnBuffer() and buffers() should not\n> > > go wrong if the client adopts either way.\n> >\n> > mmm, you're right here, the reference is a const, there's not risk of\n> > messing up the buffer ownership..\n> >\n> > > The thing is more like, where the buffer usage is managed, in client\n> > > (with buffers()), or\n> > > in FrameBufferAllocator (with Get/ReturnBuffer()).\n> > > For simplicity, I think eventually FrameBufferAllocator should have\n> > > GetBuffer() and ReturnBuffer().\n> > >\n> >\n> > Yes, I think getBuffer/returnBuffer provide a managed interface that\n> > ease the application life, but at the same time I won't be super happy\n> > of preventing application to implement custom ones...\n> >\n> > Not sure, I still feel the two interfaces can live together, and if\n> > someone mis-uses the two it's the same kind of error that one would do\n> > if buffers are get but never returned == bad application code :)\n> >\n> > > > Thanks\n> > > > j\n> > > >\n> > > > [1] I'm not sure I got your example fully. Are you suggesting an\n> > > > application might initially use getBuffer/returnBuffer then decide to\n> > > > access the buffer vector returned by buffers() and use all of them\n> > > > ignoring those are the same buffers it already got access to through\n> > > > the get/return API ?\n> > > >\n> > > > > Understand me correctly, I'm not advocating in favor of buffers() or\n> > > > > getBuffers() interface. I do however think we should only have one as it\n> > > > > makes the interface easier to use for applications. As you suggest this\n> > > > > new interface could replace boilerplait code in pipelines and possibly\n> > > > > even applications to maybe it should replace the old buffers()\n> > > > > interface?\n> > > > >\n> > > > > >\n> > > > > > >\n> > > > > > > >\n> > > > > > > > Signed-off-by: Jacopo Mondi \n> > > > > > > > ---\n> > > > > > > > include/libcamera/framebuffer_allocator.h | 5 ++\n> > > > > > > > src/libcamera/framebuffer_allocator.cpp | 60 +++++++++++++++++++++--\n> > > > > > > > 2 files changed, 62 insertions(+), 3 deletions(-)\n> > > > > > > >\n> > > > > > > > diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h\n> > > > > > > > index 2a4d538a0cb2..1f3f10d4ec03 100644\n> > > > > > > > --- a/include/libcamera/framebuffer_allocator.h\n> > > > > > > > +++ b/include/libcamera/framebuffer_allocator.h\n> > > > > > > > @@ -9,6 +9,7 @@\n> > > > > > > >\n> > > > > > > > #include \n> > > > > > > > #include \n> > > > > > > > +#include \n> > > > > > > > #include \n> > > > > > > >\n> > > > > > > > namespace libcamera {\n> > > > > > > > @@ -33,9 +34,13 @@ public:\n> > > > > > > > bool allocated() const { return !buffers_.empty(); }\n> > > > > > > > const std::vector> &buffers(Stream *stream) const;\n> > > > > > > >\n> > > > > > > > + FrameBuffer *getBuffer(Stream *stream);\n> > > > > > > > + void returnBuffer(Stream *stream, FrameBuffer *buffer);\n> > > > > > > > +\n> > > > > > > > private:\n> > > > > > > > std::shared_ptr camera_;\n> > > > > > > > std::map>> buffers_;\n> > > > > > > > + std::map> availableBuffers_;\n> > > > > > > > };\n> > > > > > > >\n> > >\n> > > I think we don't have to think of order. So how about using\n> > > std::vector and use the last entry of the vector?\n>\n> Using the last entry doesn't seem the best idea, as you can only push\n> to the end of an std::vector, so you could potentially cycle over a\n> single or a limited set of buffers. It wonder about concurrency also,\n> if an application access the end of the vector from two different\n> threads (is this possible?) we might need some kind of protection.\n>\n> One possible way forward would be to push to the end of the vector and\n> pop from its front. We can reserve space in advance knowing the number\n> of buffers, so that would be more efficient than a queue indeed.\n>\n\n> so you could potentially cycle over a single or a limited set of buffers\nIs this problematic?\n\n> It wonder about concurrency also, if an application access the end of the vector from two different\n> threads (is this possible?) we might need some kind of protection.\nI think then protection is required even if we use std::queue.\nI assume the same thread operates on this interface. Is it not guaranteed?\n\n> > >\n> > > > > > > > } /* namespace libcamera */\n> > > > > > > > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp\n> > > > > > > > index 7ed80011c845..7429d6b9edb7 100644\n> > > > > > > > --- a/src/libcamera/framebuffer_allocator.cpp\n> > > > > > > > +++ b/src/libcamera/framebuffer_allocator.cpp\n> > > > > > > > @@ -64,7 +64,7 @@ FrameBufferAllocator::FrameBufferAllocator(std::shared_ptr camera)\n> > > > > > > >\n> > > > > > > > FrameBufferAllocator::~FrameBufferAllocator()\n> > > > > > > > {\n> > > > > > > > - buffers_.clear();\n> > > > > > > > + clear();\n> > > > > > > > }\n> > > > > > > >\n> > >\n> > > clear() is unnecessary.\n>\n> As stated in the previous email, you're probably right\n>\n> > >\n> > > > > > > > /**\n> > > > > > > > @@ -93,11 +93,17 @@ int FrameBufferAllocator::allocate(Stream *stream)\n> > > > > > > > }\n> > > > > > > >\n> > > > > > > > int ret = camera_->exportFrameBuffers(stream, &buffers_[stream]);\n> > > > > > > > - if (ret == -EINVAL)\n> > > > > > > > + if (ret == -EINVAL) {\n> > > > > > > > LOG(Allocator, Error)\n> > > > > > > > << \"Stream is not part of \" << camera_->id()\n> > > > > > > > << \" active configuration\";\n> > > > > > > > - return ret;\n> > > > > > > > + return ret;\n> > > > > > > > + }\n> > > > > > > > +\n> > > > > > > > + for (const auto &buffer : buffers_[stream])\n> > > > > > > > + availableBuffers_[stream].push(buffer.get());\n> > > > > > > > +\n> > > > > > > > + return 0;\n> > > > > > > > }\n> > > > > > > >\n> > > > > > > > /**\n> > > > > > > > @@ -122,6 +128,9 @@ int FrameBufferAllocator::free(Stream *stream)\n> > > > > > > > buffers.clear();\n> > > > > > > > buffers_.erase(iter);\n> > > > > > > >\n> > > > > > > > + availableBuffers_[stream] = {};\n> > > > > > > > + availableBuffers_.erase(availableBuffers_.find(stream));\n> > > > > > > > +\n> > >\n> > > I would just availableBuffers_.erase(stream);\n>\n> Correct\n>\n> > > ditto to buffers_.\n>\n> Also probably correct as buffers_.erase(iter) would delete the vector\n> of unique pointers there stored, causing memory to be released without\n> having to\n>\n> std::vector> &buffers = iter->second;\n> buffers.clear();\n>\n\nRight. Let's do buffers_.erase(stream);\n> > >\n> > > > > > > > return 0;\n> > > > > > > > }\n> > > > > > > >\n> > > > > > > > @@ -131,6 +140,7 @@ int FrameBufferAllocator::free(Stream *stream)\n> > > > > > > > void FrameBufferAllocator::clear()\n> > > > > > > > {\n> > > > > > > > buffers_.clear();\n> > > > > > > > + availableBuffers_.clear();\n> > > > > > > > }\n> > > > > > > >\n> > > > > > > > /**\n> > > > > > > > @@ -162,4 +172,48 @@ FrameBufferAllocator::buffers(Stream *stream) const\n> > > > > > > > return iter->second;\n> > > > > > > > }\n> > > > > > > >\n> > > > > > > > +/**\n> > > > > > > > + * \\brief Get a pointer to a \\a buffer for the \\a stream\n> > > > > > > > + * \\param[in] stream The stream to get a buffer for\n> > > > > > > > + *\n> > > > > > > > + * The method returns a pointer to a FrameBuffer but does transfer the buffer\n> > > > > > > > + * ownership to the caller: the returned pointer remains valid until the\n> > > > > > > > + * FrameBufferAllocator does not get deleted or the allocated buffers do not get\n> > > > > > > > + * released with a call for free() or clear().\n> > > > > > > > + *\n> > > > > > > > + * \\return A FrameBuffer pointer or nullptr if the no buffers is available\n> > > > > > > > + */\n> > > > > > > > +FrameBuffer *FrameBufferAllocator::getBuffer(Stream *stream)\n> > > > > > > > +{\n> > > > > > > > + if (!allocated() || buffers_[stream].empty())\n> > > > > > > > + return nullptr;\n> > > > > > > > +\n> > > > > > > > + FrameBuffer *frameBuffer = availableBuffers_[stream].front();\n> > > > > > > > + availableBuffers_[stream].pop();\n> > > > > > > > +\n> > > > > > > > + return frameBuffer;\n> > > > > > > > +}\n> > > > > > > > +\n> > > > > > > > +/**\n> > > > > > > > + * \\brief Return a \\a buffer to the list of buffers available for the a \\a stream\n> > > > > > > > + * \\param[in] stream The stream to return buffer to\n> > > > > > > > + * \\param[in] buffer The buffer to return\n> > > > > > > > + */\n> > > > > > > > +void FrameBufferAllocator::returnBuffer(Stream *stream, FrameBuffer *buffer)\n> > > > > > > > +{\n> > > > > > > > + if (!allocated())\n> > > > > > > > + return;\n> > > > > > > > +\n> > > > > > > > + for (const auto &b : buffers_[stream]) {\n> > > > > > > > + /*\n> > > > > > > > + * Return the buffer to the available queue only if it was part\n> > > > > > > > + * of the vector of buffers allocated for the Stream.\n> > > > > > > > + */\n> > > > > > > > + if (b.get() != buffer)\n> > > > > > > > + continue;\n> > > > > > > > +\n> > > > > > > > + availableBuffers_[stream].push(buffer);\n> > > > > > > > + }\n> > > > > > > > +}\n> > > > > > > > +\n> > >\n> > > I expect you may want to do as below. buffers_[stream] must not have\n> > > the same buffers because they are unique_ptr.\n> > > const auto& buffers = buffers_[stream];\n> > > if (std::find_if(buffers.begin(), buffers.end(), [buffer](const auto&\n> > > b) { return b.get() == buffer; } )) {\n> > > availableBuffers_[stream].push(buffer);\n> > > }\n>\n> If I got this right, your proposal is functionally equivalent but\n> I find it less readable. This surely is me and my not being exactly in\n> love with the ability C++ has to make easy things look complex, but in\n> this case I find open coding much more explict that going through an\n> std function. I suspect we could discuss this for days,\n> for sure your proposal is much more C++-ish than mine, but I found\n> open coding more explicit.\n>\n> Anyway, seems like a minor style issue, or have I missed any\n> functional change?\n>\n\nIf you like for-loop, in order to insert twice, I would\nfor (const auto &b : buffers_[stream]) {\n if (b.get() == buffer) {\n availableBuffers_[stream].push(buffer);\n break;\n }\n}\n\nThanks,\n-Hiro\n> Thanks\n> j\n>\n>\n> > >\n> > > > > > > > } /* namespace libcamera */\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> > > > > --\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\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 11312BF01C\n\tfor ;\n\tMon, 14 Sep 2020 05:22:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8FE2D62D27;\n\tMon, 14 Sep 2020 07:22:12 +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 AC70460369\n\tfor ;\n\tMon, 14 Sep 2020 07:22:10 +0200 (CEST)","by mail-ed1-x544.google.com with SMTP id i1so16270677edv.2\n\tfor ;\n\tSun, 13 Sep 2020 22:22: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=\"GvrYTP9R\"; 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=7syKbIRqmv749ADZdztd2KwcIY8PfKX7tKu0tAoLoh8=;\n\tb=GvrYTP9R0FSAs2/6ntP16TF2RT++csU7PNKnhpVtIxDmEEzxG4d7Kr6S/S75OXPdIQ\n\t4dEE/pLLz5K4cT6pWMsq0JXce+351m3JGW5N5t3EFvMrv067Zvr0xFOBm4k3sKJpsJrx\n\tChFRuBqGHU6HOiA5PpXPRvZHUxUOUVHqqAXSs=","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=7syKbIRqmv749ADZdztd2KwcIY8PfKX7tKu0tAoLoh8=;\n\tb=XD2dN14GxNpvh4ZG9aBouuzxPG4SGhFplWS8j12ef6ppCENFhYA9tyYLyt6BnkZ2De\n\tNT7NQPu2/G1miEKGYRBSPGEwFJ3Xm5mpnYWeGc3wiATMLzJNhPL+sOaOXAcUNgQBc0VP\n\t1vuDHe2w0Ca260/NdIUsCtlKhseUgd3UtSAdjGhXGJ+pFc0PzzoU9bDLB44QkGLmnt1a\n\tV0L8jaFQkrlB0b9RSMo/vLDRidaXPSIVDwo2fmUR+RY/s3WaKKTHw5gNlQ6W4s5se/ll\n\t0w6MCUIF3CMUBnkq0hNSbbOyvw7Vx595/NcPMBL8z70V84O+zY6JR/dppG97/ptTviz4\n\tvK3A==","X-Gm-Message-State":"AOAM531HDTCNduLGiAllVpcyMIIVMSh6wLkx1pNn9TncCU+tItyWMOeM\n\tLGt1cuD84S/nqgE4FcubisxtKu520mGeeHtHnxi4+w7ugHARSQ==","X-Google-Smtp-Source":"ABdhPJx1zMV3/hSy34M3RZUwpxLMklVik5cVxlWnBlyY31I/x2nXVP1/j4Vlad20Cegn4QMWwFAW9QvdHqqeQDU3qw4=","X-Received":"by 2002:aa7:d296:: with SMTP id\n\tw22mr15621488edq.327.1600060930021; \n\tSun, 13 Sep 2020 22:22:10 -0700 (PDT)","MIME-Version":"1.0","References":"<20200909155457.153907-1-jacopo@jmondi.org>\n\t<20200909155457.153907-6-jacopo@jmondi.org>\n\t<20200910113016.GR4095624@oden.dyn.berto.se>\n\t<20200910122746.le3ndavmu6btkouk@uno.localdomain>\n\t<20200910130255.GW4095624@oden.dyn.berto.se>\n\t<20200910132255.pa4jos6xlellzrxh@uno.localdomain>\n\t\n\t<20200911162504.flumjjap5haaqpfw@uno.localdomain>\n\t<20200912094918.pqkrtsu3mj252g7l@uno.localdomain>","In-Reply-To":"<20200912094918.pqkrtsu3mj252g7l@uno.localdomain>","From":"Hirokazu Honda ","Date":"Mon, 14 Sep 2020 14:21:59 +0900","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH 5/8] libcamera: framebuffer_allocator:\n\tGet and return buffers","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","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12499,"web_url":"https://patchwork.libcamera.org/comment/12499/","msgid":"","date":"2020-09-14T13:35:32","subject":"Re: [libcamera-devel] [PATCH 5/8] libcamera: framebuffer_allocator:\n\tGet and return buffers","submitter":{"id":9,"url":"https://patchwork.libcamera.org/api/people/9/","name":"Tomasz Figa","email":"tfiga@chromium.org"},"content":"On Mon, Sep 14, 2020 at 7:22 AM Hirokazu Honda wrote:\n>\n> On Sat, Sep 12, 2020 at 6:45 PM Jacopo Mondi wrote:\n> >\n> > Hi Hiro,\n> > I missed part of your reply in the previous email\n> >\n> > Let me continue here :)\n> >\n> > On Fri, Sep 11, 2020 at 06:25:04PM +0200, Jacopo Mondi wrote:\n> > > Hi Hiro,\n> > >\n> > > On Fri, Sep 11, 2020 at 05:12:35PM +0900, Hirokazu Honda wrote:\n> > > > On Thu, Sep 10, 2020 at 10:19 PM Jacopo Mondi wrote:\n> > > > >\n> > > > > Hi Niklas,\n> > > > >\n> > > > > On Thu, Sep 10, 2020 at 03:02:55PM +0200, Niklas Söderlund wrote:\n> > > > > > Hi Jacopo,\n> > > > > >\n> > > > > > On 2020-09-10 14:27:46 +0200, Jacopo Mondi wrote:\n> > > > > > > Hi Niklas,\n> > > > > > >\n> > > > > > > On Thu, Sep 10, 2020 at 01:30:16PM +0200, Niklas Söderlund wrote:\n> > > > > > > > Hi Jacopo,\n> > > > > > > >\n> > > > > > > > Thanks for your work.\n> > > > > > > >\n> > > > > > > > On 2020-09-09 17:54:54 +0200, Jacopo Mondi wrote:\n> > > > > > > > > Add to the FrameBufferAllocator class two methods to get and\n> > > > > > > > > return buffers from the pool of buffers allocated for a Stream.\n> > > > > > > > >\n> > > > > > > > > The two methods return pointer to the allocated buffers without\n> > > > > > > > > transferring ownership to the caller.\n> > > > > > > >\n> > > > > > > > I'm sorry I don't like this patch. If it was an interface that was\n> > > > > > > > internal I would be more OK with this change. The FrameBufferAllocator\n> > > > > > > > is facing applications and this change introduces a duality between the\n> > > > > > > > new getBuffer() and the existing buffers() functionality.\n> > > > > > > >\n> > > > > > > > I think it creates complexity in this user-facing API which is not\n> > > > > > > > really needed. How about creating a HALFrameBufferAllocator that is\n> > > > > > > > local to the HAL and inherits from FrameBufferAllocator while adding the\n> > > > > > > > features you want?\n> > > > > > >\n> > > > > > > That would end up repeating the same thing we do in pipeline handlers\n> > > > > > > here. Actually I found safer to provide a get/return interface than\n> > > > > > > giving applications full access to the pool and let them deal with\n> > > > > > > that. At the same time I won't prevent that completely by removing\n> > > > > > > buffers(), but I don't see why the 'duality' is an issue.\n> > > > > > >\n> > > > > > > I'm more than open to discuss the implementation which is surely\n> > > > > > > improvable and if we want to implement this feature in\n> > > > > > > FrameBufferAllocator or as you suggested sub-class it somehow, but I'm\n> > > > > > > not sure why the problem lies in having the three methods.\n> > > > > >\n> > > > > > My concern is having an application call getBuffer() to prepare one set\n> > > > > > of Requests, maybe one with a viewfinder + raw and then iterating over\n> > > > > > buffers() to create as many Requests with just viewfinder as available.\n> > > > > > As buffers() don't consider if it have \"given\" out any buffer prior to\n> > > > > > the buffers() call we have the potential to have Requests that can't be\n> > > > > > queued at the same time as they contain a subset of the same buffers. I\n> > > > > > fear this could get even worse once Request becomes reusable.\n> > > > > >\n> > > > >\n> > > > > True that a good API is one you cannot get wrong, but if an\n> > > > > application behaves like that [1] it means it -really- is trying to\n> > > > > shoot itself in the foot. I know it's a thin line though..\n> > > > >\n> > > > > I won't push for this, but if asked which interface I would prefer, I\n> > > > > would ask for a get/return one instead of a \"here you have all you buffers\"\n> > > > > one. There's also the question about buffer ownership, which are\n> > > > > stored as unique_ptr<> and thus connected to the lifetime of the\n> > > > > allocator. A 'get all the buffers' interface would allow application\n> > > > > to move ownership to themselves and really screw up. A stricter\n> > > > > interface would allow us to decide what to do: right now we 'borrow'\n> > > > > the buffer ownership but we might as well decide to transfer that,\n> > > > > but we have a single behavior and I think that's better.\n> > > > >\n> > > >\n> > > > I am ok with this API change.\n> > > > I can understand the opinions of both Jacopo and Niklas.\n> > > > Since buffers() returns const reference of vector, client\n> > > > cannot takes\n> > > > the ownership by std::move(). Having Get/ReturnBuffer() and buffers() should not\n> > > > go wrong if the client adopts either way.\n> > >\n> > > mmm, you're right here, the reference is a const, there's not risk of\n> > > messing up the buffer ownership..\n> > >\n> > > > The thing is more like, where the buffer usage is managed, in client\n> > > > (with buffers()), or\n> > > > in FrameBufferAllocator (with Get/ReturnBuffer()).\n> > > > For simplicity, I think eventually FrameBufferAllocator should have\n> > > > GetBuffer() and ReturnBuffer().\n> > > >\n> > >\n> > > Yes, I think getBuffer/returnBuffer provide a managed interface that\n> > > ease the application life, but at the same time I won't be super happy\n> > > of preventing application to implement custom ones...\n> > >\n> > > Not sure, I still feel the two interfaces can live together, and if\n> > > someone mis-uses the two it's the same kind of error that one would do\n> > > if buffers are get but never returned == bad application code :)\n> > >\n> > > > > Thanks\n> > > > > j\n> > > > >\n> > > > > [1] I'm not sure I got your example fully. Are you suggesting an\n> > > > > application might initially use getBuffer/returnBuffer then decide to\n> > > > > access the buffer vector returned by buffers() and use all of them\n> > > > > ignoring those are the same buffers it already got access to through\n> > > > > the get/return API ?\n> > > > >\n> > > > > > Understand me correctly, I'm not advocating in favor of buffers() or\n> > > > > > getBuffers() interface. I do however think we should only have one as it\n> > > > > > makes the interface easier to use for applications. As you suggest this\n> > > > > > new interface could replace boilerplait code in pipelines and possibly\n> > > > > > even applications to maybe it should replace the old buffers()\n> > > > > > interface?\n> > > > > >\n> > > > > > >\n> > > > > > > >\n> > > > > > > > >\n> > > > > > > > > Signed-off-by: Jacopo Mondi \n> > > > > > > > > ---\n> > > > > > > > > include/libcamera/framebuffer_allocator.h | 5 ++\n> > > > > > > > > src/libcamera/framebuffer_allocator.cpp | 60 +++++++++++++++++++++--\n> > > > > > > > > 2 files changed, 62 insertions(+), 3 deletions(-)\n> > > > > > > > >\n> > > > > > > > > diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h\n> > > > > > > > > index 2a4d538a0cb2..1f3f10d4ec03 100644\n> > > > > > > > > --- a/include/libcamera/framebuffer_allocator.h\n> > > > > > > > > +++ b/include/libcamera/framebuffer_allocator.h\n> > > > > > > > > @@ -9,6 +9,7 @@\n> > > > > > > > >\n> > > > > > > > > #include \n> > > > > > > > > #include \n> > > > > > > > > +#include \n> > > > > > > > > #include \n> > > > > > > > >\n> > > > > > > > > namespace libcamera {\n> > > > > > > > > @@ -33,9 +34,13 @@ public:\n> > > > > > > > > bool allocated() const { return !buffers_.empty(); }\n> > > > > > > > > const std::vector> &buffers(Stream *stream) const;\n> > > > > > > > >\n> > > > > > > > > + FrameBuffer *getBuffer(Stream *stream);\n> > > > > > > > > + void returnBuffer(Stream *stream, FrameBuffer *buffer);\n> > > > > > > > > +\n> > > > > > > > > private:\n> > > > > > > > > std::shared_ptr camera_;\n> > > > > > > > > std::map>> buffers_;\n> > > > > > > > > + std::map> availableBuffers_;\n> > > > > > > > > };\n> > > > > > > > >\n> > > >\n> > > > I think we don't have to think of order. So how about using\n> > > > std::vector and use the last entry of the vector?\n> >\n> > Using the last entry doesn't seem the best idea, as you can only push\n> > to the end of an std::vector, so you could potentially cycle over a\n> > single or a limited set of buffers. It wonder about concurrency also,\n> > if an application access the end of the vector from two different\n> > threads (is this possible?) we might need some kind of protection.\n> >\n> > One possible way forward would be to push to the end of the vector and\n> > pop from its front. We can reserve space in advance knowing the number\n> > of buffers, so that would be more efficient than a queue indeed.\n> >\n>\n> > so you could potentially cycle over a single or a limited set of buffers\n> Is this problematic?\n>\n> > It wonder about concurrency also, if an application access the end of the vector from two different\n> > threads (is this possible?) we might need some kind of protection.\n> I think then protection is required even if we use std::queue.\n> I assume the same thread operates on this interface. Is it not guaranteed?\n>\n> > > >\n> > > > > > > > > } /* namespace libcamera */\n> > > > > > > > > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp\n> > > > > > > > > index 7ed80011c845..7429d6b9edb7 100644\n> > > > > > > > > --- a/src/libcamera/framebuffer_allocator.cpp\n> > > > > > > > > +++ b/src/libcamera/framebuffer_allocator.cpp\n> > > > > > > > > @@ -64,7 +64,7 @@ FrameBufferAllocator::FrameBufferAllocator(std::shared_ptr camera)\n> > > > > > > > >\n> > > > > > > > > FrameBufferAllocator::~FrameBufferAllocator()\n> > > > > > > > > {\n> > > > > > > > > - buffers_.clear();\n> > > > > > > > > + clear();\n> > > > > > > > > }\n> > > > > > > > >\n> > > >\n> > > > clear() is unnecessary.\n> >\n> > As stated in the previous email, you're probably right\n> >\n> > > >\n> > > > > > > > > /**\n> > > > > > > > > @@ -93,11 +93,17 @@ int FrameBufferAllocator::allocate(Stream *stream)\n> > > > > > > > > }\n> > > > > > > > >\n> > > > > > > > > int ret = camera_->exportFrameBuffers(stream, &buffers_[stream]);\n> > > > > > > > > - if (ret == -EINVAL)\n> > > > > > > > > + if (ret == -EINVAL) {\n> > > > > > > > > LOG(Allocator, Error)\n> > > > > > > > > << \"Stream is not part of \" << camera_->id()\n> > > > > > > > > << \" active configuration\";\n> > > > > > > > > - return ret;\n> > > > > > > > > + return ret;\n> > > > > > > > > + }\n> > > > > > > > > +\n> > > > > > > > > + for (const auto &buffer : buffers_[stream])\n> > > > > > > > > + availableBuffers_[stream].push(buffer.get());\n> > > > > > > > > +\n> > > > > > > > > + return 0;\n> > > > > > > > > }\n> > > > > > > > >\n> > > > > > > > > /**\n> > > > > > > > > @@ -122,6 +128,9 @@ int FrameBufferAllocator::free(Stream *stream)\n> > > > > > > > > buffers.clear();\n> > > > > > > > > buffers_.erase(iter);\n> > > > > > > > >\n> > > > > > > > > + availableBuffers_[stream] = {};\n> > > > > > > > > + availableBuffers_.erase(availableBuffers_.find(stream));\n> > > > > > > > > +\n> > > >\n> > > > I would just availableBuffers_.erase(stream);\n> >\n> > Correct\n> >\n> > > > ditto to buffers_.\n> >\n> > Also probably correct as buffers_.erase(iter) would delete the vector\n> > of unique pointers there stored, causing memory to be released without\n> > having to\n> >\n> > std::vector> &buffers = iter->second;\n> > buffers.clear();\n> >\n>\n> Right. Let's do buffers_.erase(stream);\n> > > >\n> > > > > > > > > return 0;\n> > > > > > > > > }\n> > > > > > > > >\n> > > > > > > > > @@ -131,6 +140,7 @@ int FrameBufferAllocator::free(Stream *stream)\n> > > > > > > > > void FrameBufferAllocator::clear()\n> > > > > > > > > {\n> > > > > > > > > buffers_.clear();\n> > > > > > > > > + availableBuffers_.clear();\n> > > > > > > > > }\n> > > > > > > > >\n> > > > > > > > > /**\n> > > > > > > > > @@ -162,4 +172,48 @@ FrameBufferAllocator::buffers(Stream *stream) const\n> > > > > > > > > return iter->second;\n> > > > > > > > > }\n> > > > > > > > >\n> > > > > > > > > +/**\n> > > > > > > > > + * \\brief Get a pointer to a \\a buffer for the \\a stream\n> > > > > > > > > + * \\param[in] stream The stream to get a buffer for\n> > > > > > > > > + *\n> > > > > > > > > + * The method returns a pointer to a FrameBuffer but does transfer the buffer\n> > > > > > > > > + * ownership to the caller: the returned pointer remains valid until the\n> > > > > > > > > + * FrameBufferAllocator does not get deleted or the allocated buffers do not get\n> > > > > > > > > + * released with a call for free() or clear().\n> > > > > > > > > + *\n> > > > > > > > > + * \\return A FrameBuffer pointer or nullptr if the no buffers is available\n> > > > > > > > > + */\n> > > > > > > > > +FrameBuffer *FrameBufferAllocator::getBuffer(Stream *stream)\n> > > > > > > > > +{\n> > > > > > > > > + if (!allocated() || buffers_[stream].empty())\n> > > > > > > > > + return nullptr;\n> > > > > > > > > +\n> > > > > > > > > + FrameBuffer *frameBuffer = availableBuffers_[stream].front();\n> > > > > > > > > + availableBuffers_[stream].pop();\n> > > > > > > > > +\n> > > > > > > > > + return frameBuffer;\n> > > > > > > > > +}\n> > > > > > > > > +\n> > > > > > > > > +/**\n> > > > > > > > > + * \\brief Return a \\a buffer to the list of buffers available for the a \\a stream\n> > > > > > > > > + * \\param[in] stream The stream to return buffer to\n> > > > > > > > > + * \\param[in] buffer The buffer to return\n> > > > > > > > > + */\n> > > > > > > > > +void FrameBufferAllocator::returnBuffer(Stream *stream, FrameBuffer *buffer)\n> > > > > > > > > +{\n> > > > > > > > > + if (!allocated())\n> > > > > > > > > + return;\n> > > > > > > > > +\n> > > > > > > > > + for (const auto &b : buffers_[stream]) {\n> > > > > > > > > + /*\n> > > > > > > > > + * Return the buffer to the available queue only if it was part\n> > > > > > > > > + * of the vector of buffers allocated for the Stream.\n> > > > > > > > > + */\n> > > > > > > > > + if (b.get() != buffer)\n> > > > > > > > > + continue;\n> > > > > > > > > +\n> > > > > > > > > + availableBuffers_[stream].push(buffer);\n> > > > > > > > > + }\n> > > > > > > > > +}\n> > > > > > > > > +\n> > > >\n> > > > I expect you may want to do as below. buffers_[stream] must not have\n> > > > the same buffers because they are unique_ptr.\n> > > > const auto& buffers = buffers_[stream];\n> > > > if (std::find_if(buffers.begin(), buffers.end(), [buffer](const auto&\n> > > > b) { return b.get() == buffer; } )) {\n> > > > availableBuffers_[stream].push(buffer);\n> > > > }\n> >\n> > If I got this right, your proposal is functionally equivalent but\n> > I find it less readable. This surely is me and my not being exactly in\n> > love with the ability C++ has to make easy things look complex, but in\n> > this case I find open coding much more explict that going through an\n> > std function. I suspect we could discuss this for days,\n> > for sure your proposal is much more C++-ish than mine, but I found\n> > open coding more explicit.\n> >\n> > Anyway, seems like a minor style issue, or have I missed any\n> > functional change?\n> >\n>\n> If you like for-loop, in order to insert twice, I would\n\nDo you mean \"not to insert twice\"?\n\n> for (const auto &b : buffers_[stream]) {\n> if (b.get() == buffer) {\n> availableBuffers_[stream].push(buffer);\n> break;\n> }\n> }\n\nIn any case, this implementation has the advantage of ending the\niteration when the buffer is found, without wasting CPU cycles for\nremaining iterations.\n\nOn a side note, I'm really annoyed by the lack of the find() method in\nstd::vector, making it so inconsistent with other containers, where\nfind() is the preferred approach for finding elements, because of\ncontainer-specific implementation that gives the best complexity. It\nshouldn't really matter here, though, as we expect a small number of\nbuffers anyway.\n\nBest regards,\nTomasz\n\n>\n> Thanks,\n> -Hiro\n> > Thanks\n> > j\n> >\n> >\n> > > >\n> > > > > > > > > } /* namespace libcamera */\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> > > > > > --\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\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\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 D6EEBC3B5B\n\tfor ;\n\tMon, 14 Sep 2020 13:35:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4F6A262DF5;\n\tMon, 14 Sep 2020 15:35:50 +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 DD28E62C8C\n\tfor ;\n\tMon, 14 Sep 2020 15:35:48 +0200 (CEST)","by mail-ed1-x544.google.com with SMTP id e22so7108050edq.6\n\tfor ;\n\tMon, 14 Sep 2020 06:35:48 -0700 (PDT)","from mail-wr1-f44.google.com (mail-wr1-f44.google.com.\n\t[209.85.221.44]) by smtp.gmail.com with ESMTPSA id\n\tu15sm9306393edq.96.2020.09.14.06.35.47\n\tfor \n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tMon, 14 Sep 2020 06:35:47 -0700 (PDT)","by mail-wr1-f44.google.com with SMTP id k15so18756137wrn.10\n\tfor ;\n\tMon, 14 Sep 2020 06:35:47 -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=\"WHJ3G8Vy\"; 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=POupTWIiAagKxIk1cqQfgJE6C3thHHpb4aAyP3Dlxzs=;\n\tb=WHJ3G8VydqKaWS3FmQEJinzIr+cr9/5DgB8TMJPFTNKrXiZjq30qYq7eU9nZwfriXn\n\tlh1gh6bFnBzx0P+AXhmI9Sp/ajlwIyhOqnODXi4tXdopVTVUbfZzVSrJSr0d9lQgxrMD\n\tkrOAeVeeUtEo+mBwxRmUXELRVkUTnXaOO6zYo=","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=POupTWIiAagKxIk1cqQfgJE6C3thHHpb4aAyP3Dlxzs=;\n\tb=UH0vQWdYxVdYjIXE24BZxd76V6MzX2vbkzo0Z5bAm7m4e8OU5IE/uwoFkht+9+9R58\n\tS221jtnUa/Q14wtea2s0W1jio3Zri1IgX7594LUjREMlKVT6d4oHj+kheMueb7KNiIbn\n\tAci2gwttI13Eq2UZrbp5Vg0bMvK0NocwRkKJcoqTIXqEMLsBv6TV1muMHejFOpKIJEU+\n\t9/H1nIkzruDj1nbf02Y77tq7bE9pDOYJRbnLffBEV8y3MI52oTbwIX9Y8jn8NvlzDZFk\n\tl1Te5CbrcGg/k5Bl2lqiHiONIQi718PMq4sx9dEfsQh6MpJcLOtxQovClmI7b00IKgx8\n\trU3g==","X-Gm-Message-State":"AOAM532FljNKl4qhXYi8bHiM+TwxoBipZM9C79lzcm+Uho2c6/kPe+vY\n\tlYR5MWQ6RjjL29x4uus/+kvul/SJIJdlDg==","X-Google-Smtp-Source":"ABdhPJy1/Y7L1cNCYY8/hnQwbuCgM/iVergFJ5OaKFecCnAZLajqFoYVz7Z5KncN1EKFSmAh6Xn/jA==","X-Received":["by 2002:aa7:da89:: with SMTP id\n\tq9mr16970220eds.111.1600090547956; \n\tMon, 14 Sep 2020 06:35:47 -0700 (PDT)","by 2002:a5d:5486:: with SMTP id\n\th6mr16300259wrv.415.1600090546322; \n\tMon, 14 Sep 2020 06:35:46 -0700 (PDT)"],"MIME-Version":"1.0","References":"<20200909155457.153907-1-jacopo@jmondi.org>\n\t<20200909155457.153907-6-jacopo@jmondi.org>\n\t<20200910113016.GR4095624@oden.dyn.berto.se>\n\t<20200910122746.le3ndavmu6btkouk@uno.localdomain>\n\t<20200910130255.GW4095624@oden.dyn.berto.se>\n\t<20200910132255.pa4jos6xlellzrxh@uno.localdomain>\n\t\n\t<20200911162504.flumjjap5haaqpfw@uno.localdomain>\n\t<20200912094918.pqkrtsu3mj252g7l@uno.localdomain>\n\t","In-Reply-To":"","From":"Tomasz Figa ","Date":"Mon, 14 Sep 2020 15:35:32 +0200","X-Gmail-Original-Message-ID":"","Message-ID":"","To":"Hirokazu Honda ","Subject":"Re: [libcamera-devel] [PATCH 5/8] libcamera: framebuffer_allocator:\n\tGet and return buffers","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 ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12500,"web_url":"https://patchwork.libcamera.org/comment/12500/","msgid":"<20200914141121.mbjqpztfhhfihhu5@uno.localdomain>","date":"2020-09-14T14:11:21","subject":"Re: [libcamera-devel] [PATCH 5/8] libcamera: framebuffer_allocator:\n\tGet and return buffers","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"On Mon, Sep 14, 2020 at 03:35:32PM +0200, Tomasz Figa wrote:\n> On Mon, Sep 14, 2020 at 7:22 AM Hirokazu Honda wrote:\n> >\n> > On Sat, Sep 12, 2020 at 6:45 PM Jacopo Mondi wrote:\n> > >\n> > > Hi Hiro,\n> > > I missed part of your reply in the previous email\n> > >\n> > > Let me continue here :)\n> > >\n> > > On Fri, Sep 11, 2020 at 06:25:04PM +0200, Jacopo Mondi wrote:\n> > > > Hi Hiro,\n> > > >\n> > > > On Fri, Sep 11, 2020 at 05:12:35PM +0900, Hirokazu Honda wrote:\n> > > > > On Thu, Sep 10, 2020 at 10:19 PM Jacopo Mondi wrote:\n> > > > > >\n> > > > > > Hi Niklas,\n> > > > > >\n> > > > > > On Thu, Sep 10, 2020 at 03:02:55PM +0200, Niklas Söderlund wrote:\n> > > > > > > Hi Jacopo,\n> > > > > > >\n> > > > > > > On 2020-09-10 14:27:46 +0200, Jacopo Mondi wrote:\n> > > > > > > > Hi Niklas,\n> > > > > > > >\n> > > > > > > > On Thu, Sep 10, 2020 at 01:30:16PM +0200, Niklas Söderlund wrote:\n> > > > > > > > > Hi Jacopo,\n> > > > > > > > >\n> > > > > > > > > Thanks for your work.\n> > > > > > > > >\n> > > > > > > > > On 2020-09-09 17:54:54 +0200, Jacopo Mondi wrote:\n> > > > > > > > > > Add to the FrameBufferAllocator class two methods to get and\n> > > > > > > > > > return buffers from the pool of buffers allocated for a Stream.\n> > > > > > > > > >\n> > > > > > > > > > The two methods return pointer to the allocated buffers without\n> > > > > > > > > > transferring ownership to the caller.\n> > > > > > > > >\n> > > > > > > > > I'm sorry I don't like this patch. If it was an interface that was\n> > > > > > > > > internal I would be more OK with this change. The FrameBufferAllocator\n> > > > > > > > > is facing applications and this change introduces a duality between the\n> > > > > > > > > new getBuffer() and the existing buffers() functionality.\n> > > > > > > > >\n> > > > > > > > > I think it creates complexity in this user-facing API which is not\n> > > > > > > > > really needed. How about creating a HALFrameBufferAllocator that is\n> > > > > > > > > local to the HAL and inherits from FrameBufferAllocator while adding the\n> > > > > > > > > features you want?\n> > > > > > > >\n> > > > > > > > That would end up repeating the same thing we do in pipeline handlers\n> > > > > > > > here. Actually I found safer to provide a get/return interface than\n> > > > > > > > giving applications full access to the pool and let them deal with\n> > > > > > > > that. At the same time I won't prevent that completely by removing\n> > > > > > > > buffers(), but I don't see why the 'duality' is an issue.\n> > > > > > > >\n> > > > > > > > I'm more than open to discuss the implementation which is surely\n> > > > > > > > improvable and if we want to implement this feature in\n> > > > > > > > FrameBufferAllocator or as you suggested sub-class it somehow, but I'm\n> > > > > > > > not sure why the problem lies in having the three methods.\n> > > > > > >\n> > > > > > > My concern is having an application call getBuffer() to prepare one set\n> > > > > > > of Requests, maybe one with a viewfinder + raw and then iterating over\n> > > > > > > buffers() to create as many Requests with just viewfinder as available.\n> > > > > > > As buffers() don't consider if it have \"given\" out any buffer prior to\n> > > > > > > the buffers() call we have the potential to have Requests that can't be\n> > > > > > > queued at the same time as they contain a subset of the same buffers. I\n> > > > > > > fear this could get even worse once Request becomes reusable.\n> > > > > > >\n> > > > > >\n> > > > > > True that a good API is one you cannot get wrong, but if an\n> > > > > > application behaves like that [1] it means it -really- is trying to\n> > > > > > shoot itself in the foot. I know it's a thin line though..\n> > > > > >\n> > > > > > I won't push for this, but if asked which interface I would prefer, I\n> > > > > > would ask for a get/return one instead of a \"here you have all you buffers\"\n> > > > > > one. There's also the question about buffer ownership, which are\n> > > > > > stored as unique_ptr<> and thus connected to the lifetime of the\n> > > > > > allocator. A 'get all the buffers' interface would allow application\n> > > > > > to move ownership to themselves and really screw up. A stricter\n> > > > > > interface would allow us to decide what to do: right now we 'borrow'\n> > > > > > the buffer ownership but we might as well decide to transfer that,\n> > > > > > but we have a single behavior and I think that's better.\n> > > > > >\n> > > > >\n> > > > > I am ok with this API change.\n> > > > > I can understand the opinions of both Jacopo and Niklas.\n> > > > > Since buffers() returns const reference of vector, client\n> > > > > cannot takes\n> > > > > the ownership by std::move(). Having Get/ReturnBuffer() and buffers() should not\n> > > > > go wrong if the client adopts either way.\n> > > >\n> > > > mmm, you're right here, the reference is a const, there's not risk of\n> > > > messing up the buffer ownership..\n> > > >\n> > > > > The thing is more like, where the buffer usage is managed, in client\n> > > > > (with buffers()), or\n> > > > > in FrameBufferAllocator (with Get/ReturnBuffer()).\n> > > > > For simplicity, I think eventually FrameBufferAllocator should have\n> > > > > GetBuffer() and ReturnBuffer().\n> > > > >\n> > > >\n> > > > Yes, I think getBuffer/returnBuffer provide a managed interface that\n> > > > ease the application life, but at the same time I won't be super happy\n> > > > of preventing application to implement custom ones...\n> > > >\n> > > > Not sure, I still feel the two interfaces can live together, and if\n> > > > someone mis-uses the two it's the same kind of error that one would do\n> > > > if buffers are get but never returned == bad application code :)\n> > > >\n> > > > > > Thanks\n> > > > > > j\n> > > > > >\n> > > > > > [1] I'm not sure I got your example fully. Are you suggesting an\n> > > > > > application might initially use getBuffer/returnBuffer then decide to\n> > > > > > access the buffer vector returned by buffers() and use all of them\n> > > > > > ignoring those are the same buffers it already got access to through\n> > > > > > the get/return API ?\n> > > > > >\n> > > > > > > Understand me correctly, I'm not advocating in favor of buffers() or\n> > > > > > > getBuffers() interface. I do however think we should only have one as it\n> > > > > > > makes the interface easier to use for applications. As you suggest this\n> > > > > > > new interface could replace boilerplait code in pipelines and possibly\n> > > > > > > even applications to maybe it should replace the old buffers()\n> > > > > > > interface?\n> > > > > > >\n> > > > > > > >\n> > > > > > > > >\n> > > > > > > > > >\n> > > > > > > > > > Signed-off-by: Jacopo Mondi \n> > > > > > > > > > ---\n> > > > > > > > > > include/libcamera/framebuffer_allocator.h | 5 ++\n> > > > > > > > > > src/libcamera/framebuffer_allocator.cpp | 60 +++++++++++++++++++++--\n> > > > > > > > > > 2 files changed, 62 insertions(+), 3 deletions(-)\n> > > > > > > > > >\n> > > > > > > > > > diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h\n> > > > > > > > > > index 2a4d538a0cb2..1f3f10d4ec03 100644\n> > > > > > > > > > --- a/include/libcamera/framebuffer_allocator.h\n> > > > > > > > > > +++ b/include/libcamera/framebuffer_allocator.h\n> > > > > > > > > > @@ -9,6 +9,7 @@\n> > > > > > > > > >\n> > > > > > > > > > #include \n> > > > > > > > > > #include \n> > > > > > > > > > +#include \n> > > > > > > > > > #include \n> > > > > > > > > >\n> > > > > > > > > > namespace libcamera {\n> > > > > > > > > > @@ -33,9 +34,13 @@ public:\n> > > > > > > > > > bool allocated() const { return !buffers_.empty(); }\n> > > > > > > > > > const std::vector> &buffers(Stream *stream) const;\n> > > > > > > > > >\n> > > > > > > > > > + FrameBuffer *getBuffer(Stream *stream);\n> > > > > > > > > > + void returnBuffer(Stream *stream, FrameBuffer *buffer);\n> > > > > > > > > > +\n> > > > > > > > > > private:\n> > > > > > > > > > std::shared_ptr camera_;\n> > > > > > > > > > std::map>> buffers_;\n> > > > > > > > > > + std::map> availableBuffers_;\n> > > > > > > > > > };\n> > > > > > > > > >\n> > > > >\n> > > > > I think we don't have to think of order. So how about using\n> > > > > std::vector and use the last entry of the vector?\n> > >\n> > > Using the last entry doesn't seem the best idea, as you can only push\n> > > to the end of an std::vector, so you could potentially cycle over a\n> > > single or a limited set of buffers. It wonder about concurrency also,\n> > > if an application access the end of the vector from two different\n> > > threads (is this possible?) we might need some kind of protection.\n> > >\n> > > One possible way forward would be to push to the end of the vector and\n> > > pop from its front. We can reserve space in advance knowing the number\n> > > of buffers, so that would be more efficient than a queue indeed.\n> > >\n> >\n> > > so you could potentially cycle over a single or a limited set of buffers\n> > Is this problematic?\n> >\n> > > It wonder about concurrency also, if an application access the end of the vector from two different\n> > > threads (is this possible?) we might need some kind of protection.\n> > I think then protection is required even if we use std::queue.\n> > I assume the same thread operates on this interface. Is it not guaranteed?\n> >\n> > > > >\n> > > > > > > > > > } /* namespace libcamera */\n> > > > > > > > > > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp\n> > > > > > > > > > index 7ed80011c845..7429d6b9edb7 100644\n> > > > > > > > > > --- a/src/libcamera/framebuffer_allocator.cpp\n> > > > > > > > > > +++ b/src/libcamera/framebuffer_allocator.cpp\n> > > > > > > > > > @@ -64,7 +64,7 @@ FrameBufferAllocator::FrameBufferAllocator(std::shared_ptr camera)\n> > > > > > > > > >\n> > > > > > > > > > FrameBufferAllocator::~FrameBufferAllocator()\n> > > > > > > > > > {\n> > > > > > > > > > - buffers_.clear();\n> > > > > > > > > > + clear();\n> > > > > > > > > > }\n> > > > > > > > > >\n> > > > >\n> > > > > clear() is unnecessary.\n> > >\n> > > As stated in the previous email, you're probably right\n> > >\n> > > > >\n> > > > > > > > > > /**\n> > > > > > > > > > @@ -93,11 +93,17 @@ int FrameBufferAllocator::allocate(Stream *stream)\n> > > > > > > > > > }\n> > > > > > > > > >\n> > > > > > > > > > int ret = camera_->exportFrameBuffers(stream, &buffers_[stream]);\n> > > > > > > > > > - if (ret == -EINVAL)\n> > > > > > > > > > + if (ret == -EINVAL) {\n> > > > > > > > > > LOG(Allocator, Error)\n> > > > > > > > > > << \"Stream is not part of \" << camera_->id()\n> > > > > > > > > > << \" active configuration\";\n> > > > > > > > > > - return ret;\n> > > > > > > > > > + return ret;\n> > > > > > > > > > + }\n> > > > > > > > > > +\n> > > > > > > > > > + for (const auto &buffer : buffers_[stream])\n> > > > > > > > > > + availableBuffers_[stream].push(buffer.get());\n> > > > > > > > > > +\n> > > > > > > > > > + return 0;\n> > > > > > > > > > }\n> > > > > > > > > >\n> > > > > > > > > > /**\n> > > > > > > > > > @@ -122,6 +128,9 @@ int FrameBufferAllocator::free(Stream *stream)\n> > > > > > > > > > buffers.clear();\n> > > > > > > > > > buffers_.erase(iter);\n> > > > > > > > > >\n> > > > > > > > > > + availableBuffers_[stream] = {};\n> > > > > > > > > > + availableBuffers_.erase(availableBuffers_.find(stream));\n> > > > > > > > > > +\n> > > > >\n> > > > > I would just availableBuffers_.erase(stream);\n> > >\n> > > Correct\n> > >\n> > > > > ditto to buffers_.\n> > >\n> > > Also probably correct as buffers_.erase(iter) would delete the vector\n> > > of unique pointers there stored, causing memory to be released without\n> > > having to\n> > >\n> > > std::vector> &buffers = iter->second;\n> > > buffers.clear();\n> > >\n> >\n> > Right. Let's do buffers_.erase(stream);\n> > > > >\n> > > > > > > > > > return 0;\n> > > > > > > > > > }\n> > > > > > > > > >\n> > > > > > > > > > @@ -131,6 +140,7 @@ int FrameBufferAllocator::free(Stream *stream)\n> > > > > > > > > > void FrameBufferAllocator::clear()\n> > > > > > > > > > {\n> > > > > > > > > > buffers_.clear();\n> > > > > > > > > > + availableBuffers_.clear();\n> > > > > > > > > > }\n> > > > > > > > > >\n> > > > > > > > > > /**\n> > > > > > > > > > @@ -162,4 +172,48 @@ FrameBufferAllocator::buffers(Stream *stream) const\n> > > > > > > > > > return iter->second;\n> > > > > > > > > > }\n> > > > > > > > > >\n> > > > > > > > > > +/**\n> > > > > > > > > > + * \\brief Get a pointer to a \\a buffer for the \\a stream\n> > > > > > > > > > + * \\param[in] stream The stream to get a buffer for\n> > > > > > > > > > + *\n> > > > > > > > > > + * The method returns a pointer to a FrameBuffer but does transfer the buffer\n> > > > > > > > > > + * ownership to the caller: the returned pointer remains valid until the\n> > > > > > > > > > + * FrameBufferAllocator does not get deleted or the allocated buffers do not get\n> > > > > > > > > > + * released with a call for free() or clear().\n> > > > > > > > > > + *\n> > > > > > > > > > + * \\return A FrameBuffer pointer or nullptr if the no buffers is available\n> > > > > > > > > > + */\n> > > > > > > > > > +FrameBuffer *FrameBufferAllocator::getBuffer(Stream *stream)\n> > > > > > > > > > +{\n> > > > > > > > > > + if (!allocated() || buffers_[stream].empty())\n> > > > > > > > > > + return nullptr;\n> > > > > > > > > > +\n> > > > > > > > > > + FrameBuffer *frameBuffer = availableBuffers_[stream].front();\n> > > > > > > > > > + availableBuffers_[stream].pop();\n> > > > > > > > > > +\n> > > > > > > > > > + return frameBuffer;\n> > > > > > > > > > +}\n> > > > > > > > > > +\n> > > > > > > > > > +/**\n> > > > > > > > > > + * \\brief Return a \\a buffer to the list of buffers available for the a \\a stream\n> > > > > > > > > > + * \\param[in] stream The stream to return buffer to\n> > > > > > > > > > + * \\param[in] buffer The buffer to return\n> > > > > > > > > > + */\n> > > > > > > > > > +void FrameBufferAllocator::returnBuffer(Stream *stream, FrameBuffer *buffer)\n> > > > > > > > > > +{\n> > > > > > > > > > + if (!allocated())\n> > > > > > > > > > + return;\n> > > > > > > > > > +\n> > > > > > > > > > + for (const auto &b : buffers_[stream]) {\n> > > > > > > > > > + /*\n> > > > > > > > > > + * Return the buffer to the available queue only if it was part\n> > > > > > > > > > + * of the vector of buffers allocated for the Stream.\n> > > > > > > > > > + */\n> > > > > > > > > > + if (b.get() != buffer)\n> > > > > > > > > > + continue;\n> > > > > > > > > > +\n> > > > > > > > > > + availableBuffers_[stream].push(buffer);\n> > > > > > > > > > + }\n> > > > > > > > > > +}\n> > > > > > > > > > +\n> > > > >\n> > > > > I expect you may want to do as below. buffers_[stream] must not have\n> > > > > the same buffers because they are unique_ptr.\n> > > > > const auto& buffers = buffers_[stream];\n> > > > > if (std::find_if(buffers.begin(), buffers.end(), [buffer](const auto&\n> > > > > b) { return b.get() == buffer; } )) {\n> > > > > availableBuffers_[stream].push(buffer);\n> > > > > }\n> > >\n> > > If I got this right, your proposal is functionally equivalent but\n> > > I find it less readable. This surely is me and my not being exactly in\n> > > love with the ability C++ has to make easy things look complex, but in\n> > > this case I find open coding much more explict that going through an\n> > > std function. I suspect we could discuss this for days,\n> > > for sure your proposal is much more C++-ish than mine, but I found\n> > > open coding more explicit.\n> > >\n> > > Anyway, seems like a minor style issue, or have I missed any\n> > > functional change?\n> > >\n> >\n> > If you like for-loop, in order to insert twice, I would\n>\n> Do you mean \"not to insert twice\"?\n>\n> > for (const auto &b : buffers_[stream]) {\n> > if (b.get() == buffer) {\n> > availableBuffers_[stream].push(buffer);\n> > break;\n> > }\n> > }\n>\n> In any case, this implementation has the advantage of ending the\n> iteration when the buffer is found, without wasting CPU cycles for\n> remaining iterations.\n\nIndeed I'm missing a break :D\nI'll fix.\n\n>\n> On a side note, I'm really annoyed by the lack of the find() method in\n> std::vector, making it so inconsistent with other containers, where\n> find() is the preferred approach for finding elements, because of\n> container-specific implementation that gives the best complexity. It\n> shouldn't really matter here, though, as we expect a small number of\n> buffers anyway.\n\nBut I can't fix this one, sorry :)\n\nThanks\n j\n\n>\n> Best regards,\n> Tomasz\n>\n> >\n> > Thanks,\n> > -Hiro\n> > > Thanks\n> > > j\n> > >\n> > >\n> > > > >\n> > > > > > > > > > } /* namespace libcamera */\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> > > > > > > --\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\n> > > > _______________________________________________\n> > > > libcamera-devel mailing list\n> > > > libcamera-devel@lists.libcamera.org\n> > > > https://lists.libcamera.org/listinfo/libcamera-devel\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 36E42C3B5B\n\tfor ;\n\tMon, 14 Sep 2020 14:07:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 95A4C62DAB;\n\tMon, 14 Sep 2020 16:07:32 +0200 (CEST)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1789262C8C\n\tfor ;\n\tMon, 14 Sep 2020 16:07:31 +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 relay11.mail.gandi.net (Postfix) with ESMTPSA id 272CF100014;\n\tMon, 14 Sep 2020 14:07:29 +0000 (UTC)"],"Date":"Mon, 14 Sep 2020 16:11:21 +0200","From":"Jacopo Mondi ","To":"Tomasz Figa ","Message-ID":"<20200914141121.mbjqpztfhhfihhu5@uno.localdomain>","References":"<20200909155457.153907-6-jacopo@jmondi.org>\n\t<20200910113016.GR4095624@oden.dyn.berto.se>\n\t<20200910122746.le3ndavmu6btkouk@uno.localdomain>\n\t<20200910130255.GW4095624@oden.dyn.berto.se>\n\t<20200910132255.pa4jos6xlellzrxh@uno.localdomain>\n\t\n\t<20200911162504.flumjjap5haaqpfw@uno.localdomain>\n\t<20200912094918.pqkrtsu3mj252g7l@uno.localdomain>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 5/8] libcamera: framebuffer_allocator:\n\tGet and return buffers","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 ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12517,"web_url":"https://patchwork.libcamera.org/comment/12517/","msgid":"<20200915015505.GB8166@pendragon.ideasonboard.com>","date":"2020-09-15T01:55:05","subject":"Re: [libcamera-devel] [PATCH 5/8] libcamera: framebuffer_allocator:\n\tGet and return buffers","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nCatching up on the conversation.\n\nOn Mon, Sep 14, 2020 at 04:11:21PM +0200, Jacopo Mondi wrote:\n> On Mon, Sep 14, 2020 at 03:35:32PM +0200, Tomasz Figa wrote:\n> > On Mon, Sep 14, 2020 at 7:22 AM Hirokazu Honda wrote:\n> > > On Sat, Sep 12, 2020 at 6:45 PM Jacopo Mondi wrote:\n> > > > On Fri, Sep 11, 2020 at 06:25:04PM +0200, Jacopo Mondi wrote:\n> > > > > On Fri, Sep 11, 2020 at 05:12:35PM +0900, Hirokazu Honda wrote:\n> > > > > > On Thu, Sep 10, 2020 at 10:19 PM Jacopo Mondi wrote:\n> > > > > > > On Thu, Sep 10, 2020 at 03:02:55PM +0200, Niklas Söderlund wrote:\n> > > > > > > > On 2020-09-10 14:27:46 +0200, Jacopo Mondi wrote:\n> > > > > > > > > On Thu, Sep 10, 2020 at 01:30:16PM +0200, Niklas Söderlund wrote:\n> > > > > > > > > > On 2020-09-09 17:54:54 +0200, Jacopo Mondi wrote:\n> > > > > > > > > > > Add to the FrameBufferAllocator class two methods to get and\n> > > > > > > > > > > return buffers from the pool of buffers allocated for a Stream.\n> > > > > > > > > > >\n> > > > > > > > > > > The two methods return pointer to the allocated buffers without\n\ns/pointer/pointers/\n\n> > > > > > > > > > > transferring ownership to the caller.\n> > > > > > > > > >\n> > > > > > > > > > I'm sorry I don't like this patch. If it was an interface that was\n> > > > > > > > > > internal I would be more OK with this change. The FrameBufferAllocator\n> > > > > > > > > > is facing applications and this change introduces a duality between the\n> > > > > > > > > > new getBuffer() and the existing buffers() functionality.\n> > > > > > > > > >\n> > > > > > > > > > I think it creates complexity in this user-facing API which is not\n> > > > > > > > > > really needed. How about creating a HALFrameBufferAllocator that is\n> > > > > > > > > > local to the HAL and inherits from FrameBufferAllocator while adding the\n> > > > > > > > > > features you want?\n> > > > > > > > >\n> > > > > > > > > That would end up repeating the same thing we do in pipeline handlers\n> > > > > > > > > here. Actually I found safer to provide a get/return interface than\n> > > > > > > > > giving applications full access to the pool and let them deal with\n> > > > > > > > > that. At the same time I won't prevent that completely by removing\n> > > > > > > > > buffers(), but I don't see why the 'duality' is an issue.\n> > > > > > > > >\n> > > > > > > > > I'm more than open to discuss the implementation which is surely\n> > > > > > > > > improvable and if we want to implement this feature in\n> > > > > > > > > FrameBufferAllocator or as you suggested sub-class it somehow, but I'm\n> > > > > > > > > not sure why the problem lies in having the three methods.\n> > > > > > > >\n> > > > > > > > My concern is having an application call getBuffer() to prepare one set\n> > > > > > > > of Requests, maybe one with a viewfinder + raw and then iterating over\n> > > > > > > > buffers() to create as many Requests with just viewfinder as available.\n> > > > > > > > As buffers() don't consider if it have \"given\" out any buffer prior to\n> > > > > > > > the buffers() call we have the potential to have Requests that can't be\n> > > > > > > > queued at the same time as they contain a subset of the same buffers. I\n> > > > > > > > fear this could get even worse once Request becomes reusable.\n> > > > > > >\n> > > > > > > True that a good API is one you cannot get wrong, but if an\n> > > > > > > application behaves like that [1] it means it -really- is trying to\n> > > > > > > shoot itself in the foot. I know it's a thin line though..\n> > > > > > >\n> > > > > > > I won't push for this, but if asked which interface I would prefer, I\n> > > > > > > would ask for a get/return one instead of a \"here you have all you buffers\"\n> > > > > > > one. There's also the question about buffer ownership, which are\n> > > > > > > stored as unique_ptr<> and thus connected to the lifetime of the\n> > > > > > > allocator. A 'get all the buffers' interface would allow application\n> > > > > > > to move ownership to themselves and really screw up. A stricter\n> > > > > > > interface would allow us to decide what to do: right now we 'borrow'\n> > > > > > > the buffer ownership but we might as well decide to transfer that,\n> > > > > > > but we have a single behavior and I think that's better.\n> > > > > >\n> > > > > > I am ok with this API change.\n> > > > > > I can understand the opinions of both Jacopo and Niklas.\n> > > > > > Since buffers() returns const reference of vector, client\n> > > > > > cannot takes\n> > > > > > the ownership by std::move(). Having Get/ReturnBuffer() and buffers() should not\n> > > > > > go wrong if the client adopts either way.\n> > > > >\n> > > > > mmm, you're right here, the reference is a const, there's not risk of\n> > > > > messing up the buffer ownership..\n> > > > >\n> > > > > > The thing is more like, where the buffer usage is managed, in client\n> > > > > > (with buffers()), or\n> > > > > > in FrameBufferAllocator (with Get/ReturnBuffer()).\n> > > > > > For simplicity, I think eventually FrameBufferAllocator should have\n> > > > > > GetBuffer() and ReturnBuffer().\n> > > > >\n> > > > > Yes, I think getBuffer/returnBuffer provide a managed interface that\n> > > > > ease the application life, but at the same time I won't be super happy\n> > > > > of preventing application to implement custom ones...\n> > > > >\n> > > > > Not sure, I still feel the two interfaces can live together, and if\n> > > > > someone mis-uses the two it's the same kind of error that one would do\n> > > > > if buffers are get but never returned == bad application code :)\n\nI'm with Niklas on this one, the FrameBufferAllocator is meant to\nallocate buffers, not track their usage. Maybe we'll want a buffer usage\ntracker in the public API at some point, but I would in any case keep it\nout of the FrameBufferAllocator class. It shouldn't be difficult to have\nimplement this feature in the HAL for now, it's really small.\n\n> > > > > > > [1] I'm not sure I got your example fully. Are you suggesting an\n> > > > > > > application might initially use getBuffer/returnBuffer then decide to\n> > > > > > > access the buffer vector returned by buffers() and use all of them\n> > > > > > > ignoring those are the same buffers it already got access to through\n> > > > > > > the get/return API ?\n> > > > > > >\n> > > > > > > > Understand me correctly, I'm not advocating in favor of buffers() or\n> > > > > > > > getBuffers() interface. I do however think we should only have one as it\n> > > > > > > > makes the interface easier to use for applications. As you suggest this\n> > > > > > > > new interface could replace boilerplait code in pipelines and possibly\n> > > > > > > > even applications to maybe it should replace the old buffers()\n> > > > > > > > interface?\n> > > > > > > >\n> > > > > > > > > > > Signed-off-by: Jacopo Mondi \n> > > > > > > > > > > ---\n> > > > > > > > > > > include/libcamera/framebuffer_allocator.h | 5 ++\n> > > > > > > > > > > src/libcamera/framebuffer_allocator.cpp | 60 +++++++++++++++++++++--\n> > > > > > > > > > > 2 files changed, 62 insertions(+), 3 deletions(-)\n> > > > > > > > > > >\n> > > > > > > > > > > diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h\n> > > > > > > > > > > index 2a4d538a0cb2..1f3f10d4ec03 100644\n> > > > > > > > > > > --- a/include/libcamera/framebuffer_allocator.h\n> > > > > > > > > > > +++ b/include/libcamera/framebuffer_allocator.h\n> > > > > > > > > > > @@ -9,6 +9,7 @@\n> > > > > > > > > > >\n> > > > > > > > > > > #include \n> > > > > > > > > > > #include \n> > > > > > > > > > > +#include \n> > > > > > > > > > > #include \n> > > > > > > > > > >\n> > > > > > > > > > > namespace libcamera {\n> > > > > > > > > > > @@ -33,9 +34,13 @@ public:\n> > > > > > > > > > > bool allocated() const { return !buffers_.empty(); }\n> > > > > > > > > > > const std::vector> &buffers(Stream *stream) const;\n> > > > > > > > > > >\n> > > > > > > > > > > + FrameBuffer *getBuffer(Stream *stream);\n> > > > > > > > > > > + void returnBuffer(Stream *stream, FrameBuffer *buffer);\n> > > > > > > > > > > +\n> > > > > > > > > > > private:\n> > > > > > > > > > > std::shared_ptr camera_;\n> > > > > > > > > > > std::map>> buffers_;\n> > > > > > > > > > > + std::map> availableBuffers_;\n> > > > > > > > > > > };\n> > > > > > > > > > >\n> > > > > >\n> > > > > > I think we don't have to think of order. So how about using\n> > > > > > std::vector and use the last entry of the vector?\n> > > >\n> > > > Using the last entry doesn't seem the best idea, as you can only push\n> > > > to the end of an std::vector, so you could potentially cycle over a\n> > > > single or a limited set of buffers. It wonder about concurrency also,\n> > > > if an application access the end of the vector from two different\n> > > > threads (is this possible?) we might need some kind of protection.\n> > > >\n> > > > One possible way forward would be to push to the end of the vector and\n> > > > pop from its front. We can reserve space in advance knowing the number\n> > > > of buffers, so that would be more efficient than a queue indeed.\n> > > >\n> > >\n> > > > so you could potentially cycle over a single or a limited set of buffers\n> > > Is this problematic?\n> > >\n> > > > It wonder about concurrency also, if an application access the end of the vector from two different\n> > > > threads (is this possible?) we might need some kind of protection.\n> > > I think then protection is required even if we use std::queue.\n> > > I assume the same thread operates on this interface. Is it not guaranteed?\n> > >\n> > > > > >\n> > > > > > > > > > > } /* namespace libcamera */\n> > > > > > > > > > > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp\n> > > > > > > > > > > index 7ed80011c845..7429d6b9edb7 100644\n> > > > > > > > > > > --- a/src/libcamera/framebuffer_allocator.cpp\n> > > > > > > > > > > +++ b/src/libcamera/framebuffer_allocator.cpp\n> > > > > > > > > > > @@ -64,7 +64,7 @@ FrameBufferAllocator::FrameBufferAllocator(std::shared_ptr camera)\n> > > > > > > > > > >\n> > > > > > > > > > > FrameBufferAllocator::~FrameBufferAllocator()\n> > > > > > > > > > > {\n> > > > > > > > > > > - buffers_.clear();\n> > > > > > > > > > > + clear();\n> > > > > > > > > > > }\n> > > > > > > > > > >\n> > > > > >\n> > > > > > clear() is unnecessary.\n> > > >\n> > > > As stated in the previous email, you're probably right\n> > > >\n> > > > > > > > > > > /**\n> > > > > > > > > > > @@ -93,11 +93,17 @@ int FrameBufferAllocator::allocate(Stream *stream)\n> > > > > > > > > > > }\n> > > > > > > > > > >\n> > > > > > > > > > > int ret = camera_->exportFrameBuffers(stream, &buffers_[stream]);\n> > > > > > > > > > > - if (ret == -EINVAL)\n> > > > > > > > > > > + if (ret == -EINVAL) {\n> > > > > > > > > > > LOG(Allocator, Error)\n> > > > > > > > > > > << \"Stream is not part of \" << camera_->id()\n> > > > > > > > > > > << \" active configuration\";\n> > > > > > > > > > > - return ret;\n> > > > > > > > > > > + return ret;\n> > > > > > > > > > > + }\n> > > > > > > > > > > +\n> > > > > > > > > > > + for (const auto &buffer : buffers_[stream])\n> > > > > > > > > > > + availableBuffers_[stream].push(buffer.get());\n> > > > > > > > > > > +\n> > > > > > > > > > > + return 0;\n> > > > > > > > > > > }\n> > > > > > > > > > >\n> > > > > > > > > > > /**\n> > > > > > > > > > > @@ -122,6 +128,9 @@ int FrameBufferAllocator::free(Stream *stream)\n> > > > > > > > > > > buffers.clear();\n> > > > > > > > > > > buffers_.erase(iter);\n> > > > > > > > > > >\n> > > > > > > > > > > + availableBuffers_[stream] = {};\n> > > > > > > > > > > + availableBuffers_.erase(availableBuffers_.find(stream));\n> > > > > > > > > > > +\n> > > > > >\n> > > > > > I would just availableBuffers_.erase(stream);\n> > > >\n> > > > Correct\n> > > >\n> > > > > > ditto to buffers_.\n> > > >\n> > > > Also probably correct as buffers_.erase(iter) would delete the vector\n> > > > of unique pointers there stored, causing memory to be released without\n> > > > having to\n> > > >\n> > > > std::vector> &buffers = iter->second;\n> > > > buffers.clear();\n> > >\n> > > Right. Let's do buffers_.erase(stream);\n> > >\n> > > > > > > > > > > return 0;\n> > > > > > > > > > > }\n> > > > > > > > > > >\n> > > > > > > > > > > @@ -131,6 +140,7 @@ int FrameBufferAllocator::free(Stream *stream)\n> > > > > > > > > > > void FrameBufferAllocator::clear()\n> > > > > > > > > > > {\n> > > > > > > > > > > buffers_.clear();\n> > > > > > > > > > > + availableBuffers_.clear();\n> > > > > > > > > > > }\n> > > > > > > > > > >\n> > > > > > > > > > > /**\n> > > > > > > > > > > @@ -162,4 +172,48 @@ FrameBufferAllocator::buffers(Stream *stream) const\n> > > > > > > > > > > return iter->second;\n> > > > > > > > > > > }\n> > > > > > > > > > >\n> > > > > > > > > > > +/**\n> > > > > > > > > > > + * \\brief Get a pointer to a \\a buffer for the \\a stream\n> > > > > > > > > > > + * \\param[in] stream The stream to get a buffer for\n> > > > > > > > > > > + *\n> > > > > > > > > > > + * The method returns a pointer to a FrameBuffer but does transfer the buffer\n> > > > > > > > > > > + * ownership to the caller: the returned pointer remains valid until the\n> > > > > > > > > > > + * FrameBufferAllocator does not get deleted or the allocated buffers do not get\n> > > > > > > > > > > + * released with a call for free() or clear().\n> > > > > > > > > > > + *\n> > > > > > > > > > > + * \\return A FrameBuffer pointer or nullptr if the no buffers is available\n> > > > > > > > > > > + */\n> > > > > > > > > > > +FrameBuffer *FrameBufferAllocator::getBuffer(Stream *stream)\n> > > > > > > > > > > +{\n> > > > > > > > > > > + if (!allocated() || buffers_[stream].empty())\n> > > > > > > > > > > + return nullptr;\n> > > > > > > > > > > +\n> > > > > > > > > > > + FrameBuffer *frameBuffer = availableBuffers_[stream].front();\n> > > > > > > > > > > + availableBuffers_[stream].pop();\n> > > > > > > > > > > +\n> > > > > > > > > > > + return frameBuffer;\n> > > > > > > > > > > +}\n> > > > > > > > > > > +\n> > > > > > > > > > > +/**\n> > > > > > > > > > > + * \\brief Return a \\a buffer to the list of buffers available for the a \\a stream\n> > > > > > > > > > > + * \\param[in] stream The stream to return buffer to\n> > > > > > > > > > > + * \\param[in] buffer The buffer to return\n> > > > > > > > > > > + */\n> > > > > > > > > > > +void FrameBufferAllocator::returnBuffer(Stream *stream, FrameBuffer *buffer)\n> > > > > > > > > > > +{\n> > > > > > > > > > > + if (!allocated())\n> > > > > > > > > > > + return;\n> > > > > > > > > > > +\n> > > > > > > > > > > + for (const auto &b : buffers_[stream]) {\n> > > > > > > > > > > + /*\n> > > > > > > > > > > + * Return the buffer to the available queue only if it was part\n> > > > > > > > > > > + * of the vector of buffers allocated for the Stream.\n> > > > > > > > > > > + */\n> > > > > > > > > > > + if (b.get() != buffer)\n> > > > > > > > > > > + continue;\n> > > > > > > > > > > +\n> > > > > > > > > > > + availableBuffers_[stream].push(buffer);\n> > > > > > > > > > > + }\n> > > > > > > > > > > +}\n> > > > > > > > > > > +\n> > > > > >\n> > > > > > I expect you may want to do as below. buffers_[stream] must not have\n> > > > > > the same buffers because they are unique_ptr.\n> > > > > > const auto& buffers = buffers_[stream];\n> > > > > > if (std::find_if(buffers.begin(), buffers.end(), [buffer](const auto&\n> > > > > > b) { return b.get() == buffer; } )) {\n> > > > > > availableBuffers_[stream].push(buffer);\n> > > > > > }\n> > > >\n> > > > If I got this right, your proposal is functionally equivalent but\n> > > > I find it less readable. This surely is me and my not being exactly in\n> > > > love with the ability C++ has to make easy things look complex, but in\n> > > > this case I find open coding much more explict that going through an\n> > > > std function. I suspect we could discuss this for days,\n> > > > for sure your proposal is much more C++-ish than mine, but I found\n> > > > open coding more explicit.\n> > > >\n> > > > Anyway, seems like a minor style issue, or have I missed any\n> > > > functional change?\n> > > >\n> > >\n> > > If you like for-loop, in order to insert twice, I would\n> >\n> > Do you mean \"not to insert twice\"?\n> >\n> > > for (const auto &b : buffers_[stream]) {\n> > > if (b.get() == buffer) {\n> > > availableBuffers_[stream].push(buffer);\n> > > break;\n> > > }\n> > > }\n> >\n> > In any case, this implementation has the advantage of ending the\n> > iteration when the buffer is found, without wasting CPU cycles for\n> > remaining iterations.\n> \n> Indeed I'm missing a break :D\n> I'll fix.\n> \n> > On a side note, I'm really annoyed by the lack of the find() method in\n> > std::vector, making it so inconsistent with other containers, where\n> > find() is the preferred approach for finding elements, because of\n> > container-specific implementation that gives the best complexity. It\n> > shouldn't really matter here, though, as we expect a small number of\n> > buffers anyway.\n> \n> But I can't fix this one, sorry :)","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 4FEEFC3B5B\n\tfor ;\n\tTue, 15 Sep 2020 01:55:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D7ED562D5B;\n\tTue, 15 Sep 2020 03:55:35 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D483662901\n\tfor ;\n\tTue, 15 Sep 2020 03:55:33 +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 43A0E275;\n\tTue, 15 Sep 2020 03:55:33 +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=\"CCm+n1bo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1600134933;\n\tbh=T5CwWG3C52EHFs8mO1TmNoykCRPouW9yTxnUhqqZq4c=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=CCm+n1boJr/dVyiKR0tZseBGeo+8ULYBQG7utKGFUpq34Bqlef1FCsEtmtozUcfy8\n\tvWDz9KQN3JEuJg/7o0QcUGNkjajgUS0ZDOJT2dZ6ZpOHqom9+6JaApSOi+qCGhhpTD\n\tffDy2YgmqLro1+EX0/v1wGjdYXLNMkgd/INP4bDA=","Date":"Tue, 15 Sep 2020 04:55:05 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20200915015505.GB8166@pendragon.ideasonboard.com>","References":"<20200910113016.GR4095624@oden.dyn.berto.se>\n\t<20200910122746.le3ndavmu6btkouk@uno.localdomain>\n\t<20200910130255.GW4095624@oden.dyn.berto.se>\n\t<20200910132255.pa4jos6xlellzrxh@uno.localdomain>\n\t\n\t<20200911162504.flumjjap5haaqpfw@uno.localdomain>\n\t<20200912094918.pqkrtsu3mj252g7l@uno.localdomain>\n\t\n\t\n\t<20200914141121.mbjqpztfhhfihhu5@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200914141121.mbjqpztfhhfihhu5@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 5/8] libcamera: framebuffer_allocator:\n\tGet and return buffers","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 ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12558,"web_url":"https://patchwork.libcamera.org/comment/12558/","msgid":"<20200917104350.rzzh252huvjqkgtf@uno.localdomain>","date":"2020-09-17T10:43:50","subject":"Re: [libcamera-devel] [PATCH 5/8] libcamera: framebuffer_allocator:\n\tGet and return buffers","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\nOn Tue, Sep 15, 2020 at 04:55:05AM +0300, Laurent Pinchart wrote:\n> Hello,\n>\n> Catching up on the conversation.\n\n[snip]\n\n> > > > > > > > > > >\n> > > > > > > > > > > I'm sorry I don't like this patch. If it was an interface that was\n> > > > > > > > > > > internal I would be more OK with this change. The FrameBufferAllocator\n> > > > > > > > > > > is facing applications and this change introduces a duality between the\n> > > > > > > > > > > new getBuffer() and the existing buffers() functionality.\n> > > > > > > > > > >\n> > > > > > > > > > > I think it creates complexity in this user-facing API which is not\n> > > > > > > > > > > really needed. How about creating a HALFrameBufferAllocator that is\n> > > > > > > > > > > local to the HAL and inherits from FrameBufferAllocator while adding the\n> > > > > > > > > > > features you want?\n> > > > > > > > > >\n> > > > > > > > > > That would end up repeating the same thing we do in pipeline handlers\n> > > > > > > > > > here. Actually I found safer to provide a get/return interface than\n> > > > > > > > > > giving applications full access to the pool and let them deal with\n> > > > > > > > > > that. At the same time I won't prevent that completely by removing\n> > > > > > > > > > buffers(), but I don't see why the 'duality' is an issue.\n> > > > > > > > > >\n> > > > > > > > > > I'm more than open to discuss the implementation which is surely\n> > > > > > > > > > improvable and if we want to implement this feature in\n> > > > > > > > > > FrameBufferAllocator or as you suggested sub-class it somehow, but I'm\n> > > > > > > > > > not sure why the problem lies in having the three methods.\n> > > > > > > > >\n> > > > > > > > > My concern is having an application call getBuffer() to prepare one set\n> > > > > > > > > of Requests, maybe one with a viewfinder + raw and then iterating over\n> > > > > > > > > buffers() to create as many Requests with just viewfinder as available.\n> > > > > > > > > As buffers() don't consider if it have \"given\" out any buffer prior to\n> > > > > > > > > the buffers() call we have the potential to have Requests that can't be\n> > > > > > > > > queued at the same time as they contain a subset of the same buffers. I\n> > > > > > > > > fear this could get even worse once Request becomes reusable.\n> > > > > > > >\n> > > > > > > > True that a good API is one you cannot get wrong, but if an\n> > > > > > > > application behaves like that [1] it means it -really- is trying to\n> > > > > > > > shoot itself in the foot. I know it's a thin line though..\n> > > > > > > >\n> > > > > > > > I won't push for this, but if asked which interface I would prefer, I\n> > > > > > > > would ask for a get/return one instead of a \"here you have all you buffers\"\n> > > > > > > > one. There's also the question about buffer ownership, which are\n> > > > > > > > stored as unique_ptr<> and thus connected to the lifetime of the\n> > > > > > > > allocator. A 'get all the buffers' interface would allow application\n> > > > > > > > to move ownership to themselves and really screw up. A stricter\n> > > > > > > > interface would allow us to decide what to do: right now we 'borrow'\n> > > > > > > > the buffer ownership but we might as well decide to transfer that,\n> > > > > > > > but we have a single behavior and I think that's better.\n> > > > > > >\n> > > > > > > I am ok with this API change.\n> > > > > > > I can understand the opinions of both Jacopo and Niklas.\n> > > > > > > Since buffers() returns const reference of vector, client\n> > > > > > > cannot takes\n> > > > > > > the ownership by std::move(). Having Get/ReturnBuffer() and buffers() should not\n> > > > > > > go wrong if the client adopts either way.\n> > > > > >\n> > > > > > mmm, you're right here, the reference is a const, there's not risk of\n> > > > > > messing up the buffer ownership..\n> > > > > >\n> > > > > > > The thing is more like, where the buffer usage is managed, in client\n> > > > > > > (with buffers()), or\n> > > > > > > in FrameBufferAllocator (with Get/ReturnBuffer()).\n> > > > > > > For simplicity, I think eventually FrameBufferAllocator should have\n> > > > > > > GetBuffer() and ReturnBuffer().\n> > > > > >\n> > > > > > Yes, I think getBuffer/returnBuffer provide a managed interface that\n> > > > > > ease the application life, but at the same time I won't be super happy\n> > > > > > of preventing application to implement custom ones...\n> > > > > >\n> > > > > > Not sure, I still feel the two interfaces can live together, and if\n> > > > > > someone mis-uses the two it's the same kind of error that one would do\n> > > > > > if buffers are get but never returned == bad application code :)\n>\n> I'm with Niklas on this one, the FrameBufferAllocator is meant to\n> allocate buffers, not track their usage. Maybe we'll want a buffer usage\n> tracker in the public API at some point, but I would in any case keep it\n> out of the FrameBufferAllocator class. It shouldn't be difficult to have\n> implement this feature in the HAL for now, it's really small.\n>\n\nThat's where I implemented it originally. But I ended up\nre-implementing the same thing we do in pipeline handlers for the same\npurpose.\n\nAnyway, I'll go with the majority and move this to the HAL\n\nThanks\n j\n\n> > > > > > > > [1] I'm not sure I got your example fully. Are you suggesting an\n> > > > > > > > application might initially use getBuffer/returnBuffer then decide to\n> > > > > > > > access the buffer vector returned by buffers() and use all of them\n> > > > > > > > ignoring those are the same buffers it already got access to through\n> > > > > > > > the get/return API ?\n> > > > > > > >\n> > > > > > > > > Understand me correctly, I'm not advocating in favor of buffers() or\n> > > > > > > > > getBuffers() interface. I do however think we should only have one as it\n> > > > > > > > > makes the interface easier to use for applications. As you suggest this\n> > > > > > > > > new interface could replace boilerplait code in pipelines and possibly\n> > > > > > > > > even applications to maybe it should replace the old buffers()\n> > > > > > > > > interface?\n> > > > > > > > >\n> > > > > > > > > > > > Signed-off-by: Jacopo Mondi \n> > > > > > > > > > > > ---\n> > > > > > > > > > > > include/libcamera/framebuffer_allocator.h | 5 ++\n> > > > > > > > > > > > src/libcamera/framebuffer_allocator.cpp | 60 +++++++++++++++++++++--\n> > > > > > > > > > > > 2 files changed, 62 insertions(+), 3 deletions(-)\n> > > > > > > > > > > >\n> > > > > > > > > > > > diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h\n> > > > > > > > > > > > index 2a4d538a0cb2..1f3f10d4ec03 100644\n> > > > > > > > > > > > --- a/include/libcamera/framebuffer_allocator.h\n> > > > > > > > > > > > +++ b/include/libcamera/framebuffer_allocator.h\n> > > > > > > > > > > > @@ -9,6 +9,7 @@\n> > > > > > > > > > > >\n> > > > > > > > > > > > #include \n> > > > > > > > > > > > #include \n> > > > > > > > > > > > +#include \n> > > > > > > > > > > > #include \n> > > > > > > > > > > >\n> > > > > > > > > > > > namespace libcamera {\n> > > > > > > > > > > > @@ -33,9 +34,13 @@ public:\n> > > > > > > > > > > > bool allocated() const { return !buffers_.empty(); }\n> > > > > > > > > > > > const std::vector> &buffers(Stream *stream) const;\n> > > > > > > > > > > >\n> > > > > > > > > > > > + FrameBuffer *getBuffer(Stream *stream);\n> > > > > > > > > > > > + void returnBuffer(Stream *stream, FrameBuffer *buffer);\n> > > > > > > > > > > > +\n> > > > > > > > > > > > private:\n> > > > > > > > > > > > std::shared_ptr camera_;\n> > > > > > > > > > > > std::map>> buffers_;\n> > > > > > > > > > > > + std::map> availableBuffers_;\n> > > > > > > > > > > > };\n> > > > > > > > > > > >\n> > > > > > >\n> > > > > > > I think we don't have to think of order. So how about using\n> > > > > > > std::vector and use the last entry of the vector?\n> > > > >\n> > > > > Using the last entry doesn't seem the best idea, as you can only push\n> > > > > to the end of an std::vector, so you could potentially cycle over a\n> > > > > single or a limited set of buffers. It wonder about concurrency also,\n> > > > > if an application access the end of the vector from two different\n> > > > > threads (is this possible?) we might need some kind of protection.\n> > > > >\n> > > > > One possible way forward would be to push to the end of the vector and\n> > > > > pop from its front. We can reserve space in advance knowing the number\n> > > > > of buffers, so that would be more efficient than a queue indeed.\n> > > > >\n> > > >\n> > > > > so you could potentially cycle over a single or a limited set of buffers\n> > > > Is this problematic?\n> > > >\n> > > > > It wonder about concurrency also, if an application access the end of the vector from two different\n> > > > > threads (is this possible?) we might need some kind of protection.\n> > > > I think then protection is required even if we use std::queue.\n> > > > I assume the same thread operates on this interface. Is it not guaranteed?\n> > > >\n> > > > > > >\n> > > > > > > > > > > > } /* namespace libcamera */\n> > > > > > > > > > > > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp\n> > > > > > > > > > > > index 7ed80011c845..7429d6b9edb7 100644\n> > > > > > > > > > > > --- a/src/libcamera/framebuffer_allocator.cpp\n> > > > > > > > > > > > +++ b/src/libcamera/framebuffer_allocator.cpp\n> > > > > > > > > > > > @@ -64,7 +64,7 @@ FrameBufferAllocator::FrameBufferAllocator(std::shared_ptr camera)\n> > > > > > > > > > > >\n> > > > > > > > > > > > FrameBufferAllocator::~FrameBufferAllocator()\n> > > > > > > > > > > > {\n> > > > > > > > > > > > - buffers_.clear();\n> > > > > > > > > > > > + clear();\n> > > > > > > > > > > > }\n> > > > > > > > > > > >\n> > > > > > >\n> > > > > > > clear() is unnecessary.\n> > > > >\n> > > > > As stated in the previous email, you're probably right\n> > > > >\n> > > > > > > > > > > > /**\n> > > > > > > > > > > > @@ -93,11 +93,17 @@ int FrameBufferAllocator::allocate(Stream *stream)\n> > > > > > > > > > > > }\n> > > > > > > > > > > >\n> > > > > > > > > > > > int ret = camera_->exportFrameBuffers(stream, &buffers_[stream]);\n> > > > > > > > > > > > - if (ret == -EINVAL)\n> > > > > > > > > > > > + if (ret == -EINVAL) {\n> > > > > > > > > > > > LOG(Allocator, Error)\n> > > > > > > > > > > > << \"Stream is not part of \" << camera_->id()\n> > > > > > > > > > > > << \" active configuration\";\n> > > > > > > > > > > > - return ret;\n> > > > > > > > > > > > + return ret;\n> > > > > > > > > > > > + }\n> > > > > > > > > > > > +\n> > > > > > > > > > > > + for (const auto &buffer : buffers_[stream])\n> > > > > > > > > > > > + availableBuffers_[stream].push(buffer.get());\n> > > > > > > > > > > > +\n> > > > > > > > > > > > + return 0;\n> > > > > > > > > > > > }\n> > > > > > > > > > > >\n> > > > > > > > > > > > /**\n> > > > > > > > > > > > @@ -122,6 +128,9 @@ int FrameBufferAllocator::free(Stream *stream)\n> > > > > > > > > > > > buffers.clear();\n> > > > > > > > > > > > buffers_.erase(iter);\n> > > > > > > > > > > >\n> > > > > > > > > > > > + availableBuffers_[stream] = {};\n> > > > > > > > > > > > + availableBuffers_.erase(availableBuffers_.find(stream));\n> > > > > > > > > > > > +\n> > > > > > >\n> > > > > > > I would just availableBuffers_.erase(stream);\n> > > > >\n> > > > > Correct\n> > > > >\n> > > > > > > ditto to buffers_.\n> > > > >\n> > > > > Also probably correct as buffers_.erase(iter) would delete the vector\n> > > > > of unique pointers there stored, causing memory to be released without\n> > > > > having to\n> > > > >\n> > > > > std::vector> &buffers = iter->second;\n> > > > > buffers.clear();\n> > > >\n> > > > Right. Let's do buffers_.erase(stream);\n> > > >\n> > > > > > > > > > > > return 0;\n> > > > > > > > > > > > }\n> > > > > > > > > > > >\n> > > > > > > > > > > > @@ -131,6 +140,7 @@ int FrameBufferAllocator::free(Stream *stream)\n> > > > > > > > > > > > void FrameBufferAllocator::clear()\n> > > > > > > > > > > > {\n> > > > > > > > > > > > buffers_.clear();\n> > > > > > > > > > > > + availableBuffers_.clear();\n> > > > > > > > > > > > }\n> > > > > > > > > > > >\n> > > > > > > > > > > > /**\n> > > > > > > > > > > > @@ -162,4 +172,48 @@ FrameBufferAllocator::buffers(Stream *stream) const\n> > > > > > > > > > > > return iter->second;\n> > > > > > > > > > > > }\n> > > > > > > > > > > >\n> > > > > > > > > > > > +/**\n> > > > > > > > > > > > + * \\brief Get a pointer to a \\a buffer for the \\a stream\n> > > > > > > > > > > > + * \\param[in] stream The stream to get a buffer for\n> > > > > > > > > > > > + *\n> > > > > > > > > > > > + * The method returns a pointer to a FrameBuffer but does transfer the buffer\n> > > > > > > > > > > > + * ownership to the caller: the returned pointer remains valid until the\n> > > > > > > > > > > > + * FrameBufferAllocator does not get deleted or the allocated buffers do not get\n> > > > > > > > > > > > + * released with a call for free() or clear().\n> > > > > > > > > > > > + *\n> > > > > > > > > > > > + * \\return A FrameBuffer pointer or nullptr if the no buffers is available\n> > > > > > > > > > > > + */\n> > > > > > > > > > > > +FrameBuffer *FrameBufferAllocator::getBuffer(Stream *stream)\n> > > > > > > > > > > > +{\n> > > > > > > > > > > > + if (!allocated() || buffers_[stream].empty())\n> > > > > > > > > > > > + return nullptr;\n> > > > > > > > > > > > +\n> > > > > > > > > > > > + FrameBuffer *frameBuffer = availableBuffers_[stream].front();\n> > > > > > > > > > > > + availableBuffers_[stream].pop();\n> > > > > > > > > > > > +\n> > > > > > > > > > > > + return frameBuffer;\n> > > > > > > > > > > > +}\n> > > > > > > > > > > > +\n> > > > > > > > > > > > +/**\n> > > > > > > > > > > > + * \\brief Return a \\a buffer to the list of buffers available for the a \\a stream\n> > > > > > > > > > > > + * \\param[in] stream The stream to return buffer to\n> > > > > > > > > > > > + * \\param[in] buffer The buffer to return\n> > > > > > > > > > > > + */\n> > > > > > > > > > > > +void FrameBufferAllocator::returnBuffer(Stream *stream, FrameBuffer *buffer)\n> > > > > > > > > > > > +{\n> > > > > > > > > > > > + if (!allocated())\n> > > > > > > > > > > > + return;\n> > > > > > > > > > > > +\n> > > > > > > > > > > > + for (const auto &b : buffers_[stream]) {\n> > > > > > > > > > > > + /*\n> > > > > > > > > > > > + * Return the buffer to the available queue only if it was part\n> > > > > > > > > > > > + * of the vector of buffers allocated for the Stream.\n> > > > > > > > > > > > + */\n> > > > > > > > > > > > + if (b.get() != buffer)\n> > > > > > > > > > > > + continue;\n> > > > > > > > > > > > +\n> > > > > > > > > > > > + availableBuffers_[stream].push(buffer);\n> > > > > > > > > > > > + }\n> > > > > > > > > > > > +}\n> > > > > > > > > > > > +\n> > > > > > >\n> > > > > > > I expect you may want to do as below. buffers_[stream] must not have\n> > > > > > > the same buffers because they are unique_ptr.\n> > > > > > > const auto& buffers = buffers_[stream];\n> > > > > > > if (std::find_if(buffers.begin(), buffers.end(), [buffer](const auto&\n> > > > > > > b) { return b.get() == buffer; } )) {\n> > > > > > > availableBuffers_[stream].push(buffer);\n> > > > > > > }\n> > > > >\n> > > > > If I got this right, your proposal is functionally equivalent but\n> > > > > I find it less readable. This surely is me and my not being exactly in\n> > > > > love with the ability C++ has to make easy things look complex, but in\n> > > > > this case I find open coding much more explict that going through an\n> > > > > std function. I suspect we could discuss this for days,\n> > > > > for sure your proposal is much more C++-ish than mine, but I found\n> > > > > open coding more explicit.\n> > > > >\n> > > > > Anyway, seems like a minor style issue, or have I missed any\n> > > > > functional change?\n> > > > >\n> > > >\n> > > > If you like for-loop, in order to insert twice, I would\n> > >\n> > > Do you mean \"not to insert twice\"?\n> > >\n> > > > for (const auto &b : buffers_[stream]) {\n> > > > if (b.get() == buffer) {\n> > > > availableBuffers_[stream].push(buffer);\n> > > > break;\n> > > > }\n> > > > }\n> > >\n> > > In any case, this implementation has the advantage of ending the\n> > > iteration when the buffer is found, without wasting CPU cycles for\n> > > remaining iterations.\n> >\n> > Indeed I'm missing a break :D\n> > I'll fix.\n> >\n> > > On a side note, I'm really annoyed by the lack of the find() method in\n> > > std::vector, making it so inconsistent with other containers, where\n> > > find() is the preferred approach for finding elements, because of\n> > > container-specific implementation that gives the best complexity. It\n> > > shouldn't really matter here, though, as we expect a small number of\n> > > buffers anyway.\n> >\n> > But I can't fix this one, sorry :)\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 5CA62BF01C\n\tfor ;\n\tThu, 17 Sep 2020 10:40:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E9ACF62F53;\n\tThu, 17 Sep 2020 12:40:00 +0200 (CEST)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DCC516036A\n\tfor ;\n\tThu, 17 Sep 2020 12:39:59 +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 relay11.mail.gandi.net (Postfix) with ESMTPSA id BCF56100009;\n\tThu, 17 Sep 2020 10:39:58 +0000 (UTC)"],"Date":"Thu, 17 Sep 2020 12:43:50 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20200917104350.rzzh252huvjqkgtf@uno.localdomain>","References":"<20200910122746.le3ndavmu6btkouk@uno.localdomain>\n\t<20200910130255.GW4095624@oden.dyn.berto.se>\n\t<20200910132255.pa4jos6xlellzrxh@uno.localdomain>\n\t\n\t<20200911162504.flumjjap5haaqpfw@uno.localdomain>\n\t<20200912094918.pqkrtsu3mj252g7l@uno.localdomain>\n\t\n\t\n\t<20200914141121.mbjqpztfhhfihhu5@uno.localdomain>\n\t<20200915015505.GB8166@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200915015505.GB8166@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 5/8] libcamera: framebuffer_allocator:\n\tGet and return buffers","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]