<div dir="ltr"><div dir="ltr">Hi Nicolas,<div><br></div><div>Thank you for your patch.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 24 Aug 2021 at 20:57, Nícolas F. R. A. Prado <<a href="mailto:nfraprado@collabora.com">nfraprado@collabora.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">The MinimumRequests property reports the minimum number of capture<br>
requests that the camera pipeline requires to have queued in order to<br>
sustain capture operations without frame drops. At this quantity,<br>
there's no guarantee that manual per-frame controls will apply<br>
correctly. The quantity needed for that may be added as a separate<br>
property in the future.<br>
<br>
Signed-off-by: Nícolas F. R. A. Prado <<a href="mailto:nfraprado@collabora.com" target="_blank">nfraprado@collabora.com</a>><br>
Reviewed-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><br>
<br>
---<br>
<br>
Changes in v8:<br>
- Changed the MinimumRequests property meaning to require that frames aren't<br>
  dropped<br>
- Set MinimumRequests on SimplePipeline depending on device and usage of<br>
  converter<br>
- Undid definition of default MinimumRequests on CameraSensor constructor<br>
- Updated pipeline-handler guides with new MinimumRequests property<br>
<br>
Changes in v7:<br>
- Renamed property from MinNumRequests to MinimumRequests<br>
- Changed MinimumRequests property's description<br>
<br>
Changes in v6:<br>
- Removed comment from Raspberrypi MinNumRequests setting<br>
- Removed an extra blank line<br>
<br>
 Documentation/guides/pipeline-handler.rst     | 22 ++++++++---<br>
 src/libcamera/pipeline/ipu3/ipu3.cpp          |  2 +<br>
 .../pipeline/raspberrypi/raspberrypi.cpp      |  2 +<br>
 src/libcamera/pipeline/rkisp1/rkisp1.cpp      |  2 +<br>
 src/libcamera/pipeline/simple/simple.cpp      | 38 +++++++++++++++++--<br>
 src/libcamera/pipeline/uvcvideo/uvcvideo.cpp  |  2 +<br>
 src/libcamera/pipeline/vimc/vimc.cpp          |  2 +<br>
 src/libcamera/property_ids.yaml               | 21 ++++++++++<br>
 8 files changed, 83 insertions(+), 8 deletions(-)<br>
<br>
diff --git a/Documentation/guides/pipeline-handler.rst b/Documentation/guides/pipeline-handler.rst<br>
index 54c8e7f1f553..32e3fc518d91 100644<br>
--- a/Documentation/guides/pipeline-handler.rst<br>
+++ b/Documentation/guides/pipeline-handler.rst<br>
@@ -658,19 +658,31 @@ associated with immutable values, which represent static characteristics that ca<br>
 be used by applications to identify camera devices in the system. Properties can be<br>
 registered by inspecting the values of V4L2 controls from the video devices and<br>
 camera sensor (for example to retrieve the position and orientation of a camera)<br>
-or to express other immutable characteristics. The example pipeline handler does<br>
-not register any property, but examples are available in the libcamera code<br>
-base.<br>
+or to express other immutable characteristics.<br>
<br>
-.. TODO: Add a property example to the pipeline handler. At least the model.<br>
+A required property is ``MinimumRequests``, which indicates how many requests<br>
+need to be queued in the pipeline for capture without frame drops to be<br>
+possible.<br>
+<br>
+In our case, the vivid driver requires two buffers before it'll start streaming<br>
+(can be seen in the ``min_buffers_needed`` property for the ``vid_cap`` queue in<br>
+vivid's driver code). In order to not drop frames we should have one extra<br>
+buffer queued to the driver so that it is used as soon as the previous buffer<br>
+completes. Therefore we will set our ``MinimumRequests`` to three. Append the<br>
+following line to ``init()``:<br>
+<br>
+.. code-block:: cpp<br>
+<br>
+   properties_.set(properties::MinimumRequests, 3);<br>
<br>
 At this point you need to add the following includes to the top of the file for<br>
-handling controls:<br>
+handling controls and properties:<br>
<br>
 .. code-block:: cpp<br>
<br>
    #include <libcamera/controls.h><br>
    #include <libcamera/control_ids.h><br>
+   #include <libcamera/property_ids.h><br>
<br>
 Generating a default configuration<br>
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~<br>
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp<br>
index a98d7effc1bb..2309609093cc 100644<br>
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp<br>
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp<br>
@@ -1090,6 +1090,8 @@ int PipelineHandlerIPU3::registerCameras()<br>
                /* Initialize the camera properties. */<br>
                data->properties_ = cio2->sensor()->properties();<br>
<br>
+               data->properties_.set(properties::MinimumRequests, 3);<br>
+<br>
                ret = initControls(data.get());<br>
                if (ret)<br>
                        continue;<br>
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
index b2674ac02109..d35b9714f0c3 100644<br>
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
@@ -1046,6 +1046,8 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)<br>
        /* Initialize the camera properties. */<br>
        data->properties_ = data->sensor_->properties();<br>
<br>
+       data->properties_.set(properties::MinimumRequests, 3);<br>
+<br></blockquote><div><br></div><div>I'm not sure about the number 3 in all use cases... see below...</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
        /*<br>
         * Set a default value for the ScalerCropMaximum property to show<br>
         * that we support its use, however, initialise it to zero because<br>
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp<br>
index 980088628523..cc663c983b3b 100644<br>
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp<br>
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp<br>
@@ -24,6 +24,7 @@<br>
 #include <libcamera/ipa/core_ipa_interface.h><br>
 #include <libcamera/ipa/rkisp1_ipa_interface.h><br>
 #include <libcamera/ipa/rkisp1_ipa_proxy.h><br>
+#include <libcamera/property_ids.h><br>
 #include <libcamera/request.h><br>
 #include <libcamera/stream.h><br>
<br>
@@ -948,6 +949,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)<br>
<br>
        /* Initialize the camera properties. */<br>
        data->properties_ = data->sensor_->properties();<br>
+       data->properties_.set(properties::MinimumRequests, 3);<br>
<br>
        /*<br>
         * \todo Read dealy values from the sensor itself or from a<br>
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp<br>
index 8ff954722fed..26c59e601a76 100644<br>
--- a/src/libcamera/pipeline/simple/simple.cpp<br>
+++ b/src/libcamera/pipeline/simple/simple.cpp<br>
@@ -25,6 +25,7 @@<br>
<br>
 #include <libcamera/camera.h><br>
 #include <libcamera/control_ids.h><br>
+#include <libcamera/property_ids.h><br>
 #include <libcamera/request.h><br>
 #include <libcamera/stream.h><br>
<br>
@@ -130,6 +131,10 @@ class SimplePipelineHandler;<br>
<br>
 struct SimplePipelineInfo {<br>
        const char *driver;<br>
+       /*<br>
+        * Minimum number of buffers required by the driver to start streaming.<br>
+        */<br>
+       unsigned int minimumBuffers;<br>
        /*<br>
         * Each converter in the list contains the name<br>
         * and the number of streams it supports.<br>
@@ -140,9 +145,9 @@ struct SimplePipelineInfo {<br>
 namespace {<br>
<br>
 static const SimplePipelineInfo supportedDevices[] = {<br>
-       { "imx7-csi", { { "pxp", 1 } } },<br>
-       { "qcom-camss", {} },<br>
-       { "sun6i-csi", {} },<br>
+       { "imx7-csi", 2, { { "pxp", 1 } } },<br>
+       { "qcom-camss", 1, {} },<br>
+       { "sun6i-csi", 3, {} },<br>
 };<br>
<br>
 } /* namespace */<br>
@@ -191,6 +196,8 @@ public:<br>
        std::vector<std::unique_ptr<FrameBuffer>> converterBuffers_;<br>
        bool useConverter_;<br>
        std::queue<std::map<unsigned int, FrameBuffer *>> converterQueue_;<br>
+<br>
+       const SimplePipelineInfo *deviceInfo;<br>
 };<br>
<br>
 class SimpleCameraConfiguration : public CameraConfiguration<br>
@@ -774,6 +781,29 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)<br>
        inputCfg.stride = captureFormat.planes[0].bpl;<br>
        inputCfg.bufferCount = kNumInternalBuffers;<br>
<br>
+       /* Set the MinimumRequests property. */<br>
+       unsigned int minimumRequests;<br>
+<br>
+       if (data->useConverter_) {<br>
+               /*<br>
+                * The application will interact only with the capture node of<br>
+                * the converter. Require two buffers for a frame drop free<br>
+                * conversion, plus one extra to account for requeue delays.<br>
+                */<br>
+               minimumRequests = 3;<br>
+       } else {<br>
+               /*<br>
+                * The application will interact directly with the video capture<br>
+                * device. Require the minimum required by the driver, plus one<br>
+                * extra to account for requeue delays. Force at least three<br>
+                * buffers in order to not drop frames.<br>
+                */<br>
+               minimumRequests = std::max(data->deviceInfo->minimumBuffers + 1,<br>
+                                          3U);<br>
+       }<br>
+<br>
+       data->properties_.set(properties::MinimumRequests, minimumRequests);<br>
+<br>
        return converter_->configure(inputCfg, outputCfgs);<br>
 }<br>
<br>
@@ -1040,6 +1070,8 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)<br>
        bool registered = false;<br>
<br>
        for (std::unique_ptr<SimpleCameraData> &data : pipelines) {<br>
+               data->deviceInfo = info;<br>
+<br>
                int ret = data->init();<br>
                if (ret < 0)<br>
                        continue;<br>
diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp<br>
index 973ecd5b835e..75d57300555c 100644<br>
--- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp<br>
+++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp<br>
@@ -526,6 +526,8 @@ int UVCCameraData::init(MediaDevice *media)<br>
        properties_.set(properties::PixelArraySize, resolution);<br>
        properties_.set(properties::PixelArrayActiveAreas, { Rectangle(resolution) });<br>
<br>
+       properties_.set(properties::MinimumRequests, 3);<br>
+<br>
        /* Initialise the supported controls. */<br>
        ControlInfoMap::Map ctrls;<br>
<br>
diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp<br>
index baeb6a7e6fa6..a0d8fd6c48c3 100644<br>
--- a/src/libcamera/pipeline/vimc/vimc.cpp<br>
+++ b/src/libcamera/pipeline/vimc/vimc.cpp<br>
@@ -21,6 +21,7 @@<br>
 #include <libcamera/control_ids.h><br>
 #include <libcamera/controls.h><br>
 #include <libcamera/formats.h><br>
+#include <libcamera/property_ids.h><br>
 #include <libcamera/request.h><br>
 #include <libcamera/stream.h><br>
<br>
@@ -560,6 +561,7 @@ int VimcCameraData::init()<br>
<br>
        /* Initialize the camera properties. */<br>
        properties_ = sensor_->properties();<br>
+       properties_.set(properties::MinimumRequests, 3);<br>
<br>
        return 0;<br>
 }<br>
diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml<br>
index 12ecbce5eed4..9df131265b9f 100644<br>
--- a/src/libcamera/property_ids.yaml<br>
+++ b/src/libcamera/property_ids.yaml<br>
@@ -678,6 +678,27 @@ controls:<br>
         \todo Turn this property into a "maximum control value" for the<br>
         ScalerCrop control once "dynamic" controls have been implemented.<br>
<br>
+  - MinimumRequests:<br>
+      type: int32_t<br>
+      description: |<br>
+        The minimum number of capture requests that the camera pipeline requires<br>
+        to have queued in order to sustain capture operations without frame<br>
+        drops. At this quantity, there's no guarantee that manual per-frame<br>
+        controls will apply correctly.<br>
+<br>
+        This property is based on the minimum number of memory buffers<br>
+        needed to fill the capture pipeline composed of hardware processing<br>
+        blocks plus as many buffers as needed to take into account propagation<br>
+        delays and avoid dropping frames.<br>
+<br>
+        \todo Should this be a per-stream property?<br>
+<br>
+        \todo Extend this property to expose the different minimum buffer and<br>
+        request counts (the minimum number of buffers to be able to capture at<br>
+        all, the minimum number of buffers to sustain capture without frame<br>
+        drop, and the minimum number of requests to ensure proper operation of<br>
+        per-frame controls).<br></blockquote><div><br></div><div>I have slight concerns over the definition wording here.  There is no practical number</div><div>that can guarantee no frame drops.  Should we perhaps reword this to say minimise</div><div>frame drops instead?</div><div><br></div><div>Having the bufferCount as part of the StreamConfiguration object did provide one big</div><div>advantage over the MinimumRequests property - you can specify different numbers for</div><div>different StreamRoles.  So for example viewfinder could use a different number to a </div><div>high resolution stills capture use case.  With a static property this is not the case any more.</div><div>This might be sub-optimal for many scenarios.  Not sure what the solution would be for this.</div><div><br></div><div>Regards,</div><div>Naush</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
   # ----------------------------------------------------------------------------<br>
   # Draft properties section<br>
<br>
-- <br>
2.33.0<br>
<br>
</blockquote></div></div>