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

[Hirokazu Honda]

Hi Umang,

On Tue, Sep 21, 2021 at 8:52 PM Umang Jain <umang.jain at ideasonboard.com> wrote:
>
>
> 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>

Reviewed-by: Hirokazu Honda <hiroh at chromium.org>

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