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

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

Closed
wants to merge 1 commit into from

Conversation

@avu
Copy link

@avu avu commented May 26, 2021

Set black color for underlying window background to perform correct blending operations in metal with window surface destination


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

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4206

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 26, 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 openjdk bot added the rfr label May 26, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 26, 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.

@openjdk openjdk bot added the awt label May 26, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 26, 2021

Webrevs

@mrserb
Copy link
Member

@mrserb mrserb commented May 26, 2021

I think this will resurrect the JDK-8033786, but with a different color - black instead of white.

@avu
Copy link
Author

@avu avu commented May 27, 2021

Yes, looks like we have the flashing problem back. It seems more serious than this particular bug.
We have two options here:

  1. accept this behavior for metal (It would be not a problem if we implement JDK-8265445)
  2. reimplement all the composing operations via shaders instead of using metal blending rules

@avu
Copy link
Author

@avu avu commented May 29, 2021

Looks like the second option won't work because the result of blending will still be mixed with the NSWindow background color. Also, I tried several more approaches (Use only rgb blending, make the layer opaque). They helped to resolve the test issues but caused regressions in other areas (SwingSet2 tree for example). So, now I think that the only working solution is presented here (to set the background color of the window)

Yes, looks like we have the flashing problem back. It seems more serious than this particular bug.
We have two options here:

  1. accept this behavior for metal (It would be not a problem if we implement JDK-8265445)
  2. reimplement all the composing operations via shaders instead of using metal blending rules

@mrserb
Copy link
Member

@mrserb mrserb commented Jun 1, 2021

I guess the bug happens when we blit our "layer" surface to the window? So there is no way to set a kind of "src" composite? Will it help if you "clear" the layer to some color(white?) and then immediately blit the content on top of it?

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jun 2, 2021

Mailing list message from Alan Snyder on awt-dev:

Could the flashing problem be related to the timing problem described in JDK-8209329 <https://bugs.openjdk.java.net/browse/JDK-8209329>?

I have noticed occasional red flashes using Metal, which I suspect are caused by trying to render uninitialized data.
(I?m guessing that the red is a debugging aid provided by Apple. Previously, they used yellow.)

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/awt-dev/attachments/20210601/4e09c548/attachment-0001.htm>

@avu
Copy link
Author

@avu avu commented Jun 3, 2021

I guess the bug happens when we blit our "layer" surface to the window? So there is no way to set a kind of "src" composite?

AFAIK, we don't have any control over composing the layer content on top of the window surface. We just get drawable for the layer that has a texture attached and then blits our content into it.

Will it help if you "clear" the layer to some color(white?) and then immediately blit the content on top of it?

In current implementation - no. We use a blitencoder that just replaces pixels at the texture attached to the drawable. Texture rendering also won't help as we still blend the resulting color with the window surface. See MTLLayer.m (80-133) for more details.

@avu
Copy link
Author

@avu avu commented Jun 3, 2021

Mailing list message from Alan Snyder on awt-dev:

Could the flashing problem be related to the timing problem described in JDK-8209329 https://bugs.openjdk.java.net/browse/JDK-8209329?

I have noticed occasional red flashes using Metal, which I suspect are caused by trying to render uninitialized data.
(I?m guessing that the red is a debugging aid provided by Apple. Previously, they used yellow.)

-------------- next part --------------
An HTML attachment was scrubbed...
URL: https://mail.openjdk.java.net/pipermail/awt-dev/attachments/20210601/4e09c548/attachment-0001.htm

Yes, blit encoder works asynchronously so we may have some latency before the actual content is presented to the window.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jun 3, 2021

Mailing list message from Alan Snyder on awt-dev:

My concern is that these latency problems are much more noticeable when the Metal pipeline is used.

See JDK-8267226 <https://bugs.openjdk.java.net/browse/JDK-8267226>.

Alan

On Jun 3, 2021, at 7:13 AM, Alexey Ushakov <avu at openjdk.java.net> wrote:

On Wed, 26 May 2021 13:49:24 GMT, Alexey Ushakov <avu at openjdk.org> wrote:

Set black color for underlying window background to perform correct blending operations in metal with window surface destination

_Mailing list message from [Alan Snyder](mailto:javalists at cbfiddle.com) on [awt-dev](mailto:awt-dev at mail.openjdk.java.net):_

Could the flashing problem be related to the timing problem described in JDK-8209329 <https://bugs.openjdk.java.net/browse/JDK-8209329>?

I have noticed occasional red flashes using Metal, which I suspect are caused by trying to render uninitialized data.
(I?m guessing that the red is a debugging aid provided by Apple. Previously, they used yellow.)

Yes, blit encoder works asynchronously so we may have some latency before the actual content is presented to the window.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/awt-dev/attachments/20210603/75f93f87/attachment.htm>

1 similar comment
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jun 3, 2021

Mailing list message from Alan Snyder on awt-dev:

My concern is that these latency problems are much more noticeable when the Metal pipeline is used.

See JDK-8267226 <https://bugs.openjdk.java.net/browse/JDK-8267226>.

Alan

On Jun 3, 2021, at 7:13 AM, Alexey Ushakov <avu at openjdk.java.net> wrote:

On Wed, 26 May 2021 13:49:24 GMT, Alexey Ushakov <avu at openjdk.org> wrote:

Set black color for underlying window background to perform correct blending operations in metal with window surface destination

_Mailing list message from [Alan Snyder](mailto:javalists at cbfiddle.com) on [awt-dev](mailto:awt-dev at mail.openjdk.java.net):_

Could the flashing problem be related to the timing problem described in JDK-8209329 <https://bugs.openjdk.java.net/browse/JDK-8209329>?

I have noticed occasional red flashes using Metal, which I suspect are caused by trying to render uninitialized data.
(I?m guessing that the red is a debugging aid provided by Apple. Previously, they used yellow.)

Yes, blit encoder works asynchronously so we may have some latency before the actual content is presented to the window.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/awt-dev/attachments/20210603/75f93f87/attachment.htm>

@mrserb
Copy link
Member

@mrserb mrserb commented Jun 3, 2021

AFAIK, we don't have any control over composing the layer content on top of the window surface. We just get drawable for the layer that has a texture attached and then blits our content into it.

Probably I missed your point, but there is no issue if we draw opaque color to the layer. Since it does not matter what kind of blending is used when we blit layer to nswindow and the layer has an opaque color. Why we cannot emulate the same when the app draws some transparency and use some blending? Just fill the layer with the opaque "default" color(or make it "opaque"), blend the transparent color on top of it and then blit this opaque result to the window.

Its is actually a question why the mtl layer is initialised as opaque=false in all cases.

@avu
Copy link
Author

@avu avu commented Jun 3, 2021

AFAIK, we don't have any control over composing the layer content on top of the window surface. We just get drawable for the layer that has a texture attached and then blits our content into it.

Probably I missed your point, but there is no issue if we draw opaque color to the layer. Since it does not matter what kind of blending is used when we blit layer to nswindow and the layer has an opaque color. Why we cannot emulate the same when the app draws some transparency and use some blending? Just fill the layer with the opaque "default" color(or make it "opaque"), blend the transparent color on top of it and then blit this opaque result to the window.

Its is actually a question why the mtl layer is initialised as opaque=false in all cases.

Yes, this approach (make MTLLayer opaque) works for this particular test but it causes artifacts in SwingSet demo. It prevents me from implementing the fix that way. Probably, there is another rendering problem that is masked by making the layer transparent.

… OpenGL

Implemented blit via compute kernel
@avu
Copy link
Author

@avu avu commented Jun 14, 2021

Reimplemented solution using compute shader

@prrace
Copy link
Contributor

@prrace prrace commented Jun 14, 2021

@avu - Aren't we targeting this to 17 ? Assuming yes, I think you need to submit a new PR against the JDK 17
stabilsation repo .. and withdraw this one.

@avu
Copy link
Author

@avu avu commented Jun 15, 2021

Created separate pull request in jdk17 repo: openjdk/jdk17#62

@avu avu closed this Jun 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants