Thanks Phil and Jim for your review. -----Original Message----- From: Philip Race Sent: Friday, May 06, 2016 4:17 AM To: Jayathirth D V Cc: Jim Graham; 2d-dev@openjdk.java.net Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7116979 : Unexpected pixel colour when converting images to TYPE_BYTE_INDEXED
Looks good to me. -phil. On 5/4/16, 10:46 PM, Jayathirth D V wrote: > Hi, > > Gentle reminder. Please review. > > Thanks, > Jay > > -----Original Message----- > From: Jayathirth D V > Sent: Monday, May 02, 2016 4:05 PM > To: Jim Graham; Philip Race > Cc: 2d-dev@openjdk.java.net > Subject: RE: [OpenJDK 2D-Dev] Review Request for JDK-7116979 : > Unexpected pixel colour when converting images to TYPE_BYTE_INDEXED > > Hi Jim, > > Thanks for your valuable review. > I verified what you have mentioned and we are doing the same while mapping > colormap to InverseLUT. > I also tried populating InverseLUT serially from 0 to 255 index and with this > change for white color also test is passing because we are selecting 215 > index for white instead of 255. > > But I have to figure out a way for calculating what should be the ideal > colormap index we should store in InverseLUT in INSERTNEW macro. > For this I have raised separate bug : > https://bugs.openjdk.java.net/browse/JDK-8155814 . > > After fixing new bug I will add testing of white color in JDK-7116979. I > think we can check-in present changes and work on new bug. Please let me know > your inputs. > > Thanks, > Jay > > -----Original Message----- > From: Jim Graham > Sent: Monday, May 02, 2016 3:46 AM > To: Jayathirth D V; Philip Race > Cc: 2d-dev@openjdk.java.net > Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7116979 : > Unexpected pixel colour when converting images to TYPE_BYTE_INDEXED > > Ugh. I took a look at the inverse colormap creation code and believe I see > the problem. It first takes all of the colors in the colormap and inserts a > backmapping from their index in the "inverse cube" back to the color in the > colormap. If an entry already has an index assigned, it skips it, even if > the new color is a better match for the cell in the inverse cube than the > index it already contains. This isn't a simulation of "take each entry in > the cube and pick the best color in the map for it". The INSERT macro should > check which of the two options are better in the case where the cell already > has an index assigned. > > For the default colormaps, WHITE seems to be the color that takes a hit, but > other custom colormaps could end up with poor choices for a lot of different > colors, particularly the primaries. If you test with a colormap that has, > for instance: primaries with +/- 7, followed by the pure primaries, none of > the primaries will be filled well. > > Note that one silly ideosyncracy of the way the loop is encoded in the above > case causes the problem for white. It simultaneously iterates the color cube > from 0 and from 255 towards the middle, which means we handle BLACK from the > rgb cube first and it wins the battle for its cell, but when we process WHITE > at the end of the rgb cube, we've already processed a "very close gray" from > the top of the colormap before it and that non-WHITE color won the battle for > the WHITE cell. I have no idea why the loop was written that way, though. I > can't imagine that it would help performance much in any case and that seems > to be the only reason to do it that way...? > > This should be filed as a separate bug as it will otherwise cause us to use > less than effective colors in our dithering for reasons beyond this current > case of whether or not white maps well. Fixing it would also allow us to > test WHITE in this test case... > > ...jim > > On 4/30/2016 12:59 AM, Jayathirth D V wrote: >> Hi Jim, >> >> As I have mentioned in my previously, It looks like we take index from >> (31,31) in cube for white color which is gray value increment from color-map >> in 255 index. >> >> "I have observed that in case of white color we are picking (252,252,252) >> and not (255,255,255) from color-map. What I have observed that out of 256 >> places we are storing RGB colors at start and then gray values. So we are >> picking final gray value and not in between white value. That's why I am not >> verifying white color validity in test case." >> >> There is no variable to find out exactly until where we are storing RGB >> colors in color-map as it can vary if user provides custom color-map. This >> looks like completely different issue of not extracting proper value for >> white color. >> >> Thanks, >> Jay >> >> -----Original Message----- >> From: Jim Graham >> Sent: Saturday, April 30, 2016 4:43 AM >> To: Jayathirth D V; Philip Race >> Cc: 2d-dev@openjdk.java.net >> Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7116979 : >> Unexpected pixel colour when converting images to TYPE_BYTE_INDEXED >> >> The code changes look good. >> >> One question on the test case, though - why is Color.WHITE commented out in >> the test case? Was it failing? >> >> ...jim >> >> On 4/29/16 4:45 AM, Jayathirth D V wrote: >>> Hi, >>> >>> Thanks Phil& Jim for your inputs. >>> I have made all recommended changes. >>> Please find updated webrev for review : >>> http://cr.openjdk.java.net/~jdv/7116979/webrev.02/ >>> >>> Thanks, >>> Jay >>> >>> -----Original Message----- >>> From: Phil Race >>> Sent: Tuesday, April 26, 2016 11:56 PM >>> To: Jim Graham >>> Cc: Jayathirth D V; 2d-dev@openjdk.java.net >>> Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7116979 : >>> Unexpected pixel colour when converting images to TYPE_BYTE_INDEXED >>> >>> On 04/22/2016 06:56 PM, Jim Graham wrote: >>>> Hi Jay, >>>> >>>> It looks like it will work, but there are some inconsistencies in >>>> the code that we should address and a couple of optimizations. >>>> >>>> First, there is a mixture of using "int" and "jboolean" for the >>>> type of the new boolean parameter. It would be great if we could >>>> just declare it as jboolean in the structures, but it looks like >>>> those are limited to basic C types. It seems a little clunky to >>>> mix types like this, but I'd be interested in hearing what Phil >>>> says. I think it would be fine to just use int throughout. >>> From the ones I've seen may be much better to just use int >>> throughout. Eg >>> >>> 264 static jboolean calculatePrimaryColorsAprroximation(int* >>> cmap, unsigned char* cube, int cube_size) { >>> >>> aside from having a typo in the name also makes a fair amount of internal >>> uses of jboolean and JNI_TRUE/JNI_FALSE. >>> >>> It then does this >>> >>> 385 cData->representsPrimary = >>> calculatePrimaryColorsAprroximation(pRgb, cData->img_clr_tbl, 32); >>> >>> ie stores it in an int .. I see no reason for this as there is no JNI code >>> involved. >>> >>> >>> Moreover this usage of the variables declared as boolean then has >>> code like >>> >>> 174 (representsPrimary == 1))) { \ >>> >>> >>> -phil. >>> >>>> I'd suggest a name change: >>>> representsPrimary => representsPrimaries (since there are >>>> multiple primary colors) >>>> >>>> The place to load the variable for testing in the ByteIndexed >>>> macros would be in the "InitByteIndexedStoreVarsY" macro where it >>>> sets up the "PREFIX ## InvLut" variable. It should also use a >>>> PREFIX. Look through those macros for wherever the InvLut value is >>>> declared and initialized, and follow a similar pattern for "PREFIX >>>> ##<somename>". >>>> You could use "representsPrimaries" for the name here too, but it >>>> could also be shortened to just "repPrims" or "optPrims" since the >>>> macros are all centralized here. >>>> >>>> Once you do the preceding step, you can delete a lot of lines that >>>> pre-load the "representsPrimaries" in the calling macros, which >>>> should also eliminate a lot of files from the webrev. >>>> >>>> In the code in ByteIndexed.h:StoreByteIndexedFrom3ByteRgb() that >>>> tests the variable, I'd just test "representsPrimary" with no "==" >>>> rather than comparing it to 1 since it is a boolean, not (really) an >>>> integer. >>>> >>>> With respect to the new function that tests the primary matches (in >>>> ByteIndexed.c), some of the code is unnecessarily complicated: >>>> >>>> - When you find a color match failure (lines 301, et al), you could >>>> just return false directly. As it stands, you set a variable and >>>> break, but the break only breaks out of 1 of 3 nested if >>>> statements, so it doesn't save any calculations. Just return false >>>> directly as a single failure means the answer is "no". >>>> >>>> - your r, g, and b values are extracted from the color in the wrong >>>> order near line 285. r involves a shift of 16, and b involves no >>>> shift. I suppose this is paired with computing the dr and db >>>> values with the wrong indices or this technique wouldn't work, but >>>> it is clunky to name the variables inconsistently with the data >>>> they actually hold. >>>> >>>> - you use parentheses on the left-hand side of an assignment when >>>> you extract the r, g, and b values (also near line 285). You don't >>>> need parentheses on that side of an assignment operator. >>>> >>>> There are several things that are more complicated than they need >>>> to be with your testing lines. They use a lot more computations >>>> than you need. For instance: >>>> >>>> - You test the i,j,k using a modulus operation (lines 298, 312, >>>> 326), but really you just need to know if it is 0 or not-0, so just >>>> test it for ==0 or !=0. >>>> - rather than using multiplication and modulus to assign the >>>> dr,dg,db values near line 291, why not just use "dr = (i == 0) ? 0 : 255;"? >>>> - the tests for range use a separate subtraction and a compare, >>>> when they could be inlined. >>>> >>>> First, I'd get rid of the "represents_primary" variable entirely >>>> and then rewrite the whole tail end of the loop body from 291 to >>>> the end of the loop as: >>>> >>>> if (i == 0) { >>>> if (r> delta) return false; >>>> } else { >>>> if (r< 255-delta) return false; } // 2 more similar tests for >>>> j/g and k/b >>>> >>>> Then "return true" at the end of the function if it reaches there. >>>> Keep in mind that the mapping of ijk to rgb might be affected if >>>> you fix the shifts used to extract the rgb components from color... >>>> >>>> ...jim >>>> >>>> On 4/20/2016 2:46 AM, Jayathirth D V wrote: >>>>> Hi Jim, >>>>> >>>>> As discussed we will not add dithering error values to primary >>>>> colors with color map which represents Primary colors >>>>> approximately(+/-5 delta). >>>>> I have made changes based on this suggestion and added new >>>>> function to calculate whether color map represents primary colors >>>>> approximately or not. >>>>> If it represents only then we will avoid adding dithering error >>>>> values in case ByteIndexed destination. >>>>> >>>>> I have observed that in case of white color we are picking >>>>> (252,252,252) or (246,246,246) and not (255,255,255) from colormap. >>>>> What I have observed that out of 256 places we are storing RGB >>>>> colors at start and then gray values. So we are picking final gray >>>>> value and not in between white value. That's why I am not >>>>> verifying white color validity in test case. >>>>> >>>>> Please find updated webrev for review : >>>>> http://cr.openjdk.java.net/~jdv/7116979/webrev.01/ >>>>> >>>>> Thanks, >>>>> Jay >>>>> >>>>> -----Original Message----- >>>>> From: Jim Graham >>>>> Sent: Saturday, February 20, 2016 3:02 AM >>>>> To: Jayathirth D V >>>>> Cc: 2d-dev@openjdk.java.net; Philip Race; Prasanta Sadhukhan >>>>> Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7116979 : >>>>> Unexpected pixel colour when converting images to >>>>> TYPE_BYTE_INDEXED >>>>> >>>>> Hi Jayathirth, >>>>> >>>>> Thank you for attaching the detailed logs, but I don't have a >>>>> program that can deal with the 7z files. Can you summarize what >>>>> was learned from them? >>>>> >>>>> Also, the colormap you created, while subset, was a perfectly >>>>> scaled cube. I was referring more to a random colormap where you >>>>> might have >>>>> 2 colors close to a primary, but neither is on a line from the >>>>> primary to the 0.5 gray value so each has a different hue. That's >>>>> something that can only be evaluated visually. One example of >>>>> randomized colormaps is when we display via X11 onto an 8-bit >>>>> display. That probably never happens any more, but we may not be >>>>> able to allocate colors in the colormap in such a case and if >>>>> another program has already allocated most of the dynamic colormap >>>>> then we would not be able to add our own colors. >>>>> >>>>> This could be dealt with by simply marking our standard colormap >>>>> as having primaries, or by scanning any given colormap and marking >>>>> it if it has primaries, and then only perform this code on >>>>> colormaps with the primaries. Solving it for our own colormap (or >>>>> solving it for any colormap that has all primaries) would likely >>>>> solve it for 99% of the vanishingly small number of developers who might >>>>> run into this. >>>>> >>>>> But, in the end, what would we accomplish by adding this one very >>>>> targeted fix? >>>>> >>>>> I'm also curious where the push for this is coming from. While we >>>>> aren't perfect here, so much of the world today is focused on >>>>> lossless image formats that I don't imagine there is much need for >>>>> really good 8-bit indexed image support any more...? >>>>> >>>>> If anything, spatial dithering is considered a very low quality >>>>> algorithm and error propagation dithering would handle all of the >>>>> colormaps much better. We used to do error propagation dithering >>>>> back in the JDK 1.1 days, but we dropped it when we introduced 2D >>>>> because we added a bunch of ways to render small pieces of an >>>>> image and only the current spatial dithering algorithm can be >>>>> started in the middle of an operation. That doesn't mean that you >>>>> can't still do error propagation since many operations already >>>>> operate on the full image any way and you can still handle subset >>>>> image operations by either converting the full image first and >>>>> then copying or by tracing the errors from the edge of the image >>>>> to the point of the subimage operation. We never bothered to >>>>> upgrade our algorithms it just never seemed to be work that made >>>>> sense to prioritize given that screens were overwhelmingly moving >>>>> to full-color at the time of "Java 2" (and now to HDR in 2016+). 8-bit >>>>> indexed isn't used much any more. >>>>> >>>>> If we want to make 8-bit destination images work better we'd be >>>>> better off adopting error propagation dithering again rather than >>>>> trying to tune this algorithm for black. >>>>> >>>>> It looks like the bug was closed briefly in 2014 and then reopened >>>>> soon thereafter with no justification. It is true that we still >>>>> have this anomaly, but it isn't clear why this is high enough >>>>> priority to work on in 2016 when other bit depths would work >>>>> better for destination imagery and other dithering algorithms >>>>> would work better for most customers and even this targeted fix isn't >>>>> perfect... >>>>> >>>>> ...jim >>>>> >>>>> On 2/16/2016 8:32 AM, Jayathirth D V wrote: >>>>>> Hi Jim, >>>>>> >>>>>> Thanks for letting me know about the importance of custom color >>>>>> maps. I was not able to understand what you mentioned previously >>>>>> since I am new to this terminology. >>>>>> >>>>>> Regarding performance I checked with image having complete >>>>>> 255,255,128 pixels and ran it for 100 times. I am running on >>>>>> Windows >>>>>> 7 Professional SP1 with Intel i5-5300U CPU. Overall I don't see >>>>>> much difference in time taken for drawImage() API before and >>>>>> after change. Details for INT_RGB image as input: >>>>>> >>>>>> Before change : >>>>>> Minimum time 536827ns >>>>>> Maximum time 1560947ns >>>>>> Average time 871167ns >>>>>> >>>>>> After change : >>>>>> Minimum time 536380ns >>>>>> Maximum time 1468130ns >>>>>> Average time 830778ns >>>>>> >>>>>> There is +/- 10% delta both for before and after change time but >>>>>> there is no major gap. Even for image with other input type it >>>>>> holds the same. >>>>>> >>>>>> >>>>>> Regarding custom color maps, previously I ran on default color >>>>>> map generated in TYPE_BYTE_INDEXED constructor. Now I tested with >>>>>> modified color map so that it doesn't contain any of the primary >>>>>> color values. Instead of using 0-255 values with value 51 >>>>>> increments in R,G,B components I used color map with 20-245 >>>>>> values with value >>>>>> 45 as increment. For this color map before and after change all >>>>>> the pixels are referring to same index in color map irrespective >>>>>> of removal of error delta addition in case of primary colors. >>>>>> >>>>>> I have attached log for Red and Green primary colors before and >>>>>> after change(Was not able to attach for more since it exceeded >>>>>> 120KB mailing list limit). Please search for "Final index value =" >>>>>> in the logs. Please let me know your inputs. >>>>>> >>>>>> Thanks, >>>>>> Jay >>>>>> >>>>>> -----Original Message----- >>>>>> From: Jim Graham >>>>>> Sent: Tuesday, February 16, 2016 1:03 AM >>>>>> To: Jayathirth D V >>>>>> Cc: 2d-dev@openjdk.java.net; Philip Race; Prasanta Sadhukhan >>>>>> Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7116979 : >>>>>> Unexpected pixel colour when converting images to >>>>>> TYPE_BYTE_INDEXED >>>>>> >>>>>> Hi Jayathirth, >>>>>> >>>>>> The issue with testing performance with white images is that the >>>>>> tests allow the code to eliminate 3 indexed fetches which take time. >>>>>> Primary colors end up being the one obscure case where the new >>>>>> code might be faster. But, non-primary colors would be slower by >>>>>> a fair amount, so performance testing with converting a >>>>>> randomized or non-primary-color image are the more telling case. >>>>>> An image filled with 255,255,128 would be the worst case because >>>>>> it would involve all of the tests and still have to do all of the >>>>>> lookups. >>>>>> >>>>>> My question about how this works with custom colormaps was not >>>>>> really addressed by your response. A simple test to make sure >>>>>> the colormap has accurate (or very close) conversions for the >>>>>> primary colors may be enough to validate the colormap for the >>>>>> indicated tests. If the colormap contains no pure conversions >>>>>> for some of the primary colors then they may need to be dithered >>>>>> anyway... >>>>>> >>>>>> ...jim >>>>>> >>>>>> On 2/15/16 3:39 AM, Jayathirth D V wrote: >>>>>>> Hi Jim, >>>>>>> >>>>>>> I performed performance analysis with white image so that all >>>>>>> conditions are checked in the new branch added. There is no >>>>>>> major difference in time taken before and after change. For each >>>>>>> input I have tested time taken for drawImage() API to convert >>>>>>> from every format to Byte indexed type. For every unique >>>>>>> conversion test is run for 100 times. Please find the details: >>>>>>> >>>>>>> Input >>>>>>> >>>>>>> type >>>>>>> >>>>>>> >>>>>>> >>>>>>> Min >>>>>>> >>>>>>> before change >>>>>>> >>>>>>> (ns) >>>>>>> >>>>>>> >>>>>>> >>>>>>> Min >>>>>>> >>>>>>> after change >>>>>>> >>>>>>> (ns) >>>>>>> >>>>>>> >>>>>>> >>>>>>> Max >>>>>>> >>>>>>> before change >>>>>>> >>>>>>> (ns) >>>>>>> >>>>>>> >>>>>>> >>>>>>> Max >>>>>>> >>>>>>> after change >>>>>>> >>>>>>> (ns) >>>>>>> >>>>>>> >>>>>>> >>>>>>> Average >>>>>>> >>>>>>> before change >>>>>>> >>>>>>> (ns) >>>>>>> >>>>>>> >>>>>>> >>>>>>> Average >>>>>>> >>>>>>> after >>>>>>> >>>>>>> change >>>>>>> >>>>>>> (ns) >>>>>>> >>>>>>> INT_RGB >>>>>>> >>>>>>> >>>>>>> >>>>>>> 523437 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 481491 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 1230724 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 1270440 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 789452 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 666144 >>>>>>> >>>>>>> INT_ARGB >>>>>>> >>>>>>> >>>>>>> >>>>>>> 500232 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 493986 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 12406307 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 1308816 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 793384 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 654015 >>>>>>> >>>>>>> INT_ARGB_PRE >>>>>>> >>>>>>> >>>>>>> >>>>>>> 500233 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 492201 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 1037057 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 981277 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 710250 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 699214 >>>>>>> >>>>>>> INT_BGR >>>>>>> >>>>>>> >>>>>>> >>>>>>> 537716 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 562706 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 1446703 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 2046001 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 862377 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 863256 >>>>>>> >>>>>>> 3BYTE_BGR >>>>>>> >>>>>>> >>>>>>> >>>>>>> 483275 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 481045 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 1181638 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 1384676 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 651427 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 580961 >>>>>>> >>>>>>> 4BYTE_ABGR >>>>>>> >>>>>>> >>>>>>> >>>>>>> 544410 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 499786 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 1292305 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 968783 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 690106 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 683881 >>>>>>> >>>>>>> 4BYTE_ABGR_PRE >>>>>>> >>>>>>> >>>>>>> >>>>>>> 504249 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 505588 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 1680086 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 1216445 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 756101 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 687750 >>>>>>> >>>>>>> USHORT_565_RGB >>>>>>> >>>>>>> >>>>>>> >>>>>>> 507818 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 505588 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 978153 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 1346746 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 652908 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 655782 >>>>>>> >>>>>>> USHORT_555_RGB >>>>>>> >>>>>>> >>>>>>> >>>>>>> 510496 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 509604 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 952272 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 1162004 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 650418 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 670811 >>>>>>> >>>>>>> BYTE_GRAY >>>>>>> >>>>>>> >>>>>>> >>>>>>> 481491 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 478367 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 1140585 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 1799231 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 691160 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 583250 >>>>>>> >>>>>>> USHORT_GRAY >>>>>>> >>>>>>> >>>>>>> >>>>>>> 506927 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 507373 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 1375751 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 1255267 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 728202 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 646902 >>>>>>> >>>>>>> BYTE_BINARY >>>>>>> >>>>>>> >>>>>>> >>>>>>> 541733 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 496217 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 1083466 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 959411 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 730527 >>>>>>> >>>>>>> >>>>>>> >>>>>>> 728461 >>>>>>> >>>>>>> The changes are tested with plain images having primary colors >>>>>>> like RED, GREEN, BLUE, BLACK and WHITE. >>>>>>> >>>>>>> Thanks, >>>>>>> >>>>>>> Jay >>>>>>> >>>>>>> -----Original Message----- >>>>>>> From: Jim Graham >>>>>>> Sent: Friday, February 12, 2016 4:05 AM >>>>>>> To: Jayathirth D V; 2d-dev@openjdk.java.net; Philip Race; >>>>>>> Prasanta Sadhukhan >>>>>>> Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7116979 : >>>>>>> Unexpected pixel colour when converting images to >>>>>>> TYPE_BYTE_INDEXED >>>>>>> >>>>>>> Hi Jayathirth, >>>>>>> >>>>>>> Did you do any performance analysis of this change? You are >>>>>>> adding >>>>>>> 6 tests and multiple branches to per-pixel code. >>>>>>> >>>>>>> The effectiveness of this technique depends on the colormap that >>>>>>> we have set up. For the BufferedImage.TYPE_INDEXED constructor >>>>>>> we produce a fairly nice colormap, but if someone creates a >>>>>>> custom colormap then the colors could be anything. We create a >>>>>>> decent inversion for just about any colormap, but that doesn't >>>>>>> mean that using only "the best match for solid red" will produce >>>>>>> a better result for a dithered approximation for red. It is >>>>>>> true that if there is a really good match for red then we should >>>>>>> just use that, but if there are no direct matches for red then >>>>>>> we may choose to paint a red region with solid orange even >>>>>>> though there is another color in the colormap that when mixed >>>>>>> with orange approximates a red tone better. For example, if a >>>>>>> colormap contains no pure red, but >>>>>>> >>>>>>> contains: >>>>>>> >>>>>>> 240, 20, 0 >>>>>>> >>>>>>> 240, 0, 20 >>>>>>> >>>>>>> (I'm not sure if 20 is within our current error deltas that we >>>>>>> use for dithering, but this is an example not a test case.) >>>>>>> >>>>>>> Then using one of these alone might skew the color towards >>>>>>> orange or purple. Using both together in a dither pattern might >>>>>>> keep the overall hue impression as red, but with a small amount >>>>>>> of noise in its saturation. >>>>>>> >>>>>>> What types of colormaps was this tested with? >>>>>>> >>>>>>> ...jim >>>>>>> >>>>>>> On 2/11/16 6:37 AM, Jayathirth D V wrote: >>>>>>> >>>>>>>> Hi, >>>>>>>> _Please review the following fix in JDK9:_ __ Bug >>>>>>>> :https://bugs.openjdk.java.net/browse/JDK-7116979 >>>>>>>> Webrev :http://cr.openjdk.java.net/~jdv/7116979/webrev.00/ >>>>>>>> Issue : When Image containing black pixels are converted from >>>>>>>> any format to Byte Indexed format some of the pixels are not >>>>>>>> black. >>>>>>>> They >>>>>>>> are following pattern similar to dithering. >>>>>>>> Root cause : When we convert any format type to ByteIndexed we >>>>>>>> are adding Error delta values to R,G,B components using >>>>>>>> dithering indices. >>>>>>>> This is causing some pixels values to not point to proper index >>>>>>>> in color table. >>>>>>>> Solution : There is no need to add error delta for primary >>>>>>>> colors containing basic values in R,G,B components. Exclude >>>>>>>> such pixels from delta addition. >>>>>>>> Thanks, >>>>>>>> Jay
