[libcamera-devel] [PATCH v2 2/2] android: Fix generation of thumbnail for EXIF data

[Jacopo Mondi]

Hi Umang,

On Mon, Sep 13, 2021 at 09:31:10AM +0530, Umang Jain wrote:
> Generation of thumbnail is not occuring currently because
> ANDROID_JPEG_THUMBNAIL_SIZE is not set for request metadata passed
> to PostProcessorJpeg::process(). This is a regression introduced in
> 1264628d3c92("android: jpeg: Configure thumbnailer based on request
> metadata").

I see the issue, but I'm not sure those commit was wrong. I mean, what
that commit does that itroduced a regression is:

--- a/src/android/jpeg/post_processor_jpeg.cpp
+++ b/src/android/jpeg/post_processor_jpeg.cpp
+       ret = requestMetadata.getEntry(ANDROID_JPEG_THUMBNAIL_SIZE, &entry);
+       if (ret) {
+               const int32_t *data = entry.data.i32;
+               Size thumbnailSize = { static_cast<uint32_t>(data[0]),
+                                      static_cast<uint32_t>(data[1]) };
+
+               if (thumbnailSize != Size(0, 0)) {
+                       std::vector<unsigned char> thumbnail;
+                       generateThumbnail(source, thumbnailSize, &thumbnail);
+                       if (!thumbnail.empty())
+                               exif.setThumbnail(thumbnail, Exif::Compression::JPEG);
+               }
+
+               resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, data, 2);
+
+               /* \todo Use this quality as a parameter to the encoder */
+               ret = requestMetadata.getEntry(ANDROID_JPEG_THUMBNAIL_QUALITY, &entry);
+               if (ret)
+                       resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_QUALITY,
+                                                entry.data.u8, 1);
+       }


Which I read as: "If ANDROID_JPEG_THUMBNAIL_SIZE is not specified in
the request's settings, do not populate it in result metadata".

Is this correct in your opinion, or should we populate it regardless
of the fact the tag was passed in ?

>
> The patch fixes this issue by setting ANDROID_JPEG_THUMBNAIL_SIZE in
> the request metadata template populated by
> CameraCapabilities::requestTemplatePreview(). The value for
> ANDROID_JPEG_THUMBNAIL_SIZE is set to be the first non-zero size
> reported by static metadata ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES.

Let's say just that this patch adds ANDROID_JPEG_THUMBNAIL_SIZE to the
capture request template generated for the preview use case. I wonder
if the JPEG thunbnail size should be part of the preview template now.

>
> Fixes: 1264628d3c92("android: jpeg: Configure thumbnailer based on request metadata")
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  src/android/camera_capabilities.cpp | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> index e92bca42..76dddafd 100644
> --- a/src/android/camera_capabilities.cpp
> +++ b/src/android/camera_capabilities.cpp
> @@ -1341,9 +1341,9 @@ std::unique_ptr<CameraMetadata> CameraCapabilities::requestTemplatePreview() con
>  {
>  	/*
>  	 * \todo Keep this in sync with the actual number of entries.
> -	 * Currently: 20 entries, 35 bytes
> +	 * Currently: 21 entries, 37 bytes
>  	 */
> -	auto requestTemplate = std::make_unique<CameraMetadata>(21, 36);
> +	auto requestTemplate = std::make_unique<CameraMetadata>(22, 38);

Comment says 21, code says 22.

It was there already as the comment was not aligned with the code, but
since you're at it you could fix it

>  	if (!requestTemplate->isValid()) {
>  		return nullptr;
>  	}
> @@ -1364,6 +1364,16 @@ std::unique_ptr<CameraMetadata> CameraCapabilities::requestTemplatePreview() con
>  	requestTemplate->addEntry(ANDROID_CONTROL_AE_TARGET_FPS_RANGE,
>  				  entry.data.i32, 2);
>
> +	/*
> +	 * Get thumbnail sizes from static metadata and add the first non-zero
> +	 * size to the template.
> +	 */
> +	found = staticMetadata_->getEntry(ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES,
> +					  &entry);
> +	ASSERT(found && entry.count >= 4);
> +	requestTemplate->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE,
> +				  entry.data.i32 + 2, 2);
> +

If I were to be extra paranoid I would make sure we actually discard
(0, 0). The code assumes the first two entries are (0, 0) which is
fine as we populate it after all.

If you don't want to

        unsigned int j = 0;
        while (j < entry.count / 2) {
                if (entry.data.i32[j] == 0 || entry.data.i32[j + 1] == 0)
                        continue;

                requestTemplate->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE,
                                          entry.data.i32 + j, 2);
        }

Could you at least capture that we assume the first two entries are
(0,0) ?

Thanks
   j


>  	uint8_t aeMode = ANDROID_CONTROL_AE_MODE_ON;
>  	requestTemplate->addEntry(ANDROID_CONTROL_AE_MODE, aeMode);
>
> --
> 2.31.1
>