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

[Jacopo Mondi]

Hi Niklas,

On Thu, Sep 10, 2020 at 03:02:55PM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> On 2020-09-10 14:27:46 +0200, Jacopo Mondi wrote:
> > 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.
>
> My concern is having an application call getBuffer() to prepare one set
> of Requests, maybe one with a viewfinder + raw and then iterating over
> buffers() to create as many Requests with just viewfinder as available.
> As buffers() don't consider if it have "given" out any buffer prior to
> the buffers() call we have the potential to have Requests that can't be
> queued at the same time as they contain a subset of the same buffers. I
> fear this could get even worse once Request becomes reusable.
>

True that a good API is one you cannot get wrong, but if an
application behaves like that [1] it means it -really- is trying to
shoot itself in the foot. I know it's a thin line though..

I won't push for this, but if asked which interface I would prefer, I
would ask for a get/return one instead of a "here you have all you buffers"
one. There's also the question about buffer ownership, which are
stored as unique_ptr<> and thus connected to the lifetime of the
allocator. A 'get all the buffers' interface would allow application
to move ownership to themselves and really screw up. A stricter
interface would allow us to decide what to do: right now we 'borrow'
the buffer ownership but we might as well decide to transfer that,
but we have a single behavior and I think that's better.

Thanks
  j

[1] I'm not sure I got your example fully. Are you suggesting an
application might initially use getBuffer/returnBuffer then decide to
access the buffer vector returned by buffers() and use all of them
ignoring those are the same buffers it already got access to through
the get/return API ?

> Understand me correctly, I'm not advocating in favor of buffers() or
> getBuffers() interface. I do however think we should only have one as it
> makes the interface easier to use for applications. As you suggest this
> new interface could replace boilerplait code in pipelines and possibly
> even applications to maybe it should replace the old buffers()
> interface?
>
> >
> > >
> > > >
> > > > 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
>
> --
> Regards,
> Niklas Söderlund