Bug 110818 – [WebGL] Support for texImage2D of type HALF_FLOAT_OES with ArrayBufferView.

• Component: WebGL
• Status: RESOLVED FIXED

[Nayan Kumar K]

As per OES_texture_half_float specification (http://www.khronos.org/registry/webgl/extensions/OES_texture_half_float/), texImage2D and texSubImage2D entry points taking ArrayBufferView should extended to accept null with the pixel type HALF_FLOAT_OES.

[Nayan Kumar K]

Created attachment 190335 [details]
oes_texture_half_float

[Nayan Kumar K]

(In reply to comment #1)
> Created an attachment (id=190335) [details]
> oes_texture_half_float

Uploaded first patch towards implementing support for OES_texture_half_float in WebKit. With this patch, texImage2D and texSubImage2D will return failure when called with HALF_FLOAT_OES with non-null ArrayBufferView. Subsequent patches will add support for null ArrayBufferView.

I am unable to get the test run properly with DumpRenderTree. For some reason DumpRenderTree doesn't advertise GL_OES_texture_half_float as one of the supported extension, whereas chromium/content_shell advertises this string on the same platform!

[Kenneth Russell]

Comment on attachment 190335 [details]
oes_texture_half_float

View in context: https://bugs.webkit.org/attachment.cgi?id=190335&action=review

Thanks for continuing to push this patch forward. Before it is committed I think you should get it running in the Mac port of DRT. You probably need to check for the GL_ARB_half_float_pixel OpenGL extension, and if it's present, then have GraphicsContext3D claim that it supports GL_OES_texture_half_float. Also, the patch needs to be rebaselined and pass the EWS bots.

> Source/WebCore/html/canvas/OESTextureHalfFloat.cpp:2
> + * Copyright (C) 2013 Google Inc. All rights reserved.

You might want to change the attribution to Motorola.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:5140
> +        break;

In addition to these checks, please add validation to the texImage2D and texSubImage2D entry points making sure that you can't upload ImageData, HTMLImageElement, HTMLCanvasElement, or HTMLVideoElement as HALF_FLOAT textures yet. Also, add tests verifying this.

> LayoutTests/fast/canvas/webgl/oes-texture-half-float-expected.txt:1
> +CONSOLE MESSAGE: WebGL: INVALID_ENUM: texImage2D: invalid texture type

Is this console message coming from the Chromium port of DRT? If so, you should not check in this result into fast/canvas/webgl/ , but into platform/chromium/ instead.

> LayoutTests/fast/canvas/webgl/oes-texture-half-float.html:16
> +<!-- Shaders for testing half-floating-point textures -->

If you aren't going to add tests verifying rendering to HALF_FLOAT_OES textures yet, then remove these unused shaders and associated code for now.

> LayoutTests/fast/canvas/webgl/oes-texture-half-float.html:131
> +    gl.texImage2D(gl.TEXTURE_2D, 0, format, width, height, 0, format, gl.HALF_FLOAT_OES, data);

You should also verify that allocation with null data succeeds.

> LayoutTests/fast/canvas/webgl/oes-texture-half-float.html:138
> +    }

Please either add a test verifying that you can render to a HALF_FLOAT_OES texture, similar to the oes-texture-float test, or add a FIXME indicating that such a test is needed.

> LayoutTests/fast/canvas/webgl/oes-texture-half-float.html:139
> +}

Please add missing negative tests ensuring that you can't upload ImageData, HTMLImageElement, HTMLCanvasElement, or HTMLVideoElement into HALF_FLOAT_OES textures yet.

[Nayan Kumar K]

Created attachment 190856 [details]
Patch

[WebKit Review Bot]

Attachment 190856 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/canvas/webgl/constants.html', u'LayoutTests/fast/canvas/webgl/oes-texture-half-float-expected.txt', u'LayoutTests/fast/canvas/webgl/oes-texture-half-float-not-supported-expected.txt', u'LayoutTests/fast/canvas/webgl/oes-texture-half-float-not-supported.html', u'LayoutTests/fast/canvas/webgl/oes-texture-half-float.html', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/DerivedSources.make', u'Source/WebCore/DerivedSources.pri', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Target.pri', u'Source/WebCore/WebCore.gypi', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/bindings/js/JSWebGLRenderingContextCustom.cpp', u'Source/WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp', u'Source/WebCore/html/canvas/OESTextureHalfFloat.cpp', u'Source/WebCore/html/canvas/OESTextureHalfFloat.h', u'Source/WebCore/html/canvas/OESTextureHalfFloat.idl', u'Source/WebCore/html/canvas/WebGLExtension.h', u'Source/WebCore/html/canvas/WebGLRenderingContext.cpp', u'Source/WebCore/html/canvas/WebGLRenderingContext.h', u'Source/WebCore/html/canvas/WebGLRenderingContext.idl', u'Source/WebCore/platform/graphics/Extensions3D.h', u'Source/WebCore/platform/graphics/GraphicsContext3D.cpp', u'Source/WebCore/platform/graphics/GraphicsContext3D.h', u'Source/WebCore/platform/graphics/GraphicsTypes3D.h', u'Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp']" exit_code: 1
Source/WebCore/platform/graphics/GraphicsContext3D.h:254:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nayan Kumar K]

(In reply to comment #3)
> (From update of attachment 190335 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190335&action=review
> 
> Thanks for continuing to push this patch forward. Before it is committed I think you should get it running in the Mac port of DRT. You probably need to check for the GL_ARB_half_float_pixel OpenGL extension, and if it's present, then have GraphicsContext3D claim that it supports GL_OES_texture_half_float. Also, the patch needs to be rebaselined and pass the EWS bots.

Seem like availability of GL_ARB_texture_float (http://www.opengl.org/registry/specs/ARB/texture_float.txt) is enough to implement support for half float texture in dekstop gl. Specification defines the format to be used for half floating point texture. Also, Source/WebCore/platform/graphics/opengl/Extensions3DOpenGL.cpp already has the check to determine whether GL_ARB_texture_float is available before reporting GL_OES_texture_half_float as one of the supported extensions.

I have incorporated rest of the comment.

[Build Bot]

Comment on attachment 190856 [details]
Patch

Attachment 190856 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/16789372

New failing tests:
fast/canvas/webgl/oes-texture-half-float-not-supported.html

[Build Bot]

Comment on attachment 190856 [details]
Patch

Attachment 190856 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/16863138

New failing tests:
fast/canvas/webgl/oes-texture-half-float-not-supported.html

[Nayan Kumar K]

> New failing tests:
> fast/canvas/webgl/oes-texture-half-float-not-supported.html

Test passes in my local mac machine, but fails in mac-ews. Is there a way to get the test result from mac-ews?

[Kenneth Russell]

(In reply to comment #9)
> > New failing tests:
> > fast/canvas/webgl/oes-texture-half-float-not-supported.html
> 
> Test passes in my local mac machine, but fails in mac-ews. Is there a way to get the test result from mac-ews?

I think abarth knows how to get results off the EWS bots. I suggest trying to find him on IRC tomorrow.

[Kenneth Russell]

Comment on attachment 190856 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190856&action=review

Looking good. Please make one more code change and incorporate the layout test in the new location for the WebGL tests.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3769
> +    // Uploading ImageData to half floating point texture is not supported yet

Here and in the other places in this patch, add "FIXME:" at the start of the comment, and also file a bug about fixing this and link to that bug. Also, move this check down after the call to if (!validateSettableTexFormat(...)) in all these places. That way INVALID_ENUM will still be generated (by validateTexFuncFormatAndType) if the extension hasn't been enabled yet.

> LayoutTests/fast/canvas/webgl/oes-texture-half-float.html:1
> +<!DOCTYPE html>

The new location for these tests is LayoutTests/webgl/ and you can use the script LayoutTests/webgl/generate-webgl-tests.py script to pull in the script verbatim from the one you just contributed to Khronos. Unfortunately, the webgl/ folder isn't yet enabled on any port, so it might be worthwhile to add the test in this location temporarily. However, please also add it in the new location using that script. Also, in order to get this patch landed, you may want to mark the test as having text differences temporarily for the failing platforms (e.g. in LayoutTests/platform/mac/TestExpectations). Don't skip the test, just mark it as failing. Then you can get the results off the bots and see why it's failing.

[Nayan Kumar K]

Created attachment 191045 [details]
Patch

[Nayan Kumar K]

> The new location for these tests is LayoutTests/webgl/ and you can use the script LayoutTests/webgl/generate-webgl-tests.py script to pull in the script verbatim from the one you just contributed to Khronos. Unfortunately, the webgl/ folder isn't yet enabled on any port, so it might be worthwhile to add the test in this location temporarily. However, please also add it in the new location using that script. 
Currently generation-webgl-tests.py script generates test only for 1.0.2 version of webgl test suite. Test case submitted to Khronos was for 1.0.3. May be we should pull in all 1.0.3 version of test cases at once?

[Brandon Jones]

(In reply to comment #13)
> Currently generation-webgl-tests.py script generates test only for 1.0.2 version of webgl test suite. Test case submitted to Khronos was for 1.0.3. May be we should pull in all 1.0.3 version of test cases at once?

You can use a command line flag to specify where it pulls from. In my case I point it at my local repo's skd/tests. Run "generation-webgl-tests.py -w <path-to-source>"

[Kenneth Russell]

Comment on attachment 191045 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191045&action=review

Thanks for the updates. I suppose it's OK to add the webgl/ copies of the layout tests later. Please make the few corrections above before landing, and make sure it passes the EWS. r=me

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3816
> +    // Uploading ImageElement to half floating point texture is not supported yet

FIXME:

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3817
> +    // https://bugs.webkit.org/show_bug.cgi?id=111194

Is it really worth having all of these bugs for all of these different types? I suspect that once you have the core data packing support in place that supporting each of them will be a few lines of code each.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3844
> +    // Uploading HTMLCanvasElement to half floating point texture is not supported yet

FIXME:

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3906
> +    // Uploading HTMLVideoElement to half floating point texture is not supported yet

FIXME:

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4067
> +    // Uploading ImageData to half floating point texture is not supported yet

FIXME:

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4105
> +    // Uploading HTMLImageElement to half floating point texture is not supported yet

FIXME:

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4133
> +    // Uploading HTMLCanvasElement to half floating point texture is not supported yet

FIXME:

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:4157
> +    // Uploading HTMLVideoElement to half floating point texture is not supported yet

FIXME:

> Source/WebCore/html/canvas/WebGLRenderingContext.idl:1
> + /*

Undo this change please.

[Build Bot]

Comment on attachment 191045 [details]
Patch

Attachment 191045 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/16803325

New failing tests:
fast/css/hover-update.html

[Nayan Kumar K]

Created attachment 191068 [details]
Implement HALF_FLOAT_OES

[WebKit Review Bot]

Attachment 191068 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/canvas/webgl/constants.html', u'LayoutTests/fast/canvas/webgl/oes-texture-half-float-expected.txt', u'LayoutTests/fast/canvas/webgl/oes-texture-half-float-not-supported-expected.txt', u'LayoutTests/fast/canvas/webgl/oes-texture-half-float-not-supported.html', u'LayoutTests/fast/canvas/webgl/oes-texture-half-float.html', u'LayoutTests/platform/chromium/TestExpectations', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/DerivedSources.make', u'Source/WebCore/DerivedSources.pri', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Target.pri', u'Source/WebCore/WebCore.gypi', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/bindings/js/JSWebGLRenderingContextCustom.cpp', u'Source/WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp', u'Source/WebCore/html/canvas/OESTextureHalfFloat.cpp', u'Source/WebCore/html/canvas/OESTextureHalfFloat.h', u'Source/WebCore/html/canvas/OESTextureHalfFloat.idl', u'Source/WebCore/html/canvas/WebGLExtension.h', u'Source/WebCore/html/canvas/WebGLRenderingContext.cpp', u'Source/WebCore/html/canvas/WebGLRenderingContext.h', u'Source/WebCore/html/canvas/WebGLRenderingContext.idl', u'Source/WebCore/platform/graphics/Extensions3D.h', u'Source/WebCore/platform/graphics/GraphicsContext3D.cpp', u'Source/WebCore/platform/graphics/GraphicsContext3D.h', u'Source/WebCore/platform/graphics/GraphicsTypes3D.h', u'Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp']" exit_code: 1
Source/WebCore/platform/graphics/GraphicsContext3D.h:254:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenneth Russell]

Comment on attachment 191068 [details]
Implement HALF_FLOAT_OES

View in context: https://bugs.webkit.org/attachment.cgi?id=191068&action=review

Looks good again. One thing I missed earlier.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:270
> +        type = FLOAT;

I just realized -- this should almost certainly be "type = GL_HALF_FLOAT_ARB".

[Nayan Kumar K]

> 
> You can use a command line flag to specify where it pulls from. In my case I point it at my local repo's skd/tests. Run "generation-webgl-tests.py -w <path-to-source>"

generate-webgl-tests.py filters out the tests marked 1.0.3 or above. Below is a snippet of generate-webgl-tests.py

GLOBAL_OPTIONS = {
  # version use. Tests at or below this will be included.
  "version": "1.0.2",

If I change this version to 1.0.3, it generates newly included test. Since other tests of version 1.0.2 is not yet checked-in to LayoutTests, I feel we should checkin all 1.0.3 tests at once.

[Nayan Kumar K]

Created attachment 191073 [details]
Patch

[Kenneth Russell]

Comment on attachment 191073 [details]
Patch

FYI, for this bug at this point, you could just change the "Reviewed by NOBODY (OOPS!)." to "Reviewed by Kenneth Russell." by hand, and not set the r? bit.

[WebKit Review Bot]

Attachment 191073 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/canvas/webgl/constants.html', u'LayoutTests/fast/canvas/webgl/oes-texture-half-float-expected.txt', u'LayoutTests/fast/canvas/webgl/oes-texture-half-float-not-supported-expected.txt', u'LayoutTests/fast/canvas/webgl/oes-texture-half-float-not-supported.html', u'LayoutTests/fast/canvas/webgl/oes-texture-half-float.html', u'LayoutTests/platform/chromium/TestExpectations', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/DerivedSources.make', u'Source/WebCore/DerivedSources.pri', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Target.pri', u'Source/WebCore/WebCore.gypi', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/bindings/js/JSWebGLRenderingContextCustom.cpp', u'Source/WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp', u'Source/WebCore/html/canvas/OESTextureHalfFloat.cpp', u'Source/WebCore/html/canvas/OESTextureHalfFloat.h', u'Source/WebCore/html/canvas/OESTextureHalfFloat.idl', u'Source/WebCore/html/canvas/WebGLExtension.h', u'Source/WebCore/html/canvas/WebGLRenderingContext.cpp', u'Source/WebCore/html/canvas/WebGLRenderingContext.h', u'Source/WebCore/html/canvas/WebGLRenderingContext.idl', u'Source/WebCore/platform/graphics/Extensions3D.h', u'Source/WebCore/platform/graphics/GraphicsContext3D.cpp', u'Source/WebCore/platform/graphics/GraphicsContext3D.h', u'Source/WebCore/platform/graphics/GraphicsTypes3D.h', u'Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp']" exit_code: 1
Source/WebCore/platform/graphics/GraphicsContext3D.h:254:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 191073 [details]
Patch

Attachment 191073 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16884013

New failing tests:
platform/chromium/virtual/gpu/fast/canvas/webgl/oes-texture-half-float.html
platform/chromium/virtual/gpu/fast/canvas/webgl/oes-texture-half-float-not-supported.html

[Nayan Kumar K]

Created attachment 191097 [details]
Patch

[WebKit Review Bot]

Attachment 191097 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/canvas/webgl/constants.html', u'LayoutTests/fast/canvas/webgl/oes-texture-half-float-expected.txt', u'LayoutTests/fast/canvas/webgl/oes-texture-half-float-not-supported-expected.txt', u'LayoutTests/fast/canvas/webgl/oes-texture-half-float-not-supported.html', u'LayoutTests/fast/canvas/webgl/oes-texture-half-float.html', u'LayoutTests/platform/chromium/TestExpectations', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/DerivedSources.make', u'Source/WebCore/DerivedSources.pri', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Target.pri', u'Source/WebCore/WebCore.gypi', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/bindings/js/JSWebGLRenderingContextCustom.cpp', u'Source/WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp', u'Source/WebCore/html/canvas/OESTextureHalfFloat.cpp', u'Source/WebCore/html/canvas/OESTextureHalfFloat.h', u'Source/WebCore/html/canvas/OESTextureHalfFloat.idl', u'Source/WebCore/html/canvas/WebGLExtension.h', u'Source/WebCore/html/canvas/WebGLRenderingContext.cpp', u'Source/WebCore/html/canvas/WebGLRenderingContext.h', u'Source/WebCore/html/canvas/WebGLRenderingContext.idl', u'Source/WebCore/platform/graphics/Extensions3D.h', u'Source/WebCore/platform/graphics/GraphicsContext3D.cpp', u'Source/WebCore/platform/graphics/GraphicsContext3D.h', u'Source/WebCore/platform/graphics/GraphicsTypes3D.h', u'Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp']" exit_code: 1
Source/WebCore/platform/graphics/GraphicsContext3D.h:254:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 191097 [details]
Patch

Clearing flags on attachment: 191097

Committed r144535: <http://trac.webkit.org/changeset/144535>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

Why was this feature never announced on webkit-dev? FWIW, IDL with an empty definition makes very little senes.

[Ryosuke Niwa]

Note that this patch acausezd a build fialure on Windows: https://bugs.webkit.org/show_bug.cgi?id=111267

[Kenneth Russell]

(In reply to comment #29)
> Why was this feature never announced on webkit-dev? FWIW, IDL with an empty definition makes very little senes.

Sorry about that. WebGL extensions are so narrowly focused that I never considered that their addition needed to be announced on webkit-dev. Do they need to be in the future?

WebGL extensions are specified as instances of interfaces, and are defined so that the same object is guaranteed to be returned across multiple calls to WebGLRenderingContext.getExtension. For this reason they have been defined with IDL files, even if those IDL files are empty, so that we can return objects of distinct type for different extensions. Do you have a suggestion on how to implement these semantics more simply?

[Nayan Kumar K]

(In reply to comment #30)
> Note that this patch acausezd a build fialure on Windows: https://bugs.webkit.org/show_bug.cgi?id=111267

Sorry for breaking windows build. I missed this failure as it was not caught by EWS.