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.

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

Lazily use ImageFileDirectory_v1 values from Exif #4031

Merged
merged 4 commits into from
Sep 9, 2019

Conversation

radarhere
Copy link
Member

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 _getexif to getexif, switching from JpegImagePlugin creating a dict of all the values to using the now-lazy Exif class.

Copy link
Contributor

@Glandos Glandos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of getexif instead of _getexif is not completely idempotent.

src/PIL/JpegImagePlugin.py Show resolved Hide resolved
@@ -92,7 +92,7 @@ def seek(self, frame):
n = i16(self.fp.read(2)) - 2
self.info["exif"] = ImageFile._safe_read(self.fp, n)

exif = self._getexif()
exif = self.getexif()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@hugovk
Copy link
Member

hugovk commented Sep 7, 2019

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 __str__ isn't covered by any tests at all:

image

https://codecov.io/gh/python-pillow/Pillow/compare/a44e918a5b647d5eb75e0bf3fe3df4d6501cb78d...f3ed44a5662ea2728fae2139bbdde9419356302a/diff#D1-3299

@radarhere
Copy link
Member Author

Okay, I've added a test for __str__.

The externally visible changes are the removal of info["parsed_exif"] and the fact that getexif() now returns a shared instance. I've added a test for the shared instance.

@hugovk
Copy link
Member

hugovk commented Sep 9, 2019

Thanks all!

@hugovk hugovk merged commit e5f6b86 into python-pillow:master Sep 9, 2019
@radarhere radarhere deleted the exif branch September 9, 2019 20:12
@radarhere radarhere mentioned this pull request Sep 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants