Skip to content
This repository has been archived by the owner on Jun 24, 2021. It is now read-only.

JDK-8148129: Implement Accelerated composition for WebView #51

Merged
merged 20 commits into from
Sep 5, 2018

Conversation

arajkumar
Copy link
Contributor

@arajkumar arajkumar commented Apr 2, 2018

This pull request is related to https://bugs.openjdk.java.net/browse/JDK-8148129.

How does WebKit paint?

In WebKit, each HTML element on a web page is stored as a tree of Node objects called the DOM tree.

Then, each Node that produces visual output has a corresponding RenderObject, and they are stored in another tree, called the Render Tree.

Finally, each RenderObject is associated with a RenderLayer. These RenderLayers exist so that the elements of the page are composited in the correct order to properly display overlapping content, semi-transparent elements, etc.

It is worth to mention that there is not a one-to-one correspondence between RenderObjects and RenderLayers, and that there is a RenderLayer tree as well.

WebKit fundamentally renders a web page by traversing the RenderLayer tree.

What is the accelerated compositing?

WebKit has two paths for rendering the contents of a web page: the software path and hardware accelerated path.

The software path is the traditional model, where all the work is done in the main CPU. In this mode, RenderObjects paint themselves into the final bitmap, compositing a final layer which is presented to the user.

In the hardware accelerated path, some of the RenderLayers get their own backing surface into which they paint. Then, all the backing surfaces are composited onto the destination bitmap, and this task is responsibility of the compositor.

With the introduction of compositing an additional conceptual tree is added: the GraphicsLayer tree, where each RenderLayer may have its own GraphicsLayer.

In the hardware accelerated path, it is used the GPU for compositing some of the RenderLayer contents.

The accelerated compositing involves offloading the compositing of the GraphicLayers onto the GPU, since it does the compositing very fast, releasing that burden to the CPU for delivering a better and more responsive user experience.

Although there are other options, typically, OpenGL is used to render computing graphics, interacting with the GPU to achieve hardware acceleration. And WebKit provides cross-platform implementation to render with OpenGL.

How does WebKit paint using OpenGL?

Ideally, we could go from the GraphicsLayer tree directly to OpenGL, traversing it and drawing the texture-backed layers with a common WebKit implementation.

But an abstraction layer was needed because different GPUs may behave differently, they may offer different extensions, and we still want to use the software path if hardware acceleration is not available.

This abstraction layer is known as the Texture Mapper, which is a light-weight scene-graph implementation, which is specially attuned for an efficient usage of the GPU.

It is a combination of a specialized accelerated drawing context (TextureMapper) and a scene-graph (TextureMapperLayer):

The TextureMapper is an abstract class that provides the necessary drawing primitives for the scene-graph. Its purpose is to abstract different implementations of the drawing primitives from the scene-graph.

One of the implementations is the TextureMapperGL, which provides a GPU-accelerated implementation of the drawing primitives, using shaders compatible with GL/ES 2.0.

There is a TextureMapperLayer which may represent a GraphicsLayer node in the GPU-renderable layer tree. The TextureMapperLayer tree is equivalent to the GraphicsLayer tree.

Courtesy: https://blogs.igalia.com/vjaquez/2013/07/26/composited-video-support-in-webkitgtk/

JavaFX WebKit

Right now we are disabling Accelerated Composition feature due to missing platform implementation.

There are three possible ways to implement Accelerated Composition,

  1. Implement GraphicsLayer interface for JavaFX platform using FX Scene Graph API. (~100 methods need to implemented)
  2. Reuse texture mapper - Directly implemented on top of OpenGL [ES2] - Plug few texture creation APIs (~10)
  3. Reuse texture mapper - Implement TextureMapper and BitmapTexture interfaces for JavaFX.

Approach 1

It is going to be hard because of the amount of work involved in it. As of now Mac only platform reimplemented a GraphicsLayer for it's platform on top of CoreAnimation. QtWebKit folks attempted the same few years back, but they failed to do so due to scalability issues[1].

Approach 2

It is used by all WebKit2 platforms (GTK, EFL, Mac, AppleWin). It uses OpenGL[ES2] APIs directly. Few GraphicsContext3D methods need to be implemented to create Textures and FBOs.

Approach 3

TextureMapper and BitmapTexture interfaces need to be implemented on top of Prism's GraphicsContext

This pull request is based on Approach 3

Testing

Following web pages has CSS 3D demonstration,

https://webkit.org/blog-files/3d-transforms/poster-circle.html
http://is-real.net/experiments/css3/wonder-webkit/
https://webkit.org/blog-files/leaves
https://desandro.github.io/3dtransforms/examples/cube-02-show-sides.html
https://webkit.org/blog-files/3d-transforms/morphing-cubes.html
https://davidwalsh.name/demo/css-cube.php
https://www.smashingmagazine.com/2016/07/front-end-challenge-accepted-css-3d-cube

@arajkumar arajkumar added enhancement New feature or request WIP Work In Progress labels Apr 2, 2018
@arajkumar arajkumar requested a review from kevinrushforth April 2, 2018 09:15
@arajkumar
Copy link
Contributor Author

It is not yet ready for the merge, currently I'm doing few refactoring to avoid few internal class changes.

@arajkumar arajkumar changed the title JDK-8148129: Accelerated composition is not working JDK-8148129: Implement Accelerated composition for WebView Apr 3, 2018
@arajkumar arajkumar force-pushed the JDK-8148129 branch 2 times, most recently from e181eb1 to 1b73384 Compare April 24, 2018 11:14
@arajkumar
Copy link
Contributor Author

Yet to support D3D.

@arajkumar
Copy link
Contributor Author

Accelerated composition is implemented for both D3D & OpenGL 3D backends.

@arajkumar arajkumar removed the WIP Work In Progress label May 2, 2018
@@ -648,8 +648,7 @@ void ChromeClientJava::reachedApplicationCacheOriginQuota(SecurityOrigin&, int64
notImplemented();
}

#if USE(ACCELERATED_COMPOSITING)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this still be conditional, e.g. for platforms that don't support the required OpenGL extensions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the recent WebKit, ACCELERATED_COMPOSITING preprocessor define is removed and they made it as a runtime feature. In this patch, ACCELERATED_COMPOSITING enabled based on ConditionalFeature.SCENE3D runtime flag.

@johanvos
Copy link
Collaborator

johanvos commented Jul 2, 2018

It seems to me this PR can create a significant performance for WebView rendering, so if possible I think we want to target this for OpenJFX 11

@kevinrushforth
Copy link
Collaborator

It seems to me this PR can create a significant performance for WebView rendering, so if possible I think we want to target this for OpenJFX 11

This would be a good optimization to get in....I haven't had any time to look at it yet, so it is unlikely to be ready for RDP1. We would need to evaluate the risk of getting it in late, but it seems it might still be possible.

@kevinrushforth
Copy link
Collaborator

I took a quick look at this patch and the shared (non-WebKit) part looks OK to me. I'll want to do more testing to make sure that there are no regressions.

@ararunprasad How confident are you that this won't cause any regressions in WebView behavior (for D3D, ES2, and SW/J2D pipelines)? What performance analysis have you done? If accelerated composition is disabled, either because SCENE3D isn't supported or because we set the env variable, how easy it is to prove that the behavior is unchanged from before this fix? If you would still like to consider this for openjfx11, then you will need to rebase it on top of the latest develop branch (i.e., the 606.1 WebKit).

@arajkumar
Copy link
Contributor Author

Thanks @kevinrushforth for reviewing the changeset.

How confident are you that this won't cause any regressions in WebView behavior (for D3D, ES2, and SW/J2D pipelines)? What performance analysis have you done?
Arun: I did only basic testing. I'm will be doing extensive DRT execution after rebasing to 606.1.

If accelerated composition is disabled, either because SCENE3D isn't supported or because we set the env variable, how easy it is to prove that the behavior is unchanged from before this fix?
Arun: If the above conditions meet, we disable AC by using Settings::setAcceleratedComposition(false), it is equivalent to current behaviour.

If you would still like to consider this for openjfx11, then you will need to rebase it on top of the latest develop branch (i.e., the 606.1 WebKit).
Arun: yes, it will do it ASAP.

@@ -144,9 +146,11 @@
"com.sun.webkit.useJIT", "true"));
final boolean useDFGJIT = Boolean.valueOf(System.getProperty(
"com.sun.webkit.useDFGJIT", "true"));
final boolean useCSS3D = Boolean.valueOf(System.getProperty(
"com.sun.webkit.useCSS3D", Platform.isSupported(ConditionalFeature.SCENE3D) ? "true" : "false"));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be a good idea to introduce a new property here, that defaults to ConditionalFeature.SCENE3D but that can be used to override that value? If that property is then set to false (while SCENE3D might still be true), the webkit is initialised like before this patch, so performance should be the same.
I understand the intention of the patch is an acceleration, but such a property would allow developers to fallback to the "old" mode in case there turn out to be circumstances where the accelerated version has limitations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @johanvos for reviewing the patch.

Only the default value of "com.sun.webkit.useCSS3D" is based on ConditionalFeature.SCENE3D, user's option will be honoured if they override using '-D' switch.

I mean,
if user didn't set the switch, 3D is supported => AC will be enabled
if user didn't set the switch, 3D is not_supported => AC will be disabled
if user sets the switch false, 3D is supported => AC will be disabled
if user sets the switch false, 3D is not_supported => AC will be disabled

Always user provided option is honoured. Sorry if I'm not clear.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok, that's very clear -- I overlooked the point that com.sun.webkit.useCSS3D is indeed exactly the property I was looking for.

@@ -271,7 +271,7 @@ protected void updateShaderTransform(Shader shader, BaseTransform xform) {
}

scratchTx.set(projViewTx);
updateRawMatrix(scratchTx.mul(xform));
updateRawMatrix(scratchTx.mul(xform).mul(getPerspectiveTransformNoClone()));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you check the performance impact of this? I think it should be ok, as the implementation of mul will check if the passed matrix is the Identity, and in that case return immediately.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't explicitly check it, but I assumed it will be negligible because of early isIdentity() check.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a similar comment in the D3D code. It seems worth having an identity check for perspectiveTransform here.

@arajkumar
Copy link
Contributor Author

@kevinrushforth
As discussed, Accelerated Composition is disabled by default. Developers can enabled it using a Java command-line property com.sun.webkit.useCSS3D.

Copy link
Collaborator

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this is looking good. I added a couple suggested changes relating to checking perspTrans for identity.

I don't know WebKit well enough to do a detailed review of the native WebKit code, but it all looks reasonable.

I presume you've tested it on all three platforms with/without setting com.sun.webkit.useCSS3D and verified that a perspective transform is only ever set when the property is true?

I'll do some additional testing as well.

int res;
if (xform == null || xform.isIdentity()) {
if (xform == null || (xform.isIdentity() && perspectiveTransform.isIdentity())) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can perspectiveTransform be null?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can't be null. BaseContext.setPerspectiveTransform ensures that.

res = nResetTransform(pContext);
} else {
scratchTx.setIdentity().mul(xform).mul(perspectiveTransform);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend leaving the current code in an else if (perspectiveTransform.isIdentity()) since it will be most of the time. Something like this:

    final perspectiveIsIdentity = perspectiveTransform == null || perspectiveTransform.isIdentity();

    if (xform == null || (xform.isIdentity() && perspectiveIsIdentity)) {
        res = nResetTransform(pContext);
    else if (perspectiveIsIdentity) {
        // current code
    } else {
        // new code that does a .mul(perspective)
    }

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can skip the null check if you know that perspectiveTransform can never be null.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated to my previous comment, there is a potential bug here if xform is null and perspectiveTransform is not the identity. In this case, perspectiveTransform will be ignored, but perhaps that case can't happen?

Copy link
Contributor Author

@arajkumar arajkumar Aug 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it won't happen because BaseGraphics.setTransform family of function handles the null xform case.

@@ -271,7 +271,7 @@ protected void updateShaderTransform(Shader shader, BaseTransform xform) {
}

scratchTx.set(projViewTx);
updateRawMatrix(scratchTx.mul(xform));
updateRawMatrix(scratchTx.mul(xform).mul(getPerspectiveTransformNoClone()));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a similar comment in the D3D code. It seems worth having an identity check for perspectiveTransform here.

@Override
public String toString() {
String val = "WCTransform:";
if (m.length == 6) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: it might be cleaner having a boolean or enum indicating type rather than using the length of the array for this, but that could be cleanup done later.

@kevinrushforth
Copy link
Collaborator

All my testing so far (on Mac) looks good.

One more question: have you tried this using a PerspectiveCamera on the Scene? I'm curious as to how it would interact. Given that css3d is disabled by default, we could address this later if needed.

@muralibilla
Copy link

Tested CSS 3D use cases on Windows 7 and looks good..

@muralibilla
Copy link

Webkit changes looks fine.. +1

@arapte
Copy link
Contributor

arapte commented Aug 7, 2018

Graphics java changes look good to me. +1

@arajkumar
Copy link
Contributor Author

arajkumar commented Aug 7, 2018

@kevinrushforth . Thanks for reviewing the changeset.

One more question: have you tried this using a PerspectiveCamera on the Scene? I'm curious as to how it would interact. Given that css3d is disabled by default, we could address this later if needed.

IIRC, I tested this scenario. Scene's PerspectiveCamera effect was multiplied. I will ensure it once again.

@arajkumar
Copy link
Contributor Author

@kevinrushforth: Addressed your review comments in new commits. Please take a look. Thanks.

Copy link
Collaborator

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing my comments. I noticed one other issue. I don't know whether it can happen in practice, but if it does, then perspective transform would be ignored in some cases.

int res;
if (xform == null || xform.isIdentity()) {
if (xform == null || (xform.isIdentity() && perspectiveTransform.isIdentity())) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I note that if xform is null and perspectiveTransform is not the identity, the perspectiveTransform will be ignored (I missed adding this comment last time).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Kevin. xform will never be null. But better to handle that in a similar way like ES2Context.

Fixed the same in the new commit #6c935b8

Copy link
Collaborator

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All looks good now.

+1 pending an email with an RFR (for 12) to openjfx-dev mailing list.

@kevinrushforth
Copy link
Collaborator

Since this is an Enhancement request, please wait at least 1 day to give others a chance to comment before merging it.

When you do merge this PR, be sure to select "Squash and merge".

@nlisker
Copy link

nlisker commented Aug 25, 2018

I see many changes to javafx.graphics. Are these only relevant for Webkit?

@kevinrushforth
Copy link
Collaborator

Please file a follow-on JBS issue to turn this on by default (also be targeted for openjfx12). That future fix should accompanied by tests that will validate the accelerated composition.

@kevinrushforth
Copy link
Collaborator

I see many changes to javafx.graphics. Are these only relevant for Webkit?

Yes. The existing paths are preserved so that if the perspective transform is identity (which it will be unless using WebView with the "com.sun.webkit.useCSS3D" property set), the code is unchanged from current.

@arajkumar
Copy link
Contributor Author

Hi @kevinrushforth, Shall I merge the change to develop branch?

@kevinrushforth
Copy link
Collaborator

It hasn't yet been 1 (business) day since you sent the review; it would be best to wait until the end of the day today (Monday) Pacific time.

@kevinrushforth
Copy link
Collaborator

Please file a follow-up issue to turn on composite acceleration by default (when running a system that supports the SCENE_3D conditional feature), which we can do once there is enough testing of this feature. As part of this follow-up issue, the following should be addressed:

  1. Verifying this feature with a PerspectiveCamera
  2. Adding unit tests for this feature

I also noticed that the perspectiveTransform attribute isn't handled the same way as other attributes in the Graphics interface. There is no state stored in the Graphics,it is just passed on to the Context immediately when the attribute is set on the Graphics, nor is there any validation of the shader state when it changes. It would be best to address this in a follow-up issue, too (either as part of enabling it by default or in a second follow-up issue). Ideally transform state should be consistently kept in the Graphics object, and any change to that state should cause revalidation of the shader state. It may be that the current implementation won't cause problems given how WebView uses it (and given that WebView rendering is the only caller of that method).

@arajkumar arajkumar merged commit 9d16a90 into javafxports:develop Sep 5, 2018
@arajkumar
Copy link
Contributor Author

Created the followup bugs to address concerns raised by @kevinrushforth

JDK-8210398: Add unit test for Accelerated Composition
JDK-8210400: perspectiveTransform attribute must be handled in the same way as other attributes in Graphics interface
JDK-8210397: Verifying Accelerated Composition with a PerspectiveCamera

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants