| msg155915 - (view) |
Author: Brian Landers (packetslave) |
Date: 2012-03-15 17:19 |
| There doesn't seem to be a standard definition for the constants to use when doing the matrix calculations to convert RGB to YIQ or vise versa. Also, the current colorsys library uses two digits of precision for RGB-YIQ but six digits for YIQ-RGB. The attached patch standardizes both functions to use the same constants as Matlab, using the same 3 digits of precision. This makes a roundtrip of RGB->YIQ->RGB return the original values (for 3 digits of precision). Also added tests, which brings colorsys.py to 100% coverage. |
|
|
| msg156090 - (view) |
Author: Terry J. Reedy (terry.reedy) *  |
Date: 2012-03-16 22:02 |
| The idea seems reasonable. Do you have a link or reference to a Matlab doc with the coefficients? Enhancements only go in new versions. |
|
|
| msg156098 - (view) |
Author: Brian Landers (packetslave) |
Date: 2012-03-16 23:09 |
| Matlab docs are here: - http://www.mathworks.com/help/toolbox/images/ref/rgb2ntsc.html - http://www.mathworks.com/help/toolbox/images/ref/ntsc2rgb.html Should these be referenced in the source itself? re new versions: sure, I'll create a separate patch for adding the tests to the maintenance releases so there's coverage there, too. Should that have a new bug as well? |
|
|
| msg156131 - (view) |
Author: Terry J. Reedy (terry.reedy) * |
Date: 2012-03-17 04:01 |
| Yes, put reference in source. I checked that you copied correctly. I think there is some controversy, which I do not understand, about adding tests to maintenance releases without a bug fix. I will ask, so do not do it yet. |
|
|
| msg156132 - (view) |
Author: Terry J. Reedy (terry.reedy) * |
Date: 2012-03-17 04:04 |
| Mark, I know you have worked on numerical algorithms. 1. Do you agree that this is a reasonable change? 2. Should new tests go in maintenance release? |
|
|
| msg156133 - (view) |
Author: Brian Landers (packetslave) |
Date: 2012-03-17 04:16 |
| Updated to add Matlab refs, also added a roundtrip RGB-YIQ-RGB test to match the other conversions. |
|
|
| msg158893 - (view) |
Author: Mark Dickinson (mark.dickinson) * |
Date: 2012-04-20 21:24 |
| Terry: sorry, I missed this before. Re 1. Sure, this seems reasonable, if there's a real sense in which the new numbers are better than the old. Besides MATLAB, there's also a set of numbers given on Wikipedia that might be considered. I don't have the domain knowledge to know what's sensible here. I *would* rather see the inverse transformation keep the full 6 digits of precision, though. Or perhaps even give the inverse to full float precision. Without that, the result of roundtripping RGB -> YIQ -> RGB could be significantly (perhaps even visibly) different from the original. I don't think it's acceptable for the roundtrip error to increase significantly w.r.t. Python 3.2. Re 2. For this issue, I don't see a real benefit to adding the tests to the maintenance releases. No strong opinions, though. |
|
|
| msg179323 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2013-01-08 09:16 |
| According to Wikipedia FCC conversion is defined as: y = 0.30*r + 0.59*g + 0.11*b i = 0.5990*r - 0.2773*g - 0.3217*b q = 0.2130*r - 0.5251*g + 0.3121*b and non-FCC conversion is defined as: y = 0.299*r + 0.587*g + 0.114*b i = 0.595716*r - 0.274453*g - 0.321263*b q = 0.211456*r - 0.522591*g + 0.311135*b Our current code y = 0.30*r + 0.59*g + 0.11*b i = 0.60*r - 0.28*g - 0.32*b q = 0.21*r - 0.52*g + 0.31*b looks like FCC conversion with the precision of two decimal places. Actually with this precision the difference between the different conversions are almost absent. |
|
|
| msg194433 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2013-08-04 22:23 |
| Here is a patch which implements the FCC version of RGB/YIQ conversion. |
|
|
| msg194445 - (view) |
Author: Terry J. Reedy (terry.reedy) * |
Date: 2013-08-05 02:08 |
| Can you add a reference for the coefficients? I believe def test_main(): test.support.run_unittest(ColorsysTest) if __name__ == "__main__": test_main has been and is being replaced in other test files with if __name__ == "__main__": unittest.main and should be here. This should get a short What's New entry in the library section, something like colorsys: "The number of digits in the coefficients for the RGB -- YIQ conversions have been expanded so that they match the FCC NTSC versions. The change in results should be less than 1% and may better match results found elsewhere." (You claim about the current rounding is not exactly correct. While .28*g rounds .277 rather than .274, the current .52*g rounds the non-FCC .523 rather than the FCC .5251. So I avoided making the claim in the suggested entry. It is not important.) |
|
|
| msg194460 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2013-08-05 08:20 |
| > Can you add a reference for the coefficients? I have only link to Wikipedia which refers to Code of Federal Regulations ยง73.682. This link (http://en.wikipedia.org/wiki/YIQ) already mentioned at the top of the file. > (You claim about the current rounding is not exactly correct. While .28*g rounds .277 rather than .274, the current .52*g rounds the non-FCC .523 rather than the FCC .5251. So I avoided making the claim in the suggested entry. It is not important.) A sum of coefficients in this line should be 0 (Q=0 for R=G=B). Patch updated. I added a What's New entry and update to use of unittest.main(), rewrite rgb_to_yiq() in the form as in Wikipedia (it uses less multiplications) and write coefficients in yiq_to_rgb() with maximal precision (as calculated with Python). |
|
|
| msg194481 - (view) |
Author: Ezio Melotti (ezio.melotti) * |
Date: 2013-08-05 14:15 |
| LGTM. |
|
|
| msg194504 - (view) |
Author: Terry J. Reedy (terry.reedy) * |
Date: 2013-08-05 19:06 |
| I agree. Go ahead and push. |
|
|
| msg194507 - (view) |
Author: Mark Dickinson (mark.dickinson) * |
Date: 2013-08-05 19:38 |
| LGTM too. |
|
|
| msg194531 - (view) |
Author: Roundup Robot (python-dev)  |
Date: 2013-08-06 08:52 |
| New changeset 80e9cb6163b4 by Serhiy Storchaka in branch 'default': Issue #14323: Expanded the number of digits in the coefficients for the http://hg.python.org/cpython/rev/80e9cb6163b4 |
|
|