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

[Laurent Pinchart]

Hello,

On Sat, Sep 11, 2021 at 01:14:12AM +0900, Hirokazu Honda wrote:
> On Fri, Sep 10, 2021 at 11:31 PM Umang Jain wrote:
> > On 9/10/21 6:24 PM, Hirokazu Honda wrote:
> > > On Fri, Sep 10, 2021 at 7:47 PM 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

s/2(/2 (/

> > >> metadata").
> > >>
> > >> 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.
> > >>
> > >> Fixes: 1264628d3c92("android: jpeg: Configure thumbnailer based on request metadata")
> > >> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> > >> ---
> > >>   src/android/camera_capabilities.cpp | 17 +++++++++++++++--
> > >>   1 file changed, 15 insertions(+), 2 deletions(-)
> > >>
> > >> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> > >> index e92bca42..ba551b86 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);
> > >>          if (!requestTemplate->isValid()) {
> > >>                  return nullptr;
> > >>          }
> > >> @@ -1364,6 +1364,19 @@ 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);
> > >> +       if (!found) {
> > >> +               LOG(HAL, Error) << "availableThumbnailSizes not found in static metadata";
> > >> +               return nullptr;
> > >> +       }
> > >
> > > Is it possible to check if there is the second element?
> >
> > Second element ? Do you mean second (or other sizes) being reported in
> > ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES?
> >
> > It should be plausible to extract that from the entry's data I think.
> > Is something unclear?
> 
> I meant if SIZES returns empty size (i.e. the sizes are one size
> list), accessing entry.data.i32+2 is invalid.
> I wonder if we should check the size of the returned size list.

It's not supposed to happen, but if we want to be defensive, we can add

	ASSERT(entry.count >= 4);

Actually, as ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES is supposed to
always be there, I'd drop the error message and write

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

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> > >> +       requestTemplate->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE,
> > >> +                                 entry.data.i32 + 2, 2);
> > >> +
> > >>          uint8_t aeMode = ANDROID_CONTROL_AE_MODE_ON;
> > >>          requestTemplate->addEntry(ANDROID_CONTROL_AE_MODE, aeMode);
> > >>

-- 
Regards,

Laurent Pinchart