[{"id":27686,"web_url":"https://patchwork.libcamera.org/comment/27686/","msgid":"","date":"2023-08-22T13:15:48","subject":"Re: [libcamera-devel] [PATCH 3/5] ipa: rpi: agc: Implementation of\n\tmulti-channel AGC","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi David,\n\nThank you for this patch.\n\nOn Mon, 31 Jul 2023 at 10:46, David Plowman via libcamera-devel\n wrote:\n>\n> The switchMode, prepare and process methods are updated to implement\n> multi-channel AGC correctly:\n>\n> * switchMode now invokes switchMode on all the channels (whether\n> active or not).\n>\n> * prepare must find what channel the current frame is, and run on\n> behalf of that channel.\n>\n> * process updates the most recent DeviceStatus and statistics for the\n> channel of the frame that has just arrived, but generates updated\n> values working through the active channels in round-robin fashion.\n>\n> One minor detail in process is that we don't want to change the\n> DeviceStatus metadata of the current frame, so we now pass this to the\n> AgcChannel's process method, rather than letting it find the\n> DeviceStatus in the metadata.\n>\n> Signed-off-by: David Plowman \n\nReviewed-by: Naushir Patuck \n\n> ---\n> src/ipa/rpi/controller/agc_status.h | 1 +\n> src/ipa/rpi/controller/rpi/agc.cpp | 108 +++++++++++++++++++--\n> src/ipa/rpi/controller/rpi/agc.h | 1 +\n> src/ipa/rpi/controller/rpi/agc_channel.cpp | 13 +--\n> src/ipa/rpi/controller/rpi/agc_channel.h | 4 +-\n> 5 files changed, 109 insertions(+), 18 deletions(-)\n>\n> diff --git a/src/ipa/rpi/controller/agc_status.h b/src/ipa/rpi/controller/agc_status.h\n> index 597eddd7..e5c4ee22 100644\n> --- a/src/ipa/rpi/controller/agc_status.h\n> +++ b/src/ipa/rpi/controller/agc_status.h\n> @@ -36,6 +36,7 @@ struct AgcStatus {\n> int floatingRegionEnable;\n> libcamera::utils::Duration fixedShutter;\n> double fixedAnalogueGain;\n> + unsigned int channel;\n> };\n>\n> struct AgcPrepareStatus {\n> diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp\n> index c9c9c297..7e627bba 100644\n> --- a/src/ipa/rpi/controller/rpi/agc.cpp\n> +++ b/src/ipa/rpi/controller/rpi/agc.cpp\n> @@ -22,7 +22,8 @@ LOG_DEFINE_CATEGORY(RPiAgc)\n>\n> Agc::Agc(Controller *controller)\n> : AgcAlgorithm(controller),\n> - activeChannels_({ 0 })\n> + activeChannels_({ 0 }),\n> + index_(0)\n> {\n> }\n>\n> @@ -203,20 +204,113 @@ void Agc::setActiveChannels(const std::vector &activeChannels)\n> void Agc::switchMode(CameraMode const &cameraMode,\n> Metadata *metadata)\n> {\n> - LOG(RPiAgc, Debug) << \"switchMode for channel 0\";\n> - channelData_[0].channel.switchMode(cameraMode, metadata);\n> + /*\n> + * We run switchMode on every channel, and then we're going to start over\n> + * with the first active channel again which means that this channel's\n> + * status needs to be the one we leave in the metadata.\n> + */\n> + AgcStatus status;\n> +\n> + for (unsigned int channelIndex = 0; channelIndex < channelData_.size(); channelIndex++) {\n> + LOG(RPiAgc, Debug) << \"switchMode for channel \" << channelIndex;\n> + channelData_[channelIndex].channel.switchMode(cameraMode, metadata);\n> + if (channelIndex == activeChannels_[0])\n> + metadata->get(\"agc.status\", status);\n> + }\n> +\n> + status.channel = activeChannels_[0];\n> + metadata->set(\"agc.status\", status);\n> + index_ = 0;\n> +}\n> +\n> +static void getChannelIndex(Metadata *metadata, const char *message, unsigned int &channelIndex)\n> +{\n> + std::unique_lock lock(*metadata);\n> + AgcStatus *status = metadata->getLocked(\"agc.delayed_status\");\n> + if (status)\n> + channelIndex = status->channel;\n> + else\n> + /* This does happen at startup, otherwise it would be a Warning or Error. */\n> + LOG(RPiAgc, Debug) << message;\n> +}\n> +\n> +static void setChannelIndex(Metadata *metadata, const char *message, unsigned int channelIndex)\n> +{\n> + std::unique_lock lock(*metadata);\n> + AgcStatus *status = metadata->getLocked(\"agc.status\");\n> + if (status)\n> + status->channel = channelIndex;\n> + else\n> + /* This does happen at startup, otherwise it would be a Warning or Error. */\n> + LOG(RPiAgc, Debug) << message;\n> }\n>\n> void Agc::prepare(Metadata *imageMetadata)\n> {\n> - LOG(RPiAgc, Debug) << \"prepare for channel 0\";\n> - channelData_[0].channel.prepare(imageMetadata);\n> + /*\n> + * The DeviceStatus in the metadata should be correct for the image we\n> + * are processing. THe delayed status should tell us what channel this frame\n> + * was from, so we will use that channel's prepare method.\n> + *\n> + * \\todo To be honest, there's not much that's stateful in the prepare methods\n> + * so we should perhaps re-evaluate whether prepare even needs to be done\n> + * \"per channel\".\n> + */\n> + unsigned int channelIndex = activeChannels_[0];\n> + getChannelIndex(imageMetadata, \"prepare: no delayed status\", channelIndex);\n> +\n> + LOG(RPiAgc, Debug) << \"prepare for channel \" << channelIndex;\n> + channelData_[channelIndex].channel.prepare(imageMetadata);\n> }\n>\n> void Agc::process(StatisticsPtr &stats, Metadata *imageMetadata)\n> {\n> - LOG(RPiAgc, Debug) << \"process for channel 0\";\n> - channelData_[0].channel.process(stats, imageMetadata);\n> + /*\n> + * We want to generate values for the next channel in round robin fashion\n> + * (i.e. the channel at location index_ in the activeChannel list), even though\n> + * the statistics we have will be for a different channel (which we find\n> + * again from the delayed status).\n> + */\n> +\n> + /* Generate updated AGC values for this channel: */\n> + unsigned int channelIndex = activeChannels_[index_];\n> + AgcChannelData &channelData = channelData_[channelIndex];\n> + /* Stats are from this channel: */\n> + unsigned int statsIndex = 0;\n> + getChannelIndex(imageMetadata, \"process: no delayed status for stats\", statsIndex);\n> + LOG(RPiAgc, Debug) << \"process for channel \" << channelIndex;\n> +\n> + /*\n> + * We keep a cache of the most recent DeviceStatus and stats for each channel,\n> + * so that we can invoke the next channel's process method with the most up to date\n> + * values.\n> + */\n> + LOG(RPiAgc, Debug) << \"Save DeviceStatus and stats for channel \" << statsIndex;\n> + DeviceStatus deviceStatus;\n> + if (imageMetadata->get(\"device.status\", deviceStatus) == 0)\n> + channelData_[statsIndex].deviceStatus = deviceStatus;\n> + else\n> + /* Every frame should have a DeviceStatus. */\n> + LOG(RPiAgc, Error) << \"process: no device status found\";\n> + channelData_[statsIndex].statistics = stats;\n> +\n> + /*\n> + * Finally fetch the most recent DeviceStatus and stats for this channel, if both\n> + * exist, and call process(). We must make the agc.status metadata record correctly\n> + * which channel this is.\n> + */\n> + if (channelData.statistics && channelData.deviceStatus) {\n> + deviceStatus = *channelData.deviceStatus;\n> + stats = channelData.statistics;\n> + } else\n> + /* Can also happen when new channels start. */\n> + LOG(RPiAgc, Debug) << \"process: channel \" << channelIndex << \" not seen yet\";\n> +\n> + channelData.channel.process(stats, &deviceStatus, imageMetadata);\n> + setChannelIndex(imageMetadata, \"process: no AGC status found\", channelIndex);\n> +\n> + /* And onto the next channel for the next call. */\n> + index_ = (index_ + 1) % activeChannels_.size();\n> }\n>\n> /* Register algorithm with the system. */\n> diff --git a/src/ipa/rpi/controller/rpi/agc.h b/src/ipa/rpi/controller/rpi/agc.h\n> index a9158910..2eed2bab 100644\n> --- a/src/ipa/rpi/controller/rpi/agc.h\n> +++ b/src/ipa/rpi/controller/rpi/agc.h\n> @@ -53,6 +53,7 @@ private:\n> int checkChannel(unsigned int channel) const;\n> std::vector channelData_;\n> std::vector activeChannels_;\n> + unsigned int index_; /* index into the activeChannels_ */\n> };\n>\n> } /* namespace RPiController */\n> diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> index d6e30ef2..ddec611f 100644\n> --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> @@ -447,7 +447,7 @@ void AgcChannel::prepare(Metadata *imageMetadata)\n> }\n> }\n>\n> -void AgcChannel::process(StatisticsPtr &stats, Metadata *imageMetadata)\n> +void AgcChannel::process(StatisticsPtr &stats, const DeviceStatus *deviceStatus, Metadata *imageMetadata)\n> {\n> frameCount_++;\n> /*\n> @@ -458,7 +458,7 @@ void AgcChannel::process(StatisticsPtr &stats, Metadata *imageMetadata)\n> /* Fetch the AWB status immediately, so that we can assume it's there. */\n> fetchAwbStatus(imageMetadata);\n> /* Get the current exposure values for the frame that's just arrived. */\n> - fetchCurrentExposure(imageMetadata);\n> + fetchCurrentExposure(deviceStatus);\n> /* Compute the total gain we require relative to the current exposure. */\n> double gain, targetY;\n> computeGain(stats, imageMetadata, gain, targetY);\n> @@ -570,18 +570,13 @@ void AgcChannel::housekeepConfig()\n> << meteringModeName_;\n> }\n>\n> -void AgcChannel::fetchCurrentExposure(Metadata *imageMetadata)\n> +void AgcChannel::fetchCurrentExposure(const DeviceStatus *deviceStatus)\n> {\n> - std::unique_lock lock(*imageMetadata);\n> - DeviceStatus *deviceStatus =\n> - imageMetadata->getLocked(\"device.status\");\n> if (!deviceStatus)\n> LOG(RPiAgc, Fatal) << \"No device metadata\";\n> current_.shutter = deviceStatus->shutterSpeed;\n> current_.analogueGain = deviceStatus->analogueGain;\n> - AgcStatus *agcStatus =\n> - imageMetadata->getLocked(\"agc.status\");\n> - current_.totalExposure = agcStatus ? agcStatus->totalExposureValue : 0s;\n> + current_.totalExposure = 0s; /* this value is unused */\n> current_.totalExposureNoDG = current_.shutter * current_.analogueGain;\n> }\n>\n> diff --git a/src/ipa/rpi/controller/rpi/agc_channel.h b/src/ipa/rpi/controller/rpi/agc_channel.h\n> index dc4356f3..0e3d44b0 100644\n> --- a/src/ipa/rpi/controller/rpi/agc_channel.h\n> +++ b/src/ipa/rpi/controller/rpi/agc_channel.h\n> @@ -83,13 +83,13 @@ public:\n> void disableAuto();\n> void switchMode(CameraMode const &cameraMode, Metadata *metadata);\n> void prepare(Metadata *imageMetadata);\n> - void process(StatisticsPtr &stats, Metadata *imageMetadata);\n> + void process(StatisticsPtr &stats, const DeviceStatus *deviceStatus, Metadata *imageMetadata);\n>\n> private:\n> bool updateLockStatus(DeviceStatus const &deviceStatus);\n> AgcConfig config_;\n> void housekeepConfig();\n> - void fetchCurrentExposure(Metadata *imageMetadata);\n> + void fetchCurrentExposure(const DeviceStatus *deviceStatus);\n> void fetchAwbStatus(Metadata *imageMetadata);\n> void computeGain(StatisticsPtr &statistics, Metadata *imageMetadata,\n> double &gain, double &targetY);\n> --\n> 2.30.2\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 31305BF415\n\tfor ;\n\tTue, 22 Aug 2023 13:15:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 95DA5627E0;\n\tTue, 22 Aug 2023 15:15:57 +0200 (CEST)","from mail-yw1-x1133.google.com (mail-yw1-x1133.google.com\n\t[IPv6:2607:f8b0:4864:20::1133])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 516DA6055E\n\tfor ;\n\tTue, 22 Aug 2023 15:15:56 +0200 (CEST)","by mail-yw1-x1133.google.com with SMTP id\n\t00721157ae682-5924093a9b2so11780177b3.2\n\tfor ;\n\tTue, 22 Aug 2023 06:15:56 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1692710157;\n\tbh=YNuUd94IFZ7YE0f3+PBDzE/awHAnnENIVb+df9OFkx4=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=olAQlVwALAw4ae5jiV2wthm7ma4gztRnP4JMy5uKhvTlrM2hKoKSM0pkSVsR/uv13\n\trxBZzww90e9qisyS2/T4H+84Uh7HhFh+1MxcAo/wTGw6/KxJOjmLBrNIl5GDgGT8G4\n\tRMHIZ2vxVrF2lNnEX9o0eoDTFk2X/KKvAlsCu2oPqbir5TlYysO2E0JhneeifG5Qk7\n\t4YjxjuoNpVYpZP0CWH2sKv3CteCD9JRCM2IGO5hOwgd1pJF0/GAX7HxqIrVUoHg7SF\n\tsqSqA0g6UBDhv1DN9JC5eHTzDbnqcBu+YtyoRKbbemg8HkL8fV9708wjVQBxuz73Jh\n\t52e6RFYByMR/A==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1692710155; x=1693314955;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=8EPYxb8zSNOtEnYuniEEm7orhepBb77wGYOC46K0N+k=;\n\tb=ZNk7x/zsgpagsZg4Jj2z0gPVNkUSoceyaUaby1qYLjClDUvuBxrz8H21YbzasSmlla\n\t8WP0Oq+sGJpD41BSjDJjdSXunIWcbjpWPT77RqgTezdmijptMgE6D/y0nI4/bnOBvnVa\n\t9j9U8TZwm8UHEcOTWfryL46kZ27GQ3auHxdPRGjLPmgMX6dC4qKbtacQ8TPIDpIRMGvl\n\tF6CCrWY1VC5sTjX+jbsYxt0CuF8RmehfvEbjNO7e5Ja5EuWKU0+ysPL4un/AHfynbTkR\n\tgamgHfXsNMu6W+ZByuD0IUql8VXee4L6ecjbmvOJ5uIV4hmzPl3wibBdJ3WiOjw+k6+M\n\tO9jg=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"ZNk7x/zs\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1692710155; x=1693314955;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=8EPYxb8zSNOtEnYuniEEm7orhepBb77wGYOC46K0N+k=;\n\tb=L6KIoisYQYEay2Bs3CfbGOy3E501GdPcIRS7DuqIAsUAuO779oRWx/4/zYzUGQecSV\n\t7ivgLa893Ccb4pnzVeQfq/DtIOwIERPINahnjiTPNILAjntRS5Z9VSj1phk3Y+rfARfw\n\tDT5/qFoMRVtenYnpTl33fhC06i6VJIHhryICS30qu4juq4B/FxgFMYYibBYwh13eaVvi\n\tsmiKadjwzcdPO+HR2+43AmhESpPD8/WIdNEOOBhMowRsdzxtjXQtBDKWSoEtHk+2Y86w\n\tpY7l2rZKiWxe3d+nhxob8NQR8TTwG8qxQtbFLeJ53ZaG6AqdENBWmJs2MxrYISBql/qF\n\tpsFQ==","X-Gm-Message-State":"AOJu0YxLbmNAjQeqeWc12NUEvcNPB/ZhlnL99/xQgHMsZxnCLM8j+igs\n\tESp6i9SAf6xSdadBMJm3lOvSVdXjwC0fDKCFosMQk+4aiyJbns+xtPZguw==","X-Google-Smtp-Source":"AGHT+IEi4FwB5MguUUBB3MBpiA2dbA2bRDYsDkEk1G8njPpEAHFTP9I41UvGOifGwbyE9KNWRDcEPLD+Ny54qh6G3ZM=","X-Received":"by 2002:a81:8409:0:b0:56f:fffc:56ff with SMTP id\n\tu9-20020a818409000000b0056ffffc56ffmr11645467ywf.42.1692710155060;\n\tTue, 22 Aug 2023 06:15:55 -0700 (PDT)","MIME-Version":"1.0","References":"<20230731094641.73646-1-david.plowman@raspberrypi.com>\n\t<20230731094641.73646-4-david.plowman@raspberrypi.com>","In-Reply-To":"<20230731094641.73646-4-david.plowman@raspberrypi.com>","Date":"Tue, 22 Aug 2023 14:15:48 +0100","Message-ID":"","To":"David Plowman ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 3/5] ipa: rpi: agc: Implementation of\n\tmulti-channel AGC","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]