[libcamera-devel] [PATCH 7/8] android: camera_device: Translate Android format

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, May 26, 2020 at 04:22:36PM +0200, Jacopo Mondi wrote:
> Translate the Android format code to the libcamera format code
> at stream configuration time, using the translation map built at
> camera device initialization time.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/android/camera_device.cpp | 26 +++++++++++++++++---------
>  1 file changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index f8a52342abe5..b0f5a6a2edb5 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -904,12 +904,27 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  			       << ", format: " << utils::hex(stream->format);
>  	}
>  
> -	/* Hardcode viewfinder role, collecting sizes from the stream config. */
> +	/* Only one stream is supported. */
>  	if (stream_list->num_streams != 1) {
>  		LOG(HAL, Error) << "Only one stream supported";
>  		return -EINVAL;
>  	}

Blank line here ?

> +	camera3_stream_t *camera3Stream = stream_list->streams[0];
> +
> +	/* Translate Android format code to libcamera format code. */
> +	auto it = formatsMap_.find(camera3Stream->format);
> +	if (it == formatsMap_.end()) {
> +		LOG(HAL, Error) << "Requested format "
> +				<< utils::hex(camera3Stream->format)
> +				<< " not supported";
> +		return -EINVAL;
> +	}
> +	uint32_t libcameraFormatCode = it->second;

Let's make this a PixelFormat too.

>  
> +	/*
> +	 * Hardcode viewfinder role, replacing the generate configuration

s/generate/generated/

> +	 * parameters with the Android framework requestes ones.

s/requestes/requested/

This sounds a bit strange to my ears though, I would have written "...
with the ones requested by the Android framework". I've asked Paul who
confirmed, as I don't always trust my gut feelings about English, but
I'm not sure why I find your formulation strange :-)

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

> +	 */
>  	StreamRoles roles = { StreamRole::Viewfinder };
>  	config_ = camera_->generateConfiguration(roles);
>  	if (!config_ || config_->empty()) {
> @@ -917,17 +932,10 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  		return -EINVAL;
>  	}
>  
> -	/* Only one stream is supported. */
> -	camera3_stream_t *camera3Stream = stream_list->streams[0];
>  	StreamConfiguration *streamConfiguration = &config_->at(0);
>  	streamConfiguration->size.width = camera3Stream->width;
>  	streamConfiguration->size.height = camera3Stream->height;
> -
> -	/*
> -	 * \todo We'll need to translate from Android defined pixel format codes
> -	 * to the libcamera image format codes. For now, do not change the
> -	 * format returned from Camera::generateConfiguration().
> -	 */
> +	streamConfiguration->pixelFormat = PixelFormat(libcameraFormatCode);
>  
>  	switch (config_->validate()) {
>  	case CameraConfiguration::Valid:

-- 
Regards,

Laurent Pinchart