[{"id":14740,"web_url":"https://patchwork.libcamera.org/comment/14740/","msgid":"","date":"2021-01-25T09:49:34","subject":"Re: [libcamera-devel] [PATCH v4 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 Mon, Jan 25, 2021 at 04:14:39PM +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 \n> Reviewed-by: Laurent Pinchart \n> \n> ---\n> Changes in v4:\n> - change type of subsec to std::chrono::milliseconds\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 | 178 +++++++++++++++++++++--\n> src/android/jpeg/exif.h | 39 ++++-\n> src/android/jpeg/post_processor_jpeg.cpp | 4 +-\n> 3 files changed, 205 insertions(+), 16 deletions(-)\n> \n> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp\n> index 204a237a..0f2c2d95 100644\n> --- a/src/android/jpeg/exif.cpp\n> +++ b/src/android/jpeg/exif.cpp\n> @@ -7,7 +7,10 @@\n> \n> #include \"exif.h\"\n> \n> +#include \n\nchrono is included in exif.h, you can drop it here.\n\n> +#include \n> #include \n> +#include \n> #include \n> \n> #include \"libcamera/internal/log.h\"\n> @@ -151,6 +154,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> @@ -267,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, std::chrono::milliseconds msec)\n> {\n> \tstruct tm tm;\n> \tlocaltime_r(×tamp, &tm);\n> @@ -282,19 +295,123 @@ 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::OFFSET_TIME),\n> -\t\t\t EXIF_FORMAT_ASCII, tz);\n> -\t\tsetString(EXIF_IFD_EXIF,\n> -\t\t\t static_cast(_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::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::OFFSET_TIME),\n> +\t\t EXIF_FORMAT_ASCII, tz);\n> +\tsetString(EXIF_IFD_EXIF,\n> +\t\t static_cast(_ExifTag::OFFSET_TIME_ORIGINAL),\n> +\t\t EXIF_FORMAT_ASCII, tz);\n> +\tsetString(EXIF_IFD_EXIF,\n> +\t\t static_cast(_ExifTag::OFFSET_TIME_DIGITIZED),\n> +\t\t EXIF_FORMAT_ASCII, tz);\n> +\n> +\tstd::string subsec = std::to_string(msec.count());\n\nI'm afraid you still need to pad on the left with zeros.\nhttps://en.cppreference.com/w/cpp/utility/format would be nice, but as\nwe can't depend on C++20, maybe\n\n\tstd::string subsec = \"00\" + std::to_string(msec.count());\n\tsubsec.erase(3);\n\nOr if it's too obscure, you can use the C++ string I/O with the format\nmodifiers, or just printf.\n\n> +\n> +\tsetString(EXIF_IFD_EXIF, EXIF_TAG_SUB_SEC_TIME,\n> +\t\t EXIF_FORMAT_ASCII, subsec);\n> +\tsetString(EXIF_IFD_EXIF, EXIF_TAG_SUB_SEC_TIME_ORIGINAL,\n> +\t\t EXIF_FORMAT_ASCII, subsec);\n> +\tsetString(EXIF_IFD_EXIF, EXIF_TAG_SUB_SEC_TIME_DIGITIZED,\n> +\t\t EXIF_FORMAT_ASCII, subsec);\n> +}\n> +\n> +void Exif::setGPSDateTimestamp(time_t timestamp)\n> +{\n> +\tstruct tm tm;\n> +\tgmtime_r(×tamp, &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(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(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(tm.tm_hour), 1 },\n> +\t\t{ static_cast(tm.tm_min), 1 },\n> +\t\t{ static_cast(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 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(deg), 1 },\n> +\t\t{ static_cast(min), 1 },\n> +\t\t{ static_cast(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(deg, min, sec) = degreesToDMS(coords[0]);\n> +\tsetString(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_LATITUDE_REF),\n> +\t\t EXIF_FORMAT_ASCII, deg >= 0 ? \"N\" : \"S\");\n> +\tsetGPSDMS(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_LATITUDE),\n> +\t\t std::abs(deg), min, sec);\n> +\n> +\tstd::tie(deg, min, sec) = degreesToDMS(coords[1]);\n> +\tsetString(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_LONGITUDE_REF),\n> +\t\t EXIF_FORMAT_ASCII, deg >= 0 ? \"E\" : \"W\");\n> +\tsetGPSDMS(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_LATITUDE),\n> +\t\t std::abs(deg), min, sec);\n> +\n> +\tsetByte(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_ALTITUDE_REF),\n> +\t\tcoords[2] >= 0 ? 0 : 1);\n> +\tsetRational(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_ALTITUDE),\n> +\t\t ExifRational{ static_cast(std::abs(coords[2])), 1 });\n> +}\n> +\n> +void Exif::setGPSMethod(const std::string &method)\n> +{\n> +\tsetString(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_PROCESSING_METHOD),\n> +\t\t EXIF_FORMAT_UNDEFINED, method, Unicode);\n> }\n> \n> void Exif::setOrientation(int orientation)\n> @@ -333,6 +450,39 @@ void Exif::setThumbnail(Span thumbnail,\n> \tsetShort(EXIF_IFD_0, EXIF_TAG_COMPRESSION, compression);\n> }\n> \n> +void Exif::setFocalLength(float length)\n> +{\n> +\tExifRational rational = { static_cast(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(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(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(flash));\n> +}\n> +\n> +void Exif::setWhiteBalance(WhiteBalance wb)\n> +{\n> +\tsetShort(EXIF_IFD_EXIF, EXIF_TAG_WHITE_BALANCE, static_cast(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 8b84165b..b0d61402 100644\n> --- a/src/android/jpeg/exif.h\n> +++ b/src/android/jpeg/exif.h\n> @@ -7,6 +7,7 @@\n> #ifndef __ANDROID_JPEG_EXIF_H__\n> #define __ANDROID_JPEG_EXIF_H__\n> \n> +#include \n> #include \n> #include \n> \n> @@ -26,6 +27,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 +61,18 @@ public:\n> \tvoid setSize(const libcamera::Size &size);\n> \tvoid setThumbnail(libcamera::Span thumbnail,\n> \t\t\t Compression compression);\n> -\tvoid setTimestamp(time_t timestamp);\n> +\tvoid setTimestamp(time_t timestamp, std::chrono::milliseconds msec);\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 data() const { return { exifData_, size_ }; }\n> \t[[nodiscard]] int generate();\n> @@ -49,6 +82,7 @@ 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> @@ -56,6 +90,9 @@ private:\n> \t\t StringEncoding encoding = NoEncoding);\n> \tvoid setRational(ExifIfd ifd, ExifTag tag, ExifRational item);\n> \n> +\tstd::tuple 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_;\n> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> index 436a50f8..c29cb352 100644\n> --- a/src/android/jpeg/post_processor_jpeg.cpp\n> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> @@ -7,6 +7,8 @@\n> \n> #include \"post_processor_jpeg.h\"\n> \n> +#include \n> +\n> #include \"../camera_device.h\"\n> #include \"../camera_metadata.h\"\n> #include \"encoder_libjpeg.h\"\n> @@ -97,7 +99,7 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n> \t * Since the precision we need for EXIF timestamp is only one\n> \t * second, it is good enough.\n> \t */\n> -\texif.setTimestamp(std::time(nullptr));\n> +\texif.setTimestamp(std::time(nullptr), std::chrono::milliseconds(0));\n\nDoes s/std::chrono::milliseconds(0)/0ms/ work\n(https://en.cppreference.com/w/cpp/chrono/operator%22%22ms) ? Feel free\nto pick the one you like best.\n\n> \n> \tstd::vector thumbnail;\n> \tgenerateThumbnail(source, &thumbnail);","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 0C1DAC0F2B\n\tfor ;\n\tMon, 25 Jan 2021 09:49:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8020F682AB;\n\tMon, 25 Jan 2021 10:49:55 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6D1F56030B\n\tfor ;\n\tMon, 25 Jan 2021 10:49:54 +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 E2C653E;\n\tMon, 25 Jan 2021 10:49:53 +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=\"Why4VY2J\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1611568194;\n\tbh=6Wo7k/HM0UkJ47vuQqnhba7DFR7qAn2hmD0GzVvNG04=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Why4VY2J52FKRITs9LhM7wnI4hYY54mt+d+Xh2bu3zaX1cFiXhgUE7Lp+3sCpg32H\n\t+EkPdCucdTuCp1FO/6eznE5FYRObddgowVcdywr70j/AUymdNvY7UBQO6RjPhYFGmT\n\teYP4Ryyq7E839ZS6ug1/lLuIalWc6Ouhhey20RPA=","Date":"Mon, 25 Jan 2021 11:49:34 +0200","From":"Laurent Pinchart ","To":"Paul Elder ","Message-ID":"","References":"<20210125071444.26252-1-paul.elder@ideasonboard.com>\n\t<20210125071444.26252-4-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210125071444.26252-4-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 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":"","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":14754,"web_url":"https://patchwork.libcamera.org/comment/14754/","msgid":"<20210125113026.l2rlxjfcjfybhda5@uno.localdomain>","date":"2021-01-25T11:30:26","subject":"Re: [libcamera-devel] [PATCH v4 3/8] android: jpeg: exif: Add\n\tfunctions for setting various values","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul,\n\nOn Mon, Jan 25, 2021 at 04:14:39PM +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 \n> Reviewed-by: Laurent Pinchart \n>\n\nSeems like I mostly have cosmetic comments\n\n> ---\n> Changes in v4:\n> - change type of subsec to std::chrono::milliseconds\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 | 178 +++++++++++++++++++++--\n> src/android/jpeg/exif.h | 39 ++++-\n> src/android/jpeg/post_processor_jpeg.cpp | 4 +-\n> 3 files changed, 205 insertions(+), 16 deletions(-)\n>\n> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp\n> index 204a237a..0f2c2d95 100644\n> --- a/src/android/jpeg/exif.cpp\n> +++ b/src/android/jpeg/exif.cpp\n> @@ -7,7 +7,10 @@\n>\n> #include \"exif.h\"\n>\n> +#include \n\nYou include in the header file in this patch. You can drop it\nhere.\n\n> +#include \n> #include \n> +#include \n> #include \n>\n> #include \"libcamera/internal/log.h\"\n> @@ -151,6 +154,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> @@ -267,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, std::chrono::milliseconds msec)\n> {\n> \tstruct tm tm;\n> \tlocaltime_r(×tamp, &tm);\n\nI guess the fact we use localtime there, and gmtime in gmtime in\nsetGPSDateTimestamp is intentional (you're not on GMT, so you would\nhave noticed if CTS does not like this)\n\n> @@ -282,19 +295,123 @@ 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::OFFSET_TIME),\n> -\t\t\t EXIF_FORMAT_ASCII, tz);\n> -\t\tsetString(EXIF_IFD_EXIF,\n> -\t\t\t static_cast(_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::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\nFor my education: why don't you need to create entries for these\nfields ?\n\n> +\t\t static_cast(_ExifTag::OFFSET_TIME),\n> +\t\t EXIF_FORMAT_ASCII, tz);\n> +\tsetString(EXIF_IFD_EXIF,\n> +\t\t static_cast(_ExifTag::OFFSET_TIME_ORIGINAL),\n> +\t\t EXIF_FORMAT_ASCII, tz);\n> +\tsetString(EXIF_IFD_EXIF,\n> +\t\t static_cast(_ExifTag::OFFSET_TIME_DIGITIZED),\n> +\t\t EXIF_FORMAT_ASCII, tz);\n> +\n> +\tstd::string subsec = std::to_string(msec.count());\n> +\n> +\tsetString(EXIF_IFD_EXIF, EXIF_TAG_SUB_SEC_TIME,\n> +\t\t EXIF_FORMAT_ASCII, subsec);\n> +\tsetString(EXIF_IFD_EXIF, EXIF_TAG_SUB_SEC_TIME_ORIGINAL,\n> +\t\t EXIF_FORMAT_ASCII, subsec);\n> +\tsetString(EXIF_IFD_EXIF, EXIF_TAG_SUB_SEC_TIME_DIGITIZED,\n> +\t\t EXIF_FORMAT_ASCII, subsec);\n> +}\n> +\n> +void Exif::setGPSDateTimestamp(time_t timestamp)\n> +{\n> +\tstruct tm tm;\n> +\tgmtime_r(×tamp, &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(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\nFits on the previous line\n\n> +\t\t\t static_cast(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(tm.tm_hour), 1 },\n> +\t\t{ static_cast(tm.tm_min), 1 },\n> +\t\t{ static_cast(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 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(deg), 1 },\n> +\t\t{ static_cast(min), 1 },\n> +\t\t{ static_cast(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\nIn the previous function you have an empty line here\n\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\nYou can align to the left\n\n> + */\n> +void Exif::setGPSLocation(const double *coords)\n> +{\n> +\tint deg, min, sec;\n> +\n> +\tstd::tie(deg, min, sec) = degreesToDMS(coords[0]);\n> +\tsetString(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_LATITUDE_REF),\n> +\t\t EXIF_FORMAT_ASCII, deg >= 0 ? \"N\" : \"S\");\n> +\tsetGPSDMS(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_LATITUDE),\n> +\t\t std::abs(deg), min, sec);\n> +\n> +\tstd::tie(deg, min, sec) = degreesToDMS(coords[1]);\n> +\tsetString(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_LONGITUDE_REF),\n> +\t\t EXIF_FORMAT_ASCII, deg >= 0 ? \"E\" : \"W\");\n> +\tsetGPSDMS(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_LATITUDE),\n> +\t\t std::abs(deg), min, sec);\n> +\n> +\tsetByte(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_ALTITUDE_REF),\n> +\t\tcoords[2] >= 0 ? 0 : 1);\n> +\tsetRational(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_ALTITUDE),\n> +\t\t ExifRational{ static_cast(std::abs(coords[2])), 1 });\n> +}\n> +\n> +void Exif::setGPSMethod(const std::string &method)\n> +{\n> +\tsetString(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_PROCESSING_METHOD),\n> +\t\t EXIF_FORMAT_UNDEFINED, method, Unicode);\n> }\n>\n> void Exif::setOrientation(int orientation)\n> @@ -333,6 +450,39 @@ void Exif::setThumbnail(Span thumbnail,\n> \tsetShort(EXIF_IFD_0, EXIF_TAG_COMPRESSION, compression);\n> }\n>\n> +void Exif::setFocalLength(float length)\n> +{\n> +\tExifRational rational = { static_cast(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(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(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(flash));\n> +}\n> +\n> +void Exif::setWhiteBalance(WhiteBalance wb)\n> +{\n> +\tsetShort(EXIF_IFD_EXIF, EXIF_TAG_WHITE_BALANCE, static_cast(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 8b84165b..b0d61402 100644\n> --- a/src/android/jpeg/exif.h\n> +++ b/src/android/jpeg/exif.h\n> @@ -7,6 +7,7 @@\n> #ifndef __ANDROID_JPEG_EXIF_H__\n> #define __ANDROID_JPEG_EXIF_H__\n>\n> +#include \n> #include \n> #include \n>\n> @@ -26,6 +27,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\nDo these come from the Exif spec ? are there no types defined for this\nalready ?\n\n> +\n> +\tenum WhiteBalance {\n> +\t\tAuto = 0,\n> +\t\tManual = 1,\n> +\t};\n> +\n\nSame question\n\nAs I've said, it's mostly cosmetic stuff\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> \tenum StringEncoding {\n> \t\tNoEncoding = 0,\n> \t\tASCII = 1,\n> @@ -39,7 +61,18 @@ public:\n> \tvoid setSize(const libcamera::Size &size);\n> \tvoid setThumbnail(libcamera::Span thumbnail,\n> \t\t\t Compression compression);\n> -\tvoid setTimestamp(time_t timestamp);\n> +\tvoid setTimestamp(time_t timestamp, std::chrono::milliseconds msec);\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 data() const { return { exifData_, size_ }; }\n> \t[[nodiscard]] int generate();\n> @@ -49,6 +82,7 @@ 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> @@ -56,6 +90,9 @@ private:\n> \t\t StringEncoding encoding = NoEncoding);\n> \tvoid setRational(ExifIfd ifd, ExifTag tag, ExifRational item);\n>\n> +\tstd::tuple 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_;\n> diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> index 436a50f8..c29cb352 100644\n> --- a/src/android/jpeg/post_processor_jpeg.cpp\n> +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> @@ -7,6 +7,8 @@\n>\n> #include \"post_processor_jpeg.h\"\n>\n> +#include \n> +\n> #include \"../camera_device.h\"\n> #include \"../camera_metadata.h\"\n> #include \"encoder_libjpeg.h\"\n> @@ -97,7 +99,7 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n> \t * Since the precision we need for EXIF timestamp is only one\n> \t * second, it is good enough.\n> \t */\n> -\texif.setTimestamp(std::time(nullptr));\n> +\texif.setTimestamp(std::time(nullptr), std::chrono::milliseconds(0));\n>\n> \tstd::vector thumbnail;\n> \tgenerateThumbnail(source, &thumbnail);\n> --\n> 2.27.0\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 D0ADDBD808\n\tfor ;\n\tMon, 25 Jan 2021 11:30:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 532F6682C0;\n\tMon, 25 Jan 2021 12:30:08 +0100 (CET)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DDB92682BB\n\tfor ;\n\tMon, 25 Jan 2021 12:30:06 +0100 (CET)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 485A71BF208;\n\tMon, 25 Jan 2021 11:30:06 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Mon, 25 Jan 2021 12:30:26 +0100","From":"Jacopo Mondi ","To":"Paul Elder ","Message-ID":"<20210125113026.l2rlxjfcjfybhda5@uno.localdomain>","References":"<20210125071444.26252-1-paul.elder@ideasonboard.com>\n\t<20210125071444.26252-4-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210125071444.26252-4-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 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":"","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":14768,"web_url":"https://patchwork.libcamera.org/comment/14768/","msgid":"<20210126023106.GB2215@pyrite.rasen.tech>","date":"2021-01-26T02:31:06","subject":"Re: [libcamera-devel] [PATCH v4 3/8] android: jpeg: exif: Add\n\tfunctions for setting various values","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Jan 25, 2021 at 12:30:26PM +0100, Jacopo Mondi wrote:\n> Hi Paul,\n> \n> On Mon, Jan 25, 2021 at 04:14:39PM +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 \n> > Reviewed-by: Laurent Pinchart \n> >\n> \n> Seems like I mostly have cosmetic comments\n> \n> > ---\n> > Changes in v4:\n> > - change type of subsec to std::chrono::milliseconds\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 | 178 +++++++++++++++++++++--\n> > src/android/jpeg/exif.h | 39 ++++-\n> > src/android/jpeg/post_processor_jpeg.cpp | 4 +-\n> > 3 files changed, 205 insertions(+), 16 deletions(-)\n> >\n> > diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp\n> > index 204a237a..0f2c2d95 100644\n> > --- a/src/android/jpeg/exif.cpp\n> > +++ b/src/android/jpeg/exif.cpp\n> > @@ -7,7 +7,10 @@\n> >\n> > #include \"exif.h\"\n> >\n> > +#include \n> \n> You include in the header file in this patch. You can drop it\n> here.\n> \n> > +#include \n> > #include \n> > +#include \n> > #include \n> >\n> > #include \"libcamera/internal/log.h\"\n> > @@ -151,6 +154,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> > @@ -267,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, std::chrono::milliseconds msec)\n> > {\n> > \tstruct tm tm;\n> > \tlocaltime_r(×tamp, &tm);\n> \n> I guess the fact we use localtime there, and gmtime in gmtime in\n> setGPSDateTimestamp is intentional (you're not on GMT, so you would\n> have noticed if CTS does not like this)\n\nYeah, the exif spec says that GPS timestamp is in UTC.\n\n> > @@ -282,19 +295,123 @@ 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::OFFSET_TIME),\n> > -\t\t\t EXIF_FORMAT_ASCII, tz);\n> > -\t\tsetString(EXIF_IFD_EXIF,\n> > -\t\t\t static_cast(_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::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> \n> For my education: why don't you need to create entries for these\n> fields ?\n\nWe do, setString handles that.\n\n> > +\t\t static_cast(_ExifTag::OFFSET_TIME),\n> > +\t\t EXIF_FORMAT_ASCII, tz);\n> > +\tsetString(EXIF_IFD_EXIF,\n> > +\t\t static_cast(_ExifTag::OFFSET_TIME_ORIGINAL),\n> > +\t\t EXIF_FORMAT_ASCII, tz);\n> > +\tsetString(EXIF_IFD_EXIF,\n> > +\t\t static_cast(_ExifTag::OFFSET_TIME_DIGITIZED),\n> > +\t\t EXIF_FORMAT_ASCII, tz);\n> > +\n> > +\tstd::string subsec = std::to_string(msec.count());\n> > +\n> > +\tsetString(EXIF_IFD_EXIF, EXIF_TAG_SUB_SEC_TIME,\n> > +\t\t EXIF_FORMAT_ASCII, subsec);\n> > +\tsetString(EXIF_IFD_EXIF, EXIF_TAG_SUB_SEC_TIME_ORIGINAL,\n> > +\t\t EXIF_FORMAT_ASCII, subsec);\n> > +\tsetString(EXIF_IFD_EXIF, EXIF_TAG_SUB_SEC_TIME_DIGITIZED,\n> > +\t\t EXIF_FORMAT_ASCII, subsec);\n> > +}\n> > +\n> > +void Exif::setGPSDateTimestamp(time_t timestamp)\n> > +{\n> > +\tstruct tm tm;\n> > +\tgmtime_r(×tamp, &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(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> \n> Fits on the previous line\n\nIf I do that then the next two lines will indent more and go over 80 :/\n\n> > +\t\t\t static_cast(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(tm.tm_hour), 1 },\n> > +\t\t{ static_cast(tm.tm_min), 1 },\n> > +\t\t{ static_cast(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 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(deg), 1 },\n> > +\t\t{ static_cast(min), 1 },\n> > +\t\t{ static_cast(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> \n> In the previous function you have an empty line here\n> \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> You can align to the left\n> \n> > + */\n> > +void Exif::setGPSLocation(const double *coords)\n> > +{\n> > +\tint deg, min, sec;\n> > +\n> > +\tstd::tie(deg, min, sec) = degreesToDMS(coords[0]);\n> > +\tsetString(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_LATITUDE_REF),\n> > +\t\t EXIF_FORMAT_ASCII, deg >= 0 ? \"N\" : \"S\");\n> > +\tsetGPSDMS(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_LATITUDE),\n> > +\t\t std::abs(deg), min, sec);\n> > +\n> > +\tstd::tie(deg, min, sec) = degreesToDMS(coords[1]);\n> > +\tsetString(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_LONGITUDE_REF),\n> > +\t\t EXIF_FORMAT_ASCII, deg >= 0 ? \"E\" : \"W\");\n> > +\tsetGPSDMS(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_LATITUDE),\n> > +\t\t std::abs(deg), min, sec);\n> > +\n> > +\tsetByte(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_ALTITUDE_REF),\n> > +\t\tcoords[2] >= 0 ? 0 : 1);\n> > +\tsetRational(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_ALTITUDE),\n> > +\t\t ExifRational{ static_cast(std::abs(coords[2])), 1 });\n> > +}\n> > +\n> > +void Exif::setGPSMethod(const std::string &method)\n> > +{\n> > +\tsetString(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_PROCESSING_METHOD),\n> > +\t\t EXIF_FORMAT_UNDEFINED, method, Unicode);\n> > }\n> >\n> > void Exif::setOrientation(int orientation)\n> > @@ -333,6 +450,39 @@ void Exif::setThumbnail(Span thumbnail,\n> > \tsetShort(EXIF_IFD_0, EXIF_TAG_COMPRESSION, compression);\n> > }\n> >\n> > +void Exif::setFocalLength(float length)\n> > +{\n> > +\tExifRational rational = { static_cast(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(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(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(flash));\n> > +}\n> > +\n> > +void Exif::setWhiteBalance(WhiteBalance wb)\n> > +{\n> > +\tsetShort(EXIF_IFD_EXIF, EXIF_TAG_WHITE_BALANCE, static_cast(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 8b84165b..b0d61402 100644\n> > --- a/src/android/jpeg/exif.h\n> > +++ b/src/android/jpeg/exif.h\n> > @@ -7,6 +7,7 @@\n> > #ifndef __ANDROID_JPEG_EXIF_H__\n> > #define __ANDROID_JPEG_EXIF_H__\n> >\n> > +#include \n> > #include \n> > #include \n> >\n> > @@ -26,6 +27,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> Do these come from the Exif spec ? are there no types defined for this\n> already ?\n\nYeah it does.\n\n> > +\n> > +\tenum WhiteBalance {\n> > +\t\tAuto = 0,\n> > +\t\tManual = 1,\n> > +\t};\n> > +\n> \n> Same question\n\nSame.\n\n> As I've said, it's mostly cosmetic stuff\n> \n> Reviewed-by: Jacopo Mondi \n\n\nThanks,\n\nPaul\n\n> \n> > \tenum StringEncoding {\n> > \t\tNoEncoding = 0,\n> > \t\tASCII = 1,\n> > @@ -39,7 +61,18 @@ public:\n> > \tvoid setSize(const libcamera::Size &size);\n> > \tvoid setThumbnail(libcamera::Span thumbnail,\n> > \t\t\t Compression compression);\n> > -\tvoid setTimestamp(time_t timestamp);\n> > +\tvoid setTimestamp(time_t timestamp, std::chrono::milliseconds msec);\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 data() const { return { exifData_, size_ }; }\n> > \t[[nodiscard]] int generate();\n> > @@ -49,6 +82,7 @@ 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> > @@ -56,6 +90,9 @@ private:\n> > \t\t StringEncoding encoding = NoEncoding);\n> > \tvoid setRational(ExifIfd ifd, ExifTag tag, ExifRational item);\n> >\n> > +\tstd::tuple 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_;\n> > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp\n> > index 436a50f8..c29cb352 100644\n> > --- a/src/android/jpeg/post_processor_jpeg.cpp\n> > +++ b/src/android/jpeg/post_processor_jpeg.cpp\n> > @@ -7,6 +7,8 @@\n> >\n> > #include \"post_processor_jpeg.h\"\n> >\n> > +#include \n> > +\n> > #include \"../camera_device.h\"\n> > #include \"../camera_metadata.h\"\n> > #include \"encoder_libjpeg.h\"\n> > @@ -97,7 +99,7 @@ int PostProcessorJpeg::process(const FrameBuffer &source,\n> > \t * Since the precision we need for EXIF timestamp is only one\n> > \t * second, it is good enough.\n> > \t */\n> > -\texif.setTimestamp(std::time(nullptr));\n> > +\texif.setTimestamp(std::time(nullptr), std::chrono::milliseconds(0));\n> >\n> > \tstd::vector thumbnail;\n> > \tgenerateThumbnail(source, &thumbnail);\n> > --\n> > 2.27.0\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel","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 63BBDC0F2B\n\tfor ;\n\tTue, 26 Jan 2021 02:36:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2A2B8682DE;\n\tTue, 26 Jan 2021 03:31:15 +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 A73166030A\n\tfor ;\n\tTue, 26 Jan 2021 03:31:14 +0100 (CET)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1D0B62C1;\n\tTue, 26 Jan 2021 03:31:12 +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=\"JYuednxk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1611628274;\n\tbh=XmeLibSZh/aYNQyWUWAsVWh/CxkI7gZOkgelsCGlFoc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=JYuednxkDhUJeYYjjd3ulmLghFdY64G2s4KkxFxx9tvjj+i+TdSGn42fyPiWVhDV6\n\tlUqeTttrlz3BCFdF4KBgtk7vSvkgZq5HXFMAanpnt0xtTgz/KkhWYHN7Cf+u886TnP\n\tnIkD9TY90XJo6fsSWHSeapQ44l/W/VniqefwEl5E=","Date":"Tue, 26 Jan 2021 11:31:06 +0900","From":"paul.elder@ideasonboard.com","To":"Jacopo Mondi ","Message-ID":"<20210126023106.GB2215@pyrite.rasen.tech>","References":"<20210125071444.26252-1-paul.elder@ideasonboard.com>\n\t<20210125071444.26252-4-paul.elder@ideasonboard.com>\n\t<20210125113026.l2rlxjfcjfybhda5@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210125113026.l2rlxjfcjfybhda5@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v4 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":"","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\" "}}]