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

[Barnabás Pőcze]

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.


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