[webgl] Remove BufferWithAccessor #1864

donmccurdy · 2023-12-05T20:24:20Z

Removes WebGL-specific BufferWithAccessor subclass. I'm not sure whether the goal was to migrate its functionality to WEBGLBuffer, or to remove cases that depend on it (e.g. not resizing buffers). For now I've moved the extra methods to WEBGLBuffer, and added @deprecated tags to each, but I'm happy to change that. Build and tests pass, but I haven't tested beyond that yet. Use of the additional methods has been replaced where possible, but a couple methods (getData, subData) remain.

Related Tracker: 9.0 release #1501

Status

WEBGLBuffer#reallocate() - Deleted, buffer size is immutable.
WEBGLBuffer#initialize() - Deleted, buffer size is immutable.
WEBGLBuffer#bind() - Deleted, use CommandEncoder or other APIs.
WEBGLBuffer#unbind() - Deleted, use CommandEncoder or other APIs.
WEBGLBuffer#copyData() - Deleted, use CommandEncoder APIs.
WEBGLBuffer#getData() - Deleted, use .readAsync().
- May also want a utility for sync reads in WebGL, for easier transition of DeckGL layers?
WEBGLBuffer#setData() - Deleted, buffer size is immutable.
WEBGLBuffer#subData() - Deleted, use .write().

One notable thing here is that WEBGLBuffer is untyped, and methods like getData() (if we keep it) do not know the type of the underlying data. You can either pass the correct TypedArray class to getData(), or wrap the result in a new ArrayBufferView after receiving it.

ibgreen · 2023-12-05T20:30:51Z

Actually, checking the PR I revise my comment. The goal was to remove these extra methods from Buffer as they are not part of a minimal GPU Buffer interface. I spent a lot of time splitting them out in a separate buffer subclass in preparation for later deleting them. Moving them back into Buffer undoes that and makes portability with WebGPU tricky.

donmccurdy · 2023-12-05T20:39:29Z

Would be happy to do this however you prefer. Shall we just delete the methods for now (or see what that looks like in a PR, at least) and deal with changes in Transform and Deck.gl as we come to those? Or move the methods into utility functions somewhere?

ibgreen · 2023-12-05T20:53:29Z

Yes. But I think before breaking deck better get @Pessimistress opinion. It may just be a question of timing, when to land this so it doesn't disrupt some other refactor in deck.

donmccurdy · 2023-12-05T23:13:31Z

I've updated the top post with status for each method. I've left getData() and subData() on WEBGLBuffer for now, I'm not sure what to do with those two yet.

Next steps on my side will be to add more unit test coverage, fixing the CI failure.

donmccurdy · 2023-12-06T21:43:42Z

Looking through DeckGL, I found things depending on these methods:

reallocate
subData
getData

I think we'll probably want to remove use of reallocate entirely. No preference on whether subData or getData are kept on the WEBGLBuffer class, or are moved off into utility functions.

ibgreen · 2023-12-06T22:47:59Z

I would focus on polishing the new Buffer API which is WebGPU aligned to do what we need, so that we don't need the old methods.

write() and readAsync()

https://github.com/visgl/luma.gl/blob/master/modules/core/src/adapter/resources/buffer.ts#L79

For reallocate, deck.gl could perhaps create its own helper, it might be doable using existing methods?

donmccurdy · 2023-12-07T01:11:13Z

One other thought on the API – perhaps methods write, writeAsync, readAsync, and getMappedRange should have params and return types of Uint8Array instead of ArrayBuffer? Using a view would avoid some extra copying for applications working with subsets of memory, since you don't need to slice that memory out into an isolated ArrayBuffer. Otherwise we need to pass a lot of byteOffset / byteLength parameters around to avoid copies.

modules/webgl/src/adapter/resources/webgl-buffer.ts

donmccurdy · 2023-12-07T15:38:06Z

modules/webgl/src/classic/copy-and-blit.ts

+  // TODO(donmccurdy): Do we have tests to confirm this is working?
+  const commandEncoder = source.device.createCommandEncoder();
+  commandEncoder.copyTextureToBuffer({
+    source: source as Texture,
+    width: sourceWidth,
+    height: sourceHeight,
+    origin: [sourceX, sourceY],
+    destination: target,
+    byteOffset: targetByteOffset
  });


I'll try to get some tests for this added ASAP.

If we have commandEncoder.copyTextureToBuffer do we still need readPixelsToBuffer?

I suspect that deck only uses readPixelsToArray, so maybe just port that to use commandEncoder and drop the other wrapper?

Eventually commandEncoder.copyTextureToBuffer might be all we need, yes! No strong preference there. I'm working on unit tests for it, and I think there are some important cases that aren't working correctly yet, but once that's more stable perhaps we switch over.

donmccurdy · 2023-12-07T15:42:43Z

modules/webgpu/src/adapter/resources/webgpu-buffer.ts

@@ -54,7 +54,7 @@ export class WebGPUBuffer extends Buffer {
    );
  }

-  override async readAsync(byteOffset: number = 0, byteLength: number = this.byteLength): Promise<ArrayBuffer> {
+  override async readAsync(byteOffset: number = 0, byteLength: number = this.byteLength): Promise<Uint8Array> {


Changed to match WEBGLBuffer implementation (and would be my slight preference anyway).

We could also consider making readAsync take a target Uint8Array (optional? required?) and writing data into that, so that calling this in the frame loop doesn't necessarily allocate a new buffer each time.

modules/webgl/src/adapter/webgl-device.ts

ibgreen · 2023-12-07T17:14:38Z

modules/core/src/adapter/resources/buffer.ts

@@ -77,7 +77,7 @@ export abstract class Buffer extends Resource<BufferProps> {
  }

  write(data: ArrayBufferView, byteOffset?: number): void { throw new Error('not implemented'); }
-  readAsync(byteOffset?: number, byteLength?: number): Promise<ArrayBuffer>  { throw new Error('not implemented'); }
+  readAsync(byteOffset?: number, byteLength?: number): Promise<Uint8Array>  { throw new Error('not implemented'); }


If you haven't done so, probably worth your time to read up on the WebGPU spec to understand how things work there https://www.w3.org/TR/webgpu/#buffer-mapping

Yep, have done a little work with this area of WebGPU – I don't know of anything in WebGL or WebGPU that strongly affects our choice of ArrayBuffer/Uint8Array, just trying to preserve our options for avoiding copies down the road.

modules/webgl/src/adapter/resources/webgl-buffer.ts

ibgreen · 2023-12-07T17:19:15Z

modules/webgl/src/adapter/resources/webgl-buffer.ts

@@ -183,7 +185,9 @@ export class WEBGLBuffer extends Buffer {
 // static INDIRECT = 0x0100;
 // static QUERY_RESOLVE = 0x0200;

-function getWebGLTarget(usage: number): GL.ARRAY_BUFFER | GL.ELEMENT_ARRAY_BUFFER | GL.UNIFORM_BUFFER {
+function getWebGLTarget(


Nit: Wonder why your editor/prettier cut this line differently if nothing was changed...

Since Prettier has disabled repo-wide, I think there are a lot of formatting changes stacked up from the last few months of changes.

modules/webgl/src/adapter/resources/webgl-vertex-array.ts

ibgreen · 2023-12-07T18:33:44Z

modules/webgl/src/adapter/resources/webgl-vertex-array.ts

@@ -230,10 +229,14 @@ export class WEBGLVertexArray extends VertexArray {
    const byteLength = constantValue.byteLength * elementCount;
    const length = constantValue.length * elementCount;

+    if (this.buffer && byteLength !== this.buffer.byteLength) {


I think we should just destroy the old buffer and create a new one, instead of reallocate.

Agreed, better to destroy/create than reallocate. Wasn't sure if that was the responsibility of webgl-vertex-array or the application, in this context... may want to decide if/when we find a test that hits this error.

It used to be an issue where desktop GPU drivers didn't let you use a constant for attribute 0. Not sure if that is still a problem.

But in WebGPU, there is no support whatsoever for constant (disabled) attributes, so we'll need to manufacture a buffer for every constant.

deck wants to continue to leverage constant attributes on WebGL for the memory savings...

(To allow constants in WebGPU we'd probably need fancy shader transformation code that replaces attributes with uniforms, but then those uniforms need to be in dynamically generated and populated uniform buffers so it is fairly involved)

ibgreen · 2023-12-07T18:36:18Z

modules/webgl/src/classic/copy-and-blit.ts

+  // TODO(donmccurdy): Do we have tests to confirm this is working?
+  const commandEncoder = source.device.createCommandEncoder();
+  commandEncoder.copyTextureToBuffer({
+    source: source as Texture,
+    width: sourceWidth,
+    height: sourceHeight,
+    origin: [sourceX, sourceY],
+    destination: target,
+    byteOffset: targetByteOffset
  });


If we have commandEncoder.copyTextureToBuffer do we still need readPixelsToBuffer?

I suspect that deck only uses readPixelsToArray, so maybe just port that to use commandEncoder and drop the other wrapper?

donmccurdy · 2023-12-07T20:58:27Z

Corresponding changes for DeckGL:

Remove use of deprecated BufferWithAccessor deck.gl#8345

Co-authored-by: Ib Green <[email protected]>

donmccurdy mentioned this pull request Dec 5, 2023

Tracker: 9.0 release #1501

Closed

31 tasks

This comment was marked as off-topic.

Sign in to view

donmccurdy commented Dec 7, 2023

View reviewed changes

donmccurdy marked this pull request as ready for review December 7, 2023 15:43

donmccurdy requested review from felixpalmer and ibgreen December 7, 2023 15:43

ibgreen approved these changes Dec 7, 2023

View reviewed changes

donmccurdy mentioned this pull request Dec 7, 2023

Remove use of deprecated BufferWithAccessor visgl/deck.gl#8345

Merged

donmccurdy mentioned this pull request Dec 7, 2023

[core] Restore tests for CommandBuffer#copyTextureToBuffer #1868

Merged

donmccurdy and others added 10 commits December 8, 2023 15:01

[webgl] Remove BufferWithAccessor

a474756

WEBGLBuffer: Remove reallocate(), initialize(), and setData()

6ce3b0a

WEBGLBuffer: Remove bind() and unbind()

47e3d4d

WEBGLBuffer: Remove copyData()

8f04fa0

WEBGLBuffer: Add getData()

da39fc2

WEBGLBuffer: Remove subData(), use write()

6950e76

WEBGLBuffer: Remove getData(), use readAsync()

af840dd

Clean up

10614de

Restore tests for CommandBuffer#copyBufferToBuffer

7db05d9

Apply suggestions from code review

52f690a

Co-authored-by: Ib Green <[email protected]>

donmccurdy force-pushed the donmccurdy/remove-BufferWithAccessor branch from dbb7c56 to 52f690a Compare December 8, 2023 20:01

donmccurdy merged commit cd9bbad into master Dec 8, 2023

donmccurdy deleted the donmccurdy/remove-BufferWithAccessor branch December 8, 2023 20:12

donmccurdy added this to the 9.0.0 milestone Feb 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[webgl] Remove BufferWithAccessor #1864

[webgl] Remove BufferWithAccessor #1864

donmccurdy commented Dec 5, 2023 •

edited

Loading

This comment was marked as off-topic.

ibgreen commented Dec 5, 2023

donmccurdy commented Dec 5, 2023 •

edited

Loading

ibgreen commented Dec 5, 2023

donmccurdy commented Dec 5, 2023

donmccurdy commented Dec 6, 2023

ibgreen commented Dec 6, 2023

donmccurdy commented Dec 7, 2023 •

edited

Loading

donmccurdy Dec 7, 2023

ibgreen Dec 7, 2023

donmccurdy Dec 7, 2023

donmccurdy Dec 7, 2023

ibgreen Dec 7, 2023

donmccurdy Dec 7, 2023 •

edited

Loading

ibgreen Dec 7, 2023

donmccurdy Dec 7, 2023

ibgreen Dec 7, 2023

donmccurdy Dec 7, 2023

ibgreen Dec 7, 2023

ibgreen Dec 7, 2023

donmccurdy commented Dec 7, 2023 •

edited

Loading

[webgl] Remove BufferWithAccessor #1864

[webgl] Remove BufferWithAccessor #1864

Conversation

donmccurdy commented Dec 5, 2023 • edited Loading

Status

This comment was marked as off-topic.

ibgreen commented Dec 5, 2023

donmccurdy commented Dec 5, 2023 • edited Loading

ibgreen commented Dec 5, 2023

donmccurdy commented Dec 5, 2023

donmccurdy commented Dec 6, 2023

ibgreen commented Dec 6, 2023

donmccurdy commented Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donmccurdy Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donmccurdy commented Dec 7, 2023 • edited Loading

donmccurdy commented Dec 5, 2023 •

edited

Loading

donmccurdy commented Dec 5, 2023 •

edited

Loading

donmccurdy commented Dec 7, 2023 •

edited

Loading

donmccurdy Dec 7, 2023 •

edited

Loading

donmccurdy commented Dec 7, 2023 •

edited

Loading