[{"id":12292,"web_url":"https://patchwork.libcamera.org/comment/12292/","msgid":"<936f2cf3-80b4-e8a6-1708-ac03f8eca59a@uajain.com>","date":"2020-09-04T06:43:14","subject":"Re: [libcamera-devel] [PATCH v6 2/2] android: jpeg: Support an\n\tinitial set of EXIF metadata tags","submitter":{"id":1,"url":"https://patchwork.libcamera.org/api/people/1/","name":"Umang Jain","email":"email@uajain.com"},"content":"On 9/4/20 12:10 PM, Umang Jain wrote:\n> Create a Exif object with various metadata tags set, just before\n> the encoder starts to encode the frame. The object is passed\n> directly as libcamera::Span<> to make sure EXIF tags can be set\n> in a single place i.e. in CameraDevice and the encoder only has\n> the job to write the data in the final output.\n>\n> Signed-off-by: Kieran Bingham \n> Signed-off-by: Umang Jain \n> Reviewed-by: Kieran Bingham \n> Reviewed-by: Laurent Pinchart \n> ---\n> src/android/camera_device.cpp | 19 ++++++++++-\n> src/android/jpeg/encoder.h | 3 +-\n> src/android/jpeg/encoder_libjpeg.cpp | 9 ++++-\n> src/android/jpeg/encoder_libjpeg.h | 3 +-\n> src/android/jpeg/exif.cpp | 50 ++++++++++++++++++++++++++++\n> src/android/jpeg/exif.h | 8 +++++\n> 6 files changed, 88 insertions(+), 4 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index de6f86f..0328ac6 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -24,6 +24,7 @@\n> #include \"system/graphics.h\"\n> \n> #include \"jpeg/encoder_libjpeg.h\"\n> +#include \"jpeg/exif.h\"\n> \n> using namespace libcamera;\n> \n> @@ -1434,7 +1435,23 @@ void CameraDevice::requestComplete(Request *request)\n> \t\t\tcontinue;\n> \t\t}\n> \n> -\t\tint jpeg_size = encoder->encode(buffer, mapped.maps()[0]);\n> +\t\t/* Set EXIF metadata for various tags. */\n> +\t\tExif exif;\n> +\t\t/* \\todo Set Make and Model from external vendor tags. */\n> +\t\texif.setMake(\"libcamera\");\n> +\t\texif.setModel(\"cameraModel\");\n> +\t\texif.setOrientation(orientation_);\n> +\t\texif.setSize(cameraStream->size);\n> +\t\t/*\n> +\t\t * We set the frame's EXIF timestamp as the time of encode.\n> +\t\t * Since the precision we need for EXIF timestamp is only one\n> +\t\t * second, it is good enough.\n> +\t\t */\n> +\t\texif.setTimestamp(std::time(nullptr));\n> +\t\tif (exif.generate() != 0)\n> +\t\t\tLOG(HAL, Error) << \"Failed to get valid EXIF data\";\n> +\n> +\t\tint jpeg_size = encoder->encode(buffer, mapped.maps()[0], exif.data());\n> \t\tif (jpeg_size < 0) {\n> \t\t\tLOG(HAL, Error) << \"Failed to encode stream image\";\n> \t\t\tstatus = CAMERA3_BUFFER_STATUS_ERROR;\n> diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h\n> index f9eb88e..cf26d67 100644\n> --- a/src/android/jpeg/encoder.h\n> +++ b/src/android/jpeg/encoder.h\n> @@ -18,7 +18,8 @@ public:\n> \n> \tvirtual int configure(const libcamera::StreamConfiguration &cfg) = 0;\n> \tvirtual int encode(const libcamera::FrameBuffer *source,\n> -\t\t\t const libcamera::Span &destination) = 0;\n> +\t\t\t const libcamera::Span &destination,\n> +\t\t\t const libcamera::Span &exifData) = 0;\n> };\n> \n> #endif /* __ANDROID_JPEG_ENCODER_H__ */\n> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp\n> index 977538c..510613c 100644\n> --- a/src/android/jpeg/encoder_libjpeg.cpp\n> +++ b/src/android/jpeg/encoder_libjpeg.cpp\n> @@ -180,7 +180,8 @@ void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame)\n> }\n> \n> int EncoderLibJpeg::encode(const FrameBuffer *source,\n> -\t\t\t const libcamera::Span &dest)\n> +\t\t\t const libcamera::Span &dest,\n> +\t\t\t const libcamera::Span &exifData)\n> {\n> \tMappedFrameBuffer frame(source, PROT_READ);\n> \tif (!frame.isValid()) {\n> @@ -204,6 +205,12 @@ int EncoderLibJpeg::encode(const FrameBuffer *source,\n> \n> \tjpeg_start_compress(&compress_, TRUE);\n> \n> +\tif (exifData.size())\n> +\t\t/* Store Exif data in the JPEG_APP1 data block. */\n> +\t\tjpeg_write_marker(&compress_, JPEG_APP0 + 1,\n> +\t\t\t\t static_cast(exifData.data()),\n> +\t\t\t\t exifData.size());\n> +\n> \tLOG(JPEG, Debug) << \"JPEG Encode Starting:\" << compress_.image_width\n> \t\t\t << \"x\" << compress_.image_height;\n> \n> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h\n> index aed081a..1e8df05 100644\n> --- a/src/android/jpeg/encoder_libjpeg.h\n> +++ b/src/android/jpeg/encoder_libjpeg.h\n> @@ -22,7 +22,8 @@ public:\n> \n> \tint configure(const libcamera::StreamConfiguration &cfg) override;\n> \tint encode(const libcamera::FrameBuffer *source,\n> -\t\t const libcamera::Span &destination) override;\n> +\t\t const libcamera::Span &destination,\n> +\t\t const libcamera::Span &exifData) override;\n> \n> private:\n> \tvoid compressRGB(const libcamera::MappedBuffer *frame);\n> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp\n> index 41ddf48..36922ef 100644\n> --- a/src/android/jpeg/exif.cpp\n> +++ b/src/android/jpeg/exif.cpp\n> @@ -168,6 +168,56 @@ void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::str\n> \texif_entry_unref(entry);\n> }\n> \n> +void Exif::setMake(const std::string &make)\n> +{\n> +\tsetString(EXIF_IFD_0, EXIF_TAG_MAKE, EXIF_FORMAT_ASCII, make);\n> +}\n> +\n> +void Exif::setModel(const std::string &model)\n> +{\n> +\tsetString(EXIF_IFD_0, EXIF_TAG_MODEL, EXIF_FORMAT_ASCII, model);\n> +}\n> +\n> +void Exif::setSize(const Size &size)\n> +{\n> +\tsetLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_Y_DIMENSION, size.height);\n> +\tsetLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_X_DIMENSION, size.width);\n> +}\n> +\n> +void Exif::setTimestamp(time_t timestamp)\n> +{\n> +\tchar str[20];\n> +\tstd::strftime(str, sizeof(str), \"%Y:%m:%d %H:%M:%S\",\n> +\t\t std::localtime(×tamp));\n> +\tstd::string ts(str);\n> +\n> +\tsetString(EXIF_IFD_0, EXIF_TAG_DATE_TIME, EXIF_FORMAT_ASCII, ts);\n> +\tsetString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_ORIGINAL, EXIF_FORMAT_ASCII, ts);\n> +\tsetString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_DIGITIZED, EXIF_FORMAT_ASCII, ts);\n> +}\n> +\n> +void Exif::setOrientation(int orientation)\n> +{\n> +\tint value;\n> +\tswitch (orientation) {\n> +\tcase 0:\n> +\tdefault:\n> +\t\tvalue = 1;\n> +\t\tbreak;\n> +\tcase 90:\n> +\t\tvalue = 6;\n> +\t\tbreak;\n> +\tcase 180:\n> +\t\tvalue = 3;\n> +\t\tbreak;\n> +\tcase 270:\n> +\t\tvalue = 8;\n> +\t\tbreak;\n> +\t}\n> +\n> +\tsetShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value);\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 8dfc324..622de4c 100644\n> --- a/src/android/jpeg/exif.h\n> +++ b/src/android/jpeg/exif.h\n> @@ -12,6 +12,7 @@\n> \n> #include \n> \n> +#include \n> #include \n> \n> class Exif\n> @@ -20,6 +21,13 @@ public:\n> \tExif();\n> \t~Exif();\n> \n> +\tvoid setMake(const std::string &make);\n> +\tvoid setModel(const std::string &model);\n> +\n> +\tvoid setOrientation(int orientation);\n> +\tvoid setSize(const libcamera::Size &size);\n> +\tvoid setTimestamp(time_t timestamp);\n> +\n> \tlibcamera::Span data() const { return { exifData_, size_ }; }\n> \t[[nodiscard]] int generate();\n> \nStupid internet connection dropped while sending patch 2/2\nand git send-email ..  --in-reply-to=\"[libcamera-devel] [PATCH v6 0/2] \nInitial EXIF metadata support\"\ndidn't work as expected  :(","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 1AC1ABE174\n\tfor ;\n\tFri, 4 Sep 2020 06:43:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9102C629B6;\n\tFri, 4 Sep 2020 08:43:33 +0200 (CEST)","from mail.uajain.com (static.126.159.217.95.clients.your-server.de\n\t[95.217.159.126])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 236266036F\n\tfor ;\n\tFri, 4 Sep 2020 08:43:32 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=uajain.com header.i=@uajain.com\n\theader.b=\"u4ML3oRp\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=uajain.com; s=mail;\n\tt=1599201811; bh=J3YU/zgk+fvkxx3AxxJikM6KUXBt94QWflDSkmArzGI=;\n\th=Subject:To:Cc:References:From:In-Reply-To;\n\tb=u4ML3oRprFVP8CQoSAW+bN2YakPYMhny2sbqAJ43N9TztC8TY6MMdy3hHbMuGn/AZ\n\tRBgpTgUFb5vTJjljyVne86A8jQusKgMFNgEMHZRMXSfo3k0Q98F2aVbLMEOsPuHzxR\n\tk0ycIb2YkhKhFQkDPhbPAcKBa40hTcvUOi208rkcg+ELiBspS8WhwRJxQQXSryEL18\n\twvWOk4ZnlYElzjfDF3XvQ5jAcz9XijaFp2oAV/iK4V2gRDPN5FsQlHiieh2w6DfxFM\n\tz8Q3phRTdfnMpvcQEaAjG/YsXCz3DvFggLDHxaJBZfmN8w/4+g5YUcvZRnUxxBsbks\n\tl8p4hOkwp+4sQ==","To":"libcamera-devel@lists.libcamera.org","References":"<[libcamera-devel] [PATCH v6 0/2] Initial EXIF metadata support>\n\t<20200904064019.29869-1-email@uajain.com>","From":"Umang Jain ","Message-ID":"<936f2cf3-80b4-e8a6-1708-ac03f8eca59a@uajain.com>","Date":"Fri, 4 Sep 2020 12:13:14 +0530","Mime-Version":"1.0","In-Reply-To":"<20200904064019.29869-1-email@uajain.com>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v6 2/2] android: jpeg: Support an\n\tinitial set of EXIF metadata tags","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","Content-Transfer-Encoding":"base64","Content-Type":"text/plain; charset=\"utf-8\"; Format=\"flowed\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12293,"web_url":"https://patchwork.libcamera.org/comment/12293/","msgid":"","date":"2020-09-04T08:32:37","subject":"Re: [libcamera-devel] [PATCH v6 2/2] android: jpeg: Support an\n\tinitial set of EXIF metadata tags","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Umang,\n\nOn 04/09/2020 07:43, Umang Jain wrote:\n\n\n>> +    void setOrientation(int orientation);\n>> +    void setSize(const libcamera::Size &size);\n>> +    void setTimestamp(time_t timestamp);\n>> +\n>>       libcamera::Span data() const { return {\n>> exifData_, size_ }; }\n>>       [[nodiscard]] int generate();\n>>   \n> Stupid internet connection dropped while sending patch 2/2\n> and git send-email ..  --in-reply-to=\"[libcamera-devel] [PATCH v6 0/2]\n> Initial EXIF metadata support\"\n> didn't work as expected  :(\n\nTo use git send-email --in-reply-to= you need to specify the 'message id'\n\nSomething like this:\n\ngit send-email \\\n --in-reply-to=\"<936f2cf3-80b4-e8a6-1708-ac03f8eca59a@uajain.com>\"","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 5A669BF019\n\tfor ;\n\tFri, 4 Sep 2020 08:32:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DA5D260371;\n\tFri, 4 Sep 2020 10:32:47 +0200 (CEST)","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 82F6B60371\n\tfor ;\n\tFri, 4 Sep 2020 10:32:46 +0200 (CEST)","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 4B9AB277;\n\tFri, 4 Sep 2020 10:32:40 +0200 (CEST)"],"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=\"qsdOhvEH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1599208360;\n\tbh=wcIvKmbi9x7iO0xWx/Eav3VGrfQNR9BQ+En2UYTfufs=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=qsdOhvEHPFOshnX779YO+O49pOnFBwtmwLTyrCtAZSED0/EiirNZZdecUmGz2hvXu\n\t87jCou7NkkC7k/0qnxjBl4Aaet2MA31EaN/CZytZjIGSDyHIXNdyhoBXUNIyO/g4iw\n\tXcETJ0VSZyHwCI2hH4DOHoXKFdAlyGN/jXDTq/6E=","To":"Umang Jain , libcamera-devel@lists.libcamera.org","References":"<[libcamera-devel] [PATCH v6 0/2] Initial EXIF metadata support>\n\t<20200904064019.29869-1-email@uajain.com>\n\t<936f2cf3-80b4-e8a6-1708-ac03f8eca59a@uajain.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":"","Date":"Fri, 4 Sep 2020 09:32:37 +0100","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":"<936f2cf3-80b4-e8a6-1708-ac03f8eca59a@uajain.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v6 2/2] android: jpeg: Support an\n\tinitial set of EXIF metadata tags","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=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12305,"web_url":"https://patchwork.libcamera.org/comment/12305/","msgid":"<20200904153429.GE7518@pendragon.ideasonboard.com>","date":"2020-09-04T15:34:29","subject":"Re: [libcamera-devel] [PATCH v6 2/2] android: jpeg: Support an\n\tinitial set of EXIF metadata tags","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nThank you for the patch.\n\nOn Fri, Sep 04, 2020 at 12:10:19PM +0530, Umang Jain wrote:\n> Create a Exif object with various metadata tags set, just before\n> the encoder starts to encode the frame. The object is passed\n> directly as libcamera::Span<> to make sure EXIF tags can be set\n> in a single place i.e. in CameraDevice and the encoder only has\n> the job to write the data in the final output.\n> \n> Signed-off-by: Kieran Bingham \n> Signed-off-by: Umang Jain \n> Reviewed-by: Kieran Bingham \n> Reviewed-by: Laurent Pinchart \n> ---\n> src/android/camera_device.cpp | 19 ++++++++++-\n> src/android/jpeg/encoder.h | 3 +-\n> src/android/jpeg/encoder_libjpeg.cpp | 9 ++++-\n> src/android/jpeg/encoder_libjpeg.h | 3 +-\n> src/android/jpeg/exif.cpp | 50 ++++++++++++++++++++++++++++\n> src/android/jpeg/exif.h | 8 +++++\n> 6 files changed, 88 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index de6f86f..0328ac6 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -24,6 +24,7 @@\n> #include \"system/graphics.h\"\n> \n> #include \"jpeg/encoder_libjpeg.h\"\n> +#include \"jpeg/exif.h\"\n> \n> using namespace libcamera;\n> \n> @@ -1434,7 +1435,23 @@ void CameraDevice::requestComplete(Request *request)\n> \t\t\tcontinue;\n> \t\t}\n> \n> -\t\tint jpeg_size = encoder->encode(buffer, mapped.maps()[0]);\n> +\t\t/* Set EXIF metadata for various tags. */\n> +\t\tExif exif;\n> +\t\t/* \\todo Set Make and Model from external vendor tags. */\n> +\t\texif.setMake(\"libcamera\");\n> +\t\texif.setModel(\"cameraModel\");\n> +\t\texif.setOrientation(orientation_);\n> +\t\texif.setSize(cameraStream->size);\n> +\t\t/*\n> +\t\t * We set the frame's EXIF timestamp as the time of encode.\n> +\t\t * Since the precision we need for EXIF timestamp is only one\n> +\t\t * second, it is good enough.\n> +\t\t */\n> +\t\texif.setTimestamp(std::time(nullptr));\n> +\t\tif (exif.generate() != 0)\n> +\t\t\tLOG(HAL, Error) << \"Failed to get valid EXIF data\";\n\ns/get/generate/ ?\n\n> +\n> +\t\tint jpeg_size = encoder->encode(buffer, mapped.maps()[0], exif.data());\n> \t\tif (jpeg_size < 0) {\n> \t\t\tLOG(HAL, Error) << \"Failed to encode stream image\";\n> \t\t\tstatus = CAMERA3_BUFFER_STATUS_ERROR;\n> diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h\n> index f9eb88e..cf26d67 100644\n> --- a/src/android/jpeg/encoder.h\n> +++ b/src/android/jpeg/encoder.h\n> @@ -18,7 +18,8 @@ public:\n> \n> \tvirtual int configure(const libcamera::StreamConfiguration &cfg) = 0;\n> \tvirtual int encode(const libcamera::FrameBuffer *source,\n> -\t\t\t const libcamera::Span &destination) = 0;\n> +\t\t\t const libcamera::Span &destination,\n> +\t\t\t const libcamera::Span &exifData) = 0;\n> };\n> \n> #endif /* __ANDROID_JPEG_ENCODER_H__ */\n> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp\n> index 977538c..510613c 100644\n> --- a/src/android/jpeg/encoder_libjpeg.cpp\n> +++ b/src/android/jpeg/encoder_libjpeg.cpp\n> @@ -180,7 +180,8 @@ void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame)\n> }\n> \n> int EncoderLibJpeg::encode(const FrameBuffer *source,\n> -\t\t\t const libcamera::Span &dest)\n> +\t\t\t const libcamera::Span &dest,\n> +\t\t\t const libcamera::Span &exifData)\n> {\n> \tMappedFrameBuffer frame(source, PROT_READ);\n> \tif (!frame.isValid()) {\n> @@ -204,6 +205,12 @@ int EncoderLibJpeg::encode(const FrameBuffer *source,\n> \n> \tjpeg_start_compress(&compress_, TRUE);\n> \n> +\tif (exifData.size())\n> +\t\t/* Store Exif data in the JPEG_APP1 data block. */\n> +\t\tjpeg_write_marker(&compress_, JPEG_APP0 + 1,\n> +\t\t\t\t static_cast(exifData.data()),\n> +\t\t\t\t exifData.size());\n> +\n> \tLOG(JPEG, Debug) << \"JPEG Encode Starting:\" << compress_.image_width\n> \t\t\t << \"x\" << compress_.image_height;\n> \n> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h\n> index aed081a..1e8df05 100644\n> --- a/src/android/jpeg/encoder_libjpeg.h\n> +++ b/src/android/jpeg/encoder_libjpeg.h\n> @@ -22,7 +22,8 @@ public:\n> \n> \tint configure(const libcamera::StreamConfiguration &cfg) override;\n> \tint encode(const libcamera::FrameBuffer *source,\n> -\t\t const libcamera::Span &destination) override;\n> +\t\t const libcamera::Span &destination,\n> +\t\t const libcamera::Span &exifData) override;\n> \n> private:\n> \tvoid compressRGB(const libcamera::MappedBuffer *frame);\n> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp\n> index 41ddf48..36922ef 100644\n> --- a/src/android/jpeg/exif.cpp\n> +++ b/src/android/jpeg/exif.cpp\n> @@ -168,6 +168,56 @@ void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::str\n> \texif_entry_unref(entry);\n> }\n> \n> +void Exif::setMake(const std::string &make)\n> +{\n> +\tsetString(EXIF_IFD_0, EXIF_TAG_MAKE, EXIF_FORMAT_ASCII, make);\n> +}\n> +\n> +void Exif::setModel(const std::string &model)\n> +{\n> +\tsetString(EXIF_IFD_0, EXIF_TAG_MODEL, EXIF_FORMAT_ASCII, model);\n> +}\n> +\n> +void Exif::setSize(const Size &size)\n> +{\n> +\tsetLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_Y_DIMENSION, size.height);\n> +\tsetLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_X_DIMENSION, size.width);\n> +}\n> +\n> +void Exif::setTimestamp(time_t timestamp)\n> +{\n> +\tchar str[20];\n> +\tstd::strftime(str, sizeof(str), \"%Y:%m:%d %H:%M:%S\",\n> +\t\t std::localtime(×tamp));\n> +\tstd::string ts(str);\n> +\n> +\tsetString(EXIF_IFD_0, EXIF_TAG_DATE_TIME, EXIF_FORMAT_ASCII, ts);\n> +\tsetString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_ORIGINAL, EXIF_FORMAT_ASCII, ts);\n> +\tsetString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_DIGITIZED, EXIF_FORMAT_ASCII, ts);\n> +}\n> +\n> +void Exif::setOrientation(int orientation)\n> +{\n> +\tint value;\n> +\tswitch (orientation) {\n> +\tcase 0:\n> +\tdefault:\n> +\t\tvalue = 1;\n> +\t\tbreak;\n> +\tcase 90:\n> +\t\tvalue = 6;\n> +\t\tbreak;\n> +\tcase 180:\n> +\t\tvalue = 3;\n> +\t\tbreak;\n> +\tcase 270:\n> +\t\tvalue = 8;\n> +\t\tbreak;\n\nShouldn't the the 90 and 270 values be swapped ? 6 means \"The 0th row is\nthe visual right-hand side of the image, and the 0th column is the\nvisual top\", and 8 means \"The 0th row is the visual left-hand side of\nthe image, and the 0th column is the visual bottom\".\n\nLooking at the rotation property definition, the 90° rotation examples\nshows\n\n +-------------------------------------+\n | _ _ |\n | \\ / |\n | | | |\n | | | |\n | | > |\n | < | |\n | | | |\n | . |\n | V |\n +-------------------------------------+\n\nwhere the 0th row is the left side and the 0th column the bottom side.\n\nI can fix those two small issues when applying, but I'd appreciate if\nsomeone could double-check the rotation.\n\n> +\t}\n> +\n> +\tsetShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value);\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 8dfc324..622de4c 100644\n> --- a/src/android/jpeg/exif.h\n> +++ b/src/android/jpeg/exif.h\n> @@ -12,6 +12,7 @@\n> \n> #include \n> \n> +#include \n> #include \n> \n> class Exif\n> @@ -20,6 +21,13 @@ public:\n> \tExif();\n> \t~Exif();\n> \n> +\tvoid setMake(const std::string &make);\n> +\tvoid setModel(const std::string &model);\n> +\n> +\tvoid setOrientation(int orientation);\n> +\tvoid setSize(const libcamera::Size &size);\n> +\tvoid setTimestamp(time_t timestamp);\n> +\n> \tlibcamera::Span data() const { return { exifData_, size_ }; }\n> \t[[nodiscard]] int generate();\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 34AEBBF019\n\tfor ;\n\tFri, 4 Sep 2020 15:34:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9CB4860599;\n\tFri, 4 Sep 2020 17:34:58 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5D66A6037B\n\tfor ;\n\tFri, 4 Sep 2020 17:34:56 +0200 (CEST)","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 D5E95540;\n\tFri, 4 Sep 2020 17:34:52 +0200 (CEST)"],"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=\"fNU3ScXq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1599233693;\n\tbh=MX1qOzXGQJtpbOANKZeSDTE3m/L4d3+Xe1egsjef98A=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=fNU3ScXqLFnl73RnVr9lKZJH7X7M4vub484isdUljgptZyMA73jqth3o9WjqlLF5C\n\tYz+RUoJEuxANGRk8k41MDohDBtl5UnhzZg/E/yxKM6Bn3eFbgQvd+ViaIq2aORKuRG\n\tHgnY370RQ9RxNjGcX1WVbIN24G8GzFqRYPt6DyjE=","Date":"Fri, 4 Sep 2020 18:34:29 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"<20200904153429.GE7518@pendragon.ideasonboard.com>","References":"<20200904064019.29869-1-email@uajain.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200904064019.29869-1-email@uajain.com>","Subject":"Re: [libcamera-devel] [PATCH v6 2/2] android: jpeg: Support an\n\tinitial set of EXIF metadata tags","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":12371,"web_url":"https://patchwork.libcamera.org/comment/12371/","msgid":"","date":"2020-09-08T10:12:39","subject":"Re: [libcamera-devel] [PATCH v6 2/2] android: jpeg: Support an\n\tinitial set of EXIF metadata tags","submitter":{"id":1,"url":"https://patchwork.libcamera.org/api/people/1/","name":"Umang Jain","email":"email@uajain.com"},"content":"Hi Laurent,\n\nOn 9/4/20 9:04 PM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Fri, Sep 04, 2020 at 12:10:19PM +0530, Umang Jain wrote:\n>> Create a Exif object with various metadata tags set, just before\n>> the encoder starts to encode the frame. The object is passed\n>> directly as libcamera::Span<> to make sure EXIF tags can be set\n>> in a single place i.e. in CameraDevice and the encoder only has\n>> the job to write the data in the final output.\n>>\n>> Signed-off-by: Kieran Bingham \n>> Signed-off-by: Umang Jain \n>> Reviewed-by: Kieran Bingham \n>> Reviewed-by: Laurent Pinchart \n>> ---\n>> src/android/camera_device.cpp | 19 ++++++++++-\n>> src/android/jpeg/encoder.h | 3 +-\n>> src/android/jpeg/encoder_libjpeg.cpp | 9 ++++-\n>> src/android/jpeg/encoder_libjpeg.h | 3 +-\n>> src/android/jpeg/exif.cpp | 50 ++++++++++++++++++++++++++++\n>> src/android/jpeg/exif.h | 8 +++++\n>> 6 files changed, 88 insertions(+), 4 deletions(-)\n>>\n>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>> index de6f86f..0328ac6 100644\n>> --- a/src/android/camera_device.cpp\n>> +++ b/src/android/camera_device.cpp\n>> @@ -24,6 +24,7 @@\n>> #include \"system/graphics.h\"\n>> \n>> #include \"jpeg/encoder_libjpeg.h\"\n>> +#include \"jpeg/exif.h\"\n>> \n>> using namespace libcamera;\n>> \n>> @@ -1434,7 +1435,23 @@ void CameraDevice::requestComplete(Request *request)\n>> \t\t\tcontinue;\n>> \t\t}\n>> \n>> -\t\tint jpeg_size = encoder->encode(buffer, mapped.maps()[0]);\n>> +\t\t/* Set EXIF metadata for various tags. */\n>> +\t\tExif exif;\n>> +\t\t/* \\todo Set Make and Model from external vendor tags. */\n>> +\t\texif.setMake(\"libcamera\");\n>> +\t\texif.setModel(\"cameraModel\");\n>> +\t\texif.setOrientation(orientation_);\n>> +\t\texif.setSize(cameraStream->size);\n>> +\t\t/*\n>> +\t\t * We set the frame's EXIF timestamp as the time of encode.\n>> +\t\t * Since the precision we need for EXIF timestamp is only one\n>> +\t\t * second, it is good enough.\n>> +\t\t */\n>> +\t\texif.setTimestamp(std::time(nullptr));\n>> +\t\tif (exif.generate() != 0)\n>> +\t\t\tLOG(HAL, Error) << \"Failed to get valid EXIF data\";\n> s/get/generate/ ?\n>\n>> +\n>> +\t\tint jpeg_size = encoder->encode(buffer, mapped.maps()[0], exif.data());\n>> \t\tif (jpeg_size < 0) {\n>> \t\t\tLOG(HAL, Error) << \"Failed to encode stream image\";\n>> \t\t\tstatus = CAMERA3_BUFFER_STATUS_ERROR;\n>> diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h\n>> index f9eb88e..cf26d67 100644\n>> --- a/src/android/jpeg/encoder.h\n>> +++ b/src/android/jpeg/encoder.h\n>> @@ -18,7 +18,8 @@ public:\n>> \n>> \tvirtual int configure(const libcamera::StreamConfiguration &cfg) = 0;\n>> \tvirtual int encode(const libcamera::FrameBuffer *source,\n>> -\t\t\t const libcamera::Span &destination) = 0;\n>> +\t\t\t const libcamera::Span &destination,\n>> +\t\t\t const libcamera::Span &exifData) = 0;\n>> };\n>> \n>> #endif /* __ANDROID_JPEG_ENCODER_H__ */\n>> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp\n>> index 977538c..510613c 100644\n>> --- a/src/android/jpeg/encoder_libjpeg.cpp\n>> +++ b/src/android/jpeg/encoder_libjpeg.cpp\n>> @@ -180,7 +180,8 @@ void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame)\n>> }\n>> \n>> int EncoderLibJpeg::encode(const FrameBuffer *source,\n>> -\t\t\t const libcamera::Span &dest)\n>> +\t\t\t const libcamera::Span &dest,\n>> +\t\t\t const libcamera::Span &exifData)\n>> {\n>> \tMappedFrameBuffer frame(source, PROT_READ);\n>> \tif (!frame.isValid()) {\n>> @@ -204,6 +205,12 @@ int EncoderLibJpeg::encode(const FrameBuffer *source,\n>> \n>> \tjpeg_start_compress(&compress_, TRUE);\n>> \n>> +\tif (exifData.size())\n>> +\t\t/* Store Exif data in the JPEG_APP1 data block. */\n>> +\t\tjpeg_write_marker(&compress_, JPEG_APP0 + 1,\n>> +\t\t\t\t static_cast(exifData.data()),\n>> +\t\t\t\t exifData.size());\n>> +\n>> \tLOG(JPEG, Debug) << \"JPEG Encode Starting:\" << compress_.image_width\n>> \t\t\t << \"x\" << compress_.image_height;\n>> \n>> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h\n>> index aed081a..1e8df05 100644\n>> --- a/src/android/jpeg/encoder_libjpeg.h\n>> +++ b/src/android/jpeg/encoder_libjpeg.h\n>> @@ -22,7 +22,8 @@ public:\n>> \n>> \tint configure(const libcamera::StreamConfiguration &cfg) override;\n>> \tint encode(const libcamera::FrameBuffer *source,\n>> -\t\t const libcamera::Span &destination) override;\n>> +\t\t const libcamera::Span &destination,\n>> +\t\t const libcamera::Span &exifData) override;\n>> \n>> private:\n>> \tvoid compressRGB(const libcamera::MappedBuffer *frame);\n>> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp\n>> index 41ddf48..36922ef 100644\n>> --- a/src/android/jpeg/exif.cpp\n>> +++ b/src/android/jpeg/exif.cpp\n>> @@ -168,6 +168,56 @@ void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::str\n>> \texif_entry_unref(entry);\n>> }\n>> \n>> +void Exif::setMake(const std::string &make)\n>> +{\n>> +\tsetString(EXIF_IFD_0, EXIF_TAG_MAKE, EXIF_FORMAT_ASCII, make);\n>> +}\n>> +\n>> +void Exif::setModel(const std::string &model)\n>> +{\n>> +\tsetString(EXIF_IFD_0, EXIF_TAG_MODEL, EXIF_FORMAT_ASCII, model);\n>> +}\n>> +\n>> +void Exif::setSize(const Size &size)\n>> +{\n>> +\tsetLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_Y_DIMENSION, size.height);\n>> +\tsetLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_X_DIMENSION, size.width);\n>> +}\n>> +\n>> +void Exif::setTimestamp(time_t timestamp)\n>> +{\n>> +\tchar str[20];\n>> +\tstd::strftime(str, sizeof(str), \"%Y:%m:%d %H:%M:%S\",\n>> +\t\t std::localtime(×tamp));\n>> +\tstd::string ts(str);\n>> +\n>> +\tsetString(EXIF_IFD_0, EXIF_TAG_DATE_TIME, EXIF_FORMAT_ASCII, ts);\n>> +\tsetString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_ORIGINAL, EXIF_FORMAT_ASCII, ts);\n>> +\tsetString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_DIGITIZED, EXIF_FORMAT_ASCII, ts);\n>> +}\n>> +\n>> +void Exif::setOrientation(int orientation)\n>> +{\n>> +\tint value;\n>> +\tswitch (orientation) {\n>> +\tcase 0:\n>> +\tdefault:\n>> +\t\tvalue = 1;\n>> +\t\tbreak;\n>> +\tcase 90:\n>> +\t\tvalue = 6;\n>> +\t\tbreak;\n>> +\tcase 180:\n>> +\t\tvalue = 3;\n>> +\t\tbreak;\n>> +\tcase 270:\n>> +\t\tvalue = 8;\n>> +\t\tbreak;\n> Shouldn't the the 90 and 270 values be swapped ? 6 means \"The 0th row is\n> the visual right-hand side of the image, and the 0th column is the\n> visual top\", and 8 means \"The 0th row is the visual left-hand side of\n> the image, and the 0th column is the visual bottom\".\n>\n> Looking at the rotation property definition, the 90° rotation examples\n> shows\n>\n> +-------------------------------------+\n> | _ _ |\n> | \\ / |\n> | | | |\n> | | | |\n> | | > |\n> | < | |\n> | | | |\n> | . |\n> | V |\n> +-------------------------------------+\n>\n> where the 0th row is the left side and the 0th column the bottom side.\n>\n> I can fix those two small issues when applying, but I'd appreciate if\n> someone could double-check the rotation.\nDoes the rotation property has to do anything with the fact that, camera \nsensor(webcam use-case) is intentionally mounted upside down to avoid \nrotation correction? A widely used example on the internet is a big 'F' \nimage denoting the value according to it's orientation [1], which is the \nsame as per the values used in the patch. On the other hand, I can also \nfind the code which satisfies Laurent's review and the EXIF \n'orientation' specification [2]\n\n[1]: \nhttps://chromium.googlesource.com/chromiumos/platform2/+/HEAD/camera/common/exif_utils.cc#376\n[2]: \nhttps://piexif.readthedocs.io/en/latest/sample.html#rotate-image-by-exif-orientation\n\n¯\\_(ツ)_/¯\n>\n>> +\t}\n>> +\n>> +\tsetShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value);\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 8dfc324..622de4c 100644\n>> --- a/src/android/jpeg/exif.h\n>> +++ b/src/android/jpeg/exif.h\n>> @@ -12,6 +12,7 @@\n>> \n>> #include \n>> \n>> +#include \n>> #include \n>> \n>> class Exif\n>> @@ -20,6 +21,13 @@ public:\n>> \tExif();\n>> \t~Exif();\n>> \n>> +\tvoid setMake(const std::string &make);\n>> +\tvoid setModel(const std::string &model);\n>> +\n>> +\tvoid setOrientation(int orientation);\n>> +\tvoid setSize(const libcamera::Size &size);\n>> +\tvoid setTimestamp(time_t timestamp);\n>> +\n>> \tlibcamera::Span data() const { return { exifData_, size_ }; }\n>> \t[[nodiscard]] int generate();\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 BF1D3BDB1D\n\tfor ;\n\tTue, 8 Sep 2020 10:12:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 26F8662C31;\n\tTue, 8 Sep 2020 12:12:47 +0200 (CEST)","from mail.uajain.com (static.126.159.217.95.clients.your-server.de\n\t[95.217.159.126])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E14BE6056C\n\tfor ;\n\tTue, 8 Sep 2020 12:12:45 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=uajain.com header.i=@uajain.com\n\theader.b=\"bjF1V3UO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=uajain.com; s=mail;\n\tt=1599559965; bh=bw8Uhu7HjmWVYD3E0Wt4F7tsi6F49SYvlPa/U+lN/2Q=;\n\th=Subject:To:Cc:References:From:In-Reply-To;\n\tb=bjF1V3UOTfP8l5OZvhVp5wlhMTQojhIf90UJykx07zpLswOZSpL1VWzN0U1lzJrUs\n\tie4S97Opu8m0eu4oSM21Z7YcyGhFLs+xMebvzI8UxYVkMY59KUHq3xyep45/RBVwf3\n\tMTvweo/m6fNIYOFqQo1kTheS4y0HN6DC/m6TvOSiot4XMSLMyNKj40N0e39ccf2pgW\n\t++1FXyVEGBXpLBRfJ/+xKMimG9H39mhnwR7Fb4/X3gIp7J4aW3r8dyBjtRJUfE8Bl5\n\t8q0jS+7189FadA9telLksCEIWOQGGOLWoPT7rvngkYcTVr4OwNavijNWXpRqPdX3AB\n\tJrvVNdloYAx7w==","To":"Laurent Pinchart ","References":"<20200904064019.29869-1-email@uajain.com>\n\t<20200904153429.GE7518@pendragon.ideasonboard.com>","From":"Umang Jain ","Message-ID":"","Date":"Tue, 8 Sep 2020 15:42:39 +0530","Mime-Version":"1.0","In-Reply-To":"<20200904153429.GE7518@pendragon.ideasonboard.com>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v6 2/2] android: jpeg: Support an\n\tinitial set of EXIF metadata tags","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":"multipart/mixed;\n\tboundary=\"===============5487054795241507207==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12375,"web_url":"https://patchwork.libcamera.org/comment/12375/","msgid":"<20200908120321.GC6047@pendragon.ideasonboard.com>","date":"2020-09-08T12:03:21","subject":"Re: [libcamera-devel] [PATCH v6 2/2] android: jpeg: Support an\n\tinitial set of EXIF metadata tags","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Tue, Sep 08, 2020 at 03:42:39PM +0530, Umang Jain wrote:\n> On 9/4/20 9:04 PM, Laurent Pinchart wrote:\n> > On Fri, Sep 04, 2020 at 12:10:19PM +0530, Umang Jain wrote:\n> >> Create a Exif object with various metadata tags set, just before\n> >> the encoder starts to encode the frame. The object is passed\n> >> directly as libcamera::Span<> to make sure EXIF tags can be set\n> >> in a single place i.e. in CameraDevice and the encoder only has\n> >> the job to write the data in the final output.\n> >>\n> >> Signed-off-by: Kieran Bingham \n> >> Signed-off-by: Umang Jain \n> >> Reviewed-by: Kieran Bingham \n> >> Reviewed-by: Laurent Pinchart \n> >> ---\n> >> src/android/camera_device.cpp | 19 ++++++++++-\n> >> src/android/jpeg/encoder.h | 3 +-\n> >> src/android/jpeg/encoder_libjpeg.cpp | 9 ++++-\n> >> src/android/jpeg/encoder_libjpeg.h | 3 +-\n> >> src/android/jpeg/exif.cpp | 50 ++++++++++++++++++++++++++++\n> >> src/android/jpeg/exif.h | 8 +++++\n> >> 6 files changed, 88 insertions(+), 4 deletions(-)\n> >>\n> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> >> index de6f86f..0328ac6 100644\n> >> --- a/src/android/camera_device.cpp\n> >> +++ b/src/android/camera_device.cpp\n> >> @@ -24,6 +24,7 @@\n> >> #include \"system/graphics.h\"\n> >> \n> >> #include \"jpeg/encoder_libjpeg.h\"\n> >> +#include \"jpeg/exif.h\"\n> >> \n> >> using namespace libcamera;\n> >> \n> >> @@ -1434,7 +1435,23 @@ void CameraDevice::requestComplete(Request *request)\n> >> \t\t\tcontinue;\n> >> \t\t}\n> >> \n> >> -\t\tint jpeg_size = encoder->encode(buffer, mapped.maps()[0]);\n> >> +\t\t/* Set EXIF metadata for various tags. */\n> >> +\t\tExif exif;\n> >> +\t\t/* \\todo Set Make and Model from external vendor tags. */\n> >> +\t\texif.setMake(\"libcamera\");\n> >> +\t\texif.setModel(\"cameraModel\");\n> >> +\t\texif.setOrientation(orientation_);\n> >> +\t\texif.setSize(cameraStream->size);\n> >> +\t\t/*\n> >> +\t\t * We set the frame's EXIF timestamp as the time of encode.\n> >> +\t\t * Since the precision we need for EXIF timestamp is only one\n> >> +\t\t * second, it is good enough.\n> >> +\t\t */\n> >> +\t\texif.setTimestamp(std::time(nullptr));\n> >> +\t\tif (exif.generate() != 0)\n> >> +\t\t\tLOG(HAL, Error) << \"Failed to get valid EXIF data\";\n> >\n> > s/get/generate/ ?\n> >\n> >> +\n> >> +\t\tint jpeg_size = encoder->encode(buffer, mapped.maps()[0], exif.data());\n> >> \t\tif (jpeg_size < 0) {\n> >> \t\t\tLOG(HAL, Error) << \"Failed to encode stream image\";\n> >> \t\t\tstatus = CAMERA3_BUFFER_STATUS_ERROR;\n> >> diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h\n> >> index f9eb88e..cf26d67 100644\n> >> --- a/src/android/jpeg/encoder.h\n> >> +++ b/src/android/jpeg/encoder.h\n> >> @@ -18,7 +18,8 @@ public:\n> >> \n> >> \tvirtual int configure(const libcamera::StreamConfiguration &cfg) = 0;\n> >> \tvirtual int encode(const libcamera::FrameBuffer *source,\n> >> -\t\t\t const libcamera::Span &destination) = 0;\n> >> +\t\t\t const libcamera::Span &destination,\n> >> +\t\t\t const libcamera::Span &exifData) = 0;\n> >> };\n> >> \n> >> #endif /* __ANDROID_JPEG_ENCODER_H__ */\n> >> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp\n> >> index 977538c..510613c 100644\n> >> --- a/src/android/jpeg/encoder_libjpeg.cpp\n> >> +++ b/src/android/jpeg/encoder_libjpeg.cpp\n> >> @@ -180,7 +180,8 @@ void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame)\n> >> }\n> >> \n> >> int EncoderLibJpeg::encode(const FrameBuffer *source,\n> >> -\t\t\t const libcamera::Span &dest)\n> >> +\t\t\t const libcamera::Span &dest,\n> >> +\t\t\t const libcamera::Span &exifData)\n> >> {\n> >> \tMappedFrameBuffer frame(source, PROT_READ);\n> >> \tif (!frame.isValid()) {\n> >> @@ -204,6 +205,12 @@ int EncoderLibJpeg::encode(const FrameBuffer *source,\n> >> \n> >> \tjpeg_start_compress(&compress_, TRUE);\n> >> \n> >> +\tif (exifData.size())\n> >> +\t\t/* Store Exif data in the JPEG_APP1 data block. */\n> >> +\t\tjpeg_write_marker(&compress_, JPEG_APP0 + 1,\n> >> +\t\t\t\t static_cast(exifData.data()),\n> >> +\t\t\t\t exifData.size());\n> >> +\n> >> \tLOG(JPEG, Debug) << \"JPEG Encode Starting:\" << compress_.image_width\n> >> \t\t\t << \"x\" << compress_.image_height;\n> >> \n> >> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h\n> >> index aed081a..1e8df05 100644\n> >> --- a/src/android/jpeg/encoder_libjpeg.h\n> >> +++ b/src/android/jpeg/encoder_libjpeg.h\n> >> @@ -22,7 +22,8 @@ public:\n> >> \n> >> \tint configure(const libcamera::StreamConfiguration &cfg) override;\n> >> \tint encode(const libcamera::FrameBuffer *source,\n> >> -\t\t const libcamera::Span &destination) override;\n> >> +\t\t const libcamera::Span &destination,\n> >> +\t\t const libcamera::Span &exifData) override;\n> >> \n> >> private:\n> >> \tvoid compressRGB(const libcamera::MappedBuffer *frame);\n> >> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp\n> >> index 41ddf48..36922ef 100644\n> >> --- a/src/android/jpeg/exif.cpp\n> >> +++ b/src/android/jpeg/exif.cpp\n> >> @@ -168,6 +168,56 @@ void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::str\n> >> \texif_entry_unref(entry);\n> >> }\n> >> \n> >> +void Exif::setMake(const std::string &make)\n> >> +{\n> >> +\tsetString(EXIF_IFD_0, EXIF_TAG_MAKE, EXIF_FORMAT_ASCII, make);\n> >> +}\n> >> +\n> >> +void Exif::setModel(const std::string &model)\n> >> +{\n> >> +\tsetString(EXIF_IFD_0, EXIF_TAG_MODEL, EXIF_FORMAT_ASCII, model);\n> >> +}\n> >> +\n> >> +void Exif::setSize(const Size &size)\n> >> +{\n> >> +\tsetLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_Y_DIMENSION, size.height);\n> >> +\tsetLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_X_DIMENSION, size.width);\n> >> +}\n> >> +\n> >> +void Exif::setTimestamp(time_t timestamp)\n> >> +{\n> >> +\tchar str[20];\n> >> +\tstd::strftime(str, sizeof(str), \"%Y:%m:%d %H:%M:%S\",\n> >> +\t\t std::localtime(×tamp));\n> >> +\tstd::string ts(str);\n> >> +\n> >> +\tsetString(EXIF_IFD_0, EXIF_TAG_DATE_TIME, EXIF_FORMAT_ASCII, ts);\n> >> +\tsetString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_ORIGINAL, EXIF_FORMAT_ASCII, ts);\n> >> +\tsetString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_DIGITIZED, EXIF_FORMAT_ASCII, ts);\n> >> +}\n> >> +\n> >> +void Exif::setOrientation(int orientation)\n> >> +{\n> >> +\tint value;\n> >> +\tswitch (orientation) {\n> >> +\tcase 0:\n> >> +\tdefault:\n> >> +\t\tvalue = 1;\n> >> +\t\tbreak;\n> >> +\tcase 90:\n> >> +\t\tvalue = 6;\n> >> +\t\tbreak;\n> >> +\tcase 180:\n> >> +\t\tvalue = 3;\n> >> +\t\tbreak;\n> >> +\tcase 270:\n> >> +\t\tvalue = 8;\n> >> +\t\tbreak;\n> >\n> > Shouldn't the the 90 and 270 values be swapped ? 6 means \"The 0th row is\n> > the visual right-hand side of the image, and the 0th column is the\n> > visual top\", and 8 means \"The 0th row is the visual left-hand side of\n> > the image, and the 0th column is the visual bottom\".\n> >\n> > Looking at the rotation property definition, the 90° rotation examples\n> > shows\n> >\n> > +-------------------------------------+\n> > | _ _ |\n> > | \\ / |\n> > | | | |\n> > | | | |\n> > | | > |\n> > | < | |\n> > | | | |\n> > | . |\n> > | V |\n> > +-------------------------------------+\n> >\n> > where the 0th row is the left side and the 0th column the bottom side.\n> >\n> > I can fix those two small issues when applying, but I'd appreciate if\n> > someone could double-check the rotation.\n>\n> Does the rotation property has to do anything with the fact that, camera \n> sensor(webcam use-case) is intentionally mounted upside down to avoid \n> rotation correction?\n\nThe rotation reported by libcamera exposes the optical rotation of the\ncamera module, combining both the camera sensor and the lens. It is\nindeed common to mount the camera sensor upside-down to compensate for\nthe lens inverting the image. In that case the reported rotation is 0°,\nas the camera sensor rotation and the lens inversion compensate each\nother.\n\n> A widely used example on the internet is a big 'F' \n> image denoting the value according to it's orientation [1], which is the \n> same as per the values used in the patch. On the other hand, I can also \n> find the code which satisfies Laurent's review and the EXIF \n> 'orientation' specification [2]\n> \n> [1]: \n> https://chromium.googlesource.com/chromiumos/platform2/+/HEAD/camera/common/exif_utils.cc#376\n> [2]: \n> https://piexif.readthedocs.io/en/latest/sample.html#rotate-image-by-exif-orientation\n\nThe 0° and 180° cases are fairly straightforward, they map to the EXIF\nvalues 1 and 3 respectively, there's usually no disagreement on that.\n\nThe 90° and 270° cases are much more error prone as it's easy to get the\nrotation direction wrong. The mapping between the 'F' orientation and\nthe EXIF values shown in [1] seems right to me.\n\nAndroid defines the rotation as the clockwise angle by which the image\nneeds to be rotated to appear in the correct orientation on the device\nscreen.\n\nFor EXIF value 6,\n\n88\n88 88\n8888888888\n\nthe image needs to be rotated by 90° clockwise, while for EXIF value 8,\n\n8888888888\n 88 88\n 88\n\nit needs to be rotated by 270°. The code in [2] thus appears correct.\nWhat however appears to be incorrect is our claim that the libcamera and\nAndroid numerical values are the same.\n\nJacopo, any opinion on that ?\n\n> ¯\\_(ツ)_/¯\n>\n> >> +\t}\n> >> +\n> >> +\tsetShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value);\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 8dfc324..622de4c 100644\n> >> --- a/src/android/jpeg/exif.h\n> >> +++ b/src/android/jpeg/exif.h\n> >> @@ -12,6 +12,7 @@\n> >> \n> >> #include \n> >> \n> >> +#include \n> >> #include \n> >> \n> >> class Exif\n> >> @@ -20,6 +21,13 @@ public:\n> >> \tExif();\n> >> \t~Exif();\n> >> \n> >> +\tvoid setMake(const std::string &make);\n> >> +\tvoid setModel(const std::string &model);\n> >> +\n> >> +\tvoid setOrientation(int orientation);\n> >> +\tvoid setSize(const libcamera::Size &size);\n> >> +\tvoid setTimestamp(time_t timestamp);\n> >> +\n> >> \tlibcamera::Span data() const { return { exifData_, size_ }; }\n> >> \t[[nodiscard]] int generate();\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 63E8CBDB1D\n\tfor ;\n\tTue, 8 Sep 2020 12:03:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EC38B62C33;\n\tTue, 8 Sep 2020 14:03:47 +0200 (CEST)","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 7507D62C30\n\tfor ;\n\tTue, 8 Sep 2020 14:03:46 +0200 (CEST)","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 E47C339;\n\tTue, 8 Sep 2020 14:03:45 +0200 (CEST)"],"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=\"O5+MEzIB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1599566626;\n\tbh=fOwcE2VulLdbjFMFh042sFLgmH2aj9v06nv3lTzrkCI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=O5+MEzIBeIzjc5p5pZ+FG6hiGhQIi4KAIRyXnxUMFMehHFALaMXHi0mJSQWIXWhzb\n\tRaSbgwlXYL24xmFLdnZdyqL8rg2GCIKHTchoSIiIF8896hF9eFtJ6GEXYgC9VlAGb2\n\tuSRrR6kxkxunkQr4/h2cLdExF+IgKFgRHZ6PxFac=","Date":"Tue, 8 Sep 2020 15:03:21 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"<20200908120321.GC6047@pendragon.ideasonboard.com>","References":"<20200904064019.29869-1-email@uajain.com>\n\t<20200904153429.GE7518@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v6 2/2] android: jpeg: Support an\n\tinitial set of EXIF metadata tags","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":12380,"web_url":"https://patchwork.libcamera.org/comment/12380/","msgid":"<20200908124346.rhxugbmjaw3eao2o@uno.localdomain>","date":"2020-09-08T12:43:46","subject":"Re: [libcamera-devel] [PATCH v6 2/2] android: jpeg: Support an\n\tinitial set of EXIF metadata tags","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent, Umang,\n\nOn Tue, Sep 08, 2020 at 03:03:21PM +0300, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> On Tue, Sep 08, 2020 at 03:42:39PM +0530, Umang Jain wrote:\n> > On 9/4/20 9:04 PM, Laurent Pinchart wrote:\n> > > On Fri, Sep 04, 2020 at 12:10:19PM +0530, Umang Jain wrote:\n> > >> Create a Exif object with various metadata tags set, just before\n> > >> the encoder starts to encode the frame. The object is passed\n> > >> directly as libcamera::Span<> to make sure EXIF tags can be set\n> > >> in a single place i.e. in CameraDevice and the encoder only has\n> > >> the job to write the data in the final output.\n> > >>\n> > >> Signed-off-by: Kieran Bingham \n> > >> Signed-off-by: Umang Jain \n> > >> Reviewed-by: Kieran Bingham \n> > >> Reviewed-by: Laurent Pinchart \n> > >> ---\n> > >> src/android/camera_device.cpp | 19 ++++++++++-\n> > >> src/android/jpeg/encoder.h | 3 +-\n> > >> src/android/jpeg/encoder_libjpeg.cpp | 9 ++++-\n> > >> src/android/jpeg/encoder_libjpeg.h | 3 +-\n> > >> src/android/jpeg/exif.cpp | 50 ++++++++++++++++++++++++++++\n> > >> src/android/jpeg/exif.h | 8 +++++\n> > >> 6 files changed, 88 insertions(+), 4 deletions(-)\n> > >>\n> > >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > >> index de6f86f..0328ac6 100644\n> > >> --- a/src/android/camera_device.cpp\n> > >> +++ b/src/android/camera_device.cpp\n> > >> @@ -24,6 +24,7 @@\n> > >> #include \"system/graphics.h\"\n> > >>\n> > >> #include \"jpeg/encoder_libjpeg.h\"\n> > >> +#include \"jpeg/exif.h\"\n> > >>\n> > >> using namespace libcamera;\n> > >>\n> > >> @@ -1434,7 +1435,23 @@ void CameraDevice::requestComplete(Request *request)\n> > >> \t\t\tcontinue;\n> > >> \t\t}\n> > >>\n> > >> -\t\tint jpeg_size = encoder->encode(buffer, mapped.maps()[0]);\n> > >> +\t\t/* Set EXIF metadata for various tags. */\n> > >> +\t\tExif exif;\n> > >> +\t\t/* \\todo Set Make and Model from external vendor tags. */\n> > >> +\t\texif.setMake(\"libcamera\");\n> > >> +\t\texif.setModel(\"cameraModel\");\n> > >> +\t\texif.setOrientation(orientation_);\n> > >> +\t\texif.setSize(cameraStream->size);\n> > >> +\t\t/*\n> > >> +\t\t * We set the frame's EXIF timestamp as the time of encode.\n> > >> +\t\t * Since the precision we need for EXIF timestamp is only one\n> > >> +\t\t * second, it is good enough.\n> > >> +\t\t */\n> > >> +\t\texif.setTimestamp(std::time(nullptr));\n> > >> +\t\tif (exif.generate() != 0)\n> > >> +\t\t\tLOG(HAL, Error) << \"Failed to get valid EXIF data\";\n> > >\n> > > s/get/generate/ ?\n> > >\n> > >> +\n> > >> +\t\tint jpeg_size = encoder->encode(buffer, mapped.maps()[0], exif.data());\n> > >> \t\tif (jpeg_size < 0) {\n> > >> \t\t\tLOG(HAL, Error) << \"Failed to encode stream image\";\n> > >> \t\t\tstatus = CAMERA3_BUFFER_STATUS_ERROR;\n> > >> diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h\n> > >> index f9eb88e..cf26d67 100644\n> > >> --- a/src/android/jpeg/encoder.h\n> > >> +++ b/src/android/jpeg/encoder.h\n> > >> @@ -18,7 +18,8 @@ public:\n> > >>\n> > >> \tvirtual int configure(const libcamera::StreamConfiguration &cfg) = 0;\n> > >> \tvirtual int encode(const libcamera::FrameBuffer *source,\n> > >> -\t\t\t const libcamera::Span &destination) = 0;\n> > >> +\t\t\t const libcamera::Span &destination,\n> > >> +\t\t\t const libcamera::Span &exifData) = 0;\n> > >> };\n> > >>\n> > >> #endif /* __ANDROID_JPEG_ENCODER_H__ */\n> > >> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp\n> > >> index 977538c..510613c 100644\n> > >> --- a/src/android/jpeg/encoder_libjpeg.cpp\n> > >> +++ b/src/android/jpeg/encoder_libjpeg.cpp\n> > >> @@ -180,7 +180,8 @@ void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame)\n> > >> }\n> > >>\n> > >> int EncoderLibJpeg::encode(const FrameBuffer *source,\n> > >> -\t\t\t const libcamera::Span &dest)\n> > >> +\t\t\t const libcamera::Span &dest,\n> > >> +\t\t\t const libcamera::Span &exifData)\n> > >> {\n> > >> \tMappedFrameBuffer frame(source, PROT_READ);\n> > >> \tif (!frame.isValid()) {\n> > >> @@ -204,6 +205,12 @@ int EncoderLibJpeg::encode(const FrameBuffer *source,\n> > >>\n> > >> \tjpeg_start_compress(&compress_, TRUE);\n> > >>\n> > >> +\tif (exifData.size())\n> > >> +\t\t/* Store Exif data in the JPEG_APP1 data block. */\n> > >> +\t\tjpeg_write_marker(&compress_, JPEG_APP0 + 1,\n> > >> +\t\t\t\t static_cast(exifData.data()),\n> > >> +\t\t\t\t exifData.size());\n> > >> +\n> > >> \tLOG(JPEG, Debug) << \"JPEG Encode Starting:\" << compress_.image_width\n> > >> \t\t\t << \"x\" << compress_.image_height;\n> > >>\n> > >> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h\n> > >> index aed081a..1e8df05 100644\n> > >> --- a/src/android/jpeg/encoder_libjpeg.h\n> > >> +++ b/src/android/jpeg/encoder_libjpeg.h\n> > >> @@ -22,7 +22,8 @@ public:\n> > >>\n> > >> \tint configure(const libcamera::StreamConfiguration &cfg) override;\n> > >> \tint encode(const libcamera::FrameBuffer *source,\n> > >> -\t\t const libcamera::Span &destination) override;\n> > >> +\t\t const libcamera::Span &destination,\n> > >> +\t\t const libcamera::Span &exifData) override;\n> > >>\n> > >> private:\n> > >> \tvoid compressRGB(const libcamera::MappedBuffer *frame);\n> > >> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp\n> > >> index 41ddf48..36922ef 100644\n> > >> --- a/src/android/jpeg/exif.cpp\n> > >> +++ b/src/android/jpeg/exif.cpp\n> > >> @@ -168,6 +168,56 @@ void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::str\n> > >> \texif_entry_unref(entry);\n> > >> }\n> > >>\n> > >> +void Exif::setMake(const std::string &make)\n> > >> +{\n> > >> +\tsetString(EXIF_IFD_0, EXIF_TAG_MAKE, EXIF_FORMAT_ASCII, make);\n> > >> +}\n> > >> +\n> > >> +void Exif::setModel(const std::string &model)\n> > >> +{\n> > >> +\tsetString(EXIF_IFD_0, EXIF_TAG_MODEL, EXIF_FORMAT_ASCII, model);\n> > >> +}\n> > >> +\n> > >> +void Exif::setSize(const Size &size)\n> > >> +{\n> > >> +\tsetLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_Y_DIMENSION, size.height);\n> > >> +\tsetLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_X_DIMENSION, size.width);\n> > >> +}\n> > >> +\n> > >> +void Exif::setTimestamp(time_t timestamp)\n> > >> +{\n> > >> +\tchar str[20];\n> > >> +\tstd::strftime(str, sizeof(str), \"%Y:%m:%d %H:%M:%S\",\n> > >> +\t\t std::localtime(×tamp));\n> > >> +\tstd::string ts(str);\n> > >> +\n> > >> +\tsetString(EXIF_IFD_0, EXIF_TAG_DATE_TIME, EXIF_FORMAT_ASCII, ts);\n> > >> +\tsetString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_ORIGINAL, EXIF_FORMAT_ASCII, ts);\n> > >> +\tsetString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_DIGITIZED, EXIF_FORMAT_ASCII, ts);\n> > >> +}\n> > >> +\n> > >> +void Exif::setOrientation(int orientation)\n> > >> +{\n> > >> +\tint value;\n> > >> +\tswitch (orientation) {\n> > >> +\tcase 0:\n> > >> +\tdefault:\n> > >> +\t\tvalue = 1;\n> > >> +\t\tbreak;\n> > >> +\tcase 90:\n> > >> +\t\tvalue = 6;\n> > >> +\t\tbreak;\n> > >> +\tcase 180:\n> > >> +\t\tvalue = 3;\n> > >> +\t\tbreak;\n> > >> +\tcase 270:\n> > >> +\t\tvalue = 8;\n> > >> +\t\tbreak;\n> > >\n> > > Shouldn't the the 90 and 270 values be swapped ? 6 means \"The 0th row is\n> > > the visual right-hand side of the image, and the 0th column is the\n> > > visual top\", and 8 means \"The 0th row is the visual left-hand side of\n> > > the image, and the 0th column is the visual bottom\".\n> > >\n> > > Looking at the rotation property definition, the 90° rotation examples\n> > > shows\n> > >\n> > > +-------------------------------------+\n> > > | _ _ |\n> > > | \\ / |\n> > > | | | |\n> > > | | | |\n> > > | | > |\n> > > | < | |\n> > > | | | |\n> > > | . |\n> > > | V |\n> > > +-------------------------------------+\n> > >\n> > > where the 0th row is the left side and the 0th column the bottom side.\n> > >\n\nIsn't the 0-th row (the first row of the image) on the 'visual\nright-hand side' ?\n\n> > > I can fix those two small issues when applying, but I'd appreciate if\n> > > someone could double-check the rotation.\n> >\n> > Does the rotation property has to do anything with the fact that, camera\n> > sensor(webcam use-case) is intentionally mounted upside down to avoid\n> > rotation correction?\n>\n> The rotation reported by libcamera exposes the optical rotation of the\n> camera module, combining both the camera sensor and the lens. It is\n> indeed common to mount the camera sensor upside-down to compensate for\n> the lens inverting the image. In that case the reported rotation is 0°,\n> as the camera sensor rotation and the lens inversion compensate each\n> other.\n>\n\nThe 'rotation' property definition takes into account the lens\ninversion, so you shouldn't have to care about it\n\n> > A widely used example on the internet is a big 'F'\n> > image denoting the value according to it's orientation [1], which is the\n> > same as per the values used in the patch. On the other hand, I can also\n> > find the code which satisfies Laurent's review and the EXIF\n> > 'orientation' specification [2]\n> >\n> > [1]:\n> > https://chromium.googlesource.com/chromiumos/platform2/+/HEAD/camera/common/exif_utils.cc#376\n> > [2]:\n> > https://piexif.readthedocs.io/en/latest/sample.html#rotate-image-by-exif-orientation\n>\n> The 0° and 180° cases are fairly straightforward, they map to the EXIF\n> values 1 and 3 respectively, there's usually no disagreement on that.\n>\n> The 90° and 270° cases are much more error prone as it's easy to get the\n> rotation direction wrong. The mapping between the 'F' orientation and\n> the EXIF values shown in [1] seems right to me.\n>\n> Android defines the rotation as the clockwise angle by which the image\n> needs to be rotated to appear in the correct orientation on the device\n> screen.\n>\n> For EXIF value 6,\n>\n> 88\n> 88 88\n> 8888888888\n>\n> the image needs to be rotated by 90° clockwise, while for EXIF value 8,\n>\n> 8888888888\n> 88 88\n> 88\n>\n> it needs to be rotated by 270°. The code in [2] thus appears correct.\n> What however appears to be incorrect is our claim that the libcamera and\n> Android numerical values are the same.\n>\n> Jacopo, any opinion on that ?\n>\n\nOur definition differs from the Android one, as the rotation value we\nreport is the rotation correction degrees in counter-clockwise direction.\nAndroid defines that in the clockwise direction.\n\nIf I got the above discussion right and looking at the 'F' examples in\n[1], we should report:\n 90 degrees = 8\n 270 degrees = 6\n\nHowever, this interpreation and the 'F' examples, don't match the\ntextual description Laurent reported above:\n\n\n-------------------------------------------------------------------------------\n Shouldn't the the 90 and 270 values be swapped ? 6 means \"The 0th row is\n the visual right-hand side of the image, and the 0th column is the\n visual top\", and 8 means \"The 0th row is the visual left-hand side of\n the image, and the 0th column is the visual bottom\".\n------------------------------------------------------------------------------\n\n For EXIF value 6,\n\n 88\n 88 88\n 8888888888\n\n the image needs to be rotated by 90° clockwise, while for EXIF value 8,\n\n 8888888888\n 88 88\n 88\n------------------------------------------------------------------------------\n\nThe two do not match, and I wonder what's the authoritative source for\nthe definition of the semantic associated with the '6' and '8'\nvalues.\n\nThanks\n j\n\n> > ¯\\_(ツ)_/¯\n> >\n> > >> +\t}\n> > >> +\n> > >> +\tsetShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value);\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 8dfc324..622de4c 100644\n> > >> --- a/src/android/jpeg/exif.h\n> > >> +++ b/src/android/jpeg/exif.h\n> > >> @@ -12,6 +12,7 @@\n> > >>\n> > >> #include \n> > >>\n> > >> +#include \n> > >> #include \n> > >>\n> > >> class Exif\n> > >> @@ -20,6 +21,13 @@ public:\n> > >> \tExif();\n> > >> \t~Exif();\n> > >>\n> > >> +\tvoid setMake(const std::string &make);\n> > >> +\tvoid setModel(const std::string &model);\n> > >> +\n> > >> +\tvoid setOrientation(int orientation);\n> > >> +\tvoid setSize(const libcamera::Size &size);\n> > >> +\tvoid setTimestamp(time_t timestamp);\n> > >> +\n> > >> \tlibcamera::Span data() const { return { exifData_, size_ }; }\n> > >> \t[[nodiscard]] int generate();\n> > >>\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 17B61BDB1D\n\tfor ;\n\tTue, 8 Sep 2020 12:40:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9803262C47;\n\tTue, 8 Sep 2020 14:40:00 +0200 (CEST)","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 DAA4D6056C\n\tfor ;\n\tTue, 8 Sep 2020 14:39:58 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id 815F1240003;\n\tTue, 8 Sep 2020 12:39:57 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Tue, 8 Sep 2020 14:43:46 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20200908124346.rhxugbmjaw3eao2o@uno.localdomain>","References":"<20200904064019.29869-1-email@uajain.com>\n\t<20200904153429.GE7518@pendragon.ideasonboard.com>\n\t\n\t<20200908120321.GC6047@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200908120321.GC6047@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v6 2/2] android: jpeg: Support an\n\tinitial set of EXIF metadata tags","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":12381,"web_url":"https://patchwork.libcamera.org/comment/12381/","msgid":"<20200908124614.GI6047@pendragon.ideasonboard.com>","date":"2020-09-08T12:46:14","subject":"Re: [libcamera-devel] [PATCH v6 2/2] android: jpeg: Support an\n\tinitial set of EXIF metadata tags","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Tue, Sep 08, 2020 at 02:43:46PM +0200, Jacopo Mondi wrote:\n> On Tue, Sep 08, 2020 at 03:03:21PM +0300, Laurent Pinchart wrote:\n> > On Tue, Sep 08, 2020 at 03:42:39PM +0530, Umang Jain wrote:\n> >> On 9/4/20 9:04 PM, Laurent Pinchart wrote:\n> >>> On Fri, Sep 04, 2020 at 12:10:19PM +0530, Umang Jain wrote:\n> >>>> Create a Exif object with various metadata tags set, just before\n> >>>> the encoder starts to encode the frame. The object is passed\n> >>>> directly as libcamera::Span<> to make sure EXIF tags can be set\n> >>>> in a single place i.e. in CameraDevice and the encoder only has\n> >>>> the job to write the data in the final output.\n> >>>>\n> >>>> Signed-off-by: Kieran Bingham \n> >>>> Signed-off-by: Umang Jain \n> >>>> Reviewed-by: Kieran Bingham \n> >>>> Reviewed-by: Laurent Pinchart \n> >>>> ---\n> >>>> src/android/camera_device.cpp | 19 ++++++++++-\n> >>>> src/android/jpeg/encoder.h | 3 +-\n> >>>> src/android/jpeg/encoder_libjpeg.cpp | 9 ++++-\n> >>>> src/android/jpeg/encoder_libjpeg.h | 3 +-\n> >>>> src/android/jpeg/exif.cpp | 50 ++++++++++++++++++++++++++++\n> >>>> src/android/jpeg/exif.h | 8 +++++\n> >>>> 6 files changed, 88 insertions(+), 4 deletions(-)\n> >>>>\n> >>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> >>>> index de6f86f..0328ac6 100644\n> >>>> --- a/src/android/camera_device.cpp\n> >>>> +++ b/src/android/camera_device.cpp\n> >>>> @@ -24,6 +24,7 @@\n> >>>> #include \"system/graphics.h\"\n> >>>>\n> >>>> #include \"jpeg/encoder_libjpeg.h\"\n> >>>> +#include \"jpeg/exif.h\"\n> >>>>\n> >>>> using namespace libcamera;\n> >>>>\n> >>>> @@ -1434,7 +1435,23 @@ void CameraDevice::requestComplete(Request *request)\n> >>>> \t\t\tcontinue;\n> >>>> \t\t}\n> >>>>\n> >>>> -\t\tint jpeg_size = encoder->encode(buffer, mapped.maps()[0]);\n> >>>> +\t\t/* Set EXIF metadata for various tags. */\n> >>>> +\t\tExif exif;\n> >>>> +\t\t/* \\todo Set Make and Model from external vendor tags. */\n> >>>> +\t\texif.setMake(\"libcamera\");\n> >>>> +\t\texif.setModel(\"cameraModel\");\n> >>>> +\t\texif.setOrientation(orientation_);\n> >>>> +\t\texif.setSize(cameraStream->size);\n> >>>> +\t\t/*\n> >>>> +\t\t * We set the frame's EXIF timestamp as the time of encode.\n> >>>> +\t\t * Since the precision we need for EXIF timestamp is only one\n> >>>> +\t\t * second, it is good enough.\n> >>>> +\t\t */\n> >>>> +\t\texif.setTimestamp(std::time(nullptr));\n> >>>> +\t\tif (exif.generate() != 0)\n> >>>> +\t\t\tLOG(HAL, Error) << \"Failed to get valid EXIF data\";\n> >>>\n> >>> s/get/generate/ ?\n> >>>\n> >>>> +\n> >>>> +\t\tint jpeg_size = encoder->encode(buffer, mapped.maps()[0], exif.data());\n> >>>> \t\tif (jpeg_size < 0) {\n> >>>> \t\t\tLOG(HAL, Error) << \"Failed to encode stream image\";\n> >>>> \t\t\tstatus = CAMERA3_BUFFER_STATUS_ERROR;\n> >>>> diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h\n> >>>> index f9eb88e..cf26d67 100644\n> >>>> --- a/src/android/jpeg/encoder.h\n> >>>> +++ b/src/android/jpeg/encoder.h\n> >>>> @@ -18,7 +18,8 @@ public:\n> >>>>\n> >>>> \tvirtual int configure(const libcamera::StreamConfiguration &cfg) = 0;\n> >>>> \tvirtual int encode(const libcamera::FrameBuffer *source,\n> >>>> -\t\t\t const libcamera::Span &destination) = 0;\n> >>>> +\t\t\t const libcamera::Span &destination,\n> >>>> +\t\t\t const libcamera::Span &exifData) = 0;\n> >>>> };\n> >>>>\n> >>>> #endif /* __ANDROID_JPEG_ENCODER_H__ */\n> >>>> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp\n> >>>> index 977538c..510613c 100644\n> >>>> --- a/src/android/jpeg/encoder_libjpeg.cpp\n> >>>> +++ b/src/android/jpeg/encoder_libjpeg.cpp\n> >>>> @@ -180,7 +180,8 @@ void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame)\n> >>>> }\n> >>>>\n> >>>> int EncoderLibJpeg::encode(const FrameBuffer *source,\n> >>>> -\t\t\t const libcamera::Span &dest)\n> >>>> +\t\t\t const libcamera::Span &dest,\n> >>>> +\t\t\t const libcamera::Span &exifData)\n> >>>> {\n> >>>> \tMappedFrameBuffer frame(source, PROT_READ);\n> >>>> \tif (!frame.isValid()) {\n> >>>> @@ -204,6 +205,12 @@ int EncoderLibJpeg::encode(const FrameBuffer *source,\n> >>>>\n> >>>> \tjpeg_start_compress(&compress_, TRUE);\n> >>>>\n> >>>> +\tif (exifData.size())\n> >>>> +\t\t/* Store Exif data in the JPEG_APP1 data block. */\n> >>>> +\t\tjpeg_write_marker(&compress_, JPEG_APP0 + 1,\n> >>>> +\t\t\t\t static_cast(exifData.data()),\n> >>>> +\t\t\t\t exifData.size());\n> >>>> +\n> >>>> \tLOG(JPEG, Debug) << \"JPEG Encode Starting:\" << compress_.image_width\n> >>>> \t\t\t << \"x\" << compress_.image_height;\n> >>>>\n> >>>> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h\n> >>>> index aed081a..1e8df05 100644\n> >>>> --- a/src/android/jpeg/encoder_libjpeg.h\n> >>>> +++ b/src/android/jpeg/encoder_libjpeg.h\n> >>>> @@ -22,7 +22,8 @@ public:\n> >>>>\n> >>>> \tint configure(const libcamera::StreamConfiguration &cfg) override;\n> >>>> \tint encode(const libcamera::FrameBuffer *source,\n> >>>> -\t\t const libcamera::Span &destination) override;\n> >>>> +\t\t const libcamera::Span &destination,\n> >>>> +\t\t const libcamera::Span &exifData) override;\n> >>>>\n> >>>> private:\n> >>>> \tvoid compressRGB(const libcamera::MappedBuffer *frame);\n> >>>> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp\n> >>>> index 41ddf48..36922ef 100644\n> >>>> --- a/src/android/jpeg/exif.cpp\n> >>>> +++ b/src/android/jpeg/exif.cpp\n> >>>> @@ -168,6 +168,56 @@ void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::str\n> >>>> \texif_entry_unref(entry);\n> >>>> }\n> >>>>\n> >>>> +void Exif::setMake(const std::string &make)\n> >>>> +{\n> >>>> +\tsetString(EXIF_IFD_0, EXIF_TAG_MAKE, EXIF_FORMAT_ASCII, make);\n> >>>> +}\n> >>>> +\n> >>>> +void Exif::setModel(const std::string &model)\n> >>>> +{\n> >>>> +\tsetString(EXIF_IFD_0, EXIF_TAG_MODEL, EXIF_FORMAT_ASCII, model);\n> >>>> +}\n> >>>> +\n> >>>> +void Exif::setSize(const Size &size)\n> >>>> +{\n> >>>> +\tsetLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_Y_DIMENSION, size.height);\n> >>>> +\tsetLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_X_DIMENSION, size.width);\n> >>>> +}\n> >>>> +\n> >>>> +void Exif::setTimestamp(time_t timestamp)\n> >>>> +{\n> >>>> +\tchar str[20];\n> >>>> +\tstd::strftime(str, sizeof(str), \"%Y:%m:%d %H:%M:%S\",\n> >>>> +\t\t std::localtime(×tamp));\n> >>>> +\tstd::string ts(str);\n> >>>> +\n> >>>> +\tsetString(EXIF_IFD_0, EXIF_TAG_DATE_TIME, EXIF_FORMAT_ASCII, ts);\n> >>>> +\tsetString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_ORIGINAL, EXIF_FORMAT_ASCII, ts);\n> >>>> +\tsetString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_DIGITIZED, EXIF_FORMAT_ASCII, ts);\n> >>>> +}\n> >>>> +\n> >>>> +void Exif::setOrientation(int orientation)\n> >>>> +{\n> >>>> +\tint value;\n> >>>> +\tswitch (orientation) {\n> >>>> +\tcase 0:\n> >>>> +\tdefault:\n> >>>> +\t\tvalue = 1;\n> >>>> +\t\tbreak;\n> >>>> +\tcase 90:\n> >>>> +\t\tvalue = 6;\n> >>>> +\t\tbreak;\n> >>>> +\tcase 180:\n> >>>> +\t\tvalue = 3;\n> >>>> +\t\tbreak;\n> >>>> +\tcase 270:\n> >>>> +\t\tvalue = 8;\n> >>>> +\t\tbreak;\n> >>>\n> >>> Shouldn't the the 90 and 270 values be swapped ? 6 means \"The 0th row is\n> >>> the visual right-hand side of the image, and the 0th column is the\n> >>> visual top\", and 8 means \"The 0th row is the visual left-hand side of\n> >>> the image, and the 0th column is the visual bottom\".\n> >>>\n> >>> Looking at the rotation property definition, the 90° rotation examples\n> >>> shows\n> >>>\n> >>> +-------------------------------------+\n> >>> | _ _ |\n> >>> | \\ / |\n> >>> | | | |\n> >>> | | | |\n> >>> | | > |\n> >>> | < | |\n> >>> | | | |\n> >>> | . |\n> >>> | V |\n> >>> +-------------------------------------+\n> >>>\n> >>> where the 0th row is the left side and the 0th column the bottom side.\n> \n> Isn't the 0-th row (the first row of the image) on the 'visual\n> right-hand side' ?\n\nHere's my interpretation:\n\nLooking at the fish in front of you (forgetting the camera), the visual\nright-hand side is the head, and the visual left-hand side is the tail.\nThe 0th row of the image (now we're talking about the buffer, as\nnumbering rows implies we're counting pixels, so it's the buffer, not\nthe scene anymore). is the tail, thus the visual left-hand side of the\nimage.\n\n> >>> I can fix those two small issues when applying, but I'd appreciate if\n> >>> someone could double-check the rotation.\n> >>\n> >> Does the rotation property has to do anything with the fact that, camera\n> >> sensor(webcam use-case) is intentionally mounted upside down to avoid\n> >> rotation correction?\n> >\n> > The rotation reported by libcamera exposes the optical rotation of the\n> > camera module, combining both the camera sensor and the lens. It is\n> > indeed common to mount the camera sensor upside-down to compensate for\n> > the lens inverting the image. In that case the reported rotation is 0°,\n> > as the camera sensor rotation and the lens inversion compensate each\n> > other.\n> \n> The 'rotation' property definition takes into account the lens\n> inversion, so you shouldn't have to care about it\n\nI'm not sure if this means you agree or disagree with my explanation :-)\n\n> >> A widely used example on the internet is a big 'F'\n> >> image denoting the value according to it's orientation [1], which is the\n> >> same as per the values used in the patch. On the other hand, I can also\n> >> find the code which satisfies Laurent's review and the EXIF\n> >> 'orientation' specification [2]\n> >>\n> >> [1]:\n> >> https://chromium.googlesource.com/chromiumos/platform2/+/HEAD/camera/common/exif_utils.cc#376\n> >> [2]:\n> >> https://piexif.readthedocs.io/en/latest/sample.html#rotate-image-by-exif-orientation\n> >\n> > The 0° and 180° cases are fairly straightforward, they map to the EXIF\n> > values 1 and 3 respectively, there's usually no disagreement on that.\n> >\n> > The 90° and 270° cases are much more error prone as it's easy to get the\n> > rotation direction wrong. The mapping between the 'F' orientation and\n> > the EXIF values shown in [1] seems right to me.\n> >\n> > Android defines the rotation as the clockwise angle by which the image\n> > needs to be rotated to appear in the correct orientation on the device\n> > screen.\n> >\n> > For EXIF value 6,\n> >\n> > 88\n> > 88 88\n> > 8888888888\n> >\n> > the image needs to be rotated by 90° clockwise, while for EXIF value 8,\n> >\n> > 8888888888\n> > 88 88\n> > 88\n> >\n> > it needs to be rotated by 270°. The code in [2] thus appears correct.\n> > What however appears to be incorrect is our claim that the libcamera and\n> > Android numerical values are the same.\n> >\n> > Jacopo, any opinion on that ?\n> \n> Our definition differs from the Android one, as the rotation value we\n> report is the rotation correction degrees in counter-clockwise direction.\n> Android defines that in the clockwise direction.\n\nSo there's a bug in our HAL implementation when reporting the rotation ?\n\n> If I got the above discussion right and looking at the 'F' examples in\n> [1], we should report:\n> 90 degrees = 8\n> 270 degrees = 6\n> \n> However, this interpreation and the 'F' examples, don't match the\n> textual description Laurent reported above:\n> \n> -------------------------------------------------------------------------------\n> Shouldn't the the 90 and 270 values be swapped ? 6 means \"The 0th row is\n> the visual right-hand side of the image, and the 0th column is the\n> visual top\", and 8 means \"The 0th row is the visual left-hand side of\n> the image, and the 0th column is the visual bottom\".\n> ------------------------------------------------------------------------------\n> \n> For EXIF value 6,\n> \n> 88\n> 88 88\n> 8888888888\n> \n> the image needs to be rotated by 90° clockwise, while for EXIF value 8,\n> \n> 8888888888\n> 88 88\n> 88\n> ------------------------------------------------------------------------------\n> \n> The two do not match, and I wonder what's the authoritative source for\n> the definition of the semantic associated with the '6' and '8'\n> values.\n\nThe EXIF specification is the authoritative source, and it's defined as\nI've quoted.\n\n> >> ¯\\_(ツ)_/¯\n> >>\n> >>>> +\t}\n> >>>> +\n> >>>> +\tsetShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value);\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 8dfc324..622de4c 100644\n> >>>> --- a/src/android/jpeg/exif.h\n> >>>> +++ b/src/android/jpeg/exif.h\n> >>>> @@ -12,6 +12,7 @@\n> >>>>\n> >>>> #include \n> >>>>\n> >>>> +#include \n> >>>> #include \n> >>>>\n> >>>> class Exif\n> >>>> @@ -20,6 +21,13 @@ public:\n> >>>> \tExif();\n> >>>> \t~Exif();\n> >>>>\n> >>>> +\tvoid setMake(const std::string &make);\n> >>>> +\tvoid setModel(const std::string &model);\n> >>>> +\n> >>>> +\tvoid setOrientation(int orientation);\n> >>>> +\tvoid setSize(const libcamera::Size &size);\n> >>>> +\tvoid setTimestamp(time_t timestamp);\n> >>>> +\n> >>>> \tlibcamera::Span data() const { return { exifData_, size_ }; }\n> >>>> \t[[nodiscard]] int generate();\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 0182EBDB1C\n\tfor ;\n\tTue, 8 Sep 2020 12:46:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6D9D462C66;\n\tTue, 8 Sep 2020 14:46:47 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6EF596036E\n\tfor ;\n\tTue, 8 Sep 2020 14:46:46 +0200 (CEST)","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 0AB601243;\n\tTue, 8 Sep 2020 14:46:38 +0200 (CEST)"],"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=\"DfuOmP6A\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1599569199;\n\tbh=vsj8QeBM7NB6SE+hkflFZqxzuxtmLqaSLazo2R3m7sg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DfuOmP6Ap60b3/UUsp9ImTFF3NBBWD1C1xSvjMe43pYChrDJuLH+AGyFBwboGrThF\n\tVTrKO51kp3CLbz/879Je2lh3bGmBtH4kz9WgbdIC/IDa3i2cmdm9viFaQljyTO6Il8\n\tosCmCQVqZ9admptaEcuxTB4qTZur8VXrF7WcwRx0=","Date":"Tue, 8 Sep 2020 15:46:14 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20200908124614.GI6047@pendragon.ideasonboard.com>","References":"<20200904064019.29869-1-email@uajain.com>\n\t<20200904153429.GE7518@pendragon.ideasonboard.com>\n\t\n\t<20200908120321.GC6047@pendragon.ideasonboard.com>\n\t<20200908124346.rhxugbmjaw3eao2o@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200908124346.rhxugbmjaw3eao2o@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v6 2/2] android: jpeg: Support an\n\tinitial set of EXIF metadata tags","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":12383,"web_url":"https://patchwork.libcamera.org/comment/12383/","msgid":"<20200908131425.6ou7b3zihbwqbxrq@uno.localdomain>","date":"2020-09-08T13:14:25","subject":"Re: [libcamera-devel] [PATCH v6 2/2] android: jpeg: Support an\n\tinitial set of EXIF metadata tags","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent\n\nOn Tue, Sep 08, 2020 at 03:46:14PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Tue, Sep 08, 2020 at 02:43:46PM +0200, Jacopo Mondi wrote:\n> > On Tue, Sep 08, 2020 at 03:03:21PM +0300, Laurent Pinchart wrote:\n> > > On Tue, Sep 08, 2020 at 03:42:39PM +0530, Umang Jain wrote:\n> > >> On 9/4/20 9:04 PM, Laurent Pinchart wrote:\n> > >>> On Fri, Sep 04, 2020 at 12:10:19PM +0530, Umang Jain wrote:\n> > >>>> Create a Exif object with various metadata tags set, just before\n> > >>>> the encoder starts to encode the frame. The object is passed\n> > >>>> directly as libcamera::Span<> to make sure EXIF tags can be set\n> > >>>> in a single place i.e. in CameraDevice and the encoder only has\n> > >>>> the job to write the data in the final output.\n> > >>>>\n> > >>>> Signed-off-by: Kieran Bingham \n> > >>>> Signed-off-by: Umang Jain \n> > >>>> Reviewed-by: Kieran Bingham \n> > >>>> Reviewed-by: Laurent Pinchart \n> > >>>> ---\n> > >>>> src/android/camera_device.cpp | 19 ++++++++++-\n> > >>>> src/android/jpeg/encoder.h | 3 +-\n> > >>>> src/android/jpeg/encoder_libjpeg.cpp | 9 ++++-\n> > >>>> src/android/jpeg/encoder_libjpeg.h | 3 +-\n> > >>>> src/android/jpeg/exif.cpp | 50 ++++++++++++++++++++++++++++\n> > >>>> src/android/jpeg/exif.h | 8 +++++\n> > >>>> 6 files changed, 88 insertions(+), 4 deletions(-)\n> > >>>>\n> > >>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > >>>> index de6f86f..0328ac6 100644\n> > >>>> --- a/src/android/camera_device.cpp\n> > >>>> +++ b/src/android/camera_device.cpp\n> > >>>> @@ -24,6 +24,7 @@\n> > >>>> #include \"system/graphics.h\"\n> > >>>>\n> > >>>> #include \"jpeg/encoder_libjpeg.h\"\n> > >>>> +#include \"jpeg/exif.h\"\n> > >>>>\n> > >>>> using namespace libcamera;\n> > >>>>\n> > >>>> @@ -1434,7 +1435,23 @@ void CameraDevice::requestComplete(Request *request)\n> > >>>> \t\t\tcontinue;\n> > >>>> \t\t}\n> > >>>>\n> > >>>> -\t\tint jpeg_size = encoder->encode(buffer, mapped.maps()[0]);\n> > >>>> +\t\t/* Set EXIF metadata for various tags. */\n> > >>>> +\t\tExif exif;\n> > >>>> +\t\t/* \\todo Set Make and Model from external vendor tags. */\n> > >>>> +\t\texif.setMake(\"libcamera\");\n> > >>>> +\t\texif.setModel(\"cameraModel\");\n> > >>>> +\t\texif.setOrientation(orientation_);\n> > >>>> +\t\texif.setSize(cameraStream->size);\n> > >>>> +\t\t/*\n> > >>>> +\t\t * We set the frame's EXIF timestamp as the time of encode.\n> > >>>> +\t\t * Since the precision we need for EXIF timestamp is only one\n> > >>>> +\t\t * second, it is good enough.\n> > >>>> +\t\t */\n> > >>>> +\t\texif.setTimestamp(std::time(nullptr));\n> > >>>> +\t\tif (exif.generate() != 0)\n> > >>>> +\t\t\tLOG(HAL, Error) << \"Failed to get valid EXIF data\";\n> > >>>\n> > >>> s/get/generate/ ?\n> > >>>\n> > >>>> +\n> > >>>> +\t\tint jpeg_size = encoder->encode(buffer, mapped.maps()[0], exif.data());\n> > >>>> \t\tif (jpeg_size < 0) {\n> > >>>> \t\t\tLOG(HAL, Error) << \"Failed to encode stream image\";\n> > >>>> \t\t\tstatus = CAMERA3_BUFFER_STATUS_ERROR;\n> > >>>> diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h\n> > >>>> index f9eb88e..cf26d67 100644\n> > >>>> --- a/src/android/jpeg/encoder.h\n> > >>>> +++ b/src/android/jpeg/encoder.h\n> > >>>> @@ -18,7 +18,8 @@ public:\n> > >>>>\n> > >>>> \tvirtual int configure(const libcamera::StreamConfiguration &cfg) = 0;\n> > >>>> \tvirtual int encode(const libcamera::FrameBuffer *source,\n> > >>>> -\t\t\t const libcamera::Span &destination) = 0;\n> > >>>> +\t\t\t const libcamera::Span &destination,\n> > >>>> +\t\t\t const libcamera::Span &exifData) = 0;\n> > >>>> };\n> > >>>>\n> > >>>> #endif /* __ANDROID_JPEG_ENCODER_H__ */\n> > >>>> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp\n> > >>>> index 977538c..510613c 100644\n> > >>>> --- a/src/android/jpeg/encoder_libjpeg.cpp\n> > >>>> +++ b/src/android/jpeg/encoder_libjpeg.cpp\n> > >>>> @@ -180,7 +180,8 @@ void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame)\n> > >>>> }\n> > >>>>\n> > >>>> int EncoderLibJpeg::encode(const FrameBuffer *source,\n> > >>>> -\t\t\t const libcamera::Span &dest)\n> > >>>> +\t\t\t const libcamera::Span &dest,\n> > >>>> +\t\t\t const libcamera::Span &exifData)\n> > >>>> {\n> > >>>> \tMappedFrameBuffer frame(source, PROT_READ);\n> > >>>> \tif (!frame.isValid()) {\n> > >>>> @@ -204,6 +205,12 @@ int EncoderLibJpeg::encode(const FrameBuffer *source,\n> > >>>>\n> > >>>> \tjpeg_start_compress(&compress_, TRUE);\n> > >>>>\n> > >>>> +\tif (exifData.size())\n> > >>>> +\t\t/* Store Exif data in the JPEG_APP1 data block. */\n> > >>>> +\t\tjpeg_write_marker(&compress_, JPEG_APP0 + 1,\n> > >>>> +\t\t\t\t static_cast(exifData.data()),\n> > >>>> +\t\t\t\t exifData.size());\n> > >>>> +\n> > >>>> \tLOG(JPEG, Debug) << \"JPEG Encode Starting:\" << compress_.image_width\n> > >>>> \t\t\t << \"x\" << compress_.image_height;\n> > >>>>\n> > >>>> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h\n> > >>>> index aed081a..1e8df05 100644\n> > >>>> --- a/src/android/jpeg/encoder_libjpeg.h\n> > >>>> +++ b/src/android/jpeg/encoder_libjpeg.h\n> > >>>> @@ -22,7 +22,8 @@ public:\n> > >>>>\n> > >>>> \tint configure(const libcamera::StreamConfiguration &cfg) override;\n> > >>>> \tint encode(const libcamera::FrameBuffer *source,\n> > >>>> -\t\t const libcamera::Span &destination) override;\n> > >>>> +\t\t const libcamera::Span &destination,\n> > >>>> +\t\t const libcamera::Span &exifData) override;\n> > >>>>\n> > >>>> private:\n> > >>>> \tvoid compressRGB(const libcamera::MappedBuffer *frame);\n> > >>>> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp\n> > >>>> index 41ddf48..36922ef 100644\n> > >>>> --- a/src/android/jpeg/exif.cpp\n> > >>>> +++ b/src/android/jpeg/exif.cpp\n> > >>>> @@ -168,6 +168,56 @@ void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::str\n> > >>>> \texif_entry_unref(entry);\n> > >>>> }\n> > >>>>\n> > >>>> +void Exif::setMake(const std::string &make)\n> > >>>> +{\n> > >>>> +\tsetString(EXIF_IFD_0, EXIF_TAG_MAKE, EXIF_FORMAT_ASCII, make);\n> > >>>> +}\n> > >>>> +\n> > >>>> +void Exif::setModel(const std::string &model)\n> > >>>> +{\n> > >>>> +\tsetString(EXIF_IFD_0, EXIF_TAG_MODEL, EXIF_FORMAT_ASCII, model);\n> > >>>> +}\n> > >>>> +\n> > >>>> +void Exif::setSize(const Size &size)\n> > >>>> +{\n> > >>>> +\tsetLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_Y_DIMENSION, size.height);\n> > >>>> +\tsetLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_X_DIMENSION, size.width);\n> > >>>> +}\n> > >>>> +\n> > >>>> +void Exif::setTimestamp(time_t timestamp)\n> > >>>> +{\n> > >>>> +\tchar str[20];\n> > >>>> +\tstd::strftime(str, sizeof(str), \"%Y:%m:%d %H:%M:%S\",\n> > >>>> +\t\t std::localtime(×tamp));\n> > >>>> +\tstd::string ts(str);\n> > >>>> +\n> > >>>> +\tsetString(EXIF_IFD_0, EXIF_TAG_DATE_TIME, EXIF_FORMAT_ASCII, ts);\n> > >>>> +\tsetString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_ORIGINAL, EXIF_FORMAT_ASCII, ts);\n> > >>>> +\tsetString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_DIGITIZED, EXIF_FORMAT_ASCII, ts);\n> > >>>> +}\n> > >>>> +\n> > >>>> +void Exif::setOrientation(int orientation)\n> > >>>> +{\n> > >>>> +\tint value;\n> > >>>> +\tswitch (orientation) {\n> > >>>> +\tcase 0:\n> > >>>> +\tdefault:\n> > >>>> +\t\tvalue = 1;\n> > >>>> +\t\tbreak;\n> > >>>> +\tcase 90:\n> > >>>> +\t\tvalue = 6;\n> > >>>> +\t\tbreak;\n> > >>>> +\tcase 180:\n> > >>>> +\t\tvalue = 3;\n> > >>>> +\t\tbreak;\n> > >>>> +\tcase 270:\n> > >>>> +\t\tvalue = 8;\n> > >>>> +\t\tbreak;\n> > >>>\n> > >>> Shouldn't the the 90 and 270 values be swapped ? 6 means \"The 0th row is\n> > >>> the visual right-hand side of the image, and the 0th column is the\n> > >>> visual top\", and 8 means \"The 0th row is the visual left-hand side of\n> > >>> the image, and the 0th column is the visual bottom\".\n> > >>>\n> > >>> Looking at the rotation property definition, the 90° rotation examples\n> > >>> shows\n> > >>>\n> > >>> +-------------------------------------+\n> > >>> | _ _ |\n> > >>> | \\ / |\n> > >>> | | | |\n> > >>> | | | |\n> > >>> | | > |\n> > >>> | < | |\n> > >>> | | | |\n> > >>> | . |\n> > >>> | V |\n> > >>> +-------------------------------------+\n> > >>>\n> > >>> where the 0th row is the left side and the 0th column the bottom side.\n> >\n> > Isn't the 0-th row (the first row of the image) on the 'visual\n> > right-hand side' ?\n>\n> Here's my interpretation:\n>\n> Looking at the fish in front of you (forgetting the camera), the visual\n> right-hand side is the head, and the visual left-hand side is the tail.\n> The 0th row of the image (now we're talking about the buffer, as\n> numbering rows implies we're counting pixels, so it's the buffer, not\n> the scene anymore). is the tail, thus the visual left-hand side of the\n> image.\n>\n\nI see.\n\nAs I've interpreted this, the description was about where the intended\nfirst row is located.\n\nThe 0-th row of the image (what would be the \"top\" if you look at the\nimage displayed on the screen and correctly rotated) is placed, in the\nabove imge, on the right side. That's how I read \"the 0th row is the\nvisual right-hand side\", but you interepretation might be correct too,\nas otherwise they could have phrased it as \"the 0th row is placed on\nthe right-hand side of the non-rotated image\"\n\n\n> > >>> I can fix those two small issues when applying, but I'd appreciate if\n> > >>> someone could double-check the rotation.\n> > >>\n> > >> Does the rotation property has to do anything with the fact that, camera\n> > >> sensor(webcam use-case) is intentionally mounted upside down to avoid\n> > >> rotation correction?\n> > >\n> > > The rotation reported by libcamera exposes the optical rotation of the\n> > > camera module, combining both the camera sensor and the lens. It is\n> > > indeed common to mount the camera sensor upside-down to compensate for\n> > > the lens inverting the image. In that case the reported rotation is 0°,\n> > > as the camera sensor rotation and the lens inversion compensate each\n> > > other.\n> >\n> > The 'rotation' property definition takes into account the lens\n> > inversion, so you shouldn't have to care about it\n>\n> I'm not sure if this means you agree or disagree with my explanation :-)\n>\n> > >> A widely used example on the internet is a big 'F'\n> > >> image denoting the value according to it's orientation [1], which is the\n> > >> same as per the values used in the patch. On the other hand, I can also\n> > >> find the code which satisfies Laurent's review and the EXIF\n> > >> 'orientation' specification [2]\n> > >>\n> > >> [1]:\n> > >> https://chromium.googlesource.com/chromiumos/platform2/+/HEAD/camera/common/exif_utils.cc#376\n> > >> [2]:\n> > >> https://piexif.readthedocs.io/en/latest/sample.html#rotate-image-by-exif-orientation\n> > >\n> > > The 0° and 180° cases are fairly straightforward, they map to the EXIF\n> > > values 1 and 3 respectively, there's usually no disagreement on that.\n> > >\n> > > The 90° and 270° cases are much more error prone as it's easy to get the\n> > > rotation direction wrong. The mapping between the 'F' orientation and\n> > > the EXIF values shown in [1] seems right to me.\n> > >\n> > > Android defines the rotation as the clockwise angle by which the image\n> > > needs to be rotated to appear in the correct orientation on the device\n> > > screen.\n> > >\n> > > For EXIF value 6,\n> > >\n> > > 88\n> > > 88 88\n> > > 8888888888\n> > >\n> > > the image needs to be rotated by 90° clockwise, while for EXIF value 8,\n> > >\n> > > 8888888888\n> > > 88 88\n> > > 88\n> > >\n> > > it needs to be rotated by 270°. The code in [2] thus appears correct.\n> > > What however appears to be incorrect is our claim that the libcamera and\n> > > Android numerical values are the same.\n> > >\n> > > Jacopo, any opinion on that ?\n> >\n> > Our definition differs from the Android one, as the rotation value we\n> > report is the rotation correction degrees in counter-clockwise direction.\n> > Android defines that in the clockwise direction.\n>\n> So there's a bug in our HAL implementation when reporting the rotation ?\n>\n\nI'm pretty sure we discussed that when we addressed that.\nHave we missed it that badly ? Seems weird but seems it's the case...\n\n> > If I got the above discussion right and looking at the 'F' examples in\n> > [1], we should report:\n> > 90 degrees = 8\n> > 270 degrees = 6\n> >\n> > However, this interpreation and the 'F' examples, don't match the\n> > textual description Laurent reported above:\n> >\n> > -------------------------------------------------------------------------------\n> > Shouldn't the the 90 and 270 values be swapped ? 6 means \"The 0th row is\n> > the visual right-hand side of the image, and the 0th column is the\n> > visual top\", and 8 means \"The 0th row is the visual left-hand side of\n> > the image, and the 0th column is the visual bottom\".\n> > ------------------------------------------------------------------------------\n> >\n> > For EXIF value 6,\n> >\n> > 88\n> > 88 88\n> > 8888888888\n> >\n> > the image needs to be rotated by 90° clockwise, while for EXIF value 8,\n> >\n> > 8888888888\n> > 88 88\n> > 88\n> > ------------------------------------------------------------------------------\n> >\n> > The two do not match, and I wonder what's the authoritative source for\n> > the definition of the semantic associated with the '6' and '8'\n> > values.\n>\n> The EXIF specification is the authoritative source, and it's defined as\n> I've quoted.\n>\n> > >> ¯\\_(ツ)_/¯\n> > >>\n> > >>>> +\t}\n> > >>>> +\n> > >>>> +\tsetShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value);\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 8dfc324..622de4c 100644\n> > >>>> --- a/src/android/jpeg/exif.h\n> > >>>> +++ b/src/android/jpeg/exif.h\n> > >>>> @@ -12,6 +12,7 @@\n> > >>>>\n> > >>>> #include \n> > >>>>\n> > >>>> +#include \n> > >>>> #include \n> > >>>>\n> > >>>> class Exif\n> > >>>> @@ -20,6 +21,13 @@ public:\n> > >>>> \tExif();\n> > >>>> \t~Exif();\n> > >>>>\n> > >>>> +\tvoid setMake(const std::string &make);\n> > >>>> +\tvoid setModel(const std::string &model);\n> > >>>> +\n> > >>>> +\tvoid setOrientation(int orientation);\n> > >>>> +\tvoid setSize(const libcamera::Size &size);\n> > >>>> +\tvoid setTimestamp(time_t timestamp);\n> > >>>> +\n> > >>>> \tlibcamera::Span data() const { return { exifData_, size_ }; }\n> > >>>> \t[[nodiscard]] int generate();\n> > >>>>\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 7A3DBBDB1C\n\tfor ;\n\tTue, 8 Sep 2020 13:10:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 15F9C62C4B;\n\tTue, 8 Sep 2020 15:10:40 +0200 (CEST)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A202D62C30\n\tfor ;\n\tTue, 8 Sep 2020 15:10:38 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 43323C0012;\n\tTue, 8 Sep 2020 13:10:37 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Tue, 8 Sep 2020 15:14:25 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20200908131425.6ou7b3zihbwqbxrq@uno.localdomain>","References":"<20200904064019.29869-1-email@uajain.com>\n\t<20200904153429.GE7518@pendragon.ideasonboard.com>\n\t\n\t<20200908120321.GC6047@pendragon.ideasonboard.com>\n\t<20200908124346.rhxugbmjaw3eao2o@uno.localdomain>\n\t<20200908124614.GI6047@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200908124614.GI6047@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v6 2/2] android: jpeg: Support an\n\tinitial set of EXIF metadata tags","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\" "}}]