[{"id":14229,"web_url":"https://patchwork.libcamera.org/comment/14229/","msgid":"","date":"2020-12-11T21:43:50","subject":"Re: [libcamera-devel] [PATCH v3 0/6] Raspberry Pi AGC: initial\n\tframe drop count","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nOn Tue, Dec 08, 2020 at 08:44:35PM +0000, David Plowman wrote:\n> Hi everyone\n> \n> Thanks for all the reviews, comments and discussion! I've lost track a\n> little bit of which comments were in which thread, so perhaps I can\n> address a few of the remaining issues here.\n\nThanks for the summary :-)\n\n> 1. Hide/Mistrust frames at Startup/ModeSwitch\n> \n> Yes, I see that some of these are the same now. I'm slightly inclined\n> to keep all 4 variants for the moment as it seems to me they might be\n> different, however, if it transpires that new cameras that come along\n> still don't need them maybe we do some simplification then?\n\nSounds good to me. We plan to move this information to the libcamera\ncore, so at that point we'll propose a rename, if you don't beat us to\nit.\n\n> 2. Should GetConvergenceFrames be in the Algorithm class?\n> \n> Again, I'm slightly inclined to avoid it just because there are only 2\n> implementations of this, yet there are actually over a dozen\n> \"algorithms\". But like all things, we keep an eye on it!\n\nSounds good to me.\n\n> 3. \"Hiding\" vs. \"Mistrusting\"\n> \n> Just to clarify the distinction here, \"hiding\" means \"do not show this\n> to the user because it will look unexpected in some\n> way\". \"Mistrusting\" means \"do not allow algorithms to process this\n> frame because it will cause them to wobble or do something bad\".\n\nI'd like to move away from \"hiding\". The CamHelper class should report\ninformation about the camera sensor, not contain logic that dictates how\nframes should be handled by the IPA and pipeline handler. It may very\nwell be a semantic difference only, and it doesn't have to be renamed\nnow, we can address that when moving this information to the libcamera\ncore.\n\n> \"Mistrusting\" includes both statistics and metadata from the device. I\n\nAs statistics are computed from the frame by the ISP, incorrect\nstatistics imply an incorrect frame, right ? In the event that a sensor\nwould produce a first frame containing both bogus data and bogus\nmetadata, and a second frame containing bogus metadata only, do I\nunderstand correctly that an algorithm that consumes statistics only\ncould run on the second frame already, while an algorithm that consumes\nmetadata would need to skip the first two frames ?\n\n> suppose if the ISP statistics are bad then the frame may be bad too,\n> so it would feature in the \"hide\" number already. It is possible that\n> you could get frames that look OK but where the device is lying about\n> its metadata, so there is a distinction between the two.\n> \n> Yet perhaps this distinction never really happens in practice - so\n> this should go on this list of things we keep an eye on with a view to\n> simplification.\n> \n> As regards the patches themselves, they are structured exactly as\n> before. The main differences are:\n> \n> 1. No change.\n> \n> 2. and 3. GetConvergenceFrames has dropped the parameter (that's\n> handled back in the IPA now, patch 6).\n> \n> 4. Pwl::Inverse is a bit more careful about adding to its existing end\n> points as it goes.\n> \n> 5. No more std::move.\n> \n> 6. The adding of the \"mistrusted\" frames is handled here now.\n> \n> And I think that's it, please remind me if anything has slipped\n> through the cracks!\n>\n> David Plowman (6):\n> src: raspberrypi: Pass the drop frame count in start, not configure\n> src: ipa: raspberrypi: agc: Add GetConvergenceFrames method to AGC\n> base class\n> src: ipa: raspberrypi: awb: Add GetConvergenceFrames method to AWB\n> base class\n> src: ipa: raspberrypi: Compute inverse of piecewise linear function\n> src: ipa: raspberrypi: Estimate the colour temerature if starting with\n> fixed colour gains\n> src: ipa: raspberrypi: Move initial frame drop decision to AGC/AWB\n\nMinor style issue, we don't normally include the \"src: \" prefix.\n\nIt all looks good to me, so I've pushed the patches with the commit\nsubjects updated.\n\n> src/ipa/raspberrypi/cam_helper.cpp | 6 +-\n> src/ipa/raspberrypi/cam_helper_ov5647.cpp | 10 +++\n> .../raspberrypi/controller/agc_algorithm.hpp | 1 +\n> .../raspberrypi/controller/awb_algorithm.hpp | 1 +\n> src/ipa/raspberrypi/controller/pwl.cpp | 30 ++++++++\n> src/ipa/raspberrypi/controller/pwl.hpp | 3 +\n> src/ipa/raspberrypi/controller/rpi/agc.cpp | 11 +++\n> src/ipa/raspberrypi/controller/rpi/agc.hpp | 2 +\n> src/ipa/raspberrypi/controller/rpi/alsc.cpp | 6 +-\n> src/ipa/raspberrypi/controller/rpi/awb.cpp | 25 +++++++\n> src/ipa/raspberrypi/controller/rpi/awb.hpp | 3 +\n> src/ipa/raspberrypi/raspberrypi.cpp | 73 ++++++++++++++-----\n> .../pipeline/raspberrypi/raspberrypi.cpp | 25 ++++---\n> 13 files changed, 160 insertions(+), 36 deletions(-)","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 CD3AFBD808\n\tfor ;\n\tFri, 11 Dec 2020 21:43:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 62CB868054;\n\tFri, 11 Dec 2020 22:43:56 +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 B076C67F69\n\tfor ;\n\tFri, 11 Dec 2020 22:43:55 +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 335BC99;\n\tFri, 11 Dec 2020 22:43:55 +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=\"SNaGcD7r\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1607723035;\n\tbh=xmPU0KOvR3rsxJ0jC6+nA3MssRV6OVhik61bOHWumdU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SNaGcD7rfjHSrY+EU6c60YO7WyXdo6RhV9lT3hTOTFN6YlXcnfSUqmZc7+/dBaD0w\n\thx+Sshd7tMINWb3MVGYvx0qmo6G6s2WQjbhbFsC7Oyy4EjxTMKuXFExzT9lfWyjcyY\n\tkU72Cy72R/Hb82xahAKD4F7PzEolCFZnO1fATCTY=","Date":"Fri, 11 Dec 2020 23:43:50 +0200","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"","References":"<20201208204441.9356-1-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201208204441.9356-1-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v3 0/6] Raspberry Pi AGC: initial\n\tframe drop count","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\" "}}]