[libcamera-devel,v4,3/8] android: jpeg: exif: Add functions for setting various values
diff mbox series

Message ID 20210125071444.26252-4-paul.elder@ideasonboard.com
State Superseded
Delegated to: Paul Elder
Headers show
Series
  • Fill in android result metadata and EXIF tags
Related show

Commit Message

Paul Elder Jan. 25, 2021, 7:14 a.m. UTC 
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>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

---
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(-)

Comments

Laurent Pinchart Jan. 25, 2021, 9:49 a.m. UTC | #1 
Hi Paul,

Thank you for the patch.

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@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> ---
> 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>

chrono is included in exif.h, 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(&timestamp, &tm);
> @@ -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,
> +		  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());

I'm afraid you still need to pad on the left with zeros.
https://en.cppreference.com/w/cpp/utility/format would be nice, but as
we can't depend on C++20, maybe

	std::string subsec = "00" + std::to_string(msec.count());
	subsec.erase(3);

Or if it's too obscure, you can use the C++ string I/O with the format
modifiers, or just printf.

> +
> +	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(&timestamp, &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,
> +			    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]);
> +	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
> + */
> +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,
> +	};
> +
> +	enum WhiteBalance {
> +		Auto = 0,
> +		Manual = 1,
> +	};
> +
>  	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));

Does s/std::chrono::milliseconds(0)/0ms/ work
(https://en.cppreference.com/w/cpp/chrono/operator%22%22ms) ? Feel free
to pick the one you like best.

>  
>  	std::vector<unsigned char> thumbnail;
>  	generateThumbnail(source, &thumbnail);
Jacopo Mondi Jan. 25, 2021, 11:30 a.m. UTC | #2 
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@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@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(&timestamp, &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)

> @@ -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 ?

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

> +			    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 ?

> +
> +	enum WhiteBalance {
> +		Auto = 0,
> +		Manual = 1,
> +	};
> +

Same question

As I've said, it's mostly cosmetic stuff

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

>  	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@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Paul Elder Jan. 26, 2021, 2:31 a.m. UTC | #3 
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@ideasonboard.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@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(&timestamp, &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(&timestamp, &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@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@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel

Patch
diff mbox series 

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>
+#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(&timestamp, &tm);
@@ -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,
+		  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(&timestamp, &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,
+			    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]);
+	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
+ */
+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,
+	};
+
+	enum WhiteBalance {
+		Auto = 0,
+		Manual = 1,
+	};
+
 	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);