[libcamera-devel,5/9] libcamera: camera_sensor: Validate Transform

17891 diff mbox series

The two pipeline handlers that currently support Transform do so by
operating H/V flips on the image sensor, instead of rotating on the ISP.

As the image sensor performs the actual rotation, centralize the code
that validates a requested Transform against the camera sensor rotation
and capabilities in the CameraSensor class.

The implementation in the IPU3 pipeline handler was copied from the
RaspberryPi implementation, and is now centralized in CameraSensor to
make it easier for other platforms that do not rotate on the ISP to
implement support for Transform using the CameraSensor class.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 include/libcamera/internal/camera_sensor.h    |  4 ++
 src/libcamera/camera_sensor.cpp               | 59 +++++++++++++++++++
 src/libcamera/pipeline/ipu3/ipu3.cpp          | 45 ++------------
 .../pipeline/raspberrypi/raspberrypi.cpp      | 59 +++----------------
 4 files changed, 76 insertions(+), 91 deletions(-)

Hi Jacopo

Thank you for this patch.

On Thu, 24 Nov 2022 at 12:12, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> The two pipeline handlers that currently support Transform do so by
> operating H/V flips on the image sensor, instead of rotating on the ISP.
>
> As the image sensor performs the actual rotation, centralize the code
> that validates a requested Transform against the camera sensor rotation
> and capabilities in the CameraSensor class.
>
> The implementation in the IPU3 pipeline handler was copied from the
> RaspberryPi implementation, and is now centralized in CameraSensor to
> make it easier for other platforms that do not rotate on the ISP to
> implement support for Transform using the CameraSensor class.
>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  include/libcamera/internal/camera_sensor.h    |  4 ++
>  src/libcamera/camera_sensor.cpp               | 59 +++++++++++++++++++
>  src/libcamera/pipeline/ipu3/ipu3.cpp          | 45 ++------------
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 59 +++----------------
>  4 files changed, 76 insertions(+), 91 deletions(-)
>
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index 878f3c28a3c9..bea52badaff7 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -29,6 +29,8 @@ class BayerFormat;
>  class CameraLens;
>  class MediaEntity;
>
> +enum class Transform;
> +
>  struct CameraSensorProperties;
>
>  class CameraSensor : protected Loggable
> @@ -68,6 +70,8 @@ public:
>
>         CameraLens *focusLens() { return focusLens_.get(); }
>
> +       Transform validateTransform(Transform *transform) const;
> +
>  protected:
>         std::string logPrefix() const override;
>
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 3afcbc482095..a4ad0ba9a099 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -982,6 +982,65 @@ void CameraSensor::updateControlInfo()
>   * connected to the sensor
>   */
>
> +/**
> + * \brief Validate a transform request against the sensor capabilities
> + * \param[inout] transform The requested transformation, updated to match
> + * the sensor capabilities
> + *
> + * The requested \a transform is adjusted against the sensor rotation and its
> + * capabilities.
> + *
> + * In example, if the requested \a transform is Transform::Identity and the

s/In example/For example/ reads better.

> + * sensor rotation is 180 degrees, the resulting transform returned by the
> + * function is Transform::Rot180 to automatically correct the image, but only if
> + * the sensor can actually apply horizontal and vertical flips.
> + *
> + * \return A Transform instance that represents how \a transform is applied to
> + * the camera sensor
> + */
> +Transform CameraSensor::validateTransform(Transform *transform) const

Teeny nit-pick, but I wondered if another signature might be more
convenient for callers. For example

Status CameraSensor::validateTransform(Transform &transform, Transform
&combined)

It would return "Adjusted" if the "transform out" is different from
the "transform in". But I'm not terribly bothered!!

> +{
> +       /* Adjust the requested transform to the sensor rotation. */
> +       int32_t rotation = properties().get(properties::Rotation).value_or(0);
> +       bool success;
> +
> +       Transform rotationTransform = transformFromRotation(rotation, &success);
> +       if (!success)
> +               LOG(CameraSensor, Warning) << "Invalid rotation of " << rotation
> +                                          << " degrees - ignoring";
> +
> +       Transform combined = *transform * rotationTransform;

I know I must have written this, but now I'm wondering if
"rotationTransform" here should be "-rotationTransform". I guess it
never matters with our cameras which are always 0 or 180 degrees
rotated, but maybe in other cases...?

> +
> +       /*
> +        * The camera sensor cannot do Transpose. Adjust any combined result
> +        * that includes a transpose by flipping the transpose bit to notify
> +        * applications they either get the transform they requested, or have
> +        * to do a simple transpose themselves (they don't have to worry about
> +        * the other possible cases).
> +        */
> +       if (!!(combined & Transform::Transpose)) {
> +               /*
> +                * Flipping the transpose bit in "transform" flips it in the
> +                * combined result too (as it's the last thing that happens),
> +                * which is of course clearing it.
> +                */
> +               *transform ^= Transform::Transpose;
> +               combined &= ~Transform::Transpose;
> +       }
> +
> +       /*
> +        * If the sensor can do no transforms, then combined must be changed to
> +        * the identity and the sensor rotation must be cleared from the
> +        * requested "transform".

Don't really follow this comment. Maybe

        * If the sensor cannot do transforms, then combined must be changed to
        * the identity and the only transform the user can have is the sensor
        * rotation itself.

(Which rather suggests the code following has got the transform the
wrong way round...)

> +        */
> +       if (!supportFlips_ && !!combined) {
> +               *transform = -rotationTransform;

This might really be "rotationTransform" (without the inverse).
Depending on which we round we view the sensor rotation (from in front
of or behind the camera).

Also, as we remarked elsewhere, we should possibly consider the
existence of H and V flips separately.

But apart from my existential doubts about inverse rotations, it all
seems reasonable to me, though I sense we'll be coming back to this
again anyway:

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

Thanks!
David


> +               combined = Transform::Identity;
> +       }
> +
> +       return combined;
> +}
> +
>  std::string CameraSensor::logPrefix() const
>  {
>         return "'" + entity_->name() + "'";
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index e4d79ea44aed..a424ac914859 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -184,48 +184,15 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>         if (config_.empty())
>                 return Invalid;
>
> -       Transform combined = transform * data_->rotationTransform_;
> -
> -       /*
> -        * We combine the platform and user transform, but must "adjust away"
> -        * any combined result that includes a transposition, as we can't do
> -        * those. In this case, flipping only the transpose bit is helpful to
> -        * applications - they either get the transform they requested, or have
> -        * to do a simple transpose themselves (they don't have to worry about
> -        * the other possible cases).
> -        */
> -       if (!!(combined & Transform::Transpose)) {
> -               /*
> -                * Flipping the transpose bit in "transform" flips it in the
> -                * combined result too (as it's the last thing that happens),
> -                * which is of course clearing it.
> -                */
> -               transform ^= Transform::Transpose;
> -               combined &= ~Transform::Transpose;
> -               status = Adjusted;
> -       }
> -
>         /*
> -        * We also check if the sensor doesn't do h/vflips at all, in which
> -        * case we clear them, and the application will have to do everything.
> +        * Validate the requested transform against the sensor capabilities and
> +        * rotation and store the final combined transform that configure() will
> +        * need to apply to the sensor to save us working it out again.
>          */
> -       if (!data_->supportsFlips_ && !!combined) {
> -               /*
> -                * If the sensor can do no transforms, then combined must be
> -                * changed to the identity. The only user transform that gives
> -                * rise to this is the inverse of the rotation. (Recall that
> -                * combined = transform * rotationTransform.)
> -                */
> -               transform = -data_->rotationTransform_;
> -               combined = Transform::Identity;
> +       Transform requestedTransform = transform;
> +       combinedTransform_ = data_->cio2_.sensor()->validateTransform(&transform);
> +       if (transform != requestedTransform)
>                 status = Adjusted;
> -       }
> -
> -       /*
> -        * Store the final combined transform that configure() will need to
> -        * apply to the sensor to save us working it out again.
> -        */
> -       combinedTransform_ = combined;
>
>         /* Cap the number of entries to the available streams. */
>         if (config_.size() > kMaxStreams) {
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 087c71b65700..e83984985bce 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -366,59 +366,14 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
>         status = validateColorSpaces(ColorSpaceFlag::StreamsShareColorSpace);
>
>         /*
> -        * What if the platform has a non-90 degree rotation? We can't even
> -        * "adjust" the configuration and carry on. Alternatively, raising an
> -        * error means the platform can never run. Let's just print a warning
> -        * and continue regardless; the rotation is effectively set to zero.
> +        * Validate the requested transform against the sensor capabilities and
> +        * rotation and store the final combined transform that configure() will
> +        * need to apply to the sensor to save us working it out again.
>          */
> -       int32_t rotation = data_->sensor_->properties().get(properties::Rotation).value_or(0);
> -       bool success;
> -       Transform rotationTransform = transformFromRotation(rotation, &success);
> -       if (!success)
> -               LOG(RPI, Warning) << "Invalid rotation of " << rotation
> -                                 << " degrees - ignoring";
> -       Transform combined = transform * rotationTransform;
> -
> -       /*
> -        * We combine the platform and user transform, but must "adjust away"
> -        * any combined result that includes a transform, as we can't do those.
> -        * In this case, flipping only the transpose bit is helpful to
> -        * applications - they either get the transform they requested, or have
> -        * to do a simple transpose themselves (they don't have to worry about
> -        * the other possible cases).
> -        */
> -       if (!!(combined & Transform::Transpose)) {
> -               /*
> -                * Flipping the transpose bit in "transform" flips it in the
> -                * combined result too (as it's the last thing that happens),
> -                * which is of course clearing it.
> -                */
> -               transform ^= Transform::Transpose;
> -               combined &= ~Transform::Transpose;
> -               status = Adjusted;
> -       }
> -
> -       /*
> -        * We also check if the sensor doesn't do h/vflips at all, in which
> -        * case we clear them, and the application will have to do everything.
> -        */
> -       if (!data_->supportsFlips_ && !!combined) {
> -               /*
> -                * If the sensor can do no transforms, then combined must be
> -                * changed to the identity. The only user transform that gives
> -                * rise to this the inverse of the rotation. (Recall that
> -                * combined = transform * rotationTransform.)
> -                */
> -               transform = -rotationTransform;
> -               combined = Transform::Identity;
> +       Transform requestedTransform = transform;
> +       combinedTransform_ = data_->sensor_->validateTransform(&transform);
> +       if (transform != requestedTransform)
>                 status = Adjusted;
> -       }
> -
> -       /*
> -        * Store the final combined transform that configure() will need to
> -        * apply to the sensor to save us working it out again.
> -        */
> -       combinedTransform_ = combined;
>
>         unsigned int rawCount = 0, outCount = 0, count = 0, maxIndex = 0;
>         std::pair<int, Size> outSize[2];
> @@ -453,7 +408,7 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
>                         if (data_->flipsAlterBayerOrder_) {
>                                 BayerFormat bayer = BayerFormat::fromV4L2PixelFormat(fourcc);
>                                 bayer.order = data_->nativeBayerOrder_;
> -                               bayer = bayer.transform(combined);
> +                               bayer = bayer.transform(combinedTransform_);
>                                 fourcc = bayer.toV4L2PixelFormat();
>                         }
>
> --
> 2.38.1
>

Hi!

I'd love if we could clarify the following part to ensure the Pipewire 
implementation is correct:

> > +
> > +       /*
> > +        * The camera sensor cannot do Transpose. Adjust any combined result
> > +        * that includes a transpose by flipping the transpose bit to notify
> > +        * applications they either get the transform they requested, or have
> > +        * to do a simple transpose themselves (they don't have to worry about
> > +        * the other possible cases).
> > +        */
> > +       if (!!(combined & Transform::Transpose)) {
> > +               /*
> > +                * Flipping the transpose bit in "transform" flips it in the
> > +                * combined result too (as it's the last thing that happens),
> > +                * which is of course clearing it.
> > +                */
> > +               *transform ^= Transform::Transpose;
> > +               combined &= ~Transform::Transpose;
> > +       }
> > +
> > +       /*
> > +        * If the sensor can do no transforms, then combined must be changed to
> > +        * the identity and the sensor rotation must be cleared from the
> > +        * requested "transform".
>
> Don't really follow this comment. Maybe
>
>          * If the sensor cannot do transforms, then combined must be changed to
>          * the identity and the only transform the user can have is the sensor
>          * rotation itself.
>
> (Which rather suggests the code following has got the transform the
> wrong way round...)
>
> > +        */
> > +       if (!supportFlips_ && !!combined) {
> > +               *transform = -rotationTransform;
>
> This might really be "rotationTransform" (without the inverse).
> Depending on which we round we view the sensor rotation (from in front
> of or behind the camera).
>
> Also, as we remarked elsewhere, we should possibly consider the
> existence of H and V flips separately.
>
> But apart from my existential doubts about inverse rotations, it all
> seems reasonable to me, though I sense we'll be coming back to this
> again anyway:

As example I'd like to take the back-camera of the Pinephone Pro 
(imx258). Pipewire will ask for an identity transform and the kernel 
will most likely report `V4L2_CID_CAMERA_SENSOR_ROTATION == 270` (once 
driver patches and device tree updates land), so the initial values 
we'll get are  `properties::Rotation == 270` -> `combined == 
Transform::Rot270`.

Right now neither transpose nor flips are supported, so the steps in the 
current revision are:

 1. `combined == rot270` / `transform == identity`
 2. `combined == hflip` / `transform == transpose`
 3. `combined == `identity` / `transform == rot90`

The value from `transform` will then be used for 
`CameraConfiguration::transform`, signalling to the client that it will 
have to compensate accordingly. Following the comment to not use the 
inverse it would be `rot270` instead.

 From an API-point of view, I'd say `rot90` is the more intuitive value 
for clients, because they can then just follow the documentation in 
camera_sensor.cpp and apply a transformation accordingly. Would you 
agree that this should be the desired outcome? I.e. should we stick to 
the proposed value?

Best regards,

Robert

Hi Robert

Thanks for prodding us all on this question!

On Wed, 7 Dec 2022 at 12:32, Robert Mader via libcamera-devel
<libcamera-devel@lists.libcamera.org> wrote:
>
> Hi!
>
> I'd love if we could clarify the following part to ensure the Pipewire implementation is correct:
>
> > +
> > +       /*
> > +        * The camera sensor cannot do Transpose. Adjust any combined result
> > +        * that includes a transpose by flipping the transpose bit to notify
> > +        * applications they either get the transform they requested, or have
> > +        * to do a simple transpose themselves (they don't have to worry about
> > +        * the other possible cases).
> > +        */
> > +       if (!!(combined & Transform::Transpose)) {
> > +               /*
> > +                * Flipping the transpose bit in "transform" flips it in the
> > +                * combined result too (as it's the last thing that happens),
> > +                * which is of course clearing it.
> > +                */
> > +               *transform ^= Transform::Transpose;
> > +               combined &= ~Transform::Transpose;
> > +       }
> > +
> > +       /*
> > +        * If the sensor can do no transforms, then combined must be changed to
> > +        * the identity and the sensor rotation must be cleared from the
> > +        * requested "transform".
>
> Don't really follow this comment. Maybe
>
>         * If the sensor cannot do transforms, then combined must be changed to
>         * the identity and the only transform the user can have is the sensor
>         * rotation itself.
>
> (Which rather suggests the code following has got the transform the
> wrong way round...)
>
> > +        */
> > +       if (!supportFlips_ && !!combined) {
> > +               *transform = -rotationTransform;
>
> This might really be "rotationTransform" (without the inverse).
> Depending on which we round we view the sensor rotation (from in front
> of or behind the camera).
>
> Also, as we remarked elsewhere, we should possibly consider the
> existence of H and V flips separately.
>
> But apart from my existential doubts about inverse rotations, it all
> seems reasonable to me, though I sense we'll be coming back to this
> again anyway:
>
> As example I'd like to take the back-camera of the Pinephone Pro (imx258). Pipewire will ask for an identity transform and the kernel will most likely report `V4L2_CID_CAMERA_SENSOR_ROTATION == 270` (once driver patches and device tree updates land), so the initial values we'll get are  `properties::Rotation == 270` -> `combined == Transform::Rot270`.
>
> Right now neither transpose nor flips are supported, so the steps in the current revision are:
>
> `combined == rot270` / `transform == identity`
> `combined == hflip` / `transform == transpose`
> `combined == `identity` / `transform == rot90`
>
> The value from `transform` will then be used for `CameraConfiguration::transform`, signalling to the client that it will have to compensate accordingly. Following the comment to not use the inverse it would be `rot270` instead.
>
> From an API-point of view, I'd say `rot90` is the more intuitive value for clients, because they can then just follow the documentation in camera_sensor.cpp and apply a

(What document is that? It wasn't immediately clear to me...)

> transformation accordingly. Would you agree that this should be the desired outcome? I.e. should we stick to the proposed value?

I looked back over this again, and found Jacopo's description of the
V4L2_CID_CAMERA_SENSOR_ROTATION control
(https://patchwork.kernel.org/project/linux-media/patch/20191204091056.4842-5-jacopo@jmondi.org/
is what I stumbled into)

From that, and the nice example with the stick person, I deduce that
if your camera's V4L2_CID_CAMERA_SENSOR_ROTATION property says 90
degrees, then the image you capture (if nothing else happens to it)
will look like it has had a 90 degree clockwise rotation performed.
Everyone agree with that?

So in turn, if you have a camera/ISP that can apply no transforms at
all, then the only value for the user transform that will not come
back as adjusted is "rot90", because it looks like it's had a 90
degree clockwise rotation performed (and libcamera transforms count
rotations as being clockwise). Does that also make sense?

So by a slightly arbitrary mixture of conventions, it looks to me as
though the use of the inverse that I queried is indeed wrong, because
we "need camera 90 degree rotation" => "user transform rot90" in this
case.

Does that sound plausible... what do folks think?

Thanks everyone!
David

>
> Best regards,
>
> Robert

On 07.12.22 15:28, David Plowman via libcamera-devel wrote:
> Hi Robert
>
> Thanks for prodding us all on this question!

Hi, thanks for the quick reply! And a pleasure :P

> I looked back over this again, and found Jacopo's description of the
> V4L2_CID_CAMERA_SENSOR_ROTATION control
> (https://patchwork.kernel.org/project/linux-media/patch/20191204091056.4842-5-jacopo@jmondi.org/
> is what I stumbled into)

The current kernel docu can be found at

https://github.com/torvalds/linux/blob/8ed710da2873c2aeb3bb805864a699affaf1d03b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst

the libcamera one at

https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/transform.cpp#n54

>  From that, and the nice example with the stick person, I deduce that
> if your camera's V4L2_CID_CAMERA_SENSOR_ROTATION property says 90
> degrees, then the image you capture (if nothing else happens to it)
> will look like it has had a 90 degree clockwise rotation performed.
> Everyone agree with that?
Yep, agreed. That would mean V4L2_CID_CAMERA_SENSOR_ROTATION==90 would 
translate to Transform::Rot90.
> So in turn, if you have a camera/ISP that can apply no transforms at
> all, then the only value for the user transform that will not come
> back as adjusted is "rot90", because it looks like it's had a 90
> degree clockwise rotation performed (and libcamera transforms count
> rotations as being clockwise). Does that also make sense?
>
> So by a slightly arbitrary mixture of conventions, it looks to me as
> though the use of the inverse that I queried is indeed wrong, because
> we "need camera 90 degree rotation" => "user transform rot90" in this
> case.
>
> Does that sound plausible... what do folks think?

Ah, I think the question is whether the adjusted value means: "this is 
what you'll get" vs "this is what you have to do yourself".

Above I argued in favor of the later, but I guess the first one is what 
the API is supposed to mean. That would imply that the client has 
compute the difference between requested and adjusted value itself.

In my case that would mean:

requested CameraConfiguration::transform: Transform::Identity, 
V4L2_CID_CAMERA_SENSOR_ROTATION: 270, adjusted 
CameraConfiguration::transform: Transform::Rot270

To compute what the client has to do, it would need to do: 
Transform::Identity - Transform::Rot270 = Transform::Rot90. And then 
apply that, a 90 degr. rotation clockwise.

Does that sound reasonable? In that case the patch here should indeed be 
`*transform = rotationTransform;`, without the inverse.

> Thanks everyone!
> David

Thanks!

Robert

Hi,

On Wed, Dec 07, 2022 at 01:32:04PM +0100, Robert Mader via libcamera-devel wrote:
> Hi!
>
> I'd love if we could clarify the following part to ensure the Pipewire
> implementation is correct:
>
> > > +
> > > +       /*
> > > +        * The camera sensor cannot do Transpose. Adjust any combined result
> > > +        * that includes a transpose by flipping the transpose bit to notify
> > > +        * applications they either get the transform they requested, or have
> > > +        * to do a simple transpose themselves (they don't have to worry about
> > > +        * the other possible cases).
> > > +        */
> > > +       if (!!(combined & Transform::Transpose)) {
> > > +               /*
> > > +                * Flipping the transpose bit in "transform" flips it in the
> > > +                * combined result too (as it's the last thing that happens),
> > > +                * which is of course clearing it.
> > > +                */
> > > +               *transform ^= Transform::Transpose;
> > > +               combined &= ~Transform::Transpose;
> > > +       }
> > > +
> > > +       /*
> > > +        * If the sensor can do no transforms, then combined must be changed to
> > > +        * the identity and the sensor rotation must be cleared from the
> > > +        * requested "transform".
> >
> > Don't really follow this comment. Maybe
> >
> >          * If the sensor cannot do transforms, then combined must be changed to
> >          * the identity and the only transform the user can have is the sensor
> >          * rotation itself.
> >
> > (Which rather suggests the code following has got the transform the
> > wrong way round...)
> >

I should have replied earlier to David on the patch series for this
one.

What I actually meant is "If the sensor cannot do any flip" and the
code has been copied from
https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp#n402

Will reply to rest of the discussion below in a separate email.

> > > +        */
> > > +       if (!supportFlips_ && !!combined) {
> > > +               *transform = -rotationTransform;
> >
> > This might really be "rotationTransform" (without the inverse).
> > Depending on which we round we view the sensor rotation (from in front
> > of or behind the camera).
> >
> > Also, as we remarked elsewhere, we should possibly consider the
> > existence of H and V flips separately.
> >
> > But apart from my existential doubts about inverse rotations, it all
> > seems reasonable to me, though I sense we'll be coming back to this
> > again anyway:
>
> As example I'd like to take the back-camera of the Pinephone Pro (imx258).
> Pipewire will ask for an identity transform and the kernel will most likely
> report `V4L2_CID_CAMERA_SENSOR_ROTATION == 270` (once driver patches and
> device tree updates land), so the initial values we'll get are 
> `properties::Rotation == 270` -> `combined == Transform::Rot270`.
>
> Right now neither transpose nor flips are supported, so the steps in the
> current revision are:
>
> 1. `combined == rot270` / `transform == identity`
> 2. `combined == hflip` / `transform == transpose`
> 3. `combined == `identity` / `transform == rot90`
>
> The value from `transform` will then be used for
> `CameraConfiguration::transform`, signalling to the client that it will have
> to compensate accordingly. Following the comment to not use the inverse it
> would be `rot270` instead.
>
> From an API-point of view, I'd say `rot90` is the more intuitive value for
> clients, because they can then just follow the documentation in
> camera_sensor.cpp and apply a transformation accordingly. Would you agree
> that this should be the desired outcome? I.e. should we stick to the
> proposed value?
>
> Best regards,
>
> Robert

Hi again,

On Wed, Dec 07, 2022 at 03:48:40PM +0000, Robert Mader via libcamera-devel wrote:
> On 07.12.22 15:28, David Plowman via libcamera-devel wrote:
> > Hi Robert
> >
> > Thanks for prodding us all on this question!
>
> Hi, thanks for the quick reply! And a pleasure :P
>
> > I looked back over this again, and found Jacopo's description of the
> > V4L2_CID_CAMERA_SENSOR_ROTATION control
> > (https://patchwork.kernel.org/project/linux-media/patch/20191204091056.4842-5-jacopo@jmondi.org/
> > is what I stumbled into)
>
> The current kernel docu can be found at
>
> https://github.com/torvalds/linux/blob/8ed710da2873c2aeb3bb805864a699affaf1d03b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>
> the libcamera one at
>
> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/transform.cpp#n54
>
> >  From that, and the nice example with the stick person, I deduce that
> > if your camera's V4L2_CID_CAMERA_SENSOR_ROTATION property says 90
> > degrees, then the image you capture (if nothing else happens to it)
> > will look like it has had a 90 degree clockwise rotation performed.
> > Everyone agree with that?
> Yep, agreed. That would mean V4L2_CID_CAMERA_SENSOR_ROTATION==90 would
> translate to Transform::Rot90.

Let's here put some definitions in place

V4L2_CID_CAMERA_SENSOR_ROTATION
This read-only control describes the rotation correction in degrees in
the counter-clockwise direction to be applied to the captured images
once captured to memory to compensate for the camera sensor mounting
rotation.

 \var Transform::Rot270
 Rotation by 270 degrees clockwise (90 degrees anticlockwise).

 Let's start by saying we did a sub-optimal (to be nice) job in
 having Transform and V4L2_CID_ROTATION operate in two different
 directions. Before the usage of Transform in libcamera gets widely
 adopted sould we align the two ?

 Hence if I'm not mistaken, if your camera has Rotation=90 and an
 application supplies Transform::Identity in
 CameraConfiguration::transform what you should get back is Rot270

 Correct ?

> > So in turn, if you have a camera/ISP that can apply no transforms at
> > all, then the only value for the user transform that will not come
> > back as adjusted is "rot90", because it looks like it's had a 90
> > degree clockwise rotation performed (and libcamera transforms count
> > rotations as being clockwise). Does that also make sense?

It might be the case today, but I wonder if that's desirable.

If an application supplies CameraConfiguration::transform = Identity
it means it doesn't want any rotation applied by the library. It will
get back 270 to indicate that that's how the image is rotated and will
get an Adjusted configuration.

If instead an application supplies CameraConfiguration::transform = Rot270
and your camera -cannot- do any rotation, it will still receive back
Rot270 and the state is Valid.

If it asks for CameraConfiguration::transform = Rot270 and the camera
-can- do that, it will clear CameraConfiguration::transform to
Identity and return Adjusted ?

Is this what happens (I'm looking at the above code with David's
suggestion to use the inverse of roationTransform).


> >
> > So by a slightly arbitrary mixture of conventions, it looks to me as
> > though the use of the inverse that I queried is indeed wrong, because
> > we "need camera 90 degree rotation" => "user transform rot90" in this
> > case.
> >
> > Does that sound plausible... what do folks think?
>
> Ah, I think the question is whether the adjusted value means: "this is what
> you'll get" vs "this is what you have to do yourself".
>
> Above I argued in favor of the later, but I guess the first one is what the
> API is supposed to mean. That would imply that the client has compute the
> difference between requested and adjusted value itself.

I always presumed the latter was meant to happen..

>
> In my case that would mean:
>
> requested CameraConfiguration::transform: Transform::Identity,
> V4L2_CID_CAMERA_SENSOR_ROTATION: 270, adjusted
> CameraConfiguration::transform: Transform::Rot270
>

I guess it's Rot90 because of the clockwise/counter-clockwise issue ?

> To compute what the client has to do, it would need to do:
> Transform::Identity - Transform::Rot270 = Transform::Rot90. And then apply
> that, a 90 degr. rotation clockwise.
>
> Does that sound reasonable? In that case the patch here should indeed be
> `*transform = rotationTransform;`, without the inverse.
>

        /0\

before we plumb this into and make assumptions that will be then
set into stone in our adaption layers, can we take a step back and
define what kind of API we would like to have ?

Let's start by the definition of CameraConfiguration::transform.

It's a member of CameraConfiguration, and as by definition it is
filled-in by the application and eventually adjusted by the Camera to
a value that's acceptable/doable for the current configuration.

1) Application to Camera  = (counter?)clockwise degrees the camera
system should rotate the image before presenting it to the user

2) Camera to application = the rotation the camera can actually do
(ie. only sensor flips == no Transpose)

The current documentation doesn't explain how it gets adjusted

/**
 * \var CameraConfiguration::transform
 * \brief User-specified transform to be applied to the image
 *
 * The transform is a user-specified 2D plane transform that will be applied
 * to the camera images by the processing pipeline before being handed to
 * the application. This is subsequent to any transform that is already
 * required to fix up any platform-defined rotation.
 *
 * The usual 2D plane transforms are allowed here (horizontal/vertical
 * flips, multiple of 90-degree rotations etc.), but the validate() function
 * may adjust this field at its discretion if the selection is not supported.
 */

The last part in particular, "at its discrection" it's really too
generic.

Let's start from the beginning: what application should use transform
for ? I presume they should:

1) Inspect propertis::Rotation
2) Correct the image for the properties::Rotation amount (actually,
the inverse because of the clockwise/counter-clockwise thing)
3) Add any additional rotation they would like on top of this

How should transform be adjusted ?

Identity:
If application->identity the camera does not have to apply any
transform. Should the transform be adjutes like it happens today to
compensate the Rotation (at least that's what I presume happens) ? I
presume no, applications can infer rotation from properties and if they
chose Identity either other layers above will do the transform or
they're fine with images as they are.

A supported/non-supported transform:
An application asks for a VFLIP, the camera can do it, transform is
not changed. If the camera cannot flip, the transform is set back to
Identity and the state is adjusted.

A partially non-supported transform:
An application asks for Rot270 (Transpose | HFLIP) and the camera can
only do HFLIP. transform sould be then set to HFLIP and applications
know that the remaining transformation should be handled elsewhere.

Are we missing anything with such behaviour ? I presume it's rather
similar to what happens today if not for the fact that the validate()
implementation adjusts transform to rotation in case it cannot flip

        Transform combined = transform * data_->rotationTransform_;
        if (!data_->supportsFlips_ && !!combined)
                transform = -data_->rotationTransform_;

Not really sure I got why.

All of this, but an even more fundamental question: is it too late/is
it worh it to adjust the Transform definition to adopt the same
direction as the kernel defined rotation ?

Sorry for the long email :/

> > Thanks everyone!
> > David
>
> Thanks!
>
> Robert
>

Hi Jacopo

Thanks for wading into the sea on this question! (Very appropriate now
that the stick person example has been turned into a shark...!)

On Fri, 9 Dec 2022 at 12:47, Jacopo Mondi via libcamera-devel
<libcamera-devel@lists.libcamera.org> wrote:
>
> Hi again,
>
> On Wed, Dec 07, 2022 at 03:48:40PM +0000, Robert Mader via libcamera-devel wrote:
> > On 07.12.22 15:28, David Plowman via libcamera-devel wrote:
> > > Hi Robert
> > >
> > > Thanks for prodding us all on this question!
> >
> > Hi, thanks for the quick reply! And a pleasure :P
> >
> > > I looked back over this again, and found Jacopo's description of the
> > > V4L2_CID_CAMERA_SENSOR_ROTATION control
> > > (https://patchwork.kernel.org/project/linux-media/patch/20191204091056.4842-5-jacopo@jmondi.org/
> > > is what I stumbled into)
> >
> > The current kernel docu can be found at
> >
> > https://github.com/torvalds/linux/blob/8ed710da2873c2aeb3bb805864a699affaf1d03b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst

Thanks for the updated reference. The stick person may have turned
into a shark, but apart from that I think the sense of the
documentation is unchanged. So if V4L2_CID_CAMERA_SENSOR_ROTATION says
90 degrees, then your image in memory looks like it has had a 90
degree *clockwise* rotation applied.

> >
> > the libcamera one at
> >
> > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/transform.cpp#n54
> >
> > >  From that, and the nice example with the stick person, I deduce that
> > > if your camera's V4L2_CID_CAMERA_SENSOR_ROTATION property says 90
> > > degrees, then the image you capture (if nothing else happens to it)
> > > will look like it has had a 90 degree clockwise rotation performed.
> > > Everyone agree with that?
> > Yep, agreed. That would mean V4L2_CID_CAMERA_SENSOR_ROTATION==90 would
> > translate to Transform::Rot90.
>
> Let's here put some definitions in place
>
> V4L2_CID_CAMERA_SENSOR_ROTATION
> This read-only control describes the rotation correction in degrees in
> the counter-clockwise direction to be applied to the captured images
> once captured to memory to compensate for the camera sensor mounting
> rotation.
>
>  \var Transform::Rot270
>  Rotation by 270 degrees clockwise (90 degrees anticlockwise).
>
>  Let's start by saying we did a sub-optimal (to be nice) job in
>  having Transform and V4L2_CID_ROTATION operate in two different
>  directions. Before the usage of Transform in libcamera gets widely
>  adopted sould we align the two ?

I don't mind at all... we just have to decide what we want!

>
>  Hence if I'm not mistaken, if your camera has Rotation=90 and an
>  application supplies Transform::Identity in
>  CameraConfiguration::transform what you should get back is Rot270
>
>  Correct ?

The way things are currently, if the camera says Rotation=90, then
your image in memory looks like it is rotated 90 degrees clockwise, so
the Transform (if your pipeline and sensor can't apply any
flips/rotations at all) will come back as Rot90.

That is, the only transform you can specify that won't be adjusted, is
a 90 degree clockwise rotation, so transform=Rot90 as things stand
currently.

If we choose to change the sense of the libcamera.Transform rotation,
then we'd get transform=Rot270 instead.

 (If your pipeline handler can do flips and transposes, it may be able
to do what you ask and so the transform would come back unchanged and
would be "Valid".)

>
> > > So in turn, if you have a camera/ISP that can apply no transforms at
> > > all, then the only value for the user transform that will not come
> > > back as adjusted is "rot90", because it looks like it's had a 90
> > > degree clockwise rotation performed (and libcamera transforms count
> > > rotations as being clockwise). Does that also make sense?
>
> It might be the case today, but I wonder if that's desirable.
>
> If an application supplies CameraConfiguration::transform = Identity
> it means it doesn't want any rotation applied by the library. It will
> get back 270 to indicate that that's how the image is rotated and will
> get an Adjusted configuration.

I'm not quite sure I get that, perhaps it depends how we interpret
"applied by the library". I guess I agree if we mean "applied by the
library together with any rotation of the sensor"? Maybe a real
example is helpful:

So for a Raspberry Pi, for example, our sensors are mostly mounted
upside down (V4L2_CID_CAMERA_SENSOR_ROTATION says "180"). Our users
say the transform they want is transform=Identity i.e. images come
back "the right way up". Internally it means that we do apply an extra
h+v flip to undo the 180 degree rotation of the sensor.

>
> If instead an application supplies CameraConfiguration::transform = Rot270
> and your camera -cannot- do any rotation, it will still receive back
> Rot270 and the state is Valid.

I think that's true if we change the sense of the libcamera.Transform
rotation. Currently, as I said earlier, I think we get Rot90?

>
> If it asks for CameraConfiguration::transform = Rot270 and the camera
> -can- do that, it will clear CameraConfiguration::transform to
> Identity and return Adjusted ?

I'm not sure there... The CameraConfiguration::transform is the final
transform you want. If that can be done, it's left alone and you get
back Valid. It's not telling you what else you would need to do
afterwards.

(To find out what you need to do afterwards you'd work out
"inverse(what-you-will-get) * what-you-wanted", or something like
that.)

>
> Is this what happens (I'm looking at the above code with David's
> suggestion to use the inverse of roationTransform).
>
>
> > >
> > > So by a slightly arbitrary mixture of conventions, it looks to me as
> > > though the use of the inverse that I queried is indeed wrong, because
> > > we "need camera 90 degree rotation" => "user transform rot90" in this
> > > case.
> > >
> > > Does that sound plausible... what do folks think?
> >
> > Ah, I think the question is whether the adjusted value means: "this is what
> > you'll get" vs "this is what you have to do yourself".
> >
> > Above I argued in favor of the later, but I guess the first one is what the
> > API is supposed to mean. That would imply that the client has compute the
> > difference between requested and adjusted value itself.
>
> I always presumed the latter was meant to happen..

Yes, I think this is the crux of the matter. I believe it's "what you
will get". It's not "what you need to do after". (If it was "what you
need to do after", it would change every time you try to validate the
configuration!)

>
> >
> > In my case that would mean:
> >
> > requested CameraConfiguration::transform: Transform::Identity,
> > V4L2_CID_CAMERA_SENSOR_ROTATION: 270, adjusted
> > CameraConfiguration::transform: Transform::Rot270
> >
>
> I guess it's Rot90 because of the clockwise/counter-clockwise issue ?

Yes, looks right to me!

>
> > To compute what the client has to do, it would need to do:
> > Transform::Identity - Transform::Rot270 = Transform::Rot90. And then apply
> > that, a 90 degr. rotation clockwise.

Yes, I guess I wrote it above, it's "inverse(what-you-got) *
what-you-wanted", in this case inverse(rot270) * identity which is
rot90 (in the current clockwise sense).

> >
> > Does that sound reasonable? In that case the patch here should indeed be
> > `*transform = rotationTransform;`, without the inverse.
> >

Yes, agree!

>
>         /0\
>
> before we plumb this into and make assumptions that will be then
> set into stone in our adaption layers, can we take a step back and
> define what kind of API we would like to have ?
>
> Let's start by the definition of CameraConfiguration::transform.
>
> It's a member of CameraConfiguration, and as by definition it is
> filled-in by the application and eventually adjusted by the Camera to
> a value that's acceptable/doable for the current configuration.
>
> 1) Application to Camera  = (counter?)clockwise degrees the camera
> system should rotate the image before presenting it to the user

This may depend on what exactly we mean by "the camera system". I'd
explain it as
"the final resulting transform that the application wants to be
applied to all the output images". After all, this is actually the
only thing the application knows!

>
> 2) Camera to application = the rotation the camera can actually do
> (ie. only sensor flips == no Transpose)

Agree, mostly, though I would say "transform" rather than "rotation"
because flips aren't rotations, you could in theory have a PH that
could do transposes (even though we probably don't currently).

>
> The current documentation doesn't explain how it gets adjusted

This is true, though I also think it's platform dependent. If you had
a PH that can't do transposes but can do flips, you have a choice how
you handle (for example) rot90. You could say "oh I can't do that, so
in this case I won't do anything at all" and leave the application to
sort everything out. The Pi tries to say "well, I'll do everything
except for the transpose, so an application only has to worry about
that one case". But there is a choice...

>
> /**
>  * \var CameraConfiguration::transform
>  * \brief User-specified transform to be applied to the image
>  *
>  * The transform is a user-specified 2D plane transform that will be applied
>  * to the camera images by the processing pipeline before being handed to
>  * the application. This is subsequent to any transform that is already
>  * required to fix up any platform-defined rotation.
>  *
>  * The usual 2D plane transforms are allowed here (horizontal/vertical
>  * flips, multiple of 90-degree rotations etc.), but the validate() function
>  * may adjust this field at its discretion if the selection is not supported.
>  */
>
> The last part in particular, "at its discrection" it's really too
> generic.

I guess it's hinting at the fact that it's up to the PH, and we don't
mandate the exact way in which it can fail to do what you asked for!
But it would be fine to have a discussion on that.

>
> Let's start from the beginning: what application should use transform
> for ? I presume they should:
>
> 1) Inspect propertis::Rotation
> 2) Correct the image for the properties::Rotation amount (actually,
> the inverse because of the clockwise/counter-clockwise thing)
> 3) Add any additional rotation they would like on top of this

So I think the point of the transform is that you *don't* have to
worry about the properties::Rotation. It takes care of that for you.
You say "I want upside down images" and you get them, whatever the
sensor's rotation.

>
> How should transform be adjusted ?
>
> Identity:
> If application->identity the camera does not have to apply any
> transform. Should the transform be adjutes like it happens today to
> compensate the Rotation (at least that's what I presume happens) ? I
> presume no, applications can infer rotation from properties and if they
> chose Identity either other layers above will do the transform or
> they're fine with images as they are.

If the application says it wants the "identity" transform, then it
should get output images with no transformation applied. But this does
mean it will undo the sensor rotation (this being the normal case on
the Pi).

>
> A supported/non-supported transform:
> An application asks for a VFLIP, the camera can do it, transform is
> not changed. If the camera cannot flip, the transform is set back to
> Identity and the state is adjusted.

Yes, mostly, except that the sensor may be mounted with a rotation,
but if it isn't, then I agree with this.

>
> A partially non-supported transform:
> An application asks for Rot270 (Transpose | HFLIP) and the camera can
> only do HFLIP. transform sould be then set to HFLIP and applications
> know that the remaining transformation should be handled elsewhere.

As I suggested above, I'm not sure whether we want to mandate this. I
agree it seems useful behaviour, which is why the Pi does it, but I'm
open minded about requiring it or not.

>
> Are we missing anything with such behaviour ? I presume it's rather
> similar to what happens today if not for the fact that the validate()
> implementation adjusts transform to rotation in case it cannot flip
>
>         Transform combined = transform * data_->rotationTransform_;
>         if (!data_->supportsFlips_ && !!combined)
>                 transform = -data_->rotationTransform_;
>
> Not really sure I got why.
>
> All of this, but an even more fundamental question: is it too late/is
> it worh it to adjust the Transform definition to adopt the same
> direction as the kernel defined rotation ?
>
> Sorry for the long email :/

No problem! I like nothing more than discussing transforms, except
possibly colour spaces!

More seriously, here are my key points:

1. The CameraConfiguration::transform field says what the application
wants the final output images to have.
2. It takes account of the sensor rotation. The most obvious example
is the Pi: if the sensor is mounted upside down, and the applications
ask for images "the right way up" (i.e. transform = identity), then
the PH will apply h and v flips to compensate for the sensor mounting.
3. If the PH can do what the application requests, it leaves the
transform alone and your configuration is Valid.
4. If the PH can't do what the application wants, it will set the
value to something that it can do, and the configuration will be
Adjusted.

If folks wanted a short conference call to iron any of this out, I'd
be very happy to join in!

Thanks
David

>
> > > Thanks everyone!
> > > David
> >
> > Thanks!
> >
> > Robert
> >

Hi David & Jacopo

On Mon, 12 Dec 2022 at 11:46, David Plowman via libcamera-devel
<libcamera-devel@lists.libcamera.org> wrote:
>
> Hi Jacopo
>
> Thanks for wading into the sea on this question! (Very appropriate now
> that the stick person example has been turned into a shark...!)
>
> On Fri, 9 Dec 2022 at 12:47, Jacopo Mondi via libcamera-devel
> <libcamera-devel@lists.libcamera.org> wrote:
> >
> > Hi again,
> >
> > On Wed, Dec 07, 2022 at 03:48:40PM +0000, Robert Mader via libcamera-devel wrote:
> > > On 07.12.22 15:28, David Plowman via libcamera-devel wrote:
> > > > Hi Robert
> > > >
> > > > Thanks for prodding us all on this question!
> > >
> > > Hi, thanks for the quick reply! And a pleasure :P
> > >
> > > > I looked back over this again, and found Jacopo's description of the
> > > > V4L2_CID_CAMERA_SENSOR_ROTATION control
> > > > (https://patchwork.kernel.org/project/linux-media/patch/20191204091056.4842-5-jacopo@jmondi.org/
> > > > is what I stumbled into)
> > >
> > > The current kernel docu can be found at
> > >
> > > https://github.com/torvalds/linux/blob/8ed710da2873c2aeb3bb805864a699affaf1d03b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>
> Thanks for the updated reference. The stick person may have turned
> into a shark, but apart from that I think the sense of the
> documentation is unchanged. So if V4L2_CID_CAMERA_SENSOR_ROTATION says
> 90 degrees, then your image in memory looks like it has had a 90
> degree *clockwise* rotation applied.
>
> > >
> > > the libcamera one at
> > >
> > > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/transform.cpp#n54
> > >
> > > >  From that, and the nice example with the stick person, I deduce that
> > > > if your camera's V4L2_CID_CAMERA_SENSOR_ROTATION property says 90
> > > > degrees, then the image you capture (if nothing else happens to it)
> > > > will look like it has had a 90 degree clockwise rotation performed.
> > > > Everyone agree with that?
> > > Yep, agreed. That would mean V4L2_CID_CAMERA_SENSOR_ROTATION==90 would
> > > translate to Transform::Rot90.
> >
> > Let's here put some definitions in place
> >
> > V4L2_CID_CAMERA_SENSOR_ROTATION
> > This read-only control describes the rotation correction in degrees in
> > the counter-clockwise direction to be applied to the captured images
> > once captured to memory to compensate for the camera sensor mounting
> > rotation.
> >
> >  \var Transform::Rot270
> >  Rotation by 270 degrees clockwise (90 degrees anticlockwise).
> >
> >  Let's start by saying we did a sub-optimal (to be nice) job in
> >  having Transform and V4L2_CID_ROTATION operate in two different
> >  directions. Before the usage of Transform in libcamera gets widely
> >  adopted sould we align the two ?
>
> I don't mind at all... we just have to decide what we want!
>
> >
> >  Hence if I'm not mistaken, if your camera has Rotation=90 and an
> >  application supplies Transform::Identity in
> >  CameraConfiguration::transform what you should get back is Rot270
> >
> >  Correct ?
>
> The way things are currently, if the camera says Rotation=90, then
> your image in memory looks like it is rotated 90 degrees clockwise, so
> the Transform (if your pipeline and sensor can't apply any
> flips/rotations at all) will come back as Rot90.
>
> That is, the only transform you can specify that won't be adjusted, is
> a 90 degree clockwise rotation, so transform=Rot90 as things stand
> currently.
>
> If we choose to change the sense of the libcamera.Transform rotation,
> then we'd get transform=Rot270 instead.
>
>  (If your pipeline handler can do flips and transposes, it may be able
> to do what you ask and so the transform would come back unchanged and
> would be "Valid".)
>
> >
> > > > So in turn, if you have a camera/ISP that can apply no transforms at
> > > > all, then the only value for the user transform that will not come
> > > > back as adjusted is "rot90", because it looks like it's had a 90
> > > > degree clockwise rotation performed (and libcamera transforms count
> > > > rotations as being clockwise). Does that also make sense?
> >
> > It might be the case today, but I wonder if that's desirable.
> >
> > If an application supplies CameraConfiguration::transform = Identity
> > it means it doesn't want any rotation applied by the library. It will
> > get back 270 to indicate that that's how the image is rotated and will
> > get an Adjusted configuration.
>
> I'm not quite sure I get that, perhaps it depends how we interpret
> "applied by the library". I guess I agree if we mean "applied by the
> library together with any rotation of the sensor"? Maybe a real
> example is helpful:
>
> So for a Raspberry Pi, for example, our sensors are mostly mounted
> upside down (V4L2_CID_CAMERA_SENSOR_ROTATION says "180"). Our users
> say the transform they want is transform=Identity i.e. images come
> back "the right way up". Internally it means that we do apply an extra
> h+v flip to undo the 180 degree rotation of the sensor.
>
> >
> > If instead an application supplies CameraConfiguration::transform = Rot270
> > and your camera -cannot- do any rotation, it will still receive back
> > Rot270 and the state is Valid.
>
> I think that's true if we change the sense of the libcamera.Transform
> rotation. Currently, as I said earlier, I think we get Rot90?
>
> >
> > If it asks for CameraConfiguration::transform = Rot270 and the camera
> > -can- do that, it will clear CameraConfiguration::transform to
> > Identity and return Adjusted ?
>
> I'm not sure there... The CameraConfiguration::transform is the final
> transform you want. If that can be done, it's left alone and you get
> back Valid. It's not telling you what else you would need to do
> afterwards.
>
> (To find out what you need to do afterwards you'd work out
> "inverse(what-you-will-get) * what-you-wanted", or something like
> that.)
>
> >
> > Is this what happens (I'm looking at the above code with David's
> > suggestion to use the inverse of roationTransform).
> >
> >
> > > >
> > > > So by a slightly arbitrary mixture of conventions, it looks to me as
> > > > though the use of the inverse that I queried is indeed wrong, because
> > > > we "need camera 90 degree rotation" => "user transform rot90" in this
> > > > case.
> > > >
> > > > Does that sound plausible... what do folks think?
> > >
> > > Ah, I think the question is whether the adjusted value means: "this is what
> > > you'll get" vs "this is what you have to do yourself".
> > >
> > > Above I argued in favor of the later, but I guess the first one is what the
> > > API is supposed to mean. That would imply that the client has compute the
> > > difference between requested and adjusted value itself.
> >
> > I always presumed the latter was meant to happen..
>
> Yes, I think this is the crux of the matter. I believe it's "what you
> will get". It's not "what you need to do after". (If it was "what you
> need to do after", it would change every time you try to validate the
> configuration!)
>
> >
> > >
> > > In my case that would mean:
> > >
> > > requested CameraConfiguration::transform: Transform::Identity,
> > > V4L2_CID_CAMERA_SENSOR_ROTATION: 270, adjusted
> > > CameraConfiguration::transform: Transform::Rot270
> > >
> >
> > I guess it's Rot90 because of the clockwise/counter-clockwise issue ?
>
> Yes, looks right to me!
>
> >
> > > To compute what the client has to do, it would need to do:
> > > Transform::Identity - Transform::Rot270 = Transform::Rot90. And then apply
> > > that, a 90 degr. rotation clockwise.
>
> Yes, I guess I wrote it above, it's "inverse(what-you-got) *
> what-you-wanted", in this case inverse(rot270) * identity which is
> rot90 (in the current clockwise sense).
>
> > >
> > > Does that sound reasonable? In that case the patch here should indeed be
> > > `*transform = rotationTransform;`, without the inverse.
> > >
>
> Yes, agree!
>
> >
> >         /0\
> >
> > before we plumb this into and make assumptions that will be then
> > set into stone in our adaption layers, can we take a step back and
> > define what kind of API we would like to have ?
> >
> > Let's start by the definition of CameraConfiguration::transform.
> >
> > It's a member of CameraConfiguration, and as by definition it is
> > filled-in by the application and eventually adjusted by the Camera to
> > a value that's acceptable/doable for the current configuration.
> >
> > 1) Application to Camera  = (counter?)clockwise degrees the camera
> > system should rotate the image before presenting it to the user
>
> This may depend on what exactly we mean by "the camera system". I'd
> explain it as
> "the final resulting transform that the application wants to be
> applied to all the output images". After all, this is actually the
> only thing the application knows!
>
> >
> > 2) Camera to application = the rotation the camera can actually do
> > (ie. only sensor flips == no Transpose)
>
> Agree, mostly, though I would say "transform" rather than "rotation"
> because flips aren't rotations, you could in theory have a PH that
> could do transposes (even though we probably don't currently).
>
> >
> > The current documentation doesn't explain how it gets adjusted
>
> This is true, though I also think it's platform dependent. If you had
> a PH that can't do transposes but can do flips, you have a choice how
> you handle (for example) rot90. You could say "oh I can't do that, so
> in this case I won't do anything at all" and leave the application to
> sort everything out. The Pi tries to say "well, I'll do everything
> except for the transpose, so an application only has to worry about
> that one case". But there is a choice...
>
> >
> > /**
> >  * \var CameraConfiguration::transform
> >  * \brief User-specified transform to be applied to the image
> >  *
> >  * The transform is a user-specified 2D plane transform that will be applied
> >  * to the camera images by the processing pipeline before being handed to
> >  * the application. This is subsequent to any transform that is already
> >  * required to fix up any platform-defined rotation.
> >  *
> >  * The usual 2D plane transforms are allowed here (horizontal/vertical
> >  * flips, multiple of 90-degree rotations etc.), but the validate() function
> >  * may adjust this field at its discretion if the selection is not supported.
> >  */
> >
> > The last part in particular, "at its discrection" it's really too
> > generic.
>
> I guess it's hinting at the fact that it's up to the PH, and we don't
> mandate the exact way in which it can fail to do what you asked for!
> But it would be fine to have a discussion on that.
>
> >
> > Let's start from the beginning: what application should use transform
> > for ? I presume they should:
> >
> > 1) Inspect propertis::Rotation
> > 2) Correct the image for the properties::Rotation amount (actually,
> > the inverse because of the clockwise/counter-clockwise thing)
> > 3) Add any additional rotation they would like on top of this
>
> So I think the point of the transform is that you *don't* have to
> worry about the properties::Rotation. It takes care of that for you.
> You say "I want upside down images" and you get them, whatever the
> sensor's rotation.
>
> >
> > How should transform be adjusted ?
> >
> > Identity:
> > If application->identity the camera does not have to apply any
> > transform. Should the transform be adjutes like it happens today to
> > compensate the Rotation (at least that's what I presume happens) ? I
> > presume no, applications can infer rotation from properties and if they
> > chose Identity either other layers above will do the transform or
> > they're fine with images as they are.
>
> If the application says it wants the "identity" transform, then it
> should get output images with no transformation applied. But this does
> mean it will undo the sensor rotation (this being the normal case on
> the Pi).
>
> >
> > A supported/non-supported transform:
> > An application asks for a VFLIP, the camera can do it, transform is
> > not changed. If the camera cannot flip, the transform is set back to
> > Identity and the state is adjusted.
>
> Yes, mostly, except that the sensor may be mounted with a rotation,
> but if it isn't, then I agree with this.
>
> >
> > A partially non-supported transform:
> > An application asks for Rot270 (Transpose | HFLIP) and the camera can
> > only do HFLIP. transform sould be then set to HFLIP and applications
> > know that the remaining transformation should be handled elsewhere.
>
> As I suggested above, I'm not sure whether we want to mandate this. I
> agree it seems useful behaviour, which is why the Pi does it, but I'm
> open minded about requiring it or not.
>
> >
> > Are we missing anything with such behaviour ? I presume it's rather
> > similar to what happens today if not for the fact that the validate()
> > implementation adjusts transform to rotation in case it cannot flip
> >
> >         Transform combined = transform * data_->rotationTransform_;
> >         if (!data_->supportsFlips_ && !!combined)
> >                 transform = -data_->rotationTransform_;
> >
> > Not really sure I got why.
> >
> > All of this, but an even more fundamental question: is it too late/is
> > it worh it to adjust the Transform definition to adopt the same
> > direction as the kernel defined rotation ?
> >
> > Sorry for the long email :/
>
> No problem! I like nothing more than discussing transforms, except
> possibly colour spaces!
>
> More seriously, here are my key points:
>
> 1. The CameraConfiguration::transform field says what the application
> wants the final output images to have.
> 2. It takes account of the sensor rotation. The most obvious example
> is the Pi: if the sensor is mounted upside down, and the applications
> ask for images "the right way up" (i.e. transform = identity), then
> the PH will apply h and v flips to compensate for the sensor mounting.
> 3. If the PH can do what the application requests, it leaves the
> transform alone and your configuration is Valid.
> 4. If the PH can't do what the application wants, it will set the
> value to something that it can do, and the configuration will be
> Adjusted.
>
> If folks wanted a short conference call to iron any of this out, I'd
> be very happy to join in!
>
> Thanks
> David

I'll admit to having only skim read a chunk of this, but it seems
worth throwing in how Android CameraHAL1 dealt with images (largely
from memory of about 10 years ago!).

ALL images would have the native orientation of the sensor.

Preview was told about the appropriate rotation to apply via
setDisplayOrientation().
JPEG and MP4 stored the rotation in the EXIF and Composition Matrix
respectively.
(The bit that many apps forgot about was that it didn't rotate the
pixels in a raw buffer, and they made assumptions. I remember fixing
up the HAL to get around a limitation in the Skype app, and as soon as
I'd done so they released a fixed app).

This obviously puts a requirement on the preview and playback
subsystems, but those are all pretty much mandatory these days.
As long as all the bits of the system know how to orient the images,
then it largely becomes a non-question. As long as libcamera has a
defined way to get all that metadata, then anything can be achieved.

  Dave

Hi David

On Mon, Dec 12, 2022 at 11:46:31AM +0000, David Plowman wrote:
> Hi Jacopo
>
> Thanks for wading into the sea on this question! (Very appropriate now
> that the stick person example has been turned into a shark...!)
>
> On Fri, 9 Dec 2022 at 12:47, Jacopo Mondi via libcamera-devel
> <libcamera-devel@lists.libcamera.org> wrote:
> >
> > Hi again,
> >
> > On Wed, Dec 07, 2022 at 03:48:40PM +0000, Robert Mader via libcamera-devel wrote:
> > > On 07.12.22 15:28, David Plowman via libcamera-devel wrote:
> > > > Hi Robert
> > > >
> > > > Thanks for prodding us all on this question!
> > >
> > > Hi, thanks for the quick reply! And a pleasure :P
> > >
> > > > I looked back over this again, and found Jacopo's description of the
> > > > V4L2_CID_CAMERA_SENSOR_ROTATION control
> > > > (https://patchwork.kernel.org/project/linux-media/patch/20191204091056.4842-5-jacopo@jmondi.org/
> > > > is what I stumbled into)
> > >
> > > The current kernel docu can be found at
> > >
> > > https://github.com/torvalds/linux/blob/8ed710da2873c2aeb3bb805864a699affaf1d03b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>
> Thanks for the updated reference. The stick person may have turned
> into a shark, but apart from that I think the sense of the
> documentation is unchanged. So if V4L2_CID_CAMERA_SENSOR_ROTATION says
> 90 degrees, then your image in memory looks like it has had a 90
> degree *clockwise* rotation applied.
>

Yes!

> > >
> > > the libcamera one at
> > >
> > > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/transform.cpp#n54
> > >
> > > >  From that, and the nice example with the stick person, I deduce that
> > > > if your camera's V4L2_CID_CAMERA_SENSOR_ROTATION property says 90
> > > > degrees, then the image you capture (if nothing else happens to it)
> > > > will look like it has had a 90 degree clockwise rotation performed.
> > > > Everyone agree with that?
> > > Yep, agreed. That would mean V4L2_CID_CAMERA_SENSOR_ROTATION==90 would
> > > translate to Transform::Rot90.
> >
> > Let's here put some definitions in place
> >
> > V4L2_CID_CAMERA_SENSOR_ROTATION
> > This read-only control describes the rotation correction in degrees in
> > the counter-clockwise direction to be applied to the captured images
> > once captured to memory to compensate for the camera sensor mounting
> > rotation.
> >
> >  \var Transform::Rot270
> >  Rotation by 270 degrees clockwise (90 degrees anticlockwise).
> >
> >  Let's start by saying we did a sub-optimal (to be nice) job in
> >  having Transform and V4L2_CID_ROTATION operate in two different
> >  directions. Before the usage of Transform in libcamera gets widely
> >  adopted sould we align the two ?
>
> I don't mind at all... we just have to decide what we want!
>
> >
> >  Hence if I'm not mistaken, if your camera has Rotation=90 and an
> >  application supplies Transform::Identity in
> >  CameraConfiguration::transform what you should get back is Rot270
> >
> >  Correct ?
>
> The way things are currently, if the camera says Rotation=90, then
> your image in memory looks like it is rotated 90 degrees clockwise, so
> the Transform (if your pipeline and sensor can't apply any
> flips/rotations at all) will come back as Rot90.
>
> That is, the only transform you can specify that won't be adjusted, is
> a 90 degree clockwise rotation, so transform=Rot90 as things stand
> currently.
>
> If we choose to change the sense of the libcamera.Transform rotation,
> then we'd get transform=Rot270 instead.
>
>  (If your pipeline handler can do flips and transposes, it may be able
> to do what you ask and so the transform would come back unchanged and
> would be "Valid".)
>
> >
> > > > So in turn, if you have a camera/ISP that can apply no transforms at
> > > > all, then the only value for the user transform that will not come
> > > > back as adjusted is "rot90", because it looks like it's had a 90
> > > > degree clockwise rotation performed (and libcamera transforms count
> > > > rotations as being clockwise). Does that also make sense?
> >
> > It might be the case today, but I wonder if that's desirable.
> >
> > If an application supplies CameraConfiguration::transform = Identity
> > it means it doesn't want any rotation applied by the library. It will
> > get back 270 to indicate that that's how the image is rotated and will
> > get an Adjusted configuration.
>
> I'm not quite sure I get that, perhaps it depends how we interpret
> "applied by the library". I guess I agree if we mean "applied by the
> library together with any rotation of the sensor"? Maybe a real

"Applied by the library" == happens inside libcamera, ISP or sensor
flips..

> example is helpful:
>
> So for a Raspberry Pi, for example, our sensors are mostly mounted
> upside down (V4L2_CID_CAMERA_SENSOR_ROTATION says "180"). Our users
> say the transform they want is transform=Identity i.e. images come
> back "the right way up". Internally it means that we do apply an extra
> h+v flip to undo the 180 degree rotation of the sensor.
>

Here you go, this is where we disconnect.

As I've interpreted the API so far, and the way we designed
properties::Rotation was to signal to the application what is the
status of the image as it comes from the sensor. Application could then
correct that by setting CameraConfiguration::transform, but in most
cases the image will be corrected at rendering time by the application
itself.

IWO I was not expecting auto-correction and Transform::Identity to me
means "give me the images as they come from the sensor"

> >
> > If instead an application supplies CameraConfiguration::transform = Rot270
> > and your camera -cannot- do any rotation, it will still receive back
> > Rot270 and the state is Valid.
>
> I think that's true if we change the sense of the libcamera.Transform
> rotation. Currently, as I said earlier, I think we get Rot90?
>
> >
> > If it asks for CameraConfiguration::transform = Rot270 and the camera
> > -can- do that, it will clear CameraConfiguration::transform to
> > Identity and return Adjusted ?
>
> I'm not sure there... The CameraConfiguration::transform is the final
> transform you want. If that can be done, it's left alone and you get
> back Valid. It's not telling you what else you would need to do
> afterwards.
>
> (To find out what you need to do afterwards you'd work out
> "inverse(what-you-will-get) * what-you-wanted", or something like
> that.)
>

What I would like is to make as easier as possible for applications
to know what they have to do to have the image rotated in the way they
expect. There are several "transform" steps on the image, the first
one happens in libcamera, all the other ones are left to the
appplication.

*) The image "native" transform is reported by Rotation (this assumes
that any implicit rotation buried in the sensor driver, as per our
last discussion with Dave is not taken into account)

   properties::Rotation = Rot270 = HFlip | Transpose

*) Application sets CameraConfiguration::transform and call validate() to
check what can be done by libcamera and what should be done by next
layers

   CameraConfigurat::Transform = Rot270

*) The camera validate the transform and as it can only flip it sets
transform to HFlip, and the state is Adjusted

   validate() -> The camera can only flip
              -> CameraConfiguration = HFlip

*) Application gets back what the camera can do and instrument the
rest of the rendering pipeline to perform the additional transforms
the camera cannot do. To get what you have to do I presume you have to

        additionalTransform = what_you_want & !what_you_get

So yes, I was interpreting the usage of CameraConfiguration::transform
as a negotiation of what the library can do and what applications have
to do by themselves, which if I got you is different than what's
currecntly implemented ?

> >
> > Is this what happens (I'm looking at the above code with David's
> > suggestion to use the inverse of roationTransform).
> >
> >
> > > >
> > > > So by a slightly arbitrary mixture of conventions, it looks to me as
> > > > though the use of the inverse that I queried is indeed wrong, because
> > > > we "need camera 90 degree rotation" => "user transform rot90" in this
> > > > case.
> > > >
> > > > Does that sound plausible... what do folks think?
> > >
> > > Ah, I think the question is whether the adjusted value means: "this is what
> > > you'll get" vs "this is what you have to do yourself".
> > >
> > > Above I argued in favor of the later, but I guess the first one is what the
> > > API is supposed to mean. That would imply that the client has compute the
> > > difference between requested and adjusted value itself.
> >
> > I always presumed the latter was meant to happen..
>
> Yes, I think this is the crux of the matter. I believe it's "what you
> will get". It's not "what you need to do after". (If it was "what you
> need to do after", it would change every time you try to validate the
> configuration!)
>

On the "it would change every time you try to validate the
configuration": only if the camera cannot satisfy the transform you
have requested. If you ask for a flip and the camera can do it,
transform stays un-touched and the state is Valid ?

> >
> > >
> > > In my case that would mean:
> > >
> > > requested CameraConfiguration::transform: Transform::Identity,
> > > V4L2_CID_CAMERA_SENSOR_ROTATION: 270, adjusted
> > > CameraConfiguration::transform: Transform::Rot270
> > >
> >
> > I guess it's Rot90 because of the clockwise/counter-clockwise issue ?
>
> Yes, looks right to me!
>
> >
> > > To compute what the client has to do, it would need to do:
> > > Transform::Identity - Transform::Rot270 = Transform::Rot90. And then apply
> > > that, a 90 degr. rotation clockwise.
>
> Yes, I guess I wrote it above, it's "inverse(what-you-got) *
> what-you-wanted", in this case inverse(rot270) * identity which is
> rot90 (in the current clockwise sense).
>
> > >
> > > Does that sound reasonable? In that case the patch here should indeed be
> > > `*transform = rotationTransform;`, without the inverse.
> > >
>
> Yes, agree!
>
> >
> >         /0\
> >
> > before we plumb this into and make assumptions that will be then
> > set into stone in our adaption layers, can we take a step back and
> > define what kind of API we would like to have ?
> >
> > Let's start by the definition of CameraConfiguration::transform.
> >
> > It's a member of CameraConfiguration, and as by definition it is
> > filled-in by the application and eventually adjusted by the Camera to
> > a value that's acceptable/doable for the current configuration.
> >
> > 1) Application to Camera  = (counter?)clockwise degrees the camera
> > system should rotate the image before presenting it to the user
>
> This may depend on what exactly we mean by "the camera system". I'd
> explain it as
> "the final resulting transform that the application wants to be
> applied to all the output images". After all, this is actually the
> only thing the application knows!
>

agreed

> >
> > 2) Camera to application = the rotation the camera can actually do
> > (ie. only sensor flips == no Transpose)
>
> Agree, mostly, though I would say "transform" rather than "rotation"

yes, more correct

> because flips aren't rotations, you could in theory have a PH that
> could do transposes (even though we probably don't currently).
>

none currently afaict

> >
> > The current documentation doesn't explain how it gets adjusted
>
> This is true, though I also think it's platform dependent. If you had
> a PH that can't do transposes but can do flips, you have a choice how
> you handle (for example) rot90. You could say "oh I can't do that, so
> in this case I won't do anything at all" and leave the application to
> sort everything out. The Pi tries to say "well, I'll do everything
> except for the transpose, so an application only has to worry about
> that one case". But there is a choice...
>

I agree it's platform-specific, but the way transform is negotiated with
application should be standardized

> >
> > /**
> >  * \var CameraConfiguration::transform
> >  * \brief User-specified transform to be applied to the image
> >  *
> >  * The transform is a user-specified 2D plane transform that will be applied
> >  * to the camera images by the processing pipeline before being handed to
> >  * the application. This is subsequent to any transform that is already
> >  * required to fix up any platform-defined rotation.
> >  *
> >  * The usual 2D plane transforms are allowed here (horizontal/vertical
> >  * flips, multiple of 90-degree rotations etc.), but the validate() function
> >  * may adjust this field at its discretion if the selection is not supported.
> >  */
> >
> > The last part in particular, "at its discrection" it's really too
> > generic.
>
> I guess it's hinting at the fact that it's up to the PH, and we don't
> mandate the exact way in which it can fail to do what you asked for!
> But it would be fine to have a discussion on that.
>

That's the point that most concerns me :)

> >
> > Let's start from the beginning: what application should use transform
> > for ? I presume they should:
> >
> > 1) Inspect propertis::Rotation
> > 2) Correct the image for the properties::Rotation amount (actually,
> > the inverse because of the clockwise/counter-clockwise thing)
> > 3) Add any additional rotation they would like on top of this
>
> So I think the point of the transform is that you *don't* have to
> worry about the properties::Rotation. It takes care of that for you.
> You say "I want upside down images" and you get them, whatever the
> sensor's rotation.

That's acceptable as well if we prefer that, but it has to be
standardized among pipelines imho

>
> >
> > How should transform be adjusted ?
> >
> > Identity:
> > If application->identity the camera does not have to apply any
> > transform. Should the transform be adjutes like it happens today to
> > compensate the Rotation (at least that's what I presume happens) ? I
> > presume no, applications can infer rotation from properties and if they
> > chose Identity either other layers above will do the transform or
> > they're fine with images as they are.
>
> If the application says it wants the "identity" transform, then it
> should get output images with no transformation applied. But this does
> mean it will undo the sensor rotation (this being the normal case on
> the Pi).
>

right, it depends if we decide to account for the sensor rotation
inside libcamera or not

> >
> > A supported/non-supported transform:
> > An application asks for a VFLIP, the camera can do it, transform is
> > not changed. If the camera cannot flip, the transform is set back to
> > Identity and the state is adjusted.
>
> Yes, mostly, except that the sensor may be mounted with a rotation,
> but if it isn't, then I agree with this.
>
> >
> > A partially non-supported transform:
> > An application asks for Rot270 (Transpose | HFLIP) and the camera can
> > only do HFLIP. transform sould be then set to HFLIP and applications
> > know that the remaining transformation should be handled elsewhere.
>
> As I suggested above, I'm not sure whether we want to mandate this. I
> agree it seems useful behaviour, which is why the Pi does it, but I'm
> open minded about requiring it or not.
>
> >
> > Are we missing anything with such behaviour ? I presume it's rather
> > similar to what happens today if not for the fact that the validate()
> > implementation adjusts transform to rotation in case it cannot flip
> >
> >         Transform combined = transform * data_->rotationTransform_;
> >         if (!data_->supportsFlips_ && !!combined)
> >                 transform = -data_->rotationTransform_;
> >
> > Not really sure I got why.
> >
> > All of this, but an even more fundamental question: is it too late/is
> > it worh it to adjust the Transform definition to adopt the same
> > direction as the kernel defined rotation ?
> >
> > Sorry for the long email :/
>
> No problem! I like nothing more than discussing transforms, except
> possibly colour spaces!
>

:)

> More seriously, here are my key points:

I'll try to summarize what I see differently

>
> 1. The CameraConfiguration::transform field says what the application
> wants the final output images to have.

1. The CameraConfiguration::transform field says what transform the
library has to apply to images as they arrive from the sensor in its
default configuration

> 2. It takes account of the sensor rotation. The most obvious example
> is the Pi: if the sensor is mounted upside down, and the applications
> ask for images "the right way up" (i.e. transform = identity), then
> the PH will apply h and v flips to compensate for the sensor mounting.

2. It doesn't automatically account for sensor rotation. Application
might decide to do that along the rendering pipeline, in example

> 3. If the PH can do what the application requests, it leaves the
> transform alone and your configuration is Valid.

Agreed

> 4. If the PH can't do what the application wants, it will set the
> value to something that it can do, and the configuration will be
> Adjusted.

Agreed

>
> If folks wanted a short conference call to iron any of this out, I'd
> be very happy to join in!

I've asked others to have a look here, let's see if we need a call,
I'm all for it !

Thanks
  j

>
> Thanks
> David
>
> >
> > > > Thanks everyone!
> > > > David
> > >
> > > Thanks!
> > >
> > > Robert
> > >

Hi Jacopo

Thanks for the reply. I won't write a great deal this time round, I
agree that possibly a call would be a good idea.

On Tue, 13 Dec 2022 at 11:22, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi David
>
> On Mon, Dec 12, 2022 at 11:46:31AM +0000, David Plowman wrote:
> > Hi Jacopo
> >
> > Thanks for wading into the sea on this question! (Very appropriate now
> > that the stick person example has been turned into a shark...!)
> >
> > On Fri, 9 Dec 2022 at 12:47, Jacopo Mondi via libcamera-devel
> > <libcamera-devel@lists.libcamera.org> wrote:
> > >
> > > Hi again,
> > >
> > > On Wed, Dec 07, 2022 at 03:48:40PM +0000, Robert Mader via libcamera-devel wrote:
> > > > On 07.12.22 15:28, David Plowman via libcamera-devel wrote:
> > > > > Hi Robert
> > > > >
> > > > > Thanks for prodding us all on this question!
> > > >
> > > > Hi, thanks for the quick reply! And a pleasure :P
> > > >
> > > > > I looked back over this again, and found Jacopo's description of the
> > > > > V4L2_CID_CAMERA_SENSOR_ROTATION control
> > > > > (https://patchwork.kernel.org/project/linux-media/patch/20191204091056.4842-5-jacopo@jmondi.org/
> > > > > is what I stumbled into)
> > > >
> > > > The current kernel docu can be found at
> > > >
> > > > https://github.com/torvalds/linux/blob/8ed710da2873c2aeb3bb805864a699affaf1d03b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> >
> > Thanks for the updated reference. The stick person may have turned
> > into a shark, but apart from that I think the sense of the
> > documentation is unchanged. So if V4L2_CID_CAMERA_SENSOR_ROTATION says
> > 90 degrees, then your image in memory looks like it has had a 90
> > degree *clockwise* rotation applied.
> >
>
> Yes!
>
> > > >
> > > > the libcamera one at
> > > >
> > > > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/transform.cpp#n54
> > > >
> > > > >  From that, and the nice example with the stick person, I deduce that
> > > > > if your camera's V4L2_CID_CAMERA_SENSOR_ROTATION property says 90
> > > > > degrees, then the image you capture (if nothing else happens to it)
> > > > > will look like it has had a 90 degree clockwise rotation performed.
> > > > > Everyone agree with that?
> > > > Yep, agreed. That would mean V4L2_CID_CAMERA_SENSOR_ROTATION==90 would
> > > > translate to Transform::Rot90.
> > >
> > > Let's here put some definitions in place
> > >
> > > V4L2_CID_CAMERA_SENSOR_ROTATION
> > > This read-only control describes the rotation correction in degrees in
> > > the counter-clockwise direction to be applied to the captured images
> > > once captured to memory to compensate for the camera sensor mounting
> > > rotation.
> > >
> > >  \var Transform::Rot270
> > >  Rotation by 270 degrees clockwise (90 degrees anticlockwise).
> > >
> > >  Let's start by saying we did a sub-optimal (to be nice) job in
> > >  having Transform and V4L2_CID_ROTATION operate in two different
> > >  directions. Before the usage of Transform in libcamera gets widely
> > >  adopted sould we align the two ?
> >
> > I don't mind at all... we just have to decide what we want!
> >
> > >
> > >  Hence if I'm not mistaken, if your camera has Rotation=90 and an
> > >  application supplies Transform::Identity in
> > >  CameraConfiguration::transform what you should get back is Rot270
> > >
> > >  Correct ?
> >
> > The way things are currently, if the camera says Rotation=90, then
> > your image in memory looks like it is rotated 90 degrees clockwise, so
> > the Transform (if your pipeline and sensor can't apply any
> > flips/rotations at all) will come back as Rot90.
> >
> > That is, the only transform you can specify that won't be adjusted, is
> > a 90 degree clockwise rotation, so transform=Rot90 as things stand
> > currently.
> >
> > If we choose to change the sense of the libcamera.Transform rotation,
> > then we'd get transform=Rot270 instead.
> >
> >  (If your pipeline handler can do flips and transposes, it may be able
> > to do what you ask and so the transform would come back unchanged and
> > would be "Valid".)
> >
> > >
> > > > > So in turn, if you have a camera/ISP that can apply no transforms at
> > > > > all, then the only value for the user transform that will not come
> > > > > back as adjusted is "rot90", because it looks like it's had a 90
> > > > > degree clockwise rotation performed (and libcamera transforms count
> > > > > rotations as being clockwise). Does that also make sense?
> > >
> > > It might be the case today, but I wonder if that's desirable.
> > >
> > > If an application supplies CameraConfiguration::transform = Identity
> > > it means it doesn't want any rotation applied by the library. It will
> > > get back 270 to indicate that that's how the image is rotated and will
> > > get an Adjusted configuration.
> >
> > I'm not quite sure I get that, perhaps it depends how we interpret
> > "applied by the library". I guess I agree if we mean "applied by the
> > library together with any rotation of the sensor"? Maybe a real
>
> "Applied by the library" == happens inside libcamera, ISP or sensor
> flips..
>
> > example is helpful:
> >
> > So for a Raspberry Pi, for example, our sensors are mostly mounted
> > upside down (V4L2_CID_CAMERA_SENSOR_ROTATION says "180"). Our users
> > say the transform they want is transform=Identity i.e. images come
> > back "the right way up". Internally it means that we do apply an extra
> > h+v flip to undo the 180 degree rotation of the sensor.
> >
>
> Here you go, this is where we disconnect.
>
> As I've interpreted the API so far, and the way we designed
> properties::Rotation was to signal to the application what is the
> status of the image as it comes from the sensor. Application could then
> correct that by setting CameraConfiguration::transform, but in most
> cases the image will be corrected at rendering time by the application
> itself.
>
> IWO I was not expecting auto-correction and Transform::Identity to me
> means "give me the images as they come from the sensor"
>
> > >
> > > If instead an application supplies CameraConfiguration::transform = Rot270
> > > and your camera -cannot- do any rotation, it will still receive back
> > > Rot270 and the state is Valid.
> >
> > I think that's true if we change the sense of the libcamera.Transform
> > rotation. Currently, as I said earlier, I think we get Rot90?
> >
> > >
> > > If it asks for CameraConfiguration::transform = Rot270 and the camera
> > > -can- do that, it will clear CameraConfiguration::transform to
> > > Identity and return Adjusted ?
> >
> > I'm not sure there... The CameraConfiguration::transform is the final
> > transform you want. If that can be done, it's left alone and you get
> > back Valid. It's not telling you what else you would need to do
> > afterwards.
> >
> > (To find out what you need to do afterwards you'd work out
> > "inverse(what-you-will-get) * what-you-wanted", or something like
> > that.)
> >
>
> What I would like is to make as easier as possible for applications
> to know what they have to do to have the image rotated in the way they
> expect. There are several "transform" steps on the image, the first
> one happens in libcamera, all the other ones are left to the
> appplication.

I think this is exactly what I want, namely "to make it as easy as
possible for applications".

On Raspberry Pi - and this has been the case for a couple of years
now! - if a user wants images the "normal" way up then they simply
pass Transform::Identity. If they want upside down images they pass
Transform::Rot180. It's as easy as that, there is simply nothing else
to do.

Requiring all applications to look at the sensor rotation and then
combine various things to figure out what they have to ask for (and
possibly get it wrong) seems, well, actually quite unhelpful.

But this is indeed what we need to talk about. Let's organise a call!

Thanks
David

>
> *) The image "native" transform is reported by Rotation (this assumes
> that any implicit rotation buried in the sensor driver, as per our
> last discussion with Dave is not taken into account)
>
>    properties::Rotation = Rot270 = HFlip | Transpose
>
> *) Application sets CameraConfiguration::transform and call validate() to
> check what can be done by libcamera and what should be done by next
> layers
>
>    CameraConfigurat::Transform = Rot270
>
> *) The camera validate the transform and as it can only flip it sets
> transform to HFlip, and the state is Adjusted
>
>    validate() -> The camera can only flip
>               -> CameraConfiguration = HFlip
>
> *) Application gets back what the camera can do and instrument the
> rest of the rendering pipeline to perform the additional transforms
> the camera cannot do. To get what you have to do I presume you have to
>
>         additionalTransform = what_you_want & !what_you_get
>
> So yes, I was interpreting the usage of CameraConfiguration::transform
> as a negotiation of what the library can do and what applications have
> to do by themselves, which if I got you is different than what's
> currecntly implemented ?
>
> > >
> > > Is this what happens (I'm looking at the above code with David's
> > > suggestion to use the inverse of roationTransform).
> > >
> > >
> > > > >
> > > > > So by a slightly arbitrary mixture of conventions, it looks to me as
> > > > > though the use of the inverse that I queried is indeed wrong, because
> > > > > we "need camera 90 degree rotation" => "user transform rot90" in this
> > > > > case.
> > > > >
> > > > > Does that sound plausible... what do folks think?
> > > >
> > > > Ah, I think the question is whether the adjusted value means: "this is what
> > > > you'll get" vs "this is what you have to do yourself".
> > > >
> > > > Above I argued in favor of the later, but I guess the first one is what the
> > > > API is supposed to mean. That would imply that the client has compute the
> > > > difference between requested and adjusted value itself.
> > >
> > > I always presumed the latter was meant to happen..
> >
> > Yes, I think this is the crux of the matter. I believe it's "what you
> > will get". It's not "what you need to do after". (If it was "what you
> > need to do after", it would change every time you try to validate the
> > configuration!)
> >
>
> On the "it would change every time you try to validate the
> configuration": only if the camera cannot satisfy the transform you
> have requested. If you ask for a flip and the camera can do it,
> transform stays un-touched and the state is Valid ?
>
> > >
> > > >
> > > > In my case that would mean:
> > > >
> > > > requested CameraConfiguration::transform: Transform::Identity,
> > > > V4L2_CID_CAMERA_SENSOR_ROTATION: 270, adjusted
> > > > CameraConfiguration::transform: Transform::Rot270
> > > >
> > >
> > > I guess it's Rot90 because of the clockwise/counter-clockwise issue ?
> >
> > Yes, looks right to me!
> >
> > >
> > > > To compute what the client has to do, it would need to do:
> > > > Transform::Identity - Transform::Rot270 = Transform::Rot90. And then apply
> > > > that, a 90 degr. rotation clockwise.
> >
> > Yes, I guess I wrote it above, it's "inverse(what-you-got) *
> > what-you-wanted", in this case inverse(rot270) * identity which is
> > rot90 (in the current clockwise sense).
> >
> > > >
> > > > Does that sound reasonable? In that case the patch here should indeed be
> > > > `*transform = rotationTransform;`, without the inverse.
> > > >
> >
> > Yes, agree!
> >
> > >
> > >         /0\
> > >
> > > before we plumb this into and make assumptions that will be then
> > > set into stone in our adaption layers, can we take a step back and
> > > define what kind of API we would like to have ?
> > >
> > > Let's start by the definition of CameraConfiguration::transform.
> > >
> > > It's a member of CameraConfiguration, and as by definition it is
> > > filled-in by the application and eventually adjusted by the Camera to
> > > a value that's acceptable/doable for the current configuration.
> > >
> > > 1) Application to Camera  = (counter?)clockwise degrees the camera
> > > system should rotate the image before presenting it to the user
> >
> > This may depend on what exactly we mean by "the camera system". I'd
> > explain it as
> > "the final resulting transform that the application wants to be
> > applied to all the output images". After all, this is actually the
> > only thing the application knows!
> >
>
> agreed
>
> > >
> > > 2) Camera to application = the rotation the camera can actually do
> > > (ie. only sensor flips == no Transpose)
> >
> > Agree, mostly, though I would say "transform" rather than "rotation"
>
> yes, more correct
>
> > because flips aren't rotations, you could in theory have a PH that
> > could do transposes (even though we probably don't currently).
> >
>
> none currently afaict
>
> > >
> > > The current documentation doesn't explain how it gets adjusted
> >
> > This is true, though I also think it's platform dependent. If you had
> > a PH that can't do transposes but can do flips, you have a choice how
> > you handle (for example) rot90. You could say "oh I can't do that, so
> > in this case I won't do anything at all" and leave the application to
> > sort everything out. The Pi tries to say "well, I'll do everything
> > except for the transpose, so an application only has to worry about
> > that one case". But there is a choice...
> >
>
> I agree it's platform-specific, but the way transform is negotiated with
> application should be standardized
>
> > >
> > > /**
> > >  * \var CameraConfiguration::transform
> > >  * \brief User-specified transform to be applied to the image
> > >  *
> > >  * The transform is a user-specified 2D plane transform that will be applied
> > >  * to the camera images by the processing pipeline before being handed to
> > >  * the application. This is subsequent to any transform that is already
> > >  * required to fix up any platform-defined rotation.
> > >  *
> > >  * The usual 2D plane transforms are allowed here (horizontal/vertical
> > >  * flips, multiple of 90-degree rotations etc.), but the validate() function
> > >  * may adjust this field at its discretion if the selection is not supported.
> > >  */
> > >
> > > The last part in particular, "at its discrection" it's really too
> > > generic.
> >
> > I guess it's hinting at the fact that it's up to the PH, and we don't
> > mandate the exact way in which it can fail to do what you asked for!
> > But it would be fine to have a discussion on that.
> >
>
> That's the point that most concerns me :)
>
> > >
> > > Let's start from the beginning: what application should use transform
> > > for ? I presume they should:
> > >
> > > 1) Inspect propertis::Rotation
> > > 2) Correct the image for the properties::Rotation amount (actually,
> > > the inverse because of the clockwise/counter-clockwise thing)
> > > 3) Add any additional rotation they would like on top of this
> >
> > So I think the point of the transform is that you *don't* have to
> > worry about the properties::Rotation. It takes care of that for you.
> > You say "I want upside down images" and you get them, whatever the
> > sensor's rotation.
>
> That's acceptable as well if we prefer that, but it has to be
> standardized among pipelines imho
>
> >
> > >
> > > How should transform be adjusted ?
> > >
> > > Identity:
> > > If application->identity the camera does not have to apply any
> > > transform. Should the transform be adjutes like it happens today to
> > > compensate the Rotation (at least that's what I presume happens) ? I
> > > presume no, applications can infer rotation from properties and if they
> > > chose Identity either other layers above will do the transform or
> > > they're fine with images as they are.
> >
> > If the application says it wants the "identity" transform, then it
> > should get output images with no transformation applied. But this does
> > mean it will undo the sensor rotation (this being the normal case on
> > the Pi).
> >
>
> right, it depends if we decide to account for the sensor rotation
> inside libcamera or not
>
> > >
> > > A supported/non-supported transform:
> > > An application asks for a VFLIP, the camera can do it, transform is
> > > not changed. If the camera cannot flip, the transform is set back to
> > > Identity and the state is adjusted.
> >
> > Yes, mostly, except that the sensor may be mounted with a rotation,
> > but if it isn't, then I agree with this.
> >
> > >
> > > A partially non-supported transform:
> > > An application asks for Rot270 (Transpose | HFLIP) and the camera can
> > > only do HFLIP. transform sould be then set to HFLIP and applications
> > > know that the remaining transformation should be handled elsewhere.
> >
> > As I suggested above, I'm not sure whether we want to mandate this. I
> > agree it seems useful behaviour, which is why the Pi does it, but I'm
> > open minded about requiring it or not.
> >
> > >
> > > Are we missing anything with such behaviour ? I presume it's rather
> > > similar to what happens today if not for the fact that the validate()
> > > implementation adjusts transform to rotation in case it cannot flip
> > >
> > >         Transform combined = transform * data_->rotationTransform_;
> > >         if (!data_->supportsFlips_ && !!combined)
> > >                 transform = -data_->rotationTransform_;
> > >
> > > Not really sure I got why.
> > >
> > > All of this, but an even more fundamental question: is it too late/is
> > > it worh it to adjust the Transform definition to adopt the same
> > > direction as the kernel defined rotation ?
> > >
> > > Sorry for the long email :/
> >
> > No problem! I like nothing more than discussing transforms, except
> > possibly colour spaces!
> >
>
> :)
>
> > More seriously, here are my key points:
>
> I'll try to summarize what I see differently
>
> >
> > 1. The CameraConfiguration::transform field says what the application
> > wants the final output images to have.
>
> 1. The CameraConfiguration::transform field says what transform the
> library has to apply to images as they arrive from the sensor in its
> default configuration
>
> > 2. It takes account of the sensor rotation. The most obvious example
> > is the Pi: if the sensor is mounted upside down, and the applications
> > ask for images "the right way up" (i.e. transform = identity), then
> > the PH will apply h and v flips to compensate for the sensor mounting.
>
> 2. It doesn't automatically account for sensor rotation. Application
> might decide to do that along the rendering pipeline, in example
>
> > 3. If the PH can do what the application requests, it leaves the
> > transform alone and your configuration is Valid.
>
> Agreed
>
> > 4. If the PH can't do what the application wants, it will set the
> > value to something that it can do, and the configuration will be
> > Adjusted.
>
> Agreed
>
> >
> > If folks wanted a short conference call to iron any of this out, I'd
> > be very happy to join in!
>
> I've asked others to have a look here, let's see if we need a call,
> I'm all for it !
>
> Thanks
>   j
>
> >
> > Thanks
> > David
> >
> > >
> > > > > Thanks everyone!
> > > > > David
> > > >
> > > > Thanks!
> > > >
> > > > Robert
> > > >

Hi David,

On Tue, Dec 13, 2022 at 02:30:24PM +0000, David Plowman wrote:
> Hi Jacopo
>
> Thanks for the reply. I won't write a great deal this time round, I
> agree that possibly a call would be a good idea.
>

Sure thanks

[snip]


> >
> > What I would like is to make as easier as possible for applications
> > to know what they have to do to have the image rotated in the way they
> > expect. There are several "transform" steps on the image, the first
> > one happens in libcamera, all the other ones are left to the
> > appplication.
>
> I think this is exactly what I want, namely "to make it as easy as
> possible for applications".
>
> On Raspberry Pi - and this has been the case for a couple of years
> now! - if a user wants images the "normal" way up then they simply
> pass Transform::Identity. If they want upside down images they pass
> Transform::Rot180. It's as easy as that, there is simply nothing else
> to do.
>
> Requiring all applications to look at the sensor rotation and then
> combine various things to figure out what they have to ask for (and
> possibly get it wrong) seems, well, actually quite unhelpful.

Robert, you're working on the pipewire integration. What's your
opinion on this ?

>
> But this is indeed what we need to talk about. Let's organise a call!
>
> Thanks
> David
>
> >
> > *) The image "native" transform is reported by Rotation (this assumes
> > that any implicit rotation buried in the sensor driver, as per our
> > last discussion with Dave is not taken into account)
> >
> >    properties::Rotation = Rot270 = HFlip | Transpose
> >
> > *) Application sets CameraConfiguration::transform and call validate() to
> > check what can be done by libcamera and what should be done by next
> > layers
> >
> >    CameraConfigurat::Transform = Rot270
> >
> > *) The camera validate the transform and as it can only flip it sets
> > transform to HFlip, and the state is Adjusted
> >
> >    validate() -> The camera can only flip
> >               -> CameraConfiguration = HFlip
> >
> > *) Application gets back what the camera can do and instrument the
> > rest of the rendering pipeline to perform the additional transforms
> > the camera cannot do. To get what you have to do I presume you have to
> >
> >         additionalTransform = what_you_want & !what_you_get
> >
> > So yes, I was interpreting the usage of CameraConfiguration::transform
> > as a negotiation of what the library can do and what applications have
> > to do by themselves, which if I got you is different than what's
> > currecntly implemented ?
> >
> > > >
> > > > Is this what happens (I'm looking at the above code with David's
> > > > suggestion to use the inverse of roationTransform).
> > > >
> > > >
> > > > > >
> > > > > > So by a slightly arbitrary mixture of conventions, it looks to me as
> > > > > > though the use of the inverse that I queried is indeed wrong, because
> > > > > > we "need camera 90 degree rotation" => "user transform rot90" in this
> > > > > > case.
> > > > > >
> > > > > > Does that sound plausible... what do folks think?
> > > > >
> > > > > Ah, I think the question is whether the adjusted value means: "this is what
> > > > > you'll get" vs "this is what you have to do yourself".
> > > > >
> > > > > Above I argued in favor of the later, but I guess the first one is what the
> > > > > API is supposed to mean. That would imply that the client has compute the
> > > > > difference between requested and adjusted value itself.
> > > >
> > > > I always presumed the latter was meant to happen..
> > >
> > > Yes, I think this is the crux of the matter. I believe it's "what you
> > > will get". It's not "what you need to do after". (If it was "what you
> > > need to do after", it would change every time you try to validate the
> > > configuration!)
> > >
> >
> > On the "it would change every time you try to validate the
> > configuration": only if the camera cannot satisfy the transform you
> > have requested. If you ask for a flip and the camera can do it,
> > transform stays un-touched and the state is Valid ?
> >
> > > >
> > > > >
> > > > > In my case that would mean:
> > > > >
> > > > > requested CameraConfiguration::transform: Transform::Identity,
> > > > > V4L2_CID_CAMERA_SENSOR_ROTATION: 270, adjusted
> > > > > CameraConfiguration::transform: Transform::Rot270
> > > > >
> > > >
> > > > I guess it's Rot90 because of the clockwise/counter-clockwise issue ?
> > >
> > > Yes, looks right to me!
> > >
> > > >
> > > > > To compute what the client has to do, it would need to do:
> > > > > Transform::Identity - Transform::Rot270 = Transform::Rot90. And then apply
> > > > > that, a 90 degr. rotation clockwise.
> > >
> > > Yes, I guess I wrote it above, it's "inverse(what-you-got) *
> > > what-you-wanted", in this case inverse(rot270) * identity which is
> > > rot90 (in the current clockwise sense).
> > >
> > > > >
> > > > > Does that sound reasonable? In that case the patch here should indeed be
> > > > > `*transform = rotationTransform;`, without the inverse.
> > > > >
> > >
> > > Yes, agree!
> > >
> > > >
> > > >         /0\
> > > >
> > > > before we plumb this into and make assumptions that will be then
> > > > set into stone in our adaption layers, can we take a step back and
> > > > define what kind of API we would like to have ?
> > > >
> > > > Let's start by the definition of CameraConfiguration::transform.
> > > >
> > > > It's a member of CameraConfiguration, and as by definition it is
> > > > filled-in by the application and eventually adjusted by the Camera to
> > > > a value that's acceptable/doable for the current configuration.
> > > >
> > > > 1) Application to Camera  = (counter?)clockwise degrees the camera
> > > > system should rotate the image before presenting it to the user
> > >
> > > This may depend on what exactly we mean by "the camera system". I'd
> > > explain it as
> > > "the final resulting transform that the application wants to be
> > > applied to all the output images". After all, this is actually the
> > > only thing the application knows!
> > >
> >
> > agreed
> >
> > > >
> > > > 2) Camera to application = the rotation the camera can actually do
> > > > (ie. only sensor flips == no Transpose)
> > >
> > > Agree, mostly, though I would say "transform" rather than "rotation"
> >
> > yes, more correct
> >
> > > because flips aren't rotations, you could in theory have a PH that
> > > could do transposes (even though we probably don't currently).
> > >
> >
> > none currently afaict
> >
> > > >
> > > > The current documentation doesn't explain how it gets adjusted
> > >
> > > This is true, though I also think it's platform dependent. If you had
> > > a PH that can't do transposes but can do flips, you have a choice how
> > > you handle (for example) rot90. You could say "oh I can't do that, so
> > > in this case I won't do anything at all" and leave the application to
> > > sort everything out. The Pi tries to say "well, I'll do everything
> > > except for the transpose, so an application only has to worry about
> > > that one case". But there is a choice...
> > >
> >
> > I agree it's platform-specific, but the way transform is negotiated with
> > application should be standardized
> >
> > > >
> > > > /**
> > > >  * \var CameraConfiguration::transform
> > > >  * \brief User-specified transform to be applied to the image
> > > >  *
> > > >  * The transform is a user-specified 2D plane transform that will be applied
> > > >  * to the camera images by the processing pipeline before being handed to
> > > >  * the application. This is subsequent to any transform that is already
> > > >  * required to fix up any platform-defined rotation.
> > > >  *
> > > >  * The usual 2D plane transforms are allowed here (horizontal/vertical
> > > >  * flips, multiple of 90-degree rotations etc.), but the validate() function
> > > >  * may adjust this field at its discretion if the selection is not supported.
> > > >  */
> > > >
> > > > The last part in particular, "at its discrection" it's really too
> > > > generic.
> > >
> > > I guess it's hinting at the fact that it's up to the PH, and we don't
> > > mandate the exact way in which it can fail to do what you asked for!
> > > But it would be fine to have a discussion on that.
> > >
> >
> > That's the point that most concerns me :)
> >
> > > >
> > > > Let's start from the beginning: what application should use transform
> > > > for ? I presume they should:
> > > >
> > > > 1) Inspect propertis::Rotation
> > > > 2) Correct the image for the properties::Rotation amount (actually,
> > > > the inverse because of the clockwise/counter-clockwise thing)
> > > > 3) Add any additional rotation they would like on top of this
> > >
> > > So I think the point of the transform is that you *don't* have to
> > > worry about the properties::Rotation. It takes care of that for you.
> > > You say "I want upside down images" and you get them, whatever the
> > > sensor's rotation.
> >
> > That's acceptable as well if we prefer that, but it has to be
> > standardized among pipelines imho
> >
> > >
> > > >
> > > > How should transform be adjusted ?
> > > >
> > > > Identity:
> > > > If application->identity the camera does not have to apply any
> > > > transform. Should the transform be adjutes like it happens today to
> > > > compensate the Rotation (at least that's what I presume happens) ? I
> > > > presume no, applications can infer rotation from properties and if they
> > > > chose Identity either other layers above will do the transform or
> > > > they're fine with images as they are.
> > >
> > > If the application says it wants the "identity" transform, then it
> > > should get output images with no transformation applied. But this does
> > > mean it will undo the sensor rotation (this being the normal case on
> > > the Pi).
> > >
> >
> > right, it depends if we decide to account for the sensor rotation
> > inside libcamera or not
> >
> > > >
> > > > A supported/non-supported transform:
> > > > An application asks for a VFLIP, the camera can do it, transform is
> > > > not changed. If the camera cannot flip, the transform is set back to
> > > > Identity and the state is adjusted.
> > >
> > > Yes, mostly, except that the sensor may be mounted with a rotation,
> > > but if it isn't, then I agree with this.
> > >
> > > >
> > > > A partially non-supported transform:
> > > > An application asks for Rot270 (Transpose | HFLIP) and the camera can
> > > > only do HFLIP. transform sould be then set to HFLIP and applications
> > > > know that the remaining transformation should be handled elsewhere.
> > >
> > > As I suggested above, I'm not sure whether we want to mandate this. I
> > > agree it seems useful behaviour, which is why the Pi does it, but I'm
> > > open minded about requiring it or not.
> > >
> > > >
> > > > Are we missing anything with such behaviour ? I presume it's rather
> > > > similar to what happens today if not for the fact that the validate()
> > > > implementation adjusts transform to rotation in case it cannot flip
> > > >
> > > >         Transform combined = transform * data_->rotationTransform_;
> > > >         if (!data_->supportsFlips_ && !!combined)
> > > >                 transform = -data_->rotationTransform_;
> > > >
> > > > Not really sure I got why.
> > > >
> > > > All of this, but an even more fundamental question: is it too late/is
> > > > it worh it to adjust the Transform definition to adopt the same
> > > > direction as the kernel defined rotation ?
> > > >
> > > > Sorry for the long email :/
> > >
> > > No problem! I like nothing more than discussing transforms, except
> > > possibly colour spaces!
> > >
> >
> > :)
> >
> > > More seriously, here are my key points:
> >
> > I'll try to summarize what I see differently
> >
> > >
> > > 1. The CameraConfiguration::transform field says what the application
> > > wants the final output images to have.
> >
> > 1. The CameraConfiguration::transform field says what transform the
> > library has to apply to images as they arrive from the sensor in its
> > default configuration
> >
> > > 2. It takes account of the sensor rotation. The most obvious example
> > > is the Pi: if the sensor is mounted upside down, and the applications
> > > ask for images "the right way up" (i.e. transform = identity), then
> > > the PH will apply h and v flips to compensate for the sensor mounting.
> >
> > 2. It doesn't automatically account for sensor rotation. Application
> > might decide to do that along the rendering pipeline, in example
> >
> > > 3. If the PH can do what the application requests, it leaves the
> > > transform alone and your configuration is Valid.
> >
> > Agreed
> >
> > > 4. If the PH can't do what the application wants, it will set the
> > > value to something that it can do, and the configuration will be
> > > Adjusted.
> >
> > Agreed
> >
> > >
> > > If folks wanted a short conference call to iron any of this out, I'd
> > > be very happy to join in!
> >
> > I've asked others to have a look here, let's see if we need a call,
> > I'm all for it !
> >
> > Thanks
> >   j
> >
> > >
> > > Thanks
> > > David
> > >
> > > >
> > > > > > Thanks everyone!
> > > > > > David
> > > > >
> > > > > Thanks!
> > > > >
> > > > > Robert
> > > > >

Hi all,


On Tue, 13 Dec 2022 at 14:30, David Plowman via libcamera-devel <
libcamera-devel@lists.libcamera.org> wrote:

> Hi Jacopo
>
> Thanks for the reply. I won't write a great deal this time round, I
> agree that possibly a call would be a good idea.
>
> On Tue, 13 Dec 2022 at 11:22, Jacopo Mondi <jacopo@jmondi.org> wrote:
> >
> > Hi David
> >
> > On Mon, Dec 12, 2022 at 11:46:31AM +0000, David Plowman wrote:
> > > Hi Jacopo
> > >
> > > Thanks for wading into the sea on this question! (Very appropriate now
> > > that the stick person example has been turned into a shark...!)
> > >
> > > On Fri, 9 Dec 2022 at 12:47, Jacopo Mondi via libcamera-devel
> > > <libcamera-devel@lists.libcamera.org> wrote:
> > > >
> > > > Hi again,
> > > >
> > > > On Wed, Dec 07, 2022 at 03:48:40PM +0000, Robert Mader via
> libcamera-devel wrote:
> > > > > On 07.12.22 15:28, David Plowman via libcamera-devel wrote:
> > > > > > Hi Robert
> > > > > >
> > > > > > Thanks for prodding us all on this question!
> > > > >
> > > > > Hi, thanks for the quick reply! And a pleasure :P
> > > > >
> > > > > > I looked back over this again, and found Jacopo's description of
> the
> > > > > > V4L2_CID_CAMERA_SENSOR_ROTATION control
> > > > > > (
> https://patchwork.kernel.org/project/linux-media/patch/20191204091056.4842-5-jacopo@jmondi.org/
> > > > > > is what I stumbled into)
> > > > >
> > > > > The current kernel docu can be found at
> > > > >
> > > > >
> https://github.com/torvalds/linux/blob/8ed710da2873c2aeb3bb805864a699affaf1d03b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> > >
> > > Thanks for the updated reference. The stick person may have turned
> > > into a shark, but apart from that I think the sense of the
> > > documentation is unchanged. So if V4L2_CID_CAMERA_SENSOR_ROTATION says
> > > 90 degrees, then your image in memory looks like it has had a 90
> > > degree *clockwise* rotation applied.
> > >
> >
> > Yes!
> >
> > > > >
> > > > > the libcamera one at
> > > > >
> > > > >
> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/transform.cpp#n54
> > > > >
> > > > > >  From that, and the nice example with the stick person, I deduce
> that
> > > > > > if your camera's V4L2_CID_CAMERA_SENSOR_ROTATION property says 90
> > > > > > degrees, then the image you capture (if nothing else happens to
> it)
> > > > > > will look like it has had a 90 degree clockwise rotation
> performed.
> > > > > > Everyone agree with that?
> > > > > Yep, agreed. That would mean V4L2_CID_CAMERA_SENSOR_ROTATION==90
> would
> > > > > translate to Transform::Rot90.
> > > >
> > > > Let's here put some definitions in place
> > > >
> > > > V4L2_CID_CAMERA_SENSOR_ROTATION
> > > > This read-only control describes the rotation correction in degrees
> in
> > > > the counter-clockwise direction to be applied to the captured images
> > > > once captured to memory to compensate for the camera sensor mounting
> > > > rotation.
> > > >
> > > >  \var Transform::Rot270
> > > >  Rotation by 270 degrees clockwise (90 degrees anticlockwise).
> > > >
> > > >  Let's start by saying we did a sub-optimal (to be nice) job in
> > > >  having Transform and V4L2_CID_ROTATION operate in two different
> > > >  directions. Before the usage of Transform in libcamera gets widely
> > > >  adopted sould we align the two ?
> > >
> > > I don't mind at all... we just have to decide what we want!
> > >
> > > >
> > > >  Hence if I'm not mistaken, if your camera has Rotation=90 and an
> > > >  application supplies Transform::Identity in
> > > >  CameraConfiguration::transform what you should get back is Rot270
> > > >
> > > >  Correct ?
> > >
> > > The way things are currently, if the camera says Rotation=90, then
> > > your image in memory looks like it is rotated 90 degrees clockwise, so
> > > the Transform (if your pipeline and sensor can't apply any
> > > flips/rotations at all) will come back as Rot90.
> > >
> > > That is, the only transform you can specify that won't be adjusted, is
> > > a 90 degree clockwise rotation, so transform=Rot90 as things stand
> > > currently.
> > >
> > > If we choose to change the sense of the libcamera.Transform rotation,
> > > then we'd get transform=Rot270 instead.
> > >
> > >  (If your pipeline handler can do flips and transposes, it may be able
> > > to do what you ask and so the transform would come back unchanged and
> > > would be "Valid".)
> > >
> > > >
> > > > > > So in turn, if you have a camera/ISP that can apply no
> transforms at
> > > > > > all, then the only value for the user transform that will not
> come
> > > > > > back as adjusted is "rot90", because it looks like it's had a 90
> > > > > > degree clockwise rotation performed (and libcamera transforms
> count
> > > > > > rotations as being clockwise). Does that also make sense?
> > > >
> > > > It might be the case today, but I wonder if that's desirable.
> > > >
> > > > If an application supplies CameraConfiguration::transform = Identity
> > > > it means it doesn't want any rotation applied by the library. It will
> > > > get back 270 to indicate that that's how the image is rotated and
> will
> > > > get an Adjusted configuration.
> > >
> > > I'm not quite sure I get that, perhaps it depends how we interpret
> > > "applied by the library". I guess I agree if we mean "applied by the
> > > library together with any rotation of the sensor"? Maybe a real
> >
> > "Applied by the library" == happens inside libcamera, ISP or sensor
> > flips..
> >
> > > example is helpful:
> > >
> > > So for a Raspberry Pi, for example, our sensors are mostly mounted
> > > upside down (V4L2_CID_CAMERA_SENSOR_ROTATION says "180"). Our users
> > > say the transform they want is transform=Identity i.e. images come
> > > back "the right way up". Internally it means that we do apply an extra
> > > h+v flip to undo the 180 degree rotation of the sensor.
> > >
> >
> > Here you go, this is where we disconnect.
> >
> > As I've interpreted the API so far, and the way we designed
> > properties::Rotation was to signal to the application what is the
> > status of the image as it comes from the sensor. Application could then
> > correct that by setting CameraConfiguration::transform, but in most
> > cases the image will be corrected at rendering time by the application
> > itself.
> >
> > IWO I was not expecting auto-correction and Transform::Identity to me
> > means "give me the images as they come from the sensor"
> >
> > > >
> > > > If instead an application supplies CameraConfiguration::transform =
> Rot270
> > > > and your camera -cannot- do any rotation, it will still receive back
> > > > Rot270 and the state is Valid.
> > >
> > > I think that's true if we change the sense of the libcamera.Transform
> > > rotation. Currently, as I said earlier, I think we get Rot90?
> > >
> > > >
> > > > If it asks for CameraConfiguration::transform = Rot270 and the camera
> > > > -can- do that, it will clear CameraConfiguration::transform to
> > > > Identity and return Adjusted ?
> > >
> > > I'm not sure there... The CameraConfiguration::transform is the final
> > > transform you want. If that can be done, it's left alone and you get
> > > back Valid. It's not telling you what else you would need to do
> > > afterwards.
> > >
> > > (To find out what you need to do afterwards you'd work out
> > > "inverse(what-you-will-get) * what-you-wanted", or something like
> > > that.)
> > >
> >
> > What I would like is to make as easier as possible for applications
> > to know what they have to do to have the image rotated in the way they
> > expect. There are several "transform" steps on the image, the first
> > one happens in libcamera, all the other ones are left to the
> > appplication.
>
> I think this is exactly what I want, namely "to make it as easy as
> possible for applications".
>
> On Raspberry Pi - and this has been the case for a couple of years
> now! - if a user wants images the "normal" way up then they simply
> pass Transform::Identity. If they want upside down images they pass
> Transform::Rot180. It's as easy as that, there is simply nothing else
> to do.
>
> Requiring all applications to look at the sensor rotation and then
> combine various things to figure out what they have to ask for (and
> possibly get it wrong) seems, well, actually quite unhelpful.
>

FWIW, I do agree that applications should not have to trouble themselves
with munging sensor rotation andrequested rotation to achieve their
desired outcome.  This definitely wants to be hidden in the libcamera core
or pipeline handler layers.

Naush



>
> But this is indeed what we need to talk about. Let's organise a call!
>
> Thanks
> David
>
> >
> > *) The image "native" transform is reported by Rotation (this assumes
> > that any implicit rotation buried in the sensor driver, as per our
> > last discussion with Dave is not taken into account)
> >
> >    properties::Rotation = Rot270 = HFlip | Transpose
> >
> > *) Application sets CameraConfiguration::transform and call validate() to
> > check what can be done by libcamera and what should be done by next
> > layers
> >
> >    CameraConfigurat::Transform = Rot270
> >
> > *) The camera validate the transform and as it can only flip it sets
> > transform to HFlip, and the state is Adjusted
> >
> >    validate() -> The camera can only flip
> >               -> CameraConfiguration = HFlip
> >
> > *) Application gets back what the camera can do and instrument the
> > rest of the rendering pipeline to perform the additional transforms
> > the camera cannot do. To get what you have to do I presume you have to
> >
> >         additionalTransform = what_you_want & !what_you_get
> >
> > So yes, I was interpreting the usage of CameraConfiguration::transform
> > as a negotiation of what the library can do and what applications have
> > to do by themselves, which if I got you is different than what's
> > currecntly implemented ?
> >
> > > >
> > > > Is this what happens (I'm looking at the above code with David's
> > > > suggestion to use the inverse of roationTransform).
> > > >
> > > >
> > > > > >
> > > > > > So by a slightly arbitrary mixture of conventions, it looks to
> me as
> > > > > > though the use of the inverse that I queried is indeed wrong,
> because
> > > > > > we "need camera 90 degree rotation" => "user transform rot90" in
> this
> > > > > > case.
> > > > > >
> > > > > > Does that sound plausible... what do folks think?
> > > > >
> > > > > Ah, I think the question is whether the adjusted value means:
> "this is what
> > > > > you'll get" vs "this is what you have to do yourself".
> > > > >
> > > > > Above I argued in favor of the later, but I guess the first one is
> what the
> > > > > API is supposed to mean. That would imply that the client has
> compute the
> > > > > difference between requested and adjusted value itself.
> > > >
> > > > I always presumed the latter was meant to happen..
> > >
> > > Yes, I think this is the crux of the matter. I believe it's "what you
> > > will get". It's not "what you need to do after". (If it was "what you
> > > need to do after", it would change every time you try to validate the
> > > configuration!)
> > >
> >
> > On the "it would change every time you try to validate the
> > configuration": only if the camera cannot satisfy the transform you
> > have requested. If you ask for a flip and the camera can do it,
> > transform stays un-touched and the state is Valid ?
> >
> > > >
> > > > >
> > > > > In my case that would mean:
> > > > >
> > > > > requested CameraConfiguration::transform: Transform::Identity,
> > > > > V4L2_CID_CAMERA_SENSOR_ROTATION: 270, adjusted
> > > > > CameraConfiguration::transform: Transform::Rot270
> > > > >
> > > >
> > > > I guess it's Rot90 because of the clockwise/counter-clockwise issue ?
> > >
> > > Yes, looks right to me!
> > >
> > > >
> > > > > To compute what the client has to do, it would need to do:
> > > > > Transform::Identity - Transform::Rot270 = Transform::Rot90. And
> then apply
> > > > > that, a 90 degr. rotation clockwise.
> > >
> > > Yes, I guess I wrote it above, it's "inverse(what-you-got) *
> > > what-you-wanted", in this case inverse(rot270) * identity which is
> > > rot90 (in the current clockwise sense).
> > >
> > > > >
> > > > > Does that sound reasonable? In that case the patch here should
> indeed be
> > > > > `*transform = rotationTransform;`, without the inverse.
> > > > >
> > >
> > > Yes, agree!
> > >
> > > >
> > > >         /0\
> > > >
> > > > before we plumb this into and make assumptions that will be then
> > > > set into stone in our adaption layers, can we take a step back and
> > > > define what kind of API we would like to have ?
> > > >
> > > > Let's start by the definition of CameraConfiguration::transform.
> > > >
> > > > It's a member of CameraConfiguration, and as by definition it is
> > > > filled-in by the application and eventually adjusted by the Camera to
> > > > a value that's acceptable/doable for the current configuration.
> > > >
> > > > 1) Application to Camera  = (counter?)clockwise degrees the camera
> > > > system should rotate the image before presenting it to the user
> > >
> > > This may depend on what exactly we mean by "the camera system". I'd
> > > explain it as
> > > "the final resulting transform that the application wants to be
> > > applied to all the output images". After all, this is actually the
> > > only thing the application knows!
> > >
> >
> > agreed
> >
> > > >
> > > > 2) Camera to application = the rotation the camera can actually do
> > > > (ie. only sensor flips == no Transpose)
> > >
> > > Agree, mostly, though I would say "transform" rather than "rotation"
> >
> > yes, more correct
> >
> > > because flips aren't rotations, you could in theory have a PH that
> > > could do transposes (even though we probably don't currently).
> > >
> >
> > none currently afaict
> >
> > > >
> > > > The current documentation doesn't explain how it gets adjusted
> > >
> > > This is true, though I also think it's platform dependent. If you had
> > > a PH that can't do transposes but can do flips, you have a choice how
> > > you handle (for example) rot90. You could say "oh I can't do that, so
> > > in this case I won't do anything at all" and leave the application to
> > > sort everything out. The Pi tries to say "well, I'll do everything
> > > except for the transpose, so an application only has to worry about
> > > that one case". But there is a choice...
> > >
> >
> > I agree it's platform-specific, but the way transform is negotiated with
> > application should be standardized
> >
> > > >
> > > > /**
> > > >  * \var CameraConfiguration::transform
> > > >  * \brief User-specified transform to be applied to the image
> > > >  *
> > > >  * The transform is a user-specified 2D plane transform that will be
> applied
> > > >  * to the camera images by the processing pipeline before being
> handed to
> > > >  * the application. This is subsequent to any transform that is
> already
> > > >  * required to fix up any platform-defined rotation.
> > > >  *
> > > >  * The usual 2D plane transforms are allowed here
> (horizontal/vertical
> > > >  * flips, multiple of 90-degree rotations etc.), but the validate()
> function
> > > >  * may adjust this field at its discretion if the selection is not
> supported.
> > > >  */
> > > >
> > > > The last part in particular, "at its discrection" it's really too
> > > > generic.
> > >
> > > I guess it's hinting at the fact that it's up to the PH, and we don't
> > > mandate the exact way in which it can fail to do what you asked for!
> > > But it would be fine to have a discussion on that.
> > >
> >
> > That's the point that most concerns me :)
> >
> > > >
> > > > Let's start from the beginning: what application should use transform
> > > > for ? I presume they should:
> > > >
> > > > 1) Inspect propertis::Rotation
> > > > 2) Correct the image for the properties::Rotation amount (actually,
> > > > the inverse because of the clockwise/counter-clockwise thing)
> > > > 3) Add any additional rotation they would like on top of this
> > >
> > > So I think the point of the transform is that you *don't* have to
> > > worry about the properties::Rotation. It takes care of that for you.
> > > You say "I want upside down images" and you get them, whatever the
> > > sensor's rotation.
> >
> > That's acceptable as well if we prefer that, but it has to be
> > standardized among pipelines imho
> >
> > >
> > > >
> > > > How should transform be adjusted ?
> > > >
> > > > Identity:
> > > > If application->identity the camera does not have to apply any
> > > > transform. Should the transform be adjutes like it happens today to
> > > > compensate the Rotation (at least that's what I presume happens) ? I
> > > > presume no, applications can infer rotation from properties and if
> they
> > > > chose Identity either other layers above will do the transform or
> > > > they're fine with images as they are.
> > >
> > > If the application says it wants the "identity" transform, then it
> > > should get output images with no transformation applied. But this does
> > > mean it will undo the sensor rotation (this being the normal case on
> > > the Pi).
> > >
> >
> > right, it depends if we decide to account for the sensor rotation
> > inside libcamera or not
> >
> > > >
> > > > A supported/non-supported transform:
> > > > An application asks for a VFLIP, the camera can do it, transform is
> > > > not changed. If the camera cannot flip, the transform is set back to
> > > > Identity and the state is adjusted.
> > >
> > > Yes, mostly, except that the sensor may be mounted with a rotation,
> > > but if it isn't, then I agree with this.
> > >
> > > >
> > > > A partially non-supported transform:
> > > > An application asks for Rot270 (Transpose | HFLIP) and the camera can
> > > > only do HFLIP. transform sould be then set to HFLIP and applications
> > > > know that the remaining transformation should be handled elsewhere.
> > >
> > > As I suggested above, I'm not sure whether we want to mandate this. I
> > > agree it seems useful behaviour, which is why the Pi does it, but I'm
> > > open minded about requiring it or not.
> > >
> > > >
> > > > Are we missing anything with such behaviour ? I presume it's rather
> > > > similar to what happens today if not for the fact that the validate()
> > > > implementation adjusts transform to rotation in case it cannot flip
> > > >
> > > >         Transform combined = transform * data_->rotationTransform_;
> > > >         if (!data_->supportsFlips_ && !!combined)
> > > >                 transform = -data_->rotationTransform_;
> > > >
> > > > Not really sure I got why.
> > > >
> > > > All of this, but an even more fundamental question: is it too late/is
> > > > it worh it to adjust the Transform definition to adopt the same
> > > > direction as the kernel defined rotation ?
> > > >
> > > > Sorry for the long email :/
> > >
> > > No problem! I like nothing more than discussing transforms, except
> > > possibly colour spaces!
> > >
> >
> > :)
> >
> > > More seriously, here are my key points:
> >
> > I'll try to summarize what I see differently
> >
> > >
> > > 1. The CameraConfiguration::transform field says what the application
> > > wants the final output images to have.
> >
> > 1. The CameraConfiguration::transform field says what transform the
> > library has to apply to images as they arrive from the sensor in its
> > default configuration
> >
> > > 2. It takes account of the sensor rotation. The most obvious example
> > > is the Pi: if the sensor is mounted upside down, and the applications
> > > ask for images "the right way up" (i.e. transform = identity), then
> > > the PH will apply h and v flips to compensate for the sensor mounting.
> >
> > 2. It doesn't automatically account for sensor rotation. Application
> > might decide to do that along the rendering pipeline, in example
> >
> > > 3. If the PH can do what the application requests, it leaves the
> > > transform alone and your configuration is Valid.
> >
> > Agreed
> >
> > > 4. If the PH can't do what the application wants, it will set the
> > > value to something that it can do, and the configuration will be
> > > Adjusted.
> >
> > Agreed
> >
> > >
> > > If folks wanted a short conference call to iron any of this out, I'd
> > > be very happy to join in!
> >
> > I've asked others to have a look here, let's see if we need a call,
> > I'm all for it !
> >
> > Thanks
> >   j
> >
> > >
> > > Thanks
> > > David
> > >
> > > >
> > > > > > Thanks everyone!
> > > > > > David
> > > > >
> > > > > Thanks!
> > > > >
> > > > > Robert
> > > > >
>

Hi,

just wanted to drop that

 1. I'm personally not very opinionated about the API design
 2. the new Pipewire implementation matches the current Raspberry Pi code
 3. I think/agree this patch series is the ideal place to
    change/stabilize this API
 4. I'll happily adopt the Pipewire implementation as soon as a
    consensus is found / the implementation lands :)

Best regards and thanks to everyone joining the discussion!

Robert

On 13.12.22 15:59, Naushir Patuck via libcamera-devel wrote:
> Hi all,
>
>
> On Tue, 13 Dec 2022 at 14:30, David Plowman via libcamera-devel 
> <libcamera-devel@lists.libcamera.org> wrote:
>
>     Hi Jacopo
>
>     Thanks for the reply. I won't write a great deal this time round, I
>     agree that possibly a call would be a good idea.
>
>     On Tue, 13 Dec 2022 at 11:22, Jacopo Mondi <jacopo@jmondi.org> wrote:
>     >
>     > Hi David
>     >
>     > On Mon, Dec 12, 2022 at 11:46:31AM +0000, David Plowman wrote:
>     > > Hi Jacopo
>     > >
>     > > Thanks for wading into the sea on this question! (Very
>     appropriate now
>     > > that the stick person example has been turned into a shark...!)
>     > >
>     > > On Fri, 9 Dec 2022 at 12:47, Jacopo Mondi via libcamera-devel
>     > > <libcamera-devel@lists.libcamera.org> wrote:
>     > > >
>     > > > Hi again,
>     > > >
>     > > > On Wed, Dec 07, 2022 at 03:48:40PM +0000, Robert Mader via
>     libcamera-devel wrote:
>     > > > > On 07.12.22 15:28, David Plowman via libcamera-devel wrote:
>     > > > > > Hi Robert
>     > > > > >
>     > > > > > Thanks for prodding us all on this question!
>     > > > >
>     > > > > Hi, thanks for the quick reply! And a pleasure :P
>     > > > >
>     > > > > > I looked back over this again, and found Jacopo's
>     description of the
>     > > > > > V4L2_CID_CAMERA_SENSOR_ROTATION control
>     > > > > >
>     (https://patchwork.kernel.org/project/linux-media/patch/20191204091056.4842-5-jacopo@jmondi.org/
>     > > > > > is what I stumbled into)
>     > > > >
>     > > > > The current kernel docu can be found at
>     > > > >
>     > > > >
>     https://github.com/torvalds/linux/blob/8ed710da2873c2aeb3bb805864a699affaf1d03b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>     > >
>     > > Thanks for the updated reference. The stick person may have turned
>     > > into a shark, but apart from that I think the sense of the
>     > > documentation is unchanged. So if
>     V4L2_CID_CAMERA_SENSOR_ROTATION says
>     > > 90 degrees, then your image in memory looks like it has had a 90
>     > > degree *clockwise* rotation applied.
>     > >
>     >
>     > Yes!
>     >
>     > > > >
>     > > > > the libcamera one at
>     > > > >
>     > > > >
>     https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/transform.cpp#n54
>     > > > >
>     > > > > >  From that, and the nice example with the stick person,
>     I deduce that
>     > > > > > if your camera's V4L2_CID_CAMERA_SENSOR_ROTATION
>     property says 90
>     > > > > > degrees, then the image you capture (if nothing else
>     happens to it)
>     > > > > > will look like it has had a 90 degree clockwise rotation
>     performed.
>     > > > > > Everyone agree with that?
>     > > > > Yep, agreed. That would mean
>     V4L2_CID_CAMERA_SENSOR_ROTATION==90 would
>     > > > > translate to Transform::Rot90.
>     > > >
>     > > > Let's here put some definitions in place
>     > > >
>     > > > V4L2_CID_CAMERA_SENSOR_ROTATION
>     > > > This read-only control describes the rotation correction in
>     degrees in
>     > > > the counter-clockwise direction to be applied to the
>     captured images
>     > > > once captured to memory to compensate for the camera sensor
>     mounting
>     > > > rotation.
>     > > >
>     > > >  \var Transform::Rot270
>     > > >  Rotation by 270 degrees clockwise (90 degrees anticlockwise).
>     > > >
>     > > >  Let's start by saying we did a sub-optimal (to be nice) job in
>     > > >  having Transform and V4L2_CID_ROTATION operate in two different
>     > > >  directions. Before the usage of Transform in libcamera gets
>     widely
>     > > >  adopted sould we align the two ?
>     > >
>     > > I don't mind at all... we just have to decide what we want!
>     > >
>     > > >
>     > > >  Hence if I'm not mistaken, if your camera has Rotation=90
>     and an
>     > > >  application supplies Transform::Identity in
>     > > >  CameraConfiguration::transform what you should get back is
>     Rot270
>     > > >
>     > > >  Correct ?
>     > >
>     > > The way things are currently, if the camera says Rotation=90, then
>     > > your image in memory looks like it is rotated 90 degrees
>     clockwise, so
>     > > the Transform (if your pipeline and sensor can't apply any
>     > > flips/rotations at all) will come back as Rot90.
>     > >
>     > > That is, the only transform you can specify that won't be
>     adjusted, is
>     > > a 90 degree clockwise rotation, so transform=Rot90 as things stand
>     > > currently.
>     > >
>     > > If we choose to change the sense of the libcamera.Transform
>     rotation,
>     > > then we'd get transform=Rot270 instead.
>     > >
>     > >  (If your pipeline handler can do flips and transposes, it may
>     be able
>     > > to do what you ask and so the transform would come back
>     unchanged and
>     > > would be "Valid".)
>     > >
>     > > >
>     > > > > > So in turn, if you have a camera/ISP that can apply no
>     transforms at
>     > > > > > all, then the only value for the user transform that
>     will not come
>     > > > > > back as adjusted is "rot90", because it looks like it's
>     had a 90
>     > > > > > degree clockwise rotation performed (and libcamera
>     transforms count
>     > > > > > rotations as being clockwise). Does that also make sense?
>     > > >
>     > > > It might be the case today, but I wonder if that's desirable.
>     > > >
>     > > > If an application supplies CameraConfiguration::transform =
>     Identity
>     > > > it means it doesn't want any rotation applied by the
>     library. It will
>     > > > get back 270 to indicate that that's how the image is
>     rotated and will
>     > > > get an Adjusted configuration.
>     > >
>     > > I'm not quite sure I get that, perhaps it depends how we interpret
>     > > "applied by the library". I guess I agree if we mean "applied
>     by the
>     > > library together with any rotation of the sensor"? Maybe a real
>     >
>     > "Applied by the library" == happens inside libcamera, ISP or sensor
>     > flips..
>     >
>     > > example is helpful:
>     > >
>     > > So for a Raspberry Pi, for example, our sensors are mostly mounted
>     > > upside down (V4L2_CID_CAMERA_SENSOR_ROTATION says "180"). Our
>     users
>     > > say the transform they want is transform=Identity i.e. images come
>     > > back "the right way up". Internally it means that we do apply
>     an extra
>     > > h+v flip to undo the 180 degree rotation of the sensor.
>     > >
>     >
>     > Here you go, this is where we disconnect.
>     >
>     > As I've interpreted the API so far, and the way we designed
>     > properties::Rotation was to signal to the application what is the
>     > status of the image as it comes from the sensor. Application
>     could then
>     > correct that by setting CameraConfiguration::transform, but in most
>     > cases the image will be corrected at rendering time by the
>     application
>     > itself.
>     >
>     > IWO I was not expecting auto-correction and Transform::Identity
>     to me
>     > means "give me the images as they come from the sensor"
>     >
>     > > >
>     > > > If instead an application supplies
>     CameraConfiguration::transform = Rot270
>     > > > and your camera -cannot- do any rotation, it will still
>     receive back
>     > > > Rot270 and the state is Valid.
>     > >
>     > > I think that's true if we change the sense of the
>     libcamera.Transform
>     > > rotation. Currently, as I said earlier, I think we get Rot90?
>     > >
>     > > >
>     > > > If it asks for CameraConfiguration::transform = Rot270 and
>     the camera
>     > > > -can- do that, it will clear CameraConfiguration::transform to
>     > > > Identity and return Adjusted ?
>     > >
>     > > I'm not sure there... The CameraConfiguration::transform is
>     the final
>     > > transform you want. If that can be done, it's left alone and
>     you get
>     > > back Valid. It's not telling you what else you would need to do
>     > > afterwards.
>     > >
>     > > (To find out what you need to do afterwards you'd work out
>     > > "inverse(what-you-will-get) * what-you-wanted", or something like
>     > > that.)
>     > >
>     >
>     > What I would like is to make as easier as possible for applications
>     > to know what they have to do to have the image rotated in the
>     way they
>     > expect. There are several "transform" steps on the image, the first
>     > one happens in libcamera, all the other ones are left to the
>     > appplication.
>
>     I think this is exactly what I want, namely "to make it as easy as
>     possible for applications".
>
>     On Raspberry Pi - and this has been the case for a couple of years
>     now! - if a user wants images the "normal" way up then they simply
>     pass Transform::Identity. If they want upside down images they pass
>     Transform::Rot180. It's as easy as that, there is simply nothing else
>     to do.
>
>     Requiring all applications to look at the sensor rotation and then
>     combine various things to figure out what they have to ask for (and
>     possibly get it wrong) seems, well, actually quite unhelpful.
>
>
> FWIW, I do agree that applications should not have to trouble themselves
> with munging sensor rotation andrequested rotation to achieve their
> desired outcome.  This definitely wants to be hidden in the libcamera core
> or pipeline handler layers.
>
> Naush
>
>
>     But this is indeed what we need to talk about. Let's organise a call!
>
>     Thanks
>     David
>
>     >
>     > *) The image "native" transform is reported by Rotation (this
>     assumes
>     > that any implicit rotation buried in the sensor driver, as per our
>     > last discussion with Dave is not taken into account)
>     >
>     >    properties::Rotation = Rot270 = HFlip | Transpose
>     >
>     > *) Application sets CameraConfiguration::transform and call
>     validate() to
>     > check what can be done by libcamera and what should be done by next
>     > layers
>     >
>     >    CameraConfigurat::Transform = Rot270
>     >
>     > *) The camera validate the transform and as it can only flip it sets
>     > transform to HFlip, and the state is Adjusted
>     >
>     >    validate() -> The camera can only flip
>     >               -> CameraConfiguration = HFlip
>     >
>     > *) Application gets back what the camera can do and instrument the
>     > rest of the rendering pipeline to perform the additional transforms
>     > the camera cannot do. To get what you have to do I presume you
>     have to
>     >
>     >         additionalTransform = what_you_want & !what_you_get
>     >
>     > So yes, I was interpreting the usage of
>     CameraConfiguration::transform
>     > as a negotiation of what the library can do and what
>     applications have
>     > to do by themselves, which if I got you is different than what's
>     > currecntly implemented ?
>     >
>     > > >
>     > > > Is this what happens (I'm looking at the above code with David's
>     > > > suggestion to use the inverse of roationTransform).
>     > > >
>     > > >
>     > > > > >
>     > > > > > So by a slightly arbitrary mixture of conventions, it
>     looks to me as
>     > > > > > though the use of the inverse that I queried is indeed
>     wrong, because
>     > > > > > we "need camera 90 degree rotation" => "user transform
>     rot90" in this
>     > > > > > case.
>     > > > > >
>     > > > > > Does that sound plausible... what do folks think?
>     > > > >
>     > > > > Ah, I think the question is whether the adjusted value
>     means: "this is what
>     > > > > you'll get" vs "this is what you have to do yourself".
>     > > > >
>     > > > > Above I argued in favor of the later, but I guess the
>     first one is what the
>     > > > > API is supposed to mean. That would imply that the client
>     has compute the
>     > > > > difference between requested and adjusted value itself.
>     > > >
>     > > > I always presumed the latter was meant to happen..
>     > >
>     > > Yes, I think this is the crux of the matter. I believe it's
>     "what you
>     > > will get". It's not "what you need to do after". (If it was
>     "what you
>     > > need to do after", it would change every time you try to
>     validate the
>     > > configuration!)
>     > >
>     >
>     > On the "it would change every time you try to validate the
>     > configuration": only if the camera cannot satisfy the transform you
>     > have requested. If you ask for a flip and the camera can do it,
>     > transform stays un-touched and the state is Valid ?
>     >
>     > > >
>     > > > >
>     > > > > In my case that would mean:
>     > > > >
>     > > > > requested CameraConfiguration::transform: Transform::Identity,
>     > > > > V4L2_CID_CAMERA_SENSOR_ROTATION: 270, adjusted
>     > > > > CameraConfiguration::transform: Transform::Rot270
>     > > > >
>     > > >
>     > > > I guess it's Rot90 because of the
>     clockwise/counter-clockwise issue ?
>     > >
>     > > Yes, looks right to me!
>     > >
>     > > >
>     > > > > To compute what the client has to do, it would need to do:
>     > > > > Transform::Identity - Transform::Rot270 =
>     Transform::Rot90. And then apply
>     > > > > that, a 90 degr. rotation clockwise.
>     > >
>     > > Yes, I guess I wrote it above, it's "inverse(what-you-got) *
>     > > what-you-wanted", in this case inverse(rot270) * identity which is
>     > > rot90 (in the current clockwise sense).
>     > >
>     > > > >
>     > > > > Does that sound reasonable? In that case the patch here
>     should indeed be
>     > > > > `*transform = rotationTransform;`, without the inverse.
>     > > > >
>     > >
>     > > Yes, agree!
>     > >
>     > > >
>     > > >         /0\
>     > > >
>     > > > before we plumb this into and make assumptions that will be then
>     > > > set into stone in our adaption layers, can we take a step
>     back and
>     > > > define what kind of API we would like to have ?
>     > > >
>     > > > Let's start by the definition of CameraConfiguration::transform.
>     > > >
>     > > > It's a member of CameraConfiguration, and as by definition it is
>     > > > filled-in by the application and eventually adjusted by the
>     Camera to
>     > > > a value that's acceptable/doable for the current configuration.
>     > > >
>     > > > 1) Application to Camera  = (counter?)clockwise degrees the
>     camera
>     > > > system should rotate the image before presenting it to the user
>     > >
>     > > This may depend on what exactly we mean by "the camera
>     system". I'd
>     > > explain it as
>     > > "the final resulting transform that the application wants to be
>     > > applied to all the output images". After all, this is actually the
>     > > only thing the application knows!
>     > >
>     >
>     > agreed
>     >
>     > > >
>     > > > 2) Camera to application = the rotation the camera can
>     actually do
>     > > > (ie. only sensor flips == no Transpose)
>     > >
>     > > Agree, mostly, though I would say "transform" rather than
>     "rotation"
>     >
>     > yes, more correct
>     >
>     > > because flips aren't rotations, you could in theory have a PH that
>     > > could do transposes (even though we probably don't currently).
>     > >
>     >
>     > none currently afaict
>     >
>     > > >
>     > > > The current documentation doesn't explain how it gets adjusted
>     > >
>     > > This is true, though I also think it's platform dependent. If
>     you had
>     > > a PH that can't do transposes but can do flips, you have a
>     choice how
>     > > you handle (for example) rot90. You could say "oh I can't do
>     that, so
>     > > in this case I won't do anything at all" and leave the
>     application to
>     > > sort everything out. The Pi tries to say "well, I'll do everything
>     > > except for the transpose, so an application only has to worry
>     about
>     > > that one case". But there is a choice...
>     > >
>     >
>     > I agree it's platform-specific, but the way transform is
>     negotiated with
>     > application should be standardized
>     >
>     > > >
>     > > > /**
>     > > >  * \var CameraConfiguration::transform
>     > > >  * \brief User-specified transform to be applied to the image
>     > > >  *
>     > > >  * The transform is a user-specified 2D plane transform that
>     will be applied
>     > > >  * to the camera images by the processing pipeline before
>     being handed to
>     > > >  * the application. This is subsequent to any transform that
>     is already
>     > > >  * required to fix up any platform-defined rotation.
>     > > >  *
>     > > >  * The usual 2D plane transforms are allowed here
>     (horizontal/vertical
>     > > >  * flips, multiple of 90-degree rotations etc.), but the
>     validate() function
>     > > >  * may adjust this field at its discretion if the selection
>     is not supported.
>     > > >  */
>     > > >
>     > > > The last part in particular, "at its discrection" it's
>     really too
>     > > > generic.
>     > >
>     > > I guess it's hinting at the fact that it's up to the PH, and
>     we don't
>     > > mandate the exact way in which it can fail to do what you
>     asked for!
>     > > But it would be fine to have a discussion on that.
>     > >
>     >
>     > That's the point that most concerns me :)
>     >
>     > > >
>     > > > Let's start from the beginning: what application should use
>     transform
>     > > > for ? I presume they should:
>     > > >
>     > > > 1) Inspect propertis::Rotation
>     > > > 2) Correct the image for the properties::Rotation amount
>     (actually,
>     > > > the inverse because of the clockwise/counter-clockwise thing)
>     > > > 3) Add any additional rotation they would like on top of this
>     > >
>     > > So I think the point of the transform is that you *don't* have to
>     > > worry about the properties::Rotation. It takes care of that
>     for you.
>     > > You say "I want upside down images" and you get them, whatever the
>     > > sensor's rotation.
>     >
>     > That's acceptable as well if we prefer that, but it has to be
>     > standardized among pipelines imho
>     >
>     > >
>     > > >
>     > > > How should transform be adjusted ?
>     > > >
>     > > > Identity:
>     > > > If application->identity the camera does not have to apply any
>     > > > transform. Should the transform be adjutes like it happens
>     today to
>     > > > compensate the Rotation (at least that's what I presume
>     happens) ? I
>     > > > presume no, applications can infer rotation from properties
>     and if they
>     > > > chose Identity either other layers above will do the
>     transform or
>     > > > they're fine with images as they are.
>     > >
>     > > If the application says it wants the "identity" transform, then it
>     > > should get output images with no transformation applied. But
>     this does
>     > > mean it will undo the sensor rotation (this being the normal
>     case on
>     > > the Pi).
>     > >
>     >
>     > right, it depends if we decide to account for the sensor rotation
>     > inside libcamera or not
>     >
>     > > >
>     > > > A supported/non-supported transform:
>     > > > An application asks for a VFLIP, the camera can do it,
>     transform is
>     > > > not changed. If the camera cannot flip, the transform is set
>     back to
>     > > > Identity and the state is adjusted.
>     > >
>     > > Yes, mostly, except that the sensor may be mounted with a
>     rotation,
>     > > but if it isn't, then I agree with this.
>     > >
>     > > >
>     > > > A partially non-supported transform:
>     > > > An application asks for Rot270 (Transpose | HFLIP) and the
>     camera can
>     > > > only do HFLIP. transform sould be then set to HFLIP and
>     applications
>     > > > know that the remaining transformation should be handled
>     elsewhere.
>     > >
>     > > As I suggested above, I'm not sure whether we want to mandate
>     this. I
>     > > agree it seems useful behaviour, which is why the Pi does it,
>     but I'm
>     > > open minded about requiring it or not.
>     > >
>     > > >
>     > > > Are we missing anything with such behaviour ? I presume it's
>     rather
>     > > > similar to what happens today if not for the fact that the
>     validate()
>     > > > implementation adjusts transform to rotation in case it
>     cannot flip
>     > > >
>     > > >         Transform combined = transform *
>     data_->rotationTransform_;
>     > > >         if (!data_->supportsFlips_ && !!combined)
>     > > >                 transform = -data_->rotationTransform_;
>     > > >
>     > > > Not really sure I got why.
>     > > >
>     > > > All of this, but an even more fundamental question: is it
>     too late/is
>     > > > it worh it to adjust the Transform definition to adopt the same
>     > > > direction as the kernel defined rotation ?
>     > > >
>     > > > Sorry for the long email :/
>     > >
>     > > No problem! I like nothing more than discussing transforms, except
>     > > possibly colour spaces!
>     > >
>     >
>     > :)
>     >
>     > > More seriously, here are my key points:
>     >
>     > I'll try to summarize what I see differently
>     >
>     > >
>     > > 1. The CameraConfiguration::transform field says what the
>     application
>     > > wants the final output images to have.
>     >
>     > 1. The CameraConfiguration::transform field says what transform the
>     > library has to apply to images as they arrive from the sensor in its
>     > default configuration
>     >
>     > > 2. It takes account of the sensor rotation. The most obvious
>     example
>     > > is the Pi: if the sensor is mounted upside down, and the
>     applications
>     > > ask for images "the right way up" (i.e. transform = identity),
>     then
>     > > the PH will apply h and v flips to compensate for the sensor
>     mounting.
>     >
>     > 2. It doesn't automatically account for sensor rotation. Application
>     > might decide to do that along the rendering pipeline, in example
>     >
>     > > 3. If the PH can do what the application requests, it leaves the
>     > > transform alone and your configuration is Valid.
>     >
>     > Agreed
>     >
>     > > 4. If the PH can't do what the application wants, it will set the
>     > > value to something that it can do, and the configuration will be
>     > > Adjusted.
>     >
>     > Agreed
>     >
>     > >
>     > > If folks wanted a short conference call to iron any of this
>     out, I'd
>     > > be very happy to join in!
>     >
>     > I've asked others to have a look here, let's see if we need a call,
>     > I'm all for it !
>     >
>     > Thanks
>     >   j
>     >
>     > >
>     > > Thanks
>     > > David
>     > >
>     > > >
>     > > > > > Thanks everyone!
>     > > > > > David
>     > > > >
>     > > > > Thanks!
>     > > > >
>     > > > > Robert
>     > > > >
>

Hi everyone

I wanted to give this discussion a little nudge because I don't think
it was ever quite concluded, perhaps Christmas got in the way!

To summarise a little bit, at least from my point of view, my
impression was that the current approach is correct, and the transform
field is for the user to request the transform that they want, and may
get adjusted to be the transform that they will get. Details of any
rotation in the way the sensor is mounted is hidden from the user -
they can just say "I want the image the right way up" and it
"magically" happens. (This is exactly how the Raspberry Pi pipeline
handler works today.)

There was a small detail about whether one of the transforms in the
code wanted reversing, that's mostly a matter of conventions about
using clockwise or anticlockwise rotations (or the direction of the
rotation axis).

Does anyone have anything else to add here?

Thanks!
David

On Wed, 14 Dec 2022 at 15:55, Robert Mader via libcamera-devel
<libcamera-devel@lists.libcamera.org> wrote:
>
> Hi,
>
> just wanted to drop that
>
> I'm personally not very opinionated about the API design
> the new Pipewire implementation matches the current Raspberry Pi code
> I think/agree this patch series is the ideal place to change/stabilize this API
> I'll happily adopt the Pipewire implementation as soon as a consensus is found / the implementation lands :)
>
> Best regards and thanks to everyone joining the discussion!
>
> Robert
>
> On 13.12.22 15:59, Naushir Patuck via libcamera-devel wrote:
>
> Hi all,
>
>
> On Tue, 13 Dec 2022 at 14:30, David Plowman via libcamera-devel <libcamera-devel@lists.libcamera.org> wrote:
>>
>> Hi Jacopo
>>
>> Thanks for the reply. I won't write a great deal this time round, I
>> agree that possibly a call would be a good idea.
>>
>> On Tue, 13 Dec 2022 at 11:22, Jacopo Mondi <jacopo@jmondi.org> wrote:
>> >
>> > Hi David
>> >
>> > On Mon, Dec 12, 2022 at 11:46:31AM +0000, David Plowman wrote:
>> > > Hi Jacopo
>> > >
>> > > Thanks for wading into the sea on this question! (Very appropriate now
>> > > that the stick person example has been turned into a shark...!)
>> > >
>> > > On Fri, 9 Dec 2022 at 12:47, Jacopo Mondi via libcamera-devel
>> > > <libcamera-devel@lists.libcamera.org> wrote:
>> > > >
>> > > > Hi again,
>> > > >
>> > > > On Wed, Dec 07, 2022 at 03:48:40PM +0000, Robert Mader via libcamera-devel wrote:
>> > > > > On 07.12.22 15:28, David Plowman via libcamera-devel wrote:
>> > > > > > Hi Robert
>> > > > > >
>> > > > > > Thanks for prodding us all on this question!
>> > > > >
>> > > > > Hi, thanks for the quick reply! And a pleasure :P
>> > > > >
>> > > > > > I looked back over this again, and found Jacopo's description of the
>> > > > > > V4L2_CID_CAMERA_SENSOR_ROTATION control
>> > > > > > (https://patchwork.kernel.org/project/linux-media/patch/20191204091056.4842-5-jacopo@jmondi.org/
>> > > > > > is what I stumbled into)
>> > > > >
>> > > > > The current kernel docu can be found at
>> > > > >
>> > > > > https://github.com/torvalds/linux/blob/8ed710da2873c2aeb3bb805864a699affaf1d03b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>> > >
>> > > Thanks for the updated reference. The stick person may have turned
>> > > into a shark, but apart from that I think the sense of the
>> > > documentation is unchanged. So if V4L2_CID_CAMERA_SENSOR_ROTATION says
>> > > 90 degrees, then your image in memory looks like it has had a 90
>> > > degree *clockwise* rotation applied.
>> > >
>> >
>> > Yes!
>> >
>> > > > >
>> > > > > the libcamera one at
>> > > > >
>> > > > > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/transform.cpp#n54
>> > > > >
>> > > > > >  From that, and the nice example with the stick person, I deduce that
>> > > > > > if your camera's V4L2_CID_CAMERA_SENSOR_ROTATION property says 90
>> > > > > > degrees, then the image you capture (if nothing else happens to it)
>> > > > > > will look like it has had a 90 degree clockwise rotation performed.
>> > > > > > Everyone agree with that?
>> > > > > Yep, agreed. That would mean V4L2_CID_CAMERA_SENSOR_ROTATION==90 would
>> > > > > translate to Transform::Rot90.
>> > > >
>> > > > Let's here put some definitions in place
>> > > >
>> > > > V4L2_CID_CAMERA_SENSOR_ROTATION
>> > > > This read-only control describes the rotation correction in degrees in
>> > > > the counter-clockwise direction to be applied to the captured images
>> > > > once captured to memory to compensate for the camera sensor mounting
>> > > > rotation.
>> > > >
>> > > >  \var Transform::Rot270
>> > > >  Rotation by 270 degrees clockwise (90 degrees anticlockwise).
>> > > >
>> > > >  Let's start by saying we did a sub-optimal (to be nice) job in
>> > > >  having Transform and V4L2_CID_ROTATION operate in two different
>> > > >  directions. Before the usage of Transform in libcamera gets widely
>> > > >  adopted sould we align the two ?
>> > >
>> > > I don't mind at all... we just have to decide what we want!
>> > >
>> > > >
>> > > >  Hence if I'm not mistaken, if your camera has Rotation=90 and an
>> > > >  application supplies Transform::Identity in
>> > > >  CameraConfiguration::transform what you should get back is Rot270
>> > > >
>> > > >  Correct ?
>> > >
>> > > The way things are currently, if the camera says Rotation=90, then
>> > > your image in memory looks like it is rotated 90 degrees clockwise, so
>> > > the Transform (if your pipeline and sensor can't apply any
>> > > flips/rotations at all) will come back as Rot90.
>> > >
>> > > That is, the only transform you can specify that won't be adjusted, is
>> > > a 90 degree clockwise rotation, so transform=Rot90 as things stand
>> > > currently.
>> > >
>> > > If we choose to change the sense of the libcamera.Transform rotation,
>> > > then we'd get transform=Rot270 instead.
>> > >
>> > >  (If your pipeline handler can do flips and transposes, it may be able
>> > > to do what you ask and so the transform would come back unchanged and
>> > > would be "Valid".)
>> > >
>> > > >
>> > > > > > So in turn, if you have a camera/ISP that can apply no transforms at
>> > > > > > all, then the only value for the user transform that will not come
>> > > > > > back as adjusted is "rot90", because it looks like it's had a 90
>> > > > > > degree clockwise rotation performed (and libcamera transforms count
>> > > > > > rotations as being clockwise). Does that also make sense?
>> > > >
>> > > > It might be the case today, but I wonder if that's desirable.
>> > > >
>> > > > If an application supplies CameraConfiguration::transform = Identity
>> > > > it means it doesn't want any rotation applied by the library. It will
>> > > > get back 270 to indicate that that's how the image is rotated and will
>> > > > get an Adjusted configuration.
>> > >
>> > > I'm not quite sure I get that, perhaps it depends how we interpret
>> > > "applied by the library". I guess I agree if we mean "applied by the
>> > > library together with any rotation of the sensor"? Maybe a real
>> >
>> > "Applied by the library" == happens inside libcamera, ISP or sensor
>> > flips..
>> >
>> > > example is helpful:
>> > >
>> > > So for a Raspberry Pi, for example, our sensors are mostly mounted
>> > > upside down (V4L2_CID_CAMERA_SENSOR_ROTATION says "180"). Our users
>> > > say the transform they want is transform=Identity i.e. images come
>> > > back "the right way up". Internally it means that we do apply an extra
>> > > h+v flip to undo the 180 degree rotation of the sensor.
>> > >
>> >
>> > Here you go, this is where we disconnect.
>> >
>> > As I've interpreted the API so far, and the way we designed
>> > properties::Rotation was to signal to the application what is the
>> > status of the image as it comes from the sensor. Application could then
>> > correct that by setting CameraConfiguration::transform, but in most
>> > cases the image will be corrected at rendering time by the application
>> > itself.
>> >
>> > IWO I was not expecting auto-correction and Transform::Identity to me
>> > means "give me the images as they come from the sensor"
>> >
>> > > >
>> > > > If instead an application supplies CameraConfiguration::transform = Rot270
>> > > > and your camera -cannot- do any rotation, it will still receive back
>> > > > Rot270 and the state is Valid.
>> > >
>> > > I think that's true if we change the sense of the libcamera.Transform
>> > > rotation. Currently, as I said earlier, I think we get Rot90?
>> > >
>> > > >
>> > > > If it asks for CameraConfiguration::transform = Rot270 and the camera
>> > > > -can- do that, it will clear CameraConfiguration::transform to
>> > > > Identity and return Adjusted ?
>> > >
>> > > I'm not sure there... The CameraConfiguration::transform is the final
>> > > transform you want. If that can be done, it's left alone and you get
>> > > back Valid. It's not telling you what else you would need to do
>> > > afterwards.
>> > >
>> > > (To find out what you need to do afterwards you'd work out
>> > > "inverse(what-you-will-get) * what-you-wanted", or something like
>> > > that.)
>> > >
>> >
>> > What I would like is to make as easier as possible for applications
>> > to know what they have to do to have the image rotated in the way they
>> > expect. There are several "transform" steps on the image, the first
>> > one happens in libcamera, all the other ones are left to the
>> > appplication.
>>
>> I think this is exactly what I want, namely "to make it as easy as
>> possible for applications".
>>
>> On Raspberry Pi - and this has been the case for a couple of years
>> now! - if a user wants images the "normal" way up then they simply
>> pass Transform::Identity. If they want upside down images they pass
>> Transform::Rot180. It's as easy as that, there is simply nothing else
>> to do.
>>
>> Requiring all applications to look at the sensor rotation and then
>> combine various things to figure out what they have to ask for (and
>> possibly get it wrong) seems, well, actually quite unhelpful.
>
>
> FWIW, I do agree that applications should not have to trouble themselves
> with munging sensor rotation andrequested rotation to achieve their
> desired outcome.  This definitely wants to be hidden in the libcamera core
> or pipeline handler layers.
>
> Naush
>
>
>>
>>
>> But this is indeed what we need to talk about. Let's organise a call!
>>
>> Thanks
>> David
>>
>> >
>> > *) The image "native" transform is reported by Rotation (this assumes
>> > that any implicit rotation buried in the sensor driver, as per our
>> > last discussion with Dave is not taken into account)
>> >
>> >    properties::Rotation = Rot270 = HFlip | Transpose
>> >
>> > *) Application sets CameraConfiguration::transform and call validate() to
>> > check what can be done by libcamera and what should be done by next
>> > layers
>> >
>> >    CameraConfigurat::Transform = Rot270
>> >
>> > *) The camera validate the transform and as it can only flip it sets
>> > transform to HFlip, and the state is Adjusted
>> >
>> >    validate() -> The camera can only flip
>> >               -> CameraConfiguration = HFlip
>> >
>> > *) Application gets back what the camera can do and instrument the
>> > rest of the rendering pipeline to perform the additional transforms
>> > the camera cannot do. To get what you have to do I presume you have to
>> >
>> >         additionalTransform = what_you_want & !what_you_get
>> >
>> > So yes, I was interpreting the usage of CameraConfiguration::transform
>> > as a negotiation of what the library can do and what applications have
>> > to do by themselves, which if I got you is different than what's
>> > currecntly implemented ?
>> >
>> > > >
>> > > > Is this what happens (I'm looking at the above code with David's
>> > > > suggestion to use the inverse of roationTransform).
>> > > >
>> > > >
>> > > > > >
>> > > > > > So by a slightly arbitrary mixture of conventions, it looks to me as
>> > > > > > though the use of the inverse that I queried is indeed wrong, because
>> > > > > > we "need camera 90 degree rotation" => "user transform rot90" in this
>> > > > > > case.
>> > > > > >
>> > > > > > Does that sound plausible... what do folks think?
>> > > > >
>> > > > > Ah, I think the question is whether the adjusted value means: "this is what
>> > > > > you'll get" vs "this is what you have to do yourself".
>> > > > >
>> > > > > Above I argued in favor of the later, but I guess the first one is what the
>> > > > > API is supposed to mean. That would imply that the client has compute the
>> > > > > difference between requested and adjusted value itself.
>> > > >
>> > > > I always presumed the latter was meant to happen..
>> > >
>> > > Yes, I think this is the crux of the matter. I believe it's "what you
>> > > will get". It's not "what you need to do after". (If it was "what you
>> > > need to do after", it would change every time you try to validate the
>> > > configuration!)
>> > >
>> >
>> > On the "it would change every time you try to validate the
>> > configuration": only if the camera cannot satisfy the transform you
>> > have requested. If you ask for a flip and the camera can do it,
>> > transform stays un-touched and the state is Valid ?
>> >
>> > > >
>> > > > >
>> > > > > In my case that would mean:
>> > > > >
>> > > > > requested CameraConfiguration::transform: Transform::Identity,
>> > > > > V4L2_CID_CAMERA_SENSOR_ROTATION: 270, adjusted
>> > > > > CameraConfiguration::transform: Transform::Rot270
>> > > > >
>> > > >
>> > > > I guess it's Rot90 because of the clockwise/counter-clockwise issue ?
>> > >
>> > > Yes, looks right to me!
>> > >
>> > > >
>> > > > > To compute what the client has to do, it would need to do:
>> > > > > Transform::Identity - Transform::Rot270 = Transform::Rot90. And then apply
>> > > > > that, a 90 degr. rotation clockwise.
>> > >
>> > > Yes, I guess I wrote it above, it's "inverse(what-you-got) *
>> > > what-you-wanted", in this case inverse(rot270) * identity which is
>> > > rot90 (in the current clockwise sense).
>> > >
>> > > > >
>> > > > > Does that sound reasonable? In that case the patch here should indeed be
>> > > > > `*transform = rotationTransform;`, without the inverse.
>> > > > >
>> > >
>> > > Yes, agree!
>> > >
>> > > >
>> > > >         /0\
>> > > >
>> > > > before we plumb this into and make assumptions that will be then
>> > > > set into stone in our adaption layers, can we take a step back and
>> > > > define what kind of API we would like to have ?
>> > > >
>> > > > Let's start by the definition of CameraConfiguration::transform.
>> > > >
>> > > > It's a member of CameraConfiguration, and as by definition it is
>> > > > filled-in by the application and eventually adjusted by the Camera to
>> > > > a value that's acceptable/doable for the current configuration.
>> > > >
>> > > > 1) Application to Camera  = (counter?)clockwise degrees the camera
>> > > > system should rotate the image before presenting it to the user
>> > >
>> > > This may depend on what exactly we mean by "the camera system". I'd
>> > > explain it as
>> > > "the final resulting transform that the application wants to be
>> > > applied to all the output images". After all, this is actually the
>> > > only thing the application knows!
>> > >
>> >
>> > agreed
>> >
>> > > >
>> > > > 2) Camera to application = the rotation the camera can actually do
>> > > > (ie. only sensor flips == no Transpose)
>> > >
>> > > Agree, mostly, though I would say "transform" rather than "rotation"
>> >
>> > yes, more correct
>> >
>> > > because flips aren't rotations, you could in theory have a PH that
>> > > could do transposes (even though we probably don't currently).
>> > >
>> >
>> > none currently afaict
>> >
>> > > >
>> > > > The current documentation doesn't explain how it gets adjusted
>> > >
>> > > This is true, though I also think it's platform dependent. If you had
>> > > a PH that can't do transposes but can do flips, you have a choice how
>> > > you handle (for example) rot90. You could say "oh I can't do that, so
>> > > in this case I won't do anything at all" and leave the application to
>> > > sort everything out. The Pi tries to say "well, I'll do everything
>> > > except for the transpose, so an application only has to worry about
>> > > that one case". But there is a choice...
>> > >
>> >
>> > I agree it's platform-specific, but the way transform is negotiated with
>> > application should be standardized
>> >
>> > > >
>> > > > /**
>> > > >  * \var CameraConfiguration::transform
>> > > >  * \brief User-specified transform to be applied to the image
>> > > >  *
>> > > >  * The transform is a user-specified 2D plane transform that will be applied
>> > > >  * to the camera images by the processing pipeline before being handed to
>> > > >  * the application. This is subsequent to any transform that is already
>> > > >  * required to fix up any platform-defined rotation.
>> > > >  *
>> > > >  * The usual 2D plane transforms are allowed here (horizontal/vertical
>> > > >  * flips, multiple of 90-degree rotations etc.), but the validate() function
>> > > >  * may adjust this field at its discretion if the selection is not supported.
>> > > >  */
>> > > >
>> > > > The last part in particular, "at its discrection" it's really too
>> > > > generic.
>> > >
>> > > I guess it's hinting at the fact that it's up to the PH, and we don't
>> > > mandate the exact way in which it can fail to do what you asked for!
>> > > But it would be fine to have a discussion on that.
>> > >
>> >
>> > That's the point that most concerns me :)
>> >
>> > > >
>> > > > Let's start from the beginning: what application should use transform
>> > > > for ? I presume they should:
>> > > >
>> > > > 1) Inspect propertis::Rotation
>> > > > 2) Correct the image for the properties::Rotation amount (actually,
>> > > > the inverse because of the clockwise/counter-clockwise thing)
>> > > > 3) Add any additional rotation they would like on top of this
>> > >
>> > > So I think the point of the transform is that you *don't* have to
>> > > worry about the properties::Rotation. It takes care of that for you.
>> > > You say "I want upside down images" and you get them, whatever the
>> > > sensor's rotation.
>> >
>> > That's acceptable as well if we prefer that, but it has to be
>> > standardized among pipelines imho
>> >
>> > >
>> > > >
>> > > > How should transform be adjusted ?
>> > > >
>> > > > Identity:
>> > > > If application->identity the camera does not have to apply any
>> > > > transform. Should the transform be adjutes like it happens today to
>> > > > compensate the Rotation (at least that's what I presume happens) ? I
>> > > > presume no, applications can infer rotation from properties and if they
>> > > > chose Identity either other layers above will do the transform or
>> > > > they're fine with images as they are.
>> > >
>> > > If the application says it wants the "identity" transform, then it
>> > > should get output images with no transformation applied. But this does
>> > > mean it will undo the sensor rotation (this being the normal case on
>> > > the Pi).
>> > >
>> >
>> > right, it depends if we decide to account for the sensor rotation
>> > inside libcamera or not
>> >
>> > > >
>> > > > A supported/non-supported transform:
>> > > > An application asks for a VFLIP, the camera can do it, transform is
>> > > > not changed. If the camera cannot flip, the transform is set back to
>> > > > Identity and the state is adjusted.
>> > >
>> > > Yes, mostly, except that the sensor may be mounted with a rotation,
>> > > but if it isn't, then I agree with this.
>> > >
>> > > >
>> > > > A partially non-supported transform:
>> > > > An application asks for Rot270 (Transpose | HFLIP) and the camera can
>> > > > only do HFLIP. transform sould be then set to HFLIP and applications
>> > > > know that the remaining transformation should be handled elsewhere.
>> > >
>> > > As I suggested above, I'm not sure whether we want to mandate this. I
>> > > agree it seems useful behaviour, which is why the Pi does it, but I'm
>> > > open minded about requiring it or not.
>> > >
>> > > >
>> > > > Are we missing anything with such behaviour ? I presume it's rather
>> > > > similar to what happens today if not for the fact that the validate()
>> > > > implementation adjusts transform to rotation in case it cannot flip
>> > > >
>> > > >         Transform combined = transform * data_->rotationTransform_;
>> > > >         if (!data_->supportsFlips_ && !!combined)
>> > > >                 transform = -data_->rotationTransform_;
>> > > >
>> > > > Not really sure I got why.
>> > > >
>> > > > All of this, but an even more fundamental question: is it too late/is
>> > > > it worh it to adjust the Transform definition to adopt the same
>> > > > direction as the kernel defined rotation ?
>> > > >
>> > > > Sorry for the long email :/
>> > >
>> > > No problem! I like nothing more than discussing transforms, except
>> > > possibly colour spaces!
>> > >
>> >
>> > :)
>> >
>> > > More seriously, here are my key points:
>> >
>> > I'll try to summarize what I see differently
>> >
>> > >
>> > > 1. The CameraConfiguration::transform field says what the application
>> > > wants the final output images to have.
>> >
>> > 1. The CameraConfiguration::transform field says what transform the
>> > library has to apply to images as they arrive from the sensor in its
>> > default configuration
>> >
>> > > 2. It takes account of the sensor rotation. The most obvious example
>> > > is the Pi: if the sensor is mounted upside down, and the applications
>> > > ask for images "the right way up" (i.e. transform = identity), then
>> > > the PH will apply h and v flips to compensate for the sensor mounting.
>> >
>> > 2. It doesn't automatically account for sensor rotation. Application
>> > might decide to do that along the rendering pipeline, in example
>> >
>> > > 3. If the PH can do what the application requests, it leaves the
>> > > transform alone and your configuration is Valid.
>> >
>> > Agreed
>> >
>> > > 4. If the PH can't do what the application wants, it will set the
>> > > value to something that it can do, and the configuration will be
>> > > Adjusted.
>> >
>> > Agreed
>> >
>> > >
>> > > If folks wanted a short conference call to iron any of this out, I'd
>> > > be very happy to join in!
>> >
>> > I've asked others to have a look here, let's see if we need a call,
>> > I'm all for it !
>> >
>> > Thanks
>> >   j
>> >
>> > >
>> > > Thanks
>> > > David
>> > >
>> > > >
>> > > > > > Thanks everyone!
>> > > > > > David
>> > > > >
>> > > > > Thanks!
>> > > > >
>> > > > > Robert
>> > > > >

Hi David,
        cc Sakari for context about the imx258 sensor patches

On Thu, Jan 12, 2023 at 11:18:51AM +0000, David Plowman wrote:
> Hi everyone
>
> I wanted to give this discussion a little nudge because I don't think
> it was ever quite concluded, perhaps Christmas got in the way!
>
> To summarise a little bit, at least from my point of view, my
> impression was that the current approach is correct, and the transform
> field is for the user to request the transform that they want, and may
> get adjusted to be the transform that they will get. Details of any
> rotation in the way the sensor is mounted is hidden from the user -
> they can just say "I want the image the right way up" and it
> "magically" happens. (This is exactly how the Raspberry Pi pipeline
> handler works today.)

I have been able to discuss a bit with others, and I'm here trying to
summarize the outcome

* Recap of the assumptions:

From the kernel interface we have:
- Physical mounting rotation (V4L2_CID_CAMERA_SENSOR_ROTATION)
- Sensor flips capabilities (which depends on the V4L2_CID_[H|F]LIP
  presence and if they are RW)

It is fair to assume that:
- Sensors cannot do transpose (flip along the diagonal axis)
- They can only HVFLIP (ie flip, mirror or 180 degrees rotate)

The ISP can also do transforms like transpose or even rotate, but
compared to acting flips on the sensor this transformation does not
change the row/col reading order in the image (the first pixels
visible in the will end up being the ones read last) which might have
an impact for rolling shutter sensors.

* Proposed implementation

libcamera retrieves the mounting rotation, and if it can fully
compensate it by setting sensor's flips (or if flips are already hardcoded to
1 as it happens in some drivers) it will adjust properties::Rotation
to report that the image will be rotated upright and application
shouldn't care. This is what happens today with your implementation of
the Transform handling part.

This also implies that sensor's can correct only 180 degrees rotation
and everything else will be deferred to
1) other processing layers
2) additional transforms in the ISP if the pipeline supports those

If you're ok with such plan I propose:

- let's land my series of 5 patches that centralize flip handling in
  CameraSensor but does not introduce functional changes
- let's on top make the CameraSensor class register property::Rotation
  compensated by the sensor (basically only for 180deg correction)
- let's adjust CameraSensor::setFormat() to apply flips correctly and
  remove transpose handling from there but only handle 180 degrees
  transformations.

For RPi it means 180deg rotation will be automatically compensated as
they're today by sensor's flips. The only difference is that your
consumers will see a properties::Rotation = 0 instead of 180, as for
them the image is already 'correct'.

For other devices with different rotations which require transpose
(as 90/270 deg, typical for smartphones) the whole rotation adjustment
will be performed by the application's upper layers.

Does this sound viable to you ?

>
> There was a small detail about whether one of the transforms in the
> code wanted reversing, that's mostly a matter of conventions about
> using clockwise or anticlockwise rotations (or the direction of the
> rotation axis).
>
> Does anyone have anything else to add here?
>
> Thanks!
> David
>
> On Wed, 14 Dec 2022 at 15:55, Robert Mader via libcamera-devel
> <libcamera-devel@lists.libcamera.org> wrote:
> >
> > Hi,
> >
> > just wanted to drop that
> >
> > I'm personally not very opinionated about the API design
> > the new Pipewire implementation matches the current Raspberry Pi code
> > I think/agree this patch series is the ideal place to change/stabilize this API
> > I'll happily adopt the Pipewire implementation as soon as a consensus is found / the implementation lands :)
> >
> > Best regards and thanks to everyone joining the discussion!
> >
> > Robert
> >
> > On 13.12.22 15:59, Naushir Patuck via libcamera-devel wrote:
> >
> > Hi all,
> >
> >
> > On Tue, 13 Dec 2022 at 14:30, David Plowman via libcamera-devel <libcamera-devel@lists.libcamera.org> wrote:
> >>
> >> Hi Jacopo
> >>
> >> Thanks for the reply. I won't write a great deal this time round, I
> >> agree that possibly a call would be a good idea.
> >>
> >> On Tue, 13 Dec 2022 at 11:22, Jacopo Mondi <jacopo@jmondi.org> wrote:
> >> >
> >> > Hi David
> >> >
> >> > On Mon, Dec 12, 2022 at 11:46:31AM +0000, David Plowman wrote:
> >> > > Hi Jacopo
> >> > >
> >> > > Thanks for wading into the sea on this question! (Very appropriate now
> >> > > that the stick person example has been turned into a shark...!)
> >> > >
> >> > > On Fri, 9 Dec 2022 at 12:47, Jacopo Mondi via libcamera-devel
> >> > > <libcamera-devel@lists.libcamera.org> wrote:
> >> > > >
> >> > > > Hi again,
> >> > > >
> >> > > > On Wed, Dec 07, 2022 at 03:48:40PM +0000, Robert Mader via libcamera-devel wrote:
> >> > > > > On 07.12.22 15:28, David Plowman via libcamera-devel wrote:
> >> > > > > > Hi Robert
> >> > > > > >
> >> > > > > > Thanks for prodding us all on this question!
> >> > > > >
> >> > > > > Hi, thanks for the quick reply! And a pleasure :P
> >> > > > >
> >> > > > > > I looked back over this again, and found Jacopo's description of the
> >> > > > > > V4L2_CID_CAMERA_SENSOR_ROTATION control
> >> > > > > > (https://patchwork.kernel.org/project/linux-media/patch/20191204091056.4842-5-jacopo@jmondi.org/
> >> > > > > > is what I stumbled into)
> >> > > > >
> >> > > > > The current kernel docu can be found at
> >> > > > >
> >> > > > > https://github.com/torvalds/linux/blob/8ed710da2873c2aeb3bb805864a699affaf1d03b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> >> > >
> >> > > Thanks for the updated reference. The stick person may have turned
> >> > > into a shark, but apart from that I think the sense of the
> >> > > documentation is unchanged. So if V4L2_CID_CAMERA_SENSOR_ROTATION says
> >> > > 90 degrees, then your image in memory looks like it has had a 90
> >> > > degree *clockwise* rotation applied.
> >> > >
> >> >
> >> > Yes!
> >> >
> >> > > > >
> >> > > > > the libcamera one at
> >> > > > >
> >> > > > > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/transform.cpp#n54
> >> > > > >
> >> > > > > >  From that, and the nice example with the stick person, I deduce that
> >> > > > > > if your camera's V4L2_CID_CAMERA_SENSOR_ROTATION property says 90
> >> > > > > > degrees, then the image you capture (if nothing else happens to it)
> >> > > > > > will look like it has had a 90 degree clockwise rotation performed.
> >> > > > > > Everyone agree with that?
> >> > > > > Yep, agreed. That would mean V4L2_CID_CAMERA_SENSOR_ROTATION==90 would
> >> > > > > translate to Transform::Rot90.
> >> > > >
> >> > > > Let's here put some definitions in place
> >> > > >
> >> > > > V4L2_CID_CAMERA_SENSOR_ROTATION
> >> > > > This read-only control describes the rotation correction in degrees in
> >> > > > the counter-clockwise direction to be applied to the captured images
> >> > > > once captured to memory to compensate for the camera sensor mounting
> >> > > > rotation.
> >> > > >
> >> > > >  \var Transform::Rot270
> >> > > >  Rotation by 270 degrees clockwise (90 degrees anticlockwise).
> >> > > >
> >> > > >  Let's start by saying we did a sub-optimal (to be nice) job in
> >> > > >  having Transform and V4L2_CID_ROTATION operate in two different
> >> > > >  directions. Before the usage of Transform in libcamera gets widely
> >> > > >  adopted sould we align the two ?
> >> > >
> >> > > I don't mind at all... we just have to decide what we want!
> >> > >
> >> > > >
> >> > > >  Hence if I'm not mistaken, if your camera has Rotation=90 and an
> >> > > >  application supplies Transform::Identity in
> >> > > >  CameraConfiguration::transform what you should get back is Rot270
> >> > > >
> >> > > >  Correct ?
> >> > >
> >> > > The way things are currently, if the camera says Rotation=90, then
> >> > > your image in memory looks like it is rotated 90 degrees clockwise, so
> >> > > the Transform (if your pipeline and sensor can't apply any
> >> > > flips/rotations at all) will come back as Rot90.
> >> > >
> >> > > That is, the only transform you can specify that won't be adjusted, is
> >> > > a 90 degree clockwise rotation, so transform=Rot90 as things stand
> >> > > currently.
> >> > >
> >> > > If we choose to change the sense of the libcamera.Transform rotation,
> >> > > then we'd get transform=Rot270 instead.
> >> > >
> >> > >  (If your pipeline handler can do flips and transposes, it may be able
> >> > > to do what you ask and so the transform would come back unchanged and
> >> > > would be "Valid".)
> >> > >
> >> > > >
> >> > > > > > So in turn, if you have a camera/ISP that can apply no transforms at
> >> > > > > > all, then the only value for the user transform that will not come
> >> > > > > > back as adjusted is "rot90", because it looks like it's had a 90
> >> > > > > > degree clockwise rotation performed (and libcamera transforms count
> >> > > > > > rotations as being clockwise). Does that also make sense?
> >> > > >
> >> > > > It might be the case today, but I wonder if that's desirable.
> >> > > >
> >> > > > If an application supplies CameraConfiguration::transform = Identity
> >> > > > it means it doesn't want any rotation applied by the library. It will
> >> > > > get back 270 to indicate that that's how the image is rotated and will
> >> > > > get an Adjusted configuration.
> >> > >
> >> > > I'm not quite sure I get that, perhaps it depends how we interpret
> >> > > "applied by the library". I guess I agree if we mean "applied by the
> >> > > library together with any rotation of the sensor"? Maybe a real
> >> >
> >> > "Applied by the library" == happens inside libcamera, ISP or sensor
> >> > flips..
> >> >
> >> > > example is helpful:
> >> > >
> >> > > So for a Raspberry Pi, for example, our sensors are mostly mounted
> >> > > upside down (V4L2_CID_CAMERA_SENSOR_ROTATION says "180"). Our users
> >> > > say the transform they want is transform=Identity i.e. images come
> >> > > back "the right way up". Internally it means that we do apply an extra
> >> > > h+v flip to undo the 180 degree rotation of the sensor.
> >> > >
> >> >
> >> > Here you go, this is where we disconnect.
> >> >
> >> > As I've interpreted the API so far, and the way we designed
> >> > properties::Rotation was to signal to the application what is the
> >> > status of the image as it comes from the sensor. Application could then
> >> > correct that by setting CameraConfiguration::transform, but in most
> >> > cases the image will be corrected at rendering time by the application
> >> > itself.
> >> >
> >> > IWO I was not expecting auto-correction and Transform::Identity to me
> >> > means "give me the images as they come from the sensor"
> >> >
> >> > > >
> >> > > > If instead an application supplies CameraConfiguration::transform = Rot270
> >> > > > and your camera -cannot- do any rotation, it will still receive back
> >> > > > Rot270 and the state is Valid.
> >> > >
> >> > > I think that's true if we change the sense of the libcamera.Transform
> >> > > rotation. Currently, as I said earlier, I think we get Rot90?
> >> > >
> >> > > >
> >> > > > If it asks for CameraConfiguration::transform = Rot270 and the camera
> >> > > > -can- do that, it will clear CameraConfiguration::transform to
> >> > > > Identity and return Adjusted ?
> >> > >
> >> > > I'm not sure there... The CameraConfiguration::transform is the final
> >> > > transform you want. If that can be done, it's left alone and you get
> >> > > back Valid. It's not telling you what else you would need to do
> >> > > afterwards.
> >> > >
> >> > > (To find out what you need to do afterwards you'd work out
> >> > > "inverse(what-you-will-get) * what-you-wanted", or something like
> >> > > that.)
> >> > >
> >> >
> >> > What I would like is to make as easier as possible for applications
> >> > to know what they have to do to have the image rotated in the way they
> >> > expect. There are several "transform" steps on the image, the first
> >> > one happens in libcamera, all the other ones are left to the
> >> > appplication.
> >>
> >> I think this is exactly what I want, namely "to make it as easy as
> >> possible for applications".
> >>
> >> On Raspberry Pi - and this has been the case for a couple of years
> >> now! - if a user wants images the "normal" way up then they simply
> >> pass Transform::Identity. If they want upside down images they pass
> >> Transform::Rot180. It's as easy as that, there is simply nothing else
> >> to do.
> >>
> >> Requiring all applications to look at the sensor rotation and then
> >> combine various things to figure out what they have to ask for (and
> >> possibly get it wrong) seems, well, actually quite unhelpful.
> >
> >
> > FWIW, I do agree that applications should not have to trouble themselves
> > with munging sensor rotation andrequested rotation to achieve their
> > desired outcome.  This definitely wants to be hidden in the libcamera core
> > or pipeline handler layers.
> >
> > Naush
> >
> >
> >>
> >>
> >> But this is indeed what we need to talk about. Let's organise a call!
> >>
> >> Thanks
> >> David
> >>
> >> >
> >> > *) The image "native" transform is reported by Rotation (this assumes
> >> > that any implicit rotation buried in the sensor driver, as per our
> >> > last discussion with Dave is not taken into account)
> >> >
> >> >    properties::Rotation = Rot270 = HFlip | Transpose
> >> >
> >> > *) Application sets CameraConfiguration::transform and call validate() to
> >> > check what can be done by libcamera and what should be done by next
> >> > layers
> >> >
> >> >    CameraConfigurat::Transform = Rot270
> >> >
> >> > *) The camera validate the transform and as it can only flip it sets
> >> > transform to HFlip, and the state is Adjusted
> >> >
> >> >    validate() -> The camera can only flip
> >> >               -> CameraConfiguration = HFlip
> >> >
> >> > *) Application gets back what the camera can do and instrument the
> >> > rest of the rendering pipeline to perform the additional transforms
> >> > the camera cannot do. To get what you have to do I presume you have to
> >> >
> >> >         additionalTransform = what_you_want & !what_you_get
> >> >
> >> > So yes, I was interpreting the usage of CameraConfiguration::transform
> >> > as a negotiation of what the library can do and what applications have
> >> > to do by themselves, which if I got you is different than what's
> >> > currecntly implemented ?
> >> >
> >> > > >
> >> > > > Is this what happens (I'm looking at the above code with David's
> >> > > > suggestion to use the inverse of roationTransform).
> >> > > >
> >> > > >
> >> > > > > >
> >> > > > > > So by a slightly arbitrary mixture of conventions, it looks to me as
> >> > > > > > though the use of the inverse that I queried is indeed wrong, because
> >> > > > > > we "need camera 90 degree rotation" => "user transform rot90" in this
> >> > > > > > case.
> >> > > > > >
> >> > > > > > Does that sound plausible... what do folks think?
> >> > > > >
> >> > > > > Ah, I think the question is whether the adjusted value means: "this is what
> >> > > > > you'll get" vs "this is what you have to do yourself".
> >> > > > >
> >> > > > > Above I argued in favor of the later, but I guess the first one is what the
> >> > > > > API is supposed to mean. That would imply that the client has compute the
> >> > > > > difference between requested and adjusted value itself.
> >> > > >
> >> > > > I always presumed the latter was meant to happen..
> >> > >
> >> > > Yes, I think this is the crux of the matter. I believe it's "what you
> >> > > will get". It's not "what you need to do after". (If it was "what you
> >> > > need to do after", it would change every time you try to validate the
> >> > > configuration!)
> >> > >
> >> >
> >> > On the "it would change every time you try to validate the
> >> > configuration": only if the camera cannot satisfy the transform you
> >> > have requested. If you ask for a flip and the camera can do it,
> >> > transform stays un-touched and the state is Valid ?
> >> >
> >> > > >
> >> > > > >
> >> > > > > In my case that would mean:
> >> > > > >
> >> > > > > requested CameraConfiguration::transform: Transform::Identity,
> >> > > > > V4L2_CID_CAMERA_SENSOR_ROTATION: 270, adjusted
> >> > > > > CameraConfiguration::transform: Transform::Rot270
> >> > > > >
> >> > > >
> >> > > > I guess it's Rot90 because of the clockwise/counter-clockwise issue ?
> >> > >
> >> > > Yes, looks right to me!
> >> > >
> >> > > >
> >> > > > > To compute what the client has to do, it would need to do:
> >> > > > > Transform::Identity - Transform::Rot270 = Transform::Rot90. And then apply
> >> > > > > that, a 90 degr. rotation clockwise.
> >> > >
> >> > > Yes, I guess I wrote it above, it's "inverse(what-you-got) *
> >> > > what-you-wanted", in this case inverse(rot270) * identity which is
> >> > > rot90 (in the current clockwise sense).
> >> > >
> >> > > > >
> >> > > > > Does that sound reasonable? In that case the patch here should indeed be
> >> > > > > `*transform = rotationTransform;`, without the inverse.
> >> > > > >
> >> > >
> >> > > Yes, agree!
> >> > >
> >> > > >
> >> > > >         /0\
> >> > > >
> >> > > > before we plumb this into and make assumptions that will be then
> >> > > > set into stone in our adaption layers, can we take a step back and
> >> > > > define what kind of API we would like to have ?
> >> > > >
> >> > > > Let's start by the definition of CameraConfiguration::transform.
> >> > > >
> >> > > > It's a member of CameraConfiguration, and as by definition it is
> >> > > > filled-in by the application and eventually adjusted by the Camera to
> >> > > > a value that's acceptable/doable for the current configuration.
> >> > > >
> >> > > > 1) Application to Camera  = (counter?)clockwise degrees the camera
> >> > > > system should rotate the image before presenting it to the user
> >> > >
> >> > > This may depend on what exactly we mean by "the camera system". I'd
> >> > > explain it as
> >> > > "the final resulting transform that the application wants to be
> >> > > applied to all the output images". After all, this is actually the
> >> > > only thing the application knows!
> >> > >
> >> >
> >> > agreed
> >> >
> >> > > >
> >> > > > 2) Camera to application = the rotation the camera can actually do
> >> > > > (ie. only sensor flips == no Transpose)
> >> > >
> >> > > Agree, mostly, though I would say "transform" rather than "rotation"
> >> >
> >> > yes, more correct
> >> >
> >> > > because flips aren't rotations, you could in theory have a PH that
> >> > > could do transposes (even though we probably don't currently).
> >> > >
> >> >
> >> > none currently afaict
> >> >
> >> > > >
> >> > > > The current documentation doesn't explain how it gets adjusted
> >> > >
> >> > > This is true, though I also think it's platform dependent. If you had
> >> > > a PH that can't do transposes but can do flips, you have a choice how
> >> > > you handle (for example) rot90. You could say "oh I can't do that, so
> >> > > in this case I won't do anything at all" and leave the application to
> >> > > sort everything out. The Pi tries to say "well, I'll do everything
> >> > > except for the transpose, so an application only has to worry about
> >> > > that one case". But there is a choice...
> >> > >
> >> >
> >> > I agree it's platform-specific, but the way transform is negotiated with
> >> > application should be standardized
> >> >
> >> > > >
> >> > > > /**
> >> > > >  * \var CameraConfiguration::transform
> >> > > >  * \brief User-specified transform to be applied to the image
> >> > > >  *
> >> > > >  * The transform is a user-specified 2D plane transform that will be applied
> >> > > >  * to the camera images by the processing pipeline before being handed to
> >> > > >  * the application. This is subsequent to any transform that is already
> >> > > >  * required to fix up any platform-defined rotation.
> >> > > >  *
> >> > > >  * The usual 2D plane transforms are allowed here (horizontal/vertical
> >> > > >  * flips, multiple of 90-degree rotations etc.), but the validate() function
> >> > > >  * may adjust this field at its discretion if the selection is not supported.
> >> > > >  */
> >> > > >
> >> > > > The last part in particular, "at its discrection" it's really too
> >> > > > generic.
> >> > >
> >> > > I guess it's hinting at the fact that it's up to the PH, and we don't
> >> > > mandate the exact way in which it can fail to do what you asked for!
> >> > > But it would be fine to have a discussion on that.
> >> > >
> >> >
> >> > That's the point that most concerns me :)
> >> >
> >> > > >
> >> > > > Let's start from the beginning: what application should use transform
> >> > > > for ? I presume they should:
> >> > > >
> >> > > > 1) Inspect propertis::Rotation
> >> > > > 2) Correct the image for the properties::Rotation amount (actually,
> >> > > > the inverse because of the clockwise/counter-clockwise thing)
> >> > > > 3) Add any additional rotation they would like on top of this
> >> > >
> >> > > So I think the point of the transform is that you *don't* have to
> >> > > worry about the properties::Rotation. It takes care of that for you.
> >> > > You say "I want upside down images" and you get them, whatever the
> >> > > sensor's rotation.
> >> >
> >> > That's acceptable as well if we prefer that, but it has to be
> >> > standardized among pipelines imho
> >> >
> >> > >
> >> > > >
> >> > > > How should transform be adjusted ?
> >> > > >
> >> > > > Identity:
> >> > > > If application->identity the camera does not have to apply any
> >> > > > transform. Should the transform be adjutes like it happens today to
> >> > > > compensate the Rotation (at least that's what I presume happens) ? I
> >> > > > presume no, applications can infer rotation from properties and if they
> >> > > > chose Identity either other layers above will do the transform or
> >> > > > they're fine with images as they are.
> >> > >
> >> > > If the application says it wants the "identity" transform, then it
> >> > > should get output images with no transformation applied. But this does
> >> > > mean it will undo the sensor rotation (this being the normal case on
> >> > > the Pi).
> >> > >
> >> >
> >> > right, it depends if we decide to account for the sensor rotation
> >> > inside libcamera or not
> >> >
> >> > > >
> >> > > > A supported/non-supported transform:
> >> > > > An application asks for a VFLIP, the camera can do it, transform is
> >> > > > not changed. If the camera cannot flip, the transform is set back to
> >> > > > Identity and the state is adjusted.
> >> > >
> >> > > Yes, mostly, except that the sensor may be mounted with a rotation,
> >> > > but if it isn't, then I agree with this.
> >> > >
> >> > > >
> >> > > > A partially non-supported transform:
> >> > > > An application asks for Rot270 (Transpose | HFLIP) and the camera can
> >> > > > only do HFLIP. transform sould be then set to HFLIP and applications
> >> > > > know that the remaining transformation should be handled elsewhere.
> >> > >
> >> > > As I suggested above, I'm not sure whether we want to mandate this. I
> >> > > agree it seems useful behaviour, which is why the Pi does it, but I'm
> >> > > open minded about requiring it or not.
> >> > >
> >> > > >
> >> > > > Are we missing anything with such behaviour ? I presume it's rather
> >> > > > similar to what happens today if not for the fact that the validate()
> >> > > > implementation adjusts transform to rotation in case it cannot flip
> >> > > >
> >> > > >         Transform combined = transform * data_->rotationTransform_;
> >> > > >         if (!data_->supportsFlips_ && !!combined)
> >> > > >                 transform = -data_->rotationTransform_;
> >> > > >
> >> > > > Not really sure I got why.
> >> > > >
> >> > > > All of this, but an even more fundamental question: is it too late/is
> >> > > > it worh it to adjust the Transform definition to adopt the same
> >> > > > direction as the kernel defined rotation ?
> >> > > >
> >> > > > Sorry for the long email :/
> >> > >
> >> > > No problem! I like nothing more than discussing transforms, except
> >> > > possibly colour spaces!
> >> > >
> >> >
> >> > :)
> >> >
> >> > > More seriously, here are my key points:
> >> >
> >> > I'll try to summarize what I see differently
> >> >
> >> > >
> >> > > 1. The CameraConfiguration::transform field says what the application
> >> > > wants the final output images to have.
> >> >
> >> > 1. The CameraConfiguration::transform field says what transform the
> >> > library has to apply to images as they arrive from the sensor in its
> >> > default configuration
> >> >
> >> > > 2. It takes account of the sensor rotation. The most obvious example
> >> > > is the Pi: if the sensor is mounted upside down, and the applications
> >> > > ask for images "the right way up" (i.e. transform = identity), then
> >> > > the PH will apply h and v flips to compensate for the sensor mounting.
> >> >
> >> > 2. It doesn't automatically account for sensor rotation. Application
> >> > might decide to do that along the rendering pipeline, in example
> >> >
> >> > > 3. If the PH can do what the application requests, it leaves the
> >> > > transform alone and your configuration is Valid.
> >> >
> >> > Agreed
> >> >
> >> > > 4. If the PH can't do what the application wants, it will set the
> >> > > value to something that it can do, and the configuration will be
> >> > > Adjusted.
> >> >
> >> > Agreed
> >> >
> >> > >
> >> > > If folks wanted a short conference call to iron any of this out, I'd
> >> > > be very happy to join in!
> >> >
> >> > I've asked others to have a look here, let's see if we need a call,
> >> > I'm all for it !
> >> >
> >> > Thanks
> >> >   j
> >> >
> >> > >
> >> > > Thanks
> >> > > David
> >> > >
> >> > > >
> >> > > > > > Thanks everyone!
> >> > > > > > David
> >> > > > >
> >> > > > > Thanks!
> >> > > > >
> >> > > > > Robert
> >> > > > >

Hi Jacopo

Thanks for your reply!

On Tue, 17 Jan 2023 at 12:09, Jacopo Mondi
<jacopo.mondi@ideasonboard.com> wrote:
>
> Hi David,
>         cc Sakari for context about the imx258 sensor patches
>
> On Thu, Jan 12, 2023 at 11:18:51AM +0000, David Plowman wrote:
> > Hi everyone
> >
> > I wanted to give this discussion a little nudge because I don't think
> > it was ever quite concluded, perhaps Christmas got in the way!
> >
> > To summarise a little bit, at least from my point of view, my
> > impression was that the current approach is correct, and the transform
> > field is for the user to request the transform that they want, and may
> > get adjusted to be the transform that they will get. Details of any
> > rotation in the way the sensor is mounted is hidden from the user -
> > they can just say "I want the image the right way up" and it
> > "magically" happens. (This is exactly how the Raspberry Pi pipeline
> > handler works today.)
>
> I have been able to discuss a bit with others, and I'm here trying to
> summarize the outcome
>
> * Recap of the assumptions:
>
> From the kernel interface we have:
> - Physical mounting rotation (V4L2_CID_CAMERA_SENSOR_ROTATION)
> - Sensor flips capabilities (which depends on the V4L2_CID_[H|F]LIP
>   presence and if they are RW)
>
> It is fair to assume that:
> - Sensors cannot do transpose (flip along the diagonal axis)
> - They can only HVFLIP (ie flip, mirror or 180 degrees rotate)
>
> The ISP can also do transforms like transpose or even rotate, but
> compared to acting flips on the sensor this transformation does not
> change the row/col reading order in the image (the first pixels
> visible in the will end up being the ones read last) which might have
> an impact for rolling shutter sensors.
>
> * Proposed implementation
>
> libcamera retrieves the mounting rotation, and if it can fully
> compensate it by setting sensor's flips (or if flips are already hardcoded to
> 1 as it happens in some drivers) it will adjust properties::Rotation
> to report that the image will be rotated upright and application
> shouldn't care. This is what happens today with your implementation of
> the Transform handling part.
>
> This also implies that sensor's can correct only 180 degrees rotation
> and everything else will be deferred to
> 1) other processing layers
> 2) additional transforms in the ISP if the pipeline supports those
>
> If you're ok with such plan I propose:
>
> - let's land my series of 5 patches that centralize flip handling in
>   CameraSensor but does not introduce functional changes
> - let's on top make the CameraSensor class register property::Rotation
>   compensated by the sensor (basically only for 180deg correction)
> - let's adjust CameraSensor::setFormat() to apply flips correctly and
>   remove transpose handling from there but only handle 180 degrees
>   transformations.
>
> For RPi it means 180deg rotation will be automatically compensated as
> they're today by sensor's flips. The only difference is that your
> consumers will see a properties::Rotation = 0 instead of 180, as for
> them the image is already 'correct'.
>
> For other devices with different rotations which require transpose
> (as 90/270 deg, typical for smartphones) the whole rotation adjustment
> will be performed by the application's upper layers.
>
> Does this sound viable to you ?

Mostly that's sounding good to me. The only thing I have a couple of
questions about is what we do if we can't compensate for the camera
rotation.

* It feels a bit strange to clear the advertised camera mounting
rotation if we can handle it, but not otherwise. I think we should
always advertise what it really is (even if that's in a separate
property). Perhaps there's another way to indicate what transforms a
PH can handle (a Transform is just a bit field, after all...)?

* I think the code currently tries to compensate for everything except
transpose, so that would be the only case you have to implement
outside libcamera. But obviously there's a choice there. Might an
application prefer you to do "nothing" instead of "everything bar the
transpose"? Obviously you could work out what you need to ask for to
get the "left over" transform that you want...

Finally, on the subject of rotation orientations, I think it would be
nice if you could request transformFromRotation(sensor_rotation) in
your configuration, and that would guarantee that no transforms would
be applied by libcamera anywhere. That feels "natural" to me.

But otherwise, yes, let's push this forward.

Thanks!
David

>
> >
> > There was a small detail about whether one of the transforms in the
> > code wanted reversing, that's mostly a matter of conventions about
> > using clockwise or anticlockwise rotations (or the direction of the
> > rotation axis).
> >
> > Does anyone have anything else to add here?
> >
> > Thanks!
> > David
> >
> > On Wed, 14 Dec 2022 at 15:55, Robert Mader via libcamera-devel
> > <libcamera-devel@lists.libcamera.org> wrote:
> > >
> > > Hi,
> > >
> > > just wanted to drop that
> > >
> > > I'm personally not very opinionated about the API design
> > > the new Pipewire implementation matches the current Raspberry Pi code
> > > I think/agree this patch series is the ideal place to change/stabilize this API
> > > I'll happily adopt the Pipewire implementation as soon as a consensus is found / the implementation lands :)
> > >
> > > Best regards and thanks to everyone joining the discussion!
> > >
> > > Robert
> > >
> > > On 13.12.22 15:59, Naushir Patuck via libcamera-devel wrote:
> > >
> > > Hi all,
> > >
> > >
> > > On Tue, 13 Dec 2022 at 14:30, David Plowman via libcamera-devel <libcamera-devel@lists.libcamera.org> wrote:
> > >>
> > >> Hi Jacopo
> > >>
> > >> Thanks for the reply. I won't write a great deal this time round, I
> > >> agree that possibly a call would be a good idea.
> > >>
> > >> On Tue, 13 Dec 2022 at 11:22, Jacopo Mondi <jacopo@jmondi.org> wrote:
> > >> >
> > >> > Hi David
> > >> >
> > >> > On Mon, Dec 12, 2022 at 11:46:31AM +0000, David Plowman wrote:
> > >> > > Hi Jacopo
> > >> > >
> > >> > > Thanks for wading into the sea on this question! (Very appropriate now
> > >> > > that the stick person example has been turned into a shark...!)
> > >> > >
> > >> > > On Fri, 9 Dec 2022 at 12:47, Jacopo Mondi via libcamera-devel
> > >> > > <libcamera-devel@lists.libcamera.org> wrote:
> > >> > > >
> > >> > > > Hi again,
> > >> > > >
> > >> > > > On Wed, Dec 07, 2022 at 03:48:40PM +0000, Robert Mader via libcamera-devel wrote:
> > >> > > > > On 07.12.22 15:28, David Plowman via libcamera-devel wrote:
> > >> > > > > > Hi Robert
> > >> > > > > >
> > >> > > > > > Thanks for prodding us all on this question!
> > >> > > > >
> > >> > > > > Hi, thanks for the quick reply! And a pleasure :P
> > >> > > > >
> > >> > > > > > I looked back over this again, and found Jacopo's description of the
> > >> > > > > > V4L2_CID_CAMERA_SENSOR_ROTATION control
> > >> > > > > > (https://patchwork.kernel.org/project/linux-media/patch/20191204091056.4842-5-jacopo@jmondi.org/
> > >> > > > > > is what I stumbled into)
> > >> > > > >
> > >> > > > > The current kernel docu can be found at
> > >> > > > >
> > >> > > > > https://github.com/torvalds/linux/blob/8ed710da2873c2aeb3bb805864a699affaf1d03b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> > >> > >
> > >> > > Thanks for the updated reference. The stick person may have turned
> > >> > > into a shark, but apart from that I think the sense of the
> > >> > > documentation is unchanged. So if V4L2_CID_CAMERA_SENSOR_ROTATION says
> > >> > > 90 degrees, then your image in memory looks like it has had a 90
> > >> > > degree *clockwise* rotation applied.
> > >> > >
> > >> >
> > >> > Yes!
> > >> >
> > >> > > > >
> > >> > > > > the libcamera one at
> > >> > > > >
> > >> > > > > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/transform.cpp#n54
> > >> > > > >
> > >> > > > > >  From that, and the nice example with the stick person, I deduce that
> > >> > > > > > if your camera's V4L2_CID_CAMERA_SENSOR_ROTATION property says 90
> > >> > > > > > degrees, then the image you capture (if nothing else happens to it)
> > >> > > > > > will look like it has had a 90 degree clockwise rotation performed.
> > >> > > > > > Everyone agree with that?
> > >> > > > > Yep, agreed. That would mean V4L2_CID_CAMERA_SENSOR_ROTATION==90 would
> > >> > > > > translate to Transform::Rot90.
> > >> > > >
> > >> > > > Let's here put some definitions in place
> > >> > > >
> > >> > > > V4L2_CID_CAMERA_SENSOR_ROTATION
> > >> > > > This read-only control describes the rotation correction in degrees in
> > >> > > > the counter-clockwise direction to be applied to the captured images
> > >> > > > once captured to memory to compensate for the camera sensor mounting
> > >> > > > rotation.
> > >> > > >
> > >> > > >  \var Transform::Rot270
> > >> > > >  Rotation by 270 degrees clockwise (90 degrees anticlockwise).
> > >> > > >
> > >> > > >  Let's start by saying we did a sub-optimal (to be nice) job in
> > >> > > >  having Transform and V4L2_CID_ROTATION operate in two different
> > >> > > >  directions. Before the usage of Transform in libcamera gets widely
> > >> > > >  adopted sould we align the two ?
> > >> > >
> > >> > > I don't mind at all... we just have to decide what we want!
> > >> > >
> > >> > > >
> > >> > > >  Hence if I'm not mistaken, if your camera has Rotation=90 and an
> > >> > > >  application supplies Transform::Identity in
> > >> > > >  CameraConfiguration::transform what you should get back is Rot270
> > >> > > >
> > >> > > >  Correct ?
> > >> > >
> > >> > > The way things are currently, if the camera says Rotation=90, then
> > >> > > your image in memory looks like it is rotated 90 degrees clockwise, so
> > >> > > the Transform (if your pipeline and sensor can't apply any
> > >> > > flips/rotations at all) will come back as Rot90.
> > >> > >
> > >> > > That is, the only transform you can specify that won't be adjusted, is
> > >> > > a 90 degree clockwise rotation, so transform=Rot90 as things stand
> > >> > > currently.
> > >> > >
> > >> > > If we choose to change the sense of the libcamera.Transform rotation,
> > >> > > then we'd get transform=Rot270 instead.
> > >> > >
> > >> > >  (If your pipeline handler can do flips and transposes, it may be able
> > >> > > to do what you ask and so the transform would come back unchanged and
> > >> > > would be "Valid".)
> > >> > >
> > >> > > >
> > >> > > > > > So in turn, if you have a camera/ISP that can apply no transforms at
> > >> > > > > > all, then the only value for the user transform that will not come
> > >> > > > > > back as adjusted is "rot90", because it looks like it's had a 90
> > >> > > > > > degree clockwise rotation performed (and libcamera transforms count
> > >> > > > > > rotations as being clockwise). Does that also make sense?
> > >> > > >
> > >> > > > It might be the case today, but I wonder if that's desirable.
> > >> > > >
> > >> > > > If an application supplies CameraConfiguration::transform = Identity
> > >> > > > it means it doesn't want any rotation applied by the library. It will
> > >> > > > get back 270 to indicate that that's how the image is rotated and will
> > >> > > > get an Adjusted configuration.
> > >> > >
> > >> > > I'm not quite sure I get that, perhaps it depends how we interpret
> > >> > > "applied by the library". I guess I agree if we mean "applied by the
> > >> > > library together with any rotation of the sensor"? Maybe a real
> > >> >
> > >> > "Applied by the library" == happens inside libcamera, ISP or sensor
> > >> > flips..
> > >> >
> > >> > > example is helpful:
> > >> > >
> > >> > > So for a Raspberry Pi, for example, our sensors are mostly mounted
> > >> > > upside down (V4L2_CID_CAMERA_SENSOR_ROTATION says "180"). Our users
> > >> > > say the transform they want is transform=Identity i.e. images come
> > >> > > back "the right way up". Internally it means that we do apply an extra
> > >> > > h+v flip to undo the 180 degree rotation of the sensor.
> > >> > >
> > >> >
> > >> > Here you go, this is where we disconnect.
> > >> >
> > >> > As I've interpreted the API so far, and the way we designed
> > >> > properties::Rotation was to signal to the application what is the
> > >> > status of the image as it comes from the sensor. Application could then
> > >> > correct that by setting CameraConfiguration::transform, but in most
> > >> > cases the image will be corrected at rendering time by the application
> > >> > itself.
> > >> >
> > >> > IWO I was not expecting auto-correction and Transform::Identity to me
> > >> > means "give me the images as they come from the sensor"
> > >> >
> > >> > > >
> > >> > > > If instead an application supplies CameraConfiguration::transform = Rot270
> > >> > > > and your camera -cannot- do any rotation, it will still receive back
> > >> > > > Rot270 and the state is Valid.
> > >> > >
> > >> > > I think that's true if we change the sense of the libcamera.Transform
> > >> > > rotation. Currently, as I said earlier, I think we get Rot90?
> > >> > >
> > >> > > >
> > >> > > > If it asks for CameraConfiguration::transform = Rot270 and the camera
> > >> > > > -can- do that, it will clear CameraConfiguration::transform to
> > >> > > > Identity and return Adjusted ?
> > >> > >
> > >> > > I'm not sure there... The CameraConfiguration::transform is the final
> > >> > > transform you want. If that can be done, it's left alone and you get
> > >> > > back Valid. It's not telling you what else you would need to do
> > >> > > afterwards.
> > >> > >
> > >> > > (To find out what you need to do afterwards you'd work out
> > >> > > "inverse(what-you-will-get) * what-you-wanted", or something like
> > >> > > that.)
> > >> > >
> > >> >
> > >> > What I would like is to make as easier as possible for applications
> > >> > to know what they have to do to have the image rotated in the way they
> > >> > expect. There are several "transform" steps on the image, the first
> > >> > one happens in libcamera, all the other ones are left to the
> > >> > appplication.
> > >>
> > >> I think this is exactly what I want, namely "to make it as easy as
> > >> possible for applications".
> > >>
> > >> On Raspberry Pi - and this has been the case for a couple of years
> > >> now! - if a user wants images the "normal" way up then they simply
> > >> pass Transform::Identity. If they want upside down images they pass
> > >> Transform::Rot180. It's as easy as that, there is simply nothing else
> > >> to do.
> > >>
> > >> Requiring all applications to look at the sensor rotation and then
> > >> combine various things to figure out what they have to ask for (and
> > >> possibly get it wrong) seems, well, actually quite unhelpful.
> > >
> > >
> > > FWIW, I do agree that applications should not have to trouble themselves
> > > with munging sensor rotation andrequested rotation to achieve their
> > > desired outcome.  This definitely wants to be hidden in the libcamera core
> > > or pipeline handler layers.
> > >
> > > Naush
> > >
> > >
> > >>
> > >>
> > >> But this is indeed what we need to talk about. Let's organise a call!
> > >>
> > >> Thanks
> > >> David
> > >>
> > >> >
> > >> > *) The image "native" transform is reported by Rotation (this assumes
> > >> > that any implicit rotation buried in the sensor driver, as per our
> > >> > last discussion with Dave is not taken into account)
> > >> >
> > >> >    properties::Rotation = Rot270 = HFlip | Transpose
> > >> >
> > >> > *) Application sets CameraConfiguration::transform and call validate() to
> > >> > check what can be done by libcamera and what should be done by next
> > >> > layers
> > >> >
> > >> >    CameraConfigurat::Transform = Rot270
> > >> >
> > >> > *) The camera validate the transform and as it can only flip it sets
> > >> > transform to HFlip, and the state is Adjusted
> > >> >
> > >> >    validate() -> The camera can only flip
> > >> >               -> CameraConfiguration = HFlip
> > >> >
> > >> > *) Application gets back what the camera can do and instrument the
> > >> > rest of the rendering pipeline to perform the additional transforms
> > >> > the camera cannot do. To get what you have to do I presume you have to
> > >> >
> > >> >         additionalTransform = what_you_want & !what_you_get
> > >> >
> > >> > So yes, I was interpreting the usage of CameraConfiguration::transform
> > >> > as a negotiation of what the library can do and what applications have
> > >> > to do by themselves, which if I got you is different than what's
> > >> > currecntly implemented ?
> > >> >
> > >> > > >
> > >> > > > Is this what happens (I'm looking at the above code with David's
> > >> > > > suggestion to use the inverse of roationTransform).
> > >> > > >
> > >> > > >
> > >> > > > > >
> > >> > > > > > So by a slightly arbitrary mixture of conventions, it looks to me as
> > >> > > > > > though the use of the inverse that I queried is indeed wrong, because
> > >> > > > > > we "need camera 90 degree rotation" => "user transform rot90" in this
> > >> > > > > > case.
> > >> > > > > >
> > >> > > > > > Does that sound plausible... what do folks think?
> > >> > > > >
> > >> > > > > Ah, I think the question is whether the adjusted value means: "this is what
> > >> > > > > you'll get" vs "this is what you have to do yourself".
> > >> > > > >
> > >> > > > > Above I argued in favor of the later, but I guess the first one is what the
> > >> > > > > API is supposed to mean. That would imply that the client has compute the
> > >> > > > > difference between requested and adjusted value itself.
> > >> > > >
> > >> > > > I always presumed the latter was meant to happen..
> > >> > >
> > >> > > Yes, I think this is the crux of the matter. I believe it's "what you
> > >> > > will get". It's not "what you need to do after". (If it was "what you
> > >> > > need to do after", it would change every time you try to validate the
> > >> > > configuration!)
> > >> > >
> > >> >
> > >> > On the "it would change every time you try to validate the
> > >> > configuration": only if the camera cannot satisfy the transform you
> > >> > have requested. If you ask for a flip and the camera can do it,
> > >> > transform stays un-touched and the state is Valid ?
> > >> >
> > >> > > >
> > >> > > > >
> > >> > > > > In my case that would mean:
> > >> > > > >
> > >> > > > > requested CameraConfiguration::transform: Transform::Identity,
> > >> > > > > V4L2_CID_CAMERA_SENSOR_ROTATION: 270, adjusted
> > >> > > > > CameraConfiguration::transform: Transform::Rot270
> > >> > > > >
> > >> > > >
> > >> > > > I guess it's Rot90 because of the clockwise/counter-clockwise issue ?
> > >> > >
> > >> > > Yes, looks right to me!
> > >> > >
> > >> > > >
> > >> > > > > To compute what the client has to do, it would need to do:
> > >> > > > > Transform::Identity - Transform::Rot270 = Transform::Rot90. And then apply
> > >> > > > > that, a 90 degr. rotation clockwise.
> > >> > >
> > >> > > Yes, I guess I wrote it above, it's "inverse(what-you-got) *
> > >> > > what-you-wanted", in this case inverse(rot270) * identity which is
> > >> > > rot90 (in the current clockwise sense).
> > >> > >
> > >> > > > >
> > >> > > > > Does that sound reasonable? In that case the patch here should indeed be
> > >> > > > > `*transform = rotationTransform;`, without the inverse.
> > >> > > > >
> > >> > >
> > >> > > Yes, agree!
> > >> > >
> > >> > > >
> > >> > > >         /0\
> > >> > > >
> > >> > > > before we plumb this into and make assumptions that will be then
> > >> > > > set into stone in our adaption layers, can we take a step back and
> > >> > > > define what kind of API we would like to have ?
> > >> > > >
> > >> > > > Let's start by the definition of CameraConfiguration::transform.
> > >> > > >
> > >> > > > It's a member of CameraConfiguration, and as by definition it is
> > >> > > > filled-in by the application and eventually adjusted by the Camera to
> > >> > > > a value that's acceptable/doable for the current configuration.
> > >> > > >
> > >> > > > 1) Application to Camera  = (counter?)clockwise degrees the camera
> > >> > > > system should rotate the image before presenting it to the user
> > >> > >
> > >> > > This may depend on what exactly we mean by "the camera system". I'd
> > >> > > explain it as
> > >> > > "the final resulting transform that the application wants to be
> > >> > > applied to all the output images". After all, this is actually the
> > >> > > only thing the application knows!
> > >> > >
> > >> >
> > >> > agreed
> > >> >
> > >> > > >
> > >> > > > 2) Camera to application = the rotation the camera can actually do
> > >> > > > (ie. only sensor flips == no Transpose)
> > >> > >
> > >> > > Agree, mostly, though I would say "transform" rather than "rotation"
> > >> >
> > >> > yes, more correct
> > >> >
> > >> > > because flips aren't rotations, you could in theory have a PH that
> > >> > > could do transposes (even though we probably don't currently).
> > >> > >
> > >> >
> > >> > none currently afaict
> > >> >
> > >> > > >
> > >> > > > The current documentation doesn't explain how it gets adjusted
> > >> > >
> > >> > > This is true, though I also think it's platform dependent. If you had
> > >> > > a PH that can't do transposes but can do flips, you have a choice how
> > >> > > you handle (for example) rot90. You could say "oh I can't do that, so
> > >> > > in this case I won't do anything at all" and leave the application to
> > >> > > sort everything out. The Pi tries to say "well, I'll do everything
> > >> > > except for the transpose, so an application only has to worry about
> > >> > > that one case". But there is a choice...
> > >> > >
> > >> >
> > >> > I agree it's platform-specific, but the way transform is negotiated with
> > >> > application should be standardized
> > >> >
> > >> > > >
> > >> > > > /**
> > >> > > >  * \var CameraConfiguration::transform
> > >> > > >  * \brief User-specified transform to be applied to the image
> > >> > > >  *
> > >> > > >  * The transform is a user-specified 2D plane transform that will be applied
> > >> > > >  * to the camera images by the processing pipeline before being handed to
> > >> > > >  * the application. This is subsequent to any transform that is already
> > >> > > >  * required to fix up any platform-defined rotation.
> > >> > > >  *
> > >> > > >  * The usual 2D plane transforms are allowed here (horizontal/vertical
> > >> > > >  * flips, multiple of 90-degree rotations etc.), but the validate() function
> > >> > > >  * may adjust this field at its discretion if the selection is not supported.
> > >> > > >  */
> > >> > > >
> > >> > > > The last part in particular, "at its discrection" it's really too
> > >> > > > generic.
> > >> > >
> > >> > > I guess it's hinting at the fact that it's up to the PH, and we don't
> > >> > > mandate the exact way in which it can fail to do what you asked for!
> > >> > > But it would be fine to have a discussion on that.
> > >> > >
> > >> >
> > >> > That's the point that most concerns me :)
> > >> >
> > >> > > >
> > >> > > > Let's start from the beginning: what application should use transform
> > >> > > > for ? I presume they should:
> > >> > > >
> > >> > > > 1) Inspect propertis::Rotation
> > >> > > > 2) Correct the image for the properties::Rotation amount (actually,
> > >> > > > the inverse because of the clockwise/counter-clockwise thing)
> > >> > > > 3) Add any additional rotation they would like on top of this
> > >> > >
> > >> > > So I think the point of the transform is that you *don't* have to
> > >> > > worry about the properties::Rotation. It takes care of that for you.
> > >> > > You say "I want upside down images" and you get them, whatever the
> > >> > > sensor's rotation.
> > >> >
> > >> > That's acceptable as well if we prefer that, but it has to be
> > >> > standardized among pipelines imho
> > >> >
> > >> > >
> > >> > > >
> > >> > > > How should transform be adjusted ?
> > >> > > >
> > >> > > > Identity:
> > >> > > > If application->identity the camera does not have to apply any
> > >> > > > transform. Should the transform be adjutes like it happens today to
> > >> > > > compensate the Rotation (at least that's what I presume happens) ? I
> > >> > > > presume no, applications can infer rotation from properties and if they
> > >> > > > chose Identity either other layers above will do the transform or
> > >> > > > they're fine with images as they are.
> > >> > >
> > >> > > If the application says it wants the "identity" transform, then it
> > >> > > should get output images with no transformation applied. But this does
> > >> > > mean it will undo the sensor rotation (this being the normal case on
> > >> > > the Pi).
> > >> > >
> > >> >
> > >> > right, it depends if we decide to account for the sensor rotation
> > >> > inside libcamera or not
> > >> >
> > >> > > >
> > >> > > > A supported/non-supported transform:
> > >> > > > An application asks for a VFLIP, the camera can do it, transform is
> > >> > > > not changed. If the camera cannot flip, the transform is set back to
> > >> > > > Identity and the state is adjusted.
> > >> > >
> > >> > > Yes, mostly, except that the sensor may be mounted with a rotation,
> > >> > > but if it isn't, then I agree with this.
> > >> > >
> > >> > > >
> > >> > > > A partially non-supported transform:
> > >> > > > An application asks for Rot270 (Transpose | HFLIP) and the camera can
> > >> > > > only do HFLIP. transform sould be then set to HFLIP and applications
> > >> > > > know that the remaining transformation should be handled elsewhere.
> > >> > >
> > >> > > As I suggested above, I'm not sure whether we want to mandate this. I
> > >> > > agree it seems useful behaviour, which is why the Pi does it, but I'm
> > >> > > open minded about requiring it or not.
> > >> > >
> > >> > > >
> > >> > > > Are we missing anything with such behaviour ? I presume it's rather
> > >> > > > similar to what happens today if not for the fact that the validate()
> > >> > > > implementation adjusts transform to rotation in case it cannot flip
> > >> > > >
> > >> > > >         Transform combined = transform * data_->rotationTransform_;
> > >> > > >         if (!data_->supportsFlips_ && !!combined)
> > >> > > >                 transform = -data_->rotationTransform_;
> > >> > > >
> > >> > > > Not really sure I got why.
> > >> > > >
> > >> > > > All of this, but an even more fundamental question: is it too late/is
> > >> > > > it worh it to adjust the Transform definition to adopt the same
> > >> > > > direction as the kernel defined rotation ?
> > >> > > >
> > >> > > > Sorry for the long email :/
> > >> > >
> > >> > > No problem! I like nothing more than discussing transforms, except
> > >> > > possibly colour spaces!
> > >> > >
> > >> >
> > >> > :)
> > >> >
> > >> > > More seriously, here are my key points:
> > >> >
> > >> > I'll try to summarize what I see differently
> > >> >
> > >> > >
> > >> > > 1. The CameraConfiguration::transform field says what the application
> > >> > > wants the final output images to have.
> > >> >
> > >> > 1. The CameraConfiguration::transform field says what transform the
> > >> > library has to apply to images as they arrive from the sensor in its
> > >> > default configuration
> > >> >
> > >> > > 2. It takes account of the sensor rotation. The most obvious example
> > >> > > is the Pi: if the sensor is mounted upside down, and the applications
> > >> > > ask for images "the right way up" (i.e. transform = identity), then
> > >> > > the PH will apply h and v flips to compensate for the sensor mounting.
> > >> >
> > >> > 2. It doesn't automatically account for sensor rotation. Application
> > >> > might decide to do that along the rendering pipeline, in example
> > >> >
> > >> > > 3. If the PH can do what the application requests, it leaves the
> > >> > > transform alone and your configuration is Valid.
> > >> >
> > >> > Agreed
> > >> >
> > >> > > 4. If the PH can't do what the application wants, it will set the
> > >> > > value to something that it can do, and the configuration will be
> > >> > > Adjusted.
> > >> >
> > >> > Agreed
> > >> >
> > >> > >
> > >> > > If folks wanted a short conference call to iron any of this out, I'd
> > >> > > be very happy to join in!
> > >> >
> > >> > I've asked others to have a look here, let's see if we need a call,
> > >> > I'm all for it !
> > >> >
> > >> > Thanks
> > >> >   j
> > >> >
> > >> > >
> > >> > > Thanks
> > >> > > David
> > >> > >
> > >> > > >
> > >> > > > > > Thanks everyone!
> > >> > > > > > David
> > >> > > > >
> > >> > > > > Thanks!
> > >> > > > >
> > >> > > > > Robert
> > >> > > > >