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