[libcamera-devel] [PATCH v7 02/11] libcamera: framebuffer_allocator: Make allocate() require count
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Aug 1 23:09:49 CEST 2021
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 at 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 at ideasonboard.com>
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at 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;
> > >
> > >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list