[OpenJDK 2D-Dev] Review Request for JDK-7116979 : Unexpected pixel colour when converting images to TYPE_BYTE_INDEXED
Jim Graham
james.graham at oracle.com
Fri Apr 29 23:13:13 UTC 2016
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