[PATCH v3 2/2] libcamera: pipeline: uvcvideo: Fix `ExposureTimeMode` control setting
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Tue Mar 4 11:56:36 CET 2025
Hi Barnabás
On Tue, Mar 04, 2025 at 11:46:24AM +0100, Barnabás Pőcze wrote:
> Hi
>
>
> 2025. 03. 04. 11:09 keltezéssel, Jacopo Mondi írta:
> > Hi Barnabás
> >
> > On Mon, Mar 03, 2025 at 02:42:34PM +0100, Barnabás Pőcze wrote:
> > > The mapping in `UVCCameraData::processControl()` is not entirely correct
> > > because the control value is retrieved as a `bool` instead of `int32_t`.
> > > Additionally, the available modes are not taken into account.
> > >
> > > To fix this, retrieve the control with the proper type - `int32_t` -, and
> > > use the V4L2 mode stored in `UVCCameraData` that was selected earlier in
> > > `UVCCameraData::addControl()` for the given libcamera exposure time mode.
> > >
> > > Fixes: bad8d591f8acfa ("libcamera: uvcvideo: Register ExposureTimeMode control")
> > > Signed-off-by: Barnabás Pőcze <barnabas.pocze at ideasonboard.com>
> > > ---
> > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 32 ++++++++++++++------
> > > 1 file changed, 23 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > index a6cc37366..dc3e85bd8 100644
> > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > @@ -97,8 +97,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;
> > > @@ -291,8 +291,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;
> > >
> > > @@ -336,10 +336,24 @@ 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);
> > > + int32_t exposureMode;
> > > +
> > > + switch (value.get<int32_t>()) {
> > > + case controls::ExposureTimeModeAuto:
> > > + if (!data->autoExposureMode_)
> > > + return -EINVAL;
> > > + exposureMode = *data->autoExposureMode_;
> >
> > That's more an open question, but according to your previous patch
> >
> > if (exposureModes[V4L2_EXPOSURE_AUTO])
> > autoExposureMode_ = V4L2_EXPOSURE_AUTO;
> > else if (exposureModes[V4L2_EXPOSURE_APERTURE_PRIORITY])
> > autoExposureMode_ = V4L2_EXPOSURE_APERTURE_PRIORITY;
> >
> > If the camera supported both V4L2_EXPOSURE_AUTO and
> > V4L2_EXPOSURE_APERTURE_PRIORITY autoExposureMode_ will always be
> > V4L2_EXPOSURE_AUTO meaning that we effectively make it impossible to
> > support APERTURE_PRIORITY by using auto exposure and manual gain.
>
> I based the order on the comment in the code:
>
> * ExposureTimeModeAuto = { V4L2_EXPOSURE_AUTO,
> * V4L2_EXPOSURE_APERTURE_PRIORITY }
> *
> *
> * ExposureTimeModeManual = { V4L2_EXPOSURE_MANUAL,
> * V4L2_EXPOSURE_SHUTTER_PRIORITY }
>
>
> >
> > I wonder if the above condition shouldn't be changed to
> >
> > if (exposureModes[V4L2_EXPOSURE_APERTURE_PRIORITY])
> > autoExposureMode_ = V4L2_EXPOSURE_APERTURE_PRIORITY;
> > else if (exposureModes[V4L2_EXPOSURE_AUTO])
> > autoExposureMode_ = V4L2_EXPOSURE_AUTO;
> >
> > so that if the camera supports _PRIORITY modes we use them to allow
> > implementing _PRIORITY modes in libcamera with ExposureTimeMode and
> > AnalogueGainMode.
> >
> > True that, if a user sets controls::ExposureTimeModeAuto and
> > controls::AnalogueGainModeAuto and here we set APERTURE_PRIORITY,
> > we're breaking it.
> >
> > Again, I'm not sure how many uvc cameras support PRIORITY modes, but
> > if we want to support the full handling of PRIORITY we should also
> > inspect AnalogueGainMode.
>
> I only have a single UVC camera that supports `V4L2_CID_EXPOSURE_AUTO`, and
> it implements `V4L2_EXPOSURE_MANUAL` and `V4L2_EXPOSURE_APERTURE_PRIORITY`.
>
Ah see, so it's not that uncommon
>
> >
> > The pipeline at the moment doesn't even register AnalogueGainMode so I
> > guess we can don't care about PRIORITIES, so what you have here is
> > fine.
> >
> > If someone wants to actually support priority modes in uvc, then there
> > is a bigger rework to be done most probably. I wonder if we should
> > capture it in a comment at least (can be a separate patch)
>
> I will open a bug report about this if that's fine?
Yes please, but also record in a comment that we don't support
priority modes but only Auto/Manual modes for both exposure and gain
>
> If I understand it correctly:
>
> gain
> A M
> exposure A auto aperture
> M shutter manual
That's my understanding as well
>
> This is how the two libcamera controls would be mapped to the v4l2 control.
> Will it not be an issue that you cannot express the supported feature set
> with ControlInfo exactly? E.g. if three are supported, or if two that are
> placed diagonally in the above table. I don't know if this is just theoretical,
> if such scenario is even possible/allowed.
aperture and shutter priorty modes should be implemented by
registering AnalogueGainMode as well.
Mode AnalogueGainMode ExposureTimeMode
AUTO { Auto} { Auto }
MANUAL { Manual } { Manual }
APERTURE { Manual } { Auto }
SHUTTER { Auto } { Manual }
Users should inspect the control info for both controls, and in the
case of your webcam that supports {AUTO, MANUAL, APERTURE }
they should read
AnalogueGainMode: { Auto, Manual }
ExposureTimeMode: { Auto, Manual }
Valid combinations will be
AnalogueGainMode, ExposureTimeMode = { Auto, Auto }
AnalogueGainMode, ExposureTimeMode = { Manual, Manual }
AnalogueGainMode, ExposureTimeMode = { Manual, Auto }
So yes, we can't express {Auto, Manual} (SHUTTER) is not valid until a
user tries it...
>
>
> Regards,
> Barnabás Pőcze
>
> >
> > > + break;
> > > + case controls::ExposureTimeModeManual:
> > > + if (!data->manualExposureMode_)
> > > + return -EINVAL;
> > > + exposureMode = *data->manualExposureMode_;
> > > + break;
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > +
> > > + controls->set(V4L2_CID_EXPOSURE_AUTO, exposureMode);
> >
> > Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> >
> > Thanks
> > j
> >
> > > break;
> > > }
> > >
> > > @@ -377,7 +391,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)
> > > --
> > > 2.48.1
> > >
>
More information about the libcamera-devel
mailing list