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

[Umang Jain]

Hi Laurent,
On 9/10/20 9:55 AM, Laurent Pinchart wrote:
> 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 ?
Sure. I think it would be worth it next time someone is touching this 
piece of code.
>
> 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...
It's weird how these specs conflict when one (EXIF) is a reduced subset 
of another (TIFF/EP).
>
>> +		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.
I agree with your analysis. Can I ask when/how would we add 
DateTimeOriginal in IFD0? I guess when we start supporting 
TIFF-compliant metadata via libtiff?

Also, this feels important decision/information. Should it be also 
considered to be mentioned as part of commit message?
>
>> +	}
>>   }
>>   
>>   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);