| Message ID | 20240531123840.713364-4-mzamazal@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Milan, Thank you for the patch. On Fri, May 31, 2024 at 02:38:38PM +0200, Milan Zamazal wrote: > Constructing the color mapping tables is related to stats rather than > debayering, where they are applied. Let's move the corresponding code > to stats processing. > > The same applies to the auxiliary gamma table. As the gamma value is > currently fixed and used in a single place, with the temporary exception > mentioned below, there is no need to share it anywhere anymore. > > It's necessary to initialize SoftwareIsp::debayerParams_ to default > values. These initial values are used for the first two frames, before > they are changed based on determined stats. To avoid sharing the gamma > value constant in artificial ways, we use 0.5 directly in the > initialization. This all is not a particularly elegant thing to do, > such a code belongs conceptually to the similar code in stats > processing, but doing better is left for larger refactoring. > > This is a preliminary step towards building this functionality on top of > libipa/algorithm.h, which should follow. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > Reviewed-by: Andrei Konovalov <andrey.konovalov.ynk@gmail.com> > --- > .../internal/software_isp/debayer_params.h | 18 +++---- > src/ipa/simple/soft_simple.cpp | 51 +++++++++++++++---- > src/libcamera/software_isp/debayer.cpp | 29 +++++------ > src/libcamera/software_isp/debayer_cpu.cpp | 43 +++------------- > src/libcamera/software_isp/debayer_cpu.h | 11 ++-- > src/libcamera/software_isp/software_isp.cpp | 23 +++++++-- > 6 files changed, 95 insertions(+), 80 deletions(-) > > diff --git a/include/libcamera/internal/software_isp/debayer_params.h b/include/libcamera/internal/software_isp/debayer_params.h > index ce1b5945..463d24b3 100644 > --- a/include/libcamera/internal/software_isp/debayer_params.h > +++ b/include/libcamera/internal/software_isp/debayer_params.h > @@ -1,6 +1,6 @@ > /* SPDX-License-Identifier: LGPL-2.1-or-later */ > /* > - * Copyright (C) 2023, Red Hat Inc. > + * Copyright (C) 2023, 2024 Red Hat Inc. > * > * Authors: > * Hans de Goede <hdegoede@redhat.com> > @@ -10,20 +10,20 @@ > > #pragma once > > +#include <array> > +#include <stdint.h> > + > namespace libcamera { > > struct DebayerParams { > static constexpr unsigned int kGain10 = 256; > + static constexpr unsigned int kRGBLookupSize = 256; > > - unsigned int gainR; > - unsigned int gainG; > - unsigned int gainB; > + using ColorLookupTable = std::array<uint8_t, kRGBLookupSize>; > > - float gamma; > - /** > - * \brief Level of the black point, 0..255, 0 is no correction. > - */ > - unsigned int blackLevel; > + ColorLookupTable red; > + ColorLookupTable green; > + ColorLookupTable blue; > }; > > } /* namespace libcamera */ > diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp > index f383f994..3388686f 100644 > --- a/src/ipa/simple/soft_simple.cpp > +++ b/src/ipa/simple/soft_simple.cpp > @@ -5,6 +5,7 @@ > * Simple Software Image Processing Algorithm module > */ > > +#include <cmath> > #include <numeric> > #include <stdint.h> > #include <sys/mman.h> > @@ -84,6 +85,10 @@ private: > ControlInfoMap sensorInfoMap_; > BlackLevel blackLevel_; > > + static constexpr unsigned int kGammaLookupSize = 1024; > + std::array<uint8_t, kGammaLookupSize> gammaTable_; > + int lastBlackLevel_ = -1; > + > int32_t exposureMin_, exposureMax_; > int32_t exposure_; > double againMin_, againMax_, againMinStep_; > @@ -246,7 +251,6 @@ void IPASoftSimple::processStats(const ControlList &sensorControls) > if (ignoreUpdates_ > 0) > blackLevel_.update(histogram); > const uint8_t blackLevel = blackLevel_.get(); > - params_->blackLevel = blackLevel; > > /* > * Black level must be subtracted to get the correct AWB ratios, > @@ -263,13 +267,42 @@ void IPASoftSimple::processStats(const ControlList &sensorControls) > /* > * Calculate red and blue gains for AWB. > * Clamp max gain at 4.0, this also avoids 0 division. > + * Gain: 128 = 0.5, 256 = 1.0, 512 = 2.0, etc. > */ > - params_->gainR = sumR <= sumG / 4 ? 1024 : 256 * sumG / sumR; > - params_->gainB = sumB <= sumG / 4 ? 1024 : 256 * sumG / sumB; > - > + const unsigned int gainR = sumR <= sumG / 4 ? 1024 : 256 * sumG / sumR; > + const unsigned int gainB = sumB <= sumG / 4 ? 1024 : 256 * sumG / sumB; > /* Green gain and gamma values are fixed */ > - params_->gainG = 256; > - params_->gamma = 0.5; > + constexpr unsigned int gainG = 256; > + > + /* Update the gamma table if needed */ > + if (blackLevel != lastBlackLevel_) { > + constexpr float gamma = 0.5; > + const unsigned int blackIndex = blackLevel * kGammaLookupSize / 256; > + 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 * > + std::pow((i - blackIndex) / divisor, gamma); > + > + lastBlackLevel_ = blackLevel; > + } > + > + for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) { > + constexpr unsigned int div = > + DebayerParams::kRGBLookupSize * DebayerParams::kGain10 / > + kGammaLookupSize; > + unsigned int idx; > + > + /* Apply gamma after gain! */ > + idx = std::min({ i * gainR / div, (kGammaLookupSize - 1) }); > + params_->red[i] = gammaTable_[idx]; > + > + idx = std::min({ i * gainG / div, (kGammaLookupSize - 1) }); > + params_->green[i] = gammaTable_[idx]; > + > + idx = std::min({ i * gainB / div, (kGammaLookupSize - 1) }); > + params_->blue[i] = gammaTable_[idx]; > + } > > setIspParams.emit(); > > @@ -290,7 +323,7 @@ void IPASoftSimple::processStats(const ControlList &sensorControls) > * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf > */ > const unsigned int blackLevelHistIdx = > - params_->blackLevel / (256 / SwIspStats::kYHistogramSize); > + blackLevel / (256 / SwIspStats::kYHistogramSize); > const unsigned int histogramSize = > SwIspStats::kYHistogramSize - blackLevelHistIdx; > const unsigned int yHistValsPerBin = histogramSize / kExposureBinsCount; > @@ -338,8 +371,8 @@ void IPASoftSimple::processStats(const ControlList &sensorControls) > > LOG(IPASoft, Debug) << "exposureMSV " << exposureMSV > << " exp " << exposure_ << " again " << again_ > - << " gain R/B " << params_->gainR << "/" << params_->gainB > - << " black level " << params_->blackLevel; > + << " gain R/B " << gainR << "/" << gainB > + << " black level " << static_cast<unsigned int>(blackLevel); > } > > void IPASoftSimple::updateExposure(double exposureMSV) > diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp > index efe75ea8..3f3969f7 100644 > --- a/src/libcamera/software_isp/debayer.cpp > +++ b/src/libcamera/software_isp/debayer.cpp > @@ -1,7 +1,7 @@ > /* SPDX-License-Identifier: LGPL-2.1-or-later */ > /* > * Copyright (C) 2023, Linaro Ltd > - * Copyright (C) 2023, Red Hat Inc. > + * Copyright (C) 2023, 2024 Red Hat Inc. > * > * Authors: > * Hans de Goede <hdegoede@redhat.com> > @@ -24,29 +24,28 @@ namespace libcamera { > */ > > /** > - * \var DebayerParams::gainR > - * \brief Red gain > - * > - * 128 = 0.5, 256 = 1.0, 512 = 2.0, etc. > + * \var DebayerParams::kRGBLookupSize > + * \brief Size of a color lookup table > */ > > /** > - * \var DebayerParams::gainG > - * \brief Green gain > - * > - * 128 = 0.5, 256 = 1.0, 512 = 2.0, etc. > + * \typedef DebayerParams::ColorLookupTable > + * \brief Type of the lookup tables for red, green, blue values > */ > > /** > - * \var DebayerParams::gainB > - * \brief Blue gain > - * > - * 128 = 0.5, 256 = 1.0, 512 = 2.0, etc. > + * \var DebayerParams::red > + * \brief Lookup table for red color, mapping input values to output values > + */ > + > +/** > + * \var DebayerParams::green > + * \brief Lookup table for green color, mapping input values to output values > */ > > /** > - * \var DebayerParams::gamma > - * \brief Gamma correction, 1.0 is no correction > + * \var DebayerParams::blue > + * \brief Lookup table for blue color, mapping input values to output values > */ > > /** > diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp > index 8254bbe9..c038eed4 100644 > --- a/src/libcamera/software_isp/debayer_cpu.cpp > +++ b/src/libcamera/software_isp/debayer_cpu.cpp > @@ -11,7 +11,6 @@ > > #include "debayer_cpu.h" > > -#include <math.h> > #include <stdlib.h> > #include <time.h> > > @@ -35,7 +34,7 @@ namespace libcamera { > * \param[in] stats Pointer to the stats object to use > */ > DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats) > - : stats_(std::move(stats)), gammaCorrection_(1.0), blackLevel_(0) > + : stats_(std::move(stats)) > { > /* > * Reading from uncached buffers may be very slow. > @@ -47,9 +46,9 @@ DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats) > */ > enableInputMemcpy_ = true; > > - /* Initialize gamma to 1.0 curve */ > - for (unsigned int i = 0; i < kGammaLookupSize; i++) > - gamma_[i] = i / (kGammaLookupSize / kRGBLookupSize); > + /* Initialize color lookup tables */ > + for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) > + red_[i] = green_[i] = blue_[i] = i; > > for (unsigned int i = 0; i < kMaxLineBuffers; i++) > lineBuffers_[i] = nullptr; > @@ -698,37 +697,9 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams > clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime); > } > > - /* Apply DebayerParams */ > - if (params.gamma != gammaCorrection_ || params.blackLevel != blackLevel_) { > - const unsigned int blackIndex = > - params.blackLevel * kGammaLookupSize / 256; > - std::fill(gamma_.begin(), gamma_.begin() + blackIndex, 0); > - const float divisor = kGammaLookupSize - blackIndex - 1.0; > - for (unsigned int i = blackIndex; i < kGammaLookupSize; i++) > - gamma_[i] = UINT8_MAX * powf((i - blackIndex) / divisor, params.gamma); > - > - gammaCorrection_ = params.gamma; > - blackLevel_ = params.blackLevel; > - } > - > - if (swapRedBlueGains_) > - std::swap(params.gainR, params.gainB); > - > - for (unsigned int i = 0; i < kRGBLookupSize; i++) { > - constexpr unsigned int div = > - kRGBLookupSize * DebayerParams::kGain10 / kGammaLookupSize; > - unsigned int idx; > - > - /* Apply gamma after gain! */ > - idx = std::min({ i * params.gainR / div, (kGammaLookupSize - 1) }); > - red_[i] = gamma_[idx]; > - > - idx = std::min({ i * params.gainG / div, (kGammaLookupSize - 1) }); > - green_[i] = gamma_[idx]; > - > - idx = std::min({ i * params.gainB / div, (kGammaLookupSize - 1) }); > - blue_[i] = gamma_[idx]; > - } > + green_ = params.green; > + red_ = swapRedBlueGains_ ? params.blue : params.red; > + blue_ = swapRedBlueGains_ ? params.red : params.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 de216fe3..be7dcdca 100644 > --- a/src/libcamera/software_isp/debayer_cpu.h > +++ b/src/libcamera/software_isp/debayer_cpu.h > @@ -122,15 +122,12 @@ private: > void process2(const uint8_t *src, uint8_t *dst); > void process4(const uint8_t *src, uint8_t *dst); > > - static constexpr unsigned int kGammaLookupSize = 1024; > - static constexpr unsigned int kRGBLookupSize = 256; > /* Max. supported Bayer pattern height is 4, debayering this requires 5 lines */ > static constexpr unsigned int kMaxLineBuffers = 5; > > - std::array<uint8_t, kGammaLookupSize> gamma_; > - std::array<uint8_t, kRGBLookupSize> red_; > - std::array<uint8_t, kRGBLookupSize> green_; > - std::array<uint8_t, kRGBLookupSize> blue_; > + DebayerParams::ColorLookupTable red_; > + DebayerParams::ColorLookupTable green_; > + DebayerParams::ColorLookupTable blue_; > debayerFn debayer0_; > debayerFn debayer1_; > debayerFn debayer2_; > @@ -146,8 +143,6 @@ private: > unsigned int xShift_; /* Offset of 0/1 applied to window_.x */ > bool enableInputMemcpy_; > bool swapRedBlueGains_; > - float gammaCorrection_; > - unsigned int blackLevel_; > unsigned int measuredFrames_; > int64_t frameProcessTime_; > /* Skip 30 frames for things to stabilize then measure 30 frames */ > diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp > index c9b6be56..ccd703bb 100644 > --- a/src/libcamera/software_isp/software_isp.cpp > +++ b/src/libcamera/software_isp/software_isp.cpp > @@ -7,6 +7,8 @@ > > #include "libcamera/internal/software_isp/software_isp.h" > > +#include <cmath> > +#include <stdint.h> > #include <sys/mman.h> > #include <sys/types.h> > #include <unistd.h> > @@ -18,6 +20,7 @@ > #include "libcamera/internal/framebuffer.h" > #include "libcamera/internal/ipa_manager.h" > #include "libcamera/internal/mapped_framebuffer.h" > +#include "libcamera/internal/software_isp/debayer_params.h" > > #include "debayer_cpu.h" > > @@ -63,10 +66,24 @@ LOG_DEFINE_CATEGORY(SoftwareIsp) > * handler > */ > SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor) > - : debayerParams_{ DebayerParams::kGain10, DebayerParams::kGain10, > - DebayerParams::kGain10, 0.5f, 0 }, > - dmaHeap_(DmaHeap::DmaHeapFlag::Cma | DmaHeap::DmaHeapFlag::System) > + : dmaHeap_(DmaHeap::DmaHeapFlag::Cma | DmaHeap::DmaHeapFlag::System) > { > + /* > + * debayerParams_ must be initialized because the initial value is used for > + * the first two frames, i.e. until stats processing starts providing its > + * own parameters. > + * > + * \todo This should be handled in the same place as the related operations. I would write * \todo This should be handled in the same place as the related * operations, in the IPA module. No need to resubmit just for this, I can update the comment when applying. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + */ > + std::array<uint8_t, 256> gammaTable; > + for (unsigned int i = 0; i < 256; i++) > + gammaTable[i] = UINT8_MAX * std::pow(i / 256.0, 0.5); > + for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) { > + debayerParams_.red[i] = gammaTable[i]; > + debayerParams_.green[i] = gammaTable[i]; > + debayerParams_.blue[i] = gammaTable[i]; > + } > + > if (!dmaHeap_.isValid()) { > LOG(SoftwareIsp, Error) << "Failed to create DmaHeap object"; > return;
Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > Hi Milan, > > Thank you for the patch. Hi Laurent, thank you for review. > On Fri, May 31, 2024 at 02:38:38PM +0200, Milan Zamazal wrote: >> Constructing the color mapping tables is related to stats rather than >> debayering, where they are applied. Let's move the corresponding code >> to stats processing. >> >> The same applies to the auxiliary gamma table. As the gamma value is >> currently fixed and used in a single place, with the temporary exception >> mentioned below, there is no need to share it anywhere anymore. >> >> It's necessary to initialize SoftwareIsp::debayerParams_ to default >> values. These initial values are used for the first two frames, before >> they are changed based on determined stats. To avoid sharing the gamma >> value constant in artificial ways, we use 0.5 directly in the >> initialization. This all is not a particularly elegant thing to do, >> such a code belongs conceptually to the similar code in stats >> processing, but doing better is left for larger refactoring. >> >> This is a preliminary step towards building this functionality on top of >> libipa/algorithm.h, which should follow. >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> Reviewed-by: Andrei Konovalov <andrey.konovalov.ynk@gmail.com> >> --- >> .../internal/software_isp/debayer_params.h | 18 +++---- >> src/ipa/simple/soft_simple.cpp | 51 +++++++++++++++---- >> src/libcamera/software_isp/debayer.cpp | 29 +++++------ >> src/libcamera/software_isp/debayer_cpu.cpp | 43 +++------------- >> src/libcamera/software_isp/debayer_cpu.h | 11 ++-- >> src/libcamera/software_isp/software_isp.cpp | 23 +++++++-- >> 6 files changed, 95 insertions(+), 80 deletions(-) >> >> diff --git a/include/libcamera/internal/software_isp/debayer_params.h b/include/libcamera/internal/software_isp/debayer_params.h >> index ce1b5945..463d24b3 100644 >> --- a/include/libcamera/internal/software_isp/debayer_params.h >> +++ b/include/libcamera/internal/software_isp/debayer_params.h >> @@ -1,6 +1,6 @@ >> /* SPDX-License-Identifier: LGPL-2.1-or-later */ >> /* >> - * Copyright (C) 2023, Red Hat Inc. >> + * Copyright (C) 2023, 2024 Red Hat Inc. >> * >> * Authors: >> * Hans de Goede <hdegoede@redhat.com> >> @@ -10,20 +10,20 @@ >> >> #pragma once >> >> +#include <array> >> +#include <stdint.h> >> + >> namespace libcamera { >> >> struct DebayerParams { >> static constexpr unsigned int kGain10 = 256; >> + static constexpr unsigned int kRGBLookupSize = 256; >> >> - unsigned int gainR; >> - unsigned int gainG; >> - unsigned int gainB; >> + using ColorLookupTable = std::array<uint8_t, kRGBLookupSize>; >> >> - float gamma; >> - /** >> - * \brief Level of the black point, 0..255, 0 is no correction. >> - */ >> - unsigned int blackLevel; >> + ColorLookupTable red; >> + ColorLookupTable green; >> + ColorLookupTable blue; >> }; >> >> } /* namespace libcamera */ >> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp >> index f383f994..3388686f 100644 >> --- a/src/ipa/simple/soft_simple.cpp >> +++ b/src/ipa/simple/soft_simple.cpp >> @@ -5,6 +5,7 @@ >> * Simple Software Image Processing Algorithm module >> */ >> >> +#include <cmath> >> #include <numeric> >> #include <stdint.h> >> #include <sys/mman.h> >> @@ -84,6 +85,10 @@ private: >> ControlInfoMap sensorInfoMap_; >> BlackLevel blackLevel_; >> >> + static constexpr unsigned int kGammaLookupSize = 1024; >> + std::array<uint8_t, kGammaLookupSize> gammaTable_; >> + int lastBlackLevel_ = -1; >> + >> int32_t exposureMin_, exposureMax_; >> int32_t exposure_; >> double againMin_, againMax_, againMinStep_; >> @@ -246,7 +251,6 @@ void IPASoftSimple::processStats(const ControlList &sensorControls) >> if (ignoreUpdates_ > 0) >> blackLevel_.update(histogram); >> const uint8_t blackLevel = blackLevel_.get(); >> - params_->blackLevel = blackLevel; >> >> /* >> * Black level must be subtracted to get the correct AWB ratios, >> @@ -263,13 +267,42 @@ void IPASoftSimple::processStats(const ControlList &sensorControls) >> /* >> * Calculate red and blue gains for AWB. >> * Clamp max gain at 4.0, this also avoids 0 division. >> + * Gain: 128 = 0.5, 256 = 1.0, 512 = 2.0, etc. >> */ >> - params_->gainR = sumR <= sumG / 4 ? 1024 : 256 * sumG / sumR; >> - params_->gainB = sumB <= sumG / 4 ? 1024 : 256 * sumG / sumB; >> - >> + const unsigned int gainR = sumR <= sumG / 4 ? 1024 : 256 * sumG / sumR; >> + const unsigned int gainB = sumB <= sumG / 4 ? 1024 : 256 * sumG / sumB; >> /* Green gain and gamma values are fixed */ >> - params_->gainG = 256; >> - params_->gamma = 0.5; >> + constexpr unsigned int gainG = 256; >> + >> + /* Update the gamma table if needed */ >> + if (blackLevel != lastBlackLevel_) { >> + constexpr float gamma = 0.5; >> + const unsigned int blackIndex = blackLevel * kGammaLookupSize / 256; >> + 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 * >> + std::pow((i - blackIndex) / divisor, gamma); >> + >> + lastBlackLevel_ = blackLevel; >> + } >> + >> + for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) { >> + constexpr unsigned int div = >> + DebayerParams::kRGBLookupSize * DebayerParams::kGain10 / >> + kGammaLookupSize; >> + unsigned int idx; >> + >> + /* Apply gamma after gain! */ >> + idx = std::min({ i * gainR / div, (kGammaLookupSize - 1) }); >> + params_->red[i] = gammaTable_[idx]; >> + >> + idx = std::min({ i * gainG / div, (kGammaLookupSize - 1) }); >> + params_->green[i] = gammaTable_[idx]; >> + >> + idx = std::min({ i * gainB / div, (kGammaLookupSize - 1) }); >> + params_->blue[i] = gammaTable_[idx]; >> + } >> >> setIspParams.emit(); >> >> @@ -290,7 +323,7 @@ void IPASoftSimple::processStats(const ControlList &sensorControls) >> * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf >> */ >> const unsigned int blackLevelHistIdx = >> - params_->blackLevel / (256 / SwIspStats::kYHistogramSize); >> + blackLevel / (256 / SwIspStats::kYHistogramSize); >> const unsigned int histogramSize = >> SwIspStats::kYHistogramSize - blackLevelHistIdx; >> const unsigned int yHistValsPerBin = histogramSize / kExposureBinsCount; >> @@ -338,8 +371,8 @@ void IPASoftSimple::processStats(const ControlList &sensorControls) >> >> LOG(IPASoft, Debug) << "exposureMSV " << exposureMSV >> << " exp " << exposure_ << " again " << again_ >> - << " gain R/B " << params_->gainR << "/" << params_->gainB >> - << " black level " << params_->blackLevel; >> + << " gain R/B " << gainR << "/" << gainB >> + << " black level " << static_cast<unsigned int>(blackLevel); >> } >> >> void IPASoftSimple::updateExposure(double exposureMSV) >> diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp >> index efe75ea8..3f3969f7 100644 >> --- a/src/libcamera/software_isp/debayer.cpp >> +++ b/src/libcamera/software_isp/debayer.cpp >> @@ -1,7 +1,7 @@ >> /* SPDX-License-Identifier: LGPL-2.1-or-later */ >> /* >> * Copyright (C) 2023, Linaro Ltd >> - * Copyright (C) 2023, Red Hat Inc. >> + * Copyright (C) 2023, 2024 Red Hat Inc. >> * >> * Authors: >> * Hans de Goede <hdegoede@redhat.com> >> @@ -24,29 +24,28 @@ namespace libcamera { >> */ >> >> /** >> - * \var DebayerParams::gainR >> - * \brief Red gain >> - * >> - * 128 = 0.5, 256 = 1.0, 512 = 2.0, etc. >> + * \var DebayerParams::kRGBLookupSize >> + * \brief Size of a color lookup table >> */ >> >> /** >> - * \var DebayerParams::gainG >> - * \brief Green gain >> - * >> - * 128 = 0.5, 256 = 1.0, 512 = 2.0, etc. >> + * \typedef DebayerParams::ColorLookupTable >> + * \brief Type of the lookup tables for red, green, blue values >> */ >> >> /** >> - * \var DebayerParams::gainB >> - * \brief Blue gain >> - * >> - * 128 = 0.5, 256 = 1.0, 512 = 2.0, etc. >> + * \var DebayerParams::red >> + * \brief Lookup table for red color, mapping input values to output values >> + */ >> + >> +/** >> + * \var DebayerParams::green >> + * \brief Lookup table for green color, mapping input values to output values >> */ >> >> /** >> - * \var DebayerParams::gamma >> - * \brief Gamma correction, 1.0 is no correction >> + * \var DebayerParams::blue >> + * \brief Lookup table for blue color, mapping input values to output values >> */ >> >> /** >> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp >> index 8254bbe9..c038eed4 100644 >> --- a/src/libcamera/software_isp/debayer_cpu.cpp >> +++ b/src/libcamera/software_isp/debayer_cpu.cpp >> @@ -11,7 +11,6 @@ >> >> #include "debayer_cpu.h" >> >> -#include <math.h> >> #include <stdlib.h> >> #include <time.h> >> >> @@ -35,7 +34,7 @@ namespace libcamera { >> * \param[in] stats Pointer to the stats object to use >> */ >> DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats) >> - : stats_(std::move(stats)), gammaCorrection_(1.0), blackLevel_(0) >> + : stats_(std::move(stats)) >> { >> /* >> * Reading from uncached buffers may be very slow. >> @@ -47,9 +46,9 @@ DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats) >> */ >> enableInputMemcpy_ = true; >> >> - /* Initialize gamma to 1.0 curve */ >> - for (unsigned int i = 0; i < kGammaLookupSize; i++) >> - gamma_[i] = i / (kGammaLookupSize / kRGBLookupSize); >> + /* Initialize color lookup tables */ >> + for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) >> + red_[i] = green_[i] = blue_[i] = i; >> >> for (unsigned int i = 0; i < kMaxLineBuffers; i++) >> lineBuffers_[i] = nullptr; >> @@ -698,37 +697,9 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams >> clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime); >> } >> >> - /* Apply DebayerParams */ >> - if (params.gamma != gammaCorrection_ || params.blackLevel != blackLevel_) { >> - const unsigned int blackIndex = >> - params.blackLevel * kGammaLookupSize / 256; >> - std::fill(gamma_.begin(), gamma_.begin() + blackIndex, 0); >> - const float divisor = kGammaLookupSize - blackIndex - 1.0; >> - for (unsigned int i = blackIndex; i < kGammaLookupSize; i++) >> - gamma_[i] = UINT8_MAX * powf((i - blackIndex) / divisor, params.gamma); >> - >> - gammaCorrection_ = params.gamma; >> - blackLevel_ = params.blackLevel; >> - } >> - >> - if (swapRedBlueGains_) >> - std::swap(params.gainR, params.gainB); >> - >> - for (unsigned int i = 0; i < kRGBLookupSize; i++) { >> - constexpr unsigned int div = >> - kRGBLookupSize * DebayerParams::kGain10 / kGammaLookupSize; >> - unsigned int idx; >> - >> - /* Apply gamma after gain! */ >> - idx = std::min({ i * params.gainR / div, (kGammaLookupSize - 1) }); >> - red_[i] = gamma_[idx]; >> - >> - idx = std::min({ i * params.gainG / div, (kGammaLookupSize - 1) }); >> - green_[i] = gamma_[idx]; >> - >> - idx = std::min({ i * params.gainB / div, (kGammaLookupSize - 1) }); >> - blue_[i] = gamma_[idx]; >> - } >> + green_ = params.green; >> + red_ = swapRedBlueGains_ ? params.blue : params.red; >> + blue_ = swapRedBlueGains_ ? params.red : params.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 de216fe3..be7dcdca 100644 >> --- a/src/libcamera/software_isp/debayer_cpu.h >> +++ b/src/libcamera/software_isp/debayer_cpu.h >> @@ -122,15 +122,12 @@ private: >> void process2(const uint8_t *src, uint8_t *dst); >> void process4(const uint8_t *src, uint8_t *dst); >> >> - static constexpr unsigned int kGammaLookupSize = 1024; >> - static constexpr unsigned int kRGBLookupSize = 256; >> /* Max. supported Bayer pattern height is 4, debayering this requires 5 lines */ >> static constexpr unsigned int kMaxLineBuffers = 5; >> >> - std::array<uint8_t, kGammaLookupSize> gamma_; >> - std::array<uint8_t, kRGBLookupSize> red_; >> - std::array<uint8_t, kRGBLookupSize> green_; >> - std::array<uint8_t, kRGBLookupSize> blue_; >> + DebayerParams::ColorLookupTable red_; >> + DebayerParams::ColorLookupTable green_; >> + DebayerParams::ColorLookupTable blue_; >> debayerFn debayer0_; >> debayerFn debayer1_; >> debayerFn debayer2_; >> @@ -146,8 +143,6 @@ private: >> unsigned int xShift_; /* Offset of 0/1 applied to window_.x */ >> bool enableInputMemcpy_; >> bool swapRedBlueGains_; >> - float gammaCorrection_; >> - unsigned int blackLevel_; >> unsigned int measuredFrames_; >> int64_t frameProcessTime_; >> /* Skip 30 frames for things to stabilize then measure 30 frames */ >> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp >> index c9b6be56..ccd703bb 100644 >> --- a/src/libcamera/software_isp/software_isp.cpp >> +++ b/src/libcamera/software_isp/software_isp.cpp >> @@ -7,6 +7,8 @@ >> >> #include "libcamera/internal/software_isp/software_isp.h" >> >> +#include <cmath> >> +#include <stdint.h> >> #include <sys/mman.h> >> #include <sys/types.h> >> #include <unistd.h> >> @@ -18,6 +20,7 @@ >> #include "libcamera/internal/framebuffer.h" >> #include "libcamera/internal/ipa_manager.h" >> #include "libcamera/internal/mapped_framebuffer.h" >> +#include "libcamera/internal/software_isp/debayer_params.h" >> >> #include "debayer_cpu.h" >> >> @@ -63,10 +66,24 @@ LOG_DEFINE_CATEGORY(SoftwareIsp) >> * handler >> */ >> SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor) >> - : debayerParams_{ DebayerParams::kGain10, DebayerParams::kGain10, >> - DebayerParams::kGain10, 0.5f, 0 }, >> - dmaHeap_(DmaHeap::DmaHeapFlag::Cma | DmaHeap::DmaHeapFlag::System) >> + : dmaHeap_(DmaHeap::DmaHeapFlag::Cma | DmaHeap::DmaHeapFlag::System) >> { >> + /* >> + * debayerParams_ must be initialized because the initial value is used for >> + * the first two frames, i.e. until stats processing starts providing its >> + * own parameters. >> + * >> + * \todo This should be handled in the same place as the related operations. > > I would write > > * \todo This should be handled in the same place as the related > * operations, in the IPA module. Yes, OK. > No need to resubmit just for this, I can update the comment when > applying. Thank you. > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> + */ >> + std::array<uint8_t, 256> gammaTable; >> + for (unsigned int i = 0; i < 256; i++) >> + gammaTable[i] = UINT8_MAX * std::pow(i / 256.0, 0.5); >> + for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) { >> + debayerParams_.red[i] = gammaTable[i]; >> + debayerParams_.green[i] = gammaTable[i]; >> + debayerParams_.blue[i] = gammaTable[i]; >> + } >> + >> if (!dmaHeap_.isValid()) { >> LOG(SoftwareIsp, Error) << "Failed to create DmaHeap object"; >> return;
diff --git a/include/libcamera/internal/software_isp/debayer_params.h b/include/libcamera/internal/software_isp/debayer_params.h index ce1b5945..463d24b3 100644 --- a/include/libcamera/internal/software_isp/debayer_params.h +++ b/include/libcamera/internal/software_isp/debayer_params.h @@ -1,6 +1,6 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ /* - * Copyright (C) 2023, Red Hat Inc. + * Copyright (C) 2023, 2024 Red Hat Inc. * * Authors: * Hans de Goede <hdegoede@redhat.com> @@ -10,20 +10,20 @@ #pragma once +#include <array> +#include <stdint.h> + namespace libcamera { struct DebayerParams { static constexpr unsigned int kGain10 = 256; + static constexpr unsigned int kRGBLookupSize = 256; - unsigned int gainR; - unsigned int gainG; - unsigned int gainB; + using ColorLookupTable = std::array<uint8_t, kRGBLookupSize>; - float gamma; - /** - * \brief Level of the black point, 0..255, 0 is no correction. - */ - unsigned int blackLevel; + ColorLookupTable red; + ColorLookupTable green; + ColorLookupTable blue; }; } /* namespace libcamera */ diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp index f383f994..3388686f 100644 --- a/src/ipa/simple/soft_simple.cpp +++ b/src/ipa/simple/soft_simple.cpp @@ -5,6 +5,7 @@ * Simple Software Image Processing Algorithm module */ +#include <cmath> #include <numeric> #include <stdint.h> #include <sys/mman.h> @@ -84,6 +85,10 @@ private: ControlInfoMap sensorInfoMap_; BlackLevel blackLevel_; + static constexpr unsigned int kGammaLookupSize = 1024; + std::array<uint8_t, kGammaLookupSize> gammaTable_; + int lastBlackLevel_ = -1; + int32_t exposureMin_, exposureMax_; int32_t exposure_; double againMin_, againMax_, againMinStep_; @@ -246,7 +251,6 @@ void IPASoftSimple::processStats(const ControlList &sensorControls) if (ignoreUpdates_ > 0) blackLevel_.update(histogram); const uint8_t blackLevel = blackLevel_.get(); - params_->blackLevel = blackLevel; /* * Black level must be subtracted to get the correct AWB ratios, @@ -263,13 +267,42 @@ void IPASoftSimple::processStats(const ControlList &sensorControls) /* * Calculate red and blue gains for AWB. * Clamp max gain at 4.0, this also avoids 0 division. + * Gain: 128 = 0.5, 256 = 1.0, 512 = 2.0, etc. */ - params_->gainR = sumR <= sumG / 4 ? 1024 : 256 * sumG / sumR; - params_->gainB = sumB <= sumG / 4 ? 1024 : 256 * sumG / sumB; - + const unsigned int gainR = sumR <= sumG / 4 ? 1024 : 256 * sumG / sumR; + const unsigned int gainB = sumB <= sumG / 4 ? 1024 : 256 * sumG / sumB; /* Green gain and gamma values are fixed */ - params_->gainG = 256; - params_->gamma = 0.5; + constexpr unsigned int gainG = 256; + + /* Update the gamma table if needed */ + if (blackLevel != lastBlackLevel_) { + constexpr float gamma = 0.5; + const unsigned int blackIndex = blackLevel * kGammaLookupSize / 256; + 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 * + std::pow((i - blackIndex) / divisor, gamma); + + lastBlackLevel_ = blackLevel; + } + + for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) { + constexpr unsigned int div = + DebayerParams::kRGBLookupSize * DebayerParams::kGain10 / + kGammaLookupSize; + unsigned int idx; + + /* Apply gamma after gain! */ + idx = std::min({ i * gainR / div, (kGammaLookupSize - 1) }); + params_->red[i] = gammaTable_[idx]; + + idx = std::min({ i * gainG / div, (kGammaLookupSize - 1) }); + params_->green[i] = gammaTable_[idx]; + + idx = std::min({ i * gainB / div, (kGammaLookupSize - 1) }); + params_->blue[i] = gammaTable_[idx]; + } setIspParams.emit(); @@ -290,7 +323,7 @@ void IPASoftSimple::processStats(const ControlList &sensorControls) * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf */ const unsigned int blackLevelHistIdx = - params_->blackLevel / (256 / SwIspStats::kYHistogramSize); + blackLevel / (256 / SwIspStats::kYHistogramSize); const unsigned int histogramSize = SwIspStats::kYHistogramSize - blackLevelHistIdx; const unsigned int yHistValsPerBin = histogramSize / kExposureBinsCount; @@ -338,8 +371,8 @@ void IPASoftSimple::processStats(const ControlList &sensorControls) LOG(IPASoft, Debug) << "exposureMSV " << exposureMSV << " exp " << exposure_ << " again " << again_ - << " gain R/B " << params_->gainR << "/" << params_->gainB - << " black level " << params_->blackLevel; + << " gain R/B " << gainR << "/" << gainB + << " black level " << static_cast<unsigned int>(blackLevel); } void IPASoftSimple::updateExposure(double exposureMSV) diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp index efe75ea8..3f3969f7 100644 --- a/src/libcamera/software_isp/debayer.cpp +++ b/src/libcamera/software_isp/debayer.cpp @@ -1,7 +1,7 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ /* * Copyright (C) 2023, Linaro Ltd - * Copyright (C) 2023, Red Hat Inc. + * Copyright (C) 2023, 2024 Red Hat Inc. * * Authors: * Hans de Goede <hdegoede@redhat.com> @@ -24,29 +24,28 @@ namespace libcamera { */ /** - * \var DebayerParams::gainR - * \brief Red gain - * - * 128 = 0.5, 256 = 1.0, 512 = 2.0, etc. + * \var DebayerParams::kRGBLookupSize + * \brief Size of a color lookup table */ /** - * \var DebayerParams::gainG - * \brief Green gain - * - * 128 = 0.5, 256 = 1.0, 512 = 2.0, etc. + * \typedef DebayerParams::ColorLookupTable + * \brief Type of the lookup tables for red, green, blue values */ /** - * \var DebayerParams::gainB - * \brief Blue gain - * - * 128 = 0.5, 256 = 1.0, 512 = 2.0, etc. + * \var DebayerParams::red + * \brief Lookup table for red color, mapping input values to output values + */ + +/** + * \var DebayerParams::green + * \brief Lookup table for green color, mapping input values to output values */ /** - * \var DebayerParams::gamma - * \brief Gamma correction, 1.0 is no correction + * \var DebayerParams::blue + * \brief Lookup table for blue color, mapping input values to output values */ /** diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp index 8254bbe9..c038eed4 100644 --- a/src/libcamera/software_isp/debayer_cpu.cpp +++ b/src/libcamera/software_isp/debayer_cpu.cpp @@ -11,7 +11,6 @@ #include "debayer_cpu.h" -#include <math.h> #include <stdlib.h> #include <time.h> @@ -35,7 +34,7 @@ namespace libcamera { * \param[in] stats Pointer to the stats object to use */ DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats) - : stats_(std::move(stats)), gammaCorrection_(1.0), blackLevel_(0) + : stats_(std::move(stats)) { /* * Reading from uncached buffers may be very slow. @@ -47,9 +46,9 @@ DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats) */ enableInputMemcpy_ = true; - /* Initialize gamma to 1.0 curve */ - for (unsigned int i = 0; i < kGammaLookupSize; i++) - gamma_[i] = i / (kGammaLookupSize / kRGBLookupSize); + /* Initialize color lookup tables */ + for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) + red_[i] = green_[i] = blue_[i] = i; for (unsigned int i = 0; i < kMaxLineBuffers; i++) lineBuffers_[i] = nullptr; @@ -698,37 +697,9 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime); } - /* Apply DebayerParams */ - if (params.gamma != gammaCorrection_ || params.blackLevel != blackLevel_) { - const unsigned int blackIndex = - params.blackLevel * kGammaLookupSize / 256; - std::fill(gamma_.begin(), gamma_.begin() + blackIndex, 0); - const float divisor = kGammaLookupSize - blackIndex - 1.0; - for (unsigned int i = blackIndex; i < kGammaLookupSize; i++) - gamma_[i] = UINT8_MAX * powf((i - blackIndex) / divisor, params.gamma); - - gammaCorrection_ = params.gamma; - blackLevel_ = params.blackLevel; - } - - if (swapRedBlueGains_) - std::swap(params.gainR, params.gainB); - - for (unsigned int i = 0; i < kRGBLookupSize; i++) { - constexpr unsigned int div = - kRGBLookupSize * DebayerParams::kGain10 / kGammaLookupSize; - unsigned int idx; - - /* Apply gamma after gain! */ - idx = std::min({ i * params.gainR / div, (kGammaLookupSize - 1) }); - red_[i] = gamma_[idx]; - - idx = std::min({ i * params.gainG / div, (kGammaLookupSize - 1) }); - green_[i] = gamma_[idx]; - - idx = std::min({ i * params.gainB / div, (kGammaLookupSize - 1) }); - blue_[i] = gamma_[idx]; - } + green_ = params.green; + red_ = swapRedBlueGains_ ? params.blue : params.red; + blue_ = swapRedBlueGains_ ? params.red : params.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 de216fe3..be7dcdca 100644 --- a/src/libcamera/software_isp/debayer_cpu.h +++ b/src/libcamera/software_isp/debayer_cpu.h @@ -122,15 +122,12 @@ private: void process2(const uint8_t *src, uint8_t *dst); void process4(const uint8_t *src, uint8_t *dst); - static constexpr unsigned int kGammaLookupSize = 1024; - static constexpr unsigned int kRGBLookupSize = 256; /* Max. supported Bayer pattern height is 4, debayering this requires 5 lines */ static constexpr unsigned int kMaxLineBuffers = 5; - std::array<uint8_t, kGammaLookupSize> gamma_; - std::array<uint8_t, kRGBLookupSize> red_; - std::array<uint8_t, kRGBLookupSize> green_; - std::array<uint8_t, kRGBLookupSize> blue_; + DebayerParams::ColorLookupTable red_; + DebayerParams::ColorLookupTable green_; + DebayerParams::ColorLookupTable blue_; debayerFn debayer0_; debayerFn debayer1_; debayerFn debayer2_; @@ -146,8 +143,6 @@ private: unsigned int xShift_; /* Offset of 0/1 applied to window_.x */ bool enableInputMemcpy_; bool swapRedBlueGains_; - float gammaCorrection_; - unsigned int blackLevel_; unsigned int measuredFrames_; int64_t frameProcessTime_; /* Skip 30 frames for things to stabilize then measure 30 frames */ diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp index c9b6be56..ccd703bb 100644 --- a/src/libcamera/software_isp/software_isp.cpp +++ b/src/libcamera/software_isp/software_isp.cpp @@ -7,6 +7,8 @@ #include "libcamera/internal/software_isp/software_isp.h" +#include <cmath> +#include <stdint.h> #include <sys/mman.h> #include <sys/types.h> #include <unistd.h> @@ -18,6 +20,7 @@ #include "libcamera/internal/framebuffer.h" #include "libcamera/internal/ipa_manager.h" #include "libcamera/internal/mapped_framebuffer.h" +#include "libcamera/internal/software_isp/debayer_params.h" #include "debayer_cpu.h" @@ -63,10 +66,24 @@ LOG_DEFINE_CATEGORY(SoftwareIsp) * handler */ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor) - : debayerParams_{ DebayerParams::kGain10, DebayerParams::kGain10, - DebayerParams::kGain10, 0.5f, 0 }, - dmaHeap_(DmaHeap::DmaHeapFlag::Cma | DmaHeap::DmaHeapFlag::System) + : dmaHeap_(DmaHeap::DmaHeapFlag::Cma | DmaHeap::DmaHeapFlag::System) { + /* + * debayerParams_ must be initialized because the initial value is used for + * the first two frames, i.e. until stats processing starts providing its + * own parameters. + * + * \todo This should be handled in the same place as the related operations. + */ + std::array<uint8_t, 256> gammaTable; + for (unsigned int i = 0; i < 256; i++) + gammaTable[i] = UINT8_MAX * std::pow(i / 256.0, 0.5); + for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) { + debayerParams_.red[i] = gammaTable[i]; + debayerParams_.green[i] = gammaTable[i]; + debayerParams_.blue[i] = gammaTable[i]; + } + if (!dmaHeap_.isValid()) { LOG(SoftwareIsp, Error) << "Failed to create DmaHeap object"; return;