Bug 109332

Summary: EXT_sRGB needs implementation
Product: WebKit Reporter: Remi Arnaud <Remi.Arnaud>
Component: WebGLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bajones, benjamin, buildbot, cmarrin, commit-queue, dino, gman, jonlee, kbr, pyalot, rniwa, roger_fong, webkit-bug-importer, zmo
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: http://www.khronos.org/registry/webgl/extensions/EXT_sRGB/
Bug Depends on:    
Bug Blocks: 152876    
Attachments:
Description Flags
patch
none
patch
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion
none
patch w/test fix
none
patch w/test fix
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion
none
patch
none
patch
dino: review+
patch2 none

Description Remi Arnaud 2013-02-08 16:10:14 PST
EXT_sRGB (see URL) is in Draft stage, and is awaiting implementation to enable the community to review the extension.

EXT_sRGB format is used to enable better quality high performance renderers. Developers are looking forward to this extension in order to enable them to provide rendering quality on par with what the device can do with native applications.
Comment 1 Florian Bösch 2014-06-08 22:45:46 PDT
Also missing on iOS8, recently tested: http://codeflow.org/entries/2014/jun/08/some-issues-with-apples-ios-webgl-implementation/
Comment 2 Florian Bösch 2014-06-18 00:09:20 PDT
Firefox now implements EXT_sRGB unprefixed on OSX and Linux.
Comment 3 Florian Bösch 2014-06-18 00:14:35 PDT
Chrome ticket: https://code.google.com/p/chromium/issues/detail?id=386048
Comment 4 Florian Bösch 2014-06-18 10:54:20 PDT
The EXT_sRGB extension just moved out of draft into community approved.
Comment 5 Radar WebKit Bug Importer 2014-06-18 11:03:39 PDT
<rdar://problem/17363470>
Comment 6 Florian Bösch 2014-07-04 04:02:30 PDT
The EXT_sRGB extension was moved from draft to community approved.
Comment 7 Roger Fong 2014-12-04 17:37:31 PST
I have  patch which exposes the implementation.
ext-sRGB.html passes.

A few things to note:
The test assumes

a) Extensions are only enabled after a call to getExtension() is made on the extension.
This is not consistent with how the rest of WebKit's WebGL implementation treats WebGL extensions, which simply determines enabled-ness by looking at what the underlying GL implementation supports.

b) The test makes a point that the EXT_SRGB format is not valid for frame buffer attachments. Checking frame buffer status on a frame buffer with an attachment that has an internal format of EXT_SRGB is expected to yield an INCOMPLETE FRAMEBUFFER STATUS.
I've made this work appropriately, however, I cannot find in the specs anywhere why this has to be the case...
Comment 8 Roger Fong 2014-12-04 17:54:11 PST
Created attachment 242605 [details]
patch

Ignore the style failures, they're wrong in this case.
Comment 9 Roger Fong 2014-12-04 18:53:51 PST
Created attachment 242607 [details]
patch
Comment 10 WebKit Commit Bot 2014-12-04 18:54:53 PST
Attachment 242607 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/Extensions3D.h:94:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/Extensions3D.h:95:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/Extensions3D.h:96:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/Extensions3D.h:97:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 4 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Build Bot 2014-12-05 00:32:48 PST
Comment on attachment 242607 [details]
patch

Attachment 242607 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5492950268116992

New failing tests:
fast/canvas/webgl/constants.html
Comment 12 Build Bot 2014-12-05 00:32:53 PST
Created attachment 242621 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 13 Florian Bösch 2014-12-05 00:49:51 PST
(In reply to comment #7)
> a) Extensions are only enabled after a call to getExtension() is made on the
> extension.
> This is not consistent with how the rest of WebKit's WebGL implementation
> treats WebGL extensions, which simply determines enabled-ness by looking at
> what the underlying GL implementation supports.

That's incorrect. A WebGL extension is only enabled and ready if getExtension has successfully completed on that WebGL context.

> b) The test makes a point that the EXT_SRGB format is not valid for frame
> buffer attachments. Checking frame buffer status on a frame buffer with an
> attachment that has an internal format of EXT_SRGB is expected to yield an
> INCOMPLETE FRAMEBUFFER STATUS.
> I've made this work appropriately, however, I cannot find in the specs
> anywhere why this has to be the case...

This is also incorrect. Attaching an sRGB format to a framebuffer is a valid and important use-case for sRGB.
Comment 14 Build Bot 2014-12-05 01:02:04 PST
Comment on attachment 242607 [details]
patch

Attachment 242607 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6516190054711296

New failing tests:
fast/canvas/webgl/constants.html
Comment 15 Build Bot 2014-12-05 01:02:12 PST
Created attachment 242622 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 16 Roger Fong 2014-12-05 11:35:29 PST
Hmm this builds fine for me on ToT. I'm pretty sure this patch requires a complete clean rebuild though since it generates new derived sources.(In reply to comment #13)
> (In reply to comment #7)
> > a) Extensions are only enabled after a call to getExtension() is made on the
> > extension.
> > This is not consistent with how the rest of WebKit's WebGL implementation
> > treats WebGL extensions, which simply determines enabled-ness by looking at
> > what the underlying GL implementation supports.
> 
> That's incorrect. A WebGL extension is only enabled and ready if
> getExtension has successfully completed on that WebGL context.
Okay, I've only been testing with 1.0.2 tests which don't seem to test this functionality as well as the 1.0.3 tests. I will keep this in mind when addressing 1.0.3 conformance in the future.

> 
> > b) The test makes a point that the EXT_SRGB format is not valid for frame
> > buffer attachments. Checking frame buffer status on a frame buffer with an
> > attachment that has an internal format of EXT_SRGB is expected to yield an
> > INCOMPLETE FRAMEBUFFER STATUS.
> > I've made this work appropriately, however, I cannot find in the specs
> > anywhere why this has to be the case...
> 
> This is also incorrect. Attaching an sRGB format to a framebuffer is a valid
> and important use-case for sRGB.
Looking at the test:
function runFramebufferTextureConversionTest(format) {
  var formatString;
  var validFormat;
  switch (format) {
    case ext.SRGB_EXT: formatString = "sRGB"; validFormat = false; break;
    case ext.SRGB_ALPHA_EXT: formatString = "sRGB_ALPHA"; validFormat = true; break;
    default: return null;
  }

And then later,
if (validFormat)
...
else
shouldBe("gl.checkFramebufferStatus(gl.FRAMEBUFFER)", "gl.FRAMEBUFFER_INCOMPLETE_ATTACHMENT");

Is the test wrong?
Comment 17 Roger Fong 2014-12-05 11:36:26 PST
Side note, this patch adds new derived sources so a build where all of WebCore's derived sources are deleted are required for a successful build (this is evident in the build failure).
Comment 18 Florian Bösch 2014-12-05 11:53:27 PST
> > > b) The test makes a point that the EXT_SRGB format is not valid for frame
> > > buffer attachments. Checking frame buffer status on a frame buffer with an
> > > attachment that has an internal format of EXT_SRGB is expected to yield an
> > > INCOMPLETE FRAMEBUFFER STATUS.
> > > I've made this work appropriately, however, I cannot find in the specs
> > > anywhere why this has to be the case...
> > 
> > This is also incorrect. Attaching an sRGB format to a framebuffer is a valid
> > and important use-case for sRGB.
> Looking at the test:
> function runFramebufferTextureConversionTest(format) {
>   var formatString;
>   var validFormat;
>   switch (format) {
>     case ext.SRGB_EXT: formatString = "sRGB"; validFormat = false; break;
>     case ext.SRGB_ALPHA_EXT: formatString = "sRGB_ALPHA"; validFormat =
> true; break;
>     default: return null;
>   }
> 
> And then later,
> if (validFormat)
> ...
> else
> shouldBe("gl.checkFramebufferStatus(gl.FRAMEBUFFER)",
> "gl.FRAMEBUFFER_INCOMPLETE_ATTACHMENT");
> 
> Is the test wrong?

The 1.0.3 test is correct as per https://www.khronos.org/registry/gles/extensions/EXT/EXT_sRGB.txt, here's how it works.

The only valid format for a renderbuffer to attach to a framebuffer is SRGB8_ALPHA8_EXT, the test reflects that here: https://github.com/KhronosGroup/WebGL/blob/master/conformance-suites/1.0.3/conformance/extensions/ext-sRGB.html#L178

The only valid formats for a texture are SRGB_EXT and SRGB_ALPHA_EXT, the test reflects this here: https://github.com/KhronosGroup/WebGL/blob/master/conformance-suites/1.0.3/conformance/extensions/ext-sRGB.html#L159

Attaching an sRGB texture to a framebuffer is a valid use, but this only applies to SRGB_ALPHA_EXT textures, not for SRGB_EXT textures, the test reflects this here: https://github.com/KhronosGroup/WebGL/blob/master/conformance-suites/1.0.3/conformance/extensions/ext-sRGB.html#L313
Comment 19 Roger Fong 2014-12-05 12:08:19 PST
> Attaching an sRGB texture to a framebuffer is a valid use, but this only
> applies to SRGB_ALPHA_EXT textures, not for SRGB_EXT textures, the test
> reflects this here:
> https://github.com/KhronosGroup/WebGL/blob/master/conformance-suites/1.0.3/
> conformance/extensions/ext-sRGB.html#L313

I see. I will update my patch to reflect this accordingly.
Thanks for the clarification!
Comment 20 Roger Fong 2014-12-05 12:44:47 PST
(In reply to comment #19)
> > Attaching an sRGB texture to a framebuffer is a valid use, but this only
> > applies to SRGB_ALPHA_EXT textures, not for SRGB_EXT textures, the test
> > reflects this here:
> > https://github.com/KhronosGroup/WebGL/blob/master/conformance-suites/1.0.3/
> > conformance/extensions/ext-sRGB.html#L313
> 
> I see. I will update my patch to reflect this accordingly.
> Thanks for the clarification!

Oh my original patch was actually correct.
I think the confusion here was because I said "The test makes a point that the EXT_SRGB format is not valid for frame buffer attachments."
I meant to say SRGB_EXT instead of EXT_SRGB...
Comment 21 Roger Fong 2014-12-05 13:13:49 PST
Created attachment 242653 [details]
patch w/test fix
Comment 22 WebKit Commit Bot 2014-12-05 13:15:22 PST
Attachment 242653 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/Extensions3D.h:94:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/Extensions3D.h:95:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/Extensions3D.h:96:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/Extensions3D.h:97:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 4 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 23 Roger Fong 2014-12-05 13:21:18 PST
Created attachment 242654 [details]
patch w/test fix
Comment 24 WebKit Commit Bot 2014-12-05 13:22:41 PST
Attachment 242654 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/Extensions3D.h:94:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/Extensions3D.h:95:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/Extensions3D.h:96:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/Extensions3D.h:97:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 4 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 25 Build Bot 2014-12-05 14:33:50 PST
Comment on attachment 242654 [details]
patch w/test fix

Attachment 242654 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5443287024402432

New failing tests:
fast/canvas/webgl/constants.html
Comment 26 Build Bot 2014-12-05 14:33:54 PST
Created attachment 242664 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 27 Build Bot 2014-12-05 18:37:44 PST
Comment on attachment 242654 [details]
patch w/test fix

Attachment 242654 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5818027752816640

New failing tests:
fast/canvas/webgl/constants.html
Comment 28 Build Bot 2014-12-05 18:37:47 PST
Created attachment 242694 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 29 Build Bot 2014-12-05 21:27:03 PST
Comment on attachment 242654 [details]
patch w/test fix

Attachment 242654 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5751299530293248

New failing tests:
fast/canvas/webgl/constants.html
Comment 30 Build Bot 2014-12-05 21:27:06 PST
Created attachment 242699 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 31 Roger Fong 2014-12-07 18:16:55 PST
Created attachment 242776 [details]
patch

Windows fix, test fix
Comment 32 WebKit Commit Bot 2014-12-07 18:31:12 PST
Attachment 242776 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/Extensions3D.h:94:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/Extensions3D.h:95:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/Extensions3D.h:96:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/Extensions3D.h:97:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 4 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 33 Roger Fong 2014-12-08 11:13:03 PST
Created attachment 242832 [details]
patch
Comment 34 WebKit Commit Bot 2014-12-08 11:14:44 PST
Attachment 242832 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/Extensions3D.h:94:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/Extensions3D.h:95:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/Extensions3D.h:96:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/Extensions3D.h:97:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 4 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 35 Dean Jackson 2014-12-08 11:32:32 PST
Comment on attachment 242832 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=242832&action=review

> Source/WebCore/WebCore.vcxproj/WebCore.vcxproj:14951
> -    <ClCompile Include="..\editing\win\EditorWin.cpp"/>
> +    <ClCompile Include="..\editing\win\EditorWin.cpp" />

Oops.

> Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters:7263
> +    <ClCompile Include="..\page\PageConfiguration.cpp" />
> +    <ClCompile Include="..\loader\cache\CacheValidation.cpp" />
> +    <ClCompile Include="..\html\HTMLWBRElement.cpp" />

???

> Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters:15271
> +    <ClInclude Include="..\loader\cache\CacheValidation.h" />
> +    <ClInclude Include="..\html\HTMLWBRElement.h" />
> +    <ClInclude Include="..\storage\StorageNamespaceProvider.h" />

???

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:15799
> +				5C4304AD191AC908000E2BC0 /* EXTShaderTextureLOD.cpp */,
> +				5C4304AE191AC908000E2BC0 /* EXTShaderTextureLOD.h */,
> +				5C4304AF191AC908000E2BC0 /* EXTShaderTextureLOD.idl */,

Was this intentional?

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:18734
> +				5C4304B3191AEF46000E2BC0 /* JSEXTShaderTextureLOD.cpp */,
> +				5C4304B4191AEF46000E2BC0 /* JSEXTShaderTextureLOD.h */,

And here?

> Source/WebCore/html/canvas/EXTsRGB.cpp:4
> +/*
> + * Copyright (C) 2012 Google Inc. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without

Did you really take this from Google's implementation? Or did you copy an existing file? If the latter, then the copyright should be Apple.

> Source/WebCore/html/canvas/EXTsRGB.h:2
> + * Copyright (C) 2012 Google Inc. All rights reserved.

Same here.

> Source/WebCore/html/canvas/EXTsRGB.idl:29
> +NoInterfaceObject,
> +Conditional=WEBGL,
> +GenerateIsReachable=ImplWebGLRenderingContext

We indent these lines. I don't know why.

> Source/WebCore/html/canvas/WebGLFramebuffer.cpp:436
> +        // Attaching an SRGB_EXT format attachment to a framebuffer is invalid

Nit: missing period.

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2631
> +        case Extensions3D::FRAMEBUFFER_ATTACHMENT_COLOR_ENCODING_EXT:
> +            {
> +                WebGLRenderbuffer* renderBuffer = reinterpret_cast<WebGLRenderbuffer*>(object);
> +                GC3Denum renderBufferFormat = renderBuffer->getInternalFormat();
> +                ASSERT(renderBufferFormat != Extensions3D::SRGB_EXT && renderBufferFormat != Extensions3D::SRGB_ALPHA_EXT);
> +                if (renderBufferFormat == Extensions3D::SRGB8_ALPHA8_EXT)
> +                    return WebGLGetInfo(Extensions3D::SRGB_EXT);
> +                return WebGLGetInfo(GraphicsContext3D::LINEAR);
> +            }

One level too much indentation here, and the opening { goes on the case line.

If renderBufferFormat is not allowed to be SRGB_ALPHA_EXT, why don't you check for that as well in WebGLFramebuffer.cpp?

> Source/WebCore/platform/graphics/opengl/Extensions3DOpenGL.cpp:174
> +    if (name == "GL_EXT_sRGB")
> +        return m_availableExtensions.contains("GL_EXT_texture_sRGB") && (m_availableExtensions.contains("GL_EXT_framebuffer_sRGB") || m_availableExtensions.contains("GL_ARB_framebuffer_sRGB"));
> +

Does this work on iOS?
Comment 36 Roger Fong 2014-12-08 11:41:07 PST
(In reply to comment #35)
> Comment on attachment 242832 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=242832&action=review
> 
> > Source/WebCore/WebCore.vcxproj/WebCore.vcxproj:14951
> > -    <ClCompile Include="..\editing\win\EditorWin.cpp"/>
> > +    <ClCompile Include="..\editing\win\EditorWin.cpp" />
> 
> Oops.
> 
> > Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters:7263
> > +    <ClCompile Include="..\page\PageConfiguration.cpp" />
> > +    <ClCompile Include="..\loader\cache\CacheValidation.cpp" />
> > +    <ClCompile Include="..\html\HTMLWBRElement.cpp" />
> 
> ???
> 
> > Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters:15271
> > +    <ClInclude Include="..\loader\cache\CacheValidation.h" />
> > +    <ClInclude Include="..\html\HTMLWBRElement.h" />
> > +    <ClInclude Include="..\storage\StorageNamespaceProvider.h" />
> 
> ???

Hmm, I think visual studio is doing some funky things.
> 
> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:15799
> > +				5C4304AD191AC908000E2BC0 /* EXTShaderTextureLOD.cpp */,
> > +				5C4304AE191AC908000E2BC0 /* EXTShaderTextureLOD.h */,
> > +				5C4304AF191AC908000E2BC0 /* EXTShaderTextureLOD.idl */,
> 
> Was this intentional?
> 
> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:18734
> > +				5C4304B3191AEF46000E2BC0 /* JSEXTShaderTextureLOD.cpp */,
> > +				5C4304B4191AEF46000E2BC0 /* JSEXTShaderTextureLOD.h */,
> 

And maybe Xcode too?...I'll remove those lines and see if things still work.
> And here?
> 
> > Source/WebCore/html/canvas/EXTsRGB.cpp:4
> > +/*
> > + * Copyright (C) 2012 Google Inc. All rights reserved.
> > + *
> > + * Redistribution and use in source and binary forms, with or without
> 
> Did you really take this from Google's implementation? Or did you copy an
> existing file? If the latter, then the copyright should be Apple.

I just copy pasted the header from a different file. Will switch.

> 
> > Source/WebCore/html/canvas/EXTsRGB.h:2
> > + * Copyright (C) 2012 Google Inc. All rights reserved.
> 
> Same here.
> 
> > Source/WebCore/html/canvas/EXTsRGB.idl:29
> > +NoInterfaceObject,
> > +Conditional=WEBGL,
> > +GenerateIsReachable=ImplWebGLRenderingContext
> 
> We indent these lines. I don't know why.

Ok

> 
> > Source/WebCore/html/canvas/WebGLFramebuffer.cpp:436
> > +        // Attaching an SRGB_EXT format attachment to a framebuffer is invalid
> 
> Nit: missing period.
> 
> > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:2631
> > +        case Extensions3D::FRAMEBUFFER_ATTACHMENT_COLOR_ENCODING_EXT:
> > +            {
> > +                WebGLRenderbuffer* renderBuffer = reinterpret_cast<WebGLRenderbuffer*>(object);
> > +                GC3Denum renderBufferFormat = renderBuffer->getInternalFormat();
> > +                ASSERT(renderBufferFormat != Extensions3D::SRGB_EXT && renderBufferFormat != Extensions3D::SRGB_ALPHA_EXT);
> > +                if (renderBufferFormat == Extensions3D::SRGB8_ALPHA8_EXT)
> > +                    return WebGLGetInfo(Extensions3D::SRGB_EXT);
> > +                return WebGLGetInfo(GraphicsContext3D::LINEAR);
> > +            }
> 
> One level too much indentation here, and the opening { goes on the case line.
> 
> If renderBufferFormat is not allowed to be SRGB_ALPHA_EXT, why don't you
> check for that as well in WebGLFramebuffer.cpp?

I don't think that a value of SRGB_ALPHA_ExT could ever get to this code path. But I suppose it couldn't hurt to check it in multiple places.

> 
> > Source/WebCore/platform/graphics/opengl/Extensions3DOpenGL.cpp:174
> > +    if (name == "GL_EXT_sRGB")
> > +        return m_availableExtensions.contains("GL_EXT_texture_sRGB") && (m_availableExtensions.contains("GL_EXT_framebuffer_sRGB") || m_availableExtensions.contains("GL_ARB_framebuffer_sRGB"));
> > +
> 
> Does this work on iOS?

I've completely neglected my iOS testing. Will do that.
Comment 37 Roger Fong 2014-12-08 11:42:52 PST
> > 
> > > Source/WebCore/html/canvas/EXTsRGB.cpp:4
> > > +/*
> > > + * Copyright (C) 2012 Google Inc. All rights reserved.
> > > + *
> > > + * Redistribution and use in source and binary forms, with or without
> > 
> > Did you really take this from Google's implementation? Or did you copy an
> > existing file? If the latter, then the copyright should be Apple.
> 
> I just copy pasted the header from a different file. Will switch.
> 

And by header and mean copyright header.
Comment 38 Roger Fong 2014-12-08 12:09:19 PST
> > If renderBufferFormat is not allowed to be SRGB_ALPHA_EXT, why don't you
> > check for that as well in WebGLFramebuffer.cpp?
> 
> I don't think that a value of SRGB_ALPHA_EXT could ever get to this code
> path. But I suppose it couldn't hurt to check it in multiple places.

Oh that's not the reason, the reason is that the only those formats are not valid for render buffers but for attachments to frame buffers SRGB_ALPHA_EXT is indeed valid. This is because SRGB_ALPHA_EXT can be the format for a texture attachments (as opposed to a render buffer attachment).

So to summarize:
RenderBuffers valid formats: SRGB8_ALPHA8_EXT
Textures valid formats: SRGB_EXT, SRGB_ALPHA_EXT

Valid formats for attachments to FrameBuffers: SRGB8_ALPHA8_EXT, SRGB_ALPHA_EXT
Comment 39 Roger Fong 2014-12-08 13:13:45 PST
Created attachment 242842 [details]
patch2

Test to make sure builds still okay after some project file changes
Comment 40 WebKit Commit Bot 2014-12-08 13:15:54 PST
Attachment 242842 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/Extensions3D.h:94:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/Extensions3D.h:95:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/Extensions3D.h:96:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/graphics/Extensions3D.h:97:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 4 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 41 Benjamin Poulain 2014-12-08 18:16:34 PST
Looks like it broke the iOS build.
Comment 42 Benjamin Poulain 2014-12-08 18:28:20 PST
Follow up: http://trac.webkit.org/changeset/177003
Comment 43 Roger Fong 2014-12-08 18:37:03 PST
(In reply to comment #42)
> Follow up: http://trac.webkit.org/changeset/177003

Whoops, thanks Ben!

Main patch landed:
http://trac.webkit.org/changeset/177002