Skip to content
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

WebGLRenderer: Fix feedback loop with RTT read pixels async #29320

Merged
merged 3 commits into from
Sep 4, 2024
Merged

WebGLRenderer: Fix feedback loop with RTT read pixels async #29320

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
29363a9
Fix feedback loop with RTT read async
gkjohnson Sep 4, 2024
0ec462e
Update comments, error
gkjohnson Sep 4, 2024
5d99648
Update KTX2 exporter to use read pixels async
gkjohnson Sep 4, 2024
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
2 changes: 1 addition & 1 deletion examples/jsm/exporters/EXRExporter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ async function getPixelData( renderer, rtt, info ) {

}

renderer.readRenderTargetPixels( rtt, 0, 0, info.width, info.height, dataBuffer );
await renderer.readRenderTargetPixelsAsync( rtt, 0, 0, info.width, info.height, dataBuffer );
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I notice that the function signature for readRenderTargetPixelsAsync in WebGPURenderer doesn't take a dataBuffer argument which should probably be fixed to align with the WebGLRenderer implementation. Taking a databuffer was a deliberate decision to avoid memory allocation in cases where this is run ever frame or every few frames and is common pattern use throughout the library, as well, and I think it's worth keeping.

It looks like WebGPURenderer will now create a new typed array on every call.

Copy link
Collaborator

@Mugen87 Mugen87 Sep 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Do you mind updating KTX2Exporter as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've updated KTX2Exporter but haven't tested it since I don't see an example.

Also is it worth creating a new issue to track this function signature difference?

Copy link
Contributor

@aardgoose aardgoose Sep 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The WebGPU allocates the buffer returned internally, we just pass the required size etc. We effectively cast it to the correct type on return, but can't provide a buffer to be used, I think to ensure that the buffer alignment and location within memory is suitable for efficient CPU <-> GPU transfers.

The buffer is created at

three.js/src/renderers/webgpu/utils/WebGPUTextureUtils.js

Line 384 in 3b84981

const readBuffer = device.createBuffer(

And made available to JS here:

three.js/src/renderers/webgpu/utils/WebGPUTextureUtils.js

Line 415 in 3b84981

const buffer = readBuffer.getMappedRange();

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also is it worth creating a new issue to track this function signature difference?

Sounds good. Then we have a dedicated place to discuss this topic.

Copy link
Contributor

@aardgoose aardgoose Sep 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If there is a use case that requires constant readback the recommended method would be to create a pool of WebGPU buffer objects in a ring buffer arrangement to avoid repeated allocations, something that doesn't match the current renderer API.

Copy link
Collaborator

@Mugen87 Mugen87 Sep 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I have also realized that WebGPURenderer.readRenderTargetPixelsAsync() can not handle cube render targets so far since you can't assign an active cube face index. This is required for a bunch of use cases like e.g. in LightProbeGenerator.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll take a look.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Relevant line:

three.js/examples/jsm/lights/LightProbeGenerator.js

Line 165 in ce180c9

renderer.readRenderTargetPixels( cubeRenderTarget, 0, 0, imageWidth, imageWidth, data, faceIndex );


} else {

Expand Down
72 changes: 32 additions & 40 deletions src/renderers/WebGLRenderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2446,61 +2446,53 @@ class WebGLRenderer {

if ( framebuffer ) {

state.bindFramebuffer( _gl.FRAMEBUFFER, framebuffer );

try {

const texture = renderTarget.texture;
const textureFormat = texture.format;
const textureType = texture.type;

if ( ! capabilities.textureFormatReadable( textureFormat ) ) {

throw new Error( 'THREE.WebGLRenderer.readRenderTargetPixelsAsync: renderTarget is not in RGBA or implementation defined format.' );

}

if ( ! capabilities.textureTypeReadable( textureType ) ) {
const texture = renderTarget.texture;
const textureFormat = texture.format;
const textureType = texture.type;

throw new Error( 'THREE.WebGLRenderer.readRenderTargetPixelsAsync: renderTarget is not in UnsignedByteType or implementation defined type.' );
if ( ! capabilities.textureFormatReadable( textureFormat ) ) {

}
throw new Error( 'THREE.WebGLRenderer.readRenderTargetPixelsAsync: renderTarget is not in RGBA or implementation defined format.' );

// the following if statement ensures valid read requests (no out-of-bounds pixels, see #8604)
if ( ( x >= 0 && x <= ( renderTarget.width - width ) ) && ( y >= 0 && y <= ( renderTarget.height - height ) ) ) {
}

const glBuffer = _gl.createBuffer();
_gl.bindBuffer( _gl.PIXEL_PACK_BUFFER, glBuffer );
_gl.bufferData( _gl.PIXEL_PACK_BUFFER, buffer.byteLength, _gl.STREAM_READ );
_gl.readPixels( x, y, width, height, utils.convert( textureFormat ), utils.convert( textureType ), 0 );
_gl.flush();
if ( ! capabilities.textureTypeReadable( textureType ) ) {

// check if the commands have finished every 8 ms
const sync = _gl.fenceSync( _gl.SYNC_GPU_COMMANDS_COMPLETE, 0 );
await probeAsync( _gl, sync, 4 );
throw new Error( 'THREE.WebGLRenderer.readRenderTargetPixelsAsync: renderTarget is not in UnsignedByteType or implementation defined type.' );

try {
}

_gl.bindBuffer( _gl.PIXEL_PACK_BUFFER, glBuffer );
_gl.getBufferSubData( _gl.PIXEL_PACK_BUFFER, 0, buffer );
// the following if statement ensures valid read requests (no out-of-bounds pixels, see #8604)
if ( ( x >= 0 && x <= ( renderTarget.width - width ) ) && ( y >= 0 && y <= ( renderTarget.height - height ) ) ) {

} finally {
// set the active frame buffer to the one we want to read
state.bindFramebuffer( _gl.FRAMEBUFFER, framebuffer );

_gl.deleteBuffer( glBuffer );
_gl.deleteSync( sync );
const glBuffer = _gl.createBuffer();
_gl.bindBuffer( _gl.PIXEL_PACK_BUFFER, glBuffer );
_gl.bufferData( _gl.PIXEL_PACK_BUFFER, buffer.byteLength, _gl.STREAM_READ );
_gl.readPixels( x, y, width, height, utils.convert( textureFormat ), utils.convert( textureType ), 0 );
_gl.flush();

}
// reset the frame buffer to the currently set buffer before waiting
const currFramebuffer = _currentRenderTarget !== null ? properties.get( _currentRenderTarget ).__webglFramebuffer : null;
state.bindFramebuffer( _gl.FRAMEBUFFER, currFramebuffer );

return buffer;
// check if the commands have finished every 8 ms
const sync = _gl.fenceSync( _gl.SYNC_GPU_COMMANDS_COMPLETE, 0 );
await probeAsync( _gl, sync, 4 );

}
// read the data and delete the buffer
_gl.bindBuffer( _gl.PIXEL_PACK_BUFFER, glBuffer );
_gl.getBufferSubData( _gl.PIXEL_PACK_BUFFER, 0, buffer );
_gl.deleteBuffer( glBuffer );
_gl.deleteSync( sync );

} finally {
return buffer;

// restore framebuffer of current render target if necessary
} else {

const framebuffer = ( _currentRenderTarget !== null ) ? properties.get( _currentRenderTarget ).__webglFramebuffer : null;
state.bindFramebuffer( _gl.FRAMEBUFFER, framebuffer );
throw new Error( 'THREE.WebGLRenderer.readRenderTargetPixelsAsync: requested read bounds are out of range.' );

}

Expand Down