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

GC#drawIcon draws *.ico(for some icons) with black background on Windows 10 #185

Closed
tmssngr opened this issue Jun 1, 2022 · 32 comments · Fixed by #244
Closed

GC#drawIcon draws *.ico(for some icons) with black background on Windows 10 #185

tmssngr opened this issue Jun 1, 2022 · 32 comments · Fixed by #244
Assignees
Labels
fix verified The implemented fix has been verified with an I-build regression Something that used to work Windows Happens on Windows OS
Milestone

Comments

@tmssngr
Copy link
Contributor

tmssngr commented Jun 1, 2022

Describe the bug
Please run the attached snippet on Windows 10 (default Windows mode: Dark, default app mode: light). It will get the icons for certain file types and render them. For some reason, the background of the images is rendered in black.
ProgramImageTest.java.txt

To Reproduce
See above.

Expected behavior
The background should be transparent.

Screenshots
Screenshot-Win10

Environment:
Windows 10, Version 21H1 (OS Build 19043.1706)

@sravanlakkimsetti sravanlakkimsetti added the Windows Happens on Windows OS label Jun 1, 2022
@iloveeclipse
Copy link
Member

@tmssngr :

  • which Eclipse / SWT version is this?
  • is this a regression? If yes, which version was OK?

@tmssngr
Copy link
Contributor Author

tmssngr commented Jun 1, 2022

This is reproducible with the latest SWT built from source (v4952r11). With bisecting I've found out that commit 22797d3 Revert "Revert "Bug 493455 - [win32] remove alpha and alphaData from Image"" introduced this bug.

@niraj-modi
Copy link
Member

This is reproducible with the latest SWT built from source (v4952r11). With bisecting I've found out that commit 22797d3 Revert "Revert "Bug 493455 - [win32] remove alpha and alphaData from Image"" introduced this bug.

@jonathan.meier@outlook.com
Please have a look.

@lshanmug lshanmug added the regression Something that used to work label Jun 1, 2022
@niraj-modi
Copy link
Member

I don't see this problem happening in Eclipse.

@SyntevoAlex @Phillipus
Do you see this problem in any of you Eclipse based applications ?
We are just trying to understand how severe is this problem.

@Phillipus
Copy link
Contributor

Phillipus commented Jun 1, 2022

I don't have those program icons on my system, so when I run the snippet the program icons I have render OK. @tmssngr Can we get an actual icon to test with? Also, is this Hi DPI?

@niraj-modi
Copy link
Member

I don't have those program icons on my system, so when I run the snippet the program icons I have render OK. @tmssngr Can we get an actual icon to test with? Also, is this Hi DPI?

Please share a screen grab on how the icon looks for you.

@Phillipus
Copy link
Contributor

Phillipus commented Jun 1, 2022

Please share a screen grab on how the icon looks for you.

icons

@Phillipus
Copy link
Contributor

Phillipus commented Jun 1, 2022

Found it.

I had to set Windows custom scaling to 200 or run the snippet with the argument -Dswt.autoScale=200

icons2

@niraj-modi niraj-modi changed the title Program.getImageData returns images with black background on Windows 10 [HiDPI] Program.getImageData returns images with black background on Windows 10 Jun 1, 2022
@niraj-modi
Copy link
Member

Eclipse at HiDPI is working fine for External Program Icons:
Eclipse_ExternalProgram_Icons

So problem seems specific to this test-snippet only.

@Phillipus
Copy link
Contributor

Phillipus commented Jun 1, 2022

It seems to be only some Program icons that have this problem.

Try running this modified snippet:

Modified Snippet
import java.util.List;
import java.util.*;

import org.eclipse.swt.*;
import org.eclipse.swt.graphics.*;
import org.eclipse.swt.program.*;
import org.eclipse.swt.widgets.*;

public class ProgramImageTest {

    public static void main(String[] args) {
        final Display display = new Display();

        final List<Image> images = createImages(display);

        final Shell shell = new Shell(display);

        shell.addListener(SWT.Paint, event -> {
            int x = 0;
            int y = 0;
            for(Image image : images) {
                event.gc.drawImage(image, x, y);
                x += image.getImageData().width;
                x += 10;
                if(x > 400) {
                    x = 0;
                    y += 24;
                }
            }
        });

        shell.setSize(400, 300);
        shell.open();

        while(!shell.isDisposed()) {
            if(!display.readAndDispatch()) {
                display.sleep();
            }
        }

        for(Image image : images) {
            image.dispose();
        }

        display.dispose();
    }

    private static List<Image> createImages(Display display) {
        final List<Image> images = new ArrayList<>();

        for(Program p : Program.getPrograms()) {
            final ImageData data = p.getImageData();
            if(data == null) {
                continue;
            }

            images.add(new Image(display, data));
        }

        return images;
    }
}

@Phillipus
Copy link
Contributor

Strange, now I am able to reproduce this with scaling at 100%:

icons3

@niraj-modi
Copy link
Member

niraj-modi commented Jun 1, 2022

Interestingly problem is seen only with second run onward irrespective of zoom.
Even got it working with 200% as well:
Snippet_200Zoom

Revised steps to reproduce the issue with test-snippet:

  1. Switch the Windows Display zoom 100% to 200%(or visa versa)
  2. Run the test-snippet, it will works on either zoom.
  3. Close it and relaunch, problem is seen.

@tmssngr
Copy link
Contributor Author

tmssngr commented Jun 1, 2022

Just for the records: my 4k monitor (main screen) has 175% scaling, the notebook display (second screen) 125% scaling.

@Phillipus
Copy link
Contributor

I tried to narrow it down to loading an icon from file, but couldn't reproduce it other than the icons returned from Program.getImageData(). Perhaps there is something in the way that ImageData is created there that was missed in https://bugs.eclipse.org/bugs/show_bug.cgi?id=493455

@Phillipus
Copy link
Contributor

Phillipus commented Jun 1, 2022

As an experiment I changed line 366 of org.eclipse.swt.program.Program from:

int flags = OS.SHGFI_ICON | OS.SHGFI_SMALLICON | OS.SHGFI_USEFILEATTRIBUTES;

to:

int flags = OS.SHGFI_SMALLICON | OS.SHGFI_USEFILEATTRIBUTES;

And the icons are rendered OK.

Might be a clue?

@Phillipus
Copy link
Contributor

Phillipus commented Jun 1, 2022

This issue does manifest in Eclipse 4.24 Preferences:

eclipse 4 24

Icons are rendered correctly in Eclipse 4.23:

eclipse 4 23

@niraj-modi
Copy link
Member

As an experiment I changed line 366 of org.eclipse.swt.program.Program from:

int flags = OS.SHGFI_ICON | OS.SHGFI_SMALLICON | OS.SHGFI_USEFILEATTRIBUTES;

to:

int flags = OS.SHGFI_SMALLICON | OS.SHGFI_USEFILEATTRIBUTES;

And the icons are rendered OK.

Might be a clue?

With above suggested changes the transparent background issue is addressed but the size of icons is coming too small and also seems blurred:
Possible_Fix

@Phillipus
Copy link
Contributor

With above suggested changes the transparent background issue is addressed but the size of icons is coming too small and also seems blurred:

Yes. I was just looking for a clue.

It seems that the problem lies somewhere in the changes that were made as part of https://bugs.eclipse.org/bugs/show_bug.cgi?id=493455 Still seems to be some outstanding/unknown issues with this major change.

@lshanmug lshanmug added the HiDPI Issues related to High resolution monitors label Jun 2, 2022
@Phillipus
Copy link
Contributor

I don't think it's related to HiDPI. I'm seeing it on normal 100% scaling all the time now. See #185 (comment)

@Phillipus
Copy link
Contributor

Phillipus commented Jun 2, 2022

I've isolated the problem to some *.ico files. Nothing to do with Program.getImageData or HiDPI. So this regression will affect Eclipse and RCP apps that are displaying images in the Windows *ico format.

Attached is a reproducible snippet and icon file.

Here's how it should look running on Eclipse 4.23:

icon_4 23

Here's how it looks on Eclipse 4.24:

icon_4 24

ImageTest.zip

@niraj-modi niraj-modi changed the title [HiDPI] Program.getImageData returns images with black background on Windows 10 GC#drawIcon draws *.ico(for some icons) with black background on Windows 10 Jun 2, 2022
@niraj-modi niraj-modi removed the HiDPI Issues related to High resolution monitors label Jun 2, 2022
@Phillipus
Copy link
Contributor

changed the title [HiDPI] Program.getImageData returns images with black background on Windows 10 GC#drawIcon draws *.ico(for some icons) with black background on Windows 10

GC#drawImage

@lshanmug
Copy link
Member

lshanmug commented Jun 2, 2022

Thanks Phil for investigating and narrowing down the issue!

@jonathan-meier
Copy link
Contributor

The problem is here

case 32:
newDepth = 24;
newPalette = new PaletteData(0xFF, 0xFF00, 0xFF0000);
break;

In commit 22797d3 I changed this from

case 32:
	if (!(redMask == 0xFF00 && greenMask == 0xFF0000 && blueMask == 0xFF000000)) {
		newPalette = new PaletteData(0xFF00, 0xFF0000, 0xFF000000);
	}
	break;

enforcing that any alpha data for an image must be provided via the ImageData.alphaData field and any alpha data in the fourth channel of the ImageData.data field for 32-bit images is strictly ignored. I did this to align the win32 implementation with the gtk implementation of Image, however, I forgot to consider the special case of icons (loaded from ICO files) that provide a 32-bit image and a mask.

For such icons, retrieving their ImageData via Image.getImageData (this is used inside Program.getImageData) does not populate the ImageData.alphaData field, even though the fourth channel of the image data contains valid alpha data and therefore, the alpha data is discarded when again creating an Image from this ImageData.

This becomes a problem for icons that do not use the color black for transparent pixels. Masked pixels colored in black become transparent, whereas masked pixels colored in any other color are XORed with their background color by GDI, which means that a masked pixel colored in white inverts its background color. This is exactly what happens in all examples provided above, where the white background is inverted to black by masked pixels colored in white. It happens only with a few icons, since most icons use the color black for transparent pixels.

Using the following method, you can color all masked pixels black to work around this issue:

private static void maskColor(ImageData imageData) {
	byte[] maskData = imageData.maskData;
	if (maskData == null || imageData.depth != 32) {
		return;
	}

	byte[] data = imageData.data;
	int stride = maskData.length / imageData.height;
	for (int i = 0, dp = 0, mbp = 0; i < imageData.height; i++, mbp += stride) {
		for (int j = 0, mp = mbp; j < imageData.width; mp++) {
			for (int b = 7; b >= 0 && j < imageData.width; b--, j++, dp++) {
				int mask = (maskData[mp] >> b) & 0x1;
				for (int c = 0; c < 3; c++, dp++) {
					data[dp] = (byte) (data[dp] * mask);
				}
			}
		}
	}
}

This still does not fix partially transparent pixels which are just drawn fully opaque when discarding the alpha channel, but this is much less visible and icons using partially transparent pixels are less common.

I will provide a PR with a fix for this issue as soon as possible. Thanks everyone for providing insights helping debug this issue 👍

@Phillipus
Copy link
Contributor

Phillipus commented Jun 16, 2022

I will provide a PR with a fix for this issue as soon as possible. Thanks everyone for providing insights helping debug this issue

@jonathan-meier Hi, did you find some time to implement a PR? Happy to test it if required. 👍

@tmssngr
Copy link
Contributor Author

tmssngr commented Jul 1, 2022

@jonathan-meier Unfortunately, your maskColor method doesn't resolve the problem for me (Windows 10).

@Phillipus
Copy link
Contributor

This is reproducible with the latest SWT built from source (v4952r11). With bisecting I've found out that commit 22797d3 Revert "Revert "Bug 493455 - [win32] remove alpha and alphaData from Image"" introduced this bug.

@tmssngr Do you think if we reverted this, it would solve the regression?

@tmssngr
Copy link
Contributor Author

tmssngr commented Jul 5, 2022

I've tried reverting e891658, 819a24e, def5c63 and 22797d3, but then it did not compile any more because of a missing hDC in Image. Because I'm not an expert in the internals, I've aborted my tests.

@tmssngr
Copy link
Contributor Author

tmssngr commented Jul 7, 2022

I really would appreciate it if this obvious regression could be fixed soon.

@Phillipus
Copy link
Contributor

I really would appreciate it if this obvious regression could be fixed soon.

@jonathan-meier Hi Jonathan, do you think it is possible to fix this regression?

@niraj-modi
Copy link
Member

The problem is here

case 32:
newDepth = 24;
newPalette = new PaletteData(0xFF, 0xFF00, 0xFF0000);
break;

In commit 22797d3 I changed this from

case 32:
	if (!(redMask == 0xFF00 && greenMask == 0xFF0000 && blueMask == 0xFF000000)) {
		newPalette = new PaletteData(0xFF00, 0xFF0000, 0xFF000000);
	}
	break;

Based on above input, I have a working fix for this issue:

  • Will share a PR for testing shortly.

niraj-modi added a commit to niraj-modi/eclipse.platform.swt that referenced this issue Jul 7, 2022
Windows10
- Fixes eclipse-platform#185

Change-Id: Idec2ddb0086a58c8d28c7616a628e2802d06ce56
Signed-off-by: Niraj Modi <niraj.modi@in.ibm.com>
@Phillipus
Copy link
Contributor

@niraj-modi Hi Niraj, thanks for taking a look at this. I tested your patch with the example case in #185 (comment) and I can confirm it seems to fix the problem.

niraj-modi added a commit that referenced this issue Jul 11, 2022
Windows10
- Fixes #185

Change-Id: Idec2ddb0086a58c8d28c7616a628e2802d06ce56
Signed-off-by: Niraj Modi <niraj.modi@in.ibm.com>
@niraj-modi niraj-modi self-assigned this Jul 11, 2022
@niraj-modi niraj-modi added this to the 4.25 M2 milestone Jul 11, 2022
@tmssngr
Copy link
Contributor Author

tmssngr commented Jul 11, 2022

Thanks for the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix verified The implemented fix has been verified with an I-build regression Something that used to work Windows Happens on Windows OS
Projects
None yet
7 participants