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

8284888 : [macos] javax/swing/JInternalFrame/8146321/JInternalFrameIconTest.java failed with "NimbusLookAndFeel] : ERROR: icon and imageIcon not same." #8380

Closed
wants to merge 6 commits into from

Conversation

prsadhuk
Copy link
Contributor

@prsadhuk prsadhuk commented Apr 25, 2022

Test used to fail in specific CI macos M1 system owing to miniscule color difference

x 0 y 0 red1 171 red2 171 green1 174 green2 175 blue1 184 blue2 184
x 0 y 1 red1 172 red2 173 green1 177 green2 177 blue1 185 blue2 185
x 0 y 2 red1 173 red2 174 green1 177 green2 178 blue1 187 blue2 187
x 0 y 6 red1 0 red2 0 green1 1 green2 0 blue1 0 blue2 0
x 0 y 15 red1 1 red2 0 green1 0 green2 0 blue1 0 blue2 0

SInce we already have color-tolerance check present, there is no need of exact color value check.
Also, made the frame undecorated and remove unneeded library being built.
Several iterations of the test passed in the same system (where it used to fail 4/10) along with other platforms (link in JBS)


Progress

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

Issue

  • JDK-8284888: [macos] javax/swing/JInternalFrame/8146321/JInternalFrameIconTest.java failed with "NimbusLookAndFeel] : ERROR: icon and imageIcon not same."

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8380

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 25, 2022

👋 Welcome back psadhukhan! 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 Apr 25, 2022
@openjdk
Copy link

openjdk bot commented Apr 25, 2022

@prsadhuk 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.

@openjdk openjdk bot added the client label Apr 25, 2022
@mlbridge
Copy link

mlbridge bot commented Apr 25, 2022

Webrevs

@mrserb
Copy link
Member

mrserb commented Apr 25, 2022

The JDK-8284888 bug is not public, cannot see a bug description.

@mrserb
Copy link
Member

mrserb commented Apr 25, 2022

BTW did you have a chance to check why to images are not the same? Is it a problem in the robot or in the rendering? Can it be related to upscaling/downscaling?

@prsadhuk
Copy link
Contributor Author

prsadhuk commented Apr 26, 2022

The bug is made public.
The images looks alike visually. I could not find anything wrong with rendering or robot which otherwise could have caused issue in other systems and platforms but it's only failing in this CI system (but color profile setting is ok). As per JDK-8266247 which also happens in same mc, some faulty video memory is doubted.
Also, it's not happening everytime but intermittently so seeing all this, I have just relaxed color check a bit

@mrserb
Copy link
Member

mrserb commented Apr 27, 2022

The bug is made public. The images looks alike visually. I could not find anything wrong with rendering or robot which otherwise could have caused issue in other systems and platforms but it's only failing in this CI system (but color profile setting is ok). As per JDK-8266247 which also happens in same mc, some faulty video memory is doubted. Also, it's not happening everytime but intermittently so seeing all this, I have just relaxed color check a bit

I am not sure that the color profile or something like that can be the root cause, because the test does not check an exact RGB of the color, instead, it renders the image twice and then compares, so it is strange that the results are different.

@prsadhuk
Copy link
Contributor Author

prsadhuk commented Apr 27, 2022

The bug is made public. The images looks alike visually. I could not find anything wrong with rendering or robot which otherwise could have caused issue in other systems and platforms but it's only failing in this CI system (but color profile setting is ok). As per JDK-8266247 which also happens in same mc, some faulty video memory is doubted. Also, it's not happening everytime but intermittently so seeing all this, I have just relaxed color check a bit

I am not sure that the color profile or something like that can be the root cause, because the test does not check an exact RGB of the color, instead, it renders the image twice and then compares, so it is strange that the results are different.

Not sure I understand. It does not render same image twice but it renders an ImageIcon and an Icon and compare the bufferedimage of those 2 so I guess it alright to not have exact color check if (red1 != red2 || green1 != green2 || blue1 != blue2) { and rely on color tolerance which we have in many tests involving bufferedimages..

@mrserb
Copy link
Member

mrserb commented Apr 28, 2022

Not sure I understand. It does not render same image twice but it renders an ImageIcon and an Icon and compare the bufferedimage of those 2 so I guess it alright to not have exact color check if (red1 != red2 || green1 != green2 || blue1 != blue2) { and rely on color tolerance which we have in many tests involving bufferedimages..

Sure these are not the same images, but still, we render images and get different results, on one specific system? If the robot is not a root cause then looks like the difference occurrs somewhere in the rendering pipeline?

@prrace
Copy link
Contributor

prrace commented May 3, 2022

This is occurring only on the M1 imac where despite having the right profile we see "off by 1/255" errors sometimes.
So why do we need a color tolerance of 25 ? Why isn't it 1 ?

@prsadhuk
Copy link
Contributor Author

prsadhuk commented May 4, 2022

This is occurring only on the M1 imac where despite having the right profile we see "off by 1/255" errors sometimes. So why do we need a color tolerance of 25 ? Why isn't it 1 ?

It was there from start so did not tinker although I tested with tolerance of 10 (which is the default tolerance used in other tests) which passed. Anyway, I have changed it to 1 which also passed in the intended system and all other platforms (CI job link in JBS)

@dcubed-ojdk
Copy link
Member

dcubed-ojdk commented May 4, 2022

@prsadhuk and @prrace,
We have 8 sightings of this failure mode on macosx-aarch64 in the jdk/jdk and
loom CIs. I'm about to start my noise suppression phase in preparation for the
potential Loom VT integration this weekend.

Will this (potential) fix be integrated soon or should I go ahead and ProblemList
this test on macosx-aarch64?

@dcubed-ojdk
Copy link
Member

dcubed-ojdk commented May 4, 2022

I will be integrating a ProblemListing for javax/swing/JInternalFrame/8146321/JInternalFrameIconTest.java
on macosx-aarch64 shortly. Please update this PR to include that ProblemListing changeset and then
UnProblemList the test in the PR so that the test runs in your pre-integration testing.

@prrace
Copy link
Contributor

prrace commented May 4, 2022

I believe this fix should be integrated before the weekend so there is no need to PL.
We can do that if it is needed.

@prrace
Copy link
Contributor

prrace commented May 4, 2022

This is occurring only on the M1 imac where despite having the right profile we see "off by 1/255" errors sometimes. So why do we need a color tolerance of 25 ? Why isn't it 1 ?

It was there from start so did not tinker although I tested with tolerance of 10 (which is the default tolerance used in other tests) which passed. Anyway, I have changed it to 1 which also passed in the intended system and all other platforms (CI job link in JBS)

Yes .. I went back and looked at the thread started here : https://mail.openjdk.java.net/pipermail/swing-dev/2016-January/005286.html
and it was in the first webrev and no one ever questioned why it was needed ..

perhaps some shadow effects were touching on the icon which is near the edge of the top-level window
but that could be cured easily in this test which is for JInternalFrame .. if it were actually an issue ..

@prrace
Copy link
Contributor

prrace commented May 4, 2022

OK so .. there's a difficulty facing this test. It has NO idea where the icon is.
It randomly captures a chunk of the title bar and for Aqua it doesn't even come close to grabbing a single pixel
of the icon - it is 100 pixels or more off.

So we are not comparing what we want to be comparing. It is pointless.

I think you need to come up with a way to make sure you are grabbing the right place which is L&F dependent ...

If you can't then you might as well scrap this useless test which is really just comparing pixels from the title bar.

@prsadhuk
Copy link
Contributor Author

prsadhuk commented May 5, 2022

Yes, it looks for Aqua the icon is "after" the disabled close, maximize, resizable buttons
whereas for other L&F those buttons are not there so for other L&F the icon is at extreme left
but it is strange that the images captured during the failure "imageicon-fail.png" and "iconimage-fail.png" (attached in JBS) shows the image which resembles the icon image (a black square)
I tried to create JInternalFrame(String title, boolean resizable, boolean closable, boolean maximizable, boolean iconifiable) with false for all but it still shows those buttons for Aqua..

I am not sure if there is an y deterministic way to find out the location of the icon

@openjdk openjdk bot removed the rfr label May 5, 2022
@prsadhuk
Copy link
Contributor Author

prsadhuk commented May 5, 2022

I am not sure if there is an y deterministic way to find out the location of the icon

Modified to use the capture area as used in the original webrev for Aqua which will capture the title bar with icon
iconImage-fail

Capture area is modified slightly to omit the top inset too to remove the blue background from captured image.

Testing in iMacs and other mac systems and other platforms are green.

@openjdk openjdk bot added the rfr label May 5, 2022
@prsadhuk
Copy link
Contributor Author

prsadhuk commented May 6, 2022

I am not sure if there is an y deterministic way to find out the location of the icon

Modified to use the capture area as used in the original webrev for Aqua which will capture the title bar with icon iconImage-fail

Capture area is modified slightly to omit the top inset too to remove the blue background from captured image.

Testing in iMacs and other mac systems and other platforms are green.

The original issue was apart from ImageIcon all othericon is displayed lower than it should be and to the right of where it should be, covering part of the title text.
Screenshot 2022-05-06 at 9 40 41 AM

whereas for ImageIcon
Screenshot 2022-05-06 at 10 47 47 AM

so icons not getting drawn was not an issue
so I think it should be ok testing the titlebar the bufferedimage with ImageIcon and Icon will be different thus testing the fix as well keeping all platforms happy

prrace
prrace approved these changes May 9, 2022
Copy link
Contributor

@prrace prrace left a comment

I still think it is possible to enhance this by deliberating drawing the icons in different colors and expecting to see the rendered colours be the only difference and that they are in the same place in both cases .. but maybe what you have here is enough for now.

@openjdk
Copy link

openjdk bot commented May 9, 2022

@prsadhuk This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8284888: [macos] javax/swing/JInternalFrame/8146321/JInternalFrameIconTest.java failed with "NimbusLookAndFeel] : ERROR: icon and imageIcon not same."

Reviewed-by: prr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 48 new commits pushed to the master branch:

  • 837928b: 8222323: ChildAlwaysOnTopTest.java fails with "RuntimeException: Failed to unset alwaysOnTop"
  • 397d095: 8285743: Ensure each IntegerPolynomial object is only created once
  • 29ccb8f: 8285914: AppCDS crash when using shared archive with old class file
  • fe6e0c0: 8286371: Avoid use of deprecated str[n]icmp
  • 97a9835: 8274517: java/util/DoubleStreamSums/CompensatedSums.java fails with expected [true] but found [false]
  • 034f20f: 8212136: Remove finalizer implementation in SSLSocketImpl
  • 36e4df9: 8285516: clearPassword should be called in a finally try block
  • b849efd: 8285923: [REDO] JDK-8285802 AArch64: Consistently handle offsets in MacroAssembler as 64-bit quantities
  • f143386: 8286293: Tests ShortResponseBody and ShortResponseBodyWithRetry should use less resources
  • 64b05cc: 8286346: 3-parameter version of AllocateHeap should not ignore AllocFailType
  • ... and 38 more: https://git.openjdk.java.net/jdk/compare/81d7475d20133fd7dfb0ad66caee4e929e0291af...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label May 9, 2022
@prsadhuk
Copy link
Contributor Author

prsadhuk commented May 12, 2022

/integrate

@openjdk
Copy link

openjdk bot commented May 12, 2022

Going to push as commit ff17f49.
Since your change was applied there have been 114 commits pushed to the master branch:

  • 50d47de: 8286582: Build fails on macos aarch64 when using --with-zlib=bundled
  • 89392fb: 8285820: C2: LCM prioritizes locally dependent CreateEx nodes over projections after 8270090
  • 96d48f3: 8286433: Cache certificates decoded from TLS session tickets
  • 7567627: 8286467: G1: Collection set pruning adds one region too many
  • 82d2570: 8283001: windows-x86-cmp-baseline fails in some jvmti native libs
  • e9f45bb: 8282966: AArch64: Optimize VectorMask.toLong with SVE2
  • 57a7670: 8285612: Remove jtreg tag manual=yesno for java/awt/print/PrinterJob/ImagePrinting/ClippedImages.java
  • 44a60ce: 8285867: Convert applet manual tests SelectionVisible.java to Frame and automate
  • ccbe8fa: 8282772: JButton text set as HTML content has unwanted padding
  • 1586bf8: 8286401: Address possibly lossy conversions in Microbenchmarks
  • ... and 104 more: https://git.openjdk.java.net/jdk/compare/81d7475d20133fd7dfb0ad66caee4e929e0291af...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated label May 12, 2022
@openjdk openjdk bot closed this May 12, 2022
@openjdk openjdk bot removed ready rfr labels May 12, 2022
@openjdk
Copy link

openjdk bot commented May 12, 2022

@prsadhuk Pushed as commit ff17f49.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client integrated
4 participants