[PATCH 4/5] libcamera: software_isp: Remove TODO #13
Milan Zamazal
mzamazal at redhat.com
Mon Apr 29 11:14:49 CEST 2024
Laurent Pinchart <laurent.pinchart at 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 at 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.
More information about the libcamera-devel
mailing list