webgpu: Make fromPixels not so special #6549

gyagp · 2022-06-17T09:04:09Z

This is to unify the implementation of fromPixels with other ops for better maintenance.

Close #5536

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

This change is

gyagp · 2022-06-18T11:38:05Z

@qjia7 @axinging @xhcao PTAL!

qjia7

LGTM, thanks!

xhcao · 2022-06-21T00:49:00Z

tfjs-backend-webgpu/src/backend_webgpu.ts

+      // Short-circuit the computation since the result is empty (has 0 in its
+      // shape).
+      const outData = this.tensorMap.get(output.dataId);
+      outData.values =


Is there a situation that output is not null, but output.shape is 0, output is not recorded in tensorMap? if so, outData is null.

All the tensor should be recorded in tensorMap. If output is not null, then it's already recorded.

xhcao · 2022-06-21T03:36:18Z

tfjs-backend-webgpu/src/backend_webgpu.ts

-          this.device, program, pipelineLayout, inputsData, output);
-    });
+    const pipeline = key in this.pipelineCache ?
+        this.pipelineCache[key] :


Where is the place to add elements into pipelineCache?

Good catch, will fix in next update.

gyagp · 2022-06-22T03:55:58Z

@qjia7 @axinging @xhcao I did further refactoring to unify the texture and buffer. Please review this again.

qjia7

Please also check whether all fromPixels cases pass if set WEBGPU_USE_IMPORT to true.

qjia7 · 2022-06-22T07:22:38Z

tfjs-backend-webgpu/src/backend_webgpu.ts

    const tensorData = this.tensorMap.get(tensor.dataId);
-
+    if (tensorData.textureInfo) {
+      return (tensorData.textureInfo.texture as GPUTexture).createView();


Forget the situation that tensorData.textureInfo.texture may be a GPUExternalTexture?

qjia7 · 2022-06-22T07:47:49Z

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

+      const externalTextureDescriptor = {source: pixels as HTMLVideoElement};
+      texture = backend.device.importExternalTexture(externalTextureDescriptor);
+      textureInfo = {
+        width: pixels.width,


Can you directly use the width height variables? pixels.width and pixels.height may not equal to width height in outputShape (see L57).

qjia7 · 2022-06-22T07:50:46Z

tfjs-backend-webgpu/src/backend_webgpu.ts

-      format: textureFormat,
-      usage: textureUsage,
+    return {
+      width: externalImage.width,


Be careful that externalImage.width may be not equal to outShape[1]

axinging · 2022-06-23T00:15:52Z

tfjs-backend-webgpu/src/backend_webgpu.ts

    });
    this.uniformDisposalQueue.forEach(
-        d => this.bufferManager.releaseBuffer(d.buffer, d.byteSize, d.usage));
+        d => this.bufferManager.releaseBuffer(d.buffer, d.size, d.usage));


LGTM.
Maybe we can merge releaseResource and releaseBuffer in the next change.

releaseResource includes the buffer and texture management, so they don't need to be merged.

gyagp · 2022-06-23T10:40:53Z

@qjia7 I enabled the fromPixels tests with both WEBGPU_USE_IMPORT true and false, and make sure the video is correctly loaded. Please take another look. Thanks!

qjia7

LGTM, thanks!

gyagp · 2022-06-23T21:32:52Z

@xhcao Will you take another look at this PR?

xhcao

LGTM, thanks.

hujiajie · 2022-06-27T03:44:42Z

Has anyone encountered this error after the change?

Chrome 105.0.0.0 (Windows 10) webgpu backend webgpu webgpu {"WEBGPU_CPU_FORWARD":false} should behave well if WEBGPU_USER_IMPORT is true or false FAILED
 Error: The video element has not loaded data yet. Please wait for `loadeddata` event on the <video> element.
     at fromPixels_ (tfjs-core/src/ops/browser.ts:115:13 <- tfjs/tfjs-backend-webgpu/src/tfjs-backend-webgpu_test_bundle.js:5941:15)
     at Object.<anonymous> (tfjs-core/src/ops/browser.ts:279:10 <- tfjs/tfjs-backend-webgpu/src/tfjs-backend-webgpu_test_bundle.js:6021:14)
     at <Jasmine>
     at tfjs/tfjs-backend-webgpu/src/tfjs-backend-webgpu_test_bundle.js:52:64
     at <Jasmine>
     at __async (tfjs-backend-webgpu/src/tfjs-backend-webgpu_test_bundle.js:36:12)
     at Object.fromPixelsAsync (tfjs-backend-webgpu/src/tfjs-backend-webgpu_test_bundle.js:6004:12)
     at ../../tfjs-backend-webgpu/src/backend_webgpu_test.ts:248:36 <- tfjs/tfjs-backend-webgpu/src/tfjs-backend-webgpu_test_bundle.js:84990:47
     at <Jasmine>
     at fulfilled (tfjs-backend-webgpu/src/tfjs-backend-webgpu_test_bundle.js:39:27)

hujiajie · 2022-06-28T09:09:38Z

I did more tests today:

the issue seems to be reproducible every time on UHD 620
the issue happens occasionally (maybe <10%) on UHD 630
the issue was not reproduced if I disabled hardware video decoding on UHD 620

gyagp · 2022-06-29T00:18:36Z

I suggest to open a separate issue for this. Like we discussed in the meeting, can you report the readyState? The Error triggered in TFJS is not necessary and wrong, maybe we can simply remove it.

gyagp requested a review from qjia7 June 17, 2022 09:04

gyagp force-pushed the frompixels2 branch from 4dbbea7 to 3e2391a Compare June 18, 2022 08:56

gyagp changed the title ~~webgpu: Make fromPixels a plain op~~ webgpu: Make fromPixels not so special Jun 18, 2022

gyagp force-pushed the frompixels2 branch 2 times, most recently from dc53dc9 to ee3cb16 Compare June 18, 2022 11:37

webgpu: Make fromPixels as a plain op

960364d

gyagp force-pushed the frompixels2 branch from ee3cb16 to 960364d Compare June 19, 2022 11:16

qjia7 approved these changes Jun 20, 2022

View reviewed changes

xhcao reviewed Jun 21, 2022

View reviewed changes

gyagp force-pushed the frompixels2 branch from 11f6a3a to 1ca0df9 Compare June 22, 2022 03:43

gyagp force-pushed the frompixels2 branch from c930dd2 to 87d61db Compare June 22, 2022 06:05

Unify texture and buffer management

db8ac7f

gyagp force-pushed the frompixels2 branch from 87d61db to db8ac7f Compare June 22, 2022 07:10

qjia7 reviewed Jun 22, 2022

View reviewed changes

Address comments

0e58f86

axinging reviewed Jun 23, 2022

View reviewed changes

Add tests for WEBGPU_USE_IMPORT

9516ba1

gyagp force-pushed the frompixels2 branch from fa49cdd to 9516ba1 Compare June 23, 2022 10:33

qjia7 approved these changes Jun 23, 2022

View reviewed changes

Make fromPixels tests more clear

20b2702

gyagp force-pushed the frompixels2 branch from b20afb0 to 20b2702 Compare June 23, 2022 21:11

xhcao approved these changes Jun 24, 2022

View reviewed changes

Split the test case

a243c94

gyagp force-pushed the frompixels2 branch from 4eb5a4e to a243c94 Compare June 24, 2022 01:49

Merge branch 'master' into frompixels2

62e7796

gyagp merged commit c4c1e18 into tensorflow:master Jun 24, 2022

gyagp deleted the frompixels2 branch June 24, 2022 07:31

hujiajie mentioned this pull request Jul 15, 2022

De-flake fromPixels test failure #6626

Merged

webgpu: Make fromPixels not so special #6549

webgpu: Make fromPixels not so special #6549

Uh oh!

Conversation

gyagp commented Jun 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gyagp commented Jun 18, 2022

Uh oh!

qjia7 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gyagp commented Jun 22, 2022

Uh oh!

qjia7 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gyagp commented Jun 23, 2022

Uh oh!

qjia7 left a comment

Choose a reason for hiding this comment

Uh oh!

gyagp commented Jun 23, 2022

Uh oh!

xhcao left a comment

Choose a reason for hiding this comment

Uh oh!

hujiajie commented Jun 27, 2022

Uh oh!

hujiajie commented Jun 28, 2022

Uh oh!

gyagp commented Jun 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gyagp commented Jun 17, 2022 •

edited

Loading