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

[Jacopo Mondi]

Hi Laurent,

On Tue, Sep 15, 2020 at 04:41:48AM +0300, Laurent Pinchart wrote:
> 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.

Ok. Even thoughI think erase() is in line with the std naming scheme
which we try to respect when possible.

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

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

Thanks
  j

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