[libcamera-devel] [PATCH v2 1/2] libcamera: framebuffer: Enable attaching additional data to FrameBuffer
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Nov 22 01:49:27 CET 2021
Hello,
On Tue, Nov 09, 2021 at 10:02:01AM +0100, Jacopo Mondi wrote:
> On Mon, Nov 08, 2021 at 11:19:51PM +0000, Kieran Bingham wrote:
> > Quoting Hirokazu Honda (2021-11-01 07:16:51)
> > > We cannot have a subclass of FrameBuffer because it is marked as final.
> > > This adds a FrameBuffer constructor with FrameBuffer::Private. So we
> > > can attach some additional resources with FrameBuffer through a
> > > customized FrameBuffer::Private class.
> >
> > Wow, this is interesting. Extensible was, as I understood it used to
> > protect libcamera internals from being accessed by applications (here
> > Android is an application of course).
> >
> > But this patch, lets applications create their own FrameBuffers
> > - where the private data is not accessible to libcamera, so it's the
> > complete inverse!
>
> I had the same question.. but then I realized that
> FrameBuffer::Private is not accessible to applications so they cannot
> subclass it. Also, as Hiro pointed out, the same pattern is in use in
> the Camera class.
That's right. The Extensible class pattern was designed to match derived
classes on the Extensible and Private side, but it turned out to be
useful to only derive on the Private side (this was enabled by commit
33dd4fab9d39 ("libcamera: base: class: Don't pass Extensible pointer to
Private constructor")).
To allow user-defined FrameBufferAllocator implementations, we'll need
more than this.
> > I wonder if that was the intention of the 'cookie' though, as a user
> > settable private data ... But maybe that's still needed to be separate
> > from this so that the allocator can track private data, while the usage
> > of the buffer is stored in the cookie?
> >
> > > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > > ---
> > > include/libcamera/framebuffer.h | 3 +++
> > > include/libcamera/internal/framebuffer.h | 1 +
> > > src/libcamera/framebuffer.cpp | 26 ++++++++++++++++++++++--
> > > 3 files changed, 28 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h
> > > index 7f2f176a..98ef9d5d 100644
> > > --- a/include/libcamera/framebuffer.h
> > > +++ b/include/libcamera/framebuffer.h
> > > @@ -58,6 +58,9 @@ public:
> > > };
> > >
> > > FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie = 0);
> > > + FrameBuffer(std::unique_ptr<Private> d,
> >
> > This bit feels a bit wierd, so I'm suspecting Laurent will have an
> > opinion here.
The alternative is to not make the FrameBuffer class final, which would
be more intrusive. We'll probably get there later, but I think this is
fine for now, even if it would be nice not to expose a constructor that
applications can't use.
> > But overall, I don't see why applications (or rather, external
> > FrameBufferAllocators) can't do this - it won't affect internal
> > libcamera usage... so I'm going to throw this on here.
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >
> > > + const std::vector<Plane> &planes, unsigned int cookie = 0);
> > > +
> > >
Extra blank line.
> > > const std::vector<Plane> &planes() const { return planes_; }
> > > Request *request() const;
> > > diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h
> > > index cd33c295..3cced5b1 100644
> > > --- a/include/libcamera/internal/framebuffer.h
> > > +++ b/include/libcamera/internal/framebuffer.h
> > > @@ -19,6 +19,7 @@ class FrameBuffer::Private : public Extensible::Private
> > >
> > > public:
> > > Private();
> > > + virtual ~Private();
> > >
> > > void setRequest(Request *request) { request_ = request; }
> > > bool isContiguous() const { return isContiguous_; }
> > > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp
> > > index d44a98ba..34e6cd4d 100644
> > > --- a/src/libcamera/framebuffer.cpp
> > > +++ b/src/libcamera/framebuffer.cpp
> > > @@ -116,6 +116,15 @@ FrameBuffer::Private::Private()
> > > {
> > > }
> > >
> > > +/**
> > > + * \fn FrameBuffer::Private::~Private()
This line isn't needed, it's implicit when the documentation block is
right above the function it documents.
> > > + * \brief FrameBuffer::Private destructor
> > > + *
Extra blank line.
> > > + */
> > > +FrameBuffer::Private::~Private()
> > > +{
> > > +}
> > > +
> > > /**
> > > * \fn FrameBuffer::Private::setRequest()
> > > * \brief Set the request this buffer belongs to
> > > @@ -213,8 +222,21 @@ FrameBuffer::Private::Private()
> > > * \param[in] cookie Cookie
> > > */
> > > FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie)
> > > - : Extensible(std::make_unique<Private>()), planes_(planes),
> > > - cookie_(cookie)
> > > + : FrameBuffer(std::make_unique<Private>(), planes, cookie)
> > > +{
> > > +}
> > > +
> > > +/**
> > > + * \brief Construct a FrameBuffer with an extensible private class and an array
> > > + * of planes
> > > + * \param[in] d The extensible private class
> > > + * \param[in] planes The frame memory planes
> > > + * \param[in] cookie Cookie
> > > + */
> > > +FrameBuffer::FrameBuffer(std::unique_ptr<Private> d,
> > > + const std::vector<Plane> &planes,
> > > + unsigned int cookie)
> > > + : Extensible(std::move(d)), planes_(planes), cookie_(cookie)
> > > {
> > > metadata_.planes_.resize(planes_.size());
> > >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list