[libcamera-devel,v2,1/9] libcamera: camera_sensor: Cache rotationTransform_

18823 diff mbox series

The rotationTransform_ depends on a V4L2 control whose value does not
change for the whole lifetime of the camera.

Instead of re-calculating it everytime the camera is configured, cache
it at properties initialization time.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/libcamera/internal/camera_sensor.h |  1 +
 src/libcamera/camera_sensor.cpp            | 54 +++++++++++++---------
 2 files changed, 32 insertions(+), 23 deletions(-)

Hi Jacopo

Thanks for this patch set!

On Fri, 14 Jul 2023 at 15:16, Jacopo Mondi
<jacopo.mondi@ideasonboard.com> wrote:
>
> The rotationTransform_ depends on a V4L2 control whose value does not
> change for the whole lifetime of the camera.
>
> Instead of re-calculating it everytime the camera is configured, cache
> it at properties initialization time.
>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  include/libcamera/internal/camera_sensor.h |  1 +
>  src/libcamera/camera_sensor.cpp            | 54 +++++++++++++---------
>  2 files changed, 32 insertions(+), 23 deletions(-)
>
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index 02c77ab037da..02b4b4d25e6d 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -107,6 +107,7 @@ private:
>         Rectangle activeArea_;
>         const BayerFormat *bayerFormat_;
>         bool supportFlips_;
> +       Transform rotationTransform_;
>
>         ControlList properties_;
>
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index f3a5aa37149f..9a033459742f 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -461,18 +461,36 @@ int CameraSensor::initProperties()
>
>         const auto &rotationControl = controls.find(V4L2_CID_CAMERA_SENSOR_ROTATION);
>         if (rotationControl != controls.end()) {
> +               propertyValue = rotationControl->second.def().get<int32_t>();
> +
>                 /*
> -                * validateTransform() compensates for the mounting rotation.
> -                * However, as a camera sensor can only compensate rotations
> -                * by applying H/VFlips, only rotation of 180 degrees are
> -                * automatically compensated. The other valid rotations (Rot90
> -                * and Rot270) require transposition, which the camera sensor
> -                * cannot perform, so leave them untouched.
> +                * Cache the Transform associated with the camera mounting
> +                * rotation for later use in validateTransform().
> +                */
> +               bool success;
> +               rotationTransform_ = transformFromRotation(propertyValue, &success);
> +               if (!success) {
> +                       LOG(CameraSensor, Warning)
> +                               << "Invalid rotation of " << propertyValue
> +                               << " degrees - ignoring";
> +                       rotationTransform_ = Transform::Identity;
> +               }
> +
> +               /*
> +                * Adjust property::Rotation as validateTransform() compensates
> +                * for the mounting rotation. However, as a camera sensor can
> +                * only compensate rotations by applying H/VFlips, only rotation
> +                * of 180 degrees are automatically compensated. The other valid
> +                * rotations (Rot90 and Rot270) require transposition, which the
> +                * camera sensor cannot perform, so leave them untouched.

These extra comments didn't quite make sense to me. I believe that
they correctly describe the behaviour here even though a later patch
in this series changes it (as we agreed in Prague). So reading these
comments just left me a bit puzzled. But I don't think it matters very
much, so:

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

Thanks
David

>                  */
> -               propertyValue = rotationControl->second.def().get<int32_t>();
>                 if (propertyValue == 180 && supportFlips_)
>                         propertyValue = 0;
>                 properties_.set(properties::Rotation, propertyValue);
> +       } else {
> +               LOG(CameraSensor, Warning)
> +                       << "Rotation control not available, default to 0 degrees";
> +               rotationTransform_ = Transform::Identity;
>         }
>
>         properties_.set(properties::PixelArraySize, pixelArraySize_);
> @@ -1038,21 +1056,11 @@ void CameraSensor::updateControlInfo()
>   */
>  Transform CameraSensor::validateTransform(Transform *transform) const
>  {
> -       /* Adjust the requested transform to compensate the sensor mounting rotation. */
> -       const ControlInfoMap &controls = subdev_->controls();
> -       int rotation = 0;
> -
> -       const auto &rotationControl = controls.find(V4L2_CID_CAMERA_SENSOR_ROTATION);
> -       if (rotationControl != controls.end())
> -               rotation = rotationControl->second.def().get<int32_t>();
> -
> -       bool success;
> -       Transform rotationTransform = transformFromRotation(rotation, &success);
> -       if (!success)
> -               LOG(CameraSensor, Warning) << "Invalid rotation of " << rotation
> -                                          << " degrees - ignoring";
> -
> -       Transform combined = *transform * rotationTransform;
> +       /*
> +        * Combine the requested transform to compensate the sensor mounting
> +        * rotation.
> +        */
> +       Transform combined = *transform * rotationTransform_;
>
>         /*
>          * We combine the platform and user transform, but must "adjust away"
> @@ -1083,7 +1091,7 @@ Transform CameraSensor::validateTransform(Transform *transform) const
>                  * rise to this is the inverse of the rotation. (Recall that
>                  * combined = transform * rotationTransform.)
>                  */
> -               *transform = -rotationTransform;
> +               *transform = -rotationTransform_;
>                 combined = Transform::Identity;
>         }
>
> --
> 2.40.1
>

Hi David

On Mon, Jul 17, 2023 at 10:12:15AM +0100, David Plowman via libcamera-devel wrote:
> Hi Jacopo
>
> Thanks for this patch set!
>
> On Fri, 14 Jul 2023 at 15:16, Jacopo Mondi
> <jacopo.mondi@ideasonboard.com> wrote:
> >
> > The rotationTransform_ depends on a V4L2 control whose value does not
> > change for the whole lifetime of the camera.
> >
> > Instead of re-calculating it everytime the camera is configured, cache
> > it at properties initialization time.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  include/libcamera/internal/camera_sensor.h |  1 +
> >  src/libcamera/camera_sensor.cpp            | 54 +++++++++++++---------
> >  2 files changed, 32 insertions(+), 23 deletions(-)
> >
> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > index 02c77ab037da..02b4b4d25e6d 100644
> > --- a/include/libcamera/internal/camera_sensor.h
> > +++ b/include/libcamera/internal/camera_sensor.h
> > @@ -107,6 +107,7 @@ private:
> >         Rectangle activeArea_;
> >         const BayerFormat *bayerFormat_;
> >         bool supportFlips_;
> > +       Transform rotationTransform_;
> >
> >         ControlList properties_;
> >
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index f3a5aa37149f..9a033459742f 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -461,18 +461,36 @@ int CameraSensor::initProperties()
> >
> >         const auto &rotationControl = controls.find(V4L2_CID_CAMERA_SENSOR_ROTATION);
> >         if (rotationControl != controls.end()) {
> > +               propertyValue = rotationControl->second.def().get<int32_t>();
> > +
> >                 /*
> > -                * validateTransform() compensates for the mounting rotation.
> > -                * However, as a camera sensor can only compensate rotations
> > -                * by applying H/VFlips, only rotation of 180 degrees are
> > -                * automatically compensated. The other valid rotations (Rot90
> > -                * and Rot270) require transposition, which the camera sensor
> > -                * cannot perform, so leave them untouched.
> > +                * Cache the Transform associated with the camera mounting
> > +                * rotation for later use in validateTransform().
> > +                */
> > +               bool success;
> > +               rotationTransform_ = transformFromRotation(propertyValue, &success);
> > +               if (!success) {
> > +                       LOG(CameraSensor, Warning)
> > +                               << "Invalid rotation of " << propertyValue
> > +                               << " degrees - ignoring";
> > +                       rotationTransform_ = Transform::Identity;
> > +               }
> > +
> > +               /*
> > +                * Adjust property::Rotation as validateTransform() compensates
> > +                * for the mounting rotation. However, as a camera sensor can
> > +                * only compensate rotations by applying H/VFlips, only rotation
> > +                * of 180 degrees are automatically compensated. The other valid
> > +                * rotations (Rot90 and Rot270) require transposition, which the
> > +                * camera sensor cannot perform, so leave them untouched.
>
> These extra comments didn't quite make sense to me. I believe that
> they correctly describe the behaviour here even though a later patch

Yes, they describe the current behaviour in this patch.

> in this series changes it (as we agreed in Prague). So reading these
> comments just left me a bit puzzled. But I don't think it matters very
> much, so:

They describe the current behaviour that will later be changed.
Remember this patch was already part of the previous series where the
mounting rotation auto-compensation was not meant to be removed.

As the comment is anyway correct for the status at this patch, I
didn't think it was necessary removing it.

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

Thanks
  j

>
> Thanks
> David
>
> >                  */
> > -               propertyValue = rotationControl->second.def().get<int32_t>();
> >                 if (propertyValue == 180 && supportFlips_)
> >                         propertyValue = 0;
> >                 properties_.set(properties::Rotation, propertyValue);
> > +       } else {
> > +               LOG(CameraSensor, Warning)
> > +                       << "Rotation control not available, default to 0 degrees";
> > +               rotationTransform_ = Transform::Identity;
> >         }
> >
> >         properties_.set(properties::PixelArraySize, pixelArraySize_);
> > @@ -1038,21 +1056,11 @@ void CameraSensor::updateControlInfo()
> >   */
> >  Transform CameraSensor::validateTransform(Transform *transform) const
> >  {
> > -       /* Adjust the requested transform to compensate the sensor mounting rotation. */
> > -       const ControlInfoMap &controls = subdev_->controls();
> > -       int rotation = 0;
> > -
> > -       const auto &rotationControl = controls.find(V4L2_CID_CAMERA_SENSOR_ROTATION);
> > -       if (rotationControl != controls.end())
> > -               rotation = rotationControl->second.def().get<int32_t>();
> > -
> > -       bool success;
> > -       Transform rotationTransform = transformFromRotation(rotation, &success);
> > -       if (!success)
> > -               LOG(CameraSensor, Warning) << "Invalid rotation of " << rotation
> > -                                          << " degrees - ignoring";
> > -
> > -       Transform combined = *transform * rotationTransform;
> > +       /*
> > +        * Combine the requested transform to compensate the sensor mounting
> > +        * rotation.
> > +        */
> > +       Transform combined = *transform * rotationTransform_;
> >
> >         /*
> >          * We combine the platform and user transform, but must "adjust away"
> > @@ -1083,7 +1091,7 @@ Transform CameraSensor::validateTransform(Transform *transform) const
> >                  * rise to this is the inverse of the rotation. (Recall that
> >                  * combined = transform * rotationTransform.)
> >                  */
> > -               *transform = -rotationTransform;
> > +               *transform = -rotationTransform_;
> >                 combined = Transform::Identity;
> >         }
> >
> > --
> > 2.40.1
> >