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

[Naushir Patuck]

Hi Kieran,

On Thu, 18 Feb 2021 at 11:35, Kieran Bingham <
kieran.bingham at ideasonboard.com> wrote:

> 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?
>

The compiler does not seem to complain, so I assume this works as we are
not (yet) modifying the elements of the map container.


>
>
> >
> >       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...
>

Sure, will have a look at that.


>
> > +             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 ?
>

Yes, the local ctrls will be out of scope at that point, but the call to
setControls() would have copied the
bcm2835_isp_lens_shading struct internally, so that is not an issue.


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

The bcm2835_isp_lens_shading does not hold the table directly, rather a
handle to the dmabuf
where the table is actually stored.  So again, this is not an issue.


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

The setControls() call is the last line of the patch, so you may have
missed that :-)

However, if you do feel this is still wrong, please shout!


>
>
>
> >
> >
>
> --
> Regards
> --
> Kieran
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210218/337a3b71/attachment.htm>