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

[Laurent Pinchart]

Hi Kieran,

On Fri, Jun 07, 2019 at 10:35:15PM +0100, Kieran Bingham wrote:
> 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?

Yes, a set of controls, and a set of metadata (I'm more and more
thinking that what we called read controls so far should be renamed
metadata).

> 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?

I don't expect requests to be created with "preset controls". Requests
should be created empty, and populate it by applications. This could be
done manually, or through a preset provided by libcamera. I would expect
applications to create multiple instances of lists of controls, populate
them with the help of preset and manual settings, and then use them at
runtime to populate requests.

> >> +			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 ...

Fun and painful, yes :-) I hope we'll be able to factor some of that
code to helpers at a later point.

> >> +	 */
> >> +	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'?

Ideally we should have no error :-) When an error occurs I expect it
will be fatal for the application, so I don't think we need to store
multiple error sources.

> >> +
> >>  	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.

I don't think all requests will contain all controls, I expect them to
be incremental, without having to store the full state.

> >> +			 */
> >> +			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,

Laurent Pinchart