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

[Umang Jain]

On 9/21/21 5:20 PM, Umang Jain wrote:
> 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.


errr, I mean s/ancient mistake/ancient commit/

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