Add fromPixels support for webworker

[WenheLI]

This pr add fromPixels support for the Webworker environment.

---

This change is 

[tafsiri]

Hi @WenheLI, after chatting with @nsthorat we figured that there are two pre-requisites we need to do before merging this PR.

1. Add more tests for tf.fromPixels for existing code paths. I made an issue describing what we need to do here.
2. Investigate running tests in a webworker (link to issue). This will enable us to get a better picture of what is working and what isn't in a webworker. This is a bit more of an open ended exploration, so even putting notes/code snippets in the issue would be helpful.

Would you be willing to look into this? I'd say having 1) complete is definitely needed before merging this PR and 2) is helpful but based on what we find we can decide how to proceed.

[WenheLI]

Hi @tafsiri, sure things! I will look into them later!

[dsmilkov]

Thanks Wenhe for the PR! Left some suggestions below

Reviewable status: 0 of 1 approvals obtained (waiting on @WenheLI)

---

src/backends/cpu/backend_cpu.ts, line 151 at r1 (raw file):

    } else if (
        //@ts-ignore
        typeof(WorkerGlobalScope) !== 'undefined' &&

Looks like this will break the existing cpu backend when running in the browser. Curious, why do you need to add this check in here?

---

src/backends/webgl/backend_webgl.ts, line 281 at r1 (raw file):

    if (ENV.getBool('IS_BROWSER')) {
      //@ts-ignore
      if (((typeof(WorkerGlobalScope) === 'undefined' &&

instead of growing the already complex if statement, how about adding an earlier check for webworkers. Duplicating code is completely fine in this case. Something like:

if (typeof(WorkerGlobalScope) !== 'undefined) {
  // Make sure pixels is either OffscreenCanvas or PixelData.
}
// Leave the previous logic the same.

---

src/backends/webgl/backend_webgl.ts, line 298 at r1 (raw file):

      }
      //@ts-ignore
      if (typeof(WorkerGlobalScope) === 'undefined'

is the goal of this check to make sure that instanceof HTMLVideoElement does not run when in a web worker? In yes, then let's be explicit about that by feature testing for HTMLVIdeoElement. Something like:

if (typeof(HTMLVideoElement) !== 'undefined' && pixels instanceof HTMLVideoELement) {
// Keep the existing code inside.
}

[nsthorat]

Reviewable status: 0 of 1 approvals obtained (waiting on @dsmilkov)

---

src/backends/cpu/backend_cpu.ts, line 151 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

Looks like this will break the existing cpu backend when running in the browser. Curious, why do you need to add this check in here?

Do the tests you added fail for this? If not, we should make sure the head tests do fail with this change.

---

src/backends/webgl/backend_webgl.ts, line 281 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

instead of growing the already complex if statement, how about adding an earlier check for webworkers. Duplicating code is completely fine in this case. Something like:

if (typeof(WorkerGlobalScope) !== 'undefined) {
  // Make sure pixels is either OffscreenCanvas or PixelData.
}
// Leave the previous logic the same.

Also for readability can we make this a flag (& so we can centralize that check)? See flags.ts for where we define them

[dsmilkov]

Reviewable status: 0 of 1 approvals obtained (waiting on @dsmilkov, @nsthorat, and @WenheLI)

---

src/backends/webgl/backend_webgl.ts, line 281 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

Also for readability can we make this a flag (& so we can centralize that check)? See flags.ts for where we define them

Instead of a flag, let's do something like:

const isWebWorker = typeof(WorkerGlobalScope) !== 'undefined';
if (isWebWorker) {
 const isCanvas = typeof(OffscreenCanvas) !== 'undefined' && pixels instanceof OffsreenCanvas;
 const isPixelData = (pixels as PixelData).data instanceof Uint8Array;
 const isImageData = typeof(ImageData) !== 'undefined' && pixels instanceof ImageData;
 if (!isCanvas && !isPixelData && !isImageData) {
   // throw that we expect canvas, pixel-data, image-data.
 }
} else {
  // Use the existing browser-specific code below.
}

---

src/backends/webgl/backend_webgl.ts, line 309 at r1 (raw file):

          }
          this.fromPixels2DContext =
              document.createElement('canvas').getContext('2d');

call canvas_util.createCanvas()

[dsmilkov]

Reviewable status: 0 of 1 approvals obtained (waiting on @dsmilkov, @nsthorat, and @WenheLI)

---

src/backends/webgl/backend_webgl.ts, line 281 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

Instead of a flag, let's do something like:

const isWebWorker = typeof(WorkerGlobalScope) !== 'undefined';
if (isWebWorker) {
 const isCanvas = typeof(OffscreenCanvas) !== 'undefined' && pixels instanceof OffsreenCanvas;
 const isPixelData = (pixels as PixelData).data instanceof Uint8Array;
 const isImageData = typeof(ImageData) !== 'undefined' && pixels instanceof ImageData;
 if (!isCanvas && !isPixelData && !isImageData) {
   // throw that we expect canvas, pixel-data, image-data.
 }
} else {
  // Use the existing browser-specific code below.
}

I talked to the team and we can simplify this further (no need to check for webworker)

const isCanvas = (typeof(OffscreenCanvas) !== 'undefined' && pixels instanceof OffscreenCanvas) || (typeof(HTMLCanvasElement) !== 'undefined' && pixels instanceof HTMLCanvasElement);
const isPixelData = (pixels as PixelData).data instanceof Uint8Array;
const isImageData = typeof(ImageData) !== 'undefined' && pixels instanceof ImageData;
const isVideo = ... // similar
const isImage = ... // similar
if (!isCanvas && !isPixelData && !isImageData && !isVideo && !isImage) {
 // throw error that it must be one of these, regardless of the environment.
}

[WenheLI]

Right now, I think the fromPixels should be fine. Also, due to the mismatching of functionality in HTMLCanvas and OffscreenCanvas, I have to add ts-ignore. Hope that is fine.

Reviewable status: 0 of 1 approvals obtained (waiting on @dsmilkov and @nsthorat)

---

src/backends/cpu/backend_cpu.ts, line 151 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

Do the tests you added fail for this? If not, we should make sure the head tests do fail with this change.

I have changed them now it won't fail the previous tests.

[dsmilkov]

Hi Eric, this looks great. Just a quick comment about ts-ignore. I think you can remove a few of them if you explicitly cast. If that doesn't work, don't spend much time on it, we can merge as is. Thanks!

Reviewable status: 0 of 1 approvals obtained (waiting on @WenheLI)

---

src/backends/webgl/backend_webgl.ts, line 316 at r2 (raw file):

              'on the document object');
        }
        //@ts-ignore

why do you need ts-ignore in the createCanvas case?

---

src/backends/webgl/backend_webgl.ts, line 324 at r2 (raw file):

      this.fromPixels2DContext.drawImage(
          //@ts-ignore
          pixels, 0, 0, pixels.width, pixels.height);

instead of ts-ignore, try to cast pixels to HTMLVideoElement, i.e.
drawImage(pixels as HTMLVideoElement, ...) since you are already inside a statement where you know pixels is a video element.

Similarly try to do the same in the other instances.

[WenheLI]

Thanks Daneil, currently the all ts-ignore in this commit serves for the same reason that I state below.

Reviewable status: 0 of 1 approvals obtained (waiting on @dsmilkov and @WenheLI)

---

src/backends/webgl/backend_webgl.ts, line 316 at r2 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

why do you need ts-ignore in the createCanvas case?

This is because that the fromPixels2DContext could be either an HTMLCanvas or an OffscreenCanvas context. And the two kinds of context cannot cast directly, as the OffscreenCanvas context does not have the remove method.
I can do a forced cast but I think it may not be an elegant solution.

[WenheLI]

I got an error when doing the CI, scripts/test_snippets.ts:18:40 - error TS2307: Cannot find module '@tensorflow/tfjs-core/dist/scripts/test_snippets/util'.. This seems to be an internal problem of the test script? Do you have any clue of that?

Reviewable status: 0 of 1 approvals obtained (waiting on @dsmilkov and @WenheLI)

[WenheLI]

Hi Daniel, I think this PR might be good to merge now? After this pr, I will open another one for example and try to do some tests manually to ensure the webworker does not break anything.

Reviewable status: 0 of 1 approvals obtained (waiting on @dsmilkov and @WenheLI)

[nsthorat]

Hi Wenhe,

Can you send a link to the failing CI? I can't seem to find it :)

[dsmilkov]

I synced with master so the error is gone. Yes, this is ready to merge! Thank you!

[anshumanvenkatesh]

Which version of tfjs will have this change?

[dsmilkov]

Should be available in the latest version 1.2.3. Try it out and let us know otherwise. Note that you can't use htmlImageElement or video element in a webworker -- you have to pass ImageData or ImageData-like object {data: Uint8Array}.