| Message ID | 20240528161126.35119-6-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Milan, Thank you for the patch. On Tue, May 28, 2024 at 06:11:26PM +0200, Milan Zamazal wrote: > The color lookup tables are passed from stats processing to debayering > for direct use as 8-bit color output values. Let's pass the values as > 0.0..1.0 floats to make the gains color bit width independent. > > Completes software ISP TODO #4. The implementation looks good, and this indeed covers TODO #4. The conversion of the tables from float to integer takes a big more CPU time, which is reasonable, but it got me thinking. TODO #4 was recorded at a time where the IPA module computed gain values, and the ISP computed the look up tables. The gains were higher-level parameters. I thought it would be good to not hardcode in their representation what was an internal implementation feature of the ISP, as that would allow changing the ISP implementation later without impacting the IPA module. Now that the look up tables are computed in the IPA module, the IPA and ISP are more tightly coupled. Do you think it's still useful to abstract the internal representation of the table entries from the IPA module, or would that just consume more CPU cycles without offering any benefit for later evolutions of the ISP ? I'm thinking in particular about future work on a GPU-based implementation of the soft ISP. Do you foresee that we would use the same IPA module, or a different one ? If you think it's useful we can merge this patch, otherwise we could drop the patch and drop TODO #4. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > Reviewed-by: Andrei Konovalov <andrey.konovalov.ynk@gmail.com> > --- > .../internal/software_isp/debayer_params.h | 2 +- > src/ipa/simple/soft_simple.cpp | 5 ++--- > src/libcamera/software_isp/TODO | 13 ------------- > src/libcamera/software_isp/debayer.cpp | 6 +++--- > src/libcamera/software_isp/debayer_cpu.cpp | 18 +++++++++++++++--- > src/libcamera/software_isp/debayer_cpu.h | 10 +++++++--- > 6 files changed, 28 insertions(+), 26 deletions(-) > > diff --git a/include/libcamera/internal/software_isp/debayer_params.h b/include/libcamera/internal/software_isp/debayer_params.h > index 2f75b4b5..647905c5 100644 > --- a/include/libcamera/internal/software_isp/debayer_params.h > +++ b/include/libcamera/internal/software_isp/debayer_params.h > @@ -19,7 +19,7 @@ struct DebayerParams { > static constexpr unsigned int kRGBLookupSize = 256; > static constexpr float kGamma = 0.5; > > - using ColorLookupTable = std::array<uint8_t, kRGBLookupSize>; > + using ColorLookupTable = std::array<float, kRGBLookupSize>; > > ColorLookupTable red; > ColorLookupTable green; > diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp > index 26597c99..b006ec4a 100644 > --- a/src/ipa/simple/soft_simple.cpp > +++ b/src/ipa/simple/soft_simple.cpp > @@ -86,7 +86,7 @@ private: > BlackLevel blackLevel_; > > static constexpr unsigned int kGammaLookupSize = 1024; > - std::array<uint8_t, kGammaLookupSize> gammaTable_; > + std::array<float, kGammaLookupSize> gammaTable_; > int lastBlackLevel_ = -1; > > int32_t exposureMin_, exposureMax_; > @@ -281,8 +281,7 @@ void IPASoftSimple::processStats(const ControlList &sensorControls) > std::fill(gammaTable_.begin(), gammaTable_.begin() + blackIndex, 0); > const float divisor = kGammaLookupSize - blackIndex - 1.0; > for (unsigned int i = blackIndex; i < kGammaLookupSize; i++) > - gammaTable_[i] = UINT8_MAX * > - powf((i - blackIndex) / divisor, DebayerParams::kGamma); > + gammaTable_[i] = powf((i - blackIndex) / divisor, DebayerParams::kGamma); > > lastBlackLevel_ = blackLevel; > } > diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO > index 4fcee39b..6bdc5905 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 80bf5dbe..03001ae1 100644 > --- a/src/libcamera/software_isp/debayer.cpp > +++ b/src/libcamera/software_isp/debayer.cpp > @@ -35,17 +35,17 @@ namespace libcamera { > > /** > * \var DebayerParams::red > - * \brief Lookup table for red color, mapping input values to output values > + * \brief Lookup table for red color, mapping input values to 0.0..1.0 > */ > > /** > * \var DebayerParams::green > - * \brief Lookup table for green color, mapping input values to output values > + * \brief Lookup table for green color, mapping input values to 0.0..1.0 > */ > > /** > * \var DebayerParams::blue > - * \brief Lookup table for blue color, mapping input values to output values > + * \brief Lookup table for blue color, mapping input values to 0.0..1.0 > */ > > /** > diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp > index c038eed4..9dcb5f91 100644 > --- a/src/libcamera/software_isp/debayer_cpu.cpp > +++ b/src/libcamera/software_isp/debayer_cpu.cpp > @@ -11,6 +11,8 @@ > > #include "debayer_cpu.h" > > +#include <stddef.h> > +#include <stdint.h> > #include <stdlib.h> > #include <time.h> > > @@ -688,6 +690,14 @@ static inline int64_t timeDiff(timespec &after, timespec &before) > (int64_t)after.tv_nsec - (int64_t)before.tv_nsec; > } > > +void DebayerCpu::updateColorLookupTable( > + const DebayerParams::ColorLookupTable &src, > + ColorLookupTable &dst) > +{ > + for (std::size_t i = 0; i < src.size(); i++) > + dst[i] = src[i] * UINT8_MAX; > +} > + > void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params) > { > timespec frameStartTime; > @@ -697,9 +707,11 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams > clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime); > } > > - green_ = params.green; > - red_ = swapRedBlueGains_ ? params.blue : params.red; > - blue_ = swapRedBlueGains_ ? params.red : params.blue; > + updateColorLookupTable(params.green, green_); > + updateColorLookupTable(swapRedBlueGains_ ? params.blue : params.red, > + red_); > + updateColorLookupTable(swapRedBlueGains_ ? params.red : params.blue, > + blue_); > > /* Copy metadata from the input buffer */ > FrameMetadata &metadata = output->_d()->metadata(); > diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h > index be7dcdca..dcf4e37d 100644 > --- a/src/libcamera/software_isp/debayer_cpu.h > +++ b/src/libcamera/software_isp/debayer_cpu.h > @@ -18,6 +18,7 @@ > #include <libcamera/base/object.h> > > #include "libcamera/internal/bayer_format.h" > +#include "libcamera/internal/software_isp/debayer_params.h" > > #include "debayer.h" > #include "swstats_cpu.h" > @@ -125,9 +126,12 @@ private: > /* Max. supported Bayer pattern height is 4, debayering this requires 5 lines */ > static constexpr unsigned int kMaxLineBuffers = 5; > > - DebayerParams::ColorLookupTable red_; > - DebayerParams::ColorLookupTable green_; > - DebayerParams::ColorLookupTable blue_; > + using ColorLookupTable = std::array<uint8_t, DebayerParams::kRGBLookupSize>; > + void updateColorLookupTable(const DebayerParams::ColorLookupTable &src, > + ColorLookupTable &dst); > + ColorLookupTable red_; > + ColorLookupTable green_; > + ColorLookupTable blue_; > debayerFn debayer0_; > debayerFn debayer1_; > debayerFn debayer2_;
Hi Laurent, thank you for review. Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > Hi Milan, > > Thank you for the patch. > > On Tue, May 28, 2024 at 06:11:26PM +0200, Milan Zamazal wrote: >> The color lookup tables are passed from stats processing to debayering >> for direct use as 8-bit color output values. Let's pass the values as >> 0.0..1.0 floats to make the gains color bit width independent. >> >> Completes software ISP TODO #4. > > The implementation looks good, and this indeed covers TODO #4. The > conversion of the tables from float to integer takes a big more CPU > time, which is reasonable, but it got me thinking. > > TODO #4 was recorded at a time where the IPA module computed gain > values, and the ISP computed the look up tables. The gains were > higher-level parameters. I thought it would be good to not hardcode in > their representation what was an internal implementation feature of the > ISP, as that would allow changing the ISP implementation later without > impacting the IPA module. > > Now that the look up tables are computed in the IPA module, the IPA and > ISP are more tightly coupled. Do you think it's still useful to abstract > the internal representation of the table entries from the IPA module, or > would that just consume more CPU cycles without offering any benefit for > later evolutions of the ISP ? I'm thinking in particular about future > work on a GPU-based implementation of the soft ISP. Do you foresee that > we would use the same IPA module, or a different one ? > > If you think it's useful we can merge this patch, otherwise we could > drop the patch and drop TODO #4. Well, after thinking about it too, I decided for dropping the patch (and dropping TODO #4 as a replacement patch). My reasoning: - The conversion cost is negligible to the whole frame processing on CPU but it's still some cost and it's better to avoid adding small costs without a good reason (and let's not make Hans crying ;-) ). - We don't have a good reason for the conversion right now. - Hardware pipelines seem to use integers in similar situations. - I'm not sure about a GPU-based implementation but I think using the same IPA module would be preferable. > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> Reviewed-by: Andrei Konovalov <andrey.konovalov.ynk@gmail.com> >> --- >> .../internal/software_isp/debayer_params.h | 2 +- >> src/ipa/simple/soft_simple.cpp | 5 ++--- >> src/libcamera/software_isp/TODO | 13 ------------- >> src/libcamera/software_isp/debayer.cpp | 6 +++--- >> src/libcamera/software_isp/debayer_cpu.cpp | 18 +++++++++++++++--- >> src/libcamera/software_isp/debayer_cpu.h | 10 +++++++--- >> 6 files changed, 28 insertions(+), 26 deletions(-) >> >> diff --git a/include/libcamera/internal/software_isp/debayer_params.h b/include/libcamera/internal/software_isp/debayer_params.h >> index 2f75b4b5..647905c5 100644 >> --- a/include/libcamera/internal/software_isp/debayer_params.h >> +++ b/include/libcamera/internal/software_isp/debayer_params.h >> @@ -19,7 +19,7 @@ struct DebayerParams { >> static constexpr unsigned int kRGBLookupSize = 256; >> static constexpr float kGamma = 0.5; >> >> - using ColorLookupTable = std::array<uint8_t, kRGBLookupSize>; >> + using ColorLookupTable = std::array<float, kRGBLookupSize>; >> >> ColorLookupTable red; >> ColorLookupTable green; >> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp >> index 26597c99..b006ec4a 100644 >> --- a/src/ipa/simple/soft_simple.cpp >> +++ b/src/ipa/simple/soft_simple.cpp >> @@ -86,7 +86,7 @@ private: >> BlackLevel blackLevel_; >> >> static constexpr unsigned int kGammaLookupSize = 1024; >> - std::array<uint8_t, kGammaLookupSize> gammaTable_; >> + std::array<float, kGammaLookupSize> gammaTable_; >> int lastBlackLevel_ = -1; >> >> int32_t exposureMin_, exposureMax_; >> @@ -281,8 +281,7 @@ void IPASoftSimple::processStats(const ControlList &sensorControls) >> std::fill(gammaTable_.begin(), gammaTable_.begin() + blackIndex, 0); >> const float divisor = kGammaLookupSize - blackIndex - 1.0; >> for (unsigned int i = blackIndex; i < kGammaLookupSize; i++) >> - gammaTable_[i] = UINT8_MAX * >> - powf((i - blackIndex) / divisor, DebayerParams::kGamma); >> + gammaTable_[i] = powf((i - blackIndex) / divisor, DebayerParams::kGamma); >> >> lastBlackLevel_ = blackLevel; >> } >> diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO >> index 4fcee39b..6bdc5905 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 80bf5dbe..03001ae1 100644 >> --- a/src/libcamera/software_isp/debayer.cpp >> +++ b/src/libcamera/software_isp/debayer.cpp >> @@ -35,17 +35,17 @@ namespace libcamera { >> >> /** >> * \var DebayerParams::red >> - * \brief Lookup table for red color, mapping input values to output values >> + * \brief Lookup table for red color, mapping input values to 0.0..1.0 >> */ >> >> /** >> * \var DebayerParams::green >> - * \brief Lookup table for green color, mapping input values to output values >> + * \brief Lookup table for green color, mapping input values to 0.0..1.0 >> */ >> >> /** >> * \var DebayerParams::blue >> - * \brief Lookup table for blue color, mapping input values to output values >> + * \brief Lookup table for blue color, mapping input values to 0.0..1.0 >> */ >> >> /** >> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp >> index c038eed4..9dcb5f91 100644 >> --- a/src/libcamera/software_isp/debayer_cpu.cpp >> +++ b/src/libcamera/software_isp/debayer_cpu.cpp >> @@ -11,6 +11,8 @@ >> >> #include "debayer_cpu.h" >> >> +#include <stddef.h> >> +#include <stdint.h> >> #include <stdlib.h> >> #include <time.h> >> >> @@ -688,6 +690,14 @@ static inline int64_t timeDiff(timespec &after, timespec &before) >> (int64_t)after.tv_nsec - (int64_t)before.tv_nsec; >> } >> >> +void DebayerCpu::updateColorLookupTable( >> + const DebayerParams::ColorLookupTable &src, >> + ColorLookupTable &dst) >> +{ >> + for (std::size_t i = 0; i < src.size(); i++) >> + dst[i] = src[i] * UINT8_MAX; >> +} >> + >> void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params) >> { >> timespec frameStartTime; >> @@ -697,9 +707,11 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams >> clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime); >> } >> >> - green_ = params.green; >> - red_ = swapRedBlueGains_ ? params.blue : params.red; >> - blue_ = swapRedBlueGains_ ? params.red : params.blue; >> + updateColorLookupTable(params.green, green_); >> + updateColorLookupTable(swapRedBlueGains_ ? params.blue : params.red, >> + red_); >> + updateColorLookupTable(swapRedBlueGains_ ? params.red : params.blue, >> + blue_); >> >> /* Copy metadata from the input buffer */ >> FrameMetadata &metadata = output->_d()->metadata(); >> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h >> index be7dcdca..dcf4e37d 100644 >> --- a/src/libcamera/software_isp/debayer_cpu.h >> +++ b/src/libcamera/software_isp/debayer_cpu.h >> @@ -18,6 +18,7 @@ >> #include <libcamera/base/object.h> >> >> #include "libcamera/internal/bayer_format.h" >> +#include "libcamera/internal/software_isp/debayer_params.h" >> >> #include "debayer.h" >> #include "swstats_cpu.h" >> @@ -125,9 +126,12 @@ private: >> /* Max. supported Bayer pattern height is 4, debayering this requires 5 lines */ >> static constexpr unsigned int kMaxLineBuffers = 5; >> >> - DebayerParams::ColorLookupTable red_; >> - DebayerParams::ColorLookupTable green_; >> - DebayerParams::ColorLookupTable blue_; >> + using ColorLookupTable = std::array<uint8_t, DebayerParams::kRGBLookupSize>; >> + void updateColorLookupTable(const DebayerParams::ColorLookupTable &src, >> + ColorLookupTable &dst); >> + ColorLookupTable red_; >> + ColorLookupTable green_; >> + ColorLookupTable blue_; >> debayerFn debayer0_; >> debayerFn debayer1_; >> debayerFn debayer2_;
diff --git a/include/libcamera/internal/software_isp/debayer_params.h b/include/libcamera/internal/software_isp/debayer_params.h index 2f75b4b5..647905c5 100644 --- a/include/libcamera/internal/software_isp/debayer_params.h +++ b/include/libcamera/internal/software_isp/debayer_params.h @@ -19,7 +19,7 @@ struct DebayerParams { static constexpr unsigned int kRGBLookupSize = 256; static constexpr float kGamma = 0.5; - using ColorLookupTable = std::array<uint8_t, kRGBLookupSize>; + using ColorLookupTable = std::array<float, kRGBLookupSize>; ColorLookupTable red; ColorLookupTable green; diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp index 26597c99..b006ec4a 100644 --- a/src/ipa/simple/soft_simple.cpp +++ b/src/ipa/simple/soft_simple.cpp @@ -86,7 +86,7 @@ private: BlackLevel blackLevel_; static constexpr unsigned int kGammaLookupSize = 1024; - std::array<uint8_t, kGammaLookupSize> gammaTable_; + std::array<float, kGammaLookupSize> gammaTable_; int lastBlackLevel_ = -1; int32_t exposureMin_, exposureMax_; @@ -281,8 +281,7 @@ void IPASoftSimple::processStats(const ControlList &sensorControls) std::fill(gammaTable_.begin(), gammaTable_.begin() + blackIndex, 0); const float divisor = kGammaLookupSize - blackIndex - 1.0; for (unsigned int i = blackIndex; i < kGammaLookupSize; i++) - gammaTable_[i] = UINT8_MAX * - powf((i - blackIndex) / divisor, DebayerParams::kGamma); + gammaTable_[i] = powf((i - blackIndex) / divisor, DebayerParams::kGamma); lastBlackLevel_ = blackLevel; } diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO index 4fcee39b..6bdc5905 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 80bf5dbe..03001ae1 100644 --- a/src/libcamera/software_isp/debayer.cpp +++ b/src/libcamera/software_isp/debayer.cpp @@ -35,17 +35,17 @@ namespace libcamera { /** * \var DebayerParams::red - * \brief Lookup table for red color, mapping input values to output values + * \brief Lookup table for red color, mapping input values to 0.0..1.0 */ /** * \var DebayerParams::green - * \brief Lookup table for green color, mapping input values to output values + * \brief Lookup table for green color, mapping input values to 0.0..1.0 */ /** * \var DebayerParams::blue - * \brief Lookup table for blue color, mapping input values to output values + * \brief Lookup table for blue color, mapping input values to 0.0..1.0 */ /** diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp index c038eed4..9dcb5f91 100644 --- a/src/libcamera/software_isp/debayer_cpu.cpp +++ b/src/libcamera/software_isp/debayer_cpu.cpp @@ -11,6 +11,8 @@ #include "debayer_cpu.h" +#include <stddef.h> +#include <stdint.h> #include <stdlib.h> #include <time.h> @@ -688,6 +690,14 @@ static inline int64_t timeDiff(timespec &after, timespec &before) (int64_t)after.tv_nsec - (int64_t)before.tv_nsec; } +void DebayerCpu::updateColorLookupTable( + const DebayerParams::ColorLookupTable &src, + ColorLookupTable &dst) +{ + for (std::size_t i = 0; i < src.size(); i++) + dst[i] = src[i] * UINT8_MAX; +} + void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params) { timespec frameStartTime; @@ -697,9 +707,11 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime); } - green_ = params.green; - red_ = swapRedBlueGains_ ? params.blue : params.red; - blue_ = swapRedBlueGains_ ? params.red : params.blue; + updateColorLookupTable(params.green, green_); + updateColorLookupTable(swapRedBlueGains_ ? params.blue : params.red, + red_); + updateColorLookupTable(swapRedBlueGains_ ? params.red : params.blue, + blue_); /* Copy metadata from the input buffer */ FrameMetadata &metadata = output->_d()->metadata(); diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h index be7dcdca..dcf4e37d 100644 --- a/src/libcamera/software_isp/debayer_cpu.h +++ b/src/libcamera/software_isp/debayer_cpu.h @@ -18,6 +18,7 @@ #include <libcamera/base/object.h> #include "libcamera/internal/bayer_format.h" +#include "libcamera/internal/software_isp/debayer_params.h" #include "debayer.h" #include "swstats_cpu.h" @@ -125,9 +126,12 @@ private: /* Max. supported Bayer pattern height is 4, debayering this requires 5 lines */ static constexpr unsigned int kMaxLineBuffers = 5; - DebayerParams::ColorLookupTable red_; - DebayerParams::ColorLookupTable green_; - DebayerParams::ColorLookupTable blue_; + using ColorLookupTable = std::array<uint8_t, DebayerParams::kRGBLookupSize>; + void updateColorLookupTable(const DebayerParams::ColorLookupTable &src, + ColorLookupTable &dst); + ColorLookupTable red_; + ColorLookupTable green_; + ColorLookupTable blue_; debayerFn debayer0_; debayerFn debayer1_; debayerFn debayer2_;