[{"id":12394,"web_url":"https://patchwork.libcamera.org/comment/12394/","msgid":"<20200910042547.GE4009@pendragon.ideasonboard.com>","date":"2020-09-10T04:25:47","subject":"Re: [libcamera-devel] [RFC PATCH] android: jpeg: exif: Set timezone\n\tinformation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nThank you for the patch.\n\nOn Wed, Sep 09, 2020 at 09:02:06PM +0530, Umang Jain wrote:\n> Get timestamp information from the timestamp, although the resolution\n\ns/timestamp information/timezone information/\n\n> for EXIF_TAG_TIME_ZONE_OFFSET is fairly limtied (per-hour only).\n\ns/limtied/limited/\n\n> \n> Experimentation with 'exiftool', commandline utility to read/write\n> exif metadata on images, resulted in rounding off the hours if the\n> minutes came out to >= 30. Hence, the behaviour is inspired from\n> exiftool itself. For e.g.,\n> \n> Timezone Tag's value\n> +1015 => 10\n> +0945 => 10\n> -1145 => -12\n\nSounds good to me.\n\n> Signed-off-by: Umang Jain \n> ---\n> src/android/jpeg/exif.cpp | 30 ++++++++++++++++++++++++++++++\n> src/android/jpeg/exif.h | 1 +\n> 2 files changed, 31 insertions(+)\n> \n> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp\n> index 031f5f4..d8b4537 100644\n> --- a/src/android/jpeg/exif.cpp\n> +++ b/src/android/jpeg/exif.cpp\n> @@ -135,6 +135,16 @@ void Exif::setShort(ExifIfd ifd, ExifTag tag, uint16_t item)\n> \texif_entry_unref(entry);\n> }\n> \n> +void Exif::setSShort(ExifIfd ifd, ExifTag tag, int16_t item)\n> +{\n> +\tExifEntry *entry = createEntry(ifd, tag);\n> +\tif (!entry)\n> +\t\treturn;\n> +\n> +\texif_set_sshort(entry->data, EXIF_BYTE_ORDER_INTEL, item);\n> +\texif_entry_unref(entry);\n> +}\n> +\n> void Exif::setLong(ExifIfd ifd, ExifTag tag, uint32_t item)\n> {\n> \tExifEntry *entry = createEntry(ifd, tag);\n> @@ -194,6 +204,26 @@ void Exif::setTimestamp(time_t timestamp)\n> \tsetString(EXIF_IFD_0, EXIF_TAG_DATE_TIME, EXIF_FORMAT_ASCII, ts);\n> \tsetString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_ORIGINAL, EXIF_FORMAT_ASCII, ts);\n> \tsetString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_DIGITIZED, EXIF_FORMAT_ASCII, ts);\n> +\n> +\t/* If possible, query and set timezone information via\n\n\t/*\n\t * If possible ...\n\n> +\t * EXIF_TAG_TIME_ZONE_OFFSET. There is only per-hour resolution for tag\n> +\t * hence, round up hours if minutes >= 30.\n> +\t */\n> +\tchar tz[6];\n\nHow about reusing the str variable ?\n\n> +\tint r = std::strftime(tz, sizeof(tz), \"%z\", std::localtime(×tamp));\n\nI've posted \"[PATCH] android: jpeg: exif: Use reentrant localtime_r()\"\nwhich adds a local tm variable that you can reuse instead of calling\nstd::localtime() again.\n\n> +\tif (r > 0) {\n> +\t\tint16_t timezone = atoi(tz);\n\nYou should include stdlib.h for atoi.\n\n> +\t\tint16_t hour = timezone / 100;\n> +\t\tint16_t min = abs(timezone) - (abs(hour) * 100);\n> +\n> +\t\tif (tz[0] == '-' && min >= 30)\n> +\t\t\thour--;\n> +\t\telse if (min >= 30)\n> +\t\t\thour++;\n\nAn alternative would be\n\n\t\tint16_t timezone = atoi(tz);\n\t\tint16_t hour = timezone / 100;\n\t\tint16_t min = timezone % 100;\n\n\t\tif (min <= -30)\n\t\t\thour--;\n\t\telse if (min >= 30)\n\t\t\thour++;\n\nThis relies on the fact that the modulo of a negative number will be a\nnegative number.\n\n> +\n> +\t\t/* \\todo Check if EXIF IFD is correct here or not. */\n\nIt's messy. The TIFF/EP specification defines the TimeZoneOffset tag in\nIFD0, so that's where I think it should go. The EXIF specification\ndefines three other tags (OffsetTime, OffsetTimeOriginal,\nOffsetTimeDigitized), in the EXIF IFD, but those are not supported by\nlibexif. Should we mention the unsupported tags in the commit message ?\n\nNote that the DateTimeOriginal tag is defined by both the TIFF/EP and\nEXIF specifications, but in different IFDs (IFD 0 for TIFF/EP and EXIF\nIFD for EXIF). Lovely...\n\n> +\t\tsetSShort(EXIF_IFD_EXIF, EXIF_TAG_TIME_ZONE_OFFSET, hour);\n\nThe TIFF/EP specification defined TimeZoneOffset as containing up to two\nsigned short values. The first value relates to DateTime, the second\nvalue to DateTimeOriginal. As we don't set DateTimeOriginal in IFD0 I\nwould leave the second value out, but if we added DateTimeOriginal in\nIFD0 in addition to the EXIF IFD, I would add the second value.\n\n> +\t}\n> }\n> \n> void Exif::setOrientation(int orientation)\n> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h\n> index 622de4c..4817815 100644\n> --- a/src/android/jpeg/exif.h\n> +++ b/src/android/jpeg/exif.h\n> @@ -37,6 +37,7 @@ private:\n> \t\t\t unsigned long components, unsigned int size);\n> \n> \tvoid setShort(ExifIfd ifd, ExifTag tag, uint16_t item);\n> +\tvoid setSShort(ExifIfd ifd, ExifTag tag, int16_t item);\n> \tvoid setLong(ExifIfd ifd, ExifTag tag, uint32_t item);\n> \tvoid setString(ExifIfd ifd, ExifTag tag, ExifFormat format,\n> \t\t const std::string &item);","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 55F3EBDB1C\n\tfor ;\n\tThu, 10 Sep 2020 04:26:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DE4F662C43;\n\tThu, 10 Sep 2020 06:26:17 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1A5D062B90\n\tfor ;\n\tThu, 10 Sep 2020 06:26:16 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 660F339;\n\tThu, 10 Sep 2020 06:26:14 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"DpkGX3/E\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1599711974;\n\tbh=2lz261941jMZM6PK+7xU/WmtmM62s2LVti0jYShwkG4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DpkGX3/EwCm4/TkPkIUGUo7ie2b8KSVB7DzOTx6RiN/Tg3QXNXcOce2DeUMjeXvva\n\tOCP6VgKP6M+7EuVl/R0o4tUpcjP2xLTfcUJ/Jx1RgGq/BHynzh/UllE3TZBlVn9VNI\n\t3DaMb9op/8tB7ERwqXDosLRiORQ1ROapaqwUCeGw=","Date":"Thu, 10 Sep 2020 07:25:47 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"<20200910042547.GE4009@pendragon.ideasonboard.com>","References":"<20200909153206.7700-1-email@uajain.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200909153206.7700-1-email@uajain.com>","Subject":"Re: [libcamera-devel] [RFC PATCH] android: jpeg: exif: Set timezone\n\tinformation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12597,"web_url":"https://patchwork.libcamera.org/comment/12597/","msgid":"<20200920132334.GA4128@pendragon.ideasonboard.com>","date":"2020-09-20T13:23:34","subject":"Re: [libcamera-devel] [RFC PATCH] android: jpeg: exif: Set timezone\n\tinformation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Thu, Sep 10, 2020 at 07:25:47AM +0300, Laurent Pinchart wrote:\n> Hi Umang,\n> \n> Thank you for the patch.\n> \n> On Wed, Sep 09, 2020 at 09:02:06PM +0530, Umang Jain wrote:\n> > Get timestamp information from the timestamp, although the resolution\n> \n> s/timestamp information/timezone information/\n> \n> > for EXIF_TAG_TIME_ZONE_OFFSET is fairly limtied (per-hour only).\n> \n> s/limtied/limited/\n> \n> > Experimentation with 'exiftool', commandline utility to read/write\n> > exif metadata on images, resulted in rounding off the hours if the\n> > minutes came out to >= 30. Hence, the behaviour is inspired from\n> > exiftool itself. For e.g.,\n> > \n> > Timezone Tag's value\n> > +1015 => 10\n> > +0945 => 10\n> > -1145 => -12\n> \n> Sounds good to me.\n> \n> > Signed-off-by: Umang Jain \n> > ---\n> > src/android/jpeg/exif.cpp | 30 ++++++++++++++++++++++++++++++\n> > src/android/jpeg/exif.h | 1 +\n> > 2 files changed, 31 insertions(+)\n> > \n> > diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp\n> > index 031f5f4..d8b4537 100644\n> > --- a/src/android/jpeg/exif.cpp\n> > +++ b/src/android/jpeg/exif.cpp\n> > @@ -135,6 +135,16 @@ void Exif::setShort(ExifIfd ifd, ExifTag tag, uint16_t item)\n> > \texif_entry_unref(entry);\n> > }\n> > \n> > +void Exif::setSShort(ExifIfd ifd, ExifTag tag, int16_t item)\n> > +{\n> > +\tExifEntry *entry = createEntry(ifd, tag);\n> > +\tif (!entry)\n> > +\t\treturn;\n> > +\n> > +\texif_set_sshort(entry->data, EXIF_BYTE_ORDER_INTEL, item);\n> > +\texif_entry_unref(entry);\n> > +}\n> > +\n> > void Exif::setLong(ExifIfd ifd, ExifTag tag, uint32_t item)\n> > {\n> > \tExifEntry *entry = createEntry(ifd, tag);\n> > @@ -194,6 +204,26 @@ void Exif::setTimestamp(time_t timestamp)\n> > \tsetString(EXIF_IFD_0, EXIF_TAG_DATE_TIME, EXIF_FORMAT_ASCII, ts);\n> > \tsetString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_ORIGINAL, EXIF_FORMAT_ASCII, ts);\n> > \tsetString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_DIGITIZED, EXIF_FORMAT_ASCII, ts);\n> > +\n> > +\t/* If possible, query and set timezone information via\n> \n> \t/*\n> \t * If possible ...\n> \n> > +\t * EXIF_TAG_TIME_ZONE_OFFSET. There is only per-hour resolution for tag\n> > +\t * hence, round up hours if minutes >= 30.\n> > +\t */\n> > +\tchar tz[6];\n> \n> How about reusing the str variable ?\n> \n> > +\tint r = std::strftime(tz, sizeof(tz), \"%z\", std::localtime(×tamp));\n> \n> I've posted \"[PATCH] android: jpeg: exif: Use reentrant localtime_r()\"\n> which adds a local tm variable that you can reuse instead of calling\n> std::localtime() again.\n\nI've now pushed that patch to master. Could you rebase this one ?\n\n> > +\tif (r > 0) {\n> > +\t\tint16_t timezone = atoi(tz);\n> \n> You should include stdlib.h for atoi.\n> \n> > +\t\tint16_t hour = timezone / 100;\n> > +\t\tint16_t min = abs(timezone) - (abs(hour) * 100);\n> > +\n> > +\t\tif (tz[0] == '-' && min >= 30)\n> > +\t\t\thour--;\n> > +\t\telse if (min >= 30)\n> > +\t\t\thour++;\n> \n> An alternative would be\n> \n> \t\tint16_t timezone = atoi(tz);\n> \t\tint16_t hour = timezone / 100;\n> \t\tint16_t min = timezone % 100;\n> \n> \t\tif (min <= -30)\n> \t\t\thour--;\n> \t\telse if (min >= 30)\n> \t\t\thour++;\n> \n> This relies on the fact that the modulo of a negative number will be a\n> negative number.\n> \n> > +\n> > +\t\t/* \\todo Check if EXIF IFD is correct here or not. */\n> \n> It's messy. The TIFF/EP specification defines the TimeZoneOffset tag in\n> IFD0, so that's where I think it should go.\n\nI've also checked how libexif and libexiv2 handle reading this tag\n(through the exif and exiv2 command line utilities respectively).\nlibexif seems to parse it fine regardless of whether it's stored in IFD0\nor in int EXIF IFD, while libexiv2 seem to only handle it when stored in\nIFD0 (the tag isn't printed when stored in the EXIF IFD). IFD0 thus\nseems to be the right pick.\n\n> The EXIF specification\n> defines three other tags (OffsetTime, OffsetTimeOriginal,\n> OffsetTimeDigitized), in the EXIF IFD, but those are not supported by\n> libexif. Should we mention the unsupported tags in the commit message ?\n> \n> Note that the DateTimeOriginal tag is defined by both the TIFF/EP and\n> EXIF specifications, but in different IFDs (IFD 0 for TIFF/EP and EXIF\n> IFD for EXIF). Lovely...\n> \n> > +\t\tsetSShort(EXIF_IFD_EXIF, EXIF_TAG_TIME_ZONE_OFFSET, hour);\n> \n> The TIFF/EP specification defined TimeZoneOffset as containing up to two\n> signed short values. The first value relates to DateTime, the second\n> value to DateTimeOriginal. As we don't set DateTimeOriginal in IFD0 I\n> would leave the second value out, but if we added DateTimeOriginal in\n> IFD0 in addition to the EXIF IFD, I would add the second value.\n> \n> > +\t}\n> > }\n> > \n> > void Exif::setOrientation(int orientation)\n> > diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h\n> > index 622de4c..4817815 100644\n> > --- a/src/android/jpeg/exif.h\n> > +++ b/src/android/jpeg/exif.h\n> > @@ -37,6 +37,7 @@ private:\n> > \t\t\t unsigned long components, unsigned int size);\n> > \n> > \tvoid setShort(ExifIfd ifd, ExifTag tag, uint16_t item);\n> > +\tvoid setSShort(ExifIfd ifd, ExifTag tag, int16_t item);\n> > \tvoid setLong(ExifIfd ifd, ExifTag tag, uint32_t item);\n> > \tvoid setString(ExifIfd ifd, ExifTag tag, ExifFormat format,\n> > \t\t const std::string &item);","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id AE54FC3B5B\n\tfor ;\n\tSun, 20 Sep 2020 13:24:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2163862FBC;\n\tSun, 20 Sep 2020 15:24:09 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7CFA562B90\n\tfor ;\n\tSun, 20 Sep 2020 15:24:07 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 191AB4FB;\n\tSun, 20 Sep 2020 15:24:06 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"CD6pMIib\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1600608246;\n\tbh=upkMO3yeTXxUYwpsG9CYqKAKtpm3a5yn3UdtuA8DEes=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=CD6pMIibPb7m2616dYvVeaoI+QCs166H+7S6UTHoF5CGsLF4n+dMjUJQTHo7OSM4g\n\ttrKeydIvmh4x+6Crv5G+ySOqy5+pqnyxKqwcP4TP/FocEeM+Z3q6lY/ac0Z1EMWO7r\n\t44oEL5wB0i5J3jn8dz0sui033A877c6+ubTz4SAA=","Date":"Sun, 20 Sep 2020 16:23:34 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"<20200920132334.GA4128@pendragon.ideasonboard.com>","References":"<20200909153206.7700-1-email@uajain.com>\n\t<20200910042547.GE4009@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200910042547.GE4009@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH] android: jpeg: exif: Set timezone\n\tinformation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12639,"web_url":"https://patchwork.libcamera.org/comment/12639/","msgid":"","date":"2020-09-22T11:43:39","subject":"Re: [libcamera-devel] [RFC PATCH] android: jpeg: exif: Set timezone\n\tinformation","submitter":{"id":1,"url":"https://patchwork.libcamera.org/api/people/1/","name":"Umang Jain","email":"email@uajain.com"},"content":"Hi Laurent,\nOn 9/10/20 9:55 AM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Wed, Sep 09, 2020 at 09:02:06PM +0530, Umang Jain wrote:\n>> Get timestamp information from the timestamp, although the resolution\n> s/timestamp information/timezone information/\n>\n>> for EXIF_TAG_TIME_ZONE_OFFSET is fairly limtied (per-hour only).\n> s/limtied/limited/\n>\n>> Experimentation with 'exiftool', commandline utility to read/write\n>> exif metadata on images, resulted in rounding off the hours if the\n>> minutes came out to >= 30. Hence, the behaviour is inspired from\n>> exiftool itself. For e.g.,\n>>\n>> Timezone Tag's value\n>> +1015 => 10\n>> +0945 => 10\n>> -1145 => -12\n> Sounds good to me.\n>\n>> Signed-off-by: Umang Jain \n>> ---\n>> src/android/jpeg/exif.cpp | 30 ++++++++++++++++++++++++++++++\n>> src/android/jpeg/exif.h | 1 +\n>> 2 files changed, 31 insertions(+)\n>>\n>> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp\n>> index 031f5f4..d8b4537 100644\n>> --- a/src/android/jpeg/exif.cpp\n>> +++ b/src/android/jpeg/exif.cpp\n>> @@ -135,6 +135,16 @@ void Exif::setShort(ExifIfd ifd, ExifTag tag, uint16_t item)\n>> \texif_entry_unref(entry);\n>> }\n>> \n>> +void Exif::setSShort(ExifIfd ifd, ExifTag tag, int16_t item)\n>> +{\n>> +\tExifEntry *entry = createEntry(ifd, tag);\n>> +\tif (!entry)\n>> +\t\treturn;\n>> +\n>> +\texif_set_sshort(entry->data, EXIF_BYTE_ORDER_INTEL, item);\n>> +\texif_entry_unref(entry);\n>> +}\n>> +\n>> void Exif::setLong(ExifIfd ifd, ExifTag tag, uint32_t item)\n>> {\n>> \tExifEntry *entry = createEntry(ifd, tag);\n>> @@ -194,6 +204,26 @@ void Exif::setTimestamp(time_t timestamp)\n>> \tsetString(EXIF_IFD_0, EXIF_TAG_DATE_TIME, EXIF_FORMAT_ASCII, ts);\n>> \tsetString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_ORIGINAL, EXIF_FORMAT_ASCII, ts);\n>> \tsetString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_DIGITIZED, EXIF_FORMAT_ASCII, ts);\n>> +\n>> +\t/* If possible, query and set timezone information via\n> \t/*\n> \t * If possible ...\n>\n>> +\t * EXIF_TAG_TIME_ZONE_OFFSET. There is only per-hour resolution for tag\n>> +\t * hence, round up hours if minutes >= 30.\n>> +\t */\n>> +\tchar tz[6];\n> How about reusing the str variable ?\n>\n>> +\tint r = std::strftime(tz, sizeof(tz), \"%z\", std::localtime(×tamp));\n> I've posted \"[PATCH] android: jpeg: exif: Use reentrant localtime_r()\"\n> which adds a local tm variable that you can reuse instead of calling\n> std::localtime() again.\n>\n>> +\tif (r > 0) {\n>> +\t\tint16_t timezone = atoi(tz);\n> You should include stdlib.h for atoi.\n>\n>> +\t\tint16_t hour = timezone / 100;\n>> +\t\tint16_t min = abs(timezone) - (abs(hour) * 100);\n>> +\n>> +\t\tif (tz[0] == '-' && min >= 30)\n>> +\t\t\thour--;\n>> +\t\telse if (min >= 30)\n>> +\t\t\thour++;\n> An alternative would be\n>\n> \t\tint16_t timezone = atoi(tz);\n> \t\tint16_t hour = timezone / 100;\n> \t\tint16_t min = timezone % 100;\n>\n> \t\tif (min <= -30)\n> \t\t\thour--;\n> \t\telse if (min >= 30)\n> \t\t\thour++;\n>\n> This relies on the fact that the modulo of a negative number will be a\n> negative number.\n>\n>> +\n>> +\t\t/* \\todo Check if EXIF IFD is correct here or not. */\n> It's messy. The TIFF/EP specification defines the TimeZoneOffset tag in\n> IFD0, so that's where I think it should go. The EXIF specification\n> defines three other tags (OffsetTime, OffsetTimeOriginal,\n> OffsetTimeDigitized), in the EXIF IFD, but those are not supported by\n> libexif. Should we mention the unsupported tags in the commit message ?\nSure. I think it would be worth it next time someone is touching this \npiece of code.\n>\n> Note that the DateTimeOriginal tag is defined by both the TIFF/EP and\n> EXIF specifications, but in different IFDs (IFD 0 for TIFF/EP and EXIF\n> IFD for EXIF). Lovely...\nIt's weird how these specs conflict when one (EXIF) is a reduced subset \nof another (TIFF/EP).\n>\n>> +\t\tsetSShort(EXIF_IFD_EXIF, EXIF_TAG_TIME_ZONE_OFFSET, hour);\n> The TIFF/EP specification defined TimeZoneOffset as containing up to two\n> signed short values. The first value relates to DateTime, the second\n> value to DateTimeOriginal. As we don't set DateTimeOriginal in IFD0 I\n> would leave the second value out, but if we added DateTimeOriginal in\n> IFD0 in addition to the EXIF IFD, I would add the second value.\nI agree with your analysis. Can I ask when/how would we add \nDateTimeOriginal in IFD0? I guess when we start supporting \nTIFF-compliant metadata via libtiff?\n\nAlso, this feels important decision/information. Should it be also \nconsidered to be mentioned as part of commit message?\n>\n>> +\t}\n>> }\n>> \n>> void Exif::setOrientation(int orientation)\n>> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h\n>> index 622de4c..4817815 100644\n>> --- a/src/android/jpeg/exif.h\n>> +++ b/src/android/jpeg/exif.h\n>> @@ -37,6 +37,7 @@ private:\n>> \t\t\t unsigned long components, unsigned int size);\n>> \n>> \tvoid setShort(ExifIfd ifd, ExifTag tag, uint16_t item);\n>> +\tvoid setSShort(ExifIfd ifd, ExifTag tag, int16_t item);\n>> \tvoid setLong(ExifIfd ifd, ExifTag tag, uint32_t item);\n>> \tvoid setString(ExifIfd ifd, ExifTag tag, ExifFormat format,\n>> \t\t const std::string &item);","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8D3B8C3B5B\n\tfor ;\n\tTue, 22 Sep 2020 11:43:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DFD8262FD8;\n\tTue, 22 Sep 2020 13:43:44 +0200 (CEST)","from mail.uajain.com (static.126.159.217.95.clients.your-server.de\n\t[95.217.159.126])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 918FF60366\n\tfor ;\n\tTue, 22 Sep 2020 13:43:43 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=uajain.com header.i=@uajain.com\n\theader.b=\"llcXQGiu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=uajain.com; s=mail;\n\tt=1600775023; bh=EbNroPwj8bk89r52h7hojXZtAy2wINg4HDKNPAPJykI=;\n\th=Subject:To:Cc:References:From:In-Reply-To;\n\tb=llcXQGiu9vlhkq0zcFGpEDah0lJiUGUbTJIrfiRInGjuuA//IautI1t9NEf6jEbHv\n\tpAH+NZHNFn1oRHx9wCQPxGUhzzcqSbFD4H/ZfSO4Co5kSGRb7FIRbtP6fIRTk8zw72\n\tG/r0w7/G4UX1Yxh+Pli5xCN5A19DFDB7WniMkqJRJGMT9lhbIGGhQsVTvPeNdpwmeE\n\t0NWaIZeK/RvIzNxol0f2clx406i8+nmf5Iuwgt2QXuMF/5DastRYXrTULvZ5doOfwb\n\tBAf5iJMas2B8/Fj3uZe/SP2UJtVuzCsmY1CiKhzFjrPqBDgVts+NNrjwEPLvhOU8PM\n\t5YfAukO6RFPHg==","To":"Laurent Pinchart ","References":"<20200909153206.7700-1-email@uajain.com>\n\t<20200910042547.GE4009@pendragon.ideasonboard.com>","From":"Umang Jain ","Message-ID":"","Date":"Tue, 22 Sep 2020 17:13:39 +0530","Mime-Version":"1.0","In-Reply-To":"<20200910042547.GE4009@pendragon.ideasonboard.com>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [RFC PATCH] android: jpeg: exif: Set timezone\n\tinformation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Transfer-Encoding":"7bit","Content-Type":"text/plain; charset=\"us-ascii\"; Format=\"flowed\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12640,"web_url":"https://patchwork.libcamera.org/comment/12640/","msgid":"<20200922122905.GI8290@pendragon.ideasonboard.com>","date":"2020-09-22T12:29:05","subject":"Re: [libcamera-devel] [RFC PATCH] android: jpeg: exif: Set timezone\n\tinformation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Tue, Sep 22, 2020 at 05:13:39PM +0530, Umang Jain wrote:\n> On 9/10/20 9:55 AM, Laurent Pinchart wrote:\n> > On Wed, Sep 09, 2020 at 09:02:06PM +0530, Umang Jain wrote:\n> >> Get timestamp information from the timestamp, although the resolution\n> >\n> > s/timestamp information/timezone information/\n> >\n> >> for EXIF_TAG_TIME_ZONE_OFFSET is fairly limtied (per-hour only).\n> >\n> > s/limtied/limited/\n> >\n> >> Experimentation with 'exiftool', commandline utility to read/write\n> >> exif metadata on images, resulted in rounding off the hours if the\n> >> minutes came out to >= 30. Hence, the behaviour is inspired from\n> >> exiftool itself. For e.g.,\n> >>\n> >> Timezone Tag's value\n> >> +1015 => 10\n> >> +0945 => 10\n> >> -1145 => -12\n> >\n> > Sounds good to me.\n> >\n> >> Signed-off-by: Umang Jain \n> >> ---\n> >> src/android/jpeg/exif.cpp | 30 ++++++++++++++++++++++++++++++\n> >> src/android/jpeg/exif.h | 1 +\n> >> 2 files changed, 31 insertions(+)\n> >>\n> >> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp\n> >> index 031f5f4..d8b4537 100644\n> >> --- a/src/android/jpeg/exif.cpp\n> >> +++ b/src/android/jpeg/exif.cpp\n> >> @@ -135,6 +135,16 @@ void Exif::setShort(ExifIfd ifd, ExifTag tag, uint16_t item)\n> >> \texif_entry_unref(entry);\n> >> }\n> >> \n> >> +void Exif::setSShort(ExifIfd ifd, ExifTag tag, int16_t item)\n> >> +{\n> >> +\tExifEntry *entry = createEntry(ifd, tag);\n> >> +\tif (!entry)\n> >> +\t\treturn;\n> >> +\n> >> +\texif_set_sshort(entry->data, EXIF_BYTE_ORDER_INTEL, item);\n> >> +\texif_entry_unref(entry);\n> >> +}\n> >> +\n> >> void Exif::setLong(ExifIfd ifd, ExifTag tag, uint32_t item)\n> >> {\n> >> \tExifEntry *entry = createEntry(ifd, tag);\n> >> @@ -194,6 +204,26 @@ void Exif::setTimestamp(time_t timestamp)\n> >> \tsetString(EXIF_IFD_0, EXIF_TAG_DATE_TIME, EXIF_FORMAT_ASCII, ts);\n> >> \tsetString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_ORIGINAL, EXIF_FORMAT_ASCII, ts);\n> >> \tsetString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_DIGITIZED, EXIF_FORMAT_ASCII, ts);\n> >> +\n> >> +\t/* If possible, query and set timezone information via\n> >\n> > \t/*\n> > \t * If possible ...\n> >\n> >> +\t * EXIF_TAG_TIME_ZONE_OFFSET. There is only per-hour resolution for tag\n> >> +\t * hence, round up hours if minutes >= 30.\n> >> +\t */\n> >> +\tchar tz[6];\n> >\n> > How about reusing the str variable ?\n> >\n> >> +\tint r = std::strftime(tz, sizeof(tz), \"%z\", std::localtime(×tamp));\n> >\n> > I've posted \"[PATCH] android: jpeg: exif: Use reentrant localtime_r()\"\n> > which adds a local tm variable that you can reuse instead of calling\n> > std::localtime() again.\n> >\n> >> +\tif (r > 0) {\n> >> +\t\tint16_t timezone = atoi(tz);\n> >\n> > You should include stdlib.h for atoi.\n> >\n> >> +\t\tint16_t hour = timezone / 100;\n> >> +\t\tint16_t min = abs(timezone) - (abs(hour) * 100);\n> >> +\n> >> +\t\tif (tz[0] == '-' && min >= 30)\n> >> +\t\t\thour--;\n> >> +\t\telse if (min >= 30)\n> >> +\t\t\thour++;\n> >\n> > An alternative would be\n> >\n> > \t\tint16_t timezone = atoi(tz);\n> > \t\tint16_t hour = timezone / 100;\n> > \t\tint16_t min = timezone % 100;\n> >\n> > \t\tif (min <= -30)\n> > \t\t\thour--;\n> > \t\telse if (min >= 30)\n> > \t\t\thour++;\n> >\n> > This relies on the fact that the modulo of a negative number will be a\n> > negative number.\n> >\n> >> +\n> >> +\t\t/* \\todo Check if EXIF IFD is correct here or not. */\n> >\n> > It's messy. The TIFF/EP specification defines the TimeZoneOffset tag in\n> > IFD0, so that's where I think it should go. The EXIF specification\n> > defines three other tags (OffsetTime, OffsetTimeOriginal,\n> > OffsetTimeDigitized), in the EXIF IFD, but those are not supported by\n> > libexif. Should we mention the unsupported tags in the commit message ?\n>\n> Sure. I think it would be worth it next time someone is touching this \n> piece of code.\n>\n> > Note that the DateTimeOriginal tag is defined by both the TIFF/EP and\n> > EXIF specifications, but in different IFDs (IFD 0 for TIFF/EP and EXIF\n> > IFD for EXIF). Lovely...\n>\n> It's weird how these specs conflict when one (EXIF) is a reduced subset \n> of another (TIFF/EP).\n\nStandardization is amazing, right ? :-)\n\n> >> +\t\tsetSShort(EXIF_IFD_EXIF, EXIF_TAG_TIME_ZONE_OFFSET, hour);\n> >\n> > The TIFF/EP specification defined TimeZoneOffset as containing up to two\n> > signed short values. The first value relates to DateTime, the second\n> > value to DateTimeOriginal. As we don't set DateTimeOriginal in IFD0 I\n> > would leave the second value out, but if we added DateTimeOriginal in\n> > IFD0 in addition to the EXIF IFD, I would add the second value.\n>\n> I agree with your analysis. Can I ask when/how would we add \n> DateTimeOriginal in IFD0? I guess when we start supporting \n> TIFF-compliant metadata via libtiff?\n\nThe EXIF specification is quite unclear here, it lists DateTimeOriginal\nas a tag in the EXIF IFD in table 7 (\"Exif IFD Attribute Information\" in\nsection 4.6.5), but also in table 18 (\"Tag Support Levels (2) - 0th IFD\nExif Private Tags\" in section 4.6.8). Setting DateTimeOriginal in IFD0\nin a TIFF/EP file is valid, setting it in a JFIF+EXIF file in IFD0 may\nor may not be valid.\n\nThe exif command line tool allows setting DateTimeOriginal in both IFD0\nand the EXIF IFD. It however prints the latter only, and doesn't find\nDateTimeOriginal in IFD0 when specifically asked for it. exiv2 will\nprint both.\n\nI think we can probably ignore this for now, keep DateTimeOriginal in\nthe EXIF IFD, with a single value in TimeZoneOffset.\n\n> Also, this feels important decision/information. Should it be also \n> considered to be mentioned as part of commit message?\n\nFeel free to record how desperate we are with the TIFF/EP and EXIF specs\nin the commit message :-)\n\n> >> +\t}\n> >> }\n> >> \n> >> void Exif::setOrientation(int orientation)\n> >> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h\n> >> index 622de4c..4817815 100644\n> >> --- a/src/android/jpeg/exif.h\n> >> +++ b/src/android/jpeg/exif.h\n> >> @@ -37,6 +37,7 @@ private:\n> >> \t\t\t unsigned long components, unsigned int size);\n> >> \n> >> \tvoid setShort(ExifIfd ifd, ExifTag tag, uint16_t item);\n> >> +\tvoid setSShort(ExifIfd ifd, ExifTag tag, int16_t item);\n> >> \tvoid setLong(ExifIfd ifd, ExifTag tag, uint32_t item);\n> >> \tvoid setString(ExifIfd ifd, ExifTag tag, ExifFormat format,\n> >> \t\t const std::string &item);","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 83EBBBF01C\n\tfor ;\n\tTue, 22 Sep 2020 12:29:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E8FE662FD4;\n\tTue, 22 Sep 2020 14:29:39 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 868AA60366\n\tfor ;\n\tTue, 22 Sep 2020 14:29:38 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 105562D7;\n\tTue, 22 Sep 2020 14:29:37 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"WDqqEa5B\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1600777778;\n\tbh=6LQ9vjbNVpj2btdjX5gpF6Gpj8ATIKjSSR29o8JZjVM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WDqqEa5B/1fHpbBjSYuSM4n6bhXz75HALrWoPCY9/QTYzdcFyFqI9eJuRo3oRPbBr\n\tCUbfdFkL4w6xqlL6JNItI8x14Bn7zsVG27jRsI9eFybsgnOUDetDAzOgzrPbp/aHAg\n\t0Tc6WOycuWKuKlOqvUsY28ZZGNzimKC/1MvP8+kA=","Date":"Tue, 22 Sep 2020 15:29:05 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"<20200922122905.GI8290@pendragon.ideasonboard.com>","References":"<20200909153206.7700-1-email@uajain.com>\n\t<20200910042547.GE4009@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH] android: jpeg: exif: Set timezone\n\tinformation","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]