Replace piexif with custom orientation-only exif reader/writer#1210
Replace piexif with custom orientation-only exif reader/writer#1210heynemann merged 1 commit intothumbor:masterfrom
Conversation
1 similar comment
okor
left a comment
There was a problem hiding this comment.
Thank you!
There are couple of "code quality" warnings which would be cool to clean up: https://app.codacy.com/app/scorphus/thumbor/pullRequest?prid=3867912
And I left some comments about making some of the more obscure looking code more clear to future devs.
But it's great!
| if data[:6] != EXIF_HEADER: | ||
| raise SyntaxError("not a TIFF file (header %r not valid)" % data[:4]) | ||
|
|
||
| self.fp = BytesIO(data[6:]) |
There was a problem hiding this comment.
File pointer.
Struggling to find better name. Any suggestions?
There was a problem hiding this comment.
Well, since ... I think ... what you are doing here is declaring a named, binary buffer for data[6:] range of bytes ... we might be able to call this exif_buffer?
There was a problem hiding this comment.
I agree with changing the names to more descriptive variables. Great work btw.
| raise SyntaxError("not a TIFF file (header %r not valid)" % data[:4]) | ||
|
|
||
| self.fp = BytesIO(data[6:]) | ||
| ifh = self.fp.read(8) |
There was a problem hiding this comment.
It might be helpful to comment why this is only reading out 8 bytes. May be self documenting if ifh had a more descriptive name?
| _endian = "<" | ||
| _offset = None | ||
|
|
||
| def __init__(self, data): |
There was a problem hiding this comment.
Generally speaking, this __init__ method is quite dense and very difficult to tell what is going on ... at least for a non-pythonist/non-image expert. It may be helpful for future devs to explain what is going on here via comments or even named functions.
|
As soon as we get more descriptive names, I think this is good to merge. Thanks @kkopachev!!! Great work! |
200fa6e to
c9d583a
Compare
|
Thank you for your feedback. Take a look now please. |
| self.exif_buffer.seek(ifd_offset) | ||
|
|
||
| for i in range(self._unpack("H", self.exif_buffer.read(2))[0]): | ||
| tag, typ, count, data = self._unpack("HHL4s", self.exif_buffer.read(12)) |
There was a problem hiding this comment.
The same here with data variable.
There was a problem hiding this comment.
figured we don't really need all these info, so just unpack tag id.
| ifd_offset, = self._unpack("L", header[4:]) | ||
| self.exif_buffer.seek(ifd_offset) | ||
|
|
||
| for i in range(self._unpack("H", self.exif_buffer.read(2))[0]): |
There was a problem hiding this comment.
Since we are not using the i variable, would it be good to change it to _?
What you think?
c9d583a to
d1f696b
Compare
d1f696b to
f1d1321
Compare
|
Any updates here? |
|
This looks pretty good! Thanks @kkopachev !!! |
Fixes #1204
Closes #1209
(please see both for discussions)
Replacing piexif dependency with simple orientation-only reader. It ignores all other exif tags and does not traverse through IFDs other then IFD0 where orientation supposed to be.