[{"id":18629,"web_url":"https://patchwork.libcamera.org/comment/18629/","msgid":"<20210809154942.xkhgj4p2tqttz7oh@uno.localdomain>","date":"2021-08-09T15:49:42","subject":"Re: [libcamera-devel] [PATCH 1/4] libcamera: camera_sensor:\n\tTransform CameraSensor::sizes()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Umang,\n\nOn Tue, Aug 03, 2021 at 07:02:02PM +0530, Umang Jain wrote:\n> In CameraSensor, the mbusCodes() and sizes() accessor functions\n> retrieves all the supported media bus codes and the supported sizes\n> respectively. However, this is quite limiting since the caller\n> probably isn't in a position to match which range of sizes are\n> supported for a particular mbusCode.\n>\n> Hence, the caller is most likely interested to know about the sizes\n> supported for a particular media bus code. This patch transforms the\n> existing CameraSensor::sizes() to CameraSensor::sizes(mbuscode) to\n> achieve that goal.\n>\n> To know all the frame sizes of the CameraSensor as required in IPU3\n> case Cio2Device::sizes(), one would now require to enumerate all the\n> media bus codes (can be retrieved by CameraSensor::mbusCodes()) with\n> CameraSensor::size(mbusCode). The result can be inserted in a\n> std::set<> to avoid duplicates.\n>\n> Signed-off-by: Umang Jain \n> ---\n> include/libcamera/internal/camera_sensor.h | 2 +-\n> src/libcamera/camera_sensor.cpp | 36 ++++++++++++++++------\n> src/libcamera/pipeline/ipu3/cio2.cpp | 10 +++---\n> test/camera-sensor.cpp | 2 +-\n> 4 files changed, 34 insertions(+), 16 deletions(-)\n>\n> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h\n> index db12b07e..d25a1165 100644\n> --- a/include/libcamera/internal/camera_sensor.h\n> +++ b/include/libcamera/internal/camera_sensor.h\n> @@ -38,7 +38,7 @@ public:\n> \tconst std::string &id() const { return id_; }\n> \tconst MediaEntity *entity() const { return entity_; }\n> \tconst std::vector &mbusCodes() const { return mbusCodes_; }\n> -\tconst std::vector &sizes() const { return sizes_; }\n> +\tconst std::vector sizes(unsigned int mbusCode) const;\n> \tSize resolution() const;\n> \tconst std::vector &testPatternModes() const\n> \t{\n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index cde431cc..3c3ceff3 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -471,16 +471,6 @@ int CameraSensor::initProperties()\n> * \\return The supported media bus codes sorted in increasing order\n> */\n>\n> -/**\n> - * \\fn CameraSensor::sizes()\n> - * \\brief Retrieve the frame sizes supported by the camera sensor\n> - *\n> - * The reported sizes span all media bus codes supported by the camera sensor.\n> - * Not all sizes may be supported by all media bus codes.\n> - *\n> - * \\return The supported frame sizes sorted in increasing order\n> - */\n> -\n> /**\n> * \\brief Retrieve the camera sensor resolution\n> *\n> @@ -594,6 +584,32 @@ V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector &mbu\n> \treturn format;\n> }\n>\n> +/**\n> + * \\brief Retrieve the supported frame sizes for a media bus code\n> + * \\param[in] mbusCode The media bus code for which sizes are requested\n> + *\n> + * The reported sizes for a particular \\a mbusCode are supported by the camera\n> + * sensor.\n\nI would drop this\n\n> + *\n> + * \\return The supported frame sizes for \\a mbusCode sorted in increasing order\n> + */\n> +const std::vector CameraSensor::sizes(unsigned int mbusCode) const\n\nWhy have you moved this ? Let's respect the declaration order\n\n> +{\n> +\tstd::vector sizes;\n> +\n> +\tconst auto format = formats_.find(mbusCode);\n\n&format\n\n> +\tif (format == formats_.end())\n> +\t\treturn sizes;\n> +\n> +\tconst std::vector &ranges = format->second;\n> +\tstd::transform(ranges.begin(), ranges.end(), std::back_inserter(sizes),\n> +\t\t [](const SizeRange &range) { return range.max; });\n> +\n> +\tstd::sort(sizes.begin(), sizes.end());\n> +\n> +\treturn sizes;\n> +}\n> +\n> /**\n> * \\brief Set the sensor output format\n> * \\param[in] format The desired sensor output format\n> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n> index 1bcd580e..6f2ef321 100644\n> --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> @@ -69,11 +69,13 @@ std::vector CIO2Device::sizes() const\n> \tif (!sensor_)\n> \t\treturn {};\n>\n> -\tstd::vector sizes;\n> -\tfor (const Size &size : sensor_->sizes())\n> -\t\tsizes.emplace_back(size, size);\n> +\tstd::set allSizes;\n> +\tfor (unsigned int code : sensor_->mbusCodes()) {\n> +\t\tfor (Size sz : sensor_->sizes(code))\n\nconst Size &sz\n\n> +\t\t\tallSizes.insert(sz);\n> +\t}\n>\n> -\treturn sizes;\n> +\treturn std::vector(allSizes.begin(), allSizes.end());\n\nLooking at how CIO2::sizes() is used in IPU3::generateConfiguration()\nI wonder if we shouldn't pass the desired PixelFormat to\nCIO2::sizes() so that we can actually enumerate the per-format sizes\nfor real (and avoid going through the set->vector conversion).\n\n> }\n>\n> /**\n> diff --git a/test/camera-sensor.cpp b/test/camera-sensor.cpp\n> index a8dcad82..372ee4af 100644\n> --- a/test/camera-sensor.cpp\n> +++ b/test/camera-sensor.cpp\n> @@ -76,7 +76,7 @@ protected:\n> \t\t\treturn TestFail;\n> \t\t}\n>\n> -\t\tconst std::vector &sizes = sensor_->sizes();\n> +\t\tconst std::vector &sizes = sensor_->sizes(*iter);\n\nThis changes the semantic of the test from \"let's check if a format\nsupports 4096x2160\" to \"let's check if ARGB8888_1X32 supports\n4096x2160\". I think it's better now, and as long as the test passes,\nsince it uses VIMC, we should be good!\n\nThanks\n j\n\n> \t\tauto iter2 = std::find(sizes.begin(), sizes.end(),\n> \t\t\t\t Size(4096, 2160));\n> \t\tif (iter2 == sizes.end()) {\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 061D4BD87D\n\tfor ;\n\tMon, 9 Aug 2021 15:48:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 525A06884F;\n\tMon, 9 Aug 2021 17:48:56 +0200 (CEST)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 80E8460269\n\tfor ;\n\tMon, 9 Aug 2021 17:48:54 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id EFC0BFF809;\n\tMon, 9 Aug 2021 15:48:53 +0000 (UTC)"],"Date":"Mon, 9 Aug 2021 17:49:42 +0200","From":"Jacopo Mondi ","To":"Umang Jain ","Message-ID":"<20210809154942.xkhgj4p2tqttz7oh@uno.localdomain>","References":"<20210803133205.6599-1-umang.jain@ideasonboard.com>\n\t<20210803133205.6599-2-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210803133205.6599-2-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 1/4] libcamera: camera_sensor:\n\tTransform CameraSensor::sizes()","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\" "}},{"id":18656,"web_url":"https://patchwork.libcamera.org/comment/18656/","msgid":"<954f95c5-0d29-0ade-a4bc-4870a7cd88dd@ideasonboard.com>","date":"2021-08-10T05:43:35","subject":"Re: [libcamera-devel] [PATCH 1/4] libcamera: camera_sensor:\n\tTransform CameraSensor::sizes()","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo\n\nOn 8/9/21 9:19 PM, Jacopo Mondi wrote:\n> Hi Umang,\n>\n> On Tue, Aug 03, 2021 at 07:02:02PM +0530, Umang Jain wrote:\n>> In CameraSensor, the mbusCodes() and sizes() accessor functions\n>> retrieves all the supported media bus codes and the supported sizes\n>> respectively. However, this is quite limiting since the caller\n>> probably isn't in a position to match which range of sizes are\n>> supported for a particular mbusCode.\n>>\n>> Hence, the caller is most likely interested to know about the sizes\n>> supported for a particular media bus code. This patch transforms the\n>> existing CameraSensor::sizes() to CameraSensor::sizes(mbuscode) to\n>> achieve that goal.\n>>\n>> To know all the frame sizes of the CameraSensor as required in IPU3\n>> case Cio2Device::sizes(), one would now require to enumerate all the\n>> media bus codes (can be retrieved by CameraSensor::mbusCodes()) with\n>> CameraSensor::size(mbusCode). The result can be inserted in a\n>> std::set<> to avoid duplicates.\n>>\n>> Signed-off-by: Umang Jain \n>> ---\n>> include/libcamera/internal/camera_sensor.h | 2 +-\n>> src/libcamera/camera_sensor.cpp | 36 ++++++++++++++++------\n>> src/libcamera/pipeline/ipu3/cio2.cpp | 10 +++---\n>> test/camera-sensor.cpp | 2 +-\n>> 4 files changed, 34 insertions(+), 16 deletions(-)\n>>\n\n\n>> +\t\t\tallSizes.insert(sz);\n>> +\t}\n>>\n>> -\treturn sizes;\n>> +\treturn std::vector(allSizes.begin(), allSizes.end());\n> Looking at how CIO2::sizes() is used in IPU3::generateConfiguration()\n> I wonder if we shouldn't pass the desired PixelFormat to\n\nI think you meant s/shouldn't/should here? So essentially do you want\n\n std::vector CIO2Device::sizes(const PixelFormat &format)\n const\n\n ?\n\n(which looks okay to me)\n\n> CIO2::sizes() so that we can actually enumerate the per-format sizes\n> for real (and avoid going through the set->vector conversion).\n>\n>> }\n>>\n>> /**\n>> diff --git a/test/camera-sensor.cpp b/test/camera-sensor.cpp\n>> index a8dcad82..372ee4af 100644\n>> --- a/test/camera-sensor.cpp\n>> +++ b/test/camera-sensor.cpp\n>> @@ -76,7 +76,7 @@ protected:\n>> \t\t\treturn TestFail;\n>> \t\t}\n>>\n>> -\t\tconst std::vector &sizes = sensor_->sizes();\n>> +\t\tconst std::vector &sizes = sensor_->sizes(*iter);\n> This changes the semantic of the test from \"let's check if a format\n> supports 4096x2160\" to \"let's check if ARGB8888_1X32 supports\n> 4096x2160\". I think it's better now, and as long as the test passes,\n> since it uses VIMC, we should be good!\nI'll check!\n>\n> Thanks\n> j\n>\n>> \t\tauto iter2 = std::find(sizes.begin(), sizes.end(),\n>> \t\t\t\t Size(4096, 2160));\n>> \t\tif (iter2 == sizes.end()) {\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 DFA5BC3240\n\tfor ;\n\tTue, 10 Aug 2021 05:43:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7986B687FA;\n\tTue, 10 Aug 2021 07:43:42 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0F9CF60262\n\tfor ;\n\tTue, 10 Aug 2021 07:43:41 +0200 (CEST)","from [192.168.1.104] (unknown [103.238.109.8])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 23A593F0;\n\tTue, 10 Aug 2021 07:43:39 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"aK9fU1Ka\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628574220;\n\tbh=4aeZ1TlDsyhrQgl/l7zSCRQYo3EU7OiUJXktkglPqrg=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=aK9fU1KaS3IeGoL0yGw1FL80I3fdIGPSbFrTGeM8QcWcgFgnW26bpFy7j22bvF6rH\n\tjjrsBbZ/h2cEq+EFNTgJ+gF+TDzdYRJJ1u5RGR8MiB52qx9CedL2X/Id2bIWLKo7EX\n\tFJBvDo6D71iKVIAaBkPlQb4zE+KfTo1VoAk8o6rY=","To":"Jacopo Mondi ","References":"<20210803133205.6599-1-umang.jain@ideasonboard.com>\n\t<20210803133205.6599-2-umang.jain@ideasonboard.com>\n\t<20210809154942.xkhgj4p2tqttz7oh@uno.localdomain>","From":"Umang Jain ","Message-ID":"<954f95c5-0d29-0ade-a4bc-4870a7cd88dd@ideasonboard.com>","Date":"Tue, 10 Aug 2021 11:13:35 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20210809154942.xkhgj4p2tqttz7oh@uno.localdomain>","Content-Type":"multipart/alternative;\n\tboundary=\"------------19C87756972028E14FDCF5FF\"","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH 1/4] libcamera: camera_sensor:\n\tTransform CameraSensor::sizes()","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\" "}},{"id":18663,"web_url":"https://patchwork.libcamera.org/comment/18663/","msgid":"<20210810075813.76dyhey5qi53uvok@uno.localdomain>","date":"2021-08-10T07:58:13","subject":"Re: [libcamera-devel] [PATCH 1/4] libcamera: camera_sensor:\n\tTransform CameraSensor::sizes()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Umang,\n\nOn Tue, Aug 10, 2021 at 11:13:35AM +0530, Umang Jain wrote:\n> Hi Jacopo\n>\n> On 8/9/21 9:19 PM, Jacopo Mondi wrote:\n> > Hi Umang,\n> >\n> > On Tue, Aug 03, 2021 at 07:02:02PM +0530, Umang Jain wrote:\n> > > In CameraSensor, the mbusCodes() and sizes() accessor functions\n> > > retrieves all the supported media bus codes and the supported sizes\n> > > respectively. However, this is quite limiting since the caller\n> > > probably isn't in a position to match which range of sizes are\n> > > supported for a particular mbusCode.\n> > >\n> > > Hence, the caller is most likely interested to know about the sizes\n> > > supported for a particular media bus code. This patch transforms the\n> > > existing CameraSensor::sizes() to CameraSensor::sizes(mbuscode) to\n> > > achieve that goal.\n> > >\n> > > To know all the frame sizes of the CameraSensor as required in IPU3\n> > > case Cio2Device::sizes(), one would now require to enumerate all the\n> > > media bus codes (can be retrieved by CameraSensor::mbusCodes()) with\n> > > CameraSensor::size(mbusCode). The result can be inserted in a\n> > > std::set<> to avoid duplicates.\n> > >\n> > > Signed-off-by: Umang Jain \n> > > ---\n> > > include/libcamera/internal/camera_sensor.h | 2 +-\n> > > src/libcamera/camera_sensor.cpp | 36 ++++++++++++++++------\n> > > src/libcamera/pipeline/ipu3/cio2.cpp | 10 +++---\n> > > test/camera-sensor.cpp | 2 +-\n> > > 4 files changed, 34 insertions(+), 16 deletions(-)\n> > >\n> \n>\n> > > +\t\t\tallSizes.insert(sz);\n> > > +\t}\n> > >\n> > > -\treturn sizes;\n> > > +\treturn std::vector(allSizes.begin(), allSizes.end());\n> > Looking at how CIO2::sizes() is used in IPU3::generateConfiguration()\n> > I wonder if we shouldn't pass the desired PixelFormat to\n>\n> I think you meant s/shouldn't/should here? So essentially do you want\n>\n> std::vector CIO2Device::sizes(const PixelFormat &format)\n> const\n>\n> ?\n>\n> (which looks okay to me)\n\nLooking at how it is used to generate the raw stream sizes I think it\nwould be better\n\n>\n> > CIO2::sizes() so that we can actually enumerate the per-format sizes\n> > for real (and avoid going through the set->vector conversion).\n> >\n> > > }\n> > >\n> > > /**\n> > > diff --git a/test/camera-sensor.cpp b/test/camera-sensor.cpp\n> > > index a8dcad82..372ee4af 100644\n> > > --- a/test/camera-sensor.cpp\n> > > +++ b/test/camera-sensor.cpp\n> > > @@ -76,7 +76,7 @@ protected:\n> > > \t\t\treturn TestFail;\n> > > \t\t}\n> > >\n> > > -\t\tconst std::vector &sizes = sensor_->sizes();\n> > > +\t\tconst std::vector &sizes = sensor_->sizes(*iter);\n> > This changes the semantic of the test from \"let's check if a format\n> > supports 4096x2160\" to \"let's check if ARGB8888_1X32 supports\n> > 4096x2160\". I think it's better now, and as long as the test passes,\n> > since it uses VIMC, we should be good!\n> I'll check!\n\nYeah please make sure tests still pass in full. This one shouldn't be\na problem...\n\nThanks\n j\n\n> >\n> > Thanks\n> > j\n> >\n> > > \t\tauto iter2 = std::find(sizes.begin(), sizes.end(),\n> > > \t\t\t\t Size(4096, 2160));\n> > > \t\tif (iter2 == sizes.end()) {\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 288A5BD87D\n\tfor ;\n\tTue, 10 Aug 2021 07:57:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 04495687FA;\n\tTue, 10 Aug 2021 09:57:27 +0200 (CEST)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CB2EB687DE\n\tfor ;\n\tTue, 10 Aug 2021 09:57:25 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 64468FF803;\n\tTue, 10 Aug 2021 07:57:25 +0000 (UTC)"],"Date":"Tue, 10 Aug 2021 09:58:13 +0200","From":"Jacopo Mondi ","To":"Umang Jain ","Message-ID":"<20210810075813.76dyhey5qi53uvok@uno.localdomain>","References":"<20210803133205.6599-1-umang.jain@ideasonboard.com>\n\t<20210803133205.6599-2-umang.jain@ideasonboard.com>\n\t<20210809154942.xkhgj4p2tqttz7oh@uno.localdomain>\n\t<954f95c5-0d29-0ade-a4bc-4870a7cd88dd@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<954f95c5-0d29-0ade-a4bc-4870a7cd88dd@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 1/4] libcamera: camera_sensor:\n\tTransform CameraSensor::sizes()","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\" "}}]