843668 - Implement WebGL EXT_sRGB

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, enhancement)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

http://www.khronos.org/registry/webgl/extensions/EXT_sRGB

Comment 1

[u480271]

Attachment: fix-843668.patch

Comment 2

[u480271]

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.

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

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 4

[Benoit Jacob [:bjacob] (mostly away)]

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.

Comment 5

[u480271]

Attachment: mplementation of WebGL EXT_sRGB - https://www.khronos.org/registry/webgl/extensions/EXT_sRGB/

Fixed the order of the diff.

Comment 6

[Kelsey Gilbert [:jgilbert]]

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.

Comment 7

[u480271]

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

[Kelsey Gilbert [:jgilbert]]

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.

Comment 9

[u480271]

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

Comment 10

[u480271]

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

Comment 11

[u480271]

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.

Comment 12

[u480271]

Attachment: 1-Conformance test for WebGL EXT_sRGB.

Comment 13

[u480271]

Attachment: 2-Feature and extension checks for sRGB texture and framebuffer support.

Comment 14

[u480271]

Attachment: 3-Added WebGL extension for EXT_sRGB.

Comment 15

[u480271]

Attachment: 4-Extract texture format checking into ValidateTexImage2DFormat().

Comment 16

[u480271]

Attachment: 5-Add handling of GL_SRGB_EXT and GL_SRGB_ALPHA_EXT texture formats.

Comment 17

[u480271]

Attachment: 6-Add handling for renderbuffer storage format GL_SRGB8_ALPHA8_EXT.

Comment 18

[u480271]

Attachment: 7-Update framebuffer completeness rules to support sRGB formats.

Comment 19

[u480271]

Attachment: 8-Update GetFramebufferAttachmentParameter() to support pname GL_FRAMEBUFFER_ATTACHMENT_COLOR_ENCODING_EXT.

Comment 20

[u480271]

Attachment: 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.

Comment 21

[u480271]

I've rewritten the patch, split it into smaller chunks with better descriptions and written a conformance test.

Comment 22

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Note: this just got mildly urgent, as Epic really, really wants sRGB textures.

Comment 23

[u480271]

https://tbpl.mozilla.org/?tree=Try&rev=7ee8208fce4a

Comment 24

[Kelsey Gilbert [: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.

Comment 25

[Kelsey Gilbert [:jgilbert]]

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 26

[Kelsey Gilbert [:jgilbert]]

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.

Comment 27

[Benoit Jacob [:bjacob] (mostly away)]

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

[Kelsey Gilbert [:jgilbert]]

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

Comment 29

[Kelsey Gilbert [:jgilbert]]

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

Comment 30

[u480271]

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

Comment 31

[Kelsey Gilbert [:jgilbert]]

Nice, yep, I didn't think to check that. Looks like we get the renderbuffer formats 'for free'.

Comment 32

[u480271]

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

Comment 33

[Kelsey Gilbert [:jgilbert]]

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

Comment 34

[u480271]

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

Comment 35

[u480271]

Attachment: 10-Update-MemoryUsage-calculation.patch

Update MemoryUsage() to handle the sRGB internal format sizes.

Comment 36

[u480271]

Attachment: 1-Conformance-test-for-WebGL-EXT_sRGB.patch

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

Comment 37

[u480271]

Attachment: 3-Added-WebGL-extension-for-EXT_sRGB.patch

Comment 38

[Kelsey Gilbert [: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?

Comment 39

[u480271]

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

Comment 40

[u480271]

Attachment: 0001-WebGL-EXT_sRGB.patch

Carry r=jgilbert

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f8e64e91e443

Comment 42

[Ed Morley [:emorley]]

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 43

[Kelsey Gilbert [:jgilbert]]

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.

Comment 44

[u480271]

Attachment: 0002-rename-imageinfo-format-to-internalformat

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 45

[u480271]

Attachment: 0003-IsValidAttachedTextureColorFormat

Comment 46

[u480271]

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 47

[Kelsey Gilbert [:jgilbert]]

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 48

[Kelsey Gilbert [:jgilbert]]

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

Comment 49

[u480271]

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

Comment 50

[u480271]

Attachment: 0004-Blacklist-MacOSX-10.6

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 51

[u480271]

https://tbpl.mozilla.org/?tree=Try&rev=18caba7cc2ee

Comment 52

[Kelsey Gilbert [: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'.

Comment 53

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 821582 [details] [diff] [review]
0002-rename-imageinfo-format-to-internalformat

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

Sure, that is fine.

Comment 54

[u480271]

Attachment: 0001-Conformance-test-for-WebGL-EXT_sRGB.patch

Carry r=jgilbert,bjacob

Roll up of all previous patches.

Comment 55

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e77d96c4bfee

Comment 56

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e77d96c4bfee