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

[Kieran Bingham]

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 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();
> > 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 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
>