89506 2012-06-19 13:31:26 -0700 GraphicsContext::setCTM usage in SVGInlineTextBox::paintTextWithShadows breaks per-tile painting in chromium. 2012-06-28 12:49:36 -0700 1 1 1 Unclassified WebKit SVG 528+ (Nightly build) Unspecified Unspecified RESOLVED DUPLICATE 89888 P2 Normal --- 1 reveman reveman eric jamesr reed tomhudson webkit.review.bot zimmermann oldest_to_newest 652735 0 reveman 2012-06-19 13:31:26 -0700 GraphicsContext::save()/restore() should be used instead. 652740 1 reed 2012-06-19 13:36:50 -0700 or (possibly) void setMyMatrix(const SkMatrix& mat) { SkMatrix inverse; if (canvas->getTotalMatrix().invert(&inverse)) { canvas->concat(mat); canvas->concat(inverse); } } 652773 2 jamesr 2012-06-19 13:57:07 -0700 Why can't the picture record the CTM? 652782 3 148416 reveman 2012-06-19 14:02:34 -0700 Created attachment 148416 Patch 652790 4 reveman 2012-06-19 14:08:20 -0700 The attached patch just switches to using a save()/restore(). I'd prefer to just remove the "scalingFactor != 1" conditions completely but I guess that might cause a performance regression as I assume they are there for a reason. 652792 5 148416 reed 2012-06-19 14:09:28 -0700 Comment on attachment 148416 Patch looks better than the old code, and will work with pictures. nice. 652812 6 reveman 2012-06-19 14:24:21 -0700 (In reply to comment #2) > Why can't the picture record the CTM? We use a different translation offsets for each tile. setCTM playback will only be correct when the tile offset is 0,0 as it depends on the result from the getCTM call during recording. Maybe we should abort when getCTM is call during recording? 652847 7 148431 reveman 2012-06-19 14:51:20 -0700 Created attachment 148431 Patch Add a bit more information to the ChangeLog entry about why the getCTM/setCTM usage is causing breakage in chromium. 652863 8 148431 eric 2012-06-19 15:04:28 -0700 Comment on attachment 148431 Patch It's not immediately clear to me how to review this. I would expect there to be some performance impact to this change, no? Saving a context is expensive in many implementations, and this will make scaled text save the context for every paint of every text run? Is Chromium's tile-based model the future? Are other platforms going to see this sort of correctness concern? 652877 9 jamesr 2012-06-19 15:22:33 -0700 I think we should just make GraphicsContext::(set|get)CTM work with the skia implementation, or if we can't/won't do that see if we can remove it. I don't think it is acceptable to have an implementation of GraphicsContext where some functions simply do not work. This is separate from the question of whether the SVGInlineTextBox's implementation strategy is ideal or not. 652887 10 reveman 2012-06-19 15:32:44 -0700 (In reply to comment #9) > I think we should just make GraphicsContext::(set|get)CTM work with the skia implementation, or if we can't/won't do that see if we can remove it. I don't think it is acceptable to have an implementation of GraphicsContext where some functions simply do not work. > I'm with you on that. Maybe we could add a device offset to SkCanvas that effects all rendering just like an initial translation but is not included in the CTM. 652962 11 148431 jamesr 2012-06-19 16:39:40 -0700 Comment on attachment 148431 Patch R- for now - this isn't an obvious improvement or even obviously correct (there was some logic in normalizeTransform that is no longer called, although I'm not sure what it is all about). If all this wants to do is locally set a scale, could we just undo the scale by hand instead of save()/restore()ing the whole context? 652993 12 reveman 2012-06-19 16:56:07 -0700 (In reply to comment #11) > (From update of attachment 148431 [details]) > R- for now - this isn't an obvious improvement or even obviously correct (there was some logic in normalizeTransform that is no longer called, although I'm not sure what it is all about). > > If all this wants to do is locally set a scale, could we just undo the scale by hand instead of save()/restore()ing the whole context? It's a shame that save()/restore()ing is not lightweight on all platforms. I think it's more intuitive that optimizations such as integer translation can be tracked when save()/restore() pairs are used. 652997 13 jamesr 2012-06-19 16:57:52 -0700 I don't know, maybe it is in fact cheap. None of this really gets to the point of the patch, however. 653521 14 reed 2012-06-20 08:55:43 -0700 my read of the checkin when normalizeTransform() was added, was it was a concession to DRT to make images between 32bit CG and 64bit CG more the same. Not clear it "fixed" anything at the spec or user level, and was CG specific. For skia: save/scale/restore is by far the most compact and speed-efficient, matches the rest of webkit's usage pattern, and will work seamlessly in all of its backends. 655347 15 zimmermann 2012-06-22 03:47:11 -0700 (In reply to comment #11) > (From update of attachment 148431 [details]) > R- for now - this isn't an obvious improvement or even obviously correct (there was some logic in normalizeTransform that is no longer called, although I'm not sure what it is all about). > > If all this wants to do is locally set a scale, could we just undo the scale by hand instead of save()/restore()ing the whole context? The existing code only tried to avoid 32bit vs. 64bit differences that we saw in text rendering. Short story: it failed - normalizeTransform, as well as the whole setCTM() stuff, was a test, and it failed, and can be removed. 655392 16 reed 2012-06-22 06:00:36 -0700 sweet! Such a change may create a *lot* of rebaselines. Perhaps we can opt-in to that cleanup/change per platform, so as not to force it for platforms that don't want it (or don't care). I can post such a CL, unless David wants to. 655445 17 reveman 2012-06-22 07:32:56 -0700 (In reply to comment #16) > sweet! > > Such a change may create a *lot* of rebaselines. Perhaps we can opt-in to that cleanup/change per platform, so as not to force it for platforms that don't want it (or don't care). I can post such a CL, unless David wants to. Please go ahead and create such a CL if you like. 659712 18 reveman 2012-06-28 12:49:36 -0700 *** This bug has been marked as a duplicate of bug 89888 *** 148416 2012-06-19 14:02:34 -0700 2012-06-19 14:51:17 -0700 Patch bug-89506-20120619170233.patch text/plain 2360 reveman U3VidmVyc2lvbiBSZXZpc2lvbjogMTIwNzQwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMTk2NjlmNmYwZWJmNTZh NThmZmJmYzg3ZjFlY2Q3ZTllMGFkN2NkYS4uZDc1ODVmODk2YzExZmVhMzI4ZWNjZWI2OGMxZGE5 MGI5MDVmMGZjNiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE5IEBACisyMDEyLTA2LTE5ICBEYXZp ZCBSZXZlbWFuICA8cmV2ZW1hbkBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgR3JhcGhpY3NDb250 ZXh0OjpzZXRDVE0gdXNhZ2UgaW4gU1ZHSW5saW5lVGV4dEJveDo6cGFpbnRUZXh0V2l0aFNoYWRv d3MgYnJlYWtzIHBlci10aWxlIHBhaW50aW5nIGluIGNocm9taXVtLgorICAgICAgICBodHRwczov L2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9ODk1MDYKKworICAgICAgICBSZXZpZXdl ZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBVc2UgYSBzYXZlL3Jlc3RvcmUgcGFpciBm b3IgdGVtcG9yYXJ5IG1vZGlmaWNhdGlvbiB0byB0cmFuc2Zvcm1hdGlvbgorICAgICAgICBzdGF0 ZSBpbnN0ZWFkIG9mIGdldENUTS9zZXRDVE0uIGdldENUTS9zZXRDVE0gZG9lc24ndCBuZWNlc3Nh cmlseQorICAgICAgICBwcmVzZXJ2ZSB0aGUgZnVsbCB0cmFuc2Zvcm1hdGlvbiBzdGF0ZS4KKwor ICAgICAgICBUZXN0OiBzdmcvY3VzdG9tL2pzLWxhdGUtZ3JhZGllbnQtY3JlYXRpb24uc3ZnCisK KyAgICAgICAgKiByZW5kZXJpbmcvc3ZnL1NWR0lubGluZVRleHRCb3guY3BwOgorICAgICAgICAo V2ViQ29yZTo6U1ZHSW5saW5lVGV4dEJveDo6cGFpbnRUZXh0V2l0aFNoYWRvd3MpOgorCiAyMDEy LTA2LTE5ICBBYmhpc2hlayBBcnlhICA8aW5mZXJub0BjaHJvbWl1bS5vcmc+CiAKICAgICAgICAg Q3Jhc2ggaW4gUmVuZGVySW5saW5lOjp3aWxsQmVEZXN0cm95ZWQuCmRpZmYgLS1naXQgYS9Tb3Vy Y2UvV2ViQ29yZS9yZW5kZXJpbmcvc3ZnL1NWR0lubGluZVRleHRCb3guY3BwIGIvU291cmNlL1dl YkNvcmUvcmVuZGVyaW5nL3N2Zy9TVkdJbmxpbmVUZXh0Qm94LmNwcAppbmRleCA5MzAyYWVmODAy NzkwMmRjNzQ4OTM5NWIxZGE3Y2QzMjA0Mjk2M2IzLi5iOWY3YWFjODVmZTgzYmY1MDVmYjM4NzI5 ZTdmODlkYzUxNDRjNDQ2IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvc3Zn L1NWR0lubGluZVRleHRCb3guY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9zdmcv U1ZHSW5saW5lVGV4dEJveC5jcHAKQEAgLTY0MywyMSArNjQzLDE1IEBAIHZvaWQgU1ZHSW5saW5l VGV4dEJveDo6cGFpbnRUZXh0V2l0aFNoYWRvd3MoR3JhcGhpY3NDb250ZXh0KiBjb250ZXh0LCBS ZW5kZXJTdHlsCiAgICAgICAgIGlmIChzaGFkb3cpCiAgICAgICAgICAgICBleHRyYU9mZnNldCA9 IGFwcGx5U2hhZG93VG9HcmFwaGljc0NvbnRleHQoY29udGV4dCwgc2hhZG93LCBzaGFkb3dSZWN0 LCBmYWxzZSAvKiBzdHJva2VkICovLCB0cnVlIC8qIG9wYXF1ZSAqLywgdHJ1ZSAvKiBob3Jpem9u dGFsICovKTsKIAotICAgICAgICBBZmZpbmVUcmFuc2Zvcm0gb3JpZ2luYWxUcmFuc2Zvcm07CiAg ICAgICAgIGlmIChzY2FsaW5nRmFjdG9yICE9IDEpIHsKLSAgICAgICAgICAgIG9yaWdpbmFsVHJh bnNmb3JtID0gY29udGV4dC0+Z2V0Q1RNKCk7Ci0KLSAgICAgICAgICAgIEFmZmluZVRyYW5zZm9y bSBuZXdUcmFuc2Zvcm0gPSBvcmlnaW5hbFRyYW5zZm9ybTsKLSAgICAgICAgICAgIG5ld1RyYW5z Zm9ybS5zY2FsZSgxIC8gc2NhbGluZ0ZhY3Rvcik7Ci0gICAgICAgICAgICBub3JtYWxpemVUcmFu c2Zvcm0obmV3VHJhbnNmb3JtKTsKLQotICAgICAgICAgICAgY29udGV4dC0+c2V0Q1RNKG5ld1Ry YW5zZm9ybSk7CisgICAgICAgICAgICBjb250ZXh0LT5zYXZlKCk7CisgICAgICAgICAgICBjb250 ZXh0LT5zY2FsZShGbG9hdFNpemUoMSAvIHNjYWxpbmdGYWN0b3IsIDEgLyBzY2FsaW5nRmFjdG9y KSk7CiAgICAgICAgIH0KIAogICAgICAgICBzY2FsZWRGb250LmRyYXdUZXh0KGNvbnRleHQsIHRl eHRSdW4sIHRleHRPcmlnaW4gKyBleHRyYU9mZnNldCwgc3RhcnRQb3NpdGlvbiwgZW5kUG9zaXRp b24pOwogCiAgICAgICAgIGlmIChzY2FsaW5nRmFjdG9yICE9IDEpCi0gICAgICAgICAgICBjb250 ZXh0LT5zZXRDVE0ob3JpZ2luYWxUcmFuc2Zvcm0pOworICAgICAgICAgICAgY29udGV4dC0+cmVz dG9yZSgpOwogCiAgICAgICAgIHJlc3RvcmVHcmFwaGljc0NvbnRleHRBZnRlclRleHRQYWludGlu Zyhjb250ZXh0LCB0ZXh0UnVuKTsKIAo= 148431 2012-06-19 14:51:20 -0700 2012-06-19 16:39:40 -0700 Patch bug-89506-20120619175120.patch text/plain 2693 reveman U3VidmVyc2lvbiBSZXZpc2lvbjogMTIwNzQwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMTk2NjlmNmYwZWJmNTZh NThmZmJmYzg3ZjFlY2Q3ZTllMGFkN2NkYS4uYTIzNGM5MDM3NDc4ZTU4ZWUxNDhjY2M2NDJkZGJj YjQ5NmFmNDZiZCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDI1IEBACisyMDEyLTA2LTE5ICBEYXZp ZCBSZXZlbWFuICA8cmV2ZW1hbkBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgR3JhcGhpY3NDb250 ZXh0OjpzZXRDVE0gdXNhZ2UgaW4gU1ZHSW5saW5lVGV4dEJveDo6cGFpbnRUZXh0V2l0aFNoYWRv d3MgYnJlYWtzIHBlci10aWxlIHBhaW50aW5nIGluIGNocm9taXVtLgorICAgICAgICBodHRwczov L2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9ODk1MDYKKworICAgICAgICBSZXZpZXdl ZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBVc2UgYSBzYXZlL3Jlc3RvcmUgcGFpciBm b3IgdGVtcG9yYXJ5IG1vZGlmaWNhdGlvbiB0byB0cmFuc2Zvcm1hdGlvbgorICAgICAgICBzdGF0 ZSBpbnN0ZWFkIG9mIGdldENUTS9zZXRDVE0uCisKKyAgICAgICAgQ2hyb21pdW0ncyBwZXItdGls ZSBwYWludGluZyBzeXN0ZW0gcmVjb3JkcyBhbGwgcmVuZGVyaW5nIGNvbW1hbmRzIGluCisgICAg ICAgIGEgU2tQaWN0dXJlLiBUaGUgU2tQaWN0dXJlIGlzIGxhdGVyIHBsYXllZCBiYWNrIHdpdGgg YSBkaWZmZXJlbnQgY2xpcAorICAgICAgICBhbmQgaW5pdGlhbCB0cmFuc2Zvcm1hdGlvbiBzdGF0 ZS4gQW55IHJlbmRlcmluZyBjb2RlIHRoYXQgZGVwZW5kcyBvbgorICAgICAgICB0aGUgbWF0cml4 IHJldHVybmVkIGJ5IGdldENUTSB3aWxsIG5vdCB3b3JrIGNvcnJlY3RseSBpbiB0aGlzIHN5c3Rl bQorICAgICAgICBhcyBpdCB3aWxsIGJlIHVuYWZmZWN0ZWQgYnkgdGhlIHRoZSBpbml0aWFsIHRy YW5zZm9ybWF0aW9uIHN0YXRlIHNldAorICAgICAgICBhdCBwbGF5YmFjay4KKworICAgICAgICBU ZXN0OiBzdmcvY3VzdG9tL2pzLWxhdGUtZ3JhZGllbnQtY3JlYXRpb24uc3ZnCisKKyAgICAgICAg KiByZW5kZXJpbmcvc3ZnL1NWR0lubGluZVRleHRCb3guY3BwOgorICAgICAgICAoV2ViQ29yZTo6 U1ZHSW5saW5lVGV4dEJveDo6cGFpbnRUZXh0V2l0aFNoYWRvd3MpOgorCiAyMDEyLTA2LTE5ICBB Ymhpc2hlayBBcnlhICA8aW5mZXJub0BjaHJvbWl1bS5vcmc+CiAKICAgICAgICAgQ3Jhc2ggaW4g UmVuZGVySW5saW5lOjp3aWxsQmVEZXN0cm95ZWQuCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29y ZS9yZW5kZXJpbmcvc3ZnL1NWR0lubGluZVRleHRCb3guY3BwIGIvU291cmNlL1dlYkNvcmUvcmVu ZGVyaW5nL3N2Zy9TVkdJbmxpbmVUZXh0Qm94LmNwcAppbmRleCA5MzAyYWVmODAyNzkwMmRjNzQ4 OTM5NWIxZGE3Y2QzMjA0Mjk2M2IzLi5iOWY3YWFjODVmZTgzYmY1MDVmYjM4NzI5ZTdmODlkYzUx NDRjNDQ2IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvc3ZnL1NWR0lubGlu ZVRleHRCb3guY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9zdmcvU1ZHSW5saW5l VGV4dEJveC5jcHAKQEAgLTY0MywyMSArNjQzLDE1IEBAIHZvaWQgU1ZHSW5saW5lVGV4dEJveDo6 cGFpbnRUZXh0V2l0aFNoYWRvd3MoR3JhcGhpY3NDb250ZXh0KiBjb250ZXh0LCBSZW5kZXJTdHls CiAgICAgICAgIGlmIChzaGFkb3cpCiAgICAgICAgICAgICBleHRyYU9mZnNldCA9IGFwcGx5U2hh ZG93VG9HcmFwaGljc0NvbnRleHQoY29udGV4dCwgc2hhZG93LCBzaGFkb3dSZWN0LCBmYWxzZSAv KiBzdHJva2VkICovLCB0cnVlIC8qIG9wYXF1ZSAqLywgdHJ1ZSAvKiBob3Jpem9udGFsICovKTsK IAotICAgICAgICBBZmZpbmVUcmFuc2Zvcm0gb3JpZ2luYWxUcmFuc2Zvcm07CiAgICAgICAgIGlm IChzY2FsaW5nRmFjdG9yICE9IDEpIHsKLSAgICAgICAgICAgIG9yaWdpbmFsVHJhbnNmb3JtID0g Y29udGV4dC0+Z2V0Q1RNKCk7Ci0KLSAgICAgICAgICAgIEFmZmluZVRyYW5zZm9ybSBuZXdUcmFu c2Zvcm0gPSBvcmlnaW5hbFRyYW5zZm9ybTsKLSAgICAgICAgICAgIG5ld1RyYW5zZm9ybS5zY2Fs ZSgxIC8gc2NhbGluZ0ZhY3Rvcik7Ci0gICAgICAgICAgICBub3JtYWxpemVUcmFuc2Zvcm0obmV3 VHJhbnNmb3JtKTsKLQotICAgICAgICAgICAgY29udGV4dC0+c2V0Q1RNKG5ld1RyYW5zZm9ybSk7 CisgICAgICAgICAgICBjb250ZXh0LT5zYXZlKCk7CisgICAgICAgICAgICBjb250ZXh0LT5zY2Fs ZShGbG9hdFNpemUoMSAvIHNjYWxpbmdGYWN0b3IsIDEgLyBzY2FsaW5nRmFjdG9yKSk7CiAgICAg ICAgIH0KIAogICAgICAgICBzY2FsZWRGb250LmRyYXdUZXh0KGNvbnRleHQsIHRleHRSdW4sIHRl eHRPcmlnaW4gKyBleHRyYU9mZnNldCwgc3RhcnRQb3NpdGlvbiwgZW5kUG9zaXRpb24pOwogCiAg ICAgICAgIGlmIChzY2FsaW5nRmFjdG9yICE9IDEpCi0gICAgICAgICAgICBjb250ZXh0LT5zZXRD VE0ob3JpZ2luYWxUcmFuc2Zvcm0pOworICAgICAgICAgICAgY29udGV4dC0+cmVzdG9yZSgpOwog CiAgICAgICAgIHJlc3RvcmVHcmFwaGljc0NvbnRleHRBZnRlclRleHRQYWludGluZyhjb250ZXh0 LCB0ZXh0UnVuKTsKIAo=