[libcamera-devel] [PATCH 2/2] pipeline raspberrypi: Move adding of ScalerCrop to the Request metadata

[Kieran Bingham]

Hi Naush,

On 12/05/2021 09:47, Naushir Patuck wrote:
> With the recent change to merge existing Request metadata with the
> ControlList provided by the IPA in commit fcfb1dc02a6b ("libcamera:
> raspberry: Report sensor timestamp"), we can now write the
> controls::ScalerCrop value at the start of the pipeline instead of at
> the end.
> 
> This change simplifies the logic slightly, and allows us to write all
> metadata items to the Request in one place.

That sounds nicer!

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


> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 32 +++++++------------
>  1 file changed, 12 insertions(+), 20 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index eb6d31670567..0a71325ad7c0 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -140,7 +140,7 @@ public:
>  	RPiCameraData(PipelineHandler *pipe)
>  		: CameraData(pipe), state_(State::Stopped),
>  		  supportsFlips_(false), flipsAlterBayerOrder_(false),
> -		  updateScalerCrop_(true), dropFrameCount_(0), ispOutputCount_(0)
> +		  dropFrameCount_(0), ispOutputCount_(0)
>  	{
>  	}
>  
> @@ -214,7 +214,6 @@ public:
>  	CameraSensorInfo sensorInfo_;
>  	Rectangle ispCrop_; /* crop in ISP (camera mode) pixels */
>  	Rectangle scalerCrop_; /* crop in sensor native pixels */
> -	bool updateScalerCrop_;
>  	Size ispMinCropSize_;
>  
>  	unsigned int dropFrameCount_;
> @@ -1325,23 +1324,7 @@ void RPiCameraData::statsMetadataComplete(uint32_t bufferId, const ControlList &
>  	Request *request = requestQueue_.front();
>  	request->metadata().merge(controls);
>  
> -	/*
> -	 * 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.
> -	 *
> -	 * Sending this information on every frame may be helpful.
> -	 */
> -	if (updateScalerCrop_) {
> -		updateScalerCrop_ = false;
> -		scalerCrop_ = ispCrop_.scaledBy(sensorInfo_.analogCrop.size(),
> -						sensorInfo_.outputSize);
> -		scalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft());
> -	}
> -	request->metadata().set(controls::ScalerCrop, scalerCrop_);
> -
>  	state_ = State::IpaComplete;
> -
>  	handleState();
>  }
>  
> @@ -1673,8 +1656,15 @@ void RPiCameraData::applyScalerCrop(const ControlList &controls)
>  		if (ispCrop != ispCrop_) {
>  			isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &ispCrop);
>  			ispCrop_ = ispCrop;
> -			/* queueFrameAction will have to update its scalerCrop_ */
> -			updateScalerCrop_ = true;
> +
> +			/*
> +			 * 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_ = ispCrop_.scaledBy(sensorInfo_.analogCrop.size(),
> +							sensorInfo_.outputSize);
> +			scalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft());
>  		}
>  	}
>  }
> @@ -1684,6 +1674,8 @@ void RPiCameraData::fillRequestMetadata(const ControlList &bufferControls,
>  {
>  	request->metadata().set(controls::SensorTimestamp,
>  				bufferControls.get(controls::SensorTimestamp));
> +
> +	request->metadata().set(controls::ScalerCrop, scalerCrop_);
>  }
>  
>  void RPiCameraData::tryRunPipeline()
> 

-- 
Regards
--
Kieran