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

[Kieran Bingham]

On 08/03/2021 10:13, 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.

setRational itself was working fine, but it is the lack of support for
the tags used within

 libexif::exif-entry.c::exif_entry_initialize(ExifEntry *e, ExifTag tag)

which is the issue here.

exif_entry_initialize() goes to great lengths to initialise tags for
their size and even defaults. Removing that (by using our direct
allocation with the 4 argument version of createEntry() means that we
are no longer handling those intialisations for Rational types.

That will affect at least the following, which seem to be the only
rational types initialised (yes, I see one of those groups is SRATIONAL
so we don't handle those correctly yet either).

> 
> 	/* SRATIONAL, 1 component, no default */
> 	case EXIF_TAG_EXPOSURE_BIAS_VALUE:
> 	case EXIF_TAG_BRIGHTNESS_VALUE:
> 	case EXIF_TAG_SHUTTER_SPEED_VALUE:
> 		e->components = 1;
> 		e->format = EXIF_FORMAT_SRATIONAL;
> 		e->size = exif_format_get_size (e->format) * e->components;
> 		e->data = exif_entry_alloc (e, e->size);
> 		if (!e->data) { clear_entry(e); break; }
> 		break;
> 
> 	/* RATIONAL, 1 component, no default */
> 	case EXIF_TAG_EXPOSURE_TIME:
> 	case EXIF_TAG_FOCAL_PLANE_X_RESOLUTION:
> 	case EXIF_TAG_FOCAL_PLANE_Y_RESOLUTION:
> 	case EXIF_TAG_EXPOSURE_INDEX:
> 	case EXIF_TAG_FLASH_ENERGY:
> 	case EXIF_TAG_FNUMBER:
> 	case EXIF_TAG_FOCAL_LENGTH:
> 	case EXIF_TAG_SUBJECT_DISTANCE:
> 	case EXIF_TAG_MAX_APERTURE_VALUE:
> 	case EXIF_TAG_APERTURE_VALUE:
> 	case EXIF_TAG_COMPRESSED_BITS_PER_PIXEL:
> 	case EXIF_TAG_PRIMARY_CHROMATICITIES:
> 	case EXIF_TAG_DIGITAL_ZOOM_RATIO:
> 		e->components = 1;
> 		e->format = EXIF_FORMAT_RATIONAL;
> 		e->size = exif_format_get_size (e->format) * e->components;
> 		e->data = exif_entry_alloc (e, e->size);
> 		if (!e->data) { clear_entry(e); break; }
> 		break;
> 
> 	/* RATIONAL, 1 component, default 72/1 */
> 	case EXIF_TAG_X_RESOLUTION:
> 	case EXIF_TAG_Y_RESOLUTION:
> 		e->components = 1;
> 		e->format = EXIF_FORMAT_RATIONAL;
> 		e->size = exif_format_get_size (e->format) * e->components;
> 		e->data = exif_entry_alloc (e, e->size);
> 		if (!e->data) { clear_entry(e); break; }
> 		r.numerator = 72;
> 		r.denominator = 1;
> 		exif_set_rational (e->data, o, r);
> 		break;
> 
> 	/* RATIONAL, 2 components, no default */
> 	case EXIF_TAG_WHITE_POINT:
> 		e->components = 2;
> 		e->format = EXIF_FORMAT_RATIONAL;
> 		e->size = exif_format_get_size (e->format) * e->components;
> 		e->data = exif_entry_alloc (e, e->size);
> 		if (!e->data) { clear_entry(e); break; }
> 		break;
> 
> 	/* RATIONAL, 6 components */
> 	case EXIF_TAG_REFERENCE_BLACK_WHITE:
> 		e->components = 6;
> 		e->format = EXIF_FORMAT_RATIONAL;
> 		e->size = exif_format_get_size (e->format) * e->components;
> 		e->data = exif_entry_alloc (e, e->size);
> 		if (!e->data) { clear_entry(e); break; }
> 		r.denominator = 1;
> 		r.numerator = 0;
> 		exif_set_rational (e->data, o, r);
> 		r.numerator = 255;
> 		exif_set_rational (
> 			e->data + exif_format_get_size (e->format), o, r);
> 		r.numerator = 0;
> 		exif_set_rational (
> 			e->data + 2 * exif_format_get_size (e->format), o, r);
> 		r.numerator = 255;
> 		exif_set_rational (
> 			e->data + 3 * exif_format_get_size (e->format), o, r);
> 		r.numerator = 0;
> 		exif_set_rational (
> 			e->data + 4 * exif_format_get_size (e->format), o, r);
> 		r.numerator = 255;
> 		exif_set_rational (
> 			e->data + 5 * exif_format_get_size (e->format), o, r);
> 		break;


As the reality is that most of them are not initialised with a default
value, I don't think that's too much of a problem, but we should be
aware of it.


Given that this leads us to better support for Rational generically, I
think this is a good way forwards, but could you add a note in the
commit message to say that Rational types are no longer initialised by
the exif library directly (as that has the consequence of not populating
default values for EXIF_TAG_{X,Y}_RESOLUTION.

We won't use those defaults, so it doesn't matter - but we're changing
the behaviour - so we should note that.




> 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[])
> +{
> +	ExifEntry *entry = createEntry(ifd, tag, EXIF_FORMAT_RATIONAL,
> +				       count, count * sizeof(ExifRational));


The library internally uses:
   exif_format_get_size (EXIF_FORMAT_RATIONAL);

That involves a lookup in a table, which I think we can avoid here, by
using sizeof(ExifRational) directly, but could you make sure they are
the same expected size (8?) and that nothing crazy is going on with any
padding or such?


Otherwise

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>


>  	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
--
Kieran