[libcamera-devel] [PATCH v3 09/14] libcamera: pipeline: uvcvideo: Add controls support
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Jul 1 19:20:55 CEST 2019
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 at 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 at ideasonboard.com>
>
> Why so?
I believe because the previous version was work in progress.
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at 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();
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list