[5/5] libcamera: software_isp: Remove DebayerParams::kGain10
diff mbox series

Message ID 20240423182000.1527425-6-mzamazal@redhat.com
State Superseded
Headers show
Series
  • Software ISP levels cleanup
Related show

Commit Message

Milan Zamazal April 23, 2024, 6:20 p.m. UTC
The constant is used in a single place internally and doesn't belong to
DebayerParams anymore.  Let's use 256 directly.

Completes software ISP TODO #4.

Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
---
 .../internal/software_isp/debayer_params.h          |  1 -
 src/ipa/simple/soft_simple.cpp                      |  3 +--
 src/libcamera/software_isp/TODO                     | 13 -------------
 src/libcamera/software_isp/debayer.cpp              |  5 -----
 4 files changed, 1 insertion(+), 21 deletions(-)

Comments

Laurent Pinchart April 28, 2024, 11:12 p.m. UTC | #1
Hi Milan,

Thank you for the patch.

On Tue, Apr 23, 2024 at 08:20:00PM +0200, Milan Zamazal wrote:
> The constant is used in a single place internally and doesn't belong to
> DebayerParams anymore.  Let's use 256 directly.
> 
> Completes software ISP TODO #4.

The change doesn't address the TODO item.

> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>  .../internal/software_isp/debayer_params.h          |  1 -
>  src/ipa/simple/soft_simple.cpp                      |  3 +--
>  src/libcamera/software_isp/TODO                     | 13 -------------
>  src/libcamera/software_isp/debayer.cpp              |  5 -----
>  4 files changed, 1 insertion(+), 21 deletions(-)
> 
> diff --git a/include/libcamera/internal/software_isp/debayer_params.h b/include/libcamera/internal/software_isp/debayer_params.h
> index 09f4ff00..6aaa7d4c 100644
> --- a/include/libcamera/internal/software_isp/debayer_params.h
> +++ b/include/libcamera/internal/software_isp/debayer_params.h
> @@ -16,7 +16,6 @@
>  namespace libcamera {
>  
>  struct DebayerParams {
> -	static constexpr unsigned int kGain10 = 256;
>  	static constexpr unsigned int kRGBLookupSize = 256;
>  
>  	using ColorLookupTable = std::array<uint8_t, kRGBLookupSize>;
> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
> index b2f4d308..5298836c 100644
> --- a/src/ipa/simple/soft_simple.cpp
> +++ b/src/ipa/simple/soft_simple.cpp
> @@ -300,8 +300,7 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)
>  
>  	for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {
>  		constexpr unsigned int div =
> -			DebayerParams::kRGBLookupSize * DebayerParams::kGain10 /
> -			kGammaLookupSize;
> +			DebayerParams::kRGBLookupSize * 256 / kGammaLookupSize;
>  		unsigned int idx;
>  
>  		/* Apply gamma after gain! */
> diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO
> index fcb02588..f8f00139 100644
> --- a/src/libcamera/software_isp/TODO
> +++ b/src/libcamera/software_isp/TODO
> @@ -72,19 +72,6 @@ stats in hardware, such as the i.MX7), but please keep it on your radar.
>  
>  ---
>  
> -4. Hide internal representation of gains from callers
> -
> -> struct DebayerParams {
> -> 	static constexpr unsigned int kGain10 = 256;
> -
> -Forcing the caller to deal with the internal representation of gains
> -isn't nice, especially given that it precludes implementing gains of
> -different precisions in different backend. Wouldn't it be better to pass
> -the values as floating point numbers, and convert them to the internal
> -representation in the implementation of process() before using them ?
> -
> ----
> -
>  5. Store ISP parameters in per-frame buffers
>  
>  > /**
> diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp
> index ac438a33..548c2ccd 100644
> --- a/src/libcamera/software_isp/debayer.cpp
> +++ b/src/libcamera/software_isp/debayer.cpp
> @@ -18,11 +18,6 @@ namespace libcamera {
>   * \brief Struct to hold the debayer parameters.
>   */
>  
> -/**
> - * \var DebayerParams::kGain10
> - * \brief const value for 1.0 gain
> - */
> -
>  /**
>   * \var DebayerParams::kRGBLookupSize
>   * \brief Size of a color lookup table
Milan Zamazal April 29, 2024, 8:44 a.m. UTC | #2
Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:

> Hi Milan,
>
> Thank you for the patch.
>
> On Tue, Apr 23, 2024 at 08:20:00PM +0200, Milan Zamazal wrote:
>> The constant is used in a single place internally and doesn't belong to
>> DebayerParams anymore.  Let's use 256 directly.
>> 
>> Completes software ISP TODO #4.
>
> The change doesn't address the TODO item.

Hi Laurent,

so is the remaining step to change the item type of ColorLookupTable's from
uint8_t (0..255) to float (0.0..1.0) and converting the tables just before
debayering to uint8_t for the sake of separation?  Anything else?

Thanks,
Milan

>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>> ---
>>  .../internal/software_isp/debayer_params.h          |  1 -
>>  src/ipa/simple/soft_simple.cpp                      |  3 +--
>>  src/libcamera/software_isp/TODO                     | 13 -------------
>>  src/libcamera/software_isp/debayer.cpp              |  5 -----
>>  4 files changed, 1 insertion(+), 21 deletions(-)
>> 
>> diff --git a/include/libcamera/internal/software_isp/debayer_params.h b/include/libcamera/internal/software_isp/debayer_params.h
>> index 09f4ff00..6aaa7d4c 100644
>> --- a/include/libcamera/internal/software_isp/debayer_params.h
>> +++ b/include/libcamera/internal/software_isp/debayer_params.h
>> @@ -16,7 +16,6 @@
>>  namespace libcamera {
>>  
>>  struct DebayerParams {
>> -	static constexpr unsigned int kGain10 = 256;
>>  	static constexpr unsigned int kRGBLookupSize = 256;
>>  
>>  	using ColorLookupTable = std::array<uint8_t, kRGBLookupSize>;
>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
>> index b2f4d308..5298836c 100644
>> --- a/src/ipa/simple/soft_simple.cpp
>> +++ b/src/ipa/simple/soft_simple.cpp
>> @@ -300,8 +300,7 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)
>>  
>>  	for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {
>>  		constexpr unsigned int div =
>> -			DebayerParams::kRGBLookupSize * DebayerParams::kGain10 /
>> -			kGammaLookupSize;
>> +			DebayerParams::kRGBLookupSize * 256 / kGammaLookupSize;
>>  		unsigned int idx;
>>  
>>  		/* Apply gamma after gain! */
>> diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO
>> index fcb02588..f8f00139 100644
>> --- a/src/libcamera/software_isp/TODO
>> +++ b/src/libcamera/software_isp/TODO
>> @@ -72,19 +72,6 @@ stats in hardware, such as the i.MX7), but please keep it on your radar.
>>  
>>  ---
>>  
>> -4. Hide internal representation of gains from callers
>> -
>> -> struct DebayerParams {
>> -> 	static constexpr unsigned int kGain10 = 256;
>> -
>> -Forcing the caller to deal with the internal representation of gains
>> -isn't nice, especially given that it precludes implementing gains of
>> -different precisions in different backend. Wouldn't it be better to pass
>> -the values as floating point numbers, and convert them to the internal
>> -representation in the implementation of process() before using them ?
>> -
>> ----
>> -
>>  5. Store ISP parameters in per-frame buffers
>>  
>>  > /**
>> diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp
>> index ac438a33..548c2ccd 100644
>> --- a/src/libcamera/software_isp/debayer.cpp
>> +++ b/src/libcamera/software_isp/debayer.cpp
>> @@ -18,11 +18,6 @@ namespace libcamera {
>>   * \brief Struct to hold the debayer parameters.
>>   */
>>  
>> -/**
>> - * \var DebayerParams::kGain10
>> - * \brief const value for 1.0 gain
>> - */
>> -
>>  /**
>>   * \var DebayerParams::kRGBLookupSize
>>   * \brief Size of a color lookup table

Patch
diff mbox series

diff --git a/include/libcamera/internal/software_isp/debayer_params.h b/include/libcamera/internal/software_isp/debayer_params.h
index 09f4ff00..6aaa7d4c 100644
--- a/include/libcamera/internal/software_isp/debayer_params.h
+++ b/include/libcamera/internal/software_isp/debayer_params.h
@@ -16,7 +16,6 @@ 
 namespace libcamera {
 
 struct DebayerParams {
-	static constexpr unsigned int kGain10 = 256;
 	static constexpr unsigned int kRGBLookupSize = 256;
 
 	using ColorLookupTable = std::array<uint8_t, kRGBLookupSize>;
diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
index b2f4d308..5298836c 100644
--- a/src/ipa/simple/soft_simple.cpp
+++ b/src/ipa/simple/soft_simple.cpp
@@ -300,8 +300,7 @@  void IPASoftSimple::processStats(const ControlList &sensorControls)
 
 	for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {
 		constexpr unsigned int div =
-			DebayerParams::kRGBLookupSize * DebayerParams::kGain10 /
-			kGammaLookupSize;
+			DebayerParams::kRGBLookupSize * 256 / kGammaLookupSize;
 		unsigned int idx;
 
 		/* Apply gamma after gain! */
diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO
index fcb02588..f8f00139 100644
--- a/src/libcamera/software_isp/TODO
+++ b/src/libcamera/software_isp/TODO
@@ -72,19 +72,6 @@  stats in hardware, such as the i.MX7), but please keep it on your radar.
 
 ---
 
-4. Hide internal representation of gains from callers
-
-> struct DebayerParams {
-> 	static constexpr unsigned int kGain10 = 256;
-
-Forcing the caller to deal with the internal representation of gains
-isn't nice, especially given that it precludes implementing gains of
-different precisions in different backend. Wouldn't it be better to pass
-the values as floating point numbers, and convert them to the internal
-representation in the implementation of process() before using them ?
-
----
-
 5. Store ISP parameters in per-frame buffers
 
 > /**
diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp
index ac438a33..548c2ccd 100644
--- a/src/libcamera/software_isp/debayer.cpp
+++ b/src/libcamera/software_isp/debayer.cpp
@@ -18,11 +18,6 @@  namespace libcamera {
  * \brief Struct to hold the debayer parameters.
  */
 
-/**
- * \var DebayerParams::kGain10
- * \brief const value for 1.0 gain
- */
-
 /**
  * \var DebayerParams::kRGBLookupSize
  * \brief Size of a color lookup table