Lazily use ImageFileDirectory_v1 values from Exif#4031
Lazily use ImageFileDirectory_v1 values from Exif#4031hugovk merged 4 commits intopython-pillow:masterfrom
Conversation
Glandos
left a comment
There was a problem hiding this comment.
The use of getexif instead of _getexif is not completely idempotent.
| self.info["exif"] = ImageFile._safe_read(self.fp, n) | ||
|
|
||
| exif = self._getexif() | ||
| exif = self.getexif() |
There was a problem hiding this comment.
The main difference is that getexif from Image class does not cache the parsed Exif tags.
So this call is done and forget when the user will need it.
This is not what we should expect.
There was a problem hiding this comment.
I'm not following, sorry. Yes, using _getexif would create a cache of the parsed exif tags, but it would also iterate over all the values.
From my understanding, the point of these PRs is to reduce operations on load to potentially increase speed - we don't need to work out all of the EXIF values when a user might not even be interested in them. When JpegImagePlugin and MpoImagePlugin only need two EXIF values each, isn't decoding all the values overkill?
There was a problem hiding this comment.
Sorry for the unclear review.
Yes, calling getexif is better, but right now, when reading an image, the _getexif function is called, and the result is cached so that if the user called it once again, the computation will not happen a second time.
This is not the case with getexif. If I trace a call to Image.open("my_image.jpg").getexif(), then the dictionary loading and some decoding will happens twice.
In my opinion, the Exif object should be stored into the Image instance once loaded. And if the user needs to modify it, this object could be cloned.
I hope it is clearer now 😄
There was a problem hiding this comment.
Okay, see what you think of this idea - I have pushed a commit that changes im.getexif() so that it returns a shared instance of the Exif class, meaning that getexif decoding of EXIF values is now cached. This actually makes the _getexif "parsed_exif" cache largely redundant, because _getexif uses getexif, so I have removed that. Hopefully, that should make all scenarios faster.
There was a problem hiding this comment.
Very nice now :) Iterating 1000 times over an image (open().getexif()), I go from 18.0s to 8.11s. Confirmed by %timeit in IPython, going from 3.69ms per loop to 1.68ms per loop.
The last step (for me) is to get rid of _fixup_dict call of the tag 0x8769, but this should be in another PR.
573d25e to
e9e3431
Compare
|
Thanks @radarhere for the PR and @Glandos for the review (and other PRs)! Most of the changes are covered by existing tests. 👍 Would this benefit from adding simple tests to directly test some of the changed/added functions? If I'm following this report correctly, it looks like at least this
|
b102453 to
a866b61
Compare
|
Okay, I've added a test for The externally visible changes are the removal of |
|
Thanks all! |
ImageFileDirectory_v1 is lazy, performing operations when a value is accessed.
However, accessing all of those values to create a dictionary when loading the data undoes that speed increase.
#3663 set out to fix this by adding a new class - however, it had conflicts once #3625 was merged and a different class was added to deal with Exif data. This PR takes a different approach towards the original goal, for the new context.
It also uses this change by swapping calls in JpegImagePlugin and MpoImagePlugin from
_getexiftogetexif, switching from JpegImagePlugin creating a dict of all the values to using the now-lazy Exif class.