Bug 110936 - [WebGL] Support for texImage2D of type HALF_FLOAT_OES
Summary: [WebGL] Support for texImage2D of type HALF_FLOAT_OES
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords:
: 111194 111196 111197 (view as bug list)
Depends on:
Blocks: 109853 125541
  Show dependency treegraph
 
Reported: 2013-02-26 19:18 PST by Nayan Kumar K
Modified: 2013-12-10 13:35 PST (History)
18 users (show)

See Also:


Attachments
WIP patch (11.93 KB, patch)
2013-02-26 20:21 PST, Nayan Kumar K
no flags Details | Formatted Diff | Diff
Patch (47.34 KB, patch)
2013-03-27 15:06 PDT, Nayan Kumar K
no flags Details | Formatted Diff | Diff
Patch (42.79 KB, patch)
2013-03-27 21:59 PDT, Nayan Kumar K
no flags Details | Formatted Diff | Diff
Patch (70.65 KB, patch)
2013-12-03 19:18 PST, Dean Jackson
bfulgham: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nayan Kumar K 2013-02-26 19:18:08 PST
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.
Comment 1 Nayan Kumar K 2013-02-26 20:21:46 PST
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
Comment 2 Nayan Kumar K 2013-03-01 16:39:48 PST
*** Bug 111194 has been marked as a duplicate of this bug. ***
Comment 3 Nayan Kumar K 2013-03-01 16:40:03 PST
*** Bug 111196 has been marked as a duplicate of this bug. ***
Comment 4 Nayan Kumar K 2013-03-01 16:40:13 PST
*** Bug 111197 has been marked as a duplicate of this bug. ***
Comment 5 Kenneth Russell 2013-03-18 17:01:45 PDT
Comment on attachment 190430 [details]
WIP patch

@nayankk: are you planning to move this patch forward?
Comment 6 Nayan Kumar K 2013-03-25 11:29:04 PDT
(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.
Comment 7 Nayan Kumar K 2013-03-27 15:06:07 PDT
Created attachment 195407 [details]
Patch
Comment 8 Nayan Kumar K 2013-03-27 21:59:33 PDT
Created attachment 195479 [details]
Patch
Comment 9 Kenneth Russell 2013-04-03 18:04:19 PDT
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.
Comment 10 Dean Jackson 2013-12-01 21:27:25 PST
<rdar://problem/15563029>
Comment 11 Dean Jackson 2013-12-03 19:18:24 PST
Created attachment 218374 [details]
Patch
Comment 12 Tim Horton 2013-12-04 10:43:51 PST
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!)
Comment 13 Brent Fulgham 2013-12-04 11:23:04 PST
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.
Comment 14 Brent Fulgham 2013-12-04 11:26:05 PST
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.
Comment 15 Dean Jackson 2013-12-04 11:35:23 PST
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! :)
Comment 16 Dean Jackson 2013-12-04 13:19:55 PST
(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.
Comment 17 Dean Jackson 2013-12-04 13:23:35 PST
Committed r160119: <http://trac.webkit.org/changeset/160119>