[libcamera-devel] [PATCH 3/3] android: Support initial set of EXIF metadata tags

[Kieran Bingham]

Hi Umang,

On 24/08/2020 21:46, Umang Jain wrote:
> Support a initial set of  EXIF metadata tags namely:
> Make, Model, Height, Width, Orientation and Timestamp.
> Each tag is set by a convenient high level helper API
> defined in exif.h.
> 
> Signed-off-by: Umang Jain <email at uajain.com>
> ---
>  src/android/camera_device.cpp        |  8 +++++++
>  src/android/jpeg/encoder_libjpeg.cpp | 12 ++++++++++
>  src/android/jpeg/exif.cpp            | 34 ++++++++++++++++++++++++++++
>  src/android/jpeg/exif.h              |  8 +++++++

I'd merge the helpers directly into the Exif object creation patch (and
take authorship on that if you like), or as an additional patch on it's
own after.


>  4 files changed, 62 insertions(+)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index bc5690e..fcf378a 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1435,6 +1435,14 @@ void CameraDevice::requestComplete(Request *request)
>  		}
>  
>  		Exif exif;
> +		/*
> +		 * \todo Discuss setMake String, maybe vendor ID?
> +		 * KB suggested to leave it to "libcamera" for now.
> +		 * setModel should use the 'model' property of the camera.
> +		 */

I think ultimately this will indeed need to come from some sort of
vendor tag. But I don't think we have those yet, so libcamera should be
fine for now ;-)

> +		exif.setMake("Libcamera");

libcamera always has a lowercase 'l'.

> +		exif.setModel("cameraModel");

Aha, are the patches for setting the model not integrated yet ?

That would indeed mean it's a todo for now, but hopefully that will be
usable really soon, so I hope that can be done before these patches get in.


> +		exif.setOrientation(orientation_);
>  
>  		int jpeg_size = encoder->encode(buffer, mapped.maps()[0], &exif);
>  		if (jpeg_size < 0) {
> diff --git a/src/android/jpeg/encoder_libjpeg.cpp b/src/android/jpeg/encoder_libjpeg.cpp
> index 0cd93b6..f84def9 100644
> --- a/src/android/jpeg/encoder_libjpeg.cpp
> +++ b/src/android/jpeg/encoder_libjpeg.cpp
> @@ -213,7 +213,19 @@ int EncoderLibJpeg::encode(const FrameBuffer *source,
>  	else
>  		compressRGB(&frame);
>  
> +	exif->setWidth(compress_.image_width);
> +	exif->setHeight(compress_.image_height);
> +	exif->setTimestamp(source->metadata().timestamp);

I hope all three of those could be obtained/determined in the layer
above, so we don't need to pass in the whole exif object, just the
generated span,

> +
> +	Span<uint8_t> exif_data = exif->generate();
> +

So if you can do that in the camera_device.cpp, then we

>  	jpeg_finish_compress(&compress_);
>  
> +	if (exif->size())
> +		/* Store Exif data in the JPEG_APP1 data block. */
> +		jpeg_write_marker(&compress_, JPEG_APP0 + 1,
> +				  static_cast<const JOCTET *>(exif_data.data()),
> +				  exif_data.size());
> +
>  	return size;
>  }
> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
> index f6a9f5c..b7591d5 100644
> --- a/src/android/jpeg/exif.cpp
> +++ b/src/android/jpeg/exif.cpp
> @@ -160,6 +160,40 @@ int Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::stri
>  	return 0;
>  }
>  
> +int Exif::setHeight(uint16_t height)
> +{
> +	setShort(EXIF_IFD_0, EXIF_TAG_IMAGE_LENGTH, height);
> +	setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_Y_DIMENSION, height);
> +
> +	return 0;
> +}
> +
> +int Exif::setWidth(uint16_t width)
> +{
> +	setShort(EXIF_IFD_0, EXIF_TAG_IMAGE_WIDTH, width);
> +	setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_X_DIMENSION, width);
> +
> +	return 0;
> +}
> +
> +int Exif::setTimestamp(const time_t timestamp)
> +{
> +	std::string ts(std::ctime(&timestamp));
> +	int 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;
> +}
> +
>  Span<uint8_t> Exif::generate()
>  {
>  	if (exif_data_) {
> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h
> index 7df83c7..6a8f5f5 100644
> --- a/src/android/jpeg/exif.h
> +++ b/src/android/jpeg/exif.h
> @@ -11,6 +11,7 @@
>  
>  #include <libcamera/span.h>
>  
> +#include <ctime>
>  #include <string>
>  
>  class Exif
> @@ -24,6 +25,13 @@ public:
>  	int setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::string &item);
>  	int setRational(ExifIfd ifd, ExifTag tag, uint32_t numerator, uint32_t denominator);
>  
> +	int setHeight(uint16_t height);
> +	int setMake(const std::string &make) { return setString(EXIF_IFD_0, EXIF_TAG_MAKE, EXIF_FORMAT_ASCII, make); }
> +	int setModel(const std::string &model) { return setString(EXIF_IFD_0, EXIF_TAG_MODEL, EXIF_FORMAT_ASCII, model); }
> +	int setOrientation(int orientation) { return setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, orientation); }

Even if they're short, I think these implementations would probably be
better in the .cpp so that all helpers are in the same place.

Orientation will potentially be more complex too, as I think there will
need to be a translation between the libcamera orientation value, and
the Exif orientation values.

I remember seeing some helpful comments in the USB HAL in CrOS/Camera2
for that.


> +	int setTimestamp(const time_t timestamp);
> +	int setWidth(uint16_t width);

I'd group by feature over sorting alphabetically, and keep setWidth,
setHeight together.

--
Kieran


> +
>  	libcamera::Span<uint8_t> generate();
>  	unsigned char *data() const { return exif_data_; }
>  	unsigned int size() const { return size_; }
> 

-- 
Regards
--
Kieran