[libcamera-devel,v2,4/5] android: camera_device: Use YUV post-processor
diff mbox series

Message ID 20220527093440.953377-5-paul.elder@ideasonboard.com
State Superseded
Headers show
Series
  • Plumb the YUV processor in
Related show

Commit Message

Paul Elder May 27, 2022, 9:34 a.m. UTC 
From: Hirokazu Honda <hiroh@chromium.org>

When creating the list of StreamConfiguration to be requested to the camera,
map NV12 streams of equal size and format together, so that they will be
generated by using the YUV post-processor.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/android/camera_device.cpp | 32 +++++++++++++++++++++++++++++---
 1 file changed, 29 insertions(+), 3 deletions(-)

Comments

Laurent Pinchart June 2, 2022, 8:38 a.m. UTC | #1 
Hi Paul and Hiro,

Thank you for the patch.

On Fri, May 27, 2022 at 06:34:39PM +0900, Paul Elder via libcamera-devel wrote:
> From: Hirokazu Honda <hiroh@chromium.org>
> 
> When creating the list of StreamConfiguration to be requested to the camera,
> map NV12 streams of equal size and format together, so that they will be
> generated by using the YUV post-processor.
> 
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/android/camera_device.cpp | 32 +++++++++++++++++++++++++++++---
>  1 file changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 95c14f60..9ee34b93 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -605,14 +605,40 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  			continue;
>  		}
>  
> +		/*
> +		 * While gralloc usage flags are supposed to report usage
> +		 * patterns to select a suitable buffer allocation strategy, in
> +		 * practice they're also used to make other decisions, such as
> +		 * selecting the actual format for the IMPLEMENTATION_DEFINED
> +		 * HAL pixel format. To avoid issues, we thus have to set the
> +		 * GRALLOC_USAGE_HW_CAMERA_WRITE flag unconditionally, even for
> +		 * streams that will be produced in software.
> +		 */
> +		stream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE;
> +
> +		/*
> +		 * If a CameraStream with the same size and format of the

s/of the/as the/

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

> +		 * current stream has already been requested, associate the two.
> +		 */
> +		auto iter = std::find_if(
> +			streamConfigs.begin(), streamConfigs.end(),
> +			[&size, &format](const Camera3StreamConfig &streamConfig) {
> +				return streamConfig.config.size == size &&
> +				       streamConfig.config.pixelFormat == format;
> +			});
> +		if (iter != streamConfigs.end()) {
> +			/* Add usage to copy the buffer in streams[0] to stream. */
> +			iter->streams[0].stream->usage |= GRALLOC_USAGE_SW_READ_OFTEN;
> +			stream->usage |= GRALLOC_USAGE_SW_WRITE_OFTEN;
> +			iter->streams.push_back({ stream, CameraStream::Type::Mapped });
> +			continue;
> +		}
> +
>  		Camera3StreamConfig streamConfig;
>  		streamConfig.streams = { { stream, CameraStream::Type::Direct } };
>  		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. */
Umang Jain June 2, 2022, 8:48 a.m. UTC | #2 
Hi all,

Thank you for the patch.

The patch looks good in what its supposed to do, but just that I've 
trouble understanding the GRALLOC flags..

On 5/27/22 11:34, Paul Elder via libcamera-devel wrote:
> From: Hirokazu Honda <hiroh@chromium.org>
>
> When creating the list of StreamConfiguration to be requested to the camera,
> map NV12 streams of equal size and format together, so that they will be
> generated by using the YUV post-processor.


Might add one line to add a summary/update of usage of GRALLOC flags too.
But it's there in the comment so consider them optional.

>
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>   src/android/camera_device.cpp | 32 +++++++++++++++++++++++++++++---
>   1 file changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 95c14f60..9ee34b93 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -605,14 +605,40 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>   			continue;
>   		}
>   
> +		/*
> +		 * While gralloc usage flags are supposed to report usage
> +		 * patterns to select a suitable buffer allocation strategy, in
> +		 * practice they're also used to make other decisions, such as
> +		 * selecting the actual format for the IMPLEMENTATION_DEFINED
> +		 * HAL pixel format. To avoid issues, we thus have to set the
> +		 * GRALLOC_USAGE_HW_CAMERA_WRITE flag unconditionally, even for
> +		 * streams that will be produced in software.
> +		 */
> +		stream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE;


Ok, we set this unconditionally for all camera3_stream_t(s) in the list.
It's not really "setting it" rather appending to the existing flags

> +
> +		/*
> +		 * If a CameraStream with the same size and format of the
> +		 * current stream has already been requested, associate the two.
> +		 */
> +		auto iter = std::find_if(
> +			streamConfigs.begin(), streamConfigs.end(),
> +			[&size, &format](const Camera3StreamConfig &streamConfig) {
> +				return streamConfig.config.size == size &&
> +				       streamConfig.config.pixelFormat == format;
> +			});
> +		if (iter != streamConfigs.end()) {
> +			/* Add usage to copy the buffer in streams[0] to stream. */
> +			iter->streams[0].stream->usage |= GRALLOC_USAGE_SW_READ_OFTEN;
> +			stream->usage |= GRALLOC_USAGE_SW_WRITE_OFTEN;


And then if the similar stream is previously requested, we append 
GRALLOC_USAGE_SW_WRITE_OFTEN as well.
So stream->usage will now currently have both 
GRALLOC_USAGE_HW_CAMERA_WRITE and GRALLOC_USAGE_SW_WRITE_OFTEN ?

I am wondering if that's OK / accepted? or is it just one of two. Is 
there any priority mechanism for flags? There is limited visibility on 
usage of these flags

I am reading map_usage_to_memtrack() in 
include/android/hardware/libhardware/include/hardware/gralloc.h
It seems it will just return on checking GRALLOC_USAGE_HW_CAMERA_WRITE 
and that's it. So no other flags are taken into consideration if that's set.
(Also, there might other functions handling the flags  as well in the 
frame work)

So yes, I am not confident with the flags yet, so as long as they seem 
to work okay (Thank you Jacopo for multiple CTS run testing)

Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

> +			iter->streams.push_back({ stream, CameraStream::Type::Mapped });
> +			continue;
> +		}
> +
>   		Camera3StreamConfig streamConfig;
>   		streamConfig.streams = { { stream, CameraStream::Type::Direct } };
>   		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. */

Patch
diff mbox series 

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index 95c14f60..9ee34b93 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -605,14 +605,40 @@  int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
 			continue;
 		}
 
+		/*
+		 * While gralloc usage flags are supposed to report usage
+		 * patterns to select a suitable buffer allocation strategy, in
+		 * practice they're also used to make other decisions, such as
+		 * selecting the actual format for the IMPLEMENTATION_DEFINED
+		 * HAL pixel format. To avoid issues, we thus have to set the
+		 * GRALLOC_USAGE_HW_CAMERA_WRITE flag unconditionally, even for
+		 * streams that will be produced in software.
+		 */
+		stream->usage |= GRALLOC_USAGE_HW_CAMERA_WRITE;
+
+		/*
+		 * If a CameraStream with the same size and format of the
+		 * current stream has already been requested, associate the two.
+		 */
+		auto iter = std::find_if(
+			streamConfigs.begin(), streamConfigs.end(),
+			[&size, &format](const Camera3StreamConfig &streamConfig) {
+				return streamConfig.config.size == size &&
+				       streamConfig.config.pixelFormat == format;
+			});
+		if (iter != streamConfigs.end()) {
+			/* Add usage to copy the buffer in streams[0] to stream. */
+			iter->streams[0].stream->usage |= GRALLOC_USAGE_SW_READ_OFTEN;
+			stream->usage |= GRALLOC_USAGE_SW_WRITE_OFTEN;
+			iter->streams.push_back({ stream, CameraStream::Type::Mapped });
+			continue;
+		}
+
 		Camera3StreamConfig streamConfig;
 		streamConfig.streams = { { stream, CameraStream::Type::Direct } };
 		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. */