[PATCH] libcamera: pipeline: uvcvideo: Map focus controls
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Sep 14 02:17:32 CEST 2024
Hi Cláudio,
Thank you for the patch.
On Fri, Sep 13, 2024 at 05:22:03PM +0000, Cláudio Paulo wrote:
> 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 at makewise.pt>
> ---
> include/linux/v4l2-controls.h | 4 +
> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 88 +++++++++++++++++---
> 2 files changed, 82 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/v4l2-controls.h b/include/linux/v4l2-controls.h
> index 882a8180..1ac85507 100644
> --- a/include/linux/v4l2-controls.h
> +++ b/include/linux/v4l2-controls.h
> @@ -994,6 +994,10 @@ enum v4l2_exposure_auto_type {
> #define V4L2_CID_FOCUS_ABSOLUTE (V4L2_CID_CAMERA_CLASS_BASE+10)
> #define V4L2_CID_FOCUS_RELATIVE (V4L2_CID_CAMERA_CLASS_BASE+11)
> #define V4L2_CID_FOCUS_AUTO (V4L2_CID_CAMERA_CLASS_BASE+12)
> +enum v4l2_focus_auto_type {
> + V4L2_FOCUS_MANUAL = 0,
> + V4L2_FOCUS_AUTO = 1
> +};
Files in the include/linux/ directory are imported from the Linux
kernel. They shouldn't be modified manually, as indicated in
include/linux/README. If you believe this change is important, it should
be submitted to the kernel first. As the V4L2_CID_FOCUS_AUTO control is
a boolean control, I don't think the enum is needed, you can simply
compare the value to == 0 and != 0.
>
> #define V4L2_CID_ZOOM_ABSOLUTE (V4L2_CID_CAMERA_CLASS_BASE+13)
> #define V4L2_CID_ZOOM_RELATIVE (V4L2_CID_CAMERA_CLASS_BASE+14)
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index 6b32fa18..fe3c71d0 100644
> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> @@ -304,13 +304,14 @@ 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 && controls->idMap()->count(V4L2_CID_FOCUS_ABSOLUTE)) // Check if device supports this control
libcamera uses C-style comments. The checkstyle.py script, in utils/,
should have caught that. I recommend installing the
utils/hooks/post-commit hook, as documented in
Documentation/coding-style.rst, to automate running the checkstyle
script.
Note that checkstyle.py sometimes recommends changes that are false
positives. The flagged issues should be reviewed, but the diff doesn't
need to be applied blindly.
> + cid = V4L2_CID_FOCUS_ABSOLUTE;
> + else if (id == controls::AfMode && controls->idMap()->count(V4L2_CID_FOCUS_AUTO)) // Check if device supports this control
Let's move the idMap() checks later.
> + cid = V4L2_CID_FOCUS_AUTO;
> else
> return -EINVAL;
Something along the lines of
if (controls->idMap()->find(cid) == controls->idMap()->end())
return -EINVAL;
This will avoid duplicating the check in multiple places above.
>
> 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>();
It's not nice to duplicate this in most cases below. Is there a way to
do better ?
>
> /*
> * See UVCCameraData::addControl() for explanations of the different
> @@ -318,6 +319,10 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
> */
> switch (cid) {
> case V4L2_CID_BRIGHTNESS: {
> + int32_t min = v4l2Info.min().get<int32_t>();
> + int32_t def = v4l2Info.def().get<int32_t>();
> + int32_t max = v4l2Info.max().get<int32_t>();
> +
> float scale = std::max(max - def, def - min);
> float fvalue = value.get<float>() * scale + def;
> controls->set(cid, static_cast<int32_t>(lroundf(fvalue)));
> @@ -325,6 +330,9 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
> }
>
> case V4L2_CID_SATURATION: {
> + int32_t min = v4l2Info.min().get<int32_t>();
> + int32_t def = v4l2Info.def().get<int32_t>();
> +
> float scale = def - min;
> float fvalue = value.get<float>() * scale + min;
> controls->set(cid, static_cast<int32_t>(lroundf(fvalue)));
> @@ -345,6 +353,10 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
>
> case V4L2_CID_CONTRAST:
> case V4L2_CID_GAIN: {
> + int32_t min = v4l2Info.min().get<int32_t>();
> + int32_t def = v4l2Info.def().get<int32_t>();
> + int32_t max = v4l2Info.max().get<int32_t>();
> +
> float m = (4.0f - 1.0f) / (max - def);
> float p = 1.0f - m * def;
>
> @@ -358,6 +370,22 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
> break;
> }
>
> + case V4L2_CID_FOCUS_ABSOLUTE: {
> + int32_t min = v4l2Info.min().get<int32_t>();
> + int32_t max = v4l2Info.max().get<int32_t>();
> +
> + float focusedAt50Cm = 0.15f * (max - min);
> + float scale = focusedAt50Cm / 2.0f;
> +
> + controls->set(cid, static_cast<int>(min + value.get<float>() * scale));
> + break;
> + }
> +
> + case V4L2_CID_FOCUS_AUTO: {
> + controls->set(cid, static_cast<int>(value.get<int>() == 0 ? V4L2_FOCUS_MANUAL : V4L2_FOCUS_AUTO));
'value' contains a value of the AfMode control, so you should compare it
to AfModeManual, not 0.
Please use an explicit width for the integer, it should be
get<int32_t>().
checkstyle should report that the line is too long. It should be
wrapped.
> + break;
> + }
> +
> default: {
> int32_t ivalue = value.get<int32_t>();
> controls->set(cid, ivalue);
> @@ -655,14 +683,17 @@ 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>();
>
> switch (cid) {
> case V4L2_CID_BRIGHTNESS: {
> @@ -673,6 +704,9 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
> * Accommodate this by restricting the range of the libcamera
> * control, but always within the maximum limits.
> */
> + int32_t min = v4l2Info.min().get<int32_t>();
> + int32_t max = v4l2Info.max().get<int32_t>();
> + int32_t def = v4l2Info.def().get<int32_t>();
> float scale = std::max(max - def, def - min);
>
> info = ControlInfo{
> @@ -683,36 +717,42 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
> break;
> }
>
> - case V4L2_CID_SATURATION:
> + case V4L2_CID_SATURATION: {
> /*
> * The Saturation control is a float, with 0.0 mapped to the
> * minimum value (corresponding to a fully desaturated image)
> * and 1.0 mapped to the default value. Calculate the maximum
> * value accordingly.
> */
> + int32_t min = v4l2Info.min().get<int32_t>();
> + int32_t max = v4l2Info.max().get<int32_t>();
> + int32_t def = v4l2Info.def().get<int32_t>();
> info = ControlInfo{
> { 0.0f },
> { static_cast<float>(max - min) / (def - min) },
> { 1.0f }
> };
> break;
> -
> + }
> case V4L2_CID_EXPOSURE_AUTO:
> info = ControlInfo{ false, true, true };
> break;
>
> - case V4L2_CID_EXPOSURE_ABSOLUTE:
> + case V4L2_CID_EXPOSURE_ABSOLUTE: {
> /*
> * ExposureTime is in units of 1 µs, and UVC expects
> * V4L2_CID_EXPOSURE_ABSOLUTE in units of 100 µs.
> */
> + int32_t min = v4l2Info.min().get<int32_t>();
> + int32_t max = v4l2Info.max().get<int32_t>();
> + int32_t def = v4l2Info.def().get<int32_t>();
> info = ControlInfo{
> { min * 100 },
> { max * 100 },
> { def * 100 }
> };
> break;
> -
> + }
> case V4L2_CID_CONTRAST:
> case V4L2_CID_GAIN: {
> /*
> @@ -723,6 +763,9 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
> * maximum values are respectively no lower than 0.5 and no
> * higher than 4.0.
> */
> + int32_t min = v4l2Info.min().get<int32_t>();
> + int32_t max = v4l2Info.max().get<int32_t>();
> + int32_t def = v4l2Info.def().get<int32_t>();
> float m = (4.0f - 1.0f) / (max - def);
> float p = 1.0f - m * def;
>
> @@ -738,6 +781,31 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
> };
> break;
> }
> + case V4L2_CID_FOCUS_ABSOLUTE: {
> + int32_t min = v4l2Info.min().get<int32_t>();
> + int32_t max = v4l2Info.max().get<int32_t>();
> + int32_t def = v4l2Info.def().get<int32_t>();
> +
> + float focusedAt50Cm = 0.15f * (max - min);
Where does 0.15 come from ?
> + float scale = 2.0f / focusedAt50Cm;
> +
> + info = ControlInfo{
> + { 0.0f },
> + { scale * (max - min) },
Given the calculations just above, this is equivalent to
scale * (max - min)
= 2.0f / focusedAt50Cm * (max - min)
= 2.0f / (0.15f * (max - min)) * (max - min)
= 2.0f / 0.15f
As this is the reciprocal of the focus distance, it corresponds to a
focus distance of 0.15/2 = 75mm. I'm curious to know why you picked this
particular value.
> + { scale * (def - min) }
> + };
> + break;
> + }
> + case V4L2_CID_FOCUS_AUTO: {
> + bool def = v4l2Info.def().get<bool>();
> +
> + info = ControlInfo{
> + { static_cast<int>(V4L2_FOCUS_MANUAL) },
> + { static_cast<int>(V4L2_FOCUS_AUTO) },
> + { static_cast<int>(def ? V4L2_FOCUS_AUTO : V4L2_FOCUS_MANUAL) },
> + };
This isn't correct. ControlInfo describes the AfMode control, so the
values should be AfMode* enumerators, not V4L2 control values. See the
definition of the AfMode control in src/libcamera/control_ids_core.yaml
for the list of enumerators.
> + break;
> + }
>
> default:
> info = v4l2Info;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list