-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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.
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
Add EXIF thumbnail support for JPEG files #1234
Conversation
da363f4
to
ed50faf
Compare
Those assignments are actually effective...
How can I fix the import error? |
7968bb3
to
c7c2439
Compare
Fixed CI, all set 🚀 |
45d08f8
to
2bcfb3b
Compare
Latest updated includes a fix for thumbnail distortion. |
It's pretty cool. very very fast and smooth! |
3d12d2b
to
f22c4eb
Compare
Thanks for reporting. Reason was because EXIF exists, but does not contain a thumbnail in the files provided. This is fixed in the newest version of the pull request. Additionally, it was rebased onto master and added the |
Wow, I checked it. Perfect!! very fast! especially for the folder with tons of pics. and it 's pretty cool for the low-end server! |
Oops, I just found a small bug. For some pictures, It looks like the thumbnails are broken, only shows half of them. Would you please check out these pics? |
f22c4eb
to
9fe500c
Compare
Hi niubility, thanks for testing this PR so thoroughly. The thumbnails in your files were larger than the preloaded size of 32k. Theoretically, Exif data (including the thumbnails) must fit in the APP1 marker and all JPEG markers can be max 64k. |
9fe500c
to
be495f2
Compare
Improved code style |
Great! I have tried. It's wonderful! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add tests
wrappedReader := io.MultiReader(buf, in) | ||
|
||
offset := 0 | ||
offsets := []int{12, 30} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please explain why are you checking those offsets only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those are the two most common offsets where the Exif starts in JPEG files. To avoid more complex parsing (and library dependencies), it only checks those offsets.
If no Exif is found, it will fall back to the downscaling method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked that in the provided library the different approach is used https://github.com/dsoprea/go-exif/blob/d154f10435cc/v3/exif.go#L52
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. And that's the approach in the first (unpublished) attempt. That function tries to find an exif in brute-force fashion byte by byte scanning the whole file: https://github.com/dsoprea/go-exif/blob/d154f10435cc6ae4b64cd68eed04a1d005b73ec7/v3/exif.go#L150
It is more efficient by just checking the most common offsets for exif and there was a noticeable performance increase.
Writing tests will require quite some effort... I will look into it when I have time |
go.mod
Outdated
@@ -10,6 +10,7 @@ require ( | |||
github.com/dgrijalva/jwt-go v3.2.0+incompatible | |||
github.com/disintegration/imaging v1.6.2 | |||
github.com/dsnet/compress v0.0.1 // indirect | |||
github.com/dsoprea/go-exif/v3 v3.0.0-20201216222538-db167117f483 // indirect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not stick to version v3.0.0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly no clue. That was added by go get
automatically...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix it, please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked into this and tried to stick to the version number you mentioned. However, the library does not seem provide tagged releases:
:~/Code/filebrowser*master$ go get -d github.com/dsoprea/go-exif/v3@v3.0.0
go get: github.com/dsoprea/go-exif/v3@v3.0.0: invalid version: unknown revision v3.0.0
:~/Code/filebrowser*master$ go get -d github.com/dsoprea/go-exif/v3@3.0.0
go: downloading github.com/dsoprea/go-exif v0.0.0-20200711180620-208788de2815
go get: module github.com/dsoprea/go-exif@3.0.0 found (v0.0.0-20200711180620-208788de2815), but does not contain package github.com/dsoprea/go-exif/v3
The library is pulled as recommended. Other projects use the library in a similar way. Examples are PhotoPrism (listed on Awesome-Selfhosted) and other projects on GitHub:
As such, the dependency file should probably be kept as committed.
Constructing pictures programmatically requires a lot of code and is error-prone in itself. On the other hand, the pictures provided by niubility000 are a very useful set and are "real" data. Do you prefer a specific structure to check in the files? |
@adrium I agree that using image files will be better for that. If you plan to add table tests, please look at https://github.com/filebrowser/filebrowser/blob/master/img/service_test.go for a reference. |
@o1egl Thanks a lot for your feedback. @niubility000 Do you agree to make use of the pictures you provided for testing and thereby checking them in into this repo? |
Tests added. Image credits:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I just found a bug: #2137 |
Description
Compact implementation to support EXIF thumbnails. Listing a directory with a lot of thumbnails will be a lot faster. The following was recorded with the master branch and with the feature proposed here. The app runs on a Raspberry Pi 4b accessing files on a hard disk.