| Message ID | 20250303134234.699293-3-barnabas.pocze@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás On Mon, Mar 03, 2025 at 02:42:34PM +0100, 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 V4L2 mode stored in `UVCCameraData` that was selected earlier in > `UVCCameraData::addControl()` for the given libcamera exposure time mode. > > Fixes: bad8d591f8acfa ("libcamera: uvcvideo: Register ExposureTimeMode control") > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 32 ++++++++++++++------ > 1 file changed, 23 insertions(+), 9 deletions(-) > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > index a6cc37366..dc3e85bd8 100644 > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > @@ -97,8 +97,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; > @@ -291,8 +291,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; > > @@ -336,10 +336,24 @@ 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); > + int32_t exposureMode; > + > + switch (value.get<int32_t>()) { > + case controls::ExposureTimeModeAuto: > + if (!data->autoExposureMode_) > + return -EINVAL; > + exposureMode = *data->autoExposureMode_; That's more an open question, but according to your previous patch if (exposureModes[V4L2_EXPOSURE_AUTO]) autoExposureMode_ = V4L2_EXPOSURE_AUTO; else if (exposureModes[V4L2_EXPOSURE_APERTURE_PRIORITY]) autoExposureMode_ = V4L2_EXPOSURE_APERTURE_PRIORITY; If the camera supported both V4L2_EXPOSURE_AUTO and V4L2_EXPOSURE_APERTURE_PRIORITY autoExposureMode_ will always be V4L2_EXPOSURE_AUTO meaning that we effectively make it impossible to support APERTURE_PRIORITY by using auto exposure and manual gain. I wonder if the above condition shouldn't be changed to if (exposureModes[V4L2_EXPOSURE_APERTURE_PRIORITY]) autoExposureMode_ = V4L2_EXPOSURE_APERTURE_PRIORITY; else if (exposureModes[V4L2_EXPOSURE_AUTO]) autoExposureMode_ = V4L2_EXPOSURE_AUTO; so that if the camera supports _PRIORITY modes we use them to allow implementing _PRIORITY modes in libcamera with ExposureTimeMode and AnalogueGainMode. True that, if a user sets controls::ExposureTimeModeAuto and controls::AnalogueGainModeAuto and here we set APERTURE_PRIORITY, we're breaking it. Again, I'm not sure how many uvc cameras support PRIORITY modes, but if we want to support the full handling of PRIORITY we should also inspect AnalogueGainMode. The pipeline at the moment doesn't even register AnalogueGainMode so I guess we can don't care about PRIORITIES, so what you have here is fine. If someone wants to actually support priority modes in uvc, then there is a bigger rework to be done most probably. I wonder if we should capture it in a comment at least (can be a separate patch) > + break; > + case controls::ExposureTimeModeManual: > + if (!data->manualExposureMode_) > + return -EINVAL; > + exposureMode = *data->manualExposureMode_; > + break; > + default: > + return -EINVAL; > + } > + > + controls->set(V4L2_CID_EXPOSURE_AUTO, exposureMode); Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > break; > } > > @@ -377,7 +391,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. 03. 04. 11:09 keltezéssel, Jacopo Mondi írta: > Hi Barnabás > > On Mon, Mar 03, 2025 at 02:42:34PM +0100, 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 V4L2 mode stored in `UVCCameraData` that was selected earlier in >> `UVCCameraData::addControl()` for the given libcamera exposure time mode. >> >> Fixes: bad8d591f8acfa ("libcamera: uvcvideo: Register ExposureTimeMode control") >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> --- >> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 32 ++++++++++++++------ >> 1 file changed, 23 insertions(+), 9 deletions(-) >> >> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp >> index a6cc37366..dc3e85bd8 100644 >> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp >> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp >> @@ -97,8 +97,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; >> @@ -291,8 +291,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; >> >> @@ -336,10 +336,24 @@ 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); >> + int32_t exposureMode; >> + >> + switch (value.get<int32_t>()) { >> + case controls::ExposureTimeModeAuto: >> + if (!data->autoExposureMode_) >> + return -EINVAL; >> + exposureMode = *data->autoExposureMode_; > > That's more an open question, but according to your previous patch > > if (exposureModes[V4L2_EXPOSURE_AUTO]) > autoExposureMode_ = V4L2_EXPOSURE_AUTO; > else if (exposureModes[V4L2_EXPOSURE_APERTURE_PRIORITY]) > autoExposureMode_ = V4L2_EXPOSURE_APERTURE_PRIORITY; > > If the camera supported both V4L2_EXPOSURE_AUTO and > V4L2_EXPOSURE_APERTURE_PRIORITY autoExposureMode_ will always be > V4L2_EXPOSURE_AUTO meaning that we effectively make it impossible to > support APERTURE_PRIORITY by using auto exposure and manual gain. I based the order on the comment in the code: * ExposureTimeModeAuto = { V4L2_EXPOSURE_AUTO, * V4L2_EXPOSURE_APERTURE_PRIORITY } * * * ExposureTimeModeManual = { V4L2_EXPOSURE_MANUAL, * V4L2_EXPOSURE_SHUTTER_PRIORITY } > > I wonder if the above condition shouldn't be changed to > > if (exposureModes[V4L2_EXPOSURE_APERTURE_PRIORITY]) > autoExposureMode_ = V4L2_EXPOSURE_APERTURE_PRIORITY; > else if (exposureModes[V4L2_EXPOSURE_AUTO]) > autoExposureMode_ = V4L2_EXPOSURE_AUTO; > > so that if the camera supports _PRIORITY modes we use them to allow > implementing _PRIORITY modes in libcamera with ExposureTimeMode and > AnalogueGainMode. > > True that, if a user sets controls::ExposureTimeModeAuto and > controls::AnalogueGainModeAuto and here we set APERTURE_PRIORITY, > we're breaking it. > > Again, I'm not sure how many uvc cameras support PRIORITY modes, but > if we want to support the full handling of PRIORITY we should also > inspect AnalogueGainMode. I only have a single UVC camera that supports `V4L2_CID_EXPOSURE_AUTO`, and it implements `V4L2_EXPOSURE_MANUAL` and `V4L2_EXPOSURE_APERTURE_PRIORITY`. > > The pipeline at the moment doesn't even register AnalogueGainMode so I > guess we can don't care about PRIORITIES, so what you have here is > fine. > > If someone wants to actually support priority modes in uvc, then there > is a bigger rework to be done most probably. I wonder if we should > capture it in a comment at least (can be a separate patch) I will open a bug report about this if that's fine? If I understand it correctly: gain A M exposure A auto aperture M shutter manual This is how the two libcamera controls would be mapped to the v4l2 control. Will it not be an issue that you cannot express the supported feature set with ControlInfo exactly? E.g. if three are supported, or if two that are placed diagonally in the above table. I don't know if this is just theoretical, if such scenario is even possible/allowed. Regards, Barnabás Pőcze > >> + break; >> + case controls::ExposureTimeModeManual: >> + if (!data->manualExposureMode_) >> + return -EINVAL; >> + exposureMode = *data->manualExposureMode_; >> + break; >> + default: >> + return -EINVAL; >> + } >> + >> + controls->set(V4L2_CID_EXPOSURE_AUTO, exposureMode); > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > Thanks > j > >> break; >> } >> >> @@ -377,7 +391,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 On Tue, Mar 04, 2025 at 11:46:24AM +0100, Barnabás Pőcze wrote: > Hi > > > 2025. 03. 04. 11:09 keltezéssel, Jacopo Mondi írta: > > Hi Barnabás > > > > On Mon, Mar 03, 2025 at 02:42:34PM +0100, 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 V4L2 mode stored in `UVCCameraData` that was selected earlier in > > > `UVCCameraData::addControl()` for the given libcamera exposure time mode. > > > > > > Fixes: bad8d591f8acfa ("libcamera: uvcvideo: Register ExposureTimeMode control") > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > > --- > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 32 ++++++++++++++------ > > > 1 file changed, 23 insertions(+), 9 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > > index a6cc37366..dc3e85bd8 100644 > > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > > @@ -97,8 +97,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; > > > @@ -291,8 +291,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; > > > > > > @@ -336,10 +336,24 @@ 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); > > > + int32_t exposureMode; > > > + > > > + switch (value.get<int32_t>()) { > > > + case controls::ExposureTimeModeAuto: > > > + if (!data->autoExposureMode_) > > > + return -EINVAL; > > > + exposureMode = *data->autoExposureMode_; > > > > That's more an open question, but according to your previous patch > > > > if (exposureModes[V4L2_EXPOSURE_AUTO]) > > autoExposureMode_ = V4L2_EXPOSURE_AUTO; > > else if (exposureModes[V4L2_EXPOSURE_APERTURE_PRIORITY]) > > autoExposureMode_ = V4L2_EXPOSURE_APERTURE_PRIORITY; > > > > If the camera supported both V4L2_EXPOSURE_AUTO and > > V4L2_EXPOSURE_APERTURE_PRIORITY autoExposureMode_ will always be > > V4L2_EXPOSURE_AUTO meaning that we effectively make it impossible to > > support APERTURE_PRIORITY by using auto exposure and manual gain. > > I based the order on the comment in the code: > > * ExposureTimeModeAuto = { V4L2_EXPOSURE_AUTO, > * V4L2_EXPOSURE_APERTURE_PRIORITY } > * > * > * ExposureTimeModeManual = { V4L2_EXPOSURE_MANUAL, > * V4L2_EXPOSURE_SHUTTER_PRIORITY } > > > > > > I wonder if the above condition shouldn't be changed to > > > > if (exposureModes[V4L2_EXPOSURE_APERTURE_PRIORITY]) > > autoExposureMode_ = V4L2_EXPOSURE_APERTURE_PRIORITY; > > else if (exposureModes[V4L2_EXPOSURE_AUTO]) > > autoExposureMode_ = V4L2_EXPOSURE_AUTO; > > > > so that if the camera supports _PRIORITY modes we use them to allow > > implementing _PRIORITY modes in libcamera with ExposureTimeMode and > > AnalogueGainMode. > > > > True that, if a user sets controls::ExposureTimeModeAuto and > > controls::AnalogueGainModeAuto and here we set APERTURE_PRIORITY, > > we're breaking it. > > > > Again, I'm not sure how many uvc cameras support PRIORITY modes, but > > if we want to support the full handling of PRIORITY we should also > > inspect AnalogueGainMode. > > I only have a single UVC camera that supports `V4L2_CID_EXPOSURE_AUTO`, and > it implements `V4L2_EXPOSURE_MANUAL` and `V4L2_EXPOSURE_APERTURE_PRIORITY`. > Ah see, so it's not that uncommon > > > > > The pipeline at the moment doesn't even register AnalogueGainMode so I > > guess we can don't care about PRIORITIES, so what you have here is > > fine. > > > > If someone wants to actually support priority modes in uvc, then there > > is a bigger rework to be done most probably. I wonder if we should > > capture it in a comment at least (can be a separate patch) > > I will open a bug report about this if that's fine? Yes please, but also record in a comment that we don't support priority modes but only Auto/Manual modes for both exposure and gain > > If I understand it correctly: > > gain > A M > exposure A auto aperture > M shutter manual That's my understanding as well > > This is how the two libcamera controls would be mapped to the v4l2 control. > Will it not be an issue that you cannot express the supported feature set > with ControlInfo exactly? E.g. if three are supported, or if two that are > placed diagonally in the above table. I don't know if this is just theoretical, > if such scenario is even possible/allowed. aperture and shutter priorty modes should be implemented by registering AnalogueGainMode as well. Mode AnalogueGainMode ExposureTimeMode AUTO { Auto} { Auto } MANUAL { Manual } { Manual } APERTURE { Manual } { Auto } SHUTTER { Auto } { Manual } Users should inspect the control info for both controls, and in the case of your webcam that supports {AUTO, MANUAL, APERTURE } they should read AnalogueGainMode: { Auto, Manual } ExposureTimeMode: { Auto, Manual } Valid combinations will be AnalogueGainMode, ExposureTimeMode = { Auto, Auto } AnalogueGainMode, ExposureTimeMode = { Manual, Manual } AnalogueGainMode, ExposureTimeMode = { Manual, Auto } So yes, we can't express {Auto, Manual} (SHUTTER) is not valid until a user tries it... > > > Regards, > Barnabás Pőcze > > > > > > + break; > > > + case controls::ExposureTimeModeManual: > > > + if (!data->manualExposureMode_) > > > + return -EINVAL; > > > + exposureMode = *data->manualExposureMode_; > > > + break; > > > + default: > > > + return -EINVAL; > > > + } > > > + > > > + controls->set(V4L2_CID_EXPOSURE_AUTO, exposureMode); > > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > Thanks > > j > > > > > break; > > > } > > > > > > @@ -377,7 +391,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 a6cc37366..dc3e85bd8 100644 --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp @@ -97,8 +97,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; @@ -291,8 +291,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; @@ -336,10 +336,24 @@ 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); + int32_t exposureMode; + + switch (value.get<int32_t>()) { + case controls::ExposureTimeModeAuto: + if (!data->autoExposureMode_) + return -EINVAL; + exposureMode = *data->autoExposureMode_; + break; + case controls::ExposureTimeModeManual: + if (!data->manualExposureMode_) + return -EINVAL; + exposureMode = *data->manualExposureMode_; + break; + default: + return -EINVAL; + } + + controls->set(V4L2_CID_EXPOSURE_AUTO, exposureMode); break; } @@ -377,7 +391,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 V4L2 mode stored in `UVCCameraData` that was selected earlier in `UVCCameraData::addControl()` for the given libcamera exposure time mode. Fixes: bad8d591f8acfa ("libcamera: uvcvideo: Register ExposureTimeMode control") Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 32 ++++++++++++++------ 1 file changed, 23 insertions(+), 9 deletions(-)