[libcamera-devel] [PATCH 3/8] libcamera: frame_buffer_allocator: Add clear()

[Jacopo Mondi]

Hi Hiro,

On Fri, Sep 11, 2020 at 01:42:58PM +0900, Hirokazu Honda wrote:
> On Thu, Sep 10, 2020 at 8:19 PM Niklas Söderlund
> <niklas.soderlund at ragnatech.se> wrote:
> >
> > Hi Jacopo,
> >
> > Thanks for your work.
> >
> > On 2020-09-09 17:54:52 +0200, Jacopo Mondi wrote:
> > > Add a clear() method to the FrameBufferAllocator class that
> > > frees all the buffers previously reserved by the allocator.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> >
> > Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> >
> > > ---
> > >  include/libcamera/framebuffer_allocator.h | 1 +
> > >  src/libcamera/framebuffer_allocator.cpp   | 8 ++++++++
> > >  2 files changed, 9 insertions(+)
> > >
> > > diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h
> > > index 78f1353964eb..2a4d538a0cb2 100644
> > > --- a/include/libcamera/framebuffer_allocator.h
> > > +++ b/include/libcamera/framebuffer_allocator.h
> > > @@ -28,6 +28,7 @@ public:
> > >
> > >       int allocate(Stream *stream);
> > >       int free(Stream *stream);
> > > +     void clear();
> > >
> > >       bool allocated() const { return !buffers_.empty(); }
> > >       const std::vector<std::unique_ptr<FrameBuffer>> &buffers(Stream *stream) const;
> > > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp
> > > index 2fbba37a1b0b..7ed80011c845 100644
> > > --- a/src/libcamera/framebuffer_allocator.cpp
> > > +++ b/src/libcamera/framebuffer_allocator.cpp
> > > @@ -125,6 +125,14 @@ int FrameBufferAllocator::free(Stream *stream)
> > >       return 0;
> > >  }
> > >
> > > +/**
> > > + * \brief Free all the buffers previously allocated
> > > + */
> > > +void FrameBufferAllocator::clear()
> > > +{
> > > +     buffers_.clear();
> > > +}
> > > +
>
> This is the first time I looked at FrameBufferAllocator.
> Those comments are not so related to this patch.
> I didn't get the difference between free() and clear() from the name.
> I would rename free() to erase(). If it sounds good to you, can I ask
> you to do it later?

Being this a map underneath, using erase would align with std::map

> Besides, clear() in dtor seems to be unnecessary.
>

You mean the one it's introduced in the next patch ? Probably not as
the newly added availableBuffers_ vector is just a vector of pointers,
there's not need to release memory...


> Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
>
> > >  /**
> > >   * \fn FrameBufferAllocator::allocated()
> > >   * \brief Check if the allocator has allocated buffers for any stream
> > > --
> > > 2.28.0
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel at lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
> >
> > --
> > Regards,
> > Niklas Söderlund
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel