[{"id":14058,"web_url":"https://patchwork.libcamera.org/comment/14058/","msgid":"","date":"2020-12-04T16:01:42","subject":"Re: [libcamera-devel] [PATCH 4/5] src: ipa: raspberrypi: Move\n\tinitial frame drop decision to AGC","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 patch.\n\nOn Wed, 2 Dec 2020 at 11:53, David Plowman \nwrote:\n\n> Previously the CamHelper was returning the number of frames to drop\n> (on account of AGC converging). This wasn't really appropriate, it's\n> better for the AGC to do it, which now also knows when exposure and\n> gain have been explicitly set and therefore fewer (or no) frame drops\n> are necessary at all.\n>\n> The CamHelper::HideFramesStartup method should now just be returning\n> the number of frames to hide because they're bad/invalid in some way,\n> not worrying about the AGC. For many sensors, the correct value for\n> this is zero.\n>\n> Signed-off-by: David Plowman \n> ---\n> src/ipa/raspberrypi/cam_helper.cpp | 6 +++---\n> src/ipa/raspberrypi/raspberrypi.cpp | 8 ++++++++\n> 2 files changed, 11 insertions(+), 3 deletions(-)\n>\n> diff --git a/src/ipa/raspberrypi/cam_helper.cpp\n> b/src/ipa/raspberrypi/cam_helper.cpp\n> index c8ac3232..6efa0d7f 100644\n> --- a/src/ipa/raspberrypi/cam_helper.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> @@ -82,10 +82,10 @@ bool CamHelper::SensorEmbeddedDataPresent() const\n> unsigned int CamHelper::HideFramesStartup() const\n> {\n> /*\n> - * By default, hide 6 frames completely at start-up while AGC etc.\n> sort\n> - * themselves out (converge).\n> + * The number of frames when a camera first starts that shouldn't\n> be\n> + * displayed as they are invalid in some way.\n> */\n> - return 6;\n> + return 0;\n> }\n>\n> unsigned int CamHelper::HideFramesModeSwitch() const\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 0300b8d9..ddabdb31 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -192,6 +192,14 @@ int IPARPi::start(const IPAOperationData &ipaConfig,\n> IPAOperationData *result)\n> unsigned int dropFrame = 0;\n> if (firstStart_) {\n> dropFrame = helper_->HideFramesStartup();\n> +\n> + /* The AGC algorithm may want us to drop more frames. */\n> + RPiController::AgcAlgorithm *agc =\n> dynamic_cast(\n> + controller_.GetAlgorithm(\"agc\"));\n> + if (agc)\n> + dropFrame = std::max(dropFrame,\n> agc->GetDropFrames());\n> + LOG(IPARPI, Debug) << \"Drop \" << dropFrame << \" frames on\n> startup\";\n> +\n>\n\nAll looks good with this change, however, I have a possibly silly\nquestion. In the previous code, our startup frames would account for\nconvergence in AGC, AWB, and ALS. Here we are explicitly accounting for\nconvergence only in AGC since helper_->HideFramesStartup() will return 0\nby default. Does it matter? Should each derived CamHelper return a\nnon-zero number here?\n\nRegards,\nNaush\n\n\n\n> mistrustCount_ = helper_->MistrustFramesStartup();\n> } else {\n> dropFrame = helper_->HideFramesModeSwitch();\n> --\n> 2.20.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 C416FBE176\n\tfor ;\n\tFri, 4 Dec 2020 16:02:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 522D3635DE;\n\tFri, 4 Dec 2020 17:02:00 +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 CDC5E635D0\n\tfor ;\n\tFri, 4 Dec 2020 17:01:59 +0100 (CET)","by mail-lf1-x135.google.com with SMTP id s27so8318733lfp.5\n\tfor ;\n\tFri, 04 Dec 2020 08:01:59 -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=\"MwjpJl+y\"; 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=USQ7zs1SSsDSB0o2e8u9aaMP4T+WTnsSzONh6kikBhM=;\n\tb=MwjpJl+yWFm7DmkFL3OMAoeV30Vqv9KG93PPKTK+61pTyIrhkjROEBVgSNaLBF9F8P\n\teHBw6wmcUWNAyCiobXCNTwdkjiMUtmbRJptuG0a6qqMyoUAjQpuQYrm29mE+01CYaMaY\n\tokDixvmdPB2u4y8ruxTsy2NXDpIPdrZbDpnbl45u5/+eMtVoSOdl8tBpye5TYxzhFBTW\n\tE7RgkXrDODnrNcR13R02v3tpDZnvbWMPD5+AuFoKZa+NhxAfHUdsb6lE73ADRCReAW6G\n\t3LG/M02ZEIoC1U7RVltVyZFz/aVe6IE/nZG1d6SAayWMAIZmeb7LdU5U8ecFzGxu1z0A\n\t6Rmg==","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=USQ7zs1SSsDSB0o2e8u9aaMP4T+WTnsSzONh6kikBhM=;\n\tb=rlWFu21KqEZbMoiMiUmwlA7gLns8hF+QnizOMexZZzlzPkZ+MhD5EEyd/y1h7M8aXo\n\tL/bLOaVdOLk27UgBiJz6q+814HI0PzJySZRGYALUQAW5t+OUbL+enzg3NhYC7M1mi+DP\n\tjAl1bMHRsgPwB4mRZZ9U7OpCClssMi61MOvUHZrCfp8dWgBEaqgCvh7Yh+F1VPRClJD5\n\tI9muRRvceiSKc0Pit6q8u1cJDQz2Dca0l+36ZhToMy9e0ihrhXXSOA+W/Q27mUTbGgpq\n\t+4lnCjLFSM73GxfhY3BO9Hpfa2+nmM+vLhU0uG9iL1d/86SL02/iuYohwLJTPB43yzXs\n\tcESw==","X-Gm-Message-State":"AOAM533Dsii4+zD8FHP7qpRnqtHog0WVk3nucxvu0FdDekNK5sa1X0Vy\n\tsDrGthwA9jA9OgcM9l2j5bQZdRxK5AdTglEK1o12dg==","X-Google-Smtp-Source":"ABdhPJxIyj4GS72K8BhlYzBxNvzHS/6jztnx3LGko0vtzEHj1olkK5x/26yRTh8Se8tZ/4ts0U2dm1mvrQwg/2v+qZk=","X-Received":"by 2002:a05:6512:210b:: with SMTP id\n\tq11mr3797287lfr.238.1607097719134; \n\tFri, 04 Dec 2020 08:01:59 -0800 (PST)","MIME-Version":"1.0","References":"<20201202115253.14705-1-david.plowman@raspberrypi.com>\n\t<20201202115253.14705-5-david.plowman@raspberrypi.com>","In-Reply-To":"<20201202115253.14705-5-david.plowman@raspberrypi.com>","From":"Naushir Patuck ","Date":"Fri, 4 Dec 2020 16:01:42 +0000","Message-ID":"","To":"David Plowman ","Subject":"Re: [libcamera-devel] [PATCH 4/5] src: ipa: raspberrypi: Move\n\tinitial frame drop decision to AGC","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=\"===============3949310758588712289==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14080,"web_url":"https://patchwork.libcamera.org/comment/14080/","msgid":"","date":"2020-12-05T19:50:47","subject":"Re: [libcamera-devel] [PATCH 4/5] src: ipa: raspberrypi: Move\n\tinitial frame drop decision to AGC","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush and David,\n\nOn Fri, Dec 04, 2020 at 04:01:42PM +0000, Naushir Patuck wrote:\n> On Wed, 2 Dec 2020 at 11:53, David Plowman wrote:\n> \n> > Previously the CamHelper was returning the number of frames to drop\n> > (on account of AGC converging). This wasn't really appropriate, it's\n> > better for the AGC to do it, which now also knows when exposure and\n> > gain have been explicitly set and therefore fewer (or no) frame drops\n> > are necessary at all.\n> >\n> > The CamHelper::HideFramesStartup method should now just be returning\n> > the number of frames to hide because they're bad/invalid in some way,\n> > not worrying about the AGC. For many sensors, the correct value for\n> > this is zero.\n> >\n> > Signed-off-by: David Plowman \n> > ---\n> > src/ipa/raspberrypi/cam_helper.cpp | 6 +++---\n> > src/ipa/raspberrypi/raspberrypi.cpp | 8 ++++++++\n> > 2 files changed, 11 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/src/ipa/raspberrypi/cam_helper.cpp\n> > b/src/ipa/raspberrypi/cam_helper.cpp\n> > index c8ac3232..6efa0d7f 100644\n> > --- a/src/ipa/raspberrypi/cam_helper.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> > @@ -82,10 +82,10 @@ bool CamHelper::SensorEmbeddedDataPresent() const\n> > unsigned int CamHelper::HideFramesStartup() const\n> > {\n> > /*\n> > - * By default, hide 6 frames completely at start-up while AGC etc. sort\n> > - * themselves out (converge).\n> > + * The number of frames when a camera first starts that shouldn't be\n> > + * displayed as they are invalid in some way.\n> > */\n> > - return 6;\n> > + return 0;\n> > }\n> >\n> > unsigned int CamHelper::HideFramesModeSwitch() const\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> > b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 0300b8d9..ddabdb31 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -192,6 +192,14 @@ int IPARPi::start(const IPAOperationData &ipaConfig,\n> > IPAOperationData *result)\n> > unsigned int dropFrame = 0;\n> > if (firstStart_) {\n> > dropFrame = helper_->HideFramesStartup();\n> > +\n> > + /* The AGC algorithm may want us to drop more frames. */\n> > + RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > + controller_.GetAlgorithm(\"agc\"));\n> > + if (agc)\n> > + dropFrame = std::max(dropFrame, agc->GetDropFrames());\n> > + LOG(IPARPI, Debug) << \"Drop \" << dropFrame << \" frames on startup\";\n> > +\n> \n> All looks good with this change, however, I have a possibly silly\n> question. In the previous code, our startup frames would account for\n> convergence in AGC, AWB, and ALS. Here we are explicitly accounting for\n> convergence only in AGC since helper_->HideFramesStartup() will return 0\n> by default. Does it matter? Should each derived CamHelper return a\n> non-zero number here?\n\nUnless I'm mistaken, HideFramesStartup() is meant to report how many\nincorrectly exposed (and \"gained\") frames are produced by the sensor at\nstartup, even if exposure time and gain are programmed before starting\nthe sensor. This will certainly impact AWB and ALS as they will have\ntrouble operating if the frame is greatly underexposed, but the sensor\nis otherwise not involved in AWB and ALS. I thus don't think CamHelper\nshould take AWB and ALS into account.\n\nWith this new split of responsibilities, with CamHelper reporting the\nnumber of frames that are bad (for different reasons, underexposed,\nincorrect metadata, ...) and the algorithms then deciding how long they\nneed before initially converging, Shouldn't agc->GetDropFrames() be\ngiven the HideFramesStartup() value as a parameter, and return a new\nnumber, instead of taking the maximum between the two ?\n\nI also wonder if we then need the hide/mistrust split anymore,\nespecially with the comment in CamHelperOv5647::MistrustFramesStartup()\nthat mentions underexposed frames. Is there still a difference between\nthe two concepts ?\n\nFinally, do we actually need to report a number of frames to drop at\nstartup to the pipeline handler, can't we rely on the algorithms status\nreported through AeLocked and AwbLocked ? Maybe we should report only\nthe number of frames that are definitely bad based on the CamHelper, and\nuse algorithm status to report initial convergence of the algorithms ?\n\n> > mistrustCount_ = helper_->MistrustFramesStartup();\n> > } else {\n> > dropFrame = helper_->HideFramesModeSwitch();","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 6045BBDB20\n\tfor ;\n\tSat, 5 Dec 2020 19:50:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D9FB8635F0;\n\tSat, 5 Dec 2020 20:50:50 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 41A8B60327\n\tfor ;\n\tSat, 5 Dec 2020 20:50:49 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AC565D2F;\n\tSat, 5 Dec 2020 20:50:48 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"f1tDdswj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1607197848;\n\tbh=48SGxw146AXFj6N9di3Jmk9pIGWoZrFtLvSgTS8dDNc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=f1tDdswja7kXl6ZfK81B78EKl1Ag7VtSmUcZ371rlsTlLB8bRAww1buRMVH4Ptjvj\n\tg+PesxZXoV3yGTME8jqcqcN25/snfdbtwGMb+zKeu9QvavBgLg6p8n7j5pyEpR6UY2\n\t7rRTlYL4s9BOXjXBRVCSb+ssHgDvBN+Ro8srjJ8o=","Date":"Sat, 5 Dec 2020 21:50:47 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20201202115253.14705-1-david.plowman@raspberrypi.com>\n\t<20201202115253.14705-5-david.plowman@raspberrypi.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 4/5] src: ipa: raspberrypi: Move\n\tinitial frame drop decision to AGC","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":14089,"web_url":"https://patchwork.libcamera.org/comment/14089/","msgid":"","date":"2020-12-05T23:28:49","subject":"Re: [libcamera-devel] [PATCH 4/5] src: ipa: raspberrypi: Move\n\tinitial frame drop decision to AGC","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi again Laurent\n\nThanks for the comments, yes, I agree there's a little more to think about.\n\nOn Sat, 5 Dec 2020 at 19:50, Laurent Pinchart\n wrote:\n>\n> Hi Naush and David,\n>\n> On Fri, Dec 04, 2020 at 04:01:42PM +0000, Naushir Patuck wrote:\n> > On Wed, 2 Dec 2020 at 11:53, David Plowman wrote:\n> >\n> > > Previously the CamHelper was returning the number of frames to drop\n> > > (on account of AGC converging). This wasn't really appropriate, it's\n> > > better for the AGC to do it, which now also knows when exposure and\n> > > gain have been explicitly set and therefore fewer (or no) frame drops\n> > > are necessary at all.\n> > >\n> > > The CamHelper::HideFramesStartup method should now just be returning\n> > > the number of frames to hide because they're bad/invalid in some way,\n> > > not worrying about the AGC. For many sensors, the correct value for\n> > > this is zero.\n> > >\n> > > Signed-off-by: David Plowman \n> > > ---\n> > > src/ipa/raspberrypi/cam_helper.cpp | 6 +++---\n> > > src/ipa/raspberrypi/raspberrypi.cpp | 8 ++++++++\n> > > 2 files changed, 11 insertions(+), 3 deletions(-)\n> > >\n> > > diff --git a/src/ipa/raspberrypi/cam_helper.cpp\n> > > b/src/ipa/raspberrypi/cam_helper.cpp\n> > > index c8ac3232..6efa0d7f 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper.cpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> > > @@ -82,10 +82,10 @@ bool CamHelper::SensorEmbeddedDataPresent() const\n> > > unsigned int CamHelper::HideFramesStartup() const\n> > > {\n> > > /*\n> > > - * By default, hide 6 frames completely at start-up while AGC etc. sort\n> > > - * themselves out (converge).\n> > > + * The number of frames when a camera first starts that shouldn't be\n> > > + * displayed as they are invalid in some way.\n> > > */\n> > > - return 6;\n> > > + return 0;\n> > > }\n> > >\n> > > unsigned int CamHelper::HideFramesModeSwitch() const\n> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > index 0300b8d9..ddabdb31 100644\n> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > @@ -192,6 +192,14 @@ int IPARPi::start(const IPAOperationData &ipaConfig,\n> > > IPAOperationData *result)\n> > > unsigned int dropFrame = 0;\n> > > if (firstStart_) {\n> > > dropFrame = helper_->HideFramesStartup();\n> > > +\n> > > + /* The AGC algorithm may want us to drop more frames. */\n> > > + RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > + controller_.GetAlgorithm(\"agc\"));\n> > > + if (agc)\n> > > + dropFrame = std::max(dropFrame, agc->GetDropFrames());\n> > > + LOG(IPARPI, Debug) << \"Drop \" << dropFrame << \" frames on startup\";\n> > > +\n> >\n> > All looks good with this change, however, I have a possibly silly\n> > question. In the previous code, our startup frames would account for\n> > convergence in AGC, AWB, and ALS. Here we are explicitly accounting for\n> > convergence only in AGC since helper_->HideFramesStartup() will return 0\n> > by default. Does it matter? Should each derived CamHelper return a\n> > non-zero number here?\n>\n> Unless I'm mistaken, HideFramesStartup() is meant to report how many\n> incorrectly exposed (and \"gained\") frames are produced by the sensor at\n> startup, even if exposure time and gain are programmed before starting\n> the sensor. This will certainly impact AWB and ALS as they will have\n> trouble operating if the frame is greatly underexposed, but the sensor\n> is otherwise not involved in AWB and ALS. I thus don't think CamHelper\n> should take AWB and ALS into account.\n>\n> With this new split of responsibilities, with CamHelper reporting the\n> number of frames that are bad (for different reasons, underexposed,\n> incorrect metadata, ...) and the algorithms then deciding how long they\n> need before initially converging, Shouldn't agc->GetDropFrames() be\n> given the HideFramesStartup() value as a parameter, and return a new\n> number, instead of taking the maximum between the two ?\n\nYes, I was already planning to change this as it's not quite right,\nbut I think I need to change it again. I think the correct behaviour\nis as follows\n\nFirstly, I'll look into giving AWB and ALSC the same\nGetConvergenceFrames method as the AGC.\n\nNext, I think the GetConvergenceFrames methods should be given the\nMistrustFramesStartup number. If they actually have no convergence to\ndo (e.g. exposure and gain fixed), they return 0. Otherwise they add\ntheir number to the MistrustFramesStartup number that they were given\nand return that.\n\nFinally, the dropFrames number is set to the maximum of\nHideFramesStartup and all the GetConvergenceFrames values that we\nobtained.\n\nThis gets the two important use cases right:\n\n1. When things aren't fixed, and we give the algorithms as long as\nthey say plus the MistrustFramesStartup number, and\n\n2. When everything is fixed, all the algorithms should tell us \"zero\",\nso that we only skip frames if HideFramesStartup tells us to.\n\n>\n> I also wonder if we then need the hide/mistrust split anymore,\n> especially with the comment in CamHelperOv5647::MistrustFramesStartup()\n> that mentions underexposed frames. Is there still a difference between\n> the two concepts ?\n\nSo I think there is, as discussed above. MistrustFramesModeSwitch,\nthough not used in my description above, still has a use too. If you\nwere to stop and restart the camera, so the algorithms don't need to\nconverge at all, you might still need to tell them to ignore a\nframe's-worth of statistics, if they're thought to be bad in some way\n(otherwise they risk making the algorithm go haywire when it tries to\nprocess them).\n\n>\n> Finally, do we actually need to report a number of frames to drop at\n> startup to the pipeline handler, can't we rely on the algorithms status\n> reported through AeLocked and AwbLocked ? Maybe we should report only\n> the number of frames that are definitely bad based on the CamHelper, and\n> use algorithm status to report initial convergence of the algorithms ?\n>\n> > > mistrustCount_ = helper_->MistrustFramesStartup();\n> > > } else {\n> > > dropFrame = helper_->HideFramesModeSwitch();\n\nIt's a good point. In practice we've found that users (OEMs in\nparticular) can be very sensitive about getting something on the\ndisplay asap, even if the AE/AWB isn't fully locked. On the other\nhand, it still needs to look \"visually locked\", avoiding any obviously\nbad frames, so we've always ended up in this sort of compromise\nposition where we actually display frames a little before they're\n\"technically\" locked.\n\nI'll make up another patch set on Monday with both your and Naush's\nfeedback, addressed, so thanks for all the suggestions.\n\nBest regards\nDavid\n\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 A034BBDB1F\n\tfor ;\n\tSat, 5 Dec 2020 23:29:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 160DC635F2;\n\tSun, 6 Dec 2020 00:29:05 +0100 (CET)","from mail-oi1-x242.google.com (mail-oi1-x242.google.com\n\t[IPv6:2607:f8b0:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 13933635D0\n\tfor ;\n\tSun, 6 Dec 2020 00:29:03 +0100 (CET)","by mail-oi1-x242.google.com with SMTP id x16so10943200oic.3\n\tfor ;\n\tSat, 05 Dec 2020 15:29:02 -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=\"NaOjmEkR\"; 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=mDq494i5VCRlhrI8DoTFbe+7aroVnwwAEwsI1AnrtWc=;\n\tb=NaOjmEkRM7YOF6zu9EIjyei+8vl1MgsAkxXl+lxFNYUmGLWQTRFlR7Q2MLWPaj/dVl\n\tpREqSowyS1QTea1pRttrdj0BVNpwYLcyUHNCNivPbefMChgcnOymsnqkGpkJNZolvMMM\n\tOKk7DxapIIMts/0oaF/xsIzKyCGKEIjX5Ib4ITFoHBAlzVe7xNBdDY2ogLMnIwe1Q/yu\n\ti2d+vui+Z+GAlYf4h7ZtnZAxf6rcEZ39Can0Uha12A4jA99oIcl0z2grO1DsO3VSuWK9\n\tVZd59frjkivOZnweijKydFMBrb2fXNPrBRpWFIRH3izKg4lVI9Ue0c6xeGrLIq0mouFU\n\tPRjA==","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=mDq494i5VCRlhrI8DoTFbe+7aroVnwwAEwsI1AnrtWc=;\n\tb=F24nmdVasB6ZWT27BgnxXnOrs9o99xRjpvDnjVw8GRO53E1EBMlkCtwVjY3odlGZwm\n\tqhMPJpz4Yp6IK3Dx98mU69dMqAhHiI0xlT+rKgIp8xg5wn7liQxEWlmmmKEEkdCS8XTh\n\tEoYKE0eSTrsOxT5H4ezjhVhxjfVlap4687aGZFihIJtXnUfQSwei4BJtW+X9RlFqzp41\n\tZ0J/TnonbQC6FlAluvFTtSQxkFVP6eWoefLMKAkMHE2mHLJXQClLsdbqwOSIlNKqrAnh\n\tQyO8rRz4SfTPHgZ2H/SulsRSW/s8qNfTi51Y+uC1c9ojzIrmIcQCqD0fRxKXEcbZNZ83\n\trq3A==","X-Gm-Message-State":"AOAM532hpfIZLK0Ex0jT3qmgTFLCW2RQ+qfC4BDpqxppSSyF/zMuQJFr\n\teb0/5ddrkGtTijgybUbH49FOOTN7k2OxWNqzB4kkRnaWlXm3ww==","X-Google-Smtp-Source":"ABdhPJyout0uDfTUVHc3gfBLFqtyxavvsDyqDs8zrV7yhgYGH0zx5JCliBTokE9WiOIGhVm7lcRIhHnllNWX7PQY2pI=","X-Received":"by 2002:aca:de89:: with SMTP id\n\tv131mr7858262oig.55.1607210941900; \n\tSat, 05 Dec 2020 15:29:01 -0800 (PST)","MIME-Version":"1.0","References":"<20201202115253.14705-1-david.plowman@raspberrypi.com>\n\t<20201202115253.14705-5-david.plowman@raspberrypi.com>\n\t\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Sat, 5 Dec 2020 23:28:49 +0000","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH 4/5] src: ipa: raspberrypi: Move\n\tinitial frame drop decision to AGC","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":14092,"web_url":"https://patchwork.libcamera.org/comment/14092/","msgid":"","date":"2020-12-06T10:44:44","subject":"Re: [libcamera-devel] [PATCH 4/5] src: ipa: raspberrypi: Move\n\tinitial frame drop decision to AGC","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nOn Sat, 5 Dec 2020 at 19:50, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush and David,\n>\n> On Fri, Dec 04, 2020 at 04:01:42PM +0000, Naushir Patuck wrote:\n> > On Wed, 2 Dec 2020 at 11:53, David Plowman wrote:\n> >\n> > > Previously the CamHelper was returning the number of frames to drop\n> > > (on account of AGC converging). This wasn't really appropriate, it's\n> > > better for the AGC to do it, which now also knows when exposure and\n> > > gain have been explicitly set and therefore fewer (or no) frame drops\n> > > are necessary at all.\n> > >\n> > > The CamHelper::HideFramesStartup method should now just be returning\n> > > the number of frames to hide because they're bad/invalid in some way,\n> > > not worrying about the AGC. For many sensors, the correct value for\n> > > this is zero.\n> > >\n> > > Signed-off-by: David Plowman \n> > > ---\n> > > src/ipa/raspberrypi/cam_helper.cpp | 6 +++---\n> > > src/ipa/raspberrypi/raspberrypi.cpp | 8 ++++++++\n> > > 2 files changed, 11 insertions(+), 3 deletions(-)\n> > >\n> > > diff --git a/src/ipa/raspberrypi/cam_helper.cpp\n> > > b/src/ipa/raspberrypi/cam_helper.cpp\n> > > index c8ac3232..6efa0d7f 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper.cpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> > > @@ -82,10 +82,10 @@ bool CamHelper::SensorEmbeddedDataPresent() const\n> > > unsigned int CamHelper::HideFramesStartup() const\n> > > {\n> > > /*\n> > > - * By default, hide 6 frames completely at start-up while AGC\n> etc. sort\n> > > - * themselves out (converge).\n> > > + * The number of frames when a camera first starts that\n> shouldn't be\n> > > + * displayed as they are invalid in some way.\n> > > */\n> > > - return 6;\n> > > + return 0;\n> > > }\n> > >\n> > > unsigned int CamHelper::HideFramesModeSwitch() const\n> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > index 0300b8d9..ddabdb31 100644\n> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > @@ -192,6 +192,14 @@ int IPARPi::start(const IPAOperationData\n> &ipaConfig,\n> > > IPAOperationData *result)\n> > > unsigned int dropFrame = 0;\n> > > if (firstStart_) {\n> > > dropFrame = helper_->HideFramesStartup();\n> > > +\n> > > + /* The AGC algorithm may want us to drop more frames.\n> */\n> > > + RPiController::AgcAlgorithm *agc =\n> dynamic_cast(\n> > > + controller_.GetAlgorithm(\"agc\"));\n> > > + if (agc)\n> > > + dropFrame = std::max(dropFrame,\n> agc->GetDropFrames());\n> > > + LOG(IPARPI, Debug) << \"Drop \" << dropFrame << \" frames\n> on startup\";\n> > > +\n> >\n> > All looks good with this change, however, I have a possibly silly\n> > question. In the previous code, our startup frames would account for\n> > convergence in AGC, AWB, and ALS. Here we are explicitly accounting for\n> > convergence only in AGC since helper_->HideFramesStartup() will return 0\n> > by default. Does it matter? Should each derived CamHelper return a\n> > non-zero number here?\n>\n> Unless I'm mistaken, HideFramesStartup() is meant to report how many\n> incorrectly exposed (and \"gained\") frames are produced by the sensor at\n> startup, even if exposure time and gain are programmed before starting\n> the sensor. This will certainly impact AWB and ALS as they will have\n> trouble operating if the frame is greatly underexposed, but the sensor\n> is otherwise not involved in AWB and ALS. I thus don't think CamHelper\n> should take AWB and ALS into account.\n>\n> With this new split of responsibilities, with CamHelper reporting the\n> number of frames that are bad (for different reasons, underexposed,\n> incorrect metadata, ...) and the algorithms then deciding how long they\n> need before initially converging, Shouldn't agc->GetDropFrames() be\n> given the HideFramesStartup() value as a parameter, and return a new\n> number, instead of taking the maximum between the two ?\n>\n> I also wonder if we then need the hide/mistrust split anymore,\n> especially with the comment in CamHelperOv5647::MistrustFramesStartup()\n> that mentions underexposed frames. Is there still a difference between\n> the two concepts ?\n>\n> Finally, do we actually need to report a number of frames to drop at\n> startup to the pipeline handler, can't we rely on the algorithms status\n> reported through AeLocked and AwbLocked ?\n\n\nIn my opinion, doing it this way (making the pipeline handler drop all\nframes where AE/AWB is not locked) would be very inefficient. Knowing how\nmany buffers are meant to be dropped on startup as we currently do allows\nthe pipeline handler to efficiently pipeline(!) the buffers queued to the\ndevice.\n\nAs an example, if the first Request after startup asks for the RAW frame to\nbe returned out to the application, we can pre-queue internally allocated\nbuffers to the count of the number of expected drop frames. We then queue\nup the Request provided RAW buffer to the device and have no additional\nlatency involved with handling dropped frames and returning the RAW\nbuffer. If we were to wait for AE/AWB locked status without knowing the\nnumber of dropped frames beforehand, we would have to continually queue the\nRequest provided RAW buffer to the device over and over again while we keep\ndropping them in the pipeline handler. Typically, that doubles your capture\nlatency as you cannot pipeline the buffers queued to the streaming device\nas is the current case. It's a similar story for queueing ISP output\nbuffers, but (at least for Raspberry Pi) given the mem-to-mem behavior of\nthe HW, you don't take as big a hit on latency. Hope that all makes sense!\n\nRegards,\nNaush\n\n\n\n> Maybe we should report only\n> the number of frames that are definitely bad based on the CamHelper, and\n> use algorithm status to report initial convergence of the algorithms ?\n>\n> > > mistrustCount_ = helper_->MistrustFramesStartup();\n> > > } else {\n> > > dropFrame = helper_->HideFramesModeSwitch();\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 6F9C8BDB20\n\tfor ;\n\tSun, 6 Dec 2020 10:45:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ECBFE635C4;\n\tSun, 6 Dec 2020 11:45:03 +0100 (CET)","from mail-lj1-x243.google.com (mail-lj1-x243.google.com\n\t[IPv6:2a00:1450:4864:20::243])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 21F3A635C4\n\tfor ;\n\tSun, 6 Dec 2020 11:45:02 +0100 (CET)","by mail-lj1-x243.google.com with SMTP id r18so11812561ljc.2\n\tfor ;\n\tSun, 06 Dec 2020 02:45:02 -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=\"PQu+HFY5\"; 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=GOY/4hL18CEEoN0AKeFezqx7SYPY8Zyd5AS0gJToJNI=;\n\tb=PQu+HFY5jmIp/L/AGi8qSvYdq/9bz4rmpBac3puLK9IWGTAUGX0TrrQgnkgZGRB79f\n\tDrlbdmDYVG1GtC3uSXhXBZTGLEuN8Nkqyicl5CuA86rhPqOup4oAyeFmDseEJ6l6ym1P\n\twsY685yZP+lsQGba0dTukX6ZxBzoLDSMiKmT3AKY1xSM/3y3a7haYq/DlA04ARdrokB9\n\tzPDKObRQchvcmpDy7qUSCN+OBxoW6x3F1xdkbjqHCyTxipBzHBK4zjN1Q7hY+rKpzTAv\n\tOmnqD8R7b4+SbN0NRYRbJ11XRolxs70N4LVK9mHIukwQ76gUdJo0OWeGLf6jbTOEt0fu\n\t8/HQ==","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=GOY/4hL18CEEoN0AKeFezqx7SYPY8Zyd5AS0gJToJNI=;\n\tb=RlpFVPpJnWvCLcQxhZ98iK4pAWgiBEHpuN8i+UelhesAOkI6SOErmkjg1oeSmez8eL\n\t08m/CJ7iptpbm0jpZC1ao/C16DpzuE7TvfSQ9u2BXi5jpmpZQ4GAHKAT5//frt4KtGPg\n\trSHj0d5/4dF6PFkqtJSV45Cku8zz82wQjubeVhlAH3bV4ZP12C8SSR937zJ0NguGvWGf\n\tlqd69kzpLeSgi1uPN6StNF8NoQcPiB7vQOUJZoLLpdkwsXEt27o/mz2qtfdXDhKIQGX0\n\t2LaAVbPHjUsHaY+daKpBkhtxui+2QQ1Ew4RDfVJm6UGP2F3HiqFYOACNOGfpS2XznlvR\n\tP2VQ==","X-Gm-Message-State":"AOAM531QrXr901WHEA3Q3r75fh5T1O/MwqiWEwZwgX2iQsewR/g4iVxn\n\tzAZ6dm8nlhpe7RiIEUNMfPhDHS+a6EbtH3rHhF0JwQ==","X-Google-Smtp-Source":"ABdhPJxzEdytSNuI3eGwfAEfvSWqG3fiePTUEg4zsOUcY+6zLILOaYdTggBSfo4ISsWBZeLzIAsGfgrZU8hgPXUsWlc=","X-Received":"by 2002:a2e:9ad2:: with SMTP id\n\tp18mr6877062ljj.415.1607251501373; \n\tSun, 06 Dec 2020 02:45:01 -0800 (PST)","MIME-Version":"1.0","References":"<20201202115253.14705-1-david.plowman@raspberrypi.com>\n\t<20201202115253.14705-5-david.plowman@raspberrypi.com>\n\t\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Sun, 6 Dec 2020 10:44:44 +0000","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH 4/5] src: ipa: raspberrypi: Move\n\tinitial frame drop decision to AGC","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=\"===============4135092073923624627==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14149,"web_url":"https://patchwork.libcamera.org/comment/14149/","msgid":"","date":"2020-12-08T17:35:41","subject":"Re: [libcamera-devel] [PATCH 4/5] src: ipa: raspberrypi: Move\n\tinitial frame drop decision to AGC","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nOn Sat, Dec 05, 2020 at 11:28:49PM +0000, David Plowman wrote:\n> On Sat, 5 Dec 2020 at 19:50, Laurent Pinchart wrote:\n> > On Fri, Dec 04, 2020 at 04:01:42PM +0000, Naushir Patuck wrote:\n> > > On Wed, 2 Dec 2020 at 11:53, David Plowman wrote:\n> > >\n> > > > Previously the CamHelper was returning the number of frames to drop\n> > > > (on account of AGC converging). This wasn't really appropriate, it's\n> > > > better for the AGC to do it, which now also knows when exposure and\n> > > > gain have been explicitly set and therefore fewer (or no) frame drops\n> > > > are necessary at all.\n> > > >\n> > > > The CamHelper::HideFramesStartup method should now just be returning\n> > > > the number of frames to hide because they're bad/invalid in some way,\n> > > > not worrying about the AGC. For many sensors, the correct value for\n> > > > this is zero.\n> > > >\n> > > > Signed-off-by: David Plowman \n> > > > ---\n> > > > src/ipa/raspberrypi/cam_helper.cpp | 6 +++---\n> > > > src/ipa/raspberrypi/raspberrypi.cpp | 8 ++++++++\n> > > > 2 files changed, 11 insertions(+), 3 deletions(-)\n> > > >\n> > > > diff --git a/src/ipa/raspberrypi/cam_helper.cpp\n> > > > b/src/ipa/raspberrypi/cam_helper.cpp\n> > > > index c8ac3232..6efa0d7f 100644\n> > > > --- a/src/ipa/raspberrypi/cam_helper.cpp\n> > > > +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> > > > @@ -82,10 +82,10 @@ bool CamHelper::SensorEmbeddedDataPresent() const\n> > > > unsigned int CamHelper::HideFramesStartup() const\n> > > > {\n> > > > /*\n> > > > - * By default, hide 6 frames completely at start-up while AGC etc. sort\n> > > > - * themselves out (converge).\n> > > > + * The number of frames when a camera first starts that shouldn't be\n> > > > + * displayed as they are invalid in some way.\n> > > > */\n> > > > - return 6;\n> > > > + return 0;\n> > > > }\n> > > >\n> > > > unsigned int CamHelper::HideFramesModeSwitch() const\n> > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > index 0300b8d9..ddabdb31 100644\n> > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > @@ -192,6 +192,14 @@ int IPARPi::start(const IPAOperationData &ipaConfig,\n> > > > IPAOperationData *result)\n> > > > unsigned int dropFrame = 0;\n> > > > if (firstStart_) {\n> > > > dropFrame = helper_->HideFramesStartup();\n> > > > +\n> > > > + /* The AGC algorithm may want us to drop more frames. */\n> > > > + RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > > + controller_.GetAlgorithm(\"agc\"));\n> > > > + if (agc)\n> > > > + dropFrame = std::max(dropFrame, agc->GetDropFrames());\n> > > > + LOG(IPARPI, Debug) << \"Drop \" << dropFrame << \" frames on startup\";\n> > > > +\n> > >\n> > > All looks good with this change, however, I have a possibly silly\n> > > question. In the previous code, our startup frames would account for\n> > > convergence in AGC, AWB, and ALS. Here we are explicitly accounting for\n> > > convergence only in AGC since helper_->HideFramesStartup() will return 0\n> > > by default. Does it matter? Should each derived CamHelper return a\n> > > non-zero number here?\n> >\n> > Unless I'm mistaken, HideFramesStartup() is meant to report how many\n> > incorrectly exposed (and \"gained\") frames are produced by the sensor at\n> > startup, even if exposure time and gain are programmed before starting\n> > the sensor. This will certainly impact AWB and ALS as they will have\n> > trouble operating if the frame is greatly underexposed, but the sensor\n> > is otherwise not involved in AWB and ALS. I thus don't think CamHelper\n> > should take AWB and ALS into account.\n> >\n> > With this new split of responsibilities, with CamHelper reporting the\n> > number of frames that are bad (for different reasons, underexposed,\n> > incorrect metadata, ...) and the algorithms then deciding how long they\n> > need before initially converging, Shouldn't agc->GetDropFrames() be\n> > given the HideFramesStartup() value as a parameter, and return a new\n> > number, instead of taking the maximum between the two ?\n> \n> Yes, I was already planning to change this as it's not quite right,\n> but I think I need to change it again. I think the correct behaviour\n> is as follows\n> \n> Firstly, I'll look into giving AWB and ALSC the same\n> GetConvergenceFrames method as the AGC.\n> \n> Next, I think the GetConvergenceFrames methods should be given the\n> MistrustFramesStartup number. If they actually have no convergence to\n> do (e.g. exposure and gain fixed), they return 0. Otherwise they add\n> their number to the MistrustFramesStartup number that they were given\n> and return that.\n> \n> Finally, the dropFrames number is set to the maximum of\n> HideFramesStartup and all the GetConvergenceFrames values that we\n> obtained.\n> \n> This gets the two important use cases right:\n> \n> 1. When things aren't fixed, and we give the algorithms as long as\n> they say plus the MistrustFramesStartup number, and\n> \n> 2. When everything is fixed, all the algorithms should tell us \"zero\",\n> so that we only skip frames if HideFramesStartup tells us to.\n> \n> > I also wonder if we then need the hide/mistrust split anymore,\n> > especially with the comment in CamHelperOv5647::MistrustFramesStartup()\n> > that mentions underexposed frames. Is there still a difference between\n> > the two concepts ?\n> \n> So I think there is, as discussed above. MistrustFramesModeSwitch,\n> though not used in my description above, still has a use too. If you\n> were to stop and restart the camera, so the algorithms don't need to\n> converge at all, you might still need to tell them to ignore a\n> frame's-worth of statistics, if they're thought to be bad in some way\n> (otherwise they risk making the algorithm go haywire when it tries to\n> process them).\n\nAlgorithms consume both statistics and metadata, do I assume correctly\nthat they would need to ignore both incorrect statistics (caused by\nincorrect frames being produced by the sensor) and incorrect metadata\n(caused by incorrect metadata produced by the sensor, even if the frame\nitself is correct) ? If so, should mistrust frames be the highest of the\nnumber of incorrect frames and incorrect metadata counts, or should it\nmodel the latter only, with the IPA taking the maximum of the hide count\nand the mistruct count to decide how many frames to ignore in the\nbeginning ? I think I have a preference for the latter, as CamHelper\nshould ideally report raw information about the sensor, without taking\ninto account how they will be used (same rationale as the one that made\nthis patch series exist in the first place, moving the convergence time\nof the algorithms from CamHelper to the algorithms themselves). What do\nyou think ?\n\n> > Finally, do we actually need to report a number of frames to drop at\n> > startup to the pipeline handler, can't we rely on the algorithms status\n> > reported through AeLocked and AwbLocked ? Maybe we should report only\n> > the number of frames that are definitely bad based on the CamHelper, and\n> > use algorithm status to report initial convergence of the algorithms ?\n> >\n> > > > mistrustCount_ = helper_->MistrustFramesStartup();\n> > > > } else {\n> > > > dropFrame = helper_->HideFramesModeSwitch();\n> \n> It's a good point. In practice we've found that users (OEMs in\n> particular) can be very sensitive about getting something on the\n> display asap, even if the AE/AWB isn't fully locked. On the other\n> hand, it still needs to look \"visually locked\", avoiding any obviously\n> bad frames, so we've always ended up in this sort of compromise\n> position where we actually display frames a little before they're\n> \"technically\" locked.\n\nThat's a very good point, it makes sense.\n\n> I'll make up another patch set on Monday with both your and Naush's\n> feedback, addressed, so thanks for all the suggestions.","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 CFE25BDB20\n\tfor ;\n\tTue, 8 Dec 2020 17:35:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4E87767F0A;\n\tTue, 8 Dec 2020 18:35:46 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 09C4F67F07\n\tfor ;\n\tTue, 8 Dec 2020 18:35:45 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7DE3A543;\n\tTue, 8 Dec 2020 18:35:44 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"D/2HN8Og\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1607448944;\n\tbh=6vwj5jDvxUWUah3E856OzknL71ruE9qxWOr5YZ8nHZ0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=D/2HN8OgMb2IZakXXUiQBtNMR7916hxD+QGVTq6IwwNnE+IzBvS9ZeBqq32pRvCbR\n\thbK40ueCWcvheCMYVNC0Bl4UCXKJ9M+/glo3uTJSO78IGKRFn8TZh3tfyD77ky/Hvs\n\tVeXzQ+pIizuhZGYokXtXNd7eaQWGdfXyqtnjgrLU=","Date":"Tue, 8 Dec 2020 19:35:41 +0200","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"","References":"<20201202115253.14705-1-david.plowman@raspberrypi.com>\n\t<20201202115253.14705-5-david.plowman@raspberrypi.com>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 4/5] src: ipa: raspberrypi: Move\n\tinitial frame drop decision to AGC","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":14150,"web_url":"https://patchwork.libcamera.org/comment/14150/","msgid":"","date":"2020-12-08T17:40:36","subject":"Re: [libcamera-devel] [PATCH 4/5] src: ipa: raspberrypi: Move\n\tinitial frame drop decision to AGC","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Sun, Dec 06, 2020 at 10:44:44AM +0000, Naushir Patuck wrote:\n> On Sat, 5 Dec 2020 at 19:50, Laurent Pinchart wrote:\n> > On Fri, Dec 04, 2020 at 04:01:42PM +0000, Naushir Patuck wrote:\n> > > On Wed, 2 Dec 2020 at 11:53, David Plowman wrote:\n> > >\n> > > > Previously the CamHelper was returning the number of frames to drop\n> > > > (on account of AGC converging). This wasn't really appropriate, it's\n> > > > better for the AGC to do it, which now also knows when exposure and\n> > > > gain have been explicitly set and therefore fewer (or no) frame drops\n> > > > are necessary at all.\n> > > >\n> > > > The CamHelper::HideFramesStartup method should now just be returning\n> > > > the number of frames to hide because they're bad/invalid in some way,\n> > > > not worrying about the AGC. For many sensors, the correct value for\n> > > > this is zero.\n> > > >\n> > > > Signed-off-by: David Plowman \n> > > > ---\n> > > > src/ipa/raspberrypi/cam_helper.cpp | 6 +++---\n> > > > src/ipa/raspberrypi/raspberrypi.cpp | 8 ++++++++\n> > > > 2 files changed, 11 insertions(+), 3 deletions(-)\n> > > >\n> > > > diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp\n> > > > index c8ac3232..6efa0d7f 100644\n> > > > --- a/src/ipa/raspberrypi/cam_helper.cpp\n> > > > +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> > > > @@ -82,10 +82,10 @@ bool CamHelper::SensorEmbeddedDataPresent() const\n> > > > unsigned int CamHelper::HideFramesStartup() const\n> > > > {\n> > > > /*\n> > > > - * By default, hide 6 frames completely at start-up while AGC etc. sort\n> > > > - * themselves out (converge).\n> > > > + * The number of frames when a camera first starts that shouldn't be\n> > > > + * displayed as they are invalid in some way.\n> > > > */\n> > > > - return 6;\n> > > > + return 0;\n> > > > }\n> > > >\n> > > > unsigned int CamHelper::HideFramesModeSwitch() const\n> > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > index 0300b8d9..ddabdb31 100644\n> > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > @@ -192,6 +192,14 @@ int IPARPi::start(const IPAOperationData &ipaConfig, IPAOperationData *result)\n> > > > unsigned int dropFrame = 0;\n> > > > if (firstStart_) {\n> > > > dropFrame = helper_->HideFramesStartup();\n> > > > +\n> > > > + /* The AGC algorithm may want us to drop more frames. */\n> > > > + RPiController::AgcAlgorithm *agc = dynamic_cast(\n> > > > + controller_.GetAlgorithm(\"agc\"));\n> > > > + if (agc)\n> > > > + dropFrame = std::max(dropFrame, agc->GetDropFrames());\n> > > > + LOG(IPARPI, Debug) << \"Drop \" << dropFrame << \" frames on startup\";\n> > > > +\n> > >\n> > > All looks good with this change, however, I have a possibly silly\n> > > question. In the previous code, our startup frames would account for\n> > > convergence in AGC, AWB, and ALS. Here we are explicitly accounting for\n> > > convergence only in AGC since helper_->HideFramesStartup() will return 0\n> > > by default. Does it matter? Should each derived CamHelper return a\n> > > non-zero number here?\n> >\n> > Unless I'm mistaken, HideFramesStartup() is meant to report how many\n> > incorrectly exposed (and \"gained\") frames are produced by the sensor at\n> > startup, even if exposure time and gain are programmed before starting\n> > the sensor. This will certainly impact AWB and ALS as they will have\n> > trouble operating if the frame is greatly underexposed, but the sensor\n> > is otherwise not involved in AWB and ALS. I thus don't think CamHelper\n> > should take AWB and ALS into account.\n> >\n> > With this new split of responsibilities, with CamHelper reporting the\n> > number of frames that are bad (for different reasons, underexposed,\n> > incorrect metadata, ...) and the algorithms then deciding how long they\n> > need before initially converging, Shouldn't agc->GetDropFrames() be\n> > given the HideFramesStartup() value as a parameter, and return a new\n> > number, instead of taking the maximum between the two ?\n> >\n> > I also wonder if we then need the hide/mistrust split anymore,\n> > especially with the comment in CamHelperOv5647::MistrustFramesStartup()\n> > that mentions underexposed frames. Is there still a difference between\n> > the two concepts ?\n> >\n> > Finally, do we actually need to report a number of frames to drop at\n> > startup to the pipeline handler, can't we rely on the algorithms status\n> > reported through AeLocked and AwbLocked ?\n> \n> In my opinion, doing it this way (making the pipeline handler drop all\n> frames where AE/AWB is not locked) would be very inefficient. Knowing how\n> many buffers are meant to be dropped on startup as we currently do allows\n> the pipeline handler to efficiently pipeline(!) the buffers queued to the\n> device.\n> \n> As an example, if the first Request after startup asks for the RAW frame to\n> be returned out to the application, we can pre-queue internally allocated\n> buffers to the count of the number of expected drop frames. We then queue\n> up the Request provided RAW buffer to the device and have no additional\n> latency involved with handling dropped frames and returning the RAW\n> buffer. If we were to wait for AE/AWB locked status without knowing the\n> number of dropped frames beforehand, we would have to continually queue the\n> Request provided RAW buffer to the device over and over again while we keep\n> dropping them in the pipeline handler. Typically, that doubles your capture\n> latency as you cannot pipeline the buffers queued to the streaming device\n> as is the current case. It's a similar story for queueing ISP output\n> buffers, but (at least for Raspberry Pi) given the mem-to-mem behavior of\n> the HW, you don't take as big a hit on latency. Hope that all makes sense!\n\nThat's a very good point too, I hadn't considered that. Thanks.\n\n> > Maybe we should report only\n> > the number of frames that are definitely bad based on the CamHelper, and\n> > use algorithm status to report initial convergence of the algorithms ?\n> >\n> > > > mistrustCount_ = helper_->MistrustFramesStartup();\n> > > > } else {\n> > > > dropFrame = helper_->HideFramesModeSwitch();","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 ED61CBDB1F\n\tfor ;\n\tTue, 8 Dec 2020 17:40:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 602EA67F0F;\n\tTue, 8 Dec 2020 18:40:41 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CAFFA67F08\n\tfor ;\n\tTue, 8 Dec 2020 18:40:39 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 40CD1543;\n\tTue, 8 Dec 2020 18:40:39 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"c2Z4olxn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1607449239;\n\tbh=wXS8PAQYURQf7rtsBMpByvxTGGYiBtwgPWy6ZlOPP/8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=c2Z4olxn3y63eRlMh+hITEeGN3k0bbODhn3M7pS6hpsQF/Smpz07SyYREYDq71gmt\n\tq+jz8Idd89nPqKntq/6AVj/Rix89BkEc+uI6aiRv9xG+wceLRNURS2+6PaYmzuen7O\n\tVYT9S0e95BbIHmPpppTjA2UYdsgpKKgvdsbygP2k=","Date":"Tue, 8 Dec 2020 19:40:36 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20201202115253.14705-1-david.plowman@raspberrypi.com>\n\t<20201202115253.14705-5-david.plowman@raspberrypi.com>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 4/5] src: ipa: raspberrypi: Move\n\tinitial frame drop decision to AGC","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\" "}}]