[PATCH v2 2/2] libcamera: pipeline: uvcvideo: Fix `ExposureTimeMode` control setting

[Barnabás Pőcze]

Hi


2025. február 27., csütörtök 22:32 keltezéssel, Kieran Bingham <kieran.bingham at ideasonboard.com> írta:

> Quoting Barnabás Pőcze (2025-02-17 18:53:39)
> > The mapping in `UVCCameraData::processControl()` is not entirely correct
> > because the control value is retrieved as a `bool` instead of `int32_t`.
> > Additionally, the available modes are not taken into account.
> >
> > To fix this, retrieve the control with the proper type - `int32_t` -,
> > and use the available modes stored in `UVCCameraData` to determine
> > which value to use for the V4L2 control.
> >
> > Fixes: bad8d591f8acfa ("libcamera: uvcvideo: Register ExposureTimeMode control")
> > Signed-off-by: Barnabás Pőcze <pobrn at protonmail.com>
> > ---
> >  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 38 +++++++++++++++-----
> >  1 file changed, 29 insertions(+), 9 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > index 1f604b91e..adab6aa18 100644
> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > @@ -103,8 +103,8 @@ public:
> >         bool match(DeviceEnumerator *enumerator) override;
> >
> >  private:
> > -       int processControl(ControlList *controls, unsigned int id,
> > -                          const ControlValue &value);
> > +       int processControl(UVCCameraData *data, ControlList *controls,
> > +                          unsigned int id, const ControlValue &value);
> >         int processControls(UVCCameraData *data, Request *request);
> >
> >         bool acquireDevice(Camera *camera) override;
> > @@ -297,8 +297,8 @@ void PipelineHandlerUVC::stopDevice(Camera *camera)
> >         data->video_->releaseBuffers();
> >  }
> >
> > -int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
> > -                                      const ControlValue &value)
> > +int PipelineHandlerUVC::processControl(UVCCameraData *data, ControlList *controls,
> > +                                      unsigned int id, const ControlValue &value)
> >  {
> >         uint32_t cid;
> >
> > @@ -342,10 +342,30 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
> >         }
> >
> >         case V4L2_CID_EXPOSURE_AUTO: {
> > -               int32_t ivalue = value.get<bool>()
> > -                              ? V4L2_EXPOSURE_APERTURE_PRIORITY
> > -                              : V4L2_EXPOSURE_MANUAL;
> > -               controls->set(V4L2_CID_EXPOSURE_AUTO, ivalue);
> > +               v4l2_exposure_auto_type exposureMode = {};
> > +
> > +               switch (value.get<int32_t>()) {
> > +               case controls::ExposureTimeModeAuto:
> > +                       if (data->availableExposureModes_[V4L2_EXPOSURE_AUTO])
> > +                               exposureMode = V4L2_EXPOSURE_AUTO;
> > +                       else if (data->availableExposureModes_[V4L2_EXPOSURE_APERTURE_PRIORITY])
> > +                               exposureMode = V4L2_EXPOSURE_APERTURE_PRIORITY;
> > +                       else
> 
> Is this really an impossible to reach code path ? Or can a rogue
> application 'decide' to try to set an unsupported mode just to see what
> would happen ?

I suppose you're right. I'm not sure why I thought it should be impossible to reach.
I'll change it to return -EINVAL.


Regards,
Barnabás Pőcze

> 
> > +                               ASSERT(false);
> > +                       break;
> > +               case controls::ExposureTimeModeManual:
> > +                       if (data->availableExposureModes_[V4L2_EXPOSURE_MANUAL])
> > +                               exposureMode = V4L2_EXPOSURE_MANUAL;
> > +                       else if (data->availableExposureModes_[V4L2_EXPOSURE_SHUTTER_PRIORITY])
> > +                               exposureMode = V4L2_EXPOSURE_SHUTTER_PRIORITY;
> > +                       else
> > +                               ASSERT(false);
> > +                       break;
> > +               default:
> > +                       return -EINVAL;
> > +               }
> > +
> > +               controls->set(V4L2_CID_EXPOSURE_AUTO, exposureMode);
> >                 break;
> >         }
> >
> > @@ -383,7 +403,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)
> >         ControlList controls(data->video_->controls());
> >
> >         for (const auto &[id, value] : request->controls())
> > -               processControl(&controls, id, value);
> > +               processControl(data, &controls, id, value);
> >
> >         for (const auto &ctrl : controls)
> >                 LOG(UVC, Debug)
> > --
> > 2.48.1
> >
> >
>