[libcamera-devel] [PATCH v4 1/2] libcamera: android: Add EXIF infrastructure
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Sep 4 01:30:50 CEST 2020
Hi Kieran,
On Thu, Sep 03, 2020 at 01:45:33PM +0100, Kieran Bingham wrote:
> On 28/08/2020 18:37, Laurent Pinchart wrote:
> > 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.
I hadn't noticed that, good point.
> 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.
Agreed.
> >> +
> >> + 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.
NULL-termination isn't required for some strings. The spec isn't always
very clear :-S
> 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.
[[nodiscard]] int generate();
:-)
> >> +
> >> +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,
Laurent Pinchart
More information about the libcamera-devel
mailing list