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

Resample vertical pass #1933

Merged
merged 10 commits into from
Jun 8, 2016

Conversation

homm
Copy link
Member

@homm homm commented May 28, 2016

This PR implements separate vertical pass of resampling instead of two transposes.

Originally I've replaced two passes with transpose for the following reasons:

  1. It was harder to heavily change the code in two branches simultaneously.
  2. It makes downscaling faster, although upscaling became slightly slower.

Since then there are many tests for resampling were written and many optimizations were implemented. Now it is time to return upscaling performance.

Result on aws ec2 c4.large with 1920×1280 RGB image. Megapixels of source image per second.

Destination Filter 3.2 FPI Vertical pass
Downscaling 16x16 Bilinear 229.31 381.97 386.89
Bicubic 120.64 203.59 204.98
Lanczos 81.33 135.74 136.20
320x180 Bilinear 133.08 161.96 193.30
Bicubic 77.00 109.46 123.08
Lanczos 51.36 77.48 83.27
Upscaling 2048x1155 Bilinear 25.78 27.81 40.14
Bicubic 20.60 23.75 31.83
Lanczos 17.04 19.50 25.00
5120x2880 Bilinear 5.55 5.74 8.91
Bicubic 4.54 4.98 7.25
Lanczos 3.80 4.33 5.93

I also include results of stable version and fixed-point separetely to show what FPI doesn't affect upscaling too much, while vertical pass does. Vertical pass faster for upscaling in average by 40% and also makes downscaling to reasonable sizes 10% faster in average.

This last two improvements together makes all resampling operations about 60% faster.

@coveralls
Copy link

coveralls commented May 28, 2016

Coverage Status

Changes Unknown when pulling f09067e on uploadcare:resample-vertical-pass into * on python-pillow:master*.

@@ -40,6 +40,7 @@ def getmode(mode):
_modes[m] = ModeDescriptor(m, bands, basemode, basetype)
# extra experimental modes
_modes["LA"] = ModeDescriptor("LA", ("L", "A"), "L", "L")
_modes["La"] = ModeDescriptor("La", ("L", "a"), "L", "L")
Copy link
Member Author

Choose a reason for hiding this comment

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

This required for Image.merge('La', ...). But there is no 'RGBa', so I used 'RGBX', which is. Modes support in Pillow is a mess ((

Copy link
Member Author

Choose a reason for hiding this comment

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

@wiredfool If you don't mind about adding La in _modes, I'd like to merge it. I don't see any other controversial points in this RP.

Copy link
Member

Choose a reason for hiding this comment

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

I went through the review, hit this, started wondering why we didn't have RGBa as a mode here, and then determined that modes are a mess.

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 can add RGBa here as a temporary solution.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added.

Copy link
Member

Choose a reason for hiding this comment

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

works for me

@homm
Copy link
Member Author

homm commented May 28, 2016

As a bonus: It will be easier for me to maintain Pillow-SIMD, because Pillow-SIMD does two separate passes from the start.

@homm homm mentioned this pull request Jun 3, 2016
@coveralls
Copy link

coveralls commented Jun 3, 2016

Coverage Status

Changes Unknown when pulling 3b7923c on uploadcare:resample-vertical-pass into * on python-pillow:master*.

@coveralls
Copy link

coveralls commented Jun 3, 2016

Coverage Status

Changes Unknown when pulling c826266 on uploadcare:resample-vertical-pass into * on python-pillow:master*.

@coveralls
Copy link

coveralls commented Jun 8, 2016

Coverage Status

Changes Unknown when pulling 12c8cf9 on uploadcare:resample-vertical-pass into * on python-pillow:master*.

@wiredfool
Copy link
Member

Are you ready for this to be merged?

@homm
Copy link
Member Author

homm commented Jun 8, 2016

Yes

@wiredfool wiredfool merged commit 62551a8 into python-pillow:master Jun 8, 2016
@homm homm deleted the resample-vertical-pass branch June 8, 2016 13:15
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