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

11532 diff mbox series

setRational was not working properly for EXIF tags in the GPS IFD due to
libexif not supporting those tags in exif_entry_initialize().  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.

As Rational types are no longer initialized by libexif directly, the
EXIF_TAG_{X,Y}_RESOLUTION exif tags will not have their default values
populated.

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@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

---
Changes in v2:
- use span instead of size + array
- expand commit message
---
 src/android/jpeg/exif.cpp | 15 +++++++++++++--
 src/android/jpeg/exif.h   |  2 ++
 2 files changed, 15 insertions(+), 2 deletions(-)

On Tue, Mar 09, 2021 at 05:52:32PM +0900, Paul Elder wrote:
> setRational was not working properly for EXIF tags in the GPS IFD due to
> libexif not supporting those tags in exif_entry_initialize().  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.
>
> As Rational types are no longer initialized by libexif directly, the
> EXIF_TAG_{X,Y}_RESOLUTION exif tags will not have their default values
> populated.
>
> 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@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> ---
> Changes in v2:
> - use span instead of size + array
> - expand commit message
> ---
>  src/android/jpeg/exif.cpp | 15 +++++++++++++--
>  src/android/jpeg/exif.h   |  2 ++
>  2 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp
> index fdd46070..929ac542 100644
> --- a/src/android/jpeg/exif.cpp
> +++ b/src/android/jpeg/exif.cpp
> @@ -14,6 +14,8 @@
>  #include <tuple>
>  #include <uchar.h>
>
> +#include <libcamera/span.h>
> +

span.h is already included in exif.h so you can drop it here I guess

>  #include "libcamera/internal/log.h"
>  #include "libcamera/internal/utils.h"
>
> @@ -187,11 +189,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);
> +	setRational(ifd, tag, Span<const ExifRational>{ &item, 1 });

and you can write this as
	setRational(ifd, tag, { &item, 1 });

according to my compiler :)

The rest looks good
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

> +}
> +
> +void Exif::setRational(ExifIfd ifd, ExifTag tag, Span<const ExifRational> items)
> +{
> +	ExifEntry *entry = createEntry(ifd, tag, EXIF_FORMAT_RATIONAL,
> +				       items.size(),
> +				       items.size() * sizeof(ExifRational));
>  	if (!entry)
>  		return;
>
> -	exif_set_rational(entry->data, order_, item);
> +	for (size_t i = 0; i < items.size(); 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..8aa1b123 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,
> +			 libcamera::Span<const ExifRational> items);
>
>  	std::tuple<int, int, int> degreesToDMS(double decimalDegrees);
>  	void setGPSDMS(ExifIfd ifd, ExifTag tag, int deg, int min, int sec);
> --
> 2.27.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel