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

Add support for using webgl in a worker via offscreen canvas #1221

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@bkazi

bkazi commented Aug 12, 2018

Description

tensorflow/tfjs#102

Offscreen Canvas is currently only shipping in Chrome so this could break for users who try to use WebGL backend in a worker in unsupported browsers.
The same issue applies to tests hence the support check in the test

Add feature to check if in worker
Add types for offscreen canvas
Add test for canvas when running in worker


For repository owners only:

Please remember to apply all applicable tags to your pull request.
Tags: FEATURE, BREAKING, BUG, PERF, DEV, DOC, SECURITY

For more info see: https://github.com/tensorflow/tfjs/blob/master/DEVELOPMENT.md


This change is Reviewable

Add support for using webgl in a worker via offscreen canvas
Add feature to check if in worker
Add types for offscreen canvas
Add test for canvas when running in worker
@googlebot

This comment has been minimized.

Show comment
Hide comment
@googlebot

googlebot Aug 12, 2018

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

googlebot commented Aug 12, 2018

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers
@bkazi

This comment has been minimized.

Show comment
Hide comment
@bkazi

bkazi Aug 12, 2018

I'm trying to port the mnist-core example to run in a worker, the plumbing between the worker is the tricky bit, but the main functionality seems to work fine
Can't confirm until it completely works
fingers crossed

bkazi commented Aug 12, 2018

I'm trying to port the mnist-core example to run in a worker, the plumbing between the worker is the tricky bit, but the main functionality seems to work fine
Can't confirm until it completely works
fingers crossed

@googlebot

This comment has been minimized.

Show comment
Hide comment
@googlebot

googlebot Aug 14, 2018

CLAs look good, thanks!

googlebot commented Aug 14, 2018

CLAs look good, thanks!

@bkazi

This comment has been minimized.

Show comment
Hide comment
@bkazi

bkazi Aug 14, 2018

I have a few issues that I would like some discussion around

  • There is a lot of duplication of the IS_WORKER feature check in the previous commit since it is needed for multiple checks that rely on the canvas to figure out which kind of canvas to use. My idea to improve this was to abstract away the creation of canvases, and default to using OffscreenCanvas instead of the full canvas element, falling back to the canvas element on unsupported browsers. Does that seem too future facing for now?
  • OffscreenCanvas doesn't have TypeScript types since it hasn't shipped in many browsers so this means creating the types for the time being. As a quick fix I have created it such that the type is the same as that of HTMLCanvasElement since the API is the same. Do we continue with that or define a separate type for OffscreenCanvas and extend the class property to return type HTMLCanvasElement | OffscreenCanvas? I think the second one is the better option in the long run but just checking
  • the NUM_BYTES_BEFORE_PAGING is currently based on a screen size heuristic which isn't available in workers, is there a sensible default for this?

bkazi commented Aug 14, 2018

I have a few issues that I would like some discussion around

  • There is a lot of duplication of the IS_WORKER feature check in the previous commit since it is needed for multiple checks that rely on the canvas to figure out which kind of canvas to use. My idea to improve this was to abstract away the creation of canvases, and default to using OffscreenCanvas instead of the full canvas element, falling back to the canvas element on unsupported browsers. Does that seem too future facing for now?
  • OffscreenCanvas doesn't have TypeScript types since it hasn't shipped in many browsers so this means creating the types for the time being. As a quick fix I have created it such that the type is the same as that of HTMLCanvasElement since the API is the same. Do we continue with that or define a separate type for OffscreenCanvas and extend the class property to return type HTMLCanvasElement | OffscreenCanvas? I think the second one is the better option in the long run but just checking
  • the NUM_BYTES_BEFORE_PAGING is currently based on a screen size heuristic which isn't available in workers, is there a sensible default for this?
Add files option to ts-node
Fixes failing tests on node 10
TypeStrong/ts-node#615

@bkazi bkazi changed the title from [WIP] Add support for using webgl in a worker via offscreen canvas to Add support for using webgl in a worker via offscreen canvas Aug 22, 2018

@tafsiri tafsiri self-requested a review Oct 5, 2018

@tafsiri tafsiri self-assigned this Oct 5, 2018

@tafsiri

This comment has been minimized.

Show comment
Hide comment
@tafsiri

tafsiri Oct 5, 2018

Member

@bkazi Sorry for the long delay in reviewing this, its the kind of change that requires a fair amount of background research for us to consider maintainability over time and we just didn't have the bandwidth recently. I will be taking a closer look at this though. One question I had is whether you have a proof-of-concept application using this functionality. You mentioned the mnist-core example. Is that available on github somewhere?

Member

tafsiri commented Oct 5, 2018

@bkazi Sorry for the long delay in reviewing this, its the kind of change that requires a fair amount of background research for us to consider maintainability over time and we just didn't have the bandwidth recently. I will be taking a closer look at this though. One question I had is whether you have a proof-of-concept application using this functionality. You mentioned the mnist-core example. Is that available on github somewhere?

@bkazi

This comment has been minimized.

Show comment
Hide comment
@bkazi

bkazi Oct 6, 2018

@tafsiri cheers, it is a fairly large change so any feedback is appreciated
I have a PR for minst-core using web workers at tensorflow/tfjs-examples#129 (or master of bkazi/tfjs-examples)

bkazi commented Oct 6, 2018

@tafsiri cheers, it is a fairly large change so any feedback is appreciated
I have a PR for minst-core using web workers at tensorflow/tfjs-examples#129 (or master of bkazi/tfjs-examples)

@tafsiri

This comment has been minimized.

Show comment
Hide comment
@tafsiri

tafsiri Oct 8, 2018

Member

@bkazi Thanks, so i took a look at this and I wonder if a potentially simpler approach would be to feature test for OffscreenCanvas (rather than being in a webworker) and always use it when available. That way we would not need the IS_WORKER code path.

Something along the lines of

if (ENV.get('IS_BROWSER')) {
  if (window && ('OffscreenCanvas' in window)) {
    // console.log('USING OFFSCREEN CANVAS');
    this.canvas = new OffscreenCanvas(1, 1);
  } else {
    this.canvas = document.createElement('canvas');
  }
}

I tried this and the tests passed, there is another canvas element created that would also need to be replaced by an optional OffScreenCanvas as well.

But I think the advantage of this is that we have fewer code paths and can more easily test this code path without having it execute in an actual web worker in our tests.

There would still remain things to figure out like NUM_BYTES_BEFORE_PAGING, but we can start by setting that to infinity if there is no window object as we develop some helpful heuristics for workers.

How does that sound to you? I'll take a look at your tfjs-example today.

Member

tafsiri commented Oct 8, 2018

@bkazi Thanks, so i took a look at this and I wonder if a potentially simpler approach would be to feature test for OffscreenCanvas (rather than being in a webworker) and always use it when available. That way we would not need the IS_WORKER code path.

Something along the lines of

if (ENV.get('IS_BROWSER')) {
  if (window && ('OffscreenCanvas' in window)) {
    // console.log('USING OFFSCREEN CANVAS');
    this.canvas = new OffscreenCanvas(1, 1);
  } else {
    this.canvas = document.createElement('canvas');
  }
}

I tried this and the tests passed, there is another canvas element created that would also need to be replaced by an optional OffScreenCanvas as well.

But I think the advantage of this is that we have fewer code paths and can more easily test this code path without having it execute in an actual web worker in our tests.

There would still remain things to figure out like NUM_BYTES_BEFORE_PAGING, but we can start by setting that to infinity if there is no window object as we develop some helpful heuristics for workers.

How does that sound to you? I'll take a look at your tfjs-example today.

@tafsiri

This comment has been minimized.

Show comment
Hide comment
@tafsiri

tafsiri Oct 8, 2018

Member

As an addendum to the comment above I also did some perf testing and there seemd to be no perf hit from using OffscreenCanvas compared to regular canvas.

Member

tafsiri commented Oct 8, 2018

As an addendum to the comment above I also did some perf testing and there seemd to be no perf hit from using OffscreenCanvas compared to regular canvas.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment