[libcamera-devel] [RFC v1 PATCH] android: jpeg: exif: Set timezone information
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Sep 23 15:29:34 CEST 2020
Hello,
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() ?
> > 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.
> 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