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

[Dave Stevenson]

Hi Jacopo

On Fri, 4 Nov 2022 at 10:39, Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> Hi David
>
> On Fri, Nov 04, 2022 at 09:58:41AM +0000, David Plowman 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!
> >
>
> Yeah, sorry for the long emails, in the end it was mostly me being confused
> and Dave clarifying things
>
> I'm about to test these patches on RkISP1 and then I'll finally review
> them without any further digression
>
<snip>
> > > 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.
>
> What to do if they're RO or only have a single value:
> I expect that they do not register any V4L2_CTRL_FLAG_MODIFY_LAYOUT
> flag, they should support a single bayer order which is the one
> reported in the format, and your patch won't transform it because the
> above mentioned flag isn't there. Do you agree with this ?

Read-only but with V4L2_CTRL_FLAG_MODIFY_LAYOUT would be a very odd choice.
Likewise single value with V4L2_CTRL_FLAG_MODIFY_LAYOUT is illogical.

> What to do if the driver is broken: I would say it has to be fixed,
> but we should at least try not to fail too hard. Ofc it depends on how
> broken the driver is, but I guess it's legit to assume the driver is
> somewhat compliant...

Define broken when there is no guidance on how these things should be
implemented?

Looking a little further down the line, how would it interact with
quad Bayer sensors without remosaic - what would the correct flip
behaviour be there?
Native might be
R R G G R R G G
R R G G R R G G
G G B B G G B B
G G B B G G B B
R R G G R R G G
R R G G R R G G
so now we have even more options for flipping and changing the pattern
in weird and wacky ways. Write documentation on the correct
implementation before we get there!

> >
> > 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 agree in principle, but I don't see it happening anytime soon in
> V4L2 :)

Does it need to happen in V4L2, or is this more that libcamera needs
to separate the two so that a raw stream can be requested as "Bayer
whatever", and libcamera/V4L2 can fix it up internally and report the
resulting Bayer order?

  Dave

> >
> > David