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

[Andrey Konovalov]

Hi Bryan and Hans,

On 04.12.2023 13:51, Hans de Goede wrote:
> 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.

I'll add the comment in the next version of the patch set.

> 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... ?

This isn't a fundamental limitation, but yes, the current version only support 1 stream per sensor and the
instance.

>> 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.

This patch set doesn't have a lot of comments indeed. I'll be adding more comments in the next version.

Thanks,
Andrey

>> ---
>> bod
>>
>