[libcamera-devel] [PATCH 5/8] libcamera: framebuffer_allocator: Get and return buffers
Jacopo Mondi
jacopo at jmondi.org
Fri Sep 11 18:25:04 CEST 2020
Hi Hiro,
On Fri, Sep 11, 2020 at 05:12:35PM +0900, Hirokazu Honda wrote:
> On Thu, Sep 10, 2020 at 10:19 PM Jacopo Mondi <jacopo at jmondi.org> 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 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 am ok with this API change.
> I can understand the opinions of both Jacopo and Niklas.
> Since buffers() returns const reference of vector<unique_ptr>, client
> cannot takes
> the ownership by std::move(). Having Get/ReturnBuffer() and buffers() should not
> go wrong if the client adopts either way.
mmm, you're right here, the reference is a const, there's not risk of
messing up the buffer ownership..
> The thing is more like, where the buffer usage is managed, in client
> (with buffers()), or
> in FrameBufferAllocator (with Get/ReturnBuffer()).
> For simplicity, I think eventually FrameBufferAllocator should have
> GetBuffer() and ReturnBuffer().
>
Yes, I think getBuffer/returnBuffer provide a managed interface that
ease the application life, but at the same time I won't be super happy
of preventing application to implement custom ones...
Not sure, I still feel the two interfaces can live together, and if
someone mis-uses the two it's the same kind of error that one would do
if buffers are get but never returned == bad application code :)
> > 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_;
> > > > > > };
> > > > > >
>
> I think we don't have to think of order. So how about using
> std::vector and use the last entry of the vector?
>
> > > > > > } /* 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();
> > > > > > }
> > > > > >
>
> clear() is unnecessary.
>
> > > > > > /**
> > > > > > @@ -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));
> > > > > > +
>
> I would just availableBuffers_.erase(stream);
> ditto to buffers_.
>
> > > > > > 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);
> > > > > > + }
> > > > > > +}
> > > > > > +
>
> I expect you may want to do as below. buffers_[stream] must not have
> the same buffers because they are unique_ptr.
> const auto& buffers = buffers_[stream];
> if (std::find_if(buffers.begin(), buffers.end(), [buffer](const auto&
> b) { return b.get() == buffer; } )) {
> 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
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list