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

[Kieran Bingham]

Hi Naush,

On 17/02/2021 10:08, Naushir Patuck wrote:
> 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>
> ---
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 9dd4d112a907..a60415d93705 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1338,13 +1338,14 @@ void RPiCameraData::embeddedComplete(uint32_t bufferId)
>  
>  void RPiCameraData::setIspControls(const ControlList &controls)
>  {
> -	ControlList ctrls = controls;
> +	ControlList ctrls = std::move(controls);

Oh, can we do that on a const ControlList reference ?
Doesn't that by definition modify the parameter that came in?


>  
>  	if (ctrls.contains(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING)) {
> -		Span<const uint8_t> s =
> -			ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data();
> -		const bcm2835_isp_lens_shading *ls =
> -			reinterpret_cast<const bcm2835_isp_lens_shading *>(s.data());
> +		ControlValue &value =
> +			const_cast<ControlValue &>(ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING));

Could you take a look at Laurent's V4L2 control series he posted recently?

 [PATCH/RFC 0/2] libcamera: Add Control instances for V4L2 controls

By putting the types into the Control, I think it helps simplify a lot
of code, but I'm curious how it would affect controls which map to
structure arrays like this...

> +		Span<uint8_t> s = value.data();
> +		bcm2835_isp_lens_shading *ls =
> +			reinterpret_cast<bcm2835_isp_lens_shading *>(s.data());
>  		ls->dmabuf = lsTable_.fd();
>  	}

Now that you've moved controls to the local ctrls, when it reaches the
end of this function, it would go out of scope and release the control
list won't it ?

Does that mean your now potentially pointing to a lens-shading table
that has been released? or is that not an issue?

(Or is it used later in this function which the mail has not provided
context for ?...)



>  
> 

-- 
Regards
--
Kieran