[libcamera-devel] [PATCH v2 4/5] android: camera_device: Use YUV post-processor

[Umang Jain]

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 at 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 at chromium.org>
> Signed-off-by: Jacopo Mondi <jacopo at 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 at 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. */