[libcamera-devel] [PATCH v5 01/10] libcamera: property: Add MinNumRequests property
Nícolas F. R. A. Prado
nfraprado at collabora.com
Thu Jul 8 15:28:35 CEST 2021
Hi Naushir,
On Thu, Jul 08, 2021 at 02:19:07PM +0100, Naushir Patuck wrote:
> Hi Nícolas,
>
> On Thu, 8 Jul 2021 at 13:44, Nícolas F. R. A. Prado <nfraprado at collabora.com>
> wrote:
>
> > Hi Naushir,
> >
> > On Thu, Jul 08, 2021 at 10:26:54AM +0100, Naushir Patuck wrote:
> > > Hi Nicolas,
> > >
> > > Thank you for your patch.
> > >
> > > On Wed, 7 Jul 2021 at 15:42, Nícolas F. R. A. Prado <
> > nfraprado at collabora.com>
> > > wrote:
> > >
> > > > The MinNumRequests property reports the bare minimum number of requests
> > > > needed in the camera pipeline for capture to be possible. It is equal
> > to
> > > > the number of buffers required by the driver. At this quantity, there's
> > > > no guarantee that frames won't be dropped or that manual per-frame
> > > > controls will apply correctly. The quantity needed for those may be
> > > > added as separate properties in the future.
> > > >
> > > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado at collabora.com>
> > > > ---
> > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 6 ++++++
> > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 6 ++++++
> > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 ++
> > > > src/libcamera/pipeline/simple/simple.cpp | 2 ++
> > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 ++
> > > > src/libcamera/pipeline/vimc/vimc.cpp | 3 +++
> > > > src/libcamera/property_ids.yaml | 9 +++++++++
> > > > 7 files changed, 30 insertions(+)
> > > >
> > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > index 76c3bb3d8aa9..017018c845fa 100644
> > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > @@ -1111,6 +1111,12 @@ int PipelineHandlerIPU3::registerCameras()
> > > > /* Initialize the camera properties. */
> > > > data->properties_ = cio2->sensor()->properties();
> > > >
> > > > + /*
> > > > + * \todo Make sure this is correct even after the
> > stream is
> > > > + * configured to CIO2
> > > > + */
> > > > + data->properties_.set(properties::MinNumRequests, 1);
> > > > +
> > > > ret = initControls(data.get());
> > > > if (ret)
> > > > continue;
> > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > index 082eb1ee1c23..f99a21de6918 100644
> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > @@ -1045,6 +1045,12 @@ bool PipelineHandlerRPi::match(DeviceEnumerator
> > > > *enumerator)
> > > > /* Initialize the camera properties. */
> > > > data->properties_ = data->sensor_->properties();
> > > >
> > > > + /*
> > > > + * \todo Make sure this is correct after the stream is
> > configured
> > > > for
> > > > + * any of the roles
> > > > + */
> > > > + data->properties_.set(properties::MinNumRequests, 1);
> > > > +
> > > >
> > >
> > > Could you elaborate a bit on the above \todo, I'm not entirely sure it is
> > > relevant for the
> > > Raspberry Pi pipeline handler.
> >
> > Sure. I added it because I am worried about the usage of different values
> > for
> > bufferCount depending on the StreamRole used. I saw that you use 2 buffers
> > for
> > Raw, 1 for StillCapture, 4 for VideoRecording and Viewfinder, but I don't
> > quite understand the reason for them.
> >
>
> I don't clearly know what to set bufferCount to. I assumed it is a
> "reasonable guess"
> as to what a user should request for a given use case.
Yes, I think that's how it's used normally. It could also have been used to set
a minimum value however, which makes it a little ambiguous.
>
>
> >
> > Given that I'm removing those bufferCount assignments in patch 10, and
> > that the
> > MinNumRequests is one global property for the pipeline handler, not
> > specific to
> > a single StreamRole, I just added that TODO so we could have this
> > discussion
> > eventually :).
> >
> > So, do you think 1 works fine independent of the StreamRole configuration?
> >
>
> Yes, 1 should work fine, regardless of the use case. So perhaps the todo
> is not
> needed here.
Ok, sounds good. I'll remove it for next version. I also just realized I have
already tested this on the raspberrypi pipeline with all StreamRoles and nothing
exploded, so I guess we're good :).
Thanks,
Nícolas
>
> Regards,
> Naush
>
>
> >
> > Thanks,
> > Nícolas
> >
> > >
> > > Thanks,
> > > Naush
> > >
> > > /*
> > > > * Set a default value for the ScalerCropMaximum property to
> > show
> > > > * that we support its use, however, initialise it to zero
> > because
> > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > index 00df4f0b3e6b..b5cbf2394b1c 100644
> > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > @@ -24,6 +24,7 @@
> > > > #include <libcamera/ipa/core_ipa_interface.h>
> > > > #include <libcamera/ipa/rkisp1_ipa_interface.h>
> > > > #include <libcamera/ipa/rkisp1_ipa_proxy.h>
> > > > +#include <libcamera/property_ids.h>
> > > > #include <libcamera/request.h>
> > > > #include <libcamera/stream.h>
> > > >
> > > > @@ -944,6 +945,7 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity
> > > > *sensor)
> > > >
> > > > /* Initialize the camera properties. */
> > > > data->properties_ = data->sensor_->properties();
> > > > + data->properties_.set(properties::MinNumRequests, 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 b29fff9820e5..c4adea61519f 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>
> > > >
> > > > @@ -436,6 +437,7 @@ int SimpleCameraData::init()
> > > > }
> > > >
> > > > properties_ = sensor_->properties();
> > > > + properties_.set(properties::MinNumRequests, 3);
> > > >
> > > > return 0;
> > > > }
> > > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > > b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > > index 0f634b8da609..0258111ad6cf 100644
> > > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > > @@ -525,6 +525,8 @@ int UVCCameraData::init(MediaDevice *media)
> > > > properties_.set(properties::PixelArraySize, resolution);
> > > > properties_.set(properties::PixelArrayActiveAreas, {
> > > > Rectangle(resolution) });
> > > >
> > > > + properties_.set(properties::MinNumRequests, 1);
> > > > +
> > > > /* Initialise the supported controls. */
> > > > ControlInfoMap::Map ctrls;
> > > >
> > > > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp
> > > > b/src/libcamera/pipeline/vimc/vimc.cpp
> > > > index 12f7517fd0ae..8c3f7ccb46bd 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>
> > > >
> > > > @@ -517,6 +518,8 @@ int VimcCameraData::init()
> > > > /* Initialize the camera properties. */
> > > > properties_ = sensor_->properties();
> > > >
> > > > + properties_.set(properties::MinNumRequests, 2);
> > > > +
> > > > return 0;
> > > > }
> > > >
> > > > diff --git a/src/libcamera/property_ids.yaml
> > > > b/src/libcamera/property_ids.yaml
> > > > index 12ecbce5eed4..4efa952ee393 100644
> > > > --- a/src/libcamera/property_ids.yaml
> > > > +++ b/src/libcamera/property_ids.yaml
> > > > @@ -678,6 +678,15 @@ controls:
> > > > \todo Turn this property into a "maximum control value" for
> > the
> > > > ScalerCrop control once "dynamic" controls have been
> > implemented.
> > > >
> > > > + - MinNumRequests:
> > > > + type: int32_t
> > > > + description: |
> > > > + The bare minimum number of requests needed in the camera
> > pipeline
> > > > for
> > > > + capture to be possible, as required by the driver. Note that
> > this
> > > > + quantity does not guarantee that frames won't be dropped or
> > that
> > > > manual
> > > > + per-frame controls will be applied properly.
> > > > +
> > > > +
> > > > #
> > > >
> > ----------------------------------------------------------------------------
> > > > # Draft properties section
> > > >
> > > > --
> > > > 2.32.0
> > > >
> > > >
> >
More information about the libcamera-devel
mailing list