<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>