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

21498 diff mbox series

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@ideasonboard.com>
---
 src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

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@ideasonboard.com>


Reviewed-by: Kieran Bingham <kieran.bingham@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
>

Hi Kieran

On 04/10/24 3:15 am, Kieran Bingham wrote:
> 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@ideasonboard.com>
>
> Reviewed-by: Kieran Bingham <kieran.bingham@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.

We have and now there is a fix:

[PATCH] libcamera: rkisp1: Clamp stream configuration to ISP limit on 
raw path

We missed the clamping because it didn't existed in the first place 
when  I wrote

https://git.libcamera.org/libcamera/libcamera.git/commit/?id=761545407c7666063f4c98d92a17af2a2c790b08

But I should have noticed :-/

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