[libcamera-devel] [PATCH 1/1] libcamera: controls: Change LensPosition units to dioptres

Thu Dec 8 12:31:02 CET 2022

Hi David,

Quoting David Plowman (2022-12-08 11:22:19)
> Hi everyone
> 
> Thanks for the comments on this change. I was wondering if I might
> give it a little nudge if there are no objections anywhere else?
> 
> Thanks!
> David
> 
> On Fri, 18 Nov 2022 at 08:21, Jacopo Mondi <jacopo at jmondi.org> wrote:
> >
> > Hello
> >
> > On Thu, Nov 17, 2022 at 11:06:39PM +0000, Kieran Bingham via libcamera-devel wrote:
> > > Quoting David Plowman (2022-11-17 16:11:38)
> > > > Hi Kieran
> > > >
> > > > On Thu, 17 Nov 2022 at 16:04, Kieran Bingham
> > > > <kieran.bingham at ideasonboard.com> wrote:
> > > > >
> > > > > Hi David,
> > > > >
> > > > > Quoting David Plowman via libcamera-devel (2022-11-17 15:45:00)
> > > > > > The units for the LensPosition control, previously defined as being in
> > > > > > units of 1 / hyperfocal_distance, are changed to 1 / distance (in
> > > > > > metres).
> > > > > >
> > > > > > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > > > > > ---
> > > > > >  src/libcamera/control_ids.yaml | 26 ++++++++++++++------------
> > > > > >  1 file changed, 14 insertions(+), 12 deletions(-)
> > > > > >
> > > > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > > > > > index a456e6c0..adea5f90 100644
> > > > > > --- a/src/libcamera/control_ids.yaml
> > > > > > +++ b/src/libcamera/control_ids.yaml
> > > > > > @@ -591,25 +591,27 @@ controls:
> > > > > >          AfModeManual, though the value is reported back unconditionally in all
> > > > > >          modes.
> > > > > >
> > > > > > -        The units are a reciprocal distance scale like dioptres but normalised
> > > > > > -        for the hyperfocal distance. That is, for a lens with hyperfocal
> > > > > > -        distance H, and setting it to a focal distance D, the lens position LP,
> > > > > > -        which is generally a non-integer, is given by
> > > > > > +        This value, which is generally a non-integer, is the reciprocal of the
> > > > > > +        focal distance in metres, also known as dioptres. That is, to set a
> > > > > > +        focal distance D, the lens position LP is given by
> > > > > >
> > > > > > -        \f$LP = \frac{H}{D}\f$
> > > > > > +        \f$LP = \frac{1\mathrm{m}}{D}\f$
> > > > > >
> > > > > >          For example:
> > > > > >
> > > > > >          0 moves the lens to infinity.
> > > > > > -        0.5 moves the lens to twice the hyperfocal distance.
> > > > > > -        1 moves the lens to the hyperfocal position.
> > > > > > -        And larger values will focus the lens ever closer.
> > > > > > +        0.5 moves the lens to focus on objects 2m away.
> > > > > > +        2 moves the lens to focus on objects 50cm away.
> > > > > > +        And larger values will focus the lens closer.
> > > > > >
> > > > > > -        \todo Define a property to report the Hyperforcal distance of calibrated
> > > > > > -        lenses.
> > > > > > +        The default value of the control should indicate a good general position
> > > > > > +        for the lens, often corresponding to the hyperfocal distance (the
> > > > > > +        closest position for which objects at infinity are still acceptably
> > > > > > +        sharp). The minimum will often be zero (meaning infinity), and the
> > > > > > +        maximum value defines the closest focus position.
> > > > > >
> > > > > > -        \todo Define a property to report the maximum and minimum positions of
> > > > > > -        this lens. The minimum value will often be zero (meaning infinity).
> > > > > > +        \todo Define a property to report the Hyperfocal distance of calibrated
> > > > > > +        lenses.
> > > > >
> > > > > Is this going to be a separate property, or the 'default' value of the
> > > > > control?
> > > >
> > > > We're thinking we're going to use the default value of the control.
> > > >
> > > > Actually I think there is still a point to a "hyperfocal" property,
> > > > because it's not guaranteed that the "default" position is the
> > > > hyperfocal one. But in our applications, I think we're taking the view
> > > > that we will always use this "default" position. Nearly always it will
> > > > be hyperfocal, and if not, it will be because we want to put the lens
> > > > somewhere else.
> > >
> > > Using the 'default' value, we can set it if we know we have one, or
> > > leave the default as unset if there isn't a known one yet.
> > >
> > > We can always add a dedicated hyperfocal property later if it's
> > > applicable easily.
> > >
> >
> > I tend to agree with all the above...
> >
> > It's been half an year since we have been trying to standardize
> > this property on "1 / hyperfocal distance" and so far not a single
> > lens has been calibrated, mostly because the exact calibration of the
> > the hyperfocal distance is a rather complex process, if not
> > impossibile for uncalibrated lenses ?
> >
> > Usage of dioptres means an esier calibration process if I understand
> > things correctly, by simply using a rule meter and a procedure more
> > accessible for non professional users, right ?
> >
> > Anyway, it then remains the fact we need a proper CameraLensHelper to
> > translate from the generic unit to the exact v4l2-control value, same
> > as it happens for the gain values, and if this unblocks that
> > development I'm all for it..
> >
> >
> > > I expect we'll still need some calibration on the VCM/lens anyway to be
> > > able to map a distance to the range of the underlying controls, and I
> > > suspect there will be some slight imprecisions, but at least this would
> > > let us report (approximate) distance information in the metadata too!
> > >
> > > Should we be considering renaming this to FocusPosition / FocalPoint /
> > > FocalLength ? (As it's the focal point we're trying to move rather than
> > > a specific position of the lens ?) - And that might make more sense when
> > > reporting the position in the metadata ?
> > >
> > > Anyway, I don't see anything against this change, and as you added the
> > > property in the first place, I think you're best suited to know if it
> > > was better this way at the moment, so:
> > >
> > >
> > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >
> > Likewise
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> >

I see two RB tags, so I'll run this through the tests and merge if
nothing here breaks!

--
Kieran

> > >
> > > Lets see what anyone else says ...
> > >
> > >
> > > > > In the cover letter you mentioned "we've resolved that we will be
> > > > > able to read a good default position from the control info."
> > > > >
> > > > > Presumably, that means that when lacking other information, '1' is a
> > > > > good default value?
> > > >
> > > > Lacking other information, numbers around 1 are probably not a bad
> > > > guess for many of our small modules.
> > > >
> > > > David
> > > >
> > > > >
> > > > >
> > > > > >
> > > > > >    - AfState:
> > > > > >        type: int32_t
> > > > > > --
> > > > > > 2.30.2
> > > > > >