[{"id":14747,"web_url":"https://patchwork.libcamera.org/comment/14747/","msgid":"<20210125105156.jfogws2d55ye2fdi@uno.localdomain>","date":"2021-01-25T10:51:56","subject":"Re: [libcamera-devel] [PATCH v4 1/8] android: jpeg: exif: Expand\n\tsetString to support different encodings","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:37PM +0900, Paul Elder wrote:\n> GPSProcessingMethod and UserComment in EXIF tags can be in UTF-16.\n> Expand setString to take an encoding when the field type is undefined.\n> Update callers accordingly.\n>\n> Signed-off-by: Paul Elder \n> Reviewed-by: Laurent Pinchart \n>\n> ---\n> Changes in v4:\n> - clean up switch\n>\n> Changes in v3:\n> - use array to contain string encoding codes\n> - drop JIS and Undefined\n> - clean up setString a bit\n> - use the endianness of the exif\n>\n> Changes in v2:\n> - moved from utils into exif\n> - support no-encoding\n> ---\n> src/android/jpeg/exif.cpp | 77 +++++++++++++++++++++++++++++++++++++--\n> src/android/jpeg/exif.h | 11 +++++-\n> 2 files changed, 84 insertions(+), 4 deletions(-)\n>\n> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp\n> index 33b3fa7f..89343323 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> @@ -178,11 +181,18 @@ void Exif::setRational(ExifIfd ifd, ExifTag tag, ExifRational item)\n> \texif_entry_unref(entry);\n> }\n>\n> -void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::string &item)\n> +static const std::map> stringEncodingCodes = {\n> +\t{ Exif::ASCII, { 0x41, 0x53, 0x43, 0x49, 0x49, 0x00, 0x00, 0x00 } },\n> +\t{ Exif::Unicode, { 0x55, 0x4e, 0x49, 0x43, 0x4f, 0x44, 0x45, 0x00 } },\n> +};\n> +\n> +void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format,\n> +\t\t const std::string &item, StringEncoding encoding)\n> {\n> \tstd::string ascii;\n> \tsize_t length;\n> \tconst char *str;\n> +\tstd::vector buf;\n>\n> \tif (format == EXIF_FORMAT_ASCII) {\n> \t\tascii = utils::toAscii(item);\n> @@ -191,13 +201,46 @@ void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::str\n> \t\t/* Pad 1 extra byte to null-terminate the ASCII string. */\n> \t\tlength = ascii.length() + 1;\n> \t} else {\n> -\t\tstr = item.c_str();\n> +\t\tstd::u16string u16str;\n> +\n> +\t\tauto encodingString = stringEncodingCodes.find(encoding);\n> +\t\tif (encodingString != stringEncodingCodes.end()) {\n> +\t\t\tbuf = {\n> +\t\t\t\tencodingString->second.begin(),\n> +\t\t\t\tencodingString->second.end()\n> +\t\t\t};\n> +\t\t}\n> +\n> +\t\tswitch (encoding) {\n> +\t\tcase Unicode:\n\nI would have called Exif::UTF-16 and Exif::UTF-8 to avoid repeating a\ncheck for ASCII.\n\n if (format == ASCII) {\n\n } else {\n switch (encoding) {\n case Unicode:\n break;\n case Ascii:\n default:\n break\n }\n }\n\nVery minor though\n\n> +\t\t\tu16str = utf8ToUtf16(item);\n> +\n> +\t\t\tbuf.resize(8 + u16str.size() * 2);\n> +\t\t\tfor (size_t i = 0; i < u16str.size(); i++) {\n> +\t\t\t\tif (order_ == EXIF_BYTE_ORDER_INTEL) {\n> +\t\t\t\t\tbuf[8 + 2 * i] = u16str[i] & 0xff;\n> +\t\t\t\t\tbuf[8 + 2 * i + 1] = (u16str[i] >> 8) & 0xff;\n> +\t\t\t\t} else {\n> +\t\t\t\t\tbuf[8 + 2 * i] = (u16str[i] >> 8) & 0xff;\n> +\t\t\t\t\tbuf[8 + 2 * i + 1] = u16str[i] & 0xff;\n> +\t\t\t\t}\n> +\t\t\t}\n> +\n> +\t\t\tbreak;\n> +\n> +\t\tcase ASCII:\n> +\t\tcase NoEncoding:\n> +\t\t\tbuf.insert(buf.end(), item.begin(), item.end());\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\tstr = reinterpret_cast(buf.data());\n>\n> \t\t/*\n> \t\t * Strings stored in different formats (EXIF_FORMAT_UNDEFINED)\n> \t\t * are not null-terminated.\n> \t\t */\n\nIs this still true for Unicode and Ascii encoded strings ? Just\nchecking...\n\n> -\t\tlength = item.length();\n> +\t\tlength = buf.size();\n> \t}\n>\n> \tExifEntry *entry = createEntry(ifd, tag, format, length, length);\n> @@ -290,6 +333,34 @@ void Exif::setThumbnail(Span thumbnail,\n> \tsetShort(EXIF_IFD_0, EXIF_TAG_COMPRESSION, compression);\n> }\n>\n> +/**\n> + * \\brief Convert UTF-8 string to UTF-16 string\n> + * \\param[in] str String to convert\n> + *\n> + * \\return \\a str in UTF-16\n> + */\n> +std::u16string Exif::utf8ToUtf16(const std::string &str)\n> +{\n> +\tmbstate_t state{};\n> +\tchar16_t c16;\n> +\tconst char *ptr = str.data();\n> +\tconst char *end = ptr + str.size();\n> +\n> +\tstd::u16string ret;\n> +\twhile (size_t rc = mbrtoc16(&c16, ptr, end - ptr + 1, &state)) {\n> +\t\tif (rc == static_cast(-2) ||\n> +\t\t rc == static_cast(-1))\n> +\t\t\tbreak;\n> +\n> +\t\tret.push_back(c16);\n> +\n> +\t\tif (rc > 0)\n> +\t\t\tptr += rc;\n> +\t}\n\nI thought endianess was better handled here, but it has probably been\nchanged upon a review comment, so no problem...\n\nThe rest looks good!\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n\n> +\n> +\treturn ret;\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 5cab4559..8b84165b 100644\n> --- a/src/android/jpeg/exif.h\n> +++ b/src/android/jpeg/exif.h\n> @@ -26,6 +26,12 @@ public:\n> \t\tJPEG = 6,\n> \t};\n>\n> +\tenum StringEncoding {\n> +\t\tNoEncoding = 0,\n> +\t\tASCII = 1,\n> +\t\tUnicode = 2,\n> +\t};\n> +\n> \tvoid setMake(const std::string &make);\n> \tvoid setModel(const std::string &model);\n>\n> @@ -46,9 +52,12 @@ private:\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> +\t\t const std::string &item,\n> +\t\t StringEncoding encoding = NoEncoding);\n> \tvoid setRational(ExifIfd ifd, ExifTag tag, ExifRational item);\n>\n> +\tstd::u16string utf8ToUtf16(const std::string &str);\n> +\n> \tbool valid_;\n>\n> \tExifData *data_;\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 40D10BD808\n\tfor ;\n\tMon, 25 Jan 2021 10:51:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D984A682B9;\n\tMon, 25 Jan 2021 11:51:36 +0100 (CET)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F3C5C6030B\n\tfor ;\n\tMon, 25 Jan 2021 11:51:35 +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 relay1-d.mail.gandi.net (Postfix) with ESMTPSA id 83C8B240003;\n\tMon, 25 Jan 2021 10:51:35 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Mon, 25 Jan 2021 11:51:56 +0100","From":"Jacopo Mondi ","To":"Paul Elder ","Message-ID":"<20210125105156.jfogws2d55ye2fdi@uno.localdomain>","References":"<20210125071444.26252-1-paul.elder@ideasonboard.com>\n\t<20210125071444.26252-2-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210125071444.26252-2-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 1/8] android: jpeg: exif: Expand\n\tsetString to support different encodings","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":14750,"web_url":"https://patchwork.libcamera.org/comment/14750/","msgid":"","date":"2021-01-25T11:06:05","subject":"Re: [libcamera-devel] [PATCH v4 1/8] android: jpeg: exif: Expand\n\tsetString to support different encodings","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Jan 25, 2021 at 11:51:56AM +0100, Jacopo Mondi wrote:\n> On Mon, Jan 25, 2021 at 04:14:37PM +0900, Paul Elder wrote:\n> > GPSProcessingMethod and UserComment in EXIF tags can be in UTF-16.\n> > Expand setString to take an encoding when the field type is undefined.\n> > Update callers accordingly.\n> >\n> > Signed-off-by: Paul Elder \n> > Reviewed-by: Laurent Pinchart \n> >\n> > ---\n> > Changes in v4:\n> > - clean up switch\n> >\n> > Changes in v3:\n> > - use array to contain string encoding codes\n> > - drop JIS and Undefined\n> > - clean up setString a bit\n> > - use the endianness of the exif\n> >\n> > Changes in v2:\n> > - moved from utils into exif\n> > - support no-encoding\n> > ---\n> > src/android/jpeg/exif.cpp | 77 +++++++++++++++++++++++++++++++++++++--\n> > src/android/jpeg/exif.h | 11 +++++-\n> > 2 files changed, 84 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp\n> > index 33b3fa7f..89343323 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> > @@ -178,11 +181,18 @@ void Exif::setRational(ExifIfd ifd, ExifTag tag, ExifRational item)\n> > \texif_entry_unref(entry);\n> > }\n> >\n> > -void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::string &item)\n> > +static const std::map> stringEncodingCodes = {\n> > +\t{ Exif::ASCII, { 0x41, 0x53, 0x43, 0x49, 0x49, 0x00, 0x00, 0x00 } },\n> > +\t{ Exif::Unicode, { 0x55, 0x4e, 0x49, 0x43, 0x4f, 0x44, 0x45, 0x00 } },\n> > +};\n> > +\n> > +void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format,\n> > +\t\t const std::string &item, StringEncoding encoding)\n> > {\n> > \tstd::string ascii;\n> > \tsize_t length;\n> > \tconst char *str;\n> > +\tstd::vector buf;\n> >\n> > \tif (format == EXIF_FORMAT_ASCII) {\n> > \t\tascii = utils::toAscii(item);\n> > @@ -191,13 +201,46 @@ void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::str\n> > \t\t/* Pad 1 extra byte to null-terminate the ASCII string. */\n> > \t\tlength = ascii.length() + 1;\n> > \t} else {\n> > -\t\tstr = item.c_str();\n> > +\t\tstd::u16string u16str;\n> > +\n> > +\t\tauto encodingString = stringEncodingCodes.find(encoding);\n> > +\t\tif (encodingString != stringEncodingCodes.end()) {\n> > +\t\t\tbuf = {\n> > +\t\t\t\tencodingString->second.begin(),\n> > +\t\t\t\tencodingString->second.end()\n> > +\t\t\t};\n> > +\t\t}\n> > +\n> > +\t\tswitch (encoding) {\n> > +\t\tcase Unicode:\n> \n> I would have called Exif::UTF-16 and Exif::UTF-8 to avoid repeating a\n> check for ASCII.\n> \n> if (format == ASCII) {\n> \n> } else {\n> switch (encoding) {\n> case Unicode:\n> break;\n> case Ascii:\n> default:\n> break\n> }\n> }\n> \n> Very minor though\n\nExif is amazing, it has a field format that can be ASCII or UNDEFINED\n(\"undefined\" is an amazing format...). Tags in ASCII format 0-terminated\nand need to be encoded in ASCII, tags in UNDEFINED format are not\n0-terminated and can be either \"not encoded\" (which means there's no\nencoding information, as far as I remember this is only used for the\nExif version string, which is effectively ASCII), or encoded in\n\"UNICODE\", \"JIS\", \"ASCII\" or \"UNDEFINED\" (all these are \"defined\" as\npart of the Exif spec). The Ascii case below is the ASCII encoding\napplied to the UNDEFINED format, it's not UTF-8.\n\n> > +\t\t\tu16str = utf8ToUtf16(item);\n> > +\n> > +\t\t\tbuf.resize(8 + u16str.size() * 2);\n> > +\t\t\tfor (size_t i = 0; i < u16str.size(); i++) {\n> > +\t\t\t\tif (order_ == EXIF_BYTE_ORDER_INTEL) {\n> > +\t\t\t\t\tbuf[8 + 2 * i] = u16str[i] & 0xff;\n> > +\t\t\t\t\tbuf[8 + 2 * i + 1] = (u16str[i] >> 8) & 0xff;\n> > +\t\t\t\t} else {\n> > +\t\t\t\t\tbuf[8 + 2 * i] = (u16str[i] >> 8) & 0xff;\n> > +\t\t\t\t\tbuf[8 + 2 * i + 1] = u16str[i] & 0xff;\n> > +\t\t\t\t}\n> > +\t\t\t}\n> > +\n> > +\t\t\tbreak;\n> > +\n> > +\t\tcase ASCII:\n> > +\t\tcase NoEncoding:\n> > +\t\t\tbuf.insert(buf.end(), item.begin(), item.end());\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\n> > +\t\tstr = reinterpret_cast(buf.data());\n> >\n> > \t\t/*\n> > \t\t * Strings stored in different formats (EXIF_FORMAT_UNDEFINED)\n> > \t\t * are not null-terminated.\n> > \t\t */\n> \n> Is this still true for Unicode and Ascii encoded strings ? Just\n> checking...\n> \n> > -\t\tlength = item.length();\n> > +\t\tlength = buf.size();\n> > \t}\n> >\n> > \tExifEntry *entry = createEntry(ifd, tag, format, length, length);\n> > @@ -290,6 +333,34 @@ void Exif::setThumbnail(Span thumbnail,\n> > \tsetShort(EXIF_IFD_0, EXIF_TAG_COMPRESSION, compression);\n> > }\n> >\n> > +/**\n> > + * \\brief Convert UTF-8 string to UTF-16 string\n> > + * \\param[in] str String to convert\n> > + *\n> > + * \\return \\a str in UTF-16\n> > + */\n> > +std::u16string Exif::utf8ToUtf16(const std::string &str)\n> > +{\n> > +\tmbstate_t state{};\n> > +\tchar16_t c16;\n> > +\tconst char *ptr = str.data();\n> > +\tconst char *end = ptr + str.size();\n> > +\n> > +\tstd::u16string ret;\n> > +\twhile (size_t rc = mbrtoc16(&c16, ptr, end - ptr + 1, &state)) {\n> > +\t\tif (rc == static_cast(-2) ||\n> > +\t\t rc == static_cast(-1))\n> > +\t\t\tbreak;\n> > +\n> > +\t\tret.push_back(c16);\n> > +\n> > +\t\tif (rc > 0)\n> > +\t\t\tptr += rc;\n> > +\t}\n> \n> I thought endianess was better handled here, but it has probably been\n> changed upon a review comment, so no problem...\n\nThis function returns a std::u16string, not an array of bytes, so\nendianness can't be handled here.\n\n> The rest looks good!\n> Reviewed-by: Jacopo Mondi \n> \n> > +\n> > +\treturn ret;\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 5cab4559..8b84165b 100644\n> > --- a/src/android/jpeg/exif.h\n> > +++ b/src/android/jpeg/exif.h\n> > @@ -26,6 +26,12 @@ public:\n> > \t\tJPEG = 6,\n> > \t};\n> >\n> > +\tenum StringEncoding {\n> > +\t\tNoEncoding = 0,\n> > +\t\tASCII = 1,\n> > +\t\tUnicode = 2,\n> > +\t};\n> > +\n> > \tvoid setMake(const std::string &make);\n> > \tvoid setModel(const std::string &model);\n> >\n> > @@ -46,9 +52,12 @@ private:\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> > +\t\t const std::string &item,\n> > +\t\t StringEncoding encoding = NoEncoding);\n> > \tvoid setRational(ExifIfd ifd, ExifTag tag, ExifRational item);\n> >\n> > +\tstd::u16string utf8ToUtf16(const std::string &str);\n> > +\n> > \tbool valid_;\n> >\n> > \tExifData *data_;","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 4BB85BD808\n\tfor ;\n\tMon, 25 Jan 2021 11:06:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DA779682BE;\n\tMon, 25 Jan 2021 12:06:27 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B60316030E\n\tfor ;\n\tMon, 25 Jan 2021 12:06:25 +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 1331F331;\n\tMon, 25 Jan 2021 12:06:24 +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=\"skMS19j6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1611572784;\n\tbh=fURddb6EFk2sfShEanmTxz754CHwhbp6Pr5C8cL7w6E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=skMS19j6Hvg0pp1BjL1JAHrB05X31MusFi4dJ+3XIwubWne7wyJ1pnfytNHCaoDfv\n\t795pybRRYpopDCaP9ef2kMzMq2rraG+yw9QW/vmbT73uHzWTJgBGyXByQAKxy/VSu4\n\tw6qV/An33fod5Efpcr8O5eQa4l6hOMjgKBzYhgFI=","Date":"Mon, 25 Jan 2021 13:06:05 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210125071444.26252-1-paul.elder@ideasonboard.com>\n\t<20210125071444.26252-2-paul.elder@ideasonboard.com>\n\t<20210125105156.jfogws2d55ye2fdi@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210125105156.jfogws2d55ye2fdi@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v4 1/8] android: jpeg: exif: Expand\n\tsetString to support different encodings","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\" "}}]