Skip to content
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

[core] Do not fail if half float extension not found and we are not forcing half float rendering. #2410

Merged
merged 11 commits into from Nov 19, 2019

Conversation

annxingyuan
Copy link
Collaborator

@annxingyuan annxingyuan commented Nov 19, 2019

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


This change is Reviewable

@annxingyuan annxingyuan changed the title WIP [core] Do not fail if half float extension not found and we are not forcing half float rendering. [core] Do not fail if half float extension not found and we are not forcing half float rendering. Nov 19, 2019
@annxingyuan annxingyuan self-assigned this Nov 19, 2019
Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r1.
Reviewable status: 0 of 1 approvals obtained (waiting on @annxingyuan, @dsmilkov, and @pyu10055)


tfjs-core/src/backends/webgl/gpgpu_context.ts, line 70 at r1 (raw file):

          webgl_util.getExtensionOrThrow(this.gl, this.debug, TEXTURE_FLOAT);
      if (webgl_util.hasExtension(this.gl, TEXTURE_HALF_FLOAT)) {
        this.textureHalfFloatExtension = webgl_util.getExtensionOrThrow(

are there tests to ensure if the texturehalfFloatExtension is null, things still works?

Copy link
Collaborator Author

@annxingyuan annxingyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


tfjs-core/src/backends/webgl/gpgpu_context.ts, line 70 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

are there tests to ensure if the texturehalfFloatExtension is null, things still works?

Half float textures are only used if we force F16 textures, hence the check in the else-if statement.

Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


tfjs-core/src/backends/webgl/gpgpu_context.ts, line 70 at r1 (raw file):

Previously, annxingyuan (Ann Yuan) wrote…

Half float textures are only used if we force F16 textures, hence the check in the else-if statement.

In the tex_util.ts:218, it is checking the textureHalfFloatExtension != null, but gpgpu_context seems to default it to {}, can you verify if the value set there is correct?

Copy link
Collaborator Author

@annxingyuan annxingyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


tfjs-core/src/backends/webgl/gpgpu_context.ts, line 70 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

In the tex_util.ts:218, it is checking the textureHalfFloatExtension != null, but gpgpu_context seems to default it to {}, can you verify if the value set there is correct?

gpgpu_context doesn't actually set textureHalfFloatExtension to {}, so if the extension is unavailable, textureHalfFloatExtension is undefined at tex_util.ts:218. In this case, textureTypeHalfFloat is set to null.

Copy link
Contributor

@dsmilkov dsmilkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


tfjs-core/src/backends/webgl/gpgpu_context.ts, line 80 at r1 (raw file):

      this.colorBufferFloatExtension = this.gl.getExtension(COLOR_BUFFER_FLOAT);
      this.colorBufferHalfFloatExtension =
          this.gl.getExtension(COLOR_BUFFER_HALF_FLOAT);

should you do a similar check for COLOB_BUFFER_HALF_FLOAT, that is error only in the case when float16 is forced?

Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


tfjs-core/src/backends/webgl/gpgpu_context.ts, line 70 at r1 (raw file):

Previously, annxingyuan (Ann Yuan) wrote…

gpgpu_context doesn't actually set textureHalfFloatExtension to {}, so if the extension is unavailable, textureHalfFloatExtension is undefined at tex_util.ts:218. In this case, textureTypeHalfFloat is set to null.

gpgpu_context.ts:38 is defaulting all of extensions to {}, did they get reset somewhere else?

Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


tfjs-core/src/backends/webgl/gpgpu_context.ts, line 38 at r1 (raw file):

  gl: WebGLRenderingContext;
  textureFloatExtension: {};
  textureHalfFloatExtension: {};

FYI: here are the defaults.

Copy link
Collaborator Author

@annxingyuan annxingyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


tfjs-core/src/backends/webgl/gpgpu_context.ts, line 70 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

gpgpu_context.ts:38 is defaulting all of extensions to {}, did they get reset somewhere else?

I think that's not setting defaults - it's just a type annotation (hence the colon).


tfjs-core/src/backends/webgl/gpgpu_context.ts, line 80 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

should you do a similar check for COLOB_BUFFER_HALF_FLOAT, that is error only in the case when float16 is forced?

Done

Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @annxingyuan)


tfjs-core/src/backends/webgl/gpgpu_context.ts, line 70 at r1 (raw file):

Previously, annxingyuan (Ann Yuan) wrote…

I think that's not setting defaults - it's just a type annotation (hence the colon).

you are right, need some glasses today.

Copy link
Contributor

@dsmilkov dsmilkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r2.
Reviewable status: :shipit: complete! 2 of 1 approvals obtained (waiting on @annxingyuan)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants