[libcamera-devel,v7,02/11] libcamera: framebuffer_allocator: Make allocate() require count

13076 diff mbox series

Make FrameBufferAllocator::allocate() require a 'count' argument for the
number of buffers to be allocated.

Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
---

No changes in v7

Changes in v6:
- Changed static_cast to convert 'count' to unsigned int instead of
  'bufferCount' to int when comparing

Changes in v5:
- Made sure that qcam allocates at least 2 buffers

 include/libcamera/camera.h                         |  2 +-
 include/libcamera/framebuffer_allocator.h          |  2 +-
 include/libcamera/internal/pipeline_handler.h      |  2 +-
 src/android/camera_stream.cpp                      |  5 ++++-
 src/cam/capture.cpp                                |  9 +++------
 src/gstreamer/gstlibcameraallocator.cpp            |  4 +++-
 src/lc-compliance/simple_capture.cpp               |  7 +++++--
 src/libcamera/camera.cpp                           |  4 ++--
 src/libcamera/framebuffer_allocator.cpp            |  5 +++--
 src/libcamera/pipeline/ipu3/ipu3.cpp               |  4 ++--
 src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  4 ++--
 src/libcamera/pipeline/rkisp1/rkisp1.cpp           |  4 ++--
 src/libcamera/pipeline/simple/simple.cpp           |  4 ++--
 src/libcamera/pipeline/uvcvideo/uvcvideo.cpp       |  7 ++++---
 src/libcamera/pipeline/vimc/vimc.cpp               |  7 ++++---
 src/libcamera/pipeline_handler.cpp                 |  1 +
 src/qcam/main_window.cpp                           | 11 ++++++++++-
 src/v4l2/v4l2_camera.cpp                           |  2 +-
 test/camera/capture.cpp                            |  4 +++-
 test/camera/statemachine.cpp                       |  4 +++-
 test/mapped-buffer.cpp                             |  4 +++-
 21 files changed, 60 insertions(+), 36 deletions(-)

Hi Nicolas,

On 23/07/2021 00:28, Nícolas F. R. A. Prado wrote:
> Make FrameBufferAllocator::allocate() require a 'count' argument for the
> number of buffers to be allocated.
> 
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> ---
> 
> No changes in v7
> 
> Changes in v6:
> - Changed static_cast to convert 'count' to unsigned int instead of
>   'bufferCount' to int when comparing
> 
> Changes in v5:
> - Made sure that qcam allocates at least 2 buffers
> 
>  include/libcamera/camera.h                         |  2 +-
>  include/libcamera/framebuffer_allocator.h          |  2 +-
>  include/libcamera/internal/pipeline_handler.h      |  2 +-
>  src/android/camera_stream.cpp                      |  5 ++++-
>  src/cam/capture.cpp                                |  9 +++------
>  src/gstreamer/gstlibcameraallocator.cpp            |  4 +++-
>  src/lc-compliance/simple_capture.cpp               |  7 +++++--
>  src/libcamera/camera.cpp                           |  4 ++--
>  src/libcamera/framebuffer_allocator.cpp            |  5 +++--
>  src/libcamera/pipeline/ipu3/ipu3.cpp               |  4 ++--
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  4 ++--
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp           |  4 ++--
>  src/libcamera/pipeline/simple/simple.cpp           |  4 ++--
>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp       |  7 ++++---
>  src/libcamera/pipeline/vimc/vimc.cpp               |  7 ++++---
>  src/libcamera/pipeline_handler.cpp                 |  1 +
>  src/qcam/main_window.cpp                           | 11 ++++++++++-
>  src/v4l2/v4l2_camera.cpp                           |  2 +-
>  test/camera/capture.cpp                            |  4 +++-
>  test/camera/statemachine.cpp                       |  4 +++-
>  test/mapped-buffer.cpp                             |  4 +++-
>  21 files changed, 60 insertions(+), 36 deletions(-)

This patch should also update the Documentation/application-developer
guide which references the public API in section
  "Using the libcamera ``FrameBufferAllocator``"

It can be a patch on top if it helps, or squashed into this one.

With that, I think I quite like this change as it gives better
flexibility to the application which otherwise might provide buffers
elsewhere anyway.

With the Documentation fixed, or with that being prepared as a separate
patch:

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

--
Kieran

> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index b081907e0cb1..9f1767e4c406 100644
> --- a/include/libcamera/camera.h
> +++ b/include/libcamera/camera.h
> @@ -116,7 +116,7 @@ private:
>  	void requestComplete(Request *request);
>  
>  	friend class FrameBufferAllocator;
> -	int exportFrameBuffers(Stream *stream,
> +	int exportFrameBuffers(Stream *stream, unsigned int count,
>  			       std::vector<std::unique_ptr<FrameBuffer>> *buffers);
>  };
>  
> diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h
> index cbc9ce101889..2d5a6e98e10c 100644
> --- a/include/libcamera/framebuffer_allocator.h
> +++ b/include/libcamera/framebuffer_allocator.h
> @@ -25,7 +25,7 @@ public:
>  	FrameBufferAllocator(std::shared_ptr<Camera> camera);
>  	~FrameBufferAllocator();
>  
> -	int allocate(Stream *stream);
> +	int allocate(Stream *stream, unsigned int count);
>  	int free(Stream *stream);
>  
>  	bool allocated() const { return !buffers_.empty(); }
> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> index 9e2d65d6f2c5..0b4b2e4947c0 100644
> --- a/include/libcamera/internal/pipeline_handler.h
> +++ b/include/libcamera/internal/pipeline_handler.h
> @@ -76,7 +76,7 @@ public:
>  		const StreamRoles &roles) = 0;
>  	virtual int configure(Camera *camera, CameraConfiguration *config) = 0;
>  
> -	virtual int exportFrameBuffers(Camera *camera, Stream *stream,
> +	virtual int exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,
>  				       std::vector<std::unique_ptr<FrameBuffer>> *buffers) = 0;
>  
>  	virtual int start(Camera *camera, const ControlList *controls) = 0;
> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> index bf4a7b41a70a..b168e3c0c288 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -13,6 +13,7 @@
>  #include "jpeg/post_processor_jpeg.h"
>  
>  #include <libcamera/formats.h>
> +#include <libcamera/property_ids.h>
>  
>  using namespace libcamera;
>  
> @@ -81,8 +82,10 @@ int CameraStream::configure()
>  			return ret;
>  	}
>  
> +	unsigned int bufferCount = cameraDevice_->camera()->properties().get(properties::MinimumRequests);
> +
>  	if (allocator_) {
> -		int ret = allocator_->allocate(stream());
> +		int ret = allocator_->allocate(stream(), bufferCount);
>  		if (ret < 0)
>  			return ret;
>  
> diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
> index 3c3e3a53adf7..633f9c6e4f25 100644
> --- a/src/cam/capture.cpp
> +++ b/src/cam/capture.cpp
> @@ -11,6 +11,7 @@
>  #include <sstream>
>  
>  #include <libcamera/control_ids.h>
> +#include <libcamera/property_ids.h>
>  
>  #include "capture.h"
>  #include "main.h"
> @@ -81,17 +82,13 @@ int Capture::capture(FrameBufferAllocator *allocator)
>  {
>  	int ret;
>  
> -	/* Identify the stream with the least number of buffers. */
> -	unsigned int nbuffers = UINT_MAX;
> +	unsigned int nbuffers = camera_->properties().get(properties::MinimumRequests);
>  	for (StreamConfiguration &cfg : *config_) {
> -		ret = allocator->allocate(cfg.stream());
> +		ret = allocator->allocate(cfg.stream(), nbuffers);
>  		if (ret < 0) {
>  			std::cerr << "Can't allocate buffers" << std::endl;
>  			return -ENOMEM;
>  		}
> -
> -		unsigned int allocated = allocator->buffers(cfg.stream()).size();
> -		nbuffers = std::min(nbuffers, allocated);
>  	}
>  
>  	/*
> diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp
> index 7bd8ba2db71d..8cc42edfaf61 100644
> --- a/src/gstreamer/gstlibcameraallocator.cpp
> +++ b/src/gstreamer/gstlibcameraallocator.cpp
> @@ -10,6 +10,7 @@
>  
>  #include <libcamera/camera.h>
>  #include <libcamera/framebuffer_allocator.h>
> +#include <libcamera/property_ids.h>
>  #include <libcamera/stream.h>
>  
>  #include "gstlibcamera-utils.h"
> @@ -188,13 +189,14 @@ gst_libcamera_allocator_new(std::shared_ptr<Camera> camera,
>  {
>  	auto *self = GST_LIBCAMERA_ALLOCATOR(g_object_new(GST_TYPE_LIBCAMERA_ALLOCATOR,
>  							  nullptr));
> +	unsigned int bufferCount = camera->properties().get(properties::MinimumRequests);
>  
>  	self->fb_allocator = new FrameBufferAllocator(camera);
>  	for (StreamConfiguration &streamCfg : *config_) {
>  		Stream *stream = streamCfg.stream();
>  		gint ret;
>  
> -		ret = self->fb_allocator->allocate(stream);
> +		ret = self->fb_allocator->allocate(stream, bufferCount);
>  		if (ret == 0)
>  			return nullptr;
>  
> diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp
> index 25097f28a603..6444203547be 100644
> --- a/src/lc-compliance/simple_capture.cpp
> +++ b/src/lc-compliance/simple_capture.cpp
> @@ -7,6 +7,8 @@
>  
>  #include <gtest/gtest.h>
>  
> +#include <libcamera/property_ids.h>
> +
>  #include "simple_capture.h"
>  
>  using namespace libcamera;
> @@ -44,11 +46,12 @@ void SimpleCapture::configure(StreamRole role)
>  
>  void SimpleCapture::start()
>  {
> +	unsigned int bufferCount = camera_->properties().get(properties::MinNumRequests);
>  	Stream *stream = config_->at(0).stream();
> -	int count = allocator_->allocate(stream);
> +	int count = allocator_->allocate(stream, bufferCount);
>  
>  	ASSERT_GE(count, 0) << "Failed to allocate buffers";
> -	EXPECT_EQ(count, config_->at(0).bufferCount) << "Allocated less buffers than expected";
> +	EXPECT_EQ(static_cast<unsigned int>(count), bufferCount) << "Allocated less buffers than expected";
>  
>  	camera_->requestCompleted.connect(this, &SimpleCapture::requestComplete);
>  
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index c8858e71d105..e5a55a674afb 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -660,7 +660,7 @@ void Camera::disconnect()
>  	disconnected.emit(this);
>  }
>  
> -int Camera::exportFrameBuffers(Stream *stream,
> +int Camera::exportFrameBuffers(Stream *stream, unsigned int count,
>  			       std::vector<std::unique_ptr<FrameBuffer>> *buffers)
>  {
>  	Private *const d = _d();
> @@ -677,7 +677,7 @@ int Camera::exportFrameBuffers(Stream *stream,
>  
>  	return d->pipe_->invokeMethod(&PipelineHandler::exportFrameBuffers,
>  				      ConnectionTypeBlocking, this, stream,
> -				      buffers);
> +				      count, buffers);
>  }
>  
>  /**
> diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp
> index 695073fd1c1c..2fc0db56176a 100644
> --- a/src/libcamera/framebuffer_allocator.cpp
> +++ b/src/libcamera/framebuffer_allocator.cpp
> @@ -71,6 +71,7 @@ FrameBufferAllocator::~FrameBufferAllocator()
>  /**
>   * \brief Allocate buffers for a configured stream
>   * \param[in] stream The stream to allocate buffers for
> + * \param[in] count The number of buffers to allocate
>   *
>   * Allocate buffers suitable for capturing frames from the \a stream. The Camera
>   * shall have been previously configured with Camera::configure() and shall be
> @@ -86,14 +87,14 @@ FrameBufferAllocator::~FrameBufferAllocator()
>   * not part of the active camera configuration
>   * \retval -EBUSY Buffers are already allocated for the \a stream
>   */
> -int FrameBufferAllocator::allocate(Stream *stream)
> +int FrameBufferAllocator::allocate(Stream *stream, unsigned int count)
>  {
>  	if (buffers_.count(stream)) {
>  		LOG(Allocator, Error) << "Buffers already allocated for stream";
>  		return -EBUSY;
>  	}
>  
> -	int ret = camera_->exportFrameBuffers(stream, &buffers_[stream]);
> +	int ret = camera_->exportFrameBuffers(stream, count, &buffers_[stream]);
>  	if (ret == -EINVAL)
>  		LOG(Allocator, Error)
>  			<< "Stream is not part of " << camera_->id()
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 76c3bb3d8aa9..5fd1757bfe13 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -134,7 +134,7 @@ public:
>  		const StreamRoles &roles) override;
>  	int configure(Camera *camera, CameraConfiguration *config) override;
>  
> -	int exportFrameBuffers(Camera *camera, Stream *stream,
> +	int exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,
>  			       std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
>  
>  	int start(Camera *camera, const ControlList *controls) override;
> @@ -654,10 +654,10 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
>  }
>  
>  int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,
> +					    unsigned int count,
>  					    std::vector<std::unique_ptr<FrameBuffer>> *buffers)
>  {
>  	IPU3CameraData *data = cameraData(camera);
> -	unsigned int count = stream->configuration().bufferCount;
>  
>  	if (stream == &data->outStream_)
>  		return data->imgu_->output_->exportBuffers(count, buffers);
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index f821d8fe1b6c..d1cd3d9dc082 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -251,7 +251,7 @@ public:
>  	CameraConfiguration *generateConfiguration(Camera *camera, const StreamRoles &roles) override;
>  	int configure(Camera *camera, CameraConfiguration *config) override;
>  
> -	int exportFrameBuffers(Camera *camera, Stream *stream,
> +	int exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,
>  			       std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
>  
>  	int start(Camera *camera, const ControlList *controls) override;
> @@ -795,10 +795,10 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  }
>  
>  int PipelineHandlerRPi::exportFrameBuffers([[maybe_unused]] Camera *camera, Stream *stream,
> +					   unsigned int count,
>  					   std::vector<std::unique_ptr<FrameBuffer>> *buffers)
>  {
>  	RPi::Stream *s = static_cast<RPi::Stream *>(stream);
> -	unsigned int count = stream->configuration().bufferCount;
>  	int ret = s->dev()->exportBuffers(count, buffers);
>  
>  	s->setExportedBuffers(buffers);
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index cb4ca9a85fa8..11325875b929 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -141,7 +141,7 @@ public:
>  		const StreamRoles &roles) override;
>  	int configure(Camera *camera, CameraConfiguration *config) override;
>  
> -	int exportFrameBuffers(Camera *camera, Stream *stream,
> +	int exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,
>  			       std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
>  
>  	int start(Camera *camera, const ControlList *controls) override;
> @@ -671,10 +671,10 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>  }
>  
>  int PipelineHandlerRkISP1::exportFrameBuffers([[maybe_unused]] Camera *camera, Stream *stream,
> +					      unsigned int count,
>  					      std::vector<std::unique_ptr<FrameBuffer>> *buffers)
>  {
>  	RkISP1CameraData *data = cameraData(camera);
> -	unsigned int count = stream->configuration().bufferCount;
>  
>  	if (stream == &data->mainPathStream_)
>  		return mainPath_.exportBuffers(count, buffers);
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 348c554c8be4..1c25a7344f5f 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -228,7 +228,7 @@ public:
>  						   const StreamRoles &roles) override;
>  	int configure(Camera *camera, CameraConfiguration *config) override;
>  
> -	int exportFrameBuffers(Camera *camera, Stream *stream,
> +	int exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,
>  			       std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
>  
>  	int start(Camera *camera, const ControlList *controls) override;
> @@ -776,10 +776,10 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
>  }
>  
>  int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream,
> +					      unsigned int count,
>  					      std::vector<std::unique_ptr<FrameBuffer>> *buffers)
>  {
>  	SimpleCameraData *data = cameraData(camera);
> -	unsigned int count = stream->configuration().bufferCount;
>  
>  	/*
>  	 * Export buffers on the converter or capture video node, depending on
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index 6ad7fb3c9f0c..fd39b3d3c72c 100644
> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> @@ -69,7 +69,7 @@ public:
>  		const StreamRoles &roles) override;
>  	int configure(Camera *camera, CameraConfiguration *config) override;
>  
> -	int exportFrameBuffers(Camera *camera, Stream *stream,
> +	int exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,
>  			       std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
>  
>  	int start(Camera *camera, const ControlList *controls) override;
> @@ -223,11 +223,12 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config)
>  	return 0;
>  }
>  
> -int PipelineHandlerUVC::exportFrameBuffers(Camera *camera, Stream *stream,
> +int PipelineHandlerUVC::exportFrameBuffers(Camera *camera,
> +					   [[maybe_unused]] Stream *stream,
> +					   unsigned int count,
>  					   std::vector<std::unique_ptr<FrameBuffer>> *buffers)
>  {
>  	UVCCameraData *data = cameraData(camera);
> -	unsigned int count = stream->configuration().bufferCount;
>  
>  	return data->video_->exportBuffers(count, buffers);
>  }
> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> index 44c198ccf8b8..e89d53182c6d 100644
> --- a/src/libcamera/pipeline/vimc/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> @@ -84,7 +84,7 @@ public:
>  		const StreamRoles &roles) override;
>  	int configure(Camera *camera, CameraConfiguration *config) override;
>  
> -	int exportFrameBuffers(Camera *camera, Stream *stream,
> +	int exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,
>  			       std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
>  
>  	int start(Camera *camera, const ControlList *controls) override;
> @@ -299,11 +299,12 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)
>  	return 0;
>  }
>  
> -int PipelineHandlerVimc::exportFrameBuffers(Camera *camera, Stream *stream,
> +int PipelineHandlerVimc::exportFrameBuffers(Camera *camera,
> +					    [[maybe_unused]] Stream *stream,
> +					    unsigned int count,
>  					    std::vector<std::unique_ptr<FrameBuffer>> *buffers)
>  {
>  	VimcCameraData *data = cameraData(camera);
> -	unsigned int count = stream->configuration().bufferCount;
>  
>  	return data->video_->exportBuffers(count, buffers);
>  }
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index c9928d444b04..dc83bf6924bf 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -323,6 +323,7 @@ const ControlList &PipelineHandler::properties(const Camera *camera) const
>   * \brief Allocate and export buffers for \a stream
>   * \param[in] camera The camera
>   * \param[in] stream The stream to allocate buffers for
> + * \param[in] count The number of buffers to allocate
>   * \param[out] buffers Array of buffers successfully allocated
>   *
>   * This method allocates buffers for the \a stream from the devices associated
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> index 39d034de6bb2..d7475b142d6f 100644
> --- a/src/qcam/main_window.cpp
> +++ b/src/qcam/main_window.cpp
> @@ -25,6 +25,7 @@
>  #include <QtDebug>
>  
>  #include <libcamera/camera_manager.h>
> +#include <libcamera/property_ids.h>
>  #include <libcamera/version.h>
>  
>  #include "dng_writer.h"
> @@ -463,7 +464,15 @@ int MainWindow::startCapture()
>  	for (StreamConfiguration &config : *config_) {
>  		Stream *stream = config.stream();
>  
> -		ret = allocator_->allocate(stream);
> +		unsigned int bufferCount = camera_->properties().get(properties::MinimumRequests);
> +
> +		/*
> +		 * Need at least two buffers, one for capture and another for
> +		 * display
> +		 */
> +		bufferCount = std::max(bufferCount, 2U);
> +
> +		ret = allocator_->allocate(stream, bufferCount);
>  		if (ret < 0) {
>  			qWarning() << "Failed to allocate capture buffers";
>  			goto error;
> diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> index 157ab94e0544..d01eacfa2b84 100644
> --- a/src/v4l2/v4l2_camera.cpp
> +++ b/src/v4l2/v4l2_camera.cpp
> @@ -161,7 +161,7 @@ int V4L2Camera::allocBuffers(unsigned int count)
>  {
>  	Stream *stream = config_->at(0).stream();
>  
> -	int ret = bufferAllocator_->allocate(stream);
> +	int ret = bufferAllocator_->allocate(stream, count);
>  	if (ret < 0)
>  		return ret;
>  
> diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
> index 238d98dbba16..2a531805cb2b 100644
> --- a/test/camera/capture.cpp
> +++ b/test/camera/capture.cpp
> @@ -8,6 +8,7 @@
>  #include <iostream>
>  
>  #include <libcamera/framebuffer_allocator.h>
> +#include <libcamera/property_ids.h>
>  
>  #include <libcamera/base/event_dispatcher.h>
>  #include <libcamera/base/thread.h>
> @@ -96,7 +97,8 @@ protected:
>  
>  		Stream *stream = cfg.stream();
>  
> -		int ret = allocator_->allocate(stream);
> +		unsigned int bufferCount = camera_->properties().get(properties::MinimumRequests);
> +		int ret = allocator_->allocate(stream, bufferCount);
>  		if (ret < 0)
>  			return TestFail;
>  
> diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp
> index 26fb5ca17139..70015bb56c74 100644
> --- a/test/camera/statemachine.cpp
> +++ b/test/camera/statemachine.cpp
> @@ -8,6 +8,7 @@
>  #include <iostream>
>  
>  #include <libcamera/framebuffer_allocator.h>
> +#include <libcamera/property_ids.h>
>  
>  #include "camera_test.h"
>  #include "test.h"
> @@ -119,7 +120,8 @@ protected:
>  		/* Use internally allocated buffers. */
>  		allocator_ = new FrameBufferAllocator(camera_);
>  		Stream *stream = *camera_->streams().begin();
> -		if (allocator_->allocate(stream) < 0)
> +		unsigned int bufferCount = camera_->properties().get(properties::MinimumRequests);
> +		if (allocator_->allocate(stream, bufferCount) < 0)
>  			return TestFail;
>  
>  		if (camera_->start())
> diff --git a/test/mapped-buffer.cpp b/test/mapped-buffer.cpp
> index c9479194cb68..e01211965364 100644
> --- a/test/mapped-buffer.cpp
> +++ b/test/mapped-buffer.cpp
> @@ -8,6 +8,7 @@
>  #include <iostream>
>  
>  #include <libcamera/framebuffer_allocator.h>
> +#include <libcamera/property_ids.h>
>  
>  #include "libcamera/internal/framebuffer.h"
>  
> @@ -54,7 +55,8 @@ protected:
>  
>  		stream_ = cfg.stream();
>  
> -		int ret = allocator_->allocate(stream_);
> +		unsigned int bufferCount = camera_->properties().get(properties::MinimumRequests);
> +		int ret = allocator_->allocate(stream_, bufferCount);
>  		if (ret < 0)
>  			return TestFail;
>  
>

Hello,

On Wed, Jul 28, 2021 at 09:28:36AM +0100, Kieran Bingham wrote:
> On 23/07/2021 00:28, Nícolas F. R. A. Prado wrote:
> > Make FrameBufferAllocator::allocate() require a 'count' argument for the
> > number of buffers to be allocated.
> > 
> > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> > ---
> > 
> > No changes in v7
> > 
> > Changes in v6:
> > - Changed static_cast to convert 'count' to unsigned int instead of
> >   'bufferCount' to int when comparing
> > 
> > Changes in v5:
> > - Made sure that qcam allocates at least 2 buffers
> > 
> >  include/libcamera/camera.h                         |  2 +-
> >  include/libcamera/framebuffer_allocator.h          |  2 +-
> >  include/libcamera/internal/pipeline_handler.h      |  2 +-
> >  src/android/camera_stream.cpp                      |  5 ++++-
> >  src/cam/capture.cpp                                |  9 +++------
> >  src/gstreamer/gstlibcameraallocator.cpp            |  4 +++-
> >  src/lc-compliance/simple_capture.cpp               |  7 +++++--
> >  src/libcamera/camera.cpp                           |  4 ++--
> >  src/libcamera/framebuffer_allocator.cpp            |  5 +++--
> >  src/libcamera/pipeline/ipu3/ipu3.cpp               |  4 ++--
> >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  4 ++--
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp           |  4 ++--
> >  src/libcamera/pipeline/simple/simple.cpp           |  4 ++--
> >  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp       |  7 ++++---
> >  src/libcamera/pipeline/vimc/vimc.cpp               |  7 ++++---
> >  src/libcamera/pipeline_handler.cpp                 |  1 +
> >  src/qcam/main_window.cpp                           | 11 ++++++++++-
> >  src/v4l2/v4l2_camera.cpp                           |  2 +-
> >  test/camera/capture.cpp                            |  4 +++-
> >  test/camera/statemachine.cpp                       |  4 +++-
> >  test/mapped-buffer.cpp                             |  4 +++-
> >  21 files changed, 60 insertions(+), 36 deletions(-)
> 
> This patch should also update the Documentation/application-developer
> guide which references the public API in section
>   "Using the libcamera ``FrameBufferAllocator``"
> 
> It can be a patch on top if it helps, or squashed into this one.

I'm fine with either, it may be easier to keep it on top, but it should
be part of this series.

> With that, I think I quite like this change as it gives better
> flexibility to the application which otherwise might provide buffers
> elsewhere anyway.
> 
> With the Documentation fixed, or with that being prepared as a separate
> patch:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > index b081907e0cb1..9f1767e4c406 100644
> > --- a/include/libcamera/camera.h
> > +++ b/include/libcamera/camera.h
> > @@ -116,7 +116,7 @@ private:
> >  	void requestComplete(Request *request);
> >  
> >  	friend class FrameBufferAllocator;
> > -	int exportFrameBuffers(Stream *stream,
> > +	int exportFrameBuffers(Stream *stream, unsigned int count,
> >  			       std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> >  };
> >  
> > diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h
> > index cbc9ce101889..2d5a6e98e10c 100644
> > --- a/include/libcamera/framebuffer_allocator.h
> > +++ b/include/libcamera/framebuffer_allocator.h
> > @@ -25,7 +25,7 @@ public:
> >  	FrameBufferAllocator(std::shared_ptr<Camera> camera);
> >  	~FrameBufferAllocator();
> >  
> > -	int allocate(Stream *stream);
> > +	int allocate(Stream *stream, unsigned int count);
> >  	int free(Stream *stream);
> >  
> >  	bool allocated() const { return !buffers_.empty(); }
> > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> > index 9e2d65d6f2c5..0b4b2e4947c0 100644
> > --- a/include/libcamera/internal/pipeline_handler.h
> > +++ b/include/libcamera/internal/pipeline_handler.h
> > @@ -76,7 +76,7 @@ public:
> >  		const StreamRoles &roles) = 0;
> >  	virtual int configure(Camera *camera, CameraConfiguration *config) = 0;
> >  
> > -	virtual int exportFrameBuffers(Camera *camera, Stream *stream,
> > +	virtual int exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,
> >  				       std::vector<std::unique_ptr<FrameBuffer>> *buffers) = 0;
> >  
> >  	virtual int start(Camera *camera, const ControlList *controls) = 0;
> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> > index bf4a7b41a70a..b168e3c0c288 100644
> > --- a/src/android/camera_stream.cpp
> > +++ b/src/android/camera_stream.cpp
> > @@ -13,6 +13,7 @@
> >  #include "jpeg/post_processor_jpeg.h"
> >  
> >  #include <libcamera/formats.h>
> > +#include <libcamera/property_ids.h>
> >  
> >  using namespace libcamera;
> >  
> > @@ -81,8 +82,10 @@ int CameraStream::configure()
> >  			return ret;
> >  	}
> >  
> > +	unsigned int bufferCount = cameraDevice_->camera()->properties().get(properties::MinimumRequests);
> > +
> >  	if (allocator_) {
> > -		int ret = allocator_->allocate(stream());
> > +		int ret = allocator_->allocate(stream(), bufferCount);
> >  		if (ret < 0)
> >  			return ret;
> >  
> > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
> > index 3c3e3a53adf7..633f9c6e4f25 100644
> > --- a/src/cam/capture.cpp
> > +++ b/src/cam/capture.cpp
> > @@ -11,6 +11,7 @@
> >  #include <sstream>
> >  
> >  #include <libcamera/control_ids.h>
> > +#include <libcamera/property_ids.h>
> >  
> >  #include "capture.h"
> >  #include "main.h"
> > @@ -81,17 +82,13 @@ int Capture::capture(FrameBufferAllocator *allocator)
> >  {
> >  	int ret;
> >  
> > -	/* Identify the stream with the least number of buffers. */
> > -	unsigned int nbuffers = UINT_MAX;
> > +	unsigned int nbuffers = camera_->properties().get(properties::MinimumRequests);
> >  	for (StreamConfiguration &cfg : *config_) {
> > -		ret = allocator->allocate(cfg.stream());
> > +		ret = allocator->allocate(cfg.stream(), nbuffers);

Seems that MinimumRequests should be a per-stream property (which we
don't support yet). Could you add a

	\todo Should this be a per-stream property?

to patch 01/11 ?

> >  		if (ret < 0) {
> >  			std::cerr << "Can't allocate buffers" << std::endl;
> >  			return -ENOMEM;
> >  		}
> > -
> > -		unsigned int allocated = allocator->buffers(cfg.stream()).size();
> > -		nbuffers = std::min(nbuffers, allocated);

Given that the allocator could still return less buffers than requested,
shouldn't we keep this code ? Or are you relying on the (undocumented)
assumption that the allocator will always be able to allocate at least
MinimumRequests ?

> >  	}
> >  
> >  	/*
> > diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp
> > index 7bd8ba2db71d..8cc42edfaf61 100644
> > --- a/src/gstreamer/gstlibcameraallocator.cpp
> > +++ b/src/gstreamer/gstlibcameraallocator.cpp
> > @@ -10,6 +10,7 @@
> >  
> >  #include <libcamera/camera.h>
> >  #include <libcamera/framebuffer_allocator.h>
> > +#include <libcamera/property_ids.h>
> >  #include <libcamera/stream.h>
> >  
> >  #include "gstlibcamera-utils.h"
> > @@ -188,13 +189,14 @@ gst_libcamera_allocator_new(std::shared_ptr<Camera> camera,
> >  {
> >  	auto *self = GST_LIBCAMERA_ALLOCATOR(g_object_new(GST_TYPE_LIBCAMERA_ALLOCATOR,
> >  							  nullptr));
> > +	unsigned int bufferCount = camera->properties().get(properties::MinimumRequests);
> >  
> >  	self->fb_allocator = new FrameBufferAllocator(camera);
> >  	for (StreamConfiguration &streamCfg : *config_) {
> >  		Stream *stream = streamCfg.stream();
> >  		gint ret;
> >  
> > -		ret = self->fb_allocator->allocate(stream);
> > +		ret = self->fb_allocator->allocate(stream, bufferCount);
> >  		if (ret == 0)
> >  			return nullptr;
> >  
> > diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp
> > index 25097f28a603..6444203547be 100644
> > --- a/src/lc-compliance/simple_capture.cpp
> > +++ b/src/lc-compliance/simple_capture.cpp
> > @@ -7,6 +7,8 @@
> >  
> >  #include <gtest/gtest.h>
> >  
> > +#include <libcamera/property_ids.h>
> > +
> >  #include "simple_capture.h"
> >  
> >  using namespace libcamera;
> > @@ -44,11 +46,12 @@ void SimpleCapture::configure(StreamRole role)
> >  
> >  void SimpleCapture::start()
> >  {
> > +	unsigned int bufferCount = camera_->properties().get(properties::MinNumRequests);
> >  	Stream *stream = config_->at(0).stream();
> > -	int count = allocator_->allocate(stream);
> > +	int count = allocator_->allocate(stream, bufferCount);
> >  
> >  	ASSERT_GE(count, 0) << "Failed to allocate buffers";
> > -	EXPECT_EQ(count, config_->at(0).bufferCount) << "Allocated less buffers than expected";
> > +	EXPECT_EQ(static_cast<unsigned int>(count), bufferCount) << "Allocated less buffers than expected";

Is the cast required ? bufferCount is currently an unsigned int.

> >  
> >  	camera_->requestCompleted.connect(this, &SimpleCapture::requestComplete);
> >  
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index c8858e71d105..e5a55a674afb 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -660,7 +660,7 @@ void Camera::disconnect()
> >  	disconnected.emit(this);
> >  }
> >  
> > -int Camera::exportFrameBuffers(Stream *stream,
> > +int Camera::exportFrameBuffers(Stream *stream, unsigned int count,
> >  			       std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> >  {
> >  	Private *const d = _d();
> > @@ -677,7 +677,7 @@ int Camera::exportFrameBuffers(Stream *stream,
> >  
> >  	return d->pipe_->invokeMethod(&PipelineHandler::exportFrameBuffers,
> >  				      ConnectionTypeBlocking, this, stream,
> > -				      buffers);
> > +				      count, buffers);
> >  }
> >  
> >  /**
> > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp
> > index 695073fd1c1c..2fc0db56176a 100644
> > --- a/src/libcamera/framebuffer_allocator.cpp
> > +++ b/src/libcamera/framebuffer_allocator.cpp
> > @@ -71,6 +71,7 @@ FrameBufferAllocator::~FrameBufferAllocator()
> >  /**
> >   * \brief Allocate buffers for a configured stream
> >   * \param[in] stream The stream to allocate buffers for
> > + * \param[in] count The number of buffers to allocate
> >   *
> >   * Allocate buffers suitable for capturing frames from the \a stream. The Camera
> >   * shall have been previously configured with Camera::configure() and shall be

Could you add a sentence to indicate that the function can allocate less
buffers than requested ?

 * This function may allocate less buffers than requested, due to memory and
 * other system constraints. The caller shall always check the return value to
 * verify if the number of allocate buffers matches its needs.

> > @@ -86,14 +87,14 @@ FrameBufferAllocator::~FrameBufferAllocator()
> >   * not part of the active camera configuration
> >   * \retval -EBUSY Buffers are already allocated for the \a stream
> >   */
> > -int FrameBufferAllocator::allocate(Stream *stream)
> > +int FrameBufferAllocator::allocate(Stream *stream, unsigned int count)
> >  {
> >  	if (buffers_.count(stream)) {
> >  		LOG(Allocator, Error) << "Buffers already allocated for stream";
> >  		return -EBUSY;
> >  	}
> >  
> > -	int ret = camera_->exportFrameBuffers(stream, &buffers_[stream]);
> > +	int ret = camera_->exportFrameBuffers(stream, count, &buffers_[stream]);
> >  	if (ret == -EINVAL)
> >  		LOG(Allocator, Error)
> >  			<< "Stream is not part of " << camera_->id()
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 76c3bb3d8aa9..5fd1757bfe13 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -134,7 +134,7 @@ public:
> >  		const StreamRoles &roles) override;
> >  	int configure(Camera *camera, CameraConfiguration *config) override;
> >  
> > -	int exportFrameBuffers(Camera *camera, Stream *stream,
> > +	int exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,
> >  			       std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> >  
> >  	int start(Camera *camera, const ControlList *controls) override;
> > @@ -654,10 +654,10 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> >  }
> >  
> >  int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,
> > +					    unsigned int count,
> >  					    std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> >  {
> >  	IPU3CameraData *data = cameraData(camera);
> > -	unsigned int count = stream->configuration().bufferCount;
> >  
> >  	if (stream == &data->outStream_)
> >  		return data->imgu_->output_->exportBuffers(count, buffers);
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index f821d8fe1b6c..d1cd3d9dc082 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -251,7 +251,7 @@ public:
> >  	CameraConfiguration *generateConfiguration(Camera *camera, const StreamRoles &roles) override;
> >  	int configure(Camera *camera, CameraConfiguration *config) override;
> >  
> > -	int exportFrameBuffers(Camera *camera, Stream *stream,
> > +	int exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,
> >  			       std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> >  
> >  	int start(Camera *camera, const ControlList *controls) override;
> > @@ -795,10 +795,10 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> >  }
> >  
> >  int PipelineHandlerRPi::exportFrameBuffers([[maybe_unused]] Camera *camera, Stream *stream,
> > +					   unsigned int count,
> >  					   std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> >  {
> >  	RPi::Stream *s = static_cast<RPi::Stream *>(stream);
> > -	unsigned int count = stream->configuration().bufferCount;
> >  	int ret = s->dev()->exportBuffers(count, buffers);
> >  
> >  	s->setExportedBuffers(buffers);
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index cb4ca9a85fa8..11325875b929 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -141,7 +141,7 @@ public:
> >  		const StreamRoles &roles) override;
> >  	int configure(Camera *camera, CameraConfiguration *config) override;
> >  
> > -	int exportFrameBuffers(Camera *camera, Stream *stream,
> > +	int exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,
> >  			       std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> >  
> >  	int start(Camera *camera, const ControlList *controls) override;
> > @@ -671,10 +671,10 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
> >  }
> >  
> >  int PipelineHandlerRkISP1::exportFrameBuffers([[maybe_unused]] Camera *camera, Stream *stream,
> > +					      unsigned int count,
> >  					      std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> >  {
> >  	RkISP1CameraData *data = cameraData(camera);
> > -	unsigned int count = stream->configuration().bufferCount;
> >  
> >  	if (stream == &data->mainPathStream_)
> >  		return mainPath_.exportBuffers(count, buffers);
> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > index 348c554c8be4..1c25a7344f5f 100644
> > --- a/src/libcamera/pipeline/simple/simple.cpp
> > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > @@ -228,7 +228,7 @@ public:
> >  						   const StreamRoles &roles) override;
> >  	int configure(Camera *camera, CameraConfiguration *config) override;
> >  
> > -	int exportFrameBuffers(Camera *camera, Stream *stream,
> > +	int exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,
> >  			       std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> >  
> >  	int start(Camera *camera, const ControlList *controls) override;
> > @@ -776,10 +776,10 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
> >  }
> >  
> >  int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream,
> > +					      unsigned int count,
> >  					      std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> >  {
> >  	SimpleCameraData *data = cameraData(camera);
> > -	unsigned int count = stream->configuration().bufferCount;
> >  
> >  	/*
> >  	 * Export buffers on the converter or capture video node, depending on
> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > index 6ad7fb3c9f0c..fd39b3d3c72c 100644
> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > @@ -69,7 +69,7 @@ public:
> >  		const StreamRoles &roles) override;
> >  	int configure(Camera *camera, CameraConfiguration *config) override;
> >  
> > -	int exportFrameBuffers(Camera *camera, Stream *stream,
> > +	int exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,
> >  			       std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> >  
> >  	int start(Camera *camera, const ControlList *controls) override;
> > @@ -223,11 +223,12 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config)
> >  	return 0;
> >  }
> >  
> > -int PipelineHandlerUVC::exportFrameBuffers(Camera *camera, Stream *stream,
> > +int PipelineHandlerUVC::exportFrameBuffers(Camera *camera,
> > +					   [[maybe_unused]] Stream *stream,
> > +					   unsigned int count,
> >  					   std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> >  {
> >  	UVCCameraData *data = cameraData(camera);
> > -	unsigned int count = stream->configuration().bufferCount;
> >  
> >  	return data->video_->exportBuffers(count, buffers);
> >  }
> > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> > index 44c198ccf8b8..e89d53182c6d 100644
> > --- a/src/libcamera/pipeline/vimc/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> > @@ -84,7 +84,7 @@ public:
> >  		const StreamRoles &roles) override;
> >  	int configure(Camera *camera, CameraConfiguration *config) override;
> >  
> > -	int exportFrameBuffers(Camera *camera, Stream *stream,
> > +	int exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,
> >  			       std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> >  
> >  	int start(Camera *camera, const ControlList *controls) override;
> > @@ -299,11 +299,12 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)
> >  	return 0;
> >  }
> >  
> > -int PipelineHandlerVimc::exportFrameBuffers(Camera *camera, Stream *stream,
> > +int PipelineHandlerVimc::exportFrameBuffers(Camera *camera,
> > +					    [[maybe_unused]] Stream *stream,
> > +					    unsigned int count,
> >  					    std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> >  {
> >  	VimcCameraData *data = cameraData(camera);
> > -	unsigned int count = stream->configuration().bufferCount;
> >  
> >  	return data->video_->exportBuffers(count, buffers);
> >  }
> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > index c9928d444b04..dc83bf6924bf 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -323,6 +323,7 @@ const ControlList &PipelineHandler::properties(const Camera *camera) const
> >   * \brief Allocate and export buffers for \a stream
> >   * \param[in] camera The camera
> >   * \param[in] stream The stream to allocate buffers for
> > + * \param[in] count The number of buffers to allocate
> >   * \param[out] buffers Array of buffers successfully allocated
> >   *
> >   * This method allocates buffers for the \a stream from the devices associated
> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> > index 39d034de6bb2..d7475b142d6f 100644
> > --- a/src/qcam/main_window.cpp
> > +++ b/src/qcam/main_window.cpp
> > @@ -25,6 +25,7 @@
> >  #include <QtDebug>
> >  
> >  #include <libcamera/camera_manager.h>
> > +#include <libcamera/property_ids.h>
> >  #include <libcamera/version.h>
> >  
> >  #include "dng_writer.h"
> > @@ -463,7 +464,15 @@ int MainWindow::startCapture()
> >  	for (StreamConfiguration &config : *config_) {
> >  		Stream *stream = config.stream();
> >  
> > -		ret = allocator_->allocate(stream);
> > +		unsigned int bufferCount = camera_->properties().get(properties::MinimumRequests);
> > +
> > +		/*
> > +		 * Need at least two buffers, one for capture and another for
> > +		 * display

s/display/display./

> > +		 */
> > +		bufferCount = std::max(bufferCount, 2U);

This requires inclusion of <cmath>.

I think we should use bufferCount + 1, not max(bufferCount, 2). qcam
holds on one buffer for display. If the camera needs, let's say, 2
buffers to be able to capture, it will return a first buffer which will
then be queued to the display. At that point, the camera will have a
single buffer, and will hold onto it until a new buffer is queued. This
will never happen, as the buffer being displayed won't be returned to
libcamera until a new buffer is ready. We thus need three buffers in
that case.

Thinking even more about this, I think we need

		bufferCount = std::max(bufferCount + 1, 3U);

Even if the camera needs a single buffer only (in which case we would
use two buffers, one for capture and one for display), it's clear that
it will lead to frame drops. A minimum of 3 buffers is thus better.

The cam application should also have a similar logic. It doesn't hold
onto a buffer, so in that case 

		bufferCount = std::max(bufferCount, 2U);

should be good enough to start with.

> > +
> > +		ret = allocator_->allocate(stream, bufferCount);
> >  		if (ret < 0) {
> >  			qWarning() << "Failed to allocate capture buffers";
> >  			goto error;
> > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> > index 157ab94e0544..d01eacfa2b84 100644
> > --- a/src/v4l2/v4l2_camera.cpp
> > +++ b/src/v4l2/v4l2_camera.cpp
> > @@ -161,7 +161,7 @@ int V4L2Camera::allocBuffers(unsigned int count)
> >  {
> >  	Stream *stream = config_->at(0).stream();
> >  
> > -	int ret = bufferAllocator_->allocate(stream);
> > +	int ret = bufferAllocator_->allocate(stream, count);
> >  	if (ret < 0)
> >  		return ret;
> >  
> > diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
> > index 238d98dbba16..2a531805cb2b 100644
> > --- a/test/camera/capture.cpp
> > +++ b/test/camera/capture.cpp
> > @@ -8,6 +8,7 @@
> >  #include <iostream>
> >  
> >  #include <libcamera/framebuffer_allocator.h>
> > +#include <libcamera/property_ids.h>
> >  
> >  #include <libcamera/base/event_dispatcher.h>
> >  #include <libcamera/base/thread.h>
> > @@ -96,7 +97,8 @@ protected:
> >  
> >  		Stream *stream = cfg.stream();
> >  
> > -		int ret = allocator_->allocate(stream);
> > +		unsigned int bufferCount = camera_->properties().get(properties::MinimumRequests);
> > +		int ret = allocator_->allocate(stream, bufferCount);

Similarly, this will result in frame drop at least with some pipeline
handlers. I fear we need to go one step further and already design the
API to tell applications how many buffers to allocate to avoid frame
draps. Per-frame control can be left out for now.

> >  		if (ret < 0)
> >  			return TestFail;
> >  
> > diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp
> > index 26fb5ca17139..70015bb56c74 100644
> > --- a/test/camera/statemachine.cpp
> > +++ b/test/camera/statemachine.cpp
> > @@ -8,6 +8,7 @@
> >  #include <iostream>
> >  
> >  #include <libcamera/framebuffer_allocator.h>
> > +#include <libcamera/property_ids.h>
> >  
> >  #include "camera_test.h"
> >  #include "test.h"
> > @@ -119,7 +120,8 @@ protected:
> >  		/* Use internally allocated buffers. */
> >  		allocator_ = new FrameBufferAllocator(camera_);
> >  		Stream *stream = *camera_->streams().begin();
> > -		if (allocator_->allocate(stream) < 0)
> > +		unsigned int bufferCount = camera_->properties().get(properties::MinimumRequests);
> > +		if (allocator_->allocate(stream, bufferCount) < 0)
> >  			return TestFail;
> >  
> >  		if (camera_->start())
> > diff --git a/test/mapped-buffer.cpp b/test/mapped-buffer.cpp
> > index c9479194cb68..e01211965364 100644
> > --- a/test/mapped-buffer.cpp
> > +++ b/test/mapped-buffer.cpp
> > @@ -8,6 +8,7 @@
> >  #include <iostream>
> >  
> >  #include <libcamera/framebuffer_allocator.h>
> > +#include <libcamera/property_ids.h>
> >  
> >  #include "libcamera/internal/framebuffer.h"
> >  
> > @@ -54,7 +55,8 @@ protected:
> >  
> >  		stream_ = cfg.stream();
> >  
> > -		int ret = allocator_->allocate(stream_);
> > +		unsigned int bufferCount = camera_->properties().get(properties::MinimumRequests);
> > +		int ret = allocator_->allocate(stream_, bufferCount);
> >  		if (ret < 0)
> >  			return TestFail;
> >  
> >

Another comment:

On Sun, Aug 01, 2021 at 08:23:55PM +0300, Laurent Pinchart wrote:
> On Wed, Jul 28, 2021 at 09:28:36AM +0100, Kieran Bingham wrote:
> > On 23/07/2021 00:28, Nícolas F. R. A. Prado wrote:
> > > Make FrameBufferAllocator::allocate() require a 'count' argument for the
> > > number of buffers to be allocated.
> > > 
> > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> > > ---
> > > 
> > > No changes in v7
> > > 
> > > Changes in v6:
> > > - Changed static_cast to convert 'count' to unsigned int instead of
> > >   'bufferCount' to int when comparing
> > > 
> > > Changes in v5:
> > > - Made sure that qcam allocates at least 2 buffers
> > > 
> > >  include/libcamera/camera.h                         |  2 +-
> > >  include/libcamera/framebuffer_allocator.h          |  2 +-
> > >  include/libcamera/internal/pipeline_handler.h      |  2 +-
> > >  src/android/camera_stream.cpp                      |  5 ++++-
> > >  src/cam/capture.cpp                                |  9 +++------
> > >  src/gstreamer/gstlibcameraallocator.cpp            |  4 +++-
> > >  src/lc-compliance/simple_capture.cpp               |  7 +++++--
> > >  src/libcamera/camera.cpp                           |  4 ++--
> > >  src/libcamera/framebuffer_allocator.cpp            |  5 +++--
> > >  src/libcamera/pipeline/ipu3/ipu3.cpp               |  4 ++--
> > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  4 ++--
> > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp           |  4 ++--
> > >  src/libcamera/pipeline/simple/simple.cpp           |  4 ++--
> > >  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp       |  7 ++++---
> > >  src/libcamera/pipeline/vimc/vimc.cpp               |  7 ++++---
> > >  src/libcamera/pipeline_handler.cpp                 |  1 +
> > >  src/qcam/main_window.cpp                           | 11 ++++++++++-
> > >  src/v4l2/v4l2_camera.cpp                           |  2 +-
> > >  test/camera/capture.cpp                            |  4 +++-
> > >  test/camera/statemachine.cpp                       |  4 +++-
> > >  test/mapped-buffer.cpp                             |  4 +++-
> > >  21 files changed, 60 insertions(+), 36 deletions(-)
> > 
> > This patch should also update the Documentation/application-developer
> > guide which references the public API in section
> >   "Using the libcamera ``FrameBufferAllocator``"
> > 
> > It can be a patch on top if it helps, or squashed into this one.
> 
> I'm fine with either, it may be easier to keep it on top, but it should
> be part of this series.
> 
> > With that, I think I quite like this change as it gives better
> > flexibility to the application which otherwise might provide buffers
> > elsewhere anyway.
> > 
> > With the Documentation fixed, or with that being prepared as a separate
> > patch:
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > > index b081907e0cb1..9f1767e4c406 100644
> > > --- a/include/libcamera/camera.h
> > > +++ b/include/libcamera/camera.h
> > > @@ -116,7 +116,7 @@ private:
> > >  	void requestComplete(Request *request);
> > >  
> > >  	friend class FrameBufferAllocator;
> > > -	int exportFrameBuffers(Stream *stream,
> > > +	int exportFrameBuffers(Stream *stream, unsigned int count,
> > >  			       std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> > >  };
> > >  
> > > diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h
> > > index cbc9ce101889..2d5a6e98e10c 100644
> > > --- a/include/libcamera/framebuffer_allocator.h
> > > +++ b/include/libcamera/framebuffer_allocator.h
> > > @@ -25,7 +25,7 @@ public:
> > >  	FrameBufferAllocator(std::shared_ptr<Camera> camera);
> > >  	~FrameBufferAllocator();
> > >  
> > > -	int allocate(Stream *stream);
> > > +	int allocate(Stream *stream, unsigned int count);
> > >  	int free(Stream *stream);
> > >  
> > >  	bool allocated() const { return !buffers_.empty(); }
> > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> > > index 9e2d65d6f2c5..0b4b2e4947c0 100644
> > > --- a/include/libcamera/internal/pipeline_handler.h
> > > +++ b/include/libcamera/internal/pipeline_handler.h
> > > @@ -76,7 +76,7 @@ public:
> > >  		const StreamRoles &roles) = 0;
> > >  	virtual int configure(Camera *camera, CameraConfiguration *config) = 0;
> > >  
> > > -	virtual int exportFrameBuffers(Camera *camera, Stream *stream,
> > > +	virtual int exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,
> > >  				       std::vector<std::unique_ptr<FrameBuffer>> *buffers) = 0;
> > >  
> > >  	virtual int start(Camera *camera, const ControlList *controls) = 0;
> > > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> > > index bf4a7b41a70a..b168e3c0c288 100644
> > > --- a/src/android/camera_stream.cpp
> > > +++ b/src/android/camera_stream.cpp
> > > @@ -13,6 +13,7 @@
> > >  #include "jpeg/post_processor_jpeg.h"
> > >  
> > >  #include <libcamera/formats.h>
> > > +#include <libcamera/property_ids.h>
> > >  
> > >  using namespace libcamera;
> > >  
> > > @@ -81,8 +82,10 @@ int CameraStream::configure()
> > >  			return ret;
> > >  	}
> > >  
> > > +	unsigned int bufferCount = cameraDevice_->camera()->properties().get(properties::MinimumRequests);
> > > +
> > >  	if (allocator_) {
> > > -		int ret = allocator_->allocate(stream());
> > > +		int ret = allocator_->allocate(stream(), bufferCount);
> > >  		if (ret < 0)
> > >  			return ret;
> > >  
> > > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
> > > index 3c3e3a53adf7..633f9c6e4f25 100644
> > > --- a/src/cam/capture.cpp
> > > +++ b/src/cam/capture.cpp
> > > @@ -11,6 +11,7 @@
> > >  #include <sstream>
> > >  
> > >  #include <libcamera/control_ids.h>
> > > +#include <libcamera/property_ids.h>
> > >  
> > >  #include "capture.h"
> > >  #include "main.h"
> > > @@ -81,17 +82,13 @@ int Capture::capture(FrameBufferAllocator *allocator)
> > >  {
> > >  	int ret;
> > >  
> > > -	/* Identify the stream with the least number of buffers. */
> > > -	unsigned int nbuffers = UINT_MAX;
> > > +	unsigned int nbuffers = camera_->properties().get(properties::MinimumRequests);
> > >  	for (StreamConfiguration &cfg : *config_) {
> > > -		ret = allocator->allocate(cfg.stream());
> > > +		ret = allocator->allocate(cfg.stream(), nbuffers);
> 
> Seems that MinimumRequests should be a per-stream property (which we
> don't support yet). Could you add a
> 
> 	\todo Should this be a per-stream property?
> 
> to patch 01/11 ?
> 
> > >  		if (ret < 0) {
> > >  			std::cerr << "Can't allocate buffers" << std::endl;
> > >  			return -ENOMEM;
> > >  		}
> > > -
> > > -		unsigned int allocated = allocator->buffers(cfg.stream()).size();
> > > -		nbuffers = std::min(nbuffers, allocated);
> 
> Given that the allocator could still return less buffers than requested,
> shouldn't we keep this code ? Or are you relying on the (undocumented)
> assumption that the allocator will always be able to allocate at least
> MinimumRequests ?
> 
> > >  	}
> > >  
> > >  	/*
> > > diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp
> > > index 7bd8ba2db71d..8cc42edfaf61 100644
> > > --- a/src/gstreamer/gstlibcameraallocator.cpp
> > > +++ b/src/gstreamer/gstlibcameraallocator.cpp
> > > @@ -10,6 +10,7 @@
> > >  
> > >  #include <libcamera/camera.h>
> > >  #include <libcamera/framebuffer_allocator.h>
> > > +#include <libcamera/property_ids.h>
> > >  #include <libcamera/stream.h>
> > >  
> > >  #include "gstlibcamera-utils.h"
> > > @@ -188,13 +189,14 @@ gst_libcamera_allocator_new(std::shared_ptr<Camera> camera,
> > >  {
> > >  	auto *self = GST_LIBCAMERA_ALLOCATOR(g_object_new(GST_TYPE_LIBCAMERA_ALLOCATOR,
> > >  							  nullptr));
> > > +	unsigned int bufferCount = camera->properties().get(properties::MinimumRequests);
> > >  
> > >  	self->fb_allocator = new FrameBufferAllocator(camera);
> > >  	for (StreamConfiguration &streamCfg : *config_) {
> > >  		Stream *stream = streamCfg.stream();
> > >  		gint ret;
> > >  
> > > -		ret = self->fb_allocator->allocate(stream);
> > > +		ret = self->fb_allocator->allocate(stream, bufferCount);
> > >  		if (ret == 0)
> > >  			return nullptr;
> > >  
> > > diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp
> > > index 25097f28a603..6444203547be 100644
> > > --- a/src/lc-compliance/simple_capture.cpp
> > > +++ b/src/lc-compliance/simple_capture.cpp
> > > @@ -7,6 +7,8 @@
> > >  
> > >  #include <gtest/gtest.h>
> > >  
> > > +#include <libcamera/property_ids.h>
> > > +
> > >  #include "simple_capture.h"
> > >  
> > >  using namespace libcamera;
> > > @@ -44,11 +46,12 @@ void SimpleCapture::configure(StreamRole role)
> > >  
> > >  void SimpleCapture::start()
> > >  {
> > > +	unsigned int bufferCount = camera_->properties().get(properties::MinNumRequests);

This should be MinimumRequests.

Nícolas, could you compile-test patches individually ?

> > >  	Stream *stream = config_->at(0).stream();
> > > -	int count = allocator_->allocate(stream);
> > > +	int count = allocator_->allocate(stream, bufferCount);
> > >  
> > >  	ASSERT_GE(count, 0) << "Failed to allocate buffers";
> > > -	EXPECT_EQ(count, config_->at(0).bufferCount) << "Allocated less buffers than expected";
> > > +	EXPECT_EQ(static_cast<unsigned int>(count), bufferCount) << "Allocated less buffers than expected";
> 
> Is the cast required ? bufferCount is currently an unsigned int.
> 
> > >  
> > >  	camera_->requestCompleted.connect(this, &SimpleCapture::requestComplete);
> > >  
> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > > index c8858e71d105..e5a55a674afb 100644
> > > --- a/src/libcamera/camera.cpp
> > > +++ b/src/libcamera/camera.cpp
> > > @@ -660,7 +660,7 @@ void Camera::disconnect()
> > >  	disconnected.emit(this);
> > >  }
> > >  
> > > -int Camera::exportFrameBuffers(Stream *stream,
> > > +int Camera::exportFrameBuffers(Stream *stream, unsigned int count,
> > >  			       std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> > >  {
> > >  	Private *const d = _d();
> > > @@ -677,7 +677,7 @@ int Camera::exportFrameBuffers(Stream *stream,
> > >  
> > >  	return d->pipe_->invokeMethod(&PipelineHandler::exportFrameBuffers,
> > >  				      ConnectionTypeBlocking, this, stream,
> > > -				      buffers);
> > > +				      count, buffers);
> > >  }
> > >  
> > >  /**
> > > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp
> > > index 695073fd1c1c..2fc0db56176a 100644
> > > --- a/src/libcamera/framebuffer_allocator.cpp
> > > +++ b/src/libcamera/framebuffer_allocator.cpp
> > > @@ -71,6 +71,7 @@ FrameBufferAllocator::~FrameBufferAllocator()
> > >  /**
> > >   * \brief Allocate buffers for a configured stream
> > >   * \param[in] stream The stream to allocate buffers for
> > > + * \param[in] count The number of buffers to allocate
> > >   *
> > >   * Allocate buffers suitable for capturing frames from the \a stream. The Camera
> > >   * shall have been previously configured with Camera::configure() and shall be
> 
> Could you add a sentence to indicate that the function can allocate less
> buffers than requested ?
> 
>  * This function may allocate less buffers than requested, due to memory and
>  * other system constraints. The caller shall always check the return value to
>  * verify if the number of allocate buffers matches its needs.
> 
> > > @@ -86,14 +87,14 @@ FrameBufferAllocator::~FrameBufferAllocator()
> > >   * not part of the active camera configuration
> > >   * \retval -EBUSY Buffers are already allocated for the \a stream
> > >   */
> > > -int FrameBufferAllocator::allocate(Stream *stream)
> > > +int FrameBufferAllocator::allocate(Stream *stream, unsigned int count)
> > >  {
> > >  	if (buffers_.count(stream)) {
> > >  		LOG(Allocator, Error) << "Buffers already allocated for stream";
> > >  		return -EBUSY;
> > >  	}
> > >  
> > > -	int ret = camera_->exportFrameBuffers(stream, &buffers_[stream]);
> > > +	int ret = camera_->exportFrameBuffers(stream, count, &buffers_[stream]);
> > >  	if (ret == -EINVAL)
> > >  		LOG(Allocator, Error)
> > >  			<< "Stream is not part of " << camera_->id()
> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > index 76c3bb3d8aa9..5fd1757bfe13 100644
> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > @@ -134,7 +134,7 @@ public:
> > >  		const StreamRoles &roles) override;
> > >  	int configure(Camera *camera, CameraConfiguration *config) override;
> > >  
> > > -	int exportFrameBuffers(Camera *camera, Stream *stream,
> > > +	int exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,
> > >  			       std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> > >  
> > >  	int start(Camera *camera, const ControlList *controls) override;
> > > @@ -654,10 +654,10 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> > >  }
> > >  
> > >  int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,
> > > +					    unsigned int count,
> > >  					    std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> > >  {
> > >  	IPU3CameraData *data = cameraData(camera);
> > > -	unsigned int count = stream->configuration().bufferCount;
> > >  
> > >  	if (stream == &data->outStream_)
> > >  		return data->imgu_->output_->exportBuffers(count, buffers);
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > index f821d8fe1b6c..d1cd3d9dc082 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > @@ -251,7 +251,7 @@ public:
> > >  	CameraConfiguration *generateConfiguration(Camera *camera, const StreamRoles &roles) override;
> > >  	int configure(Camera *camera, CameraConfiguration *config) override;
> > >  
> > > -	int exportFrameBuffers(Camera *camera, Stream *stream,
> > > +	int exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,
> > >  			       std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> > >  
> > >  	int start(Camera *camera, const ControlList *controls) override;
> > > @@ -795,10 +795,10 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> > >  }
> > >  
> > >  int PipelineHandlerRPi::exportFrameBuffers([[maybe_unused]] Camera *camera, Stream *stream,
> > > +					   unsigned int count,
> > >  					   std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> > >  {
> > >  	RPi::Stream *s = static_cast<RPi::Stream *>(stream);
> > > -	unsigned int count = stream->configuration().bufferCount;
> > >  	int ret = s->dev()->exportBuffers(count, buffers);
> > >  
> > >  	s->setExportedBuffers(buffers);
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > index cb4ca9a85fa8..11325875b929 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > @@ -141,7 +141,7 @@ public:
> > >  		const StreamRoles &roles) override;
> > >  	int configure(Camera *camera, CameraConfiguration *config) override;
> > >  
> > > -	int exportFrameBuffers(Camera *camera, Stream *stream,
> > > +	int exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,
> > >  			       std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> > >  
> > >  	int start(Camera *camera, const ControlList *controls) override;
> > > @@ -671,10 +671,10 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
> > >  }
> > >  
> > >  int PipelineHandlerRkISP1::exportFrameBuffers([[maybe_unused]] Camera *camera, Stream *stream,
> > > +					      unsigned int count,
> > >  					      std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> > >  {
> > >  	RkISP1CameraData *data = cameraData(camera);
> > > -	unsigned int count = stream->configuration().bufferCount;
> > >  
> > >  	if (stream == &data->mainPathStream_)
> > >  		return mainPath_.exportBuffers(count, buffers);
> > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > > index 348c554c8be4..1c25a7344f5f 100644
> > > --- a/src/libcamera/pipeline/simple/simple.cpp
> > > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > > @@ -228,7 +228,7 @@ public:
> > >  						   const StreamRoles &roles) override;
> > >  	int configure(Camera *camera, CameraConfiguration *config) override;
> > >  
> > > -	int exportFrameBuffers(Camera *camera, Stream *stream,
> > > +	int exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,
> > >  			       std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> > >  
> > >  	int start(Camera *camera, const ControlList *controls) override;
> > > @@ -776,10 +776,10 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
> > >  }
> > >  
> > >  int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream,
> > > +					      unsigned int count,
> > >  					      std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> > >  {
> > >  	SimpleCameraData *data = cameraData(camera);
> > > -	unsigned int count = stream->configuration().bufferCount;
> > >  
> > >  	/*
> > >  	 * Export buffers on the converter or capture video node, depending on
> > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > index 6ad7fb3c9f0c..fd39b3d3c72c 100644
> > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > @@ -69,7 +69,7 @@ public:
> > >  		const StreamRoles &roles) override;
> > >  	int configure(Camera *camera, CameraConfiguration *config) override;
> > >  
> > > -	int exportFrameBuffers(Camera *camera, Stream *stream,
> > > +	int exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,
> > >  			       std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> > >  
> > >  	int start(Camera *camera, const ControlList *controls) override;
> > > @@ -223,11 +223,12 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config)
> > >  	return 0;
> > >  }
> > >  
> > > -int PipelineHandlerUVC::exportFrameBuffers(Camera *camera, Stream *stream,
> > > +int PipelineHandlerUVC::exportFrameBuffers(Camera *camera,
> > > +					   [[maybe_unused]] Stream *stream,
> > > +					   unsigned int count,
> > >  					   std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> > >  {
> > >  	UVCCameraData *data = cameraData(camera);
> > > -	unsigned int count = stream->configuration().bufferCount;
> > >  
> > >  	return data->video_->exportBuffers(count, buffers);
> > >  }
> > > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> > > index 44c198ccf8b8..e89d53182c6d 100644
> > > --- a/src/libcamera/pipeline/vimc/vimc.cpp
> > > +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> > > @@ -84,7 +84,7 @@ public:
> > >  		const StreamRoles &roles) override;
> > >  	int configure(Camera *camera, CameraConfiguration *config) override;
> > >  
> > > -	int exportFrameBuffers(Camera *camera, Stream *stream,
> > > +	int exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,
> > >  			       std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> > >  
> > >  	int start(Camera *camera, const ControlList *controls) override;
> > > @@ -299,11 +299,12 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)
> > >  	return 0;
> > >  }
> > >  
> > > -int PipelineHandlerVimc::exportFrameBuffers(Camera *camera, Stream *stream,
> > > +int PipelineHandlerVimc::exportFrameBuffers(Camera *camera,
> > > +					    [[maybe_unused]] Stream *stream,
> > > +					    unsigned int count,
> > >  					    std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> > >  {
> > >  	VimcCameraData *data = cameraData(camera);
> > > -	unsigned int count = stream->configuration().bufferCount;
> > >  
> > >  	return data->video_->exportBuffers(count, buffers);
> > >  }
> > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > > index c9928d444b04..dc83bf6924bf 100644
> > > --- a/src/libcamera/pipeline_handler.cpp
> > > +++ b/src/libcamera/pipeline_handler.cpp
> > > @@ -323,6 +323,7 @@ const ControlList &PipelineHandler::properties(const Camera *camera) const
> > >   * \brief Allocate and export buffers for \a stream
> > >   * \param[in] camera The camera
> > >   * \param[in] stream The stream to allocate buffers for
> > > + * \param[in] count The number of buffers to allocate
> > >   * \param[out] buffers Array of buffers successfully allocated
> > >   *
> > >   * This method allocates buffers for the \a stream from the devices associated
> > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> > > index 39d034de6bb2..d7475b142d6f 100644
> > > --- a/src/qcam/main_window.cpp
> > > +++ b/src/qcam/main_window.cpp
> > > @@ -25,6 +25,7 @@
> > >  #include <QtDebug>
> > >  
> > >  #include <libcamera/camera_manager.h>
> > > +#include <libcamera/property_ids.h>
> > >  #include <libcamera/version.h>
> > >  
> > >  #include "dng_writer.h"
> > > @@ -463,7 +464,15 @@ int MainWindow::startCapture()
> > >  	for (StreamConfiguration &config : *config_) {
> > >  		Stream *stream = config.stream();
> > >  
> > > -		ret = allocator_->allocate(stream);
> > > +		unsigned int bufferCount = camera_->properties().get(properties::MinimumRequests);
> > > +
> > > +		/*
> > > +		 * Need at least two buffers, one for capture and another for
> > > +		 * display
> 
> s/display/display./
> 
> > > +		 */
> > > +		bufferCount = std::max(bufferCount, 2U);
> 
> This requires inclusion of <cmath>.
> 
> I think we should use bufferCount + 1, not max(bufferCount, 2). qcam
> holds on one buffer for display. If the camera needs, let's say, 2
> buffers to be able to capture, it will return a first buffer which will
> then be queued to the display. At that point, the camera will have a
> single buffer, and will hold onto it until a new buffer is queued. This
> will never happen, as the buffer being displayed won't be returned to
> libcamera until a new buffer is ready. We thus need three buffers in
> that case.
> 
> Thinking even more about this, I think we need
> 
> 		bufferCount = std::max(bufferCount + 1, 3U);
> 
> Even if the camera needs a single buffer only (in which case we would
> use two buffers, one for capture and one for display), it's clear that
> it will lead to frame drops. A minimum of 3 buffers is thus better.
> 
> The cam application should also have a similar logic. It doesn't hold
> onto a buffer, so in that case 
> 
> 		bufferCount = std::max(bufferCount, 2U);
> 
> should be good enough to start with.
> 
> > > +
> > > +		ret = allocator_->allocate(stream, bufferCount);
> > >  		if (ret < 0) {
> > >  			qWarning() << "Failed to allocate capture buffers";
> > >  			goto error;
> > > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> > > index 157ab94e0544..d01eacfa2b84 100644
> > > --- a/src/v4l2/v4l2_camera.cpp
> > > +++ b/src/v4l2/v4l2_camera.cpp
> > > @@ -161,7 +161,7 @@ int V4L2Camera::allocBuffers(unsigned int count)
> > >  {
> > >  	Stream *stream = config_->at(0).stream();
> > >  
> > > -	int ret = bufferAllocator_->allocate(stream);
> > > +	int ret = bufferAllocator_->allocate(stream, count);
> > >  	if (ret < 0)
> > >  		return ret;
> > >  
> > > diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
> > > index 238d98dbba16..2a531805cb2b 100644
> > > --- a/test/camera/capture.cpp
> > > +++ b/test/camera/capture.cpp
> > > @@ -8,6 +8,7 @@
> > >  #include <iostream>
> > >  
> > >  #include <libcamera/framebuffer_allocator.h>
> > > +#include <libcamera/property_ids.h>
> > >  
> > >  #include <libcamera/base/event_dispatcher.h>
> > >  #include <libcamera/base/thread.h>
> > > @@ -96,7 +97,8 @@ protected:
> > >  
> > >  		Stream *stream = cfg.stream();
> > >  
> > > -		int ret = allocator_->allocate(stream);
> > > +		unsigned int bufferCount = camera_->properties().get(properties::MinimumRequests);
> > > +		int ret = allocator_->allocate(stream, bufferCount);
> 
> Similarly, this will result in frame drop at least with some pipeline
> handlers. I fear we need to go one step further and already design the
> API to tell applications how many buffers to allocate to avoid frame
> draps. Per-frame control can be left out for now.
> 
> > >  		if (ret < 0)
> > >  			return TestFail;
> > >  
> > > diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp
> > > index 26fb5ca17139..70015bb56c74 100644
> > > --- a/test/camera/statemachine.cpp
> > > +++ b/test/camera/statemachine.cpp
> > > @@ -8,6 +8,7 @@
> > >  #include <iostream>
> > >  
> > >  #include <libcamera/framebuffer_allocator.h>
> > > +#include <libcamera/property_ids.h>
> > >  
> > >  #include "camera_test.h"
> > >  #include "test.h"
> > > @@ -119,7 +120,8 @@ protected:
> > >  		/* Use internally allocated buffers. */
> > >  		allocator_ = new FrameBufferAllocator(camera_);
> > >  		Stream *stream = *camera_->streams().begin();
> > > -		if (allocator_->allocate(stream) < 0)
> > > +		unsigned int bufferCount = camera_->properties().get(properties::MinimumRequests);
> > > +		if (allocator_->allocate(stream, bufferCount) < 0)
> > >  			return TestFail;
> > >  
> > >  		if (camera_->start())
> > > diff --git a/test/mapped-buffer.cpp b/test/mapped-buffer.cpp
> > > index c9479194cb68..e01211965364 100644
> > > --- a/test/mapped-buffer.cpp
> > > +++ b/test/mapped-buffer.cpp
> > > @@ -8,6 +8,7 @@
> > >  #include <iostream>
> > >  
> > >  #include <libcamera/framebuffer_allocator.h>
> > > +#include <libcamera/property_ids.h>
> > >  
> > >  #include "libcamera/internal/framebuffer.h"
> > >  
> > > @@ -54,7 +55,8 @@ protected:
> > >  
> > >  		stream_ = cfg.stream();
> > >  
> > > -		int ret = allocator_->allocate(stream_);
> > > +		unsigned int bufferCount = camera_->properties().get(properties::MinimumRequests);
> > > +		int ret = allocator_->allocate(stream_, bufferCount);
> > >  		if (ret < 0)
> > >  			return TestFail;
> > >  
> > >

Hi Laurent,

On Sun, Aug 01, 2021 at 08:23:54PM +0300, Laurent Pinchart wrote:
> Hello,
> 
> On Wed, Jul 28, 2021 at 09:28:36AM +0100, Kieran Bingham wrote:
> > On 23/07/2021 00:28, Nícolas F. R. A. Prado wrote:
> > > Make FrameBufferAllocator::allocate() require a 'count' argument for the
> > > number of buffers to be allocated.
> > > 
> > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> > > ---
> > > 
> > > No changes in v7
> > > 
> > > Changes in v6:
> > > - Changed static_cast to convert 'count' to unsigned int instead of
> > >   'bufferCount' to int when comparing
> > > 
> > > Changes in v5:
> > > - Made sure that qcam allocates at least 2 buffers
> > > 
> > >  include/libcamera/camera.h                         |  2 +-
> > >  include/libcamera/framebuffer_allocator.h          |  2 +-
> > >  include/libcamera/internal/pipeline_handler.h      |  2 +-
> > >  src/android/camera_stream.cpp                      |  5 ++++-
> > >  src/cam/capture.cpp                                |  9 +++------
> > >  src/gstreamer/gstlibcameraallocator.cpp            |  4 +++-
> > >  src/lc-compliance/simple_capture.cpp               |  7 +++++--
> > >  src/libcamera/camera.cpp                           |  4 ++--
> > >  src/libcamera/framebuffer_allocator.cpp            |  5 +++--
> > >  src/libcamera/pipeline/ipu3/ipu3.cpp               |  4 ++--
> > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp |  4 ++--
> > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp           |  4 ++--
> > >  src/libcamera/pipeline/simple/simple.cpp           |  4 ++--
> > >  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp       |  7 ++++---
> > >  src/libcamera/pipeline/vimc/vimc.cpp               |  7 ++++---
> > >  src/libcamera/pipeline_handler.cpp                 |  1 +
> > >  src/qcam/main_window.cpp                           | 11 ++++++++++-
> > >  src/v4l2/v4l2_camera.cpp                           |  2 +-
> > >  test/camera/capture.cpp                            |  4 +++-
> > >  test/camera/statemachine.cpp                       |  4 +++-
> > >  test/mapped-buffer.cpp                             |  4 +++-
> > >  21 files changed, 60 insertions(+), 36 deletions(-)
> > 
> > This patch should also update the Documentation/application-developer
> > guide which references the public API in section
> >   "Using the libcamera ``FrameBufferAllocator``"
> > 
> > It can be a patch on top if it helps, or squashed into this one.
> 
> I'm fine with either, it may be easier to keep it on top, but it should
> be part of this series.
> 
> > With that, I think I quite like this change as it gives better
> > flexibility to the application which otherwise might provide buffers
> > elsewhere anyway.
> > 
> > With the Documentation fixed, or with that being prepared as a separate
> > patch:
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > > index b081907e0cb1..9f1767e4c406 100644
> > > --- a/include/libcamera/camera.h
> > > +++ b/include/libcamera/camera.h
> > > @@ -116,7 +116,7 @@ private:
> > >  	void requestComplete(Request *request);
> > >  
> > >  	friend class FrameBufferAllocator;
> > > -	int exportFrameBuffers(Stream *stream,
> > > +	int exportFrameBuffers(Stream *stream, unsigned int count,
> > >  			       std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> > >  };
> > >  
> > > diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h
> > > index cbc9ce101889..2d5a6e98e10c 100644
> > > --- a/include/libcamera/framebuffer_allocator.h
> > > +++ b/include/libcamera/framebuffer_allocator.h
> > > @@ -25,7 +25,7 @@ public:
> > >  	FrameBufferAllocator(std::shared_ptr<Camera> camera);
> > >  	~FrameBufferAllocator();
> > >  
> > > -	int allocate(Stream *stream);
> > > +	int allocate(Stream *stream, unsigned int count);
> > >  	int free(Stream *stream);
> > >  
> > >  	bool allocated() const { return !buffers_.empty(); }
> > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> > > index 9e2d65d6f2c5..0b4b2e4947c0 100644
> > > --- a/include/libcamera/internal/pipeline_handler.h
> > > +++ b/include/libcamera/internal/pipeline_handler.h
> > > @@ -76,7 +76,7 @@ public:
> > >  		const StreamRoles &roles) = 0;
> > >  	virtual int configure(Camera *camera, CameraConfiguration *config) = 0;
> > >  
> > > -	virtual int exportFrameBuffers(Camera *camera, Stream *stream,
> > > +	virtual int exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,
> > >  				       std::vector<std::unique_ptr<FrameBuffer>> *buffers) = 0;
> > >  
> > >  	virtual int start(Camera *camera, const ControlList *controls) = 0;
> > > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> > > index bf4a7b41a70a..b168e3c0c288 100644
> > > --- a/src/android/camera_stream.cpp
> > > +++ b/src/android/camera_stream.cpp
> > > @@ -13,6 +13,7 @@
> > >  #include "jpeg/post_processor_jpeg.h"
> > >  
> > >  #include <libcamera/formats.h>
> > > +#include <libcamera/property_ids.h>
> > >  
> > >  using namespace libcamera;
> > >  
> > > @@ -81,8 +82,10 @@ int CameraStream::configure()
> > >  			return ret;
> > >  	}
> > >  
> > > +	unsigned int bufferCount = cameraDevice_->camera()->properties().get(properties::MinimumRequests);
> > > +
> > >  	if (allocator_) {
> > > -		int ret = allocator_->allocate(stream());
> > > +		int ret = allocator_->allocate(stream(), bufferCount);
> > >  		if (ret < 0)
> > >  			return ret;
> > >  
> > > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
> > > index 3c3e3a53adf7..633f9c6e4f25 100644
> > > --- a/src/cam/capture.cpp
> > > +++ b/src/cam/capture.cpp
> > > @@ -11,6 +11,7 @@
> > >  #include <sstream>
> > >  
> > >  #include <libcamera/control_ids.h>
> > > +#include <libcamera/property_ids.h>
> > >  
> > >  #include "capture.h"
> > >  #include "main.h"
> > > @@ -81,17 +82,13 @@ int Capture::capture(FrameBufferAllocator *allocator)
> > >  {
> > >  	int ret;
> > >  
> > > -	/* Identify the stream with the least number of buffers. */
> > > -	unsigned int nbuffers = UINT_MAX;
> > > +	unsigned int nbuffers = camera_->properties().get(properties::MinimumRequests);
> > >  	for (StreamConfiguration &cfg : *config_) {
> > > -		ret = allocator->allocate(cfg.stream());
> > > +		ret = allocator->allocate(cfg.stream(), nbuffers);
> 
> Seems that MinimumRequests should be a per-stream property (which we
> don't support yet). Could you add a
> 
> 	\todo Should this be a per-stream property?
> 
> to patch 01/11 ?
> 
> > >  		if (ret < 0) {
> > >  			std::cerr << "Can't allocate buffers" << std::endl;
> > >  			return -ENOMEM;
> > >  		}
> > > -
> > > -		unsigned int allocated = allocator->buffers(cfg.stream()).size();
> > > -		nbuffers = std::min(nbuffers, allocated);
> 
> Given that the allocator could still return less buffers than requested,
> shouldn't we keep this code ? Or are you relying on the (undocumented)
> assumption that the allocator will always be able to allocate at least
> MinimumRequests ?

Indeed I was since that's the current behavior, but yes, let's not assume that.
Instead of just keeping the current code, however, I'll also add check if
'allocated' is less than MinimumRequests, and bail out in that case, since the
pipeline won't work in that case anyway.

> 
> > >  	}
> > >  
> > >  	/*
> > > diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp
> > > index 7bd8ba2db71d..8cc42edfaf61 100644
> > > --- a/src/gstreamer/gstlibcameraallocator.cpp
> > > +++ b/src/gstreamer/gstlibcameraallocator.cpp
> > > @@ -10,6 +10,7 @@
> > >  
> > >  #include <libcamera/camera.h>
> > >  #include <libcamera/framebuffer_allocator.h>
> > > +#include <libcamera/property_ids.h>
> > >  #include <libcamera/stream.h>
> > >  
> > >  #include "gstlibcamera-utils.h"
> > > @@ -188,13 +189,14 @@ gst_libcamera_allocator_new(std::shared_ptr<Camera> camera,
> > >  {
> > >  	auto *self = GST_LIBCAMERA_ALLOCATOR(g_object_new(GST_TYPE_LIBCAMERA_ALLOCATOR,
> > >  							  nullptr));
> > > +	unsigned int bufferCount = camera->properties().get(properties::MinimumRequests);
> > >  
> > >  	self->fb_allocator = new FrameBufferAllocator(camera);
> > >  	for (StreamConfiguration &streamCfg : *config_) {
> > >  		Stream *stream = streamCfg.stream();
> > >  		gint ret;
> > >  
> > > -		ret = self->fb_allocator->allocate(stream);
> > > +		ret = self->fb_allocator->allocate(stream, bufferCount);
> > >  		if (ret == 0)
> > >  			return nullptr;
> > >  
> > > diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp
> > > index 25097f28a603..6444203547be 100644
> > > --- a/src/lc-compliance/simple_capture.cpp
> > > +++ b/src/lc-compliance/simple_capture.cpp
> > > @@ -7,6 +7,8 @@
> > >  
> > >  #include <gtest/gtest.h>
> > >  
> > > +#include <libcamera/property_ids.h>
> > > +
> > >  #include "simple_capture.h"
> > >  
> > >  using namespace libcamera;
> > > @@ -44,11 +46,12 @@ void SimpleCapture::configure(StreamRole role)
> > >  
> > >  void SimpleCapture::start()
> > >  {
> > > +	unsigned int bufferCount = camera_->properties().get(properties::MinNumRequests);
> > >  	Stream *stream = config_->at(0).stream();
> > > -	int count = allocator_->allocate(stream);
> > > +	int count = allocator_->allocate(stream, bufferCount);
> > >  
> > >  	ASSERT_GE(count, 0) << "Failed to allocate buffers";
> > > -	EXPECT_EQ(count, config_->at(0).bufferCount) << "Allocated less buffers than expected";
> > > +	EXPECT_EQ(static_cast<unsigned int>(count), bufferCount) << "Allocated less buffers than expected";
> 
> Is the cast required ? bufferCount is currently an unsigned int.
> 
> > >  
> > >  	camera_->requestCompleted.connect(this, &SimpleCapture::requestComplete);
> > >  
> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > > index c8858e71d105..e5a55a674afb 100644
> > > --- a/src/libcamera/camera.cpp
> > > +++ b/src/libcamera/camera.cpp
> > > @@ -660,7 +660,7 @@ void Camera::disconnect()
> > >  	disconnected.emit(this);
> > >  }
> > >  
> > > -int Camera::exportFrameBuffers(Stream *stream,
> > > +int Camera::exportFrameBuffers(Stream *stream, unsigned int count,
> > >  			       std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> > >  {
> > >  	Private *const d = _d();
> > > @@ -677,7 +677,7 @@ int Camera::exportFrameBuffers(Stream *stream,
> > >  
> > >  	return d->pipe_->invokeMethod(&PipelineHandler::exportFrameBuffers,
> > >  				      ConnectionTypeBlocking, this, stream,
> > > -				      buffers);
> > > +				      count, buffers);
> > >  }
> > >  
> > >  /**
> > > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp
> > > index 695073fd1c1c..2fc0db56176a 100644
> > > --- a/src/libcamera/framebuffer_allocator.cpp
> > > +++ b/src/libcamera/framebuffer_allocator.cpp
> > > @@ -71,6 +71,7 @@ FrameBufferAllocator::~FrameBufferAllocator()
> > >  /**
> > >   * \brief Allocate buffers for a configured stream
> > >   * \param[in] stream The stream to allocate buffers for
> > > + * \param[in] count The number of buffers to allocate
> > >   *
> > >   * Allocate buffers suitable for capturing frames from the \a stream. The Camera
> > >   * shall have been previously configured with Camera::configure() and shall be
> 
> Could you add a sentence to indicate that the function can allocate less
> buffers than requested ?
> 
>  * This function may allocate less buffers than requested, due to memory and
>  * other system constraints. The caller shall always check the return value to
>  * verify if the number of allocate buffers matches its needs.
> 
> > > @@ -86,14 +87,14 @@ FrameBufferAllocator::~FrameBufferAllocator()
> > >   * not part of the active camera configuration
> > >   * \retval -EBUSY Buffers are already allocated for the \a stream
> > >   */
> > > -int FrameBufferAllocator::allocate(Stream *stream)
> > > +int FrameBufferAllocator::allocate(Stream *stream, unsigned int count)
> > >  {
> > >  	if (buffers_.count(stream)) {
> > >  		LOG(Allocator, Error) << "Buffers already allocated for stream";
> > >  		return -EBUSY;
> > >  	}
> > >  
> > > -	int ret = camera_->exportFrameBuffers(stream, &buffers_[stream]);
> > > +	int ret = camera_->exportFrameBuffers(stream, count, &buffers_[stream]);
> > >  	if (ret == -EINVAL)
> > >  		LOG(Allocator, Error)
> > >  			<< "Stream is not part of " << camera_->id()
> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > index 76c3bb3d8aa9..5fd1757bfe13 100644
> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > @@ -134,7 +134,7 @@ public:
> > >  		const StreamRoles &roles) override;
> > >  	int configure(Camera *camera, CameraConfiguration *config) override;
> > >  
> > > -	int exportFrameBuffers(Camera *camera, Stream *stream,
> > > +	int exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,
> > >  			       std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> > >  
> > >  	int start(Camera *camera, const ControlList *controls) override;
> > > @@ -654,10 +654,10 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> > >  }
> > >  
> > >  int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,
> > > +					    unsigned int count,
> > >  					    std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> > >  {
> > >  	IPU3CameraData *data = cameraData(camera);
> > > -	unsigned int count = stream->configuration().bufferCount;
> > >  
> > >  	if (stream == &data->outStream_)
> > >  		return data->imgu_->output_->exportBuffers(count, buffers);
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > index f821d8fe1b6c..d1cd3d9dc082 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > @@ -251,7 +251,7 @@ public:
> > >  	CameraConfiguration *generateConfiguration(Camera *camera, const StreamRoles &roles) override;
> > >  	int configure(Camera *camera, CameraConfiguration *config) override;
> > >  
> > > -	int exportFrameBuffers(Camera *camera, Stream *stream,
> > > +	int exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,
> > >  			       std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> > >  
> > >  	int start(Camera *camera, const ControlList *controls) override;
> > > @@ -795,10 +795,10 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> > >  }
> > >  
> > >  int PipelineHandlerRPi::exportFrameBuffers([[maybe_unused]] Camera *camera, Stream *stream,
> > > +					   unsigned int count,
> > >  					   std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> > >  {
> > >  	RPi::Stream *s = static_cast<RPi::Stream *>(stream);
> > > -	unsigned int count = stream->configuration().bufferCount;
> > >  	int ret = s->dev()->exportBuffers(count, buffers);
> > >  
> > >  	s->setExportedBuffers(buffers);
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > index cb4ca9a85fa8..11325875b929 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > @@ -141,7 +141,7 @@ public:
> > >  		const StreamRoles &roles) override;
> > >  	int configure(Camera *camera, CameraConfiguration *config) override;
> > >  
> > > -	int exportFrameBuffers(Camera *camera, Stream *stream,
> > > +	int exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,
> > >  			       std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> > >  
> > >  	int start(Camera *camera, const ControlList *controls) override;
> > > @@ -671,10 +671,10 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
> > >  }
> > >  
> > >  int PipelineHandlerRkISP1::exportFrameBuffers([[maybe_unused]] Camera *camera, Stream *stream,
> > > +					      unsigned int count,
> > >  					      std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> > >  {
> > >  	RkISP1CameraData *data = cameraData(camera);
> > > -	unsigned int count = stream->configuration().bufferCount;
> > >  
> > >  	if (stream == &data->mainPathStream_)
> > >  		return mainPath_.exportBuffers(count, buffers);
> > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > > index 348c554c8be4..1c25a7344f5f 100644
> > > --- a/src/libcamera/pipeline/simple/simple.cpp
> > > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > > @@ -228,7 +228,7 @@ public:
> > >  						   const StreamRoles &roles) override;
> > >  	int configure(Camera *camera, CameraConfiguration *config) override;
> > >  
> > > -	int exportFrameBuffers(Camera *camera, Stream *stream,
> > > +	int exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,
> > >  			       std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> > >  
> > >  	int start(Camera *camera, const ControlList *controls) override;
> > > @@ -776,10 +776,10 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
> > >  }
> > >  
> > >  int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream,
> > > +					      unsigned int count,
> > >  					      std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> > >  {
> > >  	SimpleCameraData *data = cameraData(camera);
> > > -	unsigned int count = stream->configuration().bufferCount;
> > >  
> > >  	/*
> > >  	 * Export buffers on the converter or capture video node, depending on
> > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > index 6ad7fb3c9f0c..fd39b3d3c72c 100644
> > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > @@ -69,7 +69,7 @@ public:
> > >  		const StreamRoles &roles) override;
> > >  	int configure(Camera *camera, CameraConfiguration *config) override;
> > >  
> > > -	int exportFrameBuffers(Camera *camera, Stream *stream,
> > > +	int exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,
> > >  			       std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> > >  
> > >  	int start(Camera *camera, const ControlList *controls) override;
> > > @@ -223,11 +223,12 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config)
> > >  	return 0;
> > >  }
> > >  
> > > -int PipelineHandlerUVC::exportFrameBuffers(Camera *camera, Stream *stream,
> > > +int PipelineHandlerUVC::exportFrameBuffers(Camera *camera,
> > > +					   [[maybe_unused]] Stream *stream,
> > > +					   unsigned int count,
> > >  					   std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> > >  {
> > >  	UVCCameraData *data = cameraData(camera);
> > > -	unsigned int count = stream->configuration().bufferCount;
> > >  
> > >  	return data->video_->exportBuffers(count, buffers);
> > >  }
> > > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> > > index 44c198ccf8b8..e89d53182c6d 100644
> > > --- a/src/libcamera/pipeline/vimc/vimc.cpp
> > > +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> > > @@ -84,7 +84,7 @@ public:
> > >  		const StreamRoles &roles) override;
> > >  	int configure(Camera *camera, CameraConfiguration *config) override;
> > >  
> > > -	int exportFrameBuffers(Camera *camera, Stream *stream,
> > > +	int exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count,
> > >  			       std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> > >  
> > >  	int start(Camera *camera, const ControlList *controls) override;
> > > @@ -299,11 +299,12 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)
> > >  	return 0;
> > >  }
> > >  
> > > -int PipelineHandlerVimc::exportFrameBuffers(Camera *camera, Stream *stream,
> > > +int PipelineHandlerVimc::exportFrameBuffers(Camera *camera,
> > > +					    [[maybe_unused]] Stream *stream,
> > > +					    unsigned int count,
> > >  					    std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> > >  {
> > >  	VimcCameraData *data = cameraData(camera);
> > > -	unsigned int count = stream->configuration().bufferCount;
> > >  
> > >  	return data->video_->exportBuffers(count, buffers);
> > >  }
> > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > > index c9928d444b04..dc83bf6924bf 100644
> > > --- a/src/libcamera/pipeline_handler.cpp
> > > +++ b/src/libcamera/pipeline_handler.cpp
> > > @@ -323,6 +323,7 @@ const ControlList &PipelineHandler::properties(const Camera *camera) const
> > >   * \brief Allocate and export buffers for \a stream
> > >   * \param[in] camera The camera
> > >   * \param[in] stream The stream to allocate buffers for
> > > + * \param[in] count The number of buffers to allocate
> > >   * \param[out] buffers Array of buffers successfully allocated
> > >   *
> > >   * This method allocates buffers for the \a stream from the devices associated
> > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> > > index 39d034de6bb2..d7475b142d6f 100644
> > > --- a/src/qcam/main_window.cpp
> > > +++ b/src/qcam/main_window.cpp
> > > @@ -25,6 +25,7 @@
> > >  #include <QtDebug>
> > >  
> > >  #include <libcamera/camera_manager.h>
> > > +#include <libcamera/property_ids.h>
> > >  #include <libcamera/version.h>
> > >  
> > >  #include "dng_writer.h"
> > > @@ -463,7 +464,15 @@ int MainWindow::startCapture()
> > >  	for (StreamConfiguration &config : *config_) {
> > >  		Stream *stream = config.stream();
> > >  
> > > -		ret = allocator_->allocate(stream);
> > > +		unsigned int bufferCount = camera_->properties().get(properties::MinimumRequests);
> > > +
> > > +		/*
> > > +		 * Need at least two buffers, one for capture and another for
> > > +		 * display
> 
> s/display/display./
> 
> > > +		 */
> > > +		bufferCount = std::max(bufferCount, 2U);
> 
> This requires inclusion of <cmath>.

A bit of search showed that I should use <algorithm> actually, so I'll go with
that, unless I'm missing something.

> 
> I think we should use bufferCount + 1, not max(bufferCount, 2). qcam
> holds on one buffer for display. If the camera needs, let's say, 2
> buffers to be able to capture, it will return a first buffer which will
> then be queued to the display. At that point, the camera will have a
> single buffer, and will hold onto it until a new buffer is queued. This
> will never happen, as the buffer being displayed won't be returned to
> libcamera until a new buffer is ready. We thus need three buffers in
> that case.
> 
> Thinking even more about this, I think we need
> 
> 		bufferCount = std::max(bufferCount + 1, 3U);
> 
> Even if the camera needs a single buffer only (in which case we would
> use two buffers, one for capture and one for display), it's clear that
> it will lead to frame drops. A minimum of 3 buffers is thus better.
> 
> The cam application should also have a similar logic. It doesn't hold
> onto a buffer, so in that case 
> 
> 		bufferCount = std::max(bufferCount, 2U);
> 
> should be good enough to start with.

Right, I did some tests now and noticed that qcam was indeed dropping frames
with just 2 requests, and with 3 it works fine. cam managed to not drop any
frames even with a single request (!), but using 2 seems safer for different
workloads anyway.

> 
> > > +
> > > +		ret = allocator_->allocate(stream, bufferCount);
> > >  		if (ret < 0) {
> > >  			qWarning() << "Failed to allocate capture buffers";
> > >  			goto error;
> > > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> > > index 157ab94e0544..d01eacfa2b84 100644
> > > --- a/src/v4l2/v4l2_camera.cpp
> > > +++ b/src/v4l2/v4l2_camera.cpp
> > > @@ -161,7 +161,7 @@ int V4L2Camera::allocBuffers(unsigned int count)
> > >  {
> > >  	Stream *stream = config_->at(0).stream();
> > >  
> > > -	int ret = bufferAllocator_->allocate(stream);
> > > +	int ret = bufferAllocator_->allocate(stream, count);
> > >  	if (ret < 0)
> > >  		return ret;
> > >  
> > > diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
> > > index 238d98dbba16..2a531805cb2b 100644
> > > --- a/test/camera/capture.cpp
> > > +++ b/test/camera/capture.cpp
> > > @@ -8,6 +8,7 @@
> > >  #include <iostream>
> > >  
> > >  #include <libcamera/framebuffer_allocator.h>
> > > +#include <libcamera/property_ids.h>
> > >  
> > >  #include <libcamera/base/event_dispatcher.h>
> > >  #include <libcamera/base/thread.h>
> > > @@ -96,7 +97,8 @@ protected:
> > >  
> > >  		Stream *stream = cfg.stream();
> > >  
> > > -		int ret = allocator_->allocate(stream);
> > > +		unsigned int bufferCount = camera_->properties().get(properties::MinimumRequests);
> > > +		int ret = allocator_->allocate(stream, bufferCount);
> 
> Similarly, this will result in frame drop at least with some pipeline
> handlers. I fear we need to go one step further and already design the
> API to tell applications how many buffers to allocate to avoid frame
> draps. Per-frame control can be left out for now.

I think that would be a good idea indeed. Technically it's simple to just add
another property, so I think the biggest issue here is naming :)

I see there are plans to namespace properties, so I think ultimately the three
of them should be grouped under a MinimumRequests namespace. For now, however,
we can just concatenate the names and add a \todo like it's done for the others.

I thought of the following possible names:

MinimumRequests::LowMemory or StillCapture -> for the current property
MinimumRequests::NoFrameDrop or Standard or VideoRecording -> for the new property
MinimumRequests::PerFrameControl or PreciseControl -> for the future property

Reasonings:

LowMemory: I think only applications on platforms with low memory available
would prefer to use this.

Standard: I think most applications would allocate this many buffers. The
capture is fluid and not as many buffers are needed, but per-frame controls
aren't guaranteed. Seems like a good compromise.

Another option is to follow the StreamRole's example: the current property can
have frame drops, so it's only suitable for StillCapture, thus use this name,
while the new property, with no frame drops, would be good enough for
VideoRecording usage. For the per-frame control property there wouldn't be a
StreamRole name to use, so we could keep using PerFrameControl, but we can also
think about this one later as it won't be implemented now.

If we go with the StreamRole naming scheme, the current property would thus be
renamed to MinimumRequestsStillCapture (while namespacing isn't possible), and
the new one MinimumRequestsVideoRecording.

Thoughts? (Naming is hard, so I expect some :) )

Thanks,
Nícolas

> 
> > >  		if (ret < 0)
> > >  			return TestFail;
> > >  
> > > diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp
> > > index 26fb5ca17139..70015bb56c74 100644
> > > --- a/test/camera/statemachine.cpp
> > > +++ b/test/camera/statemachine.cpp
> > > @@ -8,6 +8,7 @@
> > >  #include <iostream>
> > >  
> > >  #include <libcamera/framebuffer_allocator.h>
> > > +#include <libcamera/property_ids.h>
> > >  
> > >  #include "camera_test.h"
> > >  #include "test.h"
> > > @@ -119,7 +120,8 @@ protected:
> > >  		/* Use internally allocated buffers. */
> > >  		allocator_ = new FrameBufferAllocator(camera_);
> > >  		Stream *stream = *camera_->streams().begin();
> > > -		if (allocator_->allocate(stream) < 0)
> > > +		unsigned int bufferCount = camera_->properties().get(properties::MinimumRequests);
> > > +		if (allocator_->allocate(stream, bufferCount) < 0)
> > >  			return TestFail;
> > >  
> > >  		if (camera_->start())
> > > diff --git a/test/mapped-buffer.cpp b/test/mapped-buffer.cpp
> > > index c9479194cb68..e01211965364 100644
> > > --- a/test/mapped-buffer.cpp
> > > +++ b/test/mapped-buffer.cpp
> > > @@ -8,6 +8,7 @@
> > >  #include <iostream>
> > >  
> > >  #include <libcamera/framebuffer_allocator.h>
> > > +#include <libcamera/property_ids.h>
> > >  
> > >  #include "libcamera/internal/framebuffer.h"
> > >  
> > > @@ -54,7 +55,8 @@ protected:
> > >  
> > >  		stream_ = cfg.stream();
> > >  
> > > -		int ret = allocator_->allocate(stream_);
> > > +		unsigned int bufferCount = camera_->properties().get(properties::MinimumRequests);
> > > +		int ret = allocator_->allocate(stream_, bufferCount);
> > >  		if (ret < 0)
> > >  			return TestFail;
> > >  
> > > 
> 
> -- 
> Regards,
> 
> Laurent Pinchart