[{"id":14550,"web_url":"https://patchwork.libcamera.org/comment/14550/","msgid":"<20210115150052.kph3twhnbl5tp5ek@uno.localdomain>","date":"2021-01-15T15:00:52","subject":"Re: [libcamera-devel] [PATCH 4/6] 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 Thu, Jan 14, 2021 at 07:40:33PM +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\nAwesome!\n\n> Signed-off-by: Paul Elder \n> ---\n> src/android/jpeg/exif.cpp | 187 ++++++++++++++++++++++++++++++++++++++\n> src/android/jpeg/exif.h | 41 +++++++++\n> 2 files changed, 228 insertions(+)\n>\n> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp\n> index b19cb4cd..fde07a63 100644\n> --- a/src/android/jpeg/exif.cpp\n> +++ b/src/android/jpeg/exif.cpp\n> @@ -7,6 +7,9 @@\n>\n> #include \"exif.h\"\n>\n> +#include \n> +#include \n> +\n> #include \"libcamera/internal/log.h\"\n> #include \"libcamera/internal/utils.h\"\n>\n> @@ -148,6 +151,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> +\tmemcpy(entry->data, &item, 1);\n\nI know nothing about the exif library, but I see\nexif_set_[short|long|rational] etc.. Isn't there an\nexif_set_[byte|char] ? If it's not there, there might be a reason why\nit has been left out ?\n\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> @@ -178,6 +191,22 @@ void Exif::setRational(ExifIfd ifd, ExifTag tag, ExifRational item)\n> \texif_entry_unref(entry);\n> }\n>\n> +/*\n> + * \\brief setArray\n\nVery brief!\nI think you can omit this doc block or rather expand it.\n\n> + * \\param[in] size sizeof(data[0])\n> + * \\param[in] count Number of elements in data\n> + */\n> +void Exif::setArray(ExifIfd ifd, ExifTag tag, ExifFormat format,\n> +\t\t const void *data, size_t size, size_t count)\n> +{\n> +\tExifEntry *entry = createEntry(ifd, tag, format, count, size * count);\n> +\tif (!entry)\n> +\t\treturn;\n> +\n> +\tmemcpy(entry->data, data, size * count);\n> +\texif_entry_unref(entry);\n> +}\n> +\n> void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::string &item)\n> {\n> \tstd::string ascii;\n> @@ -254,6 +283,111 @@ void Exif::setTimestamp(time_t timestamp)\n> \t}\n> }\n>\n> +void Exif::setGPSTimestamp(ExifIfd ifd, ExifTag tag, const struct tm &tm)\n> +{\n> +\tsize_t length = 3 * sizeof(ExifRational);\n> +\n> +\tExifEntry *entry = createEntry(ifd, tag, EXIF_FORMAT_RATIONAL, 3, length);\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\nIs this required to be rational ? the '1' here I assume is the\ndenominator..\n\n> +\n> +\tmemcpy(entry->data, ts, length);\n\nShouldn't this be setRational() ? Or is this to avoid calling it 3\ntimes ?\n\n> +\texif_entry_unref(entry);\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 ts(str);\n> +\n> +\tsetGPSTimestamp(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_TIME_STAMP), tm);\n> +\tsetString(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_DATE_STAMP), EXIF_FORMAT_ASCII, ts);\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\nI assume CTS validates the above calculations!\n\n> +\n> +void Exif::setGPSDMS(ExifIfd ifd, ExifTag tag, int deg, int min, int sec)\n> +{\n> +\tsize_t length = 3 * sizeof(ExifRational);\n> +\n> +\tExifEntry *entry = createEntry(ifd, tag, EXIF_FORMAT_RATIONAL, 3, length);\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> +\tmemcpy(entry->data, coords, length);\n> +\texif_entry_unref(entry);\n> +}\n> +\n> +/*\n> + * \\brief Set GPS location (lat, long, alt) from Android JPEG GPS coordinates\n\nWhere do we expect the GPS coordinates to come from ?\n\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 latDeg, latMin, latSec, longDeg, longMin, longSec;\n> +\n> +\tstd::tie(latDeg, latMin, latSec) = degreesToDMS(coords[0]);\n> +\tsetString(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_LATITUDE_REF),\n> +\t\t EXIF_FORMAT_ASCII, latDeg >= 0 ? \"N\" : \"S\");\n> +\tsetGPSDMS(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_LATITUDE),\n> +\t\t std::abs(latDeg), latMin, latSec);\n> +\n> +\tstd::tie(longDeg, longMin, longSec) = degreesToDMS(coords[1]);\n> +\tsetString(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_LONGITUDE_REF),\n> +\t\t EXIF_FORMAT_ASCII, longDeg >= 0 ? \"E\" : \"W\");\n> +\tsetGPSDMS(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_LATITUDE),\n> +\t\t std::abs(longDeg), longMin, longSec);\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> +\tstd::vector buf = libcamera::utils::string_to_c16(method, true);\n> +\t/* Designate that this string is Unicode (UCS-2) */\n> +\tbuf.insert(buf.begin(), { 0x55, 0x4E, 0x49, 0x43, 0x4F, 0x44, 0x45, 0x00 });\n> +\n\nThis will probable relocate, but it's not that bad I think\n\n> +\t/* 8 bytes for character code designation, plus 32 bytes from android */\n> +\tunsigned int nullTerm = 39;\n> +\tfor (int i = 8; i < buf.size(); i++) {\n> +\t\tif (!buf[i]) {\n> +\t\t\tnullTerm = i;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\tbuf.resize(nullTerm);\n\nWas I wrong assuming string_to_c16 stops at \\0 ?\n\n> +\n> +\tsetArray(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_PROCESSING_METHOD),\n> +\t\t EXIF_FORMAT_UNDEFINED, buf.data(), 1, buf.size());\n> +}\n> +\n> void Exif::setOrientation(int orientation)\n> {\n> \tint value;\n> @@ -288,6 +422,59 @@ void Exif::setThumbnail(Span thumbnail,\n> \tdata_->size = thumbnail.size();\n>\n> \tsetShort(EXIF_IFD_0, EXIF_TAG_COMPRESSION, compression);\n> +\tsetLong(EXIF_IFD_0, EXIF_TAG_JPEG_INTERCHANGE_FORMAT, 0);\n> +\tsetLong(EXIF_IFD_0, EXIF_TAG_JPEG_INTERCHANGE_FORMAT_LENGTH, data_->size);\n> +}\n> +\n> +void Exif::setFocalLength(float length)\n> +{\n> +\tExifRational rational = { static_cast(length), 1 };\n> +\tsetRational(EXIF_IFD_EXIF, EXIF_TAG_FOCAL_LENGTH, rational);\n> +}\n> +\n> +void Exif::setExposureTime(int64_t sec)\n> +{\n> +\tExifRational rational = { static_cast(sec), 1 };\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(int16_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, std::to_string(subsec));\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, 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, std::to_string(subsec));\n\nExif really requires a lot of information :/\n\nGood job overall!\n\n> }\n>\n> [[nodiscard]] int Exif::generate()\n> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h\n> index 5cab4559..cd3b78cd 100644\n> --- a/src/android/jpeg/exif.h\n> +++ b/src/android/jpeg/exif.h\n> @@ -26,6 +26,28 @@ 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\tStrobeReserved = 0x02,\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> \tvoid setMake(const std::string &make);\n> \tvoid setModel(const std::string &model);\n>\n> @@ -35,6 +57,19 @@ 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> +\tvoid setFocalLength(float length);\n> +\tvoid setExposureTime(int64_t sec);\n> +\tvoid setAperture(float size);\n> +\tvoid setISO(int16_t iso);\n> +\tvoid setFlash(Flash flash);\n> +\tvoid setWhiteBalance(WhiteBalance wb);\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> @@ -43,11 +78,17 @@ private:\n> \tExifEntry *createEntry(ExifIfd ifd, ExifTag tag, ExifFormat format,\n> \t\t\t unsigned long components, unsigned int size);\n>\n> +\tvoid setArray(ExifIfd ifd, ExifTag tag, ExifFormat format,\n> +\t\t const void *data, size_t size, size_t count);\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);\n> \tvoid setRational(ExifIfd ifd, ExifTag tag, ExifRational item);\n> +\tvoid setGPSTimestamp(ExifIfd ifd, ExifTag tag, const struct tm &tm);\n> +\tstd::tuple degreesToDMS(double decimalDegrees);\n> +\tvoid setGPSDMS(ExifIfd ifd, ExifTag tag, int deg, int min, int sec);\n>\n> \tbool valid_;\n>\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 BAC26BD808\n\tfor ;\n\tFri, 15 Jan 2021 15:00:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 53E6E680EB;\n\tFri, 15 Jan 2021 16:00:35 +0100 (CET)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 636D8680D1\n\tfor ;\n\tFri, 15 Jan 2021 16:00:34 +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 relay2-d.mail.gandi.net (Postfix) with ESMTPSA id B3AAA40012;\n\tFri, 15 Jan 2021 15:00:33 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Fri, 15 Jan 2021 16:00:52 +0100","From":"Jacopo Mondi ","To":"Paul Elder ","Message-ID":"<20210115150052.kph3twhnbl5tp5ek@uno.localdomain>","References":"<20210114104035.302968-1-paul.elder@ideasonboard.com>\n\t<20210114104035.302968-5-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210114104035.302968-5-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 4/6] 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":14556,"web_url":"https://patchwork.libcamera.org/comment/14556/","msgid":"","date":"2021-01-15T18:39:39","subject":"Re: [libcamera-devel] [PATCH 4/6] 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 Fri, Jan 15, 2021 at 04:00:52PM +0100, Jacopo Mondi wrote:\n> On Thu, Jan 14, 2021 at 07:40:33PM +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> Awesome!\n> \n> > Signed-off-by: Paul Elder \n> > ---\n> > src/android/jpeg/exif.cpp | 187 ++++++++++++++++++++++++++++++++++++++\n> > src/android/jpeg/exif.h | 41 +++++++++\n> > 2 files changed, 228 insertions(+)\n> >\n> > diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp\n> > index b19cb4cd..fde07a63 100644\n> > --- a/src/android/jpeg/exif.cpp\n> > +++ b/src/android/jpeg/exif.cpp\n> > @@ -7,6 +7,9 @@\n> >\n> > #include \"exif.h\"\n> >\n> > +#include \n> > +#include \n> > +\n> > #include \"libcamera/internal/log.h\"\n> > #include \"libcamera/internal/utils.h\"\n> >\n> > @@ -148,6 +151,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> > +\tmemcpy(entry->data, &item, 1);\n\n\tentry->data[0] = 1;\n\nshould do.\n\n> \n> I know nothing about the exif library, but I see\n> exif_set_[short|long|rational] etc.. Isn't there an\n> exif_set_[byte|char] ? If it's not there, there might be a reason why\n> it has been left out ?\n\nI think it's because writing entry->data[0] = value is simple enough.\nNot sure though.\n\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> > @@ -178,6 +191,22 @@ void Exif::setRational(ExifIfd ifd, ExifTag tag, ExifRational item)\n> > \texif_entry_unref(entry);\n> > }\n> >\n> > +/*\n> > + * \\brief setArray\n> \n> Very brief!\n> I think you can omit this doc block or rather expand it.\n> \n> > + * \\param[in] size sizeof(data[0])\n> > + * \\param[in] count Number of elements in data\n> > + */\n> > +void Exif::setArray(ExifIfd ifd, ExifTag tag, ExifFormat format,\n> > +\t\t const void *data, size_t size, size_t count)\n\nHmmm... You're using this to set strings, and we have a setString()\nmethod that handles ASCII strings already. It also supports the\n\"UNDEFINED\" Exif format, which is only used to set\nEXIF_TAG_EXIF_VERSION.\n\nWould it make sense to switch EXIF_TAG_EXIF_VERSION to Exif::setArray(),\nand modify setString() to handle the encoding ? When the format argument\nis set to EXIF_FORMAT_ASCII the setString() function would continue\noperating as it does today, and when it is set to EXIF_FORMAT_UNDEFINED\nit would encode the input string and add the character code prefix. A\nnew argument would need to be added to setString() to specify the\nencoding, although we could also default to Unicode (in the Exif sense,\nthus UTF-16) until a need to support other encodings arises.\n\n> > +{\n> > +\tExifEntry *entry = createEntry(ifd, tag, format, count, size * count);\n> > +\tif (!entry)\n> > +\t\treturn;\n> > +\n> > +\tmemcpy(entry->data, data, size * count);\n> > +\texif_entry_unref(entry);\n> > +}\n> > +\n> > void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::string &item)\n> > {\n> > \tstd::string ascii;\n> > @@ -254,6 +283,111 @@ void Exif::setTimestamp(time_t timestamp)\n> > \t}\n> > }\n> >\n> > +void Exif::setGPSTimestamp(ExifIfd ifd, ExifTag tag, const struct tm &tm)\n\nAs there are no other Exif tag than GPSTimeStamp that stores a time in\nthis format, I'd inline this function in its only caller.\n\n> > +{\n> > +\tsize_t length = 3 * sizeof(ExifRational);\n> > +\n> > +\tExifEntry *entry = createEntry(ifd, tag, EXIF_FORMAT_RATIONAL, 3, length);\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> Is this required to be rational ? the '1' here I assume is the\n> denominator..\n\nYes, that's what the Exif spec requires.\n\n> > +\n> > +\tmemcpy(entry->data, ts, length);\n> \n> Shouldn't this be setRational() ? Or is this to avoid calling it 3\n> times ?\n\nCalling setRational() 3 times would create 3 entries, we want a single\nentry with an array of 3 rational values. This should however call\nexif_set_rational() in a loop instead of using memcpy(), in order to\ndeal with different endianness. Same for Exif::setGPSDMS() below.\n\n> > +\texif_entry_unref(entry);\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 ts(str);\n> > +\n> > +\tsetGPSTimestamp(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_TIME_STAMP), tm);\n> > +\tsetString(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_DATE_STAMP), EXIF_FORMAT_ASCII, ts);\n\nHow about wrapping the second line ?\n\nExif is an amazing spec, not defining unique values for tags which\nresults in exif-tag.h using macros for the GPS tags, requiring a cast\n:-(\n\n> > +}\n> > +\n> > +std::tuple Exif::degreesToDMS(double decimalDegrees)\n\nI wonder if the second and third return values should be unsigned int,\nbut that's very minor.\n\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> I assume CTS validates the above calculations!\n> \n> > +\n> > +void Exif::setGPSDMS(ExifIfd ifd, ExifTag tag, int deg, int min, int sec)\n> > +{\n> > +\tsize_t length = 3 * sizeof(ExifRational);\n> > +\n> > +\tExifEntry *entry = createEntry(ifd, tag, EXIF_FORMAT_RATIONAL, 3, length);\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> > +\tmemcpy(entry->data, coords, length);\n> > +\texif_entry_unref(entry);\n> > +}\n> > +\n> > +/*\n> > + * \\brief Set GPS location (lat, long, alt) from Android JPEG GPS coordinates\n> \n> Where do we expect the GPS coordinates to come from ?\n\nAndroid provides it in the request. I wouldn't mention Android in the\ndocumentation though, from the point of view of the Exif class it\ndoesn't matter.\n\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\nMaybe const std::array ?\n\n> > +{\n> > +\tint latDeg, latMin, latSec, longDeg, longMin, longSec;\n\nThree variables should be enough, deg, min, sec, you can reuse them.\n\n> > +\n> > +\tstd::tie(latDeg, latMin, latSec) = degreesToDMS(coords[0]);\n> > +\tsetString(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_LATITUDE_REF),\n> > +\t\t EXIF_FORMAT_ASCII, latDeg >= 0 ? \"N\" : \"S\");\n> > +\tsetGPSDMS(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_LATITUDE),\n> > +\t\t std::abs(latDeg), latMin, latSec);\n> > +\n> > +\tstd::tie(longDeg, longMin, longSec) = degreesToDMS(coords[1]);\n> > +\tsetString(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_LONGITUDE_REF),\n> > +\t\t EXIF_FORMAT_ASCII, longDeg >= 0 ? \"E\" : \"W\");\n> > +\tsetGPSDMS(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_LATITUDE),\n> > +\t\t std::abs(longDeg), longMin, longSec);\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> > +\tstd::vector buf = libcamera::utils::string_to_c16(method, true);\n> > +\t/* Designate that this string is Unicode (UCS-2) */\n> > +\tbuf.insert(buf.begin(), { 0x55, 0x4E, 0x49, 0x43, 0x4F, 0x44, 0x45, 0x00 });\n\nLower-case hex constants please.\n\n> > +\n> \n> This will probable relocate, but it's not that bad I think\n> \n> > +\t/* 8 bytes for character code designation, plus 32 bytes from android */\n> > +\tunsigned int nullTerm = 39;\n> > +\tfor (int i = 8; i < buf.size(); i++) {\n> > +\t\tif (!buf[i]) {\n\nThis looks wrong, you'll stop at the first zero byte, which will very\nlikely be the very first byte as ascii characters are encoded in UTF-16\nwith one byte being zero.\n\nI think you should truncate the string while still in UTF-8, in the\ncaller, and encode the whole string here.\n\n> > +\t\t\tnullTerm = i;\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t}\n> > +\tbuf.resize(nullTerm);\n> \n> Was I wrong assuming string_to_c16 stops at \\0 ?\n> \n> > +\n> > +\tsetArray(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_PROCESSING_METHOD),\n> > +\t\t EXIF_FORMAT_UNDEFINED, buf.data(), 1, buf.size());\n> > +}\n> > +\n> > void Exif::setOrientation(int orientation)\n> > {\n> > \tint value;\n> > @@ -288,6 +422,59 @@ void Exif::setThumbnail(Span thumbnail,\n> > \tdata_->size = thumbnail.size();\n> >\n> > \tsetShort(EXIF_IFD_0, EXIF_TAG_COMPRESSION, compression);\n> > +\tsetLong(EXIF_IFD_0, EXIF_TAG_JPEG_INTERCHANGE_FORMAT, 0);\n\nThis should be the offset to the JPEG SOI marker, but the spec doesn't\ntell what the base address is used to compute the offset. Is 0 the right\nvalue ?\n\n> > +\tsetLong(EXIF_IFD_0, EXIF_TAG_JPEG_INTERCHANGE_FORMAT_LENGTH, data_->size);\n> > +}\n> > +\n> > +void Exif::setFocalLength(float length)\n> > +{\n> > +\tExifRational rational = { static_cast(length), 1 };\n\nDo you think sub-millimeter precision would be useful ?\n\n> > +\tsetRational(EXIF_IFD_EXIF, EXIF_TAG_FOCAL_LENGTH, rational);\n> > +}\n> > +\n> > +void Exif::setExposureTime(int64_t sec)\n\nNegative exposure times seem of limited use, and for this we definitely want sub-second precision. I'd pass a µs or ns argument.\n\n> > +{\n> > +\tExifRational rational = { static_cast(sec), 1 };\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\nMaybe this is a bit too much precision ?\n\n> > +\tsetRational(EXIF_IFD_EXIF, EXIF_TAG_FNUMBER, rational);\n> > +}\n> > +\n> > +void Exif::setISO(int16_t iso)\n\nThis can't be negative either. Maybe unsigned int ?\n\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, std::to_string(subsec));\n\nWhat's the unit of subsec ? You need to pad it with 0's on the left.\n\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, 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, std::to_string(subsec));\n> \n> Exif really requires a lot of information :/\n> \n> Good job overall!\n> \n> > }\n> >\n> > [[nodiscard]] int Exif::generate()\n> > diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h\n> > index 5cab4559..cd3b78cd 100644\n> > --- a/src/android/jpeg/exif.h\n> > +++ b/src/android/jpeg/exif.h\n> > @@ -26,6 +26,28 @@ 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\tStrobeReserved = 0x02,\n\nI'd drop this as it's reserved.\n\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> > \tvoid setMake(const std::string &make);\n> > \tvoid setModel(const std::string &model);\n> >\n> > @@ -35,6 +57,19 @@ 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\nWould it make sense to merge these three in a single function ?\n\nA blank line would be nice here.\n\n> > +\tvoid setFocalLength(float length);\n> > +\tvoid setExposureTime(int64_t sec);\n> > +\tvoid setAperture(float size);\n> > +\tvoid setISO(int16_t iso);\n> > +\tvoid setFlash(Flash flash);\n> > +\tvoid setWhiteBalance(WhiteBalance wb);\n\nA blank line would be nice here.\n\n> > +\tvoid setSubsecTime(uint64_t subsec);\n> > +\tvoid setSubsecTimeOriginal(uint64_t subsec);\n> > +\tvoid setSubsecTimeDigitized(uint64_t subsec);\n\nHow about moving these three functions to setTimestamp() ?\n\n> > +\n> > \tlibcamera::Span data() const { return { exifData_, size_ }; }\n> > \t[[nodiscard]] int generate();\n> >\n> > @@ -43,11 +78,17 @@ private:\n> > \tExifEntry *createEntry(ExifIfd ifd, ExifTag tag, ExifFormat format,\n> > \t\t\t unsigned long components, unsigned int size);\n> >\n> > +\tvoid setArray(ExifIfd ifd, ExifTag tag, ExifFormat format,\n> > +\t\t const void *data, size_t size, size_t count);\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);\n> > \tvoid setRational(ExifIfd ifd, ExifTag tag, ExifRational item);\n\nI'd add a blank line here.\n\n> > +\tvoid setGPSTimestamp(ExifIfd ifd, ExifTag tag, const struct tm &tm);\n> > +\tstd::tuple degreesToDMS(double decimalDegrees);\n> > +\tvoid setGPSDMS(ExifIfd ifd, ExifTag tag, int deg, int min, int sec);\n> >\n> > \tbool valid_;\n> >","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 E36BCBD808\n\tfor ;\n\tFri, 15 Jan 2021 18:39:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4F41A680F9;\n\tFri, 15 Jan 2021 19:39:58 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6418F680EF\n\tfor ;\n\tFri, 15 Jan 2021 19:39:57 +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 D8FC958B;\n\tFri, 15 Jan 2021 19:39:56 +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=\"X6rhVoON\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1610735997;\n\tbh=glXLL2TIOZWpaeBl31BQnoaKXJ61cWXOdFlOJqqiTNw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=X6rhVoONJHwWSx0GKfavL+6qQnqfbdzKjO5ZSNzFRbBRFmwAuMmC/l865tcNpPlFi\n\txTJIE22J9RCveG6K5aHAhAQPVelZZ8y0FpNKdAytPuhy4XWqzf5B5uIsC4gQinSmTO\n\tH4WhV1ho2X5wae9o/NU85EYXzGmuPlHehooFrECY=","Date":"Fri, 15 Jan 2021 20:39:39 +0200","From":"Laurent Pinchart ","To":"Paul Elder ","Message-ID":"","References":"<20210114104035.302968-1-paul.elder@ideasonboard.com>\n\t<20210114104035.302968-5-paul.elder@ideasonboard.com>\n\t<20210115150052.kph3twhnbl5tp5ek@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210115150052.kph3twhnbl5tp5ek@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 4/6] 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=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14626,"web_url":"https://patchwork.libcamera.org/comment/14626/","msgid":"<20210120100428.GB1943@pyrite.rasen.tech>","date":"2021-01-20T10:04:28","subject":"Re: [libcamera-devel] [PATCH 4/6] 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 Laurent,\n\nThank you for the review,\n\nOn Fri, Jan 15, 2021 at 08:39:39PM +0200, Laurent Pinchart wrote:\n> Hi Paul,\n> \n> Thank you for the patch.\n> \n> On Fri, Jan 15, 2021 at 04:00:52PM +0100, Jacopo Mondi wrote:\n> > On Thu, Jan 14, 2021 at 07:40:33PM +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> > Awesome!\n> > \n> > > Signed-off-by: Paul Elder \n> > > ---\n> > > src/android/jpeg/exif.cpp | 187 ++++++++++++++++++++++++++++++++++++++\n> > > src/android/jpeg/exif.h | 41 +++++++++\n> > > 2 files changed, 228 insertions(+)\n> > >\n> > > diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp\n> > > index b19cb4cd..fde07a63 100644\n> > > --- a/src/android/jpeg/exif.cpp\n> > > +++ b/src/android/jpeg/exif.cpp\n> > > @@ -7,6 +7,9 @@\n> > >\n> > > #include \"exif.h\"\n> > >\n> > > +#include \n> > > +#include \n> > > +\n> > > #include \"libcamera/internal/log.h\"\n> > > #include \"libcamera/internal/utils.h\"\n> > >\n> > > @@ -148,6 +151,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> > > +\tmemcpy(entry->data, &item, 1);\n> \n> \tentry->data[0] = 1;\n> \n> should do.\n> \n> > \n> > I know nothing about the exif library, but I see\n> > exif_set_[short|long|rational] etc.. Isn't there an\n> > exif_set_[byte|char] ? If it's not there, there might be a reason why\n> > it has been left out ?\n> \n> I think it's because writing entry->data[0] = value is simple enough.\n> Not sure though.\n> \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> > > @@ -178,6 +191,22 @@ void Exif::setRational(ExifIfd ifd, ExifTag tag, ExifRational item)\n> > > \texif_entry_unref(entry);\n> > > }\n> > >\n> > > +/*\n> > > + * \\brief setArray\n> > \n> > Very brief!\n> > I think you can omit this doc block or rather expand it.\n> > \n> > > + * \\param[in] size sizeof(data[0])\n> > > + * \\param[in] count Number of elements in data\n> > > + */\n> > > +void Exif::setArray(ExifIfd ifd, ExifTag tag, ExifFormat format,\n> > > +\t\t const void *data, size_t size, size_t count)\n> \n> Hmmm... You're using this to set strings, and we have a setString()\n> method that handles ASCII strings already. It also supports the\n> \"UNDEFINED\" Exif format, which is only used to set\n> EXIF_TAG_EXIF_VERSION.\n> \n> Would it make sense to switch EXIF_TAG_EXIF_VERSION to Exif::setArray(),\n> and modify setString() to handle the encoding ? When the format argument\n> is set to EXIF_FORMAT_ASCII the setString() function would continue\n> operating as it does today, and when it is set to EXIF_FORMAT_UNDEFINED\n> it would encode the input string and add the character code prefix. A\n> new argument would need to be added to setString() to specify the\n> encoding, although we could also default to Unicode (in the Exif sense,\n> thus UTF-16) until a need to support other encodings arises.\n\nAh, good idea.\n\n> > > +{\n> > > +\tExifEntry *entry = createEntry(ifd, tag, format, count, size * count);\n> > > +\tif (!entry)\n> > > +\t\treturn;\n> > > +\n> > > +\tmemcpy(entry->data, data, size * count);\n> > > +\texif_entry_unref(entry);\n> > > +}\n> > > +\n> > > void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::string &item)\n> > > {\n> > > \tstd::string ascii;\n> > > @@ -254,6 +283,111 @@ void Exif::setTimestamp(time_t timestamp)\n> > > \t}\n> > > }\n> > >\n> > > +void Exif::setGPSTimestamp(ExifIfd ifd, ExifTag tag, const struct tm &tm)\n> \n> As there are no other Exif tag than GPSTimeStamp that stores a time in\n> this format, I'd inline this function in its only caller.\n> \n> > > +{\n> > > +\tsize_t length = 3 * sizeof(ExifRational);\n> > > +\n> > > +\tExifEntry *entry = createEntry(ifd, tag, EXIF_FORMAT_RATIONAL, 3, length);\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> > Is this required to be rational ? the '1' here I assume is the\n> > denominator..\n> \n> Yes, that's what the Exif spec requires.\n> \n> > > +\n> > > +\tmemcpy(entry->data, ts, length);\n> > \n> > Shouldn't this be setRational() ? Or is this to avoid calling it 3\n> > times ?\n> \n> Calling setRational() 3 times would create 3 entries, we want a single\n> entry with an array of 3 rational values. This should however call\n> exif_set_rational() in a loop instead of using memcpy(), in order to\n> deal with different endianness. Same for Exif::setGPSDMS() below.\n\nAh yes.\n\n> > > +\texif_entry_unref(entry);\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 ts(str);\n> > > +\n> > > +\tsetGPSTimestamp(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_TIME_STAMP), tm);\n> > > +\tsetString(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_DATE_STAMP), EXIF_FORMAT_ASCII, ts);\n> \n> How about wrapping the second line ?\n> \n> Exif is an amazing spec, not defining unique values for tags which\n> results in exif-tag.h using macros for the GPS tags, requiring a cast\n> :-(\n\nYep :S\n\n> > > +}\n> > > +\n> > > +std::tuple Exif::degreesToDMS(double decimalDegrees)\n> \n> I wonder if the second and third return values should be unsigned int,\n> but that's very minor.\n> \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> > I assume CTS validates the above calculations!\n> > \n> > > +\n> > > +void Exif::setGPSDMS(ExifIfd ifd, ExifTag tag, int deg, int min, int sec)\n> > > +{\n> > > +\tsize_t length = 3 * sizeof(ExifRational);\n> > > +\n> > > +\tExifEntry *entry = createEntry(ifd, tag, EXIF_FORMAT_RATIONAL, 3, length);\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> > > +\tmemcpy(entry->data, coords, length);\n> > > +\texif_entry_unref(entry);\n> > > +}\n> > > +\n> > > +/*\n> > > + * \\brief Set GPS location (lat, long, alt) from Android JPEG GPS coordinates\n> > \n> > Where do we expect the GPS coordinates to come from ?\n> \n> Android provides it in the request. I wouldn't mention Android in the\n> documentation though, from the point of view of the Exif class it\n> doesn't matter.\n> \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> Maybe const std::array ?\n\nAndroid gives us a pointer to a double, how can I cast that to a\nstd::array?\n\n> > > +{\n> > > +\tint latDeg, latMin, latSec, longDeg, longMin, longSec;\n> \n> Three variables should be enough, deg, min, sec, you can reuse them.\n> \n> > > +\n> > > +\tstd::tie(latDeg, latMin, latSec) = degreesToDMS(coords[0]);\n> > > +\tsetString(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_LATITUDE_REF),\n> > > +\t\t EXIF_FORMAT_ASCII, latDeg >= 0 ? \"N\" : \"S\");\n> > > +\tsetGPSDMS(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_LATITUDE),\n> > > +\t\t std::abs(latDeg), latMin, latSec);\n> > > +\n> > > +\tstd::tie(longDeg, longMin, longSec) = degreesToDMS(coords[1]);\n> > > +\tsetString(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_LONGITUDE_REF),\n> > > +\t\t EXIF_FORMAT_ASCII, longDeg >= 0 ? \"E\" : \"W\");\n> > > +\tsetGPSDMS(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_LATITUDE),\n> > > +\t\t std::abs(longDeg), longMin, longSec);\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> > > +\tstd::vector buf = libcamera::utils::string_to_c16(method, true);\n> > > +\t/* Designate that this string is Unicode (UCS-2) */\n> > > +\tbuf.insert(buf.begin(), { 0x55, 0x4E, 0x49, 0x43, 0x4F, 0x44, 0x45, 0x00 });\n> \n> Lower-case hex constants please.\n> \n> > > +\n> > \n> > This will probable relocate, but it's not that bad I think\n> > \n> > > +\t/* 8 bytes for character code designation, plus 32 bytes from android */\n> > > +\tunsigned int nullTerm = 39;\n> > > +\tfor (int i = 8; i < buf.size(); i++) {\n> > > +\t\tif (!buf[i]) {\n> \n> This looks wrong, you'll stop at the first zero byte, which will very\n> likely be the very first byte as ascii characters are encoded in UTF-16\n> with one byte being zero.\n> \n> I think you should truncate the string while still in UTF-8, in the\n> caller, and encode the whole string here.\n\nWith the utf8ToUtf16 function this isn't necessary anymore.\n\n> > > +\t\t\tnullTerm = i;\n> > > +\t\t\tbreak;\n> > > +\t\t}\n> > > +\t}\n> > > +\tbuf.resize(nullTerm);\n> > \n> > Was I wrong assuming string_to_c16 stops at \\0 ?\n> > \n> > > +\n> > > +\tsetArray(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_PROCESSING_METHOD),\n> > > +\t\t EXIF_FORMAT_UNDEFINED, buf.data(), 1, buf.size());\n> > > +}\n> > > +\n> > > void Exif::setOrientation(int orientation)\n> > > {\n> > > \tint value;\n> > > @@ -288,6 +422,59 @@ void Exif::setThumbnail(Span thumbnail,\n> > > \tdata_->size = thumbnail.size();\n> > >\n> > > \tsetShort(EXIF_IFD_0, EXIF_TAG_COMPRESSION, compression);\n> > > +\tsetLong(EXIF_IFD_0, EXIF_TAG_JPEG_INTERCHANGE_FORMAT, 0);\n> \n> This should be the offset to the JPEG SOI marker, but the spec doesn't\n> tell what the base address is used to compute the offset. Is 0 the right\n> value ?\n\nUhhh I'm not sure. CTS isn't complaining with this.\n\n> > > +\tsetLong(EXIF_IFD_0, EXIF_TAG_JPEG_INTERCHANGE_FORMAT_LENGTH, data_->size);\n> > > +}\n> > > +\n> > > +void Exif::setFocalLength(float length)\n> > > +{\n> > > +\tExifRational rational = { static_cast(length), 1 };\n> \n> Do you think sub-millimeter precision would be useful ?\n\nOh, right :) android gives us mm anyway.\n\n> > > +\tsetRational(EXIF_IFD_EXIF, EXIF_TAG_FOCAL_LENGTH, rational);\n> > > +}\n> > > +\n> > > +void Exif::setExposureTime(int64_t sec)\n> \n> Negative exposure times seem of limited use, and for this we definitely want sub-second precision. I'd pass a µs or ns argument.\n\nWell android gives us an int64...\n\nBut yeah it should be ns.\n\n> > > +{\n> > > +\tExifRational rational = { static_cast(sec), 1 };\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> \n> Maybe this is a bit too much precision ?\n\nNo, this is fine. CTS wanted this much precision.\n\n> > > +\tsetRational(EXIF_IFD_EXIF, EXIF_TAG_FNUMBER, rational);\n> > > +}\n> > > +\n> > > +void Exif::setISO(int16_t iso)\n> \n> This can't be negative either. Maybe unsigned int ?\n\nYeah that's probably better.\n\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, std::to_string(subsec));\n> \n> What's the unit of subsec ? You need to pad it with 0's on the left.\n\nI don't see anything about padding to the left in the exif spec. I see\nstuff about padding to the right, but it's optional.\n\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, 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, std::to_string(subsec));\n> > \n> > Exif really requires a lot of information :/\n> > \n> > Good job overall!\n> > \n> > > }\n> > >\n> > > [[nodiscard]] int Exif::generate()\n> > > diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h\n> > > index 5cab4559..cd3b78cd 100644\n> > > --- a/src/android/jpeg/exif.h\n> > > +++ b/src/android/jpeg/exif.h\n> > > @@ -26,6 +26,28 @@ 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\tStrobeReserved = 0x02,\n> \n> I'd drop this as it's reserved.\n> \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> > > \tvoid setMake(const std::string &make);\n> > > \tvoid setModel(const std::string &model);\n> > >\n> > > @@ -35,6 +57,19 @@ 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> Would it make sense to merge these three in a single function ?\n\nI don't think so. Android gives them to use separately.\n\n> A blank line would be nice here.\n> \n> > > +\tvoid setFocalLength(float length);\n> > > +\tvoid setExposureTime(int64_t sec);\n> > > +\tvoid setAperture(float size);\n> > > +\tvoid setISO(int16_t iso);\n> > > +\tvoid setFlash(Flash flash);\n> > > +\tvoid setWhiteBalance(WhiteBalance wb);\n> \n> A blank line would be nice here.\n> \n> > > +\tvoid setSubsecTime(uint64_t subsec);\n> > > +\tvoid setSubsecTimeOriginal(uint64_t subsec);\n> > > +\tvoid setSubsecTimeDigitized(uint64_t subsec);\n> \n> How about moving these three functions to setTimestamp() ?\n>\n> > > +\n> > > \tlibcamera::Span data() const { return { exifData_, size_ }; }\n> > > \t[[nodiscard]] int generate();\n> > >\n> > > @@ -43,11 +78,17 @@ private:\n> > > \tExifEntry *createEntry(ExifIfd ifd, ExifTag tag, ExifFormat format,\n> > > \t\t\t unsigned long components, unsigned int size);\n> > >\n> > > +\tvoid setArray(ExifIfd ifd, ExifTag tag, ExifFormat format,\n> > > +\t\t const void *data, size_t size, size_t count);\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);\n> > > \tvoid setRational(ExifIfd ifd, ExifTag tag, ExifRational item);\n> \n> I'd add a blank line here.\n> \n> > > +\tvoid setGPSTimestamp(ExifIfd ifd, ExifTag tag, const struct tm &tm);\n> > > +\tstd::tuple degreesToDMS(double decimalDegrees);\n> > > +\tvoid setGPSDMS(ExifIfd ifd, ExifTag tag, int deg, int min, int sec);\n> > >\n> > > \tbool valid_;\n> > >\n\n\nThanks,\n\nPaul","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 4132EBD808\n\tfor ;\n\tWed, 20 Jan 2021 10:04:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1288C68194;\n\tWed, 20 Jan 2021 11:04:39 +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 CC9E768183\n\tfor ;\n\tWed, 20 Jan 2021 11:04:36 +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 0F759813;\n\tWed, 20 Jan 2021 11:04:34 +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=\"SWdsUXvH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1611137076;\n\tbh=t1YhiQeVzTeVodVnOh+cHw+ReV5gJ7tDph08ssRV/a4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SWdsUXvHRuQtRMPFtyq3xKQ5Fj3/DKAK8TK1CcjLdMDHwEBki4PB+IKv3oWJ1b4Pb\n\t3YcQmGBCUm2Sjl/WyONMzFwC39ZepDOhcCsAJYKLtWCpQDwXl3tUFPToWm80V9iTm5\n\tCzEsA1NGovNqenQT9UiPhSMfRyRK1kHYQBEfrk0c=","Date":"Wed, 20 Jan 2021 19:04:28 +0900","From":"paul.elder@ideasonboard.com","To":"Laurent Pinchart ","Message-ID":"<20210120100428.GB1943@pyrite.rasen.tech>","References":"<20210114104035.302968-1-paul.elder@ideasonboard.com>\n\t<20210114104035.302968-5-paul.elder@ideasonboard.com>\n\t<20210115150052.kph3twhnbl5tp5ek@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 4/6] 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=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14662,"web_url":"https://patchwork.libcamera.org/comment/14662/","msgid":"","date":"2021-01-21T20:02:49","subject":"Re: [libcamera-devel] [PATCH 4/6] 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\nOn Wed, Jan 20, 2021 at 07:04:28PM +0900, paul.elder@ideasonboard.com wrote:\n> On Fri, Jan 15, 2021 at 08:39:39PM +0200, Laurent Pinchart wrote:\n> > On Fri, Jan 15, 2021 at 04:00:52PM +0100, Jacopo Mondi wrote:\n> > > On Thu, Jan 14, 2021 at 07:40:33PM +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> > > Awesome!\n> > > \n> > > > Signed-off-by: Paul Elder \n> > > > ---\n> > > > src/android/jpeg/exif.cpp | 187 ++++++++++++++++++++++++++++++++++++++\n> > > > src/android/jpeg/exif.h | 41 +++++++++\n> > > > 2 files changed, 228 insertions(+)\n> > > >\n> > > > diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp\n> > > > index b19cb4cd..fde07a63 100644\n> > > > --- a/src/android/jpeg/exif.cpp\n> > > > +++ b/src/android/jpeg/exif.cpp\n> > > > @@ -7,6 +7,9 @@\n> > > >\n> > > > #include \"exif.h\"\n> > > >\n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > #include \"libcamera/internal/log.h\"\n> > > > #include \"libcamera/internal/utils.h\"\n> > > >\n> > > > @@ -148,6 +151,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> > > > +\tmemcpy(entry->data, &item, 1);\n> > \n> > \tentry->data[0] = 1;\n> > \n> > should do.\n> > \n> > > \n> > > I know nothing about the exif library, but I see\n> > > exif_set_[short|long|rational] etc.. Isn't there an\n> > > exif_set_[byte|char] ? If it's not there, there might be a reason why\n> > > it has been left out ?\n> > \n> > I think it's because writing entry->data[0] = value is simple enough.\n> > Not sure though.\n> > \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> > > > @@ -178,6 +191,22 @@ void Exif::setRational(ExifIfd ifd, ExifTag tag, ExifRational item)\n> > > > \texif_entry_unref(entry);\n> > > > }\n> > > >\n> > > > +/*\n> > > > + * \\brief setArray\n> > > \n> > > Very brief!\n> > > I think you can omit this doc block or rather expand it.\n> > > \n> > > > + * \\param[in] size sizeof(data[0])\n> > > > + * \\param[in] count Number of elements in data\n> > > > + */\n> > > > +void Exif::setArray(ExifIfd ifd, ExifTag tag, ExifFormat format,\n> > > > +\t\t const void *data, size_t size, size_t count)\n> > \n> > Hmmm... You're using this to set strings, and we have a setString()\n> > method that handles ASCII strings already. It also supports the\n> > \"UNDEFINED\" Exif format, which is only used to set\n> > EXIF_TAG_EXIF_VERSION.\n> > \n> > Would it make sense to switch EXIF_TAG_EXIF_VERSION to Exif::setArray(),\n> > and modify setString() to handle the encoding ? When the format argument\n> > is set to EXIF_FORMAT_ASCII the setString() function would continue\n> > operating as it does today, and when it is set to EXIF_FORMAT_UNDEFINED\n> > it would encode the input string and add the character code prefix. A\n> > new argument would need to be added to setString() to specify the\n> > encoding, although we could also default to Unicode (in the Exif sense,\n> > thus UTF-16) until a need to support other encodings arises.\n> \n> Ah, good idea.\n> \n> > > > +{\n> > > > +\tExifEntry *entry = createEntry(ifd, tag, format, count, size * count);\n> > > > +\tif (!entry)\n> > > > +\t\treturn;\n> > > > +\n> > > > +\tmemcpy(entry->data, data, size * count);\n> > > > +\texif_entry_unref(entry);\n> > > > +}\n> > > > +\n> > > > void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::string &item)\n> > > > {\n> > > > \tstd::string ascii;\n> > > > @@ -254,6 +283,111 @@ void Exif::setTimestamp(time_t timestamp)\n> > > > \t}\n> > > > }\n> > > >\n> > > > +void Exif::setGPSTimestamp(ExifIfd ifd, ExifTag tag, const struct tm &tm)\n> > \n> > As there are no other Exif tag than GPSTimeStamp that stores a time in\n> > this format, I'd inline this function in its only caller.\n> > \n> > > > +{\n> > > > +\tsize_t length = 3 * sizeof(ExifRational);\n> > > > +\n> > > > +\tExifEntry *entry = createEntry(ifd, tag, EXIF_FORMAT_RATIONAL, 3, length);\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> > > Is this required to be rational ? the '1' here I assume is the\n> > > denominator..\n> > \n> > Yes, that's what the Exif spec requires.\n> > \n> > > > +\n> > > > +\tmemcpy(entry->data, ts, length);\n> > > \n> > > Shouldn't this be setRational() ? Or is this to avoid calling it 3\n> > > times ?\n> > \n> > Calling setRational() 3 times would create 3 entries, we want a single\n> > entry with an array of 3 rational values. This should however call\n> > exif_set_rational() in a loop instead of using memcpy(), in order to\n> > deal with different endianness. Same for Exif::setGPSDMS() below.\n> \n> Ah yes.\n> \n> > > > +\texif_entry_unref(entry);\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 ts(str);\n> > > > +\n> > > > +\tsetGPSTimestamp(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_TIME_STAMP), tm);\n> > > > +\tsetString(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_DATE_STAMP), EXIF_FORMAT_ASCII, ts);\n> > \n> > How about wrapping the second line ?\n> > \n> > Exif is an amazing spec, not defining unique values for tags which\n> > results in exif-tag.h using macros for the GPS tags, requiring a cast\n> > :-(\n> \n> Yep :S\n> \n> > > > +}\n> > > > +\n> > > > +std::tuple Exif::degreesToDMS(double decimalDegrees)\n> > \n> > I wonder if the second and third return values should be unsigned int,\n> > but that's very minor.\n> > \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> > > I assume CTS validates the above calculations!\n> > > \n> > > > +\n> > > > +void Exif::setGPSDMS(ExifIfd ifd, ExifTag tag, int deg, int min, int sec)\n> > > > +{\n> > > > +\tsize_t length = 3 * sizeof(ExifRational);\n> > > > +\n> > > > +\tExifEntry *entry = createEntry(ifd, tag, EXIF_FORMAT_RATIONAL, 3, length);\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> > > > +\tmemcpy(entry->data, coords, length);\n> > > > +\texif_entry_unref(entry);\n> > > > +}\n> > > > +\n> > > > +/*\n> > > > + * \\brief Set GPS location (lat, long, alt) from Android JPEG GPS coordinates\n> > > \n> > > Where do we expect the GPS coordinates to come from ?\n> > \n> > Android provides it in the request. I wouldn't mention Android in the\n> > documentation though, from the point of view of the Exif class it\n> > doesn't matter.\n> > \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> > Maybe const std::array ?\n> \n> Android gives us a pointer to a double, how can I cast that to a\n> std::array?\n\nI think a reinterpret_cast may do, but it's dirty. Nevermind :-)\n\n> > > > +{\n> > > > +\tint latDeg, latMin, latSec, longDeg, longMin, longSec;\n> > \n> > Three variables should be enough, deg, min, sec, you can reuse them.\n> > \n> > > > +\n> > > > +\tstd::tie(latDeg, latMin, latSec) = degreesToDMS(coords[0]);\n> > > > +\tsetString(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_LATITUDE_REF),\n> > > > +\t\t EXIF_FORMAT_ASCII, latDeg >= 0 ? \"N\" : \"S\");\n> > > > +\tsetGPSDMS(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_LATITUDE),\n> > > > +\t\t std::abs(latDeg), latMin, latSec);\n> > > > +\n> > > > +\tstd::tie(longDeg, longMin, longSec) = degreesToDMS(coords[1]);\n> > > > +\tsetString(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_LONGITUDE_REF),\n> > > > +\t\t EXIF_FORMAT_ASCII, longDeg >= 0 ? \"E\" : \"W\");\n> > > > +\tsetGPSDMS(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_LATITUDE),\n> > > > +\t\t std::abs(longDeg), longMin, longSec);\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> > > > +\tstd::vector buf = libcamera::utils::string_to_c16(method, true);\n> > > > +\t/* Designate that this string is Unicode (UCS-2) */\n> > > > +\tbuf.insert(buf.begin(), { 0x55, 0x4E, 0x49, 0x43, 0x4F, 0x44, 0x45, 0x00 });\n> > \n> > Lower-case hex constants please.\n> > \n> > > > +\n> > > \n> > > This will probable relocate, but it's not that bad I think\n> > > \n> > > > +\t/* 8 bytes for character code designation, plus 32 bytes from android */\n> > > > +\tunsigned int nullTerm = 39;\n> > > > +\tfor (int i = 8; i < buf.size(); i++) {\n> > > > +\t\tif (!buf[i]) {\n> > \n> > This looks wrong, you'll stop at the first zero byte, which will very\n> > likely be the very first byte as ascii characters are encoded in UTF-16\n> > with one byte being zero.\n> > \n> > I think you should truncate the string while still in UTF-8, in the\n> > caller, and encode the whole string here.\n> \n> With the utf8ToUtf16 function this isn't necessary anymore.\n> \n> > > > +\t\t\tnullTerm = i;\n> > > > +\t\t\tbreak;\n> > > > +\t\t}\n> > > > +\t}\n> > > > +\tbuf.resize(nullTerm);\n> > > \n> > > Was I wrong assuming string_to_c16 stops at \\0 ?\n> > > \n> > > > +\n> > > > +\tsetArray(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_PROCESSING_METHOD),\n> > > > +\t\t EXIF_FORMAT_UNDEFINED, buf.data(), 1, buf.size());\n> > > > +}\n> > > > +\n> > > > void Exif::setOrientation(int orientation)\n> > > > {\n> > > > \tint value;\n> > > > @@ -288,6 +422,59 @@ void Exif::setThumbnail(Span thumbnail,\n> > > > \tdata_->size = thumbnail.size();\n> > > >\n> > > > \tsetShort(EXIF_IFD_0, EXIF_TAG_COMPRESSION, compression);\n> > > > +\tsetLong(EXIF_IFD_0, EXIF_TAG_JPEG_INTERCHANGE_FORMAT, 0);\n> > \n> > This should be the offset to the JPEG SOI marker, but the spec doesn't\n> > tell what the base address is used to compute the offset. Is 0 the right\n> > value ?\n> \n> Uhhh I'm not sure. CTS isn't complaining with this.\n\nLooking at the implementation in the IPU3 HAL, it has to be an offset.\n\n> > > > +\tsetLong(EXIF_IFD_0, EXIF_TAG_JPEG_INTERCHANGE_FORMAT_LENGTH, data_->size);\n> > > > +}\n> > > > +\n> > > > +void Exif::setFocalLength(float length)\n> > > > +{\n> > > > +\tExifRational rational = { static_cast(length), 1 };\n> > \n> > Do you think sub-millimeter precision would be useful ?\n> \n> Oh, right :) android gives us mm anyway.\n> \n> > > > +\tsetRational(EXIF_IFD_EXIF, EXIF_TAG_FOCAL_LENGTH, rational);\n> > > > +}\n> > > > +\n> > > > +void Exif::setExposureTime(int64_t sec)\n> > \n> > Negative exposure times seem of limited use, and for this we definitely want sub-second precision. I'd pass a µs or ns argument.\n> \n> Well android gives us an int64...\n> \n> But yeah it should be ns.\n> \n> > > > +{\n> > > > +\tExifRational rational = { static_cast(sec), 1 };\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> > \n> > Maybe this is a bit too much precision ?\n> \n> No, this is fine. CTS wanted this much precision.\n> \n> > > > +\tsetRational(EXIF_IFD_EXIF, EXIF_TAG_FNUMBER, rational);\n> > > > +}\n> > > > +\n> > > > +void Exif::setISO(int16_t iso)\n> > \n> > This can't be negative either. Maybe unsigned int ?\n> \n> Yeah that's probably better.\n> \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, std::to_string(subsec));\n> > \n> > What's the unit of subsec ? You need to pad it with 0's on the left.\n> \n> I don't see anything about padding to the left in the exif spec. I see\n> stuff about padding to the right, but it's optional.\n\nYou're right, my bad.\n\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, 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, std::to_string(subsec));\n> > > \n> > > Exif really requires a lot of information :/\n> > > \n> > > Good job overall!\n> > > \n> > > > }\n> > > >\n> > > > [[nodiscard]] int Exif::generate()\n> > > > diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h\n> > > > index 5cab4559..cd3b78cd 100644\n> > > > --- a/src/android/jpeg/exif.h\n> > > > +++ b/src/android/jpeg/exif.h\n> > > > @@ -26,6 +26,28 @@ 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\tStrobeReserved = 0x02,\n> > \n> > I'd drop this as it's reserved.\n> > \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> > > > \tvoid setMake(const std::string &make);\n> > > > \tvoid setModel(const std::string &model);\n> > > >\n> > > > @@ -35,6 +57,19 @@ 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> > Would it make sense to merge these three in a single function ?\n> \n> I don't think so. Android gives them to use separately.\n\nSure, but we don't expect only some of them to be provided, right ? Up\nto you.\n\n> > A blank line would be nice here.\n> > \n> > > > +\tvoid setFocalLength(float length);\n> > > > +\tvoid setExposureTime(int64_t sec);\n> > > > +\tvoid setAperture(float size);\n> > > > +\tvoid setISO(int16_t iso);\n> > > > +\tvoid setFlash(Flash flash);\n> > > > +\tvoid setWhiteBalance(WhiteBalance wb);\n> > \n> > A blank line would be nice here.\n> > \n> > > > +\tvoid setSubsecTime(uint64_t subsec);\n> > > > +\tvoid setSubsecTimeOriginal(uint64_t subsec);\n> > > > +\tvoid setSubsecTimeDigitized(uint64_t subsec);\n> > \n> > How about moving these three functions to setTimestamp() ?\n> >\n> > > > +\n> > > > \tlibcamera::Span data() const { return { exifData_, size_ }; }\n> > > > \t[[nodiscard]] int generate();\n> > > >\n> > > > @@ -43,11 +78,17 @@ private:\n> > > > \tExifEntry *createEntry(ExifIfd ifd, ExifTag tag, ExifFormat format,\n> > > > \t\t\t unsigned long components, unsigned int size);\n> > > >\n> > > > +\tvoid setArray(ExifIfd ifd, ExifTag tag, ExifFormat format,\n> > > > +\t\t const void *data, size_t size, size_t count);\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);\n> > > > \tvoid setRational(ExifIfd ifd, ExifTag tag, ExifRational item);\n> > \n> > I'd add a blank line here.\n> > \n> > > > +\tvoid setGPSTimestamp(ExifIfd ifd, ExifTag tag, const struct tm &tm);\n> > > > +\tstd::tuple degreesToDMS(double decimalDegrees);\n> > > > +\tvoid setGPSDMS(ExifIfd ifd, ExifTag tag, int deg, int min, int sec);\n> > > >\n> > > > \tbool valid_;\n> > > >","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 D48DFBD808\n\tfor ;\n\tThu, 21 Jan 2021 20:03:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5FE8868211;\n\tThu, 21 Jan 2021 21:03:10 +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 5E78568204\n\tfor ;\n\tThu, 21 Jan 2021 21:03:08 +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 C0A6B50E;\n\tThu, 21 Jan 2021 21:03:07 +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=\"R7kjuqkt\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1611259388;\n\tbh=Za3ASmQN8Dn9WqYUoX5FKNHcEkJOqbwdfsFMe3E5lX4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=R7kjuqktcAGNOJ7ziVu7aVuE17RplTm+RW3Xi0xHJ79ZOkfhICDBfhEnR+nJzXmNT\n\tzAZhcTO8S6SkR9FOx6ILyjrHfMAEXqDhCx0B/w3xzfmdoOSUuUt5sh2Y2YHIwGR/9z\n\tWjbmcywN0OVoflsTeOtmU3Lgcceyunm8zQ4/cQ4M=","Date":"Thu, 21 Jan 2021 22:02:49 +0200","From":"Laurent Pinchart ","To":"paul.elder@ideasonboard.com","Message-ID":"","References":"<20210114104035.302968-1-paul.elder@ideasonboard.com>\n\t<20210114104035.302968-5-paul.elder@ideasonboard.com>\n\t<20210115150052.kph3twhnbl5tp5ek@uno.localdomain>\n\t\n\t<20210120100428.GB1943@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210120100428.GB1943@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH 4/6] 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=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]