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

Jacopo Mondi jacopo at jmondi.org
Tue Nov 2 10:00:46 CET 2021


Hi Hiro,

On Mon, Nov 01, 2021 at 01:54:09PM +0900, Hirokazu Honda wrote:
> Hi Jacopo,
>
> On Fri, Oct 29, 2021 at 11:39 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
> >
> > Hi Hiro,
> >
> > On Thu, Oct 28, 2021 at 04:30:37PM +0900, Hirokazu Honda wrote:
> > > 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.
> > >
> > > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > > ---
> > >  include/libcamera/framebuffer.h          |  2 ++
> > >  include/libcamera/internal/framebuffer.h |  1 +
> > >  src/libcamera/framebuffer.cpp            | 17 +++++++++++++++--
> > >  3 files changed, 18 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h
> > > index 7f2f176a..02bf1533 100644
> > > --- a/include/libcamera/framebuffer.h
> > > +++ b/include/libcamera/framebuffer.h
> > > @@ -57,6 +57,8 @@ public:
> > >               unsigned int length;
> > >       };
> > >
> > > +     FrameBuffer(std::unique_ptr<Private> d,
> > > +                 const std::vector<Plane> &planes, unsigned int cookie = 0);
> >
> > I wonder if it's right to make this available from a public
> > header. We have this pattern in use for Camera, which cannot however be
> > constructed from the libcamera public API, while FrameBuffer can.
>
> Could you elaborate?
> Camera::create() takes Camera::Private and it can be called by
> libcamera public API.

Sorry, I wrote the first paragraph, then the second.

Camera::create() cannot be called by application as they don't have
access to the full definition of Camera::Private so they cannot extend
that

>
> -Hiro
> >
> > Applications cannot extend FrameBuffer::Private though, so only
> > components with access to the internal headers can, so I guess this is
> > fine!

The same happens in your patches for FrameBuffer::Private, so this is
fine.

Sorry, I should have deleted the first paragraph :)

> >
> >
> > >       FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie = 0);
> >
> > Could you align the declaration order in the .cpp and .h files ?
> >
> > Alternatively, we can decide FrameBuffer can potentially store some
> > PrivateData in its FrameBuffer::Private in the case we ever get to
> > provide an allocator based on dmabuf heaps. But that's maybe for
> > later.
> >
> > >
> > >       const std::vector<Plane> &planes() const { return planes_; }
> > > diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h
> > > index cd33c295..5ab9b3b2 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() = default;
> >
> > Recently I think we're pushing to move the '= default' declarations to
> > the .cpp files to avoid inlining then in the headers.
> >
> > Overall I think that's good!
> >
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> >
> > >
> > >       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..26587ff3 100644
> > > --- a/src/libcamera/framebuffer.cpp
> > > +++ b/src/libcamera/framebuffer.cpp
> > > @@ -213,8 +213,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
> > >


More information about the libcamera-devel mailing list