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

[Kieran Bingham]

Hi Umang,

On 23/09/2020 10:13, Umang Jain wrote:
> Hi,
> 
> On 9/22/20 9:56 PM, Umang Jain wrote:
>> Get timezone information from the timestamp, although the resolution
>> for EXIF_TAG_TIME_ZONE_OFFSET is fairly limited (per-hour only).
>>
>> 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
>>
>> 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 | 31 +++++++++++++++++++++++++++++++
>>   src/android/jpeg/exif.h   |  1 +
>>   2 files changed, 32 insertions(+)
>>
>> ---
>>
>> Hi Laurent/Kieran,
>>
>> Can you soft review(or test) this patch to see if anything
>> is wrong with it?
>> The reason I am asking, because I cannot make it work :(
>> exiftool with a captured frame shows empty Timezone Offset tag as:
>> ```
>> Time Zone Offset                :
>> ```
>>
>> exif tool shows nothing.

Hrm, maybe adding some debug to the exif tool and rebuilding might help
identify what data it actually parses or why it can't identify the field.

What does exiv2 report on the generated image?

>>
>> I made sure a valid value being generated for timezone
>> offset in setTimeStamp().
>> I also checked the case where it might be preset using
>> exif_data_fix and not been able to re-set again.
>> But no, that doesn't seem the problem in my testing.
>>
>>
>> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
>> index c0dbfcc..9c23cfb 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,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);
> Getting to bottom of this in the morning, exif_set_sshort seems to
> convert its passed int16_t value to character. I don't know what's the
> logic behind this [1] (apart from EXIF's quirky-ness), but this simply
> doesn't look right then. So..


Oh - converting a short to a char certainly doesn't sound right at all
for a function called 'set_short'.

Char's are 8-bits. Shorts are 16....

Oh - Now I've followed to [1], I see that's simply dealing with
byte-ordering issues. So it shouldn't be a problem.



>> +    exif_entry_unref(entry);
>> +}
>> +
>>   void Exif::setLong(ExifIfd ifd, ExifTag tag, uint32_t item)
>>   {
>>       ExifEntry *entry = createEntry(ifd, tag);
>> @@ -196,6 +208,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.

Where have you found the information that it is only 'per-hour' resolution?


>> +     */
>> +    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;;
> Complier should have errored out on ;; , but it didn't.
> Rectified locally on noticing.

;; isn't going to generate a compiler error because the second ';' is
just an empty statement that does nothing.

Still worth cleaning up of course ;-)


>> +
>> +        if (min <= -30)
>> +            hour--;
>> +        else if (min >= 30)
>> +            hour++;
>> +
>> +        setSShort(EXIF_IFD_0, EXIF_TAG_TIME_ZONE_OFFSET, hour);
> Instead of setSShort call, if we use setString, things seems to get
> moving. Something like:
> setString(EXIF_IFD_0, EXIF_TAG_TIME_ZONE_OFFSET, EXIF_FORMAT_ASCII, "6");
> 
> got parsed correctly by exiftool and exif commandline utilities.

Ok, so lets tie down exactly what the right format should be written to
this field.

Writing '6' as ascii infers that the actual value that got written to
memory in that place was 54....

What was presented with the tools? Did it show a '6' or the integer value?

If this really is a field for a short, we should write the ascii value
to the short, not a string... which (currently) adds null padding too!?

Interestingly:
https://github.com/libexif/libexif/blob/master/libexif/exif-tag.c#L523
seems to state that EXIF_TAG_TIME_ZONE_OFFSET is not in Exif-2.2...

Indeed, examining closely I see no reference to a tag with id 0x882a

https://www.exiv2.org/tags.html lists Exif.Image.TimeZoneOffset as an
SShort at 0x882a (34858), with the following text:

"""
This optional tag encodes the time zone of the camera clock (relative to
Greenwich Mean Time) used to create the DataTimeOriginal tag-value when
the picture was taken. It may also contain the time zone offset of the
clock used to create the DateTime tag-value when the image was modified.
"""

But that doesn't fully explain how the value is encoded.




And ... now I've gone further down the rabbit-hole, I've discovered Exif
2.31 is available from :

  http://cipa.jp/std/documents/download_e.html?DC-008-Translation-2016-E

It also doesn't list a tag at 0x882a, but does explicitly mention a tag
"OffsetTime" (0x9010) which is an ascii field matching the string
generated by the strftime(str, sizeof(str), "%z", &tm) call ;-)


See page 49 in that document from cipa.jp.

I expect we should explicitly set our exif version to "0231" if we use it?

Do the exif/exiv2 libraries support Exif 2.31?

--
Regards

Kieran



> [1]:
> https://github.com/libexif/libexif/blob/master/libexif/exif-utils.c#L107-L121
> 
>> +    }
>>   }
>>     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);
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards
--
Kieran