[libcamera-devel] [PATCH v5 1/2] libcamera: android: Add EXIF infrastructure
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Sep 4 01:33:40 CEST 2020
Hi Umang,
On Fri, Sep 04, 2020 at 02:14:05AM +0300, Laurent Pinchart wrote:
> Hi Umang,
>
> Thank you for the patch.
>
> On Thu, Sep 03, 2020 at 10:02:15PM +0530, 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 | 189 ++++++++++++++++++++++++++++++++++++++
> > src/android/jpeg/exif.h | 46 ++++++++++
> > src/android/meson.build | 2 +
> > 3 files changed, 237 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..9d8d9bc
> > --- /dev/null
> > +++ b/src/android/jpeg/exif.cpp
> > @@ -0,0 +1,189 @@
> > +/* 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)
> > +
> > +/*
> > + * 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.
> > + */
> > +Exif::Exif()
> > + : valid_(false), data_(nullptr), exifData_(0), size_(0)
> > +{
> > + /* Create an ExifMem allocator to construct entries. */
> > + mem_ = exif_mem_new_default();
> > + if (!mem_) {
> > + LOG(EXIF, Error) << "Failed to allocate ExifMem Allocator";
> > + return;
> > + }
> > +
> > + data_ = exif_data_new_mem(mem_);
> > + if (!data_) {
> > + LOG(EXIF, Error) << "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. */
> > + exif_data_fix(data_);
> > +}
> > +
> > +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;
> > + }
> > +
> > + entry = exif_entry_new_mem(mem_);
> > + if (!entry) {
> > + LOG(EXIF, Error) << "Failed to allocated new entry";
> > + valid_ = false;
> > + return nullptr;
> > + }
> > +
> > + entry->tag = tag;
>
> exif_entry_initialize() sets entry->tag, so I think you can skip this.
>
> > +
> > + exif_content_add_entry(content, entry);
> > + exif_entry_initialize(entry, tag);
> > +
> > + return entry;
> > +}
> > +
> > +ExifEntry *Exif::createEntry(ExifIfd ifd, ExifTag tag, ExifFormat format,
> > + unsigned long components, unsigned int size)
>
> I think unsigned int is enough for components, we will never need more
> than 4G components :-)
Please disregard this comment, Kieran pointed out that entry->components
is an unsigned long. I still think it's overkill, but there's value in
consistency.
> > +{
> > + 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, Error) << "Failed to allocated new entry";
> > + valid_ = false;
> > + return nullptr;
> > + }
> > +
> > + void *buffer = exif_mem_alloc(mem_, size);
> > + if (!buffer) {
> > + LOG(EXIF, Error) << "Failed to allocate buffer for variable entry";
> > + exif_mem_unref(mem_);
>
> valid_ = false;
>
> > + 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;
> > +}
> > +
> > +void Exif::setShort(ExifIfd ifd, ExifTag tag, uint16_t item)
> > +{
> > + ExifEntry *entry = createEntry(ifd, tag);
> > + if (!entry)
> > + return;
> > +
> > + exif_set_short(entry->data, EXIF_BYTE_ORDER_INTEL, item);
> > + exif_entry_unref(entry);
> > +}
> > +
> > +void Exif::setLong(ExifIfd ifd, ExifTag tag, uint32_t item)
> > +{
> > + ExifEntry *entry = createEntry(ifd, tag);
> > + if (!entry)
> > + return;
> > +
> > + exif_set_long(entry->data, EXIF_BYTE_ORDER_INTEL, item);
> > + exif_entry_unref(entry);
> > +}
> > +
> > +void Exif::setRational(ExifIfd ifd, ExifTag tag, ExifRational item)
> > +{
> > + ExifEntry *entry = createEntry(ifd, tag);
> > + if (!entry)
> > + return;
> > +
> > + exif_set_rational(entry->data, EXIF_BYTE_ORDER_INTEL, item);
> > + exif_entry_unref(entry);
> > +}
> > +
> > +void 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;
> > +
> > + ExifEntry *entry = createEntry(ifd, tag, format, length, length);
> > + if (!entry)
> > + return;
> > +
> > + memcpy(entry->data, item.c_str(), length);
> > + exif_entry_unref(entry);
> > +}
> > +
> > +int Exif::generate()
> > +{
> > + if (exifData_) {
> > + free(exifData_);
> > + exifData_ = nullptr;
> > + }
> > +
> > + if (!valid_) {
> > + LOG(EXIF, Error) << "Created EXIF instance is invalid";
> > + return -1;
> > + }
> > +
> > + exif_data_save_data(data_, &exifData_, &size_);
> > +
> > + LOG(EXIF, Debug) << "Created EXIF instance (" << size_ << " bytes)";
>
> I'd write "Generated EXIF data", as the instance is this class.
>
> > +
> > + return 0;
> > +}
> > diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h
> > new file mode 100644
> > index 0000000..8fb8ffd
> > --- /dev/null
> > +++ b/src/android/jpeg/exif.h
> > @@ -0,0 +1,46 @@
> > +/* 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 <ctime>
> > +#include <string>
> > +
> > +#include <libexif/exif-data.h>
> > +
> > +#include <libcamera/span.h>
> > +
> > +class Exif
> > +{
> > +public:
> > + Exif();
> > + ~Exif();
> > +
> > + libcamera::Span<const uint8_t> data() const { return { exifData_, size_ }; }
> > + int [[nodiscard]] generate();
>
> The attribute goes at the beginning of the line (clang generated an
> error otherwise). The attribute should also be specified in the .cpp
> file.
>
> Please make sure to (compile-)test patches before sending them :-)
>
> With those small issues fixed,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> > +
> > +private:
> > + ExifEntry *createEntry(ExifIfd ifd, ExifTag tag);
> > + ExifEntry *createEntry(ExifIfd ifd, ExifTag tag, ExifFormat format,
> > + unsigned long components, unsigned int size);
> > +
> > + void setShort(ExifIfd ifd, ExifTag tag, uint16_t item);
> > + void setLong(ExifIfd ifd, ExifTag tag, uint32_t item);
> > + void setString(ExifIfd ifd, ExifTag tag, ExifFormat format,
> > + const std::string &item);
> > + void setRational(ExifIfd ifd, ExifTag tag, ExifRational item);
> > +
> > + bool valid_;
> > +
> > + 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