[libcamera-devel,v3,09/14] libcamera: pipeline: uvcvideo: Add controls support

1551 diff mbox series

From: Kieran Bingham <kieran.bingham@ideasonboard.com>

Implement control support in the UVC pipeline handler by dynamically
querying the V4L2 device for the supported V4L2 controls and populating
the list of camera controls accordingly.

Not-signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/libcamera/pipeline/uvcvideo.cpp | 129 +++++++++++++++++++++++++---
 1 file changed, 115 insertions(+), 14 deletions(-)

Hi Kieran, Laurent,

On Mon, Jul 01, 2019 at 02:38:12AM +0300, Laurent Pinchart wrote:
> From: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> Implement control support in the UVC pipeline handler by dynamically
> querying the V4L2 device for the supported V4L2 controls and populating
> the list of camera controls accordingly.
>
> Not-signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Why so?

> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/libcamera/pipeline/uvcvideo.cpp | 129 +++++++++++++++++++++++++---
>  1 file changed, 115 insertions(+), 14 deletions(-)
>
> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> index 2e22523d7cb1..f68dc5bd6f74 100644
> --- a/src/libcamera/pipeline/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> @@ -6,8 +6,11 @@
>   */
>
>  #include <algorithm>
> +#include <iomanip>
> +#include <tuple>
>
>  #include <libcamera/camera.h>
> +#include <libcamera/controls.h>
>  #include <libcamera/request.h>
>  #include <libcamera/stream.h>
>
> @@ -16,6 +19,7 @@
>  #include "media_device.h"
>  #include "pipeline_handler.h"
>  #include "utils.h"
> +#include "v4l2_controls.h"
>  #include "v4l2_videodevice.h"
>
>  namespace libcamera {
> @@ -35,6 +39,7 @@ public:
>  		delete video_;
>  	}
>
> +	int init(MediaEntity *entity);
>  	void bufferReady(Buffer *buffer);
>
>  	V4L2VideoDevice *video_;
> @@ -71,6 +76,8 @@ public:
>  	bool match(DeviceEnumerator *enumerator) override;
>
>  private:
> +	int processControls(UVCCameraData *data, Request *request);
> +
>  	UVCCameraData *cameraData(const Camera *camera)
>  	{
>  		return static_cast<UVCCameraData *>(
> @@ -216,6 +223,54 @@ void PipelineHandlerUVC::stop(Camera *camera)
>  	PipelineHandler::stop(camera);
>  }
>
> +int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)
> +{
> +	V4L2ControlList controls;
> +
> +	for (auto it : request->controls()) {
> +		const ControlInfo *ci = it.first;
> +		ControlValue &value = it.second;

It would really be nice if we could move to a simple wrapper to
replace the typedef and be able to

        for (Control ctrl : request->controls()) {
                const ControlInfo *ci = ctrl->info();
                ControlValue &value = ctrl->value();
> +
> +		switch (ci->id()) {
> +		case Brightness:
> +			controls.add(V4L2_CID_BRIGHTNESS, value.getInt());
> +			break;
> +
> +		case Contrast:
> +			controls.add(V4L2_CID_CONTRAST, value.getInt());
> +			break;
> +
> +		case Saturation:
> +			controls.add(V4L2_CID_SATURATION, value.getInt());
> +			break;
> +
> +		case ManualExposure:
> +			controls.add(V4L2_CID_EXPOSURE_AUTO, 1);
> +			controls.add(V4L2_CID_EXPOSURE_ABSOLUTE, value.getInt());
> +			break;
> +
> +		case ManualGain:
> +			controls.add(V4L2_CID_GAIN, value.getInt());

or even be able to
                        ctrl.getInt()

> +			break;
> +
> +		default:
> +			break;
> +		}
> +	}

Will every pipeline handler have to do this translation by itself? I
guess so, as each platform would map a Libcamera control to possibly
different controls or parameters..

> +
> +	for (const V4L2Control &ctrl : controls)
> +		LOG(UVC, Debug)
> +			<< "Setting control 0x"
> +			<< std::hex << std::setw(8) << ctrl.id() << std::dec
> +			<< " to " << ctrl.value();
> +
> +	int ret = data->video_->setControls(&controls);
> +	if (ret)
> +		LOG(UVC, Error) << "Failed to set controls";

Print out the return value as it would tell which control has failed

> +
> +	return ret;
> +}
> +
>  int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)
>  {
>  	UVCCameraData *data = cameraData(camera);
> @@ -227,7 +282,11 @@ int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)
>  		return -ENOENT;
>  	}
>
> -	int ret = data->video_->queueBuffer(buffer);
> +	int ret = processControls(data, request);
> +	if (ret < 0)

Not just < 0, as V4L2Device::setControls() can return a positive
integer

> +		return ret;
> +
> +	ret = data->video_->queueBuffer(buffer);
>  	if (ret < 0)
>  		return ret;
>
> @@ -247,24 +306,14 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
>
>  	std::unique_ptr<UVCCameraData> data = utils::make_unique<UVCCameraData>(this);
>
> -	/* Locate and open the default video node. */
> +	/* Locate and initialise the camera data with the default video node. */
>  	for (MediaEntity *entity : media->entities()) {
>  		if (entity->flags() & MEDIA_ENT_FL_DEFAULT) {
> -			data->video_ = new V4L2VideoDevice(entity);
> -			break;
> +			if (data->init(entity))
> +				return false;

You can break the loop here I guess

>  		}
>  	}
>
> -	if (!data->video_) {
> -		LOG(UVC, Error) << "Could not find a default video device";
> -		return false;
> -	}
> -
> -	if (data->video_->open())
> -		return false;
> -
> -	data->video_->bufferReady.connect(data.get(), &UVCCameraData::bufferReady);
> -
>  	/* Create and register the camera. */
>  	std::set<Stream *> streams{ &data->stream_ };
>  	std::shared_ptr<Camera> camera = Camera::create(this, media->model(), streams);
> @@ -276,6 +325,58 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
>  	return true;
>  }
>
> +int UVCCameraData::init(MediaEntity *entity)
> +{
> +	int ret;
> +
> +	/* Create and open the video device. */
> +	video_ = new V4L2VideoDevice(entity);
> +	if (!video_) {
> +		LOG(UVC, Error) << "Could not find a default video device";

Update the error message

> +		return -ENODEV;
> +	}
> +
> +	ret = video_->open();
> +	if (ret)
> +		return ret;
> +
> +	video_->bufferReady.connect(this, &UVCCameraData::bufferReady);
> +
> +	/* Initialise the supported controls. */
> +	const V4L2ControlInfoMap &controls = video_->controls();
> +	for (const auto &ctrl : controls) {
> +		unsigned int v4l2Id = ctrl.first;
> +		const V4L2ControlInfo &info = ctrl.second;
> +		ControlId id;
> +
> +		switch (v4l2Id) {
> +		case V4L2_CID_BRIGHTNESS:
> +			id = Brightness;
> +			break;
> +		case V4L2_CID_CONTRAST:
> +			id = Contrast;
> +			break;
> +		case V4L2_CID_SATURATION:
> +			id = Saturation;
> +			break;
> +		case V4L2_CID_EXPOSURE_ABSOLUTE:
> +			id = ManualExposure;
> +			break;
> +		case V4L2_CID_GAIN:
> +			id = ManualGain;
> +			break;
> +		default:
> +			continue;
> +		}
> +
> +		controlInfo_.emplace(std::piecewise_construct,
> +				     std::forward_as_tuple(id),
> +				     std::forward_as_tuple(id, info.min(), info.max()));

Awesome! Thanks for clarifying offline.

Minors apart, I'm happy with the direction this is taking, if not for
the fact we have to access maps with the horrible first and second
accessors for both libcamera controls and v4l2 controls. A way to
abstract that away would make this very nice.

Thanks
   j

> +	}
> +
> +	return 0;
> +}
> +
>  void UVCCameraData::bufferReady(Buffer *buffer)
>  {
>  	Request *request = queuedRequests_.front();
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Jacopo,

On Mon, Jul 01, 2019 at 07:03:53PM +0200, Jacopo Mondi wrote:
> On Mon, Jul 01, 2019 at 02:38:12AM +0300, Laurent Pinchart wrote:
> > From: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >
> > Implement control support in the UVC pipeline handler by dynamically
> > querying the V4L2 device for the supported V4L2 controls and populating
> > the list of camera controls accordingly.
> >
> > Not-signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> Why so?

I believe because the previous version was work in progress.

> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  src/libcamera/pipeline/uvcvideo.cpp | 129 +++++++++++++++++++++++++---
> >  1 file changed, 115 insertions(+), 14 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> > index 2e22523d7cb1..f68dc5bd6f74 100644
> > --- a/src/libcamera/pipeline/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo.cpp
> > @@ -6,8 +6,11 @@
> >   */
> >
> >  #include <algorithm>
> > +#include <iomanip>
> > +#include <tuple>
> >
> >  #include <libcamera/camera.h>
> > +#include <libcamera/controls.h>
> >  #include <libcamera/request.h>
> >  #include <libcamera/stream.h>
> >
> > @@ -16,6 +19,7 @@
> >  #include "media_device.h"
> >  #include "pipeline_handler.h"
> >  #include "utils.h"
> > +#include "v4l2_controls.h"
> >  #include "v4l2_videodevice.h"
> >
> >  namespace libcamera {
> > @@ -35,6 +39,7 @@ public:
> >  		delete video_;
> >  	}
> >
> > +	int init(MediaEntity *entity);
> >  	void bufferReady(Buffer *buffer);
> >
> >  	V4L2VideoDevice *video_;
> > @@ -71,6 +76,8 @@ public:
> >  	bool match(DeviceEnumerator *enumerator) override;
> >
> >  private:
> > +	int processControls(UVCCameraData *data, Request *request);
> > +
> >  	UVCCameraData *cameraData(const Camera *camera)
> >  	{
> >  		return static_cast<UVCCameraData *>(
> > @@ -216,6 +223,54 @@ void PipelineHandlerUVC::stop(Camera *camera)
> >  	PipelineHandler::stop(camera);
> >  }
> >
> > +int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)
> > +{
> > +	V4L2ControlList controls;
> > +
> > +	for (auto it : request->controls()) {
> > +		const ControlInfo *ci = it.first;
> > +		ControlValue &value = it.second;
> 
> It would really be nice if we could move to a simple wrapper to
> replace the typedef and be able to
> 
>         for (Control ctrl : request->controls()) {
>                 const ControlInfo *ci = ctrl->info();
>                 ControlValue &value = ctrl->value();

We could create a custom iterator whose first and second members would
be named info and value. Wanna give it a try ? :-)

> > +
> > +		switch (ci->id()) {
> > +		case Brightness:
> > +			controls.add(V4L2_CID_BRIGHTNESS, value.getInt());
> > +			break;
> > +
> > +		case Contrast:
> > +			controls.add(V4L2_CID_CONTRAST, value.getInt());
> > +			break;
> > +
> > +		case Saturation:
> > +			controls.add(V4L2_CID_SATURATION, value.getInt());
> > +			break;
> > +
> > +		case ManualExposure:
> > +			controls.add(V4L2_CID_EXPOSURE_AUTO, 1);
> > +			controls.add(V4L2_CID_EXPOSURE_ABSOLUTE, value.getInt());
> > +			break;
> > +
> > +		case ManualGain:
> > +			controls.add(V4L2_CID_GAIN, value.getInt());
> 
> or even be able to
>                         ctrl.getInt()
> 
> > +			break;
> > +
> > +		default:
> > +			break;
> > +		}
> > +	}
> 
> Will every pipeline handler have to do this translation by itself? I
> guess so, as each platform would map a Libcamera control to possibly
> different controls or parameters..

Yes and no. The IPU3 and RkISP1 pipeline handlers won't use V4L2 for
controls. For UVC and VIMC there's indeed some overlap, and I think
helpers would make sense, but it's a bit early to define an API for
these helpers. I think we need to play a bit with controls first.

> > +
> > +	for (const V4L2Control &ctrl : controls)
> > +		LOG(UVC, Debug)
> > +			<< "Setting control 0x"
> > +			<< std::hex << std::setw(8) << ctrl.id() << std::dec
> > +			<< " to " << ctrl.value();
> > +
> > +	int ret = data->video_->setControls(&controls);
> > +	if (ret)
> > +		LOG(UVC, Error) << "Failed to set controls";
> 
> Print out the return value as it would tell which control has failed
> 
> > +
> > +	return ret;
> > +}
> > +
> >  int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)
> >  {
> >  	UVCCameraData *data = cameraData(camera);
> > @@ -227,7 +282,11 @@ int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)
> >  		return -ENOENT;
> >  	}
> >
> > -	int ret = data->video_->queueBuffer(buffer);
> > +	int ret = processControls(data, request);
> > +	if (ret < 0)
> 
> Not just < 0, as V4L2Device::setControls() can return a positive
> integer

I will instead return -EINVAL in processControls() if ret > 0, as we
don't want to propagate positive values up.

> > +		return ret;
> > +
> > +	ret = data->video_->queueBuffer(buffer);
> >  	if (ret < 0)
> >  		return ret;
> >
> > @@ -247,24 +306,14 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
> >
> >  	std::unique_ptr<UVCCameraData> data = utils::make_unique<UVCCameraData>(this);
> >
> > -	/* Locate and open the default video node. */
> > +	/* Locate and initialise the camera data with the default video node. */
> >  	for (MediaEntity *entity : media->entities()) {
> >  		if (entity->flags() & MEDIA_ENT_FL_DEFAULT) {
> > -			data->video_ = new V4L2VideoDevice(entity);
> > -			break;
> > +			if (data->init(entity))
> > +				return false;
> 
> You can break the loop here I guess
> 
> >  		}
> >  	}
> >
> > -	if (!data->video_) {
> > -		LOG(UVC, Error) << "Could not find a default video device";
> > -		return false;
> > -	}
> > -
> > -	if (data->video_->open())
> > -		return false;
> > -
> > -	data->video_->bufferReady.connect(data.get(), &UVCCameraData::bufferReady);
> > -
> >  	/* Create and register the camera. */
> >  	std::set<Stream *> streams{ &data->stream_ };
> >  	std::shared_ptr<Camera> camera = Camera::create(this, media->model(), streams);
> > @@ -276,6 +325,58 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
> >  	return true;
> >  }
> >
> > +int UVCCameraData::init(MediaEntity *entity)
> > +{
> > +	int ret;
> > +
> > +	/* Create and open the video device. */
> > +	video_ = new V4L2VideoDevice(entity);
> > +	if (!video_) {
> > +		LOG(UVC, Error) << "Could not find a default video device";
> 
> Update the error message

More than that, the above function needs this error check after the
loop.

        if (!data) {
                LOG(UVC, Error) << "Could not find a default video device";
                return -ENODEV;
        }

> > +		return -ENODEV;
> > +	}
> > +
> > +	ret = video_->open();
> > +	if (ret)
> > +		return ret;
> > +
> > +	video_->bufferReady.connect(this, &UVCCameraData::bufferReady);
> > +
> > +	/* Initialise the supported controls. */
> > +	const V4L2ControlInfoMap &controls = video_->controls();
> > +	for (const auto &ctrl : controls) {
> > +		unsigned int v4l2Id = ctrl.first;
> > +		const V4L2ControlInfo &info = ctrl.second;
> > +		ControlId id;
> > +
> > +		switch (v4l2Id) {
> > +		case V4L2_CID_BRIGHTNESS:
> > +			id = Brightness;
> > +			break;
> > +		case V4L2_CID_CONTRAST:
> > +			id = Contrast;
> > +			break;
> > +		case V4L2_CID_SATURATION:
> > +			id = Saturation;
> > +			break;
> > +		case V4L2_CID_EXPOSURE_ABSOLUTE:
> > +			id = ManualExposure;
> > +			break;
> > +		case V4L2_CID_GAIN:
> > +			id = ManualGain;
> > +			break;
> > +		default:
> > +			continue;
> > +		}
> > +
> > +		controlInfo_.emplace(std::piecewise_construct,
> > +				     std::forward_as_tuple(id),
> > +				     std::forward_as_tuple(id, info.min(), info.max()));
> 
> Awesome! Thanks for clarifying offline.
> 
> Minors apart, I'm happy with the direction this is taking, if not for
> the fact we have to access maps with the horrible first and second
> accessors for both libcamera controls and v4l2 controls. A way to
> abstract that away would make this very nice.
> 
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  void UVCCameraData::bufferReady(Buffer *buffer)
> >  {
> >  	Request *request = queuedRequests_.front();