[{"id":26096,"web_url":"https://patchwork.libcamera.org/comment/26096/","msgid":"<20221216141535.ssvpmb3as55ajwy6@uno.localdomain>","date":"2022-12-16T14:15:35","subject":"Re: [libcamera-devel] [PATCH v9 03/18] libcamera:\n\tframebuffer_allocator: Make allocate() require count","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul\n\nOn Fri, Dec 16, 2022 at 09:29:24PM +0900, Paul Elder via libcamera-devel wrote:\n> From: Nícolas F. R. A. Prado \n>\n> Make FrameBufferAllocator::allocate() require a 'count' argument for the\n> number of buffers to be allocated.\n>\n> Signed-off-by: Nícolas F. R. A. Prado \n> Reviewed-by: Kieran Bingham \n> Reviewed-by: Laurent Pinchart \n> Reviewed-by: Paul Elder \n> Signed-off-by: Paul Elder \n>\n\nSeeing this being extensively reviewed, I won't comment on the change\nto the public API interface. I'm a bit afraid we're putting on\napplications the burden of getting this right, when they probably\ndon't care in most cases..\n\n> ---\n\n[snip]\n\n> /**\n> diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp\n> index dabd9219..6a0bb8df 100644\n> --- a/src/libcamera/framebuffer_allocator.cpp\n> +++ b/src/libcamera/framebuffer_allocator.cpp\n> @@ -71,6 +71,7 @@ FrameBufferAllocator::~FrameBufferAllocator()\n> /**\n> * \\brief Allocate buffers for a configured stream\n> * \\param[in] stream The stream to allocate buffers for\n> + * \\param[in] count The number of buffers to allocate\n> *\n> * Allocate buffers suitable for capturing frames from the \\a stream. The Camera\n> * shall have been previously configured with Camera::configure() and shall be\n> @@ -79,6 +80,10 @@ FrameBufferAllocator::~FrameBufferAllocator()\n> * Upon successful allocation, the allocated buffers can be retrieved with the\n> * buffers() function.\n> *\n> + * This function may allocate less buffers than requested, due to memory and\n> + * other system constraints. The caller shall always check the return value to\n> + * verify if the number of allocate buffers matches its needs.\n> + *\n> * \\return The number of allocated buffers on success or a negative error code\n> * otherwise\n> * \\retval -EACCES The camera is not in a state where buffers can be allocated\n> @@ -86,7 +91,7 @@ FrameBufferAllocator::~FrameBufferAllocator()\n> * not part of the active camera configuration\n> * \\retval -EBUSY Buffers are already allocated for the \\a stream\n> */\n> -int FrameBufferAllocator::allocate(Stream *stream)\n> +int FrameBufferAllocator::allocate(Stream *stream, unsigned int count)\n> {\n> \tconst auto &[it, inserted] = buffers_.try_emplace(stream);\n>\n> @@ -95,7 +100,7 @@ int FrameBufferAllocator::allocate(Stream *stream)\n> \t\treturn -EBUSY;\n> \t}\n>\n> -\tint ret = camera_->exportFrameBuffers(stream, &it->second);\n> +\tint ret = camera_->exportFrameBuffers(stream, count, &it->second);\n\nWe have the camera, we can access properties, count could be\n\n count = max(properties::MinimumRequests, count)\n\nA detail through\n\nReviewed-by: Jacopo Mondi \n\n\n> \tif (ret == -EINVAL)\n> \t\tLOG(Allocator, Error)\n> \t\t\t<< \"Stream is not part of \" << camera_->id()\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 98a4a3e5..bab2db65 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -140,7 +140,7 @@ public:\n> \t\tconst StreamRoles &roles) override;\n> \tint configure(Camera *camera, CameraConfiguration *config) override;\n>\n> -\tint exportFrameBuffers(Camera *camera, Stream *stream,\n> +\tint exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,\n> \t\t\t std::vector> *buffers) override;\n>\n> \tint start(Camera *camera, const ControlList *controls) override;\n> @@ -687,10 +687,10 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> }\n>\n> int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,\n> +\t\t\t\t\t unsigned int count,\n> \t\t\t\t\t std::vector> *buffers)\n> {\n> \tIPU3CameraData *data = cameraData(camera);\n> -\tunsigned int count = stream->configuration().bufferCount;\n>\n> \tif (stream == &data->outStream_)\n> \t\treturn data->imgu_->output_->exportBuffers(count, buffers);\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 4a08d01e..4641c76f 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -328,7 +328,7 @@ public:\n> \t\tconst StreamRoles &roles) override;\n> \tint configure(Camera *camera, CameraConfiguration *config) override;\n>\n> -\tint exportFrameBuffers(Camera *camera, Stream *stream,\n> +\tint exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,\n> \t\t\t std::vector> *buffers) override;\n>\n> \tint start(Camera *camera, const ControlList *controls) override;\n> @@ -1005,10 +1005,10 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> }\n>\n> int PipelineHandlerRPi::exportFrameBuffers([[maybe_unused]] Camera *camera, Stream *stream,\n> +\t\t\t\t\t unsigned int count,\n> \t\t\t\t\t std::vector> *buffers)\n> {\n> \tRPi::Stream *s = static_cast(stream);\n> -\tunsigned int count = stream->configuration().bufferCount;\n> \tint ret = s->dev()->exportBuffers(count, buffers);\n>\n> \ts->setExportedBuffers(buffers);\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 45b6beaf..8fe37c4f 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -148,7 +148,7 @@ public:\n> \t\tconst StreamRoles &roles) override;\n> \tint configure(Camera *camera, CameraConfiguration *config) override;\n>\n> -\tint exportFrameBuffers(Camera *camera, Stream *stream,\n> +\tint exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,\n> \t\t\t std::vector> *buffers) override;\n>\n> \tint start(Camera *camera, const ControlList *controls) override;\n> @@ -816,10 +816,10 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> }\n>\n> int PipelineHandlerRkISP1::exportFrameBuffers([[maybe_unused]] Camera *camera, Stream *stream,\n> +\t\t\t\t\t unsigned int count,\n> \t\t\t\t\t std::vector> *buffers)\n> {\n> \tRkISP1CameraData *data = cameraData(camera);\n> -\tunsigned int count = stream->configuration().bufferCount;\n>\n> \tif (stream == &data->mainPathStream_)\n> \t\treturn mainPath_.exportBuffers(count, buffers);\n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index 8ed983fe..6b7c6d5c 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -322,7 +322,7 @@ public:\n> \t\tconst StreamRoles &roles) override;\n> \tint configure(Camera *camera, CameraConfiguration *config) override;\n>\n> -\tint exportFrameBuffers(Camera *camera, Stream *stream,\n> +\tint exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,\n> \t\t\t std::vector> *buffers) override;\n>\n> \tint start(Camera *camera, const ControlList *controls) override;\n> @@ -1202,10 +1202,10 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)\n> }\n>\n> int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream,\n> +\t\t\t\t\t unsigned int count,\n> \t\t\t\t\t std::vector> *buffers)\n> {\n> \tSimpleCameraData *data = cameraData(camera);\n> -\tunsigned int count = stream->configuration().bufferCount;\n>\n> \t/*\n> \t * Export buffers on the converter or capture video node, depending on\n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index 7f580955..4ce240a4 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -78,7 +78,7 @@ public:\n> \t\tconst StreamRoles &roles) override;\n> \tint configure(Camera *camera, CameraConfiguration *config) override;\n>\n> -\tint exportFrameBuffers(Camera *camera, Stream *stream,\n> +\tint exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,\n> \t\t\t std::vector> *buffers) override;\n>\n> \tint start(Camera *camera, const ControlList *controls) override;\n> @@ -226,11 +226,12 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config)\n> \treturn 0;\n> }\n>\n> -int PipelineHandlerUVC::exportFrameBuffers(Camera *camera, Stream *stream,\n> +int PipelineHandlerUVC::exportFrameBuffers(Camera *camera,\n> +\t\t\t\t\t [[maybe_unused]] Stream *stream,\n> +\t\t\t\t\t unsigned int count,\n> \t\t\t\t\t std::vector> *buffers)\n> {\n> \tUVCCameraData *data = cameraData(camera);\n> -\tunsigned int count = stream->configuration().bufferCount;\n>\n> \treturn data->video_->exportBuffers(count, buffers);\n> }\n> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> index d2633be4..e58caf99 100644\n> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> @@ -89,7 +89,7 @@ public:\n> \t\tconst StreamRoles &roles) override;\n> \tint configure(Camera *camera, CameraConfiguration *config) override;\n>\n> -\tint exportFrameBuffers(Camera *camera, Stream *stream,\n> +\tint exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,\n> \t\t\t std::vector> *buffers) override;\n>\n> \tint start(Camera *camera, const ControlList *controls) override;\n> @@ -321,11 +321,12 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)\n> \treturn 0;\n> }\n>\n> -int PipelineHandlerVimc::exportFrameBuffers(Camera *camera, Stream *stream,\n> +int PipelineHandlerVimc::exportFrameBuffers(Camera *camera,\n> +\t\t\t\t\t [[maybe_unused]] Stream *stream,\n> +\t\t\t\t\t unsigned int count,\n> \t\t\t\t\t std::vector> *buffers)\n> {\n> \tVimcCameraData *data = cameraData(camera);\n> -\tunsigned int count = stream->configuration().bufferCount;\n>\n> \treturn data->video_->exportBuffers(count, buffers);\n> }\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index cfade490..fffbd51b 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -280,6 +280,7 @@ void PipelineHandler::unlockMediaDevices()\n> * \\brief Allocate and export buffers for \\a stream\n> * \\param[in] camera The camera\n> * \\param[in] stream The stream to allocate buffers for\n> + * \\param[in] count The number of buffers to allocate\n> * \\param[out] buffers Array of buffers successfully allocated\n> *\n> * This function allocates buffers for the \\a stream from the devices associated\n> diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp\n> index 7b97c2d5..1c5fab64 100644\n> --- a/src/v4l2/v4l2_camera.cpp\n> +++ b/src/v4l2/v4l2_camera.cpp\n> @@ -160,7 +160,7 @@ int V4L2Camera::allocBuffers(unsigned int count)\n> {\n> \tStream *stream = config_->at(0).stream();\n>\n> -\tint ret = bufferAllocator_->allocate(stream);\n> +\tint ret = bufferAllocator_->allocate(stream, count);\n> \tif (ret < 0)\n> \t\treturn ret;\n>\n> diff --git a/test/camera/camera_reconfigure.cpp b/test/camera/camera_reconfigure.cpp\n> index 06c87730..63a97863 100644\n> --- a/test/camera/camera_reconfigure.cpp\n> +++ b/test/camera/camera_reconfigure.cpp\n> @@ -17,6 +17,7 @@\n> #include \n>\n> #include \n> +#include \n>\n> #include \"camera_test.h\"\n> #include \"test.h\"\n> @@ -78,7 +79,9 @@ private:\n> \t\t * same buffer allocation for each run.\n> \t\t */\n> \t\tif (!allocated_) {\n> -\t\t\tint ret = allocator_->allocate(stream);\n> +\t\t\tunsigned int bufferCount =\n> +\t\t\t\tcamera_->properties().get(properties::MinimumRequests).value();\n> +\t\t\tint ret = allocator_->allocate(stream, bufferCount);\n> \t\t\tif (ret < 0) {\n> \t\t\t\tcerr << \"Failed to allocate buffers\" << endl;\n> \t\t\t\treturn TestFail;\n> diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp\n> index de824083..64d3a8e7 100644\n> --- a/test/camera/capture.cpp\n> +++ b/test/camera/capture.cpp\n> @@ -7,12 +7,13 @@\n>\n> #include \n>\n> -#include \n> -\n> #include \n> #include \n> #include \n>\n> +#include \n> +#include \n> +\n> #include \"camera_test.h\"\n> #include \"test.h\"\n>\n> @@ -98,8 +99,10 @@ protected:\n>\n> \t\tStream *stream = cfg.stream();\n>\n> -\t\tint ret = allocator_->allocate(stream);\n> -\t\tif (ret < 0)\n> +\t\tunsigned int bufferCount =\n> +\t\t\tcamera_->properties().get(properties::MinimumRequests).value();\n> +\t\tint ret = allocator_->allocate(stream, bufferCount);\n> +\t\tif (ret < static_cast(bufferCount))\n> \t\t\treturn TestFail;\n>\n> \t\tfor (const std::unique_ptr &buffer : allocator_->buffers(stream)) {\n> diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp\n> index 9c2b0c6a..a9ddb323 100644\n> --- a/test/camera/statemachine.cpp\n> +++ b/test/camera/statemachine.cpp\n> @@ -8,6 +8,7 @@\n> #include \n>\n> #include \n> +#include \n>\n> #include \"camera_test.h\"\n> #include \"test.h\"\n> @@ -120,7 +121,9 @@ protected:\n> \t\t/* Use internally allocated buffers. */\n> \t\tallocator_ = new FrameBufferAllocator(camera_);\n> \t\tStream *stream = *camera_->streams().begin();\n> -\t\tif (allocator_->allocate(stream) < 0)\n> +\t\tunsigned int bufferCount =\n> +\t\t\tcamera_->properties().get(properties::MinimumRequests).value();\n> +\t\tif (allocator_->allocate(stream, bufferCount) < 0)\n> \t\t\treturn TestFail;\n>\n> \t\tif (camera_->start())\n> diff --git a/test/fence.cpp b/test/fence.cpp\n> index 1e38bc2f..88ce2857 100644\n> --- a/test/fence.cpp\n> +++ b/test/fence.cpp\n> @@ -18,6 +18,7 @@\n>\n> #include \n> #include \n> +#include \n>\n> #include \"camera_test.h\"\n> #include \"test.h\"\n> @@ -117,8 +118,11 @@ int FenceTest::init()\n> \tStreamConfiguration &cfg = config_->at(0);\n> \tstream_ = cfg.stream();\n>\n> +\tunsigned int bufferCount =\n> +\t\tcamera_->properties().get(properties::MinimumRequests).value();\n> +\n> \tallocator_ = std::make_unique(camera_);\n> -\tif (allocator_->allocate(stream_) < 0)\n> +\tif (allocator_->allocate(stream_, bufferCount) < 0)\n> \t\treturn TestFail;\n>\n> \tnbuffers_ = allocator_->buffers(stream_).size();\n> diff --git a/test/mapped-buffer.cpp b/test/mapped-buffer.cpp\n> index b4422f7d..1b98feea 100644\n> --- a/test/mapped-buffer.cpp\n> +++ b/test/mapped-buffer.cpp\n> @@ -8,6 +8,7 @@\n> #include \n>\n> #include \n> +#include \n>\n> #include \"libcamera/internal/mapped_framebuffer.h\"\n>\n> @@ -55,7 +56,9 @@ protected:\n>\n> \t\tstream_ = cfg.stream();\n>\n> -\t\tint ret = allocator_->allocate(stream_);\n> +\t\tunsigned int bufferCount =\n> +\t\t\tcamera_->properties().get(properties::MinimumRequests).value();\n> +\t\tint ret = allocator_->allocate(stream_, bufferCount);\n> \t\tif (ret < 0)\n> \t\t\treturn TestFail;\n>\n> --\n> 2.35.1\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 9DB6FC3200\n\tfor ;\n\tFri, 16 Dec 2022 14:15:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2595E63360;\n\tFri, 16 Dec 2022 15:15:40 +0100 (CET)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::229])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1C01A603D0\n\tfor ;\n\tFri, 16 Dec 2022 15:15:38 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 3E50CFF803;\n\tFri, 16 Dec 2022 14:15:36 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1671200140;\n\tbh=3RnLKnZb7ZcSpZdYpTEu8DOluhOrH9Vs4JUyYnPe4bU=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=sNlhs/E0LXku/s/6EGOTjvgK5QcNnJHUQRNeJLme3RNy7hGw5d0hSk4kgPYzYVLzI\n\tPHGSRpVbwtMHmI2o5eTbpQY6ArfvBNg3Ig/dcSfzaR+UMduh/52tO9w9eD/kzgiFS1\n\tWON9T0pdv1TGREfE4PgFdqBA9oY1ywJzOpfOScPDi8o4ghloOpbc/pnVnishay0AoJ\n\tO9JdQFz7+vH1rTnDLxAbG6rGCbbKv4OL/HhrRT5+yAklnd8IashTgQ/1H+tF+w7Dn/\n\tREyy4ai/wUOGkvDaTSiYSdIHZoausSz5SQwnmUg7DPT+SNv9jS80zxSdZvBJS2db+9\n\tX5+CLIOw+uptA==","Date":"Fri, 16 Dec 2022 15:15:35 +0100","To":"Paul Elder ","Message-ID":"<20221216141535.ssvpmb3as55ajwy6@uno.localdomain>","References":"<20221216122939.256534-1-paul.elder@ideasonboard.com>\n\t<20221216122939.256534-4-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20221216122939.256534-4-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v9 03/18] libcamera:\n\tframebuffer_allocator: Make allocate() require count","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":26121,"web_url":"https://patchwork.libcamera.org/comment/26121/","msgid":"","date":"2022-12-21T08:10:16","subject":"Re: [libcamera-devel] [PATCH v9 03/18] libcamera:\n\tframebuffer_allocator: Make allocate() require count","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Fri, Dec 16, 2022 at 03:15:35PM +0100, Jacopo Mondi wrote:\n> Hi Paul\n> \n> On Fri, Dec 16, 2022 at 09:29:24PM +0900, Paul Elder via libcamera-devel wrote:\n> > From: Nícolas F. R. A. Prado \n> >\n> > Make FrameBufferAllocator::allocate() require a 'count' argument for the\n> > number of buffers to be allocated.\n> >\n> > Signed-off-by: Nícolas F. R. A. Prado \n> > Reviewed-by: Kieran Bingham \n> > Reviewed-by: Laurent Pinchart \n> > Reviewed-by: Paul Elder \n> > Signed-off-by: Paul Elder \n> >\n> \n> Seeing this being extensively reviewed, I won't comment on the change\n> to the public API interface. I'm a bit afraid we're putting on\n> applications the burden of getting this right, when they probably\n> don't care in most cases..\n\n(I went back through the history) It looks like it was decided in v2 to\ngo this way.\n\n\nPaul\n\n> \n> > ---\n> \n> [snip]\n> \n> > /**\n> > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp\n> > index dabd9219..6a0bb8df 100644\n> > --- a/src/libcamera/framebuffer_allocator.cpp\n> > +++ b/src/libcamera/framebuffer_allocator.cpp\n> > @@ -71,6 +71,7 @@ FrameBufferAllocator::~FrameBufferAllocator()\n> > /**\n> > * \\brief Allocate buffers for a configured stream\n> > * \\param[in] stream The stream to allocate buffers for\n> > + * \\param[in] count The number of buffers to allocate\n> > *\n> > * Allocate buffers suitable for capturing frames from the \\a stream. The Camera\n> > * shall have been previously configured with Camera::configure() and shall be\n> > @@ -79,6 +80,10 @@ FrameBufferAllocator::~FrameBufferAllocator()\n> > * Upon successful allocation, the allocated buffers can be retrieved with the\n> > * buffers() function.\n> > *\n> > + * This function may allocate less buffers than requested, due to memory and\n> > + * other system constraints. The caller shall always check the return value to\n> > + * verify if the number of allocate buffers matches its needs.\n> > + *\n> > * \\return The number of allocated buffers on success or a negative error code\n> > * otherwise\n> > * \\retval -EACCES The camera is not in a state where buffers can be allocated\n> > @@ -86,7 +91,7 @@ FrameBufferAllocator::~FrameBufferAllocator()\n> > * not part of the active camera configuration\n> > * \\retval -EBUSY Buffers are already allocated for the \\a stream\n> > */\n> > -int FrameBufferAllocator::allocate(Stream *stream)\n> > +int FrameBufferAllocator::allocate(Stream *stream, unsigned int count)\n> > {\n> > \tconst auto &[it, inserted] = buffers_.try_emplace(stream);\n> >\n> > @@ -95,7 +100,7 @@ int FrameBufferAllocator::allocate(Stream *stream)\n> > \t\treturn -EBUSY;\n> > \t}\n> >\n> > -\tint ret = camera_->exportFrameBuffers(stream, &it->second);\n> > +\tint ret = camera_->exportFrameBuffers(stream, count, &it->second);\n> \n> We have the camera, we can access properties, count could be\n> \n> count = max(properties::MinimumRequests, count)\n> \n> A detail through\n> \n> Reviewed-by: Jacopo Mondi \n> \n> \n> > \tif (ret == -EINVAL)\n> > \t\tLOG(Allocator, Error)\n> > \t\t\t<< \"Stream is not part of \" << camera_->id()\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 98a4a3e5..bab2db65 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -140,7 +140,7 @@ public:\n> > \t\tconst StreamRoles &roles) override;\n> > \tint configure(Camera *camera, CameraConfiguration *config) override;\n> >\n> > -\tint exportFrameBuffers(Camera *camera, Stream *stream,\n> > +\tint exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,\n> > \t\t\t std::vector> *buffers) override;\n> >\n> > \tint start(Camera *camera, const ControlList *controls) override;\n> > @@ -687,10 +687,10 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)\n> > }\n> >\n> > int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,\n> > +\t\t\t\t\t unsigned int count,\n> > \t\t\t\t\t std::vector> *buffers)\n> > {\n> > \tIPU3CameraData *data = cameraData(camera);\n> > -\tunsigned int count = stream->configuration().bufferCount;\n> >\n> > \tif (stream == &data->outStream_)\n> > \t\treturn data->imgu_->output_->exportBuffers(count, buffers);\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 4a08d01e..4641c76f 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -328,7 +328,7 @@ public:\n> > \t\tconst StreamRoles &roles) override;\n> > \tint configure(Camera *camera, CameraConfiguration *config) override;\n> >\n> > -\tint exportFrameBuffers(Camera *camera, Stream *stream,\n> > +\tint exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,\n> > \t\t\t std::vector> *buffers) override;\n> >\n> > \tint start(Camera *camera, const ControlList *controls) override;\n> > @@ -1005,10 +1005,10 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> > }\n> >\n> > int PipelineHandlerRPi::exportFrameBuffers([[maybe_unused]] Camera *camera, Stream *stream,\n> > +\t\t\t\t\t unsigned int count,\n> > \t\t\t\t\t std::vector> *buffers)\n> > {\n> > \tRPi::Stream *s = static_cast(stream);\n> > -\tunsigned int count = stream->configuration().bufferCount;\n> > \tint ret = s->dev()->exportBuffers(count, buffers);\n> >\n> > \ts->setExportedBuffers(buffers);\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 45b6beaf..8fe37c4f 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -148,7 +148,7 @@ public:\n> > \t\tconst StreamRoles &roles) override;\n> > \tint configure(Camera *camera, CameraConfiguration *config) override;\n> >\n> > -\tint exportFrameBuffers(Camera *camera, Stream *stream,\n> > +\tint exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,\n> > \t\t\t std::vector> *buffers) override;\n> >\n> > \tint start(Camera *camera, const ControlList *controls) override;\n> > @@ -816,10 +816,10 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)\n> > }\n> >\n> > int PipelineHandlerRkISP1::exportFrameBuffers([[maybe_unused]] Camera *camera, Stream *stream,\n> > +\t\t\t\t\t unsigned int count,\n> > \t\t\t\t\t std::vector> *buffers)\n> > {\n> > \tRkISP1CameraData *data = cameraData(camera);\n> > -\tunsigned int count = stream->configuration().bufferCount;\n> >\n> > \tif (stream == &data->mainPathStream_)\n> > \t\treturn mainPath_.exportBuffers(count, buffers);\n> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> > index 8ed983fe..6b7c6d5c 100644\n> > --- a/src/libcamera/pipeline/simple/simple.cpp\n> > +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > @@ -322,7 +322,7 @@ public:\n> > \t\tconst StreamRoles &roles) override;\n> > \tint configure(Camera *camera, CameraConfiguration *config) override;\n> >\n> > -\tint exportFrameBuffers(Camera *camera, Stream *stream,\n> > +\tint exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,\n> > \t\t\t std::vector> *buffers) override;\n> >\n> > \tint start(Camera *camera, const ControlList *controls) override;\n> > @@ -1202,10 +1202,10 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)\n> > }\n> >\n> > int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream,\n> > +\t\t\t\t\t unsigned int count,\n> > \t\t\t\t\t std::vector> *buffers)\n> > {\n> > \tSimpleCameraData *data = cameraData(camera);\n> > -\tunsigned int count = stream->configuration().bufferCount;\n> >\n> > \t/*\n> > \t * Export buffers on the converter or capture video node, depending on\n> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > index 7f580955..4ce240a4 100644\n> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > @@ -78,7 +78,7 @@ public:\n> > \t\tconst StreamRoles &roles) override;\n> > \tint configure(Camera *camera, CameraConfiguration *config) override;\n> >\n> > -\tint exportFrameBuffers(Camera *camera, Stream *stream,\n> > +\tint exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,\n> > \t\t\t std::vector> *buffers) override;\n> >\n> > \tint start(Camera *camera, const ControlList *controls) override;\n> > @@ -226,11 +226,12 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config)\n> > \treturn 0;\n> > }\n> >\n> > -int PipelineHandlerUVC::exportFrameBuffers(Camera *camera, Stream *stream,\n> > +int PipelineHandlerUVC::exportFrameBuffers(Camera *camera,\n> > +\t\t\t\t\t [[maybe_unused]] Stream *stream,\n> > +\t\t\t\t\t unsigned int count,\n> > \t\t\t\t\t std::vector> *buffers)\n> > {\n> > \tUVCCameraData *data = cameraData(camera);\n> > -\tunsigned int count = stream->configuration().bufferCount;\n> >\n> > \treturn data->video_->exportBuffers(count, buffers);\n> > }\n> > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> > index d2633be4..e58caf99 100644\n> > --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> > +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> > @@ -89,7 +89,7 @@ public:\n> > \t\tconst StreamRoles &roles) override;\n> > \tint configure(Camera *camera, CameraConfiguration *config) override;\n> >\n> > -\tint exportFrameBuffers(Camera *camera, Stream *stream,\n> > +\tint exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,\n> > \t\t\t std::vector> *buffers) override;\n> >\n> > \tint start(Camera *camera, const ControlList *controls) override;\n> > @@ -321,11 +321,12 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)\n> > \treturn 0;\n> > }\n> >\n> > -int PipelineHandlerVimc::exportFrameBuffers(Camera *camera, Stream *stream,\n> > +int PipelineHandlerVimc::exportFrameBuffers(Camera *camera,\n> > +\t\t\t\t\t [[maybe_unused]] Stream *stream,\n> > +\t\t\t\t\t unsigned int count,\n> > \t\t\t\t\t std::vector> *buffers)\n> > {\n> > \tVimcCameraData *data = cameraData(camera);\n> > -\tunsigned int count = stream->configuration().bufferCount;\n> >\n> > \treturn data->video_->exportBuffers(count, buffers);\n> > }\n> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> > index cfade490..fffbd51b 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -280,6 +280,7 @@ void PipelineHandler::unlockMediaDevices()\n> > * \\brief Allocate and export buffers for \\a stream\n> > * \\param[in] camera The camera\n> > * \\param[in] stream The stream to allocate buffers for\n> > + * \\param[in] count The number of buffers to allocate\n> > * \\param[out] buffers Array of buffers successfully allocated\n> > *\n> > * This function allocates buffers for the \\a stream from the devices associated\n> > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp\n> > index 7b97c2d5..1c5fab64 100644\n> > --- a/src/v4l2/v4l2_camera.cpp\n> > +++ b/src/v4l2/v4l2_camera.cpp\n> > @@ -160,7 +160,7 @@ int V4L2Camera::allocBuffers(unsigned int count)\n> > {\n> > \tStream *stream = config_->at(0).stream();\n> >\n> > -\tint ret = bufferAllocator_->allocate(stream);\n> > +\tint ret = bufferAllocator_->allocate(stream, count);\n> > \tif (ret < 0)\n> > \t\treturn ret;\n> >\n> > diff --git a/test/camera/camera_reconfigure.cpp b/test/camera/camera_reconfigure.cpp\n> > index 06c87730..63a97863 100644\n> > --- a/test/camera/camera_reconfigure.cpp\n> > +++ b/test/camera/camera_reconfigure.cpp\n> > @@ -17,6 +17,7 @@\n> > #include \n> >\n> > #include \n> > +#include \n> >\n> > #include \"camera_test.h\"\n> > #include \"test.h\"\n> > @@ -78,7 +79,9 @@ private:\n> > \t\t * same buffer allocation for each run.\n> > \t\t */\n> > \t\tif (!allocated_) {\n> > -\t\t\tint ret = allocator_->allocate(stream);\n> > +\t\t\tunsigned int bufferCount =\n> > +\t\t\t\tcamera_->properties().get(properties::MinimumRequests).value();\n> > +\t\t\tint ret = allocator_->allocate(stream, bufferCount);\n> > \t\t\tif (ret < 0) {\n> > \t\t\t\tcerr << \"Failed to allocate buffers\" << endl;\n> > \t\t\t\treturn TestFail;\n> > diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp\n> > index de824083..64d3a8e7 100644\n> > --- a/test/camera/capture.cpp\n> > +++ b/test/camera/capture.cpp\n> > @@ -7,12 +7,13 @@\n> >\n> > #include \n> >\n> > -#include \n> > -\n> > #include \n> > #include \n> > #include \n> >\n> > +#include \n> > +#include \n> > +\n> > #include \"camera_test.h\"\n> > #include \"test.h\"\n> >\n> > @@ -98,8 +99,10 @@ protected:\n> >\n> > \t\tStream *stream = cfg.stream();\n> >\n> > -\t\tint ret = allocator_->allocate(stream);\n> > -\t\tif (ret < 0)\n> > +\t\tunsigned int bufferCount =\n> > +\t\t\tcamera_->properties().get(properties::MinimumRequests).value();\n> > +\t\tint ret = allocator_->allocate(stream, bufferCount);\n> > +\t\tif (ret < static_cast(bufferCount))\n> > \t\t\treturn TestFail;\n> >\n> > \t\tfor (const std::unique_ptr &buffer : allocator_->buffers(stream)) {\n> > diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp\n> > index 9c2b0c6a..a9ddb323 100644\n> > --- a/test/camera/statemachine.cpp\n> > +++ b/test/camera/statemachine.cpp\n> > @@ -8,6 +8,7 @@\n> > #include \n> >\n> > #include \n> > +#include \n> >\n> > #include \"camera_test.h\"\n> > #include \"test.h\"\n> > @@ -120,7 +121,9 @@ protected:\n> > \t\t/* Use internally allocated buffers. */\n> > \t\tallocator_ = new FrameBufferAllocator(camera_);\n> > \t\tStream *stream = *camera_->streams().begin();\n> > -\t\tif (allocator_->allocate(stream) < 0)\n> > +\t\tunsigned int bufferCount =\n> > +\t\t\tcamera_->properties().get(properties::MinimumRequests).value();\n> > +\t\tif (allocator_->allocate(stream, bufferCount) < 0)\n> > \t\t\treturn TestFail;\n> >\n> > \t\tif (camera_->start())\n> > diff --git a/test/fence.cpp b/test/fence.cpp\n> > index 1e38bc2f..88ce2857 100644\n> > --- a/test/fence.cpp\n> > +++ b/test/fence.cpp\n> > @@ -18,6 +18,7 @@\n> >\n> > #include \n> > #include \n> > +#include \n> >\n> > #include \"camera_test.h\"\n> > #include \"test.h\"\n> > @@ -117,8 +118,11 @@ int FenceTest::init()\n> > \tStreamConfiguration &cfg = config_->at(0);\n> > \tstream_ = cfg.stream();\n> >\n> > +\tunsigned int bufferCount =\n> > +\t\tcamera_->properties().get(properties::MinimumRequests).value();\n> > +\n> > \tallocator_ = std::make_unique(camera_);\n> > -\tif (allocator_->allocate(stream_) < 0)\n> > +\tif (allocator_->allocate(stream_, bufferCount) < 0)\n> > \t\treturn TestFail;\n> >\n> > \tnbuffers_ = allocator_->buffers(stream_).size();\n> > diff --git a/test/mapped-buffer.cpp b/test/mapped-buffer.cpp\n> > index b4422f7d..1b98feea 100644\n> > --- a/test/mapped-buffer.cpp\n> > +++ b/test/mapped-buffer.cpp\n> > @@ -8,6 +8,7 @@\n> > #include \n> >\n> > #include \n> > +#include \n> >\n> > #include \"libcamera/internal/mapped_framebuffer.h\"\n> >\n> > @@ -55,7 +56,9 @@ protected:\n> >\n> > \t\tstream_ = cfg.stream();\n> >\n> > -\t\tint ret = allocator_->allocate(stream_);\n> > +\t\tunsigned int bufferCount =\n> > +\t\t\tcamera_->properties().get(properties::MinimumRequests).value();\n> > +\t\tint ret = allocator_->allocate(stream_, bufferCount);\n> > \t\tif (ret < 0)\n> > \t\t\treturn TestFail;\n> >\n> > --\n> > 2.35.1\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 0AF2EC3200\n\tfor ;\n\tWed, 21 Dec 2022 08:10:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7C8EE633A7;\n\tWed, 21 Dec 2022 09:10:26 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 74B5C61F1A\n\tfor ;\n\tWed, 21 Dec 2022 09:10:24 +0100 (CET)","from pyrite.rasen.tech (h175-177-042-159.catv02.itscom.jp\n\t[175.177.42.159])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E6488FB;\n\tWed, 21 Dec 2022 09:10:22 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1671610226;\n\tbh=cf+Tpp+L8pY0Fh+1slbe/pZaMMUFV4ArkpR2FYYEbnw=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=XgjGlUsLskkgvbKs4Rjmz9xGQpZUT7nawIG8bK9ZD1EaGl9xrpxto0mD806RkMWRl\n\tLz5ePA0cGCnH1h3I6bOY4eJxrNS09zljlpMcj35ERUw9hUYYdg4fBuPHp1BzRRGTQ+\n\thudxkWs9y+x+cbHMh9luTGKbx7/JKcOJn3lZWvs5Lsmmplb9klNidqh7q89TGVdiWi\n\t/8KhnxYH2D8+jWn4e/mbcdz8Ll5zUGYdAeL81Azazf81LsixsaExi/mAgDCPDXUDy3\n\tvcEZ/WVGYYP1Dk6yh5x+R2xog3TnarAqQGv+f7MET3BDXSR1//5j+d7RSuTlC1Uwmq\n\ttdbiHQt5MlSdQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1671610224;\n\tbh=cf+Tpp+L8pY0Fh+1slbe/pZaMMUFV4ArkpR2FYYEbnw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lERY9/Z9jkCk1osYwqXT6gvZQVbNsxgzNElZrZFPstl330CZDWzKklz2NJYqanChq\n\tHxywaL2o1sIFb70AGDy/WiuTTe2/JYwi/YcMS3PmPWxRFitI8KvFm/04KB3iCEiGS4\n\t/IwlseZrVPv9hIdYNexkODdphy9w4e0NsFG+aW8M="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"lERY9/Z9\"; dkim-atps=neutral","Date":"Wed, 21 Dec 2022 17:10:16 +0900","To":"Jacopo Mondi ","Message-ID":"","References":"<20221216122939.256534-1-paul.elder@ideasonboard.com>\n\t<20221216122939.256534-4-paul.elder@ideasonboard.com>\n\t<20221216141535.ssvpmb3as55ajwy6@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20221216141535.ssvpmb3as55ajwy6@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v9 03/18] libcamera:\n\tframebuffer_allocator: Make allocate() require count","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Paul Elder via libcamera-devel ","Reply-To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]