| Message ID | 20250929131355.25897-1-hansg@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi 2025. 09. 29. 15:13 keltezéssel, Hans de Goede írta: > At the moment the blc code uses 255 as starting blacklevel for sensors > where there is no blacklevel info in the sensor-helper. > > There are a number of issues with this: > > 1. When the first frame is bad (e.g. mostly white) which happens sometimes > the initial blacklevel will be kept leading to a divide by zero problem > in the AGC code (this divide by 0 problem is avoided in the AGC code by > not running the AGC algorithm). The note in parentheses only applies after https://patchwork.libcamera.org/patch/24461/ right? > > 2. Not runnning the AGC algorithm means that the gain/exposure do not > change, which causes the BLC algorithm to not run on the next frames, > so we keep the bad 255 blacklevel which stops AGC from running which > stops BLC from running. Leaving things stuck at a 255 blacklevel > resulting in an unusuable image. > > 3. Sometimes the auto-blc code leads to an unrealistic high > blacklevel detection which results in lower image quality. > > To fix this start with a blacklevel of 16, which is the highest > (4096 / 256) blacklevel used for any sensor listing a blackLevel_ > value in the sensor-helper class. As far as I can see the black level is only ever decreased by the algorithm. So this changes the initial value from the "theoretical maximum" to a "heuristic maximum", correct? Regards, Barnabás Pőcze > > Note 2. could alternatively be fixed by disabling the check for > exposure/gain changes when the blacklevel is unrealistic high, > but that still leaves the other problems. > > Signed-off-by: Hans de Goede <hansg@kernel.org> > --- > src/ipa/simple/algorithms/blc.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp > index ec71e154e..370385afc 100644 > --- a/src/ipa/simple/algorithms/blc.cpp > +++ b/src/ipa/simple/algorithms/blc.cpp > @@ -43,7 +43,7 @@ int BlackLevel::configure(IPAContext &context, > if (definedLevel_.has_value()) > context.configuration.black.level = definedLevel_; > context.activeState.blc.level = > - context.configuration.black.level.value_or(255); > + context.configuration.black.level.value_or(16); > return 0; > } >
Hi Hans, thank you for the patch. Hans de Goede <hansg@kernel.org> writes: > At the moment the blc code uses 255 as starting blacklevel for sensors > where there is no blacklevel info in the sensor-helper. > > There are a number of issues with this: > > 1. When the first frame is bad (e.g. mostly white) which happens sometimes > the initial blacklevel will be kept leading to a divide by zero problem > in the AGC code (this divide by 0 problem is avoided in the AGC code by > not running the AGC algorithm). > > 2. Not runnning the AGC algorithm means that the gain/exposure do not > change, which causes the BLC algorithm to not run on the next frames, > so we keep the bad 255 blacklevel which stops AGC from running which > stops BLC from running. Leaving things stuck at a 255 blacklevel > resulting in an unusuable image. > > 3. Sometimes the auto-blc code leads to an unrealistic high > blacklevel detection which results in lower image quality. > > To fix this start with a blacklevel of 16, which is the highest > (4096 / 256) blacklevel used for any sensor listing a blackLevel_ > value in the sensor-helper class. > > Note 2. could alternatively be fixed by disabling the check for > exposure/gain changes when the blacklevel is unrealistic high, > but that still leaves the other problems. > > Signed-off-by: Hans de Goede <hansg@kernel.org> Makes sense. Reviewed-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/ipa/simple/algorithms/blc.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp > index ec71e154e..370385afc 100644 > --- a/src/ipa/simple/algorithms/blc.cpp > +++ b/src/ipa/simple/algorithms/blc.cpp > @@ -43,7 +43,7 @@ int BlackLevel::configure(IPAContext &context, > if (definedLevel_.has_value()) > context.configuration.black.level = definedLevel_; > context.activeState.blc.level = > - context.configuration.black.level.value_or(255); > + context.configuration.black.level.value_or(16); > return 0; > }
Hi, On 29-Sep-25 5:52 PM, Barnabás Pőcze wrote: > Hi > > 2025. 09. 29. 15:13 keltezéssel, Hans de Goede írta: >> At the moment the blc code uses 255 as starting blacklevel for sensors >> where there is no blacklevel info in the sensor-helper. >> >> There are a number of issues with this: >> >> 1. When the first frame is bad (e.g. mostly white) which happens sometimes >> the initial blacklevel will be kept leading to a divide by zero problem >> in the AGC code (this divide by 0 problem is avoided in the AGC code by >> not running the AGC algorithm). > > The note in parentheses only applies after https://patchwork.libcamera.org/patch/24461/ right? Right. >> 2. Not runnning the AGC algorithm means that the gain/exposure do not >> change, which causes the BLC algorithm to not run on the next frames, >> so we keep the bad 255 blacklevel which stops AGC from running which >> stops BLC from running. Leaving things stuck at a 255 blacklevel >> resulting in an unusuable image. >> >> 3. Sometimes the auto-blc code leads to an unrealistic high >> blacklevel detection which results in lower image quality. >> >> To fix this start with a blacklevel of 16, which is the highest >> (4096 / 256) blacklevel used for any sensor listing a blackLevel_ >> value in the sensor-helper class. > > As far as I can see the black level is only ever decreased by the algorithm. > So this changes the initial value from the "theoretical maximum" to a > "heuristic maximum", correct? Correct. Regards, Hans >> Note 2. could alternatively be fixed by disabling the check for >> exposure/gain changes when the blacklevel is unrealistic high, >> but that still leaves the other problems. >> >> Signed-off-by: Hans de Goede <hansg@kernel.org> >> --- >> src/ipa/simple/algorithms/blc.cpp | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp >> index ec71e154e..370385afc 100644 >> --- a/src/ipa/simple/algorithms/blc.cpp >> +++ b/src/ipa/simple/algorithms/blc.cpp >> @@ -43,7 +43,7 @@ int BlackLevel::configure(IPAContext &context, >> if (definedLevel_.has_value()) >> context.configuration.black.level = definedLevel_; >> context.activeState.blc.level = >> - context.configuration.black.level.value_or(255); >> + context.configuration.black.level.value_or(16); >> return 0; >> } >> >
diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp index ec71e154e..370385afc 100644 --- a/src/ipa/simple/algorithms/blc.cpp +++ b/src/ipa/simple/algorithms/blc.cpp @@ -43,7 +43,7 @@ int BlackLevel::configure(IPAContext &context, if (definedLevel_.has_value()) context.configuration.black.level = definedLevel_; context.activeState.blc.level = - context.configuration.black.level.value_or(255); + context.configuration.black.level.value_or(16); return 0; }
At the moment the blc code uses 255 as starting blacklevel for sensors where there is no blacklevel info in the sensor-helper. There are a number of issues with this: 1. When the first frame is bad (e.g. mostly white) which happens sometimes the initial blacklevel will be kept leading to a divide by zero problem in the AGC code (this divide by 0 problem is avoided in the AGC code by not running the AGC algorithm). 2. Not runnning the AGC algorithm means that the gain/exposure do not change, which causes the BLC algorithm to not run on the next frames, so we keep the bad 255 blacklevel which stops AGC from running which stops BLC from running. Leaving things stuck at a 255 blacklevel resulting in an unusuable image. 3. Sometimes the auto-blc code leads to an unrealistic high blacklevel detection which results in lower image quality. To fix this start with a blacklevel of 16, which is the highest (4096 / 256) blacklevel used for any sensor listing a blackLevel_ value in the sensor-helper class. Note 2. could alternatively be fixed by disabling the check for exposure/gain changes when the blacklevel is unrealistic high, but that still leaves the other problems. Signed-off-by: Hans de Goede <hansg@kernel.org> --- src/ipa/simple/algorithms/blc.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)