fix(webgl) texture to texture copy; framebuffer state leak (#2042)

visgl · Mar 17, 2024 · e393572 · e393572
1 parent 4cf6f30
commit e393572
Show file tree

Hide file tree

Showing 3 changed files with 61 additions and 70 deletions.
diff --git a/modules/core/src/adapter/resources/command-encoder.ts b/modules/core/src/adapter/resources/command-encoder.ts
@@ -89,6 +89,7 @@ export type CopyTextureToTextureOptions = {
   /**  Mip-map level of the texture to copy to/from. (Default 0) */
   mipLevel?: number;
   /** Defines the origin of the copy - the minimum corner of the texture sub-region to copy from. */
+  origin?: number[];
   /** Defines which aspects of the {@link GPUImageCopyTexture#texture} to copy to/from. */
   aspect?: 'all' | 'stencil-only' | 'depth-only';
 
@@ -101,7 +102,6 @@ export type CopyTextureToTextureOptions = {
   /** Defines which aspects of the {@link GPUImageCopyTexture#texture} to copy to/from. */
   destinationAspect?: 'all' | 'stencil-only' | 'depth-only';
 
-  origin?: number[];
   /** Width to copy */
   width?: number;
   height?: number;

diff --git a/modules/webgl/src/adapter/resources/webgl-command-buffer.ts b/modules/webgl/src/adapter/resources/webgl-command-buffer.ts
@@ -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(
+    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(
+        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

diff --git a/modules/webgl/src/adapter/resources/webgl-framebuffer.ts b/modules/webgl/src/adapter/resources/webgl-framebuffer.ts
@@ -40,7 +40,11 @@ export class WEBGLFramebuffer extends Framebuffer {
       this.autoCreateAttachmentTextures();
 
       /** Attach from a map of attachments */
-      this.gl.bindFramebuffer(GL.FRAMEBUFFER, this.handle);
+      // @ts-expect-error native bindFramebuffer is overridden by our state tracker
+      const prevHandle: WebGLFramebuffer | null = this.gl.bindFramebuffer(
+        GL.FRAMEBUFFER,
+        this.handle
+      );
 
       // Walk the attachments
       for (let i = 0; i < this.colorAttachments.length; ++i) {
@@ -67,7 +71,7 @@ export class WEBGLFramebuffer extends Framebuffer {
         }
       }
 
-      this.gl.bindFramebuffer(GL.FRAMEBUFFER, null);
+      this.gl.bindFramebuffer(GL.FRAMEBUFFER, prevHandle);
     }
   }