[{"id":28234,"web_url":"https://patchwork.libcamera.org/comment/28234/","msgid":"<9fe56456-6e0c-4db6-ba16-d7855df34ed8@nexus-software.ie>","date":"2023-12-04T10:09:45","subject":"Re: [libcamera-devel] [RFC PATCH 3/7] libcamera: software_isp: add\n\tSwIspLinaro implementation of SoftwareIsp","submitter":{"id":176,"url":"https://patchwork.libcamera.org/api/people/176/","name":"Bryan O'Donoghue","email":"pure.logic@nexus-software.ie"},"content":"On 04/12/2023 01:10, Andrey Konovalov via libcamera-devel wrote:\n> Signed-off-by: Andrey Konovalov \n> ---\n> include/libcamera/internal/meson.build | 1 +\n> .../internal/software_isp/meson.build | 6 +\n> .../internal/software_isp/statistics-linaro.h | 17 +\n> .../internal/software_isp/swisp_linaro.h | 109 ++++\n> src/libcamera/meson.build | 1 +\n> src/libcamera/software_isp/meson.build | 5 +\n> src/libcamera/software_isp/swisp_linaro.cpp | 539 ++++++++++++++++++\n> 7 files changed, 678 insertions(+)\n> create mode 100644 include/libcamera/internal/software_isp/meson.build\n> create mode 100644 include/libcamera/internal/software_isp/statistics-linaro.h\n> create mode 100644 include/libcamera/internal/software_isp/swisp_linaro.h\n> create mode 100644 src/libcamera/software_isp/meson.build\n> create mode 100644 src/libcamera/software_isp/swisp_linaro.cpp\n> \n> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> index b780777c..eeae801c 100644\n> --- a/include/libcamera/internal/meson.build\n> +++ b/include/libcamera/internal/meson.build\n\n> +void SwIspLinaro::IspWorker::debayerRaw10P(uint8_t *dst, const uint8_t *src)\n> +{\n> +\t/* for brightness values in the 0 to 255 range: */\n> +\tstatic const unsigned int BRIGHT_LVL = 200U << 8;\n> +\tstatic const unsigned int TOO_BRIGHT_LVL = 240U << 8;\n> +\n> +\tstatic const unsigned int RED_Y_MUL = 77;\t/* 0.30 * 256 */\n> +\tstatic const unsigned int GREEN_Y_MUL = 150;\t/* 0.59 * 256 */\n> +\tstatic const unsigned int BLUE_Y_MUL = 29;\t/* 0.11 * 256 */\n\nThese seem like magic numbers that deserve a little bit more explanation.\n\nWhy is blue 0.11 and not 0.10 for example ?\n\nSame comment with the brightness value - why these numbers ? Does no \nharm to add in some of the thinking behind why you've choosen these \nparticular values.\n\n> +\n> +int SwIspLinaro::queueBuffers(FrameBuffer *input,\n> +\t\t\t const std::map &outputs)\n> +{\n> +\tunsigned int mask = 0;\n> +\n> +\t/*\n> +\t * Validate the outputs as a sanity check: at least one output is\n> +\t * required, all outputs must reference a valid stream and no two\n> +\t * outputs can reference the same stream.\n> +\t */\n> +\tif (outputs.empty())\n> +\t\treturn -EINVAL;\n> +\n> +\tfor (auto [index, buffer] : outputs) {\n> +\t\tif (!buffer)\n> +\t\t\treturn -EINVAL;\n> +\t\tif (index >= 1) /* only single stream atm */\n> +\t\t\treturn -EINVAL;\n\nSingle stream per sensor ? Or single stream per instance of the class, \nwhich is ~ the same thing... ?\n\nAnyway maybe make that clearer in the comment.\n\nGeneral comment on the debayer part is more comments please. The logic \nof the algorithm deserves a bit more explanation.\n\n---\nbod","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 74E93C31E9\n\tfor ;\n\tMon, 4 Dec 2023 10:09:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 47F1B629BC;\n\tMon, 4 Dec 2023 11:09:48 +0100 (CET)","from mail-ed1-x52e.google.com (mail-ed1-x52e.google.com\n\t[IPv6:2a00:1450:4864:20::52e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E4817629BC\n\tfor ;\n\tMon, 4 Dec 2023 11:09:45 +0100 (CET)","by mail-ed1-x52e.google.com with SMTP id\n\t4fb4d7f45d1cf-54917ef6c05so5319252a12.1\n\tfor ;\n\tMon, 04 Dec 2023 02:09:45 -0800 (PST)","from ?IPV6:2001:1c06:2302:5600:366d:ca8f:f3af:381?\n\t(2001-1c06-2302-5600-366d-ca8f-f3af-0381.cable.dynamic.v6.ziggo.nl.\n\t[2001:1c06:2302:5600:366d:ca8f:f3af:381])\n\tby smtp.gmail.com with ESMTPSA id\n\tgq18-20020a170906e25200b00a0988d69549sm5101582ejb.189.2023.12.04.02.09.44\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tMon, 04 Dec 2023 02:09:44 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1701684588;\n\tbh=u7/II5i/+nwGtv0mg8FdRjwAa0c2ymo4UPxT3nf1A5Q=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=X5qrzavnNeTsO9xxf1sxXv4jLlqwKRgJOHINaXzcMadHkCsJ1lBWYsX/gCBGYPVUI\n\ts0hhHZAof8Ils0OcyLnIDr1YLmwhbVOIXXgJKH915a4N7V5mFqEFFPz2kZevCq7V1V\n\tEUOuZmWuztiBYNmYrhvQ80rkjkKRg3714dvRKPoOQdbyniz6iUtOtraHzmLwbyNpDc\n\t56gd/BLfRjn/hsHDPgarIsQD73tiKldqSXOrqo4Q4V696ktf9MxYXiqwj1mVpn3UGS\n\tInyJIBH6trGWDcqyQRy1ug5Mw/rizAOVVhfABloCm9fFcj8zBETFYCHY6LeOXVu/bb\n\t+qZNFX+AJFmDg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=nexus-software-ie.20230601.gappssmtp.com; s=20230601; t=1701684585; \n\tx=1702289385; darn=lists.libcamera.org; \n\th=content-transfer-encoding:in-reply-to:from:references:cc:to\n\t:content-language:subject:user-agent:mime-version:date:message-id\n\t:from:to:cc:subject:date:message-id:reply-to;\n\tbh=l5FNa8w+vnJWNnpcqdZAqVQN8XrftW2kDmNogJIAQGk=;\n\tb=b8zylGql5VIzNeE8BuGNfz5geYAHi98qR/Z01JYLO5RYO52wbdD5YqME6XE7qX46Vi\n\tPFhmJ8chbygyYgri+KCarf/oQ4M9t/V0R3R1BKEAyLcM0SiR7bzhoC/AHUZA87o80AL3\n\tzW0923A2pUqVJtJNQzbOTTisSoDGrMmV1YBbI0Q2sqTCneKIvXfcImWp+Tmu/7fRC7Qe\n\tXF7DyxJ6OKyEM4OYX9wqG4HQ/tew/E23f3gMDL1sNg/xBWy2jdwlFN4g4X99fTuBEnMK\n\tiyaCmEAAwVpDkfxvWO4Ir1LHdYonDtS9GKzWs7aEtqpZ35mr+xgg9wE598cfYfhgNioY\n\tDOlg=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=nexus-software-ie.20230601.gappssmtp.com\n\theader.i=@nexus-software-ie.20230601.gappssmtp.com\n\theader.b=\"b8zylGql\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1701684585; x=1702289385;\n\th=content-transfer-encoding:in-reply-to:from:references:cc:to\n\t:content-language:subject:user-agent:mime-version:date:message-id\n\t:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;\n\tbh=l5FNa8w+vnJWNnpcqdZAqVQN8XrftW2kDmNogJIAQGk=;\n\tb=I0GySZvTh3c52YS1gBsFMv6obCg1bg5RfQPFzsHPJ8iX2GC49ygnMoPIXGE/A1tNmb\n\twO4lcp7hw4yKLS+//qoO+95q6uFE5WIj5LJyrfkDgkFXmlcag+Fd/5STv1vwISY+rdNc\n\tXjbM3f6kqddY2P3FzmuoHCKrLYzqNy128me42Wv1VrhPgpSRL4hGoDmg363RZhNzH8FT\n\tOwic0FiZx36HxClVZ5oVk8ydhAm2LzJbiIcvqFDmQyw+27jXX9vyRTq9ZSnYFmy6duWo\n\tsT7/AAbNG5MoSls2FMYtcqBJenpMAP/lMQxOrZb9EdiuQWeRaOfmnz0zTtKlFlAlvSCc\n\tJqIw==","X-Gm-Message-State":"AOJu0Yyc29RLMwmL5sAn9MAvl+Gc5ppddgEns/eE4tmGr5WzBBvKdnKj\n\tEFXlK0x+kHrFVVr0NH+R31ezGg==","X-Google-Smtp-Source":"AGHT+IGWYciDDDI5bGTm1SJ5YmA8VxINx8GfIeWIZofxoJV7Q2d5PuVlhSbfOeIubhIaslvOH1yZlg==","X-Received":"by 2002:a17:906:74ca:b0:a16:3410:3d47 with SMTP id\n\tz10-20020a17090674ca00b00a1634103d47mr2280670ejl.8.1701684585310; \n\tMon, 04 Dec 2023 02:09:45 -0800 (PST)","Message-ID":"<9fe56456-6e0c-4db6-ba16-d7855df34ed8@nexus-software.ie>","Date":"Mon, 4 Dec 2023 11:09:45 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Content-Language":"en-US","To":"Andrey Konovalov ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20231204001013.404720-1-andrey.konovalov@linaro.org>\n\t<20231204001013.404720-4-andrey.konovalov@linaro.org>","In-Reply-To":"<20231204001013.404720-4-andrey.konovalov@linaro.org>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [RFC PATCH 3/7] libcamera: software_isp: add\n\tSwIspLinaro implementation of SoftwareIsp","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Bryan O'Donoghue via libcamera-devel\n\t","Reply-To":"Bryan O'Donoghue ","Cc":"mripard@redhat.com, g.martti@gmail.com, t.langendam@gmail.com,\n\tsrinivas.kandagatla@linaro.org, bryan.odonoghue@linaro.org,\n\tadmin@dennisbonke.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28235,"web_url":"https://patchwork.libcamera.org/comment/28235/","msgid":"<93cafb1e-921e-4cbd-a98b-b708041b99db@redhat.com>","date":"2023-12-04T10:51:13","subject":"Re: [libcamera-devel] [RFC PATCH 3/7] libcamera: software_isp: add\n\tSwIspLinaro implementation of SoftwareIsp","submitter":{"id":102,"url":"https://patchwork.libcamera.org/api/people/102/","name":"Hans de Goede","email":"hdegoede@redhat.com"},"content":"Hi Bryan,\n\nOn 12/4/23 11:09, Bryan O'Donoghue via libcamera-devel wrote:\n> On 04/12/2023 01:10, Andrey Konovalov via libcamera-devel wrote:\n>> Signed-off-by: Andrey Konovalov \n>> ---\n>>   include/libcamera/internal/meson.build        |   1 +\n>>   .../internal/software_isp/meson.build         |   6 +\n>>   .../internal/software_isp/statistics-linaro.h |  17 +\n>>   .../internal/software_isp/swisp_linaro.h      | 109 ++++\n>>   src/libcamera/meson.build                     |   1 +\n>>   src/libcamera/software_isp/meson.build        |   5 +\n>>   src/libcamera/software_isp/swisp_linaro.cpp   | 539 ++++++++++++++++++\n>>   7 files changed, 678 insertions(+)\n>>   create mode 100644 include/libcamera/internal/software_isp/meson.build\n>>   create mode 100644 include/libcamera/internal/software_isp/statistics-linaro.h\n>>   create mode 100644 include/libcamera/internal/software_isp/swisp_linaro.h\n>>   create mode 100644 src/libcamera/software_isp/meson.build\n>>   create mode 100644 src/libcamera/software_isp/swisp_linaro.cpp\n>>\n>> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n>> index b780777c..eeae801c 100644\n>> --- a/include/libcamera/internal/meson.build\n>> +++ b/include/libcamera/internal/meson.build\n> \n>> +void SwIspLinaro::IspWorker::debayerRaw10P(uint8_t *dst, const uint8_t *src)\n>> +{\n>> +    /* for brightness values in the 0 to 255 range: */\n>> +    static const unsigned int BRIGHT_LVL = 200U << 8;\n>> +    static const unsigned int TOO_BRIGHT_LVL = 240U << 8;\n>> +\n>> +    static const unsigned int RED_Y_MUL = 77;    /* 0.30 * 256 */\n>> +    static const unsigned int GREEN_Y_MUL = 150;    /* 0.59 * 256 */\n>> +    static const unsigned int BLUE_Y_MUL = 29;    /* 0.11 * 256 */\n> \n> These seem like magic numbers that deserve a little bit more explanation.\n> \n> Why is blue 0.11 and not 0.10 for example ?\n\nThese are the standard weights for calculating luminance aka the\nY in YUV when coming from RGB values, see e.g. :\n\nhttps://www.pcmag.com/encyclopedia/term/yuvrgb-conversion-formulas\n\nMight still be worth a comment, but for someone who is familiar\nwith this kinda stuff these weights are immediately recognizable.\n\nRegards,\n\nHans\n\n\n\n\n\n\n\n\n> \n> 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.\n> \n>> +\n>> +int SwIspLinaro::queueBuffers(FrameBuffer *input,\n>> +                  const std::map &outputs)\n>> +{\n>> +    unsigned int mask = 0;\n>> +\n>> +    /*\n>> +     * Validate the outputs as a sanity check: at least one output is\n>> +     * required, all outputs must reference a valid stream and no two\n>> +     * outputs can reference the same stream.\n>> +     */\n>> +    if (outputs.empty())\n>> +        return -EINVAL;\n>> +\n>> +    for (auto [index, buffer] : outputs) {\n>> +        if (!buffer)\n>> +            return -EINVAL;\n>> +        if (index >= 1) /* only single stream atm */\n>> +            return -EINVAL;\n> \n> Single stream per sensor ? Or single stream per instance of the class, which is ~ the same thing... ?\n> \n> Anyway maybe make that clearer in the comment.\n> \n> General comment on the debayer part is more comments please. The logic of the algorithm deserves a bit more explanation.\n> \n> ---\n> bod\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A8F4EC3226\n\tfor ;\n\tMon, 4 Dec 2023 10:51:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 34724629BC;\n\tMon, 4 Dec 2023 11:51:21 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 79B4F629BC\n\tfor ;\n\tMon, 4 Dec 2023 11:51:18 +0100 (CET)","from mail-ed1-f70.google.com (mail-ed1-f70.google.com\n\t[209.85.208.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-589-B0ASWxCvNOGtFaBONFIsgQ-1; Mon, 04 Dec 2023 05:51:15 -0500","by mail-ed1-f70.google.com with SMTP id\n\t4fb4d7f45d1cf-5411d71889aso3286888a12.2\n\tfor ;\n\tMon, 04 Dec 2023 02:51:15 -0800 (PST)","from [10.40.98.142] ([78.108.130.194])\n\tby smtp.gmail.com with ESMTPSA id\n\tdh9-20020a0564021d2900b0054c86f882bcsm2536502edb.22.2023.12.04.02.51.13\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tMon, 04 Dec 2023 02:51:14 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1701687081;\n\tbh=dHGiTQjFlyQRxlR+WAKmjsgzm3AJRex0xx9VqwosNOc=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=d8zwwEvZVNGrEzBmdydv0lWRF16l1HBM74RyOUop16mF86cg3CkFPeSt/4hFQ0RUr\n\tqVrzOy7PYTpX7Bx7R+G+ry4V3qrdny72i5hNvRgVvB2WyLnwtCPLEqkl7m4T8nLGXo\n\tmX/hCLGH9LuY5QB2lspVGA9CmMI5ufsbxzbopj6DMH0xlGDquOsIBXPgaH00GPNXxn\n\tiJ2zXSUg74RKjxf0mWAQ8oiYVXFvD9CqrAcRmlpYCwtQiwvwvT1GcpBHcjFsx/vFis\n\toCzyxIOCEpG82uM4hJfom4G6lrXfV+olhA2PWm65Fm2zRUHPBJ5uqy6Taw1lpyr3ZW\n\tnQenoD7W0pDRw==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1701687077;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tcontent-transfer-encoding:content-transfer-encoding:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=8iuI11cfaHutLfewpxM2KiCO8zwj1ABa7LtrvUxDdjk=;\n\tb=R8lxYgi5CgXE50DzK3LYfaaIZZJridgMp3NGJYWs9dLRpkXFh74B10m24A/4HiJIndIBcE\n\tmbxdHIP2S2+Dux6eAMBZyp7935ecA+d2q60Hit8egucAHQzcHCXgkCysU5ER8g0nL2KhBt\n\tazDkGzKH0qXKN6m3/j7/8Aeja702W74="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=redhat.com\n\theader.i=@redhat.com header.b=\"R8lxYgi5\"; \n\tdkim-atps=neutral","X-MC-Unique":"B0ASWxCvNOGtFaBONFIsgQ-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1701687075; x=1702291875;\n\th=content-transfer-encoding:in-reply-to:from:references:cc:to\n\t:content-language:subject:user-agent:mime-version:date:message-id\n\t:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;\n\tbh=8iuI11cfaHutLfewpxM2KiCO8zwj1ABa7LtrvUxDdjk=;\n\tb=iwKt1Kdy6tOkqv1ApHX8InhXN/kRWmfyqdgLpS/sfO9xbfcFJe0d1zKVFTAog+u6nY\n\tbFpwEFiR5fEr6RS+j8q6Q60Oj1RTi3EujaYhW7zRgzfWfHYua1aCMs+9QZA7PaDsTpw0\n\t3EWHVJKncwa9gViBaSC5pU+Hlu5CRtPws3RDpJ1NbifEOt7VqqsPQxhgeT1WCYSWczYJ\n\trqtxWWLKTrs4ZaRE5g7BDh85eH0VS9s0cf9fX43Nc5TrkWuDflSwHyn4juUmuRtFhagI\n\tms8vc1dioOcGW/2lOhiOWCLjCEpS00eGzzLEdeFM8uRH0EoRjH9MQM0DOn83Z2GEPGUf\n\tC59A==","X-Gm-Message-State":"AOJu0YwnrHS1EBbniDTgSmsdGGc/Gq0BRf7STfn74I/AlkVXBzJEyR4C\n\t28W44b12c6demfmJKBVYJP/ohCPZNYA2N8IqanYEZZi6O/VC9VLPY57ElAXrdOAebUeK1rr7JT/\n\tTpFjamuVs2hRwJQPBvK5OKMtnS+WF4JrCEA==","X-Received":["by 2002:a50:9304:0:b0:54a:ff96:2cba with SMTP id\n\tm4-20020a509304000000b0054aff962cbamr2504161eda.33.1701687074865; \n\tMon, 04 Dec 2023 02:51:14 -0800 (PST)","by 2002:a50:9304:0:b0:54a:ff96:2cba with SMTP id\n\tm4-20020a509304000000b0054aff962cbamr2504156eda.33.1701687074592; \n\tMon, 04 Dec 2023 02:51:14 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IGzpp8/38pKP+T8e/1AVLNITgV/AkuocnFYBD6h/GnjJ1oGtnbvYO6Q6uRqA2MPN0uC9neR4g==","Message-ID":"<93cafb1e-921e-4cbd-a98b-b708041b99db@redhat.com>","Date":"Mon, 4 Dec 2023 11:51:13 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","To":"Bryan O'Donoghue ,\n\tAndrey Konovalov ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20231204001013.404720-1-andrey.konovalov@linaro.org>\n\t<20231204001013.404720-4-andrey.konovalov@linaro.org>\n\t<9fe56456-6e0c-4db6-ba16-d7855df34ed8@nexus-software.ie>","In-Reply-To":"<9fe56456-6e0c-4db6-ba16-d7855df34ed8@nexus-software.ie>","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Language":"en-US","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [RFC PATCH 3/7] libcamera: software_isp: add\n\tSwIspLinaro implementation of SoftwareIsp","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Hans de Goede via libcamera-devel ","Reply-To":"Hans de Goede ","Cc":"mripard@redhat.com, g.martti@gmail.com, t.langendam@gmail.com,\n\tsrinivas.kandagatla@linaro.org, bryan.odonoghue@linaro.org,\n\tadmin@dennisbonke.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28237,"web_url":"https://patchwork.libcamera.org/comment/28237/","msgid":"<679205f1-06e5-4058-9005-27ab3c5a3244@linaro.org>","date":"2023-12-04T13:19:33","subject":"Re: [libcamera-devel] [RFC PATCH 3/7] libcamera: software_isp: add\n\tSwIspLinaro implementation of SoftwareIsp","submitter":{"id":25,"url":"https://patchwork.libcamera.org/api/people/25/","name":"Andrey Konovalov","email":"andrey.konovalov@linaro.org"},"content":"Hi Bryan and Hans,\n\nOn 04.12.2023 13:51, Hans de Goede wrote:\n> Hi Bryan,\n> \n> On 12/4/23 11:09, Bryan O'Donoghue via libcamera-devel wrote:\n>> On 04/12/2023 01:10, Andrey Konovalov via libcamera-devel wrote:\n>>> Signed-off-by: Andrey Konovalov \n>>> ---\n>>>   include/libcamera/internal/meson.build        |   1 +\n>>>   .../internal/software_isp/meson.build         |   6 +\n>>>   .../internal/software_isp/statistics-linaro.h |  17 +\n>>>   .../internal/software_isp/swisp_linaro.h      | 109 ++++\n>>>   src/libcamera/meson.build                     |   1 +\n>>>   src/libcamera/software_isp/meson.build        |   5 +\n>>>   src/libcamera/software_isp/swisp_linaro.cpp   | 539 ++++++++++++++++++\n>>>   7 files changed, 678 insertions(+)\n>>>   create mode 100644 include/libcamera/internal/software_isp/meson.build\n>>>   create mode 100644 include/libcamera/internal/software_isp/statistics-linaro.h\n>>>   create mode 100644 include/libcamera/internal/software_isp/swisp_linaro.h\n>>>   create mode 100644 src/libcamera/software_isp/meson.build\n>>>   create mode 100644 src/libcamera/software_isp/swisp_linaro.cpp\n>>>\n>>> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n>>> index b780777c..eeae801c 100644\n>>> --- a/include/libcamera/internal/meson.build\n>>> +++ b/include/libcamera/internal/meson.build\n>>\n>>> +void SwIspLinaro::IspWorker::debayerRaw10P(uint8_t *dst, const uint8_t *src)\n>>> +{\n>>> +    /* for brightness values in the 0 to 255 range: */\n>>> +    static const unsigned int BRIGHT_LVL = 200U << 8;\n>>> +    static const unsigned int TOO_BRIGHT_LVL = 240U << 8;\n>>> +\n>>> +    static const unsigned int RED_Y_MUL = 77;    /* 0.30 * 256 */\n>>> +    static const unsigned int GREEN_Y_MUL = 150;    /* 0.59 * 256 */\n>>> +    static const unsigned int BLUE_Y_MUL = 29;    /* 0.11 * 256 */\n>>\n>> These seem like magic numbers that deserve a little bit more explanation.\n>>\n>> Why is blue 0.11 and not 0.10 for example ?\n> \n> These are the standard weights for calculating luminance aka the\n> Y in YUV when coming from RGB values, see e.g. :\n> \n> https://www.pcmag.com/encyclopedia/term/yuvrgb-conversion-formulas\n> \n> Might still be worth a comment, but for someone who is familiar\n> with this kinda stuff these weights are immediately recognizable.\n\nI'll add the comment in the next version of the patch set.\n\n> Regards,\n> \n> Hans\n> \n> \n> \n> \n>>\n>> 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.\n>>\n>>> +\n>>> +int SwIspLinaro::queueBuffers(FrameBuffer *input,\n>>> +                  const std::map &outputs)\n>>> +{\n>>> +    unsigned int mask = 0;\n>>> +\n>>> +    /*\n>>> +     * Validate the outputs as a sanity check: at least one output is\n>>> +     * required, all outputs must reference a valid stream and no two\n>>> +     * outputs can reference the same stream.\n>>> +     */\n>>> +    if (outputs.empty())\n>>> +        return -EINVAL;\n>>> +\n>>> +    for (auto [index, buffer] : outputs) {\n>>> +        if (!buffer)\n>>> +            return -EINVAL;\n>>> +        if (index >= 1) /* only single stream atm */\n>>> +            return -EINVAL;\n>>\n>> Single stream per sensor ? Or single stream per instance of the class, which is ~ the same thing... ?\n\nThis isn't a fundamental limitation, but yes, the current version only support 1 stream per sensor and the\ninstance.\n\n>> Anyway maybe make that clearer in the comment.\n>>\n>> General comment on the debayer part is more comments please. The logic of the algorithm deserves a bit more explanation.\n\nThis patch set doesn't have a lot of comments indeed. I'll be adding more comments in the next version.\n\nThanks,\nAndrey\n\n>> ---\n>> bod\n>>\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3EEC3C3226\n\tfor ;\n\tMon, 4 Dec 2023 13:19:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 90F7D629C2;\n\tMon, 4 Dec 2023 14:19:37 +0100 (CET)","from mail-wr1-x435.google.com (mail-wr1-x435.google.com\n\t[IPv6:2a00:1450:4864:20::435])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 548CC61D9F\n\tfor ;\n\tMon, 4 Dec 2023 14:19:35 +0100 (CET)","by mail-wr1-x435.google.com with SMTP id\n\tffacd0b85a97d-33338c47134so1807405f8f.1\n\tfor ;\n\tMon, 04 Dec 2023 05:19:35 -0800 (PST)","from [192.168.118.20] ([87.116.166.54])\n\tby smtp.gmail.com with ESMTPSA id\n\tx18-20020adff0d2000000b003333b67f58csm6288196wro.48.2023.12.04.05.19.34\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tMon, 04 Dec 2023 05:19:34 -0800 (PST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1701695977;\n\tbh=fricUPUfLOwxNBYNqW1oPfcK+obASVzIoaf7Cbe2nko=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=ZsPuKsFVdL4Xl1U2sPx2UZHHb2r5/NhkYxgrRvCoyI0bxpY7ocZi4d/nhJXpco1Bd\n\tdeecxZPO6p18A/owRd0U2csyNZVSjZEBMr2vDANznOo1/TQNq11HnYmlg3bgNt6PNk\n\t6G7WrabyINlOV0mseHiOJau4fDU6mhkSzyAX0QaI+VkgcDDCsrkiLf8+EW1gGOj1K+\n\tBXyfFeV2s++dz2G9dQdvvbwexG45LlleMnYYXxtS+M1Aa/gZV+XA2gZqijeQ7QvMPD\n\tzEBoF6fBP7fdwHtGDJ+7SbRwTNlNYdCzPT+YDxDq2nv6wcuCV5efYtOWPwmIIjZ6kO\n\t6/q82iK5S9fNw==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=linaro.org; s=google; t=1701695975; x=1702300775;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:in-reply-to:from:references:cc:to\n\t:content-language:subject:user-agent:mime-version:date:message-id\n\t:from:to:cc:subject:date:message-id:reply-to;\n\tbh=tqeS8rfYXmfT1d+1vx7+21er1fEVHNBd+QNMbYoyHco=;\n\tb=EEpbqmV+gSp81MUibOuhTt/WZg44anhlF6BfKMUfPLuhbfd/HRHsQugXhLkBGQL/mG\n\t4v1nl3pzlsV8DoYnD/o7+7wj51N5oTh7jZOZ/FX5u1AIRfcMixBPnEHy3e3P5nJhziJw\n\t17yBCKmKlqTy1Cke1kDagWneYK4NRNUEKu0/VuBRLuz76BCrk+5d9Q8kNHF2SqorLIzd\n\tctzMLFUlqIBvKmcrce/6AFFEfJZtQPgJKxKl5Q2YM3jCt6xMNyOt95WNedV9fZUIR2e5\n\tN6Y+1nAfrqRFGd8FBIgx7k1sgFDAmZ5mPJ8RTlapPdm55eet7FtL+Ji35CaXzu9iga7j\n\tDY0A=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=linaro.org\n\theader.i=@linaro.org header.b=\"EEpbqmV+\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1701695975; x=1702300775;\n\th=content-transfer-encoding:in-reply-to:from:references:cc:to\n\t:content-language:subject:user-agent:mime-version:date:message-id\n\t:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;\n\tbh=tqeS8rfYXmfT1d+1vx7+21er1fEVHNBd+QNMbYoyHco=;\n\tb=Uj1sh1j+SsqCXKUbGXzwE14N1Fbqv7z7Alvg62XCfgpkFvDTvRzg7zn42eUzIXiCrx\n\t3VdWvG7X2FrxwHU3GoKXru8owbNhPWF8AAIYnzvk/6YgBnk7pzhZ0hCNUG/087Lz03VY\n\tBy7ltjND44ILUCRJUer2i4gIjQ6yGuuRT9WLS+2XteE6yFV9rtcrfHZ7RxO72to8LFjF\n\tfdQy6t2Xpr+CoeGyaHvNyjlx5AHApWLiag7Eytx5KMU/+cCclLVJfMvfpLD2z7e/MEoH\n\t8ARll8T/KW4StDXNbSQYBJ/ekiWNjvj1nVRPlWIob4K+27m1nRmGmO/Kzu5Jbbnp4cQc\n\tQu6g==","X-Gm-Message-State":"AOJu0Yy3AeTLiSejuexradWea/Vxvb30LnO5FQPaDTrazQ++guynrG2p\n\tw4mMOsMf1Fg11nWMAPYj3DC4lQ==","X-Google-Smtp-Source":"AGHT+IFQdF/XXBdquoFIQm3mFM5lKum49qoW6LyQHVaDiOJ246OtAnqmXBUBIEVJ2Y7LGctfGB6cEw==","X-Received":"by 2002:a05:6000:71e:b0:333:4cfa:1dc4 with SMTP id\n\tbs30-20020a056000071e00b003334cfa1dc4mr785988wrb.75.1701695974824; \n\tMon, 04 Dec 2023 05:19:34 -0800 (PST)","Message-ID":"<679205f1-06e5-4058-9005-27ab3c5a3244@linaro.org>","Date":"Mon, 4 Dec 2023 16:19:33 +0300","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Content-Language":"en-US","To":"Hans de Goede ,\n\tBryan O'Donoghue ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20231204001013.404720-1-andrey.konovalov@linaro.org>\n\t<20231204001013.404720-4-andrey.konovalov@linaro.org>\n\t<9fe56456-6e0c-4db6-ba16-d7855df34ed8@nexus-software.ie>\n\t<93cafb1e-921e-4cbd-a98b-b708041b99db@redhat.com>","In-Reply-To":"<93cafb1e-921e-4cbd-a98b-b708041b99db@redhat.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [RFC PATCH 3/7] libcamera: software_isp: add\n\tSwIspLinaro implementation of SoftwareIsp","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Andrey Konovalov via libcamera-devel\n\t","Reply-To":"Andrey Konovalov ","Cc":"mripard@redhat.com, g.martti@gmail.com, t.langendam@gmail.com,\n\tsrinivas.kandagatla@linaro.org, bryan.odonoghue@linaro.org,\n\tadmin@dennisbonke.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]