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

[Naushir Patuck]

Hi Laurent,


On Thu, 14 Jan 2021 at 06:04, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> Hi Naush,
>
> On Tue, Jan 12, 2021 at 07:17:14AM +0000, Naushir Patuck wrote:
> > On Mon, 11 Jan 2021 at 23:07, Laurent Pinchart wrote:
> > > 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.
>
> Sounds good to me.
>
> > > > +          \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?
>
> Not quite I'm afraid, but maybe it's just too early in the morning :-)
>
> Is this about documenting how other properties also get clipped by the
> sensor mode ? Or something else ?
>

It's about how the frame durations are clipped by the sensor mode limits -
as advertised by the sensor properties in the future.
We can remove this statement entirely if you do not think it's appropriate,
or a rewording as follows:

\todo Refer to the frame duration limits property (when available) to
obtain sensor
mode limits used for clipping the application-provided values.

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/20210114/39100c49/attachment.htm>