[libcamera-devel] [PATCH v3 4/4] pipeline: raspberrypi: Update the lens shading control in-place

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Thu, Feb 18, 2021 at 12:48:24PM +0000, Naushir Patuck wrote:
> Only update the lens shading control if it is present in the
> ControlList.
> 
> Add the dmabuf file descriptor to the lens shading control in-place
> rather than taking a copy.
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 21 +++++++++----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index acf2d56cddb2..a60415d93705 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1338,17 +1338,16 @@ void RPiCameraData::embeddedComplete(uint32_t bufferId)
>  
>  void RPiCameraData::setIspControls(const ControlList &controls)
>  {
> -	ControlList ctrls = controls;
> -
> -	Span<const uint8_t> s =
> -		ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data();
> -	bcm2835_isp_lens_shading ls =
> -		*reinterpret_cast<const bcm2835_isp_lens_shading *>(s.data());
> -	ls.dmabuf = lsTable_.fd();
> -
> -	ControlValue c(Span<const uint8_t>{ reinterpret_cast<uint8_t *>(&ls),
> -					    sizeof(ls) });
> -	ctrls.set(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING, c);
> +	ControlList ctrls = std::move(controls);

std::move() won't have the expected effect here. controls is a const
reference, so ctrls will be a copy. It's a bit annoying that the
compiler doesn't warn us :-/

The code will run fine, but I think avoiding std::move() would be more
explicit as it's easy to overlook the fact that a copy is created
otherwise. No need to change this now, but if you get the chance at some
point, it would be nice.

> +
> +	if (ctrls.contains(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING)) {
> +		ControlValue &value =
> +			const_cast<ControlValue &>(ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING));

The const_cast is not nice. If we want to support this kind of usage, we
should extend the ControlList API to return a non-const ControlValue.

> +		Span<uint8_t> s = value.data();
> +		bcm2835_isp_lens_shading *ls =
> +			reinterpret_cast<bcm2835_isp_lens_shading *>(s.data());
> +		ls->dmabuf = lsTable_.fd();
> +	}
>  
>  	isp_[Isp::Input].dev()->setControls(&ctrls);
>  	handleState();

-- 
Regards,

Laurent Pinchart