[libcamera-devel] [PATCH 4/4] android: camera_device: support multiple stream configurations

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Mon, Jun 29, 2020 at 05:39:16PM +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 | 72 ++++++++++++++++++-----------------
>  1 file changed, 38 insertions(+), 34 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index a6410f42fee8..568ba28067d0 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -956,48 +956,52 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  			       << " (" << 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];
> -
> -	/* Translate Android format code to libcamera pixel format. */
> -	PixelFormat format = toPixelFormat(camera3Stream->format);
> -	if (!format.isValid())
> -		return -EINVAL;
> -
>  	/*
> -	 * Hardcode viewfinder role, replacing the generated configuration
> -	 * parameters with the ones requested by the Android framework.
> +	 * Generate an empty configuration, and construct a StreamConfiguration
> +	 * for each camera3_stream to add to it.
>  	 */
> -	StreamRoles roles = { StreamRole::Viewfinder };
> -	config_ = camera_->generateConfiguration(roles);
> -	if (!config_ || config_->empty()) {
> +	config_ = camera_->generateConfiguration();
> +	if (!config_) {
>  		LOG(HAL, Error) << "Failed to generate camera configuration";
>  		return -EINVAL;
>  	}

This should really really not fail, we could possibly drop the check (it
should be caught by pipeline handler tests, albeit we don't have them
yet).

>  
> -	StreamConfiguration *streamConfiguration = &config_->at(0);
> -	streamConfiguration->size.width = camera3Stream->width;
> -	streamConfiguration->size.height = camera3Stream->height;
> -	streamConfiguration->pixelFormat = format;
> +	for (unsigned int i = 0; i < stream_list->num_streams; ++i) {
> +		camera3_stream_t *stream = stream_list->streams[i];
>  
> -	switch (config_->validate()) {
> -	case CameraConfiguration::Valid:
> -		break;
> -	case CameraConfiguration::Adjusted:
> -		LOG(HAL, Info) << "Camera configuration adjusted";
> -		config_.reset();
> -		return -EINVAL;
> -	case CameraConfiguration::Invalid:
> -		LOG(HAL, Info) << "Camera configuration invalid";
> -		config_.reset();
> -		return -EINVAL;
> -	}
> +		PixelFormat format = toPixelFormat(stream->format);
> +		if (!format.isValid())
> +			return -EINVAL;
> +
> +		StreamConfiguration streamConfiguration;
> +
> +		streamConfiguration.size.width = stream->width;
> +		streamConfiguration.size.height = stream->height;
> +		streamConfiguration.pixelFormat = format;
> +
> +		StreamConfiguration &cfg = config_->addConfiguration(streamConfiguration);
> +
> +		/* 'cfg' can be modifed during the validation process */
> +		CameraConfiguration::Status status = config_->validate();

Is there really a need to validate at each step instead of at the end ?
If it's for debugging purpose, you could print the whole camera
configuration when the validation fails, so it van be compated with the
requested stream configurations that are printed earlier. Validation is
a potentially expensive operation (involving V4L2 ioctl calls, which,
for UVC for instance, even involve interactions with the hardware), so
a loop to create the camera configuration followed by a single
validation would be best I think. You then wouldn't need patch 2/4 (I'd
like to keep changes to CameraConfiguration and StreamConfiguration
minimal until we rework them, and I'm not sure the code above that
passes a StreamConfiguration to addConfiguration() to get another
StreamConfiguration out is any less confusing than what we have today).

> +
> +		LOG(HAL, Info) << "Stream #" << i << " (validated): " << cfg.toString();

Let's try to not make the log too verbose in normal cases.

>  
> -	camera3Stream->max_buffers = streamConfiguration->bufferCount;
> +		switch (status) {
> +		case CameraConfiguration::Valid:
> +			break;
> +		case CameraConfiguration::Adjusted:
> +			LOG(HAL, Info) << "Camera configuration adjusted";
> +			config_.reset();
> +			return -EINVAL;
> +		case CameraConfiguration::Invalid:
> +			LOG(HAL, Info) << "Camera configuration invalid";
> +			config_.reset();
> +			return -EINVAL;
> +		}
> +
> +		/* Use the bufferCount confirmed by the validation process. */
> +		stream->max_buffers = cfg.bufferCount;
> +	}
>  
>  	/*
>  	 * Once the CameraConfiguration has been adjusted/validated

-- 
Regards,

Laurent Pinchart