[libcamera-devel] [PATCH 5/8] libcamera: framebuffer_allocator: Get and return buffers

[Jacopo Mondi]

Hi Niklas,

On Thu, Sep 10, 2020 at 01:30:16PM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your work.
>
> On 2020-09-09 17:54:54 +0200, Jacopo Mondi wrote:
> > Add to the FrameBufferAllocator class two methods to get and
> > return buffers from the pool of buffers allocated for a Stream.
> >
> > The two methods return pointer to the allocated buffers without
> > transferring ownership to the caller.
>
> I'm sorry I don't like this patch. If it was an interface that was
> internal I would be more OK with this change. The FrameBufferAllocator
> is facing applications and this change introduces a duality between the
> new getBuffer() and the existing buffers() functionality.
>
> I think it creates complexity in this user-facing API which is not
> really needed. How about creating a HALFrameBufferAllocator that is
> local to the HAL and inherits from FrameBufferAllocator while adding the
> features you want?

That would end up repeating the same thing we do in pipeline handlers
here. Actually I found safer to provide a get/return interface than
giving applications full access to the pool and let them deal with
that. At the same time I won't prevent that completely by removing
buffers(), but I don't see why the 'duality' is an issue.

I'm more than open to discuss the implementation which is surely
improvable and if we want to implement this feature in
FrameBufferAllocator or as you suggested sub-class it somehow, but I'm
not sure why the problem lies in having the three methods.

>
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  include/libcamera/framebuffer_allocator.h |  5 ++
> >  src/libcamera/framebuffer_allocator.cpp   | 60 +++++++++++++++++++++--
> >  2 files changed, 62 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h
> > index 2a4d538a0cb2..1f3f10d4ec03 100644
> > --- a/include/libcamera/framebuffer_allocator.h
> > +++ b/include/libcamera/framebuffer_allocator.h
> > @@ -9,6 +9,7 @@
> >
> >  #include <map>
> >  #include <memory>
> > +#include <queue>
> >  #include <vector>
> >
> >  namespace libcamera {
> > @@ -33,9 +34,13 @@ public:
> >  	bool allocated() const { return !buffers_.empty(); }
> >  	const std::vector<std::unique_ptr<FrameBuffer>> &buffers(Stream *stream) const;
> >
> > +	FrameBuffer *getBuffer(Stream *stream);
> > +	void returnBuffer(Stream *stream, FrameBuffer *buffer);
> > +
> >  private:
> >  	std::shared_ptr<Camera> camera_;
> >  	std::map<Stream *, std::vector<std::unique_ptr<FrameBuffer>>> buffers_;
> > +	std::map<Stream *, std::queue<FrameBuffer *>> availableBuffers_;
> >  };
> >
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp
> > index 7ed80011c845..7429d6b9edb7 100644
> > --- a/src/libcamera/framebuffer_allocator.cpp
> > +++ b/src/libcamera/framebuffer_allocator.cpp
> > @@ -64,7 +64,7 @@ FrameBufferAllocator::FrameBufferAllocator(std::shared_ptr<Camera> camera)
> >
> >  FrameBufferAllocator::~FrameBufferAllocator()
> >  {
> > -	buffers_.clear();
> > +	clear();
> >  }
> >
> >  /**
> > @@ -93,11 +93,17 @@ int FrameBufferAllocator::allocate(Stream *stream)
> >  	}
> >
> >  	int ret = camera_->exportFrameBuffers(stream, &buffers_[stream]);
> > -	if (ret == -EINVAL)
> > +	if (ret == -EINVAL) {
> >  		LOG(Allocator, Error)
> >  			<< "Stream is not part of " << camera_->id()
> >  			<< " active configuration";
> > -	return ret;
> > +		return ret;
> > +	}
> > +
> > +	for (const auto &buffer : buffers_[stream])
> > +		availableBuffers_[stream].push(buffer.get());
> > +
> > +	return 0;
> >  }
> >
> >  /**
> > @@ -122,6 +128,9 @@ int FrameBufferAllocator::free(Stream *stream)
> >  	buffers.clear();
> >  	buffers_.erase(iter);
> >
> > +	availableBuffers_[stream] = {};
> > +	availableBuffers_.erase(availableBuffers_.find(stream));
> > +
> >  	return 0;
> >  }
> >
> > @@ -131,6 +140,7 @@ int FrameBufferAllocator::free(Stream *stream)
> >  void FrameBufferAllocator::clear()
> >  {
> >  	buffers_.clear();
> > +	availableBuffers_.clear();
> >  }
> >
> >  /**
> > @@ -162,4 +172,48 @@ FrameBufferAllocator::buffers(Stream *stream) const
> >  	return iter->second;
> >  }
> >
> > +/**
> > + * \brief Get a pointer to a \a buffer for the \a stream
> > + * \param[in] stream The stream to get a buffer for
> > + *
> > + * The method returns a pointer to a FrameBuffer but does transfer the buffer
> > + * ownership to the caller: the returned pointer remains valid until the
> > + * FrameBufferAllocator does not get deleted or the allocated buffers do not get
> > + * released with a call for free() or clear().
> > + *
> > + * \return A FrameBuffer pointer or nullptr if the no buffers is available
> > + */
> > +FrameBuffer *FrameBufferAllocator::getBuffer(Stream *stream)
> > +{
> > +	if (!allocated() || buffers_[stream].empty())
> > +		return nullptr;
> > +
> > +	FrameBuffer *frameBuffer = availableBuffers_[stream].front();
> > +	availableBuffers_[stream].pop();
> > +
> > +	return frameBuffer;
> > +}
> > +
> > +/**
> > + * \brief Return a \a buffer to the list of buffers available for the a \a stream
> > + * \param[in] stream The stream to return buffer to
> > + * \param[in] buffer The buffer to return
> > + */
> > +void FrameBufferAllocator::returnBuffer(Stream *stream, FrameBuffer *buffer)
> > +{
> > +	if (!allocated())
> > +		return;
> > +
> > +	for (const auto &b : buffers_[stream]) {
> > +		/*
> > +		 * Return the buffer to the available queue only if it was part
> > +		 * of the vector of buffers allocated for the Stream.
> > +		 */
> > +		if (b.get() != buffer)
> > +			continue;
> > +
> > +		availableBuffers_[stream].push(buffer);
> > +	}
> > +}
> > +
> >  } /* namespace libcamera */
> > --
> > 2.28.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund