[OpenJDK 2D-Dev] Review Request for JDK-7116979 : Unexpected pixel colour when converting images to TYPE_BYTE_INDEXED

Philip Race philip.race at oracle.com
Thu May 5 22:47:08 UTC 2016


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 at 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 at 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 at 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 at 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 at 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 at 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 at 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



More information about the 2d-dev mailing list