Skip to content

Fix GLES3 packed sRGB texture transfers #2986

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.

Sign up for GitHub

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

Merged
riccardobl merged 3 commits into from
Oct 4, 2026
+1,228 −23
Merged

Fix GLES3 packed sRGB texture transfers #2986

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
48ccd63
Fix GLES3 packed sRGB texture transfers
toaster0123 Oct 2, 2026
b2d7a5b
Reuse packed texture conversion scratch and verify buffer state
toaster0123 Oct 2, 2026
9d541c8
Bound packed texture scratch and convert only cropped pixels
toaster0123 Oct 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions jme3-core/src/main/java/com/jme3/renderer/opengl/GLImageFormats.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,8 +185,13 @@ public static GLImageFormat[][] getFormatsForCaps(EnumSet<Caps> caps) {
// sRGB formats
if (caps.contains(Caps.Srgb)) {
formatSrgb(formatToGL, Format.RGB8, GLExt.GL_SRGB8_EXT, GL.GL_RGB, GL.GL_UNSIGNED_BYTE, opengl, false, true);
formatSrgb(formatToGL, Format.RGB565, GLExt.GL_SRGB8_EXT, GL.GL_RGB, GL.GL_UNSIGNED_SHORT_5_6_5, opengl, false, true);
formatSrgb(formatToGL, Format.RGB5A1, GLExt.GL_SRGB8_ALPHA8_EXT, GL.GL_RGBA, GL.GL_UNSIGNED_SHORT_5_5_5_1, opengl, false, true);
// GLES3 sRGB formats require unsigned bytes. TextureUtil expands packed source pixels.
formatSrgb(formatToGL, Format.RGB565, GLExt.GL_SRGB8_EXT, GL.GL_RGB,
opengles3 ? GL.GL_UNSIGNED_BYTE : GL.GL_UNSIGNED_SHORT_5_6_5,
opengl, false, true);
formatSrgb(formatToGL, Format.RGB5A1, GLExt.GL_SRGB8_ALPHA8_EXT, GL.GL_RGBA,
opengles3 ? GL.GL_UNSIGNED_BYTE : GL.GL_UNSIGNED_SHORT_5_5_5_1,
opengl || opengles3, false, true);

formatSrgb(formatToGL, Format.RGBA8, GLExt.GL_SRGB8_ALPHA8_EXT, GL.GL_RGBA, GL.GL_UNSIGNED_BYTE, true, false, true);
if (!coreProfile) {
Expand Down
5 changes: 3 additions & 2 deletions jme3-core/src/main/java/com/jme3/renderer/opengl/GLRenderer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -920,6 +920,7 @@ public void resetGLObjects() {
@Override
public void cleanup() {
logger.log(Level.FINE, "Deleting objects and invalidating state");
texUtil.cleanup();
objManager.deleteAllObjects(this);
statistics.clearMemory();
invalidateState();
Expand Down Expand Up @@ -3171,7 +3172,7 @@ public void modifyTexture(Texture tex, Image pixels, int x, int y) {
}
int target = convertTextureType(tex.getType(), pixels.getMultiSamples(), -1);
texUtil.uploadSubTexture(target, pixels, 0, x, y,
0, 0, pixels.getWidth(), pixels.getHeight(), linearizeSrgbImages);
0, 0, pixels.getWidth(), pixels.getHeight(), linearizeSrgbImages, tex.getImage().getColorSpace());

@jaime-jmebot jaime-jmebot Oct 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nullability heads-up for the new argument: Image.getColorSpace() can legitimately return null. Image.read() restores it with a null default (colorSpace = capsule.readEnum("colorSpace", ColorSpace.class, null);), and ColorSpace only has sRGB/Linear.

If the packed-transfer check does anything other than destColorSpace == ColorSpace.sRGB, an image loaded from a .j3f that never wrote a color space would throw an NPE mid-upload. Can you make that comparison null-safe (a null/unknown color space then just means "don't convert")?

}

/**
Expand Down Expand Up @@ -3199,7 +3200,7 @@ public void modifyTexture(Texture2D dest, Image src, int destX, int destY,
}
int target = convertTextureType(dest.getType(), src.getMultiSamples(), -1);
texUtil.uploadSubTexture(target, src, 0, destX, destY,
srcX, srcY, areaW, areaH, linearizeSrgbImages);
srcX, srcY, areaW, areaH, linearizeSrgbImages, dest.getImage().getColorSpace());
}

@Override
Expand Down
174 changes: 155 additions & 19 deletions jme3-core/src/main/java/com/jme3/renderer/opengl/TextureUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,9 @@
import com.jme3.texture.Image;
import com.jme3.texture.Image.Format;
import com.jme3.texture.image.ColorSpace;
import com.jme3.util.BufferUtils;
import java.nio.ByteBuffer;
import java.nio.ByteOrder;
import java.util.EnumSet;
import java.util.logging.Level;
import java.util.logging.Logger;
Expand All @@ -48,13 +50,16 @@
*/
public final class TextureUtil {

private static final int PACKED_PIXEL_BUFFER_SIZE = 32 * 1024 * 1024;

private static final Logger logger = Logger.getLogger(TextureUtil.class.getName());

private final GL gl;
private final GL2 gl2;
private final GLExt glext;
private GLImageFormat[][] formats;
private boolean supportUnpackRowLength;
private ByteBuffer packedPixelBuffer;

public TextureUtil(GL gl, GL2 gl2, GLExt glext) {
this.gl = gl;
Expand Down Expand Up @@ -157,6 +162,75 @@ private void setupTextureSwizzle(int target, Format format) {
}
}

private static boolean requiresPackedExpansion(Format format, GLImageFormat glFormat) {
return glFormat.dataType == GL.GL_UNSIGNED_BYTE
&& (format == Format.RGB565 || format == Format.RGB5A1);
}

private ByteBuffer expandPackedPixels(ByteBuffer data, Format format, int pixelCount) {
return expandPackedPixels(data, format, pixelCount, 1, pixelCount);
}

private ByteBuffer expandPackedPixels(ByteBuffer data, Format format,
int width, int height, int rowLength) {
// GL reads packed shorts in native byte order, regardless of the ByteBuffer's byte-order metadata.
ByteBuffer source = data.duplicate().order(ByteOrder.nativeOrder());
boolean hasAlpha = format == Format.RGB5A1;
int components = hasAlpha ? 4 : 3;
long sourcePixels = width == 0 || height == 0 ? 0 : (long) (height - 1) * rowLength + width;
if (width < 0 || height < 0 || rowLength < width || sourcePixels > source.remaining() / 2) {
throw new IllegalArgumentException("Packed image data is too small for the requested dimensions");
}
int requiredBytes = Math.multiplyExact(Math.multiplyExact(width, height), components);
if (packedPixelBuffer == null) {
packedPixelBuffer = BufferUtils.createByteBuffer(PACKED_PIXEL_BUFFER_SIZE);
}
// Keep a fixed scratch allocation; unusually large uploads must not enlarge it permanently.
ByteBuffer expanded = requiredBytes <= PACKED_PIXEL_BUFFER_SIZE
? packedPixelBuffer : BufferUtils.createByteBuffer(requiredBytes);
try {
// Client-memory pixels have been consumed when the preceding GL upload returns.
expanded.clear();
int start = source.position();
for (int row = 0; row < height && width != 0; row++) {
source.position(start + Math.multiplyExact(Math.multiplyExact(row, rowLength), 2));
for (int column = 0; column < width; column++) {
int pixel = source.getShort() & 0xffff;
int red = pixel >> 11;
int green = hasAlpha ? (pixel >> 6) & 0x1f : (pixel >> 5) & 0x3f;
int blue = hasAlpha ? (pixel >> 1) & 0x1f : pixel & 0x1f;
// Preserve encoded sRGB values; the texture performs linearization when sampled.
expanded.put((byte) ((red * 255 + 15) / 31));
expanded.put((byte) ((green * 255 + (hasAlpha ? 15 : 31)) / (hasAlpha ? 31 : 63)));
expanded.put((byte) ((blue * 255 + 15) / 31));
if (hasAlpha) {
expanded.put((byte) ((pixel & 1) * 255));
}
}
}
expanded.flip();
return expanded;
} catch (RuntimeException | Error exception) {
releasePackedPixels(expanded);
throw exception;
}
}

private void releasePackedPixels(ByteBuffer buffer) {
if (buffer != null && buffer != packedPixelBuffer) {
BufferUtils.destroyDirectBuffer(buffer);
}
}

/** Releases the owned CPU conversion scratch buffer; repeated calls are safe. */
void cleanup() {
ByteBuffer buffer = packedPixelBuffer;
packedPixelBuffer = null;
if (buffer != null) {
BufferUtils.destroyDirectBuffer(buffer);
}
}

private void uploadTextureLevel(GLImageFormat format, int target, int level, int slice, int sliceCount, int width, int height, int depth, int samples, ByteBuffer data) {
if (format.compressed && data != null) {
if (target == GL2.GL_TEXTURE_3D) {
Expand Down Expand Up @@ -267,13 +341,17 @@ public void uploadTexture(Image image,

boolean getSrgbFormat = image.getColorSpace() == ColorSpace.sRGB && linearizeSrgb;
Image.Format jmeFormat = image.getFormat();
GLImageFormat oglFormat = getImageFormatWithError(jmeFormat, getSrgbFormat);
final GLImageFormat oglFormat = getImageFormatWithError(jmeFormat, getSrgbFormat);

ByteBuffer data = null;
int sliceCount = 1;

if (index >= 0) {
data = image.getData(index);
ByteBuffer source = image.getData(index);
if (source != null) {
data = source.duplicate();
data.clear();
}
}

if (image.getData() != null && image.getData().size() > 0) {
Expand Down Expand Up @@ -312,7 +390,20 @@ public void uploadTexture(Image image,
data.limit(pos + mipSizes[i]);
}

uploadTextureLevel(oglFormat, target, i, index, sliceCount, mipWidth, mipHeight, mipDepth, samples, data);
ByteBuffer expanded = null;
try {
if (data != null && requiresPackedExpansion(jmeFormat, oglFormat)) {
int pixelCount = Math.multiplyExact(mipWidth, mipHeight);
if (target == GL2.GL_TEXTURE_3D) {
pixelCount = Math.multiplyExact(pixelCount, mipDepth);
}
expanded = expandPackedPixels(data, jmeFormat, pixelCount);
}
uploadTextureLevel(oglFormat, target, i, index, sliceCount,
mipWidth, mipHeight, mipDepth, samples, expanded != null ? expanded : data);
} finally {
releasePackedPixels(expanded);
}

pos += mipSizes[i];
}
Expand Down Expand Up @@ -344,12 +435,16 @@ public void uploadSubTexture(Image image, int target, int index, int x, int y, b
}

boolean getSrgbFormat = image.getColorSpace() == ColorSpace.sRGB && linearizeSrgb;
GLImageFormat oglFormat = getImageFormatWithError(jmeFormat, getSrgbFormat);
final GLImageFormat oglFormat = getImageFormatWithError(jmeFormat, getSrgbFormat);

ByteBuffer data = null;

if (index >= 0) {
data = image.getData(index);
ByteBuffer source = image.getData(index);
if (source != null) {
data = source.duplicate();
data.clear();
}
}

if (data == null) {
Expand All @@ -358,12 +453,27 @@ public void uploadSubTexture(Image image, int target, int index, int x, int y, b

data.position(0);
data.limit(data.capacity());

gl.glTexSubImage2D(target, 0, x, y, image.getWidth(), image.getHeight(),
oglFormat.format, oglFormat.dataType, data);
ByteBuffer expanded = null;
try {
if (requiresPackedExpansion(jmeFormat, oglFormat)) {
expanded = expandPackedPixels(data, jmeFormat,
Math.multiplyExact(image.getWidth(), image.getHeight()));
}
gl.glTexSubImage2D(target, 0, x, y, image.getWidth(), image.getHeight(),
oglFormat.format, oglFormat.dataType, expanded != null ? expanded : data);
} finally {
releasePackedPixels(expanded);
}
}

public void uploadSubTexture(int target, Image src, int index, int targetX, int targetY, int areaX, int areaY, int areaWidth, int areaHeight, boolean linearizeSrgb) {
uploadSubTexture(target, src, index, targetX, targetY, areaX, areaY, areaWidth, areaHeight,
linearizeSrgb, src.getColorSpace());
}

void uploadSubTexture(int target, Image src, int index, int targetX, int targetY,
int areaX, int areaY, int areaWidth, int areaHeight,
boolean linearizeSrgb, ColorSpace destinationColorSpace) {
if (target != GL.GL_TEXTURE_2D || src.getDepth() > 1) {
throw new UnsupportedOperationException("Updating non-2D texture is not supported");
}
Expand All @@ -384,40 +494,66 @@ public void uploadSubTexture(int target, Image src, int index, int targetX, int
throw new UnsupportedOperationException("Updating depth images is not supported");
}

boolean getSrgbFormat = src.getColorSpace() == ColorSpace.sRGB && linearizeSrgb;
GLImageFormat oglFormat = getImageFormatWithError(jmeFormat, getSrgbFormat);
// The destination storage determines which pixel types GLES3 accepts, even when the
// source has a different color space. This is a raw copy, without CPU gamma conversion.
boolean getSrgbFormat = destinationColorSpace == ColorSpace.sRGB && linearizeSrgb;
final GLImageFormat oglFormat = getImageFormatWithError(jmeFormat, getSrgbFormat);

ByteBuffer data = src.getData(index);

if (data == null) {
throw new IndexOutOfBoundsException("The image index " + index + " is not valid for the given image");
}

int Bpp = src.getFormat().getBitsPerPixel() / 8;
data = data.duplicate();
int bytesPerPixel = src.getFormat().getBitsPerPixel() / 8;
if (requiresPackedExpansion(jmeFormat, oglFormat)) {
if (areaX < 0 || areaY < 0 || areaWidth < 0 || areaHeight < 0
|| areaX > src.getWidth() - areaWidth || areaY > src.getHeight() - areaHeight) {
throw new IllegalArgumentException("The requested region is outside the source image");
}
data.clear();
if (areaWidth != 0 && areaHeight != 0) {
int firstPixel = Math.addExact(Math.multiplyExact(areaY, src.getWidth()), areaX);
data.position(Math.multiplyExact(firstPixel, bytesPerPixel));
}
ByteBuffer expanded = null;
try {
expanded = expandPackedPixels(data, jmeFormat, areaWidth, areaHeight, src.getWidth());
gl.glTexSubImage2D(target, 0, targetX, targetY, areaWidth, areaHeight,
oglFormat.format, oglFormat.dataType, expanded);
} finally {
releasePackedPixels(expanded);
}
return;
}

int srcWidth = src.getWidth();
int cpos = data.position();
int skip = areaX;
skip += areaY * srcWidth;
skip *= Bpp;
skip *= bytesPerPixel;

data.position(skip);

boolean needsStride = srcWidth != areaWidth;

if (needsStride && (!supportUnpackRowLength)) { // doesn't support stride, copy row by row (slower).
for (int i = 0; i < areaHeight; i++) {
data.position(skip + (srcWidth * Bpp * i));
gl.glTexSubImage2D(target, 0, targetX, targetY + i, areaWidth, 1, oglFormat.format, oglFormat.dataType, data);
data.position(skip + (srcWidth * bytesPerPixel * i));
gl.glTexSubImage2D(target, 0, targetX, targetY + i, areaWidth, 1,
oglFormat.format, oglFormat.dataType, data);
}
} else {
if (needsStride)
gl.glPixelStorei(GL.GL_UNPACK_ROW_LENGTH, srcWidth);
gl.glTexSubImage2D(target, 0, targetX, targetY, areaWidth, areaHeight, oglFormat.format, oglFormat.dataType, data);
if (needsStride)
gl.glPixelStorei(GL.GL_UNPACK_ROW_LENGTH, 0);
try {
gl.glTexSubImage2D(target, 0, targetX, targetY, areaWidth, areaHeight,
oglFormat.format, oglFormat.dataType, data);
} finally {
if (needsStride)
gl.glPixelStorei(GL.GL_UNPACK_ROW_LENGTH, 0);
}
}
data.position(cpos);

@jaime-jmebot jaime-jmebot Oct 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One thing I want to double-check here: this method used to end with data.position(cpos);, so the caller's Image buffer was handed back at the position it came in with. With that line gone, even the plain non-sRGB paths (desktop / linear destination, where nothing is converted) now leave the source buffer already consumed — GLRenderer.modifyTexture passes in a buffer the caller still holds.

Does the new packed-sRGB helper restore the position, or should we keep the restore at the end of this method so the buffer state contract stays as it was for every caller?

}

Expand Down
Loading
Loading