-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
[WebGL] Texture to tensor API #6853
Conversation
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 current design is to extend and expose the webgl backend's capacity directly but not expose it through tfjs-core. So you have come to an agreement to do it like this?
Two questions in my mind:
- Usually, users check https://js.tensorflow.org/api/latest/ to get all latest APIs. How should we let users know the backend directly exposed APIs?
dataToGPU
has been exposed by tfjs-core. It's wired thatcreateTensorFromTexture
is not like this way. And you also mentioned that the user can get texture bytf.backend().getTexture(a.dataId)
. SodataToGPU
is duplicated withgetTexture
from functionality?
tfjs-backend-webgl/src/webgl.ts
Outdated
* @param texture The WebGL texture to create a tensor from. The texture must | ||
* be unpacked - each texel should only store a single value. The flattened | ||
* values of the tensor will be read from left to right, and top to bottom. The | ||
* texture can have empty texels at the end, but the values must be written |
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 empty texels mean zero texels, right? When the texel size is larger than shape size, the extra texels will be filled with zero.
And it's confused to say that all empty texels must be contiguous
.
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.
Done.
tfjs-backend-webgl/src/webgl.ts
Outdated
* densely - in other words, all empty texels must be contiguous. | ||
* @param shape The logical shape of the texture. | ||
* @param dtype The dtype of the tensor to be created. | ||
* @param texShapeRC The physical dimensions of the texture expressed as [rows, |
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.
Is it better to use the same name with gl.texImage2D
, like width/height
? So you can say The width of the texture provided to gl.texImage2D during texture creation.
The height of the texture provided to gl.texImage2D during texture creation.
. Similar to the other params (internalFormat -> internalformat, textureFormat -> format, textureType -> type)?
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.
done.
tfjs-backend-webgl/src/webgl.ts
Outdated
const texConfig = backend.gpgpu.textureConfig; | ||
let params: gpgpu_util.TextureCreationParams; | ||
|
||
const isPacked = textureFormat === gl.RGBA; |
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.
Since you require that the texel of input texture must be one value, it's not possible that textureFormat
is equal to gl.RGBA
?
In fact, I prefer you support both unpacked texture and the densely rgba texture as the input.
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.
Done. Now, we support all non-empty subsequence of 'RGBA'.
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.
Hi Lin, thank you for your hard work on this important feature! Sorry I probably wasn't clear when explaining the design of this method. I meant to say that let's follow Jiajia's Life of a Tensor design of the API, please refer to the doc for detail. Basically, the method should be in tensor.ts's constructor function. async tensor(...), it's at tfjs-core level. Also, we should support this two formats, R32F and RGBA, for R32, just pass through, for RGBA, do dense to unpacked using encodeMatrix. Thank you so much!!! I think only this two changes, plus one more test, then I'm all good. Also, can you also add the following reference to your PR description? Thanks. ref:#6848
Reviewable status: 0 of 1 approvals obtained (waiting on @Linchenn and @qjia7)
tfjs-backend-webgl/src/backend_webgl.ts
line 181 at r1 (raw file):
// Writes a new entry to the data store with a WebGL texture, and registers it // to the texture manager.
The doccomment is outdated, update?
Code quote:
// Writes a new entry to the data store with a WebGL texture, and registers it
// to the texture manager.
tfjs-backend-webgl/src/backend_webgl.ts
line 219 at r1 (raw file):
gpgpu_math.runProgram( this.gpgpu, binary, inputTensorData, outputTensorData, null /* customUniformValues */);
We can provide uniform values for texShape is useUniform flag is true, right?
Code quote:
/* customUniformValues */
tfjs-backend-webgl/src/backend_webgl_test.ts
line 1331 at r1 (raw file):
const texture = gl.createTexture(); const tex2d = gl.TEXTURE_2D; const internalFormat = gl.RGBA;
Can we add a test that takes a texture with RGBA format and densely laid out? Basically, for values 0-11, only need a texture with 3 texels.
Code quote:
gl.RGBA
tfjs-backend-webgl/src/webgl.ts
line 97 at r8 (raw file):
Previously, qjia7 (Jiajia Qin) wrote…
Why we don't accept the densely rgba texture?
Can we accept dense rgba texture? I think we want to support two texture formats: R32 and RGBA. And we check, if R32, then we just clone the texture, it's already unpacked, if RGBA, we run the encodeMatrix to convert the layout from dense to unpacked. Basically we output a tensor with unpacked texture (same behavior has creating a tensor with value on CPU, which will create an unpacked texture).
tfjs-backend-webgl/src/webgl.ts
line 135 at r8 (raw file):
Previously, qjia7 (Jiajia Qin) wrote…
Since you require that the texel of input texture must be one value, it's not possible that
textureFormat
is equal togl.RGBA
?In fact, I prefer you support both unpacked texture and the densely rgba texture as the input.
Agreed with Jiajia.
this is amazing! any code snipe for threejs ? |
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 implementation is so clean, thank you Lin! LGTM. Clean up test and debug code before merge.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @Linchenn and @qjia7)
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.
great work, thanks!
Reviewable status: complete! 2 of 1 approvals obtained (waiting on @lina128, @Linchenn, @pyu10055, and @qjia7)
tfjs-backend-webgl/src/backend_webgl.ts
line 1320 at r28 (raw file):
throw new Error( `The texture is invalid. Also, please make sure the texture and ` + `the TFJS WebGL backend are using same canvas. If you want to use ` +
using the same cavas
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.
Reviewable status: complete! 2 of 1 approvals obtained (waiting on @lina128, @pyu10055, and @qjia7)
tfjs-backend-webgl/src/backend_webgl.ts
line 1320 at r28 (raw file):
Previously, pyu10055 (Ping Yu) wrote…
using the same cavas
Done. Thanks!
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.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @lina128, @Linchenn, @pyu10055, and @qjia7)
tfjs-core/src/ops/tensor.ts
line 89 at r29 (raw file):
* // channels of Pixel1... * * // For postprocessing on the GPU, it's possible to retrieve the texture
probably they should use the dataToGPU API for post processing, not grabbing the texture directly (otherwise texture is in packed 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.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @lina128, @pyu10055, and @qjia7)
tfjs-core/src/ops/tensor.ts
line 89 at r29 (raw file):
Previously, pyu10055 (Ping Yu) wrote…
probably they should use the dataToGPU API for post processing, not grabbing the texture directly (otherwise texture is in packed format)
Great catch! Thanks!
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.
Reviewed 1 of 7 files at r10, 1 of 1 files at r13, 2 of 3 files at r16, 5 of 9 files at r22, 1 of 5 files at r25, 1 of 1 files at r26, 1 of 3 files at r27, 1 of 2 files at r28, 1 of 1 files at r29, 1 of 1 files at r30, all commit messages.
Reviewable status: complete! 2 of 1 approvals obtained (waiting on @lina128, @pyu10055, and @qjia7)
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.
Thanks Lin. It looks great. Just some small comments.
tfjs-core/src/ops/tensor.ts
Outdated
* they will be encoded as utf-8 and kept as `Uint8Array[]`. | ||
* or a flat array, or a `TypedArray`, or a `WebGLData` object. If the | ||
* values are strings, they will be encoded as utf-8 and kept as `Uint8Array[]`. | ||
* If the values is a `WebGLData` object, the texture must share the same |
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.
It's better to also declare that the internal texture format for the input texture must be floating point or normalized integer since we are using floating point sampler type in shader. Otherwise, if the users give a signed/unsigned integer texture, they will meet unknown issues.
See spec https://registry.khronos.org/OpenGL/specs/es/3.0/GLSL_ES_Specification_3.00.pdf 8.8 Texture Lookup Functions
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.
Thank you for the reminder and the reference!
tfjs-core/src/tensor_util_env.ts
Outdated
let firstElem: typeof val = val; | ||
|
||
if (isTypedArray(val)) { | ||
return dtype === 'string' ? [] : [val.length]; | ||
} | ||
if (typeof val === 'object' && 'texture' in val) { | ||
return [val.height, val.width]; |
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.
Why not use return [val.height, val.width * val.channels.length()]
? Otherwise, A portion of the data will be truncated.
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.
Great catch, done.
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.
Also, since we need to fill 'RGBA' as default if channels
parameter is null. I moved this 'filling default' logics from backend to core, because to do this inferring shape in core, we also need to know the channels in core.
// to the texture manager. | ||
writeTexture( | ||
texture: WebGLTexture, shape: number[], dtype: DataType, | ||
texHeight: number, texWidth: number, color: string): DataId { |
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.
nit: color
-> channels
to keep consistent?
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.
Done.
|
||
const shapeAs3D = webgl_util.getShapeAs3D(shape); | ||
const program = | ||
new EncodeMatrixProgram(shapeAs3D, false /* isByteArray */, color); |
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.
Do we need to provide a special path for RGBA
by calling EncodeMatrixPackedProgram
? In this case, we can avoid once runWebGLProgram
to pack the tensor (PackProgram
) in many situations.
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.
RGBA input's packing schema is dense packing, while our tfjs's packing schema is square packing. As a result, even though both are packed (using all RGBA channels), we may need to transform the values through EncodeMatrixPackedProgram
.
const physicalShape: [number, number] = [width, height]; | ||
const a = tf.tensor({texture, height, width, channels: 'RGBA'}); | ||
|
||
expect(a.shape).toEqual(physicalShape); |
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.
Since you have changed the inferShape
, here also needs to be updated.
expect(a.shape).toEqual([width, height * channels.length]);
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.
Done. Thanks!
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.
LGTM after bots become green. Thanks.
PS: Please also update your PR description to align with your current changes.
cc @axinging You can continue the implementation in webgpu. Please refer to the doc of Life of a Tensor and the implementation here. Thanks. |
This fixes #3937
Changes
tf.tensor(values: WebGLData, shape, dtype)
that allows users to create tensors from textures andWebGLData
is an object ofWebGLTexture
,height
,width
andchannels
properties and thechannels
could be a non-empty subset of 'RGBA' in any order.Reference:
#6848 (The current PR starts from this PR of Na.)
#4376
To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.
This change is