8303904: [macos]Transparent Windows Paint Wrong (Opaque, Pixelated) w/o Volatile Buffering #23430

anass-baya · 2025-02-04T00:07:48Z

The PR includes two fix :
1 - The first fix targets proper rendering of the transparent image.
2 - The second fix ensures the image is painted at the correct resolution to avoid pixelation.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8303904: [macos]Transparent Windows Paint Wrong (Opaque, Pixelated) w/o Volatile Buffering (Bug - P3)

Reviewers without OpenJDK IDs

@mickleness (no known openjdk.org user name / role)

Contributors

Jeremy mickleness <mickleness@gmail.com>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/23430/head:pull/23430
$ git checkout pull/23430

Update a local copy of the PR:
$ git checkout pull/23430
$ git pull https://git.openjdk.org/jdk.git pull/23430/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23430

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/23430.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-02-04T00:08:23Z

👋 Welcome back anass-baya! 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 · 2025-02-04T00:08:30Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2025-02-04T00:09:03Z

@anass-baya The following label will be automatically applied to this pull request:

client

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.

mlbridge · 2025-02-04T00:12:27Z

Webrevs

test/jdk/javax/swing/JWindow/NonVolatileTransparentWindows/PixelatedImageTest.java

test/jdk/javax/swing/JWindow/NonVolatileTransparentWindows/TransparentWindowTest.java

mickleness · 2025-02-04T07:32:07Z

src/java.desktop/macosx/classes/sun/java2d/metal/MTLGraphicsConfig.java

+        float scaleFactor = getDevice().getScaleFactor();
+        int fullResWidth = Math.round( scaleFactor * width);
+        int fullResHeight = Math.round( scaleFactor * height);
+        WritableRaster wr = model.createCompatibleWritableRaster(fullResWidth, fullResHeight);


Thanks for working on this. :)

I have a couple of questions:

This math looks great for the simple case of a 200% high-res monitor; is the rounding safe for non-integer resolutions?

For ex if width is 33 and scaleFactor is 1.25, then we'll call round(41.25), so fullResWidth = 41.

Will the native windowing system allocate 41px for our Window? (Because if so: great. We have a perfect fit.) Or could it allocate 42px? (In which case we may have a thin border on the right/bottom that becomes unpaintable...)

I'm assuming if fullResWidth/fullResHeight is ever zero we'll get a RTE. I don't think it's stated, but the preexisting code suggests width and height were never zero. Is it possible scaleFactor can ever be less than .5? (I've never observed that in my day-to-day life, but I'm just thinking about edge cases and RTE's...)

Hello @mickleness ,
Thank you for your review.
It is well known that we can lose a fraction of a pixel. For example in #23183 we allowed a 1-pixel margin of error to compensate for that as the rounding error could accumulate

mickleness · 2025-02-04T08:02:08Z

src/java.desktop/macosx/classes/sun/java2d/metal/MTLGraphicsConfig.java

+                returnValue.scale(scaleFactor, scaleFactor);
+                return returnValue;
+            }
+        };
    }


This may be outside the scope of this ticket, but I'm seeing similar code in Win32GraphicsConfig#createAcceleratedImage(..), X11GraphicsConfig#createAcceleratedImage(..), and GLXGraphicsConfig#createAcceleratedImage(..) .

Should those be looked at, or is there another ticket to explore those? (That is: are they also going to be opaque and the wrong resolution?)

Hello @mickleness,
The problem is not reproduced on windows !! I can see it only on mac

tldr: Wow / huh. You're right. Thanks for checking.

That code looks so suspicious that I did some digging:

(A. On Windows the TransparentWindowTest says it fails because Image.getTransparency() is OPAQUE, but the test is wrong: the window looks fine.)
B. It works because on Windows we end up using the TranslucentWindowPainter. We never invoke Win32GraphicsConfig#createAcceleratedImage(..) at all (unless the TransparentWindowTest invokes it directly).

I tested:
A. repainting on a timer (so we'd explicitly go through the RepaintManager), and
B. an ellipse fill instead of a rectangle fill (to double-check that transparent pixels really were transparent).
C. using Window.setShape(..) instead of changing the window's background color to transparent (to make sure it followed the same path)

(... and I don't have access to any other platforms to make sure Linux is doing the right thing.)

Hello @mickleness,
Yes, on Windows we use TranslucentWindowPainter.
I moved the PR to draft since I'm trying to simplify the scaling handler logic. I'll come back to you once I'm done with that.
This test is only for macOS. On Windows, it will fail because we don’t explicitly set the Translucent flag.

mickleness · 2025-02-04T08:15:53Z

src/java.desktop/macosx/classes/sun/java2d/opengl/CGLGraphicsConfig.java

+                returnValue.scale(scaleFactor, scaleFactor);
+                return returnValue;
+            }
+        };
    }


Should this new createAcceleratedImage(..) logic be moved to CGraphicsConfig?

This looks like copied and pasted code in two subclasses that could be abstracted into the parent class. (I could be wrong here; I just wanted to check opinions...?)

It looks like these two classes (MTLGraphicsConfig and CGLGraphicsConfig) are the only subclasses for CGraphicsConfig.

aivanov-jdk · 2025-02-04T17:56:12Z

@anass-baya If your fix is based on @mickleness's previous PR, you should him as co-author using the /contributor command.

openjdk · 2025-02-04T20:29:52Z

@anass-baya @mickleness was not found in the census.

Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

/contributor add @openjdk-bot
/contributor add duke
/contributor add J. Duke <duke@openjdk.org>

User names can only be used for users in the census associated with this repository. For other contributors you need to supply the full name and email address.

anass-baya · 2025-02-04T20:34:31Z

/contributor add Jeremy mickleness mickleness@gmail.com

openjdk · 2025-02-04T20:34:40Z

@anass-baya
Contributor Jeremy mickleness <mickleness@gmail.com> successfully added.

aivanov-jdk · 2025-02-04T21:50:49Z

@mickleness I wonder what is your preferred email address and name. You were referenced by another email in your recent contribution.

mickleness · 2025-02-26T18:35:31Z

src/java.desktop/macosx/classes/sun/lwawt/LWComponentPeer.java

+
+        Image img = getLWGC().createAcceleratedImage(getTarget(), scaledWidth, scaledHeight);
+        if(ScaleX == 1 || ScaleY == 1)
+            return img;


Should this be && instead of ||?

mickleness

Looks good to me (note I'm not an official OpenJDK reviewer). I'm not a fan of the instanceof Graphics2D check, but any alternative I propose would be more lines of code so I accept this is good enough.

mickleness · 2025-02-26T18:55:33Z

src/java.desktop/macosx/classes/sun/lwawt/LWComponentPeer.java

-import java.awt.Rectangle;
-import java.awt.Toolkit;
-import java.awt.Window;
+import java.awt.*;


I know in other PRs reviewers have asked to expand wildcard imports. (I haven't studied that topic closely enough to have an informed opinion.)

anass-baya added 5 commits February 3, 2025 17:30

fix 1 : fix for non-transparency and black borders

b907fe8

Unit Test to Verify Window Transparency and Black Border Presence

74049d8

Test for Mac platfotm

2426de4

Add test for pixelated images at high resolution

7052240

fix for pixelated images

9cf273b

openjdk bot changed the title ~~JDK-8303904 [macos]Transparent Windows Paint Wrong (Opaque, Pixelated) w/o Volatile Buffering~~ 8303904: [macos]Transparent Windows Paint Wrong (Opaque, Pixelated) w/o Volatile Buffering Feb 4, 2025

openjdk bot added client rfr labels Feb 4, 2025

lawrence-andrew reviewed Feb 4, 2025

View reviewed changes

test/jdk/javax/swing/JWindow/NonVolatileTransparentWindows/PixelatedImageTest.java Outdated Show resolved Hide resolved

lawrence-andrew reviewed Feb 4, 2025

View reviewed changes

test/jdk/javax/swing/JWindow/NonVolatileTransparentWindows/TransparentWindowTest.java Outdated Show resolved Hide resolved

Update Copyright

4ce878b

mickleness reviewed Feb 4, 2025

View reviewed changes

Remove unnecessary comment

e65d734

anass-baya marked this pull request as draft February 5, 2025 14:26

openjdk bot removed the rfr label Feb 5, 2025

anass-baya added 3 commits February 21, 2025 16:43

enhancement - step 1

28d3ceb

Enhancement - step 2

f669918

Enhancment step 3

48ad43b

anass-baya marked this pull request as ready for review February 24, 2025 08:38

anass-baya requested a review from mickleness February 24, 2025 08:38

openjdk bot added the rfr label Feb 24, 2025

mickleness reviewed Feb 26, 2025

View reviewed changes

mickleness approved these changes Feb 26, 2025

View reviewed changes

mickleness reviewed Feb 26, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8303904: [macos]Transparent Windows Paint Wrong (Opaque, Pixelated) w/o Volatile Buffering #23430

8303904: [macos]Transparent Windows Paint Wrong (Opaque, Pixelated) w/o Volatile Buffering #23430

anass-baya commented Feb 4, 2025 •

edited by openjdk bot

Loading

bridgekeeper bot commented Feb 4, 2025

openjdk bot commented Feb 4, 2025

openjdk bot commented Feb 4, 2025

mlbridge bot commented Feb 4, 2025 •

edited

Loading

mickleness Feb 4, 2025

anass-baya Feb 4, 2025

mickleness Feb 4, 2025

anass-baya Feb 4, 2025

mickleness Feb 5, 2025

anass-baya Feb 5, 2025 •

edited

Loading

mickleness Feb 4, 2025

aivanov-jdk commented Feb 4, 2025

openjdk bot commented Feb 4, 2025

anass-baya commented Feb 4, 2025

openjdk bot commented Feb 4, 2025

aivanov-jdk commented Feb 4, 2025

mickleness Feb 26, 2025

mickleness left a comment

mickleness Feb 26, 2025

8303904: [macos]Transparent Windows Paint Wrong (Opaque, Pixelated) w/o Volatile Buffering #23430

Are you sure you want to change the base?

8303904: [macos]Transparent Windows Paint Wrong (Opaque, Pixelated) w/o Volatile Buffering #23430

Conversation

anass-baya commented Feb 4, 2025 • edited by openjdk bot Loading

Progress

Issue

Reviewers without OpenJDK IDs

Contributors

Reviewing

bridgekeeper bot commented Feb 4, 2025

openjdk bot commented Feb 4, 2025

openjdk bot commented Feb 4, 2025

mlbridge bot commented Feb 4, 2025 • edited Loading

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anass-baya Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aivanov-jdk commented Feb 4, 2025

openjdk bot commented Feb 4, 2025

anass-baya commented Feb 4, 2025

openjdk bot commented Feb 4, 2025

aivanov-jdk commented Feb 4, 2025

Choose a reason for hiding this comment

mickleness left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anass-baya commented Feb 4, 2025 •

edited by openjdk bot

Loading

mlbridge bot commented Feb 4, 2025 •

edited

Loading

anass-baya Feb 5, 2025 •

edited

Loading