[libcamera-devel] [PATCH 2/4] android: jpeg: exif: Simplify setGPSDateTimestamp and setGPSDMS

[Kieran Bingham]

On 08/03/2021 14:38, Laurent Pinchart wrote:
> Hi Paul,
> 
> Thank you for the patch.
> 
> On Mon, Mar 08, 2021 at 10:46:42AM +0000, Kieran Bingham wrote:
>> On 08/03/2021 10:13, Paul Elder wrote:
>>> Now that setRational() supports setting multiple rational values, use
>>> that in setGPSDateTimestamp and setGPSDMS which previously set every
>>> rational manually.
>>
>> This feels better to me ;-)
>>
>>> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
>>> ---
>>>  src/android/jpeg/exif.cpp | 24 ++----------------------
>>>  1 file changed, 2 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
>>> index e5a3e448..c8b2f072 100644
>>> --- a/src/android/jpeg/exif.cpp
>>> +++ b/src/android/jpeg/exif.cpp
>>> @@ -345,24 +345,13 @@ void Exif::setGPSDateTimestamp(time_t timestamp)
>>>  		  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);
>>> +	setRational(EXIF_IFD_GPS, static_cast<ExifTag>(EXIF_TAG_GPS_TIME_STAMP), 3, ts);
>>
>> We're now encoding the size of these tags (EXIF_TAG_GPS_TIME_STAMP) in
>> our layer, where for most other tags are defined by libexif.
>>
>> Given that libexif doesn't support those tags correctly it would seem,
>> this is fine - but perhaps we should send a patch to libexif as well.
> 
> I've been tempted to send patches to libexif before...
> 
>> Even if we patch libexif, we will still have to do this ourselves so no
>> blocker.
> 
> ... and this is what stopped me :-) Coupled with the fact that the
> libexif API definitely shows its age, and I would have been too tempted
> of rewriting parts of it :-D The project doesn't seem very active, but
> it could still be worth a try.
> 
>> Also rather than hardcoding '3' should we use std::size(coords) ?
> 
> I assume you mean ts here. If setRational() switched to Span, then this
> would be automatic :-)

Argh, that's because I spotted this below, moved the text up to keep the
flow - and forgot to update the reference.

Span sounds like a good idea to me though ;-)



> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
>> 5cfbbcd20731 ("libcamera: Replace ARRAY_SIZE() with std::size()")
>> suggests it should be possible.
>>
>>>  }
>>>  
>>>  std::tuple<int, int, int> Exif::degreesToDMS(double decimalDegrees)
>>> @@ -376,22 +365,13 @@ std::tuple<int, int, int> Exif::degreesToDMS(double decimalDegrees)
>>>  
>>>  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);
>>> +	setRational(ifd, tag, 3, coords);
>>
>> And here with std::size()
>>
>> With that
>>
>> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>
>>>  }
>>>  
>>>  /*
> 

-- 
Regards
--
Kieran