Skip to content
This repository was archived by the owner on Sep 11, 2022. It is now read-only.

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

Closed
wants to merge 3 commits into from

Conversation

unixpickle
Copy link

@unixpickle unixpickle commented Sep 7, 2016

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

This fixes a panic() occurring on 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 (http://image-net.org) will eventually panic unexpectedly upon finding a 4:1:0 image.
@unixpickle
Copy link
Author

unixpickle commented Sep 7, 2016

(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
Copy link
Author

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

This is a YCbCr ratio that was not present in older versions of Go, but which is now supported. As a result, it is necessary to check for 4:1:1 in order to avoid a potential panic().
@unixpickle unixpickle changed the title add support for YCbCr subsample ratio 4:1:0 support YCbCr subsample ratios 4:1:0 and 4:1:1 Sep 7, 2016
@nfnt
Copy link
Owner

nfnt commented Sep 21, 2016

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
Copy link
Author

@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
Copy link
Owner

nfnt commented Sep 21, 2016

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
Copy link
Author

@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
Copy link

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

@charlievieth
Copy link
Contributor

@rkravchik thanks for the reminder, I submitted a PR #57 that uses build tags to add support for YCbCrSubsampleRatio411 and YCbCrSubsampleRatio410 and maintains backwards compatibility with older versions of Go.

@rkravchik
Copy link

@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.

Junichi-Hosoya added a commit to veltra/resize that referenced this pull request Jun 13, 2017
Since some JPEG file paste processing panics, we merged the original pull request.
@nfnt
Copy link
Owner

nfnt commented Feb 21, 2018

Closed by #57.

@nfnt nfnt closed this Feb 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants