| Message ID | 20210824195636.1110845-1-nfraprado@collabora.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi Nicolas, Thank you for your work! The change looks good, just some (mostly cosmetic related) comments from me below. On Tue, 24 Aug 2021 at 20:57, Nícolas F. R. A. Prado < nfraprado@collabora.com> wrote: > Currently the raspberrypi pipeline handler relies on bufferCount to > decide on the number of buffers to allocate internally 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, in order to avoid thrashing dmabuf mappings, which would degrade > performance. > > Extend the Stream class to receive the number of internal buffers that > should be allocated, and optionally the number of buffer slots to > reserve. Use these new member variables to specify better suited numbers > for internal buffers and buffer slots for each stream individually, > which also allows us to remove bufferCount. > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > > --- > > Changes in v8: > - Reworked buffer allocation handling in the raspberrypi pipeline handler > - New > > .../pipeline/raspberrypi/raspberrypi.cpp | 45 +++++++++++-------- > .../pipeline/raspberrypi/rpi_stream.cpp | 28 +++++++++--- > .../pipeline/raspberrypi/rpi_stream.h | 24 ++++++++-- > 3 files changed, 69 insertions(+), 28 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 8a1040aa46be..a7c1cc1d5001 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -262,6 +262,25 @@ public: > bool match(DeviceEnumerator *enumerator) override; > > private: > + /* > + * The number of buffers to allocate internally for transferring > raw > + * frames from the Unicam Image stream to the ISP Input stream. It > is > + * defined such that: > + * - one buffer is being written to in Unicam Image > + * - one buffer is being processed in ISP Input > + * - one extra buffer is queued to Unicam Image > + */ > + static constexpr unsigned int kInternalRawBufferCount = 3; > + > + /* > + * The number of buffers to allocate internally for the external > + * streams. They're used to drop the first few frames while the IPA > + * algorithms converge. It is defined such that: > + * - one buffer is being used on the stream > + * - one extra buffer is queued > + */ > s/external/ISP since the Unicam RAW image can also be an external stream. Additionally, I would add that not only are these buffers used to drop the first few frames, but they are also used when the user does not supply a buffer in the Request. For example, if a low res is not requested by the application, we use an internal buffer since low res images are used for our colour denoise algorithm. > + static constexpr unsigned int kInternalDropoutBufferCount = 2; > + > I would probably rename these variables like: s/kInternalRawBufferCount/unicamInternalBufferCount/ s/kInternalDropoutBufferCount/ispInternalBufferCount/ I know that other libcamera source files use the "k" const prefix, but Raspberry Pi source files don't, so, I would drop it to be consistent. The reason for the rename is to more closely match the buffer usage as per the above comment. > RPiCameraData *cameraData(Camera *camera) > { > return static_cast<RPiCameraData *>(camera->_d()); > @@ -971,14 +990,14 @@ bool PipelineHandlerRPi::match(DeviceEnumerator > *enumerator) > return false; > > /* Locate and open the unicam video streams. */ > - data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam Embedded", > unicam_->getEntityByName("unicam-embedded")); > - data->unicam_[Unicam::Image] = RPi::Stream("Unicam Image", > unicam_->getEntityByName("unicam-image")); > + data->unicam_[Unicam::Embedded] = RPi::Stream("Unicam Embedded", > unicam_->getEntityByName("unicam-embedded"), kInternalRawBufferCount); > + data->unicam_[Unicam::Image] = RPi::Stream("Unicam Image", > unicam_->getEntityByName("unicam-image"), kInternalRawBufferCount); > > /* Tag the ISP input stream as an import stream. */ > - data->isp_[Isp::Input] = RPi::Stream("ISP Input", > isp_->getEntityByName("bcm2835-isp0-output0"), true); > - data->isp_[Isp::Output0] = RPi::Stream("ISP Output0", > isp_->getEntityByName("bcm2835-isp0-capture1")); > - data->isp_[Isp::Output1] = RPi::Stream("ISP Output1", > isp_->getEntityByName("bcm2835-isp0-capture2")); > - data->isp_[Isp::Stats] = RPi::Stream("ISP Stats", > isp_->getEntityByName("bcm2835-isp0-capture3")); > + data->isp_[Isp::Input] = RPi::Stream("ISP Input", > isp_->getEntityByName("bcm2835-isp0-output0"), 0, kInternalRawBufferCount, > true); > + data->isp_[Isp::Output0] = RPi::Stream("ISP Output0", > isp_->getEntityByName("bcm2835-isp0-capture1"), > kInternalDropoutBufferCount); > + data->isp_[Isp::Output1] = RPi::Stream("ISP Output1", > isp_->getEntityByName("bcm2835-isp0-capture2"), > kInternalDropoutBufferCount); > + data->isp_[Isp::Stats] = RPi::Stream("ISP Stats", > isp_->getEntityByName("bcm2835-isp0-capture3"), > kInternalDropoutBufferCount); > > Some of these lines are over the 120 character limit and will need wrapping. > /* Wire up all the buffer connections. */ > data->unicam_[Unicam::Image].dev()->frameStart.connect(data.get(), > &RPiCameraData::frameStarted); > @@ -1153,19 +1172,9 @@ int PipelineHandlerRPi::prepareBuffers(Camera > *camera) > RPiCameraData *data = cameraData(camera); > int ret; > > - /* > - * Decide how many internal buffers to allocate. For now, simply > look > - * at how many external buffers will be provided. We'll need to > improve > - * this logic. However, we really must have all streams allocate > the same > - * number of buffers to simplify error handling in > queueRequestDevice(). > - */ > - 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); > - > + /* Allocate internal buffers. */ > for (auto const stream : data->streams_) { > - ret = stream->prepareBuffers(maxBuffers); > + ret = stream->prepareBuffers(); > if (ret < 0) > return ret; > } > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > index b3265d0e8aab..98572abc2f61 100644 > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp > @@ -84,14 +84,24 @@ void Stream::removeExternalBuffer(FrameBuffer *buffer) > bufferMap_.erase(id); > } > > -int Stream::prepareBuffers(unsigned int count) > +int Stream::prepareBuffers() > { > + unsigned int slotCount; > int ret; > > if (!importOnly_) { > - if (count) { > + /* > + * External streams overallocate buffer slots in order to > handle > + * the buffers queued from applications without degrading > + * performance. Internal streams only need to have enough > buffer > + * slots to have the internal buffers queued. > + */ > + slotCount = external_ ? kRPIExternalBufferSlotCount > + : internalBufferCount_; > + > + if (internalBufferCount_) { > /* Export some frame buffers for internal use. */ > - ret = dev_->exportBuffers(count, > &internalBuffers_); > + ret = dev_->exportBuffers(internalBufferCount_, > &internalBuffers_); > if (ret < 0) > return ret; > > @@ -102,12 +112,16 @@ int Stream::prepareBuffers(unsigned int count) > for (auto const &buffer : internalBuffers_) > availableBuffers_.push(buffer.get()); > } > - > - /* We must import all internal/external exported buffers. > */ > - count = bufferMap_.size(); > + } else { > + /* > + * An importOnly stream doesn't have its own internal > buffers, > + * so it needs to have the number of buffer slots to > reserve > + * explicitly declared. > + */ > + slotCount = bufferSlotCount_; > } > > - return dev_->importBuffers(count); > + return dev_->importBuffers(slotCount); > } > > int Stream::queueBuffer(FrameBuffer *buffer) > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h > b/src/libcamera/pipeline/raspberrypi/rpi_stream.h > index f1ac715f4221..7310ba1cc371 100644 > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h > @@ -36,9 +36,13 @@ public: > { > } > > - Stream(const char *name, MediaEntity *dev, bool importOnly = false) > + Stream(const char *name, MediaEntity *dev, > + unsigned int internalBufferCount, > + unsigned int bufferSlotCount = 0, bool importOnly = false) > : external_(false), importOnly_(importOnly), name_(name), > - dev_(std::make_unique<V4L2VideoDevice>(dev)), > id_(ipa::RPi::MaskID) > + dev_(std::make_unique<V4L2VideoDevice>(dev)), > id_(ipa::RPi::MaskID), > + internalBufferCount_(internalBufferCount), > + bufferSlotCount_(bufferSlotCount) > I wonder if we can simplify this constructor by removing "bool importOnly = false"? In your patch, If a non-zero bufferSlotCount is provided, it implies importOnly == true. Could we use that to remove the latter? Perhaps renaming bufferSlotCount to importSlots to be more clear? > { > } > > @@ -57,7 +61,7 @@ public: > void setExternalBuffer(FrameBuffer *buffer); > void removeExternalBuffer(FrameBuffer *buffer); > > - int prepareBuffers(unsigned int count); > + int prepareBuffers(); > int queueBuffer(FrameBuffer *buffer); > void returnBuffer(FrameBuffer *buffer); > > @@ -65,6 +69,8 @@ public: > void releaseBuffers(); > > private: > + static constexpr unsigned int kRPIExternalBufferSlotCount = 16; > + > Similar to earlier, I would remove the "k" prefix. Regards, Naush > class IdGenerator > { > public: > @@ -127,6 +133,18 @@ private: > /* All frame buffers associated with this device stream. */ > BufferMap bufferMap_; > > + /* > + * The number of buffers that should be allocated internally for > this > + * stream. > + */ > + unsigned int internalBufferCount_; > + > + /* > + * The number of buffer slots that should be reserved for this > stream. > + * Only relevant for importOnly streams. > + */ > + unsigned int bufferSlotCount_; > + > /* > * List of frame buffers that we can use if none have been > provided by > * the application for external streams. This is populated by the > -- > 2.33.0 > >
On Tue, Aug 24, 2021 at 04:56:21PM -0300, Nícolas F. R. A. Prado wrote: > Make FrameBufferAllocator::allocate() require a 'count' argument for the > number of buffers to be allocated. > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > > Changes in v8: > - Updated application-developer and pipeline-handler guides with new allocate() > API and MinimumRequests property > - Added handling for when allocate() returns less buffers than needed in cam and > the capture unit test > - Improved FrameBufferAllocator::allocate() description > > Changes in v6: > - Changed static_cast to convert 'count' to unsigned int instead of > 'bufferCount' to int when comparing > > Changes in v5: > - Made sure that qcam allocates at least 2 buffers > > Documentation/guides/application-developer.rst | 9 +++++++-- > Documentation/guides/pipeline-handler.rst | 3 +-- > include/libcamera/camera.h | 2 +- > include/libcamera/framebuffer_allocator.h | 2 +- > include/libcamera/internal/pipeline_handler.h | 2 +- > src/android/camera_stream.cpp | 5 ++++- > src/cam/camera_session.cpp | 12 ++++++++---- > src/gstreamer/gstlibcameraallocator.cpp | 4 +++- > src/lc-compliance/simple_capture.cpp | 7 +++++-- > src/libcamera/camera.cpp | 4 ++-- > src/libcamera/framebuffer_allocator.cpp | 9 +++++++-- > src/libcamera/pipeline/ipu3/ipu3.cpp | 4 ++-- > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 4 ++-- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 4 ++-- > src/libcamera/pipeline/simple/simple.cpp | 4 ++-- > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 7 ++++--- > src/libcamera/pipeline/vimc/vimc.cpp | 7 ++++--- > src/libcamera/pipeline_handler.cpp | 1 + > src/qcam/main_window.cpp | 9 ++++++++- > src/v4l2/v4l2_camera.cpp | 2 +- > test/camera/camera_reconfigure.cpp | 4 +++- > test/camera/capture.cpp | 6 ++++-- > test/camera/statemachine.cpp | 4 +++- > test/mapped-buffer.cpp | 4 +++- > 24 files changed, 79 insertions(+), 40 deletions(-) > > diff --git a/Documentation/guides/application-developer.rst b/Documentation/guides/application-developer.rst > index fb6a80a57eb7..4dc8ac402ec6 100644 > --- a/Documentation/guides/application-developer.rst > +++ b/Documentation/guides/application-developer.rst > @@ -31,6 +31,7 @@ defined names and types without the need of prefixing them. > #include <memory> > > #include <libcamera/libcamera.h> > + #include <libcamera/property_ids.h> > > using namespace libcamera; > > @@ -260,7 +261,10 @@ Using the libcamera ``FrameBufferAllocator`` > > Applications create a ``FrameBufferAllocator`` for a Camera and use it > to allocate buffers for streams of a ``CameraConfiguration`` with the > -``allocate()`` function. > +``allocate()`` function. The number of buffers to be allocated needs to be > +specified, and should be at least equal to the value of the ``MinimumRequests`` > +property in order for the pipeline to have enough requests to be able to > +capture without frame drops. > > The list of allocated buffers can be retrieved using the ``Stream`` instance > as the parameter of the ``FrameBufferAllocator::buffers()`` function. > @@ -268,9 +272,10 @@ as the parameter of the ``FrameBufferAllocator::buffers()`` function. > .. code:: cpp > > FrameBufferAllocator *allocator = new FrameBufferAllocator(camera); > + unsigned int bufferCount = camera->properties().get(properties::MinimumRequests); > > for (StreamConfiguration &cfg : *config) { > - int ret = allocator->allocate(cfg.stream()); > + int ret = allocator->allocate(cfg.stream(), bufferCount); > if (ret < 0) { > std::cerr << "Can't allocate buffers" << std::endl; > return -ENOMEM; > diff --git a/Documentation/guides/pipeline-handler.rst b/Documentation/guides/pipeline-handler.rst > index 32e3fc518d91..2a69ef7d7461 100644 > --- a/Documentation/guides/pipeline-handler.rst > +++ b/Documentation/guides/pipeline-handler.rst > @@ -206,7 +206,7 @@ implementations for the overridden class members. > const StreamRoles &roles) override; > int configure(Camera *camera, CameraConfiguration *config) override; > > - int exportFrameBuffers(Camera *camera, Stream *stream, > + int exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count, > std::vector<std::unique_ptr<FrameBuffer>> *buffers) override; > > int start(Camera *camera, const ControlList *controls) override; > @@ -1186,7 +1186,6 @@ handle this: > > .. code-block:: cpp > > - unsigned int count = stream->configuration().bufferCount; > VividCameraData *data = cameraData(camera); > > return data->video_->exportBuffers(count, buffers); > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h > index 05cdab724b10..d52867db5137 100644 > --- a/include/libcamera/camera.h > +++ b/include/libcamera/camera.h > @@ -116,7 +116,7 @@ private: > void requestComplete(Request *request); > > friend class FrameBufferAllocator; > - int exportFrameBuffers(Stream *stream, > + int exportFrameBuffers(Stream *stream, unsigned int count, > std::vector<std::unique_ptr<FrameBuffer>> *buffers); > }; > > diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h > index cbc9ce101889..2d5a6e98e10c 100644 > --- a/include/libcamera/framebuffer_allocator.h > +++ b/include/libcamera/framebuffer_allocator.h > @@ -25,7 +25,7 @@ public: > FrameBufferAllocator(std::shared_ptr<Camera> camera); > ~FrameBufferAllocator(); > > - int allocate(Stream *stream); > + int allocate(Stream *stream, unsigned int count); > int free(Stream *stream); > > bool allocated() const { return !buffers_.empty(); } > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h > index 41cba44d990f..c6b3af18b1d3 100644 > --- a/include/libcamera/internal/pipeline_handler.h > +++ b/include/libcamera/internal/pipeline_handler.h > @@ -50,7 +50,7 @@ public: > const StreamRoles &roles) = 0; > virtual int configure(Camera *camera, CameraConfiguration *config) = 0; > > - virtual int exportFrameBuffers(Camera *camera, Stream *stream, > + virtual int exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count, > std::vector<std::unique_ptr<FrameBuffer>> *buffers) = 0; > > virtual int start(Camera *camera, const ControlList *controls) = 0; > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp > index 61b441830e18..29be1ac5ca4f 100644 > --- a/src/android/camera_stream.cpp > +++ b/src/android/camera_stream.cpp > @@ -15,6 +15,7 @@ > #include "jpeg/post_processor_jpeg.h" > > #include <libcamera/formats.h> > +#include <libcamera/property_ids.h> > > using namespace libcamera; > > @@ -83,8 +84,10 @@ int CameraStream::configure() > return ret; > } > > + unsigned int bufferCount = cameraDevice_->camera()->properties().get(properties::MinimumRequests); > + > if (allocator_) { > - int ret = allocator_->allocate(stream()); > + int ret = allocator_->allocate(stream(), bufferCount); > if (ret < 0) > return ret; > > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp > index 60d640f2b15c..d9a8500f390f 100644 > --- a/src/cam/camera_session.cpp > +++ b/src/cam/camera_session.cpp > @@ -237,17 +237,21 @@ int CameraSession::startCapture() > { > int ret; > > - /* Identify the stream with the least number of buffers. */ > - unsigned int nbuffers = UINT_MAX; > + unsigned int nbuffers = camera_->properties().get(properties::MinimumRequests); > + > for (StreamConfiguration &cfg : *config_) { > - ret = allocator_->allocate(cfg.stream()); > + ret = allocator_->allocate(cfg.stream(), nbuffers); > if (ret < 0) { > std::cerr << "Can't allocate buffers" << std::endl; > return -ENOMEM; > } > > unsigned int allocated = allocator_->buffers(cfg.stream()).size(); > - nbuffers = std::min(nbuffers, allocated); > + if (allocated < nbuffers) { > + std::cerr << "Unable to allocate enough buffers" > + << std::endl; > + return -ENOMEM; > + } > } > > /* > diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp > index 7bd8ba2db71d..8cc42edfaf61 100644 > --- a/src/gstreamer/gstlibcameraallocator.cpp > +++ b/src/gstreamer/gstlibcameraallocator.cpp > @@ -10,6 +10,7 @@ > > #include <libcamera/camera.h> > #include <libcamera/framebuffer_allocator.h> > +#include <libcamera/property_ids.h> > #include <libcamera/stream.h> > > #include "gstlibcamera-utils.h" > @@ -188,13 +189,14 @@ gst_libcamera_allocator_new(std::shared_ptr<Camera> camera, > { > auto *self = GST_LIBCAMERA_ALLOCATOR(g_object_new(GST_TYPE_LIBCAMERA_ALLOCATOR, > nullptr)); > + unsigned int bufferCount = camera->properties().get(properties::MinimumRequests); > > self->fb_allocator = new FrameBufferAllocator(camera); > for (StreamConfiguration &streamCfg : *config_) { > Stream *stream = streamCfg.stream(); > gint ret; > > - ret = self->fb_allocator->allocate(stream); > + ret = self->fb_allocator->allocate(stream, bufferCount); > if (ret == 0) > return nullptr; > > diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp > index 25097f28a603..d87f30cbeb1b 100644 > --- a/src/lc-compliance/simple_capture.cpp > +++ b/src/lc-compliance/simple_capture.cpp > @@ -7,6 +7,8 @@ > > #include <gtest/gtest.h> > > +#include <libcamera/property_ids.h> > + > #include "simple_capture.h" > > using namespace libcamera; > @@ -44,11 +46,12 @@ void SimpleCapture::configure(StreamRole role) > > void SimpleCapture::start() > { > + unsigned int bufferCount = camera_->properties().get(properties::MinimumRequests); > Stream *stream = config_->at(0).stream(); > - int count = allocator_->allocate(stream); > + int count = allocator_->allocate(stream, bufferCount); > > ASSERT_GE(count, 0) << "Failed to allocate buffers"; > - EXPECT_EQ(count, config_->at(0).bufferCount) << "Allocated less buffers than expected"; > + EXPECT_EQ(count, bufferCount) << "Allocated less buffers than expected"; > > camera_->requestCompleted.connect(this, &SimpleCapture::requestComplete); > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index c20e05014fb9..bea0d04ebd4e 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -691,7 +691,7 @@ void Camera::disconnect() > disconnected.emit(this); > } > > -int Camera::exportFrameBuffers(Stream *stream, > +int Camera::exportFrameBuffers(Stream *stream, unsigned int count, > std::vector<std::unique_ptr<FrameBuffer>> *buffers) > { > Private *const d = _d(); > @@ -708,7 +708,7 @@ int Camera::exportFrameBuffers(Stream *stream, > > return d->pipe_->invokeMethod(&PipelineHandler::exportFrameBuffers, > ConnectionTypeBlocking, this, stream, > - buffers); > + count, buffers); > } > > /** > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp > index 4df27cac0e94..8538240b4ae6 100644 > --- a/src/libcamera/framebuffer_allocator.cpp > +++ b/src/libcamera/framebuffer_allocator.cpp > @@ -71,6 +71,7 @@ FrameBufferAllocator::~FrameBufferAllocator() > /** > * \brief Allocate buffers for a configured stream > * \param[in] stream The stream to allocate buffers for > + * \param[in] count The number of buffers to allocate > * > * Allocate buffers suitable for capturing frames from the \a stream. The Camera > * shall have been previously configured with Camera::configure() and shall be > @@ -79,6 +80,10 @@ FrameBufferAllocator::~FrameBufferAllocator() > * Upon successful allocation, the allocated buffers can be retrieved with the > * buffers() function. > * > + * This function may allocate less buffers than requested, due to memory and > + * other system constraints. The caller shall always check the return value to > + * verify if the number of allocate buffers matches its needs. > + * > * \return The number of allocated buffers on success or a negative error code > * otherwise > * \retval -EACCES The camera is not in a state where buffers can be allocated > @@ -86,14 +91,14 @@ FrameBufferAllocator::~FrameBufferAllocator() > * not part of the active camera configuration > * \retval -EBUSY Buffers are already allocated for the \a stream > */ > -int FrameBufferAllocator::allocate(Stream *stream) > +int FrameBufferAllocator::allocate(Stream *stream, unsigned int count) > { > if (buffers_.count(stream)) { > LOG(Allocator, Error) << "Buffers already allocated for stream"; > return -EBUSY; > } > > - int ret = camera_->exportFrameBuffers(stream, &buffers_[stream]); > + int ret = camera_->exportFrameBuffers(stream, count, &buffers_[stream]); > if (ret == -EINVAL) > LOG(Allocator, Error) > << "Stream is not part of " << camera_->id() > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 2309609093cc..eaa503a33208 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -137,7 +137,7 @@ public: > const StreamRoles &roles) override; > int configure(Camera *camera, CameraConfiguration *config) override; > > - int exportFrameBuffers(Camera *camera, Stream *stream, > + int exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count, > std::vector<std::unique_ptr<FrameBuffer>> *buffers) override; > > int start(Camera *camera, const ControlList *controls) override; > @@ -670,10 +670,10 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c) > } > > int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream, > + unsigned int count, > std::vector<std::unique_ptr<FrameBuffer>> *buffers) > { > IPU3CameraData *data = cameraData(camera); > - unsigned int count = stream->configuration().bufferCount; > > if (stream == &data->outStream_) > return data->imgu_->output_->exportBuffers(count, buffers); > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index d35b9714f0c3..8a1040aa46be 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -251,7 +251,7 @@ public: > CameraConfiguration *generateConfiguration(Camera *camera, const StreamRoles &roles) override; > int configure(Camera *camera, CameraConfiguration *config) override; > > - int exportFrameBuffers(Camera *camera, Stream *stream, > + int exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count, > std::vector<std::unique_ptr<FrameBuffer>> *buffers) override; > > int start(Camera *camera, const ControlList *controls) override; > @@ -795,10 +795,10 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config) > } > > int PipelineHandlerRPi::exportFrameBuffers([[maybe_unused]] Camera *camera, Stream *stream, > + unsigned int count, > std::vector<std::unique_ptr<FrameBuffer>> *buffers) > { > RPi::Stream *s = static_cast<RPi::Stream *>(stream); > - unsigned int count = stream->configuration().bufferCount; > int ret = s->dev()->exportBuffers(count, buffers); > > s->setExportedBuffers(buffers); > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index cc663c983b3b..853a0ef42ba3 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -143,7 +143,7 @@ public: > const StreamRoles &roles) override; > int configure(Camera *camera, CameraConfiguration *config) override; > > - int exportFrameBuffers(Camera *camera, Stream *stream, > + int exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count, > std::vector<std::unique_ptr<FrameBuffer>> *buffers) override; > > int start(Camera *camera, const ControlList *controls) override; > @@ -674,10 +674,10 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > } > > int PipelineHandlerRkISP1::exportFrameBuffers([[maybe_unused]] Camera *camera, Stream *stream, > + unsigned int count, > std::vector<std::unique_ptr<FrameBuffer>> *buffers) > { > RkISP1CameraData *data = cameraData(camera); > - unsigned int count = stream->configuration().bufferCount; > > if (stream == &data->mainPathStream_) > return mainPath_.exportBuffers(count, buffers); > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index 26c59e601a76..737815452bae 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -236,7 +236,7 @@ public: > const StreamRoles &roles) override; > int configure(Camera *camera, CameraConfiguration *config) override; > > - int exportFrameBuffers(Camera *camera, Stream *stream, > + int exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count, > std::vector<std::unique_ptr<FrameBuffer>> *buffers) override; > > int start(Camera *camera, const ControlList *controls) override; > @@ -808,10 +808,10 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c) > } > > int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream, > + unsigned int count, > std::vector<std::unique_ptr<FrameBuffer>> *buffers) > { > SimpleCameraData *data = cameraData(camera); > - unsigned int count = stream->configuration().bufferCount; > > /* > * Export buffers on the converter or capture video node, depending on > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > index 75d57300555c..7821cacfa883 100644 > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > @@ -70,7 +70,7 @@ public: > const StreamRoles &roles) override; > int configure(Camera *camera, CameraConfiguration *config) override; > > - int exportFrameBuffers(Camera *camera, Stream *stream, > + int exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count, > std::vector<std::unique_ptr<FrameBuffer>> *buffers) override; > > int start(Camera *camera, const ControlList *controls) override; > @@ -223,11 +223,12 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config) > return 0; > } > > -int PipelineHandlerUVC::exportFrameBuffers(Camera *camera, Stream *stream, > +int PipelineHandlerUVC::exportFrameBuffers(Camera *camera, > + [[maybe_unused]] Stream *stream, > + unsigned int count, > std::vector<std::unique_ptr<FrameBuffer>> *buffers) > { > UVCCameraData *data = cameraData(camera); > - unsigned int count = stream->configuration().bufferCount; > > return data->video_->exportBuffers(count, buffers); > } > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp > index a0d8fd6c48c3..eebdfd1a4c01 100644 > --- a/src/libcamera/pipeline/vimc/vimc.cpp > +++ b/src/libcamera/pipeline/vimc/vimc.cpp > @@ -88,7 +88,7 @@ public: > const StreamRoles &roles) override; > int configure(Camera *camera, CameraConfiguration *config) override; > > - int exportFrameBuffers(Camera *camera, Stream *stream, > + int exportFrameBuffers(Camera *camera, Stream *stream, unsigned int count, > std::vector<std::unique_ptr<FrameBuffer>> *buffers) override; > > int start(Camera *camera, const ControlList *controls) override; > @@ -318,11 +318,12 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config) > return 0; > } > > -int PipelineHandlerVimc::exportFrameBuffers(Camera *camera, Stream *stream, > +int PipelineHandlerVimc::exportFrameBuffers(Camera *camera, > + [[maybe_unused]] Stream *stream, > + unsigned int count, > std::vector<std::unique_ptr<FrameBuffer>> *buffers) > { > VimcCameraData *data = cameraData(camera); > - unsigned int count = stream->configuration().bufferCount; > > return data->video_->exportBuffers(count, buffers); > } > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index 597d4c6a578a..ed4d62ae5c34 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -230,6 +230,7 @@ void PipelineHandler::unlock() > * \brief Allocate and export buffers for \a stream > * \param[in] camera The camera > * \param[in] stream The stream to allocate buffers for > + * \param[in] count The number of buffers to allocate > * \param[out] buffers Array of buffers successfully allocated > * > * This function allocates buffers for the \a stream from the devices associated > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp > index 1adaae60d83b..4089e47269a4 100644 > --- a/src/qcam/main_window.cpp > +++ b/src/qcam/main_window.cpp > @@ -25,6 +25,7 @@ > #include <QtDebug> > > #include <libcamera/camera_manager.h> > +#include <libcamera/property_ids.h> > #include <libcamera/version.h> > > #include "dng_writer.h" > @@ -463,7 +464,13 @@ int MainWindow::startCapture() > for (StreamConfiguration &config : *config_) { > Stream *stream = config.stream(); > > - ret = allocator_->allocate(stream); > + /* > + * We hold on to a buffer for display, so need one extra from > + * the minimum required for capture. > + */ > + unsigned int bufferCount = camera_->properties().get(properties::MinimumRequests) + 1; > + > + ret = allocator_->allocate(stream, bufferCount); > if (ret < 0) { > qWarning() << "Failed to allocate capture buffers"; > goto error; > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp > index 157ab94e0544..d01eacfa2b84 100644 > --- a/src/v4l2/v4l2_camera.cpp > +++ b/src/v4l2/v4l2_camera.cpp > @@ -161,7 +161,7 @@ int V4L2Camera::allocBuffers(unsigned int count) > { > Stream *stream = config_->at(0).stream(); > > - int ret = bufferAllocator_->allocate(stream); > + int ret = bufferAllocator_->allocate(stream, count); > if (ret < 0) > return ret; > > diff --git a/test/camera/camera_reconfigure.cpp b/test/camera/camera_reconfigure.cpp > index 5adef16e1c9e..00e6b113341b 100644 > --- a/test/camera/camera_reconfigure.cpp > +++ b/test/camera/camera_reconfigure.cpp > @@ -17,6 +17,7 @@ > #include <libcamera/base/timer.h> > > #include <libcamera/framebuffer_allocator.h> > +#include <libcamera/property_ids.h> > > #include "camera_test.h" > #include "test.h" > @@ -76,7 +77,8 @@ private: > * same buffer allocation for each run. > */ > if (!allocated_) { > - int ret = allocator_->allocate(stream); > + unsigned int bufferCount = camera_->properties().get(properties::MinimumRequests); > + int ret = allocator_->allocate(stream, bufferCount); > if (ret < 0) { > cerr << "Failed to allocate buffers" << endl; > return TestFail; > diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp > index 238d98dbba16..8d921fcb38f6 100644 > --- a/test/camera/capture.cpp > +++ b/test/camera/capture.cpp > @@ -8,6 +8,7 @@ > #include <iostream> > > #include <libcamera/framebuffer_allocator.h> > +#include <libcamera/property_ids.h> > > #include <libcamera/base/event_dispatcher.h> > #include <libcamera/base/thread.h> > @@ -96,8 +97,9 @@ protected: > > Stream *stream = cfg.stream(); > > - int ret = allocator_->allocate(stream); > - if (ret < 0) > + unsigned int bufferCount = camera_->properties().get(properties::MinimumRequests); > + int ret = allocator_->allocate(stream, bufferCount); > + if (ret < static_cast<int>(bufferCount)) > return TestFail; > > for (const std::unique_ptr<FrameBuffer> &buffer : allocator_->buffers(stream)) { > diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp > index 26fb5ca17139..70015bb56c74 100644 > --- a/test/camera/statemachine.cpp > +++ b/test/camera/statemachine.cpp > @@ -8,6 +8,7 @@ > #include <iostream> > > #include <libcamera/framebuffer_allocator.h> > +#include <libcamera/property_ids.h> > > #include "camera_test.h" > #include "test.h" > @@ -119,7 +120,8 @@ protected: > /* Use internally allocated buffers. */ > allocator_ = new FrameBufferAllocator(camera_); > Stream *stream = *camera_->streams().begin(); > - if (allocator_->allocate(stream) < 0) > + unsigned int bufferCount = camera_->properties().get(properties::MinimumRequests); > + if (allocator_->allocate(stream, bufferCount) < 0) > return TestFail; > > if (camera_->start()) > diff --git a/test/mapped-buffer.cpp b/test/mapped-buffer.cpp > index 97571945cbca..a5c56715a169 100644 > --- a/test/mapped-buffer.cpp > +++ b/test/mapped-buffer.cpp > @@ -8,6 +8,7 @@ > #include <iostream> > > #include <libcamera/framebuffer_allocator.h> > +#include <libcamera/property_ids.h> > > #include "libcamera/internal/mapped_framebuffer.h" > > @@ -54,7 +55,8 @@ protected: > > stream_ = cfg.stream(); > > - int ret = allocator_->allocate(stream_); > + unsigned int bufferCount = camera_->properties().get(properties::MinimumRequests); > + int ret = allocator_->allocate(stream_, bufferCount); > if (ret < 0) > return TestFail; > > -- > 2.33.0 >
On Tue, Aug 24, 2021 at 04:56:29PM -0300, Nícolas F. R. A. Prado wrote: > The capture_test.cpp file was added in the source list of meson in the > wrong place. Fix it so the list is alphabetically sorted. > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > > Changes in v8: > - New > > src/lc-compliance/meson.build | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build > index aa5852f6cb87..4be14b694426 100644 > --- a/src/lc-compliance/meson.build > +++ b/src/lc-compliance/meson.build > @@ -13,10 +13,10 @@ lc_compliance_enabled = true > lc_compliance_sources = files([ > '../cam/event_loop.cpp', > '../cam/options.cpp', > + 'capture_test.cpp', > 'environment.cpp', > 'main.cpp', > 'simple_capture.cpp', > - 'capture_test.cpp', > ]) > > lc_compliance = executable('lc-compliance', lc_compliance_sources, > -- > 2.33.0 >
On Tue, Aug 24, 2021 at 04:56:36PM -0300, Nícolas F. R. A. Prado wrote: > Add a test in lc-compliance to check that the MinimumRequests property > is set and valid, that is, greater than 0. > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > > Changes in v8: > - Moved RequiredProperties test to property_test.cpp > - Moved CameraTests to new test_base.{cpp,h} files > > Changes in v7: > - New > > src/lc-compliance/meson.build | 1 + > src/lc-compliance/property_test.cpp | 24 ++++++++++++++++++++++++ > src/lc-compliance/test_base.cpp | 10 ++++++++++ > src/lc-compliance/test_base.h | 7 +++++++ > 4 files changed, 42 insertions(+) > create mode 100644 src/lc-compliance/property_test.cpp > > diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build > index db593a6ec201..0c7e750b8264 100644 > --- a/src/lc-compliance/meson.build > +++ b/src/lc-compliance/meson.build > @@ -16,6 +16,7 @@ lc_compliance_sources = files([ > 'capture_test.cpp', > 'environment.cpp', > 'main.cpp', > + 'property_test.cpp', > 'simple_capture.cpp', > 'test_base.cpp', > ]) > diff --git a/src/lc-compliance/property_test.cpp b/src/lc-compliance/property_test.cpp > new file mode 100644 > index 000000000000..6a0951b2ce53 > --- /dev/null > +++ b/src/lc-compliance/property_test.cpp > @@ -0,0 +1,24 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > +/* > + * Copyright (C) 2021, Collabora Ltd. > + * > + * property_test.cpp - Test camera properties > + */ > + > +#include <libcamera/libcamera.h> > + > +#include <gtest/gtest.h> > + > +#include "test_base.h" > + > +using namespace libcamera; > + > +TEST_F(CameraTests, RequiredProperties) > +{ > + const ControlList &properties = camera_->properties(); > + > + using namespace properties; > + > + EXPECT_GT(properties.get(MinimumRequests), 0) > + << "Camera should have a positive value for MinimumRequests property"; > +} > diff --git a/src/lc-compliance/test_base.cpp b/src/lc-compliance/test_base.cpp > index c9957b9efd36..a5e64c8b8144 100644 > --- a/src/lc-compliance/test_base.cpp > +++ b/src/lc-compliance/test_base.cpp > @@ -26,3 +26,13 @@ void CameraHolder::releaseCamera() > camera_->release(); > camera_.reset(); > } > + > +void CameraTests::SetUp() > +{ > + acquireCamera(); > +} > + > +void CameraTests::TearDown() > +{ > + releaseCamera(); > +} > diff --git a/src/lc-compliance/test_base.h b/src/lc-compliance/test_base.h > index 52347749ab10..8125e1c60af7 100644 > --- a/src/lc-compliance/test_base.h > +++ b/src/lc-compliance/test_base.h > @@ -21,4 +21,11 @@ protected: > std::shared_ptr<libcamera::Camera> camera_; > }; > > +class CameraTests : public ::testing::Test, public CameraHolder > +{ > +protected: > + void SetUp() override; > + void TearDown() override; > +}; > + > #endif /* __LC_COMPLIANCE_TEST_BASE_H__ */ > -- > 2.33.0 >
On Tue, Aug 24, 2021 at 04:56:25PM -0300, Nícolas F. R. A. Prado wrote: > Currently the rkisp1 pipeline handler relies on bufferCount to decide on > the number of parameter and statistics buffers to allocate internally > 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@collabora.com> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > > Changes in v8: > - New > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 19 ++++++++++++------- > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 3 +-- > src/libcamera/pipeline/rkisp1/rkisp1_path.h | 2 ++ > 3 files changed, 15 insertions(+), 9 deletions(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 853a0ef42ba3..03a8d1d26e30 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -188,6 +188,16 @@ private: > std::queue<FrameBuffer *> availableStatBuffers_; > > Camera *activeCamera_; > + > + /* > + * This many internal buffers (or rather parameter and statistics buffer > + * pairs) ensures that the pipeline runs smoothly, without frame drops. > + * This number considers: > + * - three buffers queued to the driver, which is also the minimum > + * required to start streaming > + * - one buffer being processed on the IPA > + */ > + static constexpr unsigned int kRkISP1InternalBufferCount = 4; > }; > > RkISP1Frames::RkISP1Frames(PipelineHandler *pipe) > @@ -693,16 +703,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(kRkISP1InternalBufferCount, ¶mBuffers_); > if (ret < 0) > goto error; > > - ret = stat_->allocateBuffers(maxCount, &statBuffers_); > + ret = stat_->allocateBuffers(kRkISP1InternalBufferCount, &statBuffers_); > 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..515f4be16d7e 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > @@ -171,8 +171,7 @@ int RkISP1Path::start() > if (running_) > return -EBUSY; > > - /* \todo Make buffer count user configurable. */ > - ret = video_->importBuffers(RKISP1_BUFFER_COUNT); > + ret = video_->importBuffers(kRkISP1BufferSlotCount); > if (ret) > return ret; > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > index 91757600ccdc..267d8f988ace 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > @@ -69,6 +69,8 @@ private: > std::unique_ptr<V4L2Subdevice> resizer_; > std::unique_ptr<V4L2VideoDevice> video_; > MediaLink *link_; > + > + static constexpr unsigned int kRkISP1BufferSlotCount = 16; > }; > > class RkISP1MainPath : public RkISP1Path > -- > 2.33.0 >
On Tue, Aug 24, 2021 at 04:56:23PM -0300, Nícolas F. R. A. Prado wrote: > Currently the ipu3 pipeline handler relies on bufferCount to decide on > the number of V4L2 buffer slots to reserve as well as for the number of > buffers to allocate internally for the CIO2 raw buffers and > parameter-statistics ImgU buffer pairs. Instead, the number of internal > buffers should be the minimum required by the pipeline to keep the > requests flowing without frame drops, 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@collabora.com> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > > Changes in v8: > - New > > src/libcamera/pipeline/ipu3/cio2.cpp | 6 +++--- > src/libcamera/pipeline/ipu3/cio2.h | 18 +++++++++++++++++- > src/libcamera/pipeline/ipu3/imgu.cpp | 12 ++++++------ > src/libcamera/pipeline/ipu3/imgu.h | 15 ++++++++++++++- > src/libcamera/pipeline/ipu3/ipu3.cpp | 20 ++++++++------------ > 5 files changed, 48 insertions(+), 23 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp > index 1bcd580e251c..b940a0f6d7d6 100644 > --- a/src/libcamera/pipeline/ipu3/cio2.cpp > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp > @@ -225,13 +225,13 @@ int CIO2Device::exportBuffers(unsigned int count, > return output_->exportBuffers(count, buffers); > } > > -int CIO2Device::start() > +int CIO2Device::start(unsigned int bufferSlotCount) > { > - int ret = output_->exportBuffers(CIO2_BUFFER_COUNT, &buffers_); > + int ret = output_->exportBuffers(kCIO2InternalBufferCount, &buffers_); > if (ret < 0) > return ret; > > - ret = output_->importBuffers(CIO2_BUFFER_COUNT); > + ret = output_->importBuffers(bufferSlotCount); > if (ret) > LOG(IPU3, Error) << "Failed to import CIO2 buffers"; > > diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h > index f28e9f1ddf42..ab915b6a16fa 100644 > --- a/src/libcamera/pipeline/ipu3/cio2.h > +++ b/src/libcamera/pipeline/ipu3/cio2.h > @@ -45,7 +45,7 @@ public: > int exportBuffers(unsigned int count, > std::vector<std::unique_ptr<FrameBuffer>> *buffers); > > - int start(); > + int start(unsigned int bufferSlotCount); > int stop(); > > CameraSensor *sensor() { return sensor_.get(); } > @@ -69,6 +69,22 @@ private: > > std::vector<std::unique_ptr<FrameBuffer>> buffers_; > std::queue<FrameBuffer *> availableBuffers_; > + > + /* > + * This many internal buffers for the CIO2 ensures that the pipeline > + * runs smoothly, without frame drops. This number considers: > + * - one buffer being DMA'ed to in CIO2 > + * - one buffer programmed by the CIO2 as the next buffer > + * - one buffer under processing in ImgU > + * - one extra idle buffer queued to CIO2, to account for possible > + * delays in requeuing the buffer from ImgU back to CIO2 > + * > + * Transient situations can arise when one of the parts, CIO2 or ImgU, > + * finishes its processing first and experiences a lack of buffers, but > + * they will shortly after return to the state described above as the > + * other part catches up. > + */ > + static constexpr unsigned int kCIO2InternalBufferCount = 4; > }; > > } /* namespace libcamera */ > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp > index 317e482a1498..3484b378c189 100644 > --- a/src/libcamera/pipeline/ipu3/imgu.cpp > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp > @@ -593,22 +593,22 @@ int ImgUDevice::configureVideoDevice(V4L2VideoDevice *dev, unsigned int pad, > /** > * \brief Allocate buffers for all the ImgU video devices > */ > -int ImgUDevice::allocateBuffers(unsigned int bufferCount) > +int ImgUDevice::allocateBuffers(unsigned int bufferSlotCount) > { > /* Share buffers between CIO2 output and ImgU input. */ > - int ret = input_->importBuffers(bufferCount); > + int ret = input_->importBuffers(bufferSlotCount); > if (ret) { > LOG(IPU3, Error) << "Failed to import ImgU input buffers"; > return ret; > } > > - ret = param_->allocateBuffers(bufferCount, ¶mBuffers_); > + ret = param_->allocateBuffers(kImgUInternalBufferCount, ¶mBuffers_); > if (ret < 0) { > LOG(IPU3, Error) << "Failed to allocate ImgU param buffers"; > goto error; > } > > - ret = stat_->allocateBuffers(bufferCount, &statBuffers_); > + ret = stat_->allocateBuffers(kImgUInternalBufferCount, &statBuffers_); > if (ret < 0) { > LOG(IPU3, Error) << "Failed to allocate ImgU stat buffers"; > goto error; > @@ -619,13 +619,13 @@ int ImgUDevice::allocateBuffers(unsigned int bufferCount) > * corresponding stream is active or inactive, as the driver needs > * buffers to be requested on the V4L2 devices in order to operate. > */ > - ret = output_->importBuffers(bufferCount); > + ret = output_->importBuffers(bufferSlotCount); > if (ret < 0) { > LOG(IPU3, Error) << "Failed to import ImgU output buffers"; > goto error; > } > > - ret = viewfinder_->importBuffers(bufferCount); > + ret = viewfinder_->importBuffers(bufferSlotCount); > if (ret < 0) { > LOG(IPU3, Error) << "Failed to import ImgU viewfinder buffers"; > goto error; > diff --git a/src/libcamera/pipeline/ipu3/imgu.h b/src/libcamera/pipeline/ipu3/imgu.h > index 9d4915116087..61ccd17c2e31 100644 > --- a/src/libcamera/pipeline/ipu3/imgu.h > +++ b/src/libcamera/pipeline/ipu3/imgu.h > @@ -61,7 +61,7 @@ public: > outputFormat); > } > > - int allocateBuffers(unsigned int bufferCount); > + int allocateBuffers(unsigned int bufferSlotCount); > void freeBuffers(); > > int start(); > @@ -96,6 +96,19 @@ private: > > std::string name_; > MediaDevice *media_; > + > + /* > + * This many internal buffers (or rather parameter and statistics buffer > + * pairs) for the ImgU ensures that the pipeline runs smoothly, without > + * frame drops. This number considers: > + * - three buffers queued to the CIO2 (Since these buffers are bound to > + * CIO2 buffers before queuing to the CIO2) > + * - one buffer under processing in ImgU > + * > + * \todo Update this number when we make these buffers only get added to > + * the FrameInfo after the raw buffers are dequeued from CIO2. > + */ > + static constexpr unsigned int kImgUInternalBufferCount = 4; > }; > > } /* namespace libcamera */ > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index eaa503a33208..cc519ae6adbe 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -156,7 +156,7 @@ private: > int initControls(IPU3CameraData *data); > int registerCameras(); > > - int allocateBuffers(Camera *camera); > + int allocateBuffers(Camera *camera, unsigned int bufferSlotCount); > int freeBuffers(Camera *camera); > > ImgUDevice imgu0_; > @@ -165,6 +165,8 @@ private: > MediaDevice *imguMediaDev_; > > std::vector<IPABuffer> ipaBuffers_; > + > + static constexpr unsigned int kIPU3BufferSlotCount = 16; > }; > > IPU3CameraConfiguration::IPU3CameraConfiguration(IPU3CameraData *data) > @@ -693,20 +695,14 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream, > * In order to be able to start the 'viewfinder' and 'stat' nodes, we need > * memory to be reserved. > */ > -int PipelineHandlerIPU3::allocateBuffers(Camera *camera) > +int PipelineHandlerIPU3::allocateBuffers(Camera *camera, > + unsigned int bufferSlotCount) > { > IPU3CameraData *data = cameraData(camera); > ImgUDevice *imgu = data->imgu_; > - unsigned int bufferCount; > int ret; > > - bufferCount = std::max({ > - data->outStream_.configuration().bufferCount, > - data->vfStream_.configuration().bufferCount, > - data->rawStream_.configuration().bufferCount, > - }); > - > - ret = imgu->allocateBuffers(bufferCount); > + ret = imgu->allocateBuffers(bufferSlotCount); > if (ret < 0) > return ret; > > @@ -758,7 +754,7 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis > int ret; > > /* Allocate buffers for internal pipeline usage. */ > - ret = allocateBuffers(camera); > + ret = allocateBuffers(camera, kIPU3BufferSlotCount); > if (ret) > return ret; > > @@ -770,7 +766,7 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis > * Start the ImgU video devices, buffers will be queued to the > * ImgU output and viewfinder when requests will be queued. > */ > - ret = cio2->start(); > + ret = cio2->start(kIPU3BufferSlotCount); > if (ret) > goto error; > > -- > 2.33.0 >
Hi Nícolas, On Tue, Aug 24, 2021 at 04:56:19PM -0300, Nícolas F. R. A. Prado wrote: > The purpose of this series is to add a new test to lc-compliance that tests > queuing a lot of requests at once in order to ensure that pipeline handlers are > able to handle more requests than they have resources for (like internal buffers > and V4L2 buffer slots) [1]. Sorry for getting to this so late. I was considering that I would rebase this and send it again, since it's been so long; is that fine with you? Thanks, Paul > > [1] https://bugs.libcamera.org/show_bug.cgi?id=24 > > In order to achieve this, the FrameBufferAllocator had to be adapted to allow an > arbitrary number of buffers to be allocated. But there's also the issue of > reporting the minimum number of requests required by the pipeline handler, which > was solved by creating a new MinimumRequests property. > > Briefly, patches 1 through 9 rework the core and pipeline handlers to use > MinimumRequests and remove bufferCount, while patches 10 through 17 rework > lc-compliance to add the new test and an additional test for MinimumRequests. > > Patch 1 adds the new MinimumRequests property to report the minimum number of > requests needed by the pipeline handler. > > Patch 2 adds a count argument to allocate() so that the number of buffers to > allocate needs to be specified, as it is no longer assumed through bufferCount. > > Patches 3-7 decouple the number of internal buffers and V4L2 buffer slots from > bufferCount in each pipeline handler. > > Patch 8 reworks the V4L2 compatibility layer to not depend on bufferCount. > > Patch 9 removes bufferCount from the StreamConfiguration and everywhere it was > still used, as it is no longer needed. > > Patch 10 fixes a file ordering issue in lc-compliance's meson.build. > > Patches 11-14 does some refactoring in lc-compliance in order to reduce code > duplication. > > Patch 15 adds the test to lc-compliance. > > Patch 16 adds checks in lc-compliance to ensure that requests which failed to be > enqueued are reported as test failure. > > Patch 17 adds another, very short, test to lc-compliance to make sure that the > MinimumRequests property is set in the pipeline handler. > > I've run this new lc-compliance test on the raspberrypi, rkisp1, uvcvideo and > vimc pipelines. raspberrypi already handles it well, while the other three run > successfully after applying the series in [2]. The ipu3 should run fine as well > since the series in [2] was based on the internal queue already present there. > A patch for the simple pipeline is still pending. > > [2] https://lists.libcamera.org/pipermail/libcamera-devel/2021-July/022508.html > > The number of buffers to allocate in the lc-compliance test (patch 15) was > hardcoded to 8 since more than that would cause errors when allocating. > > v7: https://lists.libcamera.org/pipermail/libcamera-devel/2021-July/022577.html > v6: https://lists.libcamera.org/pipermail/libcamera-devel/2021-July/022356.html > v5: https://lists.libcamera.org/pipermail/libcamera-devel/2021-July/022098.html > v4: https://lists.libcamera.org/pipermail/libcamera-devel/2021-May/020150.html > v3: https://lists.libcamera.org/pipermail/libcamera-devel/2021-April/019613.html > v2: https://lists.libcamera.org/pipermail/libcamera-devel/2021-April/019398.html > v1: https://lists.libcamera.org/pipermail/libcamera-devel/2021-April/019139.html > > Changes in v8: > (thanks to Laurent and Kieran) > - Changed internal buffer count constants for pipeline handlers to better values > - Reordered patches to group non-lc-compliance changes together > - Split buffer allocation changes into separate commits for each pipeline > handler (patches 3-7) > - Changed the MinimumRequests property meaning to require that frames aren't > dropped > - Set MinimumRequests on SimplePipeline depending on device and usage of > converter > - Undid definition of default MinimumRequests on CameraSensor constructor > - Updated application-developer and pipeline-handler guides with new allocate() > API and MinimumRequests property > - Added handling for when allocate() returns less buffers than needed in cam and > the capture unit test > - Reworked buffer allocation handling in the raspberrypi pipeline handler > - Moved V4L2 compatibility layer changes to separate commit > - Added patch 10 to fix wrong file order in lc-compliance's meson.build > - Added requests_ member to SimpleCapture to hold ownership of queued > requests during capture > - Moved CameraHolder to new test_base.{cpp,h} files > - Fixed issue in UnbalancedStop test where requests cancelled due to stop() call > were failing the test > - Moved RequiredProperties test to property_test.cpp > - Moved CameraTests to new test_base.{cpp,h} files > > Changes in v7: > (thanks to Kieran and Jacopo) > - Renamed property from MinNumRequests to MinimumRequests > - Changed MinimumRequests property's description > - Added patch 11 to test if MinimumRequests is valid > > Changes in v6: > (thanks to Naushir) > - Fixed style issues > - Changed static_cast to unsigned int when comparing buffer count in > lc-compliance > - Added pipeline prefix to INTERNAL_BUFFER_COUNT and BUFFER_SLOT_COUNT constants > - Removed comment from Raspberrypi MinNumRequests setting > - Switched queueRequests()'s 'buffers' and 'requests' parameters order, since > 'requests' is an output variable > - Added comment to runCaptureSession() > > Changes in v5: > - Rebased on master (now that lc-compliance was refactored to use Googletest) > - Added patches 3, 5, 6 and 8 > - Fixed qcam to use at least two buffers > - Made sure that qcam allocates at least 2 buffers > > Changes in v4: > (thanks to Laurent and Niklas) > - Renamed QueueDepth property to MinNumRequests and better documented it > - Changed patch 6 to also remove bufferCount from android > - Added patch 3 to factor common code in lc-compliance > - Added patch 5 to remove pipeline dependency on bufferCount > > Changes in v3: > - Added patches 1 and 4 to add the QueueDepth property and remove bufferCount > - Made the count argument required in patch 2 > - Added previously missing changes to the gstreamer and V4L2 compatibility > layers > > Changes in v2: > - Renamed and reworded commits and series > - Dropped patches 2 and 3, which were hacks to test, and added patch 1 to add > count to FrameBufferAllocator > - Thanks to Niklas: > - Created new standalone test instead of looping over the other tests > > Nícolas F. R. A. Prado (17): > libcamera: property: Add MinimumRequests property > libcamera: framebuffer_allocator: Make allocate() require count > libcamera: pipeline: raspberrypi: Don't rely on bufferCount > libcamera: pipeline: ipu3: Don't rely on bufferCount > libcamera: pipeline: simple: Don't rely on bufferCount > libcamera: pipeline: rkisp1: Don't rely on bufferCount > libcamera: pipeline: vimc, uvcvideo: Don't rely on bufferCount > v4l2: Allocate buffers based on requested count and MinimumRequests > libcamera: stream: Remove bufferCount > lc-compliance: Fix source file ordering in meson.build > lc-compliance: Move buffer allocation to separate function > lc-compliance: Factor common capture code into SimpleCapture > lc-compliance: Move camera setup to CameraHolder class > lc-compliance: Move role to string conversion to its own function > lc-compliance: Add test to queue more requests than hardware depth > lc-compliance: Check that requests complete successfully > lc-compliance: Add test to ensure MinimumRequests is valid > > .../guides/application-developer.rst | 9 +- > Documentation/guides/pipeline-handler.rst | 40 +++-- > include/libcamera/camera.h | 2 +- > include/libcamera/framebuffer_allocator.h | 2 +- > include/libcamera/internal/pipeline_handler.h | 2 +- > include/libcamera/stream.h | 2 - > src/android/camera_stream.cpp | 7 +- > src/cam/camera_session.cpp | 12 +- > src/gstreamer/gstlibcameraallocator.cpp | 4 +- > src/lc-compliance/capture_test.cpp | 90 ++++++++--- > src/lc-compliance/meson.build | 4 +- > src/lc-compliance/property_test.cpp | 24 +++ > src/lc-compliance/simple_capture.cpp | 147 ++++++++++++------ > src/lc-compliance/simple_capture.h | 26 +++- > src/lc-compliance/test_base.cpp | 38 +++++ > src/lc-compliance/test_base.h | 31 ++++ > src/libcamera/camera.cpp | 4 +- > src/libcamera/framebuffer_allocator.cpp | 9 +- > src/libcamera/pipeline/ipu3/cio2.cpp | 7 +- > src/libcamera/pipeline/ipu3/cio2.h | 20 ++- > src/libcamera/pipeline/ipu3/imgu.cpp | 12 +- > src/libcamera/pipeline/ipu3/imgu.h | 15 +- > src/libcamera/pipeline/ipu3/ipu3.cpp | 34 ++-- > .../pipeline/raspberrypi/raspberrypi.cpp | 57 +++---- > .../pipeline/raspberrypi/rpi_stream.cpp | 28 +++- > .../pipeline/raspberrypi/rpi_stream.h | 24 ++- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 25 +-- > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 5 +- > src/libcamera/pipeline/rkisp1/rkisp1_path.h | 4 +- > src/libcamera/pipeline/simple/converter.cpp | 15 +- > src/libcamera/pipeline/simple/converter.h | 9 +- > src/libcamera/pipeline/simple/simple.cpp | 70 +++++++-- > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 17 +- > src/libcamera/pipeline/vimc/vimc.cpp | 17 +- > src/libcamera/pipeline_handler.cpp | 1 + > src/libcamera/property_ids.yaml | 21 +++ > src/libcamera/stream.cpp | 12 +- > src/qcam/main_window.cpp | 9 +- > src/v4l2/v4l2_camera.cpp | 22 ++- > src/v4l2/v4l2_camera.h | 5 +- > src/v4l2/v4l2_camera_proxy.cpp | 11 +- > test/camera/buffer_import.cpp | 10 +- > test/camera/camera_reconfigure.cpp | 4 +- > test/camera/capture.cpp | 6 +- > test/camera/statemachine.cpp | 4 +- > test/libtest/buffer_source.cpp | 4 +- > test/libtest/buffer_source.h | 2 +- > test/mapped-buffer.cpp | 4 +- > test/v4l2_videodevice/buffer_cache.cpp | 3 +- > 49 files changed, 655 insertions(+), 275 deletions(-) > create mode 100644 src/lc-compliance/property_test.cpp > create mode 100644 src/lc-compliance/test_base.cpp > create mode 100644 src/lc-compliance/test_base.h > > -- > 2.33.0 >
On Thu, Dec 01, 2022 at 09:32:02PM +0900, Paul Elder wrote: > Hi Nícolas, > > On Tue, Aug 24, 2021 at 04:56:19PM -0300, Nícolas F. R. A. Prado wrote: > > The purpose of this series is to add a new test to lc-compliance that tests > > queuing a lot of requests at once in order to ensure that pipeline handlers are > > able to handle more requests than they have resources for (like internal buffers > > and V4L2 buffer slots) [1]. > > Sorry for getting to this so late. I was considering that I would rebase > this and send it again, since it's been so long; is that fine with you? Hi, yes that would be fine by me, although I don't think I would have the time to work on this series further myself (and my brain has since let go of a lot of the concepts needed here)... Thanks, Nícolas
The purpose of this series is to add a new test to lc-compliance that tests queuing a lot of requests at once in order to ensure that pipeline handlers are able to handle more requests than they have resources for (like internal buffers and V4L2 buffer slots) [1]. [1] https://bugs.libcamera.org/show_bug.cgi?id=24 In order to achieve this, the FrameBufferAllocator had to be adapted to allow an arbitrary number of buffers to be allocated. But there's also the issue of reporting the minimum number of requests required by the pipeline handler, which was solved by creating a new MinimumRequests property. Briefly, patches 1 through 9 rework the core and pipeline handlers to use MinimumRequests and remove bufferCount, while patches 10 through 17 rework lc-compliance to add the new test and an additional test for MinimumRequests. Patch 1 adds the new MinimumRequests property to report the minimum number of requests needed by the pipeline handler. Patch 2 adds a count argument to allocate() so that the number of buffers to allocate needs to be specified, as it is no longer assumed through bufferCount. Patches 3-7 decouple the number of internal buffers and V4L2 buffer slots from bufferCount in each pipeline handler. Patch 8 reworks the V4L2 compatibility layer to not depend on bufferCount. Patch 9 removes bufferCount from the StreamConfiguration and everywhere it was still used, as it is no longer needed. Patch 10 fixes a file ordering issue in lc-compliance's meson.build. Patches 11-14 does some refactoring in lc-compliance in order to reduce code duplication. Patch 15 adds the test to lc-compliance. Patch 16 adds checks in lc-compliance to ensure that requests which failed to be enqueued are reported as test failure. Patch 17 adds another, very short, test to lc-compliance to make sure that the MinimumRequests property is set in the pipeline handler. I've run this new lc-compliance test on the raspberrypi, rkisp1, uvcvideo and vimc pipelines. raspberrypi already handles it well, while the other three run successfully after applying the series in [2]. The ipu3 should run fine as well since the series in [2] was based on the internal queue already present there. A patch for the simple pipeline is still pending. [2] https://lists.libcamera.org/pipermail/libcamera-devel/2021-July/022508.html The number of buffers to allocate in the lc-compliance test (patch 15) was hardcoded to 8 since more than that would cause errors when allocating. v7: https://lists.libcamera.org/pipermail/libcamera-devel/2021-July/022577.html v6: https://lists.libcamera.org/pipermail/libcamera-devel/2021-July/022356.html v5: https://lists.libcamera.org/pipermail/libcamera-devel/2021-July/022098.html v4: https://lists.libcamera.org/pipermail/libcamera-devel/2021-May/020150.html v3: https://lists.libcamera.org/pipermail/libcamera-devel/2021-April/019613.html v2: https://lists.libcamera.org/pipermail/libcamera-devel/2021-April/019398.html v1: https://lists.libcamera.org/pipermail/libcamera-devel/2021-April/019139.html Changes in v8: (thanks to Laurent and Kieran) - Changed internal buffer count constants for pipeline handlers to better values - Reordered patches to group non-lc-compliance changes together - Split buffer allocation changes into separate commits for each pipeline handler (patches 3-7) - Changed the MinimumRequests property meaning to require that frames aren't dropped - Set MinimumRequests on SimplePipeline depending on device and usage of converter - Undid definition of default MinimumRequests on CameraSensor constructor - Updated application-developer and pipeline-handler guides with new allocate() API and MinimumRequests property - Added handling for when allocate() returns less buffers than needed in cam and the capture unit test - Reworked buffer allocation handling in the raspberrypi pipeline handler - Moved V4L2 compatibility layer changes to separate commit - Added patch 10 to fix wrong file order in lc-compliance's meson.build - Added requests_ member to SimpleCapture to hold ownership of queued requests during capture - Moved CameraHolder to new test_base.{cpp,h} files - Fixed issue in UnbalancedStop test where requests cancelled due to stop() call were failing the test - Moved RequiredProperties test to property_test.cpp - Moved CameraTests to new test_base.{cpp,h} files Changes in v7: (thanks to Kieran and Jacopo) - Renamed property from MinNumRequests to MinimumRequests - Changed MinimumRequests property's description - Added patch 11 to test if MinimumRequests is valid Changes in v6: (thanks to Naushir) - Fixed style issues - Changed static_cast to unsigned int when comparing buffer count in lc-compliance - Added pipeline prefix to INTERNAL_BUFFER_COUNT and BUFFER_SLOT_COUNT constants - Removed comment from Raspberrypi MinNumRequests setting - Switched queueRequests()'s 'buffers' and 'requests' parameters order, since 'requests' is an output variable - Added comment to runCaptureSession() Changes in v5: - Rebased on master (now that lc-compliance was refactored to use Googletest) - Added patches 3, 5, 6 and 8 - Fixed qcam to use at least two buffers - Made sure that qcam allocates at least 2 buffers Changes in v4: (thanks to Laurent and Niklas) - Renamed QueueDepth property to MinNumRequests and better documented it - Changed patch 6 to also remove bufferCount from android - Added patch 3 to factor common code in lc-compliance - Added patch 5 to remove pipeline dependency on bufferCount Changes in v3: - Added patches 1 and 4 to add the QueueDepth property and remove bufferCount - Made the count argument required in patch 2 - Added previously missing changes to the gstreamer and V4L2 compatibility layers Changes in v2: - Renamed and reworded commits and series - Dropped patches 2 and 3, which were hacks to test, and added patch 1 to add count to FrameBufferAllocator - Thanks to Niklas: - Created new standalone test instead of looping over the other tests Nícolas F. R. A. Prado (17): libcamera: property: Add MinimumRequests property libcamera: framebuffer_allocator: Make allocate() require count libcamera: pipeline: raspberrypi: Don't rely on bufferCount libcamera: pipeline: ipu3: Don't rely on bufferCount libcamera: pipeline: simple: Don't rely on bufferCount libcamera: pipeline: rkisp1: Don't rely on bufferCount libcamera: pipeline: vimc, uvcvideo: Don't rely on bufferCount v4l2: Allocate buffers based on requested count and MinimumRequests libcamera: stream: Remove bufferCount lc-compliance: Fix source file ordering in meson.build lc-compliance: Move buffer allocation to separate function lc-compliance: Factor common capture code into SimpleCapture lc-compliance: Move camera setup to CameraHolder class lc-compliance: Move role to string conversion to its own function lc-compliance: Add test to queue more requests than hardware depth lc-compliance: Check that requests complete successfully lc-compliance: Add test to ensure MinimumRequests is valid .../guides/application-developer.rst | 9 +- Documentation/guides/pipeline-handler.rst | 40 +++-- include/libcamera/camera.h | 2 +- include/libcamera/framebuffer_allocator.h | 2 +- include/libcamera/internal/pipeline_handler.h | 2 +- include/libcamera/stream.h | 2 - src/android/camera_stream.cpp | 7 +- src/cam/camera_session.cpp | 12 +- src/gstreamer/gstlibcameraallocator.cpp | 4 +- src/lc-compliance/capture_test.cpp | 90 ++++++++--- src/lc-compliance/meson.build | 4 +- src/lc-compliance/property_test.cpp | 24 +++ src/lc-compliance/simple_capture.cpp | 147 ++++++++++++------ src/lc-compliance/simple_capture.h | 26 +++- src/lc-compliance/test_base.cpp | 38 +++++ src/lc-compliance/test_base.h | 31 ++++ src/libcamera/camera.cpp | 4 +- src/libcamera/framebuffer_allocator.cpp | 9 +- src/libcamera/pipeline/ipu3/cio2.cpp | 7 +- src/libcamera/pipeline/ipu3/cio2.h | 20 ++- src/libcamera/pipeline/ipu3/imgu.cpp | 12 +- src/libcamera/pipeline/ipu3/imgu.h | 15 +- src/libcamera/pipeline/ipu3/ipu3.cpp | 34 ++-- .../pipeline/raspberrypi/raspberrypi.cpp | 57 +++---- .../pipeline/raspberrypi/rpi_stream.cpp | 28 +++- .../pipeline/raspberrypi/rpi_stream.h | 24 ++- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 25 +-- src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 5 +- src/libcamera/pipeline/rkisp1/rkisp1_path.h | 4 +- src/libcamera/pipeline/simple/converter.cpp | 15 +- src/libcamera/pipeline/simple/converter.h | 9 +- src/libcamera/pipeline/simple/simple.cpp | 70 +++++++-- src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 17 +- src/libcamera/pipeline/vimc/vimc.cpp | 17 +- src/libcamera/pipeline_handler.cpp | 1 + src/libcamera/property_ids.yaml | 21 +++ src/libcamera/stream.cpp | 12 +- src/qcam/main_window.cpp | 9 +- src/v4l2/v4l2_camera.cpp | 22 ++- src/v4l2/v4l2_camera.h | 5 +- src/v4l2/v4l2_camera_proxy.cpp | 11 +- test/camera/buffer_import.cpp | 10 +- test/camera/camera_reconfigure.cpp | 4 +- test/camera/capture.cpp | 6 +- test/camera/statemachine.cpp | 4 +- test/libtest/buffer_source.cpp | 4 +- test/libtest/buffer_source.h | 2 +- test/mapped-buffer.cpp | 4 +- test/v4l2_videodevice/buffer_cache.cpp | 3 +- 49 files changed, 655 insertions(+), 275 deletions(-) create mode 100644 src/lc-compliance/property_test.cpp create mode 100644 src/lc-compliance/test_base.cpp create mode 100644 src/lc-compliance/test_base.h