[libcamera-devel] [PATCH 2/4] libcamera: framebuffer: Move remaining private data to Private class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Oct 3 10:38:10 CEST 2022
On Mon, Oct 03, 2022 at 10:32:35AM +0200, Jacopo Mondi wrote:
> Hi Laurent
>
> On Sun, Oct 02, 2022 at 03:36:10AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > include/libcamera/framebuffer.h | 19 ++----
> > include/libcamera/internal/framebuffer.h | 10 +++-
> > .../mm/cros_frame_buffer_allocator.cpp | 8 +--
> > .../mm/generic_frame_buffer_allocator.cpp | 9 +--
> > src/libcamera/framebuffer.cpp | 58 ++++++++++++-------
> > src/libcamera/v4l2_videodevice.cpp | 20 ++++---
> > 6 files changed, 71 insertions(+), 53 deletions(-)
> >
> > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h
> > index 36b91d11ed79..695539993f96 100644
> > --- a/include/libcamera/framebuffer.h
> > +++ b/include/libcamera/framebuffer.h
> > @@ -59,28 +59,19 @@ public:
> > };
> >
> > FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie = 0);
> > - FrameBuffer(std::unique_ptr<Private> d,
> > - const std::vector<Plane> &planes, unsigned int cookie = 0);
> > + FrameBuffer(std::unique_ptr<Private> d);
> >
> > - const std::vector<Plane> &planes() const { return planes_; }
> > + const std::vector<Plane> &planes() const;
> > Request *request() const;
> > - const FrameMetadata &metadata() const { return metadata_; }
> > + const FrameMetadata &metadata() const;
> >
> > - uint64_t cookie() const { return cookie_; }
> > - void setCookie(uint64_t cookie) { cookie_ = cookie; }
> > + uint64_t cookie() const;
> > + void setCookie(uint64_t cookie);
> >
> > std::unique_ptr<Fence> releaseFence();
> >
> > private:
> > LIBCAMERA_DISABLE_COPY_AND_MOVE(FrameBuffer)
> > -
> > - friend class V4L2VideoDevice; /* Needed to update metadata_. */
> > -
> > - std::vector<Plane> planes_;
> > -
> > - FrameMetadata metadata_;
> > -
> > - uint64_t cookie_;
> > };
> >
> > } /* namespace libcamera */
> > diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h
> > index 8a9cc98e22e3..1f42a4fcc865 100644
> > --- a/include/libcamera/internal/framebuffer.h
> > +++ b/include/libcamera/internal/framebuffer.h
> > @@ -22,7 +22,7 @@ class FrameBuffer::Private : public Extensible::Private
> > LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)
> >
> > public:
> > - Private();
> > + Private(const std::vector<Plane> &planes, uint64_t cookie = 0);
> > virtual ~Private();
> >
> > void setRequest(Request *request) { request_ = request; }
> > @@ -31,9 +31,15 @@ public:
> > Fence *fence() const { return fence_.get(); }
> > void setFence(std::unique_ptr<Fence> fence) { fence_ = std::move(fence); }
> >
> > - void cancel() { LIBCAMERA_O_PTR()->metadata_.status = FrameMetadata::FrameCancelled; }
> > + void cancel() { metadata_.status = FrameMetadata::FrameCancelled; }
> > +
> > + FrameMetadata &metadata() { return metadata_; }
> >
> > private:
> > + std::vector<Plane> planes_;
> > + FrameMetadata metadata_;
> > + uint64_t cookie_;
> > +
> > std::unique_ptr<Fence> fence_;
> > Request *request_;
> > bool isContiguous_;
> > diff --git a/src/android/mm/cros_frame_buffer_allocator.cpp b/src/android/mm/cros_frame_buffer_allocator.cpp
> > index 52e8c180e3db..1e6f1ef55434 100644
> > --- a/src/android/mm/cros_frame_buffer_allocator.cpp
> > +++ b/src/android/mm/cros_frame_buffer_allocator.cpp
> > @@ -28,8 +28,9 @@ class CrosFrameBufferData : public FrameBuffer::Private
> > LIBCAMERA_DECLARE_PUBLIC(FrameBuffer)
> >
> > public:
> > - CrosFrameBufferData(cros::ScopedBufferHandle scopedHandle)
> > - : FrameBuffer::Private(), scopedHandle_(std::move(scopedHandle))
> > + CrosFrameBufferData(cros::ScopedBufferHandle scopedHandle,
> > + const std::vector<Plane> &planes)
> > + : FrameBuffer::Private(planes), scopedHandle_(std::move(scopedHandle))
> > {
> > }
> >
> > @@ -81,8 +82,7 @@ PlatformFrameBufferAllocator::Private::allocate(int halPixelFormat,
> > }
> >
> > return std::make_unique<FrameBuffer>(
> > - std::make_unique<CrosFrameBufferData>(std::move(scopedHandle)),
> > - planes);
> > + std::make_unique<CrosFrameBufferData>(std::move(scopedHandle), planes));
> > }
> >
> > PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION
> > diff --git a/src/android/mm/generic_frame_buffer_allocator.cpp b/src/android/mm/generic_frame_buffer_allocator.cpp
> > index acb2fa2b699d..8ff4241f1506 100644
> > --- a/src/android/mm/generic_frame_buffer_allocator.cpp
> > +++ b/src/android/mm/generic_frame_buffer_allocator.cpp
> > @@ -32,8 +32,10 @@ class GenericFrameBufferData : public FrameBuffer::Private
> >
> > public:
> > GenericFrameBufferData(struct alloc_device_t *allocDevice,
> > - buffer_handle_t handle)
> > - : allocDevice_(allocDevice), handle_(handle)
> > + buffer_handle_t handle,
> > + const std::vector<Plane> &planes)
> > + : FrameBuffer::Private(planes), allocDevice_(allocDevice),
> > + handle_(handle)
> > {
> > ASSERT(allocDevice_);
> > ASSERT(handle_);
> > @@ -136,8 +138,7 @@ PlatformFrameBufferAllocator::Private::allocate(int halPixelFormat,
> > }
> >
> > return std::make_unique<FrameBuffer>(
> > - std::make_unique<GenericFrameBufferData>(allocDevice_, handle),
> > - planes);
> > + std::make_unique<GenericFrameBufferData>(allocDevice_, handle, planes));
> > }
> >
> > PUBLIC_FRAME_BUFFER_ALLOCATOR_IMPLEMENTATION
> > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp
> > index 7be18560417c..5a7f3c0b5f9a 100644
> > --- a/src/libcamera/framebuffer.cpp
> > +++ b/src/libcamera/framebuffer.cpp
> > @@ -114,9 +114,16 @@ LOG_DEFINE_CATEGORY(Buffer)
> > * pipeline handlers.
> > */
> >
> > -FrameBuffer::Private::Private()
> > - : request_(nullptr), isContiguous_(true)
> > +/**
> > + * \brief Construct a FrameBuffer::Private instance
> > + * \param[in] planes The frame memory planes
> > + * \param[in] cookie Cookie
> > + */
> > +FrameBuffer::Private::Private(const std::vector<Plane> &planes, uint64_t cookie)
> > + : planes_(planes), cookie_(cookie), request_(nullptr),
> > + isContiguous_(true)
> > {
> > + metadata_.planes_.resize(planes_.size());
> > }
> >
> > /**
> > @@ -194,6 +201,12 @@ FrameBuffer::Private::~Private()
> > * indicate that the metadata is invalid.
> > */
> >
> > +/**
> > + * \fn FrameBuffer::Private::metadata()
> > + * \brief Retrieve the dynamic metadata
> > + * \return Dynamic metadata for the frame contained in the buffer
> > + */
> > +
> > /**
> > * \class FrameBuffer
> > * \brief Frame buffer data and its associated dynamic metadata
> > @@ -291,29 +304,22 @@ ino_t fileDescriptorInode(const SharedFD &fd)
> > * \param[in] cookie Cookie
> > */
> > FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie)
> > - : FrameBuffer(std::make_unique<Private>(), planes, cookie)
> > + : FrameBuffer(std::make_unique<Private>(planes, cookie))
> > {
> > }
> >
> > /**
> > - * \brief Construct a FrameBuffer with an extensible private class and an array
> > - * of planes
> > + * \brief Construct a FrameBuffer with an extensible private class
> > * \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)
> > +FrameBuffer::FrameBuffer(std::unique_ptr<Private> d)
> > + : Extensible(std::move(d))
> > {
> > - metadata_.planes_.resize(planes_.size());
> > -
> > unsigned int offset = 0;
> > bool isContiguous = true;
> > ino_t inode = 0;
> >
> > - for (const auto &plane : planes_) {
> > + for (const auto &plane : _d()->planes_) {
> > ASSERT(plane.offset != Plane::kInvalidOffset);
> >
> > if (plane.offset != offset) {
> > @@ -325,9 +331,9 @@ FrameBuffer::FrameBuffer(std::unique_ptr<Private> d,
> > * Two different dmabuf file descriptors may still refer to the
> > * same dmabuf instance. Check this using inodes.
> > */
> > - if (plane.fd != planes_[0].fd) {
> > + if (plane.fd != _d()->planes_[0].fd) {
> > if (!inode)
> > - inode = fileDescriptorInode(planes_[0].fd);
> > + inode = fileDescriptorInode(_d()->planes_[0].fd);
> > if (fileDescriptorInode(plane.fd) != inode) {
> > isContiguous = false;
> > break;
> > @@ -344,10 +350,13 @@ FrameBuffer::FrameBuffer(std::unique_ptr<Private> d,
> > }
> >
> > /**
> > - * \fn FrameBuffer::planes()
> > * \brief Retrieve the static plane descriptors
> > * \return Array of plane descriptors
> > */
> > +const std::vector<FrameBuffer::Plane> &FrameBuffer::planes() const
> > +{
> > + return _d()->planes_;
> > +}
> >
> > /**
> > * \brief Retrieve the request this buffer belongs to
> > @@ -368,13 +377,15 @@ Request *FrameBuffer::request() const
> > }
> >
> > /**
> > - * \fn FrameBuffer::metadata()
> > * \brief Retrieve the dynamic metadata
> > * \return Dynamic metadata for the frame contained in the buffer
> > */
> > +const FrameMetadata &FrameBuffer::metadata() const
> > +{
> > + return _d()->metadata_;
> > +}
> >
> > /**
> > - * \fn FrameBuffer::cookie()
> > * \brief Retrieve the cookie
> > *
> > * The cookie belongs to the creator of the FrameBuffer, which controls its
> > @@ -384,9 +395,12 @@ Request *FrameBuffer::request() const
> > *
> > * \return The cookie
> > */
> > +uint64_t FrameBuffer::cookie() const
> > +{
> > + return _d()->cookie_;
> > +}
> >
> > /**
> > - * \fn FrameBuffer::setCookie()
> > * \brief Set the cookie
> > * \param[in] cookie Cookie to set
> > *
> > @@ -395,6 +409,10 @@ Request *FrameBuffer::request() const
> > * modify the cookie value of buffers they haven't created themselves. The
> > * libcamera core never modifies the buffer cookie.
> > */
> > +void FrameBuffer::setCookie(uint64_t cookie)
> > +{
> > + _d()->cookie_ = cookie;
> > +}
> >
> > /**
> > * \brief Extract the Fence associated with this Framebuffer
> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > index 1dbb839ed456..d25a785adeea 100644
> > --- a/src/libcamera/v4l2_videodevice.cpp
> > +++ b/src/libcamera/v4l2_videodevice.cpp
> > @@ -1793,12 +1793,14 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()
> > watchdog_.start(std::chrono::duration_cast<std::chrono::milliseconds>(watchdogDuration_));
> > }
> >
> > - buffer->metadata_.status = buf.flags & V4L2_BUF_FLAG_ERROR
> > - ? FrameMetadata::FrameError
> > - : FrameMetadata::FrameSuccess;
> > - buffer->metadata_.sequence = buf.sequence;
> > - buffer->metadata_.timestamp = buf.timestamp.tv_sec * 1000000000ULL
> > - + buf.timestamp.tv_usec * 1000ULL;
> > + FrameMetadata &metadata = buffer->_d()->metadata();
>
> A very minor nit: in queueBuffer() we use the public FrameBuffer::metadata()
> function. Might be nice to use the same pattern
FrameBuffer::metadata() returns a const reference.
> That apart
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>
> > +
> > + metadata.status = buf.flags & V4L2_BUF_FLAG_ERROR
> > + ? FrameMetadata::FrameError
> > + : FrameMetadata::FrameSuccess;
> > + metadata.sequence = buf.sequence;
> > + metadata.timestamp = buf.timestamp.tv_sec * 1000000000ULL
> > + + buf.timestamp.tv_usec * 1000ULL;
> >
> > if (V4L2_TYPE_IS_OUTPUT(buf.type))
> > return buffer;
> > @@ -1814,10 +1816,9 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()
> > << buf.sequence << ")";
> > firstFrame_ = buf.sequence;
> > }
> > - buffer->metadata_.sequence -= firstFrame_.value();
> > + metadata.sequence -= firstFrame_.value();
> >
> > unsigned int numV4l2Planes = multiPlanar ? buf.length : 1;
> > - FrameMetadata &metadata = buffer->metadata_;
> >
> > if (numV4l2Planes != buffer->planes().size()) {
> > /*
> > @@ -1943,9 +1944,10 @@ int V4L2VideoDevice::streamOff()
> > /* Send back all queued buffers. */
> > for (auto it : queuedBuffers_) {
> > FrameBuffer *buffer = it.second;
> > + FrameMetadata &metadata = buffer->_d()->metadata();
> >
> > cache_->put(it.first);
> > - buffer->metadata_.status = FrameMetadata::FrameCancelled;
> > + metadata.status = FrameMetadata::FrameCancelled;
> > bufferReady.emit(buffer);
> > }
> >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list