OpenSfM icon indicating copy to clipboard operation
OpenSfM copied to clipboard

Handle malformed XMP

Open pierotofy opened this issue 4 years ago • 10 comments

Hello :hand:

This PR proposes the addition of a second attempt in order to fix faulty XML in XMP tags from cameras that have malformed/invalid XML. It doesn't change the original behavior of the function. It loads BeautifulSoup lazily since I don't think too many cameras have this problem. The lxml library is a requirement for beautifulsoup.

I hope this can be useful to make OpenSfM more stable. 🙏 Let me know if changes are needed?

pierotofy avatar Jun 19 '20 15:06 pierotofy

I'm fine with the changes, but I have a few requests :

  • Let's imports BeautifulSoup at the top of the file since it is a requirement anyway. Makes the code less obfuscated.
  • I had trouble reading and understanding the combination of range loop + try/except. Could we remove the loop and make each step explicit, even if longuer and has nested try/except ? It would be more readable and indicates this is a tricky step part per-se.

YanNoun avatar Jun 29 '20 20:06 YanNoun

@pierotofy another thing would be to have at the image that caused the need for this fix and/or the corresponding malformed string so we have proper unit testing for that one, as it gets tricky.

YanNoun avatar Jul 01 '20 12:07 YanNoun

Thanks for the recommendations @YanNoun I'll work on the changes and also to provide you with a test case.

pierotofy avatar Jul 01 '20 12:07 pierotofy

Suggestions implemented. :+1:

Test case:

from opensfm.exif import EXIF

e = EXIF(open("./DJI_0018.JPG"))
print(e.xmp)

Input image (with corrupted XMP): https://uav4geo.com/static/downloads/DJI_0018.JPG

Before the fix the print statement should fail. After the fix it should work.

The XMP string from the image is:

<?xpacket begin="" id="W5M0MpCehiHzreSzNTczkc9d"?> <x:xmpmeta xmlns:x="adobe:ns:meta/" x:xmptk="XMP Core 4.4.0-Exiv2"> <rdf:RDF xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#"> <rdf:Description rdf:about=""xmlns:xmp="http://ns.adobe.com/xap/1.0/" xmlns:tiff="http://ns.adobe.com/tiff/1.0/" xmlns:dc="http://purl.org/dc/elements/1.1/" xmlns:drone-dji="http://www.dji.com/drone-dji/1.0/" xmlns:crs="http://ns.adobe.com/camera-raw-settings/1.0/" xmp:ModifyDate="2016-06-23" xmp:CreateDate="2016-06-23" tiff:Make="DJI" tiff:Model="FC300S" dc:format="image/jpeg" drone-dji:AbsoluteAltitude="+198.31" drone-dji:RelativeAltitude="+39.80" drone-dji:GimbalRollDegree="+0.00" drone-dji:GimbalYawDegree="+45.00" drone-dji:GimbalPitchDegree="-89.90" drone-dji:FlightRollDegree="+3.00" drone-dji:FlightYawDegree="+46.00" drone-dji:FlightPitchDegree="+1.80" crs:Version="7.0" crs:HasSettings="False" crs:HasCrop="False" crs:AlreadyApplied="False"/> </rdf:RDF>  </x:xmpmeta>

pierotofy avatar Jul 01 '20 13:07 pierotofy

Hi @pierotofy!

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

facebook-github-bot avatar Jul 17 '20 07:07 facebook-github-bot

Mm, is a CLA required now?

Not opposed to signing one, just not sure if I want to sign it via Facebook. :)

pierotofy avatar Aug 07 '20 01:08 pierotofy

Signed the CLA through Facebook, but this still fails. :confused:

Edit: ah, just a bit of delay. Check passed. :heavy_check_mark:

pierotofy avatar Sep 08 '20 14:09 pierotofy

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

facebook-github-bot avatar Sep 08 '20 14:09 facebook-github-bot

CLA signed, rebased the branch, fixed travis CI.

Let me know if further changes are needed :pray:

pierotofy avatar Sep 10 '20 03:09 pierotofy

Not sure if there's still interest in merging this one, I can close it if not. :pray:

pierotofy avatar Apr 21 '21 13:04 pierotofy