[{"id":24027,"web_url":"https://patchwork.libcamera.org/comment/24027/","msgid":"<165840891929.2228597.14693711504023462292@Monstersaurus>","date":"2022-07-21T13:08:39","subject":"Re: [libcamera-devel] [PATCH v2 3/4] ipa: rkisp1: Add support of\n\tDemosaicing control","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Florian,\n\nQuoting Florian Sylvestre via libcamera-devel (2022-07-20 16:42:20)\n> During the demosaicing step, rkisp1 ISP is processing denoising and sharpness\n> control.\n> Add demosaicing algorithm with denoise and sharpness values based on user\n> controls.\n> \n\nThis one however, is more directly related to where the data is stored.\n\nIn my series I've moved the existing 'frame context' to the 'active\nstate' ... which is an area for algorithms to store data that may be\naccessed from other algorithms. This data may not fit there now.\n\nWould you be able to look at how this would operate on top of a 'per\nframe, frame context' and see if it still functions for you ?\n\nI've pushed my branch to :\n\n\n Repo\tgithub.com:kbingham/libcamera.git\n Branch:\tkbingham/pfc\n\nto make it easier for you to access.\n\n--\nKieran\n\n\n\n\n> Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>\n> ---\n>  src/ipa/rkisp1/algorithms/demosaicing.cpp | 197 ++++++++++++++++++++++\n>  src/ipa/rkisp1/algorithms/demosaicing.h   |  30 ++++\n>  src/ipa/rkisp1/algorithms/meson.build     |   1 +\n>  src/ipa/rkisp1/data/ov5640.yaml           |   1 +\n>  src/ipa/rkisp1/ipa_context.h              |   6 +\n>  src/ipa/rkisp1/rkisp1.cpp                 |   1 +\n>  src/libcamera/pipeline/rkisp1/rkisp1.cpp  |   8 +\n>  7 files changed, 244 insertions(+)\n>  create mode 100644 src/ipa/rkisp1/algorithms/demosaicing.cpp\n>  create mode 100644 src/ipa/rkisp1/algorithms/demosaicing.h\n> \n> diff --git a/src/ipa/rkisp1/algorithms/demosaicing.cpp b/src/ipa/rkisp1/algorithms/demosaicing.cpp\n> new file mode 100644\n> index 00000000..7d55eaab\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/algorithms/demosaicing.cpp\n> @@ -0,0 +1,197 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021-2022, Ideas On Board\n> + *\n> + * demosaicing.cpp - RkISP1 Demosaicing control\n> + */\n> +\n> +#include \"demosaicing.h\"\n> +\n> +#include <libcamera/base/log.h>\n> +\n> +#include <libcamera/control_ids.h>\n> +\n> +#include \"libcamera/internal/yaml_parser.h\"\n> +\n> +/**\n> + * \\file demosaicing.h\n> + */\n> +\n> +static constexpr uint32_t kFiltLumWeightDefault = 0x00022040;\n> +static constexpr uint32_t kFiltModeDefault = 0x000004f2;\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::rkisp1::algorithms {\n> +\n> +/**\n> + * \\class Demosaicing\n> + * \\brief RkISP1 Demosaicing control\n> + *\n> + * The Demosaicing algorithm is responsible for reconstructing a full color\n> + * image from the sensor raw data. During the demosaicing step, The RKISP1\n> + * will additionally process denoise and sharpness controls.\n> + *\n> + * /todo In current version the denoise and sharpness control is based on user\n> + * controls. In a future version it should be controlled automatically by the\n> + * algorithm.\n> + */\n> +\n> +LOG_DEFINE_CATEGORY(RkISP1Demosaicing)\n> +\n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::queueRequest\n> + */\n> +void Demosaicing::queueRequest([[maybe_unused]] IPAContext &context,\n> +                              [[maybe_unused]] const uint32_t frame,\n> +                              const ControlList &controls)\n> +{\n\nThis is the part where we'll have to explore the split between\nactiveState and the frameContext.\n\nI would envisage storing the 'current active mode' in the Active state,\nbut the paramters which get applied to the ISP in the FrameContext.\n\nThat said, the FrameContext may also need to know the mode at the time\nof that frame context. I am envisioning adding a MetaData control list\nto the FrameContext that could allow algorithms to update metadata\nexplicitly. (As opposed to just duplicating the incoming ControlList).\n\n\n> +       const auto &sharpness = controls.get(controls::Sharpness);\n> +       if (sharpness) {\n> +               context.frameContext.demosaicing.sharpness = std::clamp(int(*sharpness), 0, 10);\n> +               context.frameContext.demosaicing.updateParams = true;\n> +\n> +               LOG(RkISP1Demosaicing, Debug) << \"Set sharpness to \" << *sharpness;\n> +       }\n> +\n> +       const auto &denoise = controls.get(controls::draft::NoiseReductionMode);\n> +       if (denoise) {\n> +               LOG(RkISP1Demosaicing, Debug) << \"Set denoise to \" << *denoise;\n> +\n> +               switch (*denoise) {\n> +               case controls::draft::NoiseReductionModeOff:\n> +                       context.frameContext.demosaicing.denoise = 0;\n> +                       context.frameContext.demosaicing.updateParams = true;\n> +                       break;\n> +               case controls::draft::NoiseReductionModeMinimal:\n> +                       context.frameContext.demosaicing.denoise = 1;\n> +                       context.frameContext.demosaicing.updateParams = true;\n> +                       break;\n> +               case controls::draft::NoiseReductionModeHighQuality:\n> +               case controls::draft::NoiseReductionModeFast:\n> +                       context.frameContext.demosaicing.denoise = 3;\n> +                       context.frameContext.demosaicing.updateParams = true;\n> +                       break;\n> +               default:\n> +                       LOG(RkISP1Demosaicing, Error)\n> +                               << \"Unsupported denoise value \"\n> +                               << *denoise;\n> +               }\n> +       }\n> +}\n> +\n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::prepare\n> + */\n> +void Demosaicing::prepare([[maybe_unused]] IPAContext &context,\n> +                         rkisp1_params_cfg *params)\n> +{\n> +       /* Check if the algorithm configuration has been updated. */\n> +       if (!context.frameContext.demosaicing.updateParams)\n> +               return;\n> +\n> +       context.frameContext.demosaicing.updateParams = false;\n> +\n> +       static constexpr uint16_t filt_fac_sh0[] = {\n> +               0x04, 0x07, 0x0A, 0x0C, 0x10, 0x14, 0x1A, 0x1E, 0x24, 0x2A, 0x30\n> +       };\n> +\n> +       static constexpr uint16_t filt_fac_sh1[] = {\n> +               0x04, 0x08, 0x0C, 0x10, 0x16, 0x1B, 0x20, 0x26, 0x2C, 0x30, 0x3F\n> +       };\n> +\n> +       static constexpr uint16_t filt_fac_mid[] = {\n> +               0x04, 0x06, 0x08, 0x0A, 0x0C, 0x10, 0x13, 0x17, 0x1D, 0x22, 0x28\n> +       };\n> +\n> +       static constexpr uint16_t filt_fac_bl0[] = {\n> +               0x02, 0x02, 0x04, 0x06, 0x08, 0x0A, 0x0C, 0x10, 0x15, 0x1A, 0x24\n> +       };\n> +\n> +       static constexpr uint16_t filt_fac_bl1[] = {\n> +               0x00, 0x00, 0x00, 0x02, 0x04, 0x04, 0x06, 0x08, 0x0D, 0x14, 0x20\n> +       };\n> +\n> +       static constexpr uint16_t filt_thresh_sh0[] = {\n> +               0, 18, 26, 36, 41, 75, 90, 120, 170, 250, 1023\n> +       };\n> +\n> +       static constexpr uint16_t filt_thresh_sh1[] = {\n> +               0, 33, 44, 51, 67, 100, 120, 150, 200, 300, 1023\n> +       };\n> +\n> +       static constexpr uint16_t filt_thresh_bl0[] = {\n> +               0, 8, 13, 23, 26, 50, 60, 80, 140, 180, 1023\n> +       };\n> +\n> +       static constexpr uint16_t filt_thresh_bl1[] = {\n> +               0, 2, 5, 10, 15, 20, 26, 51, 100, 150, 1023\n> +       };\n> +\n> +       static constexpr uint16_t stage1_select[] = {\n> +               6, 6, 4, 4, 3, 3, 2, 2, 2, 2, 2\n> +       };\n> +\n> +       static constexpr uint16_t filt_chr_v_mode[] = {\n> +               1, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3\n> +       };\n> +\n> +       static constexpr uint16_t filt_chr_h_mode[] = {\n> +               0, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3\n> +       };\n> +\n> +       uint8_t denoise = context.frameContext.demosaicing.denoise;\n> +       uint8_t sharpness = context.frameContext.demosaicing.sharpness;\n> +\n\nOk - I now see sharpness is clamped to acceptable ranges, and denoise is\nonly ever set explicitly.\n\n> +       params->others.flt_config.fac_sh0 = filt_fac_sh0[sharpness];\n> +       params->others.flt_config.fac_sh1 = filt_fac_sh1[sharpness];\n> +       params->others.flt_config.fac_mid = filt_fac_mid[sharpness];\n> +       params->others.flt_config.fac_bl0 = filt_fac_bl0[sharpness];\n> +       params->others.flt_config.fac_bl1 = filt_fac_bl1[sharpness];\n> +\n> +       params->others.flt_config.lum_weight = kFiltLumWeightDefault;\n> +       params->others.flt_config.mode = kFiltModeDefault;\n> +       params->others.flt_config.thresh_sh0 = filt_thresh_sh0[denoise];\n> +       params->others.flt_config.thresh_sh1 = filt_thresh_sh1[denoise];\n> +       params->others.flt_config.thresh_bl0 = filt_thresh_bl0[denoise];\n> +       params->others.flt_config.thresh_bl1 = filt_thresh_bl1[denoise];\n> +       params->others.flt_config.grn_stage1 = stage1_select[denoise];\n> +       params->others.flt_config.chr_v_mode = filt_chr_v_mode[denoise];\n> +       params->others.flt_config.chr_h_mode = filt_chr_h_mode[denoise];\n> +\n> +       if (denoise == 9) {\n\nCan this code actually be reached? Is denoise ever set to anything other\nthan 0, 1 or 3\n\n> +               if (sharpness > 3)\n> +                       params->others.flt_config.grn_stage1 = 2;\n> +               else\n> +                       params->others.flt_config.grn_stage1 = 1;\n> +       } else if (denoise == 10) {\n> +               if (sharpness > 5)\n> +                       params->others.flt_config.grn_stage1 = 2;\n> +               else if (sharpness > 3)\n> +                       params->others.flt_config.grn_stage1 = 1;\n> +               else\n> +                       params->others.flt_config.grn_stage1 = 0;\n> +       } else if (denoise > 7) {\n> +               if (sharpness > 7) {\n> +                       params->others.flt_config.fac_bl0 =\n> +                               params->others.flt_config.fac_bl0 / 2;\n> +                       params->others.flt_config.fac_bl1 =\n> +                               params->others.flt_config.fac_bl1 / 4;\n> +               } else if (sharpness > 4) {\n> +                       params->others.flt_config.fac_bl0 =\n> +                               params->others.flt_config.fac_bl0 * 3 / 4;\n> +                       params->others.flt_config.fac_bl1 =\n> +                               params->others.flt_config.fac_bl1 / 2;\n> +               }\n> +       }\n> +\n> +       params->module_en_update |= RKISP1_CIF_ISP_MODULE_FLT;\n> +       params->module_ens |= RKISP1_CIF_ISP_MODULE_FLT;\n> +       params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_FLT;\n> +}\n> +\n> +REGISTER_IPA_ALGORITHM(Demosaicing, \"Demosaicing\")\n> +\n> +} /* namespace ipa::rkisp1::algorithms */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/rkisp1/algorithms/demosaicing.h b/src/ipa/rkisp1/algorithms/demosaicing.h\n> new file mode 100644\n> index 00000000..0d0f778f\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/algorithms/demosaicing.h\n> @@ -0,0 +1,30 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021-2022, Ideas On Board\n> + *\n> + * demosaicing.h - RkISP1 Demosaicing control\n> + */\n> +\n> +#pragma once\n> +\n> +#include <sys/types.h>\n> +\n> +#include \"algorithm.h\"\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::rkisp1::algorithms {\n> +\n> +class Demosaicing : public Algorithm\n> +{\n> +public:\n> +       Demosaicing() = default;\n> +       ~Demosaicing() = default;\n> +\n> +       void queueRequest(IPAContext &context, const uint32_t frame,\n> +                         const ControlList &controls) override;\n> +       void prepare(IPAContext &context, rkisp1_params_cfg *params) override;\n> +};\n> +\n> +} /* namespace ipa::rkisp1::algorithms */\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build\n> index 87007493..7e078b0d 100644\n> --- a/src/ipa/rkisp1/algorithms/meson.build\n> +++ b/src/ipa/rkisp1/algorithms/meson.build\n> @@ -4,6 +4,7 @@ rkisp1_ipa_algorithms = files([\n>      'agc.cpp',\n>      'awb.cpp',\n>      'blc.cpp',\n> +    'demosaicing.cpp',\n>      'dpcc.cpp',\n>      'gsl.cpp',\n>      'lsc.cpp',\n> diff --git a/src/ipa/rkisp1/data/ov5640.yaml b/src/ipa/rkisp1/data/ov5640.yaml\n> index 51228218..4ae0ffc0 100644\n> --- a/src/ipa/rkisp1/data/ov5640.yaml\n> +++ b/src/ipa/rkisp1/data/ov5640.yaml\n> @@ -157,4 +157,5 @@ algorithms:\n>            rnd-offsets:\n>              green: 2\n>              red-blue: 2\n> +  - Demosaicing:\n>  ...\n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index f387cace..241a4d04 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -56,6 +56,12 @@ struct IPAFrameContext {\n>                 double temperatureK;\n>         } awb;\n>  \n> +       struct {\n> +               uint8_t denoise;\n> +               uint8_t sharpness;\n> +               bool updateParams;\n> +       } demosaicing;\n> +\n>         struct {\n>                 uint32_t exposure;\n>                 double gain;\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 34034526..aeec8f50 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -31,6 +31,7 @@\n>  #include \"algorithms/algorithm.h\"\n>  #include \"algorithms/awb.h\"\n>  #include \"algorithms/blc.h\"\n> +#include \"algorithms/demosaicing.h\"\n>  #include \"algorithms/dpcc.h\"\n>  #include \"algorithms/gsl.h\"\n>  #include \"algorithms/lsc.h\"\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 99eecd44..4e000d31 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -968,6 +968,14 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n>                                                    hasSelfPath_ ? &selfPath_ : nullptr);\n>  \n>         ControlInfoMap::Map ctrls;\n> +       ctrls.emplace(std::piecewise_construct,\n> +                     std::forward_as_tuple(&controls::Sharpness),\n> +                     std::forward_as_tuple(0.0f, 10.0f, 1.0f));\n> +\n> +       ctrls.emplace(std::piecewise_construct,\n> +                     std::forward_as_tuple(&controls::draft::NoiseReductionMode),\n> +                     std::forward_as_tuple(controls::draft::NoiseReductionModeValues));\n> +\n>         ctrls.emplace(std::piecewise_construct,\n>                       std::forward_as_tuple(&controls::AeEnable),\n>                       std::forward_as_tuple(false, true));\n> -- \n> 2.34.1\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 5CB02BD1F1\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 21 Jul 2022 13:08:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CA6B46330F;\n\tThu, 21 Jul 2022 15:08:43 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D1796601B8\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 21 Jul 2022 15:08:42 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 57BAF496;\n\tThu, 21 Jul 2022 15:08:42 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658408923;\n\tbh=jLYslORgnikO3OfUP9eReoJ2PnonioHwPxT4IQc3Prk=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=dwkRHyL/Ud7W4sVn3YMs5h1j4BVFIP2mROdPvvEyxEcdFvOx/9MUDJu3RB48x23IR\n\tMK1gMtH3ZPSn+oQcr8wqYgRmIXd9brDop/HHMMAYEYaJCZJ8UzhmwP6ZTQKkPhPq8y\n\t8bp3kxmCTPTlJPtVHT7ThR4KB1/gBz1emtvoIyxEENrLnXTwKT7Gwtn3X+hkkmF3l9\n\t8E5ZznHQBC3P6G3o8UcXDWOjCkIkzc/Eg4K7K/33PTJhu/2Rw0dne90u2IBpsQAYpw\n\tHlqre5bnphow6VUa0w5cTMKRo9ZgJJckjAO6ulQOseIc4bMi2dHUCFDtvx/e5q+Ceb\n\thX8TdmlTw2LEA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1658408922;\n\tbh=jLYslORgnikO3OfUP9eReoJ2PnonioHwPxT4IQc3Prk=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=Td+XTCeTztmaZW4EBqlFFPMY3M1ZvNDNeaztWUUC3QWLilAYeSEH5bVpUqIDWYNL1\n\tlXoHTVe+U1dzhqDSKLoCi8xyGA1ZTdfYzJcpiic0yy/pX/aO2LDp463WNpCStKB0J5\n\tFJx9SWte3InwxOzR1jBULtVW6dqh4ztcfsou123o="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Td+XTCeT\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220720154221.50937-4-fsylvestre@baylibre.com>","References":"<20220720154221.50937-1-fsylvestre@baylibre.com>\n\t<20220720154221.50937-4-fsylvestre@baylibre.com>","To":"Florian Sylvestre <fsylvestre@baylibre.com>,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 21 Jul 2022 14:08:39 +0100","Message-ID":"<165840891929.2228597.14693711504023462292@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2 3/4] ipa: rkisp1: Add support of\n\tDemosaicing control","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Kieran Bingham via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":24045,"web_url":"https://patchwork.libcamera.org/comment/24045/","msgid":"<YtnchGFDkJ70ac1K@pendragon.ideasonboard.com>","date":"2022-07-21T23:08:52","subject":"Re: [libcamera-devel] [PATCH v2 3/4] ipa: rkisp1: Add support of\n\tDemosaicing control","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Florian,\n\nThank you for the patch.\n\nOn Wed, Jul 20, 2022 at 05:42:20PM +0200, Florian Sylvestre via libcamera-devel wrote:\n> During the demosaicing step, rkisp1 ISP is processing denoising and sharpness\n> control.\n> Add demosaicing algorithm with denoise and sharpness values based on user\n> controls.\n> \n> Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>\n> ---\n>  src/ipa/rkisp1/algorithms/demosaicing.cpp | 197 ++++++++++++++++++++++\n>  src/ipa/rkisp1/algorithms/demosaicing.h   |  30 ++++\n>  src/ipa/rkisp1/algorithms/meson.build     |   1 +\n>  src/ipa/rkisp1/data/ov5640.yaml           |   1 +\n>  src/ipa/rkisp1/ipa_context.h              |   6 +\n>  src/ipa/rkisp1/rkisp1.cpp                 |   1 +\n>  src/libcamera/pipeline/rkisp1/rkisp1.cpp  |   8 +\n>  7 files changed, 244 insertions(+)\n>  create mode 100644 src/ipa/rkisp1/algorithms/demosaicing.cpp\n>  create mode 100644 src/ipa/rkisp1/algorithms/demosaicing.h\n> \n> diff --git a/src/ipa/rkisp1/algorithms/demosaicing.cpp b/src/ipa/rkisp1/algorithms/demosaicing.cpp\n> new file mode 100644\n> index 00000000..7d55eaab\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/algorithms/demosaicing.cpp\n> @@ -0,0 +1,197 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021-2022, Ideas On Board\n> + *\n> + * demosaicing.cpp - RkISP1 Demosaicing control\n\nI'm very tempted to rename this algorithm to filter, as it's a\ncombined denoising and sharpening filter. The fact that it's part of the\ndemosaicing block is just a hardware implementation detail. What do you\nthink ?\n\n> + */\n> +\n> +#include \"demosaicing.h\"\n> +\n> +#include <libcamera/base/log.h>\n> +\n> +#include <libcamera/control_ids.h>\n> +\n> +#include \"libcamera/internal/yaml_parser.h\"\n\nYou can drop this line as this algorithm doesn't retrieve any value from\nthe tuning file.\n\n> +\n> +/**\n> + * \\file demosaicing.h\n> + */\n> +\n> +static constexpr uint32_t kFiltLumWeightDefault = 0x00022040;\n> +static constexpr uint32_t kFiltModeDefault = 0x000004f2;\n\nI would move those a few lines down, to the\nlibcamera::ipa::rkisp1::algorithms namespace.\n\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::rkisp1::algorithms {\n> +\n> +/**\n> + * \\class Demosaicing\n> + * \\brief RkISP1 Demosaicing control\n> + *\n> + * The Demosaicing algorithm is responsible for reconstructing a full color\n> + * image from the sensor raw data. During the demosaicing step, The RKISP1\n> + * will additionally process denoise and sharpness controls.\n> + *\n> + * /todo In current version the denoise and sharpness control is based on user\n> + * controls. In a future version it should be controlled automatically by the\n> + * algorithm.\n> + */\n> +\n> +LOG_DEFINE_CATEGORY(RkISP1Demosaicing)\n> +\n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::queueRequest\n> + */\n> +void Demosaicing::queueRequest([[maybe_unused]] IPAContext &context,\n\ncontext is used. Same for prepare().\n\n> +\t\t\t       [[maybe_unused]] const uint32_t frame,\n> +\t\t\t       const ControlList &controls)\n> +{\n\nAs for patch 4/4 (I'm reviewing out of order, sorry)\n\n\tauto &demosaicing = context.frameContext.demosaicing;\n\nwould shorten the lines below.\n\n> +\tconst auto &sharpness = controls.get(controls::Sharpness);\n> +\tif (sharpness) {\n> +\t\tcontext.frameContext.demosaicing.sharpness = std::clamp(int(*sharpness), 0, 10);\n\nUse std::round() (see review comments in 4/4)\n\n> +\t\tcontext.frameContext.demosaicing.updateParams = true;\n> +\n> +\t\tLOG(RkISP1Demosaicing, Debug) << \"Set sharpness to \" << *sharpness;\n> +\t}\n> +\n> +\tconst auto &denoise = controls.get(controls::draft::NoiseReductionMode);\n> +\tif (denoise) {\n> +\t\tLOG(RkISP1Demosaicing, Debug) << \"Set denoise to \" << *denoise;\n> +\n> +\t\tswitch (*denoise) {\n> +\t\tcase controls::draft::NoiseReductionModeOff:\n> +\t\t\tcontext.frameContext.demosaicing.denoise = 0;\n> +\t\t\tcontext.frameContext.demosaicing.updateParams = true;\n> +\t\t\tbreak;\n> +\t\tcase controls::draft::NoiseReductionModeMinimal:\n> +\t\t\tcontext.frameContext.demosaicing.denoise = 1;\n> +\t\t\tcontext.frameContext.demosaicing.updateParams = true;\n> +\t\t\tbreak;\n> +\t\tcase controls::draft::NoiseReductionModeHighQuality:\n> +\t\tcase controls::draft::NoiseReductionModeFast:\n> +\t\t\tcontext.frameContext.demosaicing.denoise = 3;\n> +\t\t\tcontext.frameContext.demosaicing.updateParams = true;\n> +\t\t\tbreak;\n> +\t\tdefault:\n> +\t\t\tLOG(RkISP1Demosaicing, Error)\n> +\t\t\t\t<< \"Unsupported denoise value \"\n> +\t\t\t\t<< *denoise;\n> +\t\t}\n> +\t}\n> +}\n> +\n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::prepare\n> + */\n> +void Demosaicing::prepare([[maybe_unused]] IPAContext &context,\n> +\t\t\t  rkisp1_params_cfg *params)\n> +{\n\n\tauto &demosaicing = context.frameContext.demosaicing;\n\nhere too.\n\n> +\t/* Check if the algorithm configuration has been updated. */\n> +\tif (!context.frameContext.demosaicing.updateParams)\n> +\t\treturn;\n> +\n> +\tcontext.frameContext.demosaicing.updateParams = false;\n> +\n> +\tstatic constexpr uint16_t filt_fac_sh0[] = {\n> +\t\t0x04, 0x07, 0x0A, 0x0C, 0x10, 0x14, 0x1A, 0x1E, 0x24, 0x2A, 0x30\n\nPlease use lower-case for hex constants.\n\n> +\t};\n> +\n> +\tstatic constexpr uint16_t filt_fac_sh1[] = {\n> +\t\t0x04, 0x08, 0x0C, 0x10, 0x16, 0x1B, 0x20, 0x26, 0x2C, 0x30, 0x3F\n> +\t};\n> +\n> +\tstatic constexpr uint16_t filt_fac_mid[] = {\n> +\t\t0x04, 0x06, 0x08, 0x0A, 0x0C, 0x10, 0x13, 0x17, 0x1D, 0x22, 0x28\n> +\t};\n> +\n> +\tstatic constexpr uint16_t filt_fac_bl0[] = {\n> +\t\t0x02, 0x02, 0x04, 0x06, 0x08, 0x0A, 0x0C, 0x10, 0x15, 0x1A, 0x24\n> +\t};\n> +\n> +\tstatic constexpr uint16_t filt_fac_bl1[] = {\n> +\t\t0x00, 0x00, 0x00, 0x02, 0x04, 0x04, 0x06, 0x08, 0x0D, 0x14, 0x20\n> +\t};\n> +\n> +\tstatic constexpr uint16_t filt_thresh_sh0[] = {\n> +\t\t0, 18, 26, 36, 41, 75, 90, 120, 170, 250, 1023\n> +\t};\n> +\n> +\tstatic constexpr uint16_t filt_thresh_sh1[] = {\n> +\t\t0, 33, 44, 51, 67, 100, 120, 150, 200, 300, 1023\n> +\t};\n> +\n> +\tstatic constexpr uint16_t filt_thresh_bl0[] = {\n> +\t\t0, 8, 13, 23, 26, 50, 60, 80, 140, 180, 1023\n> +\t};\n> +\n> +\tstatic constexpr uint16_t filt_thresh_bl1[] = {\n> +\t\t0, 2, 5, 10, 15, 20, 26, 51, 100, 150, 1023\n> +\t};\n> +\n> +\tstatic constexpr uint16_t stage1_select[] = {\n> +\t\t6, 6, 4, 4, 3, 3, 2, 2, 2, 2, 2\n> +\t};\n> +\n> +\tstatic constexpr uint16_t filt_chr_v_mode[] = {\n> +\t\t1, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3\n> +\t};\n> +\n> +\tstatic constexpr uint16_t filt_chr_h_mode[] = {\n> +\t\t0, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3\n> +\t};\n> +\n> +\tuint8_t denoise = context.frameContext.demosaicing.denoise;\n> +\tuint8_t sharpness = context.frameContext.demosaicing.sharpness;\n> +\n\n\tauto &flt_config = params->others.flt_config;\n\n> +\tparams->others.flt_config.fac_sh0 = filt_fac_sh0[sharpness];\n> +\tparams->others.flt_config.fac_sh1 = filt_fac_sh1[sharpness];\n> +\tparams->others.flt_config.fac_mid = filt_fac_mid[sharpness];\n> +\tparams->others.flt_config.fac_bl0 = filt_fac_bl0[sharpness];\n> +\tparams->others.flt_config.fac_bl1 = filt_fac_bl1[sharpness];\n> +\n> +\tparams->others.flt_config.lum_weight = kFiltLumWeightDefault;\n> +\tparams->others.flt_config.mode = kFiltModeDefault;\n> +\tparams->others.flt_config.thresh_sh0 = filt_thresh_sh0[denoise];\n> +\tparams->others.flt_config.thresh_sh1 = filt_thresh_sh1[denoise];\n> +\tparams->others.flt_config.thresh_bl0 = filt_thresh_bl0[denoise];\n> +\tparams->others.flt_config.thresh_bl1 = filt_thresh_bl1[denoise];\n> +\tparams->others.flt_config.grn_stage1 = stage1_select[denoise];\n> +\tparams->others.flt_config.chr_v_mode = filt_chr_v_mode[denoise];\n> +\tparams->others.flt_config.chr_h_mode = filt_chr_h_mode[denoise];\n> +\n\nLet's add a comment here:\n\n\t/*\n\t * Combined high denoising and high sharpening requires some\n\t * adjustements to the configuration of the filters. A first stage\n\t * filter with a lower strength must be selected, and the blur factors\n\t * must be decreased.\n\t */\n\n> +\tif (denoise == 9) {\n> +\t\tif (sharpness > 3)\n> +\t\t\tparams->others.flt_config.grn_stage1 = 2;\n> +\t\telse\n> +\t\t\tparams->others.flt_config.grn_stage1 = 1;\n\nAs the value from the table above for denoise == 9 is 2 already, I would\nwrite this as\n\n\t\tif (sharpness <= 3)\n\t\t\tparams->others.flt_config.grn_stage1 = 1;\n\nto only specify the non-default values in the code, or set the value in\nthe stage1_select table to 0 and write here\n\n\t\tif (sharpness > 3)\n\t\t\tparams->others.flt_config.grn_stage1 = 2;\n\nI think the second option would be best, as the special cases are meant\nto cover high denoising and high sharpening.\n\n> +\t} else if (denoise == 10) {\n> +\t\tif (sharpness > 5)\n> +\t\t\tparams->others.flt_config.grn_stage1 = 2;\n> +\t\telse if (sharpness > 3)\n> +\t\t\tparams->others.flt_config.grn_stage1 = 1;\n> +\t\telse\n> +\t\t\tparams->others.flt_config.grn_stage1 = 0;\n\nSimilarly, set the value in the table to 0, and write\n\n\t\tif (sharpness > 5)\n\t\t\tparams->others.flt_config.grn_stage1 = 2;\n\t\telse if (sharpness > 3)\n\t\t\tparams->others.flt_config.grn_stage1 = 1;\n\n> +\t} else if (denoise > 7) {\n\nNot else, just\n\n\tif (denoise > 7) {\n\nas this has to be done for 8, 9 and 10.\n\n> +\t\tif (sharpness > 7) {\n> +\t\t\tparams->others.flt_config.fac_bl0 =\n> +\t\t\t\tparams->others.flt_config.fac_bl0 / 2;\n> +\t\t\tparams->others.flt_config.fac_bl1 =\n> +\t\t\t\tparams->others.flt_config.fac_bl1 / 4;\n\n\t\t\tparams->others.flt_config.fac_bl0 /= 2;\n\t\t\tparams->others.flt_config.fac_bl1 /= 4;\n\n\n> +\t\t} else if (sharpness > 4) {\n> +\t\t\tparams->others.flt_config.fac_bl0 =\n> +\t\t\t\tparams->others.flt_config.fac_bl0 * 3 / 4;\n\nThis one is annoying :-)\n\n> +\t\t\tparams->others.flt_config.fac_bl1 =\n> +\t\t\t\tparams->others.flt_config.fac_bl1 / 2;\n\n\t\t\tparams->others.flt_config.fac_bl1 /= 2;\n\n> +\t\t}\n> +\t}\n> +\n> +\tparams->module_en_update |= RKISP1_CIF_ISP_MODULE_FLT;\n> +\tparams->module_ens |= RKISP1_CIF_ISP_MODULE_FLT;\n> +\tparams->module_cfg_update |= RKISP1_CIF_ISP_MODULE_FLT;\n> +}\n> +\n> +REGISTER_IPA_ALGORITHM(Demosaicing, \"Demosaicing\")\n> +\n> +} /* namespace ipa::rkisp1::algorithms */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/rkisp1/algorithms/demosaicing.h b/src/ipa/rkisp1/algorithms/demosaicing.h\n> new file mode 100644\n> index 00000000..0d0f778f\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/algorithms/demosaicing.h\n> @@ -0,0 +1,30 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021-2022, Ideas On Board\n> + *\n> + * demosaicing.h - RkISP1 Demosaicing control\n> + */\n> +\n> +#pragma once\n> +\n> +#include <sys/types.h>\n> +\n> +#include \"algorithm.h\"\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::rkisp1::algorithms {\n> +\n> +class Demosaicing : public Algorithm\n> +{\n> +public:\n> +\tDemosaicing() = default;\n> +\t~Demosaicing() = default;\n> +\n> +\tvoid queueRequest(IPAContext &context, const uint32_t frame,\n> +\t\t\t  const ControlList &controls) override;\n> +\tvoid prepare(IPAContext &context, rkisp1_params_cfg *params) override;\n> +};\n> +\n> +} /* namespace ipa::rkisp1::algorithms */\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build\n> index 87007493..7e078b0d 100644\n> --- a/src/ipa/rkisp1/algorithms/meson.build\n> +++ b/src/ipa/rkisp1/algorithms/meson.build\n> @@ -4,6 +4,7 @@ rkisp1_ipa_algorithms = files([\n>      'agc.cpp',\n>      'awb.cpp',\n>      'blc.cpp',\n> +    'demosaicing.cpp',\n>      'dpcc.cpp',\n>      'gsl.cpp',\n>      'lsc.cpp',\n> diff --git a/src/ipa/rkisp1/data/ov5640.yaml b/src/ipa/rkisp1/data/ov5640.yaml\n> index 51228218..4ae0ffc0 100644\n> --- a/src/ipa/rkisp1/data/ov5640.yaml\n> +++ b/src/ipa/rkisp1/data/ov5640.yaml\n> @@ -157,4 +157,5 @@ algorithms:\n>            rnd-offsets:\n>              green: 2\n>              red-blue: 2\n> +  - Demosaicing:\n>  ...\n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index f387cace..241a4d04 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -56,6 +56,12 @@ struct IPAFrameContext {\n>  \t\tdouble temperatureK;\n>  \t} awb;\n>  \n> +\tstruct {\n> +\t\tuint8_t denoise;\n> +\t\tuint8_t sharpness;\n> +\t\tbool updateParams;\n> +\t} demosaicing;\n> +\n>  \tstruct {\n>  \t\tuint32_t exposure;\n>  \t\tdouble gain;\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 34034526..aeec8f50 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -31,6 +31,7 @@\n>  #include \"algorithms/algorithm.h\"\n>  #include \"algorithms/awb.h\"\n>  #include \"algorithms/blc.h\"\n> +#include \"algorithms/demosaicing.h\"\n>  #include \"algorithms/dpcc.h\"\n>  #include \"algorithms/gsl.h\"\n>  #include \"algorithms/lsc.h\"\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 99eecd44..4e000d31 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -968,6 +968,14 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n>  \t\t\t\t\t\t   hasSelfPath_ ? &selfPath_ : nullptr);\n>  \n>  \tControlInfoMap::Map ctrls;\n> +\tctrls.emplace(std::piecewise_construct,\n> +\t\t      std::forward_as_tuple(&controls::Sharpness),\n> +\t\t      std::forward_as_tuple(0.0f, 10.0f, 1.0f));\n> +\n> +\tctrls.emplace(std::piecewise_construct,\n> +\t\t      std::forward_as_tuple(&controls::draft::NoiseReductionMode),\n> +\t\t      std::forward_as_tuple(controls::draft::NoiseReductionModeValues));\n> +\n>  \tctrls.emplace(std::piecewise_construct,\n>  \t\t      std::forward_as_tuple(&controls::AeEnable),\n>  \t\t      std::forward_as_tuple(false, true));","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 F2E1ABD1F1\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 21 Jul 2022 23:08:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8AE9563311;\n\tFri, 22 Jul 2022 01:08:57 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 43CEF60487\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 22 Jul 2022 01:08:55 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A86856D5;\n\tFri, 22 Jul 2022 01:08:54 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658444937;\n\tbh=Ad6+IN4LGFrJeIQeNAv5TA29EjKKFXT4R+3uGLYSAaw=;\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=fDYX7HRiCOOXbYmcJ16ZqT1wzumb/M8Gu+06SGX68J8NEOemhIY8Bprq7dnJj/Cqz\n\tr2nwoT4XIFyGAyeLVXYBRxUxuZqC+ruIjijocXXi1Ki03pihfmpS03u1N+XfjeaS/V\n\teAcIPWXgT+VEEtlO6Y7Nr2cgWUcvfg3IS/KOPRX/qxu8D2NiQdMFBgYwVPKBmQUR2F\n\toOUXNnA51NqNEv3fdeG1LqYT5k2khlbEUKeZXECvFH/Y+hBc3cKpvvBWWcoSJldBQ1\n\t+YCJrP3guHuMMT3N5a4zFgJ9YhbQqAEt5s5Nd7iWwDBbLgKXnI/ezDETJHFxDBbSEW\n\txQNlcy4ZDxUdQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1658444934;\n\tbh=Ad6+IN4LGFrJeIQeNAv5TA29EjKKFXT4R+3uGLYSAaw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=rKumgi6/mF9xXASRlFEWOnNU/C+FCiUtfs/z2X87y8cLTgTzRIQwJXWHY+Ku+qzgD\n\tmQQ4H3uGzYPuNkMo4frm8BkUXPQ+vM0Wff1inm8o2RTPVXH0Hfw/EkpY0n4uj7jOXQ\n\totWqYeSwgKwxKCfoR/cBeMjfRMJg206Qs8aJhfIY="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"rKumgi6/\"; dkim-atps=neutral","Date":"Fri, 22 Jul 2022 02:08:52 +0300","To":"Florian Sylvestre <fsylvestre@baylibre.com>","Message-ID":"<YtnchGFDkJ70ac1K@pendragon.ideasonboard.com>","References":"<20220720154221.50937-1-fsylvestre@baylibre.com>\n\t<20220720154221.50937-4-fsylvestre@baylibre.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220720154221.50937-4-fsylvestre@baylibre.com>","Subject":"Re: [libcamera-devel] [PATCH v2 3/4] ipa: rkisp1: Add support of\n\tDemosaicing control","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Laurent Pinchart via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]