Closed Bug 843668 Opened 7 years ago Closed 6 years ago

Implement WebGL EXT_sRGB

Categories

(Core :: Canvas: WebGL, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: vlad, Assigned: kamidphish)

References

Details

Attachments

(1 file, 18 obsolete files)

53.14 KB, patch
kamidphish
: review+
Details | Diff | Splinter Review
Assignee: nobody → dglastonbury
Attached patch fix-843668.patch (obsolete) — Splinter Review
Attachment #809578 - Flags: review?(jgilbert)
Attachment #809578 - Flags: review?(bjacob)
This needs a test for conformance. I'm not certain how I should do that. There doesn't appear to be an official test in the WebGL git repo, so I wrote a simple HTML + JS test that I used in my debugging/testing for this patch.
Flags: needinfo?(bjacob)
In your local clone of the WebGL git repo, look for an existing conformance test for some texture-related extension. For example, you could look at webgl-depth-texture.html, or at oes-texture-float.html. Note that you will have to add your test to the 00_test_list.txt file in the same directory.
Flags: needinfo?(bjacob)
Comment on attachment 809578 [details] [diff] [review]
fix-843668.patch

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

Your patch is reversed! I.e. it removes what it actually means to add. How'd that happen? hg export <revision> generates a patch file from an existing hg revision.
Attachment #809578 - Flags: review?(jgilbert)
Attachment #809578 - Flags: review?(bjacob)
Attachment #809578 - Attachment is obsolete: true
Fixed the order of the diff.
Attachment #810156 - Flags: review?(jgilbert)
Attachment #810156 - Flags: review?(bjacob)
Comment on attachment 810156 [details] [diff] [review]
mplementation of WebGL EXT_sRGB - https://www.khronos.org/registry/webgl/extensions/EXT_sRGB/

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

This is a good start, but it only provides limited support. Most importantly, desktop GL doesn't appear to support sRGB renderbuffers, so we'll have to fake those, and use textures behind the scenes.

There are some other problems on desktop, such as needing to query for one of ARB_framebuffer_sRGB or EXT_framebuffer_sRGB or desktop GL3+, and permanently enabling FRAMEBUFFER_SRGB.

We should also try to have DEBUG asserts that these sRGB surfaces all support sRGB updating and blending, since the desktop GL extensions actually leaves this unspecified. (yes, really) If any driver doesn't unconditionally support sRGB update/blend, we'll have to blocklist it. I think (hope) this is unlikely, and the GL3 spec clarifies this such that if a format is SRGB, then it supports SRGB blend/update.

::: content/canvas/src/WebGL2Context.cpp
@@ +1,2 @@
>  /* -*- Mode: C++; tab-width: 20; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
> +/* vim: set tw=20 sts=4 sw=4 et: */

Orphaned change, and do we really need two of these lines?

::: content/canvas/src/WebGLContextGL.cpp
@@ +1335,5 @@
>              }
>  
> +            case LOCAL_GL_FRAMEBUFFER_ATTACHMENT_COLOR_ENCODING_EXT:
> +                if (IsExtensionEnabled(EXT_sRGB)) {
> +                    const GLenum internalFormat = fba.Renderbuffer()->InternalFormat();

The fba can be either a renderbuffer or a texture. Renderbuffer() can be null.

@@ +3631,3 @@
>  
> +    if (!ValidateTexImage2DFormat(format))
> +        return ErrorInvalidEnumInfo("texImage2D: format", format);

`ValidateFoo` functions should generate their own errors.
Otherwise, this func should be named `IsValidTexImage2DFormat` or similar.

@@ +3703,5 @@
> +    // GL requires:
> +    //      format  ->  internalformat
> +    //      GL_RGB      GL_SRGB_EXT
> +    //      GL_RGBA     GL_SRGB_ALPHA_EXT
> +    if (IsExtensionEnabled(EXT_sRGB) && !gl->IsGLES2()) {

We don't need to check for the extension here, since our data's already validated.

::: gfx/gl/GLDefs.h
@@ +46,5 @@
> +// EXT_sRGB
> +#define LOCAL_GL_SRGB_EXT                               0x8C40
> +#define LOCAL_GL_SRGB_ALPHA_EXT                         0x8C42
> +#define LOCAL_GL_SRGB8_ALPHA8_EXT                       0x8C43
> +#define LOCAL_GL_FRAMEBUFFER_ATTACHMENT_COLOR_ENCODING_EXT  0x8210

We already should have all these in GLConsts.h.
Attachment #810156 - Flags: review?(jgilbert) → review-
(In reply to Jeff Gilbert [:jgilbert] from comment #6)
> Comment on attachment 810156 [details] [diff] [review]
> mplementation of WebGL EXT_sRGB -
> https://www.khronos.org/registry/webgl/extensions/EXT_sRGB/
> 
> Review of attachment 810156 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is a good start, but it only provides limited support. Most
> importantly, desktop GL doesn't appear to support sRGB renderbuffers, so
> we'll have to fake those, and use textures behind the scenes.
> 
> There are some other problems on desktop, such as needing to query for one
> of ARB_framebuffer_sRGB or EXT_framebuffer_sRGB or desktop GL3+, and
> permanently enabling FRAMEBUFFER_SRGB.
> 
> We should also try to have DEBUG asserts that these sRGB surfaces all
> support sRGB updating and blending, since the desktop GL extensions actually
> leaves this unspecified. (yes, really) If any driver doesn't unconditionally
> support sRGB update/blend, we'll have to blocklist it. I think (hope) this
> is unlikely, and the GL3 spec clarifies this such that if a format is SRGB,
> then it supports SRGB blend/update.
> 
> ::: content/canvas/src/WebGL2Context.cpp
> @@ +1,2 @@
> >  /* -*- Mode: C++; tab-width: 20; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
> > +/* vim: set tw=20 sts=4 sw=4 et: */
> 
> Orphaned change, and do we really need two of these lines?

Some other files have both mode lines. I added it to get my vim to work right, but I see there's a vim extension to read emacs modelines, so I'll remove this.
 
> ::: content/canvas/src/WebGLContextGL.cpp
> @@ +1335,5 @@
> >              }
> >  
> > +            case LOCAL_GL_FRAMEBUFFER_ATTACHMENT_COLOR_ENCODING_EXT:
> > +                if (IsExtensionEnabled(EXT_sRGB)) {
> > +                    const GLenum internalFormat = fba.Renderbuffer()->InternalFormat();
> 
> The fba can be either a renderbuffer or a texture. Renderbuffer() can be
> null.

At line 1327: if (fba.Renderbuffer()) {

So, I was hoping that fba is a Renderbuffer and not NULL and the point I access InternalFormat().
   
> @@ +3703,5 @@
> > +    // GL requires:
> > +    //      format  ->  internalformat
> > +    //      GL_RGB      GL_SRGB_EXT
> > +    //      GL_RGBA     GL_SRGB_ALPHA_EXT
> > +    if (IsExtensionEnabled(EXT_sRGB) && !gl->IsGLES2()) {
> 
> We don't need to check for the extension here, since our data's already
> validated.
> 
> ::: gfx/gl/GLDefs.h
> @@ +46,5 @@
> > +// EXT_sRGB
> > +#define LOCAL_GL_SRGB_EXT                               0x8C40
> > +#define LOCAL_GL_SRGB_ALPHA_EXT                         0x8C42
> > +#define LOCAL_GL_SRGB8_ALPHA8_EXT                       0x8C43
> > +#define LOCAL_GL_FRAMEBUFFER_ATTACHMENT_COLOR_ENCODING_EXT  0x8210
> 
> We already should have all these in GLConsts.h.
Comment on attachment 810156 [details] [diff] [review]
mplementation of WebGL EXT_sRGB - https://www.khronos.org/registry/webgl/extensions/EXT_sRGB/

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

Missed this.

::: gfx/gl/GLContextFeatures.cpp
@@ +249,5 @@
>      },
>      {
> +        "sRGB_textures",
> +        210, // OpenGL version
> +        300, // OpenGL version

This second one is the GLES version.
(In reply to Jeff Gilbert [:jgilbert] from comment #8)
> Comment on attachment 810156 [details] [diff] [review]
> mplementation of WebGL EXT_sRGB -
> https://www.khronos.org/registry/webgl/extensions/EXT_sRGB/
> 
> Review of attachment 810156 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Missed this.
> 
> ::: gfx/gl/GLContextFeatures.cpp
> @@ +249,5 @@
> >      },
> >      {
> > +        "sRGB_textures",
> > +        210, // OpenGL version
> > +        300, // OpenGL version
> 
> This second one is the GLES version.

So I'm going to fix the comment up and change the OpenGL version to 3, which as we discussed has sRGB framebuffer support and add the extra extension checks for extensions that support sRGB framebuffers in lower versions.
(In reply to Jeff Gilbert [:jgilbert] from comment #8)
> > +        "sRGB_textures",
> > +        210, // OpenGL version
> > +        300, // OpenGL version

On second thought, I think what my original intention was to have a separate feature "sRGB_framebuffer" to check for support ARB_framebuffer_sRGB because of the way the feature checking code works.

Do you think is the correct approach?
Flags: needinfo?(jgilbert)
Attachment #810156 - Flags: review?(bjacob)
Discussed with Vlad and Benoit on IRC: To support WebGL EXT_sRGB we'll check for GL 3.0, GL ES 3.0, or GL ES EXT_sRGB and if none of those are supported then no sRGB. Vlad said this covers the hardware that is interesting. (Mac and Linux have version 3.0).  I'll change the name of the feature check.
Flags: needinfo?(jgilbert)
Attachment #810156 - Attachment is obsolete: true
I've rewritten the patch, split it into smaller chunks with better descriptions and written a conformance test.
Note: this just got mildly urgent, as Epic really, really wants sRGB textures.
Attachment #814285 - Attachment description: Conformance test for WebGL EXT_sRGB. → 1-Conformance test for WebGL EXT_sRGB.
Attachment #814285 - Attachment filename: Conformance-test-for-WebGL-EXTsRGB.patch → 1-Conformance-test-for-WebGL-EXTsRGB.patch
Attachment #814286 - Attachment filename: Feature-and-extension-checks-for-sRGB-texture-and-.patch → 2-Feature-and-extension-checks-for-sRGB-texture-and-.patch
Attachment #814286 - Attachment description: Feature and extension checks for sRGB texture and framebuffer support. → 2-Feature and extension checks for sRGB texture and framebuffer support.
Attachment #814287 - Attachment description: Added WebGL extension for EXT_sRGB. → 3-Added WebGL extension for EXT_sRGB.
Attachment #814287 - Attachment filename: Added-WebGL-extension-for-EXTsRGB.patch → 3-Added-WebGL-extension-for-EXTsRGB.patch
Attachment #814288 - Attachment description: Extract texture format checking into ValidateTexImage2DFormat(). → 4-Extract texture format checking into ValidateTexImage2DFormat().
Attachment #814288 - Attachment filename: Extract-texture-format-checking-into-ValidateTexIm.patch → 4-Extract-texture-format-checking-into-ValidateTexIm.patch
Attachment #814289 - Attachment description: Add handling of GL_SRGB_EXT and GL_SRGB_ALPHA_EXT texture formats. → 5-Add handling of GL_SRGB_EXT and GL_SRGB_ALPHA_EXT texture formats.
Attachment #814289 - Attachment filename: Add-handling-of-GLSRGBEXT-and-GLSRGBALPHAEXT-textu.patch → 5-Add-handling-of-GLSRGBEXT-and-GLSRGBALPHAEXT-textu.patch
Attachment #814290 - Attachment description: Add handling for renderbuffer storage format GL_SRGB8_ALPHA8_EXT. → 6-Add handling for renderbuffer storage format GL_SRGB8_ALPHA8_EXT.
Attachment #814290 - Attachment filename: Add-handling-for-renderbuffer-storage-format-GLSRG.patch → 6-Add-handling-for-renderbuffer-storage-format-GLSRG.patch
Attachment #814291 - Attachment description: Update framebuffer completeness rules to support sRGB formats. → 7-Update framebuffer completeness rules to support sRGB formats.
Attachment #814291 - Attachment filename: Update-framebuffer-completeness-rules-to-support-s.patch → 7-Update-framebuffer-completeness-rules-to-support-s.patch
Attachment #814292 - Attachment description: Update GetFramebufferAttachmentParameter() to support pname GL_FRAMEBUFFER_ATTACHMENT_COLOR_ENCODING_EXT. → 9-Update GetFramebufferAttachmentParameter() to support pname GL_FRAMEBUFFER_ATTACHMENT_COLOR_ENCODING_EXT.
Attachment #814292 - Attachment filename: Update-GetFramebufferAttachmentParameter-to-suppor.patch → 9-Update-GetFramebufferAttachmentParameter-to-suppor.patch
Attachment #814292 - Attachment description: 9-Update GetFramebufferAttachmentParameter() to support pname GL_FRAMEBUFFER_ATTACHMENT_COLOR_ENCODING_EXT. → 8-Update GetFramebufferAttachmentParameter() to support pname GL_FRAMEBUFFER_ATTACHMENT_COLOR_ENCODING_EXT.
Attachment #814292 - Attachment filename: 9-Update-GetFramebufferAttachmentParameter-to-suppor.patch → 8-Update-GetFramebufferAttachmentParameter-to-suppor.patch
Attachment #814293 - Attachment description: Querying the texture format was returning the wrong type for desktop OpenGL sRGB textures. All other uses of SetImageInfo() take internalformat, not format as parameter, so updating this one to be the same, in the process, fixing the issue. → 9-Querying the texture format was returning the wrong type for desktop OpenGL sRGB textures. All other uses of SetImageInfo() take internalformat, not format as parameter, so updating this one to be the same, in the process, fixing the issue.
Attachment #814293 - Attachment filename: Querying-the-texture-format-was-returning-the-wron.patch → 9-Querying-the-texture-format-was-returning-the-wron.patch
Attachment #814285 - Flags: review?(jgilbert)
Attachment #814286 - Flags: review?(jgilbert)
Attachment #814287 - Flags: review?(jgilbert)
Attachment #814288 - Flags: review?(jgilbert)
Attachment #814289 - Flags: review?(jgilbert)
Attachment #814290 - Flags: review?(jgilbert)
Attachment #814291 - Flags: review?(jgilbert)
Attachment #814292 - Flags: review?(jgilbert)
Attachment #814293 - Flags: review?(jgilbert)
Comment on attachment 814286 [details] [diff] [review]
2-Feature and extension checks for sRGB texture and framebuffer support.

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

::: gfx/gl/GLContextFeatures.cpp
@@ +401,5 @@
> +    // For sRGB support under OpenGL to match OpenGL ES spec, check for both
> +    // EXT_texture_sRGB and EXT_framebuffer_sRGB is required.
> +    if (IsExtensionSupported(EXT_texture_sRGB) &&
> +        (IsExtensionSupported(ARB_framebuffer_sRGB) || IsExtensionSupported(EXT_framebuffer_sRGB))) {
> +        mAvailableFeatures[GLFeature::sRGB] = true;

If we don't have one, we should really have a MarkFeatureSupported, instead of reaching into the guts and doing it ourselves. Can be done in another bug.
Attachment #814286 - Flags: review?(jgilbert) → review+
Comment on attachment 814287 [details] [diff] [review]
3-Added WebGL extension for EXT_sRGB.

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

::: content/canvas/src/WebGLContextState.cpp
@@ +538,5 @@
>          case LOCAL_GL_STENCIL_TEST:
>              return true;
>          case LOCAL_GL_RASTERIZER_DISCARD:
>              return IsWebGL2();
> +        case LOCAL_GL_FRAMEBUFFER_SRGB_EXT:

This is not exposed by the WebGL extension. (It's considered always-enabled in GLES)

::: content/canvas/src/WebGLExtensionSRGB.cpp
@@ +18,5 @@
> +    gl::GLContext* gl = context->GL();
> +    if (!gl->IsGLES()) {
> +        // Desktop OpenGL requires the following to be enabled to support
> +        // sRGB operations on framebuffers
> +        context->Enable(LOCAL_GL_FRAMEBUFFER_SRGB_EXT);

You'll need to call `gl->fEnable` directly, since WebGL should refuse to process this 'unknown' enum.
When doing this, you'll need to add a gl->MakeCurrent() call, as well.
Attachment #814287 - Flags: review?(jgilbert) → review-
Attachment #814288 - Flags: review?(jgilbert) → review+
Attachment #814289 - Flags: review?(jgilbert) → review+
Comment on attachment 814290 [details] [diff] [review]
6-Add handling for renderbuffer storage format GL_SRGB8_ALPHA8_EXT.

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

IIRC, you'll also need to add this format to the MemoryUsage code.
Attachment #814290 - Flags: review?(jgilbert) → review+
Attachment #814291 - Flags: review?(jgilbert) → review+
Attachment #814292 - Flags: review?(jgilbert) → review+
(In reply to Jeff Gilbert [:jgilbert] from comment #26)
> Comment on attachment 814290 [details] [diff] [review]
> 6-Add handling for renderbuffer storage format GL_SRGB8_ALPHA8_EXT.
> 
> Review of attachment 814290 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> IIRC, you'll also need to add this format to the MemoryUsage code.

And to test this, go to about:memory. In a debug build, so that you run into any assertion if something remains to be implemented.
Attachment #814293 - Flags: review?(jgilbert) → review+
Comment on attachment 814285 [details] [diff] [review]
1-Conformance test for WebGL EXT_sRGB.

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

::: content/canvas/test/webgl/conformance/extensions/ext-sRGB.html
@@ +27,5 @@
> +    gl_FragColor = vec4(uColor, uColor, uColor, 1);
> +}
> +</script>
> +
> +<script>

Please `"use strict";`, and end all expression lines with semicolons.

@@ +49,5 @@
> +
> +function readLocation(x, y) {
> +  var pixel = new Uint8Array(1 * 1 * 4)
> +  var px = Math.floor(x * canvas.width)
> +  var py = Math.floor(y * canvas.height)

canvas.drawingBufferWidth/Height

@@ +54,5 @@
> +  gl.readPixels(px, py, 1, 1, gl.RGBA, gl.UNSIGNED_BYTE, pixel)
> +  return pixel;
> +}
> +
> +function toString(v) {

function toVec3String(val) {
  if (typeof(val) == 'number')
    return toVec3String([val, val, val]);
  return val.join(', ');
}

@@ +113,5 @@
> +  return tex
> +}
> +
> +function testValidFormat(internalFormat, formatName, fn) {
> +  fn(internalFormat, internalFormat);

It looks like this should only supply the arg once. What way is this called where it needs two args?

Also, it would be better to arrange the args as (fn, internalFormat, formatName).

@@ +269,5 @@
> +  debug("")
> +  debug("Test the conversion of colors from sRGB to linear on texture read")
> +
> +  canvas.width = 4; canvas.height = 4
> +  gl.viewport(0, 0, canvas.width, canvas.height)

These two lines are incompatible on devices where CSS Pixels are bigger than Device Pixels, such as retina displays, and maybe some phones.

You need to set the size with canvas.width/height, but glViewport should take gl.drawingBufferWidth/Height. (Otherwise you'll only draw into the bottom-left corner of the viewport. (IIRC))

@@ +349,5 @@
> +    glErrorShouldBe(gl, gl.NO_ERROR)
> +
> +    var fbo = gl.createFramebuffer()
> +    gl.bindFramebuffer(gl.FRAMEBUFFER, fbo)
> +    glErrorShouldBe(gl, gl.NO_ERROR)

Shouldn't need to check for an error here.

@@ +378,5 @@
> +    gl.uniform1f(gl.getUniformLocation(program, "uColor"), conversions[ii][0]/255.0)
> +    wtu.drawQuad(gl, [0, 0, 0, 0]);
> +    expectResult(conversions[ii][1],
> +                 "framebuffer (renderbuffer) read returned the correct data",
> +                 "framebuffer (renderbuffer)  read returned incorrect data");

Extra space on this line between ')' and 'read'.
Attachment #814285 - Flags: review?(jgilbert) → review-
This also won't pass conformance yet on desktop, as desktop doesn't support sRGB renderbuffers. We need to add emulation support for them, where we masquerade a GL sRGBA texture as a WebGL sRGBA renderbuffer
(In reply to Jeff Gilbert [:jgilbert] from comment #29)
> This also won't pass conformance yet on desktop, as desktop doesn't support
> sRGB renderbuffers. We need to add emulation support for them, where we
> masquerade a GL sRGBA texture as a WebGL sRGBA renderbuffer

I was confused about this, started looking into doing the masquerading of renderbuffers with textures, and so I asked Benoit for advice. He pointed me to http://www.opengl.org/discussion_boards/showthread.php/175112-sRGB-Renderbuffers-in-GL2-1, and when I tested on MacOSX, GL_SRGB8_ALPHA8_EXT is accepted.

I found reference to ATi drivers being buggy and not supporting GL_ARGB8_ALPHA8_EXT correctly from 2011 (https://www.opengl.org/discussion_boards/showthread.php/175826-srgb-Renderbuffer)
Nice, yep, I didn't think to check that. Looks like we get the renderbuffer formats 'for free'.
(In reply to Jeff Gilbert [:jgilbert] from comment #25)
> Comment on attachment 814287 [details] [diff] [review]
> 3-Added WebGL extension for EXT_sRGB.
> 
> Review of attachment 814287 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/canvas/src/WebGLContextState.cpp
> @@ +538,5 @@
> >          case LOCAL_GL_STENCIL_TEST:
> >              return true;
> >          case LOCAL_GL_RASTERIZER_DISCARD:
> >              return IsWebGL2();
> > +        case LOCAL_GL_FRAMEBUFFER_SRGB_EXT:
> 
> This is not exposed by the WebGL extension. (It's considered always-enabled
> in GLES)
> 
> ::: content/canvas/src/WebGLExtensionSRGB.cpp
> @@ +18,5 @@
> > +    gl::GLContext* gl = context->GL();
> > +    if (!gl->IsGLES()) {
> > +        // Desktop OpenGL requires the following to be enabled to support
> > +        // sRGB operations on framebuffers
> > +        context->Enable(LOCAL_GL_FRAMEBUFFER_SRGB_EXT);
> 
> You'll need to call `gl->fEnable` directly, since WebGL should refuse to
> process this 'unknown' enum.
> When doing this, you'll need to add a gl->MakeCurrent() call, as well.

I think I misinterpreted what WebGLContextState and Enable is for. Should I remove this patch and call directly into the desktop  GL to enable the correct behaviour?
(In reply to Dan Glastonbury :djg :kamidphish from comment #32)
> (In reply to Jeff Gilbert [:jgilbert] from comment #25)
> > Comment on attachment 814287 [details] [diff] [review]
> > 3-Added WebGL extension for EXT_sRGB.
> > 
> > Review of attachment 814287 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: content/canvas/src/WebGLContextState.cpp
> > @@ +538,5 @@
> > >          case LOCAL_GL_STENCIL_TEST:
> > >              return true;
> > >          case LOCAL_GL_RASTERIZER_DISCARD:
> > >              return IsWebGL2();
> > > +        case LOCAL_GL_FRAMEBUFFER_SRGB_EXT:
> > 
> > This is not exposed by the WebGL extension. (It's considered always-enabled
> > in GLES)
> > 
> > ::: content/canvas/src/WebGLExtensionSRGB.cpp
> > @@ +18,5 @@
> > > +    gl::GLContext* gl = context->GL();
> > > +    if (!gl->IsGLES()) {
> > > +        // Desktop OpenGL requires the following to be enabled to support
> > > +        // sRGB operations on framebuffers
> > > +        context->Enable(LOCAL_GL_FRAMEBUFFER_SRGB_EXT);
> > 
> > You'll need to call `gl->fEnable` directly, since WebGL should refuse to
> > process this 'unknown' enum.
> > When doing this, you'll need to add a gl->MakeCurrent() call, as well.
> 
> I think I misinterpreted what WebGLContextState and Enable is for. Should I
> remove this patch and call directly into the desktop  GL to enable the
> correct behaviour?

The part of the patch in WebGLExtensionSRGB is good, but only needs to call into GL directly. We should just not be adding GL_FRAMEBUFFER_SRGB_EXT to WebGL's list of accepted Enable enums.
(In reply to Jeff Gilbert [:jgilbert] from comment #28) 
> function toVec3String(val) {
>   if (typeof(val) == 'number')
>     return toVec3String([val, val, val]);
>   return val.join(', ');
> }

join() doesn't work because val is a Uint8Array not an Array. So I replaced it with '[' + val[0] + ', ' + val[1] + ', ' + val[2] + ']'

> You need to set the size with canvas.width/height, but glViewport should
> take gl.drawingBufferWidth/Height. (Otherwise you'll only draw into the
> bottom-left corner of the viewport. (IIRC))
 
I fixed the tests to set up the canvas and viewport once and then refer to gl.drawingBufferWidth/Height instead of using magic values.
Update MemoryUsage() to handle the sRGB internal format sizes.
Attachment #816166 - Flags: review?(jgilbert)
Updated with :jgilbert review comments. Refactored code to remove some of the "cut-n-paste from other examples".
Attachment #814285 - Attachment is obsolete: true
Attachment #816167 - Flags: review?(jgilbert)
Attachment #814287 - Attachment is obsolete: true
Attachment #816168 - Flags: review?(jgilbert)
Comment on attachment 816167 [details] [diff] [review]
1-Conformance-test-for-WebGL-EXT_sRGB.patch

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

Fix this to be 2-space indents.

::: content/canvas/test/webgl/conformance/extensions/ext-sRGB.html
@@ +115,5 @@
> +
> +function testValidFormat(fn, internalFormat, formatName) {
> +  fn(internalFormat);
> +  glErrorShouldBe(gl, gl.NO_ERROR,
> +            "was able to create type " + formatName);

Align overflowed arguments.

@@ +122,5 @@
> +function testInvalidFormat(fn, internalFormat, formatName) {
> +  fn(internalFormat);
> +  var err = gl.getError();
> +  if (err == gl.NO_ERROR) {
> +      testFailed("should NOT be able to create type " + formatName);

Pick either 2-space or 4-space indents. :)

@@ +211,5 @@
> +    testPassed("Successfully enabled EXT_sRGB extension");
> +
> +    runSupportedTest(true);
> +
> +    setCanvasAndViewportSize(canvas, gl, 16, 16);

Do we really need to resize the canvas?
Attachment #816167 - Flags: review?(jgilbert) → review+
Attachment #816168 - Flags: review?(jgilbert) → review+
Attachment #816166 - Flags: review?(jgilbert) → review+
(In reply to Jeff Gilbert [:jgilbert] from comment #38)
> Comment on attachment 816167 [details] [diff] [review]
> 1-Conformance-test-for-WebGL-EXT_sRGB.patch
> 
> Review of attachment 816167 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Fix this to be 2-space indents.

That's what you get when you cut-n-paste from various samples. ;-)

> @@ +211,5 @@
> > +    setCanvasAndViewportSize(canvas, gl, 16, 16);
> 
> Do we really need to resize the canvas?

Probably not. I'll remove and test.
Attached patch 0001-WebGL-EXT_sRGB.patch (obsolete) — Splinter Review
Carry r=jgilbert
Attachment #814286 - Attachment is obsolete: true
Attachment #814288 - Attachment is obsolete: true
Attachment #814289 - Attachment is obsolete: true
Attachment #814290 - Attachment is obsolete: true
Attachment #814291 - Attachment is obsolete: true
Attachment #814292 - Attachment is obsolete: true
Attachment #814293 - Attachment is obsolete: true
Attachment #816166 - Attachment is obsolete: true
Attachment #816167 - Attachment is obsolete: true
Attachment #816168 - Attachment is obsolete: true
Attachment #817622 - Flags: review+
Comment on attachment 817622 [details] [diff] [review]
0001-WebGL-EXT_sRGB.patch

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

::: content/canvas/src/WebGLContextGL.cpp
@@ +1440,5 @@
>          switch (pname) {
> +             case LOCAL_GL_FRAMEBUFFER_ATTACHMENT_COLOR_ENCODING_EXT:
> +                if (IsExtensionEnabled(EXT_sRGB)) {
> +                    const GLenum internalFormat =
> +                        fba.Texture()->ImageInfoAt(LOCAL_GL_TEXTURE_2D, 0).Format();

It looks like you want ImageInfoBase() here. This will fall down when the texture attachment is a cubemap.
Since the class stores the internal format and not the format in all cases, 
rename WebGLTexture::ImageInfo::mFormat to WebGLTexture::ImageInfo::mInternalFormat and update Format() to InternalFormat()
Attachment #821582 - Flags: review?(jgilbert)
Attachment #821644 - Flags: review?(jgilbert)
Attachment #821582 - Attachment is patch: true
Attachment #821582 - Attachment mime type: message/rfc822 → text/plain
Comment on attachment 821644 [details] [diff] [review]
0003-IsValidAttachedTextureColorFormat

FrameBuffer::Attachment::IsComplete was being a little lazy in using the format to check for valid color attachment format. The change to storing the internal format necessitated updating the check to be explicit. Extracted the logic into 
IsValidAttachedTextureColorFormat().
Comment on attachment 821582 [details] [diff] [review]
0002-rename-imageinfo-format-to-internalformat

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

I think this is fine and necessary. Any concerns, bjacob? This basically converts ImageInfo's `format` to `internalFormat`, such that it now has `internalFormat` and `type`, which is a little weird, but probably fine. (Background: We need at least `internalFormat`, but I think we probably don't *also* need `format` if we have `internalFormat`)

Does this check out for you?
Attachment #821582 - Flags: review?(jgilbert)
Attachment #821582 - Flags: review?(bjacob)
Attachment #821582 - Flags: review+
Comment on attachment 821644 [details] [diff] [review]
0003-IsValidAttachedTextureColorFormat

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

::: content/canvas/src/WebGLFramebuffer.cpp
@@ +107,5 @@
>  
> +static inline bool
> +IsValidAttachedTextureColorFormat(GLenum format) {
> +    return (
> +        /* 32 BPP linear formats */

Not really 32bpp so much as 8-bit channels. RGB is only (required to be) 24bpp. (even if it's probably RGBX8888)

Same issue with the other two spots below. Probably best just as:
linear 8-bit formats
sRGB 8-bit formats
linear float32 formats
Attachment #821644 - Flags: review?(jgilbert) → review+
(In reply to Jeff Gilbert [:jgilbert] from comment #48)
> Same issue with the other two spots below. Probably best just as:
> linear 8-bit formats
> sRGB 8-bit formats
> linear float32 formats

Updated.
Attached patch 0004-Blacklist-MacOSX-10.6 (obsolete) — Splinter Review
MacOSX 10.6 has a failure when reading back the results from an sRGB format
texture attached to an FBO. Blacklist MacOSX 10.6 and all lower versions.
Attachment #824439 - Flags: review?(jgilbert)
Comment on attachment 824439 [details] [diff] [review]
0004-Blacklist-MacOSX-10.6

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

::: gfx/gl/GLContext.h
@@ +2452,5 @@
>          MOZ_CRASH("Must be called against a GLContextEGL.");
>      }
>  
>      bool CanUploadSubTextures();
> +    bool CanReadsRGBFromFBOTexture();

Let's use 'SRGB' for CamelCasing. It looks like 'CanReadRGB...' with the lowercase 's'.
Attachment #824439 - Flags: review?(jgilbert) → review+
Comment on attachment 821582 [details] [diff] [review]
0002-rename-imageinfo-format-to-internalformat

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

Sure, that is fine.
Attachment #821582 - Flags: review?(bjacob) → review+
Carry r=jgilbert,bjacob

Roll up of all previous patches.
Attachment #817622 - Attachment is obsolete: true
Attachment #821582 - Attachment is obsolete: true
Attachment #821644 - Attachment is obsolete: true
Attachment #824439 - Attachment is obsolete: true
Attachment #826535 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/e77d96c4bfee
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Blocks: 958491
Blocks: 1144889
You need to log in before you can comment on or make changes to this bug.