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

Refactor EpsImagePlugin and deprecate PSFile #6879

Merged
merged 18 commits into from Apr 1, 2023

Conversation

Yay295
Copy link
Contributor

@Yay295 Yay295 commented Jan 10, 2023

Merge the PSFile class into the EpsImageFile class to hopefully improve performance.
Also added a check for the required "%!PS-Adobe" and "%%BoundingBox" header comments.

This hopefully helps with #6877.

Yay295 and others added 2 commits January 10, 2023 08:44
Merge the PSFile class into the EpsImageFile class to hopefully improve performance.
Also added a check for the required "%!PS-Adobe" and "%%BoundingBox" header comments.
@hugovk
Copy link
Member

hugovk commented Jan 19, 2023

 class PSFile:
     """
     Wrapper for bytesio object that treats either CR or LF as end of line.
+    This class is no longer used internally, but kept for backwards-compatibility.
     """

If we're no longer using it internally, do we need to keep it around in the API?

Can it be deprecated? Is there a replacement?

@Yay295
Copy link
Contributor Author

Yay295 commented Jan 19, 2023

"PSFile is part of the public API, so we'd need to raise deprecation warnings for at least a year first before it can be removed in a major bump, which would be Pillow 11.0.0 in October 2024." - hugovk #6877 (comment)

I don't know how to add a deprecation warning though; I haven't done that before.

@hugovk
Copy link
Member

hugovk commented Jan 20, 2023

Yup.

To deprecate something:


Searching the top 5k projects on PyPI shows no real use of PSFile:

$ python3 ~/github/misc/cpython/search_pypi_top.py . "\bPSFile\b" -q
./pyre-check-0.9.16.tar.gz: pyre-check-0.9.16/typeshed/stubs/Pillow/PIL/EpsImagePlugin.pyi: class PSFile:
./Pillow-9.3.0.tar.gz: Pillow-9.3.0/Tests/test_file_eps.py: t = EpsImagePlugin.PSFile(f)
./Pillow-9.3.0.tar.gz: Pillow-9.3.0/Tests/test_file_eps.py: t = EpsImagePlugin.PSFile(r)
./Pillow-9.3.0.tar.gz: Pillow-9.3.0/src/PIL/EpsImagePlugin.py: class PSFile:
./Pillow-9.3.0.tar.gz: Pillow-9.3.0/src/PIL/EpsImagePlugin.py: fp = PSFile(self.fp)
./pytype-2022.10.26.tar.gz: pytype-2022.10.26/pytype/typeshed/stubs/Pillow/PIL/EpsImagePlugin.pyi: class PSFile:
./types-Pillow-9.3.0.0.tar.gz: types-Pillow-9.3.0.0/PIL-stubs/EpsImagePlugin.pyi: class PSFile:

Time: 0:00:23.017025
Found 7 matching lines in 4 projects

And I didn't spot any relevant use in the 940 GitHub results.


But before we go ahead, @radarhere what are your thoughts?

@radarhere
Copy link
Member

I don't mind deprecating PSFile. If it isn't used, then removing it makes things simpler.

You've also made various other changes here, such as making use of memoryview, which isn't used anywhere else in Pillow at the moment. Could you explain them a bit?

@Yay295
Copy link
Contributor Author

Yay295 commented Jan 28, 2023

This plugin reads lines byte-by-byte until it reaches the end of the line (or 255 bytes). PSFile does this by appending each byte to a list, joining the list, and decoding the bytes to a string.

def readline(self):
s = []
if self.char:
s.append(self.char)
self.char = None
c = self.fp.read(1)
while (c not in b"\r\n") and len(c) and len(b"".join(s).strip(b"\r\n")) <= 255:
s.append(c)
c = self.fp.read(1)
self.char = self.fp.read(1)
# line endings can be 1 or 2 of \r \n, in either order
if self.char in b"\r\n":
self.char = None
return b"".join(s).decode("latin-1")

My version instead reads the bytes into a fixed bytearray. This can be done because we know we can't have a valid line longer than 255 bytes, so we can allocate the space for that at the start and just reuse it for each line. A memoryview is just a wrapper around this byte array that lets us use subsections of it without having to create a copy of that subsection.

Tests/test_file_eps.py Outdated Show resolved Hide resolved
src/PIL/EpsImagePlugin.py Outdated Show resolved Hide resolved
src/PIL/EpsImagePlugin.py Outdated Show resolved Hide resolved
docs/deprecations.rst Outdated Show resolved Hide resolved
@hugovk hugovk changed the title Refactor EpsImagePlugin Refactor EpsImagePlugin and deprecate PSFile Apr 1, 2023
@hugovk hugovk merged commit 48b0be2 into python-pillow:main Apr 1, 2023
3 checks passed
@Yay295 Yay295 deleted the eps_plugin_perf branch April 1, 2023 06:25

if not box:
if not self._size:
self._size = 1, 1 # errors if this isn't set. why (1,1)?
Copy link
Member

Choose a reason for hiding this comment

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

Could you tell me what the "errors" are? I don't see any coming from our test suite - https://github.com/radarhere/Pillow/actions/runs/4582259664

Copy link
Member

Choose a reason for hiding this comment

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

I've created PR #7056 to remove this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked out the first commit in this branch, removed that line, and ran the tests, and there weren't any errors. So I'm not sure what error I had seen back then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants