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

[Jacopo Mondi]

Hi Naush,
   thanks for the update

On Thu, Dec 10, 2020 at 01:34:24PM +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 | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> index 6d6f0fee..7f1f8624 100644
> --- a/src/libcamera/control_ids.yaml
> +++ b/src/libcamera/control_ids.yaml
> @@ -554,4 +554,19 @@ controls:
>          detection, additional format conversions etc) count as an additional
>          pipeline stage.

So, I've gone through the lentghy discussions on v2 and v3.

To be very honest, I think we still have some missing pieces and the
one that concerns me the more is the interaction of this control with
the selected AE mode and the consequences on exposure/shutter-time
priorites. I see all controls about Exposure have their interaction
definition defferred to a \todo item. This is of course not the ideal
situation but adding one make the issue only slightly worse. Deferring
these to the pipeline model definition might be an option.

I'm willing to ack this patch, but I think there are a few details I
would like to discuss:

- Clipping. We need a (per-configuration, like the ScalerCrop
  rectangle) property to provide application limits and refer to it in
  this control description. I'll address this on top, but I would
  apreciate a:

  \todo Refer to the frame duration limits property to describe how
  application-provided values gets clipped or how to reset the control
  value to its default.

- This is both a control and a metadata. I think the
  description is only about 'setting' the FrameDurations, not reading
  it. We have discussed in length the former but somewhat ignored the
  latter.

  The 'reading' case could addressed by introducing a read-only control
  (ie FrameDuration) only to be used as metadata. But this might even be
  more confusing as people will wonder why they have to use
  'Duration-s-' when they want a precise value and theres a 'Duration'
  (without 's') available. I'll propose an additional section but if
  you have ideas please suggest them. Also, feel free to leave this
  last part out if it turns out to be controversial and would delay the
  series any longer.

>
> +  - FrameDurations:
> +      type: float

I understand this is meant to accommodate standard FPS like 29,97 FPS
(-.-) but won't expressing this as nanoseconds with a uin64_t
representation be capable of achieving the same. Floating point
arithmentic is generally a bit harder and clunky to handle when doing
calculations. I won't push if you think a float is better.

> +      description: |

I would:
        size: [2]
        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 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.

          \todo Refer to the frame duration limits property to describe how
          application-provided values gets clipped or how to reset the control
          values to their defaults.

          \todo Better specify how the frame durations interact with the
          exposure control algorithm.
          \sa AeEnable
          \sa AeExposureMode
          \sa ExposureTime

          -----------------8< you can cut here 8<--------------------

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

> +        Specifies the minimum and maximum (in that order) allowable frame
> +        duration, in micro-seconds, for the sensor to use. This could also limit
> +        the largest exposure times the sensor can use. For example, if a maximum
> +        frame duration of 33ms is requested (corresponding to 30 frames per
> +        second), the sensor will not be able raise the exposure time above 33ms.

s/will not be able raise/will not be able to raise/
Fixed in the above suggestions.

Thanks
  j

> +        A fixed frame duration is achieved by setting the minimum and maximum
> +        values to be the same. Note that the sensor may not always be able to
> +        provide the requested frame duration limits depending on its mode
> +        configuration.

> +        \sa ExposureTime
> +      size: [2]
>  ...
> --
> 2.25.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel