111333 2013-03-04 08:24:58 -0800 [EFL] Ensure right format is used in SnapshotImageGL. 2013-03-05 23:39:18 -0800 1 1 1 Unclassified WebKit WebKit EFL 528+ (Nightly build) PC Linux RESOLVED FIXED P2 Normal --- 105659 1 kalyan.kondapally kalyan.kondapally benjamin gyuyoung.kim laszlo.gombos lucas.de.marchi ostap73 rakuco webkit.review.bot oldest_to_newest 846515 0 kalyan.kondapally 2013-03-04 08:24:58 -0800 In SnapshotImageGL we use GL_BGRA as the format with glReadPixels. This is not the standard format on OpenGLES2.0. With GLES we should use GL_RGBA as the format and convert the result to BGRA. 846595 1 191264 kalyan.kondapally 2013-03-04 09:58:05 -0800 Created attachment 191264 patch 846617 2 191264 kenneth 2013-03-04 10:39:22 -0800 Comment on attachment 191264 patch View in context: https://bugs.webkit.org/attachment.cgi?id=191264&action=review > Source/WebKit2/UIProcess/API/efl/SnapshotImageGL.cpp:60 > + > + for (int i = 0; i < totalBytes; i += 4) > + std::swap(buf[i], buf[i + 2]); why this indentation? 846931 3 191328 kalyan.kondapally 2013-03-04 15:37:38 -0800 Created attachment 191328 patchv2 846935 4 kalyan.kondapally 2013-03-04 15:38:54 -0800 (In reply to comment #2) > (From update of attachment 191264 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=191264&action=review > > > + std::swap(buf[i], buf[i + 2]); > > why this indentation? oops, fixed it now. 848276 5 191328 benjamin 2013-03-05 17:10:38 -0800 Comment on attachment 191328 patchv2 View in context: https://bugs.webkit.org/attachment.cgi?id=191328&action=review > Source/WebKit2/UIProcess/API/efl/SnapshotImageGL.cpp:62 > +#if USE(OPENGL_ES_2) > + // Convert to BGRA. > + int totalBytes = width * height * 4; > + > + for (int i = 0; i < totalBytes; i += 4) > + std::swap(buf[i], buf[i + 2]); > +#endif > + It is hard to believe you must do something so inefficient. :( Here: std::swap(buf[i], buf[i + 2]); You swap to 32bits pixels, not 2 * 8 bits component. 848340 6 191629 kalyan.kondapally 2013-03-05 18:16:51 -0800 Created attachment 191629 patchv3 848341 7 kalyan.kondapally 2013-03-05 18:19:11 -0800 (In reply to comment #5) > (From update of attachment 191328 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=191328&action=review > It is hard to believe you must do something so inefficient. :( > > Here: std::swap(buf[i], buf[i + 2]); > You swap to 32bits pixels, not 2 * 8 bits component. Thanks for highlighting this. Fixed it now. 848512 8 191629 webkit.review.bot 2013-03-05 23:39:14 -0800 Comment on attachment 191629 patchv3 Clearing flags on attachment: 191629 Committed r144887: <http://trac.webkit.org/changeset/144887> 848513 9 webkit.review.bot 2013-03-05 23:39:18 -0800 All reviewed patches have been landed. Closing bug. 191264 2013-03-04 09:58:05 -0800 2013-03-04 15:37:38 -0800 patch snapshotreview.patch text/plain 2016 kalyan.kondapally ZGlmZiAtLWdpdCBTb3VyY2UvV2ViS2l0Mi9DaGFuZ2VMb2cgU291cmNlL1dlYktpdDIvQ2hhbmdl TG9nCmluZGV4IDkzNDY1MmUuLjRhNWI5MTYgMTAwNjQ0Ci0tLSBTb3VyY2UvV2ViS2l0Mi9DaGFu Z2VMb2cKKysrIFNvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE3IEBACisyMDEz LTAzLTA0ICBLb25kYXBhbGx5IEthbHlhbiAgPGthbHlhbi5rb25kYXBhbGx5QGludGVsLmNvbT4K KworICAgICAgICBbRUZMXSBFbnN1cmUgcmlnaHQgZm9ybWF0IGlzIHVzZWQgaW4gU25hcHNob3RJ bWFnZUdMLgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9 MTExMzMzCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAg R0xfQkdSQSBmb3JtYXQgaXMgbm90IHN0YW5kYXJkIGZvciBnbFJlYWRQaXhlbHMgd2l0aCBHTEVT CisgICAgICAgIFRoaXMgcGF0Y2ggZW5zdXJlcyB0aGF0IEdMX1JHQkEgaXMgdXNlZCBpbiBTbmFw c2hvdEltYWdlR0wKKyAgICAgICAgd2hpbGUgdXNpbmcgR0xFUy4KKworICAgICAgICAqIFVJUHJv Y2Vzcy9BUEkvZWZsL1NuYXBzaG90SW1hZ2VHTC5jcHA6CisgICAgICAgIChnZXRJbWFnZVN1cmZh Y2VGcm9tRnJhbWVCdWZmZXIpOgorCiAyMDEzLTAzLTA0ICBLdW5paGlrbyBTYWthbW90byAgPGtz YWthbW90b0BjaHJvbWl1bS5vcmc+CiAKICAgICAgICAgQWRkIGJ1aWxkIGZsYWcgZm9yIEZvbnRM b2FkZXIKZGlmZiAtLWdpdCBTb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvQVBJL2VmbC9TbmFwc2hv dEltYWdlR0wuY3BwIFNvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9BUEkvZWZsL1NuYXBzaG90SW1h Z2VHTC5jcHAKaW5kZXggOWI3MjZjZC4uMTkyMTY2YyAxMDA2NDQKLS0tIFNvdXJjZS9XZWJLaXQy L1VJUHJvY2Vzcy9BUEkvZWZsL1NuYXBzaG90SW1hZ2VHTC5jcHAKKysrIFNvdXJjZS9XZWJLaXQy L1VJUHJvY2Vzcy9BUEkvZWZsL1NuYXBzaG90SW1hZ2VHTC5jcHAKQEAgLTQwLDExICs0MCwyNiBA QCBQYXNzUmVmUHRyPGNhaXJvX3N1cmZhY2VfdD4gZ2V0SW1hZ2VTdXJmYWNlRnJvbUZyYW1lQnVm ZmVyKGludCB4LCBpbnQgeSwgaW50IHdpZAogICAgIFJlZlB0cjxjYWlyb19zdXJmYWNlX3Q+IG5l d1N1cmZhY2UgPSBhZG9wdFJlZihjYWlyb19pbWFnZV9zdXJmYWNlX2NyZWF0ZShDQUlST19GT1JN QVRfQVJHQjMyLCB3aWR0aCwgaGVpZ2h0KSk7CiAgICAgdW5zaWduZWQgY2hhciogZGF0YSA9IGNh aXJvX2ltYWdlX3N1cmZhY2VfZ2V0X2RhdGEobmV3U3VyZmFjZS5nZXQoKSk7CiAKLSAgICBnbFJl YWRQaXhlbHMoeCwgeSwgd2lkdGgsIGhlaWdodCwgR0xfQkdSQSwgR0xfVU5TSUdORURfQllURSwg ZGF0YSk7CisjaWYgVVNFKE9QRU5HTF9FU18yKQorICAgIEdMZW51bSBmb3JtYXQgPSBHTF9SR0JB OworI2Vsc2UKKyAgICBHTGVudW0gZm9ybWF0ID0gR0xfQkdSQTsKKyNlbmRpZgorCisgICAgZ2xS ZWFkUGl4ZWxzKHgsIHksIHdpZHRoLCBoZWlnaHQsIGZvcm1hdCwgR0xfVU5TSUdORURfQllURSwg ZGF0YSk7CisKIAogICAgIC8vIFRleHR1cmVzIGFyZSBmbGlwcGVkIG9uIHRoZSBZIGF4aXMsIHNv IHdlIG5lZWQgdG8gZmxpcCB0aGUgaW1hZ2UgYmFjay4KICAgICB1bnNpZ25lZCogYnVmID0gcmVp bnRlcnByZXRfY2FzdDx1bnNpZ25lZCo+KGRhdGEpOwogCisjaWYgVVNFKE9QRU5HTF9FU18yKQor ICAgIC8vIENvbnZlcnQgdG8gQkdSQS4KKyAgICBpbnQgdG90YWxCeXRlcyA9IHdpZHRoICogaGVp Z2h0ICogNDsKKworICAgICAgICBmb3IgKGludCBpID0gMDsgaSA8IHRvdGFsQnl0ZXM7IGkgKz0g NCkKKyAgICAgICAgICAgIHN0ZDo6c3dhcChidWZbaV0sIGJ1ZltpICsgMl0pOworI2VuZGlmCisK ICAgICBmb3IgKGludCBpID0gMDsgaSA8IGhlaWdodCAvIDI7ICsraSkgewogICAgICAgICBmb3Ig KGludCBqID0gMDsgaiA8IHdpZHRoOyArK2opIHsKICAgICAgICAgICAgIHVuc2lnbmVkIHRtcCA9 IGJ1ZltpICogd2lkdGggKyBqXTsK 191328 2013-03-04 15:37:38 -0800 2013-03-05 18:16:51 -0800 patchv2 reviewsnapshotfix.patch text/plain 2009 kalyan.kondapally ZGlmZiAtLWdpdCBTb3VyY2UvV2ViS2l0Mi9DaGFuZ2VMb2cgU291cmNlL1dlYktpdDIvQ2hhbmdl TG9nCmluZGV4IDkzNDY1MmUuLjRhNWI5MTYgMTAwNjQ0Ci0tLSBTb3VyY2UvV2ViS2l0Mi9DaGFu Z2VMb2cKKysrIFNvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE3IEBACisyMDEz LTAzLTA0ICBLb25kYXBhbGx5IEthbHlhbiAgPGthbHlhbi5rb25kYXBhbGx5QGludGVsLmNvbT4K KworICAgICAgICBbRUZMXSBFbnN1cmUgcmlnaHQgZm9ybWF0IGlzIHVzZWQgaW4gU25hcHNob3RJ bWFnZUdMLgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9 MTExMzMzCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAg R0xfQkdSQSBmb3JtYXQgaXMgbm90IHN0YW5kYXJkIGZvciBnbFJlYWRQaXhlbHMgd2l0aCBHTEVT LgorICAgICAgICBUaGlzIHBhdGNoIGVuc3VyZXMgdGhhdCBHTF9SR0JBIGlzIHVzZWQgaW4gU25h cHNob3RJbWFnZUdMCisgICAgICAgIHdoaWxlIHVzaW5nIEdMRVMuCisKKyAgICAgICAgKiBVSVBy b2Nlc3MvQVBJL2VmbC9TbmFwc2hvdEltYWdlR0wuY3BwOgorICAgICAgICAoZ2V0SW1hZ2VTdXJm YWNlRnJvbUZyYW1lQnVmZmVyKToKKwogMjAxMy0wMy0wNCAgS3VuaWhpa28gU2FrYW1vdG8gIDxr c2FrYW1vdG9AY2hyb21pdW0ub3JnPgogCiAgICAgICAgIEFkZCBidWlsZCBmbGFnIGZvciBGb250 TG9hZGVyCmRpZmYgLS1naXQgU291cmNlL1dlYktpdDIvVUlQcm9jZXNzL0FQSS9lZmwvU25hcHNo b3RJbWFnZUdMLmNwcCBTb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvQVBJL2VmbC9TbmFwc2hvdElt YWdlR0wuY3BwCmluZGV4IDliNzI2Y2QuLjI1NTM4NzEgMTAwNjQ0Ci0tLSBTb3VyY2UvV2ViS2l0 Mi9VSVByb2Nlc3MvQVBJL2VmbC9TbmFwc2hvdEltYWdlR0wuY3BwCisrKyBTb3VyY2UvV2ViS2l0 Mi9VSVByb2Nlc3MvQVBJL2VmbC9TbmFwc2hvdEltYWdlR0wuY3BwCkBAIC00MCwxMSArNDAsMjYg QEAgUGFzc1JlZlB0cjxjYWlyb19zdXJmYWNlX3Q+IGdldEltYWdlU3VyZmFjZUZyb21GcmFtZUJ1 ZmZlcihpbnQgeCwgaW50IHksIGludCB3aWQKICAgICBSZWZQdHI8Y2Fpcm9fc3VyZmFjZV90PiBu ZXdTdXJmYWNlID0gYWRvcHRSZWYoY2Fpcm9faW1hZ2Vfc3VyZmFjZV9jcmVhdGUoQ0FJUk9fRk9S TUFUX0FSR0IzMiwgd2lkdGgsIGhlaWdodCkpOwogICAgIHVuc2lnbmVkIGNoYXIqIGRhdGEgPSBj YWlyb19pbWFnZV9zdXJmYWNlX2dldF9kYXRhKG5ld1N1cmZhY2UuZ2V0KCkpOwogCi0gICAgZ2xS ZWFkUGl4ZWxzKHgsIHksIHdpZHRoLCBoZWlnaHQsIEdMX0JHUkEsIEdMX1VOU0lHTkVEX0JZVEUs IGRhdGEpOworI2lmIFVTRShPUEVOR0xfRVNfMikKKyAgICBHTGVudW0gZm9ybWF0ID0gR0xfUkdC QTsKKyNlbHNlCisgICAgR0xlbnVtIGZvcm1hdCA9IEdMX0JHUkE7CisjZW5kaWYKKworICAgIGds UmVhZFBpeGVscyh4LCB5LCB3aWR0aCwgaGVpZ2h0LCBmb3JtYXQsIEdMX1VOU0lHTkVEX0JZVEUs IGRhdGEpOworCiAKICAgICAvLyBUZXh0dXJlcyBhcmUgZmxpcHBlZCBvbiB0aGUgWSBheGlzLCBz byB3ZSBuZWVkIHRvIGZsaXAgdGhlIGltYWdlIGJhY2suCiAgICAgdW5zaWduZWQqIGJ1ZiA9IHJl aW50ZXJwcmV0X2Nhc3Q8dW5zaWduZWQqPihkYXRhKTsKIAorI2lmIFVTRShPUEVOR0xfRVNfMikK KyAgICAvLyBDb252ZXJ0IHRvIEJHUkEuCisgICAgaW50IHRvdGFsQnl0ZXMgPSB3aWR0aCAqIGhl aWdodCAqIDQ7CisKKyAgICBmb3IgKGludCBpID0gMDsgaSA8IHRvdGFsQnl0ZXM7IGkgKz0gNCkK KyAgICAgICAgc3RkOjpzd2FwKGJ1ZltpXSwgYnVmW2kgKyAyXSk7CisjZW5kaWYKKwogICAgIGZv ciAoaW50IGkgPSAwOyBpIDwgaGVpZ2h0IC8gMjsgKytpKSB7CiAgICAgICAgIGZvciAoaW50IGog PSAwOyBqIDwgd2lkdGg7ICsraikgewogICAgICAgICAgICAgdW5zaWduZWQgdG1wID0gYnVmW2kg KiB3aWR0aCArIGpdOwo= 191629 2013-03-05 18:16:51 -0800 2013-03-05 23:39:14 -0800 patchv3 snapshotreview.patch text/plain 1878 kalyan.kondapally ZGlmZiAtLWdpdCBTb3VyY2UvV2ViS2l0Mi9DaGFuZ2VMb2cgU291cmNlL1dlYktpdDIvQ2hhbmdl TG9nCmluZGV4IDg1OWYwNmEuLjk5MTliMGQgMTAwNjQ0Ci0tLSBTb3VyY2UvV2ViS2l0Mi9DaGFu Z2VMb2cKKysrIFNvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE3IEBACisyMDEz LTAzLTA1ICBLb25kYXBhbGx5IEthbHlhbiAgPGthbHlhbi5rb25kYXBhbGx5QGludGVsLmNvbT4K KworICAgICAgICBbRUZMXSBFbnN1cmUgcmlnaHQgZm9ybWF0IGlzIHVzZWQgaW4gU25hcHNob3RJ bWFnZUdMLgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9 MTExMzMzCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAg R0xfQkdSQSBmb3JtYXQgaXMgbm90IHN0YW5kYXJkIGZvciBnbFJlYWRQaXhlbHMgd2l0aCBHTEVT LgorICAgICAgICBUaGlzIHBhdGNoIGVuc3VyZXMgdGhhdCBHTF9SR0JBIGlzIHVzZWQgaW4gU25h cHNob3RJbWFnZUdMCisgICAgICAgIHdoaWxlIHVzaW5nIEdMRVMuCisKKyAgICAgICAgKiBVSVBy b2Nlc3MvQVBJL2VmbC9TbmFwc2hvdEltYWdlR0wuY3BwOgorICAgICAgICAoZ2V0SW1hZ2VTdXJm YWNlRnJvbUZyYW1lQnVmZmVyKToKKwogMjAxMy0wMy0wNSAgQW5kZXJzIENhcmxzc29uICA8YW5k ZXJzY2FAYXBwbGUuY29tPgogCiAgICAgICAgIFJlc2V0IHRoZSB2YWx1ZXMgaWYgdGhlcmUgaXMg YSBxdW90YSBlcnJvcgpkaWZmIC0tZ2l0IFNvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9BUEkvZWZs L1NuYXBzaG90SW1hZ2VHTC5jcHAgU291cmNlL1dlYktpdDIvVUlQcm9jZXNzL0FQSS9lZmwvU25h cHNob3RJbWFnZUdMLmNwcAppbmRleCA5YjcyNmNkLi5lZmQyNjE5IDEwMDY0NAotLS0gU291cmNl L1dlYktpdDIvVUlQcm9jZXNzL0FQSS9lZmwvU25hcHNob3RJbWFnZUdMLmNwcAorKysgU291cmNl L1dlYktpdDIvVUlQcm9jZXNzL0FQSS9lZmwvU25hcHNob3RJbWFnZUdMLmNwcApAQCAtNDAsNyAr NDAsMjEgQEAgUGFzc1JlZlB0cjxjYWlyb19zdXJmYWNlX3Q+IGdldEltYWdlU3VyZmFjZUZyb21G cmFtZUJ1ZmZlcihpbnQgeCwgaW50IHksIGludCB3aWQKICAgICBSZWZQdHI8Y2Fpcm9fc3VyZmFj ZV90PiBuZXdTdXJmYWNlID0gYWRvcHRSZWYoY2Fpcm9faW1hZ2Vfc3VyZmFjZV9jcmVhdGUoQ0FJ Uk9fRk9STUFUX0FSR0IzMiwgd2lkdGgsIGhlaWdodCkpOwogICAgIHVuc2lnbmVkIGNoYXIqIGRh dGEgPSBjYWlyb19pbWFnZV9zdXJmYWNlX2dldF9kYXRhKG5ld1N1cmZhY2UuZ2V0KCkpOwogCi0g ICAgZ2xSZWFkUGl4ZWxzKHgsIHksIHdpZHRoLCBoZWlnaHQsIEdMX0JHUkEsIEdMX1VOU0lHTkVE X0JZVEUsIGRhdGEpOworI2lmIFVTRShPUEVOR0xfRVNfMikKKyAgICBHTGVudW0gZm9ybWF0ID0g R0xfUkdCQTsKKyNlbHNlCisgICAgR0xlbnVtIGZvcm1hdCA9IEdMX0JHUkE7CisjZW5kaWYKKwor ICAgIGdsUmVhZFBpeGVscyh4LCB5LCB3aWR0aCwgaGVpZ2h0LCBmb3JtYXQsIEdMX1VOU0lHTkVE X0JZVEUsIGRhdGEpOworCisjaWYgVVNFKE9QRU5HTF9FU18yKQorICAgIC8vIENvbnZlcnQgdG8g QkdSQS4KKyAgICBpbnQgdG90YWxCeXRlcyA9IHdpZHRoICogaGVpZ2h0ICogNDsKKworICAgIGZv ciAoaW50IGkgPSAwOyBpIDwgdG90YWxCeXRlczsgaSArPSA0KQorICAgICAgICBzdGQ6OnN3YXAo ZGF0YVtpXSwgZGF0YVtpICsgMl0pOworI2VuZGlmCiAKICAgICAvLyBUZXh0dXJlcyBhcmUgZmxp cHBlZCBvbiB0aGUgWSBheGlzLCBzbyB3ZSBuZWVkIHRvIGZsaXAgdGhlIGltYWdlIGJhY2suCiAg ICAgdW5zaWduZWQqIGJ1ZiA9IHJlaW50ZXJwcmV0X2Nhc3Q8dW5zaWduZWQqPihkYXRhKTsK