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

8280861: Robot color picker broken on Linux with scaling above 100% #7425

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2005, 2021, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2005, 2022, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -37,7 +37,9 @@
#include "sizecalc.h"
#include <jni_util.h>
#include <stdio.h>
#include <math.h>
#include "awt.h"
#include "debug_assert.h"

static void *gtk3_libhandle = NULL;
static void *gthread_libhandle = NULL;
@@ -2861,17 +2863,45 @@ static void transform_detail_string (const gchar *detail,
}
}

inline static int scale_down_to_plus_inf(int what, int scale) {
return (int)ceilf(what / (float)scale);
Copy link
Contributor

@prrace prrace Feb 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the name of this and the companion function confusing.
what is "inf" short for ? I see that and I think plus_infinity.
scaled_ceiling and scaled_floor might be better names based on what I see.

Copy link
Member Author

@mkartashev mkartashev Feb 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, "inf" is for infinity; this is sort of how the rounding modes are described in the IEEE754 standard. But since full length words make the function name too long and short versions are confusing, I will rename as you suggested.

Copy link
Member Author

@mkartashev mkartashev Feb 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@prrace The change has been made, please, take a look.

}

inline static int scale_down_to_minus_inf(int what, int scale) {
return (int)floorf(what / (float)scale);
}

static gboolean gtk3_get_drawable_data(JNIEnv *env, jintArray pixelArray,
Copy link
Member

@mrserb mrserb Feb 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we need to do the same for the gtk2? I suggest to update one of the test below to cover gtk_robot on/off for both gtk2/gtk3.

Copy link
Member Author

@mkartashev mkartashev Feb 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe not as the previous fix was mode for gtk3 only, but I can double-check. Do you know if MATE desktop environment is a good candidate for finding gtk2 or are there easier options on Ubuntu?

Copy link
Contributor

@prrace prrace Feb 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea what MATE is but if you are saying you have a recent Ubuntu and you don't have GTK2, I am at least 95% sure GTK2 is still available from Ubuntu package manager for all current releases.

Copy link
Member Author

@mkartashev mkartashev Feb 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MATE desktop continues the "traditional" Gnome2 desktop development, so I thought it would be a good test for Gtk2 interfaces. FWIW, I ran the tests on such a system with DPI set to 200 (there's no other way to scale the interface under MATE AFAIK) and they all pass. However, the test executes the same code path as before going by if (gtk3_version_3_10) branch.

@prrace

GTK2 is still available from Ubuntu package manager for all current releases.

Right, but as long as gtk3 is also available, that's what we are going to use as AWT backend, see the code in gtk_interface.c.
I hacked around to forbid gtk3 from loading and the tests still passed (this time, without executing my changes).
I think gtk2 has no real notion of scale so the coordinates and size are always in screen pixels. See, for example, the documentation for gdk_pixbuf_get_from_drawable().

Copy link
Member

@mrserb mrserb Feb 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You do not need to hack the code, just add a "jdk.gtk.version" option to enable specific gtk version, this is what I suggested above. to disable the gtk usage in the robot the "awt.robot.gtk" can be used. So you can add that to some test to check that code paths works.

Copy link
Member Author

@mkartashev mkartashev Feb 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pointers, I'll set up a separate test for that.

int x, jint y, jint width, jint height, jint jwidth, int dx, int dy,
jint scale) {
Copy link
Member

@mrserb mrserb Feb 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this scale parameter is simply ignored? If the passed parameters are always in the device space and the array is allocated properly then we should not care about this scale(especially in case of gtk2)?

Copy link
Member Author

@mkartashev mkartashev Feb 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this scale parameter is simply ignored?

Looks like it. I can only find one call site of get_drawable_data() (in Java_sun_awt_X11_XRobotPeer_getRGBPixelsImpl()) and the scale parameter is always 1. I think I can drop it from the interface if you prefer.

Copy link
Member

@mrserb mrserb Feb 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes please, this "scale" just make the code complicated w/o reason.

Copy link
Member Author

@mkartashev mkartashev Feb 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's done; please, take a look at the updated code.

GdkPixbuf *pixbuf;
jint *ary;

int skip_left = 0;
int skip_top = 0;
GdkWindow *root = (*fp_gdk_get_default_root_window)();
if (gtk3_version_3_10) {
int win_scale = (*fp_gdk_window_get_scale_factor)(root);

// Scale the coordinate and size carefully such that the captured area
// is at least as large as requested. We trim off excess later by
// using the skip_* variables.
const int x_scaled = scale_down_to_minus_inf(x, win_scale);
const int y_scaled = scale_down_to_minus_inf(y, win_scale);
skip_left = x - x_scaled*win_scale;
skip_top = y - y_scaled*win_scale;
DASSERT(skip_left >= 0 && skip_top >= 0);

const int x_right_scaled = scale_down_to_plus_inf(x + width, win_scale);
const int width_scaled = x_right_scaled - x_scaled;
DASSERT(width_scaled > 0);

const int y_bottom_scaled = scale_down_to_plus_inf(y + height, win_scale);
const int height_scaled = y_bottom_scaled - y_scaled;
DASSERT(height_scaled > 0);

pixbuf = (*fp_gdk_pixbuf_get_from_drawable)(
root, x, y, (int) (width / (float) win_scale + 0.5), (int) (height / (float) win_scale + 0.5));
root, x_scaled, y_scaled, width_scaled, height_scaled);
} else {
pixbuf = (*fp_gdk_pixbuf_get_from_drawable)(root, x, y, width, height);
}
@@ -2906,7 +2936,8 @@ static gboolean gtk3_get_drawable_data(JNIEnv *env, jintArray pixelArray,
int index;
for (_y = 0; _y < height; _y++) {
for (_x = 0; _x < width; _x++) {
p = pix + (intptr_t) _y * stride + _x * nchan;
p = pix + (intptr_t) (_y + skip_top) * stride
+ (_x + skip_left) * nchan;

index = (_y + dy) * jwidth + (_x + dx);
ary[index] = 0xff000000
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2015, 2017, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2015, 2022, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -27,20 +27,26 @@
import java.awt.Frame;
import java.awt.Graphics;
import java.awt.Panel;
import java.awt.Point;
import java.awt.Rectangle;
import java.awt.Robot;
import java.awt.image.BufferedImage;
import javax.swing.UIManager;
import javax.imageio.ImageIO;
import java.io.File;
import java.io.IOException;

/**
* @test
* @key headful
* @bug 8073320
* @summary Windows HiDPI support
* @bug 8073320 8280861
* @summary Linux and Windows HiDPI support
* @author Alexander Scherbatiy
* @requires (os.family == "linux" | os.family == "windows")
* @run main/othervm -Dsun.java2d.win.uiScaleX=3 -Dsun.java2d.win.uiScaleY=2
* HiDPIRobotScreenCaptureTest
* @run main/othervm -Dsun.java2d.uiScale=1 HiDPIRobotScreenCaptureTest
* @run main/othervm -Dsun.java2d.uiScale=2 HiDPIRobotScreenCaptureTest
*/

public class HiDPIRobotScreenCaptureTest {
@@ -60,7 +66,14 @@ public static void main(String[] args) throws Exception {
}

Frame frame = new Frame();
frame.setBounds(40, 30, 400, 300);
// Position the frame on prime number coordinates to avoid
// them being multiple of the desktop scale; this tests Linux
// color picker better.
// Also, the position should be far enough from the top left
// corner of the screen to reduce the chance of being repositioned
// by the system because that area's occupied by the global
// menu bar and such.
frame.setBounds(83, 97, 400, 300);
frame.setUndecorated(true);

Panel panel = new Panel(new BorderLayout());
@@ -78,6 +91,13 @@ public void paint(Graphics g) {
g.fillRect(0, h / 2, w / 2, h / 2);
g.setColor(COLORS[3]);
g.fillRect(w / 2, h / 2, w / 2, h / 2);

// Several distinct pixels next to one another
// in order to test color picker's precision.
for (int i = 1; i < 4; i++) {
g.setColor(COLORS[i]);
g.fillRect(i, 0, 1, 1);
}
}
};

@@ -86,11 +106,15 @@ public void paint(Graphics g) {
frame.setVisible(true);
Robot robot = new Robot();
robot.waitForIdle();
Thread.sleep(200);
robot.delay(500);

final Point screenLocation = frame.getLocationOnScreen();
checkPixelColors(robot, screenLocation.x, screenLocation.y);

Rectangle rect = canvas.getBounds();
rect.setLocation(canvas.getLocationOnScreen());

System.out.println("Creating screen capture of " + rect);
BufferedImage image = robot.createScreenCapture(rect);
frame.dispose();

@@ -101,20 +125,53 @@ public void paint(Graphics g) {
throw new RuntimeException("Wrong image size!");
}

if (image.getRGB(w / 4, h / 4) != COLORS[0].getRGB()) {
throw new RuntimeException("Wrong image color!");
}

if (image.getRGB(3 * w / 4, h / 4) != COLORS[1].getRGB()) {
throw new RuntimeException("Wrong image color!");
}
checkRectColor(image, new Rectangle(0, 0, w / 2, h / 2), COLORS[0]);
checkRectColor(image, new Rectangle(w / 2, 0, w / 2, h / 2), COLORS[1]);
checkRectColor(image, new Rectangle(0, h / 2, w / 2, h / 2), COLORS[2]);
checkRectColor(image, new Rectangle(w / 2, h / 2, w / 2, h / 2), COLORS[3]);
}

static void checkPixelColors(Robot robot, int x, int y) {
for (int i = 0; i < 4; i++) {
final Color actualColor = robot.getPixelColor(x + i, y);
System.out.print("Checking color at " + (x + i) + ", " + y + " to be equal to " + COLORS[i]);
if (!actualColor.equals(COLORS[i])) {
System.out.println("... Mismatch: found " + actualColor + " instead");
throw new RuntimeException("Wrong screen pixel color");

if (image.getRGB(w / 4, 3 * h / 4) != COLORS[2].getRGB()) {
throw new RuntimeException("Wrong image color!");
} else {
System.out.println("... OK");
}
}
}

if (image.getRGB(3 * w / 4, 3 * h / 4) != COLORS[3].getRGB()) {
throw new RuntimeException("Wrong image color!");
private static final int OFFSET = 5;
static void checkRectColor(BufferedImage image, Rectangle rect, Color expectedColor) {
System.out.println("Checking rectangle " + rect + " to have color " + expectedColor);
final Point[] pointsToCheck = new Point[] {
new Point(rect.x + OFFSET, rect.y + OFFSET), // top left corner
new Point(rect.x + rect.width - OFFSET, rect.y + OFFSET), // top right corner
new Point(rect.x + rect.width / 2, rect.y + rect.height / 2), // center
new Point(rect.x + OFFSET, rect.y + rect.height - OFFSET), // bottom left corner
new Point(rect.x + rect.width - OFFSET, rect.y + rect.height - OFFSET) // bottom right corner
};

for (final var point : pointsToCheck) {
System.out.print("Checking color at " + point + " to be equal to " + expectedColor);
final int actualColor = image.getRGB(point.x, point.y);
if (actualColor != expectedColor.getRGB()) {
System.out.println("... Mismatch: found " + new Color(actualColor) + " instead. Check image.png.");
try {
ImageIO.write(image, "png", new File("image.png"));
} catch(IOException e) {
System.out.println("failed to save image.png.");
e.printStackTrace();
}
throw new RuntimeException("Wrong image color!");
} else {
System.out.println("... OK");
}
}
}
}
}
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2020, 2022, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -103,7 +103,7 @@ public void run() {
panel.add(passwordField, BorderLayout.CENTER);
frame = new JFrame("TestSelectedTextBackgroundColor");
frame.add(panel);
frame.setSize(200, 200);
frame.setSize(400, 400);
frame.setAlwaysOnTop(true);
frame.setLocationRelativeTo(null);
frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
@@ -103,7 +103,7 @@ public void run() {
panel.add(progressBar, BorderLayout.CENTER);
frame = new JFrame("TestSelectedTextBackgroundColor");
frame.add(panel);
frame.setSize(200, 200);
frame.setSize(400, 400);
frame.setAlwaysOnTop(true);
frame.setLocationRelativeTo(null);
frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2020, 2022, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -101,7 +101,7 @@ public void run() {
panel.add(slider);
frame = new JFrame("TestJSliderRendering");
frame.add(panel);
frame.setSize(200, 200);
frame.setSize(400, 400);
frame.setAlwaysOnTop(true);
frame.setLocationRelativeTo(null);
frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2020, 2022, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -119,7 +119,7 @@ public void run() {
panel.add(listModelSpinner, BorderLayout.CENTER);
frame = new JFrame("TestSelectedTextBackgroundColor");
frame.add(panel);
frame.setSize(200, 200);
frame.setSize(400, 400);
frame.setAlwaysOnTop(true);
frame.setLocationRelativeTo(null);
frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2020, 2022, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -96,7 +96,7 @@ public void run() {
panel.add(textPane, BorderLayout.CENTER);
frame = new JFrame("TestJTextPaneBackgroundColor");
frame.add(panel);
frame.setSize(200, 200);
frame.setSize(400, 400);
frame.setAlwaysOnTop(true);
frame.setLocationRelativeTo(null);
frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2020, 2022, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -134,7 +134,7 @@ public Point getToolTipLocation(MouseEvent event) {
panel.add(label, BorderLayout.CENTER);
frame = new JFrame("TestTooltipBackgroundColor");
frame.add(panel);
frame.setSize(200, 200);
frame.setSize(400, 400);
frame.setAlwaysOnTop(true);
frame.setLocationRelativeTo(null);
frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);