<div dir="ltr"><div dir="ltr">Hi Kieran,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, 18 Feb 2021 at 11:35, Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com">kieran.bingham@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush,<br>
<br>
On 17/02/2021 10:08, Naushir Patuck wrote:<br>
> Add the dmabuf file descriptor to the lens shading control in-place<br>
> rather than taking a copy.<br>
> <br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> ---<br>
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 11 ++++++-----<br>
>  1 file changed, 6 insertions(+), 5 deletions(-)<br>
> <br>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> index 9dd4d112a907..a60415d93705 100644<br>
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> @@ -1338,13 +1338,14 @@ void RPiCameraData::embeddedComplete(uint32_t bufferId)<br>
>  <br>
>  void RPiCameraData::setIspControls(const ControlList &controls)<br>
>  {<br>
> -     ControlList ctrls = controls;<br>
> +     ControlList ctrls = std::move(controls);<br>
<br>
Oh, can we do that on a const ControlList reference ?<br>
Doesn't that by definition modify the parameter that came in?<br></blockquote><div><br></div><div>The compiler does not seem to complain, so I assume this works as we are not (yet) modifying the elements of the map container.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
<br>
>  <br>
>       if (ctrls.contains(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING)) {<br>
> -             Span<const uint8_t> s =<br>
> -                     ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING).data();<br>
> -             const bcm2835_isp_lens_shading *ls =<br>
> -                     reinterpret_cast<const bcm2835_isp_lens_shading *>(s.data());<br>
> +             ControlValue &value =<br>
> +                     const_cast<ControlValue &>(ctrls.get(V4L2_CID_USER_BCM2835_ISP_LENS_SHADING));<br>
<br>
Could you take a look at Laurent's V4L2 control series he posted recently?<br>
<br>
 [PATCH/RFC 0/2] libcamera: Add Control instances for V4L2 controls<br>
<br>
By putting the types into the Control, I think it helps simplify a lot<br>
of code, but I'm curious how it would affect controls which map to<br>
structure arrays like this...<br></blockquote><div><br></div><div>Sure, will have a look at that.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +             Span<uint8_t> s = value.data();<br>
> +             bcm2835_isp_lens_shading *ls =<br>
> +                     reinterpret_cast<bcm2835_isp_lens_shading *>(s.data());<br>
>               ls->dmabuf = lsTable_.fd();<br>
>       }<br>
<br>
Now that you've moved controls to the local ctrls, when it reaches the<br>
end of this function, it would go out of scope and release the control<br>
list won't it ?<br></blockquote><div><br></div><div>Yes, the local ctrls will be out of scope at that point, but the call to setControls() would have copied the</div><div>bcm2835_isp_lens_shading struct internally, so that is not an issue.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Does that mean your now potentially pointing to a lens-shading table<br>
that has been released? or is that not an issue?<br></blockquote><div><br></div><div>The bcm2835_isp_lens_shading does not hold the table directly, rather a handle to the dmabuf</div><div>where the table is actually stored.  So again, this is not an issue.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
(Or is it used later in this function which the mail has not provided<br>
context for ?...)<br></blockquote><div><br></div><div>The setControls() call is the last line of the patch, so you may have missed that :-)</div><div><br></div><div>However, if you do feel this is still wrong, please shout!</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
<br>
<br>
>  <br>
> <br>
<br>
-- <br>
Regards<br>
--<br>
Kieran<br>
</blockquote></div></div>