[libcamera-devel] android: camera_device: Update gralloc usage flags for streams
diff mbox series

Message ID 20210305115628.19811-1-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • [libcamera-devel] android: camera_device: Update gralloc usage flags for streams
Related show

Commit Message

Laurent Pinchart March 5, 2021, 11:56 a.m. UTC 
When configuring streams, the camera HAL is supposed to update the
gralloc usage flags to reflect the operations it will need to do on the
stream buffers. Failure to do so leads to incorrect format selection by
gralloc for the HAL_PIXEL_FORMAT_IMPLEMENTATION_DEFINED format, as
gralloc will not take into consideration the need of the camera to
access the buffers.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/android/camera_device.cpp | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

Comments

Jacopo Mondi March 5, 2021, 7:58 p.m. UTC | #1 
Hi Laurent,

On Fri, Mar 05, 2021 at 01:56:28PM +0200, Laurent Pinchart wrote:
> When configuring streams, the camera HAL is supposed to update the
> gralloc usage flags to reflect the operations it will need to do on the
> stream buffers. Failure to do so leads to incorrect format selection by
> gralloc for the HAL_PIXEL_FORMAT_IMPLEMENTATION_DEFINED format, as
> gralloc will not take into consideration the need of the camera to
> access the buffers.

You know how happy this makes me!

I've tested with OpenCamera and run CTS without noticing regressions,
so please add
Tested-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
 j


>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/android/camera_device.cpp | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index ae01c362559e..3ee683705d00 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1539,6 +1539,9 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  		streamConfig.config.size = size;
>  		streamConfig.config.pixelFormat = format;
>  		streamConfigs.push_back(std::move(streamConfig));
> +
> +		/* This stream will be produced by hardware. */
> +		stream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE;
>  	}
>
>  	/* Now handle the MJPEG streams, adding a new stream if required. */
> @@ -1548,7 +1551,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>
>  		/* Search for a compatible stream in the non-JPEG ones. */
>  		for (size_t i = 0; i < streamConfigs.size(); ++i) {
> -			const auto &cfg = streamConfigs[i].config;
> +			Camera3StreamConfig &streamConfig = streamConfigs[i];
> +			const auto &cfg = streamConfig.config;
>
>  			/*
>  			 * \todo The PixelFormat must also be compatible with
> @@ -1563,6 +1567,13 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>
>  			type = CameraStream::Type::Mapped;
>  			index = i;
> +
> +			/*
> +			 * The source stream will be read by software to
> +			 * produce the JPEG stream.
> +			 */
> +			camera3_stream_t *stream = streamConfig.streams[0].stream;
> +			stream->usage |= GRALLOC_USAGE_SW_READ_OFTEN;
>  			break;
>  		}
>
> @@ -1590,6 +1601,9 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  			index = streamConfigs.size() - 1;
>  		}
>
> +		/* The JPEG stream will be produced by software. */
> +		jpegStream->usage |= GRALLOC_USAGE_SW_WRITE_OFTEN;
> +
>  		streamConfigs[index].streams.push_back({ jpegStream, type });
>  	}
>
> --
> Regards,
>
> Laurent Pinchart
>
Jacopo Mondi March 5, 2021, 8:19 p.m. UTC | #2 
Oh and I forgot, tested-by implies
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

On Fri, Mar 05, 2021 at 01:56:28PM +0200, Laurent Pinchart wrote:
> When configuring streams, the camera HAL is supposed to update the
> gralloc usage flags to reflect the operations it will need to do on the
> stream buffers. Failure to do so leads to incorrect format selection by
> gralloc for the HAL_PIXEL_FORMAT_IMPLEMENTATION_DEFINED format, as
> gralloc will not take into consideration the need of the camera to
> access the buffers.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/android/camera_device.cpp | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index ae01c362559e..3ee683705d00 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1539,6 +1539,9 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  		streamConfig.config.size = size;
>  		streamConfig.config.pixelFormat = format;
>  		streamConfigs.push_back(std::move(streamConfig));
> +
> +		/* This stream will be produced by hardware. */
> +		stream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE;
>  	}
>
>  	/* Now handle the MJPEG streams, adding a new stream if required. */
> @@ -1548,7 +1551,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>
>  		/* Search for a compatible stream in the non-JPEG ones. */
>  		for (size_t i = 0; i < streamConfigs.size(); ++i) {
> -			const auto &cfg = streamConfigs[i].config;
> +			Camera3StreamConfig &streamConfig = streamConfigs[i];
> +			const auto &cfg = streamConfig.config;
>
>  			/*
>  			 * \todo The PixelFormat must also be compatible with
> @@ -1563,6 +1567,13 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>
>  			type = CameraStream::Type::Mapped;
>  			index = i;
> +
> +			/*
> +			 * The source stream will be read by software to
> +			 * produce the JPEG stream.
> +			 */
> +			camera3_stream_t *stream = streamConfig.streams[0].stream;
> +			stream->usage |= GRALLOC_USAGE_SW_READ_OFTEN;
>  			break;
>  		}
>
> @@ -1590,6 +1601,9 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  			index = streamConfigs.size() - 1;
>  		}
>
> +		/* The JPEG stream will be produced by software. */
> +		jpegStream->usage |= GRALLOC_USAGE_SW_WRITE_OFTEN;
> +
>  		streamConfigs[index].streams.push_back({ jpegStream, type });
>  	}
>
> --
> Regards,
>
> Laurent Pinchart
>
Niklas Söderlund March 6, 2021, 10:38 p.m. UTC | #3 
Hi Laurent,

Thanks for your work.

On 2021-03-05 13:56:28 +0200, Laurent Pinchart wrote:
> When configuring streams, the camera HAL is supposed to update the
> gralloc usage flags to reflect the operations it will need to do on the
> stream buffers. Failure to do so leads to incorrect format selection by
> gralloc for the HAL_PIXEL_FORMAT_IMPLEMENTATION_DEFINED format, as
> gralloc will not take into consideration the need of the camera to
> access the buffers.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

This indeed fixes the OpenCamera issue.

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  src/android/camera_device.cpp | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index ae01c362559e..3ee683705d00 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1539,6 +1539,9 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  		streamConfig.config.size = size;
>  		streamConfig.config.pixelFormat = format;
>  		streamConfigs.push_back(std::move(streamConfig));
> +
> +		/* This stream will be produced by hardware. */
> +		stream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE;
>  	}
>  
>  	/* Now handle the MJPEG streams, adding a new stream if required. */
> @@ -1548,7 +1551,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  
>  		/* Search for a compatible stream in the non-JPEG ones. */
>  		for (size_t i = 0; i < streamConfigs.size(); ++i) {
> -			const auto &cfg = streamConfigs[i].config;
> +			Camera3StreamConfig &streamConfig = streamConfigs[i];
> +			const auto &cfg = streamConfig.config;
>  
>  			/*
>  			 * \todo The PixelFormat must also be compatible with
> @@ -1563,6 +1567,13 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  
>  			type = CameraStream::Type::Mapped;
>  			index = i;
> +
> +			/*
> +			 * The source stream will be read by software to
> +			 * produce the JPEG stream.
> +			 */
> +			camera3_stream_t *stream = streamConfig.streams[0].stream;
> +			stream->usage |= GRALLOC_USAGE_SW_READ_OFTEN;
>  			break;
>  		}
>  
> @@ -1590,6 +1601,9 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  			index = streamConfigs.size() - 1;
>  		}
>  
> +		/* The JPEG stream will be produced by software. */
> +		jpegStream->usage |= GRALLOC_USAGE_SW_WRITE_OFTEN;
> +
>  		streamConfigs[index].streams.push_back({ jpegStream, type });
>  	}
>  
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Patch
diff mbox series 

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index ae01c362559e..3ee683705d00 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -1539,6 +1539,9 @@  int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
 		streamConfig.config.size = size;
 		streamConfig.config.pixelFormat = format;
 		streamConfigs.push_back(std::move(streamConfig));
+
+		/* This stream will be produced by hardware. */
+		stream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE;
 	}
 
 	/* Now handle the MJPEG streams, adding a new stream if required. */
@@ -1548,7 +1551,8 @@  int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
 
 		/* Search for a compatible stream in the non-JPEG ones. */
 		for (size_t i = 0; i < streamConfigs.size(); ++i) {
-			const auto &cfg = streamConfigs[i].config;
+			Camera3StreamConfig &streamConfig = streamConfigs[i];
+			const auto &cfg = streamConfig.config;
 
 			/*
 			 * \todo The PixelFormat must also be compatible with
@@ -1563,6 +1567,13 @@  int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
 
 			type = CameraStream::Type::Mapped;
 			index = i;
+
+			/*
+			 * The source stream will be read by software to
+			 * produce the JPEG stream.
+			 */
+			camera3_stream_t *stream = streamConfig.streams[0].stream;
+			stream->usage |= GRALLOC_USAGE_SW_READ_OFTEN;
 			break;
 		}
 
@@ -1590,6 +1601,9 @@  int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
 			index = streamConfigs.size() - 1;
 		}
 
+		/* The JPEG stream will be produced by software. */
+		jpegStream->usage |= GRALLOC_USAGE_SW_WRITE_OFTEN;
+
 		streamConfigs[index].streams.push_back({ jpegStream, type });
 	}