| Message ID | 20210824195636.1110845-10-nfraprado@collabora.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
On Tue, Aug 24, 2021 at 04:56:28PM -0300, Nícolas F. R. A. Prado wrote: > Now that the number of buffers allocated by the FrameBufferAllocator > helper is passed through FrameBufferAllocator::allocate() and the > pipelines no longer use bufferCount for internal buffer or V4L2 buffer > slots allocation, we no longer need to have bufferCount in the > StreamConfiguration, so remove it. > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > > Changes in v8: > - Updated the pipeline-handler guide to use MinimumRequests instead of > bufferCount > - Removed kNumInternalBuffers as it was unused > > Changes in v6: > - Removed IPU3_BUFFER_COUNT as it was unused > > Documentation/guides/pipeline-handler.rst | 15 +++++++++------ > include/libcamera/stream.h | 2 -- > src/android/camera_stream.cpp | 2 +- > src/libcamera/pipeline/ipu3/cio2.cpp | 1 - > src/libcamera/pipeline/ipu3/cio2.h | 2 -- > src/libcamera/pipeline/ipu3/ipu3.cpp | 8 -------- > .../pipeline/raspberrypi/raspberrypi.cpp | 6 ------ > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 2 -- > src/libcamera/pipeline/rkisp1/rkisp1_path.h | 2 -- > src/libcamera/pipeline/simple/converter.cpp | 3 --- > src/libcamera/pipeline/simple/converter.h | 3 --- > src/libcamera/pipeline/simple/simple.cpp | 6 +----- > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 3 --- > src/libcamera/pipeline/vimc/vimc.cpp | 3 --- > src/libcamera/stream.cpp | 12 +++--------- > test/camera/buffer_import.cpp | 10 +++++++--- > test/libtest/buffer_source.cpp | 4 ++-- > test/libtest/buffer_source.h | 2 +- > test/v4l2_videodevice/buffer_cache.cpp | 3 +-- > 19 files changed, 25 insertions(+), 64 deletions(-) > > diff --git a/Documentation/guides/pipeline-handler.rst b/Documentation/guides/pipeline-handler.rst > index 2a69ef7d7461..3ee79b98c4dc 100644 > --- a/Documentation/guides/pipeline-handler.rst > +++ b/Documentation/guides/pipeline-handler.rst > @@ -826,14 +826,12 @@ As well as a list of supported StreamFormats, the StreamConfiguration is also > expected to provide an initialsed default configuration. This may be arbitrary, > but depending on use case you may which to select an output that matches the > Sensor output, or prefer a pixelformat which might provide higher performance on > -the hardware. The bufferCount represents the number of buffers required to > -support functional continuous processing on this stream. > +the hardware. > > .. code-block:: cpp > > cfg.pixelFormat = formats::BGR888; > cfg.size = { 1280, 720 }; > - cfg.bufferCount = 4; > > Finally add each ``StreamConfiguration`` generated to the > ``CameraConfiguration``, and ensure that it has been validated before returning > @@ -899,8 +897,6 @@ Add the following function implementation to your file: > status = Adjusted; > } > > - cfg.bufferCount = 4; > - > return status; > } > > @@ -1144,13 +1140,20 @@ is performed by using the ``V4L2VideoDevice`` API, which provides an > > .. _FrameBuffer: http://libcamera.org/api-html/classlibcamera_1_1FrameBuffer.html > > +The number passed to ``importBuffers()`` should be at least equal to the value > +of the ``MinimumRequests`` property in order to be possible to queue enough > +buffers to the video device that frames won't be dropped during capture. A > +bigger value can be advantageous to reduce the thrashing of dma-buf file > +descriptor mappings in case the application queues more requests and therefore > +improve performance, but for simplicity we'll just use ``MinimumRequests``. > + > Implement the pipeline handler ``start()`` function by replacing the stub > version with the following code: > > .. code-block:: c++ > > VividCameraData *data = cameraData(camera); > - unsigned int count = data->stream_.configuration().bufferCount; > + unsigned int count = camera->properties().get(properties::MinimumRequests); > > int ret = data->video_->importBuffers(count); > if (ret < 0) > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h > index 0c55e7164592..b25f0059f2f1 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/android/camera_stream.cpp b/src/android/camera_stream.cpp > index 29be1ac5ca4f..d9ee8842938f 100644 > --- a/src/android/camera_stream.cpp > +++ b/src/android/camera_stream.cpp > @@ -96,7 +96,7 @@ int CameraStream::configure() > buffers_.push_back(frameBuffer.get()); > } > > - camera3Stream_->max_buffers = configuration().bufferCount; > + camera3Stream_->max_buffers = bufferCount; > > return 0; > } > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp > index b940a0f6d7d6..c555ef5ffd27 100644 > --- a/src/libcamera/pipeline/ipu3/cio2.cpp > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp > @@ -214,7 +214,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/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h > index ab915b6a16fa..50ccb20765d3 100644 > --- a/src/libcamera/pipeline/ipu3/cio2.h > +++ b/src/libcamera/pipeline/ipu3/cio2.h > @@ -30,8 +30,6 @@ struct StreamConfiguration; > class CIO2Device > { > public: > - static constexpr unsigned int CIO2_BUFFER_COUNT = 4; > - > CIO2Device(); > > std::vector<PixelFormat> formats() const; > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index cc519ae6adbe..4a60f00ecf8b 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -39,7 +39,6 @@ namespace libcamera { > > LOG_DEFINE_CATEGORY(IPU3) > > -static constexpr unsigned int IPU3_BUFFER_COUNT = 4; > static constexpr unsigned int IPU3_MAX_STREAMS = 3; > static const Size IMGU_OUTPUT_MIN_SIZE = { 2, 2 }; > static const Size IMGU_OUTPUT_MAX_SIZE = { 4480, 34004 }; > @@ -313,7 +312,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_)); > @@ -357,7 +355,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); > > @@ -425,7 +422,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; > > @@ -446,7 +442,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; > @@ -456,7 +451,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(); > @@ -475,7 +469,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; > @@ -492,7 +485,6 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera, > StreamConfiguration cfg(formats); > cfg.size = size; > cfg.pixelFormat = pixelFormat; > - cfg.bufferCount = bufferCount; > config->addConfiguration(cfg); > } > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index a7c1cc1d5001..12d6729044e6 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -492,7 +492,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; > @@ -510,7 +509,6 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, > sensorFormat = findBestMode(fmts, size); > pixelFormat = sensorFormat.fourcc.toPixelFormat(); > ASSERT(pixelFormat.isValid()); > - bufferCount = 2; > rawCount++; > break; > > @@ -519,7 +517,6 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, > pixelFormat = formats::NV12; > /* Return the largest sensor resolution. */ > size = data->sensor_->resolution(); > - bufferCount = 1; > outCount++; > break; > > @@ -535,7 +532,6 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, > fmts = data->isp_[Isp::Output0].dev()->formats(); > pixelFormat = formats::YUV420; > size = { 1920, 1080 }; > - bufferCount = 4; > outCount++; > break; > > @@ -543,7 +539,6 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, > fmts = data->isp_[Isp::Output0].dev()->formats(); > pixelFormat = formats::ARGB8888; > size = { 800, 600 }; > - bufferCount = 4; > outCount++; > break; > > @@ -573,7 +568,6 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, > StreamConfiguration cfg(formats); > cfg.size = size; > cfg.pixelFormat = pixelFormat; > - cfg.bufferCount = bufferCount; > config->addConfiguration(cfg); > } > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > index 515f4be16d7e..9bbdf951d9b6 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/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > index 267d8f988ace..dd54fc609da6 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > @@ -57,8 +57,6 @@ public: > Signal<FrameBuffer *> &bufferReady() { return video_->bufferReady; } > > private: > - static constexpr unsigned int RKISP1_BUFFER_COUNT = 4; > - > const char *name_; > bool running_; > > diff --git a/src/libcamera/pipeline/simple/converter.cpp b/src/libcamera/pipeline/simple/converter.cpp > index 3133b3dbda07..46ab503b7c38 100644 > --- a/src/libcamera/pipeline/simple/converter.cpp > +++ b/src/libcamera/pipeline/simple/converter.cpp > @@ -89,9 +89,6 @@ int SimpleConverter::Stream::configure(const StreamConfiguration &inputCfg, > return -EINVAL; > } > > - inputBufferCount_ = inputCfg.bufferCount; > - outputBufferCount_ = outputCfg.bufferCount; > - > return 0; > } > > diff --git a/src/libcamera/pipeline/simple/converter.h b/src/libcamera/pipeline/simple/converter.h > index deb3df0d08df..365b99e9853e 100644 > --- a/src/libcamera/pipeline/simple/converter.h > +++ b/src/libcamera/pipeline/simple/converter.h > @@ -86,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 d0a658a23be8..6deba5d7dd61 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -252,7 +252,6 @@ protected: > int queueRequestDevice(Camera *camera, Request *request) override; > > private: > - static constexpr unsigned int kNumInternalBuffers = 3; > static constexpr unsigned int kSimpleBufferSlotCount = 16; > > SimpleCameraData *cameraData(Camera *camera) > @@ -638,7 +637,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); > @@ -656,8 +655,6 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() > cfg.stride = format.planes[0].bpl; > cfg.frameSize = format.planes[0].size; > } > - > - cfg.bufferCount = 3; > } > > return status; > @@ -780,7 +777,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; > > /* Set the MinimumRequests property. */ > unsigned int minimumRequests; > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > index c210cf57750f..5977312a795d 100644 > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > @@ -150,8 +150,6 @@ CameraConfiguration::Status UVCCameraConfiguration::validate() > status = Adjusted; > } > > - cfg.bufferCount = 4; > - > V4L2DeviceFormat format; > format.fourcc = data_->video_->toV4L2PixelFormat(cfg.pixelFormat); > format.size = cfg.size; > @@ -193,7 +191,6 @@ CameraConfiguration *PipelineHandlerUVC::generateConfiguration(Camera *camera, > > cfg.pixelFormat = formats.pixelformats().front(); > cfg.size = formats.sizes(cfg.pixelFormat).back(); > - cfg.bufferCount = 4; > > config->addConfiguration(cfg); > > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp > index d2943f61a745..ad71bfc67228 100644 > --- a/src/libcamera/pipeline/vimc/vimc.cpp > +++ b/src/libcamera/pipeline/vimc/vimc.cpp > @@ -170,8 +170,6 @@ CameraConfiguration::Status VimcCameraConfiguration::validate() > status = Adjusted; > } > > - cfg.bufferCount = 4; > - > V4L2DeviceFormat format; > format.fourcc = data_->video_->toV4L2PixelFormat(cfg.pixelFormat); > format.size = cfg.size; > @@ -227,7 +225,6 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera, > > cfg.pixelFormat = formats::BGR888; > cfg.size = { 1920, 1080 }; > - cfg.bufferCount = 4; > > config->addConfiguration(cfg); > > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp > index b421e17ecb36..d4cc0fafb76f 100644 > --- a/src/libcamera/stream.cpp > +++ b/src/libcamera/stream.cpp > @@ -280,8 +280,7 @@ SizeRange StreamFormats::range(const PixelFormat &pixelformat) const > * handlers provide StreamFormats. > */ > StreamConfiguration::StreamConfiguration() > - : pixelFormat(0), stride(0), frameSize(0), bufferCount(0), > - stream_(nullptr) > + : pixelFormat(0), stride(0), frameSize(0), stream_(nullptr) > { > } > > @@ -289,8 +288,8 @@ StreamConfiguration::StreamConfiguration() > * \brief Construct a configuration with stream formats > */ > StreamConfiguration::StreamConfiguration(const StreamFormats &formats) > - : pixelFormat(0), stride(0), frameSize(0), bufferCount(0), > - stream_(nullptr), formats_(formats) > + : pixelFormat(0), stride(0), frameSize(0), stream_(nullptr), > + formats_(formats) > { > } > > @@ -324,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/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp > index c504ea09e64b..67ac0ad20e15 100644 > --- a/test/camera/buffer_import.cpp > +++ b/test/camera/buffer_import.cpp > @@ -16,6 +16,8 @@ > #include <libcamera/base/thread.h> > #include <libcamera/base/timer.h> > > +#include <libcamera/property_ids.h> > + > #include "libcamera/internal/device_enumerator.h" > #include "libcamera/internal/media_device.h" > #include "libcamera/internal/v4l2_videodevice.h" > @@ -92,10 +94,12 @@ protected: > return TestFail; > } > > + unsigned int bufferCount = camera_->properties().get(properties::MinimumRequests); > + > Stream *stream = cfg.stream(); > > BufferSource source; > - int ret = source.allocate(cfg); > + int ret = source.allocate(cfg, bufferCount); > if (ret != TestPass) > return ret; > > @@ -139,10 +143,10 @@ protected: > while (timer.isRunning()) > dispatcher->processEvents(); > > - if (completeRequestsCount_ < cfg.bufferCount * 2) { > + if (completeRequestsCount_ < bufferCount * 2) { > std::cout << "Failed to capture enough frames (got " > << completeRequestsCount_ << " expected at least " > - << cfg.bufferCount * 2 << ")" << std::endl; > + << bufferCount * 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..6c8183800d0b 100644 > --- a/test/v4l2_videodevice/buffer_cache.cpp > +++ b/test/v4l2_videodevice/buffer_cache.cpp > @@ -145,10 +145,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; > > -- > 2.33.0 >
HI Nicolas, Thank you for your work. On Tue, 24 Aug 2021 at 20:57, Nícolas F. R. A. Prado < nfraprado@collabora.com> wrote: > Now that the number of buffers allocated by the FrameBufferAllocator > helper is passed through FrameBufferAllocator::allocate() and the > pipelines no longer use bufferCount for internal buffer or V4L2 buffer > slots allocation, we no longer need to have bufferCount in the > StreamConfiguration, so remove it. > I've tried to pull this series into my tree to have a closer look, but there are many merge conflicts. Am I missing some patches on-top of master that this series needs? Regarding the change, It looks like this will fail compilation with the Raspberry Pi pipeline handler. We use bufferCount in PipelineHandlerRPi::prepareBuffers() and it does not seem to be removed. I'm curious how we can update that logic, as our internal buffer allocation routine needs to know the external buffer count for the steram. Thanks, Naush > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > > --- > > Changes in v8: > - Updated the pipeline-handler guide to use MinimumRequests instead of > bufferCount > - Removed kNumInternalBuffers as it was unused > > Changes in v6: > - Removed IPU3_BUFFER_COUNT as it was unused > > Documentation/guides/pipeline-handler.rst | 15 +++++++++------ > include/libcamera/stream.h | 2 -- > src/android/camera_stream.cpp | 2 +- > src/libcamera/pipeline/ipu3/cio2.cpp | 1 - > src/libcamera/pipeline/ipu3/cio2.h | 2 -- > src/libcamera/pipeline/ipu3/ipu3.cpp | 8 -------- > .../pipeline/raspberrypi/raspberrypi.cpp | 6 ------ > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 2 -- > src/libcamera/pipeline/rkisp1/rkisp1_path.h | 2 -- > src/libcamera/pipeline/simple/converter.cpp | 3 --- > src/libcamera/pipeline/simple/converter.h | 3 --- > src/libcamera/pipeline/simple/simple.cpp | 6 +----- > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 3 --- > src/libcamera/pipeline/vimc/vimc.cpp | 3 --- > src/libcamera/stream.cpp | 12 +++--------- > test/camera/buffer_import.cpp | 10 +++++++--- > test/libtest/buffer_source.cpp | 4 ++-- > test/libtest/buffer_source.h | 2 +- > test/v4l2_videodevice/buffer_cache.cpp | 3 +-- > 19 files changed, 25 insertions(+), 64 deletions(-) > > diff --git a/Documentation/guides/pipeline-handler.rst > b/Documentation/guides/pipeline-handler.rst > index 2a69ef7d7461..3ee79b98c4dc 100644 > --- a/Documentation/guides/pipeline-handler.rst > +++ b/Documentation/guides/pipeline-handler.rst > @@ -826,14 +826,12 @@ As well as a list of supported StreamFormats, the > StreamConfiguration is also > expected to provide an initialsed default configuration. This may be > arbitrary, > but depending on use case you may which to select an output that matches > the > Sensor output, or prefer a pixelformat which might provide higher > performance on > -the hardware. The bufferCount represents the number of buffers required to > -support functional continuous processing on this stream. > +the hardware. > > .. code-block:: cpp > > cfg.pixelFormat = formats::BGR888; > cfg.size = { 1280, 720 }; > - cfg.bufferCount = 4; > > Finally add each ``StreamConfiguration`` generated to the > ``CameraConfiguration``, and ensure that it has been validated before > returning > @@ -899,8 +897,6 @@ Add the following function implementation to your file: > status = Adjusted; > } > > - cfg.bufferCount = 4; > - > return status; > } > > @@ -1144,13 +1140,20 @@ is performed by using the ``V4L2VideoDevice`` API, > which provides an > > .. _FrameBuffer: > http://libcamera.org/api-html/classlibcamera_1_1FrameBuffer.html > > +The number passed to ``importBuffers()`` should be at least equal to the > value > +of the ``MinimumRequests`` property in order to be possible to queue > enough > +buffers to the video device that frames won't be dropped during capture. A > +bigger value can be advantageous to reduce the thrashing of dma-buf file > +descriptor mappings in case the application queues more requests and > therefore > +improve performance, but for simplicity we'll just use > ``MinimumRequests``. > + > Implement the pipeline handler ``start()`` function by replacing the stub > version with the following code: > > .. code-block:: c++ > > VividCameraData *data = cameraData(camera); > - unsigned int count = data->stream_.configuration().bufferCount; > + unsigned int count = > camera->properties().get(properties::MinimumRequests); > > int ret = data->video_->importBuffers(count); > if (ret < 0) > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h > index 0c55e7164592..b25f0059f2f1 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/android/camera_stream.cpp b/src/android/camera_stream.cpp > index 29be1ac5ca4f..d9ee8842938f 100644 > --- a/src/android/camera_stream.cpp > +++ b/src/android/camera_stream.cpp > @@ -96,7 +96,7 @@ int CameraStream::configure() > buffers_.push_back(frameBuffer.get()); > } > > - camera3Stream_->max_buffers = configuration().bufferCount; > + camera3Stream_->max_buffers = bufferCount; > > return 0; > } > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp > b/src/libcamera/pipeline/ipu3/cio2.cpp > index b940a0f6d7d6..c555ef5ffd27 100644 > --- a/src/libcamera/pipeline/ipu3/cio2.cpp > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp > @@ -214,7 +214,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/cio2.h > b/src/libcamera/pipeline/ipu3/cio2.h > index ab915b6a16fa..50ccb20765d3 100644 > --- a/src/libcamera/pipeline/ipu3/cio2.h > +++ b/src/libcamera/pipeline/ipu3/cio2.h > @@ -30,8 +30,6 @@ struct StreamConfiguration; > class CIO2Device > { > public: > - static constexpr unsigned int CIO2_BUFFER_COUNT = 4; > - > CIO2Device(); > > std::vector<PixelFormat> formats() const; > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp > b/src/libcamera/pipeline/ipu3/ipu3.cpp > index cc519ae6adbe..4a60f00ecf8b 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -39,7 +39,6 @@ namespace libcamera { > > LOG_DEFINE_CATEGORY(IPU3) > > -static constexpr unsigned int IPU3_BUFFER_COUNT = 4; > static constexpr unsigned int IPU3_MAX_STREAMS = 3; > static const Size IMGU_OUTPUT_MIN_SIZE = { 2, 2 }; > static const Size IMGU_OUTPUT_MAX_SIZE = { 4480, 34004 }; > @@ -313,7 +312,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_)); > @@ -357,7 +355,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); > > @@ -425,7 +422,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; > > @@ -446,7 +442,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; > @@ -456,7 +451,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(); > @@ -475,7 +469,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; > @@ -492,7 +485,6 @@ CameraConfiguration > *PipelineHandlerIPU3::generateConfiguration(Camera *camera, > StreamConfiguration cfg(formats); > cfg.size = size; > cfg.pixelFormat = pixelFormat; > - cfg.bufferCount = bufferCount; > config->addConfiguration(cfg); > } > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index a7c1cc1d5001..12d6729044e6 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -492,7 +492,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; > @@ -510,7 +509,6 @@ CameraConfiguration > *PipelineHandlerRPi::generateConfiguration(Camera *camera, > sensorFormat = findBestMode(fmts, size); > pixelFormat = sensorFormat.fourcc.toPixelFormat(); > ASSERT(pixelFormat.isValid()); > - bufferCount = 2; > rawCount++; > break; > > @@ -519,7 +517,6 @@ CameraConfiguration > *PipelineHandlerRPi::generateConfiguration(Camera *camera, > pixelFormat = formats::NV12; > /* Return the largest sensor resolution. */ > size = data->sensor_->resolution(); > - bufferCount = 1; > outCount++; > break; > > @@ -535,7 +532,6 @@ CameraConfiguration > *PipelineHandlerRPi::generateConfiguration(Camera *camera, > fmts = data->isp_[Isp::Output0].dev()->formats(); > pixelFormat = formats::YUV420; > size = { 1920, 1080 }; > - bufferCount = 4; > outCount++; > break; > > @@ -543,7 +539,6 @@ CameraConfiguration > *PipelineHandlerRPi::generateConfiguration(Camera *camera, > fmts = data->isp_[Isp::Output0].dev()->formats(); > pixelFormat = formats::ARGB8888; > size = { 800, 600 }; > - bufferCount = 4; > outCount++; > break; > > @@ -573,7 +568,6 @@ CameraConfiguration > *PipelineHandlerRPi::generateConfiguration(Camera *camera, > StreamConfiguration cfg(formats); > cfg.size = size; > cfg.pixelFormat = pixelFormat; > - cfg.bufferCount = bufferCount; > config->addConfiguration(cfg); > } > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > index 515f4be16d7e..9bbdf951d9b6 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/rkisp1/rkisp1_path.h > b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > index 267d8f988ace..dd54fc609da6 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > @@ -57,8 +57,6 @@ public: > Signal<FrameBuffer *> &bufferReady() { return video_->bufferReady; > } > > private: > - static constexpr unsigned int RKISP1_BUFFER_COUNT = 4; > - > const char *name_; > bool running_; > > diff --git a/src/libcamera/pipeline/simple/converter.cpp > b/src/libcamera/pipeline/simple/converter.cpp > index 3133b3dbda07..46ab503b7c38 100644 > --- a/src/libcamera/pipeline/simple/converter.cpp > +++ b/src/libcamera/pipeline/simple/converter.cpp > @@ -89,9 +89,6 @@ int SimpleConverter::Stream::configure(const > StreamConfiguration &inputCfg, > return -EINVAL; > } > > - inputBufferCount_ = inputCfg.bufferCount; > - outputBufferCount_ = outputCfg.bufferCount; > - > return 0; > } > > diff --git a/src/libcamera/pipeline/simple/converter.h > b/src/libcamera/pipeline/simple/converter.h > index deb3df0d08df..365b99e9853e 100644 > --- a/src/libcamera/pipeline/simple/converter.h > +++ b/src/libcamera/pipeline/simple/converter.h > @@ -86,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 d0a658a23be8..6deba5d7dd61 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -252,7 +252,6 @@ protected: > int queueRequestDevice(Camera *camera, Request *request) override; > > private: > - static constexpr unsigned int kNumInternalBuffers = 3; > static constexpr unsigned int kSimpleBufferSlotCount = 16; > > SimpleCameraData *cameraData(Camera *camera) > @@ -638,7 +637,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); > @@ -656,8 +655,6 @@ CameraConfiguration::Status > SimpleCameraConfiguration::validate() > cfg.stride = format.planes[0].bpl; > cfg.frameSize = format.planes[0].size; > } > - > - cfg.bufferCount = 3; > } > > return status; > @@ -780,7 +777,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; > > /* Set the MinimumRequests property. */ > unsigned int minimumRequests; > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > index c210cf57750f..5977312a795d 100644 > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > @@ -150,8 +150,6 @@ CameraConfiguration::Status > UVCCameraConfiguration::validate() > status = Adjusted; > } > > - cfg.bufferCount = 4; > - > V4L2DeviceFormat format; > format.fourcc = data_->video_->toV4L2PixelFormat(cfg.pixelFormat); > format.size = cfg.size; > @@ -193,7 +191,6 @@ CameraConfiguration > *PipelineHandlerUVC::generateConfiguration(Camera *camera, > > cfg.pixelFormat = formats.pixelformats().front(); > cfg.size = formats.sizes(cfg.pixelFormat).back(); > - cfg.bufferCount = 4; > > config->addConfiguration(cfg); > > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp > b/src/libcamera/pipeline/vimc/vimc.cpp > index d2943f61a745..ad71bfc67228 100644 > --- a/src/libcamera/pipeline/vimc/vimc.cpp > +++ b/src/libcamera/pipeline/vimc/vimc.cpp > @@ -170,8 +170,6 @@ CameraConfiguration::Status > VimcCameraConfiguration::validate() > status = Adjusted; > } > > - cfg.bufferCount = 4; > - > V4L2DeviceFormat format; > format.fourcc = data_->video_->toV4L2PixelFormat(cfg.pixelFormat); > format.size = cfg.size; > @@ -227,7 +225,6 @@ CameraConfiguration > *PipelineHandlerVimc::generateConfiguration(Camera *camera, > > cfg.pixelFormat = formats::BGR888; > cfg.size = { 1920, 1080 }; > - cfg.bufferCount = 4; > > config->addConfiguration(cfg); > > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp > index b421e17ecb36..d4cc0fafb76f 100644 > --- a/src/libcamera/stream.cpp > +++ b/src/libcamera/stream.cpp > @@ -280,8 +280,7 @@ SizeRange StreamFormats::range(const PixelFormat > &pixelformat) const > * handlers provide StreamFormats. > */ > StreamConfiguration::StreamConfiguration() > - : pixelFormat(0), stride(0), frameSize(0), bufferCount(0), > - stream_(nullptr) > + : pixelFormat(0), stride(0), frameSize(0), stream_(nullptr) > { > } > > @@ -289,8 +288,8 @@ StreamConfiguration::StreamConfiguration() > * \brief Construct a configuration with stream formats > */ > StreamConfiguration::StreamConfiguration(const StreamFormats &formats) > - : pixelFormat(0), stride(0), frameSize(0), bufferCount(0), > - stream_(nullptr), formats_(formats) > + : pixelFormat(0), stride(0), frameSize(0), stream_(nullptr), > + formats_(formats) > { > } > > @@ -324,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/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp > index c504ea09e64b..67ac0ad20e15 100644 > --- a/test/camera/buffer_import.cpp > +++ b/test/camera/buffer_import.cpp > @@ -16,6 +16,8 @@ > #include <libcamera/base/thread.h> > #include <libcamera/base/timer.h> > > +#include <libcamera/property_ids.h> > + > #include "libcamera/internal/device_enumerator.h" > #include "libcamera/internal/media_device.h" > #include "libcamera/internal/v4l2_videodevice.h" > @@ -92,10 +94,12 @@ protected: > return TestFail; > } > > + unsigned int bufferCount = > camera_->properties().get(properties::MinimumRequests); > + > Stream *stream = cfg.stream(); > > BufferSource source; > - int ret = source.allocate(cfg); > + int ret = source.allocate(cfg, bufferCount); > if (ret != TestPass) > return ret; > > @@ -139,10 +143,10 @@ protected: > while (timer.isRunning()) > dispatcher->processEvents(); > > - if (completeRequestsCount_ < cfg.bufferCount * 2) { > + if (completeRequestsCount_ < bufferCount * 2) { > std::cout << "Failed to capture enough frames (got > " > << completeRequestsCount_ << " expected > at least " > - << cfg.bufferCount * 2 << ")" << > std::endl; > + << bufferCount * 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..6c8183800d0b 100644 > --- a/test/v4l2_videodevice/buffer_cache.cpp > +++ b/test/v4l2_videodevice/buffer_cache.cpp > @@ -145,10 +145,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; > > -- > 2.33.0 > >
On Thu, Dec 01, 2022 at 12:58:48PM +0000, Naushir Patuck wrote: > HI Nicolas, > > Thank you for your work. > > On Tue, 24 Aug 2021 at 20:57, Nícolas F. R. A. Prado < > nfraprado@collabora.com> wrote: > > > Now that the number of buffers allocated by the FrameBufferAllocator > > helper is passed through FrameBufferAllocator::allocate() and the > > pipelines no longer use bufferCount for internal buffer or V4L2 buffer > > slots allocation, we no longer need to have bufferCount in the > > StreamConfiguration, so remove it. > > > > I've tried to pull this series into my tree to have a closer look, but > there are many > merge conflicts. Am I missing some patches on-top of master that this > series needs? Hi, This series is one year old by this point, so it probably needs some tweaking to cleanly apply. I don't really have the time right now to work further on this series, but it looks like Paul Elder might carry on with it if I understood it right. (See reply on the cover letter) Thanks, Nícolas
diff --git a/Documentation/guides/pipeline-handler.rst b/Documentation/guides/pipeline-handler.rst index 2a69ef7d7461..3ee79b98c4dc 100644 --- a/Documentation/guides/pipeline-handler.rst +++ b/Documentation/guides/pipeline-handler.rst @@ -826,14 +826,12 @@ As well as a list of supported StreamFormats, the StreamConfiguration is also expected to provide an initialsed default configuration. This may be arbitrary, but depending on use case you may which to select an output that matches the Sensor output, or prefer a pixelformat which might provide higher performance on -the hardware. The bufferCount represents the number of buffers required to -support functional continuous processing on this stream. +the hardware. .. code-block:: cpp cfg.pixelFormat = formats::BGR888; cfg.size = { 1280, 720 }; - cfg.bufferCount = 4; Finally add each ``StreamConfiguration`` generated to the ``CameraConfiguration``, and ensure that it has been validated before returning @@ -899,8 +897,6 @@ Add the following function implementation to your file: status = Adjusted; } - cfg.bufferCount = 4; - return status; } @@ -1144,13 +1140,20 @@ is performed by using the ``V4L2VideoDevice`` API, which provides an .. _FrameBuffer: http://libcamera.org/api-html/classlibcamera_1_1FrameBuffer.html +The number passed to ``importBuffers()`` should be at least equal to the value +of the ``MinimumRequests`` property in order to be possible to queue enough +buffers to the video device that frames won't be dropped during capture. A +bigger value can be advantageous to reduce the thrashing of dma-buf file +descriptor mappings in case the application queues more requests and therefore +improve performance, but for simplicity we'll just use ``MinimumRequests``. + Implement the pipeline handler ``start()`` function by replacing the stub version with the following code: .. code-block:: c++ VividCameraData *data = cameraData(camera); - unsigned int count = data->stream_.configuration().bufferCount; + unsigned int count = camera->properties().get(properties::MinimumRequests); int ret = data->video_->importBuffers(count); if (ret < 0) diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h index 0c55e7164592..b25f0059f2f1 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/android/camera_stream.cpp b/src/android/camera_stream.cpp index 29be1ac5ca4f..d9ee8842938f 100644 --- a/src/android/camera_stream.cpp +++ b/src/android/camera_stream.cpp @@ -96,7 +96,7 @@ int CameraStream::configure() buffers_.push_back(frameBuffer.get()); } - camera3Stream_->max_buffers = configuration().bufferCount; + camera3Stream_->max_buffers = bufferCount; return 0; } diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp index b940a0f6d7d6..c555ef5ffd27 100644 --- a/src/libcamera/pipeline/ipu3/cio2.cpp +++ b/src/libcamera/pipeline/ipu3/cio2.cpp @@ -214,7 +214,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/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h index ab915b6a16fa..50ccb20765d3 100644 --- a/src/libcamera/pipeline/ipu3/cio2.h +++ b/src/libcamera/pipeline/ipu3/cio2.h @@ -30,8 +30,6 @@ struct StreamConfiguration; class CIO2Device { public: - static constexpr unsigned int CIO2_BUFFER_COUNT = 4; - CIO2Device(); std::vector<PixelFormat> formats() const; diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index cc519ae6adbe..4a60f00ecf8b 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -39,7 +39,6 @@ namespace libcamera { LOG_DEFINE_CATEGORY(IPU3) -static constexpr unsigned int IPU3_BUFFER_COUNT = 4; static constexpr unsigned int IPU3_MAX_STREAMS = 3; static const Size IMGU_OUTPUT_MIN_SIZE = { 2, 2 }; static const Size IMGU_OUTPUT_MAX_SIZE = { 4480, 34004 }; @@ -313,7 +312,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_)); @@ -357,7 +355,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); @@ -425,7 +422,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; @@ -446,7 +442,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; @@ -456,7 +451,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(); @@ -475,7 +469,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; @@ -492,7 +485,6 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera, StreamConfiguration cfg(formats); cfg.size = size; cfg.pixelFormat = pixelFormat; - cfg.bufferCount = bufferCount; config->addConfiguration(cfg); } diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index a7c1cc1d5001..12d6729044e6 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -492,7 +492,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; @@ -510,7 +509,6 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, sensorFormat = findBestMode(fmts, size); pixelFormat = sensorFormat.fourcc.toPixelFormat(); ASSERT(pixelFormat.isValid()); - bufferCount = 2; rawCount++; break; @@ -519,7 +517,6 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, pixelFormat = formats::NV12; /* Return the largest sensor resolution. */ size = data->sensor_->resolution(); - bufferCount = 1; outCount++; break; @@ -535,7 +532,6 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, fmts = data->isp_[Isp::Output0].dev()->formats(); pixelFormat = formats::YUV420; size = { 1920, 1080 }; - bufferCount = 4; outCount++; break; @@ -543,7 +539,6 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, fmts = data->isp_[Isp::Output0].dev()->formats(); pixelFormat = formats::ARGB8888; size = { 800, 600 }; - bufferCount = 4; outCount++; break; @@ -573,7 +568,6 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, StreamConfiguration cfg(formats); cfg.size = size; cfg.pixelFormat = pixelFormat; - cfg.bufferCount = bufferCount; config->addConfiguration(cfg); } diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp index 515f4be16d7e..9bbdf951d9b6 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/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h index 267d8f988ace..dd54fc609da6 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h @@ -57,8 +57,6 @@ public: Signal<FrameBuffer *> &bufferReady() { return video_->bufferReady; } private: - static constexpr unsigned int RKISP1_BUFFER_COUNT = 4; - const char *name_; bool running_; diff --git a/src/libcamera/pipeline/simple/converter.cpp b/src/libcamera/pipeline/simple/converter.cpp index 3133b3dbda07..46ab503b7c38 100644 --- a/src/libcamera/pipeline/simple/converter.cpp +++ b/src/libcamera/pipeline/simple/converter.cpp @@ -89,9 +89,6 @@ int SimpleConverter::Stream::configure(const StreamConfiguration &inputCfg, return -EINVAL; } - inputBufferCount_ = inputCfg.bufferCount; - outputBufferCount_ = outputCfg.bufferCount; - return 0; } diff --git a/src/libcamera/pipeline/simple/converter.h b/src/libcamera/pipeline/simple/converter.h index deb3df0d08df..365b99e9853e 100644 --- a/src/libcamera/pipeline/simple/converter.h +++ b/src/libcamera/pipeline/simple/converter.h @@ -86,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 d0a658a23be8..6deba5d7dd61 100644 --- a/src/libcamera/pipeline/simple/simple.cpp +++ b/src/libcamera/pipeline/simple/simple.cpp @@ -252,7 +252,6 @@ protected: int queueRequestDevice(Camera *camera, Request *request) override; private: - static constexpr unsigned int kNumInternalBuffers = 3; static constexpr unsigned int kSimpleBufferSlotCount = 16; SimpleCameraData *cameraData(Camera *camera) @@ -638,7 +637,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); @@ -656,8 +655,6 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate() cfg.stride = format.planes[0].bpl; cfg.frameSize = format.planes[0].size; } - - cfg.bufferCount = 3; } return status; @@ -780,7 +777,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; /* Set the MinimumRequests property. */ unsigned int minimumRequests; diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp index c210cf57750f..5977312a795d 100644 --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp @@ -150,8 +150,6 @@ CameraConfiguration::Status UVCCameraConfiguration::validate() status = Adjusted; } - cfg.bufferCount = 4; - V4L2DeviceFormat format; format.fourcc = data_->video_->toV4L2PixelFormat(cfg.pixelFormat); format.size = cfg.size; @@ -193,7 +191,6 @@ CameraConfiguration *PipelineHandlerUVC::generateConfiguration(Camera *camera, cfg.pixelFormat = formats.pixelformats().front(); cfg.size = formats.sizes(cfg.pixelFormat).back(); - cfg.bufferCount = 4; config->addConfiguration(cfg); diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp index d2943f61a745..ad71bfc67228 100644 --- a/src/libcamera/pipeline/vimc/vimc.cpp +++ b/src/libcamera/pipeline/vimc/vimc.cpp @@ -170,8 +170,6 @@ CameraConfiguration::Status VimcCameraConfiguration::validate() status = Adjusted; } - cfg.bufferCount = 4; - V4L2DeviceFormat format; format.fourcc = data_->video_->toV4L2PixelFormat(cfg.pixelFormat); format.size = cfg.size; @@ -227,7 +225,6 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera, cfg.pixelFormat = formats::BGR888; cfg.size = { 1920, 1080 }; - cfg.bufferCount = 4; config->addConfiguration(cfg); diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp index b421e17ecb36..d4cc0fafb76f 100644 --- a/src/libcamera/stream.cpp +++ b/src/libcamera/stream.cpp @@ -280,8 +280,7 @@ SizeRange StreamFormats::range(const PixelFormat &pixelformat) const * handlers provide StreamFormats. */ StreamConfiguration::StreamConfiguration() - : pixelFormat(0), stride(0), frameSize(0), bufferCount(0), - stream_(nullptr) + : pixelFormat(0), stride(0), frameSize(0), stream_(nullptr) { } @@ -289,8 +288,8 @@ StreamConfiguration::StreamConfiguration() * \brief Construct a configuration with stream formats */ StreamConfiguration::StreamConfiguration(const StreamFormats &formats) - : pixelFormat(0), stride(0), frameSize(0), bufferCount(0), - stream_(nullptr), formats_(formats) + : pixelFormat(0), stride(0), frameSize(0), stream_(nullptr), + formats_(formats) { } @@ -324,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/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp index c504ea09e64b..67ac0ad20e15 100644 --- a/test/camera/buffer_import.cpp +++ b/test/camera/buffer_import.cpp @@ -16,6 +16,8 @@ #include <libcamera/base/thread.h> #include <libcamera/base/timer.h> +#include <libcamera/property_ids.h> + #include "libcamera/internal/device_enumerator.h" #include "libcamera/internal/media_device.h" #include "libcamera/internal/v4l2_videodevice.h" @@ -92,10 +94,12 @@ protected: return TestFail; } + unsigned int bufferCount = camera_->properties().get(properties::MinimumRequests); + Stream *stream = cfg.stream(); BufferSource source; - int ret = source.allocate(cfg); + int ret = source.allocate(cfg, bufferCount); if (ret != TestPass) return ret; @@ -139,10 +143,10 @@ protected: while (timer.isRunning()) dispatcher->processEvents(); - if (completeRequestsCount_ < cfg.bufferCount * 2) { + if (completeRequestsCount_ < bufferCount * 2) { std::cout << "Failed to capture enough frames (got " << completeRequestsCount_ << " expected at least " - << cfg.bufferCount * 2 << ")" << std::endl; + << bufferCount * 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..6c8183800d0b 100644 --- a/test/v4l2_videodevice/buffer_cache.cpp +++ b/test/v4l2_videodevice/buffer_cache.cpp @@ -145,10 +145,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;
Now that the number of buffers allocated by the FrameBufferAllocator helper is passed through FrameBufferAllocator::allocate() and the pipelines no longer use bufferCount for internal buffer or V4L2 buffer slots allocation, we no longer need to have bufferCount in the StreamConfiguration, so remove it. Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> --- Changes in v8: - Updated the pipeline-handler guide to use MinimumRequests instead of bufferCount - Removed kNumInternalBuffers as it was unused Changes in v6: - Removed IPU3_BUFFER_COUNT as it was unused Documentation/guides/pipeline-handler.rst | 15 +++++++++------ include/libcamera/stream.h | 2 -- src/android/camera_stream.cpp | 2 +- src/libcamera/pipeline/ipu3/cio2.cpp | 1 - src/libcamera/pipeline/ipu3/cio2.h | 2 -- src/libcamera/pipeline/ipu3/ipu3.cpp | 8 -------- .../pipeline/raspberrypi/raspberrypi.cpp | 6 ------ src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 2 -- src/libcamera/pipeline/rkisp1/rkisp1_path.h | 2 -- src/libcamera/pipeline/simple/converter.cpp | 3 --- src/libcamera/pipeline/simple/converter.h | 3 --- src/libcamera/pipeline/simple/simple.cpp | 6 +----- src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 3 --- src/libcamera/pipeline/vimc/vimc.cpp | 3 --- src/libcamera/stream.cpp | 12 +++--------- test/camera/buffer_import.cpp | 10 +++++++--- test/libtest/buffer_source.cpp | 4 ++-- test/libtest/buffer_source.h | 2 +- test/v4l2_videodevice/buffer_cache.cpp | 3 +-- 19 files changed, 25 insertions(+), 64 deletions(-)