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

[Jacopo Mondi]

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 at raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> Tested-by: David Plowman <david.plowman at 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 at jmondi.org>

Thanks
  j

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