[libcamera-devel,v3,3/8] libcamera: frame_buffer_allocator: Add clear()
diff mbox series

Message ID 20200922094738.5327-4-jacopo@jmondi.org
State Superseded
Headers show
Series
  • android: camera_device: Add support for internal buffers
Related show

Commit Message

Jacopo Mondi Sept. 22, 2020, 9:47 a.m. UTC 
Add a clear() method to the FrameBufferAllocator class that
frees all the buffers previously reserved by the allocator.

Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 include/libcamera/framebuffer_allocator.h | 1 +
 src/libcamera/framebuffer_allocator.cpp   | 8 ++++++++
 2 files changed, 9 insertions(+)

Comments

Kieran Bingham Sept. 23, 2020, 12:05 p.m. UTC | #1 
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@chromium.org>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> Signed-off-by: Jacopo Mondi <jacopo@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..

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.

Otherwise, reset() would also feel appropriate here.

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@ideasonboard.com>

> +
>  /**
>   * \fn FrameBufferAllocator::allocated()
>   * \brief Check if the allocator has allocated buffers for any stream
>
Jacopo Mondi Sept. 24, 2020, 3:41 p.m. UTC | #2 
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@chromium.org>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > Signed-off-by: Jacopo Mondi <jacopo@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@ideasonboard.com>

Thanks

>
> > +
> >  /**
> >   * \fn FrameBufferAllocator::allocated()
> >   * \brief Check if the allocator has allocated buffers for any stream
> >
>
> --
> Regards
> --
> Kieran
Laurent Pinchart Sept. 29, 2020, 1:40 a.m. UTC | #3 
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@chromium.org>
> > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > Signed-off-by: Jacopo Mondi <jacopo@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@ideasonboard.com>

> > 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@ideasonboard.com>
> 
> Thanks
> 
> > > +
> > >  /**
> > >   * \fn FrameBufferAllocator::allocated()
> > >   * \brief Check if the allocator has allocated buffers for any stream
Jacopo Mondi Sept. 30, 2020, 9:20 a.m. UTC | #4 
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@chromium.org>
> > > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > > Signed-off-by: Jacopo Mondi <jacopo@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@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@ideasonboard.com>
> >
> > Thanks
> >
> > > > +
> > > >  /**
> > > >   * \fn FrameBufferAllocator::allocated()
> > > >   * \brief Check if the allocator has allocated buffers for any stream
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

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();
+}
+
 /**
  * \fn FrameBufferAllocator::allocated()
  * \brief Check if the allocator has allocated buffers for any stream