date:20171109

Re: [OpenJDK 2D-Dev] RFR JDK-8184429: Path clipper added in Marlin2D & MarlinFX 0.8.0

2017-11-09 Thread Laurent Bourgès

Phil & Kevin,
The proposed plan looks good.

I will first work on refactoring my ShapeClipTest and then focus on having
a Java2d patch ready.

I absolutely want that this new performance improvement will ship in Java
10.

PS: Another important performance change concerns tuning Java2D tile sizes.
This can be easily done: set tile log2 settings to 7 x 6 to let Marlin use
128x64 tiles: up to 60% gain for large shapes rendered on VolatileImage
(linux 64 + i7 + nvidia)

I will create new bugs to track these RFE...

Bye,
Laurent

Le 8 nov. 2017 23:44, "Phil Race"  a écrit :

I think they should be separate webrevs sent to the separate lists and you
should start with 2D
as I can then run the JDK regression tests on it. I know you can
theoretically run the open regression
tests too (are you ?) but there are some random scattered closed regression
tests that so far
as I can see can be open sourced .. that I can run but you can't .. I'll at
least run the
automated ones. I wouldn't call them anything very focused on testing
rasterization but
I can at least check off that concern ..

And yes, I'll make time to review it.

-phil.

On 11/08/2017 01:55 PM, Laurent Bourgès wrote:

Kevin & Phil,

Some news on that issue:
I successfully managed to finish the Path clipping support in Marlin 0.8.2
(release last week):
https://github.com/bourgesl/marlin-renderer/releases/tag/v0.8.2

I fixed few remaining bugs in either Stroker (1) and in PathClipFilter (2)
to have proper & tested clipping in Marlin renderer (2D). It now works
perfectly with either NZ or EO winding rules.

To ensure detecting any artefact between Clipping Off vs On, I implemented
a 'basher' test (as recommended by Jim) that renderers 10 000 random
polygons (5 -> 9 -> 50 line segments or mixed with line / quads / cubics) (
whose point coordinates are in [-50 to 150] ) to a 100x100 buffered image
with or without clipping enabled (using a system property at runtime). Of
course, all output pixels are compared and any pixel difference is
considered as a failure.

The new ShapeClipTests tests all stroke combinations (cap / join / with or
without dashes / closed or not / EO or NZ rule) and also fills (closed or
not / EO or NZ rule) => 170 tests run OK

I need some time to synchronize MarlinFX and then with either OpenJDK
forrest (new) or OpenJFX10.
If you want the new automated test (long run ~ 20 minutes), I need some
time to refactor it as it uses some code from my MapBench tool and have a
standalone test class.

Will you have time to review such (medium) changes in Marlin2D (Phil ?) and
/ or MarlinFX (Kevin ?) before the deadline (dec 14th) ?
I said 'medium' as the code is quite simple to read but the new CG
algorithms to ignore / discard useless path elements are cool but not
obvious.

Please tell me if you have time and if you prefer a combined (JDK / JFX)
webrev or start with 2D or JFX.

Cheers,
Laurent

2017-09-07 8:52 GMT+02:00 Laurent Bourgès :

> Hi Kevin,
>
> Ok I propose to withdraw or postpone this review after JavaOne where we
> will be able to discuss in a face to face meeting about Marlin & MarlinFX
> changes for JDK10.
>
> I hope the 2d / jfx groups have other Graphics Guru to help, as good as
> Jim Graham.
>
> Cheers,
> Laurent
>
> Le 6 sept. 2017 16:23, "Kevin Rushforth"  a
> écrit :
>
>> Hi Laurent,
>>
>> Some combination of Phil, Sergey, and I will take a look at this when we
>> can. Perhaps there might be others on these two lists who could lend a
>> helping hand?
>>
>> -- Kevin
>
>

Re: [OpenJDK 2D-Dev] [10] RFR: JDK-8188083- NPE in java.awt.image.FilteredImageSource.startProduction

2017-11-09 Thread Prahalad Kumar Narayanan

Hello Everyone

First, Thanks to Phil & Sergey for their time in review & suggestions.

This is a follow-up on Phil's suggestion to investigate whether the fix 
proposed in webrev.00 could lead to deadlocks.
Link: http://cr.openjdk.java.net/~pnarayanan/8188083/webrev.00/

I inspected the code, and wrote code snippets to check the behavior. 
Inferences are as follows-
1. FilteredImageSource helps in producing the image, applying an image 
filter on the pixels, and sharing the result to the ImageConsumer.
2. The object holds- An ImageProducer, An ImageFilter, and instantiates a 
HashTable for its operation.

3. The methods of FilteredImageSource are not supposed to be invoked 
directly from user-code. So how are these methods invoked?
  . startProduction:
. This method is invoked when a user creates an Image like- 
Component.createImage(new FilteredImageSource(imgProducer, imgFilter));
. Such creation could occur on any thread in user code and this 
internally invokes FilteredImageSource.startProduction through ToolkitImage.
. The method updates the hash table as required and invokes 
ImageProducer.startProduction to produce the image. 
  . removeConsumer:
. This method is invoked once ImageProducer is done with image 
preparation.
. The exact thread in which the removeConsumer gets invoked 
depends on how ImageProducer prepares the image.

4. Is there a possibility of a deadlock with the proposed fix ? 
  . In my inspection & observation with code snippets, I found that 
removeConsumer could be invoked in two ways- Multi-threaded & Single threaded.
  . Multi-threaded- 
. This was observed with FileImageSource as ImageProducer
. startProduction on this image producer requests image decode 
operation on a different thread- ImageFetcher thread.
. Once decoding is complete, removeConsumer is invoked on the 
ImageFetcher thread.
. Though startProduction & removeConsumer are invoked on 
different threads, there is no possibility of a deadlock as the code flow from 
startProduction returns immediately after posting the request to decode image 
onto ImageFetcher 's queue.
  . Single threaded-
. This was observed with OffscreenImageSource as ImageProducer
. startProduction and removeConsumer are invoked on the same 
thread. 
. Since re-entrant threads acquire the locks that they already 
hold, this wouldn't result in a deadlock.
  . Thus, in both cases I don't see possibility of a deadlock. Hence, 
we can consider the change in webrev.00 safe to fix the issue.

5. I have run JTreg test cases before and after the proposed change on 
java/awt/image/ and javax/swing (where ImageFilter is used).
 No new regressions were observed during the test.

Kindly review by assessing the above information.
If you believe, the fix could be approved, kindly review the CSR as well.

Thank you once again for your time
Have a good day

Prahalad

- Original Message -
From: Philip Race 
Sent: Thursday, October 26, 2017 9:30 PM
To: Prahalad Kumar Narayanan
Cc: Sergey Bylokhov; 2d-dev
Subject: Re: [OpenJDK 2D-Dev] [10] RFR: JDK-8188083- NPE in 
java.awt.image.FilteredImageSource.startProduction

The screenshot shows you directly calling this method in your test which
the documentation says you are not supposed to do.
So I am not able to be 100% sure that the test you have re-creates what
the submitter saw .. in his stack trace you have below it seems to be valid.

But to have a NPE at line 181 is odd.
 181 proxies.put(ic, imgf);

What is null ? proxies was just initialised in this same method so
I don't see how it can be null unless something elsewhere is
resetting it to null.

The only place I see that is removeConsumer
 138 public synchronized void removeConsumer(ImageConsumer ic) {
 139 if (proxies != null) {
 140 ImageFilter imgf =  proxies.get(ic);
 141 if (imgf != null) {
 142 src.removeConsumer(imgf);
 143 proxies.remove(ic);
 144 if (proxies.isEmpty()) {
 145 proxies = null;
 146 }
 147 }
 148 }
 149 }

Now that method is synchronized .. OK so I think it might make sense
to mark the additional methods synchronized too but then I automatically
worry about introducing a deadlock.

I can't say for sure that you have done so however and likely there
are no nested locks here so it is probably OK but please run as many
tests as you can find to try to ensure that.

Adding or removing sychronized is binary compatible but since
these are public API methods you should still do a CSR
before pushing if this ends up being the approved fix.

-phil.



On 10/25/17, 11:58 PM, Prahalad Kumar

Re: [OpenJDK 2D-Dev] [10] Review Request: JDK-8159142 : Visible artifacts in sun/java2d/SunGraphics2D/DrawImageBilinear.java

2017-11-09 Thread Pankaj Bansal

Hi Sergey,

Do these changes look good? Please give your feedback.

Regards,
Pankaj Bansal

-Original Message-
From: Pankaj Bansal 
Sent: Wednesday, November 8, 2017 8:09 PM
To: Sergey Bylokhov; awt-...@openjdk.java.net; 2d-dev@openjdk.java.net; Philip 
Race
Subject: RE:  [10] Review Request: JDK-8159142 : Visible artifacts in 
sun/java2d/SunGraphics2D/DrawImageBilinear.java

+ 2d-dev, Phil.

Regards,
Pankaj Bansal

-Original Message-
From: Pankaj Bansal
Sent: Friday, November 3, 2017 3:11 PM
To: Sergey Bylokhov; awt-...@openjdk.java.net
Subject: RE:  [10] Review Request: JDK-8159142 : Visible artifacts in 
sun/java2d/SunGraphics2D/DrawImageBilinear.java

Hi Sergey,

XRSurfaceData, GLXSurfaceData:On linux, only integer scale  is 
supported. If we give non-integer, the the uiScale is truncated to integer and 
used. So the ceil or round does not matter. In both XRSurfaceData and 
GLXSurfaceData, ceil function or direct multiplication is used arbitrarily at 
different places. So, we can make changes for making things more consistent, 
but it is not necessary as of now. 
PS: There are artifacts in same test case on Linux and there is a separate bug 
https://bugs.openjdk.java.net/browse/JDK-8189957. But these don't seem to be 
result of this precision error. I will look into this bug later.

WGLSurfaceData: In openGL, ceil function is used in WGLSurfaceData, so it 
will create WGLOffScreenSurfaceData and in turn create native resource. Then in 
DrawImage class, the clipRound is used, and these sizes are sent to native 
size. Then in OGLBlitTextureToSurface function inside OGLBlitLoops file, the 
texture coordinates are calculated by dividing the sizes sent from Java side by 
the native resource size. So the texture coordinates are wrong. For instance, 
in DrawImageBilinear, the size is 5x5 and uiScale is 2.5, so the native 
resource is of size, 13x13. But the dimensions sent to native are 12x12, so the 
textures are (0,0) and (12/13, 12/13). So artifacts in final image. 

D3DSurfaceData: In D3D, everything is similar to OpenGL, but in 
D3DBlitTextureToSurface function inside D3DBlitLoops file,  the calculated 
texture coordinates are sent to D3DDrawTextureWithHint and then data is copied 
from texture to surface. Here if the native resource size and dimensions sent 
from Java are not same, the data is copied in parts. This is somehow handling 
the wrong texture coordinates. So this test does not produce any artifacts in 
D3D pipeline. But it can in some other case. So this needs to be fixed.

GDIWindowSurfacedata: Here the volatile image creates a BufImageSurfaceData as 
backing resource using the ceil function and while copying from 
BugImageSurfaceData to a GDIWindowSurface, the 
Java_sun_java2d_loops_TransformHelper_Transform in native side is able to 
handle the difference in sizes. So the artifacts are not visible. But I think 
this should be fixed. So should make changes to getBackupImage function in 
SunVolatileImage to use clipRound instead of ceil. 

BufImageSurfaceData : It uses the image raster size to create the native 
resource.  The raster size is already scaled according to the uiScale. So no 
need to change this.

So in a nutshell,
I feel we may not change the code in XRSurfaceData, GLXSurfaceData as linux 
only support integer scaling. We can do it later if required.
We should change code for D3DSurfaceData, GDIWindowSurfacedata and 
WGLSurfacedata as it may cause artifacts. I have updated the webrev for the 
same. Please review.

Webrev:
http://cr.openjdk.java.net/~pbansal/8159142/webrev.01/

Regards,
Pankaj Bansal

-Original Message-
From: Sergey Bylokhov
Sent: Wednesday, November 1, 2017 11:46 PM
To: Pankaj Bansal; awt-...@openjdk.java.net
Subject: Re:  [10] Review Request: JDK-8159142 : Visible artifacts in 
sun/java2d/SunGraphics2D/DrawImageBilinear.java

Hi, Pankaj.
D3DSurfaceData, XRSurfaceData and probably other SurfaceData use simple 
multiplication or ceil(). Should we update them also? and the test to catch the 
errors there as well.

On 30/10/2017 03:39, Pankaj Bansal wrote:
> Hi All,
> 
> Please review the fix for JDK 10.
> 
> Bug:
> 
> https://bugs.openjdk.java.net/browse/JDK-8159142
> 
> Webrev:
> 
> http://cr.openjdk.java.net/~pbansal/8159142/webrev.00/
> 
> Issue:
> 
> The test runs with D3D and OpenGL both. The issue is with the OpenGL 
> run as there aee some visual artifacts in image when the HiDPI scale 
> is set to non-integer value like 2.5.
> 
> Fix:
> 
> The issue is due to precision error. In WGLSurfaceData.java, the width 
> and height is calculated by multiplying the width and height with scale.
> But the ceil function was being used instead of round. It should be 
> using round as at most of the places, the round function is being used.
> 
> Also changed the test to run at uiScale of 2.5 to catch any further errors.
> 
> Regards,
> 
> Pankaj Bansal
> 

--
Best regards, Sergey.

Re: [OpenJDK 2D-Dev] [10] Review Request: JDK-8159142 : Visible artifacts in sun/java2d/SunGraphics2D/DrawImageBilinear.java

2017-11-09 Thread Phil Race

Wasn't it suggested to have the Linux code stop truncating the uiScale 
if it was
explicitly set to a non-integer value ? Or at least have another 
property to tell it
not to truncate. This would facilitate ongoing testing of fixing Linux 
so we can support fractional values.


To test the software pipeline it may be that there should be one more 
line added to these

to disable the default accelerated pipeline

29 * @run main/othervm -Dsun.java2d.uiScale=2.5 DrawImageBilinear
30 * @run main/othervm -Dsun.java2d.uiScale=2.5 -Dsun.java2d.opengl=True 
-Dsun.java2d.opengl.fbobject=true DrawImageBilinear @run main/othervm 
-Dsun.java2d.uiScale=2.5 -Dsun.java2d.3d=false 
-Dsun.java2d.xrender=false That will ensure that on Windows we test the 
GDI pipeline and on Linux we test the X11 pipeline -phil.


On 11/09/2017 08:43 AM, Pankaj Bansal wrote:

Hi Sergey,

Do these changes look good? Please give your feedback.

Regards,
Pankaj Bansal

-Original Message-
From: Pankaj Bansal
Sent: Wednesday, November 8, 2017 8:09 PM
To: Sergey Bylokhov; awt-...@openjdk.java.net; 2d-dev@openjdk.java.net; Philip 
Race
Subject: RE:  [10] Review Request: JDK-8159142 : Visible artifacts in 
sun/java2d/SunGraphics2D/DrawImageBilinear.java

+ 2d-dev, Phil.

Regards,
Pankaj Bansal

-Original Message-
From: Pankaj Bansal
Sent: Friday, November 3, 2017 3:11 PM
To: Sergey Bylokhov; awt-...@openjdk.java.net
Subject: RE:  [10] Review Request: JDK-8159142 : Visible artifacts in 
sun/java2d/SunGraphics2D/DrawImageBilinear.java

Hi Sergey,


XRSurfaceData, GLXSurfaceData:On linux, only integer scale  is 
supported. If we give non-integer, the the uiScale is truncated to integer and 
used. So the ceil or round does not matter. In both XRSurfaceData and 
GLXSurfaceData, ceil function or direct multiplication is used arbitrarily at 
different places. So, we can make changes for making things more consistent, 
but it is not necessary as of now.
PS: There are artifacts in same test case on Linux and there is a separate bug 
https://bugs.openjdk.java.net/browse/JDK-8189957. But these don't seem to be 
result of this precision error. I will look into this bug later.

WGLSurfaceData: In openGL, ceil function is used in WGLSurfaceData, so it 
will create WGLOffScreenSurfaceData and in turn create native resource. Then in 
DrawImage class, the clipRound is used, and these sizes are sent to native 
size. Then in OGLBlitTextureToSurface function inside OGLBlitLoops file, the 
texture coordinates are calculated by dividing the sizes sent from Java side by 
the native resource size. So the texture coordinates are wrong. For instance, 
in DrawImageBilinear, the size is 5x5 and uiScale is 2.5, so the native 
resource is of size, 13x13. But the dimensions sent to native are 12x12, so the 
textures are (0,0) and (12/13, 12/13). So artifacts in final image.

D3DSurfaceData: In D3D, everything is similar to OpenGL, but in 
D3DBlitTextureToSurface function inside D3DBlitLoops file,  the calculated 
texture coordinates are sent to D3DDrawTextureWithHint and then data is copied 
from texture to surface. Here if the native resource size and dimensions sent 
from Java are not same, the data is copied in parts. This is somehow handling 
the wrong texture coordinates. So this test does not produce any artifacts in 
D3D pipeline. But it can in some other case. So this needs to be fixed.

GDIWindowSurfacedata: Here the volatile image creates a BufImageSurfaceData as 
backing resource using the ceil function and while copying from 
BugImageSurfaceData to a GDIWindowSurface, the 
Java_sun_java2d_loops_TransformHelper_Transform in native side is able to 
handle the difference in sizes. So the artifacts are not visible. But I think 
this should be fixed. So should make changes to getBackupImage function in 
SunVolatileImage to use clipRound instead of ceil.

BufImageSurfaceData : It uses the image raster size to create the native 
resource.  The raster size is already scaled according to the uiScale. So no 
need to change this.

So in a nutshell,
I feel we may not change the code in XRSurfaceData, GLXSurfaceData as linux 
only support integer scaling. We can do it later if required.
We should change code for D3DSurfaceData, GDIWindowSurfacedata and 
WGLSurfacedata as it may cause artifacts. I have updated the webrev for the 
same. Please review.

Webrev:
http://cr.openjdk.java.net/~pbansal/8159142/webrev.01/


Regards,
Pankaj Bansal


-Original Message-
From: Sergey Bylokhov
Sent: Wednesday, November 1, 2017 11:46 PM
To: Pankaj Bansal; awt-...@openjdk.java.net
Subject: Re:  [10] Review Request: JDK-8159142 : Visible artifacts in 
sun/java2d/SunGraphics2D/DrawImageBilinear.java

Hi, Pankaj.
D3DSurfaceData, XRSurfaceData and probably other SurfaceData use simple 
multiplication or ceil(). Should we update them also? and the test to catch the 
errors there as well.

On 30/10/2017 03:39, Pankaj Bansal wrote:

Hi All,

Please review the fix

Re: [OpenJDK 2D-Dev] [10] RFR: JDK-8188083- NPE in java.awt.image.FilteredImageSource.startProduction

2017-11-09 Thread Phil Race

Ok to the fix. But Sergey need to respond as to whether the answer about 
the test satisfies him.


-phil.

On 11/09/2017 02:18 AM, Prahalad Kumar Narayanan wrote:

Hello Everyone

First, Thanks to Phil & Sergey for their time in review & suggestions.

This is a follow-up on Phil's suggestion to investigate whether the fix 
proposed in webrev.00 could lead to deadlocks.
Link: http://cr.openjdk.java.net/~pnarayanan/8188083/webrev.00/

I inspected the code, and wrote code snippets to check the behavior.
Inferences are as follows-
 1. FilteredImageSource helps in producing the image, applying an image 
filter on the pixels, and sharing the result to the ImageConsumer.
 2. The object holds- An ImageProducer, An ImageFilter, and instantiates a 
HashTable for its operation.

 3. The methods of FilteredImageSource are not supposed to be invoked 
directly from user-code. So how are these methods invoked?
   . startProduction:
 . This method is invoked when a user creates an Image like- 
Component.createImage(new FilteredImageSource(imgProducer, imgFilter));
 . Such creation could occur on any thread in user code and 
this internally invokes FilteredImageSource.startProduction through 
ToolkitImage.
 . The method updates the hash table as required and invokes 
ImageProducer.startProduction to produce the image.
   . removeConsumer:
 . This method is invoked once ImageProducer is done with image 
preparation.
 . The exact thread in which the removeConsumer gets invoked 
depends on how ImageProducer prepares the image.

 4. Is there a possibility of a deadlock with the proposed fix ?
   . In my inspection & observation with code snippets, I found that 
removeConsumer could be invoked in two ways- Multi-threaded & Single threaded.
   . Multi-threaded-
 . This was observed with FileImageSource as ImageProducer
 . startProduction on this image producer requests image decode 
operation on a different thread- ImageFetcher thread.
 . Once decoding is complete, removeConsumer is invoked on the 
ImageFetcher thread.
 . Though startProduction & removeConsumer are invoked on 
different threads, there is no possibility of a deadlock as the code flow from 
startProduction returns immediately after posting the request to decode image onto 
ImageFetcher 's queue.
   . Single threaded-
 . This was observed with OffscreenImageSource as ImageProducer
 . startProduction and removeConsumer are invoked on the same 
thread.
 . Since re-entrant threads acquire the locks that they already 
hold, this wouldn't result in a deadlock.
   . Thus, in both cases I don't see possibility of a deadlock. Hence, 
we can consider the change in webrev.00 safe to fix the issue.

 5. I have run JTreg test cases before and after the proposed change on 
java/awt/image/ and javax/swing (where ImageFilter is used).
  No new regressions were observed during the test.

Kindly review by assessing the above information.
If you believe, the fix could be approved, kindly review the CSR as well.

Thank you once again for your time
Have a good day

Prahalad

- Original Message -
From: Philip Race
Sent: Thursday, October 26, 2017 9:30 PM
To: Prahalad Kumar Narayanan
Cc: Sergey Bylokhov; 2d-dev
Subject: Re: [OpenJDK 2D-Dev] [10] RFR: JDK-8188083- NPE in 
java.awt.image.FilteredImageSource.startProduction

The screenshot shows you directly calling this method in your test which
the documentation says you are not supposed to do.
So I am not able to be 100% sure that the test you have re-creates what
the submitter saw .. in his stack trace you have below it seems to be valid.

But to have a NPE at line 181 is odd.
  181 proxies.put(ic, imgf);

What is null ? proxies was just initialised in this same method so
I don't see how it can be null unless something elsewhere is
resetting it to null.

The only place I see that is removeConsumer
  138 public synchronized void removeConsumer(ImageConsumer ic) {
  139 if (proxies != null) {
  140 ImageFilter imgf =  proxies.get(ic);
  141 if (imgf != null) {
  142 src.removeConsumer(imgf);
  143 proxies.remove(ic);
  144 if (proxies.isEmpty()) {
  145 proxies = null;
  146 }
  147 }
  148 }
  149 }

Now that method is synchronized .. OK so I think it might make sense
to mark the additional methods synchronized too but then I automatically
worry about introducing a deadlock.

I can't say for sure that you have done so however and likely there
are no nested locks here so it is probably OK but please run as many
tests as you can find to try to ensure that.

Adding or removing

Re: [OpenJDK 2D-Dev] RFR JDK-8184429: Path clipper added in Marlin2D & MarlinFX 0.8.0

Re: [OpenJDK 2D-Dev] [10] RFR: JDK-8188083- NPE in java.awt.image.FilteredImageSource.startProduction

Re: [OpenJDK 2D-Dev] [10] Review Request: JDK-8159142 : Visible artifacts in sun/java2d/SunGraphics2D/DrawImageBilinear.java

Re: [OpenJDK 2D-Dev] [10] Review Request: JDK-8159142 : Visible artifacts in sun/java2d/SunGraphics2D/DrawImageBilinear.java

Re: [OpenJDK 2D-Dev] [10] RFR: JDK-8188083- NPE in java.awt.image.FilteredImageSource.startProduction

5 matches

Site Navigation

Mail list logo

Footer information