[{"id":14722,"web_url":"https://patchwork.libcamera.org/comment/14722/","msgid":"<YAvlMqi4nAc54cVR@pendragon.ideasonboard.com>","date":"2021-01-23T08:58:26","subject":"Re: [libcamera-devel] [PATCH v3 3/8] android: jpeg: exif: Add\n\tfunctions for setting various values","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Sat, Jan 23, 2021 at 02:16:59PM +0900, Paul Elder wrote:\n> Add functions for setting the following EXIF fields:\n> \n> - GPSDatestamp\n> - GPSTimestamp\n> - GPSLocation\n>   - GPSLatitudeRef\n>   - GPSLatitude\n>   - GPSLongitudeRef\n>   - GPSLongitude\n>   - GPSAltitudeRef\n>   - GPSAltitude\n> - GPSProcessingMethod\n> - FocalLength\n> - ExposureTime\n> - FNumber\n> - ISO\n> - Flash\n> - WhiteBalance\n> - SubsecTime\n> - SubsecTimeOriginal\n> - SubsecTimeDigitized\n> \n> These are in preparation for fixing the following CTS tests:\n> \n> - android.hardware.camera2.cts.StillCaptureTest#testFocalLengths\n> - android.hardware.camera2.cts.StillCaptureTest#testJpegExif\n> \n> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> \n> ---\n> Changes in v3:\n> - merge setting subsec into setting the main timestamp\n> \n> Changes in v2:\n> - some cosmetic and precision changes\n> - use the new setString\n> ---\n>  src/android/jpeg/exif.cpp | 175 +++++++++++++++++++++++++++++++++++---\n>  src/android/jpeg/exif.h   |  38 ++++++++-\n>  2 files changed, 198 insertions(+), 15 deletions(-)\n> \n> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp\n> index d4f86006..06cb4808 100644\n> --- a/src/android/jpeg/exif.cpp\n> +++ b/src/android/jpeg/exif.cpp\n> @@ -7,7 +7,9 @@\n>  \n>  #include \"exif.h\"\n>  \n> +#include <cmath>\n>  #include <map>\n> +#include <tuple>\n>  #include <uchar.h>\n>  \n>  #include \"libcamera/internal/log.h\"\n> @@ -151,6 +153,16 @@ ExifEntry *Exif::createEntry(ExifIfd ifd, ExifTag tag, ExifFormat format,\n>  \treturn entry;\n>  }\n>  \n> +void Exif::setByte(ExifIfd ifd, ExifTag tag, uint8_t item)\n> +{\n> +\tExifEntry *entry = createEntry(ifd, tag, EXIF_FORMAT_BYTE, 1, 1);\n> +\tif (!entry)\n> +\t\treturn;\n> +\n> +\tentry->data[0] = item;\n> +\texif_entry_unref(entry);\n> +}\n> +\n>  void Exif::setShort(ExifIfd ifd, ExifTag tag, uint16_t item)\n>  {\n>  \tExifEntry *entry = createEntry(ifd, tag);\n> @@ -268,7 +280,7 @@ void Exif::setSize(const Size &size)\n>  \tsetLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_X_DIMENSION, size.width);\n>  }\n>  \n> -void Exif::setTimestamp(time_t timestamp)\n> +void Exif::setTimestamp(time_t timestamp, uint64_t subsec)\n\nCould we rename subsec to state the unit explicitly ? nsecs if it's\nexpressed in nanoseconds for instance ? And as I assume you won't need a\nhigher precision than nanoseconds, uint32_t should be enough.\n\nAnother option would be to use\n\nvoid Exif::setTimestamp(time_t timestamp, std::chrono::nanoseconds nsecs)\n\nfor additional type safety. You can then call the function with a ns\nliteral (see https://en.cppreference.com/w/cpp/chrono/operator%22%22ns):\n\n\texif.setTimestamp(..., 0ns);\n\nCalling it with\n\n\texif.setTimestamp(..., 0);\n\nwould work, as the corresponding std::chrono::duration is explicit.\n\nDoes CTS require any specific precision ? If not, nanoseconds may be a\nbit too much possibly ? Up to you.\n\n>  {\n>  \tstruct tm tm;\n>  \tlocaltime_r(&timestamp, &tm);\n> @@ -283,19 +295,121 @@ void Exif::setTimestamp(time_t timestamp)\n>  \n>  \t/* Query and set timezone information if available. */\n>  \tint r = strftime(str, sizeof(str), \"%z\", &tm);\n> -\tif (r > 0) {\n> -\t\tstd::string tz(str);\n> -\t\ttz.insert(3, 1, ':');\n> -\t\tsetString(EXIF_IFD_EXIF,\n> -\t\t\t  static_cast<ExifTag>(_ExifTag::OFFSET_TIME),\n> -\t\t\t  EXIF_FORMAT_ASCII, tz);\n> -\t\tsetString(EXIF_IFD_EXIF,\n> -\t\t\t  static_cast<ExifTag>(_ExifTag::OFFSET_TIME_ORIGINAL),\n> -\t\t\t  EXIF_FORMAT_ASCII, tz);\n> -\t\tsetString(EXIF_IFD_EXIF,\n> -\t\t\t  static_cast<ExifTag>(_ExifTag::OFFSET_TIME_DIGITIZED),\n> -\t\t\t  EXIF_FORMAT_ASCII, tz);\n> -\t}\n> +\tif (r <= 0)\n> +\t\treturn;\n> +\n> +\tstd::string tz(str);\n> +\ttz.insert(3, 1, ':');\n> +\tsetString(EXIF_IFD_EXIF,\n> +\t\t  static_cast<ExifTag>(_ExifTag::OFFSET_TIME),\n> +\t\t  EXIF_FORMAT_ASCII, tz);\n> +\tsetString(EXIF_IFD_EXIF,\n> +\t\t  static_cast<ExifTag>(_ExifTag::OFFSET_TIME_ORIGINAL),\n> +\t\t  EXIF_FORMAT_ASCII, tz);\n> +\tsetString(EXIF_IFD_EXIF,\n> +\t\t  static_cast<ExifTag>(_ExifTag::OFFSET_TIME_DIGITIZED),\n> +\t\t  EXIF_FORMAT_ASCII, tz);\n> +\n\n\tstd::string subsec = std::to_string(nsecs);\n\nto avoid computing for every call ?\n\nI got confused previously, but I think you still need to pad wih 0's on\nthe left. The example given in the Exif documentation encodes September\n9, 1998, 9:15:30.130 as \"1998:09:01 09:15:30\\0\" for DateTime (quite\nobviously incorrect btw, the date should be 1998:09:09), and \"130\\0\" for\nSubsecTime. The unit isn't specified, but assuming milliseconds to match\nthe example, 09:15:30.012 should then be encoded as \"012\\0\" in\nSubsecTime, while the call to std:to_string() will turn 12 into \"12\\0\".\n\n> +\tsetString(EXIF_IFD_EXIF, EXIF_TAG_SUB_SEC_TIME,\n> +\t\t  EXIF_FORMAT_ASCII, std::to_string(subsec));\n> +\tsetString(EXIF_IFD_EXIF, EXIF_TAG_SUB_SEC_TIME_ORIGINAL,\n> +\t\t  EXIF_FORMAT_ASCII, std::to_string(subsec));\n> +\tsetString(EXIF_IFD_EXIF, EXIF_TAG_SUB_SEC_TIME_DIGITIZED,\n> +\t\t  EXIF_FORMAT_ASCII, std::to_string(subsec));\n> +}\n> +\n> +void Exif::setGPSDateTimestamp(time_t timestamp)\n> +{\n> +\tstruct tm tm;\n> +\tgmtime_r(&timestamp, &tm);\n> +\n> +\tchar str[11];\n> +\tstrftime(str, sizeof(str), \"%Y:%m:%d\", &tm);\n> +\tstd::string tsStr(str);\n> +\n> +\tsetString(EXIF_IFD_GPS, static_cast<ExifTag>(EXIF_TAG_GPS_DATE_STAMP),\n> +\t\t  EXIF_FORMAT_ASCII, tsStr);\n> +\n> +\t/* Set GPS_TIME_STAMP */\n> +\tExifEntry *entry =\n> +\t\tcreateEntry(EXIF_IFD_GPS,\n> +\t\t\t    static_cast<ExifTag>(EXIF_TAG_GPS_TIME_STAMP),\n> +\t\t\t    EXIF_FORMAT_RATIONAL, 3, 3 * sizeof(ExifRational));\n> +\tif (!entry)\n> +\t\treturn;\n> +\n> +\tExifRational ts[] = {\n> +\t\t{ static_cast<ExifLong>(tm.tm_hour), 1 },\n> +\t\t{ static_cast<ExifLong>(tm.tm_min),  1 },\n> +\t\t{ static_cast<ExifLong>(tm.tm_sec),  1 },\n> +\t};\n> +\n> +\tfor (int i = 0; i < 3; i++)\n> +\t\texif_set_rational(entry->data + i * sizeof(ExifRational),\n> +\t\t\t\t  order_, ts[i]);\n> +\n> +\texif_entry_unref(entry);\n> +}\n> +\n> +std::tuple<int, int, int> Exif::degreesToDMS(double decimalDegrees)\n> +{\n> +\tint degrees = std::trunc(decimalDegrees);\n> +\tdouble minutes = std::abs((decimalDegrees - degrees) * 60);\n> +\tdouble seconds = (minutes - std::trunc(minutes)) * 60;\n> +\n> +\treturn { degrees, std::trunc(minutes), std::round(seconds) };\n> +}\n> +\n> +void Exif::setGPSDMS(ExifIfd ifd, ExifTag tag, int deg, int min, int sec)\n> +{\n> +\tExifEntry *entry = createEntry(ifd, tag, EXIF_FORMAT_RATIONAL, 3,\n> +\t\t\t\t       3 * sizeof(ExifRational));\n> +\tif (!entry)\n> +\t\treturn;\n> +\n> +\tExifRational coords[] = {\n> +\t\t{ static_cast<ExifLong>(deg), 1 },\n> +\t\t{ static_cast<ExifLong>(min), 1 },\n> +\t\t{ static_cast<ExifLong>(sec), 1 },\n> +\t};\n> +\n> +\tfor (int i = 0; i < 3; i++)\n> +\t\texif_set_rational(entry->data + i * sizeof(ExifRational),\n> +\t\t\t\t  order_, coords[i]);\n> +\texif_entry_unref(entry);\n> +}\n> +\n> +/*\n> + * \\brief Set GPS location (lat, long, alt)\n> + * \\param[in] coords Pointer to coordinates latitude, longitude, and altitude,\n> + *            first two in degrees, the third in meters\n> + */\n> +void Exif::setGPSLocation(const double *coords)\n> +{\n> +\tint deg, min, sec;\n> +\n> +\tstd::tie<int, int, int>(deg, min, sec) = degreesToDMS(coords[0]);\n> +\tsetString(EXIF_IFD_GPS, static_cast<ExifTag>(EXIF_TAG_GPS_LATITUDE_REF),\n> +\t\t  EXIF_FORMAT_ASCII, deg >= 0 ? \"N\" : \"S\");\n> +\tsetGPSDMS(EXIF_IFD_GPS, static_cast<ExifTag>(EXIF_TAG_GPS_LATITUDE),\n> +\t\t  std::abs(deg), min, sec);\n> +\n> +\tstd::tie<int, int, int>(deg, min, sec) = degreesToDMS(coords[1]);\n> +\tsetString(EXIF_IFD_GPS, static_cast<ExifTag>(EXIF_TAG_GPS_LONGITUDE_REF),\n> +\t\t  EXIF_FORMAT_ASCII, deg >= 0 ? \"E\" : \"W\");\n> +\tsetGPSDMS(EXIF_IFD_GPS, static_cast<ExifTag>(EXIF_TAG_GPS_LATITUDE),\n> +\t\t  std::abs(deg), min, sec);\n> +\n> +\tsetByte(EXIF_IFD_GPS, static_cast<ExifTag>(EXIF_TAG_GPS_ALTITUDE_REF),\n> +\t\tcoords[2] >= 0 ? 0 : 1);\n> +\tsetRational(EXIF_IFD_GPS, static_cast<ExifTag>(EXIF_TAG_GPS_ALTITUDE),\n> +\t\t    ExifRational{ static_cast<ExifLong>(std::abs(coords[2])), 1 });\n> +}\n> +\n> +void Exif::setGPSMethod(const std::string &method)\n> +{\n> +\tsetString(EXIF_IFD_GPS, static_cast<ExifTag>(EXIF_TAG_GPS_PROCESSING_METHOD),\n> +\t\t  EXIF_FORMAT_UNDEFINED, method, Unicode);\n>  }\n>  \n>  void Exif::setOrientation(int orientation)\n> @@ -334,6 +448,39 @@ void Exif::setThumbnail(Span<const unsigned char> thumbnail,\n>  \tsetShort(EXIF_IFD_0, EXIF_TAG_COMPRESSION, compression);\n>  }\n>  \n> +void Exif::setFocalLength(float length)\n> +{\n> +\tExifRational rational = { static_cast<ExifLong>(length * 1000), 1000 };\n> +\tsetRational(EXIF_IFD_EXIF, EXIF_TAG_FOCAL_LENGTH, rational);\n> +}\n> +\n> +void Exif::setExposureTime(uint64_t nsec)\n> +{\n> +\tExifRational rational = { static_cast<ExifLong>(nsec), 1000000000 };\n> +\tsetRational(EXIF_IFD_EXIF, EXIF_TAG_EXPOSURE_TIME, rational);\n> +}\n> +\n> +void Exif::setAperture(float size)\n> +{\n> +\tExifRational rational = { static_cast<ExifLong>(size * 10000), 10000 };\n> +\tsetRational(EXIF_IFD_EXIF, EXIF_TAG_FNUMBER, rational);\n> +}\n> +\n> +void Exif::setISO(uint16_t iso)\n> +{\n> +\tsetShort(EXIF_IFD_EXIF, EXIF_TAG_ISO_SPEED_RATINGS, iso);\n> +}\n> +\n> +void Exif::setFlash(Flash flash)\n> +{\n> +\tsetShort(EXIF_IFD_EXIF, EXIF_TAG_FLASH, static_cast<ExifShort>(flash));\n> +}\n> +\n> +void Exif::setWhiteBalance(WhiteBalance wb)\n> +{\n> +\tsetShort(EXIF_IFD_EXIF, EXIF_TAG_WHITE_BALANCE, static_cast<ExifShort>(wb));\n> +}\n> +\n>  /**\n>   * \\brief Convert UTF-8 string to UTF-16 string\n>   * \\param[in] str String to convert\n> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h\n> index 73f231b2..7d8727ad 100644\n> --- a/src/android/jpeg/exif.h\n> +++ b/src/android/jpeg/exif.h\n> @@ -26,6 +26,27 @@ public:\n>  \t\tJPEG = 6,\n>  \t};\n>  \n> +\tenum Flash {\n> +\t\t/* bit 0 */\n> +\t\tFired = 0x01,\n> +\t\t/* bits 1 and 2 */\n> +\t\tStrobeDetected = 0x04,\n> +\t\tStrobeNotDetected = 0x06,\n> +\t\t/* bits 3 and 4 */\n> +\t\tModeCompulsoryFiring = 0x08,\n> +\t\tModeCompulsorySuppression = 0x10,\n> +\t\tModeAuto = 0x18,\n> +\t\t/* bit 5 */\n> +\t\tFlashNotPresent = 0x20,\n> +\t\t/* bit 6 */\n> +\t\tRedEye = 0x40,\n> +\t};\n> +\n> +\tenum WhiteBalance {\n> +\t\tAuto = 0,\n> +\t\tManual = 1,\n> +\t};\n> +\n>  \tenum StringEncoding {\n>  \t\tNoEncoding = 0,\n>  \t\tASCII = 1,\n> @@ -39,7 +60,18 @@ public:\n>  \tvoid setSize(const libcamera::Size &size);\n>  \tvoid setThumbnail(libcamera::Span<const unsigned char> thumbnail,\n>  \t\t\t  Compression compression);\n> -\tvoid setTimestamp(time_t timestamp);\n> +\tvoid setTimestamp(time_t timestamp, uint64_t subsec);\n\nShouldn't you also update the caller in this patch ?\n\n> +\n> +\tvoid setGPSDateTimestamp(time_t timestamp);\n> +\tvoid setGPSLocation(const double *coords);\n> +\tvoid setGPSMethod(const std::string &method);\n> +\n> +\tvoid setFocalLength(float length);\n> +\tvoid setExposureTime(uint64_t nsec);\n> +\tvoid setAperture(float size);\n> +\tvoid setISO(uint16_t iso);\n> +\tvoid setFlash(Flash flash);\n> +\tvoid setWhiteBalance(WhiteBalance wb);\n>  \n>  \tlibcamera::Span<const uint8_t> data() const { return { exifData_, size_ }; }\n>  \t[[nodiscard]] int generate();\n> @@ -49,12 +81,16 @@ private:\n>  \tExifEntry *createEntry(ExifIfd ifd, ExifTag tag, ExifFormat format,\n>  \t\t\t       unsigned long components, unsigned int size);\n>  \n> +\tvoid setByte(ExifIfd ifd, ExifTag tag, uint8_t item);\n>  \tvoid setShort(ExifIfd ifd, ExifTag tag, uint16_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, StringEncoding encoding = NoEncoding);\n>  \tvoid setRational(ExifIfd ifd, ExifTag tag, ExifRational item);\n>  \n> +\tstd::tuple<int, int, int> degreesToDMS(double decimalDegrees);\n> +\tvoid setGPSDMS(ExifIfd ifd, ExifTag tag, int deg, int min, int sec);\n> +\n>  \tstd::u16string utf8ToUtf16(const std::string &str);\n>  \n>  \tbool valid_;","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 85628C0F2B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSat, 23 Jan 2021 08:58:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 21A8B68286;\n\tSat, 23 Jan 2021 09:58:50 +0100 (CET)","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 BC14A6010B\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 23 Jan 2021 09:58:47 +0100 (CET)","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 12B7C8C7;\n\tSat, 23 Jan 2021 09:58:46 +0100 (CET)"],"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=\"YAJD5ZvB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1611392327;\n\tbh=S1MFrMNXbyPMhR49gbFF3At4iBMDE5LPE55/g6pprdI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YAJD5ZvBm2SQYLgQQmAfSB9w+QhvSSXBNNW3Tol/z6BZ1EKcAdImSo/95SURwkoTu\n\tUueC8lNbXFPmHhQcvJ3yPMR1z0qrBnO2XYFOTak+5cII8Mh+mHK3FA3av22954PPaY\n\t1QViSWICAZd8dwCjbY4PrgHwEZ6Ab6/B3/F/+qZQ=","Date":"Sat, 23 Jan 2021 10:58:26 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Paul Elder <paul.elder@ideasonboard.com>","Message-ID":"<YAvlMqi4nAc54cVR@pendragon.ideasonboard.com>","References":"<20210123051704.188117-1-paul.elder@ideasonboard.com>\n\t<20210123051704.188117-4-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210123051704.188117-4-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 3/8] android: jpeg: exif: Add\n\tfunctions for setting various values","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","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\" <libcamera-devel-bounces@lists.libcamera.org>"}}]