[{"id":14663,"web_url":"https://patchwork.libcamera.org/comment/14663/","msgid":"","date":"2021-01-21T20:09:45","subject":"Re: [libcamera-devel] [PATCH v2 4/9] 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 Thu, Jan 21, 2021 at 07:15:44PM +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> \n> ---\n> Changes in v2:\n> - some cosmetic and precision changes\n> - use the new setString\n> ---\n> src/android/jpeg/exif.cpp | 159 ++++++++++++++++++++++++++++++++++++++\n> src/android/jpeg/exif.h | 40 ++++++++++\n> 2 files changed, 199 insertions(+)\n> \n> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp\n> index 8a185130..d3d0026a 100644\n> --- a/src/android/jpeg/exif.cpp\n> +++ b/src/android/jpeg/exif.cpp\n> @@ -7,8 +7,10 @@\n> \n> #include \"exif.h\"\n> \n> +#include \n> #include \n> #include \n> +#include \n\nLast time I checked T came before U :-) Didn't checkstyle.pl warn you ?\n\n> \n> #include \"libcamera/internal/log.h\"\n> #include \"libcamera/internal/utils.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> @@ -295,6 +307,102 @@ void Exif::setTimestamp(time_t timestamp)\n> \t}\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, NoEncoding, 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\nGPSDMS may be a bit cryptic, maybe setGPSCoordinate as it's setting a\nsingle coordinate ? Up to you.\n\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> + * \\todo use std::array\n\nstd::array isn't great, sorry for proposing it in the first place. It\nwould convey the fact that we expect three values, but casting to an\narray would be a bit dirty. We could use Span instead but that won't\nprovide much, so I think you can just drop the todo.\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, NoEncoding, 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, NoEncoding, 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, Unicode, method);\n> +}\n> +\n> void Exif::setOrientation(int orientation)\n> {\n> \tint value;\n> @@ -331,6 +439,45 @@ 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> +void Exif::setSubsecTime(uint64_t subsec)\n> +{\n> +\tsetString(EXIF_IFD_EXIF, EXIF_TAG_SUB_SEC_TIME,\n> +\t\t EXIF_FORMAT_ASCII, NoEncoding, std::to_string(subsec));\n> +}\n> +\n> /**\n> * \\brief Convert UTF-8 string to UTF-16 string\n> * \\param[in] str String to convert\n> @@ -359,6 +506,18 @@ std::u16string Exif::utf8ToUtf16(const std::string &str)\n> \treturn ret;\n> }\n> \n> +void Exif::setSubsecTimeOriginal(uint64_t subsec)\n> +{\n> +\tsetString(EXIF_IFD_EXIF, EXIF_TAG_SUB_SEC_TIME_ORIGINAL,\n> +\t\t EXIF_FORMAT_ASCII, NoEncoding, std::to_string(subsec));\n> +}\n> +\n> +void Exif::setSubsecTimeDigitized(uint64_t subsec)\n> +{\n> +\tsetString(EXIF_IFD_EXIF, EXIF_TAG_SUB_SEC_TIME_DIGITIZED,\n> +\t\t EXIF_FORMAT_ASCII, NoEncoding, std::to_string(subsec));\n> +}\n\nWouldn't this be better handled by adding a subsec parameter to\nsetTimestamp() ? setTimestamp() sets the three timestamps to the same\nvalue, so the subsec API should do the same.\n\nReviewed-by: Laurent Pinchart \n\n> +\n> [[nodiscard]] int Exif::generate()\n> {\n> \tif (exifData_) {\n> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h\n> index db98ba63..baab96ed 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> @@ -43,6 +64,21 @@ public:\n> \t\t\t Compression compression);\n> \tvoid setTimestamp(time_t timestamp);\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> +\tvoid setSubsecTime(uint64_t subsec);\n> +\tvoid setSubsecTimeOriginal(uint64_t subsec);\n> +\tvoid setSubsecTimeDigitized(uint64_t subsec);\n> +\n> \tlibcamera::Span data() const { return { exifData_, size_ }; }\n> \t[[nodiscard]] int generate();\n> \n> @@ -51,12 +87,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 StringEncoding enc, const std::string &item);\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_;","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 DD669C0F2A\n\tfor ;\n\tThu, 21 Jan 2021 20:10:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 67C4D68211;\n\tThu, 21 Jan 2021 21:10:06 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1950D68204\n\tfor ;\n\tThu, 21 Jan 2021 21:10:05 +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 8176450E;\n\tThu, 21 Jan 2021 21:10:04 +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=\"HGPFtJyP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1611259804;\n\tbh=MQasWfzqW/dktBsb5vxh98s7plL/6x2ud72vD9VS4t4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HGPFtJyPVg2TIXamApxsFEDM49k+W2IWghE89WIDc+AcPGQAb5NAZXz8+kOb17dIu\n\tJNCFx4m7wDXmOpc1P/eZl8FpAJP2WxAg/EiFHwhlS3v/KB8IC9hEqvnvKc0B1/9VBN\n\tkwxMl4O4X8glIOcaaMu/LkPt7uDENpM4vtAG6wrI=","Date":"Thu, 21 Jan 2021 22:09:45 +0200","From":"Laurent Pinchart ","To":"Paul Elder ","Message-ID":"","References":"<20210121101549.134574-1-paul.elder@ideasonboard.com>\n\t<20210121101549.134574-5-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210121101549.134574-5-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 4/9] 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\" "}}]