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

[Naushir Patuck]

Hi Laurent,


On Sun, 24 May 2020 at 23:23, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Naush,
>
> Thank you for the patch.
>
> On Wed, May 13, 2020 at 10:11:18AM +0100, 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>
> > ---
> >  src/libcamera/control_ids.yaml | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >
> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > index 77ebc3f9..42694fc7 100644
> > --- a/src/libcamera/control_ids.yaml
> > +++ b/src/libcamera/control_ids.yaml
> > @@ -239,4 +239,18 @@ controls:
> >          pixel range (as if pixels ranged from 0 to 65535). The SensorBlackLevels
> >          control can only be returned in metadata.
> >        size: [4]
> > +
> > +  - FrameDurations:
> > +      type: float
>
> Do we need sub-microsecond precision, or would a uint32_t control work ?
> I'd rather have a fixed precision if possible.

The reason I chose floating point here was because of legacy standards
(e.g. 29.97/59.94 fps for SMPTE).  It also does allow us to specify <
1 fps if a user ever needs to (e.g. for a time lapse type capture use
case).

> > +      description: |
> > +        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.
> > +        Note that the sensor may not always be able to provide the requested
> > +        frame duration limits depending on its mode configuration.
>
> This looks good to me, but I'd like to discuss the corner cases I can
> already think about.
>
> - Are there use cases for an application to specify a minimum frame
>   duration only, or a maximum frame duration only (that is, any frame
>   rate lower than a limit, or any frame rate higher than a limit) ? If
>   so, how should that be specified ? We could set the minimum value to 0
>   to mean that the maximum frame rate is unbounded, and the maximum
>   value to UINT32_MAX to mean that the minimum frame rate is unbounded.
>   Is there anything I'm overlooking ?

Good point, I can see we might only want to set one limit.  In these
cases, the sensor specified limit would be used.  Speaking of this, I
was going to address sensor reported framerate after this patch.  Do
we do need a way for the sensor driver to specify its framerate limits
if the application wants to know?  Perhaps this should be part of
CameraSensorInfo, although it will be mode specific?  Or perhaps we
just use the VBLANK min/max to give us the limits?

>
> - Is 0 an acceptable value ? Or should 1 be the minimum value ?

If we stick to floating point, we should use 0.0 as the min limit.

>
> - What happens if the requested frame duration isn't achievable ? Should
>   we specify that the camera will use a frame duration as close to the
>   requested range as possible ? Or could there be cases where a
>   different behaviour would be needed ?

I think the only sensible thing would be to truncate the requested
values to the sensor limits.  See above on how we get the limits from
the sensor though.

>
> - Not something we need to address now, but do you see any future
>   relation between this control and anti-banding (50 or 60Hz flicker
>   avoidance) ? Anti-banding will mostly restrict possible exposure
>   times, and should only indirectly interact with the frame duration if
>   I'm not mistaken, is that correct ?

There should be minimal interaction with framerate control.  As you
said, we will restrict use of certain exposure times for antibanding,
but will have more flexibility with framerate.

>
> > +
> > +        \sa ExposureTime
> > +      size: [2]
> >  ...
>
> --
> Regards,
>
> Laurent Pinchart

Regards,
Naush