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.

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

[webgpu] Implement draw API #7749

Merged
merged 6 commits into from
Jun 28, 2023
Merged

Conversation

axinging
Copy link
Contributor

@axinging axinging commented Jun 9, 2023

To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.

@axinging axinging force-pushed the tfdraw-singlepass branch 4 times, most recently from 7775e4b to efbccc5 Compare June 12, 2023 06:37
@axinging axinging marked this pull request as ready for review June 12, 2023 07:25
@axinging
Copy link
Contributor Author

@qjia7 @xhcao @gyagp @hujiajie PTAL

}
canvas2dContext.drawImage(canvasWebGPU, 0, 0);
}
backend.disposeData(output.dataId);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am wondering: is it possible that we meet this error since this texture is generated by gpuContext.getCurrentTexture() not textureManager?

Cannot release a texture that was never provided by this texture manager

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, this tensor should be marked as external.

@@ -19,6 +19,11 @@ import {backend_util, DataType, DataTypeMap, env, Rank, TensorInfo, util} from '

import {symbolicallyComputeStrides} from './shader_util';

export enum PixelsOpType {
FROM_PIXELS,
TO_PIXELS
Copy link
Collaborator

Choose a reason for hiding this comment

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

To keep consistent, should we rename TO_PIXELS to DRAW?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, PTAL

@qjia7 qjia7 requested review from Linchenn, gyagp and xhcao June 13, 2023 05:24
let gpuContext = canvas.getContext(backendName);
let canvasWebGPU;
if (!gpuContext) {
canvasWebGPU = new OffscreenCanvas(width, height);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, forget one. Maybe we should cache the canvasWebGPU if needed. Otherwise, it will be created every time when we call the Draw API. And the configure only needs to be called once if the canvas doesn't change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

canvasWebGPU is a fallback when a 2d canvas is used in webgpu context. To support the cache, we should use width, height as key, and I don't know when to free this cache, may be need a new API to free.
So I suggest donot support cache in this PR, maybe in next PR.

Copy link
Collaborator

@Linchenn Linchenn left a comment

Choose a reason for hiding this comment

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

Thanks for the amazing implementation, LGTM except a nit.

tfjs-core/src/ops/draw_test.ts Outdated Show resolved Hide resolved
@axinging axinging force-pushed the tfdraw-singlepass branch 2 times, most recently from 7f22d76 to 23925aa Compare June 25, 2023 07:38
@axinging
Copy link
Contributor Author

@Linchenn, your comment is resolved, PTAL.

@Linchenn
Copy link
Collaborator

@Linchenn, your comment is resolved, PTAL.

LGTM, approved, thanks!

@Linchenn Linchenn enabled auto-merge (squash) June 28, 2023 00:13
@Linchenn Linchenn merged commit 139f595 into tensorflow:master Jun 28, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants