[{"id":14263,"web_url":"https://patchwork.libcamera.org/comment/14263/","msgid":"<20201217151743.66m46drxyoszxmlq@uno.localdomain>","date":"2020-12-17T15:17:43","subject":"Re: [libcamera-devel] [PATCH v9 2/3] libcamera: raspberrypi: Add\n\tcontrol of sensor vblanking","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Naush,\n\n a few minors here and there\n\nOn Wed, Dec 16, 2020 at 11:22:01AM +0000, Naushir Patuck wrote:\n> Add support for setting V4L2_CID_VBLANK appropriately when setting\n> V4L2_CID_EXPOSURE. This will allow adaptive framerates during\n> viewfinder use cases (e.g. when the exposure time goes above 33ms, we\n> can reduce the framerate to lower than 30fps).\n>\n> The minimum and maximum frame durations are provided via libcamera\n> controls, and will prioritise exposure time limits over any AGC request.\n>\n> V4L2_CID_VBLANK is controlled through the staggered writer, just like\n> the exposure and gain controls.\n>\n> Signed-off-by: Naushir Patuck \n> Reviewed-by: David Plowman \n> Tested-by: David Plowman \n> ---\n> include/libcamera/ipa/raspberrypi.h | 1 +\n> src/ipa/raspberrypi/cam_helper.cpp | 35 ++++++++++++++-\n> src/ipa/raspberrypi/cam_helper.hpp | 15 ++++++-\n> src/ipa/raspberrypi/cam_helper_imx219.cpp | 13 +++++-\n> src/ipa/raspberrypi/cam_helper_imx477.cpp | 11 ++++-\n> src/ipa/raspberrypi/cam_helper_ov5647.cpp | 11 ++++-\n> src/ipa/raspberrypi/raspberrypi.cpp | 45 ++++++++++++++++---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 3 +-\n> 8 files changed, 120 insertions(+), 14 deletions(-)\n>\n> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> index 01fe5abc..1de36039 100644\n> --- a/include/libcamera/ipa/raspberrypi.h\n> +++ b/include/libcamera/ipa/raspberrypi.h\n> @@ -65,6 +65,7 @@ static const ControlInfoMap Controls = {\n> \t{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> \t{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n> \t{ &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> +\t{ &controls::FrameDurations, ControlInfo(1000, 1000000000) },\n> };\n>\n> } /* namespace RPi */\n> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp\n> index 6efa0d7f..018c17b9 100644\n> --- a/src/ipa/raspberrypi/cam_helper.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> @@ -34,8 +34,10 @@ CamHelper *CamHelper::Create(std::string const &cam_name)\n> \treturn nullptr;\n> }\n>\n> -CamHelper::CamHelper(MdParser *parser)\n> -\t: parser_(parser), initialized_(false)\n> +CamHelper::CamHelper(MdParser *parser, unsigned int maxFrameLength,\n> +\t\t unsigned int frameIntegrationDiff)\n> +\t: parser_(parser), initialized_(false), maxFrameLength_(maxFrameLength),\n> +\t frameIntegrationDiff_(frameIntegrationDiff)\n> {\n> }\n>\n> @@ -56,6 +58,35 @@ double CamHelper::Exposure(uint32_t exposure_lines) const\n> \treturn exposure_lines * mode_.line_length / 1000.0;\n> }\n>\n> +uint32_t CamHelper::GetVBlanking(double &exposure, double minFrameDuration,\n\nOutput parameters should be really passed as pointers, it's very hard\nto follow what the caller does otherwise. I see the same usage of\nreference for ouput params is widly spread in the IPA, so I won't\nbother you asking for a change.\n\n> +\t\t\t\t double maxFrameDuration) const\n> +{\n> +\tuint32_t frameLengthMin, frameLengthMax, vblank;\n> +\tuint32_t exposureLines = ExposureLines(exposure);\n> +\n> +\tassert(initialized_);\n> +\n> +\t/*\n> +\t * Clip frame length by the frame duration range and the maximum allowable\n> +\t * value in the sensor, given by maxFrameLength_.\n> +\t */\n> +\tframeLengthMin = std::clamp(1e3 * minFrameDuration / mode_.line_length,\n> +\t\t\t\t\t mode_.height, maxFrameLength_);\n> +\tframeLengthMax = std::clamp(1e3 * maxFrameDuration / mode_.line_length,\n> +\t\t\t\t\t mode_.height, maxFrameLength_);\n> +\t/*\n> +\t * Limit the exposure to the maximum frame duration requested, and\n> +\t * re-calculate if it has been clipped.\n> +\t */\n> +\texposureLines = std::min(frameLengthMax - frameIntegrationDiff_, exposureLines);\n> +\texposure = Exposure(exposureLines);\n> +\n> +\t/* Limit the vblank to the range allowed by the frame length limits. */\n> +\tvblank = std::clamp(exposureLines + frameIntegrationDiff_,\n> +\t\t\t frameLengthMin, frameLengthMax) - mode_.height;\n\nThis looks good and matches the description of controls priorities\n\n> +\treturn vblank;\n> +}\n> +\n> void CamHelper::SetCameraMode(const CameraMode &mode)\n> {\n> \tmode_ = mode;\n> diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp\n> index 044c2866..b1739a57 100644\n> --- a/src/ipa/raspberrypi/cam_helper.hpp\n> +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> @@ -62,12 +62,15 @@ class CamHelper\n> {\n> public:\n> \tstatic CamHelper *Create(std::string const &cam_name);\n> -\tCamHelper(MdParser *parser);\n> +\tCamHelper(MdParser *parser, unsigned int maxFrameLength,\n> +\t\t unsigned int frameIntegrationDiff);\n> \tvirtual ~CamHelper();\n> \tvoid SetCameraMode(const CameraMode &mode);\n> \tMdParser &Parser() const { return *parser_; }\n> \tuint32_t ExposureLines(double exposure_us) const;\n> \tdouble Exposure(uint32_t exposure_lines) const; // in us\n> +\tvirtual uint32_t GetVBlanking(double &exposure_us, double minFrameDuration,\n> +\t\t\t\t double maxFrameDuration) const;\n\nIs this really virtual ?\n\n> \tvirtual uint32_t GainCode(double gain) const = 0;\n> \tvirtual double Gain(uint32_t gain_code) const = 0;\n> \tvirtual void GetDelays(int &exposure_delay, int &gain_delay) const;\n> @@ -76,10 +79,20 @@ public:\n> \tvirtual unsigned int HideFramesModeSwitch() const;\n> \tvirtual unsigned int MistrustFramesStartup() const;\n> \tvirtual unsigned int MistrustFramesModeSwitch() const;\n> +\n> protected:\n> \tMdParser *parser_;\n> \tCameraMode mode_;\n> +\n> +private:\n> \tbool initialized_;\n> +\t/* Largest possible frame length, in units of lines. */\n> +\tunsigned int maxFrameLength_;\n> +\t/*\n> +\t * Smallest difference between the frame length and integration time,\n> +\t * in units of lines.\n> +\t */\n> +\tunsigned int frameIntegrationDiff_;\n> };\n>\n> // This is for registering camera helpers with the system, so that the\n> diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> index db8ab879..8688ec09 100644\n> --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> @@ -49,13 +49,22 @@ public:\n> \tdouble Gain(uint32_t gain_code) const override;\n> \tunsigned int MistrustFramesModeSwitch() const override;\n> \tbool SensorEmbeddedDataPresent() const override;\n> +\n> +private:\n> +\t/*\n> +\t * Smallest difference between the frame length and integration time,\n> +\t * in units of lines.\n> +\t */\n> +\tstatic constexpr int frameIntegrationDiff = 4;\n> +\t/* Largest possible frame length, in units of lines. */\n> +\tstatic constexpr int maxFrameLength = 0xffff;\n> };\n>\n> CamHelperImx219::CamHelperImx219()\n> #if ENABLE_EMBEDDED_DATA\n> -\t: CamHelper(new MdParserImx219())\n> +\t: CamHelper(new MdParserImx219(), maxFrameLength, frameIntegrationDiff)\n> #else\n> -\t: CamHelper(new MdParserRPi())\n> +\t: CamHelper(new MdParserRPi(), maxFrameLength, frameIntegrationDiff)\n> #endif\n> {\n> }\n> diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> index 0e896ac7..53961310 100644\n> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> @@ -38,10 +38,19 @@ public:\n> \tuint32_t GainCode(double gain) const override;\n> \tdouble Gain(uint32_t gain_code) const override;\n> \tbool SensorEmbeddedDataPresent() const override;\n> +\n> +private:\n> +\t/*\n> +\t * Smallest difference between the frame length and integration time,\n> +\t * in units of lines.\n> +\t */\n> +\tstatic constexpr int frameIntegrationDiff = 22;\n> +\t/* Largest possible frame length, in units of lines. */\n> +\tstatic constexpr int maxFrameLength = 0xffdc;\n> };\n>\n> CamHelperImx477::CamHelperImx477()\n> -\t: CamHelper(new MdParserImx477())\n> +\t: CamHelper(new MdParserImx477(), maxFrameLength, frameIntegrationDiff)\n> {\n> }\n>\n> diff --git a/src/ipa/raspberrypi/cam_helper_ov5647.cpp b/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> index 0b841cd1..2bd8a754 100644\n> --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> @@ -23,6 +23,15 @@ public:\n> \tunsigned int HideFramesModeSwitch() const override;\n> \tunsigned int MistrustFramesStartup() const override;\n> \tunsigned int MistrustFramesModeSwitch() const override;\n> +\n> +private:\n> +\t/*\n> +\t * Smallest difference between the frame length and integration time,\n> +\t * in units of lines.\n> +\t */\n> +\tstatic constexpr int frameIntegrationDiff = 4;\n> +\t/* Largest possible frame length, in units of lines. */\n> +\tstatic constexpr int maxFrameLength = 0xffff;\n> };\n>\n> /*\n> @@ -31,7 +40,7 @@ public:\n> */\n>\n> CamHelperOv5647::CamHelperOv5647()\n> -\t: CamHelper(new MdParserRPi())\n> +\t: CamHelper(new MdParserRPi(), maxFrameLength, frameIntegrationDiff)\n> {\n> }\n>\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index d087b07e..e1fe35f5 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -58,6 +58,8 @@ namespace libcamera {\n> /* Configure the sensor with these values initially. */\n> constexpr double DefaultAnalogueGain = 1.0;\n> constexpr unsigned int DefaultExposureTime = 20000;\n> +constexpr double defaultMinFrameDuration = 1e6 / 30.0;\n> +constexpr double defaultMaxFrameDuration = 1e6 / 0.01;\n\nDo I count the number of 0s wrong, or the control maximum value is set\nto 10^9 and this is 10^8 ? Not much difference though both 100 or 1000\nseconds of max exposure should suit all needs :)\n\nThese can be made uint_64 already to avoid casts. Up to you.\n\n>\n> LOG_DEFINE_CATEGORY(IPARPI)\n>\n> @@ -150,6 +152,9 @@ private:\n>\n> \t/* Distinguish the first camera start from others. */\n> \tbool firstStart_;\n> +\n> +\t/* Frame duration (1/fps) given in microseconds. */\n> +\tdouble minFrameDuration_, maxFrameDuration_;\n> };\n>\n> int IPARPi::init(const IPASettings &settings)\n> @@ -332,7 +337,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> \t\tsensorMetadata = helper_->SensorEmbeddedDataPresent();\n>\n> \t\tresult->data.push_back(gainDelay);\n> -\t\tresult->data.push_back(exposureDelay);\n> +\t\tresult->data.push_back(exposureDelay); /* FOR EXPOSURE ctrl */\n\ns/FOR/for/\n\n> +\t\tresult->data.push_back(exposureDelay); /* For VBLANK ctrl */\n\nSo, VBLANK and EXPOSURE ctrl have the same delay. How do we guarantee\nVBLANK is updated first ?\n\n> \t\tresult->data.push_back(sensorMetadata);\n>\n> \t\tresult->operation |= RPi::IPA_CONFIG_STAGGERED_WRITE;\n> @@ -382,6 +388,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> \t\tAgcStatus agcStatus;\n> \t\tagcStatus.shutter_time = DefaultExposureTime;\n> \t\tagcStatus.analogue_gain = DefaultAnalogueGain;\n> +\t\tminFrameDuration_ = defaultMinFrameDuration;\n> +\t\tmaxFrameDuration_ = defaultMaxFrameDuration;\n\nI would initialize these before the ctrls declaration. These are not\nsent to the sensor, and seeing them in the middle here is confusing.\nOr are they needed by the applyAGC() function?\n\n> \t\tapplyAGC(&agcStatus, ctrls);\n>\n> \t\tresult->controls.emplace_back(ctrls);\n> @@ -524,7 +532,8 @@ bool IPARPi::validateSensorControls()\n> {\n> \tstatic const uint32_t ctrls[] = {\n> \t\tV4L2_CID_ANALOGUE_GAIN,\n> -\t\tV4L2_CID_EXPOSURE\n> +\t\tV4L2_CID_EXPOSURE,\n> +\t\tV4L2_CID_VBLANK\n> \t};\n>\n> \tfor (auto c : ctrls) {\n> @@ -804,6 +813,18 @@ void IPARPi::queueRequest(const ControlList &controls)\n> \t\t\tbreak;\n> \t\t}\n\nDoesn't the SetFixedShutter() function need to be updated to clip the\nshutter time in the frameDuration limits too ? Can we address it or at\nleast record it with a \\todo ?\n\n>\n> +\t\tcase controls::FRAME_DURATIONS: {\n> +\t\t\tauto frameDurations = ctrl.second.get>();\n> +\n> +\t\t\t/* This will be applied once AGC recalculations occur. */\n> +\t\t\tminFrameDuration_ = frameDurations[0] ? frameDurations[0] : defaultMinFrameDuration;\n> +\t\t\tmaxFrameDuration_ = frameDurations[1] ? frameDurations[1] : defaultMaxFrameDuration;\n\nI'm surprised how cast from int64_t to double is automatically\nhandled..\n\n> +\t\t\tlibcameraMetadata_.set(controls::FrameDurations,\n> +\t\t\t\t\t { static_cast(minFrameDuration_),\n> +\t\t\t\t\t\t static_cast(maxFrameDuration_) });\n\nI don't think this matches the control description though:\n\n+ When reported by pipelines, the control expresses the duration of the\n+ sensor frame used to produce streams part of the completed Request.\n+ The minimum and maximum values shall then be the same, as the sensor\n+ frame duration is a fixed parameter.\n\nShould you record the exposure time calculated by GetVBlanking() and\nreport it here ?\n\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> \t\tdefault:\n> \t\t\tLOG(IPARPI, Warning)\n> \t\t\t\t<< \"Ctrl \" << controls::controls.at(ctrl.first)->name()\n> @@ -962,15 +983,27 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)\n> void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)\n> {\n> \tint32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);\n> -\tint32_t exposureLines = helper_->ExposureLines(agcStatus->shutter_time);\n>\n> -\tLOG(IPARPI, Debug) << \"Applying AGC Exposure: \" << agcStatus->shutter_time\n> -\t\t\t << \" (Shutter lines: \" << exposureLines << \") Gain: \"\n> +\t/* GetVBlanking might clip exposure time to the fps limits. */\n> +\tdouble exposure = agcStatus->shutter_time;\n> +\tint32_t vblanking = helper_->GetVBlanking(exposure, minFrameDuration_,\n> +\t\t\t\t\t\t maxFrameDuration_);\n> +\tint32_t exposureLines = helper_->ExposureLines(exposure);\n> +\n> +\tLOG(IPARPI, Debug) << \"Applying AGC Exposure: \" << exposure\n> +\t\t\t << \" (Shutter lines: \" << exposureLines << \", AGC requested \"\n> +\t\t\t << agcStatus->shutter_time << \") Gain: \"\n> \t\t\t << agcStatus->analogue_gain << \" (Gain Code: \"\n> \t\t\t << gainCode << \")\";\n>\n> -\tctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);\n> +\t/*\n> +\t * Due to the behavior of V4L2, the current value of VBLANK could clip the\n> +\t * exposure time without us knowing. The next time though this function should\n> +\t * clip exposure correctly.\n\nReplying to myself: we skip one frame in case VBLANK clips exposure.\nI guess setting VBLANK one frame in advance compared to exposure won't\nhelp, as it could clip the previous frame. The only solution is to\nprioritize VBLANK when setting controls to the device I guess.\n\n> +\t */\n> +\tctrls.set(V4L2_CID_VBLANK, vblanking);\n> \tctrls.set(V4L2_CID_EXPOSURE, exposureLines);\n> +\tctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);\n> }\n>\n> void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 7a5f5881..252cab64 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -1233,7 +1233,8 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> \t\tif (!staggeredCtrl_) {\n> \t\t\tstaggeredCtrl_.init(unicam_[Unicam::Image].dev(),\n> \t\t\t\t\t { { V4L2_CID_ANALOGUE_GAIN, result.data[resultIdx++] },\n> -\t\t\t\t\t { V4L2_CID_EXPOSURE, result.data[resultIdx++] } });\n> +\t\t\t\t\t { V4L2_CID_EXPOSURE, result.data[resultIdx++] },\n> +\t\t\t\t\t { V4L2_CID_VBLANK, result.data[resultIdx++] } });\n\nA few issues apart (metadata handling and manual exposure clipping\napart) the rest is all minor stuff. With these fixed/clarified\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> \t\t\tsensorMetadata_ = result.data[resultIdx++];\n> \t\t}\n> \t}\n> --\n> 2.25.1\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 E698EBD80A\n\tfor ;\n\tThu, 17 Dec 2020 15:17:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 738CF61597;\n\tThu, 17 Dec 2020 16:17:34 +0100 (CET)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8698D61591\n\tfor ;\n\tThu, 17 Dec 2020 16:17:33 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id E497A240005;\n\tThu, 17 Dec 2020 15:17:32 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Thu, 17 Dec 2020 16:17:43 +0100","From":"Jacopo Mondi ","To":"Naushir Patuck ","Message-ID":"<20201217151743.66m46drxyoszxmlq@uno.localdomain>","References":"<20201216112202.5063-1-naush@raspberrypi.com>\n\t<20201216112202.5063-3-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201216112202.5063-3-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v9 2/3] libcamera: raspberrypi: Add\n\tcontrol of sensor vblanking","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","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14264,"web_url":"https://patchwork.libcamera.org/comment/14264/","msgid":"","date":"2020-12-17T15:49:22","subject":"Re: [libcamera-devel] [PATCH v9 2/3] libcamera: raspberrypi: Add\n\tcontrol of sensor vblanking","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Jacopo,\n\nThank you for your review comments.\n\nOn Thu, 17 Dec 2020 at 15:17, Jacopo Mondi wrote:\n\n> Hi Naush,\n>\n> a few minors here and there\n>\n> On Wed, Dec 16, 2020 at 11:22:01AM +0000, Naushir Patuck wrote:\n> > Add support for setting V4L2_CID_VBLANK appropriately when setting\n> > V4L2_CID_EXPOSURE. This will allow adaptive framerates during\n> > viewfinder use cases (e.g. when the exposure time goes above 33ms, we\n> > can reduce the framerate to lower than 30fps).\n> >\n> > The minimum and maximum frame durations are provided via libcamera\n> > controls, and will prioritise exposure time limits over any AGC request.\n> >\n> > V4L2_CID_VBLANK is controlled through the staggered writer, just like\n> > the exposure and gain controls.\n> >\n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: David Plowman \n> > Tested-by: David Plowman \n> > ---\n> > include/libcamera/ipa/raspberrypi.h | 1 +\n> > src/ipa/raspberrypi/cam_helper.cpp | 35 ++++++++++++++-\n> > src/ipa/raspberrypi/cam_helper.hpp | 15 ++++++-\n> > src/ipa/raspberrypi/cam_helper_imx219.cpp | 13 +++++-\n> > src/ipa/raspberrypi/cam_helper_imx477.cpp | 11 ++++-\n> > src/ipa/raspberrypi/cam_helper_ov5647.cpp | 11 ++++-\n> > src/ipa/raspberrypi/raspberrypi.cpp | 45 ++++++++++++++++---\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 3 +-\n> > 8 files changed, 120 insertions(+), 14 deletions(-)\n> >\n> > diff --git a/include/libcamera/ipa/raspberrypi.h\n> b/include/libcamera/ipa/raspberrypi.h\n> > index 01fe5abc..1de36039 100644\n> > --- a/include/libcamera/ipa/raspberrypi.h\n> > +++ b/include/libcamera/ipa/raspberrypi.h\n> > @@ -65,6 +65,7 @@ static const ControlInfoMap Controls = {\n> > { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> > { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n> > { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535,\n> 65535, 65535, 65535), Rectangle{}) },\n> > + { &controls::FrameDurations, ControlInfo(1000, 1000000000) },\n> > };\n> >\n> > } /* namespace RPi */\n> > diff --git a/src/ipa/raspberrypi/cam_helper.cpp\n> b/src/ipa/raspberrypi/cam_helper.cpp\n> > index 6efa0d7f..018c17b9 100644\n> > --- a/src/ipa/raspberrypi/cam_helper.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> > @@ -34,8 +34,10 @@ CamHelper *CamHelper::Create(std::string const\n> &cam_name)\n> > return nullptr;\n> > }\n> >\n> > -CamHelper::CamHelper(MdParser *parser)\n> > - : parser_(parser), initialized_(false)\n> > +CamHelper::CamHelper(MdParser *parser, unsigned int maxFrameLength,\n> > + unsigned int frameIntegrationDiff)\n> > + : parser_(parser), initialized_(false),\n> maxFrameLength_(maxFrameLength),\n> > + frameIntegrationDiff_(frameIntegrationDiff)\n> > {\n> > }\n> >\n> > @@ -56,6 +58,35 @@ double CamHelper::Exposure(uint32_t exposure_lines)\n> const\n> > return exposure_lines * mode_.line_length / 1000.0;\n> > }\n> >\n> > +uint32_t CamHelper::GetVBlanking(double &exposure, double\n> minFrameDuration,\n>\n> Output parameters should be really passed as pointers, it's very hard\n> to follow what the caller does otherwise. I see the same usage of\n> reference for ouput params is widly spread in the IPA, so I won't\n> bother you asking for a change.\n>\n\nAck. There is a tidy-up task for our Controller that we need to get to at\nsome point.\n\n\n>\n> > + double maxFrameDuration) const\n> > +{\n> > + uint32_t frameLengthMin, frameLengthMax, vblank;\n> > + uint32_t exposureLines = ExposureLines(exposure);\n> > +\n> > + assert(initialized_);\n> > +\n> > + /*\n> > + * Clip frame length by the frame duration range and the maximum\n> allowable\n> > + * value in the sensor, given by maxFrameLength_.\n> > + */\n> > + frameLengthMin = std::clamp(1e3 * minFrameDuration /\n> mode_.line_length,\n> > + mode_.height,\n> maxFrameLength_);\n> > + frameLengthMax = std::clamp(1e3 * maxFrameDuration /\n> mode_.line_length,\n> > + mode_.height,\n> maxFrameLength_);\n> > + /*\n> > + * Limit the exposure to the maximum frame duration requested, and\n> > + * re-calculate if it has been clipped.\n> > + */\n> > + exposureLines = std::min(frameLengthMax - frameIntegrationDiff_,\n> exposureLines);\n> > + exposure = Exposure(exposureLines);\n> > +\n> > + /* Limit the vblank to the range allowed by the frame length\n> limits. */\n> > + vblank = std::clamp(exposureLines + frameIntegrationDiff_,\n> > + frameLengthMin, frameLengthMax) - mode_.height;\n>\n> This looks good and matches the description of controls priorities\n>\n> > + return vblank;\n> > +}\n> > +\n> > void CamHelper::SetCameraMode(const CameraMode &mode)\n> > {\n> > mode_ = mode;\n> > diff --git a/src/ipa/raspberrypi/cam_helper.hpp\n> b/src/ipa/raspberrypi/cam_helper.hpp\n> > index 044c2866..b1739a57 100644\n> > --- a/src/ipa/raspberrypi/cam_helper.hpp\n> > +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> > @@ -62,12 +62,15 @@ class CamHelper\n> > {\n> > public:\n> > static CamHelper *Create(std::string const &cam_name);\n> > - CamHelper(MdParser *parser);\n> > + CamHelper(MdParser *parser, unsigned int maxFrameLength,\n> > + unsigned int frameIntegrationDiff);\n> > virtual ~CamHelper();\n> > void SetCameraMode(const CameraMode &mode);\n> > MdParser &Parser() const { return *parser_; }\n> > uint32_t ExposureLines(double exposure_us) const;\n> > double Exposure(uint32_t exposure_lines) const; // in us\n> > + virtual uint32_t GetVBlanking(double &exposure_us, double\n> minFrameDuration,\n> > + double maxFrameDuration) const;\n>\n> Is this really virtual ?\n>\n\nYes, I intentionally marked it as virtual. There will be some sensors that\nuse a different formula/method to calculate exposure time. In particular,\nI am thinking about imx477 when we eventually get to enable very long\nexposure modes (i.e. in order of minutes) where some extra multipliers are\nused in the calculation. Additionally, global shutter sensors are also\nones that don't use a simple VTS * HTS type calculation. Overriding this\nin the specific camera helper class will allow us to account for these.\n\n\n>\n> > virtual uint32_t GainCode(double gain) const = 0;\n> > virtual double Gain(uint32_t gain_code) const = 0;\n> > virtual void GetDelays(int &exposure_delay, int &gain_delay) const;\n> > @@ -76,10 +79,20 @@ public:\n> > virtual unsigned int HideFramesModeSwitch() const;\n> > virtual unsigned int MistrustFramesStartup() const;\n> > virtual unsigned int MistrustFramesModeSwitch() const;\n> > +\n> > protected:\n> > MdParser *parser_;\n> > CameraMode mode_;\n> > +\n> > +private:\n> > bool initialized_;\n> > + /* Largest possible frame length, in units of lines. */\n> > + unsigned int maxFrameLength_;\n> > + /*\n> > + * Smallest difference between the frame length and integration\n> time,\n> > + * in units of lines.\n> > + */\n> > + unsigned int frameIntegrationDiff_;\n> > };\n> >\n> > // This is for registering camera helpers with the system, so that the\n> > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > index db8ab879..8688ec09 100644\n> > --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > @@ -49,13 +49,22 @@ public:\n> > double Gain(uint32_t gain_code) const override;\n> > unsigned int MistrustFramesModeSwitch() const override;\n> > bool SensorEmbeddedDataPresent() const override;\n> > +\n> > +private:\n> > + /*\n> > + * Smallest difference between the frame length and integration\n> time,\n> > + * in units of lines.\n> > + */\n> > + static constexpr int frameIntegrationDiff = 4;\n> > + /* Largest possible frame length, in units of lines. */\n> > + static constexpr int maxFrameLength = 0xffff;\n> > };\n> >\n> > CamHelperImx219::CamHelperImx219()\n> > #if ENABLE_EMBEDDED_DATA\n> > - : CamHelper(new MdParserImx219())\n> > + : CamHelper(new MdParserImx219(), maxFrameLength,\n> frameIntegrationDiff)\n> > #else\n> > - : CamHelper(new MdParserRPi())\n> > + : CamHelper(new MdParserRPi(), maxFrameLength,\n> frameIntegrationDiff)\n> > #endif\n> > {\n> > }\n> > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > index 0e896ac7..53961310 100644\n> > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > @@ -38,10 +38,19 @@ public:\n> > uint32_t GainCode(double gain) const override;\n> > double Gain(uint32_t gain_code) const override;\n> > bool SensorEmbeddedDataPresent() const override;\n> > +\n> > +private:\n> > + /*\n> > + * Smallest difference between the frame length and integration\n> time,\n> > + * in units of lines.\n> > + */\n> > + static constexpr int frameIntegrationDiff = 22;\n> > + /* Largest possible frame length, in units of lines. */\n> > + static constexpr int maxFrameLength = 0xffdc;\n> > };\n> >\n> > CamHelperImx477::CamHelperImx477()\n> > - : CamHelper(new MdParserImx477())\n> > + : CamHelper(new MdParserImx477(), maxFrameLength,\n> frameIntegrationDiff)\n> > {\n> > }\n> >\n> > diff --git a/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> b/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> > index 0b841cd1..2bd8a754 100644\n> > --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> > @@ -23,6 +23,15 @@ public:\n> > unsigned int HideFramesModeSwitch() const override;\n> > unsigned int MistrustFramesStartup() const override;\n> > unsigned int MistrustFramesModeSwitch() const override;\n> > +\n> > +private:\n> > + /*\n> > + * Smallest difference between the frame length and integration\n> time,\n> > + * in units of lines.\n> > + */\n> > + static constexpr int frameIntegrationDiff = 4;\n> > + /* Largest possible frame length, in units of lines. */\n> > + static constexpr int maxFrameLength = 0xffff;\n> > };\n> >\n> > /*\n> > @@ -31,7 +40,7 @@ public:\n> > */\n> >\n> > CamHelperOv5647::CamHelperOv5647()\n> > - : CamHelper(new MdParserRPi())\n> > + : CamHelper(new MdParserRPi(), maxFrameLength,\n> frameIntegrationDiff)\n> > {\n> > }\n> >\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index d087b07e..e1fe35f5 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -58,6 +58,8 @@ namespace libcamera {\n> > /* Configure the sensor with these values initially. */\n> > constexpr double DefaultAnalogueGain = 1.0;\n> > constexpr unsigned int DefaultExposureTime = 20000;\n> > +constexpr double defaultMinFrameDuration = 1e6 / 30.0;\n> > +constexpr double defaultMaxFrameDuration = 1e6 / 0.01;\n>\n> Do I count the number of 0s wrong, or the control maximum value is set\n> to 10^9 and this is 10^8 ? Not much difference though both 100 or 1000\n> seconds of max exposure should suit all needs :)\n>\n\nYes, they are different. This default value for the IPA is just a value I\npicked, not strictly related to the Control max value. One could argue\nthat a more sensible number might be closer to around 500ms, but really\nthis gets clipped by the exposure mode profile anyway.\n\n\n>\n> These can be made uint_64 already to avoid casts. Up to you.\n>\n\nI intentionally left them as doubles as our internal representation in the\nController and IPA state uses doubles. As above, there is a tidy up task\nto do in our controllers to rationalise this at some point.\n\n\n>\n> >\n> > LOG_DEFINE_CATEGORY(IPARPI)\n> >\n> > @@ -150,6 +152,9 @@ private:\n> >\n> > /* Distinguish the first camera start from others. */\n> > bool firstStart_;\n> > +\n> > + /* Frame duration (1/fps) given in microseconds. */\n> > + double minFrameDuration_, maxFrameDuration_;\n> > };\n> >\n> > int IPARPi::init(const IPASettings &settings)\n> > @@ -332,7 +337,8 @@ void IPARPi::configure(const CameraSensorInfo\n> &sensorInfo,\n> > sensorMetadata = helper_->SensorEmbeddedDataPresent();\n> >\n> > result->data.push_back(gainDelay);\n> > - result->data.push_back(exposureDelay);\n> > + result->data.push_back(exposureDelay); /* FOR EXPOSURE\n> ctrl */\n>\n> s/FOR/for/\n>\n\nAck.\n\n\n>\n> > + result->data.push_back(exposureDelay); /* For VBLANK ctrl\n> */\n>\n> So, VBLANK and EXPOSURE ctrl have the same delay. How do we guarantee\n> VBLANK is updated first ?\n>\n> > result->data.push_back(sensorMetadata);\n> >\n> > result->operation |= RPi::IPA_CONFIG_STAGGERED_WRITE;\n> > @@ -382,6 +388,8 @@ void IPARPi::configure(const CameraSensorInfo\n> &sensorInfo,\n> > AgcStatus agcStatus;\n> > agcStatus.shutter_time = DefaultExposureTime;\n> > agcStatus.analogue_gain = DefaultAnalogueGain;\n> > + minFrameDuration_ = defaultMinFrameDuration;\n> > + maxFrameDuration_ = defaultMaxFrameDuration;\n>\n> I would initialize these before the ctrls declaration. These are not\n> sent to the sensor, and seeing them in the middle here is confusing.\n> Or are they needed by the applyAGC() function?\n>\n\nThey are needed by the applyAGC() function. However, I can move them above\nthe ctrl declaration to improve readability.\n\n\n>\n> > applyAGC(&agcStatus, ctrls);\n> >\n> > result->controls.emplace_back(ctrls);\n> > @@ -524,7 +532,8 @@ bool IPARPi::validateSensorControls()\n> > {\n> > static const uint32_t ctrls[] = {\n> > V4L2_CID_ANALOGUE_GAIN,\n> > - V4L2_CID_EXPOSURE\n> > + V4L2_CID_EXPOSURE,\n> > + V4L2_CID_VBLANK\n> > };\n> >\n> > for (auto c : ctrls) {\n> > @@ -804,6 +813,18 @@ void IPARPi::queueRequest(const ControlList\n> &controls)\n> > break;\n> > }\n>\n> Doesn't the SetFixedShutter() function need to be updated to clip the\n> shutter time in the frameDuration limits too ? Can we address it or at\n> least record it with a \\todo ?\n>\n\nYes, SetFixedShutter does need to account for frame duration limits. I do\nhave a set of patches that are waiting to be sent out for review that does\nexactly this (along with passing the limits to the AGC algorithms).\nProbably a \\todo is not worth it?\n\n\n>\n> >\n> > + case controls::FRAME_DURATIONS: {\n> > + auto frameDurations = ctrl.second.get int64_t>>();\n> > +\n> > + /* This will be applied once AGC recalculations\n> occur. */\n> > + minFrameDuration_ = frameDurations[0] ?\n> frameDurations[0] : defaultMinFrameDuration;\n> > + maxFrameDuration_ = frameDurations[1] ?\n> frameDurations[1] : defaultMaxFrameDuration;\n>\n> I'm surprised how cast from int64_t to double is automatically\n> handled..\n>\n\nSeems to compile without any warnings :-)\n\n\n>\n> > + libcameraMetadata_.set(controls::FrameDurations,\n> > + {\n> static_cast(minFrameDuration_),\n> > +\n> static_cast(maxFrameDuration_) });\n>\n> I don't think this matches the control description though:\n>\n> + When reported by pipelines, the control expresses the duration\n> of the\n> + sensor frame used to produce streams part of the completed\n> Request.\n> + The minimum and maximum values shall then be the same, as the\n> sensor\n> + frame duration is a fixed parameter.\n>\n> Should you record the exposure time calculated by GetVBlanking() and\n> report it here ?\n>\n\nYes, this should be the \"adjusted\" frame durations. I do have that fix in\nmy next set of patches, but perhaps I apply them here instead? Note that I\ndo not use exposure directly to calculate this, rather I use the vblank\nlimits provided by the sensor. There is a sensor dependent offset between\nexposure lines and VTS, so the numbers may be close, but not fully\naccurate, when using exposure time to calculate frame durations.\n\n\n>\n> > + break;\n> > + }\n> > +\n> > default:\n> > LOG(IPARPI, Warning)\n> > << \"Ctrl \" << controls::controls.at\n> (ctrl.first)->name()\n> > @@ -962,15 +983,27 @@ void IPARPi::applyAWB(const struct AwbStatus\n> *awbStatus, ControlList &ctrls)\n> > void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList\n> &ctrls)\n> > {\n> > int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);\n> > - int32_t exposureLines =\n> helper_->ExposureLines(agcStatus->shutter_time);\n> >\n> > - LOG(IPARPI, Debug) << \"Applying AGC Exposure: \" <<\n> agcStatus->shutter_time\n> > - << \" (Shutter lines: \" << exposureLines << \")\n> Gain: \"\n> > + /* GetVBlanking might clip exposure time to the fps limits. */\n> > + double exposure = agcStatus->shutter_time;\n> > + int32_t vblanking = helper_->GetVBlanking(exposure,\n> minFrameDuration_,\n> > + maxFrameDuration_);\n> > + int32_t exposureLines = helper_->ExposureLines(exposure);\n> > +\n> > + LOG(IPARPI, Debug) << \"Applying AGC Exposure: \" << exposure\n> > + << \" (Shutter lines: \" << exposureLines << \",\n> AGC requested \"\n> > + << agcStatus->shutter_time << \") Gain: \"\n> > << agcStatus->analogue_gain << \" (Gain Code: \"\n> > << gainCode << \")\";\n> >\n> > - ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);\n> > + /*\n> > + * Due to the behavior of V4L2, the current value of VBLANK could\n> clip the\n> > + * exposure time without us knowing. The next time though this\n> function should\n> > + * clip exposure correctly.\n>\n> Replying to myself: we skip one frame in case VBLANK clips exposure.\n> I guess setting VBLANK one frame in advance compared to exposure won't\n> help, as it could clip the previous frame. The only solution is to\n> prioritize VBLANK when setting controls to the device I guess.\n>\n\nIn my next patch set, I address this by allowing the staggered write\ncontrols to write VBLANK before any other controls. Depending on the\ntiming of DelayedControls being merged, I can easily port it if needed, or\njust submit my change to staggered write.\n\n\n>\n> > + */\n> > + ctrls.set(V4L2_CID_VBLANK, vblanking);\n> > ctrls.set(V4L2_CID_EXPOSURE, exposureLines);\n> > + ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);\n> > }\n> >\n> > void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList\n> &ctrls)\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 7a5f5881..252cab64 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -1233,7 +1233,8 @@ int RPiCameraData::configureIPA(const\n> CameraConfiguration *config)\n> > if (!staggeredCtrl_) {\n> > staggeredCtrl_.init(unicam_[Unicam::Image].dev(),\n> > { { V4L2_CID_ANALOGUE_GAIN,\n> result.data[resultIdx++] },\n> > - { V4L2_CID_EXPOSURE,\n> result.data[resultIdx++] } });\n> > + { V4L2_CID_EXPOSURE,\n> result.data[resultIdx++] },\n> > + { V4L2_CID_VBLANK,\n> result.data[resultIdx++] } });\n>\n> A few issues apart (metadata handling and manual exposure clipping\n> apart) the rest is all minor stuff. With these fixed/clarified\n>\n\nI will update with a v10 to address the metadata and typo. The manual\nexposure clipping ought to be part of the next patch set as it is part of\nthe bigger change where the AGC applies the limits as well. Is that ok\nwith you?\n\nRegards,\nNaush\n\n\n\n>\n> Reviewed-by: Jacopo Mondi \n>\n> Thanks\n> j\n>\n> > sensorMetadata_ = result.data[resultIdx++];\n> > }\n> > }\n> > --\n> > 2.25.1\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\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 1C688C0F1A\n\tfor ;\n\tThu, 17 Dec 2020 15:49:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8096061597;\n\tThu, 17 Dec 2020 16:49:41 +0100 (CET)","from mail-lf1-x130.google.com (mail-lf1-x130.google.com\n\t[IPv6:2a00:1450:4864:20::130])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AD72F61591\n\tfor ;\n\tThu, 17 Dec 2020 16:49:39 +0100 (CET)","by mail-lf1-x130.google.com with SMTP id o17so55606123lfg.4\n\tfor ;\n\tThu, 17 Dec 2020 07:49:39 -0800 (PST)"],"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=\"cHlG128D\"; 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=TDWzfzRFTb5nI5lKArh+ARkCjHTHl3T+IHW38W2cFjg=;\n\tb=cHlG128DqtB26c2wx1Lg3chKRYzOn4C0Ex25HVOTaCwDlm4Ybr/Qw3R5b46I1AuUP8\n\tjqTuLSvIpWxKHowgW14/g0/HmcL4M9phBzf55PLNsxr/YvZSQ3YXyWFkBQ/QjWjuPdXX\n\tfZkZxoPVS2udaX5fKpVQ1BrpFNucLrbgiiFLulpmMPpfA1PHrEZ9y+g2KENteVV9wOnT\n\tM3YzBVqHMxn91GXY03Wcf2iyR0fDZb4ONXlpOLg2Ldjxugyp1CyCza88Ai3mLWmTTZKj\n\thxE20bWJmBhyB8v35p027Wp+D5GqGTq0w8OXQUfpmeC3ZGG642BhaJFav5I3LKVDBxB5\n\tl1/g==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=TDWzfzRFTb5nI5lKArh+ARkCjHTHl3T+IHW38W2cFjg=;\n\tb=XyG/o3fC/fkiUFHq3a4r/LBGc09sX/6joFTfdDKOVQRPzuLb7ydc+I4i+paHkMsAdy\n\tynK+I0IA4FDmNFRnYOceEyrVGH98BlOCwbx8b2cqw5rhsB6cz60SjUlpaJ2IsCzSV97G\n\tIoRkM1du2Of1KutACGp6/kLY4SIC2N4m8fNYCJA3QKfVdGhuq98FhttoF1YNpjBrNn2z\n\t7WGiQ2yIWT6hHv2ydcTNrHnPKU5M513vFzsv6t4aWaiW3+1qqwKWi2doO+d4dEZlO1pK\n\tY2+bTkA3jaqDijJZVET23etYjxlkc+tIjPl5yqVZkpUJUc0nzCeGw4KsRFTt6luIrKN8\n\tgdxQ==","X-Gm-Message-State":"AOAM532BIvDBKy1OkI9e0vfoaFqSFRPMDq8Vm8IYaPgouQGAivlmWHh1\n\tUFwRTqjbeNbVLxaqzf2moLxAMSxOzYobDli8XoNVxA==","X-Google-Smtp-Source":"ABdhPJxAmzYhLldEw/UMdT53teJXBNyUdmb78ga5cEZgIjnY81nGzgIZxTS22bF4b/ZM6lpr/UhGLtewU3BjlPemDiQ=","X-Received":"by 2002:a2e:810c:: with SMTP id\n\td12mr12223109ljg.400.1608220178802; \n\tThu, 17 Dec 2020 07:49:38 -0800 (PST)","MIME-Version":"1.0","References":"<20201216112202.5063-1-naush@raspberrypi.com>\n\t<20201216112202.5063-3-naush@raspberrypi.com>\n\t<20201217151743.66m46drxyoszxmlq@uno.localdomain>","In-Reply-To":"<20201217151743.66m46drxyoszxmlq@uno.localdomain>","From":"Naushir Patuck ","Date":"Thu, 17 Dec 2020 15:49:22 +0000","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v9 2/3] libcamera: raspberrypi: Add\n\tcontrol of sensor vblanking","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 ","Content-Type":"multipart/mixed;\n\tboundary=\"===============4455224908796951004==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14265,"web_url":"https://patchwork.libcamera.org/comment/14265/","msgid":"","date":"2020-12-17T16:12:08","subject":"Re: [libcamera-devel] [PATCH v9 2/3] libcamera: raspberrypi: Add\n\tcontrol of sensor vblanking","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Jacopo,\n\n\n\nOn Thu, 17 Dec 2020 at 15:49, Naushir Patuck wrote:\n\n> Hi Jacopo,\n>\n> Thank you for your review comments.\n>\n> On Thu, 17 Dec 2020 at 15:17, Jacopo Mondi wrote:\n>\n>> Hi Naush,\n>>\n>> a few minors here and there\n>>\n>> On Wed, Dec 16, 2020 at 11:22:01AM +0000, Naushir Patuck wrote:\n>> > Add support for setting V4L2_CID_VBLANK appropriately when setting\n>> > V4L2_CID_EXPOSURE. This will allow adaptive framerates during\n>> > viewfinder use cases (e.g. when the exposure time goes above 33ms, we\n>> > can reduce the framerate to lower than 30fps).\n>> >\n>> > The minimum and maximum frame durations are provided via libcamera\n>> > controls, and will prioritise exposure time limits over any AGC request.\n>> >\n>> > V4L2_CID_VBLANK is controlled through the staggered writer, just like\n>> > the exposure and gain controls.\n>> >\n>> > Signed-off-by: Naushir Patuck \n>> > Reviewed-by: David Plowman \n>> > Tested-by: David Plowman \n>> > ---\n>> > include/libcamera/ipa/raspberrypi.h | 1 +\n>> > src/ipa/raspberrypi/cam_helper.cpp | 35 ++++++++++++++-\n>> > src/ipa/raspberrypi/cam_helper.hpp | 15 ++++++-\n>> > src/ipa/raspberrypi/cam_helper_imx219.cpp | 13 +++++-\n>> > src/ipa/raspberrypi/cam_helper_imx477.cpp | 11 ++++-\n>> > src/ipa/raspberrypi/cam_helper_ov5647.cpp | 11 ++++-\n>> > src/ipa/raspberrypi/raspberrypi.cpp | 45 ++++++++++++++++---\n>> > .../pipeline/raspberrypi/raspberrypi.cpp | 3 +-\n>> > 8 files changed, 120 insertions(+), 14 deletions(-)\n>> >\n>> > diff --git a/include/libcamera/ipa/raspberrypi.h\n>> b/include/libcamera/ipa/raspberrypi.h\n>> > index 01fe5abc..1de36039 100644\n>> > --- a/include/libcamera/ipa/raspberrypi.h\n>> > +++ b/include/libcamera/ipa/raspberrypi.h\n>> > @@ -65,6 +65,7 @@ static const ControlInfoMap Controls = {\n>> > { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n>> > { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n>> > { &controls::ScalerCrop, ControlInfo(Rectangle{},\n>> Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n>> > + { &controls::FrameDurations, ControlInfo(1000, 1000000000) },\n>> > };\n>> >\n>> > } /* namespace RPi */\n>> > diff --git a/src/ipa/raspberrypi/cam_helper.cpp\n>> b/src/ipa/raspberrypi/cam_helper.cpp\n>> > index 6efa0d7f..018c17b9 100644\n>> > --- a/src/ipa/raspberrypi/cam_helper.cpp\n>> > +++ b/src/ipa/raspberrypi/cam_helper.cpp\n>> > @@ -34,8 +34,10 @@ CamHelper *CamHelper::Create(std::string const\n>> &cam_name)\n>> > return nullptr;\n>> > }\n>> >\n>> > -CamHelper::CamHelper(MdParser *parser)\n>> > - : parser_(parser), initialized_(false)\n>> > +CamHelper::CamHelper(MdParser *parser, unsigned int maxFrameLength,\n>> > + unsigned int frameIntegrationDiff)\n>> > + : parser_(parser), initialized_(false),\n>> maxFrameLength_(maxFrameLength),\n>> > + frameIntegrationDiff_(frameIntegrationDiff)\n>> > {\n>> > }\n>> >\n>> > @@ -56,6 +58,35 @@ double CamHelper::Exposure(uint32_t exposure_lines)\n>> const\n>> > return exposure_lines * mode_.line_length / 1000.0;\n>> > }\n>> >\n>> > +uint32_t CamHelper::GetVBlanking(double &exposure, double\n>> minFrameDuration,\n>>\n>> Output parameters should be really passed as pointers, it's very hard\n>> to follow what the caller does otherwise. I see the same usage of\n>> reference for ouput params is widly spread in the IPA, so I won't\n>> bother you asking for a change.\n>>\n>\n> Ack. There is a tidy-up task for our Controller that we need to get to at\n> some point.\n>\n>\n>>\n>> > + double maxFrameDuration) const\n>> > +{\n>> > + uint32_t frameLengthMin, frameLengthMax, vblank;\n>> > + uint32_t exposureLines = ExposureLines(exposure);\n>> > +\n>> > + assert(initialized_);\n>> > +\n>> > + /*\n>> > + * Clip frame length by the frame duration range and the maximum\n>> allowable\n>> > + * value in the sensor, given by maxFrameLength_.\n>> > + */\n>> > + frameLengthMin = std::clamp(1e3 * minFrameDuration /\n>> mode_.line_length,\n>> > + mode_.height,\n>> maxFrameLength_);\n>> > + frameLengthMax = std::clamp(1e3 * maxFrameDuration /\n>> mode_.line_length,\n>> > + mode_.height,\n>> maxFrameLength_);\n>> > + /*\n>> > + * Limit the exposure to the maximum frame duration requested, and\n>> > + * re-calculate if it has been clipped.\n>> > + */\n>> > + exposureLines = std::min(frameLengthMax - frameIntegrationDiff_,\n>> exposureLines);\n>> > + exposure = Exposure(exposureLines);\n>> > +\n>> > + /* Limit the vblank to the range allowed by the frame length\n>> limits. */\n>> > + vblank = std::clamp(exposureLines + frameIntegrationDiff_,\n>> > + frameLengthMin, frameLengthMax) -\n>> mode_.height;\n>>\n>> This looks good and matches the description of controls priorities\n>>\n>> > + return vblank;\n>> > +}\n>> > +\n>> > void CamHelper::SetCameraMode(const CameraMode &mode)\n>> > {\n>> > mode_ = mode;\n>> > diff --git a/src/ipa/raspberrypi/cam_helper.hpp\n>> b/src/ipa/raspberrypi/cam_helper.hpp\n>> > index 044c2866..b1739a57 100644\n>> > --- a/src/ipa/raspberrypi/cam_helper.hpp\n>> > +++ b/src/ipa/raspberrypi/cam_helper.hpp\n>> > @@ -62,12 +62,15 @@ class CamHelper\n>> > {\n>> > public:\n>> > static CamHelper *Create(std::string const &cam_name);\n>> > - CamHelper(MdParser *parser);\n>> > + CamHelper(MdParser *parser, unsigned int maxFrameLength,\n>> > + unsigned int frameIntegrationDiff);\n>> > virtual ~CamHelper();\n>> > void SetCameraMode(const CameraMode &mode);\n>> > MdParser &Parser() const { return *parser_; }\n>> > uint32_t ExposureLines(double exposure_us) const;\n>> > double Exposure(uint32_t exposure_lines) const; // in us\n>> > + virtual uint32_t GetVBlanking(double &exposure_us, double\n>> minFrameDuration,\n>> > + double maxFrameDuration) const;\n>>\n>> Is this really virtual ?\n>>\n>\n> Yes, I intentionally marked it as virtual. There will be some sensors\n> that use a different formula/method to calculate exposure time. In\n> particular, I am thinking about imx477 when we eventually get to enable\n> very long exposure modes (i.e. in order of minutes) where some extra\n> multipliers are used in the calculation. Additionally, global shutter\n> sensors are also ones that don't use a simple VTS * HTS type calculation.\n> Overriding this in the specific camera helper class will allow us to\n> account for these.\n>\n>\n>>\n>> > virtual uint32_t GainCode(double gain) const = 0;\n>> > virtual double Gain(uint32_t gain_code) const = 0;\n>> > virtual void GetDelays(int &exposure_delay, int &gain_delay)\n>> const;\n>> > @@ -76,10 +79,20 @@ public:\n>> > virtual unsigned int HideFramesModeSwitch() const;\n>> > virtual unsigned int MistrustFramesStartup() const;\n>> > virtual unsigned int MistrustFramesModeSwitch() const;\n>> > +\n>> > protected:\n>> > MdParser *parser_;\n>> > CameraMode mode_;\n>> > +\n>> > +private:\n>> > bool initialized_;\n>> > + /* Largest possible frame length, in units of lines. */\n>> > + unsigned int maxFrameLength_;\n>> > + /*\n>> > + * Smallest difference between the frame length and integration\n>> time,\n>> > + * in units of lines.\n>> > + */\n>> > + unsigned int frameIntegrationDiff_;\n>> > };\n>> >\n>> > // This is for registering camera helpers with the system, so that the\n>> > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n>> b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n>> > index db8ab879..8688ec09 100644\n>> > --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n>> > +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n>> > @@ -49,13 +49,22 @@ public:\n>> > double Gain(uint32_t gain_code) const override;\n>> > unsigned int MistrustFramesModeSwitch() const override;\n>> > bool SensorEmbeddedDataPresent() const override;\n>> > +\n>> > +private:\n>> > + /*\n>> > + * Smallest difference between the frame length and integration\n>> time,\n>> > + * in units of lines.\n>> > + */\n>> > + static constexpr int frameIntegrationDiff = 4;\n>> > + /* Largest possible frame length, in units of lines. */\n>> > + static constexpr int maxFrameLength = 0xffff;\n>> > };\n>> >\n>> > CamHelperImx219::CamHelperImx219()\n>> > #if ENABLE_EMBEDDED_DATA\n>> > - : CamHelper(new MdParserImx219())\n>> > + : CamHelper(new MdParserImx219(), maxFrameLength,\n>> frameIntegrationDiff)\n>> > #else\n>> > - : CamHelper(new MdParserRPi())\n>> > + : CamHelper(new MdParserRPi(), maxFrameLength,\n>> frameIntegrationDiff)\n>> > #endif\n>> > {\n>> > }\n>> > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n>> b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n>> > index 0e896ac7..53961310 100644\n>> > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n>> > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n>> > @@ -38,10 +38,19 @@ public:\n>> > uint32_t GainCode(double gain) const override;\n>> > double Gain(uint32_t gain_code) const override;\n>> > bool SensorEmbeddedDataPresent() const override;\n>> > +\n>> > +private:\n>> > + /*\n>> > + * Smallest difference between the frame length and integration\n>> time,\n>> > + * in units of lines.\n>> > + */\n>> > + static constexpr int frameIntegrationDiff = 22;\n>> > + /* Largest possible frame length, in units of lines. */\n>> > + static constexpr int maxFrameLength = 0xffdc;\n>> > };\n>> >\n>> > CamHelperImx477::CamHelperImx477()\n>> > - : CamHelper(new MdParserImx477())\n>> > + : CamHelper(new MdParserImx477(), maxFrameLength,\n>> frameIntegrationDiff)\n>> > {\n>> > }\n>> >\n>> > diff --git a/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n>> b/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n>> > index 0b841cd1..2bd8a754 100644\n>> > --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n>> > +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n>> > @@ -23,6 +23,15 @@ public:\n>> > unsigned int HideFramesModeSwitch() const override;\n>> > unsigned int MistrustFramesStartup() const override;\n>> > unsigned int MistrustFramesModeSwitch() const override;\n>> > +\n>> > +private:\n>> > + /*\n>> > + * Smallest difference between the frame length and integration\n>> time,\n>> > + * in units of lines.\n>> > + */\n>> > + static constexpr int frameIntegrationDiff = 4;\n>> > + /* Largest possible frame length, in units of lines. */\n>> > + static constexpr int maxFrameLength = 0xffff;\n>> > };\n>> >\n>> > /*\n>> > @@ -31,7 +40,7 @@ public:\n>> > */\n>> >\n>> > CamHelperOv5647::CamHelperOv5647()\n>> > - : CamHelper(new MdParserRPi())\n>> > + : CamHelper(new MdParserRPi(), maxFrameLength,\n>> frameIntegrationDiff)\n>> > {\n>> > }\n>> >\n>> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n>> b/src/ipa/raspberrypi/raspberrypi.cpp\n>> > index d087b07e..e1fe35f5 100644\n>> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n>> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n>> > @@ -58,6 +58,8 @@ namespace libcamera {\n>> > /* Configure the sensor with these values initially. */\n>> > constexpr double DefaultAnalogueGain = 1.0;\n>> > constexpr unsigned int DefaultExposureTime = 20000;\n>> > +constexpr double defaultMinFrameDuration = 1e6 / 30.0;\n>> > +constexpr double defaultMaxFrameDuration = 1e6 / 0.01;\n>>\n>> Do I count the number of 0s wrong, or the control maximum value is set\n>> to 10^9 and this is 10^8 ? Not much difference though both 100 or 1000\n>> seconds of max exposure should suit all needs :)\n>>\n>\n> Yes, they are different. This default value for the IPA is just a value I\n> picked, not strictly related to the Control max value. One could argue\n> that a more sensible number might be closer to around 500ms, but really\n> this gets clipped by the exposure mode profile anyway.\n>\n>\n>>\n>> These can be made uint_64 already to avoid casts. Up to you.\n>>\n>\n> I intentionally left them as doubles as our internal representation in the\n> Controller and IPA state uses doubles. As above, there is a tidy up task\n> to do in our controllers to rationalise this at some point.\n>\n>\n>>\n>> >\n>> > LOG_DEFINE_CATEGORY(IPARPI)\n>> >\n>> > @@ -150,6 +152,9 @@ private:\n>> >\n>> > /* Distinguish the first camera start from others. */\n>> > bool firstStart_;\n>> > +\n>> > + /* Frame duration (1/fps) given in microseconds. */\n>> > + double minFrameDuration_, maxFrameDuration_;\n>> > };\n>> >\n>> > int IPARPi::init(const IPASettings &settings)\n>> > @@ -332,7 +337,8 @@ void IPARPi::configure(const CameraSensorInfo\n>> &sensorInfo,\n>> > sensorMetadata = helper_->SensorEmbeddedDataPresent();\n>> >\n>> > result->data.push_back(gainDelay);\n>> > - result->data.push_back(exposureDelay);\n>> > + result->data.push_back(exposureDelay); /* FOR EXPOSURE\n>> ctrl */\n>>\n>> s/FOR/for/\n>>\n>\n> Ack.\n>\n>\n>>\n>> > + result->data.push_back(exposureDelay); /* For VBLANK ctrl\n>> */\n>>\n>> So, VBLANK and EXPOSURE ctrl have the same delay. How do we guarantee\n>> VBLANK is updated first ?\n>>\n>> > result->data.push_back(sensorMetadata);\n>> >\n>> > result->operation |= RPi::IPA_CONFIG_STAGGERED_WRITE;\n>> > @@ -382,6 +388,8 @@ void IPARPi::configure(const CameraSensorInfo\n>> &sensorInfo,\n>> > AgcStatus agcStatus;\n>> > agcStatus.shutter_time = DefaultExposureTime;\n>> > agcStatus.analogue_gain = DefaultAnalogueGain;\n>> > + minFrameDuration_ = defaultMinFrameDuration;\n>> > + maxFrameDuration_ = defaultMaxFrameDuration;\n>>\n>> I would initialize these before the ctrls declaration. These are not\n>> sent to the sensor, and seeing them in the middle here is confusing.\n>> Or are they needed by the applyAGC() function?\n>>\n>\n> They are needed by the applyAGC() function. However, I can move them\n> above the ctrl declaration to improve readability.\n>\n>\n>>\n>> > applyAGC(&agcStatus, ctrls);\n>> >\n>> > result->controls.emplace_back(ctrls);\n>> > @@ -524,7 +532,8 @@ bool IPARPi::validateSensorControls()\n>> > {\n>> > static const uint32_t ctrls[] = {\n>> > V4L2_CID_ANALOGUE_GAIN,\n>> > - V4L2_CID_EXPOSURE\n>> > + V4L2_CID_EXPOSURE,\n>> > + V4L2_CID_VBLANK\n>> > };\n>> >\n>> > for (auto c : ctrls) {\n>> > @@ -804,6 +813,18 @@ void IPARPi::queueRequest(const ControlList\n>> &controls)\n>> > break;\n>> > }\n>>\n>> Doesn't the SetFixedShutter() function need to be updated to clip the\n>> shutter time in the frameDuration limits too ? Can we address it or at\n>> least record it with a \\todo ?\n>>\n>\n> Yes, SetFixedShutter does need to account for frame duration limits. I do\n> have a set of patches that are waiting to be sent out for review that does\n> exactly this (along with passing the limits to the AGC algorithms).\n> Probably a \\todo is not worth it?\n>\n>\n>>\n>> >\n>> > + case controls::FRAME_DURATIONS: {\n>> > + auto frameDurations = ctrl.second.get> int64_t>>();\n>> > +\n>> > + /* This will be applied once AGC recalculations\n>> occur. */\n>> > + minFrameDuration_ = frameDurations[0] ?\n>> frameDurations[0] : defaultMinFrameDuration;\n>> > + maxFrameDuration_ = frameDurations[1] ?\n>> frameDurations[1] : defaultMaxFrameDuration;\n>>\n>> I'm surprised how cast from int64_t to double is automatically\n>> handled..\n>>\n>\n> Seems to compile without any warnings :-)\n>\n>\n>>\n>> > + libcameraMetadata_.set(controls::FrameDurations,\n>> > + {\n>> static_cast(minFrameDuration_),\n>> > +\n>> static_cast(maxFrameDuration_) });\n>>\n>> I don't think this matches the control description though:\n>>\n>> + When reported by pipelines, the control expresses the duration\n>> of the\n>> + sensor frame used to produce streams part of the completed\n>> Request.\n>> + The minimum and maximum values shall then be the same, as the\n>> sensor\n>> + frame duration is a fixed parameter.\n>>\n>> Should you record the exposure time calculated by GetVBlanking() and\n>> report it here ?\n>>\n>\n> Yes, this should be the \"adjusted\" frame durations. I do have that fix in\n> my next set of patches, but perhaps I apply them here instead? Note that I\n> do not use exposure directly to calculate this, rather I use the vblank\n> limits provided by the sensor. There is a sensor dependent offset between\n> exposure lines and VTS, so the numbers may be close, but not fully\n> accurate, when using exposure time to calculate frame durations.\n>\n\nI'm really sorry, my above comment is complete rubbish, please disregard it\n(but see below first) :-) I didn't fully understand your comment, I do now.\n\nDid you expect controls::FrameDurations to be updated on every frame, even\nif no custom frame durations was set? I suspect the answer would be yes,\notherwise it may not make much sense. However, we already return exposure\ntimes per frame through controls::ExposureTime in the metadata, and\ntogether with the buffer timestamps, this control in the metadata may seem\na bit redundant.\n\nI'm circling around a bit now, but my original intention for\ncontrols::FrameDurations returned in the metadata was to report the actual\nframe durations used, if they were clipped by the sensor mode limits (given\nby vblank min() and max() v4l2 ctrls). Would returning these clipped\nvalues back in metadata if a user set custom frame durations be more\nsuitable here? Or maybe not bother returning frame durations in the\nmetadata at all, as this is already conveyed in controls::ExposureTime?\n\nRegards,\nNaush\n\n\n\n>\n>\n>>\n>> > + break;\n>> > + }\n>> > +\n>> > default:\n>> > LOG(IPARPI, Warning)\n>> > << \"Ctrl \" << controls::controls.at\n>> (ctrl.first)->name()\n>> > @@ -962,15 +983,27 @@ void IPARPi::applyAWB(const struct AwbStatus\n>> *awbStatus, ControlList &ctrls)\n>> > void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList\n>> &ctrls)\n>> > {\n>> > int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);\n>> > - int32_t exposureLines =\n>> helper_->ExposureLines(agcStatus->shutter_time);\n>> >\n>> > - LOG(IPARPI, Debug) << \"Applying AGC Exposure: \" <<\n>> agcStatus->shutter_time\n>> > - << \" (Shutter lines: \" << exposureLines << \")\n>> Gain: \"\n>> > + /* GetVBlanking might clip exposure time to the fps limits. */\n>> > + double exposure = agcStatus->shutter_time;\n>> > + int32_t vblanking = helper_->GetVBlanking(exposure,\n>> minFrameDuration_,\n>> > + maxFrameDuration_);\n>> > + int32_t exposureLines = helper_->ExposureLines(exposure);\n>> > +\n>> > + LOG(IPARPI, Debug) << \"Applying AGC Exposure: \" << exposure\n>> > + << \" (Shutter lines: \" << exposureLines << \",\n>> AGC requested \"\n>> > + << agcStatus->shutter_time << \") Gain: \"\n>> > << agcStatus->analogue_gain << \" (Gain Code: \"\n>> > << gainCode << \")\";\n>> >\n>> > - ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);\n>> > + /*\n>> > + * Due to the behavior of V4L2, the current value of VBLANK could\n>> clip the\n>> > + * exposure time without us knowing. The next time though this\n>> function should\n>> > + * clip exposure correctly.\n>>\n>> Replying to myself: we skip one frame in case VBLANK clips exposure.\n>> I guess setting VBLANK one frame in advance compared to exposure won't\n>> help, as it could clip the previous frame. The only solution is to\n>> prioritize VBLANK when setting controls to the device I guess.\n>>\n>\n> In my next patch set, I address this by allowing the staggered write\n> controls to write VBLANK before any other controls. Depending on the\n> timing of DelayedControls being merged, I can easily port it if needed, or\n> just submit my change to staggered write.\n>\n>\n>>\n>> > + */\n>> > + ctrls.set(V4L2_CID_VBLANK, vblanking);\n>> > ctrls.set(V4L2_CID_EXPOSURE, exposureLines);\n>> > + ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);\n>> > }\n>> >\n>> > void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList\n>> &ctrls)\n>> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>> > index 7a5f5881..252cab64 100644\n>> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>> > @@ -1233,7 +1233,8 @@ int RPiCameraData::configureIPA(const\n>> CameraConfiguration *config)\n>> > if (!staggeredCtrl_) {\n>> > staggeredCtrl_.init(unicam_[Unicam::Image].dev(),\n>> > { { V4L2_CID_ANALOGUE_GAIN,\n>> result.data[resultIdx++] },\n>> > - { V4L2_CID_EXPOSURE,\n>> result.data[resultIdx++] } });\n>> > + { V4L2_CID_EXPOSURE,\n>> result.data[resultIdx++] },\n>> > + { V4L2_CID_VBLANK,\n>> result.data[resultIdx++] } });\n>>\n>> A few issues apart (metadata handling and manual exposure clipping\n>> apart) the rest is all minor stuff. With these fixed/clarified\n>>\n>\n> I will update with a v10 to address the metadata and typo. The manual\n> exposure clipping ought to be part of the next patch set as it is part of\n> the bigger change where the AGC applies the limits as well. Is that ok\n> with you?\n>\n> Regards,\n> Naush\n>\n>\n>\n>>\n>> Reviewed-by: Jacopo Mondi \n>>\n>> Thanks\n>> j\n>>\n>> > sensorMetadata_ = result.data[resultIdx++];\n>> > }\n>> > }\n>> > --\n>> > 2.25.1\n>> >\n>> > _______________________________________________\n>> > libcamera-devel mailing list\n>> > libcamera-devel@lists.libcamera.org\n>> > https://lists.libcamera.org/listinfo/libcamera-devel\n>>\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 67D5DBD80A\n\tfor ;\n\tThu, 17 Dec 2020 16:12:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DFD6461597;\n\tThu, 17 Dec 2020 17:12:25 +0100 (CET)","from mail-lf1-x135.google.com (mail-lf1-x135.google.com\n\t[IPv6:2a00:1450:4864:20::135])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DA84961591\n\tfor ;\n\tThu, 17 Dec 2020 17:12:24 +0100 (CET)","by mail-lf1-x135.google.com with SMTP id l11so58763462lfg.0\n\tfor ;\n\tThu, 17 Dec 2020 08:12:24 -0800 (PST)"],"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=\"bS5qjRTs\"; 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=+rmzRti5DXcQxhGD02TR/oTs9qzc4aJlWmGw845sdGo=;\n\tb=bS5qjRTsMN48KnrqVzW2AZalpZpgtzyeZaAmYfTUxOUUG+nhHVLTP0KAzi68+sjixL\n\tNg81SHZGZ254kQXNG8lILNiCc6TF3xjNBlSXJuYVglgYyT5VN3XbZtjtUXJNVs5jHewE\n\tP6GxAL4fxvl6FOLQ0p+E0ew7W7niGFSaldY892us/7VjGRguJVu1D59EDmcIp5alMiRI\n\tcfZnC1i0D4RlC6m7AmdjURvrpkGtkjZuhPAr1snuBuu77XLQ+zgTHgSAiiYyMLbMfsK2\n\tHx2LgqrBuDDUyc0mbiuQI2xawMnd22Nmlsxn8A/wg2TdK1QkDfOxPieB2O5+/h2BeyHI\n\tQIZA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=+rmzRti5DXcQxhGD02TR/oTs9qzc4aJlWmGw845sdGo=;\n\tb=L+LfWIrDuLd9JPDhaU8zmzGTxqC0LPu31jkbka1xS2tkp9Xb9VDJyb3VUEYCLtGBsB\n\t2lLs2dtsxYDNvFVjrMMwHcRsPPIFxsCjZHxInxKFQreqVm3vwyNiIZcR4dmP9Y+7TIg/\n\tYrOrok8nu9EtW8XLlaHrePUImMfzcpO2fE3wQD8pQdqrBFeaMXH5Pdqd6PlLcV6iSmUy\n\tyqyJKBYpHCJKmp7hMM4gpSuYJEwEoveufi3jgrtNfD9xdtBxMOmXiRgJVGSMcYT5Yn9O\n\ts2cHgHgc0f8Ecfac4Tc8AwszE5pE8KHyrPlnnJMEMGQKwaQs49JA0x8FXP6GRlLflBOP\n\tMKhA==","X-Gm-Message-State":"AOAM530gHxRmzZzUSjeTgWsboIy7pg6T6QP8zZIpZfvbkGBD3T3ek2ni\n\tvM/Jrces640iOPu1vPG2aCxp1Pn2YSJlyPbU49PK7G/8IZBJBQ==","X-Google-Smtp-Source":"ABdhPJwM5j/P655VjTDrnT3+zxL394Nj9ESswXkhnqVmbf4KLtnIjuvHphkG/F/GjAYRDeJbtFMfmJBV470wTKwvDWg=","X-Received":"by 2002:a05:651c:503:: with SMTP id\n\to3mr6520ljp.253.1608221544131; \n\tThu, 17 Dec 2020 08:12:24 -0800 (PST)","MIME-Version":"1.0","References":"<20201216112202.5063-1-naush@raspberrypi.com>\n\t<20201216112202.5063-3-naush@raspberrypi.com>\n\t<20201217151743.66m46drxyoszxmlq@uno.localdomain>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Thu, 17 Dec 2020 16:12:08 +0000","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v9 2/3] libcamera: raspberrypi: Add\n\tcontrol of sensor vblanking","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 ","Content-Type":"multipart/mixed;\n\tboundary=\"===============2379652244392851119==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14266,"web_url":"https://patchwork.libcamera.org/comment/14266/","msgid":"<20201217161857.7bsktytytmxjijut@uno.localdomain>","date":"2020-12-17T16:18:57","subject":"Re: [libcamera-devel] [PATCH v9 2/3] libcamera: raspberrypi: Add\n\tcontrol of sensor vblanking","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Naush\n\nOn Thu, Dec 17, 2020 at 03:49:22PM +0000, Naushir Patuck wrote:\n> Hi Jacopo,\n>\n> Thank you for your review comments.\n>\n> On Thu, 17 Dec 2020 at 15:17, Jacopo Mondi wrote:\n>\n> > Hi Naush,\n> >\n> > a few minors here and there\n> >\n> > On Wed, Dec 16, 2020 at 11:22:01AM +0000, Naushir Patuck wrote:\n> > > Add support for setting V4L2_CID_VBLANK appropriately when setting\n> > > V4L2_CID_EXPOSURE. This will allow adaptive framerates during\n> > > viewfinder use cases (e.g. when the exposure time goes above 33ms, we\n> > > can reduce the framerate to lower than 30fps).\n> > >\n> > > The minimum and maximum frame durations are provided via libcamera\n> > > controls, and will prioritise exposure time limits over any AGC request.\n> > >\n> > > V4L2_CID_VBLANK is controlled through the staggered writer, just like\n> > > the exposure and gain controls.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > Reviewed-by: David Plowman \n> > > Tested-by: David Plowman \n> > > ---\n> > > include/libcamera/ipa/raspberrypi.h | 1 +\n> > > src/ipa/raspberrypi/cam_helper.cpp | 35 ++++++++++++++-\n> > > src/ipa/raspberrypi/cam_helper.hpp | 15 ++++++-\n> > > src/ipa/raspberrypi/cam_helper_imx219.cpp | 13 +++++-\n> > > src/ipa/raspberrypi/cam_helper_imx477.cpp | 11 ++++-\n> > > src/ipa/raspberrypi/cam_helper_ov5647.cpp | 11 ++++-\n> > > src/ipa/raspberrypi/raspberrypi.cpp | 45 ++++++++++++++++---\n> > > .../pipeline/raspberrypi/raspberrypi.cpp | 3 +-\n> > > 8 files changed, 120 insertions(+), 14 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/ipa/raspberrypi.h\n> > b/include/libcamera/ipa/raspberrypi.h\n> > > index 01fe5abc..1de36039 100644\n> > > --- a/include/libcamera/ipa/raspberrypi.h\n> > > +++ b/include/libcamera/ipa/raspberrypi.h\n> > > @@ -65,6 +65,7 @@ static const ControlInfoMap Controls = {\n> > > { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> > > { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n> > > { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535,\n> > 65535, 65535, 65535), Rectangle{}) },\n> > > + { &controls::FrameDurations, ControlInfo(1000, 1000000000) },\n> > > };\n> > >\n> > > } /* namespace RPi */\n> > > diff --git a/src/ipa/raspberrypi/cam_helper.cpp\n> > b/src/ipa/raspberrypi/cam_helper.cpp\n> > > index 6efa0d7f..018c17b9 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper.cpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> > > @@ -34,8 +34,10 @@ CamHelper *CamHelper::Create(std::string const\n> > &cam_name)\n> > > return nullptr;\n> > > }\n> > >\n> > > -CamHelper::CamHelper(MdParser *parser)\n> > > - : parser_(parser), initialized_(false)\n> > > +CamHelper::CamHelper(MdParser *parser, unsigned int maxFrameLength,\n> > > + unsigned int frameIntegrationDiff)\n> > > + : parser_(parser), initialized_(false),\n> > maxFrameLength_(maxFrameLength),\n> > > + frameIntegrationDiff_(frameIntegrationDiff)\n> > > {\n> > > }\n> > >\n> > > @@ -56,6 +58,35 @@ double CamHelper::Exposure(uint32_t exposure_lines)\n> > const\n> > > return exposure_lines * mode_.line_length / 1000.0;\n> > > }\n> > >\n> > > +uint32_t CamHelper::GetVBlanking(double &exposure, double\n> > minFrameDuration,\n> >\n> > Output parameters should be really passed as pointers, it's very hard\n> > to follow what the caller does otherwise. I see the same usage of\n> > reference for ouput params is widly spread in the IPA, so I won't\n> > bother you asking for a change.\n> >\n>\n> Ack. There is a tidy-up task for our Controller that we need to get to at\n> some point.\n>\n>\n> >\n> > > + double maxFrameDuration) const\n> > > +{\n> > > + uint32_t frameLengthMin, frameLengthMax, vblank;\n> > > + uint32_t exposureLines = ExposureLines(exposure);\n> > > +\n> > > + assert(initialized_);\n> > > +\n> > > + /*\n> > > + * Clip frame length by the frame duration range and the maximum\n> > allowable\n> > > + * value in the sensor, given by maxFrameLength_.\n> > > + */\n> > > + frameLengthMin = std::clamp(1e3 * minFrameDuration /\n> > mode_.line_length,\n> > > + mode_.height,\n> > maxFrameLength_);\n> > > + frameLengthMax = std::clamp(1e3 * maxFrameDuration /\n> > mode_.line_length,\n> > > + mode_.height,\n> > maxFrameLength_);\n> > > + /*\n> > > + * Limit the exposure to the maximum frame duration requested, and\n> > > + * re-calculate if it has been clipped.\n> > > + */\n> > > + exposureLines = std::min(frameLengthMax - frameIntegrationDiff_,\n> > exposureLines);\n> > > + exposure = Exposure(exposureLines);\n> > > +\n> > > + /* Limit the vblank to the range allowed by the frame length\n> > limits. */\n> > > + vblank = std::clamp(exposureLines + frameIntegrationDiff_,\n> > > + frameLengthMin, frameLengthMax) - mode_.height;\n> >\n> > This looks good and matches the description of controls priorities\n> >\n> > > + return vblank;\n> > > +}\n> > > +\n> > > void CamHelper::SetCameraMode(const CameraMode &mode)\n> > > {\n> > > mode_ = mode;\n> > > diff --git a/src/ipa/raspberrypi/cam_helper.hpp\n> > b/src/ipa/raspberrypi/cam_helper.hpp\n> > > index 044c2866..b1739a57 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper.hpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> > > @@ -62,12 +62,15 @@ class CamHelper\n> > > {\n> > > public:\n> > > static CamHelper *Create(std::string const &cam_name);\n> > > - CamHelper(MdParser *parser);\n> > > + CamHelper(MdParser *parser, unsigned int maxFrameLength,\n> > > + unsigned int frameIntegrationDiff);\n> > > virtual ~CamHelper();\n> > > void SetCameraMode(const CameraMode &mode);\n> > > MdParser &Parser() const { return *parser_; }\n> > > uint32_t ExposureLines(double exposure_us) const;\n> > > double Exposure(uint32_t exposure_lines) const; // in us\n> > > + virtual uint32_t GetVBlanking(double &exposure_us, double\n> > minFrameDuration,\n> > > + double maxFrameDuration) const;\n> >\n> > Is this really virtual ?\n> >\n>\n> Yes, I intentionally marked it as virtual. There will be some sensors that\n> use a different formula/method to calculate exposure time. In particular,\n> I am thinking about imx477 when we eventually get to enable very long\n> exposure modes (i.e. in order of minutes) where some extra multipliers are\n> used in the calculation. Additionally, global shutter sensors are also\n> ones that don't use a simple VTS * HTS type calculation. Overriding this\n> in the specific camera helper class will allow us to account for these.\n>\n>\n> >\n> > > virtual uint32_t GainCode(double gain) const = 0;\n> > > virtual double Gain(uint32_t gain_code) const = 0;\n> > > virtual void GetDelays(int &exposure_delay, int &gain_delay) const;\n> > > @@ -76,10 +79,20 @@ public:\n> > > virtual unsigned int HideFramesModeSwitch() const;\n> > > virtual unsigned int MistrustFramesStartup() const;\n> > > virtual unsigned int MistrustFramesModeSwitch() const;\n> > > +\n> > > protected:\n> > > MdParser *parser_;\n> > > CameraMode mode_;\n> > > +\n> > > +private:\n> > > bool initialized_;\n> > > + /* Largest possible frame length, in units of lines. */\n> > > + unsigned int maxFrameLength_;\n> > > + /*\n> > > + * Smallest difference between the frame length and integration\n> > time,\n> > > + * in units of lines.\n> > > + */\n> > > + unsigned int frameIntegrationDiff_;\n> > > };\n> > >\n> > > // This is for registering camera helpers with the system, so that the\n> > > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > > index db8ab879..8688ec09 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > > @@ -49,13 +49,22 @@ public:\n> > > double Gain(uint32_t gain_code) const override;\n> > > unsigned int MistrustFramesModeSwitch() const override;\n> > > bool SensorEmbeddedDataPresent() const override;\n> > > +\n> > > +private:\n> > > + /*\n> > > + * Smallest difference between the frame length and integration\n> > time,\n> > > + * in units of lines.\n> > > + */\n> > > + static constexpr int frameIntegrationDiff = 4;\n> > > + /* Largest possible frame length, in units of lines. */\n> > > + static constexpr int maxFrameLength = 0xffff;\n> > > };\n> > >\n> > > CamHelperImx219::CamHelperImx219()\n> > > #if ENABLE_EMBEDDED_DATA\n> > > - : CamHelper(new MdParserImx219())\n> > > + : CamHelper(new MdParserImx219(), maxFrameLength,\n> > frameIntegrationDiff)\n> > > #else\n> > > - : CamHelper(new MdParserRPi())\n> > > + : CamHelper(new MdParserRPi(), maxFrameLength,\n> > frameIntegrationDiff)\n> > > #endif\n> > > {\n> > > }\n> > > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > > index 0e896ac7..53961310 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > > @@ -38,10 +38,19 @@ public:\n> > > uint32_t GainCode(double gain) const override;\n> > > double Gain(uint32_t gain_code) const override;\n> > > bool SensorEmbeddedDataPresent() const override;\n> > > +\n> > > +private:\n> > > + /*\n> > > + * Smallest difference between the frame length and integration\n> > time,\n> > > + * in units of lines.\n> > > + */\n> > > + static constexpr int frameIntegrationDiff = 22;\n> > > + /* Largest possible frame length, in units of lines. */\n> > > + static constexpr int maxFrameLength = 0xffdc;\n> > > };\n> > >\n> > > CamHelperImx477::CamHelperImx477()\n> > > - : CamHelper(new MdParserImx477())\n> > > + : CamHelper(new MdParserImx477(), maxFrameLength,\n> > frameIntegrationDiff)\n> > > {\n> > > }\n> > >\n> > > diff --git a/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> > b/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> > > index 0b841cd1..2bd8a754 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> > > @@ -23,6 +23,15 @@ public:\n> > > unsigned int HideFramesModeSwitch() const override;\n> > > unsigned int MistrustFramesStartup() const override;\n> > > unsigned int MistrustFramesModeSwitch() const override;\n> > > +\n> > > +private:\n> > > + /*\n> > > + * Smallest difference between the frame length and integration\n> > time,\n> > > + * in units of lines.\n> > > + */\n> > > + static constexpr int frameIntegrationDiff = 4;\n> > > + /* Largest possible frame length, in units of lines. */\n> > > + static constexpr int maxFrameLength = 0xffff;\n> > > };\n> > >\n> > > /*\n> > > @@ -31,7 +40,7 @@ public:\n> > > */\n> > >\n> > > CamHelperOv5647::CamHelperOv5647()\n> > > - : CamHelper(new MdParserRPi())\n> > > + : CamHelper(new MdParserRPi(), maxFrameLength,\n> > frameIntegrationDiff)\n> > > {\n> > > }\n> > >\n> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> > b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > index d087b07e..e1fe35f5 100644\n> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > @@ -58,6 +58,8 @@ namespace libcamera {\n> > > /* Configure the sensor with these values initially. */\n> > > constexpr double DefaultAnalogueGain = 1.0;\n> > > constexpr unsigned int DefaultExposureTime = 20000;\n> > > +constexpr double defaultMinFrameDuration = 1e6 / 30.0;\n> > > +constexpr double defaultMaxFrameDuration = 1e6 / 0.01;\n> >\n> > Do I count the number of 0s wrong, or the control maximum value is set\n> > to 10^9 and this is 10^8 ? Not much difference though both 100 or 1000\n> > seconds of max exposure should suit all needs :)\n> >\n>\n> Yes, they are different. This default value for the IPA is just a value I\n> picked, not strictly related to the Control max value. One could argue\n> that a more sensible number might be closer to around 500ms, but really\n> this gets clipped by the exposure mode profile anyway.\n>\n>\n> >\n> > These can be made uint_64 already to avoid casts. Up to you.\n> >\n>\n> I intentionally left them as doubles as our internal representation in the\n> Controller and IPA state uses doubles. As above, there is a tidy up task\n> to do in our controllers to rationalise this at some point.\n>\n>\n> >\n> > >\n> > > LOG_DEFINE_CATEGORY(IPARPI)\n> > >\n> > > @@ -150,6 +152,9 @@ private:\n> > >\n> > > /* Distinguish the first camera start from others. */\n> > > bool firstStart_;\n> > > +\n> > > + /* Frame duration (1/fps) given in microseconds. */\n> > > + double minFrameDuration_, maxFrameDuration_;\n> > > };\n> > >\n> > > int IPARPi::init(const IPASettings &settings)\n> > > @@ -332,7 +337,8 @@ void IPARPi::configure(const CameraSensorInfo\n> > &sensorInfo,\n> > > sensorMetadata = helper_->SensorEmbeddedDataPresent();\n> > >\n> > > result->data.push_back(gainDelay);\n> > > - result->data.push_back(exposureDelay);\n> > > + result->data.push_back(exposureDelay); /* FOR EXPOSURE\n> > ctrl */\n> >\n> > s/FOR/for/\n> >\n>\n> Ack.\n>\n>\n> >\n> > > + result->data.push_back(exposureDelay); /* For VBLANK ctrl\n> > */\n> >\n> > So, VBLANK and EXPOSURE ctrl have the same delay. How do we guarantee\n> > VBLANK is updated first ?\n> >\n> > > result->data.push_back(sensorMetadata);\n> > >\n> > > result->operation |= RPi::IPA_CONFIG_STAGGERED_WRITE;\n> > > @@ -382,6 +388,8 @@ void IPARPi::configure(const CameraSensorInfo\n> > &sensorInfo,\n> > > AgcStatus agcStatus;\n> > > agcStatus.shutter_time = DefaultExposureTime;\n> > > agcStatus.analogue_gain = DefaultAnalogueGain;\n> > > + minFrameDuration_ = defaultMinFrameDuration;\n> > > + maxFrameDuration_ = defaultMaxFrameDuration;\n> >\n> > I would initialize these before the ctrls declaration. These are not\n> > sent to the sensor, and seeing them in the middle here is confusing.\n> > Or are they needed by the applyAGC() function?\n> >\n>\n> They are needed by the applyAGC() function. However, I can move them above\n> the ctrl declaration to improve readability.\n>\n>\n> >\n> > > applyAGC(&agcStatus, ctrls);\n> > >\n> > > result->controls.emplace_back(ctrls);\n> > > @@ -524,7 +532,8 @@ bool IPARPi::validateSensorControls()\n> > > {\n> > > static const uint32_t ctrls[] = {\n> > > V4L2_CID_ANALOGUE_GAIN,\n> > > - V4L2_CID_EXPOSURE\n> > > + V4L2_CID_EXPOSURE,\n> > > + V4L2_CID_VBLANK\n> > > };\n> > >\n> > > for (auto c : ctrls) {\n> > > @@ -804,6 +813,18 @@ void IPARPi::queueRequest(const ControlList\n> > &controls)\n> > > break;\n> > > }\n> >\n> > Doesn't the SetFixedShutter() function need to be updated to clip the\n> > shutter time in the frameDuration limits too ? Can we address it or at\n> > least record it with a \\todo ?\n> >\n>\n> Yes, SetFixedShutter does need to account for frame duration limits. I do\n> have a set of patches that are waiting to be sent out for review that does\n> exactly this (along with passing the limits to the AGC algorithms).\n> Probably a \\todo is not worth it?\n>\n>\n> >\n> > >\n> > > + case controls::FRAME_DURATIONS: {\n> > > + auto frameDurations = ctrl.second.get > int64_t>>();\n> > > +\n> > > + /* This will be applied once AGC recalculations\n> > occur. */\n> > > + minFrameDuration_ = frameDurations[0] ?\n> > frameDurations[0] : defaultMinFrameDuration;\n> > > + maxFrameDuration_ = frameDurations[1] ?\n> > frameDurations[1] : defaultMaxFrameDuration;\n> >\n> > I'm surprised how cast from int64_t to double is automatically\n> > handled..\n> >\n>\n> Seems to compile without any warnings :-)\n>\n>\n> >\n> > > + libcameraMetadata_.set(controls::FrameDurations,\n> > > + {\n> > static_cast(minFrameDuration_),\n> > > +\n> > static_cast(maxFrameDuration_) });\n> >\n> > I don't think this matches the control description though:\n> >\n> > + When reported by pipelines, the control expresses the duration\n> > of the\n> > + sensor frame used to produce streams part of the completed\n> > Request.\n> > + The minimum and maximum values shall then be the same, as the\n> > sensor\n> > + frame duration is a fixed parameter.\n> >\n> > Should you record the exposure time calculated by GetVBlanking() and\n> > report it here ?\n> >\n>\n> Yes, this should be the \"adjusted\" frame durations. I do have that fix in\n> my next set of patches, but perhaps I apply them here instead? Note that I\n> do not use exposure directly to calculate this, rather I use the vblank\n> limits provided by the sensor. There is a sensor dependent offset between\n> exposure lines and VTS, so the numbers may be close, but not fully\n> accurate, when using exposure time to calculate frame durations.\n>\n>\n> >\n> > > + break;\n> > > + }\n> > > +\n> > > default:\n> > > LOG(IPARPI, Warning)\n> > > << \"Ctrl \" << controls::controls.at\n> > (ctrl.first)->name()\n> > > @@ -962,15 +983,27 @@ void IPARPi::applyAWB(const struct AwbStatus\n> > *awbStatus, ControlList &ctrls)\n> > > void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList\n> > &ctrls)\n> > > {\n> > > int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);\n> > > - int32_t exposureLines =\n> > helper_->ExposureLines(agcStatus->shutter_time);\n> > >\n> > > - LOG(IPARPI, Debug) << \"Applying AGC Exposure: \" <<\n> > agcStatus->shutter_time\n> > > - << \" (Shutter lines: \" << exposureLines << \")\n> > Gain: \"\n> > > + /* GetVBlanking might clip exposure time to the fps limits. */\n> > > + double exposure = agcStatus->shutter_time;\n> > > + int32_t vblanking = helper_->GetVBlanking(exposure,\n> > minFrameDuration_,\n> > > + maxFrameDuration_);\n> > > + int32_t exposureLines = helper_->ExposureLines(exposure);\n> > > +\n> > > + LOG(IPARPI, Debug) << \"Applying AGC Exposure: \" << exposure\n> > > + << \" (Shutter lines: \" << exposureLines << \",\n> > AGC requested \"\n> > > + << agcStatus->shutter_time << \") Gain: \"\n> > > << agcStatus->analogue_gain << \" (Gain Code: \"\n> > > << gainCode << \")\";\n> > >\n> > > - ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);\n> > > + /*\n> > > + * Due to the behavior of V4L2, the current value of VBLANK could\n> > clip the\n> > > + * exposure time without us knowing. The next time though this\n> > function should\n> > > + * clip exposure correctly.\n> >\n> > Replying to myself: we skip one frame in case VBLANK clips exposure.\n> > I guess setting VBLANK one frame in advance compared to exposure won't\n> > help, as it could clip the previous frame. The only solution is to\n> > prioritize VBLANK when setting controls to the device I guess.\n> >\n>\n> In my next patch set, I address this by allowing the staggered write\n> controls to write VBLANK before any other controls. Depending on the\n> timing of DelayedControls being merged, I can easily port it if needed, or\n> just submit my change to staggered write.\n>\n>\n> >\n> > > + */\n> > > + ctrls.set(V4L2_CID_VBLANK, vblanking);\n> > > ctrls.set(V4L2_CID_EXPOSURE, exposureLines);\n> > > + ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);\n> > > }\n> > >\n> > > void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList\n> > &ctrls)\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index 7a5f5881..252cab64 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -1233,7 +1233,8 @@ int RPiCameraData::configureIPA(const\n> > CameraConfiguration *config)\n> > > if (!staggeredCtrl_) {\n> > > staggeredCtrl_.init(unicam_[Unicam::Image].dev(),\n> > > { { V4L2_CID_ANALOGUE_GAIN,\n> > result.data[resultIdx++] },\n> > > - { V4L2_CID_EXPOSURE,\n> > result.data[resultIdx++] } });\n> > > + { V4L2_CID_EXPOSURE,\n> > result.data[resultIdx++] },\n> > > + { V4L2_CID_VBLANK,\n> > result.data[resultIdx++] } });\n> >\n> > A few issues apart (metadata handling and manual exposure clipping\n> > apart) the rest is all minor stuff. With these fixed/clarified\n> >\n>\n> I will update with a v10 to address the metadata and typo. The manual\n> exposure clipping ought to be part of the next patch set as it is part of\n> the bigger change where the AGC applies the limits as well. Is that ok\n> with you?\n\nIndeed, I trust that you want that addressed as much as I do :)\n\nCheers\n\n>\n> Regards,\n> Naush\n>\n>\n>\n> >\n> > Reviewed-by: Jacopo Mondi \n> >\n> > Thanks\n> > j\n> >\n> > > sensorMetadata_ = result.data[resultIdx++];\n> > > }\n> > > }\n> > > --\n> > > 2.25.1\n> > >\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\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 81E8EC0F1A\n\tfor ;\n\tThu, 17 Dec 2020 16:18:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2367061595;\n\tThu, 17 Dec 2020 17:18:48 +0100 (CET)","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 D1FE961591\n\tfor ;\n\tThu, 17 Dec 2020 17:18:46 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 39A451C0007;\n\tThu, 17 Dec 2020 16:18:45 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Thu, 17 Dec 2020 17:18:57 +0100","From":"Jacopo Mondi ","To":"Naushir Patuck ","Message-ID":"<20201217161857.7bsktytytmxjijut@uno.localdomain>","References":"<20201216112202.5063-1-naush@raspberrypi.com>\n\t<20201216112202.5063-3-naush@raspberrypi.com>\n\t<20201217151743.66m46drxyoszxmlq@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v9 2/3] libcamera: raspberrypi: Add\n\tcontrol of sensor vblanking","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14267,"web_url":"https://patchwork.libcamera.org/comment/14267/","msgid":"<20201217162548.6agmstiv7mfjztsg@uno.localdomain>","date":"2020-12-17T16:25:48","subject":"Re: [libcamera-devel] [PATCH v9 2/3] libcamera: raspberrypi: Add\n\tcontrol of sensor vblanking","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hello,\n\nOn Thu, Dec 17, 2020 at 04:12:08PM +0000, Naushir Patuck wrote:\n> Hi Jacopo,\n> >>\n> >> > + libcameraMetadata_.set(controls::FrameDurations,\n> >> > + {\n> >> static_cast(minFrameDuration_),\n> >> > +\n> >> static_cast(maxFrameDuration_) });\n> >>\n> >> I don't think this matches the control description though:\n> >>\n> >> + When reported by pipelines, the control expresses the duration\n> >> of the\n> >> + sensor frame used to produce streams part of the completed\n> >> Request.\n> >> + The minimum and maximum values shall then be the same, as the\n> >> sensor\n> >> + frame duration is a fixed parameter.\n> >>\n> >> Should you record the exposure time calculated by GetVBlanking() and\n> >> report it here ?\n> >>\n> >\n> > Yes, this should be the \"adjusted\" frame durations. I do have that fix in\n> > my next set of patches, but perhaps I apply them here instead? Note that I\n> > do not use exposure directly to calculate this, rather I use the vblank\n> > limits provided by the sensor. There is a sensor dependent offset between\n> > exposure lines and VTS, so the numbers may be close, but not fully\n> > accurate, when using exposure time to calculate frame durations.\n> >\n>\n> I'm really sorry, my above comment is complete rubbish, please disregard it\n> (but see below first) :-) I didn't fully understand your comment, I do now.\n>\n> Did you expect controls::FrameDurations to be updated on every frame, even\n> if no custom frame durations was set? I suspect the answer would be yes,\n> otherwise it may not make much sense. However, we already return exposure\n> times per frame through controls::ExposureTime in the metadata, and\n> together with the buffer timestamps, this control in the metadata may seem\n> a bit redundant.\n\nI was expecting this to be returned per-frame yes.\n\n>\n> I'm circling around a bit now, but my original intention for\n> controls::FrameDurations returned in the metadata was to report the actual\n> frame durations used, if they were clipped by the sensor mode limits (given\n> by vblank min() and max() v4l2 ctrls). Would returning these clipped\n> values back in metadata if a user set custom frame durations be more\n> suitable here? Or maybe not bother returning frame durations in the\n> metadata at all, as this is already conveyed in controls::ExposureTime?\n\nMmm, as ExposureTime is directly connected to manual exposure setting,\nI was expecting it to be returned only when the application provides\nit. You seem to thinkg the same about FrameDuration :)\n\nTo be honest, my preference of having FrameDuration returned\nunconditionally is only because ExposureTime has a relationship with\nthe AEG routine being enabled or not. That's a weak preference though,\nand if you have other reasons to go in one direction or another I've no\nstrong preference.\n\n>\n> Regards,\n> Naush\n>\n>\n>\n> >\n> >\n> >>\n> >> > + break;\n> >> > + }\n> >> > +\n> >> > default:\n> >> > LOG(IPARPI, Warning)\n> >> > << \"Ctrl \" << controls::controls.at\n> >> (ctrl.first)->name()\n> >> > @@ -962,15 +983,27 @@ void IPARPi::applyAWB(const struct AwbStatus\n> >> *awbStatus, ControlList &ctrls)\n> >> > void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList\n> >> &ctrls)\n> >> > {\n> >> > int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);\n> >> > - int32_t exposureLines =\n> >> helper_->ExposureLines(agcStatus->shutter_time);\n> >> >\n> >> > - LOG(IPARPI, Debug) << \"Applying AGC Exposure: \" <<\n> >> agcStatus->shutter_time\n> >> > - << \" (Shutter lines: \" << exposureLines << \")\n> >> Gain: \"\n> >> > + /* GetVBlanking might clip exposure time to the fps limits. */\n> >> > + double exposure = agcStatus->shutter_time;\n> >> > + int32_t vblanking = helper_->GetVBlanking(exposure,\n> >> minFrameDuration_,\n> >> > + maxFrameDuration_);\n> >> > + int32_t exposureLines = helper_->ExposureLines(exposure);\n> >> > +\n> >> > + LOG(IPARPI, Debug) << \"Applying AGC Exposure: \" << exposure\n> >> > + << \" (Shutter lines: \" << exposureLines << \",\n> >> AGC requested \"\n> >> > + << agcStatus->shutter_time << \") Gain: \"\n> >> > << agcStatus->analogue_gain << \" (Gain Code: \"\n> >> > << gainCode << \")\";\n> >> >\n> >> > - ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);\n> >> > + /*\n> >> > + * Due to the behavior of V4L2, the current value of VBLANK could\n> >> clip the\n> >> > + * exposure time without us knowing. The next time though this\n> >> function should\n> >> > + * clip exposure correctly.\n> >>\n> >> Replying to myself: we skip one frame in case VBLANK clips exposure.\n> >> I guess setting VBLANK one frame in advance compared to exposure won't\n> >> help, as it could clip the previous frame. The only solution is to\n> >> prioritize VBLANK when setting controls to the device I guess.\n> >>\n> >\n> > In my next patch set, I address this by allowing the staggered write\n> > controls to write VBLANK before any other controls. Depending on the\n> > timing of DelayedControls being merged, I can easily port it if needed, or\n> > just submit my change to staggered write.\n> >\n> >\n> >>\n> >> > + */\n> >> > + ctrls.set(V4L2_CID_VBLANK, vblanking);\n> >> > ctrls.set(V4L2_CID_EXPOSURE, exposureLines);\n> >> > + ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);\n> >> > }\n> >> >\n> >> > void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList\n> >> &ctrls)\n> >> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >> > index 7a5f5881..252cab64 100644\n> >> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> >> > @@ -1233,7 +1233,8 @@ int RPiCameraData::configureIPA(const\n> >> CameraConfiguration *config)\n> >> > if (!staggeredCtrl_) {\n> >> > staggeredCtrl_.init(unicam_[Unicam::Image].dev(),\n> >> > { { V4L2_CID_ANALOGUE_GAIN,\n> >> result.data[resultIdx++] },\n> >> > - { V4L2_CID_EXPOSURE,\n> >> result.data[resultIdx++] } });\n> >> > + { V4L2_CID_EXPOSURE,\n> >> result.data[resultIdx++] },\n> >> > + { V4L2_CID_VBLANK,\n> >> result.data[resultIdx++] } });\n> >>\n> >> A few issues apart (metadata handling and manual exposure clipping\n> >> apart) the rest is all minor stuff. With these fixed/clarified\n> >>\n> >\n> > I will update with a v10 to address the metadata and typo. The manual\n> > exposure clipping ought to be part of the next patch set as it is part of\n> > the bigger change where the AGC applies the limits as well. Is that ok\n> > with you?\n> >\n> > Regards,\n> > Naush\n> >\n> >\n> >\n> >>\n> >> Reviewed-by: Jacopo Mondi \n> >>\n> >> Thanks\n> >> j\n> >>\n> >> > sensorMetadata_ = result.data[resultIdx++];\n> >> > }\n> >> > }\n> >> > --\n> >> > 2.25.1\n> >> >\n> >> > _______________________________________________\n> >> > libcamera-devel mailing list\n> >> > libcamera-devel@lists.libcamera.org\n> >> > https://lists.libcamera.org/listinfo/libcamera-devel\n> >>\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 4873EBD80A\n\tfor ;\n\tThu, 17 Dec 2020 16:25:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C47E961597;\n\tThu, 17 Dec 2020 17:25:38 +0100 (CET)","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 A9F6361591\n\tfor ;\n\tThu, 17 Dec 2020 17:25:37 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 204F41C0015;\n\tThu, 17 Dec 2020 16:25:36 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Thu, 17 Dec 2020 17:25:48 +0100","From":"Jacopo Mondi ","To":"Naushir Patuck ","Message-ID":"<20201217162548.6agmstiv7mfjztsg@uno.localdomain>","References":"<20201216112202.5063-1-naush@raspberrypi.com>\n\t<20201216112202.5063-3-naush@raspberrypi.com>\n\t<20201217151743.66m46drxyoszxmlq@uno.localdomain>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v9 2/3] libcamera: raspberrypi: Add\n\tcontrol of sensor vblanking","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14268,"web_url":"https://patchwork.libcamera.org/comment/14268/","msgid":"","date":"2020-12-17T16:35:27","subject":"Re: [libcamera-devel] [PATCH v9 2/3] libcamera: raspberrypi: Add\n\tcontrol of sensor vblanking","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Jacopo,\n\nOn Thu, 17 Dec 2020 at 16:25, Jacopo Mondi wrote:\n\n> Hello,\n>\n> On Thu, Dec 17, 2020 at 04:12:08PM +0000, Naushir Patuck wrote:\n> > Hi Jacopo,\n> > >>\n> > >> > +\n> libcameraMetadata_.set(controls::FrameDurations,\n> > >> > + {\n> > >> static_cast(minFrameDuration_),\n> > >> > +\n> > >> static_cast(maxFrameDuration_) });\n> > >>\n> > >> I don't think this matches the control description though:\n> > >>\n> > >> + When reported by pipelines, the control expresses the\n> duration\n> > >> of the\n> > >> + sensor frame used to produce streams part of the completed\n> > >> Request.\n> > >> + The minimum and maximum values shall then be the same, as\n> the\n> > >> sensor\n> > >> + frame duration is a fixed parameter.\n> > >>\n> > >> Should you record the exposure time calculated by GetVBlanking() and\n> > >> report it here ?\n> > >>\n> > >\n> > > Yes, this should be the \"adjusted\" frame durations. I do have that fix\n> in\n> > > my next set of patches, but perhaps I apply them here instead? Note\n> that I\n> > > do not use exposure directly to calculate this, rather I use the vblank\n> > > limits provided by the sensor. There is a sensor dependent offset\n> between\n> > > exposure lines and VTS, so the numbers may be close, but not fully\n> > > accurate, when using exposure time to calculate frame durations.\n> > >\n> >\n> > I'm really sorry, my above comment is complete rubbish, please disregard\n> it\n> > (but see below first) :-) I didn't fully understand your comment, I do\n> now.\n> >\n> > Did you expect controls::FrameDurations to be updated on every frame,\n> even\n> > if no custom frame durations was set? I suspect the answer would be yes,\n> > otherwise it may not make much sense. However, we already return\n> exposure\n> > times per frame through controls::ExposureTime in the metadata, and\n> > together with the buffer timestamps, this control in the metadata may\n> seem\n> > a bit redundant.\n>\n> I was expecting this to be returned per-frame yes.\n>\n> >\n> > I'm circling around a bit now, but my original intention for\n> > controls::FrameDurations returned in the metadata was to report the\n> actual\n> > frame durations used, if they were clipped by the sensor mode limits\n> (given\n> > by vblank min() and max() v4l2 ctrls). Would returning these clipped\n> > values back in metadata if a user set custom frame durations be more\n> > suitable here? Or maybe not bother returning frame durations in the\n> > metadata at all, as this is already conveyed in controls::ExposureTime?\n>\n> Mmm, as ExposureTime is directly connected to manual exposure setting,\n> I was expecting it to be returned only when the application provides\n> it. You seem to thinkg the same about FrameDuration :)\n>\n\nExact opposite thoughts! :-)\n\n\n>\n> To be honest, my preference of having FrameDuration returned\n> unconditionally is only because ExposureTime has a relationship with\n> the AEG routine being enabled or not. That's a weak preference though,\n> and if you have other reasons to go in one direction or another I've no\n> strong preference.\n>\n\nI've viewed ExposureTime and AnalogueGain as intrinsic properties of a\nframe, regardless of whether manual values were used or not. Hence why I\nwas setting it every frame. Of course, the exact same thing could be said\nfor Frame Duration! Perhaps go with what we currently do, and if Laurent\nand others have a strong preference the other way, I can make the necessary\nupdates?\n\nRegards,\nNaush\n\n\n\n>\n> >\n> > Regards,\n> > Naush\n> >\n> >\n> >\n> > >\n> > >\n> > >>\n> > >> > + break;\n> > >> > + }\n> > >> > +\n> > >> > default:\n> > >> > LOG(IPARPI, Warning)\n> > >> > << \"Ctrl \" << controls::controls.at\n> > >> (ctrl.first)->name()\n> > >> > @@ -962,15 +983,27 @@ void IPARPi::applyAWB(const struct AwbStatus\n> > >> *awbStatus, ControlList &ctrls)\n> > >> > void IPARPi::applyAGC(const struct AgcStatus *agcStatus,\n> ControlList\n> > >> &ctrls)\n> > >> > {\n> > >> > int32_t gainCode =\n> helper_->GainCode(agcStatus->analogue_gain);\n> > >> > - int32_t exposureLines =\n> > >> helper_->ExposureLines(agcStatus->shutter_time);\n> > >> >\n> > >> > - LOG(IPARPI, Debug) << \"Applying AGC Exposure: \" <<\n> > >> agcStatus->shutter_time\n> > >> > - << \" (Shutter lines: \" << exposureLines <<\n> \")\n> > >> Gain: \"\n> > >> > + /* GetVBlanking might clip exposure time to the fps limits. */\n> > >> > + double exposure = agcStatus->shutter_time;\n> > >> > + int32_t vblanking = helper_->GetVBlanking(exposure,\n> > >> minFrameDuration_,\n> > >> > + maxFrameDuration_);\n> > >> > + int32_t exposureLines = helper_->ExposureLines(exposure);\n> > >> > +\n> > >> > + LOG(IPARPI, Debug) << \"Applying AGC Exposure: \" << exposure\n> > >> > + << \" (Shutter lines: \" << exposureLines <<\n> \",\n> > >> AGC requested \"\n> > >> > + << agcStatus->shutter_time << \") Gain: \"\n> > >> > << agcStatus->analogue_gain << \" (Gain\n> Code: \"\n> > >> > << gainCode << \")\";\n> > >> >\n> > >> > - ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);\n> > >> > + /*\n> > >> > + * Due to the behavior of V4L2, the current value of VBLANK\n> could\n> > >> clip the\n> > >> > + * exposure time without us knowing. The next time though this\n> > >> function should\n> > >> > + * clip exposure correctly.\n> > >>\n> > >> Replying to myself: we skip one frame in case VBLANK clips exposure.\n> > >> I guess setting VBLANK one frame in advance compared to exposure won't\n> > >> help, as it could clip the previous frame. The only solution is to\n> > >> prioritize VBLANK when setting controls to the device I guess.\n> > >>\n> > >\n> > > In my next patch set, I address this by allowing the staggered write\n> > > controls to write VBLANK before any other controls. Depending on the\n> > > timing of DelayedControls being merged, I can easily port it if\n> needed, or\n> > > just submit my change to staggered write.\n> > >\n> > >\n> > >>\n> > >> > + */\n> > >> > + ctrls.set(V4L2_CID_VBLANK, vblanking);\n> > >> > ctrls.set(V4L2_CID_EXPOSURE, exposureLines);\n> > >> > + ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode);\n> > >> > }\n> > >> >\n> > >> > void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList\n> > >> &ctrls)\n> > >> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >> > index 7a5f5881..252cab64 100644\n> > >> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > >> > @@ -1233,7 +1233,8 @@ int RPiCameraData::configureIPA(const\n> > >> CameraConfiguration *config)\n> > >> > if (!staggeredCtrl_) {\n> > >> >\n> staggeredCtrl_.init(unicam_[Unicam::Image].dev(),\n> > >> > { {\n> V4L2_CID_ANALOGUE_GAIN,\n> > >> result.data[resultIdx++] },\n> > >> > - { V4L2_CID_EXPOSURE,\n> > >> result.data[resultIdx++] } });\n> > >> > + { V4L2_CID_EXPOSURE,\n> > >> result.data[resultIdx++] },\n> > >> > + { V4L2_CID_VBLANK,\n> > >> result.data[resultIdx++] } });\n> > >>\n> > >> A few issues apart (metadata handling and manual exposure clipping\n> > >> apart) the rest is all minor stuff. With these fixed/clarified\n> > >>\n> > >\n> > > I will update with a v10 to address the metadata and typo. The manual\n> > > exposure clipping ought to be part of the next patch set as it is part\n> of\n> > > the bigger change where the AGC applies the limits as well. Is that ok\n> > > with you?\n> > >\n> > > Regards,\n> > > Naush\n> > >\n> > >\n> > >\n> > >>\n> > >> Reviewed-by: Jacopo Mondi \n> > >>\n> > >> Thanks\n> > >> j\n> > >>\n> > >> > sensorMetadata_ = result.data[resultIdx++];\n> > >> > }\n> > >> > }\n> > >> > --\n> > >> > 2.25.1\n> > >> >\n> > >> > _______________________________________________\n> > >> > libcamera-devel mailing list\n> > >> > libcamera-devel@lists.libcamera.org\n> > >> > https://lists.libcamera.org/listinfo/libcamera-devel\n> > >>\n> > >\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 85787C0F1A\n\tfor ;\n\tThu, 17 Dec 2020 16:35:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 13F6661591;\n\tThu, 17 Dec 2020 17:35:48 +0100 (CET)","from mail-lf1-x132.google.com (mail-lf1-x132.google.com\n\t[IPv6:2a00:1450:4864:20::132])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5E7FD61591\n\tfor ;\n\tThu, 17 Dec 2020 17:35:46 +0100 (CET)","by mail-lf1-x132.google.com with SMTP id o13so35749258lfr.3\n\tfor ;\n\tThu, 17 Dec 2020 08:35:46 -0800 (PST)"],"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=\"b0vGrpMX\"; 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=JkO4I2EUxcIAo55gQHR551Y0YYXIRE3Ni94wYbXy7O0=;\n\tb=b0vGrpMXoape1ouBunqUqnNgOksS6+4mANW4rF+Lu/67/h6XiGbbXiONag7FT/3Y+U\n\tSEMlInpS+NdEHXcxjpbsZWhyERLNudjKMyggZjXj+U3ra5+DLOrRDKyocyZAFyGBOkSs\n\tzNJ4oOJ6LVQKNZ0s8DUKxeOdNPJS+aO3z7CsW1I/1tLrAA1iREGK6QWXTKBxX36lM8In\n\tQdi5KpXcgb++K+gLt4dW08e9K+BwKBC38ncbXYLJEH1YgVhPWRfZw68kktMegUOFY+lg\n\tqybnSI9rE/Os1Vf10Lhde1rMics6siEiZEKhYvB/KHWSs3mUcbwSYBvqDz1RE7r7389S\n\tJ/zw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=JkO4I2EUxcIAo55gQHR551Y0YYXIRE3Ni94wYbXy7O0=;\n\tb=evtd2oiB4SwdIz3LwUnjTTfqg1TM+hBHUsCuyGRhLrSHducEqzxTg5CVWWpjlVhz8j\n\tUSy5yaLxFrTza0YwpF+tbsmG+9Sj5fzw0t3HZktsa2NszEqxZZltAIqLhUFTXN78GlEu\n\tC3+MJjHr590Xlrr7mRbzruaUWnLwJh1gtvPZOd1Zr3i92DuBfRWzdqBMNK3h6o4pXi2B\n\tn/gm2GXi0UakYCsGrcv3nPh2CZKTMYkGCWZPxfmVNi9pwhUz1ZTEt7MROdgJsxMis7oH\n\tKH2mNzlDMWlECjsP0mehN2cRSozWzznHDpldPmEEYb+/U/9brPtdjHu0RAFIBeSvg4t/\n\twDWQ==","X-Gm-Message-State":"AOAM531qx2vwuZPAnvO2fiYPZOEwz6E/WkzkG/AhlbGF94ozNMconHJ2\n\tdRlMzUV1AcHTqma0eZa1dThetnE87MvOhrCVIfvLKA==","X-Google-Smtp-Source":"ABdhPJy1z+Xd6dk80SmGfYgDK4MKmECantJpNEy/SgvLv2b48eBmVlqOtS7zHBZnogx9P1R44REHbZRfnnEcpAp1ODs=","X-Received":"by 2002:a05:651c:503:: with SMTP id\n\to3mr45938ljp.253.1608222944282; \n\tThu, 17 Dec 2020 08:35:44 -0800 (PST)","MIME-Version":"1.0","References":"<20201216112202.5063-1-naush@raspberrypi.com>\n\t<20201216112202.5063-3-naush@raspberrypi.com>\n\t<20201217151743.66m46drxyoszxmlq@uno.localdomain>\n\t\n\t\n\t<20201217162548.6agmstiv7mfjztsg@uno.localdomain>","In-Reply-To":"<20201217162548.6agmstiv7mfjztsg@uno.localdomain>","From":"Naushir Patuck ","Date":"Thu, 17 Dec 2020 16:35:27 +0000","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v9 2/3] libcamera: raspberrypi: Add\n\tcontrol of sensor vblanking","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 ","Content-Type":"multipart/mixed;\n\tboundary=\"===============8315400436576029763==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14269,"web_url":"https://patchwork.libcamera.org/comment/14269/","msgid":"<20201217163851.2pzuspiy24pufo3n@uno.localdomain>","date":"2020-12-17T16:38:51","subject":"Re: [libcamera-devel] [PATCH v9 2/3] libcamera: raspberrypi: Add\n\tcontrol of sensor vblanking","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Naush\n\nOn Thu, Dec 17, 2020 at 04:35:27PM +0000, Naushir Patuck wrote:\n> Hi Jacopo,\n>\n> >\n> > To be honest, my preference of having FrameDuration returned\n> > unconditionally is only because ExposureTime has a relationship with\n> > the AEG routine being enabled or not. That's a weak preference though,\n> > and if you have other reasons to go in one direction or another I've no\n> > strong preference.\n> >\n>\n> I've viewed ExposureTime and AnalogueGain as intrinsic properties of a\n> frame, regardless of whether manual values were used or not. Hence why I\n> was setting it every frame. Of course, the exact same thing could be said\n> for Frame Duration! Perhaps go with what we currently do, and if Laurent\n> and others have a strong preference the other way, I can make the necessary\n> updates?\n\nThat works for me!\n\nThanks\n j","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 04B9CC0F1A\n\tfor ;\n\tThu, 17 Dec 2020 16:38:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 979A661595;\n\tThu, 17 Dec 2020 17:38:41 +0100 (CET)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2903461591\n\tfor ;\n\tThu, 17 Dec 2020 17:38:41 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 67A5CFF80A;\n\tThu, 17 Dec 2020 16:38:40 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Thu, 17 Dec 2020 17:38:51 +0100","From":"Jacopo Mondi ","To":"Naushir Patuck ","Message-ID":"<20201217163851.2pzuspiy24pufo3n@uno.localdomain>","References":"<20201216112202.5063-1-naush@raspberrypi.com>\n\t<20201216112202.5063-3-naush@raspberrypi.com>\n\t<20201217151743.66m46drxyoszxmlq@uno.localdomain>\n\t\n\t\n\t<20201217162548.6agmstiv7mfjztsg@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v9 2/3] libcamera: raspberrypi: Add\n\tcontrol of sensor vblanking","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14270,"web_url":"https://patchwork.libcamera.org/comment/14270/","msgid":"","date":"2020-12-17T16:42:52","subject":"Re: [libcamera-devel] [PATCH v9 2/3] libcamera: raspberrypi: Add\n\tcontrol of sensor vblanking","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"On Thu, 17 Dec 2020 at 16:38, Jacopo Mondi wrote:\n\n> Hi Naush\n>\n> On Thu, Dec 17, 2020 at 04:35:27PM +0000, Naushir Patuck wrote:\n> > Hi Jacopo,\n> >\n> > >\n> > > To be honest, my preference of having FrameDuration returned\n> > > unconditionally is only because ExposureTime has a relationship with\n> > > the AEG routine being enabled or not. That's a weak preference though,\n> > > and if you have other reasons to go in one direction or another I've no\n> > > strong preference.\n> > >\n> >\n> > I've viewed ExposureTime and AnalogueGain as intrinsic properties of a\n> > frame, regardless of whether manual values were used or not. Hence why I\n> > was setting it every frame. Of course, the exact same thing could be\n> said\n> > for Frame Duration! Perhaps go with what we currently do, and if Laurent\n> > and others have a strong preference the other way, I can make the\n> necessary\n> > updates?\n>\n> That works for me!\n>\n\nOk great. I will modify the wording for the control documentation to\nreflect what it is (i.e. requested frame limits clipped to what the sensor\nmode can achieve), and post a v10 shortly. I will remove your tag from\npatch 1/3 so you can comment if you are happy (or not) with the new\nwording. We will get there eventually! :)\n\nRegards,\nNaush\n\n\n> Thanks\n> j\n>\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 899DDBD80A\n\tfor ;\n\tThu, 17 Dec 2020 16:43:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2665861597;\n\tThu, 17 Dec 2020 17:43:10 +0100 (CET)","from mail-lf1-x12b.google.com (mail-lf1-x12b.google.com\n\t[IPv6:2a00:1450:4864:20::12b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9E3DB61591\n\tfor ;\n\tThu, 17 Dec 2020 17:43:08 +0100 (CET)","by mail-lf1-x12b.google.com with SMTP id o19so33387392lfo.1\n\tfor ;\n\tThu, 17 Dec 2020 08:43:08 -0800 (PST)"],"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=\"QHFHow18\"; 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=vzK/0phz5pFpZMEKGusdW/xq+UMzdPnzoa0dPs9bpVk=;\n\tb=QHFHow18WF40HdxoY3A1Q9VtyXRuT6+Y6Y7194/CYgDgeeKWRPx+6lGVkVeqEVfefJ\n\twX+3N4XMNX0E+gNx5hEsii1Wvq52iZ7jEE4ivdw/1KOQzKDx8FtAnuNNUFPUEJOMzSHc\n\tx3+L/LZujq98S7MNin2FLa50IgQ/wvHNn9yLlHrP/JhKs1mHJ9s29x0Zvl7lyguL7JtZ\n\tZfsApJo8QEQ4JZCp6Vt1gJivokE/qm33pf7y2jIxskwLAR4Exqq+JLQ+C1Hr/AQyKNHl\n\twXb9359FZthpeki8oVxDjURC4/BKQGjptRx+MUMDfy/sHHE24sEkRm02FrU7fqQiJvc2\n\txYzw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=vzK/0phz5pFpZMEKGusdW/xq+UMzdPnzoa0dPs9bpVk=;\n\tb=CxWdAGdeoIcxgjSq6XhDrScoM4TJI2Yray2F+NuUsc/7zgOw5OzqvHLudQOMdNm9HL\n\tRoY0HUZEKgc0CkATWsje+dTNqdxDtZN55HHDHH1bhWxzbs9sv8D9Xg6FMORwhD6+oMBG\n\tbBmYSV5za/JJLD79oXYtFjEaZ2oOMRiI1z6BqcdJyKrBPbUpaQ9ZLKvf46G0AQki87of\n\t+eIiktbD6lKsdO0t1OzwDbY1sd4MrU+ryMNV9fJWVmlHDP1d74PgK1HFO8eWzeHwQhyh\n\tCQ+a7gugBafg+HW/QmGot9638cq1LS2pAcfngE0unKi1UDLFwPiKl8gL9DY9dFpYpuNq\n\t8fdw==","X-Gm-Message-State":"AOAM532sSUBKsco9F6dEXpNa1T42w3fDuqtHhRAW8ouBlzC05wA+0LKc\n\tgSSTgx2RRgDtNWKZivpQnzmCkRuUMuRW0PbS6LYysw==","X-Google-Smtp-Source":"ABdhPJz0a26beid2I1z4z5kZG5UjPDm/Zcxk0prKwRjelIhIpGeLdIwK+ItOkD6j0AdQ26khSc/sSqL/2y3kETcRkJo=","X-Received":"by 2002:a19:950:: with SMTP id 77mr14581249lfj.133.1608223387874;\n\tThu, 17 Dec 2020 08:43:07 -0800 (PST)","MIME-Version":"1.0","References":"<20201216112202.5063-1-naush@raspberrypi.com>\n\t<20201216112202.5063-3-naush@raspberrypi.com>\n\t<20201217151743.66m46drxyoszxmlq@uno.localdomain>\n\t\n\t\n\t<20201217162548.6agmstiv7mfjztsg@uno.localdomain>\n\t\n\t<20201217163851.2pzuspiy24pufo3n@uno.localdomain>","In-Reply-To":"<20201217163851.2pzuspiy24pufo3n@uno.localdomain>","From":"Naushir Patuck ","Date":"Thu, 17 Dec 2020 16:42:52 +0000","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v9 2/3] libcamera: raspberrypi: Add\n\tcontrol of sensor vblanking","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 ","Content-Type":"multipart/mixed;\n\tboundary=\"===============7239346961290863530==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]