• Type: Bug
• Component: javafx
• Sub-Component: graphics
• Affected Version: fx2.0

• Priority: P3
• Status: Closed
• Resolution: Fixed

• Submitted: 2011-05-26
• Updated: 2015-06-16
• Resolved: 2013-11-08

JDK 8
8Fixed

http://hg.openjdk.java.net/openjfx/8/graphics/rt/rev/fbdf0e6d81f6 08-11-2013 Now that we have reorganized FX into a smaller number of modules, the visibility problems from comment Jun, 6 2013 06:49 AM are gone. However, the real fix probably involved moving the OverlayWarning code to Scene and synchronizing it when the Scene is synchronized. Note that the OverlayWarning FX code still cannot appear in the FX node tree and must remain hidden. 11-10-2013 Note that running a full-screen app with assertions enabled will produce warnings of the following form: $ java -ea helloworld.HelloFullscreen *** unexpected PG access java.lang.Exception: Stack trace at java.lang.Thread.dumpStack(Thread.java:1308) at javafx.scene.Node.impl_getPeer(Node.java:2223) at javafx.scene.Node.impl_updatePeer(Node.java:538) at javafx.scene.shape.Shape.impl_updatePeer(Shape.java:948) at javafx.scene.text.Text.impl_updatePeer(Text.java:1505) at com.sun.javafx.tk.quantum.OverlayWarning.warn(OverlayWarning.java:135) at com.sun.javafx.tk.quantum.WindowStage.applyFullScreen(WindowStage.java:580) at com.sun.javafx.tk.quantum.WindowStage.setFullScreen(WindowStage.java:616) ... 22-08-2013 This was not addressed in my ESC key rework, and needs to be reassigned to someone more familiar with the MT issues. 09-08-2013 Assign to David to take a look at since he is also looking at other full screen overlay issues. 16-07-2013 How to fix this and keep the idea used in the patch? When full screen is set in anywhere in the scene graph, we could ensure that their is a damaged node. To prove this, I used the following quick hack that makes HelloToggleFullScreen work on Mac: public final void setFullScreen(boolean value) { Toolkit.getToolkit().checkFxUserThread(); fullScreenPropertyImpl().set(value); if (impl_peer != null) { impl_peer.setFullScreen(value); getScene().getRoot().impl_transformsChanged(); // not right but this will force a redraw (also there is a bug: scene could be null) } } I am not a fan of this idea because it spreads the overlay code out even more. So, I'm leaning towards adding an overlay field to Scene and getting rid of all the custom overlay handling code in Quantum. Scene needs to understand that when it has an overlay it is damaged and should synchronize the overlay nodes. Since there are other changes happening around the full screen overlay code (ie. allow it to be turned off), this is a good thing to investigate, but using this approach may cause many more code changes that require coordination. 06-06-2013 Testing with HelloToggleFullScreen revealed problems on the Mac but not Windows. In the patch, in order to fix the MT problems for the overlay, access to the PG nodes was moved to the node synchronize phase for FX (which is locked using the render lock). Because of code visibility issues (Quantum cannot see UI classes such as Scene), Scene was changed to call Quantum with a runnable. The runnable (and overlay's access to PG nodes) was executed inside the render lock. This fixes the MT problems but has redraw problems. The problem with this fix is that it relies on damage in the scene in which the overlay should draw. Since the overlay itself is not actually part of the scene, any attempt to damage the overlay will not work. The overlay has a scene, but that scene is not connected to any window, so damaging the overlay is useless. On Windows, when the scene in which the overlay should draw goes full screen, there seems to always be a damaged node, which forces a synchronize and the patch works. On Mac, there is no equivalent damage. Since Quantum cannot see Scene, there is no way to force there to be a damaged node in the scene graph and hence force a synchronize. Quantum scenes are valiantly marked dirty to no avail. It's the FX scene graph nodes that need to be dirty or no synchronize will happen. The code that is currently released appears to be working but relies on the following: When PGNodes are updated from the overlay, there always seems to be damage in the scene graph on all platforms. This forces a synchronize and hence a the overlay nodes are drawn. 06-06-2013 Backed out the change set. There is a timing issue on Mac that needs more investigation. Flags were deleted from AbstractPainter that affected dirty ops. More investigation is needed. 05-06-2013 http://jfxsrc.us.oracle.com/javafx/8.0/scrum/graphics/rt/rev/4f4bc0128afe 05-06-2013 The attached patch fixes the MT problems. We should enter a follow on JIRA that describes how the overlay warning implementation should be improved so that animation can be supported. Do you approve the changes? 03-06-2013 I will leave that up to you to decide. But I don���t think this is a Major bug, the severity of the bug should be decreased. As the default message is not animated nicely but I don���t believe there is a serious MT issue here. I am also not thrilled leaving this hack in, but I don���t think it is very harmful either. What if application developer is hostile? Fullscreen and overriding overlay message should be privileged? 31-05-2013 I see. The idea is that we provide API to get rid of the default message (the API is protected by access controls). If the programmer wants his own message, he gets rid of ours and puts the nodes in his tree. Then he is not surprised when he sees them. In this case, we should fix the MT problems and ignore the fact that animation etc. doesn't work. The default warning is very simple and if a more complex message is needed, the application has a way to get rid of the default and implement his own (Dave Hill is working on this now). Agree? 31-05-2013 I haven't look at the patch, but wanted to comment on one point. he overlay warning must be hidden from the application (at least by default) for two reasons: 1) Security. It would defeat the security provided by the overlay warning if an untrusted application could get at the nodes or control their rendering 2) Confusion: An application that enumerates the scene, possibly using ScenicView, would transiently see nodes that it was not expecting and may not be prepared to handle. So it really does need to be some sort of "hidden" graph (although not necessarily at the toolkit level). 31-05-2013 This patch fixes the MT problems. I think the right answer is to add API to scene and allow the programmer to set the overlay. The default overlay would be the scene graph created by OverlayWarning. At that point, we could decide whether the nodes appear as children of the scene or not. If we go with this fix, the API that does TKScene API that does synchronization can be removed. Thoughts? Perhaps we should just fix the MT problems (the title of this JIRA) and enter a new JIRA for the enhancement. 31-05-2013 Is it critical that the nodes be hidden? At some point, we should have an API that lets the programmer put whatever UI he wants to indicate that full screen mode should exit. Why wouldn't we add that API and use it (with the correct permissions of course). 30-05-2013 RE Steve's (1) Yes you can do that, but this FX node(s) would have to be hidden from app developer. This seems like the simplest approach. 2. Current implementation short circuits the FX controlling animation, and attempts to do everything on render thread. I don���t consider this so much as a threading bug but a hack that does not work. Along with creating some animation which is not a part of pulse mechanism etc. I don���t think that approach can work well on the mac with out significant more work, and it contradicts how we do things. Render thread is a slave to FX. In fact I don���t consider this a threading issue, just a hack that does not work completely. Regarding layers, I don���t think it is necessary to uses layers in this case.(subscene provides something very similar, it has it���s own FBO (single layer render target)). Because it is a brief warning that goes away. Layers is usually something you want to use for the extent of the frame. Overlay media... At some point I had similar thoughts, on Mac we use CA Layers. Thus it occurred to me that Glass itself could handle overlay warnings, for example via a CA Layer. But after some further thought I dismissed that idea. Bottom line option 1 seems the best. 30-05-2013 1) Isn't OverlayWarning just FX code that can be moved into Scene? (ie. Kevin's option B) Can't we just provide the overlay root as the root for the synchronization rather than the actual root NGNode? 2) We have a render lock that locks render tree when it is being traversed. OverlayWarning runs in the UI thread so I imagine the MT thread is refering to when it accesses NGNodes. Can't we lock that access using the render lock to ensure that the UI thread and the Render thead don't step on each other? 30-05-2013 This might best be implemented by moving the functionality up to the Stage level, and utilizing the hardware "layers" API that we intend to add to render the overlay not in the same scene but in a higher layer on the stage. 01-08-2012 This is both too risky and not necessary for 2.1. It will need to be done no later than Lombard. 12-03-2012 Approved at 8/23 meeting 23-08-2011 SQE - ok to defer 23-08-2011 We probably want to target this to Lombard, and pull it into a Presidio Update release as a target of opportunity. 23-08-2011 This is a pretty significant scene graph change late in the release, and as the OverlayWarning is functional without the fade-out animation. Please target as deferral to Presidio-Update 22-08-2011 [Including some comments from an e-mail thread] The following in OverlayWarning.java is what needs to be addressed: // TODO - needs to be thread-safe - see RT-13813 text.impl_updatePG(); background.impl_updatePG(); root.impl_updatePG(); It probably needs to leverage the PG sync mechanism. Otherwise we won't get animation, in addition to not properly syncing at the right time, so this is likely a contributing factor to RT-13814. Creating a new Scene in createOverlayGroup() may also cause issues since there are places that expect the scene/stage to be 1-1. Two possible approaches that I can think of: A) don't create a new Scene in createOverlayGroup(); instead leave the overlayRoot "unattached" but set its "scene" field in the overlay node tree to the actual scene being rendered (using impl_setScene). B) refactor the code and move it up to javafx-ui-common I'd probably go for the first option initially, since it's easier; the second option will require an interface that allows passing in the overlay runnable to the Toolkit...much more work. The second may be a longer term goal (or may not be needed at all). 26-05-2011