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

[Jacopo Mondi]

Hi Kieran,

On Wed, Sep 23, 2020 at 01:05:37PM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> 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!

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