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

[Umang Jain]

Hi Jacopo

On 9/13/21 7:38 PM, Jacopo Mondi wrote:
> 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 ?


This is correct, but the counterpart of the patch seems missing from 
that commit, which actually resulted in regression

>
>> 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.


I wonder that too...

>
>> 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


Looking at both #entries and #bytes, that gives me an impression that 
extra entry and data buffer bytes are **intentional**, so I went ahead 
with it! Are both of them a typo? I'll need some checking tomorrow.

>
>>   	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.


Not only we populate it, it's a requirement that Size (0,0) should be in 
the vector /and/ the vector needs to be sorted in ascending order [1] if 
more sizes are provided with. We do the right thing when populating 
ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES so I won't be extra paranoid here.


[1]: 
https://developer.android.com/reference/android/hardware/camera2/CameraCharacteristics#JPEG_AVAILABLE_THUMBNAIL_SIZES

>
> 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) ?


As said above, it's a requirement for 
ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES, so should I make it extra clear 
again here?

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