[libcamera-devel] [PATCH 1/2] libcamera: framebuffer: Enable attaching additional data to FrameBuffer
Hirokazu Honda
hiroh at chromium.org
Thu Nov 4 02:23:13 CET 2021
Hi Jacopo,
On Tue, Nov 2, 2021 at 5:59 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> 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 :)
>
I got it. Thanks for clarifying.
-Hiro
> > >
> > >
> > > > 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