libcamera: android: Add optional mirrored value for config

23832 diff mbox series

Adds an optional `mirrored` value to the HAL config, which is used to modify
the `orientation` of the camera config. No rotation is used, so it can
only set the orientation to `Orientation::Rotate0Mirror`.

This enables sensors which are incorrectly mirrored to be corrected.

Signed-off-by: Allen Ballway <ballway@chromium.org>
---
 src/android/camera_device.cpp     |  6 ++++++
 src/android/camera_device.h       |  1 +
 src/android/camera_hal_config.cpp | 14 ++++++++++++++
 src/android/camera_hal_config.h   |  1 +
 4 files changed, 22 insertions(+)

--
2.50.0.727.gbf7dc18ff4-goog

Quoting Allen Ballway (2025-07-16 21:46:20)
> Adds an optional `mirrored` value to the HAL config, which is used to modify
> the `orientation` of the camera config. No rotation is used, so it can
> only set the orientation to `Orientation::Rotate0Mirror`.
> 
> This enables sensors which are incorrectly mirrored to be corrected.
> 
> Signed-off-by: Allen Ballway <ballway@chromium.org>
> ---
>  src/android/camera_device.cpp     |  6 ++++++
>  src/android/camera_device.h       |  1 +
>  src/android/camera_hal_config.cpp | 14 ++++++++++++++
>  src/android/camera_hal_config.h   |  1 +
>  4 files changed, 22 insertions(+)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 80ff248c2..b20e9f3db 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -377,6 +377,7 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData)
>                 orientation_ = 0;
>         }
> 
> +       mirrored_ = cameraConfigData && cameraConfigData->mirrored;
>         return capabilities_.initialize(camera_, orientation_, facing_);
>  }
> 
> @@ -547,6 +548,11 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>                 return -EINVAL;
>         }
> 
> +       // Rotation is unsupported, so if mirrored just set orientation.

Is that true? I thought we usually support 0 and 180 rotations on most
sensors at least ?

We (almost) always use /*  */ comments in libcamera ... it's ... probably a
stylistic choice because we are mostly started out as C / Linux-kernel
devs.



> +       if (mirrored_) {
> +               config->orientation = Orientation::Rotate0Mirror;
> +       }
> +

How does this interact with rotation as well ? will it take precedence
against 180 rotations?

>         /*
>          * Clear and remove any existing configuration from previous calls, and
>          * ensure the required entries are available without further
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 194ca3030..d304a1285 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -126,6 +126,7 @@ private:
> 
>         int facing_;
>         int orientation_;
> +       int mirrored_;
> 
>         CameraMetadata lastSettings_;
>  };
> diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp
> index 7ef451ef8..a2beba5d3 100644
> --- a/src/android/camera_hal_config.cpp
> +++ b/src/android/camera_hal_config.cpp
> @@ -33,6 +33,7 @@ private:
>         int parseCameraConfigData(const std::string &cameraId, const YamlObject &);
>         int parseLocation(const YamlObject &, CameraConfigData &cameraConfigData);
>         int parseRotation(const YamlObject &, CameraConfigData &cameraConfigData);
> +       int parseMirrored(const YamlObject &, CameraConfigData &cameraConfigData);
> 
>         std::map<std::string, CameraConfigData> *cameras_;
>  };
> @@ -106,6 +107,9 @@ int CameraHalConfig::Private::parseCameraConfigData(const std::string &cameraId,
>         if (parseRotation(cameraObject, cameraConfigData))
>                 return -EINVAL;
> 
> +       /* Parse optional property "mirrored" */
> +       parseMirrored(cameraObject, cameraConfigData);
> +
>         return 0;
>  }
> 
> @@ -145,6 +149,16 @@ int CameraHalConfig::Private::parseRotation(const YamlObject &cameraObject,
>         return 0;
>  }
> 
> +int CameraHalConfig::Private::parseMirrored(const YamlObject &cameraObject,
> +                                           CameraConfigData &cameraConfigData)
> +{
> +       if (!cameraObject.contains("mirrored"))
> +               return -EINVAL;
> +
> +       cameraConfigData.mirrored = cameraObject["mirrored"].get<bool>(false);
> +       return 0;
> +}
> +
>  CameraHalConfig::CameraHalConfig()
>         : Extensible(std::make_unique<Private>()), exists_(false), valid_(false)
>  {
> diff --git a/src/android/camera_hal_config.h b/src/android/camera_hal_config.h
> index a4bedb6e6..a96190470 100644
> --- a/src/android/camera_hal_config.h
> +++ b/src/android/camera_hal_config.h
> @@ -15,6 +15,7 @@
>  struct CameraConfigData {
>         int facing = -1;
>         int rotation = -1;
> +       bool mirrored = false;
>  };
> 
>  class CameraHalConfig final : public libcamera::Extensible
> --
> 2.50.0.727.gbf7dc18ff4-goog
>

On Fri, Jul 18, 2025 at 5:11 AM Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Quoting Allen Ballway (2025-07-16 21:46:20)
> > Adds an optional `mirrored` value to the HAL config, which is used to modify
> > the `orientation` of the camera config. No rotation is used, so it can
> > only set the orientation to `Orientation::Rotate0Mirror`.
> >
> > This enables sensors which are incorrectly mirrored to be corrected.
> >
> > Signed-off-by: Allen Ballway <ballway@chromium.org>
> > ---
> >  src/android/camera_device.cpp     |  6 ++++++
> >  src/android/camera_device.h       |  1 +
> >  src/android/camera_hal_config.cpp | 14 ++++++++++++++
> >  src/android/camera_hal_config.h   |  1 +
> >  4 files changed, 22 insertions(+)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 80ff248c2..b20e9f3db 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -377,6 +377,7 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData)
> >                 orientation_ = 0;
> >         }
> >
> > +       mirrored_ = cameraConfigData && cameraConfigData->mirrored;
> >         return capabilities_.initialize(camera_, orientation_, facing_);
> >  }
> >
> > @@ -547,6 +548,11 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >                 return -EINVAL;
> >         }
> >
> > +       // Rotation is unsupported, so if mirrored just set orientation.
>
> Is that true? I thought we usually support 0 and 180 rotations on most
> sensors at least ?

Ah I misunderstood the below comments/errors about not supporting stream
rotations. It seems the sensors on my test device don't support rotation. I will
send out a v2 with this comment removed assuming the rest of this patch
looks good.
>
> We (almost) always use /*  */ comments in libcamera ... it's ... probably a
> stylistic choice because we are mostly started out as C / Linux-kernel
> devs.
>
>
>
> > +       if (mirrored_) {
> > +               config->orientation = Orientation::Rotate0Mirror;
> > +       }
> > +
>
> How does this interact with rotation as well ? will it take precedence
> against 180 rotations?

I can't confirm because my sensors aren't affected by the rotation config
but as far as I can tell the rotation would be unaffected by setting the
orientation to mirror. There doesn't seem to be anywhere setting the
orientation in the Android layer, so any rotation would happen independently.
And the mirroring will just set the HFLIP on the kernel driver, so any higher
level rotation would just rotate the flipped output as expected.
>
> >         /*
> >          * Clear and remove any existing configuration from previous calls, and
> >          * ensure the required entries are available without further
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index 194ca3030..d304a1285 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -126,6 +126,7 @@ private:
> >
> >         int facing_;
> >         int orientation_;
> > +       int mirrored_;
> >
> >         CameraMetadata lastSettings_;
> >  };
> > diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp
> > index 7ef451ef8..a2beba5d3 100644
> > --- a/src/android/camera_hal_config.cpp
> > +++ b/src/android/camera_hal_config.cpp
> > @@ -33,6 +33,7 @@ private:
> >         int parseCameraConfigData(const std::string &cameraId, const YamlObject &);
> >         int parseLocation(const YamlObject &, CameraConfigData &cameraConfigData);
> >         int parseRotation(const YamlObject &, CameraConfigData &cameraConfigData);
> > +       int parseMirrored(const YamlObject &, CameraConfigData &cameraConfigData);
> >
> >         std::map<std::string, CameraConfigData> *cameras_;
> >  };
> > @@ -106,6 +107,9 @@ int CameraHalConfig::Private::parseCameraConfigData(const std::string &cameraId,
> >         if (parseRotation(cameraObject, cameraConfigData))
> >                 return -EINVAL;
> >
> > +       /* Parse optional property "mirrored" */
> > +       parseMirrored(cameraObject, cameraConfigData);
> > +
> >         return 0;
> >  }
> >
> > @@ -145,6 +149,16 @@ int CameraHalConfig::Private::parseRotation(const YamlObject &cameraObject,
> >         return 0;
> >  }
> >
> > +int CameraHalConfig::Private::parseMirrored(const YamlObject &cameraObject,
> > +                                           CameraConfigData &cameraConfigData)
> > +{
> > +       if (!cameraObject.contains("mirrored"))
> > +               return -EINVAL;
> > +
> > +       cameraConfigData.mirrored = cameraObject["mirrored"].get<bool>(false);
> > +       return 0;
> > +}
> > +
> >  CameraHalConfig::CameraHalConfig()
> >         : Extensible(std::make_unique<Private>()), exists_(false), valid_(false)
> >  {
> > diff --git a/src/android/camera_hal_config.h b/src/android/camera_hal_config.h
> > index a4bedb6e6..a96190470 100644
> > --- a/src/android/camera_hal_config.h
> > +++ b/src/android/camera_hal_config.h
> > @@ -15,6 +15,7 @@
> >  struct CameraConfigData {
> >         int facing = -1;
> >         int rotation = -1;
> > +       bool mirrored = false;
> >  };
> >
> >  class CameraHalConfig final : public libcamera::Extensible
> > --
> > 2.50.0.727.gbf7dc18ff4-goog
> >

Quoting Allen Ballway (2025-07-18 16:27:23)
> On Fri, Jul 18, 2025 at 5:11 AM Kieran Bingham
> <kieran.bingham@ideasonboard.com> wrote:
> >
> > Quoting Allen Ballway (2025-07-16 21:46:20)
> > > Adds an optional `mirrored` value to the HAL config, which is used to modify
> > > the `orientation` of the camera config. No rotation is used, so it can
> > > only set the orientation to `Orientation::Rotate0Mirror`.
> > >
> > > This enables sensors which are incorrectly mirrored to be corrected.
> > >
> > > Signed-off-by: Allen Ballway <ballway@chromium.org>
> > > ---
> > >  src/android/camera_device.cpp     |  6 ++++++
> > >  src/android/camera_device.h       |  1 +
> > >  src/android/camera_hal_config.cpp | 14 ++++++++++++++
> > >  src/android/camera_hal_config.h   |  1 +
> > >  4 files changed, 22 insertions(+)
> > >
> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > index 80ff248c2..b20e9f3db 100644
> > > --- a/src/android/camera_device.cpp
> > > +++ b/src/android/camera_device.cpp
> > > @@ -377,6 +377,7 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData)
> > >                 orientation_ = 0;
> > >         }
> > >
> > > +       mirrored_ = cameraConfigData && cameraConfigData->mirrored;
> > >         return capabilities_.initialize(camera_, orientation_, facing_);
> > >  }
> > >
> > > @@ -547,6 +548,11 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> > >                 return -EINVAL;
> > >         }
> > >
> > > +       // Rotation is unsupported, so if mirrored just set orientation.
> >
> > Is that true? I thought we usually support 0 and 180 rotations on most
> > sensors at least ?
> 
> Ah I misunderstood the below comments/errors about not supporting stream
> rotations. It seems the sensors on my test device don't support rotation. I will
> send out a v2 with this comment removed assuming the rest of this patch
> looks good.
> >
> > We (almost) always use /*  */ comments in libcamera ... it's ... probably a
> > stylistic choice because we are mostly started out as C / Linux-kernel
> > devs.
> >
> >
> >
> > > +       if (mirrored_) {
> > > +               config->orientation = Orientation::Rotate0Mirror;
> > > +       }
> > > +
> >
> > How does this interact with rotation as well ? will it take precedence
> > against 180 rotations?
> 
> I can't confirm because my sensors aren't affected by the rotation config
> but as far as I can tell the rotation would be unaffected by setting the
> orientation to mirror. There doesn't seem to be anywhere setting the
> orientation in the Android layer, so any rotation would happen independently.
> And the mirroring will just set the HFLIP on the kernel driver, so any higher
> level rotation would just rotate the flipped output as expected.

Some level of rotation is supposed to be handled automatically by
setting the physical orientation and rotation in device tree.

Then libcamera will 'aim' for a 0 rotation by default.

I guess the issue here is that we don't have a way in
device-tree/firmware to mark that the camera is mirrored.

what does this actually 'mean' by the way? How is the camera physically
mirrored? Is this really a bug in the driver somewhere or something
else?

I am weary that this patch is possibly a workaround for a problem
somewhere else ?

--
Kieran

> >
> > >         /*
> > >          * Clear and remove any existing configuration from previous calls, and
> > >          * ensure the required entries are available without further
> > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > > index 194ca3030..d304a1285 100644
> > > --- a/src/android/camera_device.h
> > > +++ b/src/android/camera_device.h
> > > @@ -126,6 +126,7 @@ private:
> > >
> > >         int facing_;
> > >         int orientation_;
> > > +       int mirrored_;
> > >
> > >         CameraMetadata lastSettings_;
> > >  };
> > > diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp
> > > index 7ef451ef8..a2beba5d3 100644
> > > --- a/src/android/camera_hal_config.cpp
> > > +++ b/src/android/camera_hal_config.cpp
> > > @@ -33,6 +33,7 @@ private:
> > >         int parseCameraConfigData(const std::string &cameraId, const YamlObject &);
> > >         int parseLocation(const YamlObject &, CameraConfigData &cameraConfigData);
> > >         int parseRotation(const YamlObject &, CameraConfigData &cameraConfigData);
> > > +       int parseMirrored(const YamlObject &, CameraConfigData &cameraConfigData);
> > >
> > >         std::map<std::string, CameraConfigData> *cameras_;
> > >  };
> > > @@ -106,6 +107,9 @@ int CameraHalConfig::Private::parseCameraConfigData(const std::string &cameraId,
> > >         if (parseRotation(cameraObject, cameraConfigData))
> > >                 return -EINVAL;
> > >
> > > +       /* Parse optional property "mirrored" */
> > > +       parseMirrored(cameraObject, cameraConfigData);
> > > +
> > >         return 0;
> > >  }
> > >
> > > @@ -145,6 +149,16 @@ int CameraHalConfig::Private::parseRotation(const YamlObject &cameraObject,
> > >         return 0;
> > >  }
> > >
> > > +int CameraHalConfig::Private::parseMirrored(const YamlObject &cameraObject,
> > > +                                           CameraConfigData &cameraConfigData)
> > > +{
> > > +       if (!cameraObject.contains("mirrored"))
> > > +               return -EINVAL;
> > > +
> > > +       cameraConfigData.mirrored = cameraObject["mirrored"].get<bool>(false);
> > > +       return 0;
> > > +}
> > > +
> > >  CameraHalConfig::CameraHalConfig()
> > >         : Extensible(std::make_unique<Private>()), exists_(false), valid_(false)
> > >  {
> > > diff --git a/src/android/camera_hal_config.h b/src/android/camera_hal_config.h
> > > index a4bedb6e6..a96190470 100644
> > > --- a/src/android/camera_hal_config.h
> > > +++ b/src/android/camera_hal_config.h
> > > @@ -15,6 +15,7 @@
> > >  struct CameraConfigData {
> > >         int facing = -1;
> > >         int rotation = -1;
> > > +       bool mirrored = false;
> > >  };
> > >
> > >  class CameraHalConfig final : public libcamera::Extensible
> > > --
> > > 2.50.0.727.gbf7dc18ff4-goog
> > >

Hi Kieran

On Mon, Jul 21, 2025 at 11:44:18AM +0100, Kieran Bingham wrote:
> Quoting Allen Ballway (2025-07-18 16:27:23)
> > On Fri, Jul 18, 2025 at 5:11 AM Kieran Bingham
> > <kieran.bingham@ideasonboard.com> wrote:
> > >
> > > Quoting Allen Ballway (2025-07-16 21:46:20)
> > > > Adds an optional `mirrored` value to the HAL config, which is used to modify
> > > > the `orientation` of the camera config. No rotation is used, so it can
> > > > only set the orientation to `Orientation::Rotate0Mirror`.
> > > >
> > > > This enables sensors which are incorrectly mirrored to be corrected.
> > > >
> > > > Signed-off-by: Allen Ballway <ballway@chromium.org>
> > > > ---
> > > >  src/android/camera_device.cpp     |  6 ++++++
> > > >  src/android/camera_device.h       |  1 +
> > > >  src/android/camera_hal_config.cpp | 14 ++++++++++++++
> > > >  src/android/camera_hal_config.h   |  1 +
> > > >  4 files changed, 22 insertions(+)
> > > >
> > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > > index 80ff248c2..b20e9f3db 100644
> > > > --- a/src/android/camera_device.cpp
> > > > +++ b/src/android/camera_device.cpp
> > > > @@ -377,6 +377,7 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData)
> > > >                 orientation_ = 0;
> > > >         }
> > > >
> > > > +       mirrored_ = cameraConfigData && cameraConfigData->mirrored;
> > > >         return capabilities_.initialize(camera_, orientation_, facing_);
> > > >  }
> > > >
> > > > @@ -547,6 +548,11 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> > > >                 return -EINVAL;
> > > >         }
> > > >
> > > > +       // Rotation is unsupported, so if mirrored just set orientation.
> > >
> > > Is that true? I thought we usually support 0 and 180 rotations on most
> > > sensors at least ?
> >
> > Ah I misunderstood the below comments/errors about not supporting stream
> > rotations. It seems the sensors on my test device don't support rotation. I will
> > send out a v2 with this comment removed assuming the rest of this patch
> > looks good.
> > >
> > > We (almost) always use /*  */ comments in libcamera ... it's ... probably a
> > > stylistic choice because we are mostly started out as C / Linux-kernel
> > > devs.
> > >
> > >
> > >
> > > > +       if (mirrored_) {
> > > > +               config->orientation = Orientation::Rotate0Mirror;
> > > > +       }
> > > > +

Also no {} for single line statement.

It's amusing how many times the same stylistic comments had to
repeated specifically to chromeos developers. We also have tools
available for checking this kind of issues before submitting.

> > >
> > > How does this interact with rotation as well ? will it take precedence
> > > against 180 rotations?
> >
> > I can't confirm because my sensors aren't affected by the rotation config
> > but as far as I can tell the rotation would be unaffected by setting the
> > orientation to mirror. There doesn't seem to be anywhere setting the
> > orientation in the Android layer, so any rotation would happen independently.
> > And the mirroring will just set the HFLIP on the kernel driver, so any higher
> > level rotation would just rotate the flipped output as expected.
>
> Some level of rotation is supposed to be handled automatically by
> setting the physical orientation and rotation in device tree.
>
> Then libcamera will 'aim' for a 0 rotation by default.
>
> I guess the issue here is that we don't have a way in
> device-tree/firmware to mark that the camera is mirrored.
>
> what does this actually 'mean' by the way? How is the camera physically
> mirrored? Is this really a bug in the driver somewhere or something
> else?
>
> I am weary that this patch is possibly a workaround for a problem
> somewhere else ?

If my recollection is right, when it comes to the camera HAL:

1) Rotation is first parsed from DT, but there we can only express
   [0, 90, 270, 360] so we don't have a "mirror" option

2) If no "rotation" property is specified in DT, then the HAL
   falls-back  to parse a "rotation" property from configuration file,
   where again we can only express an angle between [0 - 360]

All of this seems to suggest it is not possible to express in DT or
HAL configuration file the "mirroring" option. However this opens the
question that Kieran has asked already. How is the camera module
physically mirrored ? is the pixel array reading direction inverted in
the driver ?

>
> --
> Kieran
>
> > >
> > > >         /*
> > > >          * Clear and remove any existing configuration from previous calls, and
> > > >          * ensure the required entries are available without further
> > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > > > index 194ca3030..d304a1285 100644
> > > > --- a/src/android/camera_device.h
> > > > +++ b/src/android/camera_device.h
> > > > @@ -126,6 +126,7 @@ private:
> > > >
> > > >         int facing_;
> > > >         int orientation_;
> > > > +       int mirrored_;
> > > >
> > > >         CameraMetadata lastSettings_;
> > > >  };
> > > > diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp
> > > > index 7ef451ef8..a2beba5d3 100644
> > > > --- a/src/android/camera_hal_config.cpp
> > > > +++ b/src/android/camera_hal_config.cpp
> > > > @@ -33,6 +33,7 @@ private:
> > > >         int parseCameraConfigData(const std::string &cameraId, const YamlObject &);
> > > >         int parseLocation(const YamlObject &, CameraConfigData &cameraConfigData);
> > > >         int parseRotation(const YamlObject &, CameraConfigData &cameraConfigData);
> > > > +       int parseMirrored(const YamlObject &, CameraConfigData &cameraConfigData);
> > > >
> > > >         std::map<std::string, CameraConfigData> *cameras_;
> > > >  };
> > > > @@ -106,6 +107,9 @@ int CameraHalConfig::Private::parseCameraConfigData(const std::string &cameraId,
> > > >         if (parseRotation(cameraObject, cameraConfigData))
> > > >                 return -EINVAL;
> > > >
> > > > +       /* Parse optional property "mirrored" */
> > > > +       parseMirrored(cameraObject, cameraConfigData);
> > > > +
> > > >         return 0;
> > > >  }
> > > >
> > > > @@ -145,6 +149,16 @@ int CameraHalConfig::Private::parseRotation(const YamlObject &cameraObject,
> > > >         return 0;
> > > >  }
> > > >
> > > > +int CameraHalConfig::Private::parseMirrored(const YamlObject &cameraObject,
> > > > +                                           CameraConfigData &cameraConfigData)
> > > > +{
> > > > +       if (!cameraObject.contains("mirrored"))
> > > > +               return -EINVAL;
> > > > +
> > > > +       cameraConfigData.mirrored = cameraObject["mirrored"].get<bool>(false);
> > > > +       return 0;
> > > > +}
> > > > +
> > > >  CameraHalConfig::CameraHalConfig()
> > > >         : Extensible(std::make_unique<Private>()), exists_(false), valid_(false)
> > > >  {
> > > > diff --git a/src/android/camera_hal_config.h b/src/android/camera_hal_config.h
> > > > index a4bedb6e6..a96190470 100644
> > > > --- a/src/android/camera_hal_config.h
> > > > +++ b/src/android/camera_hal_config.h
> > > > @@ -15,6 +15,7 @@
> > > >  struct CameraConfigData {
> > > >         int facing = -1;
> > > >         int rotation = -1;
> > > > +       bool mirrored = false;
> > > >  };
> > > >
> > > >  class CameraHalConfig final : public libcamera::Extensible
> > > > --
> > > > 2.50.0.727.gbf7dc18ff4-goog
> > > >

Hi all,

On Mon, Jul 21, 2025 at 4:14 AM Jacopo Mondi
<jacopo.mondi@ideasonboard.com> wrote:
>
> Hi Kieran
>
> On Mon, Jul 21, 2025 at 11:44:18AM +0100, Kieran Bingham wrote:
> > Quoting Allen Ballway (2025-07-18 16:27:23)
> > > On Fri, Jul 18, 2025 at 5:11 AM Kieran Bingham
> > > <kieran.bingham@ideasonboard.com> wrote:
> > > >
> > > > Quoting Allen Ballway (2025-07-16 21:46:20)
> > > > > Adds an optional `mirrored` value to the HAL config, which is used to modify
> > > > > the `orientation` of the camera config. No rotation is used, so it can
> > > > > only set the orientation to `Orientation::Rotate0Mirror`.
> > > > >
> > > > > This enables sensors which are incorrectly mirrored to be corrected.
> > > > >
> > > > > Signed-off-by: Allen Ballway <ballway@chromium.org>
> > > > > ---
> > > > >  src/android/camera_device.cpp     |  6 ++++++
> > > > >  src/android/camera_device.h       |  1 +
> > > > >  src/android/camera_hal_config.cpp | 14 ++++++++++++++
> > > > >  src/android/camera_hal_config.h   |  1 +
> > > > >  4 files changed, 22 insertions(+)
> > > > >
> > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > > > index 80ff248c2..b20e9f3db 100644
> > > > > --- a/src/android/camera_device.cpp
> > > > > +++ b/src/android/camera_device.cpp
> > > > > @@ -377,6 +377,7 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData)
> > > > >                 orientation_ = 0;
> > > > >         }
> > > > >
> > > > > +       mirrored_ = cameraConfigData && cameraConfigData->mirrored;
> > > > >         return capabilities_.initialize(camera_, orientation_, facing_);
> > > > >  }
> > > > >
> > > > > @@ -547,6 +548,11 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> > > > >                 return -EINVAL;
> > > > >         }
> > > > >
> > > > > +       // Rotation is unsupported, so if mirrored just set orientation.
> > > >
> > > > Is that true? I thought we usually support 0 and 180 rotations on most
> > > > sensors at least ?
> > >
> > > Ah I misunderstood the below comments/errors about not supporting stream
> > > rotations. It seems the sensors on my test device don't support rotation. I will
> > > send out a v2 with this comment removed assuming the rest of this patch
> > > looks good.
> > > >
> > > > We (almost) always use /*  */ comments in libcamera ... it's ... probably a
> > > > stylistic choice because we are mostly started out as C / Linux-kernel
> > > > devs.
> > > >
> > > >
> > > >
> > > > > +       if (mirrored_) {
> > > > > +               config->orientation = Orientation::Rotate0Mirror;
> > > > > +       }
> > > > > +
>
> Also no {} for single line statement.
>
> It's amusing how many times the same stylistic comments had to
> repeated specifically to chromeos developers. We also have tools
> available for checking this kind of issues before submitting.
>

Apologies, I ran checkstyle.py on the change but found no issues, if there
are other tools for finding style issues I'll be sure to run them in the future.
I'll update this one as well in v2.
> > > >
> > > > How does this interact with rotation as well ? will it take precedence
> > > > against 180 rotations?
> > >
> > > I can't confirm because my sensors aren't affected by the rotation config
> > > but as far as I can tell the rotation would be unaffected by setting the
> > > orientation to mirror. There doesn't seem to be anywhere setting the
> > > orientation in the Android layer, so any rotation would happen independently.
> > > And the mirroring will just set the HFLIP on the kernel driver, so any higher
> > > level rotation would just rotate the flipped output as expected.
> >
> > Some level of rotation is supposed to be handled automatically by
> > setting the physical orientation and rotation in device tree.
> >
> > Then libcamera will 'aim' for a 0 rotation by default.
> >
> > I guess the issue here is that we don't have a way in
> > device-tree/firmware to mark that the camera is mirrored.
> >
> > what does this actually 'mean' by the way? How is the camera physically
> > mirrored? Is this really a bug in the driver somewhere or something
> > else?
> >
> > I am weary that this patch is possibly a workaround for a problem
> > somewhere else ?
>
> If my recollection is right, when it comes to the camera HAL:
>
> 1) Rotation is first parsed from DT, but there we can only express
>    [0, 90, 270, 360] so we don't have a "mirror" option
>
> 2) If no "rotation" property is specified in DT, then the HAL
>    falls-back  to parse a "rotation" property from configuration file,
>    where again we can only express an angle between [0 - 360]
>
> All of this seems to suggest it is not possible to express in DT or
> HAL configuration file the "mirroring" option. However this opens the
> question that Kieran has asked already. How is the camera module
> physically mirrored ? is the pixel array reading direction inverted in
> the driver ?
>

I'm not 100% sure where the mirroring is coming from. I confirmed it
through qcam on Ubuntu as well as through the camera app and Chromium
on a ChromiumOs build so it's not just an application bug. The location and
orientation coming from ACPI both seem to be correct. The driver seems
to be fine on all but some Surface Go devices so it seems unrelated.
Anything below that is infeasible to fix and the sensor drivers don't
maintain quirks for particular misbehaving devices, so the HAL seems
to me the best place to add a mirrored config and pipe it down to the
driver through the existing Orientation.

Thanks,
Allen
> >
> > --
> > Kieran
> >
> > > >
> > > > >         /*
> > > > >          * Clear and remove any existing configuration from previous calls, and
> > > > >          * ensure the required entries are available without further
> > > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > > > > index 194ca3030..d304a1285 100644
> > > > > --- a/src/android/camera_device.h
> > > > > +++ b/src/android/camera_device.h
> > > > > @@ -126,6 +126,7 @@ private:
> > > > >
> > > > >         int facing_;
> > > > >         int orientation_;
> > > > > +       int mirrored_;
> > > > >
> > > > >         CameraMetadata lastSettings_;
> > > > >  };
> > > > > diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp
> > > > > index 7ef451ef8..a2beba5d3 100644
> > > > > --- a/src/android/camera_hal_config.cpp
> > > > > +++ b/src/android/camera_hal_config.cpp
> > > > > @@ -33,6 +33,7 @@ private:
> > > > >         int parseCameraConfigData(const std::string &cameraId, const YamlObject &);
> > > > >         int parseLocation(const YamlObject &, CameraConfigData &cameraConfigData);
> > > > >         int parseRotation(const YamlObject &, CameraConfigData &cameraConfigData);
> > > > > +       int parseMirrored(const YamlObject &, CameraConfigData &cameraConfigData);
> > > > >
> > > > >         std::map<std::string, CameraConfigData> *cameras_;
> > > > >  };
> > > > > @@ -106,6 +107,9 @@ int CameraHalConfig::Private::parseCameraConfigData(const std::string &cameraId,
> > > > >         if (parseRotation(cameraObject, cameraConfigData))
> > > > >                 return -EINVAL;
> > > > >
> > > > > +       /* Parse optional property "mirrored" */
> > > > > +       parseMirrored(cameraObject, cameraConfigData);
> > > > > +
> > > > >         return 0;
> > > > >  }
> > > > >
> > > > > @@ -145,6 +149,16 @@ int CameraHalConfig::Private::parseRotation(const YamlObject &cameraObject,
> > > > >         return 0;
> > > > >  }
> > > > >
> > > > > +int CameraHalConfig::Private::parseMirrored(const YamlObject &cameraObject,
> > > > > +                                           CameraConfigData &cameraConfigData)
> > > > > +{
> > > > > +       if (!cameraObject.contains("mirrored"))
> > > > > +               return -EINVAL;
> > > > > +
> > > > > +       cameraConfigData.mirrored = cameraObject["mirrored"].get<bool>(false);
> > > > > +       return 0;
> > > > > +}
> > > > > +
> > > > >  CameraHalConfig::CameraHalConfig()
> > > > >         : Extensible(std::make_unique<Private>()), exists_(false), valid_(false)
> > > > >  {
> > > > > diff --git a/src/android/camera_hal_config.h b/src/android/camera_hal_config.h
> > > > > index a4bedb6e6..a96190470 100644
> > > > > --- a/src/android/camera_hal_config.h
> > > > > +++ b/src/android/camera_hal_config.h
> > > > > @@ -15,6 +15,7 @@
> > > > >  struct CameraConfigData {
> > > > >         int facing = -1;
> > > > >         int rotation = -1;
> > > > > +       bool mirrored = false;
> > > > >  };
> > > > >
> > > > >  class CameraHalConfig final : public libcamera::Extensible
> > > > > --
> > > > > 2.50.0.727.gbf7dc18ff4-goog
> > > > >

Hi Allen

On Tue, Jul 22, 2025 at 07:51:53AM -0700, Allen Ballway wrote:
> Hi all,
>
> On Mon, Jul 21, 2025 at 4:14 AM Jacopo Mondi
> <jacopo.mondi@ideasonboard.com> wrote:
> >
> > Hi Kieran
> >
> > On Mon, Jul 21, 2025 at 11:44:18AM +0100, Kieran Bingham wrote:
> > > Quoting Allen Ballway (2025-07-18 16:27:23)
> > > > On Fri, Jul 18, 2025 at 5:11 AM Kieran Bingham
> > > > <kieran.bingham@ideasonboard.com> wrote:
> > > > >
> > > > > Quoting Allen Ballway (2025-07-16 21:46:20)
> > > > > > Adds an optional `mirrored` value to the HAL config, which is used to modify
> > > > > > the `orientation` of the camera config. No rotation is used, so it can
> > > > > > only set the orientation to `Orientation::Rotate0Mirror`.
> > > > > >
> > > > > > This enables sensors which are incorrectly mirrored to be corrected.
> > > > > >
> > > > > > Signed-off-by: Allen Ballway <ballway@chromium.org>
> > > > > > ---
> > > > > >  src/android/camera_device.cpp     |  6 ++++++
> > > > > >  src/android/camera_device.h       |  1 +
> > > > > >  src/android/camera_hal_config.cpp | 14 ++++++++++++++
> > > > > >  src/android/camera_hal_config.h   |  1 +
> > > > > >  4 files changed, 22 insertions(+)
> > > > > >
> > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > > > > index 80ff248c2..b20e9f3db 100644
> > > > > > --- a/src/android/camera_device.cpp
> > > > > > +++ b/src/android/camera_device.cpp
> > > > > > @@ -377,6 +377,7 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData)
> > > > > >                 orientation_ = 0;
> > > > > >         }
> > > > > >
> > > > > > +       mirrored_ = cameraConfigData && cameraConfigData->mirrored;
> > > > > >         return capabilities_.initialize(camera_, orientation_, facing_);
> > > > > >  }
> > > > > >
> > > > > > @@ -547,6 +548,11 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> > > > > >                 return -EINVAL;
> > > > > >         }
> > > > > >
> > > > > > +       // Rotation is unsupported, so if mirrored just set orientation.
> > > > >
> > > > > Is that true? I thought we usually support 0 and 180 rotations on most
> > > > > sensors at least ?
> > > >
> > > > Ah I misunderstood the below comments/errors about not supporting stream
> > > > rotations. It seems the sensors on my test device don't support rotation. I will
> > > > send out a v2 with this comment removed assuming the rest of this patch
> > > > looks good.
> > > > >
> > > > > We (almost) always use /*  */ comments in libcamera ... it's ... probably a
> > > > > stylistic choice because we are mostly started out as C / Linux-kernel
> > > > > devs.
> > > > >
> > > > >
> > > > >
> > > > > > +       if (mirrored_) {
> > > > > > +               config->orientation = Orientation::Rotate0Mirror;
> > > > > > +       }
> > > > > > +
> >
> > Also no {} for single line statement.
> >
> > It's amusing how many times the same stylistic comments had to
> > repeated specifically to chromeos developers. We also have tools
> > available for checking this kind of issues before submitting.
> >
>
> Apologies, I ran checkstyle.py on the change but found no issues, if there
> are other tools for finding style issues I'll be sure to run them in the future.
> I'll update this one as well in v2.

This is one of your first contributions, so it has been unfair ranting
to you about the past :)

I'm just suprised a team of specialized and highly skilled engineers
continue pushing their conventions (out of habit I presume) to a different
code base. My frustration comes from having repeated the same comments
over and over in the past years. Apologies if you got ranted to.

> > > > >
> > > > > How does this interact with rotation as well ? will it take precedence
> > > > > against 180 rotations?
> > > >
> > > > I can't confirm because my sensors aren't affected by the rotation config
> > > > but as far as I can tell the rotation would be unaffected by setting the
> > > > orientation to mirror. There doesn't seem to be anywhere setting the
> > > > orientation in the Android layer, so any rotation would happen independently.
> > > > And the mirroring will just set the HFLIP on the kernel driver, so any higher
> > > > level rotation would just rotate the flipped output as expected.
> > >
> > > Some level of rotation is supposed to be handled automatically by
> > > setting the physical orientation and rotation in device tree.
> > >
> > > Then libcamera will 'aim' for a 0 rotation by default.
> > >
> > > I guess the issue here is that we don't have a way in
> > > device-tree/firmware to mark that the camera is mirrored.
> > >
> > > what does this actually 'mean' by the way? How is the camera physically
> > > mirrored? Is this really a bug in the driver somewhere or something
> > > else?
> > >
> > > I am weary that this patch is possibly a workaround for a problem
> > > somewhere else ?
> >
> > If my recollection is right, when it comes to the camera HAL:
> >
> > 1) Rotation is first parsed from DT, but there we can only express
> >    [0, 90, 270, 360] so we don't have a "mirror" option
> >
> > 2) If no "rotation" property is specified in DT, then the HAL
> >    falls-back  to parse a "rotation" property from configuration file,
> >    where again we can only express an angle between [0 - 360]
> >
> > All of this seems to suggest it is not possible to express in DT or
> > HAL configuration file the "mirroring" option. However this opens the
> > question that Kieran has asked already. How is the camera module
> > physically mirrored ? is the pixel array reading direction inverted in
> > the driver ?
> >
>
> I'm not 100% sure where the mirroring is coming from. I confirmed it
> through qcam on Ubuntu as well as through the camera app and Chromium
> on a ChromiumOs build so it's not just an application bug. The location and
> orientation coming from ACPI both seem to be correct. The driver seems

The 'rotation' property from DTS cannot express 'mirrored', and to me
this seems correct, as 'rotation' is about expressing the mounting
rotation of the camera sensor (0, 90, 180, 270)

Mirroring comes from inverting the reading directions of the pixel
units on the pixel array, what is usually called an HFLIP in v4l2
drivers and we require drivers to not apply any V/HFLIP by default
(something we might want to document in libcamera as well, not just in
Linux).

> to be fine on all but some Surface Go devices so it seems unrelated.

What do you mean with "seems fine" ? It doesn't mirror ?

> Anything below that is infeasible to fix and the sensor drivers don't
> maintain quirks for particular misbehaving devices, so the HAL seems
> to me the best place to add a mirrored config and pipe it down to the
> driver through the existing Orientation.

Which sensor are we talking about and where does its driver live ? Can
we see it ? Because some downstream (but also upstream) drivers
sometimes flip by default.


>
> Thanks,
> Allen
> > >
> > > --
> > > Kieran
> > >
> > > > >
> > > > > >         /*
> > > > > >          * Clear and remove any existing configuration from previous calls, and
> > > > > >          * ensure the required entries are available without further
> > > > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > > > > > index 194ca3030..d304a1285 100644
> > > > > > --- a/src/android/camera_device.h
> > > > > > +++ b/src/android/camera_device.h
> > > > > > @@ -126,6 +126,7 @@ private:
> > > > > >
> > > > > >         int facing_;
> > > > > >         int orientation_;
> > > > > > +       int mirrored_;
> > > > > >
> > > > > >         CameraMetadata lastSettings_;
> > > > > >  };
> > > > > > diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp
> > > > > > index 7ef451ef8..a2beba5d3 100644
> > > > > > --- a/src/android/camera_hal_config.cpp
> > > > > > +++ b/src/android/camera_hal_config.cpp
> > > > > > @@ -33,6 +33,7 @@ private:
> > > > > >         int parseCameraConfigData(const std::string &cameraId, const YamlObject &);
> > > > > >         int parseLocation(const YamlObject &, CameraConfigData &cameraConfigData);
> > > > > >         int parseRotation(const YamlObject &, CameraConfigData &cameraConfigData);
> > > > > > +       int parseMirrored(const YamlObject &, CameraConfigData &cameraConfigData);
> > > > > >
> > > > > >         std::map<std::string, CameraConfigData> *cameras_;
> > > > > >  };
> > > > > > @@ -106,6 +107,9 @@ int CameraHalConfig::Private::parseCameraConfigData(const std::string &cameraId,
> > > > > >         if (parseRotation(cameraObject, cameraConfigData))
> > > > > >                 return -EINVAL;
> > > > > >
> > > > > > +       /* Parse optional property "mirrored" */
> > > > > > +       parseMirrored(cameraObject, cameraConfigData);
> > > > > > +
> > > > > >         return 0;
> > > > > >  }
> > > > > >
> > > > > > @@ -145,6 +149,16 @@ int CameraHalConfig::Private::parseRotation(const YamlObject &cameraObject,
> > > > > >         return 0;
> > > > > >  }
> > > > > >
> > > > > > +int CameraHalConfig::Private::parseMirrored(const YamlObject &cameraObject,
> > > > > > +                                           CameraConfigData &cameraConfigData)
> > > > > > +{
> > > > > > +       if (!cameraObject.contains("mirrored"))
> > > > > > +               return -EINVAL;
> > > > > > +
> > > > > > +       cameraConfigData.mirrored = cameraObject["mirrored"].get<bool>(false);
> > > > > > +       return 0;
> > > > > > +}
> > > > > > +
> > > > > >  CameraHalConfig::CameraHalConfig()
> > > > > >         : Extensible(std::make_unique<Private>()), exists_(false), valid_(false)
> > > > > >  {
> > > > > > diff --git a/src/android/camera_hal_config.h b/src/android/camera_hal_config.h
> > > > > > index a4bedb6e6..a96190470 100644
> > > > > > --- a/src/android/camera_hal_config.h
> > > > > > +++ b/src/android/camera_hal_config.h
> > > > > > @@ -15,6 +15,7 @@
> > > > > >  struct CameraConfigData {
> > > > > >         int facing = -1;
> > > > > >         int rotation = -1;
> > > > > > +       bool mirrored = false;
> > > > > >  };
> > > > > >
> > > > > >  class CameraHalConfig final : public libcamera::Extensible
> > > > > > --
> > > > > > 2.50.0.727.gbf7dc18ff4-goog
> > > > > >

Hi Jacopo,

On Wed, Jul 23, 2025 at 1:15 AM Jacopo Mondi
<jacopo.mondi@ideasonboard.com> wrote:
>
> Hi Allen
>
> On Tue, Jul 22, 2025 at 07:51:53AM -0700, Allen Ballway wrote:
> > Hi all,
> >
> > On Mon, Jul 21, 2025 at 4:14 AM Jacopo Mondi
> > <jacopo.mondi@ideasonboard.com> wrote:
> > >
> > > Hi Kieran
> > >
> > > On Mon, Jul 21, 2025 at 11:44:18AM +0100, Kieran Bingham wrote:
> > > > Quoting Allen Ballway (2025-07-18 16:27:23)
> > > > > On Fri, Jul 18, 2025 at 5:11 AM Kieran Bingham
> > > > > <kieran.bingham@ideasonboard.com> wrote:
> > > > > >
> > > > > > Quoting Allen Ballway (2025-07-16 21:46:20)
> > > > > > > Adds an optional `mirrored` value to the HAL config, which is used to modify
> > > > > > > the `orientation` of the camera config. No rotation is used, so it can
> > > > > > > only set the orientation to `Orientation::Rotate0Mirror`.
> > > > > > >
> > > > > > > This enables sensors which are incorrectly mirrored to be corrected.
> > > > > > >
> > > > > > > Signed-off-by: Allen Ballway <ballway@chromium.org>
> > > > > > > ---
> > > > > > >  src/android/camera_device.cpp     |  6 ++++++
> > > > > > >  src/android/camera_device.h       |  1 +
> > > > > > >  src/android/camera_hal_config.cpp | 14 ++++++++++++++
> > > > > > >  src/android/camera_hal_config.h   |  1 +
> > > > > > >  4 files changed, 22 insertions(+)
> > > > > > >
> > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > > > > > index 80ff248c2..b20e9f3db 100644
> > > > > > > --- a/src/android/camera_device.cpp
> > > > > > > +++ b/src/android/camera_device.cpp
> > > > > > > @@ -377,6 +377,7 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData)
> > > > > > >                 orientation_ = 0;
> > > > > > >         }
> > > > > > >
> > > > > > > +       mirrored_ = cameraConfigData && cameraConfigData->mirrored;
> > > > > > >         return capabilities_.initialize(camera_, orientation_, facing_);
> > > > > > >  }
> > > > > > >
> > > > > > > @@ -547,6 +548,11 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> > > > > > >                 return -EINVAL;
> > > > > > >         }
> > > > > > >
> > > > > > > +       // Rotation is unsupported, so if mirrored just set orientation.
> > > > > >
> > > > > > Is that true? I thought we usually support 0 and 180 rotations on most
> > > > > > sensors at least ?
> > > > >
> > > > > Ah I misunderstood the below comments/errors about not supporting stream
> > > > > rotations. It seems the sensors on my test device don't support rotation. I will
> > > > > send out a v2 with this comment removed assuming the rest of this patch
> > > > > looks good.
> > > > > >
> > > > > > We (almost) always use /*  */ comments in libcamera ... it's ... probably a
> > > > > > stylistic choice because we are mostly started out as C / Linux-kernel
> > > > > > devs.
> > > > > >
> > > > > >
> > > > > >
> > > > > > > +       if (mirrored_) {
> > > > > > > +               config->orientation = Orientation::Rotate0Mirror;
> > > > > > > +       }
> > > > > > > +
> > >
> > > Also no {} for single line statement.
> > >
> > > It's amusing how many times the same stylistic comments had to
> > > repeated specifically to chromeos developers. We also have tools
> > > available for checking this kind of issues before submitting.
> > >
> >
> > Apologies, I ran checkstyle.py on the change but found no issues, if there
> > are other tools for finding style issues I'll be sure to run them in the future.
> > I'll update this one as well in v2.
>
> This is one of your first contributions, so it has been unfair ranting
> to you about the past :)
>
> I'm just suprised a team of specialized and highly skilled engineers
> continue pushing their conventions (out of habit I presume) to a different
> code base. My frustration comes from having repeated the same comments
> over and over in the past years. Apologies if you got ranted to.
>

Sorry to have added to the frustration, and no worries :)
> > > > > >
> > > > > > How does this interact with rotation as well ? will it take precedence
> > > > > > against 180 rotations?
> > > > >
> > > > > I can't confirm because my sensors aren't affected by the rotation config
> > > > > but as far as I can tell the rotation would be unaffected by setting the
> > > > > orientation to mirror. There doesn't seem to be anywhere setting the
> > > > > orientation in the Android layer, so any rotation would happen independently.
> > > > > And the mirroring will just set the HFLIP on the kernel driver, so any higher
> > > > > level rotation would just rotate the flipped output as expected.
> > > >
> > > > Some level of rotation is supposed to be handled automatically by
> > > > setting the physical orientation and rotation in device tree.
> > > >
> > > > Then libcamera will 'aim' for a 0 rotation by default.
> > > >
> > > > I guess the issue here is that we don't have a way in
> > > > device-tree/firmware to mark that the camera is mirrored.
> > > >
> > > > what does this actually 'mean' by the way? How is the camera physically
> > > > mirrored? Is this really a bug in the driver somewhere or something
> > > > else?
> > > >
> > > > I am weary that this patch is possibly a workaround for a problem
> > > > somewhere else ?
> > >
> > > If my recollection is right, when it comes to the camera HAL:
> > >
> > > 1) Rotation is first parsed from DT, but there we can only express
> > >    [0, 90, 270, 360] so we don't have a "mirror" option
> > >
> > > 2) If no "rotation" property is specified in DT, then the HAL
> > >    falls-back  to parse a "rotation" property from configuration file,
> > >    where again we can only express an angle between [0 - 360]
> > >
> > > All of this seems to suggest it is not possible to express in DT or
> > > HAL configuration file the "mirroring" option. However this opens the
> > > question that Kieran has asked already. How is the camera module
> > > physically mirrored ? is the pixel array reading direction inverted in
> > > the driver ?
> > >
> >
> > I'm not 100% sure where the mirroring is coming from. I confirmed it
> > through qcam on Ubuntu as well as through the camera app and Chromium
> > on a ChromiumOs build so it's not just an application bug. The location and
> > orientation coming from ACPI both seem to be correct. The driver seems
>
> The 'rotation' property from DTS cannot express 'mirrored', and to me
> this seems correct, as 'rotation' is about expressing the mounting
> rotation of the camera sensor (0, 90, 180, 270)
>
> Mirroring comes from inverting the reading directions of the pixel
> units on the pixel array, what is usually called an HFLIP in v4l2
> drivers and we require drivers to not apply any V/HFLIP by default
> (something we might want to document in libcamera as well, not just in
> Linux).
>
> > to be fine on all but some Surface Go devices so it seems unrelated.
>
> What do you mean with "seems fine" ? It doesn't mirror ?
>

"Seems fine" as in I don't have a device to test and confirm that it actually
works as expected and isn't mirrored, but the only references I find to
mirrored output from the sensor are for the Surface devices, and I would
expect that to be a prominent enough issue that it would be raised
somewhere.
> > Anything below that is infeasible to fix and the sensor drivers don't
> > maintain quirks for particular misbehaving devices, so the HAL seems
> > to me the best place to add a mirrored config and pipe it down to the
> > driver through the existing Orientation.
>
> Which sensor are we talking about and where does its driver live ? Can
> we see it ? Because some downstream (but also upstream) drivers
> sometimes flip by default.
>
This is the ov8865 sensor, driver is drivers/media/i2c/ov8865.c in the
upstream kernel. I confirmedthat the default state is unflipped, and
actually have a separate patch out (https://lkml.org/lkml/2025/7/22/1443)
because there's a bug in the driver which ends up resetting the flip
states back to default after we set it.

Thanks,
Allen
>
> >
> > Thanks,
> > Allen
> > > >
> > > > --
> > > > Kieran
> > > >
> > > > > >
> > > > > > >         /*
> > > > > > >          * Clear and remove any existing configuration from previous calls, and
> > > > > > >          * ensure the required entries are available without further
> > > > > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > > > > > > index 194ca3030..d304a1285 100644
> > > > > > > --- a/src/android/camera_device.h
> > > > > > > +++ b/src/android/camera_device.h
> > > > > > > @@ -126,6 +126,7 @@ private:
> > > > > > >
> > > > > > >         int facing_;
> > > > > > >         int orientation_;
> > > > > > > +       int mirrored_;
> > > > > > >
> > > > > > >         CameraMetadata lastSettings_;
> > > > > > >  };
> > > > > > > diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp
> > > > > > > index 7ef451ef8..a2beba5d3 100644
> > > > > > > --- a/src/android/camera_hal_config.cpp
> > > > > > > +++ b/src/android/camera_hal_config.cpp
> > > > > > > @@ -33,6 +33,7 @@ private:
> > > > > > >         int parseCameraConfigData(const std::string &cameraId, const YamlObject &);
> > > > > > >         int parseLocation(const YamlObject &, CameraConfigData &cameraConfigData);
> > > > > > >         int parseRotation(const YamlObject &, CameraConfigData &cameraConfigData);
> > > > > > > +       int parseMirrored(const YamlObject &, CameraConfigData &cameraConfigData);
> > > > > > >
> > > > > > >         std::map<std::string, CameraConfigData> *cameras_;
> > > > > > >  };
> > > > > > > @@ -106,6 +107,9 @@ int CameraHalConfig::Private::parseCameraConfigData(const std::string &cameraId,
> > > > > > >         if (parseRotation(cameraObject, cameraConfigData))
> > > > > > >                 return -EINVAL;
> > > > > > >
> > > > > > > +       /* Parse optional property "mirrored" */
> > > > > > > +       parseMirrored(cameraObject, cameraConfigData);
> > > > > > > +
> > > > > > >         return 0;
> > > > > > >  }
> > > > > > >
> > > > > > > @@ -145,6 +149,16 @@ int CameraHalConfig::Private::parseRotation(const YamlObject &cameraObject,
> > > > > > >         return 0;
> > > > > > >  }
> > > > > > >
> > > > > > > +int CameraHalConfig::Private::parseMirrored(const YamlObject &cameraObject,
> > > > > > > +                                           CameraConfigData &cameraConfigData)
> > > > > > > +{
> > > > > > > +       if (!cameraObject.contains("mirrored"))
> > > > > > > +               return -EINVAL;
> > > > > > > +
> > > > > > > +       cameraConfigData.mirrored = cameraObject["mirrored"].get<bool>(false);
> > > > > > > +       return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > >  CameraHalConfig::CameraHalConfig()
> > > > > > >         : Extensible(std::make_unique<Private>()), exists_(false), valid_(false)
> > > > > > >  {
> > > > > > > diff --git a/src/android/camera_hal_config.h b/src/android/camera_hal_config.h
> > > > > > > index a4bedb6e6..a96190470 100644
> > > > > > > --- a/src/android/camera_hal_config.h
> > > > > > > +++ b/src/android/camera_hal_config.h
> > > > > > > @@ -15,6 +15,7 @@
> > > > > > >  struct CameraConfigData {
> > > > > > >         int facing = -1;
> > > > > > >         int rotation = -1;
> > > > > > > +       bool mirrored = false;
> > > > > > >  };
> > > > > > >
> > > > > > >  class CameraHalConfig final : public libcamera::Extensible
> > > > > > > --
> > > > > > > 2.50.0.727.gbf7dc18ff4-goog
> > > > > > >

On Wed, Jul 23, 2025 at 08:14:45AM -0700, Allen Ballway wrote:
> On Wed, Jul 23, 2025 at 1:15 AM Jacopo Mondi wrote:
> > On Tue, Jul 22, 2025 at 07:51:53AM -0700, Allen Ballway wrote:
> > > On Mon, Jul 21, 2025 at 4:14 AM Jacopo Mondi wrote:
> > > > On Mon, Jul 21, 2025 at 11:44:18AM +0100, Kieran Bingham wrote:
> > > > > Quoting Allen Ballway (2025-07-18 16:27:23)
> > > > > > On Fri, Jul 18, 2025 at 5:11 AM Kieran Bingham wrote:
> > > > > > > Quoting Allen Ballway (2025-07-16 21:46:20)
> > > > > > > > Adds an optional `mirrored` value to the HAL config, which is used to modify
> > > > > > > > the `orientation` of the camera config. No rotation is used, so it can
> > > > > > > > only set the orientation to `Orientation::Rotate0Mirror`.
> > > > > > > >
> > > > > > > > This enables sensors which are incorrectly mirrored to be corrected.
> > > > > > > >
> > > > > > > > Signed-off-by: Allen Ballway <ballway@chromium.org>
> > > > > > > > ---
> > > > > > > >  src/android/camera_device.cpp     |  6 ++++++
> > > > > > > >  src/android/camera_device.h       |  1 +
> > > > > > > >  src/android/camera_hal_config.cpp | 14 ++++++++++++++
> > > > > > > >  src/android/camera_hal_config.h   |  1 +
> > > > > > > >  4 files changed, 22 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > > > > > > index 80ff248c2..b20e9f3db 100644
> > > > > > > > --- a/src/android/camera_device.cpp
> > > > > > > > +++ b/src/android/camera_device.cpp
> > > > > > > > @@ -377,6 +377,7 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData)
> > > > > > > >                 orientation_ = 0;
> > > > > > > >         }
> > > > > > > >
> > > > > > > > +       mirrored_ = cameraConfigData && cameraConfigData->mirrored;
> > > > > > > >         return capabilities_.initialize(camera_, orientation_, facing_);
> > > > > > > >  }
> > > > > > > >
> > > > > > > > @@ -547,6 +548,11 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> > > > > > > >                 return -EINVAL;
> > > > > > > >         }
> > > > > > > >
> > > > > > > > +       // Rotation is unsupported, so if mirrored just set orientation.
> > > > > > >
> > > > > > > Is that true? I thought we usually support 0 and 180 rotations on most
> > > > > > > sensors at least ?
> > > > > >
> > > > > > Ah I misunderstood the below comments/errors about not supporting stream
> > > > > > rotations. It seems the sensors on my test device don't support rotation. I will
> > > > > > send out a v2 with this comment removed assuming the rest of this patch
> > > > > > looks good.
> > > > > > >
> > > > > > > We (almost) always use /*  */ comments in libcamera ... it's ... probably a
> > > > > > > stylistic choice because we are mostly started out as C / Linux-kernel
> > > > > > > devs.
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > > +       if (mirrored_) {
> > > > > > > > +               config->orientation = Orientation::Rotate0Mirror;
> > > > > > > > +       }
> > > > > > > > +
> > > >
> > > > Also no {} for single line statement.
> > > >
> > > > It's amusing how many times the same stylistic comments had to
> > > > repeated specifically to chromeos developers. We also have tools
> > > > available for checking this kind of issues before submitting.
> > > >
> > >
> > > Apologies, I ran checkstyle.py on the change but found no issues, if there
> > > are other tools for finding style issues I'll be sure to run them in the future.
> > > I'll update this one as well in v2.
> >
> > This is one of your first contributions, so it has been unfair ranting
> > to you about the past :)
> >
> > I'm just suprised a team of specialized and highly skilled engineers
> > continue pushing their conventions (out of habit I presume) to a different
> > code base. My frustration comes from having repeated the same comments
> > over and over in the past years. Apologies if you got ranted to.
> 
> Sorry to have added to the frustration, and no worries :)
> 
> > > > > > >
> > > > > > > How does this interact with rotation as well ? will it take precedence
> > > > > > > against 180 rotations?
> > > > > >
> > > > > > I can't confirm because my sensors aren't affected by the rotation config
> > > > > > but as far as I can tell the rotation would be unaffected by setting the
> > > > > > orientation to mirror. There doesn't seem to be anywhere setting the
> > > > > > orientation in the Android layer, so any rotation would happen independently.
> > > > > > And the mirroring will just set the HFLIP on the kernel driver, so any higher
> > > > > > level rotation would just rotate the flipped output as expected.
> > > > >
> > > > > Some level of rotation is supposed to be handled automatically by
> > > > > setting the physical orientation and rotation in device tree.
> > > > >
> > > > > Then libcamera will 'aim' for a 0 rotation by default.
> > > > >
> > > > > I guess the issue here is that we don't have a way in
> > > > > device-tree/firmware to mark that the camera is mirrored.
> > > > >
> > > > > what does this actually 'mean' by the way? How is the camera physically
> > > > > mirrored? Is this really a bug in the driver somewhere or something
> > > > > else?
> > > > >
> > > > > I am weary that this patch is possibly a workaround for a problem
> > > > > somewhere else ?
> > > >
> > > > If my recollection is right, when it comes to the camera HAL:
> > > >
> > > > 1) Rotation is first parsed from DT, but there we can only express
> > > >    [0, 90, 270, 360] so we don't have a "mirror" option
> > > >
> > > > 2) If no "rotation" property is specified in DT, then the HAL
> > > >    falls-back  to parse a "rotation" property from configuration file,
> > > >    where again we can only express an angle between [0 - 360]
> > > >
> > > > All of this seems to suggest it is not possible to express in DT or
> > > > HAL configuration file the "mirroring" option. However this opens the
> > > > question that Kieran has asked already. How is the camera module
> > > > physically mirrored ? is the pixel array reading direction inverted in
> > > > the driver ?
> > >
> > > I'm not 100% sure where the mirroring is coming from. I confirmed it
> > > through qcam on Ubuntu as well as through the camera app and Chromium
> > > on a ChromiumOs build so it's not just an application bug. The location and
> > > orientation coming from ACPI both seem to be correct. The driver seems
> >
> > The 'rotation' property from DTS cannot express 'mirrored', and to me
> > this seems correct, as 'rotation' is about expressing the mounting
> > rotation of the camera sensor (0, 90, 180, 270)
> >
> > Mirroring comes from inverting the reading directions of the pixel
> > units on the pixel array, what is usually called an HFLIP in v4l2
> > drivers and we require drivers to not apply any V/HFLIP by default
> > (something we might want to document in libcamera as well, not just in
> > Linux).
> >
> > > to be fine on all but some Surface Go devices so it seems unrelated.
> >
> > What do you mean with "seems fine" ? It doesn't mirror ?
> 
> "Seems fine" as in I don't have a device to test and confirm that it actually
> works as expected and isn't mirrored, but the only references I find to
> mirrored output from the sensor are for the Surface devices, and I would
> expect that to be a prominent enough issue that it would be raised
> somewhere.

Do you mean that all devices you know about that integrate an OV8865
display mirrored images, or is there a mix of devices with the same
sensor that display mirrored and non-mirrored images ?

> > > Anything below that is infeasible to fix and the sensor drivers don't
> > > maintain quirks for particular misbehaving devices, so the HAL seems
> > > to me the best place to add a mirrored config and pipe it down to the
> > > driver through the existing Orientation.
> >
> > Which sensor are we talking about and where does its driver live ? Can
> > we see it ? Because some downstream (but also upstream) drivers
> > sometimes flip by default.
> 
> This is the ov8865 sensor, driver is drivers/media/i2c/ov8865.c in the
> upstream kernel. I confirmedthat the default state is unflipped, and
> actually have a separate patch out (https://lkml.org/lkml/2025/7/22/1443)
> because there's a bug in the driver which ends up resetting the flip
> states back to default after we set it.
> 
> > > > > > > >         /*
> > > > > > > >          * Clear and remove any existing configuration from previous calls, and
> > > > > > > >          * ensure the required entries are available without further
> > > > > > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > > > > > > > index 194ca3030..d304a1285 100644
> > > > > > > > --- a/src/android/camera_device.h
> > > > > > > > +++ b/src/android/camera_device.h
> > > > > > > > @@ -126,6 +126,7 @@ private:
> > > > > > > >
> > > > > > > >         int facing_;
> > > > > > > >         int orientation_;
> > > > > > > > +       int mirrored_;
> > > > > > > >
> > > > > > > >         CameraMetadata lastSettings_;
> > > > > > > >  };
> > > > > > > > diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp
> > > > > > > > index 7ef451ef8..a2beba5d3 100644
> > > > > > > > --- a/src/android/camera_hal_config.cpp
> > > > > > > > +++ b/src/android/camera_hal_config.cpp
> > > > > > > > @@ -33,6 +33,7 @@ private:
> > > > > > > >         int parseCameraConfigData(const std::string &cameraId, const YamlObject &);
> > > > > > > >         int parseLocation(const YamlObject &, CameraConfigData &cameraConfigData);
> > > > > > > >         int parseRotation(const YamlObject &, CameraConfigData &cameraConfigData);
> > > > > > > > +       int parseMirrored(const YamlObject &, CameraConfigData &cameraConfigData);
> > > > > > > >
> > > > > > > >         std::map<std::string, CameraConfigData> *cameras_;
> > > > > > > >  };
> > > > > > > > @@ -106,6 +107,9 @@ int CameraHalConfig::Private::parseCameraConfigData(const std::string &cameraId,
> > > > > > > >         if (parseRotation(cameraObject, cameraConfigData))
> > > > > > > >                 return -EINVAL;
> > > > > > > >
> > > > > > > > +       /* Parse optional property "mirrored" */
> > > > > > > > +       parseMirrored(cameraObject, cameraConfigData);
> > > > > > > > +
> > > > > > > >         return 0;
> > > > > > > >  }
> > > > > > > >
> > > > > > > > @@ -145,6 +149,16 @@ int CameraHalConfig::Private::parseRotation(const YamlObject &cameraObject,
> > > > > > > >         return 0;
> > > > > > > >  }
> > > > > > > >
> > > > > > > > +int CameraHalConfig::Private::parseMirrored(const YamlObject &cameraObject,
> > > > > > > > +                                           CameraConfigData &cameraConfigData)
> > > > > > > > +{
> > > > > > > > +       if (!cameraObject.contains("mirrored"))
> > > > > > > > +               return -EINVAL;
> > > > > > > > +
> > > > > > > > +       cameraConfigData.mirrored = cameraObject["mirrored"].get<bool>(false);
> > > > > > > > +       return 0;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > >  CameraHalConfig::CameraHalConfig()
> > > > > > > >         : Extensible(std::make_unique<Private>()), exists_(false), valid_(false)
> > > > > > > >  {
> > > > > > > > diff --git a/src/android/camera_hal_config.h b/src/android/camera_hal_config.h
> > > > > > > > index a4bedb6e6..a96190470 100644
> > > > > > > > --- a/src/android/camera_hal_config.h
> > > > > > > > +++ b/src/android/camera_hal_config.h
> > > > > > > > @@ -15,6 +15,7 @@
> > > > > > > >  struct CameraConfigData {
> > > > > > > >         int facing = -1;
> > > > > > > >         int rotation = -1;
> > > > > > > > +       bool mirrored = false;
> > > > > > > >  };
> > > > > > > >
> > > > > > > >  class CameraHalConfig final : public libcamera::Extensible
> > > > > > > > --
> > > > > > > > 2.50.0.727.gbf7dc18ff4-goog
> > > > > > > >

On Wed, Jul 23, 2025 at 8:43 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Wed, Jul 23, 2025 at 08:14:45AM -0700, Allen Ballway wrote:
> > On Wed, Jul 23, 2025 at 1:15 AM Jacopo Mondi wrote:
> > > On Tue, Jul 22, 2025 at 07:51:53AM -0700, Allen Ballway wrote:
> > > > On Mon, Jul 21, 2025 at 4:14 AM Jacopo Mondi wrote:
> > > > > On Mon, Jul 21, 2025 at 11:44:18AM +0100, Kieran Bingham wrote:
> > > > > > Quoting Allen Ballway (2025-07-18 16:27:23)
> > > > > > > On Fri, Jul 18, 2025 at 5:11 AM Kieran Bingham wrote:
> > > > > > > > Quoting Allen Ballway (2025-07-16 21:46:20)
> > > > > > > > > Adds an optional `mirrored` value to the HAL config, which is used to modify
> > > > > > > > > the `orientation` of the camera config. No rotation is used, so it can
> > > > > > > > > only set the orientation to `Orientation::Rotate0Mirror`.
> > > > > > > > >
> > > > > > > > > This enables sensors which are incorrectly mirrored to be corrected.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Allen Ballway <ballway@chromium.org>
> > > > > > > > > ---
> > > > > > > > >  src/android/camera_device.cpp     |  6 ++++++
> > > > > > > > >  src/android/camera_device.h       |  1 +
> > > > > > > > >  src/android/camera_hal_config.cpp | 14 ++++++++++++++
> > > > > > > > >  src/android/camera_hal_config.h   |  1 +
> > > > > > > > >  4 files changed, 22 insertions(+)
> > > > > > > > >
> > > > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > > > > > > > index 80ff248c2..b20e9f3db 100644
> > > > > > > > > --- a/src/android/camera_device.cpp
> > > > > > > > > +++ b/src/android/camera_device.cpp
> > > > > > > > > @@ -377,6 +377,7 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData)
> > > > > > > > >                 orientation_ = 0;
> > > > > > > > >         }
> > > > > > > > >
> > > > > > > > > +       mirrored_ = cameraConfigData && cameraConfigData->mirrored;
> > > > > > > > >         return capabilities_.initialize(camera_, orientation_, facing_);
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > > @@ -547,6 +548,11 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> > > > > > > > >                 return -EINVAL;
> > > > > > > > >         }
> > > > > > > > >
> > > > > > > > > +       // Rotation is unsupported, so if mirrored just set orientation.
> > > > > > > >
> > > > > > > > Is that true? I thought we usually support 0 and 180 rotations on most
> > > > > > > > sensors at least ?
> > > > > > >
> > > > > > > Ah I misunderstood the below comments/errors about not supporting stream
> > > > > > > rotations. It seems the sensors on my test device don't support rotation. I will
> > > > > > > send out a v2 with this comment removed assuming the rest of this patch
> > > > > > > looks good.
> > > > > > > >
> > > > > > > > We (almost) always use /*  */ comments in libcamera ... it's ... probably a
> > > > > > > > stylistic choice because we are mostly started out as C / Linux-kernel
> > > > > > > > devs.
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > > +       if (mirrored_) {
> > > > > > > > > +               config->orientation = Orientation::Rotate0Mirror;
> > > > > > > > > +       }
> > > > > > > > > +
> > > > >
> > > > > Also no {} for single line statement.
> > > > >
> > > > > It's amusing how many times the same stylistic comments had to
> > > > > repeated specifically to chromeos developers. We also have tools
> > > > > available for checking this kind of issues before submitting.
> > > > >
> > > >
> > > > Apologies, I ran checkstyle.py on the change but found no issues, if there
> > > > are other tools for finding style issues I'll be sure to run them in the future.
> > > > I'll update this one as well in v2.
> > >
> > > This is one of your first contributions, so it has been unfair ranting
> > > to you about the past :)
> > >
> > > I'm just suprised a team of specialized and highly skilled engineers
> > > continue pushing their conventions (out of habit I presume) to a different
> > > code base. My frustration comes from having repeated the same comments
> > > over and over in the past years. Apologies if you got ranted to.
> >
> > Sorry to have added to the frustration, and no worries :)
> >
> > > > > > > >
> > > > > > > > How does this interact with rotation as well ? will it take precedence
> > > > > > > > against 180 rotations?
> > > > > > >
> > > > > > > I can't confirm because my sensors aren't affected by the rotation config
> > > > > > > but as far as I can tell the rotation would be unaffected by setting the
> > > > > > > orientation to mirror. There doesn't seem to be anywhere setting the
> > > > > > > orientation in the Android layer, so any rotation would happen independently.
> > > > > > > And the mirroring will just set the HFLIP on the kernel driver, so any higher
> > > > > > > level rotation would just rotate the flipped output as expected.
> > > > > >
> > > > > > Some level of rotation is supposed to be handled automatically by
> > > > > > setting the physical orientation and rotation in device tree.
> > > > > >
> > > > > > Then libcamera will 'aim' for a 0 rotation by default.
> > > > > >
> > > > > > I guess the issue here is that we don't have a way in
> > > > > > device-tree/firmware to mark that the camera is mirrored.
> > > > > >
> > > > > > what does this actually 'mean' by the way? How is the camera physically
> > > > > > mirrored? Is this really a bug in the driver somewhere or something
> > > > > > else?
> > > > > >
> > > > > > I am weary that this patch is possibly a workaround for a problem
> > > > > > somewhere else ?
> > > > >
> > > > > If my recollection is right, when it comes to the camera HAL:
> > > > >
> > > > > 1) Rotation is first parsed from DT, but there we can only express
> > > > >    [0, 90, 270, 360] so we don't have a "mirror" option
> > > > >
> > > > > 2) If no "rotation" property is specified in DT, then the HAL
> > > > >    falls-back  to parse a "rotation" property from configuration file,
> > > > >    where again we can only express an angle between [0 - 360]
> > > > >
> > > > > All of this seems to suggest it is not possible to express in DT or
> > > > > HAL configuration file the "mirroring" option. However this opens the
> > > > > question that Kieran has asked already. How is the camera module
> > > > > physically mirrored ? is the pixel array reading direction inverted in
> > > > > the driver ?
> > > >
> > > > I'm not 100% sure where the mirroring is coming from. I confirmed it
> > > > through qcam on Ubuntu as well as through the camera app and Chromium
> > > > on a ChromiumOs build so it's not just an application bug. The location and
> > > > orientation coming from ACPI both seem to be correct. The driver seems
> > >
> > > The 'rotation' property from DTS cannot express 'mirrored', and to me
> > > this seems correct, as 'rotation' is about expressing the mounting
> > > rotation of the camera sensor (0, 90, 180, 270)
> > >
> > > Mirroring comes from inverting the reading directions of the pixel
> > > units on the pixel array, what is usually called an HFLIP in v4l2
> > > drivers and we require drivers to not apply any V/HFLIP by default
> > > (something we might want to document in libcamera as well, not just in
> > > Linux).
> > >
> > > > to be fine on all but some Surface Go devices so it seems unrelated.
> > >
> > > What do you mean with "seems fine" ? It doesn't mirror ?
> >
> > "Seems fine" as in I don't have a device to test and confirm that it actually
> > works as expected and isn't mirrored, but the only references I find to
> > mirrored output from the sensor are for the Surface devices, and I would
> > expect that to be a prominent enough issue that it would be raised
> > somewhere.
>
> Do you mean that all devices you know about that integrate an OV8865
> display mirrored images, or is there a mix of devices with the same
> sensor that display mirrored and non-mirrored images ?
>
I have only been able to test on Surface Go devices with this sensor, and
all of them have been mirrored. I'm unsure what other devices use this
sensor, but from a lack of complaints or issue reports around mirroring
make me assume that they receive non-mirrored images.
> > > > Anything below that is infeasible to fix and the sensor drivers don't
> > > > maintain quirks for particular misbehaving devices, so the HAL seems
> > > > to me the best place to add a mirrored config and pipe it down to the
> > > > driver through the existing Orientation.
> > >
> > > Which sensor are we talking about and where does its driver live ? Can
> > > we see it ? Because some downstream (but also upstream) drivers
> > > sometimes flip by default.
> >
> > This is the ov8865 sensor, driver is drivers/media/i2c/ov8865.c in the
> > upstream kernel. I confirmedthat the default state is unflipped, and
> > actually have a separate patch out (https://lkml.org/lkml/2025/7/22/1443)
> > because there's a bug in the driver which ends up resetting the flip
> > states back to default after we set it.
> >
> > > > > > > > >         /*
> > > > > > > > >          * Clear and remove any existing configuration from previous calls, and
> > > > > > > > >          * ensure the required entries are available without further
> > > > > > > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > > > > > > > > index 194ca3030..d304a1285 100644
> > > > > > > > > --- a/src/android/camera_device.h
> > > > > > > > > +++ b/src/android/camera_device.h
> > > > > > > > > @@ -126,6 +126,7 @@ private:
> > > > > > > > >
> > > > > > > > >         int facing_;
> > > > > > > > >         int orientation_;
> > > > > > > > > +       int mirrored_;
> > > > > > > > >
> > > > > > > > >         CameraMetadata lastSettings_;
> > > > > > > > >  };
> > > > > > > > > diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp
> > > > > > > > > index 7ef451ef8..a2beba5d3 100644
> > > > > > > > > --- a/src/android/camera_hal_config.cpp
> > > > > > > > > +++ b/src/android/camera_hal_config.cpp
> > > > > > > > > @@ -33,6 +33,7 @@ private:
> > > > > > > > >         int parseCameraConfigData(const std::string &cameraId, const YamlObject &);
> > > > > > > > >         int parseLocation(const YamlObject &, CameraConfigData &cameraConfigData);
> > > > > > > > >         int parseRotation(const YamlObject &, CameraConfigData &cameraConfigData);
> > > > > > > > > +       int parseMirrored(const YamlObject &, CameraConfigData &cameraConfigData);
> > > > > > > > >
> > > > > > > > >         std::map<std::string, CameraConfigData> *cameras_;
> > > > > > > > >  };
> > > > > > > > > @@ -106,6 +107,9 @@ int CameraHalConfig::Private::parseCameraConfigData(const std::string &cameraId,
> > > > > > > > >         if (parseRotation(cameraObject, cameraConfigData))
> > > > > > > > >                 return -EINVAL;
> > > > > > > > >
> > > > > > > > > +       /* Parse optional property "mirrored" */
> > > > > > > > > +       parseMirrored(cameraObject, cameraConfigData);
> > > > > > > > > +
> > > > > > > > >         return 0;
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > > @@ -145,6 +149,16 @@ int CameraHalConfig::Private::parseRotation(const YamlObject &cameraObject,
> > > > > > > > >         return 0;
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > > +int CameraHalConfig::Private::parseMirrored(const YamlObject &cameraObject,
> > > > > > > > > +                                           CameraConfigData &cameraConfigData)
> > > > > > > > > +{
> > > > > > > > > +       if (!cameraObject.contains("mirrored"))
> > > > > > > > > +               return -EINVAL;
> > > > > > > > > +
> > > > > > > > > +       cameraConfigData.mirrored = cameraObject["mirrored"].get<bool>(false);
> > > > > > > > > +       return 0;
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > >  CameraHalConfig::CameraHalConfig()
> > > > > > > > >         : Extensible(std::make_unique<Private>()), exists_(false), valid_(false)
> > > > > > > > >  {
> > > > > > > > > diff --git a/src/android/camera_hal_config.h b/src/android/camera_hal_config.h
> > > > > > > > > index a4bedb6e6..a96190470 100644
> > > > > > > > > --- a/src/android/camera_hal_config.h
> > > > > > > > > +++ b/src/android/camera_hal_config.h
> > > > > > > > > @@ -15,6 +15,7 @@
> > > > > > > > >  struct CameraConfigData {
> > > > > > > > >         int facing = -1;
> > > > > > > > >         int rotation = -1;
> > > > > > > > > +       bool mirrored = false;
> > > > > > > > >  };
> > > > > > > > >
> > > > > > > > >  class CameraHalConfig final : public libcamera::Extensible
> > > > > > > > > --
> > > > > > > > > 2.50.0.727.gbf7dc18ff4-goog
> > > > > > > > >
>
> --
> Regards,
>
> Laurent Pinchart

On Wed, Jul 23, 2025 at 08:54:20AM -0700, Allen Ballway wrote:
> On Wed, Jul 23, 2025 at 8:43 AM Laurent Pinchart wrote:
> > On Wed, Jul 23, 2025 at 08:14:45AM -0700, Allen Ballway wrote:
> > > On Wed, Jul 23, 2025 at 1:15 AM Jacopo Mondi wrote:
> > > > On Tue, Jul 22, 2025 at 07:51:53AM -0700, Allen Ballway wrote:
> > > > > On Mon, Jul 21, 2025 at 4:14 AM Jacopo Mondi wrote:
> > > > > > On Mon, Jul 21, 2025 at 11:44:18AM +0100, Kieran Bingham wrote:
> > > > > > > Quoting Allen Ballway (2025-07-18 16:27:23)
> > > > > > > > On Fri, Jul 18, 2025 at 5:11 AM Kieran Bingham wrote:
> > > > > > > > > Quoting Allen Ballway (2025-07-16 21:46:20)
> > > > > > > > > > Adds an optional `mirrored` value to the HAL config, which is used to modify
> > > > > > > > > > the `orientation` of the camera config. No rotation is used, so it can
> > > > > > > > > > only set the orientation to `Orientation::Rotate0Mirror`.
> > > > > > > > > >
> > > > > > > > > > This enables sensors which are incorrectly mirrored to be corrected.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Allen Ballway <ballway@chromium.org>
> > > > > > > > > > ---
> > > > > > > > > >  src/android/camera_device.cpp     |  6 ++++++
> > > > > > > > > >  src/android/camera_device.h       |  1 +
> > > > > > > > > >  src/android/camera_hal_config.cpp | 14 ++++++++++++++
> > > > > > > > > >  src/android/camera_hal_config.h   |  1 +
> > > > > > > > > >  4 files changed, 22 insertions(+)
> > > > > > > > > >
> > > > > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > > > > > > > > index 80ff248c2..b20e9f3db 100644
> > > > > > > > > > --- a/src/android/camera_device.cpp
> > > > > > > > > > +++ b/src/android/camera_device.cpp
> > > > > > > > > > @@ -377,6 +377,7 @@ int CameraDevice::initialize(const CameraConfigData *cameraConfigData)
> > > > > > > > > >                 orientation_ = 0;
> > > > > > > > > >         }
> > > > > > > > > >
> > > > > > > > > > +       mirrored_ = cameraConfigData && cameraConfigData->mirrored;
> > > > > > > > > >         return capabilities_.initialize(camera_, orientation_, facing_);
> > > > > > > > > >  }
> > > > > > > > > >
> > > > > > > > > > @@ -547,6 +548,11 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> > > > > > > > > >                 return -EINVAL;
> > > > > > > > > >         }
> > > > > > > > > >
> > > > > > > > > > +       // Rotation is unsupported, so if mirrored just set orientation.
> > > > > > > > >
> > > > > > > > > Is that true? I thought we usually support 0 and 180 rotations on most
> > > > > > > > > sensors at least ?
> > > > > > > >
> > > > > > > > Ah I misunderstood the below comments/errors about not supporting stream
> > > > > > > > rotations. It seems the sensors on my test device don't support rotation. I will
> > > > > > > > send out a v2 with this comment removed assuming the rest of this patch
> > > > > > > > looks good.
> > > > > > > > >
> > > > > > > > > We (almost) always use /*  */ comments in libcamera ... it's ... probably a
> > > > > > > > > stylistic choice because we are mostly started out as C / Linux-kernel
> > > > > > > > > devs.
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > > +       if (mirrored_) {
> > > > > > > > > > +               config->orientation = Orientation::Rotate0Mirror;
> > > > > > > > > > +       }
> > > > > > > > > > +
> > > > > >
> > > > > > Also no {} for single line statement.
> > > > > >
> > > > > > It's amusing how many times the same stylistic comments had to
> > > > > > repeated specifically to chromeos developers. We also have tools
> > > > > > available for checking this kind of issues before submitting.
> > > > > >
> > > > >
> > > > > Apologies, I ran checkstyle.py on the change but found no issues, if there
> > > > > are other tools for finding style issues I'll be sure to run them in the future.
> > > > > I'll update this one as well in v2.
> > > >
> > > > This is one of your first contributions, so it has been unfair ranting
> > > > to you about the past :)
> > > >
> > > > I'm just suprised a team of specialized and highly skilled engineers
> > > > continue pushing their conventions (out of habit I presume) to a different
> > > > code base. My frustration comes from having repeated the same comments
> > > > over and over in the past years. Apologies if you got ranted to.
> > >
> > > Sorry to have added to the frustration, and no worries :)
> > >
> > > > > > > > >
> > > > > > > > > How does this interact with rotation as well ? will it take precedence
> > > > > > > > > against 180 rotations?
> > > > > > > >
> > > > > > > > I can't confirm because my sensors aren't affected by the rotation config
> > > > > > > > but as far as I can tell the rotation would be unaffected by setting the
> > > > > > > > orientation to mirror. There doesn't seem to be anywhere setting the
> > > > > > > > orientation in the Android layer, so any rotation would happen independently.
> > > > > > > > And the mirroring will just set the HFLIP on the kernel driver, so any higher
> > > > > > > > level rotation would just rotate the flipped output as expected.
> > > > > > >
> > > > > > > Some level of rotation is supposed to be handled automatically by
> > > > > > > setting the physical orientation and rotation in device tree.
> > > > > > >
> > > > > > > Then libcamera will 'aim' for a 0 rotation by default.
> > > > > > >
> > > > > > > I guess the issue here is that we don't have a way in
> > > > > > > device-tree/firmware to mark that the camera is mirrored.
> > > > > > >
> > > > > > > what does this actually 'mean' by the way? How is the camera physically
> > > > > > > mirrored? Is this really a bug in the driver somewhere or something
> > > > > > > else?
> > > > > > >
> > > > > > > I am weary that this patch is possibly a workaround for a problem
> > > > > > > somewhere else ?
> > > > > >
> > > > > > If my recollection is right, when it comes to the camera HAL:
> > > > > >
> > > > > > 1) Rotation is first parsed from DT, but there we can only express
> > > > > >    [0, 90, 270, 360] so we don't have a "mirror" option
> > > > > >
> > > > > > 2) If no "rotation" property is specified in DT, then the HAL
> > > > > >    falls-back  to parse a "rotation" property from configuration file,
> > > > > >    where again we can only express an angle between [0 - 360]
> > > > > >
> > > > > > All of this seems to suggest it is not possible to express in DT or
> > > > > > HAL configuration file the "mirroring" option. However this opens the
> > > > > > question that Kieran has asked already. How is the camera module
> > > > > > physically mirrored ? is the pixel array reading direction inverted in
> > > > > > the driver ?
> > > > >
> > > > > I'm not 100% sure where the mirroring is coming from. I confirmed it
> > > > > through qcam on Ubuntu as well as through the camera app and Chromium
> > > > > on a ChromiumOs build so it's not just an application bug. The location and
> > > > > orientation coming from ACPI both seem to be correct. The driver seems
> > > >
> > > > The 'rotation' property from DTS cannot express 'mirrored', and to me
> > > > this seems correct, as 'rotation' is about expressing the mounting
> > > > rotation of the camera sensor (0, 90, 180, 270)
> > > >
> > > > Mirroring comes from inverting the reading directions of the pixel
> > > > units on the pixel array, what is usually called an HFLIP in v4l2
> > > > drivers and we require drivers to not apply any V/HFLIP by default
> > > > (something we might want to document in libcamera as well, not just in
> > > > Linux).
> > > >
> > > > > to be fine on all but some Surface Go devices so it seems unrelated.
> > > >
> > > > What do you mean with "seems fine" ? It doesn't mirror ?
> > >
> > > "Seems fine" as in I don't have a device to test and confirm that it actually
> > > works as expected and isn't mirrored, but the only references I find to
> > > mirrored output from the sensor are for the Surface devices, and I would
> > > expect that to be a prominent enough issue that it would be raised
> > > somewhere.
> >
> > Do you mean that all devices you know about that integrate an OV8865
> > display mirrored images, or is there a mix of devices with the same
> > sensor that display mirrored and non-mirrored images ?
> 
> I have only been able to test on Surface Go devices with this sensor, and
> all of them have been mirrored. I'm unsure what other devices use this
> sensor, but from a lack of complaints or issue reports around mirroring
> make me assume that they receive non-mirrored images.

I wouldn't be so sure about that honestly.

In any case, there's no way the Surface machines have mounted a mirror
in front of the sensor. This issue may be a bug in the driver that
affects all platforms, or possibly a difference in behaviour between
different revisions of the sensor. In any case, the fix shouldn't be in
libcamera, especially not in the HAL implementation.

> > > > > Anything below that is infeasible to fix and the sensor drivers don't
> > > > > maintain quirks for particular misbehaving devices, so the HAL seems
> > > > > to me the best place to add a mirrored config and pipe it down to the
> > > > > driver through the existing Orientation.
> > > >
> > > > Which sensor are we talking about and where does its driver live ? Can
> > > > we see it ? Because some downstream (but also upstream) drivers
> > > > sometimes flip by default.
> > >
> > > This is the ov8865 sensor, driver is drivers/media/i2c/ov8865.c in the
> > > upstream kernel. I confirmedthat the default state is unflipped, and
> > > actually have a separate patch out (https://lkml.org/lkml/2025/7/22/1443)
> > > because there's a bug in the driver which ends up resetting the flip
> > > states back to default after we set it.
> > >
> > > > > > > > > >         /*
> > > > > > > > > >          * Clear and remove any existing configuration from previous calls, and
> > > > > > > > > >          * ensure the required entries are available without further
> > > > > > > > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > > > > > > > > > index 194ca3030..d304a1285 100644
> > > > > > > > > > --- a/src/android/camera_device.h
> > > > > > > > > > +++ b/src/android/camera_device.h
> > > > > > > > > > @@ -126,6 +126,7 @@ private:
> > > > > > > > > >
> > > > > > > > > >         int facing_;
> > > > > > > > > >         int orientation_;
> > > > > > > > > > +       int mirrored_;
> > > > > > > > > >
> > > > > > > > > >         CameraMetadata lastSettings_;
> > > > > > > > > >  };
> > > > > > > > > > diff --git a/src/android/camera_hal_config.cpp b/src/android/camera_hal_config.cpp
> > > > > > > > > > index 7ef451ef8..a2beba5d3 100644
> > > > > > > > > > --- a/src/android/camera_hal_config.cpp
> > > > > > > > > > +++ b/src/android/camera_hal_config.cpp
> > > > > > > > > > @@ -33,6 +33,7 @@ private:
> > > > > > > > > >         int parseCameraConfigData(const std::string &cameraId, const YamlObject &);
> > > > > > > > > >         int parseLocation(const YamlObject &, CameraConfigData &cameraConfigData);
> > > > > > > > > >         int parseRotation(const YamlObject &, CameraConfigData &cameraConfigData);
> > > > > > > > > > +       int parseMirrored(const YamlObject &, CameraConfigData &cameraConfigData);
> > > > > > > > > >
> > > > > > > > > >         std::map<std::string, CameraConfigData> *cameras_;
> > > > > > > > > >  };
> > > > > > > > > > @@ -106,6 +107,9 @@ int CameraHalConfig::Private::parseCameraConfigData(const std::string &cameraId,
> > > > > > > > > >         if (parseRotation(cameraObject, cameraConfigData))
> > > > > > > > > >                 return -EINVAL;
> > > > > > > > > >
> > > > > > > > > > +       /* Parse optional property "mirrored" */
> > > > > > > > > > +       parseMirrored(cameraObject, cameraConfigData);
> > > > > > > > > > +
> > > > > > > > > >         return 0;
> > > > > > > > > >  }
> > > > > > > > > >
> > > > > > > > > > @@ -145,6 +149,16 @@ int CameraHalConfig::Private::parseRotation(const YamlObject &cameraObject,
> > > > > > > > > >         return 0;
> > > > > > > > > >  }
> > > > > > > > > >
> > > > > > > > > > +int CameraHalConfig::Private::parseMirrored(const YamlObject &cameraObject,
> > > > > > > > > > +                                           CameraConfigData &cameraConfigData)
> > > > > > > > > > +{
> > > > > > > > > > +       if (!cameraObject.contains("mirrored"))
> > > > > > > > > > +               return -EINVAL;
> > > > > > > > > > +
> > > > > > > > > > +       cameraConfigData.mirrored = cameraObject["mirrored"].get<bool>(false);
> > > > > > > > > > +       return 0;
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > >  CameraHalConfig::CameraHalConfig()
> > > > > > > > > >         : Extensible(std::make_unique<Private>()), exists_(false), valid_(false)
> > > > > > > > > >  {
> > > > > > > > > > diff --git a/src/android/camera_hal_config.h b/src/android/camera_hal_config.h
> > > > > > > > > > index a4bedb6e6..a96190470 100644
> > > > > > > > > > --- a/src/android/camera_hal_config.h
> > > > > > > > > > +++ b/src/android/camera_hal_config.h
> > > > > > > > > > @@ -15,6 +15,7 @@
> > > > > > > > > >  struct CameraConfigData {
> > > > > > > > > >         int facing = -1;
> > > > > > > > > >         int rotation = -1;
> > > > > > > > > > +       bool mirrored = false;
> > > > > > > > > >  };
> > > > > > > > > >
> > > > > > > > > >  class CameraHalConfig final : public libcamera::Extensible