[PATCH v3 4/4] libcamera: pipeline: uvcvideo: Fix `ExposureTime` control handling
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Fri Mar 21 15:41:45 CET 2025
On Fri, Mar 21, 2025 at 03:33:59PM +0100, Barnabás Pőcze wrote:
> Hi
>
> 2025. 03. 21. 15:17 keltezéssel, Jacopo Mondi írta:
> > Hi Barnabás
> >
> > On Fri, Mar 14, 2025 at 06:42:48PM +0100, Barnabás Pőcze wrote:
> > > The documentation of `ExposureTime` states that its value is ignored if
> > > `ExposureTimeMode` is not manual. This is currently not handled, and if
> > > `ExposureTimeMode` is automatic and `ExposureTime` is set, then the
> > > controls will be rejected, as expected.
> > >
> > > Only try to set `V4L2_CID_EXPOSURE_ABSOLUTE` if the current exposure mode
> > > is manual. To be able to handle requests that set both `ExposureTime{,Mode}`,
> > > process `ExposureTimeMode` first directly in `processControls()`, and store
> > > this new mode in a temporary location. Then have `processControl()` act on
> > > this temporary state, and only persist the temporary state if `setControls()`
> > > on the video device succeeds.
> > >
> > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=242
> > > Signed-off-by: Barnabás Pőcze <barnabas.pocze at ideasonboard.com>
> > > ---
> > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 52 ++++++++++++--------
> > > 1 file changed, 31 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > index 7d882ebe1..22d286e49 100644
> > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > @@ -108,7 +108,7 @@ public:
> > > bool match(DeviceEnumerator *enumerator) override;
> > >
> > > private:
> > > - int processControl(const UVCCameraData *data, ControlList *controls,
> > > + int processControl(const UVCCameraData::State &state, ControlList *controls,
> > > unsigned int id, const ControlValue &value);
> > > int processControls(UVCCameraData *data, Request *request);
> > >
> > > @@ -333,7 +333,7 @@ void PipelineHandlerUVC::stopDevice(Camera *camera)
> > > data->state_.reset();
> > > }
> > >
> > > -int PipelineHandlerUVC::processControl(const UVCCameraData *data, ControlList *controls,
> > > +int PipelineHandlerUVC::processControl(const UVCCameraData::State &state, ControlList *controls,
> > > unsigned int id, const ControlValue &value)
> > > {
> > > uint32_t cid;
> > > @@ -378,26 +378,13 @@ int PipelineHandlerUVC::processControl(const UVCCameraData *data, ControlList *c
> > > }
> > >
> > > case V4L2_CID_EXPOSURE_AUTO: {
> > > - std::optional<v4l2_exposure_auto_type> mode;
> > > -
> > > - switch (value.get<int32_t>()) {
> > > - case controls::ExposureTimeModeAuto:
> > > - mode = data->autoExposureMode_;
> > > - break;
> > > - case controls::ExposureTimeModeManual:
> > > - mode = data->manualExposureMode_;
> > > - break;
> > > - }
> > > -
> > > - if (!mode)
> > > - return -EINVAL;
> > > -
> > > - controls->set(V4L2_CID_EXPOSURE_AUTO, static_cast<int32_t>(*mode));
> > > + /* Handled directly in `processControls()`. */
> > > break;
> > > }
> > >
> > > case V4L2_CID_EXPOSURE_ABSOLUTE:
> > > - controls->set(cid, value.get<int32_t>() / 100);
> > > + if (state.exp == controls::ExposureTimeModeManual)
> > > + controls->set(cid, value.get<int32_t>() / 100);
> > > break;
> > >
> > > case V4L2_CID_CONTRAST:
> > > @@ -428,9 +415,30 @@ int PipelineHandlerUVC::processControl(const UVCCameraData *data, ControlList *c
> > > int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)
> > > {
> > > ControlList controls(data->video_->controls());
> > > + const auto &reqControls = request->controls();
> > > + auto newState = data->state_;
> > > +
> > > + if (const auto exp = reqControls.get(controls::ExposureTimeMode)) {
> > > + std::optional<v4l2_exposure_auto_type> mode;
> > >
> > > - for (const auto &[id, value] : request->controls())
> > > - processControl(data, &controls, id, value);
> > > + switch (*exp) {
> > > + case controls::ExposureTimeModeAuto:
> > > + mode = data->autoExposureMode_;
> > > + break;
> > > + case controls::ExposureTimeModeManual:
> > > + mode = data->manualExposureMode_;
> > > + break;
> > > + }
> > > +
> > > + if (!mode)
> > > + return -EINVAL;
> > > +
> > > + controls.set(V4L2_CID_EXPOSURE_AUTO, static_cast<int32_t>(*mode));
> > > + newState.exp = static_cast<controls::ExposureTimeModeEnum>(*exp);
> >
> > Can't you just update data->state before calling processControl to
> > avoid passing newState to that function ?
>
> If the controls cannot be set, then I think the state should not be changed, this
> is why it operates on a copy, which is only persisted after `setControls()` succeeds.
Which control ? Even if ABSOLUTE cannot be set (because we're in AUTO)
if the mode is changed, it should anyway be
Ah you mean to protect against a failure in
int ret = data->video_->setControls(&controls);
?
Then yes, maybe keep the state update after this makes sense
Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
Thanks
j
>
>
> Regards,
> Barnabás Pőcze
>
> >
> > > + }
> > > +
> > > + for (const auto &[id, value] : reqControls)
> > > + processControl(newState, &controls, id, value);
> > >
> > > for (const auto &ctrl : controls)
> > > LOG(UVC, Debug)
> > > @@ -443,7 +451,9 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)
> > > return ret < 0 ? ret : -EINVAL;
> > > }
> > >
> > > - return ret;
> > > + data->state_ = newState;
> > > +
> > > + return 0;
> > > }
> > >
> > > int PipelineHandlerUVC::queueRequestDevice(Camera *camera, Request *request)
> > > --
> > > 2.48.1
> > >
>
More information about the libcamera-devel
mailing list