[v4,4/7] libcamera: software_isp: Check processed window size alignment
diff mbox series

Message ID 20250925192856.77881-5-mzamazal@redhat.com
State Superseded
Headers show
Series
  • Fix stats related problems in software ISP
Related show

Commit Message

Milan Zamazal Sept. 25, 2025, 7:28 p.m. UTC 
The window of the image that should be debayered must be aligned to the
bayer pattern size (this is a requirement of the current debayering
implementation).  This is already ensured for the window corner
coordinates but not for the window sizes.

We can either make the window sizes aligned similarly to how the window
corner coordinates are aligned or reject an unaligned configuration.
This patches chooses the latter as using a different window size than
the requested output size could lead to artefacts.  Since such a
situation is not expected to occur in correctly set up environments, the
change should have no negative impact.

Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
---
 src/libcamera/software_isp/debayer_cpu.cpp | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Hans de Goede Sept. 29, 2025, 11:31 a.m. UTC | #1 
Hi,

On 25-Sep-25 21:28, Milan Zamazal wrote:
> The window of the image that should be debayered must be aligned to the
> bayer pattern size (this is a requirement of the current debayering
> implementation).  This is already ensured for the window corner
> coordinates but not for the window sizes.
> 
> We can either make the window sizes aligned similarly to how the window
> corner coordinates are aligned or reject an unaligned configuration.
> This patches chooses the latter as using a different window size than
> the requested output size could lead to artefacts.  Since such a
> situation is not expected to occur in correctly set up environments, the
> change should have no negative impact.
> 
> Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> Reviewed-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>  src/libcamera/software_isp/debayer_cpu.cpp | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
> index 5f3f22f07..b3b326d9c 100644
> --- a/src/libcamera/software_isp/debayer_cpu.cpp
> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
> @@ -552,7 +552,17 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,
>  	window_.y = ((inputCfg.size.height - outputCfg.size.height) / 2) &
>  		    ~(inputConfig_.patternSize.height - 1);
>  	window_.width = outputCfg.size.width;
> +	if (window_.width % inputConfig_.patternSize.width != 0) {
> +		LOG(Debayer, Error)
> +			<< "Output width is not a multiple of the bayer pattern width";
> +		return -EINVAL;
> +	}
>  	window_.height = outputCfg.size.height;
> +	if (window_.height % inputConfig_.patternSize.height != 0) {
> +		LOG(Debayer, Error)
> +			<< "Output height is not a multiple of the bayer pattern height";
> +		return -EINVAL;
> +	}
>  
>  	/*
>  	 * Set the stats window to the whole processed window. Its coordinates are

configure() already does:

        const StreamConfiguration &outputCfg = outputCfgs[0];
        SizeRange outSizeRange = sizes(inputCfg.pixelFormat, inputCfg.size);
        std::tie(outputConfig_.stride, outputConfig_.frameSize) =
                strideAndFrameSize(outputCfg.pixelFormat, outputCfg.size);

        if (!outSizeRange.contains(outputCfg.size) || outputConfig_.stride != outputCfg.stride) {
                LOG(Debayer, Error)
                        << "Invalid output size/stride: "
                        << "\n  " << outputCfg.size << " (" << outSizeRange << ")"
                        << "\n  " << outputCfg.stride << " (" << outputConfig_.stride << ")";
                return -EINVAL;
        }

and sizes() returns a SizeRange() with hstep / vstep set to inputConfig_.patternSize.width /
inputConfig_.patternSize.height.

so these 2 new checks are redundant and will never trigger since if the size is wrong
the !outSizeRange.contains(outputCfg.size) will already have triggered.

Please drop this patch, as it is a no-op.

Regards,

Hans

Patch
diff mbox series 

diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
index 5f3f22f07..b3b326d9c 100644
--- a/src/libcamera/software_isp/debayer_cpu.cpp
+++ b/src/libcamera/software_isp/debayer_cpu.cpp
@@ -552,7 +552,17 @@  int DebayerCpu::configure(const StreamConfiguration &inputCfg,
 	window_.y = ((inputCfg.size.height - outputCfg.size.height) / 2) &
 		    ~(inputConfig_.patternSize.height - 1);
 	window_.width = outputCfg.size.width;
+	if (window_.width % inputConfig_.patternSize.width != 0) {
+		LOG(Debayer, Error)
+			<< "Output width is not a multiple of the bayer pattern width";
+		return -EINVAL;
+	}
 	window_.height = outputCfg.size.height;
+	if (window_.height % inputConfig_.patternSize.height != 0) {
+		LOG(Debayer, Error)
+			<< "Output height is not a multiple of the bayer pattern height";
+		return -EINVAL;
+	}
 
 	/*
 	 * Set the stats window to the whole processed window. Its coordinates are