[libcamera-devel] [PATCH 5/8] libcamera: framebuffer_allocator: Get and return buffers
Jacopo Mondi
jacopo at jmondi.org
Sat Sep 12 11:49:18 CEST 2020
Hi Hiro,
I missed part of your reply in the previous email
Let me continue here :)
On Fri, Sep 11, 2020 at 06:25:04PM +0200, Jacopo Mondi wrote:
> 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?
Using the last entry doesn't seem the best idea, as you can only push
to the end of an std::vector, so you could potentially cycle over a
single or a limited set of buffers. It wonder about concurrency also,
if an application access the end of the vector from two different
threads (is this possible?) we might need some kind of protection.
One possible way forward would be to push to the end of the vector and
pop from its front. We can reserve space in advance knowing the number
of buffers, so that would be more efficient than a queue indeed.
> >
> > > > > > > } /* 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.
As stated in the previous email, you're probably right
> >
> > > > > > > /**
> > > > > > > @@ -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);
Correct
> > ditto to buffers_.
Also probably correct as buffers_.erase(iter) would delete the vector
of unique pointers there stored, causing memory to be released without
having to
std::vector<std::unique_ptr<FrameBuffer>> &buffers = iter->second;
buffers.clear();
> >
> > > > > > > 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);
> > }
If I got this right, your proposal is functionally equivalent but
I find it less readable. This surely is me and my not being exactly in
love with the ability C++ has to make easy things look complex, but in
this case I find open coding much more explict that going through an
std <algorithm> function. I suspect we could discuss this for days,
for sure your proposal is much more C++-ish than mine, but I found
open coding more explicit.
Anyway, seems like a minor style issue, or have I missed any
functional change?
Thanks
j
> >
> > > > > > > } /* 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
> _______________________________________________
> 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