[v3,3/4] libcamera: pipeline: uvcvideo: Retrieve current exposure mode on start
diff mbox series

Message ID 20250314174248.1015718-4-barnabas.pocze@ideasonboard.com
State Deferred
Headers show
Series
  • libcamera: pipeline: uvcvideo: Fix `ExposureTimeMode` control
Related show

Commit Message

Barnabás Pőcze March 14, 2025, 5:42 p.m. UTC
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@ideasonboard.com>
---
 src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Comments

Jacopo Mondi March 21, 2025, 2:02 p.m. UTC | #1
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@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.

> +
>  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

                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)

>  	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
>
Barnabás Pőcze March 21, 2025, 2:36 p.m. UTC | #2
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@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.


> 
>> +
>>   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
>>
Jacopo Mondi March 21, 2025, 2:53 p.m. UTC | #3
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@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
> > >
>

Patch
diff mbox series

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_;
+
 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);
+		if (!value.isNone())
+			data->state_.exp = v4l2ToExposureMode(value.get<int32_t>());
+	}
+
+	if (!data->state_.exp)
+		data->state_.exp = controls::ExposureTimeModeManual;
+
 	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,