[{"id":21469,"web_url":"https://patchwork.libcamera.org/comment/21469/","msgid":"<20211130215241.ohm67tp6aedahcvs@uno.localdomain>","date":"2021-11-30T21:52:41","subject":"Re: [libcamera-devel] [PATCH 2/2] android: camera_stream: Use\n\tPlatformFrameBufferAllocator","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro\n\nOn Tue, Nov 30, 2021 at 09:44:28PM +0900, Hirokazu Honda wrote:\n> CameraStream originally creates FrameBuffers by\n> FrameBufferAllocator and thus buffers are allocated in V4L2 API.\n> This replaces the allocator in CameraStream with\n> PlatformFrameBufferAllocator. It allocates a buffer in a platform\n> dependent graphic buffer allocation API.\n>\n> Signed-off-by: Hirokazu Honda \n> ---\n> src/android/camera_stream.cpp | 24 +++++++++++++-----------\n> src/android/camera_stream.h | 5 +++--\n> 2 files changed, 16 insertions(+), 13 deletions(-)\n>\n> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> index 9023c13c..d22dd6b3 100644\n> --- a/src/android/camera_stream.cpp\n> +++ b/src/android/camera_stream.cpp\n> @@ -22,6 +22,7 @@\n> #include \"camera_capabilities.h\"\n> #include \"camera_device.h\"\n> #include \"camera_metadata.h\"\n> +#include \"frame_buffer_allocator.h\"\n> #include \"post_processor.h\"\n>\n> using namespace libcamera;\n> @@ -118,16 +119,8 @@ int CameraStream::configure()\n> \t}\n>\n> \tif (type_ == Type::Internal) {\n> -\t\tallocator_ = std::make_unique(cameraDevice_->camera());\n> +\t\tallocator_ = std::make_unique(cameraDevice_);\n> \t\tmutex_ = std::make_unique();\n> -\n> -\t\tint ret = allocator_->allocate(stream());\n> -\t\tif (ret < 0)\n> -\t\t\treturn ret;\n> -\n> -\t\t/* Save a pointer to the reserved frame buffers */\n> -\t\tfor (const auto &frameBuffer : allocator_->buffers(stream()))\n> -\t\t\tbuffers_.push_back(frameBuffer.get());\n> \t}\n>\n> \tcamera3Stream_->max_buffers = configuration().bufferCount;\n> @@ -211,8 +204,17 @@ FrameBuffer *CameraStream::getBuffer()\n> \tstd::lock_guard locker(*mutex_);\n>\n> \tif (buffers_.empty()) {\n> -\t\tLOG(HAL, Error) << \"Buffer underrun\";\n> -\t\treturn nullptr;\n> +\t\tconst int frameFormat =\n> +\t\t\tcameraDevice_->capabilities()->toAndroidFormat(\n> +\t\t\t\tconfiguration().pixelFormat);\n> +\t\tif (frameFormat == -1)\n> +\t\t\treturn nullptr;\n\nI would be tempted to ASSERT to catch a development error earlier\n\n> +\n> +\t\tauto frameBuffer = allocator_->allocate(frameFormat,\n> +\t\t\t\t\t\t\tconfiguration().size,\n> +\t\t\t\t\t\t\tcamera3Stream_->usage);\n\nshould you check for !framebuffer ?\n\n> +\t\tallocatedBuffers_.push_back(std::move(frameBuffer));\n> +\t\tbuffers_.emplace_back(allocatedBuffers_.back().get());\n\nOh nice, so this allocates buffers on demand. I guess the max number\nof allocated buffers is equal to the max number of requests in flight,\nso a rather small one\n\nI wonder, and I honestly don't know the answer or what the usual usage\npattern is: do we need to ever keep a cache of allocated buffers ? Is\nallocation/deallocation expensive and should be minimized, or is it\nbetter to allocate and release every time ? I suspect reusing the same\nframebuffers probably maximizes the cache hits in the v4l2 device\ndmabuf mappings but I'm not sure what the usual usage pattern is. How\nis this commonly used in cros ?\n\n> \t}\n>\n> \tFrameBuffer *buffer = buffers_.back();\n> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> index 0c402deb..190ac6c0 100644\n> --- a/src/android/camera_stream.h\n> +++ b/src/android/camera_stream.h\n> @@ -19,7 +19,6 @@\n>\n> #include \n> #include \n> -#include \n> #include \n> #include \n>\n> @@ -27,6 +26,7 @@\n> #include \"post_processor.h\"\n>\n> class CameraDevice;\n> +class PlatformFrameBufferAllocator;\n>\n> class CameraStream\n> {\n> @@ -168,7 +168,8 @@ private:\n> \tcamera3_stream_t *camera3Stream_;\n> \tconst unsigned int index_;\n>\n> -\tstd::unique_ptr allocator_;\n> +\tstd::unique_ptr allocator_;\n> +\tstd::vector> allocatedBuffers_;\n\nIf I recall correctly one of the two backends had as requirement that\nframebuffers should be deleted before the allocator. If that's the\ncase, should we manually clear the vector then release the allocator_\nin the destructor ?\n\n> \tstd::vector buffers_;\n> \t/*\n> \t * The class has to be MoveConstructible as instances are stored in\n> --\n> 2.34.0.rc2.393.gf8c9666880-goog\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 4962DBDB13\n\tfor ;\n\tTue, 30 Nov 2021 21:51:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 88EA660718;\n\tTue, 30 Nov 2021 22:51:51 +0100 (CET)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7421E605C4\n\tfor ;\n\tTue, 30 Nov 2021 22:51:50 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id 8B03A100002;\n\tTue, 30 Nov 2021 21:51:49 +0000 (UTC)"],"Date":"Tue, 30 Nov 2021 22:52:41 +0100","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20211130215241.ohm67tp6aedahcvs@uno.localdomain>","References":"<20211130124428.2163669-1-hiroh@chromium.org>\n\t<20211130124428.2163669-3-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211130124428.2163669-3-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH 2/2] android: camera_stream: Use\n\tPlatformFrameBufferAllocator","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]