Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8196079: Remove obsolete Pisces rasterizer #268

Conversation

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Jul 18, 2020

This removes the obsolete OpenPiscesRasterizer (Java-based) and NativePiscesRasterizer implementations. The Marlin rasterizer was added in FX 9 and was made the default in FX 10. Marlin both outperforms Pisces and is more robust. There is no reason to keep the Pisces rasterizer(s) any more.

Note that the SW pipeline still has a Pisces-based renderer for the actual rendering of primitives. This is separate from the rasterizer and is not affected by this proposed fix.

I have tested this on Mac, Windows, and Linux.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

Reviewers

  • Phil Race (prr - Reviewer)
  • Laurent Bourgès (lbourges - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jfx pull/268/head:pull/268
$ git checkout pull/268

@kevinrushforth kevinrushforth self-assigned this Jul 18, 2020
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jul 18, 2020

👋 Welcome back kcr! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request.

@openjdk openjdk bot added the rfr label Jul 18, 2020
@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth commented Jul 18, 2020

/reviewers 2

@openjdk
Copy link

@openjdk openjdk bot commented Jul 18, 2020

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jul 18, 2020

Webrevs

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jul 18, 2020

Mailing list message from Michael Paus on openjfx-dev:

Am 18.07.20 um 17:10 schrieb Kevin Rushforth:

Note that the SW pipeline still has a Pisces-based renderer for the actual rendering of primitives. This is separate
from the rasterizer and is not affected by this proposed fix.

Just out of curiosity - why is this still needed? Wouldn't it be nice to
remove Pisces completely?

Michael

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jul 18, 2020

Mailing list message from Kevin Rushforth on openjfx-dev:

On 7/18/2020 8:50 AM, Michael Paus wrote:

Am 18.07.20 um 17:10 schrieb Kevin Rushforth:

Note that the SW pipeline still has a Pisces-based renderer for the
actual rendering of primitives. This is separate
from the rasterizer and is not affected by this proposed fix.

Just out of curiosity - why is this still needed? Wouldn't it be nice
to remove Pisces completely?

We still fall back to the SW pipeline for a few cases, such as virtual
environments without HW graphics support (Windows Remote Desktop,
VMWare, VirtualBox), systems with insufficient graphics HW or drivers
(mainly on Linux, but also on some Windows servers).

-- Kevin

prrace
prrace approved these changes Jul 18, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jul 18, 2020

Mailing list message from Michael Paus on openjfx-dev:

Am 18.07.20 um 17:57 schrieb Kevin Rushforth:

On 7/18/2020 8:50 AM, Michael Paus wrote:

Am 18.07.20 um 17:10 schrieb Kevin Rushforth:

Note that the SW pipeline still has a Pisces-based renderer for the
actual rendering of primitives. This is separate
from the rasterizer and is not affected by this proposed fix.

Just out of curiosity - why is this still needed? Wouldn't it be nice
to remove Pisces completely?

We still fall back to the SW pipeline for a few cases, such as virtual
environments without HW graphics support (Windows Remote Desktop,
VMWare, VirtualBox), systems with insufficient graphics HW or drivers
(mainly on Linux, but also on some Windows servers).

I think you misunderstood my question. Of course I know that JavaFX has
a SW pipeline which is used under certain conditions. My question was
why this SW pipeline still uses Pisces. The Marlin renderer has replaced
Pisces in all other areas, as far as I know. Why not also in the SW
pipeline. In how far are the requirements of the SW pipeline different
from the ones in AWT for example?

Michael

@bourgesl
Copy link
Collaborator

@bourgesl bourgesl commented Jul 18, 2020

That's the case, michael.
In SWContext, the Marlin renderer is called to rasterize any shape.

See https://github.com/openjdk/jfx/pull/268/files#diff-28f93b00dc9cba7197f0a6c2fef022ed

@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth commented Jul 18, 2020

I think you are conflating the rasterization step with the rendering step. In Prism the rasterization mainly includes clipping the shape and generating the mask data for the filled and/or stroked shape (the latter possibly being a wide and/or dashed stroke). The rasterized shape is then sent to the Prism GraphicsPipeline for drawing into the render target. This latter step is either done using shaders + D3D or OpenGL, or via the SW pipeline (or the J2D pipeline for printing). In the case of the SW pipeline, the renderer is based on code that was originally derived from Pisces, but it is distinct from the rasterization step that OpenPisces used to perform and Marlin now does perform. Take a look at the SW pipeline classes, the Java PiscesRendering classes, and the prism-sw native code.

@bourgesl
Copy link
Collaborator

@bourgesl bourgesl commented Jul 19, 2020

@kevinrushforth I asked skara JBS to associate my github account with my openjdk role...
I will approve your PR once I have proper credentials.

Good job, 10k LOC removed.

I wonder if I should also deprecate the Float-precision Marlin renderer to remove 10k lines too.
The Double-precision Marlin renderer was enabled as the default renderer since JDK10, so it is doing the job properly, no reason to have 2 variants anymore and it will ease the maintenance.

Laurent

@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth commented Jul 20, 2020

That's a good question about removing the single-precision float Marlin rasterizer. For desktop platforms, I see no reason to keep it. Perhaps Johan could weigh in as to whether there might be value in keeping it for some embedded platforms.

Copy link
Collaborator

@bourgesl bourgesl left a comment

Loiks good to me.

@openjdk
Copy link

@openjdk openjdk bot commented Aug 8, 2020

@kevinrushforth This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type /integrate in a new comment to proceed. After integration, the commit message will be:

8196079: Remove obsolete Pisces rasterizer

Reviewed-by: prr, lbourges
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /issue command.

Since the source branch of this PR was last updated there have been 14 commits pushed to the master branch:

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate af000b285a328f89258a699292e2ea245cf73cf6.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Aug 8, 2020
@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth commented Aug 10, 2020

/integrate

@openjdk openjdk bot closed this Aug 10, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Aug 10, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Aug 10, 2020

@kevinrushforth The following commits have been pushed to master since your change was applied:

  • af000b2: Merge
  • 5d34d72: 8245053: Keyboard doesn't show when TextInputControl has focus
  • 487854c: 8246343: Fix mistakes in FX API docs
  • fc38ce6: 8249647: Many classes in package javafx.beans.binding in module javafx.base have implicit no-arg constructors
  • a46b250: Merge
  • 926b5b6: 8249839: Cherry pick GTK WebKit 2.28.3 changes
  • 6b00892: 8250238: Media fails to load libav 58 library when using modules from maven central
  • aae8b6b: Merge
  • 3cc29e3: 8220484: JFXPanel renders a slanted image with a hidpi monitor scale of 125% or 175%
  • 5f60ea5: 8248381: Create a daemon thread for MonocleTimer
  • 9260fd1: Merge
  • f056d24: 8248908: Printer.createPageLayout() returns 0.75" margins instead of hardware margins
  • 5e7e452: 8245284: Update to 610.1 version of WebKit
  • 2f4666a: 8248490: [macOS] Undecorated stage does not minimize

Your commit was automatically rebased without conflicts.

Pushed as commit 5c596b1.

@kevinrushforth kevinrushforth deleted the 8196079-rm-pisces-rasterizer branch Aug 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants