Bug 47194 – Update ANGLE rev in Webkit

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Alok Priyadarshi]

The ANGLE snapshot in WebKit is pretty old. The latest revision of ANGLE has many bug fixes and improvements. Since ANGLE is in a very active state of development, it would also be nice to have an automated tool to update the WebKit snapshot.

[Zhenyao Mo]

Also, the current implementation in Mac port handling ANGLE is both inefficient and buggy.  Look at WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp for comparison.

[Ben Vanik]

Any status on this? Getting *really* out of date now and making it difficult to keep changes working with WebKit.

[Chris Marrin]

I have no problem with updating ANGLE, I just haven't had the time. Please feel free to snag it, or I will get to it in a couple of weeks or so...

[Ben Vanik]

I'll give it a shot!

[Zhenyao Mo]

Since ANGLE interfaces changed, you need to fix WebCore/platform/graphics/ANGLEWebkitBridge.cpp and also shader related functions in WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp.

You can look at http://src.chromium.org/viewvc/chrome/trunk/src/webkit/gpu/webgraphicscontext3d_in_process_impl.cc?view=log.  The implementation ins GraphicsContext3DOpenGL.cpp should be very similar.

[Ben Vanik]

Created attachment 79053 [details]
ANGLE r533

This updates ANGLE to r533. Tested against the WebGL conformance tests and several of the WebGL samples - all seem to work OK. Only unique changes were in WebCore to support API changes made to ANGLE since the last snapshot several months ago.

[Zhenyao Mo]

Comment on attachment 79053 [details]
ANGLE r533

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

Looks good in general.  Thanks for taking care of this long-standing task.

> Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:85
> +            char* logBuffer = new char[logSize];

Should use OwnArrayPtr<char> here.

> Source/WebCore/platform/graphics/ANGLEWebKitBridge.cpp:98
> +        char* translationBuffer = new char[translationLength];

Should use OwnArrayPtr<char> here.

[Kenneth Russell]

Comment on attachment 79053 [details]
ANGLE r533

Ben, seconded; thanks for taking care of this long outstanding task. Could you please address Mo's review comments and upload a revised patch? I'll help you get it committed.

[Ben Vanik]

Created attachment 79343 [details]
ANGLE r533 - revised patch

This patch applies the review feedback and uses OwnArrayPtr<char> instead of raw character buffers.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/ThirdParty/ANGLE/ANGLE.xcodeproj/pr..." exit_code: 1

Last 3072 characters of output:
ANGLE/src/compiler/glslang_tab.cpp:4699:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/src/compiler/glslang_tab.cpp:4706:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/glslang_tab.cpp:4706:  glslang_parse is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/ThirdParty/ANGLE/src/compiler/Initialize.h:20:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/ThirdParty/ANGLE/src/compiler/Initialize.h:20:  The parameter name "spec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/ThirdParty/ANGLE/src/compiler/Initialize.h:21:  The parameter name "resources" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/ThirdParty/ANGLE/src/compiler/Initialize.h:28:  The parameter name "type" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/ThirdParty/ANGLE/src/compiler/Initialize.h:28:  The parameter name "spec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/ThirdParty/ANGLE/src/compiler/Initialize.h:29:  The parameter name "resources" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/ThirdParty/ANGLE/src/compiler/Initialize.h:32:  The parameter name "resources" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/ThirdParty/ANGLE/src/compiler/Initialize.h:33:  The parameter name "extensionBehavior" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/ThirdParty/ANGLE/src/libGLESv2/Fence.h:9:  #ifndef header guard has wrong style, please use: WTF_Fence_h  [build/header_guard] [5]
Source/ThirdParty/ANGLE/src/libGLESv2/Fence.h:16:  Alphabetical sorting problem.  [build/include_order] [4]
Source/ThirdParty/ANGLE/src/libGLESv2/Fence.h:19:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/libGLESv2/Fence.h:22:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/libGLESv2/Fence.h:43:  One space before end of line comments  [whitespace/comments] [5]
Source/ThirdParty/ANGLE/src/compiler/glslang_tab.h:41:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/src/compiler/glslang_tab.h:133:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/ThirdParty/ANGLE/src/compiler/glslang_tab.h:231:  Extra space for operator !   [whitespace/operators] [4]
Source/ThirdParty/ANGLE/src/compiler/glslang_tab.h:234:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/ThirdParty/ANGLE/src/compiler/glslang_tab.h:267:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 3817 in 97 files


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

[Ben Vanik]

(In reply to comment #10)
> Attachment 79343 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/ThirdParty/ANGLE/ANGLE.xcodeproj/pr..." exit_code: 1

So, yeah, ANGLE doesn't pass the style check. But the unique changes I made under WebCore do pass (so all errors are from ANGLE/).

[Kenneth Russell]

(In reply to comment #11)
> (In reply to comment #10)
> > Attachment 79343 [details] [details] did not pass style-queue:
> > 
> > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/ThirdParty/ANGLE/ANGLE.xcodeproj/pr..." exit_code: 1
> 
> So, yeah, ANGLE doesn't pass the style check. But the unique changes I made under WebCore do pass (so all errors are from ANGLE/).

Not a problem for the time being.

Mo, does this update look good to you?

[Zhenyao Mo]

Looks good.  Thank you.

[Kenneth Russell]

Comment on attachment 79343 [details]
ANGLE r533 - revised patch

Thanks, Mo. Let's see whether the commit queue can handle this monster patch.

[WebKit Commit Bot]

Comment on attachment 79343 [details]
ANGLE r533 - revised patch

Rejecting attachment 79343 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'apply-..." exit_code: 2

Last 500 characters of output:
rc/libEGL/libEGL.cpp
patching file Source/ThirdParty/ANGLE/src/libEGL/Config.cpp
patching file Source/ThirdParty/ANGLE/src/libEGL/Display.cpp
patching file Source/ThirdParty/ANGLE/src/libEGL/Surface.cpp
patching file Source/ThirdParty/ANGLE/src/libEGL/Display.h
patching file Source/ThirdParty/ANGLE/src/libEGL/Surface.h
patching file Source/ThirdParty/ANGLE/ChangeLog

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Kenneth Russell', u'--..." exit_code: 1

Full output: http://queues.webkit.org/results/7550178

[Ben Vanik]

(In reply to comment #15)
> (From update of attachment 79343 [details])
> Rejecting attachment 79343 [details] from commit-queue.

Hmmm... The log snippet doesn't seem to show anything useful. Being my first submission, what exactly is the error here? What should I try next?

[Kenneth Russell]

(In reply to comment #16)
> (In reply to comment #15)
> > (From update of attachment 79343 [details] [details])
> > Rejecting attachment 79343 [details] [details] from commit-queue.
> 
> Hmmm... The log snippet doesn't seem to show anything useful. Being my first submission, what exactly is the error here? What should I try next?

I'm not sure what the error is. I'm in the process of applying your patch to a local workspace and committing it by hand.

[Eric Seidel (no email)]

Yeah, our error reporting from svn-apply is bad.  :(

[Kenneth Russell]

Committed r76091: <http://trac.webkit.org/changeset/76091>

[Kenneth Russell]

It turns out there were tabs in three autogenerated files: glslang_lex.cpp, glslang_tab.cpp and glslang_tab.h in Source/ThirdParty/ANGLE/src/compiler/. I added the allow-tabs property to these and was able to commit the patch by hand. I'm not sure whether this was actually the failure that caused the commit queue to fail to process the patch.

[Kenneth Russell]

Note: the Mac Release builds broke with this patch; one-liner fix committed in http://trac.webkit.org/changeset/76093 , being upstreamed under http://codereview.appspot.com/4043042 .

[Ben Vanik]

(In reply to comment #21)
> Note: the Mac Release builds broke with this patch; one-liner fix committed in http://trac.webkit.org/changeset/76093 , being upstreamed under http://codereview.appspot.com/4043042 .

Interesting - I did a build-webkit (set to release) and didn't hit this error - so I don't make the same mistake again, how did you build?

[Kenneth Russell]

(In reply to comment #22)
> (In reply to comment #21)
> > Note: the Mac Release builds broke with this patch; one-liner fix committed in http://trac.webkit.org/changeset/76093 , being upstreamed under http://codereview.appspot.com/4043042 .
> 
> Interesting - I did a build-webkit (set to release) and didn't hit this error - so I don't make the same mistake again, how did you build?

This may have been a Leopard-only build failure. I didn't build Release mode (on Snow Leopard) until after committing the patch. The only way to have caught this in advance therefore would have been to either use the commit queue (which didn't work) or to have built on Leopard.

[Kenneth Russell]

It turns out to also have been necessary to remove the steps from ANGLE.xcodeproj which generated the grammar from the glslang.l/y files. The generated sources were already checked in and the version of these tools on Leopard was too old to handle one of the constructs in the grammar file. See http://trac.webkit.org/changeset/76095 .