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

[Naushir Patuck]

Hi Laurent,

Thank you for your comments.

On Mon, 11 Jan 2021 at 23:07, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> Hi Naush,
>
> Thank you for the patch.
>
> On Wed, Jan 06, 2021 at 10:06:57AM +0000, Naushir Patuck wrote:
> > Add an int64_t 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>
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/libcamera/control_ids.yaml | 38 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 38 insertions(+)
> >
> > diff --git a/src/libcamera/control_ids.yaml
> b/src/libcamera/control_ids.yaml
> > index 6d6f0fee..d1240beb 100644
> > --- a/src/libcamera/control_ids.yaml
> > +++ b/src/libcamera/control_ids.yaml
> > @@ -306,6 +306,44 @@ 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.
>
> s/micro-seconds/microseconds/
>
> (The indentation should be 2 spaces btw)
>
> > +
> > +          When provided by applications, the control specifies the
> sensor frame
> > +          duration interval the pipeline has to use. This limits the
> largest
> > +          exposure time 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. Setting both values to 0 reverts to
> using the
> > +          IPA provided defaults.
> > +
> > +          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 to the limits set by this control.
>
> This looks good.
>
> >            When reported in
> > +          metadata, the control expresses the minimum and maximum frame
> > +          durations used after being clipped to these limits.
> > +
>
> But this sounds weird. The previous part states that FrameDurations has
> higher priority than all other parameters, but then this sentence says
> it's clipped by "these limits".
>

You are right, this does not read correct.  I wanted to express that the
frame durations provided may be further limited by what the sensor mode can
actually achieve.  How about replacing the above paragraph of text with the
following:

When reported in metadata, the control expresses the minimum and maximum
frame durations used after being clipped to the sensor provided frame
duration limits.


>
> > +          \sa AeExposureMode
> > +          \sa ExposureTime
> > +
> > +          \todo Refer to the frame duration limits property to describe
> how
> > +          application-provided values gets clipped and reset.
>
> It hasn't been long, and the context is already starting to escape me.
> Would it be possible to expand this just a little bit so that we'll know
> what it means in 3 months time ?
>

Perhaps this makes more sense given the rewording above?  Or maybe a reword
as follows:

\todo Refer to the frame duration limits property to describe how
application-provided values get clipped to what is supported by the sensor
mode.

Hopefully that makes things more readable?

Regards,
Naush


> > +
> > +          \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).
> > +
> > +          \todo Provide an explicit definition of default control
> values, for
> > +          this and all other controls.
> > +      size: [2]
> > +
> >    #
> ----------------------------------------------------------------------------
> >    # Draft controls section
> >
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210112/4d501e58/attachment.htm>