Skip to content
This repository has been archived by the owner. It is now read-only.

8266079: Lanai: AlphaComposite shows differences on Metal compared to OpenGL #62

Closed
wants to merge 3 commits into from

Conversation

@avu
Copy link

@avu avu commented Jun 15, 2021

Implemented blit via compute kernel


Progress

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

Issue

  • JDK-8266079: Lanai: AlphaComposite shows differences on Metal compared to OpenGL

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 62

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jun 15, 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.

@openjdk
Copy link

@openjdk openjdk bot commented Jun 15, 2021

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

  • 2d
  • awt

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

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jun 15, 2021

threadsPerThreadgroup:threadgroupSize];
[computeEncoder endEncoding];
[cb commit];
#endif
Copy link
Member

@mrserb mrserb Jun 15, 2021

This is better than changing the background color, but should be carefully checked. @jayathirthrao @aghaisas please take a look.
Since this affects every blit to the window I suggest running a full test run.

Copy link
Author

@avu avu Jun 16, 2021

@jayathirthrao @aghaisas please let me know if I can push this change into JDK17 repository

Copy link
Member

@jayathirthrao jayathirthrao Jun 16, 2021

@avu i have given all test run. I will get back to you once it is done.

Copy link
Member

@jayathirthrao jayathirthrao Jun 17, 2021

Automated jtreg/JCK test run is green.

Copy link
Member

@mrserb mrserb Jun 17, 2021

@avu If all our automated tests are green but we still have some issues, then an additional automated test needs to be added.

mrserb
mrserb approved these changes Jun 15, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jun 15, 2021

@avu This change is no longer ready for integration - check the PR body for details.

@openjdk openjdk bot added the ready label Jun 15, 2021
NSUInteger maxTotalThreadsPerThreadgroup = computePipelineState.maxTotalThreadsPerThreadgroup;
NSUInteger w = computePipelineState.threadExecutionWidth;

// Workaround for some OS/device bug reporting incorrect maxTotalThreadsPerThreadgroup
Copy link
Member

@jayathirthrao jayathirthrao Jun 17, 2021

@avu Do we know in which hardware we have this issue? Also do we have any reference to Apple bug?
Also if we use thread group as 1 what is the performance impact in such hardware?

Copy link
Member

@jayathirthrao jayathirthrao Jun 17, 2021

Forced maxTotalThreadsPerThreadgroup to 1 and tested in Intel SoC 2015 Macbook Pro, as expected i see almost ~50% reduction in FPS numbers in many use cases in RenderPerfTest. This final blit hits common operation and we would be able to make better decision whether this performance reduction is okay or not based on hardware in which we are seeing Apple bug.

@jayathirthrao
Copy link
Member

@jayathirthrao jayathirthrao commented Jun 17, 2021

/reviewers 2

@openjdk
Copy link

@openjdk openjdk bot commented Jun 17, 2021

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

@openjdk openjdk bot removed the ready label Jun 17, 2021
@jayathirthrao
Copy link
Member

@jayathirthrao jayathirthrao commented Jun 17, 2021

With this patch shaped windows with translucent/color background are showing only black background. In manual JCK shaped window test also this difference in behaviour is seen.

This behaviour difference can be checked using Ruler.java test case attached in https://bugs.openjdk.java.net/browse/JDK-8267963

@aghaisas
Copy link
Contributor

@aghaisas aghaisas commented Jun 17, 2021

What I have observed with this patch is - It does not break all shaped or translucent windows - but, a manual JCK test does show the black background.

@avu
Copy link
Author

@avu avu commented Jun 21, 2021

Just found the more simple solution to this problem: we can still use old blit code and keep MTLLayer opacity property in sync with contentView of the window (CWraper.setOpaque).

@@ -343,6 +344,9 @@ static void initLevels()
NSWindow *window = (NSWindow *)jlong_to_ptr(windowPtr);
[ThreadUtilities performOnMainThreadWaiting:NO block:^(){
[window setOpaque:(BOOL)opaque];
if ([window.contentView.layer isKindOfClass:[CAMetalLayer class]]) {
[window.contentView.layer setOpaque:(BOOL)opaque];
}
Copy link
Member

@mrserb mrserb Jun 28, 2021

This class "CWrapper" is intended to be a simple wrapper on top of cocoa methods, the changed method above is expected to call the "NSWindow#setOpaque" only. So you need to place "window.contentView.layer setOpaque" in some other place(native or java).

Copy link
Author

@avu avu Jul 5, 2021

Done

@jayathirthrao
Copy link
Member

@jayathirthrao jayathirthrao commented Jul 1, 2021

All test run is green with latest commit.

… OpenGL

Keep MTLLayer opacity in sync with window content view
@avu avu force-pushed the JDK-8266079 branch from 3eef431 to 361407b Jul 2, 2021
mrserb
Copy link
Member

mrserb commented on 361407b Jul 2, 2021

don't we need to do something like [mtlLayer setOpaque:(opaque == JNI_TRUE)]?

@aghaisas
Copy link
Contributor

@aghaisas aghaisas commented Jul 5, 2021

The latest version of this PR re-introduces JDK-8243547

@avu
Copy link
Author

@avu avu commented Jul 5, 2021

The latest version of this PR re-introduces JDK-8243547

rechecked with the previous version, still reproducible.

Alexey Ushakov added 2 commits Jul 5, 2021
@avu
Copy link
Author

@avu avu commented Jul 5, 2021

don't we need to do something like [mtlLayer setOpaque:(opaque == JNI_TRUE)]?

Done

@avu
Copy link
Author

@avu avu commented Jul 5, 2021

The latest version of this PR re-introduces JDK-8243547

Fixed

boolean isTranslucent = peer != null && peer.isTranslucent();
if (!isTranslucent) {
contentView.setWindowLayerOpaque(true);
}
Copy link
Member

@mrserb mrserb Jul 5, 2021

I think windowLayer should be always in sync with NSWindowOpaque state. And both should be changed together via setOpaque() method.

The change above will call the "setWindowLayerOpaque" twice:

  • LWWindowPeer()->platformWindow.initialize()->contentView.setWindowLayerOpaque(true)
  • LWWindowPeer()->initializeImpl()->setOpaque()->contentView.setWindowLayerOpaque()

@aghaisas
Copy link
Contributor

@aghaisas aghaisas commented Jul 7, 2021

I verified that the latest version does not re-introduce JDK-8243547.
All automated test run is also green with this version.

@jayathirthrao
Copy link
Member

@jayathirthrao jayathirthrao commented Jul 15, 2021

@avu Since we are past RDP2, lets close this JDK17 PR and open new jdk-dev(18) PR to continue the review.

@prrace
Copy link
Contributor

@prrace prrace commented Jul 21, 2021

@avu Since we are past RDP2, lets close this JDK17 PR and open new jdk-dev(18) PR to continue the review.

Yes, time to move this one (back to) JDK 18.
It may get backported to a 17u update at some point but now there's no time left for 17 GA

@avu avu closed this Jul 28, 2021
@avu
Copy link
Author

@avu avu commented Jul 28, 2021

Moving this request to jdk18

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
5 participants