[{"id":14130,"web_url":"https://patchwork.libcamera.org/comment/14130/","msgid":"","date":"2020-12-08T10:16:35","subject":"Re: [libcamera-devel] [PATCH v2 6/6] src: ipa: raspberrypi: Move\n\tinitial frame drop decision to AGC/AWB","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 Mon, 7 Dec 2020 at 18:02, David Plowman \nwrote:\n\n> Previously the CamHelper was returning the number of frames to drop\n> (on account of AGC/AWB converging). This wasn't really appropriate,\n> it's better for the algorithms to do it as they know how many frames\n> they might need.\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/AWB. For many sensors, the correct value\n> for this is zero. But the ov5647 needs updating as it must return 2.\n>\n> Signed-off-by: David Plowman \n> ---\n> src/ipa/raspberrypi/cam_helper.cpp | 6 +++---\n> src/ipa/raspberrypi/cam_helper_ov5647.cpp | 10 ++++++++++\n> src/ipa/raspberrypi/raspberrypi.cpp | 16 ++++++++++++++++\n> 3 files changed, 29 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/cam_helper_ov5647.cpp\n> b/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> index dc5d8275..0b841cd1 100644\n> --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> @@ -19,6 +19,7 @@ public:\n> uint32_t GainCode(double gain) const override;\n> double Gain(uint32_t gain_code) const override;\n> void GetDelays(int &exposure_delay, int &gain_delay) const\n> override;\n> + unsigned int HideFramesStartup() const override;\n> unsigned int HideFramesModeSwitch() const override;\n> unsigned int MistrustFramesStartup() const override;\n> unsigned int MistrustFramesModeSwitch() const override;\n> @@ -54,6 +55,15 @@ void CamHelperOv5647::GetDelays(int &exposure_delay,\n> int &gain_delay) const\n> gain_delay = 2;\n> }\n>\n> +unsigned int CamHelperOv5647::HideFramesStartup() const\n> +{\n> + /*\n> + * On startup, we get a couple of under-exposed frames which\n> + * we don't want shown.\n> + */\n> + return 2;\n> +}\n> +\n> unsigned int CamHelperOv5647::HideFramesModeSwitch() const\n> {\n> /*\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 0e89af00..da92e492 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -193,6 +193,22 @@ int IPARPi::start(const IPAOperationData &ipaConfig,\n> IPAOperationData *result)\n> if (firstStart_) {\n> dropFrame = helper_->HideFramesStartup();\n> mistrustCount_ = helper_->MistrustFramesStartup();\n> +\n> + /* The AGC and/or AWB algorithms may want us to drop more\n> frames. */\n> + unsigned int convergence_frames = 0;\n>\n\nMinor nit, but could you use camel case for convergence_frames to match the\nrest of the variables?\n\n\n> + RPiController::AgcAlgorithm *agc =\n> dynamic_cast(\n> + controller_.GetAlgorithm(\"agc\"));\n> + if (agc)\n> + convergence_frames =\n> agc->GetConvergenceFrames(mistrustCount_);\n> +\n> + RPiController::AwbAlgorithm *awb =\n> dynamic_cast(\n> + controller_.GetAlgorithm(\"awb\"));\n> + if (awb)\n> + convergence_frames = std::max(convergence_frames,\n> +\n> awb->GetConvergenceFrames(mistrustCount_));\n> +\n> + dropFrame = std::max(dropFrame, convergence_frames);\n> + LOG(IPARPI, Debug) << \"Drop \" << dropFrame << \" frames on\n> startup\";\n>\n\nApart from the above nit, and the earlier question of passing\nconvergence_frames into GetConvergenceFrames(),\n\nReviewed-by: Naushir Patuck \n\n\n> } else {\n> dropFrame = helper_->HideFramesModeSwitch();\n> mistrustCount_ = helper_->MistrustFramesModeSwitch();\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 C2FDFBDB1F\n\tfor ;\n\tTue, 8 Dec 2020 10:16:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4E10C635F2;\n\tTue, 8 Dec 2020 11:16:56 +0100 (CET)","from mail-lj1-x229.google.com (mail-lj1-x229.google.com\n\t[IPv6:2a00:1450:4864:20::229])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7D87B600FF\n\tfor ;\n\tTue, 8 Dec 2020 11:16:54 +0100 (CET)","by mail-lj1-x229.google.com with SMTP id m13so2333155ljo.11\n\tfor ;\n\tTue, 08 Dec 2020 02:16:54 -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=\"r1Fkvgi3\"; 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=TQtmGEyr7VFAmj0MqrMxz99KwKHccRwPMEqwSE59rxI=;\n\tb=r1Fkvgi3+NHmcOOYaFbRIBlvUb8gyyqogYosFpqpJiKNL0BPrlePygNgG4Xe4VuRah\n\tI0Kfg2z06Nn3EHr5k8hDq0zQkUYvmnCxh7FBw6boIceUrnMk8oTTv4r2NyscNKVHReE6\n\tezxzuHFKlCdxpVKRVbWPzZvX8SxjagPLyFiS9ajd9hi4SQk9AbotCO5+tqcEN528uAJX\n\tLP1a5X1iJAi/sIeaFexCdtiUelktd/Skk86fxWdVe6Ai9/9T2/VN1MUfdVhOjI7i9w3x\n\te2M+4LQG18tzNjqVcg0/1KIR5UeR4ZyNTfLVAOqsHs8MCNxcX2bLdivBPCJfAJEt2fBq\n\twMWw==","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=TQtmGEyr7VFAmj0MqrMxz99KwKHccRwPMEqwSE59rxI=;\n\tb=IlDCATn8Lea5PiRw9LRsX5vOniAak/dQdu/2jVg47TtxxIXCLOpqmvo+5soxQ3Vcgg\n\tnVUxFgNNJ+S7sCyFV3DlolcEsgajIaI2aGaE+vPRXDp5MfS7mT909EuW0ByYD4xKs3Q8\n\tsN3ntBk2PEczvOwTsFkoLjnVWgPMHVVZ9f/6FyHP/bPV36y1kCxh/5MtFZPF/NVqis1Y\n\tHLH35ANjzakN8cfvah+HW7c0s3x+N21OjiwHJXMGHHz0Xb4s7rahHMt7yM2O6NLYZI8L\n\tjsJentb2q6Ns1AslAyoWaeQlS2ZFLVXHci3WoxQHTDg2chvYvrfQVlmfDJgDidGt+ebF\n\t7VLQ==","X-Gm-Message-State":"AOAM532us6ID8z3xWMFXY8xoF8eifnaTmvypHYhMGMUZFnGm2nUQy2cm\n\tyTy2w0qXCqHxnqcbUQ2SBmzMTVw4dvx0BvhPvjZz1w==","X-Google-Smtp-Source":"ABdhPJy3LFPDzduNB54/0LVL8fovqxlBP+957VQJ8sCeVsLTZFtDmOIKIr7s7189jwVhH/s038/nLBBS14pF+ePxxpM=","X-Received":"by 2002:a2e:93d6:: with SMTP id p22mr570501ljh.169.1607422613935;\n\tTue, 08 Dec 2020 02:16:53 -0800 (PST)","MIME-Version":"1.0","References":"<20201207180121.6374-1-david.plowman@raspberrypi.com>\n\t<20201207180121.6374-7-david.plowman@raspberrypi.com>","In-Reply-To":"<20201207180121.6374-7-david.plowman@raspberrypi.com>","From":"Naushir Patuck ","Date":"Tue, 8 Dec 2020 10:16:35 +0000","Message-ID":"","To":"David Plowman ","Subject":"Re: [libcamera-devel] [PATCH v2 6/6] src: ipa: raspberrypi: Move\n\tinitial frame drop decision to AGC/AWB","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=\"===============0591755822620129934==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14145,"web_url":"https://patchwork.libcamera.org/comment/14145/","msgid":"","date":"2020-12-08T12:15:28","subject":"Re: [libcamera-devel] [PATCH v2 6/6] src: ipa: raspberrypi: Move\n\tinitial frame drop decision to AGC/AWB","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nThank you for the patch.\n\nOn Mon, Dec 07, 2020 at 06:01:21PM +0000, David Plowman wrote:\n> Previously the CamHelper was returning the number of frames to drop\n> (on account of AGC/AWB converging). This wasn't really appropriate,\n> it's better for the algorithms to do it as they know how many frames\n> they might need.\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/AWB. For many sensors, the correct value\n> for this is zero. But the ov5647 needs updating as it must return 2.\n> \n> Signed-off-by: David Plowman \n> ---\n> src/ipa/raspberrypi/cam_helper.cpp | 6 +++---\n> src/ipa/raspberrypi/cam_helper_ov5647.cpp | 10 ++++++++++\n> src/ipa/raspberrypi/raspberrypi.cpp | 16 ++++++++++++++++\n> 3 files changed, 29 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> \t/*\n> -\t * By default, hide 6 frames completely at start-up while AGC etc. sort\n> -\t * themselves out (converge).\n> +\t * The number of frames when a camera first starts that shouldn't be\n> +\t * displayed as they are invalid in some way.\n> \t */\n> -\treturn 6;\n> +\treturn 0;\n> }\n> \n> unsigned int CamHelper::HideFramesModeSwitch() const\n> diff --git a/src/ipa/raspberrypi/cam_helper_ov5647.cpp b/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> index dc5d8275..0b841cd1 100644\n> --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp\n> @@ -19,6 +19,7 @@ public:\n> \tuint32_t GainCode(double gain) const override;\n> \tdouble Gain(uint32_t gain_code) const override;\n> \tvoid GetDelays(int &exposure_delay, int &gain_delay) const override;\n> +\tunsigned int HideFramesStartup() const override;\n> \tunsigned int HideFramesModeSwitch() const override;\n> \tunsigned int MistrustFramesStartup() const override;\n> \tunsigned int MistrustFramesModeSwitch() const override;\n> @@ -54,6 +55,15 @@ void CamHelperOv5647::GetDelays(int &exposure_delay, int &gain_delay) const\n> \tgain_delay = 2;\n> }\n> \n> +unsigned int CamHelperOv5647::HideFramesStartup() const\n> +{\n> +\t/*\n> +\t * On startup, we get a couple of under-exposed frames which\n> +\t * we don't want shown.\n> +\t */\n> +\treturn 2;\n> +}\n> +\n\nNow both HideFramesStartup() and HideFramesModeSwitch() always return\nthe same value. Should they be merged into a single function ? This can\nbe done in a separate patch.\n\nReviewed-by: Laurent Pinchart \n\n> unsigned int CamHelperOv5647::HideFramesModeSwitch() const\n> {\n> \t/*\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 0e89af00..da92e492 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -193,6 +193,22 @@ int IPARPi::start(const IPAOperationData &ipaConfig, IPAOperationData *result)\n> \tif (firstStart_) {\n> \t\tdropFrame = helper_->HideFramesStartup();\n> \t\tmistrustCount_ = helper_->MistrustFramesStartup();\n> +\n> +\t\t/* The AGC and/or AWB algorithms may want us to drop more frames. */\n> +\t\tunsigned int convergence_frames = 0;\n> +\t\tRPiController::AgcAlgorithm *agc = dynamic_cast(\n> +\t\t\tcontroller_.GetAlgorithm(\"agc\"));\n> +\t\tif (agc)\n> +\t\t\tconvergence_frames = agc->GetConvergenceFrames(mistrustCount_);\n> +\n> +\t\tRPiController::AwbAlgorithm *awb = dynamic_cast(\n> +\t\t\tcontroller_.GetAlgorithm(\"awb\"));\n> +\t\tif (awb)\n> +\t\t\tconvergence_frames = std::max(convergence_frames,\n> +\t\t\t\t\t\t awb->GetConvergenceFrames(mistrustCount_));\n> +\n> +\t\tdropFrame = std::max(dropFrame, convergence_frames);\n> +\t\tLOG(IPARPI, Debug) << \"Drop \" << dropFrame << \" frames on startup\";\n> \t} else {\n> \t\tdropFrame = helper_->HideFramesModeSwitch();\n> \t\tmistrustCount_ = helper_->MistrustFramesModeSwitch();","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 7AA27BDB1F\n\tfor ;\n\tTue, 8 Dec 2020 12:15:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1016F67E6D;\n\tTue, 8 Dec 2020 13:15:34 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4F57967E15\n\tfor ;\n\tTue, 8 Dec 2020 13:15:32 +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 CC0AEDD;\n\tTue, 8 Dec 2020 13:15:31 +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=\"mnV7olFz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1607429732;\n\tbh=GeJkcLeTA9ZF8ou7w4yvOU0oVUQVLemgYQl9VlXMl9Q=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=mnV7olFzwBkJzZ7Tz9VDkMk3mTPUGm7QG5nFq8uYIsAUlmGrUTnr/yofDZmwqErxJ\n\t2qVtWWSNn4bzrT4T/d4R85/ZAh6mZAedn23rz7H51hSQd7Kq4+WepLB5zNMLGCRpI3\n\t95qSbtgCsg6GdLksfO2V5KF28vFnMVeVjrS9dFYI=","Date":"Tue, 8 Dec 2020 14:15:28 +0200","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"","References":"<20201207180121.6374-1-david.plowman@raspberrypi.com>\n\t<20201207180121.6374-7-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201207180121.6374-7-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v2 6/6] src: ipa: raspberrypi: Move\n\tinitial frame drop decision to AGC/AWB","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\" "}}]