[{"id":36748,"web_url":"https://patchwork.libcamera.org/comment/36748/","msgid":"","date":"2025-11-07T10:18:54","subject":"Re: [PATCH v1 19/35] libipa: algorithm: Update docs","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Stefan\n\nOn Fri, Oct 24, 2025 at 10:50:43AM +0200, Stefan Klug wrote:\n> Update the algorithm documentation to reflect the changed timing model.\n>\n> Signed-off-by: Stefan Klug \n> ---\n> src/ipa/libipa/algorithm.cpp | 28 ++++++++++++++--------------\n> 1 file changed, 14 insertions(+), 14 deletions(-)\n>\n> diff --git a/src/ipa/libipa/algorithm.cpp b/src/ipa/libipa/algorithm.cpp\n> index 201efdfdba25..44b6bdfa6972 100644\n> --- a/src/ipa/libipa/algorithm.cpp\n> +++ b/src/ipa/libipa/algorithm.cpp\n> @@ -76,11 +76,12 @@ namespace ipa {\n> *\n> * This function is called for each request queued to the camera. It provides\n> * the controls stored in the request to the algorithm. The \\a frame number\n> - * is the Request sequence number and identifies the desired corresponding\n> + * is the sensor sequence number and identifies the desired corresponding\n\nI wonder if the frame/request sequence number is even meaningful for\nalgorithsm. After all, this function should mostly pass to the\nalgorithms the control to use to change the algorithm's state.\n\nThe only usage I see of frame in algos' queueRequest is to find out if\nthis is the first call (frame == 0) and the ISP block needs to be\nfully configured or not (something which might be handled with a\nflag set/reset at configure() time ?)\n\n> * frame to target for the controls to take effect.\n> *\n> * Algorithms shall read the applicable controls and store their value for later\n> - * use during frame processing.\n> + * use during frame processing. All values that are already known shall be\n> + * updated in \\a frameContext.\n\nI'm not sure I get what you mean.\n\nIs this to suggest that the IPA that has to accummulate controls which\nhave arrived too late ?\n\n> */\n>\n> /**\n> @@ -98,14 +99,18 @@ namespace ipa {\n> * Algorithms shall fill in the parameter structure fields appropriately to\n> * configure the ISP processing blocks that they are responsible for. This\n> * includes setting fields and flags that enable those processing blocks.\n> + *\n> + * Additionally \\a frameContext shall be updated with the most up to date values\n> + * from active state.\n\nI think this needs to be expanded a little.\n\nI interpret this as the frame context shall be kept up-to-date with\nthe values that are effectivelly set in the parameters buffer, right ?\n\nIs the idea to move the values from the active state where they have\nbeen populated by process() to the frame context at preprare() time,\nso that the frame context is only updated when the parameters buffer is\nprogrammed ?\n\nI would keep this note as part of the previous paragraph, and take the\noccasion to modify it slightly.\n\n * Algorithms shall fill in the \\a params buffer appropriately to\n * configure the ISP processing blocks that they are responsible for.\n * This includes setting fields and flags that enable those processing\n * blocks. The values used to populate \\a params should be used to\n * update \\a frameContext so that it reflects the ISP configuration\n * used to produce \\a frame.\n */\n\nTake in whatever you think is approprate from this\n\n\n> */\n>\n> /**\n> * \\fn Algorithm::process()\n> * \\brief Process ISP statistics, and run algorithm operations\n> * \\param[in] context The shared IPA context\n> - * \\param[in] frame The frame context sequence number\n> - * \\param[in] frameContext The current frame's context\n> + * \\param[in] frame The frame sequence number that produces the stats\n\nfor which stats have been produced\n\n?\n\n> + * \\param[in] frameContext The frame context for the frame that produced the\n> + * stats\n\nsame here\n\n> * \\param[in] stats The IPA statistics and ISP results\n> * \\param[out] metadata Metadata for the frame, to be filled by the algorithm\n> *\n> @@ -118,19 +123,14 @@ namespace ipa {\n> * computationally expensive calculations or operations must be handled\n> * asynchronously in a separate thread.\n> *\n> - * Algorithms can store state in their respective IPAFrameContext structures,\n> - * and reference state from the IPAFrameContext of other algorithms.\n> - *\n> - * \\todo Historical data may be required as part of the processing.\n> - * Either the previous frame, or the IPAFrameContext state of the frame\n> - * that generated the statistics for this operation may be required for\n> - * some advanced algorithms to prevent oscillations or support control\n> - * loops correctly. Only a single IPAFrameContext is available currently,\n> - * and so any data stored may represent the results of the previously\n> - * completed operations.\n> + * Algorithms shall update the active state. The frameContext shall *not* be\n> + * updated as that frame was already produced.\n\nI would rather modify the first paragraph otherwise it feels like this\nhas been added here to tell us not to do what we have been doing so\nfar.\n\nThe end result might be:\n\n * This function is called while camera is running for every frame\n * processed by the ISP, to process statistics generated from that\n * frame. Algorithms shall use this data to run calculations, update\n * the active state and fill the frame metadata with the results of\n * the calculations.\n\n> *\n> * Care shall be taken to ensure the ordering of access to the information\n> * such that the algorithms use up to date state as required.\n> + *\n> + * The \\a stats parameter can be null in which case ony the frame metadata shall\n\ns/ony/only\ns/frame metadata/ \\a metadata /\n\n> + * be filled.\n\ns/filled/populated ?\n\n> */\n>\n> /**\n> --\n> 2.48.1\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 B5ED4BDE4C\n\tfor ;\n\tFri, 7 Nov 2025 10:19:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DA0DE609D8;\n\tFri, 7 Nov 2025 11:18:59 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 71290606D5\n\tfor ;\n\tFri, 7 Nov 2025 11:18:58 +0100 (CET)","from ideasonboard.com (mob-5-90-142-135.net.vodafone.it\n\t[5.90.142.135])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8D7D41189;\n\tFri, 7 Nov 2025 11:17:02 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ObnnixJY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1762510622;\n\tbh=GrPibvSnEVDc5172Cjb2P0pxs0rvLGOU+F+3zVHqESc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ObnnixJYCA2qO1AgwmEeuPnUGuI8XqXsb0Juo7CdqJp4Rhz4b6xh3j7JZEvLadZ1l\n\tfltyv0zrXGh5YjE7SXxRpASvS0HNDnrWD4jq8zwKQkgEGVZsa0qRV1mcScNNW3stgi\n\tqW6ylnjzN/MQd+gtC8ChQMUNE9kMIdCz0hwQbXBQ=","Date":"Fri, 7 Nov 2025 11:18:54 +0100","From":"Jacopo Mondi ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1 19/35] libipa: algorithm: Update docs","Message-ID":"","References":"<20251024085130.995967-1-stefan.klug@ideasonboard.com>\n\t<20251024085130.995967-20-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20251024085130.995967-20-stefan.klug@ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":36919,"web_url":"https://patchwork.libcamera.org/comment/36919/","msgid":"<176356366522.1127434.10953397519427047811@ping.linuxembedded.co.uk>","date":"2025-11-19T14:47:45","subject":"Re: [PATCH v1 19/35] libipa: algorithm: Update docs","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi (2025-11-07 10:18:54)\n> Hi Stefan\n> \n> On Fri, Oct 24, 2025 at 10:50:43AM +0200, Stefan Klug wrote:\n> > Update the algorithm documentation to reflect the changed timing model.\n> >\n> > Signed-off-by: Stefan Klug \n> > ---\n> > src/ipa/libipa/algorithm.cpp | 28 ++++++++++++++--------------\n> > 1 file changed, 14 insertions(+), 14 deletions(-)\n> >\n> > diff --git a/src/ipa/libipa/algorithm.cpp b/src/ipa/libipa/algorithm.cpp\n> > index 201efdfdba25..44b6bdfa6972 100644\n> > --- a/src/ipa/libipa/algorithm.cpp\n> > +++ b/src/ipa/libipa/algorithm.cpp\n> > @@ -76,11 +76,12 @@ namespace ipa {\n\n\nSending out my inline comments and discussions notes while talking\nthrough with Stefan which has helped me comprehend all the changes going\non in libipa:\n\n\nContext location:\n queueRequest()\n\n> > *\n> > * This function is called for each request queued to the camera. It provides\n> > * the controls stored in the request to the algorithm. The \\a frame number\n> > - * is the Request sequence number and identifies the desired corresponding\n> > + * is the sensor sequence number and identifies the desired corresponding\n> \n> I wonder if the frame/request sequence number is even meaningful for\n> algorithsm. After all, this function should mostly pass to the\n> algorithms the control to use to change the algorithm's state.\n> \n> The only usage I see of frame in algos' queueRequest is to find out if\n> this is the first call (frame == 0) and the ISP block needs to be\n> fully configured or not (something which might be handled with a\n> flag set/reset at configure() time ?)\n\n\nTalking through this with Stefan, it sounds like we might make more\nupdates here.\n\nThis is really:\n\n\"Initialise frame context for a given sensor sequence number\"\n\nThis changes the mechanics, because if we're freely running with no\nexternal requests, then initialise frame context can still be called for\nevery frame, (either actively by queueRequest, or passively by\ncomputeParams/Algorithm::prepare if it has not yet been initialised).\n\nThis way the IPA can run even when we may have underruns from the\napplication.\n\n\n> > * frame to target for the controls to take effect.\n> > *\n> > * Algorithms shall read the applicable controls and store their value for later\n> > - * use during frame processing.\n> > + * use during frame processing. All values that are already known shall be\n> > + * updated in \\a frameContext.\n> \n> I'm not sure I get what you mean.\n> \n> Is this to suggest that the IPA that has to accummulate controls which\n> have arrived too late ?\n\nStefan: \"If we're in manual exposure mode, we know what exposure (or\nother manual controls) are to be set so we can set them already\".\n\nWith that context:\n\nSo we can already decide if we know values for this frame. Anything\nmanually set in active state manual controls should be applied here.\n\nSo how can we word this more explicitly?\n\n\"\"\"\nAlgorithms shall read the applicable controls and store their value for\nlater use during frame processing. Incoming controls are merged with the\ncurrent known state to update the \\a frameContext.\n\"\"\"\n\n> \n> > */\n> >\n> > /**\n> > @@ -98,14 +99,18 @@ namespace ipa {\n\nContext: prepare()\n\n> > * Algorithms shall fill in the parameter structure fields appropriately to\n> > * configure the ISP processing blocks that they are responsible for. This\n> > * includes setting fields and flags that enable those processing blocks.\n> > + *\n> > + * Additionally \\a frameContext shall be updated with the most up to date values\n> > + * from active state.\n> \n> I think this needs to be expanded a little.\n> \n> I interpret this as the frame context shall be kept up-to-date with\n> the values that are effectivelly set in the parameters buffer, right ?\n> \n> Is the idea to move the values from the active state where they have\n> been populated by process() to the frame context at preprare() time,\n> so that the frame context is only updated when the parameters buffer is\n> programmed ?\n\nYes ? (Is my interpretation).\n\n\n> I would keep this note as part of the previous paragraph, and take the\n> occasion to modify it slightly.\n> \n> * Algorithms shall fill in the \\a params buffer appropriately to\n> * configure the ISP processing blocks that they are responsible for.\n> * This includes setting fields and flags that enable those processing\n> * blocks. The values used to populate \\a params should be used to\n> * update \\a frameContext so that it reflects the ISP configuration\n> * used to produce \\a frame.\n> */\n> \n> Take in whatever you think is approprate from this\n\nThat sounds reasonable to me ... my only hesitation is that from my\ncurrent understanding there will still be a split between which data we\nuse or rather report as 'the metadata that was used to generate this\nframe':\n - Lux, ColourTemperature, ...\n\nvs the metadata that 'is represented in this frame':\n - Exposure, Gain, ...\n\nMaybe somehow we have to find a way to categorise or group these two\ntypes of metadata... Especially as those early types can have two\nvalues!\n\n\nWe can output both of:\n - lux\n - The lux used by algorithms (from active state) to decide what to\n do to generate this frame (which is what we will report with\n https://patchwork.libcamera.org/project/libcamera/list/?series=5595)\n - The lux level *of* this frame. Which is what 'I used to think'\n this represents.\n\n - ColourTemperature:\n - The ColourTemperature (from active state, or a manual control) to\n decide what gains to generate in Awb.\n - The colour temperature of this image.\n\n\nI have no idea how we can convey these two numbers of the same metadata.\n\n\n> > */\n> >\n> > /**\n> > * \\fn Algorithm::process()\n> > * \\brief Process ISP statistics, and run algorithm operations\n> > * \\param[in] context The shared IPA context\n> > - * \\param[in] frame The frame context sequence number\n> > - * \\param[in] frameContext The current frame's context\n> > + * \\param[in] frame The frame sequence number that produces the stats\n> \n> for which stats have been produced\n> \n> ?\n> \n> > + * \\param[in] frameContext The frame context for the frame that produced the\n> > + * stats\n> \n> same here\n> \n> > * \\param[in] stats The IPA statistics and ISP results\n> > * \\param[out] metadata Metadata for the frame, to be filled by the algorithm\n> > *\n> > @@ -118,19 +123,14 @@ namespace ipa {\n\ncontext: Still in process():\n\n> > * computationally expensive calculations or operations must be handled\n> > * asynchronously in a separate thread.\n> > *\n> > - * Algorithms can store state in their respective IPAFrameContext structures,\n> > - * and reference state from the IPAFrameContext of other algorithms.\n> > - *\n> > - * \\todo Historical data may be required as part of the processing.\n> > - * Either the previous frame, or the IPAFrameContext state of the frame\n> > - * that generated the statistics for this operation may be required for\n> > - * some advanced algorithms to prevent oscillations or support control\n> > - * loops correctly. Only a single IPAFrameContext is available currently,\n> > - * and so any data stored may represent the results of the previously\n> > - * completed operations.\n> > + * Algorithms shall update the active state. The frameContext shall *not* be\n> > + * updated as that frame was already produced.\n> \n> I would rather modify the first paragraph otherwise it feels like this\n> has been added here to tell us not to do what we have been doing so\n> far.\n> \n> The end result might be:\n> \n> * This function is called while camera is running for every frame\n\nwhile the camera\n\n> * processed by the ISP, to process statistics generated from that\n> * frame. Algorithms shall use this data to run calculations, update\n> * the active state and fill the frame metadata with the results of\n> * the calculations.\n\nFrame metadata might be updated with the values from the framecontext\n... not the most recent calculations.\n\nBut I still think that's dependent/specific to different variables (past\nvariables, that report what's used for this frame, vs present state variables\nwhich are what are measured from the frame).\n\n\n> \n> > *\n> > * Care shall be taken to ensure the ordering of access to the information\n> > * such that the algorithms use up to date state as required.\n> > + *\n> > + * The \\a stats parameter can be null in which case ony the frame metadata shall\n> \n> s/ony/only\n> s/frame metadata/ \\a metadata /\n> \n> > + * be filled.\n> \n> s/filled/populated ?\n> \n> > */\n> >\n> > /**\n> > --\n> > 2.48.1\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 ABD6DC0F1B\n\tfor ;\n\tWed, 19 Nov 2025 14:47:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 156EE60A80;\n\tWed, 19 Nov 2025 15:47:49 +0100 (CET)","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 F0BBE606A0\n\tfor ;\n\tWed, 19 Nov 2025 15:47:47 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 59F37DD9;\n\tWed, 19 Nov 2025 15:45:43 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Lz62E3eO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1763563543;\n\tbh=uJ143CCtUJkdOTtZUCkdgcCCeCH5nHrlRPjaaqMSnik=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=Lz62E3eOQOtGIhm0h20Bc3PXJyUF+/LxP0fxz5ZJLNF0x2dTp5EeyizRhw/DpHcIJ\n\tD4mSSSeUuSqq5Gt7zcYAePll12eatVZBLjEr7yDAGoJawJsHz52EZO93qC7fS9OscY\n\tOrL7MwP06pJ0VZdQ3HSaLlLghjDRLlUXJZtqOuG4=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20251024085130.995967-1-stefan.klug@ideasonboard.com>\n\t<20251024085130.995967-20-stefan.klug@ideasonboard.com>\n\t","Subject":"Re: [PATCH v1 19/35] libipa: algorithm: Update docs","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","To":"Jacopo Mondi ,\n\tStefan Klug ","Date":"Wed, 19 Nov 2025 14:47:45 +0000","Message-ID":"<176356366522.1127434.10953397519427047811@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":36952,"web_url":"https://patchwork.libcamera.org/comment/36952/","msgid":"","date":"2025-11-20T16:41:12","subject":"Re: [PATCH v1 19/35] libipa: algorithm: Update docs","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Kieran\nOn Wed, Nov 19, 2025 at 02:47:45PM +0000, Kieran Bingham wrote:\n> Quoting Jacopo Mondi (2025-11-07 10:18:54)\n> > Hi Stefan\n> >\n> > On Fri, Oct 24, 2025 at 10:50:43AM +0200, Stefan Klug wrote:\n> > > Update the algorithm documentation to reflect the changed timing model.\n> > >\n> > > Signed-off-by: Stefan Klug \n> > > ---\n> > > src/ipa/libipa/algorithm.cpp | 28 ++++++++++++++--------------\n> > > 1 file changed, 14 insertions(+), 14 deletions(-)\n> > >\n> > > diff --git a/src/ipa/libipa/algorithm.cpp b/src/ipa/libipa/algorithm.cpp\n> > > index 201efdfdba25..44b6bdfa6972 100644\n> > > --- a/src/ipa/libipa/algorithm.cpp\n> > > +++ b/src/ipa/libipa/algorithm.cpp\n> > > @@ -76,11 +76,12 @@ namespace ipa {\n>\n>\n> Sending out my inline comments and discussions notes while talking\n> through with Stefan which has helped me comprehend all the changes going\n> on in libipa:\n>\n>\n> Context location:\n> queueRequest()\n>\n> > > *\n> > > * This function is called for each request queued to the camera. It provides\n> > > * the controls stored in the request to the algorithm. The \\a frame number\n> > > - * is the Request sequence number and identifies the desired corresponding\n> > > + * is the sensor sequence number and identifies the desired corresponding\n> >\n> > I wonder if the frame/request sequence number is even meaningful for\n> > algorithsm. After all, this function should mostly pass to the\n> > algorithms the control to use to change the algorithm's state.\n> >\n> > The only usage I see of frame in algos' queueRequest is to find out if\n> > this is the first call (frame == 0) and the ISP block needs to be\n> > fully configured or not (something which might be handled with a\n> > flag set/reset at configure() time ?)\n>\n>\n> Talking through this with Stefan, it sounds like we might make more\n> updates here.\n>\n> This is really:\n>\n> \"Initialise frame context for a given sensor sequence number\"\n\nsee, this is where I got disconnected..\n\nqueueRequest() is called in response to an application action and it\nit is associated with a request sequence.\n\nI understood Stefan did a a lot of work in the pipeline to associate\nto a Request the frame sequence it is tied to, but I wonder if\nthis means all pipelines should go through that complicated mechanism.\n\n>\n> This changes the mechanics, because if we're freely running with no\n> external requests, then initialise frame context can still be called for\n> every frame, (either actively by queueRequest, or passively by\n> computeParams/Algorithm::prepare if it has not yet been initialised).\n\nI agree we shall have a frame context for every sensor frame. Possibly\nthe whole IPA::queueRequest() thing is wrong and what we should really\nhave is an IPA::frameStarted().\n\nThe pipeline could keep a queue of pending request, and at every frame\nstart consume the less recent one and associate it with the frame\nsequence that just started and then call IPA::frameStarted() with the\ncontrols associated to the just-consumed request.\n\nTake this as just a discussion starter, because without a proper\ndesign I'm basically just thinking out loud.\n\n>\n> This way the IPA can run even when we may have underruns from the\n> application.\n>\n>\n> > > * frame to target for the controls to take effect.\n> > > *\n> > > * Algorithms shall read the applicable controls and store their value for later\n> > > - * use during frame processing.\n> > > + * use during frame processing. All values that are already known shall be\n> > > + * updated in \\a frameContext.\n> >\n> > I'm not sure I get what you mean.\n> >\n> > Is this to suggest that the IPA that has to accummulate controls which\n> > have arrived too late ?\n>\n> Stefan: \"If we're in manual exposure mode, we know what exposure (or\n> other manual controls) are to be set so we can set them already\".\n>\n> With that context:\n>\n> So we can already decide if we know values for this frame. Anything\n> manually set in active state manual controls should be applied here.\n>\n> So how can we word this more explicitly?\n>\n> \"\"\"\n> Algorithms shall read the applicable controls and store their value for\n> later use during frame processing. Incoming controls are merged with the\n> current known state to update the \\a frameContext.\n> \"\"\"\n\nAck, thanks for clarifying this\n\n\n>\n> >\n> > > */\n> > >\n> > > /**\n> > > @@ -98,14 +99,18 @@ namespace ipa {\n>\n> Context: prepare()\n>\n> > > * Algorithms shall fill in the parameter structure fields appropriately to\n> > > * configure the ISP processing blocks that they are responsible for. This\n> > > * includes setting fields and flags that enable those processing blocks.\n> > > + *\n> > > + * Additionally \\a frameContext shall be updated with the most up to date values\n> > > + * from active state.\n> >\n> > I think this needs to be expanded a little.\n> >\n> > I interpret this as the frame context shall be kept up-to-date with\n> > the values that are effectivelly set in the parameters buffer, right ?\n> >\n> > Is the idea to move the values from the active state where they have\n> > been populated by process() to the frame context at preprare() time,\n> > so that the frame context is only updated when the parameters buffer is\n> > programmed ?\n>\n> Yes ? (Is my interpretation).\n>\n\nMaybe we need to expand this a bit in a general document about the\nwhole IPA state machine. Or do you think what we have here is enough ?\n\n>\n> > I would keep this note as part of the previous paragraph, and take the\n> > occasion to modify it slightly.\n> >\n> > * Algorithms shall fill in the \\a params buffer appropriately to\n> > * configure the ISP processing blocks that they are responsible for.\n> > * This includes setting fields and flags that enable those processing\n> > * blocks. The values used to populate \\a params should be used to\n> > * update \\a frameContext so that it reflects the ISP configuration\n> > * used to produce \\a frame.\n> > */\n> >\n> > Take in whatever you think is approprate from this\n>\n> That sounds reasonable to me ... my only hesitation is that from my\n> current understanding there will still be a split between which data we\n> use or rather report as 'the metadata that was used to generate this\n> frame':\n> - Lux, ColourTemperature, ...\n>\n> vs the metadata that 'is represented in this frame':\n> - Exposure, Gain, ...\n>\n> Maybe somehow we have to find a way to categorise or group these two\n> types of metadata... Especially as those early types can have two\n> values!\n>\n>\n> We can output both of:\n> - lux\n> - The lux used by algorithms (from active state) to decide what to\n> do to generate this frame (which is what we will report with\n> https://patchwork.libcamera.org/project/libcamera/list/?series=5595)\n> - The lux level *of* this frame. Which is what 'I used to think'\n> this represents.\n>\n> - ColourTemperature:\n> - The ColourTemperature (from active state, or a manual control) to\n> decide what gains to generate in Awb.\n> - The colour temperature of this image.\n>\n>\n> I have no idea how we can convey these two numbers of the same metadata.\n>\n\nI'm sorry but isn't the \"lux used by algorithms\" the same as the \"lux\nof this image\" ?\n\nOr is the \"lux used by algorithm\" being estimated on older images ?\n\n>\n> > > */\n> > >\n> > > /**\n> > > * \\fn Algorithm::process()\n> > > * \\brief Process ISP statistics, and run algorithm operations\n> > > * \\param[in] context The shared IPA context\n> > > - * \\param[in] frame The frame context sequence number\n> > > - * \\param[in] frameContext The current frame's context\n> > > + * \\param[in] frame The frame sequence number that produces the stats\n> >\n> > for which stats have been produced\n> >\n> > ?\n> >\n> > > + * \\param[in] frameContext The frame context for the frame that produced the\n> > > + * stats\n> >\n> > same here\n> >\n> > > * \\param[in] stats The IPA statistics and ISP results\n> > > * \\param[out] metadata Metadata for the frame, to be filled by the algorithm\n> > > *\n> > > @@ -118,19 +123,14 @@ namespace ipa {\n>\n> context: Still in process():\n>\n> > > * computationally expensive calculations or operations must be handled\n> > > * asynchronously in a separate thread.\n> > > *\n> > > - * Algorithms can store state in their respective IPAFrameContext structures,\n> > > - * and reference state from the IPAFrameContext of other algorithms.\n> > > - *\n> > > - * \\todo Historical data may be required as part of the processing.\n> > > - * Either the previous frame, or the IPAFrameContext state of the frame\n> > > - * that generated the statistics for this operation may be required for\n> > > - * some advanced algorithms to prevent oscillations or support control\n> > > - * loops correctly. Only a single IPAFrameContext is available currently,\n> > > - * and so any data stored may represent the results of the previously\n> > > - * completed operations.\n> > > + * Algorithms shall update the active state. The frameContext shall *not* be\n> > > + * updated as that frame was already produced.\n> >\n> > I would rather modify the first paragraph otherwise it feels like this\n> > has been added here to tell us not to do what we have been doing so\n> > far.\n> >\n> > The end result might be:\n> >\n> > * This function is called while camera is running for every frame\n>\n> while the camera\n>\n> > * processed by the ISP, to process statistics generated from that\n> > * frame. Algorithms shall use this data to run calculations, update\n> > * the active state and fill the frame metadata with the results of\n> > * the calculations.\n>\n> Frame metadata might be updated with the values from the framecontext\n> ... not the most recent calculations.\n>\n> But I still think that's dependent/specific to different variables (past\n> variables, that report what's used for this frame, vs present state variables\n> which are what are measured from the frame).\n>\n>\n> >\n> > > *\n> > > * Care shall be taken to ensure the ordering of access to the information\n> > > * such that the algorithms use up to date state as required.\n> > > + *\n> > > + * The \\a stats parameter can be null in which case ony the frame metadata shall\n> >\n> > s/ony/only\n> > s/frame metadata/ \\a metadata /\n> >\n> > > + * be filled.\n> >\n> > s/filled/populated ?\n> >\n> > > */\n> > >\n> > > /**\n> > > --\n> > > 2.48.1\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 A7E3EC3333\n\tfor ;\n\tThu, 20 Nov 2025 16:41:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0EA6760A85;\n\tThu, 20 Nov 2025 17:41:17 +0100 (CET)","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 EC13B609D8\n\tfor ;\n\tThu, 20 Nov 2025 17:41:15 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4F62CB5;\n\tThu, 20 Nov 2025 17:39:10 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"LzrcAqud\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1763656750;\n\tbh=RX+Y1koCJ0+IZXc3sRLjDdcgYnbFJN6TDuuqgzrsubk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LzrcAqudQigVChKp5eUx+A9hFOufJjhm87F/b0zFJD0UUm6wqlikWfnS/hpNpWb4+\n\tWwm6n7lhWoxbb7KxPUsmBEv8HXb2v6hzemBpS0eWpMPl5/290Q9a/A8ifwAYcZGKHV\n\t3j0u5nM+lfzKDVSGTKZsN3qMuD+bh6yyBLpl8TKI=","Date":"Thu, 20 Nov 2025 17:41:12 +0100","From":"Jacopo Mondi ","To":"Kieran Bingham ","Cc":"Jacopo Mondi , \n\tStefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1 19/35] libipa: algorithm: Update docs","Message-ID":"","References":"<20251024085130.995967-1-stefan.klug@ideasonboard.com>\n\t<20251024085130.995967-20-stefan.klug@ideasonboard.com>\n\t\n\t<176356366522.1127434.10953397519427047811@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<176356366522.1127434.10953397519427047811@ping.linuxembedded.co.uk>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]