[libcamera-devel] [PATCH] android: camera_device: Return unique_ptr from createFrameBuffer

[Hirokazu Honda]

Hi Laurent, thank you for the patch.

On Thu, Sep 23, 2021 at 9:08 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> Hi Laurent
>
> On Thu, Sep 23, 2021 at 02:54:39PM +0300, Laurent Pinchart wrote:
> > Returning a non-managed pointer can cause leaks. Use a unique_ptr<>
> > instead to avoid possible future issues.
> >
> > The std::move() for the planes argument to the FrameBuffer constructor
> > is dropped as it's misleading. FrameBuffer has no constructor that takes
> > an rvalue reference to planes, so the vector was copied despite the
> > move. This only clarifies the intent, no functional change is
> > introduced.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>

Reviewed-by: Hirokazu Honda <hiroh at chromium.org>

> Looks good!
>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>
> Thanks
>    j
>
> > ---
> >  src/android/camera_device.cpp | 18 ++++++++++--------
> >  src/android/camera_device.h   |  7 ++++---
> >  2 files changed, 14 insertions(+), 11 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index a693dcbe89f4..21844e5114a9 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -774,9 +774,9 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >       return 0;
> >  }
> >
> > -FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer,
> > -                                          PixelFormat pixelFormat,
> > -                                          const Size &size)
> > +std::unique_ptr<FrameBuffer>
> > +CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer,
> > +                             PixelFormat pixelFormat, const Size &size)
> >  {
> >       CameraBuffer buf(camera3buffer, pixelFormat, size, PROT_READ);
> >       if (!buf.isValid()) {
> > @@ -797,7 +797,7 @@ FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer
> >               planes[i].length = buf.size(i);
> >       }
> >
> > -     return new FrameBuffer(std::move(planes));
> > +     return std::make_unique<FrameBuffer>(planes);
> >  }
> >
> >  int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)
> > @@ -1002,10 +1002,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >                        * associate it with the Camera3RequestDescriptor for
> >                        * lifetime management only.
> >                        */
> > -                     buffer = createFrameBuffer(*camera3Buffer.buffer,
> > -                                                cameraStream->configuration().pixelFormat,
> o
> > -                                                cameraStream->configuration().size);
> > -                     descriptor.frameBuffers_.emplace_back(buffer);
> > +                     descriptor.frameBuffers_.push_back(
> > +                             createFrameBuffer(*camera3Buffer.buffer,
> > +                                               cameraStream->configuration().pixelFormat,
> > +                                               cameraStream->configuration().size));
> > +
> > +                     buffer = descriptor.frameBuffers_.back().get();
> >                       LOG(HAL, Debug) << ss.str() << " (direct)";
> >                       break;
> >
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index 296c2f185e4e..43eb5895ba64 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -94,9 +94,10 @@ private:
> >
> >       void stop();
> >
> > -     libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer,
> > -                                               libcamera::PixelFormat pixelFormat,
> > -                                               const libcamera::Size &size);
> > +     std::unique_ptr<libcamera::FrameBuffer>
> > +     createFrameBuffer(const buffer_handle_t camera3buffer,
> > +                       libcamera::PixelFormat pixelFormat,
> > +                       const libcamera::Size &size);
> >       void abortRequest(camera3_capture_request_t *request);
> >       bool isValidRequest(camera3_capture_request_t *request) const;
> >       void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
> >
> > base-commit: 2cc4303b172a76ac5b431c4fb4df8a083f7d3fcf
> > --
> > Regards,
> >
> > Laurent Pinchart
> >