[PATCH v2 1/2] libcamera rkisp1: Reduce scope of sensor max resolution variable

[Kieran Bingham]

Quoting Umang Jain (2024-10-03 14:57:39)
> Reduce the scope of sensor's max resolution variable in
> RkISP1Path::validate(), as it is only required to constraint the
> maximum and minimum resolution bounds on the non-RAW code path.
> 
> No functional changes intended in this patch.
> 
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>


Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> ---
>  src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> index cb6117b9..e7f1f12a 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> @@ -254,7 +254,6 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,
>                                                  StreamConfiguration *cfg)
>  {
>         const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes();
> -       Size resolution = filterSensorResolution(sensor);
>  
>         const StreamConfiguration reqCfg = *cfg;
>         CameraConfiguration::Status status = CameraConfiguration::Valid;
> @@ -322,6 +321,8 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,

But in here - I do wonder if this is another location that should be
restricted to the limits of the ISP.

Bringing in surrounding context:

        if (isRaw) {
                /*
                 * Use the sensor output size closest to the requested stream
                 * size.
                 */
                uint32_t mbusCode = formatToMediaBus.at(cfg->pixelFormat);
                V4L2SubdeviceFormat sensorFormat =
                        sensor->getFormat({ mbusCode }, cfg->size);


I wonder if that section in (isRaw) also needs to be clamped to the
filteredSensorResolution, or should only pick the closest fit from the
list of modes available in the list that got filtered...

So I think moving this line has only highlighted to me that we've missed
a bit.

No objection to still merging this anyway - but I think something might
need to be adapted in here.



>                 minResolution = sensorFormat.size;
>                 maxResolution = sensorFormat.size;
>         } else {
> +               Size resolution = filterSensorResolution(sensor);
> +
>                 /*
>                  * Adjust the size based on the sensor resolution and absolute
>                  * limits of the ISP.
> -- 
> 2.45.2
>