[libcamera-devel] [PATCH v2 1/2] libcamera: framebuffer_allocator: Make allocate() accept count
Hirokazu Honda
hiroh at chromium.org
Mon Apr 19 07:56:33 CEST 2021
Hi Nicolas, thanks for the patch.
On Sat, Apr 17, 2021 at 7:29 AM Nícolas F. R. A. Prado
<nfraprado at collabora.com> wrote:
>
> Add a 'count' argument to FrameBufferAllocator::allocate() so that a
> custom amount of buffers can be allocated. If 0 is passed, the pipeline
> handler allocates the default amount, which is the configured
> bufferCount.
>
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado at collabora.com>
> ---
> include/libcamera/camera.h | 3 ++-
> include/libcamera/framebuffer_allocator.h | 2 +-
> include/libcamera/internal/pipeline_handler.h | 3 ++-
> src/cam/capture.cpp | 2 +-
> src/lc-compliance/simple_capture.cpp | 8 ++++----
> src/lc-compliance/simple_capture.h | 2 +-
> src/libcamera/camera.cpp | 5 +++--
> src/libcamera/framebuffer_allocator.cpp | 5 +++--
> src/libcamera/pipeline/ipu3/ipu3.cpp | 9 ++++++---
> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 9 ++++++---
> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++++++---
> src/libcamera/pipeline/simple/simple.cpp | 9 ++++++---
> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 9 ++++++---
> src/libcamera/pipeline/vimc/vimc.cpp | 9 ++++++---
> src/libcamera/pipeline_handler.cpp | 1 +
> src/qcam/main_window.cpp | 2 +-
> test/camera/capture.cpp | 2 +-
> test/camera/statemachine.cpp | 2 +-
> test/mapped-buffer.cpp | 2 +-
> 19 files changed, 58 insertions(+), 35 deletions(-)
>
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index 326b14d0ca01..ef6113113b6c 100644
> --- a/include/libcamera/camera.h
> +++ b/include/libcamera/camera.h
> @@ -116,7 +116,8 @@ private:
>
> friend class FrameBufferAllocator;
> int exportFrameBuffers(Stream *stream,
> - std::vector<std::unique_ptr<FrameBuffer>> *buffers);
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers,
> + unsigned int count);
> };
In my humble opinion, giving some number a special meaning is not a good design.
I would like to ask others' opinions.
Alternative (better) design is change the type of count to std::optional.
-Hiro
>
> } /* namespace libcamera */
> diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h
> index 0c85631a1da2..f1ae37288d50 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 c6454db6b2c4..37c51f3f0604 100644
> --- a/include/libcamera/internal/pipeline_handler.h
> +++ b/include/libcamera/internal/pipeline_handler.h
> @@ -76,7 +76,8 @@ public:
> virtual int configure(Camera *camera, CameraConfiguration *config) = 0;
>
> virtual int exportFrameBuffers(Camera *camera, Stream *stream,
> - std::vector<std::unique_ptr<FrameBuffer>> *buffers) = 0;
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers,
> + unsigned int count) = 0;
>
> virtual int start(Camera *camera, const ControlList *controls) = 0;
> virtual void stop(Camera *camera) = 0;
> diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
> index 7b55fc677022..dc24b12f4d10 100644
> --- a/src/cam/capture.cpp
> +++ b/src/cam/capture.cpp
> @@ -80,7 +80,7 @@ int Capture::capture(FrameBufferAllocator *allocator)
> /* Identify the stream with the least number of buffers. */
> unsigned int nbuffers = UINT_MAX;
> for (StreamConfiguration &cfg : *config_) {
> - ret = allocator->allocate(cfg.stream());
> + ret = allocator->allocate(cfg.stream(), 0);
> if (ret < 0) {
> std::cerr << "Can't allocate buffers" << std::endl;
> return -ENOMEM;
> diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp
> index 64e862a08e3a..42e4c9b1302d 100644
> --- a/src/lc-compliance/simple_capture.cpp
> +++ b/src/lc-compliance/simple_capture.cpp
> @@ -39,10 +39,10 @@ Results::Result SimpleCapture::configure(StreamRole role)
> return { Results::Pass, "Configure camera" };
> }
>
> -Results::Result SimpleCapture::start()
> +Results::Result SimpleCapture::start(unsigned int numBuffers)
> {
> Stream *stream = config_->at(0).stream();
> - if (allocator_->allocate(stream) < 0)
> + if (allocator_->allocate(stream, numBuffers) < 0)
> return { Results::Fail, "Failed to allocate buffers" };
>
> if (camera_->start())
> @@ -73,7 +73,7 @@ SimpleCaptureBalanced::SimpleCaptureBalanced(std::shared_ptr<Camera> camera)
>
> Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests)
> {
> - Results::Result ret = start();
> + Results::Result ret = start(0);
> if (ret.first != Results::Pass)
> return ret;
>
> @@ -161,7 +161,7 @@ SimpleCaptureUnbalanced::SimpleCaptureUnbalanced(std::shared_ptr<Camera> camera)
>
> Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests)
> {
> - Results::Result ret = start();
> + Results::Result ret = start(0);
> if (ret.first != Results::Pass)
> return ret;
>
> diff --git a/src/lc-compliance/simple_capture.h b/src/lc-compliance/simple_capture.h
> index d9de53fb63a3..2b1493ecaf96 100644
> --- a/src/lc-compliance/simple_capture.h
> +++ b/src/lc-compliance/simple_capture.h
> @@ -23,7 +23,7 @@ protected:
> SimpleCapture(std::shared_ptr<libcamera::Camera> camera);
> virtual ~SimpleCapture();
>
> - Results::Result start();
> + Results::Result start(unsigned int numBuffers);
> void stop();
>
> virtual void requestComplete(libcamera::Request *request) = 0;
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 763f3b9926fd..7df110ee2f52 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -657,7 +657,8 @@ void Camera::disconnect()
> }
>
> int Camera::exportFrameBuffers(Stream *stream,
> - std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers,
> + unsigned int count)
> {
> Private *const d = LIBCAMERA_D_PTR();
>
> @@ -673,7 +674,7 @@ int Camera::exportFrameBuffers(Stream *stream,
>
> return d->pipe_->invokeMethod(&PipelineHandler::exportFrameBuffers,
> ConnectionTypeBlocking, this, stream,
> - buffers);
> + buffers, count);
> }
>
> /**
> diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp
> index 2fbba37a1b0b..6b07203017cd 100644
> --- a/src/libcamera/framebuffer_allocator.cpp
> +++ b/src/libcamera/framebuffer_allocator.cpp
> @@ -70,6 +70,7 @@ FrameBufferAllocator::~FrameBufferAllocator()
> /**
> * \brief Allocate buffers for a configured stream
> * \param[in] stream The stream to allocate buffers for
> + * \param[in] count The amount 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
> @@ -85,14 +86,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, &buffers_[stream], count);
> 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 519cad4f8148..7a44900b9fbc 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -131,7 +131,8 @@ public:
> int configure(Camera *camera, CameraConfiguration *config) override;
>
> int exportFrameBuffers(Camera *camera, Stream *stream,
> - std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers,
> + unsigned int count) override;
>
> int start(Camera *camera, const ControlList *controls) override;
> void stop(Camera *camera) override;
> @@ -641,10 +642,12 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> }
>
> int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,
> - std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers,
> + unsigned int count)
> {
> IPU3CameraData *data = cameraData(camera);
> - unsigned int count = stream->configuration().bufferCount;
> + if (!count)
> + 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 f22e286ed87a..3ab27123b1ac 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -250,7 +250,8 @@ public:
> int configure(Camera *camera, CameraConfiguration *config) override;
>
> int exportFrameBuffers(Camera *camera, Stream *stream,
> - std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers,
> + unsigned int count) override;
>
> int start(Camera *camera, const ControlList *controls) override;
> void stop(Camera *camera) override;
> @@ -786,10 +787,12 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> }
>
> int PipelineHandlerRPi::exportFrameBuffers([[maybe_unused]] Camera *camera, Stream *stream,
> - std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers,
> + unsigned int count)
> {
> RPi::Stream *s = static_cast<RPi::Stream *>(stream);
> - unsigned int count = stream->configuration().bufferCount;
> + if (!count)
> + 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 549f4a4e61a8..4cc3c7739251 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -140,7 +140,8 @@ public:
> int configure(Camera *camera, CameraConfiguration *config) override;
>
> int exportFrameBuffers(Camera *camera, Stream *stream,
> - std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers,
> + unsigned int count) override;
>
> int start(Camera *camera, const ControlList *controls) override;
> void stop(Camera *camera) override;
> @@ -666,10 +667,12 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
> }
>
> int PipelineHandlerRkISP1::exportFrameBuffers([[maybe_unused]] Camera *camera, Stream *stream,
> - std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers,
> + unsigned int count)
> {
> RkISP1CameraData *data = cameraData(camera);
> - unsigned int count = stream->configuration().bufferCount;
> + if (!count)
> + 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 f6095d38e97a..e9b0698813fc 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -223,7 +223,8 @@ public:
> int configure(Camera *camera, CameraConfiguration *config) override;
>
> int exportFrameBuffers(Camera *camera, Stream *stream,
> - std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers,
> + unsigned int count) override;
>
> int start(Camera *camera, const ControlList *controls) override;
> void stop(Camera *camera) override;
> @@ -762,10 +763,12 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
> }
>
> int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream,
> - std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers,
> + unsigned int count)
> {
> SimpleCameraData *data = cameraData(camera);
> - unsigned int count = stream->configuration().bufferCount;
> + if (!count)
> + 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 b6c6ade5ebaf..298e7031d23b 100644
> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> @@ -70,7 +70,8 @@ public:
> int configure(Camera *camera, CameraConfiguration *config) override;
>
> int exportFrameBuffers(Camera *camera, Stream *stream,
> - std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers,
> + unsigned int count) override;
>
> int start(Camera *camera, const ControlList *controls) override;
> void stop(Camera *camera) override;
> @@ -224,10 +225,12 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config)
> }
>
> int PipelineHandlerUVC::exportFrameBuffers(Camera *camera, Stream *stream,
> - std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers,
> + unsigned int count)
> {
> UVCCameraData *data = cameraData(camera);
> - unsigned int count = stream->configuration().bufferCount;
> + if (!count)
> + 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 8f5f4ba30953..2f889347b624 100644
> --- a/src/libcamera/pipeline/vimc/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> @@ -84,7 +84,8 @@ public:
> int configure(Camera *camera, CameraConfiguration *config) override;
>
> int exportFrameBuffers(Camera *camera, Stream *stream,
> - std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers,
> + unsigned int count) override;
>
> int start(Camera *camera, const ControlList *controls) override;
> void stop(Camera *camera) override;
> @@ -299,10 +300,12 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)
> }
>
> int PipelineHandlerVimc::exportFrameBuffers(Camera *camera, Stream *stream,
> - std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers,
> + unsigned int count)
> {
> VimcCameraData *data = cameraData(camera);
> - unsigned int count = stream->configuration().bufferCount;
> + if (!count)
> + 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 3b3150bdbbf7..ab5f21a01438 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
> * \param[in] camera The camera
> * \param[in] stream The stream to allocate buffers for
> * \param[out] buffers Array of buffers successfully allocated
> + * \param[in] count The amount of buffers to allocate
> *
> * This method allocates buffers for the \a stream from the devices associated
> * with the stream in the corresponding pipeline handler. Those buffers shall be
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> index 39d034de6bb2..4e1dbb0656c8 100644
> --- a/src/qcam/main_window.cpp
> +++ b/src/qcam/main_window.cpp
> @@ -463,7 +463,7 @@ int MainWindow::startCapture()
> for (StreamConfiguration &config : *config_) {
> Stream *stream = config.stream();
>
> - ret = allocator_->allocate(stream);
> + ret = allocator_->allocate(stream, 0);
> if (ret < 0) {
> qWarning() << "Failed to allocate capture buffers";
> goto error;
> diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
> index c4bc21100777..6cbca15b30bc 100644
> --- a/test/camera/capture.cpp
> +++ b/test/camera/capture.cpp
> @@ -96,7 +96,7 @@ protected:
>
> Stream *stream = cfg.stream();
>
> - int ret = allocator_->allocate(stream);
> + int ret = allocator_->allocate(stream, 0);
> if (ret < 0)
> return TestFail;
>
> diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp
> index f0c3d7764027..9e076d05bc6f 100644
> --- a/test/camera/statemachine.cpp
> +++ b/test/camera/statemachine.cpp
> @@ -118,7 +118,7 @@ protected:
> /* Use internally allocated buffers. */
> allocator_ = new FrameBufferAllocator(camera_);
> Stream *stream = *camera_->streams().begin();
> - if (allocator_->allocate(stream) < 0)
> + if (allocator_->allocate(stream, 0) < 0)
> return TestFail;
>
> if (camera_->start())
> diff --git a/test/mapped-buffer.cpp b/test/mapped-buffer.cpp
> index 5de8201e45f6..f5c2e2c0169a 100644
> --- a/test/mapped-buffer.cpp
> +++ b/test/mapped-buffer.cpp
> @@ -54,7 +54,7 @@ protected:
>
> stream_ = cfg.stream();
>
> - int ret = allocator_->allocate(stream_);
> + int ret = allocator_->allocate(stream_, 0);
> if (ret < 0)
> return TestFail;
>
> --
> 2.31.1
>
More information about the libcamera-devel
mailing list