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

[Kieran Bingham]

Hi Laurent,

On 28/08/2020 18:37, Laurent Pinchart wrote:
> Hi Umang,
> 
> Thank you for the patch.
> 
> On Fri, Aug 28, 2020 at 06:57:35AM +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>
>> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> ---
>>  src/android/jpeg/exif.cpp | 172 ++++++++++++++++++++++++++++++++++++++
>>  src/android/jpeg/exif.h   |  44 ++++++++++
>>  src/android/meson.build   |   2 +
>>  3 files changed, 218 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..b49b538
>> --- /dev/null
>> +++ b/src/android/jpeg/exif.cpp
>> @@ -0,0 +1,172 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) 2020, Google Inc.
>> + *
>> + * exif.cpp - EXIF tag creation using libexif
>> + */
>> +
>> +#include "exif.h"
>> +
>> +#include "libcamera/internal/log.h"
>> +
>> +using namespace libcamera;
>> +
>> +LOG_DEFINE_CATEGORY(EXIF)
>> +
>> +Exif::Exif()
>> +	: valid_(false), exifData_(0), size_(0)
> 
> You should initialize exifData_ and data_ here, otherwise the destructor
> will access uninitialized pointers if the constructor fails.

Well, it's just data_ that seems to be missing. ;-) But indeed it's
important to initialise, as there is a fail case return after mem_ is set.


> 
>> +{
>> +	/* Create an ExifMem allocator to construct entries. */
>> +	mem_ = exif_mem_new_default();
>> +	if (!mem_) {
>> +		LOG(EXIF, Fatal) << "Failed to allocate ExifMem Allocator";
> 
> Fatal is a bit harsh, it will abort(). I'd go for Error.

I agree.

> 
>> +		return;
>> +	}
>> +
>> +	data_ = exif_data_new_mem(mem_);
>> +	if (!data_) {
>> +		LOG(EXIF, Fatal) << "Failed to allocate an ExifData structure";
> 
> Same here, and in a few locations below.
> 
>> +		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. */
>> +	exif_data_fix(data_);

Note this line for later. It creates some fields.

It 'could' be removed, but I think it's better to use it to keep to the
standards by default.

It might also be possible to put this in generate() if it doesn't modify
existing entries.

>> +}
>> +
>> +Exif::~Exif()
>> +{
>> +	if (exifData_)
>> +		free(exifData_);
>> +
>> +	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;
>> +	}
> 
> I don't think we'll hit this code path anymore as we don't reuse the
> EXIF generator, will we ?

I think it's better to keep this.

1) exif_data_fix() used in constructor creates and populates default
requried entries which might (or might not) get updated.

2) Even if we (can) move exif_data_fix() to generate(), a caller might
set an entry twice. I see no reason not to handle that generically and
gracefully.


>> +
>> +	entry = exif_entry_new_mem(mem_);
>> +	if (!entry) {
>> +		LOG(EXIF, Fatal) << "Failed to allocated new entry";
>> +		return nullptr;
> 
> When this happens, I think we should reset the valid_ flag to false, so
> that the Exif class user can add entries without checking for errors,
> and check once at the end.

Agreed, that will simplify things nicely.


>> +	}
>> +
>> +	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)
> 
> uint64_t is a lot. I think unsigned int would be enough.


http://libexif.sourceforge.net/internals/struct__ExifEntry.html

The 'components' field is an unsigned long.

Perhaps we should use that directly.

> 
>> +{
>> +	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);
> 
> Same here. Or should we keep them just in case ? That would be a bug in
> the caller I believe.

Not always if defaults are replaced.

And as mentioned, I think it's better to keep it.


>> +
>> +	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);
> 
> 	if (!entry)
> 		return -1;
> 
> I would actually make this function void, if we reset the valid_ flag on
> failure. Same below.


Agreed.


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

This should have an
	if (!entry)
		return;

(and also be a void function)

>> +	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)
> 
> Maybe wrapping at 80 columns where possible ?
> 
> As this function is privaet, you could replace numerator and denominator
> with an ExifRational if you want.
> 
>> +{
>> +	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)
>> +{
>> +	/* Pad 1 extra byte for null-terminated string. */
>> +	size_t length = item.length() + 1;

Interestingly, there is a mention in the spec that states (at least on
the VERSION string) that the null-termination *isn't* required.

I don't know if that applies to all strings though, so this is likely
'safer' all the same.

I guess if the field length is encoded in the entry it wasn't deemed as
required :-S

Therefore, we might have to check to verify if the 'version' string can
be of length(5) ... Including the null-terminator could break things ...
but I think we'll keep this as is, and handle it if we discover the
null-terminator is bad.

I believe CrOS adds the null-terminator, so that implies it's already
been well tested across exif-readers, so we should be ok.


>> +
>> +	ExifEntry *entry = createEntry(ifd, tag, format, length, length);
>> +
>> +	memcpy(entry->data, item.c_str(), length);
>> +	exif_entry_unref(entry);
>> +
>> +	return 0;
>> +}
>> +
>> +int Exif::generate()
>> +{
>> +	if (exifData_) {
>> +		free(exifData_);
>> +		exifData_ = nullptr;
>> +	}
> 
> This should also not happen anymore.

'should not' but could if a caller called generate twice.

I'd prefer to keep it, as it prevents leaks in that instance.


> 
>> +
>> +	exif_data_save_data(data_, &exifData_, &size_);
>> +
>> +	LOG(EXIF, Debug) << "Created EXIF instance (" << size_ << " bytes)";
>> +
>> +	return 0;
>> +}
>> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h
>> new file mode 100644
>> index 0000000..6c10f94
>> --- /dev/null
>> +++ b/src/android/jpeg/exif.h
>> @@ -0,0 +1,44 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) 2020, Google Inc.
>> + *
>> + * exif.h - EXIF tag creator using libexif
>> + */
>> +#ifndef __ANDROID_JPEG_EXIF_H__
>> +#define __ANDROID_JPEG_EXIF_H__
>> +
>> +#include <libexif/exif-data.h>
> 
> Would be nice if we didn't have to expose this header, but the typedefs
> make that difficult :-(

Indeed, it would be better to be self contained. Especially as it's not
required for any of the object public API.

Why does C++ need to expose private stuff anyway :-) (that's rhetorical)


>> +
>> +#include <libcamera/span.h>
>> +
>> +#include <string>
>> +
>> +class Exif
>> +{
>> +public:
>> +	Exif();
>> +	~Exif();
>> +
>> +	libcamera::Span<const uint8_t> data() const { return { exifData_, size_ }; }
>> +	int generate();

Given the other comments in this review, I think we should also change
this to:
	int [[nodiscard]] generate();

Which we now have thanks to C++17.


>> +
>> +private:
>> +	ExifEntry *createEntry(ExifIfd ifd, ExifTag tag);
>> +	ExifEntry *createEntry(ExifIfd ifd, ExifTag tag, ExifFormat format,
>> +			       uint64_t components, unsigned int size);
>> +
>> +	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);
> 
> Line wrap here too ?
> 
>> +
>> +	bool valid_;
> 
> valid_ is never used... Should it be checked in generate(), which would
> return an error if valid_ is false ?

Indeed it should.

We could also add an isValid() call - but I don't think there's any
point, as calling generate() is 'required' and the status should be
checked there.

The perhaps we should add some class documentation that states how it
shoudl be used, something like the following:

"""
The Exif class should be instantiated, and specific properties set
through the exposed public API.

Once all desired properties have been set, the user shall call
generate() to process the entries and generate the Exif data.

Calls to generate() must check the return code to determine if any error
occurred during the construction of the Exif data, and if successful the
data can be obtained using the data() method.
"""


As this file isn't covered by doxygen, perhaps just put that text up at
the top of the .cpp file before the code.

I would actually suspect that this would be better in the header - but
we have a policy to put documentation in the .cpp file. I wonder if that
policy only applies to doxygen covered documentation though ...

> 
>> +
>> +	ExifData *data_;
>> +	ExifMem *mem_;
>> +
>> +	unsigned char *exifData_;
>> +	unsigned int size_;
>> +};
>> +
>> +#endif /* __ANDROID_JPEG_EXIF_H__ */
>> diff --git a/src/android/meson.build b/src/android/meson.build
>> index f7b81a4..ecb92f6 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([
>> @@ -14,6 +15,7 @@ android_camera_metadata_sources = files([
>>  ])
>>  
>>  android_deps = [
>> +    dependency('libexif'),
>>      dependency('libjpeg'),
>>  ]
>>  
> 

-- 
Regards
--
Kieran