[08/35] libcamera: software_isp: Move useful items from DebayerCpu to Debayer base class

23514 diff mbox series

The DebayerCpu class has a number of variables, embedded structures and
methods which are useful to DebayerGpu implementation.

Move relevant variables and methods to base class.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 src/libcamera/software_isp/debayer.h     | 33 ++++++++++++++++++++++-
 src/libcamera/software_isp/debayer_cpu.h | 34 +-----------------------
 2 files changed, 33 insertions(+), 34 deletions(-)

Hi Bryan,

Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes:

> The DebayerCpu class has a number of variables, embedded structures and
> methods which are useful to DebayerGpu implementation.
>
> Move relevant variables and methods to base class.
>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  src/libcamera/software_isp/debayer.h     | 33 ++++++++++++++++++++++-
>  src/libcamera/software_isp/debayer_cpu.h | 34 +-----------------------
>  2 files changed, 33 insertions(+), 34 deletions(-)
>
> diff --git a/src/libcamera/software_isp/debayer.h b/src/libcamera/software_isp/debayer.h
> index ba033d44..45da822a 100644
> --- a/src/libcamera/software_isp/debayer.h
> +++ b/src/libcamera/software_isp/debayer.h
> @@ -14,11 +14,13 @@
>  #include <stdint.h>
>  
>  #include <libcamera/base/log.h>
> +#include <libcamera/base/object.h>
>  #include <libcamera/base/signal.h>
>  
>  #include <libcamera/geometry.h>
>  #include <libcamera/stream.h>
>  
> +#include "libcamera/internal/software_isp/benchmark.h"
>  #include "libcamera/internal/software_isp/debayer_params.h"
>  
>  namespace libcamera {
> @@ -27,7 +29,7 @@ class FrameBuffer;
>  
>  LOG_DECLARE_CATEGORY(Debayer)
>  
> -class Debayer
> +class Debayer : public Object

It would be useful to explain in the commit message which of the moved
stuff requires inheriting Object here.

>  {
>  public:
>  	virtual ~Debayer() = 0;
> @@ -45,9 +47,38 @@ public:
>  
>  	virtual SizeRange sizes(PixelFormat inputFormat, const Size &inputSize) = 0;
>  
> +	virtual const SharedFD &getStatsFD() = 0;
> +
> +	unsigned int frameSize() { return outputConfig_.frameSize; }
> +
>  	Signal<FrameBuffer *> inputBufferReady;
>  	Signal<FrameBuffer *> outputBufferReady;
>  
> +	struct DebayerInputConfig {
> +		Size patternSize;
> +		unsigned int bpp; /* Memory used per pixel, not precision */
> +		unsigned int stride;
> +		std::vector<PixelFormat> outputFormats;
> +	};
> +
> +	struct DebayerOutputConfig {
> +		unsigned int bpp; /* Memory used per pixel, not precision */
> +		unsigned int stride;
> +		unsigned int frameSize;
> +	};
> +
> +	DebayerInputConfig inputConfig_;
> +	DebayerOutputConfig outputConfig_;

OK.

> +	DebayerParams::LookupTable red_;
> +	DebayerParams::LookupTable green_;
> +	DebayerParams::LookupTable blue_;
> +	DebayerParams::CcmLookupTable redCcm_;
> +	DebayerParams::CcmLookupTable greenCcm_;
> +	DebayerParams::CcmLookupTable blueCcm_;

This also deserves an explanation why it is going to be used in GPU
debayering.

> +	DebayerParams::LookupTable gammaLut_;

IIRC we agreed at some sync this may be better than computing the gamma
on GPU directly but I already don't remember why exactly -- please add
the related info to the commit message.

> +	bool swapRedBlueGains_;

This is related to the lookup tables above and should be explained or
dropped together with them.  Preferably to be avoided and handled via
CCM if needed and possible.

> +	Benchmark bench_;

OK.

> +
>  private:
>  	virtual Size patternSize(PixelFormat inputFormat) = 0;
>  };
> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h
> index 182607cd..0b4b16e1 100644
> --- a/src/libcamera/software_isp/debayer_cpu.h
> +++ b/src/libcamera/software_isp/debayer_cpu.h
> @@ -17,7 +17,6 @@
>  
>  #include <libcamera/base/object.h>
>  
> -#include "libcamera/internal/software_isp/benchmark.h"
>  #include "libcamera/internal/bayer_format.h"
>  #include "libcamera/internal/software_isp/swstats_cpu.h"
>  
> @@ -25,7 +24,7 @@
>  
>  namespace libcamera {
>  
> -class DebayerCpu : public Debayer, public Object
> +class DebayerCpu : public Debayer
>  {
>  public:
>  	DebayerCpu(std::unique_ptr<SwStatsCpu> stats);
> @@ -48,13 +47,6 @@ public:
>  	 */
>  	const SharedFD &getStatsFD() { return stats_->getStatsFD(); }
>  
> -	/**
> -	 * \brief Get the output frame size
> -	 *
> -	 * \return The output frame size
> -	 */
> -	unsigned int frameSize() { return outputConfig_.frameSize; }
> -
>  private:
>  	/**
>  	 * \brief Called to debayer 1 line of Bayer input data to output format
> @@ -111,19 +103,6 @@ private:
>  	template<bool addAlphaByte, bool ccmEnabled>
>  	void debayer10P_RGRG_BGR888(uint8_t *dst, const uint8_t *src[]);
>  
> -	struct DebayerInputConfig {
> -		Size patternSize;
> -		unsigned int bpp; /* Memory used per pixel, not precision */
> -		unsigned int stride;
> -		std::vector<PixelFormat> outputFormats;
> -	};
> -
> -	struct DebayerOutputConfig {
> -		unsigned int bpp; /* Memory used per pixel, not precision */
> -		unsigned int stride;
> -		unsigned int frameSize;
> -	};
> -
>  	int getInputConfig(PixelFormat inputFormat, DebayerInputConfig &config);
>  	int getOutputConfig(PixelFormat outputFormat, DebayerOutputConfig &config);
>  	int setupStandardBayerOrder(BayerFormat::Order order);
> @@ -139,20 +118,11 @@ private:
>  	/* Max. supported Bayer pattern height is 4, debayering this requires 5 lines */
>  	static constexpr unsigned int kMaxLineBuffers = 5;
>  
> -	DebayerParams::LookupTable red_;
> -	DebayerParams::LookupTable green_;
> -	DebayerParams::LookupTable blue_;
> -	DebayerParams::CcmLookupTable redCcm_;
> -	DebayerParams::CcmLookupTable greenCcm_;
> -	DebayerParams::CcmLookupTable blueCcm_;
> -	DebayerParams::LookupTable gammaLut_;
>  	debayerFn debayer0_;
>  	debayerFn debayer1_;
>  	debayerFn debayer2_;
>  	debayerFn debayer3_;
>  	Rectangle window_;
> -	DebayerInputConfig inputConfig_;
> -	DebayerOutputConfig outputConfig_;
>  	std::unique_ptr<SwStatsCpu> stats_;
>  	std::vector<uint8_t> lineBuffers_[kMaxLineBuffers];
>  	unsigned int lineBufferLength_;
> @@ -160,8 +130,6 @@ private:
>  	unsigned int lineBufferIndex_;
>  	unsigned int xShift_; /* Offset of 0/1 applied to window_.x */
>  	bool enableInputMemcpy_;
> -	bool swapRedBlueGains_;
> -	Benchmark bench_;
>  };
>  
>  } /* namespace libcamera */