[libcamera-devel] [RFC PATCH 3/7] libcamera: software_isp: add SwIspLinaro implementation of SoftwareIsp

[Hans de Goede]

Hi Bryan,

On 12/4/23 11:09, Bryan O'Donoghue via libcamera-devel wrote:
> On 04/12/2023 01:10, Andrey Konovalov via libcamera-devel wrote:
>> Signed-off-by: Andrey Konovalov <andrey.konovalov at linaro.org>
>> ---
>>   include/libcamera/internal/meson.build        |   1 +
>>   .../internal/software_isp/meson.build         |   6 +
>>   .../internal/software_isp/statistics-linaro.h |  17 +
>>   .../internal/software_isp/swisp_linaro.h      | 109 ++++
>>   src/libcamera/meson.build                     |   1 +
>>   src/libcamera/software_isp/meson.build        |   5 +
>>   src/libcamera/software_isp/swisp_linaro.cpp   | 539 ++++++++++++++++++
>>   7 files changed, 678 insertions(+)
>>   create mode 100644 include/libcamera/internal/software_isp/meson.build
>>   create mode 100644 include/libcamera/internal/software_isp/statistics-linaro.h
>>   create mode 100644 include/libcamera/internal/software_isp/swisp_linaro.h
>>   create mode 100644 src/libcamera/software_isp/meson.build
>>   create mode 100644 src/libcamera/software_isp/swisp_linaro.cpp
>>
>> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
>> index b780777c..eeae801c 100644
>> --- a/include/libcamera/internal/meson.build
>> +++ b/include/libcamera/internal/meson.build
> 
>> +void SwIspLinaro::IspWorker::debayerRaw10P(uint8_t *dst, const uint8_t *src)
>> +{
>> +    /* for brightness values in the 0 to 255 range: */
>> +    static const unsigned int BRIGHT_LVL = 200U << 8;
>> +    static const unsigned int TOO_BRIGHT_LVL = 240U << 8;
>> +
>> +    static const unsigned int RED_Y_MUL = 77;    /* 0.30 * 256 */
>> +    static const unsigned int GREEN_Y_MUL = 150;    /* 0.59 * 256 */
>> +    static const unsigned int BLUE_Y_MUL = 29;    /* 0.11 * 256 */
> 
> These seem like magic numbers that deserve a little bit more explanation.
> 
> Why is blue 0.11 and not 0.10 for example ?

These are the standard weights for calculating luminance aka the
Y in YUV when coming from RGB values, see e.g. :

https://www.pcmag.com/encyclopedia/term/yuvrgb-conversion-formulas

Might still be worth a comment, but for someone who is familiar
with this kinda stuff these weights are immediately recognizable.

Regards,

Hans








> 
> Same comment with the brightness value - why these numbers ? Does no harm to add in some of the thinking behind why you've choosen these particular values.
> 
>> +
>> +int SwIspLinaro::queueBuffers(FrameBuffer *input,
>> +                  const std::map<unsigned int, FrameBuffer *> &outputs)
>> +{
>> +    unsigned int mask = 0;
>> +
>> +    /*
>> +     * Validate the outputs as a sanity check: at least one output is
>> +     * required, all outputs must reference a valid stream and no two
>> +     * outputs can reference the same stream.
>> +     */
>> +    if (outputs.empty())
>> +        return -EINVAL;
>> +
>> +    for (auto [index, buffer] : outputs) {
>> +        if (!buffer)
>> +            return -EINVAL;
>> +        if (index >= 1) /* only single stream atm */
>> +            return -EINVAL;
> 
> Single stream per sensor ? Or single stream per instance of the class, which is ~ the same thing... ?
> 
> Anyway maybe make that clearer in the comment.
> 
> General comment on the debayer part is more comments please. The logic of the algorithm deserves a bit more explanation.
> 
> ---
> bod
>