Community
Participate
Working Groups
Implement a native version of ImageLoader, using GdkPixbuf API.
New Gerrit change created: https://git.eclipse.org/r/138185
(In reply to Eclipse Genie from comment #1) > New Gerrit change created: https://git.eclipse.org/r/138185 There is a bug in GTK where GdkPixbufLoader does not work with small images, which is preventing us from loading small images through InputStream. See https://gitlab.gnome.org/GNOME/gtk/issues/1725 But large images, or loading small images through filenames work.
Notes on differences between using Java implementation of ImageLoader v.s. using native GdkPixbuf API 1) ImageLoader.load(InputStream stream) - Java implementation: reads byte array from stream one chunk at a time depending on image type -> convert chunk to ImageData using different subclasses of FileFormat - GdkPixbuf implementation: reads byte array from stream all at once -> use GdkPixbufLoader to load image -> get GdkPixbuf from GdkPixbufLoader -> convert GdkPixbuf into ImageData. - Possible limitation: memory overload with extremely large images (largest tested is 4000x2000 so there shouldn't too much use cases), alternative solution is to read byte array from stream one buffer at a time -> copies byte array into buffer in memory -> use GdkPixbufLoader to load image a small chunk at a time. However, this will make loading images extremely slow. 2) ImageLoader.load(String filename) - Java implementation: open file to InputStream -> call load(InputStream) - GdkPixbuf implementation: get GdkPixbuf from gdk_pixbuf_new_from_file -> convert GdkPixbuf into ImageData Result: loading natively makes GdkPixbuf implementation faster than Java implementation 3) ImageLoaderListener and interlaced/progressive PNG/JPG/GIF - GdkPixbuf different image type internally, there's no way to distinguish interlaced/progressive images, this causes issue with sending ImageLoaderEvent of partially loaded image 4) Image file format specific PaletteData - different image format has different PaletteData configuration, GdkPixbuf abstracts this away, so only have a generic PaletteData consisting of RGB masks currently - building PaletteData requires different processing on different image formats and must be done in Java level 5) GIF animations - Java implementation: process through GIFFileFormat - GdkPixbuf implementation: use GdkPixbufAnimation to iterate each frame -> get GdkPixbuf from each frame -> will need to investigate more on how to determine if GdkPixbufAnimation reached the end of the loop as GdkPixbuf does not provide such API.
*** Bug 57382 has been marked as a duplicate of this bug. ***
*** Bug 99253 has been marked as a duplicate of this bug. ***
New Gerrit change created: https://git.eclipse.org/r/139147
##### Broken with both implementation 1) Bug 545680 - Snippet141 does not work - Both implementation: nothing is loaded, no GIF is animated 2) Bug 545679 - Snippet194 does not work - Java implementation: Error saving GIF: org.eclipse.swt.SWTException: Unsupported color depth - Native implementation: 0 byte swt.gif saved 3) Animate button in ImageAnalyzer is broken - Both implementation: - Exception in thread "main" java.lang.IllegalArgumentException: Argument not valid at org.eclipse.swt.SWT.error(SWT.java:4680) at org.eclipse.swt.SWT.error(SWT.java:4614) at org.eclipse.swt.SWT.error(SWT.java:4585) at org.eclipse.swt.graphics.GC.drawImage(GC.java:839) at org.eclipse.swt.examples.imageanalyzer.ImageAnalyzer.paintImage(ImageAnalyzer.java:1815) ##### Working with native implementation only 1) Bug 57382 - group 4 tiff image shows blank - Java implementation: loading TIFF images shows blank - Native implementation: all TIFF files can be viewed 2) Bug 99253 - [Graphics] Alpha information lost from .ico files - Java implementation: no alpha information, black pixels when suppose to be transparent - Native implementation: correct transparent pixels 3) GIF files with multiple frames - Java implementation: load an animated GIF image, only first frame is complete, and other frames are corrupted - Native implementation: all frames are complete with GIF images 4) SVG, XPM, PPG, PGM, file formats - Native implementation have default additional support for loading all mentioned formats. ##### Inconsistency between master and native implementation 1) PalleteData indexedPalette for images with depth less than 8 (https://www.eclipse.org/articles/Article-SWT-images/graphics-resources.html#PaletteData) - Java implementation: reads into raw image data, can display indexed palette with 1, 2, 4, 8 bit depth - Native implementation: GdkPixbuf only supports 3/4 n_channels and 8 bits_per_sample. This means all images are of depth 24 / depth 32. This means loading images will result in a direct PaletteData with RGB masks, since there is no way to determine indexed PaletteData info. 2) GIF images disposalMethod is not set in ImageData (i.e. SWT.DM_FILL_*) - Currently the GIF disposalMethod field is not set with the GdkPixbuf implementation as there's no such API - This can be solved by reading directly into raw GIF image data and reading the Graphics Control Extension to get the disposalMethod information, similar to the Java implementation. (See http://giflib.sourceforge.net/whatsinagif/bits_and_bytes.html) - This would require a separete case to consider GIF images
Loading times of native v.s. java implementation #### Native Loading marbles.bmp (1419x1001) takes 48ms Loading s66-54677_lrg.jpg (1000x943) takes 15ms Loading world.topo.bathy.200407.3x5400x2700.png (5400x2700) takes 391ms Loading altimeter2_rcg_0.gif (1064x623) takes 2090ms Loading BlackMarble_2016_Americas_composite.png (4960x4000) takes 442ms Loading world.topo.bathy.200407.3x5400x2700.jpg (5400x2700) takes 122ms #### Java Loading marbles.bmp (1419x1001) takes 22ms Loading s66-54677_lrg.jpg (1000x943) takes 106ms Loading world.topo.bathy.200407.3x5400x2700.png (5400x2700) takes 565ms Loading altimeter2_rcg_0.gif (1064x623) takes 605ms Loading BlackMarble_2016_Americas_composite.png (4960x4000) takes 445ms Loading world.topo.bathy.200407.3x5400x2700.jpg (5400x2700) takes 460ms Loading JPE/PNG images are faster natively, BMP/GIF files are slower, although this could be due to GIF files having corrupted frames with java implementation.
No issues found with the patch so far and results are quite impressive performance wise from running SWT AllTests 4 times: * without patch average: 85.50725 seconds * with patch average: 73.44425 seconds aka ~16% speedup on tests which are not image intensive at all.
Gerrit change https://git.eclipse.org/r/138185 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=719370e9e5d83ce319f5ebade0ff5ecb80cf8497
Gerrit change https://git.eclipse.org/r/139147 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=ab4ab0c267933ceef6272c8edfd2b55c5656b319
New Gerrit change created: https://git.eclipse.org/r/142737
Reverting what was merged in 4.12 as there are too many regressions (bug 547529, see its Gerrit discussion for more info).
New Gerrit change created: https://git.eclipse.org/r/142738
Gerrit change https://git.eclipse.org/r/142738 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=0412725283749ee5fb79984d2aaaa13d1c939b6d
(In reply to Eric Williams from comment #13) > Reverting what was merged in 4.12 as there are too many regressions (bug > 547529, see its Gerrit discussion for more info). Looks like you neither read https://www.eclipse.org/eclipse/development/plans/freeze_plan_4_12.php nor the announcement for RC1 on eclipse-dev. Please get approval for this or revert the revert.
(In reply to Dani Megert from comment #16) > (In reply to Eric Williams from comment #13) > > Reverting what was merged in 4.12 as there are too many regressions (bug > > 547529, see its Gerrit discussion for more info). > Looks like you neither read > https://www.eclipse.org/eclipse/development/plans/freeze_plan_4_12.php nor > the announcement for RC1 on eclipse-dev. > > Please get approval for this or revert the revert. So far I have only reverted the N&N entry, which AFAICT constitutes "documentation", and per the freeze rules does not require PMC approval. When the revert patch is ready (it isn't quite ready yet) I'll ask the PMC for approval on this ticket.
(In reply to Eric Williams from comment #17) > (In reply to Dani Megert from comment #16) > > (In reply to Eric Williams from comment #13) > > > Reverting what was merged in 4.12 as there are too many regressions (bug > > > 547529, see its Gerrit discussion for more info). > > Looks like you neither read > > https://www.eclipse.org/eclipse/development/plans/freeze_plan_4_12.php nor > > the announcement for RC1 on eclipse-dev. > > > > Please get approval for this or revert the revert. > > So far I have only reverted the N&N entry, which AFAICT constitutes > "documentation", and per the freeze rules does not require PMC approval. > When the revert patch is ready (it isn't quite ready yet) I'll ask the PMC > for approval on this ticket. I see. Sorry, obviously I did not have enough coffee :-( You don't have to ask on this list. It is good enough to have a +1 from a project lead or PMC either on the bug or the change itself.
(In reply to Dani Megert from comment #18) > I see. Sorry, obviously I did not have enough coffee :-( That's okay, it happens to all of us. :) > You don't have to ask on this list. It is good enough to have a +1 from a > project lead or PMC either on the bug or the change itself. Np, I'll ask Alex to +1 the Gerrit on Monday.
Gerrit change https://git.eclipse.org/r/142737 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=700a98588c995458d893d9288359de01e3d7e557
(In reply to Eclipse Genie from comment #20) > Gerrit change https://git.eclipse.org/r/142737 was merged to [master]. > Commit: > http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/ > ?id=700a98588c995458d893d9288359de01e3d7e557 Revert has been merged to master.
New Gerrit change created: https://git.eclipse.org/r/143134
Gerrit change https://git.eclipse.org/r/143134 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=0b22592e3f6c547b0bec5a5b491bd41a583e8e60
(In reply to Eclipse Genie from comment #23) > Gerrit change https://git.eclipse.org/r/143134 was merged to [master]. > Commit: > http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/ > ?id=0b22592e3f6c547b0bec5a5b491bd41a583e8e60 Patch is in master now. Will re-merge the N&N entry sometime before M1.
New Gerrit change created: https://git.eclipse.org/r/143880
Gerrit change https://git.eclipse.org/r/143880 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=de32b532e6b52ceda841d2be9ebc7d0ac0eb74c2
(In reply to Eclipse Genie from comment #26) > Gerrit change https://git.eclipse.org/r/143880 was merged to [master]. > Commit: > http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/ > ?id=de32b532e6b52ceda841d2be9ebc7d0ac0eb74c2 N&N is in master now.
*** Bug 544501 has been marked as a duplicate of this bug. ***
New Gerrit change created: https://git.eclipse.org/r/144152
Gerrit change https://git.eclipse.org/r/144152 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=f47f4829e6c43e1e5293a73f2d16211128c8b5b8
This causes regression (same as last time) with javadoc hover icon colors. Look at the image for the interface icons - they have now wrong color. *Exact* same problem as reported on first attempt, see bug 546035 comment 11 and attachment 278138 [details].
I'll take a look.
(In reply to Andrey Loskutov from comment #31) > This causes regression (same as last time) with javadoc hover icon colors. > > Look at the image for the interface icons - they have now wrong color. > *Exact* same problem as reported on first attempt, see bug 546035 comment 11 > and attachment 278138 [details]. Is the color supposed to be the same as in the package explorer (i.e. a "deeper" purple)?
(In reply to Eric Williams from comment #33) > (In reply to Andrey Loskutov from comment #31) > > This causes regression (same as last time) with javadoc hover icon colors. > > > > Look at the image for the interface icons - they have now wrong color. > > *Exact* same problem as reported on first attempt, see bug 546035 comment 11 > > and attachment 278138 [details]. > > Is the color supposed to be the same as in the package explorer (i.e. a > "deeper" purple)? The legend for attachment 278138 [details] : "OK" is expected, "BAD" is wrong :-) I think there was a color shift in original patch due RGB/RGBA mismatch, see patch for the original code: https://git.eclipse.org/r/#/c/139929/.
(In reply to Andrey Loskutov from comment #34) > The legend for attachment 278138 [details] : "OK" is expected, "BAD" is > wrong :-) Oops, missed that one! > I think there was a color shift in original patch due RGB/RGBA mismatch, see > patch for the original code: https://git.eclipse.org/r/#/c/139929/. Yes that's definitely it, I'm investigating now.
New Gerrit change created: https://git.eclipse.org/r/144863
Created attachment 279096 [details] Missing alpha in some images I've found another regression, most likely coming from this loader patch (haven't bisected yet). Please check attached screenshot - some transparent pixels are white now. Note, this is independent of the proposed patch https://git.eclipse.org/r/#/c/144863/ - the patch does NOT fix it.
(In reply to Andrey Loskutov from comment #37) > Created attachment 279096 [details] > Missing alpha in some images > > I've found another regression, most likely coming from this loader patch > (haven't bisected yet). > > Please check attached screenshot - some transparent pixels are white now. > Note, this is independent of the proposed patch > https://git.eclipse.org/r/#/c/144863/ - the patch does NOT fix it. Reproduced it, and yes it's another regression. I'll investigate.
Finally some progress worth reporting. I dug into this interface icon that JDT is creating...turns out it's created using JFace's CompositeImageDescriptor. The ImageData is generated via CompositeImageDescriptor.getImageData(), which creates a 24 bit ImageData using a PaletteData and then sets each pixel one at a time. Out of curiosity I swapped out the ImageData created from the ImageDescriptor and instead called: Image test = descriptor.createImage(); ImageData testImageData = test.getImageData(); Feeding that ImageData into JDT results in a properly colored icon. Upon further investigation it seems that ImageData.setPixel() (used by CompositeImageDescriptor) is using MSB ordering, whereas Image.getImageData() is using LSB ordering. Investigating a proper fix now. As a side note: is it really necessary to be iterating over each pixel and setting it manually, just to get an ImageData? Seems that using Image.getImageData() -- the native implementation -- would be far more efficient. Nikita, this is the type of Platform UI Image related improvement I was talking about.
Gerrit change https://git.eclipse.org/r/144863 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=d6ae8b74de3b5b8e0b7a12eccf27300c755e9672
(In reply to Eclipse Genie from comment #40) > Gerrit change https://git.eclipse.org/r/144863 was merged to [master]. > Commit: > http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/ > ?id=d6ae8b74de3b5b8e0b7a12eccf27300c755e9672 Fixes for the latest regressions are in master. For future regressions, let's open new bugs as this one has been reopened enough times. :)
Verified in I20190710-0610.
(In reply to Eric Williams from comment #42) > Verified in I20190710-0610. Unfortunately, the issues in bug 544501 are still reproducible with 2019-09-M1 (while it worked in 2019-06-M3) - or do I need some intermediate build in order to get the fix? While, a native image loader might help for 544501, it's not really a duplicate of this one. Should I reopen the other or this bug?
(In reply to Ansgar Radermacher from comment #43) > (In reply to Eric Williams from comment #42) > > Verified in I20190710-0610. > > Unfortunately, the issues in bug 544501 are still reproducible with > 2019-09-M1 (while it worked in 2019-06-M3) - or do I need some intermediate > build in order to get the fix? > While, a native image loader might help for 544501, it's not really a > duplicate of this one. Should I reopen the other or this bug? Weird, the difference in the patches isn't that big. I'll reopen the original ticket.
