[{"id":20020,"web_url":"https://patchwork.libcamera.org/comment/20020/","msgid":"<20211001173248.athwfrz6nx4ztlnk@uno.localdomain>","date":"2021-10-01T17:32:48","subject":"Re: [libcamera-devel] [PATCH v2 6/7] android: Plumb all AE-related\n\tcontrols","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:24PM +0900, Paul Elder wrote:\n> With the AE-related controls reorganized in libcamera, with separate\n> value and lever controls for exposure and gain, plumb these through the\n> HAL layer (in order of appearence in the patch):\n> - static metadata: available AE modes, AE lock available\n> - manual template: add AE off (the other templates already have AE on)\n> - request metadata: HAL -> libcamera controls conversion\n> - result metadata: libcamera -> HAL controls conversion\n> - result metadata: AE state\n\nSo I think they should go in separate patches maybe\n\n>\n> We add class variables to CameraDevice to save the last set android AE\n> controls, as that is how android controls function (no need to resubmit\n> controls if they are unchanged).\n\nDo we work any differently ? It's an honest question, I always assumed\nwe don't.\n\n>\n> We also save libcamera's AE state in the request descriptor, as\n> otherwise there is insufficient information from libcamera's result\n> metadata alone to tell if the state is locked or manual (as they are an\n> internal state in libcamera).\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 <paul.elder@ideasonboard.com>\n>\n> ---\n> New in v2\n> ---\n>  src/android/camera_capabilities.cpp | 63 ++++++++++++++++---\n>  src/android/camera_device.cpp       | 98 +++++++++++++++++++++++++++--\n>  src/android/camera_device.h         | 16 +++++\n>  3 files changed, 165 insertions(+), 12 deletions(-)\n>\n> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> index f7a6cda9..3fed3f83 100644\n> --- a/src/android/camera_capabilities.cpp\n> +++ b/src/android/camera_capabilities.cpp\n> @@ -830,12 +830,62 @@ int CameraCapabilities::initializeStaticMetadata()\n>  \tstaticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_ANTIBANDING_MODES,\n>  \t\t\t\t  aeAvailableAntiBandingModes);\n>\n> -\tstd::vector<uint8_t> aeAvailableModes = {\n> -\t\tANDROID_CONTROL_AE_MODE_ON,\n> -\t};\n> +\tstd::vector<uint8_t> aeAvailableModes;\n> +\t/* This will cause problems if one supports only on and the other only off */\n> +\tbool aeAddedOn = false;\n> +\tbool aeAddedOff = false;\n> +\n> +\tconst auto &analogGainModeInfo = controlsInfo.find(&controls::AnalogueGainMode);\n> +\tif (analogGainModeInfo != controlsInfo.end()) {\n> +\t\tfor (const auto &value : analogGainModeInfo->second.values()) {\n> +\t\t\tswitch (value.get<int32_t>()) {\n> +\t\t\tcase controls::AnalogueGainModeAuto:\n> +\t\t\t\tif (!aeAddedOn)\n\nHow can aeAddedOn be true if it's initialized to false and set to true\njust here below, unless you expect the same value to be specified\ntwice in the info.values() ?\n\nShould you just add the value unconditionally here and below ?\n\n> +\t\t\t\t\taeAvailableModes.push_back(ANDROID_CONTROL_AE_MODE_ON);\n> +\t\t\t\taeAddedOn = true;\n> +\t\t\t\tbreak;\n> +\t\t\tcase controls::AnalogueGainModeDisabled:\n> +\t\t\t\tif (!aeAddedOff)\n> +\t\t\t\t\taeAvailableModes.push_back(ANDROID_CONTROL_AE_MODE_OFF);\n> +\t\t\t\taeAddedOff = true;\n> +\t\t\t\tbreak;\n> +\t\t\tdefault:\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\n> +\tconst auto &exposureTimeModeInfo = controlsInfo.find(&controls::ExposureTimeMode);\n> +\tif (exposureTimeModeInfo != controlsInfo.end()) {\n> +\t\tfor (const auto &value : exposureTimeModeInfo->second.values()) {\n> +\t\t\tswitch (value.get<int32_t>()) {\n> +\t\t\tcase controls::ExposureTimeModeAuto:\n> +\t\t\t\tif (!aeAddedOn)\n> +\t\t\t\t\taeAvailableModes.push_back(ANDROID_CONTROL_AE_MODE_ON);\n> +\t\t\t\taeAddedOn = true;\n\nAnd setting the aeAdded to true here has no purpose, but that's not an\nissue.\n\nWhat is the policy we want to have here ? Android has a single AE\ncontrol, we have two. I don't think we can allow situation where we\nhave an asymmetric ExposureTimeMode and AnalogueGainMode as matching\non which one to add would become painful.\n\nI would cut it short and requires libcamera implementation that aims\nto HAL full level to have both gain and exposure time controllable.\n\nThe code can thus be restructured as\n\n        availableModes = { AE_MODE_ON };\n        if (exposureTimeInfo != end && analogGainInfo != end) {\n                bool manualExp = false;\n                bool manualGain = false;\n\n                for (modes : exposureTimeInfo.values())\n                        if (mode == Disabled)\n                                manualExp = true;\n                                break;\n\n               for (modes : analogGainInfo.values())\n                        if (mode == Disabled)\n                                manualGain = true;\n                                break\n\n               if (manualExp && manualGain)\n                        availableModes.push_back(AE_MODE_OFF);\n        }\n> +\t\t\t\tbreak;\n> +\t\t\tcase controls::ExposureTimeModeDisabled:\n> +\t\t\t\tif (!aeAddedOff)\n> +\t\t\t\t\taeAvailableModes.push_back(ANDROID_CONTROL_AE_MODE_OFF);\n> +\t\t\t\taeAddedOff = true;\n\n\n> +\t\t\t\tbreak;\n> +\t\t\tdefault:\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\n> +\tif (aeAvailableModes.empty())\n> +\t\taeAvailableModes.push_back(ANDROID_CONTROL_AE_MODE_ON);\n>  \tstaticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_MODES,\n>  \t\t\t\t  aeAvailableModes);\n>\n> +\t/* In libcamera, turning off AE is equivalient to locking. */\n> +\tuint8_t aeLockAvailable = aeAddedOff ? ANDROID_CONTROL_AE_LOCK_AVAILABLE_TRUE\n> +\t\t\t\t\t     : ANDROID_CONTROL_AE_LOCK_AVAILABLE_FALSE;\n\nThis really depends on what we want to do with AeState.\nI think if we want to report locking from there we should make sure\nthat among the supported AeState values there is locked. Bummer, we\ndon't have the Camera::controls() equivalent for metadata, hence we\ndon't have a way to report the values supported for metadata-only\ncontrols as AeState is\n\n> +\tstaticMetadata_->addEntry(ANDROID_CONTROL_AE_LOCK_AVAILABLE,\n> +\t\t\t\t  aeLockAvailable);\n> +\n>  \tint64_t minFrameDurationNsec = -1;\n>  \tint64_t maxFrameDurationNsec = -1;\n>  \tconst auto frameDurationsInfo = controlsInfo.find(&controls::FrameDurationLimits);\n> @@ -946,10 +996,6 @@ int CameraCapabilities::initializeStaticMetadata()\n>  \tstaticMetadata_->addEntry(ANDROID_CONTROL_SCENE_MODE_OVERRIDES,\n>  \t\t\t\t  sceneModesOverride);\n>\n> -\tuint8_t aeLockAvailable = ANDROID_CONTROL_AE_LOCK_AVAILABLE_FALSE;\n> -\tstaticMetadata_->addEntry(ANDROID_CONTROL_AE_LOCK_AVAILABLE,\n> -\t\t\t\t  aeLockAvailable);\n> -\n>  \tuint8_t awbLockAvailable = ANDROID_CONTROL_AWB_LOCK_AVAILABLE_FALSE;\n>  \tstaticMetadata_->addEntry(ANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n>  \t\t\t\t  awbLockAvailable);\n> @@ -1358,6 +1404,9 @@ std::unique_ptr<CameraMetadata> CameraCapabilities::requestTemplateManual() cons\n>  \tif (!manualTemplate)\n>  \t\treturn nullptr;\n>\n> +\tuint8_t aeMode = ANDROID_CONTROL_AE_MODE_OFF;\n> +\tmanualTemplate->addEntry(ANDROID_CONTROL_AE_MODE, aeMode);\n\nOnly if supported, or unconditionally ?\n\n> +\n>  \treturn manualTemplate;\n>  }\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index ef4fbab8..d5027ec5 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -263,7 +263,9 @@ CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(\n>\n>  CameraDevice::CameraDevice(unsigned int id, std::shared_ptr<Camera> camera)\n>  \t: id_(id), state_(State::Stopped), camera_(std::move(camera)),\n> -\t  facing_(CAMERA_FACING_FRONT), orientation_(0)\n> +\t  facing_(CAMERA_FACING_FRONT), orientation_(0),\n> +\t  aeOn_(true), aeLocked_(false), lastExposureTime_(0),\n> +\t  lastAnalogueGain_(1.0f), lastDigitalGain_(1.0f)\n>  {\n>  \tcamera_->requestCompleted.connect(this, &CameraDevice::requestComplete);\n>\n> @@ -857,6 +859,62 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)\n>  \t\tcontrols.set(controls::draft::TestPatternMode, testPatternMode);\n>  \t}\n>\n> +\t/*\n> +\t * \\todo Can we trust that we won't receive values that we didn't\n> +\t * report supporting?\n> +\t */\n> +\tif (settings.getEntry(ANDROID_CONTROL_AE_LOCK, &entry))\n> +\t\taeLocked_ = *entry.data.u8 != ANDROID_CONTROL_AE_LOCK_OFF;\n> +\n> +\tif (settings.getEntry(ANDROID_CONTROL_AE_MODE, &entry))\n> +\t\taeOn_ = *entry.data.u8 != ANDROID_CONTROL_AE_MODE_OFF;\n> +\n> +\tAeMode aeMode;\n> +\tif (aeLocked_ && aeOn_)\n> +\t\taeMode = AeMode::Lock;\n> +\telse if (aeLocked_ && !aeOn_)\n> +\t\taeMode = AeMode::Manual;\n> +\telse if (!aeLocked_ && aeOn_)\n> +\t\taeMode = AeMode::Auto;\n> +\telse /* !aeLocked_ && !aeOn_ */\n> +\t\taeMode = AeMode::Manual;\n\n        if (!aeOn_)\n                aeMode = Manual;\n        else if (aeLocked)\n                aeMode = Lock;\n        else\n                aeMode = Auto\n> +\n> +\t/* Save this so that we can recover it in the result */\n> +\tdescriptor->aeMode_ = aeMode;\n> +\n> +\tconst auto &eInfo = camera_->controls().find(&controls::ExposureTimeMode);\n\nDoing so at every request is not nasty, but if we could avoid it it\nwould nice. Don't we have a list of capabilities to rely on ?\n\n> +\tif (eInfo != camera_->controls().end()) {\n> +\t\tcontrols.set(controls::ExposureTimeMode,\n> +\t\t\t\taeMode == AeMode::Auto ?\n> +\t\t\t\tcontrols::ExposureTimeModeAuto :\n> +\t\t\t\tcontrols::ExposureTimeModeDisabled);\n\nI'll ask again, do we expect the same controls to be in every request\neven if they do not change ?\n\n> +\t}\n> +\n> +\tconst auto &gInfo = camera_->controls().find(&controls::AnalogueGainMode);\n> +\tif (gInfo != camera_->controls().end()) {\n> +\t\tcontrols.set(controls::AnalogueGainMode,\n> +\t\t\t\taeMode == AeMode::Auto ?\n> +\t\t\t\tcontrols::AnalogueGainModeAuto :\n> +\t\t\t\tcontrols::AnalogueGainModeDisabled);\n> +\t}\n> +\n> +\tif (settings.getEntry(ANDROID_SENSOR_EXPOSURE_TIME, &entry)) {\n> +\t\tconst auto &info = camera_->controls().find(&controls::ExposureTime);\n\nShould we make sure at capabilities contruction time that if\nExposureTimeMode supports Disabled, then ExposureTime is present ?\nIf we assume so, and make it a requisite to claim we support\nAE_MODE_OFF then these controls should only be handled if\ncapabilities.contain(MANUAL_SENSOR)\n\n> +\t\tif (info != camera_->controls().end()) {\n> +\t\t\tlastExposureTime_ = (*entry.data.i64) / 1000;\n> +\t\t\t/* Don't disable libcamera's internal AeMode::Lock */\n> +\t\t\tif (aeMode != AeMode::Lock)\n\nDo we expect a request to contain both AE_LOCK and a sensor exposure\ntime ?\n\n> +\t\t\t\tcontrols.set(controls::ExposureTime, lastExposureTime_);\n> +\t\t}\n> +\t}\n> +\n> +\t/* Trigger libcamera's locked -> manual state change */\n> +\tif (aeMode == AeMode::Manual && !settings.hasEntry(ANDROID_SENSOR_EXPOSURE_TIME)) {\n> +\t\tconst auto &info = camera_->controls().find(&controls::ExposureTime);\n> +\t\tif (info != camera_->controls().end())\n> +\t\t\tcontrols.set(controls::ExposureTime, lastExposureTime_);\n\nIf we don't need to repeat the control values in every request, is\nthis necessary ? Or are you afraid we receive a AE_MODE_OFF and no\nSENSOR_EXPOSURE_TIME ?\n\n> +\t}\n> +\n>  \treturn 0;\n>  }\n>\n> @@ -1345,11 +1403,16 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons\n>  \tresultMetadata->addEntry(ANDROID_CONTROL_AE_EXPOSURE_COMPENSATION,\n>  \t\t\t\t value32);\n>\n> -\tvalue = ANDROID_CONTROL_AE_LOCK_OFF;\n> -\tresultMetadata->addEntry(ANDROID_CONTROL_AE_LOCK, value);\n> +\tuint8_t aeLock = (descriptor.aeMode_ == AeMode::Lock)\n> +\t\t       ? ANDROID_CONTROL_AE_LOCK_ON\n> +\t\t       : ANDROID_CONTROL_AE_LOCK_OFF;\n> +\tresultMetadata->addEntry(ANDROID_CONTROL_AE_LOCK, aeLock);\n>\n> -\tvalue = ANDROID_CONTROL_AE_MODE_ON;\n> -\tresultMetadata->addEntry(ANDROID_CONTROL_AE_MODE, value);\n> +\t/* Locked means auto + locked in android */\n> +\tuint8_t aeMode = (descriptor.aeMode_ != AeMode::Manual)\n> +\t\t       ? ANDROID_CONTROL_AE_MODE_ON\n> +\t\t       : ANDROID_CONTROL_AE_MODE_OFF;\n> +\tresultMetadata->addEntry(ANDROID_CONTROL_AE_MODE, aeMode);\n>\n>  \tif (settings.getEntry(ANDROID_CONTROL_AE_TARGET_FPS_RANGE, &entry))\n>  \t\t/*\n> @@ -1366,6 +1429,31 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons\n>  \tresultMetadata->addEntry(ANDROID_CONTROL_AE_PRECAPTURE_TRIGGER, value);\n>\n>  \tvalue = ANDROID_CONTROL_AE_STATE_CONVERGED;\n> +\tif (metadata.contains(controls::AeState)) {\n> +\t\tswitch (metadata.get(controls::AeState)) {\n> +\t\tcase controls::AeStateInactive:\n> +\t\t\tvalue = ANDROID_CONTROL_AE_STATE_INACTIVE;\n> +\t\t\tbreak;\n> +\t\tcase controls::AeStateSearching:\n> +\t\t\tvalue = ANDROID_CONTROL_AE_STATE_SEARCHING;\n> +\t\t\tbreak;\n> +\t\tcase controls::AeStateConverged:\n> +\t\t\tvalue = ANDROID_CONTROL_AE_STATE_CONVERGED;\n> +\t\t\tbreak;\n> +\t\tcase controls::AeStateFlashRequired:\n> +\t\t\tvalue = ANDROID_CONTROL_AE_STATE_FLASH_REQUIRED;\n> +\t\t\tbreak;\n> +\t\tcase controls::AeStatePrecapture:\n> +\t\t\tvalue = ANDROID_CONTROL_AE_STATE_PRECAPTURE;\n> +\t\t\tbreak;\n> +\t\tdefault:\n> +\t\t\tif (descriptor.aeMode_ == AeMode::Lock) {\n> +\t\t\t\tvalue = ANDROID_CONTROL_AE_STATE_LOCKED;\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t\tLOG(HAL, Error) << \"Invalid AeState, setting converged\";\n> +\t\t}\n> +\t}\n>  \tresultMetadata->addEntry(ANDROID_CONTROL_AE_STATE, value);\n>\n>  \tvalue = ANDROID_CONTROL_AF_MODE_OFF;\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index b7d774fe..f693cdbc 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -73,6 +73,12 @@ private:\n>\n>  \tCameraDevice(unsigned int id, std::shared_ptr<libcamera::Camera> camera);\n>\n> +\tenum class AeMode {\n> +\t\tAuto,\n> +\t\tLock,\n> +\t\tManual,\n> +\t};\n> +\n>  \tstruct Camera3RequestDescriptor {\n>  \t\tenum class Status {\n>  \t\t\tPending,\n> @@ -96,6 +102,9 @@ private:\n>\n>  \t\tcamera3_capture_result_t captureResult_ = {};\n>  \t\tStatus status_ = Status::Pending;\n> +\n> +\t\t/* The libcamera internal AE state for this request */\n> +\t\tAeMode aeMode_ = AeMode::Auto;\n>  \t};\n>\n>  \tenum class State {\n> @@ -146,6 +155,13 @@ private:\n>  \tint facing_;\n>  \tint orientation_;\n>\n> +\t/* Track the last-set android AE controls */\n> +\tbool aeOn_;\n> +\tbool aeLocked_;\n\nI guess this is again about if we have to repeat controls every\nrequest, otherwise they don't seem to be required as class members..\n\nThanks\n  j\n\n> +\tint32_t lastExposureTime_;\n> +\tfloat lastAnalogueGain_;\n> +\tfloat lastDigitalGain_;\n> +\n>  \tCameraMetadata lastSettings_;\n>  };\n>\n> --\n> 2.27.0\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 A8959C3243\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri,  1 Oct 2021 17:32:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1718D691AC;\n\tFri,  1 Oct 2021 19:32:04 +0200 (CEST)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AFD5D691A6\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  1 Oct 2021 19:32:02 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id DA2374000A;\n\tFri,  1 Oct 2021 17:32:01 +0000 (UTC)"],"Date":"Fri, 1 Oct 2021 19:32:48 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Paul Elder <paul.elder@ideasonboard.com>","Message-ID":"<20211001173248.athwfrz6nx4ztlnk@uno.localdomain>","References":"<20211001103325.1077590-1-paul.elder@ideasonboard.com>\n\t<20211001103325.1077590-7-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211001103325.1077590-7-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 6/7] android: Plumb all AE-related\n\tcontrols","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":20031,"web_url":"https://patchwork.libcamera.org/comment/20031/","msgid":"<20211004085257.GE4221@pyrite.rasen.tech>","date":"2021-10-04T08:52:57","subject":"Re: [libcamera-devel] [PATCH v2 6/7] android: Plumb all AE-related\n\tcontrols","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Fri, Oct 01, 2021 at 07:32:48PM +0200, Jacopo Mondi wrote:\n> Hi Paul,\n> \n> On Fri, Oct 01, 2021 at 07:33:24PM +0900, Paul Elder wrote:\n> > With the AE-related controls reorganized in libcamera, with separate\n> > value and lever controls for exposure and gain, plumb these through the\n> > HAL layer (in order of appearence in the patch):\n> > - static metadata: available AE modes, AE lock available\n> > - manual template: add AE off (the other templates already have AE on)\n> > - request metadata: HAL -> libcamera controls conversion\n> > - result metadata: libcamera -> HAL controls conversion\n> > - result metadata: AE state\n> \n> So I think they should go in separate patches maybe\n\nHmmm... they might be easily separatable... they're all related though.\n\n> \n> >\n> > We add class variables to CameraDevice to save the last set android AE\n> > controls, as that is how android controls function (no need to resubmit\n> > controls if they are unchanged).\n> \n> Do we work any differently ? It's an honest question, I always assumed\n> we don't.\n\nThat is how we should work afaik, but we currently don't (and I'm pretty\nsure pipeline handlers don't act that way either...).\n\n> \n> >\n> > We also save libcamera's AE state in the request descriptor, as\n> > otherwise there is insufficient information from libcamera's result\n> > metadata alone to tell if the state is locked or manual (as they are an\n> > internal state in libcamera).\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 <paul.elder@ideasonboard.com>\n> >\n> > ---\n> > New in v2\n> > ---\n> >  src/android/camera_capabilities.cpp | 63 ++++++++++++++++---\n> >  src/android/camera_device.cpp       | 98 +++++++++++++++++++++++++++--\n> >  src/android/camera_device.h         | 16 +++++\n> >  3 files changed, 165 insertions(+), 12 deletions(-)\n> >\n> > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> > index f7a6cda9..3fed3f83 100644\n> > --- a/src/android/camera_capabilities.cpp\n> > +++ b/src/android/camera_capabilities.cpp\n> > @@ -830,12 +830,62 @@ int CameraCapabilities::initializeStaticMetadata()\n> >  \tstaticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_ANTIBANDING_MODES,\n> >  \t\t\t\t  aeAvailableAntiBandingModes);\n> >\n> > -\tstd::vector<uint8_t> aeAvailableModes = {\n> > -\t\tANDROID_CONTROL_AE_MODE_ON,\n> > -\t};\n> > +\tstd::vector<uint8_t> aeAvailableModes;\n> > +\t/* This will cause problems if one supports only on and the other only off */\n> > +\tbool aeAddedOn = false;\n> > +\tbool aeAddedOff = false;\n> > +\n> > +\tconst auto &analogGainModeInfo = controlsInfo.find(&controls::AnalogueGainMode);\n> > +\tif (analogGainModeInfo != controlsInfo.end()) {\n> > +\t\tfor (const auto &value : analogGainModeInfo->second.values()) {\n> > +\t\t\tswitch (value.get<int32_t>()) {\n> > +\t\t\tcase controls::AnalogueGainModeAuto:\n> > +\t\t\t\tif (!aeAddedOn)\n> \n> How can aeAddedOn be true if it's initialized to false and set to true\n> just here below, unless you expect the same value to be specified\n> twice in the info.values() ?\n\nOh it was for consistency. Same code same shape :D\n\n> \n> Should you just add the value unconditionally here and below ?\n\nYeah.\n\n> \n> > +\t\t\t\t\taeAvailableModes.push_back(ANDROID_CONTROL_AE_MODE_ON);\n> > +\t\t\t\taeAddedOn = true;\n> > +\t\t\t\tbreak;\n> > +\t\t\tcase controls::AnalogueGainModeDisabled:\n> > +\t\t\t\tif (!aeAddedOff)\n> > +\t\t\t\t\taeAvailableModes.push_back(ANDROID_CONTROL_AE_MODE_OFF);\n> > +\t\t\t\taeAddedOff = true;\n> > +\t\t\t\tbreak;\n> > +\t\t\tdefault:\n> > +\t\t\t\tbreak;\n> > +\t\t\t}\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tconst auto &exposureTimeModeInfo = controlsInfo.find(&controls::ExposureTimeMode);\n> > +\tif (exposureTimeModeInfo != controlsInfo.end()) {\n> > +\t\tfor (const auto &value : exposureTimeModeInfo->second.values()) {\n> > +\t\t\tswitch (value.get<int32_t>()) {\n> > +\t\t\tcase controls::ExposureTimeModeAuto:\n> > +\t\t\t\tif (!aeAddedOn)\n> > +\t\t\t\t\taeAvailableModes.push_back(ANDROID_CONTROL_AE_MODE_ON);\n> > +\t\t\t\taeAddedOn = true;\n> \n> And setting the aeAdded to true here has no purpose, but that's not an\n> issue.\n> \n> What is the policy we want to have here ? Android has a single AE\n> control, we have two. I don't think we can allow situation where we\n\nYeah that's exactly what I was wondering. What happens if the libcamera\ncamera only has one of the two (is that possible?)? What if one is\nmanual-able but the other isn't? Even worse, what if one is auto-only\nand the other is manual-only?\n\nSo I wasn't sure what to do. I decided that at least one should be set\nto auto to enable auto, and at least one should be manual-able to\nconsider AE manual-able, and at least one should be auto-able to\nconsider AE auto-able.\n\n> have an asymmetric ExposureTimeMode and AnalogueGainMode as matching\n> on which one to add would become painful.\n\nYeah it's painful. Does anybody know what we can mandate to simplify\nthis? Can we require that if a camera supports one manual-able it must\nalso support the other manualable? And the same for auto?\n\n> \n> I would cut it short and requires libcamera implementation that aims\n> to HAL full level to have both gain and exposure time controllable.\n> \n> The code can thus be restructured as\n> \n>         availableModes = { AE_MODE_ON };\n>         if (exposureTimeInfo != end && analogGainInfo != end) {\n>                 bool manualExp = false;\n>                 bool manualGain = false;\n> \n>                 for (modes : exposureTimeInfo.values())\n>                         if (mode == Disabled)\n>                                 manualExp = true;\n>                                 break;\n> \n>                for (modes : analogGainInfo.values())\n>                         if (mode == Disabled)\n>                                 manualGain = true;\n>                                 break\n> \n>                if (manualExp && manualGain)\n>                         availableModes.push_back(AE_MODE_OFF);\n>         }\n\nYeah it would be nice, but we need to know that pipelines+IPAs *will*\nfollow this behavior.\n\n> > +\t\t\t\tbreak;\n> > +\t\t\tcase controls::ExposureTimeModeDisabled:\n> > +\t\t\t\tif (!aeAddedOff)\n> > +\t\t\t\t\taeAvailableModes.push_back(ANDROID_CONTROL_AE_MODE_OFF);\n> > +\t\t\t\taeAddedOff = true;\n> \n> \n> > +\t\t\t\tbreak;\n> > +\t\t\tdefault:\n> > +\t\t\t\tbreak;\n> > +\t\t\t}\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tif (aeAvailableModes.empty())\n> > +\t\taeAvailableModes.push_back(ANDROID_CONTROL_AE_MODE_ON);\n> >  \tstaticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_MODES,\n> >  \t\t\t\t  aeAvailableModes);\n> >\n> > +\t/* In libcamera, turning off AE is equivalient to locking. */\n> > +\tuint8_t aeLockAvailable = aeAddedOff ? ANDROID_CONTROL_AE_LOCK_AVAILABLE_TRUE\n> > +\t\t\t\t\t     : ANDROID_CONTROL_AE_LOCK_AVAILABLE_FALSE;\n> \n> This really depends on what we want to do with AeState.\n> I think if we want to report locking from there we should make sure\n\nI don't want to report locking from AeState. Locking is already reported\nfrom mode, though it's merged with manual.\n\n> that among the supported AeState values there is locked. Bummer, we\n> don't have the Camera::controls() equivalent for metadata, hence we\n> don't have a way to report the values supported for metadata-only\n> controls as AeState is\n> \n> > +\tstaticMetadata_->addEntry(ANDROID_CONTROL_AE_LOCK_AVAILABLE,\n> > +\t\t\t\t  aeLockAvailable);\n> > +\n> >  \tint64_t minFrameDurationNsec = -1;\n> >  \tint64_t maxFrameDurationNsec = -1;\n> >  \tconst auto frameDurationsInfo = controlsInfo.find(&controls::FrameDurationLimits);\n> > @@ -946,10 +996,6 @@ int CameraCapabilities::initializeStaticMetadata()\n> >  \tstaticMetadata_->addEntry(ANDROID_CONTROL_SCENE_MODE_OVERRIDES,\n> >  \t\t\t\t  sceneModesOverride);\n> >\n> > -\tuint8_t aeLockAvailable = ANDROID_CONTROL_AE_LOCK_AVAILABLE_FALSE;\n> > -\tstaticMetadata_->addEntry(ANDROID_CONTROL_AE_LOCK_AVAILABLE,\n> > -\t\t\t\t  aeLockAvailable);\n> > -\n> >  \tuint8_t awbLockAvailable = ANDROID_CONTROL_AWB_LOCK_AVAILABLE_FALSE;\n> >  \tstaticMetadata_->addEntry(ANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n> >  \t\t\t\t  awbLockAvailable);\n> > @@ -1358,6 +1404,9 @@ std::unique_ptr<CameraMetadata> CameraCapabilities::requestTemplateManual() cons\n> >  \tif (!manualTemplate)\n> >  \t\treturn nullptr;\n> >\n> > +\tuint8_t aeMode = ANDROID_CONTROL_AE_MODE_OFF;\n> > +\tmanualTemplate->addEntry(ANDROID_CONTROL_AE_MODE, aeMode);\n> \n> Only if supported, or unconditionally ?\n\nThere's already a check for this. The manual sensor capability requires\nAE off, and requestTemplateManual() checks that the manual sensor\ncapability is available.\n\n> \n> > +\n> >  \treturn manualTemplate;\n> >  }\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index ef4fbab8..d5027ec5 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -263,7 +263,9 @@ CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(\n> >\n> >  CameraDevice::CameraDevice(unsigned int id, std::shared_ptr<Camera> camera)\n> >  \t: id_(id), state_(State::Stopped), camera_(std::move(camera)),\n> > -\t  facing_(CAMERA_FACING_FRONT), orientation_(0)\n> > +\t  facing_(CAMERA_FACING_FRONT), orientation_(0),\n> > +\t  aeOn_(true), aeLocked_(false), lastExposureTime_(0),\n> > +\t  lastAnalogueGain_(1.0f), lastDigitalGain_(1.0f)\n> >  {\n> >  \tcamera_->requestCompleted.connect(this, &CameraDevice::requestComplete);\n> >\n> > @@ -857,6 +859,62 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)\n> >  \t\tcontrols.set(controls::draft::TestPatternMode, testPatternMode);\n> >  \t}\n> >\n> > +\t/*\n> > +\t * \\todo Can we trust that we won't receive values that we didn't\n> > +\t * report supporting?\n> > +\t */\n> > +\tif (settings.getEntry(ANDROID_CONTROL_AE_LOCK, &entry))\n> > +\t\taeLocked_ = *entry.data.u8 != ANDROID_CONTROL_AE_LOCK_OFF;\n> > +\n> > +\tif (settings.getEntry(ANDROID_CONTROL_AE_MODE, &entry))\n> > +\t\taeOn_ = *entry.data.u8 != ANDROID_CONTROL_AE_MODE_OFF;\n> > +\n> > +\tAeMode aeMode;\n> > +\tif (aeLocked_ && aeOn_)\n> > +\t\taeMode = AeMode::Lock;\n> > +\telse if (aeLocked_ && !aeOn_)\n> > +\t\taeMode = AeMode::Manual;\n> > +\telse if (!aeLocked_ && aeOn_)\n> > +\t\taeMode = AeMode::Auto;\n> > +\telse /* !aeLocked_ && !aeOn_ */\n> > +\t\taeMode = AeMode::Manual;\n> \n>         if (!aeOn_)\n>                 aeMode = Manual;\n>         else if (aeLocked)\n>                 aeMode = Lock;\n>         else\n>                 aeMode = Auto\n\nOh yeah that's simpler.\n\n> > +\n> > +\t/* Save this so that we can recover it in the result */\n> > +\tdescriptor->aeMode_ = aeMode;\n> > +\n> > +\tconst auto &eInfo = camera_->controls().find(&controls::ExposureTimeMode);\n> \n> Doing so at every request is not nasty, but if we could avoid it it\n> would nice. Don't we have a list of capabilities to rely on ?\n\nWe do, but again, does supporting manual sensor capability (= supporting\nAE off) guarantee that the camera has both ExposureTime/Mode and\nAnalogueGain/Mode?\n\n> \n> > +\tif (eInfo != camera_->controls().end()) {\n> > +\t\tcontrols.set(controls::ExposureTimeMode,\n> > +\t\t\t\taeMode == AeMode::Auto ?\n> > +\t\t\t\tcontrols::ExposureTimeModeAuto :\n> > +\t\t\t\tcontrols::ExposureTimeModeDisabled);\n> \n> I'll ask again, do we expect the same controls to be in every request\n> even if they do not change ?\n\nIf the control value does not change, it doesn't need to be set again in\na new request, at least with the current status quo. In android it's\ndefinitely a requirement. In libcamera, a lot (all?) of things don't\nbehave like this yet, though. This patch makes all the AE-related\ncontrols behave like this, and it's especially required since it's the\nHAL layer. That's why there's the ae state variable in the request\ndescriptor, and why there are new class variables.\n\n> \n> > +\t}\n> > +\n> > +\tconst auto &gInfo = camera_->controls().find(&controls::AnalogueGainMode);\n> > +\tif (gInfo != camera_->controls().end()) {\n> > +\t\tcontrols.set(controls::AnalogueGainMode,\n> > +\t\t\t\taeMode == AeMode::Auto ?\n> > +\t\t\t\tcontrols::AnalogueGainModeAuto :\n> > +\t\t\t\tcontrols::AnalogueGainModeDisabled);\n> > +\t}\n> > +\n> > +\tif (settings.getEntry(ANDROID_SENSOR_EXPOSURE_TIME, &entry)) {\n> > +\t\tconst auto &info = camera_->controls().find(&controls::ExposureTime);\n> \n> Should we make sure at capabilities contruction time that if\n> ExposureTimeMode supports Disabled, then ExposureTime is present ?\n> If we assume so, and make it a requisite to claim we support\n> AE_MODE_OFF then these controls should only be handled if\n> capabilities.contain(MANUAL_SENSOR)\n\nYeah I think we should. Actually I think it should be mandated on the\nlibcamera side. (Do we have such mechanism yet?)\n\n> \n> > +\t\tif (info != camera_->controls().end()) {\n> > +\t\t\tlastExposureTime_ = (*entry.data.i64) / 1000;\n> > +\t\t\t/* Don't disable libcamera's internal AeMode::Lock */\n> > +\t\t\tif (aeMode != AeMode::Lock)\n> \n> Do we expect a request to contain both AE_LOCK and a sensor exposure\n> time ?\n\nNo. But don't we have to prepare for (or at least not break) if the user\ndecides to do so anyway?\n\n> \n> > +\t\t\t\tcontrols.set(controls::ExposureTime, lastExposureTime_);\n> > +\t\t}\n> > +\t}\n> > +\n> > +\t/* Trigger libcamera's locked -> manual state change */\n> > +\tif (aeMode == AeMode::Manual && !settings.hasEntry(ANDROID_SENSOR_EXPOSURE_TIME)) {\n> > +\t\tconst auto &info = camera_->controls().find(&controls::ExposureTime);\n> > +\t\tif (info != camera_->controls().end())\n> > +\t\t\tcontrols.set(controls::ExposureTime, lastExposureTime_);\n> \n> If we don't need to repeat the control values in every request, is\n> this necessary ? Or are you afraid we receive a AE_MODE_OFF and no\n\nIt's necessary to translate android's lock control to libcamera's new\nmanual-locked hidden state. As I commented:\n/* Trigger libcamera's locked -> manual state change */\n\nBecause the Mode controls' internal state causes and exception to the\n\"don't need to repeat the control values every requrest\" rule. It's even\ndocumented in the control :D\n\n> SENSOR_EXPOSURE_TIME ?\n> \n> > +\t}\n> > +\n> >  \treturn 0;\n> >  }\n> >\n> > @@ -1345,11 +1403,16 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons\n> >  \tresultMetadata->addEntry(ANDROID_CONTROL_AE_EXPOSURE_COMPENSATION,\n> >  \t\t\t\t value32);\n> >\n> > -\tvalue = ANDROID_CONTROL_AE_LOCK_OFF;\n> > -\tresultMetadata->addEntry(ANDROID_CONTROL_AE_LOCK, value);\n> > +\tuint8_t aeLock = (descriptor.aeMode_ == AeMode::Lock)\n> > +\t\t       ? ANDROID_CONTROL_AE_LOCK_ON\n> > +\t\t       : ANDROID_CONTROL_AE_LOCK_OFF;\n> > +\tresultMetadata->addEntry(ANDROID_CONTROL_AE_LOCK, aeLock);\n> >\n> > -\tvalue = ANDROID_CONTROL_AE_MODE_ON;\n> > -\tresultMetadata->addEntry(ANDROID_CONTROL_AE_MODE, value);\n> > +\t/* Locked means auto + locked in android */\n> > +\tuint8_t aeMode = (descriptor.aeMode_ != AeMode::Manual)\n> > +\t\t       ? ANDROID_CONTROL_AE_MODE_ON\n> > +\t\t       : ANDROID_CONTROL_AE_MODE_OFF;\n> > +\tresultMetadata->addEntry(ANDROID_CONTROL_AE_MODE, aeMode);\n> >\n> >  \tif (settings.getEntry(ANDROID_CONTROL_AE_TARGET_FPS_RANGE, &entry))\n> >  \t\t/*\n> > @@ -1366,6 +1429,31 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons\n> >  \tresultMetadata->addEntry(ANDROID_CONTROL_AE_PRECAPTURE_TRIGGER, value);\n> >\n> >  \tvalue = ANDROID_CONTROL_AE_STATE_CONVERGED;\n> > +\tif (metadata.contains(controls::AeState)) {\n> > +\t\tswitch (metadata.get(controls::AeState)) {\n> > +\t\tcase controls::AeStateInactive:\n> > +\t\t\tvalue = ANDROID_CONTROL_AE_STATE_INACTIVE;\n> > +\t\t\tbreak;\n> > +\t\tcase controls::AeStateSearching:\n> > +\t\t\tvalue = ANDROID_CONTROL_AE_STATE_SEARCHING;\n> > +\t\t\tbreak;\n> > +\t\tcase controls::AeStateConverged:\n> > +\t\t\tvalue = ANDROID_CONTROL_AE_STATE_CONVERGED;\n> > +\t\t\tbreak;\n> > +\t\tcase controls::AeStateFlashRequired:\n> > +\t\t\tvalue = ANDROID_CONTROL_AE_STATE_FLASH_REQUIRED;\n> > +\t\t\tbreak;\n> > +\t\tcase controls::AeStatePrecapture:\n> > +\t\t\tvalue = ANDROID_CONTROL_AE_STATE_PRECAPTURE;\n> > +\t\t\tbreak;\n> > +\t\tdefault:\n> > +\t\t\tif (descriptor.aeMode_ == AeMode::Lock) {\n> > +\t\t\t\tvalue = ANDROID_CONTROL_AE_STATE_LOCKED;\n> > +\t\t\t\tbreak;\n> > +\t\t\t}\n> > +\t\t\tLOG(HAL, Error) << \"Invalid AeState, setting converged\";\n> > +\t\t}\n> > +\t}\n> >  \tresultMetadata->addEntry(ANDROID_CONTROL_AE_STATE, value);\n> >\n> >  \tvalue = ANDROID_CONTROL_AF_MODE_OFF;\n> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > index b7d774fe..f693cdbc 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -73,6 +73,12 @@ private:\n> >\n> >  \tCameraDevice(unsigned int id, std::shared_ptr<libcamera::Camera> camera);\n> >\n> > +\tenum class AeMode {\n> > +\t\tAuto,\n> > +\t\tLock,\n> > +\t\tManual,\n> > +\t};\n> > +\n> >  \tstruct Camera3RequestDescriptor {\n> >  \t\tenum class Status {\n> >  \t\t\tPending,\n> > @@ -96,6 +102,9 @@ private:\n> >\n> >  \t\tcamera3_capture_result_t captureResult_ = {};\n> >  \t\tStatus status_ = Status::Pending;\n> > +\n> > +\t\t/* The libcamera internal AE state for this request */\n> > +\t\tAeMode aeMode_ = AeMode::Auto;\n> >  \t};\n> >\n> >  \tenum class State {\n> > @@ -146,6 +155,13 @@ private:\n> >  \tint facing_;\n> >  \tint orientation_;\n> >\n> > +\t/* Track the last-set android AE controls */\n> > +\tbool aeOn_;\n> > +\tbool aeLocked_;\n> \n> I guess this is again about if we have to repeat controls every\n> request, otherwise they don't seem to be required as class members..\n\nYeah, exactly. These two specifically are for the android controls so\nwe don't get to decide.\n\n\nThanks,\n\nPaul\n\n> \n> > +\tint32_t lastExposureTime_;\n> > +\tfloat lastAnalogueGain_;\n> > +\tfloat lastDigitalGain_;\n> > +\n> >  \tCameraMetadata lastSettings_;\n> >  };\n> >\n> > --\n> > 2.27.0\n> >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 8C6F1C3243\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon,  4 Oct 2021 08:53:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 178BA691B9;\n\tMon,  4 Oct 2021 10:53:07 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1EB11602DC\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon,  4 Oct 2021 10:53:05 +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 88B555A1;\n\tMon,  4 Oct 2021 10:53:03 +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=\"rBUdzogP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1633337584;\n\tbh=Jb/sErhQDocjrHeXniOmFIIvYBVMZXvD7SgSTHUhGJI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=rBUdzogPu4+02Oi16t4N7jJSGo58Mnp11uDUlF9CG7T9+b2LPE5cS2MXliPYxmrYa\n\tRH+iT4WD4T1PvRPvL1Q5ixU0QM5xwUM82QSV2isu4V+4Aggo3oNN7LB+0V6PVeEdbJ\n\tQLJCy6ExpyWP54HcbsMEX4Sb/TpS782J/FNlALxs=","Date":"Mon, 4 Oct 2021 17:52:57 +0900","From":"paul.elder@ideasonboard.com","To":"Jacopo Mondi <jacopo@jmondi.org>","Message-ID":"<20211004085257.GE4221@pyrite.rasen.tech>","References":"<20211001103325.1077590-1-paul.elder@ideasonboard.com>\n\t<20211001103325.1077590-7-paul.elder@ideasonboard.com>\n\t<20211001173248.athwfrz6nx4ztlnk@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20211001173248.athwfrz6nx4ztlnk@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 6/7] android: Plumb all AE-related\n\tcontrols","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":20110,"web_url":"https://patchwork.libcamera.org/comment/20110/","msgid":"<20211011121511.ltunszvzapekha2f@uno.localdomain>","date":"2021-10-11T12:15:11","subject":"Re: [libcamera-devel] [PATCH v2 6/7] android: Plumb all AE-related\n\tcontrols","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul,\n\nOn Mon, Oct 04, 2021 at 05:52:57PM +0900, paul.elder@ideasonboard.com wrote:\n> Hi Jacopo,\n>\n> On Fri, Oct 01, 2021 at 07:32:48PM +0200, Jacopo Mondi wrote:\n> > Hi Paul,\n> >\n> > On Fri, Oct 01, 2021 at 07:33:24PM +0900, Paul Elder wrote:\n> > > With the AE-related controls reorganized in libcamera, with separate\n> > > value and lever controls for exposure and gain, plumb these through the\n> > > HAL layer (in order of appearence in the patch):\n> > > - static metadata: available AE modes, AE lock available\n> > > - manual template: add AE off (the other templates already have AE on)\n> > > - request metadata: HAL -> libcamera controls conversion\n> > > - result metadata: libcamera -> HAL controls conversion\n> > > - result metadata: AE state\n> >\n> > So I think they should go in separate patches maybe\n>\n> Hmmm... they might be easily separatable... they're all related though.\n>\n> >\n> > >\n> > > We add class variables to CameraDevice to save the last set android AE\n> > > controls, as that is how android controls function (no need to resubmit\n> > > controls if they are unchanged).\n> >\n> > Do we work any differently ? It's an honest question, I always assumed\n> > we don't.\n>\n> That is how we should work afaik, but we currently don't (and I'm pretty\n> sure pipeline handlers don't act that way either...).\n>\n> >\n> > >\n> > > We also save libcamera's AE state in the request descriptor, as\n> > > otherwise there is insufficient information from libcamera's result\n> > > metadata alone to tell if the state is locked or manual (as they are an\n> > > internal state in libcamera).\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 <paul.elder@ideasonboard.com>\n> > >\n> > > ---\n> > > New in v2\n> > > ---\n> > >  src/android/camera_capabilities.cpp | 63 ++++++++++++++++---\n> > >  src/android/camera_device.cpp       | 98 +++++++++++++++++++++++++++--\n> > >  src/android/camera_device.h         | 16 +++++\n> > >  3 files changed, 165 insertions(+), 12 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> > > index f7a6cda9..3fed3f83 100644\n> > > --- a/src/android/camera_capabilities.cpp\n> > > +++ b/src/android/camera_capabilities.cpp\n> > > @@ -830,12 +830,62 @@ int CameraCapabilities::initializeStaticMetadata()\n> > >  \tstaticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_ANTIBANDING_MODES,\n> > >  \t\t\t\t  aeAvailableAntiBandingModes);\n> > >\n> > > -\tstd::vector<uint8_t> aeAvailableModes = {\n> > > -\t\tANDROID_CONTROL_AE_MODE_ON,\n> > > -\t};\n> > > +\tstd::vector<uint8_t> aeAvailableModes;\n> > > +\t/* This will cause problems if one supports only on and the other only off */\n> > > +\tbool aeAddedOn = false;\n> > > +\tbool aeAddedOff = false;\n> > > +\n> > > +\tconst auto &analogGainModeInfo = controlsInfo.find(&controls::AnalogueGainMode);\n> > > +\tif (analogGainModeInfo != controlsInfo.end()) {\n> > > +\t\tfor (const auto &value : analogGainModeInfo->second.values()) {\n> > > +\t\t\tswitch (value.get<int32_t>()) {\n> > > +\t\t\tcase controls::AnalogueGainModeAuto:\n> > > +\t\t\t\tif (!aeAddedOn)\n> >\n> > How can aeAddedOn be true if it's initialized to false and set to true\n> > just here below, unless you expect the same value to be specified\n> > twice in the info.values() ?\n>\n> Oh it was for consistency. Same code same shape :D\n>\n> >\n> > Should you just add the value unconditionally here and below ?\n>\n> Yeah.\n>\n> >\n> > > +\t\t\t\t\taeAvailableModes.push_back(ANDROID_CONTROL_AE_MODE_ON);\n> > > +\t\t\t\taeAddedOn = true;\n> > > +\t\t\t\tbreak;\n> > > +\t\t\tcase controls::AnalogueGainModeDisabled:\n> > > +\t\t\t\tif (!aeAddedOff)\n> > > +\t\t\t\t\taeAvailableModes.push_back(ANDROID_CONTROL_AE_MODE_OFF);\n> > > +\t\t\t\taeAddedOff = true;\n> > > +\t\t\t\tbreak;\n> > > +\t\t\tdefault:\n> > > +\t\t\t\tbreak;\n> > > +\t\t\t}\n> > > +\t\t}\n> > > +\t}\n> > > +\n> > > +\tconst auto &exposureTimeModeInfo = controlsInfo.find(&controls::ExposureTimeMode);\n> > > +\tif (exposureTimeModeInfo != controlsInfo.end()) {\n> > > +\t\tfor (const auto &value : exposureTimeModeInfo->second.values()) {\n> > > +\t\t\tswitch (value.get<int32_t>()) {\n> > > +\t\t\tcase controls::ExposureTimeModeAuto:\n> > > +\t\t\t\tif (!aeAddedOn)\n> > > +\t\t\t\t\taeAvailableModes.push_back(ANDROID_CONTROL_AE_MODE_ON);\n> > > +\t\t\t\taeAddedOn = true;\n> >\n> > And setting the aeAdded to true here has no purpose, but that's not an\n> > issue.\n> >\n> > What is the policy we want to have here ? Android has a single AE\n> > control, we have two. I don't think we can allow situation where we\n>\n> Yeah that's exactly what I was wondering. What happens if the libcamera\n> camera only has one of the two (is that possible?)? What if one is\n> manual-able but the other isn't? Even worse, what if one is auto-only\n> and the other is manual-only?\n>\n> So I wasn't sure what to do. I decided that at least one should be set\n> to auto to enable auto, and at least one should be manual-able to\n> consider AE manual-able, and at least one should be auto-able to\n> consider AE auto-able.\n>\n> > have an asymmetric ExposureTimeMode and AnalogueGainMode as matching\n> > on which one to add would become painful.\n>\n> Yeah it's painful. Does anybody know what we can mandate to simplify\n> this? Can we require that if a camera supports one manual-able it must\n> also support the other manualable? And the same for auto?\n>\n> >\n> > I would cut it short and requires libcamera implementation that aims\n> > to HAL full level to have both gain and exposure time controllable.\n> >\n> > The code can thus be restructured as\n> >\n> >         availableModes = { AE_MODE_ON };\n> >         if (exposureTimeInfo != end && analogGainInfo != end) {\n> >                 bool manualExp = false;\n> >                 bool manualGain = false;\n> >\n> >                 for (modes : exposureTimeInfo.values())\n> >                         if (mode == Disabled)\n> >                                 manualExp = true;\n> >                                 break;\n> >\n> >                for (modes : analogGainInfo.values())\n> >                         if (mode == Disabled)\n> >                                 manualGain = true;\n> >                                 break\n> >\n> >                if (manualExp && manualGain)\n> >                         availableModes.push_back(AE_MODE_OFF);\n> >         }\n>\n> Yeah it would be nice, but we need to know that pipelines+IPAs *will*\n> follow this behavior.\n>\n\nThe ones that want to support fully controllable manual mode, and\nconsequentially wants to support Android's ManualSensor capabilities.\n\nI think we can assume to claim we support AE_MODE_OFF is fair to\nrequire to the ph to allow control of both exposure time and gain.\n\n> > > +\t\t\t\tbreak;\n> > > +\t\t\tcase controls::ExposureTimeModeDisabled:\n> > > +\t\t\t\tif (!aeAddedOff)\n> > > +\t\t\t\t\taeAvailableModes.push_back(ANDROID_CONTROL_AE_MODE_OFF);\n> > > +\t\t\t\taeAddedOff = true;\n> >\n> >\n> > > +\t\t\t\tbreak;\n> > > +\t\t\tdefault:\n> > > +\t\t\t\tbreak;\n> > > +\t\t\t}\n> > > +\t\t}\n> > > +\t}\n> > > +\n> > > +\tif (aeAvailableModes.empty())\n> > > +\t\taeAvailableModes.push_back(ANDROID_CONTROL_AE_MODE_ON);\n> > >  \tstaticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_MODES,\n> > >  \t\t\t\t  aeAvailableModes);\n> > >\n> > > +\t/* In libcamera, turning off AE is equivalient to locking. */\n> > > +\tuint8_t aeLockAvailable = aeAddedOff ? ANDROID_CONTROL_AE_LOCK_AVAILABLE_TRUE\n> > > +\t\t\t\t\t     : ANDROID_CONTROL_AE_LOCK_AVAILABLE_FALSE;\n> >\n> > This really depends on what we want to do with AeState.\n> > I think if we want to report locking from there we should make sure\n>\n> I don't want to report locking from AeState. Locking is already reported\n> from mode, though it's merged with manual.\n>\n> > that among the supported AeState values there is locked. Bummer, we\n> > don't have the Camera::controls() equivalent for metadata, hence we\n> > don't have a way to report the values supported for metadata-only\n> > controls as AeState is\n> >\n> > > +\tstaticMetadata_->addEntry(ANDROID_CONTROL_AE_LOCK_AVAILABLE,\n> > > +\t\t\t\t  aeLockAvailable);\n> > > +\n> > >  \tint64_t minFrameDurationNsec = -1;\n> > >  \tint64_t maxFrameDurationNsec = -1;\n> > >  \tconst auto frameDurationsInfo = controlsInfo.find(&controls::FrameDurationLimits);\n> > > @@ -946,10 +996,6 @@ int CameraCapabilities::initializeStaticMetadata()\n> > >  \tstaticMetadata_->addEntry(ANDROID_CONTROL_SCENE_MODE_OVERRIDES,\n> > >  \t\t\t\t  sceneModesOverride);\n> > >\n> > > -\tuint8_t aeLockAvailable = ANDROID_CONTROL_AE_LOCK_AVAILABLE_FALSE;\n> > > -\tstaticMetadata_->addEntry(ANDROID_CONTROL_AE_LOCK_AVAILABLE,\n> > > -\t\t\t\t  aeLockAvailable);\n> > > -\n> > >  \tuint8_t awbLockAvailable = ANDROID_CONTROL_AWB_LOCK_AVAILABLE_FALSE;\n> > >  \tstaticMetadata_->addEntry(ANDROID_CONTROL_AWB_LOCK_AVAILABLE,\n> > >  \t\t\t\t  awbLockAvailable);\n> > > @@ -1358,6 +1404,9 @@ std::unique_ptr<CameraMetadata> CameraCapabilities::requestTemplateManual() cons\n> > >  \tif (!manualTemplate)\n> > >  \t\treturn nullptr;\n> > >\n> > > +\tuint8_t aeMode = ANDROID_CONTROL_AE_MODE_OFF;\n> > > +\tmanualTemplate->addEntry(ANDROID_CONTROL_AE_MODE, aeMode);\n> >\n> > Only if supported, or unconditionally ?\n>\n> There's already a check for this. The manual sensor capability requires\n> AE off, and requestTemplateManual() checks that the manual sensor\n> capability is available.\n>\n> >\n> > > +\n> > >  \treturn manualTemplate;\n> > >  }\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index ef4fbab8..d5027ec5 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -263,7 +263,9 @@ CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(\n> > >\n> > >  CameraDevice::CameraDevice(unsigned int id, std::shared_ptr<Camera> camera)\n> > >  \t: id_(id), state_(State::Stopped), camera_(std::move(camera)),\n> > > -\t  facing_(CAMERA_FACING_FRONT), orientation_(0)\n> > > +\t  facing_(CAMERA_FACING_FRONT), orientation_(0),\n> > > +\t  aeOn_(true), aeLocked_(false), lastExposureTime_(0),\n> > > +\t  lastAnalogueGain_(1.0f), lastDigitalGain_(1.0f)\n> > >  {\n> > >  \tcamera_->requestCompleted.connect(this, &CameraDevice::requestComplete);\n> > >\n> > > @@ -857,6 +859,62 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)\n> > >  \t\tcontrols.set(controls::draft::TestPatternMode, testPatternMode);\n> > >  \t}\n> > >\n> > > +\t/*\n> > > +\t * \\todo Can we trust that we won't receive values that we didn't\n> > > +\t * report supporting?\n> > > +\t */\n> > > +\tif (settings.getEntry(ANDROID_CONTROL_AE_LOCK, &entry))\n> > > +\t\taeLocked_ = *entry.data.u8 != ANDROID_CONTROL_AE_LOCK_OFF;\n> > > +\n> > > +\tif (settings.getEntry(ANDROID_CONTROL_AE_MODE, &entry))\n> > > +\t\taeOn_ = *entry.data.u8 != ANDROID_CONTROL_AE_MODE_OFF;\n> > > +\n> > > +\tAeMode aeMode;\n> > > +\tif (aeLocked_ && aeOn_)\n> > > +\t\taeMode = AeMode::Lock;\n> > > +\telse if (aeLocked_ && !aeOn_)\n> > > +\t\taeMode = AeMode::Manual;\n> > > +\telse if (!aeLocked_ && aeOn_)\n> > > +\t\taeMode = AeMode::Auto;\n> > > +\telse /* !aeLocked_ && !aeOn_ */\n> > > +\t\taeMode = AeMode::Manual;\n> >\n> >         if (!aeOn_)\n> >                 aeMode = Manual;\n> >         else if (aeLocked)\n> >                 aeMode = Lock;\n> >         else\n> >                 aeMode = Auto\n>\n> Oh yeah that's simpler.\n>\n> > > +\n> > > +\t/* Save this so that we can recover it in the result */\n> > > +\tdescriptor->aeMode_ = aeMode;\n> > > +\n> > > +\tconst auto &eInfo = camera_->controls().find(&controls::ExposureTimeMode);\n> >\n> > Doing so at every request is not nasty, but if we could avoid it it\n> > would nice. Don't we have a list of capabilities to rely on ?\n>\n> We do, but again, does supporting manual sensor capability (= supporting\n> AE off) guarantee that the camera has both ExposureTime/Mode and\n> AnalogueGain/Mode?\n>\n\nI think we should require it and simplify the whole procedure here\nby assuming that if we list ManualSensor in the capabilities, the\ncamera supports both ExposureTimeMode = Disabled and AnalogueGainMode\n= Disabled.\n\n\n> >\n> > > +\tif (eInfo != camera_->controls().end()) {\n> > > +\t\tcontrols.set(controls::ExposureTimeMode,\n> > > +\t\t\t\taeMode == AeMode::Auto ?\n> > > +\t\t\t\tcontrols::ExposureTimeModeAuto :\n> > > +\t\t\t\tcontrols::ExposureTimeModeDisabled);\n> >\n> > I'll ask again, do we expect the same controls to be in every request\n> > even if they do not change ?\n>\n> If the control value does not change, it doesn't need to be set again in\n> a new request, at least with the current status quo. In android it's\n> definitely a requirement. In libcamera, a lot (all?) of things don't\n> behave like this yet, though. This patch makes all the AE-related\n\nWhich controls assumes to be set at every request ?\n\n> controls behave like this, and it's especially required since it's the\n> HAL layer. That's why there's the ae state variable in the request\n> descriptor, and why there are new class variables.\n>\n> >\n> > > +\t}\n> > > +\n> > > +\tconst auto &gInfo = camera_->controls().find(&controls::AnalogueGainMode);\n> > > +\tif (gInfo != camera_->controls().end()) {\n> > > +\t\tcontrols.set(controls::AnalogueGainMode,\n> > > +\t\t\t\taeMode == AeMode::Auto ?\n> > > +\t\t\t\tcontrols::AnalogueGainModeAuto :\n> > > +\t\t\t\tcontrols::AnalogueGainModeDisabled);\n> > > +\t}\n> > > +\n> > > +\tif (settings.getEntry(ANDROID_SENSOR_EXPOSURE_TIME, &entry)) {\n> > > +\t\tconst auto &info = camera_->controls().find(&controls::ExposureTime);\n> >\n> > Should we make sure at capabilities contruction time that if\n> > ExposureTimeMode supports Disabled, then ExposureTime is present ?\n> > If we assume so, and make it a requisite to claim we support\n> > AE_MODE_OFF then these controls should only be handled if\n> > capabilities.contain(MANUAL_SENSOR)\n>\n> Yeah I think we should. Actually I think it should be mandated on the\n> libcamera side. (Do we have such mechanism yet?)\n>\n\nNot for libcamera, lc-compliance might help, but yes, this should be enforced.\n\n> >\n> > > +\t\tif (info != camera_->controls().end()) {\n> > > +\t\t\tlastExposureTime_ = (*entry.data.i64) / 1000;\n> > > +\t\t\t/* Don't disable libcamera's internal AeMode::Lock */\n> > > +\t\t\tif (aeMode != AeMode::Lock)\n> >\n> > Do we expect a request to contain both AE_LOCK and a sensor exposure\n> > time ?\n>\n> No. But don't we have to prepare for (or at least not break) if the user\n> decides to do so anyway?\n>\n\nI think it would be very hard to protect against all the possible\nmis-use of controls an application can submit to libcamera. Android\ndoesn't clearly define dependencies between control not precendences,\nbut I think it's fair to assume the combination is not valid and we\ncan decide to act as we please (ignore one or ignore both ?) as far as\nCTS does not complain.\n\n> >\n> > > +\t\t\t\tcontrols.set(controls::ExposureTime, lastExposureTime_);\n> > > +\t\t}\n> > > +\t}\n> > > +\n> > > +\t/* Trigger libcamera's locked -> manual state change */\n> > > +\tif (aeMode == AeMode::Manual && !settings.hasEntry(ANDROID_SENSOR_EXPOSURE_TIME)) {\n> > > +\t\tconst auto &info = camera_->controls().find(&controls::ExposureTime);\n> > > +\t\tif (info != camera_->controls().end())\n> > > +\t\t\tcontrols.set(controls::ExposureTime, lastExposureTime_);\n> >\n> > If we don't need to repeat the control values in every request, is\n> > this necessary ? Or are you afraid we receive a AE_MODE_OFF and no\n>\n> It's necessary to translate android's lock control to libcamera's new\n> manual-locked hidden state. As I commented:\n> /* Trigger libcamera's locked -> manual state change */\n>\n> Because the Mode controls' internal state causes and exception to the\n> \"don't need to repeat the control values every requrest\" rule. It's even\n> documented in the control :D\n\nWhat I mean is why you need to submit a cached exposure time if the\nandroid request does not provide one. If it's not updated by the HAL\nclient (the app) we don't update it either. After all if the app\nswitches to manual mode but does not provide an exposure time, it's an\nill-formed request, right ? So I would rather keep the AEGC locked\ninstead of using a cached value.\n\nThanks\n  j\n\n>\n> > SENSOR_EXPOSURE_TIME ?\n> >\n> > > +\t}\n> > > +\n> > >  \treturn 0;\n> > >  }\n> > >\n> > > @@ -1345,11 +1403,16 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons\n> > >  \tresultMetadata->addEntry(ANDROID_CONTROL_AE_EXPOSURE_COMPENSATION,\n> > >  \t\t\t\t value32);\n> > >\n> > > -\tvalue = ANDROID_CONTROL_AE_LOCK_OFF;\n> > > -\tresultMetadata->addEntry(ANDROID_CONTROL_AE_LOCK, value);\n> > > +\tuint8_t aeLock = (descriptor.aeMode_ == AeMode::Lock)\n> > > +\t\t       ? ANDROID_CONTROL_AE_LOCK_ON\n> > > +\t\t       : ANDROID_CONTROL_AE_LOCK_OFF;\n> > > +\tresultMetadata->addEntry(ANDROID_CONTROL_AE_LOCK, aeLock);\n> > >\n> > > -\tvalue = ANDROID_CONTROL_AE_MODE_ON;\n> > > -\tresultMetadata->addEntry(ANDROID_CONTROL_AE_MODE, value);\n> > > +\t/* Locked means auto + locked in android */\n> > > +\tuint8_t aeMode = (descriptor.aeMode_ != AeMode::Manual)\n> > > +\t\t       ? ANDROID_CONTROL_AE_MODE_ON\n> > > +\t\t       : ANDROID_CONTROL_AE_MODE_OFF;\n> > > +\tresultMetadata->addEntry(ANDROID_CONTROL_AE_MODE, aeMode);\n> > >\n> > >  \tif (settings.getEntry(ANDROID_CONTROL_AE_TARGET_FPS_RANGE, &entry))\n> > >  \t\t/*\n> > > @@ -1366,6 +1429,31 @@ CameraDevice::getResultMetadata(const Camera3RequestDescriptor &descriptor) cons\n> > >  \tresultMetadata->addEntry(ANDROID_CONTROL_AE_PRECAPTURE_TRIGGER, value);\n> > >\n> > >  \tvalue = ANDROID_CONTROL_AE_STATE_CONVERGED;\n> > > +\tif (metadata.contains(controls::AeState)) {\n> > > +\t\tswitch (metadata.get(controls::AeState)) {\n> > > +\t\tcase controls::AeStateInactive:\n> > > +\t\t\tvalue = ANDROID_CONTROL_AE_STATE_INACTIVE;\n> > > +\t\t\tbreak;\n> > > +\t\tcase controls::AeStateSearching:\n> > > +\t\t\tvalue = ANDROID_CONTROL_AE_STATE_SEARCHING;\n> > > +\t\t\tbreak;\n> > > +\t\tcase controls::AeStateConverged:\n> > > +\t\t\tvalue = ANDROID_CONTROL_AE_STATE_CONVERGED;\n> > > +\t\t\tbreak;\n> > > +\t\tcase controls::AeStateFlashRequired:\n> > > +\t\t\tvalue = ANDROID_CONTROL_AE_STATE_FLASH_REQUIRED;\n> > > +\t\t\tbreak;\n> > > +\t\tcase controls::AeStatePrecapture:\n> > > +\t\t\tvalue = ANDROID_CONTROL_AE_STATE_PRECAPTURE;\n> > > +\t\t\tbreak;\n> > > +\t\tdefault:\n> > > +\t\t\tif (descriptor.aeMode_ == AeMode::Lock) {\n> > > +\t\t\t\tvalue = ANDROID_CONTROL_AE_STATE_LOCKED;\n> > > +\t\t\t\tbreak;\n> > > +\t\t\t}\n> > > +\t\t\tLOG(HAL, Error) << \"Invalid AeState, setting converged\";\n> > > +\t\t}\n> > > +\t}\n> > >  \tresultMetadata->addEntry(ANDROID_CONTROL_AE_STATE, value);\n> > >\n> > >  \tvalue = ANDROID_CONTROL_AF_MODE_OFF;\n> > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > > index b7d774fe..f693cdbc 100644\n> > > --- a/src/android/camera_device.h\n> > > +++ b/src/android/camera_device.h\n> > > @@ -73,6 +73,12 @@ private:\n> > >\n> > >  \tCameraDevice(unsigned int id, std::shared_ptr<libcamera::Camera> camera);\n> > >\n> > > +\tenum class AeMode {\n> > > +\t\tAuto,\n> > > +\t\tLock,\n> > > +\t\tManual,\n> > > +\t};\n> > > +\n> > >  \tstruct Camera3RequestDescriptor {\n> > >  \t\tenum class Status {\n> > >  \t\t\tPending,\n> > > @@ -96,6 +102,9 @@ private:\n> > >\n> > >  \t\tcamera3_capture_result_t captureResult_ = {};\n> > >  \t\tStatus status_ = Status::Pending;\n> > > +\n> > > +\t\t/* The libcamera internal AE state for this request */\n> > > +\t\tAeMode aeMode_ = AeMode::Auto;\n> > >  \t};\n> > >\n> > >  \tenum class State {\n> > > @@ -146,6 +155,13 @@ private:\n> > >  \tint facing_;\n> > >  \tint orientation_;\n> > >\n> > > +\t/* Track the last-set android AE controls */\n> > > +\tbool aeOn_;\n> > > +\tbool aeLocked_;\n> >\n> > I guess this is again about if we have to repeat controls every\n> > request, otherwise they don't seem to be required as class members..\n>\n> Yeah, exactly. These two specifically are for the android controls so\n> we don't get to decide.\n>\n>\n> Thanks,\n>\n> Paul\n>\n> >\n> > > +\tint32_t lastExposureTime_;\n> > > +\tfloat lastAnalogueGain_;\n> > > +\tfloat lastDigitalGain_;\n> > > +\n> > >  \tCameraMetadata lastSettings_;\n> > >  };\n> > >\n> > > --\n> > > 2.27.0\n> > >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 84740C323E\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 11 Oct 2021 12:14:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0CFBE68F4C;\n\tMon, 11 Oct 2021 14:14:25 +0200 (CEST)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E57066012B\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 11 Oct 2021 14:14:22 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 526C71C0007;\n\tMon, 11 Oct 2021 12:14:22 +0000 (UTC)"],"Date":"Mon, 11 Oct 2021 14:15:11 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"paul.elder@ideasonboard.com","Message-ID":"<20211011121511.ltunszvzapekha2f@uno.localdomain>","References":"<20211001103325.1077590-1-paul.elder@ideasonboard.com>\n\t<20211001103325.1077590-7-paul.elder@ideasonboard.com>\n\t<20211001173248.athwfrz6nx4ztlnk@uno.localdomain>\n\t<20211004085257.GE4221@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211004085257.GE4221@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH v2 6/7] android: Plumb all AE-related\n\tcontrols","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]