[v3,3/5] libcamera: software_isp: Move color mappings out of debayering
diff mbox series

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

Commit Message

Milan Zamazal May 23, 2024, 11:26 a.m. UTC 
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.

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    | 41 +++------------
 src/libcamera/software_isp/debayer_cpu.h      |  9 ++--
 src/libcamera/software_isp/software_isp.cpp   |  4 +-
 6 files changed, 75 insertions(+), 77 deletions(-)

Comments

Kieran Bingham May 24, 2024, 10:27 p.m. UTC | #1 
Quoting Milan Zamazal (2024-05-23 12:26:54)
> 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.
> 
> 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>

I haven't investigated/identified why yet - but I bisected the startup
overflow/colour issue to this patch.

There's also a CI failure with blackLevel_ and gammaCorrection_ being
left unused that got caught.


> ---
>  .../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    | 41 +++------------
>  src/libcamera/software_isp/debayer_cpu.h      |  9 ++--
>  src/libcamera/software_isp/software_isp.cpp   |  4 +-
>  6 files changed, 75 insertions(+), 77 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 722aac83..1b0655fe 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 <math.h>
>  #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;
>  
>         /*
>          * Calculate red and blue gains for AWB.
> @@ -265,12 +269,41 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)
>         const uint64_t sumG = subtractBlackLevel(stats_->sumG_, 2);
>         const uint64_t sumB = subtractBlackLevel(stats_->sumB_, 4);
>  
> -       params_->gainR = sumR <= sumG / 4 ? 1024 : 256 * sumG / sumR;
> -       params_->gainB = sumB <= sumG / 4 ? 1024 : 256 * sumG / sumB;
> -
> +       /* Gain: 128 = 0.5, 256 = 1.0, 512 = 2.0, etc. */
> +       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;
> +       /* gamma == 1.0 means no correction */
> +       constexpr float gamma = 0.5;
> +
> +       /* Update the gamma table if needed */
> +       if (blackLevel != lastBlackLevel_) {
> +               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 * powf((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();
>  
> @@ -291,7 +324,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;
> @@ -339,8 +372,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..f3099a62 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>
>  
> @@ -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;


The CI also identifies that this series leaves blackLevel_ and
gammaCorrection_ unused.

https://gitlab.freedesktop.org/camera/libcamera/-/jobs/59127177

--
Kieran


> -       }
> -
> -       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..768a4643 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_;
> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
> index c9b6be56..1003d835 100644
> --- a/src/libcamera/software_isp/software_isp.cpp
> +++ b/src/libcamera/software_isp/software_isp.cpp
> @@ -63,9 +63,7 @@ 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)
>  {
>         if (!dmaHeap_.isValid()) {
>                 LOG(SoftwareIsp, Error) << "Failed to create DmaHeap object";
> -- 
> 2.42.0
>
Milan Zamazal May 27, 2024, 5:26 p.m. UTC | #2 
Hi Kieran,

thank you for testing.

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

> Quoting Milan Zamazal (2024-05-23 12:26:54)
>> 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.
>> 
>> 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>
>
> I haven't investigated/identified why yet - but I bisected the startup
> overflow/colour issue to this patch.

I cannot reproduce that nice effect but looking at the commit (thank you
for identifying it) and given that I get black frames at the beginning,
I guess it's uninitialized debayerParams_.  My fault, I'll fix it.

> There's also a CI failure with blackLevel_ and gammaCorrection_ being
> left unused that got caught.

Ah, right.  I wonder why gcc doesn't report this (only clang does).

>> ---
>>  .../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    | 41 +++------------
>>  src/libcamera/software_isp/debayer_cpu.h      |  9 ++--
>>  src/libcamera/software_isp/software_isp.cpp   |  4 +-
>>  6 files changed, 75 insertions(+), 77 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 722aac83..1b0655fe 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 <math.h>
>>  #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;
>>  
>>         /*
>>          * Calculate red and blue gains for AWB.
>> @@ -265,12 +269,41 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)
>>         const uint64_t sumG = subtractBlackLevel(stats_->sumG_, 2);
>>         const uint64_t sumB = subtractBlackLevel(stats_->sumB_, 4);
>>  
>> -       params_->gainR = sumR <= sumG / 4 ? 1024 : 256 * sumG / sumR;
>> -       params_->gainB = sumB <= sumG / 4 ? 1024 : 256 * sumG / sumB;
>> -
>> +       /* Gain: 128 = 0.5, 256 = 1.0, 512 = 2.0, etc. */
>> +       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;
>> +       /* gamma == 1.0 means no correction */
>> +       constexpr float gamma = 0.5;
>> +
>> +       /* Update the gamma table if needed */
>> +       if (blackLevel != lastBlackLevel_) {
>> +               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 * powf((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();
>>  
>> @@ -291,7 +324,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;
>> @@ -339,8 +372,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..f3099a62 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>
>>  
>> @@ -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;
>
>
> The CI also identifies that this series leaves blackLevel_ and
> gammaCorrection_ unused.
>
> https://gitlab.freedesktop.org/camera/libcamera/-/jobs/59127177
>
> --
> Kieran
>
>
>> -       }
>> -
>> -       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..768a4643 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_;
>> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
>> index c9b6be56..1003d835 100644
>> --- a/src/libcamera/software_isp/software_isp.cpp
>> +++ b/src/libcamera/software_isp/software_isp.cpp
>> @@ -63,9 +63,7 @@ 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)
>>  {
>>         if (!dmaHeap_.isValid()) {
>>                 LOG(SoftwareIsp, Error) << "Failed to create DmaHeap object";
>> -- 
>> 2.42.0
>>

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 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 722aac83..1b0655fe 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 <math.h>
 #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;
 
 	/*
 	 * Calculate red and blue gains for AWB.
@@ -265,12 +269,41 @@  void IPASoftSimple::processStats(const ControlList &sensorControls)
 	const uint64_t sumG = subtractBlackLevel(stats_->sumG_, 2);
 	const uint64_t sumB = subtractBlackLevel(stats_->sumB_, 4);
 
-	params_->gainR = sumR <= sumG / 4 ? 1024 : 256 * sumG / sumR;
-	params_->gainB = sumB <= sumG / 4 ? 1024 : 256 * sumG / sumB;
-
+	/* Gain: 128 = 0.5, 256 = 1.0, 512 = 2.0, etc. */
+	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;
+	/* gamma == 1.0 means no correction */
+	constexpr float gamma = 0.5;
+
+	/* Update the gamma table if needed */
+	if (blackLevel != lastBlackLevel_) {
+		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 * powf((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();
 
@@ -291,7 +324,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;
@@ -339,8 +372,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..f3099a62 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>
 
@@ -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..768a4643 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_;
diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
index c9b6be56..1003d835 100644
--- a/src/libcamera/software_isp/software_isp.cpp
+++ b/src/libcamera/software_isp/software_isp.cpp
@@ -63,9 +63,7 @@  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)
 {
 	if (!dmaHeap_.isValid()) {
 		LOG(SoftwareIsp, Error) << "Failed to create DmaHeap object";