[{"id":17990,"web_url":"https://patchwork.libcamera.org/comment/17990/","msgid":"<20210705155209.lz2rpshw7nxy4wam@uno.localdomain>","date":"2021-07-05T15:52:09","subject":"Re: [libcamera-devel] [RFC PATCH v3 05/16] controls: Replace\n\tAeLocked with AeState, and add AeLock","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul,\n\nOn Fri, Jul 02, 2021 at 07:37:49PM +0900, Paul Elder wrote:\n> AeLocked alone isn't sufficient for reporting the AE state, so replace\n> it with AeState. Add an AeLock control for instructing the camera to\n> lock the AE values.\n>\n> Update the current users of AeLocked accordingly.\n\nAs discussed, I like this change. Let's see what RPi and Laurent\nthink.\n\nThanks\n j\n\n>\n> Signed-off-by: Paul Elder \n>\n> ---\n> No change in v3\n> ---\n> src/ipa/raspberrypi/raspberrypi.cpp | 5 +-\n> src/ipa/rkisp1/rkisp1.cpp | 13 ++--\n> src/libcamera/control_ids.yaml | 96 ++++++++++++++++++-----------\n> 3 files changed, 71 insertions(+), 43 deletions(-)\n>\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 4d09a84f..4981aa29 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -469,7 +469,10 @@ void IPARPi::reportMetadata()\n>\n> \tAgcStatus *agcStatus = rpiMetadata_.GetLocked(\"agc.status\");\n> \tif (agcStatus) {\n> -\t\tlibcameraMetadata_.set(controls::AeLocked, agcStatus->locked);\n> +\t\tlibcameraMetadata_.set(controls::AeState,\n> +\t\t\t\t agcStatus->locked ?\n> +\t\t\t\t controls::AeStateLocked :\n> +\t\t\t\t controls::AeStateSearching);\n> \t\tlibcameraMetadata_.set(controls::DigitalGain, agcStatus->digital_gain);\n> \t}\n>\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index cdfb4d13..4eca26e2 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -51,7 +51,7 @@ private:\n> \t\t\t const rkisp1_stat_buffer *stats);\n>\n> \tvoid setControls(unsigned int frame);\n> -\tvoid metadataReady(unsigned int frame, unsigned int aeState);\n> +\tvoid metadataReady(unsigned int frame, int aeState);\n>\n> \tstd::map buffers_;\n> \tstd::map buffersMemory_;\n> @@ -227,7 +227,7 @@ void IPARkISP1::updateStatistics(unsigned int frame,\n> \t\t\t\t const rkisp1_stat_buffer *stats)\n> {\n> \tconst rkisp1_cif_isp_stat *params = &stats->params;\n> -\tunsigned int aeState = 0;\n> +\tint aeState = controls::AeStateInactive;\n>\n> \tif (stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP) {\n> \t\tconst rkisp1_cif_isp_ae_stat *ae = ¶ms->ae;\n> @@ -262,7 +262,9 @@ void IPARkISP1::updateStatistics(unsigned int frame,\n> \t\t\tsetControls(frame + 1);\n> \t\t}\n>\n> -\t\taeState = fabs(factor - 1.0f) < 0.05f ? 2 : 1;\n> +\t\taeState = fabs(factor - 1.0f) < 0.05f ?\n> +\t\t\t controls::AeStateConverged :\n> +\t\t\t controls::AeStateSearching;\n> \t}\n>\n> \tmetadataReady(frame, aeState);\n> @@ -281,12 +283,11 @@ void IPARkISP1::setControls(unsigned int frame)\n> \tqueueFrameAction.emit(frame, op);\n> }\n>\n> -void IPARkISP1::metadataReady(unsigned int frame, unsigned int aeState)\n> +void IPARkISP1::metadataReady(unsigned int frame, int aeState)\n> {\n> \tControlList ctrls(controls::controls);\n>\n> -\tif (aeState)\n> -\t\tctrls.set(controls::AeLocked, aeState == 2);\n> +\tctrls.set(controls::AeState, aeState);\n>\n> \tRkISP1Action op;\n> \top.op = ActionMetadata;\n> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> index 9d4638ae..5717bc1f 100644\n> --- a/src/libcamera/control_ids.yaml\n> +++ b/src/libcamera/control_ids.yaml\n> @@ -14,16 +14,70 @@ controls:\n>\n> \\sa ExposureTime AnalogueGain\n>\n> - - AeLocked:\n> + - AeLock:\n> type: bool\n> description: |\n> - Report the lock status of a running AE algorithm.\n> + Control to lock the AE values.\n> + When set to true, the AE algorithm is locked to its latest parameters,\n> + and will not change exposure settings until set to false.\n> + \\sa AeState\n>\n> - If the AE algorithm is locked the value shall be set to true, if it's\n> - converging it shall be set to false. If the AE algorithm is not\n> - running the control shall not be present in the metadata control list.\n> + - AeState:\n> + type: int32_t\n> + description: |\n> + Control to report the current AE algorithm state. Enabling or disabling\n> + AE (AeEnable) always resets the AeState to AeStateInactive. The camera\n> + device can do several state transitions between two results, if it is\n> + allowed by the state transition table. For example, AeStateInactive may\n> + never actually be seen in a result.\n>\n> - \\sa AeEnable\n> + The state in the result is the state for this image (in sync with this\n> + image). If AE state becomes AeStateConverged, then the image data\n> + associated with the result should be good to use.\n> +\n> + The state transitions mentioned below assume that AeEnable is on.\n> +\n> + \\sa AeLock\n> + enum:\n> + - name: AeStateInactive\n> + value: 0\n> + description: |\n> + The AE algorithm is inactive.\n> + If the camera initiates an AE scan, the state shall go to Searching.\n> + If AeLock is on, the state shall go to Locked.\n> + - name: AeStateSearching\n> + value: 1\n> + description: |\n> + The AE algorithm has not converged yet.\n> + If the camera finishes an AE scan, the state shall go to Converged.\n> + If the camera finishes an AE scan, but flash is required, the state\n> + shall go to FlashRequired.\n> + If AeLock is on, the state shall go to Locked.\n> + - name: AeStateConverged\n> + value: 2\n> + description: |\n> + The AE algorithm has converged.\n> + If the camera initiates an AE scan, the state shall go to Searching.\n> + If AeLock is on, the state shall go to Locked.\n> + - name: AeStateLocked\n> + value: 3\n> + description: |\n> + The AE algorithm is locked.\n> + If AeLock is off, the state can go to Searching, Converged, or\n> + FlashRequired.\n> + - name: AeStateFlashRequired\n> + value: 4\n> + description: |\n> + The AE algorithm would need a flash for good results\n> + If the camera initiates an AE scan, the state shall go to Searching.\n> + If AeLock is on, the state shall go to Locked.\n> + - name: AeStatePrecapture\n> + value: 5\n> + description: |\n> + The AE algorithm has started a pre-capture metering session.\n> + After the sequence is finished, the state shall go to Converged if\n> + AeLock is off, and Locked if it is on.\n> + \\sa AePrecaptureTrigger\n>\n> # AeMeteringMode needs further attention:\n> # - Auto-generate max enum value.\n> @@ -477,36 +531,6 @@ controls:\n> High quality aberration correction which might reduce the frame\n> rate.\n>\n> - - AeState:\n> - type: int32_t\n> - draft: true\n> - description: |\n> - Control to report the current AE algorithm state. Currently identical to\n> - ANDROID_CONTROL_AE_STATE.\n> -\n> - Current state of the AE algorithm.\n> - enum:\n> - - name: AeStateInactive\n> - value: 0\n> - description: The AE algorithm is inactive.\n> - - name: AeStateSearching\n> - value: 1\n> - description: The AE algorithm has not converged yet.\n> - - name: AeStateConverged\n> - value: 2\n> - description: The AE algorithm has converged.\n> - - name: AeStateLocked\n> - value: 3\n> - description: The AE algorithm is locked.\n> - - name: AeStateFlashRequired\n> - value: 4\n> - description: The AE algorithm would need a flash for good results\n> - - name: AeStatePrecapture\n> - value: 5\n> - description: |\n> - The AE algorithm has started a pre-capture metering session.\n> - \\sa AePrecaptureTrigger\n> -\n> - AfState:\n> type: int32_t\n> draft: true\n> --\n> 2.27.0\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 3B49CBD794\n\tfor ;\n\tMon, 5 Jul 2021 15:51:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B91A7684F9;\n\tMon, 5 Jul 2021 17:51:21 +0200 (CEST)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EEC366050A\n\tfor ;\n\tMon, 5 Jul 2021 17:51:20 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 5F3A11C000E;\n\tMon, 5 Jul 2021 15:51:20 +0000 (UTC)"],"Date":"Mon, 5 Jul 2021 17:52:09 +0200","From":"Jacopo Mondi ","To":"Paul Elder ","Message-ID":"<20210705155209.lz2rpshw7nxy4wam@uno.localdomain>","References":"<20210702103800.41291-1-paul.elder@ideasonboard.com>\n\t<20210702103800.41291-6-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210702103800.41291-6-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH v3 05/16] controls: Replace\n\tAeLocked with AeState, and add AeLock","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17992,"web_url":"https://patchwork.libcamera.org/comment/17992/","msgid":"","date":"2021-07-05T17:34:07","subject":"Re: [libcamera-devel] [RFC PATCH v3 05/16] controls: Replace\n\tAeLocked with AeState, and add AeLock","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Paul,\n\nThank you for your work.\n\nOn Fri, 2 Jul 2021 at 11:38, Paul Elder wrote:\n\n> AeLocked alone isn't sufficient for reporting the AE state, so replace\n> it with AeState. Add an AeLock control for instructing the camera to\n> lock the AE values.\n>\n> Update the current users of AeLocked accordingly.\n>\n> Signed-off-by: Paul Elder \n>\n> ---\n> No change in v3\n> ---\n> src/ipa/raspberrypi/raspberrypi.cpp | 5 +-\n> src/ipa/rkisp1/rkisp1.cpp | 13 ++--\n> src/libcamera/control_ids.yaml | 96 ++++++++++++++++++-----------\n> 3 files changed, 71 insertions(+), 43 deletions(-)\n>\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 4d09a84f..4981aa29 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -469,7 +469,10 @@ void IPARPi::reportMetadata()\n>\n> AgcStatus *agcStatus =\n> rpiMetadata_.GetLocked(\"agc.status\");\n> if (agcStatus) {\n> - libcameraMetadata_.set(controls::AeLocked,\n> agcStatus->locked);\n> + libcameraMetadata_.set(controls::AeState,\n> + agcStatus->locked ?\n> + controls::AeStateLocked :\n> + controls::AeStateSearching);\n>\n\nYes, this seems like the correct state mapping.\n\n\n> libcameraMetadata_.set(controls::DigitalGain,\n> agcStatus->digital_gain);\n> }\n>\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index cdfb4d13..4eca26e2 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -51,7 +51,7 @@ private:\n> const rkisp1_stat_buffer *stats);\n>\n> void setControls(unsigned int frame);\n> - void metadataReady(unsigned int frame, unsigned int aeState);\n> + void metadataReady(unsigned int frame, int aeState);\n>\n> std::map buffers_;\n> std::map buffersMemory_;\n> @@ -227,7 +227,7 @@ void IPARkISP1::updateStatistics(unsigned int frame,\n> const rkisp1_stat_buffer *stats)\n> {\n> const rkisp1_cif_isp_stat *params = &stats->params;\n> - unsigned int aeState = 0;\n> + int aeState = controls::AeStateInactive;\n>\n> if (stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP) {\n> const rkisp1_cif_isp_ae_stat *ae = ¶ms->ae;\n> @@ -262,7 +262,9 @@ void IPARkISP1::updateStatistics(unsigned int frame,\n> setControls(frame + 1);\n> }\n>\n> - aeState = fabs(factor - 1.0f) < 0.05f ? 2 : 1;\n> + aeState = fabs(factor - 1.0f) < 0.05f ?\n> + controls::AeStateConverged :\n> + controls::AeStateSearching;\n> }\n>\n> metadataReady(frame, aeState);\n> @@ -281,12 +283,11 @@ void IPARkISP1::setControls(unsigned int frame)\n> queueFrameAction.emit(frame, op);\n> }\n>\n> -void IPARkISP1::metadataReady(unsigned int frame, unsigned int aeState)\n> +void IPARkISP1::metadataReady(unsigned int frame, int aeState)\n> {\n> ControlList ctrls(controls::controls);\n>\n> - if (aeState)\n> - ctrls.set(controls::AeLocked, aeState == 2);\n> + ctrls.set(controls::AeState, aeState);\n>\n> RkISP1Action op;\n> op.op = ActionMetadata;\n> diff --git a/src/libcamera/control_ids.yaml\n> b/src/libcamera/control_ids.yaml\n> index 9d4638ae..5717bc1f 100644\n> --- a/src/libcamera/control_ids.yaml\n> +++ b/src/libcamera/control_ids.yaml\n> @@ -14,16 +14,70 @@ controls:\n>\n> \\sa ExposureTime AnalogueGain\n>\n> - - AeLocked:\n> + - AeLock:\n> type: bool\n> description: |\n> - Report the lock status of a running AE algorithm.\n> + Control to lock the AE values.\n> + When set to true, the AE algorithm is locked to its latest\n> parameters,\n> + and will not change exposure settings until set to false.\n> + \\sa AeState\n>\n\nI know I am repeating myself here, but I don't see the need\nto differentiate between\nAeLock and AeEnable == falses.\n\n\n> - If the AE algorithm is locked the value shall be set to true, if\n> it's\n> - converging it shall be set to false. If the AE algorithm is not\n> - running the control shall not be present in the metadata control\n> list.\n> + - AeState:\n> + type: int32_t\n> + description: |\n> + Control to report the current AE algorithm state. Enabling or\n> disabling\n> + AE (AeEnable) always resets the AeState to AeStateInactive. The\n> camera\n> + device can do several state transitions between two results, if\n> it is\n> + allowed by the state transition table. For example,\n> AeStateInactive may\n> + never actually be seen in a result.\n>\n> - \\sa AeEnable\n> + The state in the result is the state for this image (in sync with\n> this\n> + image). If AE state becomes AeStateConverged, then the image data\n> + associated with the result should be good to use.\n> +\n> + The state transitions mentioned below assume that AeEnable is on.\n> +\n> + \\sa AeLock\n> + enum:\n> + - name: AeStateInactive\n> + value: 0\n> + description: |\n> + The AE algorithm is inactive.\n> + If the camera initiates an AE scan, the state shall go to\n> Searching.\n> + If AeLock is on, the state shall go to Locked.\n>\n\nThis state is also a bit ambiguous to me. Is this the state that must be\nreported\nif AeEnable == false? If so, is that not also conveyed by setting the\nlatter?\n\nIf the objective of the change is to wholesale do what Android does, then I\ndon't\nreally have any objection to this change as-is, and we can treat the above\nstates as best we can in the Raspberry Pi IPA. Otherwise, I do see some\nscope\nto simplify some of this.\n\nRegards,\nNaush\n\n\n> + - name: AeStateSearching\n> + value: 1\n> + description: |\n> + The AE algorithm has not converged yet.\n> + If the camera finishes an AE scan, the state shall go to\n> Converged.\n> + If the camera finishes an AE scan, but flash is required, the\n> state\n> + shall go to FlashRequired.\n> + If AeLock is on, the state shall go to Locked.\n> + - name: AeStateConverged\n> + value: 2\n> + description: |\n> + The AE algorithm has converged.\n> + If the camera initiates an AE scan, the state shall go to\n> Searching.\n> + If AeLock is on, the state shall go to Locked.\n> + - name: AeStateLocked\n> + value: 3\n> + description: |\n> + The AE algorithm is locked.\n> + If AeLock is off, the state can go to Searching, Converged, or\n> + FlashRequired.\n> + - name: AeStateFlashRequired\n> + value: 4\n> + description: |\n> + The AE algorithm would need a flash for good results\n> + If the camera initiates an AE scan, the state shall go to\n> Searching.\n> + If AeLock is on, the state shall go to Locked.\n> + - name: AeStatePrecapture\n> + value: 5\n> + description: |\n> + The AE algorithm has started a pre-capture metering session.\n> + After the sequence is finished, the state shall go to\n> Converged if\n> + AeLock is off, and Locked if it is on.\n> + \\sa AePrecaptureTrigger\n>\n> # AeMeteringMode needs further attention:\n> # - Auto-generate max enum value.\n> @@ -477,36 +531,6 @@ controls:\n> High quality aberration correction which might reduce the\n> frame\n> rate.\n>\n> - - AeState:\n> - type: int32_t\n> - draft: true\n> - description: |\n> - Control to report the current AE algorithm state. Currently\n> identical to\n> - ANDROID_CONTROL_AE_STATE.\n> -\n> - Current state of the AE algorithm.\n> - enum:\n> - - name: AeStateInactive\n> - value: 0\n> - description: The AE algorithm is inactive.\n> - - name: AeStateSearching\n> - value: 1\n> - description: The AE algorithm has not converged yet.\n> - - name: AeStateConverged\n> - value: 2\n> - description: The AE algorithm has converged.\n> - - name: AeStateLocked\n> - value: 3\n> - description: The AE algorithm is locked.\n> - - name: AeStateFlashRequired\n> - value: 4\n> - description: The AE algorithm would need a flash for good\n> results\n> - - name: AeStatePrecapture\n> - value: 5\n> - description: |\n> - The AE algorithm has started a pre-capture metering session.\n> - \\sa AePrecaptureTrigger\n> -\n> - AfState:\n> type: int32_t\n> draft: true\n> --\n> 2.27.0\n>\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 965B2C3223\n\tfor ;\n\tMon, 5 Jul 2021 17:34:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F22FD684F9;\n\tMon, 5 Jul 2021 19:34:26 +0200 (CEST)","from mail-lj1-x22b.google.com (mail-lj1-x22b.google.com\n\t[IPv6:2a00:1450:4864:20::22b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2A9E26050A\n\tfor ;\n\tMon, 5 Jul 2021 19:34:25 +0200 (CEST)","by mail-lj1-x22b.google.com with SMTP id q4so25534033ljp.13\n\tfor ;\n\tMon, 05 Jul 2021 10:34:25 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"ZnARgXwa\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=nc7SPE82rXz8sKYYNqxc/zb/1Pv0oZSoQ1igVUNj/P8=;\n\tb=ZnARgXwavAJKdyCE5IsGQ0rAVNH2oZIwfGWNvA2SfzcNpg/gAaQBMNgcJ9H1498fw0\n\tGBDTZAde/Qfb6VK68rr1ITjfVBpMWsru50V4T4e5Xg3mRmaNJt/pkql8g4VPb33RzrV7\n\t6rPCNaa/ivXLQ45YcXk1qhlzhbZZ5XrEbzFjJ7Md7S1RPbwqBou/Ie9ZbG61/3B1ynp8\n\tTsmn5+uCjE1JJyMyy6elUS4k4axq+8Nbw5LPHGnfFz1Ql73wuwgi2ii6ajZbv2WQzSeH\n\tsGSHc54Fm5/ARb3KjmuaXJFW/NtGGbaFKeRnDSbUK0/7np7PbLqup5KnwpsERYbrDjqx\n\tIePw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=nc7SPE82rXz8sKYYNqxc/zb/1Pv0oZSoQ1igVUNj/P8=;\n\tb=ZOUD1uS5vBTwrmqwnhN8dcsaqhsBl2dRB5LDMIhxGQm9h5A1E5NTePjgI0dZIc+Z0l\n\tpIIjYk6pTooB7OsRhGULehjBU955pbCTZ0cE1mkPJnhCj+WsauHCxmj2aLxp80vAdpq4\n\tXuv0JsunDAb///SAwJpwRB14NHY97eLu7mVScrV1wORy7L9ImSMrN+sKvQkdgt/qDr+M\n\t6uR27lt0hNW86k1KcHaX/sVHGn5uXO6D/2NttZB+RLiLt4R4itjIznqhd0ndvmIOIsVO\n\tgY9Ydf9wpMvQlWY/HASHeUac7s5OJ4bkYzx4gwaaogk8iPUGialrr43S+JSXx1k5CTNn\n\tKFsg==","X-Gm-Message-State":"AOAM531knDW1/nFkaTofccEZaQqtE2n0CDpzPL2SjxZypY4hWipj6gGv\n\txNlmAFzjeNUuktjLZPLdT6F8ygY7P/WUPCYYNkESuYJXARA=","X-Google-Smtp-Source":"ABdhPJzUZ9cVFiVsmfICTtQP9rxsBcxNpQ7XtpatOSaR30oVRVSRUXZ+FZvJufvXTY6M1Buwud4rZ4W67BSSVjbOAaw=","X-Received":"by 2002:a2e:8905:: with SMTP id d5mr7753224lji.400.1625506464257;\n\tMon, 05 Jul 2021 10:34:24 -0700 (PDT)","MIME-Version":"1.0","References":"<20210702103800.41291-1-paul.elder@ideasonboard.com>\n\t<20210702103800.41291-6-paul.elder@ideasonboard.com>","In-Reply-To":"<20210702103800.41291-6-paul.elder@ideasonboard.com>","From":"Naushir Patuck ","Date":"Mon, 5 Jul 2021 18:34:07 +0100","Message-ID":"","To":"Paul Elder ","Content-Type":"multipart/alternative; boundary=\"00000000000061e99005c663b7ba\"","Subject":"Re: [libcamera-devel] [RFC PATCH v3 05/16] controls: Replace\n\tAeLocked with AeState, and add AeLock","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","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18089,"web_url":"https://patchwork.libcamera.org/comment/18089/","msgid":"","date":"2021-07-11T22:35:29","subject":"Re: [libcamera-devel] [RFC PATCH v3 05/16] controls: Replace\n\tAeLocked with AeState, and add AeLock","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\n(CC'ing David)\n\nApologies for the late reply, I should have shared more information on\nthis topic earlier.\n\nOn Mon, Jul 05, 2021 at 06:34:07PM +0100, Naushir Patuck wrote:\n> On Fri, 2 Jul 2021 at 11:38, Paul Elder wrote:\n> > AeLocked alone isn't sufficient for reporting the AE state, so replace\n> > it with AeState. Add an AeLock control for instructing the camera to\n> > lock the AE values.\n> >\n> > Update the current users of AeLocked accordingly.\n> >\n> > Signed-off-by: Paul Elder \n> >\n> > ---\n> > No change in v3\n> > ---\n> > src/ipa/raspberrypi/raspberrypi.cpp | 5 +-\n> > src/ipa/rkisp1/rkisp1.cpp | 13 ++--\n> > src/libcamera/control_ids.yaml | 96 ++++++++++++++++++-----------\n> > 3 files changed, 71 insertions(+), 43 deletions(-)\n> >\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 4d09a84f..4981aa29 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -469,7 +469,10 @@ void IPARPi::reportMetadata()\n> >\n> > AgcStatus *agcStatus = rpiMetadata_.GetLocked(\"agc.status\");\n> > if (agcStatus) {\n> > - libcameraMetadata_.set(controls::AeLocked, agcStatus->locked);\n> > + libcameraMetadata_.set(controls::AeState,\n> > + agcStatus->locked ?\n> > + controls::AeStateLocked :\n> > + controls::AeStateSearching);\n> \n> Yes, this seems like the correct state mapping.\n\nIs it ? I thought it should be AeStateConverged.\n\n> > libcameraMetadata_.set(controls::DigitalGain, agcStatus->digital_gain);\n> > }\n> >\n> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > index cdfb4d13..4eca26e2 100644\n> > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > @@ -51,7 +51,7 @@ private:\n> > const rkisp1_stat_buffer *stats);\n> >\n> > void setControls(unsigned int frame);\n> > - void metadataReady(unsigned int frame, unsigned int aeState);\n> > + void metadataReady(unsigned int frame, int aeState);\n> >\n> > std::map buffers_;\n> > std::map buffersMemory_;\n> > @@ -227,7 +227,7 @@ void IPARkISP1::updateStatistics(unsigned int frame,\n> > const rkisp1_stat_buffer *stats)\n> > {\n> > const rkisp1_cif_isp_stat *params = &stats->params;\n> > - unsigned int aeState = 0;\n> > + int aeState = controls::AeStateInactive;\n> >\n> > if (stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP) {\n> > const rkisp1_cif_isp_ae_stat *ae = ¶ms->ae;\n> > @@ -262,7 +262,9 @@ void IPARkISP1::updateStatistics(unsigned int frame,\n> > setControls(frame + 1);\n> > }\n> >\n> > - aeState = fabs(factor - 1.0f) < 0.05f ? 2 : 1;\n> > + aeState = fabs(factor - 1.0f) < 0.05f ?\n> > + controls::AeStateConverged :\n> > + controls::AeStateSearching;\n> > }\n> >\n> > metadataReady(frame, aeState);\n> > @@ -281,12 +283,11 @@ void IPARkISP1::setControls(unsigned int frame)\n> > queueFrameAction.emit(frame, op);\n> > }\n> >\n> > -void IPARkISP1::metadataReady(unsigned int frame, unsigned int aeState)\n> > +void IPARkISP1::metadataReady(unsigned int frame, int aeState)\n> > {\n> > ControlList ctrls(controls::controls);\n> >\n> > - if (aeState)\n> > - ctrls.set(controls::AeLocked, aeState == 2);\n> > + ctrls.set(controls::AeState, aeState);\n> >\n> > RkISP1Action op;\n> > op.op = ActionMetadata;\n> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > index 9d4638ae..5717bc1f 100644\n> > --- a/src/libcamera/control_ids.yaml\n> > +++ b/src/libcamera/control_ids.yaml\n> > @@ -14,16 +14,70 @@ controls:\n> >\n> > \\sa ExposureTime AnalogueGain\n> >\n> > - - AeLocked:\n> > + - AeLock:\n> > type: bool\n> > description: |\n> > - Report the lock status of a running AE algorithm.\n> > + Control to lock the AE values.\n> > + When set to true, the AE algorithm is locked to its latest parameters,\n> > + and will not change exposure settings until set to false.\n> > + \\sa AeState\n> \n> I know I am repeating myself here, but I don't see the need to differentiate between\n> AeLock and AeEnable == falses.\n\nPlease see below.\n\n> > - If the AE algorithm is locked the value shall be set to true, if it's\n> > - converging it shall be set to false. If the AE algorithm is not\n> > - running the control shall not be present in the metadata control list.\n> > + - AeState:\n> > + type: int32_t\n> > + description: |\n> > + Control to report the current AE algorithm state. Enabling or disabling\n> > + AE (AeEnable) always resets the AeState to AeStateInactive. The camera\n> > + device can do several state transitions between two results, if it is\n> > + allowed by the state transition table. For example, AeStateInactive may\n> > + never actually be seen in a result.\n> >\n> > - \\sa AeEnable\n> > + The state in the result is the state for this image (in sync with this\n> > + image). If AE state becomes AeStateConverged, then the image data\n> > + associated with the result should be good to use.\n> > +\n> > + The state transitions mentioned below assume that AeEnable is on.\n> > +\n> > + \\sa AeLock\n> > + enum:\n> > + - name: AeStateInactive\n> > + value: 0\n> > + description: |\n> > + The AE algorithm is inactive.\n> > + If the camera initiates an AE scan, the state shall go to Searching.\n> > + If AeLock is on, the state shall go to Locked.\n> >\n> \n> This state is also a bit ambiguous to me. Is this the state that must be reported\n> if AeEnable == false? If so, is that not also conveyed by setting the latter?\n> \n> If the objective of the change is to wholesale do what Android does, then I don't\n> really have any objection to this change as-is, and we can treat the above\n> states as best we can in the Raspberry Pi IPA. Otherwise, I do see some scope\n> to simplify some of this.\n\nFirst of all, let me clarify the goal. The libcamera native API needs to\noffer the features required to implement the Android camera HAL API, but\nwe don't need to just duplicate it. When there are reasons to depart\n(for instance to fix historical mistakes made in Android, to provide\nadditional features, or just to expose an API we consider more\nconsistent), we should do so.\n\nThen, a word about the process. Implementing support for an Android\ncontrol requires plumbing it through the camera stack: definition of a\nlibcamera control, translation between the Android and libcamera control\nin the Android camera HAL, implementation of the libcamera control in\nthe pipeline handlesr, and, when applicable, in the IPAs. Defining\nlibcamera controls, as any API design effort, takes time. To allow\nmoving forward on all the other parts, we have a fast-track process to\ndefine draft controls that mimick the Android controls. All the draft\ncontrols will be redesigned before freezing the libcamera public API.\n\nThis means that AeLock and AeState should be marked as draft controls as\npart of this patch.\n\nThis being said, we don't need to define controls as draft controls if\nwe want to already go through a complete design process. The discussion\nthat stemmed from this patch series goes in that direction.\n\nIt's useful, in this context, to understand why the Android AE (and AWB)\ncontrols have been designed this way. Conceptually speaking, the\nalgorithms are considered, in Android, to be one level above manual\ncontrols.\n\n Controls Metadata\n /---------- == OFF ---------\\\n | v\naeMode -/ +------+ |\\\naeLock --> | AE | --> |0\\ +--------+\naeExposureCompensation +------+ | | --> | Sensor | --> exposureTime\n | | +--------+ sensitivity\nexposureTime -----------------------> |1/\nsensitivity |/\n\n(The AE controls are in the adnroid.control class, while the sensor\nmanual control are in the android.sensor class.)\n\nThe aeMode control selects whether the AE algorithm (when != OFF) or the\nmanual controls (when == OFF) drive the sensor. The aeLock control tells\nthe AE algorithm to freeze its output (when ON) or operate normally\n(when OFF). The metadata always report the values applied to the sensor,\nregardless of whether they come from the AE algorithm or from manual\ncontrols.\n\nOne important difference, compared to the model we currently implement,\nis that the Android AE algorithm is independent from the manual sensor\ncontrols. The values of the manual controls aren't affected by the AE\nalgorithm, and when AE is disabled, the last set manual control values\nare applied. This thus require an aeLock control to transition\nseamlessly between AE and manual mode, as the application needs to set\nthe manual controls to the values applied to the sensor on the last\nframe that AE was on, which couldn't otherwise be done due to the\ninternal delays.\n\nThe existing libcamera AE controls are ill-defined. There's a \\todo\ncomment to document the interactions between AeEnable and setting a\nfixed value for ExposureTime or AnalogueGain. I'm actually not sure what\nhappens if AeEnable is set to false when ExposureTime and/or\nAnalogueGain are 0, or when AeEnable is set to true when ExposureTime\nand/or AnalogueGain are not 0.\n\nWe need a formal model for how libcamera algorithms interact with manual\ncontrols. It doesn't have to duplicate the Android model, but it has to\noffer at least the same features. I believe we need to take it one step\nfurther, as we want to support exposure priority and aperture priority.\nI think we should actually talk about gain/sensitivity priority instead\nof aperture priority for the existing feature, and already prepare for\ndevices that have a controllable lens aperture.\n\nNow, the hard question: how should libcamera model this ? :-) Naush,\nDavid, your feedback from a RPi point of view would be very appreciated.\nPlease also let me know if the above explanation is clear or if you need\nmore information.\n\n> > + - name: AeStateSearching\n> > + value: 1\n> > + description: |\n> > + The AE algorithm has not converged yet.\n> > + If the camera finishes an AE scan, the state shall go to Converged.\n> > + If the camera finishes an AE scan, but flash is required, the state\n> > + shall go to FlashRequired.\n> > + If AeLock is on, the state shall go to Locked.\n> > + - name: AeStateConverged\n> > + value: 2\n> > + description: |\n> > + The AE algorithm has converged.\n> > + If the camera initiates an AE scan, the state shall go to Searching.\n> > + If AeLock is on, the state shall go to Locked.\n> > + - name: AeStateLocked\n> > + value: 3\n> > + description: |\n> > + The AE algorithm is locked.\n> > + If AeLock is off, the state can go to Searching, Converged, or\n> > + FlashRequired.\n> > + - name: AeStateFlashRequired\n> > + value: 4\n> > + description: |\n> > + The AE algorithm would need a flash for good results\n> > + If the camera initiates an AE scan, the state shall go to Searching.\n> > + If AeLock is on, the state shall go to Locked.\n> > + - name: AeStatePrecapture\n> > + value: 5\n> > + description: |\n> > + The AE algorithm has started a pre-capture metering session.\n> > + After the sequence is finished, the state shall go to Converged if\n> > + AeLock is off, and Locked if it is on.\n> > + \\sa AePrecaptureTrigger\n> >\n> > # AeMeteringMode needs further attention:\n> > # - Auto-generate max enum value.\n> > @@ -477,36 +531,6 @@ controls:\n> > High quality aberration correction which might reduce the frame\n> > rate.\n> >\n> > - - AeState:\n> > - type: int32_t\n> > - draft: true\n> > - description: |\n> > - Control to report the current AE algorithm state. Currently identical to\n> > - ANDROID_CONTROL_AE_STATE.\n> > -\n> > - Current state of the AE algorithm.\n> > - enum:\n> > - - name: AeStateInactive\n> > - value: 0\n> > - description: The AE algorithm is inactive.\n> > - - name: AeStateSearching\n> > - value: 1\n> > - description: The AE algorithm has not converged yet.\n> > - - name: AeStateConverged\n> > - value: 2\n> > - description: The AE algorithm has converged.\n> > - - name: AeStateLocked\n> > - value: 3\n> > - description: The AE algorithm is locked.\n> > - - name: AeStateFlashRequired\n> > - value: 4\n> > - description: The AE algorithm would need a flash for good results\n> > - - name: AeStatePrecapture\n> > - value: 5\n> > - description: |\n> > - The AE algorithm has started a pre-capture metering session.\n> > - \\sa AePrecaptureTrigger\n> > -\n> > - AfState:\n> > type: int32_t\n> > draft: true","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 9B6D0BD794\n\tfor ;\n\tSun, 11 Jul 2021 22:36:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F2C7768526;\n\tMon, 12 Jul 2021 00:36:17 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 58B3C60280\n\tfor ;\n\tMon, 12 Jul 2021 00:36:16 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B8A72CC;\n\tMon, 12 Jul 2021 00:36:15 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"tX1WwZKV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626042975;\n\tbh=1Ov81v2pUXGE+rAnWk3OoL1VGjGSIOg4oyMP6pvSUiU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tX1WwZKVF/PqbczQ+MI6SiY3i+qyuVAryZHIR1b1Sg73NK+QSd3En8r9KMiR9wwpE\n\tCMY7cTYawkUboN8jQkWXTBiNp/R7fg/pgc6KJGtBLdfSVsLgoKbm1eu+p2iKhB01+g\n\tlXtTiQH7CxeqFcAD4TwHZyV3hTgOSRQGxt0A9Q1o=","Date":"Mon, 12 Jul 2021 01:35:29 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210702103800.41291-1-paul.elder@ideasonboard.com>\n\t<20210702103800.41291-6-paul.elder@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH v3 05/16] controls: Replace\n\tAeLocked with AeState, and add AeLock","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","Cc":"David@pendragon.ideasonboard.com,\n\tlibcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18090,"web_url":"https://patchwork.libcamera.org/comment/18090/","msgid":"","date":"2021-07-11T22:36:19","subject":"Re: [libcamera-devel] [RFC PATCH v3 05/16] controls: Replace\n\tAeLocked with AeState, and add AeLock","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\n(now really CC'ing David)\n\nApologies for the late reply, I should have shared more information on\nthis topic earlier.\n\nOn Mon, Jul 05, 2021 at 06:34:07PM +0100, Naushir Patuck wrote:\n> On Fri, 2 Jul 2021 at 11:38, Paul Elder wrote:\n> > AeLocked alone isn't sufficient for reporting the AE state, so replace\n> > it with AeState. Add an AeLock control for instructing the camera to\n> > lock the AE values.\n> >\n> > Update the current users of AeLocked accordingly.\n> >\n> > Signed-off-by: Paul Elder \n> >\n> > ---\n> > No change in v3\n> > ---\n> > src/ipa/raspberrypi/raspberrypi.cpp | 5 +-\n> > src/ipa/rkisp1/rkisp1.cpp | 13 ++--\n> > src/libcamera/control_ids.yaml | 96 ++++++++++++++++++-----------\n> > 3 files changed, 71 insertions(+), 43 deletions(-)\n> >\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 4d09a84f..4981aa29 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -469,7 +469,10 @@ void IPARPi::reportMetadata()\n> >\n> > AgcStatus *agcStatus = rpiMetadata_.GetLocked(\"agc.status\");\n> > if (agcStatus) {\n> > - libcameraMetadata_.set(controls::AeLocked, agcStatus->locked);\n> > + libcameraMetadata_.set(controls::AeState,\n> > + agcStatus->locked ?\n> > + controls::AeStateLocked :\n> > + controls::AeStateSearching);\n> \n> Yes, this seems like the correct state mapping.\n\nIs it ? I thought it should be AeStateConverged.\n\n> > libcameraMetadata_.set(controls::DigitalGain, agcStatus->digital_gain);\n> > }\n> >\n> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > index cdfb4d13..4eca26e2 100644\n> > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > @@ -51,7 +51,7 @@ private:\n> > const rkisp1_stat_buffer *stats);\n> >\n> > void setControls(unsigned int frame);\n> > - void metadataReady(unsigned int frame, unsigned int aeState);\n> > + void metadataReady(unsigned int frame, int aeState);\n> >\n> > std::map buffers_;\n> > std::map buffersMemory_;\n> > @@ -227,7 +227,7 @@ void IPARkISP1::updateStatistics(unsigned int frame,\n> > const rkisp1_stat_buffer *stats)\n> > {\n> > const rkisp1_cif_isp_stat *params = &stats->params;\n> > - unsigned int aeState = 0;\n> > + int aeState = controls::AeStateInactive;\n> >\n> > if (stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP) {\n> > const rkisp1_cif_isp_ae_stat *ae = ¶ms->ae;\n> > @@ -262,7 +262,9 @@ void IPARkISP1::updateStatistics(unsigned int frame,\n> > setControls(frame + 1);\n> > }\n> >\n> > - aeState = fabs(factor - 1.0f) < 0.05f ? 2 : 1;\n> > + aeState = fabs(factor - 1.0f) < 0.05f ?\n> > + controls::AeStateConverged :\n> > + controls::AeStateSearching;\n> > }\n> >\n> > metadataReady(frame, aeState);\n> > @@ -281,12 +283,11 @@ void IPARkISP1::setControls(unsigned int frame)\n> > queueFrameAction.emit(frame, op);\n> > }\n> >\n> > -void IPARkISP1::metadataReady(unsigned int frame, unsigned int aeState)\n> > +void IPARkISP1::metadataReady(unsigned int frame, int aeState)\n> > {\n> > ControlList ctrls(controls::controls);\n> >\n> > - if (aeState)\n> > - ctrls.set(controls::AeLocked, aeState == 2);\n> > + ctrls.set(controls::AeState, aeState);\n> >\n> > RkISP1Action op;\n> > op.op = ActionMetadata;\n> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > index 9d4638ae..5717bc1f 100644\n> > --- a/src/libcamera/control_ids.yaml\n> > +++ b/src/libcamera/control_ids.yaml\n> > @@ -14,16 +14,70 @@ controls:\n> >\n> > \\sa ExposureTime AnalogueGain\n> >\n> > - - AeLocked:\n> > + - AeLock:\n> > type: bool\n> > description: |\n> > - Report the lock status of a running AE algorithm.\n> > + Control to lock the AE values.\n> > + When set to true, the AE algorithm is locked to its latest parameters,\n> > + and will not change exposure settings until set to false.\n> > + \\sa AeState\n> \n> I know I am repeating myself here, but I don't see the need to differentiate between\n> AeLock and AeEnable == falses.\n\nPlease see below.\n\n> > - If the AE algorithm is locked the value shall be set to true, if it's\n> > - converging it shall be set to false. If the AE algorithm is not\n> > - running the control shall not be present in the metadata control list.\n> > + - AeState:\n> > + type: int32_t\n> > + description: |\n> > + Control to report the current AE algorithm state. Enabling or disabling\n> > + AE (AeEnable) always resets the AeState to AeStateInactive. The camera\n> > + device can do several state transitions between two results, if it is\n> > + allowed by the state transition table. For example, AeStateInactive may\n> > + never actually be seen in a result.\n> >\n> > - \\sa AeEnable\n> > + The state in the result is the state for this image (in sync with this\n> > + image). If AE state becomes AeStateConverged, then the image data\n> > + associated with the result should be good to use.\n> > +\n> > + The state transitions mentioned below assume that AeEnable is on.\n> > +\n> > + \\sa AeLock\n> > + enum:\n> > + - name: AeStateInactive\n> > + value: 0\n> > + description: |\n> > + The AE algorithm is inactive.\n> > + If the camera initiates an AE scan, the state shall go to Searching.\n> > + If AeLock is on, the state shall go to Locked.\n> >\n> \n> This state is also a bit ambiguous to me. Is this the state that must be reported\n> if AeEnable == false? If so, is that not also conveyed by setting the latter?\n> \n> If the objective of the change is to wholesale do what Android does, then I don't\n> really have any objection to this change as-is, and we can treat the above\n> states as best we can in the Raspberry Pi IPA. Otherwise, I do see some scope\n> to simplify some of this.\n\nFirst of all, let me clarify the goal. The libcamera native API needs to\noffer the features required to implement the Android camera HAL API, but\nwe don't need to just duplicate it. When there are reasons to depart\n(for instance to fix historical mistakes made in Android, to provide\nadditional features, or just to expose an API we consider more\nconsistent), we should do so.\n\nThen, a word about the process. Implementing support for an Android\ncontrol requires plumbing it through the camera stack: definition of a\nlibcamera control, translation between the Android and libcamera control\nin the Android camera HAL, implementation of the libcamera control in\nthe pipeline handlesr, and, when applicable, in the IPAs. Defining\nlibcamera controls, as any API design effort, takes time. To allow\nmoving forward on all the other parts, we have a fast-track process to\ndefine draft controls that mimick the Android controls. All the draft\ncontrols will be redesigned before freezing the libcamera public API.\n\nThis means that AeLock and AeState should be marked as draft controls as\npart of this patch.\n\nThis being said, we don't need to define controls as draft controls if\nwe want to already go through a complete design process. The discussion\nthat stemmed from this patch series goes in that direction.\n\nIt's useful, in this context, to understand why the Android AE (and AWB)\ncontrols have been designed this way. Conceptually speaking, the\nalgorithms are considered, in Android, to be one level above manual\ncontrols.\n\n Controls Metadata\n /---------- == OFF ---------\\\n | v\naeMode -/ +------+ |\\\naeLock --> | AE | --> |0\\ +--------+\naeExposureCompensation +------+ | | --> | Sensor | --> exposureTime\n | | +--------+ sensitivity\nexposureTime -----------------------> |1/\nsensitivity |/\n\n(The AE controls are in the adnroid.control class, while the sensor\nmanual control are in the android.sensor class.)\n\nThe aeMode control selects whether the AE algorithm (when != OFF) or the\nmanual controls (when == OFF) drive the sensor. The aeLock control tells\nthe AE algorithm to freeze its output (when ON) or operate normally\n(when OFF). The metadata always report the values applied to the sensor,\nregardless of whether they come from the AE algorithm or from manual\ncontrols.\n\nOne important difference, compared to the model we currently implement,\nis that the Android AE algorithm is independent from the manual sensor\ncontrols. The values of the manual controls aren't affected by the AE\nalgorithm, and when AE is disabled, the last set manual control values\nare applied. This thus require an aeLock control to transition\nseamlessly between AE and manual mode, as the application needs to set\nthe manual controls to the values applied to the sensor on the last\nframe that AE was on, which couldn't otherwise be done due to the\ninternal delays.\n\nThe existing libcamera AE controls are ill-defined. There's a \\todo\ncomment to document the interactions between AeEnable and setting a\nfixed value for ExposureTime or AnalogueGain. I'm actually not sure what\nhappens if AeEnable is set to false when ExposureTime and/or\nAnalogueGain are 0, or when AeEnable is set to true when ExposureTime\nand/or AnalogueGain are not 0.\n\nWe need a formal model for how libcamera algorithms interact with manual\ncontrols. It doesn't have to duplicate the Android model, but it has to\noffer at least the same features. I believe we need to take it one step\nfurther, as we want to support exposure priority and aperture priority.\nI think we should actually talk about gain/sensitivity priority instead\nof aperture priority for the existing feature, and already prepare for\ndevices that have a controllable lens aperture.\n\nNow, the hard question: how should libcamera model this ? :-) Naush,\nDavid, your feedback from a RPi point of view would be very appreciated.\nPlease also let me know if the above explanation is clear or if you need\nmore information.\n\n> > + - name: AeStateSearching\n> > + value: 1\n> > + description: |\n> > + The AE algorithm has not converged yet.\n> > + If the camera finishes an AE scan, the state shall go to Converged.\n> > + If the camera finishes an AE scan, but flash is required, the state\n> > + shall go to FlashRequired.\n> > + If AeLock is on, the state shall go to Locked.\n> > + - name: AeStateConverged\n> > + value: 2\n> > + description: |\n> > + The AE algorithm has converged.\n> > + If the camera initiates an AE scan, the state shall go to Searching.\n> > + If AeLock is on, the state shall go to Locked.\n> > + - name: AeStateLocked\n> > + value: 3\n> > + description: |\n> > + The AE algorithm is locked.\n> > + If AeLock is off, the state can go to Searching, Converged, or\n> > + FlashRequired.\n> > + - name: AeStateFlashRequired\n> > + value: 4\n> > + description: |\n> > + The AE algorithm would need a flash for good results\n> > + If the camera initiates an AE scan, the state shall go to Searching.\n> > + If AeLock is on, the state shall go to Locked.\n> > + - name: AeStatePrecapture\n> > + value: 5\n> > + description: |\n> > + The AE algorithm has started a pre-capture metering session.\n> > + After the sequence is finished, the state shall go to Converged if\n> > + AeLock is off, and Locked if it is on.\n> > + \\sa AePrecaptureTrigger\n> >\n> > # AeMeteringMode needs further attention:\n> > # - Auto-generate max enum value.\n> > @@ -477,36 +531,6 @@ controls:\n> > High quality aberration correction which might reduce the frame\n> > rate.\n> >\n> > - - AeState:\n> > - type: int32_t\n> > - draft: true\n> > - description: |\n> > - Control to report the current AE algorithm state. Currently identical to\n> > - ANDROID_CONTROL_AE_STATE.\n> > -\n> > - Current state of the AE algorithm.\n> > - enum:\n> > - - name: AeStateInactive\n> > - value: 0\n> > - description: The AE algorithm is inactive.\n> > - - name: AeStateSearching\n> > - value: 1\n> > - description: The AE algorithm has not converged yet.\n> > - - name: AeStateConverged\n> > - value: 2\n> > - description: The AE algorithm has converged.\n> > - - name: AeStateLocked\n> > - value: 3\n> > - description: The AE algorithm is locked.\n> > - - name: AeStateFlashRequired\n> > - value: 4\n> > - description: The AE algorithm would need a flash for good results\n> > - - name: AeStatePrecapture\n> > - value: 5\n> > - description: |\n> > - The AE algorithm has started a pre-capture metering session.\n> > - \\sa AePrecaptureTrigger\n> > -\n> > - AfState:\n> > type: int32_t\n> > draft: true","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 07329BD794\n\tfor ;\n\tSun, 11 Jul 2021 22:37:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B3DAE68524;\n\tMon, 12 Jul 2021 00:37:07 +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 85CA060280\n\tfor ;\n\tMon, 12 Jul 2021 00:37:06 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 15170CC;\n\tMon, 12 Jul 2021 00:37:06 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ONR4jm0j\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626043026;\n\tbh=/R7xUgvJhGXC36cUoxMK91Yt4sRGBADjFZRwLx/hkfM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ONR4jm0j/9dptWOAj3Nr9DOGn9ItwVvzLQ5Dadij2D5G5IyFmu4upjy1TA1Lr0toR\n\tv/a04vPb7vNaqnSWSmLfAZPHsUL9+4MNV+8UKpUlooazUdTunNS+84w43tuYOuVxf2\n\tN9/IcvNwSoy0qyOKPoJvWlcYGV8jtvESvW455kRc=","Date":"Mon, 12 Jul 2021 01:36:19 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210702103800.41291-1-paul.elder@ideasonboard.com>\n\t<20210702103800.41291-6-paul.elder@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH v3 05/16] controls: Replace\n\tAeLocked with AeState, and add AeLock","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","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18121,"web_url":"https://patchwork.libcamera.org/comment/18121/","msgid":"","date":"2021-07-12T14:50:29","subject":"Re: [libcamera-devel] [RFC PATCH v3 05/16] controls: Replace\n\tAeLocked with AeState, and add AeLock","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nOn Sun, 11 Jul 2021 at 23:37, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> (now really CC'ing David)\n>\n> Apologies for the late reply, I should have shared more information on\n> this topic earlier.\n>\n> On Mon, Jul 05, 2021 at 06:34:07PM +0100, Naushir Patuck wrote:\n> > On Fri, 2 Jul 2021 at 11:38, Paul Elder wrote:\n> > > AeLocked alone isn't sufficient for reporting the AE state, so replace\n> > > it with AeState. Add an AeLock control for instructing the camera to\n> > > lock the AE values.\n> > >\n> > > Update the current users of AeLocked accordingly.\n> > >\n> > > Signed-off-by: Paul Elder \n> > >\n> > > ---\n> > > No change in v3\n> > > ---\n> > > src/ipa/raspberrypi/raspberrypi.cpp | 5 +-\n> > > src/ipa/rkisp1/rkisp1.cpp | 13 ++--\n> > > src/libcamera/control_ids.yaml | 96 ++++++++++++++++++-----------\n> > > 3 files changed, 71 insertions(+), 43 deletions(-)\n> > >\n> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > index 4d09a84f..4981aa29 100644\n> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > @@ -469,7 +469,10 @@ void IPARPi::reportMetadata()\n> > >\n> > > AgcStatus *agcStatus =\n> rpiMetadata_.GetLocked(\"agc.status\");\n> > > if (agcStatus) {\n> > > - libcameraMetadata_.set(controls::AeLocked,\n> agcStatus->locked);\n> > > + libcameraMetadata_.set(controls::AeState,\n> > > + agcStatus->locked ?\n> > > + controls::AeStateLocked :\n> > > + controls::AeStateSearching);\n> >\n> > Yes, this seems like the correct state mapping.\n>\n> Is it ? I thought it should be AeStateConverged.\n>\n> > > libcameraMetadata_.set(controls::DigitalGain,\n> agcStatus->digital_gain);\n> > > }\n> > >\n> > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > index cdfb4d13..4eca26e2 100644\n> > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > @@ -51,7 +51,7 @@ private:\n> > > const rkisp1_stat_buffer *stats);\n> > >\n> > > void setControls(unsigned int frame);\n> > > - void metadataReady(unsigned int frame, unsigned int aeState);\n> > > + void metadataReady(unsigned int frame, int aeState);\n> > >\n> > > std::map buffers_;\n> > > std::map buffersMemory_;\n> > > @@ -227,7 +227,7 @@ void IPARkISP1::updateStatistics(unsigned int\n> frame,\n> > > const rkisp1_stat_buffer *stats)\n> > > {\n> > > const rkisp1_cif_isp_stat *params = &stats->params;\n> > > - unsigned int aeState = 0;\n> > > + int aeState = controls::AeStateInactive;\n> > >\n> > > if (stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP) {\n> > > const rkisp1_cif_isp_ae_stat *ae = ¶ms->ae;\n> > > @@ -262,7 +262,9 @@ void IPARkISP1::updateStatistics(unsigned int\n> frame,\n> > > setControls(frame + 1);\n> > > }\n> > >\n> > > - aeState = fabs(factor - 1.0f) < 0.05f ? 2 : 1;\n> > > + aeState = fabs(factor - 1.0f) < 0.05f ?\n> > > + controls::AeStateConverged :\n> > > + controls::AeStateSearching;\n> > > }\n> > >\n> > > metadataReady(frame, aeState);\n> > > @@ -281,12 +283,11 @@ void IPARkISP1::setControls(unsigned int frame)\n> > > queueFrameAction.emit(frame, op);\n> > > }\n> > >\n> > > -void IPARkISP1::metadataReady(unsigned int frame, unsigned int\n> aeState)\n> > > +void IPARkISP1::metadataReady(unsigned int frame, int aeState)\n> > > {\n> > > ControlList ctrls(controls::controls);\n> > >\n> > > - if (aeState)\n> > > - ctrls.set(controls::AeLocked, aeState == 2);\n> > > + ctrls.set(controls::AeState, aeState);\n> > >\n> > > RkISP1Action op;\n> > > op.op = ActionMetadata;\n> > > diff --git a/src/libcamera/control_ids.yaml\n> b/src/libcamera/control_ids.yaml\n> > > index 9d4638ae..5717bc1f 100644\n> > > --- a/src/libcamera/control_ids.yaml\n> > > +++ b/src/libcamera/control_ids.yaml\n> > > @@ -14,16 +14,70 @@ controls:\n> > >\n> > > \\sa ExposureTime AnalogueGain\n> > >\n> > > - - AeLocked:\n> > > + - AeLock:\n> > > type: bool\n> > > description: |\n> > > - Report the lock status of a running AE algorithm.\n> > > + Control to lock the AE values.\n> > > + When set to true, the AE algorithm is locked to its latest\n> parameters,\n> > > + and will not change exposure settings until set to false.\n> > > + \\sa AeState\n> >\n> > I know I am repeating myself here, but I don't see the need to\n> differentiate between\n> > AeLock and AeEnable == falses.\n>\n> Please see below.\n>\n> > > - If the AE algorithm is locked the value shall be set to true,\n> if it's\n> > > - converging it shall be set to false. If the AE algorithm is\n> not\n> > > - running the control shall not be present in the metadata\n> control list.\n> > > + - AeState:\n> > > + type: int32_t\n> > > + description: |\n> > > + Control to report the current AE algorithm state. Enabling or\n> disabling\n> > > + AE (AeEnable) always resets the AeState to AeStateInactive.\n> The camera\n> > > + device can do several state transitions between two results,\n> if it is\n> > > + allowed by the state transition table. For example,\n> AeStateInactive may\n> > > + never actually be seen in a result.\n> > >\n> > > - \\sa AeEnable\n> > > + The state in the result is the state for this image (in sync\n> with this\n> > > + image). If AE state becomes AeStateConverged, then the image\n> data\n> > > + associated with the result should be good to use.\n> > > +\n> > > + The state transitions mentioned below assume that AeEnable is\n> on.\n> > > +\n> > > + \\sa AeLock\n> > > + enum:\n> > > + - name: AeStateInactive\n> > > + value: 0\n> > > + description: |\n> > > + The AE algorithm is inactive.\n> > > + If the camera initiates an AE scan, the state shall go to\n> Searching.\n> > > + If AeLock is on, the state shall go to Locked.\n> > >\n> >\n> > This state is also a bit ambiguous to me. Is this the state that must\n> be reported\n> > if AeEnable == false? If so, is that not also conveyed by setting the\n> latter?\n> >\n> > If the objective of the change is to wholesale do what Android does,\n> then I don't\n> > really have any objection to this change as-is, and we can treat the\n> above\n> > states as best we can in the Raspberry Pi IPA. Otherwise, I do see some\n> scope\n> > to simplify some of this.\n>\n> First of all, let me clarify the goal. The libcamera native API needs to\n> offer the features required to implement the Android camera HAL API, but\n> we don't need to just duplicate it. When there are reasons to depart\n> (for instance to fix historical mistakes made in Android, to provide\n> additional features, or just to expose an API we consider more\n> consistent), we should do so.\n>\n> Then, a word about the process. Implementing support for an Android\n> control requires plumbing it through the camera stack: definition of a\n> libcamera control, translation between the Android and libcamera control\n> in the Android camera HAL, implementation of the libcamera control in\n> the pipeline handlesr, and, when applicable, in the IPAs. Defining\n> libcamera controls, as any API design effort, takes time. To allow\n> moving forward on all the other parts, we have a fast-track process to\n> define draft controls that mimick the Android controls. All the draft\n> controls will be redesigned before freezing the libcamera public API.\n>\n> This means that AeLock and AeState should be marked as draft controls as\n> part of this patch.\n>\n> This being said, we don't need to define controls as draft controls if\n> we want to already go through a complete design process. The discussion\n> that stemmed from this patch series goes in that direction.\n>\n> It's useful, in this context, to understand why the Android AE (and AWB)\n> controls have been designed this way. Conceptually speaking, the\n> algorithms are considered, in Android, to be one level above manual\n> controls.\n>\n> Controls Metadata\n> /---------- == OFF ---------\\\n> | v\n> aeMode -/ +------+ |\\\n> aeLock --> | AE | --> |0\\ +--------+\n> aeExposureCompensation +------+ | | --> | Sensor | --> exposureTime\n> | | +--------+ sensitivity\n> exposureTime -----------------------> |1/\n> sensitivity |/\n>\n> (The AE controls are in the adnroid.control class, while the sensor\n> manual control are in the android.sensor class.)\n>\n> The aeMode control selects whether the AE algorithm (when != OFF) or the\n> manual controls (when == OFF) drive the sensor. The aeLock control tells\n> the AE algorithm to freeze its output (when ON) or operate normally\n> (when OFF). The metadata always report the values applied to the sensor,\n> regardless of whether they come from the AE algorithm or from manual\n> controls.\n>\n> One important difference, compared to the model we currently implement,\n> is that the Android AE algorithm is independent from the manual sensor\n> controls. The values of the manual controls aren't affected by the AE\n> algorithm, and when AE is disabled, the last set manual control values\n> are applied. This thus require an aeLock control to transition\n> seamlessly between AE and manual mode, as the application needs to set\n> the manual controls to the values applied to the sensor on the last\n> frame that AE was on, which couldn't otherwise be done due to the\n> internal delays.\n>\n\nThank you for the detailed explanation here. I can now better understand\nthe Android AE state transitions and why they may be needed.\n\n\n>\n> The existing libcamera AE controls are ill-defined. There's a \\todo\n> comment to document the interactions between AeEnable and setting a\n> fixed value for ExposureTime or AnalogueGain. I'm actually not sure what\n> happens if AeEnable is set to false when ExposureTime and/or\n> AnalogueGain are 0, or when AeEnable is set to true when ExposureTime\n> and/or AnalogueGain are not 0.\n>\n\nTo provide some brief context on the Raspberry Pi AGC, we never really\ngo into a \"disabled\" state at all. This is because the AGC algorithm always\nprovides the shutter/gain values to pass to the sensor, even in manual mode\n(more on this below). We use an internal Paused state to lock the\nshutter/gain\nvalues so that they can never change, which essentially mimics a \"disabled\"\nstate.\n\nFor manual operation, we pass shutter/gain ctrl values (either or both) to\nthe AGC,\nand that is what the AGC requests off the sensor. This happens regardless\nof\nwhether the AGC is in it's internal paused state or not. If the AGC is not\nin\npaused state, and we only provide one manual control value (e.g. shutter\nspeed),\nthen the AGC will vary the gain as it sees fit to converge to the desired\nbrightness. Setting a manual shutter/gain ctrl value back to 0 hands back\ncontrol\nof the control to the AGC and it starts adapting it as necessary to\nconverge to\nthe desired brightness.\n\nThis type of implementation does allow us to essentially do all that\nAndroid would\nexcept of the AGC, but does also allow us the flexibility to do\nAperture/Shutter/Gain\npriority modes if needed.\n\n\n> We need a formal model for how libcamera algorithms interact with manual\n> controls. It doesn't have to duplicate the Android model, but it has to\n> offer at least the same features. I believe we need to take it one step\n> further, as we want to support exposure priority and aperture priority.\n> I think we should actually talk about gain/sensitivity priority instead\n> of aperture priority for the existing feature, and already prepare for\n> devices that have a controllable lens aperture.\n>\n\n> Now, the hard question: how should libcamera model this ? :-) Naush,\n> David, your feedback from a RPi point of view would be very appreciated.\n>\n>\nIt would be easy for me to say do it the Raspberry Pi way :-)\nBut in reality, I don't think the Android AE model gains us an advantage\nover our existing\nmodel. And having to track one less state (locked vs disabled) is always a\ngood thing.\n\nExplicitly defining AeModes to have (amongst others) ShutterPrioiry,\nGainPrioiry,\nAperturePriority and Auto modes would be useful though. This avoids the\nmessiness of\nsetting 0 to hand back manual control back to the AGC?\n\nI'm sure David will also want to share his opinions on this, but he won't\nbe able to\nrespond for a few days.\n\nRegards,\nNaush\n\n\n> Please also let me know if the above explanation is clear or if you need\n> more information.\n\n> > + - name: AeStateSearching\n> > > + value: 1\n> > > + description: |\n> > > + The AE algorithm has not converged yet.\n> > > + If the camera finishes an AE scan, the state shall go to\n> Converged.\n> > > + If the camera finishes an AE scan, but flash is required,\n> the state\n> > > + shall go to FlashRequired.\n> > > + If AeLock is on, the state shall go to Locked.\n> > > + - name: AeStateConverged\n> > > + value: 2\n> > > + description: |\n> > > + The AE algorithm has converged.\n> > > + If the camera initiates an AE scan, the state shall go to\n> Searching.\n> > > + If AeLock is on, the state shall go to Locked.\n> > > + - name: AeStateLocked\n> > > + value: 3\n> > > + description: |\n> > > + The AE algorithm is locked.\n> > > + If AeLock is off, the state can go to Searching,\n> Converged, or\n> > > + FlashRequired.\n> > > + - name: AeStateFlashRequired\n> > > + value: 4\n> > > + description: |\n> > > + The AE algorithm would need a flash for good results\n> > > + If the camera initiates an AE scan, the state shall go to\n> Searching.\n> > > + If AeLock is on, the state shall go to Locked.\n> > > + - name: AeStatePrecapture\n> > > + value: 5\n> > > + description: |\n> > > + The AE algorithm has started a pre-capture metering\n> session.\n> > > + After the sequence is finished, the state shall go to\n> Converged if\n> > > + AeLock is off, and Locked if it is on.\n> > > + \\sa AePrecaptureTrigger\n> > >\n> > > # AeMeteringMode needs further attention:\n> > > # - Auto-generate max enum value.\n> > > @@ -477,36 +531,6 @@ controls:\n> > > High quality aberration correction which might reduce the\n> frame\n> > > rate.\n> > >\n> > > - - AeState:\n> > > - type: int32_t\n> > > - draft: true\n> > > - description: |\n> > > - Control to report the current AE algorithm state. Currently\n> identical to\n> > > - ANDROID_CONTROL_AE_STATE.\n> > > -\n> > > - Current state of the AE algorithm.\n> > > - enum:\n> > > - - name: AeStateInactive\n> > > - value: 0\n> > > - description: The AE algorithm is inactive.\n> > > - - name: AeStateSearching\n> > > - value: 1\n> > > - description: The AE algorithm has not converged yet.\n> > > - - name: AeStateConverged\n> > > - value: 2\n> > > - description: The AE algorithm has converged.\n> > > - - name: AeStateLocked\n> > > - value: 3\n> > > - description: The AE algorithm is locked.\n> > > - - name: AeStateFlashRequired\n> > > - value: 4\n> > > - description: The AE algorithm would need a flash for good\n> results\n> > > - - name: AeStatePrecapture\n> > > - value: 5\n> > > - description: |\n> > > - The AE algorithm has started a pre-capture metering\n> session.\n> > > - \\sa AePrecaptureTrigger\n> > > -\n> > > - AfState:\n> > > type: int32_t\n> > > draft: true\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 195E9C3226\n\tfor ;\n\tMon, 12 Jul 2021 14:50:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8976068526;\n\tMon, 12 Jul 2021 16:50:47 +0200 (CEST)","from mail-lf1-x136.google.com (mail-lf1-x136.google.com\n\t[IPv6:2a00:1450:4864:20::136])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B46AC68513\n\tfor ;\n\tMon, 12 Jul 2021 16:50:45 +0200 (CEST)","by mail-lf1-x136.google.com with SMTP id t17so43887439lfq.0\n\tfor ;\n\tMon, 12 Jul 2021 07:50:45 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"UFJixe1R\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=ATeC54Of2XkmFbmlslIR38AYUWT0bq9iHnoPYX3UVUg=;\n\tb=UFJixe1RdeyGYvuWFpXkJhThFdGynb5K2aJ5ygsK3BvfbrWBPrWP2P2VdptRNGi/t5\n\tY5fnde4PdFyY7LG/l5UyBJyySESfBHN2aapVI3tcqgXirbricy9f+X1u7bplaMBwxNzb\n\tqnR3wcG3wke2AahhZwfKtSkCdUYFsqBZcYboPO8MWeMEk3yIOkmkJCmmZ9+hiVFnLf4X\n\tKvSPL7Ah3M1G8NvPlRDYHJVPPnXA1F2VQaazG5muxHhquzeoZ6H/SEaZROHZd/PCpJqL\n\trJHIQ3PK4+gqY3GXOWKg86nQDyXkZub+IMnT49qW4UZOl00CZYBmmke4be2iSQiLD2N4\n\t8O5w==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=ATeC54Of2XkmFbmlslIR38AYUWT0bq9iHnoPYX3UVUg=;\n\tb=EscUGpw9BEEXXp8qczs910q2+Ah4ZtWPlShoinPQIr5+z5kw4LkvwLqJNG/+ALDpCP\n\t7cL37ZyNvQqVp+SzF5esLNkvlk7TqTSylSEC2x++a3IXv7LXTsg3iXHU+GxLDlbURicH\n\t/B/wpewn8HwYyElOZdH4+KQkhJo3NGO7WvtFcPhaPDScyy8HtmISmyKi880w41VTyJix\n\tu2EVTIWMlVQGrDGaosK7wT3BewHEAgCZVcwKdFfSA7MWB7bk///u3PPAHitDrC9b8LxJ\n\t85iAlYvILS+SRvHw3H/C1i7DqjR+Ip/V5spLeGpzJvMjPS3NTof3rvk1ss0aCtsB3wBZ\n\t1f9A==","X-Gm-Message-State":"AOAM531wVV/GuCWwDerXX+/P9Ry+Vw5NcKiSp9bHT+eQkbD+9bBaX0TV\n\t5ph6vAiNRjy6cYcBtUbYimPmznrMnypHfmBDpyfbAw0SmME7pA==","X-Google-Smtp-Source":"ABdhPJxVe/EMrky/sMx4juSLLMUrV2+n0o6e0IxWi3at/EdGE9CW0Ng3rNmRKbEZleIOcf9B4zpeAt0Vl2Nx4xsHWHU=","X-Received":"by 2002:a19:4f09:: with SMTP id d9mr9404743lfb.567.1626101444981;\n\tMon, 12 Jul 2021 07:50:44 -0700 (PDT)","MIME-Version":"1.0","References":"<20210702103800.41291-1-paul.elder@ideasonboard.com>\n\t<20210702103800.41291-6-paul.elder@ideasonboard.com>\n\t\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Mon, 12 Jul 2021 15:50:29 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000ff491405c6ee3e9d\"","Subject":"Re: [libcamera-devel] [RFC PATCH v3 05/16] controls: Replace\n\tAeLocked with AeState, and add AeLock","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","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":18813,"web_url":"https://patchwork.libcamera.org/comment/18813/","msgid":"","date":"2021-08-16T02:07:42","subject":"Re: [libcamera-devel] [RFC PATCH v3 05/16] controls: Replace\n\tAeLocked with AeState, and add AeLock","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Mon, Jul 12, 2021 at 03:50:29PM +0100, Naushir Patuck wrote:\n> On Sun, 11 Jul 2021 at 23:37, Laurent Pinchart wrote:\n> > On Mon, Jul 05, 2021 at 06:34:07PM +0100, Naushir Patuck wrote:\n> > > On Fri, 2 Jul 2021 at 11:38, Paul Elder wrote:\n> > > > AeLocked alone isn't sufficient for reporting the AE state, so replace\n> > > > it with AeState. Add an AeLock control for instructing the camera to\n> > > > lock the AE values.\n> > > >\n> > > > Update the current users of AeLocked accordingly.\n> > > >\n> > > > Signed-off-by: Paul Elder \n> > > >\n> > > > ---\n> > > > No change in v3\n> > > > ---\n> > > > src/ipa/raspberrypi/raspberrypi.cpp | 5 +-\n> > > > src/ipa/rkisp1/rkisp1.cpp | 13 ++--\n> > > > src/libcamera/control_ids.yaml | 96 ++++++++++++++++++-----------\n> > > > 3 files changed, 71 insertions(+), 43 deletions(-)\n> > > >\n> > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > index 4d09a84f..4981aa29 100644\n> > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > @@ -469,7 +469,10 @@ void IPARPi::reportMetadata()\n> > > >\n> > > > AgcStatus *agcStatus = rpiMetadata_.GetLocked(\"agc.status\");\n> > > > if (agcStatus) {\n> > > > - libcameraMetadata_.set(controls::AeLocked, agcStatus->locked);\n> > > > + libcameraMetadata_.set(controls::AeState,\n> > > > + agcStatus->locked ?\n> > > > + controls::AeStateLocked :\n> > > > + controls::AeStateSearching);\n> > >\n> > > Yes, this seems like the correct state mapping.\n> >\n> > Is it ? I thought it should be AeStateConverged.\n> >\n> > > > libcameraMetadata_.set(controls::DigitalGain, agcStatus->digital_gain);\n> > > > }\n> > > >\n> > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > > index cdfb4d13..4eca26e2 100644\n> > > > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > > @@ -51,7 +51,7 @@ private:\n> > > > const rkisp1_stat_buffer *stats);\n> > > >\n> > > > void setControls(unsigned int frame);\n> > > > - void metadataReady(unsigned int frame, unsigned int aeState);\n> > > > + void metadataReady(unsigned int frame, int aeState);\n> > > >\n> > > > std::map buffers_;\n> > > > std::map buffersMemory_;\n> > > > @@ -227,7 +227,7 @@ void IPARkISP1::updateStatistics(unsigned int frame,\n> > > > const rkisp1_stat_buffer *stats)\n> > > > {\n> > > > const rkisp1_cif_isp_stat *params = &stats->params;\n> > > > - unsigned int aeState = 0;\n> > > > + int aeState = controls::AeStateInactive;\n> > > >\n> > > > if (stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP) {\n> > > > const rkisp1_cif_isp_ae_stat *ae = ¶ms->ae;\n> > > > @@ -262,7 +262,9 @@ void IPARkISP1::updateStatistics(unsigned int frame,\n> > > > setControls(frame + 1);\n> > > > }\n> > > >\n> > > > - aeState = fabs(factor - 1.0f) < 0.05f ? 2 : 1;\n> > > > + aeState = fabs(factor - 1.0f) < 0.05f ?\n> > > > + controls::AeStateConverged :\n> > > > + controls::AeStateSearching;\n> > > > }\n> > > >\n> > > > metadataReady(frame, aeState);\n> > > > @@ -281,12 +283,11 @@ void IPARkISP1::setControls(unsigned int frame)\n> > > > queueFrameAction.emit(frame, op);\n> > > > }\n> > > >\n> > > > -void IPARkISP1::metadataReady(unsigned int frame, unsigned int aeState)\n> > > > +void IPARkISP1::metadataReady(unsigned int frame, int aeState)\n> > > > {\n> > > > ControlList ctrls(controls::controls);\n> > > >\n> > > > - if (aeState)\n> > > > - ctrls.set(controls::AeLocked, aeState == 2);\n> > > > + ctrls.set(controls::AeState, aeState);\n> > > >\n> > > > RkISP1Action op;\n> > > > op.op = ActionMetadata;\n> > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > > > index 9d4638ae..5717bc1f 100644\n> > > > --- a/src/libcamera/control_ids.yaml\n> > > > +++ b/src/libcamera/control_ids.yaml\n> > > > @@ -14,16 +14,70 @@ controls:\n> > > >\n> > > > \\sa ExposureTime AnalogueGain\n> > > >\n> > > > - - AeLocked:\n> > > > + - AeLock:\n> > > > type: bool\n> > > > description: |\n> > > > - Report the lock status of a running AE algorithm.\n> > > > + Control to lock the AE values.\n> > > > + When set to true, the AE algorithm is locked to its latest parameters,\n> > > > + and will not change exposure settings until set to false.\n> > > > + \\sa AeState\n> > >\n> > > I know I am repeating myself here, but I don't see the need to differentiate between\n> > > AeLock and AeEnable == falses.\n> >\n> > Please see below.\n> >\n> > > > - If the AE algorithm is locked the value shall be set to true, if it's\n> > > > - converging it shall be set to false. If the AE algorithm is not\n> > > > - running the control shall not be present in the metadata control list.\n> > > > + - AeState:\n> > > > + type: int32_t\n> > > > + description: |\n> > > > + Control to report the current AE algorithm state. Enabling or disabling\n> > > > + AE (AeEnable) always resets the AeState to AeStateInactive. The camera\n> > > > + device can do several state transitions between two results, if it is\n> > > > + allowed by the state transition table. For example, AeStateInactive may\n> > > > + never actually be seen in a result.\n> > > >\n> > > > - \\sa AeEnable\n> > > > + The state in the result is the state for this image (in sync with this\n> > > > + image). If AE state becomes AeStateConverged, then the image data\n> > > > + associated with the result should be good to use.\n> > > > +\n> > > > + The state transitions mentioned below assume that AeEnable is on.\n> > > > +\n> > > > + \\sa AeLock\n> > > > + enum:\n> > > > + - name: AeStateInactive\n> > > > + value: 0\n> > > > + description: |\n> > > > + The AE algorithm is inactive.\n> > > > + If the camera initiates an AE scan, the state shall go to Searching.\n> > > > + If AeLock is on, the state shall go to Locked.\n> > > >\n> > >\n> > > This state is also a bit ambiguous to me. Is this the state that must be reported\n> > > if AeEnable == false? If so, is that not also conveyed by setting the latter?\n> > >\n> > > If the objective of the change is to wholesale do what Android does, then I don't\n> > > really have any objection to this change as-is, and we can treat the above\n> > > states as best we can in the Raspberry Pi IPA. Otherwise, I do see some scope\n> > > to simplify some of this.\n> >\n> > First of all, let me clarify the goal. The libcamera native API needs to\n> > offer the features required to implement the Android camera HAL API, but\n> > we don't need to just duplicate it. When there are reasons to depart\n> > (for instance to fix historical mistakes made in Android, to provide\n> > additional features, or just to expose an API we consider more\n> > consistent), we should do so.\n> >\n> > Then, a word about the process. Implementing support for an Android\n> > control requires plumbing it through the camera stack: definition of a\n> > libcamera control, translation between the Android and libcamera control\n> > in the Android camera HAL, implementation of the libcamera control in\n> > the pipeline handlesr, and, when applicable, in the IPAs. Defining\n> > libcamera controls, as any API design effort, takes time. To allow\n> > moving forward on all the other parts, we have a fast-track process to\n> > define draft controls that mimick the Android controls. All the draft\n> > controls will be redesigned before freezing the libcamera public API.\n> >\n> > This means that AeLock and AeState should be marked as draft controls as\n> > part of this patch.\n> >\n> > This being said, we don't need to define controls as draft controls if\n> > we want to already go through a complete design process. The discussion\n> > that stemmed from this patch series goes in that direction.\n> >\n> > It's useful, in this context, to understand why the Android AE (and AWB)\n> > controls have been designed this way. Conceptually speaking, the\n> > algorithms are considered, in Android, to be one level above manual\n> > controls.\n> >\n> > Controls Metadata\n> > /---------- == OFF ---------\\\n> > | v\n> > aeMode -/ +------+ |\\\n> > aeLock --> | AE | --> |0\\ +--------+\n> > aeExposureCompensation +------+ | | --> | Sensor | --> exposureTime\n> > | | +--------+ sensitivity\n> > exposureTime -----------------------> |1/\n> > sensitivity |/\n> >\n> > (The AE controls are in the adnroid.control class, while the sensor\n> > manual control are in the android.sensor class.)\n> >\n> > The aeMode control selects whether the AE algorithm (when != OFF) or the\n> > manual controls (when == OFF) drive the sensor. The aeLock control tells\n> > the AE algorithm to freeze its output (when ON) or operate normally\n> > (when OFF). The metadata always report the values applied to the sensor,\n> > regardless of whether they come from the AE algorithm or from manual\n> > controls.\n> >\n> > One important difference, compared to the model we currently implement,\n> > is that the Android AE algorithm is independent from the manual sensor\n> > controls. The values of the manual controls aren't affected by the AE\n> > algorithm, and when AE is disabled, the last set manual control values\n> > are applied. This thus require an aeLock control to transition\n> > seamlessly between AE and manual mode, as the application needs to set\n> > the manual controls to the values applied to the sensor on the last\n> > frame that AE was on, which couldn't otherwise be done due to the\n> > internal delays.\n> \n> Thank you for the detailed explanation here. I can now better understand\n> the Android AE state transitions and why they may be needed.\n> \n> > The existing libcamera AE controls are ill-defined. There's a \\todo\n> > comment to document the interactions between AeEnable and setting a\n> > fixed value for ExposureTime or AnalogueGain. I'm actually not sure what\n> > happens if AeEnable is set to false when ExposureTime and/or\n> > AnalogueGain are 0, or when AeEnable is set to true when ExposureTime\n> > and/or AnalogueGain are not 0.\n> \n> To provide some brief context on the Raspberry Pi AGC, we never really\n> go into a \"disabled\" state at all. This is because the AGC algorithm always\n> provides the shutter/gain values to pass to the sensor, even in manual mode\n> (more on this below). We use an internal Paused state to lock the shutter/gain\n> values so that they can never change, which essentially mimics a \"disabled\"\n> state.\n> \n> For manual operation, we pass shutter/gain ctrl values (either or both) to the AGC,\n> and that is what the AGC requests off the sensor. This happens regardless of\n> whether the AGC is in it's internal paused state or not. If the AGC is not in\n> paused state, and we only provide one manual control value (e.g. shutter speed),\n> then the AGC will vary the gain as it sees fit to converge to the desired\n> brightness. Setting a manual shutter/gain ctrl value back to 0 hands back control\n> of the control to the AGC and it starts adapting it as necessary to converge to\n> the desired brightness.\n> \n> This type of implementation does allow us to essentially do all that Android would\n> except of the AGC, but does also allow us the flexibility to do Aperture/Shutter/Gain\n> priority modes if needed.\n\nIf I understand correctly, when the AeEnable control is set to false,\nthe AGC algorithm is paused, which just means that it sets the fixed\nshutter and gain to the last computed values. Those are the values that\nwill then be applied to the sensor, until either the AeEnable control is\nset back to true, or the manual gain or exposure time control is set to\na different value.\n\nThis leads us to the first interesting behaviour: if the application\nkeeps queuing request that contain the AeEnable control set to false, as\nwell as manual exposure and/or gain values, in order to apply the manual\nvalues as expected, the Agc::SetFixedShutter() and\nAgc::SetFixedAnalogueGain() functions copy the fixed values to the\nstatus_ structure, so that pausing the algorithm won't override them.\nThere's a bit of fragility here that I'm not sure to like, as it seems\nfairly error-prone (not in the existing Raspberry Pi IPA implementation,\nbut from a general IPA module development point of view).\n\nAnother interesting side effect is what happens if the application then\nsets the exposure time and/or gain to 0 while keeping AeEnable to false.\nThe AGC algorithm will resume operation in that case. We can of course\nargue that this is a corner case and that applications shouldn't do\nthis, but it's in any case something that needs to be documented.\n\nLooking at how to switch from auto to manual exposure seamlessly, an\napplication would need to go through the following sequence:\n\n- Set AeEnable to false in a request, without setting a manual gain or\n exposure.\n- Wait until metadata reports AeEnable set to false (this isn't\n currently implemented in the Raspberry Pi IPA), to know that the\n setting has taken effect.\n- Take the reported gain and exposure time from the request that\n reported AeEnable set to false, and use them as starting values for\n manual control of the AGC.\n\nOn Android, the equivalent sequence is documented in\nhttps://ideasonboard.org/android/docs.html#controls_android.control.aeLock.\nThe sequences are quite similar, the main difference being that when\nAndroid calls aeLock is called AeEnable in libcamera. This causes\nanother issue: the AeEnable control needs to be reported in metadata,\nbut its value doesn't actually indicate whether the gain and exposure\ntime are controlled automatically or not. If an application sets\nAeEnable to true but also sets manual gain and exposure time, metadata\nwill report AeEnable to true, but the AGC will be manually controlled.\nThis will also be difficult to document in a clear way I think.\n\nAnother point that bothers me is that treating 0 as a magic value to\nmean automatic control prevents from reporting a minimum value for the\nexposure time and gain to applications. The Raspberry Pi IPA report the\nAnalogueGain control as having a 1.0 minimum value, but when we'll\nenforce bound checking in the libcamera core (as this really shouldn't\nbe duplicated by all IPAs), this will break.\n\n> > We need a formal model for how libcamera algorithms interact with manual\n> > controls. It doesn't have to duplicate the Android model, but it has to\n> > offer at least the same features. I believe we need to take it one step\n> > further, as we want to support exposure priority and aperture priority.\n> > I think we should actually talk about gain/sensitivity priority instead\n> > of aperture priority for the existing feature, and already prepare for\n> > devices that have a controllable lens aperture.\n> >\n> > Now, the hard question: how should libcamera model this ? :-) Naush,\n> > David, your feedback from a RPi point of view would be very appreciated.\n>\n> It would be easy for me to say do it the Raspberry Pi way :-)\n\nI have no ideological opposition to that. What bothers me at the moment\nis that the interactions between the controls related to AGC are not\nwell defined in the documentation, and based on the comments above, I\nthink it will be difficult to do so in a clear way, and avoid\nerror-prone behaviours. There's also the bound checking issue for the\ngain (and exposure time, the lower limit happens to be 0 for the\nRaspberry Pi IPA, but that won't be the case universally) that seems\ndifficult to fix.\n\nI fear that documenting the behaviour of the existing controls will be\ndifficult, and will result in unclear documentation. If we want to keep\nthe existing behaviour, I'd like to see documentation that shows that my\nfears are unfounded.\n\nThis being said, I'm not advocating for replicating the Android\ncontrols, as, as discussed previously, they don't support implementing\nshutter or gain priority, which is an important feature. We may however\ndecide to design a different semantics for the AE controls that would\nsupport all our use cases and result in simpler documentation.\n\n> But in reality, I don't think the Android AE model gains us an advantage over our existing\n> model. And having to track one less state (locked vs disabled) is always a good thing.\n> \n> Explicitly defining AeModes to have (amongst others) ShutterPrioiry, GainPrioiry,\n> AperturePriority and Auto modes would be useful though. This avoids the messiness of\n> setting 0 to hand back manual control back to the AGC?\n\nOnce we'll get more than two parameters to control (adding aperture to\nshutter and gain), I think this would become a bit messy, as there will\nbe 8 combinations of what can be controlled manually vs. automatically.\n\n> I'm sure David will also want to share his opinions on this, but he won't be able to\n> respond for a few days.\n\nIf David has time to share his opinion at some point, that would be\nhelpful.\n\n> > Please also let me know if the above explanation is clear or if you need\n> > more information.\n> >\n> > > + - name: AeStateSearching\n> > > > + value: 1\n> > > > + description: |\n> > > > + The AE algorithm has not converged yet.\n> > > > + If the camera finishes an AE scan, the state shall go to Converged.\n> > > > + If the camera finishes an AE scan, but flash is required, the state\n> > > > + shall go to FlashRequired.\n> > > > + If AeLock is on, the state shall go to Locked.\n> > > > + - name: AeStateConverged\n> > > > + value: 2\n> > > > + description: |\n> > > > + The AE algorithm has converged.\n> > > > + If the camera initiates an AE scan, the state shall go to Searching.\n> > > > + If AeLock is on, the state shall go to Locked.\n> > > > + - name: AeStateLocked\n> > > > + value: 3\n> > > > + description: |\n> > > > + The AE algorithm is locked.\n> > > > + If AeLock is off, the state can go to Searching, Converged, or\n> > > > + FlashRequired.\n> > > > + - name: AeStateFlashRequired\n> > > > + value: 4\n> > > > + description: |\n> > > > + The AE algorithm would need a flash for good results\n> > > > + If the camera initiates an AE scan, the state shall go to Searching.\n> > > > + If AeLock is on, the state shall go to Locked.\n> > > > + - name: AeStatePrecapture\n> > > > + value: 5\n> > > > + description: |\n> > > > + The AE algorithm has started a pre-capture metering session.\n> > > > + After the sequence is finished, the state shall go to Converged if\n> > > > + AeLock is off, and Locked if it is on.\n> > > > + \\sa AePrecaptureTrigger\n> > > >\n> > > > # AeMeteringMode needs further attention:\n> > > > # - Auto-generate max enum value.\n> > > > @@ -477,36 +531,6 @@ controls:\n> > > > High quality aberration correction which might reduce the frame\n> > > > rate.\n> > > >\n> > > > - - AeState:\n> > > > - type: int32_t\n> > > > - draft: true\n> > > > - description: |\n> > > > - Control to report the current AE algorithm state. Currently identical to\n> > > > - ANDROID_CONTROL_AE_STATE.\n> > > > -\n> > > > - Current state of the AE algorithm.\n> > > > - enum:\n> > > > - - name: AeStateInactive\n> > > > - value: 0\n> > > > - description: The AE algorithm is inactive.\n> > > > - - name: AeStateSearching\n> > > > - value: 1\n> > > > - description: The AE algorithm has not converged yet.\n> > > > - - name: AeStateConverged\n> > > > - value: 2\n> > > > - description: The AE algorithm has converged.\n> > > > - - name: AeStateLocked\n> > > > - value: 3\n> > > > - description: The AE algorithm is locked.\n> > > > - - name: AeStateFlashRequired\n> > > > - value: 4\n> > > > - description: The AE algorithm would need a flash for good results\n> > > > - - name: AeStatePrecapture\n> > > > - value: 5\n> > > > - description: |\n> > > > - The AE algorithm has started a pre-capture metering session.\n> > > > - \\sa AePrecaptureTrigger\n> > > > -\n> > > > - AfState:\n> > > > type: int32_t\n> > > > draft: true","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 20FCBBD87C\n\tfor ;\n\tMon, 16 Aug 2021 02:07:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6325F68892;\n\tMon, 16 Aug 2021 04:07:50 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7B21360261\n\tfor ;\n\tMon, 16 Aug 2021 04:07:48 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E3EEE3E5;\n\tMon, 16 Aug 2021 04:07:47 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"tnwtL7MO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629079668;\n\tbh=8GpTpSENNeFz8/BQcQMTxQK3gwEXe13VWaQyTiLOBAQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tnwtL7MO7GZdAmjSuNNHF6HN9a1mG8qmBhu5Xx6RGwxci2pRA7E4VgnKeb/MBCHsk\n\tQU0WRewTMUdsO+ZDJrVvh+KtZ+1bZoOlDeTG8cEgghFJbGcpnt8DXRwoopBMgTDk/f\n\tmZF9A+Jtoot7K7zj+dMMnY0eIspPO3XS0bXoMBY4=","Date":"Mon, 16 Aug 2021 05:07:42 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210702103800.41291-1-paul.elder@ideasonboard.com>\n\t<20210702103800.41291-6-paul.elder@ideasonboard.com>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH v3 05/16] controls: Replace\n\tAeLocked with AeState, and add AeLock","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","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19027,"web_url":"https://patchwork.libcamera.org/comment/19027/","msgid":"","date":"2021-08-24T15:04:47","subject":"Re: [libcamera-devel] [RFC PATCH v3 05/16] controls: Replace\n\tAeLocked with AeState, and add AeLock","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Laurent\n\nSorry for the late reply, it seemed to get erroneously discarded by my\nhuman level spam filter (i.e. myself)! Apologies for that. I've also\ndeleted anything with more than three \"> > >\" indents, I was rather\ndrowning - hope that doesn't confuse anyone!\n\nAnyway, here are some of my thoughts on the matter. In the end I\ndecided to leave them at the top here, I'm not too sure how well they\nfit into all the text below.\n\n1. I like the idea of being able to fix some but not all of the\nAEC/AGC \"levers\" (e.g. gain but not exposure). Android seems not to\nallow this? But I think it's essential.\n\n2. We don't like unfixing these levers by writing a magic zero. There\nneeds to be another mechanism.\n\n3. In the RPi code, we fix specific levers by writing a fixed value.\nYet there's no way to fix a single lever at the current value, which\nis what happens to all the levers when we disable AEC/AGC. That feels\na bit inconsistent.\n\n4. I think point 1 above means that fixing all levers is a natural\nstate of the AEC/AGC algorithm, so there doesn't need to be any kind\nof completely separate manual mode. My personal view is therefore that\nlibcamera possibly shouldn't emulate the Android behaviour here.\n\nSo here's something that might work for everyone:\n\n- Every lever has its own enable, assuming the implementation can support it.\n\n- Disabling AEC/AGC globally is the same as disabling all the levers.\n\n- Enabling a specific lever is obviously the new version of writing a\nzero to it!\n\n- When you disable a lever, it gets fixed to its current value.\n\n- Or you can disable a lever by writing a specific value, which is\nobviously then the value it gets.\n\nSo I think that might cover all the use cases we have? I don't really\nsee the point of a separate \"AeLock\" control in libcamera itself, it\nsimply seems unnecessary (unless I've missed something). Nonetheless,\nthe Android view of the world can easily be implemented on top of\nthis, including the \"lock\" mechanism.\n\nI expect non-Android applications will prefer to disable levers\nwithout specifying a value so that you just pick up the current one.\nTo me this feels like the most usual situation.\n\nBut I'm always happy to be corrected or persuaded otherwise!\n\nThanks\n\nDavid\n\nOn Mon, 16 Aug 2021 at 03:07, Laurent Pinchart\n wrote:\n>\n> Hi Naush,\n>\n> On Mon, Jul 12, 2021 at 03:50:29PM +0100, Naushir Patuck wrote:\n> > On Sun, 11 Jul 2021 at 23:37, Laurent Pinchart wrote:\n> > > On Mon, Jul 05, 2021 at 06:34:07PM +0100, Naushir Patuck wrote:\n...\n> > >\n> > > Is it ? I thought it should be AeStateConverged.\n> > >\n...\n> > >\n> > > Please see below.\n> > >\n...\n> > >\n> > > First of all, let me clarify the goal. The libcamera native API needs to\n> > > offer the features required to implement the Android camera HAL API, but\n> > > we don't need to just duplicate it. When there are reasons to depart\n> > > (for instance to fix historical mistakes made in Android, to provide\n> > > additional features, or just to expose an API we consider more\n> > > consistent), we should do so.\n> > >\n> > > Then, a word about the process. Implementing support for an Android\n> > > control requires plumbing it through the camera stack: definition of a\n> > > libcamera control, translation between the Android and libcamera control\n> > > in the Android camera HAL, implementation of the libcamera control in\n> > > the pipeline handlesr, and, when applicable, in the IPAs. Defining\n> > > libcamera controls, as any API design effort, takes time. To allow\n> > > moving forward on all the other parts, we have a fast-track process to\n> > > define draft controls that mimick the Android controls. All the draft\n> > > controls will be redesigned before freezing the libcamera public API.\n> > >\n> > > This means that AeLock and AeState should be marked as draft controls as\n> > > part of this patch.\n> > >\n> > > This being said, we don't need to define controls as draft controls if\n> > > we want to already go through a complete design process. The discussion\n> > > that stemmed from this patch series goes in that direction.\n> > >\n> > > It's useful, in this context, to understand why the Android AE (and AWB)\n> > > controls have been designed this way. Conceptually speaking, the\n> > > algorithms are considered, in Android, to be one level above manual\n> > > controls.\n> > >\n> > > Controls Metadata\n> > > /---------- == OFF ---------\\\n> > > | v\n> > > aeMode -/ +------+ |\\\n> > > aeLock --> | AE | --> |0\\ +--------+\n> > > aeExposureCompensation +------+ | | --> | Sensor | --> exposureTime\n> > > | | +--------+ sensitivity\n> > > exposureTime -----------------------> |1/\n> > > sensitivity |/\n> > >\n> > > (The AE controls are in the adnroid.control class, while the sensor\n> > > manual control are in the android.sensor class.)\n> > >\n> > > The aeMode control selects whether the AE algorithm (when != OFF) or the\n> > > manual controls (when == OFF) drive the sensor. The aeLock control tells\n> > > the AE algorithm to freeze its output (when ON) or operate normally\n> > > (when OFF). The metadata always report the values applied to the sensor,\n> > > regardless of whether they come from the AE algorithm or from manual\n> > > controls.\n> > >\n> > > One important difference, compared to the model we currently implement,\n> > > is that the Android AE algorithm is independent from the manual sensor\n> > > controls. The values of the manual controls aren't affected by the AE\n> > > algorithm, and when AE is disabled, the last set manual control values\n> > > are applied. This thus require an aeLock control to transition\n> > > seamlessly between AE and manual mode, as the application needs to set\n> > > the manual controls to the values applied to the sensor on the last\n> > > frame that AE was on, which couldn't otherwise be done due to the\n> > > internal delays.\n> >\n> > Thank you for the detailed explanation here. I can now better understand\n> > the Android AE state transitions and why they may be needed.\n> >\n> > > The existing libcamera AE controls are ill-defined. There's a \\todo\n> > > comment to document the interactions between AeEnable and setting a\n> > > fixed value for ExposureTime or AnalogueGain. I'm actually not sure what\n> > > happens if AeEnable is set to false when ExposureTime and/or\n> > > AnalogueGain are 0, or when AeEnable is set to true when ExposureTime\n> > > and/or AnalogueGain are not 0.\n> >\n> > To provide some brief context on the Raspberry Pi AGC, we never really\n> > go into a \"disabled\" state at all. This is because the AGC algorithm always\n> > provides the shutter/gain values to pass to the sensor, even in manual mode\n> > (more on this below). We use an internal Paused state to lock the shutter/gain\n> > values so that they can never change, which essentially mimics a \"disabled\"\n> > state.\n> >\n> > For manual operation, we pass shutter/gain ctrl values (either or both) to the AGC,\n> > and that is what the AGC requests off the sensor. This happens regardless of\n> > whether the AGC is in it's internal paused state or not. If the AGC is not in\n> > paused state, and we only provide one manual control value (e.g. shutter speed),\n> > then the AGC will vary the gain as it sees fit to converge to the desired\n> > brightness. Setting a manual shutter/gain ctrl value back to 0 hands back control\n> > of the control to the AGC and it starts adapting it as necessary to converge to\n> > the desired brightness.\n> >\n> > This type of implementation does allow us to essentially do all that Android would\n> > except of the AGC, but does also allow us the flexibility to do Aperture/Shutter/Gain\n> > priority modes if needed.\n>\n> If I understand correctly, when the AeEnable control is set to false,\n> the AGC algorithm is paused, which just means that it sets the fixed\n> shutter and gain to the last computed values. Those are the values that\n> will then be applied to the sensor, until either the AeEnable control is\n> set back to true, or the manual gain or exposure time control is set to\n> a different value.\n>\n> This leads us to the first interesting behaviour: if the application\n> keeps queuing request that contain the AeEnable control set to false, as\n> well as manual exposure and/or gain values, in order to apply the manual\n> values as expected, the Agc::SetFixedShutter() and\n> Agc::SetFixedAnalogueGain() functions copy the fixed values to the\n> status_ structure, so that pausing the algorithm won't override them.\n> There's a bit of fragility here that I'm not sure to like, as it seems\n> fairly error-prone (not in the existing Raspberry Pi IPA implementation,\n> but from a general IPA module development point of view).\n>\n> Another interesting side effect is what happens if the application then\n> sets the exposure time and/or gain to 0 while keeping AeEnable to false.\n> The AGC algorithm will resume operation in that case. We can of course\n> argue that this is a corner case and that applications shouldn't do\n> this, but it's in any case something that needs to be documented.\n>\n> Looking at how to switch from auto to manual exposure seamlessly, an\n> application would need to go through the following sequence:\n>\n> - Set AeEnable to false in a request, without setting a manual gain or\n> exposure.\n> - Wait until metadata reports AeEnable set to false (this isn't\n> currently implemented in the Raspberry Pi IPA), to know that the\n> setting has taken effect.\n> - Take the reported gain and exposure time from the request that\n> reported AeEnable set to false, and use them as starting values for\n> manual control of the AGC.\n>\n> On Android, the equivalent sequence is documented in\n> https://ideasonboard.org/android/docs.html#controls_android.control.aeLock.\n> The sequences are quite similar, the main difference being that when\n> Android calls aeLock is called AeEnable in libcamera. This causes\n> another issue: the AeEnable control needs to be reported in metadata,\n> but its value doesn't actually indicate whether the gain and exposure\n> time are controlled automatically or not. If an application sets\n> AeEnable to true but also sets manual gain and exposure time, metadata\n> will report AeEnable to true, but the AGC will be manually controlled.\n> This will also be difficult to document in a clear way I think.\n>\n> Another point that bothers me is that treating 0 as a magic value to\n> mean automatic control prevents from reporting a minimum value for the\n> exposure time and gain to applications. The Raspberry Pi IPA report the\n> AnalogueGain control as having a 1.0 minimum value, but when we'll\n> enforce bound checking in the libcamera core (as this really shouldn't\n> be duplicated by all IPAs), this will break.\n>\n> > > We need a formal model for how libcamera algorithms interact with manual\n> > > controls. It doesn't have to duplicate the Android model, but it has to\n> > > offer at least the same features. I believe we need to take it one step\n> > > further, as we want to support exposure priority and aperture priority.\n> > > I think we should actually talk about gain/sensitivity priority instead\n> > > of aperture priority for the existing feature, and already prepare for\n> > > devices that have a controllable lens aperture.\n> > >\n> > > Now, the hard question: how should libcamera model this ? :-) Naush,\n> > > David, your feedback from a RPi point of view would be very appreciated.\n> >\n> > It would be easy for me to say do it the Raspberry Pi way :-)\n>\n> I have no ideological opposition to that. What bothers me at the moment\n> is that the interactions between the controls related to AGC are not\n> well defined in the documentation, and based on the comments above, I\n> think it will be difficult to do so in a clear way, and avoid\n> error-prone behaviours. There's also the bound checking issue for the\n> gain (and exposure time, the lower limit happens to be 0 for the\n> Raspberry Pi IPA, but that won't be the case universally) that seems\n> difficult to fix.\n>\n> I fear that documenting the behaviour of the existing controls will be\n> difficult, and will result in unclear documentation. If we want to keep\n> the existing behaviour, I'd like to see documentation that shows that my\n> fears are unfounded.\n>\n> This being said, I'm not advocating for replicating the Android\n> controls, as, as discussed previously, they don't support implementing\n> shutter or gain priority, which is an important feature. We may however\n> decide to design a different semantics for the AE controls that would\n> support all our use cases and result in simpler documentation.\n>\n> > But in reality, I don't think the Android AE model gains us an advantage over our existing\n> > model. And having to track one less state (locked vs disabled) is always a good thing.\n> >\n> > Explicitly defining AeModes to have (amongst others) ShutterPrioiry, GainPrioiry,\n> > AperturePriority and Auto modes would be useful though. This avoids the messiness of\n> > setting 0 to hand back manual control back to the AGC?\n>\n> Once we'll get more than two parameters to control (adding aperture to\n> shutter and gain), I think this would become a bit messy, as there will\n> be 8 combinations of what can be controlled manually vs. automatically.\n>\n> > I'm sure David will also want to share his opinions on this, but he won't be able to\n> > respond for a few days.\n>\n> If David has time to share his opinion at some point, that would be\n> helpful.\n>\n> > > Please also let me know if the above explanation is clear or if you need\n> > > more information.\n> > >\n...\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 215E0BD87C\n\tfor ;\n\tTue, 24 Aug 2021 15:05:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8031F688AC;\n\tTue, 24 Aug 2021 17:05:00 +0200 (CEST)","from mail-wm1-x32d.google.com (mail-wm1-x32d.google.com\n\t[IPv6:2a00:1450:4864:20::32d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4F58160505\n\tfor ;\n\tTue, 24 Aug 2021 17:04:59 +0200 (CEST)","by mail-wm1-x32d.google.com with SMTP id\n\t79-20020a1c0452000000b002e6cf79e572so2117767wme.1\n\tfor ;\n\tTue, 24 Aug 2021 08:04:59 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"g/qBmsjF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=dMPuZjytGxo+ZY6U829rQHf8LHVV73bRVmhnqvReSZo=;\n\tb=g/qBmsjFXHLOhU6vpeUNXPU1kY9cF0pTLIr5djN4wCsxVx2x1u22ZRvsYXPrZzk7dR\n\tByUp6EzOYarkDLoU+OhI3elLtmXw/UFa9/ovcCMBz0kR9KM9UmbDY8kfJguhPY7rQAVs\n\tzsdD73cYiwipyprhfGS1ObX+/2GUB9EbTEABb9oikfN4j2Z+qMJYqdT+HrSr3wZa1Po3\n\tBDUxhUAZiox0Jmwi+fSFAecRPjpjH0taV1iYA0Gy9xdAwVyj6iIqyiqYqtFKb0AWrmuu\n\t0URuhrDnh3d3u0hsxiv1MqhF/AcTum+S+9/+i1YLVtUUP+w693+XNeQu2QSJM1SoW4Yy\n\taEXw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=dMPuZjytGxo+ZY6U829rQHf8LHVV73bRVmhnqvReSZo=;\n\tb=n2ZcQCafr7EBej/rgl5ecpEtyy5ogpcvAqLVFjkRfi1iCXPZyW9F20yUnN1NrNpeBn\n\t0k9rcz3BmwceTkHpyiR/lz0B1hiacCfVqeIl30jxd0fqlCnBTdkeiL7nO0ZBp3SoAbT6\n\tRFAnZuhfSrV7UhVMprI4/vogEvhxZgFQtJgjg3Jyt+ARC4y//nao9k4A77dkN8H63cb9\n\tLZRYIer6OpVxkYHp/asXcwClMGqgpo9p8alTBpXZAFWYkgmUtHJ7GWBThx/JDEuBI+lV\n\tDKddZ0INq53IlItqLgMEVlAMG4DPOkVSS7PLdNLddlrc0iSea1UDDCwDm3JkciXwOkht\n\tih/g==","X-Gm-Message-State":"AOAM533bL7+P1nnCaEwgjJfe3POqnMSlJiXJephcTbdbsznsLRynSuNB\n\t2rXRH1B7YpWtPZULX3ZMg5Og5lEHJ611j2WnSkfb0g==","X-Google-Smtp-Source":"ABdhPJzUZ7otxeE0vybaBpJs5/WZNlcmXpKABPwpXNvsbjz2QfU1lBVVq8haZxAGv0WUL4ILb1rJBlEfPXO0CutdUy0=","X-Received":"by 2002:a05:600c:3795:: with SMTP id\n\to21mr2826748wmr.130.1629817498455; \n\tTue, 24 Aug 2021 08:04:58 -0700 (PDT)","MIME-Version":"1.0","References":"<20210702103800.41291-1-paul.elder@ideasonboard.com>\n\t<20210702103800.41291-6-paul.elder@ideasonboard.com>\n\t\n\t\n\t\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Tue, 24 Aug 2021 16:04:47 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH v3 05/16] controls: Replace\n\tAeLocked with AeState, and add AeLock","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","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19051,"web_url":"https://patchwork.libcamera.org/comment/19051/","msgid":"<20210825102056.GF968527@pyrite.rasen.tech>","date":"2021-08-25T10:20:56","subject":"Re: [libcamera-devel] [RFC PATCH v3 05/16] controls: Replace\n\tAeLocked with AeState, and add AeLock","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi David,\n\nThank you for your input.\n\nOn Tue, Aug 24, 2021 at 04:04:47PM +0100, David Plowman wrote:\n> Hi Laurent\n> \n> Sorry for the late reply, it seemed to get erroneously discarded by my\n> human level spam filter (i.e. myself)! Apologies for that. I've also\n> deleted anything with more than three \"> > >\" indents, I was rather\n> drowning - hope that doesn't confuse anyone!\n> \n> Anyway, here are some of my thoughts on the matter. In the end I\n> decided to leave them at the top here, I'm not too sure how well they\n> fit into all the text below.\n> \n> 1. I like the idea of being able to fix some but not all of the\n> AEC/AGC \"levers\" (e.g. gain but not exposure). Android seems not to\n> allow this? But I think it's essential.\n> \n> 2. We don't like unfixing these levers by writing a magic zero. There\n> needs to be another mechanism.\n> \n> 3. In the RPi code, we fix specific levers by writing a fixed value.\n> Yet there's no way to fix a single lever at the current value, which\n> is what happens to all the levers when we disable AEC/AGC. That feels\n> a bit inconsistent.\n> \n> 4. I think point 1 above means that fixing all levers is a natural\n> state of the AEC/AGC algorithm, so there doesn't need to be any kind\n> of completely separate manual mode. My personal view is therefore that\n> libcamera possibly shouldn't emulate the Android behaviour here.\n\nAfter a long meditation, I've reached these same points as well. I'm\nglad we agree on these points.\n\n> \n> So here's something that might work for everyone:\n> \n> - Every lever has its own enable, assuming the implementation can support it.\n> \n> - Disabling AEC/AGC globally is the same as disabling all the levers.\n> \n> - Enabling a specific lever is obviously the new version of writing a\n> zero to it!\n> \n> - When you disable a lever, it gets fixed to its current value.\n> \n> - Or you can disable a lever by writing a specific value, which is\n> obviously then the value it gets.\n\nAnd I'm working on a design that practically does all these things :)\n\n> \n> So I think that might cover all the use cases we have? I don't really\n> see the point of a separate \"AeLock\" control in libcamera itself, it\n> simply seems unnecessary (unless I've missed something). Nonetheless,\n> the Android view of the world can easily be implemented on top of\n> this, including the \"lock\" mechanism.\n\nWe need aelock to transition seamlessly from auto to manual. If you set\nAE to off and feed the values that were last computed by AE both in a\nrequest, then the requests that were already in-flight would still have\nAE on, and the values might change. Then once your first \"AE off\"\nrequest gets in, it'll be a seamful change from auto to manual.\n\nInstead, you set aelock and wait for it to propagate through the\npipeline. Once it is, you can copy those exposure values, and send a\nrequest with those + AE off. Thus you can have seamless switching from\nauto to off.\n\nI'm thinking of having some sort of lock for each of the \"sub-levers\"\n(as you call it, I call them \"sub-controls\"). But I need to figure out\nand confirm that the flow will work.\n\n\nThanks,\n\nPaul\n\n> I expect non-Android applications will prefer to disable levers\n> without specifying a value so that you just pick up the current one.\n> To me this feels like the most usual situation.\n> \n> But I'm always happy to be corrected or persuaded otherwise!\n> \n> Thanks\n> \n> David\n> \n> On Mon, 16 Aug 2021 at 03:07, Laurent Pinchart\n> wrote:\n> >\n> > Hi Naush,\n> >\n> > On Mon, Jul 12, 2021 at 03:50:29PM +0100, Naushir Patuck wrote:\n> > > On Sun, 11 Jul 2021 at 23:37, Laurent Pinchart wrote:\n> > > > On Mon, Jul 05, 2021 at 06:34:07PM +0100, Naushir Patuck wrote:\n> ...\n> > > >\n> > > > Is it ? I thought it should be AeStateConverged.\n> > > >\n> ...\n> > > >\n> > > > Please see below.\n> > > >\n> ...\n> > > >\n> > > > First of all, let me clarify the goal. The libcamera native API needs to\n> > > > offer the features required to implement the Android camera HAL API, but\n> > > > we don't need to just duplicate it. When there are reasons to depart\n> > > > (for instance to fix historical mistakes made in Android, to provide\n> > > > additional features, or just to expose an API we consider more\n> > > > consistent), we should do so.\n> > > >\n> > > > Then, a word about the process. Implementing support for an Android\n> > > > control requires plumbing it through the camera stack: definition of a\n> > > > libcamera control, translation between the Android and libcamera control\n> > > > in the Android camera HAL, implementation of the libcamera control in\n> > > > the pipeline handlesr, and, when applicable, in the IPAs. Defining\n> > > > libcamera controls, as any API design effort, takes time. To allow\n> > > > moving forward on all the other parts, we have a fast-track process to\n> > > > define draft controls that mimick the Android controls. All the draft\n> > > > controls will be redesigned before freezing the libcamera public API.\n> > > >\n> > > > This means that AeLock and AeState should be marked as draft controls as\n> > > > part of this patch.\n> > > >\n> > > > This being said, we don't need to define controls as draft controls if\n> > > > we want to already go through a complete design process. The discussion\n> > > > that stemmed from this patch series goes in that direction.\n> > > >\n> > > > It's useful, in this context, to understand why the Android AE (and AWB)\n> > > > controls have been designed this way. Conceptually speaking, the\n> > > > algorithms are considered, in Android, to be one level above manual\n> > > > controls.\n> > > >\n> > > > Controls Metadata\n> > > > /---------- == OFF ---------\\\n> > > > | v\n> > > > aeMode -/ +------+ |\\\n> > > > aeLock --> | AE | --> |0\\ +--------+\n> > > > aeExposureCompensation +------+ | | --> | Sensor | --> exposureTime\n> > > > | | +--------+ sensitivity\n> > > > exposureTime -----------------------> |1/\n> > > > sensitivity |/\n> > > >\n> > > > (The AE controls are in the adnroid.control class, while the sensor\n> > > > manual control are in the android.sensor class.)\n> > > >\n> > > > The aeMode control selects whether the AE algorithm (when != OFF) or the\n> > > > manual controls (when == OFF) drive the sensor. The aeLock control tells\n> > > > the AE algorithm to freeze its output (when ON) or operate normally\n> > > > (when OFF). The metadata always report the values applied to the sensor,\n> > > > regardless of whether they come from the AE algorithm or from manual\n> > > > controls.\n> > > >\n> > > > One important difference, compared to the model we currently implement,\n> > > > is that the Android AE algorithm is independent from the manual sensor\n> > > > controls. The values of the manual controls aren't affected by the AE\n> > > > algorithm, and when AE is disabled, the last set manual control values\n> > > > are applied. This thus require an aeLock control to transition\n> > > > seamlessly between AE and manual mode, as the application needs to set\n> > > > the manual controls to the values applied to the sensor on the last\n> > > > frame that AE was on, which couldn't otherwise be done due to the\n> > > > internal delays.\n> > >\n> > > Thank you for the detailed explanation here. I can now better understand\n> > > the Android AE state transitions and why they may be needed.\n> > >\n> > > > The existing libcamera AE controls are ill-defined. There's a \\todo\n> > > > comment to document the interactions between AeEnable and setting a\n> > > > fixed value for ExposureTime or AnalogueGain. I'm actually not sure what\n> > > > happens if AeEnable is set to false when ExposureTime and/or\n> > > > AnalogueGain are 0, or when AeEnable is set to true when ExposureTime\n> > > > and/or AnalogueGain are not 0.\n> > >\n> > > To provide some brief context on the Raspberry Pi AGC, we never really\n> > > go into a \"disabled\" state at all. This is because the AGC algorithm always\n> > > provides the shutter/gain values to pass to the sensor, even in manual mode\n> > > (more on this below). We use an internal Paused state to lock the shutter/gain\n> > > values so that they can never change, which essentially mimics a \"disabled\"\n> > > state.\n> > >\n> > > For manual operation, we pass shutter/gain ctrl values (either or both) to the AGC,\n> > > and that is what the AGC requests off the sensor. This happens regardless of\n> > > whether the AGC is in it's internal paused state or not. If the AGC is not in\n> > > paused state, and we only provide one manual control value (e.g. shutter speed),\n> > > then the AGC will vary the gain as it sees fit to converge to the desired\n> > > brightness. Setting a manual shutter/gain ctrl value back to 0 hands back control\n> > > of the control to the AGC and it starts adapting it as necessary to converge to\n> > > the desired brightness.\n> > >\n> > > This type of implementation does allow us to essentially do all that Android would\n> > > except of the AGC, but does also allow us the flexibility to do Aperture/Shutter/Gain\n> > > priority modes if needed.\n> >\n> > If I understand correctly, when the AeEnable control is set to false,\n> > the AGC algorithm is paused, which just means that it sets the fixed\n> > shutter and gain to the last computed values. Those are the values that\n> > will then be applied to the sensor, until either the AeEnable control is\n> > set back to true, or the manual gain or exposure time control is set to\n> > a different value.\n> >\n> > This leads us to the first interesting behaviour: if the application\n> > keeps queuing request that contain the AeEnable control set to false, as\n> > well as manual exposure and/or gain values, in order to apply the manual\n> > values as expected, the Agc::SetFixedShutter() and\n> > Agc::SetFixedAnalogueGain() functions copy the fixed values to the\n> > status_ structure, so that pausing the algorithm won't override them.\n> > There's a bit of fragility here that I'm not sure to like, as it seems\n> > fairly error-prone (not in the existing Raspberry Pi IPA implementation,\n> > but from a general IPA module development point of view).\n> >\n> > Another interesting side effect is what happens if the application then\n> > sets the exposure time and/or gain to 0 while keeping AeEnable to false.\n> > The AGC algorithm will resume operation in that case. We can of course\n> > argue that this is a corner case and that applications shouldn't do\n> > this, but it's in any case something that needs to be documented.\n> >\n> > Looking at how to switch from auto to manual exposure seamlessly, an\n> > application would need to go through the following sequence:\n> >\n> > - Set AeEnable to false in a request, without setting a manual gain or\n> > exposure.\n> > - Wait until metadata reports AeEnable set to false (this isn't\n> > currently implemented in the Raspberry Pi IPA), to know that the\n> > setting has taken effect.\n> > - Take the reported gain and exposure time from the request that\n> > reported AeEnable set to false, and use them as starting values for\n> > manual control of the AGC.\n> >\n> > On Android, the equivalent sequence is documented in\n> > https://ideasonboard.org/android/docs.html#controls_android.control.aeLock.\n> > The sequences are quite similar, the main difference being that when\n> > Android calls aeLock is called AeEnable in libcamera. This causes\n> > another issue: the AeEnable control needs to be reported in metadata,\n> > but its value doesn't actually indicate whether the gain and exposure\n> > time are controlled automatically or not. If an application sets\n> > AeEnable to true but also sets manual gain and exposure time, metadata\n> > will report AeEnable to true, but the AGC will be manually controlled.\n> > This will also be difficult to document in a clear way I think.\n> >\n> > Another point that bothers me is that treating 0 as a magic value to\n> > mean automatic control prevents from reporting a minimum value for the\n> > exposure time and gain to applications. The Raspberry Pi IPA report the\n> > AnalogueGain control as having a 1.0 minimum value, but when we'll\n> > enforce bound checking in the libcamera core (as this really shouldn't\n> > be duplicated by all IPAs), this will break.\n> >\n> > > > We need a formal model for how libcamera algorithms interact with manual\n> > > > controls. It doesn't have to duplicate the Android model, but it has to\n> > > > offer at least the same features. I believe we need to take it one step\n> > > > further, as we want to support exposure priority and aperture priority.\n> > > > I think we should actually talk about gain/sensitivity priority instead\n> > > > of aperture priority for the existing feature, and already prepare for\n> > > > devices that have a controllable lens aperture.\n> > > >\n> > > > Now, the hard question: how should libcamera model this ? :-) Naush,\n> > > > David, your feedback from a RPi point of view would be very appreciated.\n> > >\n> > > It would be easy for me to say do it the Raspberry Pi way :-)\n> >\n> > I have no ideological opposition to that. What bothers me at the moment\n> > is that the interactions between the controls related to AGC are not\n> > well defined in the documentation, and based on the comments above, I\n> > think it will be difficult to do so in a clear way, and avoid\n> > error-prone behaviours. There's also the bound checking issue for the\n> > gain (and exposure time, the lower limit happens to be 0 for the\n> > Raspberry Pi IPA, but that won't be the case universally) that seems\n> > difficult to fix.\n> >\n> > I fear that documenting the behaviour of the existing controls will be\n> > difficult, and will result in unclear documentation. If we want to keep\n> > the existing behaviour, I'd like to see documentation that shows that my\n> > fears are unfounded.\n> >\n> > This being said, I'm not advocating for replicating the Android\n> > controls, as, as discussed previously, they don't support implementing\n> > shutter or gain priority, which is an important feature. We may however\n> > decide to design a different semantics for the AE controls that would\n> > support all our use cases and result in simpler documentation.\n> >\n> > > But in reality, I don't think the Android AE model gains us an advantage over our existing\n> > > model. And having to track one less state (locked vs disabled) is always a good thing.\n> > >\n> > > Explicitly defining AeModes to have (amongst others) ShutterPrioiry, GainPrioiry,\n> > > AperturePriority and Auto modes would be useful though. This avoids the messiness of\n> > > setting 0 to hand back manual control back to the AGC?\n> >\n> > Once we'll get more than two parameters to control (adding aperture to\n> > shutter and gain), I think this would become a bit messy, as there will\n> > be 8 combinations of what can be controlled manually vs. automatically.\n> >\n> > > I'm sure David will also want to share his opinions on this, but he won't be able to\n> > > respond for a few days.\n> >\n> > If David has time to share his opinion at some point, that would be\n> > helpful.\n> >\n> > > > Please also let me know if the above explanation is clear or if you need\n> > > > more information.\n> > > >\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 EBD04BD87C\n\tfor ;\n\tWed, 25 Aug 2021 10:21:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5061468893;\n\tWed, 25 Aug 2021 12:21:07 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0DF2660259\n\tfor ;\n\tWed, 25 Aug 2021 12:21:06 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EB7EE24F;\n\tWed, 25 Aug 2021 12:21:03 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"E/i00Zkj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1629886865;\n\tbh=M7DcyRe2nojoU/xGw4SNmQ1DUSX/H9rQChrmf0OPhMA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=E/i00ZkjXysHOn40z1l0ufJHhSRFb51w3kv00NVmdj+bpAur8bMXAPC7q52LJWJf8\n\tyxkinlBe0vilY9K1nTzSwcLBPWz7IH8mWZUhO4C3hhTkg4BrD7SjarktO5JzKa0y/n\n\tUk6mqMuFd7RfuzolzJHO52beSyuz5fvdBzfEw44I=","Date":"Wed, 25 Aug 2021 19:20:56 +0900","From":"paul.elder@ideasonboard.com","To":"David Plowman ","Message-ID":"<20210825102056.GF968527@pyrite.rasen.tech>","References":"<20210702103800.41291-1-paul.elder@ideasonboard.com>\n\t<20210702103800.41291-6-paul.elder@ideasonboard.com>\n\t\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH v3 05/16] controls: Replace\n\tAeLocked with AeState, and add AeLock","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","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19190,"web_url":"https://patchwork.libcamera.org/comment/19190/","msgid":"","date":"2021-08-31T00:39:25","subject":"Re: [libcamera-devel] [RFC PATCH v3 05/16] controls: Replace\n\tAeLocked with AeState, and add AeLock","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul and David,\n\nDavid, thanks a lot for your feedback. It's very appreciated, as usual.\n\nOn Wed, Aug 25, 2021 at 07:20:56PM +0900, paul.elder@ideasonboard.com wrote:\n> On Tue, Aug 24, 2021 at 04:04:47PM +0100, David Plowman wrote:\n> > Hi Laurent\n> > \n> > Sorry for the late reply, it seemed to get erroneously discarded by my\n> > human level spam filter (i.e. myself)! Apologies for that. I've also\n\nNo need to apologize, I'd probably also want to file most of my mails in\na place where I wouldn't see them if I were to receive them ;-)\n\n> > deleted anything with more than three \"> > >\" indents, I was rather\n> > drowning - hope that doesn't confuse anyone!\n> > \n> > Anyway, here are some of my thoughts on the matter. In the end I\n> > decided to leave them at the top here, I'm not too sure how well they\n> > fit into all the text below.\n> > \n> > 1. I like the idea of being able to fix some but not all of the\n> > AEC/AGC \"levers\" (e.g. gain but not exposure). Android seems not to\n> > allow this? But I think it's essential.\n\nAgreed, I think we need this. The upside is that we'd then implement a\nsuperset of what Android requires, so Android support won't cause extra\nwork.\n\n> > 2. We don't like unfixing these levers by writing a magic zero. There\n> > needs to be another mechanism.\n\nHappy to hear that you agree on this. Let's find a good API, and then we\ncan port the Raspberry Pi AGC algorithm to it.\n\n> > 3. In the RPi code, we fix specific levers by writing a fixed value.\n> > Yet there's no way to fix a single lever at the current value, which\n> > is what happens to all the levers when we disable AEC/AGC. That feels\n> > a bit inconsistent.\n> > \n> > 4. I think point 1 above means that fixing all levers is a natural\n> > state of the AEC/AGC algorithm, so there doesn't need to be any kind\n> > of completely separate manual mode. My personal view is therefore that\n> > libcamera possibly shouldn't emulate the Android behaviour here.\n> \n> After a long meditation, I've reached these same points as well. I'm\n> glad we agree on these points.\n> \n> > So here's something that might work for everyone:\n> > \n> > - Every lever has its own enable, assuming the implementation can support it.\n> > \n> > - Disabling AEC/AGC globally is the same as disabling all the levers.\n> > \n> > - Enabling a specific lever is obviously the new version of writing a\n> > zero to it!\n> > \n> > - When you disable a lever, it gets fixed to its current value.\n\nThis is the part that may be tricky, as explained by Paul below, to\nimplement a seamless transition.\n\n> > - Or you can disable a lever by writing a specific value, which is\n> > obviously then the value it gets.\n> \n> And I'm working on a design that practically does all these things :)\n> \n> > So I think that might cover all the use cases we have? I don't really\n> > see the point of a separate \"AeLock\" control in libcamera itself, it\n> > simply seems unnecessary (unless I've missed something). Nonetheless,\n> > the Android view of the world can easily be implemented on top of\n> > this, including the \"lock\" mechanism.\n> \n> We need aelock to transition seamlessly from auto to manual. If you set\n> AE to off and feed the values that were last computed by AE both in a\n> request, then the requests that were already in-flight would still have\n> AE on, and the values might change. Then once your first \"AE off\"\n> request gets in, it'll be a seamful change from auto to manual.\n> \n> Instead, you set aelock and wait for it to propagate through the\n> pipeline. Once it is, you can copy those exposure values, and send a\n> request with those + AE off. Thus you can have seamless switching from\n> auto to off.\n> \n> I'm thinking of having some sort of lock for each of the \"sub-levers\"\n> (as you call it, I call them \"sub-controls\"). But I need to figure out\n> and confirm that the flow will work.\n\nSounds good to me. I'll now review your proposal :-)\n\n> > I expect non-Android applications will prefer to disable levers\n> > without specifying a value so that you just pick up the current one.\n> > To me this feels like the most usual situation.\n> > \n> > But I'm always happy to be corrected or persuaded otherwise!\n> > \n> > On Mon, 16 Aug 2021 at 03:07, Laurent Pinchart wrote:\n> > > On Mon, Jul 12, 2021 at 03:50:29PM +0100, Naushir Patuck wrote:\n> > > > On Sun, 11 Jul 2021 at 23:37, Laurent Pinchart wrote:\n> > > > > On Mon, Jul 05, 2021 at 06:34:07PM +0100, Naushir Patuck wrote:\n> > ...\n> > > > >\n> > > > > Is it ? I thought it should be AeStateConverged.\n> > > > >\n> > ...\n> > > > >\n> > > > > Please see below.\n> > > > >\n> > ...\n> > > > >\n> > > > > First of all, let me clarify the goal. The libcamera native API needs to\n> > > > > offer the features required to implement the Android camera HAL API, but\n> > > > > we don't need to just duplicate it. When there are reasons to depart\n> > > > > (for instance to fix historical mistakes made in Android, to provide\n> > > > > additional features, or just to expose an API we consider more\n> > > > > consistent), we should do so.\n> > > > >\n> > > > > Then, a word about the process. Implementing support for an Android\n> > > > > control requires plumbing it through the camera stack: definition of a\n> > > > > libcamera control, translation between the Android and libcamera control\n> > > > > in the Android camera HAL, implementation of the libcamera control in\n> > > > > the pipeline handlesr, and, when applicable, in the IPAs. Defining\n> > > > > libcamera controls, as any API design effort, takes time. To allow\n> > > > > moving forward on all the other parts, we have a fast-track process to\n> > > > > define draft controls that mimick the Android controls. All the draft\n> > > > > controls will be redesigned before freezing the libcamera public API.\n> > > > >\n> > > > > This means that AeLock and AeState should be marked as draft controls as\n> > > > > part of this patch.\n> > > > >\n> > > > > This being said, we don't need to define controls as draft controls if\n> > > > > we want to already go through a complete design process. The discussion\n> > > > > that stemmed from this patch series goes in that direction.\n> > > > >\n> > > > > It's useful, in this context, to understand why the Android AE (and AWB)\n> > > > > controls have been designed this way. Conceptually speaking, the\n> > > > > algorithms are considered, in Android, to be one level above manual\n> > > > > controls.\n> > > > >\n> > > > > Controls Metadata\n> > > > > /---------- == OFF ---------\\\n> > > > > | v\n> > > > > aeMode -/ +------+ |\\\n> > > > > aeLock --> | AE | --> |0\\ +--------+\n> > > > > aeExposureCompensation +------+ | | --> | Sensor | --> exposureTime\n> > > > > | | +--------+ sensitivity\n> > > > > exposureTime -----------------------> |1/\n> > > > > sensitivity |/\n> > > > >\n> > > > > (The AE controls are in the adnroid.control class, while the sensor\n> > > > > manual control are in the android.sensor class.)\n> > > > >\n> > > > > The aeMode control selects whether the AE algorithm (when != OFF) or the\n> > > > > manual controls (when == OFF) drive the sensor. The aeLock control tells\n> > > > > the AE algorithm to freeze its output (when ON) or operate normally\n> > > > > (when OFF). The metadata always report the values applied to the sensor,\n> > > > > regardless of whether they come from the AE algorithm or from manual\n> > > > > controls.\n> > > > >\n> > > > > One important difference, compared to the model we currently implement,\n> > > > > is that the Android AE algorithm is independent from the manual sensor\n> > > > > controls. The values of the manual controls aren't affected by the AE\n> > > > > algorithm, and when AE is disabled, the last set manual control values\n> > > > > are applied. This thus require an aeLock control to transition\n> > > > > seamlessly between AE and manual mode, as the application needs to set\n> > > > > the manual controls to the values applied to the sensor on the last\n> > > > > frame that AE was on, which couldn't otherwise be done due to the\n> > > > > internal delays.\n> > > >\n> > > > Thank you for the detailed explanation here. I can now better understand\n> > > > the Android AE state transitions and why they may be needed.\n> > > >\n> > > > > The existing libcamera AE controls are ill-defined. There's a \\todo\n> > > > > comment to document the interactions between AeEnable and setting a\n> > > > > fixed value for ExposureTime or AnalogueGain. I'm actually not sure what\n> > > > > happens if AeEnable is set to false when ExposureTime and/or\n> > > > > AnalogueGain are 0, or when AeEnable is set to true when ExposureTime\n> > > > > and/or AnalogueGain are not 0.\n> > > >\n> > > > To provide some brief context on the Raspberry Pi AGC, we never really\n> > > > go into a \"disabled\" state at all. This is because the AGC algorithm always\n> > > > provides the shutter/gain values to pass to the sensor, even in manual mode\n> > > > (more on this below). We use an internal Paused state to lock the shutter/gain\n> > > > values so that they can never change, which essentially mimics a \"disabled\"\n> > > > state.\n> > > >\n> > > > For manual operation, we pass shutter/gain ctrl values (either or both) to the AGC,\n> > > > and that is what the AGC requests off the sensor. This happens regardless of\n> > > > whether the AGC is in it's internal paused state or not. If the AGC is not in\n> > > > paused state, and we only provide one manual control value (e.g. shutter speed),\n> > > > then the AGC will vary the gain as it sees fit to converge to the desired\n> > > > brightness. Setting a manual shutter/gain ctrl value back to 0 hands back control\n> > > > of the control to the AGC and it starts adapting it as necessary to converge to\n> > > > the desired brightness.\n> > > >\n> > > > This type of implementation does allow us to essentially do all that Android would\n> > > > except of the AGC, but does also allow us the flexibility to do Aperture/Shutter/Gain\n> > > > priority modes if needed.\n> > >\n> > > If I understand correctly, when the AeEnable control is set to false,\n> > > the AGC algorithm is paused, which just means that it sets the fixed\n> > > shutter and gain to the last computed values. Those are the values that\n> > > will then be applied to the sensor, until either the AeEnable control is\n> > > set back to true, or the manual gain or exposure time control is set to\n> > > a different value.\n> > >\n> > > This leads us to the first interesting behaviour: if the application\n> > > keeps queuing request that contain the AeEnable control set to false, as\n> > > well as manual exposure and/or gain values, in order to apply the manual\n> > > values as expected, the Agc::SetFixedShutter() and\n> > > Agc::SetFixedAnalogueGain() functions copy the fixed values to the\n> > > status_ structure, so that pausing the algorithm won't override them.\n> > > There's a bit of fragility here that I'm not sure to like, as it seems\n> > > fairly error-prone (not in the existing Raspberry Pi IPA implementation,\n> > > but from a general IPA module development point of view).\n> > >\n> > > Another interesting side effect is what happens if the application then\n> > > sets the exposure time and/or gain to 0 while keeping AeEnable to false.\n> > > The AGC algorithm will resume operation in that case. We can of course\n> > > argue that this is a corner case and that applications shouldn't do\n> > > this, but it's in any case something that needs to be documented.\n> > >\n> > > Looking at how to switch from auto to manual exposure seamlessly, an\n> > > application would need to go through the following sequence:\n> > >\n> > > - Set AeEnable to false in a request, without setting a manual gain or\n> > > exposure.\n> > > - Wait until metadata reports AeEnable set to false (this isn't\n> > > currently implemented in the Raspberry Pi IPA), to know that the\n> > > setting has taken effect.\n> > > - Take the reported gain and exposure time from the request that\n> > > reported AeEnable set to false, and use them as starting values for\n> > > manual control of the AGC.\n> > >\n> > > On Android, the equivalent sequence is documented in\n> > > https://ideasonboard.org/android/docs.html#controls_android.control.aeLock.\n> > > The sequences are quite similar, the main difference being that when\n> > > Android calls aeLock is called AeEnable in libcamera. This causes\n> > > another issue: the AeEnable control needs to be reported in metadata,\n> > > but its value doesn't actually indicate whether the gain and exposure\n> > > time are controlled automatically or not. If an application sets\n> > > AeEnable to true but also sets manual gain and exposure time, metadata\n> > > will report AeEnable to true, but the AGC will be manually controlled.\n> > > This will also be difficult to document in a clear way I think.\n> > >\n> > > Another point that bothers me is that treating 0 as a magic value to\n> > > mean automatic control prevents from reporting a minimum value for the\n> > > exposure time and gain to applications. The Raspberry Pi IPA report the\n> > > AnalogueGain control as having a 1.0 minimum value, but when we'll\n> > > enforce bound checking in the libcamera core (as this really shouldn't\n> > > be duplicated by all IPAs), this will break.\n> > >\n> > > > > We need a formal model for how libcamera algorithms interact with manual\n> > > > > controls. It doesn't have to duplicate the Android model, but it has to\n> > > > > offer at least the same features. I believe we need to take it one step\n> > > > > further, as we want to support exposure priority and aperture priority.\n> > > > > I think we should actually talk about gain/sensitivity priority instead\n> > > > > of aperture priority for the existing feature, and already prepare for\n> > > > > devices that have a controllable lens aperture.\n> > > > >\n> > > > > Now, the hard question: how should libcamera model this ? :-) Naush,\n> > > > > David, your feedback from a RPi point of view would be very appreciated.\n> > > >\n> > > > It would be easy for me to say do it the Raspberry Pi way :-)\n> > >\n> > > I have no ideological opposition to that. What bothers me at the moment\n> > > is that the interactions between the controls related to AGC are not\n> > > well defined in the documentation, and based on the comments above, I\n> > > think it will be difficult to do so in a clear way, and avoid\n> > > error-prone behaviours. There's also the bound checking issue for the\n> > > gain (and exposure time, the lower limit happens to be 0 for the\n> > > Raspberry Pi IPA, but that won't be the case universally) that seems\n> > > difficult to fix.\n> > >\n> > > I fear that documenting the behaviour of the existing controls will be\n> > > difficult, and will result in unclear documentation. If we want to keep\n> > > the existing behaviour, I'd like to see documentation that shows that my\n> > > fears are unfounded.\n> > >\n> > > This being said, I'm not advocating for replicating the Android\n> > > controls, as, as discussed previously, they don't support implementing\n> > > shutter or gain priority, which is an important feature. We may however\n> > > decide to design a different semantics for the AE controls that would\n> > > support all our use cases and result in simpler documentation.\n> > >\n> > > > But in reality, I don't think the Android AE model gains us an advantage over our existing\n> > > > model. And having to track one less state (locked vs disabled) is always a good thing.\n> > > >\n> > > > Explicitly defining AeModes to have (amongst others) ShutterPrioiry, GainPrioiry,\n> > > > AperturePriority and Auto modes would be useful though. This avoids the messiness of\n> > > > setting 0 to hand back manual control back to the AGC?\n> > >\n> > > Once we'll get more than two parameters to control (adding aperture to\n> > > shutter and gain), I think this would become a bit messy, as there will\n> > > be 8 combinations of what can be controlled manually vs. automatically.\n> > >\n> > > > I'm sure David will also want to share his opinions on this, but he won't be able to\n> > > > respond for a few days.\n> > >\n> > > If David has time to share his opinion at some point, that would be\n> > > helpful.\n> > >\n> > > > > Please also let me know if the above explanation is clear or if you need\n> > > > > more information.\n> > > > >\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 2F67BBDC71\n\tfor ;\n\tTue, 31 Aug 2021 00:39:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 64C1A6916A;\n\tTue, 31 Aug 2021 02:39:41 +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 29E0C68891\n\tfor ;\n\tTue, 31 Aug 2021 02:39:40 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 964AE5A7;\n\tTue, 31 Aug 2021 02:39:39 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"FfA6pn27\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630370379;\n\tbh=w1T1Gw2B9KOa5vpA7cpokBm0f03PG25pvVhCv5R2gdc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=FfA6pn27+1cO8wWWykk63xhMRfSPHawO7XTFbMfh5JEuz4aTe1E/F5B4eDO1eG3Lo\n\tgjD7M1Npz4D/kEPpj8kMYKd40dbqSeZ7NtNzaGXAeCyGXCPANqs/1R8v4DpVyiIGLO\n\t8t3xyFcPvwlUGvtxea4u9pb7WMJvz1OHlRcOEuyI=","Date":"Tue, 31 Aug 2021 03:39:25 +0300","From":"Laurent Pinchart ","To":"paul.elder@ideasonboard.com","Message-ID":"","References":"<20210702103800.41291-1-paul.elder@ideasonboard.com>\n\t<20210702103800.41291-6-paul.elder@ideasonboard.com>\n\t\n\t\n\t\n\t\n\t\n\t<20210825102056.GF968527@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210825102056.GF968527@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [RFC PATCH v3 05/16] controls: Replace\n\tAeLocked with AeState, and add AeLock","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","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]