[{"id":19049,"web_url":"https://patchwork.libcamera.org/comment/19049/","msgid":"<20210825100935.fzqx2g5qag73uci5@uno.localdomain>","date":"2021-08-25T10:09:35","subject":"Re: [libcamera-devel] [PATCH v2 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 01:28:51PM +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> The patch also transforms existing CIO2Device::sizes() in IPU3 pipeline\n> handler to CIO2Device::sizes(PixelFormat) on a similar principle. The\n> function is then plumbed to CameraSensor::sizes(mbusCode) to enumerate\n> the per-format sizes as required in\n> PipelineHandlerIPU3::generateConfiguration().\n>\n> Signed-off-by: Umang Jain \n> Tested-by: Umang Jain \n\nWell, I hope you test your patches before sending them. Do we need\nTested-by tags from the author ?\n\n> Tested-by: Jacopo Mondi \n\nI was sure I had reviewed the series in full I'm sorry,\n\n\n> ---\n> include/libcamera/internal/camera_sensor.h | 2 +-\n> src/libcamera/camera_sensor.cpp | 25 ++++++++++++++++------\n> src/libcamera/pipeline/ipu3/cio2.cpp | 20 ++++++++++++-----\n> src/libcamera/pipeline/ipu3/cio2.h | 2 +-\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-\n> test/camera-sensor.cpp | 2 +-\n> 6 files changed, 38 insertions(+), 15 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 7a386415..87668509 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -472,14 +472,27 @@ int CameraSensor::initProperties()\n> */\n>\n> /**\n> - * \\fn CameraSensor::sizes()\n> - * \\brief Retrieve the frame sizes supported by the camera sensor\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 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> + * \\return The supported frame sizes for \\a mbusCode sorted in increasing order\n> */\n> +const std::vector CameraSensor::sizes(unsigned int mbusCode) const\n> +{\n> +\tstd::vector sizes;\n> +\n> +\tconst auto &format = formats_.find(mbusCode);\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 Retrieve the camera sensor resolution\n> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n> index 1bcd580e..8a40e955 100644\n> --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> @@ -64,16 +64,26 @@ std::vector CIO2Device::formats() const\n> * \\brief Retrieve the list of supported size ranges\n> * \\return The list of supported SizeRange\n> */\n> -std::vector CIO2Device::sizes() const\n> +std::vector CIO2Device::sizes(const PixelFormat &format) const\n\nI know we don't really doxygen, but as it's there, what about\ndocumenting it ?\n\n> {\n> +\tstd::vector szRange;\n\nWhy have you moved it up ? I would also keep the same name\n\n> +\tint mbusCode = -1;\n> +\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> +\tfor (const auto &iter : mbusCodesToPixelFormat) {\n> +\t\tif (iter.second == format)\n> +\t\t\tmbusCode = iter.first;\n\nThere should be only one match, right ? Then\n if (iter.second != format)\n continue;\n\n mbusCode = iter.first;\n break;\n\n> +\t}\n> +\n> +\tif (!mbusCode)\n\n if (-1) evaluates to true\n\nWith this fixed:\nReviewed-by: Jacopo Mondi \n\n> +\t\treturn {};\n> +\n> +\tfor (const Size &sz: sensor_->sizes(mbusCode))\n> +\t\tszRange.emplace_back(sz);\n>\n> -\treturn sizes;\n> +\treturn szRange;\n> }\n>\n> /**\n> diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h\n> index f28e9f1d..24272dc5 100644\n> --- a/src/libcamera/pipeline/ipu3/cio2.h\n> +++ b/src/libcamera/pipeline/ipu3/cio2.h\n> @@ -35,7 +35,7 @@ public:\n> \tCIO2Device();\n>\n> \tstd::vector formats() const;\n> -\tstd::vector sizes() const;\n> +\tstd::vector sizes(const PixelFormat &format) const;\n>\n> \tint init(const MediaDevice *media, unsigned int index);\n> \tint configure(const Size &size, V4L2DeviceFormat *outputFormat);\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 19cb5c4e..c73540fb 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -455,7 +455,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> \t\t\tbufferCount = cio2Config.bufferCount;\n>\n> \t\t\tfor (const PixelFormat &format : data->cio2_.formats())\n> -\t\t\t\tstreamFormats[format] = data->cio2_.sizes();\n> +\t\t\t\tstreamFormats[format] = data->cio2_.sizes(format);\n>\n> \t\t\tbreak;\n> \t\t}\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> \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.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 5D521BD87C\n\tfor ;\n\tWed, 25 Aug 2021 10:08:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A4AAF68893;\n\tWed, 25 Aug 2021 12:08:49 +0200 (CEST)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0E1D260259\n\tfor ;\n\tWed, 25 Aug 2021 12:08:48 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 199B51C0008;\n\tWed, 25 Aug 2021 10:08:46 +0000 (UTC)"],"Date":"Wed, 25 Aug 2021 12:09:35 +0200","From":"Jacopo Mondi ","To":"Umang Jain ","Message-ID":"<20210825100935.fzqx2g5qag73uci5@uno.localdomain>","References":"<20210810075854.86191-1-umang.jain@ideasonboard.com>\n\t<20210810075854.86191-2-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210810075854.86191-2-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 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":19058,"web_url":"https://patchwork.libcamera.org/comment/19058/","msgid":"<20210825130138.muhejygixvsbazuc@uno.localdomain>","date":"2021-08-25T13:01:38","subject":"Re: [libcamera-devel] [PATCH v2 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\n one additional comment\n\nOn Wed, Aug 25, 2021 at 12:09:35PM +0200, Jacopo Mondi wrote:\n> Hi Umang,\n>\n> On Tue, Aug 10, 2021 at 01:28:51PM +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> > The patch also transforms existing CIO2Device::sizes() in IPU3 pipeline\n> > handler to CIO2Device::sizes(PixelFormat) on a similar principle. The\n> > function is then plumbed to CameraSensor::sizes(mbusCode) to enumerate\n> > the per-format sizes as required in\n> > PipelineHandlerIPU3::generateConfiguration().\n> >\n> > Signed-off-by: Umang Jain \n> > Tested-by: Umang Jain \n>\n> Well, I hope you test your patches before sending them. Do we need\n> Tested-by tags from the author ?\n>\n> > Tested-by: Jacopo Mondi \n>\n> I was sure I had reviewed the series in full I'm sorry,\n>\n>\n> > ---\n> > include/libcamera/internal/camera_sensor.h | 2 +-\n> > src/libcamera/camera_sensor.cpp | 25 ++++++++++++++++------\n> > src/libcamera/pipeline/ipu3/cio2.cpp | 20 ++++++++++++-----\n> > src/libcamera/pipeline/ipu3/cio2.h | 2 +-\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-\n> > test/camera-sensor.cpp | 2 +-\n> > 6 files changed, 38 insertions(+), 15 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 7a386415..87668509 100644\n> > --- a/src/libcamera/camera_sensor.cpp\n> > +++ b/src/libcamera/camera_sensor.cpp\n> > @@ -472,14 +472,27 @@ int CameraSensor::initProperties()\n> > */\n> >\n> > /**\n> > - * \\fn CameraSensor::sizes()\n> > - * \\brief Retrieve the frame sizes supported by the camera sensor\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 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> > + * \\return The supported frame sizes for \\a mbusCode sorted in increasing order\n> > */\n> > +const std::vector CameraSensor::sizes(unsigned int mbusCode) const\n> > +{\n> > +\tstd::vector sizes;\n> > +\n> > +\tconst auto &format = formats_.find(mbusCode);\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 Retrieve the camera sensor resolution\n> > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > index 1bcd580e..8a40e955 100644\n> > --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> > @@ -64,16 +64,26 @@ std::vector CIO2Device::formats() const\n> > * \\brief Retrieve the list of supported size ranges\n> > * \\return The list of supported SizeRange\n> > */\n> > -std::vector CIO2Device::sizes() const\n> > +std::vector CIO2Device::sizes(const PixelFormat &format) const\n>\n> I know we don't really doxygen, but as it's there, what about\n> documenting it ?\n>\n> > {\n> > +\tstd::vector szRange;\n>\n> Why have you moved it up ? I would also keep the same name\n>\n> > +\tint mbusCode = -1;\n> > +\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> > +\tfor (const auto &iter : mbusCodesToPixelFormat) {\n> > +\t\tif (iter.second == format)\n> > +\t\t\tmbusCode = iter.first;\n>\n> There should be only one match, right ? Then\n> if (iter.second != format)\n> continue;\n>\n> mbusCode = iter.first;\n> break;\n>\n> > +\t}\n> > +\n> > +\tif (!mbusCode)\n>\n> if (-1) evaluates to true\n>\n> With this fixed:\n> Reviewed-by: Jacopo Mondi \n>\n> > +\t\treturn {};\n> > +\n> > +\tfor (const Size &sz: sensor_->sizes(mbusCode))\n\ncheckstyle reports:\n\n-\tfor (const Size &sz: sensor_->sizes(mbusCode))\n+\tfor (const Size &sz : sensor_->sizes(mbusCode))\n \t\tszRange.emplace_back(sz);\n\n> > +\t\tszRange.emplace_back(sz);\n> >\n> > -\treturn sizes;\n> > +\treturn szRange;\n> > }\n> >\n> > /**\n> > diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h\n> > index f28e9f1d..24272dc5 100644\n> > --- a/src/libcamera/pipeline/ipu3/cio2.h\n> > +++ b/src/libcamera/pipeline/ipu3/cio2.h\n> > @@ -35,7 +35,7 @@ public:\n> > \tCIO2Device();\n> >\n> > \tstd::vector formats() const;\n> > -\tstd::vector sizes() const;\n> > +\tstd::vector sizes(const PixelFormat &format) const;\n> >\n> > \tint init(const MediaDevice *media, unsigned int index);\n> > \tint configure(const Size &size, V4L2DeviceFormat *outputFormat);\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 19cb5c4e..c73540fb 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -455,7 +455,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> > \t\t\tbufferCount = cio2Config.bufferCount;\n> >\n> > \t\t\tfor (const PixelFormat &format : data->cio2_.formats())\n> > -\t\t\t\tstreamFormats[format] = data->cio2_.sizes();\n> > +\t\t\t\tstreamFormats[format] = data->cio2_.sizes(format);\n> >\n> > \t\t\tbreak;\n> > \t\t}\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> > \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.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 6BF83BD87D\n\tfor ;\n\tWed, 25 Aug 2021 13:00:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A849B68893;\n\tWed, 25 Aug 2021 15:00:52 +0200 (CEST)","from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1B46C60259\n\tfor ;\n\tWed, 25 Aug 2021 15:00:51 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 192D7E0015;\n\tWed, 25 Aug 2021 13:00:49 +0000 (UTC)"],"Date":"Wed, 25 Aug 2021 15:01:38 +0200","From":"Jacopo Mondi ","To":"Umang Jain ","Message-ID":"<20210825130138.muhejygixvsbazuc@uno.localdomain>","References":"<20210810075854.86191-1-umang.jain@ideasonboard.com>\n\t<20210810075854.86191-2-umang.jain@ideasonboard.com>\n\t<20210825100935.fzqx2g5qag73uci5@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210825100935.fzqx2g5qag73uci5@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 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":19144,"web_url":"https://patchwork.libcamera.org/comment/19144/","msgid":"","date":"2021-08-27T12:36:05","subject":"Re: [libcamera-devel] [PATCH v2 1/4] libcamera: camera_sensor:\n\tTransform CameraSensor::sizes()","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Umang,\n\nOn 25/08/2021 14:01, Jacopo Mondi wrote:\n> Hi Umang,\n> \n> one additional comment\n> \n> On Wed, Aug 25, 2021 at 12:09:35PM +0200, Jacopo Mondi wrote:\n>> Hi Umang,\n>>\n>> On Tue, Aug 10, 2021 at 01:28:51PM +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>>> The patch also transforms existing CIO2Device::sizes() in IPU3 pipeline\n>>> handler to CIO2Device::sizes(PixelFormat) on a similar principle. The\n>>> function is then plumbed to CameraSensor::sizes(mbusCode) to enumerate\n>>> the per-format sizes as required in\n>>> PipelineHandlerIPU3::generateConfiguration().\n>>>\n>>> Signed-off-by: Umang Jain \n>>> Tested-by: Umang Jain \n>>\n>> Well, I hope you test your patches before sending them. Do we need\n>> Tested-by tags from the author ?\n>>\n>>> Tested-by: Jacopo Mondi \n>>\n>> I was sure I had reviewed the series in full I'm sorry,\n>>\n>>\n>>> ---\n>>> include/libcamera/internal/camera_sensor.h | 2 +-\n>>> src/libcamera/camera_sensor.cpp | 25 ++++++++++++++++------\n>>> src/libcamera/pipeline/ipu3/cio2.cpp | 20 ++++++++++++-----\n>>> src/libcamera/pipeline/ipu3/cio2.h | 2 +-\n>>> src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +-\n>>> test/camera-sensor.cpp | 2 +-\n>>> 6 files changed, 38 insertions(+), 15 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 7a386415..87668509 100644\n>>> --- a/src/libcamera/camera_sensor.cpp\n>>> +++ b/src/libcamera/camera_sensor.cpp\n>>> @@ -472,14 +472,27 @@ int CameraSensor::initProperties()\n>>> */\n>>>\n>>> /**\n>>> - * \\fn CameraSensor::sizes()\n>>> - * \\brief Retrieve the frame sizes supported by the camera sensor\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 span all media bus codes supported by the camera sensor.\n>>> - * Not all sizes may be supported by all media bus codes.\n>>> - *\n\nThe fact that this statement is removed makes this patch sound like a\ngood idea!\n\n>>> - * \\return The supported frame sizes sorted in increasing order\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>>> +{\n>>> +\tstd::vector sizes;\n>>> +\n>>> +\tconst auto &format = formats_.find(mbusCode);\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 Retrieve the camera sensor resolution\n>>> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n>>> index 1bcd580e..8a40e955 100644\n>>> --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n>>> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n>>> @@ -64,16 +64,26 @@ std::vector CIO2Device::formats() const\n>>> * \\brief Retrieve the list of supported size ranges\n>>> * \\return The list of supported SizeRange\n>>> */\n>>> -std::vector CIO2Device::sizes() const\n>>> +std::vector CIO2Device::sizes(const PixelFormat &format) const\n>>\n>> I know we don't really doxygen, but as it's there, what about\n>> documenting it ?\n\nAgreed, we should keep that doc consistent.\n\n>>\n>>> {\n>>> +\tstd::vector szRange;\n>>\n>> Why have you moved it up ? I would also keep the same name\n>>\n>>> +\tint mbusCode = -1;\n>>> +\n\nis 0 a valid mbusCode?\n\nIf not, we could use that and keep mbusCode as unsigned.\n\n\nhttps://www.kernel.org/doc/html/latest/userspace-api/media/v4l/subdev-formats.html#v4l2-mbus-pixelcode\nshows a table of valid mbus codes, and I don't think its likely that 0x0\nwould ever be added there.\n\n\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>>> +\tfor (const auto &iter : mbusCodesToPixelFormat) {\n>>> +\t\tif (iter.second == format)\n>>> +\t\t\tmbusCode = iter.first;\n>>\n>> There should be only one match, right ? Then\n>> if (iter.second != format)\n>> continue;\n>>\n>> mbusCode = iter.first;\n>> break;\n>>\n>>> +\t}\n>>> +\n>>> +\tif (!mbusCode)\n>>\n>> if (-1) evaluates to true\n\nThen this wouldn't be a problem...\n\n\n>>\n>> With this fixed:\n\nlikewise\n\nReviewed-by: Kieran Bingham \n\n>> Reviewed-by: Jacopo Mondi \n>>\n>>> +\t\treturn {};\n>>> +\n>>> +\tfor (const Size &sz: sensor_->sizes(mbusCode))\n> \n> checkstyle reports:\n> \n> -\tfor (const Size &sz: sensor_->sizes(mbusCode))\n> +\tfor (const Size &sz : sensor_->sizes(mbusCode))\n> \t\tszRange.emplace_back(sz);\n> \n>>> +\t\tszRange.emplace_back(sz);\n>>>\n>>> -\treturn sizes;\n>>> +\treturn szRange;\n>>> }\n>>>\n>>> /**\n>>> diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h\n>>> index f28e9f1d..24272dc5 100644\n>>> --- a/src/libcamera/pipeline/ipu3/cio2.h\n>>> +++ b/src/libcamera/pipeline/ipu3/cio2.h\n>>> @@ -35,7 +35,7 @@ public:\n>>> \tCIO2Device();\n>>>\n>>> \tstd::vector formats() const;\n>>> -\tstd::vector sizes() const;\n>>> +\tstd::vector sizes(const PixelFormat &format) const;\n>>>\n>>> \tint init(const MediaDevice *media, unsigned int index);\n>>> \tint configure(const Size &size, V4L2DeviceFormat *outputFormat);\n>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>> index 19cb5c4e..c73540fb 100644\n>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n>>> @@ -455,7 +455,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n>>> \t\t\tbufferCount = cio2Config.bufferCount;\n>>>\n>>> \t\t\tfor (const PixelFormat &format : data->cio2_.formats())\n>>> -\t\t\t\tstreamFormats[format] = data->cio2_.sizes();\n>>> +\t\t\t\tstreamFormats[format] = data->cio2_.sizes(format);\n>>>\n>>> \t\t\tbreak;\n>>> \t\t}\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>>> \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.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 8AD84BD87D\n\tfor ;\n\tFri, 27 Aug 2021 12:36:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F2C356891F;\n\tFri, 27 Aug 2021 14:36:09 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 502E660256\n\tfor ;\n\tFri, 27 Aug 2021 14:36:08 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B3AB15A1;\n\tFri, 27 Aug 2021 14:36:07 +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=\"PVoSGlVi\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630067767;\n\tbh=2elYG9GiiliMKXDx65oXLwtuPL24i6mcBCLUSnBeRFY=;\n\th=To:Cc:References:From:Subject:Date:In-Reply-To:From;\n\tb=PVoSGlVicEb0VCWoluJ0CXv+yem6Z8yIHKOinZDTEdaCCHfF12yidSFtCUwgzgqvu\n\twUmIe7FCT/aSpzljMKeTUlci3SmX9i2zcaleCeEpVD5PtxqBQBYAY/s34lefz+0OgY\n\tAdR6IuDKQu4DeddxTWui1qfNw0n/e2Atrg0dchws=","To":"Jacopo Mondi ,\n\tUmang Jain ","References":"<20210810075854.86191-1-umang.jain@ideasonboard.com>\n\t<20210810075854.86191-2-umang.jain@ideasonboard.com>\n\t<20210825100935.fzqx2g5qag73uci5@uno.localdomain>\n\t<20210825130138.muhejygixvsbazuc@uno.localdomain>","From":"Kieran Bingham ","Message-ID":"","Date":"Fri, 27 Aug 2021 13:36:05 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210825130138.muhejygixvsbazuc@uno.localdomain>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v2 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\" "}}]