| Message ID | 20240423182000.1527425-5-mzamazal@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Milan, Thank you for the patch. On Tue, Apr 23, 2024 at 08:19:59PM +0200, Milan Zamazal wrote: > The current software ISP debayering implementation uses color lookup > tables that apply black level subtraction, color gain and gamma > correction in a single step, while performing debayering. In theory, > black level subtraction and color gains should be applied before > debayering and gamma correction after debayering. But because black > level subtraction and color gain corrections are currently linear and we > average only same-color pixels in debayering, we can apply all the > operations after debayering. The only difference is with clipping where > out-of-range pixels contribute more than they deserve but this is not > generally significant. > > Combining all the operations at once is conceptually not ideal but it is > not incorrect in this case and saves CPU cycles, which is critical for > software ISP CPU implementation (it may be less important with future > GPU implementation). This means we need the current implementation. It > may get replaced or become optional (configurable) in future, for > example if the separation is needed due to introducing other image > processing operations. > > Under these circumstances and considering that the lookup tables > construction has been moved out of the debayering code in the preceding > patch, it makes no sense to keep software ISP TODO #13. Let's remove it > in favor of a clarifying code comment. I still think we need to split those operations. The black level subtraction (coming before debayering and the colour gains) should then use black level values from a tuning file. An additional contrast enhancement after debayering can perform automatic histogram stretching if desired. As this is still being discussed, and experimentation is needed to assess the impact, I think the TODO item should stay until we reach a conclusion. > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/ipa/simple/soft_simple.cpp | 10 ++++++++++ > src/libcamera/software_isp/TODO | 10 ---------- > 2 files changed, 10 insertions(+), 10 deletions(-) > > diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp > index c5085f71..b2f4d308 100644 > --- a/src/ipa/simple/soft_simple.cpp > +++ b/src/ipa/simple/soft_simple.cpp > @@ -247,6 +247,16 @@ void IPASoftSimple::stop() > > void IPASoftSimple::processStats(const ControlList &sensorControls) > { > + /* > + * Here black level subtraction, color gains and gamma correction are > + * combined into a single operation (table lookup) to save CPU cycles. > + * This works because black level subtraction and color gains are currently > + * linear operations and we average only same-color pixels in debayering. > + * If we change anything on this or introduce other operations in between, > + * it may be needed to split the operations and perform black and gain > + * corrections before debayering and gamma correction after debayering. > + */ > + > SwIspStats::Histogram histogram = stats_->yHistogram; > if (ignoreUpdates_ > 0) > blackLevel_.update(histogram); > diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO > index 4fcee39b..fcb02588 100644 > --- a/src/libcamera/software_isp/TODO > +++ b/src/libcamera/software_isp/TODO > @@ -267,13 +267,3 @@ This could be handled better with DelayedControls. > > V4L2_CID_EXPOSURE })); > > You should use the DelayedControls class. > - > ---- > - > -13. Improve black level and colour gains application > - > -I think the black level should eventually be moved before debayering, and > -ideally the colour gains as well. I understand the need for optimizations to > -lower the CPU consumption, but at the same time I don't feel comfortable > -building up on top of an implementation that may work a bit more by chance than > -by correctness, as that's not very maintainable.
Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > Hi Milan, > > Thank you for the patch. Hi Laurent, thank you for your comments. > On Tue, Apr 23, 2024 at 08:19:59PM +0200, Milan Zamazal wrote: >> The current software ISP debayering implementation uses color lookup >> tables that apply black level subtraction, color gain and gamma >> correction in a single step, while performing debayering. In theory, >> black level subtraction and color gains should be applied before >> debayering and gamma correction after debayering. But because black >> level subtraction and color gain corrections are currently linear and we >> average only same-color pixels in debayering, we can apply all the >> operations after debayering. The only difference is with clipping where >> out-of-range pixels contribute more than they deserve but this is not >> generally significant. >> >> Combining all the operations at once is conceptually not ideal but it is >> not incorrect in this case and saves CPU cycles, which is critical for >> software ISP CPU implementation (it may be less important with future >> GPU implementation). This means we need the current implementation. It >> may get replaced or become optional (configurable) in future, for >> example if the separation is needed due to introducing other image >> processing operations. >> >> Under these circumstances and considering that the lookup tables >> construction has been moved out of the debayering code in the preceding >> patch, it makes no sense to keep software ISP TODO #13. Let's remove it >> in favor of a clarifying code comment. > > I still think we need to split those operations. The black level > subtraction (coming before debayering and the colour gains) should then > use black level values from a tuning file. An additional contrast > enhancement after debayering can perform automatic histogram stretching > if desired. > > As this is still being discussed, and experimentation is needed to > assess the impact, I think the TODO item should stay until we reach a > conclusion. OK but let's keep it actionable -- IMHO the TODO file should be temporary and the items should be either resolved soon or transformed into something else, such as in-code TODOs. Regards, Milan >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> src/ipa/simple/soft_simple.cpp | 10 ++++++++++ >> src/libcamera/software_isp/TODO | 10 ---------- >> 2 files changed, 10 insertions(+), 10 deletions(-) >> >> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp >> index c5085f71..b2f4d308 100644 >> --- a/src/ipa/simple/soft_simple.cpp >> +++ b/src/ipa/simple/soft_simple.cpp >> @@ -247,6 +247,16 @@ void IPASoftSimple::stop() >> >> void IPASoftSimple::processStats(const ControlList &sensorControls) >> { >> + /* >> + * Here black level subtraction, color gains and gamma correction are >> + * combined into a single operation (table lookup) to save CPU cycles. >> + * This works because black level subtraction and color gains are currently >> + * linear operations and we average only same-color pixels in debayering. >> + * If we change anything on this or introduce other operations in between, >> + * it may be needed to split the operations and perform black and gain >> + * corrections before debayering and gamma correction after debayering. >> + */ >> + >> SwIspStats::Histogram histogram = stats_->yHistogram; >> if (ignoreUpdates_ > 0) >> blackLevel_.update(histogram); >> diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO >> index 4fcee39b..fcb02588 100644 >> --- a/src/libcamera/software_isp/TODO >> +++ b/src/libcamera/software_isp/TODO >> @@ -267,13 +267,3 @@ This could be handled better with DelayedControls. >> > V4L2_CID_EXPOSURE })); >> >> You should use the DelayedControls class. >> - >> ---- >> - >> -13. Improve black level and colour gains application >> - >> -I think the black level should eventually be moved before debayering, and >> -ideally the colour gains as well. I understand the need for optimizations to >> -lower the CPU consumption, but at the same time I don't feel comfortable >> -building up on top of an implementation that may work a bit more by chance than >> -by correctness, as that's not very maintainable.
diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp index c5085f71..b2f4d308 100644 --- a/src/ipa/simple/soft_simple.cpp +++ b/src/ipa/simple/soft_simple.cpp @@ -247,6 +247,16 @@ void IPASoftSimple::stop() void IPASoftSimple::processStats(const ControlList &sensorControls) { + /* + * Here black level subtraction, color gains and gamma correction are + * combined into a single operation (table lookup) to save CPU cycles. + * This works because black level subtraction and color gains are currently + * linear operations and we average only same-color pixels in debayering. + * If we change anything on this or introduce other operations in between, + * it may be needed to split the operations and perform black and gain + * corrections before debayering and gamma correction after debayering. + */ + SwIspStats::Histogram histogram = stats_->yHistogram; if (ignoreUpdates_ > 0) blackLevel_.update(histogram); diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO index 4fcee39b..fcb02588 100644 --- a/src/libcamera/software_isp/TODO +++ b/src/libcamera/software_isp/TODO @@ -267,13 +267,3 @@ This could be handled better with DelayedControls. > V4L2_CID_EXPOSURE })); You should use the DelayedControls class. - ---- - -13. Improve black level and colour gains application - -I think the black level should eventually be moved before debayering, and -ideally the colour gains as well. I understand the need for optimizations to -lower the CPU consumption, but at the same time I don't feel comfortable -building up on top of an implementation that may work a bit more by chance than -by correctness, as that's not very maintainable.
The current software ISP debayering implementation uses color lookup tables that apply black level subtraction, color gain and gamma correction in a single step, while performing debayering. In theory, black level subtraction and color gains should be applied before debayering and gamma correction after debayering. But because black level subtraction and color gain corrections are currently linear and we average only same-color pixels in debayering, we can apply all the operations after debayering. The only difference is with clipping where out-of-range pixels contribute more than they deserve but this is not generally significant. Combining all the operations at once is conceptually not ideal but it is not incorrect in this case and saves CPU cycles, which is critical for software ISP CPU implementation (it may be less important with future GPU implementation). This means we need the current implementation. It may get replaced or become optional (configurable) in future, for example if the separation is needed due to introducing other image processing operations. Under these circumstances and considering that the lookup tables construction has been moved out of the debayering code in the preceding patch, it makes no sense to keep software ISP TODO #13. Let's remove it in favor of a clarifying code comment. Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- src/ipa/simple/soft_simple.cpp | 10 ++++++++++ src/libcamera/software_isp/TODO | 10 ---------- 2 files changed, 10 insertions(+), 10 deletions(-)