[libcamera-devel] [RFC PATCH 0/2] Resolve invalid attempts to set sensor flip controls

[Jacopo Mondi]

Hi Naush

On Thu, Nov 03, 2022 at 01:26:42PM +0000, Naushir Patuck wrote:
> Hi Jacopo,
>
> On Thu, 3 Nov 2022 at 12:59, Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> > Hi Naush
> >
> > On Thu, Nov 03, 2022 at 12:05:31PM +0000, Naushir Patuck wrote:
> > > Hi Jacopo,
> > >
> > >
> > > On Thu, 3 Nov 2022 at 11:50, Jacopo Mondi via libcamera-devel <
> > > libcamera-devel at lists.libcamera.org> wrote:
> > >
> > > > Hi David
> > > >
> > > > On Thu, Nov 03, 2022 at 10:40:25AM +0000, David Plowman via
> > > > libcamera-devel wrote:
> > > > > Hi everyone
> > > > >
> > > > > (Warning: may affect all pipeline handlers!)
> > > > >
> > > > > I wanted to start the ball rolling on this. The problem is that when
> > > > > we enumerate the cameras in the system, we try to set some of the
> > > > > controls (specifically the HFLIP and VFLIP ones, and also later
> > > > > HBLANK). But this will fail if we don't "own" the camera, for
> > example,
> > > > > it is already in use elsewhere.
> > > >
> > > > Feels like I'm missing one point here..
> > > >
> > > > How do you get to CameraSensor::init() if your pipeline handler
> > > > acquires the unicam media device at the very beginning of match() ?
> > > >
> > > > I understand we allow multiple application acquire the same PH as the
> > > > ph might register multiple cameras, but why is this a problem at
> > > > match() time ?
> > > >
> > >
> > > In match(), the rpi, rkisp and ipu3 pipeline handlers call
> > > CameraSensor::init() as part of the Camera object registration.  This
> > call
> > > is unconditional assuming we can enumerate and register the sensor.
> > >
> >
> > Exactly, so CameraSensor::init() is called once, regardless of which
> > application will later "own" the camera. Sensor's controls are
> > reset indeed, is this the problem, that application do not expect to
> > have the controls reset ?
> >
>
> Ah, this is where our understanding may differ.  The enumeration code -
> match(), registerCamera() and CameraSensor::init() will be called for every
> (concurrent or single) libcamera process.
> This happens even when one of these processes has acquired and is actively
> running the camera, and in this case the other processes doing the
> enumeration will try to write to the sensor when they shouldn't from
> CameraSensor::init().
>
> Again, if this does not make sense or actually answer your question, please
> shout :-)
>

No it does, I was assuming a single libcamera instance running
and an upper layer arbitrator that mediates the camera access. But
that would be pipewire...

Sorry, it was trivial and this indeed is a valid use case.

More below


> Naush
>
>
> >
> > > Not sure if that answers your question though?
> > >
> > > Naush
> > >
> > >
> > >
> > > >
> > > > >
> > > > > These patches address just the flip bits and not the HBLANK, though I
> > > > > think the latter is a more straightforward problem. The flip bits are
> > > > > troublesome because they affect the reported Bayer orders.
> > > > >
> > > > > So here the idea is to store the formats in the sensor's native Bayer
> > > > > order, as we were doing before. Only we can't reset the flip bits
> > now,
> > > > > we have to query them and transform any Bayer formats the sensor
> > gives
> > > > > us back into the native order. Two patches implement this:
> > > > >
> > > > > 1. The first patch adds a function to turn BayerFormats back into
> > mbus
> > > > > codes. I couldn't see that we had functions to do this already, but
> > > > > please correct me if I missed something!
> > > > >

I don't see any helper for that, so the change is welcome

> > > > > 2. The second patch queries the flip bits and transforms any
> > > > > BayerFormats back into the native order to be stored.
> > > > >
> > > > > As I said, this potentially affects all pipeline handlers, which is
> > > > > why I left it "RFC" for the moment. I did take a look through them
> > all
> > > > > to see what we might need to do:
> > > > >
> > > > > raspberrypi.cpp
> > > > >
> > > > > I think this PH is OK. We assume we're given the native Bayer order
> > > > > and transform it according to the final sensor transform, setting the
> > > > > flip bits every time. We update the format in any raw stream to
> > match.
> > > > >
> > > > > uvcvideo.cpp, simple.cpp
> > > > >
> > > > > These appear to hardcode the transform to the Identity. I think
> > mostly
> > > > > they should be OK because the formats here won't be raw and you can't
> > > > > have raw streams (?). Probably they should clear the flip bits to

It is possible to have RAW streams with the simple pipeline
theoretically. Even more so considering the plan of plumbing a
sofware ISP/debayer to it

> > zero
> > > > > where these controls exist, perhaps someone who knows more about them
> > > > > could comment.
> > > > >
> > > > > rkisp1.cpp:
> > > > >
> > > > > Seems to force the transform to the Identity as well. So I think this
> > > > > is like the above ones, it should be OK though it probably needs to
> > > > > clear the sensor flip bits as well.
> > > > >
> > > > > ipu3.cpp:
> > > > >
> > > > > This one does handle transforms and appears to set the flip bits, so
> > > > > that's good. I couldn't see it transforming the Bayer order for raw
> > > > > streams correctly, so that probably wants checking. (To be fair, this
> > > > > looks suspect already and is not made worse by these changes!)
> > > > >
> > > > > Thoughts and opinions gratefully received, as always.

I'll reply to the patch

> > > > >
> > > > > Thanks!
> > > > > David
> > > > >
> > > > > David Plowman (2):
> > > > >   libcamera: bayer_format: Add toMbusCode method
> > > > >   libcamera: camera_sensor: Do not clear camera flips when listing
> > > > >     formats
> > > > >
> > > > >  include/libcamera/internal/bayer_format.h |  1 +
> > > > >  src/libcamera/bayer_format.cpp            | 12 ++++++
> > > > >  src/libcamera/camera_sensor.cpp           | 51
> > +++++++++++++++++++----
> > > > >  3 files changed, 56 insertions(+), 8 deletions(-)
> > > > >
> > > > > --
> > > > > 2.30.2
> > > > >
> > > >
> >