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

[Laurent Pinchart]

Hello,

On Fri, Sep 11, 2020 at 06:31:08PM +0200, Jacopo Mondi wrote:
> 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 wrote:
> > > 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

The free() operation is the counterpart of allocate(), I don't think
allocate/erase would be a very good pair. I'd rather keep free(), or
rename allocate too.

Regarding clear(), I don't think that's a very intuitive name. freeAll()
would be better.

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

-- 
Regards,

Laurent Pinchart