| Message ID | 20200923191731.11433-1-email@uajain.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, On 23/09/2020 20:17, Umang Jain wrote: > Hi, > > Again posting as a RFC but the patch is now functional as expected. > Tested with few range of values (+ -) and didn't see a problem. > > Let me know what you think. > --- > > Get timezone information from the timestamp, although the resolution > for EXIF_TAG_TIME_ZONE_OFFSET is fairly limited (per-hour only). > > This commit also introduces a special case handling for this tag. > In course of development, it was found that libexif does not > support this tag practically but still carries it among its > header files. The manual patching here turned out to be a small > detail that can conveniently be removed as and when libexif's > support for timezone information improves. Oh, I'm surprised here - I'm not sure I fully understand and would have to investigate - but I thought the setXXX() functions are all capable of creating a new entry when required, and they 'remove' any existing entry ... > > 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 | 44 +++++++++++++++++++++++++++++++++++++++ > src/android/jpeg/exif.h | 1 + > 2 files changed, 45 insertions(+) > > diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp > index c0dbfcc..27d8fde 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,29 @@ 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; > + /* > + * Special case handling for EXIF_TAG_TIME_ZONE_OFFSET. We need to > + * manually create and initialize an ExifEntry for this tag, since > + * the intialization support is missing from libexif's function > + * exif_entry_initialize(). > + * > + * \todo: Remove this special case when the above issue is fixed in > + * libexif. > + */ > + if (tag == EXIF_TAG_TIME_ZONE_OFFSET) > + entry = createEntry (ifd, tag, EXIF_FORMAT_SSHORT, 1, 2); > + else > + entry = createEntry(ifd, tag); Is the special case required to determine the size? Oh - I see, createEntry(ifd, tag) uses exif_entry_initialize() presumably to initialise from a table of values based on the tag ... which isn't provided for the EXIF_TAG_TIME_ZONE_OFFSET ... So calling the extended version is explicitly setting everything up ... Hrm ... a bit nasty indeed. Becuase this is a workaround, I'd almost be tempted to handle this in setTimestamp, as that's the only call path that needs it... > + 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 +221,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); So that would be remove that line (I do prefer the setSShort() type helpers in the general case) and add something like: > /* > * exif_entry_initialize() does not yet support > * EXIF_TAG_TIME_ZONE_OFFSET, so we have to do this > * manually. > */ > ExifEntry *entry = createEntry(EXIF_IFD_0, EXIF_TAG_TIME_ZONE_OFFSET, > EXIF_FORMAT_SSHORT, 1, 2); > if (entry) { > exif_set_sshort(entry->data, EXIF_BYTE_ORDER_INTEL, hour); > exif_entry_unref(entry); > } What do you think? Better - or easier just to have it in the new setSShort() call? -- Kieran > + } > } > > 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 Kieran, On Wed, Sep 23, 2020 at 09:44:54PM +0100, Kieran Bingham wrote: > On 23/09/2020 20:17, Umang Jain wrote: > > Hi, > > > > Again posting as a RFC but the patch is now functional as expected. > > Tested with few range of values (+ -) and didn't see a problem. > > > > Let me know what you think. > > --- > > > > Get timezone information from the timestamp, although the resolution > > for EXIF_TAG_TIME_ZONE_OFFSET is fairly limited (per-hour only). > > > > This commit also introduces a special case handling for this tag. > > In course of development, it was found that libexif does not > > support this tag practically but still carries it among its > > header files. The manual patching here turned out to be a small > > detail that can conveniently be removed as and when libexif's > > support for timezone information improves. > > Oh, I'm surprised here - I'm not sure I fully understand and would have > to investigate - but I thought the setXXX() functions are all capable of > creating a new entry when required, and they 'remove' any existing entry ... > > > > > 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 | 44 +++++++++++++++++++++++++++++++++++++++ > > src/android/jpeg/exif.h | 1 + > > 2 files changed, 45 insertions(+) > > > > diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp > > index c0dbfcc..27d8fde 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,29 @@ 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; > > + /* > > + * Special case handling for EXIF_TAG_TIME_ZONE_OFFSET. We need to > > + * manually create and initialize an ExifEntry for this tag, since > > + * the intialization support is missing from libexif's function > > + * exif_entry_initialize(). > > + * > > + * \todo: Remove this special case when the above issue is fixed in > > + * libexif. > > + */ > > + if (tag == EXIF_TAG_TIME_ZONE_OFFSET) > > + entry = createEntry (ifd, tag, EXIF_FORMAT_SSHORT, 1, 2); > > + else > > + entry = createEntry(ifd, tag); > > Is the special case required to determine the size? > > Oh - I see, createEntry(ifd, tag) uses exif_entry_initialize() > presumably to initialise from a table of values based on the tag ... > which isn't provided for the EXIF_TAG_TIME_ZONE_OFFSET ... > > So calling the extended version is explicitly setting everything up ... > > Hrm ... a bit nasty indeed. > > Becuase this is a workaround, I'd almost be tempted to handle this in > setTimestamp, as that's the only call path that needs it... > > > > + 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 +221,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); > > So that would be remove that line (I do prefer the setSShort() type > helpers in the general case) and add something like: > > > /* > > * exif_entry_initialize() does not yet support > > * EXIF_TAG_TIME_ZONE_OFFSET, so we have to do this > > * manually. > > */ > > ExifEntry *entry = createEntry(EXIF_IFD_0, EXIF_TAG_TIME_ZONE_OFFSET, > > EXIF_FORMAT_SSHORT, 1, 2); > > if (entry) { > > exif_set_sshort(entry->data, EXIF_BYTE_ORDER_INTEL, hour); > > exif_entry_unref(entry); > > } > > What do you think? Better - or easier just to have it in the new > setSShort() call? That would be better, but if we go down the route of adding tags that are not fully supported by the library, we can as well add the EXIF timezone tags, not the TIFF/EP tag. Those use strings, so setString() should work out of the box. > > + } > > } > > > > 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..27d8fde 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,29 @@ 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; + /* + * Special case handling for EXIF_TAG_TIME_ZONE_OFFSET. We need to + * manually create and initialize an ExifEntry for this tag, since + * the intialization support is missing from libexif's function + * exif_entry_initialize(). + * + * \todo: Remove this special case when the above issue is fixed in + * libexif. + */ + if (tag == EXIF_TAG_TIME_ZONE_OFFSET) + entry = createEntry (ifd, tag, EXIF_FORMAT_SSHORT, 1, 2); + else + 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 +221,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);
Hi, Again posting as a RFC but the patch is now functional as expected. Tested with few range of values (+ -) and didn't see a problem. Let me know what you think. --- Get timezone information from the timestamp, although the resolution for EXIF_TAG_TIME_ZONE_OFFSET is fairly limited (per-hour only). This commit also introduces a special case handling for this tag. In course of development, it was found that libexif does not support this tag practically but still carries it among its header files. The manual patching here turned out to be a small detail that can conveniently be removed as and when libexif's support for timezone information improves. 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 | 44 +++++++++++++++++++++++++++++++++++++++ src/android/jpeg/exif.h | 1 + 2 files changed, 45 insertions(+)