[PATCH v2 4/5] ipa: simple: Report black levels in metadata

[Milan Zamazal]

Hi Kieran,

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

> Hi Milan,
>
> Quoting Milan Zamazal (2024-12-19 21:10:09)
>> From: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> 
>> Provide the determined black level values in the metadata
>> to add to the completed requests.
>> 
>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
>> ---
>>  src/ipa/simple/algorithms/blc.cpp | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>> 
>> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
>> index 1d7d370b..d5759f92 100644
>> --- a/src/ipa/simple/algorithms/blc.cpp
>> +++ b/src/ipa/simple/algorithms/blc.cpp
>> @@ -11,6 +11,8 @@
>>  
>>  #include <libcamera/base/log.h>
>>  
>> +#include "control_ids.h"
>> +
>>  namespace libcamera {
>>  
>>  namespace ipa::soft::algorithms {
>> @@ -49,8 +51,13 @@ void BlackLevel::process(IPAContext &context,
>>                          [[maybe_unused]] const uint32_t frame,
>>                          IPAFrameContext &frameContext,
>>                          const SwIspStats *stats,
>> -                        [[maybe_unused]] ControlList &metadata)
>> +                        ControlList &metadata)
>>  {
>> +       /* Assign each of the R G G B channels as the same black level. */
>> +       const int32_t blackLevel = context.activeState.blc.level * 256;
>
> What are your thoughts on
> https://patchwork.libcamera.org/project/libcamera/list/?series=4830?
>
> This could then be:
> 	const int32_t blackLevel = context.activeState.blc.level.convert<16>();
>
> or
> 	BitDepthValue<16> blackLevel = context.activeState.blc.level;
>
> I'm curious if you think this provides a helpful / expressive way to
> convey this? or if it obfuscates more ?

I like the general idea of tracking the bit depth explicitly some way,
it's definitely needed, thank you for the reminder.  As for the
particular API proposal, I usually leave making opinions on such matters
to maintainers but I'll try to make my opinion tomorrow since the RFC
deserves more attention than it has got so far.

>> +       const int32_t blackLevels[] = { blackLevel, blackLevel, blackLevel, blackLevel };
>> +       metadata.set(controls::SensorBlackLevels, blackLevels);
>> +
>>         if (context.configuration.black.level.has_value())
>>                 return;
>>  
>> -- 
>> 2.44.2
>>