[{"id":31166,"web_url":"https://patchwork.libcamera.org/comment/31166/","msgid":"<172605195661.1037309.3189482518520309086@ping.linuxembedded.co.uk>","date":"2024-09-11T10:52:36","subject":"Re: [PATCH 2/2] pipeline: rkisp1: Filter out sensor sizes not\n\tsupported by the pipeline","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Umang Jain (2024-09-09 17:37:19)\n> It is possible that the maximum sensor size (returned by\n> CameraSensor::resolution()) is not supported by the pipeline. In such\n> cases, a filter function is required to filter out resolutions of the\n> camera sensor, which cannot be supported by the pipeline.\n> \n> Introduce the filter function filterSensorResolution() in RkISP1Path\n> class and use it for validate() and generateConfiguration(). The\n> maximum sensor resolution is picked from that filtered set of\n> resolutions instead of CameraSensor::resolution().\n> \n> Signed-off-by: Umang Jain \n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 48 ++++++++++++++++++-\n> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 3 ++\n> 2 files changed, 49 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> index 9053af18..b3b27aa5 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> @@ -128,12 +128,56 @@ void RkISP1Path::populateFormats()\n> }\n> }\n> \n> +/**\n> + * \\brief Filter the sensor resolutions that can be supported\n> + * \\param[in] sensor The camera sensor\n> + *\n> + * This function retrieves all the sizes supported by the sensor and\n> + * filters all the resolutions that can be supported on the pipeline.\n> + * It is possible that the sensor's maximum output resolution is higher\n> + * than the ISP maximum input. In that case, this function filters out all\n> + * the resolution incapable of being supported and returns the maximum\n> + * sensor resolution that can be supported by the pipeline.\n> + *\n> + * \\return Maximum sensor size supported on the pipeline\n> + */\n> +Size RkISP1Path::filterSensorResolution(const CameraSensor *sensor)\n> +{\n> + if (!sensorSizes_.empty())\n> + return sensorSizes_.back();\n\nI like that we keep the full list of supported sensorSizes_. My first\nthought was that this list should be generated when the path is created,\nbut this also seems fine to me.\n\nI think there's no need to complicate / split this until there's\nactually a user of the full sensorSizes list ?\n\nI'm a little surprised that's not not used actually. Do we report the\nraw modes we support somewhere? Should that iterate the 'sensorSizes_'\nas the supported sizes ?\n\nPerhaps that's an improvement/fix on top though.\n\nReviewed-by: Kieran Bingham \n\n> +\n> + std::vector sensorSizes;\n> + const std::vector &mbusCodes = sensor->mbusCodes();\n> + for (const auto iter : mbusCodes) {\n> + std::vector sizes = sensor->sizes(iter);\n> + for (Size sz : sizes)\n> + sensorSizes.push_back(sz);\n> + }\n> +\n> + std::sort(sensorSizes.begin(), sensorSizes.end());\n> +\n> + /* Remove duplicates. */\n> + auto last = std::unique(sensorSizes.begin(), sensorSizes.end());\n> + sensorSizes.erase(last, sensorSizes.end());\n> +\n> + /* Discard any sizes that the pipeline is unable to support. */\n> + for (auto sz : sensorSizes) {\n> + if (sz.width > maxResolution_.width ||\n> + sz.height > maxResolution_.height)\n> + continue;\n> +\n> + sensorSizes_.push_back(sz);\n> + }\n> +\n> + return sensorSizes_.back();\n> +}\n> +\n> StreamConfiguration\n> RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,\n> StreamRole role)\n> {\n> const std::vector &mbusCodes = sensor->mbusCodes();\n> - const Size &resolution = sensor->resolution();\n> + Size resolution = filterSensorResolution(sensor);\n> \n> /* Min and max resolutions to populate the available stream formats. */\n> Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)\n> @@ -222,7 +266,7 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n> StreamConfiguration *cfg)\n> {\n> const std::vector &mbusCodes = sensor->mbusCodes();\n> - const Size &resolution = sensor->resolution();\n> + Size resolution = filterSensorResolution(sensor);\n> \n> const StreamConfiguration reqCfg = *cfg;\n> CameraConfiguration::Status status = CameraConfiguration::Valid;\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> index 13ba4b62..03ff9543 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> @@ -63,6 +63,7 @@ public:\n> \n> private:\n> void populateFormats();\n> + Size filterSensorResolution(const CameraSensor *sensor);\n> \n> static constexpr unsigned int RKISP1_BUFFER_COUNT = 4;\n> \n> @@ -77,6 +78,8 @@ private:\n> std::unique_ptr resizer_;\n> std::unique_ptr video_;\n> MediaLink *link_;\n> +\n> + std::vector sensorSizes_;\n> };\n> \n> class RkISP1MainPath : public RkISP1Path\n> -- \n> 2.45.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 2A90EBF415\n\tfor ;\n\tWed, 11 Sep 2024 10:52:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D3E20618F2;\n\tWed, 11 Sep 2024 12:52:43 +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 ACA81618F2\n\tfor ;\n\tWed, 11 Sep 2024 12:52:41 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6304F5B3;\n\tWed, 11 Sep 2024 12:51:22 +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=\"slbokdbV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1726051882;\n\tbh=kDeO/xVwgw5HKH8LWnlcZwsf24CJCSMVfDrkqZDlowI=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=slbokdbVKQ/jSQC6BsoNeHHYye3rZU0sCr99RrVnbNxUZKOYt2mFjer0dNYfo/0ax\n\tu3r7fUaMYfdSVWKTmDUKWvOTjh5Bt+ipP5eiUFfjfv5YyHsQiikNOUeTLV0PV1psG9\n\tREe2OBYTcs5R3VKMdP2UfDWMhHcSNRBMgLcTS/ww=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240909163719.267211-3-umang.jain@ideasonboard.com>","References":"<20240909163719.267211-1-umang.jain@ideasonboard.com>\n\t<20240909163719.267211-3-umang.jain@ideasonboard.com>","Subject":"Re: [PATCH 2/2] pipeline: rkisp1: Filter out sensor sizes not\n\tsupported by the pipeline","From":"Kieran Bingham ","Cc":"Umang Jain ","To":"Umang Jain ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 11 Sep 2024 11:52:36 +0100","Message-ID":"<172605195661.1037309.3189482518520309086@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31181,"web_url":"https://patchwork.libcamera.org/comment/31181/","msgid":"<20240911222908.GN4470@pendragon.ideasonboard.com>","date":"2024-09-11T22:29:08","subject":"Re: [PATCH 2/2] pipeline: rkisp1: Filter out sensor sizes not\n\tsupported by the pipeline","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Sep 11, 2024 at 11:52:36AM +0100, Kieran Bingham wrote:\n> Quoting Umang Jain (2024-09-09 17:37:19)\n> > It is possible that the maximum sensor size (returned by\n> > CameraSensor::resolution()) is not supported by the pipeline. In such\n> > cases, a filter function is required to filter out resolutions of the\n> > camera sensor, which cannot be supported by the pipeline.\n> > \n> > Introduce the filter function filterSensorResolution() in RkISP1Path\n> > class and use it for validate() and generateConfiguration(). The\n> > maximum sensor resolution is picked from that filtered set of\n> > resolutions instead of CameraSensor::resolution().\n> > \n> > Signed-off-by: Umang Jain \n> > ---\n> > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 48 ++++++++++++++++++-\n> > src/libcamera/pipeline/rkisp1/rkisp1_path.h | 3 ++\n> > 2 files changed, 49 insertions(+), 2 deletions(-)\n> > \n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > index 9053af18..b3b27aa5 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > @@ -128,12 +128,56 @@ void RkISP1Path::populateFormats()\n> > }\n> > }\n> > \n> > +/**\n> > + * \\brief Filter the sensor resolutions that can be supported\n> > + * \\param[in] sensor The camera sensor\n> > + *\n> > + * This function retrieves all the sizes supported by the sensor and\n> > + * filters all the resolutions that can be supported on the pipeline.\n> > + * It is possible that the sensor's maximum output resolution is higher\n> > + * than the ISP maximum input. In that case, this function filters out all\n> > + * the resolution incapable of being supported and returns the maximum\n> > + * sensor resolution that can be supported by the pipeline.\n> > + *\n> > + * \\return Maximum sensor size supported on the pipeline\n> > + */\n> > +Size RkISP1Path::filterSensorResolution(const CameraSensor *sensor)\n> > +{\n> > + if (!sensorSizes_.empty())\n> > + return sensorSizes_.back();\n\nThere are systems based on rkisp1 where multiple sensors are connected\nto the same ISP. Obviously only one of them can be used at a time, but\napplications can switch between them. This will be broken by caching\nsensorSizes_.\n\n> I like that we keep the full list of supported sensorSizes_. My first\n> thought was that this list should be generated when the path is created,\n> but this also seems fine to me.\n> \n> I think there's no need to complicate / split this until there's\n> actually a user of the full sensorSizes list ?\n> \n> I'm a little surprised that's not not used actually. Do we report the\n> raw modes we support somewhere? Should that iterate the 'sensorSizes_'\n> as the supported sizes ?\n> \n> Perhaps that's an improvement/fix on top though.\n> \n> Reviewed-by: Kieran Bingham \n> \n> > +\n> > + std::vector sensorSizes;\n> > + const std::vector &mbusCodes = sensor->mbusCodes();\n> > + for (const auto iter : mbusCodes) {\n> > + std::vector sizes = sensor->sizes(iter);\n> > + for (Size sz : sizes)\n> > + sensorSizes.push_back(sz);\n> > + }\n> > +\n> > + std::sort(sensorSizes.begin(), sensorSizes.end());\n> > +\n> > + /* Remove duplicates. */\n> > + auto last = std::unique(sensorSizes.begin(), sensorSizes.end());\n> > + sensorSizes.erase(last, sensorSizes.end());\n> > +\n> > + /* Discard any sizes that the pipeline is unable to support. */\n> > + for (auto sz : sensorSizes) {\n> > + if (sz.width > maxResolution_.width ||\n> > + sz.height > maxResolution_.height)\n> > + continue;\n> > +\n> > + sensorSizes_.push_back(sz);\n> > + }\n> > +\n> > + return sensorSizes_.back();\n> > +}\n> > +\n> > StreamConfiguration\n> > RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,\n> > StreamRole role)\n> > {\n> > const std::vector &mbusCodes = sensor->mbusCodes();\n> > - const Size &resolution = sensor->resolution();\n> > + Size resolution = filterSensorResolution(sensor);\n> > \n> > /* Min and max resolutions to populate the available stream formats. */\n> > Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)\n> > @@ -222,7 +266,7 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n> > StreamConfiguration *cfg)\n> > {\n> > const std::vector &mbusCodes = sensor->mbusCodes();\n> > - const Size &resolution = sensor->resolution();\n> > + Size resolution = filterSensorResolution(sensor);\n> > \n> > const StreamConfiguration reqCfg = *cfg;\n> > CameraConfiguration::Status status = CameraConfiguration::Valid;\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> > index 13ba4b62..03ff9543 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> > @@ -63,6 +63,7 @@ public:\n> > \n> > private:\n> > void populateFormats();\n> > + Size filterSensorResolution(const CameraSensor *sensor);\n> > \n> > static constexpr unsigned int RKISP1_BUFFER_COUNT = 4;\n> > \n> > @@ -77,6 +78,8 @@ private:\n> > std::unique_ptr resizer_;\n> > std::unique_ptr video_;\n> > MediaLink *link_;\n> > +\n> > + std::vector sensorSizes_;\n> > };\n> > \n> > class RkISP1MainPath : public RkISP1Path","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 6981DC324C\n\tfor ;\n\tWed, 11 Sep 2024 22:29:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 51D56634F9;\n\tThu, 12 Sep 2024 00:29:44 +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 7A427634F4\n\tfor ;\n\tThu, 12 Sep 2024 00:29:42 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(213-229-8-243.static.upcbusiness.at [213.229.8.243])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6A7615B3;\n\tThu, 12 Sep 2024 00:28:24 +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=\"BQd0dmlm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1726093704;\n\tbh=Kq6dQB+wqQoHcfA89rg+KUSs1Oq5L+FYtDF+OIEVUn4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=BQd0dmlmEhUm78eR+7Ot1hVHILJ7u4tLabuX3p68CKSj0o3YUPFHW3B3bPaK328wY\n\tDccT8QyuVNyrR1LEnBQJc9BianCxLywhgfKtZPXuN7eA9C+0cJ2riYWwN7Uj54RI6c\n\t6LmWExmXX6fpeEJBzBrsH0gPrx/E/UWLQyTBerQU=","Date":"Thu, 12 Sep 2024 01:29:08 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Umang Jain ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 2/2] pipeline: rkisp1: Filter out sensor sizes not\n\tsupported by the pipeline","Message-ID":"<20240911222908.GN4470@pendragon.ideasonboard.com>","References":"<20240909163719.267211-1-umang.jain@ideasonboard.com>\n\t<20240909163719.267211-3-umang.jain@ideasonboard.com>\n\t<172605195661.1037309.3189482518520309086@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<172605195661.1037309.3189482518520309086@ping.linuxembedded.co.uk>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31192,"web_url":"https://patchwork.libcamera.org/comment/31192/","msgid":"","date":"2024-09-12T10:29:40","subject":"Re: [PATCH 2/2] pipeline: rkisp1: Filter out sensor sizes not\n\tsupported by the pipeline","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent\n\nOn 12/09/24 3:59 am, Laurent Pinchart wrote:\n> On Wed, Sep 11, 2024 at 11:52:36AM +0100, Kieran Bingham wrote:\n>> Quoting Umang Jain (2024-09-09 17:37:19)\n>>> It is possible that the maximum sensor size (returned by\n>>> CameraSensor::resolution()) is not supported by the pipeline. In such\n>>> cases, a filter function is required to filter out resolutions of the\n>>> camera sensor, which cannot be supported by the pipeline.\n>>>\n>>> Introduce the filter function filterSensorResolution() in RkISP1Path\n>>> class and use it for validate() and generateConfiguration(). The\n>>> maximum sensor resolution is picked from that filtered set of\n>>> resolutions instead of CameraSensor::resolution().\n>>>\n>>> Signed-off-by: Umang Jain \n>>> ---\n>>> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 48 ++++++++++++++++++-\n>>> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 3 ++\n>>> 2 files changed, 49 insertions(+), 2 deletions(-)\n>>>\n>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n>>> index 9053af18..b3b27aa5 100644\n>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n>>> @@ -128,12 +128,56 @@ void RkISP1Path::populateFormats()\n>>> }\n>>> }\n>>> \n>>> +/**\n>>> + * \\brief Filter the sensor resolutions that can be supported\n>>> + * \\param[in] sensor The camera sensor\n>>> + *\n>>> + * This function retrieves all the sizes supported by the sensor and\n>>> + * filters all the resolutions that can be supported on the pipeline.\n>>> + * It is possible that the sensor's maximum output resolution is higher\n>>> + * than the ISP maximum input. In that case, this function filters out all\n>>> + * the resolution incapable of being supported and returns the maximum\n>>> + * sensor resolution that can be supported by the pipeline.\n>>> + *\n>>> + * \\return Maximum sensor size supported on the pipeline\n>>> + */\n>>> +Size RkISP1Path::filterSensorResolution(const CameraSensor *sensor)\n>>> +{\n>>> + if (!sensorSizes_.empty())\n>>> + return sensorSizes_.back();\n> There are systems based on rkisp1 where multiple sensors are connected\n> to the same ISP. Obviously only one of them can be used at a time, but\n> applications can switch between them. This will be broken by caching\n> sensorSizes_.\n\nIndeed, something that I missed while developing on i.MX8MP (two cameras \nbut dual ISP as well)\n\nI'll convert it to a std::map in next revision.\n\n>> I like that we keep the full list of supported sensorSizes_. My first\n>> thought was that this list should be generated when the path is created,\n>> but this also seems fine to me.\n>>\n>> I think there's no need to complicate / split this until there's\n>> actually a user of the full sensorSizes list ?\n>>\n>> I'm a little surprised that's not not used actually. Do we report the\n>> raw modes we support somewhere? Should that iterate the 'sensorSizes_'\n>> as the supported sizes ?\n\nI will test how raw sizes are reported ? sensorSizes_ is a good \ncandidate to update the raw size list as well.\n>>\n>> Perhaps that's an improvement/fix on top though.\n>>\n>> Reviewed-by: Kieran Bingham \n>>\n>>> +\n>>> + std::vector sensorSizes;\n>>> + const std::vector &mbusCodes = sensor->mbusCodes();\n>>> + for (const auto iter : mbusCodes) {\n>>> + std::vector sizes = sensor->sizes(iter);\n>>> + for (Size sz : sizes)\n>>> + sensorSizes.push_back(sz);\n>>> + }\n>>> +\n>>> + std::sort(sensorSizes.begin(), sensorSizes.end());\n>>> +\n>>> + /* Remove duplicates. */\n>>> + auto last = std::unique(sensorSizes.begin(), sensorSizes.end());\n>>> + sensorSizes.erase(last, sensorSizes.end());\n>>> +\n>>> + /* Discard any sizes that the pipeline is unable to support. */\n>>> + for (auto sz : sensorSizes) {\n>>> + if (sz.width > maxResolution_.width ||\n>>> + sz.height > maxResolution_.height)\n>>> + continue;\n>>> +\n>>> + sensorSizes_.push_back(sz);\n>>> + }\n>>> +\n>>> + return sensorSizes_.back();\n>>> +}\n>>> +\n>>> StreamConfiguration\n>>> RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,\n>>> StreamRole role)\n>>> {\n>>> const std::vector &mbusCodes = sensor->mbusCodes();\n>>> - const Size &resolution = sensor->resolution();\n>>> + Size resolution = filterSensorResolution(sensor);\n>>> \n>>> /* Min and max resolutions to populate the available stream formats. */\n>>> Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)\n>>> @@ -222,7 +266,7 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n>>> StreamConfiguration *cfg)\n>>> {\n>>> const std::vector &mbusCodes = sensor->mbusCodes();\n>>> - const Size &resolution = sensor->resolution();\n>>> + Size resolution = filterSensorResolution(sensor);\n>>> \n>>> const StreamConfiguration reqCfg = *cfg;\n>>> CameraConfiguration::Status status = CameraConfiguration::Valid;\n>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n>>> index 13ba4b62..03ff9543 100644\n>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n>>> @@ -63,6 +63,7 @@ public:\n>>> \n>>> private:\n>>> void populateFormats();\n>>> + Size filterSensorResolution(const CameraSensor *sensor);\n>>> \n>>> static constexpr unsigned int RKISP1_BUFFER_COUNT = 4;\n>>> \n>>> @@ -77,6 +78,8 @@ private:\n>>> std::unique_ptr resizer_;\n>>> std::unique_ptr video_;\n>>> MediaLink *link_;\n>>> +\n>>> + std::vector sensorSizes_;\n>>> };\n>>> \n>>> class RkISP1MainPath : public RkISP1Path","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 30BDDBF415\n\tfor ;\n\tThu, 12 Sep 2024 10:29:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 21C2C634F5;\n\tThu, 12 Sep 2024 12:29:47 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B807F634F2\n\tfor ;\n\tThu, 12 Sep 2024 12:29:44 +0200 (CEST)","from [IPV6:2405:201:2015:f873:55d7:c02e:b2eb:ee3f] (unknown\n\t[IPv6:2405:201:2015:f873:55d7:c02e:b2eb:ee3f])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0574C496;\n\tThu, 12 Sep 2024 12:28:25 +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=\"PP8ZuH3G\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1726136906;\n\tbh=FgfCNY8XsWk1fTQ9QonlWzw+SiB7IPe+lynTwDSNFf4=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=PP8ZuH3GNPWRTtc6rUQSx7DEyWU/0d+1PBFwtgt3DBmBIvp3aYsi0/sPqDt1d3z9g\n\tVqiHL+aalDEjZbGVr5x0XPDvL/vWnddo81dUb9LasqtMAtBEwOvp9kYowmKPfas7yx\n\tlb/NvDZHN7RIoEDMEpaeS59mcMwfxmHCSUEZHGH0=","Message-ID":"","Date":"Thu, 12 Sep 2024 15:59:40 +0530","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH 2/2] pipeline: rkisp1: Filter out sensor sizes not\n\tsupported by the pipeline","To":"Laurent Pinchart ,\n\tKieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20240909163719.267211-1-umang.jain@ideasonboard.com>\n\t<20240909163719.267211-3-umang.jain@ideasonboard.com>\n\t<172605195661.1037309.3189482518520309086@ping.linuxembedded.co.uk>\n\t<20240911222908.GN4470@pendragon.ideasonboard.com>","Content-Language":"en-US","From":"Umang Jain ","In-Reply-To":"<20240911222908.GN4470@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31193,"web_url":"https://patchwork.libcamera.org/comment/31193/","msgid":"<172613810899.1037309.1714064074587063355@ping.linuxembedded.co.uk>","date":"2024-09-12T10:48:28","subject":"Re: [PATCH 2/2] pipeline: rkisp1: Filter out sensor sizes not\n\tsupported by the pipeline","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Umang Jain (2024-09-12 11:29:40)\n> Hi Laurent\n> \n> On 12/09/24 3:59 am, Laurent Pinchart wrote:\n> > On Wed, Sep 11, 2024 at 11:52:36AM +0100, Kieran Bingham wrote:\n> >> Quoting Umang Jain (2024-09-09 17:37:19)\n> >>> It is possible that the maximum sensor size (returned by\n> >>> CameraSensor::resolution()) is not supported by the pipeline. In such\n> >>> cases, a filter function is required to filter out resolutions of the\n> >>> camera sensor, which cannot be supported by the pipeline.\n> >>>\n> >>> Introduce the filter function filterSensorResolution() in RkISP1Path\n> >>> class and use it for validate() and generateConfiguration(). The\n> >>> maximum sensor resolution is picked from that filtered set of\n> >>> resolutions instead of CameraSensor::resolution().\n> >>>\n> >>> Signed-off-by: Umang Jain \n> >>> ---\n> >>> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 48 ++++++++++++++++++-\n> >>> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 3 ++\n> >>> 2 files changed, 49 insertions(+), 2 deletions(-)\n> >>>\n> >>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> >>> index 9053af18..b3b27aa5 100644\n> >>> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> >>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> >>> @@ -128,12 +128,56 @@ void RkISP1Path::populateFormats()\n> >>> }\n> >>> }\n> >>> \n> >>> +/**\n> >>> + * \\brief Filter the sensor resolutions that can be supported\n> >>> + * \\param[in] sensor The camera sensor\n> >>> + *\n> >>> + * This function retrieves all the sizes supported by the sensor and\n> >>> + * filters all the resolutions that can be supported on the pipeline.\n> >>> + * It is possible that the sensor's maximum output resolution is higher\n> >>> + * than the ISP maximum input. In that case, this function filters out all\n> >>> + * the resolution incapable of being supported and returns the maximum\n> >>> + * sensor resolution that can be supported by the pipeline.\n> >>> + *\n> >>> + * \\return Maximum sensor size supported on the pipeline\n> >>> + */\n> >>> +Size RkISP1Path::filterSensorResolution(const CameraSensor *sensor)\n> >>> +{\n> >>> + if (!sensorSizes_.empty())\n> >>> + return sensorSizes_.back();\n> > There are systems based on rkisp1 where multiple sensors are connected\n> > to the same ISP. Obviously only one of them can be used at a time, but\n> > applications can switch between them. This will be broken by caching\n> > sensorSizes_.\n> \n> Indeed, something that I missed while developing on i.MX8MP (two cameras \n> but dual ISP as well)\n> \n> I'll convert it to a std::map in next revision.\n\nRather than a map - can we just put this filter/or the sensorSizes_ into\nthe camera data?\n\n\n\n> \n> >> I like that we keep the full list of supported sensorSizes_. My first\n> >> thought was that this list should be generated when the path is created,\n> >> but this also seems fine to me.\n> >>\n> >> I think there's no need to complicate / split this until there's\n> >> actually a user of the full sensorSizes list ?\n> >>\n> >> I'm a little surprised that's not not used actually. Do we report the\n> >> raw modes we support somewhere? Should that iterate the 'sensorSizes_'\n> >> as the supported sizes ?\n> \n> I will test how raw sizes are reported ? sensorSizes_ is a good \n> candidate to update the raw size list as well.\n\nI'm intrigued on what it's currently reporting...\n\n--\nKieran\n\n\n> >>\n> >> Perhaps that's an improvement/fix on top though.\n> >>\n> >> Reviewed-by: Kieran Bingham \n> >>\n> >>> +\n> >>> + std::vector sensorSizes;\n> >>> + const std::vector &mbusCodes = sensor->mbusCodes();\n> >>> + for (const auto iter : mbusCodes) {\n> >>> + std::vector sizes = sensor->sizes(iter);\n> >>> + for (Size sz : sizes)\n> >>> + sensorSizes.push_back(sz);\n> >>> + }\n> >>> +\n> >>> + std::sort(sensorSizes.begin(), sensorSizes.end());\n> >>> +\n> >>> + /* Remove duplicates. */\n> >>> + auto last = std::unique(sensorSizes.begin(), sensorSizes.end());\n> >>> + sensorSizes.erase(last, sensorSizes.end());\n> >>> +\n> >>> + /* Discard any sizes that the pipeline is unable to support. */\n> >>> + for (auto sz : sensorSizes) {\n> >>> + if (sz.width > maxResolution_.width ||\n> >>> + sz.height > maxResolution_.height)\n> >>> + continue;\n> >>> +\n> >>> + sensorSizes_.push_back(sz);\n> >>> + }\n> >>> +\n> >>> + return sensorSizes_.back();\n> >>> +}\n> >>> +\n> >>> StreamConfiguration\n> >>> RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,\n> >>> StreamRole role)\n> >>> {\n> >>> const std::vector &mbusCodes = sensor->mbusCodes();\n> >>> - const Size &resolution = sensor->resolution();\n> >>> + Size resolution = filterSensorResolution(sensor);\n> >>> \n> >>> /* Min and max resolutions to populate the available stream formats. */\n> >>> Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)\n> >>> @@ -222,7 +266,7 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n> >>> StreamConfiguration *cfg)\n> >>> {\n> >>> const std::vector &mbusCodes = sensor->mbusCodes();\n> >>> - const Size &resolution = sensor->resolution();\n> >>> + Size resolution = filterSensorResolution(sensor);\n> >>> \n> >>> const StreamConfiguration reqCfg = *cfg;\n> >>> CameraConfiguration::Status status = CameraConfiguration::Valid;\n> >>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> >>> index 13ba4b62..03ff9543 100644\n> >>> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> >>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> >>> @@ -63,6 +63,7 @@ public:\n> >>> \n> >>> private:\n> >>> void populateFormats();\n> >>> + Size filterSensorResolution(const CameraSensor *sensor);\n> >>> \n> >>> static constexpr unsigned int RKISP1_BUFFER_COUNT = 4;\n> >>> \n> >>> @@ -77,6 +78,8 @@ private:\n> >>> std::unique_ptr resizer_;\n> >>> std::unique_ptr video_;\n> >>> MediaLink *link_;\n> >>> +\n> >>> + std::vector sensorSizes_;\n> >>> };\n> >>> \n> >>> class RkISP1MainPath : public RkISP1Path\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 56A41C324C\n\tfor ;\n\tThu, 12 Sep 2024 10:48:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 28DD2634FC;\n\tThu, 12 Sep 2024 12:48:34 +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 EF097634F2\n\tfor ;\n\tThu, 12 Sep 2024 12:48:31 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 762225B3;\n\tThu, 12 Sep 2024 12:47:13 +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=\"QFSt7OUR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1726138033;\n\tbh=QzcysF5fZryEosbnw/nNHjIQaukSKTUFRcf+ZLT8yog=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=QFSt7OUR2fqSbuKOb/Supyeb8LbWijQOBKd++Hckq0sDEH5fAJcwPFWZ0qyFfwge1\n\taC3VGw+0DfD2t/ELEmTJEA2WSeKlTtbfbyA2IrxUCUx24p3GxOVc3U6GE9ONLWyMPg\n\tZgCYkf7hwZujOHt5Bjqp+GKRtKXxMMCQXgS8BP4E=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20240909163719.267211-1-umang.jain@ideasonboard.com>\n\t<20240909163719.267211-3-umang.jain@ideasonboard.com>\n\t<172605195661.1037309.3189482518520309086@ping.linuxembedded.co.uk>\n\t<20240911222908.GN4470@pendragon.ideasonboard.com>\n\t","Subject":"Re: [PATCH 2/2] pipeline: rkisp1: Filter out sensor sizes not\n\tsupported by the pipeline","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","To":"Laurent Pinchart ,\n\tUmang Jain ","Date":"Thu, 12 Sep 2024 11:48:28 +0100","Message-ID":"<172613810899.1037309.1714064074587063355@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31194,"web_url":"https://patchwork.libcamera.org/comment/31194/","msgid":"","date":"2024-09-12T10:57:50","subject":"Re: [PATCH 2/2] pipeline: rkisp1: Filter out sensor sizes not\n\tsupported by the pipeline","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"On 12/09/24 4:18 pm, Kieran Bingham wrote:\n> Quoting Umang Jain (2024-09-12 11:29:40)\n>> Hi Laurent\n>>\n>> On 12/09/24 3:59 am, Laurent Pinchart wrote:\n>>> On Wed, Sep 11, 2024 at 11:52:36AM +0100, Kieran Bingham wrote:\n>>>> Quoting Umang Jain (2024-09-09 17:37:19)\n>>>>> It is possible that the maximum sensor size (returned by\n>>>>> CameraSensor::resolution()) is not supported by the pipeline. In such\n>>>>> cases, a filter function is required to filter out resolutions of the\n>>>>> camera sensor, which cannot be supported by the pipeline.\n>>>>>\n>>>>> Introduce the filter function filterSensorResolution() in RkISP1Path\n>>>>> class and use it for validate() and generateConfiguration(). The\n>>>>> maximum sensor resolution is picked from that filtered set of\n>>>>> resolutions instead of CameraSensor::resolution().\n>>>>>\n>>>>> Signed-off-by: Umang Jain \n>>>>> ---\n>>>>> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 48 ++++++++++++++++++-\n>>>>> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 3 ++\n>>>>> 2 files changed, 49 insertions(+), 2 deletions(-)\n>>>>>\n>>>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n>>>>> index 9053af18..b3b27aa5 100644\n>>>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n>>>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n>>>>> @@ -128,12 +128,56 @@ void RkISP1Path::populateFormats()\n>>>>> }\n>>>>> }\n>>>>> \n>>>>> +/**\n>>>>> + * \\brief Filter the sensor resolutions that can be supported\n>>>>> + * \\param[in] sensor The camera sensor\n>>>>> + *\n>>>>> + * This function retrieves all the sizes supported by the sensor and\n>>>>> + * filters all the resolutions that can be supported on the pipeline.\n>>>>> + * It is possible that the sensor's maximum output resolution is higher\n>>>>> + * than the ISP maximum input. In that case, this function filters out all\n>>>>> + * the resolution incapable of being supported and returns the maximum\n>>>>> + * sensor resolution that can be supported by the pipeline.\n>>>>> + *\n>>>>> + * \\return Maximum sensor size supported on the pipeline\n>>>>> + */\n>>>>> +Size RkISP1Path::filterSensorResolution(const CameraSensor *sensor)\n>>>>> +{\n>>>>> + if (!sensorSizes_.empty())\n>>>>> + return sensorSizes_.back();\n>>> There are systems based on rkisp1 where multiple sensors are connected\n>>> to the same ISP. Obviously only one of them can be used at a time, but\n>>> applications can switch between them. This will be broken by caching\n>>> sensorSizes_.\n>> Indeed, something that I missed while developing on i.MX8MP (two cameras\n>> but dual ISP as well)\n>>\n>> I'll convert it to a std::map in next revision.\n> Rather than a map - can we just put this filter/or the sensorSizes_ into\n> the camera data?\n\nhmm, might require passing CameraData into RkISP1Path::validate(...)\n\nand\n\nRkISP1Path:: generateConfiguration(const CameraSensor *sensor,....)\n\nPossibly replacing the CameraSensor function argument in both.\n\n>\n>\n>\n>>>> I like that we keep the full list of supported sensorSizes_. My first\n>>>> thought was that this list should be generated when the path is created,\n>>>> but this also seems fine to me.\n>>>>\n>>>> I think there's no need to complicate / split this until there's\n>>>> actually a user of the full sensorSizes list ?\n>>>>\n>>>> I'm a little surprised that's not not used actually. Do we report the\n>>>> raw modes we support somewhere? Should that iterate the 'sensorSizes_'\n>>>> as the supported sizes ?\n>> I will test how raw sizes are reported ? sensorSizes_ is a good\n>> candidate to update the raw size list as well.\n> I'm intrigued on what it's currently reporting...\n>\n> --\n> Kieran\n>\n>\n>>>> Perhaps that's an improvement/fix on top though.\n>>>>\n>>>> Reviewed-by: Kieran Bingham \n>>>>\n>>>>> +\n>>>>> + std::vector sensorSizes;\n>>>>> + const std::vector &mbusCodes = sensor->mbusCodes();\n>>>>> + for (const auto iter : mbusCodes) {\n>>>>> + std::vector sizes = sensor->sizes(iter);\n>>>>> + for (Size sz : sizes)\n>>>>> + sensorSizes.push_back(sz);\n>>>>> + }\n>>>>> +\n>>>>> + std::sort(sensorSizes.begin(), sensorSizes.end());\n>>>>> +\n>>>>> + /* Remove duplicates. */\n>>>>> + auto last = std::unique(sensorSizes.begin(), sensorSizes.end());\n>>>>> + sensorSizes.erase(last, sensorSizes.end());\n>>>>> +\n>>>>> + /* Discard any sizes that the pipeline is unable to support. */\n>>>>> + for (auto sz : sensorSizes) {\n>>>>> + if (sz.width > maxResolution_.width ||\n>>>>> + sz.height > maxResolution_.height)\n>>>>> + continue;\n>>>>> +\n>>>>> + sensorSizes_.push_back(sz);\n>>>>> + }\n>>>>> +\n>>>>> + return sensorSizes_.back();\n>>>>> +}\n>>>>> +\n>>>>> StreamConfiguration\n>>>>> RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,\n>>>>> StreamRole role)\n>>>>> {\n>>>>> const std::vector &mbusCodes = sensor->mbusCodes();\n>>>>> - const Size &resolution = sensor->resolution();\n>>>>> + Size resolution = filterSensorResolution(sensor);\n>>>>> \n>>>>> /* Min and max resolutions to populate the available stream formats. */\n>>>>> Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)\n>>>>> @@ -222,7 +266,7 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,\n>>>>> StreamConfiguration *cfg)\n>>>>> {\n>>>>> const std::vector &mbusCodes = sensor->mbusCodes();\n>>>>> - const Size &resolution = sensor->resolution();\n>>>>> + Size resolution = filterSensorResolution(sensor);\n>>>>> \n>>>>> const StreamConfiguration reqCfg = *cfg;\n>>>>> CameraConfiguration::Status status = CameraConfiguration::Valid;\n>>>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n>>>>> index 13ba4b62..03ff9543 100644\n>>>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n>>>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n>>>>> @@ -63,6 +63,7 @@ public:\n>>>>> \n>>>>> private:\n>>>>> void populateFormats();\n>>>>> + Size filterSensorResolution(const CameraSensor *sensor);\n>>>>> \n>>>>> static constexpr unsigned int RKISP1_BUFFER_COUNT = 4;\n>>>>> \n>>>>> @@ -77,6 +78,8 @@ private:\n>>>>> std::unique_ptr resizer_;\n>>>>> std::unique_ptr video_;\n>>>>> MediaLink *link_;\n>>>>> +\n>>>>> + std::vector sensorSizes_;\n>>>>> };\n>>>>> \n>>>>> class RkISP1MainPath : public RkISP1Path","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 B4C9ABF415\n\tfor ;\n\tThu, 12 Sep 2024 10:57:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6918A634FC;\n\tThu, 12 Sep 2024 12:57:56 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CBE47634F2\n\tfor ;\n\tThu, 12 Sep 2024 12:57:54 +0200 (CEST)","from [IPV6:2405:201:2015:f873:55d7:c02e:b2eb:ee3f] (unknown\n\t[IPv6:2405:201:2015:f873:55d7:c02e:b2eb:ee3f])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0E1DB5B3;\n\tThu, 12 Sep 2024 12:56:35 +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=\"drI0bcwz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1726138596;\n\tbh=CH7lI0U35+unRPI0bQghf/ZR5Ww1m4q6SPgRomqdhB8=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=drI0bcwz31LPLM3dhziWSb+Abw/2RQ7L51BGyOA5CwzX1GRYG+yvH/DTv3Nnfy1sM\n\t1B8ZxA8qFBDOGQkhsSPFZ6osSaSg3h0dodGPChUPE2vRmtaI8CTQ1ruE/v+Mxm71/O\n\txzYauowix5vbWNRXXlDx8QGeblppFEsKF6tcFHFM=","Message-ID":"","Date":"Thu, 12 Sep 2024 16:27:50 +0530","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH 2/2] pipeline: rkisp1: Filter out sensor sizes not\n\tsupported by the pipeline","To":"Kieran Bingham ,\n\tLaurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20240909163719.267211-1-umang.jain@ideasonboard.com>\n\t<20240909163719.267211-3-umang.jain@ideasonboard.com>\n\t<172605195661.1037309.3189482518520309086@ping.linuxembedded.co.uk>\n\t<20240911222908.GN4470@pendragon.ideasonboard.com>\n\t\n\t<172613810899.1037309.1714064074587063355@ping.linuxembedded.co.uk>","Content-Language":"en-US","From":"Umang Jain ","In-Reply-To":"<172613810899.1037309.1714064074587063355@ping.linuxembedded.co.uk>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31199,"web_url":"https://patchwork.libcamera.org/comment/31199/","msgid":"<4aa9eb58-3b59-4ad5-9ce7-fd14ce9c97b3@ideasonboard.com>","date":"2024-09-13T05:33:33","subject":"Re: [PATCH 2/2] pipeline: rkisp1: Filter out sensor sizes not\n\tsupported by the pipeline","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hello\n\nOn 12/09/24 4:27 pm, Umang Jain wrote:\n>\n>\n> On 12/09/24 4:18 pm, Kieran Bingham wrote:\n>> Quoting Umang Jain (2024-09-12 11:29:40)\n>>> Hi Laurent\n>>>\n>>> On 12/09/24 3:59 am, Laurent Pinchart wrote:\n>>>> On Wed, Sep 11, 2024 at 11:52:36AM +0100, Kieran Bingham wrote:\n>>>>> Quoting Umang Jain (2024-09-09 17:37:19)\n>>>>>> It is possible that the maximum sensor size (returned by\n>>>>>> CameraSensor::resolution()) is not supported by the pipeline. In \n>>>>>> such\n>>>>>> cases, a filter function is required to filter out resolutions of \n>>>>>> the\n>>>>>> camera sensor, which cannot be supported by the pipeline.\n>>>>>>\n>>>>>> Introduce the filter function filterSensorResolution() in RkISP1Path\n>>>>>> class and use it for validate() and generateConfiguration(). The\n>>>>>> maximum sensor resolution is picked from that filtered set of\n>>>>>> resolutions instead of CameraSensor::resolution().\n>>>>>>\n>>>>>> Signed-off-by: Umang Jain \n>>>>>> ---\n>>>>>>    src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 48 \n>>>>>> ++++++++++++++++++-\n>>>>>>    src/libcamera/pipeline/rkisp1/rkisp1_path.h   |  3 ++\n>>>>>>    2 files changed, 49 insertions(+), 2 deletions(-)\n>>>>>>\n>>>>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp \n>>>>>> b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n>>>>>> index 9053af18..b3b27aa5 100644\n>>>>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n>>>>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n>>>>>> @@ -128,12 +128,56 @@ void RkISP1Path::populateFormats()\n>>>>>>           }\n>>>>>>    }\n>>>>>>    +/**\n>>>>>> + * \\brief Filter the sensor resolutions that can be supported\n>>>>>> + * \\param[in] sensor The camera sensor\n>>>>>> + *\n>>>>>> + * This function retrieves all the sizes supported by the sensor \n>>>>>> and\n>>>>>> + * filters all the resolutions that can be supported on the \n>>>>>> pipeline.\n>>>>>> + * It is possible that the sensor's maximum output resolution is \n>>>>>> higher\n>>>>>> + * than the ISP maximum input. In that case, this function \n>>>>>> filters out all\n>>>>>> + * the resolution incapable of being supported and returns the \n>>>>>> maximum\n>>>>>> + * sensor resolution that can be supported by the pipeline.\n>>>>>> + *\n>>>>>> + * \\return Maximum sensor size supported on the pipeline\n>>>>>> + */\n>>>>>> +Size RkISP1Path::filterSensorResolution(const CameraSensor *sensor)\n>>>>>> +{\n>>>>>> +       if (!sensorSizes_.empty())\n>>>>>> +               return sensorSizes_.back();\n>>>> There are systems based on rkisp1 where multiple sensors are connected\n>>>> to the same ISP. Obviously only one of them can be used at a time, but\n>>>> applications can switch between them. This will be broken by caching\n>>>> sensorSizes_.\n>>> Indeed, something that I missed while developing on i.MX8MP (two \n>>> cameras\n>>> but dual ISP as well)\n>>>\n>>> I'll convert it to a std::map in next revision.\n>> Rather than a map - can we just put this filter/or the sensorSizes_ into\n>> the camera data?\n>\n> hmm, might require passing CameraData into RkISP1Path::validate(...)\n>\n> and\n>\n> RkISP1Path:: generateConfiguration(const CameraSensor *sensor,....)\n>\n> Possibly replacing the CameraSensor function argument in both.\n\nDoing this seems a bit awkward, given the RkISP1CameraData is defined in \nrkisp1.cpp\nand it already has pointers to mainPath and selfPath RkISP1Path.\n\nShould I try making RkISP1CameraData shared to path classes for solely \nthis purpose?\n>\n>>\n>>\n>>\n>>>>> I like that we keep the full list of supported sensorSizes_. My first\n>>>>> thought was that this list should be generated when the path is \n>>>>> created,\n>>>>> but this also seems fine to me.\n>>>>>\n>>>>> I think there's no need to complicate / split this until there's\n>>>>> actually a user of the full sensorSizes list ?\n>>>>>\n>>>>> I'm a little surprised that's not not used actually. Do we report the\n>>>>> raw modes we support somewhere? Should that iterate the \n>>>>> 'sensorSizes_'\n>>>>> as the supported sizes ?\n>>> I will test how raw sizes are reported ? sensorSizes_ is a good\n>>> candidate to update the raw size list as well.\n>> I'm intrigued on what it's currently reporting...\n\nIt's reporting the max sizes i.e. 5472x3648\n\nAfter this series is applied, it's reports correct list with 5472x3648 \nfiltered out from the list.\n\nSince the raw formats are reported with [format, Size], just iterating \nover the sensorSize_ is not enough. So probably, the usage of \nsensorSizes_ is limited for now.\n>>\n>> -- \n>> Kieran\n>>\n>>\n>>>>> Perhaps that's an improvement/fix on top though.\n>>>>>\n>>>>> Reviewed-by: Kieran Bingham \n>>>>>\n>>>>>> +\n>>>>>> +       std::vector sensorSizes;\n>>>>>> +       const std::vector &mbusCodes = \n>>>>>> sensor->mbusCodes();\n>>>>>> +       for (const auto iter : mbusCodes) {\n>>>>>> +               std::vector sizes = sensor->sizes(iter);\n>>>>>> +               for (Size sz : sizes)\n>>>>>> +                       sensorSizes.push_back(sz);\n>>>>>> +       }\n>>>>>> +\n>>>>>> +       std::sort(sensorSizes.begin(), sensorSizes.end());\n>>>>>> +\n>>>>>> +       /* Remove duplicates. */\n>>>>>> +       auto last = std::unique(sensorSizes.begin(), \n>>>>>> sensorSizes.end());\n>>>>>> +       sensorSizes.erase(last, sensorSizes.end());\n>>>>>> +\n>>>>>> +       /* Discard any sizes that the pipeline is unable to \n>>>>>> support. */\n>>>>>> +       for (auto sz : sensorSizes) {\n>>>>>> +               if (sz.width > maxResolution_.width ||\n>>>>>> +                   sz.height > maxResolution_.height)\n>>>>>> +                       continue;\n>>>>>> +\n>>>>>> +               sensorSizes_.push_back(sz);\n>>>>>> +       }\n>>>>>> +\n>>>>>> +       return sensorSizes_.back();\n>>>>>> +}\n>>>>>> +\n>>>>>>    StreamConfiguration\n>>>>>>    RkISP1Path::generateConfiguration(const CameraSensor *sensor, \n>>>>>> const Size &size,\n>>>>>>                                     StreamRole role)\n>>>>>>    {\n>>>>>>           const std::vector &mbusCodes = \n>>>>>> sensor->mbusCodes();\n>>>>>> -       const Size &resolution = sensor->resolution();\n>>>>>> +       Size resolution = filterSensorResolution(sensor);\n>>>>>>              /* Min and max resolutions to populate the available \n>>>>>> stream formats. */\n>>>>>>           Size maxResolution = \n>>>>>> maxResolution_.boundedToAspectRatio(resolution)\n>>>>>> @@ -222,7 +266,7 @@ CameraConfiguration::Status \n>>>>>> RkISP1Path::validate(const CameraSensor *sensor,\n>>>>>> StreamConfiguration *cfg)\n>>>>>>    {\n>>>>>>           const std::vector &mbusCodes = \n>>>>>> sensor->mbusCodes();\n>>>>>> -       const Size &resolution = sensor->resolution();\n>>>>>> +       Size resolution = filterSensorResolution(sensor);\n>>>>>>              const StreamConfiguration reqCfg = *cfg;\n>>>>>>           CameraConfiguration::Status status = \n>>>>>> CameraConfiguration::Valid;\n>>>>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h \n>>>>>> b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n>>>>>> index 13ba4b62..03ff9543 100644\n>>>>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n>>>>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n>>>>>> @@ -63,6 +63,7 @@ public:\n>>>>>>       private:\n>>>>>>           void populateFormats();\n>>>>>> +       Size filterSensorResolution(const CameraSensor *sensor);\n>>>>>>              static constexpr unsigned int RKISP1_BUFFER_COUNT = 4;\n>>>>>>    @@ -77,6 +78,8 @@ private:\n>>>>>>           std::unique_ptr resizer_;\n>>>>>>           std::unique_ptr video_;\n>>>>>>           MediaLink *link_;\n>>>>>> +\n>>>>>> +       std::vector sensorSizes_;\n>>>>>>    };\n>>>>>>       class RkISP1MainPath : public RkISP1Path\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 1D3E2C3257\n\tfor ;\n\tFri, 13 Sep 2024 05:33:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 11A86634F5;\n\tFri, 13 Sep 2024 07:33:46 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6E8AC618E8\n\tfor ;\n\tFri, 13 Sep 2024 07:33:44 +0200 (CEST)","from [IPV6:2405:201:2015:f873:c173:4b:4a04:3a21] (unknown\n\t[IPv6:2405:201:2015:f873:c173:4b:4a04:3a21])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9D71F1961;\n\tFri, 13 Sep 2024 07:32:20 +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=\"eIzfnVqR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1726205542;\n\tbh=ydqfZd06B1VQg0rcf2qFRsRwbgI4eKHYign10ds5JpI=;\n\th=Date:Subject:From:To:Cc:References:In-Reply-To:From;\n\tb=eIzfnVqRiZ8z3gNYYbiHYHhZL73cpzGQukX/3H14IgYT/KxjkNUicaEPwTiukItb9\n\t2heMIFLf+EZl7CkRJmIOdG4AL6CAo2U9UToDpctbtjNkJpxeUECcAOc+44I0lH4JY1\n\t575+MebzDePSxXD7N9P2e4VAagHhL7rSTnOpilQY=","Message-ID":"<4aa9eb58-3b59-4ad5-9ce7-fd14ce9c97b3@ideasonboard.com>","Date":"Fri, 13 Sep 2024 11:03:33 +0530","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH 2/2] pipeline: rkisp1: Filter out sensor sizes not\n\tsupported by the pipeline","From":"Umang Jain ","To":"Kieran Bingham ,\n\tLaurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20240909163719.267211-1-umang.jain@ideasonboard.com>\n\t<20240909163719.267211-3-umang.jain@ideasonboard.com>\n\t<172605195661.1037309.3189482518520309086@ping.linuxembedded.co.uk>\n\t<20240911222908.GN4470@pendragon.ideasonboard.com>\n\t\n\t<172613810899.1037309.1714064074587063355@ping.linuxembedded.co.uk>\n\t","Content-Language":"en-US","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]