Re: JavaFX graphics performance and suitability for advanced animations

[Jim Graham]

Another thing to consider is that you may get your port off the ground 
and running a little faster by starting with a Canvas port since it is 
closer to your Graphics2D code.  But, that depends on how much picking 
you were doing and/or animation and you'd have to manage your own 
(partial or full) update mechanisms.


That doesn't mean it will be the right architecture in the end, but it 
may get you to demo stage sooner...


...jim

On 5/29/13 10:59 AM, Richard Bair wrote:

I would start with scene graph nodes and see how it goes. The Canvas should be 
thought of as a nice utility for drawing images, rather than a hook into the 
low-level drawing machinery.

Richard


On May 29, 2013, at 10:23 AM, Hervé Girod  wrote:


Hello,

Concerning my previous question, is it planned to provide some kind of "shape " 
drawing for JavaFX 8 in the graphic context, or is it safer to assume that the scene 
graph will be the preferred way to handle this (apart from the svgpath drawing of course)?

I have the same kind of question about attributed strings which are handled 
using the TextFlow node in JavaFX 8, but for which there is no direct way to 
handle them in the canvas graphic context.

We are currently trying to draw complex and animated map overlays in JavaFX.

We did that drawing directly in the graphic context in swing, and we assumed 
that it was safe to use the JavaFX canvas graphic context to do the same thing, 
but are now not so sure that its the right way to handle it. I'm not talking 
about the current JavaFX 2.2 API, but rather what you are planning for Java FX 
8.

Herve

Sent from my iPhone

On 29 mai 2013, at 02:40, Herve Girod  wrote:


Thanks for your answer!

As for our experience, we are currently migrating a big UI application (Java 
ARINC 661 Server: http://sourceforge.net/projects/j661/)  from being 
Swing-based to JavaFX based. We still keep the Swing compatibility, but we 
found that using the JavaFX scene graph makes things MUCH more simple for us. 
And in our very preliminary tests it does seem that our performance is good 
(the application is almost completely ported and working, but several of our 
custom widgets implementations still have to be implemented in JavaFX, so we 
don't have direct comparisons yet).

However, as you talk about the scene graph / the canvas API, I have a question. We are 
mostly rendering controls and Shapes (exactly what JavaFX scene graph is about), but we 
also have to render Map overlays (with waypoints, flight plans, etc...). We used to do it 
by overriding the paintComponent method of a custom Component in our Swing 
implementation, dealing directly with the Graphics2D Layer. The "natural" path 
for us with JavaFX would be to use the Canvas widget, and GraphicContext, but reading 
your answer, I begin to suspect that we could maybe achieve a better performance with a 
simpler architecture by using directly the scenegraph for that too. What do you think?

Herve


2013/5/29 Richard Bair 
Hi John,


1.   Can someone from Oracle please outline the full range of
applications for which JavaFX is or will be suitable for?


That's a pretty broad question. Lots of stuff? At a minimum everything Swing 
and SWT were used for, as well as mobile and embedded UIs, rich media, 
graphics, etc. I don't expect somebody to write Halo 5 with it.


2.   Is there something inherent in the JavaFX architecture (such as
CPU/GPU interaction, the performance of the JVM or the Java language itself)
that limits its suitability and thus effectiveness in advanced
animations/visualisations?


Absolutely not, in fact, quite the opposite. The basic architecture (threading 
model, GPU usage model, etc) is designed for high concurrency and throughput. 
Some of the features in Controls though (like CSS lookup, color derivation, 
etc) put a tax on performance. When it wasn't exposed in the API, every design 
decision is made with performance as a for most thought. When it comes to API 
usability is considered primarily but performance is also always considered 
(along with security). And for every feature that adds weight, there is a way 
to avoid it when absolutely necessary.


3.   Is this choppiness and lack of smoothness I have experienced
typical of JavaFX performance or is it some issue with my
environment/drivers etc.?


Hard to say. I saw a couple weeks ago a machine where scrolling the table view 
was 14fps whereas it was 320fps for me. The difference was the other system was 
falling back to the software pipeline. To determine what you're seeing, we need 
to know whether what you're running is producing consistently slow results or 
erratic results. Also, we need to know whether you are render bound or compute 
bound. What's taking so long to complete?

We have seen situations where we are preparing a frame but never rendering it, 
which might also be contributing to the choppiness.


4.   Is JavaFX more targeted at form-based UIs rather than 

Re: Mixing 2D and 3D

[Jim Graham]

I'm a little behind on getting into this discussion.

I don't have a lot of background in 3D application design, but I do have some low-level rendering 
algorithm familiarity, and Richard's summary is an excellent outline of the issues I was having 
with rampant mixing of 2D and 3D.  I see them as fundamentally different approaches to 
presentation, but that easily combine in larger chunks such as "2D UI HUD over 3D scene" 
and "3D-ish effects on otherwise 2D-ish objects".  I think the discussion covered all of 
those in better detail than I could outline.

My main concerns with having the full-3D parts of the Scene Graph be mix-in 
nodes are that there are attributes on Node that sometimes don't make sense, 
but other times aren't necessarily the best approach to providing the 
functionality in a 3D scene.  I was actually a little surprised at how few 
attributes fell into the category of not making sense when Richard went through 
the list in an earlier message.  A lot of the other attributes seem to be 
non-optimal though.

- What is the minimum space taken for "new Node()" these days?  Is that too heavyweight 
for a 3D scene with hundreds or thousands of "things", whatever granularity we have, or 
come to have, for our Nodes?

- How often do those attributes get used on a 3D object?  If one is modeling an 
engine, does one really need every mesh to be pickable, or are they likely to 
be multi-mesh groups that are pickable?  In other words, you might want to pick 
on the piston, but is that a single mesh?  And is the chain that connects it to 
the alternator a single mesh or a dozen meshes per link in the chain with 100 
links?  (Yes, I know that alternators use belts, but I'm trying to come up with 
meaningful examples.)

- How does picking work for 3D apps?  Is the ability to add listeners to 
individual objects good or bad?

- How does picking interact with meshes that are tessellated on the fly?  Does 
one ever want to know which tessellated triangle was picked?  How does that fit 
in with the 2D-ish picking events we deliver now?  If a cylinder is picked, how 
do we describe which part of the cylinder was picked?

- Are umpteen isolated 2D-ish transform attributes a convenient or useful way 
to manipulate 3D objects?  Or do we really want occasional transforms inserted 
in only a few places that are basically a full Affine3D because when you want a 
transform, you are most likely going to want to change several attributes at 
once?  2D loves isolated just-translations like candy.  It also tends to like 
simple 2D scales and rotates around Z here and there.  But doesn't 3D love 
quaternion-based 3-axis rotations and scales that very quickly fill most of the 
slots of a 3x3 or 4x3 matrix?

- Right now our Blend modes are pretty sparse, representing some of the more 
common equations that we were aware of, but I'm not sure how that may hold up 
in the future.  I can implement any blending equation that someone feeds me, 
and optimize the heck out of the math of it - but I'm pretty unfamiliar with 
which equations are useful to content creators in the 2D or 3D world or how 
they may differ now or in our evolution.

- How will the nodes, and the granularity they impose, enable or prevent 
getting to the point of an optimized bundle of vector and texture information 
stored on the card that we tweak and re-trigger?  I should probably start 
reading up on 3D hw utilization techniques.  8(

- Looking at the multi-camera issues I keep coming back to my original pre-mis-conceptions of what 
3D would add wherein I was under the novice impression that we should have 3D models that live 
outside the SG, but then have a 3DView that lives in the SG.  Multiple views would simply be 
multiple 3DView objects with shared models similar to multiple ImageViews vs. a small number of 
Image objects.  I'm not a 3D person so that was simply my amateur pre-conception of how 3D would be 
integrated, but I trust the expertise that went into what we have now.  In this pre-concept, 
though, there were fewer interactions of "3Disms" and "2Disms" - and much 
lighter weight players in the 3D models.

- In looking briefly at some 3D-lite demos it looks like there are attempts to do higher 
quality AA combined with depth sorting, possibly with breaking primitives up so that they 
depth sort more cleanly.  Some docs on the CSS3 3D attributes indicate particular 
algorithms that they recommend for slicing up the 2D objects to allow for back to front 
ordering that allows alpha to mix better with Z while not necessarily targeting the kinds 
of performance one might want for pure 3D.  Such techniques would also allow us to do the 
algorithmic AA that runs into trouble in the "circles" demo that Richard 
showed, but those techniques don't scale well.  On the other hand, it allows for things 
like the CSS3 demo that has 4 images on rotating fan blades with alpha - very pretty, but 
probably not done in a way that would facilitate a model

Re: Level of detail

[Jim Graham]

About the closest thing we have right now is the cacheHints which let you specify "that" 
you want to allow the cache to be scaled, but no ability to specify "by how much" before 
we re-render.  We've felt that mechanism needs to be eventually be evolved to allow more 
customization, but we haven't mapped out what controls/parameters to provide.

And, that is probably very basic compared to what you are asking for here, but 
a similar set of hints might work on an SVG node to control re-rendering...

...jim

On 7/29/2013 1:17 PM, Scott Palmer wrote:

The concept of Level of detail was recently brought up.  I believe it was
referring to 3D, but the concept applies to 2D as well, when we have the
concept of 2D scaling ("zoom")

Is there a current plan to address this?

Is there a current technique that can be used to get something close?

For example, the two use cases I have right now:

If I have a lot of Text nodes and they become unreadable when zoomed out,
is there a way I could have them fade out and not cost me layout and
rendering time?

If I have an image that is rendered from SVG data and used in multiple
places in the scene graph.  I want to update it if the user changes the
zoom level so that it isn't blocky, or replace it with the vector graphics
used to render it if the user zooms in so much that the image size would be
ridiculous.


I can't think of a reasonably easy way to implement those cases.

I'm thinking of some new kind of Pane that is sensitive to the size that it
will finally be rendered at.  It could be used to hide or replace it's
children based on the final bounds it will have on screen.


Scott

Re: API Change Proposal - Re: MSAA and Scene anti aliasing

[Jim Graham]

D'oh!  I knew I should have been checking this list a bit.  I hope this isn't 
too late to have any impact...

As an intermediate solution this is fine, but when we want to get into 
providing settings for MSAA and FSAA and other algorithms I think classes are 
more flexible than enums.  How about this solution:

package javafx.?

public class SceneAntialiasing {
public static final SceneAntialiasing DISABLED;
public static final SceneAntialiasing BALANCED;
public static final SceneAntialiasing FASTEST;
public static final SceneAntialiasing NICEST;

public static SceneAntialiasing[] getAvailableTechniques() { }

SceneAntialiasing() { /* package private constructor! */ }
}

public class MsaaAntialiasing extends SceneAntialiasing {
MSaaAntialiasing(int numsamp) { /* package private */ }
public int getNumSamples();
}

public class FsaaAntialiasing extends SceneAntialiasing {
FsaaAntialiasing(int numsamp) { /* package private */ }
public int getNumSamples();
}

Note that there are ways for the system to construct these objects without 
providing public constructors so that these become statically defined by the 
system.

What about Anisotropic filtering?  Is that considered a form of AA, or an 
option on top of AA?

...jim

On 7/24/2013 3:07 PM, Chien Yang wrote:

Thanks for the help! I was of 2 minds about it; alphabetical or logical.

public enum SceneAntiAliasing {
DISABLED, // disables anti-aliasing
BALANCED, // enables anti-aliasing using optimal system setting available 
that balances speed and quality
FASTEST, // enables anti-aliasing using minimum system setting available 
that results in better frame rate
NICEST // enables anti-aliasing using maximum system setting available that 
results in best visual quality
}

- Chien

On 7/24/2013 2:49 PM, Richard Bair wrote:

Just to be picky, I would put DISABLED first in the list. It seems more consistent 
to have the only OFF mode to be first and then all the rest of the options (which 
happen to then have ordinals > 0) will be some form of ON mode.

Richard

On Jul 24, 2013, at 2:37 PM, Chien Yang  wrote:


Thank you for the feedback! We decided to drop DEFAULT in favor of BALANCED. So 
here is the revised SceneAntiAliasing enum entries:

public enum SceneAntiAliasing {
BALANCED, // enables anti-aliasing using optimal system setting available 
that balances speed and quality
DISABLED, // disables anti-aliasing
FASTEST, // enables anti-aliasing using minimum system setting available 
that results in better frame rate
NICEST // enables anti-aliasing using maximum system setting available that 
results in best visual quality
}

Thanks,
- Chien

On 7/23/2013 1:29 PM, Chien Yang wrote:

Hi all,

We appreciate all the feedback you have contributed to this topic. After 
listening to the feedback and an internal discussion, we would like to propose 
a minor change to the API for supporting scene anti-aliasing. We intentionally 
choose not to expose the number of samples and techniques used in this release, 
but this doesn't preclude future addition when the time is right for more 
options. This change will be tracked by RT-31878 
(https://javafx-jira.kenai.com/browse/RT-31878):

Anti-aliasing API Change Proposal:

Constructors remove:
public Scene(Parent root, double width, double height, boolean depthBuffer, 
boolean antiAliasing)
public SubScene(Parent root, double width, double height, boolean depthBuffer, 
boolean antiAliasing)

Constructor add:
public Scene(Parent root, double width, double height, boolean depthBuffer, 
SceneAntiAliasing antiAliasing)
public SubScene(Parent root, double width, double height, boolean depthBuffer, 
SceneAntiAliasing antiAliasing)

Note:The antiAliasing argument will be used if the underlying graphics driver 
has anti-aliasing support.

Where SceneAntiAliasing is an enum with the following entries at the moment:

public enum SceneAntiAliasing {
DISABLED, // disables anti-aliasing
DEFAULT, // enables anti-aliasing using a default system setting available 
that balances speed and quality
FASTEST, // enables anti-aliasing using minimum system setting available 
that results in better frame rate
NICEST // enables anti-aliasing using maximum system setting available that 
results in best visual quality
}

Thanks,
- Chien

Re: API Change Proposal - Re: MSAA and Scene anti aliasing

[Jim Graham]

Also, the MSAA subclass might have:

public SampleMethod getSampleMethod();  // point, area, ?centroid?
public SamplePattern getSamplePattern(); // regular, sparse, stochastic

Also, static MsaaAA.get(params) returns the indicated object if there is 
an implementation that supports those parameters, otherwise returns 
null.  It might make sense to have each *AA subclass also have a static 
get() method that returns an array of all implementations of that 
particular form of AA (the get() method in the root class would be the 
union of all of those arrays)...


...jim

On 7/31/13 1:36 PM, Jim Graham wrote:

D'oh!  I knew I should have been checking this list a bit.  I hope this
isn't too late to have any impact...

As an intermediate solution this is fine, but when we want to get into
providing settings for MSAA and FSAA and other algorithms I think
classes are more flexible than enums.  How about this solution:

package javafx.?

public class SceneAntialiasing {
 public static final SceneAntialiasing DISABLED;
 public static final SceneAntialiasing BALANCED;
 public static final SceneAntialiasing FASTEST;
 public static final SceneAntialiasing NICEST;

 public static SceneAntialiasing[] getAvailableTechniques() { }

 SceneAntialiasing() { /* package private constructor! */ }
}

public class MsaaAntialiasing extends SceneAntialiasing {
 MSaaAntialiasing(int numsamp) { /* package private */ }
 public int getNumSamples();
}

public class FsaaAntialiasing extends SceneAntialiasing {
 FsaaAntialiasing(int numsamp) { /* package private */ }
 public int getNumSamples();
}

Note that there are ways for the system to construct these objects
without providing public constructors so that these become statically
defined by the system.

What about Anisotropic filtering?  Is that considered a form of AA, or
an option on top of AA?

 ...jim

On 7/24/2013 3:07 PM, Chien Yang wrote:

Thanks for the help! I was of 2 minds about it; alphabetical or logical.

public enum SceneAntiAliasing {
DISABLED, // disables anti-aliasing
BALANCED, // enables anti-aliasing using optimal system setting
available that balances speed and quality
FASTEST, // enables anti-aliasing using minimum system setting
available that results in better frame rate
NICEST // enables anti-aliasing using maximum system setting
available that results in best visual quality
}

- Chien

On 7/24/2013 2:49 PM, Richard Bair wrote:

Just to be picky, I would put DISABLED first in the list. It seems
more consistent to have the only OFF mode to be first and then all
the rest of the options (which happen to then have ordinals > 0) will
be some form of ON mode.

Richard

On Jul 24, 2013, at 2:37 PM, Chien Yang  wrote:


Thank you for the feedback! We decided to drop DEFAULT in favor of
BALANCED. So here is the revised SceneAntiAliasing enum entries:

public enum SceneAntiAliasing {
BALANCED, // enables anti-aliasing using optimal system setting
available that balances speed and quality
DISABLED, // disables anti-aliasing
FASTEST, // enables anti-aliasing using minimum system setting
available that results in better frame rate
NICEST // enables anti-aliasing using maximum system setting
available that results in best visual quality
}

Thanks,
- Chien

On 7/23/2013 1:29 PM, Chien Yang wrote:

Hi all,

We appreciate all the feedback you have contributed to this
topic. After listening to the feedback and an internal discussion,
we would like to propose a minor change to the API for supporting
scene anti-aliasing. We intentionally choose not to expose the
number of samples and techniques used in this release, but this
doesn't preclude future addition when the time is right for more
options. This change will be tracked by RT-31878
(https://javafx-jira.kenai.com/browse/RT-31878):

Anti-aliasing API Change Proposal:

Constructors remove:
public Scene(Parent root, double width, double height, boolean
depthBuffer, boolean antiAliasing)
public SubScene(Parent root, double width, double height, boolean
depthBuffer, boolean antiAliasing)

Constructor add:
public Scene(Parent root, double width, double height, boolean
depthBuffer, SceneAntiAliasing antiAliasing)
public SubScene(Parent root, double width, double height, boolean
depthBuffer, SceneAntiAliasing antiAliasing)

Note:The antiAliasing argument will be used if the underlying
graphics driver has anti-aliasing support.

Where SceneAntiAliasing is an enum with the following entries at
the moment:

public enum SceneAntiAliasing {
DISABLED, // disables anti-aliasing
DEFAULT, // enables anti-aliasing using a default system
setting available that balances speed and quality
FASTEST, // enables anti-aliasing using minimum system setting
available that results in better frame rate
NICEST // enables anti-aliasing using maximum system setting
available that results in best visual quality
}

Thanks,
- Chien

Re: API Change Proposal - Re: MSAA and Scene anti aliasing

[Jim Graham]

+1

Constants good enough for now.  First pass doesn't really need 
subclasses since the 4 constants we start with are virtual and somewhat 
opaque, but we could put them in for grins if we want.


But, when we start making the different AA types selectable and 
parameterized then I think we want to have subclasses at that point. 
The concepts of "where are the subsamples taken" (both in respect to the 
positioning of the N sub-samples that determine coverage and also which 
of them, if any, are used to execute the shader) in MSAA have no bearing 
on FSAA, for example.  Trying to mix and match disparate descriptive 
parameters in a single class gets awkward...


...jim

On 7/31/13 5:09 PM, Richard Bair wrote:

Sorry let me be clear. Having a class vs an enum was my preferred approach and 
I'm glad with Jim's nudge it looks like we'll go there. Having just the few 
predefined constants as a starting point I think is good, and we can add 
sub-types and more goodness in the future.

Richard

On Jul 31, 2013, at 4:47 PM, Richard Bair  wrote:


I'm pretty confident we'll want different sub-types as we go along (CSAA, MSAA, 
FXAA -- there are a lot of different ways to do full-scene anti-aliasing and I 
bet that they will have different parameters for controlling their various 
algorithms), but we can cross that bridge later.

Richard

On Jul 31, 2013, at 4:36 PM, Chien Yang  wrote:


I agree, however I would prefer a single class over subclasses if possible. I 
have added Jim's proposal to the JIRA for consideration.

https://javafx-jira.kenai.com/browse/RT-31878

Thanks,
- Chien



On 7/31/2013 3:21 PM, Kevin Rushforth wrote:

This seems cleaner in terms of extensibility. I think we can wait on adding 
anything other than the public static finals for this release, but plan to 
extend it using something like what Jim suggests.

-- Kevin


Richard Bair wrote:

Personally I liked this approach. It was like an enum in ease of use but much 
more extensible in the future when we add more anti-aliasing types and twiddles.

Richard

On Jul 31, 2013, at 1:36 PM, Jim Graham  wrote:


D'oh!  I knew I should have been checking this list a bit.  I hope this isn't 
too late to have any impact...

As an intermediate solution this is fine, but when we want to get into 
providing settings for MSAA and FSAA and other algorithms I think classes are 
more flexible than enums.  How about this solution:

package javafx.?

public class SceneAntialiasing {
  public static final SceneAntialiasing DISABLED;
  public static final SceneAntialiasing BALANCED;
  public static final SceneAntialiasing FASTEST;
  public static final SceneAntialiasing NICEST;

  public static SceneAntialiasing[] getAvailableTechniques() { }

  SceneAntialiasing() { /* package private constructor! */ }
}

public class MsaaAntialiasing extends SceneAntialiasing {
  MSaaAntialiasing(int numsamp) { /* package private */ }
  public int getNumSamples();
}

public class FsaaAntialiasing extends SceneAntialiasing {
  FsaaAntialiasing(int numsamp) { /* package private */ }
  public int getNumSamples();
}

Note that there are ways for the system to construct these objects without 
providing public constructors so that these become statically defined by the 
system.

What about Anisotropic filtering?  Is that considered a form of AA, or an 
option on top of AA?

   ...jim

On 7/24/2013 3:07 PM, Chien Yang wrote:

Thanks for the help! I was of 2 minds about it; alphabetical or logical.

public enum SceneAntiAliasing {
  DISABLED, // disables anti-aliasing
  BALANCED, // enables anti-aliasing using optimal system setting available 
that balances speed and quality
  FASTEST, // enables anti-aliasing using minimum system setting available that 
results in better frame rate
  NICEST // enables anti-aliasing using maximum system setting available that 
results in best visual quality
}

- Chien

On 7/24/2013 2:49 PM, Richard Bair wrote:

Just to be picky, I would put DISABLED first in the list. It seems more consistent 
to have the only OFF mode to be first and then all the rest of the options (which 
happen to then have ordinals > 0) will be some form of ON mode.

Richard

On Jul 24, 2013, at 2:37 PM, Chien Yang  wrote:


Thank you for the feedback! We decided to drop DEFAULT in favor of BALANCED. So 
here is the revised SceneAntiAliasing enum entries:

public enum SceneAntiAliasing {
  BALANCED, // enables anti-aliasing using optimal system setting available 
that balances speed and quality
  DISABLED, // disables anti-aliasing
  FASTEST, // enables anti-aliasing using minimum system setting available that 
results in better frame rate
  NICEST // enables anti-aliasing using maximum system setting available that 
results in best visual quality
}

Thanks,
- Chien

On 7/23/2013 1:29 PM, Chien Yang wrote:

Hi all,

  We appreciate all the feedback you have contributed to this topic. After

Re: Mixing 2D and 3D

[Jim Graham]

There is one Mesh per MeshView and MeshView is a Node, so we require a 
node per mesh.


But, a mesh can have millions of triangles.  As long as they all have a 
common material...


...jim

On 8/1/13 1:38 PM, Richard Bair wrote:

I intend to agree with you that Node may be "too big" to use as the basis
for a 3D model's triangles and quads given that there can easily be millions
of them and that manipulating or interacting with them on an individual
basis is mostly unlikely.  As you say, storing those models outside the
scenegraph seems to make sense...


I agree. We don't use Nodes as the basis for 3D model's mesh, we have a 
separate Mesh class which does this very efficiently (Mesh==Image, 
MeshView==ImageView).

Richard

Re: review request RT-34090: NGRegion border painting seems odd

[Jim Graham]

Hi Felipe,

The changes look good...

...jim

On 11/6/13 3:06 PM, Felipe Heidrich wrote:

Hi Jim,


Please review
https://javafx-jira.kenai.com/browse/RT-34090
http://cr.openjdk.java.net/~fheidric/RT34090/webrev/

Thanks
Felipe

Re: review request: RT-32837: Ensemble8 left arrow has line artifact

[Jim Graham]

Hi Felipe,

That looks fine - was there a reason to have the variable to store the 
predetermined border size?


...jim

On 11/4/13 4:55 PM, Felipe Heidrich wrote:

Hi Jim

Please review
https://javafx-jira.kenai.com/browse/RT-32837
You will find the webrev at the end:
http://cr.openjdk.java.net/~fheidric/RT32837-2/webrev/


Thank you
Felipe

Proportional paint object behavior inconsistent and needs to change

[Jim Graham]

Felipe mentioned recently that we encountered some issues in fixing a 
bug with SVGPath.


The outcome of this fix could be a significant change in how your 
proportional gradient fills look and so we'd like to get feedback on the 
various ideas.  You can read about them in that Jira issue, but I'll 
also summarize below.  Discussion would probably be better on the 
mailing list, but we eventually need to work the salient points back 
into the Jira issue for future maintenance.


The basic issue is that we had a disagreement between the way that the 
shape caching code worked and the way that uncached shapes were filled 
with proportional paints.


First, there is the concept of tight and loose bounds.  Loose bounds are 
very cheap to calculate, but can contain area not strictly inside the 
shape.  Tight bounds are more careful to figure out exactly how far 
curved sections of the shape reach, but a fair number of calculations 
and recursions are needed to accurately calculate those extents.


Our current code will use loose bounds of the basic shape (i.e. the part 
that is filled) to calculate the bounds for proportional paints when you 
first paint a shape - whether you stroke it, fill it, or both.


But, if you ran with shape mask caching (the default mode) then after a 
rendering or 2 we would decide to cache the antialias coverage mask and 
we would then use the node's content bounds to figure the bounds for the 
proportional paint.  The content bounds are calculated more precisely as 
tight bounds, though, so they didn't always agree with the bounds used 
in those first few uncached renderings.


The net result is that the proportional paint would shift after the 
first couple of frames unless you animated the shape and then it would 
revert while you were animating and then shift back when it was stable.


There is also the Canvas object that can also render proportional 
paints, and it does so using the same code that the shape nodes use when 
they don't have their coverage mask cached (i.e. loose fill bounds).


We'd like to make all of this as consistent as possible.  Here are the 
various decisions and how they'd impact code:


- Loose bounds are faster to calculate for shapes that aren't likely to 
be reused.  The uncached shape rendering used them because the shapes 
may change before we need the bounds again.  Canvas uses it because it 
is an immediate mode API with no input on how often it may see a 
particular shape again.


- Tight bounds would likely be less suprising to developers and users, 
though.  They are better for hit testing and damage management because 
they generate fewer false positive hits.  They are also fairly fast to 
calculate for most shapes and it is a rare shape that we'd have to 
recurse much to get it right.  Also, for straight edges there is no 
difference in performance to calculate tight or loose bounds.


All in all, it would probably be better for the FX API to standardize on 
tight bounds and treat any cases where we noticeably affect performance 
(which should be very rare) as opportunities for tuning.  This may not 
be compatible with the first rendering of current Shape nodes, but they 
would shift back and forth anyway so we aren't worried about 
incompatibility with an inconsistent system.


The other part of the decision is which bounds to use.

Currently uncached rendering uses fill bounds, and mask-cached rendering 
uses the content bounds which depends on whether you supply a stroke or 
a fill paint so it could be either fill bounds or stroke bounds.


- For filled-only shapes we obviously want to use the "fill bounds".

- For stroked and filled shapes, we have 3 choices:

- - use fill bounds for both paints so that the geometry used for 
proportional stroke and fill paints are similar for both parts of those 
nodes.  This helps line up any color discontinuities or highlights 
between the two.


- - use stroke bounds for both which also means the two are consistent, 
but Canvas can't really do this because it doesn't know if you are 
filling and stroking until it sees the latter operation


- - use fill bounds for fill and stroke bounds for stroke which means 
the geometries of the two are different and it makes it harder to line 
up the transitions of proportional stroke+fill shapes, but Canvas can do 
this so Canvas vs. Shape node remain consistent


I'm not sure which of the above is the best, but I lean towards "fill 
bounds for both" because it allows consistent geometry for stroke+fill 
and it allows consistent behavior between Canvas and Shape node, at the 
possible expense of "0% on a stroke isn't at the edge of the stroke".


Thoughts?

...jim

Re: Proportional paint object behavior inconsistent and needs to change

[Jim Graham]

Hi Scott,

That's an odd take on it.  It wouldn't be readily obvious to a developer why 
their background rectangle had the gradient a little off if they never planned 
to ever stroke it.

Also, keep in mind that while it might be slightly more expensive to calculate tight bounds than 
loose bounds, it is MUCH more expensive to calculate stroke bounds on a shape.  It's not a trivial 
"OK, add a few pixels for the stroke" type of case, you have to trace out the path and 
compute the perpendicular extensions in many cases.  So, basically you are saying "always do 
the most expensive bounds operation for ever shape that is filled even if the extra work would 
never come into play"...?

...jim

On 11/18/2013 4:58 PM, Scott Palmer wrote:

I would lean towards using the stroke bounds for both.  Those being the "real" 
bounds, and resulting in less mis-aligned gradients.  Always calculate the stroke bounds 
as if the shape will be stroked, so it doesn't affect Canvas.
If you don't want that to affect the bounds used for a gradient fill when you 
aren't stroking, set the stoke width to 0.


Scott


On Nov 18, 2013, at 5:58 PM, Jim Graham  wrote:

Felipe mentioned recently that we encountered some issues in fixing a bug with 
SVGPath.

The outcome of this fix could be a significant change in how your proportional 
gradient fills look and so we'd like to get feedback on the various ideas.  You 
can read about them in that Jira issue, but I'll also summarize below.  
Discussion would probably be better on the mailing list, but we eventually need 
to work the salient points back into the Jira issue for future maintenance.

The basic issue is that we had a disagreement between the way that the shape 
caching code worked and the way that uncached shapes were filled with 
proportional paints.

First, there is the concept of tight and loose bounds.  Loose bounds are very 
cheap to calculate, but can contain area not strictly inside the shape.  Tight 
bounds are more careful to figure out exactly how far curved sections of the 
shape reach, but a fair number of calculations and recursions are needed to 
accurately calculate those extents.

Our current code will use loose bounds of the basic shape (i.e. the part that 
is filled) to calculate the bounds for proportional paints when you first paint 
a shape - whether you stroke it, fill it, or both.

But, if you ran with shape mask caching (the default mode) then after a 
rendering or 2 we would decide to cache the antialias coverage mask and we 
would then use the node's content bounds to figure the bounds for the 
proportional paint.  The content bounds are calculated more precisely as tight 
bounds, though, so they didn't always agree with the bounds used in those first 
few uncached renderings.

The net result is that the proportional paint would shift after the first 
couple of frames unless you animated the shape and then it would revert while 
you were animating and then shift back when it was stable.

There is also the Canvas object that can also render proportional paints, and 
it does so using the same code that the shape nodes use when they don't have 
their coverage mask cached (i.e. loose fill bounds).

We'd like to make all of this as consistent as possible.  Here are the various 
decisions and how they'd impact code:

- Loose bounds are faster to calculate for shapes that aren't likely to be 
reused.  The uncached shape rendering used them because the shapes may change 
before we need the bounds again.  Canvas uses it because it is an immediate 
mode API with no input on how often it may see a particular shape again.

- Tight bounds would likely be less suprising to developers and users, though.  
They are better for hit testing and damage management because they generate 
fewer false positive hits.  They are also fairly fast to calculate for most 
shapes and it is a rare shape that we'd have to recurse much to get it right.  
Also, for straight edges there is no difference in performance to calculate 
tight or loose bounds.

All in all, it would probably be better for the FX API to standardize on tight 
bounds and treat any cases where we noticeably affect performance (which should 
be very rare) as opportunities for tuning.  This may not be compatible with the 
first rendering of current Shape nodes, but they would shift back and forth 
anyway so we aren't worried about incompatibility with an inconsistent system.

The other part of the decision is which bounds to use.

Currently uncached rendering uses fill bounds, and mask-cached rendering uses 
the content bounds which depends on whether you supply a stroke or a fill paint 
so it could be either fill bounds or stroke bounds.

- For filled-only shapes we obviously want to use the "fill bounds".

- For stroked and filled shapes, we have 3 choices:

- - use fill

Re: Proportional paint object behavior inconsistent and needs to change

[Jim Graham]

Hi Jasper,

I had a long reply over the issues of consistency and then I got hit on the 
head by an idea hammer.  Here's another wrench in the equation.

Only Path and SVGPath objects have ever had an ambiguity, but Rectangle objects 
have always had a consistent behavior.  And, that behavior is to have both 
stroke and fill match the fill bounds.

Below is a small example to show that the stroke on a rectangle has its 0.0 and 
1.0 at the fill bounds.  You can even click on the window to cause multiple 
repaints and see that the behavior remains stable over multiple frames (we 
don't cache rectangle coverage masks like we do for arbitrary shapes).

So, in order to be consistent with the non-path objects, I think we've already sunk this 
behavior into fairly firm ground here.  Unfortunately I think we're going to have to 
modify the broken Path objects to be consistent with this behavior since they are clearly 
now the "odd man out" in the compatibility equation...

import javafx.application.Application;
import javafx.stage.Stage;
import javafx.scene.*;
import javafx.scene.shape.*;
import javafx.scene.paint.*;
import javafx.scene.input.*;
import javafx.event.*;

public class Test extends Application {
public void start(Stage stage) {
Rectangle r = new Rectangle(50, 50, 200, 200);
r.setStrokeWidth(80);
r.setStroke(new LinearGradient(0, 0, 0, 1, true, CycleMethod.REPEAT,
   new Stop(0, Color.BLUE), new Stop(1, 
Color.RED)));

Scene scene = new Scene(new Group(r), 300, 300);
scene.addEventHandler(MouseEvent.MOUSE_PRESSED, new 
EventHandler() {
public void handle(MouseEvent e) {
scene.setFill(new Color(Math.random(), Math.random(), 
Math.random(), 1.0));
}
});
stage.setScene(scene);
stage.show();
}
}

...jim

On 11/18/2013 10:48 PM, Jasper Potts wrote:

I agree with Scott, it seems bad if gradient doesn't match stroke bounds when 
applied to stroke.

Jasper


On Nov 18, 2013, at 9:36 PM, Scott Palmer  wrote:

It wasn't clear from your original message that the stroke bounds were much 
more expensive. Given that additional info I don't think my idea is practical.  
That makes it harder to decide. I like your choice of fill bounds to ease the 
ability to align the gradients, but I don't like the idea of not being able to 
use a gradient that goes to the edge of the stroke.  Depending on the stroke 
width that could be a significant and very visible limitation.

I wonder how frequently aligning the gradient in the stroke and the fill comes 
up?  It seems using the stroke bounds for the stroke and the fill bounds for 
the fill is more correct, even if it makes aligning the gradients more 
difficult.  That alignment is nice, but having the gradient work consistently 
in terms of what 0% and 100% mean is probably important.

Scott


On Nov 18, 2013, at 8:17 PM, Jim Graham  wrote:

Hi Scott,

That's an odd take on it.  It wouldn't be readily obvious to a developer why 
their background rectangle had the gradient a little off if they never planned 
to ever stroke it.

Also, keep in mind that while it might be slightly more expensive to calculate tight bounds than 
loose bounds, it is MUCH more expensive to calculate stroke bounds on a shape.  It's not a trivial 
"OK, add a few pixels for the stroke" type of case, you have to trace out the path and 
compute the perpendicular extensions in many cases.  So, basically you are saying "always do 
the most expensive bounds operation for ever shape that is filled even if the extra work would 
never come into play"...?

   ...jim


On 11/18/2013 4:58 PM, Scott Palmer wrote:
I would lean towards using the stroke bounds for both.  Those being the "real" 
bounds, and resulting in less mis-aligned gradients.  Always calculate the stroke bounds 
as if the shape will be stroked, so it doesn't affect Canvas.
If you don't want that to affect the bounds used for a gradient fill when you 
aren't stroking, set the stoke width to 0.


Scott


On Nov 18, 2013, at 5:58 PM, Jim Graham  wrote:

Felipe mentioned recently that we encountered some issues in fixing a bug with 
SVGPath.

The outcome of this fix could be a significant change in how your proportional 
gradient fills look and so we'd like to get feedback on the various ideas.  You 
can read about them in that Jira issue, but I'll also summarize below.  
Discussion would probably be better on the mailing list, but we eventually need 
to work the salient points back into the Jira issue for future maintenance.

The basic issue is that we had a disagreement between the way that the shape 
caching code worked and the way that uncached shapes were filled with 
proportional paints.

First, there is the concept of tight and loose bounds.  Loose 

Re: discussion about touch events

[Jim Graham]

I'm only occasionally skimming this thread so I hope I don't disrupt 
discussions by throwing in a few observations now and then...

On 11/20/2013 7:30 AM, Assaf Yavnai wrote:

Pavel,

I think that this is a very good example why touch events should be processed 
separately from mouse events.
For example, if you will press a button with a touch it will remain in "hover" state 
although you released the finger from the screen. This is done because the "hover" state 
listen to the mouse coordinates, which is invalid for touch.
Touch events doesn't have the concept of move, only drag.


Some screens actually have a hover mode (the Galaxy Note series can detect both 
finger and stylus hover, but they use a Wacom-like sensor rather than the 
typical touch sensor).  In those cases, one could consider a hover mode to be 
similar to a mouse move event.

Even screens without hover detection have pressure detection and a light pressure below a 
configurable "touch" threshold could probably also be treated as hover as well?

Even with a mouse you have the concept of "the mouse has left the building" - consider if the mouse 
was moved to another screen for instance.  The loss of finger/stylus hover should probably be treated 
similarly, the location of the current "finger/mouse/pointer thing" is currently "nowhere near 
anything" when the touch implement leaves detection range...?

On the subject of detection of proximity of points to shapes, that is actually an area 
that I've tried to come up with good algorithms for the AA shaders and not had much great 
success (I've come up with various approximations, but they often tend to suffer from the 
points raised earlier about very wide and thin rectangles).  The CAG code of the 
java.awt.geom package (cloned into the FX source base) could potentially detect 
intersections between an oval and an arbitrary shape, but I'm not sure of the 
performance.  I know that I once found the AWT team using the Area code to do picking 
(with just rectangle shapes, though) until I explained to them that there are probably 
some more specific algorithms that would be an order of magnitude faster for the common 
case of rectangle-rectilinear math.  Still, they managed to use it just fine for a bit 
without noticing any performance issues.  The code base also has some fairly optimized 
"rectangle intersects arbitrary shape" code that!
 would b
e much faster than an Area.intersect(Shape) operation.  It might be generalizable to "oval 
intersects arbitrary shape" with some work, or perhaps "set of rectangles or polygons 
that approximate an oval intersects arbitrary shape".  I don't have time right now to look 
into that, but I'm throwing the suggestions out in case someone else is inspired...

...jim

Please evaluate draft fix for RT-33390

[Jim Graham]

A number of recent bugs have stemmed from a mistaken assumption in the 
D3D code that calls to update the render target would always cause a new 
render target to be installed which would clear the clip on D3D. 
Recently, that assumption started failing and sometimes the render 
target does not change and so the clip is not cleared.


The following patch was developed to try to fix these bugs, but I could 
not reproduce all of the original problems to know if this fix even 
targets all of these bugs.  I need the submitters of the various bugs to 
evaluate the patch to see if it fixes their problems:


webrev: http://cr.openjdk.java.net/~flar/RT-33390/webrev.00/

Should fix:

https://javafx-jira.kenai.com/browse/RT-34229
This was the original bug I was trying to fix and I've verified the 
patch fixes this bug.


https://javafx-jira.kenai.com/browse/RT-34159
It looks like Steve has already verified that it fixes this bug as well.

https://javafx-jira.kenai.com/browse/RT-33390
This is the bug that I could not reproduce to verify the fix.  I'd 
appreciate someone who saw the problem to verify the patch (attached to 
this Jira issue as a patch file).


Finally, some feedback on the ugly way that I pass through a boolean 
array to communicate the clearing of the clip back to the Java level. 
Is there another way to achieve this result in the D3D module?


...jim

review request RT-28691: Region corner radii scaling issues when radii > 2*width,height

[Jim Graham]

Hi David and Felipe,

Please review my proposed fix for: 
https://javafx-jira.kenai.com/browse/RT-28691


The webrev pointer, and a question, are in my recent comment there.

This makes every instance of corner radii handling I could find in 
Region and NGRegion internally consistent, enforced by pre-filtering the 
data through a single method in Region.


I am curious to hear from David about the interpretation I've made of 
various anomalous cases of corner radii, and Felipe from general 
graphics correctness of my calculations.  I mention a specific question 
I had while developing the fix in the Jira comment with the webrev.


I tested it with Ensemble8 and the test cases in the report.  I haven't 
written a specific test case that combines inset+percentage overflow 
until I hear back from David (or anyone) about what the correct 
interpretation is for that case...


...jim

Re: review request RT-28691: Region corner radii scaling issues when radii > 2*width,height

[Jim Graham]

Updated webrev available in the latest Jira comment, responding to 
review feedback from Felipe and (in the Jira comments) answering a 
number of questions about the methodology with David...


...jim

On 11/26/13 11:15 PM, Jim Graham wrote:

Hi David and Felipe,

Please review my proposed fix for:
https://javafx-jira.kenai.com/browse/RT-28691

The webrev pointer, and a question, are in my recent comment there.

This makes every instance of corner radii handling I could find in
Region and NGRegion internally consistent, enforced by pre-filtering the
data through a single method in Region.

I am curious to hear from David about the interpretation I've made of
various anomalous cases of corner radii, and Felipe from general
graphics correctness of my calculations.  I mention a specific question
I had while developing the fix in the Jira comment with the webrev.

I tested it with Ensemble8 and the test cases in the report.  I haven't
written a specific test case that combines inset+percentage overflow
until I hear back from David (or anyone) about what the correct
interpretation is for that case...

 ...jim

Post commit notice: RT-34635: warnings printed out when using poolstats

[Jim Graham]

The warning message was printed out unconditionally even when the 
summary was requested for other reasons...


http://cr.openjdk.java.net/~flar/RT-34635/webrev.00/
https://javafx-jira.kenai.com/browse/RT-34635

...jim

review 8.0 fix for RT-34663: pick more appropriate defaults for vram pool sizes

[Jim Graham]

Hi Lisa,

Here are the changes we discussed.  Note that I found the settings at 
different lines in the embedded configuration files than you indicated, 
but hopefully I modified all of the required platforms correctly...


Jira: https://javafx-jira.kenai.com/browse/RT-34663
webrev: http://cr.openjdk.java.net/~flar/RT-34663/webrev.00/

...jim

8u20 review request: RT-34854 - wrong stroke widths on texture-based primitives

[Jim Graham]

It's a simple fix.  I'm including Lisa on the reviewers because I'm 
surprised that this never showed up on embedded where the texture-based 
primitives are used by default...?


Jira: https://javafx-jira.kenai.com/browse/RT-34854
webrev: http://cr.openjdk.java.net/~flar/RT-34854/webrev.00/

Tested with the test cases included in the bug report with the 
appropriate flags (flags not needed on mobile)...


...jim

8u20 post commit review - RT-35089: fix poolstats output formatting

[Jim Graham]

I fixed some formatting issues with the poolstats output.  The diffs are 
in the bug report:


https://javafx-jira.kenai.com/browse/RT-35089

...jim

Re: JavaFX versus AWT/Swing Hardware Acceleration

[Jim Graham]

Some key points hidden in the shadows here...

We have direct rendering shaders for simple objects like rects, ovals, 
roundrects, simple single lines.  We can handle simple strokes on those 
with only the stroke width being customized (must use an expected join, 
cap, no dashing).


Text is done with a grayscale bitmap cache.  We also have LCD glyph 
caches as well on the GPU.  I'd have to check to see how much we cache 
stroked text vs. using a default shape fill.


These shaders can deal with most fills, except if you have too many 
colors in your gradients then we have to manually compute the colors, 
but I believe we still use a shader to merge the colors with the 
coverage and get the result on to the screen (I can dig in the code and 
check if it matters - I think the max colors are like 16).


Besides those, we render other things like Polygons, Polylines, Path, 
SVGPath using a native version of Pisces and caching the masks on the 
GPU.  The masks will be invalidated when the path changes in any way, 
and for any transform change other than a translate (I believe).  This 
is done as a grayscale texture and is separate from the kind of caching 
done when you select a Node's cache hint (that hint will result in a 
full-color cache that will be rendered with a simple blit operation and 
can cache effects and other node attributes in addition to just shape 
coverage).


On the table is for someone to create a mechanism similar to the NV_PATH 
rendering extension that lets us feed the path to the GPU and use 
stencils to render it at hardware speeds.  Unfortunately, the nVidia 
extension requires an nVidia GPU and only works on OpenGL, but I believe 
that much of that is due to them accelerating path stroking on the GPU. 
 We could probably at least use similar techniques to their extension 
to do fills and rely on our existing code to convert strokes to fill 
paths to couple with it.  Their techniques require certain stencil modes 
that are not universal so some investigation needs to be done to see how 
widely any of these techniques could be supported on either/both of D3D 
and OGL (and embedded).


Did you want a comparison to Java2D or were you more interested in just 
what FX can do?  I'd have to do some digging to figure out what parts of 
this aren't handled in J2D...


...jim

On 1/3/14 9:24 AM, Kevin Rushforth wrote:

A couple other thoughts.

1) JavaFX doesn't use software loops for the actual rendering as long as
the card is capable of supporting Prism. We do mix HW and SW in that we
generate masks from a path in SW, but we cache that on the card and
render it using shaders.

2) JavaFX can support Intel HD on Windows (something in the way Java2D
uses D3D exposes a bug in Intel's driver so it is disabled).

Jim can probably come up with more.

-- Kevin


Stephen F Northover wrote:

Hi Ryan,

I'll let others describe hardware acceleration in AWT/Swing but in
JavaFX, D3D and ES2 shaders are used to draw.  These run directly on
the graphics card and render data that is downloaded there.  Because
JavaFX has a scene graph, it has a notion of what has been changed and
can render only a subset of the scene graph nodes.

This is a very high level description of JavaFX but captures the
essence of what is happening.  Any other FX committers want to comment?

Steve

On 2014-01-01 1:39 PM, Ryan Cuprak wrote:

  What is the difference between hardware acceleration in JavaFX
versus Swing/AWT? I heard a while back someone claim that Swing/AWT
could never fully leverage hardware acceleration. However there are
the usual mix of -D parameters (sun.java2.opengl/sun.java2d.d3d,
etc.) for Swing/AWT. So I am just wondering how JavaFX’s acceleration
differs from the hardware acceleration in Swing. When I was giving a
talk at a JUG meeting someone called me out on the hardware
acceleration bit and I realized I didn’t fully understand the
differences between the two.

  I understand that JavaFX has a scene graph and Swing doesn’t etc.
So I am assuming that the scene graph operations are optimized on the
GPU whereas if you were trying to replicate a scene graph in Swing
you would obviously be doing all the work on the CPU. So Swing’s
hardware acceleration is more about buffers?

  I did come across this article:
https://weblogs.java.net/blog/opinali/archive/2010/05/03/first-long-look-javafx-13-and-prism

  Specifically this line caught my attention "Prism finally renders
effects with full hardware acceleration and without the extra buffer
copies that spoil effects on the Swing toolkit.”
  This article is a dated - brings up a second question about
hardware acceleration with a hybrid Swing/JavaFX application - what
happens?

Thanks,
-Ryan

Re: JavaFX versus AWT/Swing Hardware Acceleration

[Jim Graham]

Chien answered some of these, but here are my answers (inline)...

On 1/3/14 9:40 AM, Steve Hannah wrote:


Prism. We do mix HW and SW in that we generate masks from a path in SW,
but we cache that on the card and render it using shaders.



Can you describe roughly how the caching works?  I understand that the
alpha mask is stored in OGL as a texture, but there are an infinite number
of possible paths to be masked. How many textures can be stored at once (on
say an iPhone)?  I'm guessing JavaFX uses some sort of LRU caching
algorithm for these textures?


As Chien mentioned, Lombard/JDK8 has a new texture management component 
that lets us purge textures as needed on a LRU basis.  The caching tends 
to simply just validate the cached mask on every Shape node that has a 
cached mask on every frame and it sometimes finds that the texture needs 
to be recreated if we are running low.  Worst case, the cached texture 
needs to be recreated on every frame.



I ask this because I've been playing around directly with the Pisces C
library directly to generate alpha masks and render them using shaders.  I
can see a performance increase of 4x in using alpha masks rather than full
argb pixels (due to 1 byte per pixel instead of 4), but I'm curious about
other savings that JavaFX makes with this method.


Are you referring to the C version of Pisces in the FX sources? 
Actually, there are 2 currently in the FX source base.  There is one 
version (which has both a Java and native port) of the Pisces renderer 
that historically came from OpenJDK which got its version from an 
embedded port of Java that didn't use Ductus.  It is used by the D3D and 
OGL pipelines.  The second version came in when the SW pipeline was 
added recently, it went back to the original source base that had Pisces 
integrated with a full set of rendering loops and it brought that whole 
package in to be a complete rendering solution.  So there may be some 
divergence between the two versions of the rasterizer given the 
circuitous history of how the version used by D3D/OGL got into the FX 
source base and the fact that there was some work done on both versions 
since they diverged back in the OpenJDK early days.


I'd be curious to see your work and how it might help us to have some 
tighter integration between the rasterizers and the OGL/D3D shaders...


...jim

Re: Very poor performance of JavaFX on iPhone - 6 months later

[Jim Graham]

The following Jira is more precisely aimed at the scrolling 
optimizations that were disabled for Retina:


https://javafx-jira.kenai.com/browse/RT-27959

...jim

On 1/3/14 4:04 PM, Stephen F Northover wrote:

Hi Jeff,

Please add your weight to the JIRA (indicate the hardware you are
using).  A fix for the desktop should also fix the problem on iOS.

Steve

On 2014-01-03 5:45 PM, Jeff Martin wrote:

I noticed the high-dpi problem on a high-end MacBook Pro Retina. Took
me by surprise for JavaOne demos since I usually use an external
monitor (non-retina). I ended up switching to low dpi.

jeff


On Jan 3, 2014, at 3:59 PM, Stephen F Northover
 wrote:


Hi Tobias,

Sorry about that.

Looking at the bug, it seems to me that we have gotten to the bottom
of it.  Some iOS devices are scrolling fine while others are slower.
The difference seems to be that high dpi disables optimizations and
this causes the slowness on iOS.  The same optimizations are disabled
on the desktop, but the desktop is much faster and people don't
notice.  There was some discussion about performance in the
simulator, but we should ignore that. Performance on the device is
what matters.

iOS and Android are not currently supported platforms for JavaFX. We
are looking towards the community to step up and submit patches to
take these ports forward.  Johan Vos and others are helping with
Android.  Are you interested in working on iOS?  If so, please build
on the patches in https://javafx-jira.kenai.com/browse/RT-31453 and
take the work forward.

The first step would be to prove that we can be fast (which I think
we can if we run with the optimizations) then understand how to turn
the optimizations back on.

Steve

On 2014-01-03 3:28 PM, Tobias Bley wrote:

Hi,

many months ago I reported the „poor performance on iOS“ issue
(https://javafx-jira.kenai.com/browse/RT-31453). Now 6 months later
the bug is already open and no one of Oracle answers me on Jira.

What’s up? How can we fix this important bug?

Best,
Tobi

8u20 review request: RT-35209 - errors in the diagnostics in glContext.createProgram

[Jim Graham]

Chien, Felipe,

I've made some changes to the diagnostic code in glContext.c to help 
track down why WebLauncher is currently failing on Mac.  I need a review 
and perhaps some help making sure that the changes compile on other 
platforms...


Jira: https://javafx-jira.kenai.com/browse/RT-35209
webrev: http://cr.openjdk.java.net/~flar/RT-35209/webrev.00/

...jim

8u20 review request: RT-35210 - exceptions in WebLauncher on Mac

[Jim Graham]

Chien, Felipe,

Jira: https://javafx-jira.kenai.com/browse/RT-35210
webrev: http://cr.openjdk.java.net/~flar/RT-35210/webrev.00/

I will finish the cleanup of the "shader log" logic (RT-35209) as a 
follow-on fix after this is pushed...


...jim

FX 8u20 review request: RT-25249 ImageInput does not update for changes to WritableImage

[Jim Graham]

Jira: https://javafx-jira.kenai.com/browse/RT-25249
webrev: http://cr.openjdk.java.net/~flar/RT-25249/webrev.00/

The code was taken as a boilerplate from the ImageView code...

...jim

8u20 review request: RT-33294 - Canvas PixelWriter slow to fill canvas pixel by pixel

[Jim Graham]

Jira: https://javafx-jira.kenai.com/browse/RT-33294
webrev: http://cr.openjdk.java.net/~flar/RT-33294/webrev.00/

Tested using the submitted test case and then also using all of the 
Canvas toys in rt-closed...


...jim

[8u20] Review request: RT-13275 - 0 radius blurs produce low resolution output

[Jim Graham]

Kevin, Chien, Felipe,

I'm tagging 3 people on this review to hopefully get some more critical 
feedback mostly because the webrev is large, though most of the changes 
are simple due to some method signature changes...


Jira: https://javafx-jira.kenai.com/browse/RT-13275
webrev: http://cr.openjdk.java.net/~flar/RT-13275/webrev.00/

More information about the content of the changes appears in the latest 
comment in the Jira.


Compiled on Mac and Win7.
Test cases run on Mac with es2,sw.
Test cases run on Win7 with d3d,es2,sw.
Ensemble8 and some other samples tested on Mac.

...jim

8u20 review request: RT-35452 - Canvas does not allow missing moveto in paths

[Jim Graham]

webrev: http://cr.openjdk.java.net/~flar/RT-35452/webrev.00/
Jira: https://javafx-jira.kenai.com/browse/RT-35452

...jim

8u20 review request: RT-35058 - Zoomy gets texture lock errors on ARM

[Jim Graham]

webrev: http://cr.openjdk.java.net/~flar/RT-35058/webrev.00/
Jira: https://javafx-jira.kenai.com/browse/RT-35058

...jim

Re: Poor font rendering..

[Jim Graham]

If you look at the first "o", the version on top has a leading edge that 
is only 2 pixels wide and the one below is a dark middle column flanked 
by 2 lightly colored columns.  It's as if the placement on the lower 
text is trying harder to center its columns of sub-pixel samples to 
ensure that there is at least one column that gets hit by the darkest 
majority of the LCD sub-pixel weighted samples, but the one on top just 
distributes them as they arrive.  Spreading 6 samples, with the middle 
samples being the darkest, evenly over 2 pixels has much less contrast 
then distributing them with the darkest 3 samples in the middle on one 
pixel and the remainder lower-coverage samples distributed on the pixels 
to the left and right of the stem.


Felipe's recommendations for turning off sub-pixel shifting of the text 
may impact this if, for instance, the rasterizer returned LCD samples 
that were intended to start on an integer boundary so that all stems had 
their darkest parts on a single pixel, but then our placement algorithm 
decided to place the entire glyph on a 1/3 pixel alignment instead...


...jim

On 3/6/14 2:53 PM, Phil Race wrote:

I am not sure what you are looking at but I see 255,255,255 pixels on
all sides of
the stems. The stems are touching 3 pixels. I'm talking about the ones
to the sides of
those 3 pixel wide stems.

In any case I've used Windows 7 wordpad and Segoe UI 9pt (aka 12pixel) and
can see identical rendering to your Outlook case.

Wordpad (and so I infer Outlook) is using GDI which
1) is very likely a different rasteriser (FX is using the one from
DirectWrite)
2) doesn't do sub-pixel positioning because its only got int APIs.

So this seems to come down to DirectWrite vs GDI and personal
preferences ...

-phil.

On 3/6/2014 1:57 PM, Scott Palmer wrote:

That's not true.  There is a difference in the "white" space around
the letters.  The "white" pixel before the stem of the L is not 100%
white in either case and the difference is in line with what I would
expect if there was a sub-pixel shift..

Scott


On Thu, Mar 6, 2014 at 1:51 PM, Phil Race mailto:philip.r...@oracle.com>> wrote:

There really isn't any evidence of that. If it were true you'd see
the blending
into the pixels either side, but the pixels either side of the
stem are 100%
white in both cases. And examining the subpixels inside the
extremities of
the stem backs me up ...

-phil.


On 3/6/2014 10:40 AM, Scott Palmer wrote:

I think the stem of the L is colored differently because of
*sub-pixel* differences in its position.  I.e. it appears to
be at the same integer position, but it isn't at the same real
position.  It looks to me like that alone could account for
the differences.

Scott


On Thu, Mar 6, 2014 at 1:35 PM, Phil Race
mailto:philip.r...@oracle.com>
>> wrote:

Does the evidence really support that ?
You only need to look at the first letter "L". The stem is in
exactly the same place isn't it? And yet the colours are
different.

The overall length is different which I attribute to rounding
differences
or metrics differences used in accumulating the position
but that
is a guess.

-phil.


On 3/6/2014 10:25 AM, Scott Palmer wrote:

If you notice, in the images provided, the length of the
rendered text in pixels is significantly different
between the
two examples.  That supports the theory that it is
simply,
sub-optimal positioning of the glyphs that is
resulting in the
more pronounced LCD anti-aliasing.

Scott


On Thu, Mar 6, 2014 at 1:19 PM, Phil Race
mailto:philip.r...@oracle.com> >

8u20 review request: RT-36208: Exception using grayscale icon

[Jim Graham]

Simple cut/paste error fix:

Jira: https://javafx-jira.kenai.com/browse/RT-36208
webrev: http://cr.openjdk.java.net/~flar/RT-36208/webrev.00/

...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?) t

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: 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,

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

[9] Review request: 8091832: Provide API for getting the Screen scale on HiDPI screens

[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: [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: [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: CFV: New OpenJFX Committer: Murali Billa

[Jim Graham]

Vote: yes

...jim

On 3/31/16 2:04 PM, Kevin Rushforth wrote:

I hereby nominate Murali Billa [1] to OpenJFX Committer.

Murali 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 April 14, 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#mbilla

[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/a251a1d65932
http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/ecea43f5734c
http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/42b461505f27
http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/82ecaebd44cf
http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/8643ca988cef
http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/765fd07f22fc
http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/ae75f92d5e53
http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/25db4b2e47a1
http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/51c2129d282c
http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/cb8a24f5db2a

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&D 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]

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

[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: CFV: New OpenJFX Committer: Arunprasad Rajkumar

[Jim Graham]

Vote: yes

...jim


On 4/21/16 9:48 AM, Kevin Rushforth wrote:

I hereby nominate Arunprasad Rajkumar [1] to OpenJFX Committer.

Arunprasad 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 5, 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#arajkumar

[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/66eee7a12e81
http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/3f15f2e59063
http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/b75591ee263b
http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/84e09aadadf1
http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/4166cbca577f
http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/de87459ed168
http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/bccedad1f44b
http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/71a5499bcda4
http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/251198e174cd
http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/67a83054f43b

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] 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: 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

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: [graphics] [9] Review request for 8155903: Crash while running imported/w3c/canvas/2d.gradient.interpolate.overlap2.html

[Jim Graham]

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

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: 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: [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

[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: [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

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!

[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: [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

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

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

[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: [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 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]

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  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 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: 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] 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: [9] Code Review Request For 8157350: Encapsulate impl_ methods in Shapes related classes

[Jim Graham]

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?


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



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


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

[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] Review request for 8157600: Failed to launch hello.HelloSanity due to libglass.so: undefined symbol

[Jim Graham]

A new webrev is available now that hopes to make sure the new functions have C-style external dependency names 
regardless of how gio.h is included:


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

...jim

On 5/26/16 5:43 PM, Jim Graham wrote:

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

...jim

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

[Jim Graham]

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

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

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

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

[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: [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

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 
wrote:



Thanks. I added this to the bug report for https://bugs.openjdk.java.net/
browse/JDK-8161911

-- Kevin



Rahman USTA wrote:

Hello Kevin;

One of our user reported "Must be a memory leak somewhere" in AsciidocFX
project. It seems a similar issue.

You can see the issue here https://github.com/
asciidocfx/AsciidocFX/issues/227

Thanks.

2016-07-21 2:38 GMT+03:00 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 seconds, and memory
continues to
climb. On soft

[9] Review request for JDK-8160073: menu issues in case of two-display configuration

[Jim Graham]

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

The details are in the comments in the bug report.  To summarize:

- send a notification when DPI changes to reevaluate scene.getXY()
- correctly scale event screen coordinates for Mouse/Scroll/Gesture events
  (code is copied from Menu event code that was already doing it)

...jim

[9, 8u] Review request for 8087565: Scaling problem on OSX Retina

[Jim Graham]

Same patch applies to both (modulo source code paths).

jbs: https://bugs.openjdk.java.net/browse/JDK-8087565
8u webrev: http://cr.openjdk.java.net/~flar/JDK-8087565/webrev.8u.00/
9u webrev: http://cr.openjdk.java.net/~flar/JDK-8087565/webrev.9u.00/

This is a minor improvement on the earlier fix for JDK-8145516 to ensure that we communicate the scale of a newly mapped 
view to the FX code regardless of how the window/view are made visible...


...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: [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: 8166565 [hidpi] a flickering scrollbar when using Caspian style sheet

[Jim Graham]

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

I made the 6 new snapXY() methods in Region public for convenience here.  They are all new for 9 and had been protected, 
now they are public.


As noted in the bug report comment, there are a couple of places where I could not decide if the call should be for 
snapFooX() or snapFooY() and made a best guess and added a comment.  Please advise.


Also, advise if someone else should be CC'd on the review thread...?

...jim

Re: [9] Review request: 8166565 [hidpi] a flickering scrollbar when using Caspian style sheet

[Jim Graham]

An updated webrev is available, incorporating the feedback in the JBS comments:

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

...jim

On 9/27/16 9:09 PM, Jim Graham wrote:

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

I made the 6 new snapXY() methods in Region public for convenience here.  They 
are all new for 9 and had been protected,
now they are public.

As noted in the bug report comment, there are a couple of places where I could 
not decide if the call should be for
snapFooX() or snapFooY() and made a best guess and added a comment.  Please 
advise.

Also, advise if someone else should be CC'd on the review thread...?

...jim

Re: SVG

[Jim Graham]

Rather than try to hack around the exposure of the internal ImageLoader APIs, given the relatively small amount of glue 
code in that package it might be easier for them to reimplement the glue to use WritableImage...


...jim

On 10/11/16 5:36 AM, Kevin Rushforth wrote:

As for you other question, unless they use public API they will no longer work. 
Applications can export internal
packages via a command line switch, but it is quite fragile and not likely to 
work from release to release.

-- Kevin


Cirujano Cuesta, Diego wrote:

Hi all,

I was wondering why there is no javafx support to svg, is there any reason? I 
found nice the solution from codecentric
interesting: https://github.com/codecentric/javafxsvg. But it uses 
com.sun.javafx.iio.ImageFormatDescription,
com.sun.javafx.iio.ImageLoader, com.sun.javafx.iio.ImageStorage and 
com.sun.javafx.iio.ImageLoaderFactory. in openjfx9
is still there in com.sun... What's going to happen to these classes in Java 9? 
Is there strategy about providing ways
to use them?

Thanks!
Diego

[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

[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

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 compl

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 big

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]

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 

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: 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: 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: 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]

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]

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 :


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: [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]

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]

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: 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 th