[libcamera-devel] [PATCH v9 03/18] libcamera: framebuffer_allocator: Make allocate() require count
Paul Elder
paul.elder at ideasonboard.com
Wed Dec 21 09:10:16 CET 2022
On Fri, Dec 16, 2022 at 03:15:35PM +0100, Jacopo Mondi wrote:
> Hi Paul
>
> On Fri, Dec 16, 2022 at 09:29:24PM +0900, Paul Elder via libcamera-devel wrote:
> > From: Nícolas F. R. A. Prado <nfraprado at collabora.com>
> >
> > 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>
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> >
>
> Seeing this being extensively reviewed, I won't comment on the change
> to the public API interface. I'm a bit afraid we're putting on
> applications the burden of getting this right, when they probably
> don't care in most cases..
(I went back through the history) It looks like it was decided in v2 to
go this way.
Paul
>
> > ---
>
> [snip]
>
> > /**
> > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp
> > index dabd9219..6a0bb8df 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
> > @@ -79,6 +80,10 @@ FrameBufferAllocator::~FrameBufferAllocator()
> > * Upon successful allocation, the allocated buffers can be retrieved with the
> > * buffers() function.
> > *
> > + * 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.
> > + *
> > * \return The number of allocated buffers on success or a negative error code
> > * otherwise
> > * \retval -EACCES The camera is not in a state where buffers can be allocated
> > @@ -86,7 +91,7 @@ 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)
> > {
> > const auto &[it, inserted] = buffers_.try_emplace(stream);
> >
> > @@ -95,7 +100,7 @@ int FrameBufferAllocator::allocate(Stream *stream)
> > return -EBUSY;
> > }
> >
> > - int ret = camera_->exportFrameBuffers(stream, &it->second);
> > + int ret = camera_->exportFrameBuffers(stream, count, &it->second);
>
> We have the camera, we can access properties, count could be
>
> count = max(properties::MinimumRequests, count)
>
> A detail through
>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>
>
> > 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 98a4a3e5..bab2db65 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -140,7 +140,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;
> > @@ -687,10 +687,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 4a08d01e..4641c76f 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -328,7 +328,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;
> > @@ -1005,10 +1005,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 45b6beaf..8fe37c4f 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -148,7 +148,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;
> > @@ -816,10 +816,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 8ed983fe..6b7c6d5c 100644
> > --- a/src/libcamera/pipeline/simple/simple.cpp
> > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > @@ -322,7 +322,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;
> > @@ -1202,10 +1202,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 7f580955..4ce240a4 100644
> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > @@ -78,7 +78,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;
> > @@ -226,11 +226,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 d2633be4..e58caf99 100644
> > --- a/src/libcamera/pipeline/vimc/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> > @@ -89,7 +89,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;
> > @@ -321,11 +321,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 cfade490..fffbd51b 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -280,6 +280,7 @@ void PipelineHandler::unlockMediaDevices()
> > * \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 function allocates buffers for the \a stream from the devices associated
> > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> > index 7b97c2d5..1c5fab64 100644
> > --- a/src/v4l2/v4l2_camera.cpp
> > +++ b/src/v4l2/v4l2_camera.cpp
> > @@ -160,7 +160,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/camera_reconfigure.cpp b/test/camera/camera_reconfigure.cpp
> > index 06c87730..63a97863 100644
> > --- a/test/camera/camera_reconfigure.cpp
> > +++ b/test/camera/camera_reconfigure.cpp
> > @@ -17,6 +17,7 @@
> > #include <libcamera/base/timer.h>
> >
> > #include <libcamera/framebuffer_allocator.h>
> > +#include <libcamera/property_ids.h>
> >
> > #include "camera_test.h"
> > #include "test.h"
> > @@ -78,7 +79,9 @@ private:
> > * same buffer allocation for each run.
> > */
> > if (!allocated_) {
> > - int ret = allocator_->allocate(stream);
> > + unsigned int bufferCount =
> > + camera_->properties().get(properties::MinimumRequests).value();
> > + int ret = allocator_->allocate(stream, bufferCount);
> > if (ret < 0) {
> > cerr << "Failed to allocate buffers" << endl;
> > return TestFail;
> > diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
> > index de824083..64d3a8e7 100644
> > --- a/test/camera/capture.cpp
> > +++ b/test/camera/capture.cpp
> > @@ -7,12 +7,13 @@
> >
> > #include <iostream>
> >
> > -#include <libcamera/framebuffer_allocator.h>
> > -
> > #include <libcamera/base/event_dispatcher.h>
> > #include <libcamera/base/thread.h>
> > #include <libcamera/base/timer.h>
> >
> > +#include <libcamera/framebuffer_allocator.h>
> > +#include <libcamera/property_ids.h>
> > +
> > #include "camera_test.h"
> > #include "test.h"
> >
> > @@ -98,8 +99,10 @@ protected:
> >
> > Stream *stream = cfg.stream();
> >
> > - int ret = allocator_->allocate(stream);
> > - if (ret < 0)
> > + unsigned int bufferCount =
> > + camera_->properties().get(properties::MinimumRequests).value();
> > + int ret = allocator_->allocate(stream, bufferCount);
> > + if (ret < static_cast<int>(bufferCount))
> > return TestFail;
> >
> > for (const std::unique_ptr<FrameBuffer> &buffer : allocator_->buffers(stream)) {
> > diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp
> > index 9c2b0c6a..a9ddb323 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"
> > @@ -120,7 +121,9 @@ 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).value();
> > + if (allocator_->allocate(stream, bufferCount) < 0)
> > return TestFail;
> >
> > if (camera_->start())
> > diff --git a/test/fence.cpp b/test/fence.cpp
> > index 1e38bc2f..88ce2857 100644
> > --- a/test/fence.cpp
> > +++ b/test/fence.cpp
> > @@ -18,6 +18,7 @@
> >
> > #include <libcamera/fence.h>
> > #include <libcamera/framebuffer_allocator.h>
> > +#include <libcamera/property_ids.h>
> >
> > #include "camera_test.h"
> > #include "test.h"
> > @@ -117,8 +118,11 @@ int FenceTest::init()
> > StreamConfiguration &cfg = config_->at(0);
> > stream_ = cfg.stream();
> >
> > + unsigned int bufferCount =
> > + camera_->properties().get(properties::MinimumRequests).value();
> > +
> > allocator_ = std::make_unique<FrameBufferAllocator>(camera_);
> > - if (allocator_->allocate(stream_) < 0)
> > + if (allocator_->allocate(stream_, bufferCount) < 0)
> > return TestFail;
> >
> > nbuffers_ = allocator_->buffers(stream_).size();
> > diff --git a/test/mapped-buffer.cpp b/test/mapped-buffer.cpp
> > index b4422f7d..1b98feea 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/mapped_framebuffer.h"
> >
> > @@ -55,7 +56,9 @@ protected:
> >
> > stream_ = cfg.stream();
> >
> > - int ret = allocator_->allocate(stream_);
> > + unsigned int bufferCount =
> > + camera_->properties().get(properties::MinimumRequests).value();
> > + int ret = allocator_->allocate(stream_, bufferCount);
> > if (ret < 0)
> > return TestFail;
> >
> > --
> > 2.35.1
> >
More information about the libcamera-devel
mailing list