[09/35] libcamera: software_isp: Move Bayer parans init from DebayerCpu to Debayer
diff mbox series

Message ID 20250611013245.133785-10-bryan.odonoghue@linaro.org
State New
Headers show
Series
  • Add GLES 2.0 GPUISP to libcamera
Related show

Commit Message

Bryan O'Donoghue June 11, 2025, 1:32 a.m. UTC
Move the initialisation of Bayer params and CCM to a new constructor in the
Debayer class.

Ensure we call the base class constructor from DebayerCpu's constructor in
the expected constructor order Debayer then DebayerCpu.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 src/libcamera/software_isp/debayer.cpp     | 11 +++++++++++
 src/libcamera/software_isp/debayer.h       |  1 +
 src/libcamera/software_isp/debayer_cpu.cpp | 10 +---------
 3 files changed, 13 insertions(+), 9 deletions(-)

Comments

Milan Zamazal June 16, 2025, 6:05 p.m. UTC | #1
Hi Bryan,

in the subject: s/parans/params/

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

> Move the initialisation of Bayer params and CCM to a new constructor in the
> Debayer class.
>
> Ensure we call the base class constructor from DebayerCpu's constructor in
> the expected constructor order Debayer then DebayerCpu.

Oh, is it otherwise by default?  How about Object's constructor then
(before and after)?

> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  src/libcamera/software_isp/debayer.cpp     | 11 +++++++++++
>  src/libcamera/software_isp/debayer.h       |  1 +
>  src/libcamera/software_isp/debayer_cpu.cpp | 10 +---------
>  3 files changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp
> index e9e18c48..29fdcbbf 100644
> --- a/src/libcamera/software_isp/debayer.cpp
> +++ b/src/libcamera/software_isp/debayer.cpp
> @@ -103,6 +103,17 @@ namespace libcamera {
>  
>  LOG_DEFINE_CATEGORY(Debayer)
>  
> +Debayer::Debayer()
> +{
> +	/* Initialize color lookup tables */
> +	for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {
> +		red_[i] = green_[i] = blue_[i] = i;
> +		redCcm_[i] = { static_cast<int16_t>(i), 0, 0 };
> +		greenCcm_[i] = { 0, static_cast<int16_t>(i), 0 };
> +		blueCcm_[i] = { 0, 0, static_cast<int16_t>(i) };
> +	}
> +}
> +
>  Debayer::~Debayer()
>  {
>  }
> diff --git a/src/libcamera/software_isp/debayer.h b/src/libcamera/software_isp/debayer.h
> index 45da822a..01b6e916 100644
> --- a/src/libcamera/software_isp/debayer.h
> +++ b/src/libcamera/software_isp/debayer.h
> @@ -32,6 +32,7 @@ LOG_DECLARE_CATEGORY(Debayer)
>  class Debayer : public Object
>  {
>  public:
> +	Debayer();
>  	virtual ~Debayer() = 0;
>  
>  	virtual int configure(const StreamConfiguration &inputCfg,
> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
> index 8d30bf4a..13db8a8c 100644
> --- a/src/libcamera/software_isp/debayer_cpu.cpp
> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
> @@ -40,7 +40,7 @@ namespace libcamera {
>   * \param[in] stats Pointer to the stats object to use
>   */
>  DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats)
> -	: stats_(std::move(stats))
> +	: Debayer(), stats_(std::move(stats))
>  {
>  	/*
>  	 * Reading from uncached buffers may be very slow.
> @@ -51,14 +51,6 @@ DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats)
>  	 * future.
>  	 */
>  	enableInputMemcpy_ = true;
> -
> -	/* Initialize color lookup tables */
> -	for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {
> -		red_[i] = green_[i] = blue_[i] = i;
> -		redCcm_[i] = { static_cast<int16_t>(i), 0, 0 };
> -		greenCcm_[i] = { 0, static_cast<int16_t>(i), 0 };
> -		blueCcm_[i] = { 0, 0, static_cast<int16_t>(i) };
> -	}
>  }
>  
>  DebayerCpu::~DebayerCpu() = default;
Barnabás Pőcze June 17, 2025, 11:37 a.m. UTC | #2
Hi

2025. 06. 16. 20:05 keltezéssel, Milan Zamazal írta:
> Hi Bryan,
> 
> in the subject: s/parans/params/
> 
> Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes:
> 
>> Move the initialisation of Bayer params and CCM to a new constructor in the
>> Debayer class.
>>
>> Ensure we call the base class constructor from DebayerCpu's constructor in
>> the expected constructor order Debayer then DebayerCpu.
> > Oh, is it otherwise by default?  How about Object's constructor then
> (before and after)?

The addition of the call to `Debayer()` should not be needed.
Is there a compiler warning or something else?


Regards,
Barnabás Pőcze


> 
>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> ---
>>   src/libcamera/software_isp/debayer.cpp     | 11 +++++++++++
>>   src/libcamera/software_isp/debayer.h       |  1 +
>>   src/libcamera/software_isp/debayer_cpu.cpp | 10 +---------
>>   3 files changed, 13 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp
>> index e9e18c48..29fdcbbf 100644
>> --- a/src/libcamera/software_isp/debayer.cpp
>> +++ b/src/libcamera/software_isp/debayer.cpp
>> @@ -103,6 +103,17 @@ namespace libcamera {
>>   
>>   LOG_DEFINE_CATEGORY(Debayer)
>>   
>> +Debayer::Debayer()
>> +{
>> +	/* Initialize color lookup tables */
>> +	for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {
>> +		red_[i] = green_[i] = blue_[i] = i;
>> +		redCcm_[i] = { static_cast<int16_t>(i), 0, 0 };
>> +		greenCcm_[i] = { 0, static_cast<int16_t>(i), 0 };
>> +		blueCcm_[i] = { 0, 0, static_cast<int16_t>(i) };
>> +	}
>> +}
>> +
>>   Debayer::~Debayer()
>>   {
>>   }
>> diff --git a/src/libcamera/software_isp/debayer.h b/src/libcamera/software_isp/debayer.h
>> index 45da822a..01b6e916 100644
>> --- a/src/libcamera/software_isp/debayer.h
>> +++ b/src/libcamera/software_isp/debayer.h
>> @@ -32,6 +32,7 @@ LOG_DECLARE_CATEGORY(Debayer)
>>   class Debayer : public Object
>>   {
>>   public:
>> +	Debayer();
>>   	virtual ~Debayer() = 0;
>>   
>>   	virtual int configure(const StreamConfiguration &inputCfg,
>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
>> index 8d30bf4a..13db8a8c 100644
>> --- a/src/libcamera/software_isp/debayer_cpu.cpp
>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
>> @@ -40,7 +40,7 @@ namespace libcamera {
>>    * \param[in] stats Pointer to the stats object to use
>>    */
>>   DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats)
>> -	: stats_(std::move(stats))
>> +	: Debayer(), stats_(std::move(stats))
>>   {
>>   	/*
>>   	 * Reading from uncached buffers may be very slow.
>> @@ -51,14 +51,6 @@ DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats)
>>   	 * future.
>>   	 */
>>   	enableInputMemcpy_ = true;
>> -
>> -	/* Initialize color lookup tables */
>> -	for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {
>> -		red_[i] = green_[i] = blue_[i] = i;
>> -		redCcm_[i] = { static_cast<int16_t>(i), 0, 0 };
>> -		greenCcm_[i] = { 0, static_cast<int16_t>(i), 0 };
>> -		blueCcm_[i] = { 0, 0, static_cast<int16_t>(i) };
>> -	}
>>   }
>>   
>>   DebayerCpu::~DebayerCpu() = default;
>
Bryan O'Donoghue June 17, 2025, 11:42 a.m. UTC | #3
On 17/06/2025 12:37, Barnabás Pőcze wrote:
>>>
>> > Oh, is it otherwise by default?  How about Object's constructor then
>> (before and after)?
> 
> The addition of the call to `Debayer()` should not be needed.
> Is there a compiler warning or something else?
> 
> 
> Regards,
> Barnabás Pőcze

I have no memory of why I added this commit.

You should only have to invoke a base class constructor if its a 
non-default constructor and I don't think that stats_(std::move) matters.

---
bod

Patch
diff mbox series

diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp
index e9e18c48..29fdcbbf 100644
--- a/src/libcamera/software_isp/debayer.cpp
+++ b/src/libcamera/software_isp/debayer.cpp
@@ -103,6 +103,17 @@  namespace libcamera {
 
 LOG_DEFINE_CATEGORY(Debayer)
 
+Debayer::Debayer()
+{
+	/* Initialize color lookup tables */
+	for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {
+		red_[i] = green_[i] = blue_[i] = i;
+		redCcm_[i] = { static_cast<int16_t>(i), 0, 0 };
+		greenCcm_[i] = { 0, static_cast<int16_t>(i), 0 };
+		blueCcm_[i] = { 0, 0, static_cast<int16_t>(i) };
+	}
+}
+
 Debayer::~Debayer()
 {
 }
diff --git a/src/libcamera/software_isp/debayer.h b/src/libcamera/software_isp/debayer.h
index 45da822a..01b6e916 100644
--- a/src/libcamera/software_isp/debayer.h
+++ b/src/libcamera/software_isp/debayer.h
@@ -32,6 +32,7 @@  LOG_DECLARE_CATEGORY(Debayer)
 class Debayer : public Object
 {
 public:
+	Debayer();
 	virtual ~Debayer() = 0;
 
 	virtual int configure(const StreamConfiguration &inputCfg,
diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
index 8d30bf4a..13db8a8c 100644
--- a/src/libcamera/software_isp/debayer_cpu.cpp
+++ b/src/libcamera/software_isp/debayer_cpu.cpp
@@ -40,7 +40,7 @@  namespace libcamera {
  * \param[in] stats Pointer to the stats object to use
  */
 DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats)
-	: stats_(std::move(stats))
+	: Debayer(), stats_(std::move(stats))
 {
 	/*
 	 * Reading from uncached buffers may be very slow.
@@ -51,14 +51,6 @@  DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats)
 	 * future.
 	 */
 	enableInputMemcpy_ = true;
-
-	/* Initialize color lookup tables */
-	for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {
-		red_[i] = green_[i] = blue_[i] = i;
-		redCcm_[i] = { static_cast<int16_t>(i), 0, 0 };
-		greenCcm_[i] = { 0, static_cast<int16_t>(i), 0 };
-		blueCcm_[i] = { 0, 0, static_cast<int16_t>(i) };
-	}
 }
 
 DebayerCpu::~DebayerCpu() = default;