[{"id":12277,"web_url":"https://patchwork.libcamera.org/comment/12277/","msgid":"","date":"2020-09-03T18:38:07","subject":"Re: [libcamera-devel] [PATCH v5 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 03/09/2020 17:32, 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> ---\n> src/android/camera_device.cpp | 18 ++++++++++-\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 | 48 +++++++++++++++++++++++++++-\n> src/android/jpeg/exif.h | 8 +++++\n> 6 files changed, 84 insertions(+), 5 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index de6f86f..54ca9c6 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,22 @@ 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\texif.generate();\n\nHrm ... Did this pass compile tests?\n\nDidn't we add a [[nodiscard]] to this call?\n\nAlthough - if it does fail to generate, I think we'd still expect to\ncall into the encoder->encode() and I think the exif.data() would be\nnullptr,0, (which is what we want), as the encoder checks the size to\ndetermine if it should be added or not.\n\n\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 9d8d9bc..5def7e3 100644\n> --- a/src/android/jpeg/exif.cpp\n> +++ b/src/android/jpeg/exif.cpp\n> @@ -169,7 +169,53 @@ void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::str\n> \texif_entry_unref(entry);\n> }\n> \n> -int Exif::generate()\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 = 1;\n> +\tswitch (orientation) {\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> +void Exif::generate()\n> {\n> \tif (exifData_) {\n> \t\tfree(exifData_);\n> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h\n> index 8fb8ffd..6113ca6 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> \tint [[nodiscard]] generate();\n> \n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 43357BF019\n\tfor ;\n\tThu, 3 Sep 2020 18:38:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C6C4D629C7;\n\tThu, 3 Sep 2020 20:38:11 +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 A222F62931\n\tfor ;\n\tThu, 3 Sep 2020 20:38:10 +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 2E76256E;\n\tThu, 3 Sep 2020 20:38:10 +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=\"ZUbCgGYS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1599158290;\n\tbh=uj7fEQk51RldxKvsonRnk2vgHqzveGdr+rEA/L5PZ2k=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=ZUbCgGYSUOVo/HesQLrp/0vUYpgkt2mhHkUvZYQNmxk2NmJ3jZTkgIjjfDzQKSy3s\n\tfjNv5l0c96pATjcb8XgmnW/d6w94DF9EZdcdEX9dBYSQf4CCiAvjm1IoIw1YwnZvVe\n\tNIk3ljocQuj2tBKC3/7p/EOGIuwjsqPWPt3+iF50=","To":"Umang Jain , libcamera-devel@lists.libcamera.org","References":"<20200903163216.6359-1-email@uajain.com>\n\t<20200903163216.6359-3-email@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":"Thu, 3 Sep 2020 19:38:07 +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":"<20200903163216.6359-3-email@uajain.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v5 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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12279,"web_url":"https://patchwork.libcamera.org/comment/12279/","msgid":"<20200903224037.GK6492@pendragon.ideasonboard.com>","date":"2020-09-03T22:40:37","subject":"Re: [libcamera-devel] [PATCH v5 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":"On Thu, Sep 03, 2020 at 07:38:07PM +0100, Kieran Bingham wrote:\n> Hi Umang,\n> \n> On 03/09/2020 17:32, 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> > ---\n> > src/android/camera_device.cpp | 18 ++++++++++-\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 | 48 +++++++++++++++++++++++++++-\n> > src/android/jpeg/exif.h | 8 +++++\n> > 6 files changed, 84 insertions(+), 5 deletions(-)\n> > \n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index de6f86f..54ca9c6 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,22 @@ 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\texif.generate();\n> \n> Hrm ... Did this pass compile tests?\n\nLikely not, given that generate() is defined as returning int in the\nheader file and void in the implementation. Rebase issue, or sending the\nwrong version ?\n\n> Didn't we add a [[nodiscard]] to this call?\n> \n> Although - if it does fail to generate, I think we'd still expect to\n> call into the encoder->encode() and I think the exif.data() would be\n> nullptr,0, (which is what we want), as the encoder checks the size to\n> determine if it should be added or not.\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 9d8d9bc..5def7e3 100644\n> > --- a/src/android/jpeg/exif.cpp\n> > +++ b/src/android/jpeg/exif.cpp\n> > @@ -169,7 +169,53 @@ void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::str\n> > \texif_entry_unref(entry);\n> > }\n> > \n> > -int Exif::generate()\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 = 1;\n> > +\tswitch (orientation) {\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> > +void Exif::generate()\n> > {\n> > \tif (exifData_) {\n> > \t\tfree(exifData_);\n> > diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h\n> > index 8fb8ffd..6113ca6 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> > \tint [[nodiscard]] 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 3F11EBF019\n\tfor ;\n\tThu, 3 Sep 2020 22:41:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CCA9262901;\n\tFri, 4 Sep 2020 00:41:03 +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 CF9C560374\n\tfor ;\n\tFri, 4 Sep 2020 00:41:01 +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 5DAF956E;\n\tFri, 4 Sep 2020 00:41:01 +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=\"R8q7Rtcd\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1599172861;\n\tbh=sk+AWtc4e6vzJjTfadD2zmy9UuTz1Tq62oYf4JCfDv8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=R8q7RtcdWfcog8yZOek4KrW4wImv3K5dEfL+HcJVq+GUOMjMwJy0kiOjX7mw1wJmU\n\tfoTlTWd5TpQmwjTG9a7C8J+2YM9iQ20UIYaStzp2ceuPbbFlD+SK8w5eGtg16DkwJf\n\t42iKRKrA+5DCO1KA2kaZwyyWpuYCx2LCLFClrlmE=","Date":"Fri, 4 Sep 2020 01:40:37 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"<20200903224037.GK6492@pendragon.ideasonboard.com>","References":"<20200903163216.6359-1-email@uajain.com>\n\t<20200903163216.6359-3-email@uajain.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v5 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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12282,"web_url":"https://patchwork.libcamera.org/comment/12282/","msgid":"<20200903232300.GM6492@pendragon.ideasonboard.com>","date":"2020-09-03T23:23:00","subject":"Re: [libcamera-devel] [PATCH v5 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 Thu, Sep 03, 2020 at 10:02:16PM +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> ---\n> src/android/camera_device.cpp | 18 ++++++++++-\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 | 48 +++++++++++++++++++++++++++-\n> src/android/jpeg/exif.h | 8 +++++\n> 6 files changed, 84 insertions(+), 5 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index de6f86f..54ca9c6 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,22 @@ 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\texif.generate();\n\nYou need to either check the error value here (but what would we do with\nit, maybe just printing an error ?), or drop [[nodiscard]].\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 9d8d9bc..5def7e3 100644\n> --- a/src/android/jpeg/exif.cpp\n> +++ b/src/android/jpeg/exif.cpp\n> @@ -169,7 +169,53 @@ void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::str\n> \texif_entry_unref(entry);\n> }\n> \n> -int Exif::generate()\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 = 1;\n> +\tswitch (orientation) {\n\n\tcase 0:\n\tdefault:\n\t\tvalue = 1;\n\t\tbreak;\n\nand you can remove the initialization of the variable.\n\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> +void Exif::generate()\n\nThis line seems to be a rebase issue.\n\nWith these small issues fixed (and the patch series tested ;-)),\n\nReviewed-by: Laurent Pinchart \n\n> {\n> \tif (exifData_) {\n> \t\tfree(exifData_);\n> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h\n> index 8fb8ffd..6113ca6 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> \tint [[nodiscard]] 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 B2FBFBF019\n\tfor ;\n\tThu, 3 Sep 2020 23:23:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2E51E629B6;\n\tFri, 4 Sep 2020 01:23:26 +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 3C5AD60374\n\tfor ;\n\tFri, 4 Sep 2020 01:23:24 +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 B714F304;\n\tFri, 4 Sep 2020 01:23:23 +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=\"Qx1fCnXv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1599175403;\n\tbh=s5LSmb2KMZdBceFahGENDLwufgt8ctWLYP5Fj3qzLP0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Qx1fCnXvlUmrrqvqnhHj4YVnggA4fdu7f+MXEZvKx0vYODBmmRQJdXp6cgweKUKyb\n\t2n29q2cTpvgQc0GcNU08jpYC/bcxr0DRbsL35G2Duo3wijzy7rvpAmj7Af/l57fZcA\n\tJx+j3dDXE/vwCjh37wGBzBWwy1mCl3tldEcyiuqo=","Date":"Fri, 4 Sep 2020 02:23:00 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"<20200903232300.GM6492@pendragon.ideasonboard.com>","References":"<20200903163216.6359-1-email@uajain.com>\n\t<20200903163216.6359-3-email@uajain.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200903163216.6359-3-email@uajain.com>","Subject":"Re: [libcamera-devel] [PATCH v5 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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12285,"web_url":"https://patchwork.libcamera.org/comment/12285/","msgid":"","date":"2020-09-04T06:05:08","subject":"Re: [libcamera-devel] [PATCH v5 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, Kieran,\n\nOn 9/4/20 4:10 AM, Laurent Pinchart wrote:\n> On Thu, Sep 03, 2020 at 07:38:07PM +0100, Kieran Bingham wrote:\n>> Hi Umang,\n>>\n>> On 03/09/2020 17:32, 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>>> ---\n>>> src/android/camera_device.cpp | 18 ++++++++++-\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 | 48 +++++++++++++++++++++++++++-\n>>> src/android/jpeg/exif.h | 8 +++++\n>>> 6 files changed, 84 insertions(+), 5 deletions(-)\n>>>\n>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>>> index de6f86f..54ca9c6 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,22 @@ 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\texif.generate();\n>> Hrm ... Did this pass compile tests?\n> Likely not, given that generate() is defined as returning int in the\n> header file and void in the implementation. Rebase issue, or sending the\n> wrong version ?\nIndeed, a faulty rebase on my end .. /o\\\n\nAlthough, it *did* pass the compile test and I can confirm\ncompiling the patches again (as is this series).\nIt's quite weird. GCC versions are :\n\nC compiler for the host machine: cc (gcc 9.3.0 \"cc (Ubuntu \n9.3.0-10ubuntu2) 9.3.0\")\nC linker for the host machine: cc ld.bfd 2.34\nC++ compiler for the host machine: c++ (gcc 9.3.0 \"c++ (Ubuntu \n9.3.0-10ubuntu2) 9.3.0\")\nC++ linker for the host machine: c++ ld.bfd 2.34\n\n\n>\n>> Didn't we add a [[nodiscard]] to this call?\n>>\n>> Although - if it does fail to generate, I think we'd still expect to\n>> call into the encoder->encode() and I think the exif.data() would be\n>> nullptr,0, (which is what we want), as the encoder checks the size to\n>> determine if it should be added or not.\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 9d8d9bc..5def7e3 100644\n>>> --- a/src/android/jpeg/exif.cpp\n>>> +++ b/src/android/jpeg/exif.cpp\n>>> @@ -169,7 +169,53 @@ void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::str\n>>> \texif_entry_unref(entry);\n>>> }\n>>> \n>>> -int Exif::generate()\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 = 1;\n>>> +\tswitch (orientation) {\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>>> +void Exif::generate()\n>>> {\n>>> \tif (exifData_) {\n>>> \t\tfree(exifData_);\n>>> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h\n>>> index 8fb8ffd..6113ca6 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>>> \tint [[nodiscard]] 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 37B2BBF019\n\tfor ;\n\tFri, 4 Sep 2020 06:05:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AABAF629B6;\n\tFri, 4 Sep 2020 08:05:15 +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 862CC6036F\n\tfor ;\n\tFri, 4 Sep 2020 08:05:13 +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=\"RDIm952j\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=uajain.com; s=mail;\n\tt=1599199512; bh=nIGzutpemN+BIChjLk8D+67m5KiFOqnvRJdJBzCzjOQ=;\n\th=Subject:To:Cc:References:From:In-Reply-To;\n\tb=RDIm952jcyObgFdGKOMqQ4VfGhekKnCa3MdCTe/uEjMM9bZRtzIShMbDUNMxtP9yG\n\tfAC00CH2DSJvDDtQYFk42Vbfr9IP0YjlZzUppWStDxXpXK+YHvjYY0P9WhakBGGrG4\n\t2ku1K7A8RhnnimA4Uec/zqdAqnn9AMCFnTFSxRkblUU5seD+HLxM+u0yhUlaybI9fH\n\tXWoVx4QZ1N25r1fM8wzCIwBDb6GKOAqxKBc/3xby1qbBDC9QZip8BYq5VN1PARyXYt\n\t6WoHPoCBiaPTclj5+RWhS+dplrEs0J0sGou2IEorZPQmKbaXs4UWfT2r5sqsEFN5rU\n\tkfWPfQ/pDgeGg==","To":"Laurent Pinchart ,\n\tKieran Bingham ","References":"<20200903163216.6359-1-email@uajain.com>\n\t<20200903163216.6359-3-email@uajain.com>\n\t\n\t<20200903224037.GK6492@pendragon.ideasonboard.com>","From":"Umang Jain ","Message-ID":"","Date":"Fri, 4 Sep 2020 11:35:08 +0530","Mime-Version":"1.0","In-Reply-To":"<20200903224037.GK6492@pendragon.ideasonboard.com>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v5 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-Transfer-Encoding":"7bit","Content-Type":"text/plain; charset=\"us-ascii\"; Format=\"flowed\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12287,"web_url":"https://patchwork.libcamera.org/comment/12287/","msgid":"","date":"2020-09-04T06:09:59","subject":"Re: [libcamera-devel] [PATCH v5 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 4:53 AM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Thu, Sep 03, 2020 at 10:02:16PM +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>> ---\n>> src/android/camera_device.cpp | 18 ++++++++++-\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 | 48 +++++++++++++++++++++++++++-\n>> src/android/jpeg/exif.h | 8 +++++\n>> 6 files changed, 84 insertions(+), 5 deletions(-)\n>>\n>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>> index de6f86f..54ca9c6 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,22 @@ 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\texif.generate();\n> You need to either check the error value here (but what would we do with\n> it, maybe just printing an error ?), or drop [[nodiscard]].\nYes, it's a bit of conundrum. We surely want to keep [[nodiscard]], so I \nwould just check the value and log an error here as well. (i.e. we will \nget 2 errors if EXIF turns out to be invalid, one from EXIF Log domain \nand other one from here, HAL).\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 9d8d9bc..5def7e3 100644\n>> --- a/src/android/jpeg/exif.cpp\n>> +++ b/src/android/jpeg/exif.cpp\n>> @@ -169,7 +169,53 @@ void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::str\n>> \texif_entry_unref(entry);\n>> }\n>> \n>> -int Exif::generate()\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 = 1;\n>> +\tswitch (orientation) {\n> \tcase 0:\n> \tdefault:\n> \t\tvalue = 1;\n> \t\tbreak;\n>\n> and you can remove the initialization of the variable.\n>\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>> +void Exif::generate()\n> This line seems to be a rebase issue.\n>\n> With these small issues fixed (and the patch series tested ;-)),\nNever I have ever sent patches without atleast compiling patches. (How \nstupid would that be!) These unfortunately turned out to be a bad \nrebase, but it did pass the compile test for sure!\n>\n> Reviewed-by: Laurent Pinchart \n>\n>> {\n>> \tif (exifData_) {\n>> \t\tfree(exifData_);\n>> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h\n>> index 8fb8ffd..6113ca6 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>> \tint [[nodiscard]] 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 67D2DBE174\n\tfor ;\n\tFri, 4 Sep 2020 06:10:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E862862901;\n\tFri, 4 Sep 2020 08:10:03 +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 7C1CA6036F\n\tfor ;\n\tFri, 4 Sep 2020 08:10:02 +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=\"NzKb+4EV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=uajain.com; s=mail;\n\tt=1599199801; bh=dxR3hOq/DM7E76heGPpsdHx5UZ+NZhiGeV+D3c4os+g=;\n\th=Subject:To:Cc:References:From:In-Reply-To;\n\tb=NzKb+4EVLrv7OXGGVw3cHPc/V2L5sMQUe79yM+2eiDOydUS8Jn7Eg+I4UjrTtJfDl\n\tsq/qoVYCNfHBowG8OdUEOlbt7TSHPMoXTqTvjHDzvit8+k/r0kLMBVKzh0ABLYFtNa\n\tgYpvrsXgVsZTHXx/CVt0xvFk3pmwqHID//SwzYNaGSnnWRHgtjwLKA/jplYgHNHKye\n\tSebxQHMFweiQNLLfFdUHSs2FkiDIGlMay1jj70YSQSgODjGFg8xfsxuua7DAN2akE7\n\tS/jP1ZG8xy0aJG+z+2sctRrFC1B84v5M3GJchKVoYcAVbc2ooTGTeU56IZJ9uKNTVT\n\tN+HO0HcmsYqwg==","To":"Laurent Pinchart ","References":"<20200903163216.6359-1-email@uajain.com>\n\t<20200903163216.6359-3-email@uajain.com>\n\t<20200903232300.GM6492@pendragon.ideasonboard.com>","From":"Umang Jain ","Message-ID":"","Date":"Fri, 4 Sep 2020 11:39:59 +0530","Mime-Version":"1.0","In-Reply-To":"<20200903232300.GM6492@pendragon.ideasonboard.com>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v5 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-Transfer-Encoding":"7bit","Content-Type":"text/plain; charset=\"us-ascii\"; Format=\"flowed\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12296,"web_url":"https://patchwork.libcamera.org/comment/12296/","msgid":"<20200904124428.GB6107@pendragon.ideasonboard.com>","date":"2020-09-04T12:44:28","subject":"Re: [libcamera-devel] [PATCH v5 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 Fri, Sep 04, 2020 at 11:35:08AM +0530, Umang Jain wrote:\n> On 9/4/20 4:10 AM, Laurent Pinchart wrote:\n> > On Thu, Sep 03, 2020 at 07:38:07PM +0100, Kieran Bingham wrote:\n> >> On 03/09/2020 17:32, 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> >>> ---\n> >>> src/android/camera_device.cpp | 18 ++++++++++-\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 | 48 +++++++++++++++++++++++++++-\n> >>> src/android/jpeg/exif.h | 8 +++++\n> >>> 6 files changed, 84 insertions(+), 5 deletions(-)\n> >>>\n> >>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> >>> index de6f86f..54ca9c6 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,22 @@ 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\texif.generate();\n> >>\n> >> Hrm ... Did this pass compile tests?\n> >\n> > Likely not, given that generate() is defined as returning int in the\n> > header file and void in the implementation. Rebase issue, or sending the\n> > wrong version ?\n>\n> Indeed, a faulty rebase on my end .. /o\\\n> \n> Although, it *did* pass the compile test and I can confirm\n> compiling the patches again (as is this series).\n> It's quite weird. GCC versions are :\n> \n> C compiler for the host machine: cc (gcc 9.3.0 \"cc (Ubuntu \n> 9.3.0-10ubuntu2) 9.3.0\")\n> C linker for the host machine: cc ld.bfd 2.34\n> C++ compiler for the host machine: c++ (gcc 9.3.0 \"c++ (Ubuntu \n> 9.3.0-10ubuntu2) 9.3.0\")\n> C++ linker for the host machine: c++ ld.bfd 2.34\n\nCould it be that the android option is set to false in your build ?\nHere's what I get with gcc 9.3.0.\n\n[24/172] Compiling C++ object 'src/libcamera/4ab8042@@camera@sha/.._android_jpeg_exif.cpp.o'\nFAILED: src/libcamera/4ab8042@@camera@sha/.._android_jpeg_exif.cpp.o\ng++-9.3.0 -Isrc/libcamera/4ab8042@@camera@sha -Isrc/libcamera -I../../src/libcamera -Iinclude -I../../include -I../../include/android/hardware/libhardware/include -I../../include/android/metadata -I../../include/android/system/core/include -Iinclude/libcamera -I/usr/include/libexif -fdiagnostics-color=always -pipe -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Wextra -Werror -std=c++17 -g -include config.h -fPIC -pthread -MD -MQ 'src/libcamera/4ab8042@@camera@sha/.._android_jpeg_exif.cpp.o' -MF 'src/libcamera/4ab8042@@camera@sha/.._android_jpeg_exif.cpp.o.d' -o 'src/libcamera/4ab8042@@camera@sha/.._android_jpeg_exif.cpp.o' -c ../../src/android/jpeg/exif.cpp\nIn file included from ../../src/android/jpeg/exif.cpp:8:\n../../src/android/jpeg/exif.h:32:6: error: attribute ignored [-Werror=attributes]\n 32 | int [[nodiscard]] generate();\n | ^\n../../src/android/jpeg/exif.h:32:6: note: an attribute that appertains to a type-specifier is ignored\n../../src/android/jpeg/exif.cpp:218:6: error: no declaration matches ‘void Exif::generate()’\n 218 | void Exif::generate()\n | ^~~~\nIn file included from ../../src/android/jpeg/exif.cpp:8:\n../../src/android/jpeg/exif.h:32:21: note: candidate is: ‘int Exif::generate()’\n 32 | int [[nodiscard]] generate();\n | ^~~~~~~~\n../../src/android/jpeg/exif.h:18:7: note: ‘class Exif’ defined here\n 18 | class Exif\n | ^~~~\ncc1plus: all warnings being treated as errors\n\n> >> Didn't we add a [[nodiscard]] to this call?\n> >>\n> >> Although - if it does fail to generate, I think we'd still expect to\n> >> call into the encoder->encode() and I think the exif.data() would be\n> >> nullptr,0, (which is what we want), as the encoder checks the size to\n> >> determine if it should be added or not.\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 9d8d9bc..5def7e3 100644\n> >>> --- a/src/android/jpeg/exif.cpp\n> >>> +++ b/src/android/jpeg/exif.cpp\n> >>> @@ -169,7 +169,53 @@ void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::str\n> >>> \texif_entry_unref(entry);\n> >>> }\n> >>> \n> >>> -int Exif::generate()\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 = 1;\n> >>> +\tswitch (orientation) {\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> >>> +void Exif::generate()\n> >>> {\n> >>> \tif (exifData_) {\n> >>> \t\tfree(exifData_);\n> >>> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h\n> >>> index 8fb8ffd..6113ca6 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> >>> \tint [[nodiscard]] 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 E3DE1BE174\n\tfor ;\n\tFri, 4 Sep 2020 12:44:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 746D060599;\n\tFri, 4 Sep 2020 14:44:54 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8B7906037B\n\tfor ;\n\tFri, 4 Sep 2020 14:44:53 +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 4A435540;\n\tFri, 4 Sep 2020 14:44:51 +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=\"r+jKqQwe\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1599223491;\n\tbh=60cenM/Hsvliwgy3t78kvCEqsmDG38Cif47TgX97h64=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=r+jKqQwekgw1//Ys5wRxLUur/qBth41FVIoTCsKnJqEaBw5IUVysVVAWMaCBMtoWy\n\t2RZiQcht6q8zUM681TfEf5YLmHSXhyyt+Whz/om7uFYIOEOfRhCbhHjSNg6/uMUo8F\n\tma9PqPFAqvWow/d4FHJqNp4dFf9g3CryJHHkAu1g=","Date":"Fri, 4 Sep 2020 15:44:28 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"<20200904124428.GB6107@pendragon.ideasonboard.com>","References":"<20200903163216.6359-1-email@uajain.com>\n\t<20200903163216.6359-3-email@uajain.com>\n\t\n\t<20200903224037.GK6492@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v5 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":12365,"web_url":"https://patchwork.libcamera.org/comment/12365/","msgid":"<64903260-066c-0fd3-4c0c-0309103a2b84@uajain.com>","date":"2020-09-08T05:45:09","subject":"Re: [libcamera-devel] [PATCH v5 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 6:14 PM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> On Fri, Sep 04, 2020 at 11:35:08AM +0530, Umang Jain wrote:\n>> On 9/4/20 4:10 AM, Laurent Pinchart wrote:\n>>> On Thu, Sep 03, 2020 at 07:38:07PM +0100, Kieran Bingham wrote:\n>>>> On 03/09/2020 17:32, 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>>>>> ---\n>>>>> src/android/camera_device.cpp | 18 ++++++++++-\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 | 48 +++++++++++++++++++++++++++-\n>>>>> src/android/jpeg/exif.h | 8 +++++\n>>>>> 6 files changed, 84 insertions(+), 5 deletions(-)\n>>>>>\n>>>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>>>>> index de6f86f..54ca9c6 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,22 @@ 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\texif.generate();\n>>>> Hrm ... Did this pass compile tests?\n>>> Likely not, given that generate() is defined as returning int in the\n>>> header file and void in the implementation. Rebase issue, or sending the\n>>> wrong version ?\n>> Indeed, a faulty rebase on my end .. /o\\\n>>\n>> Although, it *did* pass the compile test and I can confirm\n>> compiling the patches again (as is this series).\n>> It's quite weird. GCC versions are :\n>>\n>> C compiler for the host machine: cc (gcc 9.3.0 \"cc (Ubuntu\n>> 9.3.0-10ubuntu2) 9.3.0\")\n>> C linker for the host machine: cc ld.bfd 2.34\n>> C++ compiler for the host machine: c++ (gcc 9.3.0 \"c++ (Ubuntu\n>> 9.3.0-10ubuntu2) 9.3.0\")\n>> C++ linker for the host machine: c++ ld.bfd 2.34\n> Could it be that the android option is set to false in your build ?\n> Here's what I get with gcc 9.3.0.\n>\n> [24/172] Compiling C++ object 'src/libcamera/4ab8042@@camera@sha/.._android_jpeg_exif.cpp.o'\n> FAILED: src/libcamera/4ab8042@@camera@sha/.._android_jpeg_exif.cpp.o\n> g++-9.3.0 -Isrc/libcamera/4ab8042@@camera@sha -Isrc/libcamera -I../../src/libcamera -Iinclude -I../../include -I../../include/android/hardware/libhardware/include -I../../include/android/metadata -I../../include/android/system/core/include -Iinclude/libcamera -I/usr/include/libexif -fdiagnostics-color=always -pipe -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Wextra -Werror -std=c++17 -g -include config.h -fPIC -pthread -MD -MQ 'src/libcamera/4ab8042@@camera@sha/.._android_jpeg_exif.cpp.o' -MF 'src/libcamera/4ab8042@@camera@sha/.._android_jpeg_exif.cpp.o.d' -o 'src/libcamera/4ab8042@@camera@sha/.._android_jpeg_exif.cpp.o' -c ../../src/android/jpeg/exif.cpp\n> In file included from ../../src/android/jpeg/exif.cpp:8:\n> ../../src/android/jpeg/exif.h:32:6: error: attribute ignored [-Werror=attributes]\n> 32 | int [[nodiscard]] generate();\n> | ^\n> ../../src/android/jpeg/exif.h:32:6: note: an attribute that appertains to a type-specifier is ignored\n> ../../src/android/jpeg/exif.cpp:218:6: error: no declaration matches ‘void Exif::generate()’\n> 218 | void Exif::generate()\n> | ^~~~\n> In file included from ../../src/android/jpeg/exif.cpp:8:\n> ../../src/android/jpeg/exif.h:32:21: note: candidate is: ‘int Exif::generate()’\n> 32 | int [[nodiscard]] generate();\n> | ^~~~~~~~\n> ../../src/android/jpeg/exif.h:18:7: note: ‘class Exif’ defined here\n> 18 | class Exif\n> | ^~~~\n> cc1plus: all warnings being treated as errors\nOh damn! In my ./build-libcamera.sh script, The -Dandroid=true had an \nspell error- -Danroid=true. The build did warn in the logs(now that I \ncan read/see) but it didn't \"abort\" the build which made me think that \nit passed, heh! I should pay more attention to meson build logs. :-( \nApologies for the noise.\n>>>> Didn't we add a [[nodiscard]] to this call?\n>>>>\n>>>> Although - if it does fail to generate, I think we'd still expect to\n>>>> call into the encoder->encode() and I think the exif.data() would be\n>>>> nullptr,0, (which is what we want), as the encoder checks the size to\n>>>> determine if it should be added or not.\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 9d8d9bc..5def7e3 100644\n>>>>> --- a/src/android/jpeg/exif.cpp\n>>>>> +++ b/src/android/jpeg/exif.cpp\n>>>>> @@ -169,7 +169,53 @@ void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::str\n>>>>> \texif_entry_unref(entry);\n>>>>> }\n>>>>> \n>>>>> -int Exif::generate()\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 = 1;\n>>>>> +\tswitch (orientation) {\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>>>>> +void Exif::generate()\n>>>>> {\n>>>>> \tif (exifData_) {\n>>>>> \t\tfree(exifData_);\n>>>>> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h\n>>>>> index 8fb8ffd..6113ca6 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>>>>> \tint [[nodiscard]] 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 EEEBDBDB1D\n\tfor ;\n\tTue, 8 Sep 2020 05:45:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 58ACC62BAA;\n\tTue, 8 Sep 2020 07:45:15 +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 1A50E603EE\n\tfor ;\n\tTue, 8 Sep 2020 07:45:14 +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=\"mxNT7A/q\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=uajain.com; s=mail;\n\tt=1599543913; bh=x96TXz5zygm8gnvNpzulCzVg6yxktxSW3VoPH2Zx/w8=;\n\th=Subject:To:Cc:References:From:In-Reply-To;\n\tb=mxNT7A/qH3QauHnfUzIjLFMtF6Uvvbsr10vCom8jxV8xzuGJqunIHAH2AMqc2ufXd\n\tTA9u8E4LMYv+TPtwV5cEax/u1a3rjFCKYdVFPg4Vbsnxvoj3SHBIZMKe4C8yWiPFuA\n\td9TmLBq2batZEX8ZdQ/7OX5ZTfPBhXKDv20b0Toiz7atPUo9KJGAMcMdGJc6muLBl3\n\t2Y+0U/x2EC7vzo+vog9aju8cRx0JBO+3DCUX0Tt4xxE284Fo4lEAYdT16lI6qxoNg6\n\tSS1jwFmFCjEFOAO3YNuKaTpjXgq6XU52qnu+TdXCqDU+PY0IltOdbigaWXAx20nKU1\n\t43jOx1uiBETUg==","To":"Laurent Pinchart ","References":"<20200903163216.6359-1-email@uajain.com>\n\t<20200903163216.6359-3-email@uajain.com>\n\t\n\t<20200903224037.GK6492@pendragon.ideasonboard.com>\n\t\n\t<20200904124428.GB6107@pendragon.ideasonboard.com>","From":"Umang Jain ","Message-ID":"<64903260-066c-0fd3-4c0c-0309103a2b84@uajain.com>","Date":"Tue, 8 Sep 2020 11:15:09 +0530","Mime-Version":"1.0","In-Reply-To":"<20200904124428.GB6107@pendragon.ideasonboard.com>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v5 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-Transfer-Encoding":"base64","Content-Type":"text/plain; charset=\"utf-8\"; Format=\"flowed\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]