[{"id":18491,"web_url":"https://patchwork.libcamera.org/comment/18491/","msgid":"<20210802165536.ztxstxvurs4acuk5@uno.localdomain>","date":"2021-08-02T16:55:36","subject":"Re: [libcamera-devel] [RFC PATCH 3/3] ipu3: cio2: Customize sensor\n\tformat selection logic","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Umang,\n\nOn Fri, Jul 30, 2021 at 01:58:28PM +0530, Umang Jain wrote:\n> Adapt the sensor format selection logic such that it address\n> platform constraints on Soraka and Nautilus. This is best-effort\n> basis hence, new platforms can bring new constraints in future\n> and we will need to adapt accordingly.\n>\n> Currently we prioritise sensor format resolutions which closest to the\n> FoV of desired output. However, Intel seems to prioritize the selection\n> based on the closest FoV with respect to sensor's maximum resolution.\n>\n> For e.g. for a desired output of 1080p, Soraka will select 2112x1568\n> since it's a better FoV match to sensor's maximum resolution\n> (4224x3136). It will not match the provided resolution of 2112x1188,\n> even if that matches exactly to the desired ratio of 1080p.\n\nSorry if it might seems picky, but this is not easy and when we'll read it\nin 1 year time (or in 1 month ?) we'll probably be confused, so I'll\nrather spend a bit more time discussing the commit message too.\n\nSo, the culprit here is the ImgU configuration procedure. I don't\nthink we have particular constraints in the hw itself, I'm sure the\nImgU could work with either resolutions, but the parameter calculation\nprocedure is what fail us, and we currently have no better\nalternatives, nor documentation to try to fix it. Hence we have to\nplease the configuration procedure, and the way to do has been reverse\nengineered looking at the sizes selected by the manufacturer and\nrecorded in an xml configuration file part of the Android HAL\nimplementation. That said, I would rework this paragraphs slightly\nas:\n\n\"Rework the sensor frame size selection procedure to take into account\nIPU3 platform specific requirements.\n\nThe correct operations of the ImgU configuration procedure depend on\nthe selected sensor frame size provided as input to the ISP. The\ncurrent implementation, based on a vendor provided configuration\nscript, fails to produce any result if the input frame does not match\nthe expected size. In order to guarantee correctness of the\nconfiguration procedure, the vendor implementation of the Android HAL\nis shipped with a sensor specific configuration file where each\nsupported output resolution combination (main and viewfinder output)\nis associated with the 'correct' sensor frame size to use. Both the\nconfiguration script and configuration file content are tightly\ncoupled and hand-tuned to guarantee correctness of the operations.\n\nAs the libcamera IPU3 pipeline handler does not limit the number of\noutput combinations to the ones supported by the vendor Android HAL,\nit can't make use of any configuration file, but needs anyhow to\nensure that the selected sensor frame size matches what is expected by\nthe configuration procedure.\n\nBased on the reverse-engineering of the configuration file content it\nseems that the ImgU configuration procedure is more reliable when the\ncapture pipeline is configured to maximize scaling and cropping in the\nISP rather than in the sensor. Currently, the IPU3 pipeline is based\non the frame size selection procedure as implemented by\nCameraSensor::getFormat() which prioritizes resolutions with the same\nFOV of the requested output size. This however requires a certain\namount of cropping to happen on the sensor's pixel array to produce\nframes in the same resolution as the desired output size.\n\nModify the frame size selection procedure to prioritize resolutions\nwith the same FOV as the sensor's native one, to avoid cropping in the\nsensor pixel array.\n\nIn example, with the current implementation :\n\n- on a Soraka device equipped with ov13858 as back sensor, with a\n native resolution of 4224x3136 (4:3), when requested to select the\n sensor output size to produce 1080p (16:9) a frame size of 2112x1188\n (16:9) is selected causing the ImgU configuration procedure to fail.\n If a resolution with the same FOV as the sensor's native size, such\n as 2112x1568 (4:3), is selected the pipeline works correctly.\n\n- Umang could you elaborate a similar example for Nautilus here ?\n\n>\n> On the other hand, on Nautilus, currently the sensor's maximum\n> resolution(4208x3118) is the only selection for any 16:9 desired\n> formats, whether it is 640x360 or 1080p or 3840x2160. That means\n> the sensor will run at full bandwidth even for ridiculously smaller\n> resolutions so, we probably don't want that either.\n>\n> This patch how the sensor format resolution is selected:\n> - It prioritizes FoV with respect to sensor's maximum resolution size\n> - It shall never return sensor's maximum resolution provided if there\n> are other lower resolutions available.\n\nI think that's not desirable and actually breaks capture with ov5670\non my Soraka, as it causes a smaller resolution, with a FOV !=\nsensor's one to be selected.\n\nAs you have clarified me offline and\nhave reported in the \"[RFC PATCH] camera_sensor: Do not always\nprioritize aspect-ratios serie\" cover letter, the issue with Nautilus\nis that the imx258 sensor driver supports the following resolutions\n\n - 4208x3118 = 1.349583066\n - 2104x1560 = 1.348717949\n - 1048x780 = 1.343589744\n\nThe FOV ratio diff with the 1080p one will always result smaller for max\nsize, by a very tiny factor, hence max size is always selected.\n\nI think we should rather round the FOV ratios to the first decimal\ndigit and consider all of the above 4:3 resolutions. We can have a\nrounding table that associates each FOV ratio to one the standard ones\nhttps://en.wikipedia.org/wiki/VGA_resolution but that might be an\noverkill.\n\n>\n> /* DNI: Preliminary testing of getSensorFormat():\n> * cam -c1 -swidth=640,height=360,role=raw\n> * cam -c1 -swidth=1280,height=720,role=raw\n> * cam -c1 -swidth=1920,height=1080,role=raw\n> * cam -c1 -swidth=3840,height=2160,role=raw\n> */\n>\n> Signed-off-by: Umang Jain \n> ---\n> Output of preliminary testing:\n> ($) cam -c1 -swidth=640,height=360,role=raw\n> INFO IPU3 cio2.cpp:326 Desired Size: 640x360, Found SGRBG10_IPU3 with Size: 2104x1560\n>\n> ($) cam -c1 -swidth=1280,height=720,role=raw\n> INFO IPU3 cio2.cpp:326 Desired Size: 1280x720, Found SGRBG10_IPU3 with Size: 2104x1560\n>\n> ($) cam -c1 -swidth=1920,height=1080,role=raw\n> INFO IPU3 cio2.cpp:326 Desired Size: 1920x1080, Found SGRBG10_IPU3 with Size: 2104x1560\n>\n> ($) cam -c1 -swidth=3840,height=2160,role=raw\n> INFO IPU3 cio2.cpp:326 Desired Size: 3840x2160, Found SGRBG10_IPU3 with Size: 4208x3118\n\nFor a comparison it would help making sure the selected frame size has\nthe same FOV as the sensor's one.\n\n(also, using FOV as I've done here in this mail is not really\ncorrect, as the FOV is already a ratio of the selected output size and\nthe sensor's native field of view. Probably using 'resolution' would\nbe more correct, but there's a clear shortage of terms and we would\nhave a lot of confusing repetitions :)\n\n> ---\n> src/libcamera/pipeline/ipu3/cio2.cpp | 62 +++++++++++++++++-----------\n> 1 file changed, 38 insertions(+), 24 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n> index aef88afd..4fd57ef7 100644\n> --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> @@ -245,20 +245,16 @@ StreamConfiguration CIO2Device::generateConfiguration(Size size) const\n> *\n> * - The desired \\a size shall fit in the sensor output size to avoid the need\n> * to up-scale.\n> - * - The sensor output size shall match the desired aspect ratio to avoid the\n> - * need to crop the field of view.\n> - * - The sensor output size shall be as small as possible to lower the required\n> - * bandwidth.\n> + * - The sensor output size will be set to maximum resolution only when there\n> + * are no other available lower sensor resolutions from any of \\a mbusCodes.\n> + * - The sensor output size shall have the closest FoV with respect\n> + * to the sensor's maximum resolution.\n> * - The desired \\a size shall be supported by one of the media bus code listed\n> * in \\a mbusCodes.\n> *\n> * When multiple media bus codes can produce the same size, the code at the\n> * lowest position in \\a mbusCodes is selected.\n> *\n> - * The use of this method is optional, as the above criteria may not match the\n> - * needs of all pipeline handlers. Pipeline handlers may implement custom\n> - * sensor format selection when needed.\n> - *\n> * The returned sensor output format is guaranteed to be acceptable by the\n> * setFormat() method without any modification.\n> *\n> @@ -268,14 +264,15 @@ StreamConfiguration CIO2Device::generateConfiguration(Size size) const\n> V4L2SubdeviceFormat CIO2Device::getSensorFormat(const std::vector &mbusCodes,\n> \t\t\t\t\t\tconst Size &size) const\n> {\n> -\tunsigned int desiredArea = size.width * size.height;\n> -\tunsigned int bestArea = UINT_MAX;\n> -\tfloat desiredRatio = static_cast(size.width) / size.height;\n> -\tfloat bestRatio = FLT_MAX;\n> -\tconst Size *bestSize = nullptr;\n> +\tstd::map selectedSizes;\n> +\tSize bestSize;\n> +\tfloat maxResRatio = FLT_MAX;\n> +\tfloat bestRatioDiff = -1.0;\n> \tuint32_t bestCode = 0;\n>\n> \tfor (unsigned int code : mbusCodes) {\n> +\t\tSize maxSensorResolution;\n\nThis is should be an absolute parameter simply computed using\nsensor->resolution() instead of re-initializing it for each format.\n\n> +\n> \t\tconst auto formats = formats_.find(code);\n> \t\tif (formats == formats_.end())\n> \t\t\tcontinue;\n> @@ -286,33 +283,50 @@ V4L2SubdeviceFormat CIO2Device::getSensorFormat(const std::vector\n> \t\t\tif (sz.width < size.width || sz.height < size.height)\n> \t\t\t\tcontinue;\n>\n> +\t\t\tif (sz > maxSensorResolution)\n> +\t\t\t\tmaxSensorResolution = sz;\n> +\n> \t\t\tfloat ratio = static_cast(sz.width) / sz.height;\n> -\t\t\tfloat ratioDiff = fabsf(ratio - desiredRatio);\n> -\t\t\tunsigned int area = sz.width * sz.height;\n> -\t\t\tunsigned int areaDiff = area - desiredArea;\n> +\t\t\tselectedSizes.emplace(sz, ratio);\n> +\t\t}\n>\n> -\t\t\tif (ratioDiff > bestRatio)\n> -\t\t\t\tcontinue;\n> +\t\tif (!selectedSizes.empty()) {\n\nI'm still not sure I got why you need a double pass and you can't\nselect the best one in a single one, as it was done before. I\nunderstand you want to remove the sensor's max size which I don't\nthink is desirable but you could have (if you take my suggestion of\nadding CameraSensor::sizes(mbusCode) in\n\n for (code : sensor->mbusCodes()) {\n for (size: sensor->sizes(code)) {\n /* sizes selection here */\n }\n }\n\nOr have I missed a constraint on Nautilus ?\n\nI actually managed to get rid of all CTS failures on Soraka by simply\n\n--- a/src/libcamera/pipeline/ipu3/cio2.cpp\n+++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n@@ -7,7 +7,8 @@\n\n #include \"cio2.h\"\n\n-#include \n+#include \n+#include \n\n #include \n\n@@ -270,7 +271,9 @@ V4L2SubdeviceFormat CIO2Device::getSensorFormat(const std::vector\n {\n unsigned int desiredArea = size.width * size.height;\n unsigned int bestArea = UINT_MAX;\n- float desiredRatio = static_cast(size.width) / size.height;\n+ const Size &resolution = sensor_->resolution();\n+ float desiredRatio = static_cast(resolution.width) /\n+ resolution.height;\n float bestRatio = FLT_MAX;\n const Size *bestSize = nullptr;\n uint32_t bestCode = 0;\n\nWhich basically just do what we have been discussing about: make the\ndesired FOV match the sensor's one instead of the output size one.\nIt's amazing how many words one can produce to describe 4 lines of\ncode :)\n\nAnyway, if you're willing to try rounding the Nautilus sizes and test\nthe above small patch on top to see how it behaves on nautilus, be\naware you need to revert\n208e70211a6 (\"libcamera: ipu3: Always use sensor full frame size\")\n\nI can prepare a branch with also the update of the FrameDuration\nproperties for a complete CTS run.\n\nThanks\n j\n\n> +\t\t\tmaxResRatio = selectedSizes[maxSensorResolution];\n>\n> -\t\t\tif (ratioDiff < bestRatio || areaDiff < bestArea) {\n> -\t\t\t\tbestRatio = ratioDiff;\n> -\t\t\t\tbestArea = areaDiff;\n> -\t\t\t\tbestSize = &sz;\n> +\t\t\tselectedSizes.erase(maxSensorResolution);\n> +\t\t\tif (selectedSizes.empty()) {\n> \t\t\t\tbestCode = code;\n> +\t\t\t\tbestSize = maxSensorResolution;\n> +\t\t\t} else { /* Find the best FoV to sensor's max resolution */\n> +\t\t\t\tfor (auto iter : selectedSizes) {\n> +\t\t\t\t\tfloat ratioDiff = fabsf(maxResRatio - iter.second);\n> +\n> +\t\t\t\t\tif (bestRatioDiff < 0 || (ratioDiff < bestRatioDiff)) {\n> +\t\t\t\t\t\tbestRatioDiff = ratioDiff;\n> +\t\t\t\t\t\tbestCode = code;\n> +\t\t\t\t\t\tbestSize = iter.first;\n> +\t\t\t\t\t}\n> +\t\t\t\t}\n> \t\t\t}\n> \t\t}\n> +\t\tselectedSizes.clear();\n> \t}\n>\n> -\tif (!bestSize) {\n> +\tif (bestSize.isNull()) {\n> \t\tLOG(IPU3, Debug) << \"No supported format or size found\";\n> \t\treturn {};\n> \t}\n>\n> \tV4L2SubdeviceFormat format{\n> \t\t.mbus_code = bestCode,\n> -\t\t.size = *bestSize,\n> +\t\t.size = bestSize,\n> \t};\n>\n> +\tLOG(IPU3, Info)\n> +\t\t<< \"Desired Size: \" << size.toString()\n> +\t\t<< \", Found \" << mbusCodesToPixelFormat.at(format.mbus_code).toString()\n> +\t\t<< \" with Size: \" << format.size.toString();\n> +\n> \treturn format;\n> }\n>\n> --\n> 2.31.0\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 C82C4C3235\n\tfor ;\n\tMon, 2 Aug 2021 16:54:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3CF60687C3;\n\tMon, 2 Aug 2021 18:54:52 +0200 (CEST)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 48EEF6026F\n\tfor ;\n\tMon, 2 Aug 2021 18:54:50 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay7-d.mail.gandi.net (Postfix) with ESMTPSA id B359520006;\n\tMon, 2 Aug 2021 16:54:49 +0000 (UTC)"],"Date":"Mon, 2 Aug 2021 18:55:36 +0200","From":"Jacopo Mondi ","To":"Umang Jain ","Message-ID":"<20210802165536.ztxstxvurs4acuk5@uno.localdomain>","References":"<20210730082832.152626-1-umang.jain@ideasonboard.com>\n\t<20210730082832.152626-4-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210730082832.152626-4-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH 3/3] ipu3: cio2: Customize sensor\n\tformat selection logic","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]