[libcamera-devel] [PATCH v5 1/7] libcamera: pipeline: uvcvideo: Refactor control handling

[Jacopo Mondi]

Hi Laurent,

On Sat, Apr 25, 2020 at 03:45:27AM +0300, Laurent Pinchart wrote:
> Move addition and processing of individual controls to separate
> functions, to prepare for more complex mappings of control values.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 103 ++++++++++++-------
>  1 file changed, 67 insertions(+), 36 deletions(-)
>
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index ffbddf27ae2f..92777b9f5fe4 100644
> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> @@ -42,6 +42,8 @@ public:
>  	}
>
>  	int init(MediaEntity *entity);
> +	void addControl(uint32_t cid, const ControlInfo &v4l2info,
> +			ControlInfoMap::Map *ctrls);
>  	void bufferReady(FrameBuffer *buffer);
>
>  	V4L2VideoDevice *video_;
> @@ -76,6 +78,8 @@ public:
>  	bool match(DeviceEnumerator *enumerator) override;
>
>  private:
> +	int processControl(ControlList *controls, unsigned int id,
> +			   const ControlValue &value);
>  	int processControls(UVCCameraData *data, Request *request);
>
>  	UVCCameraData *cameraData(const Camera *camera)
> @@ -237,6 +241,37 @@ void PipelineHandlerUVC::stop(Camera *camera)
>  	data->video_->releaseBuffers();
>  }
>
> +int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
> +				       const ControlValue &value)
> +{
> +	if (value.type() != ControlTypeInteger32)
> +		return -EINVAL;

Is this needed ? All supported controls are int32, you will fail later
anyway if the control id is not supported, and this check will have to
be removed once (and if) we support non int32_t controls

> +
> +	uint32_t cid;
> +
> +	if (id == controls::Brightness)
> +		cid = V4L2_CID_BRIGHTNESS;
> +	else if (id == controls::Contrast)
> +		cid = V4L2_CID_CONTRAST;
> +	else if (id == controls::Saturation)
> +		cid = V4L2_CID_SATURATION;
> +	else if (id == controls::ManualExposure)
> +		cid = V4L2_CID_EXPOSURE_ABSOLUTE;
> +	else if (id == controls::ManualGain)
> +		cid = V4L2_CID_GAIN;
> +	else
> +		return -EINVAL;
> +
> +	int32_t ivalue = value.get<int32_t>();

could you move this one after the next if condition ?

> +
> +	if (cid == V4L2_CID_EXPOSURE_ABSOLUTE)
> +		controls->set(V4L2_CID_EXPOSURE_AUTO, static_cast<int32_t>(1));
> +
> +	controls->set(cid, ivalue);
> +
> +	return 0;
> +}
> +
>  int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)
>  {
>  	ControlList controls(data->video_->controls());
> @@ -245,18 +280,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)
>  		unsigned int id = it.first;
>  		ControlValue &value = it.second;
>
> -		if (id == controls::Brightness) {
> -			controls.set(V4L2_CID_BRIGHTNESS, value);
> -		} else if (id == controls::Contrast) {
> -			controls.set(V4L2_CID_CONTRAST, value);
> -		} else if (id == controls::Saturation) {
> -			controls.set(V4L2_CID_SATURATION, value);
> -		} else if (id == controls::ManualExposure) {
> -			controls.set(V4L2_CID_EXPOSURE_AUTO, static_cast<int32_t>(1));
> -			controls.set(V4L2_CID_EXPOSURE_ABSOLUTE, value);
> -		} else if (id == controls::ManualGain) {
> -			controls.set(V4L2_CID_GAIN, value);
> -		}
> +		processControl(&controls, id, value);
>  	}
>
>  	for (const auto &ctrl : controls)
> @@ -346,34 +370,13 @@ int UVCCameraData::init(MediaEntity *entity)
>  	video_->bufferReady.connect(this, &UVCCameraData::bufferReady);
>
>  	/* Initialise the supported controls. */
> -	const ControlInfoMap &controls = video_->controls();
>  	ControlInfoMap::Map ctrls;
>
> -	for (const auto &ctrl : controls) {
> +	for (const auto &ctrl : video_->controls()) {
> +		uint32_t cid = ctrl.first->id();
>  		const ControlInfo &info = ctrl.second;
> -		const ControlId *id;
>
> -		switch (ctrl.first->id()) {
> -		case V4L2_CID_BRIGHTNESS:
> -			id = &controls::Brightness;
> -			break;
> -		case V4L2_CID_CONTRAST:
> -			id = &controls::Contrast;
> -			break;
> -		case V4L2_CID_SATURATION:
> -			id = &controls::Saturation;
> -			break;
> -		case V4L2_CID_EXPOSURE_ABSOLUTE:
> -			id = &controls::ManualExposure;
> -			break;
> -		case V4L2_CID_GAIN:
> -			id = &controls::ManualGain;
> -			break;
> -		default:
> -			continue;
> -		}
> -
> -		ctrls.emplace(id, info);
> +		addControl(cid, info, &ctrls);
>  	}
>
>  	controlInfo_ = std::move(ctrls);
> @@ -381,6 +384,34 @@ int UVCCameraData::init(MediaEntity *entity)
>  	return 0;
>  }
>
> +void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
> +			       ControlInfoMap::Map *ctrls)
> +{
> +	const ControlId *id;
> +
> +	switch (cid) {
> +	case V4L2_CID_BRIGHTNESS:
> +		id = &controls::Brightness;
> +		break;
> +	case V4L2_CID_CONTRAST:
> +		id = &controls::Contrast;
> +		break;
> +	case V4L2_CID_SATURATION:
> +		id = &controls::Saturation;
> +		break;
> +	case V4L2_CID_EXPOSURE_ABSOLUTE:
> +		id = &controls::ManualExposure;
> +		break;
> +	case V4L2_CID_GAIN:
> +		id = &controls::ManualGain;

I'm a bit concerned all pipeline handlers will have to implement this
libcamera-control<->v4l2-control translation, but that's not on this
patch

Anyway
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
  j

> +		break;
> +	default:
> +		return;
> +	}
> +
> +	ctrls->emplace(id, v4l2Info);
> +}
> +
>  void UVCCameraData::bufferReady(FrameBuffer *buffer)
>  {
>  	Request *request = buffer->request();
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel