[libcamera-devel] [PATCH v9 02/18] libcamera: property: Add MinimumRequests property
Umang Jain
umang.jain at ideasonboard.com
Wed Dec 28 06:04:46 CET 2022
Hi Paul,
On 12/28/22 9:39 AM, Paul Elder wrote:
> On Wed, Dec 21, 2022 at 03:15:43PM +0530, Umang Jain wrote:
>> 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...)
> Yeah it definitely will. I think we can deal with that when we come to
> it, though.
>
>> 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
> Well we want this to be a required property, so I don't think it can go
> in the draft properties.
Ah okay, I didn't know that such a constraint exists for required
properties.
>
>> 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>
>
> Thanks,
>
> Paul
>
>>> # ----------------------------------------------------------------------------
>>> # Draft properties section
More information about the libcamera-devel
mailing list