[{"id":25643,"web_url":"https://patchwork.libcamera.org/comment/25643/","msgid":"<20221028090951.35cilhmbvhbvikhb@uno.localdomain>","date":"2022-10-28T09:09:51","subject":"Re: [libcamera-devel] [PATCH v5 03/10] ipa: add rkisp1 metadata to\n\tfix Android HAL","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Nicholas\n\nsubject as\n\nipa: rkisp1: Add FrameDurationLimits control\n\nThis is not just for Android\n\nOn Thu, Oct 27, 2022 at 10:17:19PM -0500, Nicholas Roth via libcamera-devel wrote:\n> From: Nicholas Roth \n>\n> Currently, the Android HAL does not work on rkisp1-based devices because\n> required FrameDurationLimits metadata is missing from the ISP\n\nIPA = Image Processing Algorithm\nit's the software component part of libcamera that drives the image\nenhancement algorithms. IPA is libcamera lingo, in other context it\nmight be simply called \"3A library\" even if it does way more than just\nAuto[exposure, white-balance, focus].\n\nISP = Image Signal Processor\nit's the HW component that performs image enhancement computations and\nproduces statistics on the processed image\n\nThe two terms are not interchangeable\n\n> implementation.\n>\n> This change introduces sensible defaults for FrameDurationLimits that\n> should generally work most of the time.\n>\n> In theory, it should be possible to implement more sophisticated logic\n> to detect frame durations. The appropriate API hooks for doing so exist\n> and are used in IPAIPU3::updateControls(), although the implementation\n> may be suspect. This is left as future work.\n>\n> Signed-off-by: Nicholas Roth \n> ---\n> src/ipa/rkisp1/rkisp1.cpp | 6 ++++++\n> 1 file changed, 6 insertions(+)\n>\n> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> index ba3c547e..dd12c341 100644\n> --- a/src/ipa/rkisp1/rkisp1.cpp\n> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> @@ -100,6 +100,12 @@ const ControlInfoMap::Map rkisp1Controls{\n> \t{ &controls::Contrast, ControlInfo(0.0f, 1.993f) },\n> \t{ &controls::Saturation, ControlInfo(0.0f, 1.993f) },\n> \t{ &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) },\n> +\t/* libcamera requires a fixed value for minimum frame duration,\n> +\t * but this depends on the frame size and the rkisp1 device datasheets\n> +\t * measure this in pixels per second. Neither the datasheets nor the driver\n\nWhat are you referring to as \"the rkisp1 device datasheet measures\nthis in pixels per second\" ? Is it the ISP bandwidth ? Where is this\ninformation available from ?\n\nI'm still of opinion that we should report the sensor's duration\nlimits, but maybe I'm missing a point here.\n\n\n> +\t * specify a maximum. The minimum below is for 1920x1920. The maximum\n\nIs 1920x1920 is the RkISP1 self-path maximum output resolution that\nyou have used ? If the self-path has a scaler (something I'm not 100%\nsure) images in such resolution might be obtained by scaling the frame\nas produced by the sensor, or by simply cropping them.\n\nIf you connect to the ISP a sensor whose minimum resolution and\nmax framerate is 2592x1944@15FPS you would get a min frame duration of:\n\n (2592 + hblank) * (1944 + vblank) / pixel_rate = 66666 usec\n\nwhich is larger than what you report here, as the sensor would be\nclocking out frames at such rate, regardless of the final output\nresolution. Moreover I'm not sure how you got to 48505 from 1920x1920,\nhave you taken into account the pixel rate of the sensor you're using ?\nThis can't be hardcoded but should come from the CameraSensor,\ndifferent platforms use different sensors and we can't assume anything\nabout it in the IPA module.\n\nI would say that the ISP output size limits should not be taken into\naccount. It would be different if we had a clear documentation about\nhow bandwidth limitations constraints the durations, but I'm afraid\nthe relationship between the two parameters is hard to measure, if\never documented.\n\n> +\t * corresponds to two seconds. */\n\nMulti-line comments should be\n\n /*\n * comment on multiple\n * lines\n */\n\n> +\t{ &controls::FrameDurationLimits, ControlInfo(48505, 2000000) },\n> \t{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },\n> };\n>\n> --\n> 2.34.1\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 D0AA2BDB16\n\tfor ;\n\tFri, 28 Oct 2022 09:09:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4CB3462F41;\n\tFri, 28 Oct 2022 11:09:54 +0200 (CEST)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 54A9362F41\n\tfor ;\n\tFri, 28 Oct 2022 11:09:53 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id CCD7F240002;\n\tFri, 28 Oct 2022 09:09:52 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666948194;\n\tbh=0HYFlcLw/NUn71Vg3XE5LMK3dgGEXIk6DPp/KyskHUs=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=q0ChSo6hr7JArbuifNhmAMSLqDaH97RvCKRr1KtUniY6LpO69B8P3hRZ6H6OjRHh3\n\ty3OFrWmiMosYaWNbDhbck9NA+P2d0RGEyJoljfwB8ti+39Tuq0jPaFA6hw39nJMsUA\n\txwBuGrDyX7GbWBvdv1Uy1jIEGdjLXouRNAH/4mma2wYtouSTcpG/5uRx1olmRp2RiK\n\t8VXCeKpFHQhCVItkj3UN5a10WayIT2R2r4RnNaOyWGp3PRAA+h0s0W/iq3LgCLpDBJ\n\t5tVJA/0BfCNnjRBFLvUiWyWdj1NxiZjbqXGyr/9K1i2WWZXX6gUJTGgeSSXUIcFd+0\n\tX2mbsOGgrOidQ==","Date":"Fri, 28 Oct 2022 11:09:51 +0200","To":"Nicholas Roth via libcamera-devel ","Message-ID":"<20221028090951.35cilhmbvhbvikhb@uno.localdomain>","References":"<20221027224135.348115-1-nicholas@rothemail.net>\n\t<20221028031726.4849-1-nicholas@rothemail.net>\n\t<20221028031726.4849-4-nicholas@rothemail.net>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221028031726.4849-4-nicholas@rothemail.net>","Subject":"Re: [libcamera-devel] [PATCH v5 03/10] ipa: add rkisp1 metadata to\n\tfix Android HAL","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"nicholas@rothemail.net","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25646,"web_url":"https://patchwork.libcamera.org/comment/25646/","msgid":"","date":"2022-10-28T09:57:10","subject":"Re: [libcamera-devel] [PATCH v5 03/10] ipa: add rkisp1 metadata to\n\tfix Android HAL","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi all,\n\n\nOn Fri, 28 Oct 2022 at 10:09, Jacopo Mondi via libcamera-devel <\nlibcamera-devel@lists.libcamera.org> wrote:\n\n> Hi Nicholas\n>\n> subject as\n>\n> ipa: rkisp1: Add FrameDurationLimits control\n>\n> This is not just for Android\n>\n> On Thu, Oct 27, 2022 at 10:17:19PM -0500, Nicholas Roth via\n> libcamera-devel wrote:\n> > From: Nicholas Roth \n> >\n> > Currently, the Android HAL does not work on rkisp1-based devices because\n> > required FrameDurationLimits metadata is missing from the ISP\n>\n> IPA = Image Processing Algorithm\n> it's the software component part of libcamera that drives the image\n> enhancement algorithms. IPA is libcamera lingo, in other context it\n> might be simply called \"3A library\" even if it does way more than just\n> Auto[exposure, white-balance, focus].\n>\n> ISP = Image Signal Processor\n> it's the HW component that performs image enhancement computations and\n> produces statistics on the processed image\n>\n> The two terms are not interchangeable\n>\n> > implementation.\n> >\n> > This change introduces sensible defaults for FrameDurationLimits that\n> > should generally work most of the time.\n> >\n> > In theory, it should be possible to implement more sophisticated logic\n> > to detect frame durations. The appropriate API hooks for doing so exist\n> > and are used in IPAIPU3::updateControls(), although the implementation\n> > may be suspect. This is left as future work.\n> >\n> > Signed-off-by: Nicholas Roth \n> > ---\n> > src/ipa/rkisp1/rkisp1.cpp | 6 ++++++\n> > 1 file changed, 6 insertions(+)\n> >\n> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > index ba3c547e..dd12c341 100644\n> > --- a/src/ipa/rkisp1/rkisp1.cpp\n> > +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > @@ -100,6 +100,12 @@ const ControlInfoMap::Map rkisp1Controls{\n> > { &controls::Contrast, ControlInfo(0.0f, 1.993f) },\n> > { &controls::Saturation, ControlInfo(0.0f, 1.993f) },\n> > { &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) },\n> > + /* libcamera requires a fixed value for minimum frame duration,\n> > + * but this depends on the frame size and the rkisp1 device\n> datasheets\n> > + * measure this in pixels per second. Neither the datasheets nor\n> the driver\n>\n> What are you referring to as \"the rkisp1 device datasheet measures\n> this in pixels per second\" ? Is it the ISP bandwidth ? Where is this\n> information available from ?\n>\n> I'm still of opinion that we should report the sensor's duration\n> limits, but maybe I'm missing a point here.\n>\n>\n> > + * specify a maximum. The minimum below is for 1920x1920. The\n> maximum\n>\n> Is 1920x1920 is the RkISP1 self-path maximum output resolution that\n> you have used ? If the self-path has a scaler (something I'm not 100%\n> sure) images in such resolution might be obtained by scaling the frame\n> as produced by the sensor, or by simply cropping them.\n>\n> If you connect to the ISP a sensor whose minimum resolution and\n> max framerate is 2592x1944@15FPS you would get a min frame duration of:\n>\n> (2592 + hblank) * (1944 + vblank) / pixel_rate = 66666\n> usec\n>\n> which is larger than what you report here, as the sensor would be\n> clocking out frames at such rate, regardless of the final output\n> resolution. Moreover I'm not sure how you got to 48505 from 1920x1920,\n> have you taken into account the pixel rate of the sensor you're using ?\n> This can't be hardcoded but should come from the CameraSensor,\n> different platforms use different sensors and we can't assume anything\n> about it in the IPA module.\n>\n> I would say that the ISP output size limits should not be taken into\n> account. It would be different if we had a clear documentation about\n> how bandwidth limitations constraints the durations, but I'm afraid\n> the relationship between the two parameters is hard to measure, if\n> ever documented.\n>\n\nI agree with Jacopo on this - FrameDurationLimits ought to specify the\nsensor\nlimits and not the ISP. I can't speak for other platforms, but on a\nRaspberry Pi the ISP throughput depends on many many things, such as\npipeline configuration, hardware clock rate, memory bandwidth availability\nto\nname a few. It would be wholly impractical to provide such a number in the\nFrameDurationLimits control.\n\nNaush\n\n\n\n>\n> > + * corresponds to two seconds. */\n>\n> Multi-line comments should be\n>\n> /*\n> * comment on multiple\n> * lines\n> */\n>\n> > + { &controls::FrameDurationLimits, ControlInfo(48505, 2000000) },\n> > { &controls::draft::NoiseReductionMode,\n> ControlInfo(controls::draft::NoiseReductionModeValues) },\n> > };\n> >\n> > --\n> > 2.34.1\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 31190BDB16\n\tfor ;\n\tFri, 28 Oct 2022 09:57:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A2E8562FC5;\n\tFri, 28 Oct 2022 11:57:28 +0200 (CEST)","from mail-qv1-xf2c.google.com (mail-qv1-xf2c.google.com\n\t[IPv6:2607:f8b0:4864:20::f2c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6A03E62F41\n\tfor ;\n\tFri, 28 Oct 2022 11:57:27 +0200 (CEST)","by mail-qv1-xf2c.google.com with SMTP id mi9so3723612qvb.8\n\tfor ;\n\tFri, 28 Oct 2022 02:57:27 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1666951048;\n\tbh=mEjt9OXT4lUQrk+czafxVrFDUxpyOcoNZn5rY3dTMl4=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=xBfXD60jzUanWmP+qR2ut3/iAuSIt2mn0ujNX64Bpv8ky3wENJfmsuk/Ah3a5LbCM\n\tk8sqvwnjPFSk2OAuDv9gp2zEUc4F42HiHwlxsFdRTE0YleB4+MvQvS9V4MDm10DMGY\n\t1Ct60i9DYpRkVtRHCQzemZUfIBGXYiBnxktGd0NehMIyI9tXE04umbup4dEMhEjnZT\n\tP7QBC4yjYjAEOwcqpCm8MpYMeLFixaNb2h7H5LWkUhzyGm7VHM1L3tHI410a9lJe1s\n\tNHX1kclF7Zwx3wErddnrB7QZohIcxOV2H8BVIb3YfTBnNuZ4DQqeaYihwix4Jvh+en\n\t3d+6TVuX/mdCw==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=G9YtvOiYRyIhNx4A0mjlrAMPAn5HVrKXNUuY/Nrdy7g=;\n\tb=m4pkuUuHhAiVY/TJZ4EFF9knAvgLrM40+wOTM1pJSKZd6QTCRAwGrOx2FPt57Q3pnW\n\t5XdY0tcqINmzzRRtDiGIoSjKIEUJnxNopcW8S/pg7APXz2S0yuqcHXUtSR1dmr1nDdgO\n\ttHyQ3tLKNTYhvFM0qirtvPFulvztPbBCg1oF7etMrdVnvSaXqk/QmKJwiPFbT1H8bK6K\n\t259Q7K+Ihx9ve5kllkjXVwQTl7z/estllfyd0c1P4tnvd9CdyWR/qGwRyBlLwSDUsFnO\n\tyGA6Zp/6pKLY+mRlZnm5elMd0bB+6+s08t03mOihi5nZv3P1mZcPhoCjH3BPnjme+wKK\n\tCQWQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"m4pkuUuH\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=G9YtvOiYRyIhNx4A0mjlrAMPAn5HVrKXNUuY/Nrdy7g=;\n\tb=RcFe40fG+Dc6Y2CsBrZfLSRhluCA0p3Id2a9LlAkWleBzh5FJvnmRVqc8LfkU3tkx7\n\t2nUutNWKLDGP0ATsVkKPvE23Vnlxahp3yPE+ukGloXjx7cGYeEQ+12b7Ztzy7bB55VNn\n\tpoPaE61jWq1oeHOfy7QY0NnV9296W0JixoH+FROEIzhm9y3d2s+tpd2+mHOkSb7dUyWR\n\tCkfCY6+kG8JxSaVIRluHIiVZDCKH7Q5H4fzZRJS2OvyjgeZjITRFw24oExWspZNgq5Oa\n\tHPQ0uR+311remBolKfCunc2XUO7cif+2F1ZiverxtDjmYt07DcJLtTGbdNCM/aaFg6if\n\tYOxQ==","X-Gm-Message-State":"ACrzQf0rm2I8VbmzZJGwpGIQK/NJ5gFopQOJZnz/CN0eFreolzAUBk9J\n\tDGX7rIdsbSpfrCpx6ThllX7PFQDH7q/D88cQN3X1+yxFliOx+w==","X-Google-Smtp-Source":"AMsMyM6WcnukLFKVF+PGe2MO8qAr4fMi+ek4L+qDDOHZpgKFgbiQ8y7DJnyPbllTz8igBWRBSGMWwqPvs/Pi7W/6MNY=","X-Received":"by 2002:a05:6214:d87:b0:4bb:942b:f5f9 with SMTP id\n\te7-20020a0562140d8700b004bb942bf5f9mr11083455qve.126.1666951046333;\n\tFri, 28 Oct 2022 02:57:26 -0700 (PDT)","MIME-Version":"1.0","References":"<20221027224135.348115-1-nicholas@rothemail.net>\n\t<20221028031726.4849-1-nicholas@rothemail.net>\n\t<20221028031726.4849-4-nicholas@rothemail.net>\n\t<20221028090951.35cilhmbvhbvikhb@uno.localdomain>","In-Reply-To":"<20221028090951.35cilhmbvhbvikhb@uno.localdomain>","Date":"Fri, 28 Oct 2022 10:57:10 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"multipart/alternative; boundary=\"000000000000f999d505ec1548fe\"","Subject":"Re: [libcamera-devel] [PATCH v5 03/10] ipa: add rkisp1 metadata to\n\tfix Android HAL","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","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"nicholas@rothemail.net, Nicholas Roth via libcamera-devel\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]