Bug 47194

Summary: Update ANGLE rev in Webkit
Product: WebKit Reporter: Alok Priyadarshi <alokp>
Component: WebGLAssignee: Ben Vanik <ben.vanik>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ben.vanik, cmarrin, commit-queue, enne, eric, kbr, webkit.review.bot, zmo
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
ANGLE r533
kbr: review-
ANGLE r533 - revised patch kbr: review+, commit-queue: commit-queue-

Alok Priyadarshi
Reported 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.
Attachments
ANGLE r533 (1007.06 KB, patch)
2011-01-14 22:34 PST, Ben Vanik
kbr: review-
ANGLE r533 - revised patch (991.97 KB, patch)
2011-01-18 15:25 PST, Ben Vanik
kbr: review+
commit-queue: commit-queue-
Zhenyao Mo
Comment 1 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.
Ben Vanik
Comment 2 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.
Chris Marrin
Comment 3 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...
Ben Vanik
Comment 4 2011-01-14 16:02:17 PST
I'll give it a shot!
Zhenyao Mo
Comment 5 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.
Ben Vanik
Comment 6 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.
Zhenyao Mo
Comment 7 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.
Kenneth Russell
Comment 8 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.
Ben Vanik
Comment 9 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.
WebKit Review Bot
Comment 10 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.
Ben Vanik
Comment 11 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/).
Kenneth Russell
Comment 12 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?
Zhenyao Mo
Comment 13 2011-01-18 17:00:09 PST
Looks good. Thank you.
Kenneth Russell
Comment 14 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.
WebKit Commit Bot
Comment 15 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
Ben Vanik
Comment 16 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?
Kenneth Russell
Comment 17 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.
Eric Seidel (no email)
Comment 18 2011-01-18 18:23:10 PST
Yeah, our error reporting from svn-apply is bad. :(
Kenneth Russell
Comment 19 2011-01-18 19:01:00 PST
Kenneth Russell
Comment 20 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.
Kenneth Russell
Comment 21 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 .
Ben Vanik
Comment 22 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?
Kenneth Russell
Comment 23 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.
Kenneth Russell
Comment 24 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 .
Note You need to log in before you can comment on or make changes to this bug.