[PATCH 2/2] pipeline: rkisp1: Filter out sensor sizes not supported by the pipeline

[Kieran Bingham]

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 at 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 at 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
>