[libcamera-devel,RFC,3/3] android: camera_buffer: Add stride/offset/size function
diff mbox series

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

Commit Message

Hirokazu Honda Aug. 23, 2021, 12:43 p.m. UTC 
This adds getter functions of stride, offset and size to CameraBuffer
interface.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
---
 src/android/camera_buffer.h              | 16 +++++
 src/android/mm/cros_camera_buffer.cpp    | 19 ++++++
 src/android/mm/generic_camera_buffer.cpp | 78 ++++++++++++++++++------
 3 files changed, 94 insertions(+), 19 deletions(-)

Comments

Laurent Pinchart Aug. 23, 2021, 9:56 p.m. UTC | #1 
Hi Hiro,

Thank you for the patch.

On Mon, Aug 23, 2021 at 09:43:21PM +0900, Hirokazu Honda wrote:
> This adds getter functions of stride, offset and size to CameraBuffer
> interface.
> 
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> ---
>  src/android/camera_buffer.h              | 16 +++++
>  src/android/mm/cros_camera_buffer.cpp    | 19 ++++++
>  src/android/mm/generic_camera_buffer.cpp | 78 ++++++++++++++++++------
>  3 files changed, 94 insertions(+), 19 deletions(-)
> 
> diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h
> index 87df2570..226a8f5c 100644
> --- a/src/android/camera_buffer.h
> +++ b/src/android/camera_buffer.h
> @@ -31,6 +31,10 @@ public:
>  	libcamera::Span<const uint8_t> plane(unsigned int plane) const;
>  	libcamera::Span<uint8_t> plane(unsigned int plane);
>  
> +	unsigned int stride(unsigned int plane) const;
> +	unsigned int offset(unsigned int plane) const;
> +	unsigned int size(unsigned int plane) const;
> +
>  	size_t jpegBufferSize(size_t maxJpegBufferSize) const;
>  };
>  
> @@ -62,6 +66,18 @@ Span<uint8_t> CameraBuffer::plane(unsigned int plane)			\
>  {									\
>  	return _d()->plane(plane);					\
>  }									\
> +unsigned int CameraBuffer::stride(unsigned int plane) const		\
> +{									\
> +	return _d()->stride(plane);					\
> +}									\
> +unsigned int CameraBuffer::offset(unsigned int plane) const		\
> +{									\
> +	return _d()->offset(plane);					\
> +}									\
> +unsigned int CameraBuffer::size(unsigned int plane) const		\
> +{									\
> +	return _d()->size(plane);					\
> +}									\
>  size_t CameraBuffer::jpegBufferSize(size_t maxJpegBufferSize) const	\
>  {									\
>  	return _d()->jpegBufferSize(maxJpegBufferSize);			\
> diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp
> index 85ef6480..4ba24f79 100644
> --- a/src/android/mm/cros_camera_buffer.cpp
> +++ b/src/android/mm/cros_camera_buffer.cpp
> @@ -31,6 +31,10 @@ public:
>  
>  	Span<uint8_t> plane(unsigned int plane);
>  
> +	unsigned int stride(unsigned int plane) const;
> +	unsigned int offset(unsigned int plane) const;
> +	unsigned int size(unsigned int plane) const;
> +
>  	size_t jpegBufferSize(size_t maxJpegBufferSize) const;
>  
>  private:
> @@ -112,6 +116,21 @@ Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane)
>  		 bufferManager_->GetPlaneSize(handle_, plane) };
>  }
>  
> +unsigned int CameraBuffer::Private::stride(unsigned int plane) const
> +{
> +	return cros::CameraBufferManager::GetPlaneStride(handle_, plane);
> +}
> +
> +unsigned int CameraBuffer::Private::offset(unsigned int plane) const
> +{
> +	return cros::CameraBufferManager::GetPlaneOffset(handle_, plane);
> +}
> +
> +unsigned int CameraBuffer::Private::size(unsigned int plane) const
> +{
> +	return cros::CameraBufferManager::GetPlaneSize(handle_, plane);
> +}
> +
>  size_t CameraBuffer::Private::jpegBufferSize([[maybe_unused]] size_t maxJpegBufferSize) const
>  {
>  	return bufferManager_->GetPlaneSize(handle_, 0);
> diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp
> index 6c1e4611..b296b3e3 100644
> --- a/src/android/mm/generic_camera_buffer.cpp
> +++ b/src/android/mm/generic_camera_buffer.cpp
> @@ -34,15 +34,24 @@ public:
>  
>  	Span<uint8_t> plane(unsigned int plane);
>  
> +	unsigned int stride(unsigned int plane) const;
> +	unsigned int offset(unsigned int plane) const;
> +	unsigned int size(unsigned int plane) const;
> +
>  	size_t jpegBufferSize(size_t maxJpegBufferSize) const;
>  
>  private:
> +	struct PlaneInfo {
> +		unsigned int stride;
> +		unsigned int offset;
> +		unsigned int size;
> +	};
> +
>  	bool Map();
>  
>  	int fd_;
>  	int flags_;
> -	libcamera::Size size_;
> -	libcamera::PixelFormatInfo info_;
> +	std::vector<PlaneInfo> planeInfo_;
>  	/* \todo remove planes_ is added to MappedBuffer. */
>  	std::vector<Span<uint8_t>> planes_;
>  };
> @@ -51,7 +60,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), size_(size)
> +	: fd_(-1), flags_(flags)
>  {
>  	error_ = 0;
>  
> @@ -68,13 +77,29 @@ CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
>  		return;
>  	}
>  
> -	info_ = libcamera::PixelFormatInfo::info(pixelFormat);
> -	if (!info_.isValid()) {
> +	const auto &info = libcamera::PixelFormatInfo::info(pixelFormat);
> +	if (!info.isValid()) {
>  		error_ = EINVAL;
>  		LOG(HAL, Error) << "Invalid pixel format: "
>  				<< pixelFormat.toString();
>  		return;
>  	}
> +
> +	const unsigned int numPlanes = info.numPlanes();
> +	planeInfo_.resize(numPlanes);
> +	unsigned int offset = 0;
> +	for (unsigned int i = 0; i < numPlanes; ++i) {
> +		const unsigned int vertSubSample = info.planes[i].verticalSubSampling;
> +		const unsigned int stride = info.stride(size.width, i, 1u);
> +		const unsigned int planeSize =
> +			stride * ((size.height + vertSubSample - 1) / vertSubSample);
> +
> +		planeInfo_[i].stride = stride;
> +		planeInfo_[i].offset = offset;
> +		planeInfo_[i].size = planeSize;
> +
> +		offset += planeSize;
> +	}

Ah, I see this code moving back to the constructor, as I mentioned in
the review of 2/3 :-) If it doesn't make the patches too ugly, it would
be nice to keep it in the constructor in 2/3 instead of moving it back.

>  }
>  
>  CameraBuffer::Private::~Private()
> @@ -83,7 +108,7 @@ CameraBuffer::Private::~Private()
>  
>  unsigned int CameraBuffer::Private::numPlanes() const
>  {
> -	return info_.numPlanes();
> +	return planeInfo_.size();
>  }
>  
>  Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane)
> @@ -97,6 +122,30 @@ Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane)
>  	return planes_[plane];
>  }
>  
> +unsigned int CameraBuffer::Private::stride(unsigned int plane) const
> +{
> +	if (plane >= planeInfo_.size())
> +		return 0;
> +
> +	return planeInfo_[plane].stride;
> +}
> +
> +unsigned int CameraBuffer::Private::offset(unsigned int plane) const
> +{
> +	if (plane >= planeInfo_.size())
> +		return 0;
> +
> +	return planeInfo_[plane].offset;
> +}
> +
> +unsigned int CameraBuffer::Private::size(unsigned int plane) const
> +{
> +	if (plane >= planeInfo_.size())
> +		return 0;
> +
> +	return planeInfo_[plane].size;
> +}
> +
>  size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const
>  {
>  	if (maps_.empty()) {
> @@ -129,19 +178,10 @@ bool CameraBuffer::Private::Map()
>  	}
>  	maps_.emplace_back(static_cast<uint8_t *>(address), bufferLength);
>  
> -	const unsigned int numPlanes = info_.numPlanes();
> -	planes_.resize(numPlanes);
> -	unsigned int offset = 0;
> -	for (unsigned int i = 0; i < numPlanes; ++i) {
> -		const unsigned int vertSubSample = info_.planes[i].verticalSubSampling;
> -		const unsigned int stride = info_.stride(size_.width, i, 1u);
> -		const unsigned int planeSize =
> -			stride * ((size_.height + vertSubSample - 1) / vertSubSample);
> -
> -		planes_[i] = libcamera::Span<uint8_t>(
> -			static_cast<uint8_t *>(address) + offset, planeSize);
> -
> -		offset += planeSize;
> +	planes_.reserve(planeInfo_.size());
> +	for (const auto &info : planeInfo_) {
> +		planes_.emplace_back(
> +			static_cast<uint8_t *>(address) + info.offset, info.size);
>  	}
>  
>  	return true;

Patch
diff mbox series 

diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h
index 87df2570..226a8f5c 100644
--- a/src/android/camera_buffer.h
+++ b/src/android/camera_buffer.h
@@ -31,6 +31,10 @@  public:
 	libcamera::Span<const uint8_t> plane(unsigned int plane) const;
 	libcamera::Span<uint8_t> plane(unsigned int plane);
 
+	unsigned int stride(unsigned int plane) const;
+	unsigned int offset(unsigned int plane) const;
+	unsigned int size(unsigned int plane) const;
+
 	size_t jpegBufferSize(size_t maxJpegBufferSize) const;
 };
 
@@ -62,6 +66,18 @@  Span<uint8_t> CameraBuffer::plane(unsigned int plane)			\
 {									\
 	return _d()->plane(plane);					\
 }									\
+unsigned int CameraBuffer::stride(unsigned int plane) const		\
+{									\
+	return _d()->stride(plane);					\
+}									\
+unsigned int CameraBuffer::offset(unsigned int plane) const		\
+{									\
+	return _d()->offset(plane);					\
+}									\
+unsigned int CameraBuffer::size(unsigned int plane) const		\
+{									\
+	return _d()->size(plane);					\
+}									\
 size_t CameraBuffer::jpegBufferSize(size_t maxJpegBufferSize) const	\
 {									\
 	return _d()->jpegBufferSize(maxJpegBufferSize);			\
diff --git a/src/android/mm/cros_camera_buffer.cpp b/src/android/mm/cros_camera_buffer.cpp
index 85ef6480..4ba24f79 100644
--- a/src/android/mm/cros_camera_buffer.cpp
+++ b/src/android/mm/cros_camera_buffer.cpp
@@ -31,6 +31,10 @@  public:
 
 	Span<uint8_t> plane(unsigned int plane);
 
+	unsigned int stride(unsigned int plane) const;
+	unsigned int offset(unsigned int plane) const;
+	unsigned int size(unsigned int plane) const;
+
 	size_t jpegBufferSize(size_t maxJpegBufferSize) const;
 
 private:
@@ -112,6 +116,21 @@  Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane)
 		 bufferManager_->GetPlaneSize(handle_, plane) };
 }
 
+unsigned int CameraBuffer::Private::stride(unsigned int plane) const
+{
+	return cros::CameraBufferManager::GetPlaneStride(handle_, plane);
+}
+
+unsigned int CameraBuffer::Private::offset(unsigned int plane) const
+{
+	return cros::CameraBufferManager::GetPlaneOffset(handle_, plane);
+}
+
+unsigned int CameraBuffer::Private::size(unsigned int plane) const
+{
+	return cros::CameraBufferManager::GetPlaneSize(handle_, plane);
+}
+
 size_t CameraBuffer::Private::jpegBufferSize([[maybe_unused]] size_t maxJpegBufferSize) const
 {
 	return bufferManager_->GetPlaneSize(handle_, 0);
diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp
index 6c1e4611..b296b3e3 100644
--- a/src/android/mm/generic_camera_buffer.cpp
+++ b/src/android/mm/generic_camera_buffer.cpp
@@ -34,15 +34,24 @@  public:
 
 	Span<uint8_t> plane(unsigned int plane);
 
+	unsigned int stride(unsigned int plane) const;
+	unsigned int offset(unsigned int plane) const;
+	unsigned int size(unsigned int plane) const;
+
 	size_t jpegBufferSize(size_t maxJpegBufferSize) const;
 
 private:
+	struct PlaneInfo {
+		unsigned int stride;
+		unsigned int offset;
+		unsigned int size;
+	};
+
 	bool Map();
 
 	int fd_;
 	int flags_;
-	libcamera::Size size_;
-	libcamera::PixelFormatInfo info_;
+	std::vector<PlaneInfo> planeInfo_;
 	/* \todo remove planes_ is added to MappedBuffer. */
 	std::vector<Span<uint8_t>> planes_;
 };
@@ -51,7 +60,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), size_(size)
+	: fd_(-1), flags_(flags)
 {
 	error_ = 0;
 
@@ -68,13 +77,29 @@  CameraBuffer::Private::Private([[maybe_unused]] CameraBuffer *cameraBuffer,
 		return;
 	}
 
-	info_ = libcamera::PixelFormatInfo::info(pixelFormat);
-	if (!info_.isValid()) {
+	const auto &info = libcamera::PixelFormatInfo::info(pixelFormat);
+	if (!info.isValid()) {
 		error_ = EINVAL;
 		LOG(HAL, Error) << "Invalid pixel format: "
 				<< pixelFormat.toString();
 		return;
 	}
+
+	const unsigned int numPlanes = info.numPlanes();
+	planeInfo_.resize(numPlanes);
+	unsigned int offset = 0;
+	for (unsigned int i = 0; i < numPlanes; ++i) {
+		const unsigned int vertSubSample = info.planes[i].verticalSubSampling;
+		const unsigned int stride = info.stride(size.width, i, 1u);
+		const unsigned int planeSize =
+			stride * ((size.height + vertSubSample - 1) / vertSubSample);
+
+		planeInfo_[i].stride = stride;
+		planeInfo_[i].offset = offset;
+		planeInfo_[i].size = planeSize;
+
+		offset += planeSize;
+	}
 }
 
 CameraBuffer::Private::~Private()
@@ -83,7 +108,7 @@  CameraBuffer::Private::~Private()
 
 unsigned int CameraBuffer::Private::numPlanes() const
 {
-	return info_.numPlanes();
+	return planeInfo_.size();
 }
 
 Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane)
@@ -97,6 +122,30 @@  Span<uint8_t> CameraBuffer::Private::plane(unsigned int plane)
 	return planes_[plane];
 }
 
+unsigned int CameraBuffer::Private::stride(unsigned int plane) const
+{
+	if (plane >= planeInfo_.size())
+		return 0;
+
+	return planeInfo_[plane].stride;
+}
+
+unsigned int CameraBuffer::Private::offset(unsigned int plane) const
+{
+	if (plane >= planeInfo_.size())
+		return 0;
+
+	return planeInfo_[plane].offset;
+}
+
+unsigned int CameraBuffer::Private::size(unsigned int plane) const
+{
+	if (plane >= planeInfo_.size())
+		return 0;
+
+	return planeInfo_[plane].size;
+}
+
 size_t CameraBuffer::Private::jpegBufferSize(size_t maxJpegBufferSize) const
 {
 	if (maps_.empty()) {
@@ -129,19 +178,10 @@  bool CameraBuffer::Private::Map()
 	}
 	maps_.emplace_back(static_cast<uint8_t *>(address), bufferLength);
 
-	const unsigned int numPlanes = info_.numPlanes();
-	planes_.resize(numPlanes);
-	unsigned int offset = 0;
-	for (unsigned int i = 0; i < numPlanes; ++i) {
-		const unsigned int vertSubSample = info_.planes[i].verticalSubSampling;
-		const unsigned int stride = info_.stride(size_.width, i, 1u);
-		const unsigned int planeSize =
-			stride * ((size_.height + vertSubSample - 1) / vertSubSample);
-
-		planes_[i] = libcamera::Span<uint8_t>(
-			static_cast<uint8_t *>(address) + offset, planeSize);
-
-		offset += planeSize;
+	planes_.reserve(planeInfo_.size());
+	for (const auto &info : planeInfo_) {
+		planes_.emplace_back(
+			static_cast<uint8_t *>(address) + info.offset, info.size);
 	}
 
 	return true;