[libcamera-devel,v2,3/3] android: jpeg: post_processor_jpeg: Embed thumbnail into Exif metadata

10272 diff mbox series

Embed a Jpeg-encoded thumbnail into Exif metadata using the Thumbnailer
class that got introduced.

Introduce a helper function in Exif class for setting the thumbnail
data. Set the EXIF_TAG_COMPRESSION to '6' to denote that the thumbnail
is jpeg-compressed, as mentioned in Exif v2.31.

Signed-off-by: Umang Jain <email@uajain.com>
---
 src/android/jpeg/exif.cpp                | 24 ++++++++++++++++-
 src/android/jpeg/exif.h                  |  2 ++
 src/android/jpeg/post_processor_jpeg.cpp | 34 ++++++++++++++++++++++++
 src/android/jpeg/post_processor_jpeg.h   |  8 +++++-
 4 files changed, 66 insertions(+), 2 deletions(-)

Hi Umang,

Thank you for the patch.

On Wed, Oct 28, 2020 at 02:54:47AM +0530, Umang Jain wrote:
> Embed a Jpeg-encoded thumbnail into Exif metadata using the Thumbnailer
> class that got introduced.
> 
> Introduce a helper function in Exif class for setting the thumbnail
> data. Set the EXIF_TAG_COMPRESSION to '6' to denote that the thumbnail
> is jpeg-compressed, as mentioned in Exif v2.31.
> 
> Signed-off-by: Umang Jain <email@uajain.com>
> ---
>  src/android/jpeg/exif.cpp                | 24 ++++++++++++++++-
>  src/android/jpeg/exif.h                  |  2 ++
>  src/android/jpeg/post_processor_jpeg.cpp | 34 ++++++++++++++++++++++++
>  src/android/jpeg/post_processor_jpeg.h   |  8 +++++-
>  4 files changed, 66 insertions(+), 2 deletions(-)
> 
> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
> index d21534a..6ac52c6 100644
> --- a/src/android/jpeg/exif.cpp
> +++ b/src/android/jpeg/exif.cpp
> @@ -75,8 +75,16 @@ Exif::~Exif()
>  	if (exifData_)
>  		free(exifData_);
>  
> -	if (data_)
> +	if (data_) {
> +		/*
> +		 * Reset thumbnail data to avoid getting double-freed by
> +		 * libexif. It is owned by the caller (i.e. PostProcessorJpeg).
> +		 */
> +		data_->data = nullptr;
> +		data_->size = 0;
> +
>  		exif_data_unref(data_);
> +	}
>  
>  	if (mem_)
>  		exif_mem_unref(mem_);
> @@ -268,6 +276,20 @@ void Exif::setOrientation(int orientation)
>  	setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value);
>  }
>  
> +/*
> + * The thumbnail data should remain valid until the Exif object is destroyed.
> + * 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,
> +			uint16_t compression)
> +{
> +	data_->data = const_cast<unsigned char *>(thumbnail.data());
> +	data_->size = thumbnail.size();
> +
> +	setShort(EXIF_IFD_0, EXIF_TAG_COMPRESSION, compression);
> +}
> +
>  [[nodiscard]] int Exif::generate()
>  {
>  	if (exifData_) {
> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h
> index 12c27b6..6987b31 100644
> --- a/src/android/jpeg/exif.h
> +++ b/src/android/jpeg/exif.h
> @@ -26,6 +26,8 @@ public:
>  
>  	void setOrientation(int orientation);
>  	void setSize(const libcamera::Size &size);
> +	void setThumbnail(libcamera::Span<const unsigned char> thumbnail,
> +			  uint16_t compression);
>  	void setTimestamp(time_t timestamp);
>  
>  	libcamera::Span<const uint8_t> data() const { return { exifData_, size_ }; }
> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> index c56f1b2..a0db793 100644
> --- a/src/android/jpeg/post_processor_jpeg.cpp
> +++ b/src/android/jpeg/post_processor_jpeg.cpp
> @@ -39,11 +39,41 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,
>  	}
>  
>  	streamSize_ = outCfg.size;
> +
> +	thumbnailer_.configure(inCfg.size, inCfg.pixelFormat);
> +	StreamConfiguration thCfg = inCfg;
> +	thCfg.size = thumbnailer_.size();
> +	if (thumbnailEncoder_.configure(thCfg) != 0) {
> +		LOG(JPEG, Error) << "Failed to configure thumbnail encoder";
> +		return -EINVAL;
> +	}
> +
>  	encoder_ = std::make_unique<EncoderLibJpeg>();
>  
>  	return encoder_->configure(inCfg);
>  }
>  
> +void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
> +					  std::vector<unsigned char> *thumbnail)
> +{
> +	/* Stores the raw scaled-down thumbnail bytes. */
> +	std::vector<unsigned char> rawThumbnail;
> +
> +	thumbnailer_.createThumbnail(source, &rawThumbnail);
> +
> +	if (!rawThumbnail.empty()) {
> +		thumbnail->resize(rawThumbnail.size());

This will zero the contents of the vector, which isn't required. Let's
leave it as-is for now, but record the issue:

		/*
		 * \todo Avoid value-initialization of all elements of the
		 * vector.
		 */

> +
> +		int jpeg_size = thumbnailEncoder_.encode(rawThumbnail,
> +							 *thumbnail, { });
> +		thumbnail->resize(jpeg_size);
> +
> +		LOG(JPEG, Debug)
> +			<< "Thumbnail compress returned "
> +			<< jpeg_size << " bytes";
> +	}
> +}
> +
>  int PostProcessorJpeg::process(const FrameBuffer &source,
>  			       Span<uint8_t> destination,
>  			       CameraMetadata *metadata)
> @@ -64,6 +94,10 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
>  	 * second, it is good enough.
>  	 */
>  	exif.setTimestamp(std::time(nullptr));
> +	std::vector<unsigned char> thumbnail;
> +	generateThumbnail(source, &thumbnail);

I think we could return the vector from generateThumbnail() instead of
passing it from the caller, but that's also not something that needs to
be addressed now.

> +	if(!thumbnail.empty())
> +		exif.setThumbnail(thumbnail, 6);

The magic value isn't very nice. How about turning the parameter to a
'bool compressed', or, better, to an enum declared in the Exif class

	enum class Compression {
		None = 1,
		JPEG = 6,
	};

and turning setThumbnail() into

	void setThumbnail(libcamera::Span<const unsigned char> thumbnail,
			  Compression compression);

Apart from that the patch looks good to me.

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

I'll let Kieran have a look too, but I think these small issues could be
fixed while applying (unless he would prefer you to test them first).

>  	if (exif.generate() != 0)
>  		LOG(JPEG, Error) << "Failed to generate valid EXIF data";
>  
> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
> index 3706cec..5afa831 100644
> --- a/src/android/jpeg/post_processor_jpeg.h
> +++ b/src/android/jpeg/post_processor_jpeg.h
> @@ -8,12 +8,13 @@
>  #define __ANDROID_POST_PROCESSOR_JPEG_H__
>  
>  #include "../post_processor.h"
> +#include "encoder_libjpeg.h"
> +#include "thumbnailer.h"
>  
>  #include <libcamera/geometry.h>
>  
>  #include "libcamera/internal/buffer.h"
>  
> -class Encoder;
>  class CameraDevice;
>  
>  class PostProcessorJpeg : public PostProcessor
> @@ -28,9 +29,14 @@ public:
>  		    CameraMetadata *metadata) override;
>  
>  private:
> +	void generateThumbnail(const libcamera::FrameBuffer &source,
> +			       std::vector<unsigned char> *thumbnail);
> +
>  	CameraDevice *const cameraDevice_;
>  	std::unique_ptr<Encoder> encoder_;
>  	libcamera::Size streamSize_;
> +	EncoderLibJpeg thumbnailEncoder_;
> +	Thumbnailer thumbnailer_;
>  };
>  
>  #endif /* __ANDROID_POST_PROCESSOR_JPEG_H__ */

Hi Laurent,

Thanks for the review.

On 10/28/20 6:59 AM, Laurent Pinchart wrote:
> Hi Umang,
>
> Thank you for the patch.
>
> On Wed, Oct 28, 2020 at 02:54:47AM +0530, Umang Jain wrote:
>> Embed a Jpeg-encoded thumbnail into Exif metadata using the Thumbnailer
>> class that got introduced.
>>
>> Introduce a helper function in Exif class for setting the thumbnail
>> data. Set the EXIF_TAG_COMPRESSION to '6' to denote that the thumbnail
>> is jpeg-compressed, as mentioned in Exif v2.31.
>>
>> Signed-off-by: Umang Jain <email@uajain.com>
>> ---
>>   src/android/jpeg/exif.cpp                | 24 ++++++++++++++++-
>>   src/android/jpeg/exif.h                  |  2 ++
>>   src/android/jpeg/post_processor_jpeg.cpp | 34 ++++++++++++++++++++++++
>>   src/android/jpeg/post_processor_jpeg.h   |  8 +++++-
>>   4 files changed, 66 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
>> index d21534a..6ac52c6 100644
>> --- a/src/android/jpeg/exif.cpp
>> +++ b/src/android/jpeg/exif.cpp
>> @@ -75,8 +75,16 @@ Exif::~Exif()
>>   	if (exifData_)
>>   		free(exifData_);
>>   
>> -	if (data_)
>> +	if (data_) {
>> +		/*
>> +		 * Reset thumbnail data to avoid getting double-freed by
>> +		 * libexif. It is owned by the caller (i.e. PostProcessorJpeg).
>> +		 */
>> +		data_->data = nullptr;
>> +		data_->size = 0;
>> +
>>   		exif_data_unref(data_);
>> +	}
>>   
>>   	if (mem_)
>>   		exif_mem_unref(mem_);
>> @@ -268,6 +276,20 @@ void Exif::setOrientation(int orientation)
>>   	setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value);
>>   }
>>   
>> +/*
>> + * The thumbnail data should remain valid until the Exif object is destroyed.
>> + * 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,
>> +			uint16_t compression)
>> +{
>> +	data_->data = const_cast<unsigned char *>(thumbnail.data());
>> +	data_->size = thumbnail.size();
>> +
>> +	setShort(EXIF_IFD_0, EXIF_TAG_COMPRESSION, compression);
>> +}
>> +
>>   [[nodiscard]] int Exif::generate()
>>   {
>>   	if (exifData_) {
>> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h
>> index 12c27b6..6987b31 100644
>> --- a/src/android/jpeg/exif.h
>> +++ b/src/android/jpeg/exif.h
>> @@ -26,6 +26,8 @@ public:
>>   
>>   	void setOrientation(int orientation);
>>   	void setSize(const libcamera::Size &size);
>> +	void setThumbnail(libcamera::Span<const unsigned char> thumbnail,
>> +			  uint16_t compression);
>>   	void setTimestamp(time_t timestamp);
>>   
>>   	libcamera::Span<const uint8_t> data() const { return { exifData_, size_ }; }
>> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
>> index c56f1b2..a0db793 100644
>> --- a/src/android/jpeg/post_processor_jpeg.cpp
>> +++ b/src/android/jpeg/post_processor_jpeg.cpp
>> @@ -39,11 +39,41 @@ int PostProcessorJpeg::configure(const StreamConfiguration &inCfg,
>>   	}
>>   
>>   	streamSize_ = outCfg.size;
>> +
>> +	thumbnailer_.configure(inCfg.size, inCfg.pixelFormat);
>> +	StreamConfiguration thCfg = inCfg;
>> +	thCfg.size = thumbnailer_.size();
>> +	if (thumbnailEncoder_.configure(thCfg) != 0) {
>> +		LOG(JPEG, Error) << "Failed to configure thumbnail encoder";
>> +		return -EINVAL;
>> +	}
>> +
>>   	encoder_ = std::make_unique<EncoderLibJpeg>();
>>   
>>   	return encoder_->configure(inCfg);
>>   }
>>   
>> +void PostProcessorJpeg::generateThumbnail(const FrameBuffer &source,
>> +					  std::vector<unsigned char> *thumbnail)
>> +{
>> +	/* Stores the raw scaled-down thumbnail bytes. */
>> +	std::vector<unsigned char> rawThumbnail;
>> +
>> +	thumbnailer_.createThumbnail(source, &rawThumbnail);
>> +
>> +	if (!rawThumbnail.empty()) {
>> +		thumbnail->resize(rawThumbnail.size());
> This will zero the contents of the vector, which isn't required. Let's
> leave it as-is for now, but record the issue:
>
> 		/*
> 		 * \todo Avoid value-initialization of all elements of the
> 		 * vector.
> 		 */
>
>> +
>> +		int jpeg_size = thumbnailEncoder_.encode(rawThumbnail,
>> +							 *thumbnail, { });
>> +		thumbnail->resize(jpeg_size);
>> +
>> +		LOG(JPEG, Debug)
>> +			<< "Thumbnail compress returned "
>> +			<< jpeg_size << " bytes";
>> +	}
>> +}
>> +
>>   int PostProcessorJpeg::process(const FrameBuffer &source,
>>   			       Span<uint8_t> destination,
>>   			       CameraMetadata *metadata)
>> @@ -64,6 +94,10 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
>>   	 * second, it is good enough.
>>   	 */
>>   	exif.setTimestamp(std::time(nullptr));
>> +	std::vector<unsigned char> thumbnail;
>> +	generateThumbnail(source, &thumbnail);
> I think we could return the vector from generateThumbnail() instead of
> passing it from the caller, but that's also not something that needs to
> be addressed now.
>
>> +	if(!thumbnail.empty())
>> +		exif.setThumbnail(thumbnail, 6);
> The magic value isn't very nice. How about turning the parameter to a
> 'bool compressed', or, better, to an enum declared in the Exif class
>
> 	enum class Compression {
> 		None = 1,
> 		JPEG = 6,
> 	};
>
> and turning setThumbnail() into
>
> 	void setThumbnail(libcamera::Span<const unsigned char> thumbnail,
> 			  Compression compression);
>
> Apart from that the patch looks good to me.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> I'll let Kieran have a look too, but I think these small issues could be
> fixed while applying (unless he would prefer you to test them first).
I have applied the changes locally, however I will wait a bit if more 
reviews comments flow in, before send another/final version. I can test 
the patches too with the cam-file-sink branch, however upto Kieran if he 
wants to give quick go at the actual CrOS before pushing.
>
>>   	if (exif.generate() != 0)
>>   		LOG(JPEG, Error) << "Failed to generate valid EXIF data";
>>   
>> diff --git a/src/android/jpeg/post_processor_jpeg.h b/src/android/jpeg/post_processor_jpeg.h
>> index 3706cec..5afa831 100644
>> --- a/src/android/jpeg/post_processor_jpeg.h
>> +++ b/src/android/jpeg/post_processor_jpeg.h
>> @@ -8,12 +8,13 @@
>>   #define __ANDROID_POST_PROCESSOR_JPEG_H__
>>   
>>   #include "../post_processor.h"
>> +#include "encoder_libjpeg.h"
>> +#include "thumbnailer.h"
>>   
>>   #include <libcamera/geometry.h>
>>   
>>   #include "libcamera/internal/buffer.h"
>>   
>> -class Encoder;
>>   class CameraDevice;
>>   
>>   class PostProcessorJpeg : public PostProcessor
>> @@ -28,9 +29,14 @@ public:
>>   		    CameraMetadata *metadata) override;
>>   
>>   private:
>> +	void generateThumbnail(const libcamera::FrameBuffer &source,
>> +			       std::vector<unsigned char> *thumbnail);
>> +
>>   	CameraDevice *const cameraDevice_;
>>   	std::unique_ptr<Encoder> encoder_;
>>   	libcamera::Size streamSize_;
>> +	EncoderLibJpeg thumbnailEncoder_;
>> +	Thumbnailer thumbnailer_;
>>   };
>>   
>>   #endif /* __ANDROID_POST_PROCESSOR_JPEG_H__ */