Add ImageBitmap as input of fromPixel for tfjs webgpu #3660

shaoboyan · 2020-07-27T02:27:55Z

This patch support imageBitmap as input type of fromPixel. By calling
copyImageBitmapToTexture to do upload.

This change is

shaoboyan · 2020-07-27T02:49:03Z

@qjia7 Please do first round review, thanks!

shaoboyan · 2020-07-27T07:37:12Z

Per @qjia7's suggestion, will update this patch to leverage FromPixelProgram mechanism.

shaoboyan · 2020-08-03T03:15:38Z

@qjia7 Update the patch and please do first round review if you have time!

qjia7 · 2020-08-03T09:02:29Z

tfjs-backend-webgpu/src/kernels/from_pixels_webgpu.ts


-        setOutput(floor(value * 255.0 + 0.5));
+    void main() {
+      ivec3 coords = getOutputCoordsIB();


Maybe you can directly use getCoordsFromFlatIndex(int(gl_GlobalInvocationID.x) * outShape.numChannels) function in shader_preprocessor.ts and don't need to define a new one.

Thanks for the suggestion. I can have a try.

It works fine! Done.

qjia7 · 2020-08-03T09:17:07Z

tfjs-backend-webgpu/src/backend_webgpu.ts

+
+      if (pixels instanceof ImageBitmap) {
+        const output = this.makeOutputArray(outShape, 'int32');
+        if (!this.psoForIBCompiled) {


Use if (! this.fromPixelProgram)? So you can remove posForIBCompiled.

qjia7 · 2020-08-03T09:19:49Z

tfjs-backend-webgpu/src/backend_webgpu.ts

+          // buffer
+          this.fromPixelProgram.setUniform(this.device, uniformData);
+
+          this.fromPixelProgram.setProperties(bindGroupLayout, pipeline);


Rename to setWebGPUBinary?

qjia7 · 2020-08-03T09:25:13Z

tfjs-backend-webgpu/src/kernels/from_pixels_webgpu.ts

+      GPUTexture {
+    if (!this.inputTexture || this.lastPixelSize.width !== pixelWidth ||
+        this.lastPixelSize.height !== pixelHeight) {
+      this.inputTexture = device.createTexture({


Do we need to care when to dispose this texture?

A good question. Per my opnion, this texture won't be reused in another program in this patch original design. Based on this, I'd like to leave the destruct of this inputTexture (and the uniform buffer) for js object lifecycle. WDYT?

I don't think you can rely on the js object lifecycle to manage GPU textures - but maybe I'm missing something - cc @kainino0x

oops nevermind kai just saw that you already addressed this - reviewable and github reviews don't work together sigh

@kainino0x @annxingyuan Thanks for inputs.
I refer to the sample here https://github.com/austinEng/webgpu-samples/blob/master/src/examples/texturedCube.ts and I think maybe leave the destruct task for GC is Ok(because we won't recycle them). But as Kai said, GC indeed cannot understand the VRAM pressure. I'll make a place to destruct them.

Done. (By adding dispose logic).

qjia7 · 2020-08-03T09:34:51Z

+@annxingyuan @kainino0x take a look.

kainino0x

Reviewable status: 0 of 1 approvals obtained (waiting on @annxingyuan, @qjia7, and @shaoboyan)

tfjs-backend-webgpu/src/backend_webgpu.ts, line 1186 at r2 (raw file):

          !(!(pixels instanceof ImageBitmap) &&
            (pixels.data instanceof Uint8Array)) &&
          !(pixels instanceof ImageBitmap)) {

I think if you do pixels instanceof ImageBitmap first then it shouldn't be necessary to inline the !(pixels instanceof ImageBitmap) && in the line above. (Maybe I'm misreading; this expression is really hard to read now that it's nested like this.)

      if (!(pixels instanceof HTMLVideoElement) &&
          !(pixels instanceof HTMLImageElement) &&
          !(pixels instanceof HTMLCanvasElement) &&
          !(pixels instanceof ImageData) &&
          !(pixels instanceof ImageBitmap) &&
          !(pixels.data instanceof Uint8Array)) {

tfjs-backend-webgpu/src/backend_webgpu.ts, line 1190 at r2 (raw file):

            'pixels passed to tf.browser.fromPixels() must be either an ' +
            `HTMLVideoElement, HTMLImageElement, HTMLCanvasElement, ImageData` +
            ` or {data: Uint32Array, width: number, height: number}, ` +

Hm, this says Uint32Array but the code says Uint8Array?

tfjs-backend-webgpu/src/kernels/from_pixels_webgpu.ts, line 101 at r1 (raw file):

Previously, shaoboyan (Yan, Shaobo) wrote…

A good question. Per my opnion, this texture won't be reused in another program in this patch original design. Based on this, I'd like to leave the destruct of this inputTexture (and the uniform buffer) for js object lifecycle. WDYT?

If there is a place where you can destroy it, it would be best - garbage collection is not very reliable right now for WebGPU objects because the GC doesn't understand VRAM pressure (so it just looks like a small wrapper object). But if there's no place, it's fine to let it GC.

tfjs-backend-webgpu/src/kernels/from_pixels_webgpu.ts, line 51 at r2 (raw file):

      int size;
      int width;
      int numChannels;} outShape;

nit: } should be on the next line.

tfjs-backend-webgpu/src/kernels/from_pixels_webgpu.ts, line 72 at r2 (raw file):

    }
    `;
    this.shaderKey = 'fromPixel';

Needs to include the workGroupSize

tfjs-backend-webgpu/src/kernels/from_pixels_webgpu.ts, line 89 at r2 (raw file):

  }

  getInputTexture(device: GPUDevice, pixelWidth: number, pixelHeight: number):

nit: since this creates a new object, it should say something like "create" or "make" instead of just "get".

tfjs-backend-webgpu/src/kernels/from_pixels_webgpu.ts, line 108 at r2 (raw file):

  }

  generateEncoder(device: GPUDevice, output: GPUBuffer, uniformData: number[]):

nit: Seems this only handles arrays of length 3. Can the type of uniformData be constrained further to [number, number, number]?

tfjs-backend-webgpu/src/kernels/from_pixels_webgpu.ts, line 114 at r2 (raw file):

         uniformData[1] !== this.lastUniformContent[1] ||
         uniformData[2] !== this.lastUniformContent[2])) {
      // TODO(tfjs-optimization): using new mapAsync API to update the buffer.

fyi, If this data is small (seems that it is) then writeBuffer would probably be fine. But you would have to implement the queueing yourself (to make sure it is called at the right time relative to the submits). (Though same with mapAsync if you're writing into the buffer directly instead of using a staging buffer like setUniform does now.)

tfjs-backend-webgpu/src/kernels/from_pixels_webgpu.ts, line 144 at r2 (raw file):

    passEncoder.setBindGroup(0, bindGroup);
    passEncoder.dispatch(this.dispatch[0], this.dispatch[1], this.dispatch[2]);
    // passEncoder.dispatch(Math.ceil(size / numChannels / workGroupSize));

nit: remove commented out code

annxingyuan · 2020-08-04T23:23:31Z

tfjs-backend-webgpu/src/backend_webgpu.ts

      numChannels: number): Tensor3D {
    if (pixels == null) {
      throw new Error(
          'pixels passed to tf.browser.fromPixels() can not be null');
    }

+    // TODO(tfjs-optimization): These has chance to be cached and do not update
+    // buffer always.


Could you explain this comment a little bit more?

Thanks for reviewing!
Sry this is the outdated TODO and I've move the related logic into the kernel. Will remove it.

annxingyuan · 2020-08-04T23:24:41Z

tfjs-backend-webgpu/src/backend_webgpu.ts

-          !(pixels.data instanceof Uint8Array)) {
+          !(!(pixels instanceof ImageBitmap) &&
+            (pixels.data instanceof Uint8Array)) &&
+          !(pixels instanceof ImageBitmap)) {
        throw new Error(
            'pixels passed to tf.browser.fromPixels() must be either an ' +
            `HTMLVideoElement, HTMLImageElement, HTMLCanvasElement, ImageData` +
            ` or {data: Uint32Array, width: number, height: number}, ` +


or ImageBitmap

annxingyuan · 2020-08-04T23:25:33Z

tfjs-backend-webgpu/src/backend_webgpu.ts

    let imageData = (pixels as ImageData | backend_util.PixelData).data;

    if (env().getBool('IS_BROWSER')) {
      if (!(pixels instanceof HTMLVideoElement) &&
          !(pixels instanceof HTMLImageElement) &&
          !(pixels instanceof HTMLCanvasElement) &&
          !(pixels instanceof ImageData) &&
-          !(pixels.data instanceof Uint8Array)) {
+          !(!(pixels instanceof ImageBitmap) &&


do we need to add another && condition to the pixels.data instanceof Uint8Array check?

Sry, I realized that the input could be the raw uint8Array. Added it.

annxingyuan · 2020-08-04T23:26:31Z

tfjs-backend-webgpu/src/backend_webgpu.ts

+              this.glslang, this.device, this.fromPixelProgram, [], output);
+
+          // TODO(tfjs-optimization): May use direct number instead of uniform
+          // buffer


what do you mean by use direct number?

This means the same way as current workGroupSize did (and generate multiple shaders)

Modify the comments.

annxingyuan · 2020-08-04T23:28:19Z

tfjs-backend-webgpu/src/kernels/from_pixels_webgpu.ts


  constructor(outputShape: number[]) {
    this.outputShape = outputShape;
    this.dispatchLayout = flatDispatchLayout(this.outputShape);
    this.dispatch = computeDispatch(this.dispatchLayout, this.outputShape);
-
+    const workGroupSize = 256;


just curious - how did you arrive at this number?

Haha, it is the experimental value (Jiajia provided some possible values and we did some experiments) :p

Sounds good :) Could you include a brief comment that you empirically determined this to be the best value?

annxingyuan · 2020-08-04T23:29:29Z

tfjs-backend-webgpu/src/kernels/from_pixels_webgpu.ts

+      int texC = coords[1];
+      int depth = coords[2];
+      vec4 values = imageLoad(srcImage, ivec2(texC, texR));
+      float value;


can value exist only in the scope of the for loop instead?

annxingyuan · 2020-08-04T23:30:36Z

tfjs-backend-webgpu/src/kernels/from_pixels_webgpu.ts

    `;
    this.shaderKey = 'fromPixel';
  }
+
+  setWebGPUBinary(


does it make sense for these methods (setwebgpubinary and setuniform) to exist on the backend interface instead?

I think the best place to put these logic is compileAndRun. I think setWebGPUBinary and generateEncoder are only introduced by fromPixel only due to compileAndRun doesn't support texture as input.
I think there is no other ops can share these logic. So I'd like to keep them here until compileAndRun can support texture as input. I'll use a CL to remove them . WDYT?

Sounds good!

annxingyuan · 2020-08-04T23:32:42Z

tfjs-backend-webgpu/src/kernels/from_pixels_webgpu.ts

+      GPUTexture {
+    if (!this.inputTexture || this.lastPixelSize.width !== pixelWidth ||
+        this.lastPixelSize.height !== pixelHeight) {
+      this.inputTexture = device.createTexture({


I don't think you can rely on the js object lifecycle to manage GPU textures - but maybe I'm missing something - cc @kainino0x

annxingyuan · 2020-08-04T23:34:18Z

tfjs-backend-webgpu/src/kernels/from_pixels_webgpu.ts

+    this.uniform = uniformBuffer;
+  }
+
+  getInputTexture(device: GPUDevice, pixelWidth: number, pixelHeight: number):


there is a webgpu buffer manager (buffer_manager.ts) that takes care of recycling buffers whenever possible and allocating new buffers - maybe we could introduce a similar mechanism for textures

Yes, agree. But for fromPixel, the inputTexture won't be created/recycle in high frequency. (And I think other ops don't use texture) So I think maybe for fromPixels, we can keep this simple management WDYT?

annxingyuan · 2020-08-04T23:35:32Z

tfjs-backend-webgpu/src/kernels/from_pixels_webgpu.ts

+    return this.inputTexture;
+  }
+
+  generateEncoder(device: GPUDevice, output: GPUBuffer, uniformData: number[]):


again would it be possible to move this logic into the backend so it can be shared?

shaoboyan · 2020-08-05T11:18:19Z

@kainino0x sry I don't know how to reply to your comments (there is no comment text area). But I think all comments should have been addressed in latest patch.
@kainino0x @annxingyuan address most comments, PTAL again, thanks!

kainino0x · 2020-08-06T00:01:18Z

@kainino0x sry I don't know how to reply to your comments (there is no comment text area). But I think all comments should have been addressed in latest patch.

You can do it by opening Reviewable (the link is in the PR description). But no need, thanks for the revision! I'm really busy now, but will take a look when I can.

kainino0x · 2020-08-06T00:02:09Z

(No need to wait for me to rereview before merging, though, if it's ready.)

annxingyuan

Thanks @shaoboyan ! Just left one last tiny comment and then I will merge!

qjia7 · 2020-08-07T07:09:03Z

tfjs-backend-webgpu/src/kernels/from_pixels_webgpu.ts

+  inputTexture: GPUTexture = null;
+  lastPixelSize = {width: 0, height: 0};
+
+  private disposed = false;

  constructor(outputShape: number[]) {
    this.outputShape = outputShape;
    this.dispatchLayout = flatDispatchLayout(this.outputShape);
    this.dispatch = computeDispatch(this.dispatchLayout, this.outputShape);


Sorry that I didn't notice it earlier. But your dispatch should be like below. With this change, I think your shader should be accelerated a lot.

this.workGroupSize = [256, 1, 1]; // Maybe other values. Please do another round of experiments. this.workPerThread = numChannels; this.dispatch = computeDispatch(this.dispatchLayout, this.outputShape, this.workGroupSize, [this.workPerThread, 1 , 1]);

qjia7 · 2020-08-07T07:09:49Z

tfjs-backend-webgpu/src/kernels/from_pixels_webgpu.ts

@@ -26,35 +26,144 @@ export class FromPixelsProgram implements WebGPUProgram {
  variableNames = ['A'];


['A'] is not needed ?

Done. And by assign variableNames to [], we don't need the change in shader_preprocessor file.

This patch support imageBitmap as input type of fromPixel. By calling copyImageBitmapToTexture to do upload.

qjia7 · 2020-08-11T06:15:23Z

LGTM. Thanks.

shaoboyan · 2020-08-14T00:22:46Z

@annxingyuan @qjia7 I don't think the bots error is related to this patch because the build fail log here https://console.cloud.google.com/cloud-build/builds/bd30c441-a3f5-4941-8607-ac52057f34a9;step=5?project=learnjs-174218
says:
Chrome 84.0.4147 (Mac OS X 10.12.6) ERROR
Disconnected, because no message in 30000 ms.
Am I right?

Currently async fromxPixels is not done in different backend. For WebGPU backend, the WIP PR is tensorflow/tfjs#3660.

googlebot added the cla: yes label Jul 27, 2020

rthadur requested a review from annxingyuan July 27, 2020 19:33

shaoboyan force-pushed the webgpu branch from 5a419c6 to c361902 Compare August 3, 2020 03:14

shaoboyan force-pushed the webgpu branch from c361902 to ec197e0 Compare August 3, 2020 09:08

qjia7 reviewed Aug 3, 2020

View reviewed changes

shaoboyan force-pushed the webgpu branch from ec197e0 to a791435 Compare August 4, 2020 08:20

kainino0x suggested changes Aug 4, 2020

View reviewed changes

annxingyuan requested changes Aug 4, 2020

View reviewed changes

shaoboyan force-pushed the webgpu branch 2 times, most recently from 3bac731 to 2cece04 Compare August 5, 2020 11:15

annxingyuan approved these changes Aug 6, 2020

View reviewed changes

shaoboyan force-pushed the webgpu branch 2 times, most recently from 9a31949 to d0f856c Compare August 7, 2020 02:09

qjia7 reviewed Aug 7, 2020

View reviewed changes

shaoboyan force-pushed the webgpu branch 3 times, most recently from 63c4ad1 to 35f1649 Compare August 10, 2020 09:45

WebGPU: Add ImageBitmap as input of fromPixel for tfjs webgpu

c066d9e

This patch support imageBitmap as input type of fromPixel. By calling copyImageBitmapToTexture to do upload.

shaoboyan force-pushed the webgpu branch from 35f1649 to c066d9e Compare August 11, 2020 05:06

axinging added a commit to axinging/tfjs-models that referenced this pull request Aug 19, 2020

Use async fromPixels in posenet

d6a22b7

Currently async fromxPixels is not done in different backend. For WebGPU backend, the WIP PR is tensorflow/tfjs#3660.

axinging added a commit to axinging/tfjs-models that referenced this pull request Sep 25, 2020

Use async fromPixels in posenet

8ccb75f

Currently async fromxPixels is not done in different backend. For WebGPU backend, the WIP PR is tensorflow/tfjs#3660.

axinging added a commit to axinging/tfjs-models that referenced this pull request Sep 25, 2020

Use async fromPixels in posenet

b15ea46

Currently async fromxPixels is not done in different backend. For WebGPU backend, the WIP PR is tensorflow/tfjs#3660.

axinging added a commit to axinging/tfjs-models that referenced this pull request Sep 27, 2020

Use async fromPixels in posenet

1122160

Currently async fromxPixels is not done in different backend. For WebGPU backend, the WIP PR is tensorflow/tfjs#3660.

axinging added a commit to axinging/tfjs-models that referenced this pull request Sep 27, 2020

Use async fromPixels in posenet

b0995ee

Currently async fromxPixels is not done in different backend. For WebGPU backend, the WIP PR is tensorflow/tfjs#3660.

axinging added a commit to axinging/tfjs-models that referenced this pull request Sep 28, 2020

Use async fromPixels in posenet

0ee4150

Currently async fromxPixels is not done in different backend. For WebGPU backend, the WIP PR is tensorflow/tfjs#3660.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ImageBitmap as input of fromPixel for tfjs webgpu #3660

Add ImageBitmap as input of fromPixel for tfjs webgpu #3660

		@@ -26,35 +26,144 @@ export class FromPixelsProgram implements WebGPUProgram {
		variableNames = ['A'];

Add ImageBitmap as input of fromPixel for tfjs webgpu #3660

Are you sure you want to change the base?

Add ImageBitmap as input of fromPixel for tfjs webgpu #3660

Conversation

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

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

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

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

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

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