[{"id":16551,"web_url":"https://patchwork.libcamera.org/comment/16551/","msgid":"","date":"2021-04-26T02:59:09","subject":"Re: [libcamera-devel] [PATCH v3 2/4] 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":"Hi Nícolas,\n\nThank you for the patch.\n\nOn Wed, Apr 21, 2021 at 01:51:37PM -0300, Nícolas F. R. A. Prado wrote:\n> Make FrameBufferAllocator::allocate() require a 'count' argument for the\n> amount of buffers to be allocated.\n> \n> Signed-off-by: Nícolas F. R. A. Prado \n> ---\n> include/libcamera/camera.h | 2 +-\n> include/libcamera/framebuffer_allocator.h | 2 +-\n> include/libcamera/internal/pipeline_handler.h | 2 +-\n> src/cam/capture.cpp | 10 ++++------\n> src/gstreamer/gstlibcameraallocator.cpp | 4 +++-\n> src/lc-compliance/simple_capture.cpp | 13 +++++++++++--\n> src/lc-compliance/simple_capture.h | 1 +\n> src/lc-compliance/single_stream.cpp | 6 ++++++\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 | 4 +++-\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> 22 files changed, 63 insertions(+), 35 deletions(-)\n> \n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index d71641805c0a..656cd92aecde 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -115,7 +115,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 0c85631a1da2..f1ae37288d50 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 c6454db6b2c4..dc04ba041fe5 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -75,7 +75,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/cam/capture.cpp b/src/cam/capture.cpp\n> index 7b55fc677022..8ad121f5adc8 100644\n> --- a/src/cam/capture.cpp\n> +++ b/src/cam/capture.cpp\n> @@ -10,6 +10,8 @@\n> #include \n> #include \n> \n> +#include \n> +\n> #include \"capture.h\"\n> #include \"main.h\"\n> \n> @@ -77,17 +79,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::QueueDepth);\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\nIf not enough memory is available to allocate nbuffers, or if a driver\ndecides that the number is too high, we could receive less buffers than\nrequested. We would then crash below, when creating the requests. I\nthink we still need to handle this case.\n\n> \t}\n> \n> \t/*\n> diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp\n> index 7bd8ba2db71d..e7a5b5a27226 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> +\tint bufferCount = camera->properties().get(properties::QueueDepth);\n\nIt's an unsigned int above, should it be unsigned here too ? Same for\nlc-compliance and other places below.\n\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 64e862a08e3a..875772a80c27 100644\n> --- a/src/lc-compliance/simple_capture.cpp\n> +++ b/src/lc-compliance/simple_capture.cpp\n> @@ -5,6 +5,8 @@\n> * simple_capture.cpp - Simple capture helper\n> */\n> \n> +#include \n> +\n> #include \"simple_capture.h\"\n> \n> using namespace libcamera;\n> @@ -39,12 +41,19 @@ Results::Result SimpleCapture::configure(StreamRole role)\n> \treturn { Results::Pass, \"Configure camera\" };\n> }\n> \n> -Results::Result SimpleCapture::start()\n> +Results::Result SimpleCapture::allocateBuffers()\n> {\n> +\tint minBuffers = camera_->properties().get(properties::QueueDepth);\n\nDepending on how you decide to document and name QueueDepth (see the\nreview of 1/4), this variable may be better named numBuffers (or\nsomething else).\n\n> \tStream *stream = config_->at(0).stream();\n> -\tif (allocator_->allocate(stream) < 0)\n> +\n> +\tif (allocator_->allocate(stream, minBuffers) < 0)\n> \t\treturn { Results::Fail, \"Failed to allocate buffers\" };\n> \n> +\treturn { Results::Pass, \"Allocated buffers\" };\n\nDo we need a message when returning Pass ?\n\n> +}\n> +\n> +Results::Result SimpleCapture::start()\n> +{\n> \tif (camera_->start())\n> \t\treturn { Results::Fail, \"Failed to start camera\" };\n> \n> diff --git a/src/lc-compliance/simple_capture.h b/src/lc-compliance/simple_capture.h\n> index d9de53fb63a3..82e2c56a55f1 100644\n> --- a/src/lc-compliance/simple_capture.h\n> +++ b/src/lc-compliance/simple_capture.h\n> @@ -18,6 +18,7 @@ class SimpleCapture\n> {\n> public:\n> \tResults::Result configure(libcamera::StreamRole role);\n> +\tResults::Result allocateBuffers();\n> \n> protected:\n> \tSimpleCapture(std::shared_ptr camera);\n> diff --git a/src/lc-compliance/single_stream.cpp b/src/lc-compliance/single_stream.cpp\n> index 8318b42f42d6..649291c7bb73 100644\n> --- a/src/lc-compliance/single_stream.cpp\n> +++ b/src/lc-compliance/single_stream.cpp\n> @@ -23,6 +23,9 @@ Results::Result testRequestBalance(std::shared_ptr camera,\n> \t\treturn ret;\n> \n> \tfor (unsigned int starts = 0; starts < startCycles; starts++) {\n> +\t\tcapture.allocateBuffers();\n> +\t\tif (ret.first != Results::Pass)\n> +\t\t\treturn ret;\n> \t\tret = capture.capture(numRequests);\n> \t\tif (ret.first != Results::Pass)\n> \t\t\treturn ret;\n> @@ -39,6 +42,9 @@ Results::Result testRequestUnbalance(std::shared_ptr camera,\n> \tSimpleCaptureUnbalanced capture(camera);\n> \n> \tResults::Result ret = capture.configure(role);\n> +\tif (ret.first != Results::Pass)\n> +\t\treturn ret;\n> +\tcapture.allocateBuffers();\n> \tif (ret.first != Results::Pass)\n> \t\treturn ret;\n> \n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 763f3b9926fd..e4da6e042bd0 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -656,7 +656,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 = LIBCAMERA_D_PTR();\n> @@ -673,7 +673,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 2fbba37a1b0b..92d121f8c4ac 100644\n> --- a/src/libcamera/framebuffer_allocator.cpp\n> +++ b/src/libcamera/framebuffer_allocator.cpp\n> @@ -70,6 +70,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 amount of buffers to allocate\n\ns/amount/number/\n\nShould this be also updated to explicitly mention that the number of\nallocated buffers may be different than count ? Same for the\nPipelineHandler class below.\n\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> @@ -85,14 +86,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 6067db2f37a3..c8fcc2fda75f 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -130,7 +130,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> @@ -641,10 +641,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 8d1ade3a4352..3f35596fe550 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -249,7 +249,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> @@ -786,10 +786,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 7d876e9387d7..2d95c1ca2a43 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -140,7 +140,7 @@ public:\n> \t\tconst StreamRoles &roles) override;\n> \tint configure(Camera *camera, CameraConfiguration *config) override;\n> \n> -\tint exportFrameBuffers(Camera *camera, Stream *stream,\n> +\tint exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,\n> \t\t\t std::vector> *buffers) override;\n> \n> \tint start(Camera *camera, const ControlList *controls) override;\n> @@ -667,10 +667,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 6ee24f2f14e8..b9f14be6733f 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -225,7 +225,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> @@ -764,10 +764,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 591f46b60d23..a148c35f1265 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 605b3fe89152..22d6fdcbb141 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 3b3150bdbbf7..cdd456c599a4 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -322,6 +322,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 amount of buffers to allocate\n\ns/amount/number/\n\nVery nice patch overall, I think it really improves the API.\n\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..d2ddd976584a 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,8 @@ int MainWindow::startCapture()\n> \tfor (StreamConfiguration &config : *config_) {\n> \t\tStream *stream = config.stream();\n> \n> -\t\tret = allocator_->allocate(stream);\n> +\t\tint minBuffers = camera_->properties().get(properties::QueueDepth);\n> +\t\tret = allocator_->allocate(stream, minBuffers);\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 97825c715bba..53d97f3e6b86 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 c4bc21100777..24ffbd8b2f6f 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 \"libcamera/internal/event_dispatcher.h\"\n> #include \"libcamera/internal/thread.h\"\n> @@ -96,7 +97,8 @@ protected:\n> \n> \t\tStream *stream = cfg.stream();\n> \n> -\t\tint ret = allocator_->allocate(stream);\n> +\t\tint minBuffers = camera_->properties().get(properties::QueueDepth);\n> +\t\tint ret = allocator_->allocate(stream, minBuffers);\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 f0c3d7764027..80caa897bcaa 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> @@ -118,7 +119,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\tint minBuffers = camera_->properties().get(properties::QueueDepth);\n> +\t\tif (allocator_->allocate(stream, minBuffers) < 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 5de8201e45f6..7c0264157229 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/buffer.h\"\n> \n> @@ -54,7 +55,8 @@ protected:\n> \n> \t\tstream_ = cfg.stream();\n> \n> -\t\tint ret = allocator_->allocate(stream_);\n> +\t\tint minBuffers = camera_->properties().get(properties::QueueDepth);\n> +\t\tint ret = allocator_->allocate(stream_, minBuffers);\n> \t\tif (ret < 0)\n> \t\t\treturn TestFail;\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 3FE53BDC91\n\tfor ;\n\tMon, 26 Apr 2021 02:59:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A0A5068883;\n\tMon, 26 Apr 2021 04:59:16 +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 4C9CB605BD\n\tfor ;\n\tMon, 26 Apr 2021 04:59:15 +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 A7EFB4FB;\n\tMon, 26 Apr 2021 04:59:14 +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=\"ADc2rTIN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619405954;\n\tbh=nFFbcnAXynKjmgCskBwPC9v4tLczGkrx7ZIg14p5EhQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ADc2rTINo3dMxSQoxWBvBJqskfzJgcTQK0GNliTfsnmMNL8AvW29m92Idr6xJ10dE\n\t+JVUIiho7MnT8Tdge8mVJgP0WqZxRmv+4lv5UMgAeRL8Bi+iM7Yp03sEnmgROxPhWr\n\tGjlXmdb6pFneLwda8l7MxNHHoGOWoqo24nf98FoA=","Date":"Mon, 26 Apr 2021 05:59:09 +0300","From":"Laurent Pinchart ","To":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4=?= Prado ","Message-ID":"","References":"<20210421165139.318432-1-nfraprado@collabora.com>\n\t<20210421165139.318432-3-nfraprado@collabora.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210421165139.318432-3-nfraprado@collabora.com>","Subject":"Re: [libcamera-devel] [PATCH v3 2/4] 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, =?utf-8?b?QW5kcsOp?=\n\tAlmeida , kernel@collabora.com","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16593,"web_url":"https://patchwork.libcamera.org/comment/16593/","msgid":"","date":"2021-04-26T17:40:07","subject":"Re: [libcamera-devel] [PATCH v3 2/4] 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":"Em 2021-04-25 23:59, Laurent Pinchart escreveu:\n\n> Hi Nícolas,\n>\n> Thank you for the patch.\n>\n> On Wed, Apr 21, 2021 at 01:51:37PM -0300, Nícolas F. R. A. Prado wrote:\n> > Make FrameBufferAllocator::allocate() require a 'count' argument for the\n> > amount of buffers to be allocated.\n> > \n> > Signed-off-by: Nícolas F. R. A. Prado \n> > ---\n> > include/libcamera/camera.h | 2 +-\n> > include/libcamera/framebuffer_allocator.h | 2 +-\n> > include/libcamera/internal/pipeline_handler.h | 2 +-\n> > src/cam/capture.cpp | 10 ++++------\n> > src/gstreamer/gstlibcameraallocator.cpp | 4 +++-\n> > src/lc-compliance/simple_capture.cpp | 13 +++++++++++--\n> > src/lc-compliance/simple_capture.h | 1 +\n> > src/lc-compliance/single_stream.cpp | 6 ++++++\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 | 4 +++-\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> > 22 files changed, 63 insertions(+), 35 deletions(-)\n> > \n> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > index d71641805c0a..656cd92aecde 100644\n> > --- a/include/libcamera/camera.h\n> > +++ b/include/libcamera/camera.h\n> > @@ -115,7 +115,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 0c85631a1da2..f1ae37288d50 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 c6454db6b2c4..dc04ba041fe5 100644\n> > --- a/include/libcamera/internal/pipeline_handler.h\n> > +++ b/include/libcamera/internal/pipeline_handler.h\n> > @@ -75,7 +75,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/cam/capture.cpp b/src/cam/capture.cpp\n> > index 7b55fc677022..8ad121f5adc8 100644\n> > --- a/src/cam/capture.cpp\n> > +++ b/src/cam/capture.cpp\n> > @@ -10,6 +10,8 @@\n> > #include \n> > #include \n> > \n> > +#include \n> > +\n> > #include \"capture.h\"\n> > #include \"main.h\"\n> > \n> > @@ -77,17 +79,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::QueueDepth);\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>\n> If not enough memory is available to allocate nbuffers, or if a driver\n> decides that the number is too high, we could receive less buffers than\n> requested. We would then crash below, when creating the requests. I\n> think we still need to handle this case.\n\nHm, but doesn't V4L2VideoDevice::requestBuffers() catch that in \n\n\tif (rb.count < count) {\n\t\tLOG(V4L2, Error)\n\t\t\t<< \"Not enough buffers provided by V4L2VideoDevice\";\n\t\trequestBuffers(0, memoryType);\n\t\treturn -ENOMEM;\n\t}\n\n? That return value is returned all the way up which means Capture::capture()\nwill return as well.\n\nNot so much related to this but I just realized it: Isn't it a problem that the\nQueueDepth is a property global to the camera, instead of specific to a stream?\n\n>\n> > \t}\n> > \n> > \t/*\n> > diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp\n> > index 7bd8ba2db71d..e7a5b5a27226 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> > +\tint bufferCount = camera->properties().get(properties::QueueDepth);\n>\n> It's an unsigned int above, should it be unsigned here too ? Same for\n> lc-compliance and other places below.\n\nSure. I didn't use unsigned int because int32_t in the property_ids.yaml sounds like\nsigned. I'll change everything to unsigned.\n\n>\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 64e862a08e3a..875772a80c27 100644\n> > --- a/src/lc-compliance/simple_capture.cpp\n> > +++ b/src/lc-compliance/simple_capture.cpp\n> > @@ -5,6 +5,8 @@\n> > * simple_capture.cpp - Simple capture helper\n> > */\n> > \n> > +#include \n> > +\n> > #include \"simple_capture.h\"\n> > \n> > using namespace libcamera;\n> > @@ -39,12 +41,19 @@ Results::Result SimpleCapture::configure(StreamRole role)\n> > \treturn { Results::Pass, \"Configure camera\" };\n> > }\n> > \n> > -Results::Result SimpleCapture::start()\n> > +Results::Result SimpleCapture::allocateBuffers()\n> > {\n> > +\tint minBuffers = camera_->properties().get(properties::QueueDepth);\n>\n> Depending on how you decide to document and name QueueDepth (see the\n> review of 1/4), this variable may be better named numBuffers (or\n> something else).\n>\n> > \tStream *stream = config_->at(0).stream();\n> > -\tif (allocator_->allocate(stream) < 0)\n> > +\n> > +\tif (allocator_->allocate(stream, minBuffers) < 0)\n> > \t\treturn { Results::Fail, \"Failed to allocate buffers\" };\n> > \n> > +\treturn { Results::Pass, \"Allocated buffers\" };\n>\n> Do we need a message when returning Pass ?\n\nWell, the Pass message gets used to report which test passed, although since\nthis is only part of the test it doesn't get used. In any case the return type\nrequires a string, so at least an empty string should be returned (but then,\nmight as well give a meaningful message).\n\nThanks,\nNícolas","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 66317BDCA0\n\tfor ;\n\tMon, 26 Apr 2021 18:56:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E81FC605BE;\n\tMon, 26 Apr 2021 20:56:40 +0200 (CEST)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk [46.235.227.227])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B1065602D1\n\tfor ;\n\tMon, 26 Apr 2021 20:56:39 +0200 (CEST)","from localhost (unknown\n\t[IPv6:2804:14c:1a9:2978:c5f3:918d:a027:70d9])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128\n\tbits))\n\t(No client certificate requested) (Authenticated sender: nfraprado)\n\tby bhuna.collabora.co.uk (Postfix) with ESMTPSA id 176CF1F41E0B;\n\tMon, 26 Apr 2021 19:56:36 +0100 (BST)"],"Mime-Version":"1.0","From":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4gUHJhZG8=?= ","To":"\"Laurent Pinchart\" ","Date":"Mon, 26 Apr 2021 14:40:07 -0300","Message-Id":"","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 2/4] 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, =?utf-8?q?Andr=C3=A9_Almeida?=\n\t, kernel@collabora.com","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]