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

[Laurent Pinchart]

On Tue, May 26, 2020 at 09:59:57AM +0100, Naushir Patuck wrote:
> On Sun, 24 May 2020 at 23:23, Laurent Pinchart wrote:
> > 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).

But we're now using a frame duration in µs. A value lower than one would
correspond to a frame rate higher than 1Mfps, which is unlikely :-)

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

How should applications specify that ? By setting the other value to 0
(for minimum) or UINT32_MAX (for maximum) ? Or do you think another
means would be better ?

> 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?

I think reporting the minimum and maximum achievable frame durations is
useful. It could be done by specifying FrameDurations (or
FrameDurationLimits, as proposed in the review of 2/2) as a property
(Camera::properties()). The maximum won't be a problem, but the minimum
(highest frame rate) may depend on the frame size, so this can be
tricky.

> 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?

CameraSensorInfo is meant to convey information specific to the mode
that has been selected (or, more generally speaking, the sensor
configuration that has been set). It won't be a good match to report
data that need to be set in camera static properties, at least when
passed to IPAInterface::configure().

The maximum achievable frame rate depends on both the sensor and the
ISP, as the ISP bandwidth limit may be lower than what the sensor
supports. Could it also depend on the algorithms (and would thus need to
involve the IPA), or would it be possible to always compute the limits
in the pipeline handler ?

We need to figure out what information we need to calculate the limits,
and identify what information needs to be retrieved from the kernel (if
any), what static information about the sensor should come from a
userspace sensor database (similar to CamHelper, but moved to the
libcamera core or libipa), and how the pipeline handler and IPA should
collaborate to perform the computation. I can help designing all this,
but if you already have an idea regarding how it could be done (even if
in a way that would be specific to Raspberry Pi) that would be useful as
a base for discussions.

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

That sounds good to me. Could you update the description to document
this ?

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

OK, good to have a confirmation that I shouldn't worry about it :-)

> > > +
> > > +        \sa ExposureTime
> > > +      size: [2]
> > >  ...

-- 
Regards,

Laurent Pinchart