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

8218973: SVG with masking is not rendering image with mask effect #213

Open
wants to merge 17 commits into
base: master
from

Conversation

@bhaweshkc
Copy link
Collaborator

bhaweshkc commented May 7, 2020

Root cause of issue is Specifying a image mask from GraphicsContextJava.cpp in WebKit was not implemented, so masking doesn't take place at all while rendering SVGRect. to fix this issue add implementation of function clipToImageBuffer() in GraphicsContextJava.cpp and send clip image to WCGraphicsPrismContext.java
While rendering in WCGraphicsPrismContext.java if image clip mask is available, use it for rendering using MaskTextureGraphics interface otherwise use usual way of rendering.


Progress

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

Issue

  • JDK-8218973: SVG with masking is not rendering image with mask effect

Download

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

@bridgekeeper
Copy link

bridgekeeper bot commented May 7, 2020

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

bhaweshkc added 2 commits May 7, 2020
@openjdk openjdk bot added the rfr label May 7, 2020
@mlbridge
Copy link

mlbridge bot commented May 7, 2020

Copy link
Member

kevinrushforth left a comment

I verified that this does fix the bug, although there is a resource issue you will need to address. When I run a simple test (the one attached to the bug report) I get the following warning messages:

Outstanding resource locks detected:
D3D Vram Pool: 21,073,412 used (3.9%), 67,108,864 target (12.5%), 536,870,912 max
13 total resources being managed
average resource age is 0.6 frames
0 resources at maximum supported age (0.0%)
5 resources marked permanent (38.5%)
3 resources have had mismatched locks (23.1%)
3 resources locked (23.1%)
8 resources contain interesting data (61.5%)
0 resources disappeared (0.0%)

You need to unlock() and dispose() the temporary RTTs when you are done with them. You should also dispose() the temporary Texture.

I left a couple inline comments as well, the main question being about whether we really need a second RTT for the mask texture.

@kevinrushforth
Copy link
Member

kevinrushforth commented May 16, 2020

/reviewers 2

@openjdk
Copy link

openjdk bot commented May 16, 2020

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

@kevinrushforth kevinrushforth self-requested a review May 16, 2020
Copy link
Member

kevinrushforth left a comment

The code looks good (with a couple minor formatting issues).

All of the onscreen testing I did looks good on Windows. I'd like to test it on Mac as well.

There is an issue with printing in the case of Hi-DPI scaling, which is what I run by default on Windows. The gradient texture appears to be scaled incorrectly (as if the scale was applied more than once). If I force scaling to 1 with -Dglass.win.uiScale=1 then it prints correctly.

@kevinrushforth
Copy link
Member

kevinrushforth commented Jun 12, 2020

It behaves the same on Mac with a Retina display as it does on Windows with Hi-DPI scaling. The gradient doesn't appear to be scaled correctly when printing. It's fine with on-screen rendering (with both HW and SW pipeline).

@bhaweshkc
Copy link
Collaborator Author

bhaweshkc commented Jul 21, 2020

Issue related to hidpi rendering was caused due to correct pixel scale factor not being set to the context in which mask texture was getting rendered. setting correct device scale factor in RenderSVGResourceMasker.cpp fixed the issue. Below images shows the rendered mask texture in both case (HiDpi and Normal respectively)

HiDpi_Mask_1
Normal_Mask_1

Another issue was in Hi DPI printing. PrintGraphics draws with different resolution than the mask texture. Before the fix mask was not drawn correctly to RTTexture, due to which only top left portion which comes inside current draw bounds was taken to draw the whole image. After fix, entire mask texture is always considered while doing final drawing.

@bhaweshkc bhaweshkc requested a review from kevinrushforth Jul 22, 2020
@kevinrushforth kevinrushforth requested a review from arun-Joseph Aug 7, 2020
Copy link
Member

kevinrushforth left a comment

While reviewing the most recent fix, I noticed that the call to setCTM in GraphicsContextJava.cpp was only done in the fillRect case. I then took a closer look at the change in WCGraphicsPrismContext.java and I see that the application of the mask is also only done for fillRect. A mask will still not be applied for filled rounded rectangles, filled paths, and all stroked primitives.

So this is an incomplete fix. I will add a couple additional test cases to the bug report.

@@ -232,6 +232,7 @@ void GraphicsContext::fillRect(const FloatRect& rect)
CompositeCopy);
} else {
if (m_state.fillGradient) {
setCTM(m_state.transform);

This comment has been minimized.

@kevinrushforth

kevinrushforth Aug 7, 2020 Member

Why is this needed here, but not in the other places setGradient is called? Won't there be a similar problem with strokeRect, fillPath, etc?

This comment has been minimized.

@kevinrushforth

kevinrushforth Sep 11, 2020 Member

This question is still outstanding. It seems like the call to setCTM is either needed before all of the setGradient calls or none of them. Can you comment?

This comment has been minimized.

@bhaweshkc

bhaweshkc Sep 11, 2020 Author Collaborator

i believe setCTM call is needed for none of them. it is a workaround i have added till i have more concrete fix. also please note that this workaround is needed only in cases where ui scaling is more than 1.

This comment has been minimized.

@bhaweshkc

bhaweshkc Sep 14, 2020 Author Collaborator

Removed the workaround and added right fix. setCTM call is not needed in any of the case.

@bhaweshkc bhaweshkc marked this pull request as draft Aug 17, 2020
@openjdk openjdk bot removed the rfr label Aug 17, 2020
@bhaweshkc bhaweshkc marked this pull request as ready for review Sep 8, 2020
@openjdk openjdk bot added the rfr label Sep 8, 2020
@kevinrushforth kevinrushforth self-requested a review Sep 8, 2020
@@ -442,6 +442,11 @@ public void setClip(WCRectangle c) {
(int)c.getWidth(), (int)c.getHeight()));
}

public void setClip(int cx, int cy, int cw, int ch, WCImage maskImage) {
setClip(new Rectangle(cx, cy, cw, ch));
state.setClipMaskImage(maskImage);

This comment has been minimized.

@kevinrushforth

kevinrushforth Sep 11, 2020 Member

Should all of the other variants of setClip (the ones that don't take a maskImage) set the clipMaskImage to null? Otherwise it seems that it might not be reset to null in all cases.

This comment has been minimized.

@bhaweshkc

bhaweshkc Sep 14, 2020 Author Collaborator

added setting of null to maskImage for all the overloads of setClip where mask image is not present.

Graphics g1 = imagePrDrawable.createGraphics();
state.apply(g1);
g1.setPaint(paint);
if(stroke != null) {

This comment has been minimized.

@kevinrushforth

kevinrushforth Sep 11, 2020 Member

Minor: there should be a space after the if.

This comment has been minimized.

@bhaweshkc

bhaweshkc Sep 14, 2020 Author Collaborator

Fixed

g1.setStroke(stroke);
}
if (isFill) {
g1.fill(shape);

This comment has been minimized.

@kevinrushforth

kevinrushforth Sep 11, 2020 Member

This will call the slower fill(Shape) method in all cases rather than the specialized fillRect or fillRoundRect method. Given all the other things that are done to draw a shape with a clip mask, I suspect that this is fine. One thing to consider is to pass in an enum instead of a boolean. The enum could say whether to use the specialized calls or the generic fill call. It's probably not worth the effort to make this change.

This comment has been minimized.

@bhaweshkc

bhaweshkc Sep 14, 2020 Author Collaborator

other than paths only RoundRectangle2D is passed to this function. Shape can be checked if it is an instance of RoundRectangle2D and faster draw api can be called. added the same along with enum private to this class.

submit(() -> {
final WebPage webPage = WebEngineShim.getPage(getEngine());
assertNotNull(webPage);
final BufferedImage img = WebPageShim.paint(webPage, 0, 0, 200, 200);

This comment has been minimized.

@kevinrushforth

kevinrushforth Sep 11, 2020 Member

This is added to the (preexisting) problem of calling paint on the wrong thread. In this case, it doesn't seem to cause any additional problems, and other tests in this same class do it, so we can fix this in the follow-on issue that is already filed, JDK-8252596.

This comment has been minimized.

@bhaweshkc

bhaweshkc Sep 14, 2020 Author Collaborator

moved tests to system test. also consolidated all tests into one.

Copy link
Member

arun-Joseph left a comment

The fix works when the shape is displayed initially on the screen, but fails when we scroll the image off-screen and then, back. To see the issue, you need to either rotate the gradient transform (by 90 degrees) or use a circle (any shape other than a rectangle) as the mask shape, as this bug can't be seen using a mask rectangle.

@arun-Joseph
Copy link
Member

arun-Joseph commented Sep 17, 2020

To reproduce the above issue, you can either modify <linearGradient id='Gradient' gradientTransform="rotate(90)"> or <mask id='Mask3'> <circle cx='100' cy='300' r="100" fill='url(#Gradient)' /> </mask> in svgMask.html and resize the window for scrolling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.