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

[Umang Jain]

Hi Jacopo

On 9/13/21 8:11 PM, Umang Jain wrote:
> 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.


Indeed, my assumption was a mistake. I think the mismatch was long 
sitting under the hood, and never really sync-ed with all the rework 
around CameraMetadata and request templates however,

I found this ancient mistake where it was supposed to match, but 
unfortunately didn't, and the development/increment (as new entries were 
added) kept happening on top of it.

https://git.linuxtv.org/libcamera.git/commit/?id=637034742f2b0b7524

I'll mention this as a point of divergence in v3 for this particular patch.

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


I am re-thinking this. It makes sense. It's one step more defensive than the

     ASSERT(found && entry.count >= 4);

It's atleast for the better.

I'll send a v3 for this patch (the 1/2 has already been merged) and also 
test out the thumbnail generation with the v3 and we can put an end to 
this thumbnail saga...

Thanks for your thoughts!


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