Skip to content

Replace resize method#997

Merged
wiredfool merged 11 commits intopython-pillow:masterfrom
homm:replace-resize
Nov 27, 2014
Merged

Replace resize method#997
wiredfool merged 11 commits intopython-pillow:masterfrom
homm:replace-resize

Conversation

@homm
Copy link
Member

@homm homm commented Nov 9, 2014

Ok, that is why we all there.

Affine-based im.resize implementation should be replaced with convolution-based im.stretch for Bilinear and Bicubic filters (Lanczos was always there). im.stretch uses all pixels from source image for downscale (actually it uses each source pixel in several destination pixels depend of filter), while im.resize uses only fixed amount of pixels for each destination pixels (2x2 for bilinear and 4x4 for bicubic).

Examples:
unsplash2 320 bc r unsplash2 320 bc

unsplash1 320 bc r unsplash1 320 bc

This also fixes #986, #964, and possible other related bugs. im.stretch also faster for upscaling (especially on Bicubic).

As im.resize and im.stretch is part of python API (although not documented), we can't just remove one of them. Also im.stretch has own Nearest implementation, which is Nearest for upscaling and supersampling for downscaling. Therefore I suggest to leave both functions, replace im.resize implementation to convolution-based for all filter except Nearest and use ImagingTransformAffine for Nearest.

@homm
Copy link
Member Author

homm commented Nov 9, 2014

Oh, I don't even run the tests. Let's discuss first.

@hugovk
Copy link
Member

hugovk commented Nov 9, 2014

I recommend enabling Travis (and perhaps Coveralls) on your own repo.

See also https://github.com/python-pillow/Pillow/blob/master/CONTRIBUTING.md

@homm
Copy link
Member Author

homm commented Nov 11, 2014

@wiredfool Any thoughts?

@wiredfool
Copy link
Member

I'm thinking about the interfaces.

We have very similar imaging.c:_resize and imaging.c:_stretch functions. At this point, I don't think that there should be a distinction between them. If there is a distinction, it's lost on me.

Nothing in Pillow calls imaging.c:_stretch anymore. We should drop that function. If we wish to retain API compatibility, we should point _imaging.c:3030 {"stretch", (PyCFunction)_stretch, 1}, at _resize instead. Alternately, we could make _stretch just wrap _resize and issue a deprecation warning. I consider _imaging.c to be essentially a private api, so I'd be willing to just drop it completely.

I think that the lower level calls in Antialias.c should be called ImagingResize*, instead of ImagingStretch*. That way, the calls all the way down the stack are resize, and there's no trace of stretch anywhere in the code.

Other than that, it looks good to me.

@homm
Copy link
Member Author

homm commented Nov 13, 2014

Distinction in NEAREST method only. _stretch produces supersampling for downscaling instead of real NEAREST. This should be a bit faster than BILINEAR (that is why someone can use it), but much slower than separate supersampling implementation can be. I suggest to leave two versions in 2.7 and remove _stretch in 3.0. What is the best way to produce a warning from C code?

homm added 4 commits November 19, 2014 03:03
type switch default case was never achieved because special
images is in image8 pointers, not in image32
mark im.stretch as deprecated.
@homm
Copy link
Member Author

homm commented Nov 19, 2014

@wiredfool I've fixed test a bit and make stretch point to _resize.

What you think about renaming ImagingStretch to ImagingResample and Antialias.c to Resample.c? Because resample is exactly what that happening. In opposite, resize is a method who can choose what to do: make a resample, or fast affine transforms.

@wiredfool
Copy link
Member

Resample sounds like a good rename. Will look over the code after dinner.

@wiredfool
Copy link
Member

Ok, Good to 41029f0.

@homm
Copy link
Member Author

homm commented Nov 19, 2014

renamed

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) when pulling bc0f896 on homm:replace-resize into e0b94d6 on python-pillow:master.

@wiredfool
Copy link
Member

Looks good to me to bc0f896. Anything else to do?

@homm
Copy link
Member Author

homm commented Nov 19, 2014

@wiredfool No, I suppose.

wiredfool added a commit that referenced this pull request Nov 27, 2014
@wiredfool wiredfool merged commit e16ee15 into python-pillow:master Nov 27, 2014
@homm homm deleted the replace-resize branch April 27, 2016 21:42
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.

Image.resize() incorrectly centers non-nearest filters

4 participants