Bug 47194 - Update ANGLE rev in Webkit
Summary: Update ANGLE rev in Webkit
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Ben Vanik
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-05 10:33 PDT by Alok Priyadarshi
Modified: 2011-01-18 19:58 PST (History)
9 users (show)

See Also:


Attachments
ANGLE r533 (1007.06 KB, patch)
2011-01-14 22:34 PST, Ben Vanik
kbr: review-
Details | Formatted Diff | Diff
ANGLE r533 - revised patch (991.97 KB, patch)
2011-01-18 15:25 PST, Ben Vanik
kbr: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alok Priyadarshi 2010-10-05 10:33:58 PDT
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.
Comment 1 Zhenyao Mo 2010-10-22 10:50:37 PDT
Also, the current implementation in Mac port handling ANGLE is both inefficient and buggy.  Look at WebKit/chromium/src/WebGraphicsContext3DDefaultImpl.cpp for comparison.
Comment 2 Ben Vanik 2011-01-14 14:08:37 PST
Any status on this? Getting *really* out of date now and making it difficult to keep changes working with WebKit.
Comment 3 Chris Marrin 2011-01-14 15:58:49 PST
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...
Comment 4 Ben Vanik 2011-01-14 16:02:17 PST
I'll give it a shot!
Comment 5 Zhenyao Mo 2011-01-14 16:25:11 PST
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.
Comment 6 Ben Vanik 2011-01-14 22:34:39 PST
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.
Comment 7 Zhenyao Mo 2011-01-18 09:46:02 PST
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.
Comment 8 Kenneth Russell 2011-01-18 11:39:19 PST
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.
Comment 9 Ben Vanik 2011-01-18 15:25:22 PST
Created attachment 79343 [details]
ANGLE r533 - revised patch

This patch applies the review feedback and uses OwnArrayPtr<char> instead of raw character buffers.
Comment 10 WebKit Review Bot 2011-01-18 15:28:48 PST
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.
Comment 11 Ben Vanik 2011-01-18 15:34:21 PST
(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/).
Comment 12 Kenneth Russell 2011-01-18 15:41:46 PST
(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?
Comment 13 Zhenyao Mo 2011-01-18 17:00:09 PST
Looks good.  Thank you.
Comment 14 Kenneth Russell 2011-01-18 17:03:02 PST
Comment on attachment 79343 [details]
ANGLE r533 - revised patch

Thanks, Mo. Let's see whether the commit queue can handle this monster patch.
Comment 15 WebKit Commit Bot 2011-01-18 18:04:14 PST
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
Comment 16 Ben Vanik 2011-01-18 18:12:49 PST
(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?
Comment 17 Kenneth Russell 2011-01-18 18:16:28 PST
(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.
Comment 18 Eric Seidel (no email) 2011-01-18 18:23:10 PST
Yeah, our error reporting from svn-apply is bad.  :(
Comment 19 Kenneth Russell 2011-01-18 19:01:00 PST
Committed r76091: <http://trac.webkit.org/changeset/76091>
Comment 20 Kenneth Russell 2011-01-18 19:02:40 PST
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.
Comment 21 Kenneth Russell 2011-01-18 19:25:17 PST
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 .
Comment 22 Ben Vanik 2011-01-18 19:36:43 PST
(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?
Comment 23 Kenneth Russell 2011-01-18 19:56:58 PST
(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.
Comment 24 Kenneth Russell 2011-01-18 19:58:05 PST
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 .