[libcamera-devel] [PATCH 1/4] android: jpeg: exif: Fix and expand setRational

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Mon, Mar 08, 2021 at 07:13:53PM +0900, Paul Elder wrote:
> setRational was not working properly for EXIF tags in the GPS IFD.
> Manually specify the size of the EXIF entry to fix this. While at it,
> add support for setting multiple rationals, as that is a common use case
> for rational EXIF tags.
> 
> This allows the GPS altitude to be set properly, and is part of the fix
> to allow the following CTS test to pass:
> - android.hardware.cts.CameraTest#testJpegExif
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
>  src/android/jpeg/exif.cpp | 13 +++++++++++--
>  src/android/jpeg/exif.h   |  2 ++
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
> index fdd46070..e5a3e448 100644
> --- a/src/android/jpeg/exif.cpp
> +++ b/src/android/jpeg/exif.cpp
> @@ -187,11 +187,20 @@ void Exif::setLong(ExifIfd ifd, ExifTag tag, uint32_t item)
>  
>  void Exif::setRational(ExifIfd ifd, ExifTag tag, ExifRational item)
>  {
> -	ExifEntry *entry = createEntry(ifd, tag);
> +	ExifRational items[] = { item };
> +	setRational(ifd, tag, 1, items);
> +}
> +
> +void Exif::setRational(ExifIfd ifd, ExifTag tag, uint32_t count, ExifRational items[])

Our way to pass pointer and number of elements is called Span :-)

void Exif::setRational(ExifIfd ifd, ExifTag tag, Span<const ExifRational> items)

You can then replace count with items.size() below. items[i] should work
unchanged. And above, that would be

void Exif::setRational(ExifIfd ifd, ExifTag tag, ExifRational item)
{
	setRational(ifd, tag, { &items, 1 });
}

With this, and Kieran's comments addressed,

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +{
> +	ExifEntry *entry = createEntry(ifd, tag, EXIF_FORMAT_RATIONAL,
> +				       count, count * sizeof(ExifRational));
>  	if (!entry)
>  		return;
>  
> -	exif_set_rational(entry->data, order_, item);
> +	for (size_t i = 0; i < count; i++)
> +		exif_set_rational(entry->data + i * sizeof(ExifRational),
> +				  order_, items[i]);
>  	exif_entry_unref(entry);
>  }
>  
> diff --git a/src/android/jpeg/exif.h b/src/android/jpeg/exif.h
> index b0d61402..e283d3fc 100644
> --- a/src/android/jpeg/exif.h
> +++ b/src/android/jpeg/exif.h
> @@ -89,6 +89,8 @@ private:
>  		       const std::string &item,
>  		       StringEncoding encoding = NoEncoding);
>  	void setRational(ExifIfd ifd, ExifTag tag, ExifRational item);
> +	void setRational(ExifIfd ifd, ExifTag tag, uint32_t count,
> +			 ExifRational items[]);
>  
>  	std::tuple<int, int, int> degreesToDMS(double decimalDegrees);
>  	void setGPSDMS(ExifIfd ifd, ExifTag tag, int deg, int min, int sec);

-- 
Regards,

Laurent Pinchart