[libcamera-devel] [RFC PATCH 3/5] libcamera: pipeline: Add readControls(), writeControl() interfaces
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Jun 11 16:16:53 CEST 2019
Hi Laurent,
On 11/06/2019 13:13, Laurent Pinchart wrote:
> 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).
Hrm... well if all 'metadata' is read back all the time, then the
application doesn't need to request it specifically anyway.
I'm not yet sure if this would be a separate object in the request when
it's returned or just the 'controls' map updated.
I feel like just updating the controls map seems obvious at the moment.
Lets see where it goes, and that part can easily change/adapt later.
>> 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.
Ok - that's fine by me then.
>
>>>> +
>>>> 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.
Well, actually - after our discussion on the other thread, at this point
in the code - it could well be that we do want to read more controls
than were in the request, and thus a map really would be a good option.
Then the pipeline handler can update any extra control/value pairs as
necessary - and the application will get a consistent (or rather always
at least a minimum) set of data back
>
>>>> + */
>>>> + 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
More information about the libcamera-devel
mailing list