[libcamera-devel] [PATCH v5 1/7] libcamera: pipeline: uvcvideo: Refactor control handling
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Apr 26 19:00:05 CEST 2020
Hi Jacopo,
On Sun, Apr 26, 2020 at 02:50:57PM +0200, Jacopo Mondi wrote:
> On Sat, Apr 25, 2020 at 03:45:27AM +0300, Laurent Pinchart wrote:
> > Move addition and processing of individual controls to separate
> > functions, to prepare for more complex mappings of control values.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 103 ++++++++++++-------
> > 1 file changed, 67 insertions(+), 36 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > index ffbddf27ae2f..92777b9f5fe4 100644
> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > @@ -42,6 +42,8 @@ public:
> > }
> >
> > int init(MediaEntity *entity);
> > + void addControl(uint32_t cid, const ControlInfo &v4l2info,
> > + ControlInfoMap::Map *ctrls);
> > void bufferReady(FrameBuffer *buffer);
> >
> > V4L2VideoDevice *video_;
> > @@ -76,6 +78,8 @@ public:
> > bool match(DeviceEnumerator *enumerator) override;
> >
> > private:
> > + int processControl(ControlList *controls, unsigned int id,
> > + const ControlValue &value);
> > int processControls(UVCCameraData *data, Request *request);
> >
> > UVCCameraData *cameraData(const Camera *camera)
> > @@ -237,6 +241,37 @@ void PipelineHandlerUVC::stop(Camera *camera)
> > data->video_->releaseBuffers();
> > }
> >
> > +int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
> > + const ControlValue &value)
> > +{
> > + if (value.type() != ControlTypeInteger32)
> > + return -EINVAL;
>
> Is this needed ? All supported controls are int32, you will fail later
> anyway if the control id is not supported, and this check will have to
> be removed once (and if) we support non int32_t controls
It's actually removed in later patches within this series. I've added it
here for testing purpose, I will drop it.
> > +
> > + uint32_t cid;
> > +
> > + if (id == controls::Brightness)
> > + cid = V4L2_CID_BRIGHTNESS;
> > + else if (id == controls::Contrast)
> > + cid = V4L2_CID_CONTRAST;
> > + else if (id == controls::Saturation)
> > + cid = V4L2_CID_SATURATION;
> > + else if (id == controls::ManualExposure)
> > + cid = V4L2_CID_EXPOSURE_ABSOLUTE;
> > + else if (id == controls::ManualGain)
> > + cid = V4L2_CID_GAIN;
> > + else
> > + return -EINVAL;
> > +
> > + int32_t ivalue = value.get<int32_t>();
>
> could you move this one after the next if condition ?
Sure. The code is reworked later anyway.
> > +
> > + if (cid == V4L2_CID_EXPOSURE_ABSOLUTE)
> > + controls->set(V4L2_CID_EXPOSURE_AUTO, static_cast<int32_t>(1));
> > +
> > + controls->set(cid, ivalue);
> > +
> > + return 0;
> > +}
> > +
> > int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)
> > {
> > ControlList controls(data->video_->controls());
> > @@ -245,18 +280,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)
> > unsigned int id = it.first;
> > ControlValue &value = it.second;
> >
> > - if (id == controls::Brightness) {
> > - controls.set(V4L2_CID_BRIGHTNESS, value);
> > - } else if (id == controls::Contrast) {
> > - controls.set(V4L2_CID_CONTRAST, value);
> > - } else if (id == controls::Saturation) {
> > - controls.set(V4L2_CID_SATURATION, value);
> > - } else if (id == controls::ManualExposure) {
> > - controls.set(V4L2_CID_EXPOSURE_AUTO, static_cast<int32_t>(1));
> > - controls.set(V4L2_CID_EXPOSURE_ABSOLUTE, value);
> > - } else if (id == controls::ManualGain) {
> > - controls.set(V4L2_CID_GAIN, value);
> > - }
> > + processControl(&controls, id, value);
> > }
> >
> > for (const auto &ctrl : controls)
> > @@ -346,34 +370,13 @@ int UVCCameraData::init(MediaEntity *entity)
> > video_->bufferReady.connect(this, &UVCCameraData::bufferReady);
> >
> > /* Initialise the supported controls. */
> > - const ControlInfoMap &controls = video_->controls();
> > ControlInfoMap::Map ctrls;
> >
> > - for (const auto &ctrl : controls) {
> > + for (const auto &ctrl : video_->controls()) {
> > + uint32_t cid = ctrl.first->id();
> > const ControlInfo &info = ctrl.second;
> > - const ControlId *id;
> >
> > - switch (ctrl.first->id()) {
> > - case V4L2_CID_BRIGHTNESS:
> > - id = &controls::Brightness;
> > - break;
> > - case V4L2_CID_CONTRAST:
> > - id = &controls::Contrast;
> > - break;
> > - case V4L2_CID_SATURATION:
> > - id = &controls::Saturation;
> > - break;
> > - case V4L2_CID_EXPOSURE_ABSOLUTE:
> > - id = &controls::ManualExposure;
> > - break;
> > - case V4L2_CID_GAIN:
> > - id = &controls::ManualGain;
> > - break;
> > - default:
> > - continue;
> > - }
> > -
> > - ctrls.emplace(id, info);
> > + addControl(cid, info, &ctrls);
> > }
> >
> > controlInfo_ = std::move(ctrls);
> > @@ -381,6 +384,34 @@ int UVCCameraData::init(MediaEntity *entity)
> > return 0;
> > }
> >
> > +void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
> > + ControlInfoMap::Map *ctrls)
> > +{
> > + const ControlId *id;
> > +
> > + switch (cid) {
> > + case V4L2_CID_BRIGHTNESS:
> > + id = &controls::Brightness;
> > + break;
> > + case V4L2_CID_CONTRAST:
> > + id = &controls::Contrast;
> > + break;
> > + case V4L2_CID_SATURATION:
> > + id = &controls::Saturation;
> > + break;
> > + case V4L2_CID_EXPOSURE_ABSOLUTE:
> > + id = &controls::ManualExposure;
> > + break;
> > + case V4L2_CID_GAIN:
> > + id = &controls::ManualGain;
>
> I'm a bit concerned all pipeline handlers will have to implement this
> libcamera-control<->v4l2-control translation, but that's not on this
> patch
We could possibly do better indeed. Don't forget that, in practice, few
pipeline handlers will translate libcamera controls to V4L2 controls,
most controls will be handled by the IPA. For an ISP-based pipeline
handlers, the saturation control, for instance, will likely be handled
through an RGB-to-RGB conversion matrix. So will the contrast. Something
along the lines of
[R' [ YUV [1 0 0 [ RGB [R
G' = to x 0 S 0 x to x G
B'] RGB ] 0 0 S] YUV ] B]
With the hardware programmed with a RGB-to-RGB matrix equal to
[ YUV [1 0 0 [ RGB
to x 0 S 0 x to
RGB ] 0 0 S] YUV ]
So it won't be a direct mapping.
> Anyway
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>
> > + break;
> > + default:
> > + return;
> > + }
> > +
> > + ctrls->emplace(id, v4l2Info);
> > +}
> > +
> > void UVCCameraData::bufferReady(FrameBuffer *buffer)
> > {
> > Request *request = buffer->request();
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list