Bug 493455 - [win32][cocoa] GC#drawImage(..) doesn't draw alpha data from given image
Summary: [win32][cocoa] GC#drawImage(..) doesn't draw alpha data from given image
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: SWT (show other bugs)
Version: 4.6   Edit
Hardware: All All
: P3 major with 1 vote (vote)
Target Milestone: 4.24 M1   Edit
Assignee: Jonathan Meier CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on: 578710
Blocks: 495417
  Show dependency tree
 
Reported: 2016-05-11 13:53 EDT by Markus Keller CLA
Modified: 2022-06-01 06:09 EDT (History)
13 users (show)

See Also:


Attachments
SnippetDrawAlpha.java (3.05 KB, text/plain)
2016-05-11 13:53 EDT, Markus Keller CLA
no flags Details
Snippet for two-pass image scaling (4.30 KB, text/plain)
2017-04-03 09:58 EDT, András Péteri CLA
no flags Details
Java editor ruler rendering regression (155.75 KB, image/png)
2022-02-11 11:59 EST, Jonathan Meier CLA
no flags Details
SnippetDrawAlphaTwoPass padding fix (2.83 KB, patch)
2022-02-12 08:46 EST, Jonathan Meier CLA
no flags Details | Diff
Snippet and Image (1001 bytes, application/octet-stream)
2022-02-13 09:09 EST, Phil Beauvoir CLA
no flags Details
Screenshot showing the problem (7.92 KB, image/png)
2022-02-13 09:13 EST, Phil Beauvoir CLA
no flags Details
Snippet Premultiplied Alpha Regression (1.52 KB, application/x-zip-compressed)
2022-02-13 23:39 EST, Jonathan Meier CLA
no flags Details
Snippet to reproduce painting problem (1.73 KB, application/x-zip-compressed)
2022-03-29 13:28 EDT, Thomas Singer CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Markus Keller CLA 2016-05-11 13:53:35 EDT
Created attachment 261660 [details]
SnippetDrawAlpha.java

Follow-up to bug 97506 comment #11. Probably a long-standing bug in SWT on win32.

In a GC that was created on an existing fully-transparent Image, call GC#drawImage(..).
* Expected: The passed image should be drawn visibly on the the GC.
* Was: On Windows, the image stays fully transparent. On other platforms, the image is drawn as expected.

* Steps:

Save the attached snippet into the org.eclipse.swt.snippets project and then run it using on Windows 7 with 100% resolution (to avoid problems with HiDPI support).

Only the first column of images is drawn, the rest stays empty. If you uncomment the code marked with FIXME, you can see that the scaling of the image works fine, but the alphaData stays fully transparent.
Comment 1 Markus Keller CLA 2016-05-11 14:02:16 EDT
> On other platforms, the image is drawn as expected.

Actually, it only works as expected on GTK (Ubuntu 14.04 with GTK 2 and 3).
Cocoa has the same problem as win32.
Comment 2 András Péteri CLA 2017-04-03 09:58:04 EDT
Created attachment 267610 [details]
Snippet for two-pass image scaling

A possible workaround (tested on Windows only) is to scale the image mask/alpha channel as a separate image.
Comment 3 Eclipse Genie CLA 2018-02-11 13:28:56 EST
New Gerrit change created: https://git.eclipse.org/r/117121
Comment 4 Conrad Groth CLA 2018-02-11 13:34:41 EST
I investigated the problem and found out, that the alphaData of the first two images isn't modified. The Gerrit patch shows one code place where the alphaData is adjusted. This has to be done also in the hasAlphaBlendSupport-branch.

The last three images in the SnippetDrawAlpha have a transparent pixel. This should be reflected in the snippet like that:
<pre>
/* Create a image with the same depth and palette */
final ImageData resultData = new ImageData(scaledWidth, scaledHeight, imageData.depth, imageData.palette);
if (imageData.alphaData != null) {
	resultData.alphaData = new byte[scaledWidth * scaledHeight];
} else if (imageData.transparentPixel != -1) {
	resultData.transparentPixel = imageData.transparentPixel;
}
resultData.type = imageData.type;
</pre>
Comment 5 Conrad Groth CLA 2018-02-11 13:38:20 EST
The alphaData fix works.

The transparent-pixel images currently cause an exception in the native call GetDIBColorTable in line 1596 of GC.java. I will investigate that problem in the next days.
Comment 6 Conrad Groth CLA 2018-02-12 17:18:20 EST
I updated the patch so that it works for the PNG files. But to be honest the scaled images look very bad.
Comment 7 Niraj Modi CLA 2018-02-21 11:15:36 EST
(In reply to Conrad Groth from comment #6)
> I updated the patch so that it works for the PNG files. But to be honest the
> scaled images look very bad.

Tried the latest patch and confirm above. In order to further improve the scaled image quality, we are open to any possible options if any exists in native API.
Comment 8 Conrad Groth CLA 2018-03-03 08:13:53 EST
If the patch solves any problem we should merge it. The scaled image quality is part of bug 97506.
Comment 9 Lars Vogel CLA 2018-04-20 03:28:05 EDT
(In reply to Conrad Groth from comment #8)
> If the patch solves any problem we should merge it. 

Can we merge the patch to get this closed?
Comment 10 Eclipse Genie CLA 2022-02-03 19:53:27 EST
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/190395
Comment 11 Jonathan Meier CLA 2022-02-03 19:57:09 EST
I took a stab at solving this long standing issue, see my Gerrit patch above.

As Conrad Groth already outlined earlier, the alphaData of an Image that is used as a drawable for a GC is not modified when running SnippetDrawAlpha from the attachements. Specifically, the alphaData is not properly kept in sync with the underlying win32 BITMAP. Using drawImage on a GC backed by an Image (as demonstrated in the snippet) directly draws into the win32 BITMAP with GDI+ and never touches alphaData. When reading back the image data from the BITMAP in Image.getImageDataAtCurrentZoom, one can see that the alpha channel in the returned ImageData.data array is correct, but ImageData.alphaData is simply copied from the alphaData the Image initially was constructed with instead of syncing it back from the alpha channel in ImageData.data.

Conrad Groth took the approach of trying to keep the alphaData in sync with the underlying BITMAP in GC's drawImage method. However, I fully agree with Nikita Nemkin's conclusion on the patch (second last comment on the Gerrit patch: https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/117121), this approach is fragile. It does not scale well, since it is hard if not impossible to keep alphaData properly in sync with the underlying BITMAP. GDI+ supports alpha for various drawing operations, so drawImage is by far not the only method that would have to be augmented with code keeping up with the alphaData synchronization. Therefore, the right approach is indeed to get rid of alphaData (and alpha for that matter) to have a single source of truth regarding alpha data, which is the alpha channel of the underlying BITMAP.

The patch I provide does exactly this. A few notes regarding its implementation:

* As soon as an Image is created from ImageData that contains alphaData or has alpha set to something different than -1, the ImageData is immediately converted to a 32-bit RGBA image in pre-multiplied alpha format, which is the only image format that can deal with alpha data in GDI/GDI+. This is new. Previously, the underlying BITMAP was only constructed with 32-bit RGBA when ImageData provided the data with depth 32 already.

* An Image constructed from ImageData with depth 32 that has no alphaData and alpha is set to -1 is automatically converted to a 24-bit RGB image. This is potentially a breaking change on Windows, since 32-bit image data always constructed a 32-bit BITMAP before, regardless of alphaData and alpha. However, I'd argue this lenience was an oversight and with my patch the behavior on Windows is now aligned with the behavior on Linux/GTK.

* BITMAP's with alpha data always have to be in pre-multiplied alpha format. This is the input format that GDI/GDI+ expects and the output format GDI/GDI+ produces for all drawing operations that consider the alpha channel.

* Some GDI methods ignore the alpha data completely and even worse, they reset all alpha data to 0. These cases are detected in Image.getImageDataAtCurrentZoom where we checked whether the image is in proper pre-multiplied alpha format (all R, G and B values are at most as large as the alpha value). If it is not, we return a fully opaque image with no alpha data.

Final note: It's quite hard to implement such a fundamental change and at the same time keeping all the code backwards compatible. I did my best, but there's exotic areas such as drawing to printers with only partial alpha blending support etc., of which I might have easily missed a few cases. Please be critical in the review, I'm happy to discuss!
Comment 13 Phil Beauvoir CLA 2022-02-11 05:50:33 EST
Has this patch been tested on Hi DPI setting (200% scale)? 

It probably does work OK, but some previous changes in SWT needed special treatment at high DPI scaling so I thought I'd flag it as something to test.
Comment 14 Eclipse Genie CLA 2022-02-11 11:53:43 EST
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/190727
Comment 15 Jonathan Meier CLA 2022-02-11 11:59:46 EST
Created attachment 288030 [details]
Java editor ruler rendering regression

I just realized that my change introduced a regression but luckily I can provide a relatively small fix (see new Gerrit patch above). Sorry for not catching that earlier!

Empty images, that is images constructed only from a width and a height (using the Image(Device device, int width, int height) or the Image(Device device, Rectangle bounds) constructor), may now be treated as RGBA if the bitmap created in init(int width, int height) happens to be in a 32-bit format (this depends on the system and the image size). This was not the case before since empty images did not have alpha or alphaData set and therefore they were always treated as opaque images.

The regression manifests itself for instance in a broken rendering of the ruler in the Java file editor (the small vertical band region to the left of editor content containing the line number etc., see attached screenshot), best visible with the dark theme. This can happen anywhere an Image created using one of the two above mentioned constructors is drawn onto a GC.

The fix needs to consider the two different cases that can happen in init(int width, int height):

- When the creation of the device-dependent bitmap (DDB) using OS.CreateCompatibleBitmap(hDC, width, height) fails, we need to make sure that we create a device-independent bitmap (DIB) with depth less than 32.

- Otherwise, we need to make sure that everywhere in the Image and GC class device-dependent bitmaps (DDBs) are always treated as opaque. While transparent DDBs in RGBA format are technically supported by GDI/GDI+, SWT fortunately never uses DDBs with an alpha channel anywhere. The only two places where an Image based on a DDB could be constructed are:
* in Image.init(int width, int height) where we know the intended format is RGB
* in Image.win32_new(Device device, int type, long handle) where we simply retrieve a handle to the underlying BITMAP and therefore can't tell the caller's intended interpretation of the BITMAP. However, inspecting all of the call sites of Image.win32_new we see that we're either passed an ICON handle (we are only concerned about BITMAP handles) or a DIB. Moreover, Image.win32_new is marked as not being part of the public API. So we are lucky and can conclude that there's indeed no code trying to work with transparent DDBs.
Comment 16 Jonathan Meier CLA 2022-02-11 12:19:21 EST
(In reply to Phil Beauvoir from comment #13)
> Has this patch been tested on Hi DPI setting (200% scale)? 
> 
> It probably does work OK, but some previous changes in SWT needed special
> treatment at high DPI scaling so I thought I'd flag it as something to test.

Thanks for mentioning it! Actually, HiDPI was the main motivation for me to develop this patch. The change makes Eclipse on Windows work with the smooth auto scaling setting for images.

Previously, auto scaling images only worked with the nearest neighbor interpolation on Windows. This can be tested for example by passing the argument -Dswt.autoScale=150 to the VM when running Eclipse, which will scale the UI by a factor of 1.5x. This correctly scales the images (such as button icons, etc.) in the Eclipse UI, but they look pixelated due to the nearest neighbor interpolation. To enable smooth scaling, we can additionally pass the argument -Dswt.autoScale.method=SMOOTH. Before my change, this made all scaled images disappear in the Eclipse UI. Now they all appear smoothly scaled. They still don't look great, but that's another issue.

Note that with -Dswt.autoScale=200 or on any HiDPI screen without the -Dswt.autoScale argument, you won't see any difference, because by default Eclipse only supports the zoom factors 1.0x and 2.0x and provides a separate version of the image/icon for both of these scale factors.
Comment 17 Phil Beauvoir CLA 2022-02-11 12:21:36 EST
Thanks for the explanation, Jonathan, appreciate it.
Comment 19 Eclipse Genie CLA 2022-02-12 07:12:44 EST
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/190739
Comment 20 Andrey Loskutov CLA 2022-02-12 07:25:15 EST
(In reply to Eclipse Genie from comment #18)
> Gerrit change
> https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/190727 was
> merged to [master].
> Commit:
> http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/
> ?id=dbdc293578b21c6aa23f33d9a3175f4f457f31d0

I've merged this even if I don't understand much from Win32 imaging because it was causing bug 578710. Latest few SDK builds are unusable for debugging on Windows, and this prevents tests & validation of other patches on that platform.

@Jonathan: next time you see there is a severe regression (like in comment 15), please don't just wait till someone per occasion will stumble over that (like me), warn actively on platform mailing list about broken SDK state if your patch isn't noticed & merged in time.

(In reply to Eclipse Genie from comment #19)
> New Gerrit change created:
> https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/190739

Just a manual test for the fixes on this bug. Please check what happens with transparent.png in SnippetDrawAlphaTwoPass - seem to be unrelated to the new patches, but might be there is something else still broken with alpha.
Comment 22 Jonathan Meier CLA 2022-02-12 08:46:19 EST
Created attachment 288034 [details]
SnippetDrawAlphaTwoPass padding fix

(In reply to Andrey Loskutov from comment #20)
> 
> @Jonathan: next time you see there is a severe regression (like in comment
> 15), please don't just wait till someone per occasion will stumble over that
> (like me), warn actively on platform mailing list about broken SDK state if
> your patch isn't noticed & merged in time.

My apologies for not making more noise! I'll definitely remember this for the next time. I'm still learning about the processes as this is my first bigger contribution, so thank you for your guidance, appreciate it. Super happy how open you are towards contributions and impressed how quickly they are considered :-)

> Just a manual test for the fixes on this bug. Please check what happens with
> transparent.png in SnippetDrawAlphaTwoPass - seem to be unrelated to the new
> patches, but might be there is something else still broken with alpha.

I looked into the crash of this snippet. It is indeed unrelated to my changes as it already crashed before. As far as I can tell, both the crash itself as well as the wrongly displayed alpha values for scale factors 1 and 2 after fixing the crash are due to wrong padding of the alpha data in the snippet itself. Alpha data should never be padded (i.e. pad is always 1 byte). Properly setting the scanlinePad to 1 when creating imageMaskData from the original image alpha data fixes the crash. Converting back to a scanlinePad of 1 from the padded scaledResultMaskData when setting the alpha data of scaledResult fixes the wrong transparency for scale factors 1 and 2.
Comment 23 Jonathan Meier CLA 2022-02-12 08:48:52 EST
A patch to fix the snippet in question is attached to the previous comment.
Comment 24 Andrey Loskutov CLA 2022-02-12 08:50:39 EST
(In reply to Jonathan Meier from comment #23)
> A patch to fix the snippet in question is attached to the previous comment.

Could you please push updated snippet to gerrit?

These were manual tests. Do you think you could write some regression test for the fix? Breaking image rendering is easy :)
Comment 25 Eclipse Genie CLA 2022-02-12 09:03:48 EST
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/190741
Comment 26 Jonathan Meier CLA 2022-02-12 09:06:01 EST
(In reply to Andrey Loskutov from comment #24)
> 
> Could you please push updated snippet to gerrit?

Done!

> These were manual tests. Do you think you could write some regression test
> for the fix? Breaking image rendering is easy :)

As demonstrated nicely by yours truly :D Yeah, I think I could come up with a couple of JUnit regression tests. I'll add them here as well once I have them ready.
Comment 28 Phil Beauvoir CLA 2022-02-13 08:40:46 EST
This has caused a regression in our RCP app, Archi.

At this stage I'm not sure how to create a short snippet to show you the problem, but here's what's happening:

We create different coloured folder icons by replacing the pixel data in an image data. With this change the pixels are not being changed.

We have an ImageDescriptor sub-class that over-rides getImageData(int zoom) and replaces the pixel of the image data like this:

int pixel = imageData.palette.getPixel(rgb);
for(int x = 0; x < imageData.width; x++) {
    for(int y = 0; y < imageData.height; y++) {
        imageData.setPixel(x, y, pixel);
    }
}

The full code is here - https://github.com/archimatetool/archi/blob/1c61b75c2afbe0e5145d3df312260f38ac978593/com.archimatetool.editor/src/com/archimatetool/editor/ui/ImageFactory.java#L264

Can you say what might cause this regression?
Comment 29 Phil Beauvoir CLA 2022-02-13 09:09:49 EST
Created attachment 288035 [details]
Snippet and Image

Actually, ignore Comment #28

Attached is a zip file containing a stand-alone Snippet and an image PNG that we use in our app for a folder.

Run the snippet and note the color of the folder icon compared with the original PNG file.

The image is rendered differently in the Tree.
Comment 30 Phil Beauvoir CLA 2022-02-13 09:13:39 EST
Created attachment 288036 [details]
Screenshot showing the problem

Screenshot showing the problem
Comment 31 Andrey Loskutov CLA 2022-02-13 09:16:40 EST
(In reply to Phil Beauvoir from comment #28)
> This has caused a regression in our RCP app, Archi.

@Jonathan: we should have a fix till tomorrow evening, because we are in M3 week, see
https://www.eclipse.org/lists/platform-releng-dev/msg38175.html

Without a fix or at least assessment how big is the impact of this regression, I would vote for reverting of the patches here, so that we can ship 4.23 without regressions, and continue work in 4.24.
Comment 32 Phil Beauvoir CLA 2022-02-13 09:47:36 EST
I don't want to sound unappreciative of the work in this patch, but it seems to be quite a major change that has a big impact and could have other side effects. It was only by chance that I tested the latest I build, and only by chance that we use a certain PNG image that shows the problem.

There may be other issues not yet discovered. Therefore, would it be best to revert this?

But even if this issue is fixed, how can we be sure there are no more side-effects and regressions from an edge case not yet discovered?
Comment 33 Andrey Loskutov CLA 2022-02-13 10:06:00 EST
(In reply to Phil Beauvoir from comment #32)
> I don't want to sound unappreciative of the work in this patch, but it seems
> to be quite a major change that has a big impact and could have other side
> effects. It was only by chance that I tested the latest I build, and only by
> chance that we use a certain PNG image that shows the problem.
> 
> There may be other issues not yet discovered. Therefore, would it be best to
> revert this?

Imaging (especially on Windows) is not my area of interest, so unfortunately I can't say anything about the patch itself. From our experience with bug 545032 I would say changing internal image processing algorithms is dangerous in the sense that if the error is typically doesn't show up immediately as crash, it requires a human to detect the color/transparency shift etc - tests can't "see".

> But even if this issue is fixed, how can we be sure there are no more
> side-effects and regressions from an edge case not yet discovered?

There are no guarantees for anything. Code reviews, regressions tests & testing early nightly builds is the only source of the confidence we have.

One worrying point is: most contributors work on Linux/Mac, so there is almost no human testing of nightly Windows builds with this patch.

From this point of view it would probably make sense to revert for M3.

I would wait for the answer from Jonathan, Niraj and Nikita - they wrote/reviewed the patch - what is their confidence in changes.
Comment 34 Jonathan Meier CLA 2022-02-13 10:33:48 EST
(In reply to Phil Beauvoir from comment #29)
> Created attachment 288035 [details]
> Snippet and Image
> 
> Actually, ignore Comment #28
> 
> Attached is a zip file containing a stand-alone Snippet and an image PNG
> that we use in our app for a folder.
> 
> Run the snippet and note the color of the folder icon compared with the
> original PNG file.
> 
> The image is rendered differently in the Tree.

Thanks for the reproducer, I will have a closer look later today to assess the severity of the regression. There‘s also the possibility that my change actually exposes a bug that existed before in the PNG loader that just did not surface before.

I am also generally not opposed to revert the patch, as it is quite a fundamental change indeed.
Comment 35 Eclipse Genie CLA 2022-02-13 23:35:06 EST
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/190757
Comment 36 Jonathan Meier CLA 2022-02-13 23:39:24 EST
Created attachment 288039 [details]
Snippet Premultiplied Alpha Regression

I found the issue. My change switched transparent images from being stored in straight alpha format to premultiplied alpha format. This is because most win32 APIs working with transparent bitmaps expect them to be in premultiplied alpha format. However, a few APIs do the alpha multiplication themselves and therefore expect passed bitmaps to be in straight format. This is terribly confusing and unfortunately heavily underdocumented by Microsoft.

There are two places in SWT where APIs expecting straight alpha format are used: When creating icons from bitmaps and when adding images to an ImageList. In these cases we have to explicitly convert back from premultiplied to straight alpha format. This is what I missed to do in my initial patch. Since tree items use an ImageList internally, they are affected by the regression as reported by Phil Beauvoir. There are a couple of other controls, such as toolbars and buttons that are affected as well.

Moreover, I also found the reverse case: Images for menu items are expected to be in premultiplied alpha format. Since images were stored in straight alpha format before, they had to be explicitly converted to premultiplied alpha format. I missed to remove that now superfluous conversion as well and therefore already premultiplied RGB values were multiplied again.

I fixed all of the above and attached a new snippet instantiating all affected controls.

Note that interpreting premultiplied alpha data as straight alpha data leads to transparent regions appearing darker. The effect increases with decreasing alpha. This is exactly what we can observe in the screenshot provided by Phil Beauvoir, where the heavily transparent inner region of the folder icon is drawn way too dark.
Comment 37 Niraj Modi CLA 2022-02-14 05:37:31 EST
Since this bug touches something that's very fundamental to image usage and at the same time issues may be difficult to notice.

So, I plan to revert below two code patch here:
https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/190727
https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/190395

IMO, we can get these fixes in master early M1 4.24 and giving it full release cycle for getting tested.
Comment 38 Eclipse Genie CLA 2022-02-14 05:39:17 EST
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/190414
Comment 40 Eclipse Genie CLA 2022-02-14 05:40:57 EST
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/190415
Comment 42 Jonathan Meier CLA 2022-02-15 06:18:39 EST
Thanks for taking care of it, Niraj, and sorry for the trouble. I'm not in a rush with this patch, so let's get back to it for 4.24 :-)
Comment 43 Eclipse Genie CLA 2022-03-11 09:50:50 EST
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/190430
Comment 44 Eclipse Genie CLA 2022-03-21 08:57:58 EDT
New Gerrit change created: https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/191861
Comment 48 Niraj Modi CLA 2022-03-22 03:58:20 EDT
(In reply to Jonathan Meier from comment #42)
> Thanks for taking care of it, Niraj, and sorry for the trouble. I'm not in a
> rush with this patch, so let's get back to it for 4.24 :-)

Thanks Jonathan for the fix, all the required patches have gone into master now.
Fix can be found on below 4.24 Eclipse IBuild & on-wards:
https://download.eclipse.org/eclipse/downloads/drops4/I20220321-1800/
Comment 49 Lars Vogel CLA 2022-03-22 11:54:46 EDT
In my fresh I-build eclipse.buildId=4.24.0.I20220321-1800 I see the following crash:

org.eclipse.swt.SWTError: No more handles
	at org.eclipse.swt.SWT.error(SWT.java:4944)
	at org.eclipse.swt.SWT.error(SWT.java:4833)
	at org.eclipse.swt.SWT.error(SWT.java:4804)
	at org.eclipse.swt.internal.ImageList.copyWithAlpha(ImageList.java:139)
	at org.eclipse.swt.internal.ImageList.set(ImageList.java:396)
	at org.eclipse.swt.internal.ImageList.add(ImageList.java:52)
	at org.eclipse.swt.widgets.Button.updateImageList(Button.java:1024)
	at org.eclipse.swt.widgets.Button.enableWidget(Button.java:506)
	at org.eclipse.swt.widgets.Control.setEnabled(Control.java:3420)
	at org.eclipse.ui.internal.views.log.EventDetailsDialog.updateButtons(EventDetailsDialog.java:398)
	at org.eclipse.ui.internal.views.log.EventDeta

Is this related to the work here?
Comment 50 Jonathan Meier CLA 2022-03-22 20:15:26 EDT
It could be related, though I don't immediately see how and I can't reproduce it either. The Event Details dialog opens without any issues for me with build 4.24.0.I20220321-1800. I even stepped through the code in the debugger to verify that I get the same stack trace, but the error was not raised.

Do you have any more hints that might help reproduce this crash?
Comment 51 Thomas Singer CLA 2022-03-29 09:16:11 EDT
On March 21 Niraj Modi reverted commits

22797d3f: Revert "Revert "Bug 493455 - [win32] remove alpha and alphaData from Image""
def5c630: Revert "Revert "Bug 493455 - [win32] always treat empty images as opaque""

which causes problems in our owner-drawn tables that images are not drawn correctly any more (we use a large image where only a portion is drawn as file icon in a Table). With bisecting I've found out that everything is drawn fine with commit 1bb1a46e, but with commit 22797d3f it is broken.
Comment 52 Andrey Loskutov CLA 2022-03-29 10:03:53 EDT
(In reply to Thomas Singer from comment #51)
> which causes problems in our owner-drawn tables that images are not drawn
> correctly any more

@Thomas: could you please attach SWT snippet that shows the problem?
Comment 53 Thomas Singer CLA 2022-03-29 13:28:19 EDT
Created attachment 288322 [details]
Snippet to reproduce painting problem
Comment 54 Jonathan Meier CLA 2022-03-29 14:01:59 EDT
Thanks a lot for the reproducer snippet, that helped a lot! Indeed, the source position is ignored (i.e. set to 0,0, see [1]) when drawing a transparent image. I will provide a fix. Sorry about that!

[1] see the 7th and 8th argument to OS.AlphaBlend being just 0 instead of srcX and srcY: https://github.com/eclipse-platform/eclipse.platform.swt/commit/22797d3f9f09ca67bfd29c864f2aaf8eb452c75c#diff-123cdea57a765f8a9bfcbf420155fae82a328a463a11862df704f5e5badb33e6R1288
Comment 55 Alexandr Miloslavskiy CLA 2022-04-05 21:10:20 EDT
This patch caused a regression:
https://github.com/eclipse-platform/eclipse.platform.swt/issues/21
Comment 56 Niraj Modi CLA 2022-04-06 07:15:36 EDT
(In reply to Alexandr Miloslavskiy from comment #55)
> This patch caused a regression:
> https://github.com/eclipse-platform/eclipse.platform.swt/issues/21

Thanks to Alexandr, above issue is fixed now.
Comment 57 Niraj Modi CLA 2022-04-06 07:28:14 EDT
There is a JUnit(test_bug493455_drawImageAlpha_srcPos) failure seen on MAC with latest Eclipse IBuild:
https://download.eclipse.org/eclipse/downloads/drops4/I20220405-1800/testresults/html/org.eclipse.swt.tests_ep424I-unit-mac64-java17_macosx.cocoa.x86_64_17.html
Comment 58 Andrey Loskutov CLA 2022-04-07 03:31:29 EDT
(In reply to Niraj Modi from comment #57)
> There is a JUnit(test_bug493455_drawImageAlpha_srcPos) failure seen on MAC
> with latest Eclipse IBuild:
> https://download.eclipse.org/eclipse/downloads/drops4/I20220405-1800/
> testresults/html/org.eclipse.swt.tests_ep424I-unit-mac64-java17_macosx.cocoa.
> x86_64_17.html

I've created https://github.com/eclipse-platform/eclipse.platform.swt/issues/30.
Comment 59 Niraj Modi CLA 2022-04-07 06:08:53 EDT
IMO, we can close this bug now. Failing JUnits issue is tracked via below github issue:
https://github.com/eclipse-platform/eclipse.platform.swt/issues/30

Thanks Jonathan for the fix, marking as resolved.
Comment 60 Niraj Modi CLA 2022-06-01 06:09:48 EDT
Looks like this issue cause one regression in SWT:
https://github.com/eclipse-platform/eclipse.platform.swt/issues/185

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