[{"id":14135,"web_url":"https://patchwork.libcamera.org/comment/14135/","msgid":"","date":"2020-12-08T11:31:46","subject":"Re: [libcamera-devel] [PATCH v2 1/6] src: raspberrypi: Pass the\n\tdrop frame count in start, not configure","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:16PM +0000, David Plowman wrote:\n> The number of frames to drop (not display) is passed back now from the\n> start method, not configure. This means applications have a chance to\n> set fixed exposure/gain before starting the camera and this can affect\n> the frame drop count that is returned.\n> \n> Note how we need to be able to tell the very first time we start the\n> camera from subsequent restarts, hence addition of the \"firstStart_\"\n> flag.\n\nfirstStart_ will stay set for the lifetime of the Camera object, as the\nIPA interface lifetime matches it. That's probably too long, we should\nat least reset if it the application closes the camera, but that's not\ncommunicated to the IPA at the moment. Maybe an alternate mechanism,\nsuch as a timeout, would be better. There's no need to address it now,\nbut I'm fairly sure the question will come back on the table at some\npoint :-)\n\n> Both the IPA implementation file and the pipeline handler need\n> matching modifications.\n> \n> Signed-off-by: David Plowman \n> Reviewed-by: Naushir Patuck \n> ---\n> src/ipa/raspberrypi/raspberrypi.cpp | 45 ++++++++++---------\n> .../pipeline/raspberrypi/raspberrypi.cpp | 25 ++++++-----\n> 2 files changed, 38 insertions(+), 32 deletions(-)\n> \n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 2027f1c0..0e89af00 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -67,7 +67,7 @@ public:\n> \tIPARPi()\n> \t\t: lastMode_({}), controller_(), controllerInit_(false),\n> \t\t frameCount_(0), checkCount_(0), mistrustCount_(0),\n> -\t\t lsTable_(nullptr)\n> +\t\t lsTable_(nullptr), firstStart_(true)\n> \t{\n> \t}\n> \n> @@ -145,6 +145,9 @@ private:\n> \t/* LS table allocation passed in from the pipeline handler. */\n> \tFileDescriptor lsTableHandle_;\n> \tvoid *lsTable_;\n> +\n> +\t/* Distinguish the first camera start from others. */\n> +\tbool firstStart_;\n> };\n> \n> int IPARPi::init(const IPASettings &settings)\n> @@ -179,6 +182,27 @@ int IPARPi::start(const IPAOperationData &ipaConfig, IPAOperationData *result)\n> \t\tresult->operation |= RPi::IPA_CONFIG_SENSOR;\n> \t}\n> \n> +\t/*\n> +\t * Initialise frame counts, and decide how many frames must be hidden or\n> +\t *\"mistrusted\", which depends on whether this is a startup from cold,\n\nMissing space before \"mistrusted\".\n\nAlso not something to be addressed now: we'll likely move the\nhide/mistrust count to a common helper in the future, to be shared by\nIPA modules. As part of that effort, I think we'll try to formally\ndefine what \"hide\" and \"mistrust\" means, and possibly rename those\nterms. Do I understand correctly that \"hide\" refers to the number of\nframes that are generated with incorrect parameters (mostly exposure\ntime I suppose), while \"mistrust\" is the number of frames generated with\nincorrect metadata but without the frame itself being incorrect ?\n\nI understand that the number of frames to hide is different as startup\nand mode switch, as the IPA starts without context in the first case.\nI'm however a bit puzzled as to why the mistrust count is different\nbetween startup and mode switch. It seems to be\n\n- 1 for startup and 0 for mode switch by default\n- 2 in both cases for the OV5647\n- 1 in both cases for the IMX219\n\nThe latter has a comment that the sensor sometimes returns incorrect\nmetadata on mode switch but not on startup, but the implementation for\nIMX219 doesn't override MistrustFramesStartup(), and this uses a value\nof 1. I wonder if this means the possible incorrect metadata at startup\nhas then just never been noticed, and if MistrustFramesStartup() and\nMistrustFramesModeSwitch() should be merged into a single function.\n\nThese are not issues we need to address as part of this series, so\n\nReviewed-by: Laurent Pinchart \n\n> +\t * or merely a mode switch in a running system.\n> +\t */\n> +\tframeCount_ = 0;\n> +\tcheckCount_ = 0;\n> +\tunsigned int dropFrame = 0;\n> +\tif (firstStart_) {\n> +\t\tdropFrame = helper_->HideFramesStartup();\n> +\t\tmistrustCount_ = helper_->MistrustFramesStartup();\n> +\t} else {\n> +\t\tdropFrame = helper_->HideFramesModeSwitch();\n> +\t\tmistrustCount_ = helper_->MistrustFramesModeSwitch();\n> +\t}\n> +\n> +\tresult->data.push_back(dropFrame);\n> +\tresult->operation |= RPi::IPA_CONFIG_DROP_FRAMES;\n> +\n> +\tfirstStart_ = false;\n> +\n> \treturn 0;\n> }\n> \n> @@ -293,25 +317,6 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,\n> \t/* Pass the camera mode to the CamHelper to setup algorithms. */\n> \thelper_->SetCameraMode(mode_);\n> \n> -\t/*\n> -\t * Initialise frame counts, and decide how many frames must be hidden or\n> -\t *\"mistrusted\", which depends on whether this is a startup from cold,\n> -\t * or merely a mode switch in a running system.\n> -\t */\n> -\tframeCount_ = 0;\n> -\tcheckCount_ = 0;\n> -\tunsigned int dropFrame = 0;\n> -\tif (controllerInit_) {\n> -\t\tdropFrame = helper_->HideFramesModeSwitch();\n> -\t\tmistrustCount_ = helper_->MistrustFramesModeSwitch();\n> -\t} else {\n> -\t\tdropFrame = helper_->HideFramesStartup();\n> -\t\tmistrustCount_ = helper_->MistrustFramesStartup();\n> -\t}\n> -\n> -\tresult->data.push_back(dropFrame);\n> -\tresult->operation |= RPi::IPA_CONFIG_DROP_FRAMES;\n> -\n> \tif (!controllerInit_) {\n> \t\t/* Load the tuning file for this sensor. */\n> \t\tcontroller_.Read(tuningFile_.c_str());\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 89a44763..5ae56628 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -745,13 +745,6 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont\n> \t\treturn ret;\n> \t}\n> \n> -\tret = queueAllBuffers(camera);\n> -\tif (ret) {\n> -\t\tLOG(RPI, Error) << \"Failed to queue buffers\";\n> -\t\tstop(camera);\n> -\t\treturn ret;\n> -\t}\n> -\n> \t/* Check if a ScalerCrop control was specified. */\n> \tif (controls)\n> \t\tdata->applyScalerCrop(*controls);\n> @@ -778,6 +771,19 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont\n> \t\t\tLOG(RPI, Error) << \"V4L2 staggered set failed\";\n> \t}\n> \n> +\tif (result.operation & RPi::IPA_CONFIG_DROP_FRAMES) {\n> +\t\t/* Configure the number of dropped frames required on startup. */\n> +\t\tdata->dropFrameCount_ = result.data[0];\n> +\t}\n> +\n> +\t/* We need to set the dropFrameCount_ before queueing buffers. */\n> +\tret = queueAllBuffers(camera);\n> +\tif (ret) {\n> +\t\tLOG(RPI, Error) << \"Failed to queue buffers\";\n> +\t\tstop(camera);\n> +\t\treturn ret;\n> +\t}\n> +\n> \t/*\n> \t * IPA configure may have changed the sensor flips - hence the bayer\n> \t * order. Get the sensor format and set the ISP input now.\n> @@ -1231,11 +1237,6 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> \t\t\tLOG(RPI, Error) << \"V4L2 staggered set failed\";\n> \t}\n> \n> -\tif (result.operation & RPi::IPA_CONFIG_DROP_FRAMES) {\n> -\t\t/* Configure the number of dropped frames required on startup. */\n> -\t\tdropFrameCount_ = result.data[resultIdx++];\n> -\t}\n> -\n> \t/*\n> \t * Configure the H/V flip controls based on the combination of\n> \t * the sensor and user transform.","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 01A75BDB20\n\tfor ;\n\tTue, 8 Dec 2020 11:31:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7A66767E6D;\n\tTue, 8 Dec 2020 12:31:51 +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 DA86B600FF\n\tfor ;\n\tTue, 8 Dec 2020 12:31: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 59DD6DD;\n\tTue, 8 Dec 2020 12:31:49 +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=\"mRMjPZ05\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1607427109;\n\tbh=jRZXUee0bYSjyygBYUYYOfm93og4TsFrtoJGRqsT/YE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=mRMjPZ05IMoSG3MXHrDSC7kxafATPQ7TXrY+doVpwhsYlAGScjZkjGC4qJXtkG3XZ\n\t09akNCKu0Uqjk4NV9x6aZIh9hHFk766umsDNnXfllPHP9XIcpIuury+ok5CjqWU0gf\n\tkUZLxrQAmUyX0X39V2bLmQ7BrlyFAu3+s3RM2Q48=","Date":"Tue, 8 Dec 2020 13:31:46 +0200","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"","References":"<20201207180121.6374-1-david.plowman@raspberrypi.com>\n\t<20201207180121.6374-2-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201207180121.6374-2-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v2 1/6] src: raspberrypi: Pass the\n\tdrop frame count in start, not configure","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\" "}}]