| Message ID | 20210308101356.59333-3-paul.elder@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
On 08/03/2021 10:13, Paul Elder wrote: > Now that setRational() supports setting multiple rational values, use > that in setGPSDateTimestamp and setGPSDMS which previously set every > rational manually. This feels better to me ;-) > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > --- > src/android/jpeg/exif.cpp | 24 ++---------------------- > 1 file changed, 2 insertions(+), 22 deletions(-) > > diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp > index e5a3e448..c8b2f072 100644 > --- a/src/android/jpeg/exif.cpp > +++ b/src/android/jpeg/exif.cpp > @@ -345,24 +345,13 @@ void Exif::setGPSDateTimestamp(time_t timestamp) > EXIF_FORMAT_ASCII, tsStr); > > /* Set GPS_TIME_STAMP */ > - ExifEntry *entry = > - createEntry(EXIF_IFD_GPS, > - static_cast<ExifTag>(EXIF_TAG_GPS_TIME_STAMP), > - EXIF_FORMAT_RATIONAL, 3, 3 * sizeof(ExifRational)); > - if (!entry) > - return; > - > ExifRational ts[] = { > { static_cast<ExifLong>(tm.tm_hour), 1 }, > { static_cast<ExifLong>(tm.tm_min), 1 }, > { static_cast<ExifLong>(tm.tm_sec), 1 }, > }; > > - for (int i = 0; i < 3; i++) > - exif_set_rational(entry->data + i * sizeof(ExifRational), > - order_, ts[i]); > - > - exif_entry_unref(entry); > + setRational(EXIF_IFD_GPS, static_cast<ExifTag>(EXIF_TAG_GPS_TIME_STAMP), 3, ts); We're now encoding the size of these tags (EXIF_TAG_GPS_TIME_STAMP) in our layer, where for most other tags are defined by libexif. Given that libexif doesn't support those tags correctly it would seem, this is fine - but perhaps we should send a patch to libexif as well. Even if we patch libexif, we will still have to do this ourselves so no blocker. Also rather than hardcoding '3' should we use std::size(coords) ? 5cfbbcd20731 ("libcamera: Replace ARRAY_SIZE() with std::size()") suggests it should be possible. > } > > std::tuple<int, int, int> Exif::degreesToDMS(double decimalDegrees) > @@ -376,22 +365,13 @@ std::tuple<int, int, int> Exif::degreesToDMS(double decimalDegrees) > > void Exif::setGPSDMS(ExifIfd ifd, ExifTag tag, int deg, int min, int sec) > { > - ExifEntry *entry = createEntry(ifd, tag, EXIF_FORMAT_RATIONAL, 3, > - 3 * sizeof(ExifRational)); > - if (!entry) > - return; > - > ExifRational coords[] = { > { static_cast<ExifLong>(deg), 1 }, > { static_cast<ExifLong>(min), 1 }, > { static_cast<ExifLong>(sec), 1 }, > }; > > - for (int i = 0; i < 3; i++) > - exif_set_rational(entry->data + i * sizeof(ExifRational), > - order_, coords[i]); > - > - exif_entry_unref(entry); > + setRational(ifd, tag, 3, coords); And here with std::size() With that Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > } > > /* >
Hi Paul, Thank you for the patch. On Mon, Mar 08, 2021 at 10:46:42AM +0000, Kieran Bingham wrote: > On 08/03/2021 10:13, Paul Elder wrote: > > Now that setRational() supports setting multiple rational values, use > > that in setGPSDateTimestamp and setGPSDMS which previously set every > > rational manually. > > This feels better to me ;-) > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > > src/android/jpeg/exif.cpp | 24 ++---------------------- > > 1 file changed, 2 insertions(+), 22 deletions(-) > > > > diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp > > index e5a3e448..c8b2f072 100644 > > --- a/src/android/jpeg/exif.cpp > > +++ b/src/android/jpeg/exif.cpp > > @@ -345,24 +345,13 @@ void Exif::setGPSDateTimestamp(time_t timestamp) > > EXIF_FORMAT_ASCII, tsStr); > > > > /* Set GPS_TIME_STAMP */ > > - ExifEntry *entry = > > - createEntry(EXIF_IFD_GPS, > > - static_cast<ExifTag>(EXIF_TAG_GPS_TIME_STAMP), > > - EXIF_FORMAT_RATIONAL, 3, 3 * sizeof(ExifRational)); > > - if (!entry) > > - return; > > - > > ExifRational ts[] = { > > { static_cast<ExifLong>(tm.tm_hour), 1 }, > > { static_cast<ExifLong>(tm.tm_min), 1 }, > > { static_cast<ExifLong>(tm.tm_sec), 1 }, > > }; > > > > - for (int i = 0; i < 3; i++) > > - exif_set_rational(entry->data + i * sizeof(ExifRational), > > - order_, ts[i]); > > - > > - exif_entry_unref(entry); > > + setRational(EXIF_IFD_GPS, static_cast<ExifTag>(EXIF_TAG_GPS_TIME_STAMP), 3, ts); > > We're now encoding the size of these tags (EXIF_TAG_GPS_TIME_STAMP) in > our layer, where for most other tags are defined by libexif. > > Given that libexif doesn't support those tags correctly it would seem, > this is fine - but perhaps we should send a patch to libexif as well. I've been tempted to send patches to libexif before... > Even if we patch libexif, we will still have to do this ourselves so no > blocker. ... and this is what stopped me :-) Coupled with the fact that the libexif API definitely shows its age, and I would have been too tempted of rewriting parts of it :-D The project doesn't seem very active, but it could still be worth a try. > Also rather than hardcoding '3' should we use std::size(coords) ? I assume you mean ts here. If setRational() switched to Span, then this would be automatic :-) Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > 5cfbbcd20731 ("libcamera: Replace ARRAY_SIZE() with std::size()") > suggests it should be possible. > > > } > > > > std::tuple<int, int, int> Exif::degreesToDMS(double decimalDegrees) > > @@ -376,22 +365,13 @@ std::tuple<int, int, int> Exif::degreesToDMS(double decimalDegrees) > > > > void Exif::setGPSDMS(ExifIfd ifd, ExifTag tag, int deg, int min, int sec) > > { > > - ExifEntry *entry = createEntry(ifd, tag, EXIF_FORMAT_RATIONAL, 3, > > - 3 * sizeof(ExifRational)); > > - if (!entry) > > - return; > > - > > ExifRational coords[] = { > > { static_cast<ExifLong>(deg), 1 }, > > { static_cast<ExifLong>(min), 1 }, > > { static_cast<ExifLong>(sec), 1 }, > > }; > > > > - for (int i = 0; i < 3; i++) > > - exif_set_rational(entry->data + i * sizeof(ExifRational), > > - order_, coords[i]); > > - > > - exif_entry_unref(entry); > > + setRational(ifd, tag, 3, coords); > > And here with std::size() > > With that > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > } > > > > /*
On 08/03/2021 14:38, Laurent Pinchart wrote: > Hi Paul, > > Thank you for the patch. > > On Mon, Mar 08, 2021 at 10:46:42AM +0000, Kieran Bingham wrote: >> On 08/03/2021 10:13, Paul Elder wrote: >>> Now that setRational() supports setting multiple rational values, use >>> that in setGPSDateTimestamp and setGPSDMS which previously set every >>> rational manually. >> >> This feels better to me ;-) >> >>> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> >>> --- >>> src/android/jpeg/exif.cpp | 24 ++---------------------- >>> 1 file changed, 2 insertions(+), 22 deletions(-) >>> >>> diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp >>> index e5a3e448..c8b2f072 100644 >>> --- a/src/android/jpeg/exif.cpp >>> +++ b/src/android/jpeg/exif.cpp >>> @@ -345,24 +345,13 @@ void Exif::setGPSDateTimestamp(time_t timestamp) >>> EXIF_FORMAT_ASCII, tsStr); >>> >>> /* Set GPS_TIME_STAMP */ >>> - ExifEntry *entry = >>> - createEntry(EXIF_IFD_GPS, >>> - static_cast<ExifTag>(EXIF_TAG_GPS_TIME_STAMP), >>> - EXIF_FORMAT_RATIONAL, 3, 3 * sizeof(ExifRational)); >>> - if (!entry) >>> - return; >>> - >>> ExifRational ts[] = { >>> { static_cast<ExifLong>(tm.tm_hour), 1 }, >>> { static_cast<ExifLong>(tm.tm_min), 1 }, >>> { static_cast<ExifLong>(tm.tm_sec), 1 }, >>> }; >>> >>> - for (int i = 0; i < 3; i++) >>> - exif_set_rational(entry->data + i * sizeof(ExifRational), >>> - order_, ts[i]); >>> - >>> - exif_entry_unref(entry); >>> + setRational(EXIF_IFD_GPS, static_cast<ExifTag>(EXIF_TAG_GPS_TIME_STAMP), 3, ts); >> >> We're now encoding the size of these tags (EXIF_TAG_GPS_TIME_STAMP) in >> our layer, where for most other tags are defined by libexif. >> >> Given that libexif doesn't support those tags correctly it would seem, >> this is fine - but perhaps we should send a patch to libexif as well. > > I've been tempted to send patches to libexif before... > >> Even if we patch libexif, we will still have to do this ourselves so no >> blocker. > > ... and this is what stopped me :-) Coupled with the fact that the > libexif API definitely shows its age, and I would have been too tempted > of rewriting parts of it :-D The project doesn't seem very active, but > it could still be worth a try. > >> Also rather than hardcoding '3' should we use std::size(coords) ? > > I assume you mean ts here. If setRational() switched to Span, then this > would be automatic :-) Argh, that's because I spotted this below, moved the text up to keep the flow - and forgot to update the reference. Span sounds like a good idea to me though ;-) > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> 5cfbbcd20731 ("libcamera: Replace ARRAY_SIZE() with std::size()") >> suggests it should be possible. >> >>> } >>> >>> std::tuple<int, int, int> Exif::degreesToDMS(double decimalDegrees) >>> @@ -376,22 +365,13 @@ std::tuple<int, int, int> Exif::degreesToDMS(double decimalDegrees) >>> >>> void Exif::setGPSDMS(ExifIfd ifd, ExifTag tag, int deg, int min, int sec) >>> { >>> - ExifEntry *entry = createEntry(ifd, tag, EXIF_FORMAT_RATIONAL, 3, >>> - 3 * sizeof(ExifRational)); >>> - if (!entry) >>> - return; >>> - >>> ExifRational coords[] = { >>> { static_cast<ExifLong>(deg), 1 }, >>> { static_cast<ExifLong>(min), 1 }, >>> { static_cast<ExifLong>(sec), 1 }, >>> }; >>> >>> - for (int i = 0; i < 3; i++) >>> - exif_set_rational(entry->data + i * sizeof(ExifRational), >>> - order_, coords[i]); >>> - >>> - exif_entry_unref(entry); >>> + setRational(ifd, tag, 3, coords); >> >> And here with std::size() >> >> With that >> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> >>> } >>> >>> /* >
diff --git a/src/android/jpeg/exif.cpp b/src/android/jpeg/exif.cpp index e5a3e448..c8b2f072 100644 --- a/src/android/jpeg/exif.cpp +++ b/src/android/jpeg/exif.cpp @@ -345,24 +345,13 @@ void Exif::setGPSDateTimestamp(time_t timestamp) EXIF_FORMAT_ASCII, tsStr); /* Set GPS_TIME_STAMP */ - ExifEntry *entry = - createEntry(EXIF_IFD_GPS, - static_cast<ExifTag>(EXIF_TAG_GPS_TIME_STAMP), - EXIF_FORMAT_RATIONAL, 3, 3 * sizeof(ExifRational)); - if (!entry) - return; - ExifRational ts[] = { { static_cast<ExifLong>(tm.tm_hour), 1 }, { static_cast<ExifLong>(tm.tm_min), 1 }, { static_cast<ExifLong>(tm.tm_sec), 1 }, }; - for (int i = 0; i < 3; i++) - exif_set_rational(entry->data + i * sizeof(ExifRational), - order_, ts[i]); - - exif_entry_unref(entry); + setRational(EXIF_IFD_GPS, static_cast<ExifTag>(EXIF_TAG_GPS_TIME_STAMP), 3, ts); } std::tuple<int, int, int> Exif::degreesToDMS(double decimalDegrees) @@ -376,22 +365,13 @@ std::tuple<int, int, int> Exif::degreesToDMS(double decimalDegrees) void Exif::setGPSDMS(ExifIfd ifd, ExifTag tag, int deg, int min, int sec) { - ExifEntry *entry = createEntry(ifd, tag, EXIF_FORMAT_RATIONAL, 3, - 3 * sizeof(ExifRational)); - if (!entry) - return; - ExifRational coords[] = { { static_cast<ExifLong>(deg), 1 }, { static_cast<ExifLong>(min), 1 }, { static_cast<ExifLong>(sec), 1 }, }; - for (int i = 0; i < 3; i++) - exif_set_rational(entry->data + i * sizeof(ExifRational), - order_, coords[i]); - - exif_entry_unref(entry); + setRational(ifd, tag, 3, coords); } /*
Now that setRational() supports setting multiple rational values, use that in setGPSDateTimestamp and setGPSDMS which previously set every rational manually. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- src/android/jpeg/exif.cpp | 24 ++---------------------- 1 file changed, 2 insertions(+), 22 deletions(-)