[PATCH v1 2/7] pipeline: rpi: Remove CameraData::scalerCrop_

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Thu, Aug 08, 2024 at 11:23:41AM +0100, Naushir Patuck wrote:
> Do not cache the scalerCrop_ parameter. The cached value is used to
> update the request metadata, but since this is not an expensive
> operation (and can only occur once per frame), caching it is of limited
> value.
> 
> This will simplify logic in a future commit where we can specify a
> crop per-output stream.
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> ---
>  .../pipeline/rpi/common/pipeline_base.cpp       | 17 +++--------------
>  .../pipeline/rpi/common/pipeline_base.h         |  1 -
>  2 files changed, 3 insertions(+), 15 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> index 3041fd1ed9fd..5322fd798a36 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> @@ -544,12 +544,6 @@ int PipelineHandlerBase::configure(Camera *camera, CameraConfiguration *config)
>  		return ret;
>  	}
>  
> -	/*
> -	 * Set the scaler crop to the value we are using (scaled to native sensor
> -	 * coordinates).
> -	 */
> -	data->scalerCrop_ = data->scaleIspCrop(data->ispCrop_);
> -
>  	/*
>  	 * Update the ScalerCropMaximum to the correct value for this camera mode.
>  	 * For us, it's the same as the "analogue crop".
> @@ -569,7 +563,8 @@ int PipelineHandlerBase::configure(Camera *camera, CameraConfiguration *config)
>  
>  	/* Add the ScalerCrop control limits based on the current mode. */
>  	Rectangle ispMinCrop = data->scaleIspCrop(Rectangle(data->ispMinCropSize_));
> -	ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, data->sensorInfo_.analogCrop, data->scalerCrop_);
> +	ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, data->sensorInfo_.analogCrop,
> +						     data->scaleIspCrop(data->ispCrop_));
>  
>  	data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap());
>  
> @@ -1321,12 +1316,6 @@ void CameraData::applyScalerCrop(const ControlList &controls)
>  			ispCrop_ = ispCrop;
>  			platformSetIspCrop();
>  
> -			/*
> -			 * Also update the ScalerCrop in the metadata with what we actually
> -			 * used. But we must first rescale that from ISP (camera mode) pixels
> -			 * back into sensor native pixels.
> -			 */
> -			scalerCrop_ = scaleIspCrop(ispCrop_);
>  		}
>  	}
>  }
> @@ -1483,7 +1472,7 @@ void CameraData::fillRequestMetadata(const ControlList &bufferControls, Request
>  	request->metadata().set(controls::SensorTimestamp,
>  				bufferControls.get(controls::SensorTimestamp).value_or(0));
>  
> -	request->metadata().set(controls::ScalerCrop, scalerCrop_);
> +	request->metadata().set(controls::ScalerCrop, scaleIspCrop(ispCrop_));
>  }
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> index f9cecf70f179..5161c16e518f 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> @@ -134,7 +134,6 @@ public:
>  	/* For handling digital zoom. */
>  	IPACameraSensorInfo sensorInfo_;
>  	Rectangle ispCrop_; /* crop in ISP (camera mode) pixels */
> -	Rectangle scalerCrop_; /* crop in sensor native pixels */
>  	Size ispMinCropSize_;
>  
>  	unsigned int dropFrameCount_;

-- 
Regards,

Laurent Pinchart