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

[Jacopo Mondi]

Hi Laurent,

On Tue, Sep 29, 2020 at 04:40:26AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Thu, Sep 24, 2020 at 05:41:29PM +0200, Jacopo Mondi wrote:
> > On Wed, Sep 23, 2020 at 01:05:37PM +0100, Kieran Bingham wrote:
> > > On 22/09/2020 10:47, Jacopo Mondi wrote:
> > > > Add a clear() method to the FrameBufferAllocator class that
> > > > frees all the buffers previously reserved by the allocator.
> > > >
> > > > Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
> > > > Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > > ---
> > > >  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();
> > > > +}
> > >
> > > Throwing more opportunities into the bike-shed..
> >
> > By popular demand, I'll change this
> >
> > > clear doesn't fit well for me either, I see Laurent suggested freeAll()
> > > or such.
> > >
> > > We could have an overload for free(no-arg) which clears 'all streams',
> > > i.e. free(stream) is just a filtered subset.
> >
> > I'm not thrilled by an overload :/
> >
> > > Otherwise, reset() would also feel appropriate here.
> >
> > But I like reset!
>
> I'd still vote for freeAll() :-) (or a free() overload as proposed by
> Kieran, but I don't like that option that much). The brief documentation
> you wrote above says "free all", so that would be a name that matches
> the function's purpose quite well.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>

I'm not sure how long we should keep discussing something like this,
as it's purely a matter of tastes as we have decided we don't care to
have an API that resembles the STL containers' one.

To cut down any further discussions I'll make it a freeAll() just because
I don't want to spend any more time on this.

A process like "the one that keeps its point for longer wins" is not
optimal, if you ask me :)


> > > But I can see the need to be able to reset a FrameBufferAllocator() in
> > > some form, so if you're settled on the name:
> > >
> > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >
> > Thanks
> >
> > > > +
> > > >  /**
> > > >   * \fn FrameBufferAllocator::allocated()
> > > >   * \brief Check if the allocator has allocated buffers for any stream
>
> --
> Regards,
>
> Laurent Pinchart