[libcamera-devel] wip: android: Fix generation of EXIF thumbnail
diff mbox series

Message ID 20210907060145.97676-1-umang.jain@ideasonboard.com
State Superseded
Delegated to: Umang Jain
Headers show
Series
  • [libcamera-devel] wip: android: Fix generation of EXIF thumbnail
Related show

Commit Message

Umang Jain Sept. 7, 2021, 6:01 a.m. UTC 
Generation of EXIF thumbnail seems to have regressed due to
1264628d3c92("android: jpeg: Configure thumbnailer based on request
metadata"). The patch tries to fix the issue of
ANDROID_JPEG_THUMBNAIL_SIZE not being available in request metadata
template.

However, there are a few open questions marked as comments in the
patch, which should be addressed before merging the fix.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 src/android/camera_capabilities.cpp      | 17 ++++++++++++++++-
 src/android/jpeg/post_processor_jpeg.cpp |  1 +
 2 files changed, 17 insertions(+), 1 deletion(-)

Comments

Hirokazu Honda Sept. 7, 2021, 9:19 a.m. UTC | #1 
Hi Umang, thank you for the patch.

On Tue, Sep 7, 2021 at 3:01 PM Umang Jain <umang.jain@ideasonboard.com> wrote:
>
> Generation of EXIF thumbnail seems to have regressed due to
> 1264628d3c92("android: jpeg: Configure thumbnailer based on request
> metadata"). The patch tries to fix the issue of
> ANDROID_JPEG_THUMBNAIL_SIZE not being available in request metadata
> template.
>
> However, there are a few open questions marked as comments in the
> patch, which should be addressed before merging the fix.
>
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  src/android/camera_capabilities.cpp      | 17 ++++++++++++++++-
>  src/android/jpeg/post_processor_jpeg.cpp |  1 +
>  2 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> index e92bca42..e7f91020 100644
> --- a/src/android/camera_capabilities.cpp
> +++ b/src/android/camera_capabilities.cpp
> @@ -951,7 +951,7 @@ int CameraCapabilities::initializeStaticMetadata()
>          */
>         constexpr Size maxJpegThumbnail(160, 160);
>         std::vector<Size> thumbnailSizes;
> -       thumbnailSizes.push_back({ 0, 0 });
> +       // thumbnailSizes.push_back({ 0, 0 }); not sure why this was added in first place?

Looking at the spec, (0, 0) is required.
If JPEG_THUMNAIL_SIZE is (0, 0), we should not generate a thumbnail.
(0, 0) indicates no thumbnail generation is supported.
https://developer.android.com/reference/android/hardware/camera2/CameraCharacteristics#JPEG_AVAILABLE_THUMBNAIL_SIZES

>         for (const auto &entry : streamConfigurations_) {
>                 if (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB)
>                         continue;
> @@ -1364,6 +1364,21 @@ 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 to template.
> +        *
> +        * \todo Make sure you update entry size and byte size above to
> +        *  incorporate this.
> +        */
> +       found = staticMetadata_->getEntry(ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES,
> +                                         &entry);
> +       if (!found) {
> +               LOG(HAL, Error) << "ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES not found";
> +               // return here?
> +       }
> +       requestTemplate->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE,
> +                                 entry.data.i32, 2);
> +

I think Size should be added here.
We should set just some non zero resolution just as a default.
Now I can understand why you would like to remove (0, 0) in the above.
I think we may want to add the second element of AVAILABLE_THUMBNAIL_SIZES?

-Hiro

>         uint8_t aeMode = ANDROID_CONTROL_AE_MODE_ON;
>         requestTemplate->addEntry(ANDROID_CONTROL_AE_MODE, aeMode);
>
> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> index 3160a784..2671a1ab 100644
> --- a/src/android/jpeg/post_processor_jpeg.cpp
> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> @@ -150,6 +150,7 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
>                         generateThumbnail(source, thumbnailSize, quality, &thumbnail);
>                         if (!thumbnail.empty())
>                                 exif.setThumbnail(thumbnail, Exif::Compression::JPEG);
> +                       LOG(JPEG, Info) << "====THUMBNAIL GENERATED AND SET====";
>                 }
>
>                 resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, data, 2);
> --
> 2.31.0
>
Umang Jain Sept. 7, 2021, 9:53 a.m. UTC | #2 
Hi Hiro,

On 9/7/21 2:49 PM, Hirokazu Honda wrote:
> Hi Umang, thank you for the patch.
>
> On Tue, Sep 7, 2021 at 3:01 PM Umang Jain <umang.jain@ideasonboard.com> wrote:
>> Generation of EXIF thumbnail seems to have regressed due to
>> 1264628d3c92("android: jpeg: Configure thumbnailer based on request
>> metadata"). The patch tries to fix the issue of
>> ANDROID_JPEG_THUMBNAIL_SIZE not being available in request metadata
>> template.
>>
>> However, there are a few open questions marked as comments in the
>> patch, which should be addressed before merging the fix.
>>
>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>> ---
>>   src/android/camera_capabilities.cpp      | 17 ++++++++++++++++-
>>   src/android/jpeg/post_processor_jpeg.cpp |  1 +
>>   2 files changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
>> index e92bca42..e7f91020 100644
>> --- a/src/android/camera_capabilities.cpp
>> +++ b/src/android/camera_capabilities.cpp
>> @@ -951,7 +951,7 @@ int CameraCapabilities::initializeStaticMetadata()
>>           */
>>          constexpr Size maxJpegThumbnail(160, 160);
>>          std::vector<Size> thumbnailSizes;
>> -       thumbnailSizes.push_back({ 0, 0 });
>> +       // thumbnailSizes.push_back({ 0, 0 }); not sure why this was added in first place?
> Looking at the spec, (0, 0) is required.
> If JPEG_THUMNAIL_SIZE is (0, 0), we should not generate a thumbnail.
> (0, 0) indicates no thumbnail generation is supported.
> https://developer.android.com/reference/android/hardware/camera2/CameraCharacteristics#JPEG_AVAILABLE_THUMBNAIL_SIZES


Yes, I think it's required after reading the doc. While reading the code 
it didn't made any sense as why one would require a thumbnail of 0x0.

>
>>          for (const auto &entry : streamConfigurations_) {
>>                  if (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB)
>>                          continue;
>> @@ -1364,6 +1364,21 @@ 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 to template.
>> +        *
>> +        * \todo Make sure you update entry size and byte size above to
>> +        *  incorporate this.
>> +        */
>> +       found = staticMetadata_->getEntry(ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES,
>> +                                         &entry);
>> +       if (!found) {
>> +               LOG(HAL, Error) << "ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES not found";
>> +               // return here?
>> +       }
>> +       requestTemplate->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE,
>> +                                 entry.data.i32, 2);
>> +
> I think Size should be added here.
> We should set just some non zero resolution just as a default.
> Now I can understand why you would like to remove (0, 0) in the above.
> I think we may want to add the second element of AVAILABLE_THUMBNAIL_SIZES?


I am not sure here as well. Maybe the second (or say, first non-zero 
size) or maybe the highest size reported in AVAILABLE_THUMBNAIL_SIZES; 
that should used as target dimensions for the thumbnail. All I care, it 
should be non-zero :)

>
> -Hiro
>
>>          uint8_t aeMode = ANDROID_CONTROL_AE_MODE_ON;
>>          requestTemplate->addEntry(ANDROID_CONTROL_AE_MODE, aeMode);
>>
>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
>> index 3160a784..2671a1ab 100644
>> --- a/src/android/jpeg/post_processor_jpeg.cpp
>> +++ b/src/android/jpeg/post_processor_jpeg.cpp
>> @@ -150,6 +150,7 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
>>                          generateThumbnail(source, thumbnailSize, quality, &thumbnail);
>>                          if (!thumbnail.empty())
>>                                  exif.setThumbnail(thumbnail, Exif::Compression::JPEG);
>> +                       LOG(JPEG, Info) << "====THUMBNAIL GENERATED AND SET====";
>>                  }
>>
>>                  resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, data, 2);
>> --
>> 2.31.0
>>
Hirokazu Honda Sept. 8, 2021, 7:43 a.m. UTC | #3 
Hi Umang,

On Tue, Sep 7, 2021 at 6:53 PM Umang Jain <umang.jain@ideasonboard.com> wrote:
>
> Hi Hiro,
>
> On 9/7/21 2:49 PM, Hirokazu Honda wrote:
> > Hi Umang, thank you for the patch.
> >
> > On Tue, Sep 7, 2021 at 3:01 PM Umang Jain <umang.jain@ideasonboard.com> wrote:
> >> Generation of EXIF thumbnail seems to have regressed due to
> >> 1264628d3c92("android: jpeg: Configure thumbnailer based on request
> >> metadata"). The patch tries to fix the issue of
> >> ANDROID_JPEG_THUMBNAIL_SIZE not being available in request metadata
> >> template.
> >>
> >> However, there are a few open questions marked as comments in the
> >> patch, which should be addressed before merging the fix.
> >>
> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> >> ---
> >>   src/android/camera_capabilities.cpp      | 17 ++++++++++++++++-
> >>   src/android/jpeg/post_processor_jpeg.cpp |  1 +
> >>   2 files changed, 17 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> >> index e92bca42..e7f91020 100644
> >> --- a/src/android/camera_capabilities.cpp
> >> +++ b/src/android/camera_capabilities.cpp
> >> @@ -951,7 +951,7 @@ int CameraCapabilities::initializeStaticMetadata()
> >>           */
> >>          constexpr Size maxJpegThumbnail(160, 160);
> >>          std::vector<Size> thumbnailSizes;
> >> -       thumbnailSizes.push_back({ 0, 0 });
> >> +       // thumbnailSizes.push_back({ 0, 0 }); not sure why this was added in first place?
> > Looking at the spec, (0, 0) is required.
> > If JPEG_THUMNAIL_SIZE is (0, 0), we should not generate a thumbnail.
> > (0, 0) indicates no thumbnail generation is supported.
> > https://developer.android.com/reference/android/hardware/camera2/CameraCharacteristics#JPEG_AVAILABLE_THUMBNAIL_SIZES
>
>
> Yes, I think it's required after reading the doc. While reading the code
> it didn't made any sense as why one would require a thumbnail of 0x0.
>
> >
> >>          for (const auto &entry : streamConfigurations_) {
> >>                  if (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB)
> >>                          continue;
> >> @@ -1364,6 +1364,21 @@ 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 to template.
> >> +        *
> >> +        * \todo Make sure you update entry size and byte size above to
> >> +        *  incorporate this.
> >> +        */
> >> +       found = staticMetadata_->getEntry(ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES,
> >> +                                         &entry);
> >> +       if (!found) {
> >> +               LOG(HAL, Error) << "ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES not found";
> >> +               // return here?
> >> +       }
> >> +       requestTemplate->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE,
> >> +                                 entry.data.i32, 2);
> >> +
> > I think Size should be added here.
> > We should set just some non zero resolution just as a default.
> > Now I can understand why you would like to remove (0, 0) in the above.
> > I think we may want to add the second element of AVAILABLE_THUMBNAIL_SIZES?
>
>
> I am not sure here as well. Maybe the second (or say, first non-zero
> size) or maybe the highest size reported in AVAILABLE_THUMBNAIL_SIZES;
> that should used as target dimensions for the thumbnail. All I care, it
> should be non-zero :)

I wonder now if it should be zero. If there is a thumbnail, the
correct thumbnail size should be overwritten later?

-Hiro
>
> >
> > -Hiro
> >
> >>          uint8_t aeMode = ANDROID_CONTROL_AE_MODE_ON;
> >>          requestTemplate->addEntry(ANDROID_CONTROL_AE_MODE, aeMode);
> >>
> >> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> >> index 3160a784..2671a1ab 100644
> >> --- a/src/android/jpeg/post_processor_jpeg.cpp
> >> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> >> @@ -150,6 +150,7 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
> >>                          generateThumbnail(source, thumbnailSize, quality, &thumbnail);
> >>                          if (!thumbnail.empty())
> >>                                  exif.setThumbnail(thumbnail, Exif::Compression::JPEG);
> >> +                       LOG(JPEG, Info) << "====THUMBNAIL GENERATED AND SET====";
> >>                  }
> >>
> >>                  resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, data, 2);
> >> --
> >> 2.31.0
> >>

Patch
diff mbox series 

diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
index e92bca42..e7f91020 100644
--- a/src/android/camera_capabilities.cpp
+++ b/src/android/camera_capabilities.cpp
@@ -951,7 +951,7 @@  int CameraCapabilities::initializeStaticMetadata()
 	 */
 	constexpr Size maxJpegThumbnail(160, 160);
 	std::vector<Size> thumbnailSizes;
-	thumbnailSizes.push_back({ 0, 0 });
+	// thumbnailSizes.push_back({ 0, 0 }); not sure why this was added in first place?
 	for (const auto &entry : streamConfigurations_) {
 		if (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB)
 			continue;
@@ -1364,6 +1364,21 @@  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 to template.
+	 *
+	 * \todo Make sure you update entry size and byte size above to
+	 *  incorporate this.
+	 */
+	found = staticMetadata_->getEntry(ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES,
+					  &entry);
+	if (!found) {
+		LOG(HAL, Error) << "ANDROID_JPEG_AVAILABLE_THUMBNAIL_SIZES not found";
+		// return here?
+	}
+	requestTemplate->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE,
+				  entry.data.i32, 2);
+
 	uint8_t aeMode = ANDROID_CONTROL_AE_MODE_ON;
 	requestTemplate->addEntry(ANDROID_CONTROL_AE_MODE, aeMode);
 
diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
index 3160a784..2671a1ab 100644
--- a/src/android/jpeg/post_processor_jpeg.cpp
+++ b/src/android/jpeg/post_processor_jpeg.cpp
@@ -150,6 +150,7 @@  int PostProcessorJpeg::process(const FrameBuffer &source,
 			generateThumbnail(source, thumbnailSize, quality, &thumbnail);
 			if (!thumbnail.empty())
 				exif.setThumbnail(thumbnail, Exif::Compression::JPEG);
+			LOG(JPEG, Info) << "====THUMBNAIL GENERATED AND SET====";
 		}
 
 		resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, data, 2);