Re: [OpenJDK 2D-Dev] RFR 8159638: Improve array caches and renderer stats in Marlin renderer

[Laurent Bourgès]

Dear Jim,

Thanks for your review, I'll answer your questions in the text below and
will later propose a new webrev:


2016-07-19 9:00 GMT+02:00 Jim Graham :

>
> Some work should be done on the comments at the top of ArrayCache.java -
> line 38 and 42 make the same claim about 2 different thresholds.


I agree THRESHOLD_LARGE_ARRAY_SIZE can be removed as THRESHOLD_ARRAY_SIZE =
THRESHOLD_LARGE_ARRAY_SIZE now !


> It seems silly, but in ArrayCache.getNewLargeSize(), lines 162 and 164 are
> both ">" tests and then the newly added test at 166 is a "<" test.  It
> would be nice to main symmetry of the tests in that if/then/elseif sequence.
>

Will do.


> As far as I can see, the buckets are:
>
> 0 - 4K
> 1 - 16k
> 2 - 64k
> 3 - 256k
> 4 - 1m
> 5 - 4m
> 6 - 8m
> 7 - 16m
>
> which makes MAX_ARRAY_SIZE == THRESHOLD_ARRAY_SIZE == 16m and
> THRESHOLD_LARGE is also 16m...?
>

As theses values are all equals now, I will only keep MAX_ARRAY_SIZE (=
maximum size of cached array in buckets) and fix the getNewSize() and
getNewLargeSize() to use that value as the threshold.


> I don't have any issues with those numbers, but the way that they are
> calculated makes it look like they are supposed to be unique numbers and
> yet through the obscurity of the loops used to populate the sizes they just
> end up all being the same numbers - it makes me wonder if there was a
> mistake in there or not...?
>

Initially these values have different values / meanings but it can be
simplified now.



> CacheStats.reset() - should totalInitial be reset as well?  Also, there
> should be a reset method on the BucketStats class rather than just digging
> in and modifying its members directly from outside the class.
>

Initial arrays are allocated by the Reference constructor so totalInitial
must not be reset to give the total memory allocated by initial arrays.
Will add the BucketStats.reset() method.



> It feels odd to have all of the cache classes import the static members of
> ArrayCache rather than just subclassing it. Is there a reason it wasn't
> just subclassed?
>

As all the members are static, I prefer having an explicit static import
instead of subclasses.
Moreover, I deliberately avoided any class inheritance to avoid the
performance penalty of bimorphic / megamorphic method calls (abstract or
overriden methods).


> The Dirty and Clean subclasses are nearly identical and yet they share no
> code simply because buried inside one of their inner classes one of them
> clears the data and the other does not.  That seems a waste for something
> so trivial in the design.
>

The only reason is performance: I want to ensure straightforward method
calls ie no bimorphic or virtual calls to be inlined by hotspot. Maybe such
fear or approach is too conservative i.e. the performance penalty is too
small to consider such optimization. I made many tests with jmh (in june)
but I agree the design can be simpler:
- abstract [Byte/Int/Float]ArrayCache (where putArray() is abstract)
- Clean[Byte/Int/Float]ArrayCache and Dirty[Byte/Int/Float]ArrayCache
implements properly the putArray method but also the createArray() method
(new array or Unsafe.allocateUninitializedArray)

I could try again but I prefer the current design as it is (overly)
strongly typed.
Please propose an alternative / simpler design !


> The various Reference.putArray() methods protect against putting the
> initial arrays, and you the code that calls them also makes the same
> check.  I'd remove the code that checks for initial from the callers so
> that the call sites are more streamlined, but there's no functional issue
> with the way it is now.
>

I will improve that and that will save several tests / lines.
FYI I adopted the double-checks to promote the initial array case as the
fast path whereas the array cache is the slow path (less probable /
exceptional). Typically hotspot optimizes very well such code when only
initial arrays are in use (general use case).


> Also, since you never put the initial arrays, they aren't automatically
> cleaned...?
>

Exactly: initial arrays are allocated by the Reference class and directly
used by callers (fill / clean) and the XxxArrayCache never touch them.

Only CleanIntArrayCache.Reference are used by Marlin:
- MarlinCache
  116:  touchedTile_ref = rdrCtx.newCleanIntArrayRef(INITIAL_ARRAY); // 1K
= 1 tile line
- Renderer
  549:  edgeBuckets_ref  =
rdrCtx.newCleanIntArrayRef(INITIAL_BUCKET_ARRAY); //
64K
  550:  edgeBucketCounts_ref =
rdrCtx.newCleanIntArrayRef(INITIAL_BUCKET_ARRAY); //
64K
  556:  alphaLine_ref = rdrCtx.newCleanIntArrayRef(INITIAL_AA_ARRAY); // 8K
  571:  blkFlags_ref = rdrCtx.newCleanIntArrayRef(INITIAL_ARRAY); // 1K = 1
tile line

 All cases are manually cleaned in MarlinCache.resetTileLine(),
Renderer.dispose() and MarlinCache.copyAARowNoRLE...() for alphaLine and
blkFlags arrays with several cleanup patterns (optimized and
performance-critical).


> Is the shell script only used by you to replicate changes 

[OpenJDK 2D-Dev] RFR (urgent) 8162097: [PIT] A series of closed tests about SunFontManager throw NPE on Windows

[Phil Race]

http://cr.openjdk.java.net/~prr/8162097/

I have an urgent RFR which is blocking PIT.

The fix  : https://bugs.openjdk.java.net/browse/JDK-8152971 to eliminate 
some JNI warnings.


is prematurely deleting a local ref which is used in a callback function.
As as result we get many NPEs in Java code.
The fix is to move the (normal case) delete to after we've done with the 
callback.


The tests which failed before this fix now pass.
Also the JNI check test is not seeing "new" issues .. there are already 
JNI warnings

that it finds in other code.

-phil.

Re: [OpenJDK 2D-Dev] [9] RFR JDK-4882305: StreamPrintServ.getSupportedAttributeValues returns null for Orientation attr

[Jayathirth D V]

Hi Prasanta,

 

You have many lines of commented code in test case, which is not needed.

Also there are cases of null pointer dereferencing at line 68 & 70 in test 
case. Please add checks for the same.

And you can add bug evaluation in JBS for the same.

 

Thanks,

Jay

 

From: Philip Race 
Sent: Wednesday, July 20, 2016 8:14 AM
To: Prasanta Sadhukhan
Cc: 2d-dev
Subject: Re: [OpenJDK 2D-Dev] [9] RFR JDK-4882305: 
StreamPrintServ.getSupportedAttributeValues returns null for Orientation attr

 

+1

-phil.

On 7/14/16, 2:09 AM, Prasanta Sadhukhan wrote: 

Hi All,

Please review a fix for an issue where it is seen that 
even though StreamPrintService returns OrientationRequested category as 
supported, when actually querying the supported attribute value with respect to 
the supported DocFlavors, 
null values are returned for the Orientation attributes when the DocFlavor is 
not either Pageable or Printable (SERVICE_FORMATTED)
BUT we can print a jpeg iamge using StreamPrintService in LANDSCAPE mode and it 
worked fine so StreamPrintService should not return the supported values as 
null for supported DocFlavor.

Bug: https://bugs.openjdk.java.net/browse/JDK-4882305
webrev: HYPERLINK 
"http://cr.openjdk.java.net/%7Epsadhukhan/4882305/webrev.00/"http://cr.openjdk.java.net/~psadhukhan/4882305/webrev.00/

Proposed fix is to add the image/jpeg, image/gif, image/png docflavor in 
addition to Pageable/Printable for Orientation attribute.

Regards
Prasanta

Re: [OpenJDK 2D-Dev] RFR (urgent) 8162097: [PIT] A series of closed tests about SunFontManager throw NPE on Windows

[Brian Burkhalter]

This looks fine as far as I can see.

Brian

On Jul 21, 2016, at 9:57 AM, Phil Race  wrote:

> http://cr.openjdk.java.net/~prr/8162097/
> 
> I have an urgent RFR which is blocking PIT.
> 
> The fix  : https://bugs.openjdk.java.net/browse/JDK-8152971 to eliminate some 
> JNI warnings.
> 
> is prematurely deleting a local ref which is used in a callback function.
> As as result we get many NPEs in Java code.
> The fix is to move the (normal case) delete to after we've done with the 
> callback.
> 
> The tests which failed before this fix now pass.
> Also the JNI check test is not seeing "new" issues .. there are already JNI 
> warnings
> that it finds in other code.
> 
> -phil.

Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

[Phil Race]

OK by me.

-phil.

On 07/20/2016 09:53 AM, Jayathirth D V wrote:

Hi Jim,

Thanks for your input.
I have updated specification of ColorModel and its subclasses.
Please find new webrev for review:
http://cr.openjdk.java.net/~jdv/7107905/webrev.13/

Thanks,
Jay

-Original Message-
From: Jim Graham
Sent: Tuesday, July 12, 2016 7:41 PM
To: Jayathirth D V; Philip Race
Cc: 2d-dev@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel 
subclasses are missing hashCode() or equals() or both methods

Hi Jay,

When I write javadoc suggestions I sometimes use the short-hand "{some text}" to indicate that it should be 
formatted using javadoc protocols, typically that expands to "{@code some text}", but sometimes a link may be 
appropriate.  It looks like you copied and pasted a number of places where I wrote "{foo}" in email and just 
left the bare braces there, please adjust those (most of them should probably just have "@code" inserted.

In ColorModel.equals(), I'd replace the entire "following properties" sentence 
with:

* Subclasses may check additional properties, but this method
* will check the following basic properties for equivalence to
* determine if the target object equals this object:

The text for IndexColorModel is also a little odd.  I'd change it to something 
like:

* The target object and this object will be equal iff
* {@link ColorModel#equals()} returns true and the following
* properties are also the same:

For Packed:

* The target object and this object will be equal iff
* {@link ColorModel#equals()} returns true and the masks
* of the color and alpha samples are the same.

In terms of Joe's request, I'd add the following text to ColorModel.equals() 
right after we talk about all of the properties that it checks:

* 
* Subclasses should override this method if they have any additional
* properties to compare and should use the following implementation.
* Note that the base {@code ColorModel} class already ensures that
* the target object is the same class as this object so the cast to
* the subclass type can be assumed if {@code super.equals(obj)} returns
* true.
* 
* public boolean equals(Object obj) {
* if (!super.equals(obj)) {
* return false;
* }
* MyCMClass cm = (MyCMClass) obj;
* // test additional properties on "cm" and "this"
* }
* 


On 7/1/16 3:31 AM, Jayathirth D V wrote:

Hi Jim,

Thanks for your inputs.
I have discussed with Phil also regarding the same issue offline.
I have collated all the changes mentioned by you and Phil in the latest webrev:
http://cr.openjdk.java.net/~jdv/7107905/webrev.12/

But I was not able to understand your statement - "Arguably, we could omit the class 
comparison text from the subclasses as well by using a reference like that, but I think 
that the class equivalence test is enough out of the ordinary that it does bear 
reiterating it in every subclass as you already do now even though we only reference the 
specific other properties tested by a reference.", please clarify.

Also I am not able find a way to describe Joe's concern of "The ColorModel equals 
and hashCode methods should spell out the protocol subclasses need to follow to obey the 
respective contracts of methods.", I have discussed with Phil also regarding the 
same offline. Any inputs on this also would be helpful.

Thanks,
Jay

-Original Message-
From: Jim Graham
Sent: Thursday, June 30, 2016 4:05 AM
To: Jayathirth D V; Philip Race; 2d-dev@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905:
ColorModel subclasses are missing hashCode() or equals() or both
methods

Hi Jay,

We need to reference the properties from the base class in the subclasses.  We don't need 
to list them every time, but we could introduce the "additional properties" 
using something like:

"... we check all of the properties checked by the {equals} method of {ColorModel}, 
and the following additional properties:"

Arguably, we could omit the class comparison text from the subclasses as well 
by using a reference like that, but I think that the class equivalence test is 
enough out of the ordinary that it does bear reiterating it in every subclass 
as you already do now even though we only reference the specific other 
properties tested by a reference.

A few grammar fixes:

In all of the classes, insert a space before any parentheses in
comment text as in "the same class (and not a subclass)".  (That
wouldn't apply if the text in the comment is referring to code - then
space it as you would actual code.)

In CM, insert the word "the" as in "also check the following ..."

In ICM I would refer to the "Valid bits of" instead as "The list of valid pixel 
indices in".

In PCM I would change "check maskArray of ..." to "check the masks of the ..." and 
pluralize the word "samples".

...jim

On 06/29/2016 04:13 AM, Jayathirth D V wrote:

Hi,

Since Joe mentioned to add 

Re: [OpenJDK 2D-Dev] Review Request for JDK-8160943 : [PIT] new failure of closed/javax/imageio/ReadAllThumbnailsTest.java

[Jim Graham]

Hi Jay,

How did you generate that image with thumbnail?  With ImageIO?

...jim

On 07/19/2016 12:18 PM, Jayathirth D V wrote:

Hi Phil,

I generated Jpeg image with thumbnail as given in test case attached for 
https://bugs.openjdk.java.net/browse/JDK-4958271 with JDK 9.
The image has only two APP0 markers and after the second marker it has "00 FF" which we can 
consider as "X FF" as per https://www.w3.org/Graphics/JPEG/itu-t81.pdf. But it is just continuation 
of pattern "00 00 FF" which we see around the end of second APP0 marker.

For this new image I don't see whether we are writing improperly in JDK9(If we consider 
"X FF" bits between markers).
So we can just apply http://cr.openjdk.java.net/~jdv/8160943/webrev.00/ and 
open up the test case(after verifying whether it is the only test case which 
uses this particular image) with new image that I have created.

Thanks,
Jay

-Original Message-
From: Philip Race
Sent: Wednesday, July 13, 2016 10:06 AM
To: Jim Graham
Cc: Jayathirth D V; 2d-dev
Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-8160943 : [PIT] new 
failure of closed/javax/imageio/ReadAllThumbnailsTest.java

Whilst looking at the reason this test was originally created
https://bugs.openjdk.java.net/browse/JDK-4958271
it started to look likely that the image was (as I sort of suspected) was 
generated by IIO itself.

I think there a couple of things we can do here
1) Open up the test (and image - although if moving it make sure no other tests 
in that location need the image).
2) Investigate to see if the bug such that the "bad padding" is written out is 
still reproducible with current IIO ... or if it was fixed sometime between when that 
image was created and now.

-phil.

On 7/12/16, 7:48 PM, Jim Graham wrote:

I think this is fine, but I noticed that some of the recently added
comments have some grammar issues.  It might be nice to change the
following:

 565 // markers which do not contain length data
(doesn't => do not)

 576 // markers which contain length data
(contains => contain)

I don't need to review those changes...

...jim

On 7/12/16 9:15 AM, Jayathirth D V wrote:

Hi,



Thanks for your input Phil.

I have made changes just to parse "FF FF"(Like "FF 00" or any marker
without length)and not consider it as an invalid marker in
skipImage() of JPEGImageReader.java.

Also I have removed closed/javax/imageio/ReadAllThumbnailsTest.java
from ProblemList.txt as part of fix.



Bug : https://bugs.openjdk.java.net/browse/JDK-8160943



Please find webrev for review for JDK9:

http://cr.openjdk.java.net/~jdv/8160943/webrev.00/



Thanks,

Jay



*From:*Phil Race
*Sent:* Saturday, July 09, 2016 12:37 AM
*To:* Jayathirth D V
*Cc:* Jim Graham; 2d-dev
*Subject:* Re: REG : JDK-8160943 : [PIT] new failure of
closed/javax/imageio/ReadAllThumbnailsTest.java



On 07/08/2016 04:08 AM, Jayathirth D V wrote:

Hi,



In JDK-8152672 
we modified skipImage() in JpegImageReader.java
and added tighter checks for parsing Jpeg data.



We have to find any marker,0 or EOF after we find "FF" while
parsing JPEG data.

But in JDK-8160943
 we have gap
between APP0 marker and DQT(FF DB)
marker which contains data "00 FF".



APP0_End -> 00 FF -> FF DB(DQT)



So after we skip APP0 marker we find two bytes of data which is
"FF FF". In the present code we consider this as
invalid marker.


See https://www.w3.org/Graphics/JPEG/itu-t81.pdf

B.1.1.2 Markers
Markers serve to identify the various structural parts of the
compressed data formats.
Most markers start marker segments containing a related group of
parameters; some markers stand alone. All markers are assigned
two-byte codes: an X'FF' byte followed by a byte which is not equal
to 0 or X'FF' (see Table B.1).
Any marker may optionally be preceded by any number of fill bytes,
^
which are bytes assigned code X'FF
^^

-phil.


Because of this JDK-8160943
 is failing.



Is the length of APP0 marker not valid in the image or we should
not consider "FF FF" as invalid maker?

Please let me know your input.



Thanks,

Jay

Re: [OpenJDK 2D-Dev] RFR 8159638: Improve array caches and renderer stats in Marlin renderer

[Jim Graham]

Hi Laurent,

On 07/21/2016 06:56 AM, Laurent Bourgès wrote:

I don't have any issues with those numbers, but the way that they
are calculated makes it look like they are supposed to be unique
numbers and yet through the obscurity of the loops used to populate
the sizes they just end up all being the same numbers - it makes me
wonder if there was a mistake in there or not...?


Initially these values have different values / meanings but it can be
simplified now.


To be clear, I wasn't complaining about the multitude of thresholds, but 
rather that the way they were apportioned - sort of as a side effect of 
the computations in a loop - ended up accidentally squashing a couple of 
them out of existence.  Another option would be to make sure that the 
thresholds make sense, but keep all 4 threshold ranges, but you are the 
one who knows the details of how these sizes impact performance...



It feels odd to have all of the cache classes import the static
members of ArrayCache rather than just subclassing it. Is there a
reason it wasn't just subclassed?


As all the members are static, I prefer having an explicit static import
instead of subclasses.
Moreover, I deliberately avoided any class inheritance to avoid the
performance penalty of bimorphic / megamorphic method calls (abstract or
overriden methods).


First, I would have expected MumbleArrayCache classes to be a subclass 
of the ArrayCache class in the first place.  If it is not going to be 
the base class then it should have the name "ArrayCacheUtils" or "Const" 
or something.


Second, wildcard imports are recommended against.

Finally, if you are going to use static methods from another class I 
would much rather see explicit naming rather than importing them.  While 
static imports work for methods as well as constants, they are typically 
used for constants and it is confusing to apply them to methods.



The only reason is performance: I want to ensure straightforward method
calls ie no bimorphic or virtual calls to be inlined by hotspot. Maybe
such fear or approach is too conservative i.e. the performance penalty
is too small to consider such optimization. I made many tests with jmh
(in june) but I agree the design can be simpler:
- abstract [Byte/Int/Float]ArrayCache (where putArray() is abstract)
- Clean[Byte/Int/Float]ArrayCache and Dirty[Byte/Int/Float]ArrayCache
implements properly the putArray method but also the createArray()
method (new array or Unsafe.allocateUninitializedArray)

I could try again but I prefer the current design as it is (overly)
strongly typed.
Please propose an alternative / simpler design !


That's something that can be investigated later as it would be a big 
disruption for the current task.  Generally, though, as long as the leaf 
classes are final and the cache classes are strongly typed then HS 
should inline freely.



Also, since you never put the initial arrays, they aren't
automatically cleaned...?

Exactly: initial arrays are allocated by the Reference class and
directly used by callers (fill / clean) and the XxxArrayCache never
touch them.


What I was getting at was that this was potentially a bug?  If you carry 
over use of an initial array in a clean setting without calling to the 
cache object, then who clears the used portion?



 All cases are manually cleaned in MarlinCache.resetTileLine(),
Renderer.dispose() and MarlinCache.copyAARowNoRLE...() for alphaLine and
blkFlags arrays with several cleanup patterns (optimized and
performance-critical).


I see.  Is this really a noticeable performance issue to rely on the 
cache to do the cleaning and have much more readable code?


...jim

Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel subclasses are missing hashCode() or equals() or both methods

[Jim Graham]

Looks good to me...

...jim

On 07/20/2016 09:53 AM, Jayathirth D V wrote:

Hi Jim,

Thanks for your input.
I have updated specification of ColorModel and its subclasses.
Please find new webrev for review:
http://cr.openjdk.java.net/~jdv/7107905/webrev.13/

Thanks,
Jay

-Original Message-
From: Jim Graham
Sent: Tuesday, July 12, 2016 7:41 PM
To: Jayathirth D V; Philip Race
Cc: 2d-dev@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905: ColorModel 
subclasses are missing hashCode() or equals() or both methods

Hi Jay,

When I write javadoc suggestions I sometimes use the short-hand "{some text}" to indicate that it should be 
formatted using javadoc protocols, typically that expands to "{@code some text}", but sometimes a link may be 
appropriate.  It looks like you copied and pasted a number of places where I wrote "{foo}" in email and just 
left the bare braces there, please adjust those (most of them should probably just have "@code" inserted.

In ColorModel.equals(), I'd replace the entire "following properties" sentence 
with:

* Subclasses may check additional properties, but this method
* will check the following basic properties for equivalence to
* determine if the target object equals this object:

The text for IndexColorModel is also a little odd.  I'd change it to something 
like:

* The target object and this object will be equal iff
* {@link ColorModel#equals()} returns true and the following
* properties are also the same:

For Packed:

* The target object and this object will be equal iff
* {@link ColorModel#equals()} returns true and the masks
* of the color and alpha samples are the same.

In terms of Joe's request, I'd add the following text to ColorModel.equals() 
right after we talk about all of the properties that it checks:

* 
* Subclasses should override this method if they have any additional
* properties to compare and should use the following implementation.
* Note that the base {@code ColorModel} class already ensures that
* the target object is the same class as this object so the cast to
* the subclass type can be assumed if {@code super.equals(obj)} returns
* true.
* 
* public boolean equals(Object obj) {
* if (!super.equals(obj)) {
* return false;
* }
* MyCMClass cm = (MyCMClass) obj;
* // test additional properties on "cm" and "this"
* }
* 


On 7/1/16 3:31 AM, Jayathirth D V wrote:

Hi Jim,

Thanks for your inputs.
I have discussed with Phil also regarding the same issue offline.
I have collated all the changes mentioned by you and Phil in the latest webrev:
http://cr.openjdk.java.net/~jdv/7107905/webrev.12/

But I was not able to understand your statement - "Arguably, we could omit the class 
comparison text from the subclasses as well by using a reference like that, but I think 
that the class equivalence test is enough out of the ordinary that it does bear 
reiterating it in every subclass as you already do now even though we only reference the 
specific other properties tested by a reference.", please clarify.

Also I am not able find a way to describe Joe's concern of "The ColorModel equals 
and hashCode methods should spell out the protocol subclasses need to follow to obey the 
respective contracts of methods.", I have discussed with Phil also regarding the 
same offline. Any inputs on this also would be helpful.

Thanks,
Jay

-Original Message-
From: Jim Graham
Sent: Thursday, June 30, 2016 4:05 AM
To: Jayathirth D V; Philip Race; 2d-dev@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-7107905:
ColorModel subclasses are missing hashCode() or equals() or both
methods

Hi Jay,

We need to reference the properties from the base class in the subclasses.  We don't need 
to list them every time, but we could introduce the "additional properties" 
using something like:

"... we check all of the properties checked by the {equals} method of {ColorModel}, 
and the following additional properties:"

Arguably, we could omit the class comparison text from the subclasses as well 
by using a reference like that, but I think that the class equivalence test is 
enough out of the ordinary that it does bear reiterating it in every subclass 
as you already do now even though we only reference the specific other 
properties tested by a reference.

A few grammar fixes:

In all of the classes, insert a space before any parentheses in
comment text as in "the same class (and not a subclass)".  (That
wouldn't apply if the text in the comment is referring to code - then
space it as you would actual code.)

In CM, insert the word "the" as in "also check the following ..."

In ICM I would refer to the "Valid bits of" instead as "The list of valid pixel 
indices in".

In PCM I would change "check maskArray of ..." to "check the masks of the ..." and 
pluralize the word "samples".

...jim

On 06/29/2016 04:13 AM, Jayathirth D V wrote:

Re: [OpenJDK 2D-Dev] Review Request for JDK-8160943 : [PIT] new failure of closed/javax/imageio/ReadAllThumbnailsTest.java

[Jayathirth D V]

Hi Jim,

I just ran the test case attached in 
https://bugs.openjdk.java.net/browse/JDK-4958271 .
It is actually generating an image and using it to do reader.readAll().

Thanks,
Jay
-Original Message-
From: Jim Graham 
Sent: Thursday, July 21, 2016 2:22 PM
To: Jayathirth D V; Philip Race
Cc: 2d-dev
Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-8160943 : [PIT] new 
failure of closed/javax/imageio/ReadAllThumbnailsTest.java

Hi Jay,

How did you generate that image with thumbnail?  With ImageIO?

...jim

On 07/19/2016 12:18 PM, Jayathirth D V wrote:
> Hi Phil,
>
> I generated Jpeg image with thumbnail as given in test case attached for 
> https://bugs.openjdk.java.net/browse/JDK-4958271 with JDK 9.
> The image has only two APP0 markers and after the second marker it has "00 
> FF" which we can consider as "X FF" as per 
> https://www.w3.org/Graphics/JPEG/itu-t81.pdf. But it is just continuation of 
> pattern "00 00 FF" which we see around the end of second APP0 marker.
>
> For this new image I don't see whether we are writing improperly in JDK9(If 
> we consider "X FF" bits between markers).
> So we can just apply http://cr.openjdk.java.net/~jdv/8160943/webrev.00/ and 
> open up the test case(after verifying whether it is the only test case which 
> uses this particular image) with new image that I have created.
>
> Thanks,
> Jay
>
> -Original Message-
> From: Philip Race
> Sent: Wednesday, July 13, 2016 10:06 AM
> To: Jim Graham
> Cc: Jayathirth D V; 2d-dev
> Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-8160943 : [PIT] 
> new failure of closed/javax/imageio/ReadAllThumbnailsTest.java
>
> Whilst looking at the reason this test was originally created
> https://bugs.openjdk.java.net/browse/JDK-4958271
> it started to look likely that the image was (as I sort of suspected) was 
> generated by IIO itself.
>
> I think there a couple of things we can do here
> 1) Open up the test (and image - although if moving it make sure no other 
> tests in that location need the image).
> 2) Investigate to see if the bug such that the "bad padding" is written out 
> is still reproducible with current IIO ... or if it was fixed sometime 
> between when that image was created and now.
>
> -phil.
>
> On 7/12/16, 7:48 PM, Jim Graham wrote:
>> I think this is fine, but I noticed that some of the recently added 
>> comments have some grammar issues.  It might be nice to change the
>> following:
>>
>>  565 // markers which do not contain length data
>> (doesn't => do not)
>>
>>  576 // markers which contain length data
>> (contains => contain)
>>
>> I don't need to review those changes...
>>
>> ...jim
>>
>> On 7/12/16 9:15 AM, Jayathirth D V wrote:
>>> Hi,
>>>
>>>
>>>
>>> Thanks for your input Phil.
>>>
>>> I have made changes just to parse "FF FF"(Like "FF 00" or any marker 
>>> without length)and not consider it as an invalid marker in
>>> skipImage() of JPEGImageReader.java.
>>>
>>> Also I have removed closed/javax/imageio/ReadAllThumbnailsTest.java
>>> from ProblemList.txt as part of fix.
>>>
>>>
>>>
>>> Bug : https://bugs.openjdk.java.net/browse/JDK-8160943
>>>
>>>
>>>
>>> Please find webrev for review for JDK9:
>>>
>>> http://cr.openjdk.java.net/~jdv/8160943/webrev.00/
>>>
>>>
>>>
>>> Thanks,
>>>
>>> Jay
>>>
>>>
>>>
>>> *From:*Phil Race
>>> *Sent:* Saturday, July 09, 2016 12:37 AM
>>> *To:* Jayathirth D V
>>> *Cc:* Jim Graham; 2d-dev
>>> *Subject:* Re: REG : JDK-8160943 : [PIT] new failure of 
>>> closed/javax/imageio/ReadAllThumbnailsTest.java
>>>
>>>
>>>
>>> On 07/08/2016 04:08 AM, Jayathirth D V wrote:
>>>
>>> Hi,
>>>
>>>
>>>
>>> In JDK-8152672 
>>> 
>>> we modified skipImage() in JpegImageReader.java
>>> and added tighter checks for parsing Jpeg data.
>>>
>>>
>>>
>>> We have to find any marker,0 or EOF after we find "FF" while 
>>> parsing JPEG data.
>>>
>>> But in JDK-8160943
>>>  we have gap 
>>> between APP0 marker and DQT(FF DB)
>>> marker which contains data "00 FF".
>>>
>>>
>>>
>>> APP0_End -> 00 FF -> FF DB(DQT)
>>>
>>>
>>>
>>> So after we skip APP0 marker we find two bytes of data which is 
>>> "FF FF". In the present code we consider this as
>>> invalid marker.
>>>
>>>
>>> See https://www.w3.org/Graphics/JPEG/itu-t81.pdf
>>>
>>> B.1.1.2 Markers
>>> Markers serve to identify the various structural parts of the 
>>> compressed data formats.
>>> Most markers start marker segments containing a related group of 
>>> parameters; some markers stand alone. All markers are assigned 
>>> two-byte codes: an X'FF' byte followed by a byte which is not equal 
>>> to 0 or X'FF' (see Table B.1).
>>> Any marker may optionally be preceded by any number of fill bytes, 
>>> ^
>>> which are bytes assigned code X'FF
>>> ^^
>>>
>>> -phil.

Re: [OpenJDK 2D-Dev] RFR (urgent) 8162097: [PIT] A series of closed tests about SunFontManager throw NPE on Windows

[Sergey Bylokhov]

+1

On 21.07.16 20:54, Brian Burkhalter wrote:

This looks fine as far as I can see.

Brian

On Jul 21, 2016, at 9:57 AM, Phil Race  wrote:


http://cr.openjdk.java.net/~prr/8162097/

I have an urgent RFR which is blocking PIT.

The fix  : https://bugs.openjdk.java.net/browse/JDK-8152971 to eliminate some 
JNI warnings.

is prematurely deleting a local ref which is used in a callback function.
As as result we get many NPEs in Java code.
The fix is to move the (normal case) delete to after we've done with the 
callback.

The tests which failed before this fix now pass.
Also the JNI check test is not seeing "new" issues .. there are already JNI 
warnings
that it finds in other code.

-phil.





--
Best regards, Sergey.

[OpenJDK 2D-Dev] RFR: 7175487: Cannot customize font configuration on Linux

[Phil Race]

https://bugs.openjdk.java.net/browse/JDK-7175487
http://cr.openjdk.java.net/~prr/7175487/

As I note in the bug eval the probable issue here is that the file was 
found but rejected.
I've made the code more forgiving by removing the test that the file 
have references to font files.
The XLFD pattern for locating the files will work if they are missing (I 
did some manual testing).


Also the reference to OpenSolaris here is obsolete. There's more clean 
up that could be done
around that but for this fix I wanted to keep it about this Linux issue 
except for cleaning up

the overall if (..) logic

-phil.