[libcamera-devel,RFC,v2,09/10] android: camera_device: Fill offset and right length in CreateFrameBuffer()
diff mbox series

Message ID 20210823131221.1034059-10-hiroh@chromium.org
State Superseded
Headers show
Series
  • Add offset to FrameBuffer::Plane
Related show

Commit Message

Hirokazu Honda Aug. 23, 2021, 1:12 p.m. UTC
CameraDevice::CreateFrameBuffer() fills the length of the buffer to
each FrameBuffer::Plane::length. It should rather be the length of
plane. This also changes CreateFrameBuffer() to fill offset of
FrameBuffer::Plane.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
---
 src/android/camera_device.cpp | 47 ++++++++++++++++++++---------------
 src/android/camera_device.h   |  6 ++++-
 2 files changed, 32 insertions(+), 21 deletions(-)

Comments

Laurent Pinchart Aug. 24, 2021, 12:15 a.m. UTC | #1
Hi Hiro,

Thank you for the patch.

On Mon, Aug 23, 2021 at 10:12:20PM +0900, Hirokazu Honda wrote:
> CameraDevice::CreateFrameBuffer() fills the length of the buffer to
> each FrameBuffer::Plane::length. It should rather be the length of
> plane. This also changes CreateFrameBuffer() to fill offset of
> FrameBuffer::Plane.
> 
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> ---
>  src/android/camera_device.cpp | 47 ++++++++++++++++++++---------------
>  src/android/camera_device.h   |  6 ++++-
>  2 files changed, 32 insertions(+), 21 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index a69b687a..6663817d 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -12,6 +12,7 @@
>  
>  #include <algorithm>
>  #include <fstream>
> +#include <sys/mman.h>
>  #include <unistd.h>
>  #include <vector>
>  
> @@ -744,31 +745,35 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  	return 0;
>  }
>  
> -FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer)
> +FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer,
> +					     libcamera::PixelFormat pixelFormat,
> +					     const libcamera::Size &size)
>  {
> -	std::vector<FrameBuffer::Plane> planes;
> +	FileDescriptor fd;
> +	/* This assumes all the planes are in the same buffer. */

s/buffer/dmabuf/

I'd also add

	/*
	 * \todo Verify that this assumption holds, fstat() can be used to check
	 * if two fds refer to the same dmabuf.
	 */

>  	for (int i = 0; i < camera3buffer->numFds; i++) {
> -		/* Skip unused planes. */
> -		if (camera3buffer->data[i] == -1)
> +		if (camera3buffer->data[i] != -1) {
> +			fd = FileDescriptor(camera3buffer->data[i]);
>  			break;
> -
> -		FrameBuffer::Plane plane;
> -		plane.fd = FileDescriptor(camera3buffer->data[i]);
> -		if (!plane.fd.isValid()) {
> -			LOG(HAL, Error) << "Failed to obtain FileDescriptor ("
> -					<< camera3buffer->data[i] << ") "
> -					<< " on plane " << i;
> -			return nullptr;
>  		}
> +	}
>  
> -		off_t length = lseek(plane.fd.fd(), 0, SEEK_END);
> -		if (length == -1) {
> -			LOG(HAL, Error) << "Failed to query plane length";
> -			return nullptr;
> -		}
> +	if (!fd.isValid()) {
> +		LOG(HAL, Fatal) << "No valid fd";
> +		return nullptr;
> +	}
> +
> +	CameraBuffer buf(camera3buffer, pixelFormat, size, PROT_READ);
> +	if (!buf.isValid()) {
> +		LOG(HAL, Fatal) << "Failed mapping buffer";

We don't map the buffer anymore :-) "Failed to create CameraBuffer"
maybe ?

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

> +		return nullptr;
> +	}
>  
> -		plane.length = length;
> -		planes.push_back(std::move(plane));
> +	std::vector<FrameBuffer::Plane> planes(buf.numPlanes());
> +	for (size_t i = 0; i < buf.numPlanes(); ++i) {
> +		planes[i].fd = fd;
> +		planes[i].offset = buf.offset(i);
> +		planes[i].length = buf.size(i);
>  	}
>  
>  	return new FrameBuffer(std::move(planes));
> @@ -976,7 +981,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  			 * associate it with the Camera3RequestDescriptor for
>  			 * lifetime management only.
>  			 */
> -			buffer = createFrameBuffer(*camera3Buffer.buffer);
> +			buffer = createFrameBuffer(*camera3Buffer.buffer,
> +						   cameraStream->configuration().pixelFormat,
> +						   cameraStream->configuration().size);
>  			descriptor.frameBuffers_.emplace_back(buffer);
>  			LOG(HAL, Debug) << ss.str() << " (direct)";
>  			break;
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index dd9aebba..a5576927 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -21,6 +21,8 @@
>  
>  #include <libcamera/camera.h>
>  #include <libcamera/framebuffer.h>
> +#include <libcamera/geometry.h>
> +#include <libcamera/pixel_format.h>
>  #include <libcamera/request.h>
>  #include <libcamera/stream.h>
>  
> @@ -91,7 +93,9 @@ private:
>  
>  	void stop();
>  
> -	libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);
> +	libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer,
> +						  libcamera::PixelFormat pixelFormat,
> +						  const libcamera::Size &size);
>  	void abortRequest(camera3_capture_request_t *request);
>  	bool isValidRequest(camera3_capture_request_t *request) const;
>  	void notifyShutter(uint32_t frameNumber, uint64_t timestamp);

Patch
diff mbox series

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index a69b687a..6663817d 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -12,6 +12,7 @@ 
 
 #include <algorithm>
 #include <fstream>
+#include <sys/mman.h>
 #include <unistd.h>
 #include <vector>
 
@@ -744,31 +745,35 @@  int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
 	return 0;
 }
 
-FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer)
+FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer,
+					     libcamera::PixelFormat pixelFormat,
+					     const libcamera::Size &size)
 {
-	std::vector<FrameBuffer::Plane> planes;
+	FileDescriptor fd;
+	/* This assumes all the planes are in the same buffer. */
 	for (int i = 0; i < camera3buffer->numFds; i++) {
-		/* Skip unused planes. */
-		if (camera3buffer->data[i] == -1)
+		if (camera3buffer->data[i] != -1) {
+			fd = FileDescriptor(camera3buffer->data[i]);
 			break;
-
-		FrameBuffer::Plane plane;
-		plane.fd = FileDescriptor(camera3buffer->data[i]);
-		if (!plane.fd.isValid()) {
-			LOG(HAL, Error) << "Failed to obtain FileDescriptor ("
-					<< camera3buffer->data[i] << ") "
-					<< " on plane " << i;
-			return nullptr;
 		}
+	}
 
-		off_t length = lseek(plane.fd.fd(), 0, SEEK_END);
-		if (length == -1) {
-			LOG(HAL, Error) << "Failed to query plane length";
-			return nullptr;
-		}
+	if (!fd.isValid()) {
+		LOG(HAL, Fatal) << "No valid fd";
+		return nullptr;
+	}
+
+	CameraBuffer buf(camera3buffer, pixelFormat, size, PROT_READ);
+	if (!buf.isValid()) {
+		LOG(HAL, Fatal) << "Failed mapping buffer";
+		return nullptr;
+	}
 
-		plane.length = length;
-		planes.push_back(std::move(plane));
+	std::vector<FrameBuffer::Plane> planes(buf.numPlanes());
+	for (size_t i = 0; i < buf.numPlanes(); ++i) {
+		planes[i].fd = fd;
+		planes[i].offset = buf.offset(i);
+		planes[i].length = buf.size(i);
 	}
 
 	return new FrameBuffer(std::move(planes));
@@ -976,7 +981,9 @@  int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
 			 * associate it with the Camera3RequestDescriptor for
 			 * lifetime management only.
 			 */
-			buffer = createFrameBuffer(*camera3Buffer.buffer);
+			buffer = createFrameBuffer(*camera3Buffer.buffer,
+						   cameraStream->configuration().pixelFormat,
+						   cameraStream->configuration().size);
 			descriptor.frameBuffers_.emplace_back(buffer);
 			LOG(HAL, Debug) << ss.str() << " (direct)";
 			break;
diff --git a/src/android/camera_device.h b/src/android/camera_device.h
index dd9aebba..a5576927 100644
--- a/src/android/camera_device.h
+++ b/src/android/camera_device.h
@@ -21,6 +21,8 @@ 
 
 #include <libcamera/camera.h>
 #include <libcamera/framebuffer.h>
+#include <libcamera/geometry.h>
+#include <libcamera/pixel_format.h>
 #include <libcamera/request.h>
 #include <libcamera/stream.h>
 
@@ -91,7 +93,9 @@  private:
 
 	void stop();
 
-	libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);
+	libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer,
+						  libcamera::PixelFormat pixelFormat,
+						  const libcamera::Size &size);
 	void abortRequest(camera3_capture_request_t *request);
 	bool isValidRequest(camera3_capture_request_t *request) const;
 	void notifyShutter(uint32_t frameNumber, uint64_t timestamp);