| Message ID | 20241009191940.2359227-2-mzamazal@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Quoting Milan Zamazal (2024-10-09 20:19:33) > The black level in software ISP is unconditionally guessed from the > obtained frames. CameraSensorHelper optionally provides the black level > from camera specifications now. Let's use the value if available. > > If the black level is not available from the given CameraSensorHelper > instance, it's still determined on the fly. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/ipa/simple/algorithms/blc.cpp | 6 +++++- > src/ipa/simple/ipa_context.h | 4 ++++ > src/ipa/simple/soft_simple.cpp | 3 +++ > 3 files changed, 12 insertions(+), 1 deletion(-) > > diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp > index b9f2aaa6d..a7af2e12c 100644 > --- a/src/ipa/simple/algorithms/blc.cpp > +++ b/src/ipa/simple/algorithms/blc.cpp > @@ -24,7 +24,8 @@ BlackLevel::BlackLevel() > int BlackLevel::configure(IPAContext &context, > [[maybe_unused]] const IPAConfigInfo &configInfo) > { > - context.activeState.blc.level = 255; > + context.activeState.blc.level = > + context.configuration.black.level.value_or(255); > return 0; > } > > @@ -34,6 +35,9 @@ void BlackLevel::process(IPAContext &context, > const SwIspStats *stats, > [[maybe_unused]] ControlList &metadata) > { > + if (context.configuration.black.level.has_value()) > + return; > + > if (frameContext.sensor.exposure == exposure_ && > frameContext.sensor.gain == gain_) { > return; > diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h > index 3519f20f6..fd121eebe 100644 > --- a/src/ipa/simple/ipa_context.h > +++ b/src/ipa/simple/ipa_context.h > @@ -8,6 +8,7 @@ > #pragma once > > #include <array> > +#include <optional> > #include <stdint.h> > > #include <libipa/fc_queue.h> > @@ -22,6 +23,9 @@ struct IPASessionConfiguration { > int32_t exposureMin, exposureMax; > double againMin, againMax, againMinStep; > } agc; > + struct { > + std::optional<uint8_t> level; Maybe I asked this before. Is 8 bits enough ? I guess it is if you're dividing by 256 ... but I don't understand what happens on a 12 bit sensor here ... > + } black; > }; > > struct IPAActiveState { > diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp > index b28c7039f..ad6334ff6 100644 > --- a/src/ipa/simple/soft_simple.cpp > +++ b/src/ipa/simple/soft_simple.cpp > @@ -201,6 +201,9 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo) > (context_.configuration.agc.againMax - > context_.configuration.agc.againMin) / > 100.0; > + if (camHelper_->blackLevel().has_value()) > + context_.configuration.black.level = > + camHelper_->blackLevel().value() / 256; Is 256 going to work for all sensor modes here? What happens if a sensor supports 8, 10, and 12 bits per pixel? > } else { > /* > * The camera sensor gain (g) is usually not equal to the value written > -- > 2.44.1 >
Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > Quoting Milan Zamazal (2024-10-09 20:19:33) >> The black level in software ISP is unconditionally guessed from the >> obtained frames. CameraSensorHelper optionally provides the black level > >> from camera specifications now. Let's use the value if available. >> >> If the black level is not available from the given CameraSensorHelper >> instance, it's still determined on the fly. >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> src/ipa/simple/algorithms/blc.cpp | 6 +++++- >> src/ipa/simple/ipa_context.h | 4 ++++ >> src/ipa/simple/soft_simple.cpp | 3 +++ >> 3 files changed, 12 insertions(+), 1 deletion(-) >> >> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp >> index b9f2aaa6d..a7af2e12c 100644 >> --- a/src/ipa/simple/algorithms/blc.cpp >> +++ b/src/ipa/simple/algorithms/blc.cpp >> @@ -24,7 +24,8 @@ BlackLevel::BlackLevel() >> int BlackLevel::configure(IPAContext &context, >> [[maybe_unused]] const IPAConfigInfo &configInfo) >> { >> - context.activeState.blc.level = 255; >> + context.activeState.blc.level = >> + context.configuration.black.level.value_or(255); >> return 0; >> } >> >> @@ -34,6 +35,9 @@ void BlackLevel::process(IPAContext &context, >> const SwIspStats *stats, >> [[maybe_unused]] ControlList &metadata) >> { >> + if (context.configuration.black.level.has_value()) >> + return; >> + >> if (frameContext.sensor.exposure == exposure_ && >> frameContext.sensor.gain == gain_) { >> return; >> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h >> index 3519f20f6..fd121eebe 100644 >> --- a/src/ipa/simple/ipa_context.h >> +++ b/src/ipa/simple/ipa_context.h >> @@ -8,6 +8,7 @@ >> #pragma once >> >> #include <array> >> +#include <optional> >> #include <stdint.h> >> >> #include <libipa/fc_queue.h> >> @@ -22,6 +23,9 @@ struct IPASessionConfiguration { >> int32_t exposureMin, exposureMax; >> double againMin, againMax, againMinStep; >> } agc; >> + struct { >> + std::optional<uint8_t> level; > > Maybe I asked this before. Is 8 bits enough ? I guess it is if you're > dividing by 256 ... but I don't understand what happens on a 12 bit > sensor here ... Software ISP currently works with 8 bits everywhere. The sensor pixels are cut to 8bit if needed before processing, see e.g. https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/software_isp/debayer_cpu.cpp#n168. This might change with GPU processing but in such a case all the algorithms would have to be revised. >> + } black; >> }; >> >> struct IPAActiveState { >> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp >> index b28c7039f..ad6334ff6 100644 >> --- a/src/ipa/simple/soft_simple.cpp >> +++ b/src/ipa/simple/soft_simple.cpp >> @@ -201,6 +201,9 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo) >> (context_.configuration.agc.againMax - >> context_.configuration.agc.againMin) / >> 100.0; >> + if (camHelper_->blackLevel().has_value()) >> + context_.configuration.black.level = >> + camHelper_->blackLevel().value() / 256; > > Is 256 going to work for all sensor modes here? What happens if a sensor > supports 8, 10, and 12 bits per pixel? The black level from CameraSensorHelper is a 16 bit value (https://libcamera.org/internal-api-html/classlibcamera_1_1ipa_1_1CameraSensorHelper.html#ad6d462fc5828e841563289eef22d3174) and software ISP works with 8 bit values regardless of the number of sensor pixel bits. That means all the values are sensor pixel width independent. >> } else { >> /* >> * The camera sensor gain (g) is usually not equal to the value written >> -- >> 2.44.1 >>
Quoting Milan Zamazal (2024-10-10 09:33:21) > Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > > > Quoting Milan Zamazal (2024-10-09 20:19:33) > >> The black level in software ISP is unconditionally guessed from the > >> obtained frames. CameraSensorHelper optionally provides the black level > > > >> from camera specifications now. Let's use the value if available. > >> > >> If the black level is not available from the given CameraSensorHelper > >> instance, it's still determined on the fly. > >> > >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > >> --- > >> src/ipa/simple/algorithms/blc.cpp | 6 +++++- > >> src/ipa/simple/ipa_context.h | 4 ++++ > >> src/ipa/simple/soft_simple.cpp | 3 +++ > >> 3 files changed, 12 insertions(+), 1 deletion(-) > >> > >> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp > >> index b9f2aaa6d..a7af2e12c 100644 > >> --- a/src/ipa/simple/algorithms/blc.cpp > >> +++ b/src/ipa/simple/algorithms/blc.cpp > >> @@ -24,7 +24,8 @@ BlackLevel::BlackLevel() > >> int BlackLevel::configure(IPAContext &context, > >> [[maybe_unused]] const IPAConfigInfo &configInfo) > >> { > >> - context.activeState.blc.level = 255; > >> + context.activeState.blc.level = > >> + context.configuration.black.level.value_or(255); > >> return 0; > >> } > >> > >> @@ -34,6 +35,9 @@ void BlackLevel::process(IPAContext &context, > >> const SwIspStats *stats, > >> [[maybe_unused]] ControlList &metadata) > >> { > >> + if (context.configuration.black.level.has_value()) > >> + return; > >> + > >> if (frameContext.sensor.exposure == exposure_ && > >> frameContext.sensor.gain == gain_) { > >> return; > >> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h > >> index 3519f20f6..fd121eebe 100644 > >> --- a/src/ipa/simple/ipa_context.h > >> +++ b/src/ipa/simple/ipa_context.h > >> @@ -8,6 +8,7 @@ > >> #pragma once > >> > >> #include <array> > >> +#include <optional> > >> #include <stdint.h> > >> > >> #include <libipa/fc_queue.h> > >> @@ -22,6 +23,9 @@ struct IPASessionConfiguration { > >> int32_t exposureMin, exposureMax; > >> double againMin, againMax, againMinStep; > >> } agc; > >> + struct { > >> + std::optional<uint8_t> level; > > > > Maybe I asked this before. Is 8 bits enough ? I guess it is if you're > > dividing by 256 ... but I don't understand what happens on a 12 bit > > sensor here ... > > Software ISP currently works with 8 bits everywhere. The sensor pixels > are cut to 8bit if needed before processing, see e.g. https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/software_isp/debayer_cpu.cpp#n168. > > This might change with GPU processing but in such a case all the > algorithms would have to be revised. > > >> + } black; > >> }; > >> > >> struct IPAActiveState { > >> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp > >> index b28c7039f..ad6334ff6 100644 > >> --- a/src/ipa/simple/soft_simple.cpp > >> +++ b/src/ipa/simple/soft_simple.cpp > >> @@ -201,6 +201,9 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo) > >> (context_.configuration.agc.againMax - > >> context_.configuration.agc.againMin) / > >> 100.0; > >> + if (camHelper_->blackLevel().has_value()) > >> + context_.configuration.black.level = > >> + camHelper_->blackLevel().value() / 256; > > > > Is 256 going to work for all sensor modes here? What happens if a sensor > > supports 8, 10, and 12 bits per pixel? > > The black level from CameraSensorHelper is a 16 bit value > (https://libcamera.org/internal-api-html/classlibcamera_1_1ipa_1_1CameraSensorHelper.html#ad6d462fc5828e841563289eef22d3174) > and software ISP works with 8 bit values regardless of the number of > sensor pixel bits. That means all the values are sensor pixel width > independent. It would be really helpful to add a comment here stating that I think... Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > >> } else { > >> /* > >> * The camera sensor gain (g) is usually not equal to the value written > >> -- > >> 2.44.1 > >> >
Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > Quoting Milan Zamazal (2024-10-10 09:33:21) >> Kieran Bingham <kieran.bingham@ideasonboard.com> writes: >> > >> > Quoting Milan Zamazal (2024-10-09 20:19:33) >> >> The black level in software ISP is unconditionally guessed from the >> >> obtained frames. CameraSensorHelper optionally provides the black level >> > >> >> from camera specifications now. Let's use the value if available. >> >> >> >> If the black level is not available from the given CameraSensorHelper >> >> instance, it's still determined on the fly. >> >> >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> >> --- >> >> src/ipa/simple/algorithms/blc.cpp | 6 +++++- >> >> src/ipa/simple/ipa_context.h | 4 ++++ >> >> src/ipa/simple/soft_simple.cpp | 3 +++ >> >> 3 files changed, 12 insertions(+), 1 deletion(-) >> >> >> >> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp >> >> index b9f2aaa6d..a7af2e12c 100644 >> >> --- a/src/ipa/simple/algorithms/blc.cpp >> >> +++ b/src/ipa/simple/algorithms/blc.cpp >> >> @@ -24,7 +24,8 @@ BlackLevel::BlackLevel() >> >> int BlackLevel::configure(IPAContext &context, >> >> [[maybe_unused]] const IPAConfigInfo &configInfo) >> >> { >> >> - context.activeState.blc.level = 255; >> >> + context.activeState.blc.level = >> >> + context.configuration.black.level.value_or(255); >> >> return 0; >> >> } >> >> >> >> @@ -34,6 +35,9 @@ void BlackLevel::process(IPAContext &context, >> >> const SwIspStats *stats, >> >> [[maybe_unused]] ControlList &metadata) >> >> { >> >> + if (context.configuration.black.level.has_value()) >> >> + return; >> >> + >> >> if (frameContext.sensor.exposure == exposure_ && >> >> frameContext.sensor.gain == gain_) { >> >> return; >> >> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h >> >> index 3519f20f6..fd121eebe 100644 >> >> --- a/src/ipa/simple/ipa_context.h >> >> +++ b/src/ipa/simple/ipa_context.h >> >> @@ -8,6 +8,7 @@ >> >> #pragma once >> >> >> >> #include <array> >> >> +#include <optional> >> >> #include <stdint.h> >> >> >> >> #include <libipa/fc_queue.h> >> >> @@ -22,6 +23,9 @@ struct IPASessionConfiguration { >> >> int32_t exposureMin, exposureMax; >> >> double againMin, againMax, againMinStep; >> >> } agc; >> >> + struct { >> >> + std::optional<uint8_t> level; >> > >> > Maybe I asked this before. Is 8 bits enough ? I guess it is if you're >> > dividing by 256 ... but I don't understand what happens on a 12 bit >> > sensor here ... >> >> Software ISP currently works with 8 bits everywhere. The sensor pixels >> are cut to 8bit if needed before processing, see >> e.g. https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/software_isp/debayer_cpu.cpp#n168. >> >> This might change with GPU processing but in such a case all the >> algorithms would have to be revised. >> >> >> + } black; >> >> }; >> >> >> >> struct IPAActiveState { >> >> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp >> >> index b28c7039f..ad6334ff6 100644 >> >> --- a/src/ipa/simple/soft_simple.cpp >> >> +++ b/src/ipa/simple/soft_simple.cpp >> >> @@ -201,6 +201,9 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo) >> >> (context_.configuration.agc.againMax - >> >> context_.configuration.agc.againMin) / >> >> 100.0; >> >> + if (camHelper_->blackLevel().has_value()) >> >> + context_.configuration.black.level = >> >> + camHelper_->blackLevel().value() / 256; >> > >> > Is 256 going to work for all sensor modes here? What happens if a sensor >> > supports 8, 10, and 12 bits per pixel? >> >> The black level from CameraSensorHelper is a 16 bit value >> (https://libcamera.org/internal-api-html/classlibcamera_1_1ipa_1_1CameraSensorHelper.html#ad6d462fc5828e841563289eef22d3174) >> and software ISP works with 8 bit values regardless of the number of >> sensor pixel bits. That means all the values are sensor pixel width >> independent. > > It would be really helpful to add a comment here stating that I think... Added in v3 (the only change there). > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> >> >> } else { >> >> /* >> >> * The camera sensor gain (g) is usually not equal to the value written >> >> -- >> >> 2.44.1 >> >> >>
diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp index b9f2aaa6d..a7af2e12c 100644 --- a/src/ipa/simple/algorithms/blc.cpp +++ b/src/ipa/simple/algorithms/blc.cpp @@ -24,7 +24,8 @@ BlackLevel::BlackLevel() int BlackLevel::configure(IPAContext &context, [[maybe_unused]] const IPAConfigInfo &configInfo) { - context.activeState.blc.level = 255; + context.activeState.blc.level = + context.configuration.black.level.value_or(255); return 0; } @@ -34,6 +35,9 @@ void BlackLevel::process(IPAContext &context, const SwIspStats *stats, [[maybe_unused]] ControlList &metadata) { + if (context.configuration.black.level.has_value()) + return; + if (frameContext.sensor.exposure == exposure_ && frameContext.sensor.gain == gain_) { return; diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h index 3519f20f6..fd121eebe 100644 --- a/src/ipa/simple/ipa_context.h +++ b/src/ipa/simple/ipa_context.h @@ -8,6 +8,7 @@ #pragma once #include <array> +#include <optional> #include <stdint.h> #include <libipa/fc_queue.h> @@ -22,6 +23,9 @@ struct IPASessionConfiguration { int32_t exposureMin, exposureMax; double againMin, againMax, againMinStep; } agc; + struct { + std::optional<uint8_t> level; + } black; }; struct IPAActiveState { diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp index b28c7039f..ad6334ff6 100644 --- a/src/ipa/simple/soft_simple.cpp +++ b/src/ipa/simple/soft_simple.cpp @@ -201,6 +201,9 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo) (context_.configuration.agc.againMax - context_.configuration.agc.againMin) / 100.0; + if (camHelper_->blackLevel().has_value()) + context_.configuration.black.level = + camHelper_->blackLevel().value() / 256; } else { /* * The camera sensor gain (g) is usually not equal to the value written
The black level in software ISP is unconditionally guessed from the obtained frames. CameraSensorHelper optionally provides the black level from camera specifications now. Let's use the value if available. If the black level is not available from the given CameraSensorHelper instance, it's still determined on the fly. Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- src/ipa/simple/algorithms/blc.cpp | 6 +++++- src/ipa/simple/ipa_context.h | 4 ++++ src/ipa/simple/soft_simple.cpp | 3 +++ 3 files changed, 12 insertions(+), 1 deletion(-)