[libcamera-devel] [RFC PATCH 2/2] libcamera: camera_sensor: Do not clear camera flips when listing formats

[David Plowman]

Hi Jacopo

Thanks for this suggestion!

On Fri, 4 Nov 2022 at 12:17, Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> Hi David
>
> On Thu, Nov 03, 2022 at 10:40:27AM +0000, David Plowman via libcamera-devel wrote:
> > Previously the code used to clear the camnera's h and v flip bits when
> > enumerating the supported formats so as to obtain any Bayer formats in
> > the sensor's native (untransformed) orientation. However this fails
> > when the camera is already in use elsewhere.
> >
> > Instead, we query the current state of the flip bits and transform the
> > formats - which we obtain in their flipped orientation - back into
> > their native orientation to be stored.
> >
> > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > ---
> >  src/libcamera/camera_sensor.cpp | 51 +++++++++++++++++++++++++++------
> >  1 file changed, 43 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index 572a313a..6670dfb9 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -19,6 +19,8 @@
> >
> >  #include <libcamera/base/utils.h>
> >
> > +#include <libcamera/transform.h>
> > +
> >  #include "libcamera/internal/bayer_format.h"
> >  #include "libcamera/internal/camera_lens.h"
> >  #include "libcamera/internal/camera_sensor_properties.h"
> > @@ -108,18 +110,51 @@ int CameraSensor::init()
> >               return ret;
> >
> >       /*
> > -      * Clear any flips to be sure we get the "native" Bayer order. This is
> > -      * harmless for sensors where the flips don't affect the Bayer order.
> > +      * We want to get the native mbus codes for the sensor, without any flips.
> > +      * We can't clear any flips here, so we have to read the current values
> > +      * (if the flip controls exist), decide whether they actually modify any
> > +      * output Bayer pattern, and finally undo their effect on the formats.
> > +      *
> > +      * First, check if the flip controls exist and if so read them.
> >        */
> >       ControlList ctrls(subdev_->controls());
> > -     if (subdev_->controls().find(V4L2_CID_HFLIP) != subdev_->controls().end())
> > -             ctrls.set(V4L2_CID_HFLIP, 0);
> > -     if (subdev_->controls().find(V4L2_CID_VFLIP) != subdev_->controls().end())
> > -             ctrls.set(V4L2_CID_VFLIP, 0);
> > -     subdev_->setControls(&ctrls);
> > +     std::vector<uint32_t> flipCtrlIds;
> > +     bool hasHflip = subdev_->controls().find(V4L2_CID_HFLIP) != subdev_->controls().end();
> > +     bool hasVflip = subdev_->controls().find(V4L2_CID_VFLIP) != subdev_->controls().end();
> > +     if (hasHflip)
> > +             flipCtrlIds.push_back(V4L2_CID_HFLIP);
> > +     if (hasVflip)
> > +             flipCtrlIds.push_back(V4L2_CID_VFLIP);
> > +     ControlList flipCtrls = subdev_->getControls(flipCtrlIds);
> > +
> > +     /* Now construct a transform that would undo any flips. */
> > +     Transform transform = Transform::Identity;
> > +     if (hasHflip && flipCtrls.get(V4L2_CID_HFLIP).get<int>()) {
> > +             const struct v4l2_query_ext_ctrl *extCtrl = subdev_->controlInfo(V4L2_CID_HFLIP);
> > +             if (extCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT)
> > +                     transform |= Transform::HFlip;
> > +     }
> > +     if (hasVflip && flipCtrls.get(V4L2_CID_VFLIP).get<int>()) {
> > +             const struct v4l2_query_ext_ctrl *extCtrl = subdev_->controlInfo(V4L2_CID_VFLIP);
> > +             if (extCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT)
> > +                     transform |= Transform::VFlip;
> > +     }
>
> Might avoid a few lookups with:
>
>         const struct v4l2_query_ext_ctrl *hflipInfo = subdev_->controlInfo(V4L2_CID_HFLIP);
>         const struct v4l2_query_ext_ctrl *vflipInfo = subdev_->controlInfo(V4L2_CID_VFLIP);
>         if (hflipInfo)
>                 flipCtrlIds.push_back(V4L2_CID_HFLIP);
>         if (vflipInfo)
>                 flipCtrlIds.push_back(V4L2_CID_VFLIP);
>         ControlList flipCtrls = subdev_->getControls(flipCtrlIds);
>
>         /* Now construct a transform that would undo any flips. */
>         Transform transform = Transform::Identity;
>         if (hflipInfo && flipCtrls.get(V4L2_CID_HFLIP).get<int>() &&
>             (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT))
>                 transform |= Transform::HFlip;
>         if (vflipInfo && flipCtrls.get(V4L2_CID_VFLIP).get<int>() &&
>             (vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT))
>                 transform |= Transform::VFlip;
>
>
> Tested with a sensor without flips and with flips but not flag

Yes, I think that looks a bit nicer, doesn't it? I'll post a v2 with
that update shortly.

Thanks!
David

>
>
> > +
> > +     /* Finally get the formats, and apply the transform to the mbus codes. */
> > +     auto formats = subdev_->formats(pad_);
> > +     for (const auto &format : formats) {
> > +             unsigned int mbusCode = format.first;
> > +             BayerFormat bayerFormat = BayerFormat::fromMbusCode(mbusCode);
> > +             bool valid = true;
> > +
> > +             if (bayerFormat.isValid())
> > +                     mbusCode = bayerFormat.transform(transform).toMbusCode(valid);
> > +
> > +             if (valid)
> > +                     formats_[mbusCode] = std::move(format.second);
> > +     }
> >
> >       /* Enumerate, sort and cache media bus codes and sizes. */
> > -     formats_ = subdev_->formats(pad_);
> >       if (formats_.empty()) {
> >               LOG(CameraSensor, Error) << "No image format found";
> >               return -EINVAL;
> > --
> > 2.30.2
> >