| Message ID | 20240423182000.1527425-6-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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
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
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
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(-)