Skip to content

Fix stale framebuffer attachments, leaked renderbuffers, and nonzero-default copies #2984

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

Open
toaster0123 wants to merge 2 commits into
base: master
Choose a base branch
from
+984 −31
Open

Fix stale framebuffer attachments, leaked renderbuffers, and nonzero-default copies #2984

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f1646c1
Fix framebuffer attachment updates and resource cleanup
toaster0123 Oct 2, 2026
ca0de89
Remove redundant framebuffer disposal test
toaster0123 Oct 2, 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
122 changes: 92 additions & 30 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 @@ -102,6 +102,17 @@ public final class GLRenderer implements Renderer {

private FrameBuffer mainFbOverride = null;
private int defaultFBO = 0;

/** Native attachment state, without a reference to the owning FrameBuffer. */
private static final class FrameBufferState {
final Set<Integer> attachmentSlots = new HashSet<>();
final Map<RenderBuffer, Integer> renderBuffers = new HashMap<>();
final ArrayList<Texture> colorTextures = new ArrayList<>();
}

// Destructible framebuffer clones contain only an ID. Keep their resources
// here so allocations and target changes after registration are also tracked.
private final Map<Integer, FrameBufferState> frameBufferStates = new HashMap<>();
private final Statistics statistics = new Statistics();
private int vpX, vpY, vpW, vpH;
private int clipX, clipY, clipW, clipH;
Expand Down Expand Up @@ -893,11 +904,10 @@ public void initialize() {
gl.glGetInteger(GL.GL_FRAMEBUFFER_BINDING, tmp);
tmp.rewind();
int fbOnLoad = tmp.get();
if(fbOnLoad > 0)
{
// Override default FB to fbOnLoad. Mostly an iOS fix for scene processors and filters.
defaultFBO = fbOnLoad;
}
// The presentation framebuffer can be nonzero, notably on iOS.
defaultFBO = fbOnLoad;
context.boundFBO = fbOnLoad;
context.boundFB = null;
}

@Override
Expand All @@ -913,6 +923,12 @@ public void invalidateState() {
public void resetGLObjects() {
logger.log(Level.FINE, "Resetting objects and invalidating state");
objManager.resetObjects();
for (FrameBufferState state : frameBufferStates.values()) {
for (RenderBuffer rb : state.renderBuffers.keySet()) {
rb.resetObject();
}
}
frameBufferStates.clear();
statistics.clearMemory();
invalidateState();
}
Expand Down Expand Up @@ -2023,7 +2039,7 @@ public void copyFrameBuffer(FrameBuffer src, FrameBuffer dst, boolean copyColor,
}

if (src == null) {
glfbo.glBindFramebufferEXT(GLFbo.GL_READ_FRAMEBUFFER_EXT, 0);
glfbo.glBindFramebufferEXT(GLFbo.GL_READ_FRAMEBUFFER_EXT, defaultFBO);
srcX0 = vpX;
srcY0 = vpY;
srcX1 = vpX + vpW;
Expand All @@ -2034,7 +2050,7 @@ public void copyFrameBuffer(FrameBuffer src, FrameBuffer dst, boolean copyColor,
srcY1 = src.getHeight();
}
if (dst == null) {
glfbo.glBindFramebufferEXT(GLFbo.GL_DRAW_FRAMEBUFFER_EXT, 0);
glfbo.glBindFramebufferEXT(GLFbo.GL_DRAW_FRAMEBUFFER_EXT, defaultFBO);
dstX0 = vpX;
dstY0 = vpY;
dstX1 = vpX + vpW;
Expand Down Expand Up @@ -2104,6 +2120,8 @@ private void checkFrameBufferError() {

private void updateRenderBuffer(FrameBuffer fb, RenderBuffer rb) {
int id = rb.getId();
FrameBufferState state = frameBufferStates.get(fb.getId());
state.renderBuffers.putIfAbsent(rb, 0);
if (id == -1) {
glfbo.glGenRenderbuffersEXT(intBuf1);
id = intBuf1.get(0);
Expand All @@ -2122,6 +2140,10 @@ private void updateRenderBuffer(FrameBuffer fb, RenderBuffer rb) {
}

GLImageFormat glFmt = texUtil.getImageFormatWithError(rb.getFormat(), fb.isSrgb());
if (state.renderBuffers.get(rb) == glFmt.internalFormat) {
// Reattaching an unchanged buffer must not discard its contents.
return;
}

if (fb.getSamples() > 1 && caps.contains(Caps.FrameBufferMultisample)) {
int samples = fb.getSamples();
Expand All @@ -2140,6 +2162,7 @@ private void updateRenderBuffer(FrameBuffer fb, RenderBuffer rb) {
fb.getWidth(),
fb.getHeight());
}
state.renderBuffers.put(rb, glFmt.internalFormat);
}

private int convertAttachmentSlot(int attachmentSlot) {
Expand Down Expand Up @@ -2201,21 +2224,17 @@ public void updateFrameBufferAttachment(FrameBuffer fb, RenderBuffer rb) {
+ " is not depth-renderable and cannot be used as a depth attachment.");
}

boolean needAttach;
if (rb.getTexture() == null) {
// if it hasn't been created yet, then attach is required.
needAttach = rb.getId() == -1;
updateRenderBuffer(fb, rb);
} else {
needAttach = false;
updateRenderTexture(fb, rb);
}
if (needAttach) {
// A surviving renderbuffer may have moved to a different color slot.
glfbo.glFramebufferRenderbufferEXT(GLFbo.GL_FRAMEBUFFER_EXT,
convertAttachmentSlot(rb.getSlot()),
GLFbo.GL_RENDERBUFFER_EXT,
rb.getId());
} else {
updateRenderTexture(fb, rb);
}
frameBufferStates.get(fb.getId()).attachmentSlots.add(rb.getSlot());
}

private void bindFrameBuffer(FrameBuffer fb) {
Expand Down Expand Up @@ -2265,11 +2284,40 @@ public void updateFrameBuffer(FrameBuffer fb) {
glfbo.glGenFramebuffersEXT(intBuf1);
id = intBuf1.get(0);
fb.setId(id);
frameBufferStates.put(id, new FrameBufferState());
objManager.registerObject(fb);
statistics.onNewFrameBuffer();
}

bindFrameBuffer(fb);
FrameBufferState state = frameBufferStates.get(id);
Set<Integer> slots = new HashSet<>();
Set<RenderBuffer> buffers = new HashSet<>();
if (fb.getDepthTarget() != null) {
slots.add(fb.getDepthTarget().getSlot());
buffers.add(fb.getDepthTarget());
}
for (int i = 0; i < fb.getNumColorTargets(); i++) {
RenderBuffer rb = fb.getColorTarget(i);
slots.add(rb.getSlot());
buffers.add(rb);
}
for (Iterator<Integer> it = state.attachmentSlots.iterator(); it.hasNext();) {
int slot = it.next();
if (!slots.contains(slot)) {
glfbo.glFramebufferRenderbufferEXT(GLFbo.GL_FRAMEBUFFER_EXT,
convertAttachmentSlot(slot), GLFbo.GL_RENDERBUFFER_EXT, 0);
it.remove();
}
}
for (Iterator<RenderBuffer> it = state.renderBuffers.keySet().iterator(); it.hasNext();) {
RenderBuffer rb = it.next();
if (!buffers.contains(rb)) {
deleteRenderBuffer(rb);
it.remove();
}
}
state.colorTextures.clear();

FrameBuffer.RenderBuffer depthBuf = fb.getDepthTarget();
if (depthBuf != null) {
Expand All @@ -2279,6 +2327,9 @@ public void updateFrameBuffer(FrameBuffer fb) {
for (int i = 0; i < fb.getNumColorTargets(); i++) {
FrameBuffer.RenderBuffer colorBuf = fb.getColorTarget(i);
updateFrameBufferAttachment(fb, colorBuf);
if (colorBuf.getTexture() != null) {
state.colorTextures.add(colorBuf.getTexture());
}
}

setReadDrawBuffers(fb);
Expand Down Expand Up @@ -2311,7 +2362,7 @@ public Vector2f[] getFrameBufferSamplePositions(FrameBuffer fb) {
@Override
public void setMainFrameBufferOverride(FrameBuffer fb) {
mainFbOverride = null;
if (context.boundFBO == 0) {
if (context.boundFBO == defaultFBO) {
// Main FB is now set to fb, make sure its bound
setFrameBuffer(fb);
}
Expand Down Expand Up @@ -2402,23 +2453,25 @@ public void setFrameBuffer(FrameBuffer fb) {
if (boundFB != null && (boundFB.getMipMapsGenerationHint() != null
? boundFB.getMipMapsGenerationHint()
: generateMipmapsForFramebuffers)) {
for (int i = 0; i < boundFB.getNumColorTargets(); i++) {
RenderBuffer rb = boundFB.getColorTarget(i);
Texture tex = rb.getTexture();
// Use the native attachment snapshot: the Java targets may already
// have been replaced with textures that have not been allocated yet.
FrameBufferState state = frameBufferStates.get(boundFB.getId());
for (Texture tex : state.colorTextures) {
if (tex != null && tex.getMinFilter().usesMipMapLevels()
&& isMipmapGenerationSupported(tex.getImage().getFormat(),
linearizeSrgbImages && boundFB.isSrgb()
? ColorSpace.sRGB : ColorSpace.Linear)) {
try {
final int textureUnitIndex = 0;
setTexture(textureUnitIndex, rb.getTexture());
setTexture(textureUnitIndex, tex);
} catch (TextureUnitException exception) {
throw new RuntimeException("Renderer lacks texture units?");
}
if (tex.getType() == Texture.Type.CubeMap) {
glfbo.glGenerateMipmapEXT(GL.GL_TEXTURE_CUBE_MAP);
} else {
int textureType = convertTextureType(tex.getType(), tex.getImage().getMultiSamples(), rb.getFace());
int textureType = convertTextureType(
tex.getType(), tex.getImage().getMultiSamples(), -1);
glfbo.glGenerateMipmapEXT(textureType);
}
} else if (tex != null && tex.getMinFilter().usesMipMapLevels()) {
Expand Down Expand Up @@ -2480,28 +2533,37 @@ public void readFrameBufferWithFormat(FrameBuffer fb, ByteBuffer byteBuf, Image.
readFrameBufferWithGLFormat(fb, byteBuf, glFormat.format, glFormat.dataType);
}

private void deleteRenderBuffer(FrameBuffer fb, RenderBuffer rb) {
intBuf1.put(0, rb.getId());
private void deleteRenderBuffer(RenderBuffer rb) {
int id = rb.getId();
if (id == -1 || rb.getTexture() != null) {
return;
}
intBuf1.put(0, id);
glfbo.glDeleteRenderbuffersEXT(intBuf1);
if (context.boundRB == id) {
context.boundRB = 0;
}
rb.resetObject();
}

@Override
public void deleteFrameBuffer(FrameBuffer fb) {
if (fb.getId() != -1) {
if (context.boundFBO == fb.getId()) {
glfbo.glBindFramebufferEXT(GLFbo.GL_FRAMEBUFFER_EXT, 0);
context.boundFBO = 0;
bindFrameBuffer(null);
toggleFramebufferSrgb(null);
}

if (fb.getDepthTarget() != null) {
deleteRenderBuffer(fb, fb.getDepthTarget());
}
if (fb.getColorTarget() != null) {
deleteRenderBuffer(fb, fb.getColorTarget());
FrameBufferState state = frameBufferStates.remove(fb.getId());
if (state != null) {
for (RenderBuffer rb : state.renderBuffers.keySet()) {
deleteRenderBuffer(rb);
}
}

intBuf1.put(0, fb.getId());
glfbo.glDeleteFramebuffersEXT(intBuf1);
objManager.unregisterObject(fb);
fb.resetObject();

statistics.onDeleteFrameBuffer();
Expand Down
19 changes: 18 additions & 1 deletion jme3-core/src/main/java/com/jme3/texture/FrameBuffer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,12 +258,14 @@ private FrameBuffer() {
public void addColorTarget(FrameBufferBufferTarget colorBuf){
colorBuf.slot=colorBufs.size();
colorBufs.add(colorBuf);
setUpdateNeeded();
}

public void addColorTarget(FrameBufferTextureTarget colorBuf){
// checkSetTexture(colorBuf.getTexture(), false); // TODO: this won't work for levels.
colorBuf.slot=colorBufs.size();
colorBufs.add(colorBuf);
setUpdateNeeded();
}

/**
Expand All @@ -281,6 +283,7 @@ public void replaceColorTarget(int i, FrameBufferTextureTarget colorBuf) {
}
colorBuf.slot = i;
colorBufs.set(i, colorBuf);
setUpdateNeeded();
}

/**
Expand All @@ -293,9 +296,15 @@ public void replaceColorTarget(int i, FrameBufferTextureTarget colorBuf) {
*/
public void removeColorTarget(int i) {
colorBufs.remove(i);
if (colorBufIndex > i) {
colorBufIndex--;
} else if (colorBufIndex == i && colorBufIndex == colorBufs.size()) {
colorBufIndex = Math.max(0, colorBufs.size() - 1);
}
for (; i < colorBufs.size(); i++) {
colorBufs.get(i).slot = i;
}
setUpdateNeeded();
}

/**
Expand All @@ -310,19 +319,22 @@ public void addColorTarget(FrameBufferTextureTarget colorBuf, TextureCubeMap.Fac
colorBuf.slot = colorBufs.size();
colorBuf.face = face.ordinal();
colorBufs.add(colorBuf);
setUpdateNeeded();
}

public void setDepthTarget(FrameBufferBufferTarget depthBuf){
if (!depthBuf.getFormat().isDepthFormat())
throw new IllegalArgumentException("Depth buffer format must be depth.");
this.depthBuf = depthBuf;
this.depthBuf.slot = this.depthBuf.getFormat().isDepthStencilFormat() ? SLOT_DEPTH_STENCIL : SLOT_DEPTH;
setUpdateNeeded();
}

public void setDepthTarget(FrameBufferTextureTarget depthBuf){
checkSetTexture(depthBuf.getTexture(), true);
this.depthBuf = depthBuf;
this.depthBuf.slot = depthBuf.getTexture().getImage().getFormat().isDepthStencilFormat() ? SLOT_DEPTH_STENCIL : SLOT_DEPTH;
setUpdateNeeded();
}

public int getNumColorTargets(){
Expand Down Expand Up @@ -474,6 +486,7 @@ public void setMultiTarget(boolean enabled) {
} else {
colorBufIndex = 0;
}
setUpdateNeeded();
}

/**
Expand All @@ -499,7 +512,7 @@ public void setTargetIndex(int index) {
throw new IllegalArgumentException("Target index must be between 0 and 16");
}

if (colorBufs.size() < index) {
if (colorBufs.size() <= index) {
throw new IllegalArgumentException("The target at " + index + " is not set!");
}

Expand Down Expand Up @@ -568,6 +581,10 @@ public void setColorTexture(TextureCubeMap tex, TextureCubeMap.Face face) {
*/
public void clearColorTargets() {
colorBufs.clear();
if (colorBufIndex >= 0) {
colorBufIndex = 0;
}
setUpdateNeeded();
}

/**
Expand Down
19 changes: 19 additions & 0 deletions jme3-core/src/main/java/com/jme3/util/NativeObjectManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,20 @@ public void registerObject(NativeObject obj) {
}
}

/**
* Unregisters an object explicitly deleted by a renderer. This is also safe
* when the manager has already unregistered it before calling the renderer.
*
* @param obj the deleted native object (not null, with its original ID)
*/
public void unregisterObject(NativeObject obj) {
NativeObjectRef ref = refMap.remove(obj.getUniqueId());
if (ref != null) {
ref.clear();
}
userDeletionQueue.removeIf(queued -> queued == obj);
}

private void deleteNativeObject(Object rendererObject, NativeObject obj, NativeObjectRef ref,
boolean deleteGL, boolean deleteBufs) {
assert rendererObject != null;
Expand Down Expand Up @@ -187,6 +201,11 @@ public void deleteUnused(Object rendererObject){
if (ref == null) {
break;
}
// Explicit deletion may have unregistered an already queued reference.
// Its native name can now belong to a different object.
if (refMap.get(ref.objClone.getUniqueId()) != ref) {
continue;
}

deleteNativeObject(rendererObject, ref.objClone, ref, true, false);
removed++;
Expand Down
Loading