[{"id":12221,"web_url":"https://patchwork.libcamera.org/comment/12221/","msgid":"<20200828173740.GP10034@pendragon.ideasonboard.com>","date":"2020-08-28T17:37:40","subject":"Re: [libcamera-devel] [PATCH v4 1/2] libcamera: android: Add EXIF\n\tinfrastructure","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, Aug 28, 2020 at 06:57:35AM +0000, Umang Jain wrote:\n> From: Kieran Bingham \n> \n> Provide helper classes to utilise the libexif interfaces and link\n> against libexif to support tag additions when creating JPEG images.\n> \n> Signed-off-by: Kieran Bingham \n> Signed-off-by: Umang Jain \n> Reviewed-by: Kieran Bingham \n> ---\n> src/android/jpeg/exif.cpp | 172 ++++++++++++++++++++++++++++++++++++++\n> src/android/jpeg/exif.h | 44 ++++++++++\n> src/android/meson.build | 2 +\n> 3 files changed, 218 insertions(+)\n> create mode 100644 src/android/jpeg/exif.cpp\n> create mode 100644 src/android/jpeg/exif.h\n> \n> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp\n> new file mode 100644\n> index 0000000..b49b538\n> --- /dev/null\n> +++ b/src/android/jpeg/exif.cpp\n> @@ -0,0 +1,172 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * exif.cpp - EXIF tag creation using libexif\n> + */\n> +\n> +#include \"exif.h\"\n> +\n> +#include \"libcamera/internal/log.h\"\n> +\n> +using namespace libcamera;\n> +\n> +LOG_DEFINE_CATEGORY(EXIF)\n> +\n> +Exif::Exif()\n> +\t: valid_(false), exifData_(0), size_(0)\n\nYou should initialize exifData_ and data_ here, otherwise the destructor\nwill access uninitialized pointers if the constructor fails.\n\n> +{\n> +\t/* Create an ExifMem allocator to construct entries. */\n> +\tmem_ = exif_mem_new_default();\n> +\tif (!mem_) {\n> +\t\tLOG(EXIF, Fatal) << \"Failed to allocate ExifMem Allocator\";\n\nFatal is a bit harsh, it will abort(). I'd go for Error.\n\n> +\t\treturn;\n> +\t}\n> +\n> +\tdata_ = exif_data_new_mem(mem_);\n> +\tif (!data_) {\n> +\t\tLOG(EXIF, Fatal) << \"Failed to allocate an ExifData structure\";\n\nSame here, and in a few locations below.\n\n> +\t\treturn;\n> +\t}\n> +\n> +\tvalid_ = true;\n> +\n> +\texif_data_set_option(data_, EXIF_DATA_OPTION_FOLLOW_SPECIFICATION);\n> +\texif_data_set_data_type(data_, EXIF_DATA_TYPE_COMPRESSED);\n> +\n> +\t/*\n> +\t * Big-Endian: EXIF_BYTE_ORDER_MOTOROLA\n> +\t * Little Endian: EXIF_BYTE_ORDER_INTEL\n> +\t */\n> +\texif_data_set_byte_order(data_, EXIF_BYTE_ORDER_INTEL);\n> +\n> +\t/* Create the mandatory EXIF fields with default data. */\n> +\texif_data_fix(data_);\n> +}\n> +\n> +Exif::~Exif()\n> +{\n> +\tif (exifData_)\n> +\t\tfree(exifData_);\n> +\n> +\tif (data_)\n> +\t\texif_data_unref(data_);\n> +\n> +\tif (mem_)\n> +\t\texif_mem_unref(mem_);\n> +}\n> +\n> +ExifEntry *Exif::createEntry(ExifIfd ifd, ExifTag tag)\n> +{\n> +\tExifContent *content = data_->ifd[ifd];\n> +\tExifEntry *entry = exif_content_get_entry(content, tag);\n> +\n> +\tif (entry) {\n> +\t\texif_entry_ref(entry);\n> +\t\treturn entry;\n> +\t}\n\nI don't think we'll hit this code path anymore as we don't reuse the\nEXIF generator, will we ?\n\n> +\n> +\tentry = exif_entry_new_mem(mem_);\n> +\tif (!entry) {\n> +\t\tLOG(EXIF, Fatal) << \"Failed to allocated new entry\";\n> +\t\treturn nullptr;\n\nWhen this happens, I think we should reset the valid_ flag to false, so\nthat the Exif class user can add entries without checking for errors,\nand check once at the end.\n\n> +\t}\n> +\n> +\tentry->tag = tag;\n> +\n> +\texif_content_add_entry(content, entry);\n> +\texif_entry_initialize(entry, tag);\n> +\n> +\treturn entry;\n> +}\n> +\n> +ExifEntry *Exif::createEntry(ExifIfd ifd, ExifTag tag, ExifFormat format,\n> +\t\t\t uint64_t components, unsigned int size)\n\nuint64_t is a lot. I think unsigned int would be enough.\n\n> +{\n> +\tExifContent *content = data_->ifd[ifd];\n> +\n> +\t/* Replace any existing entry with the same tag. */\n> +\tExifEntry *existing = exif_content_get_entry(content, tag);\n> +\texif_content_remove_entry(content, existing);\n\nSame here. Or should we keep them just in case ? That would be a bug in\nthe caller I believe.\n\n> +\n> +\tExifEntry *entry = exif_entry_new_mem(mem_);\n> +\tif (!entry) {\n> +\t\tLOG(EXIF, Fatal) << \"Failed to allocated new entry\";\n> +\t\treturn nullptr;\n> +\t}\n> +\n> +\tvoid *buffer = exif_mem_alloc(mem_, size);\n> +\tif (!buffer) {\n> +\t\tLOG(EXIF, Fatal) << \"Failed to allocate buffer for variable entry\";\n> +\t\texif_mem_unref(mem_);\n> +\t\treturn nullptr;\n> +\t}\n> +\n> +\tentry->data = static_cast(buffer);\n> +\tentry->components = components;\n> +\tentry->format = format;\n> +\tentry->size = size;\n> +\tentry->tag = tag;\n> +\n> +\texif_content_add_entry(content, entry);\n> +\n> +\treturn entry;\n> +}\n> +\n> +int Exif::setShort(ExifIfd ifd, ExifTag tag, uint16_t item)\n> +{\n> +\tExifEntry *entry = createEntry(ifd, tag);\n\n\tif (!entry)\n\t\treturn -1;\n\nI would actually make this function void, if we reset the valid_ flag on\nfailure. Same below.\n\n> +\n> +\texif_set_short(entry->data, EXIF_BYTE_ORDER_INTEL, item);\n> +\texif_entry_unref(entry);\n> +\n> +\treturn 0;\n> +}\n> +\n> +int Exif::setLong(ExifIfd ifd, ExifTag tag, uint32_t item)\n> +{\n> +\tExifEntry *entry = createEntry(ifd, tag);\n> +\n> +\texif_set_long(entry->data, EXIF_BYTE_ORDER_INTEL, item);\n> +\texif_entry_unref(entry);\n> +\n> +\treturn 0;\n> +}\n> +\n> +int Exif::setRational(ExifIfd ifd, ExifTag tag, uint32_t numerator, uint32_t denominator)\n\nMaybe wrapping at 80 columns where possible ?\n\nAs this function is privaet, you could replace numerator and denominator\nwith an ExifRational if you want.\n\n> +{\n> +\tExifEntry *entry = createEntry(ifd, tag);\n> +\tExifRational item{ numerator, denominator };\n> +\n> +\texif_set_rational(entry->data, EXIF_BYTE_ORDER_INTEL, item);\n> +\texif_entry_unref(entry);\n> +\n> +\treturn 0;\n> +}\n> +\n> +int Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::string &item)\n> +{\n> +\t/* Pad 1 extra byte for null-terminated string. */\n> +\tsize_t length = item.length() + 1;\n> +\n> +\tExifEntry *entry = createEntry(ifd, tag, format, length, length);\n> +\n> +\tmemcpy(entry->data, item.c_str(), length);\n> +\texif_entry_unref(entry);\n> +\n> +\treturn 0;\n> +}\n> +\n> +int Exif::generate()\n> +{\n> +\tif (exifData_) {\n> +\t\tfree(exifData_);\n> +\t\texifData_ = nullptr;\n> +\t}\n\nThis should also not happen anymore.\n\n> +\n> +\texif_data_save_data(data_, &exifData_, &size_);\n> +\n> +\tLOG(EXIF, Debug) << \"Created EXIF instance (\" << size_ << \" bytes)\";\n> +\n> +\treturn 0;\n> +}\n> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h\n> new file mode 100644\n> index 0000000..6c10f94\n> --- /dev/null\n> +++ b/src/android/jpeg/exif.h\n> @@ -0,0 +1,44 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * exif.h - EXIF tag creator using libexif\n> + */\n> +#ifndef __ANDROID_JPEG_EXIF_H__\n> +#define __ANDROID_JPEG_EXIF_H__\n> +\n> +#include \n\nWould be nice if we didn't have to expose this header, but the typedefs\nmake that difficult :-(\n\n> +\n> +#include \n> +\n> +#include \n> +\n> +class Exif\n> +{\n> +public:\n> +\tExif();\n> +\t~Exif();\n> +\n> +\tlibcamera::Span data() const { return { exifData_, size_ }; }\n> +\tint generate();\n> +\n> +private:\n> +\tExifEntry *createEntry(ExifIfd ifd, ExifTag tag);\n> +\tExifEntry *createEntry(ExifIfd ifd, ExifTag tag, ExifFormat format,\n> +\t\t\t uint64_t components, unsigned int size);\n> +\n> +\tint setShort(ExifIfd ifd, ExifTag tag, uint16_t item);\n> +\tint setLong(ExifIfd ifd, ExifTag tag, uint32_t item);\n> +\tint setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::string &item);\n> +\tint setRational(ExifIfd ifd, ExifTag tag, uint32_t numerator, uint32_t denominator);\n\nLine wrap here too ?\n\n> +\n> +\tbool valid_;\n\nvalid_ is never used... Should it be checked in generate(), which would\nreturn an error if valid_ is false ?\n\n> +\n> +\tExifData *data_;\n> +\tExifMem *mem_;\n> +\n> +\tunsigned char *exifData_;\n> +\tunsigned int size_;\n> +};\n> +\n> +#endif /* __ANDROID_JPEG_EXIF_H__ */\n> diff --git a/src/android/meson.build b/src/android/meson.build\n> index f7b81a4..ecb92f6 100644\n> --- a/src/android/meson.build\n> +++ b/src/android/meson.build\n> @@ -7,6 +7,7 @@ android_hal_sources = files([\n> 'camera_metadata.cpp',\n> 'camera_ops.cpp',\n> 'jpeg/encoder_libjpeg.cpp',\n> + 'jpeg/exif.cpp',\n> ])\n> \n> android_camera_metadata_sources = files([\n> @@ -14,6 +15,7 @@ android_camera_metadata_sources = files([\n> ])\n> \n> android_deps = [\n> + dependency('libexif'),\n> dependency('libjpeg'),\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 88DE6BDC71\n\tfor ;\n\tFri, 28 Aug 2020 17:38:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 200A36291E;\n\tFri, 28 Aug 2020 19:38: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 DB7BC60379\n\tfor ;\n\tFri, 28 Aug 2020 19:38: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 54502303;\n\tFri, 28 Aug 2020 19:38: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=\"DUlp6gZT\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1598636281;\n\tbh=CyVmJKBTT6XDoGjvod7pUIdJoG0exxRgmOApt89MsAA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DUlp6gZTtzQAceSSHsYxR68B5oH5y0bMqu+t/IoJ0qD26Obk8hjIl0MqatK6YMoeb\n\tIKKFXCTq5VcG61kzLiCDRUDr6G74B49WESW5AsERcDPgkFF4l65pcxkckmHbfRqoNh\n\t0rQqpb7EayP782bax4+pJlyQYwOCnZT1Fofy+b7w=","Date":"Fri, 28 Aug 2020 20:37:40 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"<20200828173740.GP10034@pendragon.ideasonboard.com>","References":"<20200828065727.9909-1-email@uajain.com>\n\t<20200828065727.9909-2-email@uajain.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200828065727.9909-2-email@uajain.com>","Subject":"Re: [libcamera-devel] [PATCH v4 1/2] libcamera: android: Add EXIF\n\tinfrastructure","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":12275,"web_url":"https://patchwork.libcamera.org/comment/12275/","msgid":"<434bdf52-e382-2ab5-2647-d897519ba56f@ideasonboard.com>","date":"2020-09-03T12:45:33","subject":"Re: [libcamera-devel] [PATCH v4 1/2] libcamera: android: Add EXIF\n\tinfrastructure","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 28/08/2020 18:37, Laurent Pinchart wrote:\n> Hi Umang,\n> \n> Thank you for the patch.\n> \n> On Fri, Aug 28, 2020 at 06:57:35AM +0000, Umang Jain wrote:\n>> From: Kieran Bingham \n>>\n>> Provide helper classes to utilise the libexif interfaces and link\n>> against libexif to support tag additions when creating JPEG images.\n>>\n>> Signed-off-by: Kieran Bingham \n>> Signed-off-by: Umang Jain \n>> Reviewed-by: Kieran Bingham \n>> ---\n>> src/android/jpeg/exif.cpp | 172 ++++++++++++++++++++++++++++++++++++++\n>> src/android/jpeg/exif.h | 44 ++++++++++\n>> src/android/meson.build | 2 +\n>> 3 files changed, 218 insertions(+)\n>> create mode 100644 src/android/jpeg/exif.cpp\n>> create mode 100644 src/android/jpeg/exif.h\n>>\n>> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp\n>> new file mode 100644\n>> index 0000000..b49b538\n>> --- /dev/null\n>> +++ b/src/android/jpeg/exif.cpp\n>> @@ -0,0 +1,172 @@\n>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>> +/*\n>> + * Copyright (C) 2020, Google Inc.\n>> + *\n>> + * exif.cpp - EXIF tag creation using libexif\n>> + */\n>> +\n>> +#include \"exif.h\"\n>> +\n>> +#include \"libcamera/internal/log.h\"\n>> +\n>> +using namespace libcamera;\n>> +\n>> +LOG_DEFINE_CATEGORY(EXIF)\n>> +\n>> +Exif::Exif()\n>> +\t: valid_(false), exifData_(0), size_(0)\n> \n> You should initialize exifData_ and data_ here, otherwise the destructor\n> will access uninitialized pointers if the constructor fails.\n\nWell, it's just data_ that seems to be missing. ;-) But indeed it's\nimportant to initialise, as there is a fail case return after mem_ is set.\n\n\n> \n>> +{\n>> +\t/* Create an ExifMem allocator to construct entries. */\n>> +\tmem_ = exif_mem_new_default();\n>> +\tif (!mem_) {\n>> +\t\tLOG(EXIF, Fatal) << \"Failed to allocate ExifMem Allocator\";\n> \n> Fatal is a bit harsh, it will abort(). I'd go for Error.\n\nI agree.\n\n> \n>> +\t\treturn;\n>> +\t}\n>> +\n>> +\tdata_ = exif_data_new_mem(mem_);\n>> +\tif (!data_) {\n>> +\t\tLOG(EXIF, Fatal) << \"Failed to allocate an ExifData structure\";\n> \n> Same here, and in a few locations below.\n> \n>> +\t\treturn;\n>> +\t}\n>> +\n>> +\tvalid_ = true;\n>> +\n>> +\texif_data_set_option(data_, EXIF_DATA_OPTION_FOLLOW_SPECIFICATION);\n>> +\texif_data_set_data_type(data_, EXIF_DATA_TYPE_COMPRESSED);\n>> +\n>> +\t/*\n>> +\t * Big-Endian: EXIF_BYTE_ORDER_MOTOROLA\n>> +\t * Little Endian: EXIF_BYTE_ORDER_INTEL\n>> +\t */\n>> +\texif_data_set_byte_order(data_, EXIF_BYTE_ORDER_INTEL);\n>> +\n>> +\t/* Create the mandatory EXIF fields with default data. */\n>> +\texif_data_fix(data_);\n\nNote this line for later. It creates some fields.\n\nIt 'could' be removed, but I think it's better to use it to keep to the\nstandards by default.\n\nIt might also be possible to put this in generate() if it doesn't modify\nexisting entries.\n\n>> +}\n>> +\n>> +Exif::~Exif()\n>> +{\n>> +\tif (exifData_)\n>> +\t\tfree(exifData_);\n>> +\n>> +\tif (data_)\n>> +\t\texif_data_unref(data_);\n>> +\n>> +\tif (mem_)\n>> +\t\texif_mem_unref(mem_);\n>> +}\n>> +\n>> +ExifEntry *Exif::createEntry(ExifIfd ifd, ExifTag tag)\n>> +{\n>> +\tExifContent *content = data_->ifd[ifd];\n>> +\tExifEntry *entry = exif_content_get_entry(content, tag);\n>> +\n>> +\tif (entry) {\n>> +\t\texif_entry_ref(entry);\n>> +\t\treturn entry;\n>> +\t}\n> \n> I don't think we'll hit this code path anymore as we don't reuse the\n> EXIF generator, will we ?\n\nI think it's better to keep this.\n\n1) exif_data_fix() used in constructor creates and populates default\nrequried entries which might (or might not) get updated.\n\n2) Even if we (can) move exif_data_fix() to generate(), a caller might\nset an entry twice. I see no reason not to handle that generically and\ngracefully.\n\n\n>> +\n>> +\tentry = exif_entry_new_mem(mem_);\n>> +\tif (!entry) {\n>> +\t\tLOG(EXIF, Fatal) << \"Failed to allocated new entry\";\n>> +\t\treturn nullptr;\n> \n> When this happens, I think we should reset the valid_ flag to false, so\n> that the Exif class user can add entries without checking for errors,\n> and check once at the end.\n\nAgreed, that will simplify things nicely.\n\n\n>> +\t}\n>> +\n>> +\tentry->tag = tag;\n>> +\n>> +\texif_content_add_entry(content, entry);\n>> +\texif_entry_initialize(entry, tag);\n>> +\n>> +\treturn entry;\n>> +}\n>> +\n>> +ExifEntry *Exif::createEntry(ExifIfd ifd, ExifTag tag, ExifFormat format,\n>> +\t\t\t uint64_t components, unsigned int size)\n> \n> uint64_t is a lot. I think unsigned int would be enough.\n\n\nhttp://libexif.sourceforge.net/internals/struct__ExifEntry.html\n\nThe 'components' field is an unsigned long.\n\nPerhaps we should use that directly.\n\n> \n>> +{\n>> +\tExifContent *content = data_->ifd[ifd];\n>> +\n>> +\t/* Replace any existing entry with the same tag. */\n>> +\tExifEntry *existing = exif_content_get_entry(content, tag);\n>> +\texif_content_remove_entry(content, existing);\n> \n> Same here. Or should we keep them just in case ? That would be a bug in\n> the caller I believe.\n\nNot always if defaults are replaced.\n\nAnd as mentioned, I think it's better to keep it.\n\n\n>> +\n>> +\tExifEntry *entry = exif_entry_new_mem(mem_);\n>> +\tif (!entry) {\n>> +\t\tLOG(EXIF, Fatal) << \"Failed to allocated new entry\";\n>> +\t\treturn nullptr;\n>> +\t}\n>> +\n>> +\tvoid *buffer = exif_mem_alloc(mem_, size);\n>> +\tif (!buffer) {\n>> +\t\tLOG(EXIF, Fatal) << \"Failed to allocate buffer for variable entry\";\n>> +\t\texif_mem_unref(mem_);\n>> +\t\treturn nullptr;\n>> +\t}\n>> +\n>> +\tentry->data = static_cast(buffer);\n>> +\tentry->components = components;\n>> +\tentry->format = format;\n>> +\tentry->size = size;\n>> +\tentry->tag = tag;\n>> +\n>> +\texif_content_add_entry(content, entry);\n>> +\n>> +\treturn entry;\n>> +}\n>> +\n>> +int Exif::setShort(ExifIfd ifd, ExifTag tag, uint16_t item)\n>> +{\n>> +\tExifEntry *entry = createEntry(ifd, tag);\n> \n> \tif (!entry)\n> \t\treturn -1;\n> \n> I would actually make this function void, if we reset the valid_ flag on\n> failure. Same below.\n\n\nAgreed.\n\n\n> \n>> +\n>> +\texif_set_short(entry->data, EXIF_BYTE_ORDER_INTEL, item);\n>> +\texif_entry_unref(entry);\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> +int Exif::setLong(ExifIfd ifd, ExifTag tag, uint32_t item)\n>> +{\n>> +\tExifEntry *entry = createEntry(ifd, tag);\n>> +\n\nThis should have an\n\tif (!entry)\n\t\treturn;\n\n(and also be a void function)\n\n>> +\texif_set_long(entry->data, EXIF_BYTE_ORDER_INTEL, item);\n>> +\texif_entry_unref(entry);\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> +int Exif::setRational(ExifIfd ifd, ExifTag tag, uint32_t numerator, uint32_t denominator)\n> \n> Maybe wrapping at 80 columns where possible ?\n> \n> As this function is privaet, you could replace numerator and denominator\n> with an ExifRational if you want.\n> \n>> +{\n>> +\tExifEntry *entry = createEntry(ifd, tag);\n>> +\tExifRational item{ numerator, denominator };\n>> +\n>> +\texif_set_rational(entry->data, EXIF_BYTE_ORDER_INTEL, item);\n>> +\texif_entry_unref(entry);\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> +int Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::string &item)\n>> +{\n>> +\t/* Pad 1 extra byte for null-terminated string. */\n>> +\tsize_t length = item.length() + 1;\n\nInterestingly, there is a mention in the spec that states (at least on\nthe VERSION string) that the null-termination *isn't* required.\n\nI don't know if that applies to all strings though, so this is likely\n'safer' all the same.\n\nI guess if the field length is encoded in the entry it wasn't deemed as\nrequired :-S\n\nTherefore, we might have to check to verify if the 'version' string can\nbe of length(5) ... Including the null-terminator could break things ...\nbut I think we'll keep this as is, and handle it if we discover the\nnull-terminator is bad.\n\nI believe CrOS adds the null-terminator, so that implies it's already\nbeen well tested across exif-readers, so we should be ok.\n\n\n>> +\n>> +\tExifEntry *entry = createEntry(ifd, tag, format, length, length);\n>> +\n>> +\tmemcpy(entry->data, item.c_str(), length);\n>> +\texif_entry_unref(entry);\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> +int Exif::generate()\n>> +{\n>> +\tif (exifData_) {\n>> +\t\tfree(exifData_);\n>> +\t\texifData_ = nullptr;\n>> +\t}\n> \n> This should also not happen anymore.\n\n'should not' but could if a caller called generate twice.\n\nI'd prefer to keep it, as it prevents leaks in that instance.\n\n\n> \n>> +\n>> +\texif_data_save_data(data_, &exifData_, &size_);\n>> +\n>> +\tLOG(EXIF, Debug) << \"Created EXIF instance (\" << size_ << \" bytes)\";\n>> +\n>> +\treturn 0;\n>> +}\n>> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h\n>> new file mode 100644\n>> index 0000000..6c10f94\n>> --- /dev/null\n>> +++ b/src/android/jpeg/exif.h\n>> @@ -0,0 +1,44 @@\n>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>> +/*\n>> + * Copyright (C) 2020, Google Inc.\n>> + *\n>> + * exif.h - EXIF tag creator using libexif\n>> + */\n>> +#ifndef __ANDROID_JPEG_EXIF_H__\n>> +#define __ANDROID_JPEG_EXIF_H__\n>> +\n>> +#include \n> \n> Would be nice if we didn't have to expose this header, but the typedefs\n> make that difficult :-(\n\nIndeed, it would be better to be self contained. Especially as it's not\nrequired for any of the object public API.\n\nWhy does C++ need to expose private stuff anyway :-) (that's rhetorical)\n\n\n>> +\n>> +#include \n>> +\n>> +#include \n>> +\n>> +class Exif\n>> +{\n>> +public:\n>> +\tExif();\n>> +\t~Exif();\n>> +\n>> +\tlibcamera::Span data() const { return { exifData_, size_ }; }\n>> +\tint generate();\n\nGiven the other comments in this review, I think we should also change\nthis to:\n\tint [[nodiscard]] generate();\n\nWhich we now have thanks to C++17.\n\n\n>> +\n>> +private:\n>> +\tExifEntry *createEntry(ExifIfd ifd, ExifTag tag);\n>> +\tExifEntry *createEntry(ExifIfd ifd, ExifTag tag, ExifFormat format,\n>> +\t\t\t uint64_t components, unsigned int size);\n>> +\n>> +\tint setShort(ExifIfd ifd, ExifTag tag, uint16_t item);\n>> +\tint setLong(ExifIfd ifd, ExifTag tag, uint32_t item);\n>> +\tint setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::string &item);\n>> +\tint setRational(ExifIfd ifd, ExifTag tag, uint32_t numerator, uint32_t denominator);\n> \n> Line wrap here too ?\n> \n>> +\n>> +\tbool valid_;\n> \n> valid_ is never used... Should it be checked in generate(), which would\n> return an error if valid_ is false ?\n\nIndeed it should.\n\nWe could also add an isValid() call - but I don't think there's any\npoint, as calling generate() is 'required' and the status should be\nchecked there.\n\nThe perhaps we should add some class documentation that states how it\nshoudl be used, something like the following:\n\n\"\"\"\nThe Exif class should be instantiated, and specific properties set\nthrough the exposed public API.\n\nOnce all desired properties have been set, the user shall call\ngenerate() to process the entries and generate the Exif data.\n\nCalls to generate() must check the return code to determine if any error\noccurred during the construction of the Exif data, and if successful the\ndata can be obtained using the data() method.\n\"\"\"\n\n\nAs this file isn't covered by doxygen, perhaps just put that text up at\nthe top of the .cpp file before the code.\n\nI would actually suspect that this would be better in the header - but\nwe have a policy to put documentation in the .cpp file. I wonder if that\npolicy only applies to doxygen covered documentation though ...\n\n> \n>> +\n>> +\tExifData *data_;\n>> +\tExifMem *mem_;\n>> +\n>> +\tunsigned char *exifData_;\n>> +\tunsigned int size_;\n>> +};\n>> +\n>> +#endif /* __ANDROID_JPEG_EXIF_H__ */\n>> diff --git a/src/android/meson.build b/src/android/meson.build\n>> index f7b81a4..ecb92f6 100644\n>> --- a/src/android/meson.build\n>> +++ b/src/android/meson.build\n>> @@ -7,6 +7,7 @@ android_hal_sources = files([\n>> 'camera_metadata.cpp',\n>> 'camera_ops.cpp',\n>> 'jpeg/encoder_libjpeg.cpp',\n>> + 'jpeg/exif.cpp',\n>> ])\n>> \n>> android_camera_metadata_sources = files([\n>> @@ -14,6 +15,7 @@ android_camera_metadata_sources = files([\n>> ])\n>> \n>> android_deps = [\n>> + dependency('libexif'),\n>> dependency('libjpeg'),\n>> ]\n>> \n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id F1345BE174\n\tfor ;\n\tThu, 3 Sep 2020 12:45:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6B6CA629B6;\n\tThu, 3 Sep 2020 14:45:38 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6643D62901\n\tfor ;\n\tThu, 3 Sep 2020 14:45:36 +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 BAEF456E;\n\tThu, 3 Sep 2020 14:45:35 +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=\"QsuAdG0o\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1599137136;\n\tbh=9pLcGHsTl2ac3wdAFBwpOhr6Um+SFhK8cIhbfKMKnzI=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=QsuAdG0o/cVUrFEgPAf7xgX6oAPrDbpvqodyZMNmQ8shOxXJo/K9SSdxy3yjJkuaT\n\t+6gQwy6ry5H6mr+LzkVI58WKAtQBWiErFU10rO+wNFYWsDqq4IvNj69GgD1hw1Ntw4\n\tyhOfoOAeoNYzBNkFBbGukERa8zVuwAkmHz7eWp+s=","To":"Laurent Pinchart ,\n\tUmang Jain ","References":"<20200828065727.9909-1-email@uajain.com>\n\t<20200828065727.9909-2-email@uajain.com>\n\t<20200828173740.GP10034@pendragon.ideasonboard.com>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<434bdf52-e382-2ab5-2647-d897519ba56f@ideasonboard.com>","Date":"Thu, 3 Sep 2020 13:45:33 +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":"<20200828173740.GP10034@pendragon.ideasonboard.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v4 1/2] libcamera: android: Add EXIF\n\tinfrastructure","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12283,"web_url":"https://patchwork.libcamera.org/comment/12283/","msgid":"<20200903233050.GN6492@pendragon.ideasonboard.com>","date":"2020-09-03T23:30:50","subject":"Re: [libcamera-devel] [PATCH v4 1/2] libcamera: android: Add EXIF\n\tinfrastructure","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Thu, Sep 03, 2020 at 01:45:33PM +0100, Kieran Bingham wrote:\n> On 28/08/2020 18:37, Laurent Pinchart wrote:\n> > On Fri, Aug 28, 2020 at 06:57:35AM +0000, Umang Jain wrote:\n> >> From: Kieran Bingham \n> >>\n> >> Provide helper classes to utilise the libexif interfaces and link\n> >> against libexif to support tag additions when creating JPEG images.\n> >>\n> >> Signed-off-by: Kieran Bingham \n> >> Signed-off-by: Umang Jain \n> >> Reviewed-by: Kieran Bingham \n> >> ---\n> >> src/android/jpeg/exif.cpp | 172 ++++++++++++++++++++++++++++++++++++++\n> >> src/android/jpeg/exif.h | 44 ++++++++++\n> >> src/android/meson.build | 2 +\n> >> 3 files changed, 218 insertions(+)\n> >> create mode 100644 src/android/jpeg/exif.cpp\n> >> create mode 100644 src/android/jpeg/exif.h\n> >>\n> >> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp\n> >> new file mode 100644\n> >> index 0000000..b49b538\n> >> --- /dev/null\n> >> +++ b/src/android/jpeg/exif.cpp\n> >> @@ -0,0 +1,172 @@\n> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> >> +/*\n> >> + * Copyright (C) 2020, Google Inc.\n> >> + *\n> >> + * exif.cpp - EXIF tag creation using libexif\n> >> + */\n> >> +\n> >> +#include \"exif.h\"\n> >> +\n> >> +#include \"libcamera/internal/log.h\"\n> >> +\n> >> +using namespace libcamera;\n> >> +\n> >> +LOG_DEFINE_CATEGORY(EXIF)\n> >> +\n> >> +Exif::Exif()\n> >> +\t: valid_(false), exifData_(0), size_(0)\n> > \n> > You should initialize exifData_ and data_ here, otherwise the destructor\n> > will access uninitialized pointers if the constructor fails.\n> \n> Well, it's just data_ that seems to be missing. ;-) But indeed it's\n> important to initialise, as there is a fail case return after mem_ is set.\n> \n> >> +{\n> >> +\t/* Create an ExifMem allocator to construct entries. */\n> >> +\tmem_ = exif_mem_new_default();\n> >> +\tif (!mem_) {\n> >> +\t\tLOG(EXIF, Fatal) << \"Failed to allocate ExifMem Allocator\";\n> > \n> > Fatal is a bit harsh, it will abort(). I'd go for Error.\n> \n> I agree.\n> \n> >> +\t\treturn;\n> >> +\t}\n> >> +\n> >> +\tdata_ = exif_data_new_mem(mem_);\n> >> +\tif (!data_) {\n> >> +\t\tLOG(EXIF, Fatal) << \"Failed to allocate an ExifData structure\";\n> > \n> > Same here, and in a few locations below.\n> > \n> >> +\t\treturn;\n> >> +\t}\n> >> +\n> >> +\tvalid_ = true;\n> >> +\n> >> +\texif_data_set_option(data_, EXIF_DATA_OPTION_FOLLOW_SPECIFICATION);\n> >> +\texif_data_set_data_type(data_, EXIF_DATA_TYPE_COMPRESSED);\n> >> +\n> >> +\t/*\n> >> +\t * Big-Endian: EXIF_BYTE_ORDER_MOTOROLA\n> >> +\t * Little Endian: EXIF_BYTE_ORDER_INTEL\n> >> +\t */\n> >> +\texif_data_set_byte_order(data_, EXIF_BYTE_ORDER_INTEL);\n> >> +\n> >> +\t/* Create the mandatory EXIF fields with default data. */\n> >> +\texif_data_fix(data_);\n> \n> Note this line for later. It creates some fields.\n\nI hadn't noticed that, good point.\n\n> It 'could' be removed, but I think it's better to use it to keep to the\n> standards by default.\n> \n> It might also be possible to put this in generate() if it doesn't modify\n> existing entries.\n> \n> >> +}\n> >> +\n> >> +Exif::~Exif()\n> >> +{\n> >> +\tif (exifData_)\n> >> +\t\tfree(exifData_);\n> >> +\n> >> +\tif (data_)\n> >> +\t\texif_data_unref(data_);\n> >> +\n> >> +\tif (mem_)\n> >> +\t\texif_mem_unref(mem_);\n> >> +}\n> >> +\n> >> +ExifEntry *Exif::createEntry(ExifIfd ifd, ExifTag tag)\n> >> +{\n> >> +\tExifContent *content = data_->ifd[ifd];\n> >> +\tExifEntry *entry = exif_content_get_entry(content, tag);\n> >> +\n> >> +\tif (entry) {\n> >> +\t\texif_entry_ref(entry);\n> >> +\t\treturn entry;\n> >> +\t}\n> > \n> > I don't think we'll hit this code path anymore as we don't reuse the\n> > EXIF generator, will we ?\n> \n> I think it's better to keep this.\n> \n> 1) exif_data_fix() used in constructor creates and populates default\n> requried entries which might (or might not) get updated.\n> \n> 2) Even if we (can) move exif_data_fix() to generate(), a caller might\n> set an entry twice. I see no reason not to handle that generically and\n> gracefully.\n\nAgreed.\n\n> >> +\n> >> +\tentry = exif_entry_new_mem(mem_);\n> >> +\tif (!entry) {\n> >> +\t\tLOG(EXIF, Fatal) << \"Failed to allocated new entry\";\n> >> +\t\treturn nullptr;\n> > \n> > When this happens, I think we should reset the valid_ flag to false, so\n> > that the Exif class user can add entries without checking for errors,\n> > and check once at the end.\n> \n> Agreed, that will simplify things nicely.\n> \n> >> +\t}\n> >> +\n> >> +\tentry->tag = tag;\n> >> +\n> >> +\texif_content_add_entry(content, entry);\n> >> +\texif_entry_initialize(entry, tag);\n> >> +\n> >> +\treturn entry;\n> >> +}\n> >> +\n> >> +ExifEntry *Exif::createEntry(ExifIfd ifd, ExifTag tag, ExifFormat format,\n> >> +\t\t\t uint64_t components, unsigned int size)\n> > \n> > uint64_t is a lot. I think unsigned int would be enough.\n> \n> http://libexif.sourceforge.net/internals/struct__ExifEntry.html\n> \n> The 'components' field is an unsigned long.\n> \n> Perhaps we should use that directly.\n> \n> >> +{\n> >> +\tExifContent *content = data_->ifd[ifd];\n> >> +\n> >> +\t/* Replace any existing entry with the same tag. */\n> >> +\tExifEntry *existing = exif_content_get_entry(content, tag);\n> >> +\texif_content_remove_entry(content, existing);\n> > \n> > Same here. Or should we keep them just in case ? That would be a bug in\n> > the caller I believe.\n> \n> Not always if defaults are replaced.\n> \n> And as mentioned, I think it's better to keep it.\n> \n> >> +\n> >> +\tExifEntry *entry = exif_entry_new_mem(mem_);\n> >> +\tif (!entry) {\n> >> +\t\tLOG(EXIF, Fatal) << \"Failed to allocated new entry\";\n> >> +\t\treturn nullptr;\n> >> +\t}\n> >> +\n> >> +\tvoid *buffer = exif_mem_alloc(mem_, size);\n> >> +\tif (!buffer) {\n> >> +\t\tLOG(EXIF, Fatal) << \"Failed to allocate buffer for variable entry\";\n> >> +\t\texif_mem_unref(mem_);\n> >> +\t\treturn nullptr;\n> >> +\t}\n> >> +\n> >> +\tentry->data = static_cast(buffer);\n> >> +\tentry->components = components;\n> >> +\tentry->format = format;\n> >> +\tentry->size = size;\n> >> +\tentry->tag = tag;\n> >> +\n> >> +\texif_content_add_entry(content, entry);\n> >> +\n> >> +\treturn entry;\n> >> +}\n> >> +\n> >> +int Exif::setShort(ExifIfd ifd, ExifTag tag, uint16_t item)\n> >> +{\n> >> +\tExifEntry *entry = createEntry(ifd, tag);\n> > \n> > \tif (!entry)\n> > \t\treturn -1;\n> > \n> > I would actually make this function void, if we reset the valid_ flag on\n> > failure. Same below.\n> \n> Agreed.\n> \n> \n> >> +\n> >> +\texif_set_short(entry->data, EXIF_BYTE_ORDER_INTEL, item);\n> >> +\texif_entry_unref(entry);\n> >> +\n> >> +\treturn 0;\n> >> +}\n> >> +\n> >> +int Exif::setLong(ExifIfd ifd, ExifTag tag, uint32_t item)\n> >> +{\n> >> +\tExifEntry *entry = createEntry(ifd, tag);\n> >> +\n> \n> This should have an\n> \tif (!entry)\n> \t\treturn;\n> \n> (and also be a void function)\n> \n> >> +\texif_set_long(entry->data, EXIF_BYTE_ORDER_INTEL, item);\n> >> +\texif_entry_unref(entry);\n> >> +\n> >> +\treturn 0;\n> >> +}\n> >> +\n> >> +int Exif::setRational(ExifIfd ifd, ExifTag tag, uint32_t numerator, uint32_t denominator)\n> > \n> > Maybe wrapping at 80 columns where possible ?\n> > \n> > As this function is privaet, you could replace numerator and denominator\n> > with an ExifRational if you want.\n> > \n> >> +{\n> >> +\tExifEntry *entry = createEntry(ifd, tag);\n> >> +\tExifRational item{ numerator, denominator };\n> >> +\n> >> +\texif_set_rational(entry->data, EXIF_BYTE_ORDER_INTEL, item);\n> >> +\texif_entry_unref(entry);\n> >> +\n> >> +\treturn 0;\n> >> +}\n> >> +\n> >> +int Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::string &item)\n> >> +{\n> >> +\t/* Pad 1 extra byte for null-terminated string. */\n> >> +\tsize_t length = item.length() + 1;\n> \n> Interestingly, there is a mention in the spec that states (at least on\n> the VERSION string) that the null-termination *isn't* required.\n\nNULL-termination isn't required for some strings. The spec isn't always\nvery clear :-S\n\n> I don't know if that applies to all strings though, so this is likely\n> 'safer' all the same.\n> \n> I guess if the field length is encoded in the entry it wasn't deemed as\n> required :-S\n> \n> Therefore, we might have to check to verify if the 'version' string can\n> be of length(5) ... Including the null-terminator could break things ...\n> but I think we'll keep this as is, and handle it if we discover the\n> null-terminator is bad.\n> \n> I believe CrOS adds the null-terminator, so that implies it's already\n> been well tested across exif-readers, so we should be ok.\n> \n> >> +\n> >> +\tExifEntry *entry = createEntry(ifd, tag, format, length, length);\n> >> +\n> >> +\tmemcpy(entry->data, item.c_str(), length);\n> >> +\texif_entry_unref(entry);\n> >> +\n> >> +\treturn 0;\n> >> +}\n> >> +\n> >> +int Exif::generate()\n> >> +{\n> >> +\tif (exifData_) {\n> >> +\t\tfree(exifData_);\n> >> +\t\texifData_ = nullptr;\n> >> +\t}\n> > \n> > This should also not happen anymore.\n> \n> 'should not' but could if a caller called generate twice.\n> \n> I'd prefer to keep it, as it prevents leaks in that instance.\n> \n> >> +\n> >> +\texif_data_save_data(data_, &exifData_, &size_);\n> >> +\n> >> +\tLOG(EXIF, Debug) << \"Created EXIF instance (\" << size_ << \" bytes)\";\n> >> +\n> >> +\treturn 0;\n> >> +}\n> >> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h\n> >> new file mode 100644\n> >> index 0000000..6c10f94\n> >> --- /dev/null\n> >> +++ b/src/android/jpeg/exif.h\n> >> @@ -0,0 +1,44 @@\n> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> >> +/*\n> >> + * Copyright (C) 2020, Google Inc.\n> >> + *\n> >> + * exif.h - EXIF tag creator using libexif\n> >> + */\n> >> +#ifndef __ANDROID_JPEG_EXIF_H__\n> >> +#define __ANDROID_JPEG_EXIF_H__\n> >> +\n> >> +#include \n> > \n> > Would be nice if we didn't have to expose this header, but the typedefs\n> > make that difficult :-(\n> \n> Indeed, it would be better to be self contained. Especially as it's not\n> required for any of the object public API.\n> \n> Why does C++ need to expose private stuff anyway :-) (that's rhetorical)\n> \n> >> +\n> >> +#include \n> >> +\n> >> +#include \n> >> +\n> >> +class Exif\n> >> +{\n> >> +public:\n> >> +\tExif();\n> >> +\t~Exif();\n> >> +\n> >> +\tlibcamera::Span data() const { return { exifData_, size_ }; }\n> >> +\tint generate();\n> \n> Given the other comments in this review, I think we should also change\n> this to:\n> \tint [[nodiscard]] generate();\n> \n> Which we now have thanks to C++17.\n\n\t[[nodiscard]] int generate();\n\n:-)\n\n> >> +\n> >> +private:\n> >> +\tExifEntry *createEntry(ExifIfd ifd, ExifTag tag);\n> >> +\tExifEntry *createEntry(ExifIfd ifd, ExifTag tag, ExifFormat format,\n> >> +\t\t\t uint64_t components, unsigned int size);\n> >> +\n> >> +\tint setShort(ExifIfd ifd, ExifTag tag, uint16_t item);\n> >> +\tint setLong(ExifIfd ifd, ExifTag tag, uint32_t item);\n> >> +\tint setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::string &item);\n> >> +\tint setRational(ExifIfd ifd, ExifTag tag, uint32_t numerator, uint32_t denominator);\n> > \n> > Line wrap here too ?\n> > \n> >> +\n> >> +\tbool valid_;\n> > \n> > valid_ is never used... Should it be checked in generate(), which would\n> > return an error if valid_ is false ?\n> \n> Indeed it should.\n> \n> We could also add an isValid() call - but I don't think there's any\n> point, as calling generate() is 'required' and the status should be\n> checked there.\n> \n> The perhaps we should add some class documentation that states how it\n> shoudl be used, something like the following:\n> \n> \"\"\"\n> The Exif class should be instantiated, and specific properties set\n> through the exposed public API.\n> \n> Once all desired properties have been set, the user shall call\n> generate() to process the entries and generate the Exif data.\n> \n> Calls to generate() must check the return code to determine if any error\n> occurred during the construction of the Exif data, and if successful the\n> data can be obtained using the data() method.\n> \"\"\"\n> \n> As this file isn't covered by doxygen, perhaps just put that text up at\n> the top of the .cpp file before the code.\n> \n> I would actually suspect that this would be better in the header - but\n> we have a policy to put documentation in the .cpp file. I wonder if that\n> policy only applies to doxygen covered documentation though ...\n> \n> >> +\n> >> +\tExifData *data_;\n> >> +\tExifMem *mem_;\n> >> +\n> >> +\tunsigned char *exifData_;\n> >> +\tunsigned int size_;\n> >> +};\n> >> +\n> >> +#endif /* __ANDROID_JPEG_EXIF_H__ */\n> >> diff --git a/src/android/meson.build b/src/android/meson.build\n> >> index f7b81a4..ecb92f6 100644\n> >> --- a/src/android/meson.build\n> >> +++ b/src/android/meson.build\n> >> @@ -7,6 +7,7 @@ android_hal_sources = files([\n> >> 'camera_metadata.cpp',\n> >> 'camera_ops.cpp',\n> >> 'jpeg/encoder_libjpeg.cpp',\n> >> + 'jpeg/exif.cpp',\n> >> ])\n> >> \n> >> android_camera_metadata_sources = files([\n> >> @@ -14,6 +15,7 @@ android_camera_metadata_sources = files([\n> >> ])\n> >> \n> >> android_deps = [\n> >> + dependency('libexif'),\n> >> dependency('libjpeg'),\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 E3757BE174\n\tfor ;\n\tThu, 3 Sep 2020 23:31:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 555D5629B6;\n\tFri, 4 Sep 2020 01:31:15 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8AEB060374\n\tfor ;\n\tFri, 4 Sep 2020 01:31:14 +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 07D42540;\n\tFri, 4 Sep 2020 01:31:13 +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=\"MwSjEUU/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1599175874;\n\tbh=lBs0JG8VRT82fTBziWhGbydTQ8fz31ToD6UpcHgjRdA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=MwSjEUU//dGXhZJGWR5nDae0YThAmkCkZsuJjFIWlUycmnahNsU/dffA2SphvafkI\n\ta3NQ/pwghYSdFSYqOeELmZ3YbG1nj6E6Ym/4W/HXyGGkCuTYyXCHSXEfYUJBqdNa6O\n\tHJ1IcJSC+Xkfndb7fXQtIp6WmaDJOJdCTJJ+kzNk=","Date":"Fri, 4 Sep 2020 02:30:50 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"<20200903233050.GN6492@pendragon.ideasonboard.com>","References":"<20200828065727.9909-1-email@uajain.com>\n\t<20200828065727.9909-2-email@uajain.com>\n\t<20200828173740.GP10034@pendragon.ideasonboard.com>\n\t<434bdf52-e382-2ab5-2647-d897519ba56f@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<434bdf52-e382-2ab5-2647-d897519ba56f@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 1/2] libcamera: android: Add EXIF\n\tinfrastructure","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\" "}}]