| Message ID | 20240926133236.98137-1-umang.jain@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang On Thu, Sep 26, 2024 at 07:02:36PM GMT, Umang Jain wrote: > 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> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > Changes v3: > - None, just a resent over latest master > - Split out from [v2,0/2] pipeline: rkisp1: Filter out sensor sizes not supported by the pipeline > so that, this can make independent progress. > --- > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 51 ++++++++++++++++++- > src/libcamera/pipeline/rkisp1/rkisp1_path.h | 8 +++ > 2 files changed, 57 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > index c49017d1..2421d06c 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > @@ -126,12 +126,59 @@ 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) > +{ > + auto iter = sensorSizesMap_.find(sensor); > + if (iter != sensorSizesMap_.end() && !iter->second.empty()) > + return iter->second.back(); > + > + sensorSizesMap_.emplace(sensor, std::vector<Size>()); > + > + std::vector<Size> sensorSizes; > + const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes(); > + for (const auto it : mbusCodes) { > + std::vector<Size> sizes = sensor->sizes(it); > + for (Size sz : sizes) Can't you filter out sizes that are larger than max sizes ? So you can populate sensorSizesMap_.second directly avoiding copies ? > + sensorSizes.push_back(sz); > + } Can't we build this map once at populateFormats() time and solely return the max supported size when needed ? > + > + 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; > + > + sensorSizesMap_[sensor].push_back(sz); > + } > + > + return sensorSizesMap_[sensor].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) > @@ -220,7 +267,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 08edefec..9f75fe1f 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > @@ -7,6 +7,7 @@ > > #pragma once > > +#include <map> > #include <memory> > #include <set> > #include <vector> > @@ -63,6 +64,7 @@ public: > > private: > void populateFormats(); > + Size filterSensorResolution(const CameraSensor *sensor); > > static constexpr unsigned int RKISP1_BUFFER_COUNT = 4; > > @@ -77,6 +79,12 @@ private: > std::unique_ptr<V4L2Subdevice> resizer_; > std::unique_ptr<V4L2VideoDevice> video_; > MediaLink *link_; > + > + /* > + * Map from camera sensors to the sizes (in increasing order), > + * which are guaranteed to be supported by the pipeline. > + */ > + std::map<const CameraSensor *, std::vector<Size>> sensorSizesMap_; > }; > > class RkISP1MainPath : public RkISP1Path > -- > 2.45.0 >
Hi Jacopo, On 26/09/24 7:41 pm, Jacopo Mondi wrote: > Hi Umang > > On Thu, Sep 26, 2024 at 07:02:36PM GMT, Umang Jain wrote: >> 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> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> --- >> Changes v3: >> - None, just a resent over latest master >> - Split out from [v2,0/2] pipeline: rkisp1: Filter out sensor sizes not supported by the pipeline >> so that, this can make independent progress. >> --- >> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 51 ++++++++++++++++++- >> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 8 +++ >> 2 files changed, 57 insertions(+), 2 deletions(-) >> >> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp >> index c49017d1..2421d06c 100644 >> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp >> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp >> @@ -126,12 +126,59 @@ 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) >> +{ >> + auto iter = sensorSizesMap_.find(sensor); >> + if (iter != sensorSizesMap_.end() && !iter->second.empty()) >> + return iter->second.back(); >> + >> + sensorSizesMap_.emplace(sensor, std::vector<Size>()); >> + >> + std::vector<Size> sensorSizes; >> + const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes(); >> + for (const auto it : mbusCodes) { >> + std::vector<Size> sizes = sensor->sizes(it); >> + for (Size sz : sizes) > Can't you filter out sizes that are larger than max sizes ? > > So you can populate sensorSizesMap_.second directly avoiding copies ? If I try to directly populate sensorSizesMap_ with the max size filter, it might have duplicated sizes entries (same size for different codes). That's why, I opted a temporary vector sensorSizes here, is used to collect all sizes for the formats that the sensor supports. Then later sorted, de-duplicated and filtered, before populating the sensorSizeMap_.second > >> + sensorSizes.push_back(sz); >> + } > Can't we build this map once at populateFormats() time and solely The map is only built once. Checkout the early return at the start of the function > return the max supported size when needed ? I don't think you can do at populateFormats() as it is called in init() And RkISP1Path::init() is called in match() for RkISP1 pipeline handler, before the camera is even created. > >> + >> + 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; >> + >> + sensorSizesMap_[sensor].push_back(sz); >> + } >> + >> + return sensorSizesMap_[sensor].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) >> @@ -220,7 +267,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 08edefec..9f75fe1f 100644 >> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h >> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h >> @@ -7,6 +7,7 @@ >> >> #pragma once >> >> +#include <map> >> #include <memory> >> #include <set> >> #include <vector> >> @@ -63,6 +64,7 @@ public: >> >> private: >> void populateFormats(); >> + Size filterSensorResolution(const CameraSensor *sensor); >> >> static constexpr unsigned int RKISP1_BUFFER_COUNT = 4; >> >> @@ -77,6 +79,12 @@ private: >> std::unique_ptr<V4L2Subdevice> resizer_; >> std::unique_ptr<V4L2VideoDevice> video_; >> MediaLink *link_; >> + >> + /* >> + * Map from camera sensors to the sizes (in increasing order), >> + * which are guaranteed to be supported by the pipeline. >> + */ >> + std::map<const CameraSensor *, std::vector<Size>> sensorSizesMap_; >> }; >> >> class RkISP1MainPath : public RkISP1Path >> -- >> 2.45.0 >>
Hello Umang On Thu, Sep 26, 2024 at 09:12:49PM GMT, Umang Jain wrote: > Hi Jacopo, > > On 26/09/24 7:41 pm, Jacopo Mondi wrote: > > Hi Umang > > > > On Thu, Sep 26, 2024 at 07:02:36PM GMT, Umang Jain wrote: > > > 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> > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > --- > > > Changes v3: > > > - None, just a resent over latest master > > > - Split out from [v2,0/2] pipeline: rkisp1: Filter out sensor sizes not supported by the pipeline > > > so that, this can make independent progress. > > > --- > > > src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 51 ++++++++++++++++++- > > > src/libcamera/pipeline/rkisp1/rkisp1_path.h | 8 +++ > > > 2 files changed, 57 insertions(+), 2 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > > > index c49017d1..2421d06c 100644 > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp > > > @@ -126,12 +126,59 @@ 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) > > > +{ > > > + auto iter = sensorSizesMap_.find(sensor); > > > + if (iter != sensorSizesMap_.end() && !iter->second.empty()) Can iter->second.empty() happen ? > > > + return iter->second.back(); > > > + > > > + sensorSizesMap_.emplace(sensor, std::vector<Size>()); > > > + > > > + std::vector<Size> sensorSizes; > > > + const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes(); > > > + for (const auto it : mbusCodes) { > > > + std::vector<Size> sizes = sensor->sizes(it); > > > + for (Size sz : sizes) > > Can't you filter out sizes that are larger than max sizes ? > > > > So you can populate sensorSizesMap_.second directly avoiding copies ? > > If I try to directly populate sensorSizesMap_ with the max size filter, it > might have duplicated sizes entries (same size for different codes). > > That's why, I opted a temporary vector sensorSizes here, is used to collect > all sizes for the formats that the sensor supports. > > Then later sorted, de-duplicated and filtered, before populating the > sensorSizeMap_.second > I think you can do the same filtering/sorting directly on the vector inside the map already. What I mean is something like Size RkISP1Path::filterSensorResolution(const CameraSensor *sensor) { auto iter = sensorSizesMap_.find(sensor); if (iter != sensorSizesMap_.end()) return iter->second.back(); std::vector<Size> &sizes = sensorSizesMap_[sensor]; for (unsigned int code : sensor->mbusCodes()) { for (const Size &size : sensor->sizes(code)) { if (size.width > maxResolution_.width || size.height > maxResolution_.height) continue; sizes.push_back(size); } } /* Sort in increasing order and remove duplicates. */ std::sort(sizes.begin(), sizes.end()); auto last = std::unique(sizes.begin(), sizes.end()); sizes.erase(last, sizes.end()); return sizes.back(); } warning: compile tested only Do you think we need the list of supported, unique and sorted formats anywhere else ? Otherwise you could just store the max res and not the whole vector. I won't bikeshed too much on the name, but even if this function indeed does filter the sensor's resolutions, what it does is to return the maxium sensor resolution supported by the pipeline, so I would have gone for something like RkISP1Path::maxResolution(sensor *). Up to you. Thanks j > > > > > + sensorSizes.push_back(sz); > > > + } > > Can't we build this map once at populateFormats() time and solely > > The map is only built once. Checkout the early return at the start of the > function > > return the max supported size when needed ? > > I don't think you can do at populateFormats() as it is called in init() > > And RkISP1Path::init() is called in match() for RkISP1 pipeline handler, > before the camera is even created. > > Right, at init() time you don't have a sensor to operate with, fine then! > > > + > > > + 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; > > > + > > > + sensorSizesMap_[sensor].push_back(sz); > > > + } > > > + > > > + return sensorSizesMap_[sensor].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) > > > @@ -220,7 +267,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 08edefec..9f75fe1f 100644 > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h > > > @@ -7,6 +7,7 @@ > > > > > > #pragma once > > > > > > +#include <map> > > > #include <memory> > > > #include <set> > > > #include <vector> > > > @@ -63,6 +64,7 @@ public: > > > > > > private: > > > void populateFormats(); > > > + Size filterSensorResolution(const CameraSensor *sensor); > > > > > > static constexpr unsigned int RKISP1_BUFFER_COUNT = 4; > > > > > > @@ -77,6 +79,12 @@ private: > > > std::unique_ptr<V4L2Subdevice> resizer_; > > > std::unique_ptr<V4L2VideoDevice> video_; > > > MediaLink *link_; > > > + > > > + /* > > > + * Map from camera sensors to the sizes (in increasing order), > > > + * which are guaranteed to be supported by the pipeline. > > > + */ > > > + std::map<const CameraSensor *, std::vector<Size>> sensorSizesMap_; > > > }; > > > > > > class RkISP1MainPath : public RkISP1Path > > > -- > > > 2.45.0 > > > >
Hi Jacopo On 27/09/24 12:55 pm, Jacopo Mondi wrote: > Hello Umang > > On Thu, Sep 26, 2024 at 09:12:49PM GMT, Umang Jain wrote: >> Hi Jacopo, >> >> On 26/09/24 7:41 pm, Jacopo Mondi wrote: >>> Hi Umang >>> >>> On Thu, Sep 26, 2024 at 07:02:36PM GMT, Umang Jain wrote: >>>> 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> >>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>>> Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>>> --- >>>> Changes v3: >>>> - None, just a resent over latest master >>>> - Split out from [v2,0/2] pipeline: rkisp1: Filter out sensor sizes not supported by the pipeline >>>> so that, this can make independent progress. >>>> --- >>>> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 51 ++++++++++++++++++- >>>> src/libcamera/pipeline/rkisp1/rkisp1_path.h | 8 +++ >>>> 2 files changed, 57 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp >>>> index c49017d1..2421d06c 100644 >>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp >>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp >>>> @@ -126,12 +126,59 @@ 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) >>>> +{ >>>> + auto iter = sensorSizesMap_.find(sensor); >>>> + if (iter != sensorSizesMap_.end() && !iter->second.empty()) > Can iter->second.empty() happen ? Probably not (I believe). > >>>> + return iter->second.back(); >>>> + >>>> + sensorSizesMap_.emplace(sensor, std::vector<Size>()); >>>> + >>>> + std::vector<Size> sensorSizes; >>>> + const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes(); >>>> + for (const auto it : mbusCodes) { >>>> + std::vector<Size> sizes = sensor->sizes(it); >>>> + for (Size sz : sizes) >>> Can't you filter out sizes that are larger than max sizes ? >>> >>> So you can populate sensorSizesMap_.second directly avoiding copies ? >> If I try to directly populate sensorSizesMap_ with the max size filter, it >> might have duplicated sizes entries (same size for different codes). >> >> That's why, I opted a temporary vector sensorSizes here, is used to collect >> all sizes for the formats that the sensor supports. >> >> Then later sorted, de-duplicated and filtered, before populating the >> sensorSizeMap_.second >> > I think you can do the same filtering/sorting directly on the vector > inside the map already. > > What I mean is something like > > Size RkISP1Path::filterSensorResolution(const CameraSensor *sensor) > { > auto iter = sensorSizesMap_.find(sensor); > if (iter != sensorSizesMap_.end()) > return iter->second.back(); > > std::vector<Size> &sizes = sensorSizesMap_[sensor]; > for (unsigned int code : sensor->mbusCodes()) { > for (const Size &size : sensor->sizes(code)) { > if (size.width > maxResolution_.width || > size.height > maxResolution_.height) > continue; > > sizes.push_back(size); > } > } > > /* Sort in increasing order and remove duplicates. */ > std::sort(sizes.begin(), sizes.end()); > auto last = std::unique(sizes.begin(), sizes.end()); > sizes.erase(last, sizes.end()); > > return sizes.back(); > } > > warning: compile tested only > > Do you think we need the list of supported, unique and sorted formats > anywhere else ? Otherwise you could just store the max res and not the > whole vector. It's something on my mind lately, that should be keep the whole vector, or just max filtered out sensor resolution. There might be some value in keeping all the supported sensor resolution on the pipeline but again, there should be a use case for it, which I haven't found at the moment. > > I won't bikeshed too much on the name, but even if this function > indeed does filter the sensor's resolutions, what it does is to return > the maxium sensor resolution supported by the pipeline, so I would > have gone for something like RkISP1Path::maxResolution(sensor *). Up > to you. If we ditch the vector and just focus on sensor max resolution, your suggestion will be applied. Thanks, Umang. > > Thanks > j > >>>> + sensorSizes.push_back(sz); >>>> + } >>> Can't we build this map once at populateFormats() time and solely >> The map is only built once. Checkout the early return at the start of the >> function >>> return the max supported size when needed ? >> I don't think you can do at populateFormats() as it is called in init() >> >> And RkISP1Path::init() is called in match() for RkISP1 pipeline handler, >> before the camera is even created. > Right, at init() time you don't have a sensor to operate with, fine > then! > >>>> + >>>> + 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; >>>> + >>>> + sensorSizesMap_[sensor].push_back(sz); >>>> + } >>>> + >>>> + return sensorSizesMap_[sensor].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) >>>> @@ -220,7 +267,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 08edefec..9f75fe1f 100644 >>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h >>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h >>>> @@ -7,6 +7,7 @@ >>>> >>>> #pragma once >>>> >>>> +#include <map> >>>> #include <memory> >>>> #include <set> >>>> #include <vector> >>>> @@ -63,6 +64,7 @@ public: >>>> >>>> private: >>>> void populateFormats(); >>>> + Size filterSensorResolution(const CameraSensor *sensor); >>>> >>>> static constexpr unsigned int RKISP1_BUFFER_COUNT = 4; >>>> >>>> @@ -77,6 +79,12 @@ private: >>>> std::unique_ptr<V4L2Subdevice> resizer_; >>>> std::unique_ptr<V4L2VideoDevice> video_; >>>> MediaLink *link_; >>>> + >>>> + /* >>>> + * Map from camera sensors to the sizes (in increasing order), >>>> + * which are guaranteed to be supported by the pipeline. >>>> + */ >>>> + std::map<const CameraSensor *, std::vector<Size>> sensorSizesMap_; >>>> }; >>>> >>>> class RkISP1MainPath : public RkISP1Path >>>> -- >>>> 2.45.0 >>>>
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp index c49017d1..2421d06c 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp @@ -126,12 +126,59 @@ 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) +{ + auto iter = sensorSizesMap_.find(sensor); + if (iter != sensorSizesMap_.end() && !iter->second.empty()) + return iter->second.back(); + + sensorSizesMap_.emplace(sensor, std::vector<Size>()); + + std::vector<Size> sensorSizes; + const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes(); + for (const auto it : mbusCodes) { + std::vector<Size> sizes = sensor->sizes(it); + 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; + + sensorSizesMap_[sensor].push_back(sz); + } + + return sensorSizesMap_[sensor].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) @@ -220,7 +267,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 08edefec..9f75fe1f 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h @@ -7,6 +7,7 @@ #pragma once +#include <map> #include <memory> #include <set> #include <vector> @@ -63,6 +64,7 @@ public: private: void populateFormats(); + Size filterSensorResolution(const CameraSensor *sensor); static constexpr unsigned int RKISP1_BUFFER_COUNT = 4; @@ -77,6 +79,12 @@ private: std::unique_ptr<V4L2Subdevice> resizer_; std::unique_ptr<V4L2VideoDevice> video_; MediaLink *link_; + + /* + * Map from camera sensors to the sizes (in increasing order), + * which are guaranteed to be supported by the pipeline. + */ + std::map<const CameraSensor *, std::vector<Size>> sensorSizesMap_; }; class RkISP1MainPath : public RkISP1Path