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

[Naushir Patuck]

Hi Jacopo,

On Thu, 10 Dec 2020 at 17:44, Jacopo Mondi <jacopo at 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 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..
>

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 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20201211/cd598450/attachment-0001.htm>