[libcamera-devel] [PATCH] libcamera: framebuffer: Return plane begin address by MappedBuffer::maps()

[Kieran Bingham]

Hi Hiro,

On 05/08/2021 18:49, Hirokazu Honda wrote:
> MappedBuffer::maps() returns std::vector<MappedBuffer::Plane>.
> Plane has the address, but the address points the beginning of the
> buffer containing the plane.
> This makes the Plane point the beginning of the plane. So
> MappedBuffer::maps()[i].data() returns the address of i-th plane.

If we can handle the logic in MappedBuffer to get the correct addresses
that would certainly be beneficial indeed, so this sounds like good
development.


> 
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  include/libcamera/internal/framebuffer.h |  1 +
>  src/libcamera/framebuffer.cpp            | 41 +++++++++++++++++-------
>  2 files changed, 31 insertions(+), 11 deletions(-)
> 
> diff --git a/include/libcamera/internal/framebuffer.h b/include/libcamera/internal/framebuffer.h
> index 8c187adf..2eb3b7c9 100644
> --- a/include/libcamera/internal/framebuffer.h
> +++ b/include/libcamera/internal/framebuffer.h
> @@ -36,6 +36,7 @@ protected:
>  
>  	int error_;
>  	std::vector<Plane> maps_;
> +	std::vector<Plane> chunks_;
>  
>  private:
>  	LIBCAMERA_DISABLE_COPY(MappedBuffer)
> diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp
> index a59e93fb..8231c1df 100644
> --- a/src/libcamera/framebuffer.cpp
> +++ b/src/libcamera/framebuffer.cpp
> @@ -310,6 +310,7 @@ MappedBuffer &MappedBuffer::operator=(MappedBuffer &&other)
>  {
>  	error_ = other.error_;
>  	maps_ = std::move(other.maps_);
> +	chunks_ = std::move(other.chunks_);
>  	other.error_ = -ENOENT;
>  
>  	return *this;
> @@ -317,8 +318,8 @@ MappedBuffer &MappedBuffer::operator=(MappedBuffer &&other)
>  
>  MappedBuffer::~MappedBuffer()
>  {
> -	for (Plane &map : maps_)
> -		munmap(map.data(), map.size());
> +	for (Plane &chunk : chunks_)
> +		munmap(chunk.data(), chunk.size());
>  }
>  
>  /**
> @@ -381,18 +382,36 @@ MappedBuffer::~MappedBuffer()
>   */
>  MappedFrameBuffer::MappedFrameBuffer(const FrameBuffer *buffer, int flags)
>  {
> +	if (buffer->planes().empty())
> +		return;

Can we have a buffer with zero planes? Is this something that should
generate an Error or Warning message?

(Or if it really implies there's a bug, a Fatal?)


>  	maps_.reserve(buffer->planes().size());
>  
> +	/*
> +	 * Below assumes planes are in the same buffer and planes are
> +	 * consecutive.
> +	 * \todo remove this assumption.

To do that, will we need to store the PixelFormat information as part of
the FrameBuffer? Or at least some additional data there so that we know
it's layout?


And - now I've seen Laurent posted a patch to expose a Format for that
purpose...



> +	 */
> +	const int fd = buffer->planes()[0].fd.fd();
> +	const off_t length = lseek(fd, 0, SEEK_END);
> +	if (length < 0) {
> +		error_ = -errno;
> +		LOG(Buffer, Error) << "Failed to lseek buffer";
> +		return;
> +	}
> +
> +	void *address = mmap(nullptr, length, flags, MAP_SHARED, fd, 0);
> +	if (address == MAP_FAILED) {
> +		error_ = -errno;
> +		LOG(Buffer, Error) << "Failed to mmap plane";
> +		return;
> +	}
> +	chunks_ = { MappedBuffer::Plane(static_cast<uint8_t *>(address),
> +					static_cast<size_t>(length)) };
> +	size_t offset = 0;
>  	for (const FrameBuffer::Plane &plane : buffer->planes()) {
> -		void *address = mmap(nullptr, plane.length, flags,
> -				     MAP_SHARED, plane.fd.fd(), 0);
> -		if (address == MAP_FAILED) {
> -			error_ = -errno;
> -			LOG(Buffer, Error) << "Failed to mmap plane";
> -			break;
> -		}
> -
> -		maps_.emplace_back(static_cast<uint8_t *>(address), plane.length);
> +		maps_.emplace_back(static_cast<uint8_t *>(address) + offset,
> +				   plane.length);
> +		offset += plane.length;
>  	}
>  }
>  
>