[v4] libcamera: pipeline: uvcvideo: Fix `ExposureTime` control handling

23340 diff mbox series

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.

To fix that, keep track of the current value of `ExposureTimeMode`, and
only set `V4L2_CID_EXPOSURE_ABSOLUTE` if the current exposure mode is
manual.

However, the current state of `V4L2_CID_EXPOSURE_AUTO` is not retrieved
when streaming starts, thus if the user does not set `ExposureTimeMode`
then it will remain in an "unset" state and all attempts to set
`ExposureTime` will be ignored. This is done for two reasons:

  * the current values of controls cannot be retrieved with libcamera,
    therefore this unset state cannot be observed by the user;
  * the user will have to set `ExposureTimeMode` either way if they
    want to guarantee that setting `ExposureTime` will have any effect.

If `ExposureTimeMode` is not exposed by the camera, but `ExposeTime` is
supported, then the exposure mode is assumed to be manual to allow
changing `V4L2_CID_EXPOSURE_ABSOLUTE`.

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@ideasonboard.com>
---
changes in v4:
  * first two patches of v3 already merged
  * last two patches of v3 combined into this single patch
  * do not retrieve V4L2_CID_EXPOSURE_AUTO when streaming starts

changes in v3:
  * fix default value: set it based on the v4l2 default
  * store current value of `ExposureTimeMode`
  * fix https://bugs.libcamera.org/show_bug.cgi?id=242 as well

changes in v2:
  * split into two patches
  * make it compile with old libstdc++ when _GLIBCXX_DEBUG is set

v3: https://patchwork.libcamera.org/patch/22959/
    https://patchwork.libcamera.org/patch/22958/
v2: https://patchwork.libcamera.org/project/libcamera/list/?series=5006
v1: https://patchwork.libcamera.org/project/libcamera/list/?series=4974
---
 src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 67 ++++++++++++++------
 1 file changed, 46 insertions(+), 21 deletions(-)

--
2.49.0

Quoting Barnabás Pőcze (2025-05-06 16:53:14)
> 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.
> 
> To fix that, keep track of the current value of `ExposureTimeMode`, and
> only set `V4L2_CID_EXPOSURE_ABSOLUTE` if the current exposure mode is
> manual.
> 
> However, the current state of `V4L2_CID_EXPOSURE_AUTO` is not retrieved
> when streaming starts, thus if the user does not set `ExposureTimeMode`
> then it will remain in an "unset" state and all attempts to set
> `ExposureTime` will be ignored. This is done for two reasons:
> 
>   * the current values of controls cannot be retrieved with libcamera,
>     therefore this unset state cannot be observed by the user;
>   * the user will have to set `ExposureTimeMode` either way if they
>     want to guarantee that setting `ExposureTime` will have any effect.
> 
> If `ExposureTimeMode` is not exposed by the camera, but `ExposeTime` is
> supported, then the exposure mode is assumed to be manual to allow
> changing `V4L2_CID_EXPOSURE_ABSOLUTE`.
> 
> 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@ideasonboard.com>
> ---
> changes in v4:
>   * first two patches of v3 already merged
>   * last two patches of v3 combined into this single patch
>   * do not retrieve V4L2_CID_EXPOSURE_AUTO when streaming starts
> 
> changes in v3:
>   * fix default value: set it based on the v4l2 default
>   * store current value of `ExposureTimeMode`
>   * fix https://bugs.libcamera.org/show_bug.cgi?id=242 as well
> 
> changes in v2:
>   * split into two patches
>   * make it compile with old libstdc++ when _GLIBCXX_DEBUG is set
> 
> v3: https://patchwork.libcamera.org/patch/22959/
>     https://patchwork.libcamera.org/patch/22958/
> v2: https://patchwork.libcamera.org/project/libcamera/list/?series=5006
> v1: https://patchwork.libcamera.org/project/libcamera/list/?series=4974
> ---
>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 67 ++++++++++++++------
>  1 file changed, 46 insertions(+), 21 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index 586e932d2..af0c8dedd 100644
> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> @@ -53,6 +53,8 @@ public:
> 
>         const std::string &id() const { return id_; }
> 
> +       bool supportsExposureTimeMode() const { return autoExposureMode_ || manualExposureMode_; }
> +
>         Mutex openLock_;
>         std::unique_ptr<V4L2VideoDevice> video_;
>         Stream stream_;
> @@ -61,6 +63,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_ = {};

Doesn't std::optional already support .reset() [0] ? So I can't see benefit
to adding this extra State wrapper ?

[0] https://en.cppreference.com/w/cpp/utility/optional/reset.html

Or is it that you anticipate extra modes to be tracked in this state ?

> +
>  private:
>         bool generateId();
> 
> @@ -98,7 +109,7 @@ public:
>         bool match(DeviceEnumerator *enumerator) override;
> 
>  private:
> -       int processControl(const UVCCameraData *data, ControlList *controls,
> +       int processControl(const UVCCameraData::State &state, ControlList *controls,

Oh! I see - passing State makes much more sense.

Ignore the above then ;-)

Does this interact ok with the AeEnable handling that occurs in
src/libcamera/camera.cpp?

>                            unsigned int id, const ControlValue &value);
>         int processControls(UVCCameraData *data, Request *request);
> 
> @@ -302,6 +313,9 @@ int PipelineHandlerUVC::start(Camera *camera, [[maybe_unused]] const ControlList
>                 return ret;
>         }
> 
> +       if (!data->supportsExposureTimeMode())
> +               data->state_.exp = controls::ExposureTimeModeManual;
> +
>         return 0;
>  }
> 
> @@ -310,9 +324,10 @@ 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,
> +int PipelineHandlerUVC::processControl(const UVCCameraData::State &state, ControlList *controls,
>                                        unsigned int id, const ControlValue &value)
>  {
>         uint32_t cid;
> @@ -359,26 +374,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:
> @@ -413,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_;
> 
> -       for (const auto &[id, value] : request->controls())
> -               processControl(data, &controls, id, value);
> +       if (const auto exp = reqControls.get(controls::ExposureTimeMode)) {
> +               std::optional<v4l2_exposure_auto_type> mode;
> +
> +               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);
> +       }
> +
> +       for (const auto &[id, value] : reqControls)
> +               processControl(newState, &controls, id, value);
> 
>         for (const auto &ctrl : controls)
>                 LOG(UVC, Debug)
> @@ -428,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.49.0

On Mon, Jul 21, 2025 at 08:00:49PM +0100, Kieran Bingham wrote:
> Quoting Barnabás Pőcze (2025-05-06 16:53:14)
> > 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.
> > 
> > To fix that, keep track of the current value of `ExposureTimeMode`, and
> > only set `V4L2_CID_EXPOSURE_ABSOLUTE` if the current exposure mode is
> > manual.
> > 
> > However, the current state of `V4L2_CID_EXPOSURE_AUTO` is not retrieved
> > when streaming starts, thus if the user does not set `ExposureTimeMode`
> > then it will remain in an "unset" state and all attempts to set
> > `ExposureTime` will be ignored. This is done for two reasons:
> > 
> >   * the current values of controls cannot be retrieved with libcamera,
> >     therefore this unset state cannot be observed by the user;
> >   * the user will have to set `ExposureTimeMode` either way if they
> >     want to guarantee that setting `ExposureTime` will have any effect.

Shouldn't we actually set all controls to their default value when
streaming start ?

> > If `ExposureTimeMode` is not exposed by the camera, but `ExposeTime` is
> > supported, then the exposure mode is assumed to be manual to allow
> > changing `V4L2_CID_EXPOSURE_ABSOLUTE`.
> > 
> > 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@ideasonboard.com>
> > ---
> > changes in v4:
> >   * first two patches of v3 already merged
> >   * last two patches of v3 combined into this single patch
> >   * do not retrieve V4L2_CID_EXPOSURE_AUTO when streaming starts
> > 
> > changes in v3:
> >   * fix default value: set it based on the v4l2 default
> >   * store current value of `ExposureTimeMode`
> >   * fix https://bugs.libcamera.org/show_bug.cgi?id=242 as well
> > 
> > changes in v2:
> >   * split into two patches
> >   * make it compile with old libstdc++ when _GLIBCXX_DEBUG is set
> > 
> > v3: https://patchwork.libcamera.org/patch/22959/
> >     https://patchwork.libcamera.org/patch/22958/
> > v2: https://patchwork.libcamera.org/project/libcamera/list/?series=5006
> > v1: https://patchwork.libcamera.org/project/libcamera/list/?series=4974
> > ---
> >  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 67 ++++++++++++++------
> >  1 file changed, 46 insertions(+), 21 deletions(-)
> > 
> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > index 586e932d2..af0c8dedd 100644
> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > @@ -53,6 +53,8 @@ public:
> > 
> >         const std::string &id() const { return id_; }
> > 
> > +       bool supportsExposureTimeMode() const { return autoExposureMode_ || manualExposureMode_; }
> > +
> >         Mutex openLock_;
> >         std::unique_ptr<V4L2VideoDevice> video_;
> >         Stream stream_;
> > @@ -61,6 +63,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_ = {};
> 
> Doesn't std::optional already support .reset() [0] ? So I can't see benefit
> to adding this extra State wrapper ?
> 
> [0] https://en.cppreference.com/w/cpp/utility/optional/reset.html
> 
> Or is it that you anticipate extra modes to be tracked in this state ?
> 
> > +
> >  private:
> >         bool generateId();
> > 
> > @@ -98,7 +109,7 @@ public:
> >         bool match(DeviceEnumerator *enumerator) override;
> > 
> >  private:
> > -       int processControl(const UVCCameraData *data, ControlList *controls,
> > +       int processControl(const UVCCameraData::State &state, ControlList *controls,
> 
> Oh! I see - passing State makes much more sense.
> 
> Ignore the above then ;-)
> 
> Does this interact ok with the AeEnable handling that occurs in
> src/libcamera/camera.cpp?
> 
> >                            unsigned int id, const ControlValue &value);
> >         int processControls(UVCCameraData *data, Request *request);
> > 
> > @@ -302,6 +313,9 @@ int PipelineHandlerUVC::start(Camera *camera, [[maybe_unused]] const ControlList
> >                 return ret;
> >         }
> > 
> > +       if (!data->supportsExposureTimeMode())
> > +               data->state_.exp = controls::ExposureTimeModeManual;
> > +
> >         return 0;
> >  }
> > 
> > @@ -310,9 +324,10 @@ 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,
> > +int PipelineHandlerUVC::processControl(const UVCCameraData::State &state, ControlList *controls,
> >                                        unsigned int id, const ControlValue &value)
> >  {
> >         uint32_t cid;
> > @@ -359,26 +374,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:
> > @@ -413,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_;

Please use explicit types when possible to increase readability.

> > 
> > -       for (const auto &[id, value] : request->controls())
> > -               processControl(data, &controls, id, value);

A comment here to explain why this happens outside of
PipelineHandlerUVC::processControl() would be useful.

> > +       if (const auto exp = reqControls.get(controls::ExposureTimeMode)) {

And avoid abbreviating too much (including in the State structure).

> > +               std::optional<v4l2_exposure_auto_type> mode;
> > +
> > +               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);
> > +       }
> > +
> > +       for (const auto &[id, value] : reqControls)
> > +               processControl(newState, &controls, id, value);
> > 
> >         for (const auto &ctrl : controls)
> >                 LOG(UVC, Debug)
> > @@ -428,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)

Hi

2025. 07. 24. 13:45 keltezéssel, Laurent Pinchart írta:
> On Mon, Jul 21, 2025 at 08:00:49PM +0100, Kieran Bingham wrote:
>> Quoting Barnabás Pőcze (2025-05-06 16:53:14)
>>> 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.
>>>
>>> To fix that, keep track of the current value of `ExposureTimeMode`, and
>>> only set `V4L2_CID_EXPOSURE_ABSOLUTE` if the current exposure mode is
>>> manual.
>>>
>>> However, the current state of `V4L2_CID_EXPOSURE_AUTO` is not retrieved
>>> when streaming starts, thus if the user does not set `ExposureTimeMode`
>>> then it will remain in an "unset" state and all attempts to set
>>> `ExposureTime` will be ignored. This is done for two reasons:
>>>
>>>    * the current values of controls cannot be retrieved with libcamera,
>>>      therefore this unset state cannot be observed by the user;
>>>    * the user will have to set `ExposureTimeMode` either way if they
>>>      want to guarantee that setting `ExposureTime` will have any effect.
> 
> Shouldn't we actually set all controls to their default value when
> streaming start ?

See https://bugs.libcamera.org/show_bug.cgi?id=271. It's not clear to me
what the behaviour should be. Setting the default values are some point
is probably desired.


Regards,
Barnabás Pőcze

> 
>>> If `ExposureTimeMode` is not exposed by the camera, but `ExposeTime` is
>>> supported, then the exposure mode is assumed to be manual to allow
>>> changing `V4L2_CID_EXPOSURE_ABSOLUTE`.
>>>
>>> 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@ideasonboard.com>
>>> ---
>>> changes in v4:
>>>    * first two patches of v3 already merged
>>>    * last two patches of v3 combined into this single patch
>>>    * do not retrieve V4L2_CID_EXPOSURE_AUTO when streaming starts
>>>
>>> changes in v3:
>>>    * fix default value: set it based on the v4l2 default
>>>    * store current value of `ExposureTimeMode`
>>>    * fix https://bugs.libcamera.org/show_bug.cgi?id=242 as well
>>>
>>> changes in v2:
>>>    * split into two patches
>>>    * make it compile with old libstdc++ when _GLIBCXX_DEBUG is set
>>>
>>> v3: https://patchwork.libcamera.org/patch/22959/
>>>      https://patchwork.libcamera.org/patch/22958/
>>> v2: https://patchwork.libcamera.org/project/libcamera/list/?series=5006
>>> v1: https://patchwork.libcamera.org/project/libcamera/list/?series=4974
>>> ---
>>>   src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 67 ++++++++++++++------
>>>   1 file changed, 46 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
>>> index 586e932d2..af0c8dedd 100644
>>> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
>>> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
>>> @@ -53,6 +53,8 @@ public:
>>>
>>>          const std::string &id() const { return id_; }
>>>
>>> +       bool supportsExposureTimeMode() const { return autoExposureMode_ || manualExposureMode_; }
>>> +
>>>          Mutex openLock_;
>>>          std::unique_ptr<V4L2VideoDevice> video_;
>>>          Stream stream_;
>>> @@ -61,6 +63,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_ = {};
>>
>> Doesn't std::optional already support .reset() [0] ? So I can't see benefit
>> to adding this extra State wrapper ?
>>
>> [0] https://en.cppreference.com/w/cpp/utility/optional/reset.html
>>
>> Or is it that you anticipate extra modes to be tracked in this state ?
>>
>>> +
>>>   private:
>>>          bool generateId();
>>>
>>> @@ -98,7 +109,7 @@ public:
>>>          bool match(DeviceEnumerator *enumerator) override;
>>>
>>>   private:
>>> -       int processControl(const UVCCameraData *data, ControlList *controls,
>>> +       int processControl(const UVCCameraData::State &state, ControlList *controls,
>>
>> Oh! I see - passing State makes much more sense.
>>
>> Ignore the above then ;-)
>>
>> Does this interact ok with the AeEnable handling that occurs in
>> src/libcamera/camera.cpp?
>>
>>>                             unsigned int id, const ControlValue &value);
>>>          int processControls(UVCCameraData *data, Request *request);
>>>
>>> @@ -302,6 +313,9 @@ int PipelineHandlerUVC::start(Camera *camera, [[maybe_unused]] const ControlList
>>>                  return ret;
>>>          }
>>>
>>> +       if (!data->supportsExposureTimeMode())
>>> +               data->state_.exp = controls::ExposureTimeModeManual;
>>> +
>>>          return 0;
>>>   }
>>>
>>> @@ -310,9 +324,10 @@ 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,
>>> +int PipelineHandlerUVC::processControl(const UVCCameraData::State &state, ControlList *controls,
>>>                                         unsigned int id, const ControlValue &value)
>>>   {
>>>          uint32_t cid;
>>> @@ -359,26 +374,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:
>>> @@ -413,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_;
> 
> Please use explicit types when possible to increase readability.
> 
>>>
>>> -       for (const auto &[id, value] : request->controls())
>>> -               processControl(data, &controls, id, value);
> 
> A comment here to explain why this happens outside of
> PipelineHandlerUVC::processControl() would be useful.
> 
>>> +       if (const auto exp = reqControls.get(controls::ExposureTimeMode)) {
> 
> And avoid abbreviating too much (including in the State structure).
> 
>>> +               std::optional<v4l2_exposure_auto_type> mode;
>>> +
>>> +               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);
>>> +       }
>>> +
>>> +       for (const auto &[id, value] : reqControls)
>>> +               processControl(newState, &controls, id, value);
>>>
>>>          for (const auto &ctrl : controls)
>>>                  LOG(UVC, Debug)
>>> @@ -428,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)