-
Notifications
You must be signed in to change notification settings - Fork 205
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.
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
fix(webgl) texture to texture copy; framebuffer state leak #2042
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -149,6 +149,7 @@ function _copyTextureToBuffer(device: WebGLDevice, options: CopyTextureToBufferO | |
|
||
// Asynchronous read (PIXEL_PACK_BUFFER) is WebGL2 only feature | ||
const {framebuffer, destroyFramebuffer} = getFramebuffer(source); | ||
let prevHandle: WebGLFramebuffer | null | undefined; | ||
try { | ||
const webglBuffer = destination as WEBGLBuffer; | ||
const sourceWidth = width || framebuffer.width; | ||
|
@@ -168,7 +169,8 @@ function _copyTextureToBuffer(device: WebGLDevice, options: CopyTextureToBufferO | |
// } | ||
|
||
device.gl.bindBuffer(GL.PIXEL_PACK_BUFFER, webglBuffer.handle); | ||
device.gl.bindFramebuffer(GL.FRAMEBUFFER, framebuffer.handle); | ||
// @ts-expect-error native bindFramebuffer is overridden by our state tracker | ||
prevHandle = device.gl.bindFramebuffer(GL.FRAMEBUFFER, framebuffer.handle); | ||
|
||
device.gl.readPixels( | ||
origin[0], | ||
|
@@ -181,7 +183,10 @@ function _copyTextureToBuffer(device: WebGLDevice, options: CopyTextureToBufferO | |
); | ||
} finally { | ||
device.gl.bindBuffer(GL.PIXEL_PACK_BUFFER, null); | ||
device.gl.bindFramebuffer(GL.FRAMEBUFFER, null); | ||
// prevHandle may be unassigned if the try block failed before binding | ||
if (prevHandle !== undefined) { | ||
device.gl.bindFramebuffer(GL.FRAMEBUFFER, prevHandle); | ||
} | ||
|
||
if (destroyFramebuffer) { | ||
framebuffer.destroy(); | ||
|
@@ -216,13 +221,16 @@ function _copyTextureToTexture(device: WebGLDevice, options: CopyTextureToTextur | |
const { | ||
/** Texture to copy to/from. */ | ||
source, | ||
/** Mip-map level of the texture to copy to/from. (Default 0) */ | ||
// mipLevel = 0, | ||
/** Mip-map level of the texture to copy to (Default 0) */ | ||
destinationMipLevel = 0, | ||
/** Defines which aspects of the texture to copy to/from. */ | ||
// aspect = 'all', | ||
/** Defines the origin of the copy - the minimum corner of the texture sub-region to copy to/from. */ | ||
/** Defines the origin of the copy - the minimum corner of the texture sub-region to copy from. */ | ||
origin = [0, 0], | ||
|
||
/** Defines the origin of the copy - the minimum corner of the texture sub-region to copy to. */ | ||
destinationOrigin = [0, 0], | ||
|
||
/** Texture to copy to/from. */ | ||
destination | ||
/** Mip-map level of the texture to copy to/from. (Default 0) */ | ||
|
@@ -235,93 +243,72 @@ function _copyTextureToTexture(device: WebGLDevice, options: CopyTextureToTextur | |
|
||
let { | ||
width = options.destination.width, | ||
height = options.destination.width | ||
height = options.destination.height | ||
// depthOrArrayLayers = 0 | ||
} = options; | ||
|
||
const destinationMipmaplevel = 0; | ||
const destinationInternalFormat = GL.RGBA; | ||
|
||
const {framebuffer, destroyFramebuffer} = getFramebuffer(source); | ||
const [sourceX, sourceY] = origin; | ||
const [destinationX, destinationY, destinationZ] = destinationOrigin; | ||
|
||
const isSubCopy = false; | ||
// typeof destinationX !== 'undefined' || | ||
// typeof destinationY !== 'undefined' || | ||
// typeof destinationZ !== 'undefined'; | ||
|
||
// destinationX = destinationX || 0; | ||
// destinationY = destinationY || 0; | ||
// destinationZ = destinationZ || 0; | ||
device.gl.bindFramebuffer(GL.FRAMEBUFFER, framebuffer.handle); | ||
// @ts-expect-error native bindFramebuffer is overridden by our state tracker | ||
const prevHandle: WebGLFramebuffer | null = device.gl.bindFramebuffer( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No undefined here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not in a try/catch. |
||
GL.FRAMEBUFFER, | ||
framebuffer.handle | ||
); | ||
// TODO - support gl.readBuffer (WebGL2 only) | ||
// const prevBuffer = gl.readBuffer(attachment); | ||
|
||
let texture = null; | ||
let texture: WEBGLTexture = null; | ||
let textureTarget: GL; | ||
if (destination instanceof WEBGLTexture) { | ||
texture = destination; | ||
width = Number.isFinite(width) ? width : texture.width; | ||
height = Number.isFinite(height) ? height : texture.height; | ||
texture.bind(0); | ||
textureTarget = texture.destination; | ||
textureTarget = texture.glTarget; | ||
} else { | ||
throw new Error('whoops'); | ||
// textureTarget = destination; | ||
throw new Error('invalid destination'); | ||
} | ||
|
||
if (!isSubCopy) { | ||
device.gl.copyTexImage2D( | ||
textureTarget, | ||
destinationMipmaplevel, | ||
destinationInternalFormat, | ||
sourceX, | ||
sourceY, | ||
width, | ||
height, | ||
0 /* border must be 0 */ | ||
); | ||
} else { | ||
// switch (textureTarget) { | ||
// case GL.TEXTURE_2D: | ||
// case GL.TEXTURE_CUBE_MAP: | ||
// device.gl.copyTexSubImage2D( | ||
// textureTarget, | ||
// destinationMipmaplevel, | ||
// destinationX, | ||
// destinationY, | ||
// sourceX, | ||
// sourceY, | ||
// width, | ||
// height | ||
// ); | ||
// break; | ||
// case GL.TEXTURE_2D_ARRAY: | ||
// case GL.TEXTURE_3D: | ||
// device.gl.copyTexSubImage3D( | ||
// textureTarget, | ||
// destinationMipmaplevel, | ||
// destinationX, | ||
// destinationY, | ||
// destinationZ, | ||
// sourceX, | ||
// sourceY, | ||
// width, | ||
// height | ||
// ); | ||
// break; | ||
// default: | ||
// } | ||
switch (textureTarget) { | ||
case GL.TEXTURE_2D: | ||
case GL.TEXTURE_CUBE_MAP: | ||
device.gl.copyTexSubImage2D( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are new helper functions for 9.1 in webgl-texture-helpers that take care of calling the right webgl function and handle more cases, but if this is meant for cherry-pick to 9.0 the maybe aligning with those can be a separate step. |
||
textureTarget, | ||
destinationMipLevel, | ||
destinationX, | ||
destinationY, | ||
sourceX, | ||
sourceY, | ||
width, | ||
height | ||
); | ||
break; | ||
case GL.TEXTURE_2D_ARRAY: | ||
case GL.TEXTURE_3D: | ||
device.gl.copyTexSubImage3D( | ||
textureTarget, | ||
destinationMipLevel, | ||
destinationX, | ||
destinationY, | ||
destinationZ, | ||
sourceX, | ||
sourceY, | ||
width, | ||
height | ||
); | ||
break; | ||
default: | ||
} | ||
|
||
if (texture) { | ||
texture.unbind(); | ||
} | ||
// ts-expect-error | ||
// device.gl.bindFramebuffer(GL.FRAMEBUFFER, prevHandle || null); | ||
device.gl.bindFramebuffer(GL.FRAMEBUFFER, prevHandle); | ||
if (destroyFramebuffer) { | ||
framebuffer.destroy(); | ||
} | ||
return texture; | ||
} | ||
|
||
// Returns number of components in a specific readPixels WebGL format | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm - When do we get undefined? When not instrumented?
Can we add a comment explaining why we do not reset in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assignment of
prevHandle
is in a try/catch so the code may not get executed.