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

[David Plowman]

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
might work then.

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
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
> >