[libcamera-devel] [PATCH v3 4/4] libcamera: stream: Remove bufferCount
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Apr 26 05:19:57 CEST 2021
Hi Nícolas,
Thank you for the patch.
On Wed, Apr 21, 2021 at 01:51:39PM -0300, Nícolas F. R. A. Prado wrote:
> Now that the amount of internal buffers allocated is hardcoded by the
> pipelines, and the amount of buffers allocated by the
> FrameBufferAllocator helper is passed through
> FrameBufferAllocator::allocate(), we no longer need to have bufferCount
> in the StreamConfiguration, so remove it.
>
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado at collabora.com>
> ---
> include/libcamera/stream.h | 2 --
> src/libcamera/pipeline/ipu3/cio2.cpp | 1 -
> src/libcamera/pipeline/ipu3/ipu3.cpp | 15 +--------------
> .../pipeline/raspberrypi/raspberrypi.cpp | 9 ++-------
> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++-------
> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 2 --
> src/libcamera/pipeline/simple/converter.cpp | 7 ++-----
> src/libcamera/pipeline/simple/converter.h | 5 ++---
> src/libcamera/pipeline/simple/simple.cpp | 15 ++++-----------
> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 5 +----
> src/libcamera/pipeline/vimc/vimc.cpp | 5 +----
> src/libcamera/stream.cpp | 9 ++-------
> src/v4l2/v4l2_camera.cpp | 14 ++++++++++----
> src/v4l2/v4l2_camera.h | 5 +++--
> src/v4l2/v4l2_camera_proxy.cpp | 8 +++-----
> test/camera/buffer_import.cpp | 10 +++++++---
> test/libtest/buffer_source.cpp | 4 ++--
> test/libtest/buffer_source.h | 2 +-
> test/v4l2_videodevice/buffer_cache.cpp | 4 ++--
An update to src/android/ is missing, which breaks compilation of the
HAL :-S
> 19 files changed, 45 insertions(+), 86 deletions(-)
>
> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
> index bb47c390f8a1..f36aeffd9540 100644
> --- a/include/libcamera/stream.h
> +++ b/include/libcamera/stream.h
> @@ -45,8 +45,6 @@ struct StreamConfiguration {
> unsigned int stride;
> unsigned int frameSize;
>
> - unsigned int bufferCount;
> -
> Stream *stream() const { return stream_; }
> void setStream(Stream *stream) { stream_ = stream; }
> const StreamFormats &formats() const { return formats_; }
> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> index 3cd777d1b742..1e110fe0c189 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> @@ -213,7 +213,6 @@ StreamConfiguration CIO2Device::generateConfiguration(Size size) const
>
> cfg.size = sensorFormat.size;
> cfg.pixelFormat = mbusCodesToPixelFormat.at(sensorFormat.mbus_code);
> - cfg.bufferCount = CIO2_BUFFER_COUNT;
>
> return cfg;
> }
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index c8fcc2fda75f..f0a17a553bd3 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -291,7 +291,6 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> /* Initialize the RAW stream with the CIO2 configuration. */
> cfg->size = cio2Configuration_.size;
> cfg->pixelFormat = cio2Configuration_.pixelFormat;
> - cfg->bufferCount = cio2Configuration_.bufferCount;
> cfg->stride = info.stride(cfg->size.width, 0, 64);
> cfg->frameSize = info.frameSize(cfg->size, 64);
> cfg->setStream(const_cast<Stream *>(&data_->rawStream_));
> @@ -335,7 +334,6 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> IMGU_OUTPUT_HEIGHT_ALIGN);
>
> cfg->pixelFormat = formats::NV12;
> - cfg->bufferCount = IPU3_BUFFER_COUNT;
> cfg->stride = info.stride(cfg->size.width, 0, 1);
> cfg->frameSize = info.frameSize(cfg->size, 1);
>
> @@ -403,7 +401,6 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> Size sensorResolution = data->cio2_.sensor()->resolution();
> for (const StreamRole role : roles) {
> std::map<PixelFormat, std::vector<SizeRange>> streamFormats;
> - unsigned int bufferCount;
> PixelFormat pixelFormat;
> Size size;
>
> @@ -424,7 +421,6 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> size.height = utils::alignDown(size.height - 1,
> IMGU_OUTPUT_HEIGHT_MARGIN);
> pixelFormat = formats::NV12;
> - bufferCount = IPU3_BUFFER_COUNT;
> streamFormats[pixelFormat] = { { IMGU_OUTPUT_MIN_SIZE, size } };
>
> break;
> @@ -434,7 +430,6 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> data->cio2_.generateConfiguration(sensorResolution);
> pixelFormat = cio2Config.pixelFormat;
> size = cio2Config.size;
> - bufferCount = cio2Config.bufferCount;
>
> for (const PixelFormat &format : data->cio2_.formats())
> streamFormats[format] = data->cio2_.sizes();
> @@ -453,7 +448,6 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> .alignedDownTo(IMGU_OUTPUT_WIDTH_ALIGN,
> IMGU_OUTPUT_HEIGHT_ALIGN);
> pixelFormat = formats::NV12;
> - bufferCount = IPU3_BUFFER_COUNT;
> streamFormats[pixelFormat] = { { IMGU_OUTPUT_MIN_SIZE, size } };
>
> break;
> @@ -470,7 +464,6 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> StreamConfiguration cfg(formats);
> cfg.size = size;
> cfg.pixelFormat = pixelFormat;
> - cfg.bufferCount = bufferCount;
> config->addConfiguration(cfg);
> }
>
> @@ -668,15 +661,9 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera)
> {
> IPU3CameraData *data = cameraData(camera);
> ImgUDevice *imgu = data->imgu_;
> - unsigned int bufferCount;
> + unsigned int bufferCount = data->properties_.get(properties::QueueDepth);
> int ret;
>
> - bufferCount = std::max({
> - data->outStream_.configuration().bufferCount,
> - data->vfStream_.configuration().bufferCount,
> - data->rawStream_.configuration().bufferCount,
> - });
I'm not sure properties::QueueDepth is the right value here. This deals
with both allocation of stats and params buffers, which are internal,
and the number of V4L2 buffer slots for the ImgU input and output. For
the latter, we probably should overallocate, as underallocation would
mean trashing dmabuf mappings. For the former, we shouldn't allocate too
much to avoid wasting memory, so we should take into account how long
the IPA would need to hold on the params and stats buffers.
Jacopo, Jean-Michel, Niklas, any comment ?
> -
> ret = imgu->allocateBuffers(bufferCount);
> if (ret < 0)
> return ret;
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 3f35596fe550..44a8a472ae4f 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -471,7 +471,6 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,
> RPiCameraData *data = cameraData(camera);
> CameraConfiguration *config = new RPiCameraConfiguration(data);
> V4L2DeviceFormat sensorFormat;
> - unsigned int bufferCount;
> PixelFormat pixelFormat;
> V4L2VideoDevice::Formats fmts;
> Size size;
> @@ -489,7 +488,6 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,
> sensorFormat = findBestMode(fmts, size);
> pixelFormat = sensorFormat.fourcc.toPixelFormat();
> ASSERT(pixelFormat.isValid());
> - bufferCount = 2;
> rawCount++;
> break;
>
> @@ -498,7 +496,6 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,
> pixelFormat = formats::NV12;
> /* Return the largest sensor resolution. */
> size = data->sensor_->resolution();
> - bufferCount = 1;
> outCount++;
> break;
>
> @@ -514,7 +511,6 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,
> fmts = data->isp_[Isp::Output0].dev()->formats();
> pixelFormat = formats::YUV420;
> size = { 1920, 1080 };
> - bufferCount = 4;
> outCount++;
> break;
>
> @@ -522,7 +518,6 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,
> fmts = data->isp_[Isp::Output0].dev()->formats();
> pixelFormat = formats::ARGB8888;
> size = { 800, 600 };
> - bufferCount = 4;
> outCount++;
> break;
>
> @@ -552,7 +547,6 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,
> StreamConfiguration cfg(formats);
> cfg.size = size;
> cfg.pixelFormat = pixelFormat;
> - cfg.bufferCount = bufferCount;
> config->addConfiguration(cfg);
> }
>
> @@ -1142,6 +1136,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> {
> RPiCameraData *data = cameraData(camera);
> int ret;
> + unsigned int bufferCount = data->properties_.get(properties::QueueDepth);
>
> /*
> * Decide how many internal buffers to allocate. For now, simply look
> @@ -1152,7 +1147,7 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)
> unsigned int maxBuffers = 0;
> for (const Stream *s : camera->streams())
> if (static_cast<const RPi::Stream *>(s)->isExternal())
> - maxBuffers = std::max(maxBuffers, s->configuration().bufferCount);
> + maxBuffers = std::max(maxBuffers, bufferCount);
That now looks a bit weird, as bufferCount is constant. This being said,
the above comment for the IPU3 applies here. David, Naush, any comment ?
>
> for (auto const stream : data->streams_) {
> ret = stream->prepareBuffers(maxBuffers);
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 2d95c1ca2a43..73d4ea6ba8f5 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -686,16 +686,11 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)
> unsigned int ipaBufferId = 1;
> int ret;
>
> - unsigned int maxCount = std::max({
> - data->mainPathStream_.configuration().bufferCount,
> - data->selfPathStream_.configuration().bufferCount,
> - });
> -
> - ret = param_->allocateBuffers(maxCount, ¶mBuffers_);
> + ret = param_->allocateBuffers(RKISP1_BUFFER_COUNT, ¶mBuffers_);
> if (ret < 0)
> goto error;
>
> - ret = stat_->allocateBuffers(maxCount, &statBuffers_);
> + ret = stat_->allocateBuffers(RKISP1_BUFFER_COUNT, &statBuffers_);
Here it should be fine.
> if (ret < 0)
> goto error;
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> index 25f482eb8d8e..200e3c2c4cca 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> @@ -61,7 +61,6 @@ StreamConfiguration RkISP1Path::generateConfiguration(const Size &resolution)
> StreamConfiguration cfg(formats);
> cfg.pixelFormat = formats::NV12;
> cfg.size = maxResolution;
> - cfg.bufferCount = RKISP1_BUFFER_COUNT;
>
> return cfg;
> }
> @@ -77,7 +76,6 @@ CameraConfiguration::Status RkISP1Path::validate(StreamConfiguration *cfg)
>
> cfg->size.boundTo(maxResolution_);
> cfg->size.expandTo(minResolution_);
> - cfg->bufferCount = RKISP1_BUFFER_COUNT;
>
> V4L2DeviceFormat format;
> format.fourcc = video_->toV4L2PixelFormat(cfg->pixelFormat);
> diff --git a/src/libcamera/pipeline/simple/converter.cpp b/src/libcamera/pipeline/simple/converter.cpp
> index 68644ef6477f..54e7f1b051f7 100644
> --- a/src/libcamera/pipeline/simple/converter.cpp
> +++ b/src/libcamera/pipeline/simple/converter.cpp
> @@ -86,9 +86,6 @@ int SimpleConverter::Stream::configure(const StreamConfiguration &inputCfg,
> return -EINVAL;
> }
>
> - inputBufferCount_ = inputCfg.bufferCount;
> - outputBufferCount_ = outputCfg.bufferCount;
> -
> return 0;
> }
>
> @@ -100,11 +97,11 @@ int SimpleConverter::Stream::exportBuffers(unsigned int count,
>
> int SimpleConverter::Stream::start()
> {
> - int ret = m2m_->output()->importBuffers(inputBufferCount_);
> + int ret = m2m_->output()->importBuffers(SIMPLE_QUEUE_DEPTH);
> if (ret < 0)
> return ret;
>
> - ret = m2m_->capture()->importBuffers(outputBufferCount_);
> + ret = m2m_->capture()->importBuffers(SIMPLE_QUEUE_DEPTH);
As above, we should probably overallocate here to avoid trashing the
dmabuf mappings.
> if (ret < 0) {
> stop();
> return ret;
> diff --git a/src/libcamera/pipeline/simple/converter.h b/src/libcamera/pipeline/simple/converter.h
> index 480e528d2210..32313748cd24 100644
> --- a/src/libcamera/pipeline/simple/converter.h
> +++ b/src/libcamera/pipeline/simple/converter.h
> @@ -29,6 +29,8 @@ class SizeRange;
> struct StreamConfiguration;
> class V4L2M2MDevice;
>
> +constexpr unsigned int SIMPLE_QUEUE_DEPTH = 3;
> +
> class SimpleConverter
> {
> public:
> @@ -84,9 +86,6 @@ private:
> SimpleConverter *converter_;
> unsigned int index_;
> std::unique_ptr<V4L2M2MDevice> m2m_;
> -
> - unsigned int inputBufferCount_;
> - unsigned int outputBufferCount_;
> };
>
> std::string deviceNode_;
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index b9f14be6733f..9f28bb66e2e7 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -142,8 +142,6 @@ static const SimplePipelineInfo supportedDevices[] = {
>
> } /* namespace */
>
> -static constexpr unsigned int kNumInternalBuffers = 3;
> -
> class SimpleCameraData : public CameraData
> {
> public:
> @@ -425,7 +423,7 @@ int SimpleCameraData::init()
> }
>
> properties_ = sensor_->properties();
> - properties_.set(properties::QueueDepth, kNumInternalBuffers);
> + properties_.set(properties::QueueDepth, SIMPLE_QUEUE_DEPTH);
>
> return 0;
> }
> @@ -616,7 +614,7 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()
> cfg.size != pipeConfig_->captureSize)
> needConversion_ = true;
>
> - /* Set the stride, frameSize and bufferCount. */
> + /* Set the stride and frameSize. */
> if (needConversion_) {
> std::tie(cfg.stride, cfg.frameSize) =
> converter->strideAndFrameSize(cfg.pixelFormat, cfg.size);
> @@ -634,8 +632,6 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()
> cfg.stride = format.planes[0].bpl;
> cfg.frameSize = format.planes[0].size;
> }
> -
> - cfg.bufferCount = 3;
> }
>
> return status;
> @@ -758,7 +754,6 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
> inputCfg.pixelFormat = pipeConfig->captureFormat;
> inputCfg.size = pipeConfig->captureSize;
> inputCfg.stride = captureFormat.planes[0].bpl;
> - inputCfg.bufferCount = kNumInternalBuffers;
>
> return converter_->configure(inputCfg, outputCfgs);
> }
> @@ -791,12 +786,10 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL
> * When using the converter allocate a fixed number of internal
> * buffers.
> */
> - ret = video->allocateBuffers(kNumInternalBuffers,
> + ret = video->allocateBuffers(SIMPLE_QUEUE_DEPTH,
This should definitely not overallocate :-) 3 sounds good.
> &data->converterBuffers_);
> } else {
> - /* Otherwise, prepare for using buffers from the only stream. */
> - Stream *stream = &data->streams_[0];
> - ret = video->importBuffers(stream->configuration().bufferCount);
> + ret = video->importBuffers(SIMPLE_QUEUE_DEPTH);
Here we can overallocate too.
> }
> if (ret < 0)
> return ret;
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index a148c35f1265..94e6fd9d2d56 100644
> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> @@ -148,8 +148,6 @@ CameraConfiguration::Status UVCCameraConfiguration::validate()
> status = Adjusted;
> }
>
> - cfg.bufferCount = 4;
> -
> V4L2DeviceFormat format;
> format.fourcc = data_->video_->toV4L2PixelFormat(cfg.pixelFormat);
> format.size = cfg.size;
> @@ -191,7 +189,6 @@ CameraConfiguration *PipelineHandlerUVC::generateConfiguration(Camera *camera,
>
> cfg.pixelFormat = formats.pixelformats().front();
> cfg.size = formats.sizes(cfg.pixelFormat).back();
> - cfg.bufferCount = 4;
>
> config->addConfiguration(cfg);
>
> @@ -236,7 +233,7 @@ int PipelineHandlerUVC::exportFrameBuffers(Camera *camera,
> int PipelineHandlerUVC::start(Camera *camera, [[maybe_unused]] const ControlList *controls)
> {
> UVCCameraData *data = cameraData(camera);
> - unsigned int count = data->stream_.configuration().bufferCount;
> + unsigned int count = data->properties_.get(properties::QueueDepth);
>
> int ret = data->video_->importBuffers(count);
Same here, overallocation is best.
> if (ret < 0)
> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> index 22d6fdcbb141..891571afb3e5 100644
> --- a/src/libcamera/pipeline/vimc/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> @@ -165,8 +165,6 @@ CameraConfiguration::Status VimcCameraConfiguration::validate()
> status = Adjusted;
> }
>
> - cfg.bufferCount = 4;
> -
> V4L2DeviceFormat format;
> format.fourcc = data_->video_->toV4L2PixelFormat(cfg.pixelFormat);
> format.size = cfg.size;
> @@ -222,7 +220,6 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,
>
> cfg.pixelFormat = formats::BGR888;
> cfg.size = { 1920, 1080 };
> - cfg.bufferCount = 4;
>
> config->addConfiguration(cfg);
>
> @@ -312,7 +309,7 @@ int PipelineHandlerVimc::exportFrameBuffers(Camera *camera,
> int PipelineHandlerVimc::start(Camera *camera, [[maybe_unused]] const ControlList *controls)
> {
> VimcCameraData *data = cameraData(camera);
> - unsigned int count = data->stream_.configuration().bufferCount;
> + unsigned int count = data->properties_.get(properties::QueueDepth);
>
> int ret = data->video_->importBuffers(count);
I think you know by now what I would say :-)
> if (ret < 0)
> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
> index f7bafcf8fc97..be57abce4eb3 100644
> --- a/src/libcamera/stream.cpp
> +++ b/src/libcamera/stream.cpp
> @@ -279,7 +279,7 @@ SizeRange StreamFormats::range(const PixelFormat &pixelformat) const
> * handlers provide StreamFormats.
> */
> StreamConfiguration::StreamConfiguration()
> - : pixelFormat(0), stride(0), frameSize(0), bufferCount(0),
> + : pixelFormat(0), stride(0), frameSize(0),
> stream_(nullptr)
> {
> }
> @@ -288,7 +288,7 @@ StreamConfiguration::StreamConfiguration()
> * \brief Construct a configuration with stream formats
> */
> StreamConfiguration::StreamConfiguration(const StreamFormats &formats)
> - : pixelFormat(0), stride(0), frameSize(0), bufferCount(0),
> + : pixelFormat(0), stride(0), frameSize(0),
> stream_(nullptr), formats_(formats)
> {
> }
> @@ -323,11 +323,6 @@ StreamConfiguration::StreamConfiguration(const StreamFormats &formats)
> * validating the configuration with a call to CameraConfiguration::validate().
> */
>
> -/**
> - * \var StreamConfiguration::bufferCount
> - * \brief Requested number of buffers to allocate for the stream
> - */
> -
> /**
> * \fn StreamConfiguration::stream()
> * \brief Retrieve the stream associated with the configuration
> diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> index 53d97f3e6b86..79bf7aec7782 100644
> --- a/src/v4l2/v4l2_camera.cpp
> +++ b/src/v4l2/v4l2_camera.cpp
> @@ -10,6 +10,8 @@
> #include <errno.h>
> #include <unistd.h>
>
> +#include <libcamera/property_ids.h>
> +
> #include "libcamera/internal/log.h"
>
> using namespace libcamera;
> @@ -107,14 +109,12 @@ void V4L2Camera::requestComplete(Request *request)
> }
>
> int V4L2Camera::configure(StreamConfiguration *streamConfigOut,
> - const Size &size, const PixelFormat &pixelformat,
> - unsigned int bufferCount)
> + const Size &size, const PixelFormat &pixelformat)
> {
> StreamConfiguration &streamConfig = config_->at(0);
> streamConfig.size.width = size.width;
> streamConfig.size.height = size.height;
> streamConfig.pixelFormat = pixelformat;
> - streamConfig.bufferCount = bufferCount;
> /* \todo memoryType (interval vs external) */
>
> CameraConfiguration::Status validation = config_->validate();
> @@ -146,7 +146,6 @@ int V4L2Camera::validateConfiguration(const PixelFormat &pixelFormat,
> StreamConfiguration &cfg = config->at(0);
> cfg.size = size;
> cfg.pixelFormat = pixelFormat;
> - cfg.bufferCount = 1;
>
> CameraConfiguration::Status validation = config->validate();
> if (validation == CameraConfiguration::Invalid)
> @@ -299,3 +298,10 @@ bool V4L2Camera::isRunning()
> {
> return isRunning_;
> }
> +
> +int V4L2Camera::getBufCount(int count)
> +{
> + int min = camera_->properties().get(properties::QueueDepth);
> +
> + return std::max(count, min);
> +}
I'd name this function queueDepth() (we don't usually use a "get" prefix
for getters) and return the value of the property only. The caller can
then decide what to do with it.
> diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h
> index d238046250e3..68df8ad05917 100644
> --- a/src/v4l2/v4l2_camera.h
> +++ b/src/v4l2/v4l2_camera.h
> @@ -45,8 +45,7 @@ public:
> std::vector<Buffer> completedBuffers();
>
> int configure(StreamConfiguration *streamConfigOut,
> - const Size &size, const PixelFormat &pixelformat,
> - unsigned int bufferCount);
> + const Size &size, const PixelFormat &pixelformat);
> int validateConfiguration(const PixelFormat &pixelformat,
> const Size &size,
> StreamConfiguration *streamConfigOut);
> @@ -65,6 +64,8 @@ public:
>
> bool isRunning();
>
> + int getBufCount(int count);
> +
> private:
> void requestComplete(Request *request);
>
> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> index f8bfe595e90e..cd32e44a01ad 100644
> --- a/src/v4l2/v4l2_camera_proxy.cpp
> +++ b/src/v4l2/v4l2_camera_proxy.cpp
> @@ -348,8 +348,7 @@ int V4L2CameraProxy::vidioc_s_fmt(V4L2CameraFile *file, struct v4l2_format *arg)
> Size size(arg->fmt.pix.width, arg->fmt.pix.height);
> V4L2PixelFormat v4l2Format = V4L2PixelFormat(arg->fmt.pix.pixelformat);
> ret = vcam_->configure(&streamConfig_, size,
> - PixelFormatInfo::info(v4l2Format).format,
> - bufferCount_);
> + PixelFormatInfo::info(v4l2Format).format);
> if (ret < 0)
> return -EINVAL;
>
> @@ -490,14 +489,13 @@ int V4L2CameraProxy::vidioc_reqbufs(V4L2CameraFile *file, struct v4l2_requestbuf
> Size size(v4l2PixFormat_.width, v4l2PixFormat_.height);
> V4L2PixelFormat v4l2Format = V4L2PixelFormat(v4l2PixFormat_.pixelformat);
> int ret = vcam_->configure(&streamConfig_, size,
> - PixelFormatInfo::info(v4l2Format).format,
> - arg->count);
> + PixelFormatInfo::info(v4l2Format).format);
> if (ret < 0)
> return -EINVAL;
>
> setFmtFromConfig(streamConfig_);
>
> - arg->count = streamConfig_.bufferCount;
> + arg->count = vcam_->getBufCount(arg->count);
> bufferCount_ = arg->count;
>
> ret = vcam_->allocBuffers(arg->count);
> diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp
> index 61f4eb92ae95..f2c38bfb0b72 100644
> --- a/test/camera/buffer_import.cpp
> +++ b/test/camera/buffer_import.cpp
> @@ -12,6 +12,8 @@
> #include <numeric>
> #include <vector>
>
> +#include <libcamera/property_ids.h>
> +
> #include "libcamera/internal/device_enumerator.h"
> #include "libcamera/internal/event_dispatcher.h"
> #include "libcamera/internal/media_device.h"
> @@ -91,10 +93,12 @@ protected:
> return TestFail;
> }
>
> + unsigned int bufCount = camera_->properties().get(properties::QueueDepth);
Let's name the variable bufferCount or numBuffers for consistency.
There are a few issues to solve, but this is a really nice change !
> +
> Stream *stream = cfg.stream();
>
> BufferSource source;
> - int ret = source.allocate(cfg);
> + int ret = source.allocate(cfg, bufCount);
> if (ret != TestPass)
> return ret;
>
> @@ -138,10 +142,10 @@ protected:
> while (timer.isRunning())
> dispatcher->processEvents();
>
> - if (completeRequestsCount_ < cfg.bufferCount * 2) {
> + if (completeRequestsCount_ < bufCount * 2) {
> std::cout << "Failed to capture enough frames (got "
> << completeRequestsCount_ << " expected at least "
> - << cfg.bufferCount * 2 << ")" << std::endl;
> + << bufCount * 2 << ")" << std::endl;
> return TestFail;
> }
>
> diff --git a/test/libtest/buffer_source.cpp b/test/libtest/buffer_source.cpp
> index 73563f2fc39d..c3d5286a2462 100644
> --- a/test/libtest/buffer_source.cpp
> +++ b/test/libtest/buffer_source.cpp
> @@ -24,7 +24,7 @@ BufferSource::~BufferSource()
> media_->release();
> }
>
> -int BufferSource::allocate(const StreamConfiguration &config)
> +int BufferSource::allocate(const StreamConfiguration &config, unsigned int count)
> {
> /* Locate and open the video device. */
> std::string videoDeviceName = "vivid-000-vid-out";
> @@ -77,7 +77,7 @@ int BufferSource::allocate(const StreamConfiguration &config)
> return TestFail;
> }
>
> - if (video->allocateBuffers(config.bufferCount, &buffers_) < 0) {
> + if (video->allocateBuffers(count, &buffers_) < 0) {
> std::cout << "Failed to allocate buffers" << std::endl;
> return TestFail;
> }
> diff --git a/test/libtest/buffer_source.h b/test/libtest/buffer_source.h
> index 14b4770e8d8a..6a18e269a575 100644
> --- a/test/libtest/buffer_source.h
> +++ b/test/libtest/buffer_source.h
> @@ -20,7 +20,7 @@ public:
> BufferSource();
> ~BufferSource();
>
> - int allocate(const StreamConfiguration &config);
> + int allocate(const StreamConfiguration &config, unsigned int count);
> const std::vector<std::unique_ptr<FrameBuffer>> &buffers();
>
> private:
> diff --git a/test/v4l2_videodevice/buffer_cache.cpp b/test/v4l2_videodevice/buffer_cache.cpp
> index b3f2bec11783..07fddfd2617c 100644
> --- a/test/v4l2_videodevice/buffer_cache.cpp
> +++ b/test/v4l2_videodevice/buffer_cache.cpp
> @@ -10,6 +10,7 @@
> #include <vector>
>
> #include <libcamera/formats.h>
> +#include <libcamera/property_ids.h>
> #include <libcamera/stream.h>
>
> #include "buffer_source.h"
> @@ -145,10 +146,9 @@ public:
> StreamConfiguration cfg;
> cfg.pixelFormat = formats::YUYV;
> cfg.size = Size(600, 800);
> - cfg.bufferCount = numBuffers;
>
> BufferSource source;
> - int ret = source.allocate(cfg);
> + int ret = source.allocate(cfg, numBuffers);
> if (ret != TestPass)
> return ret;
>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list