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

[WebGL] Texture to tensor API #6853

Merged
merged 51 commits into from Oct 9, 2022
Merged

[WebGL] Texture to tensor API #6853

merged 51 commits into from Oct 9, 2022

Conversation

Linchenn
Copy link
Collaborator

@Linchenn Linchenn commented Sep 24, 2022

This fixes #3937

Changes

  • Add a new method to the API: tf.tensor(values: WebGLData, shape, dtype) that allows users to create tensors from textures and WebGLData is an object of WebGLTexture, height, width and channels properties and the channels 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 Reviewable

Copy link
Collaborator

@qjia7 qjia7 left a 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:

  1. 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?
  2. dataToGPU has been exposed by tfjs-core. It's wired that createTensorFromTexture is not like this way. And you also mentioned that the user can get texture by tf.backend().getTexture(a.dataId). So dataToGPU is duplicated with getTexture from functionality?

tfjs-backend-webgl/src/backend_webgl.ts Show resolved Hide resolved
tfjs-backend-webgl/src/webgl.ts Outdated Show resolved Hide resolved
* @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
Copy link
Collaborator

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

* 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,
Copy link
Collaborator

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)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done.

const texConfig = backend.gpgpu.textureConfig;
let params: gpgpu_util.TextureCreationParams;

const isPacked = textureFormat === gl.RGBA;
Copy link
Collaborator

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.

Copy link
Collaborator Author

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'.

Copy link
Collaborator

@lina128 lina128 left a 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 to gl.RGBA?

In fact, I prefer you support both unpacked texture and the densely rgba texture as the input.

Agreed with Jiajia.

@arpu
Copy link

arpu commented Sep 27, 2022

this is amazing! any code snipe for threejs ?

Copy link
Collaborator

@lina128 lina128 left a 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: :shipit: complete! 1 of 1 approvals obtained (waiting on @Linchenn and @qjia7)

Copy link
Collaborator

@pyu10055 pyu10055 left a 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: :shipit: 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

@Linchenn Linchenn requested a review from pyu10055 October 5, 2022 17:36
Copy link
Collaborator Author

@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.

Reviewable status: :shipit: 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!

Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: 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)

Copy link
Collaborator Author

@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.

Reviewable status: :shipit: 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!

@Linchenn Linchenn requested a review from pyu10055 October 5, 2022 20:58
Copy link
Collaborator

@pyu10055 pyu10055 left a 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: :shipit: complete! 2 of 1 approvals obtained (waiting on @lina128, @pyu10055, and @qjia7)

Copy link
Collaborator

@qjia7 qjia7 left a 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.

* 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
Copy link
Collaborator

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

Copy link
Collaborator Author

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!

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];
Copy link
Collaborator

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great catch, done.

Copy link
Collaborator Author

@Linchenn Linchenn Oct 8, 2022

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 {
Copy link
Collaborator

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?

Copy link
Collaborator Author

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);
Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

@Linchenn Linchenn requested a review from qjia7 October 8, 2022 18:41
const physicalShape: [number, number] = [width, height];
const a = tf.tensor({texture, height, width, channels: 'RGBA'});

expect(a.shape).toEqual(physicalShape);
Copy link
Collaborator

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]);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done. Thanks!

Copy link
Collaborator

@qjia7 qjia7 left a 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.

@qjia7
Copy link
Collaborator

qjia7 commented Oct 9, 2022

cc @axinging You can continue the implementation in webgpu. Please refer to the doc of Life of a Tensor and the implementation here. Thanks.

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.

[webgl] allow direct WebGL tensor creation/retrieval using texture handle
5 participants