8353542: No native raster data for common pixel-interleaved BufferedImages

[YaaZ]

---

Progress

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

Error

 ⚠️ The pull request body must not be empty.

Integration blocker

 ⚠️ Title mismatch between PR and JBS for issue JDK-8353542

Issue

• JDK-8353542: Support native raster data for additional custom pixel-interleaved BufferedImage formats (Enhancement - P4) ⚠️ Title mismatch between PR and JBS.

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24378/head:pull/24378
$ git checkout pull/24378

Update a local copy of the PR:
$ git checkout pull/24378
$ git pull https://git.openjdk.org/jdk.git pull/24378/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24378

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24378.diff

[bridgekeeper[bot]]

:wave: Welcome back ngubarkov! 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[bot]]

❗ This change is not yet ready to be integrated. See the Progress checklist in the description for automated requirements.

[openjdk[bot]]

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

[YaaZ]

These changes cause a regression in BMPSubsamplingTest which is worth a separate discussion. The test starts failing with TYPE_3BYTE_GRB custom format:

Test failed due to wrong dst color ff00ff00 at 50,50instead of ffff0000

The root cause of this is that SunGraphics2D.pixel is calculated incorrectly for that format, but Java-side drawing loops doesn't use it. Now that my patch starts initializing native raster data, it starts using C loops (sun.java2d.loops.FillRect#FillRect) which do use this field and mix color components as a result.

This is because SurfaceType for that format is Any3Byte, which doesn't have a specialized PixelConverter, so it falls back to the generic one, which results in, e.g. RED color - SunGraphics2D.eargb=0xffff0000, SunGraphics2D.pixel=0xff, although red component is actually in the middle.

I don't feel like I fully understand the logic of all of this, but it seems to me like this PixelConverter is something not actually belonging in the SurfaceType (and probably not belonging at all being just a hack?), as this is an implementation detail which cannot be controlled by user, assigned to the surface automatically together with SurfaceType. So in the end, user can create an image with arbitrary component order using custom SampleModel, but cannot make Graphics2D work on it correctly because one has no control over the SurfaceType and PixelConverter assigned to it? It looks to me, like PixelConverter provides a subset of functionality done by SampleModel and badly so - with the help of ColorModel in its generic implementation, although ColorModel doesn't necessarily know the underlying order of components, like ComponentColorModel, and therefore cannot really help there.

Maybe I am completely misunderstanding something, but I am kinda stuck there. Would really appreciate your help, @mrserb

[YaaZ]

After looking into this a little bit more, I think I now understand the logic of PixelConverter being tied to the SurfaceType as it is directly related to the native blit loops which expect color to be encoded in some specific way (which although often, but not always directly map to the underlying image layout). So saying that PixelConverter implements the subset of functionality of SampleModel is wrong - more like PixelConverter in some implementations may need help of both ColorModel and SampleModel?

[mrserb]

is wrong - more like PixelConverter in some implementations may need help of both ColorModel and SampleModel?

Check how this API is used in SunGraphics2D. It converts input pixels from a standard format (e.g., XRGB) into the format expected by the target surface. It is just an utility method, and if it used during the rendering(via blits) means that some generic slow blit is in use.

[mrserb]

I profiled Intellij IDEA to see which blit loops are often used there and noticed that icons are loaded as 4-byte RGBA images. See the issue? That's a TYPE_CUSTOM image with some generic SurfaceType - it doesn't have native raster ops initialized, so it can only go through a generic software loop! That's how I came to https://github.com/openjdk/jdk/pull/24378.

I would be good to check from where these custom images are come from, and check the possibility to use GraphicsConfiguration.createCompatibleImage in that codepath or some other standard type of the image. also it will be good to check why it is not cached in VRam.

[mrserb]

So what I was trying to do with that Vulkan work is to make it more generic - register loops for more generic surface types and dynamically inspect their properties to see whether we can actually do an optimal blit. And given that 99% of the time those are 3/4-bytes per pixel RGBA/ARGB/BGR/etc., it's very easy to generalize, but we don't seem to have enough flexibility for that.

This should be possible: you can register Vulkan blits for generic surface types (e.g., ANY_Byte), then inspect their properties (e.g., pixel layout, alignment) at runtime to ensure optimal performance during rendering, if it does not work it might be a bug somewhere.

[YaaZ]

It converts input pixels from a standard format (e.g., XRGB) into the format expected by the target surface.

Yes, I get it, but I mean, the generic PixelConverter implementation doesn't work correctly in some cases and it affects simple "fill rect" operation, not the blit. And similarly to how PixelConverter uses ColorModel in it's generic implementation, it also needs SampleModel for correct calculations in those cases (e.g. for Any4Byte/Any3Byte surfaces).

I would be good to check from where these custom images are come from

Yes, this is actually in my TODO list - I just started digging into J2D first, as it is actually a very simple format and looks like a common scenario, which should be trivial to support.

you can register Vulkan blits for generic surface types (e.g., ANY_Byte), then inspect their properties (e.g., pixel layout, alignment) at runtime to ensure optimal performance during rendering

Yes, that's exactly what I did and it works fine. I think what I meant about "not having enough flexibility" is that there is sometimes not enough info for us to know, how exactly should we work with a given non-leaf SurfaceType. Consider the code, which sets the starting offset when initializing native raster data - we set the starting offset to the offset of the first band, which is hardcoded, because we know that this is, say BGR format, so B comes first. We can easily generalize this and find the first band automatically - but if we wanted to have 4-byte xBGR with first byte of each pixel unused, there is no way for us to know that, because this information is not stored anywhere at all. SampleModel says how we can extract samples from the data buffer, but doesn't (explicitly) tell us how those samples are arranged inside the pixel.

[bridgekeeper[bot]]

@YaaZ This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bridgekeeper[bot]]

@YaaZ This pull request has been inactive for more than 16 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.