[libcamera-devel] [PATCH v9 03/18] libcamera: framebuffer_allocator: Make allocate() require count
Jacopo Mondi
jacopo at jmondi.org
Fri Dec 16 15:15:35 CET 2022
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..
> ---
[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