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

Jeremy Wood (@mickleness - Author) 🔄 Re-review required (review applies to 48ad43b7)

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 ||?

Yes It should

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.)

mrserb · 2025-04-25T07:52:29Z

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

+        Image img = getLWGC().createAcceleratedImage(getTarget(), scaledWidth, scaledHeight);
+        if(ScaleX == 1 || ScaleY == 1)
+            return img;
+        return new BaseMultiResolutionImage(img.getScaledInstance(width, height, Image.SCALE_DEFAULT), img) {


Unfortunately, even our own code assumes that at least a BufferedImage is returned here.

The getScaledInstance() method is slow and largely unsupported.

Unfortunately, even our own code assumes that at least a BufferedImage is returned here.

Where?

Assuming that's a problem: this expectation sounds (IMO) like something we should bake into the new unit test for this PR (right?)

The getScaledInstance() method is slow and largely unsupported.

Sure. And I don't love using this sort of trick BaseMultiResolutionImage. But I have to admit: there's an elegance to this design that I've come to like.

I agree getScaledInstance should be mostly avoided, but in this case: it looks harmless to me. This code constructs what is mostly a dummy ToolkitImage & FilteredImageSource. We never actually have to produce the scaled pixels (which is the slow/bad part).

In reality I think the Image we return only needs to implement getWidth, getHeight, and getGraphics.

I don't know Anass's availability/preferences, but if I better understand the problems we need to resolve here: I might be able to add to this PR (or fork it).

Where?
Here

When HiDPI support was integrated, these old methods for non-accelerated buffers were not updated, because it is not possible to return a BufferedImage whose bounds bigger than the component itself. And some code assumes that we always return a BufferedImage. We could consider returning a MultiResolution version of the BufferedImage, but it is better try something else before that.

I agree getScaledInstance should be mostly avoided, but in this case: it looks harmless to me. This code constructs what is mostly a dummy ToolkitImage & FilteredImageSource. We never actually have to produce the scaled pixels (which is the slow/bad part).

But why do we need to use the slow ToolkitImage? We can use non-accelerated but still fast BufferedImages instead, we just need to scale the image from one to another without changing its type.

Here

Oof. That is... both helpful and not helpful. (That's not a criticism towards you; it's towards that TODO note.)

I can confirm that the TransparentWindowTest in this PR works. (That is: there's no obvious ClassCastException or anything bad.) But now I'm worried about other code paths that aren't represented in this PR that will break. Does anyone has any theories on what "some stuff will break" in that link refers to? (That is: what should I be testing to see something break?)

(This replaces my previous response.)

But why do we need to use the slow ToolkitImage?

To use the BaseMultiResolutionImage we needed our first (base) image to be the correct size. That's all we needed that ToolkitImage for: it reported the correct dimensions.

(We do NOT ever want to actually scale any image. So whether we use a ToolkitImage or a BufferedImage: there should be no scaling.)

Here's an alternative approach that wraps a scaled BufferedImage in a MultiResolutionImage:

@Override public final Image createImage(final int width, final int height) { GraphicsConfiguration gc = getGraphicsConfiguration(); AffineTransform at = gc.getDefaultTransform(); double ScaleX = at.getScaleX(); double ScaleY = at.getScaleY(); int scaledWidth = Math.round((float)ScaleX * width); int scaledHeight = Math.round((float)ScaleY * height); Image img = getLWGC().createAcceleratedImage(getTarget(), scaledWidth, scaledHeight); if(ScaleX == 1 && ScaleY == 1) return img; // This new subclass functions as a scaled BufferedImage. For ex: this // image may present its perceived size as 100x100px, but it's actually // backed by a BufferedImage that's 150x150px, or 200x200px. return new AbstractMultiResolutionImage() { @Override public Graphics getGraphics() { Graphics graphics = img.getGraphics(); if (graphics instanceof Graphics2D) ((Graphics2D) graphics).scale(ScaleX, ScaleY); return graphics; } @Override public int getWidth(ImageObserver observer) { return width; } @Override public int getHeight(ImageObserver observer) { return height; } @Override public Image getBaseImage() { return img; } @Override public Image getResolutionVariant(double destImageWidth, double destImageHeight) { return img; } @Override public List<Image> getResolutionVariants() { return Arrays.asList(img); } }; }

This is longer, but it may be a little clearer what it's trying to do.

(I confirmed this passes the new test case in this PR.)

Does this speak to your question?

Hello @mickleness, @mrserb,
I submitted the PR as a draft due to a few issues.
It's not incorrect to return a BaseMultiResolutionImage, but one of our CI tests DrawImageBilinear.java assumes that createImage() returns something that can be cast to a BufferedImage. So, if we proceed with that change, I’ll need to adjust the test accordingly.

@mickleness , regarding your proposed fix using MultiResolutionImage: I’ve already tried that, and it works fine in tests. This approach is also used in JViewport.

However, I’m not entirely sure whether we actually need either method here. Do we really need multi-resolution images in this context ? I think we might just be able to scale the image in createAcceleratedImage and return an OffScreenImage, or even just a BufferedImage. (I could be wrong, I'm still new to graphics.) What do you think?

It might be worth checking how hard it is to change the implementation of RepaintManager and create a buffer there w/o using this api.

This is Anass's branch/PR so I'm not sure if I can/should try to commit to it. But I moved these changes to RepaintManager and wrote some fully automated tests:

https://github.com/openjdk/jdk/pull/13196/files#diff-c4425bd7c3151fbc23c7f40fcc81e02d2bba1c14793edb438fdfdc43b454451f

This is functionally the same as the approach we were discussing. Architecturally I still prefer the idea of Component#createImage() being responsible for creating the multi-resolution image, but I'm more interested in resolving this bug than architecture.

mrserb · 2025-04-25T08:01:31Z

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

@@ -984,7 +988,25 @@ public boolean requestFocus(Component lightweightChild, boolean temporary,

    @Override
    public final Image createImage(final int width, final int height) {
-        return getLWGC().createAcceleratedImage(getTarget(), width, height);


Where is this method called from? If it is invoked by the RepaintManager, it might be better to create the appropriate back buffer there - or alternatively, you can implement OffScreenImage similar to VolatileImage. Then it will cover all platforms.

Also note that the RepaintManager uses different round logic.

where is this method called from?

Very few places:

Interestingly JViewPort may already have a helper class for this ( BackingStoreMultiResolutionImage ). I'll see if I can extract/reuse that in the next few days.

mickleness · 2025-05-05T18:22:44Z

@anass-baya (and anyone else): would you like me to try adapting the test(s) here so they don't require a person to pass/fail them? (I'm not sure what the prevailing philosophy of this group is on this subject.)

anass-baya · 2025-05-06T15:32:53Z

Hello @mickleness,
These were my first created jdk tests. I discussed them with Alexey, and he informed me that it's okay to write a manual test if I couldn't come up with an automated one. I tried automating it, but it was inconsistent because I couldn't come up with a finely-tuned sharpness threshold to detected the pixelated edge of the text, as the image is not too pixelated.

Feel free to propose any suggestions. However, I still need to modify these tests. The keyboard @headful is missing in the TransparentWindowTest, and the keyboard manual is missing in PixelatedImageTest.

This reverts commit 295b166.

bridgekeeper · 2025-07-09T20:01:39Z

@anass-baya This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

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 client-libs-dev@openjdk.org rfr Pull request is ready for review 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 Pull request is ready for review 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 Pull request is ready for review 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

anass-baya changed the title ~~8303904: [macos]Transparent Windows Paint Wrong (Opaque, Pixelated) w/o Volatile Buffering~~ Draft: 8303904: [macos]Transparent Windows Paint Wrong (Opaque, Pixelated) w/o Volatile Buffering Apr 9, 2025

openjdk bot removed the rfr Pull request is ready for review label Apr 9, 2025

Enhancment

0968fc8

anass-baya marked this pull request as draft April 9, 2025 18:05

anass-baya changed the title ~~Draft: 8303904: [macos]Transparent Windows Paint Wrong (Opaque, Pixelated) w/o Volatile Buffering~~ 8303904: [macos]Transparent Windows Paint Wrong (Opaque, Pixelated) w/o Volatile Buffering Apr 9, 2025

mrserb reviewed Apr 25, 2025

View reviewed changes

Merge branch 'openjdk:master' into JDK-8303904

0294046

anass-baya added 2 commits May 12, 2025 13:18

the count of the latch has to be 2

295b166

Revert "the count of the latch has to be 2"

8320d5b

This reverts commit 295b166.

mickleness mentioned this pull request Aug 12, 2025

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

Open

3 tasks

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 Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Contributors

Reviewing

Uh oh!

bridgekeeper bot commented Feb 4, 2025

Uh oh!

openjdk bot commented Feb 4, 2025

Uh oh!

openjdk bot commented Feb 4, 2025

Uh oh!

mlbridge bot commented Feb 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anass-baya Feb 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aivanov-jdk commented Feb 4, 2025

Uh oh!

openjdk bot commented Feb 4, 2025

Uh oh!

anass-baya commented Feb 4, 2025

Uh oh!

openjdk bot commented Feb 4, 2025

Uh oh!

aivanov-jdk commented Feb 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mickleness left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anass-baya May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

anass-baya May 6, 2025 •

edited

Loading