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

[Laurent Pinchart]

Hi Jacopo,

On Thu, Sep 17, 2020 at 12:41:55PM +0200, Jacopo Mondi wrote:
> On Tue, Sep 15, 2020 at 04:41:48AM +0300, Laurent Pinchart wrote:
> > 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.
> 
> Ok. Even thoughI think erase() is in line with the std naming scheme
> which we try to respect when possible.

Sorry, I meant allocate/free.

> > Regarding clear(), I don't think that's a very intuitive name. freeAll()
> > would be better.
> 
> What's not intuitive in clear() ? All the std containers have a
> clear() method with the same description: "Clear the content".
> Isn't that what it happens here?

But this isn't a container, it's an allocator...

> I would agree more on this if there was any explicity buffer releasing
> operation, but since memory release is a consequence of deleting the
> FrameBuffer instances that wraps the dmabuf fds, what we actually do
> is just call clear() on the buffes_ map.
> 
> freeAll() is more clunky to me.
> 
> I'll keep clear() and free().
> 
> > > > 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