Implement WebGL OES_texture_half_float extension

RESOLVED FIXED in mozilla29

Status

()

Core
Canvas: WebGL
--
enhancement
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: jgilbert, Assigned: vlad)

Tracking

unspecified
mozilla29
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=jgilbert][lang=c++] webgl-extension, URL)

Attachments

(3 attachments, 7 obsolete attachments)

2.45 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
34.71 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
7.42 KB, patch
jgilbert
: review-
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
The extension draft is here:
http://www.khronos.org/registry/webgl/extensions/OES_texture_half_float/

An overview of how WebGL extensions are added is here: https://wiki.mozilla.org/Platform/GFX/WebGL/Contribute/Extensions

OES_texture_float was implemented in bug 630672, and should be very similar to what is needed here.
No longer blocks: 728319
Whiteboard: [mentor=jgilbert][lang=c++] → [mentor=jgilbert][lang=c++] webgl-extension
Assignee: nobody → andrew.quartey
Created attachment 623693 [details] [diff] [review]
patch
Attachment #623693 - Flags: feedback?(jgilbert)
(Reporter)

Comment 2

6 years ago
Comment on attachment 623693 [details] [diff] [review]
patch

Review of attachment 623693 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/dom_quickstubs.qsconf
@@ +508,5 @@
>      'nsIWebGLExtensionStandardDerivatives' : 'nsIDOMWebGLRenderingContext',
>      'nsIWebGLExtensionTextureFilterAnisotropic' : 'nsIDOMWebGLRenderingContext',
>      'nsIWebGLExtensionLoseContext' : 'nsIDOMWebGLRenderingContext',
>      'nsIWebGLExtensionCompressedTextureS3TC' : 'nsIDOMWebGLRenderingContext',
> +	'nsIWebGLExtensionTextureHalfFloat' : 'nsIDOMWebGLRenderingContext',

This should be 4-space, not tab.
Attachment #623693 - Flags: feedback?(jgilbert) → feedback+
I ran the conformance test suite against the latest build without the patch and of the 1019 failures, an overwhelming majority are texture based. Is this expected?
Are you talking about:
 - our snapshot of 1.0.1?
 - upstream 1.0.1?
 - upstream trunk aka 1.0.2-beta?

Our snapshot should pass, barring driver bugs; upstream 1.0.1 should pass except for 1 non-texture-related page that was recently modified; I haven't looked into 1.0.2 yet.

In this case I would only run the specific page that tests this extension, from the trunk if our snapshot doesn't have this test page.
(In reply to Benoit Jacob [:bjacob] from comment #4)
> Are you talking about:
>  - our snapshot of 1.0.1?
>  - upstream 1.0.1?
>  - upstream trunk aka 1.0.2-beta?

The conformance test version is 1.0.1(beta). That's the one from the latest pull of mozilla-central. 


> In this case I would only run the specific page that tests this extension,
> from the trunk if our snapshot doesn't have this test page.

The Khronos trunk doesn't have a test for this extension. I reckon modifying the existing OES_texture_float extension test for this should suffice since they differ very little.
Created attachment 628554 [details] [diff] [review]
WIP

WIP - added texture conversion bits and test. 

Currently, since i'm unable to get neither pixel types: 'HALF_FLOAT' & 'HALF_FLOAT_OES' to be exposed, i hard coded the value in associated test for this; for demonstration purposes. I do think i'm missing something but unsure at this point.
Attachment #623693 - Attachment is obsolete: true
Attachment #628554 - Flags: feedback?(jgilbert)
(Reporter)

Comment 7

6 years ago
Comment on attachment 628554 [details] [diff] [review]
WIP

Review of attachment 628554 [details] [diff] [review]:
-----------------------------------------------------------------

Good progress.

::: content/canvas/src/WebGLContext.cpp
@@ +872,5 @@
>              isSupported = gl->IsExtensionSupported(gl->IsGLES2() ? GLContext::OES_texture_float 
>                                                                   : GLContext::ARB_texture_float);
> +            break;
> +        case WebGL_OES_texture_half_float:
> +            isSupported = gl->IsExtensionSupported(GLContext:: OES_texture_half_float);

No space after colons.

Also, note that this will only enable it for GLES.
The desktop extensions appear to be GL_ARB_half_float_pixel and possibly GL_NV_half_float, though the latter should be superceeded by the former.

::: content/canvas/src/WebGLContextGL.cpp
@@ +6104,5 @@
>              default:
>                  NS_ABORT_IF_FALSE(false, "Coding mistake?! Should never reach this point.");
>                  return WebGLTexelConversions::BadFormat;
>          }
> +    }else if (type == LOCAL_GL_HALF_FLOAT_OES) {

Space before else.

::: content/canvas/test/webgl/conformance/extensions/oes-texture-half-float.html
@@ +57,5 @@
> +
> +  var wtu = WebGLTestUtils;
> +  var canvas = document.getElementById("canvas");
> +  var gl = create3DContext(canvas);
> +  var HALF_FLOAT_OES = 0x8D61;

I believe this constant should be exposed at |gl.getExtension("OES_texture_half_float").HALF_FLOAT_OES|.

@@ +133,5 @@
> +    // Verify that the texture actually works for sampling and contains the expected data.
> +    gl.uniform1i(gl.getUniformLocation(testProgram, "tex"), 0);
> +    wtu.drawQuad(gl);
> +    checkRenderingResults();
> +  }

Other tests we need for this:
texSubImage2D, both for when it should work, and when it shouldn't. (mismatched types, mostly)
More formats: ALPHA, LUMINANCE, LUNINANCE_ALPHA, and RGB are supported, as well as RGBA.
Check that we can't upload data using typed arrays, but can upload from DOM elements.

@@ +153,5 @@
> +    // While strictly speaking it is probably legal for a WebGL implementation to support
> +    // floating-point textures but not as attachments to framebuffer objects, any such
> +    // implementation is so poor that it arguably should not advertise support for the
> +    // OES_texture_half_float extension. For this reason the conformance test requires that the
> +    // framebuffer is complete here.

The spec allows implementations to fail to attach a HALF_FLOAT texture to an FBO, so we can't actually test this, unfortunately.
I believe in particular there are GLES2 devices which support sampling float textures but not rendering to them.

::: content/canvas/test/webgl/resources/desktop-gl-constants.js
@@ +2071,5 @@
>    'PROGRAM_RESULT_COMPONENTS_NV': 0x8907,
>    'MAX_PROGRAM_ATTRIB_COMPONENTS_NV': 0x8908,
>    'MAX_PROGRAM_RESULT_COMPONENTS_NV': 0x8909,
>    'MAX_PROGRAM_GENERIC_ATTRIBS_NV': 0x8DA5,
> +  'MAX_PROGRAM_GENERIC_RESULTS_NV': 0x8DA6,

Maybe my eyes are tired, but I can't see what this change was?

::: gfx/gl/GLDefs.h
@@ +2399,5 @@
>  #define LOCAL_GL_MAX_PROGRAM_LOOP_COUNT_NV 0x88F8
>  #define LOCAL_GL_NV_fragment_program_option 1
>  #define LOCAL_GL_NV_half_float 1
>  #define LOCAL_GL_HALF_FLOAT_NV 0x140B
> +#define LOCAL_GL_HALF_FLOAT_OES 0x8D61

Don't put this here. Instead, add a new mini-section down near |// ARB_sync|, such as |// OES_texture_float|.
Attachment #628554 - Flags: feedback?(jgilbert) → feedback-
Automatically unassigning mentored bugs that haven't seen action for two weeks. The idea is to maximize the number of mentored bugs that newcomers can pick from.
Assignee: andrew.quartey → nobody
Is it OK for me take Andrew's patch and continue work on it?
(Reporter)

Comment 10

5 years ago
It has been 7 months, so I don't see why not.
Created attachment 752488 [details] [diff] [review]
Updated version of patch

This is an upated version of Andrew's patch. I did not touch any of the test code, but the C/C++ code, webidl, and bindings have been updated to apply against the latest sources. Unfortunatly, it is untested as it turns out that I do not have a GPU that implements the half_float extension.
Attachment #752488 - Flags: feedback?(jgilbert)
(Reporter)

Comment 12

5 years ago
Comment on attachment 752488 [details] [diff] [review]
Updated version of patch

Review of attachment 752488 [details] [diff] [review]:
-----------------------------------------------------------------

The only big problem is that WebGLTexelConversion needs to know that half-float types are floats, so it can convert properly.

::: content/canvas/src/WebGLContext.cpp
@@ +955,5 @@
>              return gl->IsExtensionSupported(gl->IsGLES2() ? GLContext::OES_texture_float
>                                                            : GLContext::ARB_texture_float);
> +        case OES_texture_half_float:
> +            return gl->IsExtensionSupported(gl->IsGLES2() ? GLContext::OES_texture_float
> +                                                            : GLContext::ARB_texture_float);

Indent alignment between '?' and ':'.

::: content/canvas/src/WebGLContextGL.cpp
@@ +5342,4 @@
>                  NS_ABORT_IF_FALSE(false, "Coding mistake?! Should never reach this point.");
>                  return WebGLTexelConversions::BadFormat;
>          }
> +    }else if (type == LOCAL_GL_HALF_FLOAT_OES) {

Space before 'else'.

@@ +5355,5 @@
> +                return WebGLTexelConversions::R16F;
> +            case LOCAL_GL_LUMINANCE_ALPHA:
> +                return WebGLTexelConversions::RA16F;
> +            default:
> +                NS_ABORT_IF_FALSE(false, "Coding mistake?! Should never reach this point.");

MOZ_NOT_REACHED.

::: content/canvas/src/WebGLContextValidate.cpp
@@ +610,5 @@
>      {
>          if (jsArrayType != -1) {
>              if ((type == LOCAL_GL_UNSIGNED_BYTE && jsArrayType != js::ArrayBufferView::TYPE_UINT8) ||
> +                (type == LOCAL_GL_FLOAT && jsArrayType != js::ArrayBufferView::TYPE_FLOAT32) ||
> +                (type == LOCAL_GL_HALF_FLOAT_OES && (jsArrayType != js::ArrayBufferView::TYPE_INT16 && jsArrayType != js::ArrayBufferView::TYPE_UINT16)))

We shouldn't allow initializing half-floats from TypedArrays, since there's no Float16 type.
Uploads can still happen with ArrayBuffers, though. Reduce this addition to fail if `type == LOCAL_GL_HALF_FLOAT_OES`.

@@ +620,5 @@
>  
> +        int texMultiplier = 1;
> +        if (type == LOCAL_GL_HALF_FLOAT_OES)
> +            texMultiplier = 2;
> +        if (type == LOCAL_GL_FLOAT)

else if

::: content/canvas/src/WebGLTexelConversions.h
@@ +109,5 @@
>          case WebGLTexelConversions::RGBA5551:
>          case WebGLTexelConversions::RGBA4444:
>          case WebGLTexelConversions::RGB565:
> +        case WebGLTexelConversions::R16F:
> +        case WebGLTexelConversions::A16F:

All these half-float formats need to be added to `IsFloatFormat`.

::: content/canvas/test/webgl/conformance/extensions/oes-texture-half-float.html
@@ +150,5 @@
> +    gl.framebufferTexture2D(gl.FRAMEBUFFER, gl.COLOR_ATTACHMENT0, gl.TEXTURE_2D, texture, 0);
> +    gl.bindTexture(gl.TEXTURE_2D, null);
> +    shouldBe("gl.checkFramebufferStatus(gl.FRAMEBUFFER)", "gl.FRAMEBUFFER_COMPLETE");
> +    // While strictly speaking it is probably legal for a WebGL implementation to support
> +    // floating-point textures but not as attachments to framebuffer objects, any such

This is bad, but I suppose I should really take it up with upstream.
Attachment #752488 - Flags: feedback?(jgilbert) → feedback-
In regard to the WebGLTexelConversion changes, how would I implement that? It appears that I need to return 0.5 from WebGLImageConverter::NumElementsPerTexelForFormat() when Format is R16F or A16F (or I am missunderstainding the purpose of the function)

Comment 14

5 years ago
Created attachment 797484 [details] [diff] [review]
patch step 1 - revision 1: GLContext stuff

All right, I continued the bug, and now the conformance test is all green.
Attachment #797484 - Flags: review?(jgilbert)

Comment 15

5 years ago
Created attachment 797485 [details] [diff] [review]
patch step 2 - revision 1: Moce WebGL texture entry points in separated files

No modification guaranteed. All tested passed.
Attachment #797485 - Flags: review?(jgilbert)

Comment 16

5 years ago
Created attachment 797486 [details] [diff] [review]
patch step 3 - revision 1: Implement the WebGL extension against desktop driver

As I said, the webgl conformance test is all green.
Other patches are coming :
 - Adding the test on tbpl
 - Compatibility on the OpenGL ES extension
Attachment #797486 - Flags: review?(jgilbert)

Comment 17

5 years ago
I would like to precise that all contributors on this bug will be in the commit message on the mozilla-central log of course. =)
(Reporter)

Updated

5 years ago
Attachment #797484 - Flags: review?(jgilbert) → review+
(Reporter)

Comment 18

5 years ago
Comment on attachment 797485 [details] [diff] [review]
patch step 2 - revision 1: Moce WebGL texture entry points in separated files

Review of attachment 797485 [details] [diff] [review]:
-----------------------------------------------------------------

As we talked about, moving things out of the huge WebGLContextGL is good, but I don't think we want to split them up this finely. A combined WebGLContextTexture should only be about 1200-1300 lines, which is pretty managable, and I think is worth the bonus of having all the texture stuff in one place. (For instance, I would have expected GenMipmaps to be in TextureLoad)
Attachment #797485 - Flags: review?(jgilbert) → review-
(Reporter)

Comment 19

5 years ago
Comment on attachment 797486 [details] [diff] [review]
patch step 3 - revision 1: Implement the WebGL extension against desktop driver

Review of attachment 797486 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/WebGLContextGL.cpp
@@ +353,5 @@
>          // first, compute the size of the buffer we should allocate to initialize the texture as black
>  
>          uint32_t texelSize = 0;
> +        WebGLenum type = LOCAL_GL_UNSIGNED_BYTE;
> +        if (!ValidateTexFormatAndType(internalformat, type, -1, &texelSize, info))

It is our style not to pass-back by reference, since pass-back by pointer makes it more explicit at the call-site. In-out params are not great in general, either.

::: content/canvas/src/WebGLContextTextureValidate.cpp
@@ +163,5 @@
>                      return false;
>              }
>  
>              return true;
> +            

whitespace

@@ +218,5 @@
>      }
>  
> +    int texMultiplier = 1;
> +
> +    // If not a packed types, then it's might be a standart type.

// If not a packed type, then it might be a standard type.

@@ +242,5 @@
> +            ErrorInvalidEnum("%s: invalid format HALF_FLOAT_OES: need OES_texture_half_float enabled", info);
> +            return false;
> +        }
> +
> +        if (jsArrayType != -1)

To match other behaviors, we should probably force this to be from a Uint16 array.

Actually, WebGL1's OES_texture_[half_]float shouldn't allow initializing textures from array buffers, anyways. (It's only supposed to accept `null`)

@@ +252,5 @@
> +        /**
> +         * GL_HALF_FLOAT_OES != GL_HALF_FLOAT
> +         * So internally, we use GL_HALF_FLOAT_OES as GL_HALF_FLOAT
> +         */
> +        type = LOCAL_GL_HALF_FLOAT;

I still don't believe we should do this. It may still be tricky to combine these, since the rules are ever-so-slightly different. The whole WG has been burned on these float textures many times, so we're unlikely to want to keep messing with them.

If we want to combine these two, we will need to spec the combination. For now, just treat them dumbly as different types, since that requires no spec change.

::: content/canvas/src/WebGLTexelConversions.h
@@ +53,5 @@
> +// half precision float
> +// seeeeemmmmmmmmmm
> +
> +// IEEE 16bits floating point:
> +const uint16_t kFloat16Value_Zero     = 0x0000; // = 0000000000000000b

Specifically, this is +0.

@@ +55,5 @@
> +
> +// IEEE 16bits floating point:
> +const uint16_t kFloat16Value_Zero     = 0x0000; // = 0000000000000000b
> +const uint16_t kFloat16Value_One      = 0x3C00; // = 0011110000000000b
> +const uint16_t kFloat16Value_Infinity = 0x7C00; // = 0111110000000000b

This is +Inf.

@@ +56,5 @@
> +// IEEE 16bits floating point:
> +const uint16_t kFloat16Value_Zero     = 0x0000; // = 0000000000000000b
> +const uint16_t kFloat16Value_One      = 0x3C00; // = 0011110000000000b
> +const uint16_t kFloat16Value_Infinity = 0x7C00; // = 0111110000000000b
> +const uint16_t kFloat16Value_Nan      = 0x7FFF; // = 0111111111111111b

NaN is any x11111xxxxxxxxxx. (And 'NaN' always has both 'N's capitalized)

@@ +59,5 @@
> +const uint16_t kFloat16Value_Infinity = 0x7C00; // = 0111110000000000b
> +const uint16_t kFloat16Value_Nan      = 0x7FFF; // = 0111111111111111b
> +
> +FORCE_INLINE uint16_t
> +packToFloat16(float v)

This is where it gets tricky extremely fast.
We basically need to duplicate what we have in:
http://mxr.mozilla.org/mozilla-central/source/mfbt/FloatingPoint.h

Evidently, PGO in particular tends to get these things really wrong.

I think it's best to leave this out of the patch, for now, and just emit an non-spec INVALID_OPERATION if an app tries to create a half-float texture from an HTML element.

::: content/canvas/src/WebGLTexture.cpp
@@ +352,5 @@
> +        {
> +            if (mMinFilter == LOCAL_GL_LINEAR ||
> +                mMinFilter == LOCAL_GL_LINEAR_MIPMAP_LINEAR ||
> +                mMinFilter == LOCAL_GL_LINEAR_MIPMAP_NEAREST ||
> +                mMinFilter == LOCAL_GL_NEAREST_MIPMAP_LINEAR)

Flip this to check if we're not NEAREST and not NEAREST_MIPMAP_NEAREST.

@@ +358,5 @@
> +                mContext->GenerateWarning("%s is a texture with a linear minification filter, "
> +                                          "which is not compatible with gl.HALF_FLOAT by default.", msg_rendering_as_black);
> +                mFakeBlackStatus = DoNeedFakeBlack;
> +            }
> +            else if (mMagFilter == LOCAL_GL_LINEAR)

Flip this to check if we're not NEAREST.
Attachment #797486 - Flags: review?(jgilbert) → review-

Comment 20

5 years ago
(In reply to Jeff Gilbert [:jgilbert] from comment #18)
> Comment on attachment 797485 [details] [diff] [review]
> patch step 2 - revision 1: Moce WebGL texture entry points in separated files
> 
> Review of attachment 797485 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> As we talked about, moving things out of the huge WebGLContextGL is good,
> but I don't think we want to split them up this finely. A combined
> WebGLContextTexture should only be about 1200-1300 lines, which is pretty
> managable, and I think is worth the bonus of having all the texture stuff in
> one place. (For instance, I would have expected GenMipmaps to be in
> TextureLoad)

Sure!
(In reply to Jeff Gilbert [:jgilbert] from comment #19)
> This is where it gets tricky extremely fast.

Well, the code that Guillaume is adding here is only a few dozen lines, is self-contained, and is enough to be conformant. It doesn't seem too bad to me.

> We basically need to duplicate what we have in:
> http://mxr.mozilla.org/mozilla-central/source/mfbt/FloatingPoint.h

Do you mean that we should add code to mfbt/FloatingPoint.h ? I would disagree, as it's not clear that there will be any user of half-float than the present WebGL code, in the near future.

Or do you mean that the present code should be refactored to introduce local helper function modelled after what's in mfbt/FloatingPoint.h? I would agree that's a good idea, but I'm not sure that I would agree with r-ing this for that --- seems like decent follow-up material.

> 
> Evidently, PGO in particular tends to get these things really wrong.

I'm not sure that PGO matters here. PGO matters for code that is not obvious how to compile to efficient machine code --- e.g. when it's not obvious which functions to inline, typically for large bodies of not-self-contained, not-carefully-written-for-speed code.

But for the kind of self-contained, plain low-level code that we have here, PGO shouldn't make a big difference.

> 
> I think it's best to leave this out of the patch, for now, and just emit an
> non-spec INVALID_OPERATION if an app tries to create a half-float texture
> from an HTML element.

It feels a bit unusual to reject a conformant patch that already works and ask for something non-conformant instead.

Comment 22

5 years ago
Created attachment 797973 [details] [diff] [review]
patch step 2 - revision 2: Moce WebGL texture entry points in separated files
Attachment #797485 - Attachment is obsolete: true
Attachment #797973 - Flags: review?(jgilbert)
(Reporter)

Comment 23

5 years ago
(In reply to Benoit Jacob [:bjacob] from comment #21)
> (In reply to Jeff Gilbert [:jgilbert] from comment #19)
> > This is where it gets tricky extremely fast.
> 
> Well, the code that Guillaume is adding here is only a few dozen lines, is
> self-contained, and is enough to be conformant. It doesn't seem too bad to
> me.
I'm not saying that it looks wrong, I'm saying that we've been explicitly warned that 'it looks fine' isn't good enough.
> 
> > We basically need to duplicate what we have in:
> > http://mxr.mozilla.org/mozilla-central/source/mfbt/FloatingPoint.h
> 
> Do you mean that we should add code to mfbt/FloatingPoint.h ? I would
> disagree, as it's not clear that there will be any user of half-float than
> the present WebGL code, in the near future.
> 
> Or do you mean that the present code should be refactored to introduce local
> helper function modelled after what's in mfbt/FloatingPoint.h? I would agree
> that's a good idea, but I'm not sure that I would agree with r-ing this for
> that --- seems like decent follow-up material.
We should replicate and adapt what functions we need from FloatingPoint.h. Templatizing FloatingPoint.h is a good follow-up, though.
> 
> > 
> > Evidently, PGO in particular tends to get these things really wrong.
> 
> I'm not sure that PGO matters here. PGO matters for code that is not obvious
> how to compile to efficient machine code --- e.g. when it's not obvious
> which functions to inline, typically for large bodies of not-self-contained,
> not-carefully-written-for-speed code.
> 
> But for the kind of self-contained, plain low-level code that we have here,
> PGO shouldn't make a big difference.
Yet that's what the comments in FloatingPoint.h warns about, even for "seemingly reasonable bitwise algorithms"!
> 
> > 
> > I think it's best to leave this out of the patch, for now, and just emit an
> > non-spec INVALID_OPERATION if an app tries to create a half-float texture
> > from an HTML element.
> 
> It feels a bit unusual to reject a conformant patch that already works and
> ask for something non-conformant instead.
It's true, but I think that's the simplest and quickest way forward. Alternatively, we can hold this whole patch up on this sticky float16 stuff. 

The big issue here is that we *don't* know that it works right, we merely suspect it to be so. However, the big comment at the top of FloatingPoint.h is pretty persuasive that we should be careful with this, since it has known problems which we *have* run into in the past.
(Reporter)

Updated

5 years ago
Blocks: 912280

Comment 24

5 years ago
(In reply to Jeff Gilbert [:jgilbert] from comment #23)
> (In reply to Benoit Jacob [:bjacob] from comment #21)
> > (In reply to Jeff Gilbert [:jgilbert] from comment #19)
> > > This is where it gets tricky extremely fast.
> > 
> > Well, the code that Guillaume is adding here is only a few dozen lines, is
> > self-contained, and is enough to be conformant. It doesn't seem too bad to
> > me.
> I'm not saying that it looks wrong, I'm saying that we've been explicitly
> warned that 'it looks fine' isn't good enough.
> > 
> > > We basically need to duplicate what we have in:
> > > http://mxr.mozilla.org/mozilla-central/source/mfbt/FloatingPoint.h
> > 
> > Do you mean that we should add code to mfbt/FloatingPoint.h ? I would
> > disagree, as it's not clear that there will be any user of half-float than
> > the present WebGL code, in the near future.
> > 
> > Or do you mean that the present code should be refactored to introduce local
> > helper function modelled after what's in mfbt/FloatingPoint.h? I would agree
> > that's a good idea, but I'm not sure that I would agree with r-ing this for
> > that --- seems like decent follow-up material.
> We should replicate and adapt what functions we need from FloatingPoint.h.
> Templatizing FloatingPoint.h is a good follow-up, though.
> > 
> > > 
> > > Evidently, PGO in particular tends to get these things really wrong.
> > 
> > I'm not sure that PGO matters here. PGO matters for code that is not obvious
> > how to compile to efficient machine code --- e.g. when it's not obvious
> > which functions to inline, typically for large bodies of not-self-contained,
> > not-carefully-written-for-speed code.
> > 
> > But for the kind of self-contained, plain low-level code that we have here,
> > PGO shouldn't make a big difference.
> Yet that's what the comments in FloatingPoint.h warns about, even for
> "seemingly reasonable bitwise algorithms"!
> > 
> > > 
> > > I think it's best to leave this out of the patch, for now, and just emit an
> > > non-spec INVALID_OPERATION if an app tries to create a half-float texture
> > > from an HTML element.
> > 
> > It feels a bit unusual to reject a conformant patch that already works and
> > ask for something non-conformant instead.
> It's true, but I think that's the simplest and quickest way forward.
> Alternatively, we can hold this whole patch up on this sticky float16 stuff. 
> 
> The big issue here is that we *don't* know that it works right, we merely
> suspect it to be so. However, the big comment at the top of FloatingPoint.h
> is pretty persuasive that we should be careful with this, since it has known
> problems which we *have* run into in the past.
So what is the plan? Because what your are talking about seams to be a separated bug...
Note: I hadn't started reading FloatingPoint.h so it important things are said there, I wouldn't have known them.
(Reporter)

Comment 26

5 years ago
I think we should land what we can (no support for conversions yet) immediately, since there's no reason to delay that. We would have to pretend to blacklist it (temporarily disabling it) until we actually land the conversion support. This is something we definitely want to support, but part of the support will require more work, so we should land what we can now, and finish it as we get review on things.

Comment 27

5 years ago
Here's an Emscripten-based application that empirically tests the extensions OES_texture_float, OES_texture_half_float, OES_texture_float_linear and OES_texture_half_float_linear:

https://dl.dropboxusercontent.com/u/40949268/emcc/float_rendertarget_test/Cube_d.html?-colorformat&TextureFormat_R32G32B32A32_FLOAT&-renderbuffer

Perhaps it can be of some help in checking the formats in action.
(In reply to Jeff Gilbert [:jgilbert] from comment #26)
> I think we should land what we can (no support for conversions yet)
> immediately, since there's no reason to delay that. We would have to pretend
> to blacklist it (temporarily disabling it) until we actually land the
> conversion support. This is something we definitely want to support, but
> part of the support will require more work, so we should land what we can
> now, and finish it as we get review on things.

The danger of getting things wrong in the conversions is not very large, no?  Worst case, we send weird floating point data to the GPU, which can already be done anyway.  If we run into the PGO-related problems, we can go back and diagnose and fix things then.  But otherwise there's no path forward -- the conversions won't get checked in because "something" might munge up the bitwise operations, but we have no example of that type of problem happening.
(Reporter)

Comment 29

5 years ago
That's fair. I'll take another look tomorrow.
(Reporter)

Comment 30

5 years ago
Comment on attachment 797973 [details] [diff] [review]
patch step 2 - revision 2: Moce WebGL texture entry points in separated files

Review of attachment 797973 [details] [diff] [review]:
-----------------------------------------------------------------

Not sure why this was in limbo.

::: content/canvas/src/WebGLContextTextures.cpp
@@ +718,5 @@
> +                            ErrorResult& rv)
> +{
> +    if (!IsContextStable())
> +        return;
> +    

Whitespace. Let's strip the existing whitespace when we move things.
Attachment #797973 - Flags: review?(jgilbert) → review+
(Reporter)

Comment 31

5 years ago
Comment on attachment 797486 [details] [diff] [review]
patch step 3 - revision 1: Implement the WebGL extension against desktop driver

Review of attachment 797486 [details] [diff] [review]:
-----------------------------------------------------------------

Alright, this should be good after some fixes.

::: content/canvas/src/WebGLTexelConversions.h
@@ +56,5 @@
> +// IEEE 16bits floating point:
> +const uint16_t kFloat16Value_Zero     = 0x0000; // = 0000000000000000b
> +const uint16_t kFloat16Value_One      = 0x3C00; // = 0011110000000000b
> +const uint16_t kFloat16Value_Infinity = 0x7C00; // = 0111110000000000b
> +const uint16_t kFloat16Value_Nan      = 0x7FFF; // = 0111111111111111b

Rather, NaN is any x11111yyyyyyyyyy where yyyyyyyyyy is non-zero.

@@ +62,5 @@
> +FORCE_INLINE uint16_t
> +packToFloat16(float v)
> +{
> +    union
> +    {

`union {`

@@ +68,5 @@
> +        uint32_t f32Bits;
> +    };
> +
> +    f32Value = v;
> +    uint16_t f16Bits = uint16_t(f32Bits >> 16) & 0x8000;

Comment that this pulls in the sign from v.

@@ +75,5 @@
> +        // +0 or -0
> +        return f16Bits;
> +    } else if (f32Value != f32Value) {
> +        // this is NAN
> +        return f16Bits | kFloat16Value_Nan;

s/nan/NaN/i

@@ +102,5 @@
> +        float f32Value;
> +        uint32_t f32Bits;
> +    };
> +
> +    f32Bits = uint32_t(v & 0x8000) << 16;

// Grab sign bit.

@@ +120,5 @@
> +            f32Bits |= 0x7F800000;
> +        }
> +        return f32Value;
> +    } else {
> +        f32Bits |= uint32_t(exp - 15 + 127) << 10;

This is technically OK, but is something we should avoid. Just switch this to `exp + 127 - 15` or `exp + (-15 + 127)`.

@@ +168,5 @@
>  
>  template<int Format,
>           bool IsFloat = IsFloatFormat<Format>::Value,
> +         bool Is16bpp = Is16bppFormat<Format>::Value,
> +         bool IsHalfFloat = IsHalfFloatFormat<Format>::Value>

We should change this to IsFloat32 and IsFloat16.
ping -- is this still good after a couple of fixes? :)  Jeff, you may need to just land this, if you don't mind.. would probably be fastest.
Taking
Assignee: nobody → vladimir
Created attachment 8366189 [details] [diff] [review]
part 1, OES_texture_half_float - GLContext support

Basically the same as previous step 1, just rebased.  Just adds needed pieces to GLContext.
Attachment #628554 - Attachment is obsolete: true
Attachment #752488 - Attachment is obsolete: true
Attachment #797484 - Attachment is obsolete: true
Attachment #8366189 - Flags: review?(jgilbert)
Created attachment 8366190 [details] [diff] [review]
part 2, OES_texture_half_float - core implementation

This is the core implementation.  It doesn't move the WebGL texture entry points into separate files, because that patch bitrotted badly, and I didn't want to revive it.  Replaces steps 2 and 3 earlier, and also fixes a few bugs along the way.
Attachment #797486 - Attachment is obsolete: true
Attachment #797973 - Attachment is obsolete: true
Created attachment 8366191 [details] [diff] [review]
part 3, OES_texture_half_float - desktop GL support

This handles the differences between GL_HALF_FLOAT and GL_HALF_FLOAT_OES.  This is done in a different way than the original patches, resolving the difference at the point of usage, instead of weaving a confusing mix of the OES and non-OES enums.
Attachment #8366191 - Flags: review?(jgilbert)
(Reporter)

Updated

4 years ago
Attachment #8366189 - Flags: review?(jgilbert) → review+
(Reporter)

Comment 37

4 years ago
Comment on attachment 8366190 [details] [diff] [review]
part 2, OES_texture_half_float - core implementation

Review of attachment 8366190 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/WebGLContextGL.cpp
@@ +4195,5 @@
> +                return WebGLTexelFormat::R16F;
> +            case LOCAL_GL_LUMINANCE_ALPHA:
> +                return WebGLTexelFormat::RA16F;
> +            default:
> +                NS_ABORT_IF_FALSE(false, "Coding mistake?! Should never reach this point.");

MOZ_ASSERT

::: gfx/gl/GLContext.cpp
@@ +1134,5 @@
>      const bool firstRun = false;
>  #endif
>  
> +#ifdef DEBUG
> +    printf_stderr("OpenGL extensions: %s\n", extensions);

We already have something like this, though it only is supposed to spew on the first run. (IIRC)
This should be gated behind DebugMode().
Attachment #8366190 - Flags: review?(jgilbert) → review+
(Reporter)

Comment 38

4 years ago
Comment on attachment 8366191 [details] [diff] [review]
part 3, OES_texture_half_float - desktop GL support

Review of attachment 8366191 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/WebGLContextExtensions.cpp
@@ +90,5 @@
>          case OES_texture_half_float:
> +            if (gl->IsExtensionSupported(GLContext::OES_texture_half_float))
> +                return true;
> +            if (gl->IsSupported(GLFeature::texture_half_float)) {
> +                mNeedsHalfFloatTypeTranslation = true;

I really don't like this side-effect.

::: content/canvas/src/WebGLContextGL.cpp
@@ +3975,5 @@
>      size_t dstStride = RoundedToNextMultipleOf(dstPlainRowSize, mPixelStoreUnpackAlignment).value();
>  
> +    // convert type for half float
> +    GLenum realType = type;
> +    if (realType == LOCAL_GL_HALF_FLOAT_OES && mNeedsHalfFloatTypeTranslation) {

We should unconditionally choose the right type, instead of only when this variable is set.
Attachment #8366191 - Flags: review?(jgilbert) → review-
(In reply to Jeff Gilbert [:jgilbert] from comment #38)
> Comment on attachment 8366191 [details] [diff] [review]
> part 3, OES_texture_half_float - desktop GL support
> 
> Review of attachment 8366191 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/WebGLContextExtensions.cpp
> @@ +90,5 @@
> >          case OES_texture_half_float:
> > +            if (gl->IsExtensionSupported(GLContext::OES_texture_half_float))
> > +                return true;
> > +            if (gl->IsSupported(GLFeature::texture_half_float)) {
> > +                mNeedsHalfFloatTypeTranslation = true;
> 
> I really don't like this side-effect.

Well, not really a side-effect.. I mean it's intended if we have texture_half_float and not OES_texture_half_float.

> ::: content/canvas/src/WebGLContextGL.cpp
> @@ +3975,5 @@
> >      size_t dstStride = RoundedToNextMultipleOf(dstPlainRowSize, mPixelStoreUnpackAlignment).value();
> >  
> > +    // convert type for half float
> > +    GLenum realType = type;
> > +    if (realType == LOCAL_GL_HALF_FLOAT_OES && mNeedsHalfFloatTypeTranslation) {
> 
> We should unconditionally choose the right type, instead of only when this
> variable is set.

Hmm, I guess I can check if we're GLES.  That makes sense!  Will do.
Blocks: 964788
Erm -- https://hg.mozilla.org/integration/mozilla-inbound/rev/ce3ebfe3fe11 has r+ from jgilbert on irc.  I jumped the gun a bit and forgot to upload it.  Patches that landed have been rebased from the ones here; green try server run.
https://hg.mozilla.org/mozilla-central/rev/2cbd9b9551cf
https://hg.mozilla.org/mozilla-central/rev/3cd30244cf6a
https://hg.mozilla.org/mozilla-central/rev/ce3ebfe3fe11
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.