54224 2011-02-10 10:58:21 -0800 toDataURL() fails if skia device is backed by GPU 2011-02-11 12:10:24 -0800 1 1 1 Unclassified WebKit Canvas 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 reed webkit-unassigned commit-queue gwright jamesr jpetsovits kbr mdelaney7 reed oldest_to_newest 349191 0 reed 2011-02-10 10:58:21 -0800 The current impl assumes it can read the address returned by the device's bitmap, and had it to the image encoders. The fix is to check if we need to call device->readPixels() first, which will always succeed, even if the device is backed by some other means than a CPU bitmap (e.g. opengl fbo) 349195 1 82008 reed 2011-02-10 11:02:07 -0800 Created attachment 82008 fix for 54224 - call readPixels() if need be, if the address from getPixels() is null 349205 2 82008 jamesr 2011-02-10 11:14:21 -0800 Comment on attachment 82008 fix for 54224 - call readPixels() if need be, if the address from getPixels() is null View in context: https://bugs.webkit.org/attachment.cgi?id=82008&action=review R=me > WebCore/ChangeLog:9 > + No new tests. Existing canvas tests exercise this. Do we know which test(s) cover this? 349210 3 jpetsovits 2011-02-10 11:18:46 -0800 Don't you need to unlock the bitmap to be able to access the pixel data? iirc, there were cases where a perfectly valid memory-backed bitmap returns 0 for getPixels() because the bitmap wasn't locked. Maybe that doesn't apply in this specific case, depends on what bitmap the canvas is created on. 349226 4 82014 reed 2011-02-10 11:35:15 -0800 Created attachment 82014 add explicit test that exercises this code path, explicitly lockPixels() before calling getPixels() 349456 5 82014 jamesr 2011-02-10 15:46:18 -0800 Comment on attachment 82014 add explicit test that exercises this code path, explicitly lockPixels() before calling getPixels() View in context: https://bugs.webkit.org/attachment.cgi?id=82014&action=review Great! Thanks for adding the test reference, it'll be useful to have this information if that test every breaks in the future. Would you like me to c-q+ this as well? > WebCore/platform/graphics/skia/ImageBufferSkia.cpp:354 > + // if we can't see the pixels directly, call readPixels() to get a copy. > + // this could happen if the device is backed by a GPU. > + bitmap.lockPixels(); // balanced by our destructor, or explicitly if getPixels() fails nit: In WebKit, comments start with an uppercase letter and end with a period. 349751 6 reed 2011-02-11 06:50:31 -0800 can this be queued for commit? thanks. 349906 7 82014 commit-queue 2011-02-11 12:10:19 -0800 Comment on attachment 82014 add explicit test that exercises this code path, explicitly lockPixels() before calling getPixels() Clearing flags on attachment: 82014 Committed r78356: <http://trac.webkit.org/changeset/78356> 349907 8 commit-queue 2011-02-11 12:10:24 -0800 All reviewed patches have been landed. Closing bug. 82008 2011-02-10 11:02:07 -0800 2011-02-10 11:35:15 -0800 fix for 54224 - call readPixels() if need be, if the address from getPixels() is null todataurl.diff text/plain 2428 reed SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA3ODI0NykKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTYgQEAKKzIwMTEtMDItMTAgIE1pa2UgUmVlZCAgPHJlZWRAZ29vZ2xlLmNvbT4K KworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBGaXggdG9E YXRhVVJMKCkgdG8gdXNlIGRldmljZS0+cmVhZFBpeGVscygpIGlmIG5lZWQgYmUsIHJhdGhlciB0 aGFuIGFsd2F5cyBkZXJlZmVyZW5jaW5nCisgICAgICAgIHRoZSBhZGRyZXNzIHJldHVybmVkIGJ5 IGdldFBpeGVscygpIChhcyB0aGUgZGV2aWNlIG1heSBub3QgYmUgYmFja2VkIGJ5IGEgQ1BVIGJp dG1hcCkKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTU0 MjI0CisKKyAgICAgICAgTm8gbmV3IHRlc3RzLiBFeGlzdGluZyBjYW52YXMgdGVzdHMgZXhlcmNp c2UgdGhpcy4gCisKKyAgICAgICAgKiBwbGF0Zm9ybS9ncmFwaGljcy9za2lhL0ltYWdlQnVmZmVy U2tpYS5jcHA6CisgICAgICAgIChXZWJDb3JlOjpJbWFnZUJ1ZmZlcjo6dG9EYXRhVVJMKToKKwog MjAxMS0wMi0xMCAgQ2hyaXMgRmxlaXphY2ggIDxjZmxlaXphY2hAYXBwbGUuY29tPgogCiAgICAg ICAgIFJldmlld2VkIGJ5IEFuZGVycyBDYXJsc3Nvbi4KSW5kZXg6IFdlYkNvcmUvcGxhdGZvcm0v Z3JhcGhpY3Mvc2tpYS9JbWFnZUJ1ZmZlclNraWEuY3BwCj09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUv cGxhdGZvcm0vZ3JhcGhpY3Mvc2tpYS9JbWFnZUJ1ZmZlclNraWEuY3BwCShyZXZpc2lvbiA3ODE3 OCkKKysrIFdlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3Mvc2tpYS9JbWFnZUJ1ZmZlclNraWEuY3Bw CSh3b3JraW5nIGNvcHkpCkBAIC0zNDYsMTQgKzM0NiwyNSBAQCBTdHJpbmcgSW1hZ2VCdWZmZXI6 OnRvRGF0YVVSTChjb25zdCBTdHJpCiAgICAgQVNTRVJUKE1JTUVUeXBlUmVnaXN0cnk6OmlzU3Vw cG9ydGVkSW1hZ2VNSU1FVHlwZUZvckVuY29kaW5nKG1pbWVUeXBlKSk7CiAKICAgICBWZWN0b3I8 dW5zaWduZWQgY2hhcj4gZW5jb2RlZEltYWdlOworICAgIFNrRGV2aWNlKiBkZXZpY2UgPSBjb250 ZXh0KCktPnBsYXRmb3JtQ29udGV4dCgpLT5jYW52YXMoKS0+Z2V0RGV2aWNlKCk7CisgICAgU2tC aXRtYXAgYml0bWFwID0gZGV2aWNlLT5hY2Nlc3NCaXRtYXAoZmFsc2UpOworCisgICAgLy8gaWYg d2UgY2FuJ3Qgc2VlIHRoZSBwaXhlbHMgZGlyZWN0bHksIGNhbGwgcmVhZFBpeGVscygpIHRvIGdl dCBhIGNvcHkuCisgICAgLy8gdGhpcyBjb3VsZCBoYXBwZW4gaWYgdGhlIGRldmljZSBpcyBiYWNr ZWQgYnkgYSBHUFUuCisgICAgaWYgKCFiaXRtYXAuZ2V0UGl4ZWxzKCkpIHsKKyAgICAgICAgU2tJ UmVjdCBib3VuZHMgPSBTa0lSZWN0OjpNYWtlV0goZGV2aWNlLT53aWR0aCgpLCBkZXZpY2UtPmhl aWdodCgpKTsKKyAgICAgICAgaWYgKCFkZXZpY2UtPnJlYWRQaXhlbHMoYm91bmRzLCAmYml0bWFw KSkKKyAgICAgICAgICAgIHJldHVybiAiZGF0YTosIjsKKyAgICB9CisgICAgCiAgICAgaWYgKG1p bWVUeXBlID09ICJpbWFnZS9qcGVnIikgewogICAgICAgICBpbnQgY29tcHJlc3Npb25RdWFsaXR5 ID0gSlBFR0ltYWdlRW5jb2Rlcjo6RGVmYXVsdENvbXByZXNzaW9uUXVhbGl0eTsKICAgICAgICAg aWYgKHF1YWxpdHkgJiYgKnF1YWxpdHkgPj0gMC4wICYmICpxdWFsaXR5IDw9IDEuMCkKICAgICAg ICAgICAgIGNvbXByZXNzaW9uUXVhbGl0eSA9IHN0YXRpY19jYXN0PGludD4oKnF1YWxpdHkgKiAx MDAgKyAwLjUpOwotICAgICAgICBpZiAoIUpQRUdJbWFnZUVuY29kZXI6OmVuY29kZSgqY29udGV4 dCgpLT5wbGF0Zm9ybUNvbnRleHQoKS0+Yml0bWFwKCksIGNvbXByZXNzaW9uUXVhbGl0eSwgJmVu Y29kZWRJbWFnZSkpCisgICAgICAgIGlmICghSlBFR0ltYWdlRW5jb2Rlcjo6ZW5jb2RlKGJpdG1h cCwgY29tcHJlc3Npb25RdWFsaXR5LCAmZW5jb2RlZEltYWdlKSkKICAgICAgICAgICAgIHJldHVy biAiZGF0YTosIjsKICAgICB9IGVsc2UgewotICAgICAgICBpZiAoIVBOR0ltYWdlRW5jb2Rlcjo6 ZW5jb2RlKCpjb250ZXh0KCktPnBsYXRmb3JtQ29udGV4dCgpLT5iaXRtYXAoKSwgJmVuY29kZWRJ bWFnZSkpCisgICAgICAgIGlmICghUE5HSW1hZ2VFbmNvZGVyOjplbmNvZGUoYml0bWFwLCAmZW5j b2RlZEltYWdlKSkKICAgICAgICAgICAgIHJldHVybiAiZGF0YTosIjsKICAgICAgICAgQVNTRVJU KG1pbWVUeXBlID09ICJpbWFnZS9wbmciKTsKICAgICB9Cg== 82014 2011-02-10 11:35:15 -0800 2011-02-11 12:10:19 -0800 add explicit test that exercises this code path, explicitly lockPixels() before calling getPixels() todataurl.diff text/plain 2629 reed SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA3ODI0NykKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTcgQEAKKzIwMTEtMDItMTAgIE1pa2UgUmVlZCAgPHJlZWRAZ29vZ2xlLmNvbT4K KworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBGaXggdG9E YXRhVVJMKCkgdG8gdXNlIGRldmljZS0+cmVhZFBpeGVscygpIGlmIG5lZWQgYmUsIHJhdGhlciB0 aGFuIGFsd2F5cyBkZXJlZmVyZW5jaW5nCisgICAgICAgIHRoZSBhZGRyZXNzIHJldHVybmVkIGJ5 IGdldFBpeGVscygpIChhcyB0aGUgZGV2aWNlIG1heSBub3QgYmUgYmFja2VkIGJ5IGEgQ1BVIGJp dG1hcCkKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTU0 MjI0CisKKyAgICAgICAgTm8gbmV3IHRlc3RzLiBFeGlzdGluZyBjYW52YXMgdGVzdHMgZXhlcmNp c2UgdGhpcy4uLgorICAgICAgICBmYXN0L2NhbnZhcy9zY3JpcHQtdGVzdHMvY2FudmFzLWNyZWF0 ZVBhdHRlcm4tZmlsbFJlY3Qtc2hhZG93Lmh0bWwKKworICAgICAgICAqIHBsYXRmb3JtL2dyYXBo aWNzL3NraWEvSW1hZ2VCdWZmZXJTa2lhLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkltYWdlQnVm ZmVyOjp0b0RhdGFVUkwpOgorCiAyMDExLTAyLTEwICBDaHJpcyBGbGVpemFjaCAgPGNmbGVpemFj aEBhcHBsZS5jb20+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgQW5kZXJzIENhcmxzc29uLgpJbmRl eDogV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9za2lhL0ltYWdlQnVmZmVyU2tpYS5jcHAKPT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PQotLS0gV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9za2lhL0ltYWdlQnVmZmVyU2tp YS5jcHAJKHJldmlzaW9uIDc4MTc4KQorKysgV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9za2lh L0ltYWdlQnVmZmVyU2tpYS5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTM0NiwxNCArMzQ2LDI3IEBA IFN0cmluZyBJbWFnZUJ1ZmZlcjo6dG9EYXRhVVJMKGNvbnN0IFN0cmkKICAgICBBU1NFUlQoTUlN RVR5cGVSZWdpc3RyeTo6aXNTdXBwb3J0ZWRJbWFnZU1JTUVUeXBlRm9yRW5jb2RpbmcobWltZVR5 cGUpKTsKIAogICAgIFZlY3Rvcjx1bnNpZ25lZCBjaGFyPiBlbmNvZGVkSW1hZ2U7CisgICAgU2tE ZXZpY2UqIGRldmljZSA9IGNvbnRleHQoKS0+cGxhdGZvcm1Db250ZXh0KCktPmNhbnZhcygpLT5n ZXREZXZpY2UoKTsKKyAgICBTa0JpdG1hcCBiaXRtYXAgPSBkZXZpY2UtPmFjY2Vzc0JpdG1hcChm YWxzZSk7CisKKyAgICAvLyBpZiB3ZSBjYW4ndCBzZWUgdGhlIHBpeGVscyBkaXJlY3RseSwgY2Fs bCByZWFkUGl4ZWxzKCkgdG8gZ2V0IGEgY29weS4KKyAgICAvLyB0aGlzIGNvdWxkIGhhcHBlbiBp ZiB0aGUgZGV2aWNlIGlzIGJhY2tlZCBieSBhIEdQVS4KKyAgICBiaXRtYXAubG9ja1BpeGVscygp OyAvLyBiYWxhbmNlZCBieSBvdXIgZGVzdHJ1Y3Rvciwgb3IgZXhwbGljaXRseSBpZiBnZXRQaXhl bHMoKSBmYWlscworICAgIGlmICghYml0bWFwLmdldFBpeGVscygpKSB7CisgICAgICAgIGJpdG1h cC51bmxvY2tQaXhlbHMoKTsKKyAgICAgICAgU2tJUmVjdCBib3VuZHMgPSBTa0lSZWN0OjpNYWtl V0goZGV2aWNlLT53aWR0aCgpLCBkZXZpY2UtPmhlaWdodCgpKTsKKyAgICAgICAgaWYgKCFkZXZp Y2UtPnJlYWRQaXhlbHMoYm91bmRzLCAmYml0bWFwKSkKKyAgICAgICAgICAgIHJldHVybiAiZGF0 YTosIjsKKyAgICB9CisgICAgCiAgICAgaWYgKG1pbWVUeXBlID09ICJpbWFnZS9qcGVnIikgewog ICAgICAgICBpbnQgY29tcHJlc3Npb25RdWFsaXR5ID0gSlBFR0ltYWdlRW5jb2Rlcjo6RGVmYXVs dENvbXByZXNzaW9uUXVhbGl0eTsKICAgICAgICAgaWYgKHF1YWxpdHkgJiYgKnF1YWxpdHkgPj0g MC4wICYmICpxdWFsaXR5IDw9IDEuMCkKICAgICAgICAgICAgIGNvbXByZXNzaW9uUXVhbGl0eSA9 IHN0YXRpY19jYXN0PGludD4oKnF1YWxpdHkgKiAxMDAgKyAwLjUpOwotICAgICAgICBpZiAoIUpQ RUdJbWFnZUVuY29kZXI6OmVuY29kZSgqY29udGV4dCgpLT5wbGF0Zm9ybUNvbnRleHQoKS0+Yml0 bWFwKCksIGNvbXByZXNzaW9uUXVhbGl0eSwgJmVuY29kZWRJbWFnZSkpCisgICAgICAgIGlmICgh SlBFR0ltYWdlRW5jb2Rlcjo6ZW5jb2RlKGJpdG1hcCwgY29tcHJlc3Npb25RdWFsaXR5LCAmZW5j b2RlZEltYWdlKSkKICAgICAgICAgICAgIHJldHVybiAiZGF0YTosIjsKICAgICB9IGVsc2Ugewot ICAgICAgICBpZiAoIVBOR0ltYWdlRW5jb2Rlcjo6ZW5jb2RlKCpjb250ZXh0KCktPnBsYXRmb3Jt Q29udGV4dCgpLT5iaXRtYXAoKSwgJmVuY29kZWRJbWFnZSkpCisgICAgICAgIGlmICghUE5HSW1h Z2VFbmNvZGVyOjplbmNvZGUoYml0bWFwLCAmZW5jb2RlZEltYWdlKSkKICAgICAgICAgICAgIHJl dHVybiAiZGF0YTosIjsKICAgICAgICAgQVNTRVJUKG1pbWVUeXBlID09ICJpbWFnZS9wbmciKTsK ICAgICB9Cg==