[RFC PATCH v1] libcamera: pipeline: uvcvideo: Fix `ExposureTimeMode` control
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Tue Jan 28 19:17:03 CET 2025
Hi Barnabás
On Tue, Jan 28, 2025 at 12:14:01PM +0000, Barnabás Pőcze wrote:
> `ControlInfo(Span<const int32_t>{...})` calls the incorrect constructor
> of `ControlInfo`. The intended constructor to be called is
> `ControlInfo(Span<const ControlValue>, ...)` however that is not called
> because a span of `const int32_t` is passed. Instead, the constructor
> `ControlInfo(const ControlValue &min, const ControlValue &max, ...)`
> will be called.
So the two constructors your point our are
ControlInfo::ControlInfo(const ControlValue &min,
const ControlValue &max,
const ControlValue &def)
: min_(min), max_(max), def_(def)
{
}
ControlInfo::ControlInfo(const ControlValue &min,
const ControlValue &max,
const ControlValue &def)
: min_(min), max_(max), def_(def)
{
}
right ?
And we call them with
info = ControlInfo{Span<int32_t>{values}, values[0]};
which, if I got you right gets expanded to
ControlInfo::ControlInfo(const ControlValue &min, const ControlValue &max,
const ControlValue &def)
as int32_t are implicitly converted to ControlValue instances.
As 'values' is declared as
std::array<int32_t, 2> values{};
I presume the intermediate call would look like
ControlInfo(int32_t, int32_t, int32_t)
and because of implicit conversion we get to
ControlInfo(const ControlValue &min, const ControlValue &max,
const ControlValue &def)
How come that, if I do (before this patch)
- info = ControlInfo{Span<int32_t>{values}, values[0]};
+ info = ControlInfo{Span<int32_t>{values}, values[0], values[0]};
the code still compiles even if values is of size 2 ?
>
> To fix this, simply pass a span of `ControlValue` instead.
>
> Furthermore, the mapping in `UVCCameraData::processControl()` is also not
> entirely correct because the control value is retrieved as a `bool`
> instead of - the correct type - `int32_t`. Additionally, the available
> modes are not taken into account.
Please split this to a different patch. And in any case, but I have to
re-check, I think the ControlValidator makes sure the control set by
the applications on a camera are supported.
>
> To fix this, stores the available modes for `V4L2_CID_EXPOSURE_AUTO`
> and select the appropriate mode based on the mapping established
> in the comment.
>
> Fixes: bad8d591f8acfa ("libcamera: uvcvideo: Register ExposureTimeMode control")
> Signed-off-by: Barnabás Pőcze <pobrn at protonmail.com>
> ---
> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 81 +++++++++++++-------
> 1 file changed, 53 insertions(+), 28 deletions(-)
>
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index dedcac89b..7821cceb0 100644
> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> @@ -6,6 +6,7 @@
> */
>
> #include <algorithm>
> +#include <bitset>
> #include <cmath>
> #include <fstream>
> #include <map>
> @@ -58,6 +59,13 @@ public:
> Stream stream_;
> std::map<PixelFormat, std::vector<SizeRange>> formats_;
>
> + std::bitset<std::max({
> + V4L2_EXPOSURE_AUTO,
> + V4L2_EXPOSURE_MANUAL,
> + V4L2_EXPOSURE_APERTURE_PRIORITY,
> + V4L2_EXPOSURE_SHUTTER_PRIORITY,
> + }) + 1> availableExposureModes_;
> +
> private:
> bool generateId();
>
> @@ -95,8 +103,8 @@ public:
> bool match(DeviceEnumerator *enumerator) override;
>
> private:
> - int processControl(ControlList *controls, unsigned int id,
> - const ControlValue &value);
> + int processControl(UVCCameraData *data, ControlList *controls,
> + unsigned int id, const ControlValue &value);
> int processControls(UVCCameraData *data, Request *request);
>
> bool acquireDevice(Camera *camera) override;
> @@ -289,8 +297,8 @@ void PipelineHandlerUVC::stopDevice(Camera *camera)
> data->video_->releaseBuffers();
> }
>
> -int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
> - const ControlValue &value)
> +int PipelineHandlerUVC::processControl(UVCCameraData *data, ControlList *controls,
> + unsigned int id, const ControlValue &value)
> {
> uint32_t cid;
>
> @@ -334,10 +342,27 @@ 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;
> - controls->set(V4L2_CID_EXPOSURE_AUTO, ivalue);
> + switch (value.get<int32_t>()) {
> + case controls::ExposureTimeModeAuto:
> + if (data->availableExposureModes_[V4L2_EXPOSURE_AUTO])
> + controls->set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_AUTO);
> + else if (data->availableExposureModes_[V4L2_EXPOSURE_APERTURE_PRIORITY])
> + controls->set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_APERTURE_PRIORITY);
> + else
> + ASSERT(false);
> + break;
> + case controls::ExposureTimeModeManual:
> + if (data->availableExposureModes_[V4L2_EXPOSURE_MANUAL])
> + controls->set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_MANUAL);
> + else if (data->availableExposureModes_[V4L2_EXPOSURE_SHUTTER_PRIORITY])
> + controls->set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_SHUTTER_PRIORITY);
> + else
> + ASSERT(false);
> + break;
> + default:
> + ASSERT(false);
> + break;
> + }
> break;
> }
>
> @@ -375,7 +400,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)
> ControlList controls(data->video_->controls());
>
> for (const auto &[id, value] : request->controls())
> - processControl(&controls, id, value);
> + processControl(data, &controls, id, value);
>
> for (const auto &ctrl : controls)
> LOG(UVC, Debug)
> @@ -725,25 +750,25 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
> * ExposureTimeModeManual = { V4L2_EXPOSURE_MANUAL,
> * V4L2_EXPOSURE_SHUTTER_PRIORITY }
> */
> - std::array<int32_t, 2> values{};
> -
> - auto it = std::find_if(v4l2Values.begin(), v4l2Values.end(),
> - [&](const ControlValue &val) {
> - return (val.get<int32_t>() == V4L2_EXPOSURE_APERTURE_PRIORITY ||
> - val.get<int32_t>() == V4L2_EXPOSURE_AUTO) ? true : false;
> - });
> - if (it != v4l2Values.end())
> - values.back() = static_cast<int32_t>(controls::ExposureTimeModeAuto);
> -
> - it = std::find_if(v4l2Values.begin(), v4l2Values.end(),
> - [&](const ControlValue &val) {
> - return (val.get<int32_t>() == V4L2_EXPOSURE_SHUTTER_PRIORITY ||
> - val.get<int32_t>() == V4L2_EXPOSURE_MANUAL) ? true : false;
> - });
> - if (it != v4l2Values.end())
> - values.back() = static_cast<int32_t>(controls::ExposureTimeModeManual);
> -
> - info = ControlInfo{Span<int32_t>{values}, values[0]};
> + for (const ControlValue &value : v4l2Values) {
> + auto x = value.get<int32_t>();
> + if (0 <= x && size_t(x) < availableExposureModes_.size())
> + availableExposureModes_[x] = true;
> + }
> +
> + std::array<ControlValue, 2> values;
> + std::size_t count = 0;
> +
> + if (availableExposureModes_[V4L2_EXPOSURE_AUTO] || availableExposureModes_[V4L2_EXPOSURE_APERTURE_PRIORITY])
> + values[count++] = controls::ExposureTimeModeAuto;
> +
> + if (availableExposureModes_[V4L2_EXPOSURE_MANUAL] || availableExposureModes_[V4L2_EXPOSURE_SHUTTER_PRIORITY])
> + values[count++] = controls::ExposureTimeModeManual;
> +
> + if (count == 0)
> + return;
> +
> + info = ControlInfo{Span<const ControlValue>{ values.data(), count }, values[0]};
isn't this simpler ?
--- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
+++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
@@ -723,7 +723,7 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
* ExposureTimeModeManual = { V4L2_EXPOSURE_MANUAL,
* V4L2_EXPOSURE_SHUTTER_PRIORITY }
*/
- std::array<int32_t, 2> values{};
+ std::array<ControlValue, 2> values{};
auto it = std::find_if(v4l2Values.begin(), v4l2Values.end(),
[&](const ControlValue &val) {
@@ -731,7 +731,7 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
val.get<int32_t>() == V4L2_EXPOSURE_AUTO) ? true : false;
});
if (it != v4l2Values.end())
- values.back() = static_cast<int32_t>(controls::ExposureTimeModeAuto);
+ values.back() = controls::ExposureTimeModeAuto;
it = std::find_if(v4l2Values.begin(), v4l2Values.end(),
[&](const ControlValue &val) {
@@ -739,9 +739,9 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
val.get<int32_t>() == V4L2_EXPOSURE_MANUAL) ? true : false;
});
if (it != v4l2Values.end())
- values.back() = static_cast<int32_t>(controls::ExposureTimeModeManual);
+ values.back() = controls::ExposureTimeModeManual;
- info = ControlInfo{Span<int32_t>{values}, values[0]};
+ info = ControlInfo{Span<const ControlValue>{values}, values[0]};
break;
}
case V4L2_CID_EXPOSURE_ABSOLUTE:
> break;
> }
> case V4L2_CID_EXPOSURE_ABSOLUTE:
> --
> 2.48.1
>
>
More information about the libcamera-devel
mailing list