[{"id":12152,"web_url":"https://patchwork.libcamera.org/comment/12152/","msgid":"<20200825234620.GQ6767@pendragon.ideasonboard.com>","date":"2020-08-25T23:46:20","subject":"Re: [libcamera-devel] [PATCH v2 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 Tue, Aug 25, 2020 at 08:10:40PM +0000, Umang Jain wrote:\n> From: Kieran Bingham <kieran.bingham@ideasonboard.com>\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 <kieran.bingham@ideasonboard.com>\n> Signed-off-by: Umang Jain <email@uajain.com>\n> ---\n>  src/android/jpeg/exif.cpp | 176 ++++++++++++++++++++++++++++++++++++++\n>  src/android/jpeg/exif.h   |  45 ++++++++++\n>  src/android/meson.build   |   2 +\n>  3 files changed, 223 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..f6a9f5c\n> --- /dev/null\n> +++ b/src/android/jpeg/exif.cpp\n> @@ -0,0 +1,176 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * exif.cpp - EXIF tag creation and parser 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), exif_data_(0), size_(0)\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> +\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> +\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\ns/data/data./\n\n> +\texif_data_fix(data_);\n> +}\n> +\n> +Exif::~Exif()\n> +{\n> +\tif (exif_data_)\n> +\t\tfree(exif_data_);\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> +\tentry = 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> +\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> +\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> +\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<unsigned char *>(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> +\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> +{\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> +\tsize_t length = item.length();\n\nShouldn't string be null-terminated ?\n\n> +\n> +\tExifEntry *entry = createEntry(ifd, tag, format, length, length);\n> +\tif (!entry) {\n> +\t\tLOG(EXIF, Error) << \"Failed to add tag: \" << tag;\n\nI think you can drop this message, createEntry() already logs errors.\nThe other createEntry() calls above don't check for null, should then ?\n\n> +\t\treturn -ENOMEM;\n> +\t}\n> +\n> +\tmemcpy(entry->data, item.c_str(), length);\n> +\texif_entry_unref(entry);\n> +\n> +\treturn 0;\n> +}\n> +\n> +Span<uint8_t> Exif::generate()\n> +{\n> +\tif (exif_data_) {\n> +\t\tfree(exif_data_);\n> +\t\texif_data_ = nullptr;\n> +\t}\n> +\n> +\texif_data_save_data(data_, &exif_data_, &size_);\n> +\n> +\tLOG(EXIF, Debug) << \"Created EXIF instance (\" << size_ << \" bytes)\";\n> +\n> +\treturn { exif_data_, size_ };\n> +}\n> +\n> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h\n> new file mode 100644\n> index 0000000..7df83c7\n> --- /dev/null\n> +++ b/src/android/jpeg/exif.h\n> @@ -0,0 +1,45 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * exif.h - EXIF tag creator and parser using libexif\n\nParser too ? I only see creation here :-)\n\n> + */\n> +#ifndef __LIBCAMERA_EXIF_H__\n> +#define __LIBCAMERA_EXIF_H__\n\nThis should be __ANDROID_JPEG_EXIF_H__ to match the other header guards.\n\n> +\n> +#include <libexif/exif-data.h>\n> +\n> +#include <libcamera/span.h>\n> +\n> +#include <string>\n> +\n> +class Exif\n> +{\n> +public:\n> +\tExif();\n> +\t~Exif();\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\nShould these functions be private ? They don't seem to be used outside\nof this class, neither here, not in 2/2.\n\n> +\n> +\tlibcamera::Span<uint8_t> generate();\n\nShould this be a libcamera::Span<const uint8_t>, or do you want to allow\nmodification of the EXIF data by the caller ?\n\n> +\tunsigned char *data() const { return exif_data_; }\n\nThis should return a const pointer, or not be a const function.\n\n> +\tunsigned int size() const { return size_; }\n\nI would combine these two methods into\n\n\tlibcamera::Span<const uint8_t> data() const { return { exif_data_, size_ }; }\n\nas they are (and rightfully so) always used together.\n\nI'm not sure to be a big fan of the Exif class model to be honest. It's\nnice to cache entries that don't change, but caching all entries is an\nissue. Different JPEG frames may need different EXIF tags. For instance\none frame may be capture with flash, and another frame without. I'm\nafraid we'll be better off recreating all tags for every frame, with a\nnew ExifData instance every time. The ExifMem could still be cached, but\nI don't think it's worth it, it would be a very small optimization at\nthe expense of a more complex API (requiring a reset function for\ninstance, as well as careful handling of the lifetime of the\nexif_data_).\n\nI've had a look at the ExifMem API, and unfortunately it doesn't seem to\nbe possible to create an ExifMem instance only for\nexif_data_save_data(). Otherwise I would have recommended creating a\ncustom one that would wrap a std::vector, in order to decouple the\nlifetime of the data from the generator. The generate() function would\nreturn a std::unique_ptr to the data (or even just an instance of the\ndata, as long as we're careful not to copy it).\n\nSo, all this being said, I'd make the Exif class non-reusable, would\nturn generate() to return an int error code, and have a data function\nthat returns a span to const uint8_t.\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> +\tbool valid_;\n> +\n> +\tExifData *data_;\n> +\tExifMem *mem_;\n> +\n> +\tunsigned char *exif_data_;\n\nThis should be called exifData_;\n\n> +\tunsigned int size_;\n> +};\n> +\n> +#endif /* __LIBCAMERA_EXIF_H__ */\n> diff --git a/src/android/meson.build b/src/android/meson.build\n> index f7b81a4..0f49b25 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> @@ -15,6 +16,7 @@ android_camera_metadata_sources = files([\n>  \n>  android_deps = [\n>      dependency('libjpeg'),\n> +    dependency('libexif'),\n\nMaybe alphabetically sorted ?\n\nNot something to be addressed in this patch, but I wonder how we should\nhandle the case where libjpeg and/or libexif are not available. Making\nthem optional would be additional churn for little gain, but should\nwe disable the HAL component automatically ? Or, given that the\n\"android\" option defaults to false, maybe just failing the meson setup\nstep as already done is good enough ?\n\n>  ]\n>  \n>  android_camera_metadata = static_library('camera_metadata',","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 1B860BE173\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 25 Aug 2020 23:46:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 988D162882;\n\tWed, 26 Aug 2020 01:46:41 +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 70A25616B1\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 26 Aug 2020 01:46:40 +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 ED411B96;\n\tWed, 26 Aug 2020 01:46:39 +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=\"LtYg6d9l\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1598399200;\n\tbh=YFDtBjDa+58nnedGbR6+t5oTLeNAZhlyvPBAxHz9Wiw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LtYg6d9lz+h2br3ztdwcvAUU0k0ch6+m1UJLR7pSwoD3q1YmqlCqlgndjkSb7VUWb\n\taFHMw9hQce4z+d5OrOpA7MBcwbvii6mtUXCtrIZYmkYMdSS4CK69TiT3xQvNQYixo3\n\tPyRRnzT6c5b+LtsayH2vVFoFgcECxH6TyqJ+nl5U=","Date":"Wed, 26 Aug 2020 02:46:20 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Umang Jain <email@uajain.com>","Message-ID":"<20200825234620.GQ6767@pendragon.ideasonboard.com>","References":"<20200825201035.13553-1-email@uajain.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200825201035.13553-1-email@uajain.com>","Subject":"Re: [libcamera-devel] [PATCH v2 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":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","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\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":12156,"web_url":"https://patchwork.libcamera.org/comment/12156/","msgid":"<115c5df1-b043-adce-b06a-bbd154f4dae0@ideasonboard.com>","date":"2020-08-26T08:01:02","subject":"Re: [libcamera-devel] [PATCH v2 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 26/08/2020 00:46, Laurent Pinchart wrote:\n> Hi Umang,\n> \n> Thank you for the patch.\n> \n> On Tue, Aug 25, 2020 at 08:10:40PM +0000, Umang Jain wrote:\n>> From: Kieran Bingham <kieran.bingham@ideasonboard.com>\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 <kieran.bingham@ideasonboard.com>\n>> Signed-off-by: Umang Jain <email@uajain.com>\n>> ---\n>>  src/android/jpeg/exif.cpp | 176 ++++++++++++++++++++++++++++++++++++++\n>>  src/android/jpeg/exif.h   |  45 ++++++++++\n>>  src/android/meson.build   |   2 +\n>>  3 files changed, 223 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..f6a9f5c\n>> --- /dev/null\n>> +++ b/src/android/jpeg/exif.cpp\n>> @@ -0,0 +1,176 @@\n>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>> +/*\n>> + * Copyright (C) 2020, Google Inc.\n>> + *\n>> + * exif.cpp - EXIF tag creation and parser 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), exif_data_(0), size_(0)\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>> +\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>> +\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> \n> s/data/data./\n> \n>> +\texif_data_fix(data_);\n>> +}\n>> +\n>> +Exif::~Exif()\n>> +{\n>> +\tif (exif_data_)\n>> +\t\tfree(exif_data_);\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>> +\tentry = 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>> +\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>> +\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>> +\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<unsigned char *>(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>> +\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>> +{\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>> +\tsize_t length = item.length();\n> \n> Shouldn't string be null-terminated ?\n\nHrm ... I'm not sure if it was intentional or accidental to not pad here.\n\nThere is a comment in the ExifUtils::SetString in platform2/camera/common:\n\n> // Since the exif format is undefined, NULL termination is not necessary.\n\nHowever, I think they then go on to add an extra byte for null anyway,\nso indeed perhaps we should.\n\n> \n>> +\n>> +\tExifEntry *entry = createEntry(ifd, tag, format, length, length);\n>> +\tif (!entry) {\n>> +\t\tLOG(EXIF, Error) << \"Failed to add tag: \" << tag;\n> \n> I think you can drop this message, createEntry() already logs errors.\n> The other createEntry() calls above don't check for null, should then ?\n\nI think we can drop this.\n\n> \n>> +\t\treturn -ENOMEM;\n>> +\t}\n>> +\n>> +\tmemcpy(entry->data, item.c_str(), length);\n>> +\texif_entry_unref(entry);\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> +Span<uint8_t> Exif::generate()\n>> +{\n>> +\tif (exif_data_) {\n>> +\t\tfree(exif_data_);\n>> +\t\texif_data_ = nullptr;\n>> +\t}\n>> +\n>> +\texif_data_save_data(data_, &exif_data_, &size_);\n>> +\n>> +\tLOG(EXIF, Debug) << \"Created EXIF instance (\" << size_ << \" bytes)\";\n>> +\n>> +\treturn { exif_data_, size_ };\n>> +}\n>> +\n>> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h\n>> new file mode 100644\n>> index 0000000..7df83c7\n>> --- /dev/null\n>> +++ b/src/android/jpeg/exif.h\n>> @@ -0,0 +1,45 @@\n>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>> +/*\n>> + * Copyright (C) 2020, Google Inc.\n>> + *\n>> + * exif.h - EXIF tag creator and parser using libexif\n> \n> Parser too ? I only see creation here :-)\n\nYeah, that can be simplified ;-)\n\n> \n>> + */\n>> +#ifndef __LIBCAMERA_EXIF_H__\n>> +#define __LIBCAMERA_EXIF_H__\n> \n> This should be __ANDROID_JPEG_EXIF_H__ to match the other header guards.\n\nagreed\n\n> \n>> +\n>> +#include <libexif/exif-data.h>\n>> +\n>> +#include <libcamera/span.h>\n>> +\n>> +#include <string>\n>> +\n>> +class Exif\n>> +{\n>> +public:\n>> +\tExif();\n>> +\t~Exif();\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> Should these functions be private ? They don't seem to be used outside\n> of this class, neither here, not in 2/2.\n\nThey can I think. I prefer the usage through explicit helpers, but these\ncan also be public to manually set tags directly, which is how it was\ndone before Umang added the (better IMO) helpers.\n\nIt depends on whether all tags are better set through a helper to\nimprove readability or not.\n\nWe can always make them private, and open them up if it's better in the\nfuture.\n\n\n>> +\n>> +\tlibcamera::Span<uint8_t> generate();\n> \n> Should this be a libcamera::Span<const uint8_t>, or do you want to allow\n> modification of the EXIF data by the caller ?\n\nI agree, I think this should be const.\n\nI don't think there would be any expectation of modifying it after it\nwas generated.\n\n> \n>> +\tunsigned char *data() const { return exif_data_; }\n> \n> This should return a const pointer, or not be a const function.\n> \n>> +\tunsigned int size() const { return size_; }\n> \n> I would combine these two methods into\n> \n> \tlibcamera::Span<const uint8_t> data() const { return { exif_data_, size_ }; }\n\n\nI think data() and size() are leftovers from before generate returned a\nspan. I'm not sure they're used anymore ?\n\n\n> as they are (and rightfully so) always used together.\n> \n> I'm not sure to be a big fan of the Exif class model to be honest. It's\n> nice to cache entries that don't change, but caching all entries is an\n> issue. Different JPEG frames may need different EXIF tags. For instance\n> one frame may be capture with flash, and another frame without. I'm\n> afraid we'll be better off recreating all tags for every frame, with a\n> new ExifData instance every time. The ExifMem could still be cached, but\n> I don't think it's worth it, it would be a very small optimization at\n> the expense of a more complex API (requiring a reset function for\n> instance, as well as careful handling of the lifetime of the\n> exif_data_).\n\nOh :-( I asked Umang to do that specifically to try to reduce\nallocations, and remove the need to re-set parameters which do not\nchange each frame. (I.e. make,model,size can all be set once).\n\nIn regards to things like flash, I would expect that if it needs to be\nset, it would be set explicitly (either on or off), but then that leads\ntowards the overhead of the 4 constant (make,model,width,height) fields\nnot being very high anyway.\n\nWe could add a reset() or a .deleteTag() if we wanted too ...\nBut it depends on what the bigger overhead would be.\n\n\n> I've had a look at the ExifMem API, and unfortunately it doesn't seem to\n> be possible to create an ExifMem instance only for\n> exif_data_save_data(). Otherwise I would have recommended creating a\n> custom one that would wrap a std::vector, in order to decouple the\n> lifetime of the data from the generator. The generate() function would\n> return a std::unique_ptr to the data (or even just an instance of the\n> data, as long as we're careful not to copy it).\n\nIndeed, the libexif doesn't give us quite enough flexibility in the\nmemory allocators to be able to do anything much there.\n\n\n> So, all this being said, I'd make the Exif class non-reusable, would\n> turn generate() to return an int error code, and have a data function\n> that returns a span to const uint8_t.\n\nHrm, I like that generate returns the Span directly.\n\nI guess it could also return an empty Span on error, but that wouldn't\nconvey any information as to 'why' it failed.\n\nOne question though, is if failing to generate an Exif tag should be\nfatal for generating the JPEG. The image itself will still be usable, so\nI wonder if it should still continue. Which would make me think an empty\nspan would be fine.\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>> +\tbool valid_;\n>> +\n>> +\tExifData *data_;\n>> +\tExifMem *mem_;\n>> +\n>> +\tunsigned char *exif_data_;\n> \n> This should be called exifData_;\n\nagreed.\n\n> \n>> +\tunsigned int size_;\n>> +};\n>> +\n>> +#endif /* __LIBCAMERA_EXIF_H__ */\n>> diff --git a/src/android/meson.build b/src/android/meson.build\n>> index f7b81a4..0f49b25 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>> @@ -15,6 +16,7 @@ android_camera_metadata_sources = files([\n>>  \n>>  android_deps = [\n>>      dependency('libjpeg'),\n>> +    dependency('libexif'),\n> \n> Maybe alphabetically sorted ?\n> \n> Not something to be addressed in this patch, but I wonder how we should\n> handle the case where libjpeg and/or libexif are not available. Making\n> them optional would be additional churn for little gain, but should\n> we disable the HAL component automatically ? Or, given that the\n> \"android\" option defaults to false, maybe just failing the meson setup\n> step as already done is good enough ?\n\nI think that given enabling the android layer is an explicit option,\nfailing to find it's dependencies should be a failure, so I don't think\nthere is any change required here.\n\n>>  ]\n>>  \n>>  android_camera_metadata = static_library('camera_metadata',\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 3D3C1BE173\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 26 Aug 2020 08:01:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BB5E0628DD;\n\tWed, 26 Aug 2020 10:01:07 +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 90FD56037A\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 26 Aug 2020 10:01:06 +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 9229E53C;\n\tWed, 26 Aug 2020 10:01:04 +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=\"k+J/og4B\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1598428864;\n\tbh=Uf03MGpa1MP7Rym5Pzqgb86JDKwz1na4d0JBC0MyIMM=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=k+J/og4BIfauZBo/IEThyNLZdA6E3ZFcMMFUS5ez8uJSKTDWqd5q36OMKld/MJDhi\n\tt9TNJ4Aq1hTisgoOpmai+59depdPDClm3TcDaXgW/JXWxeF+SxwqsXXfDiJxWvU/Et\n\tPm11dntg/U2LUIUVGtgq2ps/Likw4tkjMR/pf3V4=","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>,\n\tUmang Jain <email@uajain.com>","References":"<20200825201035.13553-1-email@uajain.com>\n\t<20200825234620.GQ6767@pendragon.ideasonboard.com>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","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":"<115c5df1-b043-adce-b06a-bbd154f4dae0@ideasonboard.com>","Date":"Wed, 26 Aug 2020 09:01:02 +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":"<20200825234620.GQ6767@pendragon.ideasonboard.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v2 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":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","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\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":12159,"web_url":"https://patchwork.libcamera.org/comment/12159/","msgid":"<20200826111731.GA6077@pendragon.ideasonboard.com>","date":"2020-08-26T11:17:31","subject":"Re: [libcamera-devel] [PATCH v2 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 Wed, Aug 26, 2020 at 09:01:02AM +0100, Kieran Bingham wrote:\n> On 26/08/2020 00:46, Laurent Pinchart wrote:\n> > On Tue, Aug 25, 2020 at 08:10:40PM +0000, Umang Jain wrote:\n> >> From: Kieran Bingham <kieran.bingham@ideasonboard.com>\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 <kieran.bingham@ideasonboard.com>\n> >> Signed-off-by: Umang Jain <email@uajain.com>\n> >> ---\n> >>  src/android/jpeg/exif.cpp | 176 ++++++++++++++++++++++++++++++++++++++\n> >>  src/android/jpeg/exif.h   |  45 ++++++++++\n> >>  src/android/meson.build   |   2 +\n> >>  3 files changed, 223 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..f6a9f5c\n> >> --- /dev/null\n> >> +++ b/src/android/jpeg/exif.cpp\n> >> @@ -0,0 +1,176 @@\n> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> >> +/*\n> >> + * Copyright (C) 2020, Google Inc.\n> >> + *\n> >> + * exif.cpp - EXIF tag creation and parser 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), exif_data_(0), size_(0)\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> >> +\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> >> +\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> > \n> > s/data/data./\n> > \n> >> +\texif_data_fix(data_);\n> >> +}\n> >> +\n> >> +Exif::~Exif()\n> >> +{\n> >> +\tif (exif_data_)\n> >> +\t\tfree(exif_data_);\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> >> +\tentry = 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> >> +\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> >> +\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> >> +\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<unsigned char *>(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> >> +\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> >> +{\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> >> +\tsize_t length = item.length();\n> > \n> > Shouldn't string be null-terminated ?\n> \n> Hrm ... I'm not sure if it was intentional or accidental to not pad here.\n> \n> There is a comment in the ExifUtils::SetString in platform2/camera/common:\n> \n> > // Since the exif format is undefined, NULL termination is not necessary.\n> \n> However, I think they then go on to add an extra byte for null anyway,\n> so indeed perhaps we should.\n> \n> >> +\n> >> +\tExifEntry *entry = createEntry(ifd, tag, format, length, length);\n> >> +\tif (!entry) {\n> >> +\t\tLOG(EXIF, Error) << \"Failed to add tag: \" << tag;\n> > \n> > I think you can drop this message, createEntry() already logs errors.\n> > The other createEntry() calls above don't check for null, should then ?\n> \n> I think we can drop this.\n> \n> >> +\t\treturn -ENOMEM;\n> >> +\t}\n> >> +\n> >> +\tmemcpy(entry->data, item.c_str(), length);\n> >> +\texif_entry_unref(entry);\n> >> +\n> >> +\treturn 0;\n> >> +}\n> >> +\n> >> +Span<uint8_t> Exif::generate()\n> >> +{\n> >> +\tif (exif_data_) {\n> >> +\t\tfree(exif_data_);\n> >> +\t\texif_data_ = nullptr;\n> >> +\t}\n> >> +\n> >> +\texif_data_save_data(data_, &exif_data_, &size_);\n> >> +\n> >> +\tLOG(EXIF, Debug) << \"Created EXIF instance (\" << size_ << \" bytes)\";\n> >> +\n> >> +\treturn { exif_data_, size_ };\n> >> +}\n> >> +\n> >> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h\n> >> new file mode 100644\n> >> index 0000000..7df83c7\n> >> --- /dev/null\n> >> +++ b/src/android/jpeg/exif.h\n> >> @@ -0,0 +1,45 @@\n> >> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> >> +/*\n> >> + * Copyright (C) 2020, Google Inc.\n> >> + *\n> >> + * exif.h - EXIF tag creator and parser using libexif\n> > \n> > Parser too ? I only see creation here :-)\n> \n> Yeah, that can be simplified ;-)\n> \n> >> + */\n> >> +#ifndef __LIBCAMERA_EXIF_H__\n> >> +#define __LIBCAMERA_EXIF_H__\n> > \n> > This should be __ANDROID_JPEG_EXIF_H__ to match the other header guards.\n> \n> agreed\n> \n> >> +\n> >> +#include <libexif/exif-data.h>\n> >> +\n> >> +#include <libcamera/span.h>\n> >> +\n> >> +#include <string>\n> >> +\n> >> +class Exif\n> >> +{\n> >> +public:\n> >> +\tExif();\n> >> +\t~Exif();\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> > Should these functions be private ? They don't seem to be used outside\n> > of this class, neither here, not in 2/2.\n> \n> They can I think. I prefer the usage through explicit helpers, but these\n> can also be public to manually set tags directly, which is how it was\n> done before Umang added the (better IMO) helpers.\n> \n> It depends on whether all tags are better set through a helper to\n> improve readability or not.\n> \n> We can always make them private, and open them up if it's better in the\n> future.\n> \n> >> +\n> >> +\tlibcamera::Span<uint8_t> generate();\n> > \n> > Should this be a libcamera::Span<const uint8_t>, or do you want to allow\n> > modification of the EXIF data by the caller ?\n> \n> I agree, I think this should be const.\n> \n> I don't think there would be any expectation of modifying it after it\n> was generated.\n> \n> >> +\tunsigned char *data() const { return exif_data_; }\n> > \n> > This should return a const pointer, or not be a const function.\n> > \n> >> +\tunsigned int size() const { return size_; }\n> > \n> > I would combine these two methods into\n> > \n> > \tlibcamera::Span<const uint8_t> data() const { return { exif_data_, size_ }; }\n> \n> I think data() and size() are leftovers from before generate returned a\n> span. I'm not sure they're used anymore ?\n> \n> > as they are (and rightfully so) always used together.\n> > \n> > I'm not sure to be a big fan of the Exif class model to be honest. It's\n> > nice to cache entries that don't change, but caching all entries is an\n> > issue. Different JPEG frames may need different EXIF tags. For instance\n> > one frame may be capture with flash, and another frame without. I'm\n> > afraid we'll be better off recreating all tags for every frame, with a\n> > new ExifData instance every time. The ExifMem could still be cached, but\n> > I don't think it's worth it, it would be a very small optimization at\n> > the expense of a more complex API (requiring a reset function for\n> > instance, as well as careful handling of the lifetime of the\n> > exif_data_).\n> \n> Oh :-( I asked Umang to do that specifically to try to reduce\n> allocations, and remove the need to re-set parameters which do not\n> change each frame. (I.e. make,model,size can all be set once).\n> \n> In regards to things like flash, I would expect that if it needs to be\n> set, it would be set explicitly (either on or off), but then that leads\n> towards the overhead of the 4 constant (make,model,width,height) fields\n> not being very high anyway.\n> \n> We could add a reset() or a .deleteTag() if we wanted too ...\n> But it depends on what the bigger overhead would be.\n\nThe problem I tried to outline with the flash example isn't just whether\nflash is on or off, but when it's on, there are more tags that need to\nbe set, and those would need to be explicitly removed when flash is off.\nThe logic could become quite complex, I don't think it's worth it.\n\n> > I've had a look at the ExifMem API, and unfortunately it doesn't seem to\n> > be possible to create an ExifMem instance only for\n> > exif_data_save_data(). Otherwise I would have recommended creating a\n> > custom one that would wrap a std::vector, in order to decouple the\n> > lifetime of the data from the generator. The generate() function would\n> > return a std::unique_ptr to the data (or even just an instance of the\n> > data, as long as we're careful not to copy it).\n> \n> Indeed, the libexif doesn't give us quite enough flexibility in the\n> memory allocators to be able to do anything much there.\n> \n> > So, all this being said, I'd make the Exif class non-reusable, would\n> > turn generate() to return an int error code, and have a data function\n> > that returns a span to const uint8_t.\n> \n> Hrm, I like that generate returns the Span directly.\n> \n> I guess it could also return an empty Span on error, but that wouldn't\n> convey any information as to 'why' it failed.\n> \n> One question though, is if failing to generate an Exif tag should be\n> fatal for generating the JPEG. The image itself will still be usable, so\n> I wonder if it should still continue. Which would make me think an empty\n> span would be fine.\n\nHow happy will the camera stack be without EXIF data (Chrome OS and\nAndroid) ? I think failing may be a better option. What are the expected\nfailure causes, just bugs on our side, or could there be legitimate\nfailure reasons ?\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> >> +\tbool valid_;\n> >> +\n> >> +\tExifData *data_;\n> >> +\tExifMem *mem_;\n> >> +\n> >> +\tunsigned char *exif_data_;\n> > \n> > This should be called exifData_;\n> \n> agreed.\n> \n> >> +\tunsigned int size_;\n> >> +};\n> >> +\n> >> +#endif /* __LIBCAMERA_EXIF_H__ */\n> >> diff --git a/src/android/meson.build b/src/android/meson.build\n> >> index f7b81a4..0f49b25 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> >> @@ -15,6 +16,7 @@ android_camera_metadata_sources = files([\n> >>  \n> >>  android_deps = [\n> >>      dependency('libjpeg'),\n> >> +    dependency('libexif'),\n> > \n> > Maybe alphabetically sorted ?\n> > \n> > Not something to be addressed in this patch, but I wonder how we should\n> > handle the case where libjpeg and/or libexif are not available. Making\n> > them optional would be additional churn for little gain, but should\n> > we disable the HAL component automatically ? Or, given that the\n> > \"android\" option defaults to false, maybe just failing the meson setup\n> > step as already done is good enough ?\n> \n> I think that given enabling the android layer is an explicit option,\n> failing to find it's dependencies should be a failure, so I don't think\n> there is any change required here.\n> \n> >>  ]\n> >>  \n> >>  android_camera_metadata = static_library('camera_metadata',","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 C3F6DBE173\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 26 Aug 2020 11:17:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 54708628EE;\n\tWed, 26 Aug 2020 13:17:53 +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 2C4BE628BB\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 26 Aug 2020 13:17:52 +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 A232A53C;\n\tWed, 26 Aug 2020 13:17: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=\"pBCekbG6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1598440671;\n\tbh=SgZljKHgHGNPmXyyJhx5ZG3ds/ZOMB+/pnZzr2cu4rc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pBCekbG6C8XcUSHQkVbBL1pGhKH6LSwwJmp23GU8OC9IbAxTau99qgH6R04jzsl17\n\tAaowBbsETVbZUypWGRCn1DeuIIddp1eSmBsp476jY7D/7umb1K/ZF+IkOX6TXVdNj0\n\tRd+x3DO6i41mEhRKrYHVDGkM/X6y19JCaVikvJDw=","Date":"Wed, 26 Aug 2020 14:17:31 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Message-ID":"<20200826111731.GA6077@pendragon.ideasonboard.com>","References":"<20200825201035.13553-1-email@uajain.com>\n\t<20200825234620.GQ6767@pendragon.ideasonboard.com>\n\t<115c5df1-b043-adce-b06a-bbd154f4dae0@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<115c5df1-b043-adce-b06a-bbd154f4dae0@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 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":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","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\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":12160,"web_url":"https://patchwork.libcamera.org/comment/12160/","msgid":"<c92126fe-fd8c-dcb0-c2a7-00472117f2a9@ideasonboard.com>","date":"2020-08-26T11:27:53","subject":"Re: [libcamera-devel] [PATCH v2 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 26/08/2020 12:17, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> On Wed, Aug 26, 2020 at 09:01:02AM +0100, Kieran Bingham wrote:\n>> On 26/08/2020 00:46, Laurent Pinchart wrote:\n>>> On Tue, Aug 25, 2020 at 08:10:40PM +0000, Umang Jain wrote:\n>>>> From: Kieran Bingham <kieran.bingham@ideasonboard.com>\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 <kieran.bingham@ideasonboard.com>\n>>>> Signed-off-by: Umang Jain <email@uajain.com>\n>>>> ---\n>>>>  src/android/jpeg/exif.cpp | 176 ++++++++++++++++++++++++++++++++++++++\n>>>>  src/android/jpeg/exif.h   |  45 ++++++++++\n>>>>  src/android/meson.build   |   2 +\n>>>>  3 files changed, 223 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..f6a9f5c\n>>>> --- /dev/null\n>>>> +++ b/src/android/jpeg/exif.cpp\n>>>> @@ -0,0 +1,176 @@\n>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>>>> +/*\n>>>> + * Copyright (C) 2020, Google Inc.\n>>>> + *\n>>>> + * exif.cpp - EXIF tag creation and parser 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), exif_data_(0), size_(0)\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>>>> +\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>>>> +\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>>>\n>>> s/data/data./\n>>>\n>>>> +\texif_data_fix(data_);\n>>>> +}\n>>>> +\n>>>> +Exif::~Exif()\n>>>> +{\n>>>> +\tif (exif_data_)\n>>>> +\t\tfree(exif_data_);\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>>>> +\tentry = 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>>>> +\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>>>> +\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>>>> +\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<unsigned char *>(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>>>> +\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>>>> +{\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>>>> +\tsize_t length = item.length();\n>>>\n>>> Shouldn't string be null-terminated ?\n>>\n>> Hrm ... I'm not sure if it was intentional or accidental to not pad here.\n>>\n>> There is a comment in the ExifUtils::SetString in platform2/camera/common:\n>>\n>>> // Since the exif format is undefined, NULL termination is not necessary.\n>>\n>> However, I think they then go on to add an extra byte for null anyway,\n>> so indeed perhaps we should.\n>>\n>>>> +\n>>>> +\tExifEntry *entry = createEntry(ifd, tag, format, length, length);\n>>>> +\tif (!entry) {\n>>>> +\t\tLOG(EXIF, Error) << \"Failed to add tag: \" << tag;\n>>>\n>>> I think you can drop this message, createEntry() already logs errors.\n>>> The other createEntry() calls above don't check for null, should then ?\n>>\n>> I think we can drop this.\n>>\n>>>> +\t\treturn -ENOMEM;\n>>>> +\t}\n>>>> +\n>>>> +\tmemcpy(entry->data, item.c_str(), length);\n>>>> +\texif_entry_unref(entry);\n>>>> +\n>>>> +\treturn 0;\n>>>> +}\n>>>> +\n>>>> +Span<uint8_t> Exif::generate()\n>>>> +{\n>>>> +\tif (exif_data_) {\n>>>> +\t\tfree(exif_data_);\n>>>> +\t\texif_data_ = nullptr;\n>>>> +\t}\n>>>> +\n>>>> +\texif_data_save_data(data_, &exif_data_, &size_);\n>>>> +\n>>>> +\tLOG(EXIF, Debug) << \"Created EXIF instance (\" << size_ << \" bytes)\";\n>>>> +\n>>>> +\treturn { exif_data_, size_ };\n>>>> +}\n>>>> +\n>>>> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h\n>>>> new file mode 100644\n>>>> index 0000000..7df83c7\n>>>> --- /dev/null\n>>>> +++ b/src/android/jpeg/exif.h\n>>>> @@ -0,0 +1,45 @@\n>>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>>>> +/*\n>>>> + * Copyright (C) 2020, Google Inc.\n>>>> + *\n>>>> + * exif.h - EXIF tag creator and parser using libexif\n>>>\n>>> Parser too ? I only see creation here :-)\n>>\n>> Yeah, that can be simplified ;-)\n>>\n>>>> + */\n>>>> +#ifndef __LIBCAMERA_EXIF_H__\n>>>> +#define __LIBCAMERA_EXIF_H__\n>>>\n>>> This should be __ANDROID_JPEG_EXIF_H__ to match the other header guards.\n>>\n>> agreed\n>>\n>>>> +\n>>>> +#include <libexif/exif-data.h>\n>>>> +\n>>>> +#include <libcamera/span.h>\n>>>> +\n>>>> +#include <string>\n>>>> +\n>>>> +class Exif\n>>>> +{\n>>>> +public:\n>>>> +\tExif();\n>>>> +\t~Exif();\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>>> Should these functions be private ? They don't seem to be used outside\n>>> of this class, neither here, not in 2/2.\n>>\n>> They can I think. I prefer the usage through explicit helpers, but these\n>> can also be public to manually set tags directly, which is how it was\n>> done before Umang added the (better IMO) helpers.\n>>\n>> It depends on whether all tags are better set through a helper to\n>> improve readability or not.\n>>\n>> We can always make them private, and open them up if it's better in the\n>> future.\n>>\n>>>> +\n>>>> +\tlibcamera::Span<uint8_t> generate();\n>>>\n>>> Should this be a libcamera::Span<const uint8_t>, or do you want to allow\n>>> modification of the EXIF data by the caller ?\n>>\n>> I agree, I think this should be const.\n>>\n>> I don't think there would be any expectation of modifying it after it\n>> was generated.\n>>\n>>>> +\tunsigned char *data() const { return exif_data_; }\n>>>\n>>> This should return a const pointer, or not be a const function.\n>>>\n>>>> +\tunsigned int size() const { return size_; }\n>>>\n>>> I would combine these two methods into\n>>>\n>>> \tlibcamera::Span<const uint8_t> data() const { return { exif_data_, size_ }; }\n>>\n>> I think data() and size() are leftovers from before generate returned a\n>> span. I'm not sure they're used anymore ?\n>>\n>>> as they are (and rightfully so) always used together.\n>>>\n>>> I'm not sure to be a big fan of the Exif class model to be honest. It's\n>>> nice to cache entries that don't change, but caching all entries is an\n>>> issue. Different JPEG frames may need different EXIF tags. For instance\n>>> one frame may be capture with flash, and another frame without. I'm\n>>> afraid we'll be better off recreating all tags for every frame, with a\n>>> new ExifData instance every time. The ExifMem could still be cached, but\n>>> I don't think it's worth it, it would be a very small optimization at\n>>> the expense of a more complex API (requiring a reset function for\n>>> instance, as well as careful handling of the lifetime of the\n>>> exif_data_).\n>>\n>> Oh :-( I asked Umang to do that specifically to try to reduce\n>> allocations, and remove the need to re-set parameters which do not\n>> change each frame. (I.e. make,model,size can all be set once).\n>>\n>> In regards to things like flash, I would expect that if it needs to be\n>> set, it would be set explicitly (either on or off), but then that leads\n>> towards the overhead of the 4 constant (make,model,width,height) fields\n>> not being very high anyway.\n>>\n>> We could add a reset() or a .deleteTag() if we wanted too ...\n>> But it depends on what the bigger overhead would be.\n> \n> The problem I tried to outline with the flash example isn't just whether\n> flash is on or off, but when it's on, there are more tags that need to\n> be set, and those would need to be explicitly removed when flash is off.\n> The logic could become quite complex, I don't think it's worth it.\n\nOk, I still can't see the complexity growth yet, but we haven't got\nthere - but it's no real issue to just construct a new Exif object each\ntime.\n\n\n>>> I've had a look at the ExifMem API, and unfortunately it doesn't seem to\n>>> be possible to create an ExifMem instance only for\n>>> exif_data_save_data(). Otherwise I would have recommended creating a\n>>> custom one that would wrap a std::vector, in order to decouple the\n>>> lifetime of the data from the generator. The generate() function would\n>>> return a std::unique_ptr to the data (or even just an instance of the\n>>> data, as long as we're careful not to copy it).\n>>\n>> Indeed, the libexif doesn't give us quite enough flexibility in the\n>> memory allocators to be able to do anything much there.\n>>\n>>> So, all this being said, I'd make the Exif class non-reusable, would\n>>> turn generate() to return an int error code, and have a data function\n>>> that returns a span to const uint8_t.\n>>\n>> Hrm, I like that generate returns the Span directly.\n>>\n>> I guess it could also return an empty Span on error, but that wouldn't\n>> convey any information as to 'why' it failed.\n>>\n>> One question though, is if failing to generate an Exif tag should be\n>> fatal for generating the JPEG. The image itself will still be usable, so\n>> I wonder if it should still continue. Which would make me think an empty\n>> span would be fine.\n> \n> How happy will the camera stack be without EXIF data (Chrome OS and\n> Android) ? I think failing may be a better option. What are the expected\n> failure causes, just bugs on our side, or could there be legitimate\n> failure reasons ?\n\nThe only failure cases I can see are failure to allocate memory, (or\nperhaps some sort of bug our side indeed).\n\nThe camera stack currently operates without EXIF data and saves out\nimages happily, so I expect the lack thereof is not critical...\n\nBut given that the only anticipated failure case is 'lack of memory' or\n'bugz' ... maybe it doesn't matter so much if we fail the whole output.\nI'm not sure. It's just the 'non-criticality' of the exif that made me\nthink it isn't essential to fail hard if there isn't an exif data segement.\n\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>>>> +\tbool valid_;\n>>>> +\n>>>> +\tExifData *data_;\n>>>> +\tExifMem *mem_;\n>>>> +\n>>>> +\tunsigned char *exif_data_;\n>>>\n>>> This should be called exifData_;\n>>\n>> agreed.\n>>\n>>>> +\tunsigned int size_;\n>>>> +};\n>>>> +\n>>>> +#endif /* __LIBCAMERA_EXIF_H__ */\n>>>> diff --git a/src/android/meson.build b/src/android/meson.build\n>>>> index f7b81a4..0f49b25 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>>>> @@ -15,6 +16,7 @@ android_camera_metadata_sources = files([\n>>>>  \n>>>>  android_deps = [\n>>>>      dependency('libjpeg'),\n>>>> +    dependency('libexif'),\n>>>\n>>> Maybe alphabetically sorted ?\n>>>\n>>> Not something to be addressed in this patch, but I wonder how we should\n>>> handle the case where libjpeg and/or libexif are not available. Making\n>>> them optional would be additional churn for little gain, but should\n>>> we disable the HAL component automatically ? Or, given that the\n>>> \"android\" option defaults to false, maybe just failing the meson setup\n>>> step as already done is good enough ?\n>>\n>> I think that given enabling the android layer is an explicit option,\n>> failing to find it's dependencies should be a failure, so I don't think\n>> there is any change required here.\n>>\n>>>>  ]\n>>>>  \n>>>>  android_camera_metadata = static_library('camera_metadata',\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 489ECBD87E\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 26 Aug 2020 11:27:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CEE60628E2;\n\tWed, 26 Aug 2020 13:27:57 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3AF65628DD\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 26 Aug 2020 13:27:56 +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 9C54353C;\n\tWed, 26 Aug 2020 13:27:55 +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=\"qWmHqAGL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1598441275;\n\tbh=GXQ4uOwipfePjURAzDL1UWPxKrWgw51R9vKMw0DtyGA=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=qWmHqAGLkZWjh+c7mX6/+evINaviJugW8R7FQlkpCwy4s7KaPza7WfiTwLNC39oo2\n\to+PHeWTnhWjosgcZo4au00rn6eXC5dbPOTfi/tx+QYQw7OAS6+N9Hyh0S1JihtzfyQ\n\tbEtvmJ4igKL20r/bEGnd30hpGsgY7+iRKMylRt9k=","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","References":"<20200825201035.13553-1-email@uajain.com>\n\t<20200825234620.GQ6767@pendragon.ideasonboard.com>\n\t<115c5df1-b043-adce-b06a-bbd154f4dae0@ideasonboard.com>\n\t<20200826111731.GA6077@pendragon.ideasonboard.com>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","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":"<c92126fe-fd8c-dcb0-c2a7-00472117f2a9@ideasonboard.com>","Date":"Wed, 26 Aug 2020 12:27:53 +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":"<20200826111731.GA6077@pendragon.ideasonboard.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v2 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":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","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\" <libcamera-devel-bounces@lists.libcamera.org>"}}]