[{"id":17628,"web_url":"https://patchwork.libcamera.org/comment/17628/","msgid":"","date":"2021-06-18T12:07:28","subject":"Re: [libcamera-devel] [RFC PATCH 03/14] controls: Replace AeLocked\n\twith 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 patch.\n\nOn Fri, 18 Jun 2021 at 11:34, Paul Elder \nwrote:\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> 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 1c1e802a..ad0132c0 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -468,7 +468,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\nI think, strictly speaking, if agcStatus->locked == true, we should return\ncontrol::AeStateConverged by the definition below. David, do you agree?\n\nThanks,\nNaush\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 b47ea324..88a562a8 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> - 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> + - 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 B9EC6C3218\n\tfor ;\n\tFri, 18 Jun 2021 12:07:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 19CFC68942;\n\tFri, 18 Jun 2021 14:07:47 +0200 (CEST)","from mail-lf1-x129.google.com (mail-lf1-x129.google.com\n\t[IPv6:2a00:1450:4864:20::129])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 665FC60298\n\tfor ;\n\tFri, 18 Jun 2021 14:07:45 +0200 (CEST)","by mail-lf1-x129.google.com with SMTP id i13so16276753lfc.7\n\tfor ;\n\tFri, 18 Jun 2021 05:07:45 -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=\"HPg0rzo9\"; 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=z7GKSm4H8FxF0flpiVZbVcjnICIQy0Uu1iicU8jiyBA=;\n\tb=HPg0rzo9asGdqEEJQSPllmu0he0vUJGtqa6Sw0CL8YPgycXKWGda5zVXCS1sMGH0uY\n\tFSp0NNuwMVVwYGGtmDwKSXQuCC2qq0Nx1GigzpR38TpNhPxo2jcjX2vyIyKcYG2S4uSX\n\tB058lQJIY7EqRCVWqCI9zZbbuxJJAmhV00o3yc3DVwkRjrFZg+mn11feSG8taf2VYyB3\n\tpBrRCpqjO0NggkJJn1AERXQkF3ECG8xUkBJba/dk0MJUWbJ99RmFlr8xDMNqNUw/Zn+5\n\tx9apE6PQU5sFAierndo0qtw0FXZU3/9vKiM5vUdgz9kuty5tHI0fQxc+n5kc3UdmTD5g\n\t34uw==","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=z7GKSm4H8FxF0flpiVZbVcjnICIQy0Uu1iicU8jiyBA=;\n\tb=jWMyv8RgB+6t/cpwJf5kECCsw95sPqqMje7lWYwwf6fHKXCp7Lt3LxgQCyLRqHMeNe\n\t5bHG19OHuKw612Cpba8cYEam2P3Ss+yhu5PMlZvsIJvnYRUuIVU9RLyW4o6LfQV7P05Q\n\t8VBKQhrs14YoDn7bNIBSby8Fil+k5RQU4Iq7+LU22TRwwKoCWiUb0bEPO9C4lfjs1UVj\n\txFHnrtmeDz/v+sN0L0StLKY5ctWqBR2OdMMfoy4u+/I9asX4lGXF6VHtaXA0QDIqkr6t\n\t80sQ6aRlDzWQQm00qJna5aiiWO8y+jchRbZS/RvvtP9HZ399NBvqxij+WfezO5GAX1qq\n\tn30w==","X-Gm-Message-State":"AOAM533shwOlY5YUGJymxWNE6XbxbkVde7BMN9ZP2pMRIToYTjmFunM1\n\teNn41SzY8fKybtZSoPHlg8uk9HETJVxHsXYvbAGr2Q==","X-Google-Smtp-Source":"ABdhPJzQufIkSDK13Vfo65Xypk9g2ozS6hYuibb0hVhr434ydq5mAnI5EpxPaEomSnsF8IOEHhBulDYiyhla1Iy3RVM=","X-Received":"by 2002:a05:6512:1184:: with SMTP id\n\tg4mr2853795lfr.567.1624018064569; \n\tFri, 18 Jun 2021 05:07:44 -0700 (PDT)","MIME-Version":"1.0","References":"<20210618103351.1642060-1-paul.elder@ideasonboard.com>\n\t<20210618103351.1642060-4-paul.elder@ideasonboard.com>","In-Reply-To":"<20210618103351.1642060-4-paul.elder@ideasonboard.com>","From":"Naushir Patuck ","Date":"Fri, 18 Jun 2021 13:07:28 +0100","Message-ID":"","To":"Paul Elder ","Content-Type":"multipart/alternative; boundary=\"000000000000d908ef05c5092b65\"","Subject":"Re: [libcamera-devel] [RFC PATCH 03/14] controls: Replace AeLocked\n\twith 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":17630,"web_url":"https://patchwork.libcamera.org/comment/17630/","msgid":"","date":"2021-06-18T13:13:15","subject":"Re: [libcamera-devel] [RFC PATCH 03/14] controls: Replace AeLocked\n\twith 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 Naush, everyone\n\nOn Fri, 18 Jun 2021 at 13:07, Naushir Patuck wrote:\n>\n> Hi Paul,\n>\n> Thank you for your patch.\n>\n> On Fri, 18 Jun 2021 at 11:34, 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>> 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 1c1e802a..ad0132c0 100644\n>> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n>> @@ -468,7 +468,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>\n> I think, strictly speaking, if agcStatus->locked == true, we should return\n> control::AeStateConverged by the definition below. David, do you agree?\n\nHmm, indeed. My reading is that what we used to know as AeEnable has\nnow been renamed/replaced by AeLock, is that right? And as you say,\n\"locked\" does not mean \"converged\" (any more).\n\nSo:\n\nWe set controls::AeLocked in the metadata according to the value that\nwe received in the corresponding control (which we now receive instead\nof AeEnable), and\n\nWe set controls::AeState in the metadata to AeStateConverged when\nagcStatus->locked is true, otherwise to AeStateSearching. We should\nprobably bite the bullet and rename that \"locked\" field to\n\"converged\", otherwise it's going to cause confusion for the rest of\neternity.\n\nDavid\n\n>\n> Thanks,\n> Naush\n>\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 b47ea324..88a562a8 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>> - 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 C5129C3218\n\tfor ;\n\tFri, 18 Jun 2021 13:13:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EF64F68945;\n\tFri, 18 Jun 2021 15:13:27 +0200 (CEST)","from mail-wm1-x332.google.com (mail-wm1-x332.google.com\n\t[IPv6:2a00:1450:4864:20::332])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1D7DE60298\n\tfor ;\n\tFri, 18 Jun 2021 15:13:26 +0200 (CEST)","by mail-wm1-x332.google.com with SMTP id\n\tn35-20020a05600c3ba3b02901cdecb6bda8so8659042wms.5\n\tfor ;\n\tFri, 18 Jun 2021 06:13:26 -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=\"EjHrmeCW\"; 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=AQ+EFLJhzlKdaSLlfsFmy6qTi9gX+LeYAHv46PbyuJ4=;\n\tb=EjHrmeCW875qM2n4lYViGt0xRXT60piXAxISanIKMxSdWONCwI94li0zDcn7O666rE\n\t7nPljvEIT7fC6FvXCCkbO5n3qYLWzWdM7bI8eU7hQitMa1ZvTzOWo476nhdvFnN6BkEO\n\tq+///mBQGQTWh/axgEYj3DVsjBir70cZw10GTzQ2U+LcxQng8MYkGAtyGaGW9eByz6la\n\tnzgWF3Sr3Sl/CZZhBJzsJggSyhzumWnj+uwQnLY+9KAvwLfxYo1EZ2UntN6fVWIcLeek\n\tj2WCVmZgB/157N7QjLdvc8V0PaWogxAes4Oq06dxiid0UctR6GOV0u4SGckK+86WoqN3\n\tNEUg==","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=AQ+EFLJhzlKdaSLlfsFmy6qTi9gX+LeYAHv46PbyuJ4=;\n\tb=OT+CxDg2HplZ6ZCksdbvqxEcN/ZYQwE1EcckqwaJ1OHR+KV//px5oRPexJyEUHa0y7\n\t65MaM4GmM3H6W2BosdJ5h1msClfaWCd/nR7nqWQeYyN14/FB9dbwRaxgNQs/EuylhfHo\n\tndUSZa2KtBSsQyDp9jvMEARiWDFHk1YQBicMSgabac1Bx/0IyEuPVgwkclOsZ3IOL8FR\n\th4NPAVqpkUAc21yR2DyXyx/Fj2tPwMqxKIG6TcklbYdjWjaFTJLB1myJS8ntGIS/og7y\n\tKFJ7fO2EMcFg9L9ZHsve0Y8aflW+CavoYAn/5vmufQ4SpojsetZjtvFWyxtyPXmmjTzd\n\tr9IQ==","X-Gm-Message-State":"AOAM532BaV8ubjmjz/G+G06KOKtuQ6WHnLdTwAEFPvludQ32e1m2dJQ+\n\tFrg2ZeCxJM1t+JG4LL4vr56DH1aJCC5rh6hcpHYgjA==","X-Google-Smtp-Source":"ABdhPJx6rr+h55GATsx+XkMdPJGe4Zf1RaZcB3JrvP333nc5Yx4hy2HO0zOoPjtNZgIGts+CuGusC/ThNqhu1WhJx5c=","X-Received":"by 2002:a05:600c:3658:: with SMTP id\n\ty24mr11303916wmq.6.1624022005647; \n\tFri, 18 Jun 2021 06:13:25 -0700 (PDT)","MIME-Version":"1.0","References":"<20210618103351.1642060-1-paul.elder@ideasonboard.com>\n\t<20210618103351.1642060-4-paul.elder@ideasonboard.com>\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Fri, 18 Jun 2021 14:13:15 +0100","Message-ID":"","To":"Naushir Patuck ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH 03/14] controls: Replace AeLocked\n\twith 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":17631,"web_url":"https://patchwork.libcamera.org/comment/17631/","msgid":"","date":"2021-06-18T13:58:52","subject":"Re: [libcamera-devel] [RFC PATCH 03/14] controls: Replace AeLocked\n\twith AeState, and add AeLock","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"On Fri, 18 Jun 2021 at 14:13, David Plowman \nwrote:\n\n> Hi Naush, everyone\n>\n> On Fri, 18 Jun 2021 at 13:07, Naushir Patuck \n> wrote:\n> >\n> > Hi Paul,\n> >\n> > Thank you for your patch.\n> >\n> > On Fri, 18 Jun 2021 at 11:34, Paul Elder \n> 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> >> 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 1c1e802a..ad0132c0 100644\n> >> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> >> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> >> @@ -468,7 +468,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> >\n> > I think, strictly speaking, if agcStatus->locked == true, we should\n> return\n> > control::AeStateConverged by the definition below. David, do you agree?\n>\n> Hmm, indeed. My reading is that what we used to know as AeEnable has\n> now been renamed/replaced by AeLock, is that right? And as you say,\n> \"locked\" does not mean \"converged\" (any more).\n>\n\nYes I agree, but we now have both AeEnable and AeLock defined with this\nchange.\nPerhaps combining them to avoid redundancy makes sense? Or do folks think\nthey\ncan they serve different purposes?\n\n\n>\n> So:\n>\n> We set controls::AeLocked in the metadata according to the value that\n> we received in the corresponding control (which we now receive instead\n> of AeEnable), and\n>\n> We set controls::AeState in the metadata to AeStateConverged when\n> agcStatus->locked is true, otherwise to AeStateSearching. We should\n> probably bite the bullet and rename that \"locked\" field to\n> \"converged\", otherwise it's going to cause confusion for the rest of\n> eternity.\n>\n\nAgree, I think we should rename \"locked\" to \"converged\" in the controller\nmetadata struct.\n\nRegards,\nNaush\n\n\n>\n> David\n>\n> >\n> > Thanks,\n> > Naush\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 b47ea324..88a562a8 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> >> - 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 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> >> + - 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> >> 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 D4979BD78E\n\tfor ;\n\tFri, 18 Jun 2021 13:59:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4E8B368945;\n\tFri, 18 Jun 2021 15:59:09 +0200 (CEST)","from mail-lf1-x12e.google.com (mail-lf1-x12e.google.com\n\t[IPv6:2a00:1450:4864:20::12e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C913760298\n\tfor ;\n\tFri, 18 Jun 2021 15:59:08 +0200 (CEST)","by mail-lf1-x12e.google.com with SMTP id a11so3789872lfg.11\n\tfor ;\n\tFri, 18 Jun 2021 06:59:08 -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=\"rIFuiTpu\"; 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=TLJL4Rr24WHtNb58vUhm61daMwG3ypBeTMiO188DUUA=;\n\tb=rIFuiTpua+vBeUio4uL9PyQAZrD9kppVCt7ocHixy4yA0pyEFjyJXdJGbchjTIzI2y\n\t4v8+OIgIY3PqSaV7t2R1HEGhXlc/HsoIJYqQ4LcE2LnrTAZXRO0HAAsz+EC84XXK1eWm\n\tBylA0P7S+SHF8VQ2Klac1i257W3zzOpmpvDsZ1F3HouqagmNx/vvwhHIm0pujHgq5kn9\n\tOG8Cuih7Zlmrrypoezx5v3odAJaRePTIfJ5H8CrqcnhluB6vo9R6HJc9/y/hYorKmrkU\n\tG5GiPzbjCOebIc8Y6fOXl0MIw+x56IdLI3joUVdl8XDemdw9Sz2n+PF1G3rcRrcockaA\n\tvMqg==","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=TLJL4Rr24WHtNb58vUhm61daMwG3ypBeTMiO188DUUA=;\n\tb=krVfTrFNleK1bH9ENkKJ+zqujJy6GahxA5zeyYCwrMCBJUVDWh6N1NM1VAX0YGP0+y\n\tYUl/OnE0Z03kFK54whirBYJezdR/TRpsPePXbkDCzijCMeKn85DPp5qHiZwJyzuQDMMB\n\t5Coexs9Orq3NzYRIfYFvkQFibFvDkzCD32NaB4dy+M78xrZ28EFgLxLjtuspevitWWOq\n\tQdmvjesen4V07gFmJCRNfNQjtCUZhmWgnw9JmGtBuBdrLaX75YEDBi1DjbV3pSO8AESW\n\t/+O2sYmIC7kCPFzIClxLJVL7yQnInVD1DBI73P7SA/1nGTwJwugvNmMa6JuaYUhmMjHE\n\tbDRQ==","X-Gm-Message-State":"AOAM5323dIqXJPX9GJpoEzPujnzqhUs/NzfZWERF2TJ3MaMyF2+M+8vt\n\tj7CNPkmjKVYKv9YRBnjW5YoDu3OvuFUvvDQ68eRITZzMgfZGQQ==","X-Google-Smtp-Source":"ABdhPJzDIXyU/jL3WExAdOOf4c1TU8f4ycJaPRSERe8xHK7XNEJoMZij3FnnQ5r5Sevhr/virUm+6nbWIcZRFsSvz84=","X-Received":"by 2002:ac2:43b2:: with SMTP id\n\tt18mr3251818lfl.133.1624024748160; \n\tFri, 18 Jun 2021 06:59:08 -0700 (PDT)","MIME-Version":"1.0","References":"<20210618103351.1642060-1-paul.elder@ideasonboard.com>\n\t<20210618103351.1642060-4-paul.elder@ideasonboard.com>\n\t\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Fri, 18 Jun 2021 14:58:52 +0100","Message-ID":"","To":"David Plowman ","Content-Type":"multipart/alternative; boundary=\"0000000000003888b905c50aba9f\"","Subject":"Re: [libcamera-devel] [RFC PATCH 03/14] controls: Replace AeLocked\n\twith 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":17651,"web_url":"https://patchwork.libcamera.org/comment/17651/","msgid":"<20210621090503.GK1351869@pyrite.rasen.tech>","date":"2021-06-21T09:05:03","subject":"Re: [libcamera-devel] [RFC PATCH 03/14] controls: Replace AeLocked\n\twith 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 Naush,\n\nThank you for the feedback.\n\nOn Fri, Jun 18, 2021 at 02:58:52PM +0100, Naushir Patuck wrote:\n> \n> \n> On Fri, 18 Jun 2021 at 14:13, David Plowman \n> wrote:\n> \n> Hi Naush, everyone\n> \n> On Fri, 18 Jun 2021 at 13:07, Naushir Patuck wrote:\n> >\n> > Hi Paul,\n> >\n> > Thank you for your patch.\n> >\n> > On Fri, 18 Jun 2021 at 11:34, Paul Elder \n> 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> >>  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/\n> raspberrypi.cpp\n> >> index 1c1e802a..ad0132c0 100644\n> >> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> >> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> >> @@ -468,7 +468,10 @@ void IPARPi::reportMetadata()\n> >>\n> >>         AgcStatus *agcStatus = rpiMetadata_.GetLocked\n> (\"agc.status\");\n> >>         if (agcStatus) {\n> >> -               libcameraMetadata_.set(controls::AeLocked, agcStatus->\n> locked);\n> >> +               libcameraMetadata_.set(controls::AeState,\n> >> +                                      agcStatus->locked ?\n> >> +                                      controls::AeStateLocked :\n> >> +                                      controls::AeStateSearching);\n> >\n> >\n> > I think, strictly speaking, if agcStatus->locked == true, we should\n> return\n> > control::AeStateConverged by the definition below.  David, do you agree?\n\nOh shoot, that is indeed what I meant. I got mixed up with the status\nname :/\n\n> \n> Hmm, indeed. My reading is that what we used to know as AeEnable has\n> now been renamed/replaced by AeLock, is that right? And as you say,\n> \"locked\" does not mean \"converged\" (any more).\n> \n> \n> Yes I agree, but we now have both AeEnable and AeLock defined with this change.\n\nAeEnable is for enabling/disabling the AE routine. AeLock is to lock the\nAE values... which would normally be the same as turning off AE, but you\nwant to be able to unlock AE without having to restart the routine.\nThat's the idea afaict.\n\nSupporting AeLock is optional, of course. Same for AeState, not all of\nthe states need to be supported. You can specify which ones are\nsupported in pipeline's ControlInfoMap.\n\n\nThanks,\n\nPaul\n\n> Perhaps combining them to avoid redundancy makes sense?  Or do folks think they\n> can they serve different purposes?\n>  \n> \n> \n> So:\n> \n> We set controls::AeLocked in the metadata according to the value that\n> we received in the corresponding control (which we now receive instead\n> of AeEnable), and\n> \n> We set controls::AeState in the metadata to AeStateConverged when\n> agcStatus->locked is true, otherwise to AeStateSearching. We should\n> probably bite the bullet and rename that \"locked\" field to\n> \"converged\", otherwise it's going to cause confusion for the rest of\n> eternity.\n> \n> \n> Agree, I think we should rename \"locked\" to \"converged\" in the controller\n> metadata struct.\n> \n> Regards,\n> Naush\n>  \n> \n> \n> David\n> \n> >\n> > Thanks,\n> > Naush\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 b47ea324..88a562a8 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/\n> 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> >> -        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 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. 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\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> >> +        - 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, Converged,\n> 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> >> 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 3069BBE58C\n\tfor ;\n\tMon, 21 Jun 2021 09:05:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4F9A168937;\n\tMon, 21 Jun 2021 11:05:13 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D962D60295\n\tfor ;\n\tMon, 21 Jun 2021 11:05:11 +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 19C359DF;\n\tMon, 21 Jun 2021 11:05:09 +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=\"PZd6Cye7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1624266311;\n\tbh=4A7gZweZl1Zr5RIEVn5qkrHXYkB1ngDn5VhnVGN/pXk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=PZd6Cye7/sNDWts7N4rCKEPjcA0RTIkc7DekFo65DhQvjJYhUaTGvj0EUs/68DcIp\n\ta2yT9lnAhezWvPIO5kTzwAiSTkp6bXy4spk0DRC0ngQdnqlLLYukHu7q8hdfBM8DOD\n\tdt54Q1fzdbJjzMxDYccJC2AMT5DFPW/+O6wuDpjU=","Date":"Mon, 21 Jun 2021 18:05:03 +0900","From":"paul.elder@ideasonboard.com","To":"Naushir Patuck ","Message-ID":"<20210621090503.GK1351869@pyrite.rasen.tech>","References":"<20210618103351.1642060-1-paul.elder@ideasonboard.com>\n\t<20210618103351.1642060-4-paul.elder@ideasonboard.com>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH 03/14] controls: Replace AeLocked\n\twith 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":17652,"web_url":"https://patchwork.libcamera.org/comment/17652/","msgid":"","date":"2021-06-21T09:15:21","subject":"Re: [libcamera-devel] [RFC PATCH 03/14] controls: Replace AeLocked\n\twith 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 Paul\n\nThanks for the extra explanations.\n\nOn Mon, 21 Jun 2021 at 10:05, wrote:\n>\n> Hi Naush,\n>\n> Thank you for the feedback.\n>\n> On Fri, Jun 18, 2021 at 02:58:52PM +0100, Naushir Patuck wrote:\n> >\n> >\n> > On Fri, 18 Jun 2021 at 14:13, David Plowman \n> > wrote:\n> >\n> > Hi Naush, everyone\n> >\n> > On Fri, 18 Jun 2021 at 13:07, Naushir Patuck wrote:\n> > >\n> > > Hi Paul,\n> > >\n> > > Thank you for your patch.\n> > >\n> > > On Fri, 18 Jun 2021 at 11:34, Paul Elder \n> > 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> > >> 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/\n> > raspberrypi.cpp\n> > >> index 1c1e802a..ad0132c0 100644\n> > >> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > >> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > >> @@ -468,7 +468,10 @@ void IPARPi::reportMetadata()\n> > >>\n> > >> AgcStatus *agcStatus = rpiMetadata_.GetLocked\n> > (\"agc.status\");\n> > >> if (agcStatus) {\n> > >> - libcameraMetadata_.set(controls::AeLocked, agcStatus->\n> > locked);\n> > >> + libcameraMetadata_.set(controls::AeState,\n> > >> + agcStatus->locked ?\n> > >> + controls::AeStateLocked :\n> > >> + controls::AeStateSearching);\n> > >\n> > >\n> > > I think, strictly speaking, if agcStatus->locked == true, we should\n> > return\n> > > control::AeStateConverged by the definition below. David, do you agree?\n>\n> Oh shoot, that is indeed what I meant. I got mixed up with the status\n> name :/\n>\n> >\n> > Hmm, indeed. My reading is that what we used to know as AeEnable has\n> > now been renamed/replaced by AeLock, is that right? And as you say,\n> > \"locked\" does not mean \"converged\" (any more).\n> >\n> >\n> > Yes I agree, but we now have both AeEnable and AeLock defined with this change.\n>\n> AeEnable is for enabling/disabling the AE routine. AeLock is to lock the\n> AE values... which would normally be the same as turning off AE, but you\n> want to be able to unlock AE without having to restart the routine.\n\nI'm still struggling a bit with this, however. What would it mean to\nunlock the AE without restarting it? Isn't it then still fixed at\nwhatever values it had? Perhaps you could provide an example of this\nbehaviour which would show how it is different.\n\nSorry for still being puzzled...\n\nThanks!\nDavid\n\n> That's the idea afaict.\n>\n> Supporting AeLock is optional, of course. Same for AeState, not all of\n> the states need to be supported. You can specify which ones are\n> supported in pipeline's ControlInfoMap.\n>\n>\n> Thanks,\n>\n> Paul\n>\n> > Perhaps combining them to avoid redundancy makes sense? Or do folks think they\n> > can they serve different purposes?\n> >\n> >\n> >\n> > So:\n> >\n> > We set controls::AeLocked in the metadata according to the value that\n> > we received in the corresponding control (which we now receive instead\n> > of AeEnable), and\n> >\n> > We set controls::AeState in the metadata to AeStateConverged when\n> > agcStatus->locked is true, otherwise to AeStateSearching. We should\n> > probably bite the bullet and rename that \"locked\" field to\n> > \"converged\", otherwise it's going to cause confusion for the rest of\n> > eternity.\n> >\n> >\n> > Agree, I think we should rename \"locked\" to \"converged\" in the controller\n> > metadata struct.\n> >\n> > Regards,\n> > Naush\n> >\n> >\n> >\n> > David\n> >\n> > >\n> > > Thanks,\n> > > Naush\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 b47ea324..88a562a8 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/\n> > 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> > >> - 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 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. 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\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> > >> + - 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, Converged,\n> > 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> > >> 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 23C64C3218\n\tfor ;\n\tMon, 21 Jun 2021 09:15:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5F88968935;\n\tMon, 21 Jun 2021 11:15:34 +0200 (CEST)","from mail-wm1-x333.google.com (mail-wm1-x333.google.com\n\t[IPv6:2a00:1450:4864:20::333])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D209260295\n\tfor ;\n\tMon, 21 Jun 2021 11:15:32 +0200 (CEST)","by mail-wm1-x333.google.com with SMTP id\n\tl18-20020a1ced120000b029014c1adff1edso13304678wmh.4\n\tfor ;\n\tMon, 21 Jun 2021 02:15:32 -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=\"jhs3XkUg\"; 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=ucjKbAoE0GYOHaUZwEOrSWTE8439//kZKEUjiRHfVxo=;\n\tb=jhs3XkUgSi5VG2MWEdmGXJCyLe60E1K+qOgTUaGf8Jl4s4cRph4U4P02/I7qh86ivR\n\tlebNfSjivMWbRDYl3h5uK+Bjluu2MXoZyTl7CEJE9cH+zVkLDAq8J2gAjK69GCQ/3pEQ\n\tF05L9a6Cg9S4dBbxMMHjqSncU/FTpEOKMybs/9+Cu8Y1uK6wRYBsHKDUH2/3DRY+lAh5\n\tsuuXvb50buSaVVtCMalzL8n24izNs3himD3L+IBMyC3Hu0AM/Pazb8T/gW9NsPuSc4h5\n\tHA1RlLdlek8xkXkxg1wSWQFMG5XHpNf22muI6pZMQRW75JbSWSSOQjV6+yu4jlejk+O7\n\tTKOQ==","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=ucjKbAoE0GYOHaUZwEOrSWTE8439//kZKEUjiRHfVxo=;\n\tb=BUZRhx1TcxoBIM1qIipIeSKKxANBZexES/KUZMJadO7JwoYl54DqZghmVIIibaczQF\n\tE2qu8MPi9e+9MF6VvJIl65TUIaENuiWriVciM94S22QzGql5VuNTaSMuQRI5gwNBTdUw\n\tr6yznLGC7uHyCv5AIPgQzFrHxDM52BIrav1O94nyTiKTiao1UpLhbIR1wM9Yq1sTRh4O\n\tw1JDdZtgkXC6mNLJcpNa/2/zP+xiPNBK1T9ntfjTWphW/SBBLxPsJClUKSskTpSGaFda\n\tP3kYBx/y8yhN+zsHhHPkoaPmowA/imzM4m/55t8S7vRo17/YSH2kUlAji0+spbaBAAaE\n\tkvhQ==","X-Gm-Message-State":"AOAM5338iaHeAwYwoyY2mouBnQE/CpFvkfSLzTrSpAzY/sLJTlOMalVM\n\t3PtOPtT3SV+BZkV/7WlhFi69u3fkO3n1/+5GNh2qug==","X-Google-Smtp-Source":"ABdhPJx1pflL4fnQoK1oAmgTwKGmZPJTFCzXFkWb82hYjIyLVQ509tdDnz2081+fLAxqltUVOWG8qcoJNXyBjm1Pljo=","X-Received":"by 2002:a7b:c76a:: with SMTP id\n\tx10mr20434539wmk.135.1624266932420; \n\tMon, 21 Jun 2021 02:15:32 -0700 (PDT)","MIME-Version":"1.0","References":"<20210618103351.1642060-1-paul.elder@ideasonboard.com>\n\t<20210618103351.1642060-4-paul.elder@ideasonboard.com>\n\t\n\t\n\t\n\t<20210621090503.GK1351869@pyrite.rasen.tech>","In-Reply-To":"<20210621090503.GK1351869@pyrite.rasen.tech>","From":"David Plowman ","Date":"Mon, 21 Jun 2021 10:15:21 +0100","Message-ID":"","To":"Paul Elder ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH 03/14] controls: Replace AeLocked\n\twith 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":17701,"web_url":"https://patchwork.libcamera.org/comment/17701/","msgid":"<20210622161226.jhgillexctcakgay@uno.localdomain>","date":"2021-06-22T16:12:26","subject":"Re: [libcamera-devel] [RFC PATCH 03/14] controls: Replace AeLocked\n\twith AeState, and add AeLock","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi David,\n\nOn Mon, Jun 21, 2021 at 10:15:21AM +0100, David Plowman wrote:\n> Hi Paul\n>\n> Thanks for the extra explanations.\n>\n> On Mon, 21 Jun 2021 at 10:05, wrote:\n> >\n> > Hi Naush,\n> >\n> > Thank you for the feedback.\n> >\n> > On Fri, Jun 18, 2021 at 02:58:52PM +0100, Naushir Patuck wrote:\n> > >\n> > >\n> > > On Fri, 18 Jun 2021 at 14:13, David Plowman \n> > > wrote:\n> > >\n> > > Hi Naush, everyone\n> > >\n> > > On Fri, 18 Jun 2021 at 13:07, Naushir Patuck wrote:\n> > > >\n> > > > Hi Paul,\n> > > >\n> > > > Thank you for your patch.\n> > > >\n> > > > On Fri, 18 Jun 2021 at 11:34, Paul Elder \n> > > 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> > > >> 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/\n> > > raspberrypi.cpp\n> > > >> index 1c1e802a..ad0132c0 100644\n> > > >> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > >> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > >> @@ -468,7 +468,10 @@ void IPARPi::reportMetadata()\n> > > >>\n> > > >> AgcStatus *agcStatus = rpiMetadata_.GetLocked\n> > > (\"agc.status\");\n> > > >> if (agcStatus) {\n> > > >> - libcameraMetadata_.set(controls::AeLocked, agcStatus->\n> > > locked);\n> > > >> + libcameraMetadata_.set(controls::AeState,\n> > > >> + agcStatus->locked ?\n> > > >> + controls::AeStateLocked :\n> > > >> + controls::AeStateSearching);\n> > > >\n> > > >\n> > > > I think, strictly speaking, if agcStatus->locked == true, we should\n> > > return\n> > > > control::AeStateConverged by the definition below. David, do you agree?\n> >\n> > Oh shoot, that is indeed what I meant. I got mixed up with the status\n> > name :/\n> >\n> > >\n> > > Hmm, indeed. My reading is that what we used to know as AeEnable has\n> > > now been renamed/replaced by AeLock, is that right? And as you say,\n> > > \"locked\" does not mean \"converged\" (any more).\n> > >\n> > >\n> > > Yes I agree, but we now have both AeEnable and AeLock defined with this change.\n> >\n> > AeEnable is for enabling/disabling the AE routine. AeLock is to lock the\n> > AE values... which would normally be the same as turning off AE, but you\n> > want to be able to unlock AE without having to restart the routine.\n>\n> I'm still struggling a bit with this, however. What would it mean to\n> unlock the AE without restarting it? Isn't it then still fixed at\n> whatever values it had? Perhaps you could provide an example of this\n> behaviour which would show how it is different.\n>\n> Sorry for still being puzzled...\n\nAnd I'm sorry as well for asking possibly stupid questions, but:\n\n- It seems to me the RPi AGC routine has two Pause/Resume functions that:\n - Pause: sets fixed gain and shutter time to the last computed\n shutter and gain values\n - Resume: re-sets them to 0\n\n - In the RPi AEGC implementation, if fixed shutter and gains are set\n then those values are used\n\n - The AEGC routine is paused/resumed on controls::AeEnable on/off\n\n - When AeEnable == Off, then the only way to override the fixed\n shutter/gain is by using the ExposureTime and AnalogueGain controls\n (ie manual shutter/gain control)\n\nSo in you implementation disabling AE has the effect that the AEGC\nroutine uses the last computed values until a new shutter/gain value\nis set by the application (through what is usually called \"manual\" mode).\n\nThe difference between your implementation and a new one that uses\nAeLocked and AeEnable is, in my understanding, that when the AeLocked\nis set the AEGC routine is still in control of the shutter/gain which\ngets \"fixed\" as you're doing, but it does not accept manual control of\nthe shutter/gain like you're doing unless the AEGC is disabled.\n\nThis seems to be suggested by the following paragraph in the Android\ndocumentation:\n\n-------------------------------------------------------------------------------\n\nIf an application is switching between automatic and manual control\nand wishes to eliminate any flicker during the switch, the following\nprocedure is recommended:\n\nStarting in auto-AE mode:\n- Lock AE\n- Wait for the first result to be output that has the AE locked\n- Copy exposure settings from that result into a request, set the request to manual AE\n- Submit the capture request, proceed to run manual AE as desired.\n\nSee android.control.aeState for AE lock related state transition details.\n-------------------------------------------------------------------------------\n\nIf we were able to report the AEGC convergence state I think the same\ncould be done without AeLock, by disabling AEGC once it has converged\nand continue from there with manual exposure. But my understanding is that\nhaving a lock controls guarantees that once the AEGC has\nconverged it won't change, so that the in-flight requests that are\ncompleted before the first one with manual AE control is processed\nwon't have floating AEGC values,\n\n(Let me try to draw it down, hoping it render ok with your client,\notherwise https://paste.debian.net/1202043/)\n\nQueue of\nsubmitted Completed AeState in the\nRequests Requests completed Request\n-----------------------------------------------------------------\n\n0 (AeLock)\n1\n2\n3 0 Searching\n4 1 Converged\n5 (AeDisable + 2 Locked <- COPY AE VALUES\n Manual AE with 3 Locked\n settings from #2) 4 Locked\n 5 Use Manual AE\n\nWithout a Lock the AEGC routine would keep adjusting its values for\nrequests 3 and 4. Request 5 will contain settings copied from Request\n2 so a small hiccup might happen. Of course, if the number of frames\nrequired to get to a converged state is longer, the duration of the\npossible AE adjustment period is longer, making the flickering more\nvisible ?\n\nDoes this make sense ?\n\nThe android.control.aeMode documentation also reports that:\n\n-----------------------------------------------------------------\nNote that auto-white balance (AWB) and auto-focus (AF) behavior is\ndevice dependent when AE is in OFF mode. To have consistent behavior\nacross different devices, it is recommended to either set AWB and AF to\nOFF mode or lock AWB and AF before setting AE to OFF.See\nandroid.control.awbMode,\nandroid.control.afMode,android.control.awbLock, and\nandroid.control.afTrigger for more details.\n-----------------------------------------------------------------\n\nSuggesting that the AF AWB and AE locks might play a role on the interactions\nbetween the three algorithms.\n\nDoes this make sense and provide a bit more of context about what\nAeLock could be used for ?\n\nThanks\n j\n\n\n>\n> Thanks!\n> David\n>\n> > That's the idea afaict.\n> >\n> > Supporting AeLock is optional, of course. Same for AeState, not all of\n> > the states need to be supported. You can specify which ones are\n> > supported in pipeline's ControlInfoMap.\n> >\n> >\n> > Thanks,\n> >\n> > Paul\n> >\n> > > Perhaps combining them to avoid redundancy makes sense? Or do folks think they\n> > > can they serve different purposes?\n> > >\n> > >\n> > >\n> > > So:\n> > >\n> > > We set controls::AeLocked in the metadata according to the value that\n> > > we received in the corresponding control (which we now receive instead\n> > > of AeEnable), and\n> > >\n> > > We set controls::AeState in the metadata to AeStateConverged when\n> > > agcStatus->locked is true, otherwise to AeStateSearching. We should\n> > > probably bite the bullet and rename that \"locked\" field to\n> > > \"converged\", otherwise it's going to cause confusion for the rest of\n> > > eternity.\n> > >\n> > >\n> > > Agree, I think we should rename \"locked\" to \"converged\" in the controller\n> > > metadata struct.\n> > >\n> > > Regards,\n> > > Naush\n> > >\n> > >\n> > >\n> > > David\n> > >\n> > > >\n> > > > Thanks,\n> > > > Naush\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 b47ea324..88a562a8 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/\n> > > 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> > > >> - 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 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. 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\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> > > >> + - 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, Converged,\n> > > 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> > > >> 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 1891AC321A\n\tfor ;\n\tTue, 22 Jun 2021 16:11:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5944968935;\n\tTue, 22 Jun 2021 18:11:40 +0200 (CEST)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3179160292\n\tfor ;\n\tTue, 22 Jun 2021 18:11:39 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 1488D20002;\n\tTue, 22 Jun 2021 16:11:37 +0000 (UTC)"],"Date":"Tue, 22 Jun 2021 18:12:26 +0200","From":"Jacopo Mondi ","To":"David Plowman ","Message-ID":"<20210622161226.jhgillexctcakgay@uno.localdomain>","References":"<20210618103351.1642060-1-paul.elder@ideasonboard.com>\n\t<20210618103351.1642060-4-paul.elder@ideasonboard.com>\n\t\n\t\n\t\n\t<20210621090503.GK1351869@pyrite.rasen.tech>\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 03/14] controls: Replace AeLocked\n\twith 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":17710,"web_url":"https://patchwork.libcamera.org/comment/17710/","msgid":"","date":"2021-06-23T07:57:04","subject":"Re: [libcamera-devel] [RFC PATCH 03/14] controls: Replace AeLocked\n\twith 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 Jacopo\n\nThanks for taking more time to explain this!\n\nOn Tue, 22 Jun 2021 at 17:11, Jacopo Mondi wrote:\n>\n> Hi David,\n>\n> On Mon, Jun 21, 2021 at 10:15:21AM +0100, David Plowman wrote:\n> > Hi Paul\n> >\n> > Thanks for the extra explanations.\n> >\n> > On Mon, 21 Jun 2021 at 10:05, wrote:\n> > >\n> > > Hi Naush,\n> > >\n> > > Thank you for the feedback.\n> > >\n> > > On Fri, Jun 18, 2021 at 02:58:52PM +0100, Naushir Patuck wrote:\n> > > >\n> > > >\n> > > > On Fri, 18 Jun 2021 at 14:13, David Plowman \n> > > > wrote:\n> > > >\n> > > > Hi Naush, everyone\n> > > >\n> > > > On Fri, 18 Jun 2021 at 13:07, Naushir Patuck wrote:\n> > > > >\n> > > > > Hi Paul,\n> > > > >\n> > > > > Thank you for your patch.\n> > > > >\n> > > > > On Fri, 18 Jun 2021 at 11:34, Paul Elder \n> > > > 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> > > > >> 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/\n> > > > raspberrypi.cpp\n> > > > >> index 1c1e802a..ad0132c0 100644\n> > > > >> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > >> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > >> @@ -468,7 +468,10 @@ void IPARPi::reportMetadata()\n> > > > >>\n> > > > >> AgcStatus *agcStatus = rpiMetadata_.GetLocked\n> > > > (\"agc.status\");\n> > > > >> if (agcStatus) {\n> > > > >> - libcameraMetadata_.set(controls::AeLocked, agcStatus->\n> > > > locked);\n> > > > >> + libcameraMetadata_.set(controls::AeState,\n> > > > >> + agcStatus->locked ?\n> > > > >> + controls::AeStateLocked :\n> > > > >> + controls::AeStateSearching);\n> > > > >\n> > > > >\n> > > > > I think, strictly speaking, if agcStatus->locked == true, we should\n> > > > return\n> > > > > control::AeStateConverged by the definition below. David, do you agree?\n> > >\n> > > Oh shoot, that is indeed what I meant. I got mixed up with the status\n> > > name :/\n> > >\n> > > >\n> > > > Hmm, indeed. My reading is that what we used to know as AeEnable has\n> > > > now been renamed/replaced by AeLock, is that right? And as you say,\n> > > > \"locked\" does not mean \"converged\" (any more).\n> > > >\n> > > >\n> > > > Yes I agree, but we now have both AeEnable and AeLock defined with this change.\n> > >\n> > > AeEnable is for enabling/disabling the AE routine. AeLock is to lock the\n> > > AE values... which would normally be the same as turning off AE, but you\n> > > want to be able to unlock AE without having to restart the routine.\n> >\n> > I'm still struggling a bit with this, however. What would it mean to\n> > unlock the AE without restarting it? Isn't it then still fixed at\n> > whatever values it had? Perhaps you could provide an example of this\n> > behaviour which would show how it is different.\n> >\n> > Sorry for still being puzzled...\n>\n> And I'm sorry as well for asking possibly stupid questions, but:\n>\n> - It seems to me the RPi AGC routine has two Pause/Resume functions that:\n> - Pause: sets fixed gain and shutter time to the last computed\n> shutter and gain values\n> - Resume: re-sets them to 0\n>\n> - In the RPi AEGC implementation, if fixed shutter and gains are set\n> then those values are used\n>\n> - The AEGC routine is paused/resumed on controls::AeEnable on/off\n>\n> - When AeEnable == Off, then the only way to override the fixed\n> shutter/gain is by using the ExposureTime and AnalogueGain controls\n> (ie manual shutter/gain control)\n>\n> So in you implementation disabling AE has the effect that the AEGC\n> routine uses the last computed values until a new shutter/gain value\n> is set by the application (through what is usually called \"manual\" mode).\n>\n> The difference between your implementation and a new one that uses\n> AeLocked and AeEnable is, in my understanding, that when the AeLocked\n> is set the AEGC routine is still in control of the shutter/gain which\n> gets \"fixed\" as you're doing, but it does not accept manual control of\n> the shutter/gain like you're doing unless the AEGC is disabled.\n>\n> This seems to be suggested by the following paragraph in the Android\n> documentation:\n>\n> -------------------------------------------------------------------------------\n>\n> If an application is switching between automatic and manual control\n> and wishes to eliminate any flicker during the switch, the following\n> procedure is recommended:\n>\n> Starting in auto-AE mode:\n> - Lock AE\n> - Wait for the first result to be output that has the AE locked\n> - Copy exposure settings from that result into a request, set the request to manual AE\n> - Submit the capture request, proceed to run manual AE as desired.\n>\n> See android.control.aeState for AE lock related state transition details.\n> -------------------------------------------------------------------------------\n>\n> If we were able to report the AEGC convergence state I think the same\n> could be done without AeLock, by disabling AEGC once it has converged\n> and continue from there with manual exposure. But my understanding is that\n> having a lock controls guarantees that once the AEGC has\n> converged it won't change, so that the in-flight requests that are\n> completed before the first one with manual AE control is processed\n> won't have floating AEGC values,\n>\n> (Let me try to draw it down, hoping it render ok with your client,\n> otherwise https://paste.debian.net/1202043/)\n>\n> Queue of\n> submitted Completed AeState in the\n> Requests Requests completed Request\n> -----------------------------------------------------------------\n>\n> 0 (AeLock)\n> 1\n> 2\n> 3 0 Searching\n> 4 1 Converged\n> 5 (AeDisable + 2 Locked <- COPY AE VALUES\n> Manual AE with 3 Locked\n> settings from #2) 4 Locked\n> 5 Use Manual AE\n\nAre we saying that AeDisable means \"stop adjusting the AEC/AGC\nimmediately, whether it's converged or not\", but AeLock means \"wait\nuntil it's converged and then immediately stop adjusting the AEC/AGC\"?\n\nThanks!\nDavid\n\n>\n> Without a Lock the AEGC routine would keep adjusting its values for\n> requests 3 and 4. Request 5 will contain settings copied from Request\n> 2 so a small hiccup might happen. Of course, if the number of frames\n> required to get to a converged state is longer, the duration of the\n> possible AE adjustment period is longer, making the flickering more\n> visible ?\n>\n> Does this make sense ?\n>\n> The android.control.aeMode documentation also reports that:\n>\n> -----------------------------------------------------------------\n> Note that auto-white balance (AWB) and auto-focus (AF) behavior is\n> device dependent when AE is in OFF mode. To have consistent behavior\n> across different devices, it is recommended to either set AWB and AF to\n> OFF mode or lock AWB and AF before setting AE to OFF.See\n> android.control.awbMode,\n> android.control.afMode,android.control.awbLock, and\n> android.control.afTrigger for more details.\n> -----------------------------------------------------------------\n>\n> Suggesting that the AF AWB and AE locks might play a role on the interactions\n> between the three algorithms.\n>\n> Does this make sense and provide a bit more of context about what\n> AeLock could be used for ?\n>\n> Thanks\n> j\n>\n>\n> >\n> > Thanks!\n> > David\n> >\n> > > That's the idea afaict.\n> > >\n> > > Supporting AeLock is optional, of course. Same for AeState, not all of\n> > > the states need to be supported. You can specify which ones are\n> > > supported in pipeline's ControlInfoMap.\n> > >\n> > >\n> > > Thanks,\n> > >\n> > > Paul\n> > >\n> > > > Perhaps combining them to avoid redundancy makes sense? Or do folks think they\n> > > > can they serve different purposes?\n> > > >\n> > > >\n> > > >\n> > > > So:\n> > > >\n> > > > We set controls::AeLocked in the metadata according to the value that\n> > > > we received in the corresponding control (which we now receive instead\n> > > > of AeEnable), and\n> > > >\n> > > > We set controls::AeState in the metadata to AeStateConverged when\n> > > > agcStatus->locked is true, otherwise to AeStateSearching. We should\n> > > > probably bite the bullet and rename that \"locked\" field to\n> > > > \"converged\", otherwise it's going to cause confusion for the rest of\n> > > > eternity.\n> > > >\n> > > >\n> > > > Agree, I think we should rename \"locked\" to \"converged\" in the controller\n> > > > metadata struct.\n> > > >\n> > > > Regards,\n> > > > Naush\n> > > >\n> > > >\n> > > >\n> > > > David\n> > > >\n> > > > >\n> > > > > Thanks,\n> > > > > Naush\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 b47ea324..88a562a8 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/\n> > > > 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> > > > >> - 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 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. 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\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> > > > >> + - 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, Converged,\n> > > > 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> > > > >> 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 5759CC321B\n\tfor ;\n\tWed, 23 Jun 2021 07:57:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9DDDC68936;\n\tWed, 23 Jun 2021 09:57:17 +0200 (CEST)","from mail-wm1-x32e.google.com (mail-wm1-x32e.google.com\n\t[IPv6:2a00:1450:4864:20::32e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 986B768932\n\tfor ;\n\tWed, 23 Jun 2021 09:57:15 +0200 (CEST)","by mail-wm1-x32e.google.com with SMTP id\n\tu8-20020a7bcb080000b02901e44e9caa2aso578511wmj.4\n\tfor ;\n\tWed, 23 Jun 2021 00:57:15 -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=\"j/9OGbpE\"; 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=m75nKRTbk8x9l9PJYAWLxxD8kg6XCKIO5Bjgx+o3BM0=;\n\tb=j/9OGbpEQzPNMCmJREVEuTVq8obogha+54UVpiLAg7jyQiQNhpH/x7M1zGJJo5kyuq\n\th+P82fPCAelU5Y0t0nKVZJ+0gJrjMBiNSBSZqP3qWeVNhsbP35Gj2GVXeUX900QYh4VA\n\tT8+BX2TYSD5eq9hCxQ03eRzqNfJR1kfy6+QI4a4kCu8Lx0ByhNGctJ0DG5NHPJBTaazK\n\t24UzEKXHOC/YnKzADwznLF8PiIu/n4Jwbe4YSVu/mFRx4EmTxFmMf1V7RaZZ/QCqmXQ5\n\tBF3b5lQPqgDHho5IuBZCpgRd4Vlun8xbfE9GYDOSE+y2+k4MI+thD93kFFLN13d9AToj\n\t8LDw==","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=m75nKRTbk8x9l9PJYAWLxxD8kg6XCKIO5Bjgx+o3BM0=;\n\tb=GyzP3MRLp513tblgQMy+ArGeoTo4iAKcVjW03TgBT8gN/+6BKLmrb3LZ4VLJ7CN+4K\n\ttV5OXglDl6D7OVgsA2EbjY0KXqDDyAscYfthJFDKNZoZPAzPNB3LQNKzo/fW1WcfUeHd\n\tLXwUIy+k7D7QfIRp6szLa3bP3pdpHh8lWzUcFfAu2AlSephlmhKaxAVNBzGB6j3wayRv\n\tSsgn4koNXAQyolJ0rsJfTkT5wWGC1Xxz0ONU1wSZ0p5culSaeYgo+lycHpSMamUoqu1d\n\tyy5w/V6IYd0WzBgkevJ90/pBPeSU6AzqC2EcgbcffGM+5UckCwHEc0pwCyDiHQ/6iNwT\n\txNmA==","X-Gm-Message-State":"AOAM532V1jpXZklNsLXxQ3XAoHN6ds4aMXQQSd1IQBV0lsdkuc7k1+l7\n\tbbkE9hFzKGjYXjcjluZDRiYI1dzHuAinahzdYwEtXA==","X-Google-Smtp-Source":"ABdhPJy5A5KmCztjVL9fhFwJRiqKJz8NfrjY6wmLqSJHot+BK5W8t0LxJTH7D5KhVzdj+SHhnRFLh8yfTRHsPHEmuUU=","X-Received":"by 2002:a05:600c:2184:: with SMTP id\n\te4mr8904067wme.40.1624435035182; \n\tWed, 23 Jun 2021 00:57:15 -0700 (PDT)","MIME-Version":"1.0","References":"<20210618103351.1642060-1-paul.elder@ideasonboard.com>\n\t<20210618103351.1642060-4-paul.elder@ideasonboard.com>\n\t\n\t\n\t\n\t<20210621090503.GK1351869@pyrite.rasen.tech>\n\t\n\t<20210622161226.jhgillexctcakgay@uno.localdomain>","In-Reply-To":"<20210622161226.jhgillexctcakgay@uno.localdomain>","From":"David Plowman ","Date":"Wed, 23 Jun 2021 08:57:04 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH 03/14] controls: Replace AeLocked\n\twith 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":17717,"web_url":"https://patchwork.libcamera.org/comment/17717/","msgid":"<20210623081303.kdzuwna4ilpgap3m@uno.localdomain>","date":"2021-06-23T08:13:03","subject":"Re: [libcamera-devel] [RFC PATCH 03/14] controls: Replace AeLocked\n\twith AeState, and add AeLock","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi David,\n\nOn Wed, Jun 23, 2021 at 08:57:04AM +0100, David Plowman wrote:\n> Hi Jacopo\n>\n> Thanks for taking more time to explain this!\n\nI'm just trying to wrap my head around this as well, it's far from being an\n\"explanation\" :)\n>\n> On Tue, 22 Jun 2021 at 17:11, Jacopo Mondi wrote:\n> >\n> > Hi David,\n> >\n> > On Mon, Jun 21, 2021 at 10:15:21AM +0100, David Plowman wrote:\n> > > Hi Paul\n> > >\n> > > Thanks for the extra explanations.\n> > >\n> > > On Mon, 21 Jun 2021 at 10:05, wrote:\n> > > >\n> > > > Hi Naush,\n> > > >\n> > > > Thank you for the feedback.\n> > > >\n> > > > On Fri, Jun 18, 2021 at 02:58:52PM +0100, Naushir Patuck wrote:\n> > > > >\n> > > > >\n> > > > > On Fri, 18 Jun 2021 at 14:13, David Plowman \n> > > > > wrote:\n> > > > >\n> > > > > Hi Naush, everyone\n> > > > >\n> > > > > On Fri, 18 Jun 2021 at 13:07, Naushir Patuck wrote:\n> > > > > >\n> > > > > > Hi Paul,\n> > > > > >\n> > > > > > Thank you for your patch.\n> > > > > >\n> > > > > > On Fri, 18 Jun 2021 at 11:34, Paul Elder \n> > > > > 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> > > > > >> 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/\n> > > > > raspberrypi.cpp\n> > > > > >> index 1c1e802a..ad0132c0 100644\n> > > > > >> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > > >> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > > >> @@ -468,7 +468,10 @@ void IPARPi::reportMetadata()\n> > > > > >>\n> > > > > >> AgcStatus *agcStatus = rpiMetadata_.GetLocked\n> > > > > (\"agc.status\");\n> > > > > >> if (agcStatus) {\n> > > > > >> - libcameraMetadata_.set(controls::AeLocked, agcStatus->\n> > > > > locked);\n> > > > > >> + libcameraMetadata_.set(controls::AeState,\n> > > > > >> + agcStatus->locked ?\n> > > > > >> + controls::AeStateLocked :\n> > > > > >> + controls::AeStateSearching);\n> > > > > >\n> > > > > >\n> > > > > > I think, strictly speaking, if agcStatus->locked == true, we should\n> > > > > return\n> > > > > > control::AeStateConverged by the definition below. David, do you agree?\n> > > >\n> > > > Oh shoot, that is indeed what I meant. I got mixed up with the status\n> > > > name :/\n> > > >\n> > > > >\n> > > > > Hmm, indeed. My reading is that what we used to know as AeEnable has\n> > > > > now been renamed/replaced by AeLock, is that right? And as you say,\n> > > > > \"locked\" does not mean \"converged\" (any more).\n> > > > >\n> > > > >\n> > > > > Yes I agree, but we now have both AeEnable and AeLock defined with this change.\n> > > >\n> > > > AeEnable is for enabling/disabling the AE routine. AeLock is to lock the\n> > > > AE values... which would normally be the same as turning off AE, but you\n> > > > want to be able to unlock AE without having to restart the routine.\n> > >\n> > > I'm still struggling a bit with this, however. What would it mean to\n> > > unlock the AE without restarting it? Isn't it then still fixed at\n> > > whatever values it had? Perhaps you could provide an example of this\n> > > behaviour which would show how it is different.\n> > >\n> > > Sorry for still being puzzled...\n> >\n> > And I'm sorry as well for asking possibly stupid questions, but:\n> >\n> > - It seems to me the RPi AGC routine has two Pause/Resume functions that:\n> > - Pause: sets fixed gain and shutter time to the last computed\n> > shutter and gain values\n> > - Resume: re-sets them to 0\n> >\n> > - In the RPi AEGC implementation, if fixed shutter and gains are set\n> > then those values are used\n> >\n> > - The AEGC routine is paused/resumed on controls::AeEnable on/off\n> >\n> > - When AeEnable == Off, then the only way to override the fixed\n> > shutter/gain is by using the ExposureTime and AnalogueGain controls\n> > (ie manual shutter/gain control)\n> >\n> > So in you implementation disabling AE has the effect that the AEGC\n> > routine uses the last computed values until a new shutter/gain value\n> > is set by the application (through what is usually called \"manual\" mode).\n> >\n> > The difference between your implementation and a new one that uses\n> > AeLocked and AeEnable is, in my understanding, that when the AeLocked\n> > is set the AEGC routine is still in control of the shutter/gain which\n> > gets \"fixed\" as you're doing, but it does not accept manual control of\n> > the shutter/gain like you're doing unless the AEGC is disabled.\n> >\n> > This seems to be suggested by the following paragraph in the Android\n> > documentation:\n> >\n> > -------------------------------------------------------------------------------\n> >\n> > If an application is switching between automatic and manual control\n> > and wishes to eliminate any flicker during the switch, the following\n> > procedure is recommended:\n> >\n> > Starting in auto-AE mode:\n> > - Lock AE\n> > - Wait for the first result to be output that has the AE locked\n> > - Copy exposure settings from that result into a request, set the request to manual AE\n> > - Submit the capture request, proceed to run manual AE as desired.\n> >\n> > See android.control.aeState for AE lock related state transition details.\n> > -------------------------------------------------------------------------------\n> >\n> > If we were able to report the AEGC convergence state I think the same\n> > could be done without AeLock, by disabling AEGC once it has converged\n> > and continue from there with manual exposure. But my understanding is that\n> > having a lock controls guarantees that once the AEGC has\n> > converged it won't change, so that the in-flight requests that are\n> > completed before the first one with manual AE control is processed\n> > won't have floating AEGC values,\n> >\n> > (Let me try to draw it down, hoping it render ok with your client,\n> > otherwise https://paste.debian.net/1202043/)\n> >\n> > Queue of\n> > submitted Completed AeState in the\n> > Requests Requests completed Request\n> > -----------------------------------------------------------------\n> >\n> > 0 (AeLock)\n> > 1\n> > 2\n> > 3 0 Searching\n> > 4 1 Converged\n> > 5 (AeDisable + 2 Locked <- COPY AE VALUES\n> > Manual AE with 3 Locked\n> > settings from #2) 4 Locked\n> > 5 Use Manual AE\n>\n> Are we saying that AeDisable means \"stop adjusting the AEC/AGC\n> immediately, whether it's converged or not\", but AeLock means \"wait\n> until it's converged and then immediately stop adjusting the AEC/AGC\"?\n\nIf AeLock is set on a Converged state then yes, the values are not\nchanged until the lock is lifted.\n\nDoes the transition table repoted here helps ?\nhttps://developer.android.com/reference/android/hardware/camera2/CaptureResult#CONTROL_AE_STATE\n\nThanks\n j\n\n>\n> Thanks!\n> David\n>\n> >\n> > Without a Lock the AEGC routine would keep adjusting its values for\n> > requests 3 and 4. Request 5 will contain settings copied from Request\n> > 2 so a small hiccup might happen. Of course, if the number of frames\n> > required to get to a converged state is longer, the duration of the\n> > possible AE adjustment period is longer, making the flickering more\n> > visible ?\n> >\n> > Does this make sense ?\n> >\n> > The android.control.aeMode documentation also reports that:\n> >\n> > -----------------------------------------------------------------\n> > Note that auto-white balance (AWB) and auto-focus (AF) behavior is\n> > device dependent when AE is in OFF mode. To have consistent behavior\n> > across different devices, it is recommended to either set AWB and AF to\n> > OFF mode or lock AWB and AF before setting AE to OFF.See\n> > android.control.awbMode,\n> > android.control.afMode,android.control.awbLock, and\n> > android.control.afTrigger for more details.\n> > -----------------------------------------------------------------\n> >\n> > Suggesting that the AF AWB and AE locks might play a role on the interactions\n> > between the three algorithms.\n> >\n> > Does this make sense and provide a bit more of context about what\n> > AeLock could be used for ?\n> >\n> > Thanks\n> > j\n> >\n> >\n> > >\n> > > Thanks!\n> > > David\n> > >\n> > > > That's the idea afaict.\n> > > >\n> > > > Supporting AeLock is optional, of course. Same for AeState, not all of\n> > > > the states need to be supported. You can specify which ones are\n> > > > supported in pipeline's ControlInfoMap.\n> > > >\n> > > >\n> > > > Thanks,\n> > > >\n> > > > Paul\n> > > >\n> > > > > Perhaps combining them to avoid redundancy makes sense? Or do folks think they\n> > > > > can they serve different purposes?\n> > > > >\n> > > > >\n> > > > >\n> > > > > So:\n> > > > >\n> > > > > We set controls::AeLocked in the metadata according to the value that\n> > > > > we received in the corresponding control (which we now receive instead\n> > > > > of AeEnable), and\n> > > > >\n> > > > > We set controls::AeState in the metadata to AeStateConverged when\n> > > > > agcStatus->locked is true, otherwise to AeStateSearching. We should\n> > > > > probably bite the bullet and rename that \"locked\" field to\n> > > > > \"converged\", otherwise it's going to cause confusion for the rest of\n> > > > > eternity.\n> > > > >\n> > > > >\n> > > > > Agree, I think we should rename \"locked\" to \"converged\" in the controller\n> > > > > metadata struct.\n> > > > >\n> > > > > Regards,\n> > > > > Naush\n> > > > >\n> > > > >\n> > > > >\n> > > > > David\n> > > > >\n> > > > > >\n> > > > > > Thanks,\n> > > > > > Naush\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 b47ea324..88a562a8 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/\n> > > > > 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> > > > > >> - 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 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. 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\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> > > > > >> + - 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, Converged,\n> > > > > 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> > > > > >> 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 D44D8C321A\n\tfor ;\n\tWed, 23 Jun 2021 08:12:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2280E68936;\n\tWed, 23 Jun 2021 10:12:17 +0200 (CEST)","from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6BE6168932\n\tfor ;\n\tWed, 23 Jun 2021 10:12:15 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay12.mail.gandi.net (Postfix) with ESMTPSA id 9F68A200012;\n\tWed, 23 Jun 2021 08:12:14 +0000 (UTC)"],"Date":"Wed, 23 Jun 2021 10:13:03 +0200","From":"Jacopo Mondi ","To":"David Plowman ","Message-ID":"<20210623081303.kdzuwna4ilpgap3m@uno.localdomain>","References":"<20210618103351.1642060-1-paul.elder@ideasonboard.com>\n\t<20210618103351.1642060-4-paul.elder@ideasonboard.com>\n\t\n\t\n\t\n\t<20210621090503.GK1351869@pyrite.rasen.tech>\n\t\n\t<20210622161226.jhgillexctcakgay@uno.localdomain>\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 03/14] controls: Replace AeLocked\n\twith 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":17719,"web_url":"https://patchwork.libcamera.org/comment/17719/","msgid":"","date":"2021-06-23T09:06:00","subject":"Re: [libcamera-devel] [RFC PATCH 03/14] controls: Replace AeLocked\n\twith AeState, and add AeLock","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Jacopo and David,\n\nOn Wed, 23 Jun 2021 at 09:12, Jacopo Mondi wrote:\n\n\n\n\n> >\n> > Are we saying that AeDisable means \"stop adjusting the AEC/AGC\n> > immediately, whether it's converged or not\", but AeLock means \"wait\n> > until it's converged and then immediately stop adjusting the AEC/AGC\"?\n>\n> If AeLock is set on a Converged state then yes, the values are not\n> changed until the lock is lifted.\n>\n> Does the transition table repoted here helps ?\n>\n> https://developer.android.com/reference/android/hardware/camera2/CaptureResult#CONTROL_AE_STATE\n>\n> Thanks\n> j\n>\n\nHaving a brief look at the above document, I think\nJacopo's description makes\nsense now. Enabling AeLock locks the current AE computed values, whether\nthe algorithm has converged or not. However, with AeLock set to on, other\ncontrols are effectively ignored by the AE (e.g. aePrecaptureTrigger, maybe\nother things as well). Not sure why this is the case, as the application\nsurely\nknows what state it wants the AE to be in...?\n\nWe don't deal with pre-capture sequences in RPi AE, so for us AeLock and\nAeDisable are actually the same thing. Thinking about it, even with a\npre-capture\nsequence, we could probably say the same. This may also be the case for\nother vendor AE algorithms, but Android makes these states explicit.\n\n Or it could be that I've just completely misunderstood that document :-)\n\nNaush","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 A6539C321A\n\tfor ;\n\tWed, 23 Jun 2021 09:06:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1EBA568935;\n\tWed, 23 Jun 2021 11:06:19 +0200 (CEST)","from mail-lf1-x133.google.com (mail-lf1-x133.google.com\n\t[IPv6:2a00:1450:4864:20::133])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F0A7868931\n\tfor ;\n\tWed, 23 Jun 2021 11:06:16 +0200 (CEST)","by mail-lf1-x133.google.com with SMTP id f30so2935280lfj.1\n\tfor ;\n\tWed, 23 Jun 2021 02:06:16 -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=\"ZqZABr81\"; 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=Lc8rRnkg19J8wz8hGECWrHXRLvzocqPtb7bf6l4Ta10=;\n\tb=ZqZABr8139Q8biv6+rskJcYYAH3OSMtz4C+wrpg8jTgF0668WrOirjkg75np4EY812\n\tzNe4juvHkYUGTndhhkIqAmsl1bmVSGoQkEucoLIv54VCZSAdERX+FyRLlG7EMJQtBNaW\n\tak4qbVCuM+HtRL1KtsuHTys6+PfJCbiP2HKLIAo9VBvoQuGYfXO2Su9B1jhlTm4sJPwB\n\tkGvCmcubwSlY2zNVvQHnxuDv0VE/pOuUdJeibn3nniPFKHloXV6lxdUgECnZRZZ6df39\n\tcFTDZb+6q3OJ/QlsBDIfuBPG4+lP0xVoZ5xtIeSvBLVW/N3rZQgSfrohOIHsFssbhZNk\n\tpK8g==","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=Lc8rRnkg19J8wz8hGECWrHXRLvzocqPtb7bf6l4Ta10=;\n\tb=CLmL08LIqPCvH08fXrmzzj2NaYrD1H6oBWxJYtHZ3S8FBFoTF6umOBvpT7LYLVK89z\n\tv8snMhe/H+1djbHPjkwmvN7JplVjRaPCLMcyofm4fDwwqYZVuVAZMwsyYO+YQNKtDBW9\n\tXfyDAC9Ib4BOQLayFsemBPZCmoNyPi0ax46XUYoc3wWGF4kS7RP0GyT2mheI7SV1QDPG\n\tvyewWkYc3xgwaO2hio6LFFg8PHsoL2E+M4Tu9yxdY2gmRQB3yRkv04gbmyCKijeBI7Bk\n\t9cBmkpVxbmVmiMdsuTJezT5pQHpajgz7I/aPmRvQWzEBm2ERWdKlZG3lMVHDISr0lO9m\n\tEXaw==","X-Gm-Message-State":"AOAM530p657l28YQSq67EJIem1sAcnCiuiVR7R1mnOpGB+MVou8WBtCN\n\tHZAx2JpWZLtxUhr+cj7rGQSJjzLKQPv5qC3FvJlbDQ==","X-Google-Smtp-Source":"ABdhPJzTBhWK1fW3v4hD78639sjdntxjVJnZbfZN2EKQiI230Du2lKABcaZGDLEMLPJxlXFi60FkXJt7RgV0L7nUpBg=","X-Received":"by 2002:ac2:4db6:: with SMTP id\n\th22mr5987558lfe.171.1624439176229; \n\tWed, 23 Jun 2021 02:06:16 -0700 (PDT)","MIME-Version":"1.0","References":"<20210618103351.1642060-1-paul.elder@ideasonboard.com>\n\t<20210618103351.1642060-4-paul.elder@ideasonboard.com>\n\t\n\t\n\t\n\t<20210621090503.GK1351869@pyrite.rasen.tech>\n\t\n\t<20210622161226.jhgillexctcakgay@uno.localdomain>\n\t\n\t<20210623081303.kdzuwna4ilpgap3m@uno.localdomain>","In-Reply-To":"<20210623081303.kdzuwna4ilpgap3m@uno.localdomain>","From":"Naushir Patuck ","Date":"Wed, 23 Jun 2021 10:06:00 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"multipart/alternative; boundary=\"0000000000000f07dc05c56b380b\"","Subject":"Re: [libcamera-devel] [RFC PATCH 03/14] controls: Replace AeLocked\n\twith 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":17723,"web_url":"https://patchwork.libcamera.org/comment/17723/","msgid":"<20210623103335.6gpkateh66n7aiho@uno.localdomain>","date":"2021-06-23T10:33:35","subject":"Re: [libcamera-devel] [RFC PATCH 03/14] controls: Replace AeLocked\n\twith AeState, and add AeLock","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Naush,\n\nOn Wed, Jun 23, 2021 at 10:06:00AM +0100, Naushir Patuck wrote:\n> Hi Jacopo and David,\n>\n> On Wed, 23 Jun 2021 at 09:12, Jacopo Mondi wrote:\n>\n> \n>\n>\n> > >\n> > > Are we saying that AeDisable means \"stop adjusting the AEC/AGC\n> > > immediately, whether it's converged or not\", but AeLock means \"wait\n> > > until it's converged and then immediately stop adjusting the AEC/AGC\"?\n> >\n> > If AeLock is set on a Converged state then yes, the values are not\n> > changed until the lock is lifted.\n> >\n> > Does the transition table repoted here helps ?\n> >\n> > https://developer.android.com/reference/android/hardware/camera2/CaptureResult#CONTROL_AE_STATE\n> >\n> > Thanks\n> > j\n> >\n>\n> Having a brief look at the above document, I think\n> Jacopo's description makes\n> sense now. Enabling AeLock locks the current AE computed values, whether\n> the algorithm has converged or not. However, with AeLock set to on, other\n> controls are effectively ignored by the AE (e.g. aePrecaptureTrigger, maybe\n> other things as well). Not sure why this is the case, as the application\n> surely\n> knows what state it wants the AE to be in...?\n\nSo my previous example is slightly wrong, as the AeLock was meant to\nbe set only once the AE has converged, not in request #0.\n\n>\n> We don't deal with pre-capture sequences in RPi AE, so for us AeLock and\n> AeDisable are actually the same thing. Thinking about it, even with a\n> pre-capture\n> sequence, we could probably say the same. This may also be the case for\n> other vendor AE algorithms, but Android makes these states explicit.\n\nI think that's fine.\n\nThat's more of a philosophical question, but I don't think we should\ntry to impose all the IPA implementation to behave exactly the same,\nas that would impede the implementation of the most advanced features.\nThe means the most performant application won't behave exactly the\nsame between different platform, but I think at least in the controls\ndefinition we should try to get to the minimum common denominator that\nsatisfies most needs. After all I think Android does the same, the\ndefinition is there for all platforms, but each implementation is\nslightly different and indeed it's usually the phone manufacturer\nprovided camera app that actually makes use of the most advanced\nfeatures, while a generic camera app is usually limited to the\nsimplest use cases.\n\nFor this specific use case (the AECG lock/state/enable), let me try to\nsummarize how we could move forward:\n\n- Replace controls::AeLocked with a more descriptive AeState where the\n current 'locked' is represented by the 'Converged' state.\n- Introduce controls::AeLock (which for RPi won't be supported)\n- We have controls::AeEnable which more or less maps to\n android.AeMode. AeMode has more states which include control of the\n flash. We do not strictly have to support it now afaict but moving\n controls::AeMode from being a boolean on/off to a list of states\n might help that transition to happen in future.\n\nDo you agree with my understanding here ?\n\nThe controls::AeLocked -> controls::AeState[Converged] will require us\nand the corresponding RPi camera app update to happen synchronously\neven if I admit I don't know how you distribute libcamera. IOW, are the\ncamera apps compiled against the most recent master or against a\nfrozen version? So that master can freely advance and the app updates can\nfollow later once you update the version of libcamera you ship.\n\n>\n> Or it could be that I've just completely misunderstood that document :-)\n\nThe fact we all keep asking that to ourselves makes me wonder if we're\nall a bunch of idiots or if we're actually exploring rather new\nterritories in the open camera support landscape. I think time will\ntell :)\n\nThanks\n j\n\nPS As you might have noticed we're actually in the process of\naddressing the most advanced controls that android requires for\nhigh-end devices, so expect this kind of discussions to happen again\nin future :)\n\n>\n> Naush","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 59B69C321A\n\tfor ;\n\tWed, 23 Jun 2021 10:32:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AFADB68937;\n\tWed, 23 Jun 2021 12:32:49 +0200 (CEST)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CA70A68932\n\tfor ;\n\tWed, 23 Jun 2021 12:32:47 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id 9CCC8240008;\n\tWed, 23 Jun 2021 10:32:46 +0000 (UTC)"],"Date":"Wed, 23 Jun 2021 12:33:35 +0200","From":"Jacopo Mondi ","To":"Naushir Patuck ","Message-ID":"<20210623103335.6gpkateh66n7aiho@uno.localdomain>","References":"<20210618103351.1642060-4-paul.elder@ideasonboard.com>\n\t\n\t\n\t\n\t<20210621090503.GK1351869@pyrite.rasen.tech>\n\t\n\t<20210622161226.jhgillexctcakgay@uno.localdomain>\n\t\n\t<20210623081303.kdzuwna4ilpgap3m@uno.localdomain>\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 03/14] controls: Replace AeLocked\n\twith 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":17724,"web_url":"https://patchwork.libcamera.org/comment/17724/","msgid":"","date":"2021-06-23T10:56:32","subject":"Re: [libcamera-devel] [RFC PATCH 03/14] controls: Replace AeLocked\n\twith AeState, and add AeLock","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Jacopo,\n\n\nOn Wed, 23 Jun 2021 at 11:32, Jacopo Mondi wrote:\n\n> Hi Naush,\n>\n> On Wed, Jun 23, 2021 at 10:06:00AM +0100, Naushir Patuck wrote:\n> > Hi Jacopo and David,\n> >\n> > On Wed, 23 Jun 2021 at 09:12, Jacopo Mondi wrote:\n> >\n> > \n> >\n> >\n> > > >\n> > > > Are we saying that AeDisable means \"stop adjusting the AEC/AGC\n> > > > immediately, whether it's converged or not\", but AeLock means \"wait\n> > > > until it's converged and then immediately stop adjusting the\n> AEC/AGC\"?\n> > >\n> > > If AeLock is set on a Converged state then yes, the values are not\n> > > changed until the lock is lifted.\n> > >\n> > > Does the transition table repoted here helps ?\n> > >\n> > >\n> https://developer.android.com/reference/android/hardware/camera2/CaptureResult#CONTROL_AE_STATE\n> > >\n> > > Thanks\n> > > j\n> > >\n> >\n> > Having a brief look at the above document, I think\n> > Jacopo's description makes\n> > sense now. Enabling AeLock locks the current AE computed values, whether\n> > the algorithm has converged or not. However, with AeLock set to on,\n> other\n> > controls are effectively ignored by the AE (e.g. aePrecaptureTrigger,\n> maybe\n> > other things as well). Not sure why this is the case, as the application\n> > surely\n> > knows what state it wants the AE to be in...?\n>\n> So my previous example is slightly wrong, as the AeLock was meant to\n> be set only once the AE has converged, not in request #0.\n>\n> >\n> > We don't deal with pre-capture sequences in RPi AE, so for us AeLock and\n> > AeDisable are actually the same thing. Thinking about it, even with a\n> > pre-capture\n> > sequence, we could probably say the same. This may also be the case for\n> > other vendor AE algorithms, but Android makes these states explicit.\n>\n> I think that's fine.\n>\n> That's more of a philosophical question, but I don't think we should\n> try to impose all the IPA implementation to behave exactly the same,\n> as that would impede the implementation of the most advanced features.\n> The means the most performant application won't behave exactly the\n> same between different platform, but I think at least in the controls\n> definition we should try to get to the minimum common denominator that\n> satisfies most needs. After all I think Android does the same, the\n> definition is there for all platforms, but each implementation is\n> slightly different and indeed it's usually the phone manufacturer\n> provided camera app that actually makes use of the most advanced\n> features, while a generic camera app is usually limited to the\n> simplest use cases.\n>\n> For this specific use case (the AECG lock/state/enable), let me try to\n> summarize how we could move forward:\n>\n> - Replace controls::AeLocked with a more descriptive AeState where the\n> current 'locked' is represented by the 'Converged' state.\n> - Introduce controls::AeLock (which for RPi won't be supported)\n> - We have controls::AeEnable which more or less maps to\n> android.AeMode. AeMode has more states which include control of the\n> flash. We do not strictly have to support it now afaict but moving\n> controls::AeMode from being a boolean on/off to a list of states\n> might help that transition to happen in future.\n>\n\n> Do you agree with my understanding here ?\n>\n\nThat all sounds good to me, but I would like David to have his say as\nwell. It will probably be the case that our IPA will treat controls::AeLock\nthe same as our \"Paused\" state.\n\n\n> The controls::AeLocked -> controls::AeState[Converged] will require us\n> and the corresponding RPi camera app update to happen synchronously\n> even if I admit I don't know how you distribute libcamera. IOW, are the\n> camera apps compiled against the most recent master or against a\n> frozen version? So that master can freely advance and the app updates can\n> follow later once you update the version of libcamera you ship.\n>\n\nWe currently track master, so will have to update our apps separately when\nthis\nchange goes through. Eventually, we will freeze versions in our\ndistribution to\navoid breakages, but we are not there yet.\n\nRegards,\nNaush\n\n\n> >\n> > Or it could be that I've just completely misunderstood that document :-)\n>\n> The fact we all keep asking that to ourselves makes me wonder if we're\n> all a bunch of idiots or if we're actually exploring rather new\n> territories in the open camera support landscape. I think time will\n> tell :)\n>\n> Thanks\n> j\n>\n> PS As you might have noticed we're actually in the process of\n> addressing the most advanced controls that android requires for\n> high-end devices, so expect this kind of discussions to happen again\n> in future :)\n>\n> >\n> > Naush\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 41964C321B\n\tfor ;\n\tWed, 23 Jun 2021 10:56:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BEC8F68937;\n\tWed, 23 Jun 2021 12:56:50 +0200 (CEST)","from mail-lf1-x12b.google.com (mail-lf1-x12b.google.com\n\t[IPv6:2a00:1450:4864:20::12b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5370768932\n\tfor ;\n\tWed, 23 Jun 2021 12:56:49 +0200 (CEST)","by mail-lf1-x12b.google.com with SMTP id i13so3362805lfc.7\n\tfor ;\n\tWed, 23 Jun 2021 03:56:49 -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=\"WZsSZK+W\"; 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=4jdXKHRxDP1b+9JASKWmsptYXSe2MfULJekHaw7qNpg=;\n\tb=WZsSZK+WD/TglPM92+x/S7zBQWUPN26EeAWClCeCNZjTBQz5NvIv3mcvuGdMYvaZsy\n\tjLTbjw7M+OxmjMmRlzr4tel7g/ygH9bN1QqH88TcsBAqxmcC/1zQVQ5qCsKSVCQmRM/B\n\tGHmDofnwRtiU7tV7nDyWsDh23xUVofHMV1RKON0MTMEdrktTXHuV0JR+cCIn19XjKWBs\n\t0i2djIu+D1hEbf5p+hTxoEfNrj4oWfP3xYAYe4K4ltIcVaadfYAFc4hs6ldpM11wFTUs\n\tnZ8yqfYn8dIiIGmh0BwVyvcVuIeZb1h/lBigvIniz9ZHwbBZBKDUwp7cRAKSUiTFY4vn\n\tGJMQ==","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=4jdXKHRxDP1b+9JASKWmsptYXSe2MfULJekHaw7qNpg=;\n\tb=fuMUdUMYknM8HsJH3bRkChSaA7Uh2ejfgsGpfWA8JenbO0w92drJZBXICPfvAg1V7S\n\tlQFr5k7lrovaOXcDSCYBRmHPPF3A5vRXbZwmPDzr8S5IXR1GmOsNxvnIVrP9NKfe+zP9\n\tYE5oGlDAd+rh0U6G34ii0OXQnJ2ohMUDQsEZrOLj4TlkTZgRvFK808DBnKkRGp0mZjpf\n\tuZDRqPBSfuuwnG6Kqu7Dw/SqjmgzMwrbQkjJP6N3tZg1L2BPzsq2KRCJy16pSSxcRjEc\n\tzNaPDD57LNoRDOZcp4cV+EhIvymCo1Kodudq7lZRSCOz8vGjX5xxxpItLo2Rj/55txoa\n\ta/ZQ==","X-Gm-Message-State":"AOAM532LqAeRxFYDzv+PlPHC9C2SUlD9qY260YIa5hztA2gCDKfk4eZX\n\tDaCdVwgVothjL/f4FHlEdnhkvoGgS/B8GPAl1Sa6AeICiOu2Vw==","X-Google-Smtp-Source":"ABdhPJx4VUnK9WrouyA1mrN8F7oGYvdd/Mkfg1X8A495BWsYl9D6pKjoorv5DHnbnulpkJYpEqCDo0LWNl8j3u9qcEo=","X-Received":"by 2002:ac2:548e:: with SMTP id\n\tt14mr6535529lfk.617.1624445808788; \n\tWed, 23 Jun 2021 03:56:48 -0700 (PDT)","MIME-Version":"1.0","References":"<20210618103351.1642060-4-paul.elder@ideasonboard.com>\n\t\n\t\n\t\n\t<20210621090503.GK1351869@pyrite.rasen.tech>\n\t\n\t<20210622161226.jhgillexctcakgay@uno.localdomain>\n\t\n\t<20210623081303.kdzuwna4ilpgap3m@uno.localdomain>\n\t\n\t<20210623103335.6gpkateh66n7aiho@uno.localdomain>","In-Reply-To":"<20210623103335.6gpkateh66n7aiho@uno.localdomain>","From":"Naushir Patuck ","Date":"Wed, 23 Jun 2021 11:56:32 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"multipart/alternative; boundary=\"00000000000063da6805c56cc360\"","Subject":"Re: [libcamera-devel] [RFC PATCH 03/14] controls: Replace AeLocked\n\twith 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":17734,"web_url":"https://patchwork.libcamera.org/comment/17734/","msgid":"","date":"2021-06-23T15:19:55","subject":"Re: [libcamera-devel] [RFC PATCH 03/14] controls: Replace AeLocked\n\twith 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 Naush, Jacopo\n\nOn Wed, 23 Jun 2021 at 11:56, Naushir Patuck wrote:\n>\n> Hi Jacopo,\n>\n>\n> On Wed, 23 Jun 2021 at 11:32, Jacopo Mondi wrote:\n>>\n>> Hi Naush,\n>>\n>> On Wed, Jun 23, 2021 at 10:06:00AM +0100, Naushir Patuck wrote:\n>> > Hi Jacopo and David,\n>> >\n>> > On Wed, 23 Jun 2021 at 09:12, Jacopo Mondi wrote:\n>> >\n>> > \n>> >\n>> >\n>> > > >\n>> > > > Are we saying that AeDisable means \"stop adjusting the AEC/AGC\n>> > > > immediately, whether it's converged or not\", but AeLock means \"wait\n>> > > > until it's converged and then immediately stop adjusting the AEC/AGC\"?\n>> > >\n>> > > If AeLock is set on a Converged state then yes, the values are not\n>> > > changed until the lock is lifted.\n>> > >\n>> > > Does the transition table repoted here helps ?\n>> > >\n>> > > https://developer.android.com/reference/android/hardware/camera2/CaptureResult#CONTROL_AE_STATE\n>> > >\n>> > > Thanks\n>> > > j\n>> > >\n>> >\n>> > Having a brief look at the above document, I think\n>> > Jacopo's description makes\n>> > sense now. Enabling AeLock locks the current AE computed values, whether\n>> > the algorithm has converged or not. However, with AeLock set to on, other\n>> > controls are effectively ignored by the AE (e.g. aePrecaptureTrigger, maybe\n>> > other things as well). Not sure why this is the case, as the application\n>> > surely\n>> > knows what state it wants the AE to be in...?\n>>\n>> So my previous example is slightly wrong, as the AeLock was meant to\n>> be set only once the AE has converged, not in request #0.\n>>\n>> >\n>> > We don't deal with pre-capture sequences in RPi AE, so for us AeLock and\n>> > AeDisable are actually the same thing. Thinking about it, even with a\n>> > pre-capture\n>> > sequence, we could probably say the same. This may also be the case for\n>> > other vendor AE algorithms, but Android makes these states explicit.\n>>\n>> I think that's fine.\n>>\n>> That's more of a philosophical question, but I don't think we should\n>> try to impose all the IPA implementation to behave exactly the same,\n>> as that would impede the implementation of the most advanced features.\n>> The means the most performant application won't behave exactly the\n>> same between different platform, but I think at least in the controls\n>> definition we should try to get to the minimum common denominator that\n>> satisfies most needs. After all I think Android does the same, the\n>> definition is there for all platforms, but each implementation is\n>> slightly different and indeed it's usually the phone manufacturer\n>> provided camera app that actually makes use of the most advanced\n>> features, while a generic camera app is usually limited to the\n>> simplest use cases.\n>>\n>> For this specific use case (the AECG lock/state/enable), let me try to\n>> summarize how we could move forward:\n>>\n>> - Replace controls::AeLocked with a more descriptive AeState where the\n>> current 'locked' is represented by the 'Converged' state.\n>> - Introduce controls::AeLock (which for RPi won't be supported)\n>> - We have controls::AeEnable which more or less maps to\n>> android.AeMode. AeMode has more states which include control of the\n>> flash. We do not strictly have to support it now afaict but moving\n>> controls::AeMode from being a boolean on/off to a list of states\n>> might help that transition to happen in future.\n>>\n>>\n>> Do you agree with my understanding here ?\n>\n>\n> That all sounds good to me, but I would like David to have his say as\n> well. It will probably be the case that our IPA will treat controls::AeLock\n> the same as our \"Paused\" state.\n\nI think we treat \"lock\" essentially the same as \"enable\". Maybe we\nignore manual settings in the former but not the latter, but it's not\nsounding like a big deal. Will discuss with Naush when I'm next in the\noffice.\n\n>\n>>\n>> The controls::AeLocked -> controls::AeState[Converged] will require us\n>> and the corresponding RPi camera app update to happen synchronously\n>> even if I admit I don't know how you distribute libcamera. IOW, are the\n>> camera apps compiled against the most recent master or against a\n>> frozen version? So that master can freely advance and the app updates can\n>> follow later once you update the version of libcamera you ship.\n>\n>\n> We currently track master, so will have to update our apps separately when this\n> change goes through. Eventually, we will freeze versions in our distribution to\n> avoid breakages, but we are not there yet.\n\nI'd prefer to attach ourselves to a particular libcamera version once\nwe have frozen a first version of the API, though maybe that's still\ntoo far off? I'd also like to sort out our sporadic crash\n(https://bugs.libcamera.org/show_bug.cgi?id=26) before doing that too.\n\nThanks\nDavid\n\n>\n> Regards,\n> Naush\n>\n>>\n>> >\n>> > Or it could be that I've just completely misunderstood that document :-)\n>>\n>> The fact we all keep asking that to ourselves makes me wonder if we're\n>> all a bunch of idiots or if we're actually exploring rather new\n>> territories in the open camera support landscape. I think time will\n>> tell :)\n>>\n>> Thanks\n>> j\n>>\n>> PS As you might have noticed we're actually in the process of\n>> addressing the most advanced controls that android requires for\n>> high-end devices, so expect this kind of discussions to happen again\n>> in future :)\n>>\n>> >\n>> > Naush","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 A8776C321B\n\tfor ;\n\tWed, 23 Jun 2021 15:20:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2A6B568932;\n\tWed, 23 Jun 2021 17:20:09 +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 A680768932\n\tfor ;\n\tWed, 23 Jun 2021 17:20:06 +0200 (CEST)","by mail-wm1-x32d.google.com with SMTP id\n\tl7-20020a05600c1d07b02901b0e2ebd6deso1615114wms.1\n\tfor ;\n\tWed, 23 Jun 2021 08:20:06 -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=\"sq7ZP3k8\"; 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=NKjETzn5/4Myma+6fx3YHpgDqV9KLDxEJZzmcu5ZZGQ=;\n\tb=sq7ZP3k8GAv8SfudzdhMaHphl+R+eYsFqvnAp0uOPwCvGGkdTs+hwWNNjpWHtMAeDo\n\tCY0bDf3ejGIjDq/oXhC/xqjk3PAYgJ/ePs1KNzflZtLKl8LX8O+EsG5RQXHR1cINHxmw\n\tIgjStIyBXsoFesaFb2XahKSP0YZ4nPbPg3m72xOl+76hYbzWiP/IhWekIG0Yyp2Fh/Tu\n\tEM+1k84kFnVy0uFNaslH5CF7PPTOApwSfL0KoS9uat5oMuRgcwSDU4Z+GMdAvydCF+Yp\n\t0hAlOzzkcswbKu1yCt5jL9nOcXmVmKJgHPaK8gdKT72d2aL+Izg0GWNagzZfayJkw15B\n\t3VJQ==","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=NKjETzn5/4Myma+6fx3YHpgDqV9KLDxEJZzmcu5ZZGQ=;\n\tb=No9/MCVot8BoERh1MSpdRLEuQ4K5mM6A0XymxEmQVELNT+o7KgW1fj66aqWTOu3zcg\n\tMrGS1JGj3BKqjJZaqcLzsBN9X+9HqsxHuRTluqi+7JBXDISDGjhwAxk4mI1/AHqtyFt4\n\t8hhqlV3PiCt6LE2Lut43kAvGZV14+es1lnS1ULI+vwU0DzYdOULwjywowcIU+ODUQ+SF\n\t2GPJXNLj2pAbeB2ZW7UdzHfU3UvJQdDG8j+RYmnFfrf7xCYDg0quu+lQ3ofnFuxThgxw\n\t8GdtZO6ttp8cluAwo6C6Bp7WyRAfP+6JMAUI4BAcRY/KT9Fef7j+rzydxOrI0JyY7I+v\n\tDH6A==","X-Gm-Message-State":"AOAM533MPH8RbL0lq/Dor4IBqFXInFj/ruTBuuJlzKrze0qL9uBaDC6G\n\tUVyAPABgcVn4lU5nyQ78PMMfjBSHVPOC4VTJ1gP4jg==","X-Google-Smtp-Source":"ABdhPJx6X2IG7OOv4Sxr6Ql67JsApdGvWpzqv+gDe5vt+lOzZsYVlj154uEanBjLwJp0u0DYsvXxVcCneMRskbaCAnc=","X-Received":"by 2002:a05:600c:2184:: with SMTP id\n\te4mr284048wme.40.1624461606291; \n\tWed, 23 Jun 2021 08:20:06 -0700 (PDT)","MIME-Version":"1.0","References":"<20210618103351.1642060-4-paul.elder@ideasonboard.com>\n\t\n\t\n\t\n\t<20210621090503.GK1351869@pyrite.rasen.tech>\n\t\n\t<20210622161226.jhgillexctcakgay@uno.localdomain>\n\t\n\t<20210623081303.kdzuwna4ilpgap3m@uno.localdomain>\n\t\n\t<20210623103335.6gpkateh66n7aiho@uno.localdomain>\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Wed, 23 Jun 2021 16:19:55 +0100","Message-ID":"","To":"Naushir Patuck ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH 03/14] controls: Replace AeLocked\n\twith 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":17826,"web_url":"https://patchwork.libcamera.org/comment/17826/","msgid":"","date":"2021-06-28T00:06:47","subject":"Re: [libcamera-devel] [RFC PATCH 03/14] controls: Replace AeLocked\n\twith 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 David,\n\nOn Wed, Jun 23, 2021 at 04:19:55PM +0100, David Plowman wrote:\n> On Wed, 23 Jun 2021 at 11:56, Naushir Patuck wrote:\n> > On Wed, 23 Jun 2021 at 11:32, Jacopo Mondi wrote:\n> >> On Wed, Jun 23, 2021 at 10:06:00AM +0100, Naushir Patuck wrote:\n> >>> On Wed, 23 Jun 2021 at 09:12, Jacopo Mondi wrote:\n> >>>\n> >>> \n> >>>\n> >>>>> Are we saying that AeDisable means \"stop adjusting the AEC/AGC\n> >>>>> immediately, whether it's converged or not\", but AeLock means \"wait\n> >>>>> until it's converged and then immediately stop adjusting the AEC/AGC\"?\n> >>>>\n> >>>> If AeLock is set on a Converged state then yes, the values are not\n> >>>> changed until the lock is lifted.\n> >>>>\n> >>>> Does the transition table repoted here helps ?\n> >>>>\n> >>>> https://developer.android.com/reference/android/hardware/camera2/CaptureResult#CONTROL_AE_STATE\n> >>>\n> >>> Having a brief look at the above document, I think\n> >>> Jacopo's description makes\n> >>> sense now. Enabling AeLock locks the current AE computed values, whether\n> >>> the algorithm has converged or not. However, with AeLock set to on, other\n> >>> controls are effectively ignored by the AE (e.g. aePrecaptureTrigger, maybe\n> >>> other things as well). Not sure why this is the case, as the application\n> >>> surely\n> >>> knows what state it wants the AE to be in...?\n> >>\n> >> So my previous example is slightly wrong, as the AeLock was meant to\n> >> be set only once the AE has converged, not in request #0.\n> >>\n> >>> We don't deal with pre-capture sequences in RPi AE, so for us AeLock and\n> >>> AeDisable are actually the same thing. Thinking about it, even with a\n> >>> pre-capture\n> >>> sequence, we could probably say the same. This may also be the case for\n> >>> other vendor AE algorithms, but Android makes these states explicit.\n> >>\n> >> I think that's fine.\n> >>\n> >> That's more of a philosophical question, but I don't think we should\n> >> try to impose all the IPA implementation to behave exactly the same,\n> >> as that would impede the implementation of the most advanced features.\n> >> The means the most performant application won't behave exactly the\n> >> same between different platform, but I think at least in the controls\n> >> definition we should try to get to the minimum common denominator that\n> >> satisfies most needs. After all I think Android does the same, the\n> >> definition is there for all platforms, but each implementation is\n> >> slightly different and indeed it's usually the phone manufacturer\n> >> provided camera app that actually makes use of the most advanced\n> >> features, while a generic camera app is usually limited to the\n> >> simplest use cases.\n> >>\n> >> For this specific use case (the AECG lock/state/enable), let me try to\n> >> summarize how we could move forward:\n> >>\n> >> - Replace controls::AeLocked with a more descriptive AeState where the\n> >> current 'locked' is represented by the 'Converged' state.\n> >> - Introduce controls::AeLock (which for RPi won't be supported)\n> >> - We have controls::AeEnable which more or less maps to\n> >> android.AeMode. AeMode has more states which include control of the\n> >> flash. We do not strictly have to support it now afaict but moving\n> >> controls::AeMode from being a boolean on/off to a list of states\n> >> might help that transition to happen in future.\n> >>\n> >> Do you agree with my understanding here ?\n> >\n> > That all sounds good to me, but I would like David to have his say as\n> > well. It will probably be the case that our IPA will treat controls::AeLock\n> > the same as our \"Paused\" state.\n> \n> I think we treat \"lock\" essentially the same as \"enable\". Maybe we\n> ignore manual settings in the former but not the latter, but it's not\n> sounding like a big deal. Will discuss with Naush when I'm next in the\n> office.\n> \n> >> The controls::AeLocked -> controls::AeState[Converged] will require us\n> >> and the corresponding RPi camera app update to happen synchronously\n> >> even if I admit I don't know how you distribute libcamera. IOW, are the\n> >> camera apps compiled against the most recent master or against a\n> >> frozen version? So that master can freely advance and the app updates can\n> >> follow later once you update the version of libcamera you ship.\n> >\n> > We currently track master, so will have to update our apps separately when this\n> > change goes through. Eventually, we will freeze versions in our distribution to\n> > avoid breakages, but we are not there yet.\n> \n> I'd prefer to attach ourselves to a particular libcamera version once\n> we have frozen a first version of the API, though maybe that's still\n> too far off? I'd also like to sort out our sporadic crash\n> (https://bugs.libcamera.org/show_bug.cgi?id=26) before doing that too.\n\nThat one is still on my todo list, it will get fixed.\n\n> >>> Or it could be that I've just completely misunderstood that document :-)\n> >>\n> >> The fact we all keep asking that to ourselves makes me wonder if we're\n> >> all a bunch of idiots or if we're actually exploring rather new\n> >> territories in the open camera support landscape. I think time will\n> >> tell :)\n> >>\n> >> PS As you might have noticed we're actually in the process of\n> >> addressing the most advanced controls that android requires for\n> >> high-end devices, so expect this kind of discussions to happen again\n> >> in future :)","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 BC405C321D\n\tfor ;\n\tMon, 28 Jun 2021 00:06:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0E142684D7;\n\tMon, 28 Jun 2021 02:06:51 +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 E9CE16028C\n\tfor ;\n\tMon, 28 Jun 2021 02:06: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 67B40EE;\n\tMon, 28 Jun 2021 02:06:48 +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=\"fI/i7Y3u\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1624838808;\n\tbh=TzsSqPlUflPZsDcQEyi5NQPIdpXUCsxxDy+91jHy7Xs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=fI/i7Y3ukDdk2vtNbjJjHI72F2RQFM+u6rtEKFf7vtcRFgYiz3C+4HeEmTs3ZjnMC\n\tfjyHMwHKeX9XPd1VZGnlmwNLLRMPxGkrT67rlmJ4f/y9I6hCaV3ieTaNIxbubtRJNq\n\tcSEecEs+h59rEVJ1DMShhCLyuQMSBoGdJOkBrTS8=","Date":"Mon, 28 Jun 2021 03:06:47 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"","References":"\n\t<20210621090503.GK1351869@pyrite.rasen.tech>\n\t\n\t<20210622161226.jhgillexctcakgay@uno.localdomain>\n\t\n\t<20210623081303.kdzuwna4ilpgap3m@uno.localdomain>\n\t\n\t<20210623103335.6gpkateh66n7aiho@uno.localdomain>\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 03/14] controls: Replace AeLocked\n\twith 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":17827,"web_url":"https://patchwork.libcamera.org/comment/17827/","msgid":"","date":"2021-06-28T00:49:31","subject":"Re: [libcamera-devel] [RFC PATCH 03/14] controls: Replace AeLocked\n\twith AeState, and add AeLock","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello everybody,\n\nOn Tue, Jun 22, 2021 at 06:12:26PM +0200, Jacopo Mondi wrote:\n> On Mon, Jun 21, 2021 at 10:15:21AM +0100, David Plowman wrote:\n> > On Mon, 21 Jun 2021 at 10:05, wrote:\n> >> On Fri, Jun 18, 2021 at 02:58:52PM +0100, Naushir Patuck wrote:\n> >>> On Fri, 18 Jun 2021 at 14:13, David Plowman wrote:\n> >>>> On Fri, 18 Jun 2021 at 13:07, Naushir Patuck wrote:\n> >>>>> On Fri, 18 Jun 2021 at 11:34, 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> >>>>>> 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 1c1e802a..ad0132c0 100644\n> >>>>>> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> >>>>>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> >>>>>> @@ -468,7 +468,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> >>>>> I think, strictly speaking, if agcStatus->locked == true, we should return\n> >>>>> control::AeStateConverged by the definition below. David, do you agree?\n> >>\n> >> Oh shoot, that is indeed what I meant. I got mixed up with the status\n> >> name :/\n> >>\n> >>>> Hmm, indeed. My reading is that what we used to know as AeEnable has\n> >>>> now been renamed/replaced by AeLock, is that right? And as you say,\n> >>>> \"locked\" does not mean \"converged\" (any more).\n> >>>\n> >>> Yes I agree, but we now have both AeEnable and AeLock defined with this change.\n> >>\n> >> AeEnable is for enabling/disabling the AE routine. AeLock is to lock the\n> >> AE values... which would normally be the same as turning off AE, but you\n> >> want to be able to unlock AE without having to restart the routine.\n> >\n> > I'm still struggling a bit with this, however. What would it mean to\n> > unlock the AE without restarting it? Isn't it then still fixed at\n> > whatever values it had? Perhaps you could provide an example of this\n> > behaviour which would show how it is different.\n> >\n> > Sorry for still being puzzled...\n> \n> And I'm sorry as well for asking possibly stupid questions, but:\n> \n> - It seems to me the RPi AGC routine has two Pause/Resume functions that:\n> - Pause: sets fixed gain and shutter time to the last computed\n> shutter and gain values\n> - Resume: re-sets them to 0\n> \n> - In the RPi AEGC implementation, if fixed shutter and gains are set\n> then those values are used\n> \n> - The AEGC routine is paused/resumed on controls::AeEnable on/off\n> \n> - When AeEnable == Off, then the only way to override the fixed\n> shutter/gain is by using the ExposureTime and AnalogueGain controls\n> (ie manual shutter/gain control)\n> \n> So in you implementation disabling AE has the effect that the AEGC\n> routine uses the last computed values until a new shutter/gain value\n> is set by the application (through what is usually called \"manual\" mode).\n> \n> The difference between your implementation and a new one that uses\n> AeLocked and AeEnable is, in my understanding, that when the AeLocked\n> is set the AEGC routine is still in control of the shutter/gain which\n> gets \"fixed\" as you're doing, but it does not accept manual control of\n> the shutter/gain like you're doing unless the AEGC is disabled.\n> \n> This seems to be suggested by the following paragraph in the Android\n> documentation:\n> \n> -------------------------------------------------------------------------------\n> \n> If an application is switching between automatic and manual control\n> and wishes to eliminate any flicker during the switch, the following\n> procedure is recommended:\n> \n> Starting in auto-AE mode:\n> - Lock AE\n> - Wait for the first result to be output that has the AE locked\n> - Copy exposure settings from that result into a request, set the request to manual AE\n> - Submit the capture request, proceed to run manual AE as desired.\n> \n> See android.control.aeState for AE lock related state transition details.\n> -------------------------------------------------------------------------------\n> \n> If we were able to report the AEGC convergence state I think the same\n> could be done without AeLock, by disabling AEGC once it has converged\n> and continue from there with manual exposure. But my understanding is that\n> having a lock controls guarantees that once the AEGC has\n> converged it won't change, so that the in-flight requests that are\n> completed before the first one with manual AE control is processed\n> won't have floating AEGC values,\n> \n> (Let me try to draw it down, hoping it render ok with your client,\n> otherwise https://paste.debian.net/1202043/)\n> \n> Queue of\n> submitted Completed AeState in the\n> Requests Requests completed Request\n> -----------------------------------------------------------------\n> \n> 0 (AeLock)\n> 1\n> 2\n> 3 0 Searching\n> 4 1 Converged\n> 5 (AeDisable + 2 Locked <- COPY AE VALUES\n> Manual AE with 3 Locked\n> settings from #2) 4 Locked\n> 5 Use Manual AE\n> \n> Without a Lock the AEGC routine would keep adjusting its values for\n> requests 3 and 4. Request 5 will contain settings copied from Request\n> 2 so a small hiccup might happen. Of course, if the number of frames\n> required to get to a converged state is longer, the duration of the\n> possible AE adjustment period is longer, making the flickering more\n> visible ?\n> \n> Does this make sense ?\n> \n> The android.control.aeMode documentation also reports that:\n> \n> -----------------------------------------------------------------\n> Note that auto-white balance (AWB) and auto-focus (AF) behavior is\n> device dependent when AE is in OFF mode. To have consistent behavior\n> across different devices, it is recommended to either set AWB and AF to\n> OFF mode or lock AWB and AF before setting AE to OFF.See\n> android.control.awbMode,\n> android.control.afMode,android.control.awbLock, and\n> android.control.afTrigger for more details.\n> -----------------------------------------------------------------\n> \n> Suggesting that the AF AWB and AE locks might play a role on the interactions\n> between the three algorithms.\n> \n> Does this make sense and provide a bit more of context about what\n> AeLock could be used for ?\n\nI'm still a bit puzzled by the need for an AeLock control. I do\nunderstand that it's used in Android to switch to manual exposure\nwithout glitches, as detailed above. However, couldn't the same result\nbe achieved by switching AeMode from on to off (thus switching to manual\nmode), without specifying manual values for the sensitivity, exposure\nand frame duration ? The camera would then keep the last values computed\nby the AEGC, which would be reported in metadata, and could be then used\nas a basis to set other manual values.\n\nThis may be related to defining what happens to manual control values\nwhen running in auto mode. The camera will report computed values for\nthe sensitivity, exposure time and frame duration in metadata, but\nshould those values be implicitly copied to the request controls\n(effectively only, not really adding them to the Request instances) when\nthe algorithm is switched off, or should the last value that was set\nmanually be used in that case ? I think we need to clearly specify this,\nideally in a consistent fashion across all controls. If we decide that\nthe camera behaviour should be to implicitly copy the metadata value to\nthe control value, then AeLock doesn't seem to be needed. Otherwise,\nswitching seamlessly from auto to manual mode requires a mechanism\nsimilar to AeLock.\n\nIs there any other need for the AeLock control that I may have missed ?\n\n> >> That's the idea afaict.\n> >>\n> >> Supporting AeLock is optional, of course. Same for AeState, not all of\n> >> the states need to be supported. You can specify which ones are\n> >> supported in pipeline's ControlInfoMap.\n> >>\n> >>> Perhaps combining them to avoid redundancy makes sense? Or do folks think they\n> >>> can they serve different purposes?\n> >>>\n> >>>> So:\n> >>>>\n> >>>> We set controls::AeLocked in the metadata according to the value that\n> >>>> we received in the corresponding control (which we now receive instead\n> >>>> of AeEnable), and\n> >>>>\n> >>>> We set controls::AeState in the metadata to AeStateConverged when\n> >>>> agcStatus->locked is true, otherwise to AeStateSearching. We should\n> >>>> probably bite the bullet and rename that \"locked\" field to\n> >>>> \"converged\", otherwise it's going to cause confusion for the rest of\n> >>>> eternity.\n> >>>\n> >>> Agree, I think we should rename \"locked\" to \"converged\" in the controller\n> >>> metadata struct.\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 b47ea324..88a562a8 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> >>>>>> - 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","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 B77EBC321A\n\tfor ;\n\tMon, 28 Jun 2021 00:49:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EBF47684D5;\n\tMon, 28 Jun 2021 02:49:34 +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 60B2F6028C\n\tfor ;\n\tMon, 28 Jun 2021 02:49:33 +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 BF478E1A;\n\tMon, 28 Jun 2021 02:49:32 +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=\"FabDNp+r\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1624841372;\n\tbh=SYHAQBKRXJM4ZwtbVKQBvOy7xc6u5bN3AvxrGATy96E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=FabDNp+rGqQ5zZbmW2eHJB4ZZmxwbp2lsCWe32qW2Gt8BCdttIYTwrG5wDzzXOeun\n\t3i+7OSUKZUtyqk55kEys2XbjePYw7LlRgjGy6cBAk8XT2nWfGpXDBscv2kFVE+CVLb\n\t7xkc0Fd2Vpr79dyPcfMwZ/t3kJWJ4jKykP70dhcs=","Date":"Mon, 28 Jun 2021 03:49:31 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210618103351.1642060-1-paul.elder@ideasonboard.com>\n\t<20210618103351.1642060-4-paul.elder@ideasonboard.com>\n\t\n\t\n\t\n\t<20210621090503.GK1351869@pyrite.rasen.tech>\n\t\n\t<20210622161226.jhgillexctcakgay@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210622161226.jhgillexctcakgay@uno.localdomain>","Subject":"Re: [libcamera-devel] [RFC PATCH 03/14] controls: Replace AeLocked\n\twith 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":17880,"web_url":"https://patchwork.libcamera.org/comment/17880/","msgid":"","date":"2021-06-28T09:14:02","subject":"Re: [libcamera-devel] [RFC PATCH 03/14] controls: Replace AeLocked\n\twith 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 Mon, 28 Jun 2021 at 01:49, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hello everybody,\n>\n> On Tue, Jun 22, 2021 at 06:12:26PM +0200, Jacopo Mondi wrote:\n> > On Mon, Jun 21, 2021 at 10:15:21AM +0100, David Plowman wrote:\n> > > On Mon, 21 Jun 2021 at 10:05, wrote:\n> > >> On Fri, Jun 18, 2021 at 02:58:52PM +0100, Naushir Patuck wrote:\n> > >>> On Fri, 18 Jun 2021 at 14:13, David Plowman wrote:\n> > >>>> On Fri, 18 Jun 2021 at 13:07, Naushir Patuck wrote:\n> > >>>>> On Fri, 18 Jun 2021 at 11:34, Paul Elder wrote:\n> > >>>>>>\n> > >>>>>> AeLocked alone isn't sufficient for reporting the AE state, so\n> replace\n> > >>>>>> it with AeState. Add an AeLock control for instructing the camera\n> to\n> > >>>>>> lock the AE values.\n> > >>>>>>\n> > >>>>>> Update the current users of AeLocked accordingly.\n> > >>>>>>\n> > >>>>>> Signed-off-by: Paul Elder \n> > >>>>>> ---\n> > >>>>>> src/ipa/raspberrypi/raspberrypi.cpp | 5 +-\n> > >>>>>> src/ipa/rkisp1/rkisp1.cpp | 13 ++--\n> > >>>>>> src/libcamera/control_ids.yaml | 96\n> ++++++++++++++++++-----------\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 1c1e802a..ad0132c0 100644\n> > >>>>>> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > >>>>>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > >>>>>> @@ -468,7 +468,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> > >>>>>> +\n> controls::AeStateSearching);\n> > >>>>>\n> > >>>>> I think, strictly speaking, if agcStatus->locked == true, we\n> should return\n> > >>>>> control::AeStateConverged by the definition below. David, do you\n> agree?\n> > >>\n> > >> Oh shoot, that is indeed what I meant. I got mixed up with the status\n> > >> name :/\n> > >>\n> > >>>> Hmm, indeed. My reading is that what we used to know as AeEnable has\n> > >>>> now been renamed/replaced by AeLock, is that right? And as you say,\n> > >>>> \"locked\" does not mean \"converged\" (any more).\n> > >>>\n> > >>> Yes I agree, but we now have both AeEnable and AeLock defined with\n> this change.\n> > >>\n> > >> AeEnable is for enabling/disabling the AE routine. AeLock is to lock\n> the\n> > >> AE values... which would normally be the same as turning off AE, but\n> you\n> > >> want to be able to unlock AE without having to restart the routine.\n> > >\n> > > I'm still struggling a bit with this, however. What would it mean to\n> > > unlock the AE without restarting it? Isn't it then still fixed at\n> > > whatever values it had? Perhaps you could provide an example of this\n> > > behaviour which would show how it is different.\n> > >\n> > > Sorry for still being puzzled...\n> >\n> > And I'm sorry as well for asking possibly stupid questions, but:\n> >\n> > - It seems to me the RPi AGC routine has two Pause/Resume functions that:\n> > - Pause: sets fixed gain and shutter time to the last computed\n> > shutter and gain values\n> > - Resume: re-sets them to 0\n> >\n> > - In the RPi AEGC implementation, if fixed shutter and gains are set\n> > then those values are used\n> >\n> > - The AEGC routine is paused/resumed on controls::AeEnable on/off\n> >\n> > - When AeEnable == Off, then the only way to override the fixed\n> > shutter/gain is by using the ExposureTime and AnalogueGain controls\n> > (ie manual shutter/gain control)\n> >\n> > So in you implementation disabling AE has the effect that the AEGC\n> > routine uses the last computed values until a new shutter/gain value\n> > is set by the application (through what is usually called \"manual\" mode).\n> >\n> > The difference between your implementation and a new one that uses\n> > AeLocked and AeEnable is, in my understanding, that when the AeLocked\n> > is set the AEGC routine is still in control of the shutter/gain which\n> > gets \"fixed\" as you're doing, but it does not accept manual control of\n> > the shutter/gain like you're doing unless the AEGC is disabled.\n> >\n> > This seems to be suggested by the following paragraph in the Android\n> > documentation:\n> >\n> >\n> -------------------------------------------------------------------------------\n> >\n> > If an application is switching between automatic and manual control\n> > and wishes to eliminate any flicker during the switch, the following\n> > procedure is recommended:\n> >\n> > Starting in auto-AE mode:\n> > - Lock AE\n> > - Wait for the first result to be output that has the AE locked\n> > - Copy exposure settings from that result into a request, set the\n> request to manual AE\n> > - Submit the capture request, proceed to run manual AE as desired.\n> >\n> > See android.control.aeState for AE lock related state transition details.\n> >\n> -------------------------------------------------------------------------------\n> >\n> > If we were able to report the AEGC convergence state I think the same\n> > could be done without AeLock, by disabling AEGC once it has converged\n> > and continue from there with manual exposure. But my understanding is\n> that\n> > having a lock controls guarantees that once the AEGC has\n> > converged it won't change, so that the in-flight requests that are\n> > completed before the first one with manual AE control is processed\n> > won't have floating AEGC values,\n> >\n> > (Let me try to draw it down, hoping it render ok with your client,\n> > otherwise https://paste.debian.net/1202043/)\n> >\n> > Queue of\n> > submitted Completed AeState in the\n> > Requests Requests completed Request\n> > -----------------------------------------------------------------\n> >\n> > 0 (AeLock)\n> > 1\n> > 2\n> > 3 0 Searching\n> > 4 1 Converged\n> > 5 (AeDisable + 2 Locked <- COPY AE VALUES\n> > Manual AE with 3 Locked\n> > settings from #2) 4 Locked\n> > 5 Use Manual AE\n> >\n> > Without a Lock the AEGC routine would keep adjusting its values for\n> > requests 3 and 4. Request 5 will contain settings copied from Request\n> > 2 so a small hiccup might happen. Of course, if the number of frames\n> > required to get to a converged state is longer, the duration of the\n> > possible AE adjustment period is longer, making the flickering more\n> > visible ?\n> >\n> > Does this make sense ?\n> >\n> > The android.control.aeMode documentation also reports that:\n> >\n> > -----------------------------------------------------------------\n> > Note that auto-white balance (AWB) and auto-focus (AF) behavior is\n> > device dependent when AE is in OFF mode. To have consistent behavior\n> > across different devices, it is recommended to either set AWB and AF to\n> > OFF mode or lock AWB and AF before setting AE to OFF.See\n> > android.control.awbMode,\n> > android.control.afMode,android.control.awbLock, and\n> > android.control.afTrigger for more details.\n> > -----------------------------------------------------------------\n> >\n> > Suggesting that the AF AWB and AE locks might play a role on the\n> interactions\n> > between the three algorithms.\n> >\n> > Does this make sense and provide a bit more of context about what\n> > AeLock could be used for ?\n>\n> I'm still a bit puzzled by the need for an AeLock control. I do\n> understand that it's used in Android to switch to manual exposure\n> without glitches, as detailed above. However, couldn't the same result\n> be achieved by switching AeMode from on to off (thus switching to manual\n> mode), without specifying manual values for the sensitivity, exposure\n> and frame duration ? The camera would then keep the last values computed\n> by the AEGC, which would be reported in metadata, and could be then used\n> as a basis to set other manual values.\n>\n\nI agree with the sentiment that AeLock and AeEnable sound like the same\nthing,\nand the Raspberry Pi AGC will almost certainly be treating it the same to\nachieve\nwhat the user expects without glitching.\n\n\n>\n> This may be related to defining what happens to manual control values\n> when running in auto mode. The camera will report computed values for\n> the sensitivity, exposure time and frame duration in metadata, but\n> should those values be implicitly copied to the request controls\n> (effectively only, not really adding them to the Request instances) when\n> the algorithm is switched off, or should the last value that was set\n> manually be used in that case ?\n\n\nIn my opinion, the values returned in the metadata, particularly\ncontrols::ExposureTime and controls::AnalogueGain should be the values\nused for that frame. So if the AE is switched off, we should still get\nthese\nvalues returned in the Request, as the currently used values (which may\nbe set manually, or frozen to what the AE last selected).\n\nI did have one bit of confusion in the paragraph above. If we have manual\ncontrols set, does that not imply that AE is not running in auto mode, so\nthe\ntwo operations are mutually exclusive?\n\nRegards,\nNaush\n\n\n> I think we need to clearly specify this,\n> ideally in a consistent fashion across all controls. If we decide that\n> the camera behaviour should be to implicitly copy the metadata value to\n> the control value, then AeLock doesn't seem to be needed. Otherwise,\n> switching seamlessly from auto to manual mode requires a mechanism\n> similar to AeLock.\n>\n> Is there any other need for the AeLock control that I may have missed ?\n>\n> > >> That's the idea afaict.\n> > >>\n> > >> Supporting AeLock is optional, of course. Same for AeState, not all of\n> > >> the states need to be supported. You can specify which ones are\n> > >> supported in pipeline's ControlInfoMap.\n> > >>\n> > >>> Perhaps combining them to avoid redundancy makes sense? Or do folks\n> think they\n> > >>> can they serve different purposes?\n> > >>>\n> > >>>> So:\n> > >>>>\n> > >>>> We set controls::AeLocked in the metadata according to the value\n> that\n> > >>>> we received in the corresponding control (which we now receive\n> instead\n> > >>>> of AeEnable), and\n> > >>>>\n> > >>>> We set controls::AeState in the metadata to AeStateConverged when\n> > >>>> agcStatus->locked is true, otherwise to AeStateSearching. We should\n> > >>>> probably bite the bullet and rename that \"locked\" field to\n> > >>>> \"converged\", otherwise it's going to cause confusion for the rest of\n> > >>>> eternity.\n> > >>>\n> > >>> Agree, I think we should rename \"locked\" to \"converged\" in the\n> controller\n> > >>> metadata struct.\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 b47ea324..88a562a8 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\n> 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\n> 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\n> 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\n> true, if it's\n> > >>>>>> - converging it shall be set to false. If the AE algorithm\n> is 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.\n> Enabling or disabling\n> > >>>>>> + AE (AeEnable) always resets the AeState to\n> AeStateInactive. The camera\n> > >>>>>> + device can do several state transitions between two\n> results, 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\n> sync with this\n> > >>>>>> + image). If AE state becomes AeStateConverged, then the\n> image data\n> > >>>>>> + associated with the result should be good to use.\n> > >>>>>> +\n> > >>>>>> + The state transitions mentioned below assume that\n> 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\n> 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\n> go to Converged.\n> > >>>>>> + If the camera finishes an AE scan, but flash is\n> 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\n> 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,\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\n> 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\n> session.\n> > >>>>>> + After the sequence is finished, the state shall go\n> 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\n> 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.\n> 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\n> good 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 C011DC3220\n\tfor ;\n\tMon, 28 Jun 2021 09:14:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7C75F684D8;\n\tMon, 28 Jun 2021 11:14:20 +0200 (CEST)","from mail-lj1-x22d.google.com (mail-lj1-x22d.google.com\n\t[IPv6:2a00:1450:4864:20::22d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 402B2684D2\n\tfor ;\n\tMon, 28 Jun 2021 11:14:19 +0200 (CEST)","by mail-lj1-x22d.google.com with SMTP id a6so752443ljq.3\n\tfor ;\n\tMon, 28 Jun 2021 02:14:19 -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=\"lRlrfvrJ\"; 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=Tj5LpAvxc2WJcXiOgPZukqo4VapL+TbGkAmLA0nTvUM=;\n\tb=lRlrfvrJ/chFT7uPqV/O8F7apvAAj3HWiooIfgQPiAp/LMizx58rgfvo+CGKlNQ31U\n\tWSs0WgU/TSa+wqKRn1bIeBSZrPTaHhITvyFtoxXxJroW+BDmKPJw5ZVavQqCbk0xlx0z\n\tA1GX9Trmq8sPFj0k/yrQU+/aFka9pM/qgbqITWJbJH1K75SNbXjfJje4DVa8XCvsEMc8\n\tUhPLyVjkHg3I+naKKInIILYBSN6Pd/ic0tOVPIszaMBOpjYJZmINai9q14ig/cKj9+pS\n\tThcLhaNNtME66g/f03K5DRuVW9g3+MmFxCnN95qvpqMIxa6CTIx1/k8CVNuHS2DvkMbs\n\tZJPQ==","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=Tj5LpAvxc2WJcXiOgPZukqo4VapL+TbGkAmLA0nTvUM=;\n\tb=QVP8arVwxcb8HQ5ML/qsS9TkvtDTwwen3NJ0ALQimad8mvU5/t9LMlVN8ivGIshLLn\n\tnRBAt6o0AXe3RBMN39EQWRfJZEyMoyL6PGThMM7N5HxMWw57bnbvjuJW0ubt7QdVv3Rn\n\titR4mVdp1qwIRIg4LZI/SxMBnwQwigUlDDRHCRMhCnvhJn2M0M5pHzm7nNhYsMdw49Qj\n\tiacjWEFh7dq10v13WV19J3dSHD23Dk4ZhigFh4bgHPw2GBbXkDNWMrNb5Fj0X/5w10cU\n\tg5g5R6e7rFbwf/duY1rglf0GoMvnP2cbpr3IqnkkK6i/d11q8dpZ+x7CiZCgcNjntU6G\n\tbbOw==","X-Gm-Message-State":"AOAM533tTI1AldBJ8SqJ4D4ueIDdprD1lJULFzkwS6n5DPqRIAbSXbeB\n\tO5+0SwqIde2eUWeGhjE90k9gERljN4u++JtBX33smER323I=","X-Google-Smtp-Source":"ABdhPJxADQmIAOF9DVcp0zxOHMRFZRoPR8o7r8fACX8nHFDH+SDnXkAStP4KytaQyXDamUVgLfBG8NnmvFUiWL21bvA=","X-Received":"by 2002:a05:651c:542:: with SMTP id\n\tq2mr18481273ljp.400.1624871658137; \n\tMon, 28 Jun 2021 02:14:18 -0700 (PDT)","MIME-Version":"1.0","References":"<20210618103351.1642060-1-paul.elder@ideasonboard.com>\n\t<20210618103351.1642060-4-paul.elder@ideasonboard.com>\n\t\n\t\n\t\n\t<20210621090503.GK1351869@pyrite.rasen.tech>\n\t\n\t<20210622161226.jhgillexctcakgay@uno.localdomain>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Mon, 28 Jun 2021 10:14:02 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000fd40b005c5cfe900\"","Subject":"Re: [libcamera-devel] [RFC PATCH 03/14] controls: Replace AeLocked\n\twith 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\" "}}]