[libcamera-devel] [PATCH v5 1/3] libcamera: controls: Add frame duration control
Naushir Patuck
naush at raspberrypi.com
Thu Dec 10 16:17:49 CET 2020
Hi Jacopo,
Thank you for your review comments.
On Thu, 10 Dec 2020 at 14:48, Jacopo Mondi <jacopo at jmondi.org> wrote:
> Hi Naush,
> thanks for the update
>
> On Thu, Dec 10, 2020 at 01:34:24PM +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 | 15 +++++++++++++++
> > 1 file changed, 15 insertions(+)
> >
> > diff --git a/src/libcamera/control_ids.yaml
> b/src/libcamera/control_ids.yaml
> > index 6d6f0fee..7f1f8624 100644
> > --- a/src/libcamera/control_ids.yaml
> > +++ b/src/libcamera/control_ids.yaml
> > @@ -554,4 +554,19 @@ controls:
> > detection, additional format conversions etc) count as an
> additional
> > pipeline stage.
>
> So, I've gone through the lentghy discussions on v2 and v3.
>
> To be very honest, I think we still have some missing pieces and the
> one that concerns me the more is the interaction of this control with
> the selected AE mode and the consequences on exposure/shutter-time
> priorites. I see all controls about Exposure have their interaction
> definition defferred to a \todo item. This is of course not the ideal
> situation but adding one make the issue only slightly worse. Deferring
> these to the pipeline model definition might be an option.
>
You are entirely correct here on all ponts. There is a tightly coupled
interaction with this FrameDurations control and interaction with the AGC
algorithm. You have also thrown a spanner in the works for my plan :)
David and I have been working on exactly this and we think we have
addressed all these interactions. I have a set of patches that are ready
to be pushed as a "phase 2", but wanted to get this through first so as not
to muddy the waters too much. This set of "phase 2" patches address the
following:
1) Set VBLANK ahead of any other controls. This avoids the problem of
setting EXPOSURE and having v4l2 reject the value due to stale limits.
2) Limit VBLANK (and hence frame duration) to what the sensor mode can
actually support. This forms part of your Clipping discussion below.
3) Add an interaction with AE when FrameDurations. This addresses your
concerns above, with AE knowing exactly what limits of shutter speed are
achievable and working around any limitations based on exposure modes
selected.
Perhaps I should make those changes as part of this series so you get the
full picture right now?
>
> I'm willing to ack this patch, but I think there are a few details I
> would like to discuss:
>
> - Clipping. We need a (per-configuration, like the ScalerCrop
> rectangle) property to provide application limits and refer to it in
> this control description. I'll address this on top, but I would
> apreciate a:
>
> \todo Refer to the frame duration limits property to describe how
> application-provided values gets clipped or how to reset the control
> value to its default.
>
This is partially addressed in my point (2) above. However, I did not add
a property, rather just did clipping based on VBLANK limits to the IPA. So
do you think we should have a "properties::SensorMaxFramerate" or similar
that provides the application the maximum possible framerate for this mode
(derived from VBLANK control limits)? As mentioned above, right now I
simply clip the user request to what the sensor mode can achieve.
>
> - This is both a control and a metadata. I think the
> description is only about 'setting' the FrameDurations, not reading
> it. We have discussed in length the former but somewhat ignored the
> latter.
>
> The 'reading' case could addressed by introducing a read-only control
> (ie FrameDuration) only to be used as metadata. But this might even be
> more confusing as people will wonder why they have to use
> 'Duration-s-' when they want a precise value and theres a 'Duration'
> (without 's') available. I'll propose an additional section but if
> you have ideas please suggest them. Also, feel free to leave this
> last part out if it turns out to be controversial and would delay the
> series any longer.
>
Yes, good point here. Returning a "FrameDuration" may be a bit redundant,
as this information is conveyed in the FrameBuffer timestamps. What do
other folks think?
>
> >
> > + - FrameDurations:
> > + type: float
>
> I understand this is meant to accommodate standard FPS like 29,97 FPS
> (-.-) but won't expressing this as nanoseconds with a uin64_t
> representation be capable of achieving the same. Floating point
> arithmentic is generally a bit harder and clunky to handle when doing
> calculations. I won't push if you think a float is better.
>
I only used float here as that is what we use in our IPA :) Happy to change
to int64_t based numbers.
>
> > + description: |
>
> I would:
> size: [2]
> 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 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.
>
> \todo Refer to the frame duration limits property to describe how
> application-provided values gets clipped or how to reset the
> control
> values to their defaults.
>
> \todo Better specify how the frame durations interact with the
> exposure control algorithm.
> \sa AeEnable
> \sa AeExposureMode
> \sa ExposureTime
>
> -----------------8< you can cut here 8<--------------------
>
> 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).
>
Sure, no problem. I will use the above wording and perhaps expand a bit
more on the AE interactions.
>
> > + 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.
>
> s/will not be able raise/will not be able to raise/
> Fixed in the above suggestions.
>
Ack.
Regards,
Naush
>
> Thanks
> j
> > + A fixed frame duration is achieved by setting the minimum and
> maximum
> > + values to be the same. Note that the sensor may not always be
> able to
> > + provide the requested frame duration limits depending on its
> mode
> > + configuration.
>
> > + \sa ExposureTime
> > + size: [2]
> > ...
> > --
> > 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/20201210/39424b4f/attachment.htm>
More information about the libcamera-devel
mailing list