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

[Hirokazu Honda]

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.

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