Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Replace exif lib #8761

Closed
wants to merge 3 commits into from
Closed

Replace exif lib #8761

wants to merge 3 commits into from

Conversation

bep
Copy link
Member

@bep bep commented Jul 15, 2021 •
edited
Loading

No description provided.

@bep bep changed the title Add failing minify test Replace exif lib Jul 15, 2021
@bep bep mentioned this pull request Jul 15, 2021
Closed
@danedavid @bep
Replace rwcarlsen/goexif with dsoprea/go-exif
fd1f1a1 
Replace underlying EXIF library (rwcarlsen/goexif) with an actively developed one(dsoprea/go-exif). Add tests for disable options (date & location) while decoding EXIF.
Fixes gohugoio#8586
@bep
Copy link
Member Author

bep commented Jul 15, 2021

@chrillek @jmooring I have reviewed and also tested this on a Exif project I have, and this all looks great to me -- but if you could, I would appreciate if you also could take this branch for a spin before I merge it ...?

@chrillek
Copy link

@chrillek @jmooring I have reviewed and also tested this on a Exif project I have, and this all looks great to me -- but if you could, I would appreciate if you also could take this branch for a spin before I merge it ...?

I'm afraid that's a bit beyond my abilities. If there'd be a binary, I could download that and test it.

@jmooring
Copy link
Member

jmooring commented Jul 15, 2021 •
edited
Loading

@bep Regression. No warnings. See:

Minimal example:

git clone --single-branch -b hugo-github-issue-8519 https://github.com/jmooring/hugo-testing hugo-github-issue-8519
cd hugo-github-issue-8519
hugo server

Edit: of course there are no warnings. The new lib fixed the problem. Sorry for the noise.

@jmooring
Copy link
Member

jmooring commented Jul 17, 2021 •
edited
Loading

I tested 2bd9e91 against Phil Harvey's collection of sample images. Of the 6924 images in his collection, only 9 gave me problems, so I think we're in pretty good shape. We were unable to decode the EXIF data in 117 of the remaining 6915 images, but at least there we no errors.

Site configuration: get default EXIF fields

Image v0.85.0 2bd9e91
LG_KC910.jpg Pass Pass
LG_KE990.jpg Pass Pass
LG_KE998.jpg Pass Pass
LG_KU990.jpg Pass Pass
LG_KU990i.jpg Pass Pass
LG_U990i.jpg Pass Pass
MotorolaC261.jpg Fail (1) Fail (3)
SamsungGT-i5700.jpg Fail (2) Fail (2)
SamsungSGH-D980.jpg Fail (1) Fail (4)

Site configuration: get all EXIF fields

Image v0.85.0 2bd9e91
LG_KC910.jpg Fail (1) Fail (4)
LG_KE990.jpg Fail (1) Fail (4)
LG_KE998.jpg Fail (1) Fail (4)
LG_KU990.jpg Fail (1) Fail (4)
LG_KU990i.jpg Fail (1) Fail (4)
LG_U990i.jpg Fail (1) Fail (4)
MotorolaC261.jpg Fail (1) Fail (4)
SamsungGT-i5700.jpg Fail (1) Fail (3)
SamsungSGH-D980.jpg Fail (1) Fail (4)

Notes

  1. Warning: Unable to decode Exif metadata from image
  2. Error: executing "main" at <.Exif>: error calling Exif: metadata init failed: json: unsupported value: NaN
  3. No error, no warning, and no EXIF fields displayed.
  4. Error: executing "main" at <.Exif>: error calling Exif: interface conversion: string is not error: missing method Error

Test site

git clone --single-branch -b hugo-github-issue-8761 https://github.com/jmooring/hugo-testing hugo-github-issue-8761
cd hugo-github-issue-8761
hugo

@bep
exif: Improve performance
9a00eef 
Also, do not return an error when input format isn't supported. Just return `nil` exif.

Using the stream API and avoid creating the Ifd mapping and tag index every time makes it faster:

```
name           old time/op    new time/op    delta
DecodeExif-16    3.42ms ± 0%    0.65ms ± 0%  -81.12%  (p=0.029 n=4+4)

name           old alloc/op   new alloc/op   delta
DecodeExif-16    1.51MB ± 0%    0.59MB ± 0%  -60.77%  (p=0.029 n=4+4)

name           old allocs/op  new allocs/op  delta
DecodeExif-16     12.9k ± 0%      1.9k ± 0%  -85.48%  (p=0.029 n=4+4)
```

It's still slower than what we had, though, but correctness comes at a cost:

```

name           old time/op    new time/op    delta
DecodeExif-16     186µs ± 0%     648µs ± 1%  +249.21%  (p=0.029 n=4+4)

name           old alloc/op   new alloc/op   delta
DecodeExif-16     184kB ± 0%     593kB ± 0%  +222.38%  (p=0.029 n=4+4)

name           old allocs/op  new allocs/op  delta
DecodeExif-16     1.20k ± 0%     1.88k ± 0%   +55.99%  (p=0.029 n=4+4)
```

See gohugoio#8586
@bep
Copy link
Member Author

bep commented Jul 19, 2021

Waiting for a merge of this one: dsoprea/go-exif#63

@jmooring note that the last commit I added means that it will not build for you. Thanks for the extensive test work, some of the errors looks a little odd, but we can live with those. Knowing about them means we can eventually fix them.

@jmooring jmooring mentioned this pull request Sep 26, 2021
Closed
@bep
Copy link
Member Author

bep commented Feb 16, 2022

Closing this. The upstream library doesn't seem to be actively maintained, and I'm certainly not prepared to take on another one.

@bep bep closed this Feb 16, 2022
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
BlockedUpstream
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bep @chrillek @jmooring @danedavid