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

9382 diff mbox series

From: Kieran Bingham <kieran.bingham@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@ideasonboard.com>
Signed-off-by: Umang Jain <email@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

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@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@ideasonboard.com>
> Signed-off-by: Umang Jain <email@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',

Hi Laurent,

On 26/08/2020 00:46, Laurent Pinchart wrote:
> 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@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@ideasonboard.com>
>> Signed-off-by: Umang Jain <email@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 ?

Hrm ... I'm not sure if it was intentional or accidental to not pad here.

There is a comment in the ExifUtils::SetString in platform2/camera/common:

> // Since the exif format is undefined, NULL termination is not necessary.

However, I think they then go on to add an extra byte for null anyway,
so indeed perhaps we should.

> 
>> +
>> +	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 ?

I think we can drop this.

> 
>> +		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 :-)

Yeah, that can be simplified ;-)

> 
>> + */
>> +#ifndef __LIBCAMERA_EXIF_H__
>> +#define __LIBCAMERA_EXIF_H__
> 
> This should be __ANDROID_JPEG_EXIF_H__ to match the other header guards.

agreed

> 
>> +
>> +#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.

They can I think. I prefer the usage through explicit helpers, but these
can also be public to manually set tags directly, which is how it was
done before Umang added the (better IMO) helpers.

It depends on whether all tags are better set through a helper to
improve readability or not.

We can always make them private, and open them up if it's better in the
future.


>> +
>> +	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 ?

I agree, I think this should be const.

I don't think there would be any expectation of modifying it after it
was generated.

> 
>> +	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_ }; }


I think data() and size() are leftovers from before generate returned a
span. I'm not sure they're used anymore ?


> 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_).

Oh :-( I asked Umang to do that specifically to try to reduce
allocations, and remove the need to re-set parameters which do not
change each frame. (I.e. make,model,size can all be set once).

In regards to things like flash, I would expect that if it needs to be
set, it would be set explicitly (either on or off), but then that leads
towards the overhead of the 4 constant (make,model,width,height) fields
not being very high anyway.

We could add a reset() or a .deleteTag() if we wanted too ...
But it depends on what the bigger overhead would be.


> 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).

Indeed, the libexif doesn't give us quite enough flexibility in the
memory allocators to be able to do anything much there.


> 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.

Hrm, I like that generate returns the Span directly.

I guess it could also return an empty Span on error, but that wouldn't
convey any information as to 'why' it failed.

One question though, is if failing to generate an Exif tag should be
fatal for generating the JPEG. The image itself will still be usable, so
I wonder if it should still continue. Which would make me think an empty
span would be fine.


>> +
>> +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_;

agreed.

> 
>> +	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 ?

I think that given enabling the android layer is an explicit option,
failing to find it's dependencies should be a failure, so I don't think
there is any change required here.

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

Hi Kieran,

On Wed, Aug 26, 2020 at 09:01:02AM +0100, Kieran Bingham wrote:
> On 26/08/2020 00:46, Laurent Pinchart wrote:
> > On Tue, Aug 25, 2020 at 08:10:40PM +0000, Umang Jain wrote:
> >> From: Kieran Bingham <kieran.bingham@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@ideasonboard.com>
> >> Signed-off-by: Umang Jain <email@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 ?
> 
> Hrm ... I'm not sure if it was intentional or accidental to not pad here.
> 
> There is a comment in the ExifUtils::SetString in platform2/camera/common:
> 
> > // Since the exif format is undefined, NULL termination is not necessary.
> 
> However, I think they then go on to add an extra byte for null anyway,
> so indeed perhaps we should.
> 
> >> +
> >> +	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 ?
> 
> I think we can drop this.
> 
> >> +		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 :-)
> 
> Yeah, that can be simplified ;-)
> 
> >> + */
> >> +#ifndef __LIBCAMERA_EXIF_H__
> >> +#define __LIBCAMERA_EXIF_H__
> > 
> > This should be __ANDROID_JPEG_EXIF_H__ to match the other header guards.
> 
> agreed
> 
> >> +
> >> +#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.
> 
> They can I think. I prefer the usage through explicit helpers, but these
> can also be public to manually set tags directly, which is how it was
> done before Umang added the (better IMO) helpers.
> 
> It depends on whether all tags are better set through a helper to
> improve readability or not.
> 
> We can always make them private, and open them up if it's better in the
> future.
> 
> >> +
> >> +	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 ?
> 
> I agree, I think this should be const.
> 
> I don't think there would be any expectation of modifying it after it
> was generated.
> 
> >> +	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_ }; }
> 
> I think data() and size() are leftovers from before generate returned a
> span. I'm not sure they're used anymore ?
> 
> > 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_).
> 
> Oh :-( I asked Umang to do that specifically to try to reduce
> allocations, and remove the need to re-set parameters which do not
> change each frame. (I.e. make,model,size can all be set once).
> 
> In regards to things like flash, I would expect that if it needs to be
> set, it would be set explicitly (either on or off), but then that leads
> towards the overhead of the 4 constant (make,model,width,height) fields
> not being very high anyway.
> 
> We could add a reset() or a .deleteTag() if we wanted too ...
> But it depends on what the bigger overhead would be.

The problem I tried to outline with the flash example isn't just whether
flash is on or off, but when it's on, there are more tags that need to
be set, and those would need to be explicitly removed when flash is off.
The logic could become quite complex, I don't think it's worth it.

> > 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).
> 
> Indeed, the libexif doesn't give us quite enough flexibility in the
> memory allocators to be able to do anything much there.
> 
> > 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.
> 
> Hrm, I like that generate returns the Span directly.
> 
> I guess it could also return an empty Span on error, but that wouldn't
> convey any information as to 'why' it failed.
> 
> One question though, is if failing to generate an Exif tag should be
> fatal for generating the JPEG. The image itself will still be usable, so
> I wonder if it should still continue. Which would make me think an empty
> span would be fine.

How happy will the camera stack be without EXIF data (Chrome OS and
Android) ? I think failing may be a better option. What are the expected
failure causes, just bugs on our side, or could there be legitimate
failure reasons ?

> >> +
> >> +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_;
> 
> agreed.
> 
> >> +	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 ?
> 
> I think that given enabling the android layer is an explicit option,
> failing to find it's dependencies should be a failure, so I don't think
> there is any change required here.
> 
> >>  ]
> >>  
> >>  android_camera_metadata = static_library('camera_metadata',

Hi Laurent,

On 26/08/2020 12:17, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Wed, Aug 26, 2020 at 09:01:02AM +0100, Kieran Bingham wrote:
>> On 26/08/2020 00:46, Laurent Pinchart wrote:
>>> On Tue, Aug 25, 2020 at 08:10:40PM +0000, Umang Jain wrote:
>>>> From: Kieran Bingham <kieran.bingham@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@ideasonboard.com>
>>>> Signed-off-by: Umang Jain <email@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 ?
>>
>> Hrm ... I'm not sure if it was intentional or accidental to not pad here.
>>
>> There is a comment in the ExifUtils::SetString in platform2/camera/common:
>>
>>> // Since the exif format is undefined, NULL termination is not necessary.
>>
>> However, I think they then go on to add an extra byte for null anyway,
>> so indeed perhaps we should.
>>
>>>> +
>>>> +	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 ?
>>
>> I think we can drop this.
>>
>>>> +		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 :-)
>>
>> Yeah, that can be simplified ;-)
>>
>>>> + */
>>>> +#ifndef __LIBCAMERA_EXIF_H__
>>>> +#define __LIBCAMERA_EXIF_H__
>>>
>>> This should be __ANDROID_JPEG_EXIF_H__ to match the other header guards.
>>
>> agreed
>>
>>>> +
>>>> +#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.
>>
>> They can I think. I prefer the usage through explicit helpers, but these
>> can also be public to manually set tags directly, which is how it was
>> done before Umang added the (better IMO) helpers.
>>
>> It depends on whether all tags are better set through a helper to
>> improve readability or not.
>>
>> We can always make them private, and open them up if it's better in the
>> future.
>>
>>>> +
>>>> +	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 ?
>>
>> I agree, I think this should be const.
>>
>> I don't think there would be any expectation of modifying it after it
>> was generated.
>>
>>>> +	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_ }; }
>>
>> I think data() and size() are leftovers from before generate returned a
>> span. I'm not sure they're used anymore ?
>>
>>> 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_).
>>
>> Oh :-( I asked Umang to do that specifically to try to reduce
>> allocations, and remove the need to re-set parameters which do not
>> change each frame. (I.e. make,model,size can all be set once).
>>
>> In regards to things like flash, I would expect that if it needs to be
>> set, it would be set explicitly (either on or off), but then that leads
>> towards the overhead of the 4 constant (make,model,width,height) fields
>> not being very high anyway.
>>
>> We could add a reset() or a .deleteTag() if we wanted too ...
>> But it depends on what the bigger overhead would be.
> 
> The problem I tried to outline with the flash example isn't just whether
> flash is on or off, but when it's on, there are more tags that need to
> be set, and those would need to be explicitly removed when flash is off.
> The logic could become quite complex, I don't think it's worth it.

Ok, I still can't see the complexity growth yet, but we haven't got
there - but it's no real issue to just construct a new Exif object each
time.


>>> 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).
>>
>> Indeed, the libexif doesn't give us quite enough flexibility in the
>> memory allocators to be able to do anything much there.
>>
>>> 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.
>>
>> Hrm, I like that generate returns the Span directly.
>>
>> I guess it could also return an empty Span on error, but that wouldn't
>> convey any information as to 'why' it failed.
>>
>> One question though, is if failing to generate an Exif tag should be
>> fatal for generating the JPEG. The image itself will still be usable, so
>> I wonder if it should still continue. Which would make me think an empty
>> span would be fine.
> 
> How happy will the camera stack be without EXIF data (Chrome OS and
> Android) ? I think failing may be a better option. What are the expected
> failure causes, just bugs on our side, or could there be legitimate
> failure reasons ?

The only failure cases I can see are failure to allocate memory, (or
perhaps some sort of bug our side indeed).

The camera stack currently operates without EXIF data and saves out
images happily, so I expect the lack thereof is not critical...

But given that the only anticipated failure case is 'lack of memory' or
'bugz' ... maybe it doesn't matter so much if we fail the whole output.
I'm not sure. It's just the 'non-criticality' of the exif that made me
think it isn't essential to fail hard if there isn't an exif data segement.



> 
>>>> +
>>>> +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_;
>>
>> agreed.
>>
>>>> +	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 ?
>>
>> I think that given enabling the android layer is an explicit option,
>> failing to find it's dependencies should be a failure, so I don't think
>> there is any change required here.
>>
>>>>  ]
>>>>  
>>>>  android_camera_metadata = static_library('camera_metadata',
>