[v2,4/7] pipeline: rpi: Introduce CameraData::CropParams
diff mbox series

Message ID 20240930141415.8857-5-naush@raspberrypi.com
State Superseded
Headers show
Series
  • Raspberry Pi: Add controls::rpi::ScalerCrops
Related show

Commit Message

Naushir Patuck Sept. 30, 2024, 2:14 p.m. UTC 
In preparation for assigning separate crop windows for each stream, add
a new CropParams structure that stores the existing ispCrop_ and
ispMinCropSize_ as fields. Use a new std::map to store a CropParams
structure where the map key is the index of the stream configuration in
the CameraConfiguration vector.

At preset, only a single CropParams structure will be set at key == 0 to
preserve the existing crop handling logic.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 .../pipeline/rpi/common/pipeline_base.cpp     | 28 +++++++++++++------
 .../pipeline/rpi/common/pipeline_base.h       | 21 ++++++++++++--
 src/libcamera/pipeline/rpi/vc4/vc4.cpp        | 12 ++++++--
 3 files changed, 47 insertions(+), 14 deletions(-)

Comments

Jacopo Mondi Oct. 1, 2024, 6:26 a.m. UTC | #1 
Hi Naush

On Mon, Sep 30, 2024 at 03:14:12PM GMT, Naushir Patuck wrote:
> In preparation for assigning separate crop windows for each stream, add
> a new CropParams structure that stores the existing ispCrop_ and
> ispMinCropSize_ as fields. Use a new std::map to store a CropParams
> structure where the map key is the index of the stream configuration in
> the CameraConfiguration vector.
>
> At preset, only a single CropParams structure will be set at key == 0 to
> preserve the existing crop handling logic.
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  .../pipeline/rpi/common/pipeline_base.cpp     | 28 +++++++++++++------
>  .../pipeline/rpi/common/pipeline_base.h       | 21 ++++++++++++--
>  src/libcamera/pipeline/rpi/vc4/vc4.cpp        | 12 ++++++--
>  3 files changed, 47 insertions(+), 14 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> index 2de6111bacfd..220c7b962280 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> @@ -533,6 +533,7 @@ int PipelineHandlerBase::configure(Camera *camera, CameraConfiguration *config)
>  	 * Platform specific internal stream configuration. This also assigns
>  	 * external streams which get configured below.
>  	 */
> +	data->cropParams_.clear();
>  	ret = data->platformConfigure(rpiConfig);
>  	if (ret)
>  		return ret;
> @@ -561,10 +562,14 @@ int PipelineHandlerBase::configure(Camera *camera, CameraConfiguration *config)
>  	for (auto const &c : result.controlInfo)
>  		ctrlMap.emplace(c.first, c.second);
>
> -	/* 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->scaleIspCrop(data->ispCrop_));
> +	const auto cropParamsIt = data->cropParams_.find(0);
> +	if (cropParamsIt != data->cropParams_.end()) {
> +		const CameraData::CropParams &cropParams = cropParamsIt->second;
> +		/* Add the ScalerCrop control limits based on the current mode. */
> +		Rectangle ispMinCrop = data->scaleIspCrop(Rectangle(cropParams.ispMinCropSize));
> +		ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, data->sensorInfo_.analogCrop,
> +							     data->scaleIspCrop(cropParams.ispCrop));
> +	}
>
>  	data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap());
>
> @@ -1291,8 +1296,10 @@ Rectangle CameraData::scaleIspCrop(const Rectangle &ispCrop) const
>  void CameraData::applyScalerCrop(const ControlList &controls)
>  {
>  	const auto &scalerCrop = controls.get<Rectangle>(controls::ScalerCrop);
> -	if (scalerCrop) {
> +	const auto cropParamsIt = cropParams_.find(0);
> +	if (scalerCrop && cropParamsIt != cropParams_.end()) {
>  		Rectangle nativeCrop = *scalerCrop;
> +		CropParams &cropParams = cropParamsIt->second;
>
>  		if (!nativeCrop.width || !nativeCrop.height)
>  			nativeCrop = { 0, 0, 1, 1 };
> @@ -1308,12 +1315,12 @@ void CameraData::applyScalerCrop(const ControlList &controls)
>  		 * 2. With the same mid-point, if possible.
>  		 * 3. But it can't go outside the sensor area.
>  		 */
> -		Size minSize = ispMinCropSize_.expandedToAspectRatio(nativeCrop.size());
> +		Size minSize = cropParams.ispMinCropSize.expandedToAspectRatio(nativeCrop.size());
>  		Size size = ispCrop.size().expandedTo(minSize);
>  		ispCrop = size.centeredTo(ispCrop.center()).enclosedIn(Rectangle(sensorInfo_.outputSize));
>
> -		if (ispCrop != ispCrop_) {
> -			ispCrop_ = ispCrop;
> +		if (ispCrop != cropParams.ispCrop) {
> +			cropParams.ispCrop = ispCrop;
>  			platformSetIspCrop(ispCrop);

With the question from the previous patch clarified

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

>  		}
>  	}
> @@ -1471,7 +1478,10 @@ void CameraData::fillRequestMetadata(const ControlList &bufferControls, Request
>  	request->metadata().set(controls::SensorTimestamp,
>  				bufferControls.get(controls::SensorTimestamp).value_or(0));
>
> -	request->metadata().set(controls::ScalerCrop, scaleIspCrop(ispCrop_));
> +	const auto cropParamsIt = cropParams_.find(0);
> +	if (cropParamsIt != cropParams_.end())
> +		request->metadata().set(controls::ScalerCrop,
> +					scaleIspCrop(cropParamsIt->second.ispCrop));
>  }
>
>  } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> index d65b695c30b5..75babbe17f31 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> @@ -133,8 +133,25 @@ public:
>
>  	/* For handling digital zoom. */
>  	IPACameraSensorInfo sensorInfo_;
> -	Rectangle ispCrop_; /* crop in ISP (camera mode) pixels */
> -	Size ispMinCropSize_;
> +
> +	struct CropParams {
> +		CropParams(Rectangle ispCrop_, Size ispMinCropSize_)
> +			: ispCrop(ispCrop_), ispMinCropSize(ispMinCropSize_)
> +		{
> +		}
> +
> +		CropParams()
> +		{
> +		}
> +
> +		/* Crop in ISP (camera mode) pixels */
> +		Rectangle ispCrop;
> +		/* Minimum crop size in ISP output pixels */
> +		Size ispMinCropSize;
> +	};
> +
> +	/* Mapping of CropParams keyed by the output stream order in CameraConfiguration */
> +	std::map<unsigned int, CropParams> cropParams_;
>
>  	unsigned int dropFrameCount_;
>
> diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> index 0ea032293bc9..8080f55a1cf4 100644
> --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> @@ -702,13 +702,19 @@ int Vc4CameraData::platformConfigure(const RPi::RPiCameraConfiguration *rpiConfi
>  	/* Figure out the smallest selection the ISP will allow. */
>  	Rectangle testCrop(0, 0, 1, 1);
>  	isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &testCrop);
> -	ispMinCropSize_ = testCrop.size();
>
>  	/* Adjust aspect ratio by providing crops on the input image. */
>  	Size size = unicamFormat.size.boundedToAspectRatio(maxSize);
> -	ispCrop_ = size.centeredTo(Rectangle(unicamFormat.size).center());
> +	Rectangle ispCrop = size.centeredTo(Rectangle(unicamFormat.size).center());
>
> -	platformSetIspCrop(ispCrop_);
> +	platformSetIspCrop(ispCrop);
> +	/*
> +	 * Set the scaler crop to the value we are using (scaled to native sensor
> +	 * coordinates).
> +	 */
> +	cropParams_.emplace(std::piecewise_construct,
> +			    std::forward_as_tuple(0),
> +			    std::forward_as_tuple(ispCrop, testCrop.size()));
>
>  	return 0;
>  }
> --
> 2.34.1
>
Jacopo Mondi Oct. 1, 2024, 6:30 a.m. UTC | #2 
Hi Naush,
  one more nit

On Mon, Sep 30, 2024 at 03:14:12PM GMT, Naushir Patuck wrote:
> In preparation for assigning separate crop windows for each stream, add
> a new CropParams structure that stores the existing ispCrop_ and
> ispMinCropSize_ as fields. Use a new std::map to store a CropParams
> structure where the map key is the index of the stream configuration in
> the CameraConfiguration vector.
>
> At preset, only a single CropParams structure will be set at key == 0 to
> preserve the existing crop handling logic.
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  .../pipeline/rpi/common/pipeline_base.cpp     | 28 +++++++++++++------
>  .../pipeline/rpi/common/pipeline_base.h       | 21 ++++++++++++--
>  src/libcamera/pipeline/rpi/vc4/vc4.cpp        | 12 ++++++--
>  3 files changed, 47 insertions(+), 14 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> index 2de6111bacfd..220c7b962280 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> @@ -533,6 +533,7 @@ int PipelineHandlerBase::configure(Camera *camera, CameraConfiguration *config)
>  	 * Platform specific internal stream configuration. This also assigns
>  	 * external streams which get configured below.
>  	 */
> +	data->cropParams_.clear();
>  	ret = data->platformConfigure(rpiConfig);
>  	if (ret)
>  		return ret;
> @@ -561,10 +562,14 @@ int PipelineHandlerBase::configure(Camera *camera, CameraConfiguration *config)
>  	for (auto const &c : result.controlInfo)
>  		ctrlMap.emplace(c.first, c.second);
>
> -	/* 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->scaleIspCrop(data->ispCrop_));
> +	const auto cropParamsIt = data->cropParams_.find(0);
> +	if (cropParamsIt != data->cropParams_.end()) {
> +		const CameraData::CropParams &cropParams = cropParamsIt->second;
> +		/* Add the ScalerCrop control limits based on the current mode. */
> +		Rectangle ispMinCrop = data->scaleIspCrop(Rectangle(cropParams.ispMinCropSize));
> +		ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, data->sensorInfo_.analogCrop,
> +							     data->scaleIspCrop(cropParams.ispCrop));
> +	}
>
>  	data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap());
>
> @@ -1291,8 +1296,10 @@ Rectangle CameraData::scaleIspCrop(const Rectangle &ispCrop) const
>  void CameraData::applyScalerCrop(const ControlList &controls)
>  {
>  	const auto &scalerCrop = controls.get<Rectangle>(controls::ScalerCrop);
> -	if (scalerCrop) {
> +	const auto cropParamsIt = cropParams_.find(0);
> +	if (scalerCrop && cropParamsIt != cropParams_.end()) {
>  		Rectangle nativeCrop = *scalerCrop;
> +		CropParams &cropParams = cropParamsIt->second;
>
>  		if (!nativeCrop.width || !nativeCrop.height)
>  			nativeCrop = { 0, 0, 1, 1 };
> @@ -1308,12 +1315,12 @@ void CameraData::applyScalerCrop(const ControlList &controls)
>  		 * 2. With the same mid-point, if possible.
>  		 * 3. But it can't go outside the sensor area.
>  		 */
> -		Size minSize = ispMinCropSize_.expandedToAspectRatio(nativeCrop.size());
> +		Size minSize = cropParams.ispMinCropSize.expandedToAspectRatio(nativeCrop.size());
>  		Size size = ispCrop.size().expandedTo(minSize);
>  		ispCrop = size.centeredTo(ispCrop.center()).enclosedIn(Rectangle(sensorInfo_.outputSize));
>
> -		if (ispCrop != ispCrop_) {
> -			ispCrop_ = ispCrop;
> +		if (ispCrop != cropParams.ispCrop) {
> +			cropParams.ispCrop = ispCrop;
>  			platformSetIspCrop(ispCrop);
>  		}
>  	}
> @@ -1471,7 +1478,10 @@ void CameraData::fillRequestMetadata(const ControlList &bufferControls, Request
>  	request->metadata().set(controls::SensorTimestamp,
>  				bufferControls.get(controls::SensorTimestamp).value_or(0));
>
> -	request->metadata().set(controls::ScalerCrop, scaleIspCrop(ispCrop_));
> +	const auto cropParamsIt = cropParams_.find(0);
> +	if (cropParamsIt != cropParams_.end())
> +		request->metadata().set(controls::ScalerCrop,
> +					scaleIspCrop(cropParamsIt->second.ispCrop));
>  }
>
>  } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> index d65b695c30b5..75babbe17f31 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> @@ -133,8 +133,25 @@ public:
>
>  	/* For handling digital zoom. */
>  	IPACameraSensorInfo sensorInfo_;
> -	Rectangle ispCrop_; /* crop in ISP (camera mode) pixels */
> -	Size ispMinCropSize_;
> +
> +	struct CropParams {
> +		CropParams(Rectangle ispCrop_, Size ispMinCropSize_)
> +			: ispCrop(ispCrop_), ispMinCropSize(ispMinCropSize_)
> +		{
> +		}
> +
> +		CropParams()
> +		{
> +		}

You probably don't need this ?

Thanks
  j

> +
> +		/* Crop in ISP (camera mode) pixels */
> +		Rectangle ispCrop;
> +		/* Minimum crop size in ISP output pixels */
> +		Size ispMinCropSize;
> +	};
> +
> +	/* Mapping of CropParams keyed by the output stream order in CameraConfiguration */
> +	std::map<unsigned int, CropParams> cropParams_;
>
>  	unsigned int dropFrameCount_;
>
> diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> index 0ea032293bc9..8080f55a1cf4 100644
> --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> @@ -702,13 +702,19 @@ int Vc4CameraData::platformConfigure(const RPi::RPiCameraConfiguration *rpiConfi
>  	/* Figure out the smallest selection the ISP will allow. */
>  	Rectangle testCrop(0, 0, 1, 1);
>  	isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &testCrop);
> -	ispMinCropSize_ = testCrop.size();
>
>  	/* Adjust aspect ratio by providing crops on the input image. */
>  	Size size = unicamFormat.size.boundedToAspectRatio(maxSize);
> -	ispCrop_ = size.centeredTo(Rectangle(unicamFormat.size).center());
> +	Rectangle ispCrop = size.centeredTo(Rectangle(unicamFormat.size).center());
>
> -	platformSetIspCrop(ispCrop_);
> +	platformSetIspCrop(ispCrop);
> +	/*
> +	 * Set the scaler crop to the value we are using (scaled to native sensor
> +	 * coordinates).
> +	 */
> +	cropParams_.emplace(std::piecewise_construct,
> +			    std::forward_as_tuple(0),
> +			    std::forward_as_tuple(ispCrop, testCrop.size()));
>
>  	return 0;
>  }
> --
> 2.34.1
>
Naushir Patuck Oct. 1, 2024, 11:12 a.m. UTC | #3 
Hi Jacopo,

On Tue, 1 Oct 2024 at 07:30, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:
>
> Hi Naush,
>   one more nit
>
> On Mon, Sep 30, 2024 at 03:14:12PM GMT, Naushir Patuck wrote:
> > In preparation for assigning separate crop windows for each stream, add
> > a new CropParams structure that stores the existing ispCrop_ and
> > ispMinCropSize_ as fields. Use a new std::map to store a CropParams
> > structure where the map key is the index of the stream configuration in
> > the CameraConfiguration vector.
> >
> > At preset, only a single CropParams structure will be set at key == 0 to
> > preserve the existing crop handling logic.
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  .../pipeline/rpi/common/pipeline_base.cpp     | 28 +++++++++++++------
> >  .../pipeline/rpi/common/pipeline_base.h       | 21 ++++++++++++--
> >  src/libcamera/pipeline/rpi/vc4/vc4.cpp        | 12 ++++++--
> >  3 files changed, 47 insertions(+), 14 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > index 2de6111bacfd..220c7b962280 100644
> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > @@ -533,6 +533,7 @@ int PipelineHandlerBase::configure(Camera *camera, CameraConfiguration *config)
> >        * Platform specific internal stream configuration. This also assigns
> >        * external streams which get configured below.
> >        */
> > +     data->cropParams_.clear();
> >       ret = data->platformConfigure(rpiConfig);
> >       if (ret)
> >               return ret;
> > @@ -561,10 +562,14 @@ int PipelineHandlerBase::configure(Camera *camera, CameraConfiguration *config)
> >       for (auto const &c : result.controlInfo)
> >               ctrlMap.emplace(c.first, c.second);
> >
> > -     /* 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->scaleIspCrop(data->ispCrop_));
> > +     const auto cropParamsIt = data->cropParams_.find(0);
> > +     if (cropParamsIt != data->cropParams_.end()) {
> > +             const CameraData::CropParams &cropParams = cropParamsIt->second;
> > +             /* Add the ScalerCrop control limits based on the current mode. */
> > +             Rectangle ispMinCrop = data->scaleIspCrop(Rectangle(cropParams.ispMinCropSize));
> > +             ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, data->sensorInfo_.analogCrop,
> > +                                                          data->scaleIspCrop(cropParams.ispCrop));
> > +     }
> >
> >       data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap());
> >
> > @@ -1291,8 +1296,10 @@ Rectangle CameraData::scaleIspCrop(const Rectangle &ispCrop) const
> >  void CameraData::applyScalerCrop(const ControlList &controls)
> >  {
> >       const auto &scalerCrop = controls.get<Rectangle>(controls::ScalerCrop);
> > -     if (scalerCrop) {
> > +     const auto cropParamsIt = cropParams_.find(0);
> > +     if (scalerCrop && cropParamsIt != cropParams_.end()) {
> >               Rectangle nativeCrop = *scalerCrop;
> > +             CropParams &cropParams = cropParamsIt->second;
> >
> >               if (!nativeCrop.width || !nativeCrop.height)
> >                       nativeCrop = { 0, 0, 1, 1 };
> > @@ -1308,12 +1315,12 @@ void CameraData::applyScalerCrop(const ControlList &controls)
> >                * 2. With the same mid-point, if possible.
> >                * 3. But it can't go outside the sensor area.
> >                */
> > -             Size minSize = ispMinCropSize_.expandedToAspectRatio(nativeCrop.size());
> > +             Size minSize = cropParams.ispMinCropSize.expandedToAspectRatio(nativeCrop.size());
> >               Size size = ispCrop.size().expandedTo(minSize);
> >               ispCrop = size.centeredTo(ispCrop.center()).enclosedIn(Rectangle(sensorInfo_.outputSize));
> >
> > -             if (ispCrop != ispCrop_) {
> > -                     ispCrop_ = ispCrop;
> > +             if (ispCrop != cropParams.ispCrop) {
> > +                     cropParams.ispCrop = ispCrop;
> >                       platformSetIspCrop(ispCrop);
> >               }
> >       }
> > @@ -1471,7 +1478,10 @@ void CameraData::fillRequestMetadata(const ControlList &bufferControls, Request
> >       request->metadata().set(controls::SensorTimestamp,
> >                               bufferControls.get(controls::SensorTimestamp).value_or(0));
> >
> > -     request->metadata().set(controls::ScalerCrop, scaleIspCrop(ispCrop_));
> > +     const auto cropParamsIt = cropParams_.find(0);
> > +     if (cropParamsIt != cropParams_.end())
> > +             request->metadata().set(controls::ScalerCrop,
> > +                                     scaleIspCrop(cropParamsIt->second.ispCrop));
> >  }
> >
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> > index d65b695c30b5..75babbe17f31 100644
> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h
> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> > @@ -133,8 +133,25 @@ public:
> >
> >       /* For handling digital zoom. */
> >       IPACameraSensorInfo sensorInfo_;
> > -     Rectangle ispCrop_; /* crop in ISP (camera mode) pixels */
> > -     Size ispMinCropSize_;
> > +
> > +     struct CropParams {
> > +             CropParams(Rectangle ispCrop_, Size ispMinCropSize_)
> > +                     : ispCrop(ispCrop_), ispMinCropSize(ispMinCropSize_)
> > +             {
> > +             }
> > +
> > +             CropParams()
> > +             {
> > +             }
>
> You probably don't need this ?

Yes, I can get rid of this.  I will have to change from cropParams_[i]
to cropParams_.at(i) in the code since it's a std::map. But I think
that's a good idea anyway!

Regards,
Naush

>
> Thanks
>   j
>
> > +
> > +             /* Crop in ISP (camera mode) pixels */
> > +             Rectangle ispCrop;
> > +             /* Minimum crop size in ISP output pixels */
> > +             Size ispMinCropSize;
> > +     };
> > +
> > +     /* Mapping of CropParams keyed by the output stream order in CameraConfiguration */
> > +     std::map<unsigned int, CropParams> cropParams_;
> >
> >       unsigned int dropFrameCount_;
> >
> > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> > index 0ea032293bc9..8080f55a1cf4 100644
> > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> > @@ -702,13 +702,19 @@ int Vc4CameraData::platformConfigure(const RPi::RPiCameraConfiguration *rpiConfi
> >       /* Figure out the smallest selection the ISP will allow. */
> >       Rectangle testCrop(0, 0, 1, 1);
> >       isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &testCrop);
> > -     ispMinCropSize_ = testCrop.size();
> >
> >       /* Adjust aspect ratio by providing crops on the input image. */
> >       Size size = unicamFormat.size.boundedToAspectRatio(maxSize);
> > -     ispCrop_ = size.centeredTo(Rectangle(unicamFormat.size).center());
> > +     Rectangle ispCrop = size.centeredTo(Rectangle(unicamFormat.size).center());
> >
> > -     platformSetIspCrop(ispCrop_);
> > +     platformSetIspCrop(ispCrop);
> > +     /*
> > +      * Set the scaler crop to the value we are using (scaled to native sensor
> > +      * coordinates).
> > +      */
> > +     cropParams_.emplace(std::piecewise_construct,
> > +                         std::forward_as_tuple(0),
> > +                         std::forward_as_tuple(ispCrop, testCrop.size()));
> >
> >       return 0;
> >  }
> > --
> > 2.34.1
> >
Jacopo Mondi Oct. 1, 2024, 6:23 p.m. UTC | #4 
Hi Naush

On Tue, Oct 01, 2024 at 12:12:46PM GMT, Naushir Patuck wrote:
> Hi Jacopo,
>
> On Tue, 1 Oct 2024 at 07:30, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:
> >
> > Hi Naush,
> >   one more nit
> >
> > On Mon, Sep 30, 2024 at 03:14:12PM GMT, Naushir Patuck wrote:
> > > In preparation for assigning separate crop windows for each stream, add
> > > a new CropParams structure that stores the existing ispCrop_ and
> > > ispMinCropSize_ as fields. Use a new std::map to store a CropParams
> > > structure where the map key is the index of the stream configuration in
> > > the CameraConfiguration vector.
> > >
> > > At preset, only a single CropParams structure will be set at key == 0 to
> > > preserve the existing crop handling logic.
> > >
> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >  .../pipeline/rpi/common/pipeline_base.cpp     | 28 +++++++++++++------
> > >  .../pipeline/rpi/common/pipeline_base.h       | 21 ++++++++++++--
> > >  src/libcamera/pipeline/rpi/vc4/vc4.cpp        | 12 ++++++--
> > >  3 files changed, 47 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > index 2de6111bacfd..220c7b962280 100644
> > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > @@ -533,6 +533,7 @@ int PipelineHandlerBase::configure(Camera *camera, CameraConfiguration *config)
> > >        * Platform specific internal stream configuration. This also assigns
> > >        * external streams which get configured below.
> > >        */
> > > +     data->cropParams_.clear();
> > >       ret = data->platformConfigure(rpiConfig);
> > >       if (ret)
> > >               return ret;
> > > @@ -561,10 +562,14 @@ int PipelineHandlerBase::configure(Camera *camera, CameraConfiguration *config)
> > >       for (auto const &c : result.controlInfo)
> > >               ctrlMap.emplace(c.first, c.second);
> > >
> > > -     /* 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->scaleIspCrop(data->ispCrop_));
> > > +     const auto cropParamsIt = data->cropParams_.find(0);
> > > +     if (cropParamsIt != data->cropParams_.end()) {
> > > +             const CameraData::CropParams &cropParams = cropParamsIt->second;
> > > +             /* Add the ScalerCrop control limits based on the current mode. */
> > > +             Rectangle ispMinCrop = data->scaleIspCrop(Rectangle(cropParams.ispMinCropSize));
> > > +             ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, data->sensorInfo_.analogCrop,
> > > +                                                          data->scaleIspCrop(cropParams.ispCrop));
> > > +     }
> > >
> > >       data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap());
> > >
> > > @@ -1291,8 +1296,10 @@ Rectangle CameraData::scaleIspCrop(const Rectangle &ispCrop) const
> > >  void CameraData::applyScalerCrop(const ControlList &controls)
> > >  {
> > >       const auto &scalerCrop = controls.get<Rectangle>(controls::ScalerCrop);
> > > -     if (scalerCrop) {
> > > +     const auto cropParamsIt = cropParams_.find(0);
> > > +     if (scalerCrop && cropParamsIt != cropParams_.end()) {
> > >               Rectangle nativeCrop = *scalerCrop;
> > > +             CropParams &cropParams = cropParamsIt->second;
> > >
> > >               if (!nativeCrop.width || !nativeCrop.height)
> > >                       nativeCrop = { 0, 0, 1, 1 };
> > > @@ -1308,12 +1315,12 @@ void CameraData::applyScalerCrop(const ControlList &controls)
> > >                * 2. With the same mid-point, if possible.
> > >                * 3. But it can't go outside the sensor area.
> > >                */
> > > -             Size minSize = ispMinCropSize_.expandedToAspectRatio(nativeCrop.size());
> > > +             Size minSize = cropParams.ispMinCropSize.expandedToAspectRatio(nativeCrop.size());
> > >               Size size = ispCrop.size().expandedTo(minSize);
> > >               ispCrop = size.centeredTo(ispCrop.center()).enclosedIn(Rectangle(sensorInfo_.outputSize));
> > >
> > > -             if (ispCrop != ispCrop_) {
> > > -                     ispCrop_ = ispCrop;
> > > +             if (ispCrop != cropParams.ispCrop) {
> > > +                     cropParams.ispCrop = ispCrop;
> > >                       platformSetIspCrop(ispCrop);
> > >               }
> > >       }
> > > @@ -1471,7 +1478,10 @@ void CameraData::fillRequestMetadata(const ControlList &bufferControls, Request
> > >       request->metadata().set(controls::SensorTimestamp,
> > >                               bufferControls.get(controls::SensorTimestamp).value_or(0));
> > >
> > > -     request->metadata().set(controls::ScalerCrop, scaleIspCrop(ispCrop_));
> > > +     const auto cropParamsIt = cropParams_.find(0);
> > > +     if (cropParamsIt != cropParams_.end())
> > > +             request->metadata().set(controls::ScalerCrop,
> > > +                                     scaleIspCrop(cropParamsIt->second.ispCrop));
> > >  }
> > >
> > >  } /* namespace libcamera */
> > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> > > index d65b695c30b5..75babbe17f31 100644
> > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h
> > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> > > @@ -133,8 +133,25 @@ public:
> > >
> > >       /* For handling digital zoom. */
> > >       IPACameraSensorInfo sensorInfo_;
> > > -     Rectangle ispCrop_; /* crop in ISP (camera mode) pixels */
> > > -     Size ispMinCropSize_;
> > > +
> > > +     struct CropParams {
> > > +             CropParams(Rectangle ispCrop_, Size ispMinCropSize_)
> > > +                     : ispCrop(ispCrop_), ispMinCropSize(ispMinCropSize_)
> > > +             {
> > > +             }
> > > +
> > > +             CropParams()
> > > +             {
> > > +             }
> >
> > You probably don't need this ?
>
> Yes, I can get rid of this.  I will have to change from cropParams_[i]
> to cropParams_.at(i) in the code since it's a std::map. But I think
> that's a good idea anyway!

Ah right

../src/libcamera/pipeline/rpi/common/pipeline_base.cpp:577:74:   required from here
  577 |                 Rectangle ispMinCrop = data->scaleIspCrop(Rectangle(data->cropParams_[0].ispMinCropSize));
      |                                                                                        ^
/usr/include/c++/14/tuple:2888:9: error: no matching function for call to ‘libcamera::RPi::CameraData::CropParams::CropParams()’
 2888 |         second(std::forward<_Args2>(std::get<_Indexes2>(__tuple2))...)
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This will prevent erroneously creating non-initialized instances.
However std::vector::at() might throw an exception

Maybe
		CropParams() = default;

it's not that bad after all :)


>
> Regards,
> Naush
>
> >
> > Thanks
> >   j
> >
> > > +
> > > +             /* Crop in ISP (camera mode) pixels */
> > > +             Rectangle ispCrop;
> > > +             /* Minimum crop size in ISP output pixels */
> > > +             Size ispMinCropSize;
> > > +     };
> > > +
> > > +     /* Mapping of CropParams keyed by the output stream order in CameraConfiguration */
> > > +     std::map<unsigned int, CropParams> cropParams_;
> > >
> > >       unsigned int dropFrameCount_;
> > >
> > > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> > > index 0ea032293bc9..8080f55a1cf4 100644
> > > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> > > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> > > @@ -702,13 +702,19 @@ int Vc4CameraData::platformConfigure(const RPi::RPiCameraConfiguration *rpiConfi
> > >       /* Figure out the smallest selection the ISP will allow. */
> > >       Rectangle testCrop(0, 0, 1, 1);
> > >       isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &testCrop);
> > > -     ispMinCropSize_ = testCrop.size();
> > >
> > >       /* Adjust aspect ratio by providing crops on the input image. */
> > >       Size size = unicamFormat.size.boundedToAspectRatio(maxSize);
> > > -     ispCrop_ = size.centeredTo(Rectangle(unicamFormat.size).center());
> > > +     Rectangle ispCrop = size.centeredTo(Rectangle(unicamFormat.size).center());
> > >
> > > -     platformSetIspCrop(ispCrop_);
> > > +     platformSetIspCrop(ispCrop);
> > > +     /*
> > > +      * Set the scaler crop to the value we are using (scaled to native sensor
> > > +      * coordinates).
> > > +      */
> > > +     cropParams_.emplace(std::piecewise_construct,
> > > +                         std::forward_as_tuple(0),
> > > +                         std::forward_as_tuple(ispCrop, testCrop.size()));
> > >
> > >       return 0;
> > >  }
> > > --
> > > 2.34.1
> > >

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
index 2de6111bacfd..220c7b962280 100644
--- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
+++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
@@ -533,6 +533,7 @@  int PipelineHandlerBase::configure(Camera *camera, CameraConfiguration *config)
 	 * Platform specific internal stream configuration. This also assigns
 	 * external streams which get configured below.
 	 */
+	data->cropParams_.clear();
 	ret = data->platformConfigure(rpiConfig);
 	if (ret)
 		return ret;
@@ -561,10 +562,14 @@  int PipelineHandlerBase::configure(Camera *camera, CameraConfiguration *config)
 	for (auto const &c : result.controlInfo)
 		ctrlMap.emplace(c.first, c.second);
 
-	/* 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->scaleIspCrop(data->ispCrop_));
+	const auto cropParamsIt = data->cropParams_.find(0);
+	if (cropParamsIt != data->cropParams_.end()) {
+		const CameraData::CropParams &cropParams = cropParamsIt->second;
+		/* Add the ScalerCrop control limits based on the current mode. */
+		Rectangle ispMinCrop = data->scaleIspCrop(Rectangle(cropParams.ispMinCropSize));
+		ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, data->sensorInfo_.analogCrop,
+							     data->scaleIspCrop(cropParams.ispCrop));
+	}
 
 	data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap());
 
@@ -1291,8 +1296,10 @@  Rectangle CameraData::scaleIspCrop(const Rectangle &ispCrop) const
 void CameraData::applyScalerCrop(const ControlList &controls)
 {
 	const auto &scalerCrop = controls.get<Rectangle>(controls::ScalerCrop);
-	if (scalerCrop) {
+	const auto cropParamsIt = cropParams_.find(0);
+	if (scalerCrop && cropParamsIt != cropParams_.end()) {
 		Rectangle nativeCrop = *scalerCrop;
+		CropParams &cropParams = cropParamsIt->second;
 
 		if (!nativeCrop.width || !nativeCrop.height)
 			nativeCrop = { 0, 0, 1, 1 };
@@ -1308,12 +1315,12 @@  void CameraData::applyScalerCrop(const ControlList &controls)
 		 * 2. With the same mid-point, if possible.
 		 * 3. But it can't go outside the sensor area.
 		 */
-		Size minSize = ispMinCropSize_.expandedToAspectRatio(nativeCrop.size());
+		Size minSize = cropParams.ispMinCropSize.expandedToAspectRatio(nativeCrop.size());
 		Size size = ispCrop.size().expandedTo(minSize);
 		ispCrop = size.centeredTo(ispCrop.center()).enclosedIn(Rectangle(sensorInfo_.outputSize));
 
-		if (ispCrop != ispCrop_) {
-			ispCrop_ = ispCrop;
+		if (ispCrop != cropParams.ispCrop) {
+			cropParams.ispCrop = ispCrop;
 			platformSetIspCrop(ispCrop);
 		}
 	}
@@ -1471,7 +1478,10 @@  void CameraData::fillRequestMetadata(const ControlList &bufferControls, Request
 	request->metadata().set(controls::SensorTimestamp,
 				bufferControls.get(controls::SensorTimestamp).value_or(0));
 
-	request->metadata().set(controls::ScalerCrop, scaleIspCrop(ispCrop_));
+	const auto cropParamsIt = cropParams_.find(0);
+	if (cropParamsIt != cropParams_.end())
+		request->metadata().set(controls::ScalerCrop,
+					scaleIspCrop(cropParamsIt->second.ispCrop));
 }
 
 } /* namespace libcamera */
diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h
index d65b695c30b5..75babbe17f31 100644
--- a/src/libcamera/pipeline/rpi/common/pipeline_base.h
+++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h
@@ -133,8 +133,25 @@  public:
 
 	/* For handling digital zoom. */
 	IPACameraSensorInfo sensorInfo_;
-	Rectangle ispCrop_; /* crop in ISP (camera mode) pixels */
-	Size ispMinCropSize_;
+
+	struct CropParams {
+		CropParams(Rectangle ispCrop_, Size ispMinCropSize_)
+			: ispCrop(ispCrop_), ispMinCropSize(ispMinCropSize_)
+		{
+		}
+
+		CropParams()
+		{
+		}
+
+		/* Crop in ISP (camera mode) pixels */
+		Rectangle ispCrop;
+		/* Minimum crop size in ISP output pixels */
+		Size ispMinCropSize;
+	};
+
+	/* Mapping of CropParams keyed by the output stream order in CameraConfiguration */
+	std::map<unsigned int, CropParams> cropParams_;
 
 	unsigned int dropFrameCount_;
 
diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
index 0ea032293bc9..8080f55a1cf4 100644
--- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp
+++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
@@ -702,13 +702,19 @@  int Vc4CameraData::platformConfigure(const RPi::RPiCameraConfiguration *rpiConfi
 	/* Figure out the smallest selection the ISP will allow. */
 	Rectangle testCrop(0, 0, 1, 1);
 	isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &testCrop);
-	ispMinCropSize_ = testCrop.size();
 
 	/* Adjust aspect ratio by providing crops on the input image. */
 	Size size = unicamFormat.size.boundedToAspectRatio(maxSize);
-	ispCrop_ = size.centeredTo(Rectangle(unicamFormat.size).center());
+	Rectangle ispCrop = size.centeredTo(Rectangle(unicamFormat.size).center());
 
-	platformSetIspCrop(ispCrop_);
+	platformSetIspCrop(ispCrop);
+	/*
+	 * Set the scaler crop to the value we are using (scaled to native sensor
+	 * coordinates).
+	 */
+	cropParams_.emplace(std::piecewise_construct,
+			    std::forward_as_tuple(0),
+			    std::forward_as_tuple(ispCrop, testCrop.size()));
 
 	return 0;
 }