[{"id":15504,"web_url":"https://patchwork.libcamera.org/comment/15504/","msgid":"<3246ec4a-f065-cecc-23a8-e476f60e2d38@ideasonboard.com>","date":"2021-03-08T10:46:42","subject":"Re: [libcamera-devel] [PATCH 2/4] android: jpeg: exif: Simplify\n\tsetGPSDateTimestamp and setGPSDMS","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 08/03/2021 10:13, Paul Elder wrote:\n> Now that setRational() supports setting multiple rational values, use\n> that in setGPSDateTimestamp and setGPSDMS which previously set every\n> rational manually.\n\nThis feels better to me ;-)\n\n> Signed-off-by: Paul Elder \n> ---\n> src/android/jpeg/exif.cpp | 24 ++----------------------\n> 1 file changed, 2 insertions(+), 22 deletions(-)\n> \n> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp\n> index e5a3e448..c8b2f072 100644\n> --- a/src/android/jpeg/exif.cpp\n> +++ b/src/android/jpeg/exif.cpp\n> @@ -345,24 +345,13 @@ void Exif::setGPSDateTimestamp(time_t timestamp)\n> \t\t EXIF_FORMAT_ASCII, tsStr);\n> \n> \t/* Set GPS_TIME_STAMP */\n> -\tExifEntry *entry =\n> -\t\tcreateEntry(EXIF_IFD_GPS,\n> -\t\t\t static_cast(EXIF_TAG_GPS_TIME_STAMP),\n> -\t\t\t EXIF_FORMAT_RATIONAL, 3, 3 * sizeof(ExifRational));\n> -\tif (!entry)\n> -\t\treturn;\n> -\n> \tExifRational ts[] = {\n> \t\t{ static_cast(tm.tm_hour), 1 },\n> \t\t{ static_cast(tm.tm_min), 1 },\n> \t\t{ static_cast(tm.tm_sec), 1 },\n> \t};\n> \n> -\tfor (int i = 0; i < 3; i++)\n> -\t\texif_set_rational(entry->data + i * sizeof(ExifRational),\n> -\t\t\t\t order_, ts[i]);\n> -\n> -\texif_entry_unref(entry);\n> +\tsetRational(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_TIME_STAMP), 3, ts);\n\nWe're now encoding the size of these tags (EXIF_TAG_GPS_TIME_STAMP) in\nour layer, where for most other tags are defined by libexif.\n\nGiven that libexif doesn't support those tags correctly it would seem,\nthis is fine - but perhaps we should send a patch to libexif as well.\n\nEven if we patch libexif, we will still have to do this ourselves so no\nblocker.\n\n\nAlso rather than hardcoding '3' should we use std::size(coords) ?\n\n5cfbbcd20731 (\"libcamera: Replace ARRAY_SIZE() with std::size()\")\nsuggests it should be possible.\n\n\n> }\n> \n> std::tuple Exif::degreesToDMS(double decimalDegrees)\n> @@ -376,22 +365,13 @@ std::tuple Exif::degreesToDMS(double decimalDegrees)\n> \n> void Exif::setGPSDMS(ExifIfd ifd, ExifTag tag, int deg, int min, int sec)\n> {\n> -\tExifEntry *entry = createEntry(ifd, tag, EXIF_FORMAT_RATIONAL, 3,\n> -\t\t\t\t 3 * sizeof(ExifRational));\n> -\tif (!entry)\n> -\t\treturn;\n> -\n> \tExifRational coords[] = {\n> \t\t{ static_cast(deg), 1 },\n> \t\t{ static_cast(min), 1 },\n> \t\t{ static_cast(sec), 1 },\n> \t};\n> \n> -\tfor (int i = 0; i < 3; i++)\n> -\t\texif_set_rational(entry->data + i * sizeof(ExifRational),\n> -\t\t\t\t order_, coords[i]);\n> -\n> -\texif_entry_unref(entry);\n> +\tsetRational(ifd, tag, 3, coords);\n\n\nAnd here with std::size()\n\nWith that\n\nReviewed-by: Kieran Bingham \n\n\n\n> }\n> \n> /*\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 27937BD80C\n\tfor ;\n\tMon, 8 Mar 2021 10:46:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9A130602EC;\n\tMon, 8 Mar 2021 11:46:46 +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 50E79602E6\n\tfor ;\n\tMon, 8 Mar 2021 11:46:45 +0100 (CET)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AD4F6AC1;\n\tMon, 8 Mar 2021 11:46:44 +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=\"HebLlton\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1615200404;\n\tbh=fu30Mik7yhPlr/F4orbjeih380rAdrirmcQGNBF+L0E=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=HebLltonBIad3+I0gwO/kzcs4L6mj6sA/oPfnD8IK/I2BL2xlg2K0nhbidcFDZYkX\n\tM/s9pjZbr2C+LpzVs6MvVB71pBUcUl2wa9CtO83/HRtRx2dja1SVSVAzKCNlDBN2ST\n\tZ6FkZtDi0Si875bCrRU7veqYMjGtuR9z7+234gI4=","To":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210308101356.59333-1-paul.elder@ideasonboard.com>\n\t<20210308101356.59333-3-paul.elder@ideasonboard.com>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<3246ec4a-f065-cecc-23a8-e476f60e2d38@ideasonboard.com>","Date":"Mon, 8 Mar 2021 10:46:42 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20210308101356.59333-3-paul.elder@ideasonboard.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH 2/4] android: jpeg: exif: Simplify\n\tsetGPSDateTimestamp and setGPSDMS","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","Reply-To":"kieran.bingham@ideasonboard.com","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15516,"web_url":"https://patchwork.libcamera.org/comment/15516/","msgid":"","date":"2021-03-08T14:38:44","subject":"Re: [libcamera-devel] [PATCH 2/4] android: jpeg: exif: Simplify\n\tsetGPSDateTimestamp and setGPSDMS","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Mon, Mar 08, 2021 at 10:46:42AM +0000, Kieran Bingham wrote:\n> On 08/03/2021 10:13, Paul Elder wrote:\n> > Now that setRational() supports setting multiple rational values, use\n> > that in setGPSDateTimestamp and setGPSDMS which previously set every\n> > rational manually.\n> \n> This feels better to me ;-)\n> \n> > Signed-off-by: Paul Elder \n> > ---\n> > src/android/jpeg/exif.cpp | 24 ++----------------------\n> > 1 file changed, 2 insertions(+), 22 deletions(-)\n> > \n> > diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp\n> > index e5a3e448..c8b2f072 100644\n> > --- a/src/android/jpeg/exif.cpp\n> > +++ b/src/android/jpeg/exif.cpp\n> > @@ -345,24 +345,13 @@ void Exif::setGPSDateTimestamp(time_t timestamp)\n> > \t\t EXIF_FORMAT_ASCII, tsStr);\n> > \n> > \t/* Set GPS_TIME_STAMP */\n> > -\tExifEntry *entry =\n> > -\t\tcreateEntry(EXIF_IFD_GPS,\n> > -\t\t\t static_cast(EXIF_TAG_GPS_TIME_STAMP),\n> > -\t\t\t EXIF_FORMAT_RATIONAL, 3, 3 * sizeof(ExifRational));\n> > -\tif (!entry)\n> > -\t\treturn;\n> > -\n> > \tExifRational ts[] = {\n> > \t\t{ static_cast(tm.tm_hour), 1 },\n> > \t\t{ static_cast(tm.tm_min), 1 },\n> > \t\t{ static_cast(tm.tm_sec), 1 },\n> > \t};\n> > \n> > -\tfor (int i = 0; i < 3; i++)\n> > -\t\texif_set_rational(entry->data + i * sizeof(ExifRational),\n> > -\t\t\t\t order_, ts[i]);\n> > -\n> > -\texif_entry_unref(entry);\n> > +\tsetRational(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_TIME_STAMP), 3, ts);\n> \n> We're now encoding the size of these tags (EXIF_TAG_GPS_TIME_STAMP) in\n> our layer, where for most other tags are defined by libexif.\n> \n> Given that libexif doesn't support those tags correctly it would seem,\n> this is fine - but perhaps we should send a patch to libexif as well.\n\nI've been tempted to send patches to libexif before...\n\n> Even if we patch libexif, we will still have to do this ourselves so no\n> blocker.\n\n... and this is what stopped me :-) Coupled with the fact that the\nlibexif API definitely shows its age, and I would have been too tempted\nof rewriting parts of it :-D The project doesn't seem very active, but\nit could still be worth a try.\n\n> Also rather than hardcoding '3' should we use std::size(coords) ?\n\nI assume you mean ts here. If setRational() switched to Span, then this\nwould be automatic :-)\n\nReviewed-by: Laurent Pinchart \n\n> 5cfbbcd20731 (\"libcamera: Replace ARRAY_SIZE() with std::size()\")\n> suggests it should be possible.\n> \n> > }\n> > \n> > std::tuple Exif::degreesToDMS(double decimalDegrees)\n> > @@ -376,22 +365,13 @@ std::tuple Exif::degreesToDMS(double decimalDegrees)\n> > \n> > void Exif::setGPSDMS(ExifIfd ifd, ExifTag tag, int deg, int min, int sec)\n> > {\n> > -\tExifEntry *entry = createEntry(ifd, tag, EXIF_FORMAT_RATIONAL, 3,\n> > -\t\t\t\t 3 * sizeof(ExifRational));\n> > -\tif (!entry)\n> > -\t\treturn;\n> > -\n> > \tExifRational coords[] = {\n> > \t\t{ static_cast(deg), 1 },\n> > \t\t{ static_cast(min), 1 },\n> > \t\t{ static_cast(sec), 1 },\n> > \t};\n> > \n> > -\tfor (int i = 0; i < 3; i++)\n> > -\t\texif_set_rational(entry->data + i * sizeof(ExifRational),\n> > -\t\t\t\t order_, coords[i]);\n> > -\n> > -\texif_entry_unref(entry);\n> > +\tsetRational(ifd, tag, 3, coords);\n> \n> And here with std::size()\n> \n> With that\n> \n> Reviewed-by: Kieran Bingham \n> \n> > }\n> > \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 40CD5BD80C\n\tfor ;\n\tMon, 8 Mar 2021 14:39:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7E78968A9C;\n\tMon, 8 Mar 2021 15:39:16 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4B967602ED\n\tfor ;\n\tMon, 8 Mar 2021 15:39:15 +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 A894BAC1;\n\tMon, 8 Mar 2021 15:39:14 +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=\"nPOgUXOV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1615214354;\n\tbh=o/YGWLhmluWuQJsCMzbKTmjENdUZt0dNWL7ttlCkKBQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nPOgUXOVNb2tq7hhhQsF0chIwudLlAAbmEKbuhwjKv1f+eFbS04DA8TRXbTo2he0R\n\tVw5j3CLpBXJI/xTt8YwepJVR3BnYn43n+ZmfG2LcgjGZot9PKoyDt8WA5tC1cPPShX\n\tQulKhsm/xU8rvGyV8v8PQMj7EIesKpDrz1/bS6k8=","Date":"Mon, 8 Mar 2021 16:38:44 +0200","From":"Laurent Pinchart ","To":"Paul Elder ","Message-ID":"","References":"<20210308101356.59333-1-paul.elder@ideasonboard.com>\n\t<20210308101356.59333-3-paul.elder@ideasonboard.com>\n\t<3246ec4a-f065-cecc-23a8-e476f60e2d38@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<3246ec4a-f065-cecc-23a8-e476f60e2d38@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 2/4] android: jpeg: exif: Simplify\n\tsetGPSDateTimestamp and setGPSDMS","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":15518,"web_url":"https://patchwork.libcamera.org/comment/15518/","msgid":"<804c86cd-2026-5118-fc58-48be7196a3a0@ideasonboard.com>","date":"2021-03-08T14:43:10","subject":"Re: [libcamera-devel] [PATCH 2/4] android: jpeg: exif: Simplify\n\tsetGPSDateTimestamp and setGPSDMS","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 08/03/2021 14:38, Laurent Pinchart wrote:\n> Hi Paul,\n> \n> Thank you for the patch.\n> \n> On Mon, Mar 08, 2021 at 10:46:42AM +0000, Kieran Bingham wrote:\n>> On 08/03/2021 10:13, Paul Elder wrote:\n>>> Now that setRational() supports setting multiple rational values, use\n>>> that in setGPSDateTimestamp and setGPSDMS which previously set every\n>>> rational manually.\n>>\n>> This feels better to me ;-)\n>>\n>>> Signed-off-by: Paul Elder \n>>> ---\n>>> src/android/jpeg/exif.cpp | 24 ++----------------------\n>>> 1 file changed, 2 insertions(+), 22 deletions(-)\n>>>\n>>> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp\n>>> index e5a3e448..c8b2f072 100644\n>>> --- a/src/android/jpeg/exif.cpp\n>>> +++ b/src/android/jpeg/exif.cpp\n>>> @@ -345,24 +345,13 @@ void Exif::setGPSDateTimestamp(time_t timestamp)\n>>> \t\t EXIF_FORMAT_ASCII, tsStr);\n>>> \n>>> \t/* Set GPS_TIME_STAMP */\n>>> -\tExifEntry *entry =\n>>> -\t\tcreateEntry(EXIF_IFD_GPS,\n>>> -\t\t\t static_cast(EXIF_TAG_GPS_TIME_STAMP),\n>>> -\t\t\t EXIF_FORMAT_RATIONAL, 3, 3 * sizeof(ExifRational));\n>>> -\tif (!entry)\n>>> -\t\treturn;\n>>> -\n>>> \tExifRational ts[] = {\n>>> \t\t{ static_cast(tm.tm_hour), 1 },\n>>> \t\t{ static_cast(tm.tm_min), 1 },\n>>> \t\t{ static_cast(tm.tm_sec), 1 },\n>>> \t};\n>>> \n>>> -\tfor (int i = 0; i < 3; i++)\n>>> -\t\texif_set_rational(entry->data + i * sizeof(ExifRational),\n>>> -\t\t\t\t order_, ts[i]);\n>>> -\n>>> -\texif_entry_unref(entry);\n>>> +\tsetRational(EXIF_IFD_GPS, static_cast(EXIF_TAG_GPS_TIME_STAMP), 3, ts);\n>>\n>> We're now encoding the size of these tags (EXIF_TAG_GPS_TIME_STAMP) in\n>> our layer, where for most other tags are defined by libexif.\n>>\n>> Given that libexif doesn't support those tags correctly it would seem,\n>> this is fine - but perhaps we should send a patch to libexif as well.\n> \n> I've been tempted to send patches to libexif before...\n> \n>> Even if we patch libexif, we will still have to do this ourselves so no\n>> blocker.\n> \n> ... and this is what stopped me :-) Coupled with the fact that the\n> libexif API definitely shows its age, and I would have been too tempted\n> of rewriting parts of it :-D The project doesn't seem very active, but\n> it could still be worth a try.\n> \n>> Also rather than hardcoding '3' should we use std::size(coords) ?\n> \n> I assume you mean ts here. If setRational() switched to Span, then this\n> would be automatic :-)\n\nArgh, that's because I spotted this below, moved the text up to keep the\nflow - and forgot to update the reference.\n\nSpan sounds like a good idea to me though ;-)\n\n\n\n> \n> Reviewed-by: Laurent Pinchart \n> \n>> 5cfbbcd20731 (\"libcamera: Replace ARRAY_SIZE() with std::size()\")\n>> suggests it should be possible.\n>>\n>>> }\n>>> \n>>> std::tuple Exif::degreesToDMS(double decimalDegrees)\n>>> @@ -376,22 +365,13 @@ std::tuple Exif::degreesToDMS(double decimalDegrees)\n>>> \n>>> void Exif::setGPSDMS(ExifIfd ifd, ExifTag tag, int deg, int min, int sec)\n>>> {\n>>> -\tExifEntry *entry = createEntry(ifd, tag, EXIF_FORMAT_RATIONAL, 3,\n>>> -\t\t\t\t 3 * sizeof(ExifRational));\n>>> -\tif (!entry)\n>>> -\t\treturn;\n>>> -\n>>> \tExifRational coords[] = {\n>>> \t\t{ static_cast(deg), 1 },\n>>> \t\t{ static_cast(min), 1 },\n>>> \t\t{ static_cast(sec), 1 },\n>>> \t};\n>>> \n>>> -\tfor (int i = 0; i < 3; i++)\n>>> -\t\texif_set_rational(entry->data + i * sizeof(ExifRational),\n>>> -\t\t\t\t order_, coords[i]);\n>>> -\n>>> -\texif_entry_unref(entry);\n>>> +\tsetRational(ifd, tag, 3, coords);\n>>\n>> And here with std::size()\n>>\n>> With that\n>>\n>> Reviewed-by: Kieran Bingham \n>>\n>>> }\n>>> \n>>> /*\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 5FA71BD1F1\n\tfor ;\n\tMon, 8 Mar 2021 14:43:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DB97460520;\n\tMon, 8 Mar 2021 15:43:14 +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 A70CB60520\n\tfor ;\n\tMon, 8 Mar 2021 15:43:13 +0100 (CET)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1A1E5AC1;\n\tMon, 8 Mar 2021 15:43:13 +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=\"PN1IRB5O\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1615214593;\n\tbh=JKKpToGNUo8kq/bB/EPXPQtIm7/8kkKsO7nyKu/VoPk=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=PN1IRB5O8c/GLjHXwbjaNlIiVp3N8WudNDPecxUV32Hc6gcaEIfXwxqpvkjKEF1f9\n\tASXzS922PhIyFEvFwp7YQrbaqIIR7d8XY0lqnD3IA1ZfXQn8Ukjn7IdfSdg1Y1JHwV\n\tQg72AFtCVTrYaEvb4aPnxcIHpFDv1WayVQjMwXqw=","To":"Laurent Pinchart ,\n\tPaul Elder ","References":"<20210308101356.59333-1-paul.elder@ideasonboard.com>\n\t<20210308101356.59333-3-paul.elder@ideasonboard.com>\n\t<3246ec4a-f065-cecc-23a8-e476f60e2d38@ideasonboard.com>\n\t","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<804c86cd-2026-5118-fc58-48be7196a3a0@ideasonboard.com>","Date":"Mon, 8 Mar 2021 14:43:10 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH 2/4] android: jpeg: exif: Simplify\n\tsetGPSDateTimestamp and setGPSDMS","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","Reply-To":"kieran.bingham@ideasonboard.com","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\" "}}]