[libcamera-devel] [PATCH v9 02/18] libcamera: property: Add MinimumRequests property
Umang Jain
umang.jain at ideasonboard.com
Tue Dec 20 09:34:15 CET 2022
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()
> 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..
> 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.
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 ?
>
>>> /*
>>> * \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