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

[Kieran Bingham]

Hi Jacopo,

On 04/02/2021 14:35, 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
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/android/camera_device.cpp | 39 ++++++++++++++++++++++++++++++-----
>  1 file changed, 34 insertions(+), 5 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index df5e295656d7..0b699c957990 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -871,12 +871,41 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
>  				  &availableControlModes, 1);
>  
>  	/* JPEG static metadata. */
> -	std::vector<int32_t> availableThumbnailSizes = {
> -		0, 0,
> -	};
> +
> +	/*
> +	 * Create the list of supported tumbnail sizes by inspecting the

s/tumbnail/thumbnail/

> +	 * 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, 120) size as designated thumbnail size.

Shouldn't we give a square size here? 160,160?

That way a 9:16 ratio would be the same 'size' as a 16:9 for example.
(Thinking of phones/devices taking portrait rather than landscape pictures).

> +	 */
> +	constexpr Size maxJpegThumbnail(160, 120);
> +	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 });

I love how easy those helpers make maintaining aspect ratios.

> +		thumbnailSizes.push_back(thumbnailSize);
> +	}
> +
> +	std::sort(thumbnailSizes.begin(), thumbnailSizes.end());
> +	auto last = std::unique(thumbnailSizes.begin(), thumbnailSizes.end());
> +	thumbnailSizes.erase(last, thumbnailSizes.end());
> +
> +	/* Transform it in a list of integers that can be consumed. */

'in to a list' ?

> +	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());

The number of times we add a vector to addEntry() shouldn't we have an
addEntry(id, vector) so we don't duplicate .data(), .size() each time?

But that's a digression - not for this patch.

Otherwise sounds good to me.

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>



>  
>  	/*
>  	 * \todo Calculate the maximum JPEG buffer size by asking the encoder
> 

-- 
Regards
--
Kieran