[libcamera-devel] [PATCH v2 1/2] libcamera: framebuffer: Enable attaching additional data to FrameBuffer

[Jacopo Mondi]

Hi Kieran

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.

>
> 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.
>
> 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);
> > +
> >
> >         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()
> > + * \brief FrameBuffer::Private destructor
> > + *
> > + */
> > +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());
> >
> > --
> > 2.33.1.1089.g2158813163f-goog
> >