| Message ID | 20200922162624.32321-1-email@uajain.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@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. > > 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.. > + 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. > + */ > + 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. > + > + 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. [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);
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@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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Kieran On 9/23/20 4:26 PM, Kieran Bingham wrote: > 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@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. <https://github.com/jim-easterbrook/Photini/commit/686d26> > > >>> + */ >>> + 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/ > > 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. 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. > > -- > 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@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel
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@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);
Hi Laurent, On 9/23/20 6:59 PM, Laurent Pinchart wrote: > 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@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. > >>> 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);
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@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);
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); + 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. + */ + 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); + } } 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);
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@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. 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.