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

8273355: Lanai: Flickering on tooltip appearance IntelliJ IDEA 2021.2.1 #5373

Closed
wants to merge 2 commits into from

Conversation

@avu
Copy link

@avu avu commented Sep 4, 2021

Used setOpaque() method to set correct background of platform window


Progress

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

Issue

  • JDK-8273355: Lanai: Flickering on tooltip appearance IntelliJ IDEA 2021.2.1

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5373/head:pull/5373
$ git checkout pull/5373

Update a local copy of the PR:
$ git checkout pull/5373
$ git pull https://git.openjdk.java.net/jdk pull/5373/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5373

View PR using the GUI difftool:
$ git pr show -t 5373

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5373.diff

Used setOpaque() method to set correct background of platform window
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 4, 2021

👋 Welcome back avu! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

Loading

@openjdk openjdk bot added the rfr label Sep 4, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 4, 2021

@avu The following label will be automatically applied to this pull request:

  • awt

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

Loading

@openjdk openjdk bot added the awt label Sep 4, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 4, 2021

Webrevs

Loading

if (visible) {
// Set correct background for a window before making it visible
platformWindow.setOpaque(!isTranslucent());
}
Copy link
Contributor

@prrace prrace Sep 4, 2021

Choose a reason for hiding this comment

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

This looks as if it will affect OpenGL as well as Metal.
Do you see the flckering with both pipelines ?
What testing have you done with both of them ?

Loading

Copy link
Author

@avu avu Sep 5, 2021

Choose a reason for hiding this comment

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

Yes, it affects both pipelines, but flickering was noticed only with Metal. I've tried IDEA on both pipelines and I've run desktop tests only with Metal. I'm going to run desktop ones with OGL too.

Loading

Copy link
Author

@avu avu Sep 9, 2021

Choose a reason for hiding this comment

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

OGL desktop tests are OK.

This looks as if it will affect OpenGL as well as Metal.
Do you see the flckering with both pipelines ?
What testing have you done with both of them ?

Loading

Copy link
Member

@mrserb mrserb Sep 11, 2021

Choose a reason for hiding this comment

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

If the user will create a peer then change the background color, then show the window the flickering will occur. you need to override the LWWIndowPeer.setBackground and call the platformWindow.setBackground from there. The LWWIndowPeer.setBackground is also called during peer initialization so the code above will not be needed.

But it will be good to root cause the problem if it is not related to the performance.

Loading

@mrserb
Copy link
Member

@mrserb mrserb commented Sep 4, 2021

Calling "platformWindow.setOpaque(!isTranslucent());" is equvivalent of calling these three methods:

  setOpaque(getTarget().isOpaque());
  applyShape(Region.getInstance(shape, null));
  peer.setTextured(IS(TEXTURED, styleBits));

All of them are executed before "platformWindow.setVisible(visible);" why do we need one more call to trigger updateOpaque?

Loading

@mrserb
Copy link
Member

@mrserb mrserb commented Sep 4, 2021

Is it possible that the problem is in the performance, what happens if you just add a delay at that place to wait while the metal layer will be updated from the previous calls to updateOpaque?

Loading

@avu
Copy link
Author

@avu avu commented Sep 5, 2021

Calling "platformWindow.setOpaque(!isTranslucent());" is equvivalent of calling these three methods:

  setOpaque(getTarget().isOpaque());
  applyShape(Region.getInstance(shape, null));
  peer.setTextured(IS(TEXTURED, styleBits));

All of them are executed before "platformWindow.setVisible(visible);" why do we need one more call to trigger updateOpaque?

The flickering is only noticed with some components and adding setOpaque (that sets correct background color inside) definitely helps. I'll check more carefully all the paths.

Loading

@avu
Copy link
Author

@avu avu commented Sep 5, 2021

Is it possible that the problem is in the performance, what happens if you just add a delay at that place to wait while the metal layer will be updated from the previous calls to updateOpaque?

I'll check it.

Loading

@avu
Copy link
Author

@avu avu commented Sep 9, 2021

Is it possible that the problem is in the performance, what happens if you just add a delay at that place to wait while the metal layer will be updated from the previous calls to updateOpaque?

I'll check it.

I did some more investigation and the answer is no, it's not a performance issue. The problem is that we never set the opacity to the platform window on IDEA tooltip appearance because of caching code in LWWindowPeer.setOpaque() that is called from LWWindowPeer.initializeImpl. So, I think that a direct setting in the place that I've suggested is necessary.

Loading

@mrserb
Copy link
Member

@mrserb mrserb commented Sep 9, 2021

I did some more investigation and the answer is no, it's not a performance issue. The problem is that we never set the opacity to the platform window on IDEA tooltip appearance because of caching code in LWWindowPeer.setOpaque() that is called from LWWindowPeer.initializeImpl. So, I think that a direct setting in the place that I've suggested is necessary.

There is no caching in the LWWindowPeer.setOpaque() it just do not call the updateOpaque() if the opaque property did not change. What you are want to do is to call the CWrapper.NSWindow.setBackgroundColor from the LWWindowPeer.setBackground(). So the native color will be in sync with the peer's color. That method actually was added as part of JDK-8033786 and removed in JDK-8253977.

I think that the root cause is in the performance, because the first thing we draw is the surface data which is initialized/filled by the background color. Probably in OGL it works fine because we draw mostly immediately, and in case of metal we are waiting for DisplayLink callback?

Loading

@openjdk openjdk bot added the client label Sep 10, 2021
Extracted setting background to the particular method in PlatformWindow. Provided appropriate implementation in CPlatformWindow.
@avu
Copy link
Author

@avu avu commented Sep 10, 2021

There is no caching in the LWWindowPeer.setOpaque() it just do not call the updateOpaque() if the opaque property did not change.

Yes, I meant just that - cached state.

I think that the root cause is in the performance, because the first thing we draw is the surface data which is initialized/filled by the background color. Probably in OGL it works fine because we draw mostly immediately, and in case of metal we are waiting for DisplayLink callback?

Yes, to blit the content we just start DisplayLink. So, definitely there is some lag between window appearance and the drawing of the layer.

What you are want to do is to call the CWrapper.NSWindow.setBackgroundColor from the LWWindowPeer.setBackground().

Yes, it was the purpose of use setOpaque() method. But, more verbose way with a separate method (setBackground) is better. I've prepared a new version with such a change.

Loading

@mrserb
Copy link
Member

@mrserb mrserb commented Sep 10, 2021

Yes, to blit the content we just start DisplayLink. So, definitely there is some lag between window appearance and the drawing of the layer.

If that lag is visible by the human, and so different from the OGL, means that we should enhance our performance testing for Lanai. As far as I remember that tests report better performance than OGL, is it possible that for Metal the perf tests just skip the rendering phase and did not wait for a DisplayLink callback?

Probably we can force metal to draw some initial surfaces w/o using DisplayLink?

Loading

@avu
Copy link
Author

@avu avu commented Sep 10, 2021

As far as I remember that tests report better performance than OGL, is it possible that for Metal the perf tests just skip the rendering phase and did not wait for a DisplayLink callback?

I'm not sure about Swingmark but RenderPerf tracks the real content rendering (there are color marks for each frame that are rendered after the bunch of primitives and the java robot detects the change of the color marks). So, we can trust the numbers in the RenderPerf.

Probably we can force metal to draw some initial surfaces w/o using DisplayLink?

It could work but complicates things a bit. I'll see what we can do here. Meanwhile, I'll provide the solution that we discussed earlier.

Loading

@mrserb
Copy link
Member

@mrserb mrserb commented Sep 11, 2021

I'm not sure about Swingmark but RenderPerf tracks the real content rendering (there are color marks for each frame that are rendered after the bunch of primitives and the java robot detects the change of the color marks). So, we can trust the numbers in the RenderPerf.

Then why it does not catch this one? If it works fast then probably we just draw some wrong texture in the beginning or draw some garbage?

Loading

@avu
Copy link
Author

@avu avu commented Sep 12, 2021

I'm not sure about Swingmark but RenderPerf tracks the real content rendering (there are color marks for each frame that are rendered after the bunch of primitives and the java robot detects the change of the color marks). So, we can trust the numbers in the RenderPerf.

Then why it does not catch this one? If it works fast then probably we just draw some wrong texture in the beginning or draw some garbage?

The scenario from RenderPerf differs from this issue. In RenderPerf we create a window and then perform rendering of multiple primitives. Whereas in this bug we create a new window each time and see the flickering.

Loading

@mrserb
Copy link
Member

@mrserb mrserb commented Sep 12, 2021

The scenario from RenderPerf differs from this issue. In RenderPerf we create a window and then perform rendering of multiple primitives. Whereas in this bug we create a new window each time and see the flickering.

But the code path is the same, we create a window and try to clear/fill it by the background as fast as possible. And the flickering occurs because the time between g.fillRect()->actual image on the screen is too big.

Loading

@avu
Copy link
Author

@avu avu commented Sep 13, 2021

The scenario from RenderPerf differs from this issue. In RenderPerf we create a window and then perform rendering of multiple primitives. Whereas in this bug we create a new window each time and see the flickering.

But the code path is the same, we create a window and try to clear/fill it by the background as fast as possible. And the flickering occurs because the time between g.fillRect()->actual image on the screen is too big.

I suppose that the problem is not because the lag is too big - in a normal state (when the window is created) we don't need to flush the layer content faster than DisplayLink provides. The problem happens in the transition state during creating the window. So, long term solution would be an immediate rendering to the window surface for the first time (as you previously suggested). In the short term, we can just provide the correct background.

Loading

@mrserb
Copy link
Member

@mrserb mrserb commented Sep 13, 2021

in a normal state (when the window is created) we don't need to flush the layer content faster than DisplayLink provide.

But this is exactly related to the performance, no? When the RenderPerf test is executed it should check that delay between fillRect and the actual rendering is as small as possible. Can you please just confirm that the problem is in the "performance" of DisplayLink and unrelated to some broken rendering? I guess you can create two timestamps in NSWindow creation and in Layer callback(where we actually blit the content/ drawInXXXContext). How that timing is different for metal and OGL?

Loading

@avu
Copy link
Author

@avu avu commented Sep 14, 2021

in a normal state (when the window is created) we don't need to flush the layer content faster than DisplayLink provide.

But this is exactly related to the performance, no? When the RenderPerf test is executed it should check that delay between fillRect and the actual rendering is as small as possible. Can you please just confirm that the problem is in the "performance" of DisplayLink and unrelated to some broken rendering? I guess you can create two timestamps in NSWindow creation and in Layer callback(where we actually blit the content/ drawInXXXContext). How that timing is different for metal and OGL?

I did some measurements for Metal and OGL and didn't find any noticeable difference with the simple test (just showing the frame)
Metal
1631605170711 window appeared
1631605170723 rendering submitted
1631605170724 rendering complete
1631605170747 rendering submitted
1631605170747 rendering complete
OGL
1631605401836 rendering complete
1631605401838 rendering complete
1631605401843 window appeared
1631605401881 rendering complete

Metal has additional output because we can track two stages - submission and completion. Also, I saw the flickering with OGL too (if we have no match with content and window colors)

Loading

@mrserb
Copy link
Member

@mrserb mrserb commented Sep 15, 2021

Then why you clearly see that missmatch in case of metal? probably we initialise the metal layer or its texture to some wrong color? or it is transparent and background of the nswindow is visible?

Loading

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 13, 2021

@avu This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Loading

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Nov 10, 2021

@avu This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

Loading

@bridgekeeper bridgekeeper bot closed this Nov 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants