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

Naushir Patuck naush at raspberrypi.com
Thu Dec 10 17:20:38 CET 2020 

Hi Jacopo,


On Thu, 10 Dec 2020 at 16:04, Jacopo Mondi <jacopo at jmondi.org> wrote:

> Hi Naush,
>
> On Thu, Dec 10, 2020 at 03:17:49PM +0000, Naushir Patuck wrote:
> > 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:
>
> Ah great :)
>
> >
> > 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.
>
> These are all improvements to the RPi implementation which are
> certainly positive, but if I got these right, it's mostly about
> implementation.
>

That's correct.  The "phase 2" patches are entirely Raspberry Pi
implementation specific.


>
> Using your point 3) as example, my main concern is more where/how
> to specify how pipelines behaves (in the control documentation, in the
> pipeline handler model etc), or if we want all pipelines to behave the
> same, which I'm not even sure it's possible or desirable to enforce.
>

I suppose what the best thing to do for now is put some wording in the
control documentation on how it *should* interact with the AE.  My next
patch will add this, and we can discuss if this is appropriate or not.
Maybe a larger and seperate piece of work would be to expand on this in the
pipeline handler model as well?


>
> Did I get you right and your IPA will:
> 1) Select the appropriate exposure time using the hint provided by
>    AeExposureMode
> 2) Clip the selected interval in the FrameDurations limits
>

Yes, that is precisely what happens.


>
> This seems more than reasonable but then the first question I have is
> how to make this consistent for application among different
> platforms/IPA implementations and how to clearly document that.
>

The control documentation? :-)


>
> This additional controls does not define that, and I think it's fair.
> What we should avoid is going in a direction that makes it too hard to
> backtrack, and I don't think there's anything that bad here.
>
> We just need more IPAs, to have a large enough base to prove these
> concepts against a different implementation I guess.
>
> >
> > Perhaps I should make those changes as part of this series so you get the
> > full picture right now?
>
> The risk is to delay this more. I would do that on top if I got you
> right and these are mostly improvements specific to the RPi
> implementation.
>

Agreed, this is why I did not introduce it just yet.


>
> >
> >
> > >
> > > 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.
>
> As an implementation it's fine, but I think we need a mechanism to allow
> application to access that information. At least, I know we need this
> for Android, so something has to be added.
>
> And I think once we have that property is fair to mention here that
> values outside the limits there reported will be clipped (and that's
> something I think we can assume for all pipelines).
>
> Then if you want to support this, you will simply have to update the
> Camera property to report the VBLANK limits.
>
> >
> >
> > >
> > > - 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?
>
> That's a good point. We have timestamps and durations can be
> calculated. I still think it would be nicer for application to
> have a direct way to access this as part of the Request metadata. Or
> we can expose the sensor timestamp from the Request and let them calculate
> durations..
>

Yes, any of these options would work, and should be easy to put in place.
For now, perhaps I keep "FrameDurations" metadata as what the limits are as
per the user request.


> >
> >
> > >
> > > >
> > > > +  - 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.
> >
>
> I would slightly prefer, but it's a gut feeling. I suspect you have a
> reasons why you used float in your IPA I am missing. If that's the
> case feel free to keep float here.
>

int64_t is fine, and it somewhat mirrors ExposureTime (int32_t) which is
nice.  All of our controller code uses floats, so it is trivial for me to
do a conversion in our IPA.


>
> > >
> > > > +      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.
>
> Great thanks, we can hopefully ack this sooner than later then.
>

Will post an update with the new description text (and the other discussed
changes), and we can look to see if it is more appropriate.

Regards,
Naush



>
> Thanks
>   j
>
> >
> >
> > >
> > > > +        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/fa5ec870/attachment-0001.htm>

More information about the libcamera-devel mailing list