[libcamera-devel] [PATCH v9 02/18] libcamera: property: Add MinimumRequests property
Umang Jain
umang.jain at ideasonboard.com
Wed Dec 21 10:45:43 CET 2022
Hi Paul, Nicolas
Thank you for the patch.
On 12/16/22 5:59 PM, 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>
>
> ---
> 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()``:
> +
> +.. 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);
> +
> 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);
> +
on my local kernel branch unicam reports min_buffers_needed = 1 but
bcm2835-isp doesn't report any min_buffers_needed (dumb guess it'll be 1).
Probably worth checking in with RPi devs regarding the
min_buffers_needed for their ISP (adding a note in my todo list)
> /*
> * 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);
>
> /*
> * \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 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).
> +
So, I like the direction here.
But given the questions mentioned in the \todo(s) and provided how we
actually split Requests <> Buffers association going ahead - I think
this will get impacted (probably MinimumRequests will become
MinimumBuffers and what not...)
However, this is still under discussion, so I don't want to start yet
another parallel discussion here except that - in my opinion this
property should be under "Draft properties section" for now. This will
take a considerable set of iterations to get it right (provided how we
define and declare other properties on this front to sustain capture,
framedrops etc. etc.).
Acked-by: Umang Jain <umang.jain at ideasonboard.com>
> # ----------------------------------------------------------------------------
> # Draft properties section
>
More information about the libcamera-devel
mailing list