| Message ID | 01020191f6184753-fa7f549e-0b7c-4f7a-9bcf-c9389c9f6fd1-000000@eu-west-1.amazonses.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Cláudio, Thank you for the patch. On Sun, Sep 15, 2024 at 02:31:11PM +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 s/v4l2/V4L2/ > respectively when the device supports them. > > If not supported, they are not registered. > > Signed-off-by: Cláudio Paulo <claudio.paulo@makewise.pt> > --- > Fixed more issues raised in review. > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 97 ++++++++++++++++++-- > 1 file changed, 91 insertions(+), 6 deletions(-) > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > index 6b32fa18..401d3052 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 > @@ -358,6 +371,21 @@ 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; > + > + float fvalue = value.get<float>() * scale + min; > + controls->set(cid, static_cast<int32_t>(lroundf(fvalue))); You should use std::lround() here. Please remember to replace <math.h> with <cmath> at the top of the file. > + 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 +683,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>(); As the min, max and def values all have the same type, you can write if (v4l2Info.min().type() == ControlTypeInteger32) { min = v4l2Info.min().get<int32_t>(); def = v4l2Info.def().get<int32_t>(); max = v4l2Info.max().get<int32_t>(); } else if (v4l2Info.min().type() == ControlTypeBool) { min = v4l2Info.min().get<bool>(); def = v4l2Info.def().get<bool>(); max = v4l2Info.max().get<bool>(); } > > switch (cid) { > case V4L2_CID_BRIGHTNESS: { > @@ -738,6 +784,45 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info, > }; > break; > } Add a blank line here. > + 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. > + * > + * \todo These values will definitely vary for each different > + * hardware, so this should be tunable somehow. 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 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). Also, the values might not scale > + * linearly, but this implementation assumes they do. > + */ > + float focusedAt50Cm = 0.15f * (max - min); > + float scale = 2.0f / focusedAt50Cm; > + > + info = ControlInfo{ > + { 0.0f }, > + { (max - min) * scale }, > + { (def - min) * scale } > + }; > + break; > + } And here too. > + case V4L2_CID_FOCUS_AUTO: { > + int32_t manual = controls::AfModeManual; > + int32_t continuous = controls::AfModeContinuous; > + > + info = ControlInfo{ > + { manual }, > + { continuous }, > + { def ? continuous : manual }, How about hardcoding the default to continuous, to make it less device-specific ? Then you could write info = ControlInfo{ { controls::AfModeManual }, { controls::AfModeContinuous }, { controls::AfModeContinuous }, }; But for enumerated controls, you should use the ControlInfo constructor that takes a list of supported values: info = ControlInfo{ std::array<ControlValue, 2>{ static_cast<int32_t>(controls::AfModeManual), static_cast<int32_t>(controls::AfModeContinuous), }, { static_cast<int32_t>(controls::AfModeContinuous) }, }; This is a bit complicated. I've posted a patch ("") that let's you simplify the construct: info = ControlInfo{ std::array<ControlValue, 2>{ controls::AfModeManual, controls::AfModeContinuous, }, { controls::AfModeContinuous }, }; This being said, I think you should also check what the minimum and maximum values are for the V4L2 control. It's conceivable that a UVC device would implement the V4L2_CID_FOCUS_AUTO control but only supports manual focus, or auto-focus. I think the following should work. std::vector<ControlValue> values; if (!min || !max) values.push_back(controls::AfModeManual); if (min ||max) values.push_back(controls::AfModeContinuous); info = ControlInfo{ values, values.back() }; break; With these changes (please test them, as I may have made a mistake), I think we can merge the next version. > + }; > + break; > + } > > default: > info = v4l2Info;
On Thu, Sep 26, 2024 at 02:59:29AM +0300, Laurent Pinchart wrote: > Hi Cláudio, > > Thank you for the patch. > > On Sun, Sep 15, 2024 at 02:31:11PM +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 > > s/v4l2/V4L2/ > > > respectively when the device supports them. > > > > If not supported, they are not registered. > > > > Signed-off-by: Cláudio Paulo <claudio.paulo@makewise.pt> > > --- > > Fixed more issues raised in review. > > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 97 ++++++++++++++++++-- > > 1 file changed, 91 insertions(+), 6 deletions(-) > > > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > index 6b32fa18..401d3052 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 > > @@ -358,6 +371,21 @@ 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; > > + > > + float fvalue = value.get<float>() * scale + min; > > + controls->set(cid, static_cast<int32_t>(lroundf(fvalue))); > > You should use std::lround() here. Please remember to replace <math.h> > with <cmath> at the top of the file. > > > + 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 +683,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>(); > > As the min, max and def values all have the same type, you can write > > if (v4l2Info.min().type() == ControlTypeInteger32) { > min = v4l2Info.min().get<int32_t>(); > def = v4l2Info.def().get<int32_t>(); > max = v4l2Info.max().get<int32_t>(); > } else if (v4l2Info.min().type() == ControlTypeBool) { > min = v4l2Info.min().get<bool>(); > def = v4l2Info.def().get<bool>(); > max = v4l2Info.max().get<bool>(); > } > > > > > switch (cid) { > > case V4L2_CID_BRIGHTNESS: { > > @@ -738,6 +784,45 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info, > > }; > > break; > > } > > Add a blank line here. > > > + 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. > > + * > > + * \todo These values will definitely vary for each different > > + * hardware, so this should be tunable somehow. 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 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). Also, the values might not scale > > + * linearly, but this implementation assumes they do. > > + */ > > + float focusedAt50Cm = 0.15f * (max - min); > > + float scale = 2.0f / focusedAt50Cm; > > + > > + info = ControlInfo{ > > + { 0.0f }, > > + { (max - min) * scale }, > > + { (def - min) * scale } > > + }; > > + break; > > + } > > And here too. > > > + case V4L2_CID_FOCUS_AUTO: { > > + int32_t manual = controls::AfModeManual; > > + int32_t continuous = controls::AfModeContinuous; > > + > > + info = ControlInfo{ > > + { manual }, > > + { continuous }, > > + { def ? continuous : manual }, > > How about hardcoding the default to continuous, to make it less > device-specific ? Then you could write > > info = ControlInfo{ > { controls::AfModeManual }, > { controls::AfModeContinuous }, > { controls::AfModeContinuous }, > }; > > But for enumerated controls, you should use the ControlInfo constructor > that takes a list of supported values: > > info = ControlInfo{ > std::array<ControlValue, 2>{ > static_cast<int32_t>(controls::AfModeManual), > static_cast<int32_t>(controls::AfModeContinuous), > }, > { static_cast<int32_t>(controls::AfModeContinuous) }, > }; > > This is a bit complicated. I've posted a patch ("") that let's you The patch is named "[PATCH] libcamera: controls: Handle enum values without a cast" and I've CC'ed you. > simplify the construct: > > info = ControlInfo{ > std::array<ControlValue, 2>{ > controls::AfModeManual, > controls::AfModeContinuous, > }, > { controls::AfModeContinuous }, > }; > > This being said, I think you should also check what the minimum and > maximum values are for the V4L2 control. It's conceivable that a UVC > device would implement the V4L2_CID_FOCUS_AUTO control but only supports > manual focus, or auto-focus. I think the following should work. > > std::vector<ControlValue> values; > > if (!min || !max) > values.push_back(controls::AfModeManual); > if (min ||max) > values.push_back(controls::AfModeContinuous); > > info = ControlInfo{ values, values.back() }; > break; > > With these changes (please test them, as I may have made a mistake), I > think we can merge the next version. > > > + }; > > + break; > > + } > > > > default: > > info = v4l2Info;
diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp index 6b32fa18..401d3052 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 @@ -358,6 +371,21 @@ 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; + + float fvalue = value.get<float>() * scale + min; + controls->set(cid, static_cast<int32_t>(lroundf(fvalue))); + 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 +683,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 +784,45 @@ 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. + * + * \todo These values will definitely vary for each different + * hardware, so this should be tunable somehow. 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 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). Also, the values might not scale + * linearly, but this implementation assumes they do. + */ + float focusedAt50Cm = 0.15f * (max - min); + 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;
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 more issues raised in review. src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 97 ++++++++++++++++++-- 1 file changed, 91 insertions(+), 6 deletions(-)