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

[Jim Graham]

I want to elaborate on winding count issues for segments to the left of the 
clip for both winding modes.

All curves have the property that the winding count of any sample point to the right of them is identical to the winding 
count of a line from their starting point to their ending point.


Consider a quad.  If it is monotonic, then the observation should be fairly obvious as the curve only ever has one 
crossing for any scanline and only the X location of that crossing varies and it only varies within the bounds of the X 
coordinates of all of 3 defining points.  Once you are to the right of all 3 X coordinates, the winding count is 
uniformly a single value over the entire Y range of the (monotonic) quad.


Consider a quad with a vertical reversal.  If one of the endpoints is higher than the other, then in the area between 
the Y coordinates of the endpoints it will increase or decrease the winding count by only 1, and the sign of that 
winding count change will be identical to a segment that connects the two endpoints.  For the rest of the quad, the part 
where it doubles back on itself, those 2 parts of the path cancel each other out.  You either have a downward portion 
that curves into an upward portion, or vice versa.  In both cases, that portion has a resulting winding count of 0 
because the direction of those 2 parts of the curve are opposite.


A cubic is more complicated with more cases to consider, but if you diagram it out you can see that the winding count 
contribution off to the right of the cubic will be identical in all cases to the winding count contribution of a line 
segment joining the two endpoints.  You can have up to 3 reversals, but all reversals that are above or below the end 
points will be paired with each other and all reversals within the Y range of the end points will either result in 1 or 
3 competing pieces and the 1 or the majority of the 3 will be in the same direction/sign as the direction/sign of the 
segment between the endpoints.  If there are 3 then 2 will cancel out and the remaining one will be the same direction 
as the endpoint line segment.


So, all path segments, regardless of line/quad/cubic that lie entirely to the left of the clip have identical winding 
count to a simple line segment.


And with the proper winding count computed, the winding mode has no impact, the same winding count is appropriate and 
accurate whether the entire path is EO or NZ...


...jim

On 8/31/17 4:15 PM, Jim Graham wrote:
For the Even-odd filling rule, I think it needs exact segment intersections on the left side, so I am focused on the 
Non-zero filling rule for now.


They are identical.  All that matters is that you have the proper starting winding count as you enter the clip from the 
left.  They could be replaced by either a segment with the same "Y range" as I mention above, or they could be replaced 
by having a value in the segments list that is "starting count", so if you have a segment that is out-left and it goes 
from Y=10 to Y=20, you simply add (or subtract for reversed lines) one to the "starting count" field for all scanlines 
from 10->20.

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

[Jim Graham]

Hi Laurent,

Area is overkill for this as it tries to find exact intersection points of arbitrary geometry.   You simply need 
something that will trace accurately around the outside of a clip to get from an exit point back to an entry point. 
That is a much simpler operation.


The performance issues with Area, btw, have nothing to do with array caching, they have to do with the numerical 
instabilities in performing an operation which boils down to be numerically equivalent to solving a 9th order equation. 
While you are welcome to investigate that, it will involve a much different set of techniques than what was applied to 
Marlin...  :(


Also, I thought that the Renderer already did basic clipping along the lines that you indicate.  It does that on a 
per-segment basis, but all it would take is a simple test at the top of quadTo() and curveTo() to completely reject all 
curves that lie outside the fill region (and simply shadow any part that is entirely to the left so that we maintain the 
proper crossings count)...


...jim

On 8/31/17 12:43 AM, Laurent Bourgès wrote:

Jim,

FYI I am working on a more general clipper for filled shapes (only non zero winding rule) that ignores useless segments 
on left / right sides but leaves the path closed for filling primitives.


It is more tricky... to handle turning points (corners) but I think it could be applied later to the Stroker case to 
avoid opening the path (and require ClosedPathDetector).


Should I look at the Area class that performs such clipping but is known to be extremely slow ? If you think it is 
useful, I could optimize it as I did in Marlin (array caching...). What is your opinion ? Is Area used in the java2d 
pipeline ? That would improve the overall performance.


PS: I wonder if curves should be subdivided at inflexion / root points in this (too) basic fill pipeline to improve 
cubic / quad accuracy (as it is the case in Stroker) and have monotonic curves.
I studied AFD accuracy (inc/dec thresholds) but did not try curve subdivision to guarantee the special point accuracy 
(cups...)


Hope you will have time soon to look at the webrev, your feedback may help a 
lot.

Cheers,
Laurent

Le 29 août 2017 2:58 AM, "Jim Graham" <james.gra...@oracle.com 
<mailto:james.gra...@oracle.com>> a écrit :

Hi Laurent,


On 8/28/17 2:09 PM, Laurent Bourgès wrote:

Hi Jim,

Thanks for your comments, it helped refining the webrev.

Here are my answers:

2017-08-26 2:22 GMT+02:00 Jim Graham <james.gra...@oracle.com 
<mailto:james.gra...@oracle.com>
<mailto:james.gra...@oracle.com <mailto:james.gra...@oracle.com>>>:


     [D]Dasher.java - why the changes from (firstSegIdx > 0) to 
(firstSegIdx != 0)?

As firstSegIdx is initialized to 0, I prefer testing (firstSegIdx!= 0) 
as it looks more obvious.

For me, (firstSegIdx > 0) indicates that the sign has a particular 
meaning and that firstSegIdxmay be negative.


Interesting, I'm used to != 0 being only used in contexts where the value 
might have some specific reason for being
negative, but I can see why you did that.


     [D]Stroker.java, line 196ish - why are ROUND treated differently.  
You have a question on that as well in a
comment.

I found the answer: C = 4/3 * (SQRT(2) - 1) is used to compute the 
control points (cubics) to approximate a
circle. I fixed the constant estimation according to the math formula.


The control points don't control how far the path gets from the line, 
though - that measurement is arbitrary
compared to the clipping operation.  The correct distance from the vertex 
to the edge of the drawn path is lw/2.


I agree your new rules.
I fixed the (D)Stroker init() methods according the latter rules and 
tested again.


Looks good.


Probably I should write a new Clip test rendering Z shapes with all  
(cap / join) combinations and their bounds
aligned to the Stroker's outside clip rules.

Here is an updated webrev (Marlin2D only):
http://cr.openjdk.java.net/~lbourges/marlin/marlin-080.1/
<http://cr.openjdk.java.net/~lbourges/marlin/marlin-080.1/>

PS: I can send you an updated MarlinFX patch (when you need it).


Not yet until I've had a chance to review the guts of the algorithm.  So 
far I've only looked at a few boundary
changes.  I'll get to that soon...

                         ...jim

Re: RFR JDK-8184429: Path clipper added in Marlin2D & MarlinFX 0.8.0

[Jim Graham]

Hi Laurent,

On 8/28/17 2:09 PM, Laurent Bourgès wrote:

Hi Jim,

Thanks for your comments, it helped refining the webrev.

Here are my answers:

2017-08-26 2:22 GMT+02:00 Jim Graham <james.gra...@oracle.com 
<mailto:james.gra...@oracle.com>>:

[D]Dasher.java - why the changes from (firstSegIdx > 0) to (firstSegIdx != 
0)?

As firstSegIdx is initialized to 0, I prefer testing (firstSegIdx!= 0) as it 
looks more obvious.
For me, (firstSegIdx > 0) indicates that the sign has a particular meaning and 
that firstSegIdxmay be negative.


Interesting, I'm used to != 0 being only used in contexts where the value might have some specific reason for being 
negative, but I can see why you did that.



[D]Stroker.java, line 196ish - why are ROUND treated differently.  You have 
a question on that as well in a comment.

I found the answer: C = 4/3 * (SQRT(2) - 1) is used to compute the control points (cubics) to approximate a circle. I 
fixed the constant estimation according to the math formula.


The control points don't control how far the path gets from the line, though - that measurement is arbitrary compared to 
the clipping operation.  The correct distance from the vertex to the edge of the drawn path is lw/2.



I agree your new rules.
I fixed the (D)Stroker init() methods according the latter rules and tested 
again.


Looks good.

Probably I should write a new Clip test rendering Z shapes with all  (cap / join) combinations and their bounds aligned 
to the Stroker's outside clip rules.


Here is an updated webrev (Marlin2D only):
http://cr.openjdk.java.net/~lbourges/marlin/marlin-080.1/

PS: I can send you an updated MarlinFX patch (when you need it).


Not yet until I've had a chance to review the guts of the algorithm.  So far I've only looked at a few boundary changes. 
 I'll get to that soon...


...jim

Re: RFR JDK-8184429: Path clipper added in Marlin2D & MarlinFX 0.8.0

[Jim Graham]

Hi Laurent,

I'm just reading through the code now to get a handle on the nature of the 
changes...(and starting with the 2D version)...

[D]Dasher.java - why the changes from (firstSegIdx > 0) to (firstSegIdx != 0)?
[D]Dasher.java - why is there a new goto_starting() which is only used from one place?  To be able to add final to the 
parameters?

[D]Dasher.java, line 268ish - why move the call to out.moveto() down a line?

[D]Stroker.java, line 170ish - you added final to the float params, but not the 
double
[D]Stroker.java, line 196ish - why are ROUND treated differently.  You have a 
question on that as well in a comment.
[D]Stroker.java, line 196ish - CAP_SQUARE would require more than lw/2 padding. 
 I think it needs lw*sqrt(2)/2 padding.

I would think the padding would be (max of the CAP/JOIN values below):
CAP_BUTT - lw/2
CAP_ROUND - lw/2
CAP_SQUARE - lw/2 * sqrt(2)
JOIN_BEVEL - lw/2
JOIN_ROUND - lw/2
JOIN_MITER - max(lw/2, miter_limit * lw)

In other words:
- lw/2 by default
- if CAP_SQUARE then multiply that by sqrt(2)
- if JOIN_MITER then max it with limit

I'm still looking through the management of the closed path detection coordinates, but I thought I'd get at least these 
questions out first before the weekend...


...jim

On 8/25/17 1:09 PM, Laurent Bourgès wrote:

Hi,

Please review Marlin/FX upgrades that provide an efficient path clipper in
Stroker (float / double variants) fixing the bug JDK-8184429


Marlin2D patch (jdk10):
http://cr.openjdk.java.net/~lbourges/marlin/marlin-080.0/
MarlinFX patch (openjfx10):
http://cr.openjdk.java.net/~lbourges/marlinFX/marlin-080.0/

Path Clipper in (D)Stroker:
- it uses outcode computation (cohen - sutherland) for segment edges (2 for
lines, 3 for quads, 4 for cubics)
- it opens the path when a segment is invisible ie special moveTo() and
ignore invisible joins; it does not compute any intersection (line /
curve), it just skips useless segment for better performance and accuracy
- the ClosedPathDetector is needed to infer if the path is closed or not
(call to closePath) to produce caps when appropriate. It reuses the former
PolyStack (moved into Helper classes) to store the forward segments
- the clip rectangle is adjusted either in Stroker but also in
Transformer2D to take into account the mitter limit, stroker width and also
the Renderer offset

That's why it can not be applied to closed paths (fill operations) as the
path must remain closed in such case (concave polygon).
This could be implemented later as it needs to insert corner points when
needed to avoid artefacts; so the algorithm seem more complicated to me.

Marlin2D / FX Patches are slightly different to handle the Renderer offsets.

I tested these patches against my MapBench test with a small clip and
several affine transforms: it does not produce any artefact (0 pixel
difference between clip=true/false)

PS: I also improved the accuracy of Renderer's AFD by using the kaham
compensated-sum approach (later patch)

Cheers,
Laurent Bourgès

Re: [10] RFR: JDK-8133329: Drag and Drop of files in a SwingNode fails

[Jim Graham]

This looks good... +1

...jim

On 8/3/17 9:20 PM, Prasanta Sadhukhan wrote:



On 8/4/2017 2:57 AM, Jim Graham wrote:

I noticed a "FIXME" comment in there.  Didn't we do a pass a while back and 
convert all of them into JBS entries?

The new code simply repeats what was done for the existing Exception which is what has the "FIXME" comment so I would 
use this opportunity to upgrade that FIXME into an actual bug entry.


Also, perhaps we could use the multi-catch syntax here instead of creating a 
new catch?


Ok. I have put it as a mult-catch in this webrev
http://cr.openjdk.java.net/~psadhukhan/fx/8133329/webrev.01/

Regards
Prasanta

...jim

On 8/3/17 2:01 AM, Prasanta Sadhukhan wrote:

Hi All,

Please review a fix
http://cr.openjdk.java.net/~psadhukhan/fx/8133329/webrev.00/
for issue
https://bugs.openjdk.java.net/browse/JDK-8133329
More information in JBS.

Regards
Prasanta

Re: [10] RFR: JDK-8133329: Drag and Drop of files in a SwingNode fails

[Jim Graham]

I noticed a "FIXME" comment in there.  Didn't we do a pass a while back and 
convert all of them into JBS entries?

The new code simply repeats what was done for the existing Exception which is what has the "FIXME" comment so I would 
use this opportunity to upgrade that FIXME into an actual bug entry.


Also, perhaps we could use the multi-catch syntax here instead of creating a 
new catch?

...jim

On 8/3/17 2:01 AM, Prasanta Sadhukhan wrote:

Hi All,

Please review a fix
http://cr.openjdk.java.net/~psadhukhan/fx/8133329/webrev.00/
for issue
https://bugs.openjdk.java.net/browse/JDK-8133329
More information in JBS.

Regards
Prasanta

[10] Review request for 8183530: JavaFX charts peg rendering thread as more data is added

[Jim Graham]

JBS: https://bugs.openjdk.java.net/browse/JDK-8183530
webrev: http://cr.openjdk.java.net/~flar/JDK-8183530/webrev.03/

...jim

Re: javafx.scene.shape.Path (memory) inefficient PathElements

[Jim Graham]

And, to clarify, the super-lazy property fix is currently only targeted to JDK10.  A case would need to be made to 
back-port it to any update releases once it is fixed there.  We haven't seen or presented such a case at this point so 
we are only planning to fix it in JDK10 and Tom has indicated that he has found his own workaround for JDK8.


The other bug related to creating a new Path object with lighter weight storage would be new API and not be eligible to 
back-porting.


8u144 was not a general update release, it was an emergency regression fix for the 8u141 CPU release.  Even if we had 
pushed this into JDK10, it would not have been eligible to go into either of those releases...


...jim

On 7/27/17 5:07 AM, Kevin Rushforth wrote:

No, this fix hasn't yet been reviewed and pushed. When it is it will go into 
JDK 10.

-- Kevin


Jose Martinez wrote:

Just curious, did this make it to the 8u144 release?  If not, any idea which 
release (or when) we can expect it?
Thank youjose


On ‎Wednesday‎, ‎May‎ ‎24‎, ‎2017‎ ‎08‎:‎38‎:‎41‎ ‎PM‎ ‎EDT, Jim Graham 
<james.gra...@oracle.com> wrote:

Thanks Tom, I've already posted a patch for 8180938 (lazy property creation).  Check it out and let me know how it 
performs for you.


I have a couple of changes to make to it (and an independent memory usage test to write) before I send it out for 
formal review...


...jim

On 5/24/17 3:42 AM, Tom Schindl wrote:

Hi,

I created:
- https://bugs.openjdk.java.net/browse/JDK-8180935
- https://bugs.openjdk.java.net/browse/JDK-8180938

I'll work on a showcase to find out how much memory one can save.

Tom

On 04.05.17 23:33, Jim Graham wrote:

Hi Tom,

Those look like good suggestions.  I would file bugs in JBS and create
them separately:

- Bug for lazy property creation in path elements
- Feature request for lower-memory paths

Did you benchmark how much the lazy properties, on their own, would save
your application?

  ...jim

On 5/4/17 2:22 PM, Tom Schindl wrote:

Hi,

We are currently working on a PDF-Rendering library in JavaFX and we
need to draw glyphs using the JavaFX Path API (there are multiple
reasons why we don't use the Text-Node and or Canvas).

When drawing a page full of Text this means that we have a Path-Object
with gazillions of MoveTo and CubicCurveTo elements who sum up to 30MB
just to represent them in the SG because PathElements store their
information in properties and forcefully intialize them in their
constructor.

The only public API to work around this problem is to construct a
StringBuffer and use SVGPath which means:
* it takes time to construct the SVG-Path-String
* it takes time to parse the SVG-Path-String in JavaFX

As an experiment (and because we are still on Java8 we can easily do
that) was that we created our own Shape-Subclass who:
* uses floats (there's no reason to use double in the SG when the
backing API - Path2D - is float based)
* passes the floats directly to the Path2D/NGPath API

Guess what: We now need 2.5MB / page which means 27.5MB is the overhead
added by the current Path-API - ouch!

I think a fairly low hanging memory optimization for the PathElement
would be to create properties lazy (only if someone access the property).

For MoveTo this would mean the following minimal change (eg for the
x-value):

private DoubleProperty x;
private double _x;

public final void setX(double value) {
if (x != null) {
  xProperty().set(value);
} else {
  _x = value;
  u();
}
}

public final double getX() {
return x == null ? _x : x.get();
}

public final DoubleProperty xProperty() {
if (x == null) {
  x = new DoublePropertyBase(_x) {

@Override
public void invalidated() {
  u();
}

@Override
public Object getBean() {
  return MoveTo.this;
}

@Override
public String getName() {
  return "x";
}
};
}
return x;
}

I guess 99% of the code out there never access the Property so the small
footprint added by the primitive field is justifiable.

This still has the overhead of all the needless PathElement objects
hanging around so another idea is to have a 3rd SG-Path-Type who
strictly uses the float-Primitives with a similar API to Path2D (in fact
it only acts as a public API to Path2D).

Thoughts?

Tom

[10] Review request: JDK-8181976 - Specifying desired dimensions for Image with HiDPI (@2x) variant gets the size wrong

[Jim Graham]

JBS: https://bugs.openjdk.java.net/browse/JDK-8181976
webrev: http://cr.openjdk.java.net/~flar/JDK-8181976/webrev.00/

Simple fix is to carry the double "size requested" values all the way down to where the image pixel scale is determined 
(not strictly required, but important for precision) and then to scale them with the final image pixel scale before 
rounding to integer and using them to resize the thumbnail...


...jim

Re: CFV: New OpenJFX Committer: Ajit Ghaisas

[Jim Graham]

Vote: yes

...jim

On 5/26/17 5:20 AM, Kevin Rushforth wrote:

I hereby nominate Ajit Ghaisas [1] to OpenJFX Committer.

Ajit is a member of JavaFX team at Oracle, who has contributed 10 changesets to OpenJFX. A list of these changesets is 
available by the following link:


http://hg.openjdk.java.net/openjfx/10-dev/rt/log?revcount=40=author%28aghaisas%29

Votes are due by June 9, 2017.

Only current OpenJFX Committers [2] are eligible to vote on this nomination. Votes must be cast in the open by replying 
to this mailing list.


For Lazy Consensus voting instructions, see [3]. Nomination to a project 
Committer is described in [4].

Thanks.

-- Kevin

[1] http://openjdk.java.net/census#aghaisas

[2] http://openjdk.java.net/census#openjfx

[3] http://openjdk.java.net/bylaws#lazy-consensus

[4] http://openjdk.java.net/projects#project-committer

Re: javafx.scene.shape.Path (memory) inefficient PathElements

[Jim Graham]

Thanks Tom, I've already posted a patch for 8180938 (lazy property creation).  Check it out and let me know how it 
performs for you.


I have a couple of changes to make to it (and an independent memory usage test to write) before I send it out for formal 
review...


...jim

On 5/24/17 3:42 AM, Tom Schindl wrote:

Hi,

I created:
- https://bugs.openjdk.java.net/browse/JDK-8180935
- https://bugs.openjdk.java.net/browse/JDK-8180938

I'll work on a showcase to find out how much memory one can save.

Tom

On 04.05.17 23:33, Jim Graham wrote:

Hi Tom,

Those look like good suggestions.  I would file bugs in JBS and create
them separately:

- Bug for lazy property creation in path elements
- Feature request for lower-memory paths

Did you benchmark how much the lazy properties, on their own, would save
your application?

 ...jim

On 5/4/17 2:22 PM, Tom Schindl wrote:

Hi,

We are currently working on a PDF-Rendering library in JavaFX and we
need to draw glyphs using the JavaFX Path API (there are multiple
reasons why we don't use the Text-Node and or Canvas).

When drawing a page full of Text this means that we have a Path-Object
with gazillions of MoveTo and CubicCurveTo elements who sum up to 30MB
just to represent them in the SG because PathElements store their
information in properties and forcefully intialize them in their
constructor.

The only public API to work around this problem is to construct a
StringBuffer and use SVGPath which means:
* it takes time to construct the SVG-Path-String
* it takes time to parse the SVG-Path-String in JavaFX

As an experiment (and because we are still on Java8 we can easily do
that) was that we created our own Shape-Subclass who:
* uses floats (there's no reason to use double in the SG when the
   backing API - Path2D - is float based)
* passes the floats directly to the Path2D/NGPath API

Guess what: We now need 2.5MB / page which means 27.5MB is the overhead
added by the current Path-API - ouch!

I think a fairly low hanging memory optimization for the PathElement
would be to create properties lazy (only if someone access the property).

For MoveTo this would mean the following minimal change (eg for the
x-value):

private DoubleProperty x;
private double _x;

public final void setX(double value) {
   if (x != null) {
  xProperty().set(value);
   } else {
  _x = value;
  u();
   }
}

public final double getX() {
   return x == null ? _x : x.get();
}

public final DoubleProperty xProperty() {
   if (x == null) {
 x = new DoublePropertyBase(_x) {

   @Override
   public void invalidated() {
 u();
   }

   @Override
   public Object getBean() {
 return MoveTo.this;
   }

   @Override
   public String getName() {
 return "x";
   }
};
   }
   return x;
}

I guess 99% of the code out there never access the Property so the small
footprint added by the primitive field is justifiable.

This still has the overhead of all the needless PathElement objects
hanging around so another idea is to have a 3rd SG-Path-Type who
strictly uses the float-Primitives with a similar API to Path2D (in fact
it only acts as a public API to Path2D).

Thoughts?

Tom

Re: MarlinFX upgrade 0.7.5

[Jim Graham]

This looks good.  Did you create a JBS bug?

...jim

On 5/16/17 2:11 PM, Laurent Bourgès wrote:

Finally I propose another MarlinFX webrev:
http://cr.openjdk.java.net/~lbourges/marlinFX/marlinFX-075.2/

Changes:
- (D)Helpers: revert syntax changed in cubicRootsInAB() + fixed comment in
subdivide()
- (D)MarlinRenderingEngine: added logs for cubic/quad properties
- (D)Renderer and (D)RendererNoAA: use cubic/quad properties like Marlin2D
- (D)RendererNoAA: fixed syntax changes mentioned: missing "d" suffix +
comment about test
- MarlinProperties: added quality settings (cubic/quad)


PS: I will send an updated webrev for the Marlin2D patch.


Regards,
Laurent

Re: MarlinFX upgrade 0.7.5

[Jim Graham]

That's fine, I just wanted to make sure the difference was intentional and not 
something that fell through the cracks...

...jim

On 5/16/17 2:17 PM, Laurent Bourgès wrote:

One more comment about '(D)RendererSharedMemory in FX, but not 2D':

This reduces the memory footprint of the RendererContext among Renderer and
RendererNoAA implementations (shared arrays) that can co-exist at runtime
if both AA and NonAA rendering is needed.

As this behavior is unique to JavaFX, I wonder if it is worth to backport
this small difference in Marlin2D where there will be no gain.

Laurent


2017-05-16 23:11 GMT+02:00 Laurent Bourgès :


Hi Jim,

I didn't notice anything functionally wrong with reviewing the webrev.

I'm going to do some basic testing on it now while you create a JBS bug for
it and submit for a formal review.  But I did notice some discrepancies by
diffing the sources against each other which it would be nice to know if
they were oversights or "future work".  If there is something you think
should be addressed now, do that in the process of switching to a formal
review (with JBS #) on it...



I will create the JBS bug asap.



I did a bit of diffing:

- Marlin2D against MarlinFX
- *NoAA against the AA versions

and noticed:



Thanks for your carefull analysis, here are my comments below:

- RendererNoAA and DRendererNoAA line 38 are missing a "d" suffix.

- RendererNoAA and DRendererNoAA lines 61,90 - missing comment about test



I forgot to synchronize recent changes into RendererNoAA variants: good
catch !



- Helpers and DHelpers - some adjustment of "pts[ off+1 ]" lines that
didn't happen in 2D



I decided to revert those minor syntax changes.



- Configurable constants for Inc/Dec/Quad/Cubic in 2D, but not FX



I hesitated but decided to backport the support for these configurable
constants in all (D)Renderer(NoAA) (4 variants)



- DRendererSharedMemory in FX, but not 2D





- FloatMath.ceil_f deleted from FX, but not 2D (not used in 2D)
- (FloatMath.floor_f is also only in 2D, but it is used in
MarlinRenderingEngine)



I decided to leave those FloatMath differences as Marlin2D (and FloatMath
Tests) do use these methods.


Finally I propose another MarlinFX webrev:
http://cr.openjdk.java.net/~lbourges/marlinFX/marlinFX-075.2/

Changes:
- (D)Helpers: revert syntax changed in cubicRootsInAB() + fixed comment in
subdivide()
- (D)MarlinRenderingEngine: added logs for cubic/quad properties
- (D)Renderer and (D)RendererNoAA: use cubic/quad properties like Marlin2D
- (D)RendererNoAA: fixed syntax changes mentioned: missing "d" suffix +
comment about test
- MarlinProperties: added quality settings (cubic/quad)


PS: I will send an updated webrev for the Marlin2D patch.


Regards,
Laurent

Re: MarlinFX upgrade 0.7.5

[Jim Graham]

Hi Laurent,

I didn't notice anything functionally wrong with reviewing the webrev.  I'm going to do some basic testing on it now 
while you create a JBS bug for it and submit for a formal review.  But I did notice some discrepancies by diffing the 
sources against each other which it would be nice to know if they were oversights or "future work".  If there is 
something you think should be addressed now, do that in the process of switching to a formal review (with JBS #) on it...


I did a bit of diffing:

- Marlin2D against MarlinFX
- *NoAA against the AA versions

and noticed:

- RendererNoAA and DRendererNoAA line 38 are missing a "d" suffix.
- RendererNoAA and DRendererNoAA lines 61,90 - missing comment about test
- Helpers and DHelpers - some adjustment of "pts[ off+1 ]" lines that didn't 
happen in 2D
- Configurable constants for Inc/Dec/Quad/Cubic in 2D, but not FX
- DRendererSharedMemory in FX, but not 2D
- FloatMath.ceil_f deleted from FX, but not 2D (not used in 2D)
- (FloatMath.floor_f is also only in 2D, but it is used in 
MarlinRenderingEngine)

...jim

On 5/11/17 2:14 PM, Laurent Bourgès wrote:

Hi Jim,

Please review this updated webrev:
http://cr.openjdk.java.net/~lbourges/marlinFX/marlinFX-075.1/

I mainly synchronized again all my Marlin repositories and this provides an
up-to-date patch in synch with Marlin2D.

Changes:
- Renderer (4 variants): use shared memory among AA and NonAA renderer
- Fixed comments in Helpers, XXXArrayCache
- Marlin2D backports in ArrayCacheConst, Renderer ...
- SWUtils: remove import + JavaShapeRenderer made static
- fixed copyright year to 2017

You can see the incremental changes by comparing the openjfx10.patch:
diff marlinFX-075.0/openjfx10.patch marlinFX-075.1/openjfx10.patch

Regards,
Laurent


2017-04-26 7:06 GMT+02:00 Jim Graham <james.gra...@oracle.com>:


I've reviewed the code and run a number of tests.  Things look fine.

I spotted at least one thing that I brought up in the 2D Marlin review,
but since the 2 source bases are moving towards synchronizing with each
other I didn't look too closely since many of the changes in the 2D Marlin
update are things that are already "fixed" in this FX Marlin code, so I
thought I would focus my scrutiny more on the 2D review instead. Would this
code base be affected by the review comments I made there?  Did you want to
hold both until they both are ready to go in and then push them at the same
time (to keep them in sync)?

Minimally, it is time to file a bug against FX for this...

...jim


On 4/19/17 11:35 PM, Laurent Bourgès wrote:


Hi,

Please review this MarlinFX upgrade to Marlin 0.7.5:

JBS: no bug yet for OpenJFX 10
webrev: http://cr.openjdk.java.net/~lbourges/marlinFX/marlinFX-075.0/

Changes:
- Renderers: fixed block processing
- dead code & few comment removals in Strokers
- fixed all floating-point number literals to be x.0f or x.0d to simplify
the conversion between float & double variants

PS: I plan to run later FindBugs, Netbeans & IntelliJ code analysis tools
to fix any warning

Cheers,
Laurent

[10] Review request: JDK-8179946 - Objects are not rendered for certain rotation angle and cache hint combinations

[Jim Graham]

JBS: https://bugs.openjdk.java.net/browse/JDK-8179946
webrev: http://cr.openjdk.java.net/~flar/JDK-8179946/webrev.00/

This should get back-ported to 9 as well, as soon as makes sense...

...jim

Re: javafx.scene.shape.Path (memory) inefficient PathElements

[Jim Graham]

Hi Tom,

Those look like good suggestions.  I would file bugs in JBS and create them 
separately:

- Bug for lazy property creation in path elements
- Feature request for lower-memory paths

Did you benchmark how much the lazy properties, on their own, would save your 
application?

...jim

On 5/4/17 2:22 PM, Tom Schindl wrote:

Hi,

We are currently working on a PDF-Rendering library in JavaFX and we
need to draw glyphs using the JavaFX Path API (there are multiple
reasons why we don't use the Text-Node and or Canvas).

When drawing a page full of Text this means that we have a Path-Object
with gazillions of MoveTo and CubicCurveTo elements who sum up to 30MB
just to represent them in the SG because PathElements store their
information in properties and forcefully intialize them in their
constructor.

The only public API to work around this problem is to construct a
StringBuffer and use SVGPath which means:
* it takes time to construct the SVG-Path-String
* it takes time to parse the SVG-Path-String in JavaFX

As an experiment (and because we are still on Java8 we can easily do
that) was that we created our own Shape-Subclass who:
* uses floats (there's no reason to use double in the SG when the
  backing API - Path2D - is float based)
* passes the floats directly to the Path2D/NGPath API

Guess what: We now need 2.5MB / page which means 27.5MB is the overhead
added by the current Path-API - ouch!

I think a fairly low hanging memory optimization for the PathElement
would be to create properties lazy (only if someone access the property).

For MoveTo this would mean the following minimal change (eg for the
x-value):

private DoubleProperty x;
private double _x;

public final void setX(double value) {
  if (x != null) {
 xProperty().set(value);
  } else {
 _x = value;
 u();
  }
}

public final double getX() {
  return x == null ? _x : x.get();
}

public final DoubleProperty xProperty() {
  if (x == null) {
x = new DoublePropertyBase(_x) {

  @Override
  public void invalidated() {
u();
  }

  @Override
  public Object getBean() {
return MoveTo.this;
  }

  @Override
  public String getName() {
return "x";
  }
   };
  }
  return x;
}

I guess 99% of the code out there never access the Property so the small
footprint added by the primitive field is justifiable.

This still has the overhead of all the needless PathElement objects
hanging around so another idea is to have a 3rd SG-Path-Type who
strictly uses the float-Primitives with a similar API to Path2D (in fact
it only acts as a public API to Path2D).

Thoughts?

Tom

Re: MarlinFX upgrade 0.7.5

[Jim Graham]

Hi Laurent,

On 4/26/17 2:37 PM, Laurent Bourgès wrote:

I spotted at least one thing that I brought up in the 2D Marlin review, but 
since the 2 source bases are moving
towards synchronizing with each other I didn't look too closely since many 
of the changes in the 2D Marlin update
are things that are already "fixed" in this FX Marlin code, so I thought I 
would focus my scrutiny more on the 2D
review instead. Would this code base be affected by the review comments I 
made there?  Did you want to hold both
until they both are ready to go in and then push them at the same time (to 
keep them in sync)?

I really do not know how to push both patches at the same time as it points to 
different forrests ... (I only have
commit rights in graphics repo ?) Probably I let you push the final patches 
once ready and I will also maintain my
github branches in sync.


I wasn't talking about literally simultaneously, I was referring to waiting to push either until both have been reviewed 
and approved...


...jim

Re: MarlinFX upgrade 0.7.5

[Jim Graham]

I've reviewed the code and run a number of tests.  Things look fine.

I spotted at least one thing that I brought up in the 2D Marlin review, but since the 2 source bases are moving towards 
synchronizing with each other I didn't look too closely since many of the changes in the 2D Marlin update are things 
that are already "fixed" in this FX Marlin code, so I thought I would focus my scrutiny more on the 2D review instead. 
Would this code base be affected by the review comments I made there?  Did you want to hold both until they both are 
ready to go in and then push them at the same time (to keep them in sync)?


Minimally, it is time to file a bug against FX for this...

...jim

On 4/19/17 11:35 PM, Laurent Bourgès wrote:

Hi,

Please review this MarlinFX upgrade to Marlin 0.7.5:

JBS: no bug yet for OpenJFX 10
webrev: http://cr.openjdk.java.net/~lbourges/marlinFX/marlinFX-075.0/

Changes:
- Renderers: fixed block processing
- dead code & few comment removals in Strokers
- fixed all floating-point number literals to be x.0f or x.0d to simplify
the conversion between float & double variants

PS: I plan to run later FindBugs, Netbeans & IntelliJ code analysis tools
to fix any warning

Cheers,
Laurent

Re: consider setInput/getInput as method in Effect

[Jim Graham]

Some effects have multiple inputs and have no single bare getInput/setInput 
methods and ColorInput has no input at all...

...jim

On 4/21/17 5:13 AM, Jose Martinez wrote:

Hello,
Shouldn't the setInput/getInput method be part of the Effect abstract class?  
Even if its just there as an abstract method with no default implementation.
I cannot do simple things like this (not that this is simple, but simpler than 
the alternative):
private void addNewEffect(Node node, Effect myNewEffect) {Effect effect = 
node.getEffect();if (effect != null) {while (effect.getInput() != 
null) {effect = effect.getInput();}
effect.setInput(myNewEffect);} else {node.setEffect(myNewEffect);   
 }}

Instead I find myself write code like this (and this would not scale well):
//this works because the two Effects below have been declared as DropShadow 
and ColorAdjust
private void refreshEffects() {
if (destroyed && selected) {
selectedEffect.setInput(destroyedEffect);
imageView.setEffect(selectedEffect);
} else if (!destroyed && selected) {
selectedEffect.setInput(null);
imageView.setEffect(selectedEffect);
} else if (destroyed && !selected) {
destroyedEffect.setInput(null);
imageView.setEffect(destroyedEffect);
} else if (!destroyed && !selected) {
imageView.setEffect(null);
}
}

Case in point:  http://stackoverflow.com/a/32020458/1490322
Maybe there is a good reason for not having setInput/getInput in Effect, as I 
suspect there is, in that case, sorry for the email (I tried searching for the 
reason).  One concern might be that some Effects just cannot play well with 
others.  In that case may I suggest another method in Effect called 
isChainable().  No matter what is done I can definitely see that this is not 
easy and can appreciate that some compromises have been made that lead to 
setInput/getInput not being in Effect.
thanksjose

[10] Review request: 8178521 Severe performance drop for path rendering

[Jim Graham]

Review history already in JBS: https://bugs.openjdk.java.net/browse/JDK-8178521
final webrev: http://cr.openjdk.java.net/~flar/JDK-8178521/webrev.02/

This should be considered for a backport to 9 as well...

...jim

Re: [10] Review request: 8177985 Use Marlin renderer as the default FX rasterizer

[Jim Graham]

Yes...

...jim

On 4/14/17 1:45 PM, Michael Paus wrote:

Will marlindouble remain the default when you say nothing or when you specify 
just marlin?

Am 14.04.17 um 22:31 schrieb Jim Graham:

JBS: https://bugs.openjdk.java.net/browse/JDK-8177985
webrev: http://cr.openjdk.java.net/~flar/JDK-8177985/webrev.00/

The only way to get one of the old Pisces rasterizers now is to manually 
disable Marlin (or use the new order option
as follows).

This fix also introduces a similar pattern for rasterizer selection that we use 
for pipeline selection:

-Dprism.rasterizerorder=listof(
marlin|marlinfloat|marlindouble|
pisces|javapisces|nativepisces|
*)
(unknown values are ignored for future/backwards-proofing)

Suggestions for naming of the new rasterizer identifiers and the "public name" 
strings are welcome...

...jim

Re: [10] Review request: 8177985 Use Marlin renderer as the default FX rasterizer

[Jim Graham]

I was going to tag Kevin and Laurent on this, but forgot to amend the address 
lines before I hit send...

...jim

On 4/14/17 1:31 PM, Jim Graham wrote:

JBS: https://bugs.openjdk.java.net/browse/JDK-8177985
webrev: http://cr.openjdk.java.net/~flar/JDK-8177985/webrev.00/

The only way to get one of the old Pisces rasterizers now is to manually 
disable Marlin (or use the new order option as
follows).

This fix also introduces a similar pattern for rasterizer selection that we use 
for pipeline selection:

-Dprism.rasterizerorder=listof(
marlin|marlinfloat|marlindouble|
pisces|javapisces|nativepisces|
*)
(unknown values are ignored for future/backwards-proofing)

Suggestions for naming of the new rasterizer identifiers and the "public name" 
strings are welcome...

...jim

[10] Review request: 8177985 Use Marlin renderer as the default FX rasterizer

[Jim Graham]

JBS: https://bugs.openjdk.java.net/browse/JDK-8177985
webrev: http://cr.openjdk.java.net/~flar/JDK-8177985/webrev.00/

The only way to get one of the old Pisces rasterizers now is to manually disable Marlin (or use the new order option as 
follows).


This fix also introduces a similar pattern for rasterizer selection that we use 
for pipeline selection:

-Dprism.rasterizerorder=listof(
marlin|marlinfloat|marlindouble|
pisces|javapisces|nativepisces|
*)
(unknown values are ignored for future/backwards-proofing)

Suggestions for naming of the new rasterizer identifiers and the "public name" 
strings are welcome...

...jim

Re: Canvas Content Shift

[Jim Graham]

On 4/14/17 7:36 AM, Michael Paus wrote:

For my Mac (Retina) I know that the renderScale is 2.0 but how do you find that 
out in the general case?


In JDK 9 you have:

Screen.getOutputScaleX/Y()
Window.outputScaleX/Y (read-only double properties, based on Screen values)
Window.renderScaleX/Y (get/set double properties, default to output scales)
Window.forceIntegerRenderScale (get/set boolean property)

In the case of Canvas, though, since we can't re-render currently (we have an outstanding wish-list item to add 
invalidation and repaint capabilities to it so we can make the data non-persistent) we have to choose the largest 
rendering scale that we might ever need - which is basically the max of all of the Screen output scales.


We need to add (for 10, too late for 9):

Canvas.renderScaleX/Y properties, gettable for all, settable only for 
non-persistent canvas objects

and maybe:

Canvas.repaint/validate/etc. for enabling non-persistence

...jim

Re: Canvas Content Shift

[Jim Graham]

Any suggestions on how to implement this when the size of pixels may be an 
arbitrary non-integer number?

Consider rendering on a 125% scaled Windows 10 screen.  If you want to scroll by 2 pixels you would want to scroll by 
1.6 coordinate units.  If you want to scroll by 2 coordinate units you are out of luck because that would attempt to 
"scroll by 2.5 pixels" and there is no good definition of that type of operation.


We could add a pixel size parameter (note that it might be different than the window or screen render scale because the 
Canvas cannot re-render and so it chooses the scale of the deepest screen).  Then it would be up to the developer to 
take this into account when determining how far to scroll, but that is a bit more complicated than what developers tend 
to be used to when they deal with scrolling.


Note that the scrolling of JViewport is handled by our own code, not developer code, so we can take these adjustments 
into account ourselves internally and know if/when we can blit or if/when we need to re-render...


...jim

On 4/10/17 1:32 AM, Dirk Lemmermann wrote:

HI there,

I was wondering if there is any chance that Java 10 could implement some sort 
of „content shifting“ for the Canvas API?

I would love to have this feature for supporting faster horizontal scrolling in 
my Gantt Chart framework FlexGanttFX. Currently when the user scrolls then the 
entire content of each canvas in each row of the chart will be redrawn. This 
could be optimized by only drawing the time range that has been moved into the 
„viewport“ and by shifting or copying the remaining time range graphics. E.g. 
the user currently looks at one week and scrolls one day to the right then the 
data / graphics of 6 days would stay the same and could just be reused. Only 
one day worth of data would need to be redrawn.

I think in Swing this is comparable with the BLIT_SCROLL_MODE of JViewport.

Dirk

Re: [10] Review request for 8176844: Menus not always selected properly with GTK 3

[Jim Graham]

I don't think I was specifically involved in AWT fixes for that issue, but the concerns that David raises are all valid 
and Phil correctly points out that this is much worse in a network display environment...


...jim

On 4/4/17 3:53 PM, Philip Race wrote:

AWT used to have really bad at X11 remote display and
it was looked at a few times and I think it was improved
noticeably when we could get rid of  "round trip" requests.
I think Jim had a hand in some of that work.

So I am sure a round trip - or similar - is bad for performance.

If you want to measure the effect of such change, remote display to
your desktop from a machine in a geographically distant site.

It is the latency that kills performance, not the bandwidth.

-phil.

On 4/4/17, 3:43 PM, David Hill wrote:

On 4/4/17, 1:27 PM, Semyon Sadetsky wrote:

Hello Kevin & David,

Please review the fix for jfx9:

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

webrev: http://cr.openjdk.java.net/~ssadetsky/8176844/

--Semyon



Semyon,

I have been sitting here for a while thinking about adding
gdk_display_sync(gdk_display_get_default());

I can see why this might address many issues, as it flushes the pipeline and 
waits for the X11 server to catch up.
That is balanced out by a historical distrust of using XSync in any situation 
where the consequences.

Part of me thinks it is minimal overhead though, the other part does not like 
stalling the asynchronous X11 design.

The other part of me would like to use this only for the window events that 
need it, instead of all of them.

and I found this in hte GTK docs:
gdk_events_pending ()
Waits for a GraphicsExpose or NoExpose event from the X server. This is used in 
the GtkText and GtkCList widgets in
GTK+ to make sure any GraphicsExpose events are handled before the widget is 
scrolled.

so perhaps this should be used in some cases (like setVisible).

sigh.

Will try to make up my mind tomorrow.

Dave.

[9] Review request for 8174671: Native debug build fails on Windows

[Jim Graham]

Fix suggested in description works just fine:

JBS: https://bugs.openjdk.java.net/browse/JDK-8174671
webrev: http://cr.openjdk.java.net/~flar/JDK-8174671/webrev.00/

...jim

[9] Review request for 8174688: JavaFX Applet popup windows are in the wrong location

[Jim Graham]

JBS: https://bugs.openjdk.java.net/browse/JDK-8174688
webrev: http://cr.openjdk.java.net/~flar/JDK-8174688/webrev.00/

It was a one-ish-line-ish fix (just needed to invert one Y coordinate consistent with other cases, but it took a couple 
of lines to fetch the necessary data for inversion).


Still need to investigate if it fails on 8u-dev where we have nearly the same 
code and request a backport if necessary.

Will need to file a couple of follow-on bugs to investigate the way we flip Y coordinates to make sure there are no 
additional, but more subtle, bugs in there.  Until then, this one fix improves the popup window positioning for the 
9-dev release and makes the code more consistent with the way we handle the Mac inverted Y coordinate systems...


...jim

[9, 8u] Review request for 8148549: Region is not rendered correctly when node cache is enabled

[Jim Graham]

JBS: https://bugs.openjdk.java.net/browse/JDK-8148549
webrev: http://cr.openjdk.java.net/~flar/JDK-8148549/webrev.00/

The webrev is for 9, but the same code appears in 8u so I will apply and test the fix there and request a backport when 
I'm sure that it works...


...jim

Re: 8173385: spelling errors in JavaFX javadoc

[Jim Graham]

The spelling changes look right, but "of Foo method" bothers me - it should be "of the Foo method".  The same comment 
would reply to the retainAll doc comment... :(


...jim

On 1/29/17 1:31 PM, Jonathan Giles wrote:

Ajit,

Could you please review the following jira issue and webrev:

https://bugs.openjdk.java.net/browse/JDK-8173385

http://cr.openjdk.java.net/~jgiles/8173385/

Thanks

Re: CFV: New OpenJFX Committer: Semyon Sadetsky

[Jim Graham]

Vote: yes

...jim

On 1/25/17 11:39 AM, David Hill wrote:


I hereby nominate Semyon Sadetsky to OpenJFX Committer.

Semyon Sadetsky is part of the JavaFX team focusing on glass.

A list of Semyon's commits and reviews is available by the following links

http://hg.openjdk.java.net/openjfx/9-dev/rt/log?rev=author%28ssadetsky%29

Votes are due by Feb 9th, 2017.

Only current OpenJFX Committers [1] are eligible to vote on this nomination. 
Votes must be cast in the open by replying
to this mailing list.

For Lazy Consensus voting instructions, see [2]. Nomination to a project 
Committer is described in [3].

[1] http://openjdk.java.net/census#openjfx

[2] http://openjdk.java.net/bylaws#lazy-consensus

[3] http://openjdk.java.net/projects#project-committer

Thanks,

Dave

Re: [8u] Review request for 8169777: MenuBar unoperable after moving Application to second monitor

[Jim Graham]

I was able to fix my Win7 permission problems and verify the fix on 
Windows 7 as well now...


...jim

On 1/5/2017 5:16 PM, Jim Graham wrote:

This is essentially a backport request for the fix for JDK-8160073, but
a modification of the fix was needed as the code changed between 8u and 9.

JBS: https://bugs.openjdk.java.net/browse/JDK-8169777
webrev: http://cr.openjdk.java.net/~flar/JDK-8169777/webrev.00/

I tested on Win10 and it works fine, though the platform support for DPI
scaling on Win8 (pre-8.1) and Win7 was different than their handling on
Win10 so a test run of the test case on Win7 and/or Win8.0 would be
helpful for sanity.  Unfortunately my Win7 boot partition is buried in a
cesspool of permission issues trying to run the test build... :(

...jim

Re: Review request: Fx applet fails to get loaded on Ubuntu with jre9-b150

[Jim Graham]

This fails if the coordinate is not on any screen (screen will remain null).

Also, spacing on the if statement at line 315...

...jim

On 1/11/17 1:54 PM, David Hill wrote:



Kevin, Jim,
   please review:

https://bugs.openjdk.java.net/browse/JDK-8171985
webrev: http://cr.openjdk.java.net/~ddhill/8171985.1 


--
David Hill 
Java Embedded Development

"A man's feet should be planted in his country, but his eyes should survey the 
world."
-- George Santayana (1863 - 1952)

[8u] Review request for 8169777: MenuBar unoperable after moving Application to second monitor

[Jim Graham]

This is essentially a backport request for the fix for JDK-8160073, but a modification of the fix was needed as the code 
changed between 8u and 9.


JBS: https://bugs.openjdk.java.net/browse/JDK-8169777
webrev: http://cr.openjdk.java.net/~flar/JDK-8169777/webrev.00/

I tested on Win10 and it works fine, though the platform support for DPI scaling on Win8 (pre-8.1) and Win7 was 
different than their handling on Win10 so a test run of the test case on Win7 and/or Win8.0 would be helpful for sanity. 
 Unfortunately my Win7 boot partition is buried in a cesspool of permission issues trying to run the test build... :(


...jim

[9] Review request for JDK-8171513: Fix typo in API doc for AnimationTimer

[Jim Graham]

JBS: https://bugs.openjdk.java.net/browse/JDK-8171513
The fix is small so the patch is included inline below...

...jim

---
diff -r 2bae8f04958b 
modules/javafx.graphics/src/main/java/javafx/animation/AnimationTimer.java
--- 
a/modules/javafx.graphics/src/main/java/javafx/animation/AnimationTimer.java
Mon Dec 19 15:44:04 2016 -0800
+++ 
b/modules/javafx.graphics/src/main/java/javafx/animation/AnimationTimer.java
Tue Dec 20 10:22:18 2016 -0800
@@ -91,11 +91,11 @@
 public abstract void handle(long now);

 /**
- * Starts the {@code AnimationTimers}. Once it is started, the
- * {@link #handle(long)} method of this {@code AnimationTimers} will be
+ * Starts the {@code AnimationTimer}. Once it is started, the
+ * {@link #handle(long)} method of this {@code AnimationTimer} will be
  * called in every frame.
  *
- * The {@code AnimationTimers} can be stopped by calling {@link #stop()}.
+ * The {@code AnimationTimer} can be stopped by calling {@link #stop()}.
  */
 public void start() {
 if (!active) {
@@ -107,7 +107,7 @@
 }

 /**
- * Stops the {@code AnimationTimers}. It can be activated again by calling
+ * Stops the {@code AnimationTimer}. It can be activated again by calling
  * {@link #start()}.
  */
 public void stop() {

[9] Review request for JDK-8146920: [hidpi] Multi-Monitor issue with HiDpi scaling and undecorated stages

[Jim Graham]

JBS: https://bugs.openjdk.java.net/browse/JDK-8146920
webrev: http://cr.openjdk.java.net/~flar/JDK-8146920/webrev.03/

Details are in the JBS comments.

I'm tagging Kevin here on the review, but any engineers with more than a passing familiarity with Glass should review 
the changes as it represents a significant shift in how we manage window bounds, screens, and bookkeeping for the 
platform coordinate scales on Windows platforms.


Having said that, the code change is not huge, it's more that the notifications and actions are now tied into different 
WM_ event streams...


...jim

[9] Review request for 8171393: Integrate precision fixes for Marlin

[Jim Graham]

Laurent sent these webrevs to Kevin and I earlier to evaluate for JDK9.  We're testing them and planning to integrate 
them so I thought I should send out the official review request, though it is really Laurent's work and he'll be the 
eventual author listed on the changeset:


JBS: https://bugs.openjdk.java.net/browse/JDK-8171393
webrev: http://cr.openjdk.java.net/~lbourges/marlinFX-D/marlinFX-Double-raw.1/

webrev to compare against the float code to see how the double variant differs:
http://cr.openjdk.java.net/~lbourges/marlinFX-D/marlinFX-Double-cmp.1/

...jim

Re: [9] Review request: JDK-8170030 Code in Marlin-based rasterizers may have an off-by-1 bug

[Jim Graham]

Hi Laurent,

Normally we'd isolate fixes for different bugs even if they are just 
preparatory formatting changes on comments.

Some comments on the formatting changes:

line 35 - you should also upper-case the E

line 148 - I don't understand why this "float" was a formatting danger, but the one on line 328 isn't?  Not a big issue, 
but curious since I think the edges array does contain floats in it, doesn't it?


line 328 - (ints) => [ints] to match?  (Also would match line 148)


Comments on the bug fix:

line 1401 - why does maxY want to be bounds+1?
  - later this is used to calculate buckets_maxY
  - do we want an extra Y bucket for some reason?
  - and why only in case of spmaxY was clipped?
  - a comment about that would help
Other than that one odd case of using "+1" there, that whole block of code is 
just:
spmaxY = min(edgeMaxY, _boundsMaxY)
which is much easier to read.
Perhaps we need an extra bucket only when there were clipped edges below the 
clip?

line 1456 - I can understand why we lose the +1, but not the min() with pmaxY?


NoAA comments are identical, though line numbers may be different...

...jim

On 12/9/16 8:46 AM, Laurent Bourgès wrote:

Please review this simple fix for MarlinFX:

JBS: https://bugs.openjdk.java.net/browse/JDK-8170030
webrev: http://cr.openjdk.java.net/~lbourges/marlinFX/marlinFX-8170030.0/

PS: I will provide asap a Marlin2D patch incorporating changes made in
MarlinFX (including this one)

Cheers,
Laurent

Re: [9,8u] Review request: JDK-8088857 Menu slow to respond after resizing a window multiple times with animation running

[Jim Graham]

The fix applied cleanly to 8u-dev and fixed the same pair of bugs so this is 
now also a backport request.

8u-specific webrev: 
http://cr.openjdk.java.net/~flar/JDK-8088857/webrev.8u.rt.00/

...jim

On 12/7/16 8:06 AM, Jim Graham wrote:

JBS: https://bugs.openjdk.java.net/browse/JDK-8088857
webrev: http://cr.openjdk.java.net/~flar/JDK-8088857/webrev.rt.00/

Fix is as we discussed.

I'll investigate applying it to 8u-dev soon...

...jim

[9] Review request: JDK-8088857 Menu slow to respond after resizing a window multiple times with animation running

[Jim Graham]

JBS: https://bugs.openjdk.java.net/browse/JDK-8088857
webrev: http://cr.openjdk.java.net/~flar/JDK-8088857/webrev.rt.00/

Fix is as we discussed.

I'll investigate applying it to 8u-dev soon...

...jim

Re: Follow-on bugs

[Jim Graham]

Before we come to any conclusions on performance, though, we might want to run this on something like a Microsoft 
Surface that uses a mobile processor (m3 in the low end) that might not perform as well with doubles.  Does anyone have 
a low-end Surface they could try these patches on and see how they affect performance?


...jim

On 11/30/16 2:12 PM, Laurent Bourgès wrote:

Jim,

As announced yesterday night, I made a new 'Double' (alias D) pipeline for
marlinFX with 2 webrevs:
- cmp that compares the D pipeline vs the float pipeline:
http://cr.openjdk.java.net/~lbourges/marlinFX-D/marlinFX-Double-cmp/
- raw that makes no comparison to be easily applicable as a patch:
http://cr.openjdk.java.net/~lbourges/marlinFX-D/marlinFX-Double-raw/

As explained, I duplicated the complete pipeline (including
MarlinRasterizer) so both can be used for comparisons.
To enable the D pipeline, just add -Dprism.marlin.double=true else the
float pipeline will be used !

I left the conv.sh script that makes the 90% of the class conversions.


It is very preliminary as I doubt we will keep two pipelines (for
maintenance reasons) and here my test results:
- DemoFX (stars / sierpinski) / GuiMark performance are so close that I can
not really see any real difference on my linux with intel 7-4800 ; maybe
other cpu may have more impact with double vs float but the floating-point
computations are representing a minor part of the shape rendering (dasher,
stroker, curve interpolation in Renderer)

Quality:
- Dasher issue with large shapes: all issues are fixed with marlinFX-D
(rect, circle)
- TestNonAARasterization: the errors seem are more important (poly, quad,
cubic) so there may be either a bug in the test (Path2D comparisons) or an
important issue in the D pipeline as also polygons are affected

I wanted to present you this work to get your early feedback and mention
the issues with TestNonAARasterization that will require some work to
understand clearly what's happening !

Cheers,
Laurent

As I wanted to use double precision for long, I ported main marlinFX

classes using a tricky sed scripts... fixed the 'D' pipeline and it seems
already working well.

The 2 mentioned bugs are then fixed + the performance ob DemoFX / GUIMark
(webview) seems the same as the hot spot is in scanline processing +
coverage / mask processing, not in the dasher or stroker parts.

This first prototype is promising and it only took me few hours...

However, subdiving large curves is still an interesting option to preserve
quality / accuracy even with double precision as mentioned in my previous
email.

Re: Follow-on bugs

[Jim Graham]

If this eliminates the regressions in TestNonAA, then I'm for reworking this as 
an in-place patch for MarlinFX.

We can't use the existing Dasher bug for this because Marlin isn't the default renderer, but we can mention the 
DMarlinFX bug as a workaround for the Dasher bug...


...jim

On 11/30/16 2:41 PM, Laurent Bourgès wrote:

Jim,

I found the bug in RendererNoAA caused by a 'bad' regexp match:
0x7fff was modified to 0x7Dff !

so there is no more regression in TestNonAARasterization !

Laurent


FYI, here is the diff of the correction (over the previous big patches):

< +++
new/modules/javafx.graphics/src/main/java/com/sun/marlin/DRenderer.java
2016-11-30 22:48:51.710420442 +0100
---

+++

new/modules/javafx.graphics/src/main/java/com/sun/marlin/DRenderer.java
2016-11-30 23:35:42.879588649 +0100
2276,2277c2276,2277
< +private static final int ALL_BUT_LSB = 0xfffe;
< +private static final int ERR_STEP_MAX = 0x7Dff; // = 2^31 - 1
---

+private static final int ALL_BUT_LSB = 0xFFFe;
+private static final int ERR_STEP_MAX = 0x7FFF; // = 2^31 - 1

2675,2676c2675,2676
< +// = fixed_floor(x1_fixed + 0x7Dff)
< +// and error   = fixed_fract(x1_fixed + 0x7Dff)
---

+// = fixed_floor(x1_fixed + 0x7FFF)
+// and error   = fixed_fract(x1_fixed + 0x7FFF)

2681c2681
< + + 0x7DffL;
---

+ + 0x7FFFL;


< +++
new/modules/javafx.graphics/src/main/java/com/sun/marlin/DRendererNoAA.java
2016-11-30 22:48:52.662420517 +0100
---

+++

new/modules/javafx.graphics/src/main/java/com/sun/marlin/DRendererNoAA.java
2016-11-30 23:35:43.835587936 +0100
4147,4148c4147,4148
< +private static final int ALL_BUT_LSB = 0xfffe;
< +private static final int ERR_STEP_MAX = 0x7Dff; // = 2^31 - 1
---

+private static final int ALL_BUT_LSB = 0xFFFe;
+private static final int ERR_STEP_MAX = 0x7FFF; // = 2^31 - 1

4537,4538c4537,4538
< +// = fixed_floor(x1_fixed + 0x7Dff)
< +// and error   = fixed_fract(x1_fixed + 0x7Dff)
---

+// = fixed_floor(x1_fixed + 0x7FFF)
+// and error   = fixed_fract(x1_fixed + 0x7FFF)

4543c4543
< + + 0x7DffL;
---

+ + 0x7FFFL;




2016-11-30 23:12 GMT+01:00 Laurent Bourgès :


Jim,

As announced yesterday night, I made a new 'Double' (alias D) pipeline for
marlinFX with 2 webrevs:
- cmp that compares the D pipeline vs the float pipeline:
http://cr.openjdk.java.net/~lbourges/marlinFX-D/marlinFX-Double-cmp/
- raw that makes no comparison to be easily applicable as a patch:
http://cr.openjdk.java.net/~lbourges/marlinFX-D/marlinFX-Double-raw/

As explained, I duplicated the complete pipeline (including
MarlinRasterizer) so both can be used for comparisons.
To enable the D pipeline, just add -Dprism.marlin.double=true else the
float pipeline will be used !

I left the conv.sh script that makes the 90% of the class conversions.


It is very preliminary as I doubt we will keep two pipelines (for
maintenance reasons) and here my test results:
- DemoFX (stars / sierpinski) / GuiMark performance are so close that I
can not really see any real difference on my linux with intel 7-4800 ;
maybe other cpu may have more impact with double vs float but the
floating-point computations are representing a minor part of the shape
rendering (dasher, stroker, curve interpolation in Renderer)

Quality:
- Dasher issue with large shapes: all issues are fixed with marlinFX-D
(rect, circle)
- TestNonAARasterization: the errors seem are more important (poly, quad,
cubic) so there may be either a bug in the test (Path2D comparisons) or an
important issue in the D pipeline as also polygons are affected

I wanted to present you this work to get your early feedback and mention
the issues with TestNonAARasterization that will require some work to
understand clearly what's happening !

Cheers,
Laurent

As I wanted to use double precision for long, I ported main marlinFX

classes using a tricky sed scripts... fixed the 'D' pipeline and it seems
already working well.

The 2 mentioned bugs are then fixed + the performance ob DemoFX / GUIMark
(webview) seems the same as the hot spot is in scanline processing +
coverage / mask processing, not in the dasher or stroker parts.

This first prototype is promising and it only took me few hours...

However, subdiving large curves is still an interesting option to
preserve quality / accuracy even with double precision as mentioned in my
previous email.

Re: Follow-on bugs

[Jim Graham]

One thing that might cause a problem is that the script modified ERR_STEP_MAX because it had 7f, but it was 0x7f, not 
1.7f.  Some other constants might be affected as well...


...jim

On 11/30/16 2:12 PM, Laurent Bourgès wrote:

- TestNonAARasterization: the errors seem are more important (poly, quad, 
cubic) so there may be either a bug in the
test (Path2D comparisons) or an important issue in the D pipeline as also 
polygons are affected

I wanted to present you this work to get your early feedback and mention the 
issues with TestNonAARasterization that
will require some work to understand clearly what's happening !

Re: Follow-on bugs

[Jim Graham]

Hi Laurent,

I think you've looked into the issues of flatness and how inflection points affect it about as much as I have at this 
point.  I'm not sure that sub-dividing at min/max values helps filling, but a way to subdivide at inflection points 
might improve the flattening algorithm.  It's worth a try.


I believe the original AFD was intended to be used in the inner loop to render each pixel rather than as a flattening 
metric the way we are using it.  Another concept might be to have the scanline converter that currently only deals with 
lines support quads and cubics as well so we don't have to flatten them (though subdividing to avoid extreme rates of 
change in the step size might still be called for).  It would complicate the code that increments the cur_x on each 
sample line, but it would reduce the number of segments we'd have to store.  Also, we might want to do subdivision at 
min/max of Y values in that case so that the Y values are monotonic.


With respect to float/double, I have another bug somewhere where we have a large inaccuracy for a very large circle that 
intersects the viewing area along only a very tiny portion.  The errors get especially bad with dashing because we 
iterate each dash using an incrementally relative sub-divide rather than returning always to the original curve to 
sub-divide from t1-to-t2.  I'll look it up and send you a pointer so you can see how Marlin does with those paths.  I 
believe that I briefly modified the Pisces Dasher to simply use doubles and the problem went away, but I didn't do any 
performance analysis and the newly accurate dashes no longer matched the still-float-based fills so more work was 
needed.  Modern processors tend to deal with double precision natively and so other than storage considerations double 
calculations are often as fast or sometimes faster than float (because of not needing to be cast back to 32-bit float 
when the FPU always produces 64-bit double answers)...


...jim

On 11/28/16 1:58 PM, Laurent Bourgès wrote:

Jim,

There is one thing I think you should look at, though, and I wasn't sure if

I should file a bug.  If you've downloaded the Rasterization verifier from
the old JBS bug for non-AA rendering, try running it with the 4quads mode
(-quad argument I think).  It looks like it averages slightly more failures
on Marlin than jpisces or npisces.  (3 bad pixels per shape rather than 2 -
or 4 vs. 3 - I forget the exact numbers).  This may simply be a difference
in the DEC_INC_BND settings and an easy fix.  File a bug if you find
something to fix...




I made tests related to nonAA quality and the related quad decimation
thresholds:
The quad decrement threshold seem a bit too high => lowering to 0.5
subpixel leads to 2.07 error:

OpenPisces:
bad paths (78759/10 == 78,76%, 177046 bad pixels (avg = 2,25), 6524
warnings (avg = 0,08)

MarlinFX:
bad paths (90332/10 == 90,33%, 288202 bad pixels (avg = 3,19), 6702
warnings (avg = 0,07)

MarlinFX with DEC_BND=0.5:
bad paths (77342/10 == 77,34%, 164702 bad pixels (avg = 2,13), 6649
warnings (avg = 0,09)

So the trivial fix consist in lowering the threshold.

HOWEVER, it led me having a look again at the algorithmic approach:
The (Sun) paper (Lien 87) describes this approach (AFD) to draw curves
pixels by pixels so the original thresholds were defined to 1 / 0.5 pixels
and it indicates that this algorithm generates at least 1 point per pixel
... so is not related to any error bound nor really the good approach to
minimize the number of segments when the curve is "flat".

Moreover, this test performs path fills (no stroke) so I am asking several
questions about the algorithmic approach:
- AFD thresholds are in fact related to delta X/Y or speed and are ensuring
small displacements (subpixel) unrelated to any error (ROC ?) between the
segment and the curve => it will generate lots more segments than needed
except when the curve has cups or inflexions ...
- subdividing curves may be a more appropriate approach (agg, QT...) as
there are several flatness tests (intermediate point distance like in
ShapeSpanIterator.c) to ensure the curve is under-control ie within the
tolerance
- in Stroker, the curve is subdivided at cups, inflexion points, roots to
have monotonic cuves but in the case of path fills, the curve is directly
processed by the Renderer: maybe we should first subdivide the curves at
those extrema and then use the current approach to improve the affinity at
these special points.

Finally the curve segments are generated with the floating-point maths so
the pixel accuracy is also strongly related to rounding points on the grid
(at pixel centers), so this implies to have small tolerance to compensate
rounding issues.

Jim, do you have advices on:
- how to improve both curve accuracy but also minimize the number of
generated segments ? which algorithm ?
- which flatness test has your recommendations ?
- do you know an adaptive AFD variant 

Re: [9] Code Review Request For 8088179: [Quantum] White flashing when opening a stage with dark background

[Jim Graham]

Not as trivial as the Gradient cases, true.

+1 on the fix if we're happy with that limitation...

...jim

On 11/10/16 3:34 PM, Chien Yang wrote:

Yes, we decide to punt on ImagePattern as there isn't a trivial way to find a 
reasonable background color for it.

- Chien

On 11/10/16, 10:22 AM, Jim Graham wrote:

I guess we'll punt on ImagePattern?

Other than that the fix looks fine...

...jim

On 11/9/16 5:35 PM, Chien Yang wrote:

Hi Jim and Kevin,

Please review the proposed fix:

JIRA: https://bugs.openjdk.java.net/browse/JDK-8088179
Webrev: http://cr.openjdk.java.net/~ckyang/JDK-8088179/webrev.00/

Thanks,
- Chien

Re: Fwd: Re: Marlin-Renderer and JavaFX

[Jim Graham]

Another thought on multi-threaded scan-line rasterization would be to pre-scan the node tree and pre-rasterize all 
shapes into masks before we run through and render them to the destination.  Again, that would be outside the scope of 
9, but it would be a change only to the rendering process (and dealing with the caching of alpha masks) that would 
leverage your ability to create multiple Marlin contexts without any changes to MarlinFX...


...jim

On 11/10/16 5:13 AM, Laurent Bourgès wrote:

Jim,


I would think an effort to parallelize a single shape rasterization would 
be much simpler in scope.  Still outside
the current JDK 9 timeline, but definitely something that could help in 
future releases.  I believe that once we put
the edges into the internal structures we could parallelize the 
rasterization of individual scanlines and maybe
break a tall shape up into N horizontal bands for N threads.  Other 
thoughts would be a thread to generate the
crossings and N threads to populate the alphas...?


Multithreaded rendering is a complex task that should be discussed in another 
thread as it is totally out of my current
scope.

My 2 cents: there is two approach:
- render shapes in parallel (but it requires the JavaFX pipeline to be 
parallelized = tricky as caching mask, gpu need
single thread communication ...)
- render shape with several threads (as you proposed) but the scanline 
algorithm requires the previous scanline
information like previous edges (so it becomes tricky to make it efficient) and 
this will only speed up large shapes
(point clouds will remain slow !)

Anyway the major issue concerns latency ie how to spread the workload on 
several threads when the scene contains shapes
with various sizes or complexity (edge count): the latency corresponds to the 
slowest thread to complete its rendering task.
=> adaptive algorithm to make a sort of load balancing (work stealing approach) 
or small tasks (average same cpu cost).

PS: I have some experience on parallelization, but please let's postpone this 
discussion or let's do it in another thread.

Laurent

Re: [9] Code Review Request For 8088179: [Quantum] White flashing when opening a stage with dark background

[Jim Graham]

I guess we'll punt on ImagePattern?

Other than that the fix looks fine...

...jim

On 11/9/16 5:35 PM, Chien Yang wrote:

Hi Jim and Kevin,

Please review the proposed fix:

JIRA: https://bugs.openjdk.java.net/browse/JDK-8088179
Webrev: http://cr.openjdk.java.net/~ckyang/JDK-8088179/webrev.00/

Thanks,
- Chien

Re: Fwd: Re: Marlin-Renderer and JavaFX

[Jim Graham]

Hi Laurent,

Great job on creating a very obvious minimal impact!  That should help 
streamline it through the approval process.

Is MarlinProperties.isEnabled still used?  (Searching patch file - no, I don't 
think so)

Kevin and I had a long back and forth over the packaging of the Reentrant files and came up with 
"com.sun.util.reentrant" as we'd like to avoid "com.sun" as a terminal package.  We should eventually move a few other 
common utilities into that new tree as well, but we can start with these classes.


I still want to take a pass through the changes to the Marlin files themselves, but above is my review of the glue code 
and with that Kevin can pursue the internal processes.


Before I pursue too far on reading the Marlin source changes, I'd like to hear back about how this version of Marlin(FX) 
differs from the OpenJDK version of Marlin(2D) - were they based on different versions of your Marlin project?  Also, 
you mentioned waiting for the jigsaw build patch integration.  I'm guessing you meant the build changes that happened 
yesterday, so we can proceed now?


With respect to the changes to the native part of the SW pipeline that would trim the rendering - please continue to 
investigate them, but we should pursue those as a separate bug fix so as not to complicate this particular change.  It 
sounds like you've adapted the new code in the SWContext wrapper class to not require that native change yet, so let's 
go with that for the first phase...


...jim

On 11/7/16 2:55 PM, Laurent Bourgès wrote:

Jim,

Here is the new patch:
http://cr.openjdk.java.net/~lbourges/marlinFX/marlinFX-s02-ofx9.3/

Changes:
- cleanup wrt OpenJDK9 (Unsafe is OK but I switch to the standard Cleaner)
- modified PrismSettings as recommended and renamed all Marlin properties
to use the prefix 'prism.marlin'
- SWContext: added Marlin SW support: I had to tweak the processed range[x;
pix_to[ instead of [pix_from; pix_to[ as the PiscesRenderer does not handle
properly the bbox so it processes many extra pixels on the left side (empty
coverage) = to be discussed as it can be fixed in the native prism-sw code
... See my comment in the code:

+@Override+public void
setAndClearRelativeAlphas(final int[] alphaDeltas, final int pix_y,+
   final int pix_from,
final int pix_to)+{+// use x instead of
pix_from as it cause artefacts:+// note: it would be
more efficient to skip all those empty pixels [x to pix_from[+
   // but the native implementation must be fixed too.+//
  pr.emitAndClearAlphaRow(alpha_map, alphaDeltas, pix_y, pix_from,
pix_to, rowNum);+pr.emitAndClearAlphaRow(alpha_map,
alphaDeltas, pix_y, x, pix_to, rowNum);

I successfully built OpenJFX9 (gradle) and tested OK with OpenJDK9 b143
(xpatch.args) with Ensemble8 + DemoFX.

Maybe it is time to start the review for the FX enhancement ?
https://bugs.openjdk.java.net/browse/JDK-8169270

Cheers,
Laurent


2016-11-04 19:55 GMT+01:00 Jim Graham <james.gra...@oracle.com>:


On 11/4/2016 11:33 AM, Laurent Bourgès wrote:


For SWContext, I figured out that only openpisces.* classes were used
directly via imports (hardcoded) so I left it unchanged. So you propose
to generalize use of marlin or native pisces ?



I didn't notice that, I was just searching on the use of
"doNativePisces".  We can look at that separately, and Kevin would know how
frequently we end up in the SW pipeline these days...

Re: Fwd: Re: Marlin-Renderer and JavaFX

[Jim Graham]

Going forward perhaps we should refer to the version of Marlin in Java2D as 
Marlin2D?

Then Marlin is your original plug-in version that is still being worked on.
Marlin2D is what you integrated into OpenJDK/Java2D.
MarlinFX is what you are planning for FX.

That's just for conversational purposes, I'm not proposing that we formalize those into formal project names, but this 
will keep discussions smaller?


On 11/2/16 2:54 PM, Laurent Bourgès wrote:

PS: MarlinFX is based on my (internal) Marlin-renderer 0.7.5 (a bit
different than OpenJDK9's Marlin as it is compatible with jdk 1.8) so I
will synchronize both branches to provide soon MarlinFX9 patches closer to
OpenJDK9's Marlin renderer if we are going to work on its integration.


If I read this correctly, you have outstanding changes you've made to Marlin that you plan to propose for Marlin2D, but 
they haven't been proposed yet?


And you based MarlinFX on the newer version of Marlin with those outstanding 
changes?

Is your plan to propose a patch for Marlin2D soon?

For now I'd like to keep the 2 versions of Marlin in the JDK in sync other than differences required for Java2D's 
rendering pipelines vs. the FX ones.  That will simplify at least the intial proposal to integrate this if we can say it 
is "99% the same as the one in Java2D other than differences required by the pipelines" which should quiet any fears 
that might get in the way.


Did I read that correctly?  Does that sound reasonable?

...jim

Re: Fwd: Re: Marlin-Renderer and JavaFX

[Jim Graham]

On 10/21/16 9:51 AM, Laurent Bourgès wrote:

Jim, do you think possible to unify Marlin and MarlinFX for openjdk9 ? The
main difference relies in different Shape/PathConsumer classes and Fx uses
the AlphaConsumer + different initialization.
Did you have a look at the diffs ?


One of the big hurdles here is that Java2DMarlin is in a very large module in the JDK and we currently run independently 
of it.  We'd have to break Marlin out into a separate module shared by both and I think that is outside our scope right 
now...?


...jim

Re: Fwd: Re: Marlin-Renderer and JavaFX

[Jim Graham]

On 10/20/16 5:34 AM, Kevin Rushforth wrote:

For now the OpenPiscesRasterizer class uses a static Renderer (single
instance) so it is single-threaded.

In MarlinFX I could prepare the multi-threading support by using 1
RendererContext per thread (ThreadLocal) as I did in Marlin for java2d.

However it seems a complex task to enable parallelization in the javafx
pipeline but I could help there also...



Enabling parallel rasterization seems like a good follow-on task, but is out of 
scope for the short term given the
limited amount of time. Also, the only way that MarlinFX even has a chance of 
getting approved for in JDK 9 is for the
default OpenPisces path to be unaltered.


Also, such a parallelization of the javafx pipelines would be a fairly large 
task.

I would think an effort to parallelize a single shape rasterization would be much simpler in scope.  Still outside the 
current JDK 9 timeline, but definitely something that could help in future releases.  I believe that once we put the 
edges into the internal structures we could parallelize the rasterization of individual scanlines and maybe break a tall 
shape up into N horizontal bands for N threads.  Other thoughts would be a thread to generate the crossings and N 
threads to populate the alphas...?


...jim

Re: Review request for 8169294:

[Jim Graham]

On 11/7/2016 1:14 PM, Laurent Bourgès wrote:

The only difference in Path2D.java I noted is that the Java2D version has
an EXPAND_MIN which is 10, but you re-use INIT_SIZE, which is 20, here for
the same purpose.


You're right; I think I didn't want to add an extra constant but if you
prefer being more consistent, I can prepare another webrev.


In looking at it again, I don't think it matters since it would only 
ever be triggered if they created a path that intentionally had fewer 
than the default number of entries, so it makes more sense to always 
grow by at least the INIT_SIZE rather than a smaller number.


I think I like the new policy better than the one in Java2D...

...jim

Re: Review request for 8169294:

[Jim Graham]

I'd like to see Kevin review the test as I'm not the best expert on our 
JUnit framework.


It looks like it is mostly just going to emit some printouts about 
performance (using echo() and log()) and verify that we don't get any 
ArrayBounds related exceptions (or worse, OOME)?


The only difference in Path2D.java I noted is that the Java2D version 
has an EXPAND_MIN which is 10, but you re-use INIT_SIZE, which is 20, 
here for the same purpose.


I think that's fine, but wanted to point it out so you could comment...

...jim

On 11/6/2016 1:01 PM, Laurent Bourgès wrote:

Hi,

Please review this Path2D fix improving its storage growth algorithm as
done in java2d:
JBS: https://bugs.openjdk.java.net/browse/JDK-8169294
Webrev: http://cr.openjdk.java.net/~lbourges/fx/path2D/

PS: I converted the former jtreg test to JUnit test for OpenJFX. I hope
it is correct, as I was not able to run this test within gradle (but it
works in netbeans)

Best regards,
Laurent

Re: Fwd: Re: Marlin-Renderer and JavaFX

[Jim Graham]

FX enhancement: https://bugs.openjdk.java.net/browse/JDK-8169270

...jim

On 11/4/2016 8:21 AM, Kevin Rushforth wrote:

Hi Jim,

Thanks for the summary. We should proceed as you outlined. Can you file
a new RFE (Enhancement) to integrate Marlin into JavaFX as an optional
rasterizer (disabled by default) so we have a clean JBS issue to use for
the JDK 9 feature extension request, rather than using JDK-8092373? You
can assign the RFE to Laurent. It can be linked to the Java2D/Marlin JEP
and to JDK-8092373.

Laurent: Since there are multiple reasons for submitting the Path2D fix
as a separate bug fix, please file a new bug for this, linking it to the
equivalent Java2D bug and also to the new RFE that Jim will file.

Thanks.

-- Kevin


Jim Graham wrote:

There are basically 2 isolated changes to the existing code base and
then a set of added source files.

The first change is to use Marlin if the appropriate property is
specified, and those changes are very localized and easy to verify
that they won't hurt anything.

The second change is to modify the growth pattern of Path2D.  While
these changes are live in AWT already and have already been code
reviewed, it would probably be better to submit them as a separate FX
fix if they are only performance oriented and not strictly required
for Marlin to function.  That way we compartmentalize anything that
could possibly result in a regression into a separate bugid so we
don't have to pull everything if someone complains that the new growth
pattern is having negative consequences for their app.  I doubt that
will happen, but it is simple enough to break them into 2 separate
fixes so it couldn't hurt to do that.

After that, this just boils down to adding a bunch of code that has
already been vetted elsewhere and a small and simple change to use it
only optionally and conditionally, which is a very low risk fix.

That would take this change from "no obvious drawbacks" to "obviously
no drawbacks" (or, more precisely, one "obviously no drawbacks" fix
and one related "no obvious drawbacks" fix)...

...jim

On 11/2/2016 2:54 PM, Laurent Bourgès wrote:

Jim,

Here is an updated patch for MarlinFX:
http://cr.openjdk.java.net/~lbourges/marlinFX/marlinFX-s02-ofx9/

I made big improvements: MarlinFX is now "feature-complete":
- Added MarlinAlphaConsumer & MarlinRenderer interfaces to define new
methods on AlphaConsumer and common methods between AA & noAA renderers
- Renderer: fixed cubic dec/inc lengths (like openpisces) + use
optimized
copyAlphas from MarlinAlphaConsumer (with block flag optimization
derived
from former MarlinCache)
- RendererNoAA: optimized Renderer for non-antialiasing (tested OK)
- Dasher & Stroker: backported changes from openpisces (small dash
phase &
refined cubic / quad mitters)
- Added MarlinPrismUtils & MarlinRasterizer classes to wrap the Marlin
renderer as a JavaFX ShapeRasterizer and implement the
MarlinAlphaConsumer
efficiently (mimics former MarlinCache ie support the block flag
optimization); MarlinPrismUtils performs properly NaN / Infinity
coordinates filtering and use the same pipeline stages (delta / invDelta
transformers for Dasher/Stroker) like in the MarlinRenderingEngine
- Thread safety: MarlinRasterizer completely thread-safe (for future
multi-threaded rendering) using ReentrantContext...
- Modified (OpenJFX) ShapeUtil to use the MarlinRasterizer instead of
the
OpenPiscesRasterizer class (use -Dsun.javafx.marlin=true to enable
Marlin-FX)
- Fixed Path2D growing algorithm (like java2d)

So MarlinFX is 13K LOC (few unused classes could be removed soon) and
only
few lines are added in ShapeUtil to switch MarlinFX ON:

if (PrismSettings.doNativePisces) {
shapeRasterizer = new NativePiscesRasterizer();
} else {






*// TODO: move that new setting into PrismSettings:
// Enable Marlin-FX by setting -Dsun.javafx.marlin=trueif
(MarlinProperties.isMarlinEnabled()) {
System.out.println("Marlin-FX[" + Version.getVersion() + "]
enabled.");shapeRasterizer = new
MarlinRasterizer();} else {*
shapeRasterizer =
new OpenPiscesRasterizer();

*}*}

So the OpenPisces classes are totally left unchanged and MarlinFX is
just
added as another rasterizer and is enabled with the following settings:
-Dsun.javafx.marlin=true and -Dprism.nativepisces=false

Of course, we could adapt these few lines to satisfy your requirements
(system properties ...); please tell me what you prefer.

I tested this new release with DemoFX, Guimark HTML5, Ensemble8 and
everything is working fine.


Does it look acceptable as a low risk RFE ?

Finally what do you prefer for OpenJDK9 integration ?
- as a new javafx rasterizer (like the provided marlinFX patch)
- or as a javafx wrapper using OpenJDK9's marlin renderer (java2d) ?
I wonder if it would be bette

Re: Fwd: Re: Marlin-Renderer and JavaFX

[Jim Graham]

On 11/4/2016 11:33 AM, Laurent Bourgès wrote:

For SWContext, I figured out that only openpisces.* classes were used
directly via imports (hardcoded) so I left it unchanged. So you propose
to generalize use of marlin or native pisces ?


I didn't notice that, I was just searching on the use of 
"doNativePisces".  We can look at that separately, and Kevin would know 
how frequently we end up in the SW pipeline these days...


...jim

Re: Fwd: Re: Marlin-Renderer and JavaFX

[Jim Graham]

There are basically 2 isolated changes to the existing code base and 
then a set of added source files.


The first change is to use Marlin if the appropriate property is 
specified, and those changes are very localized and easy to verify that 
they won't hurt anything.


The second change is to modify the growth pattern of Path2D.  While 
these changes are live in AWT already and have already been code 
reviewed, it would probably be better to submit them as a separate FX 
fix if they are only performance oriented and not strictly required for 
Marlin to function.  That way we compartmentalize anything that could 
possibly result in a regression into a separate bugid so we don't have 
to pull everything if someone complains that the new growth pattern is 
having negative consequences for their app.  I doubt that will happen, 
but it is simple enough to break them into 2 separate fixes so it 
couldn't hurt to do that.


After that, this just boils down to adding a bunch of code that has 
already been vetted elsewhere and a small and simple change to use it 
only optionally and conditionally, which is a very low risk fix.


That would take this change from "no obvious drawbacks" to "obviously no 
drawbacks" (or, more precisely, one "obviously no drawbacks" fix and one 
related "no obvious drawbacks" fix)...


...jim

On 11/2/2016 2:54 PM, Laurent Bourgès wrote:

Jim,

Here is an updated patch for MarlinFX:
http://cr.openjdk.java.net/~lbourges/marlinFX/marlinFX-s02-ofx9/

I made big improvements: MarlinFX is now "feature-complete":
- Added MarlinAlphaConsumer & MarlinRenderer interfaces to define new
methods on AlphaConsumer and common methods between AA & noAA renderers
- Renderer: fixed cubic dec/inc lengths (like openpisces) + use optimized
copyAlphas from MarlinAlphaConsumer (with block flag optimization derived
from former MarlinCache)
- RendererNoAA: optimized Renderer for non-antialiasing (tested OK)
- Dasher & Stroker: backported changes from openpisces (small dash phase &
refined cubic / quad mitters)
- Added MarlinPrismUtils & MarlinRasterizer classes to wrap the Marlin
renderer as a JavaFX ShapeRasterizer and implement the MarlinAlphaConsumer
efficiently (mimics former MarlinCache ie support the block flag
optimization); MarlinPrismUtils performs properly NaN / Infinity
coordinates filtering and use the same pipeline stages (delta / invDelta
transformers for Dasher/Stroker) like in the MarlinRenderingEngine
- Thread safety: MarlinRasterizer completely thread-safe (for future
multi-threaded rendering) using ReentrantContext...
- Modified (OpenJFX) ShapeUtil to use the MarlinRasterizer instead of the
OpenPiscesRasterizer class (use -Dsun.javafx.marlin=true to enable
Marlin-FX)
- Fixed Path2D growing algorithm (like java2d)

So MarlinFX is 13K LOC (few unused classes could be removed soon) and only
few lines are added in ShapeUtil to switch MarlinFX ON:

if (PrismSettings.doNativePisces) {
shapeRasterizer = new NativePiscesRasterizer();
} else {






*// TODO: move that new setting into PrismSettings:
// Enable Marlin-FX by setting -Dsun.javafx.marlin=trueif
(MarlinProperties.isMarlinEnabled()) {
System.out.println("Marlin-FX[" + Version.getVersion() + "]
enabled.");shapeRasterizer = new
MarlinRasterizer();} else {*shapeRasterizer =
new OpenPiscesRasterizer();

*}*}

So the OpenPisces classes are totally left unchanged and MarlinFX is just
added as another rasterizer and is enabled with the following settings:
-Dsun.javafx.marlin=true and -Dprism.nativepisces=false

Of course, we could adapt these few lines to satisfy your requirements
(system properties ...); please tell me what you prefer.

I tested this new release with DemoFX, Guimark HTML5, Ensemble8 and
everything is working fine.


Does it look acceptable as a low risk RFE ?

Finally what do you prefer for OpenJDK9 integration ?
- as a new javafx rasterizer (like the provided marlinFX patch)
- or as a javafx wrapper using OpenJDK9's marlin renderer (java2d) ?
I wonder if it would be better to create another JavaFX ShapeRasterizer
that wraps OpenJDK9 Marlin (java2d) to minimize the code duplication but it
will add some complexity in the marlin renderer (itself) to introduce the
AlphaConsumer interface...


I can make separate patches for all these changes concerning jfx Path2D or
Marlin changes for OpenJDK9 first, then MarlinFX.

PS: MarlinFX is based on my (internal) Marlin-renderer 0.7.5 (a bit
different than OpenJDK9's Marlin as it is compatible with jdk 1.8) so I
will synchronize both branches to provide soon MarlinFX9 patches closer to
OpenJDK9's Marlin renderer if we are going to work on its integration.

PS2: I will also work on another MarlinFX variant not using Unsafe but only
plain arrays to evaluate the performance loss (small) that could simplify
the integration with Jigsaw ...

So I made a 

Re: Fwd: Re: Marlin-Renderer and JavaFX

[Jim Graham]

We currently control the configuration logging with prism.verbose, and 
we already have a place in PrismSettings where we dump out the 
configuration including which rasterizer we're using (native or 
java-based) if verbose is set.  We should probably consolidate this 
along with the TODO to move the setting to PrismSettings.


With 3 choices it might make sense to use a "prism.rasterizerorder" type 
of property like we use for the general pipeline (es2 d3d sw), but for 
the sake of keeping the changes fairly localized, I'd recommend:


prism.marlinrasterizer=true|false
(overrides prism.nativepisces=true|false)

(It might seem natural to use "useMarlin" or something like that, but 
most of the other prism.* settings in that file are of the form 
"mechanism=true/false" to mean "useMechanism"...)


If set to true, use Marlin regardless of "nativepisces" value.  If not 
set to true, consult nativepisces or use the default as the code already 
does.  Also, change the print statement in the "verbose" block in that 
same method to indicate we are using Marlin, printing out one of the 
following:


"Using java-based Pisces rasterizer"
"Using native-based Pisces rasterizer"
"Using Marlin rasterizer"

I also noticed that the SWContext also chooses a rasterizer for its use 
using a copy of the decision logic in ShapeUtils.  We should probably 
add a ShapeUtil.getRasterizer() method to return the one that was 
determined by its algorithm rather than repeat the selection algorithm 
in SWContext.


In MarlinProperties we should probably use prism.marlin.* as the prefix 
for these settings to keep them isolated from the java2d settings...?


...jim

On 11/2/2016 5:27 PM, Sergey Bylokhov wrote:

Probably the logging should be enabled only if we pass the upper case
"True" to "-Dsun.javafx.marlin=True". I do not know is it correct to
write to the system output unconditionally, can this affect application?
And instead of "sun." can we use something different like "jdk." I guess
the same question can be applied to jdk?

On 03.11.16 0:54, Laurent Bourgès wrote:

Jim,

Here is an updated patch for MarlinFX:
http://cr.openjdk.java.net/~lbourges/marlinFX/marlinFX-s02-ofx9/

I made big improvements: MarlinFX is now "feature-complete":
- Added MarlinAlphaConsumer & MarlinRenderer interfaces to define new
methods on AlphaConsumer and common methods between AA & noAA renderers
- Renderer: fixed cubic dec/inc lengths (like openpisces) + use optimized
copyAlphas from MarlinAlphaConsumer (with block flag optimization derived
from former MarlinCache)
- RendererNoAA: optimized Renderer for non-antialiasing (tested OK)
- Dasher & Stroker: backported changes from openpisces (small dash
phase &
refined cubic / quad mitters)
- Added MarlinPrismUtils & MarlinRasterizer classes to wrap the Marlin
renderer as a JavaFX ShapeRasterizer and implement the
MarlinAlphaConsumer
efficiently (mimics former MarlinCache ie support the block flag
optimization); MarlinPrismUtils performs properly NaN / Infinity
coordinates filtering and use the same pipeline stages (delta / invDelta
transformers for Dasher/Stroker) like in the MarlinRenderingEngine
- Thread safety: MarlinRasterizer completely thread-safe (for future
multi-threaded rendering) using ReentrantContext...
- Modified (OpenJFX) ShapeUtil to use the MarlinRasterizer instead of the
OpenPiscesRasterizer class (use -Dsun.javafx.marlin=true to enable
Marlin-FX)
- Fixed Path2D growing algorithm (like java2d)

So MarlinFX is 13K LOC (few unused classes could be removed soon) and
only
few lines are added in ShapeUtil to switch MarlinFX ON:

if (PrismSettings.doNativePisces) {
shapeRasterizer = new NativePiscesRasterizer();
} else {






*// TODO: move that new setting into PrismSettings:
// Enable Marlin-FX by setting -Dsun.javafx.marlin=trueif
(MarlinProperties.isMarlinEnabled()) {
System.out.println("Marlin-FX[" + Version.getVersion() + "]
enabled.");shapeRasterizer = new
MarlinRasterizer();} else {*shapeRasterizer =
new OpenPiscesRasterizer();

*}*}

So the OpenPisces classes are totally left unchanged and MarlinFX is just
added as another rasterizer and is enabled with the following settings:
-Dsun.javafx.marlin=true and -Dprism.nativepisces=false

Of course, we could adapt these few lines to satisfy your requirements
(system properties ...); please tell me what you prefer.

I tested this new release with DemoFX, Guimark HTML5, Ensemble8 and
everything is working fine.


Does it look acceptable as a low risk RFE ?

Finally what do you prefer for OpenJDK9 integration ?
- as a new javafx rasterizer (like the provided marlinFX patch)
- or as a javafx wrapper using OpenJDK9's marlin renderer (java2d) ?
I wonder if it would be better to create another JavaFX ShapeRasterizer
that wraps OpenJDK9 Marlin (java2d) to minimize the code duplication
but it
will add some 

[9] Review request for 8166856: OS X: dual screen rendering issue

[Jim Graham]

JBS: https://bugs.openjdk.java.net/browse/JDK-8166856
webrev: http://cr.openjdk.java.net/~flar/JDK-8166856/webrev.00/

Some analysis in the last 2 comments of the JBS, but the main fix is to 
not send bounds changes down to native when no change has happened from 
FX code.  The main culprit here was that they would be sent down when 
only a scale change had occurred.  Since scale changes occur during 
dragging across screens this could confuse the native code as to whether 
a window is in its transitional state due to a drag operation or due to 
a programmatic change (triggered by echoing back the bounds when the 
scale change occurs) - each of which might have their own heuristics 
relating to whether or not the scale is that of the first or second 
screens.  Now we only send the bounds down when they have changed.


I also break out the scale notifications on MacOS to only be sent when 
the OS determines them to be appropriate rather than when we think they 
have changed.  This keeps us in better sync with the native scale.


This should not affect 8 (but I'll test to be sure) because we didn't 
send scales up the chain to FX like we do on 9 so the only reason 8 
would have to try to send the bounds down is when they have actually 
changed from FX code.  It wasn't until scaling was added to the 
notification path in 9 that we started sending down false NOP bounds 
changes...


...jim

[9] Review request: 8166382 [hidpi] Ubuntu 16.04: invalid coordinates when using glass robot

[Jim Graham]

JBS: https://bugs.openjdk.java.net/browse/JDK-8166382
webrev: http://cr.openjdk.java.net/~flar/JDK-8166382/webrev.00/

Pretty self-explanatory fix...

...jim

Re: [9] Review request for 8090176: Pisces software renderer shows incomplete border images in particular situation

[Jim Graham]

There is now an updated fix that addresses a potential performance loss when simple 1:1 blits were accidentally getting 
redirected through interpolate-per-pixel code by the first fix...


http://cr.openjdk.java.net/~flar/JDK-8090176/webrev.9u.02/

...jim

On 9/2/16 12:24 PM, Jim Graham wrote:

JBS: https://bugs.openjdk.java.net/browse/JDK-8090176
webrev for 9u: http://cr.openjdk.java.net/~flar/JDK-8090176/webrev.9u.01/

The webrev is prepared for 8u, but I will be holding off on submitting that for 
backport review until the fix has baked
in the 9u-dev repo for a couple of weeks...

...jim

[9] Review request for 8090176: Pisces software renderer shows incomplete border images in particular situation

[Jim Graham]

JBS: https://bugs.openjdk.java.net/browse/JDK-8090176
webrev for 9u: http://cr.openjdk.java.net/~flar/JDK-8090176/webrev.9u.01/

The webrev is prepared for 8u, but I will be holding off on submitting that for backport review until the fix has baked 
in the 9u-dev repo for a couple of weeks...


...jim

Re: Memory leaks on Linux with hardware renderer

[Jim Graham]

It looks like we create a dummy drawable for the context and install it when we are done with the frame.  This appeared 
in rev bbb8d2772b37, but it looks like that revision involved removing the unnecessary RenderingContext class, so we may 
have been doing something similar via the RenderingContext class and the code was merely moved into SwapChain when the 
other class was deleted.  Further investigation would be needed, but my mercurial rev/diffing skills are pretty 
primitive.  Anyone care to dig a little on this and figure out if there is a reason for us to unset the drawable at the 
end of a frame?


In either case, if we had 2 windows open, and that will happen if there is a simple popup on a choice box or a menu item 
I think unfortunately, then we'd still have the problem so whether or not it happens with a single window with no popup 
items in it, it still looks like we could potentially trigger this in common cases anyway.  We should track the fix to 
GLX and make sure we document required patches if there is a fix...


...jim

On 8/11/16 6:13 AM, Itai wrote:

I'm sorry to see the issue could still not be reproduced on any OpenJFX
team members.

Meanwhile, I have noticed a user on reddit (JavaFX sub-reddit) had the same
issue:
https://www.reddit.com/r/JavaFX/comments/4nr2ln/memory_leak_when_calling_imageviewsettranslatex/
.
However, they have managed to profile it (VisualVM has a bug making it
nearly impossible to CPU profile JavaFX programs), and found out a lot of
time is taken by `com.sun.prism.es2.X11GLContext.makeCurrent`.

Taking this lead, I have found this:
http://www.gamedev.net/topic/679705-glxmakecurrent-slowly-leaks-memory/

Sadly I don't know enough about OpenGL to understand most of it, but it
seems to me like it's the same issue, so possibly it's not a Java issue at
all. However, maybe it can be avoided? In this linked post it is mentioned
that the leak only happens when having two windows, but in JavaFX this
always happens, so maybe there is a redundant call to `makeCurrent`?

Hope this helps to find the source of the problem, and if it's indeed
outside of Java/JavaFX scope - report it to the relevant project.

On Thu, Jul 21, 2016 at 6:12 PM, Kevin Rushforth :


I'll add a comment to that effect (although our incident triage team is
good about spotting such duplicates).

-- Kevin


Itai wrote:


Thank you. Having gotten no reply, and seeing the bug report was closed
and with not means of commenting in the bug report system, I have since
(about an hour ago) filed a more detailed report (JI-9042009). I believe
they could be safely merged, but the second one does contain some more
info.
On Thu, Jul 21, 2016 at 2:27 AM, Kevin Rushforth <
kevin.rushfo...@oracle.com > wrote:

JI-9041860 has now been transferred to the JDK project as:

https://bugs.openjdk.java.net/browse/JDK-8161911

Our support engineer was not able to reproduce the problem, so
closed it as such. Based on the additional information you
provided, I have reopened the bug and will ask someone on our team
with a physical Linux setup to try to reproduce it.

To answer your question, we are not aware of any such leaks.

-- Kevin



Itai wrote:

I'm experiencing multiple memory leaks with JavaFX on Linux,
to the point
where I'm not sure which bug to report, as it seems like a
systematic
issue.

The memory leak seems to be completely absent when using the
software
renderer (-Dprism.order=sw), and does not seem to happen on
Windows
(presumably not on Mac either, although I have no Mac to test
it).

Test cases include:

1. Use ProgressIndicator with progress set to Indeterminate -
with default
(HW) renderer memory consumption quickly rises, climbing to
8GB and more if
not killed. With software renderer memory usage is reasonable.
2. Using Scene Builder - after a few minutes with Scene
Builder it quickly
gobbles up all system memory - again, problem seems to go away
if using
software renderer. This test is less repeatable, as some
actions seem more
detrimental than others.
3. Using Transitions on nodes (See attached code "Demo.java".
I have filed
a bug report about this issue, JI-9041860). Running with
default renderer
the simple program reaches 3GB within 30 

Re: [9] Review request for 8159892: Ubuntu 16.04: invalid rendering of FX app stage in case of scaling

[Jim Graham]

New webrev: http://cr.openjdk.java.net/~flar/JDK-8159892/webrev.01/

This one makes the presence of the new functions being used optional.  Details 
are in JBS.

With respect to my comment below about the consistency of declarations and definitions, there didn't seem to be much 
consistency in the order of things as it exists so I moved a couple of the functions purely for my own OCD enjoyment, 
but didn't sweat the details too much...


...jim

On 8/9/16 6:41 PM, Jim Graham wrote:

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

There are a number of bugs filed on this, or very similar issues as well - all 
related to broken DPI scaling on GTK3.

It looks like there is a simple way of disabling automatic scaling for GTK3 so 
we can do our own (which has been working
fairly well so far for GTK2 and is preferable to the integer-operation scaling 
we'd get automatically via the
integer-dominated APIs of GDK/GTK).

Questions:

- Is this the best bug ID to use for this fix?  I used it as it was the first 
one that I looked at and it had screen
shots of the problem that immediately pointed me to the underlying problem.  
Other bugids are mentioned in the bug
report as being "possible dups" and most of them should be a duplicate, but so 
far I've only tested the test case shown
in the screen shots of 8159892...

- The new function is specifically in 3.10 of the GTK 3 libraries.  Should I 
add additional protections in case a system
has an earlier 3.XX version of GTK3?

- Minor note - I discovered after publishing the webrev that the function is 
defined, declared, loaded and stubbed in
inconsistent locations in the list of functions - not really an issue, but I'll 
try to normalize where it appears in all
of the various function lists if I have to recut the webrev...

...jim

[9] Review request for 8159892: Ubuntu 16.04: invalid rendering of FX app stage in case of scaling

[Jim Graham]

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

There are a number of bugs filed on this, or very similar issues as well - all 
related to broken DPI scaling on GTK3.

It looks like there is a simple way of disabling automatic scaling for GTK3 so we can do our own (which has been working 
fairly well so far for GTK2 and is preferable to the integer-operation scaling we'd get automatically via the 
integer-dominated APIs of GDK/GTK).


Questions:

- Is this the best bug ID to use for this fix?  I used it as it was the first one that I looked at and it had screen 
shots of the problem that immediately pointed me to the underlying problem.  Other bugids are mentioned in the bug 
report as being "possible dups" and most of them should be a duplicate, but so far I've only tested the test case shown 
in the screen shots of 8159892...


- The new function is specifically in 3.10 of the GTK 3 libraries.  Should I add additional protections in case a system 
has an earlier 3.XX version of GTK3?


- Minor note - I discovered after publishing the webrev that the function is defined, declared, loaded and stubbed in 
inconsistent locations in the list of functions - not really an issue, but I'll try to normalize where it appears in all 
of the various function lists if I have to recut the webrev...


...jim

Re: CFV: New OpenJFX Committer: Andrey Rusakov

[Jim Graham]

Vote: yes

...jim

On 7/23/16 7:42 AM, Kevin Rushforth wrote:

I hereby nominate Andrey Rusakov [1] to OpenJFX Committer.

Andrey is a member of JavaFX SQE team at Oracle working on test development, 
who has contributed 19 changesets [5] to
OpenJFX, at least 8 of which are significant.

Votes are due by August 6, 2016.

Only current OpenJFX Committers [2] are eligible to vote on this nomination. 
Votes must be cast in the open by replying
to this mailing list.

For Lazy Consensus voting instructions, see [3]. Nomination to a project 
Committer is described in [4].

Thanks,

-- Kevin

[1] http://openjdk.java.net/census#arusakov

[2] http://openjdk.java.net/census#openjfx

[3] http://openjdk.java.net/bylaws#lazy-consensus

[4] http://openjdk.java.net/projects#project-committer

[5] List of changesets:
http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/a1e73dd4613e
http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/7cce7ed57d89
http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/34e657660c5c
http://hg.openjdk.java.net/openjfx/9-dev/tests/rev/0e78b9e0f50e
http://hg.openjdk.java.net/openjfx/9-dev/tests/rev/d356db6612c7
http://hg.openjdk.java.net/openjfx/9-dev/tests/rev/1be0ce8c6667
http://hg.openjdk.java.net/openjfx/9-dev/tests/rev/adbed89b4b79
http://hg.openjdk.java.net/openjfx/9-dev/tests/rev/30fbc7690076
http://hg.openjdk.java.net/openjfx/9-dev/tests/rev/b6bd30234e94
http://hg.openjdk.java.net/openjfx/9-dev/tests/rev/9b0f70918dc4
http://hg.openjdk.java.net/openjfx/9-dev/tests/rev/38c836fde6ab
http://hg.openjdk.java.net/openjfx/9-dev/tests/rev/bee64c78137a
http://hg.openjdk.java.net/openjfx/9-dev/tests/rev/1e1aea3def0f
http://hg.openjdk.java.net/openjfx/9-dev/tests/rev/c20f5594ed04
http://hg.openjdk.java.net/openjfx/9-dev/tests/rev/f34e58cb89ff
http://hg.openjdk.java.net/openjfx/9-dev/tests/rev/558c27a5d330
http://hg.openjdk.java.net/openjfx/9-dev/tests/rev/8ab04be9195b
http://hg.openjdk.java.net/openjfx/9-dev/tests/rev/f741badededf
http://hg.openjdk.java.net/openjfx/9-dev/tests/rev/2e6d6f449f08

Re: Exception on render thread: "NGTriangleMesh: buildGeometry failed"

[Jim Graham]

Would a better exception help?  OOME?

...jim

On 6/28/16 9:18 AM, Kevin Rushforth wrote:

Glad to hear it.

-- Kevin


GUEVEL, Emanuel wrote:

It worked.

The problem is not present anymore after switching to a 64-bit JVM.

Many thanks for this. :)

Best regards,
Emanuel


-Message d'origine-
De : Kevin Rushforth [mailto:kevin.rushfo...@oracle.com] Envoyé : mardi 28 juin 
2016 16:33
À : GUEVEL, Emanuel
Cc : openjfx-dev@openjdk.java.net
Objet : Re: Exception on render thread: "NGTriangleMesh: buildGeometry failed"

This suggests that you are running out of native memory. You might consider 
using a 64-bit JVM.

-- Kevin


GUEVEL, Emanuel wrote:


Hello,

I am using JavaFX 8 for a 3D visualization application.
I display a set of TriangleMesh and under some circumstances, an exception is 
thrown on the JavaFX rendering thread
("QuantumRenderer") in NGTriangleMesh.validate().
The message is "NGTriangleMesh: buildGeometry failed".

I know this exception is thrown when the mesh geometry data are incorrect, but 
in this case the behavior is
inconsistent : the same data may or may not be considered invalid.
It seems the problem occurs more often when more memory is allocated to the JVM 
(via -Xms and -Xms command arguments).

I use JDK8u92 32 bits on Windows.

Does anyone encounter the same problem ?
Do you have a solution ?

Thanks in advance.

Best regards,
Emanuel

This message contains information that may be privileged or confidential and is 
the property of the Capgemini Group.
It is intended only for the person to whom it is addressed. If you are not the 
intended recipient, you are not
authorized to read, print, retain, copy, disseminate, distribute, or use this 
message or any part thereof. If you
receive this message in error, please notify the sender immediately and delete 
all copies of this message.

[8u, 9] Review request: 8159860: JavaFX Path drawing appears to leak native memory

[Jim Graham]

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

...jim

Re: [8u, 9] Review request: 8145516: Scene content shows too large on Retina display, when a regular screen attached

[Jim Graham]

Updating the subject line to request 8u backport approval.  The fix applies 
cleanly to 8u and fixes the bug there as well...

...jim

On 6/17/16 2:09 PM, Jim Graham wrote:

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

Information on the fix is in the bug report comments.

I need to repair my 8u-dev repos to check, but I believe this can be ported 
directly to an 8u release...

...jim

[9] Review request for 8157600: Failed to launch hello.HelloSanity due to libglass.so: undefined symbol

[Jim Graham]

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

...jim

Re: [9] Code Review Request For 8157350: Encapsulate impl_ methods in Shapes related classes

[Jim Graham]

Sounds good...

...jim

On 5/20/16 3:56 PM, Kevin Rushforth wrote:



Jim Graham wrote:



On 5/20/16 3:33 PM, Kevin Rushforth wrote:

This is needed for those cases where we need to encapsulate a method in the 
base Shape class that used to be public and
overridden in the subclasses, not all of which are in the same package. It may 
seem like overkill, but we need a way to
associate the the Shape instance of a particular subtype with the helper 
instance of the correct subtype. Each class in
the hierarchy calls the specific XHelper.initHelper(this) method so that it 
can store back an instance of the right
helper in the base class. A package-private method wouldn't work given that 
some shapes (e.g., Text) are in different
packages.


Right, but (taking Arc as an example) Arc makes a specific reference to 
ArcHelper which turns around and hands a
specific instance to its own instance field to a method that stores the value 
in the shapeHelper field.  How is that
any different from just putting shapeHelper = ArcHelper.instance without 2 
method calls and an accessor in the way?


But the shapeHelper field is in the base Shape class not in the Arc class. If 
we wanted a different pattern for classes
in the same package as the base class from classes in a different package then 
I guess I can see how this is solvable by
making the ArcHelper.getInstance() method public and having the Arc() 
constructors call the package-scope
setHelper(ShapeHelper) method in Shape, but as soon as Chien move the stored 
helper instance up to Node (which is the
next step) it would stop working.


Also, what if someone creates a custom sub-class of Shape?  (Not sure if that 
is supported or possible, but it is a
public class with a public constructor so I don't think it is impossible.)


Since we don't have public API for many of the things they would need even 
today, an application isn't able to do that.
They couldn't really do it anyway before this change, since impl_getPeer() and 
several other methods aren't
implementable by an application (NGNode is not publicly exported for example).



Good reminder about the implicit "public Shape()" constructor. Chien already 
had to add an explicit public no-arg
constructor in two classes. We really shouldn't rely on the implicit 
constructor in our public classes, since it makes
it easy to make such a mistake.


It would be good to have a tool and/or automated test that warns about this.  
Another reason is that the implicit
constructor has no javadocs associated with it...


Indeed. I wonder if doclint will help (we are in the process of making our docs 
doclint clean).

-- Kevin




...jim

Re: [9] Code Review Request For 8157350: Encapsulate impl_ methods in Shapes related classes

[Jim Graham]

It feels like there is extra indirection for the code that sets the helpers.  Is there a reason it isn't as simple as 
"this.shapeHelper = FooHelper.instance"?  Or, even a package-private Shape(ShapeHelper) constructor on Shape?  (*)


Also, many of the helper classes have argument names that were obviously cut and pasted from a different shape class. 
The names don't functionally matter, but seeing the ArcHelper manipulate variables called "quadThisOrThat" is a little 
jarring...


...jim

(*) Note that since Shape currently has a public empty constructor that is added implicitly by the fact that it includes 
no explicit constructors you have to make sure to make an explicit one if you ever add an explicit constructor...


On 5/20/16 2:11 PM, Chien Yang wrote:

Hi Kevin,

Please review the proposed fix as we have discussed.

JIRA: https://bugs.openjdk.java.net/browse/JDK-8157350
Webrev: http://cr.openjdk.java.net/~ckyang/JDK-8157350/webrev.00/

Thanks,
- Chien

Re: JDK9 build 119 fails on Ubuntu Gnome 16.04

[Jim Graham]

This should be fixed in the next build (fix is already pushed to our 9-dev FX repo).  It was caused by getting a "0" 
from the GDK APIs that specify the scaling factor.  We now protect against that uninitialized value...


...jim

On 5/20/16 8:30 AM, Kevin Rushforth wrote:

Jim can comment further, but this might be related to the recent Hi-DPI support 
for Linux that went into build 119.

Btw, the updated WebKit will be in build 120.

-- Kevin


Thomas Kruse wrote:

Hi,

I tested the latest build 119 of JDK9. I was curious to try out the
WebKit update, but even a very simple JavaFX class threw the following
exception. I tried to force GTK2, since I suspected GTK3 support as the
culprit, but this did not help.

java -Djdk.gtk.version=2 -Djdk.gtk.verbose=true -cp target/classes
sample.Basic
Loading GTK libraries version 2
trying GTK library set libgtk-x11-2.0.so.0, libgdk-x11-2.0.so.0,
libgdk_pixbuf-2.0.so
failed to load libgdk_pixbuf-2.0.so
trying GTK library set libgtk-x11-2.0.so, libgdk-x11-2.0.so,
libgdk_pixbuf-2.0.so
failed to load libgtk-x11-2.0.so
trying GTK library set libgtk-3.so.0, libgdk-3.so.0, libgdk_pixbuf-2.0.so.0
using GTK library set libgtk-3.so.0, libgdk-3.so.0, libgdk_pixbuf-2.0.so.0
Exception in thread "JavaFX Application Thread"
java.lang.IllegalArgumentException: Both width and height must be >= 0
at
javafx.geometry.Rectangle2D.(javafx.graphics@9-ea/Rectangle2D.java:104)
at javafx.stage.Screen.nativeToScreen(javafx.graphics@9-ea/Screen.java:152)
at
javafx.stage.Screen.updateConfiguration(javafx.graphics@9-ea/Screen.java:88)
at javafx.stage.Screen.checkDirty(javafx.graphics@9-ea/Screen.java:82)
at javafx.stage.Screen.getPrimary(javafx.graphics@9-ea/Screen.java:185)
at
com.sun.javafx.tk.quantum.QuantumToolkit.initSceneGraph(javafx.graphics@9-ea/QuantumToolkit.java:298)
at
com.sun.javafx.tk.quantum.QuantumToolkit.runToolkit(javafx.graphics@9-ea/QuantumToolkit.java:340)
at
com.sun.javafx.tk.quantum.QuantumToolkit.lambda$startup$10(javafx.graphics@9-ea/QuantumToolkit.java:257)
at
com.sun.glass.ui.Application.lambda$run$1(javafx.graphics@9-ea/Application.java:155)
at
com.sun.glass.ui.gtk.GtkApplication._runLoop(javafx.graphics@9-ea/Native
Method)
at
com.sun.glass.ui.gtk.GtkApplication.lambda$runLoop$8(javafx.graphics@9-ea/GtkApplication.java:195)
at java.lang.Thread.run(java.base@9-ea/Thread.java:804)

Re: [9] Review request for 8157213: HiDPI support for Linux creates unnecessary dependency

[Jim Graham]

New updated webrevs with the new default value at:

http://cr.openjdk.java.net/~flar/JDK-8157213/webrev-01/

I also found a way to remove the schema from the Gentoo LiveCD environment and 
tested that way as well...

...jim

On 5/18/16 1:28 PM, Erik De Rijcke wrote:

Yes that seems to fix it.

On Wed, May 18, 2016 at 9:55 PM, Jim Graham <james.gra...@oracle.com> wrote:


Ah, this is probably me returning a -1 as an uint.  If you change the
"defval" used (line 167) in the call to query the property from -1 to 0
does it work as intended?

...jim


On 5/18/16 11:59 AM, Erik De Rijcke wrote:


I tested the patch.

Without the dependency I get a an enormous stage (I explicitly set it to
be 100x100). Debugging shows me the screen is
initialized with these values:

Screen:
ptr:0
adapter:0
depth:24
x:0
y:0
width:0
height:0
platformX:0
platformY:0
platformWidth:1680
platformHeight:946
visibleX:0
visibleY:0
visibleWidth:0
visibleHeight:0
platformScaleX:4.2949673E9
platformScaleY:4.2949673E9
outputScaleX:4.2949673E9
outputScaleY:4.2949673E9
resolutionX:0
resolutionY:0


I assume the scaling factor is not what it should be?

With the dependency installed the stage looks fine.

On Wed, May 18, 2016 at 5:52 AM, Jim Graham <james.gra...@oracle.com
<mailto:james.gra...@oracle.com>> wrote:

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

Details of what was fixed are listed in the bug report.  This will
hopefully fix all of the dependencies that Erik
ran into in his Gentoo environment...

...jim

Re: [9] Review request for 8157213: HiDPI support for Linux creates unnecessary dependency

[Jim Graham]

Ah, this is probably me returning a -1 as an uint.  If you change the "defval" used (line 167) in the call to query the 
property from -1 to 0 does it work as intended?


...jim

On 5/18/16 11:59 AM, Erik De Rijcke wrote:

I tested the patch.

Without the dependency I get a an enormous stage (I explicitly set it to be 
100x100). Debugging shows me the screen is
initialized with these values:

Screen:
ptr:0
adapter:0
depth:24
x:0
y:0
width:0
height:0
platformX:0
platformY:0
platformWidth:1680
platformHeight:946
visibleX:0
visibleY:0
visibleWidth:0
visibleHeight:0
platformScaleX:4.2949673E9
platformScaleY:4.2949673E9
outputScaleX:4.2949673E9
outputScaleY:4.2949673E9
resolutionX:0
resolutionY:0


I assume the scaling factor is not what it should be?

With the dependency installed the stage looks fine.

On Wed, May 18, 2016 at 5:52 AM, Jim Graham <james.gra...@oracle.com 
<mailto:james.gra...@oracle.com>> wrote:

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

Details of what was fixed are listed in the bug report.  This will 
hopefully fix all of the dependencies that Erik
ran into in his Gentoo environment...

...jim

[9] Review request for 8157213: HiDPI support for Linux creates unnecessary dependency

[Jim Graham]

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

Details of what was fixed are listed in the bug report.  This will hopefully fix all of the dependencies that Erik ran 
into in his Gentoo environment...


...jim

Re: Support for GTK 2 & 3 now in JFX

[Jim Graham]

Hi Erik,

I tried testing our build under a Gentoo LiveCD.  Even the minimal 
environment of the LiveCD has the org.gnome.desktop.interface schema 
defined so I'm not sure how minimal your environment was.  I did 
discover that, although the schema was there, it had a value of "0" for 
the scaling-factor which caused us to create bogus logical screen sizes 
that probably caused the exception you saw, so simply finding that 
schema and key is not enough for us to trust the value.  I'll add some 
protection for bad data from the key, but I'm curious to find out how 
minimal your install was and whether we should simply state our 
dependency, or if your configuration is likely to be common enough that 
we should protect against it in our code...?


Also, what does:

% gsettings get org.gnome.desktop.interface scaling-factor

return for your system?  If you set it to "1", does FX come up fine?

...jim

On 05/16/2016 01:01 PM, Jim Graham wrote:

These may both be related to the HiDPI fix instead.  I added a usage of
g_settings in that fix that went in on Friday.  It looks like I'll have
to check for the schema existing before I access it.

Can you file a bug report?

(On a side note, the call that grabs the schema does not have a "schema
not known" return value - if you ask for a schema that doesn't exist
they abort your application.  Seems kind of drastic, but the bug reports
that request that they simply return null were met with push back
because the developers couldn't imagine why anyone would want such a
thing (huh?) and later an attempt to return NULL on unrecognized schemas
and keys had to be backed out because of the huge disaster the NULL
values caused (and somehow ABORT'ing on behalf of the application is
better in that case?).  Unfortunately I don't see much sanity in those
APIs or any support for a case like this of "I'd like to get that value,
but if you haven't heard of it, that's fine as I have a backup plan".
The only way around this will be to enumerate all schemas and all keys
and see if the ones we want are found in the list - a rather extreme
workaround for bad error handling behavior in their APIs...)

 ...jim

On 05/16/2016 05:15 AM, Erik De Rijcke wrote:

I ran into several issues, however I'm not sure they are related to gtk3

first of all it seems there is a dependency on:
gsettings-desktop-schemas.
I'm not sure how desirable this is (I did not have it installed,
installing
it fixed the error but I can image people not wanting to install it?)

Second, and this seems unrelated to gtk(3?), the screen initialization
fails (test was done using gentoo linux on virtualbox with windows 7 as
host):

Exception in thread "JavaFX Application Thread"
java.lang.IllegalArgumentException: Both width and height must be >= 0

at javafx.geometry.Rectangle2D.(Rectangle2D.java:104)
at javafx.stage.Screen.nativeToScreen(Screen.java:152)
at javafx.stage.Screen.updateConfiguration(Screen.java:88)
at javafx.stage.Screen.checkDirty(Screen.java:82)
at javafx.stage.Screen.getPrimary(Screen.java:185)
at
com.sun.javafx.tk.quantum.QuantumToolkit.initSceneGraph(QuantumToolkit.java:298)

at
com.sun.javafx.tk.quantum.QuantumToolkit.runToolkit(QuantumToolkit.java:340)

at
com.sun.javafx.tk.quantum.QuantumToolkit.lambda$startup$10(QuantumToolkit.java:257)

at com.sun.glass.ui.Application.lambda$run$1(Application.java:155)
at com.sun.glass.ui.gtk.GtkApplication._runLoop(Native Method)
at
com.sun.glass.ui.gtk.GtkApplication.lambda$runLoop$7(GtkApplication.java:194)

at java.lang.Thread.run(Thread.java:804)


Nearly all values seem to be -2147483648


I used openjdk9 109 as per instructions of the wiki, and the latest
javafx
dev.


On Mon, May 9, 2016 at 8:56 PM, Jim Graham <james.gra...@oracle.com>
wrote:


Should we integrate that into prism.verbose output?

 ...jim


On 5/9/16 6:18 AM, David Hill wrote:



I added a new feature Friday and would like some help testing it.

This new feature (8087516: Conditional support for GTK 3 on Linux)
allows
us to use either GTK v2 or 3 with JavaFX.

The default has not changed - we will use gtk 2 by preference.

The help I need is for anyone doing testing on Linux with the current
tree - like todays sanity testing. Adding

-Djdk.gtk.verbose=true

should output the version detected and used. I would appreciate a paste
of that along with the OS version.

-Djdk.gtk.version=3

toggles the preferred version to GTK 3. Testing using that toggled
would
also be appreciated, along with a note to me
with the OS version.

thanks!

Re: Support for GTK 2 & 3 now in JFX

[Jim Graham]

These may both be related to the HiDPI fix instead.  I added a usage of 
g_settings in that fix that went in on Friday.  It looks like I'll have 
to check for the schema existing before I access it.


Can you file a bug report?

(On a side note, the call that grabs the schema does not have a "schema 
not known" return value - if you ask for a schema that doesn't exist 
they abort your application.  Seems kind of drastic, but the bug reports 
that request that they simply return null were met with push back 
because the developers couldn't imagine why anyone would want such a 
thing (huh?) and later an attempt to return NULL on unrecognized schemas 
and keys had to be backed out because of the huge disaster the NULL 
values caused (and somehow ABORT'ing on behalf of the application is 
better in that case?).  Unfortunately I don't see much sanity in those 
APIs or any support for a case like this of "I'd like to get that value, 
but if you haven't heard of it, that's fine as I have a backup plan". 
The only way around this will be to enumerate all schemas and all keys 
and see if the ones we want are found in the list - a rather extreme 
workaround for bad error handling behavior in their APIs...)


...jim

On 05/16/2016 05:15 AM, Erik De Rijcke wrote:

I ran into several issues, however I'm not sure they are related to gtk3

first of all it seems there is a dependency on: gsettings-desktop-schemas.
I'm not sure how desirable this is (I did not have it installed, installing
it fixed the error but I can image people not wanting to install it?)

Second, and this seems unrelated to gtk(3?), the screen initialization
fails (test was done using gentoo linux on virtualbox with windows 7 as
host):

Exception in thread "JavaFX Application Thread"
java.lang.IllegalArgumentException: Both width and height must be >= 0

at javafx.geometry.Rectangle2D.(Rectangle2D.java:104)
at javafx.stage.Screen.nativeToScreen(Screen.java:152)
at javafx.stage.Screen.updateConfiguration(Screen.java:88)
at javafx.stage.Screen.checkDirty(Screen.java:82)
at javafx.stage.Screen.getPrimary(Screen.java:185)
at
com.sun.javafx.tk.quantum.QuantumToolkit.initSceneGraph(QuantumToolkit.java:298)
at
com.sun.javafx.tk.quantum.QuantumToolkit.runToolkit(QuantumToolkit.java:340)
at
com.sun.javafx.tk.quantum.QuantumToolkit.lambda$startup$10(QuantumToolkit.java:257)
at com.sun.glass.ui.Application.lambda$run$1(Application.java:155)
at com.sun.glass.ui.gtk.GtkApplication._runLoop(Native Method)
at
com.sun.glass.ui.gtk.GtkApplication.lambda$runLoop$7(GtkApplication.java:194)
at java.lang.Thread.run(Thread.java:804)


Nearly all values seem to be -2147483648


I used openjdk9 109 as per instructions of the wiki, and the latest javafx
dev.


On Mon, May 9, 2016 at 8:56 PM, Jim Graham <james.gra...@oracle.com> wrote:


Should we integrate that into prism.verbose output?

 ...jim


On 5/9/16 6:18 AM, David Hill wrote:



I added a new feature Friday and would like some help testing it.

This new feature (8087516: Conditional support for GTK 3 on Linux) allows
us to use either GTK v2 or 3 with JavaFX.

The default has not changed - we will use gtk 2 by preference.

The help I need is for anyone doing testing on Linux with the current
tree - like todays sanity testing. Adding

-Djdk.gtk.verbose=true

should output the version detected and used. I would appreciate a paste
of that along with the OS version.

-Djdk.gtk.version=3

toggles the preferred version to GTK 3. Testing using that toggled would
also be appreciated, along with a note to me
with the OS version.

thanks!

Re: [9] Review request for 8137050: Support Linux settings for HiDPI scaling

[Jim Graham]

It is implemented in the FX Robot in that webrev...?

...jim

On 5/12/16 3:07 PM, Sergey Bylokhov wrote:

Hi Jim.
Do you plan to implement the new HiDPI API for the Robot class?

On 13.05.16 0:43, Jim Graham wrote:

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

The order of taking scaling parameters in order of higher to lower
priority is:
- -Dglass.gtk.uiScale system property
- Same property in the environment
- GDK_SCALE in the environment (optional in Gnome)
- g_settings (the typical way to set scaling...)

As noted in the bug report, I'll delete the old lines in the Screen
constructor in glass_screen.cpp that are currently commented out if
there are no other changes noted in the review...

I was able to run dev-build-full.sh and got the same errors as I did
without the fix.

I ran SwingNode toys and none of them scaled despite the fact that I was
running on jdk9 and AWT was supposed to support scaling, but they worked
fine in that I could manipulate all of the Swing controls.

...jim

[9] Review request for 8137050: Support Linux settings for HiDPI scaling

[Jim Graham]

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

The order of taking scaling parameters in order of higher to lower priority is:
- -Dglass.gtk.uiScale system property
- Same property in the environment
- GDK_SCALE in the environment (optional in Gnome)
- g_settings (the typical way to set scaling...)

As noted in the bug report, I'll delete the old lines in the Screen constructor in glass_screen.cpp that are currently 
commented out if there are no other changes noted in the review...


I was able to run dev-build-full.sh and got the same errors as I did without 
the fix.

I ran SwingNode toys and none of them scaled despite the fact that I was running on jdk9 and AWT was supposed to support 
scaling, but they worked fine in that I could manipulate all of the Swing controls.


...jim

Re: Support for GTK 2 & 3 now in JFX

[Jim Graham]

Should we integrate that into prism.verbose output?

...jim

On 5/9/16 6:18 AM, David Hill wrote:


I added a new feature Friday and would like some help testing it.

This new feature (8087516: Conditional support for GTK 3 on Linux) allows us to 
use either GTK v2 or 3 with JavaFX.

The default has not changed - we will use gtk 2 by preference.

The help I need is for anyone doing testing on Linux with the current tree - 
like todays sanity testing. Adding

-Djdk.gtk.verbose=true

should output the version detected and used. I would appreciate a paste of that 
along with the OS version.

-Djdk.gtk.version=3

toggles the preferred version to GTK 3. Testing using that toggled would also 
be appreciated, along with a note to me
with the OS version.

thanks!

Re: [graphics] [9] Review request for 8155903: Crash while running imported/w3c/canvas/2d.gradient.interpolate.overlap2.html

[Jim Graham]

That looks good for the case where Imin is zero, but it appears that we could also have overflow as well, with a single 
very tiny Imin the accumulation of estimatedSize with an "int" type could easily overflow and become essentially a 
random number.  Changing the estimatedSize variable to a float should prevent that related issue...


...jim

On 5/9/16 6:16 AM, Arunprasad Rajkumar wrote:

Hello Jim,

Thanks for your suggestions. As of now I taking an easy way to fix the issue, 
New changes are available at
http://cr.openjdk.java.net/~arajkumar/8155903/webrev.02

I couldn't write a reliable test case using public javafx APIs, the behavior is 
intermittent. However I could
consistently produce the issue using our DRT internal test case which is based 
on W3C functional test[1].

[1] 
http://w3c-test.org/2dcontext/fill-and-stroke-styles/2d.gradient.interpolate.overlap2.html

Thanks,
Arun

On 5/5/2016 11:51 PM, Jim Graham wrote:

Hi Arun,

The change you made to the calculateSingleArray method looks like it produces a 
bad array of color stops for the case
where Imin is 0.  You should fall into the calculateMultipleArray method 
instead which should not have any trouble
with zero length intervals.  At that point you don't have to have any code in 
calculateSingleArray that deals with
Imin being zero because that can be part of its calling contract.

That is the quick fix.

The harder fix that would let us maintain speed when there is a zero interval 
would be to teach the code what to do in
that special case. Basically a zero interval means that we have a case where 
approaching the interval point from the
left is interpolating towards colorA, but leaving that point from the right is 
interpolating from colorB, with a
sudden transition between those 2.  In that case, a zero interval shouldn't 
affect the Imin, since the Imin is trying
to determine the size of each interpolated region and we don't interpolate 
across a zero-sized interval.  So, the scan
for Imin would simply ignore zero length intervals.  Later, the code that 
populates the array in the
calculateSingleArray function would know that the zero length interval simply means swap 
in a new "from color" without
any actual interpolation.

Getting that harder fix right would require a lot of testing, so it may be 
better to do the quick fix now to stop the
exceptions and then deal with the optimization as a tweak filed for later...

...jim

On 05/05/2016 01:57 AM, Arunprasad Rajkumar wrote:

Hello Jim,

Please review the below patch.

JIRA: https://bugs.openjdk.java.net/browse/JDK-8155903

Webrev: http://cr.openjdk.java.net/~arajkumar/8155903/webrev.01

Issue: Divide by zero while adding multiple gradient stops at same offset.

Regards,
Arun

[8u, 9] Review request for 8156094: ContextMenu shown at wrong position on Windows10 with Extended Screen

[Jim Graham]

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

webrev for 9-dev: http://cr.openjdk.java.net/~flar/JDK-8156094/9-dev/webrev.00/
webrev for 8u-dev: 
http://cr.openjdk.java.net/~flar/JDK-8156094/8u-dev/webrev.00/

The two fixes ended up being closer than I thought due to an idiosyncrasy of how Windows arranges screens for system-dpi 
apps...


...jim

Re: [9] Code Review Request For 8155762: Encapsulate JavaFX impl_* implementation methods in transform package

[Jim Graham]

TransformUtils.java - can't this class be deleted now since all it 
exists to do is to forward to TransformHelper?


Alternatively, why create TransformHelper in the first place when it is 
90% just a copy of what TransformUtils used to be (ignoring the inner 
class that got moved to Transform) with an accessor interface thrown in 
for good measure?  Just to change the name?  That could be done by using 
a source rename rather than gutting the old file and creating the new 
one from scratch...?  And we don't need both of them, do we?


Transform.java, line 2172 & 2187 - is there a reason to forward to the 
factory rather than just instantiating directly?


Transform.java, line 2177 & 2189 - I suppose the reason that the 
arguments are not declared to be Immutable in the first place is that 
some of these are used from other packages that have no visibility to 
the Immutable.  I was going to say that we should protect against them 
not being Immutables, but I guess these uses are hidden so it doesn't 
really affect anything by doing the cast - but it seems sloppy when 
looking at the code without looking everywhere to see how they are used 
(which I suppose is short-hand for "someone in the future might mess up 
because it's not obvious that these can't deal with any other type").


TransformHelper.java, line 108 & 112 - [formatting] perhaps move the 
m[xyz],tx to the next line for consistency between the two (like 68 & 76)?


The implementation of ImmutableTransform copies a fair bit of internals 
from Affine, but it could just wrap an Affine and save itself all of 
that work.  Arguably, we could make it a super-class of Affine and then 
it would own all of that code and Affine would subclass only for the 
modification methods, but we'd either have to expose it as a public 
class or have an unexplained inaccessible superclass on Affine...? 
(Also, it's not clear if public methods on a non-public superclass of 
Affine would be visible through Affine so Affine would either have to 
have forwarding methods or we'd have to make the super class public).


On the other hand, what would be wrong with having a public BaseAffine 
that was Affine without its modification methods, and having Affine 
subclass it?  It would be effectively Immutable, but using the word 
Immutable in its name might sound like a promise which would be violated 
by its subclass - better to leave that property implied by its lack of 
mutating methods and possibly mentioning the intent in the javadoc...


...jim

On 5/3/2016 11:24 PM, Chien Yang wrote:

Kevin and Jim,

Please review the proposed fix:

Webrev: https://bugs.openjdk.java.net/browse/JDK-8155762
JIRA: http://cr.openjdk.java.net/~ckyang/JDK-8155762/webrev.00/

Thanks,
- Chien

Re: RFR: 8152423: Generated temp files (+JXF...temp) for custom fonts not deleted on exit.

[Jim Graham]

In that case, in order to avoid race conditions (GC could be 
accidentally triggered by this or subsequent shutdown hooks in the 
system), it should be released by calling disposer.dispose() instead of 
directly calling Release (still setting fontFace to null just in case). 
 That will ensure only one release per fontFace.


Also, in the constructor it looks like we'll construct a disposer for a 
null fontFace.  I don't think it hurts anything, and probably shouldn't 
happen in practice, but still seems odd...


...jim

On 05/05/2016 11:40 AM, Phil Race wrote:


On 05/05/2016 11:26 AM, Jim Graham wrote:

Is this true of any other objects managed by DWDisposer?


DWDisposer is only the class used to implement "dispose()" of
a  single DWFontFile that occurs during running/gc.

It doesn't really "manage" anything and I don't see it used
to dispose any other resource.


Would it be better to simply run through the DWDisposer queue on
shutdown and force it to dispose (i.e. release) everything it has?


There isn't a "DWDisposer" queue - it is the shared disposer queue for
all resources across Prism - so far as I can tell anyway.

This clean up on exit of font files is initiated from a shutdown
hook specific to the temporary font files (shared, not DW specific).
To be able to delete the files we need to be sure that Release() was
called before File.delete().
So we either would need the font file shutdown hook to take ownership
of first releasing other Prism resources as well or find a way to kick
off a different
hook specific to DWDisposer and ensure it runs before the deletion hook.
I am not sure there is any way to guarantee that order.

Also if we do an 8ux backport I'd want to do it this simpler way there
even if something different were done for 9.

-phil.



...jim

On 05/05/2016 11:12 AM, Phil Race wrote:

Please review :-
Bug : https://bugs.openjdk.java.net/browse/JDK-8152423
Fix : http://cr.openjdk.java.net/~prr/8152423/
<http://cr.openjdk.java.net/%7Eprr/8152423/>

"Release" was not being called on the DirectWrite font object
so it held a reference to the file, which on Windows prevents
the file from being deleted.

-phil

Re: RFR: 8152423: Generated temp files (+JXF...temp) for custom fonts not deleted on exit.

[Jim Graham]

Is this true of any other objects managed by DWDisposer?  Would it be 
better to simply run through the DWDisposer queue on shutdown and force 
it to dispose (i.e. release) everything it has?


...jim

On 05/05/2016 11:12 AM, Phil Race wrote:

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


"Release" was not being called on the DirectWrite font object
so it held a reference to the file, which on Windows prevents
the file from being deleted.

-phil

Re: Code Review Request For 8155757: Encapsulate impl_ methods in animation, canvas, image, input, layout, paint, and text packages

[Jim Graham]

Maybe we could ask someone in Hotspot if they recognize these bytecode 
patterns and optimize out the helper methods.  If that's the case, then 
it is really just down to a bundled size issue...


...jim

On 5/1/2016 11:55 PM, Chien Yang wrote:

Hi Jim,

Thanks for sharing this information and your thought. I'm not sure is
this saving worth violating the principle of minimizing scope in code. I
guess you did bring up a good point let me think over it and discuss
with Kevin tomorrow.

- Chien

On 4/29/16, 4:04 PM, Jim Graham wrote:

One comment on the implementation.  For the methods used by an
accessor inner class, if you make them private in the outer class then
that inner class will need a hidden accessor method to be added in the
bytecodes.  If you make them package-private then they can access the
method directly.

Basically, an inner class is really "just another class in the
package, but with a special name" and actually have no access to
private methods in their outer classes at all, but javac works around
this by adding a hidden method that has more open access and using that.

An example is Image.getPlatformImage() - you turned it from "public
and impl_" into private.  Why not just leave it
package-private/default access?

For example, compiling this class:

public class InnerPrivateTest {
private void foo() {}
public class InnerClass {
public void bar() { foo(); }
}
}

yields this byte code for InnerPrivateTest.class:

public class InnerPrivateTest {
  public InnerPrivateTest();
Code:
   0: aload_0
   1: invokespecial #2  // Method
java/lang/Object."":()V
   4: return

  private void foo();
Code:
   0: return

  static void access$000(InnerPrivateTest);
Code:
   0: aload_0
   1: invokespecial #1  // Method foo:()V
   4: return
}

and this for the InnerClass:

public class InnerPrivateTest$InnerClass {
  final InnerPrivateTest this$0;

  public InnerPrivateTest$InnerClass(InnerPrivateTest);
Code:
   0: aload_0
   1: aload_1
   2: putfield  #1  // Field
this$0:LInnerPrivateTest;
   5: aload_0
   6: invokespecial #2  // Method
java/lang/Object."":()V
   9: return

  public void bar();
Code:
   0: aload_0
   1: getfield  #1  // Field
this$0:LInnerPrivateTest;
   4: invokestatic  #3  // Method
InnerPrivateTest.access$000:(LInnerPrivateTest;)V
   7: return
}

Changing the access on foo() to default (package private), yields this
byte code:

public class InnerPackageTest {
  public InnerPackageTest();
Code:
   0: aload_0
   1: invokespecial #1  // Method
java/lang/Object."":()V
   4: return

  void foo();
Code:
   0: return
}

public class InnerPackageTest$InnerClass {
  final InnerPackageTest this$0;

  public InnerPackageTest$InnerClass(InnerPackageTest);
Code:
   0: aload_0
   1: aload_1
   2: putfield  #1  // Field
this$0:LInnerPackageTest;
   5: aload_0
   6: invokespecial #2  // Method
java/lang/Object."":()V
   9: return

  public void bar();
Code:
   0: aload_0
   1: getfield  #1  // Field
this$0:LInnerPackageTest;
   4: invokevirtual #3  // Method
InnerPackageTest.foo:()V
   7: return
}

...jim

On 4/29/16 11:50 AM, Chien Yang wrote:

Hi Kevin,

Please review the proposed fix:

JIRA: https://bugs.openjdk.java.net/browse/JDK-8155757
Webrev: http://cr.openjdk.java.net/~ckyang/JDK-8155757/webrev.00/

Thanks,
- Chien

Re: Code Review Request For 8155757: Encapsulate impl_ methods in animation, canvas, image, input, layout, paint, and text packages

[Jim Graham]

One comment on the implementation.  For the methods used by an accessor inner class, if you make them private in the 
outer class then that inner class will need a hidden accessor method to be added in the bytecodes.  If you make them 
package-private then they can access the method directly.


Basically, an inner class is really "just another class in the package, but with a special name" and actually have no 
access to private methods in their outer classes at all, but javac works around this by adding a hidden method that has 
more open access and using that.


An example is Image.getPlatformImage() - you turned it from "public and impl_" into private.  Why not just leave it 
package-private/default access?


For example, compiling this class:

public class InnerPrivateTest {
private void foo() {}
public class InnerClass {
public void bar() { foo(); }
}
}

yields this byte code for InnerPrivateTest.class:

public class InnerPrivateTest {
  public InnerPrivateTest();
Code:
   0: aload_0
   1: invokespecial #2  // Method 
java/lang/Object."":()V
   4: return

  private void foo();
Code:
   0: return

  static void access$000(InnerPrivateTest);
Code:
   0: aload_0
   1: invokespecial #1  // Method foo:()V
   4: return
}

and this for the InnerClass:

public class InnerPrivateTest$InnerClass {
  final InnerPrivateTest this$0;

  public InnerPrivateTest$InnerClass(InnerPrivateTest);
Code:
   0: aload_0
   1: aload_1
   2: putfield  #1  // Field this$0:LInnerPrivateTest;
   5: aload_0
   6: invokespecial #2  // Method 
java/lang/Object."":()V
   9: return

  public void bar();
Code:
   0: aload_0
   1: getfield  #1  // Field this$0:LInnerPrivateTest;
   4: invokestatic  #3  // Method 
InnerPrivateTest.access$000:(LInnerPrivateTest;)V
   7: return
}

Changing the access on foo() to default (package private), yields this byte 
code:

public class InnerPackageTest {
  public InnerPackageTest();
Code:
   0: aload_0
   1: invokespecial #1  // Method 
java/lang/Object."":()V
   4: return

  void foo();
Code:
   0: return
}

public class InnerPackageTest$InnerClass {
  final InnerPackageTest this$0;

  public InnerPackageTest$InnerClass(InnerPackageTest);
Code:
   0: aload_0
   1: aload_1
   2: putfield  #1  // Field this$0:LInnerPackageTest;
   5: aload_0
   6: invokespecial #2  // Method 
java/lang/Object."":()V
   9: return

  public void bar();
Code:
   0: aload_0
   1: getfield  #1  // Field this$0:LInnerPackageTest;
   4: invokevirtual #3  // Method InnerPackageTest.foo:()V
   7: return
}

...jim

On 4/29/16 11:50 AM, Chien Yang wrote:

Hi Kevin,

Please review the proposed fix:

JIRA: https://bugs.openjdk.java.net/browse/JDK-8155757
Webrev: http://cr.openjdk.java.net/~ckyang/JDK-8155757/webrev.00/

Thanks,
- Chien

[9] review request: 8155692: changes to compile under Visual Studio 14.0

[Jim Graham]

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

It's mostly just a build file change to pick up the compilers from the 
new VS140COMNTOOLS location, but it also includes a change to minimize 
the impact of a recent change to throw an error on the deprecation of 
hash_map and hash_set.  I briefly looked at upgrading GlassClipboard.cpp 
to the preferred unordered_map and unordered_set classes, but the hash 
functions weren't compatible in a way that wasn't trivial to sort out. 
We should probably upgrade to the more standard collections classes.  I 
can file a follow-on bug for that if we decide to go with the #define 
IGNORE fix here...


...jim

Re: CFV: New OpenJFX Committer: Guru Hb

[Jim Graham]

Vote: yes

...jim

On 4/28/16 8:16 AM, Kevin Rushforth wrote:

I hereby nominate Guru Hb [1] to OpenJFX Committer.

Guru is a member of JavaFX team at Oracle working on WebKit, who has
contributed 10 changesets [5] to OpenJFX, at least 8 of which are
significant.

Votes are due by May 12, 2016.

Only current OpenJFX Committers [2] are eligible to vote on this
nomination. Votes must be cast in the open by replying to this mailing
list.

For Lazy Consensus voting instructions, see [3]. Nomination to a project
Committer is described in [4].

Thanks,

-- Kevin

[1] http://openjdk.java.net/census#ghb

[2] http://openjdk.java.net/census#openjfx

[3] http://openjdk.java.net/bylaws#lazy-consensus

[4] http://openjdk.java.net/projects#project-committer

[5] List of changesets:
http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/1ec63f261e9f
http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/fb4c37073893
http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/3ccf14ef836f
http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/45efd3d83bf1
http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/08a57f273c76
http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/bd97f8ca31fc
http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/0576d7a6f137
http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/d4f8c9496683
http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/620f5c6b4383
http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/aa51df7e1d41

[9] post-commit review: 8153348: JavaFX API doc examples use nonexistent package "javafx.animation.transition"

[Jim Graham]

https://bugs.openjdk.java.net/browse/JDK-8153348

...jim

Changeset: e1688df54bdc
Author:flar 
Date:  2016-04-12 15:26 -0700
URL:   http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/e1688df54bdc

8153348: JavaFX API doc examples use nonexistent package 
"javafx.animation.transition"


! modules/graphics/src/main/java/javafx/animation/FadeTransition.java
! modules/graphics/src/main/java/javafx/animation/FillTransition.java
! modules/graphics/src/main/java/javafx/animation/PathTransition.java
! modules/graphics/src/main/java/javafx/animation/PauseTransition.java
! modules/graphics/src/main/java/javafx/animation/RotateTransition.java
! modules/graphics/src/main/java/javafx/animation/ScaleTransition.java
! modules/graphics/src/main/java/javafx/animation/StrokeTransition.java
! modules/graphics/src/main/java/javafx/animation/TranslateTransition.java
! modules/graphics/src/main/java/javafx/animation/package.html

Re: [9] Review request: 8091832: Provide API for getting the Screen scale on HiDPI screens

[Jim Graham]

Updated again with the following additions:

http://cr.openjdk.java.net/~flar/JDK-8091832/webrev.rt.03/

- Switch to using USER_DEFAULT_SCREEN_DPI pre-defined constant instead 
of hard-coded 96's
- Added @Ignore to failing test RT32570Test.java, to be fixed under 
JDK-8153542


...jim

On 3/31/16 12:52 AM, Jim Graham wrote:

I've updated the fix with the following additions:

http://cr.openjdk.java.net/~flar/JDK-8091832/webrev.rt.02/

- Redundant or obsolete command line overrides removed from Windows code
as follows:
   Settings still supported:
 -Dglass.win.uiScale
   Settings no longer supported, implementation conflicts with
Per-Monitor DPI support:
 -Dglass.win.minHiDPI
   Settings no longer supported, replaced by API in FX classes:
 -Dglass.win.renderScale
 -Dglass.win.forceIntegerRenderScale

- Font size now scales with uiScale override on Windows
- Fixes to scaling in JFXPanel
- Fixes to scaling in SwingPanel
- Monocle Screen initialization fixed

Note that SwingNode does not seem to relay the FX scaling parameters to
Swing correctly, but that was true before these fixes. This patch will
keep the functionality roughly the same, but additional fixes are needed
to do proper scaling of embedded Swing nodes. I looked at what was
needed and have an idea of what the fix would involve, but decided that
it was outside the scope of these fixes that are needed to get the HiDPI
FX properties implemented.

 ...jim

On 3/28/16 6:25 PM, Jim Graham wrote:

bug: https://bugs.openjdk.java.net/browse/JDK-8091832
webrev: http://cr.openjdk.java.net/~flar/JDK-8091832/webrev.rt.00/

This webrev fixes pixel snapping and application control over pixel
scaling on HiDPI screens:

- snap*() methods are all updated to take the current scale into account
- new variants of snap*() methods are added for separate X/Y control:
 Added: Region.snapSpaceX/Y()
 Added: Region.snapSizeX/Y()
 Added: Region.snapPositionX/Y()
- the non-X/Y variants of the above methods are now deprecated:
 Deprecated: Region.snapSpace()
 Deprecated: Region.snapSize()
 Deprecated: Region.snapPosition()
- methods to query the scale values of Screen objects:
 Added: Screen.getOutputScaleX/Y()
- properties to query and/or modify the scale values of Window objects:
 Added Read-Only  DoubleProperty:  Window.getOutputScaleX/Y()
 Added Read-Write BooleanProperty:
Window.set/getForceIntegerRenderScale()
 Added Read-Write DoubleProperty:  Window.set/getRenderScaleX/Y()

The changes have been compiled and tested on Windows and Mac and there
were trivial changes needed to the Linux files to adapt to one new
method signature, but I haven't done the test build on Linux yet...

 ...jim

Re: Is there a Anti-aliasing grayscale prism option?

[Jim Graham]

All Shape antialiasing should be grayscale.  The only non-grayscale AA 
we have is for text only, and that can be controlled using the 
fontSmoothingType property on the Text node.  Are these Text nodes or 
other nodes that show the colored pixels?


It might help to submit a small test case (as in, a couple of nodes) and 
a screen-shot...


...jim

On 4/1/2016 1:59 PM, Dell Green wrote:



Hi Guys,

I am designing a grayscale javafx application for an RGB666 LCD screen.
When I display it on screen the antialiasing contains the odd pink and blue 
pixels in the anti-aliasing.
I am seeing this on shape anti-aliasing.
Is there  a system property (prism?) I can pass in the will change the 
anti-aliasing algorithm to us a grayscale anti-aliasing?

I have set all colors used in the applications to grayscale by calling 
Color.grayscale()

Dell Green
R Software Manager
t: (+44)203 668 9870




206 Great Portland Street
London W1W 5QJ


Ideaworks (London) Ltd is a company registered in England and Wales, Company 
Registration no: 3943726. Registered office: 206 Great Portland Street, London, 
W1W 5QJ. This email and its contents are confidential. If you have received 
this message in error, please notify us and delete it. Any views presented in 
this email are solely those of the author and do not necessarily represent 
those of the company.

Re: [9] Review request: 8091832: Provide API for getting the Screen scale on HiDPI screens

[Jim Graham]

I've updated the fix with the following additions:

http://cr.openjdk.java.net/~flar/JDK-8091832/webrev.rt.02/

- Redundant or obsolete command line overrides removed from Windows code 
as follows:

  Settings still supported:
-Dglass.win.uiScale
  Settings no longer supported, implementation conflicts with 
Per-Monitor DPI support:

-Dglass.win.minHiDPI
  Settings no longer supported, replaced by API in FX classes:
-Dglass.win.renderScale
-Dglass.win.forceIntegerRenderScale

- Font size now scales with uiScale override on Windows
- Fixes to scaling in JFXPanel
- Fixes to scaling in SwingPanel
- Monocle Screen initialization fixed

Note that SwingNode does not seem to relay the FX scaling parameters to 
Swing correctly, but that was true before these fixes. This patch will 
keep the functionality roughly the same, but additional fixes are needed 
to do proper scaling of embedded Swing nodes. I looked at what was 
needed and have an idea of what the fix would involve, but decided that 
it was outside the scope of these fixes that are needed to get the HiDPI 
FX properties implemented.


...jim

On 3/28/16 6:25 PM, Jim Graham wrote:

bug: https://bugs.openjdk.java.net/browse/JDK-8091832
webrev: http://cr.openjdk.java.net/~flar/JDK-8091832/webrev.rt.00/

This webrev fixes pixel snapping and application control over pixel
scaling on HiDPI screens:

- snap*() methods are all updated to take the current scale into account
- new variants of snap*() methods are added for separate X/Y control:
 Added: Region.snapSpaceX/Y()
 Added: Region.snapSizeX/Y()
 Added: Region.snapPositionX/Y()
- the non-X/Y variants of the above methods are now deprecated:
 Deprecated: Region.snapSpace()
 Deprecated: Region.snapSize()
 Deprecated: Region.snapPosition()
- methods to query the scale values of Screen objects:
 Added: Screen.getOutputScaleX/Y()
- properties to query and/or modify the scale values of Window objects:
 Added Read-Only  DoubleProperty:  Window.getOutputScaleX/Y()
 Added Read-Write BooleanProperty:
Window.set/getForceIntegerRenderScale()
 Added Read-Write DoubleProperty:  Window.set/getRenderScaleX/Y()

The changes have been compiled and tested on Windows and Mac and there
were trivial changes needed to the Linux files to adapt to one new
method signature, but I haven't done the test build on Linux yet...

 ...jim

Re: [9] Review request: 8091832: Provide API for getting the Screen scale on HiDPI screens

[Jim Graham]

Windows provides separate values for the X & Y direction.  All of their 
system APIs that deal with screen and window resolutions provide both 
values...


...jim

On 3/30/2016 12:12 AM, Dr. Michael Paus wrote:

Hi,
I'd like to know on which systems this distinction between X- and
Y-direction is actually relevant.
I've never seen such a system before.
Michael

Am 29.03.16 um 03:25 schrieb Jim Graham:

bug: https://bugs.openjdk.java.net/browse/JDK-8091832
webrev: http://cr.openjdk.java.net/~flar/JDK-8091832/webrev.rt.00/

This webrev fixes pixel snapping and application control over pixel
scaling on HiDPI screens:

- snap*() methods are all updated to take the current scale into account
- new variants of snap*() methods are added for separate X/Y control:
Added: Region.snapSpaceX/Y()
Added: Region.snapSizeX/Y()
Added: Region.snapPositionX/Y()
- the non-X/Y variants of the above methods are now deprecated:
Deprecated: Region.snapSpace()
Deprecated: Region.snapSize()
Deprecated: Region.snapPosition()
- methods to query the scale values of Screen objects:
Added: Screen.getOutputScaleX/Y()
- properties to query and/or modify the scale values of Window objects:
Added Read-Only  DoubleProperty:  Window.getOutputScaleX/Y()
Added Read-Write BooleanProperty:
Window.set/getForceIntegerRenderScale()
Added Read-Write DoubleProperty: Window.set/getRenderScaleX/Y()

The changes have been compiled and tested on Windows and Mac and there
were trivial changes needed to the Linux files to adapt to one new
method signature, but I haven't done the test build on Linux yet...

...jim

Re: buffer too small

[Jim Graham]

I thought this sounded familiar.  I remember having the thought before, 
but forgot that I actually did something about this already in another 
part of the code.


Both of those sets of methods already exist and they are used in the 
decora ImagePool object to avoid having to allocate a larger scratch 
texture when this over-allocation occurs.  Similar code should be used 
in the updates of the maskTex object.  It would probably be worthwhile 
to immediately adjust it to maxContent dimensions right after it is 
allocated.


That doesn't obviate the fix below, but it adds an optimization...

...jim

On 3/9/16 3:26 PM, Jim Graham wrote:

Hi Johan,

That sounds like the fix then.

Note that there is another optimization issue here that could be
addressed in a follow-on - basically when a larger physical size is
allocated, then we could expand the content dimensions to match what was
allocated.  We would possibly need a new method on the Texture to do
something like "getMaxContentW/H()" and "setContentW/H()", and that
could avoid reallocating a texture in that case...

 ...jim

On 3/9/16 2:01 AM, Johan Vos wrote:

Hi Jim,

Passing the contentWidth to the update() fixes the problem as well, and
if the excessive memory is not needed (as in my proposal), this is of
course much better.
Can I create an issue and suggest the following patch (this is for 8,
but I can do a similar for 9)?

- Johan

---
a/modules/graphics/src/main/java/com/sun/prism/impl/BaseContext.javaThu
Mar 03 03:22:09 2016 +0100

+++
b/modules/graphics/src/main/java/com/sun/prism/impl/BaseContext.javaWed
Mar 09 10:59:35 2016 +0100

@@ -104,7 +104,7 @@

  // since it was bound and unflushed...

  maskTex.update(maskBuffer, maskTex.getPixelFormat(),

 0, 0, 0, 0, highMaskCol, nextMaskRow,

-   maskTex.getPhysicalWidth(), true);

+   maskTex.getContentWidth(), true);

  maskTex.unlock();

  curMaskRow = curMaskCol = nextMaskRow = highMaskCol = 0;

  }


On Tue, Mar 8, 2016 at 10:43 PM, Jim Graham <james.gra...@oracle.com
<mailto:james.gra...@oracle.com>> wrote:

I think I see the issue.

In the code that calls maskTex.update(...) it passes
maskTex.physicalWidth() as the scan stride of the buffer, but the
scan stride of the buffer is based on the content size, not the
physical size.  Thus, the checkUpdateParams() method overestimates
how many bytes are consumed by the operation.

Changing that to maskTex.getContentWidth() should be fine...

 ...jim

On 3/8/16 6:14 AM, Johan Vos wrote:

We got a number of bug reports (on Android and iOS) reported by
developers
using large images:

java.lang.IllegalArgumentException: Upload requires 2475266
elements, but
only 1549938 elements remain in the buffer

at

com.sun.prism.impl.BaseTexture.checkUpdateParams(BaseTexture.java:354)

at com.sun.prism.es2.ES2Texture.update(ES2Texture.java:640)

at

com.sun.prism.impl.BaseContext.flushVertexBuffer(BaseContext.java:106)

at

com.sun.prism.impl.BaseContext.updateMaskTexture(BaseContext.java:248)

at
com.sun.prism.impl.ps

<http://com.sun.prism.impl.ps>.BaseShaderGraphics.renderShape(BaseShaderGraphics.java:482)



I traced this down to the following:

initially, a buffer of [1024 * 1024] is allocated by
BaseContext.validateMaskTexture. When the MaskData becomes
bigger than 1024
(w or h), a new buffer is allocated with capacity [newTexW *
newTexH] with
newTexW and newTexH the new width/height that are passed when
creating a
new Texture. However, the physical size of the texture can be
different --
e.g.

ES2Texture.create (ES2Context, PixelFormat, WrapMode, int, int,
bool) will
in some cases set the real texWidth/height to the next power
of 2.

Subsequently, in next rendering loops when the Texture needs to
be updated,
it is checked whether the capacity of the buffer is large enough
to hold
the texture. In this case, the physical width is passed and the
buffer is
not large enough.

Adding the following two lines in
BaseContext.validateMaskTexture() (line
220) fixes the problem:

  newTexW = Math.max(newTexW,
maskTex.getPhysicalWidth());

  newTexH = Math.max(newTexH,
maskTex.getPhysicalHeight());

Using this patch, the size of the buffer will take the physical
size of the
texture into account. I'm not sure this is the best approach
though.

- Johan

Re: buffer too small

[Jim Graham]

Hi Johan,

That sounds like the fix then.

Note that there is another optimization issue here that could be 
addressed in a follow-on - basically when a larger physical size is 
allocated, then we could expand the content dimensions to match what was 
allocated.  We would possibly need a new method on the Texture to do 
something like "getMaxContentW/H()" and "setContentW/H()", and that 
could avoid reallocating a texture in that case...


...jim

On 3/9/16 2:01 AM, Johan Vos wrote:

Hi Jim,

Passing the contentWidth to the update() fixes the problem as well, and
if the excessive memory is not needed (as in my proposal), this is of
course much better.
Can I create an issue and suggest the following patch (this is for 8,
but I can do a similar for 9)?

- Johan

---
a/modules/graphics/src/main/java/com/sun/prism/impl/BaseContext.javaThu
Mar 03 03:22:09 2016 +0100

+++
b/modules/graphics/src/main/java/com/sun/prism/impl/BaseContext.javaWed
Mar 09 10:59:35 2016 +0100

@@ -104,7 +104,7 @@

  // since it was bound and unflushed...

  maskTex.update(maskBuffer, maskTex.getPixelFormat(),

 0, 0, 0, 0, highMaskCol, nextMaskRow,

-   maskTex.getPhysicalWidth(), true);

+   maskTex.getContentWidth(), true);

  maskTex.unlock();

  curMaskRow = curMaskCol = nextMaskRow = highMaskCol = 0;

  }


On Tue, Mar 8, 2016 at 10:43 PM, Jim Graham <james.gra...@oracle.com
<mailto:james.gra...@oracle.com>> wrote:

I think I see the issue.

In the code that calls maskTex.update(...) it passes
maskTex.physicalWidth() as the scan stride of the buffer, but the
scan stride of the buffer is based on the content size, not the
physical size.  Thus, the checkUpdateParams() method overestimates
how many bytes are consumed by the operation.

Changing that to maskTex.getContentWidth() should be fine...

 ...jim

On 3/8/16 6:14 AM, Johan Vos wrote:

We got a number of bug reports (on Android and iOS) reported by
developers
using large images:

java.lang.IllegalArgumentException: Upload requires 2475266
elements, but
only 1549938 elements remain in the buffer

at
com.sun.prism.impl.BaseTexture.checkUpdateParams(BaseTexture.java:354)

at com.sun.prism.es2.ES2Texture.update(ES2Texture.java:640)

at
com.sun.prism.impl.BaseContext.flushVertexBuffer(BaseContext.java:106)

at
com.sun.prism.impl.BaseContext.updateMaskTexture(BaseContext.java:248)

at
com.sun.prism.impl.ps

<http://com.sun.prism.impl.ps>.BaseShaderGraphics.renderShape(BaseShaderGraphics.java:482)


I traced this down to the following:

initially, a buffer of [1024 * 1024] is allocated by
BaseContext.validateMaskTexture. When the MaskData becomes
bigger than 1024
(w or h), a new buffer is allocated with capacity [newTexW *
newTexH] with
newTexW and newTexH the new width/height that are passed when
creating a
new Texture. However, the physical size of the texture can be
different --
e.g.

ES2Texture.create (ES2Context, PixelFormat, WrapMode, int, int,
bool) will
in some cases set the real texWidth/height to the next power of 2.

Subsequently, in next rendering loops when the Texture needs to
be updated,
it is checked whether the capacity of the buffer is large enough
to hold
the texture. In this case, the physical width is passed and the
buffer is
not large enough.

Adding the following two lines in
BaseContext.validateMaskTexture() (line
220) fixes the problem:

  newTexW = Math.max(newTexW,
maskTex.getPhysicalWidth());

  newTexH = Math.max(newTexH,
maskTex.getPhysicalHeight());

Using this patch, the size of the buffer will take the physical
size of the
texture into account. I'm not sure this is the best approach though.

- Johan

Re: buffer too small

[Jim Graham]

I think I see the issue.

In the code that calls maskTex.update(...) it passes 
maskTex.physicalWidth() as the scan stride of the buffer, but the scan 
stride of the buffer is based on the content size, not the physical 
size.  Thus, the checkUpdateParams() method overestimates how many bytes 
are consumed by the operation.


Changing that to maskTex.getContentWidth() should be fine...

...jim

On 3/8/16 6:14 AM, Johan Vos wrote:

We got a number of bug reports (on Android and iOS) reported by developers
using large images:

java.lang.IllegalArgumentException: Upload requires 2475266 elements, but
only 1549938 elements remain in the buffer

at com.sun.prism.impl.BaseTexture.checkUpdateParams(BaseTexture.java:354)

at com.sun.prism.es2.ES2Texture.update(ES2Texture.java:640)

at com.sun.prism.impl.BaseContext.flushVertexBuffer(BaseContext.java:106)

at com.sun.prism.impl.BaseContext.updateMaskTexture(BaseContext.java:248)

at
com.sun.prism.impl.ps.BaseShaderGraphics.renderShape(BaseShaderGraphics.java:482)


I traced this down to the following:

initially, a buffer of [1024 * 1024] is allocated by
BaseContext.validateMaskTexture. When the MaskData becomes bigger than 1024
(w or h), a new buffer is allocated with capacity [newTexW * newTexH] with
newTexW and newTexH the new width/height that are passed when creating a
new Texture. However, the physical size of the texture can be different --
e.g.

ES2Texture.create (ES2Context, PixelFormat, WrapMode, int, int, bool) will
in some cases set the real texWidth/height to the next power of 2.

Subsequently, in next rendering loops when the Texture needs to be updated,
it is checked whether the capacity of the buffer is large enough to hold
the texture. In this case, the physical width is passed and the buffer is
not large enough.

Adding the following two lines in BaseContext.validateMaskTexture() (line
220) fixes the problem:

 newTexW = Math.max(newTexW, maskTex.getPhysicalWidth());

 newTexH = Math.max(newTexH, maskTex.getPhysicalHeight());

Using this patch, the size of the buffer will take the physical size of the
texture into account. I'm not sure this is the best approach though.

- Johan

Re: buffer too small

[Jim Graham]

Hi Johan,

Notes in line below...

On 3/8/16 6:14 AM, Johan Vos wrote:

We got a number of bug reports (on Android and iOS) reported by developers
using large images:

java.lang.IllegalArgumentException: Upload requires 2475266 elements, but
only 1549938 elements remain in the buffer

at com.sun.prism.impl.BaseTexture.checkUpdateParams(BaseTexture.java:354)

at com.sun.prism.es2.ES2Texture.update(ES2Texture.java:640)

at com.sun.prism.impl.BaseContext.flushVertexBuffer(BaseContext.java:106)

at com.sun.prism.impl.BaseContext.updateMaskTexture(BaseContext.java:248)

at
com.sun.prism.impl.ps.BaseShaderGraphics.renderShape(BaseShaderGraphics.java:482)


I traced this down to the following:

initially, a buffer of [1024 * 1024] is allocated by
BaseContext.validateMaskTexture. When the MaskData becomes bigger than 1024
(w or h), a new buffer is allocated with capacity [newTexW * newTexH] with
newTexW and newTexH the new width/height that are passed when creating a
new Texture. However, the physical size of the texture can be different --
e.g.

ES2Texture.create (ES2Context, PixelFormat, WrapMode, int, int, bool) will
in some cases set the real texWidth/height to the next power of 2.


This is one of the primary reasons why content dimensions and physical 
dimensions are not the same.



Subsequently, in next rendering loops when the Texture needs to be updated,
it is checked whether the capacity of the buffer is large enough to hold
the texture. In this case, the physical width is passed and the buffer is
not large enough.


Where is this code that you referred to above?  Where is the physical 
width passed from/to?  Lines of code?


The capacity checks at the top of updateMaskTexture() are all using 
content dimensions which are appropriate, so where are the capacity 
checks you see which are using the physical width?



Adding the following two lines in BaseContext.validateMaskTexture() (line
220) fixes the problem:

 newTexW = Math.max(newTexW, maskTex.getPhysicalWidth());

 newTexH = Math.max(newTexH, maskTex.getPhysicalHeight());


That may mask the problem, but it doesn't fix the underlying problem. 
newTexWH are supposed to be the new content dimensions and should not be 
based on the old phyiscal dimensions or we will grow the texture 
unnecessarily large in many cases.



Using this patch, the size of the buffer will take the physical size of the
texture into account. I'm not sure this is the best approach though.


The buffer is used to update the content portion of the texture, it 
should only ever take the content dimensions of the texture into 
account.  The physical dimensions are not relevant to the discussion here.


There is an optimization that could avoid allocating a new texture that 
could be based on physical dimensions, but you aren't tying into it with 
the above analysis.  I want to get up to speed on what you've found 
above before I can recommend a more appropriate solution...


...jim

Re: GLS language errors (Was: Internal error: Error loading stock shader FillRoundRect_LinearGradient,_PAD on Vivante ARM)

[Jim Graham]

The thing about this use of pixcoord is that the same information can be 
supplied much more efficiently as a pair of texture coordinates.  The 
value of pixcoord ends up being a linear equation over the area of the 
primitive which is exactly what texture coordinate samples give you for 
free.


I believe some of the other gradient methods use that texture coordinate 
technique to avoid having to use pixcoord, but the issue is that we've 
hard-coded all of our VertexBuffer streams to have exactly 2 sets of 
texture coordinates and so you only get room to pass in so many values 
and these (i.e. this family of) shaders are already using those texture 
coordinates to pass in too many values to leave enough free for the 
gradient fractions.


This shader could be avoided, I believe, by rasterizing the shape into 
an alpha mask and using one of the alpha mask gradient shaders that 
doesn't rely on pixcoord.  In fact, in some embedded environments these 
shaders have so many computations per pixel that running the shape 
rasterizer on the CPU actually wins performance (and especially if you 
cache the alpha masks as some of our NGShape nodes do)...


...jim

On 3/1/16 9:10 AM, Maurice wrote:

Erik,

I've included the generated source code of
FillRoundRect_LinearGradient_PAD. I've made the offending lines bold.
I've experimented with adding some modifiers, but to no avail.

Maurice.

#ifdef GL_ES
#extension GL_OES_standard_derivatives : enable
#ifdef GL_FRAGMENT_PRECISION_HIGH
precision highp float;
precision highp int;
#else
precision mediump float;
precision mediump int;
#endif
#else
#define highp
#define mediump
#define lowp
#endif
varying vec2 texCoord0;
varying vec2 texCoord1;
varying lowp vec4 perVertexColor;
uniform vec4 jsl_pixCoordOffset;
*vec2 pixcoord = vec2*(
*gl_FragCoord.x-jsl_pixCoordOffset.x,**
**((jsl_pixCoordOffset.z-gl_FragCoord.y)*jsl_pixCoordOffset.w)-jsl_pixCoordOffset.y);**

*uniform vec2 oinvarcradii;
lowp float mask(vec2 tco, vec2 oflatdim) {
vec2 absecctco = max(abs(tco) - oflatdim, 0.001) * oinvarcradii;
float ecclensq = dot(absecctco, absecctco);
float pix = dot(absecctco / ecclensq, oinvarcradii);
return clamp(0.5 + (1.0 + 0.25 * pix * pix - ecclensq) / (2.0 * pix),
0.0, 1.0);
}
const int MAX_FRACTIONS = 12;
const float TEXTURE_WIDTH = 16.0;
const float FULL_TEXEL_X = 1.0 / TEXTURE_WIDTH;
const float HALF_TEXEL_X = FULL_TEXEL_X / 2.0;
uniform vec4 fractions[12];
uniform sampler2D colors;
uniform float offset;
vec4 sampleGradient(float dist) {
int i;
float relFraction = 0.0;
{
relFraction += clamp((dist - fractions[0].x) * fractions[0].y, 0.0, 1.0);
}
{
relFraction += clamp((dist - fractions[1].x) * fractions[1].y, 0.0, 1.0);
}
{
relFraction += clamp((dist - fractions[2].x) * fractions[2].y, 0.0, 1.0);
}
{
relFraction += clamp((dist - fractions[3].x) * fractions[3].y, 0.0, 1.0);
}
{
relFraction += clamp((dist - fractions[4].x) * fractions[4].y, 0.0, 1.0);
}
{
relFraction += clamp((dist - fractions[5].x) * fractions[5].y, 0.0, 1.0);
}
{
relFraction += clamp((dist - fractions[6].x) * fractions[6].y, 0.0, 1.0);
}
{
relFraction += clamp((dist - fractions[7].x) * fractions[7].y, 0.0, 1.0);
}
{
relFraction += clamp((dist - fractions[8].x) * fractions[8].y, 0.0, 1.0);
}
{
relFraction += clamp((dist - fractions[9].x) * fractions[9].y, 0.0, 1.0);
}
{
relFraction += clamp((dist - fractions[10].x) * fractions[10].y, 0.0, 1.0);
}
float tc = HALF_TEXEL_X + (FULL_TEXEL_X * relFraction);
return texture2D(colors, vec2(tc, offset));
}
vec4 cycleNone(float dist) {
if (dist <= 0.0){
return texture2D(colors, vec2(0.0, offset));
}
  else if (dist >= 1.0){
return texture2D(colors, vec2(1.0, offset));
}
  else {
return sampleGradient(dist);
}
}
vec4 cycleReflect(float dist) {
dist = 1.0 - (abs(fract(dist * 0.5) - 0.5) * 2.0);
return sampleGradient(dist);
}
vec4 cycleRepeat(float dist) {
dist = fract(dist);
return sampleGradient(dist);
}
uniform vec4 gradParams;
uniform vec3 perspVec;
lowp vec4 paint(vec2 winCoord) {
vec3 fragCoord = vec3(winCoord.x, winCoord.y, 1.0);
float dist = dot(fragCoord, gradParams.xyz);
float wdist = dot(fragCoord, perspVec);
return cycleNone(gradParams.w + dist / wdist);
}
void main() {
gl_FragColor = mask(texCoord0, texCoord1) * paint(pixcoord) *
perVertexColor;
}

Op 01-03-16 om 15:09 schreef Erik De Rijcke:

I'm not a shader expert  either but I have worked (and written some
myself) with them, so I'll give my 0.02$ (given that I have no idea
what the complete shader looks like)

It looks to me like the initialization of a default scoped global (eg
Johan's vec2 pixcoord) is done using non constant variables. What that
means is if those variables are  'varying' variables or basically
anything that is only know at the time the shader is invoked... well
that's a paddlin' because you are referring to per invocation scoped
variables (like 'gl_FragCoord' but anything with a 'varying' or maybe
even 'uniforms'storage qualifier might fail too I would assume?) 

Re: [9] review request: 8149967: Cannot compile JFXPanel with JDK 9: SurfaceData::getDefaultScale not found

[Jim Graham]

I think what Sergey was getting at is that in the short term you could 
use GC.getDefaultTransform() without any reflection as a workaround, but 
if you've already done the reflection work and it's going away shortly 
it doesn't really matter.


In the end you would want to use the getScaleXY() methods anyway as that 
avoids having to construct a transform as in the getDefaultTX() case...


...jim

On 2/17/2016 6:52 AM, Kevin Rushforth wrote:

Thanks, Sergey.

I will file a follow-up issue to remove the reflection code and provide
a suitable replacement.

-- Kevin


Sergey Bylokhov wrote:

Note that in jdk9 the same data can be obtained via
GraphicsConfiguration.getDefaultTransform();

On 17.02.16 2:53, Jim Graham wrote:

I added a comment on the bug about BC, but it sounds like you already
considered it.  I'm fine with this as is, but would push for moving to
JDK9 ASAP...

 ...jim

On 2/16/2016 3:49 PM, Kevin Rushforth wrote:

[resend with correct subject line]

Jim and Alexander,

Please review the following fix to allow JFXPanel to compile (and run)
again on JDK 9.

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

Note that I did not provide a fallback to the existing JDK 8 method,
since we are going to require JDK 9 soon anyway.

Note also that there is still one more bug fix needed before we can
compile everything with JDK 9, so this is necessary but not sufficient.

-- Kevin