[libcamera-devel] [PATCH v2 1/2] libcamera: android: Add EXIF infrastructure

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Tue, Aug 25, 2020 at 08:10:40PM +0000, Umang Jain wrote:
> From: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> Provide helper classes to utilise the libexif interfaces and link
> against libexif to support tag additions when creating JPEG images.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> Signed-off-by: Umang Jain <email at uajain.com>
> ---
>  src/android/jpeg/exif.cpp | 176 ++++++++++++++++++++++++++++++++++++++
>  src/android/jpeg/exif.h   |  45 ++++++++++
>  src/android/meson.build   |   2 +
>  3 files changed, 223 insertions(+)
>  create mode 100644 src/android/jpeg/exif.cpp
>  create mode 100644 src/android/jpeg/exif.h
> 
> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
> new file mode 100644
> index 0000000..f6a9f5c
> --- /dev/null
> +++ b/src/android/jpeg/exif.cpp
> @@ -0,0 +1,176 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * exif.cpp - EXIF tag creation and parser using libexif
> + */
> +
> +#include "exif.h"
> +
> +#include "libcamera/internal/log.h"
> +
> +using namespace libcamera;
> +
> +LOG_DEFINE_CATEGORY(EXIF)
> +
> +Exif::Exif()
> +	: valid_(false), exif_data_(0), size_(0)
> +{
> +	/* Create an ExifMem allocator to construct entries. */
> +	mem_ = exif_mem_new_default();
> +	if (!mem_) {
> +		LOG(EXIF, Fatal) << "Failed to allocate ExifMem Allocator";
> +		return;
> +	}
> +
> +	data_ = exif_data_new_mem(mem_);
> +	if (!data_) {
> +		LOG(EXIF, Fatal) << "Failed to allocate an ExifData structure";
> +		return;
> +	}
> +
> +	valid_ = true;
> +
> +	exif_data_set_option(data_, EXIF_DATA_OPTION_FOLLOW_SPECIFICATION);
> +	exif_data_set_data_type(data_, EXIF_DATA_TYPE_COMPRESSED);
> +
> +	/*
> +	 * Big-Endian: EXIF_BYTE_ORDER_MOTOROLA
> +	 * Little Endian: EXIF_BYTE_ORDER_INTEL
> +	 */
> +	exif_data_set_byte_order(data_, EXIF_BYTE_ORDER_INTEL);
> +
> +	/* Create the mandatory EXIF fields with default data */

s/data/data./

> +	exif_data_fix(data_);
> +}
> +
> +Exif::~Exif()
> +{
> +	if (exif_data_)
> +		free(exif_data_);
> +
> +	if (data_)
> +		exif_data_unref(data_);
> +
> +	if (mem_)
> +		exif_mem_unref(mem_);
> +}
> +
> +ExifEntry *Exif::createEntry(ExifIfd ifd, ExifTag tag)
> +{
> +	ExifContent *content = data_->ifd[ifd];
> +	ExifEntry *entry = exif_content_get_entry(content, tag);
> +
> +	if (entry) {
> +		exif_entry_ref(entry);
> +		return entry;
> +	}
> +
> +	entry = exif_entry_new_mem(mem_);
> +	if (!entry) {
> +		LOG(EXIF, Fatal) << "Failed to allocated new entry";
> +		return nullptr;
> +	}
> +
> +	entry->tag = tag;
> +
> +	exif_content_add_entry(content, entry);
> +	exif_entry_initialize(entry, tag);
> +
> +	return entry;
> +}
> +
> +ExifEntry *Exif::createEntry(ExifIfd ifd, ExifTag tag, ExifFormat format,
> +			     uint64_t components, unsigned int size)
> +{
> +	ExifContent *content = data_->ifd[ifd];
> +
> +	/* Replace any existing entry with the same tag. */
> +	ExifEntry *existing = exif_content_get_entry(content, tag);
> +	exif_content_remove_entry(content, existing);
> +
> +	ExifEntry *entry = exif_entry_new_mem(mem_);
> +	if (!entry) {
> +		LOG(EXIF, Fatal) << "Failed to allocated new entry";
> +		return nullptr;
> +	}
> +
> +	void *buffer = exif_mem_alloc(mem_, size);
> +	if (!buffer) {
> +		LOG(EXIF, Fatal) << "Failed to allocate buffer for variable entry";
> +		exif_mem_unref(mem_);
> +		return nullptr;
> +	}
> +
> +	entry->data = static_cast<unsigned char *>(buffer);
> +	entry->components = components;
> +	entry->format = format;
> +	entry->size = size;
> +	entry->tag = tag;
> +
> +	exif_content_add_entry(content, entry);
> +
> +	return entry;
> +}
> +
> +int Exif::setShort(ExifIfd ifd, ExifTag tag, uint16_t item)
> +{
> +	ExifEntry *entry = createEntry(ifd, tag);
> +
> +	exif_set_short(entry->data, EXIF_BYTE_ORDER_INTEL, item);
> +	exif_entry_unref(entry);
> +
> +	return 0;
> +}
> +
> +int Exif::setLong(ExifIfd ifd, ExifTag tag, uint32_t item)
> +{
> +	ExifEntry *entry = createEntry(ifd, tag);
> +
> +	exif_set_long(entry->data, EXIF_BYTE_ORDER_INTEL, item);
> +	exif_entry_unref(entry);
> +
> +	return 0;
> +}
> +
> +int Exif::setRational(ExifIfd ifd, ExifTag tag, uint32_t numerator, uint32_t denominator)
> +{
> +	ExifEntry *entry = createEntry(ifd, tag);
> +	ExifRational item{ numerator, denominator };
> +
> +	exif_set_rational(entry->data, EXIF_BYTE_ORDER_INTEL, item);
> +	exif_entry_unref(entry);
> +
> +	return 0;
> +}
> +
> +int Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::string &item)
> +{
> +	size_t length = item.length();

Shouldn't string be null-terminated ?

> +
> +	ExifEntry *entry = createEntry(ifd, tag, format, length, length);
> +	if (!entry) {
> +		LOG(EXIF, Error) << "Failed to add tag: " << tag;

I think you can drop this message, createEntry() already logs errors.
The other createEntry() calls above don't check for null, should then ?

> +		return -ENOMEM;
> +	}
> +
> +	memcpy(entry->data, item.c_str(), length);
> +	exif_entry_unref(entry);
> +
> +	return 0;
> +}
> +
> +Span<uint8_t> Exif::generate()
> +{
> +	if (exif_data_) {
> +		free(exif_data_);
> +		exif_data_ = nullptr;
> +	}
> +
> +	exif_data_save_data(data_, &exif_data_, &size_);
> +
> +	LOG(EXIF, Debug) << "Created EXIF instance (" << size_ << " bytes)";
> +
> +	return { exif_data_, size_ };
> +}
> +
> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h
> new file mode 100644
> index 0000000..7df83c7
> --- /dev/null
> +++ b/src/android/jpeg/exif.h
> @@ -0,0 +1,45 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * exif.h - EXIF tag creator and parser using libexif

Parser too ? I only see creation here :-)

> + */
> +#ifndef __LIBCAMERA_EXIF_H__
> +#define __LIBCAMERA_EXIF_H__

This should be __ANDROID_JPEG_EXIF_H__ to match the other header guards.

> +
> +#include <libexif/exif-data.h>
> +
> +#include <libcamera/span.h>
> +
> +#include <string>
> +
> +class Exif
> +{
> +public:
> +	Exif();
> +	~Exif();
> +
> +	int setShort(ExifIfd ifd, ExifTag tag, uint16_t item);
> +	int setLong(ExifIfd ifd, ExifTag tag, uint32_t item);
> +	int setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::string &item);
> +	int setRational(ExifIfd ifd, ExifTag tag, uint32_t numerator, uint32_t denominator);

Should these functions be private ? They don't seem to be used outside
of this class, neither here, not in 2/2.

> +
> +	libcamera::Span<uint8_t> generate();

Should this be a libcamera::Span<const uint8_t>, or do you want to allow
modification of the EXIF data by the caller ?

> +	unsigned char *data() const { return exif_data_; }

This should return a const pointer, or not be a const function.

> +	unsigned int size() const { return size_; }

I would combine these two methods into

	libcamera::Span<const uint8_t> data() const { return { exif_data_, size_ }; }

as they are (and rightfully so) always used together.

I'm not sure to be a big fan of the Exif class model to be honest. It's
nice to cache entries that don't change, but caching all entries is an
issue. Different JPEG frames may need different EXIF tags. For instance
one frame may be capture with flash, and another frame without. I'm
afraid we'll be better off recreating all tags for every frame, with a
new ExifData instance every time. The ExifMem could still be cached, but
I don't think it's worth it, it would be a very small optimization at
the expense of a more complex API (requiring a reset function for
instance, as well as careful handling of the lifetime of the
exif_data_).

I've had a look at the ExifMem API, and unfortunately it doesn't seem to
be possible to create an ExifMem instance only for
exif_data_save_data(). Otherwise I would have recommended creating a
custom one that would wrap a std::vector, in order to decouple the
lifetime of the data from the generator. The generate() function would
return a std::unique_ptr to the data (or even just an instance of the
data, as long as we're careful not to copy it).

So, all this being said, I'd make the Exif class non-reusable, would
turn generate() to return an int error code, and have a data function
that returns a span to const uint8_t.

> +
> +private:
> +	ExifEntry *createEntry(ExifIfd ifd, ExifTag tag);
> +	ExifEntry *createEntry(ExifIfd ifd, ExifTag tag, ExifFormat format,
> +			       uint64_t components, unsigned int size);
> +
> +	bool valid_;
> +
> +	ExifData *data_;
> +	ExifMem *mem_;
> +
> +	unsigned char *exif_data_;

This should be called exifData_;

> +	unsigned int size_;
> +};
> +
> +#endif /* __LIBCAMERA_EXIF_H__ */
> diff --git a/src/android/meson.build b/src/android/meson.build
> index f7b81a4..0f49b25 100644
> --- a/src/android/meson.build
> +++ b/src/android/meson.build
> @@ -7,6 +7,7 @@ android_hal_sources = files([
>      'camera_metadata.cpp',
>      'camera_ops.cpp',
>      'jpeg/encoder_libjpeg.cpp',
> +    'jpeg/exif.cpp',
>  ])
>  
>  android_camera_metadata_sources = files([
> @@ -15,6 +16,7 @@ android_camera_metadata_sources = files([
>  
>  android_deps = [
>      dependency('libjpeg'),
> +    dependency('libexif'),

Maybe alphabetically sorted ?

Not something to be addressed in this patch, but I wonder how we should
handle the case where libjpeg and/or libexif are not available. Making
them optional would be additional churn for little gain, but should
we disable the HAL component automatically ? Or, given that the
"android" option defaults to false, maybe just failing the meson setup
step as already done is good enough ?

>  ]
>  
>  android_camera_metadata = static_library('camera_metadata',

-- 
Regards,

Laurent Pinchart