[libcamera-devel] [PATCH 4/6] android: jpeg: exif: Add functions for setting various values

[Laurent Pinchart]

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 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.

> > +{
> > +	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(&timestamp, &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_;
> >

-- 
Regards,

Laurent Pinchart