halostatue / color

Color tools for Ruby.
Other
133 stars 41 forks source link

Correct YIQ conversion, and YIQ#to_rgb #6

Closed bkudria closed 7 years ago

bkudria commented 10 years ago

This patch fixes RGB=>YIQ conversion, and provides YIQ=>RGB conversion. I'd love to replace to_yiq, but this would break existing clients. This seems like a reasonable compromise, but I'm open to other ideas.

You can verify these conversions using http://www.picturetopeople.org/color_converter.html. The Wikipedia article on YIQ also notes these ranges: http://en.wikipedia.org/wiki/YIQ. (I'm using the same numbers you are, just at higher precision.)

In YIQ, only Y is in the 0..1 range, I and Q have a different min/max. In this change:

halostatue commented 10 years ago

Awesome. I'm good with this, but (1) I need tests around this, and (2) most of the comments that @houndci added are things that I would look at reformatting myself.

If the only difference between Color::RGB#to_yiq and Color::RGB#to_correct_yiq is one of precision, then I don't think I've got a problem replacing the function in 1.6 (the next release, coming up soonish). If you prefer, we could do that with a parameter (Color::RGB#to_yiq(precision = :low)) and replace it with your higher precision version unconditionally in Color 2.x.

I'll need to study your use of Matrix—I've already determined that the Color 2.x is going to be 1.9+-only and that I'm going to be using Rational when operating with XYZ and L_a_b* colour spaces (see #5 for code that introduced that), but Matrix will help since everything in colour theory appears to be Matrix math. I think I'll have to learn it again, since I've forgotten almost everything I learned in college mumblety years ago.

Thanks so much for this, and I look forward to pulling this in soon.

bkudria commented 10 years ago

Austin, unfortunately, the difference is not just one of precision. RGB#to_yiq calls YIQ.from_fraction, which calls new, which assumes a 0..1 range and normalizes. But YIQ has a different range defined for I and Q. So negative I and Q values are being truncated. The coefficients we use (mine differ only in precision) assume the standard range, not 0..1, so normalizing to 0..1 is incorrect. My RGB#to_correct_yiq calls YIQ.from_ntsc_fraction, which scales the standard (NTSC) range to a 0..1 range, and passes that in to from_fraction, so the correct colors are preserved.

Unfortunately, YIQ as-is in master is incorrect - the color space is being distorted. Colors with I and Q > 0 are stored correctly, but [I,Q] < 0 are being truncated to 0. This is a breaking change and therefore requires deprecation warnings, a new major version, etc. That said, I can't imagine YIQ usage in the wild is very popular, it's a fairly obscure format as far as I can tell. I used it because I found Y (luma) to be a better representation when making contrasting text than the L (luminance) in HSL.

The Matrix math is pretty straightforward. In RGB#to_correct_yiq, I'm multiplying the coefficients by a vector (a matrix with one column, essentially) containing the RGB components. The result is a vector YIQ. You do the same exact thing manually in to_yiq - compare the numbers in the matrix by the numbers you're using - they're the same, and in the same position. Algebraically, you can see this operation here http://en.wikipedia.org/wiki/Matrix_multiplication#Examples_of_matrix_products under "Square matrix and column vector" - A is the coefficient matrix, B is the RGB vector, and AB is the YIQ vector.

I'll fix the Hound warnings imminently!

halostatue commented 10 years ago

Let me consider how to do this. I really don't want to have bad math out there—sort of the whole point of this library is to be able to trust it. I'm leaning toward asking you to replace the existing YIQ code and the next release that includes this will be 2.0, even though I had different plans for the release I was thinking of as 2.0, including dropping Ruby 1.8 support.

halostatue commented 10 years ago

I've started working toward a release of Color 2.0 in the branch color-2. If we retarget your changes against color-2, you can replace to_yiq and make any other changes you want here.

Sorry it took so long to get back to you on this.