| Message ID | 20230725125641.1557350-3-kieran.bingham@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran Thanks for the patch. On Tue, 25 Jul 2023 at 13:56, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > UVC devices often provide controls to support setting the power line > frequency which adapts the camera processing to avoid flicker visible to > the user. > > Now that libcamera has implemented AeFlickerMode, use the new control to > implement support for UVC devices. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 59 ++++++++++++++++++-- > 1 file changed, 55 insertions(+), 4 deletions(-) > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > index 38f48a5d9269..365410284e5b 100644 > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > @@ -89,8 +89,8 @@ public: > bool match(DeviceEnumerator *enumerator) override; > > private: > - int processControl(ControlList *controls, unsigned int id, > - const ControlValue &value); > + int processControl(Request *request, ControlList *controls, > + unsigned int id, const ControlValue &value); > int processControls(UVCCameraData *data, Request *request); > > UVCCameraData *cameraData(Camera *camera) > @@ -260,7 +260,8 @@ void PipelineHandlerUVC::stopDevice(Camera *camera) > data->video_->releaseBuffers(); > } > > -int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id, > +int PipelineHandlerUVC::processControl(Request *request, ControlList *controls, > + unsigned int id, > const ControlValue &value) > { > uint32_t cid; > @@ -277,6 +278,8 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id, > cid = V4L2_CID_EXPOSURE_ABSOLUTE; > else if (id == controls::AnalogueGain) > cid = V4L2_CID_GAIN; > + else if (id == controls::AeFlickerMode) > + cid = V4L2_CID_POWER_LINE_FREQUENCY; > else > return -EINVAL; > > @@ -331,6 +334,42 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id, > break; > } > > + case V4L2_CID_POWER_LINE_FREQUENCY: { > + enum v4l2_power_line_frequency mode; > + unsigned int period = request->controls().get(controls::AeFlickerPeriod) > + .value_or(0); > + switch (value.get<int32_t>()) { > + default: > + case controls::FlickerOff: > + mode = V4L2_CID_POWER_LINE_FREQUENCY_DISABLED; > + break; > + case controls::FlickerAuto: > + mode = V4L2_CID_POWER_LINE_FREQUENCY_AUTO; > + break; > + case controls::FlickerManual: > + switch (period) { > + case 10000: /* 100Hz in microseconds */ > + mode = V4L2_CID_POWER_LINE_FREQUENCY_50HZ; > + break; > + case 8333: /* 120Hz in microseconds */ I suppose we're happy with the exact comparisons here? So if an application calculated the frequency, and it wasn't exactly 100 or 120Hz, then that number would have to be tidied up before being passed in. I think that seems OK. > + mode = V4L2_CID_POWER_LINE_FREQUENCY_60HZ; > + break; > + default: > + LOG(UVC, Warning) > + << "Unsupported AeFlickerPeriod : " > + << period; > + [[fallthrough]]; > + case 0: > + mode = V4L2_CID_POWER_LINE_FREQUENCY_DISABLED; > + break; > + } > + break; > + } > + > + controls->set(cid, static_cast<int32_t>(mode)); > + break; > + } > + > default: { > int32_t ivalue = value.get<int32_t>(); > controls->set(cid, ivalue); > @@ -346,7 +385,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(request, &controls, id, value); > > for (const auto &ctrl : controls) > LOG(UVC, Debug) > @@ -605,6 +644,9 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info, > case V4L2_CID_GAIN: > id = &controls::AnalogueGain; > break; > + case V4L2_CID_POWER_LINE_FREQUENCY: > + id = &controls::AeFlickerMode; > + break; > default: > return; > } > @@ -689,6 +731,15 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info, > break; > } > > + case V4L2_CID_POWER_LINE_FREQUENCY: { > + info = ControlInfo{ > + { static_cast<int>(controls::FlickerOff) }, > + { static_cast<int>(controls::FlickerAuto) }, > + { static_cast<int>(controls::FlickerOff) } I assume this is setting the default value of "off"? There was some discussion which resulted in the suggestion that the default should be "auto" if available, otherwise "off". Do we know if a UVC device even supports "auto"? It doesn't particularly bother me, but we should probably just ensure that folks are happy with this. So from my point of view: Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Thanks! David > + }; > + break; > + } > + > default: > info = v4l2Info; > break; > -- > 2.34.1 >
On Tue, Jul 25, 2023 at 02:10:16PM +0100, 📷-dev wrote: > On Tue, 25 Jul 2023 at 13:56, Kieran Bingham wrote: > > > > UVC devices often provide controls to support setting the power line > > frequency which adapts the camera processing to avoid flicker visible to > > the user. > > > > Now that libcamera has implemented AeFlickerMode, use the new control to > > implement support for UVC devices. > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 59 ++++++++++++++++++-- > > 1 file changed, 55 insertions(+), 4 deletions(-) > > > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > index 38f48a5d9269..365410284e5b 100644 > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > @@ -89,8 +89,8 @@ public: > > bool match(DeviceEnumerator *enumerator) override; > > > > private: > > - int processControl(ControlList *controls, unsigned int id, > > - const ControlValue &value); > > + int processControl(Request *request, ControlList *controls, > > + unsigned int id, const ControlValue &value); > > int processControls(UVCCameraData *data, Request *request); > > > > UVCCameraData *cameraData(Camera *camera) > > @@ -260,7 +260,8 @@ void PipelineHandlerUVC::stopDevice(Camera *camera) > > data->video_->releaseBuffers(); > > } > > > > -int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id, > > +int PipelineHandlerUVC::processControl(Request *request, ControlList *controls, > > + unsigned int id, > > const ControlValue &value) > > { > > uint32_t cid; > > @@ -277,6 +278,8 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id, > > cid = V4L2_CID_EXPOSURE_ABSOLUTE; > > else if (id == controls::AnalogueGain) > > cid = V4L2_CID_GAIN; > > + else if (id == controls::AeFlickerMode) > > + cid = V4L2_CID_POWER_LINE_FREQUENCY; > > else > > return -EINVAL; > > > > @@ -331,6 +334,42 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id, > > break; > > } > > > > + case V4L2_CID_POWER_LINE_FREQUENCY: { > > + enum v4l2_power_line_frequency mode; > > + unsigned int period = request->controls().get(controls::AeFlickerPeriod) > > + .value_or(0); If the application modifies AeFlickerPeriod when in manualy mode, but without setting the AeFlickerMode in the same request, the frequency won't be updated. Similarly, if the application set AeFlickerMode to manual without setting the period in the same request, you won't get the right behaviour. > > + switch (value.get<int32_t>()) { > > + default: > > + case controls::FlickerOff: > > + mode = V4L2_CID_POWER_LINE_FREQUENCY_DISABLED; > > + break; > > + case controls::FlickerAuto: > > + mode = V4L2_CID_POWER_LINE_FREQUENCY_AUTO; > > + break; > > + case controls::FlickerManual: > > + switch (period) { > > + case 10000: /* 100Hz in microseconds */ > > + mode = V4L2_CID_POWER_LINE_FREQUENCY_50HZ; > > + break; > > + case 8333: /* 120Hz in microseconds */ > > I suppose we're happy with the exact comparisons here? So if an > application calculated the frequency, and it wasn't exactly 100 or > 120Hz, then that number would have to be tidied up before being passed > in. I think that seems OK. > > > + mode = V4L2_CID_POWER_LINE_FREQUENCY_60HZ; > > + break; > > + default: > > + LOG(UVC, Warning) > > + << "Unsupported AeFlickerPeriod : " > > + << period; > > + [[fallthrough]]; > > + case 0: > > + mode = V4L2_CID_POWER_LINE_FREQUENCY_DISABLED; > > + break; > > + } > > + break; > > + } > > + > > + controls->set(cid, static_cast<int32_t>(mode)); > > + break; > > + } > > + > > default: { > > int32_t ivalue = value.get<int32_t>(); > > controls->set(cid, ivalue); > > @@ -346,7 +385,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(request, &controls, id, value); > > > > for (const auto &ctrl : controls) > > LOG(UVC, Debug) > > @@ -605,6 +644,9 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info, > > case V4L2_CID_GAIN: > > id = &controls::AnalogueGain; > > break; > > + case V4L2_CID_POWER_LINE_FREQUENCY: > > + id = &controls::AeFlickerMode; > > + break; > > default: > > return; > > } > > @@ -689,6 +731,15 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info, > > break; > > } > > > > + case V4L2_CID_POWER_LINE_FREQUENCY: { > > + info = ControlInfo{ > > + { static_cast<int>(controls::FlickerOff) }, > > + { static_cast<int>(controls::FlickerAuto) }, > > + { static_cast<int>(controls::FlickerOff) } > > I assume this is setting the default value of "off"? There was some > discussion which resulted in the suggestion that the default should be > "auto" if available, otherwise "off". Do we know if a UVC device even > supports "auto"? The V4L2 control should report the supported values, so we can default to auto indeed. This needs to be taken into account for the max value too. Note that some UVC devices don't support the "off" mode and only support 50Hz and 60Hz. This should also be taken into account, both here to report the limits, and in processControl() to adjust (or drop) unsupported values. > It doesn't particularly bother me, but we should probably just ensure > that folks are happy with this. So from my point of view: > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > > + }; > > + break; > > + } > > + > > default: > > info = v4l2Info;
diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp index 38f48a5d9269..365410284e5b 100644 --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp @@ -89,8 +89,8 @@ public: bool match(DeviceEnumerator *enumerator) override; private: - int processControl(ControlList *controls, unsigned int id, - const ControlValue &value); + int processControl(Request *request, ControlList *controls, + unsigned int id, const ControlValue &value); int processControls(UVCCameraData *data, Request *request); UVCCameraData *cameraData(Camera *camera) @@ -260,7 +260,8 @@ void PipelineHandlerUVC::stopDevice(Camera *camera) data->video_->releaseBuffers(); } -int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id, +int PipelineHandlerUVC::processControl(Request *request, ControlList *controls, + unsigned int id, const ControlValue &value) { uint32_t cid; @@ -277,6 +278,8 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id, cid = V4L2_CID_EXPOSURE_ABSOLUTE; else if (id == controls::AnalogueGain) cid = V4L2_CID_GAIN; + else if (id == controls::AeFlickerMode) + cid = V4L2_CID_POWER_LINE_FREQUENCY; else return -EINVAL; @@ -331,6 +334,42 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id, break; } + case V4L2_CID_POWER_LINE_FREQUENCY: { + enum v4l2_power_line_frequency mode; + unsigned int period = request->controls().get(controls::AeFlickerPeriod) + .value_or(0); + switch (value.get<int32_t>()) { + default: + case controls::FlickerOff: + mode = V4L2_CID_POWER_LINE_FREQUENCY_DISABLED; + break; + case controls::FlickerAuto: + mode = V4L2_CID_POWER_LINE_FREQUENCY_AUTO; + break; + case controls::FlickerManual: + switch (period) { + case 10000: /* 100Hz in microseconds */ + mode = V4L2_CID_POWER_LINE_FREQUENCY_50HZ; + break; + case 8333: /* 120Hz in microseconds */ + mode = V4L2_CID_POWER_LINE_FREQUENCY_60HZ; + break; + default: + LOG(UVC, Warning) + << "Unsupported AeFlickerPeriod : " + << period; + [[fallthrough]]; + case 0: + mode = V4L2_CID_POWER_LINE_FREQUENCY_DISABLED; + break; + } + break; + } + + controls->set(cid, static_cast<int32_t>(mode)); + break; + } + default: { int32_t ivalue = value.get<int32_t>(); controls->set(cid, ivalue); @@ -346,7 +385,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(request, &controls, id, value); for (const auto &ctrl : controls) LOG(UVC, Debug) @@ -605,6 +644,9 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info, case V4L2_CID_GAIN: id = &controls::AnalogueGain; break; + case V4L2_CID_POWER_LINE_FREQUENCY: + id = &controls::AeFlickerMode; + break; default: return; } @@ -689,6 +731,15 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info, break; } + case V4L2_CID_POWER_LINE_FREQUENCY: { + info = ControlInfo{ + { static_cast<int>(controls::FlickerOff) }, + { static_cast<int>(controls::FlickerAuto) }, + { static_cast<int>(controls::FlickerOff) } + }; + break; + } + default: info = v4l2Info; break;
UVC devices often provide controls to support setting the power line frequency which adapts the camera processing to avoid flicker visible to the user. Now that libcamera has implemented AeFlickerMode, use the new control to implement support for UVC devices. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 59 ++++++++++++++++++-- 1 file changed, 55 insertions(+), 4 deletions(-)