[libcamera-devel] [PATCH v9 06/18] libcamera: pipeline: simple: Don't rely on bufferCount
Jacopo Mondi
jacopo at jmondi.org
Fri Dec 16 16:17:05 CET 2022
Hi Paul
On Fri, Dec 16, 2022 at 09:29:27PM +0900, Paul Elder via libcamera-devel wrote:
> From: Nícolas F. R. A. Prado <nfraprado at collabora.com>
>
> Currently the simple pipeline handler relies on bufferCount to decide on
> the number of buffers to allocate internally when a converter is in use
> and for the number of V4L2 buffer slots to reserve. Instead, the number
> of internal buffers should be the minimum required by the pipeline to
> keep the requests flowing, in order to avoid wasting memory, while the
> number of V4L2 buffer slots should be greater than the expected number
> of requests queued by the application, in order to avoid thrashing
> dmabuf mappings, which would degrade performance.
>
> Stop relying on bufferCount for these numbers and instead set them to
> appropriate, and independent, constants.
>
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado at collabora.com>
> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
>
> ---
> Changes in v9:
> - rebased
>
> Changes in v8:
> - New
> ---
> include/libcamera/internal/converter.h | 3 ++-
> .../internal/converter/converter_v4l2_m2m.h | 6 +++--
> .../converter/converter_v4l2_m2m.cpp | 12 +++++-----
> src/libcamera/pipeline/simple/simple.cpp | 22 ++++++++++++++-----
> 4 files changed, 30 insertions(+), 13 deletions(-)
>
> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h
> index 834ec5ab..32490fe0 100644
> --- a/include/libcamera/internal/converter.h
> +++ b/include/libcamera/internal/converter.h
> @@ -49,7 +49,8 @@ public:
> virtual int exportBuffers(unsigned int output, unsigned int count,
> std::vector<std::unique_ptr<FrameBuffer>> *buffers) = 0;
>
> - virtual int start() = 0;
> + virtual int start(unsigned int internalBufferCount,
> + unsigned int bufferSlotCount) = 0;
> virtual void stop() = 0;
>
> virtual int queueBuffers(FrameBuffer *input,
> diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> index 815916d0..1f471071 100644
> --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h
> +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> @@ -50,7 +50,8 @@ public:
> int exportBuffers(unsigned int ouput, unsigned int count,
> std::vector<std::unique_ptr<FrameBuffer>> *buffers);
>
> - int start();
> + int start(unsigned int internalBufferCount,
> + unsigned int bufferSlotCount);
> void stop();
>
> int queueBuffers(FrameBuffer *input,
> @@ -69,7 +70,8 @@ private:
> int exportBuffers(unsigned int count,
> std::vector<std::unique_ptr<FrameBuffer>> *buffers);
>
> - int start();
> + int start(unsigned int internalBufferCount,
> + unsigned int bufferSlotCount);
> void stop();
>
> int queueBuffers(FrameBuffer *input, FrameBuffer *output);
> diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp
> index 2a4d1d99..9d25f25a 100644
> --- a/src/libcamera/converter/converter_v4l2_m2m.cpp
> +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp
> @@ -107,13 +107,14 @@ int V4L2M2MConverter::Stream::exportBuffers(unsigned int count,
> return m2m_->capture()->exportBuffers(count, buffers);
> }
>
> -int V4L2M2MConverter::Stream::start()
> +int V4L2M2MConverter::Stream::start(unsigned int internalBufferCount,
> + unsigned int bufferSlotCount)
> {
> - int ret = m2m_->output()->importBuffers(inputBufferCount_);
> + int ret = m2m_->output()->importBuffers(internalBufferCount);
> if (ret < 0)
> return ret;
>
> - ret = m2m_->capture()->importBuffers(outputBufferCount_);
> + ret = m2m_->capture()->importBuffers(bufferSlotCount);
Are inputBufferCount_ and outputBufferCount_ used anymore ?
And to be honest I would keep the names as they are
Also the number of output buffers could be defined by the converter
class itself ?
> if (ret < 0) {
> stop();
> return ret;
> @@ -373,12 +374,13 @@ int V4L2M2MConverter::exportBuffers(unsigned int output, unsigned int count,
> /**
> * \copydoc libcamera::Converter::start
> */
> -int V4L2M2MConverter::start()
> +int V4L2M2MConverter::start(unsigned int internalBufferCount,
> + unsigned int bufferSlotCount)
> {
> int ret;
>
> for (Stream &stream : streams_) {
> - ret = stream.start();
> + ret = stream.start(internalBufferCount, bufferSlotCount);
> if (ret < 0) {
> stop();
> return ret;
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 6b7c6d5c..196e5252 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -339,6 +339,7 @@ protected:
>
> private:
> static constexpr unsigned int kNumInternalBuffers = 3;
> + static constexpr unsigned int kSimpleBufferSlotCount = 16;
>
> struct EntityData {
> std::unique_ptr<V4L2VideoDevice> video;
> @@ -1232,17 +1233,27 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL
> return -EBUSY;
> }
>
> + /*
> + * Number of internal buffers that will be used to move video capture
> + * device frames to the converter.
> + *
> + * \todo Make this depend on the driver in use instead of being
> + * hardcoded. In order to not drop frames, the realtime requirements for
> + * each device should be checked, so it may not be as simple as just
> + * using the minimum number of buffers required by the driver.
> + */
> + static constexpr unsigned int internalBufferCount = 3;
> +
> if (data->useConverter_) {
> /*
> * When using the converter allocate a fixed number of internal
> * buffers.
> */
> - ret = video->allocateBuffers(kNumInternalBuffers,
> + ret = video->allocateBuffers(internalBufferCount,
> &data->converterBuffers_);
> } else {
> - /* Otherwise, prepare for using buffers from the only stream. */
> - Stream *stream = &data->streams_[0];
> - ret = video->importBuffers(stream->configuration().bufferCount);
> + /* Otherwise, prepare for using buffers. */
> + ret = video->importBuffers(kSimpleBufferSlotCount);
> }
> if (ret < 0) {
> releasePipeline(data);
> @@ -1258,7 +1269,8 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL
> }
>
> if (data->useConverter_) {
> - ret = data->converter_->start();
> + ret = data->converter_->start(internalBufferCount,
> + kSimpleBufferSlotCount);
> if (ret < 0) {
> stop(camera);
> return ret;
> --
> 2.35.1
>
More information about the libcamera-devel
mailing list