[libcamera-devel] [PATCH v2 3/9] android: camera_device: Support multiple stream configurations

[Niklas Söderlund]

Hi Kieran,

Thanks for your work.

On 2020-07-02 22:36:48 +0100, Kieran Bingham wrote:
> Create an initial Camera Configuration using an empty role set, and
> populate the StreamConfigurations manually from each of the streams
> given by the Android camera3_stream_configuration_t stream_list.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  src/android/camera_device.cpp | 51 +++++++++++++++++------------------
>  1 file changed, 25 insertions(+), 26 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index b9031ff0c2a4..03dcdd520794 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -942,6 +942,16 @@ PixelFormat CameraDevice::toPixelFormat(int format)
>   */
>  int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  {
> +	/*
> +	 * Generate an empty configuration, and construct a StreamConfiguration
> +	 * for each camera3_stream to add to it.
> +	 */
> +	config_ = camera_->generateConfiguration();
> +	if (!config_) {
> +		LOG(HAL, Error) << "Failed to generate camera configuration";
> +		return -EINVAL;
> +	}
> +
>  	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
>  		camera3_stream_t *stream = stream_list->streams[i];
>  
> @@ -953,35 +963,18 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  			       << ", height: " << stream->height
>  			       << ", format: " << utils::hex(stream->format)
>  			       << " (" << format.toString() << ")";
> -	}
>  
> -	/* Only one stream is supported. */
> -	if (stream_list->num_streams != 1) {
> -		LOG(HAL, Error) << "Only one stream supported";
> -		return -EINVAL;
> -	}
> -	camera3_stream_t *camera3Stream = stream_list->streams[0];
> +		if (!format.isValid())
> +			return -EINVAL;
>  
> -	/* Translate Android format code to libcamera pixel format. */
> -	PixelFormat format = toPixelFormat(camera3Stream->format);
> -	if (!format.isValid())
> -		return -EINVAL;
> +		StreamConfiguration streamConfiguration;
>  
> -	/*
> -	 * Hardcode viewfinder role, replacing the generated configuration
> -	 * parameters with the ones requested by the Android framework.
> -	 */
> -	StreamRoles roles = { StreamRole::Viewfinder };
> -	config_ = camera_->generateConfiguration(roles);
> -	if (!config_ || config_->empty()) {
> -		LOG(HAL, Error) << "Failed to generate camera configuration";
> -		return -EINVAL;
> -	}
> +		streamConfiguration.size.width = stream->width;
> +		streamConfiguration.size.height = stream->height;
> +		streamConfiguration.pixelFormat = format;

This is not related to this patch but blindly translating a HAL format 
using a lookup table to a libcamera format will not work for RAW. I'm 
just curious if it is sufficient for JPEG?

>  
> -	StreamConfiguration *streamConfiguration = &config_->at(0);
> -	streamConfiguration->size.width = camera3Stream->width;
> -	streamConfiguration->size.height = camera3Stream->height;
> -	streamConfiguration->pixelFormat = format;
> +		config_->addConfiguration(streamConfiguration);
> +	}
>  
>  	switch (config_->validate()) {
>  	case CameraConfiguration::Valid:
> @@ -996,7 +989,13 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  		return -EINVAL;
>  	}
>  
> -	camera3Stream->max_buffers = streamConfiguration->bufferCount;
> +	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {

Small nit, would it make sens to iterate over config_ instead of 
stream_list->num_streams? If we have a discrepancy we would end up with 
less streams then requested but if we use ->at(i) an exception.

This is a small thing that poped out at me, with or without this 
changed,

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

> +		camera3_stream_t *stream = stream_list->streams[i];
> +		StreamConfiguration &streamConfiguration = config_->at(i);
> +
> +		/* Use the bufferCount confirmed by the validation process. */
> +		stream->max_buffers = streamConfiguration.bufferCount;
> +	}
>  
>  	/*
>  	 * Once the CameraConfiguration has been adjusted/validated
> -- 
> 2.25.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund