Skip to content

Add transpose and speedup rotation#994

Merged
wiredfool merged 8 commits intopython-pillow:masterfrom
homm:add-transpose
Nov 7, 2014
Merged

Add transpose and speedup rotation#994
wiredfool merged 8 commits intopython-pillow:masterfrom
homm:add-transpose

Conversation

@homm
Copy link
Member

@homm homm commented Nov 7, 2014

This adds new Imaging.TRANSPOSE method for Image.transpose() operation. Also this speeds up Image.ROTATE_90 and Image.ROTATE_270.

Method Before After
Large 5120x2880 image
Flip left right 0.02635 0.0262
Flip top bottom 0.01105 0.011
Rotate 90 0.3706 0.04535
Rotate 180 0.02968 0.02933
Rotate 270 0.3786 0.04542
Transpose 0.04404
Small 586x387 image
Flip left right 0.0003951 0.0003891
Flip top bottom 0.0001121 0.000108
Rotate 90 0.000602 0.000457
Rotate 180 0.000464 0.000463
Rotate 270 0.0005708 0.0004828
Transpose 0.0004091

As you can see, this doesn't improve performance for relatively small images, because they whole fit in cache, but for large images this gives 8x speedup.

Tests pending.

Copy link
Member

Choose a reason for hiding this comment

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

Rewording to native english. I think this is what you mean.

For large images rotation is an inefficient operation in terms of CPU cache.
One row in the source image affects each column in destination.
Rotating in chunks that fit in the cache can speed up rotation 8x on a modern CPU.
A chunk size of 128 requires only 65k of CPU cache and is large enough
that the overhead from the extra loops are not apparent.

@wiredfool
Copy link
Member

Looks good to me.

  • The one API change is documented in the docstring.
  • I've noted one comment anglicization.
  • I want to check to see how well we're testing this code prior to merging, and possibly add tests to make sure that we're matching the original.

Question: Merge order of this vs the other ones? After #977 ?

update comment
@homm
Copy link
Member Author

homm commented Nov 7, 2014

Have added tests. Merge order doesn't matter.

Copy link
Member Author

Choose a reason for hiding this comment

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

You really need change standard test image size :) #947

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it would be good, but it took quite a while to regenerate all the test images. (If we do decide to re-generate them, I tried to mention how they were generated in the commit messages.) We should at aim for any new non-hopper test images committed to meet the #947 requirements.

You could always open a different, non-square test image, or do a resize/crop before continuing.

@homm
Copy link
Member Author

homm commented Nov 7, 2014

Update tests with not square image and fix bugs in tests related to this!

@hugovk
Copy link
Member

hugovk commented Nov 7, 2014

Great, non-square test images are definitely the way forward!

Do you think you could add some error tests to cover these lines:
https://coveralls.io/files/345689512#L150 - 153

And the other similar ones in the same file if you like.

@hugovk hugovk removed the Needs Tests label Nov 7, 2014
@homm
Copy link
Member Author

homm commented Nov 7, 2014

I think this is impossible, because those functions are used only from '_rotate', which creates temporary images with right mode and size.

@hugovk
Copy link
Member

hugovk commented Nov 7, 2014

Sure, no problem. I'll let wiredfool have a look over it before merging.

Thanks for all these improvements!

wiredfool added a commit that referenced this pull request Nov 7, 2014
Add transpose and cache aware rotation
@wiredfool wiredfool merged commit cfbe49f into python-pillow:master Nov 7, 2014
@homm homm deleted the add-transpose branch April 27, 2016 21:42
@homm homm mentioned this pull request Sep 11, 2017
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.

3 participants