[libcamera-devel,2/4] libcamera: framebuffer: Move remaining private data to Private class
diff mbox series

Message ID 20221002003612.13603-3-laurent.pinchart@ideasonboard.com
State Superseded
Headers show
Series
  • libcamera: Fix kernel deprecation warning with output buffers
Related show

Commit Message

Laurent Pinchart Oct. 2, 2022, 12:36 a.m. UTC 
Signed-off-by: Laurent Pinchart <laurent.pinchart@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(-)

Comments

Jacopo Mondi Oct. 3, 2022, 8:32 a.m. UTC | #1 
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@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

That apart
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

> +
> +	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
>
Laurent Pinchart Oct. 3, 2022, 8:38 a.m. UTC | #2 
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@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@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);
> >  	}
> >

Patch
diff mbox series 

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();
+
+	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);
 	}