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

12252 diff mbox series

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.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 .../pipeline/raspberrypi/raspberrypi.cpp      | 32 +++++++------------
 1 file changed, 12 insertions(+), 20 deletions(-)

Hi Naush

Thanks for this patch too. I have no particular comments - it seems
like a sensible little tidy-up.

On Wed, 12 May 2021 at 09:47, Naushir Patuck <naush@raspberrypi.com> 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.
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>

I've been testing this patch too along with the previous one, so:

Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
Tested-by: David Plowman <david.plowman@raspberrypi.com>

Thanks!
David

> ---
>  .../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()
> --
> 2.25.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

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


> Signed-off-by: Naushir Patuck <naush@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()
>