[libcamera-devel,v3,2/3] android: camera_buffer: Map buffer in the first plane() call
diff mbox series

Message ID 20210826080021.3454520-3-hiroh@chromium.org
State Accepted
Headers show
Series
  • Improve CameraBuffer implementation
Related show

Commit Message

Hirokazu Honda Aug. 26, 2021, 8 a.m. UTC 
CameraBuffer implementation maps a given buffer_handle_t in
constructor. Mapping is redundant to only know the plane info like
stride and offset. Mapping should be executed rater in the first
plane() call.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
---
 src/android/mm/cros_camera_buffer.cpp    | 71 +++++++++++--------
 src/android/mm/generic_camera_buffer.cpp | 86 ++++++++++++++++--------
 2 files changed, 100 insertions(+), 57 deletions(-)

Comments

Laurent Pinchart Aug. 26, 2021, 12:06 p.m. UTC | #1 
Hi Hiro,

Thank you for the patch.

On Thu, Aug 26, 2021 at 05:00:20PM +0900, Hirokazu Honda wrote:
> CameraBuffer implementation maps a given buffer_handle_t in
> constructor. Mapping is redundant to only know the plane info like
> stride and offset. Mapping should be executed rater in the first
> plane() call.
> 
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> ---
>  src/android/mm/cros_camera_buffer.cpp    | 71 +++++++++++--------
>  src/android/mm/generic_camera_buffer.cpp | 86 ++++++++++++++++--------
>  2 files changed, 100 insertions(+), 57 deletions(-)
> 
> diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp
> index 50732637..ba6650cf 100644
> --- a/src/android/mm/cros_camera_buffer.cpp
> +++ b/src/android/mm/cros_camera_buffer.cpp
> @@ -25,7 +25,7 @@ public:
>  		int flags);
>  	~Private();
>  
> -	bool isValid() const { return valid_; }
> +	bool isValid() const { return registered_; }
>  
>  	unsigned int numPlanes() const;
>  
> @@ -34,10 +34,12 @@ public:
>  	size_t jpegBufferSize(size_t maxJpegBufferSize) const;
>  
>  private:
> +	void map();
> +
>  	cros::CameraBufferManager *bufferManager_;
>  	buffer_handle_t handle_;
>  	unsigned int numPlanes_;
> -	bool valid_;
> +	bool mapped_;
>  	bool registered_;
>  	union {
>  		void *addr;
> @@ -50,7 +52,7 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
>  			       [[maybe_unused]] libcamera::PixelFormat pixelFormat,
>  			       [[maybe_unused]] const libcamera::Size &size,
>  			       [[maybe_unused]] int flags)
> -	: handle_(camera3Buffer), numPlanes_(0), valid_(false),
> +	: handle_(camera3Buffer), numPlanes_(0), mapped_(false),
>  	  registered_(false)
>  {
>  	bufferManager_ = cros::CameraBufferManager::GetInstance();
> @@ -63,36 +65,11 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
>  
>  	registered_ = true;
>  	numPlanes_ = bufferManager_->GetNumPlanes(camera3Buffer);
> -	switch (numPlanes_) {
> -	case 1: {
> -		ret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);
> -		if (ret) {
> -			LOG(HAL, Error) << "Single plane buffer mapping failed";
> -			return;
> -		}
> -		break;
> -	}
> -	case 2:
> -	case 3: {
> -		ret = bufferManager_->LockYCbCr(handle_, 0, 0, 0, 0, 0,
> -						&mem.ycbcr);
> -		if (ret) {
> -			LOG(HAL, Error) << "YCbCr buffer mapping failed";
> -			return;
> -		}
> -		break;
> -	}
> -	default:
> -		LOG(HAL, Error) << "Invalid number of planes: " << numPlanes_;
> -		return;
> -	}
> -
> -	valid_ = true;
>  }
>  
>  CameraBuffer::Private::~Private()
>  {
> -	if (valid_)
> +	if (mapped_)
>  		bufferManager_->Unlock(handle_);
>  	if (registered_)
>  		bufferManager_->Deregister(handle_);
> @@ -105,6 +82,11 @@ unsigned int CameraBuffer::Private::numPlanes() const
>  
>  Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane)
>  {
> +	if (!mapped_)
> +		map();
> +	if (!mapped_)
> +		return {};
> +
>  	void *addr;
>  
>  	switch (numPlanes()) {
> @@ -134,4 +116,35 @@ size_t CameraBuffer::Private::jpegBufferSize([[maybe_unused]] size_t maxJpegBuff
>  	return bufferManager_->GetPlaneSize(handle_, 0);
>  }
>  
> +void CameraBuffer::Private::map()
> +{
> +	int ret;
> +	switch (numPlanes_) {
> +	case 1: {
> +		ret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);
> +		if (ret) {
> +			LOG(HAL, Error) << "Single plane buffer mapping failed";
> +			return;
> +		}
> +		break;
> +	}
> +	case 2:
> +	case 3: {
> +		ret = bufferManager_->LockYCbCr(handle_, 0, 0, 0, 0, 0,
> +						&mem.ycbcr);
> +		if (ret) {
> +			LOG(HAL, Error) << "YCbCr buffer mapping failed";
> +			return;
> +		}
> +		break;
> +	}
> +	default:
> +		LOG(HAL, Error) << "Invalid number of planes: " << numPlanes_;
> +		return;
> +	}
> +
> +	mapped_ = true;
> +	return;
> +}
> +
>  PUBLIC_CAMERA_BUFFER_IMPLEMENTATION
> diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp
> index 22753490..cfe55b69 100644
> --- a/src/android/mm/generic_camera_buffer.cpp
> +++ b/src/android/mm/generic_camera_buffer.cpp
> @@ -37,6 +37,18 @@ public:
>  	size_t jpegBufferSize(size_t maxJpegBufferSize) const;
>  
>  private:
> +	struct PlaneInfo {
> +		unsigned int offset;
> +		unsigned int size;
> +	};
> +
> +	void map();
> +
> +	int fd_;
> +	int flags_;
> +	off_t bufferLength_;
> +	bool mapped_;
> +	std::vector<PlaneInfo> planeInfo_;
>  	/* \todo Remove planes_ when it will be added to MappedBuffer */
>  	std::vector<Span<uint8_t>> planes_;
>  };
> @@ -45,6 +57,7 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
>  			       buffer_handle_t camera3Buffer,
>  			       libcamera::PixelFormat pixelFormat,
>  			       const libcamera::Size &size, int flags)
> +	: fd_(-1), flags_(flags), mapped_(false)

This should initialize bufferLength_ to 0, otherwise the ASSERT()s that
checks the variable will be pointless if the constructor returns an
error before setting bufferLength_.

And it will also avoid a Coverity warning :-)

I can handle this when pushing the series.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  {
>  	error_ = 0;
>  
> @@ -61,43 +74,35 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
>  	 * now that the buffer is backed by a single dmabuf, with planes being
>  	 * stored contiguously.
>  	 */
> -	int fd = -1;
>  	for (int i = 0; i < camera3Buffer->numFds; i++) {
> -		if (camera3Buffer->data[i] == -1 || camera3Buffer->data[i] == fd)
> +		if (camera3Buffer->data[i] == -1 || camera3Buffer->data[i] == fd_)
>  			continue;
>  
> -		if (fd != -1) {
> +		if (fd_ != -1) {
>  			error_ = -EINVAL;
>  			LOG(HAL, Error) << "Discontiguous planes are not supported";
>  			return;
>  		}
>  
> -		fd = camera3Buffer->data[i];
> +		fd_ = camera3Buffer->data[i];
>  	}
>  
> -	if (fd == -1) {
> +	if (fd_ == -1) {
>  		error_ = -EINVAL;
>  		LOG(HAL, Error) << "No valid file descriptor";
>  		return;
>  	}
>  
> -	off_t bufferLength = lseek(fd, 0, SEEK_END);
> -	if (bufferLength < 0) {
> +	bufferLength_ = lseek(fd_, 0, SEEK_END);
> +	if (bufferLength_ < 0) {
>  		error_ = -errno;
>  		LOG(HAL, Error) << "Failed to get buffer length";
>  		return;
>  	}
>  
> -	void *address = mmap(nullptr, bufferLength, flags, MAP_SHARED, fd, 0);
> -	if (address == MAP_FAILED) {
> -		error_ = -errno;
> -		LOG(HAL, Error) << "Failed to mmap plane";
> -		return;
> -	}
> -	maps_.emplace_back(static_cast<uint8_t *>(address), bufferLength);
> -
>  	const unsigned int numPlanes = info.numPlanes();
> -	planes_.resize(numPlanes);
> +	planeInfo_.resize(numPlanes);
> +
>  	unsigned int offset = 0;
>  	for (unsigned int i = 0; i < numPlanes; ++i) {
>  		/*
> @@ -109,17 +114,17 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
>  		const unsigned int planeSize =
>  			stride * ((size.height + vertSubSample - 1) / vertSubSample);
>  
> -		planes_[i] = libcamera::Span<uint8_t>(
> -			static_cast<uint8_t *>(address) + offset, planeSize);
> +		planeInfo_[i].offset = offset;
> +		planeInfo_[i].size = planeSize;
>  
> -		if (bufferLength < offset + planeSize) {
> -			error_ = -EINVAL;
> -			LOG(HAL, Error) << "Plane " << i << " is out of buffer"
> -					<< ", buffer length=" << bufferLength
> -					<< ", offset=" << offset
> -					<< ", size=" << planeSize;
> +		if (bufferLength_ < offset + planeSize) {
> +			LOG(HAL, Error) << "Plane " << i << " is out of buffer:"
> +					<< " plane offset=" << offset
> +					<< ", plane size=" << planeSize
> +					<< ", buffer length=" << bufferLength_;
>  			return;
>  		}
> +
>  		offset += planeSize;
>  	}
>  }
> @@ -130,12 +135,14 @@ CameraBuffer::Private::~Private()
>  
>  unsigned int CameraBuffer::Private::numPlanes() const
>  {
> -	return planes_.size();
> +	return planeInfo_.size();
>  }
>  
>  Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane)
>  {
> -	if (plane >= planes_.size())
> +	if (!mapped_)
> +		map();
> +	if (!mapped_)
>  		return {};
>  
>  	return planes_[plane];
> @@ -143,8 +150,31 @@ Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane)
>  
>  size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const
>  {
> -	return std::min<unsigned int>(maps_[0].size(),
> -				      maxJpegBufferSize);
> +	ASSERT(bufferLength_ >= 0);
> +
> +	return std::min<unsigned int>(bufferLength_, maxJpegBufferSize);
> +}
> +
> +void CameraBuffer::Private::map()
> +{
> +	ASSERT(fd_ != -1);
> +	ASSERT(bufferLength_ >= 0);
> +
> +	void *address = mmap(nullptr, bufferLength_, flags_, MAP_SHARED, fd_, 0);
> +	if (address == MAP_FAILED) {
> +		error_ = -errno;
> +		LOG(HAL, Error) << "Failed to mmap plane";
> +		return;
> +	}
> +	maps_.emplace_back(static_cast<uint8_t *>(address), bufferLength_);
> +
> +	planes_.reserve(planeInfo_.size());
> +	for (const auto &info : planeInfo_) {
> +		planes_.emplace_back(
> +			static_cast<uint8_t *>(address) + info.offset, info.size);
> +	}
> +
> +	mapped_ = true;
>  }
>  
>  PUBLIC_CAMERA_BUFFER_IMPLEMENTATION
Jacopo Mondi Aug. 26, 2021, 9:45 p.m. UTC | #2 
Hi Hiro,

On Thu, Aug 26, 2021 at 05:00:20PM +0900, Hirokazu Honda wrote:
> CameraBuffer implementation maps a given buffer_handle_t in
> constructor. Mapping is redundant to only know the plane info like
> stride and offset. Mapping should be executed rater in the first
> plane() call.

With the issue pointed out by Laurent
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

>
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> ---
>  src/android/mm/cros_camera_buffer.cpp    | 71 +++++++++++--------
>  src/android/mm/generic_camera_buffer.cpp | 86 ++++++++++++++++--------
>  2 files changed, 100 insertions(+), 57 deletions(-)
>
> diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp
> index 50732637..ba6650cf 100644
> --- a/src/android/mm/cros_camera_buffer.cpp
> +++ b/src/android/mm/cros_camera_buffer.cpp
> @@ -25,7 +25,7 @@ public:
>  		int flags);
>  	~Private();
>
> -	bool isValid() const { return valid_; }
> +	bool isValid() const { return registered_; }
>
>  	unsigned int numPlanes() const;
>
> @@ -34,10 +34,12 @@ public:
>  	size_t jpegBufferSize(size_t maxJpegBufferSize) const;
>
>  private:
> +	void map();
> +
>  	cros::CameraBufferManager *bufferManager_;
>  	buffer_handle_t handle_;
>  	unsigned int numPlanes_;
> -	bool valid_;
> +	bool mapped_;
>  	bool registered_;
>  	union {
>  		void *addr;
> @@ -50,7 +52,7 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
>  			       [[maybe_unused]] libcamera::PixelFormat pixelFormat,
>  			       [[maybe_unused]] const libcamera::Size &size,
>  			       [[maybe_unused]] int flags)
> -	: handle_(camera3Buffer), numPlanes_(0), valid_(false),
> +	: handle_(camera3Buffer), numPlanes_(0), mapped_(false),
>  	  registered_(false)
>  {
>  	bufferManager_ = cros::CameraBufferManager::GetInstance();
> @@ -63,36 +65,11 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
>
>  	registered_ = true;
>  	numPlanes_ = bufferManager_->GetNumPlanes(camera3Buffer);
> -	switch (numPlanes_) {
> -	case 1: {
> -		ret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);
> -		if (ret) {
> -			LOG(HAL, Error) << "Single plane buffer mapping failed";
> -			return;
> -		}
> -		break;
> -	}
> -	case 2:
> -	case 3: {
> -		ret = bufferManager_->LockYCbCr(handle_, 0, 0, 0, 0, 0,
> -						&mem.ycbcr);
> -		if (ret) {
> -			LOG(HAL, Error) << "YCbCr buffer mapping failed";
> -			return;
> -		}
> -		break;
> -	}
> -	default:
> -		LOG(HAL, Error) << "Invalid number of planes: " << numPlanes_;
> -		return;
> -	}
> -
> -	valid_ = true;
>  }
>
>  CameraBuffer::Private::~Private()
>  {
> -	if (valid_)
> +	if (mapped_)
>  		bufferManager_->Unlock(handle_);
>  	if (registered_)
>  		bufferManager_->Deregister(handle_);
> @@ -105,6 +82,11 @@ unsigned int CameraBuffer::Private::numPlanes() const
>
>  Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane)
>  {
> +	if (!mapped_)
> +		map();
> +	if (!mapped_)
> +		return {};
> +
>  	void *addr;
>
>  	switch (numPlanes()) {
> @@ -134,4 +116,35 @@ size_t CameraBuffer::Private::jpegBufferSize([[maybe_unused]] size_t maxJpegBuff
>  	return bufferManager_->GetPlaneSize(handle_, 0);
>  }
>
> +void CameraBuffer::Private::map()
> +{
> +	int ret;
> +	switch (numPlanes_) {
> +	case 1: {
> +		ret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);
> +		if (ret) {
> +			LOG(HAL, Error) << "Single plane buffer mapping failed";
> +			return;
> +		}
> +		break;
> +	}
> +	case 2:
> +	case 3: {
> +		ret = bufferManager_->LockYCbCr(handle_, 0, 0, 0, 0, 0,
> +						&mem.ycbcr);
> +		if (ret) {
> +			LOG(HAL, Error) << "YCbCr buffer mapping failed";
> +			return;
> +		}
> +		break;
> +	}
> +	default:
> +		LOG(HAL, Error) << "Invalid number of planes: " << numPlanes_;
> +		return;
> +	}
> +
> +	mapped_ = true;
> +	return;
> +}
> +
>  PUBLIC_CAMERA_BUFFER_IMPLEMENTATION
> diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp
> index 22753490..cfe55b69 100644
> --- a/src/android/mm/generic_camera_buffer.cpp
> +++ b/src/android/mm/generic_camera_buffer.cpp
> @@ -37,6 +37,18 @@ public:
>  	size_t jpegBufferSize(size_t maxJpegBufferSize) const;
>
>  private:
> +	struct PlaneInfo {
> +		unsigned int offset;
> +		unsigned int size;
> +	};
> +
> +	void map();
> +
> +	int fd_;
> +	int flags_;
> +	off_t bufferLength_;
> +	bool mapped_;
> +	std::vector<PlaneInfo> planeInfo_;
>  	/* \todo Remove planes_ when it will be added to MappedBuffer */
>  	std::vector<Span<uint8_t>> planes_;
>  };
> @@ -45,6 +57,7 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
>  			       buffer_handle_t camera3Buffer,
>  			       libcamera::PixelFormat pixelFormat,
>  			       const libcamera::Size &size, int flags)
> +	: fd_(-1), flags_(flags), mapped_(false)
>  {
>  	error_ = 0;
>
> @@ -61,43 +74,35 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
>  	 * now that the buffer is backed by a single dmabuf, with planes being
>  	 * stored contiguously.
>  	 */
> -	int fd = -1;
>  	for (int i = 0; i < camera3Buffer->numFds; i++) {
> -		if (camera3Buffer->data[i] == -1 || camera3Buffer->data[i] == fd)
> +		if (camera3Buffer->data[i] == -1 || camera3Buffer->data[i] == fd_)
>  			continue;
>
> -		if (fd != -1) {
> +		if (fd_ != -1) {
>  			error_ = -EINVAL;
>  			LOG(HAL, Error) << "Discontiguous planes are not supported";
>  			return;
>  		}
>
> -		fd = camera3Buffer->data[i];
> +		fd_ = camera3Buffer->data[i];
>  	}
>
> -	if (fd == -1) {
> +	if (fd_ == -1) {
>  		error_ = -EINVAL;
>  		LOG(HAL, Error) << "No valid file descriptor";
>  		return;
>  	}
>
> -	off_t bufferLength = lseek(fd, 0, SEEK_END);
> -	if (bufferLength < 0) {
> +	bufferLength_ = lseek(fd_, 0, SEEK_END);
> +	if (bufferLength_ < 0) {
>  		error_ = -errno;
>  		LOG(HAL, Error) << "Failed to get buffer length";
>  		return;
>  	}
>
> -	void *address = mmap(nullptr, bufferLength, flags, MAP_SHARED, fd, 0);
> -	if (address == MAP_FAILED) {
> -		error_ = -errno;
> -		LOG(HAL, Error) << "Failed to mmap plane";
> -		return;
> -	}
> -	maps_.emplace_back(static_cast<uint8_t *>(address), bufferLength);
> -
>  	const unsigned int numPlanes = info.numPlanes();
> -	planes_.resize(numPlanes);
> +	planeInfo_.resize(numPlanes);
> +
>  	unsigned int offset = 0;
>  	for (unsigned int i = 0; i < numPlanes; ++i) {
>  		/*
> @@ -109,17 +114,17 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
>  		const unsigned int planeSize =
>  			stride * ((size.height + vertSubSample - 1) / vertSubSample);
>
> -		planes_[i] = libcamera::Span<uint8_t>(
> -			static_cast<uint8_t *>(address) + offset, planeSize);
> +		planeInfo_[i].offset = offset;
> +		planeInfo_[i].size = planeSize;
>
> -		if (bufferLength < offset + planeSize) {
> -			error_ = -EINVAL;
> -			LOG(HAL, Error) << "Plane " << i << " is out of buffer"
> -					<< ", buffer length=" << bufferLength
> -					<< ", offset=" << offset
> -					<< ", size=" << planeSize;
> +		if (bufferLength_ < offset + planeSize) {
> +			LOG(HAL, Error) << "Plane " << i << " is out of buffer:"
> +					<< " plane offset=" << offset
> +					<< ", plane size=" << planeSize
> +					<< ", buffer length=" << bufferLength_;
>  			return;
>  		}
> +
>  		offset += planeSize;
>  	}
>  }
> @@ -130,12 +135,14 @@ CameraBuffer::Private::~Private()
>
>  unsigned int CameraBuffer::Private::numPlanes() const
>  {
> -	return planes_.size();
> +	return planeInfo_.size();
>  }
>
>  Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane)
>  {
> -	if (plane >= planes_.size())
> +	if (!mapped_)
> +		map();
> +	if (!mapped_)
>  		return {};
>
>  	return planes_[plane];
> @@ -143,8 +150,31 @@ Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane)
>
>  size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const
>  {
> -	return std::min<unsigned int>(maps_[0].size(),
> -				      maxJpegBufferSize);
> +	ASSERT(bufferLength_ >= 0);
> +
> +	return std::min<unsigned int>(bufferLength_, maxJpegBufferSize);
> +}
> +
> +void CameraBuffer::Private::map()
> +{
> +	ASSERT(fd_ != -1);
> +	ASSERT(bufferLength_ >= 0);
> +
> +	void *address = mmap(nullptr, bufferLength_, flags_, MAP_SHARED, fd_, 0);
> +	if (address == MAP_FAILED) {
> +		error_ = -errno;
> +		LOG(HAL, Error) << "Failed to mmap plane";
> +		return;
> +	}
> +	maps_.emplace_back(static_cast<uint8_t *>(address), bufferLength_);
> +
> +	planes_.reserve(planeInfo_.size());
> +	for (const auto &info : planeInfo_) {
> +		planes_.emplace_back(
> +			static_cast<uint8_t *>(address) + info.offset, info.size);
> +	}
> +
> +	mapped_ = true;
>  }
>
>  PUBLIC_CAMERA_BUFFER_IMPLEMENTATION
> --
> 2.33.0.rc2.250.ged5fa647cd-goog
>
Hirokazu Honda Aug. 27, 2021, 6:57 a.m. UTC | #3 
Hi Laurent,

On Thu, Aug 26, 2021 at 9:06 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Hiro,
>
> Thank you for the patch.
>
> On Thu, Aug 26, 2021 at 05:00:20PM +0900, Hirokazu Honda wrote:
> > CameraBuffer implementation maps a given buffer_handle_t in
> > constructor. Mapping is redundant to only know the plane info like
> > stride and offset. Mapping should be executed rater in the first
> > plane() call.
> >
> > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > ---
> >  src/android/mm/cros_camera_buffer.cpp    | 71 +++++++++++--------
> >  src/android/mm/generic_camera_buffer.cpp | 86 ++++++++++++++++--------
> >  2 files changed, 100 insertions(+), 57 deletions(-)
> >
> > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp
> > index 50732637..ba6650cf 100644
> > --- a/src/android/mm/cros_camera_buffer.cpp
> > +++ b/src/android/mm/cros_camera_buffer.cpp
> > @@ -25,7 +25,7 @@ public:
> >               int flags);
> >       ~Private();
> >
> > -     bool isValid() const { return valid_; }
> > +     bool isValid() const { return registered_; }
> >
> >       unsigned int numPlanes() const;
> >
> > @@ -34,10 +34,12 @@ public:
> >       size_t jpegBufferSize(size_t maxJpegBufferSize) const;
> >
> >  private:
> > +     void map();
> > +
> >       cros::CameraBufferManager *bufferManager_;
> >       buffer_handle_t handle_;
> >       unsigned int numPlanes_;
> > -     bool valid_;
> > +     bool mapped_;
> >       bool registered_;
> >       union {
> >               void *addr;
> > @@ -50,7 +52,7 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
> >                              [[maybe_unused]] libcamera::PixelFormat pixelFormat,
> >                              [[maybe_unused]] const libcamera::Size &size,
> >                              [[maybe_unused]] int flags)
> > -     : handle_(camera3Buffer), numPlanes_(0), valid_(false),
> > +     : handle_(camera3Buffer), numPlanes_(0), mapped_(false),
> >         registered_(false)
> >  {
> >       bufferManager_ = cros::CameraBufferManager::GetInstance();
> > @@ -63,36 +65,11 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
> >
> >       registered_ = true;
> >       numPlanes_ = bufferManager_->GetNumPlanes(camera3Buffer);
> > -     switch (numPlanes_) {
> > -     case 1: {
> > -             ret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);
> > -             if (ret) {
> > -                     LOG(HAL, Error) << "Single plane buffer mapping failed";
> > -                     return;
> > -             }
> > -             break;
> > -     }
> > -     case 2:
> > -     case 3: {
> > -             ret = bufferManager_->LockYCbCr(handle_, 0, 0, 0, 0, 0,
> > -                                             &mem.ycbcr);
> > -             if (ret) {
> > -                     LOG(HAL, Error) << "YCbCr buffer mapping failed";
> > -                     return;
> > -             }
> > -             break;
> > -     }
> > -     default:
> > -             LOG(HAL, Error) << "Invalid number of planes: " << numPlanes_;
> > -             return;
> > -     }
> > -
> > -     valid_ = true;
> >  }
> >
> >  CameraBuffer::Private::~Private()
> >  {
> > -     if (valid_)
> > +     if (mapped_)
> >               bufferManager_->Unlock(handle_);
> >       if (registered_)
> >               bufferManager_->Deregister(handle_);
> > @@ -105,6 +82,11 @@ unsigned int CameraBuffer::Private::numPlanes() const
> >
> >  Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane)
> >  {
> > +     if (!mapped_)
> > +             map();
> > +     if (!mapped_)
> > +             return {};
> > +
> >       void *addr;
> >
> >       switch (numPlanes()) {
> > @@ -134,4 +116,35 @@ size_t CameraBuffer::Private::jpegBufferSize([[maybe_unused]] size_t maxJpegBuff
> >       return bufferManager_->GetPlaneSize(handle_, 0);
> >  }
> >
> > +void CameraBuffer::Private::map()
> > +{
> > +     int ret;
> > +     switch (numPlanes_) {
> > +     case 1: {
> > +             ret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);
> > +             if (ret) {
> > +                     LOG(HAL, Error) << "Single plane buffer mapping failed";
> > +                     return;
> > +             }
> > +             break;
> > +     }
> > +     case 2:
> > +     case 3: {
> > +             ret = bufferManager_->LockYCbCr(handle_, 0, 0, 0, 0, 0,
> > +                                             &mem.ycbcr);
> > +             if (ret) {
> > +                     LOG(HAL, Error) << "YCbCr buffer mapping failed";
> > +                     return;
> > +             }
> > +             break;
> > +     }
> > +     default:
> > +             LOG(HAL, Error) << "Invalid number of planes: " << numPlanes_;
> > +             return;
> > +     }
> > +
> > +     mapped_ = true;
> > +     return;
> > +}
> > +
> >  PUBLIC_CAMERA_BUFFER_IMPLEMENTATION
> > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp
> > index 22753490..cfe55b69 100644
> > --- a/src/android/mm/generic_camera_buffer.cpp
> > +++ b/src/android/mm/generic_camera_buffer.cpp
> > @@ -37,6 +37,18 @@ public:
> >       size_t jpegBufferSize(size_t maxJpegBufferSize) const;
> >
> >  private:
> > +     struct PlaneInfo {
> > +             unsigned int offset;
> > +             unsigned int size;
> > +     };
> > +
> > +     void map();
> > +
> > +     int fd_;
> > +     int flags_;
> > +     off_t bufferLength_;
> > +     bool mapped_;
> > +     std::vector<PlaneInfo> planeInfo_;
> >       /* \todo Remove planes_ when it will be added to MappedBuffer */
> >       std::vector<Span<uint8_t>> planes_;
> >  };
> > @@ -45,6 +57,7 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
> >                              buffer_handle_t camera3Buffer,
> >                              libcamera::PixelFormat pixelFormat,
> >                              const libcamera::Size &size, int flags)
> > +     : fd_(-1), flags_(flags), mapped_(false)
>
> This should initialize bufferLength_ to 0, otherwise the ASSERT()s that
> checks the variable will be pointless if the constructor returns an
> error before setting bufferLength_.
>

Shall I set bufferLength to -1 and then ASSERT(bufferLength_ >= 0), or
set to 0 and ASSERT(bufferLength > 0)?

-Hiro
> And it will also avoid a Coverity warning :-)
>
> I can handle this when pushing the series.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> >  {
> >       error_ = 0;
> >
> > @@ -61,43 +74,35 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
> >        * now that the buffer is backed by a single dmabuf, with planes being
> >        * stored contiguously.
> >        */
> > -     int fd = -1;
> >       for (int i = 0; i < camera3Buffer->numFds; i++) {
> > -             if (camera3Buffer->data[i] == -1 || camera3Buffer->data[i] == fd)
> > +             if (camera3Buffer->data[i] == -1 || camera3Buffer->data[i] == fd_)
> >                       continue;
> >
> > -             if (fd != -1) {
> > +             if (fd_ != -1) {
> >                       error_ = -EINVAL;
> >                       LOG(HAL, Error) << "Discontiguous planes are not supported";
> >                       return;
> >               }
> >
> > -             fd = camera3Buffer->data[i];
> > +             fd_ = camera3Buffer->data[i];
> >       }
> >
> > -     if (fd == -1) {
> > +     if (fd_ == -1) {
> >               error_ = -EINVAL;
> >               LOG(HAL, Error) << "No valid file descriptor";
> >               return;
> >       }
> >
> > -     off_t bufferLength = lseek(fd, 0, SEEK_END);
> > -     if (bufferLength < 0) {
> > +     bufferLength_ = lseek(fd_, 0, SEEK_END);
> > +     if (bufferLength_ < 0) {
> >               error_ = -errno;
> >               LOG(HAL, Error) << "Failed to get buffer length";
> >               return;
> >       }
> >
> > -     void *address = mmap(nullptr, bufferLength, flags, MAP_SHARED, fd, 0);
> > -     if (address == MAP_FAILED) {
> > -             error_ = -errno;
> > -             LOG(HAL, Error) << "Failed to mmap plane";
> > -             return;
> > -     }
> > -     maps_.emplace_back(static_cast<uint8_t *>(address), bufferLength);
> > -
> >       const unsigned int numPlanes = info.numPlanes();
> > -     planes_.resize(numPlanes);
> > +     planeInfo_.resize(numPlanes);
> > +
> >       unsigned int offset = 0;
> >       for (unsigned int i = 0; i < numPlanes; ++i) {
> >               /*
> > @@ -109,17 +114,17 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
> >               const unsigned int planeSize =
> >                       stride * ((size.height + vertSubSample - 1) / vertSubSample);
> >
> > -             planes_[i] = libcamera::Span<uint8_t>(
> > -                     static_cast<uint8_t *>(address) + offset, planeSize);
> > +             planeInfo_[i].offset = offset;
> > +             planeInfo_[i].size = planeSize;
> >
> > -             if (bufferLength < offset + planeSize) {
> > -                     error_ = -EINVAL;
> > -                     LOG(HAL, Error) << "Plane " << i << " is out of buffer"
> > -                                     << ", buffer length=" << bufferLength
> > -                                     << ", offset=" << offset
> > -                                     << ", size=" << planeSize;
> > +             if (bufferLength_ < offset + planeSize) {
> > +                     LOG(HAL, Error) << "Plane " << i << " is out of buffer:"
> > +                                     << " plane offset=" << offset
> > +                                     << ", plane size=" << planeSize
> > +                                     << ", buffer length=" << bufferLength_;
> >                       return;
> >               }
> > +
> >               offset += planeSize;
> >       }
> >  }
> > @@ -130,12 +135,14 @@ CameraBuffer::Private::~Private()
> >
> >  unsigned int CameraBuffer::Private::numPlanes() const
> >  {
> > -     return planes_.size();
> > +     return planeInfo_.size();
> >  }
> >
> >  Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane)
> >  {
> > -     if (plane >= planes_.size())
> > +     if (!mapped_)
> > +             map();
> > +     if (!mapped_)
> >               return {};
> >
> >       return planes_[plane];
> > @@ -143,8 +150,31 @@ Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane)
> >
> >  size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const
> >  {
> > -     return std::min<unsigned int>(maps_[0].size(),
> > -                                   maxJpegBufferSize);
> > +     ASSERT(bufferLength_ >= 0);
> > +
> > +     return std::min<unsigned int>(bufferLength_, maxJpegBufferSize);
> > +}
> > +
> > +void CameraBuffer::Private::map()
> > +{
> > +     ASSERT(fd_ != -1);
> > +     ASSERT(bufferLength_ >= 0);
> > +
> > +     void *address = mmap(nullptr, bufferLength_, flags_, MAP_SHARED, fd_, 0);
> > +     if (address == MAP_FAILED) {
> > +             error_ = -errno;
> > +             LOG(HAL, Error) << "Failed to mmap plane";
> > +             return;
> > +     }
> > +     maps_.emplace_back(static_cast<uint8_t *>(address), bufferLength_);
> > +
> > +     planes_.reserve(planeInfo_.size());
> > +     for (const auto &info : planeInfo_) {
> > +             planes_.emplace_back(
> > +                     static_cast<uint8_t *>(address) + info.offset, info.size);
> > +     }
> > +
> > +     mapped_ = true;
> >  }
> >
> >  PUBLIC_CAMERA_BUFFER_IMPLEMENTATION
>
> --
> Regards,
>
> Laurent Pinchart
Hirokazu Honda Aug. 27, 2021, 7 a.m. UTC | #4 
On Fri, Aug 27, 2021 at 3:57 PM Hirokazu Honda <hiroh@chromium.org> wrote:
>
> Hi Laurent,
>
> On Thu, Aug 26, 2021 at 9:06 PM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > Hi Hiro,
> >
> > Thank you for the patch.
> >
> > On Thu, Aug 26, 2021 at 05:00:20PM +0900, Hirokazu Honda wrote:
> > > CameraBuffer implementation maps a given buffer_handle_t in
> > > constructor. Mapping is redundant to only know the plane info like
> > > stride and offset. Mapping should be executed rater in the first
> > > plane() call.
> > >
> > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > > ---
> > >  src/android/mm/cros_camera_buffer.cpp    | 71 +++++++++++--------
> > >  src/android/mm/generic_camera_buffer.cpp | 86 ++++++++++++++++--------
> > >  2 files changed, 100 insertions(+), 57 deletions(-)
> > >
> > > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp
> > > index 50732637..ba6650cf 100644
> > > --- a/src/android/mm/cros_camera_buffer.cpp
> > > +++ b/src/android/mm/cros_camera_buffer.cpp
> > > @@ -25,7 +25,7 @@ public:
> > >               int flags);
> > >       ~Private();
> > >
> > > -     bool isValid() const { return valid_; }
> > > +     bool isValid() const { return registered_; }
> > >
> > >       unsigned int numPlanes() const;
> > >
> > > @@ -34,10 +34,12 @@ public:
> > >       size_t jpegBufferSize(size_t maxJpegBufferSize) const;
> > >
> > >  private:
> > > +     void map();
> > > +
> > >       cros::CameraBufferManager *bufferManager_;
> > >       buffer_handle_t handle_;
> > >       unsigned int numPlanes_;
> > > -     bool valid_;
> > > +     bool mapped_;
> > >       bool registered_;
> > >       union {
> > >               void *addr;
> > > @@ -50,7 +52,7 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
> > >                              [[maybe_unused]] libcamera::PixelFormat pixelFormat,
> > >                              [[maybe_unused]] const libcamera::Size &size,
> > >                              [[maybe_unused]] int flags)
> > > -     : handle_(camera3Buffer), numPlanes_(0), valid_(false),
> > > +     : handle_(camera3Buffer), numPlanes_(0), mapped_(false),
> > >         registered_(false)
> > >  {
> > >       bufferManager_ = cros::CameraBufferManager::GetInstance();
> > > @@ -63,36 +65,11 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
> > >
> > >       registered_ = true;
> > >       numPlanes_ = bufferManager_->GetNumPlanes(camera3Buffer);
> > > -     switch (numPlanes_) {
> > > -     case 1: {
> > > -             ret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);
> > > -             if (ret) {
> > > -                     LOG(HAL, Error) << "Single plane buffer mapping failed";
> > > -                     return;
> > > -             }
> > > -             break;
> > > -     }
> > > -     case 2:
> > > -     case 3: {
> > > -             ret = bufferManager_->LockYCbCr(handle_, 0, 0, 0, 0, 0,
> > > -                                             &mem.ycbcr);
> > > -             if (ret) {
> > > -                     LOG(HAL, Error) << "YCbCr buffer mapping failed";
> > > -                     return;
> > > -             }
> > > -             break;
> > > -     }
> > > -     default:
> > > -             LOG(HAL, Error) << "Invalid number of planes: " << numPlanes_;
> > > -             return;
> > > -     }
> > > -
> > > -     valid_ = true;
> > >  }
> > >
> > >  CameraBuffer::Private::~Private()
> > >  {
> > > -     if (valid_)
> > > +     if (mapped_)
> > >               bufferManager_->Unlock(handle_);
> > >       if (registered_)
> > >               bufferManager_->Deregister(handle_);
> > > @@ -105,6 +82,11 @@ unsigned int CameraBuffer::Private::numPlanes() const
> > >
> > >  Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane)
> > >  {
> > > +     if (!mapped_)
> > > +             map();
> > > +     if (!mapped_)
> > > +             return {};
> > > +
> > >       void *addr;
> > >
> > >       switch (numPlanes()) {
> > > @@ -134,4 +116,35 @@ size_t CameraBuffer::Private::jpegBufferSize([[maybe_unused]] size_t maxJpegBuff
> > >       return bufferManager_->GetPlaneSize(handle_, 0);
> > >  }
> > >
> > > +void CameraBuffer::Private::map()
> > > +{
> > > +     int ret;
> > > +     switch (numPlanes_) {
> > > +     case 1: {
> > > +             ret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);
> > > +             if (ret) {
> > > +                     LOG(HAL, Error) << "Single plane buffer mapping failed";
> > > +                     return;
> > > +             }
> > > +             break;
> > > +     }
> > > +     case 2:
> > > +     case 3: {
> > > +             ret = bufferManager_->LockYCbCr(handle_, 0, 0, 0, 0, 0,
> > > +                                             &mem.ycbcr);
> > > +             if (ret) {
> > > +                     LOG(HAL, Error) << "YCbCr buffer mapping failed";
> > > +                     return;
> > > +             }
> > > +             break;
> > > +     }
> > > +     default:
> > > +             LOG(HAL, Error) << "Invalid number of planes: " << numPlanes_;
> > > +             return;
> > > +     }
> > > +
> > > +     mapped_ = true;
> > > +     return;
> > > +}
> > > +
> > >  PUBLIC_CAMERA_BUFFER_IMPLEMENTATION
> > > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp
> > > index 22753490..cfe55b69 100644
> > > --- a/src/android/mm/generic_camera_buffer.cpp
> > > +++ b/src/android/mm/generic_camera_buffer.cpp
> > > @@ -37,6 +37,18 @@ public:
> > >       size_t jpegBufferSize(size_t maxJpegBufferSize) const;
> > >
> > >  private:
> > > +     struct PlaneInfo {
> > > +             unsigned int offset;
> > > +             unsigned int size;
> > > +     };
> > > +
> > > +     void map();
> > > +
> > > +     int fd_;
> > > +     int flags_;
> > > +     off_t bufferLength_;
> > > +     bool mapped_;
> > > +     std::vector<PlaneInfo> planeInfo_;
> > >       /* \todo Remove planes_ when it will be added to MappedBuffer */
> > >       std::vector<Span<uint8_t>> planes_;
> > >  };
> > > @@ -45,6 +57,7 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
> > >                              buffer_handle_t camera3Buffer,
> > >                              libcamera::PixelFormat pixelFormat,
> > >                              const libcamera::Size &size, int flags)
> > > +     : fd_(-1), flags_(flags), mapped_(false)
> >
> > This should initialize bufferLength_ to 0, otherwise the ASSERT()s that
> > checks the variable will be pointless if the constructor returns an
> > error before setting bufferLength_.
> >
>
> Shall I set bufferLength to -1 and then ASSERT(bufferLength_ >= 0), or
> set to 0 and ASSERT(bufferLength > 0)?
>

Uploaded with the former. If you think the latter is correct, please
fix it upon applying.
Thanks in advance.

-Hiro

> -Hiro
> > And it will also avoid a Coverity warning :-)
> >
> > I can handle this when pushing the series.
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > >  {
> > >       error_ = 0;
> > >
> > > @@ -61,43 +74,35 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
> > >        * now that the buffer is backed by a single dmabuf, with planes being
> > >        * stored contiguously.
> > >        */
> > > -     int fd = -1;
> > >       for (int i = 0; i < camera3Buffer->numFds; i++) {
> > > -             if (camera3Buffer->data[i] == -1 || camera3Buffer->data[i] == fd)
> > > +             if (camera3Buffer->data[i] == -1 || camera3Buffer->data[i] == fd_)
> > >                       continue;
> > >
> > > -             if (fd != -1) {
> > > +             if (fd_ != -1) {
> > >                       error_ = -EINVAL;
> > >                       LOG(HAL, Error) << "Discontiguous planes are not supported";
> > >                       return;
> > >               }
> > >
> > > -             fd = camera3Buffer->data[i];
> > > +             fd_ = camera3Buffer->data[i];
> > >       }
> > >
> > > -     if (fd == -1) {
> > > +     if (fd_ == -1) {
> > >               error_ = -EINVAL;
> > >               LOG(HAL, Error) << "No valid file descriptor";
> > >               return;
> > >       }
> > >
> > > -     off_t bufferLength = lseek(fd, 0, SEEK_END);
> > > -     if (bufferLength < 0) {
> > > +     bufferLength_ = lseek(fd_, 0, SEEK_END);
> > > +     if (bufferLength_ < 0) {
> > >               error_ = -errno;
> > >               LOG(HAL, Error) << "Failed to get buffer length";
> > >               return;
> > >       }
> > >
> > > -     void *address = mmap(nullptr, bufferLength, flags, MAP_SHARED, fd, 0);
> > > -     if (address == MAP_FAILED) {
> > > -             error_ = -errno;
> > > -             LOG(HAL, Error) << "Failed to mmap plane";
> > > -             return;
> > > -     }
> > > -     maps_.emplace_back(static_cast<uint8_t *>(address), bufferLength);
> > > -
> > >       const unsigned int numPlanes = info.numPlanes();
> > > -     planes_.resize(numPlanes);
> > > +     planeInfo_.resize(numPlanes);
> > > +
> > >       unsigned int offset = 0;
> > >       for (unsigned int i = 0; i < numPlanes; ++i) {
> > >               /*
> > > @@ -109,17 +114,17 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
> > >               const unsigned int planeSize =
> > >                       stride * ((size.height + vertSubSample - 1) / vertSubSample);
> > >
> > > -             planes_[i] = libcamera::Span<uint8_t>(
> > > -                     static_cast<uint8_t *>(address) + offset, planeSize);
> > > +             planeInfo_[i].offset = offset;
> > > +             planeInfo_[i].size = planeSize;
> > >
> > > -             if (bufferLength < offset + planeSize) {
> > > -                     error_ = -EINVAL;
> > > -                     LOG(HAL, Error) << "Plane " << i << " is out of buffer"
> > > -                                     << ", buffer length=" << bufferLength
> > > -                                     << ", offset=" << offset
> > > -                                     << ", size=" << planeSize;
> > > +             if (bufferLength_ < offset + planeSize) {
> > > +                     LOG(HAL, Error) << "Plane " << i << " is out of buffer:"
> > > +                                     << " plane offset=" << offset
> > > +                                     << ", plane size=" << planeSize
> > > +                                     << ", buffer length=" << bufferLength_;
> > >                       return;
> > >               }
> > > +
> > >               offset += planeSize;
> > >       }
> > >  }
> > > @@ -130,12 +135,14 @@ CameraBuffer::Private::~Private()
> > >
> > >  unsigned int CameraBuffer::Private::numPlanes() const
> > >  {
> > > -     return planes_.size();
> > > +     return planeInfo_.size();
> > >  }
> > >
> > >  Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane)
> > >  {
> > > -     if (plane >= planes_.size())
> > > +     if (!mapped_)
> > > +             map();
> > > +     if (!mapped_)
> > >               return {};
> > >
> > >       return planes_[plane];
> > > @@ -143,8 +150,31 @@ Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane)
> > >
> > >  size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const
> > >  {
> > > -     return std::min<unsigned int>(maps_[0].size(),
> > > -                                   maxJpegBufferSize);
> > > +     ASSERT(bufferLength_ >= 0);
> > > +
> > > +     return std::min<unsigned int>(bufferLength_, maxJpegBufferSize);
> > > +}
> > > +
> > > +void CameraBuffer::Private::map()
> > > +{
> > > +     ASSERT(fd_ != -1);
> > > +     ASSERT(bufferLength_ >= 0);
> > > +
> > > +     void *address = mmap(nullptr, bufferLength_, flags_, MAP_SHARED, fd_, 0);
> > > +     if (address == MAP_FAILED) {
> > > +             error_ = -errno;
> > > +             LOG(HAL, Error) << "Failed to mmap plane";
> > > +             return;
> > > +     }
> > > +     maps_.emplace_back(static_cast<uint8_t *>(address), bufferLength_);
> > > +
> > > +     planes_.reserve(planeInfo_.size());
> > > +     for (const auto &info : planeInfo_) {
> > > +             planes_.emplace_back(
> > > +                     static_cast<uint8_t *>(address) + info.offset, info.size);
> > > +     }
> > > +
> > > +     mapped_ = true;
> > >  }
> > >
> > >  PUBLIC_CAMERA_BUFFER_IMPLEMENTATION
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
Laurent Pinchart Aug. 30, 2021, 3 p.m. UTC | #5 
Hi Hiro,

On Fri, Aug 27, 2021 at 04:00:10PM +0900, Hirokazu Honda wrote:
> On Fri, Aug 27, 2021 at 3:57 PM Hirokazu Honda wrote:
> > On Thu, Aug 26, 2021 at 9:06 PM Laurent Pinchart wrote:
> > > On Thu, Aug 26, 2021 at 05:00:20PM +0900, Hirokazu Honda wrote:
> > > > CameraBuffer implementation maps a given buffer_handle_t in
> > > > constructor. Mapping is redundant to only know the plane info like
> > > > stride and offset. Mapping should be executed rater in the first
> > > > plane() call.
> > > >
> > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > > > ---
> > > >  src/android/mm/cros_camera_buffer.cpp    | 71 +++++++++++--------
> > > >  src/android/mm/generic_camera_buffer.cpp | 86 ++++++++++++++++--------
> > > >  2 files changed, 100 insertions(+), 57 deletions(-)
> > > >
> > > > diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp
> > > > index 50732637..ba6650cf 100644
> > > > --- a/src/android/mm/cros_camera_buffer.cpp
> > > > +++ b/src/android/mm/cros_camera_buffer.cpp
> > > > @@ -25,7 +25,7 @@ public:
> > > >               int flags);
> > > >       ~Private();
> > > >
> > > > -     bool isValid() const { return valid_; }
> > > > +     bool isValid() const { return registered_; }
> > > >
> > > >       unsigned int numPlanes() const;
> > > >
> > > > @@ -34,10 +34,12 @@ public:
> > > >       size_t jpegBufferSize(size_t maxJpegBufferSize) const;
> > > >
> > > >  private:
> > > > +     void map();
> > > > +
> > > >       cros::CameraBufferManager *bufferManager_;
> > > >       buffer_handle_t handle_;
> > > >       unsigned int numPlanes_;
> > > > -     bool valid_;
> > > > +     bool mapped_;
> > > >       bool registered_;
> > > >       union {
> > > >               void *addr;
> > > > @@ -50,7 +52,7 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
> > > >                              [[maybe_unused]] libcamera::PixelFormat pixelFormat,
> > > >                              [[maybe_unused]] const libcamera::Size &size,
> > > >                              [[maybe_unused]] int flags)
> > > > -     : handle_(camera3Buffer), numPlanes_(0), valid_(false),
> > > > +     : handle_(camera3Buffer), numPlanes_(0), mapped_(false),
> > > >         registered_(false)
> > > >  {
> > > >       bufferManager_ = cros::CameraBufferManager::GetInstance();
> > > > @@ -63,36 +65,11 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
> > > >
> > > >       registered_ = true;
> > > >       numPlanes_ = bufferManager_->GetNumPlanes(camera3Buffer);
> > > > -     switch (numPlanes_) {
> > > > -     case 1: {
> > > > -             ret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);
> > > > -             if (ret) {
> > > > -                     LOG(HAL, Error) << "Single plane buffer mapping failed";
> > > > -                     return;
> > > > -             }
> > > > -             break;
> > > > -     }
> > > > -     case 2:
> > > > -     case 3: {
> > > > -             ret = bufferManager_->LockYCbCr(handle_, 0, 0, 0, 0, 0,
> > > > -                                             &mem.ycbcr);
> > > > -             if (ret) {
> > > > -                     LOG(HAL, Error) << "YCbCr buffer mapping failed";
> > > > -                     return;
> > > > -             }
> > > > -             break;
> > > > -     }
> > > > -     default:
> > > > -             LOG(HAL, Error) << "Invalid number of planes: " << numPlanes_;
> > > > -             return;
> > > > -     }
> > > > -
> > > > -     valid_ = true;
> > > >  }
> > > >
> > > >  CameraBuffer::Private::~Private()
> > > >  {
> > > > -     if (valid_)
> > > > +     if (mapped_)
> > > >               bufferManager_->Unlock(handle_);
> > > >       if (registered_)
> > > >               bufferManager_->Deregister(handle_);
> > > > @@ -105,6 +82,11 @@ unsigned int CameraBuffer::Private::numPlanes() const
> > > >
> > > >  Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane)
> > > >  {
> > > > +     if (!mapped_)
> > > > +             map();
> > > > +     if (!mapped_)
> > > > +             return {};
> > > > +
> > > >       void *addr;
> > > >
> > > >       switch (numPlanes()) {
> > > > @@ -134,4 +116,35 @@ size_t CameraBuffer::Private::jpegBufferSize([[maybe_unused]] size_t maxJpegBuff
> > > >       return bufferManager_->GetPlaneSize(handle_, 0);
> > > >  }
> > > >
> > > > +void CameraBuffer::Private::map()
> > > > +{
> > > > +     int ret;
> > > > +     switch (numPlanes_) {
> > > > +     case 1: {
> > > > +             ret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);
> > > > +             if (ret) {
> > > > +                     LOG(HAL, Error) << "Single plane buffer mapping failed";
> > > > +                     return;
> > > > +             }
> > > > +             break;
> > > > +     }
> > > > +     case 2:
> > > > +     case 3: {
> > > > +             ret = bufferManager_->LockYCbCr(handle_, 0, 0, 0, 0, 0,
> > > > +                                             &mem.ycbcr);
> > > > +             if (ret) {
> > > > +                     LOG(HAL, Error) << "YCbCr buffer mapping failed";
> > > > +                     return;
> > > > +             }
> > > > +             break;
> > > > +     }
> > > > +     default:
> > > > +             LOG(HAL, Error) << "Invalid number of planes: " << numPlanes_;
> > > > +             return;
> > > > +     }
> > > > +
> > > > +     mapped_ = true;
> > > > +     return;
> > > > +}
> > > > +
> > > >  PUBLIC_CAMERA_BUFFER_IMPLEMENTATION
> > > > diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp
> > > > index 22753490..cfe55b69 100644
> > > > --- a/src/android/mm/generic_camera_buffer.cpp
> > > > +++ b/src/android/mm/generic_camera_buffer.cpp
> > > > @@ -37,6 +37,18 @@ public:
> > > >       size_t jpegBufferSize(size_t maxJpegBufferSize) const;
> > > >
> > > >  private:
> > > > +     struct PlaneInfo {
> > > > +             unsigned int offset;
> > > > +             unsigned int size;
> > > > +     };
> > > > +
> > > > +     void map();
> > > > +
> > > > +     int fd_;
> > > > +     int flags_;
> > > > +     off_t bufferLength_;
> > > > +     bool mapped_;
> > > > +     std::vector<PlaneInfo> planeInfo_;
> > > >       /* \todo Remove planes_ when it will be added to MappedBuffer */
> > > >       std::vector<Span<uint8_t>> planes_;
> > > >  };
> > > > @@ -45,6 +57,7 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
> > > >                              buffer_handle_t camera3Buffer,
> > > >                              libcamera::PixelFormat pixelFormat,
> > > >                              const libcamera::Size &size, int flags)
> > > > +     : fd_(-1), flags_(flags), mapped_(false)
> > >
> > > This should initialize bufferLength_ to 0, otherwise the ASSERT()s that
> > > checks the variable will be pointless if the constructor returns an
> > > error before setting bufferLength_.
> >
> > Shall I set bufferLength to -1 and then ASSERT(bufferLength_ >= 0), or
> > set to 0 and ASSERT(bufferLength > 0)?
> 
> Uploaded with the former. If you think the latter is correct, please
> fix it upon applying.
> Thanks in advance.

I've mistakenly pushed this patch already. I'll turn yours into a fix
and will send it to the list. Sorry about the mistake.

> > > And it will also avoid a Coverity warning :-)
> > >
> > > I can handle this when pushing the series.
> > >
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > >
> > > >  {
> > > >       error_ = 0;
> > > >
> > > > @@ -61,43 +74,35 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
> > > >        * now that the buffer is backed by a single dmabuf, with planes being
> > > >        * stored contiguously.
> > > >        */
> > > > -     int fd = -1;
> > > >       for (int i = 0; i < camera3Buffer->numFds; i++) {
> > > > -             if (camera3Buffer->data[i] == -1 || camera3Buffer->data[i] == fd)
> > > > +             if (camera3Buffer->data[i] == -1 || camera3Buffer->data[i] == fd_)
> > > >                       continue;
> > > >
> > > > -             if (fd != -1) {
> > > > +             if (fd_ != -1) {
> > > >                       error_ = -EINVAL;
> > > >                       LOG(HAL, Error) << "Discontiguous planes are not supported";
> > > >                       return;
> > > >               }
> > > >
> > > > -             fd = camera3Buffer->data[i];
> > > > +             fd_ = camera3Buffer->data[i];
> > > >       }
> > > >
> > > > -     if (fd == -1) {
> > > > +     if (fd_ == -1) {
> > > >               error_ = -EINVAL;
> > > >               LOG(HAL, Error) << "No valid file descriptor";
> > > >               return;
> > > >       }
> > > >
> > > > -     off_t bufferLength = lseek(fd, 0, SEEK_END);
> > > > -     if (bufferLength < 0) {
> > > > +     bufferLength_ = lseek(fd_, 0, SEEK_END);
> > > > +     if (bufferLength_ < 0) {
> > > >               error_ = -errno;
> > > >               LOG(HAL, Error) << "Failed to get buffer length";
> > > >               return;
> > > >       }
> > > >
> > > > -     void *address = mmap(nullptr, bufferLength, flags, MAP_SHARED, fd, 0);
> > > > -     if (address == MAP_FAILED) {
> > > > -             error_ = -errno;
> > > > -             LOG(HAL, Error) << "Failed to mmap plane";
> > > > -             return;
> > > > -     }
> > > > -     maps_.emplace_back(static_cast<uint8_t *>(address), bufferLength);
> > > > -
> > > >       const unsigned int numPlanes = info.numPlanes();
> > > > -     planes_.resize(numPlanes);
> > > > +     planeInfo_.resize(numPlanes);
> > > > +
> > > >       unsigned int offset = 0;
> > > >       for (unsigned int i = 0; i < numPlanes; ++i) {
> > > >               /*
> > > > @@ -109,17 +114,17 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
> > > >               const unsigned int planeSize =
> > > >                       stride * ((size.height + vertSubSample - 1) / vertSubSample);
> > > >
> > > > -             planes_[i] = libcamera::Span<uint8_t>(
> > > > -                     static_cast<uint8_t *>(address) + offset, planeSize);
> > > > +             planeInfo_[i].offset = offset;
> > > > +             planeInfo_[i].size = planeSize;
> > > >
> > > > -             if (bufferLength < offset + planeSize) {
> > > > -                     error_ = -EINVAL;
> > > > -                     LOG(HAL, Error) << "Plane " << i << " is out of buffer"
> > > > -                                     << ", buffer length=" << bufferLength
> > > > -                                     << ", offset=" << offset
> > > > -                                     << ", size=" << planeSize;
> > > > +             if (bufferLength_ < offset + planeSize) {
> > > > +                     LOG(HAL, Error) << "Plane " << i << " is out of buffer:"
> > > > +                                     << " plane offset=" << offset
> > > > +                                     << ", plane size=" << planeSize
> > > > +                                     << ", buffer length=" << bufferLength_;
> > > >                       return;
> > > >               }
> > > > +
> > > >               offset += planeSize;
> > > >       }
> > > >  }
> > > > @@ -130,12 +135,14 @@ CameraBuffer::Private::~Private()
> > > >
> > > >  unsigned int CameraBuffer::Private::numPlanes() const
> > > >  {
> > > > -     return planes_.size();
> > > > +     return planeInfo_.size();
> > > >  }
> > > >
> > > >  Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane)
> > > >  {
> > > > -     if (plane >= planes_.size())
> > > > +     if (!mapped_)
> > > > +             map();
> > > > +     if (!mapped_)
> > > >               return {};
> > > >
> > > >       return planes_[plane];
> > > > @@ -143,8 +150,31 @@ Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane)
> > > >
> > > >  size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const
> > > >  {
> > > > -     return std::min<unsigned int>(maps_[0].size(),
> > > > -                                   maxJpegBufferSize);
> > > > +     ASSERT(bufferLength_ >= 0);
> > > > +
> > > > +     return std::min<unsigned int>(bufferLength_, maxJpegBufferSize);
> > > > +}
> > > > +
> > > > +void CameraBuffer::Private::map()
> > > > +{
> > > > +     ASSERT(fd_ != -1);
> > > > +     ASSERT(bufferLength_ >= 0);
> > > > +
> > > > +     void *address = mmap(nullptr, bufferLength_, flags_, MAP_SHARED, fd_, 0);
> > > > +     if (address == MAP_FAILED) {
> > > > +             error_ = -errno;
> > > > +             LOG(HAL, Error) << "Failed to mmap plane";
> > > > +             return;
> > > > +     }
> > > > +     maps_.emplace_back(static_cast<uint8_t *>(address), bufferLength_);
> > > > +
> > > > +     planes_.reserve(planeInfo_.size());
> > > > +     for (const auto &info : planeInfo_) {
> > > > +             planes_.emplace_back(
> > > > +                     static_cast<uint8_t *>(address) + info.offset, info.size);
> > > > +     }
> > > > +
> > > > +     mapped_ = true;
> > > >  }
> > > >
> > > >  PUBLIC_CAMERA_BUFFER_IMPLEMENTATION

Patch
diff mbox series 

diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp
index 50732637..ba6650cf 100644
--- a/src/android/mm/cros_camera_buffer.cpp
+++ b/src/android/mm/cros_camera_buffer.cpp
@@ -25,7 +25,7 @@  public:
 		int flags);
 	~Private();
 
-	bool isValid() const { return valid_; }
+	bool isValid() const { return registered_; }
 
 	unsigned int numPlanes() const;
 
@@ -34,10 +34,12 @@  public:
 	size_t jpegBufferSize(size_t maxJpegBufferSize) const;
 
 private:
+	void map();
+
 	cros::CameraBufferManager *bufferManager_;
 	buffer_handle_t handle_;
 	unsigned int numPlanes_;
-	bool valid_;
+	bool mapped_;
 	bool registered_;
 	union {
 		void *addr;
@@ -50,7 +52,7 @@  CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
 			       [[maybe_unused]] libcamera::PixelFormat pixelFormat,
 			       [[maybe_unused]] const libcamera::Size &size,
 			       [[maybe_unused]] int flags)
-	: handle_(camera3Buffer), numPlanes_(0), valid_(false),
+	: handle_(camera3Buffer), numPlanes_(0), mapped_(false),
 	  registered_(false)
 {
 	bufferManager_ = cros::CameraBufferManager::GetInstance();
@@ -63,36 +65,11 @@  CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
 
 	registered_ = true;
 	numPlanes_ = bufferManager_->GetNumPlanes(camera3Buffer);
-	switch (numPlanes_) {
-	case 1: {
-		ret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);
-		if (ret) {
-			LOG(HAL, Error) << "Single plane buffer mapping failed";
-			return;
-		}
-		break;
-	}
-	case 2:
-	case 3: {
-		ret = bufferManager_->LockYCbCr(handle_, 0, 0, 0, 0, 0,
-						&mem.ycbcr);
-		if (ret) {
-			LOG(HAL, Error) << "YCbCr buffer mapping failed";
-			return;
-		}
-		break;
-	}
-	default:
-		LOG(HAL, Error) << "Invalid number of planes: " << numPlanes_;
-		return;
-	}
-
-	valid_ = true;
 }
 
 CameraBuffer::Private::~Private()
 {
-	if (valid_)
+	if (mapped_)
 		bufferManager_->Unlock(handle_);
 	if (registered_)
 		bufferManager_->Deregister(handle_);
@@ -105,6 +82,11 @@  unsigned int CameraBuffer::Private::numPlanes() const
 
 Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane)
 {
+	if (!mapped_)
+		map();
+	if (!mapped_)
+		return {};
+
 	void *addr;
 
 	switch (numPlanes()) {
@@ -134,4 +116,35 @@  size_t CameraBuffer::Private::jpegBufferSize([[maybe_unused]] size_t maxJpegBuff
 	return bufferManager_->GetPlaneSize(handle_, 0);
 }
 
+void CameraBuffer::Private::map()
+{
+	int ret;
+	switch (numPlanes_) {
+	case 1: {
+		ret = bufferManager_->Lock(handle_, 0, 0, 0, 0, 0, &mem.addr);
+		if (ret) {
+			LOG(HAL, Error) << "Single plane buffer mapping failed";
+			return;
+		}
+		break;
+	}
+	case 2:
+	case 3: {
+		ret = bufferManager_->LockYCbCr(handle_, 0, 0, 0, 0, 0,
+						&mem.ycbcr);
+		if (ret) {
+			LOG(HAL, Error) << "YCbCr buffer mapping failed";
+			return;
+		}
+		break;
+	}
+	default:
+		LOG(HAL, Error) << "Invalid number of planes: " << numPlanes_;
+		return;
+	}
+
+	mapped_ = true;
+	return;
+}
+
 PUBLIC_CAMERA_BUFFER_IMPLEMENTATION
diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp
index 22753490..cfe55b69 100644
--- a/src/android/mm/generic_camera_buffer.cpp
+++ b/src/android/mm/generic_camera_buffer.cpp
@@ -37,6 +37,18 @@  public:
 	size_t jpegBufferSize(size_t maxJpegBufferSize) const;
 
 private:
+	struct PlaneInfo {
+		unsigned int offset;
+		unsigned int size;
+	};
+
+	void map();
+
+	int fd_;
+	int flags_;
+	off_t bufferLength_;
+	bool mapped_;
+	std::vector<PlaneInfo> planeInfo_;
 	/* \todo Remove planes_ when it will be added to MappedBuffer */
 	std::vector<Span<uint8_t>> planes_;
 };
@@ -45,6 +57,7 @@  CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
 			       buffer_handle_t camera3Buffer,
 			       libcamera::PixelFormat pixelFormat,
 			       const libcamera::Size &size, int flags)
+	: fd_(-1), flags_(flags), mapped_(false)
 {
 	error_ = 0;
 
@@ -61,43 +74,35 @@  CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
 	 * now that the buffer is backed by a single dmabuf, with planes being
 	 * stored contiguously.
 	 */
-	int fd = -1;
 	for (int i = 0; i < camera3Buffer->numFds; i++) {
-		if (camera3Buffer->data[i] == -1 || camera3Buffer->data[i] == fd)
+		if (camera3Buffer->data[i] == -1 || camera3Buffer->data[i] == fd_)
 			continue;
 
-		if (fd != -1) {
+		if (fd_ != -1) {
 			error_ = -EINVAL;
 			LOG(HAL, Error) << "Discontiguous planes are not supported";
 			return;
 		}
 
-		fd = camera3Buffer->data[i];
+		fd_ = camera3Buffer->data[i];
 	}
 
-	if (fd == -1) {
+	if (fd_ == -1) {
 		error_ = -EINVAL;
 		LOG(HAL, Error) << "No valid file descriptor";
 		return;
 	}
 
-	off_t bufferLength = lseek(fd, 0, SEEK_END);
-	if (bufferLength < 0) {
+	bufferLength_ = lseek(fd_, 0, SEEK_END);
+	if (bufferLength_ < 0) {
 		error_ = -errno;
 		LOG(HAL, Error) << "Failed to get buffer length";
 		return;
 	}
 
-	void *address = mmap(nullptr, bufferLength, flags, MAP_SHARED, fd, 0);
-	if (address == MAP_FAILED) {
-		error_ = -errno;
-		LOG(HAL, Error) << "Failed to mmap plane";
-		return;
-	}
-	maps_.emplace_back(static_cast<uint8_t *>(address), bufferLength);
-
 	const unsigned int numPlanes = info.numPlanes();
-	planes_.resize(numPlanes);
+	planeInfo_.resize(numPlanes);
+
 	unsigned int offset = 0;
 	for (unsigned int i = 0; i < numPlanes; ++i) {
 		/*
@@ -109,17 +114,17 @@  CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
 		const unsigned int planeSize =
 			stride * ((size.height + vertSubSample - 1) / vertSubSample);
 
-		planes_[i] = libcamera::Span<uint8_t>(
-			static_cast<uint8_t *>(address) + offset, planeSize);
+		planeInfo_[i].offset = offset;
+		planeInfo_[i].size = planeSize;
 
-		if (bufferLength < offset + planeSize) {
-			error_ = -EINVAL;
-			LOG(HAL, Error) << "Plane " << i << " is out of buffer"
-					<< ", buffer length=" << bufferLength
-					<< ", offset=" << offset
-					<< ", size=" << planeSize;
+		if (bufferLength_ < offset + planeSize) {
+			LOG(HAL, Error) << "Plane " << i << " is out of buffer:"
+					<< " plane offset=" << offset
+					<< ", plane size=" << planeSize
+					<< ", buffer length=" << bufferLength_;
 			return;
 		}
+
 		offset += planeSize;
 	}
 }
@@ -130,12 +135,14 @@  CameraBuffer::Private::~Private()
 
 unsigned int CameraBuffer::Private::numPlanes() const
 {
-	return planes_.size();
+	return planeInfo_.size();
 }
 
 Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane)
 {
-	if (plane >= planes_.size())
+	if (!mapped_)
+		map();
+	if (!mapped_)
 		return {};
 
 	return planes_[plane];
@@ -143,8 +150,31 @@  Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane)
 
 size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const
 {
-	return std::min<unsigned int>(maps_[0].size(),
-				      maxJpegBufferSize);
+	ASSERT(bufferLength_ >= 0);
+
+	return std::min<unsigned int>(bufferLength_, maxJpegBufferSize);
+}
+
+void CameraBuffer::Private::map()
+{
+	ASSERT(fd_ != -1);
+	ASSERT(bufferLength_ >= 0);
+
+	void *address = mmap(nullptr, bufferLength_, flags_, MAP_SHARED, fd_, 0);
+	if (address == MAP_FAILED) {
+		error_ = -errno;
+		LOG(HAL, Error) << "Failed to mmap plane";
+		return;
+	}
+	maps_.emplace_back(static_cast<uint8_t *>(address), bufferLength_);
+
+	planes_.reserve(planeInfo_.size());
+	for (const auto &info : planeInfo_) {
+		planes_.emplace_back(
+			static_cast<uint8_t *>(address) + info.offset, info.size);
+	}
+
+	mapped_ = true;
 }
 
 PUBLIC_CAMERA_BUFFER_IMPLEMENTATION