[{"id":15681,"web_url":"https://patchwork.libcamera.org/comment/15681/","msgid":"","date":"2021-03-14T22:29:28","subject":"Re: [libcamera-devel] [RFC PATCH 0/1] Raspberry Pi generalised\n\tembedded data parsing","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nOn Wed, Mar 10, 2021 at 05:23:47PM +0000, David Plowman wrote:\n> Hi\n> \n> I'm just submitting this patch for comments in the first instance\n> (mostly from Naush, I guess, but everyone is welcome!). It's part of\n> our plan for more flexible handling of metadata from the sensor.\n> \n> (The background is that we have some interesting sensors coming up\n> that give us other forms of embedded data, not just register\n> dumps, and we need to be able to deal with those!)\n> \n> The plan is to give our CamHelpers a Prepare() and a Process() method,\n> just like all our algorithms. As usual, Prepare() runs just before the\n> ISP starts, Process() just after. A version of Prepare() is provided\n> that has basically just sucked that little bit of\n> register-dump-parsing functionality out of the IPA file\n> (raspberrypi.cpp). Process() does nothing by default.\n> \n> There aren't actually many changes, but some observations on what I've\n> done:\n> \n> * I've not updated various CamHelper comments yet, that can wait!\n> \n> * I've made the Prepare() method responsible for reading the delayed\n> control values if we can't use the metadata to get the\n> exposure/gain. I wonder if perhaps that is better left in the\n> IPA. Prepare() might indicate via a return value whether it found\n> them in the embedded data or not.\n> \n> * The parser object is completely hidden behind the helper now, so the\n> distinction between them is rather blurring. Maybe they could be\n> combined, but that can happen in a later patch.\n\nI'm trying to figure out how this will play along with the refactoring\nof camera sensor helpers. There are a few points to consider:\n\n- The helpers are not specific to the Raspberry Pi IPA, and should thus\n be moved to a common location. This is more or less a mechanical\n change and shouldn't cause any issue by itself. The code could be\n moved to libipa for instance.\n\n- We have sensor-specific data on the pipeline handler side, provided by\n the CameraSensor class and passed to the IPA through CameraSensorInfo.\n All the information is currently retrieved from the kernel driver.\n Work is planned to create a sensor database that will allow hardcoding\n sensor-specific information in libcamera.\n\n I'd like to see if we could centralize all sensor-specific information\n on the pipeline handler side, or least for static data. This would\n cover information related to delayed controls, which is currently\n provided by the IPA but not used by it, but also information about\n mistrusted frames, that would then be passed to the IPA by the\n pipeline handler.\n\n- There's a need for sensor-specific code (as opposed to data),\n currently used on the IPA side. There's room for refactoring this\n though, including replacing the virtual Gain and GainCode functions\n with parametric formulas. For instance, the CCS specification computes\n the sensor gain as either\n\n - gain = (m0 * x + c0) / (m1 * x + c1), with x being the register\n value, m0, m1, c0 and c1 being static parameters, and either m0 or\n m1 being equal to 0 ; or\n\n - gain = a * 2^x, with a and x being register values.\n\n It seems this wouldn't match the IMX290, but we could add additional\n formulas.\n\n- For embedded data parsing, I wonder if it would be best handled in the\n IPA or in the pipeline handler. The latter would have the advantage of\n gathering more sensor-specific code on the pipeline handler side, as\n well as not having to involve the IPA in the parsing for other types\n of ancillary data produced by the sensors.\n\n> David Plowman (1):\n> ipa: raspberrypi: Use CamHelpers to generalise embedded data parsing\n> \n> src/ipa/raspberrypi/cam_helper.cpp | 49 ++++++++++++++++\n> src/ipa/raspberrypi/cam_helper.hpp | 14 ++++-\n> src/ipa/raspberrypi/raspberrypi.cpp | 88 ++++++++---------------------\n> 3 files changed, 84 insertions(+), 67 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 6CECCBD80C\n\tfor ;\n\tSun, 14 Mar 2021 22:30:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DBCCD602E4;\n\tSun, 14 Mar 2021 23:30:05 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 740B5602E3\n\tfor ;\n\tSun, 14 Mar 2021 23:30:04 +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 E42283E6;\n\tSun, 14 Mar 2021 23:30:03 +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=\"cbxQ0eBx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1615761004;\n\tbh=wHi2eIeaKaAsUklqbIaOUciJoTv1WzZQk3UiGY7iNNk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cbxQ0eBxlGdkCk+x/jc0gWjrmfKnD4pdI5eEQ38XgmwXxUJ9bSSGQbGyjTfYsPjTL\n\tYLgn1kXq8CETMNK7B5a4RrbJFRhQ0ANo7I7ZMWt2CfCA5Q2e1n2IK4i+g1wI4RCKMf\n\tWilGld4hJaE69RMPffPUears7Bpe9hRtkFO6+W08=","Date":"Mon, 15 Mar 2021 00:29:28 +0200","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"","References":"<20210310172348.4312-1-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210310172348.4312-1-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [RFC PATCH 0/1] Raspberry Pi generalised\n\tembedded data parsing","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":15718,"web_url":"https://patchwork.libcamera.org/comment/15718/","msgid":"","date":"2021-03-16T10:33:40","subject":"Re: [libcamera-devel] [RFC PATCH 0/1] Raspberry Pi generalised\n\tembedded data parsing","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi David and Laurent,\n\nOn Sun, 14 Mar 2021 at 22:30, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi David,\n>\n> On Wed, Mar 10, 2021 at 05:23:47PM +0000, David Plowman wrote:\n> > Hi\n> >\n> > I'm just submitting this patch for comments in the first instance\n> > (mostly from Naush, I guess, but everyone is welcome!). It's part of\n> > our plan for more flexible handling of metadata from the sensor.\n> >\n> > (The background is that we have some interesting sensors coming up\n> > that give us other forms of embedded data, not just register\n> > dumps, and we need to be able to deal with those!)\n> >\n> > The plan is to give our CamHelpers a Prepare() and a Process() method,\n> > just like all our algorithms. As usual, Prepare() runs just before the\n> > ISP starts, Process() just after. A version of Prepare() is provided\n> > that has basically just sucked that little bit of\n> > register-dump-parsing functionality out of the IPA file\n> > (raspberrypi.cpp). Process() does nothing by default.\n> >\n> > There aren't actually many changes, but some observations on what I've\n> > done:\n> >\n> > * I've not updated various CamHelper comments yet, that can wait!\n> >\n> > * I've made the Prepare() method responsible for reading the delayed\n> > control values if we can't use the metadata to get the\n> > exposure/gain. I wonder if perhaps that is better left in the\n> > IPA. Prepare() might indicate via a return value whether it found\n> > them in the embedded data or not.\n> >\n> > * The parser object is completely hidden behind the helper now, so the\n> > distinction between them is rather blurring. Maybe they could be\n> > combined, but that can happen in a later patch.\n>\n> I'm trying to figure out how this will play along with the refactoring\n> of camera sensor helpers. There are a few points to consider:\n>\n> - The helpers are not specific to the Raspberry Pi IPA, and should thus\n> be moved to a common location. This is more or less a mechanical\n> change and shouldn't cause any issue by itself. The code could be\n> moved to libipa for instance.\n>\n> - We have sensor-specific data on the pipeline handler side, provided by\n> the CameraSensor class and passed to the IPA through CameraSensorInfo.\n> All the information is currently retrieved from the kernel driver.\n> Work is planned to create a sensor database that will allow hardcoding\n> sensor-specific information in libcamera.\n>\n\nWould the sensor database be a drop in replacement for the CamHelper,\nor do you think we will still have a CamHelper doing certain operations and\nthe sensor database is for static sensor properties?\n\n\n>\n> I'd like to see if we could centralize all sensor-specific information\n> on the pipeline handler side, or least for static data. This would\n> cover information related to delayed controls, which is currently\n> provided by the IPA but not used by it, but also information about\n> mistrusted frames, that would then be passed to the IPA by the\n> pipeline handler.\n>\n\nThis does seem like the right approach to me.\n\n\n>\n> - There's a need for sensor-specific code (as opposed to data),\n> currently used on the IPA side. There's room for refactoring this\n> though, including replacing the virtual Gain and GainCode functions\n> with parametric formulas. For instance, the CCS specification computes\n> the sensor gain as either\n>\n> - gain = (m0 * x + c0) / (m1 * x + c1), with x being the register\n> value, m0, m1, c0 and c1 being static parameters, and either m0 or\n> m1 being equal to 0 ; or\n>\n> - gain = a * 2^x, with a and x being register values.\n>\n> It seems this wouldn't match the IMX290, but we could add additional\n> formulas.\n>\n\nI think allowing this to remain virtual might still be needed.\n\nSome sensors have entirely tables based gain -> code translations, so\nwe should not restrict to only parametric formulas. Similar comment for\nexposure lines -> time calculations. Some sensors I've encountered\n(global shutter and ultra long exposure modes) do not follow the typical\nline length * frame_length approach.\n\n\n>\n> - For embedded data parsing, I wonder if it would be best handled in the\n> IPA or in the pipeline handler. The latter would have the advantage of\n> gathering more sensor-specific code on the pipeline handler side, as\n> well as not having to involve the IPA in the parsing for other types\n> of ancillary data produced by the sensors.\n>\n\nHaving the embedded data parsing in the pipeline handler does seem\nlike a reasonable thing to do. However, we must keep in mind that\nthere may be vendor specific data in there (e.g. focus pixel stats) that\nwill need to be passed into the IPA. This may happen through named\ncontrols, or some other opaque way.\n\nRegards,\nNaush\n\n\n>\n> > David Plowman (1):\n> > ipa: raspberrypi: Use CamHelpers to generalise embedded data parsing\n> >\n> > src/ipa/raspberrypi/cam_helper.cpp | 49 ++++++++++++++++\n> > src/ipa/raspberrypi/cam_helper.hpp | 14 ++++-\n> > src/ipa/raspberrypi/raspberrypi.cpp | 88 ++++++++---------------------\n> > 3 files changed, 84 insertions(+), 67 deletions(-)\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 14894C32E1\n\tfor ;\n\tTue, 16 Mar 2021 10:33:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5E0F168D47;\n\tTue, 16 Mar 2021 11:33:58 +0100 (CET)","from mail-lf1-x12c.google.com (mail-lf1-x12c.google.com\n\t[IPv6:2a00:1450:4864:20::12c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 00B02602E8\n\tfor ;\n\tTue, 16 Mar 2021 11:33:56 +0100 (CET)","by mail-lf1-x12c.google.com with SMTP id d3so61541138lfg.10\n\tfor ;\n\tTue, 16 Mar 2021 03:33:56 -0700 (PDT)"],"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=\"kIxYGC9L\"; 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=u0MKCsMWXzXW1bpe1Dyv2csUYk9Dkl+zZgYN1u65Rsg=;\n\tb=kIxYGC9LsZ79mML4IQUymIfWMhPd9n32oYMIXpjYKCuxz6YwRwRuYR8fNYtThOfsml\n\tzljqfN9Aa0Zk3RHeAiawUcZPa8lAVxBNPCZ/AXjWLy/TaLPVh9ekBYmoi2z9QVp40XI6\n\toZVSfXu3X92Td8I4O8v4+l3w7Db6a2IdWERoDF++W40uxig9yHDk7KKTpQncLc2J29K3\n\t95pgP4Q05sIc5swMWYlk0tM86kvYPWnaBaeQ8lRsdVCWvA05uoeie7b56VgSOo5qexyG\n\thgonD75LHdLSF4RJHz2BVD6vW0hyx6yT47vm2XaXkx4mwK+jgSKmh+TZhpO7DHtucER5\n\tjXsw==","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=u0MKCsMWXzXW1bpe1Dyv2csUYk9Dkl+zZgYN1u65Rsg=;\n\tb=svcw0w1weTeCeWbI+d1jyyEC/rYpjBoI7Oi8dYcxq9ra/lTRrcGfGsRcofOcsTOawD\n\tjeLoRUHMuEnu4iCAef0GcQESkfsqp17qzWI2UK98Y4PyrIVizg+uSfBTuOVBkZLG2TLo\n\tssNyngBRMQYZ6a44RcOK9aodeWWh0fF++l1QwmLTyAL9dzmD4lcXJT4Zuq0WB/HdjZDx\n\tdeo1uhvXcAdv1UW/dVLFGl2jqPaXGuE+oq9I0V7TXVugCDSZlFrTY4Im4Dz6Iexsj+nt\n\tO1pjRSQvx8WHIvArmZovilBL+CSaPO++2e7J+hWSe9EdDGUsu09o1pNZmrvWyChW6RAF\n\tcBbw==","X-Gm-Message-State":"AOAM5328MJsLGPggH9Pf9p435xrmjOJ9YPPiq3uEeZ2zFQcMsfBSeKrF\n\tXGcFrMUYmxpvJQzWy2Johc8OtZ3MDDc/Ewx8c5WrVA==","X-Google-Smtp-Source":"ABdhPJyqEU7YJLBCcFbzDxqAc2hSh4d+C+NaADeRb3PnQKifvDdGV3byqLZf8VWxfZBK03N4foJ5q9JzB5pBTGrkCik=","X-Received":"by 2002:a05:6512:210b:: with SMTP id\n\tq11mr10778734lfr.133.1615890836418; \n\tTue, 16 Mar 2021 03:33:56 -0700 (PDT)","MIME-Version":"1.0","References":"<20210310172348.4312-1-david.plowman@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Tue, 16 Mar 2021 10:33:40 +0000","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [RFC PATCH 0/1] Raspberry Pi generalised\n\tembedded data parsing","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=\"===============9153582858144803814==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15726,"web_url":"https://patchwork.libcamera.org/comment/15726/","msgid":"","date":"2021-03-16T16:40:41","subject":"Re: [libcamera-devel] [RFC PATCH 0/1] Raspberry Pi generalised\n\tembedded data parsing","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Naush, Laurent\n\nThanks for the replies on this subject. In fact I'm still fiddling around,\nand discovering some new problems, so there's certainly still some way to\ngo!\n\nOne new thing I'm wrestling with at the moment is a sensor where the\n\"usual\" relationship between binned and full res modes, in respect of noise\nlevels, doesn't hold. I suppose this is all information you could encode in\na database, though I wonder whether it might become quite burdensome.\n\nAs things stand currently, I'm thinking about making\nCamHelper::SetCameraMode virtual, so that we can amend the mode for certain\nsensors. The CameraMode that we pass to our SwitchMode methods would have\nto come from the helper.\n\nOn some of the other questions:\n\nOn Tue, 16 Mar 2021 at 10:33, Naushir Patuck wrote:\n\n> Hi David and Laurent,\n>\n> On Sun, 14 Mar 2021 at 22:30, Laurent Pinchart <\n> laurent.pinchart@ideasonboard.com> wrote:\n>\n>> Hi David,\n>>\n>> On Wed, Mar 10, 2021 at 05:23:47PM +0000, David Plowman wrote:\n>> > Hi\n>> >\n>> > I'm just submitting this patch for comments in the first instance\n>> > (mostly from Naush, I guess, but everyone is welcome!). It's part of\n>> > our plan for more flexible handling of metadata from the sensor.\n>> >\n>> > (The background is that we have some interesting sensors coming up\n>> > that give us other forms of embedded data, not just register\n>> > dumps, and we need to be able to deal with those!)\n>> >\n>> > The plan is to give our CamHelpers a Prepare() and a Process() method,\n>> > just like all our algorithms. As usual, Prepare() runs just before the\n>> > ISP starts, Process() just after. A version of Prepare() is provided\n>> > that has basically just sucked that little bit of\n>> > register-dump-parsing functionality out of the IPA file\n>> > (raspberrypi.cpp). Process() does nothing by default.\n>> >\n>> > There aren't actually many changes, but some observations on what I've\n>> > done:\n>> >\n>> > * I've not updated various CamHelper comments yet, that can wait!\n>> >\n>> > * I've made the Prepare() method responsible for reading the delayed\n>> > control values if we can't use the metadata to get the\n>> > exposure/gain. I wonder if perhaps that is better left in the\n>> > IPA. Prepare() might indicate via a return value whether it found\n>> > them in the embedded data or not.\n>>\n>\nI don't like what I did here. I'd prefer to move the code that fetches\nvalues from the controls out of the helper into the IPA, that is,\nsrc/ipa/raspberrypi/raspberrypi.cpp.\n\n\n> >\n>> > * The parser object is completely hidden behind the helper now, so the\n>> > distinction between them is rather blurring. Maybe they could be\n>> > combined, but that can happen in a later patch.\n>\n>\n>> I'm trying to figure out how this will play along with the refactoring\n>> of camera sensor helpers. There are a few points to consider:\n>>\n>> - The helpers are not specific to the Raspberry Pi IPA, and should thus\n>> be moved to a common location. This is more or less a mechanical\n>> change and shouldn't cause any issue by itself. The code could be\n>> moved to libipa for instance.\n>>\n>> - We have sensor-specific data on the pipeline handler side, provided by\n>> the CameraSensor class and passed to the IPA through CameraSensorInfo.\n>> All the information is currently retrieved from the kernel driver.\n>> Work is planned to create a sensor database that will allow hardcoding\n>> sensor-specific information in libcamera.\n>>\n>\n> Would the sensor database be a drop in replacement for the CamHelper,\n> or do you think we will still have a CamHelper doing certain operations and\n> the sensor database is for static sensor properties?\n>\n\nI suspect a database is not enough. More below...\n\n\n>\n>\n>>\n>> I'd like to see if we could centralize all sensor-specific information\n>> on the pipeline handler side, or least for static data. This would\n>> cover information related to delayed controls, which is currently\n>> provided by the IPA but not used by it, but also information about\n>> mistrusted frames, that would then be passed to the IPA by the\n>> pipeline handler.\n>>\n>\n> This does seem like the right approach to me.\n>\n>\n>>\n>> - There's a need for sensor-specific code (as opposed to data),\n>> currently used on the IPA side. There's room for refactoring this\n>> though, including replacing the virtual Gain and GainCode functions\n>> with parametric formulas. For instance, the CCS specification computes\n>> the sensor gain as either\n>>\n>> - gain = (m0 * x + c0) / (m1 * x + c1), with x being the register\n>> value, m0, m1, c0 and c1 being static parameters, and either m0 or\n>> m1 being equal to 0 ; or\n>>\n>> - gain = a * 2^x, with a and x being register values.\n>>\n>> It seems this wouldn't match the IMX290, but we could add additional\n>> formulas.\n>>\n>\n> I think allowing this to remain virtual might still be needed.\n>\n> Some sensors have entirely tables based gain -> code translations, so\n> we should not restrict to only parametric formulas. Similar comment for\n> exposure lines -> time calculations. Some sensors I've encountered\n> (global shutter and ultra long exposure modes) do not follow the typical\n> line length * frame_length approach.\n>\n\nAgree. I have a sensor where some modes can't have a gain less than (in\nthis case) 1.12x. I'm \"hiding\" that in the CamHelper methods. Also the\nbinned mode produces 2x the signal level of the full res mode. This too I'm\n\"hiding\" in the CamHelper methods (though it doesn't feel like the best\nsolution.) There seem to be quite a few \"awkward\" devices out there!\n\n\n>\n>\n>>\n>> - For embedded data parsing, I wonder if it would be best handled in the\n>> IPA or in the pipeline handler. The latter would have the advantage of\n>> gathering more sensor-specific code on the pipeline handler side, as\n>> well as not having to involve the IPA in the parsing for other types\n>> of ancillary data produced by the sensors.\n>>\n>\n> Having the embedded data parsing in the pipeline handler does seem\n> like a reasonable thing to do. However, we must keep in mind that\n> there may be vendor specific data in there (e.g. focus pixel stats) that\n> will need to be passed into the IPA. This may happen through named\n> controls, or some other opaque way.\n>\n\nI'd be OK to see embedded data parsing move out of the helper, though I\nthink that being able to cope with other types of metadata is a powerful\nfeature. It might be focus pixels, or other kinds of image analysis. In\ngeneral I would expect the CamHelper::Prepare() method to put this\ninformation into the Raspberry Pi metadata object. From there, an algorithm\nwould be loaded by being listed in the json file, which would retrieve the\nitem from the Raspberry Pi metadata and handle it.\n\nI'm thinking I should perhaps submit another version that encapsulates my\nlatest thoughts on what I think I need - then we can consider what might\nbenefit from being done differently.\n\nThanks!\n\nDavid\n\n\n>\n> Regards,\n> Naush\n>\n>\n>>\n>> > David Plowman (1):\n>> > ipa: raspberrypi: Use CamHelpers to generalise embedded data parsing\n>> >\n>> > src/ipa/raspberrypi/cam_helper.cpp | 49 ++++++++++++++++\n>> > src/ipa/raspberrypi/cam_helper.hpp | 14 ++++-\n>> > src/ipa/raspberrypi/raspberrypi.cpp | 88 ++++++++---------------------\n>> > 3 files changed, 84 insertions(+), 67 deletions(-)\n>>\n>> --\n>> Regards,\n>>\n>> Laurent Pinchart\n>> _______________________________________________\n>> libcamera-devel mailing list\n>> libcamera-devel@lists.libcamera.org\n>> https://lists.libcamera.org/listinfo/libcamera-devel\n>\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 D66FABD80C\n\tfor ;\n\tTue, 16 Mar 2021 16:40:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 43C0268D4D;\n\tTue, 16 Mar 2021 17:40:58 +0100 (CET)","from mail-ot1-x32a.google.com (mail-ot1-x32a.google.com\n\t[IPv6:2607:f8b0:4864:20::32a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4B14A68D48\n\tfor ;\n\tTue, 16 Mar 2021 17:40:57 +0100 (CET)","by mail-ot1-x32a.google.com with SMTP id\n\ty19-20020a0568301d93b02901b9f88a238eso7182840oti.11\n\tfor ;\n\tTue, 16 Mar 2021 09:40:57 -0700 (PDT)"],"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=\"niXvPlfY\"; 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=LDoyPSB9Lg17kZtna1lq01povbiao2PPatslmi65QPY=;\n\tb=niXvPlfYXRhNjXdBECYlKtItHPldG/jO/ZoRsR77WfLGfN6WanFbQJ2LnSv/XrIjYA\n\tOLDerrY0ayzaZD6ofXMmzXbwMJYyVgCdPPisqaiDCMnAiPx/i7/RuKQU7v6QpT05J1r+\n\t1piluiey00Gj/GD02PVBE9qp2QyJzQnM7bc0V2N+Qq1zXcUPMbDLyFn2fv46V44yNSQs\n\tQLsEThBOTPjE+BbCGTx/FR/7DAj6mhItuErdNu+0+2O+UDnGVwWr9kN3njRUF864xD0F\n\tsDD165aI0781gd+vstYANYKojodb/h+OHB7f/GTXqNuNNQpoTrbppfW/uPxjfjyYjM1f\n\tiacQ==","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=LDoyPSB9Lg17kZtna1lq01povbiao2PPatslmi65QPY=;\n\tb=fN8j9JDZbWNKPsWESIFK3RfqIRnZmKO2j+ng3nYqRhGoo0tR/PxvScwaHyU5w9TOXd\n\tdPgLFLababGwmzPGShQQ233477hw7VjCRHKQzdq083ECoCyz1Fj1bHjsvWees2VQPJcu\n\tuFX1WEYg3wLlpYHG1lgS2jBNEKcrE7PzxThXLX9eNDyL5/1W36WMYFeqhomJeuMG4C+h\n\ts53J7PoefsRf1Wr3YgAJKZsLuhA3autgVk0O/lgxendwYykPlesubl4i/kicepSbxKEG\n\tyzVmC37l5e1ZKpTpyQ2XJ7VkEUPeh+4tQOHYXUf2OfLcicIszXO/uAqIuAuyenexb0ZM\n\tOAmg==","X-Gm-Message-State":"AOAM532V0PvMB0xcJgBKvW96dUUt5vfnyLl/gkenbTlhCb5fkPt+wmAP\n\tgwVSqBP82rhY7Rkrv65DHYxOJiU+o7lnyUgvFXE9fQ==","X-Google-Smtp-Source":"ABdhPJxu8zX260NAd1Zd4pXCABpX/h+FLrZfwSBL1pC/8sokpy3GlCNU9Je32b6T45Bk6OC8lE5xnZp7lW9/s45T9s4=","X-Received":"by 2002:a9d:6249:: with SMTP id i9mr4433056otk.166.1615912853747;\n\tTue, 16 Mar 2021 09:40:53 -0700 (PDT)","MIME-Version":"1.0","References":"<20210310172348.4312-1-david.plowman@raspberrypi.com>\n\t\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Tue, 16 Mar 2021 16:40:41 +0000","Message-ID":"","To":"Naushir Patuck ","Subject":"Re: [libcamera-devel] [RFC PATCH 0/1] Raspberry Pi generalised\n\tembedded data parsing","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=\"===============7654464423177608303==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16759,"web_url":"https://patchwork.libcamera.org/comment/16759/","msgid":"","date":"2021-05-04T12:45:01","subject":"Re: [libcamera-devel] [RFC PATCH 0/1] Raspberry Pi generalised\n\tembedded data parsing","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Tue, Mar 16, 2021 at 10:33:40AM +0000, Naushir Patuck wrote:\n> On Sun, 14 Mar 2021 at 22:30, Laurent Pinchart wrote:\n> > On Wed, Mar 10, 2021 at 05:23:47PM +0000, David Plowman wrote:\n> > > Hi\n> > >\n> > > I'm just submitting this patch for comments in the first instance\n> > > (mostly from Naush, I guess, but everyone is welcome!). It's part of\n> > > our plan for more flexible handling of metadata from the sensor.\n> > >\n> > > (The background is that we have some interesting sensors coming up\n> > > that give us other forms of embedded data, not just register\n> > > dumps, and we need to be able to deal with those!)\n> > >\n> > > The plan is to give our CamHelpers a Prepare() and a Process() method,\n> > > just like all our algorithms. As usual, Prepare() runs just before the\n> > > ISP starts, Process() just after. A version of Prepare() is provided\n> > > that has basically just sucked that little bit of\n> > > register-dump-parsing functionality out of the IPA file\n> > > (raspberrypi.cpp). Process() does nothing by default.\n> > >\n> > > There aren't actually many changes, but some observations on what I've\n> > > done:\n> > >\n> > > * I've not updated various CamHelper comments yet, that can wait!\n> > >\n> > > * I've made the Prepare() method responsible for reading the delayed\n> > > control values if we can't use the metadata to get the\n> > > exposure/gain. I wonder if perhaps that is better left in the\n> > > IPA. Prepare() might indicate via a return value whether it found\n> > > them in the embedded data or not.\n> > >\n> > > * The parser object is completely hidden behind the helper now, so the\n> > > distinction between them is rather blurring. Maybe they could be\n> > > combined, but that can happen in a later patch.\n> >\n> > I'm trying to figure out how this will play along with the refactoring\n> > of camera sensor helpers. There are a few points to consider:\n> >\n> > - The helpers are not specific to the Raspberry Pi IPA, and should thus\n> > be moved to a common location. This is more or less a mechanical\n> > change and shouldn't cause any issue by itself. The code could be\n> > moved to libipa for instance.\n> >\n> > - We have sensor-specific data on the pipeline handler side, provided by\n> > the CameraSensor class and passed to the IPA through CameraSensorInfo.\n> > All the information is currently retrieved from the kernel driver.\n> > Work is planned to create a sensor database that will allow hardcoding\n> > sensor-specific information in libcamera.\n> \n> Would the sensor database be a drop in replacement for the CamHelper,\n> or do you think we will still have a CamHelper doing certain operations and\n> the sensor database is for static sensor properties?\n\nThe sensor database will certainly contain static information. Generally\nspeaking, the more we can use static data, the better. We can then have\na cam helper class that would use the static data to perform common\ntasks, such as converting exposure time and gain between sensor-specific\nunits and standard units. Those functions wouldn't need to be virtual\nanymore.\n\nI believe we'll still need virtual functions though, for things like\nembedded data parsing at least, and possibly other operations, as not\neverything can be expressed as data (well, the logic of a function can\nbe expressed as data, in the worst case as assembly instructions, but\nthat's going too far :-) - jokes aside, where to draw the line is an\ninteresting question).\n\n> > I'd like to see if we could centralize all sensor-specific information\n> > on the pipeline handler side, or least for static data. This would\n> > cover information related to delayed controls, which is currently\n> > provided by the IPA but not used by it, but also information about\n> > mistrusted frames, that would then be passed to the IPA by the\n> > pipeline handler.\n> \n> This does seem like the right approach to me.\n> \n> > - There's a need for sensor-specific code (as opposed to data),\n> > currently used on the IPA side. There's room for refactoring this\n> > though, including replacing the virtual Gain and GainCode functions\n> > with parametric formulas. For instance, the CCS specification computes\n> > the sensor gain as either\n> >\n> > - gain = (m0 * x + c0) / (m1 * x + c1), with x being the register\n> > value, m0, m1, c0 and c1 being static parameters, and either m0 or\n> > m1 being equal to 0 ; or\n> >\n> > - gain = a * 2^x, with a and x being register values.\n> >\n> > It seems this wouldn't match the IMX290, but we could add additional\n> > formulas.\n> \n> I think allowing this to remain virtual might still be needed.\n> \n> Some sensors have entirely tables based gain -> code translations, so\n> we should not restrict to only parametric formulas.\n\n:-( Is this common ? The tables could be stored in the sensor database\nthough.\n\n> Similar comment for\n> exposure lines -> time calculations. Some sensors I've encountered\n> (global shutter and ultra long exposure modes) do not follow the typical\n> line length * frame_length approach.\n\nDo they need very device-specific conversion, or are there a set of\ncommon conversions that would cover all our needs ?\n\n> > - For embedded data parsing, I wonder if it would be best handled in the\n> > IPA or in the pipeline handler. The latter would have the advantage of\n> > gathering more sensor-specific code on the pipeline handler side, as\n> > well as not having to involve the IPA in the parsing for other types\n> > of ancillary data produced by the sensors.\n> \n> Having the embedded data parsing in the pipeline handler does seem\n> like a reasonable thing to do. However, we must keep in mind that\n> there may be vendor specific data in there (e.g. focus pixel stats) that\n> will need to be passed into the IPA. This may happen through named\n> controls, or some other opaque way.\n\nGood point. When sensors produce raw PDAF statistics, are they\ntransmitted as part of the \"regular\" embedded data, or separately ?\n\n> > > David Plowman (1):\n> > > ipa: raspberrypi: Use CamHelpers to generalise embedded data parsing\n> > >\n> > > src/ipa/raspberrypi/cam_helper.cpp | 49 ++++++++++++++++\n> > > src/ipa/raspberrypi/cam_helper.hpp | 14 ++++-\n> > > src/ipa/raspberrypi/raspberrypi.cpp | 88 ++++++++---------------------\n> > > 3 files changed, 84 insertions(+), 67 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 A6BDCBDE7A\n\tfor ;\n\tTue, 4 May 2021 12:45:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0C48C68918;\n\tTue, 4 May 2021 14:45:07 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 225C4602BF\n\tfor ;\n\tTue, 4 May 2021 14:45:05 +0200 (CEST)","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 ADDF858E;\n\tTue, 4 May 2021 14:45:04 +0200 (CEST)"],"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=\"oFwCg/5X\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1620132304;\n\tbh=a9F1a4LkBYbKgrB064uJQuApPHFpS61hEBg8xThgkpE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=oFwCg/5X5MSkGADSicfRsA17J9UNuMwg1gTdbZvSwTJTWfk+DBSzASn7zscip7/NY\n\trkh/+aIRCy5uXe1eRdUEHxsXUi/BXrJuIoax2WoRkVrkzCvUpBPm+F1Um0mapnnPkW\n\t5ZW6gRSsiBGkLXXG+fpcoAovyA6TiBAWhfVarO10=","Date":"Tue, 4 May 2021 15:45:01 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210310172348.4312-1-david.plowman@raspberrypi.com>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH 0/1] Raspberry Pi generalised\n\tembedded data parsing","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":16764,"web_url":"https://patchwork.libcamera.org/comment/16764/","msgid":"","date":"2021-05-04T23:45:40","subject":"Re: [libcamera-devel] [RFC PATCH 0/1] Raspberry Pi generalised\n\tembedded data parsing","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, Mar 16, 2021 at 04:40:41PM +0000, David Plowman wrote:\n> Hi Naush, Laurent\n> \n> Thanks for the replies on this subject. In fact I'm still fiddling around,\n> and discovering some new problems, so there's certainly still some way to\n> go!\n> \n> One new thing I'm wrestling with at the moment is a sensor where the\n> \"usual\" relationship between binned and full res modes, in respect of noise\n> levels, doesn't hold. I suppose this is all information you could encode in\n> a database, though I wonder whether it might become quite burdensome.\n\nIt has to be encoded somewhere, right ? I don't know the details for\nthis specific sensor, but from your description I don't think we\ncould/should convey that information through V4L2, so it would need to\nbe stored somewhere in userspace.\n\n> As things stand currently, I'm thinking about making\n> CamHelper::SetCameraMode virtual, so that we can amend the mode for certain\n> sensors. The CameraMode that we pass to our SwitchMode methods would have\n> to come from the helper.\n\nFollowing our recent discussions on the topic of virtual functions, I'm\nnot completely opposed to them as they have valid use cases, but I also\nbelieve that most sensor features could be described with static data in\nmost cases. Static data has the advantage of providing more information\nthan \"I've mangled the parameters you gave me, here's the result\" to the\nIPA, possibly enabling it to make more clever choices. Maybe a good\nmiddle ground would be to make these camera helper functions virtual for\ncorner cases, with a base implementation that works for most sensors,\nbased on static data.\n\n> On some of the other questions:\n> \n> On Tue, 16 Mar 2021 at 10:33, Naushir Patuck wrote:\n> > On Sun, 14 Mar 2021 at 22:30, Laurent Pinchart wrote:\n> >> On Wed, Mar 10, 2021 at 05:23:47PM +0000, David Plowman wrote:\n> >> > Hi\n> >> >\n> >> > I'm just submitting this patch for comments in the first instance\n> >> > (mostly from Naush, I guess, but everyone is welcome!). It's part of\n> >> > our plan for more flexible handling of metadata from the sensor.\n> >> >\n> >> > (The background is that we have some interesting sensors coming up\n> >> > that give us other forms of embedded data, not just register\n> >> > dumps, and we need to be able to deal with those!)\n> >> >\n> >> > The plan is to give our CamHelpers a Prepare() and a Process() method,\n> >> > just like all our algorithms. As usual, Prepare() runs just before the\n> >> > ISP starts, Process() just after. A version of Prepare() is provided\n> >> > that has basically just sucked that little bit of\n> >> > register-dump-parsing functionality out of the IPA file\n> >> > (raspberrypi.cpp). Process() does nothing by default.\n> >> >\n> >> > There aren't actually many changes, but some observations on what I've\n> >> > done:\n> >> >\n> >> > * I've not updated various CamHelper comments yet, that can wait!\n> >> >\n> >> > * I've made the Prepare() method responsible for reading the delayed\n> >> > control values if we can't use the metadata to get the\n> >> > exposure/gain. I wonder if perhaps that is better left in the\n> >> > IPA. Prepare() might indicate via a return value whether it found\n> >> > them in the embedded data or not.\n>\n> I don't like what I did here. I'd prefer to move the code that fetches\n> values from the controls out of the helper into the IPA, that is,\n> src/ipa/raspberrypi/raspberrypi.cpp.\n\nSounds good to me.\n\n> >> > * The parser object is completely hidden behind the helper now, so the\n> >> > distinction between them is rather blurring. Maybe they could be\n> >> > combined, but that can happen in a later patch.\n> >>\n> >> I'm trying to figure out how this will play along with the refactoring\n> >> of camera sensor helpers. There are a few points to consider:\n> >>\n> >> - The helpers are not specific to the Raspberry Pi IPA, and should thus\n> >> be moved to a common location. This is more or less a mechanical\n> >> change and shouldn't cause any issue by itself. The code could be\n> >> moved to libipa for instance.\n> >>\n> >> - We have sensor-specific data on the pipeline handler side, provided by\n> >> the CameraSensor class and passed to the IPA through CameraSensorInfo.\n> >> All the information is currently retrieved from the kernel driver.\n> >> Work is planned to create a sensor database that will allow hardcoding\n> >> sensor-specific information in libcamera.\n> >\n> > Would the sensor database be a drop in replacement for the CamHelper,\n> > or do you think we will still have a CamHelper doing certain operations and\n> > the sensor database is for static sensor properties?\n> \n> I suspect a database is not enough. More below...\n> \n> >> I'd like to see if we could centralize all sensor-specific information\n> >> on the pipeline handler side, or least for static data. This would\n> >> cover information related to delayed controls, which is currently\n> >> provided by the IPA but not used by it, but also information about\n> >> mistrusted frames, that would then be passed to the IPA by the\n> >> pipeline handler.\n> >\n> > This does seem like the right approach to me.\n> >\n> >> - There's a need for sensor-specific code (as opposed to data),\n> >> currently used on the IPA side. There's room for refactoring this\n> >> though, including replacing the virtual Gain and GainCode functions\n> >> with parametric formulas. For instance, the CCS specification computes\n> >> the sensor gain as either\n> >>\n> >> - gain = (m0 * x + c0) / (m1 * x + c1), with x being the register\n> >> value, m0, m1, c0 and c1 being static parameters, and either m0 or\n> >> m1 being equal to 0 ; or\n> >>\n> >> - gain = a * 2^x, with a and x being register values.\n> >>\n> >> It seems this wouldn't match the IMX290, but we could add additional\n> >> formulas.\n> >\n> > I think allowing this to remain virtual might still be needed.\n> >\n> > Some sensors have entirely tables based gain -> code translations, so\n> > we should not restrict to only parametric formulas. Similar comment for\n> > exposure lines -> time calculations. Some sensors I've encountered\n> > (global shutter and ultra long exposure modes) do not follow the typical\n> > line length * frame_length approach.\n> \n> Agree. I have a sensor where some modes can't have a gain less than (in\n> this case) 1.12x. I'm \"hiding\" that in the CamHelper methods. Also the\n> binned mode produces 2x the signal level of the full res mode. This too I'm\n> \"hiding\" in the CamHelper methods (though it doesn't feel like the best\n> solution.) There seem to be quite a few \"awkward\" devices out there!\n\nThat seems a reasonable place to handle that, regarless of whether we\nexpress the information as code, or as data used by helper functions. I\nbelieve we'll end up merging the camera helpers and the camera sensor\ndatabase into a single entity at some point.\n\n> >> - For embedded data parsing, I wonder if it would be best handled in the\n> >> IPA or in the pipeline handler. The latter would have the advantage of\n> >> gathering more sensor-specific code on the pipeline handler side, as\n> >> well as not having to involve the IPA in the parsing for other types\n> >> of ancillary data produced by the sensors.\n> >\n> > Having the embedded data parsing in the pipeline handler does seem\n> > like a reasonable thing to do. However, we must keep in mind that\n> > there may be vendor specific data in there (e.g. focus pixel stats) that\n> > will need to be passed into the IPA. This may happen through named\n> > controls, or some other opaque way.\n> \n> I'd be OK to see embedded data parsing move out of the helper, though I\n> think that being able to cope with other types of metadata is a powerful\n> feature. It might be focus pixels, or other kinds of image analysis. In\n> general I would expect the CamHelper::Prepare() method to put this\n> information into the Raspberry Pi metadata object. From there, an algorithm\n> would be loaded by being listed in the json file, which would retrieve the\n> item from the Raspberry Pi metadata and handle it.\n> \n> I'm thinking I should perhaps submit another version that encapsulates my\n> latest thoughts on what I think I need - then we can consider what might\n> benefit from being done differently.\n\nSounds good to me too. We probably want to experiment anyway, as there\nare quite a few new features and use cases here. We don't need to aim\nfor perfection from day one :-) Please consider my comments as part of\nthe brainstorming effort, I'm particularly interested in knowing what\nyou think makes sense, either right away or as a future enhancement, and\nwhat wouldn't be a good idea. \n\n> >> > David Plowman (1):\n> >> > ipa: raspberrypi: Use CamHelpers to generalise embedded data parsing\n> >> >\n> >> > src/ipa/raspberrypi/cam_helper.cpp | 49 ++++++++++++++++\n> >> > src/ipa/raspberrypi/cam_helper.hpp | 14 ++++-\n> >> > src/ipa/raspberrypi/raspberrypi.cpp | 88 ++++++++---------------------\n> >> > 3 files changed, 84 insertions(+), 67 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 92561BDE79\n\tfor ;\n\tTue, 4 May 2021 23:45:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EAFE368918;\n\tWed, 5 May 2021 01:45:48 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AE880688AC\n\tfor ;\n\tWed, 5 May 2021 01:45:46 +0200 (CEST)","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 0B207556;\n\tWed, 5 May 2021 01:45:44 +0200 (CEST)"],"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=\"uZgKW7Ag\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1620171945;\n\tbh=ZKGEkSm+u/BMkPbZHjCeOxyXx3rwRYT4VlSuPARhk9E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=uZgKW7AgUCOgVxZm9WOXHFdoxnpzfA2uWoR0cufrdv83mbNhDS0KrK2DgcJ2bnhy2\n\tWGVvvXu5U7HgX/UymQ0nsCzLHfs0/H9mQjZyG2iH5RqndT30WyJwp/EsTEK8+6Ee5H\n\tzxO/fr2iGj6edlLML+6e4qSmflBc3ETAzv30ktrY=","Date":"Wed, 5 May 2021 02:45:40 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"","References":"<20210310172348.4312-1-david.plowman@raspberrypi.com>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH 0/1] Raspberry Pi generalised\n\tembedded data parsing","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":16768,"web_url":"https://patchwork.libcamera.org/comment/16768/","msgid":"","date":"2021-05-05T09:09:31","subject":"Re: [libcamera-devel] [RFC PATCH 0/1] Raspberry Pi generalised\n\tembedded data parsing","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nOn Tue, 4 May 2021 at 13:45, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> On Tue, Mar 16, 2021 at 10:33:40AM +0000, Naushir Patuck wrote:\n> > On Sun, 14 Mar 2021 at 22:30, Laurent Pinchart wrote:\n> > > On Wed, Mar 10, 2021 at 05:23:47PM +0000, David Plowman wrote:\n> > > > Hi\n> > > >\n> > > > I'm just submitting this patch for comments in the first instance\n> > > > (mostly from Naush, I guess, but everyone is welcome!). It's part of\n> > > > our plan for more flexible handling of metadata from the sensor.\n> > > >\n> > > > (The background is that we have some interesting sensors coming up\n> > > > that give us other forms of embedded data, not just register\n> > > > dumps, and we need to be able to deal with those!)\n> > > >\n> > > > The plan is to give our CamHelpers a Prepare() and a Process()\n> method,\n> > > > just like all our algorithms. As usual, Prepare() runs just before\n> the\n> > > > ISP starts, Process() just after. A version of Prepare() is provided\n> > > > that has basically just sucked that little bit of\n> > > > register-dump-parsing functionality out of the IPA file\n> > > > (raspberrypi.cpp). Process() does nothing by default.\n> > > >\n> > > > There aren't actually many changes, but some observations on what\n> I've\n> > > > done:\n> > > >\n> > > > * I've not updated various CamHelper comments yet, that can wait!\n> > > >\n> > > > * I've made the Prepare() method responsible for reading the delayed\n> > > > control values if we can't use the metadata to get the\n> > > > exposure/gain. I wonder if perhaps that is better left in the\n> > > > IPA. Prepare() might indicate via a return value whether it found\n> > > > them in the embedded data or not.\n> > > >\n> > > > * The parser object is completely hidden behind the helper now, so\n> the\n> > > > distinction between them is rather blurring. Maybe they could be\n> > > > combined, but that can happen in a later patch.\n> > >\n> > > I'm trying to figure out how this will play along with the refactoring\n> > > of camera sensor helpers. There are a few points to consider:\n> > >\n> > > - The helpers are not specific to the Raspberry Pi IPA, and should thus\n> > > be moved to a common location. This is more or less a mechanical\n> > > change and shouldn't cause any issue by itself. The code could be\n> > > moved to libipa for instance.\n> > >\n> > > - We have sensor-specific data on the pipeline handler side, provided\n> by\n> > > the CameraSensor class and passed to the IPA through\n> CameraSensorInfo.\n> > > All the information is currently retrieved from the kernel driver.\n> > > Work is planned to create a sensor database that will allow\n> hardcoding\n> > > sensor-specific information in libcamera.\n> >\n> > Would the sensor database be a drop in replacement for the CamHelper,\n> > or do you think we will still have a CamHelper doing certain operations\n> and\n> > the sensor database is for static sensor properties?\n>\n> The sensor database will certainly contain static information. Generally\n> speaking, the more we can use static data, the better. We can then have\n> a cam helper class that would use the static data to perform common\n> tasks, such as converting exposure time and gain between sensor-specific\n> units and standard units. Those functions wouldn't need to be virtual\n> anymore.\n>\n> I believe we'll still need virtual functions though, for things like\n> embedded data parsing at least, and possibly other operations, as not\n> everything can be expressed as data (well, the logic of a function can\n> be expressed as data, in the worst case as assembly instructions, but\n> that's going too far :-) - jokes aside, where to draw the line is an\n> interesting question).\n>\n\nYes, this is a tricky one. I suspect we will just need to add support for\na whole bunch of sensors slowly, and a logical partition may emerge.\n\n\n>\n> > > I'd like to see if we could centralize all sensor-specific\n> information\n> > > on the pipeline handler side, or least for static data. This would\n> > > cover information related to delayed controls, which is currently\n> > > provided by the IPA but not used by it, but also information about\n> > > mistrusted frames, that would then be passed to the IPA by the\n> > > pipeline handler.\n> >\n> > This does seem like the right approach to me.\n> >\n> > > - There's a need for sensor-specific code (as opposed to data),\n> > > currently used on the IPA side. There's room for refactoring this\n> > > though, including replacing the virtual Gain and GainCode functions\n> > > with parametric formulas. For instance, the CCS specification\n> computes\n> > > the sensor gain as either\n> > >\n> > > - gain = (m0 * x + c0) / (m1 * x + c1), with x being the register\n> > > value, m0, m1, c0 and c1 being static parameters, and either m0 or\n> > > m1 being equal to 0 ; or\n> > >\n> > > - gain = a * 2^x, with a and x being register values.\n> > >\n> > > It seems this wouldn't match the IMX290, but we could add additional\n> > > formulas.\n> >\n> > I think allowing this to remain virtual might still be needed.\n> >\n> > Some sensors have entirely tables based gain -> code translations, so\n> > we should not restrict to only parametric formulas.\n>\n> :-( Is this common ? The tables could be stored in the sensor database\n> though.\n>\n\nI would not say it is common, but I have encountered a few over the years.\nIn these cases, storing a table in the database should not be too bothersome\nI would hope.\n\n\n>\n> > Similar comment for\n> > exposure lines -> time calculations. Some sensors I've encountered\n> > (global shutter and ultra long exposure modes) do not follow the typical\n> > line length * frame_length approach.\n>\n> Do they need very device-specific conversion, or are there a set of\n> common conversions that would cover all our needs ?\n>\n\nAgain, this is not common, but I have seen device specific conversions\nthat may be required. Unfortunately, this may not even be vendor specific.\n\n\n>\n> > > - For embedded data parsing, I wonder if it would be best handled in\n> the\n> > > IPA or in the pipeline handler. The latter would have the advantage\n> of\n> > > gathering more sensor-specific code on the pipeline handler side, as\n> > > well as not having to involve the IPA in the parsing for other types\n> > > of ancillary data produced by the sensors.\n> >\n> > Having the embedded data parsing in the pipeline handler does seem\n> > like a reasonable thing to do. However, we must keep in mind that\n> > there may be vendor specific data in there (e.g. focus pixel stats) that\n> > will need to be passed into the IPA. This may happen through named\n> > controls, or some other opaque way.\n>\n> Good point. When sensors produce raw PDAF statistics, are they\n> transmitted as part of the \"regular\" embedded data, or separately ?\n>\n\nFrom my very little experience with such sensors, it comes as embedded\ndata - the register set itself includes pdaf statistics. I do know one\nother\nvendor that can provide an entirely separate stream for such statistics,\nbut our Unciam block does not support more than 2 streams.\n\nRegards,\nNaush\n\n\n> > > > David Plowman (1):\n> > > > ipa: raspberrypi: Use CamHelpers to generalise embedded data\n> parsing\n> > > >\n> > > > src/ipa/raspberrypi/cam_helper.cpp | 49 ++++++++++++++++\n> > > > src/ipa/raspberrypi/cam_helper.hpp | 14 ++++-\n> > > > src/ipa/raspberrypi/raspberrypi.cpp | 88\n> ++++++++---------------------\n> > > > 3 files changed, 84 insertions(+), 67 deletions(-)\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 6D299BDE7B\n\tfor ;\n\tWed, 5 May 2021 09:09:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CDE9F68918;\n\tWed, 5 May 2021 11:09:50 +0200 (CEST)","from mail-lf1-x132.google.com (mail-lf1-x132.google.com\n\t[IPv6:2a00:1450:4864:20::132])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 70792602BD\n\tfor ;\n\tWed, 5 May 2021 11:09:48 +0200 (CEST)","by mail-lf1-x132.google.com with SMTP id x2so1526403lff.10\n\tfor ;\n\tWed, 05 May 2021 02:09:48 -0700 (PDT)"],"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=\"ZyhrB/Ua\"; 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=mQYZjdbycJZpi/qiZS8Uyy98HshG/cf1fMhK/spKtQ8=;\n\tb=ZyhrB/Ua2o/bSLGIyd/RNlrhkv/rYorecYWZ3sRiI6dkO1hwDpdzARidl/3VOW09fa\n\tXgl3PcvT0409wdNJJqxgVGPNwclGEzQGVTawENc97g8u+P8UWDBgUf5yzmipfoZQ/824\n\tuo8B1Diudmfypiq+TfU0mEY0Ozy5IxawlqZLxcarTRS7bhq5zHMeLm+n0T2bkDrcy1Lb\n\t9bTfJWlBi5+0uURs3pijSFjEefVPc8zU8GS+r33y6SZc+SQVtz3dvZCeeymNYCiZ4ulw\n\tdJgas50dxuh2h3JiV34cyB4DGp61829uq3RHxKX11uUlUV+3zXipXe/KP9R/1GKifQP4\n\t5UcQ==","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=mQYZjdbycJZpi/qiZS8Uyy98HshG/cf1fMhK/spKtQ8=;\n\tb=XqUHWJwAmkxmdkf1Sdlb36WB6P3fO9u4iNQgRzyhOvxkj9kn+8zuU468fi/kVH2uUd\n\te3wFQdSAeNLc+/pMl5f/N1xGIkXGKnygfTz1aivmte01/3DCWhei0OyOZ8x8jCfDwiN5\n\tbkqGrpCgWGLc1Z4bdjwt34Pbcv2keSP4ty81wh98or6csZkQNfOJGz/DpKf0+NDLboKB\n\tcUIJr02xIsczgLv5BL4AjYoOSPi6VGLaBhpQQ5rdCqGf7Gf8ebpQ9sx9A5MAyxnOnwLS\n\tFpfWgijCay7/7xlJTkU+3XAc2vJjPQprD+U25yoIH2Uq5MKMNjGmsmB3fUZtNujdnpBP\n\tYl9Q==","X-Gm-Message-State":"AOAM530YgyyES/YmhbORKWVwPxBjo08vFydH6pHGkUn91473+6YdlNL4\n\trrYdt0P7WdvGaqkF6nHmjRP6sHIBRFMDlgTuNVwg9A==","X-Google-Smtp-Source":"ABdhPJyTBT1ZqkFfzQI0H5I+cSXkBubPjCl/BTsUYVjdxmclJGITpjWCF6B27cI4gf4WWDrbTFQMS/qyAEluSGaO3G8=","X-Received":"by 2002:a05:6512:10d4:: with SMTP id\n\tk20mr5408408lfg.210.1620205787830; \n\tWed, 05 May 2021 02:09:47 -0700 (PDT)","MIME-Version":"1.0","References":"<20210310172348.4312-1-david.plowman@raspberrypi.com>\n\t\n\t\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Wed, 5 May 2021 10:09:31 +0100","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [RFC PATCH 0/1] Raspberry Pi generalised\n\tembedded data parsing","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=\"===============8399587701709546060==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]