add experimental webgl support for RN platform

[tafsiri]

This PR adds experimental support for the WebGL backend on react native platform. It is experimental because it mocks out extension testing in the platform itself. This is needed until the upstream GL context supports the extension checking.

Additionally, it makes core check for EXT_color_buffer_half_float if EXT_color_buffer_float is not present.

---

This change is 

[tafsiri]

Reviewable status: 0 of 1 approvals obtained

---

tfjs-react-native/src/platform_react_native.ts, line 160 at r1 (raw file):

      // We always use WebGL2 in RN. Also we need to set this here
      // because the flag check in GPGPUContext does not use the passed

Is this something we want to change about flag evaluation for this flag? One possible fix would be that the flag evaluation would need to be able to take a parameter (in this case the passed in glContext) do do the extension check on. Currently evaluating WEBGL_VERSION will always try and make a canvas and its own context.

[nsthorat]

Reviewed 12 of 12 files at r1, 2 of 2 files at r2.
Reviewable status: 0 of 1 approvals obtained (waiting on @dsmilkov, @nkreeger, @nsthorat, and @tafsiri)

---

src/backends/webgl/gpgpu_context.ts, line 73 at r2 (raw file):

      }
    } else {
      if (webgl_util.hasExtension(this.gl, 'EXT_color_buffer_float')) {

pull these extension names to a constant since you use them twice

---

tfjs-react-native/package.json, line 7 at r2 (raw file):

  "main": "dist/src/index.js",
  "types": "dist/src/index.d.ts",
  "jsnext:main": "dist/src/tf-react-native.esm.js",

we've typically been avoiding a src/ in a dist/, can you unpack src => dist without the directory like we do for the other packages?

---

tfjs-react-native/src/bundle_resource_io.ts, line 107 at r2 (raw file):

export function bundleResourceIO(
    modelJson: io.ModelJSON, modelWeightsId: number): io.IOHandler {
  if (typeof modelWeightsId !== 'object') {

should there be a unit test for this?

---

tfjs-react-native/src/platform_react_native.ts, line 160 at r1 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

Is this something we want to change about flag evaluation for this flag? One possible fix would be that the flag evaluation would need to be able to take a parameter (in this case the passed in glContext) do do the extension check on. Currently evaluating WEBGL_VERSION will always try and make a canvas and its own context.

i think if you do this you will get a GL 2:

https://github.com/tensorflow/tfjs-wechat/blob/master/src/plugin/utils/wechat_platform.ts#L106

then you dont need the stuff below and you can use the regular webgl backend

---

tfjs-react-native/src/platform_react_native.ts, line 167 at r2 (raw file):

      const backend = new tf.webgl.MathBackendWebGL(context);
      backend.read = async (dataId) => {
        return backend.readSync(dataId);

can we avoid doing it like this (overriding the function here)?

why do you need read() to be sync? ios is sync download and we do that with feature testing flags

[tafsiri]

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

---

tfjs-react-native/package.json, line 7 at r2 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

we've typically been avoiding a src/ in a dist/, can you unpack src => dist without the directory like we do for the other packages?

I can't without putting the test utils in src/ (ts rootDir issues), which I had been avoiding (preferring a top level test folder next to src). I can put test in src if we strongly prefer dropping the dist prefix from main.

---

tfjs-react-native/src/bundle_resource_io.ts, line 107 at r2 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

should there be a unit test for this?

Done.

---

tfjs-react-native/src/platform_react_native.ts, line 160 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

i think if you do this you will get a GL 2:

https://github.com/tensorflow/tfjs-wechat/blob/master/src/plugin/utils/wechat_platform.ts#L106

then you dont need the stuff below and you can use the regular webgl backend

Done.

Why do we allow passing in a glContext if we have this setWebGLContext method?

---

tfjs-react-native/src/platform_react_native.ts, line 167 at r2 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

can we avoid doing it like this (overriding the function here)?

why do you need read() to be sync? ios is sync download and we do that with feature testing flags

Fixed. Not needed.

[nkreeger]

Might be good to add a comment here about why this case is handled.

[nsthorat]

Reviewed 5 of 5 files at r3.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @dsmilkov, @nkreeger, and @tafsiri)

---

src/backends/webgl/glsl_version.ts, line 59 at r3 (raw file):

    `;
    defineSpecialInf = `
      const float INFINITY = uintBitsToFloat(uint(0x7f800000));

why can you remove this?

---

tfjs-react-native/package.json, line 7 at r2 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

I can't without putting the test utils in src/ (ts rootDir issues), which I had been avoiding (preferring a top level test folder next to src). I can put test in src if we strongly prefer dropping the dist prefix from main.

lets do that

---

tfjs-react-native/src/platform_react_native.ts, line 160 at r1 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

Done.

Why do we allow passing in a glContext if we have this setWebGLContext method?

you're right, we shouldn't

---

tfjs-react-native/src/platform_react_native.ts, line 1 at r3 (raw file):

/**

this file is a misnomer since it's both a platform and a backend

---

tfjs-react-native/src/platform_react_native.ts, line 147 at r3 (raw file):

      //@ts-ignore
      const getExt = glContext.getExtension.bind(glContext);
      // tslint:disable-next-line:only-arrow-functions

why this and not just make an arrow function?

---

tfjs-react-native/src/platform_react_native.ts, line 150 at r3 (raw file):

Previously, nkreeger (Nick Kreeger) wrote…

Might be good to add a comment here about why this case is handled.

+1 / is there a way to remove this entirely? why do you need to do this/

---

tfjs-react-native/src/platform_react_native.ts, line 160 at r3 (raw file):

          // WEBGL_DOWNLOAD_FLOAT_ENABLED is true for iOS via expo but not in
          // Safari so we override the flag here
          tf.ENV.set('WEBGL_DOWNLOAD_FLOAT_ENABLED', true);

this doesn't seem right to me -- why does this not evaluate to true automatically?

---

tfjs-react-native/src/platform_react_native.ts, line 174 at r3 (raw file):

      const backend = new tf.webgl.MathBackendWebGL(context);
      return backend;
    }, 1);

pull 1 to a named variable for readability here

[nkreeger]

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @dsmilkov, @nkreeger, and @tafsiri)

---

tfjs-react-native/src/platform_react_native.ts, line 160 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

you're right, we shouldn't

I was just looking at this too - the WEBGL_VERSION we can try the doing something similar to this logic:

    let f32Ext = gl.getExtension('EXT_color_buffer_float');
    let f16Ext = gl.getExtension('EXT_color_buffer_half_float');
    if (f32Ext || f16Ext) {
      this.isWebGL2_ = true;
    } else {
      // Both required WebGL2 extensions are not supported, fallback to WebGL1
      // extensions:
      this.isWebGL2_ = false;
      f32Ext = gl.getExtension('OES_texture_float');
      f16Ext = gl.getExtension('OES_texture_half_float');
    }

    this.float32Support_ = f32Ext !== null;
    this.float16Support_ = f16Ext !== null;

    // TODO - add fallbacks for iOS and other non-compliant WebGL platforms

    // If neither f32 or f16 textures are supported, throw an exception because
    // the library will not run on this device. Determine WebGL version by
    if (!this.float32Support_ || !this.float16Support_) {
      throw new Error(
          'This device is not compatible with running TensorFlow.js');
    }

That is from the headless webgl doc. I can start a WIP PR tonight

[tafsiri]

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @dsmilkov, @nkreeger, and @nsthorat)

---

src/backends/webgl/glsl_version.ts, line 59 at r3 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

why can you remove this?

We never used the definition of INFINITY anywhere in the shaders. In the WebGL1 path INFINITY is used to override the definition of isInf (the function we do use), but we don't override it in WebGL 2.

---

src/backends/webgl/gpgpu_context.ts, line 73 at r2 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

pull these extension names to a constant since you use them twice

Done

---

tfjs-react-native/package.json, line 7 at r2 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

lets do that

Done

---

tfjs-react-native/src/platform_react_native.ts, line 1 at r3 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

this file is a misnomer since it's both a platform and a backend

discussed offline. going to keep this name for now.

---

tfjs-react-native/src/platform_react_native.ts, line 147 at r3 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

why this and not just make an arrow function?

Just avoiding binding this since at assignment I'm going to explicitly bind this and wanted to make it clearly stand out (it also makes using it fail more clearly if the bind in the line below is not included. It's not necessary, so I can remove it if you prefer.

---

tfjs-react-native/src/platform_react_native.ts, line 150 at r3 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

+1 / is there a way to remove this entirely? why do you need to do this/

Added a comment here and expanded the comment at the start of this section.

Here is the new comment at the start of this whole section.

Mock extension support for EXT_color_buffer_float and
EXT_color_buffer_half_float on the expo-gl context object.
In react native we do not have to get a handle to the extension 
in order to use the functionality of that extension on the device.

This code block makes iOS and Android devices pass the extension checks
used in core. After those are done core will actually test whether
we can render/download float or half float textures.

We can remove this block once we upstream checking for these
extensions in expo.

---

tfjs-react-native/src/platform_react_native.ts, line 160 at r3 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

this doesn't seem right to me -- why does this not evaluate to true automatically?

Fixed in recent commits with updates to our flag evaluation

---

tfjs-react-native/src/platform_react_native.ts, line 174 at r3 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

pull 1 to a named variable for readability here

Done

[dsmilkov]

Nice work. Just a few comments -- nothing major

Reviewed 3 of 12 files at r1, 2 of 2 files at r2, 2 of 5 files at r3, 22 of 22 files at r4.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @dsmilkov, @nkreeger, and @tafsiri)

---

src/backends/webgl/glsl_version.ts, line 59 at r3 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

We never used the definition of INFINITY anywhere in the shaders. In the WebGL1 path INFINITY is used to override the definition of isInf (the function we do use), but we don't override it in WebGL 2.

looks good! Just added a comment on top that webgl doesn't need a special infinity constant.

---

src/backends/webgl/gpgpu_context.ts, line 76 at r4 (raw file):

      const COLOR_BUFFER_HALF_FLOAT = 'EXT_color_buffer_half_float';
      if (webgl_util.hasExtension(this.gl, COLOR_BUFFER_FLOAT)) {
        this.colorBufferFloatExtension = webgl_util.getExtensionOrThrow(

since you already check for hasExtension, call getExtension instead of getExtensionOrThrow

---

src/backends/webgl/webgl_util.ts, line 584 at r4 (raw file):

 * Note that for performance reasons we use binding a texture to a framebuffer
 * as a proxy for ability to download float values later using readPixels. The
 * texture params of this texture will not match those in readPxiels exactly

typo: s/readPxiels/readPixels

---

src/io/types.ts, line 464 at r4 (raw file):

   * Is this request for a binary file (as opposed to a json file)
   */
  isBinary?: true;

isBinary: boolean

---

tfjs-react-native/src/bundle_resource_io_test.ts, line 121 at r4 (raw file):

    const resourceId = 1;
    //@ts-ignore
    expect(() => bundleResourceIO(modelJson, resourceId))

modelJson as {} (if that doesn't work, modelJson as unknown as {}). Then you can remove ts-ignore and the context is in the cast.

---

tfjs-react-native/src/platform_react_native.ts, line 21 at r4 (raw file):

import {Platform} from '@tensorflow/tfjs-core';
import {Buffer} from 'buffer';
import {GLView} from 'expo-gl';

if the import of expo-gl doesn't fail at runtime in the browser, it would be better to split browser-friendly tests with a BROWSER_ENVS predicate, and later guard react-native tests with a REACT_NATIVE_ENVS predicate.

---

tfjs-react-native/src/platform_react_native.ts, line 98 at r4 (raw file):

    if (options != null && options.isBinary) {
      xhr.responseType = 'arraybuffer';

still add a comment that fetching of binary files in react native won't work without this header.

---

tfjs-react-native/src/test_utils/gl_view_mock.ts, line 18 at r4 (raw file):

 */

// Mock gl-view to export nothing as we don't test it in unit tests.

maybe clarify that these are unit tests in the browser, where importing a native library won't work

[tafsiri]

Reviewable status: complete! 1 of 1 approvals obtained (waiting on @dsmilkov and @nkreeger)

---

src/backends/webgl/glsl_version.ts, line 59 at r3 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

looks good! Just added a comment on top that webgl doesn't need a special infinity constant.

Done (added a comment)

---

src/backends/webgl/gpgpu_context.ts, line 76 at r4 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

since you already check for hasExtension, call getExtension instead of getExtensionOrThrow

Done

---

src/backends/webgl/webgl_util.ts, line 584 at r4 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

typo: s/readPxiels/readPixels

Done

---

src/io/types.ts, line 464 at r4 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

isBinary: boolean

Done

---

tfjs-react-native/src/bundle_resource_io_test.ts, line 121 at r4 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

modelJson as {} (if that doesn't work, modelJson as unknown as {}). Then you can remove ts-ignore and the context is in the cast.

Done

---

tfjs-react-native/src/platform_react_native.ts, line 21 at r4 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

if the import of expo-gl doesn't fail at runtime in the browser, it would be better to split browser-friendly tests with a BROWSER_ENVS predicate, and later guard react-native tests with a REACT_NATIVE_ENVS predicate.

I don't think this will work right now the way we have it set up. Our BROWSER_ENVS predicate checks that the name of the backend that has been set is ' browser'. In the tests in this package, the platform is set to react-native, but I am using karma to run them in a browser (with mocks).

I think I should defer using the describeWithFlags and predicates until I have an example of something that shouldn't run in the browser.

---

tfjs-react-native/src/platform_react_native.ts, line 98 at r4 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

still add a comment that fetching of binary files in react native won't work without this header.

Done

---

tfjs-react-native/src/test_utils/gl_view_mock.ts, line 18 at r4 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

maybe clarify that these are unit tests in the browser, where importing a native library won't work

Done, here and elsewhere

[nsthorat]

Reviewed 6 of 22 files at r4, 5 of 9 files at r5.
Reviewable status: complete! 2 of 1 approvals obtained (waiting on @dsmilkov, @nkreeger, and @tafsiri)

---

tfjs-react-native/src/platform_react_native.ts, line 147 at r3 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

Just avoiding binding this since at assignment I'm going to explicitly bind this and wanted to make it clearly stand out (it also makes using it fail more clearly if the bind in the line below is not included. It's not necessary, so I can remove it if you prefer.

up to you, usually we do arrow functions

---

tfjs-react-native/src/platform_react_native.ts, line 59 at r5 (raw file):

 * Makes an HTTP request.
 * @param path The URL path to make a request to
 * @param init The request init. See init here:

@param options

---

tfjs-react-native/src/platform_react_native.ts, line 123 at r5 (raw file):

  async fetch(
      path: string, init?: RequestInit, options?: tf.io.RequestDetails) {
    return fetch(path, init, options)

;

---

tfjs-react-native/src/platform_react_native.ts, line 198 at r5 (raw file):

      const context = new tf.webgl.GPGPUContext();
      const backend = new tf.webgl.MathBackendWebGL(context);
      // Manually make 'read' synchronous. glContext has a defined gl.fenceSync

can you do this by overriding things on the glContext instead of overriding this method?

[tafsiri]

Reviewable status: complete! 2 of 1 approvals obtained (waiting on @dsmilkov, @nkreeger, and @nsthorat)

---

tfjs-react-native/src/platform_react_native.ts, line 147 at r3 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

up to you, usually we do arrow functions

I think i'll leave it this way to help it stand out more.

---

tfjs-react-native/src/platform_react_native.ts, line 59 at r5 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

@param options

Done

---

tfjs-react-native/src/platform_react_native.ts, line 123 at r5 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

;

Done.

Fixed, here and elsewhere

---

tfjs-react-native/src/platform_react_native.ts, line 198 at r5 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

can you do this by overriding things on the glContext instead of overriding this method?

I don't think so, I don't think there is a way to shim fenceSync to just make it effectively implement read.