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

[Kieran Bingham]

Hi Umang,

On 23/09/2020 20:17, Umang Jain wrote:
> Hi,
> 
> Again posting as a RFC but the patch is now functional as expected.
> Tested with few range of values (+ -) and didn't see a problem.
> 
> Let me know what you think.
> ---
> 
> Get timezone information from the timestamp, although the resolution
> for EXIF_TAG_TIME_ZONE_OFFSET is fairly limited (per-hour only).
> 
> This commit also introduces a special case handling for this tag.
> In course of development, it was found that libexif does not
> support this tag practically but still carries it among its
> header files. The manual patching here turned out to be a small
> detail that can conveniently be removed as and when libexif's
> support for timezone information improves.

Oh, I'm surprised here - I'm not sure I fully understand and would have
to investigate - but I thought the setXXX() functions are all capable of
creating a new entry when required, and they 'remove' any existing entry ...

> 
> The EXIF specification defines three other tags (OffsetTime,
> OffsetTimeOriginal, OffsetTimeDigitized), in the EXIF IFD.
> These are not supported by libexif yet.
> 
> Signed-off-by: Umang Jain <email at uajain.com>
> ---
>  src/android/jpeg/exif.cpp | 44 +++++++++++++++++++++++++++++++++++++++
>  src/android/jpeg/exif.h   |  1 +
>  2 files changed, 45 insertions(+)
> 
> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
> index c0dbfcc..27d8fde 100644
> --- a/src/android/jpeg/exif.cpp
> +++ b/src/android/jpeg/exif.cpp
> @@ -7,6 +7,8 @@
>  
>  #include "exif.h"
>  
> +#include <stdlib.h>
> +
>  #include "libcamera/internal/log.h"
>  
>  using namespace libcamera;
> @@ -135,6 +137,29 @@ 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;
> +	/*
> +	 * Special case handling for EXIF_TAG_TIME_ZONE_OFFSET. We need to
> +	 * manually create and initialize an ExifEntry for this tag, since
> +	 * the intialization support is missing from libexif's function
> +	 * exif_entry_initialize().
> +	 *
> +	 * \todo: Remove this special case when the above issue is fixed in
> +	 * libexif.
> +	 */
> +	if (tag == EXIF_TAG_TIME_ZONE_OFFSET)
> +		entry = createEntry (ifd, tag, EXIF_FORMAT_SSHORT, 1, 2);
> +	else
> +		entry = createEntry(ifd, tag);

Is the special case required to determine the size?

Oh - I see, createEntry(ifd, tag) uses exif_entry_initialize()
presumably to initialise from a table of values based on the tag ...
which isn't provided for the EXIF_TAG_TIME_ZONE_OFFSET ...

So calling the extended version is explicitly setting everything up ...

Hrm ... a bit nasty indeed.

Becuase this is a workaround, I'd almost be tempted to handle this in
setTimestamp, as that's the only call path that needs it...


> +	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);
> @@ -196,6 +221,25 @@ 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
> +	 * EXIF_TAG_TIME_ZONE_OFFSET. There is only per-hour resolution for tag
> +	 * hence, round up hours if minutes >= 30.
> +	 */
> +	int r = strftime(str, sizeof(str), "%z", &tm);
> +	if (r > 0) {
> +		int16_t timezone = atoi(str);
> +		int16_t hour = timezone / 100;
> +		int16_t min = timezone % 100;
> +
> +		if (min <= -30)
> +			hour--;
> +		else if (min >= 30)
> +			hour++;
> +
> +		setSShort(EXIF_IFD_0, EXIF_TAG_TIME_ZONE_OFFSET, hour);

So that would be remove that line (I do prefer the setSShort() type
helpers in the general case) and add something like:

> 		/*
> 		 * exif_entry_initialize() does not yet support
> 		 * EXIF_TAG_TIME_ZONE_OFFSET, so we have to do this
> 		 * manually.
> 		 */
> 		ExifEntry *entry = createEntry(EXIF_IFD_0, EXIF_TAG_TIME_ZONE_OFFSET,
> 						EXIF_FORMAT_SSHORT, 1, 2);
> 		if (entry) {
> 			exif_set_sshort(entry->data, EXIF_BYTE_ORDER_INTEL, hour);
> 			exif_entry_unref(entry);
> 		}

What do you think? Better - or easier just to have it in the new
setSShort() call?

--
Kieran


> +	}
>  }
>  
>  void Exif::setOrientation(int orientation)
> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h
> index f04cefc..9c9cc3b 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
--
Kieran