[libcamera-devel] [RFC PATCH 2/2] libcamera: camera_sensor: Do not clear camera flips when listing formats
Dave Stevenson
dave.stevenson at raspberrypi.com
Fri Nov 4 11:40:51 CET 2022
Hi David
On Fri, 4 Nov 2022 at 09:58, David Plowman
<david.plowman at raspberrypi.com> wrote:
>
> Hi Jacopo, Dave, everyone
>
> Lots of discussion! And you all know far more about camera drivers
> than I ever want to, so I probably don't have that much to add.
>
> It seems to me like the choice of looking at that imx258 driver was a
> bit unlucky. As far as I could see, it simply has no HFLIP or VFLIP
> controls at all, and so doesn't really affect the debate here (I
> think, am I right?). I notice that on our linux tree it is supported -
> which I take to be all of Dave's magic!
Yes, I worked on imx258 as Soho Enterprises produced and sent us a module.
(The fact that 19 patches were needed to an accepted mainline driver
really demonstrates my complaint of drivers being written for specific
products rather than generic use).
Actually I'd say that it was a useful choice as it shows how messed up
accepted drivers can be, and therefore emphasises that userspace
should never make assumptions.
> On Thu, 3 Nov 2022 at 17:29, Jacopo Mondi <jacopo at jmondi.org> wrote:
> >
> > Hi Dave
> >
> > On Thu, Nov 03, 2022 at 04:26:07PM +0000, Dave Stevenson wrote:
> > > Hi Jacopo
> > >
> > > On Thu, 3 Nov 2022 at 15:26, Jacopo Mondi <jacopo at jmondi.org> wrote:
> > > >
> > > > 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);
> > >
> > > #define REG_CONFIG_MIRROR_FLIP 0x03 [1]
> > > so this is doing BOTH H & V flips in one hit.
> > >
> > > Datasheet for imx258 at [2].
> >
> > Your google-fu is certainly better than mine
> >
> > >
> > > [1] https://github.com/torvalds/linux/blob/master/drivers/media/i2c/imx258.c#L78
> > > [2] https://web.archive.org/web/20201027131326/www.hi.app/IMX258-datasheet.pdf
> > >
> > > > - 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)
> > >
> > > It's not uncommon that the test patterns are independent of flips as
> > > there is no image array to read out in the reverse direction.
> > > There are often 4 registers to configure the values to stick into each
> > > of the 4 colour channels, and those are simply inserted in order.
> > > Registers 0x0602-0x0609 on IMX258.
> >
> > Thanks, indeed my assumption that the test pattern was reflecting the
> > native order was incorrect
> >
> > >
> > > TBH I've never understood the love of implementing test patterns in
> > > drivers. If a driver is merged into mainline then there should be a
> > > fair confidence that it works, therefore why worry about test
> > > patterns? Isn't a captured image more valuable?
> > >
> > > > So it might seem legit to presume the native Bayer pattern on the
> > > > pixel array is BGGR, read from the right-bottom corner:
> > >
> > > I'm not sure if you're referencing imx258 specifically here, or
> > > talking generically.
> > > imx258 is a total mess, and I need to get around to upstreaming my
> > > patches at [3].
> > >
> > > The native Bayer order for imx258 is RGGB (see Fig 5-4 page 84, Fig
> > > 5-3 page 79, and several places in section 4). Surely the datasheet
> > > has to count as definitive.
> > >
> >
> > Indeed
> >
> > > As in patch [4], Y_ADD_STA register is set to 0, and Y_ADD_END to
> > > 3118, giving 3119 lines total for 3118 lines of readout. The hardcoded
> > > V flip on that starts us on the "wrong" line of the Bayer pattern for
> > > any sane person looking at it and trying to work out what the heck is
> > > going on - we're still getting RGGB. H flip that and the resulting
> > > Bayer order is GRBG. QED.
> > >
> >
> > As usual, everything works by chance!
> >
> > With a correct Y_ADD_END we would then get
> >
> > RGGB -> vflip -> GBRG -> hflip -> BGGR
> >
> > It mean that fixing the driver will imply changing the reported
> > bayer pattern I presume..
> >
> > > [3] https://github.com/raspberrypi/linux/commits/rpi-5.15.y/drivers/media/i2c/imx258.c
> > > [4] https://github.com/raspberrypi/linux/commit/1942d0da85bac79f3d2a1c2d8e797e97bd16b618
> >
> > Ah yes it does :)
> >
> > >
> > > >
> > > > 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
> > >
> > > Due to the error on Y_ADD_END, implementing a VFLIP gave no change in
> > > Bayer order! Work that one out in userspace (although it is a driver
> > > bug that causes it).
> > >
> > > > 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.
> > >
> > > Doing so goes against the V4L2 principle of the V4L2_PIX_FMT_* telling
> > > you exactly how to interpret the pixel data.
> > >
> > > I need to check the wording, but I thought CID_CAMERA_ROTATION told
> > > you the orientation of the sensor within the device such that you
> > > could stick it into EXIF or similar metadata. It doesn't tell you how
> > > the Bayer order would change.
> > >
> >
> > Rotation != flipping, you're right I was mixing the two.
> >
> > > > 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 ?
> > >
> > > AIUI At the point of actually wanting to stream, flips will be
> > > applied, and the format reported by the sensor driver will then be
> > > used to configure the rest of the pipeline.
> > > I hope that userspace never thinks it knows better than the driver
> > > with regard to Bayer order - as above there are enough quirks that it
> > > will get it wrong.
> > >
> >
> > just for sake of discussion:
> > libcamera would simply get from somewhere (driver or sensor properties
> > database) the native (or non-rotated) format, inspect the flips and
> > know what format report to applications. Driver will accept a wildcard
> > BAYER format. But yes, good luck configuring the pipeline if any
> > component along the way is sensible to the bayer ordering.
> >
> > > > 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.
> > >
> > > A colour equivalent to ov9282 is going to blow up your assumption on
> > > default value. The original driver didn't support flips, but H & V
> > > flipped the image (much like imx258), so to avoid regressions we have
> > > a default of being flipped.
> >
> > Not sure I get why. The color equivalent will report the bayer order
> > of the H&V flipped Bayer format version. According to
> > your series, it will also report VFLIP = HFLIP = 1 by default and
> > read-only. As they're RO, no V4L2_CTRL_FLAG_MODIFY_LAYOUT flag is
> > registered, and no transform is applied. The sensor will only speak
> > the rot180 bayer format version.
> >
> > The patch below transforms only if V4L2_CTRL_FLAG_MODIFY_LAYOUT, which
> > implies the driver can speak the non-modified format too, so I guess
> > it works under the assumptions that:
> >
> > 1) Drivers report all the flips they apply implicitly and not all of
> > them do
> > 2) They correctly register V4L2_CTRL_FLAG_MODIFY_LAYOUT if flip
> > modifies the bayer pattern order and as you noticed not all of them
> > do so
>
> There are definitely some corner cases to check here. I think we're
> fine if there are simply no H/VFLIP controls, but what if they exist
> and are read-only? What if they're writable but allow only a single
> value? And what to do about drivers that aren't behaving as we expect.
Never try to predict the driver in userspace.
> I think things will always work out and after configure() you should
> see the correct Bayer pattern, but good luck to applications trying to
> figure out what raw format to ask for.
>
> I know I've commented, as have others, that specifying the actual
> Bayer order in the application is not really useful, though things
> like bit depth and packing are. Any strong expectation to get the
> Bayer order right would be quite onerous - it would be much more
> helpful to be able to ask for that XXXX ("whatever!") Bayer order!
I haven't looked at the libcamera API for raw streams, but I'll
certainly agree that the client trying to guess the Bayer order to
request is almost certainly going to fail, and being able to request a
generic "Bayer X bit" which then tells you the actual format on a
frame makes a lot of sense.
Dave
> David
>
> >
More information about the libcamera-devel
mailing list