Re: Fwd: Re: Marlin-Renderer and JavaFX
There are basically 2 isolated changes to the existing code base and then a set of added source files. The first change is to use Marlin if the appropriate property is specified, and those changes are very localized and easy to verify that they won't hurt anything. The second change is to modify the growth pattern of Path2D. While these changes are live in AWT already and have already been code reviewed, it would probably be better to submit them as a separate FX fix if they are only performance oriented and not strictly required for Marlin to function. That way we compartmentalize anything that could possibly result in a regression into a separate bugid so we don't have to pull everything if someone complains that the new growth pattern is having negative consequences for their app. I doubt that will happen, but it is simple enough to break them into 2 separate fixes so it couldn't hurt to do that. After that, this just boils down to adding a bunch of code that has already been vetted elsewhere and a small and simple change to use it only optionally and conditionally, which is a very low risk fix. That would take this change from "no obvious drawbacks" to "obviously no drawbacks" (or, more precisely, one "obviously no drawbacks" fix and one related "no obvious drawbacks" fix)... ...jim On 11/2/2016 2:54 PM, Laurent Bourgès wrote: Jim, Here is an updated patch for MarlinFX: http://cr.openjdk.java.net/~lbourges/marlinFX/marlinFX-s02-ofx9/ I made big improvements: MarlinFX is now "feature-complete": - Added MarlinAlphaConsumer & MarlinRenderer interfaces to define new methods on AlphaConsumer and common methods between AA & noAA renderers - Renderer: fixed cubic dec/inc lengths (like openpisces) + use optimized copyAlphas from MarlinAlphaConsumer (with block flag optimization derived from former MarlinCache) - RendererNoAA: optimized Renderer for non-antialiasing (tested OK) - Dasher & Stroker: backported changes from openpisces (small dash phase & refined cubic / quad mitters) - Added MarlinPrismUtils & MarlinRasterizer classes to wrap the Marlin renderer as a JavaFX ShapeRasterizer and implement the MarlinAlphaConsumer efficiently (mimics former MarlinCache ie support the block flag optimization); MarlinPrismUtils performs properly NaN / Infinity coordinates filtering and use the same pipeline stages (delta / invDelta transformers for Dasher/Stroker) like in the MarlinRenderingEngine - Thread safety: MarlinRasterizer completely thread-safe (for future multi-threaded rendering) using ReentrantContext... - Modified (OpenJFX) ShapeUtil to use the MarlinRasterizer instead of the OpenPiscesRasterizer class (use -Dsun.javafx.marlin=true to enable Marlin-FX) - Fixed Path2D growing algorithm (like java2d) So MarlinFX is 13K LOC (few unused classes could be removed soon) and only few lines are added in ShapeUtil to switch MarlinFX ON: if (PrismSettings.doNativePisces) { shapeRasterizer = new NativePiscesRasterizer(); } else { *// TODO: move that new setting into PrismSettings: // Enable Marlin-FX by setting -Dsun.javafx.marlin=trueif (MarlinProperties.isMarlinEnabled()) { System.out.println("Marlin-FX[" + Version.getVersion() + "] enabled.");shapeRasterizer = new MarlinRasterizer();} else {*shapeRasterizer = new OpenPiscesRasterizer(); *}*} So the OpenPisces classes are totally left unchanged and MarlinFX is just added as another rasterizer and is enabled with the following settings: -Dsun.javafx.marlin=true and -Dprism.nativepisces=false Of course, we could adapt these few lines to satisfy your requirements (system properties ...); please tell me what you prefer. I tested this new release with DemoFX, Guimark HTML5, Ensemble8 and everything is working fine. Does it look acceptable as a low risk RFE ? Finally what do you prefer for OpenJDK9 integration ? - as a new javafx rasterizer (like the provided marlinFX patch) - or as a javafx wrapper using OpenJDK9's marlin renderer (java2d) ? I wonder if it would be better to create another JavaFX ShapeRasterizer that wraps OpenJDK9 Marlin (java2d) to minimize the code duplication but it will add some complexity in the marlin renderer (itself) to introduce the AlphaConsumer interface... I can make separate patches for all these changes concerning jfx Path2D or Marlin changes for OpenJDK9 first, then MarlinFX. PS: MarlinFX is based on my (internal) Marlin-renderer 0.7.5 (a bit different than OpenJDK9's Marlin as it is compatible with jdk 1.8) so I will synchronize both branches to provide soon MarlinFX9 patches closer to OpenJDK9's Marlin renderer if we are going to work on its integration. PS2: I will also work on another MarlinFX variant not using Unsafe but only plain arrays to evaluate the performance loss (small) that could simplify the integration with Jigsaw ... So I made a
Re: Fwd: Re: Marlin-Renderer and JavaFX
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
Re: Fwd: Re: Marlin-Renderer and JavaFX
Hi Laurent, Great progress. I agree with Sergey about avoiding unconditional logging, and about wanting a different name for the property (work with Jim on the property name). To answer a couple other questions: Does it look acceptable as a low risk RFE ? I would need to see the final diff against FX 9-dev, but if the only change to the current control flow in the existing logic is the additional "if" test for the existence of the new property, and if the Marlin code base itself is basically a copy of what is in Java2D, then I think it is low-risk enough to propose it. I can't guarantee that it will be accepted. 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) ? The former. It will add risk and complexity to wrap the version of Marlin in Java2D for JDK 9. I can make separate patches for all these changes concerning jfx Path2D or Marlin changes for OpenJDK9 first, then MarlinFX. Please do file a new JBS bug for Path2D and let's fix that separately. As a bug fix it will not need to go through the feature request extension process. 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. We will need a patch based on FX 9 before we can propose this for 9, so this sounds like a good plan. 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 ... I recommend whatever is easiest to get to a patch that will apply to FX 9-dev and build using JDK 9. -- Kevin 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