[{"id":14653,"web_url":"https://patchwork.libcamera.org/comment/14653/","msgid":"","date":"2021-01-21T14:52:53","subject":"Re: [libcamera-devel] [PATCH 2/4] ipa: raspberrypi: Limit the\n\tcalculated vblank based on the sensor mode","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Naush\n\nI don't think I can claim to understand every last nuance of this, but\nlet me try anyway but here goes...!\n\nOn Thu, 21 Jan 2021 at 11:58, Naushir Patuck wrote:\n>\n> The existing framerate/vblank calculations did not account for the\n> sensor mode limits. This commit extracts vblank limits from the sensor\n> v4l2 controls, and stores it in the camera modes structure.\n>\n> Exposure and vblank value calculations now use values clamped to the\n> sensor mode limits. The libcamera::controls::FrameDurations metadata\n> return values now reflect the minimum and maximum after clamping.\n>\n> Signed-off-by: Naushir Patuck \n> ---\n> src/ipa/raspberrypi/cam_helper.cpp | 16 +++---\n> src/ipa/raspberrypi/cam_helper.hpp | 5 +-\n> src/ipa/raspberrypi/cam_helper_imx219.cpp | 6 +--\n> src/ipa/raspberrypi/cam_helper_imx477.cpp | 4 +-\n> src/ipa/raspberrypi/cam_helper_ov5647.cpp | 4 +-\n> src/ipa/raspberrypi/controller/camera_mode.h | 2 +\n> src/ipa/raspberrypi/raspberrypi.cpp | 51 ++++++++++++++------\n> 7 files changed, 49 insertions(+), 39 deletions(-)\n>\n> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp\n> index b7b8faf09c69..3e17255737b2 100644\n> --- a/src/ipa/raspberrypi/cam_helper.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> @@ -34,9 +34,8 @@ CamHelper *CamHelper::Create(std::string const &cam_name)\n> return nullptr;\n> }\n>\n> -CamHelper::CamHelper(MdParser *parser, unsigned int maxFrameLength,\n> - unsigned int frameIntegrationDiff)\n> - : parser_(parser), initialized_(false), maxFrameLength_(maxFrameLength),\n> +CamHelper::CamHelper(MdParser *parser, unsigned int frameIntegrationDiff)\n> + : parser_(parser), initialized_(false),\n> frameIntegrationDiff_(frameIntegrationDiff)\n> {\n> }\n> @@ -67,13 +66,12 @@ uint32_t CamHelper::GetVBlanking(double &exposure, double minFrameDuration,\n> assert(initialized_);\n>\n> /*\n> - * Clamp frame length by the frame duration range and the maximum allowable\n> - * value in the sensor, given by maxFrameLength_.\n> + * minFrameDuration and maxFrameDuration will have been clamped to the\n> + * maximum allowable values in the sensor for this mode.\n> */\n> - frameLengthMin = std::clamp(1e3 * minFrameDuration / mode_.line_length,\n> - mode_.height, maxFrameLength_);\n> - frameLengthMax = std::clamp(1e3 * maxFrameDuration / mode_.line_length,\n> - mode_.height, maxFrameLength_);\n> + frameLengthMin = 1e3 * minFrameDuration / mode_.line_length;\n> + frameLengthMax = 1e3 * maxFrameDuration / mode_.line_length;\n> +\n> /*\n> * Limit the exposure to the maximum frame duration requested, and\n> * re-calculate if it has been clipped.\n> diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp\n> index b1739a57e342..14d70112cb62 100644\n> --- a/src/ipa/raspberrypi/cam_helper.hpp\n> +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> @@ -62,8 +62,7 @@ class CamHelper\n> {\n> public:\n> static CamHelper *Create(std::string const &cam_name);\n> - CamHelper(MdParser *parser, unsigned int maxFrameLength,\n> - unsigned int frameIntegrationDiff);\n> + CamHelper(MdParser *parser, unsigned int frameIntegrationDiff);\n> virtual ~CamHelper();\n> void SetCameraMode(const CameraMode &mode);\n> MdParser &Parser() const { return *parser_; }\n> @@ -86,8 +85,6 @@ protected:\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 time,\n> * in units of lines.\n> diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> index 8688ec091c44..95b8e698fe3b 100644\n> --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> @@ -56,15 +56,13 @@ private:\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(), maxFrameLength, frameIntegrationDiff)\n> + : CamHelper(new MdParserImx219(), frameIntegrationDiff)\n> #else\n> - : CamHelper(new MdParserRPi(), maxFrameLength, frameIntegrationDiff)\n> + : CamHelper(new MdParserRPi(), 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 5396131003f1..eaa7be16d20e 100644\n> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> @@ -45,12 +45,10 @@ private:\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(), maxFrameLength, frameIntegrationDiff)\n> + : CamHelper(new MdParserImx477(), 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 2bd8a754f133..a7f417324048 100644\n> --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> @@ -30,8 +30,6 @@ private:\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> @@ -40,7 +38,7 @@ private:\n> */\n>\n> CamHelperOv5647::CamHelperOv5647()\n> - : CamHelper(new MdParserRPi(), maxFrameLength, frameIntegrationDiff)\n> + : CamHelper(new MdParserRPi(), frameIntegrationDiff)\n> {\n> }\n>\n> diff --git a/src/ipa/raspberrypi/controller/camera_mode.h b/src/ipa/raspberrypi/controller/camera_mode.h\n> index 920f11beb0b0..3baeadaca076 100644\n> --- a/src/ipa/raspberrypi/controller/camera_mode.h\n> +++ b/src/ipa/raspberrypi/controller/camera_mode.h\n> @@ -37,6 +37,8 @@ struct CameraMode {\n> double line_length;\n> // any camera transform *not* reflected already in the camera tuning\n> libcamera::Transform transform;\n> + // minimum and maximum vblanking limits for this mode\n> + uint32_t vblank_min, vblank_max;\n> };\n>\n> #ifdef __cplusplus\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 5ccc7a6551f5..9e6e030c4997 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -102,6 +102,7 @@ private:\n> void reportMetadata();\n> bool parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &deviceStatus);\n> void processStats(unsigned int bufferId);\n> + void applyFrameDurations(double min, double max);\n> void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);\n> void applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls);\n> void applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls);\n> @@ -279,6 +280,14 @@ void IPARPi::setMode(const CameraSensorInfo &sensorInfo)\n> * the line length in pixels and the pixel rate.\n> */\n> mode_.line_length = 1e9 * sensorInfo.lineLength / sensorInfo.pixelRate;\n> +\n> + /*\n> + * Set the vertical blanking (frame duration) limits for the mode to\n> + * ensure exposure and framerate calculations are clipped appropriately.\n> + */\n> + const auto itVblank = sensorCtrls_.find(V4L2_CID_VBLANK);\n\nAre we happy to assume this exists? (I realise that if it doesn't then\neverything is doomed and we probably don't care very much...)\n\n> + mode_.vblank_min = itVblank->second.min().get();\n> + mode_.vblank_max = itVblank->second.max().get();\n> }\n>\n> void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> @@ -384,8 +393,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> controller_.Initialise();\n> controllerInit_ = true;\n>\n> - minFrameDuration_ = defaultMinFrameDuration;\n> - maxFrameDuration_ = defaultMaxFrameDuration;\n> + /* Supply initial values for frame durations. */\n> + applyFrameDurations(defaultMinFrameDuration, defaultMaxFrameDuration);\n>\n> /* Supply initial values for gain and exposure. */\n> ControlList ctrls(sensorCtrls_);\n> @@ -819,20 +828,7 @@ void IPARPi::queueRequest(const ControlList &controls)\n>\n> case controls::FRAME_DURATIONS: {\n> auto frameDurations = ctrl.second.get>();\n> -\n> - /* This will be applied once AGC recalculations occur. */\n> - minFrameDuration_ = frameDurations[0] ? frameDurations[0] : defaultMinFrameDuration;\n> - maxFrameDuration_ = frameDurations[1] ? frameDurations[1] : defaultMaxFrameDuration;\n> - maxFrameDuration_ = std::max(maxFrameDuration_, minFrameDuration_);\n> -\n> - /*\n> - * \\todo The values returned in the metadata below must be\n> - * correctly clipped by what the sensor mode supports and\n> - * what the AGC exposure mode or manual shutter speed limits\n> - */\n> - libcameraMetadata_.set(controls::FrameDurations,\n> - { static_cast(minFrameDuration_),\n> - static_cast(maxFrameDuration_) });\n> + applyFrameDurations(frameDurations[0], frameDurations[1]);\n> break;\n> }\n>\n> @@ -991,6 +987,29 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)\n> static_cast(awbStatus->gain_b * 1000));\n> }\n>\n> +void IPARPi::applyFrameDurations(double min, double max)\n\nI didn't terribly like variables named just \"min\" and \"max\", as I\nimmediately wonder what they are the min and max of... or maybe it's\njust a hangover from C programming where there often seemed to be min\nand max macros flying around!\n\nBut these terribly minor things aside:\n\nReviewed-by: David Plowman \n\nThanks!\nDavid\n\n> +{\n> + const double minSensorFrameDuration = 1e-3 * (mode_.vblank_min + mode_.height) *\n> + mode_.line_length;\n> + const double maxSensorFrameDuration = 1e-3 * (mode_.vblank_max + mode_.height) *\n> + mode_.line_length;\n> + /*\n> + * This will only be applied once AGC recalculations occur.\n> + * The values may be clamped based on the sensor mode capabilities as well.\n> + */\n> + minFrameDuration_ = min ? min : defaultMaxFrameDuration;\n> + maxFrameDuration_ = max ? max : defaultMinFrameDuration;\n> + minFrameDuration_ = std::clamp(minFrameDuration_,\n> + minSensorFrameDuration, maxSensorFrameDuration);\n> + maxFrameDuration_ = std::clamp(maxFrameDuration_,\n> + minSensorFrameDuration, maxSensorFrameDuration);\n> +\n> + /* Return the validated limits out though metadata. */\n> + libcameraMetadata_.set(controls::FrameDurations,\n> + { static_cast(minFrameDuration_),\n> + static_cast(maxFrameDuration_) });\n> +}\n> +\n> void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)\n> {\n> int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);\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 B28E5BD808\n\tfor ;\n\tThu, 21 Jan 2021 14:53:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2363C681F0;\n\tThu, 21 Jan 2021 15:53:08 +0100 (CET)","from mail-oi1-x22b.google.com (mail-oi1-x22b.google.com\n\t[IPv6:2607:f8b0:4864:20::22b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1E0F3681DB\n\tfor ;\n\tThu, 21 Jan 2021 15:53:06 +0100 (CET)","by mail-oi1-x22b.google.com with SMTP id h6so1003720oie.5\n\tfor ;\n\tThu, 21 Jan 2021 06:53:06 -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=\"XFdd04us\"; 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=2DmWU3R7z2zVc9c+pt67KkRRWa33AxnE09mK+TknCmo=;\n\tb=XFdd04us8ws3Um3CrZobGDvD/RBt9f7KYiBGdVXUycWC5EWy125+/xnsGM+6ElKl1n\n\tTycrmAJeXcRhw9AHcNFAYmw7WzKqMC+NFGGCL2Hus75No7Xtm7QabTBKcXVxGyseZ7Ib\n\t3Evajn3cgYy8sXZhNXcPPlCHBV7Xnyl9pp6WBvHonCP/DF0I5cvmnuIsa+TtgmgW+IUc\n\twHGAvlcMRe77O/mjreN0Fsg0K5Vs3XLea5BpDAGTozroRO7WoUsTxTAVf5l/vt6DdrB0\n\tptlLYbO2kolv5WnYSummvOsY7jfJmmJmDwuVjmD4c3BN2vzt/CYSACFF/bB2tmxRSGmG\n\tPj5g==","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=2DmWU3R7z2zVc9c+pt67KkRRWa33AxnE09mK+TknCmo=;\n\tb=YlYSriHhci2AlcPHm6RyKn/H2iaAPDJpxfgdbox1rj1/yKYn0C55WwmiBc5wasIUls\n\tNEg2+dTbyknDi4M0AbjavNJ3uxUryRL0M7l7ohVaxf9OFsMFtZI6vse0Lf3stXmmxRuN\n\tOkw9HMEVCEmyLjWO7WKRXb1jMyHDu3w7KNU2ywHtgpQyG2LzVcHO5HgKQh7VPk9MoWWK\n\tddhelpMmeLGCgcQxJ7riIWAkljgZvF66Jj8rGMXmVe4ZtgFOdanjWbIeBAA3Wn7LlHqm\n\thtoC2YJSYr+ASglw0vHHz1Jex9PaJc2yIr7/ushMu0pGEEc1YP+VcNI7faC5ITEzvGWk\n\twbsg==","X-Gm-Message-State":"AOAM531/vSbmIeSkeQzj4hyvNUO8lUseDxcFt7QS9kQYWkR8rIC9WKj/\n\tD3XVEVS3zqBiv2AMAvRrOJFl5vUbv6+xaaGQWqHQfg==","X-Google-Smtp-Source":"ABdhPJwDbp4F7RGlWwJUjY8XqaYertoohoGd5C8pHtj7pQJJadpH39MyL2B7keA+pALe390lXtqntY+QP0bb/mUGab8=","X-Received":"by 2002:aca:5c0a:: with SMTP id q10mr6328782oib.55.1611240784568;\n\tThu, 21 Jan 2021 06:53:04 -0800 (PST)","MIME-Version":"1.0","References":"<20210121115849.682130-1-naush@raspberrypi.com>\n\t<20210121115849.682130-3-naush@raspberrypi.com>","In-Reply-To":"<20210121115849.682130-3-naush@raspberrypi.com>","From":"David Plowman ","Date":"Thu, 21 Jan 2021 14:52:53 +0000","Message-ID":"","To":"Naushir Patuck ","Subject":"Re: [libcamera-devel] [PATCH 2/4] ipa: raspberrypi: Limit the\n\tcalculated vblank based on the sensor mode","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":14656,"web_url":"https://patchwork.libcamera.org/comment/14656/","msgid":"","date":"2021-01-21T15:45:00","subject":"Re: [libcamera-devel] [PATCH 2/4] ipa: raspberrypi: Limit the\n\tcalculated vblank based on the sensor mode","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi David,\n\nThank you for your review feedback.\n\nOn Thu, 21 Jan 2021 at 14:53, David Plowman \nwrote:\n\n> Hi Naush\n>\n> I don't think I can claim to understand every last nuance of this, but\n> let me try anyway but here goes...!\n>\n> On Thu, 21 Jan 2021 at 11:58, Naushir Patuck \n> wrote:\n> >\n> > The existing framerate/vblank calculations did not account for the\n> > sensor mode limits. This commit extracts vblank limits from the sensor\n> > v4l2 controls, and stores it in the camera modes structure.\n> >\n> > Exposure and vblank value calculations now use values clamped to the\n> > sensor mode limits. The libcamera::controls::FrameDurations metadata\n> > return values now reflect the minimum and maximum after clamping.\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > src/ipa/raspberrypi/cam_helper.cpp | 16 +++---\n> > src/ipa/raspberrypi/cam_helper.hpp | 5 +-\n> > src/ipa/raspberrypi/cam_helper_imx219.cpp | 6 +--\n> > src/ipa/raspberrypi/cam_helper_imx477.cpp | 4 +-\n> > src/ipa/raspberrypi/cam_helper_ov5647.cpp | 4 +-\n> > src/ipa/raspberrypi/controller/camera_mode.h | 2 +\n> > src/ipa/raspberrypi/raspberrypi.cpp | 51 ++++++++++++++------\n> > 7 files changed, 49 insertions(+), 39 deletions(-)\n> >\n> > diff --git a/src/ipa/raspberrypi/cam_helper.cpp\n> b/src/ipa/raspberrypi/cam_helper.cpp\n> > index b7b8faf09c69..3e17255737b2 100644\n> > --- a/src/ipa/raspberrypi/cam_helper.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> > @@ -34,9 +34,8 @@ CamHelper *CamHelper::Create(std::string const\n> &cam_name)\n> > return nullptr;\n> > }\n> >\n> > -CamHelper::CamHelper(MdParser *parser, unsigned int maxFrameLength,\n> > - unsigned int frameIntegrationDiff)\n> > - : parser_(parser), initialized_(false),\n> maxFrameLength_(maxFrameLength),\n> > +CamHelper::CamHelper(MdParser *parser, unsigned int\n> frameIntegrationDiff)\n> > + : parser_(parser), initialized_(false),\n> > frameIntegrationDiff_(frameIntegrationDiff)\n> > {\n> > }\n> > @@ -67,13 +66,12 @@ uint32_t CamHelper::GetVBlanking(double &exposure,\n> double minFrameDuration,\n> > assert(initialized_);\n> >\n> > /*\n> > - * Clamp frame length by the frame duration range and the\n> maximum allowable\n> > - * value in the sensor, given by maxFrameLength_.\n> > + * minFrameDuration and maxFrameDuration will have been clamped\n> to the\n> > + * maximum allowable values in the sensor for this mode.\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> > + frameLengthMin = 1e3 * minFrameDuration / mode_.line_length;\n> > + frameLengthMax = 1e3 * maxFrameDuration / mode_.line_length;\n> > +\n> > /*\n> > * Limit the exposure to the maximum frame duration requested,\n> and\n> > * re-calculate if it has been clipped.\n> > diff --git a/src/ipa/raspberrypi/cam_helper.hpp\n> b/src/ipa/raspberrypi/cam_helper.hpp\n> > index b1739a57e342..14d70112cb62 100644\n> > --- a/src/ipa/raspberrypi/cam_helper.hpp\n> > +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> > @@ -62,8 +62,7 @@ class CamHelper\n> > {\n> > public:\n> > static CamHelper *Create(std::string const &cam_name);\n> > - CamHelper(MdParser *parser, unsigned int maxFrameLength,\n> > - unsigned int frameIntegrationDiff);\n> > + CamHelper(MdParser *parser, unsigned int frameIntegrationDiff);\n> > virtual ~CamHelper();\n> > void SetCameraMode(const CameraMode &mode);\n> > MdParser &Parser() const { return *parser_; }\n> > @@ -86,8 +85,6 @@ protected:\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> > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > index 8688ec091c44..95b8e698fe3b 100644\n> > --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > @@ -56,15 +56,13 @@ private:\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(), maxFrameLength,\n> frameIntegrationDiff)\n> > + : CamHelper(new MdParserImx219(), frameIntegrationDiff)\n> > #else\n> > - : CamHelper(new MdParserRPi(), maxFrameLength,\n> frameIntegrationDiff)\n> > + : CamHelper(new MdParserRPi(), 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 5396131003f1..eaa7be16d20e 100644\n> > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > @@ -45,12 +45,10 @@ private:\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(), maxFrameLength,\n> frameIntegrationDiff)\n> > + : CamHelper(new MdParserImx477(), 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 2bd8a754f133..a7f417324048 100644\n> > --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> > @@ -30,8 +30,6 @@ private:\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> > @@ -40,7 +38,7 @@ private:\n> > */\n> >\n> > CamHelperOv5647::CamHelperOv5647()\n> > - : CamHelper(new MdParserRPi(), maxFrameLength,\n> frameIntegrationDiff)\n> > + : CamHelper(new MdParserRPi(), frameIntegrationDiff)\n> > {\n> > }\n> >\n> > diff --git a/src/ipa/raspberrypi/controller/camera_mode.h\n> b/src/ipa/raspberrypi/controller/camera_mode.h\n> > index 920f11beb0b0..3baeadaca076 100644\n> > --- a/src/ipa/raspberrypi/controller/camera_mode.h\n> > +++ b/src/ipa/raspberrypi/controller/camera_mode.h\n> > @@ -37,6 +37,8 @@ struct CameraMode {\n> > double line_length;\n> > // any camera transform *not* reflected already in the camera\n> tuning\n> > libcamera::Transform transform;\n> > + // minimum and maximum vblanking limits for this mode\n> > + uint32_t vblank_min, vblank_max;\n> > };\n> >\n> > #ifdef __cplusplus\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 5ccc7a6551f5..9e6e030c4997 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -102,6 +102,7 @@ private:\n> > void reportMetadata();\n> > bool parseEmbeddedData(unsigned int bufferId, struct\n> DeviceStatus &deviceStatus);\n> > void processStats(unsigned int bufferId);\n> > + void applyFrameDurations(double min, double max);\n> > void applyAGC(const struct AgcStatus *agcStatus, ControlList\n> &ctrls);\n> > void applyAWB(const struct AwbStatus *awbStatus, ControlList\n> &ctrls);\n> > void applyDG(const struct AgcStatus *dgStatus, ControlList\n> &ctrls);\n> > @@ -279,6 +280,14 @@ void IPARPi::setMode(const CameraSensorInfo\n> &sensorInfo)\n> > * the line length in pixels and the pixel rate.\n> > */\n> > mode_.line_length = 1e9 * sensorInfo.lineLength /\n> sensorInfo.pixelRate;\n> > +\n> > + /*\n> > + * Set the vertical blanking (frame duration) limits for the\n> mode to\n> > + * ensure exposure and framerate calculations are clipped\n> appropriately.\n> > + */\n> > + const auto itVblank = sensorCtrls_.find(V4L2_CID_VBLANK);\n>\n> Are we happy to assume this exists? (I realise that if it doesn't then\n> everything is doomed and we probably don't care very much...)\n>\n\nYes, we do validate that all controls (sensor and isp) are available on\nipa::configure(). If not we fail hard, so this should be safe.\n\n\n>\n> > + mode_.vblank_min = itVblank->second.min().get();\n> > + mode_.vblank_max = itVblank->second.max().get();\n> > }\n> >\n> > void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > @@ -384,8 +393,8 @@ void IPARPi::configure(const CameraSensorInfo\n> &sensorInfo,\n> > controller_.Initialise();\n> > controllerInit_ = true;\n> >\n> > - minFrameDuration_ = defaultMinFrameDuration;\n> > - maxFrameDuration_ = defaultMaxFrameDuration;\n> > + /* Supply initial values for frame durations. */\n> > + applyFrameDurations(defaultMinFrameDuration,\n> defaultMaxFrameDuration);\n> >\n> > /* Supply initial values for gain and exposure. */\n> > ControlList ctrls(sensorCtrls_);\n> > @@ -819,20 +828,7 @@ void IPARPi::queueRequest(const ControlList\n> &controls)\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> > - maxFrameDuration_ = std::max(maxFrameDuration_,\n> minFrameDuration_);\n> > -\n> > - /*\n> > - * \\todo The values returned in the metadata\n> below must be\n> > - * correctly clipped by what the sensor mode\n> supports and\n> > - * what the AGC exposure mode or manual shutter\n> speed limits\n> > - */\n> > - libcameraMetadata_.set(controls::FrameDurations,\n> > - {\n> static_cast(minFrameDuration_),\n> > -\n> static_cast(maxFrameDuration_) });\n> > + applyFrameDurations(frameDurations[0],\n> frameDurations[1]);\n> > break;\n> > }\n> >\n> > @@ -991,6 +987,29 @@ void IPARPi::applyAWB(const struct AwbStatus\n> *awbStatus, ControlList &ctrls)\n> > static_cast(awbStatus->gain_b * 1000));\n> > }\n> >\n> > +void IPARPi::applyFrameDurations(double min, double max)\n>\n> I didn't terribly like variables named just \"min\" and \"max\", as I\n> immediately wonder what they are the min and max of... or maybe it's\n> just a hangover from C programming where there often seemed to be min\n> and max macros flying around!\n>\n\nI can change the names to something more appropriate.\n\nRegards,\nNaush\n\n\n\n>\n> But these terribly minor things aside:\n>\n> Reviewed-by: David Plowman \n>\n> Thanks!\n> David\n>\n> > +{\n> > + const double minSensorFrameDuration = 1e-3 * (mode_.vblank_min +\n> mode_.height) *\n> > + mode_.line_length;\n> > + const double maxSensorFrameDuration = 1e-3 * (mode_.vblank_max +\n> mode_.height) *\n> > + mode_.line_length;\n> > + /*\n> > + * This will only be applied once AGC recalculations occur.\n> > + * The values may be clamped based on the sensor mode\n> capabilities as well.\n> > + */\n> > + minFrameDuration_ = min ? min : defaultMaxFrameDuration;\n> > + maxFrameDuration_ = max ? max : defaultMinFrameDuration;\n> > + minFrameDuration_ = std::clamp(minFrameDuration_,\n> > + minSensorFrameDuration,\n> maxSensorFrameDuration);\n> > + maxFrameDuration_ = std::clamp(maxFrameDuration_,\n> > + minSensorFrameDuration,\n> maxSensorFrameDuration);\n> > +\n> > + /* Return the validated limits out though metadata. */\n> > + libcameraMetadata_.set(controls::FrameDurations,\n> > + { static_cast(minFrameDuration_),\n> > + static_cast(maxFrameDuration_)\n> });\n> > +}\n> > +\n> > void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList\n> &ctrls)\n> > {\n> > int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);\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 F2B7BC0F2A\n\tfor ;\n\tThu, 21 Jan 2021 15:45:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 45ACF681F9;\n\tThu, 21 Jan 2021 16:45:20 +0100 (CET)","from mail-qv1-xf2b.google.com (mail-qv1-xf2b.google.com\n\t[IPv6:2607:f8b0:4864:20::f2b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E639F681DB\n\tfor ;\n\tThu, 21 Jan 2021 16:45:18 +0100 (CET)","by mail-qv1-xf2b.google.com with SMTP id 2so1103947qvd.0\n\tfor ;\n\tThu, 21 Jan 2021 07:45:18 -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=\"jdtrD3nq\"; 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=6iTkDD71JWcL00r33Y4dTWdpCXjxoOd/kNefqC3Jpi4=;\n\tb=jdtrD3nqhKMxeJnn99UaUmqdR8Rm8cNaTnSEA/Pb1Qpm/oU3d67EIbgmDZZyBa6vcj\n\tel75lBhcTqgbuMnaDMbw9d31Y7nXsepgte/HHqevIK/OY2Vc9r5MLzX2EMAtwB2wi02E\n\tPM1IqdKSWR3e9gvSKJr7tcGWwpONdQprkiWwAoE4S5A578YCS1IQ+AWemcNWvRo4F0Hk\n\tLSFTNW5UDviEx/G2u9hqRzEKxLn2OqpcCL/eEA58Fh/nZWVXkjo4f0aBfdnY7FDaeUK/\n\tvZDl9gTyVUqGqftRQNld/KyHQfZ97PL25dOWoXy2dOcesGGu2kBg6+bT3Z/+FzD9e7aA\n\teIow==","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=6iTkDD71JWcL00r33Y4dTWdpCXjxoOd/kNefqC3Jpi4=;\n\tb=OGSakyXwROH7xenMw6XFW/KV+1pmH4b9Gw6yxaX02KcApjz2a90Rd6lSAZT9mXLQlR\n\toclzx9BDcdyRpFM87u8PikjDSN8yCLW6Y+BzaJ8tA1EhmTu09MN5bjFaD+oXRqW/lpIT\n\txfkZpHB1YGNdkaJvvOlUkBOphkjezvhB4hBObG3UuNA6n+ZXPaW+9jGnATQfZJfYhTK0\n\tDSbWM8xCXzCFYUOUO4VsQvpv70ThmpgA3MRT+JTelh+Wt4ZYpNE8PbjW2pLmanHLhqP9\n\tdq7xKKmrH/1dQCOq1cqBxpWsF3t7ftrzQQt3uQH+gYMacv0UJxhHrdSzCzfqS6pel5l2\n\t5H3g==","X-Gm-Message-State":"AOAM530fqAFLDl+FU6Gu4yBuDH8/tOv6fItuV7j+B9sAfsK/vo99jxca\n\tpCrwhl8CTxztboPIA4jaoXfaHo49xg7npLvlSTcanw==","X-Google-Smtp-Source":"ABdhPJx8RgnK9AZ6shvRDXxiAcTWaedaaEMRls/ITg4VVwNHCt8z3aEgJW6R5EsOUzxLPsjPNQGMo8C/iarkRuIMgfo=","X-Received":"by 2002:ad4:4108:: with SMTP id i8mr14704978qvp.49.1611243917762;\n\tThu, 21 Jan 2021 07:45:17 -0800 (PST)","MIME-Version":"1.0","References":"<20210121115849.682130-1-naush@raspberrypi.com>\n\t<20210121115849.682130-3-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Thu, 21 Jan 2021 15:45:00 +0000","Message-ID":"","To":"David Plowman ","Subject":"Re: [libcamera-devel] [PATCH 2/4] ipa: raspberrypi: Limit the\n\tcalculated vblank based on the sensor mode","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=\"===============3130472526916116011==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14739,"web_url":"https://patchwork.libcamera.org/comment/14739/","msgid":"<20210125093843.anideq5ijrhwdsg7@uno.localdomain>","date":"2021-01-25T09:38:43","subject":"Re: [libcamera-devel] [PATCH 2/4] ipa: raspberrypi: Limit the\n\tcalculated vblank based on the sensor mode","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Nuash,\n\nOn Thu, Jan 21, 2021 at 11:58:47AM +0000, Naushir Patuck wrote:\n> The existing framerate/vblank calculations did not account for the\n> sensor mode limits. This commit extracts vblank limits from the sensor\n> v4l2 controls, and stores it in the camera modes structure.\n>\n> Exposure and vblank value calculations now use values clamped to the\n> sensor mode limits. The libcamera::controls::FrameDurations metadata\n> return values now reflect the minimum and maximum after clamping.\n>\n> Signed-off-by: Naushir Patuck \n> ---\n> src/ipa/raspberrypi/cam_helper.cpp | 16 +++---\n> src/ipa/raspberrypi/cam_helper.hpp | 5 +-\n> src/ipa/raspberrypi/cam_helper_imx219.cpp | 6 +--\n> src/ipa/raspberrypi/cam_helper_imx477.cpp | 4 +-\n> src/ipa/raspberrypi/cam_helper_ov5647.cpp | 4 +-\n> src/ipa/raspberrypi/controller/camera_mode.h | 2 +\n> src/ipa/raspberrypi/raspberrypi.cpp | 51 ++++++++++++++------\n\nAny reason I am missing on why you kept this change private to your\nIPA instead of making vblank part of the CameraSensorInfo contructed\nby the CameraSensor class ?\n\nThanks\n j\n\n> 7 files changed, 49 insertions(+), 39 deletions(-)\n>\n> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp\n> index b7b8faf09c69..3e17255737b2 100644\n> --- a/src/ipa/raspberrypi/cam_helper.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> @@ -34,9 +34,8 @@ CamHelper *CamHelper::Create(std::string const &cam_name)\n> \treturn nullptr;\n> }\n>\n> -CamHelper::CamHelper(MdParser *parser, unsigned int maxFrameLength,\n> -\t\t unsigned int frameIntegrationDiff)\n> -\t: parser_(parser), initialized_(false), maxFrameLength_(maxFrameLength),\n> +CamHelper::CamHelper(MdParser *parser, unsigned int frameIntegrationDiff)\n> +\t: parser_(parser), initialized_(false),\n> \t frameIntegrationDiff_(frameIntegrationDiff)\n> {\n> }\n> @@ -67,13 +66,12 @@ uint32_t CamHelper::GetVBlanking(double &exposure, double minFrameDuration,\n> \tassert(initialized_);\n>\n> \t/*\n> -\t * Clamp frame length by the frame duration range and the maximum allowable\n> -\t * value in the sensor, given by maxFrameLength_.\n> +\t * minFrameDuration and maxFrameDuration will have been clamped to the\n> +\t * maximum allowable values in the sensor for this mode.\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> +\tframeLengthMin = 1e3 * minFrameDuration / mode_.line_length;\n> +\tframeLengthMax = 1e3 * maxFrameDuration / mode_.line_length;\n> +\n> \t/*\n> \t * Limit the exposure to the maximum frame duration requested, and\n> \t * re-calculate if it has been clipped.\n> diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp\n> index b1739a57e342..14d70112cb62 100644\n> --- a/src/ipa/raspberrypi/cam_helper.hpp\n> +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> @@ -62,8 +62,7 @@ class CamHelper\n> {\n> public:\n> \tstatic CamHelper *Create(std::string const &cam_name);\n> -\tCamHelper(MdParser *parser, unsigned int maxFrameLength,\n> -\t\t unsigned int frameIntegrationDiff);\n> +\tCamHelper(MdParser *parser, unsigned int frameIntegrationDiff);\n> \tvirtual ~CamHelper();\n> \tvoid SetCameraMode(const CameraMode &mode);\n> \tMdParser &Parser() const { return *parser_; }\n> @@ -86,8 +85,6 @@ protected:\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> diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> index 8688ec091c44..95b8e698fe3b 100644\n> --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> @@ -56,15 +56,13 @@ private:\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(), maxFrameLength, frameIntegrationDiff)\n> +\t: CamHelper(new MdParserImx219(), frameIntegrationDiff)\n> #else\n> -\t: CamHelper(new MdParserRPi(), maxFrameLength, frameIntegrationDiff)\n> +\t: CamHelper(new MdParserRPi(), 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 5396131003f1..eaa7be16d20e 100644\n> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> @@ -45,12 +45,10 @@ private:\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(), maxFrameLength, frameIntegrationDiff)\n> +\t: CamHelper(new MdParserImx477(), 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 2bd8a754f133..a7f417324048 100644\n> --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> @@ -30,8 +30,6 @@ private:\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> @@ -40,7 +38,7 @@ private:\n> */\n>\n> CamHelperOv5647::CamHelperOv5647()\n> -\t: CamHelper(new MdParserRPi(), maxFrameLength, frameIntegrationDiff)\n> +\t: CamHelper(new MdParserRPi(), frameIntegrationDiff)\n> {\n> }\n>\n> diff --git a/src/ipa/raspberrypi/controller/camera_mode.h b/src/ipa/raspberrypi/controller/camera_mode.h\n> index 920f11beb0b0..3baeadaca076 100644\n> --- a/src/ipa/raspberrypi/controller/camera_mode.h\n> +++ b/src/ipa/raspberrypi/controller/camera_mode.h\n> @@ -37,6 +37,8 @@ struct CameraMode {\n> \tdouble line_length;\n> \t// any camera transform *not* reflected already in the camera tuning\n> \tlibcamera::Transform transform;\n> +\t// minimum and maximum vblanking limits for this mode\n> +\tuint32_t vblank_min, vblank_max;\n> };\n>\n> #ifdef __cplusplus\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 5ccc7a6551f5..9e6e030c4997 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -102,6 +102,7 @@ private:\n> \tvoid reportMetadata();\n> \tbool parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &deviceStatus);\n> \tvoid processStats(unsigned int bufferId);\n> +\tvoid applyFrameDurations(double min, double max);\n> \tvoid applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);\n> \tvoid applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls);\n> \tvoid applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls);\n> @@ -279,6 +280,14 @@ void IPARPi::setMode(const CameraSensorInfo &sensorInfo)\n> \t * the line length in pixels and the pixel rate.\n> \t */\n> \tmode_.line_length = 1e9 * sensorInfo.lineLength / sensorInfo.pixelRate;\n> +\n> +\t/*\n> +\t * Set the vertical blanking (frame duration) limits for the mode to\n> +\t * ensure exposure and framerate calculations are clipped appropriately.\n> +\t */\n> +\tconst auto itVblank = sensorCtrls_.find(V4L2_CID_VBLANK);\n> +\tmode_.vblank_min = itVblank->second.min().get();\n> +\tmode_.vblank_max = itVblank->second.max().get();\n> }\n>\n> void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> @@ -384,8 +393,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> \t\tcontroller_.Initialise();\n> \t\tcontrollerInit_ = true;\n>\n> -\t\tminFrameDuration_ = defaultMinFrameDuration;\n> -\t\tmaxFrameDuration_ = defaultMaxFrameDuration;\n> +\t\t/* Supply initial values for frame durations. */\n> +\t\tapplyFrameDurations(defaultMinFrameDuration, defaultMaxFrameDuration);\n>\n> \t\t/* Supply initial values for gain and exposure. */\n> \t\tControlList ctrls(sensorCtrls_);\n> @@ -819,20 +828,7 @@ void IPARPi::queueRequest(const ControlList &controls)\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> -\t\t\tmaxFrameDuration_ = std::max(maxFrameDuration_, minFrameDuration_);\n> -\n> -\t\t\t/*\n> -\t\t\t * \\todo The values returned in the metadata below must be\n> -\t\t\t * correctly clipped by what the sensor mode supports and\n> -\t\t\t * what the AGC exposure mode or manual shutter speed limits\n> -\t\t\t */\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> +\t\t\tapplyFrameDurations(frameDurations[0], frameDurations[1]);\n> \t\t\tbreak;\n> \t\t}\n>\n> @@ -991,6 +987,29 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)\n> \t\t static_cast(awbStatus->gain_b * 1000));\n> }\n>\n> +void IPARPi::applyFrameDurations(double min, double max)\n> +{\n> +\tconst double minSensorFrameDuration = 1e-3 * (mode_.vblank_min + mode_.height) *\n> +\t\t\t\t\t mode_.line_length;\n> +\tconst double maxSensorFrameDuration = 1e-3 * (mode_.vblank_max + mode_.height) *\n> +\t\t\t\t\t mode_.line_length;\n> +\t/*\n> +\t * This will only be applied once AGC recalculations occur.\n> +\t * The values may be clamped based on the sensor mode capabilities as well.\n> +\t */\n> +\tminFrameDuration_ = min ? min : defaultMaxFrameDuration;\n> +\tmaxFrameDuration_ = max ? max : defaultMinFrameDuration;\n> +\tminFrameDuration_ = std::clamp(minFrameDuration_,\n> +\t\t\t\t minSensorFrameDuration, maxSensorFrameDuration);\n> +\tmaxFrameDuration_ = std::clamp(maxFrameDuration_,\n> +\t\t\t\t minSensorFrameDuration, maxSensorFrameDuration);\n> +\n> +\t/* Return the validated limits out though metadata. */\n> +\tlibcameraMetadata_.set(controls::FrameDurations,\n> +\t\t\t { static_cast(minFrameDuration_),\n> +\t\t\t\t static_cast(maxFrameDuration_) });\n> +}\n> +\n> void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)\n> {\n> \tint32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);\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 D6B9DBD808\n\tfor ;\n\tMon, 25 Jan 2021 09:38:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7A4D8682A3;\n\tMon, 25 Jan 2021 10:38:24 +0100 (CET)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 45B016030B\n\tfor ;\n\tMon, 25 Jan 2021 10:38:23 +0100 (CET)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id B4EC424000F;\n\tMon, 25 Jan 2021 09:38:22 +0000 (UTC)"],"Date":"Mon, 25 Jan 2021 10:38:43 +0100","From":"Jacopo Mondi ","To":"Naushir Patuck ","Message-ID":"<20210125093843.anideq5ijrhwdsg7@uno.localdomain>","References":"<20210121115849.682130-1-naush@raspberrypi.com>\n\t<20210121115849.682130-3-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210121115849.682130-3-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH 2/4] ipa: raspberrypi: Limit the\n\tcalculated vblank based on the sensor mode","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":14762,"web_url":"https://patchwork.libcamera.org/comment/14762/","msgid":"","date":"2021-01-25T12:49:46","subject":"Re: [libcamera-devel] [PATCH 2/4] ipa: raspberrypi: Limit the\n\tcalculated vblank based on the sensor mode","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Jacopo,\n\nOn Mon, 25 Jan 2021 at 09:38, Jacopo Mondi wrote:\n\n> Hi Nuash,\n>\n> On Thu, Jan 21, 2021 at 11:58:47AM +0000, Naushir Patuck wrote:\n> > The existing framerate/vblank calculations did not account for the\n> > sensor mode limits. This commit extracts vblank limits from the sensor\n> > v4l2 controls, and stores it in the camera modes structure.\n> >\n> > Exposure and vblank value calculations now use values clamped to the\n> > sensor mode limits. The libcamera::controls::FrameDurations metadata\n> > return values now reflect the minimum and maximum after clamping.\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > src/ipa/raspberrypi/cam_helper.cpp | 16 +++---\n> > src/ipa/raspberrypi/cam_helper.hpp | 5 +-\n> > src/ipa/raspberrypi/cam_helper_imx219.cpp | 6 +--\n> > src/ipa/raspberrypi/cam_helper_imx477.cpp | 4 +-\n> > src/ipa/raspberrypi/cam_helper_ov5647.cpp | 4 +-\n> > src/ipa/raspberrypi/controller/camera_mode.h | 2 +\n> > src/ipa/raspberrypi/raspberrypi.cpp | 51 ++++++++++++++------\n>\n> Any reason I am missing on why you kept this change private to your\n> IPA instead of making vblank part of the CameraSensorInfo contructed\n> by the CameraSensor class ?\n>\n\nDid you mean adding vblank min/max values to the CameraSensorInfo class?\nIf so, sure I can add that and lookup the values in the Raspberry Pi IPA.\nNote that I would still need the values available in the CameraMode (by\ncopying from the CameraSensorInfo field). This object gets passed into our\ncontrollers, and at some point when I have enabled long exposures on\nimx477, I will need these values in our cam helper class.\n\n\nRegards,\nNaush\n\n\n> Thanks\n> j\n>\n> > 7 files changed, 49 insertions(+), 39 deletions(-)\n> >\n> > diff --git a/src/ipa/raspberrypi/cam_helper.cpp\n> b/src/ipa/raspberrypi/cam_helper.cpp\n> > index b7b8faf09c69..3e17255737b2 100644\n> > --- a/src/ipa/raspberrypi/cam_helper.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> > @@ -34,9 +34,8 @@ CamHelper *CamHelper::Create(std::string const\n> &cam_name)\n> > return nullptr;\n> > }\n> >\n> > -CamHelper::CamHelper(MdParser *parser, unsigned int maxFrameLength,\n> > - unsigned int frameIntegrationDiff)\n> > - : parser_(parser), initialized_(false),\n> maxFrameLength_(maxFrameLength),\n> > +CamHelper::CamHelper(MdParser *parser, unsigned int\n> frameIntegrationDiff)\n> > + : parser_(parser), initialized_(false),\n> > frameIntegrationDiff_(frameIntegrationDiff)\n> > {\n> > }\n> > @@ -67,13 +66,12 @@ uint32_t CamHelper::GetVBlanking(double &exposure,\n> double minFrameDuration,\n> > assert(initialized_);\n> >\n> > /*\n> > - * Clamp frame length by the frame duration range and the maximum\n> allowable\n> > - * value in the sensor, given by maxFrameLength_.\n> > + * minFrameDuration and maxFrameDuration will have been clamped to\n> the\n> > + * maximum allowable values in the sensor for this mode.\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> > + frameLengthMin = 1e3 * minFrameDuration / mode_.line_length;\n> > + frameLengthMax = 1e3 * maxFrameDuration / mode_.line_length;\n> > +\n> > /*\n> > * Limit the exposure to the maximum frame duration requested, and\n> > * re-calculate if it has been clipped.\n> > diff --git a/src/ipa/raspberrypi/cam_helper.hpp\n> b/src/ipa/raspberrypi/cam_helper.hpp\n> > index b1739a57e342..14d70112cb62 100644\n> > --- a/src/ipa/raspberrypi/cam_helper.hpp\n> > +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> > @@ -62,8 +62,7 @@ class CamHelper\n> > {\n> > public:\n> > static CamHelper *Create(std::string const &cam_name);\n> > - CamHelper(MdParser *parser, unsigned int maxFrameLength,\n> > - unsigned int frameIntegrationDiff);\n> > + CamHelper(MdParser *parser, unsigned int frameIntegrationDiff);\n> > virtual ~CamHelper();\n> > void SetCameraMode(const CameraMode &mode);\n> > MdParser &Parser() const { return *parser_; }\n> > @@ -86,8 +85,6 @@ protected:\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> > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > index 8688ec091c44..95b8e698fe3b 100644\n> > --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > @@ -56,15 +56,13 @@ private:\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(), maxFrameLength,\n> frameIntegrationDiff)\n> > + : CamHelper(new MdParserImx219(), frameIntegrationDiff)\n> > #else\n> > - : CamHelper(new MdParserRPi(), maxFrameLength,\n> frameIntegrationDiff)\n> > + : CamHelper(new MdParserRPi(), 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 5396131003f1..eaa7be16d20e 100644\n> > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > @@ -45,12 +45,10 @@ private:\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(), maxFrameLength,\n> frameIntegrationDiff)\n> > + : CamHelper(new MdParserImx477(), 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 2bd8a754f133..a7f417324048 100644\n> > --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> > @@ -30,8 +30,6 @@ private:\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> > @@ -40,7 +38,7 @@ private:\n> > */\n> >\n> > CamHelperOv5647::CamHelperOv5647()\n> > - : CamHelper(new MdParserRPi(), maxFrameLength,\n> frameIntegrationDiff)\n> > + : CamHelper(new MdParserRPi(), frameIntegrationDiff)\n> > {\n> > }\n> >\n> > diff --git a/src/ipa/raspberrypi/controller/camera_mode.h\n> b/src/ipa/raspberrypi/controller/camera_mode.h\n> > index 920f11beb0b0..3baeadaca076 100644\n> > --- a/src/ipa/raspberrypi/controller/camera_mode.h\n> > +++ b/src/ipa/raspberrypi/controller/camera_mode.h\n> > @@ -37,6 +37,8 @@ struct CameraMode {\n> > double line_length;\n> > // any camera transform *not* reflected already in the camera\n> tuning\n> > libcamera::Transform transform;\n> > + // minimum and maximum vblanking limits for this mode\n> > + uint32_t vblank_min, vblank_max;\n> > };\n> >\n> > #ifdef __cplusplus\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 5ccc7a6551f5..9e6e030c4997 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -102,6 +102,7 @@ private:\n> > void reportMetadata();\n> > bool parseEmbeddedData(unsigned int bufferId, struct DeviceStatus\n> &deviceStatus);\n> > void processStats(unsigned int bufferId);\n> > + void applyFrameDurations(double min, double max);\n> > void applyAGC(const struct AgcStatus *agcStatus, ControlList\n> &ctrls);\n> > void applyAWB(const struct AwbStatus *awbStatus, ControlList\n> &ctrls);\n> > void applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls);\n> > @@ -279,6 +280,14 @@ void IPARPi::setMode(const CameraSensorInfo\n> &sensorInfo)\n> > * the line length in pixels and the pixel rate.\n> > */\n> > mode_.line_length = 1e9 * sensorInfo.lineLength /\n> sensorInfo.pixelRate;\n> > +\n> > + /*\n> > + * Set the vertical blanking (frame duration) limits for the mode\n> to\n> > + * ensure exposure and framerate calculations are clipped\n> appropriately.\n> > + */\n> > + const auto itVblank = sensorCtrls_.find(V4L2_CID_VBLANK);\n> > + mode_.vblank_min = itVblank->second.min().get();\n> > + mode_.vblank_max = itVblank->second.max().get();\n> > }\n> >\n> > void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > @@ -384,8 +393,8 @@ void IPARPi::configure(const CameraSensorInfo\n> &sensorInfo,\n> > controller_.Initialise();\n> > controllerInit_ = true;\n> >\n> > - minFrameDuration_ = defaultMinFrameDuration;\n> > - maxFrameDuration_ = defaultMaxFrameDuration;\n> > + /* Supply initial values for frame durations. */\n> > + applyFrameDurations(defaultMinFrameDuration,\n> defaultMaxFrameDuration);\n> >\n> > /* Supply initial values for gain and exposure. */\n> > ControlList ctrls(sensorCtrls_);\n> > @@ -819,20 +828,7 @@ void IPARPi::queueRequest(const ControlList\n> &controls)\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> > - maxFrameDuration_ = std::max(maxFrameDuration_,\n> minFrameDuration_);\n> > -\n> > - /*\n> > - * \\todo The values returned in the metadata below\n> must be\n> > - * correctly clipped by what the sensor mode\n> supports and\n> > - * what the AGC exposure mode or manual shutter\n> speed limits\n> > - */\n> > - libcameraMetadata_.set(controls::FrameDurations,\n> > - {\n> static_cast(minFrameDuration_),\n> > -\n> static_cast(maxFrameDuration_) });\n> > + applyFrameDurations(frameDurations[0],\n> frameDurations[1]);\n> > break;\n> > }\n> >\n> > @@ -991,6 +987,29 @@ void IPARPi::applyAWB(const struct AwbStatus\n> *awbStatus, ControlList &ctrls)\n> > static_cast(awbStatus->gain_b * 1000));\n> > }\n> >\n> > +void IPARPi::applyFrameDurations(double min, double max)\n> > +{\n> > + const double minSensorFrameDuration = 1e-3 * (mode_.vblank_min +\n> mode_.height) *\n> > + mode_.line_length;\n> > + const double maxSensorFrameDuration = 1e-3 * (mode_.vblank_max +\n> mode_.height) *\n> > + mode_.line_length;\n> > + /*\n> > + * This will only be applied once AGC recalculations occur.\n> > + * The values may be clamped based on the sensor mode capabilities\n> as well.\n> > + */\n> > + minFrameDuration_ = min ? min : defaultMaxFrameDuration;\n> > + maxFrameDuration_ = max ? max : defaultMinFrameDuration;\n> > + minFrameDuration_ = std::clamp(minFrameDuration_,\n> > + minSensorFrameDuration,\n> maxSensorFrameDuration);\n> > + maxFrameDuration_ = std::clamp(maxFrameDuration_,\n> > + minSensorFrameDuration,\n> maxSensorFrameDuration);\n> > +\n> > + /* Return the validated limits out though metadata. */\n> > + libcameraMetadata_.set(controls::FrameDurations,\n> > + { static_cast(minFrameDuration_),\n> > + static_cast(maxFrameDuration_)\n> });\n> > +}\n> > +\n> > void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList\n> &ctrls)\n> > {\n> > int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);\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 B5913BD808\n\tfor ;\n\tMon, 25 Jan 2021 12:50:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3CBEF682CF;\n\tMon, 25 Jan 2021 13:50:06 +0100 (CET)","from mail-lf1-x133.google.com (mail-lf1-x133.google.com\n\t[IPv6:2a00:1450:4864:20::133])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 220A8682BB\n\tfor ;\n\tMon, 25 Jan 2021 13:50:04 +0100 (CET)","by mail-lf1-x133.google.com with SMTP id h7so17544055lfc.6\n\tfor ;\n\tMon, 25 Jan 2021 04:50:04 -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=\"qELtpZ2k\"; 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=Stv2v5/cai+6IgNzd6x1ivw4LANaNBhVKf+P/TwAh+s=;\n\tb=qELtpZ2k/mdKokW9w8vymMhxUF7ElBIR8oYeVxAX5pQ7qacP+KXtd+Qf5y1SewiBJq\n\tUMYnql3Uzg8USR7W5wgiWve30xnEVLK1o8n6OcTOjO8WZqL7oTl3GUQQVz0lI0cxCf+8\n\tqGo80y/8FknvRNk/M+zByk3sVlBxD6FaoqUFZxYKycg74AJ1YCBdYXJplRFxai7CX90C\n\tmYeCm2P3UtktsPo/uVT9wfiYlbk1eGCusjmSBAgwoUAs9hgWjRwpR0uCj9prrySupW97\n\t2jkciqPYiSxfIjFr2Dh7G/RLfcY4qKQ7fnLmYlBelH4ekFOr329ZS9/EyjCMPhLBZGG4\n\tAdOg==","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=Stv2v5/cai+6IgNzd6x1ivw4LANaNBhVKf+P/TwAh+s=;\n\tb=BV78Gp6D77r6FkzbWZUD5QQ6s97VVlnpH/njEOY6zjU5hmsKDqosgdhUXfoFyjRc0E\n\tue9/Qp/oT2yBhaJX3grr2jvUYVqMmSUgLELHhcHVVIK/QHo7ZNDm7XGt7i+edfN70OHl\n\t6DlYKUKH1CsZmpN7PxIF3hd6n4nsSoZpixGhpjslEAgIYUlhoert5IVMuSQ66vQPAXPP\n\tVjWeudzRbEOnc9TdMCNWAwGoMNjxGa0Ubu/ScAheHoaq2rsoXfmhY9ZfM+gr43ZnTCI0\n\tJIrWLBoV0syMGMqsIg6MJAY7HiAY3KgMUI8tPqkp6qwgAr0wOBMcr9gyd2UqdwaAyvz1\n\t6Dsg==","X-Gm-Message-State":"AOAM533tIuYS+Z5c4sIfCRMxu7cRyOb4rMw0R7gQbKw59p2+1zB6Y4zE\n\tFigrhaXA5XClUj50S2z8z31ZBZqq07WMQFJ5ePtdGATgQOsg/jKN","X-Google-Smtp-Source":"ABdhPJzuz7LOcCjERQkYP+G0Q8MTcBLxSjZHYUrhlPHS7zfu9rV4HnK5CSmy3ApdcR/aLrpzk7YUY16p/szj9Q+MUug=","X-Received":"by 2002:ac2:5f5b:: with SMTP id 27mr193528lfz.375.1611579003343; \n\tMon, 25 Jan 2021 04:50:03 -0800 (PST)","MIME-Version":"1.0","References":"<20210121115849.682130-1-naush@raspberrypi.com>\n\t<20210121115849.682130-3-naush@raspberrypi.com>\n\t<20210125093843.anideq5ijrhwdsg7@uno.localdomain>","In-Reply-To":"<20210125093843.anideq5ijrhwdsg7@uno.localdomain>","From":"Naushir Patuck ","Date":"Mon, 25 Jan 2021 12:49:46 +0000","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH 2/4] ipa: raspberrypi: Limit the\n\tcalculated vblank based on the sensor mode","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=\"===============2020732632186929919==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14764,"web_url":"https://patchwork.libcamera.org/comment/14764/","msgid":"<20210125131603.swhtfxjqeabq3wil@uno.localdomain>","date":"2021-01-25T13:16:03","subject":"Re: [libcamera-devel] [PATCH 2/4] ipa: raspberrypi: Limit the\n\tcalculated vblank based on the sensor mode","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Naush,\n\nOn Mon, Jan 25, 2021 at 12:49:46PM +0000, Naushir Patuck wrote:\n> Hi Jacopo,\n>\n> On Mon, 25 Jan 2021 at 09:38, Jacopo Mondi wrote:\n>\n> > Hi Nuash,\n> >\n> > On Thu, Jan 21, 2021 at 11:58:47AM +0000, Naushir Patuck wrote:\n> > > The existing framerate/vblank calculations did not account for the\n> > > sensor mode limits. This commit extracts vblank limits from the sensor\n> > > v4l2 controls, and stores it in the camera modes structure.\n> > >\n> > > Exposure and vblank value calculations now use values clamped to the\n> > > sensor mode limits. The libcamera::controls::FrameDurations metadata\n> > > return values now reflect the minimum and maximum after clamping.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > ---\n> > > src/ipa/raspberrypi/cam_helper.cpp | 16 +++---\n> > > src/ipa/raspberrypi/cam_helper.hpp | 5 +-\n> > > src/ipa/raspberrypi/cam_helper_imx219.cpp | 6 +--\n> > > src/ipa/raspberrypi/cam_helper_imx477.cpp | 4 +-\n> > > src/ipa/raspberrypi/cam_helper_ov5647.cpp | 4 +-\n> > > src/ipa/raspberrypi/controller/camera_mode.h | 2 +\n> > > src/ipa/raspberrypi/raspberrypi.cpp | 51 ++++++++++++++------\n> >\n> > Any reason I am missing on why you kept this change private to your\n> > IPA instead of making vblank part of the CameraSensorInfo contructed\n> > by the CameraSensor class ?\n> >\n>\n> Did you mean adding vblank min/max values to the CameraSensorInfo class?\n> If so, sure I can add that and lookup the values in the Raspberry Pi IPA.\n> Note that I would still need the values available in the CameraMode (by\n> copying from the CameraSensorInfo field). This object gets passed into our\n> controllers, and at some point when I have enabled long exposures on\n> imx477, I will need these values in our cam helper class.\n>\n\nI see you need the control limits, not just the value itself.. This\nmight seems philosophical as a distinction, but CameraSensorInfo reports\ninformation about the current configuration, so indeed having limits\nthere might not fit that well. Furthermore you have access to the control\nlimits from the pipeline, your solution might be the most correct one\nat the moment.. My thinking was to find a direction where this\ninformation will be made available to all IPA, without them having to\nreplicate this same implementation, but at the current time that's\nprobably the best we can do.\n\nI'll review the rest of the series shortly then.\n\nThanks\n j\n\n>\n> Regards,\n> Naush\n>\n>\n> > Thanks\n> > j\n> >\n> > > 7 files changed, 49 insertions(+), 39 deletions(-)\n> > >\n> > > diff --git a/src/ipa/raspberrypi/cam_helper.cpp\n> > b/src/ipa/raspberrypi/cam_helper.cpp\n> > > index b7b8faf09c69..3e17255737b2 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper.cpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> > > @@ -34,9 +34,8 @@ CamHelper *CamHelper::Create(std::string const\n> > &cam_name)\n> > > return nullptr;\n> > > }\n> > >\n> > > -CamHelper::CamHelper(MdParser *parser, unsigned int maxFrameLength,\n> > > - unsigned int frameIntegrationDiff)\n> > > - : parser_(parser), initialized_(false),\n> > maxFrameLength_(maxFrameLength),\n> > > +CamHelper::CamHelper(MdParser *parser, unsigned int\n> > frameIntegrationDiff)\n> > > + : parser_(parser), initialized_(false),\n> > > frameIntegrationDiff_(frameIntegrationDiff)\n> > > {\n> > > }\n> > > @@ -67,13 +66,12 @@ uint32_t CamHelper::GetVBlanking(double &exposure,\n> > double minFrameDuration,\n> > > assert(initialized_);\n> > >\n> > > /*\n> > > - * Clamp frame length by the frame duration range and the maximum\n> > allowable\n> > > - * value in the sensor, given by maxFrameLength_.\n> > > + * minFrameDuration and maxFrameDuration will have been clamped to\n> > the\n> > > + * maximum allowable values in the sensor for this mode.\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> > > + frameLengthMin = 1e3 * minFrameDuration / mode_.line_length;\n> > > + frameLengthMax = 1e3 * maxFrameDuration / mode_.line_length;\n> > > +\n> > > /*\n> > > * Limit the exposure to the maximum frame duration requested, and\n> > > * re-calculate if it has been clipped.\n> > > diff --git a/src/ipa/raspberrypi/cam_helper.hpp\n> > b/src/ipa/raspberrypi/cam_helper.hpp\n> > > index b1739a57e342..14d70112cb62 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper.hpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> > > @@ -62,8 +62,7 @@ class CamHelper\n> > > {\n> > > public:\n> > > static CamHelper *Create(std::string const &cam_name);\n> > > - CamHelper(MdParser *parser, unsigned int maxFrameLength,\n> > > - unsigned int frameIntegrationDiff);\n> > > + CamHelper(MdParser *parser, unsigned int frameIntegrationDiff);\n> > > virtual ~CamHelper();\n> > > void SetCameraMode(const CameraMode &mode);\n> > > MdParser &Parser() const { return *parser_; }\n> > > @@ -86,8 +85,6 @@ protected:\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> > > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > > index 8688ec091c44..95b8e698fe3b 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > > @@ -56,15 +56,13 @@ private:\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(), maxFrameLength,\n> > frameIntegrationDiff)\n> > > + : CamHelper(new MdParserImx219(), frameIntegrationDiff)\n> > > #else\n> > > - : CamHelper(new MdParserRPi(), maxFrameLength,\n> > frameIntegrationDiff)\n> > > + : CamHelper(new MdParserRPi(), 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 5396131003f1..eaa7be16d20e 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > > @@ -45,12 +45,10 @@ private:\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(), maxFrameLength,\n> > frameIntegrationDiff)\n> > > + : CamHelper(new MdParserImx477(), 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 2bd8a754f133..a7f417324048 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> > > @@ -30,8 +30,6 @@ private:\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> > > @@ -40,7 +38,7 @@ private:\n> > > */\n> > >\n> > > CamHelperOv5647::CamHelperOv5647()\n> > > - : CamHelper(new MdParserRPi(), maxFrameLength,\n> > frameIntegrationDiff)\n> > > + : CamHelper(new MdParserRPi(), frameIntegrationDiff)\n> > > {\n> > > }\n> > >\n> > > diff --git a/src/ipa/raspberrypi/controller/camera_mode.h\n> > b/src/ipa/raspberrypi/controller/camera_mode.h\n> > > index 920f11beb0b0..3baeadaca076 100644\n> > > --- a/src/ipa/raspberrypi/controller/camera_mode.h\n> > > +++ b/src/ipa/raspberrypi/controller/camera_mode.h\n> > > @@ -37,6 +37,8 @@ struct CameraMode {\n> > > double line_length;\n> > > // any camera transform *not* reflected already in the camera\n> > tuning\n> > > libcamera::Transform transform;\n> > > + // minimum and maximum vblanking limits for this mode\n> > > + uint32_t vblank_min, vblank_max;\n> > > };\n> > >\n> > > #ifdef __cplusplus\n> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> > b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > index 5ccc7a6551f5..9e6e030c4997 100644\n> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > @@ -102,6 +102,7 @@ private:\n> > > void reportMetadata();\n> > > bool parseEmbeddedData(unsigned int bufferId, struct DeviceStatus\n> > &deviceStatus);\n> > > void processStats(unsigned int bufferId);\n> > > + void applyFrameDurations(double min, double max);\n> > > void applyAGC(const struct AgcStatus *agcStatus, ControlList\n> > &ctrls);\n> > > void applyAWB(const struct AwbStatus *awbStatus, ControlList\n> > &ctrls);\n> > > void applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls);\n> > > @@ -279,6 +280,14 @@ void IPARPi::setMode(const CameraSensorInfo\n> > &sensorInfo)\n> > > * the line length in pixels and the pixel rate.\n> > > */\n> > > mode_.line_length = 1e9 * sensorInfo.lineLength /\n> > sensorInfo.pixelRate;\n> > > +\n> > > + /*\n> > > + * Set the vertical blanking (frame duration) limits for the mode\n> > to\n> > > + * ensure exposure and framerate calculations are clipped\n> > appropriately.\n> > > + */\n> > > + const auto itVblank = sensorCtrls_.find(V4L2_CID_VBLANK);\n> > > + mode_.vblank_min = itVblank->second.min().get();\n> > > + mode_.vblank_max = itVblank->second.max().get();\n> > > }\n> > >\n> > > void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> > > @@ -384,8 +393,8 @@ void IPARPi::configure(const CameraSensorInfo\n> > &sensorInfo,\n> > > controller_.Initialise();\n> > > controllerInit_ = true;\n> > >\n> > > - minFrameDuration_ = defaultMinFrameDuration;\n> > > - maxFrameDuration_ = defaultMaxFrameDuration;\n> > > + /* Supply initial values for frame durations. */\n> > > + applyFrameDurations(defaultMinFrameDuration,\n> > defaultMaxFrameDuration);\n> > >\n> > > /* Supply initial values for gain and exposure. */\n> > > ControlList ctrls(sensorCtrls_);\n> > > @@ -819,20 +828,7 @@ void IPARPi::queueRequest(const ControlList\n> > &controls)\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> > > - maxFrameDuration_ = std::max(maxFrameDuration_,\n> > minFrameDuration_);\n> > > -\n> > > - /*\n> > > - * \\todo The values returned in the metadata below\n> > must be\n> > > - * correctly clipped by what the sensor mode\n> > supports and\n> > > - * what the AGC exposure mode or manual shutter\n> > speed limits\n> > > - */\n> > > - libcameraMetadata_.set(controls::FrameDurations,\n> > > - {\n> > static_cast(minFrameDuration_),\n> > > -\n> > static_cast(maxFrameDuration_) });\n> > > + applyFrameDurations(frameDurations[0],\n> > frameDurations[1]);\n> > > break;\n> > > }\n> > >\n> > > @@ -991,6 +987,29 @@ void IPARPi::applyAWB(const struct AwbStatus\n> > *awbStatus, ControlList &ctrls)\n> > > static_cast(awbStatus->gain_b * 1000));\n> > > }\n> > >\n> > > +void IPARPi::applyFrameDurations(double min, double max)\n> > > +{\n> > > + const double minSensorFrameDuration = 1e-3 * (mode_.vblank_min +\n> > mode_.height) *\n> > > + mode_.line_length;\n> > > + const double maxSensorFrameDuration = 1e-3 * (mode_.vblank_max +\n> > mode_.height) *\n> > > + mode_.line_length;\n> > > + /*\n> > > + * This will only be applied once AGC recalculations occur.\n> > > + * The values may be clamped based on the sensor mode capabilities\n> > as well.\n> > > + */\n> > > + minFrameDuration_ = min ? min : defaultMaxFrameDuration;\n> > > + maxFrameDuration_ = max ? max : defaultMinFrameDuration;\n> > > + minFrameDuration_ = std::clamp(minFrameDuration_,\n> > > + minSensorFrameDuration,\n> > maxSensorFrameDuration);\n> > > + maxFrameDuration_ = std::clamp(maxFrameDuration_,\n> > > + minSensorFrameDuration,\n> > maxSensorFrameDuration);\n> > > +\n> > > + /* Return the validated limits out though metadata. */\n> > > + libcameraMetadata_.set(controls::FrameDurations,\n> > > + { static_cast(minFrameDuration_),\n> > > + static_cast(maxFrameDuration_)\n> > });\n> > > +}\n> > > +\n> > > void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList\n> > &ctrls)\n> > > {\n> > > int32_t gainCode = helper_->GainCode(agcStatus->analogue_gain);\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 6F0B2BD808\n\tfor ;\n\tMon, 25 Jan 2021 13:15:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 056D9682D0;\n\tMon, 25 Jan 2021 14:15:45 +0100 (CET)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D959B682C8\n\tfor ;\n\tMon, 25 Jan 2021 14:15:43 +0100 (CET)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 54E591BF20D;\n\tMon, 25 Jan 2021 13:15:43 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Mon, 25 Jan 2021 14:16:03 +0100","From":"Jacopo Mondi ","To":"Naushir Patuck ","Message-ID":"<20210125131603.swhtfxjqeabq3wil@uno.localdomain>","References":"<20210121115849.682130-1-naush@raspberrypi.com>\n\t<20210121115849.682130-3-naush@raspberrypi.com>\n\t<20210125093843.anideq5ijrhwdsg7@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 2/4] ipa: raspberrypi: Limit the\n\tcalculated vblank based on the sensor mode","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\" "}}]