[libcamera-devel] [PATCH v4 3/8] android: jpeg: exif: Add functions for setting various values
paul.elder at ideasonboard.com
paul.elder at ideasonboard.com
Tue Jan 26 03:31:06 CET 2021
Hi Jacopo,
On Mon, Jan 25, 2021 at 12:30:26PM +0100, Jacopo Mondi wrote:
> Hi Paul,
>
> On Mon, Jan 25, 2021 at 04:14:39PM +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
> >
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
>
> Seems like I mostly have cosmetic comments
>
> > ---
> > Changes in v4:
> > - change type of subsec to std::chrono::milliseconds
> >
> > Changes in v3:
> > - merge setting subsec into setting the main timestamp
> >
> > Changes in v2:
> > - some cosmetic and precision changes
> > - use the new setString
> > ---
> > src/android/jpeg/exif.cpp | 178 +++++++++++++++++++++--
> > src/android/jpeg/exif.h | 39 ++++-
> > src/android/jpeg/post_processor_jpeg.cpp | 4 +-
> > 3 files changed, 205 insertions(+), 16 deletions(-)
> >
> > diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
> > index 204a237a..0f2c2d95 100644
> > --- a/src/android/jpeg/exif.cpp
> > +++ b/src/android/jpeg/exif.cpp
> > @@ -7,7 +7,10 @@
> >
> > #include "exif.h"
> >
> > +#include <chrono>
>
> You include <chrono> in the header file in this patch. You can drop it
> here.
>
> > +#include <cmath>
> > #include <map>
> > +#include <tuple>
> > #include <uchar.h>
> >
> > #include "libcamera/internal/log.h"
> > @@ -151,6 +154,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;
> > +
> > + entry->data[0] = item;
> > + exif_entry_unref(entry);
> > +}
> > +
> > void Exif::setShort(ExifIfd ifd, ExifTag tag, uint16_t item)
> > {
> > ExifEntry *entry = createEntry(ifd, tag);
> > @@ -267,7 +280,7 @@ void Exif::setSize(const Size &size)
> > setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_X_DIMENSION, size.width);
> > }
> >
> > -void Exif::setTimestamp(time_t timestamp)
> > +void Exif::setTimestamp(time_t timestamp, std::chrono::milliseconds msec)
> > {
> > struct tm tm;
> > localtime_r(×tamp, &tm);
>
> I guess the fact we use localtime there, and gmtime in gmtime in
> setGPSDateTimestamp is intentional (you're not on GMT, so you would
> have noticed if CTS does not like this)
Yeah, the exif spec says that GPS timestamp is in UTC.
> > @@ -282,19 +295,123 @@ void Exif::setTimestamp(time_t timestamp)
> >
> > /* Query and set timezone information if available. */
> > int r = strftime(str, sizeof(str), "%z", &tm);
> > - if (r > 0) {
> > - std::string tz(str);
> > - tz.insert(3, 1, ':');
> > - setString(EXIF_IFD_EXIF,
> > - static_cast<ExifTag>(_ExifTag::OFFSET_TIME),
> > - EXIF_FORMAT_ASCII, tz);
> > - setString(EXIF_IFD_EXIF,
> > - static_cast<ExifTag>(_ExifTag::OFFSET_TIME_ORIGINAL),
> > - EXIF_FORMAT_ASCII, tz);
> > - setString(EXIF_IFD_EXIF,
> > - static_cast<ExifTag>(_ExifTag::OFFSET_TIME_DIGITIZED),
> > - EXIF_FORMAT_ASCII, tz);
> > - }
> > + if (r <= 0)
> > + return;
> > +
> > + std::string tz(str);
> > + tz.insert(3, 1, ':');
> > + setString(EXIF_IFD_EXIF,
>
> For my education: why don't you need to create entries for these
> fields ?
We do, setString handles that.
> > + static_cast<ExifTag>(_ExifTag::OFFSET_TIME),
> > + EXIF_FORMAT_ASCII, tz);
> > + setString(EXIF_IFD_EXIF,
> > + static_cast<ExifTag>(_ExifTag::OFFSET_TIME_ORIGINAL),
> > + EXIF_FORMAT_ASCII, tz);
> > + setString(EXIF_IFD_EXIF,
> > + static_cast<ExifTag>(_ExifTag::OFFSET_TIME_DIGITIZED),
> > + EXIF_FORMAT_ASCII, tz);
> > +
> > + std::string subsec = std::to_string(msec.count());
> > +
> > + setString(EXIF_IFD_EXIF, EXIF_TAG_SUB_SEC_TIME,
> > + EXIF_FORMAT_ASCII, subsec);
> > + setString(EXIF_IFD_EXIF, EXIF_TAG_SUB_SEC_TIME_ORIGINAL,
> > + EXIF_FORMAT_ASCII, subsec);
> > + setString(EXIF_IFD_EXIF, EXIF_TAG_SUB_SEC_TIME_DIGITIZED,
> > + EXIF_FORMAT_ASCII, subsec);
> > +}
> > +
> > +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 tsStr(str);
> > +
> > + setString(EXIF_IFD_GPS, static_cast<ExifTag>(EXIF_TAG_GPS_DATE_STAMP),
> > + EXIF_FORMAT_ASCII, tsStr);
> > +
> > + /* Set GPS_TIME_STAMP */
> > + ExifEntry *entry =
> > + createEntry(EXIF_IFD_GPS,
>
> Fits on the previous line
If I do that then the next two lines will indent more and go over 80 :/
> > + static_cast<ExifTag>(EXIF_TAG_GPS_TIME_STAMP),
> > + EXIF_FORMAT_RATIONAL, 3, 3 * sizeof(ExifRational));
> > + 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 },
> > + };
> > +
> > + for (int i = 0; i < 3; i++)
> > + exif_set_rational(entry->data + i * sizeof(ExifRational),
> > + order_, ts[i]);
> > +
> > + exif_entry_unref(entry);
> > +}
> > +
> > +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)
> > +{
> > + ExifEntry *entry = createEntry(ifd, tag, EXIF_FORMAT_RATIONAL, 3,
> > + 3 * sizeof(ExifRational));
> > + if (!entry)
> > + return;
> > +
> > + ExifRational coords[] = {
> > + { static_cast<ExifLong>(deg), 1 },
> > + { static_cast<ExifLong>(min), 1 },
> > + { static_cast<ExifLong>(sec), 1 },
> > + };
> > +
> > + for (int i = 0; i < 3; i++)
> > + exif_set_rational(entry->data + i * sizeof(ExifRational),
> > + order_, coords[i]);
>
> In the previous function you have an empty line here
>
> > + exif_entry_unref(entry);
> > +}
> > +
> > +/*
> > + * \brief Set GPS location (lat, long, alt)
> > + * \param[in] coords Pointer to coordinates latitude, longitude, and altitude,
> > + * first two in degrees, the third in meters
>
> You can align to the left
>
> > + */
> > +void Exif::setGPSLocation(const double *coords)
> > +{
> > + int deg, min, sec;
> > +
> > + std::tie<int, int, int>(deg, min, sec) = degreesToDMS(coords[0]);
> > + setString(EXIF_IFD_GPS, static_cast<ExifTag>(EXIF_TAG_GPS_LATITUDE_REF),
> > + EXIF_FORMAT_ASCII, deg >= 0 ? "N" : "S");
> > + setGPSDMS(EXIF_IFD_GPS, static_cast<ExifTag>(EXIF_TAG_GPS_LATITUDE),
> > + std::abs(deg), min, sec);
> > +
> > + std::tie<int, int, int>(deg, min, sec) = degreesToDMS(coords[1]);
> > + setString(EXIF_IFD_GPS, static_cast<ExifTag>(EXIF_TAG_GPS_LONGITUDE_REF),
> > + EXIF_FORMAT_ASCII, deg >= 0 ? "E" : "W");
> > + setGPSDMS(EXIF_IFD_GPS, static_cast<ExifTag>(EXIF_TAG_GPS_LATITUDE),
> > + std::abs(deg), min, sec);
> > +
> > + 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)
> > +{
> > + setString(EXIF_IFD_GPS, static_cast<ExifTag>(EXIF_TAG_GPS_PROCESSING_METHOD),
> > + EXIF_FORMAT_UNDEFINED, method, Unicode);
> > }
> >
> > void Exif::setOrientation(int orientation)
> > @@ -333,6 +450,39 @@ void Exif::setThumbnail(Span<const unsigned char> thumbnail,
> > setShort(EXIF_IFD_0, EXIF_TAG_COMPRESSION, compression);
> > }
> >
> > +void Exif::setFocalLength(float length)
> > +{
> > + ExifRational rational = { static_cast<ExifLong>(length * 1000), 1000 };
> > + setRational(EXIF_IFD_EXIF, EXIF_TAG_FOCAL_LENGTH, rational);
> > +}
> > +
> > +void Exif::setExposureTime(uint64_t nsec)
> > +{
> > + ExifRational rational = { static_cast<ExifLong>(nsec), 1000000000 };
> > + 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(uint16_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));
> > +}
> > +
> > /**
> > * \brief Convert UTF-8 string to UTF-16 string
> > * \param[in] str String to convert
> > diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h
> > index 8b84165b..b0d61402 100644
> > --- a/src/android/jpeg/exif.h
> > +++ b/src/android/jpeg/exif.h
> > @@ -7,6 +7,7 @@
> > #ifndef __ANDROID_JPEG_EXIF_H__
> > #define __ANDROID_JPEG_EXIF_H__
> >
> > +#include <chrono>
> > #include <string>
> > #include <time.h>
> >
> > @@ -26,6 +27,27 @@ public:
> > JPEG = 6,
> > };
> >
> > + enum Flash {
> > + /* bit 0 */
> > + Fired = 0x01,
> > + /* bits 1 and 2 */
> > + StrobeDetected = 0x04,
> > + StrobeNotDetected = 0x06,
> > + /* bits 3 and 4 */
> > + ModeCompulsoryFiring = 0x08,
> > + ModeCompulsorySuppression = 0x10,
> > + ModeAuto = 0x18,
> > + /* bit 5 */
> > + FlashNotPresent = 0x20,
> > + /* bit 6 */
> > + RedEye = 0x40,
> > + };
>
> Do these come from the Exif spec ? are there no types defined for this
> already ?
Yeah it does.
> > +
> > + enum WhiteBalance {
> > + Auto = 0,
> > + Manual = 1,
> > + };
> > +
>
> Same question
Same.
> As I've said, it's mostly cosmetic stuff
>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
Thanks,
Paul
>
> > enum StringEncoding {
> > NoEncoding = 0,
> > ASCII = 1,
> > @@ -39,7 +61,18 @@ public:
> > void setSize(const libcamera::Size &size);
> > void setThumbnail(libcamera::Span<const unsigned char> thumbnail,
> > Compression compression);
> > - void setTimestamp(time_t timestamp);
> > + void setTimestamp(time_t timestamp, std::chrono::milliseconds msec);
> > +
> > + void setGPSDateTimestamp(time_t timestamp);
> > + void setGPSLocation(const double *coords);
> > + void setGPSMethod(const std::string &method);
> > +
> > + void setFocalLength(float length);
> > + void setExposureTime(uint64_t nsec);
> > + void setAperture(float size);
> > + void setISO(uint16_t iso);
> > + void setFlash(Flash flash);
> > + void setWhiteBalance(WhiteBalance wb);
> >
> > libcamera::Span<const uint8_t> data() const { return { exifData_, size_ }; }
> > [[nodiscard]] int generate();
> > @@ -49,6 +82,7 @@ private:
> > ExifEntry *createEntry(ExifIfd ifd, ExifTag tag, ExifFormat format,
> > unsigned long components, unsigned int size);
> >
> > + 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,
> > @@ -56,6 +90,9 @@ private:
> > StringEncoding encoding = NoEncoding);
> > void setRational(ExifIfd ifd, ExifTag tag, ExifRational item);
> >
> > + std::tuple<int, int, int> degreesToDMS(double decimalDegrees);
> > + void setGPSDMS(ExifIfd ifd, ExifTag tag, int deg, int min, int sec);
> > +
> > std::u16string utf8ToUtf16(const std::string &str);
> >
> > bool valid_;
> > diff --git a/src/android/jpeg/post_processor_jpeg.cpp b/src/android/jpeg/post_processor_jpeg.cpp
> > index 436a50f8..c29cb352 100644
> > --- a/src/android/jpeg/post_processor_jpeg.cpp
> > +++ b/src/android/jpeg/post_processor_jpeg.cpp
> > @@ -7,6 +7,8 @@
> >
> > #include "post_processor_jpeg.h"
> >
> > +#include <chrono>
> > +
> > #include "../camera_device.h"
> > #include "../camera_metadata.h"
> > #include "encoder_libjpeg.h"
> > @@ -97,7 +99,7 @@ int PostProcessorJpeg::process(const FrameBuffer &source,
> > * Since the precision we need for EXIF timestamp is only one
> > * second, it is good enough.
> > */
> > - exif.setTimestamp(std::time(nullptr));
> > + exif.setTimestamp(std::time(nullptr), std::chrono::milliseconds(0));
> >
> > std::vector<unsigned char> thumbnail;
> > generateThumbnail(source, &thumbnail);
> > --
> > 2.27.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list