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

[Niklas Söderlund]

Hi Jacopo,

On 2020-09-10 15:22:55 +0200, Jacopo Mondi wrote:
> 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 think we should aim to have an API where it's impossible to shoot 
yourself in the foot ;-)

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

I think it's a good point about ownership and I agree with you we can 
probably improve and replace on the current buffers() interface. While 
doing so I think we should also keep in mind that we aim to make 
Requests reusable as this may impact on how we wish to model the 
ownership model.

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

Yes, sorry if I was unclear :-)

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

-- 
Regards,
Niklas Söderlund