[libcamera-devel] [PATCH v9 02/18] libcamera: property: Add MinimumRequests property
Jacopo Mondi
jacopo at jmondi.org
Wed Dec 21 10:09:28 CET 2022
Hi Paul
On Wed, Dec 21, 2022 at 04:13:38PM +0900, Paul Elder wrote:
> Hi Umang,
>
> On Tue, Dec 20, 2022 at 03:03:26PM +0530, Umang Jain wrote:
> > Hi Paul,
> >
> > On 12/20/22 2:34 PM, Paul Elder wrote:
> > > On Tue, Dec 20, 2022 at 02:04:15PM +0530, Umang Jain wrote:
> > > > Hi Paul,
> > > >
> > > > On 12/20/22 1:51 PM, Paul Elder via libcamera-devel wrote:
> > > > > Hi Jacopo,
> > > > >
> > > > > On Fri, Dec 16, 2022 at 03:00:59PM +0100, Jacopo Mondi wrote:
> > > > > > Hi Paul
> > > > > >
> > > > > > On Fri, Dec 16, 2022 at 09:29:23PM +0900, Paul Elder via libcamera-devel wrote:
> > > > > > > From: Nícolas F. R. A. Prado <nfraprado at collabora.com>
> > > > > > >
> > > > > > > The MinimumRequests property reports the minimum number of capture
> > > > > > > requests that the camera pipeline requires to have queued in order to
> > > > > > > sustain capture operations without frame drops. At this quantity,
> > > > > > > there's no guarantee that manual per-frame controls will apply
> > > > > > > correctly. The quantity needed for that may be added as a separate
> > > > > > > property in the future.
> > > > > > >
> > > > > > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado at collabora.com>
> > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > > > > > Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> > > > > > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > > > > > >
> > > > > > Seems quite on-spot considering the thread Umang has just started
> > > > > >
> > > > > > To be honest I would have expected this to generate more discussion,
> > > > > > but seeing Laurent's tag there makes me presume this has been blessed
> > > > > > already during the previous review.
> > > > > >
> > > > > >
> > > > > > > ---
> > > > > > > Changes in v9:
> > > > > > > - rebased
> > > > > > >
> > > > > > > Changes in v8:
> > > > > > > - 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 pipeline-handler guides with new MinimumRequests property
> > > > > > >
> > > > > > > Changes in v7:
> > > > > > > - Renamed property from MinNumRequests to MinimumRequests
> > > > > > > - Changed MinimumRequests property's description
> > > > > > >
> > > > > > > Changes in v6:
> > > > > > > - Removed comment from Raspberrypi MinNumRequests setting
> > > > > > > - Removed an extra blank line
> > > > > > > ---
> > > > > > > Documentation/guides/pipeline-handler.rst | 22 +++++++---
> > > > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +
> > > > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 2 +
> > > > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +
> > > > > > > src/libcamera/pipeline/simple/simple.cpp | 40 +++++++++++++++++--
> > > > > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 +
> > > > > > > src/libcamera/pipeline/vimc/vimc.cpp | 2 +
> > > > > > > src/libcamera/property_ids.yaml | 21 ++++++++++
> > > > > > > 8 files changed, 84 insertions(+), 9 deletions(-)
> > > > > > >
> > > > > > > diff --git a/Documentation/guides/pipeline-handler.rst b/Documentation/guides/pipeline-handler.rst
> > > > > > > index e1930fdf..a7356e4a 100644
> > > > > > > --- a/Documentation/guides/pipeline-handler.rst
> > > > > > > +++ b/Documentation/guides/pipeline-handler.rst
> > > > > > > @@ -658,19 +658,31 @@ associated with immutable values, which represent static characteristics that ca
> > > > > > > be used by applications to identify camera devices in the system. Properties can be
> > > > > > > registered by inspecting the values of V4L2 controls from the video devices and
> > > > > > > camera sensor (for example to retrieve the position and orientation of a camera)
> > > > > > > -or to express other immutable characteristics. The example pipeline handler does
> > > > > > > -not register any property, but examples are available in the libcamera code
> > > > > > > -base.
> > > > > > > +or to express other immutable characteristics.
> > > > > > >
> > > > > > > -.. TODO: Add a property example to the pipeline handler. At least the model.
> > > > > > > +A required property is ``MinimumRequests``, which indicates how many requests
> > > > > > > +need to be queued in the pipeline for capture without frame drops to be
> > > > > > > +possible.
> > > > > > > +
> > > > > > > +In our case, the vivid driver requires two buffers before it'll start streaming
> > > > > > > +(can be seen in the ``min_buffers_needed`` property for the ``vid_cap`` queue in
> > > > > > > +vivid's driver code). In order to not drop frames we should have one extra
> > > > > > > +buffer queued to the driver so that it is used as soon as the previous buffer
> > > > > > > +completes. Therefore we will set our ``MinimumRequests`` to three. Append the
> > > > > > > +following line to ``init()``:
> > > > > > The driver requires two buffers to operate, why an extra one is needed ?
> > > > > > For vivid in particular, where buffer's content is generated..
> > > > > I have a feeling that the intention was a spare buffer floating around
> > > > > in the application, but given the points that you've identified below,
> > > > > also maybe not.
> > > > >
> > > > > Also I feel like this description is mixing up Requests and buffers. I
> > > > > guess I should've vetted it a bit more (I was too distracted by merge
> > > > > conflicts).
> > > > >
> > > > > > > +
> > > > > > > +.. code-block:: cpp
> > > > > > > +
> > > > > > > + properties_.set(properties::MinimumRequests, 3);
> > > > > > >
> > > > > > > At this point you need to add the following includes to the top of the file for
> > > > > > > -handling controls:
> > > > > > > +handling controls and properties:
> > > > > > >
> > > > > > > .. code-block:: cpp
> > > > > > >
> > > > > > > #include <libcamera/controls.h>
> > > > > > > #include <libcamera/control_ids.h>
> > > > > > > + #include <libcamera/property_ids.h>
> > > > > > >
> > > > > > > Generating a default configuration
> > > > > > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > > > > index e4d79ea4..98a4a3e5 100644
> > > > > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > > > > @@ -1126,6 +1126,8 @@ int PipelineHandlerIPU3::registerCameras()
> > > > > > > /* Initialize the camera properties. */
> > > > > > > data->properties_ = cio2->sensor()->properties();
> > > > > > >
> > > > > > > + data->properties_.set(properties::MinimumRequests, 3);
> > > > > > > +
> > > > > > I see
> > > > > >
> > > > > > static constexpr unsigned int kBufferCount = 4;
> > > > > >
> > > > > > Which seems to suggest that at least 4 buffers need to be queued to
> > > > > > the video device before streaming is started.
> > > > > >
> > > > > > I wonder if I'm confusing the two concepts
> > > > > Gah, the ipu3 code is so convoluted.
> > > > >
> > > > > kBufferCount is the number of internal buffers to allocate on the CIO2.
> > > > I see kBufferCount being set to non-raw cfg->bufferCount in
> > > > IPU3CameraConfiguration::validate()
> > > ...I don't see it
> >
> > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/ipu3/ipu3.cpp#n371
>
> It's removed in patch 10/18 "libcamera: stream: Remove bufferCount"
>
So you removed that lines in a patch -after- this one, don't you ?
I guess what we both tried to express was "currently we require 4
buffers, you're changing it to 3. Does it work ? Is this intentional
?"
>
> Paul
>
> > >
> > > > > As far as I understand, they're only used if no buffer is provided in
> > > > > the Request.
> > > > >
> > > > > So yes, I think you are confusing the two concepts. MinimumRequests is
> > > > > the minumum number of Requests that need to be queued in order to
> > > > > sustain capture without frame drops. kBufferCount is the number of
> > > > > internal buffers to allocate on the CIO2.
> > > > >
> > > > > > > ret = initControls(data.get());
> > > > > > > if (ret)
> > > > > > > continue;
> > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > > > index 8569df17..4a08d01e 100644
> > > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > > > @@ -1063,6 +1063,8 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)
> > > > > > > /* Enable SOF event generation. */
> > > > > > > data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true);
> > > > > > >
> > > > > > > + data->properties_.set(properties::MinimumRequests, 3);
> > > > > > > +
> > > > > > > /*
> > > > > > > * Reset the delayed controls with the gain and exposure values set by
> > > > > > > * the IPA.
> > > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > > > > index f658d75e..45b6beaf 100644
> > > > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > > > > @@ -23,6 +23,7 @@
> > > > > > > #include <libcamera/control_ids.h>
> > > > > > > #include <libcamera/formats.h>
> > > > > > > #include <libcamera/framebuffer.h>
> > > > > > > +#include <libcamera/property_ids.h>
> > > > > > > #include <libcamera/request.h>
> > > > > > > #include <libcamera/stream.h>
> > > > > > > #include <libcamera/ipa/core_ipa_interface.h>
> > > > > > > @@ -1102,6 +1103,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> > > > > > >
> > > > > > > /* Initialize the camera properties. */
> > > > > > > data->properties_ = data->sensor_->properties();
> > > > > > > + data->properties_.set(properties::MinimumRequests, 3);
> > > > > > Same question for RkISP1, it seems we don't even populate
> > > > > > StreamConfiguration::bufferCount there, but I'm pretty sure you need 4
> > > > > > requests queued before capture can start..
> > > You only need 3.
> > >
> > > > > I see internal buffers but only for params and stats...
> > > > >
> > > > > I guess I have to check this again too then.
> > > > I think MinimumRequests and minimum no. of buffers required to start and
> > > > sustain capture - are closely related. More so, because you can't queue
> > > > buffers arbitrarily (from application's PoV) - capture buffers come _via_
> > > > queuing requests.
> > > That's true.
> > >
> > > > The internal buffers, I always understood as something not provided by
> > > > applications like stats and params - completely internal to libcamera. The
> > > > minimumRequests property/criteria should not hinder/apply to internal
> > > > buffers right ?
> > > Yeah, they shouldn't apply.
> > >
> > >
> > > Paul
> > >
> > > > > > > /*
> > > > > > > * \todo Read dealy values from the sensor itself or from a
> > > > > > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > > > > > > index 24ded4db..8ed983fe 100644
> > > > > > > --- a/src/libcamera/pipeline/simple/simple.cpp
> > > > > > > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > > > > > > @@ -25,6 +25,7 @@
> > > > > > >
> > > > > > > #include <libcamera/camera.h>
> > > > > > > #include <libcamera/control_ids.h>
> > > > > > > +#include <libcamera/property_ids.h>
> > > > > > > #include <libcamera/request.h>
> > > > > > > #include <libcamera/stream.h>
> > > > > > >
> > > > > > > @@ -180,6 +181,10 @@ class SimplePipelineHandler;
> > > > > > >
> > > > > > > struct SimplePipelineInfo {
> > > > > > > const char *driver;
> > > > > > > + /*
> > > > > > > + * Minimum number of buffers required by the driver to start streaming.
> > > > > > > + */
> > > > > > > + unsigned int minimumBuffers;
> > > > > > > /*
> > > > > > > * Each converter in the list contains the name
> > > > > > > * and the number of streams it supports.
> > > > > > > @@ -190,10 +195,10 @@ struct SimplePipelineInfo {
> > > > > > > namespace {
> > > > > > >
> > > > > > > static const SimplePipelineInfo supportedDevices[] = {
> > > > > > > - { "imx7-csi", { { "pxp", 1 } } },
> > > > > > > - { "mxc-isi", {} },
> > > > > > > - { "qcom-camss", {} },
> > > > > > > - { "sun6i-csi", {} },
> > > > > > > + { "imx7-csi", 2, { { "pxp", 1 } } },
> > > > > > > + { "mxc-isi", 3, {} },
> > > > > > > + { "qcom-camss", 1, {} },
> > > > > > > + { "sun6i-csi", 3, {} },
> > > > > > > };
> > > > > > >
> > > > > > > } /* namespace */
> > > > > > > @@ -271,6 +276,8 @@ public:
> > > > > > > bool useConverter_;
> > > > > > > std::queue<std::map<unsigned int, FrameBuffer *>> converterQueue_;
> > > > > > >
> > > > > > > + const SimplePipelineInfo *deviceInfo;
> > > > > > > +
> > > > > > > private:
> > > > > > > void tryPipeline(unsigned int code, const Size &size);
> > > > > > > static std::vector<const MediaPad *> routedSourcePads(MediaPad *sink);
> > > > > > > @@ -1168,6 +1175,29 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
> > > > > > > inputCfg.stride = captureFormat.planes[0].bpl;
> > > > > > > inputCfg.bufferCount = kNumInternalBuffers;
> > > > > > >
> > > > > > > + /* Set the MinimumRequests property. */
> > > > > > > + unsigned int minimumRequests;
> > > > > > > +
> > > > > > > + if (data->useConverter_) {
> > > > > > > + /*
> > > > > > > + * The application will interact only with the capture node of
> > > > > > > + * the converter. Require two buffers for a frame drop free
> > > > > > > + * conversion, plus one extra to account for requeue delays.
> > > > > > > + */
> > > > > > > + minimumRequests = 3;
> > > > > > > + } else {
> > > > > > > + /*
> > > > > > > + * The application will interact directly with the video capture
> > > > > > > + * device. Require the minimum required by the driver, plus one
> > > > > > > + * extra to account for requeue delays. Force at least three
> > > > > > > + * buffers in order to not drop frames.
> > > > > > > + */
> > > > > > > + minimumRequests = std::max(data->deviceInfo->minimumBuffers + 1,
> > > > > > > + 3U);
> > > > > > > + }
> > > > > > > +
> > > > > > > + data->properties_.set(properties::MinimumRequests, minimumRequests);
> > > > > > > +
> > > > > > > return data->converter_->configure(inputCfg, outputCfgs);
> > > > > > > }
> > > > > > >
> > > > > > > @@ -1506,6 +1536,8 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
> > > > > > > bool registered = false;
> > > > > > >
> > > > > > > for (std::unique_ptr<SimpleCameraData> &data : pipelines) {
> > > > > > > + data->deviceInfo = info;
> > > > > > > +
> > > > > > > int ret = data->init();
> > > > > > > if (ret < 0)
> > > > > > > continue;
> > > > > > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > > > > > index 277465b7..7f580955 100644
> > > > > > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > > > > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > > > > > @@ -500,6 +500,8 @@ int UVCCameraData::init(MediaDevice *media)
> > > > > > > properties_.set(properties::PixelArraySize, resolution);
> > > > > > > properties_.set(properties::PixelArrayActiveAreas, { Rectangle(resolution) });
> > > > > > >
> > > > > > > + properties_.set(properties::MinimumRequests, 3);
> > > > > > > +
> > > > > > > /* Initialise the supported controls. */
> > > > > > > ControlInfoMap::Map ctrls;
> > > > > > >
> > > > > > > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> > > > > > > index 204f5ad7..d2633be4 100644
> > > > > > > --- a/src/libcamera/pipeline/vimc/vimc.cpp
> > > > > > > +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> > > > > > > @@ -21,6 +21,7 @@
> > > > > > > #include <libcamera/control_ids.h>
> > > > > > > #include <libcamera/controls.h>
> > > > > > > #include <libcamera/formats.h>
> > > > > > > +#include <libcamera/property_ids.h>
> > > > > > > #include <libcamera/request.h>
> > > > > > > #include <libcamera/stream.h>
> > > > > > >
> > > > > > > @@ -571,6 +572,7 @@ int VimcCameraData::init()
> > > > > > >
> > > > > > > /* Initialize the camera properties. */
> > > > > > > properties_ = sensor_->properties();
> > > > > > > + properties_.set(properties::MinimumRequests, 3);
> > > > > > >
> > > > > > > return 0;
> > > > > > > }
> > > > > > > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
> > > > > > > index cb55e0ed..c82ac17e 100644
> > > > > > > --- a/src/libcamera/property_ids.yaml
> > > > > > > +++ b/src/libcamera/property_ids.yaml
> > > > > > > @@ -690,6 +690,27 @@ controls:
> > > > > > > that is twice that of the full resolution mode. This value will be valid
> > > > > > > after the configure method has returned successfully.
> > > > > > >
> > > > > > > + - MinimumRequests:
> > > > > > > + type: int32_t
> > > > > > > + description: |
> > > > > > > + The minimum number of capture requests that the camera pipeline requires
> > > > > > > + to have queued in order to sustain capture operations without frame
> > > > > > > + drops. At this quantity, there's no guarantee that manual per-frame
> > > > > > > + controls will apply correctly.
> > > > > > This property is also relevant for the startup phase, so mentioning
> > > > > > that this is the minium number of requests that need to be queued
> > > > > > before capture starts might be necessary ?
> > > > > Indeed.
> > > > >
> > > > >
> > > > > Thanks,
> > > > >
> > > > > Paul
> > > > >
> > > > > > > +
> > > > > > > + This property is based on the minimum number of memory buffers
> > > > > > > + needed to fill the capture pipeline composed of hardware processing
> > > > > > > + blocks plus as many buffers as needed to take into account propagation
> > > > > > > + delays and avoid dropping frames.
> > > > > > > +
> > > > > > > + \todo Should this be a per-stream property?
> > > > > > > +
> > > > > > > + \todo Extend this property to expose the different minimum buffer and
> > > > > > > + request counts (the minimum number of buffers to be able to capture at
> > > > > > > + all, the minimum number of buffers to sustain capture without frame
> > > > > > > + drop, and the minimum number of requests to ensure proper operation of
> > > > > > > + per-frame controls).
> > > > > > > +
> > > > > > > # ----------------------------------------------------------------------------
> > > > > > > # Draft properties section
> > > > > > >
> > > > > > > --
> > > > > > > 2.35.1
> > > > > > >
> >
More information about the libcamera-devel
mailing list