[libcamera-devel,4/9] libcamera: camera_sensor: Verify flips support

17890 diff mbox series

During the camera sensor driver validation, verify if the sensor
supports horizontal and vertical flips and store a flag as
CameraSensor::supportFlips_ class member.

The flag will be later inspected when applying flips.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 include/libcamera/internal/camera_sensor.h |  1 +
 src/libcamera/camera_sensor.cpp            | 15 ++++++++++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

Hi Jacopo

Thanks for the patch.

On Thu, 24 Nov 2022 at 12:12, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> During the camera sensor driver validation, verify if the sensor
> supports horizontal and vertical flips and store a flag as
> CameraSensor::supportFlips_ class member.
>
> The flag will be later inspected when applying flips.
>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

I'm certainly fine with this, though I should perhaps add that when we
originally did this in the Pi PH we discussed a little bit whether a
camera might support H flips but not V, or vice versa. We slightly
passed over the question because it was "only the Pi", but maybe we
want to re-examine that?

There's also the question as to whether the flips modify the Bayer
order. Don't know if that's a thing we want to store here? (And
separately for H and V flips?)

But as I said previously, this all works for me, so:

Reviewed-by: David Plowman <david.plowman@raspberrypi.com>

Thanks!
David

> ---
>  include/libcamera/internal/camera_sensor.h |  1 +
>  src/libcamera/camera_sensor.cpp            | 15 ++++++++++++++-
>  2 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index b9f4d7867854..878f3c28a3c9 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -101,6 +101,7 @@ private:
>         Size pixelArraySize_;
>         Rectangle activeArea_;
>         const BayerFormat *bayerFormat_;
> +       bool supportFlips_;
>
>         ControlList properties_;
>
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index cbac9e7801ae..3afcbc482095 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -57,7 +57,8 @@ LOG_DEFINE_CATEGORY(CameraSensor)
>   */
>  CameraSensor::CameraSensor(const MediaEntity *entity)
>         : entity_(entity), pad_(UINT_MAX), staticProps_(nullptr),
> -         bayerFormat_(nullptr), properties_(properties::properties)
> +         bayerFormat_(nullptr), supportFlips_(false),
> +         properties_(properties::properties)
>  {
>  }
>
> @@ -271,6 +272,18 @@ int CameraSensor::validateSensorDriver()
>                 }
>         }
>
> +       /* Verify if sensor supports horizontal/vertical flips. */
> +       const struct v4l2_query_ext_ctrl *hflipInfo = subdev_->controlInfo(V4L2_CID_HFLIP);
> +       const struct v4l2_query_ext_ctrl *vflipInfo = subdev_->controlInfo(V4L2_CID_VFLIP);
> +       if (hflipInfo && vflipInfo &&
> +           !(hflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY) &&
> +           !(vflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY))
> +               supportFlips_ = true;
> +
> +       if (!supportFlips_)
> +               LOG(CameraSensor, Warning)
> +                       << "Camera sensor does not support horizontal/vertical flip";
> +
>         /*
>          * Make sure the required selection targets are supported.
>          *
> --
> 2.38.1
>

Hi David

On Thu, Nov 24, 2022 at 01:43:34PM +0000, David Plowman wrote:
> Hi Jacopo
>
> Thanks for the patch.
>
> On Thu, 24 Nov 2022 at 12:12, Jacopo Mondi <jacopo@jmondi.org> wrote:
> >
> > During the camera sensor driver validation, verify if the sensor
> > supports horizontal and vertical flips and store a flag as
> > CameraSensor::supportFlips_ class member.
> >
> > The flag will be later inspected when applying flips.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>
> I'm certainly fine with this, though I should perhaps add that when we
> originally did this in the Pi PH we discussed a little bit whether a
> camera might support H flips but not V, or vice versa. We slightly
> passed over the question because it was "only the Pi", but maybe we
> want to re-examine that?

I think we should.

I recall I had a \todo somewhere in these patches to split H/V flip
handling, but seems like I removed it.

As this is what we have already I would do this on top, as it would
require to modify the newly introduced
CameraSensor::validateTransform() as well as the new logic in
setFormat().

>
> There's also the question as to whether the flips modify the Bayer
> order. Don't know if that's a thing we want to store here? (And
> separately for H and V flips?)

-Ideally- this should be transparent for us. What I mean is that,
assuming a sensor driver that behaves "correctly" with:

- RW CID_VFLIP/CID_HFLIP
- V4L2_CTRL_MODIFY_LAYOUT for both controls
- Updates the bayer pattern order when a new flip is applied

With your #2 patch we retrieve the "native" (no flips) Bayer order.
When a new format is set on the CameraSensor flips are applied before
calling V4L2Subdevice::setFormat() and the format returned from this
function contains the correct mbus code, and we use this to configure
the rest of the pipeline.

All of this assumes that the sensor is configured first, then the rest
of the pipeline, but I think it's a fair assumption, and it's not
different than what happens with the image format.

Speaking of this: I saw this on your pipeline handler validate()

        /*
         * Some sensors change their Bayer order when they are
         * h-flipped or v-flipped, according to the transform.
         * If this one does, we must advertise the transformed
         * Bayer order in the raw stream. Note how we must
         * fetch the "native" (i.e. untransformed) Bayer order,
         * because the sensor may currently be flipped!
         */
        V4L2PixelFormat fourcc = unicamFormat.fourcc;
        if (data_->flipsAlterBayerOrder_) {
                BayerFormat bayer = BayerFormat::fromV4L2PixelFormat(fourcc);
                bayer.order = data_->nativeBayerOrder_;
                bayer = bayer.transform(combinedTransform_);
                fourcc = bayer.toV4L2PixelFormat();
        }

That's probably something we want on the other pipelines too, and I
would like to avoid caching in each pipeline flipsAlterBayerOrder_ and
nativeBayerOrder_ and delegate this to some shared helper, again in
the CameraSensor class ?

>
> But as I said previously, this all works for me, so:
>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>

Thanks
  j

>
> Thanks!
> David
>
> > ---
> >  include/libcamera/internal/camera_sensor.h |  1 +
> >  src/libcamera/camera_sensor.cpp            | 15 ++++++++++++++-
> >  2 files changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > index b9f4d7867854..878f3c28a3c9 100644
> > --- a/include/libcamera/internal/camera_sensor.h
> > +++ b/include/libcamera/internal/camera_sensor.h
> > @@ -101,6 +101,7 @@ private:
> >         Size pixelArraySize_;
> >         Rectangle activeArea_;
> >         const BayerFormat *bayerFormat_;
> > +       bool supportFlips_;
> >
> >         ControlList properties_;
> >
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index cbac9e7801ae..3afcbc482095 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -57,7 +57,8 @@ LOG_DEFINE_CATEGORY(CameraSensor)
> >   */
> >  CameraSensor::CameraSensor(const MediaEntity *entity)
> >         : entity_(entity), pad_(UINT_MAX), staticProps_(nullptr),
> > -         bayerFormat_(nullptr), properties_(properties::properties)
> > +         bayerFormat_(nullptr), supportFlips_(false),
> > +         properties_(properties::properties)
> >  {
> >  }
> >
> > @@ -271,6 +272,18 @@ int CameraSensor::validateSensorDriver()
> >                 }
> >         }
> >
> > +       /* Verify if sensor supports horizontal/vertical flips. */
> > +       const struct v4l2_query_ext_ctrl *hflipInfo = subdev_->controlInfo(V4L2_CID_HFLIP);
> > +       const struct v4l2_query_ext_ctrl *vflipInfo = subdev_->controlInfo(V4L2_CID_VFLIP);
> > +       if (hflipInfo && vflipInfo &&
> > +           !(hflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY) &&
> > +           !(vflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY))
> > +               supportFlips_ = true;
> > +
> > +       if (!supportFlips_)
> > +               LOG(CameraSensor, Warning)
> > +                       << "Camera sensor does not support horizontal/vertical flip";
> > +
> >         /*
> >          * Make sure the required selection targets are supported.
> >          *
> > --
> > 2.38.1
> >