D16617: fix extraction of GPS altitude for exif data
This revision was automatically updated to reflect the committed changes. Closed by commit R286:ca8c11351fe7: fix extraction of GPS altitude for exif data (authored by astippich). REPOSITORY R286 KFileMetaData CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D16617?vs=44723&id=46624 REVISION DETAIL https://phabricator.kde.org/D16617 AFFECTED FILES autotests/exiv2extractortest.cpp autotests/samplefiles/test.jpg src/extractors/exiv2extractor.cpp src/extractors/exiv2extractor.h To: astippich, #frameworks, bruns Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D16617: fix extraction of GPS altitude for exif data
astippich added a comment. Thanks INLINE COMMENTS > bruns wrote in exiv2extractor.cpp:217 > This is bogus, why am I not allowed to take photos at sea level? Or in > Greenwhich, at the Equator? > > Invalid data should **not** be signaled by 0.0 Yep, I will change that in another diff REPOSITORY R286 KFileMetaData BRANCH exif_gps_altitude REVISION DETAIL https://phabricator.kde.org/D16617 To: astippich, #frameworks, bruns Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D16617: fix extraction of GPS altitude for exif data
bruns accepted this revision. bruns added inline comments. This revision is now accepted and ready to land. INLINE COMMENTS > exiv2extractor.cpp:217 > > if (altitude) { > result->add(Property::PhotoGpsAltitude, altitude); This is bogus, why am I not allowed to take photos at sea level? Or in Greenwhich, at the Equator? Invalid data should **not** be signaled by 0.0 > astippich wrote in exiv2extractor.cpp:285 > true, but why not use the converted value directly? Otherwise the conversion > would have to be made manually by dividing, see fetchGpsDouble() You are correct, the implicit conversions done by Exiv2 are easy to miss. > exiv2extractor.cpp:294 > +} > +return alt; > +} You could return `std::numeric_limits::quiet_NaN()` here for the error case. REPOSITORY R286 KFileMetaData BRANCH exif_gps_altitude REVISION DETAIL https://phabricator.kde.org/D16617 To: astippich, #frameworks, bruns Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D16617: fix extraction of GPS altitude for exif data
astippich added a comment. gentle ping REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D16617 To: astippich, #frameworks, bruns Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D16617: fix extraction of GPS altitude for exif data
astippich added a dependent revision: D16931: Extract more tags from exif metadata. REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D16617 To: astippich, #frameworks, bruns Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D16617: fix extraction of GPS altitude for exif data
astippich edited the summary of this revision. REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D16617 To: astippich, #frameworks, bruns Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D16617: fix extraction of GPS altitude for exif data
astippich added inline comments. INLINE COMMENTS > bruns wrote in exiv2extractor.cpp:285 > According to: > https://www.sno.phy.queensu.ca/~phil/exiftool/TagNames/GPS.html > and > http://www.cipa.jp/std/documents/e/DC-008-Translation-2016-E.pdf > > the absolute altitude value is a rational, like deg/min/sec. true, but why not use the converted value directly? Otherwise the conversion would have to be made manually by dividing, see fetchGpsDouble() REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D16617 To: astippich, #frameworks, bruns Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D16617: fix extraction of GPS altitude for exif data
bruns requested changes to this revision. bruns added inline comments. This revision now requires changes to proceed. INLINE COMMENTS > exiv2extractor.cpp:285 > +if (it != data.end()) { > +alt = it->value().toFloat(); > +it = data.findKey(Exiv2::ExifKey("Exif.GPSInfo.GPSAltitudeRef")); According to: https://www.sno.phy.queensu.ca/~phil/exiftool/TagNames/GPS.html and http://www.cipa.jp/std/documents/e/DC-008-Translation-2016-E.pdf the absolute altitude value is a rational, like deg/min/sec. REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D16617 To: astippich, #frameworks, bruns Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D16617: fix extraction of GPS altitude for exif data
astippich edited the summary of this revision. REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D16617 To: astippich, #frameworks, bruns Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D16617: fix extraction of GPS altitude for exif data
astippich added a dependency: D16560: create a separate test file for embedded images test. REPOSITORY R286 KFileMetaData REVISION DETAIL https://phabricator.kde.org/D16617 To: astippich, #frameworks, bruns Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams
D16617: fix extraction of GPS altitude for exif data
astippich created this revision. astippich added reviewers: Frameworks, bruns. Herald added projects: Frameworks, Baloo. Herald added subscribers: Baloo, kde-frameworks-devel. astippich requested review of this revision. REVISION SUMMARY The previous code was wrong, since the altitude is given with only a single double value below or above sea level. REPOSITORY R286 KFileMetaData BRANCH exif_gps_altitude REVISION DETAIL https://phabricator.kde.org/D16617 AFFECTED FILES autotests/exiv2extractortest.cpp autotests/samplefiles/test.jpg src/extractors/exiv2extractor.cpp src/extractors/exiv2extractor.h To: astippich, #frameworks, bruns Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, abrahams