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

[Laurent Pinchart]

Hello,

On Mon, Mar 08, 2021 at 10:41:43AM +0000, Kieran Bingham wrote:
> 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.

It's a bit annoying, it would be nice if we could still benefit from the
initialization, although I don't expect lots of changes in libexif that
would cause breakages later.

Maybe we could, in setRational(), check the tag, and call the simple or
extended version of createEntry() accordingly ? It's a bit of a layering
violation and isn't very nice. Up to you, I'd be fine either way.

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

Laurent Pinchart