| Message ID | 20210308101356.59333-2-paul.elder@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@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@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); >
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@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@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);
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@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@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);
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)); 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);
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@ideasonboard.com> --- src/android/jpeg/exif.cpp | 13 +++++++++++-- src/android/jpeg/exif.h | 2 ++ 2 files changed, 13 insertions(+), 2 deletions(-)