[libcamera-devel] [RFC v1 PATCH] android: jpeg: exif: Set timezone information
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Sep 23 17:41:12 CEST 2020
Hi Umang,
On Wed, Sep 23, 2020 at 07:18:03PM +0530, Umang Jain wrote:
> On 9/23/20 6:59 PM, Laurent Pinchart wrote:
> > On Wed, Sep 23, 2020 at 06:23:23PM +0530, Umang Jain wrote:
> >> On 9/23/20 4:26 PM, Kieran Bingham wrote:
> >>> On 23/09/2020 10:13, Umang Jain wrote:
> >>>> 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.
> >>
> >> I didn't find any particular debug option in exiftool that would give
> >> out a error message. Although I found -v3 verbose output which can give
> >> hexdump of each tag. So attaching the various outputs here:
> >>
> >> With:
> >> a) setString(EXIF_IFD_0, EXIF_TAG_TIME_ZONE_OFFSET, EXIF_FORMAT_ASCII, "6");
> >> o/p:
> >> | 8) TimeZoneOffset = 6
> >> | - Tag 0x882a (2 bytes, string[2]):
> >> | 0090: 36 00 [6.]
> >> JPEG DQT (65 bytes):
> >>
> >>
> >> b) setSShort(EXIF_IFD_0, EXIF_TAG_TIME_ZONE_OFFSET, '6' or 6);
> >> o/p:
> >> | 8) TimeZoneOffset =
> >> | - Tag 0x882a (0 bytes, undef):
> >>
> >>> What does exiv2 report on the generated image?
> >>
> >> `exiv2` tool doesn't report the tag in any of the cases.
> >> `exif` tool reports it properly only if setString is used...
> >>
> >>>>> 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.
> >>
> >> oh, I misinterpreted things then.
> >>
> >>>>> + 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?
> >>
> >> It's what the tools use to do. They report on per-hour basis only. I
> >> have mentioned the behaviour in the commit message.
> >> Also stumbled upon
> >> https://github.com/jim-easterbrook/Photini/commit/686d26 literally few
> >> minutes ago.
> >
> > TimeZoneOffset is defined in the TIFF/EP specification (ISO/DIS
> > 12234-2).
> >
> > 5.2.48 TimeZoneOffset
> >
> > 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.
> >
> > Tag Name = TimeZoneOffset
> > Tag = 34858 (882A.H)
> > Type = SSHORT
> > N = 1 or 2
> > Value = VALUE
> > The allowed values are -12 to +11.
> >
> > SSHORT 0 Time Zone Offset (in hours) of DateTimeOriginal tag-value relative to Greenwich Mean Time
> > SSHORT 1 If present, Time Zone Offset (in hours) of DateTime tag-value relative to Greenwich Mean Time
> >
> > Usage: IFD0
> >
> > It's fairly ill-defined as time zones extend to UTC+14, but it's clearly
> > a signed short, with one or two values, expressed as a number of hours.
> > We should really use the time zone tags of the EXIF specification
> > instead, but they're not supported by libexif.
> >
> >>>>> + */
> >>>>> + 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?
> >>
> >> It showed '6', no integer/ascii value of '6' was spotted anytime with
> >> any tool.
> >>
> >>> 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!?
> >>
> >> tried to setSShort with ascii value of 6, no improvements.
> >> If setSShort is used, the exiftool reports it as 'empty' value :(
> >> https://paste.debian.net/1164408/
> >
> > Can you provide a sample image that has the time zone offset tag set
> > with setSShort() ?
>
> Please find attached a sample image to this mail.
There's clearly something wrong. The IFD entry is set to
2A 88 -> Tag = 0x882a, TimeZoneOffset
07 00 -> Type = Undefined
00 00 00 00 -> Count = 0
00 00 00 00 -> Value = 0
A short investigation in exif_entry_initialize() showed that it doesn't
support EXIF_TAG_TIME_ZONE_OFFSET.
I see two options, either adding the tag manually instead of calling
exif_entry_initialize(), or stop bothering with time zones until libexif
gets proper support for them.
> >>> 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.
> >>
> >> There are 3 more timezone offset related tags that are mentioned but not
> >> supported by libexif. Laurent has pointed this out in previous replies
> >> to this thread.
> >>
> >>> I expect we should explicitly set our exif version to "0231" if we use it?
> >>
> >> hmm, yea, looking at the above pastebin output, my exif version is
> >> '0210' so I would agree setting it 0231 because that's where the tag is
> >> introduced.
> >
> > The TimeZoneOffset tag doesn't exist in any EXIF specification, so I
> > don't think the EXIF version will make any difference.
>
> Ah, right. Booooing! :head_bang:
>
> >> My local version of libexif is 0.6.21 which already seems to support the
> >> timezone offset tag. Although, I would expect it to report that the tag
> >> I am trying set needs a higher exif version.
> >>
> >> (Explicitly set exif version to 0231, no improvements observed)
> >>
> >>> Do the exif/exiv2 libraries support Exif 2.31?
> >>
> >> hmm, probably : https://github.com/libexif/libexif/commit/54333d8b8c
> >>
> >> Not sure, how they keep up with minor versions, OR rather support new
> >> tags directly.
> >>
> >>>> [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);
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list