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

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Wed, Sep 09, 2020 at 09:02:06PM +0530, Umang Jain wrote:
> Get timestamp information from the timestamp, although the resolution

s/timestamp information/timezone information/

> for EXIF_TAG_TIME_ZONE_OFFSET is fairly limtied (per-hour only).

s/limtied/limited/

> 
> Experimentation with 'exiftool', commandline utility to read/write
> exif metadata on images, resulted in rounding off the hours if the
> minutes came out to >= 30. Hence, the behaviour is inspired from
> exiftool itself. For e.g.,
> 
> Timezone      Tag's value
>  +1015     =>     10
>  +0945     =>     10
>  -1145     =>    -12

Sounds good to me.

> Signed-off-by: Umang Jain <email at uajain.com>
> ---
>  src/android/jpeg/exif.cpp | 30 ++++++++++++++++++++++++++++++
>  src/android/jpeg/exif.h   |  1 +
>  2 files changed, 31 insertions(+)
> 
> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
> index 031f5f4..d8b4537 100644
> --- a/src/android/jpeg/exif.cpp
> +++ b/src/android/jpeg/exif.cpp
> @@ -135,6 +135,16 @@ void Exif::setShort(ExifIfd ifd, ExifTag tag, uint16_t item)
>  	exif_entry_unref(entry);
>  }
>  
> +void Exif::setSShort(ExifIfd ifd, ExifTag tag, int16_t item)
> +{
> +	ExifEntry *entry = createEntry(ifd, tag);
> +	if (!entry)
> +		return;
> +
> +	exif_set_sshort(entry->data, EXIF_BYTE_ORDER_INTEL, item);
> +	exif_entry_unref(entry);
> +}
> +
>  void Exif::setLong(ExifIfd ifd, ExifTag tag, uint32_t item)
>  {
>  	ExifEntry *entry = createEntry(ifd, tag);
> @@ -194,6 +204,26 @@ 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);
> +
> +	/* If possible, query and set timezone information via

	/*
	 * If possible ...

> +	 * EXIF_TAG_TIME_ZONE_OFFSET. There is only per-hour resolution for tag
> +	 * hence, round up hours if minutes >= 30.
> +	 */
> +	char tz[6];

How about reusing the str variable ?

> +	int r = std::strftime(tz, sizeof(tz), "%z", std::localtime(&timestamp));

I've posted "[PATCH] android: jpeg: exif: Use reentrant localtime_r()"
which adds a local tm variable that you can reuse instead of calling
std::localtime() again.

> +	if (r > 0) {
> +		int16_t timezone = atoi(tz);

You should include stdlib.h for atoi.

> +		int16_t hour = timezone / 100;
> +		int16_t min = abs(timezone) - (abs(hour) * 100);
> +
> +		if (tz[0] == '-' && min >= 30)
> +			hour--;
> +		else if (min >= 30)
> +			hour++;

An alternative would be

		int16_t timezone = atoi(tz);
		int16_t hour = timezone / 100;
		int16_t min = timezone % 100;

		if (min <= -30)
			hour--;
		else if (min >= 30)
			hour++;

This relies on the fact that the modulo of a negative number will be a
negative number.

> +
> +		/* \todo Check if EXIF IFD is correct here or not. */

It's messy. The TIFF/EP specification defines the TimeZoneOffset tag in
IFD0, so that's where I think it should go. The EXIF specification
defines three other tags (OffsetTime, OffsetTimeOriginal,
OffsetTimeDigitized), in the EXIF IFD, but those are not supported by
libexif. Should we mention the unsupported tags in the commit message ?

Note that the DateTimeOriginal tag is defined by both the TIFF/EP and
EXIF specifications, but in different IFDs (IFD 0 for TIFF/EP and EXIF
IFD for EXIF). Lovely...

> +		setSShort(EXIF_IFD_EXIF, EXIF_TAG_TIME_ZONE_OFFSET, hour);

The TIFF/EP specification defined TimeZoneOffset as containing up to two
signed short values. The first value relates to DateTime, the second
value to DateTimeOriginal. As we don't set DateTimeOriginal in IFD0 I
would leave the second value out, but if we added DateTimeOriginal in
IFD0 in addition to the EXIF IFD, I would add the second value.

> +	}
>  }
>  
>  void Exif::setOrientation(int orientation)
> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h
> index 622de4c..4817815 100644
> --- a/src/android/jpeg/exif.h
> +++ b/src/android/jpeg/exif.h
> @@ -37,6 +37,7 @@ private:
>  			       unsigned long components, unsigned int size);
>  
>  	void setShort(ExifIfd ifd, ExifTag tag, uint16_t item);
> +	void setSShort(ExifIfd ifd, ExifTag tag, int16_t item);
>  	void setLong(ExifIfd ifd, ExifTag tag, uint32_t item);
>  	void setString(ExifIfd ifd, ExifTag tag, ExifFormat format,
>  		       const std::string &item);

-- 
Regards,

Laurent Pinchart