Bug 125293 – GL_HALF_FLOAT_ARB causes a build break when using GLES.

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: WebGL

[ChangSeok Oh]

GL_HALF_FLOAT_ARB is not a valid parameter for GLES. Maybe r160119 is the related changeset.

[ChangSeok Oh]

Created attachment 218494 [details]
Patch

[WebKit Commit Bot]

Attachment 218494 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp', u'Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp', u'Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:283:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:283:  Comma should be at the beggining of the line in a member initialization list.  [whitespace/init] [4]
ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:216:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:216:  Comma should be at the beggining of the line in a member initialization list.  [whitespace/init] [4]
Total errors found: 4 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[ChangSeok Oh]

(In reply to comment #2)
> Attachment 218494 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp', u'Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp', u'Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp', '--commit-queue']" exit_code: 1
> ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:283:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
> ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:283:  Comma should be at the beggining of the line in a member initialization list.  [whitespace/init] [4]
> ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:216:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
> ERROR: Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:216:  Comma should be at the beggining of the line in a member initialization list.  [whitespace/init] [4]
> Total errors found: 4 in 3 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

I think this is a false alarm.

[Martin Robinson]

Comment on attachment 218494 [details]
Patch

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

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:216
> +    // FIXME: we will need to deal with PixelStore params when dealing with image buffers that differ from the subimage size.
> +    ::glTexSubImage2D(target, level, xoff, yoff, width, height, format, type, pixels);

Is HALF_FLOAT_OES supported properly without any translation?

[Eduardo Lima Mitev]

Looks like the style errors are indeed false positives. Can we move on and review the it?
I'm having the same problem.

[Martin Robinson]

So it looks like HALF_FLOAT_OES is supported if http://www.khronos.org/registry/gles/extensions/OES/OES_texture_float.txt is supported, but Extensions3DOpenGLES::supportsExtension is not written correctly to detect OpenGLES extensions. It should be written to detect if m_availableExtensions contains the appropriate extensions. Proper support for OES_texture_float also requires properly reporting that the extension is available. We should do more than fix the build here and implement a proper fix.

[ChangSeok Oh]

(In reply to comment #6)
> So it looks like HALF_FLOAT_OES is supported if http://www.khronos.org/registry/gles/extensions/OES/OES_texture_float.txt is supported, but Extensions3DOpenGLES::supportsExtension is not written correctly to detect OpenGLES extensions. It should be written to detect if m_availableExtensions contains the appropriate extensions. Proper support for OES_texture_float also requires properly reporting that the extension is available. We should do more than fix the build here and implement a proper fix.

got it. I'll do!

[Brent Fulgham]

Comment on attachment 218494 [details]
Patch

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

I don't think this patch is needed now that Bug 125541 has landed.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLCommon.cpp:-1366
> -

Why is this being removed? ANGLE-based code uses it, so this will break Windows at the very least.

https://bugs.webkit.org/show_bug.cgi?id=125541 protects against converting HALF_FLOAT_OES to HALF_FLOAT_ARB on GLES-based platforms, so this should be safe.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:218
> +

I don't think this should be moved here.

[ChangSeok Oh]

*** This bug has been marked as a duplicate of bug 125541 ***

[ChangSeok Oh]

Comment on attachment 218494 [details]
Patch

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

>> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGLES.cpp:218
>> +
> 
> I don't think this should be moved here.

Well.. AFAIK, why we split GC3D into GC3DOpenGLCommon, GC3DOpenGL and GC3DOpenGLES was to prevent abusing build flags like USE(OPENGL_ES_2). That was why I moved texSubImage2D here.