| Message ID | 20240909163719.267211-3-umang.jain@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Quoting Umang Jain (2024-09-09 17:37:19) > It is possible that the maximum sensor size (returned by > CameraSensor::resolution()) is not supported by the pipeline. In such > cases, a filter function is required to filter out resolutions of the > camera sensor, which cannot be supported by the pipeline. > > Introduce the filter function filterSensorResolution() in RkISP1Path > class and use it for validate() and generateConfiguration(). The > maximum sensor resolution is picked from that filtered set of > resolutions instead of CameraSensor::resolution(). > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 48 ++++++++++++++++++- > src/libcamera/pipeline/rkisp1/rkisp1_path.h | 3 ++ > 2 files changed, 49 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > index 9053af18..b3b27aa5 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > @@ -128,12 +128,56 @@ void RkISP1Path::populateFormats() > } > } > > +/** > + * \brief Filter the sensor resolutions that can be supported > + * \param[in] sensor The camera sensor > + * > + * This function retrieves all the sizes supported by the sensor and > + * filters all the resolutions that can be supported on the pipeline. > + * It is possible that the sensor's maximum output resolution is higher > + * than the ISP maximum input. In that case, this function filters out all > + * the resolution incapable of being supported and returns the maximum > + * sensor resolution that can be supported by the pipeline. > + * > + * \return Maximum sensor size supported on the pipeline > + */ > +Size RkISP1Path::filterSensorResolution(const CameraSensor *sensor) > +{ > + if (!sensorSizes_.empty()) > + return sensorSizes_.back(); I like that we keep the full list of supported sensorSizes_. My first thought was that this list should be generated when the path is created, but this also seems fine to me. I think there's no need to complicate / split this until there's actually a user of the full sensorSizes list ? I'm a little surprised that's not not used actually. Do we report the raw modes we support somewhere? Should that iterate the 'sensorSizes_' as the supported sizes ? Perhaps that's an improvement/fix on top though. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + > + std::vector<Size> sensorSizes; > + const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes(); > + for (const auto iter : mbusCodes) { > + std::vector<Size> sizes = sensor->sizes(iter); > + for (Size sz : sizes) > + sensorSizes.push_back(sz); > + } > + > + std::sort(sensorSizes.begin(), sensorSizes.end()); > + > + /* Remove duplicates. */ > + auto last = std::unique(sensorSizes.begin(), sensorSizes.end()); > + sensorSizes.erase(last, sensorSizes.end()); > + > + /* Discard any sizes that the pipeline is unable to support. */ > + for (auto sz : sensorSizes) { > + if (sz.width > maxResolution_.width || > + sz.height > maxResolution_.height) > + continue; > + > + sensorSizes_.push_back(sz); > + } > + > + return sensorSizes_.back(); > +} > + > StreamConfiguration > RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size, > StreamRole role) > { > const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes(); > - const Size &resolution = sensor->resolution(); > + Size resolution = filterSensorResolution(sensor); > > /* Min and max resolutions to populate the available stream formats. */ > Size maxResolution = maxResolution_.boundedToAspectRatio(resolution) > @@ -222,7 +266,7 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor, > StreamConfiguration *cfg) > { > const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes(); > - const Size &resolution = sensor->resolution(); > + Size resolution = filterSensorResolution(sensor); > > const StreamConfiguration reqCfg = *cfg; > CameraConfiguration::Status status = CameraConfiguration::Valid; > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > index 13ba4b62..03ff9543 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > @@ -63,6 +63,7 @@ public: > > private: > void populateFormats(); > + Size filterSensorResolution(const CameraSensor *sensor); > > static constexpr unsigned int RKISP1_BUFFER_COUNT = 4; > > @@ -77,6 +78,8 @@ private: > std::unique_ptr<V4L2Subdevice> resizer_; > std::unique_ptr<V4L2VideoDevice> video_; > MediaLink *link_; > + > + std::vector<Size> sensorSizes_; > }; > > class RkISP1MainPath : public RkISP1Path > -- > 2.45.0 >
On Wed, Sep 11, 2024 at 11:52:36AM +0100, Kieran Bingham wrote: > Quoting Umang Jain (2024-09-09 17:37:19) > > It is possible that the maximum sensor size (returned by > > CameraSensor::resolution()) is not supported by the pipeline. In such > > cases, a filter function is required to filter out resolutions of the > > camera sensor, which cannot be supported by the pipeline. > > > > Introduce the filter function filterSensorResolution() in RkISP1Path > > class and use it for validate() and generateConfiguration(). The > > maximum sensor resolution is picked from that filtered set of > > resolutions instead of CameraSensor::resolution(). > > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > --- > > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 48 ++++++++++++++++++- > > src/libcamera/pipeline/rkisp1/rkisp1_path.h | 3 ++ > > 2 files changed, 49 insertions(+), 2 deletions(-) > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > > index 9053af18..b3b27aa5 100644 > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > > @@ -128,12 +128,56 @@ void RkISP1Path::populateFormats() > > } > > } > > > > +/** > > + * \brief Filter the sensor resolutions that can be supported > > + * \param[in] sensor The camera sensor > > + * > > + * This function retrieves all the sizes supported by the sensor and > > + * filters all the resolutions that can be supported on the pipeline. > > + * It is possible that the sensor's maximum output resolution is higher > > + * than the ISP maximum input. In that case, this function filters out all > > + * the resolution incapable of being supported and returns the maximum > > + * sensor resolution that can be supported by the pipeline. > > + * > > + * \return Maximum sensor size supported on the pipeline > > + */ > > +Size RkISP1Path::filterSensorResolution(const CameraSensor *sensor) > > +{ > > + if (!sensorSizes_.empty()) > > + return sensorSizes_.back(); There are systems based on rkisp1 where multiple sensors are connected to the same ISP. Obviously only one of them can be used at a time, but applications can switch between them. This will be broken by caching sensorSizes_. > I like that we keep the full list of supported sensorSizes_. My first > thought was that this list should be generated when the path is created, > but this also seems fine to me. > > I think there's no need to complicate / split this until there's > actually a user of the full sensorSizes list ? > > I'm a little surprised that's not not used actually. Do we report the > raw modes we support somewhere? Should that iterate the 'sensorSizes_' > as the supported sizes ? > > Perhaps that's an improvement/fix on top though. > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > + > > + std::vector<Size> sensorSizes; > > + const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes(); > > + for (const auto iter : mbusCodes) { > > + std::vector<Size> sizes = sensor->sizes(iter); > > + for (Size sz : sizes) > > + sensorSizes.push_back(sz); > > + } > > + > > + std::sort(sensorSizes.begin(), sensorSizes.end()); > > + > > + /* Remove duplicates. */ > > + auto last = std::unique(sensorSizes.begin(), sensorSizes.end()); > > + sensorSizes.erase(last, sensorSizes.end()); > > + > > + /* Discard any sizes that the pipeline is unable to support. */ > > + for (auto sz : sensorSizes) { > > + if (sz.width > maxResolution_.width || > > + sz.height > maxResolution_.height) > > + continue; > > + > > + sensorSizes_.push_back(sz); > > + } > > + > > + return sensorSizes_.back(); > > +} > > + > > StreamConfiguration > > RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size, > > StreamRole role) > > { > > const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes(); > > - const Size &resolution = sensor->resolution(); > > + Size resolution = filterSensorResolution(sensor); > > > > /* Min and max resolutions to populate the available stream formats. */ > > Size maxResolution = maxResolution_.boundedToAspectRatio(resolution) > > @@ -222,7 +266,7 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor, > > StreamConfiguration *cfg) > > { > > const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes(); > > - const Size &resolution = sensor->resolution(); > > + Size resolution = filterSensorResolution(sensor); > > > > const StreamConfiguration reqCfg = *cfg; > > CameraConfiguration::Status status = CameraConfiguration::Valid; > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > > index 13ba4b62..03ff9543 100644 > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > > @@ -63,6 +63,7 @@ public: > > > > private: > > void populateFormats(); > > + Size filterSensorResolution(const CameraSensor *sensor); > > > > static constexpr unsigned int RKISP1_BUFFER_COUNT = 4; > > > > @@ -77,6 +78,8 @@ private: > > std::unique_ptr<V4L2Subdevice> resizer_; > > std::unique_ptr<V4L2VideoDevice> video_; > > MediaLink *link_; > > + > > + std::vector<Size> sensorSizes_; > > }; > > > > class RkISP1MainPath : public RkISP1Path
Hi Laurent On 12/09/24 3:59 am, Laurent Pinchart wrote: > On Wed, Sep 11, 2024 at 11:52:36AM +0100, Kieran Bingham wrote: >> Quoting Umang Jain (2024-09-09 17:37:19) >>> It is possible that the maximum sensor size (returned by >>> CameraSensor::resolution()) is not supported by the pipeline. In such >>> cases, a filter function is required to filter out resolutions of the >>> camera sensor, which cannot be supported by the pipeline. >>> >>> Introduce the filter function filterSensorResolution() in RkISP1Path >>> class and use it for validate() and generateConfiguration(). The >>> maximum sensor resolution is picked from that filtered set of >>> resolutions instead of CameraSensor::resolution(). >>> >>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >>> --- >>> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 48 ++++++++++++++++++- >>> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 3 ++ >>> 2 files changed, 49 insertions(+), 2 deletions(-) >>> >>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp >>> index 9053af18..b3b27aa5 100644 >>> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp >>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp >>> @@ -128,12 +128,56 @@ void RkISP1Path::populateFormats() >>> } >>> } >>> >>> +/** >>> + * \brief Filter the sensor resolutions that can be supported >>> + * \param[in] sensor The camera sensor >>> + * >>> + * This function retrieves all the sizes supported by the sensor and >>> + * filters all the resolutions that can be supported on the pipeline. >>> + * It is possible that the sensor's maximum output resolution is higher >>> + * than the ISP maximum input. In that case, this function filters out all >>> + * the resolution incapable of being supported and returns the maximum >>> + * sensor resolution that can be supported by the pipeline. >>> + * >>> + * \return Maximum sensor size supported on the pipeline >>> + */ >>> +Size RkISP1Path::filterSensorResolution(const CameraSensor *sensor) >>> +{ >>> + if (!sensorSizes_.empty()) >>> + return sensorSizes_.back(); > There are systems based on rkisp1 where multiple sensors are connected > to the same ISP. Obviously only one of them can be used at a time, but > applications can switch between them. This will be broken by caching > sensorSizes_. Indeed, something that I missed while developing on i.MX8MP (two cameras but dual ISP as well) I'll convert it to a std::map in next revision. >> I like that we keep the full list of supported sensorSizes_. My first >> thought was that this list should be generated when the path is created, >> but this also seems fine to me. >> >> I think there's no need to complicate / split this until there's >> actually a user of the full sensorSizes list ? >> >> I'm a little surprised that's not not used actually. Do we report the >> raw modes we support somewhere? Should that iterate the 'sensorSizes_' >> as the supported sizes ? I will test how raw sizes are reported ? sensorSizes_ is a good candidate to update the raw size list as well. >> >> Perhaps that's an improvement/fix on top though. >> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> >>> + >>> + std::vector<Size> sensorSizes; >>> + const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes(); >>> + for (const auto iter : mbusCodes) { >>> + std::vector<Size> sizes = sensor->sizes(iter); >>> + for (Size sz : sizes) >>> + sensorSizes.push_back(sz); >>> + } >>> + >>> + std::sort(sensorSizes.begin(), sensorSizes.end()); >>> + >>> + /* Remove duplicates. */ >>> + auto last = std::unique(sensorSizes.begin(), sensorSizes.end()); >>> + sensorSizes.erase(last, sensorSizes.end()); >>> + >>> + /* Discard any sizes that the pipeline is unable to support. */ >>> + for (auto sz : sensorSizes) { >>> + if (sz.width > maxResolution_.width || >>> + sz.height > maxResolution_.height) >>> + continue; >>> + >>> + sensorSizes_.push_back(sz); >>> + } >>> + >>> + return sensorSizes_.back(); >>> +} >>> + >>> StreamConfiguration >>> RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size, >>> StreamRole role) >>> { >>> const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes(); >>> - const Size &resolution = sensor->resolution(); >>> + Size resolution = filterSensorResolution(sensor); >>> >>> /* Min and max resolutions to populate the available stream formats. */ >>> Size maxResolution = maxResolution_.boundedToAspectRatio(resolution) >>> @@ -222,7 +266,7 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor, >>> StreamConfiguration *cfg) >>> { >>> const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes(); >>> - const Size &resolution = sensor->resolution(); >>> + Size resolution = filterSensorResolution(sensor); >>> >>> const StreamConfiguration reqCfg = *cfg; >>> CameraConfiguration::Status status = CameraConfiguration::Valid; >>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h >>> index 13ba4b62..03ff9543 100644 >>> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h >>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h >>> @@ -63,6 +63,7 @@ public: >>> >>> private: >>> void populateFormats(); >>> + Size filterSensorResolution(const CameraSensor *sensor); >>> >>> static constexpr unsigned int RKISP1_BUFFER_COUNT = 4; >>> >>> @@ -77,6 +78,8 @@ private: >>> std::unique_ptr<V4L2Subdevice> resizer_; >>> std::unique_ptr<V4L2VideoDevice> video_; >>> MediaLink *link_; >>> + >>> + std::vector<Size> sensorSizes_; >>> }; >>> >>> class RkISP1MainPath : public RkISP1Path
Quoting Umang Jain (2024-09-12 11:29:40) > Hi Laurent > > On 12/09/24 3:59 am, Laurent Pinchart wrote: > > On Wed, Sep 11, 2024 at 11:52:36AM +0100, Kieran Bingham wrote: > >> Quoting Umang Jain (2024-09-09 17:37:19) > >>> It is possible that the maximum sensor size (returned by > >>> CameraSensor::resolution()) is not supported by the pipeline. In such > >>> cases, a filter function is required to filter out resolutions of the > >>> camera sensor, which cannot be supported by the pipeline. > >>> > >>> Introduce the filter function filterSensorResolution() in RkISP1Path > >>> class and use it for validate() and generateConfiguration(). The > >>> maximum sensor resolution is picked from that filtered set of > >>> resolutions instead of CameraSensor::resolution(). > >>> > >>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > >>> --- > >>> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 48 ++++++++++++++++++- > >>> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 3 ++ > >>> 2 files changed, 49 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > >>> index 9053af18..b3b27aa5 100644 > >>> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > >>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > >>> @@ -128,12 +128,56 @@ void RkISP1Path::populateFormats() > >>> } > >>> } > >>> > >>> +/** > >>> + * \brief Filter the sensor resolutions that can be supported > >>> + * \param[in] sensor The camera sensor > >>> + * > >>> + * This function retrieves all the sizes supported by the sensor and > >>> + * filters all the resolutions that can be supported on the pipeline. > >>> + * It is possible that the sensor's maximum output resolution is higher > >>> + * than the ISP maximum input. In that case, this function filters out all > >>> + * the resolution incapable of being supported and returns the maximum > >>> + * sensor resolution that can be supported by the pipeline. > >>> + * > >>> + * \return Maximum sensor size supported on the pipeline > >>> + */ > >>> +Size RkISP1Path::filterSensorResolution(const CameraSensor *sensor) > >>> +{ > >>> + if (!sensorSizes_.empty()) > >>> + return sensorSizes_.back(); > > There are systems based on rkisp1 where multiple sensors are connected > > to the same ISP. Obviously only one of them can be used at a time, but > > applications can switch between them. This will be broken by caching > > sensorSizes_. > > Indeed, something that I missed while developing on i.MX8MP (two cameras > but dual ISP as well) > > I'll convert it to a std::map in next revision. Rather than a map - can we just put this filter/or the sensorSizes_ into the camera data? > > >> I like that we keep the full list of supported sensorSizes_. My first > >> thought was that this list should be generated when the path is created, > >> but this also seems fine to me. > >> > >> I think there's no need to complicate / split this until there's > >> actually a user of the full sensorSizes list ? > >> > >> I'm a little surprised that's not not used actually. Do we report the > >> raw modes we support somewhere? Should that iterate the 'sensorSizes_' > >> as the supported sizes ? > > I will test how raw sizes are reported ? sensorSizes_ is a good > candidate to update the raw size list as well. I'm intrigued on what it's currently reporting... -- Kieran > >> > >> Perhaps that's an improvement/fix on top though. > >> > >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> > >>> + > >>> + std::vector<Size> sensorSizes; > >>> + const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes(); > >>> + for (const auto iter : mbusCodes) { > >>> + std::vector<Size> sizes = sensor->sizes(iter); > >>> + for (Size sz : sizes) > >>> + sensorSizes.push_back(sz); > >>> + } > >>> + > >>> + std::sort(sensorSizes.begin(), sensorSizes.end()); > >>> + > >>> + /* Remove duplicates. */ > >>> + auto last = std::unique(sensorSizes.begin(), sensorSizes.end()); > >>> + sensorSizes.erase(last, sensorSizes.end()); > >>> + > >>> + /* Discard any sizes that the pipeline is unable to support. */ > >>> + for (auto sz : sensorSizes) { > >>> + if (sz.width > maxResolution_.width || > >>> + sz.height > maxResolution_.height) > >>> + continue; > >>> + > >>> + sensorSizes_.push_back(sz); > >>> + } > >>> + > >>> + return sensorSizes_.back(); > >>> +} > >>> + > >>> StreamConfiguration > >>> RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size, > >>> StreamRole role) > >>> { > >>> const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes(); > >>> - const Size &resolution = sensor->resolution(); > >>> + Size resolution = filterSensorResolution(sensor); > >>> > >>> /* Min and max resolutions to populate the available stream formats. */ > >>> Size maxResolution = maxResolution_.boundedToAspectRatio(resolution) > >>> @@ -222,7 +266,7 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor, > >>> StreamConfiguration *cfg) > >>> { > >>> const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes(); > >>> - const Size &resolution = sensor->resolution(); > >>> + Size resolution = filterSensorResolution(sensor); > >>> > >>> const StreamConfiguration reqCfg = *cfg; > >>> CameraConfiguration::Status status = CameraConfiguration::Valid; > >>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > >>> index 13ba4b62..03ff9543 100644 > >>> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h > >>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > >>> @@ -63,6 +63,7 @@ public: > >>> > >>> private: > >>> void populateFormats(); > >>> + Size filterSensorResolution(const CameraSensor *sensor); > >>> > >>> static constexpr unsigned int RKISP1_BUFFER_COUNT = 4; > >>> > >>> @@ -77,6 +78,8 @@ private: > >>> std::unique_ptr<V4L2Subdevice> resizer_; > >>> std::unique_ptr<V4L2VideoDevice> video_; > >>> MediaLink *link_; > >>> + > >>> + std::vector<Size> sensorSizes_; > >>> }; > >>> > >>> class RkISP1MainPath : public RkISP1Path >
On 12/09/24 4:18 pm, Kieran Bingham wrote: > Quoting Umang Jain (2024-09-12 11:29:40) >> Hi Laurent >> >> On 12/09/24 3:59 am, Laurent Pinchart wrote: >>> On Wed, Sep 11, 2024 at 11:52:36AM +0100, Kieran Bingham wrote: >>>> Quoting Umang Jain (2024-09-09 17:37:19) >>>>> It is possible that the maximum sensor size (returned by >>>>> CameraSensor::resolution()) is not supported by the pipeline. In such >>>>> cases, a filter function is required to filter out resolutions of the >>>>> camera sensor, which cannot be supported by the pipeline. >>>>> >>>>> Introduce the filter function filterSensorResolution() in RkISP1Path >>>>> class and use it for validate() and generateConfiguration(). The >>>>> maximum sensor resolution is picked from that filtered set of >>>>> resolutions instead of CameraSensor::resolution(). >>>>> >>>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >>>>> --- >>>>> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 48 ++++++++++++++++++- >>>>> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 3 ++ >>>>> 2 files changed, 49 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp >>>>> index 9053af18..b3b27aa5 100644 >>>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp >>>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp >>>>> @@ -128,12 +128,56 @@ void RkISP1Path::populateFormats() >>>>> } >>>>> } >>>>> >>>>> +/** >>>>> + * \brief Filter the sensor resolutions that can be supported >>>>> + * \param[in] sensor The camera sensor >>>>> + * >>>>> + * This function retrieves all the sizes supported by the sensor and >>>>> + * filters all the resolutions that can be supported on the pipeline. >>>>> + * It is possible that the sensor's maximum output resolution is higher >>>>> + * than the ISP maximum input. In that case, this function filters out all >>>>> + * the resolution incapable of being supported and returns the maximum >>>>> + * sensor resolution that can be supported by the pipeline. >>>>> + * >>>>> + * \return Maximum sensor size supported on the pipeline >>>>> + */ >>>>> +Size RkISP1Path::filterSensorResolution(const CameraSensor *sensor) >>>>> +{ >>>>> + if (!sensorSizes_.empty()) >>>>> + return sensorSizes_.back(); >>> There are systems based on rkisp1 where multiple sensors are connected >>> to the same ISP. Obviously only one of them can be used at a time, but >>> applications can switch between them. This will be broken by caching >>> sensorSizes_. >> Indeed, something that I missed while developing on i.MX8MP (two cameras >> but dual ISP as well) >> >> I'll convert it to a std::map in next revision. > Rather than a map - can we just put this filter/or the sensorSizes_ into > the camera data? hmm, might require passing CameraData into RkISP1Path::validate(...) and RkISP1Path:: generateConfiguration(const CameraSensor *sensor,....) Possibly replacing the CameraSensor function argument in both. > > > >>>> I like that we keep the full list of supported sensorSizes_. My first >>>> thought was that this list should be generated when the path is created, >>>> but this also seems fine to me. >>>> >>>> I think there's no need to complicate / split this until there's >>>> actually a user of the full sensorSizes list ? >>>> >>>> I'm a little surprised that's not not used actually. Do we report the >>>> raw modes we support somewhere? Should that iterate the 'sensorSizes_' >>>> as the supported sizes ? >> I will test how raw sizes are reported ? sensorSizes_ is a good >> candidate to update the raw size list as well. > I'm intrigued on what it's currently reporting... > > -- > Kieran > > >>>> Perhaps that's an improvement/fix on top though. >>>> >>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>>> >>>>> + >>>>> + std::vector<Size> sensorSizes; >>>>> + const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes(); >>>>> + for (const auto iter : mbusCodes) { >>>>> + std::vector<Size> sizes = sensor->sizes(iter); >>>>> + for (Size sz : sizes) >>>>> + sensorSizes.push_back(sz); >>>>> + } >>>>> + >>>>> + std::sort(sensorSizes.begin(), sensorSizes.end()); >>>>> + >>>>> + /* Remove duplicates. */ >>>>> + auto last = std::unique(sensorSizes.begin(), sensorSizes.end()); >>>>> + sensorSizes.erase(last, sensorSizes.end()); >>>>> + >>>>> + /* Discard any sizes that the pipeline is unable to support. */ >>>>> + for (auto sz : sensorSizes) { >>>>> + if (sz.width > maxResolution_.width || >>>>> + sz.height > maxResolution_.height) >>>>> + continue; >>>>> + >>>>> + sensorSizes_.push_back(sz); >>>>> + } >>>>> + >>>>> + return sensorSizes_.back(); >>>>> +} >>>>> + >>>>> StreamConfiguration >>>>> RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size, >>>>> StreamRole role) >>>>> { >>>>> const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes(); >>>>> - const Size &resolution = sensor->resolution(); >>>>> + Size resolution = filterSensorResolution(sensor); >>>>> >>>>> /* Min and max resolutions to populate the available stream formats. */ >>>>> Size maxResolution = maxResolution_.boundedToAspectRatio(resolution) >>>>> @@ -222,7 +266,7 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor, >>>>> StreamConfiguration *cfg) >>>>> { >>>>> const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes(); >>>>> - const Size &resolution = sensor->resolution(); >>>>> + Size resolution = filterSensorResolution(sensor); >>>>> >>>>> const StreamConfiguration reqCfg = *cfg; >>>>> CameraConfiguration::Status status = CameraConfiguration::Valid; >>>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h >>>>> index 13ba4b62..03ff9543 100644 >>>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h >>>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h >>>>> @@ -63,6 +63,7 @@ public: >>>>> >>>>> private: >>>>> void populateFormats(); >>>>> + Size filterSensorResolution(const CameraSensor *sensor); >>>>> >>>>> static constexpr unsigned int RKISP1_BUFFER_COUNT = 4; >>>>> >>>>> @@ -77,6 +78,8 @@ private: >>>>> std::unique_ptr<V4L2Subdevice> resizer_; >>>>> std::unique_ptr<V4L2VideoDevice> video_; >>>>> MediaLink *link_; >>>>> + >>>>> + std::vector<Size> sensorSizes_; >>>>> }; >>>>> >>>>> class RkISP1MainPath : public RkISP1Path
Hello On 12/09/24 4:27 pm, Umang Jain wrote: > > > On 12/09/24 4:18 pm, Kieran Bingham wrote: >> Quoting Umang Jain (2024-09-12 11:29:40) >>> Hi Laurent >>> >>> On 12/09/24 3:59 am, Laurent Pinchart wrote: >>>> On Wed, Sep 11, 2024 at 11:52:36AM +0100, Kieran Bingham wrote: >>>>> Quoting Umang Jain (2024-09-09 17:37:19) >>>>>> It is possible that the maximum sensor size (returned by >>>>>> CameraSensor::resolution()) is not supported by the pipeline. In >>>>>> such >>>>>> cases, a filter function is required to filter out resolutions of >>>>>> the >>>>>> camera sensor, which cannot be supported by the pipeline. >>>>>> >>>>>> Introduce the filter function filterSensorResolution() in RkISP1Path >>>>>> class and use it for validate() and generateConfiguration(). The >>>>>> maximum sensor resolution is picked from that filtered set of >>>>>> resolutions instead of CameraSensor::resolution(). >>>>>> >>>>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >>>>>> --- >>>>>> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 48 >>>>>> ++++++++++++++++++- >>>>>> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 3 ++ >>>>>> 2 files changed, 49 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp >>>>>> b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp >>>>>> index 9053af18..b3b27aa5 100644 >>>>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp >>>>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp >>>>>> @@ -128,12 +128,56 @@ void RkISP1Path::populateFormats() >>>>>> } >>>>>> } >>>>>> +/** >>>>>> + * \brief Filter the sensor resolutions that can be supported >>>>>> + * \param[in] sensor The camera sensor >>>>>> + * >>>>>> + * This function retrieves all the sizes supported by the sensor >>>>>> and >>>>>> + * filters all the resolutions that can be supported on the >>>>>> pipeline. >>>>>> + * It is possible that the sensor's maximum output resolution is >>>>>> higher >>>>>> + * than the ISP maximum input. In that case, this function >>>>>> filters out all >>>>>> + * the resolution incapable of being supported and returns the >>>>>> maximum >>>>>> + * sensor resolution that can be supported by the pipeline. >>>>>> + * >>>>>> + * \return Maximum sensor size supported on the pipeline >>>>>> + */ >>>>>> +Size RkISP1Path::filterSensorResolution(const CameraSensor *sensor) >>>>>> +{ >>>>>> + if (!sensorSizes_.empty()) >>>>>> + return sensorSizes_.back(); >>>> There are systems based on rkisp1 where multiple sensors are connected >>>> to the same ISP. Obviously only one of them can be used at a time, but >>>> applications can switch between them. This will be broken by caching >>>> sensorSizes_. >>> Indeed, something that I missed while developing on i.MX8MP (two >>> cameras >>> but dual ISP as well) >>> >>> I'll convert it to a std::map in next revision. >> Rather than a map - can we just put this filter/or the sensorSizes_ into >> the camera data? > > hmm, might require passing CameraData into RkISP1Path::validate(...) > > and > > RkISP1Path:: generateConfiguration(const CameraSensor *sensor,....) > > Possibly replacing the CameraSensor function argument in both. Doing this seems a bit awkward, given the RkISP1CameraData is defined in rkisp1.cpp and it already has pointers to mainPath and selfPath RkISP1Path. Should I try making RkISP1CameraData shared to path classes for solely this purpose? > >> >> >> >>>>> I like that we keep the full list of supported sensorSizes_. My first >>>>> thought was that this list should be generated when the path is >>>>> created, >>>>> but this also seems fine to me. >>>>> >>>>> I think there's no need to complicate / split this until there's >>>>> actually a user of the full sensorSizes list ? >>>>> >>>>> I'm a little surprised that's not not used actually. Do we report the >>>>> raw modes we support somewhere? Should that iterate the >>>>> 'sensorSizes_' >>>>> as the supported sizes ? >>> I will test how raw sizes are reported ? sensorSizes_ is a good >>> candidate to update the raw size list as well. >> I'm intrigued on what it's currently reporting... It's reporting the max sizes i.e. 5472x3648 After this series is applied, it's reports correct list with 5472x3648 filtered out from the list. Since the raw formats are reported with [format, Size], just iterating over the sensorSize_ is not enough. So probably, the usage of sensorSizes_ is limited for now. >> >> -- >> Kieran >> >> >>>>> Perhaps that's an improvement/fix on top though. >>>>> >>>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>>>> >>>>>> + >>>>>> + std::vector<Size> sensorSizes; >>>>>> + const std::vector<unsigned int> &mbusCodes = >>>>>> sensor->mbusCodes(); >>>>>> + for (const auto iter : mbusCodes) { >>>>>> + std::vector<Size> sizes = sensor->sizes(iter); >>>>>> + for (Size sz : sizes) >>>>>> + sensorSizes.push_back(sz); >>>>>> + } >>>>>> + >>>>>> + std::sort(sensorSizes.begin(), sensorSizes.end()); >>>>>> + >>>>>> + /* Remove duplicates. */ >>>>>> + auto last = std::unique(sensorSizes.begin(), >>>>>> sensorSizes.end()); >>>>>> + sensorSizes.erase(last, sensorSizes.end()); >>>>>> + >>>>>> + /* Discard any sizes that the pipeline is unable to >>>>>> support. */ >>>>>> + for (auto sz : sensorSizes) { >>>>>> + if (sz.width > maxResolution_.width || >>>>>> + sz.height > maxResolution_.height) >>>>>> + continue; >>>>>> + >>>>>> + sensorSizes_.push_back(sz); >>>>>> + } >>>>>> + >>>>>> + return sensorSizes_.back(); >>>>>> +} >>>>>> + >>>>>> StreamConfiguration >>>>>> RkISP1Path::generateConfiguration(const CameraSensor *sensor, >>>>>> const Size &size, >>>>>> StreamRole role) >>>>>> { >>>>>> const std::vector<unsigned int> &mbusCodes = >>>>>> sensor->mbusCodes(); >>>>>> - const Size &resolution = sensor->resolution(); >>>>>> + Size resolution = filterSensorResolution(sensor); >>>>>> /* Min and max resolutions to populate the available >>>>>> stream formats. */ >>>>>> Size maxResolution = >>>>>> maxResolution_.boundedToAspectRatio(resolution) >>>>>> @@ -222,7 +266,7 @@ CameraConfiguration::Status >>>>>> RkISP1Path::validate(const CameraSensor *sensor, >>>>>> StreamConfiguration *cfg) >>>>>> { >>>>>> const std::vector<unsigned int> &mbusCodes = >>>>>> sensor->mbusCodes(); >>>>>> - const Size &resolution = sensor->resolution(); >>>>>> + Size resolution = filterSensorResolution(sensor); >>>>>> const StreamConfiguration reqCfg = *cfg; >>>>>> CameraConfiguration::Status status = >>>>>> CameraConfiguration::Valid; >>>>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h >>>>>> b/src/libcamera/pipeline/rkisp1/rkisp1_path.h >>>>>> index 13ba4b62..03ff9543 100644 >>>>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h >>>>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h >>>>>> @@ -63,6 +63,7 @@ public: >>>>>> private: >>>>>> void populateFormats(); >>>>>> + Size filterSensorResolution(const CameraSensor *sensor); >>>>>> static constexpr unsigned int RKISP1_BUFFER_COUNT = 4; >>>>>> @@ -77,6 +78,8 @@ private: >>>>>> std::unique_ptr<V4L2Subdevice> resizer_; >>>>>> std::unique_ptr<V4L2VideoDevice> video_; >>>>>> MediaLink *link_; >>>>>> + >>>>>> + std::vector<Size> sensorSizes_; >>>>>> }; >>>>>> class RkISP1MainPath : public RkISP1Path >
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp index 9053af18..b3b27aa5 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp @@ -128,12 +128,56 @@ void RkISP1Path::populateFormats() } } +/** + * \brief Filter the sensor resolutions that can be supported + * \param[in] sensor The camera sensor + * + * This function retrieves all the sizes supported by the sensor and + * filters all the resolutions that can be supported on the pipeline. + * It is possible that the sensor's maximum output resolution is higher + * than the ISP maximum input. In that case, this function filters out all + * the resolution incapable of being supported and returns the maximum + * sensor resolution that can be supported by the pipeline. + * + * \return Maximum sensor size supported on the pipeline + */ +Size RkISP1Path::filterSensorResolution(const CameraSensor *sensor) +{ + if (!sensorSizes_.empty()) + return sensorSizes_.back(); + + std::vector<Size> sensorSizes; + const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes(); + for (const auto iter : mbusCodes) { + std::vector<Size> sizes = sensor->sizes(iter); + for (Size sz : sizes) + sensorSizes.push_back(sz); + } + + std::sort(sensorSizes.begin(), sensorSizes.end()); + + /* Remove duplicates. */ + auto last = std::unique(sensorSizes.begin(), sensorSizes.end()); + sensorSizes.erase(last, sensorSizes.end()); + + /* Discard any sizes that the pipeline is unable to support. */ + for (auto sz : sensorSizes) { + if (sz.width > maxResolution_.width || + sz.height > maxResolution_.height) + continue; + + sensorSizes_.push_back(sz); + } + + return sensorSizes_.back(); +} + StreamConfiguration RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size, StreamRole role) { const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes(); - const Size &resolution = sensor->resolution(); + Size resolution = filterSensorResolution(sensor); /* Min and max resolutions to populate the available stream formats. */ Size maxResolution = maxResolution_.boundedToAspectRatio(resolution) @@ -222,7 +266,7 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor, StreamConfiguration *cfg) { const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes(); - const Size &resolution = sensor->resolution(); + Size resolution = filterSensorResolution(sensor); const StreamConfiguration reqCfg = *cfg; CameraConfiguration::Status status = CameraConfiguration::Valid; diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h index 13ba4b62..03ff9543 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h @@ -63,6 +63,7 @@ public: private: void populateFormats(); + Size filterSensorResolution(const CameraSensor *sensor); static constexpr unsigned int RKISP1_BUFFER_COUNT = 4; @@ -77,6 +78,8 @@ private: std::unique_ptr<V4L2Subdevice> resizer_; std::unique_ptr<V4L2VideoDevice> video_; MediaLink *link_; + + std::vector<Size> sensorSizes_; }; class RkISP1MainPath : public RkISP1Path
It is possible that the maximum sensor size (returned by CameraSensor::resolution()) is not supported by the pipeline. In such cases, a filter function is required to filter out resolutions of the camera sensor, which cannot be supported by the pipeline. Introduce the filter function filterSensorResolution() in RkISP1Path class and use it for validate() and generateConfiguration(). The maximum sensor resolution is picked from that filtered set of resolutions instead of CameraSensor::resolution(). Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 48 ++++++++++++++++++- src/libcamera/pipeline/rkisp1/rkisp1_path.h | 3 ++ 2 files changed, 49 insertions(+), 2 deletions(-)