[libcamera-devel,v1,2/2] android: Fix generation of thumbnail for EXIF data
diff mbox series

Message ID 20210910104729.542779-3-umang.jain@ideasonboard.com
State Superseded
Delegated to: Umang Jain
Headers show
Series
  • android: Fix generation of thumbnail for EXIF data
Related show

Commit Message

Umang Jain Sept. 10, 2021, 10:47 a.m. UTC 
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").

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@ideasonboard.com>
---
 src/android/camera_capabilities.cpp | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

Comments

Hirokazu Honda Sept. 10, 2021, 12:54 p.m. UTC | #1 
Hi Umang, thank you for the patch.

On Fri, Sep 10, 2021 at 7:47 PM Umang Jain <umang.jain@ideasonboard.com> 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").
>
> 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@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?

-Hiro
> +       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);
>
> --
> 2.31.0
>
Umang Jain Sept. 10, 2021, 2:31 p.m. UTC | #2 
Hi Hiro

On 9/10/21 6:24 PM, Hirokazu Honda wrote:
> Hi Umang, thank you for the patch.
>
> On Fri, Sep 10, 2021 at 7:47 PM Umang Jain <umang.jain@ideasonboard.com> 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").
>>
>> 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@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?

> -Hiro
>> +       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);
>>
>> --
>> 2.31.0
>>
Hirokazu Honda Sept. 10, 2021, 4:14 p.m. UTC | #3 
Hi Umang,

On Fri, Sep 10, 2021 at 11:31 PM Umang Jain <umang.jain@ideasonboard.com> wrote:
>
> Hi Hiro
>
> On 9/10/21 6:24 PM, Hirokazu Honda wrote:
> > Hi Umang, thank you for the patch.
> >
> > On Fri, Sep 10, 2021 at 7:47 PM Umang Jain <umang.jain@ideasonboard.com> 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").
> >>
> >> 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@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.

-Hiro
> > -Hiro
> >> +       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);
> >>
> >> --
> >> 2.31.0
> >>
Laurent Pinchart Sept. 10, 2021, 4:31 p.m. UTC | #4 
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@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@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);
> > >>

Patch
diff mbox series 

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;
+	}
+	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);