JDK-8169270 : Leverage new Java2D Marlin rasterizer for JavaFX
  • Type: Enhancement
  • Component: javafx
  • Sub-Component: graphics
  • Affected Version: 9
  • Priority: P2
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2016-11-04
  • Updated: 2023-09-22
  • Resolved: 2016-11-18
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 9
9Fixed
Related Reports
Blocks :  
JDK-8092373 - Improve Path rendering performance
Relates :  
JDK-8177985 - Use Marlin renderer as the default FX rasterizer
Relates :  
JDK-8170029 - Comments in Stroker.computeOffsetQuad don't make sense (all 3 FX rasterizers)
Relates :  
JDK-8094493 - Need a more accurate center of pixel sampling for supporting 1 sample per pixel rendering
Relates :  
JDK-8131760 - JEP 265: Marlin Graphics Renderer
Relates :  
JDK-8170030 - Code in Marlin-based rasterizers may have an off-by-1 bug (causing extra work, no failures)
Relates :  
JDK-8170140 - Rendering anomaly with MarlinFX renderer
Relates :  
JDK-8170026 - Some tests in RegionBackgroundFillUITest fail with fractional window scale
Description
JDK 9 integrated a new rasterizer for Java2D called Marlin. That rasterizer is now the default shape scan-line converter for all of AWT/Swing/Java2D in JDK 9.

JavaFX uses its own pluggable rasterizer mechanism with a slightly different internal interface that is very close to the interface used by Java2D.  The changes to use Marlin from the JavaFX pipelines would be minor.

For JDK 9, Marlin should be added as a non-default alternate rasterizer for JavaFX using command line switches to enable it similar to how we control using the Java or native version of the existing JavaFX rasterizer (based on Pisces from the OpenJDK).
Comments
To follow-up on the test failures, I also thought we had a bug filed for the RegionBackgroundFillUITest. I'll look for it as well, and if I can't find one, I file it so we don't lose track of it.
18-11-2016
http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/904ac9ba4ddb
18-11-2016
Jim, please push the patch as I agree your last comment. Thanks for your reviews
18-11-2016
Actually, I'm going to reverse my last comment. Both of the formatting changes are consistent with the code that was copied from the Java2D repo (i.e. Marlin2D) so if those are changed, then they should be changed in a coordinated update to both versions of Marlin (Marlin2D and MarlinFX). I will be pushing the .2 patch based on: http://cr.openjdk.java.net/~lbourges/marlinFX/marlinFX-8169270.2-fx9.2/ as soon as my test build with the latest pulls from the 9dev repo completes...
18-11-2016
I thought we already had a bugid for the failing RegionBackgroundFillUITest tests on HiDPI, but I can't find it. I'm good with the latest patch modulo whether or not Laurent wants to make the formatting changes in Kevin's comment. Laurent?
18-11-2016
All my testing looks good. The modifications to the existing code looks good, and my sanity checking of the newly added files looks good. I have two minor comments, but they are non-blocking. Neither of these need to be fixed before the code can be integrated. 1. On Windows with 125% scaling we currently (with OpenPisces) have 6 failures in test.robot.javafx.scene.layout.RegionBackgroundFillUITest in the :systemTests project. When I enable Marlin, I see those same 6 failures, plus an additional two failing tests: RegionBackgroundFillUITest.testScenario3 RegionBackgroundFillUITest.testScenario4 All tests pass with Marlin at 100% scaling (and at 200% scaling which is what Mac Retina uses). 2. In my spot-checking of the newly added files, I noticed the following places where the initial curly brace should be on the same line as the class / method declaration rather than on a line by itself. modules/javafx.graphics/src/main/java/com/sun/marlin/MarlinRenderingEngine.java 40 public class MarlinRenderingEngine implements MarlinConst 41 { modules/javafx.graphics/src/main/java/com/sun/marlin/RendererStats.java 44 static RendererStats createInstance(final Object parent, final String name) 45 { There may be others, since I was just doing a cursory check. I approve the above .2 version of the webrev pending Jim's final approval. +1
18-11-2016
Great. The above version applies fine and I will finish my review / test this morning.
18-11-2016
Sorry for the previous webrev, here is a completely clean webrev (no reference to any previous file): http://cr.openjdk.java.net/~lbourges/marlinFX/marlinFX-8169270.2-fx9.2/
18-11-2016
The patch still doesn't apply. The options which make it easier for us to review, by seeing the changes against the original files, cause the patch to fail to apply. When I apply the patch either with "patch -p 1" or with "hg qimport ... ; hg qpush" it fails. Can you please either generate a "clean" webrev by running "webrev" with no options, or else generate a changeset patch by using "hg diff --git > marilin.patch" and upload that? Thanks.
18-11-2016
Kevin, As webrev.ksh script does not support cross-repository web revs (OpenJDK vs OpenJFX trees), I made my patches at an higher level: the client JDK9 (client folder) contains the OpenJFX9 forrest using the alias openfx9. Anyway here is the same patch based on latest OpenJFX9 repository (only): http://cr.openjdk.java.net/~lbourges/marlinFX/marlinFX-8169270.2-fx9/ Good sanity tests, Laurent
18-11-2016
The patch doesn't apply cleanly for me. Can you also provide an actual changeset patch (with git diffs), based against the tip of openjfx/9-dev/rt ?
17-11-2016
Here is the proposed webrev: http://cr.openjdk.java.net/~lbourges/marlinFX/marlinFX-8169270.2/ Changes: - OffHeapArray made completely package-private so the I revert the changes to getAddress() ie use directly the address field - extracted MarlinRasterizer.Consumer class to marlin.MaskMarlinAlphaConsumer - set the MaskMarlinAlphaConsumer type to the RendererContext.consumer field Finally Unsafe is now totally encapsulated into the com.sun.marlin package.
17-11-2016
If that can also mean that the entire OffHeapArray class can be made package private then that is one step even more secure...
17-11-2016
That makes sense, confine all uses of the UNSAFE to the marlin package. Does SWContext also need to move something there?
17-11-2016
- I missed something earlier. Please make the UNSAFE field in OffHeapArray private or package-private. You're right, I do not remind that point. As the MarlinRasterizer.Consumer class uses the OffHeapArray.UNSAFE field, I propose to move the MarlinRasterizer.Consumer class into the marlin package to be able to limit the visibility scope. Are you ok with that simple solution (that works) ? or do you have another proposal ?
17-11-2016
Updated webrev: http://cr.openjdk.java.net/~lbourges/marlinFX/marlinFX-8169270.1/ Here are my answers to your comments: PrismSettings.java: - Update Copyright to 2016. Fixed. SWContext.java: - Rename new inner class to DirectRTMarlinAlphaConsumer Fixed. - There is no check for Path2D in the renderShape method to use the Path2D specific setupRenderer? Fixed (but it was missing for OpenPisces). - (An aside comment...) Looking back on why xmin/xmax can't be optimized, it looks like the existing OpenPisces rasterizer has broken tracking of the min and max rendered alphas on each row (mea culpa)? OK, I noticed that; could you create a follow-up bug to fix the native-prism-sw for MarlinFX i.e. only process the range [pix_from, pix_to[ (I do not know how to explain it correctly) ? - The code that clears the last alpha after emitAndClear seems to clear 2 entries. Wouldn't there only be one clear needed (either [w] or [to])? Fixed, I had doubts how the native code works but now it is simpler: tested OK with doChecks=true Reentrant*.java: - move all of these to com.sun.util.reentrant Fixed OffHeapArray.java: - make the address field non-public with a getter to prevent any malicious setting from outside the file - Also set the address to 0 after freeing it to prevent access to memory not specifically allocated to the off heap array Both fixed. Stroker.java: - I noticed a reference to "sqrt" that hadn't been converted and wondered how it compiled until I noticed that it was in a big block of code that had been commented out. Not sure if it is worth updating it just for consistency or not, but I thought I'd point it out since I noticed it... Fixed. - The comments in computeOffsetQuad() talk about 4 points, but quads only have 3. I just backported these changes from OpenPisces.Stroker where the comments are wrong. Could you please fix comments in OpenPisces first or help me fixing them in Marlin as I am unsure what I should rewrite. RendererNoAA.java: - I scratch my head a little over the treatment of spminmax bounds in the endRendering() function, and it's related to the fact that I didn't think the original version in the AA renderer was written very clearly so I want to look at it some more... I tested with TestNonAARasterization (from JBS) and it is 'perfect' with +1: maybe the AA renderer may suffer some boundary issue ... 2nd comment: - I've completed my testing on Mac, Win10 and Ubuntu 14. - I found no regressions. Excellent! - Performance on microbenchmarks is greatly improved on all 3 platforms. In all cases Marlin was the fastest renderer. On micro-benchmarks the difference was usually pretty noticeable, 30-50% and in one case double. On a more complete scene graph benchmark the differences were closer together, but Marlin was still the fastest. - Of the above code review comments, the issues that should be addressed for the initial push are: - copyright in PrismSettings - packaging of Reentrant* - address protection in OffHeapArray, private/getter/set-to-0-on-free - comments updates in Stroker - clarify emitAndClear question above All are fixed except the 'comments updates in Stroker' as I am the best person to update them; please help ! - The name change and Path2D check in SWContext are optional. Fixed. - I've reviewed the endRendering() code in RendererNoAA and don't think there is a bug, but I may file a follow-on issue to either comment it better or to make some of the logic flow more consistently for future maintainability. As mentioned above, the TestNonAARasterization passed. Probably we should check also the AA renderer to have better consistency and maybe a simpler approach. - Of the above code review comments, the issues that should be addressed for the initial push are: - copyright in PrismSettings - packaging of Reentrant* - address protection in OffHeapArray, private/getter/set-to-0-on-free - comments updates in Stroker - clarify emitAndClear question above All are fixed except the 'comments updates in Stroker' as I am the best person to update them; please help ! - The name change and Path2D check in SWContext are optional. Fixed. - I've reviewed the endRendering() code in RendererNoAA and don't think there is a bug, but I may file a follow-on issue to either comment it better or to make some of the logic flow more consistently for future maintainability. As mentioned above, the TestNonAARasterization passed. Probably we should check also the AA renderer to have better consistency and maybe a simpler approach in a follow-up bug.
17-11-2016
Laurent - your last comment had a lot of duplicated content, as if you had hit "paste" twice by accident...? I missed something earlier. Please make the UNSAFE field in OffHeapArray private or package-private. comments in Stroker - OK for now RendererNoAA - my comments about endRendering() don't point to a problem, my only prediction is that we might be processing one extra pixel in some cases. This is definitely a follow-up issue and would apply equally to Marlin2D. I'm good except for the OffHeapArray.UNSAFE declaration.
17-11-2016
I'd like to do a quick review of this, too (just sanity checking).
17-11-2016
I've completed my testing on Mac, Win10 and Ubuntu 14. I found no regressions. Performance on microbenchmarks is greatly improved on all 3 platforms. In all cases Marlin was the fastest renderer. On micro-benchmarks the difference was usually pretty noticeable, 30-50% and in one case double. On a more complete scene graph benchmark the differences were closer together, but Marlin was still the fastest. Of the above code review comments, the issues that should be addressed for the initial push are: - copyright in PrismSettings - packaging of Reentrant* - address protection in OffHeapArray, private/getter/set-to-0-on-free - comments updates in Stroker - clarify emitAndClear question above The name change and Path2D check in SWContext are optional. I've reviewed the endRendering() code in RendererNoAA and don't think there is a bug, but I may file a follow-on issue to either comment it better or to make some of the logic flow more consistently for future maintainability.
17-11-2016
Comments on http://cr.openjdk.java.net/~lbourges/marlinFX/marlinFX-8169270.0/ PrismSettings.java: - Update Copyright to 2016. SWContext.java: - Rename new inner class to DirectRTMarlinAlphaConsumer - There is no check for Path2D in the renderShape method to use the Path2D specific setupRenderer? - (An aside comment...) Looking back on why xmin/xmax can't be optimized, it looks like the existing OpenPisces rasterizer has broken tracking of the min and max rendered alphas on each row (mea culpa)? - The code that clears the last alpha after emitAndClear seems to clear 2 entries. Wouldn't there only be one clear needed (either [w] or [to])? Reentrant*.java: - move all of these to com.sun.util.reentrant OffHeapArray.java: - make the address field non-public with a getter to prevent any malicious setting from outside the file - Also set the address to 0 after freeing it to prevent access to memory not specifically allocated to the off heap array Stroker.java: - I noticed a reference to "sqrt" that hadn't been converted and wondered how it compiled until I noticed that it was in a big block of code that had been commented out. Not sure if it is worth updating it just for consistency or not, but I thought I'd point it out since I noticed it... - The comments in computeOffsetQuad() talk about 4 points, but quads only have 3. RendererNoAA.java: - I scratch my head a little over the treatment of spminmax bounds in the endRendering() function, and it's related to the fact that I didn't think the original version in the AA renderer was written very clearly so I want to look at it some more... That's it for my read-through of the code. I'll work next on running some tests on all of the platforms I have, but that will likely be on Monday at this point as tomorrow is a holiday in the US. I wanted to get these review comments out tonight, though...
11-11-2016
FC Extension Request We propose this enhancement request for JDK 9. The proposal is to take the Marlin rasterizer, which was integrated into JDK 9 for use by Java2D in JEP 265, with a few bug fixes and changes needed to interface to the FX renderer, and integrate it in such a way that it is turned off by default. The key points are: * The code is nearly identical to Java2D version (so already been debugged) * Marlin will be added to JavaFX as optional rasterizer - Enabled only with command line switch * Existing code paths are unaffected (no SQE impact) ---- Risk: Low (no behavior change without a system property being set) Justification: Marlin is the default renderer in Java2D and this will allow developers to test it for use with JavaFX. We have seen significant performance improvements. It will give a head-start on getting this turned on by default in JDK 10. Estimated completion date: 2016-11-22
10-11-2016
Please review the marlin-FX webrev: http://cr.openjdk.java.net/~lbourges/marlinFX/marlinFX-8169270.0/ Reviewers: flar This webrev consist in Prism classes (ShapeUtil, SWContext) using the new Marlin renderer ported to JavaFX. The PrismSettings class has been modified to have the new prism.marlinrasterizer flag (disabled by default). Marlin-FX is slightly different than Marlin OpenJDK9 to: - use javafx classes instead of java2d classes - fix Dasher / Stroker as done in OpenPisces - support both AA and nonAA Renderer (see MarlinRenderer interface) - fix Renderer cubic decimation thresholds to match OpenPisces nonAA quality - the new MarlinAlphaConsumer supports optimized alpha copies (only valuable ranges) and the block flag optimized fills for the MarlinRasterizer only (not SWContext) Note: The SWContext.DirectRTPiscesMarlinAlphaConsumer relies on PiscesRenderer.emitAndClearAlphaRow() that is not optimal with the Marlin approach as it expects a complete row [x0, x1] although Marlin gives the span [xmin, xmax] where coverage != 0. The prism-native-sw (c code) could be improved to accept an extra offset to skip useless pixels [x0, xmin] and improve the blending performance. Will upgrade later OpenJDK9's Marlin renderer or if you prefer, I can do it first.
09-11-2016
Targeting to tbd_major for now (I moved it to 9 prematurely). When ready we might make a request to get it into 9 following the JDK 9 feature extension process.
04-11-2016