99854 2012-10-19 10:51:40 -0700 [WebGL] getUniformLocation fails for uniform array name without array brackets 2012-10-19 15:54:56 -0700 1 1 1 Unclassified WebKit WebGL 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 mvujovic mvujovic achicu dino roger_fong webkit.review.bot oldest_to_newest 746443 0 mvujovic 2012-10-19 10:51:40 -0700 For example: Given the GLSL: uniform float array[10]; In JS, the following fails: gl.getUniformLocation(program, "array"); The following succeeds: gl.getUniformLocation(program, "array[0]"); gl.getUniformLocation(program, "array[1]"); ... gl.getUniformLocation(program, "array[9]"); According to the glGetUniformLocation man page [1], the former should succeed: """ Except if the last part of name indicates a uniform variable array, the location of the first element of an array can be retrieved by using the name of the array, or by using the name appended by "[0]". """ [1]: http://www.khronos.org/opengles/sdk/docs/man/xhtml/glGetUniformLocation.xml 746447 1 169650 mvujovic 2012-10-19 10:57:07 -0700 Created attachment 169650 Patch For this patch's test, I've made to following change to Khronos WebGL conformance test suite: https://github.com/KhronosGroup/WebGL/commit/6451bfd138b2f603b667cbf7839885f665607c7a Do we need a WebKit test for this or is the WebGL conformance test sufficient? 746624 2 roger_fong 2012-10-19 14:09:27 -0700 Hmm based on this patch it looks like just using the array name with no subscripts was probably broken before I landed r131105. Sorry I missed that :/ but at least I didn't break it! woo? Also I think that the single size array does work properly, if it's size 1 then it'll just hit the 'else' path which behaves properly (although that probably wasn't the clearest way of doing things...). Hmm, o wait, I guess that in order to fix the first problem, the distinction between 'array' and 'not array' has to be made regardless. Anyways, looks good to me, although I don't have review so we'll have to wait for dino. Also hi! 746625 3 roger_fong 2012-10-19 14:10:25 -0700 (In reply to comment #1) > Do we need a WebKit test for this or is the WebGL conformance test sufficient? I wanna say just the conformance test, but Dean will know better. 746654 4 mvujovic 2012-10-19 14:34:49 -0700 (In reply to comment #2) > Hmm based on this patch it looks like just using the array name with no subscripts was probably broken before I landed r131105. Sorry I missed that :/ but at least I didn't break it! woo? No worries- this was broken before you touched it :) > > Also I think that the single size array does work properly, if it's size 1 then it'll just hit the 'else' path which behaves properly (although that probably wasn't the clearest way of doing things...). Yup, that worked properly before. I think it's just more explicit now. > > Hmm, o wait, I guess that in order to fix the first problem, the distinction between 'array' and 'not array' has to be made regardless. > > Anyways, looks good to me, although I don't have review so we'll have to wait for dino. Thanks for looking over it. I'll ask Dino for a review. > > Also hi! Hi! Nice to see you working on WebKit :) (In reply to comment #3) > (In reply to comment #1) > > Do we need a WebKit test for this or is the WebGL conformance test sufficient? > > I wanna say just the conformance test, but Dean will know better. Ok, will do. 746709 5 dino 2012-10-19 15:35:34 -0700 (In reply to comment #1) > Created an attachment (id=169650) [details] > Patch > > For this patch's test, I've made to following change to Khronos WebGL conformance test suite: https://github.com/KhronosGroup/WebGL/commit/6451bfd138b2f603b667cbf7839885f665607c7a > > Do we need a WebKit test for this or is the WebGL conformance test sufficient? I have been submitting patches to WebKit that reference the Khronos suite. There is an open bug to merge that into WebKit. Once that's done we'll be in a great position for testing regressions. 746715 6 mvujovic 2012-10-19 15:42:26 -0700 Thanks for the review! (In reply to comment #5) > (In reply to comment #1) > > Created an attachment (id=169650) [details] [details] > > Patch > > > > For this patch's test, I've made to following change to Khronos WebGL conformance test suite: https://github.com/KhronosGroup/WebGL/commit/6451bfd138b2f603b667cbf7839885f665607c7a > > > > Do we need a WebKit test for this or is the WebGL conformance test sufficient? > > I have been submitting patches to WebKit that reference the Khronos suite. There is an open bug to merge that into WebKit. Once that's done we'll be in a great position for testing regressions. Ok- I'm looking forward to it! 746721 7 169650 webkit.review.bot 2012-10-19 15:54:52 -0700 Comment on attachment 169650 Patch Clearing flags on attachment: 169650 Committed r131952: <http://trac.webkit.org/changeset/131952> 746722 8 webkit.review.bot 2012-10-19 15:54:56 -0700 All reviewed patches have been landed. Closing bug. 169650 2012-10-19 10:57:07 -0700 2012-10-19 15:54:52 -0700 Patch 99854-1.patch text/plain 4945 mvujovic ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCBlOWFmYjk5Li41NGQyZGY1IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMzQg QEAKKzIwMTItMTAtMTkgIE1heCBWdWpvdmljICA8bXZ1am92aWNAYWRvYmUuY29tPgorCisgICAg ICAgIFtXZWJHTF0gZ2V0VW5pZm9ybUxvY2F0aW9uIGZhaWxzIGZvciB1bmlmb3JtIGFycmF5IG5h bWUgd2l0aG91dCBhcnJheSBicmFja2V0cworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9y Zy9zaG93X2J1Zy5jZ2k/aWQ9OTk4NTQKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9P UFMhKS4KKworICAgICAgICBCZWZvcmUgdGhpcyBwYXRjaCwgZ2wuZ2V0VW5pZm9ybUxvY2F0aW9u KHByb2dyYW0sICJhcnJheVswXSIpIHdvdWxkIHJldHVybiB0aGUgYXJyYXkKKyAgICAgICAgbG9j YXRpb24sIGJ1dCBnbC5nZXRVbmlmb3JtTG9jYXRpb24ocHJvZ3JhbSwgImFycmF5Iikgd291bGQg bm90LiBOb3csIHRoZSBsYXR0ZXIgYWxzbworICAgICAgICByZXR1cm5zIHRoZSBhcnJheSBsb2Nh dGlvbi4KKworICAgICAgICBJbiB0aGUgcHJvY2VzcyBvZiBhZGRpbmcgYSBjaGVjayB0byB0aGUg Zm9sbG93aW5nIEtocm9ub3MgV2ViR0wgY29uZm9ybWFuY2UgdGVzdDoKKyAgICAgICAgY29uZm9y bWFuY2UvZ2xzbC9taXNjL2dsc2wtbG9uZy12YXJpYWJsZS1uYW1lcy5odG1sCisKKyAgICAgICAg KiBwbGF0Zm9ybS9ncmFwaGljcy9BTkdMRVdlYktpdEJyaWRnZS5jcHA6CisgICAgICAgIChXZWJD b3JlOjpnZXRTeW1ib2xJbmZvKToKKyAgICAgICAgICAgIEJlZm9yZSwgd2UgdXNlZCB0byBjaGVj ayB0aGF0IHRoZSBzeW1ib2wgc2l6ZSB3YXMgZ3JlYXRlciB0aGFuIG9uZSB0byBkZXRlcm1pbmUg dGhhdCB0aGUKKyAgICAgICAgICAgIHN5bWJvbCB3YXMgYW4gYXJyYXkuIEhvd2V2ZXIsIHRoaXMg ZG9lc24ndCBpZGVudGlmeSBhcnJheXMgb2YgbGVuZ3RoIG9uZS4gTm93LCB3ZSBjaGVjaworICAg ICAgICAgICAgaWYgdGhlIHN5bWJvbCBuYW1lIGVuZHMgaW4gIlswXSIsIHNpbmNlIEFOR0xFIGFw cGVuZHMgdGhpcyBzdWZmaXggdG8gYXJyYXkgc3ltYm9sCisgICAgICAgICAgICBuYW1lcy4KKyAg ICAgICAgICAgIElmIHRoZSBzeW1ib2wgaXMgYW4gYXJyYXksIHdlIHN0cmlwIG9mZiB0aGUgIlsw XSIgYW5kIGFkZCBhIHN5bWJvbCB3aXRoIGp1c3QgdGhlIGJhc2UKKyAgICAgICAgICAgIG5hbWUu IFdlIHNldCB0aGUgaXNBcnJheSBmbGFnIG9uIHRoZSBzeW1ib2wsIHNvIHdlIGRvbid0IGxvc2Ug dGhlIGluZm9ybWF0aW9uIHRoYXQgaXQgaXMKKyAgICAgICAgICAgIGFuIGFycmF5LgorICAgICAg ICAgICAgVGhlbiwgd2UgY3JlYXRlIHN5bWJvbHMgZm9yIGVhY2ggYXJyYXkgZWxlbWVudCBsaWtl IGJlZm9yZS4gSG93ZXZlciwgaW5zdGVhZCBvZgorICAgICAgICAgICAgcmVwbGFjaW5nIHRoZSAi MCIgaW4gYXJyYXlbMF0iIHdpdGggZWFjaCBpbmRleCwgd2UgdGFrZSB0aGUgYmFzZSBuYW1lICJh cnJheSIgYW5kCisgICAgICAgICAgICBhcHBlbmQgYXJyYXkgYnJhY2tldHMgY29udGFpbmluZyBl YWNoIGluZGV4IChlLmcuICJhcnJheSIgKyAiWzddIikuCisgICAgICAgICogcGxhdGZvcm0vZ3Jh cGhpY3MvQU5HTEVXZWJLaXRCcmlkZ2UuaDoKKyAgICAgICAgKEFOR0xFU2hhZGVyU3ltYm9sKToK KyAgICAgICAgICAgIEFkZCBpc0FycmF5IGJvb2xlYW4gdG8gQU5HTEVTaGFkZXJTeW1ib2wuIFNp bmNlIGFycmF5IHN5bWJvbHMgZG9uJ3QgZW5kIGluICJbMF0iCisgICAgICAgICAgICBhbnltb3Jl LCB0aGlzIGlzIHRoZSBvbmx5IHdheSB0byBpZGVudGlmeSBhcnJheXMuCisKIDIwMTItMTAtMTcg IENocmlzIEZsZWl6YWNoICA8Y2ZsZWl6YWNoQGFwcGxlLmNvbT4KIAogICAgICAgICBBWDogUmVm YWN0b3IgYWNjZXNzaWJpbGl0eSBuYW1lIGNvbXB1dGF0aW9uIHNvIGl0J3MgbW9yZSBwbGF0Zm9y bSBpbmRlcGVuZGVudApkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhp Y3MvQU5HTEVXZWJLaXRCcmlkZ2UuY3BwIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhp Y3MvQU5HTEVXZWJLaXRCcmlkZ2UuY3BwCmluZGV4IDIyZmZhYzAuLmQyNzg5YWQgMTAwNjQ0Ci0t LSBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL0FOR0xFV2ViS2l0QnJpZGdlLmNw cAorKysgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9BTkdMRVdlYktpdEJyaWRn ZS5jcHAKQEAgLTk1LDIxICs5NSwzMiBAQCBzdGF0aWMgYm9vbCBnZXRTeW1ib2xJbmZvKFNoSGFu ZGxlIGNvbXBpbGVyLCBTaFNoYWRlckluZm8gc3ltYm9sVHlwZSwgVmVjdG9yPEFORwogICAgICAg ICAvLyBpcyBhIHN1YnNldCBvZiBMYXRpbi0xIGFzIHNwZWNpZmllZCBieSB0aGUgT3BlbkdMIEVT IFNoYWRpbmcgTGFuZ3VhZ2UsIFNlY3Rpb24gMy4xIGFuZAogICAgICAgICAvLyBXZWJHTCwgU2Vj dGlvbiAiQ2hhcmFjdGVycyBPdXRzaWRlIHRoZSBHTFNMIFNvdXJjZSBDaGFyYWN0ZXIgU2V0Ii4K IAotICAgICAgICAvLyBJZiB0aGUgdmFyaWFibGUgaXMgYW4gYXJyYXksIGFkZCBzeW1ib2xzIGZv ciBlYWNoIGFycmF5IGVsZW1lbnQKLSAgICAgICAgaWYgKHN5bWJvbC5zaXplID4gMSkgeworICAg ICAgICBTdHJpbmcgbmFtZSA9IFN0cmluZyhuYW1lQnVmZmVyLmRhdGEoKSk7CisgICAgICAgIFN0 cmluZyBtYXBwZWROYW1lID0gU3RyaW5nKG1hcHBlZE5hbWVCdWZmZXIuZGF0YSgpKTsKKyAgICAg ICAgCisgICAgICAgIC8vIEFOR0xFIHJldHVybnMgYXJyYXkgbmFtZXMgaW4gdGhlIGZvcm1hdCAi YXJyYXlbMF0iLgorICAgICAgICAvLyBUaGUgb25seSB3YXkgdG8ga25vdyBpZiBhIHN5bWJvbCBp cyBhbiBhcnJheSBpcyB0byBjaGVjayBpZiBpdCBlbmRzIHdpdGggIlswXSIuCisgICAgICAgIC8v IFdlIGNhbid0IGNoZWNrIHRoZSBzaXplIGJlY2F1c2UgcmVndWxhciBzeW1ib2xzIGFuZCBhcnJh eXMgb2YgbGVuZ3RoIDEgYm90aCBoYXZlIGEgc2l6ZSBvZiAxLgorICAgICAgICBzeW1ib2wuaXNB cnJheSA9IG5hbWUuZW5kc1dpdGgoIlswXSIpICYmIG1hcHBlZE5hbWUuZW5kc1dpdGgoIlswXSIp OworICAgICAgICBpZiAoc3ltYm9sLmlzQXJyYXkpIHsKKyAgICAgICAgICAgIC8vIEFkZCBhIHN5 bWJvbCBmb3IgdGhlIGFycmF5IG5hbWUgd2l0aG91dCB0aGUgIlswXSIgc3VmZml4LgorICAgICAg ICAgICAgbmFtZS50cnVuY2F0ZShuYW1lLmxlbmd0aCgpIC0gMyk7CisgICAgICAgICAgICBtYXBw ZWROYW1lLnRydW5jYXRlKG1hcHBlZE5hbWUubGVuZ3RoKCkgLSAzKTsKKyAgICAgICAgfQorCisg ICAgICAgIHN5bWJvbC5uYW1lID0gbmFtZTsKKyAgICAgICAgc3ltYm9sLm1hcHBlZE5hbWUgPSBt YXBwZWROYW1lOworICAgICAgICBzeW1ib2xzLmFwcGVuZChzeW1ib2wpOworICAgIAorICAgICAg ICBpZiAoc3ltYm9sLmlzQXJyYXkpIHsKKyAgICAgICAgICAgIC8vIEFkZCBzeW1ib2xzIGZvciBl YWNoIGFycmF5IGVsZW1lbnQuCisgICAgICAgICAgICBzeW1ib2wuaXNBcnJheSA9IGZhbHNlOwog ICAgICAgICAgICAgZm9yIChpbnQgaSA9IDA7IGkgPCBzeW1ib2wuc2l6ZTsgaSsrKSB7Ci0gICAg ICAgICAgICAgICAgU3RyaW5nIG5hbWUgPSBuYW1lQnVmZmVyLmRhdGEoKTsKLSAgICAgICAgICAg ICAgICBTdHJpbmcgbWFwcGVkTmFtZSA9IG1hcHBlZE5hbWVCdWZmZXIuZGF0YSgpOwotICAgICAg ICAgICAgICAgIG5hbWUucmVwbGFjZShuYW1lLmxlbmd0aCgpIC0gMiwgMSwgU3RyaW5nOjpudW1i ZXIoaSkpOwotICAgICAgICAgICAgICAgIG1hcHBlZE5hbWUucmVwbGFjZShtYXBwZWROYW1lLmxl bmd0aCgpIC0gMiwgMSwgU3RyaW5nOjpudW1iZXIoaSkpOwotICAgICAgICAgICAgICAgIHN5bWJv bC5uYW1lID0gbmFtZTsKLSAgICAgICAgICAgICAgICBzeW1ib2wubWFwcGVkTmFtZSA9IG1hcHBl ZE5hbWU7CisgICAgICAgICAgICAgICAgU3RyaW5nIGFycmF5QnJhY2tldHMgPSAiWyIgKyBTdHJp bmc6Om51bWJlcihpKSArICJdIjsKKyAgICAgICAgICAgICAgICBzeW1ib2wubmFtZSA9IG5hbWUg KyBhcnJheUJyYWNrZXRzOworICAgICAgICAgICAgICAgIHN5bWJvbC5tYXBwZWROYW1lID0gbWFw cGVkTmFtZSArIGFycmF5QnJhY2tldHM7CiAgICAgICAgICAgICAgICAgc3ltYm9scy5hcHBlbmQo c3ltYm9sKTsKICAgICAgICAgICAgIH0KLSAgICAgICAgfSBlbHNlIHsKLSAgICAgICAgICAgIHN5 bWJvbC5uYW1lID0gU3RyaW5nKG5hbWVCdWZmZXIuZGF0YSgpKTsKLSAgICAgICAgICAgIHN5bWJv bC5tYXBwZWROYW1lID0gU3RyaW5nKG1hcHBlZE5hbWVCdWZmZXIuZGF0YSgpKTsKLSAgICAgICAg ICAgIHN5bWJvbHMuYXBwZW5kKHN5bWJvbCk7CiAgICAgICAgIH0KICAgICB9CiAgICAgcmV0dXJu IHRydWU7CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9BTkdM RVdlYktpdEJyaWRnZS5oIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvQU5HTEVX ZWJLaXRCcmlkZ2UuaAppbmRleCA1YzUwOThlLi41MmY3MWRkIDEwMDY0NAotLS0gYS9Tb3VyY2Uv V2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9BTkdMRVdlYktpdEJyaWRnZS5oCisrKyBiL1NvdXJj ZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL0FOR0xFV2ViS2l0QnJpZGdlLmgKQEAgLTUzLDYg KzUzLDcgQEAgc3RydWN0IEFOR0xFU2hhZGVyU3ltYm9sIHsKICAgICBTdHJpbmcgbWFwcGVkTmFt ZTsKICAgICBTaERhdGFUeXBlIGRhdGFUeXBlOwogICAgIGludCBzaXplOworICAgIGJvb2wgaXNB cnJheTsKIAogICAgIGJvb2wgaXNTYW1wbGVyKCkgY29uc3QKICAgICB7Cg==