[PATCH v3 3/4] libcamera: pipeline: uvcvideo: Retrieve current exposure mode on start

[Jacopo Mondi]

Hi Barnabás

On Fri, Mar 21, 2025 at 03:36:10PM +0100, Barnabás Pőcze wrote:
> Hi
>
> 2025. 03. 21. 15:02 keltezéssel, Jacopo Mondi írta:
> > Hi Barnabás
> >
> > On Fri, Mar 14, 2025 at 06:42:47PM +0100, Barnabás Pőcze wrote:
> > > If `ExposureTimeMode` is supported, then retrieve the current value
> > > of `V4L2_CID_EXPOSURE_AUTO` in `PipelineHandlerUVC::start()`, and
> > > convert it to the appropriate `ExposureTimeModeEnum` value. If it
> > > is not supported or the conversion fails, then fall back to assuming
> > > that `ExposureTimeModeManual` is in effect.
> > >
> > > This is necessary to be able to properly handle the `ExposureTime`
> > > control as its value is required to be ignored if `ExposureTimeMode`
> > > is not manual.
> > >
> > > Signed-off-by: Barnabás Pőcze <barnabas.pocze at ideasonboard.com>
> > > ---
> > >   src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 20 ++++++++++++++++++++
> > >   1 file changed, 20 insertions(+)
> > >
> > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > index 4ff79c291..7d882ebe1 100644
> > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > @@ -62,6 +62,15 @@ public:
> > >   	std::optional<v4l2_exposure_auto_type> autoExposureMode_;
> > >   	std::optional<v4l2_exposure_auto_type> manualExposureMode_;
> > >
> > > +	struct State {
> > > +		std::optional<controls::ExposureTimeModeEnum> exp;
> > > +
> > > +		void reset()
> > > +		{
> > > +			exp.reset();
> > > +		}
> > > +	} state_;
> >
> > Do you expect this to grow ? Otherwise there's no much difference than
> > just using an optional<> directly.
>
> No idea to be honest, but state objects are passed to functions, so
> it's certainly less typing.
>

True, and probably more tidy than just passing the optional<> around..

Let's make it clear this is about controls maybe naming the type
ControlsState ? (uvc doesn't have an IPA, but in IPA-terms this would
actually be the ActiveState..)

>
> >
> > > +
> > >   private:
> > >   	bool generateId();
> > >
> > > @@ -303,6 +312,16 @@ int PipelineHandlerUVC::start(Camera *camera, [[maybe_unused]] const ControlList
> > >   		return ret;
> > >   	}
> > >
> > > +	if (data->autoExposureMode_ || data->manualExposureMode_) {
> > > +		const auto &ctrls = data->video_->getControls({ V4L2_CID_EXPOSURE_AUTO });
> > > +		const auto &value = ctrls.get(V4L2_CID_EXPOSURE_AUTO);
> >
> > Is this safe in case there's no V4L2_CID_EXPOSURE_AUTO ? Isn't it
> > safer to check
>
> It is apparently not, documentation says undefined behaviour. I have
> added a `ctrls.contains(...)` check.
>
>
> >
> >                  if (!ctrls.empty()) {
> >                          const auto &value = ctrls.get(V4L2_CID_EXPOSURE_AUTO);
> > 			data->state_.exp = v4l2ToExposureMode(value.get<int32_t>());
> >
> >                  }
> > > +		if (!value.isNone())
> > > +			data->state_.exp = v4l2ToExposureMode(value.get<int32_t>());
> > > +	}
> > > +
> > > +	if (!data->state_.exp)
> > > +		data->state_.exp = controls::ExposureTimeModeManual;
> > > +
> >
> > I presume this is safer than just relying on the control default
> > value, specifically in start-stop-start sessions (if controls are not
> > reset in between ofc)
>
> At this point I have different thoughts every time I look at this...
> This is a fallback path for very unlikely scenarios, but now I am
> thinking that the current value shouldn't even be retrieved for
> two reasons:
>
>   * the current values of controls are not retrievable by the user application
>     (with libcamera at least);
>   * the user application will have to set `ExposureTimeMode=Manual` explicitly
>     before setting `ExposureTime` at least once (otherwise correct operation is
>     not guaranteed by the docs) and at that point up to date state information
>     will be available.
>
> Or is there possibly an expectation that the controls will have their "default"
> values when streaming starts? That is surely not guaranteed by the UVC pipeline
> handler, do others guarantee that?
>
>
> Regards,
> Barnabás Pőcze
>
> >
> > >   	return 0;
> > >   }
> > >
> > > @@ -311,6 +330,7 @@ void PipelineHandlerUVC::stopDevice(Camera *camera)
> > >   	UVCCameraData *data = cameraData(camera);
> > >   	data->video_->streamOff();
> > >   	data->video_->releaseBuffers();
> > > +	data->state_.reset();
> > >   }
> > >
> > >   int PipelineHandlerUVC::processControl(const UVCCameraData *data, ControlList *controls,
> > > --
> > > 2.48.1
> > >
>