nfnt / resize

Pure golang image resizing
ISC License
3.02k stars 321 forks source link

support YCbCr subsample ratios 4:1:0 and 4:1:1 #51

Closed unixpickle closed 6 years ago

unixpickle commented 8 years ago

See also #37 and #39.

This fixes a panic() for a few JPEG images found in the wild. Without this fix, a program which downloads and resizes tons of images from a source like ImageNet will eventually panic unexpectedly upon finding a 4:1:0 or 4:1:1 image.

To test this change, I have attached a 4:1:0 image found in the wild (from ImageNet, in fact). I zipped the image to prevent any potential image compression from Github (which would presumably destroy the 4:1:0-ness of the image).

YCbCr_410.jpg.zip

I could not find a 4:1:1 in the wild (although I only searched for several minutes). To address this, I have attached a small Go program which can convert an image in-memory to a 4:1:1 image, then resize said image. I could not use this program to generate a 4:1:1 image file because Go's jpeg package converts 4:1:1 into 4:2:0 (as of Go 1.7)

411_test.go.zip

unixpickle commented 8 years ago

(See update below, older versions of Go now supported)

This comment is no longer true, except for the possible solution / update

Note: this pull request will not work on older versions of Go which don't support 4:1:0. Accepting this pull request would mean removing backwards compatibility with older Go versions, unless some extra build voodoo is done (e.g. separate .go files for different Go versions). Hence the Travis CI failures.

Possible solution: if we converted ycbcr.SubsampleRatio to a string using it's String method, we could check that string against "YCbCrSubsampleRatio410" without breaking backwards compatibility with Go1.1. Although older versions of Go wouldn't support 4:1:0, the resize package would still compile and run on those versions while supporting 4:1:0 in newer versions.

Update: I used the String() method like I described above. This change now passes on older versions of Go, while providing 4:1:0 support on newer versions.

unixpickle commented 8 years ago

This does part of what #39 does, but without needing to duplicate two .go files (one for Go1.4 and earlier).

nfnt commented 8 years ago

I'm okay with dropping support for older versions of Go, but only after we're certain that using build constraints in go/build isn't an option. IMO that's the superior solution, because we can keep backwards compatibility. Unfortunately I couldn't really look into it too deeply, but will soon(-ish). Code duplication is something we should avoid as much as possible, the current code is already (for performance reason) a bad example. Want to figure out if that's something that would work with build constraints. If it doesn't, i.e. having build constraints would mean a lot of code duplication, I'd be okay to merge this PR.

unixpickle commented 8 years ago

@nfnt Actually, this no longer requires Go >1.1. If you note my last comment, I now use the Stringer interface to avoid symbols which are undefined in earlier versions of Go. Any code duplication could be easily avoided, but the only code duplication was the result of an already-duplicated switch statement in the master branch.

nfnt commented 8 years ago

Ah, sorry, then I misunderstood what you wrote. That's great! So it's only some tests that'll fail. Would be even greater when we could apply build constraints on these few test cases. Don't know if that's possible though.

unixpickle commented 8 years ago

@nfnt all tests pass. I even added tests that use the Stringer interface, that way they can test the new functionality on newer versions of Go.

rkravchik commented 7 years ago

@nfnt say, pls, when will you merge this PR or Charlie Vieth solution (with build tags) to solve 411 problem?

charlievieth commented 7 years ago

@rkravchik thanks for the reminder, I submitted a PR https://github.com/nfnt/resize/pull/57 that uses build tags to add support for YCbCrSubsampleRatio411 and YCbCrSubsampleRatio410 and maintains backwards compatibility with older versions of Go.

rkravchik commented 7 years ago

@charlievieth i saw your repo and just wanted to remind @nfnt to make a choice between two solutions (your and @unixpickle ) soon. P.S. I have forked this repo and merged with your's: https://github.com/rkravchik/resize It is temporary solution for me now.

nfnt commented 6 years ago

Closed by #57.