[libcamera-devel,v6,1/3] libcamera: controls: Add frame duration control

10635 diff mbox series

Add a float array control (controls::FrameDurations) to specify the
minimum and maximum (in that order) frame duration to be used by the
camera sensor.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
Tested-by: David Plowman <david.plowman@raspberrypi.com>
---
 src/libcamera/control_ids.yaml | 40 ++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

Hi Naush,
   thanks for the update

On Thu, Dec 10, 2020 at 04:33:35PM +0000, Naushir Patuck wrote:
> Add a float array control (controls::FrameDurations) to specify the
> minimum and maximum (in that order) frame duration to be used by the
> camera sensor.
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> Tested-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  src/libcamera/control_ids.yaml | 40 ++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>
> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> index 6d6f0fee..5ee31865 100644
> --- a/src/libcamera/control_ids.yaml
> +++ b/src/libcamera/control_ids.yaml
> @@ -306,6 +306,46 @@ controls:
>          maximum valid value is given by the properties::ScalerCropMaximum
>          property, and the two can be used to implement digital zoom.
>
> +  - FrameDurations:
> +      type: int64_t
> +      description: |
> +          The minimum and maximum (in that order) frame duration,
> +          expressed in micro-seconds.
> +
> +          When provided by applications, the control specifies the sensor frame
> +          duration interval the pipeline has to use. This could also limit the
> +          largest exposure times the sensor can use. For example, if a maximum

Is 'times' intentional ? Or did you mean 'time' ? Honest question..

> +          frame duration of 33ms is requested (corresponding to 30 frames per
> +          second), the sensor will not be able to raise the exposure time above
> +          33ms. A fixed frame duration is achieved by setting the minimum and
> +          maximum values to be the same. Setting both values to 0 resets the
> +          frame duration to the IPA defaults.

I would drop the last statement as we should better think where
defaults will be specified (a property or implicitly in the IPA). I'll
rather have a \todo.

> +
> +          The maximum frame duration provides the absolute limit to the shutter
> +          speed available to the AE algorithm. This limit also overrides any
> +          limits set by the exposure mode (AeExposureMode). Similarly, a manual
> +          ExposureTime value provided by the application may also be clipped by
> +          this limit.

I would re-phrase this as:

          The maximum frame duration provides the absolute limit to the shutter
          speed computed by the AE algorithm and it overrides any exposure mode
          setting specified with controls::AeExposureMode. Similarly, when a
          manual exposure time is set through controls::ExposureTime, it also
          gets clipped in the limits set by this control.

to make it more 'imperative'. It might sound all very "must/shall", but
I fear documenting things as "may/might" as it means one application
might behave differently when run on different platforms. I would
prefer to start strict, and in case the need arise, re-think how to
make these precedence relationships controllable ?

> +
> +          \sa AeExposureMode
> +          \sa ExposureTime
> +
> +          \todo Refer to the frame duration limits property to describe how
> +          application-provided values gets clipped.

If you agree the statement above about reset should be dropped, I
would "gets clipped and reset".
> +
> +          When reported by pipelines, the control expresses the duration of the
> +          sensor frame used to produce streams part of the completed Request.
> +          The minimum and maximum values shall then be the same, as the sensor
> +          frame duration is a fixed parameter. The sensor frame duration is one
> +          of the parameter that defines the capture frame rate but it does not
> +          alone provide enough information to fully calculate it as it does not
> +          account for pipeline processing delays.
> +
> +          \todo Define how to calculate the capture frame rate by
> +          defining controls to report additional delays introduced by
> +          the capture pipeline or post-processing stages (ie JPEG
> +          conversion, frame scaling).
> +

Thanks, minor apart this looks good
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

>    # ----------------------------------------------------------------------------
>    # Draft controls section
>
> --
> 2.25.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Jacopo,

On Thu, 10 Dec 2020 at 17:44, Jacopo Mondi <jacopo@jmondi.org> wrote:

> Hi Naush,
>    thanks for the update
>
> On Thu, Dec 10, 2020 at 04:33:35PM +0000, Naushir Patuck wrote:
> > Add a float array control (controls::FrameDurations) to specify the
> > minimum and maximum (in that order) frame duration to be used by the
> > camera sensor.
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > Tested-by: David Plowman <david.plowman@raspberrypi.com>
> > ---
> >  src/libcamera/control_ids.yaml | 40 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 40 insertions(+)
> >
> > diff --git a/src/libcamera/control_ids.yaml
> b/src/libcamera/control_ids.yaml
> > index 6d6f0fee..5ee31865 100644
> > --- a/src/libcamera/control_ids.yaml
> > +++ b/src/libcamera/control_ids.yaml
> > @@ -306,6 +306,46 @@ controls:
> >          maximum valid value is given by the
> properties::ScalerCropMaximum
> >          property, and the two can be used to implement digital zoom.
> >
> > +  - FrameDurations:
> > +      type: int64_t
> > +      description: |
> > +          The minimum and maximum (in that order) frame duration,
> > +          expressed in micro-seconds.
> > +
> > +          When provided by applications, the control specifies the
> sensor frame
> > +          duration interval the pipeline has to use. This could also
> limit the
> > +          largest exposure times the sensor can use. For example, if a
> maximum
>
> Is 'times' intentional ? Or did you mean 'time' ? Honest question..
>

You are correct, it should be 'time' :)


>
> > +          frame duration of 33ms is requested (corresponding to 30
> frames per
> > +          second), the sensor will not be able to raise the exposure
> time above
> > +          33ms. A fixed frame duration is achieved by setting the
> minimum and
> > +          maximum values to be the same. Setting both values to 0
> resets the
> > +          frame duration to the IPA defaults.
>
> I would drop the last statement as we should better think where
> defaults will be specified (a property or implicitly in the IPA). I'll
> rather have a \todo.
>

Good point.  I will add a todo for this.  FYI, for Raspberry Pi, if you set
0, it does return back to the IPA specified default.  If we decide this is
not appropriate, it is an easy modification.


>
> > +
> > +          The maximum frame duration provides the absolute limit to the
> shutter
> > +          speed available to the AE algorithm. This limit also
> overrides any
> > +          limits set by the exposure mode (AeExposureMode). Similarly,
> a manual
> > +          ExposureTime value provided by the application may also be
> clipped by
> > +          this limit.
>
> I would re-phrase this as:
>
>           The maximum frame duration provides the absolute limit to the
> shutter
>           speed computed by the AE algorithm and it overrides any exposure
> mode
>           setting specified with controls::AeExposureMode. Similarly, when
> a
>           manual exposure time is set through controls::ExposureTime, it
> also
>           gets clipped in the limits set by this control.
>
> to make it more 'imperative'. It might sound all very "must/shall", but
> I fear documenting things as "may/might" as it means one application
> might behave differently when run on different platforms. I would
> prefer to start strict, and in case the need arise, re-think how to
> make these precedence relationships controllable ?
>

Sure, I will reword that to your suggested text.


>
> > +
> > +          \sa AeExposureMode
> > +          \sa ExposureTime
> > +
> > +          \todo Refer to the frame duration limits property to describe
> how
> > +          application-provided values gets clipped.
>
> If you agree the statement above about reset should be dropped, I
> would "gets clipped and reset".
>

Agreed.  Will post a new version with the updates shortly.

> +
> > +          When reported by pipelines, the control expresses the
> duration of the
> > +          sensor frame used to produce streams part of the completed
> Request.
> > +          The minimum and maximum values shall then be the same, as the
> sensor
> > +          frame duration is a fixed parameter. The sensor frame
> duration is one
> > +          of the parameter that defines the capture frame rate but it
> does not
> > +          alone provide enough information to fully calculate it as it
> does not
> > +          account for pipeline processing delays.
> > +
> > +          \todo Define how to calculate the capture frame rate by
> > +          defining controls to report additional delays introduced by
> > +          the capture pipeline or post-processing stages (ie JPEG
> > +          conversion, frame scaling).
> > +
>
> Thanks, minor apart this looks good
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>
> Thanks
>   j
>
> >    #
> ----------------------------------------------------------------------------
> >    # Draft controls section
> >
> > --
> > 2.25.1
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>