[libcamera-devel] [RFC PATCH] android: jpeg: exif: Set timezone information

[Laurent Pinchart]

Hi Umang,

On Thu, Sep 10, 2020 at 07:25:47AM +0300, Laurent Pinchart wrote:
> Hi Umang,
> 
> Thank you for the patch.
> 
> On Wed, Sep 09, 2020 at 09:02:06PM +0530, Umang Jain wrote:
> > Get timestamp information from the timestamp, although the resolution
> 
> s/timestamp information/timezone information/
> 
> > for EXIF_TAG_TIME_ZONE_OFFSET is fairly limtied (per-hour only).
> 
> s/limtied/limited/
> 
> > Experimentation with 'exiftool', commandline utility to read/write
> > exif metadata on images, resulted in rounding off the hours if the
> > minutes came out to >= 30. Hence, the behaviour is inspired from
> > exiftool itself. For e.g.,
> > 
> > Timezone      Tag's value
> >  +1015     =>     10
> >  +0945     =>     10
> >  -1145     =>    -12
> 
> Sounds good to me.
> 
> > Signed-off-by: Umang Jain <email at uajain.com>
> > ---
> >  src/android/jpeg/exif.cpp | 30 ++++++++++++++++++++++++++++++
> >  src/android/jpeg/exif.h   |  1 +
> >  2 files changed, 31 insertions(+)
> > 
> > diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
> > index 031f5f4..d8b4537 100644
> > --- a/src/android/jpeg/exif.cpp
> > +++ b/src/android/jpeg/exif.cpp
> > @@ -135,6 +135,16 @@ void Exif::setShort(ExifIfd ifd, ExifTag tag, uint16_t item)
> >  	exif_entry_unref(entry);
> >  }
> >  
> > +void Exif::setSShort(ExifIfd ifd, ExifTag tag, int16_t item)
> > +{
> > +	ExifEntry *entry = createEntry(ifd, tag);
> > +	if (!entry)
> > +		return;
> > +
> > +	exif_set_sshort(entry->data, EXIF_BYTE_ORDER_INTEL, item);
> > +	exif_entry_unref(entry);
> > +}
> > +
> >  void Exif::setLong(ExifIfd ifd, ExifTag tag, uint32_t item)
> >  {
> >  	ExifEntry *entry = createEntry(ifd, tag);
> > @@ -194,6 +204,26 @@ void Exif::setTimestamp(time_t timestamp)
> >  	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);
> > +
> > +	/* If possible, query and set timezone information via
> 
> 	/*
> 	 * If possible ...
> 
> > +	 * EXIF_TAG_TIME_ZONE_OFFSET. There is only per-hour resolution for tag
> > +	 * hence, round up hours if minutes >= 30.
> > +	 */
> > +	char tz[6];
> 
> How about reusing the str variable ?
> 
> > +	int r = std::strftime(tz, sizeof(tz), "%z", std::localtime(&timestamp));
> 
> I've posted "[PATCH] android: jpeg: exif: Use reentrant localtime_r()"
> which adds a local tm variable that you can reuse instead of calling
> std::localtime() again.

I've now pushed that patch to master. Could you rebase this one ?

> > +	if (r > 0) {
> > +		int16_t timezone = atoi(tz);
> 
> You should include stdlib.h for atoi.
> 
> > +		int16_t hour = timezone / 100;
> > +		int16_t min = abs(timezone) - (abs(hour) * 100);
> > +
> > +		if (tz[0] == '-' && min >= 30)
> > +			hour--;
> > +		else if (min >= 30)
> > +			hour++;
> 
> An alternative would be
> 
> 		int16_t timezone = atoi(tz);
> 		int16_t hour = timezone / 100;
> 		int16_t min = timezone % 100;
> 
> 		if (min <= -30)
> 			hour--;
> 		else if (min >= 30)
> 			hour++;
> 
> This relies on the fact that the modulo of a negative number will be a
> negative number.
> 
> > +
> > +		/* \todo Check if EXIF IFD is correct here or not. */
> 
> It's messy. The TIFF/EP specification defines the TimeZoneOffset tag in
> IFD0, so that's where I think it should go.

I've also checked how libexif and libexiv2 handle reading this tag
(through the exif and exiv2 command line utilities respectively).
libexif seems to parse it fine regardless of whether it's stored in IFD0
or in int EXIF IFD, while libexiv2 seem to only handle it when stored in
IFD0 (the tag isn't printed when stored in the EXIF IFD). IFD0 thus
seems to be the right pick.

> The EXIF specification
> defines three other tags (OffsetTime, OffsetTimeOriginal,
> OffsetTimeDigitized), in the EXIF IFD, but those are not supported by
> libexif. Should we mention the unsupported tags in the commit message ?
> 
> Note that the DateTimeOriginal tag is defined by both the TIFF/EP and
> EXIF specifications, but in different IFDs (IFD 0 for TIFF/EP and EXIF
> IFD for EXIF). Lovely...
> 
> > +		setSShort(EXIF_IFD_EXIF, EXIF_TAG_TIME_ZONE_OFFSET, hour);
> 
> The TIFF/EP specification defined TimeZoneOffset as containing up to two
> signed short values. The first value relates to DateTime, the second
> value to DateTimeOriginal. As we don't set DateTimeOriginal in IFD0 I
> would leave the second value out, but if we added DateTimeOriginal in
> IFD0 in addition to the EXIF IFD, I would add the second value.
> 
> > +	}
> >  }
> >  
> >  void Exif::setOrientation(int orientation)
> > diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h
> > index 622de4c..4817815 100644
> > --- a/src/android/jpeg/exif.h
> > +++ b/src/android/jpeg/exif.h
> > @@ -37,6 +37,7 @@ private:
> >  			       unsigned long components, unsigned int size);
> >  
> >  	void setShort(ExifIfd ifd, ExifTag tag, uint16_t item);
> > +	void setSShort(ExifIfd ifd, ExifTag tag, int16_t item);
> >  	void setLong(ExifIfd ifd, ExifTag tag, uint32_t item);
> >  	void setString(ExifIfd ifd, ExifTag tag, ExifFormat format,
> >  		       const std::string &item);

-- 
Regards,

Laurent Pinchart