[{"id":24357,"web_url":"https://patchwork.libcamera.org/comment/24357/","msgid":"<20220803150410.GO311202@pyrite.rasen.tech>","date":"2022-08-03T15:04:10","subject":"Re: [libcamera-devel] [PATCH 4/9] fixup: Expand AeState\n\tdocumentation","submitter":{"id":97,"url":"https://patchwork.libcamera.org/api/people/97/","name":"Nicolas Dufresne via libcamera-devel","email":"libcamera-devel@lists.libcamera.org"},"content":"Hi Jacopo,\n\nOn Fri, Jul 01, 2022 at 05:46:56PM +0200, Jacopo Mondi wrote:\n> The AeState control is tricky, it describes the state of the whole AEGC\n> block which is independently controlled by ExposureTimeMode and\n> AnalogueGainMode.\n> \n> Try to expand the documentation.\n> \n> Use \"AEGC\" when referring to the algorithm.\n> \n> Maybe the control should be named AEGCState ?\n\nIt's uuuugly.\n\nBut also maybe more correct. idk I'm neutral maybe people that have a\nbigger preference can do the bikeshedding :)\n\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/control_ids.yaml | 64 ++++++++++++++++++++++++----------\n> 1 file changed, 46 insertions(+), 18 deletions(-)\n> \n> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> index bb5eeb1507a9..d50df8bcad28 100644\n> --- a/src/libcamera/control_ids.yaml\n> +++ b/src/libcamera/control_ids.yaml\n> @@ -10,43 +10,71 @@ controls:\n> - AeState:\n> type: int32_t\n> description: |\n> - Control to report the AE algorithm state associated with the capture\n> - result.\n> + Control to report the AEGC algorithm state.\n> \n> - The state is still reported even if ExposureTimeMode or\n> - AnalogueGainMode is set to Manual.\n> + The AEGC algorithm computes the exposure time and the analogue gain\n> + values to be applied to the image sensor.\n> +\n> + The AEGC algorithm behaviour is controlled by the ExposureTimeMode and\n> + AnalogueGainMode controls, which allow applications to decide how\n> + the exposure time and gain are computed, in Auto or Manual mode,\n> + independently one from the other.\n\ns/one from the other/from one another/\n\n> +\n> + The AeState control reports the AEGC algorithm state through a single\n> + value and describes it a single computation block which computes\n\ns/it //\n\n> + both the exposure time and the analogue gain values.\n> +\n> + When both the exposure time and analogue gain values are configured to\n> + be in Manual mode, the AEGC algorithm is quiescent and does not actively\n> + compute any value and the AeState control will report AeStateIdle.\n> +\n> + When at least the exposure time or analogue gain are configured to be\n> + computed by the AEGC algorithm, the AeState control will report if the\n> + algorithm has converged to stable values for any of the controls set\n\nI would've s/any/all/, but now I'm wondering if it's even possible for\nAEGC with both E and G set to auto to have only one converged but not\nthe other.\n\nAlthough maybe s/any/all/ is proper, because this is *one* state to\ndescribe *all* of AEGC, so I think if at least one is still converging\nit should be converging, and only if both are converged then it should\nbe converged. Even if it's not really possible, that's probably the more\ncorrect description.\n\n> + to be computed in Auto mode.\n> \n> - \\sa AnalogueGain\n> \\sa AnalogueGainMode\n> - \\sa ExposureTime\n> \\sa ExposureTimeMode\n> \n> enum:\n> - name: AeStateIdle\n> value: 0\n> description: |\n> - The AE algorithm is inactive.\n> + The AEGC algorithm is inactive.\n> +\n> + This state is returned when both AnalogueGainMode and\n> + ExposureTimeMode are set to Manual and the algorithm is not\n> + actively computing any value.\n\nDo we need to explicitly state an exception for if only one is\nmanual-able? Although due to the constraints that we've defined, if\nonly one is manual-able then both are auto-able. So maybe it's implicit?\n\n> \n> - This state should be returned if both AnalogueGainMode and\n> - ExposureTimeMode are set to manual (or one, if the camera only\n> - supports one of the two controls).\n> + When ExposureTimeMode or AnalogueGainMode are set to Auto mode, the\n> + AEGC algorithm might spontaneously initiate a new scan, in which\n> + case the AeState control is moved to AeStateSearching.\n> - name: AeStateSearching\n> value: 1\n> description: |\n> - The AE algorithm has not converged yet.\n> + The AEGC algorithm is actively computing new values, for either the\n> + exposure time or the analogue gain, but has not converged to a\n> + stable result yet.\n> +\n> + This state is returned if at least one of AnalogueGainMode\n> + or ExposureTimeMode is set to auto and the algorithm hasn't\n> + converged yet.\n> \n> - This state should be returned if at least one of AnalogueGainMode\n> - or ExposureTimeMode is set to auto, and the AE algorithm hasn't\n> - converged yet. If the AE algorithm converges, the state shall go to\n> - AeStateConverged.\n> + The AEGC algorithm converges once stable values are computed for\n> + any of the controls set to be computed in Auto mode.\n> +\n> + Once the algorithm converges the state is moved to AeStateConverged.\n> - name: AeStateConverged\n> value: 2\n> description: |\n> The AE algorithm has converged.\n\ns/AE/AEGC/?\n\n\nOtherwise looks good.\n\n\nThanks,\n\nPaul\n\n\n> \n> - This state should be returned if at least one of AnalogueGainMode\n> - or ExposureTimeMode is set to auto, and the AE algorithm has\n> - converged.\n> + This state is returned if at least one of AnalogueGainMode\n> + or ExposureTimeMode is set to Auto, and the AEGC algorithm has\n> + converged to stable value.\n> +\n> + The AEGC algorithm might spontaneously re-initiate an AE scan, in\n> + which case the state is moved to AeStateSearching.\n> \n> # AeMeteringMode needs further attention:\n> # - Auto-generate max enum value.\n> -- \n> 2.36.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 88775C3272\n\tfor ;\n\tWed, 3 Aug 2022 15:04:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CE9A863310;\n\tWed, 3 Aug 2022 17:04:19 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 63076603E6\n\tfor ;\n\tWed, 3 Aug 2022 17:04:18 +0200 (CEST)","from pyrite.rasen.tech (h175-177-042-159.catv02.itscom.jp\n\t[175.177.42.159])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 149628B;\n\tWed, 3 Aug 2022 17:04:16 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659539059;\n\tbh=1s59lF+FvuViIKdGa9ggFIy1jblwGfR2egvjJ5mFHaE=;\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=kMEJ9+yVIBPqbQNdl7YHik9FZb6lVzUi89vcQubXaUxKgFsl2ucM6gNGRAp+nRl37\n\tK3Otxi9/J2KNGPsFAAwytwXW8oHX3lTiVw0Js0O4exj5sW/raMSs6naKi95nuEYyhq\n\tkKjXLMDhK/czKku3+zp9376zSlqS2dWeqHF5+mY8b2aP+h21EJQe9IK5MNnSINQl6G\n\tn43iryqPOeBff5z+04Y1N+4zuz7WbVmlFldpbrP/GcLRZy8WDu+9GKu0p4WoA7nGiZ\n\tkJ5oQhEqpclJIiSBCmEiEjn0965flYszK3SlJ/urak0YAolcDaSZE25awE6H6vNlkJ\n\t6uV39plFFORuQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1659539058;\n\tbh=1s59lF+FvuViIKdGa9ggFIy1jblwGfR2egvjJ5mFHaE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=NAPlWToXRw+SGQ/WH0G8fNqLTpBfsjcSFNRMGyzeRLzW5A/N4OUqc+63wYYYgJ6g5\n\tRiHh96ZiPNS5zMWmU9UDWTDIUvsKS9oo+wkCMlKB8PL0p6Dc5PGIfFBojfw2ZxX0Bz\n\tYz5O/s6fFCyKNxdisCXLo0FQ7vjAryXuaeLpkfcA="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"NAPlWToX\"; dkim-atps=neutral","Date":"Thu, 4 Aug 2022 00:04:10 +0900","To":"Jacopo Mondi ","Message-ID":"<20220803150410.GO311202@pyrite.rasen.tech>","References":"<20220518134728.777709-2-paul.elder@ideasonboard.com>\n\t<20220701154701.354052-1-jacopo@jmondi.org>\n\t<20220701154701.354052-4-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20220701154701.354052-4-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 4/9] fixup: Expand AeState\n\tdocumentation","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":"Paul Elder via libcamera-devel ","Reply-To":"paul.elder@ideasonboard.com","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24541,"web_url":"https://patchwork.libcamera.org/comment/24541/","msgid":"<20220811073923.3lqfxgpz6gxhbhte@uno.localdomain>","date":"2022-08-11T07:39:23","subject":"Re: [libcamera-devel] [PATCH 4/9] fixup: Expand AeState\n\tdocumentation","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul\n\nOn Thu, Aug 04, 2022 at 12:04:10AM +0900, paul.elder@ideasonboard.com wrote:\n> Hi Jacopo,\n>\n> On Fri, Jul 01, 2022 at 05:46:56PM +0200, Jacopo Mondi wrote:\n> > The AeState control is tricky, it describes the state of the whole AEGC\n> > block which is independently controlled by ExposureTimeMode and\n> > AnalogueGainMode.\n> >\n> > Try to expand the documentation.\n> >\n> > Use \"AEGC\" when referring to the algorithm.\n> >\n> > Maybe the control should be named AEGCState ?\n>\n> It's uuuugly.\n>\n\nIt is !\n\n> But also maybe more correct. idk I'm neutral maybe people that have a\n> bigger preference can do the bikeshedding :)\n>\n\nLet's see if we get more comments, I'll keep it as it is for now\n\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/control_ids.yaml | 64 ++++++++++++++++++++++++----------\n> > 1 file changed, 46 insertions(+), 18 deletions(-)\n> >\n> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > index bb5eeb1507a9..d50df8bcad28 100644\n> > --- a/src/libcamera/control_ids.yaml\n> > +++ b/src/libcamera/control_ids.yaml\n> > @@ -10,43 +10,71 @@ controls:\n> > - AeState:\n> > type: int32_t\n> > description: |\n> > - Control to report the AE algorithm state associated with the capture\n> > - result.\n> > + Control to report the AEGC algorithm state.\n> >\n> > - The state is still reported even if ExposureTimeMode or\n> > - AnalogueGainMode is set to Manual.\n> > + The AEGC algorithm computes the exposure time and the analogue gain\n> > + values to be applied to the image sensor.\n> > +\n> > + The AEGC algorithm behaviour is controlled by the ExposureTimeMode and\n> > + AnalogueGainMode controls, which allow applications to decide how\n> > + the exposure time and gain are computed, in Auto or Manual mode,\n> > + independently one from the other.\n>\n> s/one from the other/from one another/\n>\n\nThanks :)\n\n> > +\n> > + The AeState control reports the AEGC algorithm state through a single\n> > + value and describes it a single computation block which computes\n>\n> s/it //\n>\n\nI actually meant \"describes it as a single computation block\"\n\nDo you think it's correct ?\n\n> > + both the exposure time and the analogue gain values.\n> > +\n> > + When both the exposure time and analogue gain values are configured to\n> > + be in Manual mode, the AEGC algorithm is quiescent and does not actively\n> > + compute any value and the AeState control will report AeStateIdle.\n> > +\n> > + When at least the exposure time or analogue gain are configured to be\n> > + computed by the AEGC algorithm, the AeState control will report if the\n> > + algorithm has converged to stable values for any of the controls set\n>\n> I would've s/any/all/, but now I'm wondering if it's even possible for\n> AEGC with both E and G set to auto to have only one converged but not\n> the other.\n\nGood question\n\n>\n> Although maybe s/any/all/ is proper, because this is *one* state to\n> describe *all* of AEGC, so I think if at least one is still converging\n> it should be converging, and only if both are converged then it should\n> be converged. Even if it's not really possible, that's probably the more\n> correct description.\n>\n\nI think s/any/all as you suggested helps clarify it\n\n> > + to be computed in Auto mode.\n> >\n> > - \\sa AnalogueGain\n> > \\sa AnalogueGainMode\n> > - \\sa ExposureTime\n> > \\sa ExposureTimeMode\n> >\n> > enum:\n> > - name: AeStateIdle\n> > value: 0\n> > description: |\n> > - The AE algorithm is inactive.\n> > + The AEGC algorithm is inactive.\n> > +\n> > + This state is returned when both AnalogueGainMode and\n> > + ExposureTimeMode are set to Manual and the algorithm is not\n> > + actively computing any value.\n>\n> Do we need to explicitly state an exception for if only one is\n> manual-able? Although due to the constraints that we've defined, if\n> only one is manual-able then both are auto-able. So maybe it's implicit?\n>\n\nIf I got your question right you suggest that only one might be\nmanual-able and the other is auto-only, hence the state will never be\nreported as idle ?\n\nIsn't this desired ? The AeState control will report the state of the\nauto control, either if it is searching or it has converged, while the\nother will be operated in manual mode.\n\nI might have missed what constraints you are referring to here.\n\n> >\n> > - This state should be returned if both AnalogueGainMode and\n> > - ExposureTimeMode are set to manual (or one, if the camera only\n> > - supports one of the two controls).\n> > + When ExposureTimeMode or AnalogueGainMode are set to Auto mode, the\n> > + AEGC algorithm might spontaneously initiate a new scan, in which\n> > + case the AeState control is moved to AeStateSearching.\n> > - name: AeStateSearching\n> > value: 1\n> > description: |\n> > - The AE algorithm has not converged yet.\n> > + The AEGC algorithm is actively computing new values, for either the\n> > + exposure time or the analogue gain, but has not converged to a\n> > + stable result yet.\n> > +\n> > + This state is returned if at least one of AnalogueGainMode\n> > + or ExposureTimeMode is set to auto and the algorithm hasn't\n> > + converged yet.\n> >\n> > - This state should be returned if at least one of AnalogueGainMode\n> > - or ExposureTimeMode is set to auto, and the AE algorithm hasn't\n> > - converged yet. If the AE algorithm converges, the state shall go to\n> > - AeStateConverged.\n> > + The AEGC algorithm converges once stable values are computed for\n> > + any of the controls set to be computed in Auto mode.\n> > +\n> > + Once the algorithm converges the state is moved to AeStateConverged.\n> > - name: AeStateConverged\n> > value: 2\n> > description: |\n> > The AE algorithm has converged.\n>\n> s/AE/AEGC/?\n>\n\nThanks!\n\n>\n> Otherwise looks good.\n>\n>\n> Thanks,\n>\n> Paul\n>\n>\n> >\n> > - This state should be returned if at least one of AnalogueGainMode\n> > - or ExposureTimeMode is set to auto, and the AE algorithm has\n> > - converged.\n> > + This state is returned if at least one of AnalogueGainMode\n> > + or ExposureTimeMode is set to Auto, and the AEGC algorithm has\n> > + converged to stable value.\n> > +\n> > + The AEGC algorithm might spontaneously re-initiate an AE scan, in\n> > + which case the state is moved to AeStateSearching.\n> >\n> > # AeMeteringMode needs further attention:\n> > # - Auto-generate max enum value.\n> > --\n> > 2.36.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 A8C43BE173\n\tfor ;\n\tThu, 11 Aug 2022 07:39:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D73B36332B;\n\tThu, 11 Aug 2022 09:39:26 +0200 (CEST)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B936963327\n\tfor ;\n\tThu, 11 Aug 2022 09:39:25 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 1B0121BF210;\n\tThu, 11 Aug 2022 07:39:24 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660203566;\n\tbh=u36HULo9vA7dCA1XZYVaSRrDSytiDzUM9+1pF1ikYiM=;\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=L8Zs6dZGFgxYcSga/p+CfZQPuwsUuQsKI0sOE216J6LVJyICeWCcSo6XR3LJ9my2M\n\tNFaBV3ZiVZDuDd4IHjIEcIR5vVVEwWws3vvwDp29HGzAVPCar8lVEkeo2fK3x3o5fk\n\t0ycSG3lby2yApScop09dEq30heuujoNs1vldnxOiSBpSXMgXVRqAkcMy8lMn6jwQmP\n\ta6iNjVxtBkHee4fseLVGR2qWFWIjQPbSw3uUopbXVGzxxtnwGMS2HV5bnIo4pJ8j2x\n\ti1ZJckJrSHjcVxypMSNyA8o1RE2BQUod7F51P/wZHqH2YVKv9jvppvaxv/CHwbseHn\n\tph6vwbszXvzcw==","Date":"Thu, 11 Aug 2022 09:39:23 +0200","To":"paul.elder@ideasonboard.com","Message-ID":"<20220811073923.3lqfxgpz6gxhbhte@uno.localdomain>","References":"<20220518134728.777709-2-paul.elder@ideasonboard.com>\n\t<20220701154701.354052-1-jacopo@jmondi.org>\n\t<20220701154701.354052-4-jacopo@jmondi.org>\n\t<20220803150410.GO311202@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220803150410.GO311202@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH 4/9] fixup: Expand AeState\n\tdocumentation","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\" "}}]