[{"id":18403,"web_url":"https://patchwork.libcamera.org/comment/18403/","msgid":"<650924e2-8a93-ba95-48c8-3e8d2eebfe0b@ideasonboard.com>","date":"2021-07-28T08:28:36","subject":"Re: [libcamera-devel] [PATCH v7 02/11] libcamera:\n\tframebuffer_allocator: Make allocate() require count","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Nicolas,\n\nOn 23/07/2021 00:28, Nícolas F. R. A. Prado wrote:\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> ---\n> \n> No changes in v7\n> \n> Changes in v6:\n> - Changed static_cast to convert 'count' to unsigned int instead of\n> 'bufferCount' to int when comparing\n> \n> Changes in v5:\n> - Made sure that qcam allocates at least 2 buffers\n> \n> include/libcamera/camera.h | 2 +-\n> include/libcamera/framebuffer_allocator.h | 2 +-\n> include/libcamera/internal/pipeline_handler.h | 2 +-\n> src/android/camera_stream.cpp | 5 ++++-\n> src/cam/capture.cpp | 9 +++------\n> src/gstreamer/gstlibcameraallocator.cpp | 4 +++-\n> src/lc-compliance/simple_capture.cpp | 7 +++++--\n> src/libcamera/camera.cpp | 4 ++--\n> src/libcamera/framebuffer_allocator.cpp | 5 +++--\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 4 ++--\n> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 4 ++--\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 4 ++--\n> src/libcamera/pipeline/simple/simple.cpp | 4 ++--\n> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 7 ++++---\n> src/libcamera/pipeline/vimc/vimc.cpp | 7 ++++---\n> src/libcamera/pipeline_handler.cpp | 1 +\n> src/qcam/main_window.cpp | 11 ++++++++++-\n> src/v4l2/v4l2_camera.cpp | 2 +-\n> test/camera/capture.cpp | 4 +++-\n> test/camera/statemachine.cpp | 4 +++-\n> test/mapped-buffer.cpp | 4 +++-\n> 21 files changed, 60 insertions(+), 36 deletions(-)\n\nThis patch should also update the Documentation/application-developer\nguide which references the public API in section\n \"Using the libcamera ``FrameBufferAllocator``\"\n\nIt can be a patch on top if it helps, or squashed into this one.\n\nWith that, I think I quite like this change as it gives better\nflexibility to the application which otherwise might provide buffers\nelsewhere anyway.\n\nWith the Documentation fixed, or with that being prepared as a separate\npatch:\n\nReviewed-by: Kieran Bingham \n\n--\nKieran\n\n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index b081907e0cb1..9f1767e4c406 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -116,7 +116,7 @@ private:\n> \tvoid requestComplete(Request *request);\n> \n> \tfriend class FrameBufferAllocator;\n> -\tint exportFrameBuffers(Stream *stream,\n> +\tint exportFrameBuffers(Stream *stream, unsigned int count,\n> \t\t\t std::vector> *buffers);\n> };\n> \n> diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h\n> index cbc9ce101889..2d5a6e98e10c 100644\n> --- a/include/libcamera/framebuffer_allocator.h\n> +++ b/include/libcamera/framebuffer_allocator.h\n> @@ -25,7 +25,7 @@ public:\n> \tFrameBufferAllocator(std::shared_ptr camera);\n> \t~FrameBufferAllocator();\n> \n> -\tint allocate(Stream *stream);\n> +\tint allocate(Stream *stream, unsigned int count);\n> \tint free(Stream *stream);\n> \n> \tbool allocated() const { return !buffers_.empty(); }\n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index 9e2d65d6f2c5..0b4b2e4947c0 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -76,7 +76,7 @@ public:\n> \t\tconst StreamRoles &roles) = 0;\n> \tvirtual int configure(Camera *camera, CameraConfiguration *config) = 0;\n> \n> -\tvirtual int exportFrameBuffers(Camera *camera, Stream *stream,\n> +\tvirtual int exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,\n> \t\t\t\t std::vector> *buffers) = 0;\n> \n> \tvirtual int start(Camera *camera, const ControlList *controls) = 0;\n> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> index bf4a7b41a70a..b168e3c0c288 100644\n> --- a/src/android/camera_stream.cpp\n> +++ b/src/android/camera_stream.cpp\n> @@ -13,6 +13,7 @@\n> #include \"jpeg/post_processor_jpeg.h\"\n> \n> #include \n> +#include \n> \n> using namespace libcamera;\n> \n> @@ -81,8 +82,10 @@ int CameraStream::configure()\n> \t\t\treturn ret;\n> \t}\n> \n> +\tunsigned int bufferCount = cameraDevice_->camera()->properties().get(properties::MinimumRequests);\n> +\n> \tif (allocator_) {\n> -\t\tint ret = allocator_->allocate(stream());\n> +\t\tint ret = allocator_->allocate(stream(), bufferCount);\n> \t\tif (ret < 0)\n> \t\t\treturn ret;\n> \n> diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp\n> index 3c3e3a53adf7..633f9c6e4f25 100644\n> --- a/src/cam/capture.cpp\n> +++ b/src/cam/capture.cpp\n> @@ -11,6 +11,7 @@\n> #include \n> \n> #include \n> +#include \n> \n> #include \"capture.h\"\n> #include \"main.h\"\n> @@ -81,17 +82,13 @@ int Capture::capture(FrameBufferAllocator *allocator)\n> {\n> \tint ret;\n> \n> -\t/* Identify the stream with the least number of buffers. */\n> -\tunsigned int nbuffers = UINT_MAX;\n> +\tunsigned int nbuffers = camera_->properties().get(properties::MinimumRequests);\n> \tfor (StreamConfiguration &cfg : *config_) {\n> -\t\tret = allocator->allocate(cfg.stream());\n> +\t\tret = allocator->allocate(cfg.stream(), nbuffers);\n> \t\tif (ret < 0) {\n> \t\t\tstd::cerr << \"Can't allocate buffers\" << std::endl;\n> \t\t\treturn -ENOMEM;\n> \t\t}\n> -\n> -\t\tunsigned int allocated = allocator->buffers(cfg.stream()).size();\n> -\t\tnbuffers = std::min(nbuffers, allocated);\n> \t}\n> \n> \t/*\n> diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp\n> index 7bd8ba2db71d..8cc42edfaf61 100644\n> --- a/src/gstreamer/gstlibcameraallocator.cpp\n> +++ b/src/gstreamer/gstlibcameraallocator.cpp\n> @@ -10,6 +10,7 @@\n> \n> #include \n> #include \n> +#include \n> #include \n> \n> #include \"gstlibcamera-utils.h\"\n> @@ -188,13 +189,14 @@ gst_libcamera_allocator_new(std::shared_ptr camera,\n> {\n> \tauto *self = GST_LIBCAMERA_ALLOCATOR(g_object_new(GST_TYPE_LIBCAMERA_ALLOCATOR,\n> \t\t\t\t\t\t\t nullptr));\n> +\tunsigned int bufferCount = camera->properties().get(properties::MinimumRequests);\n> \n> \tself->fb_allocator = new FrameBufferAllocator(camera);\n> \tfor (StreamConfiguration &streamCfg : *config_) {\n> \t\tStream *stream = streamCfg.stream();\n> \t\tgint ret;\n> \n> -\t\tret = self->fb_allocator->allocate(stream);\n> +\t\tret = self->fb_allocator->allocate(stream, bufferCount);\n> \t\tif (ret == 0)\n> \t\t\treturn nullptr;\n> \n> diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp\n> index 25097f28a603..6444203547be 100644\n> --- a/src/lc-compliance/simple_capture.cpp\n> +++ b/src/lc-compliance/simple_capture.cpp\n> @@ -7,6 +7,8 @@\n> \n> #include \n> \n> +#include \n> +\n> #include \"simple_capture.h\"\n> \n> using namespace libcamera;\n> @@ -44,11 +46,12 @@ void SimpleCapture::configure(StreamRole role)\n> \n> void SimpleCapture::start()\n> {\n> +\tunsigned int bufferCount = camera_->properties().get(properties::MinNumRequests);\n> \tStream *stream = config_->at(0).stream();\n> -\tint count = allocator_->allocate(stream);\n> +\tint count = allocator_->allocate(stream, bufferCount);\n> \n> \tASSERT_GE(count, 0) << \"Failed to allocate buffers\";\n> -\tEXPECT_EQ(count, config_->at(0).bufferCount) << \"Allocated less buffers than expected\";\n> +\tEXPECT_EQ(static_cast(count), bufferCount) << \"Allocated less buffers than expected\";\n> \n> \tcamera_->requestCompleted.connect(this, &SimpleCapture::requestComplete);\n> \n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index c8858e71d105..e5a55a674afb 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -660,7 +660,7 @@ void Camera::disconnect()\n> \tdisconnected.emit(this);\n> }\n> \n> -int Camera::exportFrameBuffers(Stream *stream,\n> +int Camera::exportFrameBuffers(Stream *stream, unsigned int count,\n> \t\t\t std::vector> *buffers)\n> {\n> \tPrivate *const d = _d();\n> @@ -677,7 +677,7 @@ int Camera::exportFrameBuffers(Stream *stream,\n> \n> \treturn d->pipe_->invokeMethod(&PipelineHandler::exportFrameBuffers,\n> \t\t\t\t ConnectionTypeBlocking, this, stream,\n> -\t\t\t\t buffers);\n> +\t\t\t\t count, buffers);\n> }\n> \n> /**\n> diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp\n> index 695073fd1c1c..2fc0db56176a 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> @@ -86,14 +87,14 @@ 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> \tif (buffers_.count(stream)) {\n> \t\tLOG(Allocator, Error) << \"Buffers already allocated for stream\";\n> \t\treturn -EBUSY;\n> \t}\n> \n> -\tint ret = camera_->exportFrameBuffers(stream, &buffers_[stream]);\n> +\tint ret = camera_->exportFrameBuffers(stream, count, &buffers_[stream]);\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 76c3bb3d8aa9..5fd1757bfe13 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -134,7 +134,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> @@ -654,10 +654,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 f821d8fe1b6c..d1cd3d9dc082 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -251,7 +251,7 @@ public:\n> \tCameraConfiguration *generateConfiguration(Camera *camera, const 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> @@ -795,10 +795,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 cb4ca9a85fa8..11325875b929 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -141,7 +141,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> @@ -671,10 +671,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 348c554c8be4..1c25a7344f5f 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -228,7 +228,7 @@ public:\n> \t\t\t\t\t\t const 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> @@ -776,10 +776,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 6ad7fb3c9f0c..fd39b3d3c72c 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -69,7 +69,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> @@ -223,11 +223,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 44c198ccf8b8..e89d53182c6d 100644\n> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> @@ -84,7 +84,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> @@ -299,11 +299,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 c9928d444b04..dc83bf6924bf 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -323,6 +323,7 @@ const ControlList &PipelineHandler::properties(const Camera *camera) const\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 method allocates buffers for the \\a stream from the devices associated\n> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> index 39d034de6bb2..d7475b142d6f 100644\n> --- a/src/qcam/main_window.cpp\n> +++ b/src/qcam/main_window.cpp\n> @@ -25,6 +25,7 @@\n> #include \n> \n> #include \n> +#include \n> #include \n> \n> #include \"dng_writer.h\"\n> @@ -463,7 +464,15 @@ int MainWindow::startCapture()\n> \tfor (StreamConfiguration &config : *config_) {\n> \t\tStream *stream = config.stream();\n> \n> -\t\tret = allocator_->allocate(stream);\n> +\t\tunsigned int bufferCount = camera_->properties().get(properties::MinimumRequests);\n> +\n> +\t\t/*\n> +\t\t * Need at least two buffers, one for capture and another for\n> +\t\t * display\n> +\t\t */\n> +\t\tbufferCount = std::max(bufferCount, 2U);\n> +\n> +\t\tret = allocator_->allocate(stream, bufferCount);\n> \t\tif (ret < 0) {\n> \t\t\tqWarning() << \"Failed to allocate capture buffers\";\n> \t\t\tgoto error;\n> diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp\n> index 157ab94e0544..d01eacfa2b84 100644\n> --- a/src/v4l2/v4l2_camera.cpp\n> +++ b/src/v4l2/v4l2_camera.cpp\n> @@ -161,7 +161,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/capture.cpp b/test/camera/capture.cpp\n> index 238d98dbba16..2a531805cb2b 100644\n> --- a/test/camera/capture.cpp\n> +++ b/test/camera/capture.cpp\n> @@ -8,6 +8,7 @@\n> #include \n> \n> #include \n> +#include \n> \n> #include \n> #include \n> @@ -96,7 +97,8 @@ protected:\n> \n> \t\tStream *stream = cfg.stream();\n> \n> -\t\tint ret = allocator_->allocate(stream);\n> +\t\tunsigned int bufferCount = camera_->properties().get(properties::MinimumRequests);\n> +\t\tint ret = allocator_->allocate(stream, bufferCount);\n> \t\tif (ret < 0)\n> \t\t\treturn TestFail;\n> \n> diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp\n> index 26fb5ca17139..70015bb56c74 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> @@ -119,7 +120,8 @@ 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 = camera_->properties().get(properties::MinimumRequests);\n> +\t\tif (allocator_->allocate(stream, bufferCount) < 0)\n> \t\t\treturn TestFail;\n> \n> \t\tif (camera_->start())\n> diff --git a/test/mapped-buffer.cpp b/test/mapped-buffer.cpp\n> index c9479194cb68..e01211965364 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/framebuffer.h\"\n> \n> @@ -54,7 +55,8 @@ protected:\n> \n> \t\tstream_ = cfg.stream();\n> \n> -\t\tint ret = allocator_->allocate(stream_);\n> +\t\tunsigned int bufferCount = camera_->properties().get(properties::MinimumRequests);\n> +\t\tint ret = allocator_->allocate(stream_, bufferCount);\n> \t\tif (ret < 0)\n> \t\t\treturn TestFail;\n> \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 8AEAFC322E\n\tfor ;\n\tWed, 28 Jul 2021 08:28:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 41096687C6;\n\tWed, 28 Jul 2021 10:28:40 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B2392687B6\n\tfor ;\n\tWed, 28 Jul 2021 10:28:39 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 111603F;\n\tWed, 28 Jul 2021 10:28:39 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"p9keT20H\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627460919;\n\tbh=IlDSa5nZG+C9MSWHXJRTamfp1zc4pF5rPSeAqwKZtJE=;\n\th=To:Cc:References:From:Subject:Date:In-Reply-To:From;\n\tb=p9keT20HRV7kG/nLtM1DLFZZI/g2loJhHuKuABoaP7yMGFVI75XdjgsoKFWnKcWbb\n\tg7DcPIIZa6W5xaVcgRE08og3+oX9AHAnvmdByrgQe/VAJrhnaohe8/59vl7njuoEdJ\n\tDdsz2rm4y7lP8Gdok78ftziY3Bez1j/5BRaRTFEc=","To":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4gUHJhZG8=?= ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210722232851.747614-1-nfraprado@collabora.com>\n\t<20210722232851.747614-3-nfraprado@collabora.com>","From":"Kieran Bingham ","Message-ID":"<650924e2-8a93-ba95-48c8-3e8d2eebfe0b@ideasonboard.com>","Date":"Wed, 28 Jul 2021 09:28:36 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210722232851.747614-3-nfraprado@collabora.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v7 02/11] 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","Cc":"kernel@collabora.com, =?utf-8?q?Andr=C3=A9_Almeida?=\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18452,"web_url":"https://patchwork.libcamera.org/comment/18452/","msgid":"","date":"2021-08-01T17:23:54","subject":"Re: [libcamera-devel] [PATCH v7 02/11] libcamera:\n\tframebuffer_allocator: Make allocate() require count","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Wed, Jul 28, 2021 at 09:28:36AM +0100, Kieran Bingham wrote:\n> On 23/07/2021 00:28, Nícolas F. R. A. Prado wrote:\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> > ---\n> > \n> > No changes in v7\n> > \n> > Changes in v6:\n> > - Changed static_cast to convert 'count' to unsigned int instead of\n> > 'bufferCount' to int when comparing\n> > \n> > Changes in v5:\n> > - Made sure that qcam allocates at least 2 buffers\n> > \n> > include/libcamera/camera.h | 2 +-\n> > include/libcamera/framebuffer_allocator.h | 2 +-\n> > include/libcamera/internal/pipeline_handler.h | 2 +-\n> > src/android/camera_stream.cpp | 5 ++++-\n> > src/cam/capture.cpp | 9 +++------\n> > src/gstreamer/gstlibcameraallocator.cpp | 4 +++-\n> > src/lc-compliance/simple_capture.cpp | 7 +++++--\n> > src/libcamera/camera.cpp | 4 ++--\n> > src/libcamera/framebuffer_allocator.cpp | 5 +++--\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 ++--\n> > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 4 ++--\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 4 ++--\n> > src/libcamera/pipeline/simple/simple.cpp | 4 ++--\n> > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 7 ++++---\n> > src/libcamera/pipeline/vimc/vimc.cpp | 7 ++++---\n> > src/libcamera/pipeline_handler.cpp | 1 +\n> > src/qcam/main_window.cpp | 11 ++++++++++-\n> > src/v4l2/v4l2_camera.cpp | 2 +-\n> > test/camera/capture.cpp | 4 +++-\n> > test/camera/statemachine.cpp | 4 +++-\n> > test/mapped-buffer.cpp | 4 +++-\n> > 21 files changed, 60 insertions(+), 36 deletions(-)\n> \n> This patch should also update the Documentation/application-developer\n> guide which references the public API in section\n> \"Using the libcamera ``FrameBufferAllocator``\"\n> \n> It can be a patch on top if it helps, or squashed into this one.\n\nI'm fine with either, it may be easier to keep it on top, but it should\nbe part of this series.\n\n> With that, I think I quite like this change as it gives better\n> flexibility to the application which otherwise might provide buffers\n> elsewhere anyway.\n> \n> With the Documentation fixed, or with that being prepared as a separate\n> patch:\n> \n> Reviewed-by: Kieran Bingham \n\nReviewed-by: Laurent Pinchart \n\n> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > index b081907e0cb1..9f1767e4c406 100644\n> > --- a/include/libcamera/camera.h\n> > +++ b/include/libcamera/camera.h\n> > @@ -116,7 +116,7 @@ private:\n> > \tvoid requestComplete(Request *request);\n> > \n> > \tfriend class FrameBufferAllocator;\n> > -\tint exportFrameBuffers(Stream *stream,\n> > +\tint exportFrameBuffers(Stream *stream, unsigned int count,\n> > \t\t\t std::vector> *buffers);\n> > };\n> > \n> > diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h\n> > index cbc9ce101889..2d5a6e98e10c 100644\n> > --- a/include/libcamera/framebuffer_allocator.h\n> > +++ b/include/libcamera/framebuffer_allocator.h\n> > @@ -25,7 +25,7 @@ public:\n> > \tFrameBufferAllocator(std::shared_ptr camera);\n> > \t~FrameBufferAllocator();\n> > \n> > -\tint allocate(Stream *stream);\n> > +\tint allocate(Stream *stream, unsigned int count);\n> > \tint free(Stream *stream);\n> > \n> > \tbool allocated() const { return !buffers_.empty(); }\n> > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > index 9e2d65d6f2c5..0b4b2e4947c0 100644\n> > --- a/include/libcamera/internal/pipeline_handler.h\n> > +++ b/include/libcamera/internal/pipeline_handler.h\n> > @@ -76,7 +76,7 @@ public:\n> > \t\tconst StreamRoles &roles) = 0;\n> > \tvirtual int configure(Camera *camera, CameraConfiguration *config) = 0;\n> > \n> > -\tvirtual int exportFrameBuffers(Camera *camera, Stream *stream,\n> > +\tvirtual int exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,\n> > \t\t\t\t std::vector> *buffers) = 0;\n> > \n> > \tvirtual int start(Camera *camera, const ControlList *controls) = 0;\n> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> > index bf4a7b41a70a..b168e3c0c288 100644\n> > --- a/src/android/camera_stream.cpp\n> > +++ b/src/android/camera_stream.cpp\n> > @@ -13,6 +13,7 @@\n> > #include \"jpeg/post_processor_jpeg.h\"\n> > \n> > #include \n> > +#include \n> > \n> > using namespace libcamera;\n> > \n> > @@ -81,8 +82,10 @@ int CameraStream::configure()\n> > \t\t\treturn ret;\n> > \t}\n> > \n> > +\tunsigned int bufferCount = cameraDevice_->camera()->properties().get(properties::MinimumRequests);\n> > +\n> > \tif (allocator_) {\n> > -\t\tint ret = allocator_->allocate(stream());\n> > +\t\tint ret = allocator_->allocate(stream(), bufferCount);\n> > \t\tif (ret < 0)\n> > \t\t\treturn ret;\n> > \n> > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp\n> > index 3c3e3a53adf7..633f9c6e4f25 100644\n> > --- a/src/cam/capture.cpp\n> > +++ b/src/cam/capture.cpp\n> > @@ -11,6 +11,7 @@\n> > #include \n> > \n> > #include \n> > +#include \n> > \n> > #include \"capture.h\"\n> > #include \"main.h\"\n> > @@ -81,17 +82,13 @@ int Capture::capture(FrameBufferAllocator *allocator)\n> > {\n> > \tint ret;\n> > \n> > -\t/* Identify the stream with the least number of buffers. */\n> > -\tunsigned int nbuffers = UINT_MAX;\n> > +\tunsigned int nbuffers = camera_->properties().get(properties::MinimumRequests);\n> > \tfor (StreamConfiguration &cfg : *config_) {\n> > -\t\tret = allocator->allocate(cfg.stream());\n> > +\t\tret = allocator->allocate(cfg.stream(), nbuffers);\n\nSeems that MinimumRequests should be a per-stream property (which we\ndon't support yet). Could you add a\n\n\t\\todo Should this be a per-stream property?\n\nto patch 01/11 ?\n\n> > \t\tif (ret < 0) {\n> > \t\t\tstd::cerr << \"Can't allocate buffers\" << std::endl;\n> > \t\t\treturn -ENOMEM;\n> > \t\t}\n> > -\n> > -\t\tunsigned int allocated = allocator->buffers(cfg.stream()).size();\n> > -\t\tnbuffers = std::min(nbuffers, allocated);\n\nGiven that the allocator could still return less buffers than requested,\nshouldn't we keep this code ? Or are you relying on the (undocumented)\nassumption that the allocator will always be able to allocate at least\nMinimumRequests ?\n\n> > \t}\n> > \n> > \t/*\n> > diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp\n> > index 7bd8ba2db71d..8cc42edfaf61 100644\n> > --- a/src/gstreamer/gstlibcameraallocator.cpp\n> > +++ b/src/gstreamer/gstlibcameraallocator.cpp\n> > @@ -10,6 +10,7 @@\n> > \n> > #include \n> > #include \n> > +#include \n> > #include \n> > \n> > #include \"gstlibcamera-utils.h\"\n> > @@ -188,13 +189,14 @@ gst_libcamera_allocator_new(std::shared_ptr camera,\n> > {\n> > \tauto *self = GST_LIBCAMERA_ALLOCATOR(g_object_new(GST_TYPE_LIBCAMERA_ALLOCATOR,\n> > \t\t\t\t\t\t\t nullptr));\n> > +\tunsigned int bufferCount = camera->properties().get(properties::MinimumRequests);\n> > \n> > \tself->fb_allocator = new FrameBufferAllocator(camera);\n> > \tfor (StreamConfiguration &streamCfg : *config_) {\n> > \t\tStream *stream = streamCfg.stream();\n> > \t\tgint ret;\n> > \n> > -\t\tret = self->fb_allocator->allocate(stream);\n> > +\t\tret = self->fb_allocator->allocate(stream, bufferCount);\n> > \t\tif (ret == 0)\n> > \t\t\treturn nullptr;\n> > \n> > diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp\n> > index 25097f28a603..6444203547be 100644\n> > --- a/src/lc-compliance/simple_capture.cpp\n> > +++ b/src/lc-compliance/simple_capture.cpp\n> > @@ -7,6 +7,8 @@\n> > \n> > #include \n> > \n> > +#include \n> > +\n> > #include \"simple_capture.h\"\n> > \n> > using namespace libcamera;\n> > @@ -44,11 +46,12 @@ void SimpleCapture::configure(StreamRole role)\n> > \n> > void SimpleCapture::start()\n> > {\n> > +\tunsigned int bufferCount = camera_->properties().get(properties::MinNumRequests);\n> > \tStream *stream = config_->at(0).stream();\n> > -\tint count = allocator_->allocate(stream);\n> > +\tint count = allocator_->allocate(stream, bufferCount);\n> > \n> > \tASSERT_GE(count, 0) << \"Failed to allocate buffers\";\n> > -\tEXPECT_EQ(count, config_->at(0).bufferCount) << \"Allocated less buffers than expected\";\n> > +\tEXPECT_EQ(static_cast(count), bufferCount) << \"Allocated less buffers than expected\";\n\nIs the cast required ? bufferCount is currently an unsigned int.\n\n> > \n> > \tcamera_->requestCompleted.connect(this, &SimpleCapture::requestComplete);\n> > \n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index c8858e71d105..e5a55a674afb 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -660,7 +660,7 @@ void Camera::disconnect()\n> > \tdisconnected.emit(this);\n> > }\n> > \n> > -int Camera::exportFrameBuffers(Stream *stream,\n> > +int Camera::exportFrameBuffers(Stream *stream, unsigned int count,\n> > \t\t\t std::vector> *buffers)\n> > {\n> > \tPrivate *const d = _d();\n> > @@ -677,7 +677,7 @@ int Camera::exportFrameBuffers(Stream *stream,\n> > \n> > \treturn d->pipe_->invokeMethod(&PipelineHandler::exportFrameBuffers,\n> > \t\t\t\t ConnectionTypeBlocking, this, stream,\n> > -\t\t\t\t buffers);\n> > +\t\t\t\t count, buffers);\n> > }\n> > \n> > /**\n> > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp\n> > index 695073fd1c1c..2fc0db56176a 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\nCould you add a sentence to indicate that the function can allocate less\nbuffers than requested ?\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> > @@ -86,14 +87,14 @@ 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> > \tif (buffers_.count(stream)) {\n> > \t\tLOG(Allocator, Error) << \"Buffers already allocated for stream\";\n> > \t\treturn -EBUSY;\n> > \t}\n> > \n> > -\tint ret = camera_->exportFrameBuffers(stream, &buffers_[stream]);\n> > +\tint ret = camera_->exportFrameBuffers(stream, count, &buffers_[stream]);\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 76c3bb3d8aa9..5fd1757bfe13 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -134,7 +134,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> > @@ -654,10 +654,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 f821d8fe1b6c..d1cd3d9dc082 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -251,7 +251,7 @@ public:\n> > \tCameraConfiguration *generateConfiguration(Camera *camera, const 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> > @@ -795,10 +795,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 cb4ca9a85fa8..11325875b929 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -141,7 +141,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> > @@ -671,10 +671,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 348c554c8be4..1c25a7344f5f 100644\n> > --- a/src/libcamera/pipeline/simple/simple.cpp\n> > +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > @@ -228,7 +228,7 @@ public:\n> > \t\t\t\t\t\t const 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> > @@ -776,10 +776,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 6ad7fb3c9f0c..fd39b3d3c72c 100644\n> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > @@ -69,7 +69,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> > @@ -223,11 +223,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 44c198ccf8b8..e89d53182c6d 100644\n> > --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> > +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> > @@ -84,7 +84,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> > @@ -299,11 +299,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 c9928d444b04..dc83bf6924bf 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -323,6 +323,7 @@ const ControlList &PipelineHandler::properties(const Camera *camera) const\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 method allocates buffers for the \\a stream from the devices associated\n> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> > index 39d034de6bb2..d7475b142d6f 100644\n> > --- a/src/qcam/main_window.cpp\n> > +++ b/src/qcam/main_window.cpp\n> > @@ -25,6 +25,7 @@\n> > #include \n> > \n> > #include \n> > +#include \n> > #include \n> > \n> > #include \"dng_writer.h\"\n> > @@ -463,7 +464,15 @@ int MainWindow::startCapture()\n> > \tfor (StreamConfiguration &config : *config_) {\n> > \t\tStream *stream = config.stream();\n> > \n> > -\t\tret = allocator_->allocate(stream);\n> > +\t\tunsigned int bufferCount = camera_->properties().get(properties::MinimumRequests);\n> > +\n> > +\t\t/*\n> > +\t\t * Need at least two buffers, one for capture and another for\n> > +\t\t * display\n\ns/display/display./\n\n> > +\t\t */\n> > +\t\tbufferCount = std::max(bufferCount, 2U);\n\nThis requires inclusion of .\n\nI think we should use bufferCount + 1, not max(bufferCount, 2). qcam\nholds on one buffer for display. If the camera needs, let's say, 2\nbuffers to be able to capture, it will return a first buffer which will\nthen be queued to the display. At that point, the camera will have a\nsingle buffer, and will hold onto it until a new buffer is queued. This\nwill never happen, as the buffer being displayed won't be returned to\nlibcamera until a new buffer is ready. We thus need three buffers in\nthat case.\n\nThinking even more about this, I think we need\n\n\t\tbufferCount = std::max(bufferCount + 1, 3U);\n\nEven if the camera needs a single buffer only (in which case we would\nuse two buffers, one for capture and one for display), it's clear that\nit will lead to frame drops. A minimum of 3 buffers is thus better.\n\nThe cam application should also have a similar logic. It doesn't hold\nonto a buffer, so in that case \n\n\t\tbufferCount = std::max(bufferCount, 2U);\n\nshould be good enough to start with.\n\n> > +\n> > +\t\tret = allocator_->allocate(stream, bufferCount);\n> > \t\tif (ret < 0) {\n> > \t\t\tqWarning() << \"Failed to allocate capture buffers\";\n> > \t\t\tgoto error;\n> > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp\n> > index 157ab94e0544..d01eacfa2b84 100644\n> > --- a/src/v4l2/v4l2_camera.cpp\n> > +++ b/src/v4l2/v4l2_camera.cpp\n> > @@ -161,7 +161,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/capture.cpp b/test/camera/capture.cpp\n> > index 238d98dbba16..2a531805cb2b 100644\n> > --- a/test/camera/capture.cpp\n> > +++ b/test/camera/capture.cpp\n> > @@ -8,6 +8,7 @@\n> > #include \n> > \n> > #include \n> > +#include \n> > \n> > #include \n> > #include \n> > @@ -96,7 +97,8 @@ protected:\n> > \n> > \t\tStream *stream = cfg.stream();\n> > \n> > -\t\tint ret = allocator_->allocate(stream);\n> > +\t\tunsigned int bufferCount = camera_->properties().get(properties::MinimumRequests);\n> > +\t\tint ret = allocator_->allocate(stream, bufferCount);\n\nSimilarly, this will result in frame drop at least with some pipeline\nhandlers. I fear we need to go one step further and already design the\nAPI to tell applications how many buffers to allocate to avoid frame\ndraps. Per-frame control can be left out for now.\n\n> > \t\tif (ret < 0)\n> > \t\t\treturn TestFail;\n> > \n> > diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp\n> > index 26fb5ca17139..70015bb56c74 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> > @@ -119,7 +120,8 @@ 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 = camera_->properties().get(properties::MinimumRequests);\n> > +\t\tif (allocator_->allocate(stream, bufferCount) < 0)\n> > \t\t\treturn TestFail;\n> > \n> > \t\tif (camera_->start())\n> > diff --git a/test/mapped-buffer.cpp b/test/mapped-buffer.cpp\n> > index c9479194cb68..e01211965364 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/framebuffer.h\"\n> > \n> > @@ -54,7 +55,8 @@ protected:\n> > \n> > \t\tstream_ = cfg.stream();\n> > \n> > -\t\tint ret = allocator_->allocate(stream_);\n> > +\t\tunsigned int bufferCount = camera_->properties().get(properties::MinimumRequests);\n> > +\t\tint ret = allocator_->allocate(stream_, bufferCount);\n> > \t\tif (ret < 0)\n> > \t\t\treturn TestFail;\n> > \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 ADFDEC3232\n\tfor ;\n\tSun, 1 Aug 2021 17:24:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2E84A687C3;\n\tSun, 1 Aug 2021 19:24:07 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5DDF4687B1\n\tfor ;\n\tSun, 1 Aug 2021 19:24:04 +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 B1EBF87C;\n\tSun, 1 Aug 2021 19:24:03 +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=\"t+9U69tV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627838643;\n\tbh=raSmsr94pW2YbFLExvlIYQP+fJd2JN7VKPpfJKApB9c=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=t+9U69tV9Gi10fjeS1TKYSBJrf9ZkVCH1zinOAbmQ0IW77gRVzcaCibc0UxZEZg2n\n\tRg4IOX+mo4EOOPwVM/Dh7ukcPpKp3Afe+xVDACIvxWWQf4RIKMAwTRn+34FD8aWLe9\n\tp9+sIZGtij9eI4B4GXvol+0Ja9Qn+zlGBhhawKqQ=","Date":"Sun, 1 Aug 2021 20:23:54 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210722232851.747614-1-nfraprado@collabora.com>\n\t<20210722232851.747614-3-nfraprado@collabora.com>\n\t<650924e2-8a93-ba95-48c8-3e8d2eebfe0b@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<650924e2-8a93-ba95-48c8-3e8d2eebfe0b@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v7 02/11] 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","Cc":"libcamera-devel@lists.libcamera.org, kernel@collabora.com, =?utf-8?q?A?=\n\t=?utf-8?b?bmRyw6k=?= Almeida ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18456,"web_url":"https://patchwork.libcamera.org/comment/18456/","msgid":"","date":"2021-08-01T21:09:49","subject":"Re: [libcamera-devel] [PATCH v7 02/11] libcamera:\n\tframebuffer_allocator: Make allocate() require count","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Another comment:\n\nOn Sun, Aug 01, 2021 at 08:23:55PM +0300, Laurent Pinchart wrote:\n> On Wed, Jul 28, 2021 at 09:28:36AM +0100, Kieran Bingham wrote:\n> > On 23/07/2021 00:28, Nícolas F. R. A. Prado wrote:\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> > > ---\n> > > \n> > > No changes in v7\n> > > \n> > > Changes in v6:\n> > > - Changed static_cast to convert 'count' to unsigned int instead of\n> > > 'bufferCount' to int when comparing\n> > > \n> > > Changes in v5:\n> > > - Made sure that qcam allocates at least 2 buffers\n> > > \n> > > include/libcamera/camera.h | 2 +-\n> > > include/libcamera/framebuffer_allocator.h | 2 +-\n> > > include/libcamera/internal/pipeline_handler.h | 2 +-\n> > > src/android/camera_stream.cpp | 5 ++++-\n> > > src/cam/capture.cpp | 9 +++------\n> > > src/gstreamer/gstlibcameraallocator.cpp | 4 +++-\n> > > src/lc-compliance/simple_capture.cpp | 7 +++++--\n> > > src/libcamera/camera.cpp | 4 ++--\n> > > src/libcamera/framebuffer_allocator.cpp | 5 +++--\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 ++--\n> > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 4 ++--\n> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 4 ++--\n> > > src/libcamera/pipeline/simple/simple.cpp | 4 ++--\n> > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 7 ++++---\n> > > src/libcamera/pipeline/vimc/vimc.cpp | 7 ++++---\n> > > src/libcamera/pipeline_handler.cpp | 1 +\n> > > src/qcam/main_window.cpp | 11 ++++++++++-\n> > > src/v4l2/v4l2_camera.cpp | 2 +-\n> > > test/camera/capture.cpp | 4 +++-\n> > > test/camera/statemachine.cpp | 4 +++-\n> > > test/mapped-buffer.cpp | 4 +++-\n> > > 21 files changed, 60 insertions(+), 36 deletions(-)\n> > \n> > This patch should also update the Documentation/application-developer\n> > guide which references the public API in section\n> > \"Using the libcamera ``FrameBufferAllocator``\"\n> > \n> > It can be a patch on top if it helps, or squashed into this one.\n> \n> I'm fine with either, it may be easier to keep it on top, but it should\n> be part of this series.\n> \n> > With that, I think I quite like this change as it gives better\n> > flexibility to the application which otherwise might provide buffers\n> > elsewhere anyway.\n> > \n> > With the Documentation fixed, or with that being prepared as a separate\n> > patch:\n> > \n> > Reviewed-by: Kieran Bingham \n> \n> Reviewed-by: Laurent Pinchart \n> \n> > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > > index b081907e0cb1..9f1767e4c406 100644\n> > > --- a/include/libcamera/camera.h\n> > > +++ b/include/libcamera/camera.h\n> > > @@ -116,7 +116,7 @@ private:\n> > > \tvoid requestComplete(Request *request);\n> > > \n> > > \tfriend class FrameBufferAllocator;\n> > > -\tint exportFrameBuffers(Stream *stream,\n> > > +\tint exportFrameBuffers(Stream *stream, unsigned int count,\n> > > \t\t\t std::vector> *buffers);\n> > > };\n> > > \n> > > diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h\n> > > index cbc9ce101889..2d5a6e98e10c 100644\n> > > --- a/include/libcamera/framebuffer_allocator.h\n> > > +++ b/include/libcamera/framebuffer_allocator.h\n> > > @@ -25,7 +25,7 @@ public:\n> > > \tFrameBufferAllocator(std::shared_ptr camera);\n> > > \t~FrameBufferAllocator();\n> > > \n> > > -\tint allocate(Stream *stream);\n> > > +\tint allocate(Stream *stream, unsigned int count);\n> > > \tint free(Stream *stream);\n> > > \n> > > \tbool allocated() const { return !buffers_.empty(); }\n> > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > > index 9e2d65d6f2c5..0b4b2e4947c0 100644\n> > > --- a/include/libcamera/internal/pipeline_handler.h\n> > > +++ b/include/libcamera/internal/pipeline_handler.h\n> > > @@ -76,7 +76,7 @@ public:\n> > > \t\tconst StreamRoles &roles) = 0;\n> > > \tvirtual int configure(Camera *camera, CameraConfiguration *config) = 0;\n> > > \n> > > -\tvirtual int exportFrameBuffers(Camera *camera, Stream *stream,\n> > > +\tvirtual int exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,\n> > > \t\t\t\t std::vector> *buffers) = 0;\n> > > \n> > > \tvirtual int start(Camera *camera, const ControlList *controls) = 0;\n> > > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> > > index bf4a7b41a70a..b168e3c0c288 100644\n> > > --- a/src/android/camera_stream.cpp\n> > > +++ b/src/android/camera_stream.cpp\n> > > @@ -13,6 +13,7 @@\n> > > #include \"jpeg/post_processor_jpeg.h\"\n> > > \n> > > #include \n> > > +#include \n> > > \n> > > using namespace libcamera;\n> > > \n> > > @@ -81,8 +82,10 @@ int CameraStream::configure()\n> > > \t\t\treturn ret;\n> > > \t}\n> > > \n> > > +\tunsigned int bufferCount = cameraDevice_->camera()->properties().get(properties::MinimumRequests);\n> > > +\n> > > \tif (allocator_) {\n> > > -\t\tint ret = allocator_->allocate(stream());\n> > > +\t\tint ret = allocator_->allocate(stream(), bufferCount);\n> > > \t\tif (ret < 0)\n> > > \t\t\treturn ret;\n> > > \n> > > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp\n> > > index 3c3e3a53adf7..633f9c6e4f25 100644\n> > > --- a/src/cam/capture.cpp\n> > > +++ b/src/cam/capture.cpp\n> > > @@ -11,6 +11,7 @@\n> > > #include \n> > > \n> > > #include \n> > > +#include \n> > > \n> > > #include \"capture.h\"\n> > > #include \"main.h\"\n> > > @@ -81,17 +82,13 @@ int Capture::capture(FrameBufferAllocator *allocator)\n> > > {\n> > > \tint ret;\n> > > \n> > > -\t/* Identify the stream with the least number of buffers. */\n> > > -\tunsigned int nbuffers = UINT_MAX;\n> > > +\tunsigned int nbuffers = camera_->properties().get(properties::MinimumRequests);\n> > > \tfor (StreamConfiguration &cfg : *config_) {\n> > > -\t\tret = allocator->allocate(cfg.stream());\n> > > +\t\tret = allocator->allocate(cfg.stream(), nbuffers);\n> \n> Seems that MinimumRequests should be a per-stream property (which we\n> don't support yet). Could you add a\n> \n> \t\\todo Should this be a per-stream property?\n> \n> to patch 01/11 ?\n> \n> > > \t\tif (ret < 0) {\n> > > \t\t\tstd::cerr << \"Can't allocate buffers\" << std::endl;\n> > > \t\t\treturn -ENOMEM;\n> > > \t\t}\n> > > -\n> > > -\t\tunsigned int allocated = allocator->buffers(cfg.stream()).size();\n> > > -\t\tnbuffers = std::min(nbuffers, allocated);\n> \n> Given that the allocator could still return less buffers than requested,\n> shouldn't we keep this code ? Or are you relying on the (undocumented)\n> assumption that the allocator will always be able to allocate at least\n> MinimumRequests ?\n> \n> > > \t}\n> > > \n> > > \t/*\n> > > diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp\n> > > index 7bd8ba2db71d..8cc42edfaf61 100644\n> > > --- a/src/gstreamer/gstlibcameraallocator.cpp\n> > > +++ b/src/gstreamer/gstlibcameraallocator.cpp\n> > > @@ -10,6 +10,7 @@\n> > > \n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > > \n> > > #include \"gstlibcamera-utils.h\"\n> > > @@ -188,13 +189,14 @@ gst_libcamera_allocator_new(std::shared_ptr camera,\n> > > {\n> > > \tauto *self = GST_LIBCAMERA_ALLOCATOR(g_object_new(GST_TYPE_LIBCAMERA_ALLOCATOR,\n> > > \t\t\t\t\t\t\t nullptr));\n> > > +\tunsigned int bufferCount = camera->properties().get(properties::MinimumRequests);\n> > > \n> > > \tself->fb_allocator = new FrameBufferAllocator(camera);\n> > > \tfor (StreamConfiguration &streamCfg : *config_) {\n> > > \t\tStream *stream = streamCfg.stream();\n> > > \t\tgint ret;\n> > > \n> > > -\t\tret = self->fb_allocator->allocate(stream);\n> > > +\t\tret = self->fb_allocator->allocate(stream, bufferCount);\n> > > \t\tif (ret == 0)\n> > > \t\t\treturn nullptr;\n> > > \n> > > diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp\n> > > index 25097f28a603..6444203547be 100644\n> > > --- a/src/lc-compliance/simple_capture.cpp\n> > > +++ b/src/lc-compliance/simple_capture.cpp\n> > > @@ -7,6 +7,8 @@\n> > > \n> > > #include \n> > > \n> > > +#include \n> > > +\n> > > #include \"simple_capture.h\"\n> > > \n> > > using namespace libcamera;\n> > > @@ -44,11 +46,12 @@ void SimpleCapture::configure(StreamRole role)\n> > > \n> > > void SimpleCapture::start()\n> > > {\n> > > +\tunsigned int bufferCount = camera_->properties().get(properties::MinNumRequests);\n\nThis should be MinimumRequests.\n\nNícolas, could you compile-test patches individually ?\n\n> > > \tStream *stream = config_->at(0).stream();\n> > > -\tint count = allocator_->allocate(stream);\n> > > +\tint count = allocator_->allocate(stream, bufferCount);\n> > > \n> > > \tASSERT_GE(count, 0) << \"Failed to allocate buffers\";\n> > > -\tEXPECT_EQ(count, config_->at(0).bufferCount) << \"Allocated less buffers than expected\";\n> > > +\tEXPECT_EQ(static_cast(count), bufferCount) << \"Allocated less buffers than expected\";\n> \n> Is the cast required ? bufferCount is currently an unsigned int.\n> \n> > > \n> > > \tcamera_->requestCompleted.connect(this, &SimpleCapture::requestComplete);\n> > > \n> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > index c8858e71d105..e5a55a674afb 100644\n> > > --- a/src/libcamera/camera.cpp\n> > > +++ b/src/libcamera/camera.cpp\n> > > @@ -660,7 +660,7 @@ void Camera::disconnect()\n> > > \tdisconnected.emit(this);\n> > > }\n> > > \n> > > -int Camera::exportFrameBuffers(Stream *stream,\n> > > +int Camera::exportFrameBuffers(Stream *stream, unsigned int count,\n> > > \t\t\t std::vector> *buffers)\n> > > {\n> > > \tPrivate *const d = _d();\n> > > @@ -677,7 +677,7 @@ int Camera::exportFrameBuffers(Stream *stream,\n> > > \n> > > \treturn d->pipe_->invokeMethod(&PipelineHandler::exportFrameBuffers,\n> > > \t\t\t\t ConnectionTypeBlocking, this, stream,\n> > > -\t\t\t\t buffers);\n> > > +\t\t\t\t count, buffers);\n> > > }\n> > > \n> > > /**\n> > > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp\n> > > index 695073fd1c1c..2fc0db56176a 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> \n> Could you add a sentence to indicate that the function can allocate less\n> buffers than requested ?\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> > > @@ -86,14 +87,14 @@ 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> > > \tif (buffers_.count(stream)) {\n> > > \t\tLOG(Allocator, Error) << \"Buffers already allocated for stream\";\n> > > \t\treturn -EBUSY;\n> > > \t}\n> > > \n> > > -\tint ret = camera_->exportFrameBuffers(stream, &buffers_[stream]);\n> > > +\tint ret = camera_->exportFrameBuffers(stream, count, &buffers_[stream]);\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 76c3bb3d8aa9..5fd1757bfe13 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -134,7 +134,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> > > @@ -654,10 +654,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 f821d8fe1b6c..d1cd3d9dc082 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -251,7 +251,7 @@ public:\n> > > \tCameraConfiguration *generateConfiguration(Camera *camera, const 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> > > @@ -795,10 +795,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 cb4ca9a85fa8..11325875b929 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > @@ -141,7 +141,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> > > @@ -671,10 +671,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 348c554c8be4..1c25a7344f5f 100644\n> > > --- a/src/libcamera/pipeline/simple/simple.cpp\n> > > +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > > @@ -228,7 +228,7 @@ public:\n> > > \t\t\t\t\t\t const 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> > > @@ -776,10 +776,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 6ad7fb3c9f0c..fd39b3d3c72c 100644\n> > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > @@ -69,7 +69,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> > > @@ -223,11 +223,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 44c198ccf8b8..e89d53182c6d 100644\n> > > --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> > > +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> > > @@ -84,7 +84,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> > > @@ -299,11 +299,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 c9928d444b04..dc83bf6924bf 100644\n> > > --- a/src/libcamera/pipeline_handler.cpp\n> > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > @@ -323,6 +323,7 @@ const ControlList &PipelineHandler::properties(const Camera *camera) const\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 method allocates buffers for the \\a stream from the devices associated\n> > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> > > index 39d034de6bb2..d7475b142d6f 100644\n> > > --- a/src/qcam/main_window.cpp\n> > > +++ b/src/qcam/main_window.cpp\n> > > @@ -25,6 +25,7 @@\n> > > #include \n> > > \n> > > #include \n> > > +#include \n> > > #include \n> > > \n> > > #include \"dng_writer.h\"\n> > > @@ -463,7 +464,15 @@ int MainWindow::startCapture()\n> > > \tfor (StreamConfiguration &config : *config_) {\n> > > \t\tStream *stream = config.stream();\n> > > \n> > > -\t\tret = allocator_->allocate(stream);\n> > > +\t\tunsigned int bufferCount = camera_->properties().get(properties::MinimumRequests);\n> > > +\n> > > +\t\t/*\n> > > +\t\t * Need at least two buffers, one for capture and another for\n> > > +\t\t * display\n> \n> s/display/display./\n> \n> > > +\t\t */\n> > > +\t\tbufferCount = std::max(bufferCount, 2U);\n> \n> This requires inclusion of .\n> \n> I think we should use bufferCount + 1, not max(bufferCount, 2). qcam\n> holds on one buffer for display. If the camera needs, let's say, 2\n> buffers to be able to capture, it will return a first buffer which will\n> then be queued to the display. At that point, the camera will have a\n> single buffer, and will hold onto it until a new buffer is queued. This\n> will never happen, as the buffer being displayed won't be returned to\n> libcamera until a new buffer is ready. We thus need three buffers in\n> that case.\n> \n> Thinking even more about this, I think we need\n> \n> \t\tbufferCount = std::max(bufferCount + 1, 3U);\n> \n> Even if the camera needs a single buffer only (in which case we would\n> use two buffers, one for capture and one for display), it's clear that\n> it will lead to frame drops. A minimum of 3 buffers is thus better.\n> \n> The cam application should also have a similar logic. It doesn't hold\n> onto a buffer, so in that case \n> \n> \t\tbufferCount = std::max(bufferCount, 2U);\n> \n> should be good enough to start with.\n> \n> > > +\n> > > +\t\tret = allocator_->allocate(stream, bufferCount);\n> > > \t\tif (ret < 0) {\n> > > \t\t\tqWarning() << \"Failed to allocate capture buffers\";\n> > > \t\t\tgoto error;\n> > > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp\n> > > index 157ab94e0544..d01eacfa2b84 100644\n> > > --- a/src/v4l2/v4l2_camera.cpp\n> > > +++ b/src/v4l2/v4l2_camera.cpp\n> > > @@ -161,7 +161,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/capture.cpp b/test/camera/capture.cpp\n> > > index 238d98dbba16..2a531805cb2b 100644\n> > > --- a/test/camera/capture.cpp\n> > > +++ b/test/camera/capture.cpp\n> > > @@ -8,6 +8,7 @@\n> > > #include \n> > > \n> > > #include \n> > > +#include \n> > > \n> > > #include \n> > > #include \n> > > @@ -96,7 +97,8 @@ protected:\n> > > \n> > > \t\tStream *stream = cfg.stream();\n> > > \n> > > -\t\tint ret = allocator_->allocate(stream);\n> > > +\t\tunsigned int bufferCount = camera_->properties().get(properties::MinimumRequests);\n> > > +\t\tint ret = allocator_->allocate(stream, bufferCount);\n> \n> Similarly, this will result in frame drop at least with some pipeline\n> handlers. I fear we need to go one step further and already design the\n> API to tell applications how many buffers to allocate to avoid frame\n> draps. Per-frame control can be left out for now.\n> \n> > > \t\tif (ret < 0)\n> > > \t\t\treturn TestFail;\n> > > \n> > > diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp\n> > > index 26fb5ca17139..70015bb56c74 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> > > @@ -119,7 +120,8 @@ 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 = camera_->properties().get(properties::MinimumRequests);\n> > > +\t\tif (allocator_->allocate(stream, bufferCount) < 0)\n> > > \t\t\treturn TestFail;\n> > > \n> > > \t\tif (camera_->start())\n> > > diff --git a/test/mapped-buffer.cpp b/test/mapped-buffer.cpp\n> > > index c9479194cb68..e01211965364 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/framebuffer.h\"\n> > > \n> > > @@ -54,7 +55,8 @@ protected:\n> > > \n> > > \t\tstream_ = cfg.stream();\n> > > \n> > > -\t\tint ret = allocator_->allocate(stream_);\n> > > +\t\tunsigned int bufferCount = camera_->properties().get(properties::MinimumRequests);\n> > > +\t\tint ret = allocator_->allocate(stream_, bufferCount);\n> > > \t\tif (ret < 0)\n> > > \t\t\treturn TestFail;\n> > > \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 29300BD878\n\tfor ;\n\tSun, 1 Aug 2021 21:10:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 978F1687C3;\n\tSun, 1 Aug 2021 23:10:00 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 589AD687B6\n\tfor ;\n\tSun, 1 Aug 2021 23:09:59 +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 B422387C;\n\tSun, 1 Aug 2021 23:09:58 +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=\"SHpk84qx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627852198;\n\tbh=pYtudt3B7emLuOmjXJm8y057JnGFPpl9OJsixPoEZy4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SHpk84qxDFe+plkeRJDuwjUk+xT1J1rGPeu6vxJ9UGqBQ5c9FmJA6gvfzAAmWxMlJ\n\tMiIQwpg1Y2zLz7zYqrBOGc96dbzbg/4Fo6Jw38zLUu9MzrKTI6jJFn6gQCtW470yTu\n\tFag+EPP3NPZwNu4weRbHMinvVlmqZBzFiSQUWhZs=","Date":"Mon, 2 Aug 2021 00:09:49 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210722232851.747614-1-nfraprado@collabora.com>\n\t<20210722232851.747614-3-nfraprado@collabora.com>\n\t<650924e2-8a93-ba95-48c8-3e8d2eebfe0b@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v7 02/11] 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","Cc":"libcamera-devel@lists.libcamera.org, kernel@collabora.com, =?utf-8?q?A?=\n\t=?utf-8?b?bmRyw6k=?= Almeida ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18557,"web_url":"https://patchwork.libcamera.org/comment/18557/","msgid":"<20210804121040.xaie7xcw5pqv4syw@notapiano>","date":"2021-08-04T12:10:40","subject":"Re: [libcamera-devel] [PATCH v7 02/11] libcamera:\n\tframebuffer_allocator: Make allocate() require count","submitter":{"id":84,"url":"https://patchwork.libcamera.org/api/people/84/","name":"Nícolas F. R. A. Prado","email":"nfraprado@collabora.com"},"content":"Hi Laurent,\n\nOn Sun, Aug 01, 2021 at 08:23:54PM +0300, Laurent Pinchart wrote:\n> Hello,\n> \n> On Wed, Jul 28, 2021 at 09:28:36AM +0100, Kieran Bingham wrote:\n> > On 23/07/2021 00:28, Nícolas F. R. A. Prado wrote:\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> > > ---\n> > > \n> > > No changes in v7\n> > > \n> > > Changes in v6:\n> > > - Changed static_cast to convert 'count' to unsigned int instead of\n> > > 'bufferCount' to int when comparing\n> > > \n> > > Changes in v5:\n> > > - Made sure that qcam allocates at least 2 buffers\n> > > \n> > > include/libcamera/camera.h | 2 +-\n> > > include/libcamera/framebuffer_allocator.h | 2 +-\n> > > include/libcamera/internal/pipeline_handler.h | 2 +-\n> > > src/android/camera_stream.cpp | 5 ++++-\n> > > src/cam/capture.cpp | 9 +++------\n> > > src/gstreamer/gstlibcameraallocator.cpp | 4 +++-\n> > > src/lc-compliance/simple_capture.cpp | 7 +++++--\n> > > src/libcamera/camera.cpp | 4 ++--\n> > > src/libcamera/framebuffer_allocator.cpp | 5 +++--\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 ++--\n> > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 4 ++--\n> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 4 ++--\n> > > src/libcamera/pipeline/simple/simple.cpp | 4 ++--\n> > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 7 ++++---\n> > > src/libcamera/pipeline/vimc/vimc.cpp | 7 ++++---\n> > > src/libcamera/pipeline_handler.cpp | 1 +\n> > > src/qcam/main_window.cpp | 11 ++++++++++-\n> > > src/v4l2/v4l2_camera.cpp | 2 +-\n> > > test/camera/capture.cpp | 4 +++-\n> > > test/camera/statemachine.cpp | 4 +++-\n> > > test/mapped-buffer.cpp | 4 +++-\n> > > 21 files changed, 60 insertions(+), 36 deletions(-)\n> > \n> > This patch should also update the Documentation/application-developer\n> > guide which references the public API in section\n> > \"Using the libcamera ``FrameBufferAllocator``\"\n> > \n> > It can be a patch on top if it helps, or squashed into this one.\n> \n> I'm fine with either, it may be easier to keep it on top, but it should\n> be part of this series.\n> \n> > With that, I think I quite like this change as it gives better\n> > flexibility to the application which otherwise might provide buffers\n> > elsewhere anyway.\n> > \n> > With the Documentation fixed, or with that being prepared as a separate\n> > patch:\n> > \n> > Reviewed-by: Kieran Bingham \n> \n> Reviewed-by: Laurent Pinchart \n> \n> > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > > index b081907e0cb1..9f1767e4c406 100644\n> > > --- a/include/libcamera/camera.h\n> > > +++ b/include/libcamera/camera.h\n> > > @@ -116,7 +116,7 @@ private:\n> > > \tvoid requestComplete(Request *request);\n> > > \n> > > \tfriend class FrameBufferAllocator;\n> > > -\tint exportFrameBuffers(Stream *stream,\n> > > +\tint exportFrameBuffers(Stream *stream, unsigned int count,\n> > > \t\t\t std::vector> *buffers);\n> > > };\n> > > \n> > > diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h\n> > > index cbc9ce101889..2d5a6e98e10c 100644\n> > > --- a/include/libcamera/framebuffer_allocator.h\n> > > +++ b/include/libcamera/framebuffer_allocator.h\n> > > @@ -25,7 +25,7 @@ public:\n> > > \tFrameBufferAllocator(std::shared_ptr camera);\n> > > \t~FrameBufferAllocator();\n> > > \n> > > -\tint allocate(Stream *stream);\n> > > +\tint allocate(Stream *stream, unsigned int count);\n> > > \tint free(Stream *stream);\n> > > \n> > > \tbool allocated() const { return !buffers_.empty(); }\n> > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > > index 9e2d65d6f2c5..0b4b2e4947c0 100644\n> > > --- a/include/libcamera/internal/pipeline_handler.h\n> > > +++ b/include/libcamera/internal/pipeline_handler.h\n> > > @@ -76,7 +76,7 @@ public:\n> > > \t\tconst StreamRoles &roles) = 0;\n> > > \tvirtual int configure(Camera *camera, CameraConfiguration *config) = 0;\n> > > \n> > > -\tvirtual int exportFrameBuffers(Camera *camera, Stream *stream,\n> > > +\tvirtual int exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,\n> > > \t\t\t\t std::vector> *buffers) = 0;\n> > > \n> > > \tvirtual int start(Camera *camera, const ControlList *controls) = 0;\n> > > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> > > index bf4a7b41a70a..b168e3c0c288 100644\n> > > --- a/src/android/camera_stream.cpp\n> > > +++ b/src/android/camera_stream.cpp\n> > > @@ -13,6 +13,7 @@\n> > > #include \"jpeg/post_processor_jpeg.h\"\n> > > \n> > > #include \n> > > +#include \n> > > \n> > > using namespace libcamera;\n> > > \n> > > @@ -81,8 +82,10 @@ int CameraStream::configure()\n> > > \t\t\treturn ret;\n> > > \t}\n> > > \n> > > +\tunsigned int bufferCount = cameraDevice_->camera()->properties().get(properties::MinimumRequests);\n> > > +\n> > > \tif (allocator_) {\n> > > -\t\tint ret = allocator_->allocate(stream());\n> > > +\t\tint ret = allocator_->allocate(stream(), bufferCount);\n> > > \t\tif (ret < 0)\n> > > \t\t\treturn ret;\n> > > \n> > > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp\n> > > index 3c3e3a53adf7..633f9c6e4f25 100644\n> > > --- a/src/cam/capture.cpp\n> > > +++ b/src/cam/capture.cpp\n> > > @@ -11,6 +11,7 @@\n> > > #include \n> > > \n> > > #include \n> > > +#include \n> > > \n> > > #include \"capture.h\"\n> > > #include \"main.h\"\n> > > @@ -81,17 +82,13 @@ int Capture::capture(FrameBufferAllocator *allocator)\n> > > {\n> > > \tint ret;\n> > > \n> > > -\t/* Identify the stream with the least number of buffers. */\n> > > -\tunsigned int nbuffers = UINT_MAX;\n> > > +\tunsigned int nbuffers = camera_->properties().get(properties::MinimumRequests);\n> > > \tfor (StreamConfiguration &cfg : *config_) {\n> > > -\t\tret = allocator->allocate(cfg.stream());\n> > > +\t\tret = allocator->allocate(cfg.stream(), nbuffers);\n> \n> Seems that MinimumRequests should be a per-stream property (which we\n> don't support yet). Could you add a\n> \n> \t\\todo Should this be a per-stream property?\n> \n> to patch 01/11 ?\n> \n> > > \t\tif (ret < 0) {\n> > > \t\t\tstd::cerr << \"Can't allocate buffers\" << std::endl;\n> > > \t\t\treturn -ENOMEM;\n> > > \t\t}\n> > > -\n> > > -\t\tunsigned int allocated = allocator->buffers(cfg.stream()).size();\n> > > -\t\tnbuffers = std::min(nbuffers, allocated);\n> \n> Given that the allocator could still return less buffers than requested,\n> shouldn't we keep this code ? Or are you relying on the (undocumented)\n> assumption that the allocator will always be able to allocate at least\n> MinimumRequests ?\n\nIndeed I was since that's the current behavior, but yes, let's not assume that.\nInstead of just keeping the current code, however, I'll also add check if\n'allocated' is less than MinimumRequests, and bail out in that case, since the\npipeline won't work in that case anyway.\n\n> \n> > > \t}\n> > > \n> > > \t/*\n> > > diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp\n> > > index 7bd8ba2db71d..8cc42edfaf61 100644\n> > > --- a/src/gstreamer/gstlibcameraallocator.cpp\n> > > +++ b/src/gstreamer/gstlibcameraallocator.cpp\n> > > @@ -10,6 +10,7 @@\n> > > \n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > > \n> > > #include \"gstlibcamera-utils.h\"\n> > > @@ -188,13 +189,14 @@ gst_libcamera_allocator_new(std::shared_ptr camera,\n> > > {\n> > > \tauto *self = GST_LIBCAMERA_ALLOCATOR(g_object_new(GST_TYPE_LIBCAMERA_ALLOCATOR,\n> > > \t\t\t\t\t\t\t nullptr));\n> > > +\tunsigned int bufferCount = camera->properties().get(properties::MinimumRequests);\n> > > \n> > > \tself->fb_allocator = new FrameBufferAllocator(camera);\n> > > \tfor (StreamConfiguration &streamCfg : *config_) {\n> > > \t\tStream *stream = streamCfg.stream();\n> > > \t\tgint ret;\n> > > \n> > > -\t\tret = self->fb_allocator->allocate(stream);\n> > > +\t\tret = self->fb_allocator->allocate(stream, bufferCount);\n> > > \t\tif (ret == 0)\n> > > \t\t\treturn nullptr;\n> > > \n> > > diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp\n> > > index 25097f28a603..6444203547be 100644\n> > > --- a/src/lc-compliance/simple_capture.cpp\n> > > +++ b/src/lc-compliance/simple_capture.cpp\n> > > @@ -7,6 +7,8 @@\n> > > \n> > > #include \n> > > \n> > > +#include \n> > > +\n> > > #include \"simple_capture.h\"\n> > > \n> > > using namespace libcamera;\n> > > @@ -44,11 +46,12 @@ void SimpleCapture::configure(StreamRole role)\n> > > \n> > > void SimpleCapture::start()\n> > > {\n> > > +\tunsigned int bufferCount = camera_->properties().get(properties::MinNumRequests);\n> > > \tStream *stream = config_->at(0).stream();\n> > > -\tint count = allocator_->allocate(stream);\n> > > +\tint count = allocator_->allocate(stream, bufferCount);\n> > > \n> > > \tASSERT_GE(count, 0) << \"Failed to allocate buffers\";\n> > > -\tEXPECT_EQ(count, config_->at(0).bufferCount) << \"Allocated less buffers than expected\";\n> > > +\tEXPECT_EQ(static_cast(count), bufferCount) << \"Allocated less buffers than expected\";\n> \n> Is the cast required ? bufferCount is currently an unsigned int.\n> \n> > > \n> > > \tcamera_->requestCompleted.connect(this, &SimpleCapture::requestComplete);\n> > > \n> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > index c8858e71d105..e5a55a674afb 100644\n> > > --- a/src/libcamera/camera.cpp\n> > > +++ b/src/libcamera/camera.cpp\n> > > @@ -660,7 +660,7 @@ void Camera::disconnect()\n> > > \tdisconnected.emit(this);\n> > > }\n> > > \n> > > -int Camera::exportFrameBuffers(Stream *stream,\n> > > +int Camera::exportFrameBuffers(Stream *stream, unsigned int count,\n> > > \t\t\t std::vector> *buffers)\n> > > {\n> > > \tPrivate *const d = _d();\n> > > @@ -677,7 +677,7 @@ int Camera::exportFrameBuffers(Stream *stream,\n> > > \n> > > \treturn d->pipe_->invokeMethod(&PipelineHandler::exportFrameBuffers,\n> > > \t\t\t\t ConnectionTypeBlocking, this, stream,\n> > > -\t\t\t\t buffers);\n> > > +\t\t\t\t count, buffers);\n> > > }\n> > > \n> > > /**\n> > > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp\n> > > index 695073fd1c1c..2fc0db56176a 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> \n> Could you add a sentence to indicate that the function can allocate less\n> buffers than requested ?\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> > > @@ -86,14 +87,14 @@ 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> > > \tif (buffers_.count(stream)) {\n> > > \t\tLOG(Allocator, Error) << \"Buffers already allocated for stream\";\n> > > \t\treturn -EBUSY;\n> > > \t}\n> > > \n> > > -\tint ret = camera_->exportFrameBuffers(stream, &buffers_[stream]);\n> > > +\tint ret = camera_->exportFrameBuffers(stream, count, &buffers_[stream]);\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 76c3bb3d8aa9..5fd1757bfe13 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -134,7 +134,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> > > @@ -654,10 +654,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 f821d8fe1b6c..d1cd3d9dc082 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -251,7 +251,7 @@ public:\n> > > \tCameraConfiguration *generateConfiguration(Camera *camera, const 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> > > @@ -795,10 +795,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 cb4ca9a85fa8..11325875b929 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > @@ -141,7 +141,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> > > @@ -671,10 +671,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 348c554c8be4..1c25a7344f5f 100644\n> > > --- a/src/libcamera/pipeline/simple/simple.cpp\n> > > +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > > @@ -228,7 +228,7 @@ public:\n> > > \t\t\t\t\t\t const 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> > > @@ -776,10 +776,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 6ad7fb3c9f0c..fd39b3d3c72c 100644\n> > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > @@ -69,7 +69,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> > > @@ -223,11 +223,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 44c198ccf8b8..e89d53182c6d 100644\n> > > --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> > > +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> > > @@ -84,7 +84,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> > > @@ -299,11 +299,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 c9928d444b04..dc83bf6924bf 100644\n> > > --- a/src/libcamera/pipeline_handler.cpp\n> > > +++ b/src/libcamera/pipeline_handler.cpp\n> > > @@ -323,6 +323,7 @@ const ControlList &PipelineHandler::properties(const Camera *camera) const\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 method allocates buffers for the \\a stream from the devices associated\n> > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> > > index 39d034de6bb2..d7475b142d6f 100644\n> > > --- a/src/qcam/main_window.cpp\n> > > +++ b/src/qcam/main_window.cpp\n> > > @@ -25,6 +25,7 @@\n> > > #include \n> > > \n> > > #include \n> > > +#include \n> > > #include \n> > > \n> > > #include \"dng_writer.h\"\n> > > @@ -463,7 +464,15 @@ int MainWindow::startCapture()\n> > > \tfor (StreamConfiguration &config : *config_) {\n> > > \t\tStream *stream = config.stream();\n> > > \n> > > -\t\tret = allocator_->allocate(stream);\n> > > +\t\tunsigned int bufferCount = camera_->properties().get(properties::MinimumRequests);\n> > > +\n> > > +\t\t/*\n> > > +\t\t * Need at least two buffers, one for capture and another for\n> > > +\t\t * display\n> \n> s/display/display./\n> \n> > > +\t\t */\n> > > +\t\tbufferCount = std::max(bufferCount, 2U);\n> \n> This requires inclusion of .\n\nA bit of search showed that I should use actually, so I'll go with\nthat, unless I'm missing something.\n\n> \n> I think we should use bufferCount + 1, not max(bufferCount, 2). qcam\n> holds on one buffer for display. If the camera needs, let's say, 2\n> buffers to be able to capture, it will return a first buffer which will\n> then be queued to the display. At that point, the camera will have a\n> single buffer, and will hold onto it until a new buffer is queued. This\n> will never happen, as the buffer being displayed won't be returned to\n> libcamera until a new buffer is ready. We thus need three buffers in\n> that case.\n> \n> Thinking even more about this, I think we need\n> \n> \t\tbufferCount = std::max(bufferCount + 1, 3U);\n> \n> Even if the camera needs a single buffer only (in which case we would\n> use two buffers, one for capture and one for display), it's clear that\n> it will lead to frame drops. A minimum of 3 buffers is thus better.\n> \n> The cam application should also have a similar logic. It doesn't hold\n> onto a buffer, so in that case \n> \n> \t\tbufferCount = std::max(bufferCount, 2U);\n> \n> should be good enough to start with.\n\nRight, I did some tests now and noticed that qcam was indeed dropping frames\nwith just 2 requests, and with 3 it works fine. cam managed to not drop any\nframes even with a single request (!), but using 2 seems safer for different\nworkloads anyway.\n\n> \n> > > +\n> > > +\t\tret = allocator_->allocate(stream, bufferCount);\n> > > \t\tif (ret < 0) {\n> > > \t\t\tqWarning() << \"Failed to allocate capture buffers\";\n> > > \t\t\tgoto error;\n> > > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp\n> > > index 157ab94e0544..d01eacfa2b84 100644\n> > > --- a/src/v4l2/v4l2_camera.cpp\n> > > +++ b/src/v4l2/v4l2_camera.cpp\n> > > @@ -161,7 +161,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/capture.cpp b/test/camera/capture.cpp\n> > > index 238d98dbba16..2a531805cb2b 100644\n> > > --- a/test/camera/capture.cpp\n> > > +++ b/test/camera/capture.cpp\n> > > @@ -8,6 +8,7 @@\n> > > #include \n> > > \n> > > #include \n> > > +#include \n> > > \n> > > #include \n> > > #include \n> > > @@ -96,7 +97,8 @@ protected:\n> > > \n> > > \t\tStream *stream = cfg.stream();\n> > > \n> > > -\t\tint ret = allocator_->allocate(stream);\n> > > +\t\tunsigned int bufferCount = camera_->properties().get(properties::MinimumRequests);\n> > > +\t\tint ret = allocator_->allocate(stream, bufferCount);\n> \n> Similarly, this will result in frame drop at least with some pipeline\n> handlers. I fear we need to go one step further and already design the\n> API to tell applications how many buffers to allocate to avoid frame\n> draps. Per-frame control can be left out for now.\n\nI think that would be a good idea indeed. Technically it's simple to just add\nanother property, so I think the biggest issue here is naming :)\n\nI see there are plans to namespace properties, so I think ultimately the three\nof them should be grouped under a MinimumRequests namespace. For now, however,\nwe can just concatenate the names and add a \\todo like it's done for the others.\n\nI thought of the following possible names:\n\nMinimumRequests::LowMemory or StillCapture -> for the current property\nMinimumRequests::NoFrameDrop or Standard or VideoRecording -> for the new property\nMinimumRequests::PerFrameControl or PreciseControl -> for the future property\n\nReasonings:\n\nLowMemory: I think only applications on platforms with low memory available\nwould prefer to use this.\n\nStandard: I think most applications would allocate this many buffers. The\ncapture is fluid and not as many buffers are needed, but per-frame controls\naren't guaranteed. Seems like a good compromise.\n\nAnother option is to follow the StreamRole's example: the current property can\nhave frame drops, so it's only suitable for StillCapture, thus use this name,\nwhile the new property, with no frame drops, would be good enough for\nVideoRecording usage. For the per-frame control property there wouldn't be a\nStreamRole name to use, so we could keep using PerFrameControl, but we can also\nthink about this one later as it won't be implemented now.\n\nIf we go with the StreamRole naming scheme, the current property would thus be\nrenamed to MinimumRequestsStillCapture (while namespacing isn't possible), and\nthe new one MinimumRequestsVideoRecording.\n\nThoughts? (Naming is hard, so I expect some :) )\n\nThanks,\nNícolas\n\n> \n> > > \t\tif (ret < 0)\n> > > \t\t\treturn TestFail;\n> > > \n> > > diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp\n> > > index 26fb5ca17139..70015bb56c74 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> > > @@ -119,7 +120,8 @@ 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 = camera_->properties().get(properties::MinimumRequests);\n> > > +\t\tif (allocator_->allocate(stream, bufferCount) < 0)\n> > > \t\t\treturn TestFail;\n> > > \n> > > \t\tif (camera_->start())\n> > > diff --git a/test/mapped-buffer.cpp b/test/mapped-buffer.cpp\n> > > index c9479194cb68..e01211965364 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/framebuffer.h\"\n> > > \n> > > @@ -54,7 +55,8 @@ protected:\n> > > \n> > > \t\tstream_ = cfg.stream();\n> > > \n> > > -\t\tint ret = allocator_->allocate(stream_);\n> > > +\t\tunsigned int bufferCount = camera_->properties().get(properties::MinimumRequests);\n> > > +\t\tint ret = allocator_->allocate(stream_, bufferCount);\n> > > \t\tif (ret < 0)\n> > > \t\t\treturn TestFail;\n> > > \n> > > \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 63E83C3235\n\tfor ;\n\tWed, 4 Aug 2021 12:10:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C765B6881B;\n\tWed, 4 Aug 2021 14:10:48 +0200 (CEST)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk [46.235.227.227])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 979566026E\n\tfor ;\n\tWed, 4 Aug 2021 14:10:47 +0200 (CEST)","from notapiano (unknown [IPv6:2804:14c:1a9:2434:b693:c9:5cb6:b688])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\t(Authenticated sender: nfraprado)\n\tby bhuna.collabora.co.uk (Postfix) with ESMTPSA id 393361F40F39;\n\tWed, 4 Aug 2021 13:10:44 +0100 (BST)"],"Date":"Wed, 4 Aug 2021 09:10:40 -0300","From":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4=?= Prado ","To":"Laurent Pinchart ","Message-ID":"<20210804121040.xaie7xcw5pqv4syw@notapiano>","References":"<20210722232851.747614-1-nfraprado@collabora.com>\n\t<20210722232851.747614-3-nfraprado@collabora.com>\n\t<650924e2-8a93-ba95-48c8-3e8d2eebfe0b@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v7 02/11] 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","Cc":"libcamera-devel@lists.libcamera.org, kernel@collabora.com, =?utf-8?q?A?=\n\t=?utf-8?b?bmRyw6k=?= Almeida ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]