[libcamera-devel,v4,08/13] libcamera: pipeline: uvcvideo: Add controls support

1567 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>
---
Changes since v3:

- Fixed error checking when setting controls
- Fixed handling of the failure to find a default video device
---
 src/libcamera/pipeline/uvcvideo.cpp | 124 +++++++++++++++++++++++++---
 1 file changed, 114 insertions(+), 10 deletions(-)

Hi Kieran, Laurent

Thanks for your work.

On 2019-07-01 23:14:59 +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>

Maybe drop the Not- before merging ;-)

> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v3:
> 
> - Fixed error checking when setting controls
> - Fixed handling of the failure to find a default video device
> ---
>  src/libcamera/pipeline/uvcvideo.cpp | 124 +++++++++++++++++++++++++---
>  1 file changed, 114 insertions(+), 10 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> index 2e22523d7cb1..b2f5b2eeed80 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,56 @@ 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;
> +
> +		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());
> +			break;
> +
> +		default:
> +			break;
> +		}
> +	}
> +
> +	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: " << ret;
> +		return ret < 0 ? ret : -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
>  int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)
>  {
>  	UVCCameraData *data = cameraData(camera);
> @@ -227,7 +284,11 @@ int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)
>  		return -ENOENT;
>  	}
>  
> -	int ret = data->video_->queueBuffer(buffer);
> +	int ret = processControls(data, request);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = data->video_->queueBuffer(buffer);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -247,24 +308,20 @@ 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);
> +			if (data->init(entity))
> +				return false;
>  			break;
>  		}
>  	}
>  
> -	if (!data->video_) {
> +	if (!data) {
>  		LOG(UVC, Error) << "Could not find a default video device";
> -		return false;
> +		return -ENODEV;

This function returns a bool so returning -ENODEV would be interpret as 
a successful operation by the caller.

>  	}
>  
> -	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 +333,53 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
>  	return true;
>  }
>  
> +int UVCCameraData::init(MediaEntity *entity)
> +{
> +	int ret;
> +
> +	/* Create and open the video device. */
> +	video_ = new V4L2VideoDevice(entity);
> +	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()));

I love C++, so easy to parse ;-)

With the small issues fixed,

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> +	}
> +
> +	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 Laurent,
On Mon, Jul 01, 2019 at 11:14:59PM +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>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v3:
>
> - Fixed error checking when setting controls
> - Fixed handling of the failure to find a default video device

Thanks
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

> ---
>  src/libcamera/pipeline/uvcvideo.cpp | 124 +++++++++++++++++++++++++---
>  1 file changed, 114 insertions(+), 10 deletions(-)
>
> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> index 2e22523d7cb1..b2f5b2eeed80 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,56 @@ 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;
> +
> +		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());
> +			break;
> +
> +		default:
> +			break;
> +		}
> +	}
> +
> +	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: " << ret;
> +		return ret < 0 ? ret : -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
>  int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)
>  {
>  	UVCCameraData *data = cameraData(camera);
> @@ -227,7 +284,11 @@ int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)
>  		return -ENOENT;
>  	}
>
> -	int ret = data->video_->queueBuffer(buffer);
> +	int ret = processControls(data, request);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = data->video_->queueBuffer(buffer);
>  	if (ret < 0)
>  		return ret;
>
> @@ -247,24 +308,20 @@ 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);
> +			if (data->init(entity))
> +				return false;
>  			break;
>  		}
>  	}
>
> -	if (!data->video_) {
> +	if (!data) {
>  		LOG(UVC, Error) << "Could not find a default video device";
> -		return false;
> +		return -ENODEV;
>  	}
>
> -	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 +333,53 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
>  	return true;
>  }
>
> +int UVCCameraData::init(MediaEntity *entity)
> +{
> +	int ret;
> +
> +	/* Create and open the video device. */
> +	video_ = new V4L2VideoDevice(entity);
> +	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()));
> +	}
> +
> +	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 Laurent,

On 01/07/2019 21:14, 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>

With the bool issue Niklas spotted repaired, I think this patch has come
a long way forwards since the [PoC], and my SoB can have s/Not-s/S/

We are going to duplicate the conversion between libcamera control
values and v4l2 controls on controls where there is a direct 1-1
mapping. For that I think we'll need some internal helpers, but lets get
the code to grow organically first so we know what we need.

This means we'll have duplication here and at VIMC at least initially,
and possibly in other pipeline handlers. The Out-of-tree RPi will also
need the same conversions currently. But that's fine for the moment.

Also, if relevant..

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v3:
> 
> - Fixed error checking when setting controls
> - Fixed handling of the failure to find a default video device
> ---
>  src/libcamera/pipeline/uvcvideo.cpp | 124 +++++++++++++++++++++++++---
>  1 file changed, 114 insertions(+), 10 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> index 2e22523d7cb1..b2f5b2eeed80 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,56 @@ 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;
> +
> +		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());
> +			break;
> +
> +		default:
> +			break;
> +		}
> +	}
> +
> +	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: " << ret;
> +		return ret < 0 ? ret : -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
>  int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)
>  {
>  	UVCCameraData *data = cameraData(camera);
> @@ -227,7 +284,11 @@ int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)
>  		return -ENOENT;
>  	}
>  
> -	int ret = data->video_->queueBuffer(buffer);
> +	int ret = processControls(data, request);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = data->video_->queueBuffer(buffer);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -247,24 +308,20 @@ 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);
> +			if (data->init(entity))
> +				return false;
>  			break;
>  		}
>  	}
>  
> -	if (!data->video_) {
> +	if (!data) {
>  		LOG(UVC, Error) << "Could not find a default video device";
> -		return false;
> +		return -ENODEV;
>  	}
>  
> -	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 +333,53 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
>  	return true;
>  }
>  
> +int UVCCameraData::init(MediaEntity *entity)
> +{
> +	int ret;
> +
> +	/* Create and open the video device. */
> +	video_ = new V4L2VideoDevice(entity);
> +	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()));
> +	}
> +
> +	return 0;
> +}
> +
>  void UVCCameraData::bufferReady(Buffer *buffer)
>  {
>  	Request *request = queuedRequests_.front();
>