[{"id":27711,"web_url":"https://patchwork.libcamera.org/comment/27711/","msgid":"<7l3q76li7qbapltbskqxtyjhkcojw3d5b67lredsnehshffcih@p4mb7bq2wnb2>","date":"2023-08-30T07:56:59","subject":"Re: [libcamera-devel] [PATCH v2 3/5] ipa: rpi: agc: Implementation\n\tof multi-channel AGC","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi David\n\nOn Wed, Aug 23, 2023 at 01:09:13PM +0100, David Plowman via libcamera-devel wrote:\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> Reviewed-by: Naushir Patuck \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> \tint floatingRegionEnable;\n> \tlibcamera::utils::Duration fixedShutter;\n> \tdouble fixedAnalogueGain;\n> +\tunsigned 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> \t: AgcAlgorithm(controller),\n> -\t activeChannels_({ 0 })\n> +\t activeChannels_({ 0 }),\n> +\t 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> \t\t Metadata *metadata)\n> {\n> -\tLOG(RPiAgc, Debug) << \"switchMode for channel 0\";\n> -\tchannelData_[0].channel.switchMode(cameraMode, metadata);\n> +\t/*\n> +\t * We run switchMode on every channel, and then we're going to start over\n> +\t * with the first active channel again which means that this channel's\n> +\t * status needs to be the one we leave in the metadata.\n> +\t */\n> +\tAgcStatus status;\n> +\n> +\tfor (unsigned int channelIndex = 0; channelIndex < channelData_.size(); channelIndex++) {\n> +\t\tLOG(RPiAgc, Debug) << \"switchMode for channel \" << channelIndex;\n> +\t\tchannelData_[channelIndex].channel.switchMode(cameraMode, metadata);\n> +\t\tif (channelIndex == activeChannels_[0])\n> +\t\t\tmetadata->get(\"agc.status\", status);\n> +\t}\n> +\n> +\tstatus.channel = activeChannels_[0];\n> +\tmetadata->set(\"agc.status\", status);\n> +\tindex_ = 0;\n> +}\n> +\n> +static void getChannelIndex(Metadata *metadata, const char *message, unsigned int &channelIndex)\n> +{\n> +\tstd::unique_lock lock(*metadata);\n> +\tAgcStatus *status = metadata->getLocked(\"agc.delayed_status\");\n> +\tif (status)\n> +\t\tchannelIndex = status->channel;\n\nI'm missing in the code base where delayed_status.channel gets set :/\n\n> +\telse\n> +\t\t/* This does happen at startup, otherwise it would be a Warning or Error. */\n> +\t\tLOG(RPiAgc, Debug) << message;\n\nIf this is a known condition, do you need to have the message\npolluting the logs ?\n\n> +}\n> +\n> +static void setChannelIndex(Metadata *metadata, const char *message, unsigned int channelIndex)\n> +{\n> +\tstd::unique_lock lock(*metadata);\n> +\tAgcStatus *status = metadata->getLocked(\"agc.status\");\n> +\tif (status)\n> +\t\tstatus->channel = channelIndex;\n> +\telse\n> +\t\t/* This does happen at startup, otherwise it would be a Warning or Error. */\n> +\t\tLOG(RPiAgc, Debug) << message;\n\nditto\n\n> }\n>\n> void Agc::prepare(Metadata *imageMetadata)\n> {\n> -\tLOG(RPiAgc, Debug) << \"prepare for channel 0\";\n> -\tchannelData_[0].channel.prepare(imageMetadata);\n> +\t/*\n> +\t * The DeviceStatus in the metadata should be correct for the image we\n> +\t * are processing. THe delayed status should tell us what channel this frame\n> +\t * was from, so we will use that channel's prepare method.\n> +\t *\n> +\t * \\todo To be honest, there's not much that's stateful in the prepare methods\n> +\t * so we should perhaps re-evaluate whether prepare even needs to be done\n> +\t * \"per channel\".\n\non which channel would you do 'prepare' if not the last active one ?\n\n> +\t */\n> +\tunsigned int channelIndex = activeChannels_[0];\n> +\tgetChannelIndex(imageMetadata, \"prepare: no delayed status\", channelIndex);\n> +\n> +\tLOG(RPiAgc, Debug) << \"prepare for channel \" << channelIndex;\n> +\tchannelData_[channelIndex].channel.prepare(imageMetadata);\n> }\n>\n> void Agc::process(StatisticsPtr &stats, Metadata *imageMetadata)\n> {\n> -\tLOG(RPiAgc, Debug) << \"process for channel 0\";\n> -\tchannelData_[0].channel.process(stats, imageMetadata);\n> +\t/*\n> +\t * We want to generate values for the next channel in round robin fashion\n> +\t * (i.e. the channel at location index_ in the activeChannel list), even though\n> +\t * the statistics we have will be for a different channel (which we find\n> +\t * again from the delayed status).\n> +\t */\n> +\n> +\t/* Generate updated AGC values for this channel: */\n> +\tunsigned int channelIndex = activeChannels_[index_];\n> +\tAgcChannelData &channelData = channelData_[channelIndex];\n> +\t/* Stats are from this channel: */\n> +\tunsigned int statsIndex = 0;\n\nwhy not use 'activeChannels_[index_]' again here ?\n\n> +\tgetChannelIndex(imageMetadata, \"process: no delayed status for stats\", statsIndex);\n> +\tLOG(RPiAgc, Debug) << \"process for channel \" << channelIndex;\n> +\n> +\t/*\n> +\t * We keep a cache of the most recent DeviceStatus and stats for each channel,\n> +\t * so that we can invoke the next channel's process method with the most up to date\n> +\t * values.\n> +\t */\n> +\tLOG(RPiAgc, Debug) << \"Save DeviceStatus and stats for channel \" << statsIndex;\n> +\tDeviceStatus deviceStatus;\n> +\tif (imageMetadata->get(\"device.status\", deviceStatus) == 0)\n> +\t\tchannelData_[statsIndex].deviceStatus = deviceStatus;\n> +\telse\n> +\t\t/* Every frame should have a DeviceStatus. */\n> +\t\tLOG(RPiAgc, Error) << \"process: no device status found\";\n> +\tchannelData_[statsIndex].statistics = stats;\n\nAgain I'm missing why we need to use a different channelIndex and\nstatsIndex..\n> +\n> +\t/*\n> +\t * Finally fetch the most recent DeviceStatus and stats for this channel, if both\n> +\t * exist, and call process(). We must make the agc.status metadata record correctly\n> +\t * which channel this is.\n> +\t */\n> +\tif (channelData.statistics && channelData.deviceStatus) {\n> +\t\tdeviceStatus = *channelData.deviceStatus;\n> +\t\tstats = channelData.statistics;\n\nDo you need to copy ? Could channelData.statistics get overwritten\nduring the process() call chain ?\n\n> +\t} else\n> +\t\t/* Can also happen when new channels start. */\n> +\t\tLOG(RPiAgc, Debug) << \"process: channel \" << channelIndex << \" not seen yet\";\n> +\n> +\tchannelData.channel.process(stats, &deviceStatus, imageMetadata);\n> +\tsetChannelIndex(imageMetadata, \"process: no AGC status found\", channelIndex);\n> +\n> +\t/* And onto the next channel for the next call. */\n> +\tindex_ = (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> \tint checkChannel(unsigned int channel) const;\n> \tstd::vector channelData_;\n> \tstd::vector activeChannels_;\n> +\tunsigned 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> \t}\n> }\n>\n> -void AgcChannel::process(StatisticsPtr &stats, Metadata *imageMetadata)\n> +void AgcChannel::process(StatisticsPtr &stats, const DeviceStatus *deviceStatus, Metadata *imageMetadata)\n> {\n> \tframeCount_++;\n> \t/*\n> @@ -458,7 +458,7 @@ void AgcChannel::process(StatisticsPtr &stats, Metadata *imageMetadata)\n> \t/* Fetch the AWB status immediately, so that we can assume it's there. */\n> \tfetchAwbStatus(imageMetadata);\n> \t/* Get the current exposure values for the frame that's just arrived. */\n> -\tfetchCurrentExposure(imageMetadata);\n> +\tfetchCurrentExposure(deviceStatus);\n> \t/* Compute the total gain we require relative to the current exposure. */\n> \tdouble gain, targetY;\n> \tcomputeGain(stats, imageMetadata, gain, targetY);\n> @@ -570,18 +570,13 @@ void AgcChannel::housekeepConfig()\n> \t\t\t << meteringModeName_;\n> }\n>\n> -void AgcChannel::fetchCurrentExposure(Metadata *imageMetadata)\n> +void AgcChannel::fetchCurrentExposure(const DeviceStatus *deviceStatus)\n> {\n> -\tstd::unique_lock lock(*imageMetadata);\n> -\tDeviceStatus *deviceStatus =\n> -\t\timageMetadata->getLocked(\"device.status\");\n> \tif (!deviceStatus)\n> \t\tLOG(RPiAgc, Fatal) << \"No device metadata\";\n> \tcurrent_.shutter = deviceStatus->shutterSpeed;\n> \tcurrent_.analogueGain = deviceStatus->analogueGain;\n> -\tAgcStatus *agcStatus =\n> -\t\timageMetadata->getLocked(\"agc.status\");\n> -\tcurrent_.totalExposure = agcStatus ? agcStatus->totalExposureValue : 0s;\n> +\tcurrent_.totalExposure = 0s; /* this value is unused */\n\nUh, why is this not used anymore ? :)\n\n> \tcurrent_.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> \tvoid disableAuto();\n> \tvoid switchMode(CameraMode const &cameraMode, Metadata *metadata);\n> \tvoid prepare(Metadata *imageMetadata);\n> -\tvoid process(StatisticsPtr &stats, Metadata *imageMetadata);\n> +\tvoid process(StatisticsPtr &stats, const DeviceStatus *deviceStatus, Metadata *imageMetadata);\n>\n> private:\n> \tbool updateLockStatus(DeviceStatus const &deviceStatus);\n> \tAgcConfig config_;\n> \tvoid housekeepConfig();\n> -\tvoid fetchCurrentExposure(Metadata *imageMetadata);\n> +\tvoid fetchCurrentExposure(const DeviceStatus *deviceStatus);\n> \tvoid fetchAwbStatus(Metadata *imageMetadata);\n> \tvoid computeGain(StatisticsPtr &statistics, Metadata *imageMetadata,\n> \t\t\t 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 7E21BC0F1B\n\tfor ;\n\tWed, 30 Aug 2023 07:57:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C2CEB627E0;\n\tWed, 30 Aug 2023 09:57:04 +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 D1FC961E00\n\tfor ;\n\tWed, 30 Aug 2023 09:57:03 +0200 (CEST)","from ideasonboard.com (mob-5-90-48-184.net.vodafone.it\n\t[5.90.48.184])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4342E2D8;\n\tWed, 30 Aug 2023 09:55:41 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1693382224;\n\tbh=IPiMSvNAn0Akuu1/V4TAuR6gHehyKcjcAaQ6vKz1Jrw=;\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=Lj60FYSOZsxMy0OOqdwSiUfKhGNCN0oz/haHBFZ1DuAHY9Ahw264YuKvLlp0Juj5X\n\tk8x43NRWDS8QB3uAFkDL1hI9Qk+q1y7u/lQFRigQeVQhPwHk+h6hpSs0FeTSozm0l8\n\tGEiukcTgmmGiXxAn2gXVRmozfvhjuVY9gtSzyIUGkxWCgxVZ2pbuJwXmkBEJD+0MAP\n\tUg8lcSxobsvMJRRTuyExZE+0MDG817ffo/J9SfSbyj/emUnBskEt+9P5q4NEV7RBqD\n\tNVuUk2s7pCcNFYv+xiEDSOxDj27+wFdiIdjKJ4caQ1rXMDMJ+HytQEnnciXyj10ALA\n\tBwHH8F5JQxHXw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1693382141;\n\tbh=IPiMSvNAn0Akuu1/V4TAuR6gHehyKcjcAaQ6vKz1Jrw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YtQqX9lTIH3RlDfQ6MMOHrLjtTpDLdWhI8EwxXOOha10iVjbxLtuI+c9biy0czjjW\n\t4f5AdFIp+lXkzUyJ/DpeBlI4iNtd3hgD4E4AbwP1ArNsb2TwfdogGBuIJWxCJyf3RY\n\tjIfCzwGD9jtV3FmseShIi/dhPWIvXokjxS8saiLg="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"YtQqX9lT\"; dkim-atps=neutral","Date":"Wed, 30 Aug 2023 09:56:59 +0200","To":"David Plowman ","Message-ID":"<7l3q76li7qbapltbskqxtyjhkcojw3d5b67lredsnehshffcih@p4mb7bq2wnb2>","References":"<20230823120915.18621-1-david.plowman@raspberrypi.com>\n\t<20230823120915.18621-4-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230823120915.18621-4-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v2 3/5] ipa: rpi: agc: Implementation\n\tof multi-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":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27751,"web_url":"https://patchwork.libcamera.org/comment/27751/","msgid":"","date":"2023-09-11T13:50:54","subject":"Re: [libcamera-devel] [PATCH v2 3/5] ipa: rpi: agc: Implementation\n\tof multi-channel AGC","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Jacopo\n\nThanks for the review!\n\nOn Wed, 30 Aug 2023 at 08:57, Jacopo Mondi\n wrote:\n>\n> Hi David\n>\n> On Wed, Aug 23, 2023 at 01:09:13PM +0100, David Plowman via libcamera-devel wrote:\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> > Reviewed-by: Naushir Patuck \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>\n> I'm missing in the code base where delayed_status.channel gets set :/\n\nIndeed, there's some background you need to know. The channel gets set\nin the process method in this file, when it calls setChannelIndex near\nthe end. Later, the \"agc.status\" metadata gets copied as the\n\"agc.delayed_status\" when the frame, for which \"agc.status\" calculated\nthe desired exposure a while back, actually arrives from the sensor.\nIt's \"here's what you asked for frames ago, and now this frame is\nwhat you got!\".\n\n>\n> > + else\n> > + /* This does happen at startup, otherwise it would be a Warning or Error. */\n> > + LOG(RPiAgc, Debug) << message;\n>\n> If this is a known condition, do you need to have the message\n> polluting the logs ?\n\nNot sure. It's not really meant to happen, so a warning would have\nbeen nice, on the other hand it _does_ happen while things are\nstarting up so a warning might be scary. Another option might be to\nhave an easy way to detect the \"startup\" case, then we could output a\nwarning conditionally. On the other hand, there has been some\ndiscussion about signalling those \"startup\" frames differently, so\nmaybe the whole thing will change... so I'm not too sure what is best\nat the moment!\n\n>\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> ditto\n>\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> on which channel would you do 'prepare' if not the last active one ?\n\nJust thinking out loud, really. It kind of feels to me like we don't\nreally need to touch anything in the AgcChannel object itself, so it\ncould probably be a static class method. Though that's not true of\nsome of the \"lock counts\", but I don't really like how those work\neither. Maybe I should remove such speculative thoughts until they're\na bit clearer?\n\n>\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>\n> why not use 'activeChannels_[index_]' again here ?\n\nactiveChannels_[index_] is the channel for which we want to generate\nnew exposure/gain values. As you can see, index_ just goes up by 1\nevery time (modulo active_channels_.size()).\n\nThe statsIndex needs to be the channel number of this frame that has\njust arrived, so it will be found in the agc.status that was written\nseveral frames ago, and which is now available as agc.delayed_status.\n\nPossibly \"statsIndex\" isn't the best name. Perhaps statsChannelIndex\nwould be better? It's not easy to know what to name things when there\nare indices and channel numbers, which I then manage to call channel\nindices. Perhaps I should be really strict about not using \"channel\nindex\" when I'm referring to a channel number, and indices only when\nI'm indexing into the activeChannels_ list? Except that channel\nnumbers are also indices into the list of channels. So I'm a bit\nconfused as well!\n\n>\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> Again I'm missing why we need to use a different channelIndex and\n> statsIndex..\n\nI hope I explained that above, but in the interests of trying harder...\n\nWhen we're running with two channels (0 and 1), we always alternate\nthe exposure/gain values that we request. These get put into the\nagc.status where the IPA will find them.\n\nThe important point is that we don't generate values for channel 0\nonly when a previous channel 0 frame arrives. That's because exposure\nupdates in the camera _can_ get lost if we're unlucky... in which case\nchannel 0 might disappear completely! So it's always channel 0,\nchannel 1, channel 0, channel 1...\n\nBut of course, this means that we might find ourselves generating\nvalues for channel 0 when the frame and statistics that have just\narrived are from channel 1. We have to record the channel 1\nstatistics, but use the most recent channel 0 statistics instead.\n\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>\n> Do you need to copy ? Could channelData.statistics get overwritten\n> during the process() call chain ?\n\nWe actually copy out all the statistics as soon as we get them (see\nIpaVc4::platformProcessStats in src/ipa/rpi/vc4/vc4.ccp), so I think\nthat should be safe. After that we use the copied version through a\nshared pointer so the memory should get cleared up once no one is\nholding onto it any more.\n\n>\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>\n> Uh, why is this not used anymore ? :)\n\nI think the reason is that it was never being used, but it wasn't\nquite so obvious until I started to look at this and worry what\nchannel \"agc.status\" might be referring to.\n\nThe current exposure values are only required to interpret the\nstatistics, so that we can work out how much to change the exposure by\nin order to cause a particular change in the statistics. And of course\nthe statistics depend only on totalExposureNoDG (the requested\nexposure excluding any digital gain). So the \"totalExposure\",\nincluding digital gain, simply isn't required. I've tried to make that\nmore obvious by simply setting that number to zero.\n\nThanks again for looking at this patch, I'm sorry that it can be a bit\ntricky to understand in places.\n\nDavid\n\n>\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 DF317BD160\n\tfor ;\n\tMon, 11 Sep 2023 13:51:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EDF0661DF0;\n\tMon, 11 Sep 2023 15:51:08 +0200 (CEST)","from mail-qt1-x82a.google.com (mail-qt1-x82a.google.com\n\t[IPv6:2607:f8b0:4864:20::82a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0D69F61DF0\n\tfor ;\n\tMon, 11 Sep 2023 15:51:07 +0200 (CEST)","by mail-qt1-x82a.google.com with SMTP id\n\td75a77b69052e-414d37bd1e9so33251831cf.3\n\tfor ;\n\tMon, 11 Sep 2023 06:51:06 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1694440268;\n\tbh=L5d37ALjv8RhiLiP/r3HL0EZozuXfDF7ajiQ9LjFmuw=;\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=SsGYEft4c2d4vbKDBmkGYitOAqYxKY3MR+9vjsHvjJVsCfly7wBWh+OdGryqFJNno\n\txb7mWKF+wrP2TlWoayTj54Z24oa1PXjQKJ0lh/3QS+D4YeLCUXumSt25i+z3J+u4Xd\n\t7ENImJlvSmmQ/Hu3jzmoice94wLcINCcIrOIXTWz/kZznPnKkCgsag9YtdpZ9dz0W5\n\t1VF4MAdliiHGrkjnAQDHweFHctn6DMriSh1hiy97Hw4QO41hpXk7y8DLOqaBvxqkjC\n\tRkv53C55eRVstd3wBlQxIb0Xr7TVy4VbXNW7jbGUTe2rKcL0tQkJhghjYS2ABqJtqg\n\tvSo+4Valmsa6A==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1694440266; x=1695045066;\n\tdarn=lists.libcamera.org; \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=45K2URCjZuwpkPRfGfyg5/6QJsi1ILKsAAc+9QReJXE=;\n\tb=Z7RRsCxDoqVmkDcAlMobmUfQ6cr8beagXNHbO9K9I1Ria0NSQ6OTiNLpA7oMed2HqA\n\t5zOZCEUTi9jqwjVugTHt7uBo8Y5d5gznZeUjYGI6GirVF7dbyfU4b3tAKvhKJ0p9O+i0\n\tg1yZi79E/GVLAtL0OLGLSlF2hvalj755O5riUmSntbdUrpu/lIQJqWbucGKwJ0KQgjGk\n\tv+77Jdnmh8UuEOwfopBU2SifbZkWhyKviQw71FrqbSrHBOK7JPAjjik9FuF/6AhXvrm6\n\tzvP0bjkW5qsG/UHpOUdPIjBhoZpwGPYvrQDLM71BnrTf6RidMISY745fz2MIEYYf9gU6\n\tNpNg=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"Z7RRsCxD\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1694440266; x=1695045066;\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=45K2URCjZuwpkPRfGfyg5/6QJsi1ILKsAAc+9QReJXE=;\n\tb=qRm8haFWkySTBwTncAXexQRCvIxYKhPd4WRcx2ZcLcAQL+VRqX9cKa/sfKssQK3Xtd\n\tm9VDXAMFtHHYtFJcY9bFb4US3QPWVzsdDhSwv/Xy2CSsPWoDjeX8FfIo4azMKPcSbs6W\n\t+YZ3+a4MxuS9szdyjyYq09vKElpF5Y0vadZkkSYByJyOWsagKs4JaN+r5hz09umPuRtt\n\twaX3Fl5kFG8eKHzkdbFmJoDTZ1cczRweckIFmKp2SBpByPevciNgUh0+ZL+XIvMMEzts\n\thoT5cuomMWFOM3AoI9Qt0s64EqDJWuraqfyRyctTBNLLY0QE6Ngr5z2ckyTgH+w2cilD\n\tWUWw==","X-Gm-Message-State":"AOJu0Yyg/x13yjLLWxxU+T8INqEjpZ6i8usEAUrzaauQUL7kECE8Bb8Z\n\td1foyRyKRRAS8R7ELE851sdRDswjurg+DXSuzHtXnmxmqhBuCyPm","X-Google-Smtp-Source":"AGHT+IHtp+9WI8wLJALKGkkEVjmkqjAvWU991JHWhO6RlMeMuCgev64sX+0Jw2aP6uv735zEFY4HQ9ZpO/NC3DmzfX8=","X-Received":"by 2002:ac8:7d15:0:b0:40f:f435:1af7 with SMTP id\n\tg21-20020ac87d15000000b0040ff4351af7mr11207063qtb.19.1694440265484;\n\tMon, 11 Sep 2023 06:51:05 -0700 (PDT)","MIME-Version":"1.0","References":"<20230823120915.18621-1-david.plowman@raspberrypi.com>\n\t<20230823120915.18621-4-david.plowman@raspberrypi.com>\n\t<7l3q76li7qbapltbskqxtyjhkcojw3d5b67lredsnehshffcih@p4mb7bq2wnb2>","In-Reply-To":"<7l3q76li7qbapltbskqxtyjhkcojw3d5b67lredsnehshffcih@p4mb7bq2wnb2>","Date":"Mon, 11 Sep 2023 14:50:54 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 3/5] ipa: rpi: agc: Implementation\n\tof multi-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":"David Plowman via libcamera-devel ","Reply-To":"David Plowman ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]