[libcamera-devel] [PATCH] android: jpeg: exif: Set timezone information

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Thu, Sep 24, 2020 at 02:36:31PM +0530, Umang Jain wrote:
> The EXIF specification defines three timezone related tags namely,
> OffsetTime, OffsetTimeOriginal and OffsetTimeDigitized. However,
> these are not supported by libexif (as of v0.6.21) hence, carry
> the tags' positional values in our implementation until we get
> this support from libexif itself.
> 
> Since these tags were introduced in EXIF specification v2.31, set
> the exif version number explicitly too.
> 
> Signed-off-by: Umang Jain <email at uajain.com>
> ---
>  src/android/jpeg/exif.cpp | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
> index c0dbfcc..df38765 100644
> --- a/src/android/jpeg/exif.cpp
> +++ b/src/android/jpeg/exif.cpp
> @@ -13,6 +13,16 @@ using namespace libcamera;
>  
>  LOG_DEFINE_CATEGORY(EXIF)
>  
> +/*
> + * List of EXIF tags that we set directly because they are not supported
> + * by libexif version 0.6.21.
> + */
> +enum _ExifTag {

I'd make this "enum class" as you always use the _ExifTag:: prefix
below.

> +	OFFSET_TIME              = 0x9010,
> +	OFFSET_TIME_ORIGINAL     = 0x9011,
> +	OFFSET_TIME_DIGITIZED    = 0x9012,
> +};
> +
>  /*
>   * The Exif class should be instantiated and specific properties set
>   * through the exposed public API.
> @@ -51,6 +61,9 @@ Exif::Exif()
>  	 */
>  	exif_data_set_byte_order(data_, EXIF_BYTE_ORDER_INTEL);
>  
> +	setString(EXIF_IFD_EXIF, EXIF_TAG_EXIF_VERSION,
> +		  EXIF_FORMAT_UNDEFINED, "0231");
> +

According to the EXIF standard, "Since the type is UNDEFINED, it shall
not be terminated with NULL". How about modifying setString() to only
include the NULL terminator when the type is ASCII ? And maybe splitting
this to a separate patch ? Up to you.

The rest looks good to me.

>  	/* Create the mandatory EXIF fields with default data. */
>  	exif_data_fix(data_);
>  }
> @@ -196,6 +209,22 @@ void Exif::setTimestamp(time_t timestamp)
>  	setString(EXIF_IFD_0, EXIF_TAG_DATE_TIME, EXIF_FORMAT_ASCII, ts);
>  	setString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_ORIGINAL, EXIF_FORMAT_ASCII, ts);
>  	setString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_DIGITIZED, EXIF_FORMAT_ASCII, ts);
> +
> +	/* Query and set timezone information if available. */
> +	int r = strftime(str, sizeof(str), "%z", &tm);
> +	if (r > 0) {
> +		std::string tz(str);
> +		tz.insert(3, 1, ':');
> +		setString(EXIF_IFD_EXIF,
> +			  static_cast<ExifTag>(_ExifTag::OFFSET_TIME),
> +			  EXIF_FORMAT_ASCII, tz);
> +		setString(EXIF_IFD_EXIF,
> +			  static_cast<ExifTag>(_ExifTag::OFFSET_TIME_ORIGINAL),
> +			  EXIF_FORMAT_ASCII, tz);
> +		setString(EXIF_IFD_EXIF,
> +			  static_cast<ExifTag>(_ExifTag::OFFSET_TIME_DIGITIZED),
> +			  EXIF_FORMAT_ASCII, tz);
> +	}
>  }
>  
>  void Exif::setOrientation(int orientation)

-- 
Regards,

Laurent Pinchart