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

Laurent Pinchart laurent.pinchart at ideasonboard.com
Fri Sep 4 00:40:37 CEST 2020 

On Thu, Sep 03, 2020 at 07:38:07PM +0100, Kieran Bingham wrote:
> Hi Umang,
> 
> On 03/09/2020 17:32, 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 at ideasonboard.com>
> > Signed-off-by: Umang Jain <email at uajain.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham at 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            | 48 +++++++++++++++++++++++++++-
> >  src/android/jpeg/exif.h              |  8 +++++
> >  6 files changed, 84 insertions(+), 5 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();
> 
> Hrm ... Did this pass compile tests?

Likely not, given that generate() is defined as returning int in the
header file and void in the implementation. Rebase issue, or sending the
wrong version ?

> Didn't we add a [[nodiscard]] to this call?
> 
> Although - if it does fail to generate, I think we'd still expect to
> call into the encoder->encode() and I think the exif.data() would be
> nullptr,0, (which is what we want), as the encoder checks the size to
> determine if it should be added or not.
> 
> > +
> > +		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 9d8d9bc..5def7e3 100644
> > --- a/src/android/jpeg/exif.cpp
> > +++ b/src/android/jpeg/exif.cpp
> > @@ -169,7 +169,53 @@ void Exif::setString(ExifIfd ifd, ExifTag tag, ExifFormat format, const std::str
> >  	exif_entry_unref(entry);
> >  }
> >  
> > -int Exif::generate()
> > +void Exif::setMake(const std::string &make)
> > +{
> > +	setString(EXIF_IFD_0, EXIF_TAG_MAKE, EXIF_FORMAT_ASCII, make);
> > +}
> > +
> > +void Exif::setModel(const std::string &model)
> > +{
> > +	setString(EXIF_IFD_0, EXIF_TAG_MODEL, EXIF_FORMAT_ASCII, model);
> > +}
> > +
> > +void Exif::setSize(const Size &size)
> > +{
> > +	setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_Y_DIMENSION, size.height);
> > +	setLong(EXIF_IFD_EXIF, EXIF_TAG_PIXEL_X_DIMENSION, size.width);
> > +}
> > +
> > +void Exif::setTimestamp(time_t timestamp)
> > +{
> > +	char str[20];
> > +	std::strftime(str, sizeof(str), "%Y:%m:%d %H:%M:%S",
> > +		      std::localtime(&timestamp));
> > +	std::string ts(str);
> > +
> > +	setString(EXIF_IFD_0, EXIF_TAG_DATE_TIME, EXIF_FORMAT_ASCII, ts);
> > +	setString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_ORIGINAL, EXIF_FORMAT_ASCII, ts);
> > +	setString(EXIF_IFD_EXIF, EXIF_TAG_DATE_TIME_DIGITIZED, EXIF_FORMAT_ASCII, ts);
> > +}
> > +
> > +void Exif::setOrientation(int orientation)
> > +{
> > +	int value = 1;
> > +	switch (orientation) {
> > +	case 90:
> > +		value = 6;
> > +		break;
> > +	case 180:
> > +		value = 3;
> > +		break;
> > +	case 270:
> > +		value = 8;
> > +		break;
> > +	}
> > +
> > +	setShort(EXIF_IFD_0, EXIF_TAG_ORIENTATION, value);
> > +}
> > +
> > +void Exif::generate()
> >  {
> >  	if (exifData_) {
> >  		free(exifData_);
> > diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h
> > index 8fb8ffd..6113ca6 100644
> > --- a/src/android/jpeg/exif.h
> > +++ b/src/android/jpeg/exif.h
> > @@ -12,6 +12,7 @@
> >  
> >  #include <libexif/exif-data.h>
> >  
> > +#include <libcamera/geometry.h>
> >  #include <libcamera/span.h>
> >  
> >  class Exif
> > @@ -20,6 +21,13 @@ public:
> >  	Exif();
> >  	~Exif();
> >  
> > +	void setMake(const std::string &make);
> > +	void setModel(const std::string &model);
> > +
> > +	void setOrientation(int orientation);
> > +	void setSize(const libcamera::Size &size);
> > +	void setTimestamp(time_t timestamp);
> > +
> >  	libcamera::Span<const uint8_t> data() const { return { exifData_, size_ }; }
> >  	int [[nodiscard]]  generate();
> >  

-- 
Regards,

Laurent Pinchart

More information about the libcamera-devel mailing list