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

Lazily use ImageFileDirectory_v1 values from Exif #4031

Merged
merged 4 commits into from Sep 9, 2019
Merged

Lazily use ImageFileDirectory_v1 values from Exif #4031

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0b405c8
Lazily use ImageFileDirectory_v1 values from Exif
radarhere Aug 18, 2019
f3ed44a
Changed the Image getexif method to return a shared Exif instance
radarhere Aug 22, 2019
ef16cb8
ImageFileDirectory_v1 does not raise KeyError
radarhere Sep 7, 2019
5a66877
Added tests
radarhere Sep 8, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 10 additions & 0 deletions Tests/test_imagefile.py
View file
Open in desktop
Expand Up @@ -321,3 +321,13 @@ def test_exif_interop(self):
self.assertEqual(
exif.get_ifd(0xA005), {1: "R98", 2: b"0100", 4097: 2272, 4098: 1704}
)

def test_exif_shared(self):
im = Image.open("Tests/images/exif.png")
exif = im.getexif()
self.assertIs(im.getexif(), exif)

def test_exif_str(self):
im = Image.open("Tests/images/exif.png")
exif = im.getexif()
self.assertEqual(str(exif), "{274: 1}")
96 changes: 61 additions & 35 deletions src/PIL/Image.py
View file
Open in desktop
Expand Up @@ -555,6 +555,7 @@ def __init__(self):
self.category = NORMAL
self.readonly = 0
self.pyaccess = None
self._exif = None

@property
def width(self):
Expand Down Expand Up @@ -1324,10 +1325,10 @@ def getextrema(self):
return self.im.getextrema()

def getexif(self):
exif = Exif()
if "exif" in self.info:
exif.load(self.info["exif"])
return exif
if self._exif is None:
self._exif = Exif()
self._exif.load(self.info.get("exif"))
return self._exif

def getim(self):
"""
Expand Down Expand Up @@ -3137,25 +3138,27 @@ class Exif(MutableMapping):
def __init__(self):
self._data = {}
self._ifds = {}
self._info = None
self._loaded_exif = None

def _fixup(self, value):
try:
if len(value) == 1 and not isinstance(value, dict):
return value[0]
except Exception:
pass
return value

def _fixup_dict(self, src_dict):
# Helper function for _getexif()
# returns a dict with any single item tuples/lists as individual values
def _fixup(value):
try:
if len(value) == 1 and not isinstance(value, dict):
return value[0]
except Exception:
pass
return value

return {k: _fixup(v) for k, v in src_dict.items()}
return {k: self._fixup(v) for k, v in src_dict.items()}

def _get_ifd_dict(self, tag):
try:
# an offset pointer to the location of the nested embedded IFD.
# It should be a long, but may be corrupted.
self.fp.seek(self._data[tag])
self.fp.seek(self[tag])
except (KeyError, TypeError):
pass
else:
Expand All @@ -3172,29 +3175,31 @@ def load(self, data):

# The EXIF record consists of a TIFF file embedded in a JPEG
# application marker (!).
if data == self._loaded_exif:
return
self._loaded_exif = data
self._data.clear()
self._ifds.clear()
self._info = None
if not data:
return

self.fp = io.BytesIO(data[6:])
self.head = self.fp.read(8)
# process dictionary
from . import TiffImagePlugin

info = TiffImagePlugin.ImageFileDirectory_v1(self.head)
self.endian = info._endian
self.fp.seek(info.next)
info.load(self.fp)
self._data = dict(self._fixup_dict(info))
self._info = TiffImagePlugin.ImageFileDirectory_v1(self.head)
self.endian = self._info._endian
self.fp.seek(self._info.next)
self._info.load(self.fp)

# get EXIF extension
ifd = self._get_ifd_dict(0x8769)
if ifd:
self._data.update(ifd)
self._ifds[0x8769] = ifd

# get gpsinfo extension
ifd = self._get_ifd_dict(0x8825)
if ifd:
self._data[0x8825] = ifd
self._ifds[0x8825] = ifd

def tobytes(self, offset=0):
from . import TiffImagePlugin

Expand All @@ -3203,19 +3208,20 @@ def tobytes(self, offset=0):
else:
head = b"MM\x00\x2A\x00\x00\x00\x08"
ifd = TiffImagePlugin.ImageFileDirectory_v2(ifh=head)
for tag, value in self._data.items():
for tag, value in self.items():
ifd[tag] = value
return b"Exif\x00\x00" + head + ifd.tobytes(offset)

def get_ifd(self, tag):
if tag not in self._ifds and tag in self._data:
if tag == 0xA005: # interop
if tag not in self._ifds and tag in self:
if tag in [0x8825, 0xA005]:
# gpsinfo, interop
self._ifds[tag] = self._get_ifd_dict(tag)
elif tag == 0x927C: # makernote
from .TiffImagePlugin import ImageFileDirectory_v2

if self._data[0x927C][:8] == b"FUJIFILM":
exif_data = self._data[0x927C]
if self[0x927C][:8] == b"FUJIFILM":
exif_data = self[0x927C]
ifd_offset = i32le(exif_data[8:12])
ifd_data = exif_data[ifd_offset:]

Expand Down Expand Up @@ -3252,8 +3258,8 @@ def get_ifd(self, tag):
ImageFileDirectory_v2(), data, False
)
self._ifds[0x927C] = dict(self._fixup_dict(makernote))
elif self._data.get(0x010F) == "Nintendo":
ifd_data = self._data[0x927C]
elif self.get(0x010F) == "Nintendo":
ifd_data = self[0x927C]

makernote = {}
for i in range(0, struct.unpack(">H", ifd_data[:2])[0]):
Expand Down Expand Up @@ -3291,27 +3297,47 @@ def get_ifd(self, tag):
return self._ifds.get(tag, {})

def __str__(self):
if self._info is not None:
# Load all keys into self._data
for tag in self._info.keys():
self[tag]

return str(self._data)

def __len__(self):
return len(self._data)
keys = set(self._data)
if self._info is not None:
keys.update(self._info)
return len(keys)

def __getitem__(self, tag):
if self._info is not None and tag not in self._data and tag in self._info:
self._data[tag] = self._fixup(self._info[tag])
if tag == 0x8825:
self._data[tag] = self.get_ifd(tag)
del self._info[tag]
return self._data[tag]

def __contains__(self, tag):
return tag in self._data
return tag in self._data or (self._info is not None and tag in self._info)

if not py3:

def has_key(self, tag):
return tag in self

def __setitem__(self, tag, value):
if self._info is not None and tag in self._info:
del self._info[tag]
self._data[tag] = value

def __delitem__(self, tag):
if self._info is not None and tag in self._info:
del self._info[tag]
del self._data[tag]

def __iter__(self):
return iter(set(self._data))
keys = set(self._data)
if self._info is not None:
keys.update(self._info)
return iter(keys)
14 changes: 2 additions & 12 deletions src/PIL/JpegImagePlugin.py
View file
Open in desktop
Expand Up @@ -158,7 +158,7 @@ def APP(self, marker):
# If DPI isn't in JPEG header, fetch from EXIF
if "dpi" not in self.info and "exif" in self.info:
try:
exif = self._getexif()
exif = self.getexif()
radarhere marked this conversation as resolved.
Show resolved Hide resolved
resolution_unit = exif[0x0128]
x_resolution = exif[0x011A]
try:
Expand Down Expand Up @@ -485,19 +485,9 @@ def _fixup_dict(src_dict):


def _getexif(self):
# Use the cached version if possible
try:
return self.info["parsed_exif"]
except KeyError:
pass

if "exif" not in self.info:
return None
exif = dict(self.getexif())

# Cache the result for future use
self.info["parsed_exif"] = exif
return exif
return dict(self.getexif())


def _getmp(self):
Expand Down
4 changes: 1 addition & 3 deletions src/PIL/MpoImagePlugin.py
View file
Open in desktop
Expand Up @@ -86,13 +86,11 @@ def seek(self, frame):
self.offset = self.__mpoffsets[frame]

self.fp.seek(self.offset + 2) # skip SOI marker
if "parsed_exif" in self.info:
del self.info["parsed_exif"]
if i16(self.fp.read(2)) == 0xFFE1: # APP1
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.

if 40962 in exif and 40963 in exif:
self._size = (exif[40962], exif[40963])
elif "exif" in self.info:
Expand Down