[libcamera-devel] [PATCH v3 2/2] android: camera_device: Generate JPEG thumbnail sizes

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Feb 04, 2021 at 05:22:44PM +0100, Jacopo Mondi wrote:
> The list of the available thumbnail sizes is generated from the
> list of available JPEG resolution, one for each aspect ratio.
> 
> This change fixes the CTS test
> android.hardware.cts.CameraTest#testJpegThumbnailSize
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/android/camera_device.cpp | 55 ++++++++++++++++++++++++++++++-----
>  1 file changed, 48 insertions(+), 7 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 70170e243d06..65d37860cf95 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -705,10 +705,10 @@ std::tuple<uint32_t, uint32_t> CameraDevice::calculateStaticMetadataSize()
>  {
>  	/*
>  	 * \todo Keep this in sync with the actual number of entries.
> -	 * Currently: 53 entries, 846 bytes of static metadata
> +	 * Currently: 53 entries, 838 bytes of static metadata
>  	 */
>  	uint32_t numEntries = 53;
> -	uint32_t byteSize = 846;
> +	uint32_t byteSize = 838;
>  
>  	/*
>  	 * Calculate space occupation in bytes for dynamically built metadata
> @@ -720,6 +720,18 @@ std::tuple<uint32_t, uint32_t> CameraDevice::calculateStaticMetadataSize()
>  	 */
>  	byteSize += streamConfigurations_.size() * 48;
>  
> +	/*
> +	 * 2 32bits integers for each HAL_PIXEL_FORMAT_BLOB for thumbnail sizes
> +	 * 2 32bits integers for the (0, 0) thumbnail size

I'd add

	* This is a worst case estimates as different configurations with the
	* same aspect ratio will generate the same size.

(and there's no need to optimize this)

> +	 */
> +	for (const auto &entry : streamConfigurations_) {
> +		if (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB)
> +			continue;
> +
> +		byteSize += 8;
> +	}
> +	byteSize += 8;
> +
>  	return std::make_tuple(numEntries, byteSize);
>  }
>  
> @@ -871,12 +883,41 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
>  				  &availableControlModes, 1);
>  
>  	/* JPEG static metadata. */
> -	std::vector<int32_t> availableThumbnailSizes = {
> -		0, 0,
> -	};
> +
> +	/*
> +	 * Create the list of supported thumbnail sizes by inspecting the
> +	 * available JPEG resolutions collected in streamConfigurations_ and
> +	 * generate one entry for each aspect ratio.
> +	 *
> +	 * The JPEG thumbnailer can freely scale, so pick an arbitrary
> +	 * (160, 160) size as designated thumbnail size.

Maybe

	 * (160, 160) size as the bounding rectangle, which is then cropped to
	 * the different supported aspect ratios.

or something similar ?

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

> +	 */
> +	constexpr Size maxJpegThumbnail(160, 160);
> +	std::vector<Size> thumbnailSizes;
> +	thumbnailSizes.push_back({ 0, 0 });
> +	for (const auto &entry : streamConfigurations_) {
> +		if (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB)
> +			continue;
> +
> +		Size thumbnailSize = maxJpegThumbnail
> +				     .boundedToAspectRatio({ entry.resolution.width,
> +							     entry.resolution.height });
> +		thumbnailSizes.push_back(thumbnailSize);
> +	}
> +
> +	std::sort(thumbnailSizes.begin(), thumbnailSizes.end());
> +	auto last = std::unique(thumbnailSizes.begin(), thumbnailSizes.end());
> +	thumbnailSizes.erase(last, thumbnailSizes.end());
> +
> +	/* Transform sizes in to a list of integers that can be consumed. */
> +	std::vector<int32_t> thumbnailEntries;
> +	thumbnailEntries.reserve(thumbnailSizes.size() * 2);
> +	for (const auto &size : thumbnailSizes) {
> +		thumbnailEntries.push_back(size.width);
> +		thumbnailEntries.push_back(size.height);
> +	}
>  	staticMetadata_->addEntry(ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES,
> -				  availableThumbnailSizes.data(),
> -				  availableThumbnailSizes.size());
> +				  thumbnailEntries.data(), thumbnailEntries.size());
>  
>  	/*
>  	 * \todo Calculate the maximum JPEG buffer size by asking the encoder

-- 
Regards,

Laurent Pinchart