| Message ID | 20250217185327.306509-3-pobrn@protonmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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 ? > + 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 > >
Hi 2025. február 27., csütörtök 22:32 keltezéssel, Kieran Bingham <kieran.bingham@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@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 > > > > >
Hi Barnabás nice fix On Mon, Feb 17, 2025 at 06:53:39PM +0000, Barnabás Pőcze wrote: > 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. With the assertion removal as suggested by Kieran Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> (I actually wonder if it is possible to receive a controls::ExposureTimeModeAuto !data->availableExposureModes_[V4L2_EXPOSURE_AUTO] && !data->availableExposureModes_[V4L2_EXPOSURE_APERTURE_PRIORITY]) as the Camera shouldn't register controls::ExposureTimeModeAuto in the ::controls() ControlInfoMap... However, checking for this surely doesn't hurt. > > Fixes: bad8d591f8acfa ("libcamera: uvcvideo: Register ExposureTimeMode control") > Signed-off-by: Barnabás Pőcze <pobrn@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 > + 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 > >
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 + 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)
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@protonmail.com> --- src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 38 +++++++++++++++----- 1 file changed, 29 insertions(+), 9 deletions(-)