[v3,01/11] software-isp: Center the GPU image crop
diff mbox series

Message ID 20261016202818.1405645-2-halfline@gmail.com
State Superseded
Headers show
Series
  • software-isp: Align crops, statistics and metadata
Related show

Commit Message

Ray Strode Oct. 16, 2026, 8:28 p.m. UTC 
From: Ray Strode <rstrode@redhat.com>

The GPU debayer path scales the image to fill a requested output size and
crops any excess image area.

Its projection positions the scaled image against one side of the output,
so aspect-ratio changes and Bayer border removal shift the visible scene.
The series aligns rendering and metering with a centered sensor crop.

This commit centers the projection on the output region in sensor
coordinates, distributing the crop across both sides.

Signed-off-by: Ray Strode <rstrode@redhat.com>
---
 .../internal/software_isp/debayer_geometry.h  | 43 +++++++++++++++++++
 .../internal/software_isp/meson.build         |  1 +
 src/libcamera/software_isp/debayer_egl.cpp    | 21 +++------
 3 files changed, 49 insertions(+), 16 deletions(-)
 create mode 100644 include/libcamera/internal/software_isp/debayer_geometry.h

Comments

Kieran Bingham Oct. 18, 2026, 8:48 p.m. UTC | #1 
Quoting Ray Strode (2026-10-16 21:28:08)
> From: Ray Strode <rstrode@redhat.com>
> 
> The GPU debayer path scales the image to fill a requested output size and
> crops any excess image area.
> 
> Its projection positions the scaled image against one side of the output,
> so aspect-ratio changes and Bayer border removal shift the visible scene.
> The series aligns rendering and metering with a centered sensor crop.
> 
> This commit centers the projection on the output region in sensor
> coordinates, distributing the crop across both sides.
> 
> Signed-off-by: Ray Strode <rstrode@redhat.com>
> ---
>  .../internal/software_isp/debayer_geometry.h  | 43 +++++++++++++++++++
>  .../internal/software_isp/meson.build         |  1 +
>  src/libcamera/software_isp/debayer_egl.cpp    | 21 +++------
>  3 files changed, 49 insertions(+), 16 deletions(-)
>  create mode 100644 include/libcamera/internal/software_isp/debayer_geometry.h
> 
> diff --git a/include/libcamera/internal/software_isp/debayer_geometry.h b/include/libcamera/internal/software_isp/debayer_geometry.h
> new file mode 100644
> index 00000000..70ae6a8b
> --- /dev/null
> +++ b/include/libcamera/internal/software_isp/debayer_geometry.h

Is this really 'debayer' geometry?

As far as I can tell, this file is added just to be able to add some
unit tests around this perhaps?

If this really is to be re-usable, then I could imagine some geometry
helpers around additional components like dewarp perhaps, but then it
really isn't a "debayer geometry".


I'd be tempted to suggest that this patch should just focus on fixing
the transX and transY in place in DebayerEGL::setShaderVariableValues.
It would be a lot easier for reviewers to see the functional change.



> @@ -0,0 +1,43 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2026, Red Hat Inc.
> + *
> + * Geometry for scaling and cropping debayered images
> + */
> +
> +#pragma once
> +
> +#include <algorithm>
> +#include <array>
> +
> +#include <libcamera/geometry.h>
> +
> +namespace libcamera {
> +
> +/*
> + * Return a column-major projection matrix for an input-sized viewport and an
> + * output-sized framebuffer. Scale to fill the output, centering the crop on
> + * the input. All sizes must be nonzero, with nativeSize contained in inputSize.
> + */
> +inline std::array<float, 16> debayerProjection(const Size &inputSize,
> +                                              const Size &nativeSize,
> +                                              const Size &outputSize)
> +{
> +       float scale = std::max(static_cast<float>(outputSize.width) / nativeSize.width,
> +                              static_cast<float>(outputSize.height) / nativeSize.height);
> +
> +       /* The framebuffer center is output/viewport - 1 in normalized coordinates. */
> +       float transX = static_cast<float>(outputSize.width) / inputSize.width - 1.0f;
> +       float transY = static_cast<float>(outputSize.height) / inputSize.height - 1.0f;
> +
> +       /* clang-format off */
> +       return {
> +               scale, 0, 0, 0,
> +               0, scale, 0, 0,
> +               0, 0, 1, 0,
> +               transX, transY, 0, 1,
> +       };
> +       /* clang-format on */
> +}
> +
> +} /* namespace libcamera */
> diff --git a/include/libcamera/internal/software_isp/meson.build b/include/libcamera/internal/software_isp/meson.build
> index df7c3b97..e18597ed 100644
> --- a/include/libcamera/internal/software_isp/meson.build
> +++ b/include/libcamera/internal/software_isp/meson.build
> @@ -2,6 +2,7 @@
>  
>  libcamera_internal_headers += files([
>      'benchmark.h',
> +    'debayer_geometry.h',
>      'debayer_params.h',
>      'software_isp.h',
>      'swisp_stats.h',
> diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp
> index 300822f9..1a7ad25c 100644
> --- a/src/libcamera/software_isp/debayer_egl.cpp
> +++ b/src/libcamera/software_isp/debayer_egl.cpp
> @@ -9,7 +9,7 @@
>  
>  #include "debayer_egl.h"
>  
> -#include <algorithm>
> +#include <array>
>  #include <assert.h>
>  #include <memory>
>  #include <span>
> @@ -24,6 +24,7 @@
>  
>  #include "libcamera/internal/formats.h"
>  #include "libcamera/internal/framebuffer.h"
> +#include "libcamera/internal/software_isp/debayer_geometry.h"
>  
>  #include "../shaders/glsl_shaders.h"
>  
> @@ -388,20 +389,8 @@ void DebayerEGL::setShaderVariableValues(eGLImage &eglImageIn, const DebayerPara
>         GLfloat Step[] = { static_cast<float>(bytesPerPixel_) / (inputConfig_.stride - 1),
>                            1.0f / (height_ - 1) };
>         GLfloat Stride = (GLfloat)width_ / (shaderStridePixels_ / bytesPerPixel_);
> -       /*
> -        * Scale the output size from the native size the algorithm produces for
> -        * the input size. Keep the aspect ratio and prefer cropping over black
> -        * bars.
> -        */
> -       GLfloat scale = std::max((GLfloat)outputSize_.width / nativeOutputSize_.width,
> -                                (GLfloat)outputSize_.height / nativeOutputSize_.height);
> -       GLfloat trans = -(1.0f - scale);
> -       GLfloat projMatrix[] = {
> -               scale, 0, 0, 0,
> -               0, scale, 0, 0,
> -               0, 0, 1, 0,
> -               trans, trans, 0, 1
> -       };
> +       const std::array<float, 16> projMatrix =
> +               debayerProjection({ width_, height_ }, nativeOutputSize_, outputSize_);
>         /* Static const coordinates */
>         static const GLfloat vcoordinates[4][2] = {
>                 { -1.0f, -1.0f },
> @@ -445,7 +434,7 @@ void DebayerEGL::setShaderVariableValues(eGLImage &eglImageIn, const DebayerPara
>         glUniform2fv(textureUniformSize_, 1, imgSize);
>         glUniform2fv(textureUniformStep_, 1, Step);
>         glUniform1f(textureUniformStrideFactor_, Stride);
> -       glUniformMatrix4fv(textureUniformProjMatrix_, 1, GL_FALSE, projMatrix);
> +       glUniformMatrix4fv(textureUniformProjMatrix_, 1, GL_FALSE, projMatrix.data());
>  
>         LOG(Debayer, Debug) << "vertexIn " << attributeVertex_ << " textureIn " << attributeTexture_
>                             << " tex_y " << textureUniformBayerDataIn_
> -- 
> 2.56.0
>
Kieran Bingham Oct. 19, 2026, 1:46 p.m. UTC | #2 
Quoting Kieran Bingham (2026-10-18 21:48:40)
> Quoting Ray Strode (2026-10-16 21:28:08)
> > From: Ray Strode <rstrode@redhat.com>
> > 
> > The GPU debayer path scales the image to fill a requested output size and
> > crops any excess image area.
> > 
> > Its projection positions the scaled image against one side of the output,
> > so aspect-ratio changes and Bayer border removal shift the visible scene.
> > The series aligns rendering and metering with a centered sensor crop.
> > 
> > This commit centers the projection on the output region in sensor
> > coordinates, distributing the crop across both sides.
> > 
> > Signed-off-by: Ray Strode <rstrode@redhat.com>
> > ---
> >  .../internal/software_isp/debayer_geometry.h  | 43 +++++++++++++++++++
> >  .../internal/software_isp/meson.build         |  1 +
> >  src/libcamera/software_isp/debayer_egl.cpp    | 21 +++------
> >  3 files changed, 49 insertions(+), 16 deletions(-)
> >  create mode 100644 include/libcamera/internal/software_isp/debayer_geometry.h
> > 
> > diff --git a/include/libcamera/internal/software_isp/debayer_geometry.h b/include/libcamera/internal/software_isp/debayer_geometry.h
> > new file mode 100644
> > index 00000000..70ae6a8b
> > --- /dev/null
> > +++ b/include/libcamera/internal/software_isp/debayer_geometry.h
> 
> Is this really 'debayer' geometry?
> 
> As far as I can tell, this file is added just to be able to add some
> unit tests around this perhaps?
> 
> If this really is to be re-usable, then I could imagine some geometry
> helpers around additional components like dewarp perhaps, but then it
> really isn't a "debayer geometry".
> 
> 
> I'd be tempted to suggest that this patch should just focus on fixing
> the transX and transY in place in DebayerEGL::setShaderVariableValues.
> It would be a lot easier for reviewers to see the functional change.
> 

In fact, I see this was proposed already in:

 - https://patchwork.libcamera.org/patch/28178/

 
> > @@ -0,0 +1,43 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2026, Red Hat Inc.
> > + *
> > + * Geometry for scaling and cropping debayered images
> > + */
> > +
> > +#pragma once
> > +
> > +#include <algorithm>
> > +#include <array>
> > +
> > +#include <libcamera/geometry.h>
> > +
> > +namespace libcamera {
> > +
> > +/*
> > + * Return a column-major projection matrix for an input-sized viewport and an
> > + * output-sized framebuffer. Scale to fill the output, centering the crop on
> > + * the input. All sizes must be nonzero, with nativeSize contained in inputSize.
> > + */
> > +inline std::array<float, 16> debayerProjection(const Size &inputSize,
> > +                                              const Size &nativeSize,
> > +                                              const Size &outputSize)
> > +{
> > +       float scale = std::max(static_cast<float>(outputSize.width) / nativeSize.width,
> > +                              static_cast<float>(outputSize.height) / nativeSize.height);
> > +
> > +       /* The framebuffer center is output/viewport - 1 in normalized coordinates. */
> > +       float transX = static_cast<float>(outputSize.width) / inputSize.width - 1.0f;
> > +       float transY = static_cast<float>(outputSize.height) / inputSize.height - 1.0f;
> > +
> > +       /* clang-format off */
> > +       return {
> > +               scale, 0, 0, 0,
> > +               0, scale, 0, 0,
> > +               0, 0, 1, 0,
> > +               transX, transY, 0, 1,
> > +       };
> > +       /* clang-format on */
> > +}
> > +
> > +} /* namespace libcamera */
> > diff --git a/include/libcamera/internal/software_isp/meson.build b/include/libcamera/internal/software_isp/meson.build
> > index df7c3b97..e18597ed 100644
> > --- a/include/libcamera/internal/software_isp/meson.build
> > +++ b/include/libcamera/internal/software_isp/meson.build
> > @@ -2,6 +2,7 @@
> >  
> >  libcamera_internal_headers += files([
> >      'benchmark.h',
> > +    'debayer_geometry.h',
> >      'debayer_params.h',
> >      'software_isp.h',
> >      'swisp_stats.h',
> > diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp
> > index 300822f9..1a7ad25c 100644
> > --- a/src/libcamera/software_isp/debayer_egl.cpp
> > +++ b/src/libcamera/software_isp/debayer_egl.cpp
> > @@ -9,7 +9,7 @@
> >  
> >  #include "debayer_egl.h"
> >  
> > -#include <algorithm>
> > +#include <array>
> >  #include <assert.h>
> >  #include <memory>
> >  #include <span>
> > @@ -24,6 +24,7 @@
> >  
> >  #include "libcamera/internal/formats.h"
> >  #include "libcamera/internal/framebuffer.h"
> > +#include "libcamera/internal/software_isp/debayer_geometry.h"
> >  
> >  #include "../shaders/glsl_shaders.h"
> >  
> > @@ -388,20 +389,8 @@ void DebayerEGL::setShaderVariableValues(eGLImage &eglImageIn, const DebayerPara
> >         GLfloat Step[] = { static_cast<float>(bytesPerPixel_) / (inputConfig_.stride - 1),
> >                            1.0f / (height_ - 1) };
> >         GLfloat Stride = (GLfloat)width_ / (shaderStridePixels_ / bytesPerPixel_);
> > -       /*
> > -        * Scale the output size from the native size the algorithm produces for
> > -        * the input size. Keep the aspect ratio and prefer cropping over black
> > -        * bars.
> > -        */
> > -       GLfloat scale = std::max((GLfloat)outputSize_.width / nativeOutputSize_.width,
> > -                                (GLfloat)outputSize_.height / nativeOutputSize_.height);
> > -       GLfloat trans = -(1.0f - scale);
> > -       GLfloat projMatrix[] = {
> > -               scale, 0, 0, 0,
> > -               0, scale, 0, 0,
> > -               0, 0, 1, 0,
> > -               trans, trans, 0, 1
> > -       };
> > +       const std::array<float, 16> projMatrix =
> > +               debayerProjection({ width_, height_ }, nativeOutputSize_, outputSize_);
> >         /* Static const coordinates */
> >         static const GLfloat vcoordinates[4][2] = {
> >                 { -1.0f, -1.0f },
> > @@ -445,7 +434,7 @@ void DebayerEGL::setShaderVariableValues(eGLImage &eglImageIn, const DebayerPara
> >         glUniform2fv(textureUniformSize_, 1, imgSize);
> >         glUniform2fv(textureUniformStep_, 1, Step);
> >         glUniform1f(textureUniformStrideFactor_, Stride);
> > -       glUniformMatrix4fv(textureUniformProjMatrix_, 1, GL_FALSE, projMatrix);
> > +       glUniformMatrix4fv(textureUniformProjMatrix_, 1, GL_FALSE, projMatrix.data());
> >  
> >         LOG(Debayer, Debug) << "vertexIn " << attributeVertex_ << " textureIn " << attributeTexture_
> >                             << " tex_y " << textureUniformBayerDataIn_
> > -- 
> > 2.56.0
> >
Ray Strode Oct. 19, 2026, 3:14 p.m. UTC | #3 
Hi,

On Mon, Oct 19, 2026 at 9:47 AM Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
> Quoting Kieran Bingham (2026-10-18 21:48:40)
> >
> > Is this really 'debayer' geometry?

I suppose it was more general, calculating scaling and translation
between the input viewport and the output framebuffer.

> > As far as I can tell, this file is added just to be able to add some
> > unit tests around this perhaps?

Indeed, the point of splitting it out was entirely so we could add
test coverage to check the matrix calculations.

> > If this really is to be re-usable, then I could imagine some geometry
> > helpers around additional components like dewarp perhaps, but then it
> > really isn't a "debayer geometry".

You're right, a better name would probably be image_projection or
something like that.

> > I'd be tempted to suggest that this patch should just focus on fixing
> > the transX and transY in place in DebayerEGL::setShaderVariableValues.
> > It would be a lot easier for reviewers to see the functional change.

I think I agree with you now, we should keep the changes as targeted,
reviewable and as noncontroversial as possible.

> In fact, I see this was proposed already in:
>
>  - https://patchwork.libcamera.org/patch/28178/

oh perfect. i'll use that as the start and then put white
balance/exposure fixes and scalercrop metadata on top. I don't think
the unit
test gained by splitting out the projection helper is any huge win, we
can just drop that.

--Ray

Patch
diff mbox series 

diff --git a/include/libcamera/internal/software_isp/debayer_geometry.h b/include/libcamera/internal/software_isp/debayer_geometry.h
new file mode 100644
index 00000000..70ae6a8b
--- /dev/null
+++ b/include/libcamera/internal/software_isp/debayer_geometry.h
@@ -0,0 +1,43 @@ 
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+/*
+ * Copyright (C) 2026, Red Hat Inc.
+ *
+ * Geometry for scaling and cropping debayered images
+ */
+
+#pragma once
+
+#include <algorithm>
+#include <array>
+
+#include <libcamera/geometry.h>
+
+namespace libcamera {
+
+/*
+ * Return a column-major projection matrix for an input-sized viewport and an
+ * output-sized framebuffer. Scale to fill the output, centering the crop on
+ * the input. All sizes must be nonzero, with nativeSize contained in inputSize.
+ */
+inline std::array<float, 16> debayerProjection(const Size &inputSize,
+					       const Size &nativeSize,
+					       const Size &outputSize)
+{
+	float scale = std::max(static_cast<float>(outputSize.width) / nativeSize.width,
+			       static_cast<float>(outputSize.height) / nativeSize.height);
+
+	/* The framebuffer center is output/viewport - 1 in normalized coordinates. */
+	float transX = static_cast<float>(outputSize.width) / inputSize.width - 1.0f;
+	float transY = static_cast<float>(outputSize.height) / inputSize.height - 1.0f;
+
+	/* clang-format off */
+	return {
+		scale, 0, 0, 0,
+		0, scale, 0, 0,
+		0, 0, 1, 0,
+		transX, transY, 0, 1,
+	};
+	/* clang-format on */
+}
+
+} /* namespace libcamera */
diff --git a/include/libcamera/internal/software_isp/meson.build b/include/libcamera/internal/software_isp/meson.build
index df7c3b97..e18597ed 100644
--- a/include/libcamera/internal/software_isp/meson.build
+++ b/include/libcamera/internal/software_isp/meson.build
@@ -2,6 +2,7 @@ 
 
 libcamera_internal_headers += files([
     'benchmark.h',
+    'debayer_geometry.h',
     'debayer_params.h',
     'software_isp.h',
     'swisp_stats.h',
diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp
index 300822f9..1a7ad25c 100644
--- a/src/libcamera/software_isp/debayer_egl.cpp
+++ b/src/libcamera/software_isp/debayer_egl.cpp
@@ -9,7 +9,7 @@ 
 
 #include "debayer_egl.h"
 
-#include <algorithm>
+#include <array>
 #include <assert.h>
 #include <memory>
 #include <span>
@@ -24,6 +24,7 @@ 
 
 #include "libcamera/internal/formats.h"
 #include "libcamera/internal/framebuffer.h"
+#include "libcamera/internal/software_isp/debayer_geometry.h"
 
 #include "../shaders/glsl_shaders.h"
 
@@ -388,20 +389,8 @@  void DebayerEGL::setShaderVariableValues(eGLImage &eglImageIn, const DebayerPara
 	GLfloat Step[] = { static_cast<float>(bytesPerPixel_) / (inputConfig_.stride - 1),
 			   1.0f / (height_ - 1) };
 	GLfloat Stride = (GLfloat)width_ / (shaderStridePixels_ / bytesPerPixel_);
-	/*
-	 * Scale the output size from the native size the algorithm produces for
-	 * the input size. Keep the aspect ratio and prefer cropping over black
-	 * bars.
-	 */
-	GLfloat scale = std::max((GLfloat)outputSize_.width / nativeOutputSize_.width,
-				 (GLfloat)outputSize_.height / nativeOutputSize_.height);
-	GLfloat trans = -(1.0f - scale);
-	GLfloat projMatrix[] = {
-		scale, 0, 0, 0,
-		0, scale, 0, 0,
-		0, 0, 1, 0,
-		trans, trans, 0, 1
-	};
+	const std::array<float, 16> projMatrix =
+		debayerProjection({ width_, height_ }, nativeOutputSize_, outputSize_);
 	/* Static const coordinates */
 	static const GLfloat vcoordinates[4][2] = {
 		{ -1.0f, -1.0f },
@@ -445,7 +434,7 @@  void DebayerEGL::setShaderVariableValues(eGLImage &eglImageIn, const DebayerPara
 	glUniform2fv(textureUniformSize_, 1, imgSize);
 	glUniform2fv(textureUniformStep_, 1, Step);
 	glUniform1f(textureUniformStrideFactor_, Stride);
-	glUniformMatrix4fv(textureUniformProjMatrix_, 1, GL_FALSE, projMatrix);
+	glUniformMatrix4fv(textureUniformProjMatrix_, 1, GL_FALSE, projMatrix.data());
 
 	LOG(Debayer, Debug) << "vertexIn " << attributeVertex_ << " textureIn " << attributeTexture_
 			    << " tex_y " << textureUniformBayerDataIn_