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