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

[Barnabás Pőcze]

Hi


2025. 03. 21. 15:53 keltezéssel, Jacopo Mondi írta:
> 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..)

Hmm... I like `ActiveState` better than `ControlsState`, but let's continue
this discussion at the new version of the patch.


Regards,
Barnabás Pőcze


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