[{"id":23937,"web_url":"https://patchwork.libcamera.org/comment/23937/","msgid":"<165789637197.1177852.13156141960167722933@Monstersaurus>","date":"2022-07-15T14:46:11","subject":"Re: [libcamera-devel] [PATCH 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":"Quoting Florian Sylvestre via libcamera-devel (2022-07-04 16:23:17)\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 | 208 ++++++++++++++++++++++\n>  src/ipa/rkisp1/algorithms/demosaicing.h   |  37 ++++\n>  src/ipa/rkisp1/algorithms/meson.build     |   1 +\n>  src/ipa/rkisp1/data/ov5640.yaml           |   1 +\n>  src/ipa/rkisp1/rkisp1.cpp                 |   1 +\n>  src/libcamera/pipeline/rkisp1/rkisp1.cpp  |   8 +\n>  6 files changed, 256 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..2597735e\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/algorithms/demosaicing.cpp\n> @@ -0,0 +1,208 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021-2022, Ideas On Board\n> + *\n> + * lsc.cpp - RkISP1 Defect Pixel Cluster Correction control\n\n* demosaicing.cpp - ... here perhaps.\n\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> +\n> +/**\n> + * \\file demosaicing.h\n> + */\n> +\n> +#define ISP_FILT_LUM_WEIGHT_DEFAULT 0x00022040\n> +#define ISP_FILT_MODE_DEFAULT 0x000004f2\n> +\n> +/* Default values : should be updated with automatic control implementation. */\n> +#define SHARPNESS_DEFAULT 2\n> +#define DENOISE_DEFAULT 2\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 to reconstruct a full color image\n> + * from the sensor raw data. During the demosaicing step, rkisp1 ISP is\n> + * processing denoising and sharpness control.\n\n\nOnly some subtle suggestion for reword - but it's up to you. It might\nnot be accurate:\n\n* The Demosaicing algorithm is responsible for reconstructing a full\n* color image from the sensor raw data. During the demosaicing step, The\n* RKISP1 will additionally process denoise and sharpness controls.\n\n\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> +Demosaicing::Demosaicing()\n> +       : userSharpness_(SHARPNESS_DEFAULT), userDenoise_(DENOISE_DEFAULT),\n> +         updateUserControls_(true)\n> +{\n> +}\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> +       for (auto const &ctrl : controls) {\n> +               if (ctrl.first == controls::SHARPNESS) {\n\nI wonder if \n\t\tswitch (ctrl.first) {\n\n\t\tcase controls::SHARPNESS:\n\t\t\t...\n\t\t\tbreak;\n\nwould be cleaner than split if/elses.\n\n> +                       userSharpness_ = ctrl.second.get<float>();\n\nIn preparation for supporting 'per frame controls' (once you've added\npassing the FrameContext reference into queueRequest), I think you could\nalready put userSharpness_ into IPAFrameContext.demosaic.sharpness\n\n(Add a demosaic structure in [0])\n[0] https://git.libcamera.org/libcamera/libcamera.git/tree/src/ipa/rkisp1/ipa_context.h#n43\n\nSame for denoise control too of course.\n\n> +                       updateUserControls_ = true;\n> +\n> +                       LOG(RkISP1Demosaicing, Debug)\n> +                               << \"Set sharpness to: \"\n> +                               << int(userSharpness_);\n> +               } else if (ctrl.first == controls::NOISE_REDUCTION_MODE) {\n> +                       int32_t denoiseControl = ctrl.second.get<int32_t>();\n> +\n> +                       switch (denoiseControl) {\n> +                       case controls::draft::NoiseReductionModeOff:\n> +                               userDenoise_ = 0;\n\nAre these 'values' or indexes into the arrays below? Could they be\nenums?\n\n> +                               break;\n> +                       case controls::draft::NoiseReductionModeMinimal:\n> +                               userDenoise_ = 1;\n> +                               break;\n> +                       case controls::draft::NoiseReductionModeHighQuality:\n> +                       case controls::draft::NoiseReductionModeFast:\n> +                               userDenoise_ = 3;\n> +                               break;\n> +                       default:\n> +                               LOG(RkISP1Demosaicing, Error)\n> +                                       << \"Unsupported denoise value: \"\n> +                                       << int(denoiseControl);\n> +                               continue;\n> +                       }\n> +\n> +                       updateUserControls_ = true;\n> +\n> +                       LOG(RkISP1Demosaicing, Debug)\n> +                               << \"Set denoise to: \"\n> +                               << int(userDenoise_);\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 (!updateUserControls_)\n> +               return;\n> +\n> +       updateUserControls_ = 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\nAs far as I can tell, these sets are only used by either the sharpness\nor denoise.\n\nI'd be tempted to pull each one to it's own structure - and define the\nfull state of each level as a contained structure.\n\nBut I can see some merit in keeping the levels for each parameter in an\narray like this so the levels can be seen too across each setting ...\nI'm torn :(\n\n> +\n> +       params->others.flt_config.fac_sh0 = filt_fac_sh0[userSharpness_];\n> +       params->others.flt_config.fac_sh1 = filt_fac_sh1[userSharpness_];\n> +       params->others.flt_config.fac_mid = filt_fac_mid[userSharpness_];\n> +       params->others.flt_config.fac_bl0 = filt_fac_bl0[userSharpness_];\n> +       params->others.flt_config.fac_bl1 = filt_fac_bl1[userSharpness_];\n\nI don't believe this is safe currently.\n\nI don't think we have any specific clamping of the control values to\nensure they are within range - so accepting a user input value as an\nindex to an array could access memory out of bounds.\n\nThis likely needs to be clamped - (or we need to make sure that controls\nare enforced to be clamped within their ranges too).\n\n\n> +\n> +       params->others.flt_config.lum_weight = ISP_FILT_LUM_WEIGHT_DEFAULT;\n> +       params->others.flt_config.mode = ISP_FILT_MODE_DEFAULT;\n> +       params->others.flt_config.thresh_sh0 = filt_thresh_sh0[userDenoise_];\n> +       params->others.flt_config.thresh_sh1 = filt_thresh_sh1[userDenoise_];\n> +       params->others.flt_config.thresh_bl0 = filt_thresh_bl0[userDenoise_];\n> +       params->others.flt_config.thresh_bl1 = filt_thresh_bl1[userDenoise_];\n> +       params->others.flt_config.grn_stage1 = stage1_select[userDenoise_];\n> +       params->others.flt_config.chr_v_mode = filt_chr_v_mode[userDenoise_];\n> +       params->others.flt_config.chr_h_mode = filt_chr_h_mode[userDenoise_];\n> +\n> +       if (userDenoise_ == 9) {\n> +               if (userSharpness_ > 3)\n> +                       params->others.flt_config.grn_stage1 = 2;\n> +               else\n> +                       params->others.flt_config.grn_stage1 = 1;\n> +       } else if (userDenoise_ == 10) {\n> +               if (userSharpness_ > 5)\n> +                       params->others.flt_config.grn_stage1 = 2;\n> +               else if (userSharpness_ > 3)\n> +                       params->others.flt_config.grn_stage1 = 1;\n> +               else\n> +                       params->others.flt_config.grn_stage1 = 0;\n> +       } else if (userDenoise_ > 7) {\n> +               if (userSharpness_ > 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 (userSharpness_ > 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\nI only ever see userDenoise_ set to either 0, 1, or 3 ... Did I miss\nsomething going on elsewhere? Can it be set to 10?\n\nIs this logic taken from some sample code somewhere? Or a reference\nmanual perhaps?\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..6d05b0e4\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/algorithms/demosaicing.h\n> @@ -0,0 +1,37 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2021-2022, Ideas On Board\n> + *\n> + * dpcc.h - RkISP1 Demosaicing control\n\nMaybe we shouldn't be specifying the filename in the comments ...\n\n\n> + */\n> +\n> +#pragma once\n> +\n> +#include <sys/types.h>\n> +#include <linux/rkisp1-config.h>\n> +\n> +#include \"algorithm.h\"\n> +\n> +namespace libcamera {\n> +\n> +struct IPACameraSensorInfo;\n> +\n> +namespace ipa::rkisp1::algorithms {\n> +\n> +class Demosaicing : public Algorithm\n> +{\n> +public:\n> +       Demosaicing();\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> +private:\n> +       uint32_t userSharpness_;\n> +       uint32_t userDenoise_;\n> +       bool updateUserControls_;\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/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index 9b0d675c..85a75070 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 72689c88..3d0075ee 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -953,6 +953,14 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n>                 std::make_unique<RkISP1CameraData>(this, &mainPath_, &selfPath_);\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\nI think these should be added from the IPA - but perhaps that's\nalso missing some plumbing?\n\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 1FC1EBD1F1\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 15 Jul 2022 14:46:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7A9FC63312;\n\tFri, 15 Jul 2022 16:46:15 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 55B086330A\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 15 Jul 2022 16:46:14 +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 E4B85993;\n\tFri, 15 Jul 2022 16:46:13 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1657896375;\n\tbh=ZyfyQiz8xvrJG6DDfOOd1BrTKSyb2Gg83a4NqteT9dg=;\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=yWmOLQ2jmmGbuSHRElEo/9+yPLgWc42n5symABEqcXEkArMFjgZtObhz5gNrbSO4/\n\tCIZCC35rfoGtrXZ///tymO8Fduuyt3w/zhmZ+D1cQZIA7n9Remjt3Ql/WKJvIU03oE\n\tsfwZ1YDGh7PTnSbNLqvyADmoJXCaU2gQ41t8fjocd3dygHG1DkT1pKGkDUtZ7Cbxyx\n\trGJnHvlo5RPhdTpXedvU5pgkx6kS4aojAuUBgBdXqAJ+DbOZsyXlXzgCo2aRnqSCYd\n\tZCzRgaZUow65Dt5+IuLMFgggMBTnEYpxGqAMW/tR3S6I0TT/UNWTPrI+AgeXedv37A\n\tjG+6Sw4quEp2g==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1657896374;\n\tbh=ZyfyQiz8xvrJG6DDfOOd1BrTKSyb2Gg83a4NqteT9dg=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=ohhr4WWmY8bSfDJwA/xzyiiE99R6qR06C2yTzqIhxNHUpSYWD/wrxE6MusuT2Nkhf\n\tl1KDJFukxqHd3scWwCAWDS3uhlgVcUbamNtbykdBDb6QbnrbsGSRAn5OdtIcP5X9R4\n\tREJ9O0l+1AYWxqlGBH4cpZADAQKTnHp/rycKVjgg="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ohhr4WWm\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220704152318.221213-4-fsylvestre@baylibre.com>","References":"<20220704152318.221213-1-fsylvestre@baylibre.com>\n\t<20220704152318.221213-4-fsylvestre@baylibre.com>","To":"Florian Sylvestre <fsylvestre@baylibre.com>,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 15 Jul 2022 15:46:11 +0100","Message-ID":"<165789637197.1177852.13156141960167722933@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH 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":24046,"web_url":"https://patchwork.libcamera.org/comment/24046/","msgid":"<YtneTPHoKWEeLGpA@pendragon.ideasonboard.com>","date":"2022-07-21T23:16:28","subject":"Re: [libcamera-devel] [PATCH 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 Kieran,\n\nOn Fri, Jul 15, 2022 at 03:46:11PM +0100, Kieran Bingham via libcamera-devel wrote:\n> Quoting Florian Sylvestre via libcamera-devel (2022-07-04 16:23:17)\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 | 208 ++++++++++++++++++++++\n> >  src/ipa/rkisp1/algorithms/demosaicing.h   |  37 ++++\n> >  src/ipa/rkisp1/algorithms/meson.build     |   1 +\n> >  src/ipa/rkisp1/data/ov5640.yaml           |   1 +\n> >  src/ipa/rkisp1/rkisp1.cpp                 |   1 +\n> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp  |   8 +\n> >  6 files changed, 256 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..2597735e\n> > --- /dev/null\n> > +++ b/src/ipa/rkisp1/algorithms/demosaicing.cpp\n> > @@ -0,0 +1,208 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021-2022, Ideas On Board\n> > + *\n> > + * lsc.cpp - RkISP1 Defect Pixel Cluster Correction control\n> \n> * demosaicing.cpp - ... here perhaps.\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> > +\n> > +/**\n> > + * \\file demosaicing.h\n> > + */\n> > +\n> > +#define ISP_FILT_LUM_WEIGHT_DEFAULT 0x00022040\n> > +#define ISP_FILT_MODE_DEFAULT 0x000004f2\n> > +\n> > +/* Default values : should be updated with automatic control implementation. */\n> > +#define SHARPNESS_DEFAULT 2\n> > +#define DENOISE_DEFAULT 2\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 to reconstruct a full color image\n> > + * from the sensor raw data. During the demosaicing step, rkisp1 ISP is\n> > + * processing denoising and sharpness control.\n> \n> Only some subtle suggestion for reword - but it's up to you. It might\n> not be accurate:\n> \n> * The Demosaicing algorithm is responsible for reconstructing a full\n> * color image from the sensor raw data. During the demosaicing step, The\n> * RKISP1 will additionally process denoise and sharpness controls.\n> \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> > +Demosaicing::Demosaicing()\n> > +       : userSharpness_(SHARPNESS_DEFAULT), userDenoise_(DENOISE_DEFAULT),\n> > +         updateUserControls_(true)\n> > +{\n> > +}\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> > +       for (auto const &ctrl : controls) {\n> > +               if (ctrl.first == controls::SHARPNESS) {\n> \n> I wonder if \n> \t\tswitch (ctrl.first) {\n> \n> \t\tcase controls::SHARPNESS:\n> \t\t\t...\n> \t\t\tbreak;\n> \n> would be cleaner than split if/elses.\n> \n> > +                       userSharpness_ = ctrl.second.get<float>();\n> \n> In preparation for supporting 'per frame controls' (once you've added\n> passing the FrameContext reference into queueRequest), I think you could\n> already put userSharpness_ into IPAFrameContext.demosaic.sharpness\n> \n> (Add a demosaic structure in [0])\n> [0] https://git.libcamera.org/libcamera/libcamera.git/tree/src/ipa/rkisp1/ipa_context.h#n43\n> \n> Same for denoise control too of course.\n> \n> > +                       updateUserControls_ = true;\n> > +\n> > +                       LOG(RkISP1Demosaicing, Debug)\n> > +                               << \"Set sharpness to: \"\n> > +                               << int(userSharpness_);\n> > +               } else if (ctrl.first == controls::NOISE_REDUCTION_MODE) {\n> > +                       int32_t denoiseControl = ctrl.second.get<int32_t>();\n> > +\n> > +                       switch (denoiseControl) {\n> > +                       case controls::draft::NoiseReductionModeOff:\n> > +                               userDenoise_ = 0;\n> \n> Are these 'values' or indexes into the arrays below? Could they be\n> enums?\n\nThose are denoising levels, and they index the arrays below indeed.\nThey're not enumerated values though, they're really levels.\n\n> > +                               break;\n> > +                       case controls::draft::NoiseReductionModeMinimal:\n> > +                               userDenoise_ = 1;\n> > +                               break;\n> > +                       case controls::draft::NoiseReductionModeHighQuality:\n> > +                       case controls::draft::NoiseReductionModeFast:\n> > +                               userDenoise_ = 3;\n> > +                               break;\n> > +                       default:\n> > +                               LOG(RkISP1Demosaicing, Error)\n> > +                                       << \"Unsupported denoise value: \"\n> > +                                       << int(denoiseControl);\n> > +                               continue;\n> > +                       }\n> > +\n> > +                       updateUserControls_ = true;\n> > +\n> > +                       LOG(RkISP1Demosaicing, Debug)\n> > +                               << \"Set denoise to: \"\n> > +                               << int(userDenoise_);\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 (!updateUserControls_)\n> > +               return;\n> > +\n> > +       updateUserControls_ = 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> As far as I can tell, these sets are only used by either the sharpness\n> or denoise.\n> \n> I'd be tempted to pull each one to it's own structure - and define the\n> full state of each level as a contained structure.\n> \n> But I can see some merit in keeping the levels for each parameter in an\n> array like this so the levels can be seen too across each setting ...\n> I'm torn :(\n> \n> > +\n> > +       params->others.flt_config.fac_sh0 = filt_fac_sh0[userSharpness_];\n> > +       params->others.flt_config.fac_sh1 = filt_fac_sh1[userSharpness_];\n> > +       params->others.flt_config.fac_mid = filt_fac_mid[userSharpness_];\n> > +       params->others.flt_config.fac_bl0 = filt_fac_bl0[userSharpness_];\n> > +       params->others.flt_config.fac_bl1 = filt_fac_bl1[userSharpness_];\n> \n> I don't believe this is safe currently.\n> \n> I don't think we have any specific clamping of the control values to\n> ensure they are within range - so accepting a user input value as an\n> index to an array could access memory out of bounds.\n> \n> This likely needs to be clamped - (or we need to make sure that controls\n> are enforced to be clamped within their ranges too).\n> \n> > +\n> > +       params->others.flt_config.lum_weight = ISP_FILT_LUM_WEIGHT_DEFAULT;\n> > +       params->others.flt_config.mode = ISP_FILT_MODE_DEFAULT;\n> > +       params->others.flt_config.thresh_sh0 = filt_thresh_sh0[userDenoise_];\n> > +       params->others.flt_config.thresh_sh1 = filt_thresh_sh1[userDenoise_];\n> > +       params->others.flt_config.thresh_bl0 = filt_thresh_bl0[userDenoise_];\n> > +       params->others.flt_config.thresh_bl1 = filt_thresh_bl1[userDenoise_];\n> > +       params->others.flt_config.grn_stage1 = stage1_select[userDenoise_];\n> > +       params->others.flt_config.chr_v_mode = filt_chr_v_mode[userDenoise_];\n> > +       params->others.flt_config.chr_h_mode = filt_chr_h_mode[userDenoise_];\n> > +\n> > +       if (userDenoise_ == 9) {\n> > +               if (userSharpness_ > 3)\n> > +                       params->others.flt_config.grn_stage1 = 2;\n> > +               else\n> > +                       params->others.flt_config.grn_stage1 = 1;\n> > +       } else if (userDenoise_ == 10) {\n> > +               if (userSharpness_ > 5)\n> > +                       params->others.flt_config.grn_stage1 = 2;\n> > +               else if (userSharpness_ > 3)\n> > +                       params->others.flt_config.grn_stage1 = 1;\n> > +               else\n> > +                       params->others.flt_config.grn_stage1 = 0;\n> > +       } else if (userDenoise_ > 7) {\n> > +               if (userSharpness_ > 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 (userSharpness_ > 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> I only ever see userDenoise_ set to either 0, 1, or 3 ... Did I miss\n> something going on elsewhere? Can it be set to 10?\n\nThe denoising level is currently hardcoded, the next step will be to set\nit dynamically based on sensor noise characterization (depending on the\nexposure time and analog gain). It could thus reach higher values.\n\n> Is this logic taken from some sample code somewhere? Or a reference\n> manual perhaps?\n\nSomewhere not public I'm afraid.\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..6d05b0e4\n> > --- /dev/null\n> > +++ b/src/ipa/rkisp1/algorithms/demosaicing.h\n> > @@ -0,0 +1,37 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2021-2022, Ideas On Board\n> > + *\n> > + * dpcc.h - RkISP1 Demosaicing control\n> \n> Maybe we shouldn't be specifying the filename in the comments ...\n\nI've been thinking about this too :-)\n\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include <sys/types.h>\n> > +#include <linux/rkisp1-config.h>\n> > +\n> > +#include \"algorithm.h\"\n> > +\n> > +namespace libcamera {\n> > +\n> > +struct IPACameraSensorInfo;\n> > +\n> > +namespace ipa::rkisp1::algorithms {\n> > +\n> > +class Demosaicing : public Algorithm\n> > +{\n> > +public:\n> > +       Demosaicing();\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> > +private:\n> > +       uint32_t userSharpness_;\n> > +       uint32_t userDenoise_;\n> > +       bool updateUserControls_;\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/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > index 9b0d675c..85a75070 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 72689c88..3d0075ee 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -953,6 +953,14 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)\n> >                 std::make_unique<RkISP1CameraData>(this, &mainPath_, &selfPath_);\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> \n> I think these should be added from the IPA - but perhaps that's\n> also missing some plumbing?\n\nI'll work on that.\n\n> >         ctrls.emplace(std::piecewise_construct,\n> >                       std::forward_as_tuple(&controls::AeEnable),\n> >                       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 A61BDBE173\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 21 Jul 2022 23:16:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 17AD863311;\n\tFri, 22 Jul 2022 01:16:33 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 651DD60487\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 22 Jul 2022 01:16:31 +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 CB3DA6D5;\n\tFri, 22 Jul 2022 01:16:30 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1658445393;\n\tbh=uq5pXowtfNPwdcDqMU3pha9oUmd97FckZsMD/1esfG0=;\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=cGZW7Af7vt0AEuQ4M/gJ24yb6M7AEBhjRv7AGrd6VViO2To7TYN6eIdtSqBwb7Ack\n\tSR7plSnoDpRXlilTxHVROch+fmj6KZvV7z4fQgRv5msbWEfwnlVB5hPGEclDkyZMPn\n\t125Ce6DbiCUuir+Kp4XoqBSBNTaA01rEbtXTo0b5Of+HVc94bs51DVR6lh5oci8PHo\n\tP3CQ7cITH/6acbvcr5h9j1MBCKj5zXjsudcZmq2TRm8RJ1KwRCJx9lW5nkRoISXQ0c\n\tx7SCxejN3BMC1dhp07F4aPOG/SIwwgNpWikXjOj1z/UmGgAp3bJUlLT9VgyzCOFosc\n\tvnKiALirZDyPg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1658445391;\n\tbh=uq5pXowtfNPwdcDqMU3pha9oUmd97FckZsMD/1esfG0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nyR8HQBq7gWsBsG8nAQprjW/DKVy/l5QFPrX+hXdA4aNs6ncMULbKydR++i89qgCK\n\tI487g/xGBd4HAnIdJ8ZfPnz9OXHAADRqSJFwqlZqIPoSC49YwK5o2a86JDybVzMTM6\n\t4aqAbDULeA8Bj4IZIDaTwKIBgaCMhxZDENqeyXt8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"nyR8HQBq\"; dkim-atps=neutral","Date":"Fri, 22 Jul 2022 02:16:28 +0300","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Message-ID":"<YtneTPHoKWEeLGpA@pendragon.ideasonboard.com>","References":"<20220704152318.221213-1-fsylvestre@baylibre.com>\n\t<20220704152318.221213-4-fsylvestre@baylibre.com>\n\t<165789637197.1177852.13156141960167722933@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<165789637197.1177852.13156141960167722933@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH 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>"}}]