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.

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.

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.

Fixed the order of the diff.

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.

(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?

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.

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.

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.

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.

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.

(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.

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'.

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.

Updated with :jgilbert review comments. Refactored code to remove some of the "cut-n-paste from other examples".

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?

(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.

Carry r=jgilbert

Backed out for failures of form: https://tbpl.mozilla.org/php/getParsedLog.php?id=29189284&tree=Mozilla-Inbound remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/3f2c42551ac6

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()

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?

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

(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.

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.

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'.

Comment on attachment 821582 [details] [diff] [review] 0002-rename-imageinfo-format-to-internalformat Review of attachment 821582 [details] [diff] [review]: ----------------------------------------------------------------- Sure, that is fine.

Carry r=jgilbert,bjacob Roll up of all previous patches.