[libcamera-devel] [PATCH v6 1/1] Fix Android adaptor thumbnail buffer lifetime

[Umang Jain]

Hello Harvey,

Thank you for the patch.

On 7/20/22 15:28, Harvey Yang via libcamera-devel wrote:
> Previously the thumbnail buffer is destructed before even being used in


Great catch, I think this was the reason for flaky failure for 
Camera3SimpleStillCaptureTest.JpegExifTest as mentioned!

> Exif. This patch moves the buffer into class Exif, so that the developer
> won't need to worry about its lifetime.
>
> Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> ---
>   src/android/jpeg/exif.cpp                | 10 ++++++----
>   src/android/jpeg/exif.h                  | 10 ++++++++--
>   src/android/jpeg/post_processor_jpeg.cpp |  6 ++----
>   3 files changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
> index 3220b458..584a29d0 100644
> --- a/src/android/jpeg/exif.cpp
> +++ b/src/android/jpeg/exif.cpp
> @@ -435,11 +435,13 @@ void Exif::setOrientation(int orientation)
>    * Failing to do so, might result in no thumbnail data being set even after a
>    * call to Exif::setThumbnail().
>    */
> -void Exif::setThumbnail(Span<const unsigned char> thumbnail,
> -			Compression compression)
> +void Exif::setThumbnail(Compression compression)
>   {
> -	data_->data = const_cast<unsigned char *>(thumbnail.data());
> -	data_->size = thumbnail.size();
> +	if (thumbnail_raw_.empty())
> +		return;
> +
> +	data_->data = const_cast<unsigned char *>(thumbnail_raw_.data());


I think we can do away with the const_cast here? Might not be compatible 
with previous C++ versions?

I quickly compile tested

-       data_->data = const_cast<unsigned char *>(thumbnail_raw_.data());
+       data_->data = thumbnail_raw_.data();

on clang version 12.0.1 and gcc (GCC) 11.0.1 which seems OK.


> +	data_->size = thumbnail_raw_.size();
>   
>   	setShort(EXIF_IFD_0, EXIF_TAG_COMPRESSION, compression);
>   }
> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h
> index 2ff8fb78..4784496b 100644
> --- a/src/android/jpeg/exif.h
> +++ b/src/android/jpeg/exif.h
> @@ -10,6 +10,7 @@
>   #include <chrono>
>   #include <string>
>   #include <time.h>
> +#include <vector>
>   
>   #include <libexif/exif-data.h>
>   
> @@ -60,8 +61,7 @@ public:
>   
>   	void setOrientation(int orientation);
>   	void setSize(const libcamera::Size &size);
> -	void setThumbnail(libcamera::Span<const unsigned char> thumbnail,
> -			  Compression compression);
> +	void setThumbnail(Compression compression);
>   	void setTimestamp(time_t timestamp, std::chrono::milliseconds msec);
>   
>   	void setGPSDateTimestamp(time_t timestamp);
> @@ -78,6 +78,10 @@ public:
>   	libcamera::Span<const uint8_t> data() const { return { exifData_, size_ }; }
>   	[[nodiscard]] int generate();
>   
> +	std::vector<unsigned char>& getThumbnailRaw() {
> +		return thumbnail_raw_;
> +	}
> +
>   private:
>   	ExifEntry *createEntry(ExifIfd ifd, ExifTag tag);
>   	ExifEntry *createEntry(ExifIfd ifd, ExifTag tag, ExifFormat format,
> @@ -106,4 +110,6 @@ private:
>   
>   	unsigned char *exifData_;
>   	unsigned int size_;
> +
> +	std::vector<unsigned char> thumbnail_raw_;


CamelCase everywhere please ;-)

Once reviewed by someone else, I 'll handle the merge so,

Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>

>   };
> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> index d72ebc3c..506b00b9 100644
> --- a/src/android/jpeg/post_processor_jpeg.cpp
> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> @@ -163,10 +163,8 @@ void PostProcessorJpeg::process(Camera3RequestDescriptor::StreamBuffer *streamBu
>   		resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_QUALITY, quality);
>   
>   		if (thumbnailSize != Size(0, 0)) {
> -			std::vector<unsigned char> thumbnail;
> -			generateThumbnail(source, thumbnailSize, quality, &thumbnail);
> -			if (!thumbnail.empty())
> -				exif.setThumbnail(thumbnail, Exif::Compression::JPEG);
> +			generateThumbnail(source, thumbnailSize, quality, &exif.getThumbnailRaw());
> +			exif.setThumbnail(Exif::Compression::JPEG);
>   		}
>   
>   		resultMetadata->addEntry(ANDROID_JPEG_THUMBNAIL_SIZE, data, 2);