[libcamera-devel] [RFC PATCH 3/5] libcamera: pipeline: Add readControls(), writeControl() interfaces

[Kieran Bingham]

Hi Laurent,

Thank you for your comments, they are certainly helpful for sorting out
some of the design issues.

On 07/06/2019 17:19, Laurent Pinchart wrote:
> On Thu, Jun 06, 2019 at 09:56:52PM +0100, Kieran Bingham wrote:
>> Pipeline handlers must implement functions to handle controls as part of
>> their interface.
>>
>> Extend the pipeline handler interface to declare this requirement and
>> implement basic control functionality in the currently supported
>> PipelineHandlers
> 
> I don't think this should be exposed through the PipelineHandler API, it
> should be handled inside each pipeline handler instead. You're calling
> the readControls() and writeControls() methods of the pipeline handler
> internally only in this patch, so it's effectively internal already.

I had visions of the 'call' to read/write the controls being in a common
point, but as the PipelineHandlers determine when the request completes,
and the internal timings - I think you're right. There is no need to
enforce these functions on the handler API.

It's just up to a pipeline handler to decide how to deal with controls.


>> ---
>>  src/libcamera/include/pipeline_handler.h |   3 +
>>  src/libcamera/pipeline/ipu3/ipu3.cpp     |  19 ++++
>>  src/libcamera/pipeline/raspberrypi.cpp   | 108 ++++++++++++++++++-
>>  src/libcamera/pipeline/rkisp1/rkisp1.cpp |  19 ++++
>>  src/libcamera/pipeline/uvcvideo.cpp      | 127 ++++++++++++++++++++++-
>>  src/libcamera/pipeline/vimc.cpp          |  19 ++++
>>  6 files changed, 293 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
>> index 7da6df1ab2a0..4e306d964bff 100644
>> --- a/src/libcamera/include/pipeline_handler.h
>> +++ b/src/libcamera/include/pipeline_handler.h
>> @@ -72,6 +72,9 @@ public:
>>  	virtual int start(Camera *camera) = 0;
>>  	virtual void stop(Camera *camera);
>>  
>> +	virtual int readControls(Camera *camera, Request *request) = 0;
>> +	virtual int writeControls(Camera *camera, Request *request) = 0;
>> +
>>  	virtual int queueRequest(Camera *camera, Request *request);
>>  
>>  	bool completeBuffer(Camera *camera, Request *request, Buffer *buffer);
> 
> [snip]
> 
>> diff --git a/src/libcamera/pipeline/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi.cpp
>> index d6749eaae759..337554501c82 100644
>> --- a/src/libcamera/pipeline/raspberrypi.cpp
>> +++ b/src/libcamera/pipeline/raspberrypi.cpp
>> @@ -15,6 +15,7 @@
>>  #include "media_device.h"
>>  #include "pipeline_handler.h"
>>  #include "utils.h"
>> +#include "v4l2_controls.h"
>>  #include "v4l2_device.h"
>>  
>>  namespace libcamera {
>> @@ -77,6 +78,9 @@ public:
>>  	int start(Camera *camera) override;
>>  	void stop(Camera *camera) override;
>>  
>> +	int writeControls(Camera *camera, Request *request);
>> +	int readControls(Camera *camera, Request *request);
>> +
>>  	int queueRequest(Camera *camera, Request *request) override;
>>  
>>  	bool match(DeviceEnumerator *enumerator) override;
>> @@ -293,6 +297,94 @@ void PipelineHandlerRPi::stop(Camera *camera)
>>  	PipelineHandler::stop(camera);
>>  }
>>  
>> +int PipelineHandlerRPi::writeControls(Camera *camera, Request *request)
>> +{
>> +	RPiCameraData *data = cameraData(camera);
>> +
>> +	std::vector<V4L2Control *> controls;
>> +
>> +	for (Control c : request->controls()) {
>> +		if (c.value().isWrite()) {
> 
> Would it make sense to split the read and write sets at the Request
> level ?


I'd love to be able to do a python generator type syntax here, and
'yield' to cheaply iterate the controls handling ones that are appropriate.

This might be possible by writing our own iterators, but I'm not sure
how elegant that would be.


If you are imagining that every request defines every control, how would
you anticipate then splitting up the distinction at the request level?
With multiple sets?


What values would you expect to be present in the reqeust when it is
created with preset controls?

 - All values at default?
 - All values at current / cached values?
 - Not set at all... perhaps in the request in an 'unset' state.?
 - Some other option?





>> +			switch (c.id()) {
>> +			case ManualGain:
>> +				break;
>> +			case ManualExposure:
>> +				break;
>> +			default:
>> +				LOG(RPI, Error)
>> +					<< "Unhandled write control";
>> +				break;
>> +			}
>> +		}
>> +	}
>> +
>> +	/* Perhaps setControls could accept an empty vector/list as success? */
>> +	if (!controls.empty())
>> +		return data->unicam_->setControls(controls);
>> +
>> +	return 0;
>> +}
>> +
>> +/* This is becoming horrible. How can we improve this
>> + * - Iterate reqeust controls to find controls to read
>> + * - Add those to a new list
>> + * - Query the device for that list to get controls
>> + * - iterate returned list
>> + * 	- For each control, search for corresponding control in request
>> + * 	- Set control value to return value.
>> + * 	- Return list.
>> + * 	- Check for any values that were not updated?
>> + */
>> +int PipelineHandlerRPi::readControls(Camera *camera, Request *request)
>> +{
>> +	RPiCameraData *data = cameraData(camera);
>> +	std::vector<unsigned int> controlIDs;
>> +	std::vector<V4L2Control *> controls;
>> +
>> +	for (Control c : request->controls()) {
>> +		if (c.value().isRead()) {
>> +			LOG(RPI, Error) << "Read Control: " << c;
>> +
>> +			switch (c.id()) {
>> +			case ManualGain:
>> +				controlIDs.push_back(V4L2_CID_ANALOGUE_GAIN);
>> +				break;
>> +			case ManualExposure:
>> +				controlIDs.push_back(V4L2_CID_EXPOSURE);
>> +				break;
>> +			default:
>> +				LOG(RPI, Error)
>> +					<< "Unhandled write control";
>> +				break;
>> +			}
>> +		}
>> +	}
>> +
>> +	/* Perhaps setControls could accept an empty vector/list as success? */
>> +	if (controlIDs.empty())
>> +		return 0;
>> +
>> +	controls = data->unicam_->getControls(controlIDs);
>> +	if (controls.empty())
>> +		return -ENODATA;
>> +
>> +	for (V4L2Control *ctrl : controls) {
>> +		switch(ctrl->id()) {
>> +		case V4L2_CID_ANALOGUE_GAIN:
>> +			//Control gain(ManualGain); //= request->controls().find(ManualGain);
>> +			auto it = request->controls().find(ManualGain);
>> +			Control gain = *it;
>> +
>> +			//V4L2IntControl *c = static_cast<V4L2IntControl *>(ctrl);
>> +			gain.value().set(0x88);
>> +
>> +			break;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  int PipelineHandlerRPi::queueRequest(Camera *camera, Request *request)
>>  {
>>  	RPiCameraData *data = cameraData(camera);
>> @@ -304,7 +396,15 @@ int PipelineHandlerRPi::queueRequest(Camera *camera, Request *request)
>>  		return -ENOENT;
>>  	}
>>  
>> -	int ret = data->isp_.capture->queueBuffer(buffer);
>> +	/*
>> +	 * Handle 'set' controls first
>> +	 * Todo: Validate ordering and timing.
> 
> Handling timing will be interesting, as V4L2 controls are synchronous,
> while buffers are not. I expect we'll need to evaluate when the buffer
> will be captured, and set a timer accordingly to set controls (taking
> into account the latencies of the capture pipeline).

Indeed - making sure we set the value for that buffer is going to be
interesting. It might be some 'guess work' :-( - but we should know how
many buffers are queued ahead of us, and we can store the completion
time of each previously completed buffer so we should be able to
estimate when things happen in some form, and calculate our expected
'latency' for any given request when it is queued.

Hrm ... that sounds like it might actually be quite fun (but probably
also painful somehow) to implement ...


>> +	 */
>> +	int ret = writeControls(camera, request);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = data->isp_.capture->queueBuffer(buffer);
>>  	if (ret < 0)
>>  		return ret;
>>  
>> @@ -394,6 +494,12 @@ void RPiCameraData::ispCaptureReady(Buffer *buffer)
>>  	Request *request = queuedRequests_.front();
>>  
>>  	pipe_->completeBuffer(camera_, request, buffer);
>> +
>> +	int ret = pipe_->readControls(camera_, request);
>> +	if (ret < 0)
>> +		LOG(RPI, Error)
>> +			<< "Failed to read controls. No way to pass error";
> 
> Should this be reflected in the request status ?

Aha, yes of course. That is indeed how the status could be returned!

Can we end up in a situation were we have multiple error status' to
return in a request? Perhaps we might have to store a vector of Error
status'?

> 
>> +
>>  	pipe_->completeRequest(camera_, request);
>>  }
>>  
> 
> [snip]
> 
>> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
>> index e2d02c0dafb8..216fbe237827 100644
>> --- a/src/libcamera/pipeline/uvcvideo.cpp
>> +++ b/src/libcamera/pipeline/uvcvideo.cpp
>> @@ -14,6 +14,7 @@
>>  #include "media_device.h"
>>  #include "pipeline_handler.h"
>>  #include "utils.h"
>> +#include "v4l2_controls.h"
>>  #include "v4l2_device.h"
>>  
>>  namespace libcamera {
>> @@ -73,6 +74,9 @@ public:
>>  	int start(Camera *camera) override;
>>  	void stop(Camera *camera) override;
>>  
>> +	int readControls(Camera *camera, Request *request) override;
>> +	int writeControls(Camera *camera, Request *request) override;
>> +
>>  	int queueRequest(Camera *camera, Request *request) override;
>>  
>>  	bool match(DeviceEnumerator *enumerator) override;
>> @@ -252,6 +256,117 @@ void PipelineHandlerUVC::stop(Camera *camera)
>>  	PipelineHandler::stop(camera);
>>  }
>>  
>> +int PipelineHandlerUVC::writeControls(Camera *camera, Request *request)
>> +{
>> +	UVCCameraData *data = cameraData(camera);
>> +
>> +	std::vector<V4L2Control *> controls;
>> +
>> +	for (Control c : request->controls()) {
>> +		if (c.value().isWrite()) {
>> +			switch (c.id()) {
>> +			case ManualBrightness:
>> +				controls.emplace_back(new V4L2IntControl(V4L2_CID_BRIGHTNESS, c.value().getInt()));
>> +				break;
>> +
>> +			case IpaAwbEnable:
>> +			case ManualGain:
>> +			case ManualExposure:
>> +				break;
>> +			default:
>> +				LOG(UVC, Error)
>> +					<< "Unhandled write control";
>> +				break;
>> +			}
>> +		}
>> +	}
>> +
>> +	/* Perhaps setControls could accept an empty vector/list as success? */
>> +	if (!controls.empty()) {
>> +		int ret = data->video_->setControls(controls);
>> +
>> +		/* It would be nice to avoid the need to manually delete
>> +		 * the controls */
> 
> Yes, this seems like a no-go to me.

I think we can adapt the V4L2Control interface to accept a set or map as
well, and potentially even (where relevant) put in a reference to the
ControlValue supplied to prevent excess copying of what will essentially
be the same object type to update.



> 
>> +		for (V4L2Control *c : controls)
>> +			delete c;
>> +
>> +		return ret;
>> +	}
>> +
>> +
>> +	return 0;
>> +}
>> +
>> +int PipelineHandlerUVC::readControls(Camera *camera, Request *request)
>> +{
>> +	UVCCameraData *data = cameraData(camera);
>> +	std::vector<unsigned int> controlIDs;
>> +	std::vector<V4L2Control *> controls;
>> +
>> +	for (Control c : request->controls()) {
>> +		if (c.value().isRead()) {
>> +			LOG(UVC, Error) << "Read Control: " << c;
>> +
>> +			switch (c.id()) {
>> +			case ManualBrightness:
>> +				controlIDs.push_back(V4L2_CID_BRIGHTNESS);
>> +				break;
>> +			case ManualGain:
>> +				controlIDs.push_back(V4L2_CID_ANALOGUE_GAIN);
>> +				break;
>> +			case ManualExposure:
>> +				controlIDs.push_back(V4L2_CID_EXPOSURE);
>> +				break;
>> +			default:
>> +				LOG(UVC, Error)
>> +					<< "Unhandled write control";
>> +				break;
>> +			}
>> +		}
>> +	}
>> +
>> +	/* Perhaps setControls could accept an empty vector/list as success? */
>> +	if (controlIDs.empty())
>> +		return 0;
>> +
>> +	controls = data->video_->getControls(controlIDs);
>> +	if (controls.empty())
>> +		return -ENODATA;
>> +
>> +	for (V4L2Control *ctrl : controls) {
>> +		switch (ctrl->id()) {
>> +		case V4L2_CID_BRIGHTNESS: {
>> +			Control bright = *request->controls().find(ManualBrightness);
>> +			/* RFC:
>> +			 * If the iterator doesn't find the control ^
>> +			 * then it causes a segfault, so this is really nasty...
>> +			 * and where just a map might be better - (ornot?) as it
>> +			 * will create the object at that key.
>> +			 * Now , that *shouldn't* happen (we should only look
>> +			 * for controls that were given to us in this
>> +			 * function ... but
> 
> 			auto iter = request->controls().find(ManualBrightness);
> 			if (iter == request->controls().end())
> 				...

It shouldn't ever occur, as we would only look for a value we have
requested... but that's what made me interested in the difference for
using a map, as it would then create the ControlValue for use.

But then also if we are always going to have all controls in the
request, then failing to find a control would become an assertion failure.


> 
>> +			 */
>> +			V4L2IntControl *vc = static_cast<V4L2IntControl *>(ctrl);
>> +			bright.value().set(vc->value());
>> +
>> +			LOG(UVC, Debug) << "Setting Brightness: " << bright;
>> +
>> +			break;
>> +		}
>> +		case V4L2_CID_ANALOGUE_GAIN: {
>> +			Control gain = *request->controls().find(ManualGain);
>> +			V4L2IntControl *vc = static_cast<V4L2IntControl *>(ctrl);
>> +			gain.value().set(vc->value());
>> +			break;
>> +		}
>> +		default:
>> +			LOG(UVC, Warning) << "Unhandled V4L2 Control ID";
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)
>>  {
>>  	UVCCameraData *data = cameraData(camera);
>> @@ -263,7 +378,11 @@ int PipelineHandlerUVC::queueRequest(Camera *camera, Request *request)
>>  		return -ENOENT;
>>  	}
>>  
>> -	int ret = data->video_->queueBuffer(buffer);
>> +	int ret = writeControls(camera, request);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = data->video_->queueBuffer(buffer);
>>  	if (ret < 0)
>>  		return ret;
>>  
>> @@ -317,6 +436,12 @@ void UVCCameraData::bufferReady(Buffer *buffer)
>>  	Request *request = queuedRequests_.front();
>>  
>>  	pipe_->completeBuffer(camera_, request, buffer);
>> +
>> +	int ret = pipe_->readControls(camera_, request);
>> +	if (ret < 0)
>> +		LOG(UVC, Error)
>> +			<< "Failed to read controls. No way to pass error";
>> +
>>  	pipe_->completeRequest(camera_, request);
>>  }
>>  
> 
> [snip]
> 

-- 
Regards
--
Kieran