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

[Umang Jain]

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.

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