[v2] libcamera: debayer: Use helpers for strides and frame sizes
diff mbox series

Message ID 20260720113935.52936-1-robert.mader@collabora.com
State Accepted
Headers show
Series
  • [v2] libcamera: debayer: Use helpers for strides and frame sizes
Related show

Commit Message

Robert Mader July 20, 2026, 11:39 a.m. UTC 
Clean up some open-coded logic, making the code shorter, more generic
and easier to follow.

Signed-off-by: Robert Mader <robert.mader@collabora.com>

---

Changes in v2:
 - remove bpp docs to fix CI
---
 src/libcamera/software_isp/debayer.cpp     |  3 ---
 src/libcamera/software_isp/debayer.h       |  1 -
 src/libcamera/software_isp/debayer_cpu.cpp | 29 +++-------------------
 src/libcamera/software_isp/debayer_cpu.h   |  1 -
 src/libcamera/software_isp/debayer_egl.cpp | 24 ++----------------
 src/libcamera/software_isp/debayer_egl.h   |  1 -
 6 files changed, 5 insertions(+), 54 deletions(-)

Comments

Milan Zamazal July 21, 2026, 4:14 p.m. UTC | #1 
Hi Robert,

thank you for the cleanup.

Robert Mader <robert.mader@collabora.com> writes:

> Clean up some open-coded logic, making the code shorter, more generic
> and easier to follow.
>
> Signed-off-by: Robert Mader <robert.mader@collabora.com>
>
> ---
>
> Changes in v2:
>  - remove bpp docs to fix CI
> ---
>  src/libcamera/software_isp/debayer.cpp     |  3 ---
>  src/libcamera/software_isp/debayer.h       |  1 -
>  src/libcamera/software_isp/debayer_cpu.cpp | 29 +++-------------------
>  src/libcamera/software_isp/debayer_cpu.h   |  1 -
>  src/libcamera/software_isp/debayer_egl.cpp | 24 ++----------------
>  src/libcamera/software_isp/debayer_egl.h   |  1 -
>  6 files changed, 5 insertions(+), 54 deletions(-)
>
> diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp
> index 56446f55d..a4854e51b 100644
> --- a/src/libcamera/software_isp/debayer.cpp
> +++ b/src/libcamera/software_isp/debayer.cpp
> @@ -192,9 +192,6 @@ Debayer::~Debayer()
>   * Defines how the output of the debayer process is laid out in memory.
>   * It includes per-pixel size, stride, and total frame size.
>   *
> - * \var Debayer::DebayerOutputConfig::bpp
> - * Bytes used per pixel in the output format.
> - *
>   * \var Debayer::DebayerOutputConfig::stride
>   * Line stride in bytes for the output frame.
>   *
> diff --git a/src/libcamera/software_isp/debayer.h b/src/libcamera/software_isp/debayer.h
> index 28f1b857b..556852260 100644
> --- a/src/libcamera/software_isp/debayer.h
> +++ b/src/libcamera/software_isp/debayer.h
> @@ -69,7 +69,6 @@ public:
>  	};
>  
>  	struct DebayerOutputConfig {
> -		unsigned int bpp;
>  		unsigned int stride;
>  		unsigned int frameSize;
>  	};
> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
> index 49382b4c2..25e9830c8 100644
> --- a/src/libcamera/software_isp/debayer_cpu.cpp
> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
> @@ -24,6 +24,7 @@
>  
>  #include "libcamera/internal/bayer_format.h"
>  #include "libcamera/internal/camera_manager.h"
> +#include "libcamera/internal/formats.h"
>  #include "libcamera/internal/framebuffer.h"
>  #include "libcamera/internal/global_configuration.h"
>  #include "libcamera/internal/mapped_framebuffer.h"
> @@ -474,24 +475,6 @@ int DebayerCpu::getInputConfig(PixelFormat inputFormat, DebayerInputConfig &conf
>  	return -EINVAL;
>  }
>  
> -int DebayerCpu::getOutputConfig(PixelFormat outputFormat, DebayerOutputConfig &config)
> -{
> -	if (outputFormat == formats::RGB888 || outputFormat == formats::BGR888) {
> -		config.bpp = 24;
> -		return 0;
> -	}
> -
> -	if (outputFormat == formats::XRGB8888 || outputFormat == formats::ARGB8888 ||
> -	    outputFormat == formats::XBGR8888 || outputFormat == formats::ABGR8888) {
> -		config.bpp = 32;
> -		return 0;
> -	}
> -
> -	LOG(Debayer, Info)
> -		<< "Unsupported output format " << outputFormat.toString();
> -	return -EINVAL;

What to do about unsupported formats now?  I don't think this was
handled correctly in the original case.  But can we do better now?  Or
safely assume that anything that reaches debayering is a supported
format?

> -}
> -
>  /*
>   * Check for standard Bayer orders and set xShift_ and swap debayer0/1, so that
>   * a single pair of BGGR debayer functions can be used for all 4 standard orders.
> @@ -776,15 +759,9 @@ std::vector<PixelFormat> DebayerCpu::formats(PixelFormat inputFormat)
>  std::tuple<unsigned int, unsigned int>
>  DebayerCpu::strideAndFrameSize(const PixelFormat &outputFormat, const Size &size)
>  {
> -	DebayerCpu::DebayerOutputConfig config;
> -
> -	if (getOutputConfig(outputFormat, config) != 0)
> -		return std::make_tuple(0, 0);
> -
>  	/* round up to multiple of 8 for 64 bits alignment */
> -	unsigned int stride = (size.width * config.bpp / 8 + 7) & ~7;
> -
> -	return std::make_tuple(stride, stride * size.height);
> +	const PixelFormatInfo &info = PixelFormatInfo::info(outputFormat);
> +	return std::make_tuple(info.stride(size.width, 0, 8), info.frameSize(size, 8));
>  }
>  
>  void DebayerCpuThread::setupInputMemcpy(const uint8_t *linePointers[])
> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h
> index 5281c65cb..2c88c9e1a 100644
> --- a/src/libcamera/software_isp/debayer_cpu.h
> +++ b/src/libcamera/software_isp/debayer_cpu.h
> @@ -121,7 +121,6 @@ private:
>  	void debayer12P_RGRG_BGR888(uint8_t *dst, const uint8_t *src[]);
>  
>  	static int getInputConfig(PixelFormat inputFormat, DebayerInputConfig &config);
> -	static int getOutputConfig(PixelFormat outputFormat, DebayerOutputConfig &config);
>  	int setupStandardBayerOrder(BayerFormat::Order order);
>  	int setDebayerFunctions(PixelFormat inputFormat,
>  				PixelFormat outputFormat,
> diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp
> index 02651fe87..24faf480d 100644
> --- a/src/libcamera/software_isp/debayer_egl.cpp
> +++ b/src/libcamera/software_isp/debayer_egl.cpp
> @@ -96,20 +96,6 @@ int DebayerEGL::getInputConfig(PixelFormat inputFormat, DebayerInputConfig &conf
>  	return -EINVAL;
>  }
>  
> -int DebayerEGL::getOutputConfig(PixelFormat outputFormat, DebayerOutputConfig &config)
> -{
> -	if (outputFormat == formats::XRGB8888 || outputFormat == formats::ARGB8888 ||
> -	    outputFormat == formats::XBGR8888 || outputFormat == formats::ABGR8888) {
> -		config.bpp = 32;
> -		return 0;
> -	}
> -
> -	LOG(Debayer, Info)
> -		<< "Unsupported output format " << outputFormat;
> -
> -	return -EINVAL;
> -}
> -
>  int DebayerEGL::getShaderVariableLocations(void)
>  {
>  	attributeVertex_ = glGetAttribLocation(programId_, "vertexIn");
> @@ -374,15 +360,9 @@ std::vector<PixelFormat> DebayerEGL::formats(PixelFormat inputFormat)
>  std::tuple<unsigned int, unsigned int>
>  DebayerEGL::strideAndFrameSize(const PixelFormat &outputFormat, const Size &size)
>  {
> -	DebayerEGL::DebayerOutputConfig config;
> -
> -	if (getOutputConfig(outputFormat, config) != 0)
> -		return std::make_tuple(0, 0);
> -
>  	/* Align stride to 256 bytes as a generic GPU memory access alignment */
> -	unsigned int stride = libcamera::utils::alignUp(size.width * config.bpp / 8, 256);
> -
> -	return std::make_tuple(stride, stride * size.height);
> +	const PixelFormatInfo &info = PixelFormatInfo::info(outputFormat);
> +	return std::make_tuple(info.stride(size.width, 0, 256), info.frameSize(size, 256));
>  }
>  
>  uint32_t DebayerEGL::preferredInputStride(const PixelFormat &inputFormat, const Size &size)
> diff --git a/src/libcamera/software_isp/debayer_egl.h b/src/libcamera/software_isp/debayer_egl.h
> index d6223f3a9..e613639f5 100644
> --- a/src/libcamera/software_isp/debayer_egl.h
> +++ b/src/libcamera/software_isp/debayer_egl.h
> @@ -63,7 +63,6 @@ public:
>  
>  private:
>  	static int getInputConfig(PixelFormat inputFormat, DebayerInputConfig &config);
> -	static int getOutputConfig(PixelFormat outputFormat, DebayerOutputConfig &config);
>  	int initBayerShaders(PixelFormat inputFormat, PixelFormat outputFormat);
>  	int getShaderVariableLocations();
>  	void setShaderVariableValues(eGLImage &eGLImageIn, const DebayerParams &params);
Robert Mader July 21, 2026, 4:41 p.m. UTC | #2 
Hi Milan, thanks for the feedback!

On 21.07.26 18:14, Milan Zamazal wrote:
> Hi Robert,
>
> thank you for the cleanup.
>
> Robert Mader <robert.mader@collabora.com> writes:
>
>> Clean up some open-coded logic, making the code shorter, more generic
>> and easier to follow.
>>
>> Signed-off-by: Robert Mader <robert.mader@collabora.com>
>>
>> ---
>>
>> Changes in v2:
>>   - remove bpp docs to fix CI
>> ---
>>   src/libcamera/software_isp/debayer.cpp     |  3 ---
>>   src/libcamera/software_isp/debayer.h       |  1 -
>>   src/libcamera/software_isp/debayer_cpu.cpp | 29 +++-------------------
>>   src/libcamera/software_isp/debayer_cpu.h   |  1 -
>>   src/libcamera/software_isp/debayer_egl.cpp | 24 ++----------------
>>   src/libcamera/software_isp/debayer_egl.h   |  1 -
>>   6 files changed, 5 insertions(+), 54 deletions(-)
>>
>> diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp
>> index 56446f55d..a4854e51b 100644
>> --- a/src/libcamera/software_isp/debayer.cpp
>> +++ b/src/libcamera/software_isp/debayer.cpp
>> @@ -192,9 +192,6 @@ Debayer::~Debayer()
>>    * Defines how the output of the debayer process is laid out in memory.
>>    * It includes per-pixel size, stride, and total frame size.
>>    *
>> - * \var Debayer::DebayerOutputConfig::bpp
>> - * Bytes used per pixel in the output format.
>> - *
>>    * \var Debayer::DebayerOutputConfig::stride
>>    * Line stride in bytes for the output frame.
>>    *
>> diff --git a/src/libcamera/software_isp/debayer.h b/src/libcamera/software_isp/debayer.h
>> index 28f1b857b..556852260 100644
>> --- a/src/libcamera/software_isp/debayer.h
>> +++ b/src/libcamera/software_isp/debayer.h
>> @@ -69,7 +69,6 @@ public:
>>   	};
>>   
>>   	struct DebayerOutputConfig {
>> -		unsigned int bpp;
>>   		unsigned int stride;
>>   		unsigned int frameSize;
>>   	};
>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
>> index 49382b4c2..25e9830c8 100644
>> --- a/src/libcamera/software_isp/debayer_cpu.cpp
>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
>> @@ -24,6 +24,7 @@
>>   
>>   #include "libcamera/internal/bayer_format.h"
>>   #include "libcamera/internal/camera_manager.h"
>> +#include "libcamera/internal/formats.h"
>>   #include "libcamera/internal/framebuffer.h"
>>   #include "libcamera/internal/global_configuration.h"
>>   #include "libcamera/internal/mapped_framebuffer.h"
>> @@ -474,24 +475,6 @@ int DebayerCpu::getInputConfig(PixelFormat inputFormat, DebayerInputConfig &conf
>>   	return -EINVAL;
>>   }
>>   
>> -int DebayerCpu::getOutputConfig(PixelFormat outputFormat, DebayerOutputConfig &config)
>> -{
>> -	if (outputFormat == formats::RGB888 || outputFormat == formats::BGR888) {
>> -		config.bpp = 24;
>> -		return 0;
>> -	}
>> -
>> -	if (outputFormat == formats::XRGB8888 || outputFormat == formats::ARGB8888 ||
>> -	    outputFormat == formats::XBGR8888 || outputFormat == formats::ABGR8888) {
>> -		config.bpp = 32;
>> -		return 0;
>> -	}
>> -
>> -	LOG(Debayer, Info)
>> -		<< "Unsupported output format " << outputFormat.toString();
>> -	return -EINVAL;
> What to do about unsupported formats now?  I don't think this was
> handled correctly in the original case.  But can we do better now?  Or
> safely assume that anything that reaches debayering is a supported
> format?

First of all that would require a pretty broken pipeline handler - in 
simple.cpp we have

config.outputFormats = swIsp_->formats(pixelFormat);

ensuring that only supported formats are set for an output config (AFAICS).

Secondly we'd still fail in DebayerCpu::setDebayerFunctions() (called in 
configure(), just like strideAndFrameSize()) and 
DebayerEGL::initBayerShaders() (called in start() - i.e. slightly later 
/ not in configure()).

So a broken pipeline handler would fail slightly later in the GPU path. 
If we want to be extra sure we could add a dedicated check in 
DebayerEGL::configure(), checking that outputCfg.pixelFormat is present 
in inputConfig_.outputFormats. I'm not fully convinced it's worth it, 
but it's small and easy to add if you prefer.

Regards

Robert

>> -}
>> -
>>   /*
>>    * Check for standard Bayer orders and set xShift_ and swap debayer0/1, so that
>>    * a single pair of BGGR debayer functions can be used for all 4 standard orders.
>> @@ -776,15 +759,9 @@ std::vector<PixelFormat> DebayerCpu::formats(PixelFormat inputFormat)
>>   std::tuple<unsigned int, unsigned int>
>>   DebayerCpu::strideAndFrameSize(const PixelFormat &outputFormat, const Size &size)
>>   {
>> -	DebayerCpu::DebayerOutputConfig config;
>> -
>> -	if (getOutputConfig(outputFormat, config) != 0)
>> -		return std::make_tuple(0, 0);
>> -
>>   	/* round up to multiple of 8 for 64 bits alignment */
>> -	unsigned int stride = (size.width * config.bpp / 8 + 7) & ~7;
>> -
>> -	return std::make_tuple(stride, stride * size.height);
>> +	const PixelFormatInfo &info = PixelFormatInfo::info(outputFormat);
>> +	return std::make_tuple(info.stride(size.width, 0, 8), info.frameSize(size, 8));
>>   }
>>   
>>   void DebayerCpuThread::setupInputMemcpy(const uint8_t *linePointers[])
>> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h
>> index 5281c65cb..2c88c9e1a 100644
>> --- a/src/libcamera/software_isp/debayer_cpu.h
>> +++ b/src/libcamera/software_isp/debayer_cpu.h
>> @@ -121,7 +121,6 @@ private:
>>   	void debayer12P_RGRG_BGR888(uint8_t *dst, const uint8_t *src[]);
>>   
>>   	static int getInputConfig(PixelFormat inputFormat, DebayerInputConfig &config);
>> -	static int getOutputConfig(PixelFormat outputFormat, DebayerOutputConfig &config);
>>   	int setupStandardBayerOrder(BayerFormat::Order order);
>>   	int setDebayerFunctions(PixelFormat inputFormat,
>>   				PixelFormat outputFormat,
>> diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp
>> index 02651fe87..24faf480d 100644
>> --- a/src/libcamera/software_isp/debayer_egl.cpp
>> +++ b/src/libcamera/software_isp/debayer_egl.cpp
>> @@ -96,20 +96,6 @@ int DebayerEGL::getInputConfig(PixelFormat inputFormat, DebayerInputConfig &conf
>>   	return -EINVAL;
>>   }
>>   
>> -int DebayerEGL::getOutputConfig(PixelFormat outputFormat, DebayerOutputConfig &config)
>> -{
>> -	if (outputFormat == formats::XRGB8888 || outputFormat == formats::ARGB8888 ||
>> -	    outputFormat == formats::XBGR8888 || outputFormat == formats::ABGR8888) {
>> -		config.bpp = 32;
>> -		return 0;
>> -	}
>> -
>> -	LOG(Debayer, Info)
>> -		<< "Unsupported output format " << outputFormat;
>> -
>> -	return -EINVAL;
>> -}
>> -
>>   int DebayerEGL::getShaderVariableLocations(void)
>>   {
>>   	attributeVertex_ = glGetAttribLocation(programId_, "vertexIn");
>> @@ -374,15 +360,9 @@ std::vector<PixelFormat> DebayerEGL::formats(PixelFormat inputFormat)
>>   std::tuple<unsigned int, unsigned int>
>>   DebayerEGL::strideAndFrameSize(const PixelFormat &outputFormat, const Size &size)
>>   {
>> -	DebayerEGL::DebayerOutputConfig config;
>> -
>> -	if (getOutputConfig(outputFormat, config) != 0)
>> -		return std::make_tuple(0, 0);
>> -
>>   	/* Align stride to 256 bytes as a generic GPU memory access alignment */
>> -	unsigned int stride = libcamera::utils::alignUp(size.width * config.bpp / 8, 256);
>> -
>> -	return std::make_tuple(stride, stride * size.height);
>> +	const PixelFormatInfo &info = PixelFormatInfo::info(outputFormat);
>> +	return std::make_tuple(info.stride(size.width, 0, 256), info.frameSize(size, 256));
>>   }
>>   
>>   uint32_t DebayerEGL::preferredInputStride(const PixelFormat &inputFormat, const Size &size)
>> diff --git a/src/libcamera/software_isp/debayer_egl.h b/src/libcamera/software_isp/debayer_egl.h
>> index d6223f3a9..e613639f5 100644
>> --- a/src/libcamera/software_isp/debayer_egl.h
>> +++ b/src/libcamera/software_isp/debayer_egl.h
>> @@ -63,7 +63,6 @@ public:
>>   
>>   private:
>>   	static int getInputConfig(PixelFormat inputFormat, DebayerInputConfig &config);
>> -	static int getOutputConfig(PixelFormat outputFormat, DebayerOutputConfig &config);
>>   	int initBayerShaders(PixelFormat inputFormat, PixelFormat outputFormat);
>>   	int getShaderVariableLocations();
>>   	void setShaderVariableValues(eGLImage &eGLImageIn, const DebayerParams &params);
Milan Zamazal July 21, 2026, 8:30 p.m. UTC | #3 
Robert Mader <robert.mader@collabora.com> writes:

> Hi Milan, thanks for the feedback!
>
> On 21.07.26 18:14, Milan Zamazal wrote:
>> Hi Robert,
>>
>> thank you for the cleanup.
>>
>> Robert Mader <robert.mader@collabora.com> writes:
>>
>>> Clean up some open-coded logic, making the code shorter, more generic
>>> and easier to follow.
>>>
>>> Signed-off-by: Robert Mader <robert.mader@collabora.com>
>>>
>>> ---
>>>
>>> Changes in v2:
>>>   - remove bpp docs to fix CI
>>> ---
>>>   src/libcamera/software_isp/debayer.cpp     |  3 ---
>>>   src/libcamera/software_isp/debayer.h       |  1 -
>>>   src/libcamera/software_isp/debayer_cpu.cpp | 29 +++-------------------
>>>   src/libcamera/software_isp/debayer_cpu.h   |  1 -
>>>   src/libcamera/software_isp/debayer_egl.cpp | 24 ++----------------
>>>   src/libcamera/software_isp/debayer_egl.h   |  1 -
>>>   6 files changed, 5 insertions(+), 54 deletions(-)
>>>
>>> diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp
>>> index 56446f55d..a4854e51b 100644
>>> --- a/src/libcamera/software_isp/debayer.cpp
>>> +++ b/src/libcamera/software_isp/debayer.cpp
>>> @@ -192,9 +192,6 @@ Debayer::~Debayer()
>>>    * Defines how the output of the debayer process is laid out in memory.
>>>    * It includes per-pixel size, stride, and total frame size.
>>>    *
>>> - * \var Debayer::DebayerOutputConfig::bpp
>>> - * Bytes used per pixel in the output format.
>>> - *
>>>    * \var Debayer::DebayerOutputConfig::stride
>>>    * Line stride in bytes for the output frame.
>>>    *
>>> diff --git a/src/libcamera/software_isp/debayer.h b/src/libcamera/software_isp/debayer.h
>>> index 28f1b857b..556852260 100644
>>> --- a/src/libcamera/software_isp/debayer.h
>>> +++ b/src/libcamera/software_isp/debayer.h
>>> @@ -69,7 +69,6 @@ public:
>>>   	};
>>>     	struct DebayerOutputConfig {
>>> -		unsigned int bpp;
>>>   		unsigned int stride;
>>>   		unsigned int frameSize;
>>>   	};
>>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
>>> index 49382b4c2..25e9830c8 100644
>>> --- a/src/libcamera/software_isp/debayer_cpu.cpp
>>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
>>> @@ -24,6 +24,7 @@
>>>     #include "libcamera/internal/bayer_format.h"
>>>   #include "libcamera/internal/camera_manager.h"
>>> +#include "libcamera/internal/formats.h"
>>>   #include "libcamera/internal/framebuffer.h"
>>>   #include "libcamera/internal/global_configuration.h"
>>>   #include "libcamera/internal/mapped_framebuffer.h"
>>> @@ -474,24 +475,6 @@ int DebayerCpu::getInputConfig(PixelFormat inputFormat, DebayerInputConfig &conf
>>>   	return -EINVAL;
>>>   }
>>>   -int DebayerCpu::getOutputConfig(PixelFormat outputFormat, DebayerOutputConfig &config)
>>> -{
>>> -	if (outputFormat == formats::RGB888 || outputFormat == formats::BGR888) {
>>> -		config.bpp = 24;
>>> -		return 0;
>>> -	}
>>> -
>>> -	if (outputFormat == formats::XRGB8888 || outputFormat == formats::ARGB8888 ||
>>> -	    outputFormat == formats::XBGR8888 || outputFormat == formats::ABGR8888) {
>>> -		config.bpp = 32;
>>> -		return 0;
>>> -	}
>>> -
>>> -	LOG(Debayer, Info)
>>> -		<< "Unsupported output format " << outputFormat.toString();
>>> -	return -EINVAL;
>> What to do about unsupported formats now?  I don't think this was
>> handled correctly in the original case.  But can we do better now?  Or
>> safely assume that anything that reaches debayering is a supported
>> format?
>
> First of all that would require a pretty broken pipeline handler - in simple.cpp we have
>
> config.outputFormats = swIsp_->formats(pixelFormat);
>
> ensuring that only supported formats are set for an output config (AFAICS).
>
> Secondly we'd still fail in DebayerCpu::setDebayerFunctions() (called in configure(), just like
> strideAndFrameSize()) and DebayerEGL::initBayerShaders() (called in start() - i.e. slightly later / not in
> configure()).
>
> So a broken pipeline handler would fail slightly later in the GPU path. If we want to be extra sure we
> could add a dedicated check in DebayerEGL::configure(), checking that outputCfg.pixelFormat is present in
> inputConfig_.outputFormats. I'm not fully convinced it's worth it, but it's small and easy to add if you
> prefer.

Thank you for clarification.  I think the current measures are
sufficient.

Reviewed-by: Milan Zamazal <mzamazal@redhat.com>

> Regards
>
> Robert
>
>>> -}
>>> -
>>>   /*
>>>    * Check for standard Bayer orders and set xShift_ and swap debayer0/1, so that
>>>    * a single pair of BGGR debayer functions can be used for all 4 standard orders.
>>> @@ -776,15 +759,9 @@ std::vector<PixelFormat> DebayerCpu::formats(PixelFormat inputFormat)
>>>   std::tuple<unsigned int, unsigned int>
>>>   DebayerCpu::strideAndFrameSize(const PixelFormat &outputFormat, const Size &size)
>>>   {
>>> -	DebayerCpu::DebayerOutputConfig config;
>>> -
>>> -	if (getOutputConfig(outputFormat, config) != 0)
>>> -		return std::make_tuple(0, 0);
>>> -
>>>   	/* round up to multiple of 8 for 64 bits alignment */
>>> -	unsigned int stride = (size.width * config.bpp / 8 + 7) & ~7;
>>> -
>>> -	return std::make_tuple(stride, stride * size.height);
>>> +	const PixelFormatInfo &info = PixelFormatInfo::info(outputFormat);
>>> +	return std::make_tuple(info.stride(size.width, 0, 8), info.frameSize(size, 8));
>>>   }
>>>     void DebayerCpuThread::setupInputMemcpy(const uint8_t *linePointers[])
>>> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h
>>> index 5281c65cb..2c88c9e1a 100644
>>> --- a/src/libcamera/software_isp/debayer_cpu.h
>>> +++ b/src/libcamera/software_isp/debayer_cpu.h
>>> @@ -121,7 +121,6 @@ private:
>>>   	void debayer12P_RGRG_BGR888(uint8_t *dst, const uint8_t *src[]);
>>>     	static int getInputConfig(PixelFormat inputFormat, DebayerInputConfig &config);
>>> -	static int getOutputConfig(PixelFormat outputFormat, DebayerOutputConfig &config);
>>>   	int setupStandardBayerOrder(BayerFormat::Order order);
>>>   	int setDebayerFunctions(PixelFormat inputFormat,
>>>   				PixelFormat outputFormat,
>>> diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp
>>> index 02651fe87..24faf480d 100644
>>> --- a/src/libcamera/software_isp/debayer_egl.cpp
>>> +++ b/src/libcamera/software_isp/debayer_egl.cpp
>>> @@ -96,20 +96,6 @@ int DebayerEGL::getInputConfig(PixelFormat inputFormat, DebayerInputConfig &conf
>>>   	return -EINVAL;
>>>   }
>>>   -int DebayerEGL::getOutputConfig(PixelFormat outputFormat, DebayerOutputConfig &config)
>>> -{
>>> -	if (outputFormat == formats::XRGB8888 || outputFormat == formats::ARGB8888 ||
>>> -	    outputFormat == formats::XBGR8888 || outputFormat == formats::ABGR8888) {
>>> -		config.bpp = 32;
>>> -		return 0;
>>> -	}
>>> -
>>> -	LOG(Debayer, Info)
>>> -		<< "Unsupported output format " << outputFormat;
>>> -
>>> -	return -EINVAL;
>>> -}
>>> -
>>>   int DebayerEGL::getShaderVariableLocations(void)
>>>   {
>>>   	attributeVertex_ = glGetAttribLocation(programId_, "vertexIn");
>>> @@ -374,15 +360,9 @@ std::vector<PixelFormat> DebayerEGL::formats(PixelFormat inputFormat)
>>>   std::tuple<unsigned int, unsigned int>
>>>   DebayerEGL::strideAndFrameSize(const PixelFormat &outputFormat, const Size &size)
>>>   {
>>> -	DebayerEGL::DebayerOutputConfig config;
>>> -
>>> -	if (getOutputConfig(outputFormat, config) != 0)
>>> -		return std::make_tuple(0, 0);
>>> -
>>>   	/* Align stride to 256 bytes as a generic GPU memory access alignment */
>>> -	unsigned int stride = libcamera::utils::alignUp(size.width * config.bpp / 8, 256);
>>> -
>>> -	return std::make_tuple(stride, stride * size.height);
>>> +	const PixelFormatInfo &info = PixelFormatInfo::info(outputFormat);
>>> +	return std::make_tuple(info.stride(size.width, 0, 256), info.frameSize(size, 256));
>>>   }
>>>     uint32_t DebayerEGL::preferredInputStride(const PixelFormat &inputFormat, const Size &size)
>>> diff --git a/src/libcamera/software_isp/debayer_egl.h b/src/libcamera/software_isp/debayer_egl.h
>>> index d6223f3a9..e613639f5 100644
>>> --- a/src/libcamera/software_isp/debayer_egl.h
>>> +++ b/src/libcamera/software_isp/debayer_egl.h
>>> @@ -63,7 +63,6 @@ public:
>>>     private:
>>>   	static int getInputConfig(PixelFormat inputFormat, DebayerInputConfig &config);
>>> -	static int getOutputConfig(PixelFormat outputFormat, DebayerOutputConfig &config);
>>>   	int initBayerShaders(PixelFormat inputFormat, PixelFormat outputFormat);
>>>   	int getShaderVariableLocations();
>>>   	void setShaderVariableValues(eGLImage &eGLImageIn, const DebayerParams &params);

Patch
diff mbox series 

diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp
index 56446f55d..a4854e51b 100644
--- a/src/libcamera/software_isp/debayer.cpp
+++ b/src/libcamera/software_isp/debayer.cpp
@@ -192,9 +192,6 @@  Debayer::~Debayer()
  * Defines how the output of the debayer process is laid out in memory.
  * It includes per-pixel size, stride, and total frame size.
  *
- * \var Debayer::DebayerOutputConfig::bpp
- * Bytes used per pixel in the output format.
- *
  * \var Debayer::DebayerOutputConfig::stride
  * Line stride in bytes for the output frame.
  *
diff --git a/src/libcamera/software_isp/debayer.h b/src/libcamera/software_isp/debayer.h
index 28f1b857b..556852260 100644
--- a/src/libcamera/software_isp/debayer.h
+++ b/src/libcamera/software_isp/debayer.h
@@ -69,7 +69,6 @@  public:
 	};
 
 	struct DebayerOutputConfig {
-		unsigned int bpp;
 		unsigned int stride;
 		unsigned int frameSize;
 	};
diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
index 49382b4c2..25e9830c8 100644
--- a/src/libcamera/software_isp/debayer_cpu.cpp
+++ b/src/libcamera/software_isp/debayer_cpu.cpp
@@ -24,6 +24,7 @@ 
 
 #include "libcamera/internal/bayer_format.h"
 #include "libcamera/internal/camera_manager.h"
+#include "libcamera/internal/formats.h"
 #include "libcamera/internal/framebuffer.h"
 #include "libcamera/internal/global_configuration.h"
 #include "libcamera/internal/mapped_framebuffer.h"
@@ -474,24 +475,6 @@  int DebayerCpu::getInputConfig(PixelFormat inputFormat, DebayerInputConfig &conf
 	return -EINVAL;
 }
 
-int DebayerCpu::getOutputConfig(PixelFormat outputFormat, DebayerOutputConfig &config)
-{
-	if (outputFormat == formats::RGB888 || outputFormat == formats::BGR888) {
-		config.bpp = 24;
-		return 0;
-	}
-
-	if (outputFormat == formats::XRGB8888 || outputFormat == formats::ARGB8888 ||
-	    outputFormat == formats::XBGR8888 || outputFormat == formats::ABGR8888) {
-		config.bpp = 32;
-		return 0;
-	}
-
-	LOG(Debayer, Info)
-		<< "Unsupported output format " << outputFormat.toString();
-	return -EINVAL;
-}
-
 /*
  * Check for standard Bayer orders and set xShift_ and swap debayer0/1, so that
  * a single pair of BGGR debayer functions can be used for all 4 standard orders.
@@ -776,15 +759,9 @@  std::vector<PixelFormat> DebayerCpu::formats(PixelFormat inputFormat)
 std::tuple<unsigned int, unsigned int>
 DebayerCpu::strideAndFrameSize(const PixelFormat &outputFormat, const Size &size)
 {
-	DebayerCpu::DebayerOutputConfig config;
-
-	if (getOutputConfig(outputFormat, config) != 0)
-		return std::make_tuple(0, 0);
-
 	/* round up to multiple of 8 for 64 bits alignment */
-	unsigned int stride = (size.width * config.bpp / 8 + 7) & ~7;
-
-	return std::make_tuple(stride, stride * size.height);
+	const PixelFormatInfo &info = PixelFormatInfo::info(outputFormat);
+	return std::make_tuple(info.stride(size.width, 0, 8), info.frameSize(size, 8));
 }
 
 void DebayerCpuThread::setupInputMemcpy(const uint8_t *linePointers[])
diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h
index 5281c65cb..2c88c9e1a 100644
--- a/src/libcamera/software_isp/debayer_cpu.h
+++ b/src/libcamera/software_isp/debayer_cpu.h
@@ -121,7 +121,6 @@  private:
 	void debayer12P_RGRG_BGR888(uint8_t *dst, const uint8_t *src[]);
 
 	static int getInputConfig(PixelFormat inputFormat, DebayerInputConfig &config);
-	static int getOutputConfig(PixelFormat outputFormat, DebayerOutputConfig &config);
 	int setupStandardBayerOrder(BayerFormat::Order order);
 	int setDebayerFunctions(PixelFormat inputFormat,
 				PixelFormat outputFormat,
diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp
index 02651fe87..24faf480d 100644
--- a/src/libcamera/software_isp/debayer_egl.cpp
+++ b/src/libcamera/software_isp/debayer_egl.cpp
@@ -96,20 +96,6 @@  int DebayerEGL::getInputConfig(PixelFormat inputFormat, DebayerInputConfig &conf
 	return -EINVAL;
 }
 
-int DebayerEGL::getOutputConfig(PixelFormat outputFormat, DebayerOutputConfig &config)
-{
-	if (outputFormat == formats::XRGB8888 || outputFormat == formats::ARGB8888 ||
-	    outputFormat == formats::XBGR8888 || outputFormat == formats::ABGR8888) {
-		config.bpp = 32;
-		return 0;
-	}
-
-	LOG(Debayer, Info)
-		<< "Unsupported output format " << outputFormat;
-
-	return -EINVAL;
-}
-
 int DebayerEGL::getShaderVariableLocations(void)
 {
 	attributeVertex_ = glGetAttribLocation(programId_, "vertexIn");
@@ -374,15 +360,9 @@  std::vector<PixelFormat> DebayerEGL::formats(PixelFormat inputFormat)
 std::tuple<unsigned int, unsigned int>
 DebayerEGL::strideAndFrameSize(const PixelFormat &outputFormat, const Size &size)
 {
-	DebayerEGL::DebayerOutputConfig config;
-
-	if (getOutputConfig(outputFormat, config) != 0)
-		return std::make_tuple(0, 0);
-
 	/* Align stride to 256 bytes as a generic GPU memory access alignment */
-	unsigned int stride = libcamera::utils::alignUp(size.width * config.bpp / 8, 256);
-
-	return std::make_tuple(stride, stride * size.height);
+	const PixelFormatInfo &info = PixelFormatInfo::info(outputFormat);
+	return std::make_tuple(info.stride(size.width, 0, 256), info.frameSize(size, 256));
 }
 
 uint32_t DebayerEGL::preferredInputStride(const PixelFormat &inputFormat, const Size &size)
diff --git a/src/libcamera/software_isp/debayer_egl.h b/src/libcamera/software_isp/debayer_egl.h
index d6223f3a9..e613639f5 100644
--- a/src/libcamera/software_isp/debayer_egl.h
+++ b/src/libcamera/software_isp/debayer_egl.h
@@ -63,7 +63,6 @@  public:
 
 private:
 	static int getInputConfig(PixelFormat inputFormat, DebayerInputConfig &config);
-	static int getOutputConfig(PixelFormat outputFormat, DebayerOutputConfig &config);
 	int initBayerShaders(PixelFormat inputFormat, PixelFormat outputFormat);
 	int getShaderVariableLocations();
 	void setShaderVariableValues(eGLImage &eGLImageIn, const DebayerParams &params);