[v2,2/3] libcamera: ipa: core: Use native coordinates for IPACameraSensorInfo::analogCrop

27848 diff mbox series

Now that the camera properties use the physical pixel array as reference
coordinate system, do the same with IPACameraSensorInfo::analogCrop.

Replace IPACameraSensorInfo::activeAreaSize with
IPACameraSensorInfo::activeArea to be able to keep the existing
calculations.

Co-developed-by: Naushir Patuck <naush@raspberrypi.com>
Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>

---

Changes in v2:
- Moved all hunks related to IPACameraSensorInfo from the previous patch
  to this one
- Fixed some missing documentation changes
- Moved IPACameraSensorInfo::pixelArraySize to a separate patch
- Pulled in changes by Naush
- Kept the rkisp1 lsc logic. The related changes will come one top
---
 include/libcamera/ipa/core.mojom              | 19 +++++++++++++------
 src/ipa/mali-c55/algorithms/lsc.cpp           |  2 +-
 src/ipa/rkisp1/algorithms/lsc.cpp             |  2 +-
 src/ipa/rpi/common/ipa_base.cpp               | 13 +++++++++----
 src/libcamera/sensor/camera_sensor_legacy.cpp | 17 +++--------------
 src/libcamera/sensor/camera_sensor_raw.cpp    | 17 +++--------------
 6 files changed, 30 insertions(+), 40 deletions(-)

Hi Stefan and Naush

Thanks for tackling this one!!

On Wed, 19 Aug 2026 at 15:15, Stefan Klug <stefan.klug@ideasonboard.com> wrote:
>
> Now that the camera properties use the physical pixel array as reference
> coordinate system, do the same with IPACameraSensorInfo::analogCrop.
>
> Replace IPACameraSensorInfo::activeAreaSize with
> IPACameraSensorInfo::activeArea to be able to keep the existing
> calculations.
>
> Co-developed-by: Naushir Patuck <naush@raspberrypi.com>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
>
> ---
>
> Changes in v2:
> - Moved all hunks related to IPACameraSensorInfo from the previous patch
>   to this one
> - Fixed some missing documentation changes
> - Moved IPACameraSensorInfo::pixelArraySize to a separate patch
> - Pulled in changes by Naush
> - Kept the rkisp1 lsc logic. The related changes will come one top
> ---
>  include/libcamera/ipa/core.mojom              | 19 +++++++++++++------
>  src/ipa/mali-c55/algorithms/lsc.cpp           |  2 +-
>  src/ipa/rkisp1/algorithms/lsc.cpp             |  2 +-
>  src/ipa/rpi/common/ipa_base.cpp               | 13 +++++++++----
>  src/libcamera/sensor/camera_sensor_legacy.cpp | 17 +++--------------
>  src/libcamera/sensor/camera_sensor_raw.cpp    | 17 +++--------------
>  6 files changed, 30 insertions(+), 40 deletions(-)
>
> diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom
> index bce797245829..d3375333baa0 100644
> --- a/include/libcamera/ipa/core.mojom
> +++ b/include/libcamera/ipa/core.mojom
> @@ -153,17 +153,24 @@ module libcamera;
>   */
>
>  /**
> - * \var IPACameraSensorInfo::activeAreaSize
> - * \brief The size of the pixel array active area of the sensor
> + * \var IPACameraSensorInfo::activeArea
> + * \brief The active pixel area of the sensor, relative to the physical pixel
> + * array
> + *
> + * The active area rectangle is expressed relative to the top-left corner of the
> + * physical pixel array. It describes the region of the pixel array which
> + * produces valid image data, excluding non-readable and optical black pixels.
> + *
> + * \note New IPA implementations should *not* use this property and instead rely
> + * on coordinates relative to the physical sensor array
>   */
>
>  /**
>   * \var IPACameraSensorInfo::analogCrop
> - * \brief The portion of the pixel array active area which is read-out and
> - * processed
> + * \brief The portion of the pixel array which is read-out and processed
>   *
>   * The analog crop rectangle top-left corner is defined as the displacement
> - * from the top-left corner of the pixel array active area. The rectangle
> + * from the top-left corner of the physical pixel array. The rectangle
>   * horizontal and vertical sizes define the portion of the pixel array which
>   * is read-out and provided to the sensor's internal processing pipeline, before
>   * any pixel sub-sampling method, such as pixel binning, skipping and averaging
> @@ -243,7 +250,7 @@ struct IPACameraSensorInfo {
>         uint32 bitsPerPixel;
>         uint32 cfaPattern;
>
> -       Size activeAreaSize;
> +       Rectangle activeArea;
>         Rectangle analogCrop;
>         Size outputSize;
>
> diff --git a/src/ipa/mali-c55/algorithms/lsc.cpp b/src/ipa/mali-c55/algorithms/lsc.cpp
> index fff0dc7d0e64..6cd4e9020504 100644
> --- a/src/ipa/mali-c55/algorithms/lsc.cpp
> +++ b/src/ipa/mali-c55/algorithms/lsc.cpp
> @@ -57,7 +57,7 @@ int Lsc::init(IPAContext &context, const ValueNode &tuningData)
>                                 .keys = { "r", "g", "b" },
>                                 .numHSamples = kMeshSize,
>                                 .numVSamples = kMeshSize,
> -                               .sensorSize = context.sensorInfo.activeAreaSize
> +                               .sensorSize = context.sensorInfo.activeArea.size()
>                              });
>  }
>
> diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp
> index f2858656023e..bb41dd3682bd 100644
> --- a/src/ipa/rkisp1/algorithms/lsc.cpp
> +++ b/src/ipa/rkisp1/algorithms/lsc.cpp
> @@ -127,7 +127,7 @@ int LensShadingCorrection::init([[maybe_unused]] IPAContext &context,
>                                 .keys = { "r", "gr", "gb", "b" },
>                                 .numHSamples = RKISP1_CIF_ISP_LSC_SAMPLES_MAX,
>                                 .numVSamples = RKISP1_CIF_ISP_LSC_SAMPLES_MAX,
> -                               .sensorSize = context.sensorInfo.activeAreaSize
> +                               .sensorSize = context.sensorInfo.activeArea.size()
>                              });
>  }
>
> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> index 7e00c2799b4e..9b851007a124 100644
> --- a/src/ipa/rpi/common/ipa_base.cpp
> +++ b/src/ipa/rpi/common/ipa_base.cpp
> @@ -561,10 +561,15 @@ void IpaBase::setMode(const IPACameraSensorInfo &sensorInfo)
>         mode_.bitdepth = sensorInfo.bitsPerPixel;
>         mode_.width = sensorInfo.outputSize.width;
>         mode_.height = sensorInfo.outputSize.height;
> -       mode_.sensorWidth = sensorInfo.activeAreaSize.width;
> -       mode_.sensorHeight = sensorInfo.activeAreaSize.height;
> -       mode_.cropX = sensorInfo.analogCrop.x;
> -       mode_.cropY = sensorInfo.analogCrop.y;
> +       mode_.sensorWidth = sensorInfo.activeArea.width;
> +       mode_.sensorHeight = sensorInfo.activeArea.height;
> +       /*
> +        * CameraMode::cropX/Y are defined relative to the active pixel area,
> +        * whereas IPACameraSensorInfo::analogCrop is relative to the physical
> +        * pixel array. Rebase the crop origin onto the active area.
> +        */
> +       mode_.cropX = sensorInfo.analogCrop.x - sensorInfo.activeArea.x;
> +       mode_.cropY = sensorInfo.analogCrop.y - sensorInfo.activeArea.y;

I wonder if here we should be guarding against negative values, which
we've cause weird behaviour and even crashes in LSC on occasion in the
past?

If sensorInfo.activeArea.x > sensorInfo.analogCrop.x (shouldn't be, of
course...) then set mode_.cropX to zero and issue a warning.

>         mode_.pixelRate = sensorInfo.pixelRate;
>
>         /*
> diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp
> index ef2bf656f845..9e32f798c2dd 100644
> --- a/src/libcamera/sensor/camera_sensor_legacy.cpp
> +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp
> @@ -883,11 +883,10 @@ int CameraSensorLegacy::sensorInfo(IPACameraSensorInfo *info) const
>         info->model = model();
>
>         /*
> -        * The active area size is a static property, while the crop
> -        * rectangle needs to be re-read as it depends on the sensor
> -        * configuration.
> +        * The active area is a static property, while the crop rectangle needs
> +        * to be re-read as it depends on the sensor configuration.
>          */
> -       info->activeAreaSize = { activeArea_.width, activeArea_.height };
> +       info->activeArea = activeArea_;
>
>         /*
>          * \todo Support for retreiving the crop rectangle is scheduled to

I know it wasn't changed here, but s/retreiving/retrieving/

David

> @@ -901,16 +900,6 @@ int CameraSensorLegacy::sensorInfo(IPACameraSensorInfo *info) const
>                         << "The analogue crop rectangle has been defaulted to the active area size";
>         }
>
> -       /*
> -        * IPACameraSensorInfo::analogCrop::x and IPACameraSensorInfo::analogCrop::y
> -        * are defined relatively to the active pixel area, while V4L2's
> -        * TGT_CROP target is defined in respect to the full pixel array.
> -        *
> -        * Compensate it by subtracting the active area offset.
> -        */
> -       info->analogCrop.x -= activeArea_.x;
> -       info->analogCrop.y -= activeArea_.y;
> -
>         /* The bit depth and image size depend on the currently applied format. */
>         V4L2SubdeviceFormat format{};
>         ret = subdev_->getFormat(pad_, &format);
> diff --git a/src/libcamera/sensor/camera_sensor_raw.cpp b/src/libcamera/sensor/camera_sensor_raw.cpp
> index 544d0e52c20d..a1f48b5ff557 100644
> --- a/src/libcamera/sensor/camera_sensor_raw.cpp
> +++ b/src/libcamera/sensor/camera_sensor_raw.cpp
> @@ -1009,27 +1009,16 @@ int CameraSensorRaw::sensorInfo(IPACameraSensorInfo *info) const
>         info->model = model();
>
>         /*
> -        * The active area size is a static property, while the crop
> -        * rectangle needs to be re-read as it depends on the sensor
> -        * configuration.
> +        * The active area is a static property, while the crop rectangle needs
> +        * to be re-read as it depends on the sensor configuration.
>          */
> -       info->activeAreaSize = { activeArea_.width, activeArea_.height };
> +       info->activeArea = activeArea_;
>
>         int ret = subdev_->getSelection(streams_.image.sink, V4L2_SEL_TGT_CROP,
>                                         &info->analogCrop);
>         if (ret)
>                 return ret;
>
> -       /*
> -        * IPACameraSensorInfo::analogCrop::x and IPACameraSensorInfo::analogCrop::y
> -        * are defined relatively to the active pixel area, while V4L2's
> -        * TGT_CROP target is defined in respect to the full pixel array.
> -        *
> -        * Compensate it by subtracting the active area offset.
> -        */
> -       info->analogCrop.x -= activeArea_.x;
> -       info->analogCrop.y -= activeArea_.y;
> -
>         /* The bit depth and image size depend on the currently applied format. */
>         V4L2SubdeviceFormat format{};
>         ret = subdev_->getFormat(streams_.image.source, &format);
> --
> 2.53.0
>

Hi David,

Quoting David Plowman (2026-08-20 16:34:12)
> Hi Stefan and Naush
> 
> Thanks for tackling this one!!
> 
> On Wed, 19 Aug 2026 at 15:15, Stefan Klug <stefan.klug@ideasonboard.com> wrote:
> >
> > Now that the camera properties use the physical pixel array as reference
> > coordinate system, do the same with IPACameraSensorInfo::analogCrop.
> >
> > Replace IPACameraSensorInfo::activeAreaSize with
> > IPACameraSensorInfo::activeArea to be able to keep the existing
> > calculations.
> >
> > Co-developed-by: Naushir Patuck <naush@raspberrypi.com>
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> >
> > ---
> >
> > Changes in v2:
> > - Moved all hunks related to IPACameraSensorInfo from the previous patch
> >   to this one
> > - Fixed some missing documentation changes
> > - Moved IPACameraSensorInfo::pixelArraySize to a separate patch
> > - Pulled in changes by Naush
> > - Kept the rkisp1 lsc logic. The related changes will come one top
> > ---
> >  include/libcamera/ipa/core.mojom              | 19 +++++++++++++------
> >  src/ipa/mali-c55/algorithms/lsc.cpp           |  2 +-
> >  src/ipa/rkisp1/algorithms/lsc.cpp             |  2 +-
> >  src/ipa/rpi/common/ipa_base.cpp               | 13 +++++++++----
> >  src/libcamera/sensor/camera_sensor_legacy.cpp | 17 +++--------------
> >  src/libcamera/sensor/camera_sensor_raw.cpp    | 17 +++--------------
> >  6 files changed, 30 insertions(+), 40 deletions(-)
> >
> > diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom
> > index bce797245829..d3375333baa0 100644
> > --- a/include/libcamera/ipa/core.mojom
> > +++ b/include/libcamera/ipa/core.mojom
> > @@ -153,17 +153,24 @@ module libcamera;
> >   */
> >
> >  /**
> > - * \var IPACameraSensorInfo::activeAreaSize
> > - * \brief The size of the pixel array active area of the sensor
> > + * \var IPACameraSensorInfo::activeArea
> > + * \brief The active pixel area of the sensor, relative to the physical pixel
> > + * array
> > + *
> > + * The active area rectangle is expressed relative to the top-left corner of the
> > + * physical pixel array. It describes the region of the pixel array which
> > + * produces valid image data, excluding non-readable and optical black pixels.
> > + *
> > + * \note New IPA implementations should *not* use this property and instead rely
> > + * on coordinates relative to the physical sensor array
> >   */
> >
> >  /**
> >   * \var IPACameraSensorInfo::analogCrop
> > - * \brief The portion of the pixel array active area which is read-out and
> > - * processed
> > + * \brief The portion of the pixel array which is read-out and processed
> >   *
> >   * The analog crop rectangle top-left corner is defined as the displacement
> > - * from the top-left corner of the pixel array active area. The rectangle
> > + * from the top-left corner of the physical pixel array. The rectangle
> >   * horizontal and vertical sizes define the portion of the pixel array which
> >   * is read-out and provided to the sensor's internal processing pipeline, before
> >   * any pixel sub-sampling method, such as pixel binning, skipping and averaging
> > @@ -243,7 +250,7 @@ struct IPACameraSensorInfo {
> >         uint32 bitsPerPixel;
> >         uint32 cfaPattern;
> >
> > -       Size activeAreaSize;
> > +       Rectangle activeArea;
> >         Rectangle analogCrop;
> >         Size outputSize;
> >
> > diff --git a/src/ipa/mali-c55/algorithms/lsc.cpp b/src/ipa/mali-c55/algorithms/lsc.cpp
> > index fff0dc7d0e64..6cd4e9020504 100644
> > --- a/src/ipa/mali-c55/algorithms/lsc.cpp
> > +++ b/src/ipa/mali-c55/algorithms/lsc.cpp
> > @@ -57,7 +57,7 @@ int Lsc::init(IPAContext &context, const ValueNode &tuningData)
> >                                 .keys = { "r", "g", "b" },
> >                                 .numHSamples = kMeshSize,
> >                                 .numVSamples = kMeshSize,
> > -                               .sensorSize = context.sensorInfo.activeAreaSize
> > +                               .sensorSize = context.sensorInfo.activeArea.size()
> >                              });
> >  }
> >
> > diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp
> > index f2858656023e..bb41dd3682bd 100644
> > --- a/src/ipa/rkisp1/algorithms/lsc.cpp
> > +++ b/src/ipa/rkisp1/algorithms/lsc.cpp
> > @@ -127,7 +127,7 @@ int LensShadingCorrection::init([[maybe_unused]] IPAContext &context,
> >                                 .keys = { "r", "gr", "gb", "b" },
> >                                 .numHSamples = RKISP1_CIF_ISP_LSC_SAMPLES_MAX,
> >                                 .numVSamples = RKISP1_CIF_ISP_LSC_SAMPLES_MAX,
> > -                               .sensorSize = context.sensorInfo.activeAreaSize
> > +                               .sensorSize = context.sensorInfo.activeArea.size()
> >                              });
> >  }
> >
> > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> > index 7e00c2799b4e..9b851007a124 100644
> > --- a/src/ipa/rpi/common/ipa_base.cpp
> > +++ b/src/ipa/rpi/common/ipa_base.cpp
> > @@ -561,10 +561,15 @@ void IpaBase::setMode(const IPACameraSensorInfo &sensorInfo)
> >         mode_.bitdepth = sensorInfo.bitsPerPixel;
> >         mode_.width = sensorInfo.outputSize.width;
> >         mode_.height = sensorInfo.outputSize.height;
> > -       mode_.sensorWidth = sensorInfo.activeAreaSize.width;
> > -       mode_.sensorHeight = sensorInfo.activeAreaSize.height;
> > -       mode_.cropX = sensorInfo.analogCrop.x;
> > -       mode_.cropY = sensorInfo.analogCrop.y;
> > +       mode_.sensorWidth = sensorInfo.activeArea.width;
> > +       mode_.sensorHeight = sensorInfo.activeArea.height;
> > +       /*
> > +        * CameraMode::cropX/Y are defined relative to the active pixel area,
> > +        * whereas IPACameraSensorInfo::analogCrop is relative to the physical
> > +        * pixel array. Rebase the crop origin onto the active area.
> > +        */
> > +       mode_.cropX = sensorInfo.analogCrop.x - sensorInfo.activeArea.x;
> > +       mode_.cropY = sensorInfo.analogCrop.y - sensorInfo.activeArea.y;
> 
> I wonder if here we should be guarding against negative values, which
> we've cause weird behaviour and even crashes in LSC on occasion in the
> past?
> 
> If sensorInfo.activeArea.x > sensorInfo.analogCrop.x (shouldn't be, of
> course...) then set mode_.cropX to zero and issue a warning.

Sure. I can add that to v3. While writing the code I realized that
cropX/Y are unsigned and will therefore wrap around which is
understandably not nice for further processing.

My current change looks like this:

-	mode_.cropX = sensorInfo.analogCrop.x - sensorInfo.activeArea.x;
-	mode_.cropY = sensorInfo.analogCrop.y - sensorInfo.activeArea.y;
+	mode_.cropX = sensorInfo.analogCrop.x - std::min(sensorInfo.activeArea.x,
+							 sensorInfo.analogCrop.x);
+	mode_.cropY = sensorInfo.analogCrop.y - std::min(sensorInfo.activeArea.y,
+							 sensorInfo.analogCrop.y);
+	if (sensorInfo.analogCrop.x < sensorInfo.activeArea.x ||
+	    sensorInfo.analogCrop.y < sensorInfo.activeArea.y) {
+		LOG(IPARPI, Warning) << "Analog crop lies outside the active area."
+				     << " This can result in image artifacts.";
+	}

I'm not too excited about it. Maybe you have a nicer way to write that
or a wish for a different log message?

> 
> >         mode_.pixelRate = sensorInfo.pixelRate;
> >
> >         /*
> > diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp
> > index ef2bf656f845..9e32f798c2dd 100644
> > --- a/src/libcamera/sensor/camera_sensor_legacy.cpp
> > +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp
> > @@ -883,11 +883,10 @@ int CameraSensorLegacy::sensorInfo(IPACameraSensorInfo *info) const
> >         info->model = model();
> >
> >         /*
> > -        * The active area size is a static property, while the crop
> > -        * rectangle needs to be re-read as it depends on the sensor
> > -        * configuration.
> > +        * The active area is a static property, while the crop rectangle needs
> > +        * to be re-read as it depends on the sensor configuration.
> >          */
> > -       info->activeAreaSize = { activeArea_.width, activeArea_.height };
> > +       info->activeArea = activeArea_;
> >
> >         /*
> >          * \todo Support for retreiving the crop rectangle is scheduled to
> 
> I know it wasn't changed here, but s/retreiving/retrieving/

Yep, added.

Best regards,
Stefan

> 
> David
> 
> > @@ -901,16 +900,6 @@ int CameraSensorLegacy::sensorInfo(IPACameraSensorInfo *info) const
> >                         << "The analogue crop rectangle has been defaulted to the active area size";
> >         }
> >
> > -       /*
> > -        * IPACameraSensorInfo::analogCrop::x and IPACameraSensorInfo::analogCrop::y
> > -        * are defined relatively to the active pixel area, while V4L2's
> > -        * TGT_CROP target is defined in respect to the full pixel array.
> > -        *
> > -        * Compensate it by subtracting the active area offset.
> > -        */
> > -       info->analogCrop.x -= activeArea_.x;
> > -       info->analogCrop.y -= activeArea_.y;
> > -
> >         /* The bit depth and image size depend on the currently applied format. */
> >         V4L2SubdeviceFormat format{};
> >         ret = subdev_->getFormat(pad_, &format);
> > diff --git a/src/libcamera/sensor/camera_sensor_raw.cpp b/src/libcamera/sensor/camera_sensor_raw.cpp
> > index 544d0e52c20d..a1f48b5ff557 100644
> > --- a/src/libcamera/sensor/camera_sensor_raw.cpp
> > +++ b/src/libcamera/sensor/camera_sensor_raw.cpp
> > @@ -1009,27 +1009,16 @@ int CameraSensorRaw::sensorInfo(IPACameraSensorInfo *info) const
> >         info->model = model();
> >
> >         /*
> > -        * The active area size is a static property, while the crop
> > -        * rectangle needs to be re-read as it depends on the sensor
> > -        * configuration.
> > +        * The active area is a static property, while the crop rectangle needs
> > +        * to be re-read as it depends on the sensor configuration.
> >          */
> > -       info->activeAreaSize = { activeArea_.width, activeArea_.height };
> > +       info->activeArea = activeArea_;
> >
> >         int ret = subdev_->getSelection(streams_.image.sink, V4L2_SEL_TGT_CROP,
> >                                         &info->analogCrop);
> >         if (ret)
> >                 return ret;
> >
> > -       /*
> > -        * IPACameraSensorInfo::analogCrop::x and IPACameraSensorInfo::analogCrop::y
> > -        * are defined relatively to the active pixel area, while V4L2's
> > -        * TGT_CROP target is defined in respect to the full pixel array.
> > -        *
> > -        * Compensate it by subtracting the active area offset.
> > -        */
> > -       info->analogCrop.x -= activeArea_.x;
> > -       info->analogCrop.y -= activeArea_.y;
> > -
> >         /* The bit depth and image size depend on the currently applied format. */
> >         V4L2SubdeviceFormat format{};
> >         ret = subdev_->getFormat(streams_.image.source, &format);
> > --
> > 2.53.0
> >