[{"id":20016,"web_url":"https://patchwork.libcamera.org/comment/20016/","msgid":"","date":"2021-10-01T12:53:18","subject":"Re: [libcamera-devel] [PATCH v2 1/7] controls: Reorganize the\n\tAE-related controls","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 all this work! I was fine with all the functionality being\ndescribed, and mostly everything made sense to me, but there were just\na couple of places where I thought we might be able to explain things\nmore clearly. But this is just my opinion, others may disagree, so\nplease take with the necessary quantities of salt!\n\nOn Fri, 1 Oct 2021 at 11:33, Paul Elder wrote:\n>\n> We have multiple goals:\n> - we need a lock of some sort, to instruct the AEGC to not update output\n> results\n> - we need manual modes, to override the values computed by the AEGC\n> - we need to support seamless transitions from auto -> manual, and do so\n> without flickering\n> - we need custom minimum values for the manual controls, that is no\n> magic values for enabling/disabling auto\n> - all of these need to be done with AE sub-controls (exposure time,\n> analogue gain)\n>\n> To achieve these goals, we introduce mode controls for the AE\n> sub-controls: ExposureTimeMode and AnalogueGainMode. These have an auto\n> state, and a disabled state. The disabled state has an internal one-way\n> state change from locked to manual, triggered by the presence of the\n> value-controls (ExposureTime and AnalogueGain).\n>\n> We then remove the AeEnable control, as it is a redundant control in the\n> face of these two mode controls.\n>\n> We also remove AeLocked, as it is insufficient for reporting the AE\n> state, and we promote AeState to non-draft to fill its role. Notably,\n> the locked state is removed, since this information can be obtained from\n> the aforementioned mode controls.\n>\n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=42\n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=43\n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=47\n> Signed-off-by: Paul Elder \n>\n> ---\n> Changes in v2:\n> - No changes, just resubmitting at the head of this series so that it's\n> together and so that /people will actually see it/\n>\n> Initial version:\n> Still RFC as I haven't updated the users of the control yet, and I want\n> to check that these are the controls and docs that we want.\n>\n> We've decided that the \"master AE control\" will be implemented by a\n> helper... but looking at uvcvideo and the V4L2 controls I'm wondering if\n> such helper should come earlier than later?\n> ---\n> src/libcamera/control_ids.yaml | 215 +++++++++++++++++++++++----------\n> 1 file changed, 150 insertions(+), 65 deletions(-)\n>\n> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> index 9d4638ae..18b186e8 100644\n> --- a/src/libcamera/control_ids.yaml\n> +++ b/src/libcamera/control_ids.yaml\n> @@ -7,23 +7,61 @@\n> # Unless otherwise stated, all controls are bi-directional, i.e. they can be\n> # set through Request::controls() and returned out through Request::metadata().\n> controls:\n> - - AeEnable:\n> - type: bool\n> - description: |\n> - Enable or disable the AE.\n> -\n> - \\sa ExposureTime AnalogueGain\n> -\n> - - AeLocked:\n> - type: bool\n> + - AeState:\n> + type: int32_t\n> description: |\n> - Report the lock status of a running AE algorithm.\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> + Control to report the current AE algorithm state. Enabling or disabling\n> + any of the AE-related mode controls (eg. AnalogueGainMode,\n> + ExposureTimeMode) is not required to reset the state to\n> + AeStateInactive. The camera device can do several state transitions\n> + between two results, if it is allowed by the state transition table.\n> + For example, AeStateInactive may never actually be seen in a result.\n\nI was fine until we got to that bit \"Enabling or disabling\". The rest\nof this paragraph struck me as a real burst of detail and corner cases\n(?) while I was still struggling with what AeStateInactive meant.\n\nPerhaps some of this specific stuff can be moved until after all the\nenum values have been explained? I found the enum descriptions quite\neasy to follow and having read through those felt in a better place to\ntackle the details. Having said that, I'm still a bit vague as to why\n\"resetting the state to AeStateInactive\" is a particular issue of\nconcern?\n\n> +\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> + As some AE algorithms may still be running when any of the AE-related\n> + controls are in manual mode, the states are valid even when the mode\n> + controls are set to Disabled. To know if this image image used AE or\n\n\"image\" is repeated.\n\nDo we need to say anything about what happens when all controls are\ndisabled? (Does anything different happen??)\n\n> + manual (or a mixture), check the relevant modes (eg. AnalogueGainMode,\n> + ExposureTimeMode).\n> +\n> + \\sa AnalogueGain\n> + \\sa AnalogueGainMode\n> + \\sa ExposureTime\n> + \\sa ExposureTimeMode\n>\n> - \\sa AeEnable\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> + - 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> + - 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> + - name: AeStateFlashRequired\n> + value: 3\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: 4\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\nThis was all good, my only minor comment might be that using\nAeStateConverged and AeStateSearching in place of just\nConverged/Searching might be a little more precise?\n\n> + \\sa AePrecaptureTrigger\n>\n> # AeMeteringMode needs further attention:\n> # - Auto-generate max enum value.\n> @@ -93,6 +131,17 @@ controls:\n> how the desired total exposure is divided between the shutter time\n> and the sensor's analogue gain. The exposure modes are platform\n> specific, and not all exposure modes may be supported.\n> +\n> + When one of AnalogueGainMode or ExposureTimeMode is set to Disabled and\n> + a corresponding manual control is provided, AeExposureMode may be\n\nActually I think the same is true even if you don't give a manual\nvalue. As soon as you fix any of them, the other(s) will vary in such\na way as to ignore the exposure profile.\n\n> + ignored. This is because, for example, if AeExposureMode is set to\n> + ExposureLong but ExposureTimeMode is Disabled and a short ExposureTime\n> + is provided (AnalogueGainMode set to Auto), then the AeExposureMode\n> + doesn't make sense.\n\nThe example is good, but seemed a bit wordy to me? Maybe the whole\nthing might be:\n\n\"When any of AnalogueGainMode or ExposureTimeMode is set to Disabled.\nthe fixed values will override any choices made by the AeExposureMode which may\nconsequently be ignored.\"\n\n(Or perhaps I'm assuming a deeper understanding on the part of the\nreader than most will have?)\n\n> +\n> + \\sa AnalogueGainMode\n> + \\sa ExposureTimeMode\n> +\n> enum:\n> - name: ExposureNormal\n> value: 0\n> @@ -111,13 +160,15 @@ controls:\n> type: float\n> description: |\n> Specify an Exposure Value (EV) parameter. The EV parameter will only be\n> - applied if the AE algorithm is currently enabled.\n> + applied if the AE algorithm is currently enabled, that is, at least one\n> + of AnalogueGainMode and ExposureTimeMode are auto.\n>\n> By convention EV adjusts the exposure as log2. For example\n> EV = [-2, -1, 0.5, 0, 0.5, 1, 2] results in an exposure adjustment\n> of [1/4x, 1/2x, 1/sqrt(2)x, 1x, sqrt(2)x, 2x, 4x].\n>\n> - \\sa AeEnable\n> + \\sa AnalogueGainMode\n> + \\sa ExposureTimeMode\n>\n> - ExposureTime:\n> type: int32_t\n> @@ -125,17 +176,49 @@ controls:\n> Exposure time (shutter speed) for the frame applied in the sensor\n> device. This value is specified in micro-seconds.\n>\n> - Setting this value means that it is now fixed and the AE algorithm may\n> - not change it. Setting it back to zero returns it to the control of the\n> - AE algorithm.\n> + This control will only take effect if ExposureTimeMode is Disabled. Its\n> + presence in a request acts as a trigger to switch to the internal\n> + manual mode within ExposureTimeModeDisabled.\n> +\n> + When reported in metadata, this control indicates what exposure time\n> + was used for the current request, regardless of ExposureTimeMode.\n> + ExposureTimeMode will indicate the source of the exposure time value,\n> + whether it came from the AE algorithm or not.\n>\n> - \\sa AnalogueGain AeEnable\n> + \\sa AnalogueGain\n> + \\sa ExposureTimeMode\n>\n> - \\todo Document the interactions between AeEnable and setting a fixed\n> - value for this control. Consider interactions with other AE features,\n> - such as aperture and aperture/shutter priority mode, and decide if\n> - control of which features should be automatically adjusted shouldn't\n> - better be handled through a separate AE mode control.\n> + - ExposureTimeMode:\n> + type: int32_t\n> + description: |\n> + Control to set and report the source of the exposure time that will be\n> + applied.\n> +\n> + \\sa ExposureTime\n> + enum:\n> + - name: ExposureTimeModeAuto\n> + value: 0\n> + description: |\n> + The exposure time will be calculated automatically and set by the\n> + AE algorithm.\n> + - name: ExposureTimeModeDisabled\n> + value: 1\n> + description: |\n> + The exposure time will not be updated by the AE algorithm. It will\n> + come from the last calculated value when the mode was Auto, or from\n> + the value specified in ExposureTime.\n> +\n> + This Disabled state has two internal states; locked and manual.\n\nTBH I still struggle a bit with the word \"locked\". I think I prefer\n\"fixed\", it seems more obvious. But not to worry... if the Android\nworld prefers it then I will get used to it!\n\nThanks again for doing all this!\n\nBest regards\nDavid\n\n> + When the Disabled state is first entered, the internal state will\n> + be locked, and the latest exposure time value set by the AE\n> + algorithm will be used (or the default ExposureTime value, if the\n> + mode started out as Disabled). When an ExposureTime is provided,\n> + then the internal state will go to manual, and the provided value\n> + will be used for the exposure time. Going from manual to locked is\n> + not possible. If an ExposureTime value is provided in the same\n> + request as going into the Disabled state, then the internal locked\n> + state will be skipped, and the internal state will start out as\n> + manual.\n>\n> - AnalogueGain:\n> type: float\n> @@ -144,17 +227,49 @@ controls:\n> The value of the control specifies the gain multiplier applied to all\n> colour channels. This value cannot be lower than 1.0.\n>\n> - Setting this value means that it is now fixed and the AE algorithm may\n> - not change it. Setting it back to zero returns it to the control of the\n> - AE algorithm.\n> + This control will only take effect if ExposureTimeMode is Disabled. Its\n> + presence in a request acts as a trigger to switch to the internal\n> + manual mode within ExposureTimeModeDisabled.\n>\n> - \\sa ExposureTime AeEnable\n> + When reported in metadata, this control indicates what exposure time\n> + was used for the current request, regardless of ExposureTimeMode.\n> + ExposureTimeMode will indicate the source of the exposure time value,\n> + whether it came from the AE algorithm or not.\n> +\n> + \\sa ExposureTime\n> + \\sa AnalogueGainMode\n> +\n> + - AnalogueGainMode:\n> + type: int32_t\n> + description: |\n> + Control to set and report the source of the analogue gain that will be\n> + applied.\n>\n> - \\todo Document the interactions between AeEnable and setting a fixed\n> - value for this control. Consider interactions with other AE features,\n> - such as aperture and aperture/shutter priority mode, and decide if\n> - control of which features should be automatically adjusted shouldn't\n> - better be handled through a separate AE mode control.\n> + \\sa AnalogueGain\n> + enum:\n> + - name: AnalogueGainModeAuto\n> + value: 0\n> + description: |\n> + The analogue gain will be calculated automatically and set by the\n> + AE algorithm.\n> + - name: AnalogueGainModeDisabled\n> + value: 1\n> + description: |\n> + The analogue gain will not be updated by the AE algorithm. It will\n> + come from the last calculated value when the mode was Auto, or from\n> + the value specified in AnalogueGain.\n> +\n> + This Disabled state has two internal states; locked and manual.\n> + When the Disabled state is first entered, the internal state will\n> + be locked, and the latest analogue gain value set by the AE\n> + algorithm will be used (or the default AnalogueGain value, if the\n> + mode started out as Disabled). When an AnalogueGain is provided,\n> + then the internal state will go to manual, and the provided value\n> + will be used for the analogue gain. Going from manual to locked is\n> + not possible. If an AnalogueGain value is provided in the same\n> + request as going into the Disabled state, then the internal locked\n> + state will be skipped, and the internal state will start out as\n> + manual.\n>\n> - Brightness:\n> type: float\n> @@ -477,36 +592,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 E24A9BDC71\n\tfor ;\n\tFri, 1 Oct 2021 12:53:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 62589691AC;\n\tFri, 1 Oct 2021 14:53:32 +0200 (CEST)","from mail-wr1-x431.google.com (mail-wr1-x431.google.com\n\t[IPv6:2a00:1450:4864:20::431])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9779C691A6\n\tfor ;\n\tFri, 1 Oct 2021 14:53:30 +0200 (CEST)","by mail-wr1-x431.google.com with SMTP id s21so15305088wra.7\n\tfor ;\n\tFri, 01 Oct 2021 05:53:30 -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=\"lBmTL2Kx\"; 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=UQWl4Cxs1JQn+X4XyPKKtKXqU2UQJGxTF/ChSprEoQo=;\n\tb=lBmTL2KxDk40gSxK3zG+RRxqaB/VOuOde74Mj0LMvAB2F4pshU8pYaCZKjRZG4OtNS\n\tmEGQkoG5vlakexGXuf6ggv+XALnWwCXVbc01BcRSbqoMzvyvKvRSAc1VNix5VRu+eKYD\n\tW3i1FfLr2egV1uy4kSjqOJeOkqvhZL3quT6gC9Ju9rh9sFJPlUqNdYL3Kscfhf10q91f\n\thFbe+D8fvlwK+ZbTKRrQQVRXybHeL6jwNlqZR0IQB2ykB4PaltLR4xsZsRVYqPiIxItP\n\t/1mIf6HDpQmm0yVO//o6l+JjV4DL7nbGwUjbYYKvKQY4/kKzWfVd/0z8cSTZSGzqOUhV\n\tuOzw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=UQWl4Cxs1JQn+X4XyPKKtKXqU2UQJGxTF/ChSprEoQo=;\n\tb=p21+9elyaLagA6t/3aVHjyxoqsPgQacuE49QZB9ihC5vVRMTUHFiiV9GDe/cPwdIMh\n\t0oFP4k44iY21z9rKk3S8/BYCaDUCtM7pA10i04vL+rZG6zvYffneTR8ry0REVFg3a5JR\n\tRuxRsFjt0NvH7JmSL9kQhR0Kt304STIWU7sSsGeydJ8wxz5c7OjOSWpSNmbVKPuWFfSe\n\tl/MneJvsHau3RELKqw3EVTXVmsHClAwmKq3LekXQZOJ7tRXkzQeo8r+5CslyykZwpHIL\n\t1wMmDpRTNy3lrTwf+JaRj8zqDwE+iRW8NwF/s/FdzdR9804t+Z/D/Bzpg0WpGl2t+OzS\n\trFaA==","X-Gm-Message-State":"AOAM530DR9DHf1FxjBLV9kyrJ1NlP8dB56akNJ7xFllYvBTwMPPNOIkL\n\tyLJqkgMJTLfyvHpWFCLbIykMGjGStC3Rr4Jfuw8XKHn8sAu5JtlJ","X-Google-Smtp-Source":"ABdhPJzNM/cqUYN19cJtlQOB26PoI4BTXKyxwSj/G3+KAXvTp23s7dAVtx+ociJILi27wXzJ3xUDnXblXCl7wxWBlZs=","X-Received":"by 2002:adf:fb0a:: with SMTP id\n\tc10mr12589230wrr.354.1633092809596; \n\tFri, 01 Oct 2021 05:53:29 -0700 (PDT)","MIME-Version":"1.0","References":"<20211001103325.1077590-1-paul.elder@ideasonboard.com>\n\t<20211001103325.1077590-2-paul.elder@ideasonboard.com>","In-Reply-To":"<20211001103325.1077590-2-paul.elder@ideasonboard.com>","From":"David Plowman ","Date":"Fri, 1 Oct 2021 13:53:18 +0100","Message-ID":"","To":"Paul Elder ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 1/7] controls: Reorganize the\n\tAE-related controls","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":20017,"web_url":"https://patchwork.libcamera.org/comment/20017/","msgid":"<20211001160819.oqav7qozcggi2kq5@uno.localdomain>","date":"2021-10-01T16:08:19","subject":"Re: [libcamera-devel] [PATCH v2 1/7] controls: Reorganize the\n\tAE-related controls","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul,\n\nOn Fri, Oct 01, 2021 at 07:33:19PM +0900, Paul Elder wrote:\n> We have multiple goals:\n> - we need a lock of some sort, to instruct the AEGC to not update output\n> results\n> - we need manual modes, to override the values computed by the AEGC\n> - we need to support seamless transitions from auto -> manual, and do so\n> without flickering\n> - we need custom minimum values for the manual controls, that is no\n> magic values for enabling/disabling auto\n> - all of these need to be done with AE sub-controls (exposure time,\n> analogue gain)\n>\n> To achieve these goals, we introduce mode controls for the AE\n> sub-controls: ExposureTimeMode and AnalogueGainMode. These have an auto\n> state, and a disabled state. The disabled state has an internal one-way\n> state change from locked to manual, triggered by the presence of the\n> value-controls (ExposureTime and AnalogueGain).\n>\n> We then remove the AeEnable control, as it is a redundant control in the\n> face of these two mode controls.\n>\n> We also remove AeLocked, as it is insufficient for reporting the AE\n> state, and we promote AeState to non-draft to fill its role. Notably,\n> the locked state is removed, since this information can be obtained from\n> the aforementioned mode controls.\n>\n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=42\n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=43\n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=47\n> Signed-off-by: Paul Elder \n>\n> ---\n> Changes in v2:\n> - No changes, just resubmitting at the head of this series so that it's\n> together and so that /people will actually see it/\n>\n> Initial version:\n> Still RFC as I haven't updated the users of the control yet, and I want\n> to check that these are the controls and docs that we want.\n>\n> We've decided that the \"master AE control\" will be implemented by a\n> helper... but looking at uvcvideo and the V4L2 controls I'm wondering if\n> such helper should come earlier than later?\n> ---\n> src/libcamera/control_ids.yaml | 215 +++++++++++++++++++++++----------\n> 1 file changed, 150 insertions(+), 65 deletions(-)\n>\n> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> index 9d4638ae..18b186e8 100644\n> --- a/src/libcamera/control_ids.yaml\n> +++ b/src/libcamera/control_ids.yaml\n> @@ -7,23 +7,61 @@\n> # Unless otherwise stated, all controls are bi-directional, i.e. they can be\n> # set through Request::controls() and returned out through Request::metadata().\n> controls:\n> - - AeEnable:\n> - type: bool\n> - description: |\n> - Enable or disable the AE.\n> -\n> - \\sa ExposureTime AnalogueGain\n> -\n> - - AeLocked:\n> - type: bool\n> + - AeState:\n\nNow that we have two separate controls for the exposure time and\nanalogue gain, do we want a single state for the whole AEGC block ?\n\n> + type: int32_t\n> description: |\n> - Report the lock status of a running AE algorithm.\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> + Control to report the current AE algorithm state. Enabling or disabling\n> + any of the AE-related mode controls (eg. AnalogueGainMode,\n> + ExposureTimeMode) is not required to reset the state to\n> + AeStateInactive. The camera device can do several state transitions\n\nI understand that, from where we come from the\n\n\"Enabling or disabling any of the AE-related mode controls (eg. AnalogueGainMode,\nExposureTimeMode) is not required to reset the state to\nAeStateInactive.\"\n\nmakes sense, but you're reading this for the first time you would be\nwondering why is this information here, even before what the inactive\nstate is has been defined. If you consider this relevant, I would\nplace this in the Inactive state description itself\n\n> + between two results, if it is allowed by the state transition table.\n\nwhich table ? :)\n\n> + For example, AeStateInactive may never actually be seen in a result.\n\nI would place this information in the transition table if we have one\n\n> +\n> + The state in the result is the state for this image (in sync with this\n\nThe () part is a repetition, and doesn't this apply to all controls\npart of a result ? Don't they all apply to the frames part of the\nrequest they are part of ? (Also I would refer to requests, not\nimages)\n\n> + image). If AE state becomes AeStateConverged, then the image data\n> + associated with the result should be good to use.\n> +\n> + As some AE algorithms may still be running when any of the AE-related\n> + controls are in manual mode, the states are valid even when the mode\n\nWhat is manual mode ? Don't we have an \"enabled\"/\"disabled\" state only\n?\n\nI would just say that the state is reported even if the exposure time\nand the analogue gain algorithms are disabled\n\n> + controls are set to Disabled. To know if this image image used AE or\n> + manual (or a mixture), check the relevant modes (eg. AnalogueGainMode,\n> + ExposureTimeMode).\n> +\n> + \\sa AnalogueGain\n> + \\sa AnalogueGainMode\n> + \\sa ExposureTime\n> + \\sa ExposureTimeMode\n>\n> - \\sa AeEnable\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> + - 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> + - 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> + - name: AeStateFlashRequired\n> + value: 3\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: 4\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> + \\sa AePrecaptureTrigger\n\nWe don't have AE capture triggers, nor a transition table defined at\nall. And as said above, now we have two controls for exposure and\nanalog gain, and a single state for the whole AE.\n\nDo we want AeState at all ?\n\n>\n> # AeMeteringMode needs further attention:\n> # - Auto-generate max enum value.\n> @@ -93,6 +131,17 @@ controls:\n> how the desired total exposure is divided between the shutter time\n> and the sensor's analogue gain. The exposure modes are platform\n> specific, and not all exposure modes may be supported.\n> +\n> + When one of AnalogueGainMode or ExposureTimeMode is set to Disabled and\n> + a corresponding manual control is provided, AeExposureMode may be\n> + ignored. This is because, for example, if AeExposureMode is set to\n> + ExposureLong but ExposureTimeMode is Disabled and a short ExposureTime\n> + is provided (AnalogueGainMode set to Auto), then the AeExposureMode\n> + doesn't make sense.\n\nWhy not just:\n\n This controls takes effect only if ExposureTimeMode is set\n to ExposureTimeModeAuto and it's ignored otherwise\n\n> +\n> + \\sa AnalogueGainMode\n> + \\sa ExposureTimeMode\n> +\n> enum:\n> - name: ExposureNormal\n> value: 0\n> @@ -111,13 +160,15 @@ controls:\n> type: float\n> description: |\n> Specify an Exposure Value (EV) parameter. The EV parameter will only be\n> - applied if the AE algorithm is currently enabled.\n> + applied if the AE algorithm is currently enabled, that is, at least one\n> + of AnalogueGainMode and ExposureTimeMode are auto.\n\nMostly a question for RPi who upstreamed this, but\nI don't fully get how ExposureValue works. Seems like it is used as a\nglobal multiplier for the analogue gain in AGC::computeGain. Is it\nworth reworking it or is it just me that I'm missing some part ?\n\n>\n> By convention EV adjusts the exposure as log2. For example\n> EV = [-2, -1, 0.5, 0, 0.5, 1, 2] results in an exposure adjustment\n> of [1/4x, 1/2x, 1/sqrt(2)x, 1x, sqrt(2)x, 2x, 4x].\n>\n> - \\sa AeEnable\n> + \\sa AnalogueGainMode\n> + \\sa ExposureTimeMode\n>\n> - ExposureTime:\n> type: int32_t\n> @@ -125,17 +176,49 @@ controls:\n> Exposure time (shutter speed) for the frame applied in the sensor\n> device. This value is specified in micro-seconds.\n>\n> - Setting this value means that it is now fixed and the AE algorithm may\n> - not change it. Setting it back to zero returns it to the control of the\n> - AE algorithm.\n> + This control will only take effect if ExposureTimeMode is Disabled. Its\n> + presence in a request acts as a trigger to switch to the internal\n> + manual mode within ExposureTimeModeDisabled.\n\nI find the concept of \"internal manual mode\" confusing, and I think we\nshould not insert it in the documentation.\n\n This control will only take effect if ExposureTimeMode is Disabled.\n\n ^ take or takes ?\n\n and is ignored when ExposureTimeMode is set to Auto.\n\n\n> + When reported in metadata, this control indicates what exposure time\n> + was used for the current request, regardless of ExposureTimeMode.\n> + ExposureTimeMode will indicate the source of the exposure time value,\n> + whether it came from the AE algorithm or not.\n>\n> - \\sa AnalogueGain AeEnable\n> + \\sa AnalogueGain\n> + \\sa ExposureTimeMode\n>\n> - \\todo Document the interactions between AeEnable and setting a fixed\n> - value for this control. Consider interactions with other AE features,\n> - such as aperture and aperture/shutter priority mode, and decide if\n> - control of which features should be automatically adjusted shouldn't\n> - better be handled through a separate AE mode control.\n> + - ExposureTimeMode:\n\nSo, I don't want to be too picky, but does it make sense to say a\n\"Mode\" is either \"Auto\" or \"Disabled\" ? Aren't \"AutoExposure = {Enable,\nDisabled}\" or \"ExposureMode = {Auto, Manual}\" better ? I'm asking\nwithout having a real answer to the question.\n\n> + type: int32_t\n> + description: |\n> + Control to set and report the source of the exposure time that will be\n> + applied.\n\n Controls how the frame exposure time is computed. When set\n to \"Auto\" the auto-exposure algorithm computes the exposure\n time of the next frame and configures the image sensor\n accordingly. When set to \"Disabled\" the auto-exposure\n algorithm stops computing the exposure time.\n\n> + \\sa ExposureTime\n> + enum:\n> + - name: ExposureTimeModeAuto\n> + value: 0\n> + description: |\n> + The exposure time will be calculated automatically and set by the\n> + AE algorithm.\n> + - name: ExposureTimeModeDisabled\n> + value: 1\n> + description: |\n> + The exposure time will not be updated by the AE algorithm. It will\n> + come from the last calculated value when the mode was Auto, or from\n> + the value specified in ExposureTime.\n> +\n> + This Disabled state has two internal states; locked and manual.\n> + When the Disabled state is first entered, the internal state will\n> + be locked, and the latest exposure time value set by the AE\n> + algorithm will be used (or the default ExposureTime value, if the\n> + mode started out as Disabled). When an ExposureTime is provided,\n> + then the internal state will go to manual, and the provided value\n> + will be used for the exposure time. Going from manual to locked is\n> + not possible. If an ExposureTime value is provided in the same\n> + request as going into the Disabled state, then the internal locked\n> + state will be skipped, and the internal state will start out as\n> + manual.\n\nIs it useful to define such internal states ? Isn't the same expressed\nwith ?\n\n When transitioning from Auto to Disabled mode the last\n computed exposure value is used until a new value is\n specified through the ExposureTime control. If an\n ExposureTime value is specified in the same request where\n the ExposureTimeMode is set to Disabled, the provided\n ExposureTime is applied immediately.\n\nI also think we need to describe how to perform a 'flickerless'\ntransition, but that depends on what we decide to do with AeState. If\nI'm not mistaken we can deduce that the exposure value is 'locked' by\ninspecting the ExposureTimeMode value in the result.\n\nWe can have a dedicated section in the control description. I'll make\nan attempt but I'm sure this can be vastly improved.\n\n - Flicker-less manual exposure time transition:\n\n Applications that transition from ExposureTimeMode\n Auto mode to the direct control of the exposure time through\n the ExposureTime control should aim to do so by selecting\n an ExposureTime value as close as possible to the last\n value computed by the auto exposure algorithm to avoid\n any visible flickering.\n\n In order to select the correct value to use in the first\n Request with the ExposureTime control specified,\n applications should accommodate the natural delay in\n applying controls caused by the capture pipeline frame\n depth.\n\n When switching to manual exposure time control,\n applications should not immediately specify an\n ExposureTime value in the same request where\n ExposureTimeMode is set to Disabled. They should instead\n wait for the first Request where ExposureTimeMode is\n returned as Disabled in the Request metadata and use the\n there returned exposure time to populate the ExposureTime\n value in the next queued Request.\n\n The implementation of the auto-exposure time algorithm\n should equally try to minimize flickering and when\n transitioning from manual exposure time control to auto\n exposure, the last used value should be used as starting\n point.\n\nI'm not sure about the last paragraph though, does it match the\noutcome of our last discussion ?\n\n>\n> - AnalogueGain:\n> type: float\n> @@ -144,17 +227,49 @@ controls:\n> The value of the control specifies the gain multiplier applied to all\n> colour channels. This value cannot be lower than 1.0.\n>\n> - Setting this value means that it is now fixed and the AE algorithm may\n> - not change it. Setting it back to zero returns it to the control of the\n> - AE algorithm.\n> + This control will only take effect if ExposureTimeMode is Disabled. Its\n> + presence in a request acts as a trigger to switch to the internal\n> + manual mode within ExposureTimeModeDisabled.\n\ns/ExposureTimeMode/AnalogueGainMode ?\n>\n> - \\sa ExposureTime AeEnable\n> + When reported in metadata, this control indicates what exposure time\n> + was used for the current request, regardless of ExposureTimeMode.\n> + ExposureTimeMode will indicate the source of the exposure time value,\n> + whether it came from the AE algorithm or not.\n\nAgain, all the 'exposure' should be 'analogue gain' instead ?\n\n> +\n> + \\sa ExposureTime\n> + \\sa AnalogueGainMode\n> +\n> + - AnalogueGainMode:\n> + type: int32_t\n> + description: |\n> + Control to set and report the source of the analogue gain that will be\n> + applied.\n>\n> - \\todo Document the interactions between AeEnable and setting a fixed\n> - value for this control. Consider interactions with other AE features,\n> - such as aperture and aperture/shutter priority mode, and decide if\n> - control of which features should be automatically adjusted shouldn't\n> - better be handled through a separate AE mode control.\n> + \\sa AnalogueGain\n> + enum:\n> + - name: AnalogueGainModeAuto\n> + value: 0\n> + description: |\n> + The analogue gain will be calculated automatically and set by the\n> + AE algorithm.\n> + - name: AnalogueGainModeDisabled\n> + value: 1\n> + description: |\n> + The analogue gain will not be updated by the AE algorithm. It will\n> + come from the last calculated value when the mode was Auto, or from\n> + the value specified in AnalogueGain.\n> +\n> + This Disabled state has two internal states; locked and manual.\n> + When the Disabled state is first entered, the internal state will\n> + be locked, and the latest analogue gain value set by the AE\n> + algorithm will be used (or the default AnalogueGain value, if the\n> + mode started out as Disabled). When an AnalogueGain is provided,\n> + then the internal state will go to manual, and the provided value\n> + will be used for the analogue gain. Going from manual to locked is\n> + not possible. If an AnalogueGain value is provided in the same\n> + request as going into the Disabled state, then the internal locked\n> + state will be skipped, and the internal state will start out as\n> + manual.\n\nSame comments as per the ExposureTimeMode control.\n\n>\n> - Brightness:\n> type: float\n> @@ -477,36 +592,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 91852C3243\n\tfor ;\n\tFri, 1 Oct 2021 16:07:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D7ECE691AE;\n\tFri, 1 Oct 2021 18:07:35 +0200 (CEST)","from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B5440691A6\n\tfor ;\n\tFri, 1 Oct 2021 18:07:33 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 3CD5DE0006;\n\tFri, 1 Oct 2021 16:07:32 +0000 (UTC)"],"Date":"Fri, 1 Oct 2021 18:08:19 +0200","From":"Jacopo Mondi ","To":"Paul Elder ","Message-ID":"<20211001160819.oqav7qozcggi2kq5@uno.localdomain>","References":"<20211001103325.1077590-1-paul.elder@ideasonboard.com>\n\t<20211001103325.1077590-2-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211001103325.1077590-2-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 1/7] controls: Reorganize the\n\tAE-related controls","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20021,"web_url":"https://patchwork.libcamera.org/comment/20021/","msgid":"","date":"2021-10-02T04:10:36","subject":"Re: [libcamera-devel] [PATCH v2 1/7] controls: Reorganize the\n\tAE-related controls","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi everyone\n\nJust to answer Jacopo's question that he asked us...\n\nOn Fri, 1 Oct 2021 at 17:07, Jacopo Mondi wrote:\n>\n> Hi Paul,\n>\n> On Fri, Oct 01, 2021 at 07:33:19PM +0900, Paul Elder wrote:\n> > We have multiple goals:\n> > - we need a lock of some sort, to instruct the AEGC to not update output\n> > results\n> > - we need manual modes, to override the values computed by the AEGC\n> > - we need to support seamless transitions from auto -> manual, and do so\n> > without flickering\n> > - we need custom minimum values for the manual controls, that is no\n> > magic values for enabling/disabling auto\n> > - all of these need to be done with AE sub-controls (exposure time,\n> > analogue gain)\n> >\n> > To achieve these goals, we introduce mode controls for the AE\n> > sub-controls: ExposureTimeMode and AnalogueGainMode. These have an auto\n> > state, and a disabled state. The disabled state has an internal one-way\n> > state change from locked to manual, triggered by the presence of the\n> > value-controls (ExposureTime and AnalogueGain).\n> >\n> > We then remove the AeEnable control, as it is a redundant control in the\n> > face of these two mode controls.\n> >\n> > We also remove AeLocked, as it is insufficient for reporting the AE\n> > state, and we promote AeState to non-draft to fill its role. Notably,\n> > the locked state is removed, since this information can be obtained from\n> > the aforementioned mode controls.\n> >\n> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=42\n> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=43\n> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=47\n> > Signed-off-by: Paul Elder \n> >\n> > ---\n> > Changes in v2:\n> > - No changes, just resubmitting at the head of this series so that it's\n> > together and so that /people will actually see it/\n> >\n> > Initial version:\n> > Still RFC as I haven't updated the users of the control yet, and I want\n> > to check that these are the controls and docs that we want.\n> >\n> > We've decided that the \"master AE control\" will be implemented by a\n> > helper... but looking at uvcvideo and the V4L2 controls I'm wondering if\n> > such helper should come earlier than later?\n> > ---\n> > src/libcamera/control_ids.yaml | 215 +++++++++++++++++++++++----------\n> > 1 file changed, 150 insertions(+), 65 deletions(-)\n> >\n> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > index 9d4638ae..18b186e8 100644\n> > --- a/src/libcamera/control_ids.yaml\n> > +++ b/src/libcamera/control_ids.yaml\n> > @@ -7,23 +7,61 @@\n> > # Unless otherwise stated, all controls are bi-directional, i.e. they can be\n> > # set through Request::controls() and returned out through Request::metadata().\n> > controls:\n> > - - AeEnable:\n> > - type: bool\n> > - description: |\n> > - Enable or disable the AE.\n> > -\n> > - \\sa ExposureTime AnalogueGain\n> > -\n> > - - AeLocked:\n> > - type: bool\n> > + - AeState:\n>\n> Now that we have two separate controls for the exposure time and\n> analogue gain, do we want a single state for the whole AEGC block ?\n>\n> > + type: int32_t\n> > description: |\n> > - Report the lock status of a running AE algorithm.\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> > + Control to report the current AE algorithm state. Enabling or disabling\n> > + any of the AE-related mode controls (eg. AnalogueGainMode,\n> > + ExposureTimeMode) is not required to reset the state to\n> > + AeStateInactive. The camera device can do several state transitions\n>\n> I understand that, from where we come from the\n>\n> \"Enabling or disabling any of the AE-related mode controls (eg. AnalogueGainMode,\n> ExposureTimeMode) is not required to reset the state to\n> AeStateInactive.\"\n>\n> makes sense, but you're reading this for the first time you would be\n> wondering why is this information here, even before what the inactive\n> state is has been defined. If you consider this relevant, I would\n> place this in the Inactive state description itself\n>\n> > + between two results, if it is allowed by the state transition table.\n>\n> which table ? :)\n>\n> > + For example, AeStateInactive may never actually be seen in a result.\n>\n> I would place this information in the transition table if we have one\n>\n> > +\n> > + The state in the result is the state for this image (in sync with this\n>\n> The () part is a repetition, and doesn't this apply to all controls\n> part of a result ? Don't they all apply to the frames part of the\n> request they are part of ? (Also I would refer to requests, not\n> images)\n>\n> > + image). If AE state becomes AeStateConverged, then the image data\n> > + associated with the result should be good to use.\n> > +\n> > + As some AE algorithms may still be running when any of the AE-related\n> > + controls are in manual mode, the states are valid even when the mode\n>\n> What is manual mode ? Don't we have an \"enabled\"/\"disabled\" state only\n> ?\n>\n> I would just say that the state is reported even if the exposure time\n> and the analogue gain algorithms are disabled\n>\n> > + controls are set to Disabled. To know if this image image used AE or\n> > + manual (or a mixture), check the relevant modes (eg. AnalogueGainMode,\n> > + ExposureTimeMode).\n> > +\n> > + \\sa AnalogueGain\n> > + \\sa AnalogueGainMode\n> > + \\sa ExposureTime\n> > + \\sa ExposureTimeMode\n> >\n> > - \\sa AeEnable\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> > + - 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> > + - 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> > + - name: AeStateFlashRequired\n> > + value: 3\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: 4\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> > + \\sa AePrecaptureTrigger\n>\n> We don't have AE capture triggers, nor a transition table defined at\n> all. And as said above, now we have two controls for exposure and\n> analog gain, and a single state for the whole AE.\n>\n> Do we want AeState at all ?\n>\n> >\n> > # AeMeteringMode needs further attention:\n> > # - Auto-generate max enum value.\n> > @@ -93,6 +131,17 @@ controls:\n> > how the desired total exposure is divided between the shutter time\n> > and the sensor's analogue gain. The exposure modes are platform\n> > specific, and not all exposure modes may be supported.\n> > +\n> > + When one of AnalogueGainMode or ExposureTimeMode is set to Disabled and\n> > + a corresponding manual control is provided, AeExposureMode may be\n> > + ignored. This is because, for example, if AeExposureMode is set to\n> > + ExposureLong but ExposureTimeMode is Disabled and a short ExposureTime\n> > + is provided (AnalogueGainMode set to Auto), then the AeExposureMode\n> > + doesn't make sense.\n>\n> Why not just:\n>\n> This controls takes effect only if ExposureTimeMode is set\n> to ExposureTimeModeAuto and it's ignored otherwise\n>\n> > +\n> > + \\sa AnalogueGainMode\n> > + \\sa ExposureTimeMode\n> > +\n> > enum:\n> > - name: ExposureNormal\n> > value: 0\n> > @@ -111,13 +160,15 @@ controls:\n> > type: float\n> > description: |\n> > Specify an Exposure Value (EV) parameter. The EV parameter will only be\n> > - applied if the AE algorithm is currently enabled.\n> > + applied if the AE algorithm is currently enabled, that is, at least one\n> > + of AnalogueGainMode and ExposureTimeMode are auto.\n>\n> Mostly a question for RPi who upstreamed this, but\n> I don't fully get how ExposureValue works. Seems like it is used as a\n> global multiplier for the analogue gain in AGC::computeGain. Is it\n> worth reworking it or is it just me that I'm missing some part ?\n\nIt's used as a global multiplier for the AGC's _target_ values, at\nleast in our implementation. For example, generally a 2x increase in\ntarget values will give a 2x increase in combined exposure and\nanalogue gain, though exactly how much each increases by is determined\nby the exposure profile.\n\nIf any levers are fixed, then any such increase has to go into the\nones that aren't fixed. Clearly if all are fixed, then changing the ev\ncan have no effect at all. Does that all make sense? I think Paul's\ntext seems accurate to me.\n\nThanks!\n\nDavid\n\n>\n> >\n> > By convention EV adjusts the exposure as log2. For example\n> > EV = [-2, -1, 0.5, 0, 0.5, 1, 2] results in an exposure adjustment\n> > of [1/4x, 1/2x, 1/sqrt(2)x, 1x, sqrt(2)x, 2x, 4x].\n> >\n> > - \\sa AeEnable\n> > + \\sa AnalogueGainMode\n> > + \\sa ExposureTimeMode\n> >\n> > - ExposureTime:\n> > type: int32_t\n> > @@ -125,17 +176,49 @@ controls:\n> > Exposure time (shutter speed) for the frame applied in the sensor\n> > device. This value is specified in micro-seconds.\n> >\n> > - Setting this value means that it is now fixed and the AE algorithm may\n> > - not change it. Setting it back to zero returns it to the control of the\n> > - AE algorithm.\n> > + This control will only take effect if ExposureTimeMode is Disabled. Its\n> > + presence in a request acts as a trigger to switch to the internal\n> > + manual mode within ExposureTimeModeDisabled.\n>\n> I find the concept of \"internal manual mode\" confusing, and I think we\n> should not insert it in the documentation.\n>\n> This control will only take effect if ExposureTimeMode is Disabled.\n>\n> ^ take or takes ?\n>\n> and is ignored when ExposureTimeMode is set to Auto.\n>\n>\n> > + When reported in metadata, this control indicates what exposure time\n> > + was used for the current request, regardless of ExposureTimeMode.\n> > + ExposureTimeMode will indicate the source of the exposure time value,\n> > + whether it came from the AE algorithm or not.\n> >\n> > - \\sa AnalogueGain AeEnable\n> > + \\sa AnalogueGain\n> > + \\sa ExposureTimeMode\n> >\n> > - \\todo Document the interactions between AeEnable and setting a fixed\n> > - value for this control. Consider interactions with other AE features,\n> > - such as aperture and aperture/shutter priority mode, and decide if\n> > - control of which features should be automatically adjusted shouldn't\n> > - better be handled through a separate AE mode control.\n> > + - ExposureTimeMode:\n>\n> So, I don't want to be too picky, but does it make sense to say a\n> \"Mode\" is either \"Auto\" or \"Disabled\" ? Aren't \"AutoExposure = {Enable,\n> Disabled}\" or \"ExposureMode = {Auto, Manual}\" better ? I'm asking\n> without having a real answer to the question.\n>\n> > + type: int32_t\n> > + description: |\n> > + Control to set and report the source of the exposure time that will be\n> > + applied.\n>\n> Controls how the frame exposure time is computed. When set\n> to \"Auto\" the auto-exposure algorithm computes the exposure\n> time of the next frame and configures the image sensor\n> accordingly. When set to \"Disabled\" the auto-exposure\n> algorithm stops computing the exposure time.\n>\n> > + \\sa ExposureTime\n> > + enum:\n> > + - name: ExposureTimeModeAuto\n> > + value: 0\n> > + description: |\n> > + The exposure time will be calculated automatically and set by the\n> > + AE algorithm.\n> > + - name: ExposureTimeModeDisabled\n> > + value: 1\n> > + description: |\n> > + The exposure time will not be updated by the AE algorithm. It will\n> > + come from the last calculated value when the mode was Auto, or from\n> > + the value specified in ExposureTime.\n> > +\n> > + This Disabled state has two internal states; locked and manual.\n> > + When the Disabled state is first entered, the internal state will\n> > + be locked, and the latest exposure time value set by the AE\n> > + algorithm will be used (or the default ExposureTime value, if the\n> > + mode started out as Disabled). When an ExposureTime is provided,\n> > + then the internal state will go to manual, and the provided value\n> > + will be used for the exposure time. Going from manual to locked is\n> > + not possible. If an ExposureTime value is provided in the same\n> > + request as going into the Disabled state, then the internal locked\n> > + state will be skipped, and the internal state will start out as\n> > + manual.\n>\n> Is it useful to define such internal states ? Isn't the same expressed\n> with ?\n>\n> When transitioning from Auto to Disabled mode the last\n> computed exposure value is used until a new value is\n> specified through the ExposureTime control. If an\n> ExposureTime value is specified in the same request where\n> the ExposureTimeMode is set to Disabled, the provided\n> ExposureTime is applied immediately.\n>\n> I also think we need to describe how to perform a 'flickerless'\n> transition, but that depends on what we decide to do with AeState. If\n> I'm not mistaken we can deduce that the exposure value is 'locked' by\n> inspecting the ExposureTimeMode value in the result.\n>\n> We can have a dedicated section in the control description. I'll make\n> an attempt but I'm sure this can be vastly improved.\n>\n> - Flicker-less manual exposure time transition:\n>\n> Applications that transition from ExposureTimeMode\n> Auto mode to the direct control of the exposure time through\n> the ExposureTime control should aim to do so by selecting\n> an ExposureTime value as close as possible to the last\n> value computed by the auto exposure algorithm to avoid\n> any visible flickering.\n>\n> In order to select the correct value to use in the first\n> Request with the ExposureTime control specified,\n> applications should accommodate the natural delay in\n> applying controls caused by the capture pipeline frame\n> depth.\n>\n> When switching to manual exposure time control,\n> applications should not immediately specify an\n> ExposureTime value in the same request where\n> ExposureTimeMode is set to Disabled. They should instead\n> wait for the first Request where ExposureTimeMode is\n> returned as Disabled in the Request metadata and use the\n> there returned exposure time to populate the ExposureTime\n> value in the next queued Request.\n>\n> The implementation of the auto-exposure time algorithm\n> should equally try to minimize flickering and when\n> transitioning from manual exposure time control to auto\n> exposure, the last used value should be used as starting\n> point.\n>\n> I'm not sure about the last paragraph though, does it match the\n> outcome of our last discussion ?\n>\n> >\n> > - AnalogueGain:\n> > type: float\n> > @@ -144,17 +227,49 @@ controls:\n> > The value of the control specifies the gain multiplier applied to all\n> > colour channels. This value cannot be lower than 1.0.\n> >\n> > - Setting this value means that it is now fixed and the AE algorithm may\n> > - not change it. Setting it back to zero returns it to the control of the\n> > - AE algorithm.\n> > + This control will only take effect if ExposureTimeMode is Disabled. Its\n> > + presence in a request acts as a trigger to switch to the internal\n> > + manual mode within ExposureTimeModeDisabled.\n>\n> s/ExposureTimeMode/AnalogueGainMode ?\n> >\n> > - \\sa ExposureTime AeEnable\n> > + When reported in metadata, this control indicates what exposure time\n> > + was used for the current request, regardless of ExposureTimeMode.\n> > + ExposureTimeMode will indicate the source of the exposure time value,\n> > + whether it came from the AE algorithm or not.\n>\n> Again, all the 'exposure' should be 'analogue gain' instead ?\n>\n> > +\n> > + \\sa ExposureTime\n> > + \\sa AnalogueGainMode\n> > +\n> > + - AnalogueGainMode:\n> > + type: int32_t\n> > + description: |\n> > + Control to set and report the source of the analogue gain that will be\n> > + applied.\n> >\n> > - \\todo Document the interactions between AeEnable and setting a fixed\n> > - value for this control. Consider interactions with other AE features,\n> > - such as aperture and aperture/shutter priority mode, and decide if\n> > - control of which features should be automatically adjusted shouldn't\n> > - better be handled through a separate AE mode control.\n> > + \\sa AnalogueGain\n> > + enum:\n> > + - name: AnalogueGainModeAuto\n> > + value: 0\n> > + description: |\n> > + The analogue gain will be calculated automatically and set by the\n> > + AE algorithm.\n> > + - name: AnalogueGainModeDisabled\n> > + value: 1\n> > + description: |\n> > + The analogue gain will not be updated by the AE algorithm. It will\n> > + come from the last calculated value when the mode was Auto, or from\n> > + the value specified in AnalogueGain.\n> > +\n> > + This Disabled state has two internal states; locked and manual.\n> > + When the Disabled state is first entered, the internal state will\n> > + be locked, and the latest analogue gain value set by the AE\n> > + algorithm will be used (or the default AnalogueGain value, if the\n> > + mode started out as Disabled). When an AnalogueGain is provided,\n> > + then the internal state will go to manual, and the provided value\n> > + will be used for the analogue gain. Going from manual to locked is\n> > + not possible. If an AnalogueGain value is provided in the same\n> > + request as going into the Disabled state, then the internal locked\n> > + state will be skipped, and the internal state will start out as\n> > + manual.\n>\n> Same comments as per the ExposureTimeMode control.\n>\n> >\n> > - Brightness:\n> > type: float\n> > @@ -477,36 +592,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 B0480C3243\n\tfor ;\n\tSat, 2 Oct 2021 04:10:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 38866691AE;\n\tSat, 2 Oct 2021 06:10:50 +0200 (CEST)","from mail-wr1-x435.google.com (mail-wr1-x435.google.com\n\t[IPv6:2a00:1450:4864:20::435])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1FFC3684C5\n\tfor ;\n\tSat, 2 Oct 2021 06:10:47 +0200 (CEST)","by mail-wr1-x435.google.com with SMTP id d6so18432730wrc.11\n\tfor ;\n\tFri, 01 Oct 2021 21:10:47 -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=\"p1YyuDoA\"; 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=E6GBKIM//afYxcdINjX4op2ZW9Qkzh/8nAjeWjuosHk=;\n\tb=p1YyuDoAh755tPW0AK4Q+q6oRgZfUfX095iE+Erb5O94Kd5HRamEC06kdiCtLyMIWg\n\txeGTQZiGO+Uv0fuB6/9PitQK/cR6SV5uJ+jMb60S5Imt43931QmxHBGT1Hsr/PgPq19X\n\tg9OTdtQRReHAr29o+XI8JIT3V5bl+MOi+VEpv9Qu31T2BAw5wkMxnc/+9Xudqs4urtA1\n\tzzQ/q2gMQhCYlO3BOMeMxubfGzfSJW/SKpxQnn2XsIHiBXs1QLQgrlW0J8t1nlDYmC8/\n\t3XSWB7W6H3LO3XujxtHJ817R0f6F1LkX8s96yo/aFm1ZXEKWBUsDFLbXB1hntCqbUOY3\n\t3cCw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=E6GBKIM//afYxcdINjX4op2ZW9Qkzh/8nAjeWjuosHk=;\n\tb=VS/KksQPGY55QNQEx/JaMgNTkpn3lelXmGbPQdWd6FvIBtQr3mhlHBnFIEY7M74rX5\n\tBSX+DZYPuRIYNo7rDcrWXwkhHYlJqxdrEu4oGkMQ8op7yEfQA8BLJvkaZk5eR5XhWbbo\n\tVb6aU2l9Tts6pw+GNr/xLvkkrCoXj6nZtlJxm8v+V0ftyg8stPQNK+HEIPb581CM5EyQ\n\taPhfz3Q/3YFRZY0V47IjORySIJQ8k+QZ2pqtQIcX7+q+/TD5uqUNM8UzEXPfD7ExtdSc\n\tdlvbnt9SbQ9Jg5It3WNZ7r7OucvhW4lJ5KxoB+0X27oMYmH8X9oWefcR6x5Q1d1L8DsW\n\tkSaA==","X-Gm-Message-State":"AOAM532JRfg72KzDslzdJ/XsWn9KS8vutxfq/5FqcgsEWSlYj/nNqb7A\n\tN/euVDjvjvTvLICr2i+7GSeslxtFHMZDJstisT+GSg==","X-Google-Smtp-Source":"ABdhPJxbMu9DwVrlwUpXgU6mso1u9qZIv5ah7WIOGtI7VV8L7JR4B7sUmLSQ7dSH06ppV9jU0QxDlOtG1Wb0yysLBnM=","X-Received":"by 2002:adf:a402:: with SMTP id d2mr1447850wra.266.1633147847287;\n\tFri, 01 Oct 2021 21:10:47 -0700 (PDT)","MIME-Version":"1.0","References":"<20211001103325.1077590-1-paul.elder@ideasonboard.com>\n\t<20211001103325.1077590-2-paul.elder@ideasonboard.com>\n\t<20211001160819.oqav7qozcggi2kq5@uno.localdomain>","In-Reply-To":"<20211001160819.oqav7qozcggi2kq5@uno.localdomain>","From":"David Plowman ","Date":"Sat, 2 Oct 2021 05:10:36 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 1/7] controls: Reorganize the\n\tAE-related controls","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":20024,"web_url":"https://patchwork.libcamera.org/comment/20024/","msgid":"<20211004061733.GB4221@pyrite.rasen.tech>","date":"2021-10-04T06:17:33","subject":"Re: [libcamera-devel] [PATCH v2 1/7] controls: Reorganize the\n\tAE-related controls","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi David,\n\nThanks for the feedback.\n\nOn Fri, Oct 01, 2021 at 01:53:18PM +0100, David Plowman wrote:\n> Hi Paul\n> \n> Thanks for all this work! I was fine with all the functionality being\n> described, and mostly everything made sense to me, but there were just\n> a couple of places where I thought we might be able to explain things\n> more clearly. But this is just my opinion, others may disagree, so\n> please take with the necessary quantities of salt!\n> \n> On Fri, 1 Oct 2021 at 11:33, Paul Elder wrote:\n> >\n> > We have multiple goals:\n> > - we need a lock of some sort, to instruct the AEGC to not update output\n> > results\n> > - we need manual modes, to override the values computed by the AEGC\n> > - we need to support seamless transitions from auto -> manual, and do so\n> > without flickering\n> > - we need custom minimum values for the manual controls, that is no\n> > magic values for enabling/disabling auto\n> > - all of these need to be done with AE sub-controls (exposure time,\n> > analogue gain)\n> >\n> > To achieve these goals, we introduce mode controls for the AE\n> > sub-controls: ExposureTimeMode and AnalogueGainMode. These have an auto\n> > state, and a disabled state. The disabled state has an internal one-way\n> > state change from locked to manual, triggered by the presence of the\n> > value-controls (ExposureTime and AnalogueGain).\n> >\n> > We then remove the AeEnable control, as it is a redundant control in the\n> > face of these two mode controls.\n> >\n> > We also remove AeLocked, as it is insufficient for reporting the AE\n> > state, and we promote AeState to non-draft to fill its role. Notably,\n> > the locked state is removed, since this information can be obtained from\n> > the aforementioned mode controls.\n> >\n> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=42\n> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=43\n> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=47\n> > Signed-off-by: Paul Elder \n> >\n> > ---\n> > Changes in v2:\n> > - No changes, just resubmitting at the head of this series so that it's\n> > together and so that /people will actually see it/\n> >\n> > Initial version:\n> > Still RFC as I haven't updated the users of the control yet, and I want\n> > to check that these are the controls and docs that we want.\n> >\n> > We've decided that the \"master AE control\" will be implemented by a\n> > helper... but looking at uvcvideo and the V4L2 controls I'm wondering if\n> > such helper should come earlier than later?\n> > ---\n> > src/libcamera/control_ids.yaml | 215 +++++++++++++++++++++++----------\n> > 1 file changed, 150 insertions(+), 65 deletions(-)\n> >\n> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > index 9d4638ae..18b186e8 100644\n> > --- a/src/libcamera/control_ids.yaml\n> > +++ b/src/libcamera/control_ids.yaml\n> > @@ -7,23 +7,61 @@\n> > # Unless otherwise stated, all controls are bi-directional, i.e. they can be\n> > # set through Request::controls() and returned out through Request::metadata().\n> > controls:\n> > - - AeEnable:\n> > - type: bool\n> > - description: |\n> > - Enable or disable the AE.\n> > -\n> > - \\sa ExposureTime AnalogueGain\n> > -\n> > - - AeLocked:\n> > - type: bool\n> > + - AeState:\n> > + type: int32_t\n> > description: |\n> > - Report the lock status of a running AE algorithm.\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> > + Control to report the current AE algorithm state. Enabling or disabling\n> > + any of the AE-related mode controls (eg. AnalogueGainMode,\n> > + ExposureTimeMode) is not required to reset the state to\n> > + AeStateInactive. The camera device can do several state transitions\n> > + between two results, if it is allowed by the state transition table.\n> > + For example, AeStateInactive may never actually be seen in a result.\n> \n> I was fine until we got to that bit \"Enabling or disabling\". The rest\n> of this paragraph struck me as a real burst of detail and corner cases\n> (?) while I was still struggling with what AeStateInactive meant.\n\nAh, that was because the android description said \"enabling or disabling\nAE must reset the state to inactive\" or something along those lines and\nI just substituted that whole line with \"no\".\n\n> \n> Perhaps some of this specific stuff can be moved until after all the\n> enum values have been explained? I found the enum descriptions quite\n\nNot sure I can move it after the enums... but maybe at the end of the\noverview at lesat?\n\n> easy to follow and having read through those felt in a better place to\n\nThat's good!\n\n> tackle the details. Having said that, I'm still a bit vague as to why\n> \"resetting the state to AeStateInactive\" is a particular issue of\n> concern?\n> \n> > +\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> > + As some AE algorithms may still be running when any of the AE-related\n> > + controls are in manual mode, the states are valid even when the mode\n> > + controls are set to Disabled. To know if this image image used AE or\n> \n> \"image\" is repeated.\n> \n> Do we need to say anything about what happens when all controls are\n> disabled? (Does anything different happen??)\n\nMaybe I should've said \"if at least one is disabled\".\n\n> \n> > + manual (or a mixture), check the relevant modes (eg. AnalogueGainMode,\n> > + ExposureTimeMode).\n> > +\n> > + \\sa AnalogueGain\n> > + \\sa AnalogueGainMode\n> > + \\sa ExposureTime\n> > + \\sa ExposureTimeMode\n> >\n> > - \\sa AeEnable\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> > + - 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> > + - 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> > + - name: AeStateFlashRequired\n> > + value: 3\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: 4\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> \n> This was all good, my only minor comment might be that using\n> AeStateConverged and AeStateSearching in place of just\n> Converged/Searching might be a little more precise?\n\nAh okay. Probably better to be explicit.\n\n> \n> > + \\sa AePrecaptureTrigger\n> >\n> > # AeMeteringMode needs further attention:\n> > # - Auto-generate max enum value.\n> > @@ -93,6 +131,17 @@ controls:\n> > how the desired total exposure is divided between the shutter time\n> > and the sensor's analogue gain. The exposure modes are platform\n> > specific, and not all exposure modes may be supported.\n> > +\n> > + When one of AnalogueGainMode or ExposureTimeMode is set to Disabled and\n> > + a corresponding manual control is provided, AeExposureMode may be\n> \n> Actually I think the same is true even if you don't give a manual\n> value. As soon as you fix any of them, the other(s) will vary in such\n> a way as to ignore the exposure profile.\n\nAh yes you're right.\n\n> \n> > + ignored. This is because, for example, if AeExposureMode is set to\n> > + ExposureLong but ExposureTimeMode is Disabled and a short ExposureTime\n> > + is provided (AnalogueGainMode set to Auto), then the AeExposureMode\n> > + doesn't make sense.\n> \n> The example is good, but seemed a bit wordy to me? Maybe the whole\n> thing might be:\n> \n> \"When any of AnalogueGainMode or ExposureTimeMode is set to Disabled.\n> the fixed values will override any choices made by the AeExposureMode which may\n> consequently be ignored.\"\n\nThat probably is sufficient.\n\n> \n> (Or perhaps I'm assuming a deeper understanding on the part of the\n> reader than most will have?)\n\nThe example was me thinking aloud when coming up with the new rule :p\n\n> \n> > +\n> > + \\sa AnalogueGainMode\n> > + \\sa ExposureTimeMode\n> > +\n> > enum:\n> > - name: ExposureNormal\n> > value: 0\n> > @@ -111,13 +160,15 @@ controls:\n> > type: float\n> > description: |\n> > Specify an Exposure Value (EV) parameter. The EV parameter will only be\n> > - applied if the AE algorithm is currently enabled.\n> > + applied if the AE algorithm is currently enabled, that is, at least one\n> > + of AnalogueGainMode and ExposureTimeMode are auto.\n> >\n> > By convention EV adjusts the exposure as log2. For example\n> > EV = [-2, -1, 0.5, 0, 0.5, 1, 2] results in an exposure adjustment\n> > of [1/4x, 1/2x, 1/sqrt(2)x, 1x, sqrt(2)x, 2x, 4x].\n> >\n> > - \\sa AeEnable\n> > + \\sa AnalogueGainMode\n> > + \\sa ExposureTimeMode\n> >\n> > - ExposureTime:\n> > type: int32_t\n> > @@ -125,17 +176,49 @@ controls:\n> > Exposure time (shutter speed) for the frame applied in the sensor\n> > device. This value is specified in micro-seconds.\n> >\n> > - Setting this value means that it is now fixed and the AE algorithm may\n> > - not change it. Setting it back to zero returns it to the control of the\n> > - AE algorithm.\n> > + This control will only take effect if ExposureTimeMode is Disabled. Its\n> > + presence in a request acts as a trigger to switch to the internal\n> > + manual mode within ExposureTimeModeDisabled.\n> > +\n> > + When reported in metadata, this control indicates what exposure time\n> > + was used for the current request, regardless of ExposureTimeMode.\n> > + ExposureTimeMode will indicate the source of the exposure time value,\n> > + whether it came from the AE algorithm or not.\n> >\n> > - \\sa AnalogueGain AeEnable\n> > + \\sa AnalogueGain\n> > + \\sa ExposureTimeMode\n> >\n> > - \\todo Document the interactions between AeEnable and setting a fixed\n> > - value for this control. Consider interactions with other AE features,\n> > - such as aperture and aperture/shutter priority mode, and decide if\n> > - control of which features should be automatically adjusted shouldn't\n> > - better be handled through a separate AE mode control.\n> > + - ExposureTimeMode:\n> > + type: int32_t\n> > + description: |\n> > + Control to set and report the source of the exposure time that will be\n> > + applied.\n> > +\n> > + \\sa ExposureTime\n> > + enum:\n> > + - name: ExposureTimeModeAuto\n> > + value: 0\n> > + description: |\n> > + The exposure time will be calculated automatically and set by the\n> > + AE algorithm.\n> > + - name: ExposureTimeModeDisabled\n> > + value: 1\n> > + description: |\n> > + The exposure time will not be updated by the AE algorithm. It will\n> > + come from the last calculated value when the mode was Auto, or from\n> > + the value specified in ExposureTime.\n> > +\n> > + This Disabled state has two internal states; locked and manual.\n> \n> TBH I still struggle a bit with the word \"locked\". I think I prefer\n> \"fixed\", it seems more obvious. But not to worry... if the Android\n> world prefers it then I will get used to it!\n\nI'm fine with either, I've just gotten used to saying \"locked\" since\nthat was the informal name of this whole ordeal :/\n\n\nThanks,\n\nPaul\n\n> \n> Thanks again for doing all this!\n> \n> Best regards\n> David\n> \n> > + When the Disabled state is first entered, the internal state will\n> > + be locked, and the latest exposure time value set by the AE\n> > + algorithm will be used (or the default ExposureTime value, if the\n> > + mode started out as Disabled). When an ExposureTime is provided,\n> > + then the internal state will go to manual, and the provided value\n> > + will be used for the exposure time. Going from manual to locked is\n> > + not possible. If an ExposureTime value is provided in the same\n> > + request as going into the Disabled state, then the internal locked\n> > + state will be skipped, and the internal state will start out as\n> > + manual.\n> >\n> > - AnalogueGain:\n> > type: float\n> > @@ -144,17 +227,49 @@ controls:\n> > The value of the control specifies the gain multiplier applied to all\n> > colour channels. This value cannot be lower than 1.0.\n> >\n> > - Setting this value means that it is now fixed and the AE algorithm may\n> > - not change it. Setting it back to zero returns it to the control of the\n> > - AE algorithm.\n> > + This control will only take effect if ExposureTimeMode is Disabled. Its\n> > + presence in a request acts as a trigger to switch to the internal\n> > + manual mode within ExposureTimeModeDisabled.\n> >\n> > - \\sa ExposureTime AeEnable\n> > + When reported in metadata, this control indicates what exposure time\n> > + was used for the current request, regardless of ExposureTimeMode.\n> > + ExposureTimeMode will indicate the source of the exposure time value,\n> > + whether it came from the AE algorithm or not.\n> > +\n> > + \\sa ExposureTime\n> > + \\sa AnalogueGainMode\n> > +\n> > + - AnalogueGainMode:\n> > + type: int32_t\n> > + description: |\n> > + Control to set and report the source of the analogue gain that will be\n> > + applied.\n> >\n> > - \\todo Document the interactions between AeEnable and setting a fixed\n> > - value for this control. Consider interactions with other AE features,\n> > - such as aperture and aperture/shutter priority mode, and decide if\n> > - control of which features should be automatically adjusted shouldn't\n> > - better be handled through a separate AE mode control.\n> > + \\sa AnalogueGain\n> > + enum:\n> > + - name: AnalogueGainModeAuto\n> > + value: 0\n> > + description: |\n> > + The analogue gain will be calculated automatically and set by the\n> > + AE algorithm.\n> > + - name: AnalogueGainModeDisabled\n> > + value: 1\n> > + description: |\n> > + The analogue gain will not be updated by the AE algorithm. It will\n> > + come from the last calculated value when the mode was Auto, or from\n> > + the value specified in AnalogueGain.\n> > +\n> > + This Disabled state has two internal states; locked and manual.\n> > + When the Disabled state is first entered, the internal state will\n> > + be locked, and the latest analogue gain value set by the AE\n> > + algorithm will be used (or the default AnalogueGain value, if the\n> > + mode started out as Disabled). When an AnalogueGain is provided,\n> > + then the internal state will go to manual, and the provided value\n> > + will be used for the analogue gain. Going from manual to locked is\n> > + not possible. If an AnalogueGain value is provided in the same\n> > + request as going into the Disabled state, then the internal locked\n> > + state will be skipped, and the internal state will start out as\n> > + manual.\n> >\n> > - Brightness:\n> > type: float\n> > @@ -477,36 +592,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 1EAD2BDC71\n\tfor ;\n\tMon, 4 Oct 2021 06:17:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7D607691B9;\n\tMon, 4 Oct 2021 08:17:43 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A62886023E\n\tfor ;\n\tMon, 4 Oct 2021 08:17:41 +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 2FAB771;\n\tMon, 4 Oct 2021 08:17:39 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"WDQ3W6n7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1633328261;\n\tbh=D2uUCSRV1kRIhiVh0sASi1noB208HGjtzwt+hzFLpQo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WDQ3W6n7e7ks1SScrzNmWt7d8x2rzS93/9DxiuNk3bycvjszogbpHIxW8r8PCfiSF\n\tAQ5LBec0nYGuRBBVIXRGmzxSzbHOth3oh/H6iha6YTPJU8Q6g0jNva0Rc5eTpb2ChU\n\t6fnaEoODS6bjNM3Qy7KZ21wy//A8fYMzUNAi5VI0=","Date":"Mon, 4 Oct 2021 15:17:33 +0900","From":"paul.elder@ideasonboard.com","To":"David Plowman ","Message-ID":"<20211004061733.GB4221@pyrite.rasen.tech>","References":"<20211001103325.1077590-1-paul.elder@ideasonboard.com>\n\t<20211001103325.1077590-2-paul.elder@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 1/7] controls: Reorganize the\n\tAE-related controls","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":20025,"web_url":"https://patchwork.libcamera.org/comment/20025/","msgid":"<20211004065424.GC4221@pyrite.rasen.tech>","date":"2021-10-04T06:54:24","subject":"Re: [libcamera-devel] [PATCH v2 1/7] controls: Reorganize the\n\tAE-related controls","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nThanks for the feedback.\n\nOn Fri, Oct 01, 2021 at 06:08:19PM +0200, Jacopo Mondi wrote:\n> Hi Paul,\n> \n> On Fri, Oct 01, 2021 at 07:33:19PM +0900, Paul Elder wrote:\n> > We have multiple goals:\n> > - we need a lock of some sort, to instruct the AEGC to not update output\n> > results\n> > - we need manual modes, to override the values computed by the AEGC\n> > - we need to support seamless transitions from auto -> manual, and do so\n> > without flickering\n> > - we need custom minimum values for the manual controls, that is no\n> > magic values for enabling/disabling auto\n> > - all of these need to be done with AE sub-controls (exposure time,\n> > analogue gain)\n> >\n> > To achieve these goals, we introduce mode controls for the AE\n> > sub-controls: ExposureTimeMode and AnalogueGainMode. These have an auto\n> > state, and a disabled state. The disabled state has an internal one-way\n> > state change from locked to manual, triggered by the presence of the\n> > value-controls (ExposureTime and AnalogueGain).\n> >\n> > We then remove the AeEnable control, as it is a redundant control in the\n> > face of these two mode controls.\n> >\n> > We also remove AeLocked, as it is insufficient for reporting the AE\n> > state, and we promote AeState to non-draft to fill its role. Notably,\n> > the locked state is removed, since this information can be obtained from\n> > the aforementioned mode controls.\n> >\n> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=42\n> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=43\n> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=47\n> > Signed-off-by: Paul Elder \n> >\n> > ---\n> > Changes in v2:\n> > - No changes, just resubmitting at the head of this series so that it's\n> > together and so that /people will actually see it/\n> >\n> > Initial version:\n> > Still RFC as I haven't updated the users of the control yet, and I want\n> > to check that these are the controls and docs that we want.\n> >\n> > We've decided that the \"master AE control\" will be implemented by a\n> > helper... but looking at uvcvideo and the V4L2 controls I'm wondering if\n> > such helper should come earlier than later?\n> > ---\n> > src/libcamera/control_ids.yaml | 215 +++++++++++++++++++++++----------\n> > 1 file changed, 150 insertions(+), 65 deletions(-)\n> >\n> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > index 9d4638ae..18b186e8 100644\n> > --- a/src/libcamera/control_ids.yaml\n> > +++ b/src/libcamera/control_ids.yaml\n> > @@ -7,23 +7,61 @@\n> > # Unless otherwise stated, all controls are bi-directional, i.e. they can be\n> > # set through Request::controls() and returned out through Request::metadata().\n> > controls:\n> > - - AeEnable:\n> > - type: bool\n> > - description: |\n> > - Enable or disable the AE.\n> > -\n> > - \\sa ExposureTime AnalogueGain\n> > -\n> > - - AeLocked:\n> > - type: bool\n> > + - AeState:\n> \n> Now that we have two separate controls for the exposure time and\n> analogue gain, do we want a single state for the whole AEGC block ?\n\nI think so. I learned a while back that the whole AEGC block takes both\nE and G and outputs both E and G and that AEC and AGC are not separate\nblocks. Thus we have separate controls for the input and output, but a\nunified state for the state.\n\n> \n> > + type: int32_t\n> > description: |\n> > - Report the lock status of a running AE algorithm.\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> > + Control to report the current AE algorithm state. Enabling or disabling\n> > + any of the AE-related mode controls (eg. AnalogueGainMode,\n> > + ExposureTimeMode) is not required to reset the state to\n> > + AeStateInactive. The camera device can do several state transitions\n> \n> I understand that, from where we come from the\n> \n> \"Enabling or disabling any of the AE-related mode controls (eg. AnalogueGainMode,\n> ExposureTimeMode) is not required to reset the state to\n> AeStateInactive.\"\n> \n> makes sense, but you're reading this for the first time you would be\n> wondering why is this information here, even before what the inactive\n> state is has been defined. If you consider this relevant, I would\n> place this in the Inactive state description itself\n\nI think it's a rule for the control in general, so I'll move it to the\nend of the overview.\n\n> \n> > + between two results, if it is allowed by the state transition table.\n> \n> which table ? :)\n\nIt's just a different representation of a table :p\n\n> \n> > + For example, AeStateInactive may never actually be seen in a result.\n> \n> I would place this information in the transition table if we have one\n\nIt's a general rule about the control, so I think it belongs in the\noverview. It can be any state, it's just that inactive is the most\ntrivial example.\n\n> \n> > +\n> > + The state in the result is the state for this image (in sync with this\n> \n> The () part is a repetition, and doesn't this apply to all controls\n> part of a result ? Don't they all apply to the frames part of the\n> request they are part of ? (Also I would refer to requests, not\n> images)\n\nWhoops, yeah it is. I just wanted to be explicit (not sure why).\n\n> \n> > + image). If AE state becomes AeStateConverged, then the image data\n> > + associated with the result should be good to use.\n> > +\n> > + As some AE algorithms may still be running when any of the AE-related\n> > + controls are in manual mode, the states are valid even when the mode\n> \n> What is manual mode ? Don't we have an \"enabled\"/\"disabled\" state only\n> ?\n\nOh yeah s/manual/disabled/ might be better here.\n\n> \n> I would just say that the state is reported even if the exposure time\n> and the analogue gain algorithms are disabled\n\nYeah.\n\n> \n> > + controls are set to Disabled. To know if this image image used AE or\n> > + manual (or a mixture), check the relevant modes (eg. AnalogueGainMode,\n> > + ExposureTimeMode).\n> > +\n> > + \\sa AnalogueGain\n> > + \\sa AnalogueGainMode\n> > + \\sa ExposureTime\n> > + \\sa ExposureTimeMode\n> >\n> > - \\sa AeEnable\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> > + - 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> > + - 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> > + - name: AeStateFlashRequired\n> > + value: 3\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: 4\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> > + \\sa AePrecaptureTrigger\n> \n> We don't have AE capture triggers, nor a transition table defined at\n\nThis is a table in list form :p\n\nidk how capture triggers work yet. I pulled the state in from android.\n\n> all. And as said above, now we have two controls for exposure and\n> analog gain, and a single state for the whole AE.\n> \n> Do we want AeState at all ?\n\nI think we do. We still need a way to report at least AE converged vs\nnot-converged, and the state is shared between AEC and AGC as they're\nlinked together.\n\n> \n> >\n> > # AeMeteringMode needs further attention:\n> > # - Auto-generate max enum value.\n> > @@ -93,6 +131,17 @@ controls:\n> > how the desired total exposure is divided between the shutter time\n> > and the sensor's analogue gain. The exposure modes are platform\n> > specific, and not all exposure modes may be supported.\n> > +\n> > + When one of AnalogueGainMode or ExposureTimeMode is set to Disabled and\n> > + a corresponding manual control is provided, AeExposureMode may be\n> > + ignored. This is because, for example, if AeExposureMode is set to\n> > + ExposureLong but ExposureTimeMode is Disabled and a short ExposureTime\n> > + is provided (AnalogueGainMode set to Auto), then the AeExposureMode\n> > + doesn't make sense.\n> \n> Why not just:\n> \n> This controls takes effect only if ExposureTimeMode is set\n> to ExposureTimeModeAuto and it's ignored otherwise\n\nAnalogueGainMode also affects this afaict.\n\nIn any case, you also think that the example is unnecessary? (I suppose\nit was just me reasoning aloud...)\n\n> \n> > +\n> > + \\sa AnalogueGainMode\n> > + \\sa ExposureTimeMode\n> > +\n> > enum:\n> > - name: ExposureNormal\n> > value: 0\n> > @@ -111,13 +160,15 @@ controls:\n> > type: float\n> > description: |\n> > Specify an Exposure Value (EV) parameter. The EV parameter will only be\n> > - applied if the AE algorithm is currently enabled.\n> > + applied if the AE algorithm is currently enabled, that is, at least one\n> > + of AnalogueGainMode and ExposureTimeMode are auto.\n> \n> Mostly a question for RPi who upstreamed this, but\n> I don't fully get how ExposureValue works. Seems like it is used as a\n> global multiplier for the analogue gain in AGC::computeGain. Is it\n> worth reworking it or is it just me that I'm missing some part ?\n> \n> >\n> > By convention EV adjusts the exposure as log2. For example\n> > EV = [-2, -1, 0.5, 0, 0.5, 1, 2] results in an exposure adjustment\n> > of [1/4x, 1/2x, 1/sqrt(2)x, 1x, sqrt(2)x, 2x, 4x].\n> >\n> > - \\sa AeEnable\n> > + \\sa AnalogueGainMode\n> > + \\sa ExposureTimeMode\n> >\n> > - ExposureTime:\n> > type: int32_t\n> > @@ -125,17 +176,49 @@ controls:\n> > Exposure time (shutter speed) for the frame applied in the sensor\n> > device. This value is specified in micro-seconds.\n> >\n> > - Setting this value means that it is now fixed and the AE algorithm may\n> > - not change it. Setting it back to zero returns it to the control of the\n> > - AE algorithm.\n> > + This control will only take effect if ExposureTimeMode is Disabled. Its\n> > + presence in a request acts as a trigger to switch to the internal\n> > + manual mode within ExposureTimeModeDisabled.\n> \n> I find the concept of \"internal manual mode\" confusing, and I think we\n> should not insert it in the documentation.\n\nWe have a premise that \"controls that are submitted in one request but\nnot resubmitted in subsequent requests still have their value\nmaintained\", and we have these control values that act as triggers for\nlocked -> manual which violates this premise. Thus we either need to\ndocument that exception (which I think mentioning the internal state is\nuseful), or get rid of the premise and require all unchanging control\nvalues to still require resubmission.\n\n> \n> This control will only take effect if ExposureTimeMode is Disabled.\n> \n> ^ take or takes ?\n\nThe control will -> take\n\nThe control (no will) -> takes\n\nThe controls will -> take\n\nThe controls (no will) -> take\n\n> \n> and is ignored when ExposureTimeMode is set to Auto.\n> \n> \n> > + When reported in metadata, this control indicates what exposure time\n> > + was used for the current request, regardless of ExposureTimeMode.\n> > + ExposureTimeMode will indicate the source of the exposure time value,\n> > + whether it came from the AE algorithm or not.\n> >\n> > - \\sa AnalogueGain AeEnable\n> > + \\sa AnalogueGain\n> > + \\sa ExposureTimeMode\n> >\n> > - \\todo Document the interactions between AeEnable and setting a fixed\n> > - value for this control. Consider interactions with other AE features,\n> > - such as aperture and aperture/shutter priority mode, and decide if\n> > - control of which features should be automatically adjusted shouldn't\n> > - better be handled through a separate AE mode control.\n> > + - ExposureTimeMode:\n> \n> So, I don't want to be too picky, but does it make sense to say a\n> \"Mode\" is either \"Auto\" or \"Disabled\" ? Aren't \"AutoExposure = {Enable,\n> Disabled}\" or \"ExposureMode = {Auto, Manual}\" better ? I'm asking\n> without having a real answer to the question.\n\nI don't have a real answer either. I heard objections against \"Mode\" but\nnobody proposed a better alternative :D\n\nSame thing with \"Disabled\". It's not \"manual\" or \"locked\" or \"fixed\"\nalone, it's a superposition of the two, so we need a word that can\nencompass those :/\n\n> \n> > + type: int32_t\n> > + description: |\n> > + Control to set and report the source of the exposure time that will be\n> > + applied.\n> \n> Controls how the frame exposure time is computed. When set\n> to \"Auto\" the auto-exposure algorithm computes the exposure\n> time of the next frame and configures the image sensor\n> accordingly. When set to \"Disabled\" the auto-exposure\n> algorithm stops computing the exposure time.\n\nOh that's good.\n\n> \n> > + \\sa ExposureTime\n> > + enum:\n> > + - name: ExposureTimeModeAuto\n> > + value: 0\n> > + description: |\n> > + The exposure time will be calculated automatically and set by the\n> > + AE algorithm.\n> > + - name: ExposureTimeModeDisabled\n> > + value: 1\n> > + description: |\n> > + The exposure time will not be updated by the AE algorithm. It will\n> > + come from the last calculated value when the mode was Auto, or from\n> > + the value specified in ExposureTime.\n> > +\n> > + This Disabled state has two internal states; locked and manual.\n> > + When the Disabled state is first entered, the internal state will\n> > + be locked, and the latest exposure time value set by the AE\n> > + algorithm will be used (or the default ExposureTime value, if the\n> > + mode started out as Disabled). When an ExposureTime is provided,\n> > + then the internal state will go to manual, and the provided value\n> > + will be used for the exposure time. Going from manual to locked is\n> > + not possible. If an ExposureTime value is provided in the same\n> > + request as going into the Disabled state, then the internal locked\n> > + state will be skipped, and the internal state will start out as\n> > + manual.\n> \n> Is it useful to define such internal states ? Isn't the same expressed\n> with ?\n> \n> When transitioning from Auto to Disabled mode the last\n> computed exposure value is used until a new value is\n> specified through the ExposureTime control. If an\n> ExposureTime value is specified in the same request where\n> the ExposureTimeMode is set to Disabled, the provided\n> ExposureTime is applied immediately.\n\nTrue, that is sufficient. I was thinking that there was some value in\nbeing explicit with states...\n\n> \n> I also think we need to describe how to perform a 'flickerless'\n\nAh yeah we should.\n\n> transition, but that depends on what we decide to do with AeState. If\n> I'm not mistaken we can deduce that the exposure value is 'locked' by\n> inspecting the ExposureTimeMode value in the result.\n\nYeah, AeState doesn't contribute to determining if AEC or AGC are\nlocked. Those are determined by the two Mode controls. AeState is for\ndetermining if the combined AEGC has converged or not.\n\nHowever, the two Mode controls are not sufficient for determining if the\ninternal AEC/AGC states are actually in the \"locked\" state (for the\npurpose of reporting Lock to android). But that's in a later patch, and\nwe just tag that state onto the request descriptor.\n\n> \n> We can have a dedicated section in the control description. I'll make\n> an attempt but I'm sure this can be vastly improved.\n> \n> - Flicker-less manual exposure time transition:\n> \n> Applications that transition from ExposureTimeMode\n> Auto mode to the direct control of the exposure time through\n> the ExposureTime control should aim to do so by selecting\n> an ExposureTime value as close as possible to the last\n> value computed by the auto exposure algorithm to avoid\n> any visible flickering.\n> \n> In order to select the correct value to use in the first\n> Request with the ExposureTime control specified,\n> applications should accommodate the natural delay in\n> applying controls caused by the capture pipeline frame\n> depth.\n> \n> When switching to manual exposure time control,\n> applications should not immediately specify an\n> ExposureTime value in the same request where\n> ExposureTimeMode is set to Disabled. They should instead\n> wait for the first Request where ExposureTimeMode is\n> returned as Disabled in the Request metadata and use the\n> there returned exposure time to populate the ExposureTime\n> value in the next queued Request.\n\nOoh that's good.\n\n> \n> The implementation of the auto-exposure time algorithm\n> should equally try to minimize flickering and when\n> transitioning from manual exposure time control to auto\n> exposure, the last used value should be used as starting\n> point.\n> \n> I'm not sure about the last paragraph though, does it match the\n> outcome of our last discussion ?\n\nI'm not sure I understand the purpose of the last paragraph. It seems\nmore directed to IPA developers, that their AEGC should minimize\nflickering (afaict that's in their interest already), and that they need\nto use the last used value as a stating point when switching from\nmanual to auto. Maybe the latter point would be good to mention here?\n\n> \n> >\n> > - AnalogueGain:\n> > type: float\n> > @@ -144,17 +227,49 @@ controls:\n> > The value of the control specifies the gain multiplier applied to all\n> > colour channels. This value cannot be lower than 1.0.\n> >\n> > - Setting this value means that it is now fixed and the AE algorithm may\n> > - not change it. Setting it back to zero returns it to the control of the\n> > - AE algorithm.\n> > + This control will only take effect if ExposureTimeMode is Disabled. Its\n> > + presence in a request acts as a trigger to switch to the internal\n> > + manual mode within ExposureTimeModeDisabled.\n> \n> s/ExposureTimeMode/AnalogueGainMode ?\n> >\n> > - \\sa ExposureTime AeEnable\n> > + When reported in metadata, this control indicates what exposure time\n> > + was used for the current request, regardless of ExposureTimeMode.\n> > + ExposureTimeMode will indicate the source of the exposure time value,\n> > + whether it came from the AE algorithm or not.\n> \n> Again, all the 'exposure' should be 'analogue gain' instead ?\n\nOops, yeah :p\n\nI forgot to fix it in the copy-paste...\n\n\nThanks,\n\nPaul\n\n> \n> > +\n> > + \\sa ExposureTime\n> > + \\sa AnalogueGainMode\n> > +\n> > + - AnalogueGainMode:\n> > + type: int32_t\n> > + description: |\n> > + Control to set and report the source of the analogue gain that will be\n> > + applied.\n> >\n> > - \\todo Document the interactions between AeEnable and setting a fixed\n> > - value for this control. Consider interactions with other AE features,\n> > - such as aperture and aperture/shutter priority mode, and decide if\n> > - control of which features should be automatically adjusted shouldn't\n> > - better be handled through a separate AE mode control.\n> > + \\sa AnalogueGain\n> > + enum:\n> > + - name: AnalogueGainModeAuto\n> > + value: 0\n> > + description: |\n> > + The analogue gain will be calculated automatically and set by the\n> > + AE algorithm.\n> > + - name: AnalogueGainModeDisabled\n> > + value: 1\n> > + description: |\n> > + The analogue gain will not be updated by the AE algorithm. It will\n> > + come from the last calculated value when the mode was Auto, or from\n> > + the value specified in AnalogueGain.\n> > +\n> > + This Disabled state has two internal states; locked and manual.\n> > + When the Disabled state is first entered, the internal state will\n> > + be locked, and the latest analogue gain value set by the AE\n> > + algorithm will be used (or the default AnalogueGain value, if the\n> > + mode started out as Disabled). When an AnalogueGain is provided,\n> > + then the internal state will go to manual, and the provided value\n> > + will be used for the analogue gain. Going from manual to locked is\n> > + not possible. If an AnalogueGain value is provided in the same\n> > + request as going into the Disabled state, then the internal locked\n> > + state will be skipped, and the internal state will start out as\n> > + manual.\n> \n> Same comments as per the ExposureTimeMode control.\n> \n> >\n> > - Brightness:\n> > type: float\n> > @@ -477,36 +592,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 40431C3243\n\tfor ;\n\tMon, 4 Oct 2021 06:54:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 94885691BB;\n\tMon, 4 Oct 2021 08:54: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 7F9D06023E\n\tfor ;\n\tMon, 4 Oct 2021 08:54:33 +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 E61365A1;\n\tMon, 4 Oct 2021 08:54:31 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Tq2M3vug\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1633330473;\n\tbh=iVjkEx2GQzMu98d71RZ/XJj+oruZEKyze55DZGYUmes=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Tq2M3vugNee6Ej4PywN0NrD+AQsGdKKMEEkAO9znG1eYGB/z7xhA0NeJPOEA9Z2d7\n\tTCsTg3+RTsUChiZQzA4BUDPZNz3l+SfTK1CgVQZnQN7HdyLiuSgZkRiE/kZP9RUtxQ\n\tgD92OEpJr3BgXnRz7kTHNsFtUIj2hREtg10LeNoE=","Date":"Mon, 4 Oct 2021 15:54:24 +0900","From":"paul.elder@ideasonboard.com","To":"Jacopo Mondi ","Message-ID":"<20211004065424.GC4221@pyrite.rasen.tech>","References":"<20211001103325.1077590-1-paul.elder@ideasonboard.com>\n\t<20211001103325.1077590-2-paul.elder@ideasonboard.com>\n\t<20211001160819.oqav7qozcggi2kq5@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20211001160819.oqav7qozcggi2kq5@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 1/7] controls: Reorganize the\n\tAE-related controls","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]