Re: [OpenJDK 2D-Dev] [9] Review Request: 8152683 Deadlock when resuming from sleep with different monitor setup

[Vadim Pakhnushev]

Looks good.

Vadim

On 05.03.2017 21:20, Sergey Bylokhov wrote:

Hello,
Please review the fix for jdk9.

Bug: https://bugs.openjdk.java.net/browse/JDK-8152683
Webrev can be found at: http://cr.openjdk.java.net/~serb/8152683/webrev.00/

In the fix we update the mainDisplayId when the list of displays are updated on 
Appkit thread, instead of reading it each time from the different threads. No 
new issues were found by jck and jtreg tests.

The fix was contributed by Karl von Randow :
[1] http://mail.openjdk.java.net/pipermail/2d-dev/2017-January/008087.html

Re: [OpenJDK 2D-Dev] [9] Review request for 8175025: The copyright section in the test/java/awt/font/TextLayout/DiacriticsDrawingTest.java should be updated

[Vadim Pakhnushev]

+1

Vadim

On 16.02.2017 15:13, Sergey Bylokhov wrote:

Looks fine.


Hello,

Could you review a simple fix for jdk9, please?

bug: https://bugs.openjdk.java.net/browse/JDK-8175025
webrev: http://cr.openjdk.java.net/~dmarkov/8175025/webrev.00/

Changed the license in the test to GPL v2.

Thanks,
Dmitry

Re: [OpenJDK 2D-Dev] RFR: 8170552: [macosx] Wrong rendering of diacritics on macOS

[Vadim Pakhnushev]

+1

Vadim

On 11/02/2017 16:15, dmitry markov wrote:

Hi Dmitry,

The fix looks good to me too, but I am not a reviewer. So  you need 
one more +1 from someone else with reviewer status.

I will sponsor integration of the fix once it is finally approved.

Thanks,
Dmitry
On 11/02/2017 02:48, Phil Race wrote:

Looks good to me.
It used to be a pain keeping several places in sync, hence the single 
util function.
I guess that pre-dated the Apple code and we didn't realise it had 
its own copy ..

So much code ... so little time ...

-phil.

On 02/10/2017 01:07 AM, Dmitry Batrak wrote:

Hello,

I'd like to propose a fix for JDK-8170552.
  bug: https://bugs.openjdk.java.net/browse/JDK-8170552
  webrev: http://cr.openjdk.java.net/~dbatrak/8170552/webrev.00/ 



I don't have a Committer status, so I'll require a sponsor.

I've proposed this patch earlier via support ticket (it's also 
attached to JIRA
issue). It brings the code used to detect complex text when working 
with system
fonts on macOS on par with similar code used for other fonts and 
platforms.


As compared to the previously provided patch, I've also added a test 
case. It
depends on specifics of the font being used (Menlo), but I couldn't 
think of

another way to create an automated test for this issue.

Best regards,
Dmitry Batrak

Re: [OpenJDK 2D-Dev] RFR: 8173028: Incorrect processing of supplementary-plane characters in text fields

[Vadim Pakhnushev]

+1

Vadim

On 13/02/2017 20:52, Sergey Bylokhov wrote:

Looks fine, but I’am not an expert in this area.


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

An off by-one bug in assigning remaining glyphs in the cluster to char indices
so that the zero advance gets assigned to latin "a" and it is not a hit anymore.

font/layout/swing text regression tests have been run.

-phil.

Re: [OpenJDK 2D-Dev] RFR: 8170913: Java "1.8.0_112" on Windows 10 displays different characters for EUDCs from ones created in eudcedit.exe.

[Vadim Pakhnushev]

Looks good.

Vadim

On 10.02.2017 21:02, Phil Race wrote:

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

The EUDC font is now placed first in the list of fall backs, since 
anyone who

has a EUDC font presumably wants the code points from there more than
any other fallbacks that map PUA code points.

No reg. test as it requires system configuration.

-phil

Re: [OpenJDK 2D-Dev] RFR: 8172999: Crash on Windows getting FontMetrics since JDK 9 b96

[Vadim Pakhnushev]

Looks good.

On 19.01.2017 2:08, Phil Race wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8172999
Webrev: http://cr.openjdk.java.net/~prr/8172999/

fix for a crash on windows.

-phil.

Re: [OpenJDK 2D-Dev] RFR: 8168364: [macosx] Delete unused class NSPrintinfo

[Vadim Pakhnushev]

Looks good.

Vadim

On 24.10.2016 21:37, Philip Race wrote:

Can I get a 2nd reviewer on this one ?

-phil.

On 10/20/16, 3:52 AM, Sergey Bylokhov wrote:

Looks fine.

On 20.10.16 2:35, Philip Race wrote:

bug: https://bugs.openjdk.java.net/browse/JDK-8168364
webrev : http://cr.openjdk.java.net/~prr/8168364/

NSPrintInfo is an Attribute so it can presumably be stuffed in an
AttributeSet.
Apart from being inaccessible to apps and unused it is not a good idea.
It was perhaps once used to (re-?)initialise fNSPrintInfo .. and that
needs to
be synchronized it seems. I fixed the one other unsynchronised use 
of that

as well as deleting the unused code.

-phil.

Re: [OpenJDK 2D-Dev] [9] RFR JDK-8168367: Table in javax.imageio package description does not mention TIFF

[Vadim Pakhnushev]

My go-to place for BMP spec is 
https://msdn.microsoft.com/en-us/library/dd183391(v=vs.85).aspx which is 
where we point from JavaFX docs.


Vadim

On 24.10.2016 18:57, Philip Race wrote:

I've poked around the Microsoft website and didn't turn up a location
I am sure we could rely on.

A top-level link to microsoft.com and some words that the format
was devised by Microsoft for Windows may be the best we can do.

-phil.

On 10/24/16, 8:54 AM, Brian Burkhalter wrote:
I suspected as much. Could be that for BMP we are out of luck as 
there is no “real” specification AFAIK.


Brian

On Oct 24, 2016, at 8:30 AM, Philip Race > wrote:



Do not point to wikipedia.
If we must point to external links we want to point to a 
specification in the
most "official" place that exists .. one maintained by the owner of 
that spec.
Strictly that even becomes a part of the SE spec. Wikipedia is not a 
place

we'd ever want to say Java SE relies on for its spec ..

Re: [OpenJDK 2D-Dev] RFR: 8089573: [macosx] Incorrect char to glyph mapping printing on OSX 10.10

[Vadim Pakhnushev]

I guess you could reorder the calls for CFRelease(font); and 
CFRelease(desc); so the desc gets released first in two locations under 
family == NULL and name == NULL.

Just for the sake of consistency.
Other than that, +1.

Vadim

On 16.10.2016 19:09, Philip Race wrote:

Anyone ? I'd like to get this backported to 8u this week so it
can the 8u122 release which will enter RDP2 soon

http://openjdk.java.net/projects/jdk8u/releases/8u122.html

Although I have failed to find any email documenting the exact date
the freeze is usually a week earlier than the RDP date so it could
be as early as a week from now ..

-phil.

On 10/12/16, 1:06 PM, Philip Race wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8089573
Webrev : http://cr.openjdk.java.net/~prr/8089573/

See the bug for some images showing the problem of scrambled text
when printing.

Although only 
JavaFX is affected the solution is entirely on the JDK side.


JavaFX looks up some UI "system" fonts via special API calls and 
these are fonts

that are not otherwise enumerated by CoreText.
JavaFX expects JDK to be able to access these fonts but it cannot.
We need to make the same API calls on JDK.
The exact fonts returned vary by MacOS release so it seems hard to 
create a JDK regression
test but on the FX side it is seen instantly if you print a UI 
control as the text is messed up

due to incorrect glyph ids.

I intend to backport this to 8u and I've verified this does fix it on 
8u too ..


Also both 8 & 9 build successfully with JPRT.

-phil.

Re: [OpenJDK 2D-Dev] RFR: 8162531solaris.fontconfig.properties needs updating

[Vadim Pakhnushev]

+1

Vadim

On 24.09.2016 8:00, Philip Race wrote:
1) version change is only if the format is incompatible - so no change 
there.
2) paths being absent is not a new issue. It has always been the case 
that a
file is not present on a system at runtime. This was historically very 
common. The

implementation should handle this.

-phil.

On 9/23/16, 8:17 AM, Sergey Bylokhov wrote:
The changes looks fine, should we increment version=1 to other 
version or it is not used? Unrelated question is how we validate all 
these paths if some of them are absent?


On 23.09.16 8:08, Philip Race wrote:

PING .. any takers on this two week old RFR ?

-phil.

On 9/8/16, 1:12 PM, Philip Race wrote:

[Fix i18n-dev address]

-phil.

On 9/8/16, 1:10 PM, Philip Race wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8162531
Webrev: http://cr.openjdk.java.net/~prr/8162531/

Solaris 10 is not supported by JDK 9.
This updates the fontconfig file to focus on the default set of fonts
installed on Solaris 11 as part its desktop.

Preference is given to the Arial, Times New Roman, and Courier New 
fonts

that are included with Solaris, as well as other monotype fonts.
Dejavau Sans is listed for some fallback coverage of less commonly 
used

unicode blocks.

10646-1 encoding is referenced so as to reduce the number of entries.
More UTF 8 locales were added since that is the default on Solaris 
11.


-phil.

Re: [OpenJDK 2D-Dev] [9] Review Request 8155753: Removing a monitor in the OS dispaly configuration causes assertion fails under Windows

[Vadim Pakhnushev]

On 12/09/16 23:06, Semyon Sadetsky wrote:

On 9/12/2016 10:36 PM, Vadim Pakhnushev wrote:


Looks good to me (have you submitted JPRT job just in case?)
Is JPRT really necessary? The native part is only changed for Windows 
on which the build is deterministic.


As you wish



--Semyon


Vadim

On 12/09/16 22:31, Semyon Sadetsky wrote:

 I missed this file in the list of changes.

http://cr.openjdk.java.net/~ssadetsky/8155753/webrev.02/

--Semyon


On 9/12/2016 9:33 PM, Vadim Pakhnushev wrote:

Have you forgotten adding changes in AccelGraphicsConfig?

c:\Vadim\jdk9-client\jdk\src\java.desktop\windows\classes\sun\java2d\d3d\D3DGraphicsConfig.java:52: 
error: D3DGraphicsConfig is not abstract and does not override 
abstract method removeDeviceEventListener(AccelDeviceEventListener) 
in AccelGraphicsConfig

public class D3DGraphicsConfig
   ^
c:\Vadim\jdk9-client\jdk\src\java.desktop\windows\classes\sun\java2d\opengl\WGLGraphicsConfig.java:59: 
error: WGLGraphicsConfig is not abstract and does not override 
abstract method removeDeviceEventListener(AccelDeviceEventListener) 
in AccelGraphicsConfig

public class WGLGraphicsConfig
   ^

Also +#include "Devices.h" in the D3DContext.cpp is a leftover.

Vadim

On 12.09.2016 21:11, Semyon Sadetsky wrote:

http://cr.openjdk.java.net/~ssadetsky/8155753/webrev.01/

AccelDeviceEventNotifier is removed.

--Semyon


On 9/12/2016 6:56 PM, Semyon Sadetsky wrote:

Okay. I will remove AccelDeviceEventNotifier and all related code.

--Semyon


On 9/12/2016 6:43 PM, Vadim Pakhnushev wrote:

Hi Semyon,

Generally seems reasonable, it seems that you should use screen 
instead of gdiScreen in the JNU_CallStaticMethodByName, 
otherwise the code won't compile.
Not sure how the rest of the code handles monitor removal, seems 
to me that there are no usages of this notifications anywhere, 
so maybe we don't need this code at all?


Thanks,
Vadim

On 12.09.2016 17:24, Semyon Sadetsky wrote:

Hello,

Please review fix for JDK9:

bug: https://bugs.openjdk.java.net/browse/JDK-8155753

webrev: http://cr.openjdk.java.net/~ssadetsky/8155753/webrev.00/

The issue take place on Windows platform if Direct3d is on. The 
notification routine about the monitor removal tries to get 
screen number using the monitor handle which is obviously null 
at this moment. As a fix the screen number is recorded in D3D 
context for further possible notifications.


--Semyon

Re: [OpenJDK 2D-Dev] [9] Review Request 8155753: Removing a monitor in the OS dispaly configuration causes assertion fails under Windows

[Vadim Pakhnushev]

Looks good to me (have you submitted JPRT job just in case?)

Vadim

On 12/09/16 22:31, Semyon Sadetsky wrote:

 I missed this file in the list of changes.

http://cr.openjdk.java.net/~ssadetsky/8155753/webrev.02/

--Semyon


On 9/12/2016 9:33 PM, Vadim Pakhnushev wrote:

Have you forgotten adding changes in AccelGraphicsConfig?

c:\Vadim\jdk9-client\jdk\src\java.desktop\windows\classes\sun\java2d\d3d\D3DGraphicsConfig.java:52: 
error: D3DGraphicsConfig is not abstract and does not override 
abstract method removeDeviceEventListener(AccelDeviceEventListener) 
in AccelGraphicsConfig

public class D3DGraphicsConfig
   ^
c:\Vadim\jdk9-client\jdk\src\java.desktop\windows\classes\sun\java2d\opengl\WGLGraphicsConfig.java:59: 
error: WGLGraphicsConfig is not abstract and does not override 
abstract method removeDeviceEventListener(AccelDeviceEventListener) 
in AccelGraphicsConfig

public class WGLGraphicsConfig
   ^

Also +#include "Devices.h" in the D3DContext.cpp is a leftover.

Vadim

On 12.09.2016 21:11, Semyon Sadetsky wrote:

http://cr.openjdk.java.net/~ssadetsky/8155753/webrev.01/

AccelDeviceEventNotifier is removed.

--Semyon


On 9/12/2016 6:56 PM, Semyon Sadetsky wrote:

Okay. I will remove AccelDeviceEventNotifier and all related code.

--Semyon


On 9/12/2016 6:43 PM, Vadim Pakhnushev wrote:

Hi Semyon,

Generally seems reasonable, it seems that you should use screen 
instead of gdiScreen in the JNU_CallStaticMethodByName, otherwise 
the code won't compile.
Not sure how the rest of the code handles monitor removal, seems 
to me that there are no usages of this notifications anywhere, so 
maybe we don't need this code at all?


Thanks,
Vadim

On 12.09.2016 17:24, Semyon Sadetsky wrote:

Hello,

Please review fix for JDK9:

bug: https://bugs.openjdk.java.net/browse/JDK-8155753

webrev: http://cr.openjdk.java.net/~ssadetsky/8155753/webrev.00/

The issue take place on Windows platform if Direct3d is on. The 
notification routine about the monitor removal tries to get 
screen number using the monitor handle which is obviously null at 
this moment. As a fix the screen number is recorded in D3D 
context for further possible notifications.


--Semyon

Re: [OpenJDK 2D-Dev] [9] Review Request 8155753: Removing a monitor in the OS dispaly configuration causes assertion fails under Windows

[Vadim Pakhnushev]

Have you forgotten adding changes in AccelGraphicsConfig?

c:\Vadim\jdk9-client\jdk\src\java.desktop\windows\classes\sun\java2d\d3d\D3DGraphicsConfig.java:52: 
error: D3DGraphicsConfig is not abstract and does not override abstract 
method removeDeviceEventListener(AccelDeviceEventListener) in 
AccelGraphicsConfig

public class D3DGraphicsConfig
   ^
c:\Vadim\jdk9-client\jdk\src\java.desktop\windows\classes\sun\java2d\opengl\WGLGraphicsConfig.java:59: 
error: WGLGraphicsConfig is not abstract and does not override abstract 
method removeDeviceEventListener(AccelDeviceEventListener) in 
AccelGraphicsConfig

public class WGLGraphicsConfig
   ^

Also +#include "Devices.h" in the D3DContext.cpp is a leftover.

Vadim

On 12.09.2016 21:11, Semyon Sadetsky wrote:

http://cr.openjdk.java.net/~ssadetsky/8155753/webrev.01/

AccelDeviceEventNotifier is removed.

--Semyon


On 9/12/2016 6:56 PM, Semyon Sadetsky wrote:

Okay. I will remove AccelDeviceEventNotifier and all related code.

--Semyon


On 9/12/2016 6:43 PM, Vadim Pakhnushev wrote:

Hi Semyon,

Generally seems reasonable, it seems that you should use screen 
instead of gdiScreen in the JNU_CallStaticMethodByName, otherwise 
the code won't compile.
Not sure how the rest of the code handles monitor removal, seems to 
me that there are no usages of this notifications anywhere, so maybe 
we don't need this code at all?


Thanks,
Vadim

On 12.09.2016 17:24, Semyon Sadetsky wrote:

Hello,

Please review fix for JDK9:

bug: https://bugs.openjdk.java.net/browse/JDK-8155753

webrev: http://cr.openjdk.java.net/~ssadetsky/8155753/webrev.00/

The issue take place on Windows platform if Direct3d is on. The 
notification routine about the monitor removal tries to get screen 
number using the monitor handle which is obviously null at this 
moment. As a fix the screen number is recorded in D3D context for 
further possible notifications.


--Semyon

Re: [OpenJDK 2D-Dev] [9] Review Request 8155753: Removing a monitor in the OS dispaly configuration causes assertion fails under Windows

[Vadim Pakhnushev]

Hi Semyon,

Generally seems reasonable, it seems that you should use screen instead 
of gdiScreen in the JNU_CallStaticMethodByName, otherwise the code won't 
compile.
Not sure how the rest of the code handles monitor removal, seems to me 
that there are no usages of this notifications anywhere, so maybe we 
don't need this code at all?


Thanks,
Vadim

On 12.09.2016 17:24, Semyon Sadetsky wrote:

Hello,

Please review fix for JDK9:

bug: https://bugs.openjdk.java.net/browse/JDK-8155753

webrev: http://cr.openjdk.java.net/~ssadetsky/8155753/webrev.00/

The issue take place on Windows platform if Direct3d is on. The 
notification routine about the monitor removal tries to get screen 
number using the monitor handle which is obviously null at this 
moment. As a fix the screen number is recorded in D3D context for 
further possible notifications.


--Semyon

Re: [OpenJDK 2D-Dev] [9] Review Request: 8146042 Offscreen rendering is different from onscreen one

[Vadim Pakhnushev]

On 09.09.2016 15:58, Semyon Sadetsky wrote:

On 09.09.2016 14:45, Vadim Pakhnushev wrote:





My cards are HD Graphics 3000 (0x8086/0x0126) with 9.17.10.4101 and
ATI Radeon HD 5700 (0x1002/0x68b8) with driver 8.17.10.1333

ATI is unrelated to this fix. Could you upgrade to the latest driver
from https://downloadcenter.intel.com/search?keyword=hd+graphics+3000
and try to reproduce 803944 again?


Alas, the issue is still there even with the latest driver:
https://i.imgur.com/1kXVbaI.png
I definitely cannot reproduce this picture on my system. Maybe it take 
place for old Intel cards only. HD 3000 is  5+ years old and probably 
it does not fully support Windows 7 d3d. HD graphics declares support 
for dx11 starting from HD4000.
This bug and all related were resolved by disabling d3d for all Intel 
video cards. We could at least to defer those bugs and disable d3d in 
a separate bug as a temp solution. We are forgetting about Intel d3d 
forever?
Its a pity, because Intel APUs as main graphics card are rather 
popular. Ultrabooks rarely have discrete video.


We had a lot of reports on HD4000 as well, listed as duplicates of 803944.
DX11 support is not relevant here at all since we are only use DX9.

Since your fix explicitly checks for Intel hardware, it doesn't make any 
sense to use it without unblocking them.
Moreover, I can't see how this bug (8146042) can be reproduced without 
modified build which unblocks Intel hardware.


Vadim

Re: [OpenJDK 2D-Dev] [9] Review Request: 8146042 Offscreen rendering is different from onscreen one

[Vadim Pakhnushev]

My cards are HD Graphics 3000 (0x8086/0x0126) with 9.17.10.4101 and
ATI Radeon HD 5700 (0x1002/0x68b8) with driver 8.17.10.1333

ATI is unrelated to this fix. Could you upgrade to the latest driver
from https://downloadcenter.intel.com/search?keyword=hd+graphics+3000
and try to reproduce 803944 again?


Alas, the issue is still there even with the latest driver:
https://i.imgur.com/1kXVbaI.png

Vadim

Re: [OpenJDK 2D-Dev] [9] Review Request: 8146042 Offscreen rendering is different from onscreen one

[Vadim Pakhnushev]

Yes I can see it now, the test fails only if I remove Intel cards from 
blacklist.

And it passes with the fix and SwingSet2 looks better.
Unfortunately, this reintroduce 803944 at least on my card/driver and 
from the history of that bug the issue can be reproduced on wide variety 
of cards/drivers.
To check if it's reproducible for you, hover the mouse above checkboxes 
in SwingSet2.

For me they appears corrupted after the hover.

Thanks,
Vadim

On 09.09.2016 14:16, Semyon Sadetsky wrote:



On 09.09.2016 13:43, Vadim Pakhnushev wrote:

On 09.09.2016 13:12, Semyon Sadetsky wrote:

On 09.09.2016 12:56, Vadim Pakhnushev wrote:


Semyon,

I can reproduce JDK-803944 on my machine with your fix.
More importantly, the RenderRectTest is passed for me on Windows 7 
on ATI and Intel cards _without the fix_.
My fix is not aimed to fix 803944. I cannot reproduce it. I have 
i5-4300M (HD Graphics 4600) and Intel driver 10.18.10.3412 signed by 
Microsoft. Which Intel card/APU and driver do you have?


You removed all Intel cards from blacklist and this reintroduced 
803944 since your fix doesn't address it.
My cards are HD Graphics 3000 (0x8086/0x0126) with 9.17.10.4101 and 
ATI Radeon HD 5700 (0x1002/0x68b8) with driver 8.17.10.1333
ATI is unrelated to this fix. Could you upgrade to the latest driver 
from https://downloadcenter.intel.com/search?keyword=hd+graphics+3000 
and try to reproduce 803944 again?


What about the RenderRectTest? In what configuration it fails?

It falls with d3d on Intel APUs.

--Semyon




Do you have a standalone test from the original report?

Simply run SwinSet2: all horizontal/vertical lines are shifted +1 px.


Could you please be more specific? Shifted from where to where? Maybe 
attach good/bad screenshots to the JIRA?


Thanks,
Vadim



--Semyon


Vadim

On 09.09.2016 10:31, Semyon Sadetsky wrote:
I cannot reproduce JDK-803944. It is about very specific hardware 
configuration with two different video cards.


I didn't find any evaluation/justification, neither in JIRA nor in 
the review on the alias, for the 803944 resolution that d3d should 
be disabled for all Intel video cards. Why?


It may be disabled by default, but at least the 
sun.java2d.d3d=true option could enable it, no?


--Semyon


On 9/9/2016 4:58 AM, Philip Race wrote:

Please consider https://bugs.openjdk.java.net/browse/JDK-8039444
and the various bugs that were closed as a duplicate of that bug.
I don't think you can easily show this fix resolves all of these ..

-phil.


On 9/8/16, 5:12 PM, Semyon Sadetsky wrote:
I have 2 laptops Intel i5, i7. Both working with d3d normally. 
Some visual defects will be corrected after this fix.
And I didn't get why d3d is disabled for all Intel without 
possibility to switch it on?


--Semyon

On 09.09.2016 02:10, Philip Race wrote:
The following is just for testing right ? It should not be in 
this webrev

as part of what you propose to push ..
--
src/java.desktop/windows/native/libawt/java2d/d3d/D3DBadHardware.h
Print this page

@@ -49,13 +49,10 @@
 // all versions must fail ("there's no version of the driver 
that passes")

 #define NO_VERSION D_VERSION(0x, 0x, 0x, 0x)

 static const ADAPTER_INFO badHardware[] = {

-// All Intel Chips.
-{ 0x8086, ALL_DEVICEIDS, NO_VERSION, OS_ALL },
-
 // ATI Mobility Radeon X1600, X1400, X1450, X1300, X1350
 // Reason: workaround for 6613066, 6687166
 // X1300 (four sub ids)
 { 0x1002, 0x714A, D_VERSION(6,14,10,6706), OS_WINXP },
 { 0x1002, 0x714A, D_VERSION(7,14,10,0567), OS_VISTA },

---

-phil.

On 9/8/16, 4:06 PM, Semyon Sadetsky wrote:

I have reworked fix to not affect ATI and NVidia.

http://cr.openjdk.java.net/~ssadetsky/8146042/webrev.05/

--Semyon


On 9/9/2016 12:20 AM, Semyon Sadetsky wrote:



On 08.09.2016 22:57, Philip Race wrote:


Can you provide something like a rationale for why these 
particular values might work ?
Otherwise this seems like a fix that can't be reviewed, only 
tested.

So that testing will be important. If you can be sure it passes
on ATI, Nvidia, and Intel then we can take it .. otherwise 
we should defer this.
I suppose those fudge factors are obtained experimentally. 
Not sure that any rationale is possible here.
The fix simply tested on different hardware. I hope after 
this fix D3D maybe enabled again for Intel APUs.

Currently it has been blacklisted in 8039444.

--Semyon


IIRC Semyon will need to change the code to bypass the check
for Intel hardware. There is no env. variable or system 
property to do this.


-phil.

On 9/8/16, 3:47 AM, Sergey Bylokhov wrote:

On 05.09.16 13:36, Semyon Sadetsky wrote:

At last I could get NVidia machine (special thanks to Yuri).

The updated fix should improve the situation on NVidia. 
For that one
common height/width fudge factor was separated in two 
different.


http://cr.openjdk.java.net/~ssadetsky/8146042/webrev.04/


Can you please confirm that the fix and test works if d3d 
is enabled on the intel vk

Re: [OpenJDK 2D-Dev] [9] Review Request: 8146042 Offscreen rendering is different from onscreen one

[Vadim Pakhnushev]

On 09.09.2016 13:12, Semyon Sadetsky wrote:

On 09.09.2016 12:56, Vadim Pakhnushev wrote:


Semyon,

I can reproduce JDK-803944 on my machine with your fix.
More importantly, the RenderRectTest is passed for me on Windows 7 on 
ATI and Intel cards _without the fix_.
My fix is not aimed to fix 803944. I cannot reproduce it. I have 
i5-4300M (HD Graphics 4600) and Intel driver 10.18.10.3412 signed by 
Microsoft. Which Intel card/APU and driver do you have?


You removed all Intel cards from blacklist and this reintroduced 803944 
since your fix doesn't address it.
My cards are HD Graphics 3000 (0x8086/0x0126) with 9.17.10.4101 and ATI 
Radeon HD 5700 (0x1002/0x68b8) with driver 8.17.10.1333


What about the RenderRectTest? In what configuration it fails?



Do you have a standalone test from the original report?

Simply run SwinSet2: all horizontal/vertical lines are shifted +1 px.


Could you please be more specific? Shifted from where to where? Maybe 
attach good/bad screenshots to the JIRA?


Thanks,
Vadim



--Semyon


Vadim

On 09.09.2016 10:31, Semyon Sadetsky wrote:
I cannot reproduce JDK-803944. It is about very specific hardware 
configuration with two different video cards.


I didn't find any evaluation/justification, neither in JIRA nor in 
the review on the alias, for the 803944 resolution that d3d should 
be disabled for all Intel video cards. Why?


It may be disabled by default, but at least the sun.java2d.d3d=true 
option could enable it, no?


--Semyon


On 9/9/2016 4:58 AM, Philip Race wrote:

Please consider https://bugs.openjdk.java.net/browse/JDK-8039444
and the various bugs that were closed as a duplicate of that bug.
I don't think you can easily show this fix resolves all of these ..

-phil.


On 9/8/16, 5:12 PM, Semyon Sadetsky wrote:
I have 2 laptops Intel i5, i7. Both working with d3d normally. 
Some visual defects will be corrected after this fix.
And I didn't get why d3d is disabled for all Intel without 
possibility to switch it on?


--Semyon

On 09.09.2016 02:10, Philip Race wrote:
The following is just for testing right ? It should not be in 
this webrev

as part of what you propose to push ..
--
src/java.desktop/windows/native/libawt/java2d/d3d/D3DBadHardware.h
Print this page

@@ -49,13 +49,10 @@
 // all versions must fail ("there's no version of the driver 
that passes")

 #define NO_VERSION D_VERSION(0x, 0x, 0x, 0x)

 static const ADAPTER_INFO badHardware[] = {

-// All Intel Chips.
-{ 0x8086, ALL_DEVICEIDS, NO_VERSION, OS_ALL },
-
 // ATI Mobility Radeon X1600, X1400, X1450, X1300, X1350
 // Reason: workaround for 6613066, 6687166
 // X1300 (four sub ids)
 { 0x1002, 0x714A, D_VERSION(6,14,10,6706), OS_WINXP },
 { 0x1002, 0x714A, D_VERSION(7,14,10,0567), OS_VISTA },

---

-phil.

On 9/8/16, 4:06 PM, Semyon Sadetsky wrote:

I have reworked fix to not affect ATI and NVidia.

http://cr.openjdk.java.net/~ssadetsky/8146042/webrev.05/

--Semyon


On 9/9/2016 12:20 AM, Semyon Sadetsky wrote:



On 08.09.2016 22:57, Philip Race wrote:


Can you provide something like a rationale for why these 
particular values might work ?
Otherwise this seems like a fix that can't be reviewed, only 
tested.

So that testing will be important. If you can be sure it passes
on ATI, Nvidia, and Intel then we can take it .. otherwise we 
should defer this.
I suppose those fudge factors are obtained experimentally. Not 
sure that any rationale is possible here.
The fix simply tested on different hardware. I hope after this 
fix D3D maybe enabled again for Intel APUs.

Currently it has been blacklisted in 8039444.

--Semyon


IIRC Semyon will need to change the code to bypass the check
for Intel hardware. There is no env. variable or system 
property to do this.


-phil.

On 9/8/16, 3:47 AM, Sergey Bylokhov wrote:

On 05.09.16 13:36, Semyon Sadetsky wrote:

At last I could get NVidia machine (special thanks to Yuri).

The updated fix should improve the situation on NVidia. For 
that one
common height/width fudge factor was separated in two 
different.


http://cr.openjdk.java.net/~ssadetsky/8146042/webrev.04/


Can you please confirm that the fix and test works if d3d is 
enabled on the intel vk? I recall that d3d was disabled on 
intel so probably to check that we need to force d3d manually.



On 3/18/2016 9:12 AM, Semyon Sadetsky wrote:

Hi Phil,

Sergey wrote it fails on nvidia cards. I could play with 
fudge factors

values but I don't have nvidia based video to test.

--Semyon

On 3/17/2016 11:05 PM, Phil Race wrote:

Semyon,

Any update on this ?
FWIW I used jprt to build your patch as I am having 
windows build

problems and
it passed on my ATI card.

-phil.


On 03/01/2016 04:37 PM, Sergey Bylokhov wrote:

On 15.01.16 9:59, Semyon Sadetsky wrote:

Hi Phil & Sergey,

I have integrated Intel GPU i5 and cannot test other 
hardware.
On Mac's retina display the screen capture doesn't 
return exact

pixel to
pixel image but the scal

Re: [OpenJDK 2D-Dev] [9] Review Request: 8146042 Offscreen rendering is different from onscreen one

[Vadim Pakhnushev]

Semyon,

I can reproduce JDK-803944 on my machine with your fix.
More importantly, the RenderRectTest is passed for me on Windows 7 on 
ATI and Intel cards _without the fix_.

Do you have a standalone test from the original report?

Vadim

On 09.09.2016 10:31, Semyon Sadetsky wrote:
I cannot reproduce JDK-803944. It is about very specific hardware 
configuration with two different video cards.


I didn't find any evaluation/justification, neither in JIRA nor in the 
review on the alias, for the 803944 resolution that d3d should be 
disabled for all Intel video cards. Why?


It may be disabled by default, but at least the sun.java2d.d3d=true 
option could enable it, no?


--Semyon


On 9/9/2016 4:58 AM, Philip Race wrote:

Please consider https://bugs.openjdk.java.net/browse/JDK-8039444
and the various bugs that were closed as a duplicate of that bug.
I don't think you can easily show this fix resolves all of these ..

-phil.


On 9/8/16, 5:12 PM, Semyon Sadetsky wrote:
I have 2 laptops Intel i5, i7. Both working with d3d normally. Some 
visual defects will be corrected after this fix.
And I didn't get why d3d is disabled for all Intel without 
possibility to switch it on?


--Semyon

On 09.09.2016 02:10, Philip Race wrote:
The following is just for testing right ? It should not be in this 
webrev

as part of what you propose to push ..
--
src/java.desktop/windows/native/libawt/java2d/d3d/D3DBadHardware.h
Print this page

@@ -49,13 +49,10 @@
 // all versions must fail ("there's no version of the driver that 
passes")

 #define NO_VERSION D_VERSION(0x, 0x, 0x, 0x)

 static const ADAPTER_INFO badHardware[] = {

-// All Intel Chips.
-{ 0x8086, ALL_DEVICEIDS, NO_VERSION, OS_ALL },
-
 // ATI Mobility Radeon X1600, X1400, X1450, X1300, X1350
 // Reason: workaround for 6613066, 6687166
 // X1300 (four sub ids)
 { 0x1002, 0x714A, D_VERSION(6,14,10,6706), OS_WINXP },
 { 0x1002, 0x714A, D_VERSION(7,14,10,0567), OS_VISTA },

---

-phil.

On 9/8/16, 4:06 PM, Semyon Sadetsky wrote:

I have reworked fix to not affect ATI and NVidia.

http://cr.openjdk.java.net/~ssadetsky/8146042/webrev.05/

--Semyon


On 9/9/2016 12:20 AM, Semyon Sadetsky wrote:



On 08.09.2016 22:57, Philip Race wrote:


Can you provide something like a rationale for why these 
particular values might work ?
Otherwise this seems like a fix that can't be reviewed, only 
tested.

So that testing will be important. If you can be sure it passes
on ATI, Nvidia, and Intel then we can take it .. otherwise we 
should defer this.
I suppose those fudge factors are obtained experimentally. Not 
sure that any rationale is possible here.
The fix simply tested on different hardware. I hope after this 
fix D3D maybe enabled again for Intel APUs.

Currently it has been blacklisted in 8039444.

--Semyon


IIRC Semyon will need to change the code to bypass the check
for Intel hardware. There is no env. variable or system property 
to do this.


-phil.

On 9/8/16, 3:47 AM, Sergey Bylokhov wrote:

On 05.09.16 13:36, Semyon Sadetsky wrote:

At last I could get NVidia machine (special thanks to Yuri).

The updated fix should improve the situation on NVidia. For 
that one

common height/width fudge factor was separated in two different.

http://cr.openjdk.java.net/~ssadetsky/8146042/webrev.04/


Can you please confirm that the fix and test works if d3d is 
enabled on the intel vk? I recall that d3d was disabled on 
intel so probably to check that we need to force d3d manually.



On 3/18/2016 9:12 AM, Semyon Sadetsky wrote:

Hi Phil,

Sergey wrote it fails on nvidia cards. I could play with 
fudge factors

values but I don't have nvidia based video to test.

--Semyon

On 3/17/2016 11:05 PM, Phil Race wrote:

Semyon,

Any update on this ?
FWIW I used jprt to build your patch as I am having windows 
build

problems and
it passed on my ATI card.

-phil.


On 03/01/2016 04:37 PM, Sergey Bylokhov wrote:

On 15.01.16 9:59, Semyon Sadetsky wrote:

Hi Phil & Sergey,

I have integrated Intel GPU i5 and cannot test other 
hardware.
On Mac's retina display the screen capture doesn't return 
exact

pixel to
pixel image but the scaled one. So Mac platform should be 
excluded

from
testing:
http://cr.openjdk.java.net/~ssadetsky/8146042/webrev.01/


I run the latest test(webrev.03) on my nvidia card, and it 
fails
after the fix, but pass before =(. I have no ati to check. 
Also I
cannot check the fix on intel card, because I cannot enable 
d3d on it.




--Semyon

On 1/14/2016 9:23 PM, Phil Race wrote:

This fudge factor was last adjusted in
https://bugs.openjdk.java.net/browse/JDK-6597822
way back before the D3D pipeline was released and the 
comments

seem to
indicate that
there was a fair amount of testing on different hardware.

I don't know why this seems to be in un-specified 
hardware-dependent

territory but
it seems quite possible that this could just as easily 
introduce a

different artifact
on some other hardware.

What exactly are 

Re: [OpenJDK 2D-Dev] Review Request: JDK-8144735 [hidpi] javax/swing/JWindow/ShapedAndTranslucentWindows/TranslucentPerPixelTranslucentGradient.java fails

[Vadim Pakhnushev]

+1

Vadim

On 31.08.2016 16:05, Prem Balakrishnan wrote:


Hi Vadim,

Thankyou for the Review.

I have updated the patch as per review comments,

http://cr.openjdk.java.net/~pkbalakr/8144735/webrev.03/ 
<http://cr.openjdk.java.net/%7Epkbalakr/8144735/webrev.03/>


>According to the comments in the 
TranslucentWindowPainter.createInstance you will get BIWindowPainter 
even for OpenGL pipeline unless you force optimized path via 
>sun.java2d.twp.forceopt so you will need to check these paths in the 
fix for OpenGL.


 Yes, BIWindowPainter get called in OpenGl pipleline , I am working on 
OpenGL rendering issue(8164811).


Regards,

Prem

*From:*Vadim Pakhnushev
*Sent:* Wednesday, August 31, 2016 5:00 PM
*To:* Prem Balakrishnan; Sergey Bylokhov; 2d-dev@openjdk.java.net
*Subject:* Re: Review Request: JDK-8144735 [hidpi] 
javax/swing/JWindow/ShapedAndTranslucentWindows/TranslucentPerPixelTranslucentGradient.java 
fails


Prem,

I believe it's possible to use AccelSurface.getBounds() method and 
skip casting to SurfaceData completely.

The calculations are basically the same.

According to the comments in the 
TranslucentWindowPainter.createInstance you will get BIWindowPainter 
even for OpenGL pipeline unless you force optimized path via 
sun.java2d.twp.forceopt so you will need to check these paths in the 
fix for OpenGL.


Thanks,
Vadim

On 30.08.2016 9:17, Prem Balakrishnan wrote:

Reminder

*From:*Prasanta Sadhukhan
*Sent:* Friday, August 26, 2016 12:08 PM
*To:* Prem Balakrishnan; 2d-dev@openjdk.java.net
<mailto:2d-dev@openjdk.java.net>
*Subject:* Re: Review Request: JDK-8144735 [hidpi]

javax/swing/JWindow/ShapedAndTranslucentWindows/TranslucentPerPixelTranslucentGradient.java
fails

Ok. so long you are addressing the opengl issue albeit via
different bugid, it's ok with me. +1.

Regards
Prasanta

On 8/26/2016 11:56 AM, Prem Balakrishnan wrote:

Hi Prasanta,

Thankyou for the review.

The code compiles , Actual scenario is as below:

public class Temp {

AccelSurface as = new AccelSurface() {};

SurfaceData sd = (SurfaceData)as;

}

class SurfaceData {}

interface Surface {}

interface AccelSurface extends Surface{}

class D3DSurfaceData extends SurfaceData implements AccelSurface{}

class OGLSurfaceData extends SurfaceData implements AccelSurface{}

---

In suggested fix, the SurfaceData is always of type
D3DSurfaceData.

And hence getDefaultScaleX/Y holds good. OpenGL rendering is
handled in a different flow.

http://cr.openjdk.java.net/~pkbalakr/8144735/webrev.02/
<http://cr.openjdk.java.net/%7Epkbalakr/8144735/webrev.02/>

Suggested fix also resolves the following failures on hidpi
windows 8

javax/swing/JWindow/ShapedAndTranslucentWindows/PerPixelTranslucent.java


javax/swing/JWindow/ShapedAndTranslucentWindows/PerPixelTranslucentGradient.java


javax/swing/JWindow/ShapedAndTranslucentWindows/SetShapeAndClickSwing.java


javax/swing/JWindow/ShapedAndTranslucentWindows/ShapedPerPixelTranslucentGradient.java


javax/swing/JWindow/ShapedAndTranslucentWindows/ShapedTranslucentPerPixelTranslucentGradient.java


javax/swing/JWindow/ShapedAndTranslucentWindows/TranslucentPerPixelTranslucentGradient.java

--
OpenGL Rendering issue will be addressed in JDK-8164811
<https://bugs.openjdk.java.net/browse/JDK-8164811>



Regards,

Prem

*From:*Prasanta Sadhukhan
*Sent:* Monday, August 22, 2016 12:32 PM
*To:* Prem Balakrishnan; Rajeev Chamyal;
awt-...@openjdk.java.net <mailto:awt-...@openjdk.java.net>;
2d-dev@openjdk.java.net <mailto:2d-dev@openjdk.java.net>
*Subject:* Re: Review Request: JDK-8144735 [hidpi]

javax/swing/JWindow/ShapedAndTranslucentWindows/TranslucentPerPixelTranslucentGradient.java
fails

I wonder how it compiles?
I tried a small program
--
public class Test {
AccelSurface b = new AccelSurface();
SurfaceData c = (SurfaceData)b;
}

class SurfaceData extends Surface {}
class Surface {}
class AccelSurface extends Surface {}
-
and it fails to compile
Test.java:3: error: incompatible types: AccelSurface cannot be
converted to SurfaceData
SurfaceData c = (SurfaceData)b;

ANyways, did you check with opengl pipeline? It seems
getDefaultScaleX/Y is not present in OGLSurfaceData which will
result in falling back to SurfaceData in which case the scale
will be 1.

Regards
Prasanta

On 8/18/2016 3:12 PM, Prem Balakrishnan wrote:

Re: [OpenJDK 2D-Dev] Review Request: JDK-8144735 [hidpi] javax/swing/JWindow/ShapedAndTranslucentWindows/TranslucentPerPixelTranslucentGradient.java fails

[Vadim Pakhnushev]

Prem,

I believe it's possible to use AccelSurface.getBounds() method and skip 
casting to SurfaceData completely.

The calculations are basically the same.

According to the comments in the TranslucentWindowPainter.createInstance 
you will get BIWindowPainter even for OpenGL pipeline unless you force 
optimized path via sun.java2d.twp.forceopt so you will need to check 
these paths in the fix for OpenGL.


Thanks,
Vadim

On 30.08.2016 9:17, Prem Balakrishnan wrote:


Reminder

*From:*Prasanta Sadhukhan
*Sent:* Friday, August 26, 2016 12:08 PM
*To:* Prem Balakrishnan; 2d-dev@openjdk.java.net
*Subject:* Re: Review Request: JDK-8144735 [hidpi] 
javax/swing/JWindow/ShapedAndTranslucentWindows/TranslucentPerPixelTranslucentGradient.java 
fails


Ok. so long you are addressing the opengl issue albeit via different 
bugid, it's ok with me. +1.


Regards
Prasanta

On 8/26/2016 11:56 AM, Prem Balakrishnan wrote:

Hi Prasanta,

Thankyou for the review.

The code compiles , Actual scenario is as below:

public class Temp {

AccelSurface as = new AccelSurface() {};

SurfaceData sd = (SurfaceData)as;

}

class SurfaceData {}

interface Surface {}

interface AccelSurface extends Surface{}

class D3DSurfaceData extends SurfaceData implements AccelSurface{}

class OGLSurfaceData extends SurfaceData implements AccelSurface{}

---

In suggested fix, the SurfaceData is always of type D3DSurfaceData.

And hence getDefaultScaleX/Y holds good. OpenGL rendering is
handled in a different flow.

http://cr.openjdk.java.net/~pkbalakr/8144735/webrev.02/


Suggested fix also resolves the following failures on hidpi windows 8

javax/swing/JWindow/ShapedAndTranslucentWindows/PerPixelTranslucent.java


javax/swing/JWindow/ShapedAndTranslucentWindows/PerPixelTranslucentGradient.java

javax/swing/JWindow/ShapedAndTranslucentWindows/SetShapeAndClickSwing.java


javax/swing/JWindow/ShapedAndTranslucentWindows/ShapedPerPixelTranslucentGradient.java


javax/swing/JWindow/ShapedAndTranslucentWindows/ShapedTranslucentPerPixelTranslucentGradient.java


javax/swing/JWindow/ShapedAndTranslucentWindows/TranslucentPerPixelTranslucentGradient.java

--
OpenGL Rendering issue will be addressed in JDK-8164811




Regards,

Prem

*From:*Prasanta Sadhukhan
*Sent:* Monday, August 22, 2016 12:32 PM
*To:* Prem Balakrishnan; Rajeev Chamyal; awt-...@openjdk.java.net
; 2d-dev@openjdk.java.net

*Subject:* Re: Review Request: JDK-8144735 [hidpi]

javax/swing/JWindow/ShapedAndTranslucentWindows/TranslucentPerPixelTranslucentGradient.java
fails

I wonder how it compiles?
I tried a small program
--
public class Test {
AccelSurface b = new AccelSurface();
SurfaceData c = (SurfaceData)b;
}

class SurfaceData extends Surface {}
class Surface {}
class AccelSurface extends Surface {}
-
and it fails to compile
Test.java:3: error: incompatible types: AccelSurface cannot be
converted to SurfaceData
SurfaceData c = (SurfaceData)b;

ANyways, did you check with opengl pipeline? It seems
getDefaultScaleX/Y is not present in OGLSurfaceData which will
result in falling back to SurfaceData in which case the scale will
be 1.

Regards
Prasanta

On 8/18/2016 3:12 PM, Prem Balakrishnan wrote:

Added “2d-dev” team for review

Regards,

Prem

*From:*Alexandr Scherbatiy
*Sent:* Thursday, August 18, 2016 2:57 PM
*To:* Prem Balakrishnan; Rajeev Chamyal;
awt-...@openjdk.java.net ;
Sergey Bylokhov
*Subject:* Re: Review Request: JDK-8144735 [hidpi]

javax/swing/JWindow/ShapedAndTranslucentWindows/TranslucentPerPixelTranslucentGradient.java
fails

Could you also send the review to the 2d-dev alias?

Thanks,
Alexandr.

On 8/18/2016 9:59 AM, Prem Balakrishnan wrote:

Hi Alexandr,

AccelSurface is implemented by *ONLY* D3DSurfaceData and
OGLSurfaceData classes,

Both of these classes extend SurfaceData as well.

Hence, casting of 'as' variable which is of type
AccelSurface object to SurfaceData is always VALID.

Regards,
Prem

*From:*Alexandr Scherbatiy
*Sent:* Wednesday, August 17, 2016 4:42 PM
*To:* Prem Balakrishnan; Rajeev Chamyal;
awt-...@openjdk.java.net
; Sergey Bylokhov
*Subject:* Re: Review Request: JDK-8144735 [hidpi]

Re: [OpenJDK 2D-Dev] [9] RFR JDK-6357887: selected printertray is ignored under linux

[Vadim Pakhnushev]

You see, you now have
 102 MediaTray mt = null;
 114 return( mt);

This can be just return null;
No need for new webrev for that, +1.

Vadim

On 25.08.2016 19:14, Prasanta Sadhukhan wrote:



On 8/25/2016 7:29 PM, Vadim Pakhnushev wrote:
In the PSPrinterJob.java customTray.getChoiceName() is not checked 
for null but in the UnixPrintJob it is checked.

Could this be a problem?


Checked for null just to be safe and simplified test method
Modified webrev
http://cr.openjdk.java.net/~psadhukhan/6357887/webrev.03/

Regards
Prasanta

The getMediaTray method in the test could be simplified to
for (Media m : media) {
if (m instanceof MediaTray) {
if (m.toString().trim()...) {
  return (MediaTray)m;
}
}
}
return null;

Vadim

On 25.08.2016 16:25, Philip Race wrote:

+1

-phil

On 8/24/16, 11:27 PM, Prasanta Sadhukhan wrote:

Thanks Phil for the comments.
Modified webrev:
http://cr.openjdk.java.net/~psadhukhan/6357887/webrev.02/

Regards
Prasanta
On 8/25/2016 12:05 AM, Phil Race wrote:

In fact what you should be doing here is
Attribute attr = attrs.get(Media.class);
if (attr instanceof CustomMediaTray) 

The program below should show that the lookup works so long as the 
key

is the class understood by the API - not a sub-type.

-phil.
import javax.print.*;
import javax.print.attribute.*;
import javax.print.attribute.standard.*;

public class CMT {

   public static void main(String args[]) throws Exception {

  PrintService svc = 
PrintServiceLookup.lookupPrintServices(null,null)[0];

  Media[] allMedia =
(Media[])svc.getSupportedAttributeValues(Media.class, null, null);
  PrintRequestAttributeSet aset = new 
HashPrintRequestAttributeSet();

  for (int m=0;m<allMedia.length;m++) {
Media in = allMedia[m];
aset.add(in);
Media out1 = (Media)aset.get(Media.class);
System.out.println("Class="+in.getClass()+" in="+in+ " 
out="+out1);

Media out2 = (Media)aset.get(in.getClass());
System.out.println("Class="+in.getClass()+" in="+in+ " 
out="+out2);

  }
   }



On 8/18/2016 11:20 PM, Prasanta Sadhukhan wrote:



On 8/19/2016 3:36 AM, Philip Race wrote:

Oh .. right under the covers the map is just a HashMap
and the key to the map is the class so just decides if these are
equal class objects. Hmm .. I don't know anymore if that was the 
original intent
but we probably should not mess with it right now just for the 
optimisation.
But now I am wondering why it has to be CustomMediaTray and not 
just
MediaTray ... although I suspect that in the case of CUPS we are 
not

ever mapping the media to standard ones, so they are always custom.
Yes, in CUPS 
http://hg.openjdk.java.net/jdk9/client/jdk/file/9f38d4f86e3d/src/java.desktop/unix/classes/sun/print/CUPSPrinter.java#l254
we instantiate CustomMediaTray (and do not map to standard) so I 
need to check for CustomMediatray to get the tray instance.


Regards
Prasanta
That might be arise as a problem in the case of your other open 
review regarding
validating the margins. I'll comment on that in that review but 
still surely
any MediaTray is what you would want here but it is probably 
moot if
you don't ever get a standard one. Please check into this and 
confirm

what I suspect .. or not ...

-phil.

On 8/17/16, 11:41 PM, Prasanta Sadhukhan wrote:
MediaTray values are bundled with "Media" attribute so calling 
attributes.get(CustomMediatray.class) returns null.
Modified webrev to get Media attribute and then see if the 
attribute is an instance of CustomMediatray.


http://cr.openjdk.java.net/~psadhukhan/6357887/webrev.01/

Regards
Prasanta
On 8/17/2016 9:09 PM, Philip Race wrote:

This all looks fine although this can be a simple call to
attributes.get(CustomMediaTray.class) can't it ?

502 for (int i=0; i< attrs.length; i++) {
 503 Attribute attr = attrs[i];
 504 try {
 505 if (attr instanceof CustomMediaTray) {
 506 CustomMediaTray customTray = 
(CustomMediaTray)attr;
 507 mOptions = " InputSlot="+ 
customTray.getChoiceName();

 508 }
 509 } catch (ClassCastException e) {

-phil.

On 8/10/16, 1:59 AM, Prasanta Sadhukhan wrote:

Hi All,

Please review a fix for an issue where it is seen that the 
selected printer tray is ignored in linux and default 
standard tray is used for printing.


Bug: https://bugs.openjdk.java.net/browse/JDK-6357887
webrev: 
http://cr.openjdk.java.net/~psadhukhan/6357887/webrev.00/


Issue was lpr command needs "-o InputSlot=" to 
select the printertray which was not being passed to lpr 
command.
Proposed fix added InputSlot option to lpr command to select 
the printertray.


Tested in ubuntu and solaris11.

Regards
Prasanta

Re: [OpenJDK 2D-Dev] [9] RFR JDK-6357887: selected printertray is ignored under linux

[Vadim Pakhnushev]

In the PSPrinterJob.java customTray.getChoiceName() is not checked for 
null but in the UnixPrintJob it is checked.

Could this be a problem?

The getMediaTray method in the test could be simplified to
for (Media m : media) {
if (m instanceof MediaTray) {
if (m.toString().trim()...) {
  return (MediaTray)m;
}
}
}
return null;

Vadim

On 25.08.2016 16:25, Philip Race wrote:

+1

-phil

On 8/24/16, 11:27 PM, Prasanta Sadhukhan wrote:

Thanks Phil for the comments.
Modified webrev:
http://cr.openjdk.java.net/~psadhukhan/6357887/webrev.02/

Regards
Prasanta
On 8/25/2016 12:05 AM, Phil Race wrote:

In fact what you should be doing here is
Attribute attr = attrs.get(Media.class);
if (attr instanceof CustomMediaTray) 

The program below should show that the lookup works so long as the key
is the class understood by the API - not a sub-type.

-phil.
import javax.print.*;
import javax.print.attribute.*;
import javax.print.attribute.standard.*;

public class CMT {

   public static void main(String args[]) throws Exception {

  PrintService svc = 
PrintServiceLookup.lookupPrintServices(null,null)[0];

  Media[] allMedia =
 (Media[])svc.getSupportedAttributeValues(Media.class, null, 
null);
  PrintRequestAttributeSet aset = new 
HashPrintRequestAttributeSet();

  for (int m=0;mRe: [OpenJDK 2D-Dev] RFR(XS): 8161923: Fix free in awt_PrintControl.


2016-08-25

Thread
Vadim Pakhnushev


Looks good.

Vadim

On 25.08.2016 16:42, Lindenmaier, Goetz wrote:


Hi,

Could I please get a review for this tiny fix?

http://cr.openjdk.java.net/~goetz/wr16/8161923-jdkMem/webrev.03/ 



Best regards,

  Goetz.








Re: [OpenJDK 2D-Dev] RFR: 8139176: [macosx] java.awt.TextLayout does not handle correctly the bolded logical fonts


2016-08-23

Thread
Vadim Pakhnushev


+1

Vadim

On 21.08.2016 21:31, Philip Race wrote:

Thanks Sergey .. I am still waiting for a 2nd +1 from someone else.

-phil.

On 8/12/16, 12:28 PM, Sergey Bylokhov wrote:

Looks fine.

On 12.08.16 20:15, Philip Race wrote:

Updated test : http://cr.openjdk.java.net/~prr/8139176.1/

-phil.

On 8/12/16, 10:11 AM, Philip Race wrote:



On 8/12/16, 5:48 AM, Sergey Bylokhov wrote:

Hi, Phil.
A few comments about the test:
 - jtreg can stop the test at the end of the main, before invokeLater
will start/complete.(on my system it always pass because of that)


hmm. The only reason this test shows a window is to be helpful to 
humans.

I think I'll make jtreg just draw to a BI and the UI only be there for
the "interactive" mode
which is meant to be used outside jtreg.
That will dispense with (get rid of) the need for @headful too.


 - The frame should be disposed at the end of the test.

OK .. perhaps also non-essential if I make the above change

 - Probably the correct location of the test is
java/awt/font/TextLayout/?


OK.

I will tweak the test and resubmit shortly.


On 11.08.16 0:15, Philip Race wrote:

Equals was returning true because the full name is the same for
all members of the family. That in itself seems wrong .. no two
fonts should have the same name, but that will be addressed in
a follow-on fix.


I assume the added code will be removed after follow-on fix?


It does not have to be .. it is harmless and perhaps useful. I was
thinking of leaving it.

-phil.












Re: [OpenJDK 2D-Dev] RFR: 8074843: Resolve disabled warnings for libmlib_image and libmlib_image_v


2016-08-02

Thread
Vadim Pakhnushev


Or as
j = w & 1;
if (j != 0) {
as in other longer cases. Too much parentheses to my taste.

Vadim

On 03.08.2016 1:04, Jim Graham wrote:
In that case, then I'd write it as "if ((j = (w & 1)) != 0)" to make 
it clear that the LHS is an assignment.  A casual reading of the code 
might see this as a comparison with an extra set of parentheses until 
someone counts the equal signs...


...jim

On 08/02/2016 10:38 AM, Phil Race wrote:

On 08/01/2016 10:48 PM, Prasanta Sadhukhan wrote:

+1. Only one thing,

mlib_c_ImageCopy.c#185 do we really need that extra parentheses,
((j = (w & 1))). I guess this should just do if (j = (w & 1)), isn't 
it?


I originally tried exactly that but the compiler still complained and
did insist on what you see.

Just to prove it I just now (temporarily) changed the code to what you
suggest and the result is this :-

libmlib_image/mlib_c_ImageCopy.c: In function 'mlib_c_ImageCopy_u8':
mlib_c_ImageCopy.c:331:5: error: suggest parentheses around 
assignment used as truth value [-Werror=parentheses]

 STRIP(pdst, psrc, src_width, src_height, mlib_u8);
 ^


-phil.


Regards
Prasanta
On 8/1/2016 10:43 PM, Phil Race wrote:

Looking for another "+1" ...

-phil.

On 07/29/2016 10:13 PM, Vadim Pakhnushev wrote:

Looks good!

Vadim

On 30.07.2016 6:49, Philip Race wrote:

See http://cr.openjdk.java.net/~prr/8074843.1/

Also passes JPRT

-phil.

On 7/29/16, 7:35 AM, Vadim Pakhnushev wrote:

On 29.07.2016 17:30, Philip Race wrote:



On 7/29/16, 7:00 AM, Vadim Pakhnushev wrote:

On 29.07.2016 16:28, Philip Race wrote:



On 7/29/16, 3:23 AM, Vadim Pakhnushev wrote:

Phil,

I guess you wanted to remove the lines in the 
Awt2dLibraries.gmk?


Ah, yes. Not just disable them


Do you think it's worth it to rewrite these assignments as
separate assignment and a condition?
Especially long ones with a lot of parentheses?
Like this one, instead of
if ((j = ((mlib_s32) ((mlib_addr) psrc_row & 4) >> 2))) {

j = (mlib_s32) ((mlib_addr) psrc_row & 4) >> 2;
if (j != 0) {


I don't know. Where would I stop ?


Where it doesn't feel weird anymore?
For example, is this line correct?
  if (j = (((mlib_addr) pdst_row & 2) != 0)) {
It seems very weird for me that we assign a boolean value to the
loop variable.
It looks like there's an error in parentheses and it should
instead look like:
  if ((j = (((mlib_addr) pdst_row & 2) != 0) {
Yeah, in this particular case it doesn't even matter but still
assignments in conditions can very easily lead to errors.


OK I will take a *limited* look at this.


Yeah it's just 2 identical lines in the mlib_c_ImageCopy.c







Also seeing macro calls without a semicolon is weird.
I don't see any need in parentheses in the definition of
FREE_AND_RETURN_STATUS so maybe it's possible to rewrite it
without trailing semicolon?


I anticipated the question and it is already addressed in my 
last

paragraph right at the very bottom of the review request.


Oops, missed that.

Basically that pattern has an "if (x==NULL) return". If you 
don't
have that "if" then the compiler would have objected to that 
too !


I'm not sure I undestand this.


I mean I  without the condition the compiler can tell you *never*
reach
the while (0) and so objected on those grounds. I tried this.


Got it.



I mean why not rewrite the macro like this:
#define FREE_AND_RETURN_STATUS \
if (pbuff != buff) mlib_free(pbuff); \
if (k != akernel) mlib_free(k); \
return status
#endif /* FREE_AND_RETURN_STATUS */

Yes it's prone to errors like if (foo) FREE_AND_RETURN_STATUS;
but all its usages are separate statements.


hmm .. I suppose could .. but not pretty either.
Also if going that route it could be

#define FREE_AND_RETURN_STATUS \
{ \
if (pbuff != buff) mlib_free(pbuff); \
if (k != akernel) mlib_free(k); \
} \
return status
#endif /* FREE_AND_RETURN_STATUS */

??


What's the point of parentheses here?
I thought that the whole point of curly braces and do 
while(0) stuff was to enable the macro calls in conditions like if
(foo) CALL_MACRO; without the curly braces around CALL_MACRO.
But if you put curly braces around only part of the macro
definition this would be broken anyway.

Vadim



-phil.


Vadim



Thanks,
Vadim

On 29.07.2016 2:31, Philip Race wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8074843
Fix: http://cr.openjdk.java.net/~prr/8074843/

Here's a sampling of the warnings that I think covers most,
maybe all, of the cases
LINUX
mlib_ImageAffine_NN_Bit.c:87:81: error: suggest parentheses
around '-' in operand of '&' [-Werror=parentheses]
 res = (res & ~(1 << bit)) | (((srcPixelPtr[X >>
(MLIB_SHIFT + 3)] >> (7 - (X >> MLIB_SHIFT) & 7)) & 1) <<

^
mlib_ImageAffine_NN_Bit.c:153:81: error: suggest parentheses
around '-' in operand of '&' [-Werror=parentheses]
 res = (res & ~(1 << bit)) | (((srcPixelPtr



Re: [OpenJDK 2D-Dev] RFR: 8074843: Resolve disabled warnings for libmlib_image and libmlib_image_v


2016-08-02

Thread
Vadim Pakhnushev

That's what warning said: "place parentheses *around the assignment *to 
silence this warning"


Vadim

On 02.08.2016 8:48, Prasanta Sadhukhan wrote:

+1. Only one thing,

mlib_c_ImageCopy.c#185 do we really need that extra parentheses,
((j = (w & 1))). I guess this should just do if (j = (w & 1)), isn't it? Regards
Prasanta
On 8/1/2016 10:43 PM, Phil Race wrote:

Looking for another "+1" ...

-phil.

On 07/29/2016 10:13 PM, Vadim Pakhnushev wrote:

Looks good!

Vadim

On 30.07.2016 6:49, Philip Race wrote:

See http://cr.openjdk.java.net/~prr/8074843.1/

Also passes JPRT

-phil.

On 7/29/16, 7:35 AM, Vadim Pakhnushev wrote:

On 29.07.2016 17:30, Philip Race wrote:



On 7/29/16, 7:00 AM, Vadim Pakhnushev wrote:

On 29.07.2016 16:28, Philip Race wrote:



On 7/29/16, 3:23 AM, Vadim Pakhnushev wrote:

Phil,

I guess you wanted to remove the lines in the Awt2dLibraries.gmk?


Ah, yes. Not just disable them


Do you think it's worth it to rewrite these assignments as 
separate assignment and a condition?

Especially long ones with a lot of parentheses?
Like this one, instead of
if ((j = ((mlib_s32) ((mlib_addr) psrc_row & 4) >> 2))) {

j = (mlib_s32) ((mlib_addr) psrc_row & 4) >> 2;
if (j != 0) {


I don't know. Where would I stop ?


Where it doesn't feel weird anymore?
For example, is this line correct?
  if (j = (((mlib_addr) pdst_row & 2) != 0)) {
It seems very weird for me that we assign a boolean value to the 
loop variable.
It looks like there's an error in parentheses and it should 
instead look like:

  if ((j = (((mlib_addr) pdst_row & 2) != 0) {
Yeah, in this particular case it doesn't even matter but still 
assignments in conditions can very easily lead to errors.


OK I will take a *limited* look at this.


Yeah it's just 2 identical lines in the mlib_c_ImageCopy.c







Also seeing macro calls without a semicolon is weird.
I don't see any need in parentheses in the definition of 
FREE_AND_RETURN_STATUS so maybe it's possible to rewrite it 
without trailing semicolon?


I anticipated the question and it is already addressed in my last
paragraph right at the very bottom of the review request.


Oops, missed that.


Basically that pattern has an "if (x==NULL) return". If you don't
have that "if" then the compiler would have objected to that too !


I'm not sure I undestand this.


I mean I  without the condition the compiler can tell you *never* 
reach

the while (0) and so objected on those grounds. I tried this.


Got it.



I mean why not rewrite the macro like this:
#define FREE_AND_RETURN_STATUS \
if (pbuff != buff) mlib_free(pbuff); \
if (k != akernel) mlib_free(k); \
return status
#endif /* FREE_AND_RETURN_STATUS */

Yes it's prone to errors like if (foo) FREE_AND_RETURN_STATUS; 
but all its usages are separate statements.


hmm .. I suppose could .. but not pretty either.
Also if going that route it could be

#define FREE_AND_RETURN_STATUS \
{ \
if (pbuff != buff) mlib_free(pbuff); \
if (k != akernel) mlib_free(k); \
} \
return status
#endif /* FREE_AND_RETURN_STATUS */

??


What's the point of parentheses here?
I thought that the whole point of curly braces and do  
while(0) stuff was to enable the macro calls in conditions like if 
(foo) CALL_MACRO; without the curly braces around CALL_MACRO.
But if you put curly braces around only part of the macro 
definition this would be broken anyway.


Vadim



-phil.


Vadim



Thanks,
Vadim

On 29.07.2016 2:31, Philip Race wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8074843
Fix: http://cr.openjdk.java.net/~prr/8074843/

Here's a sampling of the warnings that I think covers most, 
maybe all, of the cases

LINUX
mlib_ImageAffine_NN_Bit.c:87:81: error: suggest parentheses 
around '-' in operand of '&' [-Werror=parentheses]
 res = (res & ~(1 << bit)) | (((srcPixelPtr[X >> 
(MLIB_SHIFT + 3)] >> (7 - (X >> MLIB_SHIFT) & 7)) & 1) <<

^
mlib_ImageAffine_NN_Bit.c:153:81: error: suggest parentheses 
around '-' in operand of '&' [-Werror=parentheses]
 res = (res & ~(1 << bit)) | (((srcPixelPtr[X >> 
(MLIB_SHIFT + 3)] >> (7 - (X >> MLIB_SHIFT) & 7)) & 1) << bit);


-
mlib_c_ImageCopy.c: In function 'mlib_c_ImageCopy_s16':
mlib_c_ImageCopy.c:439:5: error: suggest parentheses around 
assignment used as truth value [-Werror=parentheses]

 STRIP(pdst, psrc, src_width, src_height, mlib_u16);
 ^
-
MAC ...

mlib_c_ImageCopy.c:331:5: error: using the result of an 
assignment as a condition without parentheses 
[-Werror,-Wparentheses]

STRIP(pdst, psrc, src_width, src_height, mlib_u8);
^
mlib_c_ImageCopy.c:185:11: note: expanded from macro 'STRIP'
if (j = w & 1) \
~~^~~
mlib_c_ImageCopy.c:331:5: note: place parentheses around the 
assignment to silence this warning\





Re: [OpenJDK 2D-Dev] [9] RFR JDK-8160736 : KSS : unnecessary class.forName in TIFFJPEGCompressor.java


2016-08-01

Thread
Vadim Pakhnushev


Why not just registry.getServiceProviders(ImageReaderSpi.class, ?

Vadim

On 01.08.2016 19:13, Jayathirth D V wrote:


Hi,

Please review the following fix in JDK9 at your convenience :

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

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



Root Cause : KSS tool has detected usage of class.forName where it can 
be avoided.


Solution : Use  instead of using class.forName.

Thanks,

Jay








Re: [OpenJDK 2D-Dev] RFR: 8074843: Resolve disabled warnings for libmlib_image and libmlib_image_v


2016-07-29

Thread
Vadim Pakhnushev


Looks good!

Vadim

On 30.07.2016 6:49, Philip Race wrote:

See http://cr.openjdk.java.net/~prr/8074843.1/

Also passes JPRT

-phil.

On 7/29/16, 7:35 AM, Vadim Pakhnushev wrote:

On 29.07.2016 17:30, Philip Race wrote:



On 7/29/16, 7:00 AM, Vadim Pakhnushev wrote:

On 29.07.2016 16:28, Philip Race wrote:



On 7/29/16, 3:23 AM, Vadim Pakhnushev wrote:

Phil,

I guess you wanted to remove the lines in the Awt2dLibraries.gmk?


Ah, yes. Not just disable them


Do you think it's worth it to rewrite these assignments as 
separate assignment and a condition?

Especially long ones with a lot of parentheses?
Like this one, instead of
if ((j = ((mlib_s32) ((mlib_addr) psrc_row & 4) >> 2))) {

j = (mlib_s32) ((mlib_addr) psrc_row & 4) >> 2;
if (j != 0) {


I don't know. Where would I stop ?


Where it doesn't feel weird anymore?
For example, is this line correct?
  if (j = (((mlib_addr) pdst_row & 2) != 0)) {
It seems very weird for me that we assign a boolean value to the 
loop variable.
It looks like there's an error in parentheses and it should instead 
look like:

  if ((j = (((mlib_addr) pdst_row & 2) != 0) {
Yeah, in this particular case it doesn't even matter but still 
assignments in conditions can very easily lead to errors.


OK I will take a *limited* look at this.


Yeah it's just 2 identical lines in the mlib_c_ImageCopy.c







Also seeing macro calls without a semicolon is weird.
I don't see any need in parentheses in the definition of 
FREE_AND_RETURN_STATUS so maybe it's possible to rewrite it 
without trailing semicolon?


I anticipated the question and it is already addressed in my last
paragraph right at the very bottom of the review request.


Oops, missed that.


Basically that pattern has an "if (x==NULL) return". If you don't
have that "if" then the compiler would have objected to that too !


I'm not sure I undestand this.


I mean I  without the condition the compiler can tell you *never* reach
the while (0) and so objected on those grounds. I tried this.


Got it.



I mean why not rewrite the macro like this:
#define FREE_AND_RETURN_STATUS \
if (pbuff != buff) mlib_free(pbuff); \
if (k != akernel) mlib_free(k); \
return status
#endif /* FREE_AND_RETURN_STATUS */

Yes it's prone to errors like if (foo) FREE_AND_RETURN_STATUS; but 
all its usages are separate statements.


hmm .. I suppose could .. but not pretty either.
Also if going that route it could be

#define FREE_AND_RETURN_STATUS \
{ \
if (pbuff != buff) mlib_free(pbuff); \
if (k != akernel) mlib_free(k); \
} \
return status
#endif /* FREE_AND_RETURN_STATUS */

??


What's the point of parentheses here?
I thought that the whole point of curly braces and do  while(0) 
stuff was to enable the macro calls in conditions like if (foo) 
CALL_MACRO; without the curly braces around CALL_MACRO.
But if you put curly braces around only part of the macro definition 
this would be broken anyway.


Vadim



-phil.


Vadim



Thanks,
Vadim

On 29.07.2016 2:31, Philip Race wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8074843
Fix: http://cr.openjdk.java.net/~prr/8074843/

Here's a sampling of the warnings that I think covers most, 
maybe all, of the cases

LINUX
mlib_ImageAffine_NN_Bit.c:87:81: error: suggest parentheses 
around '-' in operand of '&' [-Werror=parentheses]
 res = (res & ~(1 << bit)) | (((srcPixelPtr[X >> 
(MLIB_SHIFT + 3)] >> (7 - (X >> MLIB_SHIFT) & 7)) & 1) <<

^
mlib_ImageAffine_NN_Bit.c:153:81: error: suggest parentheses 
around '-' in operand of '&' [-Werror=parentheses]
 res = (res & ~(1 << bit)) | (((srcPixelPtr[X >> 
(MLIB_SHIFT + 3)] >> (7 - (X >> MLIB_SHIFT) & 7)) & 1) << bit);


-
mlib_c_ImageCopy.c: In function 'mlib_c_ImageCopy_s16':
mlib_c_ImageCopy.c:439:5: error: suggest parentheses around 
assignment used as truth value [-Werror=parentheses]

 STRIP(pdst, psrc, src_width, src_height, mlib_u16);
 ^
-
MAC ...

mlib_c_ImageCopy.c:331:5: error: using the result of an 
assignment as a condition without parentheses 
[-Werror,-Wparentheses]

STRIP(pdst, psrc, src_width, src_height, mlib_u8);
^
mlib_c_ImageCopy.c:185:11: note: expanded from macro 'STRIP'
if (j = w & 1)  \
~~^~~
mlib_c_ImageCopy.c:331:5: note: place parentheses around the 
assignment to silence this warning\


---


---
SOLARIS
mlib_ImageConv_16ext.c", line 532: statement not reached 
(E_STATEMENT_NOT_REACHED)


This last one was not nice just the ";" was considered a statement
after the {XX; YY; return Z} macro expansion
and turning into "do { {} } while (0)" did not help since
then it said "loop terminator not reached - other cases we have
like this have at least a condition in the macro.

-phil.













Re: [OpenJDK 2D-Dev] RFR: 8074843: Resolve disabled warnings for libmlib_image and libmlib_image_v


2016-07-29

Thread
Vadim Pakhnushev


On 29.07.2016 17:30, Philip Race wrote:



On 7/29/16, 7:00 AM, Vadim Pakhnushev wrote:

On 29.07.2016 16:28, Philip Race wrote:



On 7/29/16, 3:23 AM, Vadim Pakhnushev wrote:

Phil,

I guess you wanted to remove the lines in the Awt2dLibraries.gmk?


Ah, yes. Not just disable them


Do you think it's worth it to rewrite these assignments as separate 
assignment and a condition?

Especially long ones with a lot of parentheses?
Like this one, instead of
if ((j = ((mlib_s32) ((mlib_addr) psrc_row & 4) >> 2))) {

j = (mlib_s32) ((mlib_addr) psrc_row & 4) >> 2;
if (j != 0) {


I don't know. Where would I stop ?


Where it doesn't feel weird anymore?
For example, is this line correct?
  if (j = (((mlib_addr) pdst_row & 2) != 0)) {
It seems very weird for me that we assign a boolean value to the loop 
variable.
It looks like there's an error in parentheses and it should instead 
look like:

  if ((j = (((mlib_addr) pdst_row & 2) != 0) {
Yeah, in this particular case it doesn't even matter but still 
assignments in conditions can very easily lead to errors.


OK I will take a *limited* look at this.


Yeah it's just 2 identical lines in the mlib_c_ImageCopy.c







Also seeing macro calls without a semicolon is weird.
I don't see any need in parentheses in the definition of 
FREE_AND_RETURN_STATUS so maybe it's possible to rewrite it without 
trailing semicolon?


I anticipated the question and it is already addressed in my last
paragraph right at the very bottom of the review request.


Oops, missed that.


Basically that pattern has an "if (x==NULL) return". If you don't
have that "if" then the compiler would have objected to that too !


I'm not sure I undestand this.


I mean I  without the condition the compiler can tell you *never* reach
the while (0) and so objected on those grounds. I tried this.


Got it.



I mean why not rewrite the macro like this:
#define FREE_AND_RETURN_STATUS \
if (pbuff != buff) mlib_free(pbuff); \
if (k != akernel) mlib_free(k); \
return status
#endif /* FREE_AND_RETURN_STATUS */

Yes it's prone to errors like if (foo) FREE_AND_RETURN_STATUS; but 
all its usages are separate statements.


hmm .. I suppose could .. but not pretty either.
Also if going that route it could be

#define FREE_AND_RETURN_STATUS \
{ \
if (pbuff != buff) mlib_free(pbuff); \
if (k != akernel) mlib_free(k); \
} \
return status
#endif /* FREE_AND_RETURN_STATUS */

??


What's the point of parentheses here?
I thought that the whole point of curly braces and do  while(0) 
stuff was to enable the macro calls in conditions like if (foo) 
CALL_MACRO; without the curly braces around CALL_MACRO.
But if you put curly braces around only part of the macro definition 
this would be broken anyway.


Vadim



-phil.


Vadim



Thanks,
Vadim

On 29.07.2016 2:31, Philip Race wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8074843
Fix: http://cr.openjdk.java.net/~prr/8074843/

Here's a sampling of the warnings that I think covers most, maybe 
all, of the cases

LINUX
mlib_ImageAffine_NN_Bit.c:87:81: error: suggest parentheses around 
'-' in operand of '&' [-Werror=parentheses]
 res = (res & ~(1 << bit)) | (((srcPixelPtr[X >> 
(MLIB_SHIFT + 3)] >> (7 - (X >> MLIB_SHIFT) & 7)) & 1) <<

^
mlib_ImageAffine_NN_Bit.c:153:81: error: suggest parentheses 
around '-' in operand of '&' [-Werror=parentheses]
 res = (res & ~(1 << bit)) | (((srcPixelPtr[X >> 
(MLIB_SHIFT + 3)] >> (7 - (X >> MLIB_SHIFT) & 7)) & 1) << bit);


-
mlib_c_ImageCopy.c: In function 'mlib_c_ImageCopy_s16':
mlib_c_ImageCopy.c:439:5: error: suggest parentheses around 
assignment used as truth value [-Werror=parentheses]

 STRIP(pdst, psrc, src_width, src_height, mlib_u16);
 ^
-
MAC ...

mlib_c_ImageCopy.c:331:5: error: using the result of an assignment 
as a condition without parentheses [-Werror,-Wparentheses]

STRIP(pdst, psrc, src_width, src_height, mlib_u8);
^
mlib_c_ImageCopy.c:185:11: note: expanded from macro 'STRIP'
if (j = w & 1)  \
~~^~~
mlib_c_ImageCopy.c:331:5: note: place parentheses around the 
assignment to silence this warning\


---


---
SOLARIS
mlib_ImageConv_16ext.c", line 532: statement not reached 
(E_STATEMENT_NOT_REACHED)


This last one was not nice just the ";" was considered a statement
after the {XX; YY; return Z} macro expansion
and turning into "do { {} } while (0)" did not  help since
then it said "loop terminator not reached - other cases we have
like this have at least a condition in the macro.

-phil.











Re: [OpenJDK 2D-Dev] RFR: 8074843: Resolve disabled warnings for libmlib_image and libmlib_image_v


2016-07-29

Thread
Vadim Pakhnushev


On 29.07.2016 16:28, Philip Race wrote:



On 7/29/16, 3:23 AM, Vadim Pakhnushev wrote:

Phil,

I guess you wanted to remove the lines in the Awt2dLibraries.gmk?


Ah, yes. Not just disable them


Do you think it's worth it to rewrite these assignments as separate 
assignment and a condition?

Especially long ones with a lot of parentheses?
Like this one, instead of
if ((j = ((mlib_s32) ((mlib_addr) psrc_row & 4) >> 2))) {

j = (mlib_s32) ((mlib_addr) psrc_row & 4) >> 2;
if (j != 0) {


I don't know. Where would I stop ?


Where it doesn't feel weird anymore?
For example, is this line correct?
  if (j = (((mlib_addr) pdst_row & 2) != 0)) {
It seems very weird for me that we assign a boolean value to the loop 
variable.
It looks like there's an error in parentheses and it should instead look 
like:

  if ((j = (((mlib_addr) pdst_row & 2) != 0) {
Yeah, in this particular case it doesn't even matter but still 
assignments in conditions can very easily lead to errors.






Also seeing macro calls without a semicolon is weird.
I don't see any need in parentheses in the definition of 
FREE_AND_RETURN_STATUS so maybe it's possible to rewrite it without 
trailing semicolon?


I anticipated the question and it is already addressed in my last
paragraph right at the very bottom of the review request.


Oops, missed that.


Basically that pattern has an "if (x==NULL) return". If you don't
have that "if" then the compiler would have objected to that too !


I'm not sure I undestand this.

I mean why not rewrite the macro like this:
#define FREE_AND_RETURN_STATUS \
if (pbuff != buff) mlib_free(pbuff); \
if (k != akernel) mlib_free(k); \
return status
#endif /* FREE_AND_RETURN_STATUS */

Yes it's prone to errors like if (foo) FREE_AND_RETURN_STATUS; but all 
its usages are separate statements.


Vadim



Thanks,
Vadim

On 29.07.2016 2:31, Philip Race wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8074843
Fix: http://cr.openjdk.java.net/~prr/8074843/

Here's a sampling of the warnings that I think covers most, maybe 
all, of the cases

LINUX
mlib_ImageAffine_NN_Bit.c:87:81: error: suggest parentheses around 
'-' in operand of '&' [-Werror=parentheses]
 res = (res & ~(1 << bit)) | (((srcPixelPtr[X >> (MLIB_SHIFT 
+ 3)] >> (7 - (X >> MLIB_SHIFT) & 7)) & 1) <<

^
mlib_ImageAffine_NN_Bit.c:153:81: error: suggest parentheses around 
'-' in operand of '&' [-Werror=parentheses]
 res = (res & ~(1 << bit)) | (((srcPixelPtr[X >> (MLIB_SHIFT 
+ 3)] >> (7 - (X >> MLIB_SHIFT) & 7)) & 1) << bit);


-
mlib_c_ImageCopy.c: In function 'mlib_c_ImageCopy_s16':
mlib_c_ImageCopy.c:439:5: error: suggest parentheses around 
assignment used as truth value [-Werror=parentheses]

 STRIP(pdst, psrc, src_width, src_height, mlib_u16);
 ^
-
MAC ...

mlib_c_ImageCopy.c:331:5: error: using the result of an assignment 
as a condition without parentheses [-Werror,-Wparentheses]

STRIP(pdst, psrc, src_width, src_height, mlib_u8);
^
mlib_c_ImageCopy.c:185:11: note: expanded from macro 'STRIP'
if (j = w & 1)  \
~~^~~
mlib_c_ImageCopy.c:331:5: note: place parentheses around the 
assignment to silence this warning\


---


---
SOLARIS
mlib_ImageConv_16ext.c", line 532: statement not reached 
(E_STATEMENT_NOT_REACHED)


This last one was not nice just the ";" was considered a statement
after the {XX; YY; return Z} macro expansion
and turning into "do { {} } while (0)" did not  help since
then it said "loop terminator not reached - other cases we have
like this have at least a condition in the macro.

-phil.









Re: [OpenJDK 2D-Dev] RFR: 8074843: Resolve disabled warnings for libmlib_image and libmlib_image_v


2016-07-29

Thread
Vadim Pakhnushev


Phil,

I guess you wanted to remove the lines in the Awt2dLibraries.gmk?

Do you think it's worth it to rewrite these assignments as separate 
assignment and a condition?

Especially long ones with a lot of parentheses?
Like this one, instead of
if ((j = ((mlib_s32) ((mlib_addr) psrc_row & 4) >> 2))) {

j = (mlib_s32) ((mlib_addr) psrc_row & 4) >> 2;
if (j != 0) {

Also seeing macro calls without a semicolon is weird.
I don't see any need in parentheses in the definition of 
FREE_AND_RETURN_STATUS so maybe it's possible to rewrite it without 
trailing semicolon?


Thanks,
Vadim

On 29.07.2016 2:31, Philip Race wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8074843
Fix: http://cr.openjdk.java.net/~prr/8074843/

Here's a sampling of the warnings that I think covers most, maybe all, 
of the cases

LINUX
mlib_ImageAffine_NN_Bit.c:87:81: error: suggest parentheses around '-' 
in operand of '&' [-Werror=parentheses]
 res = (res & ~(1 << bit)) | (((srcPixelPtr[X >> (MLIB_SHIFT + 
3)] >> (7 - (X >> MLIB_SHIFT) & 7)) & 1) <<

^
mlib_ImageAffine_NN_Bit.c:153:81: error: suggest parentheses around 
'-' in operand of '&' [-Werror=parentheses]
 res = (res & ~(1 << bit)) | (((srcPixelPtr[X >> (MLIB_SHIFT + 
3)] >> (7 - (X >> MLIB_SHIFT) & 7)) & 1) << bit);


-
mlib_c_ImageCopy.c: In function 'mlib_c_ImageCopy_s16':
mlib_c_ImageCopy.c:439:5: error: suggest parentheses around assignment 
used as truth value [-Werror=parentheses]

 STRIP(pdst, psrc, src_width, src_height, mlib_u16);
 ^
-
MAC ...

mlib_c_ImageCopy.c:331:5: error: using the result of an assignment as 
a condition without parentheses [-Werror,-Wparentheses]

STRIP(pdst, psrc, src_width, src_height, mlib_u8);
^
mlib_c_ImageCopy.c:185:11: note: expanded from macro 'STRIP'
if (j = w & 1)  \
~~^~~
mlib_c_ImageCopy.c:331:5: note: place parentheses around the 
assignment to silence this warning\


---


---
SOLARIS
mlib_ImageConv_16ext.c", line 532: statement not reached 
(E_STATEMENT_NOT_REACHED)


This last one was not nice just the ";" was considered a statement
after the {XX; YY; return Z} macro expansion
and turning into "do { {} } while (0)" did not  help since
then it said "loop terminator not reached - other cases we have
like this have at least a condition in the macro.

-phil.







Re: [OpenJDK 2D-Dev] RFR: 8162545: Mac build failure


2016-07-26

Thread
Vadim Pakhnushev


I checked that this fix passes JPRT with Phil's fix as well.

Vadim

On 26.07.2016 18:10, Prasanta Sadhukhan wrote:

Looks good.
But strangely, I do not see this function in this file when Phil sent 
for review for his disable warning fix ;-)


Regards
Prasanta
On 7/26/2016 8:20 PM, Vadim Pakhnushev wrote:

Hi all,

Please review the build failure fix for:
https://bugs.openjdk.java.net/browse/JDK-8162545

diff -r c0cf6ec85273 
src/java.desktop/share/native/libjavajpeg/imageioJPEG.c
--- a/src/java.desktop/share/native/libjavajpeg/imageioJPEG.c Tue Jul 
26 15:55:22 2016 +0300
+++ b/src/java.desktop/share/native/libjavajpeg/imageioJPEG.c Tue Jul 
26 17:49:12 2016 +0300

@@ -2634,7 +2634,7 @@
 RELEASE_ARRAYS(env, data, NULL);
 }

-static void freeArray(void** arr, jint size) {
+static void freeArray(UINT8** arr, jint size) {
 int i;
 if (arr != NULL) {
 for (i = 0; i < size; i++) {


Thanks,
Vadim









[OpenJDK 2D-Dev] RFR: 8162545: Mac build failure


2016-07-26

Thread
Vadim Pakhnushev


Hi all,

Please review the build failure fix for:
https://bugs.openjdk.java.net/browse/JDK-8162545

diff -r c0cf6ec85273 src/java.desktop/share/native/libjavajpeg/imageioJPEG.c
--- a/src/java.desktop/share/native/libjavajpeg/imageioJPEG.c	Tue Jul 26 
15:55:22 2016 +0300
+++ b/src/java.desktop/share/native/libjavajpeg/imageioJPEG.c	Tue Jul 26 
17:49:12 2016 +0300

@@ -2634,7 +2634,7 @@
 RELEASE_ARRAYS(env, data, NULL);
 }

-static void freeArray(void** arr, jint size) {
+static void freeArray(UINT8** arr, jint size) {
 int i;
 if (arr != NULL) {
 for (i = 0; i < size; i++) {


Thanks,
Vadim





Re: [OpenJDK 2D-Dev]  RFR(S): 8161923: Fix two memory	issues.


2016-07-20

Thread
Vadim Pakhnushev


Hi Goetz,

Maybe instead of increasing the stack size we could move the increment 
from the assignment to the previous if statement where we check for the 
overwrite possibility?

There are similar code patterns in this file.
Also there is almost identical file LigatureSubstProc.cpp which also 
contains similar code.


Thanks,
Vadim

On 20.07.2016 16:13, Lindenmaier, Goetz wrote:


Hi

This changes fixes two memory issues.

In awt_PrintControl.cpp, a wrong pointer is freed.

In LigatureSubstProc2.cpp, line 157:

stack[++mm] = componentGlyph;

can overwrite the stack by one element. It will write

stack[nComponents], because ++mm increments before

accessing the array.

Fix: increase the size of the array by one.

Please review this change:

http://cr.openjdk.java.net/~goetz/wr16/8161923-jdkMem/webrev.01/ 



Best regards,

  Goetz.








Re: [OpenJDK 2D-Dev] RFR: 8047931: Remove unused medialib code


2016-05-04

Thread
Vadim Pakhnushev


Hi Andrew,

Thanks for the review!
I think I'll file a follow-up bug for unneeded data types and 
corresponding functions removal.


Vadim

On 04.05.2016 18:18, Andrew Brygin wrote:

Hello Vadim,

  the change looks fine to me.

  Probably, there is more room for optimization: we actually use only mlib 
images
  with types MLIB_BYTE, and MLIB_SHORT (see allocateArray() and 
allocateReasterArray())
  so routines for other types likely can be eliminated.

Thanks,
Andrew


On May 4, 2016, at 1:08 PM, Vadim Pakhnushev <vadim.pakhnus...@oracle.com> 
wrote:

Ping?

On 25.04.2016 20:13, Vadim Pakhnushev wrote:

Hi all,

Please review this cleanup fix:
https://bugs.openjdk.java.net/browse/JDK-8047931

While investigating unused files in the sparc build, it was found that there 
are a lot of unused code in the medialib.
So without further ado, here's the whopping 35000 lines removal webrev:
http://cr.openjdk.java.net/~vadim/8047931/webrev.00/

At least on Windows, it reduced the size of the mlib_image.dll by almost 180KB 
from 663KB to 486KB and shortened the build by 3 minutes on my i5.
It was tested with the JPRT build on all platforms.

Some notes in order of the files in the webrev:

Awt2dLibraries.gmk
Deleted removed files from the sparc exclude list.

mlib_ImageAffine.c
Main entry point mlib_ImageAffine always pass NULL as a colormap to the 
mlib_ImageAffine_alltypes.
This is basically a starting point for the majority of removal which leads to 
removal of colormap parameters of several other functions and elimination of 
several if (colormap != NULL) blocks.
Which in turn leads to removal of function pointer arrays such as 
mlib_AffineFunArr_bl_i and mlib_AffineFunArr_bc_i.

mlib_ImageAffine.h
Removed colormap parameters and related declarations.

mlib_ImageAffineEdge.c
Removed parts of the code in the mlib_ImageAffineEdge* functions related to the 
colormaps.

mlib_ImageConv*
mlib_v_ImageConv*
Removed 2x2 through 7x7 convolution functions as we only use NxM ones.

mlib_image_proto.h
Basically removed all functions with a colormap parameter.

mlib_v_ImageAffine*
Removed sparc implementations of unused mlib_ImageAffine* functions.

Thanks,
Vadim









Re: [OpenJDK 2D-Dev] RFR: 8047931: Remove unused medialib code


2016-05-04

Thread
Vadim Pakhnushev


Ping?

On 25.04.2016 20:13, Vadim Pakhnushev wrote:

Hi all,

Please review this cleanup fix:
https://bugs.openjdk.java.net/browse/JDK-8047931

While investigating unused files in the sparc build, it was found that 
there are a lot of unused code in the medialib.

So without further ado, here's the whopping 35000 lines removal webrev:
http://cr.openjdk.java.net/~vadim/8047931/webrev.00/

At least on Windows, it reduced the size of the mlib_image.dll by 
almost 180KB from 663KB to 486KB and shortened the build by 3 minutes 
on my i5.

It was tested with the JPRT build on all platforms.

Some notes in order of the files in the webrev:

Awt2dLibraries.gmk
Deleted removed files from the sparc exclude list.

mlib_ImageAffine.c
Main entry point mlib_ImageAffine always pass NULL as a colormap to 
the mlib_ImageAffine_alltypes.
This is basically a starting point for the majority of removal which 
leads to removal of colormap parameters of several other functions and 
elimination of several if (colormap != NULL) blocks.
Which in turn leads to removal of function pointer arrays such as 
mlib_AffineFunArr_bl_i and mlib_AffineFunArr_bc_i.


mlib_ImageAffine.h
Removed colormap parameters and related declarations.

mlib_ImageAffineEdge.c
Removed parts of the code in the mlib_ImageAffineEdge* functions 
related to the colormaps.


mlib_ImageConv*
mlib_v_ImageConv*
Removed 2x2 through 7x7 convolution functions as we only use NxM ones.

mlib_image_proto.h
Basically removed all functions with a colormap parameter.

mlib_v_ImageAffine*
Removed sparc implementations of unused mlib_ImageAffine* functions.

Thanks,
Vadim








Re: [OpenJDK 2D-Dev] RFR: 8039444: Swing applications not being displayed properly


2016-04-28

Thread
Vadim Pakhnushev


+1

Vadim

On 28.04.2016 23:52, Phil Race wrote:

Bug: https://bugs.openjdk.java.net/browse/JDK-8039444
Webrev: http://cr.openjdk.java.net/~prr/8039444/

This makes 9 consistent in black-listing all intel chipsets as we did 
in 8u40 :


http://mail.openjdk.java.net/pipermail/2d-dev/2014-November/004939.html

Ideally we would not have to do that but we have not made any solid
progress in being to 'white list' any.

One thing I intend to do is make sure there is a way to force it on 
regardless.

That will be a follow-up.

-phil.









[OpenJDK 2D-Dev] RFR: 8047931: Remove unused medialib code


2016-04-25

Thread
Vadim Pakhnushev


Hi all,

Please review this cleanup fix:
https://bugs.openjdk.java.net/browse/JDK-8047931

While investigating unused files in the sparc build, it was found that 
there are a lot of unused code in the medialib.

So without further ado, here's the whopping 35000 lines removal webrev:
http://cr.openjdk.java.net/~vadim/8047931/webrev.00/

At least on Windows, it reduced the size of the mlib_image.dll by almost 
180KB from 663KB to 486KB and shortened the build by 3 minutes on my i5.

It was tested with the JPRT build on all platforms.

Some notes in order of the files in the webrev:

Awt2dLibraries.gmk
Deleted removed files from the sparc exclude list.

mlib_ImageAffine.c
Main entry point mlib_ImageAffine always pass NULL as a colormap to the 
mlib_ImageAffine_alltypes.
This is basically a starting point for the majority of removal which 
leads to removal of colormap parameters of several other functions and 
elimination of several if (colormap != NULL) blocks.
Which in turn leads to removal of function pointer arrays such as 
mlib_AffineFunArr_bl_i and mlib_AffineFunArr_bc_i.


mlib_ImageAffine.h
Removed colormap parameters and related declarations.

mlib_ImageAffineEdge.c
Removed parts of the code in the mlib_ImageAffineEdge* functions related 
to the colormaps.


mlib_ImageConv*
mlib_v_ImageConv*
Removed 2x2 through 7x7 convolution functions as we only use NxM ones.

mlib_image_proto.h
Basically removed all functions with a colormap parameter.

mlib_v_ImageAffine*
Removed sparc implementations of unused mlib_ImageAffine* functions.

Thanks,
Vadim





Re: [OpenJDK 2D-Dev] RFR: 8055463: Needs public API allowing full access to font collections in Font.createFont()


2016-03-21

Thread
Vadim Pakhnushev


Phil,

Now the javadoc for both java.awt.Font.createFonts methods is incorrect:
* The explicit purpose of this overloading of the
* {@code createFont(int, InputStream)} method

It's not an overload anymore.

Other than that, looks good.

Vadim

On 21.03.2016 20:52, Phil Race wrote:
After some CCC discussion the method name was changed from createFont 
to createFonts

since it is expected to suport returning > 1. No other changes.
http://cr.openjdk.java.net/~prr/8055463.2

I am still looking for a 2nd reviewer !

-phil.


On 03/12/2016 08:22 AM, Sergey Bylokhov wrote:

Looks fine.
ps: for the record - long time ago the OSX code was different:
http://hg.openjdk.java.net/macosx-port/macosx-port/jdk/rev/e910dde2c36c

On 11.03.16 23:34, Phil Race wrote:

Oops. Fixed : http://cr.openjdk.java.net/~prr/8055463.1/

-phil.

On 03/11/2016 12:22 PM, Sergey Bylokhov wrote:

Hi, Phil.
The FileFont.java contains debug code:

System.out.println("COUNT="+count);
System.out.println("FILE="+fontFile);


On 11.03.16 2:02, Phil Race wrote:

PS there are a couple of other test files updated here

BigFont is one of the tests I ran whilst checking I didn't create
regressions
and it failed because it assumes a SecurityManager. I could not 
find an

open bug on that.*

*GetLCIDFromLocale happened to be the one I chose to paste in
the GPL and then I noticed it incorrectly had the classpath exception
so I am fixing that here too.

-phil.


On 03/10/2016 02:53 PM, Phil Race wrote:

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

Proposal is to add two new methods in the Font class paralleling the
existing ones
that return only a single font.

Font[] createFont(File)
Font[] createFont(InputStream)

I tried to make the single & multiple code be shared as much as
possible
to avoid duplication and I eliminated code in the OSX sub-class 
which

seemed to be a pointless copy of the superclass.

The test checks the various assertions I intend by probing for
platform fonts.

-phil.



















Re: [OpenJDK 2D-Dev] RFR: 8143177: Integrate harfbuzz opentype layout engine per JEP 258


2015-11-22

Thread
Vadim Pakhnushev


Looks good to me.

Vadim

On 22.11.2015 2:07, Philip Race wrote:

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

is uploaded. See comments below.

On 11/21/15, 2:41 AM, Vadim Pakhnushev wrote:

Phil,

In the HBShaper.c in the init_JNI_IDs function I think CHECK_NULL is 
missing for the gvdIndicesFID.

I guess it's just a copy from SunLayoutEngine.cpp but still.


Actually this points out that I only return from the init function
and don't return from the calling function. I will restructure this a 
bit so that we

return a success value and the caller checks it.

Calling init_JNI_IDs each time seems unneeded (not very harmful 
though), could it be somehow merged with the 
Java_sun_font_SunLayoutEngine_initGVIDs?


They are static and I have added a static flag that indicates they 
have all been initialised.
If it fails then we will not proceed. In the normal case overhead 
should be down to

near zero.

What's the point of this super optimized euclidianDistance if it's 
called only 2 times while creating a layout?


I just copied this from ICU code so we are doing the same thing.


pScaler is not used anywhere it seems, can it be removed altogether?
It was used when I was directly testing freetype (using harfbuzz's 
hb-ft.cc)

That was where I started out but I moved on to a different approach.
It is left it there in case I wanted to test ft to isolate any bugs.



typo in the hb-jdk-font.cc:124* which *be could* based on some 
on-the fly glyph analysis.


ok

I had to make one additional change. The hb call to create a coretext font
was being handed the pt size in device pixels as used elsewhere but in
the coretext path we must pass the user pt size so I had to pass that
down and in .. it does not affect anything except the coretext path for
AAT fonts on OS X. I spotted this when testing with a transform in
that code path.

-phil.


Thanks,
Vadim

On 21.11.2015 2:09, Phil Race wrote:


On 11/19/2015 12:51 PM, Steven Loomis wrote:
I have reviewed the non-harfbuzz portion (outside of the /harfbuzz/ 
directory) and it looks good so far.


Thanks. The harfbuzz code itself probably does not need careful 
reviewing as it is just
the upstream harfbuzz. I have updated it to remove the gpl text from 
the hb src files but
I haven't changed anything else :- 
http://cr.openjdk.java.net/~prr/8143177.1/


Any takers to be 2nd reviewer ?

-phil.



-s

On 11/17/2015 4:05 PM, Philip Race wrote:

Webrev : http://cr.openjdk.java.net/~prr/8143177/
Bug : https://bugs.openjdk.java.net/browse/JDK-8143177

This webrev contains the changes accumulated in the harfbuzz 
project forest
that migrate JDK from using ICU for opentype font layout to 
harfbuzz for the same.


The change does not introduce API. It is mostly adding the 
harfbuzz library.

The ICU library remains there but is no longer the default.
Eventually (not necessarily in 9) it may be removed once we are 
comfortable

with harfbuzz.
You may select ICU by using -Dsun.font.layoutengine=icu

You may see which layout engine is in use by using 
-Dsun.font.layoutengine.verbose=true

it will print either "Using harfbuzz", or "Using ICU".

The change has no impact on typical Latin script rendering but is 
a big advance
for complex scripts and also applies if you select kerning or 
ligatures for Latin.
However the latter is only detectable if the font implements 
support for these.
By "big advance" I mean that ICU has not been updated to recognise 
recent opentype features.
So harfbuzz should fix a number of things but unexpected changes 
that look wrong

should be reported so we can investigate.

To use harfbuzz we invoke its shaper and we provide a way to get 
jdk font information.
This means that we do not need a different layer depending on 
whether freetype or t2k

is used. It also enables some caching in the JDK font layer.

On macosx harfbuzz does not natively read the AAT tables but will 
invoke CoreText.
This does mean that an AAT font installed on Linux would not be 
processed but
this is not a significant issue since AAT fonts are not found 
other than on macosx.


The majority of the files in the webrev are harfbuzz itself, 
changed only to comply
with JDK whitespace rules. Reviewers should probably concentrate 
on all of the rest.
I've listed it so that all those files are at the beginning and 
you can ignore all those

that follow that are in the "harfbuzz" directory.

The harfbuzz version used here is 1.0.6 which is the latest source 
release (at the time of writing).

We expect to update this to keep reasonably current.

I would like to push this in on Friday, but at the very latest 
Monday because of the
upcoming Feature Complete date so there are a couple of days to 
make small
tweaks for serious problems but there will be plenty of time after 
integration to fix

remaining problems.

-phil.














Re: [OpenJDK 2D-Dev] RFR: 8143177: Integrate harfbuzz opentype layout engine per JEP 258


2015-11-21

Thread
Vadim Pakhnushev


Phil,

In the HBShaper.c in the init_JNI_IDs function I think CHECK_NULL is 
missing for the gvdIndicesFID.

I guess it's just a copy from SunLayoutEngine.cpp but still.
Calling init_JNI_IDs each time seems unneeded (not very harmful though), 
could it be somehow merged with the Java_sun_font_SunLayoutEngine_initGVIDs?
What's the point of this super optimized euclidianDistance if it's 
called only 2 times while creating a layout?

pScaler is not used anywhere it seems, can it be removed altogether?

typo in the hb-jdk-font.cc:124* which *be could* based on some 
on-the fly glyph analysis.


Thanks,
Vadim

On 21.11.2015 2:09, Phil Race wrote:


On 11/19/2015 12:51 PM, Steven Loomis wrote:
I have reviewed the non-harfbuzz portion (outside of the /harfbuzz/ 
directory) and it looks good so far.


Thanks. The harfbuzz code itself probably does not need careful 
reviewing as it is just
the upstream harfbuzz. I have updated it to remove the gpl text from 
the hb src files but
I haven't changed anything else :- 
http://cr.openjdk.java.net/~prr/8143177.1/


Any takers to be 2nd reviewer ?

-phil.



-s

On 11/17/2015 4:05 PM, Philip Race wrote:

Webrev : http://cr.openjdk.java.net/~prr/8143177/
Bug : https://bugs.openjdk.java.net/browse/JDK-8143177

This webrev contains the changes accumulated in the harfbuzz project 
forest
that migrate JDK from using ICU for opentype font layout to harfbuzz 
for the same.


The change does not introduce API. It is mostly adding the harfbuzz 
library.

The ICU library remains there but is no longer the default.
Eventually (not necessarily in 9) it may be removed once we are 
comfortable

with harfbuzz.
You may select ICU by using -Dsun.font.layoutengine=icu

You may see which layout engine is in use by using 
-Dsun.font.layoutengine.verbose=true

it will print either "Using harfbuzz", or "Using ICU".

The change has no impact on typical Latin script rendering but is a 
big advance
for complex scripts and also applies if you select kerning or 
ligatures for Latin.
However the latter is only detectable if the font implements support 
for these.
By "big advance" I mean that ICU has not been updated to recognise 
recent opentype features.
So harfbuzz should fix a number of things but unexpected changes 
that look wrong

should be reported so we can investigate.

To use harfbuzz we invoke its shaper and we provide a way to get jdk 
font information.
This means that we do not need a different layer depending on 
whether freetype or t2k

is used. It also enables some caching in the JDK font layer.

On macosx harfbuzz does not natively read the AAT tables but will 
invoke CoreText.
This does mean that an AAT font installed on Linux would not be 
processed but
this is not a significant issue since AAT fonts are not found other 
than on macosx.


The majority of the files in the webrev are harfbuzz itself, changed 
only to comply
with JDK whitespace rules. Reviewers should probably concentrate on 
all of the rest.
I've listed it so that all those files are at the beginning and you 
can ignore all those

that follow that are in the "harfbuzz" directory.

The harfbuzz version used here is 1.0.6 which is the latest source 
release (at the time of writing).

We expect to update this to keep reasonably current.

I would like to push this in on Friday, but at the very latest 
Monday because of the
upcoming Feature Complete date so there are a couple of days to make 
small
tweaks for serious problems but there will be plenty of time after 
integration to fix

remaining problems.

-phil.












Re: [OpenJDK 2D-Dev]  Review request for JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing


2015-10-20

Thread
Vadim Pakhnushev


+1

On 20.10.2015 8:31, Jayathirth D V wrote:

Hi Vadim,

Thanks for throwing light on performance aspect of Boxing & Unboxing in Java.

I have made changes, so that we use Float.compare and then use equality operator to 
determine whether expected & returned values are same.

Please find updated Webrev:

http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.07/

Please review.

Thanks,
Jay

-Original Message-
From: Vadim Pakhnushev
Sent: Monday, October 19, 2015 4:50 PM
To: Jayathirth D V; Sergey Bylokhov; 2d-dev@openjdk.java.net; Philip Race
Subject: Re: [OpenJDK 2D-Dev]  Review request for JDK-7182758: 
BMPMetadata returns invalid PhysicalPixelSpacing

Jay,
What I mean is that you can either declare two floats (lowercase) and then you 
need to do if (Float.compare(f1, f2) == 0) Or you declare a Float f1 and then 
you can write if (f1.equals(f2)) In the first case there isn't any boxing, 
while in the second case second float will be boxed (and unboxed in the equals 
method).
So technically Float.compare(f1, f2) == 0 is the most efficient way to compare 
two primitive floats, especially given that Float.parseFloat returns primitive 
float.
In this particular case simple == would be sufficient though, since one of the 
floats is computed at compile time and you know that you won't be comparing 
NaN's and expecting that they are equal...

I'm OK with both approaches, but would prefer primitive types and 
(Float.compare(f1, f2) == 0).

Thanks,
Vadim

On 19.10.2015 14:04, Jayathirth D V wrote:

Hi Vadim,

I think doing compare and then equals, actually increases the computation we 
are doing to check whether expected value and returned value are same.

New approach of directly using equals to compare between expected and returned 
value is efficient.

I have made changes you mentioned regarding the typo in "spacing".  Please find 
updated Webrev :

http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.06/

Please review so that we can push the change.

Thanks,
Jay

-Original Message-
From: Vadim Pakhnushev
Sent: Monday, October 19, 2015 4:03 PM
To: Jayathirth D V; Sergey Bylokhov; 2d-dev@openjdk.java.net; Philip
Race
Subject: Re: [OpenJDK 2D-Dev]  Review request for
JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing

Hi Jay,

I'm sorry, actually the usage of Float.compare was perfectly fine in your case, 
given that you were comparing floats (not Floats).
The thing which caught my eye was the use of Integer boxing as Sergey pointed 
out.
Sorry about the confusion.

Thanks,
Vadim

On 19.10.2015 12:04, Jayathirth D V wrote:

Hi Vadim,

Thanks for the review.
I have made suggested changes. Updated Webrev :
http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.05/

Please review.

Thanks,
Jay

-Original Message-
From: Vadim Pakhnushev
Sent: Friday, October 16, 2015 8:15 PM
To: Jayathirth D V; Sergey Bylokhov; 2d-dev@openjdk.java.net; Philip
Race
Subject: Re: [OpenJDK 2D-Dev]  Review request for
JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing

Hi Jay,

What's the point of using Float.compare in the test?
Why not just check if
(horizontalNodeValue.equals(expectedHorizontalValue)) ?
Also please capitalize Attr in the declaration of horizontalattr and 
verticalattr.

Thanks,
Vadim

On 16.10.2015 17:36, Jayathirth D V wrote:

Hello All,

Can I get one more review please.

Thanks,
Jay

-Original Message-
From: Sergey Bylokhov
Sent: Thursday, October 15, 2015 6:05 PM
To: Jayathirth D V; 2d-dev@openjdk.java.net; Philip Race
Subject: Re:  Review request for JDK-7182758:
BMPMetadata returns invalid PhysicalPixelSpacing

The fix looks fine. The test can be improved a little bit to simplify the 
int->Integer boxing, but it is not necessary for now.

Thanks.

On 15.10.15 13:17, Jayathirth D V wrote:

Hi Sergey,

I thought you suggested to check for tighter "true" condition instead of 
"false" condition.

I have made changes to map horizontalNodeValue and verticalNodeValue to 
expected values. Please find updated Webrev link:

http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.04/

Please review.

Thanks,
Jay

-Original Message-
From: Sergey Bylokhov
Sent: Wednesday, October 14, 2015 7:34 PM
To: Jayathirth D V; 2d-dev@openjdk.java.net; Philip Race
Subject: Re:  Review request for JDK-7182758:
BMPMetadata returns invalid PhysicalPixelSpacing

Hi, Jay.

I suggest to check correct/expected result in the test instead of previous 
incorrect zero.

Here, I suggested to check that the resulted horizontalNodeValue and verticalNodeValue 
are equal to some expected value. Because if it bigger than zero does not mean it is 
correct(note to use Float.compare(f1, f2) instead of "==").


Previously I forgot to mention, that it will be good to name the test by some 
useful name instead of some bug number(see examples in javax/imageio/plugins).

On 13.10.15 11:12, Jayathirth D V wrote:

Hello All,

Removed Traili



Re: [OpenJDK 2D-Dev]  Review request for JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing


2015-10-19

Thread
Vadim Pakhnushev


Jay,
What I mean is that you can either declare two floats (lowercase) and 
then you need to do if (Float.compare(f1, f2) == 0)

Or you declare a Float f1 and then you can write if (f1.equals(f2))
In the first case there isn't any boxing, while in the second case 
second float will be boxed (and unboxed in the equals method).
So technically Float.compare(f1, f2) == 0 is the most efficient way to 
compare two primitive floats, especially given that Float.parseFloat 
returns primitive float.
In this particular case simple == would be sufficient though, since one 
of the floats is computed at compile time and you know that you won't be 
comparing NaN's and expecting that they are equal...


I'm OK with both approaches, but would prefer primitive types and 
(Float.compare(f1, f2) == 0).


Thanks,
Vadim

On 19.10.2015 14:04, Jayathirth D V wrote:

Hi Vadim,

I think doing compare and then equals, actually increases the computation we 
are doing to check whether expected value and returned value are same.

New approach of directly using equals to compare between expected and returned 
value is efficient.

I have made changes you mentioned regarding the typo in "spacing".  Please find 
updated Webrev :

http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.06/

Please review so that we can push the change.

Thanks,
Jay

-Original Message-----
From: Vadim Pakhnushev
Sent: Monday, October 19, 2015 4:03 PM
To: Jayathirth D V; Sergey Bylokhov; 2d-dev@openjdk.java.net; Philip Race
Subject: Re: [OpenJDK 2D-Dev]  Review request for JDK-7182758: 
BMPMetadata returns invalid PhysicalPixelSpacing

Hi Jay,

I'm sorry, actually the usage of Float.compare was perfectly fine in your case, 
given that you were comparing floats (not Floats).
The thing which caught my eye was the use of Integer boxing as Sergey pointed 
out.
Sorry about the confusion.

Thanks,
Vadim

On 19.10.2015 12:04, Jayathirth D V wrote:

Hi Vadim,

Thanks for the review.
I have made suggested changes. Updated Webrev :
http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.05/

Please review.

Thanks,
Jay

-Original Message-----
From: Vadim Pakhnushev
Sent: Friday, October 16, 2015 8:15 PM
To: Jayathirth D V; Sergey Bylokhov; 2d-dev@openjdk.java.net; Philip
Race
Subject: Re: [OpenJDK 2D-Dev]  Review request for
JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing

Hi Jay,

What's the point of using Float.compare in the test?
Why not just check if
(horizontalNodeValue.equals(expectedHorizontalValue)) ?
Also please capitalize Attr in the declaration of horizontalattr and 
verticalattr.

Thanks,
Vadim

On 16.10.2015 17:36, Jayathirth D V wrote:

Hello All,

Can I get one more review please.

Thanks,
Jay

-Original Message-
From: Sergey Bylokhov
Sent: Thursday, October 15, 2015 6:05 PM
To: Jayathirth D V; 2d-dev@openjdk.java.net; Philip Race
Subject: Re:  Review request for JDK-7182758:
BMPMetadata returns invalid PhysicalPixelSpacing

The fix looks fine. The test can be improved a little bit to simplify the 
int->Integer boxing, but it is not necessary for now.

Thanks.

On 15.10.15 13:17, Jayathirth D V wrote:

Hi Sergey,

I thought you suggested to check for tighter "true" condition instead of 
"false" condition.

I have made changes to map horizontalNodeValue and verticalNodeValue to 
expected values. Please find updated Webrev link:

http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.04/

Please review.

Thanks,
Jay

-Original Message-
From: Sergey Bylokhov
Sent: Wednesday, October 14, 2015 7:34 PM
To: Jayathirth D V; 2d-dev@openjdk.java.net; Philip Race
Subject: Re:  Review request for JDK-7182758:
BMPMetadata returns invalid PhysicalPixelSpacing

Hi, Jay.

I suggest to check correct/expected result in the test instead of previous 
incorrect zero.

Here, I suggested to check that the resulted horizontalNodeValue and verticalNodeValue 
are equal to some expected value. Because if it bigger than zero does not mean it is 
correct(note to use Float.compare(f1, f2) instead of "==").


Previously I forgot to mention, that it will be good to name the test by some 
useful name instead of some bug number(see examples in javax/imageio/plugins).

On 13.10.15 11:12, Jayathirth D V wrote:

Hello All,

Removed Trailing whitespace present in code.

Please find updated webrev link:

http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.02/

Thanks,

Jay

*From:* Jayathirth D V
*Sent:* Monday, October 12, 2015 2:15 PM
*To:* 2d-dev@openjdk.java.net; Philip Race; Sergey Bylokhov
*Subject:* [OpenJDK 2D-Dev]  Review request for
JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing

Hello All,

Made small change on how we need to represent floating point
constant in
Java(1000.0 -> 1000.0F).

Please find updated Webrev link:

Webrev :
http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.01/

Please review.

Thanks,

Jay

*From:* Jayathirth D V
*Sent:* Thursday, October 08, 2015



Re: [OpenJDK 2D-Dev]  Review request for JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing


2015-10-19

Thread
Vadim Pakhnushev


Hi Jay,

I'm sorry, actually the usage of Float.compare was perfectly fine in 
your case, given that you were comparing floats (not Floats).
The thing which caught my eye was the use of Integer boxing as Sergey 
pointed out.

Sorry about the confusion.

Thanks,
Vadim

On 19.10.2015 12:04, Jayathirth D V wrote:

Hi Vadim,

Thanks for the review.
I have made suggested changes. Updated Webrev :
http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.05/

Please review.

Thanks,
Jay

-Original Message-
From: Vadim Pakhnushev
Sent: Friday, October 16, 2015 8:15 PM
To: Jayathirth D V; Sergey Bylokhov; 2d-dev@openjdk.java.net; Philip Race
Subject: Re: [OpenJDK 2D-Dev]  Review request for JDK-7182758: 
BMPMetadata returns invalid PhysicalPixelSpacing

Hi Jay,

What's the point of using Float.compare in the test?
Why not just check if
(horizontalNodeValue.equals(expectedHorizontalValue)) ?
Also please capitalize Attr in the declaration of horizontalattr and 
verticalattr.

Thanks,
Vadim

On 16.10.2015 17:36, Jayathirth D V wrote:

Hello All,

Can I get one more review please.

Thanks,
Jay

-Original Message-
From: Sergey Bylokhov
Sent: Thursday, October 15, 2015 6:05 PM
To: Jayathirth D V; 2d-dev@openjdk.java.net; Philip Race
Subject: Re:  Review request for JDK-7182758:
BMPMetadata returns invalid PhysicalPixelSpacing

The fix looks fine. The test can be improved a little bit to simplify the 
int->Integer boxing, but it is not necessary for now.

Thanks.

On 15.10.15 13:17, Jayathirth D V wrote:

Hi Sergey,

I thought you suggested to check for tighter "true" condition instead of 
"false" condition.

I have made changes to map horizontalNodeValue and verticalNodeValue to 
expected values. Please find updated Webrev link:

http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.04/

Please review.

Thanks,
Jay

-Original Message-
From: Sergey Bylokhov
Sent: Wednesday, October 14, 2015 7:34 PM
To: Jayathirth D V; 2d-dev@openjdk.java.net; Philip Race
Subject: Re:  Review request for JDK-7182758:
BMPMetadata returns invalid PhysicalPixelSpacing

Hi, Jay.

I suggest to check correct/expected result in the test instead of previous 
incorrect zero.

Here, I suggested to check that the resulted horizontalNodeValue and verticalNodeValue 
are equal to some expected value. Because if it bigger than zero does not mean it is 
correct(note to use Float.compare(f1, f2) instead of "==").


Previously I forgot to mention, that it will be good to name the test by some 
useful name instead of some bug number(see examples in javax/imageio/plugins).

On 13.10.15 11:12, Jayathirth D V wrote:

Hello All,

Removed Trailing whitespace present in code.

Please find updated webrev link:

http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.02/

Thanks,

Jay

*From:* Jayathirth D V
*Sent:* Monday, October 12, 2015 2:15 PM
*To:* 2d-dev@openjdk.java.net; Philip Race; Sergey Bylokhov
*Subject:* [OpenJDK 2D-Dev]  Review request for
JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing

Hello All,

Made small change on how we need to represent floating point
constant in
Java(1000.0 -> 1000.0F).

Please find updated Webrev link:

Webrev :
http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.01/

Please review.

Thanks,

Jay

*From:* Jayathirth D V
*Sent:* Thursday, October 08, 2015 2:20 PM
*To:* 2d-dev@openjdk.java.net <mailto:2d-dev@openjdk.java.net>;
Philip Race; Sergey Bylokhov
*Subject:* [OpenJDK 2D-Dev]  Review request for
JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing

Hello All,

Please review following fix in jdk9:

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

Webrev :
http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.00/

Bug : BMPMetadata returns invalid PhysicalPixelSpacing

Root cause : Whenever XPixelsPerMter or YPixelsPerMeter is more
than value 1 in BMP header. Horizontal & Vertical Physical pixel
spacing were returned as zero.

   In getStandardDimensionNode() method
of BMPMetadata.java we are dividing 1 by XPixelsPerMter/ YPixelsPerMter.
When

   XPixelsPerMter/ YPixelsPerMter is
more than 1. Resulted value is stored without decimal part, which resulted in 
zero.

Solution : Made changes to how Horizontal & Vertical Physical pixel
spacing is calculated so that decimal value is not truncated.

Thanks,

Jay


--
Best regards, Sergey.


--
Best regards, Sergey.


--
Best regards, Sergey.







Re: [OpenJDK 2D-Dev]  Review request for JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing


2015-10-19

Thread
Vadim Pakhnushev


Looks good,
Please fix the typo before the push here ("spcaing"):
throw new RuntimeException("Invalid pixel spcaing");

Thanks,
Vadim

On 19.10.2015 12:04, Jayathirth D V wrote:

Hi Vadim,

Thanks for the review.
I have made suggested changes. Updated Webrev :
http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.05/

Please review.

Thanks,
Jay

-Original Message-
From: Vadim Pakhnushev
Sent: Friday, October 16, 2015 8:15 PM
To: Jayathirth D V; Sergey Bylokhov; 2d-dev@openjdk.java.net; Philip Race
Subject: Re: [OpenJDK 2D-Dev]  Review request for JDK-7182758: 
BMPMetadata returns invalid PhysicalPixelSpacing

Hi Jay,

What's the point of using Float.compare in the test?
Why not just check if
(horizontalNodeValue.equals(expectedHorizontalValue)) ?
Also please capitalize Attr in the declaration of horizontalattr and 
verticalattr.

Thanks,
Vadim

On 16.10.2015 17:36, Jayathirth D V wrote:

Hello All,

Can I get one more review please.

Thanks,
Jay

-Original Message-
From: Sergey Bylokhov
Sent: Thursday, October 15, 2015 6:05 PM
To: Jayathirth D V; 2d-dev@openjdk.java.net; Philip Race
Subject: Re:  Review request for JDK-7182758:
BMPMetadata returns invalid PhysicalPixelSpacing

The fix looks fine. The test can be improved a little bit to simplify the 
int->Integer boxing, but it is not necessary for now.

Thanks.

On 15.10.15 13:17, Jayathirth D V wrote:

Hi Sergey,

I thought you suggested to check for tighter "true" condition instead of 
"false" condition.

I have made changes to map horizontalNodeValue and verticalNodeValue to 
expected values. Please find updated Webrev link:

http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.04/

Please review.

Thanks,
Jay

-Original Message-
From: Sergey Bylokhov
Sent: Wednesday, October 14, 2015 7:34 PM
To: Jayathirth D V; 2d-dev@openjdk.java.net; Philip Race
Subject: Re:  Review request for JDK-7182758:
BMPMetadata returns invalid PhysicalPixelSpacing

Hi, Jay.

I suggest to check correct/expected result in the test instead of previous 
incorrect zero.

Here, I suggested to check that the resulted horizontalNodeValue and verticalNodeValue 
are equal to some expected value. Because if it bigger than zero does not mean it is 
correct(note to use Float.compare(f1, f2) instead of "==").


Previously I forgot to mention, that it will be good to name the test by some 
useful name instead of some bug number(see examples in javax/imageio/plugins).

On 13.10.15 11:12, Jayathirth D V wrote:

Hello All,

Removed Trailing whitespace present in code.

Please find updated webrev link:

http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.02/

Thanks,

Jay

*From:* Jayathirth D V
*Sent:* Monday, October 12, 2015 2:15 PM
*To:* 2d-dev@openjdk.java.net; Philip Race; Sergey Bylokhov
*Subject:* [OpenJDK 2D-Dev]  Review request for
JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing

Hello All,

Made small change on how we need to represent floating point
constant in
Java(1000.0 -> 1000.0F).

Please find updated Webrev link:

Webrev :
http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.01/

Please review.

Thanks,

Jay

*From:* Jayathirth D V
*Sent:* Thursday, October 08, 2015 2:20 PM
*To:* 2d-dev@openjdk.java.net <mailto:2d-dev@openjdk.java.net>;
Philip Race; Sergey Bylokhov
*Subject:* [OpenJDK 2D-Dev]  Review request for
JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing

Hello All,

Please review following fix in jdk9:

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

Webrev :
http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.00/

Bug : BMPMetadata returns invalid PhysicalPixelSpacing

Root cause : Whenever XPixelsPerMter or YPixelsPerMeter is more
than value 1 in BMP header. Horizontal & Vertical Physical pixel
spacing were returned as zero.

   In getStandardDimensionNode() method
of BMPMetadata.java we are dividing 1 by XPixelsPerMter/ YPixelsPerMter.
When

   XPixelsPerMter/ YPixelsPerMter is
more than 1. Resulted value is stored without decimal part, which resulted in 
zero.

Solution : Made changes to how Horizontal & Vertical Physical pixel
spacing is calculated so that decimal value is not truncated.

Thanks,

Jay


--
Best regards, Sergey.


--
Best regards, Sergey.


--
Best regards, Sergey.







Re: [OpenJDK 2D-Dev] RFR: JDK-8079652: Could not enable D3D pipeline


2015-05-26

Thread
Vadim Pakhnushev


Could somebody take a look?

On 13.05.2015 13:48, Vadim Pakhnushev wrote:

Actually I've found a better solution - specify WS_POPUP window style.
In this case the client area size will be exactly as specified instead 
of adjusting for some default window style.

So please review the second iteration:

diff --git 
a/src/java.desktop/windows/native/libawt/java2d/d3d/D3DPipelineManager.cpp 
b/src/java.desktop/windows/native/libawt/java2d/d3d/D3DPipelineManager.cpp 

--- 
a/src/java.desktop/windows/native/libawt/java2d/d3d/D3DPipelineManager.cpp
+++ 
b/src/java.desktop/windows/native/libawt/java2d/d3d/D3DPipelineManager.cpp

@@ -828,7 +828,7 @@
 return 0;
 }

-HWND hWnd = CreateWindow(LD3DFocusWindow, LD3DFocusWindow, 0,
+HWND hWnd = CreateWindow(LD3DFocusWindow, LD3DFocusWindow, 
WS_POPUP,

 mi.rcMonitor.left, mi.rcMonitor.top, 1, 1,
 NULL, NULL, GetModuleHandle(NULL), NULL);
 if (hWnd == 0) {

Thanks,
Vadim

On 08.05.2015 21:38, Phil Race wrote:
I guess this is OK since 100x100 ought to be always big enough but 
not too big ..
I suppose it may imply a different default window style is being 
added by CreateWindow

than we got before.

-phil.



On 5/8/2015 6:28 AM, Sergey Bylokhov wrote:

Hi, Vadim.
Thanks for clarification, please add this information as a comment 
to the code, before the push.


On 08.05.15 16:19, Vadim Pakhnushev wrote:
It's invisible and used only for getting application focus 
notifications internally by Direct3D.


On 08.05.2015 16:14, Sergey Bylokhov wrote:

Hi, Vadim.
Why we do not use the full screen size for this window?

On 08.05.15 14:07, Vadim Pakhnushev wrote:

Hi,
Please review the fix for 
https://bugs.openjdk.java.net/browse/JDK-8079652
Focus window's client area should be bigger otherwise 
CreateDevice fails.


diff --git 
a/src/java.desktop/windows/native/libawt/java2d/d3d/D3DPipelineManager.cpp 
b/src/java.desktop/windows/native/libawt/java2d/d3d/D3DPipelineManager.cpp 

--- 
a/src/java.desktop/windows/native/libawt/java2d/d3d/D3DPipelineManager.cpp
+++ 
b/src/java.desktop/windows/native/libawt/java2d/d3d/D3DPipelineManager.cpp

@@ -829,7 +829,7 @@
 }

 HWND hWnd = CreateWindow(LD3DFocusWindow, 
LD3DFocusWindow, 0,

-mi.rcMonitor.left, mi.rcMonitor.top, 1, 1,
+mi.rcMonitor.left, mi.rcMonitor.top, 100, 100,
 NULL, NULL, GetModuleHandle(NULL), NULL);
 if (hWnd == 0) {
 J2dRlsTraceLn(J2D_TRACE_ERROR,




















Re: [OpenJDK 2D-Dev] RFR: JDK-8079652: Could not enable D3D pipeline


2015-05-13

Thread
Vadim Pakhnushev


Actually I've found a better solution - specify WS_POPUP window style.
In this case the client area size will be exactly as specified instead 
of adjusting for some default window style.

So please review the second iteration:

diff --git 
a/src/java.desktop/windows/native/libawt/java2d/d3d/D3DPipelineManager.cpp 
b/src/java.desktop/windows/native/libawt/java2d/d3d/D3DPipelineManager.cpp
--- 
a/src/java.desktop/windows/native/libawt/java2d/d3d/D3DPipelineManager.cpp
+++ 
b/src/java.desktop/windows/native/libawt/java2d/d3d/D3DPipelineManager.cpp

@@ -828,7 +828,7 @@
 return 0;
 }

-HWND hWnd = CreateWindow(LD3DFocusWindow, LD3DFocusWindow, 0,
+HWND hWnd = CreateWindow(LD3DFocusWindow, LD3DFocusWindow, 
WS_POPUP,

 mi.rcMonitor.left, mi.rcMonitor.top, 1, 1,
 NULL, NULL, GetModuleHandle(NULL), NULL);
 if (hWnd == 0) {

Thanks,
Vadim

On 08.05.2015 21:38, Phil Race wrote:
I guess this is OK since 100x100 ought to be always big enough but not 
too big ..
I suppose it may imply a different default window style is being added 
by CreateWindow

than we got before.

-phil.



On 5/8/2015 6:28 AM, Sergey Bylokhov wrote:

Hi, Vadim.
Thanks for clarification, please add this information as a comment to 
the code, before the push.


On 08.05.15 16:19, Vadim Pakhnushev wrote:
It's invisible and used only for getting application focus 
notifications internally by Direct3D.


On 08.05.2015 16:14, Sergey Bylokhov wrote:

Hi, Vadim.
Why we do not use the full screen size for this window?

On 08.05.15 14:07, Vadim Pakhnushev wrote:

Hi,
Please review the fix for 
https://bugs.openjdk.java.net/browse/JDK-8079652
Focus window's client area should be bigger otherwise CreateDevice 
fails.


diff --git 
a/src/java.desktop/windows/native/libawt/java2d/d3d/D3DPipelineManager.cpp 
b/src/java.desktop/windows/native/libawt/java2d/d3d/D3DPipelineManager.cpp 

--- 
a/src/java.desktop/windows/native/libawt/java2d/d3d/D3DPipelineManager.cpp
+++ 
b/src/java.desktop/windows/native/libawt/java2d/d3d/D3DPipelineManager.cpp

@@ -829,7 +829,7 @@
 }

 HWND hWnd = CreateWindow(LD3DFocusWindow, 
LD3DFocusWindow, 0,

-mi.rcMonitor.left, mi.rcMonitor.top, 1, 1,
+mi.rcMonitor.left, mi.rcMonitor.top, 100, 100,
 NULL, NULL, GetModuleHandle(NULL), NULL);
 if (hWnd == 0) {
 J2dRlsTraceLn(J2D_TRACE_ERROR,


















[OpenJDK 2D-Dev] RFR: JDK-8079652: Could not enable D3D pipeline


2015-05-08

Thread
Vadim Pakhnushev


Hi,
Please review the fix for https://bugs.openjdk.java.net/browse/JDK-8079652
Focus window's client area should be bigger otherwise CreateDevice fails.

diff --git 
a/src/java.desktop/windows/native/libawt/java2d/d3d/D3DPipelineManager.cpp 
b/src/java.desktop/windows/native/libawt/java2d/d3d/D3DPipelineManager.cpp
--- 
a/src/java.desktop/windows/native/libawt/java2d/d3d/D3DPipelineManager.cpp
+++ 
b/src/java.desktop/windows/native/libawt/java2d/d3d/D3DPipelineManager.cpp

@@ -829,7 +829,7 @@
 }

 HWND hWnd = CreateWindow(LD3DFocusWindow, LD3DFocusWindow, 0,
-mi.rcMonitor.left, mi.rcMonitor.top, 1, 1,
+mi.rcMonitor.left, mi.rcMonitor.top, 100, 100,
 NULL, NULL, GetModuleHandle(NULL), NULL);
 if (hWnd == 0) {
 J2dRlsTraceLn(J2D_TRACE_ERROR,






Re: [OpenJDK 2D-Dev] RFR: JDK-8079652: Could not enable D3D pipeline


2015-05-08

Thread
Vadim Pakhnushev

It's invisible and used only for getting application focus notifications 
internally by Direct3D.


On 08.05.2015 16:14, Sergey Bylokhov wrote:

Hi, Vadim.
Why we do not use the full screen size for this window?

On 08.05.15 14:07, Vadim Pakhnushev wrote:

Hi,
Please review the fix for 
https://bugs.openjdk.java.net/browse/JDK-8079652
Focus window's client area should be bigger otherwise CreateDevice 
fails.


diff --git 
a/src/java.desktop/windows/native/libawt/java2d/d3d/D3DPipelineManager.cpp 
b/src/java.desktop/windows/native/libawt/java2d/d3d/D3DPipelineManager.cpp 

--- 
a/src/java.desktop/windows/native/libawt/java2d/d3d/D3DPipelineManager.cpp
+++ 
b/src/java.desktop/windows/native/libawt/java2d/d3d/D3DPipelineManager.cpp

@@ -829,7 +829,7 @@
 }

 HWND hWnd = CreateWindow(LD3DFocusWindow, LD3DFocusWindow, 0,
-mi.rcMonitor.left, mi.rcMonitor.top, 1, 1,
+mi.rcMonitor.left, mi.rcMonitor.top, 100, 100,
 NULL, NULL, GetModuleHandle(NULL), NULL);
 if (hWnd == 0) {
 J2dRlsTraceLn(J2D_TRACE_ERROR,











Re: [OpenJDK 2D-Dev] RFR: (JDK-7011935) Using AWT in fullscreen exclusive mode results in a black screen


2015-05-07

Thread
Vadim Pakhnushev


Hi Prasanta,
I don't think that this is the right fix, it basically changes the D3D 
fullscreen mode from exclusive to simulated (as per 
GraphicsDevice.setFullScreenWindow terminology).

I think further investigation is needed as to why this happens.

BTW, while trying this, I've found that after the JDK9 compiler upgrade 
D3D device can't be created.
I've traced it to the focus window client area size and filed 
https://bugs.openjdk.java.net/browse/JDK-8079652


Thanks,
Vadim

On 05.05.2015 11:12, prasanta sadhukhan wrote:

Hi,

Please review a fix for fullscreen mode not working in D3D pipeline. 
It seems D3DPRESENT_PARAMETERS Windowed flag needs to be enabled for 
fullscreen mode to be in effect.


https://bugs.openjdk.java.net/browse/JDK-7011935
http://cr.openjdk.java.net/~psadhukhan/7011935/webrev.00/

Regards
Prasanta







Re: [OpenJDK 2D-Dev] AWT Dev [9] Review Request: 8074028 Remove API references to java.awt.peer


2015-03-04

Thread
Vadim Pakhnushev


On 04/03/15 15:37, Sergey Bylokhov wrote:
http://cr.openjdk.java.net/~serb/8074028/webrev.05/src/java.desktop/share/classes/java/awt/MenuComponent.java.sdiff.html 


What's the point of ComponentPeer peer = this.peer; ?
Why not just use peer as in other places in this file?

Thanks,
Vadim





Re: [OpenJDK 2D-Dev] RFR: JDK-8039444: Swing applications not being	displayed properly


2014-11-07

Thread
Vadim Pakhnushev


Looks good to me.

On 07.11.2014 21:52, Phil Race wrote:

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

Historically we enabled D3D only on Nvidia  ATI hardware.
Intel chipsets are a large part of the market but we continually 
encountered

rendering artifacts that apparently were driver or hardware level issues.

For JDK 8 we did some testing on the newest (at the time) Intel HD 
chipsets

that FX was using successfully and enabled it under this bug ID
https://bugs.openjdk.java.net/browse/JDK-8000936

However we are exercising a different set of features than FX and people
have reported issues already, and there is a concern that now that 
auto-update
to JDK 8 is kicking in that we'll get more. I've started to label 
these bugs as follows :-


https://bugs.openjdk.java.net/issues/?jql=labels%20%3D%20d3d-driver-intel

At this time I propose to fix only in 8u40 so we can try again for 
JDK9 on

(even) later chipsets ..

-phil.










[OpenJDK 2D-Dev] hg: jdk8/2d/jdk: 8029628: Many graphic artifacts


2013-12-12

Thread
vadim . pakhnushev

Changeset: c8c4aef922ff
Author:vadim
Date:  2013-12-13 11:49 +0400
URL:   http://hg.openjdk.java.net/jdk8/2d/jdk/rev/c8c4aef922ff

8029628: Many graphic artifacts
Reviewed-by: prr, bae

! src/windows/native/sun/java2d/d3d/D3DBadHardware.h






[OpenJDK 2D-Dev] RFR: 8029628: Large amount of graphic artifacts


2013-12-05

Thread
Vadim Pakhnushev


Hello,

Please review a fix for the https://bugs.openjdk.java.net/browse/JDK-8029628
http://cr.openjdk.java.net/~vadim/8029628/webrev.00/
Intel HD Graphics 2000/3000 seems to be not usable (yet again!)
So the proposed fix is to blacklist all Intel cards except for HD 4000+, 
which seems to be working OK.
I'm not quite sure where this fix should go, perhaps we could squeeze it 
into JDK 8 GA?


Thanks,
Vadim





Re: [OpenJDK 2D-Dev] [8] request for review: 8026702: Fix for 8025429 breaks jdk build on windows


2013-10-16

Thread
Vadim Pakhnushev


Looks good.

On 16.10.2013 17:00, Andrew Brygin wrote:

Hello,

could you please review a fix for CR 8026702?

Bug: http://bugs.sun.com/view_bug.do?bug_id=8026702
Webrev: http://cr.openjdk.java.net/~bae/8026702/webrev.00/

Suggested fix is to declare local variables in the beginning of the 
function.


Please take a look.

Thanks,
Andrew.







[OpenJDK 2D-Dev] RFR: 8023590: REGRESSION: large count of graphics artifacts with Java 8 on Windows 8 on Intel HD card.


2013-10-14

Thread
Vadim Pakhnushev


Hi,

Please review a fix for this issue:
https://bugs.openjdk.java.net/browse/JDK-8023590
http://cr.openjdk.java.net/~vadim/8023590/webrev.00/

In JDK 8 b91 we enabled D3D pipeline on Intel HD Graphics chipsets
https://bugs.openjdk.java.net/browse/JDK-8000936
After that we received a number of reports about rendering artifacts.
Apparently, updating drivers to the latest version resolved these bugs.
While it's probable that we are using D3D in some not exactly compatible 
way, it's more likely a driver issue.


So the fix is to update drivers blacklist to the latest versions 
available from the Intel website.
I've tested the software from this bug report as well as simple test 
case and internal test from the other report on Lenovo laptops with HD 
Graphics 3000 and 4000 with Windows 7 x64.


Thanks,
Vadim





Re: [OpenJDK 2D-Dev] Handling of premultication in the D3D  OGL pipelines


2013-10-01

Thread
Vadim Pakhnushev

BTW, before you stumble upon XOR XRender bug yourself - I'm currently 
working on it and will send a fix for review shortly.


Thanks,
Vadim

On 01.10.2013 9:52, Clemens Eisserer wrote:

Hi,

I am currently testing compatibility of the xrender pipeline with
different composition operations, and I noticed for AlphaComposite.SRC
the D3D and OGL pipelines store pre-multiplied colors in surfaces
without an alpha-channel.

For example the following code results in a black rectangle, instead
of a red one when rendering to a BufferedImage of type INT_RGB:

 ((Graphics2D) g).setComposite(AlphaComposite.Src);
 g.setColor(new Color(255, 0, 0, 2));
 g.fillRect(10, 10, 100, 100);

Is this intentional or should it be considered a bug?


Thanks, Clemens







Re: [OpenJDK 2D-Dev] RFR: 8024343: Change different color with the The XOR alternation color combobox, the color of the image can not shown immediately.


2013-10-01

Thread
Vadim Pakhnushev


Clemens,

This bug is filed against JCK test, and there were a lot of internal 
urls in descriptions, hence it's confidential.

So I have created open automatic regression test for that.

Thanks,
Vadim

On 01.10.2013 20:43, Clemens Eisserer wrote:


Hi Vadim,

The fix itself looks fine to me - thanks for taking care about this.

- Clemens

PS: what is the reason for this bug is confidential?

 Please review a fix for this bug:
 https://bugs.openjdk.java.net/browse/JDK-8024343








Re: [OpenJDK 2D-Dev] [8] request for review: 8024511: Crash during	color profile destruction


2013-09-10

Thread
Vadim Pakhnushev


Looks good.

On 10.09.2013 14:16, Andrew Brygin wrote:

Hello,

could you please review a fix for CR 8024511?

Bug: http://bugs.sun.com/view_bug.do?bug_id=8024511
Webrev: http://cr.openjdk.java.net/~bae/8024511/webrev.00/

Starting fix for 7043064, we are using disposer to destroy a native
part of a color profile. However an incorrect disposal method is
registered in the disposer. It results in occasional crashed during
profile destruction. Typically, the crash happens in cmsPipelineFree.

Supplied regression test demonstrates the problem.

Suggested fix is to register correct disposal method  for profiles.

Please take a look.

Thanks,
Andrew.







Re: [OpenJDK 2D-Dev] RFR: 8008022: Upgrade Direct X SDK used to build JDK


2013-09-09

Thread
Vadim Pakhnushev


Phil,

I've tested it on the clean Windows 7 virtual machine with only VS2010 
installed as well on a number of configurations with and without DXSDK 
(even on my VS2012 only setup).


Thanks,
Vadim

On 09.09.2013 19:49, Phil Race wrote:
Seems fine to me given that the versions are identical across all SDKs 
we might use.
Also since this simply removes a required build component, there 
should be no
'flag day' where people need to be given notice to install a new build 
component.
I think you said that you didn't have the standalone DX SDK installed 
at all.

Is that correct ?

Whilst we need to be sure that JPRT builds fine it will still be 
configured with

that SDK, so your independent testing is also needed for that.

-phil.

On 9/6/2013 1:21 PM, Vadim Pakhnushev wrote:

Hi all,

Please review the fix for this bug:
http://bugs.sun.com/view_bug.do?bug_id=8008022

http://cr.openjdk.java.net/~vadim/8008022/webrev.00/

I've found that all needed DirectX 9 SDK files (that is, d3d9.h, 
dsound.h and dsound.lib) are included in the Windows SDK 7.0a shipped 
with Visual Studio 2010.

They are also in all later SDKs - 7.1, 7.1a.
The version is the same in all SDKs.
Officially the whole DXSDK is included with Windows SDK 8 only (VS2012).
While researching this, I've found that Kelly also came to this 
conclusion some time ago, mentioned here:

http://bugs.sun.com/view_bug.do?bug_id=8008073

So instead of updating the version of DirectX SDK, we can safely 
remove this dependency altogether.

Proposed fix is to basically revert the fix for 8008073.
Phil also requested that the old build be updated as well (BTW, it 
seems that the old build system is broken in couple of places in the 
security packages).


The weakness of this fix is that I couldn't find an easy way to check 
for the existence of needed files.
When we used single directory for DXSDK location, we used to check 
the existence by using if test ! -f $DXSDK_LIB_PATH/dsound.lib; then
Now these files are found by compiler through INCLUDE and LIB 
environment variables which are in the windows format.
I'm not sure if it will be worth to parse these variables and check 
every path for existence of needed files.
Absence of files seems to be very unlikely, and if they are not 
there, the compiler will produce meaningful error message.


I tested this with local new and old windows build.
All DirectX regtests has passed.
JPRT job is in progress now, seems to be stuck in queue.

Thanks,
Vadim









[OpenJDK 2D-Dev] [8] request for review: 8016254 several sun/java2d/OpenGL tests failed with SIGFPE


2013-06-25

Thread
Vadim Pakhnushev


Hi,

Please review the fix for 8016254:
http://bugs.sun.com/view_bug.do?bug_id=8016254
https://jbs.oracle.com/bugs/browse/JDK-8016254
http://cr.openjdk.java.net/~vadim/8016254/webrev.00/

The problem is that Intel 865G chipset (and some other ancient video 
cards) doesn't support GL_ARB_depth_texture extension, although does 
support GL_EXT_framebuffer_object.
So the OGLContext_IsFBObjectExtensionAvailable function calls 
OGLSD_InitFBObject which tries to create a depth renderbuffer.
Ideally glRenderbufferStorageEXT should return GL_INVALID_ENUM to 
indicate that the format is not supported, but instead it passes 
incorrect values further to the driver and MESA driver crashes.
The proposed fix is to check GL_ARB_depth_texture extension and return 
false from OGLContext_IsFBObjectExtensionAvailable if it's not supported.


Thanks,
Vadim





Re: [OpenJDK 2D-Dev] [8] request for review: 4892259 GIF ImageReader does not call passComplete in IIOReadUpdateListener


2013-05-16

Thread
Vadim Pakhnushev


Phil, Andrew,

Would you please take a look at this updated fix posted half a year ago?

Thanks,
Vadim

On 20.11.2012 22:14, Vadim Pakhnushev wrote:

Phil,

That's right, interlaced images works as expected without the fix.
Non-interlaced images could be considered single pass but the 
documentation states that the passStarted is called when progressive 
pass started.
For examples, BMP reader doesn't call passStarted, but non-progressive 
JPEGs or PNGs do, so there is some inconsistency here.
In the internal discussion back in 2007 Chris stated that 
IIOReadUpdateListener.processPassStarted() is only intended to be 
called for progressive (aka interlaced) images.


I've created automated regression test which checks that if the image 
was created with interlaced flag then corresponding methods are called 
and are not if the image is not interlaced.


Please review the test:
http://cr.openjdk.java.net/~bae/4892259/webrev.01/

Thanks,
Vadim

On 20.11.2012 3:47, Phil Race wrote:

Vadim,

So I take it that you confirmed interlaced images do *not* exhibit 
this problem ?
Its not obvious from the bug report that its only non-interlaced 
images that are the issue.


I suppose a non-interlaced image could be considered a single pass but
I can't find anywhere that we mandated such a behaviour and it would 
seem

odd for many formats to require it.

Can we include a regression test for this fix ?

--phil.

On 11/19/2012 11:15 AM, Vadim Pakhnushev wrote:

Hi,

Please review a fix for bug 4892259:
http://bugs.sun.com/view_bug.do?bug_id=4892259
http://cr.openjdk.java.net/~bae/4892259/webrev.00/

Actually GIFImageReader should not call neither passComplete, nor 
passStarted for non-interlaced images in the first place.
So the fix is to check whether the image is interlaced or not and 
skip startPass method entirely in case of interlaced image.

passComplete is called only for interlaced images.

Thanks,
Vadim













[OpenJDK 2D-Dev] [8] request for review: 4892259 GIF ImageReader does not call passComplete in IIOReadUpdateListener


2012-11-19

Thread
Vadim Pakhnushev


Hi,

Please review a fix for bug 4892259:
http://bugs.sun.com/view_bug.do?bug_id=4892259
http://cr.openjdk.java.net/~bae/4892259/webrev.00/

Actually GIFImageReader should not call neither passComplete, nor 
passStarted for non-interlaced images in the first place.
So the fix is to check whether the image is interlaced or not and skip 
startPass method entirely in case of interlaced image.

passComplete is called only for interlaced images.

Thanks,
Vadim






[OpenJDK 2D-Dev] [8] request for review: 5082749: GIF stream metadata specification of aspect ratio is incorrect.


2012-11-19

Thread
Vadim Pakhnushev


Hi,

Please review fix for the bug 5082749:
http://bugs.sun.com/view_bug.do?bug_id=5082749
http://cr.openjdk.java.net/~bae/5082749/webrev.00/

There is a typo in the documentation, there are correct values 
everywhere else in the code.


Thanks,
Vadim





Re: [OpenJDK 2D-Dev] [8] request for review JDK-8000176


2012-10-05

Thread
Vadim Pakhnushev


Phil,

I've added these 2 lines to the test, Andrew will push updated version.

Thanks,
Vadim

On 05.10.2012 0:08, Phil Race wrote:
I'm OK with it too although since xrender isn't applicable to Windows, 
either True or false is actually

testing the default (hopefully d3d) pipeline there.

If you wanted to thoroughly test , including windows as well you could add
2 more lines
@run main/othervm -Dsun.java2d.d3d=false InterpolationQualityTest
@run main/othervm -Dsun.java2d.d3d=True InterpolationQualityTest

On any platform where any of these options isn't applicable I
think it'll be ignored, so you'll run the default twice, but that's OK.

-phil.

On 10/3/12 7:00 AM, Andrew Brygin wrote:

Hello Vadim,

 the test looks fine to me.

Thanks,
Andrew

On 03.10.2012 17:41, Vadim Pakhnushev wrote:

Hello,
I've made changes according to Andrew Brygin's feedback:
Created unified drawTestImage method so reference and tested images 
are rendered from one place.

Simplified renderImage method to eliminate nested while loop.

webrev: http://cr.openjdk.java.net/~bae/8000176/webrev.01/

Thanks,
Vadim

On 28.09.2012 16:20, Vadim Pakhnushev wrote:

Hello,

Please review automated test which checks that accelerated 
pipelines correctly use interpolation rendering hints.

Discussion of the bug for which the test was created:
http://mail.openjdk.java.net/pipermail/2d-dev/2012-September/002767.html 



webrev: http://cr.openjdk.java.net/~bae/8000176/webrev.00/

Thanks,
Vadim













Re: [OpenJDK 2D-Dev] [8] request for review JDK-8000176


2012-10-03

Thread
Vadim Pakhnushev


Hello,
I've made changes according to Andrew Brygin's feedback:
Created unified drawTestImage method so reference and tested images are 
rendered from one place.

Simplified renderImage method to eliminate nested while loop.

webrev: http://cr.openjdk.java.net/~bae/8000176/webrev.01/

Thanks,
Vadim

On 28.09.2012 16:20, Vadim Pakhnushev wrote:

Hello,

Please review automated test which checks that accelerated pipelines 
correctly use interpolation rendering hints.

Discussion of the bug for which the test was created:
http://mail.openjdk.java.net/pipermail/2d-dev/2012-September/002767.html

webrev: http://cr.openjdk.java.net/~bae/8000176/webrev.00/

Thanks,
Vadim







[OpenJDK 2D-Dev] [8] request for review JDK-8000176


2012-09-28

Thread
Vadim Pakhnushev


Hello,

Please review automated test which checks that accelerated pipelines 
correctly use interpolation rendering hints.

Discussion of the bug for which the test was created:
http://mail.openjdk.java.net/pipermail/2d-dev/2012-September/002767.html

webrev: http://cr.openjdk.java.net/~bae/8000176/webrev.00/

Thanks,
Vadim





Re: [OpenJDK 2D-Dev] request for review: 7188093


2012-09-20

Thread
Vadim Pakhnushev


Phil,

I'm not sure about original test case, it uses cow.gif file. Moreover, 
it's not useful for this particular bug at all.

I can open my version of the test which clearly shows the bug.
I suppose I should post a webrev for this same CR with added test?

Thanks,
Vadim

On 18.09.2012 22:32, Phil Race wrote:

Approved (2nd reviewer).

Vadim: can you also see if we can 'open' the regression test that 
identified this bug.


-phi.

On 9/18/2012 10:37 AM, Jim Graham wrote:

This looks good.  OpenGL and D3D punt under the same circumstances...

...jim

On 9/18/2012 1:16 AM, Clemens Eisserer wrote:

Hello,

Please review my fix for bug 7188093, located at:
http://cr.openjdk.java.net/~ceisserer/7188093/webrev.01/

Billinear interpolation is the highest quality interpolation supported
by xrender, so the xrender-pipeline has to fall back software loops in
case bicubic interpolation is requested.
Nimbus only uses billinear interpolation, however I have no idea how
much this affects other real-world applications out there.

Thank you in advance, Clemens









Re: [OpenJDK 2D-Dev] request for review: 7188093


2012-09-20

Thread
Vadim Pakhnushev

I copied original test .java and .html files, renamed and adjusted them 
for this case. Is it enough for jtreg?


Automated test will require taking screenshot and comparing results, I'm 
not sure if this mechanism works good.
If it is, I could develop automated test, Andrew mentioned that there is 
some helper class which provides this functionality.


Thanks,
Vadim

On 21.09.2012 0:28, Phil Race wrote:
Ideally it should go under this bug id but its not the end of the 
world if we

push it under another one.

If you can provide it soon in jtreg form we can do it as part of this 
fix.

Ideally it should be automated.

-phil.

On 9/20/2012 7:51 AM, Vadim Pakhnushev wrote:

Phil,

I'm not sure about original test case, it uses cow.gif file. 
Moreover, it's not useful for this particular bug at all.

I can open my version of the test which clearly shows the bug.
I suppose I should post a webrev for this same CR with added test?

Thanks,
Vadim

On 18.09.2012 22:32, Phil Race wrote:

Approved (2nd reviewer).

Vadim: can you also see if we can 'open' the regression test that 
identified this bug.


-phi.

On 9/18/2012 10:37 AM, Jim Graham wrote:

This looks good.  OpenGL and D3D punt under the same circumstances...

...jim

On 9/18/2012 1:16 AM, Clemens Eisserer wrote:

Hello,

Please review my fix for bug 7188093, located at:
http://cr.openjdk.java.net/~ceisserer/7188093/webrev.01/

Billinear interpolation is the highest quality interpolation 
supported
by xrender, so the xrender-pipeline has to fall back software 
loops in

case bicubic interpolation is requested.
Nimbus only uses billinear interpolation, however I have no idea how
much this affects other real-world applications out there.

Thank you in advance, Clemens













Re: [OpenJDK 2D-Dev] request for review: 7188093


2012-09-20

Thread
Vadim Pakhnushev


That's good, I will try to improve my test to be automated then.

Thanks,
Vadim

On 21.09.2012 0:40, Clemens Eisserer wrote:

Hi,


Automated test will require taking screenshot and comparing results, I'm not
sure if this mechanism works good.

It should be possible to render to a VolatileImage, which allows
direct readback without taking a screenshot.

- Clemens