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

Fixed point integer resample #1881

Merged
merged 21 commits into from
May 26, 2016
Merged

Fixed point integer resample #1881

merged 21 commits into from
May 26, 2016

Conversation

homm
Copy link
Member

@homm homm commented May 3, 2016 •
edited
Loading

This change speeds up resampling up to 60% and eliminates ugly i2f hack.

Source Operation Filter Before After
7712×4352 Resize to 16x16 Bilinear 210 330
Bicubic 109 177
Lanczos 73.5 120
Resize to 320x180 Bilinear 154 241
Bicubic 88.6 139
Lanczos 60.4 96.5
Resize to 2048x1155 Bilinear 83.9 105
Bicubic 55.3 75.1
Lanczos 38.9 57.3

I'll add tests for other modes and maybe return old implementation for IMAGING_TYPE_INT32 and IMAGING_TYPE_FLOAT32.

@coveralls
Copy link

coveralls commented May 3, 2016 •
edited
Loading

Coverage Status

Coverage decreased (-0.009%) to 77.621% when pulling 4ac220f on uploadcare:fpi-resample into 12bd44a on python-pillow:master.

@wiredfool
Copy link
Member

What are the units on the performance measurement?

@homm
Copy link
Member Author

homm commented May 3, 2016 via email

@coveralls
Copy link

coveralls commented May 5, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.05%) to 77.684% when pulling ba4a539 on uploadcare:fpi-resample into 12bd44a on python-pillow:master.

@homm
Copy link
Member Author

homm commented May 6, 2016

I split ImagingResampleHorizontal to two versions: for 8 bits per channel with int accumulators and for 32 bits per channel with double accumulator. I have tried to reduce code duplication and make separate ImagingPrecompute function.

New tests really good, because they check the consistency of result after manipulations: there should not be any shifts or dithering. And just for information, none of new tests are passed on master:

======================================================================
FAIL: CoreResampleConsistencyTest.test_32f
----------------------------------------------------------------------
AssertionError: 1.00000011921 != 1.0 for pixel (0, 1)
AssertionError: nan != 3.40282306074e+38 for pixel (0, 0)
AssertionError: 1.17549351004e-38 != 1.17549393043e-38 for pixel (0, 0)
AssertionError: 1.19209289551e-07 != 1.19209303762e-07 for pixel (0, 0)

======================================================================
FAIL: CoreResampleConsistencyTest.test_32i
----------------------------------------------------------------------
AssertionError: 11 != 12 for pixel (0, 0)
AssertionError: -2147483392 != 2147483647 for pixel (0, 0)
AssertionError: -11 != -12 for pixel (0, 0)
AssertionError: -2147483392 != -2147483648 for pixel (0, 0)

With the latest changes (separate ImagingPrecompute) new implementation is about 1% slower than initial for 8bpc and about 10% slower than master for 32bpc: this is a fee for the accuracy. Also, the bug with GCC 4.8 and below is still relevant for I mode, but I think this is the time then we can remove workaround: users who really faced with this problem can update compiler.

Could anyone help me with AppVeyor windows builds? Error message command 'link.exe' failed with exit status 1120 doesn't make any sense and I don't see any other errors.

@wiredfool
Copy link
Member

I'll see what I can do on the windows builds. Probably a compile error, and for some reason we're not getting the traces. The usual compile error is from a declaration not being at the beginning of a block, but I'm not seeing that from a quick scan of the diff.

@wiredfool
Copy link
Member

Error appears to be this: https://ci.appveyor.com/project/wiredfool/pillow/build/3.3.pre.290/job/n525alym7ebxiips#L1369

Resample.obj : error LNK2019: unresolved external symbol lround referenced in function ImagingResampleHorizontal_32bpc
build\lib.win-amd64-2.7\PIL\_imaging.pyd : fatal error LNK1120: 1 unresolved externals

@wiredfool wiredfool mentioned this pull request May 6, 2016
Merged
@wiredfool
Copy link
Member

If you rebase, you'll get the appveyor changes.

@homm
Copy link
Member Author

homm commented May 6, 2016

unresolved external symbol lround

ok, I'll check what we can use in Visual C

@coveralls
Copy link

coveralls commented May 6, 2016 •
edited
Loading

Coverage Status

Changes Unknown when pulling b04e736 on uploadcare:fpi-resample into * on python-pillow:master*.

@coveralls
Copy link

coveralls commented May 9, 2016 •
edited
Loading

Coverage Status

Changes Unknown when pulling c23e897 on uploadcare:fpi-resample into * on python-pillow:master*.

@coveralls
Copy link

coveralls commented May 9, 2016 •
edited
Loading

Coverage Status

Changes Unknown when pulling 0da1564 on uploadcare:fpi-resample into * on python-pillow:master*.

@coveralls
Copy link

coveralls commented May 9, 2016 •
edited
Loading

Coverage Status

Changes Unknown when pulling 233bbb6 on uploadcare:fpi-resample into * on python-pillow:master*.

@coveralls
Copy link

coveralls commented May 9, 2016 •
edited
Loading

Coverage Status

Changes Unknown when pulling 8ca0a5d on uploadcare:fpi-resample into * on python-pillow:master*.

@homm
Copy link
Member Author

homm commented May 9, 2016

LOL, new problems with windows: build hangs up on test-installed.py.

I'm ok with all other changes.

@coveralls
Copy link

coveralls commented May 11, 2016 •
edited
Loading

Coverage Status

Changes Unknown when pulling aaf51e4 on uploadcare:fpi-resample into * on python-pillow:master*.

@homm
Copy link
Member Author

homm commented May 11, 2016 •
edited
Loading

@wiredfool Any thoughts about win builds?

@wiredfool
Copy link
Member

I'll take a look, bot probably not tonight.

@wiredfool
Copy link
Member

Looks like it's this test: https://github.com/python-pillow/Pillow/blob/master/Tests/test_image_filter.py#L39 that is hanging.

See: https://ci.appveyor.com/project/wiredfool/pillow/build/3.3.pre.295/job/wq9ms96g7w1amjvy

The code to change the way the tests run is in wiredfool@55b2c3b, which I'm planning to pr/commit shortly.

@homm
Copy link
Member Author

homm commented May 15, 2016

I found out that the problem is in replacing malloc with calloc. I have no idea why is it happens because calloc is already used in Pillow in many places. Moreover, as I understand, TestImageFilter.test_crash doesn't use resampling and don't call the code with calloc from this PR.

@coveralls
Copy link

coveralls commented May 16, 2016 •
edited
Loading

Coverage Status

Changes Unknown when pulling 757a728 on uploadcare:fpi-resample into * on python-pillow:master*.

@homm
Copy link
Member Author

homm commented May 16, 2016

Returning malloc makes builds green again. I don't understand anything )

wiredfool
wiredfool reviewed May 25, 2016
View reviewed changes
libImaging/Resample.c
return (Imaging) ImagingError_MemoryError();
}

kk = malloc(xsize * kmax * sizeof(int));
Copy link
Member

Choose a reason for hiding this comment

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

Need to check this against INT_MAX for overflow. Also probably convert to calloc.

homm added 20 commits May 26, 2016 02:07
@homm
move checks before mallocs to prevent memory leaks
a5b99dd
@homm
intermediate not normalized buffer for coefficients
0e2a8e4
@homm
pass struct filter to ImagingResampleHorizontal
3ee407e
@homm
extra line
342e6f2
@homm
make coefficients ints
e13297b
@homm
test for consistency
9618ec9
@homm
tests for 32bit modes
e47002d
@homm
separate 8 and 32 bpc implementations
7693da9
@homm
precompute function
023f7ad
@homm
use ImagingPrecompute in ImagingResampleHorizontal_32bpc
04552b0
@homm
unroll 32bpc version
f3f03bc
@homm
use ImagingPrecompute in ImagingResampleHorizontal_8bpc
126c315
@homm
unroll 8bpc version
d48324b
@homm
raise on special cases even if imIn->image8
babaaf9
@homm
implement round
f44a7f8 
fix windows build
@homm
possible increase precision and avoid compiler warnings
d3749cc
@homm
simpler coefficients calculation in ImagingResampleHorizontal_8bpc
7722d00
@homm
change xmax meaning to avoid substructions
eec17c0
@homm
add test for L
555d2ac
@homm
return malloc
1c3def1
@coveralls
Copy link

coveralls commented May 25, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.01%) to 77.708% when pulling 1c3def1 on uploadcare:fpi-resample into 73c8140 on python-pillow:master.

@coveralls
Copy link

coveralls commented May 25, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.01%) to 77.708% when pulling 5ffd9e5 on uploadcare:fpi-resample into 73c8140 on python-pillow:master.

@homm
Copy link
Member Author

homm commented May 26, 2016

@wiredfool Done. Also rebased against master to pass the latest resampling accuracy tests.

@wiredfool wiredfool merged commit 0cfcc9e into python-pillow:master May 26, 2016
@homm homm deleted the fpi-resample branch May 26, 2016 12:13
@homm homm mentioned this pull request Aug 13, 2017
Merged
@radarhere radarhere mentioned this pull request May 6, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@homm @coveralls @wiredfool