[v2] libcamera: pipeline: uvcvideo: Map focus controls

21267 diff mbox series

Added mapping of controls::LensPosition and controls::AfMode to
v4l2 controls V4L2_CID_FOCUS_ABSOLUTE and V4L2_CID_FOCUS_AUTO
respectively when the device supports them.

If not supported, they are not registered.

Signed-off-by: Cláudio Paulo <claudio.paulo@makewise.pt>
---
Fixed issues pointed out on feedback.

 src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 101 +++++++++++++++++--
 1 file changed, 93 insertions(+), 8 deletions(-)

Quoting Cláudio Paulo (2024-09-14 22:55:49)
> Added mapping of controls::LensPosition and controls::AfMode to
> v4l2 controls V4L2_CID_FOCUS_ABSOLUTE and V4L2_CID_FOCUS_AUTO
> respectively when the device supports them.
> 
> If not supported, they are not registered.
> 
> Signed-off-by: Cláudio Paulo <claudio.paulo@makewise.pt>
> ---

I've tested this and it's functional.

Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> Fixed issues pointed out on feedback.
> 
>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 101 +++++++++++++++++--
>  1 file changed, 93 insertions(+), 8 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index 6b32fa18..11e34e95 100644
> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> @@ -304,13 +304,26 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
>                 cid = V4L2_CID_EXPOSURE_ABSOLUTE;
>         else if (id == controls::AnalogueGain)
>                 cid = V4L2_CID_GAIN;
> +       else if (id == controls::LensPosition)
> +               cid = V4L2_CID_FOCUS_ABSOLUTE;
> +       else if (id == controls::AfMode)
> +               cid = V4L2_CID_FOCUS_AUTO;
>         else
>                 return -EINVAL;
>  
> +       /* Check if the device supports this control. */
> +       if (controls->idMap()->find(cid) == controls->idMap()->end())
> +               return -EINVAL;
> +
>         const ControlInfo &v4l2Info = controls->infoMap()->at(cid);
> -       int32_t min = v4l2Info.min().get<int32_t>();
> -       int32_t def = v4l2Info.def().get<int32_t>();
> -       int32_t max = v4l2Info.max().get<int32_t>();
> +
> +       int32_t min = -1, def = -1, max = -1;
> +       if (v4l2Info.min().type() == ControlTypeInteger32)
> +               min = v4l2Info.min().get<int32_t>();
> +       if (v4l2Info.min().type() == ControlTypeInteger32)
> +               def = v4l2Info.def().get<int32_t>();
> +       if (v4l2Info.min().type() == ControlTypeInteger32)
> +               max = v4l2Info.max().get<int32_t>();
>  
>         /*
>          * See UVCCameraData::addControl() for explanations of the different
> @@ -333,8 +346,8 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
>  
>         case V4L2_CID_EXPOSURE_AUTO: {
>                 int32_t ivalue = value.get<bool>()
> -                              ? V4L2_EXPOSURE_APERTURE_PRIORITY
> -                              : V4L2_EXPOSURE_MANUAL;
> +                                        ? V4L2_EXPOSURE_APERTURE_PRIORITY
> +                                        : V4L2_EXPOSURE_MANUAL;

I'm sorry for this one - but this is a false positive from clang-format
in checkstyle. I never could coax it into styling this how we like.

We should remove this hunk from the patch.

>                 controls->set(V4L2_CID_EXPOSURE_AUTO, ivalue);
>                 break;
>         }
> @@ -358,6 +371,20 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
>                 break;
>         }
>  
> +       case V4L2_CID_FOCUS_ABSOLUTE: {
> +               float focusedAt50Cm = 0.15f * (max - min);
> +               float scale = focusedAt50Cm / 2.0f;
> +
> +               controls->set(cid, static_cast<int32_t>(value.get<float>() * scale + min));
> +               break;
> +       }
> +
> +       case V4L2_CID_FOCUS_AUTO: {
> +               int32_t ivalue = value.get<int32_t>() != controls::AfModeManual;
> +               controls->set(cid, ivalue);
> +               break;
> +       }
> +
>         default: {
>                 int32_t ivalue = value.get<int32_t>();
>                 controls->set(cid, ivalue);
> @@ -655,14 +682,32 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
>         case V4L2_CID_GAIN:
>                 id = &controls::AnalogueGain;
>                 break;
> +       case V4L2_CID_FOCUS_ABSOLUTE:
> +               id = &controls::LensPosition;
> +               break;
> +       case V4L2_CID_FOCUS_AUTO:
> +               id = &controls::AfMode;
> +               break;
>         default:
>                 return;
>         }
>  
>         /* Map the control info. */
> -       int32_t min = v4l2Info.min().get<int32_t>();
> -       int32_t max = v4l2Info.max().get<int32_t>();
> -       int32_t def = v4l2Info.def().get<int32_t>();
> +       int32_t min = -1, def = -1, max = -1;
> +       if (v4l2Info.min().type() == ControlTypeInteger32)
> +               min = v4l2Info.min().get<int32_t>();
> +       else if (v4l2Info.min().type() == ControlTypeBool)
> +               min = v4l2Info.min().get<bool>();
> +
> +       if (v4l2Info.def().type() == ControlTypeInteger32)
> +               def = v4l2Info.def().get<int32_t>();
> +       else if (v4l2Info.def().type() == ControlTypeBool)
> +               def = v4l2Info.def().get<bool>();
> +
> +       if (v4l2Info.max().type() == ControlTypeInteger32)
> +               max = v4l2Info.max().get<int32_t>();
> +       else if (v4l2Info.max().type() == ControlTypeBool)
> +               max = v4l2Info.max().get<bool>();
>  
>         switch (cid) {
>         case V4L2_CID_BRIGHTNESS: {
> @@ -738,6 +783,46 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
>                 };
>                 break;
>         }
> +       case V4L2_CID_FOCUS_ABSOLUTE: {
> +               /*
> +                * LensPosition expects values to be in the range of
> +                * [0.0f, maxDioptres], while a value of 0 means focused to
> +                * infinity, 0.5 means focused at 2m, 2 means focused at 50cm
> +                * and maxDioptres will be the closest possible focus which
> +                * will equate to a focus distance of (1 / maxDioptres) metres.
> +                *
> +                * In this case 0.15f (~= 150 / (1023 - 1)) was a value read
> +                * from V4L2_CID_FOCUS_ABSOLUTE control when using a random
> +                * camera and having it autofocus at a distance of about 50cm.

This is a potentially contentious area if it's hardcoding to one
specific device.

> +                * This means the minimum focus distance of this camera should
> +                * be 75mm (0.15 / 2) which equals a maxDioptres value of ~=
> +                * 13.3333 (2 / 0.15).
> +                *
> +                * These values will definitely vary for each different
> +                * hardware. Also, the values might not scale linearly, but
> +                * this implementation assumes they do.
> +                */

I think we'll have to put this in as a \todo rather than a comment to
express that there is further work required here.

> +               float focusedAt50Cm = 0.15f * (max - min);

So it seems like this should be a tunable parameter. But I'm not sure
how we can handle that...

Perhaps if we can determine some (not insane) default value and let it
be specified in the upcoming camera configuration files that Milan is
working on. Or we go and put a full tuning file for UVC ... but that
seems impractical..

But otherwise, this indeed lets the UVC camera start to progress with
the plumbing.


> +               float scale = 2.0f / focusedAt50Cm;
> +
> +               info = ControlInfo{
> +                       { 0.0f },
> +                       { (max - min) * scale },
> +                       { (def - min) * scale }
> +               };
> +               break;
> +       }
> +       case V4L2_CID_FOCUS_AUTO: {
> +               int32_t manual = controls::AfModeManual;
> +               int32_t continuous = controls::AfModeContinuous;
> +
> +               info = ControlInfo{
> +                       { manual },
> +                       { continuous },
> +                       { def ? continuous : manual },
> +               };
> +               break;
> +       }
>  
>         default:
>                 info = v4l2Info;
> -- 
> 2.34.1
>

Hi Kieran,

On Sun, Sep 15, 2024 at 11:39:22AM +0100, Kieran Bingham wrote:
> Quoting Cláudio Paulo (2024-09-14 22:55:49)
> > Added mapping of controls::LensPosition and controls::AfMode to
> > v4l2 controls V4L2_CID_FOCUS_ABSOLUTE and V4L2_CID_FOCUS_AUTO
> > respectively when the device supports them.
> > 
> > If not supported, they are not registered.
> > 
> > Signed-off-by: Cláudio Paulo <claudio.paulo@makewise.pt>
> > ---
> 
> I've tested this and it's functional.
> 
> Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> > Fixed issues pointed out on feedback.
> > 
> >  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 101 +++++++++++++++++--
> >  1 file changed, 93 insertions(+), 8 deletions(-)
> > 
> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > index 6b32fa18..11e34e95 100644
> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > @@ -304,13 +304,26 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
> >                 cid = V4L2_CID_EXPOSURE_ABSOLUTE;
> >         else if (id == controls::AnalogueGain)
> >                 cid = V4L2_CID_GAIN;
> > +       else if (id == controls::LensPosition)
> > +               cid = V4L2_CID_FOCUS_ABSOLUTE;
> > +       else if (id == controls::AfMode)
> > +               cid = V4L2_CID_FOCUS_AUTO;
> >         else
> >                 return -EINVAL;
> >  
> > +       /* Check if the device supports this control. */
> > +       if (controls->idMap()->find(cid) == controls->idMap()->end())
> > +               return -EINVAL;
> > +
> >         const ControlInfo &v4l2Info = controls->infoMap()->at(cid);
> > -       int32_t min = v4l2Info.min().get<int32_t>();
> > -       int32_t def = v4l2Info.def().get<int32_t>();
> > -       int32_t max = v4l2Info.max().get<int32_t>();
> > +
> > +       int32_t min = -1, def = -1, max = -1;
> > +       if (v4l2Info.min().type() == ControlTypeInteger32)
> > +               min = v4l2Info.min().get<int32_t>();
> > +       if (v4l2Info.min().type() == ControlTypeInteger32)
> > +               def = v4l2Info.def().get<int32_t>();
> > +       if (v4l2Info.min().type() == ControlTypeInteger32)
> > +               max = v4l2Info.max().get<int32_t>();
> >  
> >         /*
> >          * See UVCCameraData::addControl() for explanations of the different
> > @@ -333,8 +346,8 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
> >  
> >         case V4L2_CID_EXPOSURE_AUTO: {
> >                 int32_t ivalue = value.get<bool>()
> > -                              ? V4L2_EXPOSURE_APERTURE_PRIORITY
> > -                              : V4L2_EXPOSURE_MANUAL;
> > +                                        ? V4L2_EXPOSURE_APERTURE_PRIORITY
> > +                                        : V4L2_EXPOSURE_MANUAL;
> 
> I'm sorry for this one - but this is a false positive from clang-format
> in checkstyle. I never could coax it into styling this how we like.

I tried to find a way to get clang-format to understand our coding
style, but didn't succeed :-(

> We should remove this hunk from the patch.
> 
> >                 controls->set(V4L2_CID_EXPOSURE_AUTO, ivalue);
> >                 break;
> >         }
> > @@ -358,6 +371,20 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
> >                 break;
> >         }
> >  
> > +       case V4L2_CID_FOCUS_ABSOLUTE: {
> > +               float focusedAt50Cm = 0.15f * (max - min);
> > +               float scale = focusedAt50Cm / 2.0f;
> > +
> > +               controls->set(cid, static_cast<int32_t>(value.get<float>() * scale + min));
> > +               break;
> > +       }
> > +
> > +       case V4L2_CID_FOCUS_AUTO: {
> > +               int32_t ivalue = value.get<int32_t>() != controls::AfModeManual;
> > +               controls->set(cid, ivalue);
> > +               break;
> > +       }
> > +
> >         default: {
> >                 int32_t ivalue = value.get<int32_t>();
> >                 controls->set(cid, ivalue);
> > @@ -655,14 +682,32 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
> >         case V4L2_CID_GAIN:
> >                 id = &controls::AnalogueGain;
> >                 break;
> > +       case V4L2_CID_FOCUS_ABSOLUTE:
> > +               id = &controls::LensPosition;
> > +               break;
> > +       case V4L2_CID_FOCUS_AUTO:
> > +               id = &controls::AfMode;
> > +               break;
> >         default:
> >                 return;
> >         }
> >  
> >         /* Map the control info. */
> > -       int32_t min = v4l2Info.min().get<int32_t>();
> > -       int32_t max = v4l2Info.max().get<int32_t>();
> > -       int32_t def = v4l2Info.def().get<int32_t>();
> > +       int32_t min = -1, def = -1, max = -1;
> > +       if (v4l2Info.min().type() == ControlTypeInteger32)
> > +               min = v4l2Info.min().get<int32_t>();
> > +       else if (v4l2Info.min().type() == ControlTypeBool)
> > +               min = v4l2Info.min().get<bool>();
> > +
> > +       if (v4l2Info.def().type() == ControlTypeInteger32)
> > +               def = v4l2Info.def().get<int32_t>();
> > +       else if (v4l2Info.def().type() == ControlTypeBool)
> > +               def = v4l2Info.def().get<bool>();
> > +
> > +       if (v4l2Info.max().type() == ControlTypeInteger32)
> > +               max = v4l2Info.max().get<int32_t>();
> > +       else if (v4l2Info.max().type() == ControlTypeBool)
> > +               max = v4l2Info.max().get<bool>();
> >  
> >         switch (cid) {
> >         case V4L2_CID_BRIGHTNESS: {
> > @@ -738,6 +783,46 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
> >                 };
> >                 break;
> >         }
> > +       case V4L2_CID_FOCUS_ABSOLUTE: {
> > +               /*
> > +                * LensPosition expects values to be in the range of
> > +                * [0.0f, maxDioptres], while a value of 0 means focused to
> > +                * infinity, 0.5 means focused at 2m, 2 means focused at 50cm
> > +                * and maxDioptres will be the closest possible focus which
> > +                * will equate to a focus distance of (1 / maxDioptres) metres.
> > +                *
> > +                * In this case 0.15f (~= 150 / (1023 - 1)) was a value read
> > +                * from V4L2_CID_FOCUS_ABSOLUTE control when using a random
> > +                * camera and having it autofocus at a distance of about 50cm.
> 
> This is a potentially contentious area if it's hardcoding to one
> specific device.
> 
> > +                * This means the minimum focus distance of this camera should
> > +                * be 75mm (0.15 / 2) which equals a maxDioptres value of ~=
> > +                * 13.3333 (2 / 0.15).
> > +                *
> > +                * These values will definitely vary for each different
> > +                * hardware. Also, the values might not scale linearly, but
> > +                * this implementation assumes they do.
> > +                */
> 
> I think we'll have to put this in as a \todo rather than a comment to
> express that there is further work required here.
> 
> > +               float focusedAt50Cm = 0.15f * (max - min);
> 
> So it seems like this should be a tunable parameter. But I'm not sure
> how we can handle that...
> 
> Perhaps if we can determine some (not insane) default value and let it
> be specified in the upcoming camera configuration files that Milan is
> working on. Or we go and put a full tuning file for UVC ... but that
> seems impractical..

There are thousands of UVC cameras out there, I don't think we can do
that. We may need to expose a new property to indicate that the focus
lens isn't calibrated and that applications can't rely on any particular
mapping between the LensPosition control and focal values.

> But otherwise, this indeed lets the UVC camera start to progress with
> the plumbing.

Indeed :-)

> > +               float scale = 2.0f / focusedAt50Cm;
> > +
> > +               info = ControlInfo{
> > +                       { 0.0f },
> > +                       { (max - min) * scale },
> > +                       { (def - min) * scale }
> > +               };
> > +               break;
> > +       }
> > +       case V4L2_CID_FOCUS_AUTO: {
> > +               int32_t manual = controls::AfModeManual;
> > +               int32_t continuous = controls::AfModeContinuous;
> > +
> > +               info = ControlInfo{
> > +                       { manual },
> > +                       { continuous },
> > +                       { def ? continuous : manual },
> > +               };
> > +               break;
> > +       }
> >  
> >         default:
> >                 info = v4l2Info;