[libcamera-devel] [PATCH 4/6] libcamera: pipeline: uvcvideo: Set device's flip controls correctly

[Jacopo Mondi]

Hi David,

On Thu, Nov 17, 2022 at 01:02:29PM +0000, David Plowman wrote:
> Hi Jacopo
>
> Thanks for the comments!
>
> On Thu, 17 Nov 2022 at 10:32, Jacopo Mondi <jacopo at jmondi.org> wrote:
> >
> > Hi David
> >
> > On Thu, Nov 10, 2022 at 02:45:54PM +0000, David Plowman via libcamera-devel wrote:
> > > Set the horizontal and vertical flip controls correctly by
> > > calling the device's setTransform method now that this no longer
> > > happens in CameraSensor::init().
> > >
> > > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > > ---
> > >  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > index 277465b7..4a891c23 100644
> > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > @@ -209,6 +209,9 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config)
> > >       StreamConfiguration &cfg = config->at(0);
> > >       int ret;
> > >
> > > +     /* Set up the video device's horizontal and vertical flips. */
> > > +     data->video_->setTransform(config->transform);
> > > +
> >
> > Do we need this ? The validate() function does
> >
> >         if (transform != Transform::Identity) {
> >                 transform = Transform::Identity;
> >                 status = Adjusted;
> >         }
> >
> > Same for simple and rkisp1.
> >
> > This opens a different question. What happens if another application
> > sets a flip before libcamera is started ? Should we here instead
> > reset all flips to their default values ? Using the newly introduced
> > setTrasform(Identity) wouldn't be enough as it would not clean up any
> > externally set flip...
> >
> > What would the expected configuration from the application point of
> > view ? Whatever flip is externally set is kept, or all flips
> > are reset to their default ? Please note that "reset to their
> > defaults" != "clear any flip flag" as most drivers (and we discussed
> > this with Dave recently) do inizialize their flip controls to report
> > what is actually programmed in registers, and quite often sensors are
> > 180 degrees flipped by default:
> > https://lore.kernel.org/all/CAPY8ntCCpMZtG-eJQ8cgd_GO06ZDP32oRK8mGrZKGMU2W401Dg@mail.gmail.com/
> > This is one more reason to remove the "set flips to 0" in init.
> >
> > Do we need a V4L2Device::resetFlips() function instead ?
>
> That's all interesting. Indeed I see there's a problem with devices
> that might have non-zero defaults, or where someone else has changed
> them without us being aware.
>
> As regards uvcvideo, maybe the best thing is then just to *do
> nothing*? At least that preserves the current behaviour. Although the
> pipeline handler doesn't support flips, someone could actually set
> them externally and they would be preserved. If the pipeline handler
> ever does support flips (should it one day?) we can figure out how it

On "should it be one day": I presume so, but I can't tell if there are
uvc-specific reasons why this can't be made

> might work then.

I presume so. Again not a uvc expert

>
> Probably the same argument holds for the simple PH too?
>
> I wonder if rkisp1 is different. That uses the CameraSensor class, so
> previously I assume it was clearing the flip bits. Therefore resetting
> the transform (it's always the Identity) might be correct? If a change

It seems to me Simple uses CameraSensor as well, but yes, it's mostly
intended to work with YUV/RGB sensors. Nothing prevents to use RAW
sensors though. The idea is to have one day a GPU-based ISP
implementation that could be plugged to Simple, so RAW+Simple is a use
case we care about..

My concern is only about RAW sensors.

- Assume a sensor driver reports BGGR with both v4l2 flips set to 1 and both
  report V4L2_CTRL_FLAG_MODIFY_LAYOUT.
- With your patch we transform it to RGGB and report it as
  properties::ColorFilterArrangement (should we rename it btw?).
- If any other user changes on filp it changes the bayer components
  ordering too and applications might get confused ?

Is something we should be concerned ?

> of behaviour is necessary (actually handling the transforms would be
> ideal!), that's probably a separate piece of work.
>
> Thoughts?
>
> David
>
> >
> > >       V4L2DeviceFormat format;
> > >       format.fourcc = data->video_->toV4L2PixelFormat(cfg.pixelFormat);
> > >       format.size = cfg.size;
> > > --
> > > 2.30.2
> > >