[libcamera-devel,v4,2/2] android: jpeg: Support an initial set of EXIF metadata tags

Message ID 20200828065727.9909-3-email@uajain.com
State Accepted
Headers show
Series
  • Initial EXIF metadata support
Related show

Commit Message

Umang Jain Aug. 28, 2020, 6:57 a.m. UTC
Create a Exif object with various metadata tags set, just before
the encoder starts to encode the frame. The object is passed
directly as libcamera::Span<> to make sure EXIF tags can be set
in a single place i.e. in CameraDevice and the encoder only has
the job to write the data in the final output.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Umang Jain <email@uajain.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/android/camera_device.cpp        | 18 +++++++-
 src/android/jpeg/encoder.h           |  3 +-
 src/android/jpeg/encoder_libjpeg.cpp |  9 +++-
 src/android/jpeg/encoder_libjpeg.h   |  3 +-
 src/android/jpeg/exif.cpp            | 62 ++++++++++++++++++++++++++++
 src/android/jpeg/exif.h              |  9 ++++
 6 files changed, 100 insertions(+), 4 deletions(-)

Comments

Laurent Pinchart Aug. 28, 2020, 6:33 p.m. UTC | #1
Hi Umang,

Thank you for the patch.

On Fri, Aug 28, 2020 at 06:57:40AM +0000, Umang Jain wrote:
> Create a Exif object with various metadata tags set, just before
> the encoder starts to encode the frame. The object is passed
> directly as libcamera::Span<> to make sure EXIF tags can be set
> in a single place i.e. in CameraDevice and the encoder only has
> the job to write the data in the final output.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Umang Jain <email@uajain.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/android/camera_device.cpp        | 18 +++++++-
>  src/android/jpeg/encoder.h           |  3 +-
>  src/android/jpeg/encoder_libjpeg.cpp |  9 +++-
>  src/android/jpeg/encoder_libjpeg.h   |  3 +-
>  src/android/jpeg/exif.cpp            | 62 ++++++++++++++++++++++++++++
>  src/android/jpeg/exif.h              |  9 ++++
>  6 files changed, 100 insertions(+), 4 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index de6f86f..54ca9c6 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -24,6 +24,7 @@
>  #include "system/graphics.h"
>  
>  #include "jpeg/encoder_libjpeg.h"
> +#include "jpeg/exif.h"
>  
>  using namespace libcamera;
>  
> @@ -1434,7 +1435,22 @@ void CameraDevice::requestComplete(Request *request)
>  			continue;
>  		}
>  
> -		int jpeg_size = encoder->encode(buffer, mapped.maps()[0]);
> +		/* Set EXIF metadata for various tags. */
> +		Exif exif;
> +		/* \todo Set Make and Model from external vendor tags. */
> +		exif.setMake("libcamera");
> +		exif.setModel("cameraModel");
> +		exif.setOrientation(orientation_);
> +		exif.setSize(cameraStream->size);
> +		/*
> +		 * We set the frame's EXIF timestamp as the time of encode.
> +		 * Since the precision we need for EXIF timestamp is only one
> +		 * second, it is good enough.
> +		 */
> +		exif.setTimestamp(std::time(nullptr));
> +		exif.generate();
> +
> +		int jpeg_size = encoder->encode(buffer, mapped.maps()[0], exif.data());
>  		if (jpeg_size < 0) {
>  			LOG(HAL, Error) << "Failed to encode stream image";
>  			status = CAMERA3_BUFFER_STATUS_ERROR;
> diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h
> index f9eb88e..cf26d67 100644
> --- a/src/android/jpeg/encoder.h
> +++ b/src/android/jpeg/encoder.h
> @@ -18,7 +18,8 @@ public:
>  
>  	virtual int configure(const libcamera::StreamConfiguration &cfg) = 0;
>  	virtual int encode(const libcamera::FrameBuffer *source,
> -			   const libcamera::Span<uint8_t> &destination) = 0;
> +			   const libcamera::Span<uint8_t> &destination,
> +			   const libcamera::Span<const uint8_t> &exifData) = 0;
>  };
>  
>  #endif /* __ANDROID_JPEG_ENCODER_H__ */
> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp
> index 977538c..510613c 100644
> --- a/src/android/jpeg/encoder_libjpeg.cpp
> +++ b/src/android/jpeg/encoder_libjpeg.cpp
> @@ -180,7 +180,8 @@ void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame)
>  }
>  
>  int EncoderLibJpeg::encode(const FrameBuffer *source,
> -			   const libcamera::Span<uint8_t> &dest)
> +			   const libcamera::Span<uint8_t> &dest,
> +			   const libcamera::Span<const uint8_t> &exifData)
>  {
>  	MappedFrameBuffer frame(source, PROT_READ);
>  	if (!frame.isValid()) {
> @@ -204,6 +205,12 @@ int EncoderLibJpeg::encode(const FrameBuffer *source,
>  
>  	jpeg_start_compress(&compress_, TRUE);
>  
> +	if (exifData.size())
> +		/* Store Exif data in the JPEG_APP1 data block. */
> +		jpeg_write_marker(&compress_, JPEG_APP0 + 1,
> +				  static_cast<const JOCTET *>(exifData.data()),
> +				  exifData.size());
> +
>  	LOG(JPEG, Debug) << "JPEG Encode Starting:" << compress_.image_width
>  			 << "x" << compress_.image_height;
>  
> diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h
> index aed081a..1e8df05 100644
> --- a/src/android/jpeg/encoder_libjpeg.h
> +++ b/src/android/jpeg/encoder_libjpeg.h
> @@ -22,7 +22,8 @@ public:
>  
>  	int configure(const libcamera::StreamConfiguration &cfg) override;
>  	int encode(const libcamera::FrameBuffer *source,
> -		   const libcamera::Span<uint8_t> &destination) override;
> +		   const libcamera::Span<uint8_t> &destination,
> +		   const libcamera::Span<const uint8_t> &exifData) override;
>  
>  private:
>  	void compressRGB(const libcamera::MappedBuffer *frame);
> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
> index b49b538..3fbb117 100644
> --- a/src/android/jpeg/exif.cpp
> +++ b/src/android/jpeg/exif.cpp
> @@ -157,6 +157,68 @@ int Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::stri
>  	return 0;
>  }
>  
> +int Exif::setMake(const std::string &make)
> +{
> +	return setString(EXIF_IFD_0, EXIF_TAG_MAKE, EXIF_FORMAT_ASCII, make);
> +}
> +
> +int Exif::setModel(const std::string &model)
> +{
> +	return setString(EXIF_IFD_0, EXIF_TAG_MODEL, EXIF_FORMAT_ASCII, model);
> +}
> +
> +int Exif::setSize(Size size)

const Size &size ?

> +{
> +	setShort(EXIF_IFD_0, EXIF_TAG_IMAGE_LENGTH, size.height);

The EXIF specification says that EXIF_TAG_IMAGE_LENGTH and
EXIF_TAG_IMAGE_WIDTH should not be set for JPEG compressed images.
X_DIMENSION and Y_DIMENSION are fine.

> +	setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_Y_DIMENSION, size.height);
> +
> +	setShort(EXIF_IFD_0, EXIF_TAG_IMAGE_WIDTH, size.width);
> +	setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_X_DIMENSION, size.width);
> +
> +	return 0;
> +}
> +
> +int Exif::setTimestamp(const time_t timestamp)
> +{
> +	char str[20];
> +	std::strftime(str, sizeof(str), "%Y:%m:%d %H:%M:%S",
> +		      std::localtime(&timestamp));
> +	std::string ts(str);
> +	int ret = -1;
> +
> +	ret = setString(EXIF_IFD_0, EXIF_TAG_DATE_TIME, EXIF_FORMAT_ASCII, ts);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = setString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_ORIGINAL, EXIF_FORMAT_ASCII, ts);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = setString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_DIGITIZED, EXIF_FORMAT_ASCII, ts);
> +	if (ret < 0)
> +		return ret;

This makes me think that skipping error handling and relying on valid_
would be a good idea :-)

Doesn't have to be part of this series, but adding the timezone would be
great (EXIF_TAG_TIME_ZONE_OFFSET).

> +
> +	return ret;
> +}
> +
> +int Exif::setOrientation(int orientation)
> +{
> +	int value = 1;
> +	switch (orientation) {
> +	case 90:
> +		value = 6;
> +		break;
> +	case 180:
> +		value = 3;
> +		break;
> +	case 270:
> +		value = 8;
> +		break;
> +	}
> +
> +	return setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value);
> +}
> +
>  int Exif::generate()
>  {
>  	if (exifData_) {
> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h
> index 6c10f94..f15afb9 100644
> --- a/src/android/jpeg/exif.h
> +++ b/src/android/jpeg/exif.h
> @@ -9,8 +9,10 @@
>  
>  #include <libexif/exif-data.h>
>  
> +#include <libcamera/geometry.h>
>  #include <libcamera/span.h>
>  
> +#include <ctime>
>  #include <string>

I missed this on 1/2, but we usually put the C/C++ headers first.

>  
>  class Exif
> @@ -19,6 +21,13 @@ public:
>  	Exif();
>  	~Exif();
>  
> +	int setMake(const std::string &make);
> +	int setModel(const std::string &model);
> +
> +	int setOrientation(int orientation);
> +	int setSize(libcamera::Size size);
> +	int setTimestamp(const time_t timestamp);

No need for const when passing by value :-)

> +
>  	libcamera::Span<const uint8_t> data() const { return { exifData_, size_ }; }
>  	int generate();
>

Patch

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index de6f86f..54ca9c6 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -24,6 +24,7 @@ 
 #include "system/graphics.h"
 
 #include "jpeg/encoder_libjpeg.h"
+#include "jpeg/exif.h"
 
 using namespace libcamera;
 
@@ -1434,7 +1435,22 @@  void CameraDevice::requestComplete(Request *request)
 			continue;
 		}
 
-		int jpeg_size = encoder->encode(buffer, mapped.maps()[0]);
+		/* Set EXIF metadata for various tags. */
+		Exif exif;
+		/* \todo Set Make and Model from external vendor tags. */
+		exif.setMake("libcamera");
+		exif.setModel("cameraModel");
+		exif.setOrientation(orientation_);
+		exif.setSize(cameraStream->size);
+		/*
+		 * We set the frame's EXIF timestamp as the time of encode.
+		 * Since the precision we need for EXIF timestamp is only one
+		 * second, it is good enough.
+		 */
+		exif.setTimestamp(std::time(nullptr));
+		exif.generate();
+
+		int jpeg_size = encoder->encode(buffer, mapped.maps()[0], exif.data());
 		if (jpeg_size < 0) {
 			LOG(HAL, Error) << "Failed to encode stream image";
 			status = CAMERA3_BUFFER_STATUS_ERROR;
diff --git a/src/android/jpeg/encoder.h b/src/android/jpeg/encoder.h
index f9eb88e..cf26d67 100644
--- a/src/android/jpeg/encoder.h
+++ b/src/android/jpeg/encoder.h
@@ -18,7 +18,8 @@  public:
 
 	virtual int configure(const libcamera::StreamConfiguration &cfg) = 0;
 	virtual int encode(const libcamera::FrameBuffer *source,
-			   const libcamera::Span<uint8_t> &destination) = 0;
+			   const libcamera::Span<uint8_t> &destination,
+			   const libcamera::Span<const uint8_t> &exifData) = 0;
 };
 
 #endif /* __ANDROID_JPEG_ENCODER_H__ */
diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp
index 977538c..510613c 100644
--- a/src/android/jpeg/encoder_libjpeg.cpp
+++ b/src/android/jpeg/encoder_libjpeg.cpp
@@ -180,7 +180,8 @@  void EncoderLibJpeg::compressNV(const libcamera::MappedBuffer *frame)
 }
 
 int EncoderLibJpeg::encode(const FrameBuffer *source,
-			   const libcamera::Span<uint8_t> &dest)
+			   const libcamera::Span<uint8_t> &dest,
+			   const libcamera::Span<const uint8_t> &exifData)
 {
 	MappedFrameBuffer frame(source, PROT_READ);
 	if (!frame.isValid()) {
@@ -204,6 +205,12 @@  int EncoderLibJpeg::encode(const FrameBuffer *source,
 
 	jpeg_start_compress(&compress_, TRUE);
 
+	if (exifData.size())
+		/* Store Exif data in the JPEG_APP1 data block. */
+		jpeg_write_marker(&compress_, JPEG_APP0 + 1,
+				  static_cast<const JOCTET *>(exifData.data()),
+				  exifData.size());
+
 	LOG(JPEG, Debug) << "JPEG Encode Starting:" << compress_.image_width
 			 << "x" << compress_.image_height;
 
diff --git a/src/android/jpeg/encoder_libjpeg.h b/src/android/jpeg/encoder_libjpeg.h
index aed081a..1e8df05 100644
--- a/src/android/jpeg/encoder_libjpeg.h
+++ b/src/android/jpeg/encoder_libjpeg.h
@@ -22,7 +22,8 @@  public:
 
 	int configure(const libcamera::StreamConfiguration &cfg) override;
 	int encode(const libcamera::FrameBuffer *source,
-		   const libcamera::Span<uint8_t> &destination) override;
+		   const libcamera::Span<uint8_t> &destination,
+		   const libcamera::Span<const uint8_t> &exifData) override;
 
 private:
 	void compressRGB(const libcamera::MappedBuffer *frame);
diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
index b49b538..3fbb117 100644
--- a/src/android/jpeg/exif.cpp
+++ b/src/android/jpeg/exif.cpp
@@ -157,6 +157,68 @@  int Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::stri
 	return 0;
 }
 
+int Exif::setMake(const std::string &make)
+{
+	return setString(EXIF_IFD_0, EXIF_TAG_MAKE, EXIF_FORMAT_ASCII, make);
+}
+
+int Exif::setModel(const std::string &model)
+{
+	return setString(EXIF_IFD_0, EXIF_TAG_MODEL, EXIF_FORMAT_ASCII, model);
+}
+
+int Exif::setSize(Size size)
+{
+	setShort(EXIF_IFD_0, EXIF_TAG_IMAGE_LENGTH, size.height);
+	setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_Y_DIMENSION, size.height);
+
+	setShort(EXIF_IFD_0, EXIF_TAG_IMAGE_WIDTH, size.width);
+	setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_X_DIMENSION, size.width);
+
+	return 0;
+}
+
+int Exif::setTimestamp(const time_t timestamp)
+{
+	char str[20];
+	std::strftime(str, sizeof(str), "%Y:%m:%d %H:%M:%S",
+		      std::localtime(&timestamp));
+	std::string ts(str);
+	int ret = -1;
+
+	ret = setString(EXIF_IFD_0, EXIF_TAG_DATE_TIME, EXIF_FORMAT_ASCII, ts);
+	if (ret < 0)
+		return ret;
+
+	ret = setString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_ORIGINAL, EXIF_FORMAT_ASCII, ts);
+	if (ret < 0)
+		return ret;
+
+	ret = setString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_DIGITIZED, EXIF_FORMAT_ASCII, ts);
+	if (ret < 0)
+		return ret;
+
+	return ret;
+}
+
+int Exif::setOrientation(int orientation)
+{
+	int value = 1;
+	switch (orientation) {
+	case 90:
+		value = 6;
+		break;
+	case 180:
+		value = 3;
+		break;
+	case 270:
+		value = 8;
+		break;
+	}
+
+	return setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value);
+}
+
 int Exif::generate()
 {
 	if (exifData_) {
diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h
index 6c10f94..f15afb9 100644
--- a/src/android/jpeg/exif.h
+++ b/src/android/jpeg/exif.h
@@ -9,8 +9,10 @@ 
 
 #include <libexif/exif-data.h>
 
+#include <libcamera/geometry.h>
 #include <libcamera/span.h>
 
+#include <ctime>
 #include <string>
 
 class Exif
@@ -19,6 +21,13 @@  public:
 	Exif();
 	~Exif();
 
+	int setMake(const std::string &make);
+	int setModel(const std::string &model);
+
+	int setOrientation(int orientation);
+	int setSize(libcamera::Size size);
+	int setTimestamp(const time_t timestamp);
+
 	libcamera::Span<const uint8_t> data() const { return { exifData_, size_ }; }
 	int generate();