Bug 110936 – [WebGL] Support for texImage2D of type HALF_FLOAT_OES

• 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 ImageData should be extended to accept the pixel type HALF_FLOAT_OES. Subsequent patch(es) will add support for HTMLImageElement, HTMLCanvasElement and HTMLVideoElement.

[Nayan Kumar K]

Created attachment 190430 [details]
WIP patch

Attached is the WIP patch to implement support for half floating point textures with ImageData. Basic idea is to convert the unsigned int data to float data (using unpack routine) and then convert it to half float point. 

Also test case is added to WebGL conformance test  https://github.com/nayankk/WebGL/commit/5479e4d707fed3c6ef545ad8e7377239487af785

[Nayan Kumar K]

*** Bug 111194 has been marked as a duplicate of this bug. ***

[Nayan Kumar K]

*** Bug 111196 has been marked as a duplicate of this bug. ***

[Nayan Kumar K]

*** Bug 111197 has been marked as a duplicate of this bug. ***

[Kenneth Russell]

Comment on attachment 190430 [details]
WIP patch

@nayankk: are you planning to move this patch forward?

[Nayan Kumar K]

(In reply to comment #5)
> (From update of attachment 190430 [details])
> @nayankk: are you planning to move this patch forward?

Sorry for the delay, I will be submitting a patch for review soon.

[Nayan Kumar K]

Created attachment 195407 [details]
Patch

[Nayan Kumar K]

Created attachment 195479 [details]
Patch

[Kenneth Russell]

Comment on attachment 195479 [details]
Patch

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

Given today's announcement regarding the Chromium project, I'm not sure I should review this patch; dino should. However, I'm positive that this patch isn't correct as written, so setting r- to make that clear. (It's good work so far, but needs to be made robust. Negative tests are also necessary given the incomplete state.)

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:-3753
> -

The rest of this patch only has incomplete support for HALF_FLOAT_OES packing and unpacking. If you are going to remove this check, then you need to modify validateTexFuncFormatAndType so that it only supports the combination of RGBA/HALF_FLOAT_OES when dealing with non-ArrayBufferView sources, and generates INVALID_OPERATION otherwise.

> Source/WebCore/platform/graphics/GraphicsContext3D.cpp:141
> +            ASSERT_NOT_REACHED();

This is missing support for the other formats like RGB, ALPHA, LUMINANCE, and LUMINANCE_ALPHA.

> Source/WebCore/platform/graphics/GraphicsContext3D.cpp:1083
> +}

This is missing support for the other formats like RGB, ALPHA, LUMINANCE, and LUMINANCE_ALPHA.

[Dean Jackson]

<rdar://problem/15563029>

[Dean Jackson]

Created attachment 218374 [details]
Patch

[Tim Horton]

Comment on attachment 218374 [details]
Patch

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

> Source/WebCore/platform/graphics/GraphicsContext3D.cpp:1450
> +    static const int Value = (IsFloatFormat<Format>::Value || IsHalfFloatFormat<Format>::Value) ? GraphicsContext3D::DataFormatRGBA32F : GraphicsContext3D::DataFormatRGBA8;

Is this right? HalfFloatFormats should use DataFormatRGBA32F, not DataFormatRGBA16f? (I have no idea what this is!)

[Brent Fulgham]

Comment on attachment 218374 [details]
Patch

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

> Source/WebCore/platform/graphics/GraphicsContext3D.cpp:1220
> +        destination[3] = convertFloatToHalfFloat(source[3]);

This pattern is repeated a bunch of times. I wonder if this could be a template, too?

>> Source/WebCore/platform/graphics/GraphicsContext3D.cpp:1450
>> +    static const int Value = (IsFloatFormat<Format>::Value || IsHalfFloatFormat<Format>::Value) ? GraphicsContext3D::DataFormatRGBA32F : GraphicsContext3D::DataFormatRGBA8;
> 
> Is this right? HalfFloatFormats should use DataFormatRGBA32F, not DataFormatRGBA16f? (I have no idea what this is!)

Might need a three-step check!

    static const int Value = IsFloatFormat<Format>::Value? GraphicsContext3D::DataFormatRGBA32F : (IsHalfFloatFormat<Format>::Value ? GraphicsContext3D::DataFormatRGBA16F : GraphicsContext3D::DataFormatRGBA8);

> LayoutTests/fast/canvas/webgl/oes-texture-half-float-expected.txt:25
> +PASS getError was expected value: INVALID_ENUM : Half floating point texture must be disallowed if OES_texture_half_float isn't enabled

Good catch!

> LayoutTests/fast/canvas/webgl/oes-texture-half-float.html:94
> +    if (!(ext = gl.getExtension("OES_texture_half_float"))) {

Personal preference: I think assigning inside the conditional is a bit obfuscated-c-contest-y.

[Brent Fulgham]

Comment on attachment 218374 [details]
Patch

Dino has confirmed that intermediates always pass through the 32-bit path, so this patch is great as-is.

[Dean Jackson]

Comment on attachment 218374 [details]
Patch

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

>> Source/WebCore/platform/graphics/GraphicsContext3D.cpp:1220
>> +        destination[3] = convertFloatToHalfFloat(source[3]);
> 
> This pattern is repeated a bunch of times. I wonder if this could be a template, too?

Good point.

>> LayoutTests/fast/canvas/webgl/oes-texture-half-float.html:94
>> +    if (!(ext = gl.getExtension("OES_texture_half_float"))) {
> 
> Personal preference: I think assigning inside the conditional is a bit obfuscated-c-contest-y.

Me too, but this test comes from Khronos, and eventually we'll replace it with the direct version from there, so I'm not too concerned with the style for the moment. (Of course, it's still an ugly style in the official test! :)

[Dean Jackson]

(In reply to comment #15)
> (From update of attachment 218374 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218374&action=review
> 
> >> Source/WebCore/platform/graphics/GraphicsContext3D.cpp:1220
> >> +        destination[3] = convertFloatToHalfFloat(source[3]);
> > 
> > This pattern is repeated a bunch of times. I wonder if this could be a template, too?
> 
> Good point.

Actually, it's slightly different in enough places that it didn't really reduce much source code.

[Dean Jackson]

Committed r160119: <http://trac.webkit.org/changeset/160119>