Skip to content

Conversation

@ujpv
Copy link
Collaborator

@ujpv ujpv commented Aug 4, 2025

The corresponding JBR API review:
JetBrains/JetBrainsRuntimeApi#20

@ujpv ujpv force-pushed the kharitonov/shared_texture_opengl branch 2 times, most recently from f9181fe to d218421 Compare August 5, 2025 11:39
@ujpv ujpv changed the base branch from main to kharitonov/main August 5, 2025 11:40
@ujpv ujpv requested review from YaaZ and avu August 5, 2025 11:44
@ujpv ujpv force-pushed the kharitonov/shared_texture_opengl branch 3 times, most recently from c29910a to 9be77e8 Compare August 7, 2025 11:29
(JNIEnv *env, jclass clazz, jlong texture) {
if (gTextureType == OPENGL_TEXTURE_TYPE) {
GLuint texId = (GLuint)texture;
glDeleteTextures(1, &texId);\
Copy link
Member

Choose a reason for hiding this comment

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

Backslash?

Comment on lines 173 to 178
final static TexturePixelLayout RGBA = new TexturePixelLayout(0, 1, 2, 3);
final static TexturePixelLayout BGRA = new TexturePixelLayout(2, 1, 0, 3);
static TexturePixelLayout get(int textureType) {
return switch (textureType) {
case SharedTextures.OPENGL_TEXTURE_TYPE -> TexturePixelLayout.RGBA;
case SharedTextures.METAL_TEXTURE_TYPE -> TexturePixelLayout.BGRA;
Copy link
Member

Choose a reason for hiding this comment

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

We seem to expect different pixel layouts for Metal and OGL, however for *TextureWrapperSurfaceData classes we always use ColorModel.getRGBdefault(), which seems inconsistent - could you check that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I made it consistent with how it's done for VolatileImage. The color model is taken now from the GraphicsConfig.
However, look like those color model from TextureWrapperSurfaceData is the color model on the public image API side. It doesn't have to be the same as the in the accelerated surface.

@ujpv ujpv changed the title Shared textures OpenGL [work in process]Shared textures OpenGL Oct 15, 2025
@ujpv ujpv force-pushed the kharitonov/shared_texture_opengl branch 2 times, most recently from de0bfbd to 60ce7f2 Compare October 15, 2025 15:12
@ujpv ujpv changed the title [work in process]Shared textures OpenGL Shared textures OpenGL Oct 15, 2025
@ujpv ujpv changed the base branch from kharitonov/main to kharitonov/shaared_textures_refactor_mac October 15, 2025 15:28
@ujpv ujpv changed the base branch from kharitonov/shaared_textures_refactor_mac to kharitonov/shared_texture_refactor_metal October 15, 2025 15:28
@ujpv ujpv force-pushed the kharitonov/shared_texture_opengl branch from 60ce7f2 to 98362d9 Compare October 15, 2025 15:32
@ujpv ujpv requested a review from YaaZ October 15, 2025 16:52
Copy link
Member

@YaaZ YaaZ left a comment

Choose a reason for hiding this comment

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

LGTM overall. Although I see a lot of instanceofs, I guess you're intentionally trying to keep your code separate from the JDK's, right?

-framework Cocoa -framework SystemConfiguration
BUILD_JDK_JTREG_LIBRARIES_LIBS_libSharedTexturesTest := \
-framework Cocoa -framework Metal
-framework Cocoa -framework Metal -framework OpenGL
Copy link
Member

Choose a reason for hiding this comment

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

I forgot whether I already asked this, but do we really need an OGL implementation on macOS, given that it's deprecated there?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, we talked about it and concluded that it won't heart. :)

We still have this pipeline, the OS still supports it so far.

private final Image image;

public WGLTextureWrapperSurfaceData(WGLGraphicsConfig gc, Image image, long textureId) {
super(null, gc, ColorModel.getRGBdefault(), RT_TEXTURE);
Copy link
Member

Choose a reason for hiding this comment

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

In other places you take color model from the GC, this one looks inconsistent with the others

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops. Thanks.

@ujpv
Copy link
Collaborator Author

ujpv commented Oct 23, 2025

LGTM overall. Although I see a lot of instanceofs, I guess you're intentionally trying to keep your code separate from the JDK's, right?

Yes, sort of.
I wanted to not touch the OpenJDK code as much as I can to avoid not needed conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants