[libcamera-devel] [PATCH v2 02/12] android: camera_device: Generate JPEG sizes

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Sep 02, 2020 at 05:22:26PM +0200, Jacopo Mondi wrote:
> When producing the list of image resolution to claim as supported by the

s/resolution/resolutions/

> camera HAL, the JPEG stream was assumed to be 'always valid' as, at the
> time, there was no JPEG support in place at all.
> 
> With the introduction of support for JPEG compression, reporting
> non-valid sizes as supported obviously causes troubles.
> 
> In order to avoid reporting non-supported resolutions as supported,
> produce the list of available JPEG sizes by using the ones supported
> by the YCbCr_420_888 format, from which the JPEG stream is encoded.

Down the line we'll likely need to scale down images for JPEG
compression, but for now this looks fine to me.

> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/android/camera_device.cpp | 43 +++++++++++++++++++++++------------
>  1 file changed, 28 insertions(+), 15 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index ad0d7fd15d90..8a8072123961 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -363,17 +363,27 @@ int CameraDevice::initializeStreamConfigurations()
>  		const std::vector<PixelFormat> &libcameraFormats =
>  			camera3Format.libcameraFormats;
>  
> +		/*
> +		 * Fixed format mapping for JPEG.
> +		 *
> +		 * The list of supported JPEG resolutions is generated
> +		 * from the list of resolutions supported by
> +		 * HAL_PIXEL_FORMAT_YCbCr_420_888 from which JPEG is produced.

This doesn seem to correspond to the code below. Should it read

		/*
		 * JPEG is always supported, either produced directly by the
		 * camera, or encoded in the HAL.
		 */

?

> +		 *
> +		 * \todo Wire the JPEG encoder interface to query the list
> +		 * of supported resolutions.

Is this still valid ?

> +		 */
> +		if (androidFormat == HAL_PIXEL_FORMAT_BLOB) {
> +			formatsMap_[androidFormat] = formats::MJPEG;
> +			continue;
> +		}
> +
>  		/*
>  		 * Test the libcamera formats that can produce images
>  		 * compatible with the format defined by Android.
>  		 */
>  		PixelFormat mappedFormat;
>  		for (const PixelFormat &pixelFormat : libcameraFormats) {
> -			/* \todo Fixed mapping for JPEG. */
> -			if (androidFormat == HAL_PIXEL_FORMAT_BLOB) {
> -				mappedFormat = formats::MJPEG;
> -				break;
> -			}
>  
>  			/*
>  			 * The stream configuration size can be adjusted,
> @@ -416,19 +426,22 @@ int CameraDevice::initializeStreamConfigurations()
>  			cfg.size = res;
>  
>  			CameraConfiguration::Status status = cameraConfig->validate();
> -			/*
> -			 * Unconditionally report we can produce JPEG.
> -			 *
> -			 * \todo The JPEG stream will be implemented as an
> -			 * HAL-only stream, but some cameras can produce it
> -			 * directly. As of now, claim support for JPEG without
> -			 * inspecting where the JPEG stream is produced.
> -			 */
> -			if (androidFormat != HAL_PIXEL_FORMAT_BLOB &&
> -			    status != CameraConfiguration::Valid)
> +			if (status != CameraConfiguration::Valid)
>  				continue;
>  
>  			streamConfigurations_.push_back({ res, androidFormat });
> +
> +			/*
> +			 * If the format is HAL_PIXEL_FORMAT_YCbCr_420_888
> +			 * from which JPEG is produced, add an entry for
> +			 * the JPEG stream.
> +			 *
> +			 * \todo Wire the JPEG encoder to query the supported
> +			 * sizes provided a list of formats it can encode.

We shouldn't do this if the device can provide JPEG natively. Is this
something you'd like to address already ? Otherwise we should add a
\todo item for this too.

> +			 */
> +			if (androidFormat == HAL_PIXEL_FORMAT_YCbCr_420_888)
> +				streamConfigurations_.push_back(
> +					{ res, HAL_PIXEL_FORMAT_BLOB });
>  		}
>  	}
>  

-- 
Regards,

Laurent Pinchart