JDK-8093185 : GaussianBlur always renders at 1:1 scale and may be unnecessarily blurry in a scaled scene
  • Type: Bug
  • Component: javafx
  • Sub-Component: graphics
  • Affected Version: fx2.0
  • Priority: P4
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2011-05-09
  • Updated: 2015-06-12
  • Resolved: 2014-02-25
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 8
8u20Fixed
Related Reports
Relates :  
Relates :  
Relates :  
Relates :  
Relates :  
Relates :  
Description
In the following code, the second button is blurred (and should not be?):

package test;

import javafx.application.*;
import javafx.scene.*;
import javafx.scene.control.*;
import javafx.scene.effect.*;
import javafx.stage.*;

public class BugBlur extends Application {

	public static void main(String[] args) {
		Launcher.launch(BugBlur.class, args);
	}

	public void start(final Stage stage) {
		Group group = new Group ();
		
		Button button = new Button ("TEST");
		button.setLayoutX(100);
		button.setLayoutY(100);
		button.setScaleX(2);
		button.setScaleY(2);
		group.getChildren().add(button);
		
		Button button2 = new Button ("TEST");
		button2.setLayoutX(100);
		button2.setLayoutY(150);
		button2.setScaleX(2);
		button2.setScaleY(2);
		group.getChildren().add(button2);
	    GaussianBlur blur = new GaussianBlur();
	    blur.setRadius(0);
	    button2.setEffect(blur);
		
		Scene scene = new Scene(group);
		stage.setScene(scene);
		stage.setVisible(true);
	}
}
Comments
Note that RT-2892 mentions using multiple Gaussian passes to simulate a larger radius Gaussian blur. With the fixes for RT-13275 we will use scaling and a Gaussian kernel of 127 values to achieve an arbitrarily large gaussian blur. At a kernel size of 127, it is not clear if you could measure the difference between "scaling and using a blur of size 127" and "using a blur of the proper size (>127) for the indicated device scale". In either case, it is unlikely the difference would be visually noticeable. In case someone finds a visual example where the "scale then blur with r=127" technique implemented here is less than visually optimal I am leaving this comment here to remind us of alternate solutions that were once proposed, but since disregarded...
18-06-2014

Pushed to the 8udev graphics scrum with the following changeset: changeset: 6366:c328c0a247e1 date: Tue Feb 25 11:53:01 2014 -0800 summary: Fix RT-13275: GaussianBlur with zero radius still blurs http://hg.openjdk.java.net/openjfx/8u-dev/rt/rev/c328c0a247e1 Tested with the test cases attached to this bug report at a variety of blur sizes and rotation angles.
25-02-2014

I made one last webrev before the push with the last few cosmetic changes (generics specification and import removal): http://cr.openjdk.java.net/~flar/RT-13275/webrev.03/
25-02-2014

The design looks good. I skimmed the webrev, but didn't do as extensive a review as Felipe did. I ran several tests and everything looks fine. +1
25-02-2014

The unused import in GS.java is gone now - I remember deleting an import when doing the <RenderState> pass so that must have been it. (Not planning on publishing another webrev for that, though.)
25-02-2014

Hi Jim, The latest webrev (v2) looks good to me overall. I was able to build and test on my Windows 8 laptop too. BTW, you might want to cleanup the unused import in GaussianShadow.java.
25-02-2014

I updated all of the subclasses of FilterEffect/CoreEffect to pass in RenderState for now. Since it doesn't affect building with the command line and since there are plenty of other warnings in this package, I'm going to stop there for now and leave the rest for a warning clean-up day. I won't submit a new webrev for the "<RenderState>" additions unless there is another problem that someone spots...
24-02-2014

Right, I get the list from warnings from the IDE. Not sure how to do it from the command line/gradle. Get the patch, it is easy enough for me to test.
24-02-2014

Some of the shaders use "getRenderState().specificSubclassMethod()" so that's why I used generics (to avoid ugly casts in the .jsl files). Since I think I had to modify every file that extended FilterEffect to change its "operatesInUserSpace()" method anyway, I'll look into adding the <RenderState>. BTW, where are these compiler warnings? I didn't see them...
24-02-2014

'old value not used' in com.sun.scenario.effect.
24-02-2014

Putting the generic added around 50 compiler warnings to com.sun.scenario.effect; The same package also has 57 warnings of 'The value of the local variable old..' I noticed you fixed a few in the webrev.
24-02-2014

Now that you explained I can't say that getState() and getRenderState() are bad names, Maybe rename getState() to getEffectState(), but not necessary. I'll leave it to your discretion. As for the generic, you can remove the warnings, for example, using: public class Crop extends CoreEffect<RenderState> to all subclasses. or you remove the generic and add a single cast from RenderState to LinearConvolveRenderState in LinearConvolveCoreEffect#filterImageDatas(). (i think)
24-02-2014

Hi Felipe, Thanks for looking at it. The old state objects are really attribute data, which is a form of state. The LCK used to also compute render-time data as well, but now I moved the render-time state data to a new object that handles state for a single rendering operation. So, perhaps the old state objects could be renamed to something more specific to "just holding on to the attributes" (i.e. AttrState?)? Or RenderState => RenderData? RenderInfo? The only classes that use RenderState for anything other than "which transforms to use" are the LinearConvolve classes [Gaussian,Box,Motion][Blur,Shadow]. Is there a way to express the generics on FilterEffect so that the other classes that have no subclass compile fine with just the base type "RenderState" assumed, or should I manually insert <RenderState> in all places?
24-02-2014

Here is a new webrev that fixes the "gradle test" failures: - Some of the getBounds() methods were using a null transform (we should eventually require all callers to pass in IDENTITY_TRANSFORM to avoid null checks), so the new GaussianRenderState constructors had to protect against a null transform. This turned up in running the automated tests, but not in running any actual FX code so it's not clear if those bounds methods are used at runtime, but we'll protect against nulls for now and do a proper search for null usage later. - Upgrading the Scenario BoxBlur interfaces to float (internally they have to support float because of transforms, but the API and constructors were all integers) caused some test failures that assumed they could do integer comparisons on the attributes. A more complete upgrade of the box blurs to float support in the constructors and setters will have to be done in a separate patch so as not to derail this fix to transform awareness. http://cr.openjdk.java.net/~flar/RT-13275/webrev.02/
24-02-2014

Hi Jim I did review the and test the code, worked for me. Unfortunately, I���m not familiar with code base or the nature of problem to give you any input on the over all design. That said, it all looks fine to me. A few comments: In the effect class hierarchy there is getState() and a getRenderState(). In LCCE, the state is a LinearConvolveKernel, and the render state is a LinearConvolveRenderState. The render state (LCRS), is actually stored inside of the state (LCK), basically a state with state. This is not a big deal, but it had me going in circles for a bit (especially first time browsing the code). You add a generic to FilterEffect class declaration, but only LinearConvolveCoreEffect (and descents) use it, on all other subclasses now have a ���raw type��� warning message. Do you actually need the return of getRenderState() as a specific type anywhere ?
24-02-2014

Webrev: http://cr.openjdk.java.net/~flar/RT-13275/webrev.00/ Some notes: - LinearConvolve effects used to forward the filter operation to the filter() method on LinearConvolveKernel - LinearConvolveKernel (and its subclasses) are the objects that maintain the "convolve kernel" state values for all of the Blur and Shadow effects - LCK.filter() used to use the LCK object to get various values about which kernel to apply and at what scales, etc. - Since LCK was "stateless" with respect to the filter operation, it was limited in how it could plan the kernel convolution operation The new mechanism: - LinearConvolveCoreEffect now handles the basic filter() algorithm for all blurs and shadows - LCCE first asks the LCK for a "RenderState" object which can plan how it will work for a given transform - The various LinearConvolveRenderState subclasses are now involved in all steps of the linear convolution and can plan accordingly and save their state - Note how the old LCK.filter() operation evolved and moved to LCCE.filter() - Note how a hodgepodge of "convolution attribute" getter methods on the LCK* classes evolved into a simpler set of methods on the LCRS* objects - Presently, the new RS object is mostly vestigial for any other filter operation, but other operations which have to supply calculated state to their effect peers will migrate towards it in the future. The webrev hits a ton of files, but most of the changes are very straightforward involving mostly 2 changes: - A new parameter was added to the filter() method that a ton of classes override. Most do nothing with the new parameter. - All FilterEffect subclasses used to statically declare "I work (or don't work) in User Space" which is now subsumed into the answers provided by RenderState. (As a result a lot of Effect classes no longer override "operatesInUserSpace()" and override "getRenderState(...)" instead
22-02-2014

Added a new test ShadowTest.java that is very similar to BlurTest, but tests the Shadow versions of the effects. The various buttons are: Row 1 - regular button Row 2 - gaussian shadow, drop shadow Row 3 - BOX_ONE, BOX_TWO, BOX_THREE shadows Note that the drop shadow version shows the original button plus an additional shadow offset down and to the right of it, but the other "shadow" buttons are essentially replaced by the shadow effect so all you see is a black or grey blob for those versions.
22-02-2014

I removed the prior version of BlurTest and replaced it with a new version (same name) that has a checkbox toggle that allows you to substitute a blue bg color for the buttons so that their blur effects are more visible (at large radii, the "mostly white" default colors of the buttons ended up getting so blurry that they were no longer visible). Checking the checkbox makes the blurred pixels more visible so you can see fringe effects, but leaving it unchecked uses a higher contrast visual that lets you evaluate the quality of the text at small blur sizes...
22-02-2014

Looks good. How about adding a HelloBlur example at some point?
20-02-2014

Attaching a new test case (BlurTest.java) that tests all of the blur operations with sliders to control horizontal and vertical dimensions of the blur, and a slider to rotate the test buttons. (A 4th slider is only for the angle on the MotionBlur instance). Top row is the normal button Middle row are the Gaussian and MotionBlur buttons Bottom row are the 1,2, and 3 step BoxBlur buttons
20-02-2014

I didn't mean to imply that the special case was the only fix - I was just mentioning that the obvious simple "fix", which is really a hack, wouldn't really fix this but might cover it up for the test case in question. A real fix, blurring the image starting from its intended resolution, would have no jumps and would preserve the image when radius=0. The hack special case wouldn't be that hard to add, but I really need to get going on reworking Decora to preserve resolution in more cases and so I'd rather bump up the priority on fixing it right than to put in a special case that would only work for the submitted test case. Note that we do, and may, intentionally downsample the image before blurring, but only for high blur radii so that we can use a smaller convolution kernel. In those cases, the lower resolution starting image cannot really be seen in the output because there is so much blurring. But, the way we implement that "downsample before blur" isn't generalized enough to provide an easy solution to the "low radii, but high scale" cases since it is very specific to "how many powers of 2 have you removed from the blur radius?" instead of "what arbitrary starting resolution did you use?". Still, it should provide a reasonable framework for doing the real fix by just generalizing that mechanism...
27-01-2014

So we can have a jump or we can have blur applied when one should not be. I'm still not sure why the original problem can't be fixed. The blur effect acts upon the source nodes using an identity transform (1x) scales up the result by 2x? At 1x the blur is gone (the bug doesn't happen). Shouldn't the scaled up result not have the bug?
27-01-2014

Is this happening because I has scaled the button and it would not happen otherwise? I'd be interested in seeing the special case and how odd it looked. Also, this is not a particularly critical bug because there is an easy work around (get rid of the effect). However, this causes a problem for Mike Hearn.
27-01-2014

Yep, I saw this on Retina displays. Nice to know it's not going to be visible for lots of users though. BTW I'm Mike not Mark :)
25-01-2014

The special case would work fine at 1:1 resolution, but if there is any scaling or if you are running on a retina display then you'd see the above blurriness (see the screenshots) just prior to the special case kicking in and then it would jump to the unblurred version right at the special case. I think the test case is using a scale of 2 so it is demonstrating exactly the effect on retina with no (added) scale...
25-01-2014

This is an issue with the various blur effects requesting their source nodes to render at Identity transform regardless of the current transform and then scaling the results according to the actual rendering transform they were given "after the fact". We could special case 0.0, but it would still look out of place when the radius was small and the node was being blurred by both the effect and the downsampling and there would be a sudden discontinuity when you jumped to a 0 radius.
24-01-2014

Attached a snapshot. Notice the bottom button has a GaussianBlur Effect with radius = 0.
23-01-2014

Attached a test program
23-01-2014

I have an app that blurs out the GUI behind overlay UIs, and the blur is animated. It looks OK but this bug means that there's a noticeable jump at the end of the animation as I delete the blur effect, due to the fact that you can't animate away the blur entirely.
22-09-2013