| Message ID | 20221117154500.8829-2-david.plowman@raspberrypi.com |
|---|---|
| State | Accepted |
| Commit | 1bd74b91be2da535918d509ff32c1cb2b76dcec3 |
| Headers | show |
| Series |
|
| Related | show |
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@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? 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? > > - AfState: > type: int32_t > -- > 2.30.2 >
Hi Kieran On Thu, 17 Nov 2022 at 16:04, Kieran Bingham <kieran.bingham@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@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. > > 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 > >
Quoting David Plowman (2022-11-17 16:11:38) > Hi Kieran > > On Thu, 17 Nov 2022 at 16:04, Kieran Bingham > <kieran.bingham@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@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 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@ideasonboard.com> 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 > > >
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@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@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@ideasonboard.com> Likewise Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > 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 > > > >
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@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@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@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@ideasonboard.com> > > Likewise > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > 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 > > > > >
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@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@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@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@ideasonboard.com> > > > > Likewise > > Reviewed-by: Jacopo Mondi <jacopo@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 > > > > > >
Quoting Kieran Bingham (2022-12-08 11:31:02) > 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@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@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@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@ideasonboard.com> > > > > > > Likewise > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > I see two RB tags, so I'll run this through the tests and merge if > nothing here breaks! > Ahem, with no actual code change - nothing can break ;-) So I guess this gets an easy pass. -- Kieran > -- > 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 > > > > > > >
Great, thank you Kieran! David On Thu, 8 Dec 2022 at 11:35, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Quoting Kieran Bingham (2022-12-08 11:31:02) > > 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@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@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@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@ideasonboard.com> > > > > > > > > Likewise > > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > > > > I see two RB tags, so I'll run this through the tests and merge if > > nothing here breaks! > > > > Ahem, with no actual code change - nothing can break ;-) So I guess this > gets an easy pass. > -- > Kieran > > > > -- > > 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 > > > > > > > >
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. - AfState: type: int32_t
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@raspberrypi.com> --- src/libcamera/control_ids.yaml | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-)