[libcamera-devel] [RFC PATCH 2/2] libcamera: camera_sensor: Do not clear camera flips when listing formats
Jacopo Mondi
jacopo at jmondi.org
Thu Nov 3 16:26:55 CET 2022
Hi again
On Thu, Nov 03, 2022 at 11:52:23AM +0000, Dave Stevenson via libcamera-devel wrote:
> On Thu, 3 Nov 2022 at 11:24, David Plowman
> <david.plowman at raspberrypi.com> wrote:
> >
> > 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...
>
> Yes I believe V4L2_CTRL_FLAG_MODIFY_LAYOUT should do what you want.
>
> The OnSemi sensors I'm thinking of don't have drivers in mainline, so
> there's no driver to check currently.
>
> Looking at mainline, only imx219 and imx258 set the flag.
> - ov2680, imx208, imx319, imx355, mt9m001, ov08d10 all change the
> order but don't set V4L2_CTRL_FLAG_MODIFY_LAYOUT
> - hi847 and ov13b10 move the crop to maintain the Bayer order.
> - imx274 and ov5648 only ever report one order, but supports flips and
> has no apparent cropping shift.
> - I have missed it in my imx290 patches to add flips that I was about to send!
>
> So, as Jacopo summarised the situation on event handlers for controls,
> room for improvement!
>
To add more pleasure, as discussed on a review comment on Dave's
series
https://patchwork.linuxtv.org/project/linux-media/patch/20221005152809.3785786-12-dave.stevenson@raspberrypi.com/
Some sensors assumes to be rotated and bury their rotation
settings in their register tables.
In the example of the imx258 sensor Dave has mentioned:
- The bayer order reported through the format is GRBG
https://github.com/torvalds/linux/blob/master/drivers/media/i2c/imx258.c#L822
- The sensor driver applies mirroring by default at s_stream
time (without specifying the direction, vertical or horizontal)
/* Set Orientation be 180 degree */
ret = imx258_write_reg(imx258, REG_MIRROR_FLIP_CONTROL,
IMX258_REG_VALUE_08BIT, REG_CONFIG_MIRROR_FLIP);
- I found an interesting comment on the patch series changelog:
https://www.spinics.net/lists/linux-media/msg133840.html
Output order of test pattern is always BGGR, so it needs a flip to
rotate bayer pattern to required one (GRBG)
So it might seem legit to presume the native Bayer pattern on the
pixel array is BGGR, read from the right-bottom corner:
RG RG RG RG RG
GB GB GB GB GB
(1, n) RG RG RG RG RG (1,0)
GB GB GB GB GB
(0, n) <-- (0,0)
To obtain the Bayer GRBG order advertised as output format I presume the
REG_CONFIG_MIRROR_FLIP vertically flips the image, moving the (0,0)
pixel coordinate, from where reading is started, on the top-right corner:
(0, n) <-- (0,0)
RG RG RG RG RG
(1, n) GB GB GB GB GB (1, 0)
RG RG RG RG RG
GB GB GB GB GB
According to your below patch and Bayer::transform():
- BGGR becomes GRBG via a vertical flip, right ? So this should
confirm the above assumption about an unconditional vertical flip
If the driver would register flip controls, it would register them as
VFLIP = 1, HFLIP = 0. Here below you would get GRBG, vertically
transform it to BGGR and store BGGR as "native" format, but now you
won't be able to apply it to the sensor, as it only supports GRBG.
What drivers should do is to register somehow their -native- Bayer
pattern, use a generic BAYER format in set_fmt and let userspace deal
with rotations and Bayer pattern re-odering as userspace is
able to access the mounting rotation via the CID_CAMERA_ROTATION
control.
Unfortunately no driver afaict behaves this way. They might report via
v4l2-ctl if any flip is applied to them, as Dave does in the series,
but adjusting the Bayer pattern as you do below might lead to results
that won't apply on the sensor. Do you agree or did I get lost ?
To be honest I would drop clearing the flips, and to
protect against concurrent usages, validate that the current flip values
match the default controls value. If the two do not match, and the
driver advertises V4L2_CTRL_FLAG_MODIFY_LAYOUT, it means the mbus code
we're seeing right now is the result of another user having flipped
the image. In this case you could flip the format like you're doing
below to obtain the "unflipped" Bayer pattern, as we know it will work
as it is the "defaul configuration" order.
Final question, and sorry for the long email, but what would it be the
real purpose of obtaining the native bayer pattern order in your use case. I
presume we had that attempt to get the native format because android
wants to have the native order reported. I think that CameraSensor
should instead try to use a bayer order that work for the drive in its
default configuration which, if the driver claims to be rotated by
default, won't match the actual native order.
We have a camera properties database where such native information
could eventually be stored if it's of interest!
Hope I didn't get lost!
> Dave
>
> > 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!!
Agreed, userspace has all the information to adjust the ordering as it
likes..
> >
> > 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
> > > >
More information about the libcamera-devel
mailing list