| Message ID | 20210114104035.302968-5-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Paul Elder |
| Headers | show |
| Series |
|
| Related | show |
Hi Paul, 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@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); 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 ? > + 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) > +{ > + 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) > +{ > + 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.. > + > + memcpy(entry->data, ts, length); Shouldn't this be setRational() ? Or is this to avoid calling it 3 times ? > + 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); > +} > + > +std::tuple<int, int, int> Exif::degreesToDMS(double decimalDegrees) > +{ > + 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 ? > + * \param[in] coords Pointer to coordinates latitude, longitude, and altitude, > + * first two in degrees, the third in meters > + */ > +void Exif::setGPSLocation(const double *coords) > +{ > + int latDeg, latMin, latSec, longDeg, longMin, longSec; > + > + 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 }); > + 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]) { > + 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); > + setLong(EXIF_IFD_0, EXIF_TAG_JPEG_INTERCHANGE_FORMAT_LENGTH, data_->size); > +} > + > +void Exif::setFocalLength(float length) > +{ > + ExifRational rational = { static_cast<ExifLong>(length), 1 }; > + setRational(EXIF_IFD_EXIF, EXIF_TAG_FOCAL_LENGTH, rational); > +} > + > +void Exif::setExposureTime(int64_t sec) > +{ > + 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 }; > + setRational(EXIF_IFD_EXIF, EXIF_TAG_FNUMBER, rational); > +} > + > +void Exif::setISO(int16_t iso) > +{ > + 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)); > +} > + > +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, > + 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); > + 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); > + void setSubsecTime(uint64_t subsec); > + void setSubsecTimeOriginal(uint64_t subsec); > + void setSubsecTimeDigitized(uint64_t subsec); > + > 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); > + 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_; > > -- > 2.27.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Paul, Thank you for the patch. 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@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. > > +{ > > + 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. > > + 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 :-( > > +} > > + > > +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> ? > > +{ > > + 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. > > + 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 ? > > + 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 ? > > + 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. > > +{ > > + 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 ? > > + setRational(EXIF_IFD_EXIF, EXIF_TAG_FNUMBER, rational); > > +} > > + > > +void Exif::setISO(int16_t iso) This can't be negative either. Maybe unsigned int ? > > +{ > > + 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. > > +} > > + > > +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 ? 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_; > >
Hi Laurent, Thank you for the review, On Fri, Jan 15, 2021 at 08:39:39PM +0200, Laurent Pinchart wrote: > Hi Paul, > > Thank you for the patch. > > 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@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>? > > > +{ > > > + 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. > > > + 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. > > > +} > > > + > > > +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. > 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_; > > > Thanks, Paul
Hi Paul, On Wed, Jan 20, 2021 at 07:04:28PM +0900, paul.elder@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@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_; > > > >
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); + 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 + * \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) +{ + 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) +{ + 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 }, + }; + + memcpy(entry->data, ts, length); + 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); +} + +std::tuple<int, int, int> Exif::degreesToDMS(double decimalDegrees) +{ + 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) }; +} + +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 + * \param[in] coords Pointer to coordinates latitude, longitude, and altitude, + * first two in degrees, the third in meters + */ +void Exif::setGPSLocation(const double *coords) +{ + int latDeg, latMin, latSec, longDeg, longMin, longSec; + + 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 }); + + /* 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]) { + nullTerm = i; + break; + } + } + buf.resize(nullTerm); + + 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); + setLong(EXIF_IFD_0, EXIF_TAG_JPEG_INTERCHANGE_FORMAT_LENGTH, data_->size); +} + +void Exif::setFocalLength(float length) +{ + ExifRational rational = { static_cast<ExifLong>(length), 1 }; + setRational(EXIF_IFD_EXIF, EXIF_TAG_FOCAL_LENGTH, rational); +} + +void Exif::setExposureTime(int64_t sec) +{ + 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 }; + setRational(EXIF_IFD_EXIF, EXIF_TAG_FNUMBER, rational); +} + +void Exif::setISO(int16_t iso) +{ + 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)); +} + +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)); } [[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, + 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); + 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); + void setSubsecTime(uint64_t subsec); + void setSubsecTimeOriginal(uint64_t subsec); + void setSubsecTimeDigitized(uint64_t subsec); + 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); + 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_;
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 Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- src/android/jpeg/exif.cpp | 187 ++++++++++++++++++++++++++++++++++++++ src/android/jpeg/exif.h | 41 +++++++++ 2 files changed, 228 insertions(+)