[{"id":5311,"web_url":"https://patchwork.libcamera.org/comment/5311/","msgid":"<20200622023322.GF25355@pendragon.ideasonboard.com>","date":"2020-06-22T02:33:22","subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: raspberrypi: add\n\tsharpness strength control to Raspberry Pi IPAs","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nThank you for the patch.\n\nOn Fri, Jun 19, 2020 at 10:27:24AM +0100, David Plowman wrote:\n> The sharpness control is, loosely speaking, a gain applied to\n> the amount of sharpening added to an image. We also report the\n> sharpness setting used back to the caller in metadata.\n> \n> Signed-off-by: David Plowman \n> ---\n> .../raspberrypi/controller/rpi/sharpen.cpp | 23 +++++++++++++++----\n> .../raspberrypi/controller/rpi/sharpen.hpp | 6 +++--\n> .../controller/sharpen_algorithm.hpp | 21 +++++++++++++++++\n> .../raspberrypi/controller/sharpen_status.h | 2 ++\n> 4 files changed, 46 insertions(+), 6 deletions(-)\n> create mode 100644 src/ipa/raspberrypi/controller/sharpen_algorithm.hpp\n> \n> diff --git a/src/ipa/raspberrypi/controller/rpi/sharpen.cpp b/src/ipa/raspberrypi/controller/rpi/sharpen.cpp\n> index 1f07bb6..45fe3fe 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/sharpen.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/sharpen.cpp\n> @@ -17,7 +17,7 @@ using namespace RPi;\n> #define NAME \"rpi.sharpen\"\n> \n> Sharpen::Sharpen(Controller *controller)\n> -\t: Algorithm(controller)\n> +\t: SharpenAlgorithm(controller), user_strength_(1.0)\n> {\n> }\n> \n> @@ -40,15 +40,30 @@ void Sharpen::Read(boost::property_tree::ptree const ¶ms)\n> \tlimit_ = params.get(\"limit\", 1.0);\n> }\n> \n> +void Sharpen::SetStrength(double strength)\n> +{\n> +\t// Note that this method is how an application sets the overall\n> +\t// sharpening \"strength\". We call this the \"user strength\" field\n> +\t// as there already is a strength_ field - being an internal gain\n> +\t// parameter that gets passed to the ISP control code.\n> +\tuser_strength_ = strength;\n> +}\n> +\n> void Sharpen::Prepare(Metadata *image_metadata)\n> {\n> \tdouble mode_factor = mode_factor_;\n> +\t// The user_strength affects the algorithm's internal gain directly, but\n> +\t// we adjust the limit and threshold more cautiously, hence the sqrt.\n\nI didn't understand this initially, but reading your camera tuning guide\nhelped. I think s/we adjust/we want to adjust/ would be a bit clearer.\nMaybe \"hence the sqrt\" could also be reworded to clarify that it's an\narbitrary heuristic. Or, if it isn't, then my understanding is incorrect\nand some more information would be useful :-)\n\n> +\tdouble user_strength = user_strength_;\n\nDo you need this local variable, can't you use user_strength_ directly\nin the code below ? Or are there multi-threading race conditions ? If\nso, doesn't the same race condition affect mode_factor and user_strength\nas you read them both here without any lock ?\n\n> +\tdouble user_strength_sqrt = sqrt(user_strength);\n> \tstruct SharpenStatus status;\n> \t// Binned modes seem to need the sharpening toned down with this\n> \t// pipeline.\n\nOut of curiosity, why is that ?\n\n> -\tstatus.threshold = threshold_ * mode_factor;\n> -\tstatus.strength = strength_ / mode_factor;\n> -\tstatus.limit = limit_ / mode_factor;\n> +\tstatus.threshold = threshold_ * mode_factor / user_strength_sqrt;\n> +\tstatus.strength = (strength_ / mode_factor) * user_strength;\n> +\tstatus.limit = (limit_ / mode_factor) * user_strength_sqrt;\n\nThe parentheses are not needed in the above two lines.\n\n> +\t// Finally, report any application-supplied parameters that were used.\n> +\tstatus.user_strength = user_strength;\n> \timage_metadata->Set(\"sharpen.status\", status);\n> }\n> \n> diff --git a/src/ipa/raspberrypi/controller/rpi/sharpen.hpp b/src/ipa/raspberrypi/controller/rpi/sharpen.hpp\n> index 3b0d680..5b419b4 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/sharpen.hpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/sharpen.hpp\n> @@ -6,20 +6,21 @@\n> */\n> #pragma once\n> \n> -#include \"../algorithm.hpp\"\n> +#include \"../sharpen_algorithm.hpp\"\n> #include \"../sharpen_status.h\"\n> \n> // This is our implementation of the \"sharpen algorithm\".\n> \n> namespace RPi {\n> \n> -class Sharpen : public Algorithm\n> +class Sharpen : public SharpenAlgorithm\n> {\n> public:\n> \tSharpen(Controller *controller);\n> \tchar const *Name() const override;\n> \tvoid SwitchMode(CameraMode const &camera_mode) override;\n> \tvoid Read(boost::property_tree::ptree const ¶ms) override;\n> +\tvoid SetStrength(double strength) override;\n> \tvoid Prepare(Metadata *image_metadata) override;\n> \n> private:\n> @@ -27,6 +28,7 @@ private:\n> \tdouble strength_;\n> \tdouble limit_;\n> \tstd::atomic mode_factor_;\n> +\tstd::atomic user_strength_;\n> };\n> \n> } // namespace RPi\n> diff --git a/src/ipa/raspberrypi/controller/sharpen_algorithm.hpp b/src/ipa/raspberrypi/controller/sharpen_algorithm.hpp\n> new file mode 100644\n> index 0000000..3b27a74\n> --- /dev/null\n> +++ b/src/ipa/raspberrypi/controller/sharpen_algorithm.hpp\n> @@ -0,0 +1,21 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2020, Raspberry Pi (Trading) Limited\n> + *\n> + * sharpen_algorithm.hpp - sharpness control algorithm interface\n> + */\n> +#pragma once\n> +\n> +#include \"algorithm.hpp\"\n> +\n> +namespace RPi {\n> +\n> +class SharpenAlgorithm : public Algorithm\n> +{\n> +public:\n> +\tSharpenAlgorithm(Controller *controller) : Algorithm(controller) {}\n> +\t// A sharpness control algorithm must provide the following:\n> +\tvirtual void SetStrength(double strength) = 0;\n> +};\n> +\n> +} // namespace RPi\n> diff --git a/src/ipa/raspberrypi/controller/sharpen_status.h b/src/ipa/raspberrypi/controller/sharpen_status.h\n> index 6de80f4..7501b19 100644\n> --- a/src/ipa/raspberrypi/controller/sharpen_status.h\n> +++ b/src/ipa/raspberrypi/controller/sharpen_status.h\n> @@ -19,6 +19,8 @@ struct SharpenStatus {\n> \tdouble strength;\n> \t// upper limit of the allowed sharpening response\n> \tdouble limit;\n> +\t// The sharpening strength requested by the user or application.\n> +\tdouble user_strength;\n> };\n> \n> #ifdef __cplusplus","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A3095609A3\n\tfor ;\n\tMon, 22 Jun 2020 04:33:47 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 10ED630D;\n\tMon, 22 Jun 2020 04:33:46 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"K+EkKF9F\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1592793227;\n\tbh=W5KnvzjeuttOmG3dyGl9EOt571e/A9Lde+6RHY7xadE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=K+EkKF9FL/DBX3XsQXZGdVwnWvg+OzDfF4h3J8615rT89rJxGQlO39i7RAcIn72S4\n\tHJxza8NQvsRMXDlwBGMLiIu5oqiRP7u/zPkByAL+rCeL/Zn+pR1PgzjdvX6kvjwzpK\n\tOnwEW99iTr1Q7Sua4rBjYz9paNQMPjAGZGENiGB8=","Date":"Mon, 22 Jun 2020 05:33:22 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200622023322.GF25355@pendragon.ideasonboard.com>","References":"<20200619092725.19109-1-david.plowman@raspberrypi.com>\n\t<20200619092725.19109-3-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200619092725.19109-3-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: raspberrypi: add\n\tsharpness strength control to Raspberry Pi IPAs","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","X-List-Received-Date":"Mon, 22 Jun 2020 02:33:47 -0000"}},{"id":5328,"web_url":"https://patchwork.libcamera.org/comment/5328/","msgid":"","date":"2020-06-22T10:33:10","subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: raspberrypi: add\n\tsharpness strength control to Raspberry Pi IPAs","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Laurent\n\nThanks for the comments!\n\nOn Mon, 22 Jun 2020 at 03:33, Laurent Pinchart\n wrote:\n>\n> Hi David,\n>\n> Thank you for the patch.\n>\n> On Fri, Jun 19, 2020 at 10:27:24AM +0100, David Plowman wrote:\n> > The sharpness control is, loosely speaking, a gain applied to\n> > the amount of sharpening added to an image. We also report the\n> > sharpness setting used back to the caller in metadata.\n> >\n> > Signed-off-by: David Plowman \n> > ---\n> > .../raspberrypi/controller/rpi/sharpen.cpp | 23 +++++++++++++++----\n> > .../raspberrypi/controller/rpi/sharpen.hpp | 6 +++--\n> > .../controller/sharpen_algorithm.hpp | 21 +++++++++++++++++\n> > .../raspberrypi/controller/sharpen_status.h | 2 ++\n> > 4 files changed, 46 insertions(+), 6 deletions(-)\n> > create mode 100644 src/ipa/raspberrypi/controller/sharpen_algorithm.hpp\n> >\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/sharpen.cpp b/src/ipa/raspberrypi/controller/rpi/sharpen.cpp\n> > index 1f07bb6..45fe3fe 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/sharpen.cpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/sharpen.cpp\n> > @@ -17,7 +17,7 @@ using namespace RPi;\n> > #define NAME \"rpi.sharpen\"\n> >\n> > Sharpen::Sharpen(Controller *controller)\n> > - : Algorithm(controller)\n> > + : SharpenAlgorithm(controller), user_strength_(1.0)\n> > {\n> > }\n> >\n> > @@ -40,15 +40,30 @@ void Sharpen::Read(boost::property_tree::ptree const ¶ms)\n> > limit_ = params.get(\"limit\", 1.0);\n> > }\n> >\n> > +void Sharpen::SetStrength(double strength)\n> > +{\n> > + // Note that this method is how an application sets the overall\n> > + // sharpening \"strength\". We call this the \"user strength\" field\n> > + // as there already is a strength_ field - being an internal gain\n> > + // parameter that gets passed to the ISP control code.\n> > + user_strength_ = strength;\n> > +}\n> > +\n> > void Sharpen::Prepare(Metadata *image_metadata)\n> > {\n> > double mode_factor = mode_factor_;\n> > + // The user_strength affects the algorithm's internal gain directly, but\n> > + // we adjust the limit and threshold more cautiously, hence the sqrt.\n>\n> I didn't understand this initially, but reading your camera tuning guide\n> helped. I think s/we adjust/we want to adjust/ would be a bit clearer.\n> Maybe \"hence the sqrt\" could also be reworded to clarify that it's an\n> arbitrary heuristic. Or, if it isn't, then my understanding is incorrect\n> and some more information would be useful :-)\n\nSure, I'll re-word that a bit better!\n\n>\n> > + double user_strength = user_strength_;\n>\n> Do you need this local variable, can't you use user_strength_ directly\n> in the code below ? Or are there multi-threading race conditions ? If\n> so, doesn't the same race condition affect mode_factor and user_strength\n> as you read them both here without any lock ?\n\nTBH, probably the right thing to do is to make mode_factor_ a\nregular double. SwitchMode can only run synchronously with the\nPrepare method, but that might not have been clear to me when\nI first wrote this. SetStrength really does run asynchronously, which\nis why the std::atomic makes sense. I need to fix the potential\ndivide-by-zero from your other comments, too.\n\n>\n> > + double user_strength_sqrt = sqrt(user_strength);\n> > struct SharpenStatus status;\n> > // Binned modes seem to need the sharpening toned down with this\n> > // pipeline.\n>\n> Out of curiosity, why is that ?\n\nIt's a good question. When an image is downscaled the high-pass filter\nresponse, on a per-pixel basis, gets larger (because sharp edges are\nin reality always smeared over a few pixels). This leads to\nover-sharpening, at least in the hardware that we have. I think you\ncan argue that downscaling an image by 2x2 causes lengths to halve and\ntherefore corresponds in some respects to a doubling of some of these\nfilter responses. It so happens that the \"mode factor\" that we encode\nfor our camera modes represents the same thing. But I'd be lying if I\ndidn't confess it's also rather empirical!\n\n>\n> > - status.threshold = threshold_ * mode_factor;\n> > - status.strength = strength_ / mode_factor;\n> > - status.limit = limit_ / mode_factor;\n> > + status.threshold = threshold_ * mode_factor / user_strength_sqrt;\n> > + status.strength = (strength_ / mode_factor) * user_strength;\n> > + status.limit = (limit_ / mode_factor) * user_strength_sqrt;\n>\n> The parentheses are not needed in the above two lines.\n\nNo problem!\n\nVersion 2 of all this incoming shortly...\n\nBest regards\nDavid\n\n>\n> > + // Finally, report any application-supplied parameters that were used.\n> > + status.user_strength = user_strength;\n> > image_metadata->Set(\"sharpen.status\", status);\n> > }\n> >\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/sharpen.hpp b/src/ipa/raspberrypi/controller/rpi/sharpen.hpp\n> > index 3b0d680..5b419b4 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/sharpen.hpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/sharpen.hpp\n> > @@ -6,20 +6,21 @@\n> > */\n> > #pragma once\n> >\n> > -#include \"../algorithm.hpp\"\n> > +#include \"../sharpen_algorithm.hpp\"\n> > #include \"../sharpen_status.h\"\n> >\n> > // This is our implementation of the \"sharpen algorithm\".\n> >\n> > namespace RPi {\n> >\n> > -class Sharpen : public Algorithm\n> > +class Sharpen : public SharpenAlgorithm\n> > {\n> > public:\n> > Sharpen(Controller *controller);\n> > char const *Name() const override;\n> > void SwitchMode(CameraMode const &camera_mode) override;\n> > void Read(boost::property_tree::ptree const ¶ms) override;\n> > + void SetStrength(double strength) override;\n> > void Prepare(Metadata *image_metadata) override;\n> >\n> > private:\n> > @@ -27,6 +28,7 @@ private:\n> > double strength_;\n> > double limit_;\n> > std::atomic mode_factor_;\n> > + std::atomic user_strength_;\n> > };\n> >\n> > } // namespace RPi\n> > diff --git a/src/ipa/raspberrypi/controller/sharpen_algorithm.hpp b/src/ipa/raspberrypi/controller/sharpen_algorithm.hpp\n> > new file mode 100644\n> > index 0000000..3b27a74\n> > --- /dev/null\n> > +++ b/src/ipa/raspberrypi/controller/sharpen_algorithm.hpp\n> > @@ -0,0 +1,21 @@\n> > +/* SPDX-License-Identifier: BSD-2-Clause */\n> > +/*\n> > + * Copyright (C) 2020, Raspberry Pi (Trading) Limited\n> > + *\n> > + * sharpen_algorithm.hpp - sharpness control algorithm interface\n> > + */\n> > +#pragma once\n> > +\n> > +#include \"algorithm.hpp\"\n> > +\n> > +namespace RPi {\n> > +\n> > +class SharpenAlgorithm : public Algorithm\n> > +{\n> > +public:\n> > + SharpenAlgorithm(Controller *controller) : Algorithm(controller) {}\n> > + // A sharpness control algorithm must provide the following:\n> > + virtual void SetStrength(double strength) = 0;\n> > +};\n> > +\n> > +} // namespace RPi\n> > diff --git a/src/ipa/raspberrypi/controller/sharpen_status.h b/src/ipa/raspberrypi/controller/sharpen_status.h\n> > index 6de80f4..7501b19 100644\n> > --- a/src/ipa/raspberrypi/controller/sharpen_status.h\n> > +++ b/src/ipa/raspberrypi/controller/sharpen_status.h\n> > @@ -19,6 +19,8 @@ struct SharpenStatus {\n> > double strength;\n> > // upper limit of the allowed sharpening response\n> > double limit;\n> > + // The sharpening strength requested by the user or application.\n> > + double user_strength;\n> > };\n> >\n> > #ifdef __cplusplus\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from mail-oi1-x244.google.com (mail-oi1-x244.google.com\n\t[IPv6:2607:f8b0:4864:20::244])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 31E29603BD\n\tfor ;\n\tMon, 22 Jun 2020 12:33:23 +0200 (CEST)","by mail-oi1-x244.google.com with SMTP id j189so15120094oih.10\n\tfor ;\n\tMon, 22 Jun 2020 03:33:23 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"bvoI217O\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=/HfJezTvyQYf3x5cyWYzhmsSzQ5lOAHWkziMtJg80e0=;\n\tb=bvoI217On9dyo035k2trzRUpeMr7rIR07nKNmcjn7IwoLOEyjpedBkNSFAwOyPyLOA\n\tFQVPStAbV5r5jMnMdl1cb/rXNCk9OX7hNk9c6sCRp0Pc96T7tZHtLTyfjz3Nh++Zf4z5\n\tu0r7Bd4Yv0FPxM6hDKeNGJOovjuy8HzqPUyWnTdfJZgEYlvQYloEfoWQyPDxNwrDd327\n\td/d7JiuDhFuDVPHJRlcdyQYnJ8HmmIpi5CqOqfauIt6pSNJZLyzxpd60nk7fWEcmthjl\n\tobf4iD958xv8OFHYw45awjveTWEQknGqFIcCXqXISVxfvSZqIVA6llptHdOGp/V0igFm\n\tE1ug==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=/HfJezTvyQYf3x5cyWYzhmsSzQ5lOAHWkziMtJg80e0=;\n\tb=pGW+XhnuyBGLtrF9XjJTS9GY2zbq7MUAz9dM9r0CazahV4nn2Ebxypo95gG44F7esm\n\tc7bEpkScxVTlN3/7owAte56q4PR4K3q3qMjzwK8AurFr9Wx0mCLh9KiqDsJIb6ObymFF\n\tBy66SbArYyvkWCTiBty9+tG62zNm7rEYRhE+zy1QS7VOMrUSgf26ASN6WAxj1ZynEFCC\n\tnOF1+pHOQxyd8oWaH5fKt+LH0qrjPjeaYXhnnVz5+iOlptp8aNdsCuVnJ5Qk0a8yRbVR\n\tg8UubwUoqIZeKHmbAnjZBpmCN6MEqnRflHzwb/OqXfiNuqYkFCC5FpcCIZxF6uI7fbHi\n\tjGaQ==","X-Gm-Message-State":"AOAM532d6M2EFplJTXQjVbCv/7XLa1Py/p5fT3zwPSWh0YL+dWP4A0+e\n\txy1wA+2C13NhLDf2fzjyGD/Zsf9pPs4nkxtrKL4HWR+itiY=","X-Google-Smtp-Source":"ABdhPJythbS5xbppo9EnSrKsNsLEW6Y5gKzZ+Yn7TOP01RNt2/ClpXhF53ENJVWUrEvaGtJ2cnUP22vQRJNCA9UZo7A=","X-Received":"by 2002:aca:8c3:: with SMTP id 186mr11383928oii.55.1592822001994;\n\tMon, 22 Jun 2020 03:33:21 -0700 (PDT)","MIME-Version":"1.0","References":"<20200619092725.19109-1-david.plowman@raspberrypi.com>\n\t<20200619092725.19109-3-david.plowman@raspberrypi.com>\n\t<20200622023322.GF25355@pendragon.ideasonboard.com>","In-Reply-To":"<20200622023322.GF25355@pendragon.ideasonboard.com>","From":"David Plowman ","Date":"Mon, 22 Jun 2020 11:33:10 +0100","Message-ID":"","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 2/3] libcamera: raspberrypi: add\n\tsharpness strength control to Raspberry Pi IPAs","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","X-List-Received-Date":"Mon, 22 Jun 2020 10:33:23 -0000"}}]