| Message ID | 20211028073038.653786-2-hiroh@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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. 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@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 >
Hi Jacopo, On Fri, Oct 29, 2021 at 11:39 PM Jacopo Mondi <jacopo@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@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@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 > >
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@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@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@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 > > >
Hi Jacopo, On Tue, Nov 2, 2021 at 5:59 PM Jacopo Mondi <jacopo@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@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@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@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 > > > >
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); FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie = 0); 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; 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());
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@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(-)