[{"id":11721,"web_url":"https://patchwork.libcamera.org/comment/11721/","msgid":"<20200729152602.GI6183@pendragon.ideasonboard.com>","date":"2020-07-29T15:26:02","subject":"Re: [libcamera-devel] [PATCH 3/5] libcamera: raspberrypi: Plumb\n\tuser transform through to individual 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 Wed, Jul 29, 2020 at 10:31:52AM +0100, David Plowman wrote:\n> This commit takes the user/application supplied Transform and plumbs\n> it through to all our individual IPAs using the SwitchMode method of\n> the Controller/Algorithms. IPARPi::configure has to be re-ordered just\n> a little to ensure the userTransform_ is available before calling\n> SwitchMode.\n> ---\n>  src/ipa/raspberrypi/controller/algorithm.cpp  |  4 ++-\n>  src/ipa/raspberrypi/controller/algorithm.hpp  |  3 +-\n>  src/ipa/raspberrypi/controller/controller.cpp |  5 +--\n>  src/ipa/raspberrypi/controller/controller.hpp |  5 ++-\n>  src/ipa/raspberrypi/controller/rpi/agc.cpp    |  3 +-\n>  src/ipa/raspberrypi/controller/rpi/agc.hpp    |  3 +-\n>  src/ipa/raspberrypi/controller/rpi/alsc.cpp   |  3 +-\n>  src/ipa/raspberrypi/controller/rpi/alsc.hpp   |  3 +-\n>  src/ipa/raspberrypi/controller/rpi/noise.cpp  |  4 ++-\n>  src/ipa/raspberrypi/controller/rpi/noise.hpp  |  3 +-\n>  .../raspberrypi/controller/rpi/sharpen.cpp    |  3 +-\n>  .../raspberrypi/controller/rpi/sharpen.hpp    |  3 +-\n>  src/ipa/raspberrypi/raspberrypi.cpp           | 31 ++++++++++---------\n>  13 files changed, 45 insertions(+), 28 deletions(-)\n> \n> diff --git a/src/ipa/raspberrypi/controller/algorithm.cpp b/src/ipa/raspberrypi/controller/algorithm.cpp\n> index 55cb201..b95ebb8 100644\n> --- a/src/ipa/raspberrypi/controller/algorithm.cpp\n> +++ b/src/ipa/raspberrypi/controller/algorithm.cpp\n> @@ -16,9 +16,11 @@ void Algorithm::Read(boost::property_tree::ptree const &params)\n>  \n>  void Algorithm::Initialise() {}\n>  \n> -void Algorithm::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> +void Algorithm::SwitchMode(CameraMode const &camera_mode, libcamera::Transform transform,\n> +\t\t\t\t\t\t   Metadata *metadata)\n\nThat's a strange indentation. Is your text editor configure with 4\nspaces per tab by any chance ? :-)\n\nSame comment for the rest of the file.\n\n>  {\n>  \t(void)camera_mode;\n> +\t(void)transform;\n>  \t(void)metadata;\n\nBy the way, we will likely switch to C++17 in the not too distant\nfuture, it will give us the [[maybe_unused]] attribute.\n\n>  }\n>  \n> diff --git a/src/ipa/raspberrypi/controller/algorithm.hpp b/src/ipa/raspberrypi/controller/algorithm.hpp\n> index 187c50c..b38f167 100644\n> --- a/src/ipa/raspberrypi/controller/algorithm.hpp\n> +++ b/src/ipa/raspberrypi/controller/algorithm.hpp\n> @@ -37,7 +37,8 @@ public:\n>  \tvirtual void Resume() { paused_ = false; }\n>  \tvirtual void Read(boost::property_tree::ptree const &params);\n>  \tvirtual void Initialise();\n> -\tvirtual void SwitchMode(CameraMode const &camera_mode, Metadata *metadata);\n> +\tvirtual void SwitchMode(CameraMode const &camera_mode, libcamera::Transform transform,\n> +\t\t\t\t\t\t\tMetadata *metadata);\n\nInstead of passing it explicitly everywhere, would it make sense to\nstore the transformation in CameraMode ?\n\n>  \tvirtual void Prepare(Metadata *image_metadata);\n>  \tvirtual void Process(StatisticsPtr &stats, Metadata *image_metadata);\n>  \tMetadata &GetGlobalMetadata() const\n> diff --git a/src/ipa/raspberrypi/controller/controller.cpp b/src/ipa/raspberrypi/controller/controller.cpp\n> index 7c4b04f..7a4310f 100644\n> --- a/src/ipa/raspberrypi/controller/controller.cpp\n> +++ b/src/ipa/raspberrypi/controller/controller.cpp\n> @@ -56,11 +56,12 @@ void Controller::Initialise()\n>  \tRPI_LOG(\"Controller finished\");\n>  }\n>  \n> -void Controller::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> +void Controller::SwitchMode(CameraMode const &camera_mode, libcamera::Transform transform,\n> +\t\t\t\t\t\t\tMetadata *metadata)\n>  {\n>  \tRPI_LOG(\"Controller starting\");\n>  \tfor (auto &algo : algorithms_)\n> -\t\talgo->SwitchMode(camera_mode, metadata);\n> +\t\talgo->SwitchMode(camera_mode, transform, metadata);\n>  \tswitch_mode_called_ = true;\n>  \tRPI_LOG(\"Controller finished\");\n>  }\n> diff --git a/src/ipa/raspberrypi/controller/controller.hpp b/src/ipa/raspberrypi/controller/controller.hpp\n> index 6ba9412..738c4b6 100644\n> --- a/src/ipa/raspberrypi/controller/controller.hpp\n> +++ b/src/ipa/raspberrypi/controller/controller.hpp\n> @@ -15,6 +15,8 @@\n>  \n>  #include <linux/bcm2835-isp.h>\n>  \n> +#include <libcamera/transform.h>\n> +\n>  #include \"camera_mode.h\"\n>  #include \"device_status.h\"\n>  #include \"metadata.hpp\"\n> @@ -39,7 +41,8 @@ public:\n>  \tAlgorithm *CreateAlgorithm(char const *name);\n>  \tvoid Read(char const *filename);\n>  \tvoid Initialise();\n> -\tvoid SwitchMode(CameraMode const &camera_mode, Metadata *metadata);\n> +\tvoid SwitchMode(CameraMode const &camera_mode, libcamera::Transform transform,\n> +\t\t\t\t\tMetadata *metadata);\n>  \tvoid Prepare(Metadata *image_metadata);\n>  \tvoid Process(StatisticsPtr stats, Metadata *image_metadata);\n>  \tMetadata &GetGlobalMetadata();\n> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> index c02b5ec..ceac771 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> @@ -221,7 +221,8 @@ void Agc::SetConstraintMode(std::string const &constraint_mode_name)\n>  \tconstraint_mode_name_ = constraint_mode_name;\n>  }\n>  \n> -void Agc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> +void Agc::SwitchMode(CameraMode const &camera_mode, libcamera::Transform transform,\n> +\t\t\t\t\t Metadata *metadata)\n>  {\n>  \t// On a mode switch, it's possible the exposure profile could change,\n>  \t// so we run through the dividing up of exposure/gain again and\n> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> index 9a7e89c..78f016e 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> @@ -75,7 +75,8 @@ public:\n>  \tvoid SetMeteringMode(std::string const &metering_mode_name) override;\n>  \tvoid SetExposureMode(std::string const &exposure_mode_name) override;\n>  \tvoid SetConstraintMode(std::string const &contraint_mode_name) override;\n> -\tvoid SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override;\n> +\tvoid SwitchMode(CameraMode const &camera_mode, libcamera::Transform transform,\n> +\t\t\t\t\tMetadata *metadata) override;\n>  \tvoid Prepare(Metadata *image_metadata) override;\n>  \tvoid Process(StatisticsPtr &stats, Metadata *image_metadata) override;\n>  \n> diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> index 76e2f04..5fac9dd 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp\n> @@ -12,6 +12,7 @@\n>  // Raspberry Pi ALSC (Auto Lens Shading Correction) algorithm.\n>  \n>  using namespace RPi;\n> +using libcamera::Transform;\n\nYou use libcamera::Transform directly in the other files, shouldn't we\ntry to be consistent ?\n\n>  \n>  #define NAME \"rpi.alsc\"\n>  \n> @@ -173,7 +174,7 @@ void Alsc::Initialise()\n>  \t\tlambda_r_[i] = lambda_b_[i] = 1.0;\n>  }\n>  \n> -void Alsc::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> +void Alsc::SwitchMode(CameraMode const &camera_mode, Transform transform, Metadata *metadata)\n>  {\n>  \t(void)metadata;\n\n\t(void)metadata;\n\n?\n\nSame for Agc::SwitchMode() and other functions below.\n\n>  \n> diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.hpp b/src/ipa/raspberrypi/controller/rpi/alsc.hpp\n> index 3806257..9b54578 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/alsc.hpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/alsc.hpp\n> @@ -50,7 +50,8 @@ public:\n>  \t~Alsc();\n>  \tchar const *Name() const override;\n>  \tvoid Initialise() override;\n> -\tvoid SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override;\n> +\tvoid SwitchMode(CameraMode const &camera_mode, libcamera::Transform transform,\n> +\t\t\t\t\tMetadata *metadata) override;\n>  \tvoid Read(boost::property_tree::ptree const &params) override;\n>  \tvoid Prepare(Metadata *image_metadata) override;\n>  \tvoid Process(StatisticsPtr &stats, Metadata *image_metadata) override;\n> diff --git a/src/ipa/raspberrypi/controller/rpi/noise.cpp b/src/ipa/raspberrypi/controller/rpi/noise.cpp\n> index 2cafde3..0f4002b 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/noise.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/noise.cpp\n> @@ -27,8 +27,10 @@ char const *Noise::Name() const\n>  \treturn NAME;\n>  }\n>  \n> -void Noise::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> +void Noise::SwitchMode(CameraMode const &camera_mode, libcamera::Transform transform,\n> +\t\t\t\t\t   Metadata *metadata)\n>  {\n> +\t(void)transform;\n>  \t(void)metadata;\n>  \n>  \t// For example, we would expect a 2x2 binned mode to have a \"noise\n> diff --git a/src/ipa/raspberrypi/controller/rpi/noise.hpp b/src/ipa/raspberrypi/controller/rpi/noise.hpp\n> index 25bf188..37a22fd 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/noise.hpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/noise.hpp\n> @@ -18,7 +18,8 @@ class Noise : public Algorithm\n>  public:\n>  \tNoise(Controller *controller);\n>  \tchar const *Name() const override;\n> -\tvoid SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override;\n> +\tvoid SwitchMode(CameraMode const &camera_mode, libcamera::Transform transform,\n> +\t\t\t\t\tMetadata *metadata) override;\n>  \tvoid Read(boost::property_tree::ptree const &params) override;\n>  \tvoid Prepare(Metadata *image_metadata) override;\n>  \n> diff --git a/src/ipa/raspberrypi/controller/rpi/sharpen.cpp b/src/ipa/raspberrypi/controller/rpi/sharpen.cpp\n> index 2b701db..64d269d 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/sharpen.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/sharpen.cpp\n> @@ -26,7 +26,8 @@ char const *Sharpen::Name() const\n>  \treturn NAME;\n>  }\n>  \n> -void Sharpen::SwitchMode(CameraMode const &camera_mode, Metadata *metadata)\n> +void Sharpen::SwitchMode(CameraMode const &camera_mode, libcamera::Transform transform,\n> +\t\t\t\t\t\t Metadata *metadata)\n>  {\n>  \t(void)metadata;\n>  \n> diff --git a/src/ipa/raspberrypi/controller/rpi/sharpen.hpp b/src/ipa/raspberrypi/controller/rpi/sharpen.hpp\n> index 568521b..8a1c164 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/sharpen.hpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/sharpen.hpp\n> @@ -18,7 +18,8 @@ class Sharpen : public SharpenAlgorithm\n>  public:\n>  \tSharpen(Controller *controller);\n>  \tchar const *Name() const override;\n> -\tvoid SwitchMode(CameraMode const &camera_mode, Metadata *metadata) override;\n> +\tvoid SwitchMode(CameraMode const &camera_mode, libcamera::Transform transform,\n> +\t\t\t\t\tMetadata *metadata) override;\n>  \tvoid Read(boost::property_tree::ptree const &params) override;\n>  \tvoid SetStrength(double strength) override;\n>  \tvoid Prepare(Metadata *image_metadata) override;\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 2809521..a8676fb 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -270,21 +270,6 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n>  \t\tagcStatus.analogue_gain = DEFAULT_ANALOGUE_GAIN;\n>  \t}\n>  \n> -\tRPi::Metadata metadata;\n> -\tcontroller_.SwitchMode(mode_, &metadata);\n> -\n> -\t/* SwitchMode may supply updated exposure/gain values to use. */\n> -\tmetadata.Get(\"agc.status\", agcStatus);\n> -\tif (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {\n> -\t\tControlList ctrls(unicam_ctrls_);\n> -\t\tapplyAGC(&agcStatus, ctrls);\n> -\t\tresult->controls.push_back(ctrls);\n> -\n> -\t\tresult->operation |= RPI_IPA_CONFIG_SENSOR;\n> -\t}\n> -\n> -\tlastMode_ = mode_;\n> -\n>  \t/* Store the lens shading table pointer and handle if available. */\n>  \tunsigned int next_element = 0;\n>  \tif (ipaConfig.operation & RPI_IPA_CONFIG_LS_TABLE) {\n> @@ -312,6 +297,22 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n>  \t\tuint32_t transformType = ipaConfig.data[next_element++];\n>  \t\tuserTransform_ = reinterpret_cast<Transform &>(transformType);\n>  \t}\n> +\n> +\t/* Finally we can tell the IPAs about the new camera mode. */\n> +\tRPi::Metadata metadata;\n> +\tcontroller_.SwitchMode(mode_, userTransform_, &metadata);\n> +\n> +\t/* SwitchMode may supply updated exposure/gain values to use. */\n> +\tmetadata.Get(\"agc.status\", agcStatus);\n> +\tif (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {\n> +\t\tControlList ctrls(unicam_ctrls_);\n> +\t\tapplyAGC(&agcStatus, ctrls);\n> +\t\tresult->controls.push_back(ctrls);\n> +\n> +\t\tresult->operation |= RPI_IPA_CONFIG_SENSOR;\n> +\t}\n> +\n> +\tlastMode_ = mode_;\n>  }\n>  \n>  void IPARPi::mapBuffers(const std::vector<IPABuffer> &buffers)","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 ECA2BBD86F\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 29 Jul 2020 15:26:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7F47B613C6;\n\tWed, 29 Jul 2020 17:26:12 +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 564286039D\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 29 Jul 2020 17:26:11 +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 C11D931F;\n\tWed, 29 Jul 2020 17:26:10 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"VB7QySU5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1596036370;\n\tbh=gn4pm3B3CLz+b5T6XLDyts0z5FBImspKfNQQW3Jq5rg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VB7QySU5Lt96x3A4Y9MnWxIcHVFDXD4bF0dW4CDX8m/iLj3FhibeBsf6jtMEEAcE4\n\tEkqtUXEWFP/9E25UDKDcZZFYro7UIj9R0s5hMOa6Wedw2vbbUetTseK0wg0zxykdaQ\n\tit9gGZ8iSYr7XUzVaY5EGUOh8OE/ohEK2O7o5j/g=","Date":"Wed, 29 Jul 2020 18:26:02 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"David Plowman <david.plowman@raspberrypi.com>","Message-ID":"<20200729152602.GI6183@pendragon.ideasonboard.com>","References":"<20200729093154.12296-1-david.plowman@raspberrypi.com>\n\t<20200729093154.12296-4-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200729093154.12296-4-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH 3/5] libcamera: raspberrypi: Plumb\n\tuser transform through to individual IPAs","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>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]