[RFC,5/7] libcamera: ipa: simple: Introduce a general correction matrix
diff mbox series

Message ID 20251112082715.17823-6-mzamazal@redhat.com
State New
Headers show
Series
  • Simple pipeline IPA cleanup
Related show

Commit Message

Milan Zamazal Nov. 12, 2025, 8:27 a.m. UTC 
Let's introduce IPAActiveState::correctionMatrix that is separate from
IPAActiveState::ccm and represents the overall correction matrix, not
only the sensor colour correction matrix.

IPAActiveState::ccm still includes everything; this is changed in the
followup patch.

Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
---
 src/ipa/simple/algorithms/ccm.cpp | 1 +
 src/ipa/simple/algorithms/lut.cpp | 2 +-
 src/ipa/simple/ipa_context.h      | 1 +
 3 files changed, 3 insertions(+), 1 deletion(-)

Comments

Kieran Bingham Nov. 12, 2025, 9:50 a.m. UTC | #1 
Quoting Milan Zamazal (2025-11-12 08:27:13)
> Let's introduce IPAActiveState::correctionMatrix that is separate from
> IPAActiveState::ccm and represents the overall correction matrix, not
> only the sensor colour correction matrix.
> 
> IPAActiveState::ccm still includes everything; this is changed in the
> followup patch.

I think this is answering my previous questions :-)

Bikeshedding, I wonder if this is the 'globalMatrix' or
'combinedMatrix', but correctionMatrix could be fine too.


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


> 
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>  src/ipa/simple/algorithms/ccm.cpp | 1 +
>  src/ipa/simple/algorithms/lut.cpp | 2 +-
>  src/ipa/simple/ipa_context.h      | 1 +
>  3 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp
> index e05e5bc28..3e9528a91 100644
> --- a/src/ipa/simple/algorithms/ccm.cpp
> +++ b/src/ipa/simple/algorithms/ccm.cpp
> @@ -104,6 +104,7 @@ void Ccm::prepare(IPAContext &context, const uint32_t frame,
>         if (saturation)
>                 applySaturation(ccm, saturation.value());
>  
> +       context.activeState.correctionMatrix = ccm;
>         context.activeState.ccm = ccm;
>         frameContext.saturation = saturation;
>         context.activeState.matrixChanged = true;
> diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp
> index b27491042..8751bb31c 100644
> --- a/src/ipa/simple/algorithms/lut.cpp
> +++ b/src/ipa/simple/algorithms/lut.cpp
> @@ -123,7 +123,7 @@ void Lut::prepare(IPAContext &context,
>                 Matrix<float, 3, 3> gainMatrix = { { gains.r(), 0, 0,
>                                                      0, gains.g(), 0,
>                                                      0, 0, gains.b() } };
> -               auto matrix = context.activeState.ccm * gainMatrix;
> +               auto matrix = context.activeState.correctionMatrix * gainMatrix;
>                 auto &red = params->redCcm;
>                 auto &green = params->greenCcm;
>                 auto &blue = params->blueCcm;
> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
> index b6b3cbfed..14680c726 100644
> --- a/src/ipa/simple/ipa_context.h
> +++ b/src/ipa/simple/ipa_context.h
> @@ -61,6 +61,7 @@ struct IPAActiveState {
>         } gamma;
>  
>         Matrix<float, 3, 3> ccm;
> +       Matrix<float, 3, 3> correctionMatrix;
>         bool matrixChanged = false;
>  
>         struct {
> -- 
> 2.51.1
>
Milan Zamazal Nov. 13, 2025, 2:13 p.m. UTC | #2 
Hi Kieran,

thank you for the reviews.

Kieran Bingham <kieran.bingham@ideasonboard.com> writes:

> Quoting Milan Zamazal (2025-11-12 08:27:13)
>> Let's introduce IPAActiveState::correctionMatrix that is separate from
>> IPAActiveState::ccm and represents the overall correction matrix, not
>
>> only the sensor colour correction matrix.
>> 
>> IPAActiveState::ccm still includes everything; this is changed in the
>> followup patch.
>
> I think this is answering my previous questions :-)

OK, I'll add a clarification note to the previous commit messages.

> Bikeshedding, I wonder if this is the 'globalMatrix' or
> 'combinedMatrix', but correctionMatrix could be fine too.

I like combinedMatrix, this is more distinguishing, I'll rename it.

> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
>
>> 
>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>> ---
>>  src/ipa/simple/algorithms/ccm.cpp | 1 +
>>  src/ipa/simple/algorithms/lut.cpp | 2 +-
>>  src/ipa/simple/ipa_context.h      | 1 +
>>  3 files changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp
>> index e05e5bc28..3e9528a91 100644
>> --- a/src/ipa/simple/algorithms/ccm.cpp
>> +++ b/src/ipa/simple/algorithms/ccm.cpp
>> @@ -104,6 +104,7 @@ void Ccm::prepare(IPAContext &context, const uint32_t frame,
>>         if (saturation)
>>                 applySaturation(ccm, saturation.value());
>>  
>> +       context.activeState.correctionMatrix = ccm;
>>         context.activeState.ccm = ccm;
>>         frameContext.saturation = saturation;
>>         context.activeState.matrixChanged = true;
>> diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp
>> index b27491042..8751bb31c 100644
>> --- a/src/ipa/simple/algorithms/lut.cpp
>> +++ b/src/ipa/simple/algorithms/lut.cpp
>> @@ -123,7 +123,7 @@ void Lut::prepare(IPAContext &context,
>>                 Matrix<float, 3, 3> gainMatrix = { { gains.r(), 0, 0,
>>                                                      0, gains.g(), 0,
>>                                                      0, 0, gains.b() } };
>> -               auto matrix = context.activeState.ccm * gainMatrix;
>> +               auto matrix = context.activeState.correctionMatrix * gainMatrix;
>>                 auto &red = params->redCcm;
>>                 auto &green = params->greenCcm;
>>                 auto &blue = params->blueCcm;
>> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
>> index b6b3cbfed..14680c726 100644
>> --- a/src/ipa/simple/ipa_context.h
>> +++ b/src/ipa/simple/ipa_context.h
>> @@ -61,6 +61,7 @@ struct IPAActiveState {
>>         } gamma;
>>  
>>         Matrix<float, 3, 3> ccm;
>> +       Matrix<float, 3, 3> correctionMatrix;
>>         bool matrixChanged = false;
>>  
>>         struct {
>> -- 
>> 2.51.1
>>
Barnabás Pőcze Nov. 17, 2025, 11:09 a.m. UTC | #3 
Hi

2025. 11. 12. 9:27 keltezéssel, Milan Zamazal írta:
> Let's introduce IPAActiveState::correctionMatrix that is separate from
> IPAActiveState::ccm and represents the overall correction matrix, not
> only the sensor colour correction matrix.
> 
> IPAActiveState::ccm still includes everything; this is changed in the
> followup patch.
> 
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>   src/ipa/simple/algorithms/ccm.cpp | 1 +
>   src/ipa/simple/algorithms/lut.cpp | 2 +-
>   src/ipa/simple/ipa_context.h      | 1 +
>   3 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp
> index e05e5bc28..3e9528a91 100644
> --- a/src/ipa/simple/algorithms/ccm.cpp
> +++ b/src/ipa/simple/algorithms/ccm.cpp
> @@ -104,6 +104,7 @@ void Ccm::prepare(IPAContext &context, const uint32_t frame,
>   	if (saturation)
>   		applySaturation(ccm, saturation.value());
>   
> +	context.activeState.correctionMatrix = ccm;
>   	context.activeState.ccm = ccm;
>   	frameContext.saturation = saturation;
>   	context.activeState.matrixChanged = true;
> diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp
> index b27491042..8751bb31c 100644
> --- a/src/ipa/simple/algorithms/lut.cpp
> +++ b/src/ipa/simple/algorithms/lut.cpp
> @@ -123,7 +123,7 @@ void Lut::prepare(IPAContext &context,
>   		Matrix<float, 3, 3> gainMatrix = { { gains.r(), 0, 0,
>   						     0, gains.g(), 0,
>   						     0, 0, gains.b() } };
> -		auto matrix = context.activeState.ccm * gainMatrix;
> +		auto matrix = context.activeState.correctionMatrix * gainMatrix;
>   		auto &red = params->redCcm;
>   		auto &green = params->greenCcm;
>   		auto &blue = params->blueCcm;
> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
> index b6b3cbfed..14680c726 100644
> --- a/src/ipa/simple/ipa_context.h
> +++ b/src/ipa/simple/ipa_context.h
> @@ -61,6 +61,7 @@ struct IPAActiveState {
>   	} gamma;
>   
>   	Matrix<float, 3, 3> ccm;
> +	Matrix<float, 3, 3> correctionMatrix;

I have looked at the other changes. And it is not entirely clear to me what the goal is.
It doesn't seem to be used too much. And for example, here is `context.activeState.correctionMatrix = ccm`;
I would've expected that it gets initialized to the identity matrix at the beginning of
each frame(?), and then simply gets multiplied by each color transformation, and at the
end it gets fed into the debayering parts.

But that doesn't quite appear to be what is implemented by the end of this series.
Am I misunderstanding something here?


Regards,
Barnabás Pőcze



>   	bool matrixChanged = false;
>   
>   	struct {
Milan Zamazal Nov. 18, 2025, 11:18 a.m. UTC | #4 
Hi Barnabás,

thank you for reviews.

Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes:

> Hi
>
> 2025. 11. 12. 9:27 keltezéssel, Milan Zamazal írta:
>> Let's introduce IPAActiveState::correctionMatrix that is separate from
>> IPAActiveState::ccm and represents the overall correction matrix, not
>> only the sensor colour correction matrix.
>> IPAActiveState::ccm still includes everything; this is changed in the
>> followup patch.
>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>> ---
>>   src/ipa/simple/algorithms/ccm.cpp | 1 +
>>   src/ipa/simple/algorithms/lut.cpp | 2 +-
>>   src/ipa/simple/ipa_context.h      | 1 +
>>   3 files changed, 3 insertions(+), 1 deletion(-)
>> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp
>> index e05e5bc28..3e9528a91 100644
>> --- a/src/ipa/simple/algorithms/ccm.cpp
>> +++ b/src/ipa/simple/algorithms/ccm.cpp
>> @@ -104,6 +104,7 @@ void Ccm::prepare(IPAContext &context, const uint32_t frame,
>>   	if (saturation)
>>   		applySaturation(ccm, saturation.value());
>>   +	context.activeState.correctionMatrix = ccm;
>>   	context.activeState.ccm = ccm;
>>   	frameContext.saturation = saturation;
>>   	context.activeState.matrixChanged = true;
>> diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp
>> index b27491042..8751bb31c 100644
>> --- a/src/ipa/simple/algorithms/lut.cpp
>> +++ b/src/ipa/simple/algorithms/lut.cpp
>> @@ -123,7 +123,7 @@ void Lut::prepare(IPAContext &context,
>>   		Matrix<float, 3, 3> gainMatrix = { { gains.r(), 0, 0,
>>   						     0, gains.g(), 0,
>>   						     0, 0, gains.b() } };
>> -		auto matrix = context.activeState.ccm * gainMatrix;
>> +		auto matrix = context.activeState.correctionMatrix * gainMatrix;
>>   		auto &red = params->redCcm;
>>   		auto &green = params->greenCcm;
>>   		auto &blue = params->blueCcm;
>> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
>> index b6b3cbfed..14680c726 100644
>> --- a/src/ipa/simple/ipa_context.h
>> +++ b/src/ipa/simple/ipa_context.h
>> @@ -61,6 +61,7 @@ struct IPAActiveState {
>>   	} gamma;
>>     	Matrix<float, 3, 3> ccm;
>> +	Matrix<float, 3, 3> correctionMatrix;
>
> I have looked at the other changes. And it is not entirely clear to me what the goal is.
> It doesn't seem to be used too much. And for example, here is `context.activeState.correctionMatrix = ccm`;
> I would've expected that it gets initialized to the identity matrix at the beginning of
> each frame(?), and then simply gets multiplied by each color transformation, and at the
> end it gets fed into the debayering parts.
>
> But that doesn't quite appear to be what is implemented by the end of this series.
> Am I misunderstanding something here?

It should be as you say.  I'll try to do something about it in v2.

> Regards,
> Barnabás Pőcze
>
>
>
>>   	bool matrixChanged = false;
>>     	struct {

Patch
diff mbox series 

diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp
index e05e5bc28..3e9528a91 100644
--- a/src/ipa/simple/algorithms/ccm.cpp
+++ b/src/ipa/simple/algorithms/ccm.cpp
@@ -104,6 +104,7 @@  void Ccm::prepare(IPAContext &context, const uint32_t frame,
 	if (saturation)
 		applySaturation(ccm, saturation.value());
 
+	context.activeState.correctionMatrix = ccm;
 	context.activeState.ccm = ccm;
 	frameContext.saturation = saturation;
 	context.activeState.matrixChanged = true;
diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp
index b27491042..8751bb31c 100644
--- a/src/ipa/simple/algorithms/lut.cpp
+++ b/src/ipa/simple/algorithms/lut.cpp
@@ -123,7 +123,7 @@  void Lut::prepare(IPAContext &context,
 		Matrix<float, 3, 3> gainMatrix = { { gains.r(), 0, 0,
 						     0, gains.g(), 0,
 						     0, 0, gains.b() } };
-		auto matrix = context.activeState.ccm * gainMatrix;
+		auto matrix = context.activeState.correctionMatrix * gainMatrix;
 		auto &red = params->redCcm;
 		auto &green = params->greenCcm;
 		auto &blue = params->blueCcm;
diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
index b6b3cbfed..14680c726 100644
--- a/src/ipa/simple/ipa_context.h
+++ b/src/ipa/simple/ipa_context.h
@@ -61,6 +61,7 @@  struct IPAActiveState {
 	} gamma;
 
 	Matrix<float, 3, 3> ccm;
+	Matrix<float, 3, 3> correctionMatrix;
 	bool matrixChanged = false;
 
 	struct {