[libcamera-devel] [PATCH v2 1/2] libcamera: framebuffer_allocator: Make allocate() accept count
Hirokazu Honda
hiroh at chromium.org
Mon Apr 19 08:09:30 CEST 2021
Hi Laurent,
On Mon, Apr 19, 2021 at 3:08 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> 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 ?
>
I got it. It sounds good to me.
> > > } /* 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