[{"id":19915,"web_url":"https://patchwork.libcamera.org/comment/19915/","msgid":"","date":"2021-09-28T09:04:20","subject":"Re: [libcamera-devel] [RFC PATCH 2/4] libcamera: pipeline:\n\traspberrypi: Support the new AE 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 this patch. I think it looks good mostly, there might just\nbe a couple of small tweaks that I would suggest.\n\nOn Tue, 28 Sept 2021 at 08:50, Paul Elder wrote:\n>\n> Add support for the new AE controls in the raspberrypi pipeline handler,\n> and in the IPA.\n>\n> Signed-off-by: Paul Elder \n>\n> ---\n> This is very hacky. I wasn't sure what the best way was to plumb it into\n> the raspberrypi IPA as it was a bit hairy...\n> ---\n> include/libcamera/ipa/raspberrypi.h | 3 +-\n> src/ipa/raspberrypi/controller/rpi/agc.cpp | 18 ++++++++-\n> src/ipa/raspberrypi/controller/rpi/agc.hpp | 5 +++\n> src/ipa/raspberrypi/raspberrypi.cpp | 42 ++++++++++++++++----\n> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 17 ++++----\n> 5 files changed, 67 insertions(+), 18 deletions(-)\n>\n> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> index 521eaecd..363ea038 100644\n> --- a/include/libcamera/ipa/raspberrypi.h\n> +++ b/include/libcamera/ipa/raspberrypi.h\n> @@ -28,8 +28,9 @@ namespace RPi {\n> * unsupported control is encountered.\n> */\n> static const ControlInfoMap Controls({\n> - { &controls::AeEnable, ControlInfo(false, true) },\n\nI guess no global control just yet... :)\n\n> + { &controls::ExposureTimeMode, ControlInfo(controls::ExposureTimeModeValues) },\n> { &controls::ExposureTime, ControlInfo(0, 999999) },\n> + { &controls::AnalogueGainMode, ControlInfo(controls::AnalogueGainModeValues) },\n> { &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n> { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n> { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> index 289c1fce..b45ea454 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> @@ -203,14 +203,30 @@ bool Agc::IsPaused() const\n> }\n>\n> void Agc::Pause()\n> +{\n> +}\n> +\n> +void Agc::Resume()\n> +{\n> +}\n> +\n> +void Agc::PauseExposure()\n> {\n> fixed_shutter_ = status_.shutter_time;\n> +}\n> +\n> +void Agc::PauseGain()\n> +{\n> fixed_analogue_gain_ = status_.analogue_gain;\n> }\n>\n> -void Agc::Resume()\n> +void Agc::ResumeExposure()\n> {\n> fixed_shutter_ = 0s;\n> +}\n> +\n> +void Agc::ResumeGain()\n> +{\n> fixed_analogue_gain_ = 0;\n> }\n\nClearly we need these new Pause/Resume Exposure/Gain methods, but of\ncourse we're still left with the old ones (just Pause/Resume) as our\nAlgorithm interface requires them. Maybe they should still do what\nthey used to do? (perhaps where each calls the two new functions) This\nmight support a global enable/disable at some point, should we ever\nhave one!\n\n>\n> diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> index 82063636..7ca3ca2f 100644\n> --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> @@ -92,6 +92,11 @@ public:\n> void Prepare(Metadata *image_metadata) override;\n> void Process(StatisticsPtr &stats, Metadata *image_metadata) override;\n>\n> + void PauseExposure();\n> + void PauseGain();\n> + void ResumeExposure();\n> + void ResumeGain();\n> +\n\nI think I'd like to see these go as pure virtual functions into\nagc_algorithm.hpp, and then we override them here. The idea is that\nour controller will work with any AGC algorithm that implements the\ninterface in agc_algorithm.hpp (so Foo Corporation could replace ours\nwith their own) and I think this would break that.\n\n> private:\n> void updateLockStatus(DeviceStatus const &device_status);\n> AgcConfig config_;\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 047123ab..99935515 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -53,6 +53,8 @@\n> #include \"sharpen_algorithm.hpp\"\n> #include \"sharpen_status.h\"\n>\n> +#include \"controller/rpi/agc.hpp\"\n> +\n> namespace libcamera {\n>\n> using namespace std::literals::chrono_literals;\n> @@ -478,7 +480,10 @@ void IPARPi::reportMetadata()\n>\n> AgcStatus *agcStatus = rpiMetadata_.GetLocked(\"agc.status\");\n> if (agcStatus) {\n> - libcameraMetadata_.set(controls::AeLocked, agcStatus->locked);\n\nI really want to replace \"locked\" by \"converged\" here, but that's a\npatch for another day!\n\n> + libcameraMetadata_.set(controls::AeState,\n> + agcStatus->locked ?\n> + controls::AeStateConverged :\n> + controls::AeStateSearching);\n> libcameraMetadata_.set(controls::DigitalGain, agcStatus->digital_gain);\n> }\n>\n> @@ -623,20 +628,22 @@ void IPARPi::queueRequest(const ControlList &controls)\n> << \" = \" << ctrl.second.toString();\n>\n> switch (ctrl.first) {\n> - case controls::AE_ENABLE: {\n> - RPiController::Algorithm *agc = controller_.GetAlgorithm(\"agc\");\n> + case controls::EXPOSURE_TIME_MODE: {\n> + RPiController::Algorithm *algo = controller_.GetAlgorithm(\"agc\");\n> + RPiController::Agc *agc = reinterpret_cast(algo);\n\nAh yes, here it is. We should probably cast to an\nRPiController::AgcAlgorithm here. Once those new Pause/Resume\nfunctions have been moved to that class then a cast to AgcAlgorithm\nshould be fine.\n\n> if (!agc) {\n> LOG(IPARPI, Warning)\n> - << \"Could not set AE_ENABLE - no AGC algorithm\";\n> + << \"Could not set EXPOSURE_TIME_MODE - no AGC algorithm\";\n> break;\n> }\n>\n> - if (ctrl.second.get() == false)\n> - agc->Pause();\n> + if (ctrl.second.get() == controls::ExposureTimeModeDisabled)\n> + agc->PauseExposure();\n> else\n> - agc->Resume();\n> + agc->ResumeExposure();\n>\n> - libcameraMetadata_.set(controls::AeEnable, ctrl.second.get());\n> + libcameraMetadata_.set(controls::ExposureTimeMode,\n> + ctrl.second.get());\n> break;\n> }\n>\n> @@ -656,6 +663,25 @@ void IPARPi::queueRequest(const ControlList &controls)\n> break;\n> }\n>\n> + case controls::ANALOGUE_GAIN_MODE: {\n> + RPiController::Algorithm *algo = controller_.GetAlgorithm(\"agc\");\n> + RPiController::Agc *agc = reinterpret_cast(algo);\n\nSame thing here.\n\n> + if (!agc) {\n> + LOG(IPARPI, Warning)\n> + << \"Could not set ANALOGUE_GAIN_MODE - no AGC algorithm\";\n> + break;\n> + }\n> +\n> + if (ctrl.second.get() == controls::AnalogueGainModeDisabled)\n> + agc->PauseGain();\n> + else\n> + agc->ResumeGain();\n> +\n> + libcameraMetadata_.set(controls::AnalogueGainMode,\n> + ctrl.second.get());\n> + break;\n> + }\n> +\n> case controls::ANALOGUE_GAIN: {\n> RPiController::AgcAlgorithm *agc = dynamic_cast(\n> controller_.GetAlgorithm(\"agc\"));\n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index c227d885..3a9c3b8d 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -309,25 +309,25 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,\n> /* \\todo Make this nicer. */\n> int32_t ivalue;\n> if (id == controls::ExposureTimeMode && exposureSet) {\n> - int32_t exposureMode = controls->get(V4L2_CID_EXPOSURE_AUTO);\n> - ivalue = value.get() == ExposureTimeModeAuto\n> + int32_t exposureMode = controls->get(V4L2_CID_EXPOSURE_AUTO).get();\n> + ivalue = value.get() == controls::ExposureTimeModeAuto\n> ? (exposureMode == V4L2_EXPOSURE_SHUTTER_PRIORITY\n> ? V4L2_EXPOSURE_AUTO\n> : V4L2_EXPOSURE_APERTURE_PRIORITY)\n> : V4L2_EXPOSURE_MANUAL;\n> } else if (id == controls::ExposureTimeMode && !exposureSet) {\n> - ivalue = value.get() == ExposureTimeModeAuto\n> + ivalue = value.get() == controls::ExposureTimeModeAuto\n> ? V4L2_EXPOSURE_APERTURE_PRIORITY\n> : V4L2_EXPOSURE_MANUAL;\n> } else if (id == controls::AnalogueGainMode && exposureSet) {\n> - int32_t exposureMode = controls->get(V4L2_CID_EXPOSURE_AUTO);\n> - ivalue = value.get() == AnalogueGainModeAuto\n> + int32_t exposureMode = controls->get(V4L2_CID_EXPOSURE_AUTO).get();\n> + ivalue = value.get() == controls::AnalogueGainModeAuto\n> ? (exposureMode == V4L2_EXPOSURE_APERTURE_PRIORITY\n> ? V4L2_EXPOSURE_AUTO\n> : V4L2_EXPOSURE_SHUTTER_PRIORITY)\n> : V4L2_EXPOSURE_MANUAL;\n> } else if (id == controls::AnalogueGainMode && !exposureSet) {\n> - ivalue = value.get() == AnalogueGainModeAuto\n> + ivalue = value.get() == controls::AnalogueGainModeAuto\n> ? V4L2_EXPOSURE_SHUTTER_PRIORITY\n> : V4L2_EXPOSURE_MANUAL;\n> }\n> @@ -636,8 +636,9 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,\n> break;\n>\n> case V4L2_CID_EXPOSURE_AUTO:\n> - info = ControlInfo{ { ExposureTimeModeAuto, ExposureTimeModeDisabled },\n> - ExposureTimeModeDisabled };\n> + info = ControlInfo{ { controls::ExposureTimeModeAuto,\n> + controls::ExposureTimeModeDisabled },\n> + controls::ExposureTimeModeDisabled };\n> break;\n>\n> case V4L2_CID_EXPOSURE_ABSOLUTE:\n> --\n> 2.27.0\n>\n\n(Was this UVC stuff meant to be in the same patch?)\n\nApart from those small things this is looking pretty close to me!\n\nThanks\n\nDavid","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 7A0F5BDC71\n\tfor ;\n\tTue, 28 Sep 2021 09:04:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B00616918B;\n\tTue, 28 Sep 2021 11:04:38 +0200 (CEST)","from mail-wm1-x32b.google.com (mail-wm1-x32b.google.com\n\t[IPv6:2a00:1450:4864:20::32b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EB56C69185\n\tfor ;\n\tTue, 28 Sep 2021 11:04:31 +0200 (CEST)","by mail-wm1-x32b.google.com with SMTP id\n\tz184-20020a1c7ec1000000b003065f0bc631so1602169wmc.0\n\tfor ;\n\tTue, 28 Sep 2021 02:04:31 -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=\"W6MCY4ue\"; 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=peZ8lr3pNALKPRtl6avdQRR4wcxGooGFn8lht32YSqs=;\n\tb=W6MCY4ueNicxVgctRhjwRcDYbQG47/+JXTRTkxUj2wNLKMHVYfs44ROPlKk5FbwFGW\n\tz4RW7Hl8G+DSmV1RBtWRdEyiHd5I/KTPDyoXo75A2hncEsDFAIGp63JSY2fpumfbxmOY\n\tkWmxLlUu9ScOmddbFXzQIr6+h5VhVpK5UiLzB+8Jo5+eTFgR0EVfc0vimLluM3+LF/nk\n\trhPZPB6u2tQfdRQRSX2teK4XKQ3P6wMMy4WHYqnZseiPcW0ws5w/ou+5FteU/YBh2hJL\n\taUN/MZIdKpMSCbSsJsJqsmcWmF0KsKqyVlcQHOB0ermYkD7zz+sgNhEfGyhX+24l93ee\n\t7OwQ==","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=peZ8lr3pNALKPRtl6avdQRR4wcxGooGFn8lht32YSqs=;\n\tb=TLr27/VOh++AmcpJzz2/oLh/hGNyQv1g6OTo6QInMPYyX7KEA6VxO67ZQEUfZzrQoI\n\tr3bH96RHzraxJ7LOCMJbIBWoihujYeU4AdoostZ71lgP79ASo0lL1Qq0st408VZUkQIp\n\t+CYfKrfufqhsXM2wNQJyKZUgKD8lLxs9oQhfBrUrbT9MhhVpblbf6kvPGUMMDmIFUKW9\n\tOVtplSXzAD79XOQBb6W43ztKUFzzp67c67NdWjwL+SX+SyWsNh264z/whj0iLmk0IDOL\n\tj3itbC9szl4cwSd4SvNEK+JRsc9wIkwdXe2CQuZYCpaqo1c7O29vkSFv6SbmOf0FLTJs\n\t5tRA==","X-Gm-Message-State":"AOAM530mPqxl3pN9P6GuH461p6KS/UginOQboAca1WeJYLykVsPOVlda\n\tz6OpL6livMfZ0TzsYLgwdnlSNCxjg+ncAXeX9oEjZUsCu1JWzg==","X-Google-Smtp-Source":"ABdhPJxSAQ4yxSmri73eNpagjBSsqPEuBy3gnDcSiW98hRFalXiyOpU14UVKbsFWViuIiTKeO6Ls6BtMbM9VZ9JIsO0=","X-Received":"by 2002:a05:600c:350a:: with SMTP id\n\th10mr3415923wmq.163.1632819871402; \n\tTue, 28 Sep 2021 02:04:31 -0700 (PDT)","MIME-Version":"1.0","References":"<20210928074959.3489544-1-paul.elder@ideasonboard.com>\n\t<20210928074959.3489544-3-paul.elder@ideasonboard.com>","In-Reply-To":"<20210928074959.3489544-3-paul.elder@ideasonboard.com>","From":"David Plowman ","Date":"Tue, 28 Sep 2021 10:04:20 +0100","Message-ID":"","To":"Paul Elder ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [RFC PATCH 2/4] libcamera: pipeline:\n\traspberrypi: Support the new AE 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":19916,"web_url":"https://patchwork.libcamera.org/comment/19916/","msgid":"<20210928095121.GK4382@pyrite.rasen.tech>","date":"2021-09-28T09:51:21","subject":"Re: [libcamera-devel] [RFC PATCH 2/4] libcamera: pipeline:\n\traspberrypi: Support the new AE controls","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi David,\n\nOn Tue, Sep 28, 2021 at 10:04:20AM +0100, David Plowman wrote:\n> Hi Paul\n> \n> Thanks for this patch. I think it looks good mostly, there might just\n> be a couple of small tweaks that I would suggest.\n\nThank you for the review!\n\n> \n> On Tue, 28 Sept 2021 at 08:50, Paul Elder wrote:\n> >\n> > Add support for the new AE controls in the raspberrypi pipeline handler,\n> > and in the IPA.\n> >\n> > Signed-off-by: Paul Elder \n> >\n> > ---\n> > This is very hacky. I wasn't sure what the best way was to plumb it into\n> > the raspberrypi IPA as it was a bit hairy...\n> > ---\n> > include/libcamera/ipa/raspberrypi.h | 3 +-\n> > src/ipa/raspberrypi/controller/rpi/agc.cpp | 18 ++++++++-\n> > src/ipa/raspberrypi/controller/rpi/agc.hpp | 5 +++\n> > src/ipa/raspberrypi/raspberrypi.cpp | 42 ++++++++++++++++----\n> > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 17 ++++----\n> > 5 files changed, 67 insertions(+), 18 deletions(-)\n> >\n> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> > index 521eaecd..363ea038 100644\n> > --- a/include/libcamera/ipa/raspberrypi.h\n> > +++ b/include/libcamera/ipa/raspberrypi.h\n> > @@ -28,8 +28,9 @@ namespace RPi {\n> > * unsupported control is encountered.\n> > */\n> > static const ControlInfoMap Controls({\n> > - { &controls::AeEnable, ControlInfo(false, true) },\n> \n> I guess no global control just yet... :)\n\nThat's a \\todo :D\n\n> \n> > + { &controls::ExposureTimeMode, ControlInfo(controls::ExposureTimeModeValues) },\n> > { &controls::ExposureTime, ControlInfo(0, 999999) },\n> > + { &controls::AnalogueGainMode, ControlInfo(controls::AnalogueGainModeValues) },\n> > { &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n> > { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n> > { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.cpp b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > index 289c1fce..b45ea454 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.cpp\n> > @@ -203,14 +203,30 @@ bool Agc::IsPaused() const\n> > }\n> >\n> > void Agc::Pause()\n> > +{\n> > +}\n> > +\n> > +void Agc::Resume()\n> > +{\n> > +}\n> > +\n> > +void Agc::PauseExposure()\n> > {\n> > fixed_shutter_ = status_.shutter_time;\n> > +}\n> > +\n> > +void Agc::PauseGain()\n> > +{\n> > fixed_analogue_gain_ = status_.analogue_gain;\n> > }\n> >\n> > -void Agc::Resume()\n> > +void Agc::ResumeExposure()\n> > {\n> > fixed_shutter_ = 0s;\n> > +}\n> > +\n> > +void Agc::ResumeGain()\n> > +{\n> > fixed_analogue_gain_ = 0;\n> > }\n> \n> Clearly we need these new Pause/Resume Exposure/Gain methods, but of\n\nPhew, glad that was the right direction.\n\n> course we're still left with the old ones (just Pause/Resume) as our\n> Algorithm interface requires them. Maybe they should still do what\n> they used to do? (perhaps where each calls the two new functions) This\n\nAh, good idea.\n\n> might support a global enable/disable at some point, should we ever\n> have one!\n> \n> >\n> > diff --git a/src/ipa/raspberrypi/controller/rpi/agc.hpp b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > index 82063636..7ca3ca2f 100644\n> > --- a/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > +++ b/src/ipa/raspberrypi/controller/rpi/agc.hpp\n> > @@ -92,6 +92,11 @@ public:\n> > void Prepare(Metadata *image_metadata) override;\n> > void Process(StatisticsPtr &stats, Metadata *image_metadata) override;\n> >\n> > + void PauseExposure();\n> > + void PauseGain();\n> > + void ResumeExposure();\n> > + void ResumeGain();\n> > +\n> \n> I think I'd like to see these go as pure virtual functions into\n> agc_algorithm.hpp, and then we override them here. The idea is that\n> our controller will work with any AGC algorithm that implements the\n> interface in agc_algorithm.hpp (so Foo Corporation could replace ours\n> with their own) and I think this would break that.\n\nI see; I'll move them there then.\n\n> \n> > private:\n> > void updateLockStatus(DeviceStatus const &device_status);\n> > AgcConfig config_;\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 047123ab..99935515 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -53,6 +53,8 @@\n> > #include \"sharpen_algorithm.hpp\"\n> > #include \"sharpen_status.h\"\n> >\n> > +#include \"controller/rpi/agc.hpp\"\n> > +\n> > namespace libcamera {\n> >\n> > using namespace std::literals::chrono_literals;\n> > @@ -478,7 +480,10 @@ void IPARPi::reportMetadata()\n> >\n> > AgcStatus *agcStatus = rpiMetadata_.GetLocked(\"agc.status\");\n> > if (agcStatus) {\n> > - libcameraMetadata_.set(controls::AeLocked, agcStatus->locked);\n> \n> I really want to replace \"locked\" by \"converged\" here, but that's a\n> patch for another day!\n> \n> > + libcameraMetadata_.set(controls::AeState,\n> > + agcStatus->locked ?\n> > + controls::AeStateConverged :\n> > + controls::AeStateSearching);\n> > libcameraMetadata_.set(controls::DigitalGain, agcStatus->digital_gain);\n> > }\n> >\n> > @@ -623,20 +628,22 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > << \" = \" << ctrl.second.toString();\n> >\n> > switch (ctrl.first) {\n> > - case controls::AE_ENABLE: {\n> > - RPiController::Algorithm *agc = controller_.GetAlgorithm(\"agc\");\n> > + case controls::EXPOSURE_TIME_MODE: {\n> > + RPiController::Algorithm *algo = controller_.GetAlgorithm(\"agc\");\n> > + RPiController::Agc *agc = reinterpret_cast(algo);\n> \n> Ah yes, here it is. We should probably cast to an\n> RPiController::AgcAlgorithm here. Once those new Pause/Resume\n> functions have been moved to that class then a cast to AgcAlgorithm\n> should be fine.\n\nAah okay.\n\n> \n> > if (!agc) {\n> > LOG(IPARPI, Warning)\n> > - << \"Could not set AE_ENABLE - no AGC algorithm\";\n> > + << \"Could not set EXPOSURE_TIME_MODE - no AGC algorithm\";\n> > break;\n> > }\n> >\n> > - if (ctrl.second.get() == false)\n> > - agc->Pause();\n> > + if (ctrl.second.get() == controls::ExposureTimeModeDisabled)\n> > + agc->PauseExposure();\n> > else\n> > - agc->Resume();\n> > + agc->ResumeExposure();\n> >\n> > - libcameraMetadata_.set(controls::AeEnable, ctrl.second.get());\n> > + libcameraMetadata_.set(controls::ExposureTimeMode,\n> > + ctrl.second.get());\n> > break;\n> > }\n> >\n> > @@ -656,6 +663,25 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > break;\n> > }\n> >\n> > + case controls::ANALOGUE_GAIN_MODE: {\n> > + RPiController::Algorithm *algo = controller_.GetAlgorithm(\"agc\");\n> > + RPiController::Agc *agc = reinterpret_cast(algo);\n> \n> Same thing here.\n> \n> > + if (!agc) {\n> > + LOG(IPARPI, Warning)\n> > + << \"Could not set ANALOGUE_GAIN_MODE - no AGC algorithm\";\n> > + break;\n> > + }\n> > +\n> > + if (ctrl.second.get() == controls::AnalogueGainModeDisabled)\n> > + agc->PauseGain();\n> > + else\n> > + agc->ResumeGain();\n> > +\n> > + libcameraMetadata_.set(controls::AnalogueGainMode,\n> > + ctrl.second.get());\n> > + break;\n> > + }\n> > +\n> > case controls::ANALOGUE_GAIN: {\n> > RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > controller_.GetAlgorithm(\"agc\"));\n> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > index c227d885..3a9c3b8d 100644\n> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > @@ -309,25 +309,25 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,\n> > /* \\todo Make this nicer. */\n> > int32_t ivalue;\n> > if (id == controls::ExposureTimeMode && exposureSet) {\n> > - int32_t exposureMode = controls->get(V4L2_CID_EXPOSURE_AUTO);\n> > - ivalue = value.get() == ExposureTimeModeAuto\n> > + int32_t exposureMode = controls->get(V4L2_CID_EXPOSURE_AUTO).get();\n> > + ivalue = value.get() == controls::ExposureTimeModeAuto\n> > ? (exposureMode == V4L2_EXPOSURE_SHUTTER_PRIORITY\n> > ? V4L2_EXPOSURE_AUTO\n> > : V4L2_EXPOSURE_APERTURE_PRIORITY)\n> > : V4L2_EXPOSURE_MANUAL;\n> > } else if (id == controls::ExposureTimeMode && !exposureSet) {\n> > - ivalue = value.get() == ExposureTimeModeAuto\n> > + ivalue = value.get() == controls::ExposureTimeModeAuto\n> > ? V4L2_EXPOSURE_APERTURE_PRIORITY\n> > : V4L2_EXPOSURE_MANUAL;\n> > } else if (id == controls::AnalogueGainMode && exposureSet) {\n> > - int32_t exposureMode = controls->get(V4L2_CID_EXPOSURE_AUTO);\n> > - ivalue = value.get() == AnalogueGainModeAuto\n> > + int32_t exposureMode = controls->get(V4L2_CID_EXPOSURE_AUTO).get();\n> > + ivalue = value.get() == controls::AnalogueGainModeAuto\n> > ? (exposureMode == V4L2_EXPOSURE_APERTURE_PRIORITY\n> > ? V4L2_EXPOSURE_AUTO\n> > : V4L2_EXPOSURE_SHUTTER_PRIORITY)\n> > : V4L2_EXPOSURE_MANUAL;\n> > } else if (id == controls::AnalogueGainMode && !exposureSet) {\n> > - ivalue = value.get() == AnalogueGainModeAuto\n> > + ivalue = value.get() == controls::AnalogueGainModeAuto\n> > ? V4L2_EXPOSURE_SHUTTER_PRIORITY\n> > : V4L2_EXPOSURE_MANUAL;\n> > }\n> > @@ -636,8 +636,9 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,\n> > break;\n> >\n> > case V4L2_CID_EXPOSURE_AUTO:\n> > - info = ControlInfo{ { ExposureTimeModeAuto, ExposureTimeModeDisabled },\n> > - ExposureTimeModeDisabled };\n> > + info = ControlInfo{ { controls::ExposureTimeModeAuto,\n> > + controls::ExposureTimeModeDisabled },\n> > + controls::ExposureTimeModeDisabled };\n> > break;\n> >\n> > case V4L2_CID_EXPOSURE_ABSOLUTE:\n> > --\n> > 2.27.0\n> >\n> \n> (Was this UVC stuff meant to be in the same patch?)\n\nOops, must be a rebase mistake :S\n\n> \n> Apart from those small things this is looking pretty close to me!\n\nGlad to hear that!\n\n(Just wondering, have you seen the patch that adds these new controls\nyet?)\n\n\nThanks,\n\nPaul","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 28442C3243\n\tfor ;\n\tTue, 28 Sep 2021 09:51:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 829B66918E;\n\tTue, 28 Sep 2021 11:51:29 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BFEBD684C8\n\tfor ;\n\tTue, 28 Sep 2021 11:51:28 +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 4B0FF3F1;\n\tTue, 28 Sep 2021 11:51:26 +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=\"ZeUfSFIL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632822688;\n\tbh=h/NZAyCutoIXDekrLLPDdycRU0mRG5ZiaPoCcHqV1Ww=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ZeUfSFILvS14X9PX+1qwwbUwFw68tMafceBZ8CgUaeWP6n+whxiyxVYYDres/+W8+\n\ti+ybOun2kXOb4JklDKKOI/xj+m6nb2i+0nsgEler6drdEF2h3nO98LMsKM+3YoFBI2\n\tRpGhHJslaOwkD8/KtsWTbpvifGQ/DKv4Cz8J2I8A=","Date":"Tue, 28 Sep 2021 18:51:21 +0900","From":"paul.elder@ideasonboard.com","To":"David Plowman ","Message-ID":"<20210928095121.GK4382@pyrite.rasen.tech>","References":"<20210928074959.3489544-1-paul.elder@ideasonboard.com>\n\t<20210928074959.3489544-3-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] [RFC PATCH 2/4] libcamera: pipeline:\n\traspberrypi: Support the new AE 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\" "}}]