[libcamera-devel] [PATCH v9 02/18] libcamera: property: Add MinimumRequests property
Umang Jain
umang.jain at ideasonboard.com
Tue Dec 20 10:33:26 CET 2022
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
>
>>> 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