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

[David Plowman]

Hi Dave

On Thu, 3 Nov 2022 at 11:02, Dave Stevenson
<dave.stevenson at raspberrypi.com> wrote:
>
> Hi David
>
> On Thu, 3 Nov 2022 at 10:40, David Plowman via libcamera-devel
> <libcamera-devel at lists.libcamera.org> 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.
>
> I don't believe that libcamera knows for definite whether the sensor
> changes the Bayer order or not. Several of the OnSemi sensors I have
> seen do not change, presumably as they shift their crop by 1 pixel.

Yes, that's a good point. I've vaguely assumed that I can check the
control's V4L2_CTRL_FLAG_MODIFY_LAYOUT to discover whether this is the
case or not, but as we keep on discovering, there may be no guarantees
about this. Do we know what these particular sensors do here? The Pi
PH actually already assumes this flag "works", so the changes here
possibly don't make anything worse, though they do engrain the
assumption more deeply...

Presumably you can rely on the formats being returned correctly after
setting the flip bits, though having to re-query the formats from the
device like that would be irritating too. And you'd never be able to
predict the format ahead of time, only after setting it. I don't know
if that's better or worse than saying "change the driver". Opinions
welcome on that one!

To be honest I always have difficulty with the Bayer order being part
of the format. An application might legitimately want to have a say
about the bit depth or packing, but the Bayer order?? Being able to
request an XXXX ("anything goes") Bayer order, which gets updated once
the sensor is configured, would be much more application-friendly. But
another can of worms!!

David

>
> The original comment mentioned that scenario:
> "This is harmless for sensors where the flips don't affect the Bayer order"
>
>   Dave
>
> > 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;
> > +       }
> > +
> > +       /* 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
> >