[libcamera-devel] [PATCH 4/6] android: jpeg: exif: Add functions for setting various values
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Jan 21 21:02:49 CET 2021
Hi Paul,
On Wed, Jan 20, 2021 at 07:04:28PM +0900, paul.elder at ideasonboard.com wrote:
> On Fri, Jan 15, 2021 at 08:39:39PM +0200, Laurent Pinchart wrote:
> > On Fri, Jan 15, 2021 at 04:00:52PM +0100, Jacopo Mondi wrote:
> > > On Thu, Jan 14, 2021 at 07:40:33PM +0900, Paul Elder wrote:
> > > > Add functions for setting the following EXIF fields:
> > > >
> > > > - GPSDatestamp
> > > > - GPSTimestamp
> > > > - GPSLocation
> > > > - GPSLatitudeRef
> > > > - GPSLatitude
> > > > - GPSLongitudeRef
> > > > - GPSLongitude
> > > > - GPSAltitudeRef
> > > > - GPSAltitude
> > > > - GPSProcessingMethod
> > > > - FocalLength
> > > > - ExposureTime
> > > > - FNumber
> > > > - ISO
> > > > - Flash
> > > > - WhiteBalance
> > > > - SubsecTime
> > > > - SubsecTimeOriginal
> > > > - SubsecTimeDigitized
> > > >
> > > > These are in preparation for fixing the following CTS tests:
> > > >
> > > > - android.hardware.camera2.cts.StillCaptureTest#testFocalLengths
> > > > - android.hardware.camera2.cts.StillCaptureTest#testJpegExif
> > >
> > > Awesome!
> > >
> > > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > > > ---
> > > > src/android/jpeg/exif.cpp | 187 ++++++++++++++++++++++++++++++++++++++
> > > > src/android/jpeg/exif.h | 41 +++++++++
> > > > 2 files changed, 228 insertions(+)
> > > >
> > > > diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
> > > > index b19cb4cd..fde07a63 100644
> > > > --- a/src/android/jpeg/exif.cpp
> > > > +++ b/src/android/jpeg/exif.cpp
> > > > @@ -7,6 +7,9 @@
> > > >
> > > > #include "exif.h"
> > > >
> > > > +#include <cmath>
> > > > +#include <tuple>
> > > > +
> > > > #include "libcamera/internal/log.h"
> > > > #include "libcamera/internal/utils.h"
> > > >
> > > > @@ -148,6 +151,16 @@ ExifEntry *Exif::createEntry(ExifIfd ifd, ExifTag tag, ExifFormat format,
> > > > return entry;
> > > > }
> > > >
> > > > +void Exif::setByte(ExifIfd ifd, ExifTag tag, uint8_t item)
> > > > +{
> > > > + ExifEntry *entry = createEntry(ifd, tag, EXIF_FORMAT_BYTE, 1, 1);
> > > > + if (!entry)
> > > > + return;
> > > > +
> > > > + memcpy(entry->data, &item, 1);
> >
> > entry->data[0] = 1;
> >
> > should do.
> >
> > >
> > > I know nothing about the exif library, but I see
> > > exif_set_[short|long|rational] etc.. Isn't there an
> > > exif_set_[byte|char] ? If it's not there, there might be a reason why
> > > it has been left out ?
> >
> > I think it's because writing entry->data[0] = value is simple enough.
> > Not sure though.
> >
> > > > + exif_entry_unref(entry);
> > > > +}
> > > > +
> > > > void Exif::setShort(ExifIfd ifd, ExifTag tag, uint16_t item)
> > > > {
> > > > ExifEntry *entry = createEntry(ifd, tag);
> > > > @@ -178,6 +191,22 @@ void Exif::setRational(ExifIfd ifd, ExifTag tag, ExifRational item)
> > > > exif_entry_unref(entry);
> > > > }
> > > >
> > > > +/*
> > > > + * \brief setArray
> > >
> > > Very brief!
> > > I think you can omit this doc block or rather expand it.
> > >
> > > > + * \param[in] size sizeof(data[0])
> > > > + * \param[in] count Number of elements in data
> > > > + */
> > > > +void Exif::setArray(ExifIfd ifd, ExifTag tag, ExifFormat format,
> > > > + const void *data, size_t size, size_t count)
> >
> > Hmmm... You're using this to set strings, and we have a setString()
> > method that handles ASCII strings already. It also supports the
> > "UNDEFINED" Exif format, which is only used to set
> > EXIF_TAG_EXIF_VERSION.
> >
> > Would it make sense to switch EXIF_TAG_EXIF_VERSION to Exif::setArray(),
> > and modify setString() to handle the encoding ? When the format argument
> > is set to EXIF_FORMAT_ASCII the setString() function would continue
> > operating as it does today, and when it is set to EXIF_FORMAT_UNDEFINED
> > it would encode the input string and add the character code prefix. A
> > new argument would need to be added to setString() to specify the
> > encoding, although we could also default to Unicode (in the Exif sense,
> > thus UTF-16) until a need to support other encodings arises.
>
> Ah, good idea.
>
> > > > +{
> > > > + ExifEntry *entry = createEntry(ifd, tag, format, count, size * count);
> > > > + if (!entry)
> > > > + return;
> > > > +
> > > > + memcpy(entry->data, data, size * count);
> > > > + exif_entry_unref(entry);
> > > > +}
> > > > +
> > > > void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::string &item)
> > > > {
> > > > std::string ascii;
> > > > @@ -254,6 +283,111 @@ void Exif::setTimestamp(time_t timestamp)
> > > > }
> > > > }
> > > >
> > > > +void Exif::setGPSTimestamp(ExifIfd ifd, ExifTag tag, const struct tm &tm)
> >
> > As there are no other Exif tag than GPSTimeStamp that stores a time in
> > this format, I'd inline this function in its only caller.
> >
> > > > +{
> > > > + size_t length = 3 * sizeof(ExifRational);
> > > > +
> > > > + ExifEntry *entry = createEntry(ifd, tag, EXIF_FORMAT_RATIONAL, 3, length);
> > > > + if (!entry)
> > > > + return;
> > > > +
> > > > + ExifRational ts[] = {
> > > > + { static_cast<ExifLong>(tm.tm_hour), 1 },
> > > > + { static_cast<ExifLong>(tm.tm_min), 1 },
> > > > + { static_cast<ExifLong>(tm.tm_sec), 1 },
> > > > + };
> > >
> > > Is this required to be rational ? the '1' here I assume is the
> > > denominator..
> >
> > Yes, that's what the Exif spec requires.
> >
> > > > +
> > > > + memcpy(entry->data, ts, length);
> > >
> > > Shouldn't this be setRational() ? Or is this to avoid calling it 3
> > > times ?
> >
> > Calling setRational() 3 times would create 3 entries, we want a single
> > entry with an array of 3 rational values. This should however call
> > exif_set_rational() in a loop instead of using memcpy(), in order to
> > deal with different endianness. Same for Exif::setGPSDMS() below.
>
> Ah yes.
>
> > > > + exif_entry_unref(entry);
> > > > +}
> > > > +
> > > > +void Exif::setGPSDateTimestamp(time_t timestamp)
> > > > +{
> > > > + struct tm tm;
> > > > + gmtime_r(×tamp, &tm);
> > > > +
> > > > + char str[11];
> > > > + strftime(str, sizeof(str), "%Y:%m:%d", &tm);
> > > > + std::string ts(str);
> > > > +
> > > > + setGPSTimestamp(EXIF_IFD_GPS, static_cast<ExifTag>(EXIF_TAG_GPS_TIME_STAMP), tm);
> > > > + setString(EXIF_IFD_GPS, static_cast<ExifTag>(EXIF_TAG_GPS_DATE_STAMP), EXIF_FORMAT_ASCII, ts);
> >
> > How about wrapping the second line ?
> >
> > Exif is an amazing spec, not defining unique values for tags which
> > results in exif-tag.h using macros for the GPS tags, requiring a cast
> > :-(
>
> Yep :S
>
> > > > +}
> > > > +
> > > > +std::tuple<int, int, int> Exif::degreesToDMS(double decimalDegrees)
> >
> > I wonder if the second and third return values should be unsigned int,
> > but that's very minor.
> >
> > > > +{
> > > > + int degrees = std::trunc(decimalDegrees);
> > > > + double minutes = std::abs((decimalDegrees - degrees) * 60);
> > > > + double seconds = (minutes - std::trunc(minutes)) * 60;
> > > > +
> > > > + return { degrees, std::trunc(minutes), std::round(seconds) };
> > > > +}
> > >
> > > I assume CTS validates the above calculations!
> > >
> > > > +
> > > > +void Exif::setGPSDMS(ExifIfd ifd, ExifTag tag, int deg, int min, int sec)
> > > > +{
> > > > + size_t length = 3 * sizeof(ExifRational);
> > > > +
> > > > + ExifEntry *entry = createEntry(ifd, tag, EXIF_FORMAT_RATIONAL, 3, length);
> > > > + if (!entry)
> > > > + return;
> > > > +
> > > > + ExifRational coords[] = {
> > > > + { static_cast<ExifLong>(deg), 1 },
> > > > + { static_cast<ExifLong>(min), 1 },
> > > > + { static_cast<ExifLong>(sec), 1 },
> > > > + };
> > > > +
> > > > + memcpy(entry->data, coords, length);
> > > > + exif_entry_unref(entry);
> > > > +}
> > > > +
> > > > +/*
> > > > + * \brief Set GPS location (lat, long, alt) from Android JPEG GPS coordinates
> > >
> > > Where do we expect the GPS coordinates to come from ?
> >
> > Android provides it in the request. I wouldn't mention Android in the
> > documentation though, from the point of view of the Exif class it
> > doesn't matter.
> >
> > > > + * \param[in] coords Pointer to coordinates latitude, longitude, and altitude,
> > > > + * first two in degrees, the third in meters
> > > > + */
> > > > +void Exif::setGPSLocation(const double *coords)
> >
> > Maybe const std::array<double, 3> ?
>
> Android gives us a pointer to a double, how can I cast that to a
> std::array<double, 3>?
I think a reinterpret_cast may do, but it's dirty. Nevermind :-)
> > > > +{
> > > > + int latDeg, latMin, latSec, longDeg, longMin, longSec;
> >
> > Three variables should be enough, deg, min, sec, you can reuse them.
> >
> > > > +
> > > > + std::tie<int, int, int>(latDeg, latMin, latSec) = degreesToDMS(coords[0]);
> > > > + setString(EXIF_IFD_GPS, static_cast<ExifTag>(EXIF_TAG_GPS_LATITUDE_REF),
> > > > + EXIF_FORMAT_ASCII, latDeg >= 0 ? "N" : "S");
> > > > + setGPSDMS(EXIF_IFD_GPS, static_cast<ExifTag>(EXIF_TAG_GPS_LATITUDE),
> > > > + std::abs(latDeg), latMin, latSec);
> > > > +
> > > > + std::tie<int, int, int>(longDeg, longMin, longSec) = degreesToDMS(coords[1]);
> > > > + setString(EXIF_IFD_GPS, static_cast<ExifTag>(EXIF_TAG_GPS_LONGITUDE_REF),
> > > > + EXIF_FORMAT_ASCII, longDeg >= 0 ? "E" : "W");
> > > > + setGPSDMS(EXIF_IFD_GPS, static_cast<ExifTag>(EXIF_TAG_GPS_LATITUDE),
> > > > + std::abs(longDeg), longMin, longSec);
> > > > +
> > > > + setByte(EXIF_IFD_GPS, static_cast<ExifTag>(EXIF_TAG_GPS_ALTITUDE_REF),
> > > > + coords[2] >= 0 ? 0 : 1);
> > > > + setRational(EXIF_IFD_GPS, static_cast<ExifTag>(EXIF_TAG_GPS_ALTITUDE),
> > > > + ExifRational{ static_cast<ExifLong>(std::abs(coords[2])), 1 });
> > > > +}
> > > > +
> > > > +void Exif::setGPSMethod(const std::string &method)
> > > > +{
> > > > + std::vector<uint8_t> buf = libcamera::utils::string_to_c16(method, true);
> > > > + /* Designate that this string is Unicode (UCS-2) */
> > > > + buf.insert(buf.begin(), { 0x55, 0x4E, 0x49, 0x43, 0x4F, 0x44, 0x45, 0x00 });
> >
> > Lower-case hex constants please.
> >
> > > > +
> > >
> > > This will probable relocate, but it's not that bad I think
> > >
> > > > + /* 8 bytes for character code designation, plus 32 bytes from android */
> > > > + unsigned int nullTerm = 39;
> > > > + for (int i = 8; i < buf.size(); i++) {
> > > > + if (!buf[i]) {
> >
> > This looks wrong, you'll stop at the first zero byte, which will very
> > likely be the very first byte as ascii characters are encoded in UTF-16
> > with one byte being zero.
> >
> > I think you should truncate the string while still in UTF-8, in the
> > caller, and encode the whole string here.
>
> With the utf8ToUtf16 function this isn't necessary anymore.
>
> > > > + nullTerm = i;
> > > > + break;
> > > > + }
> > > > + }
> > > > + buf.resize(nullTerm);
> > >
> > > Was I wrong assuming string_to_c16 stops at \0 ?
> > >
> > > > +
> > > > + setArray(EXIF_IFD_GPS, static_cast<ExifTag>(EXIF_TAG_GPS_PROCESSING_METHOD),
> > > > + EXIF_FORMAT_UNDEFINED, buf.data(), 1, buf.size());
> > > > +}
> > > > +
> > > > void Exif::setOrientation(int orientation)
> > > > {
> > > > int value;
> > > > @@ -288,6 +422,59 @@ void Exif::setThumbnail(Span<const unsigned char> thumbnail,
> > > > data_->size = thumbnail.size();
> > > >
> > > > setShort(EXIF_IFD_0, EXIF_TAG_COMPRESSION, compression);
> > > > + setLong(EXIF_IFD_0, EXIF_TAG_JPEG_INTERCHANGE_FORMAT, 0);
> >
> > This should be the offset to the JPEG SOI marker, but the spec doesn't
> > tell what the base address is used to compute the offset. Is 0 the right
> > value ?
>
> Uhhh I'm not sure. CTS isn't complaining with this.
Looking at the implementation in the IPU3 HAL, it has to be an offset.
> > > > + setLong(EXIF_IFD_0, EXIF_TAG_JPEG_INTERCHANGE_FORMAT_LENGTH, data_->size);
> > > > +}
> > > > +
> > > > +void Exif::setFocalLength(float length)
> > > > +{
> > > > + ExifRational rational = { static_cast<ExifLong>(length), 1 };
> >
> > Do you think sub-millimeter precision would be useful ?
>
> Oh, right :) android gives us mm anyway.
>
> > > > + setRational(EXIF_IFD_EXIF, EXIF_TAG_FOCAL_LENGTH, rational);
> > > > +}
> > > > +
> > > > +void Exif::setExposureTime(int64_t sec)
> >
> > Negative exposure times seem of limited use, and for this we definitely want sub-second precision. I'd pass a µs or ns argument.
>
> Well android gives us an int64...
>
> But yeah it should be ns.
>
> > > > +{
> > > > + ExifRational rational = { static_cast<ExifLong>(sec), 1 };
> > > > + setRational(EXIF_IFD_EXIF, EXIF_TAG_EXPOSURE_TIME, rational);
> > > > +}
> > > > +
> > > > +void Exif::setAperture(float size)
> > > > +{
> > > > + ExifRational rational = { static_cast<ExifLong>(size * 10000), 10000 };
> >
> > Maybe this is a bit too much precision ?
>
> No, this is fine. CTS wanted this much precision.
>
> > > > + setRational(EXIF_IFD_EXIF, EXIF_TAG_FNUMBER, rational);
> > > > +}
> > > > +
> > > > +void Exif::setISO(int16_t iso)
> >
> > This can't be negative either. Maybe unsigned int ?
>
> Yeah that's probably better.
>
> > > > +{
> > > > + setShort(EXIF_IFD_EXIF, EXIF_TAG_ISO_SPEED_RATINGS, iso);
> > > > +}
> > > > +
> > > > +void Exif::setFlash(Flash flash)
> > > > +{
> > > > + setShort(EXIF_IFD_EXIF, EXIF_TAG_FLASH, static_cast<ExifShort>(flash));
> > > > +}
> > > > +
> > > > +void Exif::setWhiteBalance(WhiteBalance wb)
> > > > +{
> > > > + setShort(EXIF_IFD_EXIF, EXIF_TAG_WHITE_BALANCE, static_cast<ExifShort>(wb));
> > > > +}
> > > > +
> > > > +void Exif::setSubsecTime(uint64_t subsec)
> > > > +{
> > > > + setString(EXIF_IFD_EXIF, EXIF_TAG_SUB_SEC_TIME,
> > > > + EXIF_FORMAT_ASCII, std::to_string(subsec));
> >
> > What's the unit of subsec ? You need to pad it with 0's on the left.
>
> I don't see anything about padding to the left in the exif spec. I see
> stuff about padding to the right, but it's optional.
You're right, my bad.
> > > > +}
> > > > +
> > > > +void Exif::setSubsecTimeOriginal(uint64_t subsec)
> > > > +{
> > > > + setString(EXIF_IFD_EXIF, EXIF_TAG_SUB_SEC_TIME_ORIGINAL,
> > > > + EXIF_FORMAT_ASCII, std::to_string(subsec));
> > > > +}
> > > > +
> > > > +void Exif::setSubsecTimeDigitized(uint64_t subsec)
> > > > +{
> > > > + setString(EXIF_IFD_EXIF, EXIF_TAG_SUB_SEC_TIME_DIGITIZED,
> > > > + EXIF_FORMAT_ASCII, std::to_string(subsec));
> > >
> > > Exif really requires a lot of information :/
> > >
> > > Good job overall!
> > >
> > > > }
> > > >
> > > > [[nodiscard]] int Exif::generate()
> > > > diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h
> > > > index 5cab4559..cd3b78cd 100644
> > > > --- a/src/android/jpeg/exif.h
> > > > +++ b/src/android/jpeg/exif.h
> > > > @@ -26,6 +26,28 @@ public:
> > > > JPEG = 6,
> > > > };
> > > >
> > > > + enum Flash {
> > > > + /* bit 0 */
> > > > + Fired = 0x01,
> > > > + /* bits 1 and 2 */
> > > > + StrobeReserved = 0x02,
> >
> > I'd drop this as it's reserved.
> >
> > > > + StrobeDetected = 0x04,
> > > > + StrobeNotDetected = 0x06,
> > > > + /* bits 3 and 4 */
> > > > + ModeCompulsoryFiring = 0x08,
> > > > + ModeCompulsorySuppression = 0x10,
> > > > + ModeAuto = 0x18,
> > > > + /* bit 5 */
> > > > + FlashNotPresent = 0x20,
> > > > + /* bit 6 */
> > > > + RedEye = 0x40,
> > > > + };
> > > > +
> > > > + enum WhiteBalance {
> > > > + Auto = 0,
> > > > + Manual = 1,
> > > > + };
> > > > +
> > > > void setMake(const std::string &make);
> > > > void setModel(const std::string &model);
> > > >
> > > > @@ -35,6 +57,19 @@ public:
> > > > Compression compression);
> > > > void setTimestamp(time_t timestamp);
> > > >
> > > > + void setGPSDateTimestamp(time_t timestamp);
> > > > + void setGPSLocation(const double *coords);
> > > > + void setGPSMethod(const std::string &method);
> >
> > Would it make sense to merge these three in a single function ?
>
> I don't think so. Android gives them to use separately.
Sure, but we don't expect only some of them to be provided, right ? Up
to you.
> > A blank line would be nice here.
> >
> > > > + void setFocalLength(float length);
> > > > + void setExposureTime(int64_t sec);
> > > > + void setAperture(float size);
> > > > + void setISO(int16_t iso);
> > > > + void setFlash(Flash flash);
> > > > + void setWhiteBalance(WhiteBalance wb);
> >
> > A blank line would be nice here.
> >
> > > > + void setSubsecTime(uint64_t subsec);
> > > > + void setSubsecTimeOriginal(uint64_t subsec);
> > > > + void setSubsecTimeDigitized(uint64_t subsec);
> >
> > How about moving these three functions to setTimestamp() ?
> >
> > > > +
> > > > libcamera::Span<const uint8_t> data() const { return { exifData_, size_ }; }
> > > > [[nodiscard]] int generate();
> > > >
> > > > @@ -43,11 +78,17 @@ private:
> > > > ExifEntry *createEntry(ExifIfd ifd, ExifTag tag, ExifFormat format,
> > > > unsigned long components, unsigned int size);
> > > >
> > > > + void setArray(ExifIfd ifd, ExifTag tag, ExifFormat format,
> > > > + const void *data, size_t size, size_t count);
> > > > + void setByte(ExifIfd ifd, ExifTag tag, uint8_t item);
> > > > 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);
> >
> > I'd add a blank line here.
> >
> > > > + void setGPSTimestamp(ExifIfd ifd, ExifTag tag, const struct tm &tm);
> > > > + std::tuple<int, int, int> degreesToDMS(double decimalDegrees);
> > > > + void setGPSDMS(ExifIfd ifd, ExifTag tag, int deg, int min, int sec);
> > > >
> > > > bool valid_;
> > > >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list