[libcamera-devel] [PATCH v2 1/2] libcamera: framebuffer_allocator: Make allocate() accept count
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Apr 19 08:07:59 CEST 2021
Hello,
On Mon, Apr 19, 2021 at 02:56:33PM +0900, Hirokazu Honda wrote:
> Hi Nicolas, thanks for the patch.
>
> On Sat, Apr 17, 2021 at 7:29 AM Nícolas F. R. A. Prado 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.
std::optional isn't an option, as it's a C++17 feature, and the public
API has to stay compatible with C++14 to support Chromium (the browser,
not the OS).
How about making the buffer count mandatory, always passing a value
explicitly ?
> > } /* 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;
> >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list